Address Benjie's review feedback#56
Conversation
- Add explicit ref: main to checkout step (prevents config poisoning) - Include commit hash in wrangler deploy message for traceability - Extract hide-old-comments logic to .github/scripts/hide-old-deploy-comments.mjs - Add .github/scripts to sparse-checkout so the extracted script is available - Add security comments documenting zip-slip safety and static-only wrangler model
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
gaps | e680187 | Commit Preview URL Branch Preview URL |
Jul 14 2026, 06:30 AM |
| apiToken: ${{ secrets.CLOUDFLARE_API_TOKEN }} | ||
| accountId: ${{ secrets.CLOUDFLARE_ACCOUNT_ID }} | ||
| command: versions upload --preview-alias "pr-${{ steps.pr.outputs.number }}" --message "PR #${{ steps.pr.outputs.number }} preview" | ||
| command: versions upload --preview-alias "pr-${{ steps.pr.outputs.number }}" --message "PR #${{ steps.pr.outputs.number }} preview (${{ github.event.workflow_run.head_sha }})" |
There was a problem hiding this comment.
Does wranger versions upload do a pure upload, or might it read any configuration/run any scripts/be influenced by any content from within _site?
There was a problem hiding this comment.
My claude is very confident this is safe:
For an assets-only config (no main), wrangler versions upload --no-bundle does not execute anything from _site.
- Config — Wrangler resolves config (
wrangler.jsonc) by starting from the current working directory and walking upward; it does not search downward into_site. - Entry — because the config has
assetsand nomain,getEntry()takes the assets branch and resolves the Worker entry to Wrangler’s own internaltemplates/no-op-worker.js, not anything
from_site. - Custom build — skipped here because the config has no
build.command. - Bundling —
--no-bundleskips esbuild/bundling; Wrangler just packages/copies the internal no-op Worker entry. - Asset upload — uploads files from
_siteas static assets to Cloudflare's CDN
So no, code from _site is not executed by Wrangler. The files in _site are used only as static asset content for the upload.
Relevant source: resolveEntryWithAssets() returns the internal no-op template when config has assets but no main, and getEntry() hits the args.assets || config.assets branch.
...and I had codex confirm it too:
Confirmed: the security substance is right. For an assets-only Wrangler config with no main and no build.command, _site is read as asset content, not as config, an entrypoint, or executable build input. getEntry() reaches the args.assets || config.assets branch, resolveEntryWithAssets() resolves the Worker entry to Wrangler’s internal templates/no-op-worker.js, runCustomBuild() only runs a command when build.command exists, and --no-bundle takes the no-bundle path instead of bundleWorker()/esbuild.
this isn't explicitly clarified in docs that i can find, so without fully auditing and pentesting I can't be 100% sure, but this passes the smell test to me. wdyt?
- Use github.paginate to find deploy-preview comments beyond the first 30 results - Filter by github-actions[bot] author to avoid hiding human comments that quote the deploy-preview marker Co-Authored-By: Claude <noreply@anthropic.com>
The assets-only config already resolves to wrangler's internal no-op template, so bundling is unnecessary. Skipping it removes any ambiguity about whether esbuild could be influenced by _site contents. Co-Authored-By: Claude <noreply@anthropic.com>
2 participants
Addresses the review comments from benjie on #40:
ref: mainon checkout step — makes it explicit we're pulling trusted config from main, not from the PR branch(${{ github.event.workflow_run.head_sha }})for traceability.github/scripts/hide-old-deploy-comments.mjs(also adds.github/scriptsto sparse-checkout so it's available at runtime)actions/download-artifacthandles extraction safelyAll changes verified end-to-end on magicmark/gaps-website-test-1 with both fork and non-fork PRs deploying successfully to Cloudflare Workers.