Enable deploy previews via cloudflare#40
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
First half of a secure fork-safe deploy preview setup. This workflow runs on all PRs (including forks), builds the site with no access to secrets, and uploads the artifact. A separate workflow_run-triggered workflow will handle the privileged Cloudflare deploy. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Second half of the secure fork-safe deploy preview setup. Downloads the build artifact from the unprivileged build workflow and deploys to Cloudflare Pages. Has access to secrets but never executes fork code. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ssions - Fix critical bug: pull_requests[] is empty for fork PRs in workflow_run events. Save PR number as artifact metadata instead. - Add actions: read permission so download-artifact can fetch cross-workflow - Update action versions to latest: checkout@v6, upload-artifact@v7, download-artifact@v8, wrangler-action@v4 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Pin all actions to commit SHAs (supply-chain protection) - Replace artifact-based PR number with gh pr list --head lookup (prevents PR number spoofing from untrusted artifacts) - Remove "Save PR metadata" step from build (no longer needed) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
benjie
left a comment
There was a problem hiding this comment.
I don’t know much about the security model of workflow completed, I’ll need to read up. Seems a promising direction!
| with: | ||
| 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" |
There was a problem hiding this comment.
Maybe include the commit has in the description?
There was a problem hiding this comment.
Can you confirm that this is a simple upload where under no circumstances is any code ran, config files read, etc from within _site?
| name: preview-build | ||
| run-id: ${{ github.event.workflow_run.id }} | ||
| github-token: ${{ secrets.GITHUB_TOKEN }} | ||
| path: _site |
There was a problem hiding this comment.
AFAIK, this downloads a tarball or similar and extracts it to that folder. There have been vulnerabilities in the past where a maliciously structured tarball has resulted in the extractor writing files outside of the target directory, are we certain we’re not at risk of this?
There was a problem hiding this comment.
looks like actions/download-artifact has a test for this on extract:
evil.zip contains files that are formatted x/../../etc/hosts
Tested here:
There was a problem hiding this comment.
Relies on this logic from the unzip-stream dependency:
- 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
- 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>
Addresses the review comments from benjie on #40: 1. **`ref: main`** on checkout step — makes it explicit we're pulling trusted config from main, not from the PR branch 2. **Commit hash in deploy message** — adds `(${{ github.event.workflow_run.head_sha }})` for traceability 3. **Extract inline JS** — moves the hide-old-comments logic to `.github/scripts/hide-old-deploy-comments.mjs` (also adds `.github/scripts` to sparse-checkout so it's available at runtime) 4. **Security: outside bounds zip extraction** — documents that `actions/download-artifact` handles extraction safely 5. **Security: static-only wrangler** — documents that wrangler is configured for static asset serving only All changes verified end-to-end on [magicmark/gaps-website-test-1](https://github.com/magicmark/gaps-website-test-1) with both fork and non-fork PRs deploying successfully to Cloudflare Workers.
- actions/checkout v6 → v7 - actions/setup-node v6 → v7 - actions/github-script v7 → v9 Co-Authored-By: Claude <noreply@anthropic.com>
Update deploy preview workflows to match graphql/gaps#40
- Upgrade actions/checkout to v7, actions/setup-node to v7, actions/github-script to v9 - Add --no-bundle flag to wrangler deploy command - Use paginate() and user.login filter in hide-old-deploy-comments script - Remove pages.yml (replaced by Cloudflare deploy) Co-Authored-By: Claude <noreply@anthropic.com>
The wrangler-action splits the `command` input by newlines and runs each as a separate wrangler invocation, so the multiline >- scalar doesn't work as expected here. Put the command on a single line. Co-Authored-By: Claude <noreply@anthropic.com>
@benjie for your consideration...
Turns out Cloudflare Pages / Workers cannot be run auto-run on fork-PRs.
(Vercel gives us this out the box. This PR is basically "we have vercel deploy previews at home".)
Flow + Security
This is set up as two actions:
on: pull_request) - to build the app + upload artifacton: workflow_run) - fires whenever the pull_request job completes, and deploys the site to a subdomainPrior art: https://docs.github.com/en/actions/reference/workflows-and-actions/events-that-trigger-workflows#using-data-from-the-triggering-workflow
(1)
on: pull_requestactions run untrusted code. This file can be modified by PRs, do anything - yikes. Luckily, this only gets a RO Github API token (which the public already implicitly has). This cannot be modified.(2)
on: workflow_runactions execute the workflow definition frommainbranch (not the PR branch). It does have access to the Cloudflare Token, but we're only running trusted code here. Sadly it doesn't look like cloudflare lets you more scope tokens to specific projects, it's org-wide.Another layer of protection we already have set up in https://github.com/graphql/graphql.github.io is that workflows should not run automatically on fork PRs. A maintainer has to click a button to see a deploy preview.
This would be nice, but conflicts with "editors can merge their own edits to GAPs" philosophy. I think we have to disable this if we want to encourage easy and frictionless edits to GAPs. More specifically, there is an option to only make someone click a button for first-time contributors, which could be a nice middleground and prevents drive-bys.
Turning off Cloudflare's Github integration
Note: I would recommend removing the git integration in the cloudflare dashboard to avoid double-commenting deploy preview urls on non-fork prs.
Should we ship this?
🤷 idk all that yaml ain't pretty, but it does provide some real value.
If you like this approach, I can package this up as reusable action and we can reuse it across our various websites (
graphql/website-deploy-previewor whatever)On the other hand, if you want to stick with vercel, or abandon non-fork prs altogether that's fine too.
Validation / Example PRs:
Tested this all against a copy of this repo here: https://github.com/magicmark/gaps-website-test-1 so you can see what it looks like: