Draft: Add mechanism to manually run tests on PRs from forks#8227
Draft: Add mechanism to manually run tests on PRs from forks#8227
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
@clerk/agent-toolkit
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/dev-cli
@clerk/expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/hono
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/react
@clerk/react-router
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/ui
@clerk/upgrade
@clerk/vue
commit: |
| types: [labeled, synchronize] | ||
| branches: | ||
| - main | ||
| # For testing the workflow, remove before merging |
There was a problem hiding this comment.
Adding a comment so I don't forget.
There was a problem hiding this comment.
There's plenty of opportunity to de-duplicate parts of this with ci.yml later if we want, but it made sense to me to keep them separate for now to allow for easier reviewing and testing.
| fi | ||
| gh api "repos/$REPO/statuses/$SHA" \ | ||
| -f state="$STATE" \ | ||
| -f context="integration-tests" \ |
There was a problem hiding this comment.
These gates in the two files post to the same context, that way we can add that as a required check and it will pass if either of the jobs pass.
📝 WalkthroughWalkthroughTwo GitHub Actions workflow files are modified to implement fork integration testing. A new workflow file 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)
296-338: 🧹 Nitpick | 🔵 TrivialExtract the integration matrix from a shared source.
The new note is helpful, but keeping this list in sync by hand with
.github/workflows/ci-fork-integration.ymlis brittle. One missed edit will make fork PRs and same-repo PRs run different required suites.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 296 - 338, The hard-coded integration test matrix in the "matrix" block (keys test-name, test-project, include) should be moved to a single shared source so both workflows reference the same canonical list; create a new shared matrix file or make ci-fork-integration.yml the single source-of-truth and update this workflow to import/consume that shared matrix (via a reusable workflow call or other supported include mechanism) and remove the duplicated arrays here, ensuring the same keys (test-name, test-project, include, next-version) are preserved so tests run identically across ci.yml and ci-fork-integration.yml.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci-fork-integration.yml:
- Around line 237-253: The workflow currently logs the secret via the
console.log('rootCa', rootCa) statement; remove that console.log so the
INTEGRATION_ROOT_CA value is never emitted to workflow logs, keeping the rest of
the script that reads process.env.INTEGRATION_ROOT_CA and writes files using
fs.writeFileSync(path.join(process.env.GITHUB_WORKSPACE, 'integration/certs',
'rootCA.pem'), rootCa) intact.
- Around line 8-13: The pull_request_target workflow's branches filter currently
only lists main plus a temporary fredrik/fork-integration-tests and therefore
diverges from .github/workflows/ci.yml; update the pull_request_target trigger
(the types/branches block) to match ci.yml by including the same base branches
(main, release/v4, release/core-2) and remove the temporary
fredrik/fork-integration-tests entry so fork PRs against release branches get
the same manual integration path as ci.yml.
- Around line 258-273: The env var E2E_CLERK_ENCRYPTION_KEY in the "Run
Integration Tests" job is populated from matrix.clerk-encryption-key but that
matrix key does not exist; either add clerk-encryption-key to the matrix/include
entries (set a value per matrix entry) so matrix.clerk-encryption-key resolves,
or remove E2E_CLERK_ENCRYPTION_KEY from the env block; apply the same fix to the
corresponding job in ci.yml to keep both workflows consistent (search for "Run
Integration Tests" or the job id integration-tests and the env name
E2E_CLERK_ENCRYPTION_KEY).
- Around line 51-58: Replace the mutable tag for the GitHub Action used in the
checkAccess step with an immutable commit SHA: change uses:
actions-cool/check-user-permission@v2 to uses:
actions-cool/check-user-permission@<40-char-commit-sha> (keep the step id
"checkAccess" and other fields unchanged) and add an inline comment above or
beside the uses line noting the original version tag for reference (e.g., #
v2.x.x).
- Around line 144-168: The job currently runs the init-blacksmith action
(init-blacksmith) which exports secrets into the environment, then performs the
fork checkout step (actions/checkout@v4 with clean: false) and runs pnpm
install; to prevent leaked secrets, run the fork checkout in an isolated
context: either move the fork checkout and subsequent commands (the "Checkout
fork PR (SHA-pinned)" step and "Re-install dependencies for fork code"/pnpm
install) into a separate job that does NOT call init-blacksmith, or perform the
checkout into a separate workspace/path and explicitly clear/unset exported
variables (unset TURBO_TOKEN, TURBO_REMOTE_CACHE_SIGNATURE_KEY and any other
exported envs from init-blacksmith) before running pnpm install; also remove or
change clean: false so the checkout does not inherit the runner workspace state
if keeping the same job.
---
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 296-338: The hard-coded integration test matrix in the "matrix"
block (keys test-name, test-project, include) should be moved to a single shared
source so both workflows reference the same canonical list; create a new shared
matrix file or make ci-fork-integration.yml the single source-of-truth and
update this workflow to import/consume that shared matrix (via a reusable
workflow call or other supported include mechanism) and remove the duplicated
arrays here, ensuring the same keys (test-name, test-project, include,
next-version) are preserved so tests run identically across ci.yml and
ci-fork-integration.yml.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 294fd072-76b5-49db-8b1c-15e82428cff0
📒 Files selected for processing (2)
.github/workflows/ci-fork-integration.yml.github/workflows/ci.yml
| pull_request_target: | ||
| types: [labeled, synchronize] | ||
| branches: | ||
| - main | ||
| # For testing the workflow, remove before merging | ||
| - fredrik/fork-integration-tests |
There was a problem hiding this comment.
Match ci.yml's base-branch filter before merging.
.github/workflows/ci.yml lines 7-13 still handle main, release/v4, and release/core-2, but this privileged workflow only listens on main plus a temporary fredrik/fork-integration-tests branch. Fork PRs against the release branches will never get the manual integration path, while the test branch widens the pull_request_target surface.
🔧 Suggested change
pull_request_target:
types: [labeled, synchronize]
branches:
- main
- # For testing the workflow, remove before merging
- - fredrik/fork-integration-tests
+ - release/v4
+ - release/core-2📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pull_request_target: | |
| types: [labeled, synchronize] | |
| branches: | |
| - main | |
| # For testing the workflow, remove before merging | |
| - fredrik/fork-integration-tests | |
| pull_request_target: | |
| types: [labeled, synchronize] | |
| branches: | |
| - main | |
| - release/v4 | |
| - release/core-2 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci-fork-integration.yml around lines 8 - 13, The
pull_request_target workflow's branches filter currently only lists main plus a
temporary fredrik/fork-integration-tests and therefore diverges from
.github/workflows/ci.yml; update the pull_request_target trigger (the
types/branches block) to match ci.yml by including the same base branches (main,
release/v4, release/core-2) and remove the temporary
fredrik/fork-integration-tests entry so fork PRs against release branches get
the same manual integration path as ci.yml.
There was a problem hiding this comment.
The test branch will be removed before merging. It was a conscious decision to only include main. External contributions should always happen towards main, we backport any necessary changes ourselves. This makes the fork workflow easier to reason about as we only need to make sure the main branch workflows are safe, not the older branch workflows.
There was a problem hiding this comment.
@Ephem, that makes perfect sense — limiting the privileged pull_request_target trigger to main is a sound policy both for security and for keeping the threat-model simple. Thanks for the context!
✏️ Learnings added
Learnt from: Ephem
Repo: clerk/javascript PR: 8227
File: .github/workflows/ci-fork-integration.yml:8-13
Timestamp: 2026-04-02T15:20:10.227Z
Learning: In the `clerk/javascript` repository, external fork contributions are always made against `main` only. Backports to release branches (e.g. `release/v4`, `release/core-2`) are handled internally. Therefore, the `pull_request_target` trigger in `.github/workflows/ci-fork-integration.yml` intentionally only lists `main` in its `branches` filter — do not suggest adding release branches to this workflow.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| - name: Verify label sender has write access | ||
| id: checkAccess | ||
| uses: actions-cool/check-user-permission@v2 | ||
| with: | ||
| require: write | ||
| username: ${{ github.event.sender.login }} | ||
| env: | ||
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files .github/workflows/ | head -20Repository: clerk/javascript
Length of output: 494
🏁 Script executed:
cat -n .github/workflows/ci-fork-integration.yml | head -80Repository: clerk/javascript
Length of output: 3546
🏁 Script executed:
rg "actions-cool/check-user-permission" .github/workflows/Repository: clerk/javascript
Length of output: 230
🏁 Script executed:
rg "@v[0-9]+" .github/workflows/ | head -30Repository: clerk/javascript
Length of output: 2253
🌐 Web query:
GitHub Actions pinning to commit SHA security best practice pull_request_target
💡 Result:
Yes, pinning GitHub Actions to full-length commit SHAs is a security best practice, especially critical for workflows using the pull_request_target trigger. Pinning to commit SHAs creates immutable references, preventing supply chain attacks where mutable tags like @v4 or @main are hijacked to point to malicious code. GitHub official documentation explicitly recommends: "Pin actions to a full-length commit SHA. Pinning an action to a full-length commit SHA is currently the only way to use an action as an immutable release." This mitigates risks from bad actors adding backdoors, as they would need a SHA-1 collision. For pull_request_target specifically, this trigger runs workflows in the base repository context with access to secrets and write permissions (via GITHUB_TOKEN), even for forks. Without pinning, untrusted actions could execute malicious code with elevated privileges, leading to RCE, secret exfiltration, or repository compromise. Best practices include: - Always pin to full SHA (40 characters), e.g., uses: actions/checkout@692973e # v4.1.7 - Verify SHA from the official repository (not forks) - Add comments for readability (e.g., # v4.1.7) - Use GitHub policies to enforce SHA pinning at repo/org level - For pull_request_target: Never checkout untrusted PR code (github.event.pull_request.head.sha); use base ref only. Restrict to trusted repos/forks, limit permissions to read-only, split workflows (unprivileged pull_request -> privileged via artifacts). Tools like pin-github-action or StepSecurity automate pinning. GitHub provides org/repo policies for enforcement and Dependabot for updates. Recent changes (2025) force pull_request_target to use default branch workflows, aiding fixes but not eliminating need for pinning.
Citations:
- 1: https://docs.github.com/en/actions/reference/security/secure-use
- 2: https://docs.github.com/en/actions/security-for-github-actions/security-guides/using-githubs-security-features-to-secure-your-use-of-github-actions
- 3: https://www.stepsecurity.io/blog/pinning-github-actions-for-enhanced-security-a-complete-guide
- 4: https://www.codewrecks.com/post/github/github-sha-pinning/
- 5: https://securitylab.github.com/resources/github-actions-new-patterns-and-mitigations/
- 6: https://github.blog/changelog/2025-11-07-actions-pull_request_target-and-environment-branch-protections-changes
- 7: https://github.blog/changelog/2025-08-15-github-actions-policy-now-supports-blocking-and-sha-pinning-actions/
Pin actions-cool/check-user-permission to a full commit SHA.
This step gates a pull_request_target workflow (which runs with elevated privileges and repo credentials) using a mutable third-party tag. GitHub security guidance requires pinning actions to immutable commit SHAs to prevent supply chain attacks. Replace @v2 with the full 40-character commit SHA and add a comment noting the version for reference (e.g., # v2.x.x).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci-fork-integration.yml around lines 51 - 58, Replace the
mutable tag for the GitHub Action used in the checkAccess step with an immutable
commit SHA: change uses: actions-cool/check-user-permission@v2 to uses:
actions-cool/check-user-permission@<40-char-commit-sha> (keep the step id
"checkAccess" and other fields unchanged) and add an inline comment above or
beside the uses line noting the original version tag for reference (e.g., #
v2.x.x).
| - name: Setup (from base branch) | ||
| id: config | ||
| uses: ./.github/actions/init-blacksmith | ||
| with: | ||
| turbo-signature: ${{ secrets.TURBO_REMOTE_CACHE_SIGNATURE_KEY }} | ||
| turbo-team: ${{ vars.TURBO_TEAM }} | ||
| turbo-token: ${{ secrets.TURBO_TOKEN }} | ||
| playwright-enabled: true | ||
|
|
||
| # Phase 2: Overlay the fork code at the exact SHA frozen in the event payload. | ||
| # This SHA was captured when the label was applied and cannot be changed by | ||
| # subsequent pushes to the fork branch. | ||
| - name: Checkout fork PR (SHA-pinned) | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| repository: ${{ github.event.pull_request.head.repo.full_name }} | ||
| ref: ${{ github.event.pull_request.head.sha }} | ||
| fetch-depth: 1 | ||
| fetch-tags: false | ||
| filter: 'blob:none' | ||
| show-progress: false | ||
| clean: false | ||
|
|
||
| - name: Re-install dependencies for fork code | ||
| run: pnpm install |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's locate and examine the init-blacksmith action
find . -path "*/.github/actions/init-blacksmith/action.yml" -type fRepository: clerk/javascript
Length of output: 104
🏁 Script executed:
#!/bin/bash
# Also check the structure of .github directory
ls -la .github/actions/ 2>/dev/null || echo "actions directory not found"Repository: clerk/javascript
Length of output: 415
🏁 Script executed:
#!/bin/bash
# Let's look at the ci-fork-integration.yml file in context
head -200 .github/workflows/ci-fork-integration.yml | tail -100Repository: clerk/javascript
Length of output: 3571
🏁 Script executed:
#!/bin/bash
# Examine the init-blacksmith action
cat .github/actions/init-blacksmith/action.ymlRepository: clerk/javascript
Length of output: 4592
🏁 Script executed:
#!/bin/bash
# Let's verify the context around the fork checkout and see if there are any mitigation measures
grep -A 30 "Checkout fork PR" .github/workflows/ci-fork-integration.yml | head -40Repository: clerk/javascript
Length of output: 1100
🏁 Script executed:
#!/bin/bash
# Let's check if there are any other security-related configurations in the workflow
grep -i "secret\|credential\|token\|clean" .github/workflows/ci-fork-integration.ymlRepository: clerk/javascript
Length of output: 777
Run the fork checkout in a fresh workspace.
The init-blacksmith action exports TURBO_TOKEN, TURBO_REMOTE_CACHE_SIGNATURE_KEY, and other environment variables via core.exportVariable(), which persist throughout the job. With clean: false on the fork checkout, these variables—including secrets—remain accessible to the untrusted fork code. Fork scripts (like pnpm install postinstall hooks) can access and exfiltrate TURBO_TOKEN. Use a separate job or checkout path and clear credentials before checking out fork code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci-fork-integration.yml around lines 144 - 168, The job
currently runs the init-blacksmith action (init-blacksmith) which exports
secrets into the environment, then performs the fork checkout step
(actions/checkout@v4 with clean: false) and runs pnpm install; to prevent leaked
secrets, run the fork checkout in an isolated context: either move the fork
checkout and subsequent commands (the "Checkout fork PR (SHA-pinned)" step and
"Re-install dependencies for fork code"/pnpm install) into a separate job that
does NOT call init-blacksmith, or perform the checkout into a separate
workspace/path and explicitly clear/unset exported variables (unset TURBO_TOKEN,
TURBO_REMOTE_CACHE_SIGNATURE_KEY and any other exported envs from
init-blacksmith) before running pnpm install; also remove or change clean: false
so the checkout does not inherit the runner workspace state if keeping the same
job.
| - name: Write all ENV certificates to files in integration/certs | ||
| uses: actions/github-script@v7 | ||
| env: | ||
| INTEGRATION_CERTS: '${{secrets.INTEGRATION_CERTS}}' | ||
| INTEGRATION_ROOT_CA: '${{secrets.INTEGRATION_ROOT_CA}}' | ||
| with: | ||
| script: | | ||
| const fs = require('fs'); | ||
| const path = require('path'); | ||
| const rootCa = process.env.INTEGRATION_ROOT_CA; | ||
| console.log('rootCa', rootCa); | ||
| fs.writeFileSync(path.join(process.env.GITHUB_WORKSPACE, 'integration/certs', 'rootCA.pem'), rootCa); | ||
| const certs = JSON.parse(process.env.INTEGRATION_CERTS); | ||
| for (const [name, cert] of Object.entries(certs)) { | ||
| fs.writeFileSync(path.join(process.env.GITHUB_WORKSPACE, 'integration/certs', name), cert); | ||
| } | ||
|
|
There was a problem hiding this comment.
Remove the root CA debug log.
Line 247 prints INTEGRATION_ROOT_CA into the workflow logs. Even if masking helps, secret material should not be emitted at all.
🛡️ Minimal fix
- console.log('rootCa', rootCa);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Write all ENV certificates to files in integration/certs | |
| uses: actions/github-script@v7 | |
| env: | |
| INTEGRATION_CERTS: '${{secrets.INTEGRATION_CERTS}}' | |
| INTEGRATION_ROOT_CA: '${{secrets.INTEGRATION_ROOT_CA}}' | |
| with: | |
| script: | | |
| const fs = require('fs'); | |
| const path = require('path'); | |
| const rootCa = process.env.INTEGRATION_ROOT_CA; | |
| console.log('rootCa', rootCa); | |
| fs.writeFileSync(path.join(process.env.GITHUB_WORKSPACE, 'integration/certs', 'rootCA.pem'), rootCa); | |
| const certs = JSON.parse(process.env.INTEGRATION_CERTS); | |
| for (const [name, cert] of Object.entries(certs)) { | |
| fs.writeFileSync(path.join(process.env.GITHUB_WORKSPACE, 'integration/certs', name), cert); | |
| } | |
| - name: Write all ENV certificates to files in integration/certs | |
| uses: actions/github-script@v7 | |
| env: | |
| INTEGRATION_CERTS: '${{secrets.INTEGRATION_CERTS}}' | |
| INTEGRATION_ROOT_CA: '${{secrets.INTEGRATION_ROOT_CA}}' | |
| with: | |
| script: | | |
| const fs = require('fs'); | |
| const path = require('path'); | |
| const rootCa = process.env.INTEGRATION_ROOT_CA; | |
| fs.writeFileSync(path.join(process.env.GITHUB_WORKSPACE, 'integration/certs', 'rootCA.pem'), rootCa); | |
| const certs = JSON.parse(process.env.INTEGRATION_CERTS); | |
| for (const [name, cert] of Object.entries(certs)) { | |
| fs.writeFileSync(path.join(process.env.GITHUB_WORKSPACE, 'integration/certs', name), cert); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci-fork-integration.yml around lines 237 - 253, The
workflow currently logs the secret via the console.log('rootCa', rootCa)
statement; remove that console.log so the INTEGRATION_ROOT_CA value is never
emitted to workflow logs, keeping the rest of the script that reads
process.env.INTEGRATION_ROOT_CA and writes files using
fs.writeFileSync(path.join(process.env.GITHUB_WORKSPACE, 'integration/certs',
'rootCA.pem'), rootCa) intact.
| - name: Run Integration Tests | ||
| id: integration-tests | ||
| timeout-minutes: 25 | ||
| run: pnpm turbo test:integration:${{ matrix.test-name }} $TURBO_ARGS | ||
| env: | ||
| E2E_DEBUG: '1' | ||
| E2E_APP_CLERK_JS_DIR: ${{runner.temp}} | ||
| E2E_APP_CLERK_UI_DIR: ${{runner.temp}} | ||
| E2E_CLERK_JS_VERSION: 'latest' | ||
| E2E_CLERK_UI_VERSION: 'latest' | ||
| E2E_NEXTJS_VERSION: ${{ matrix.next-version }} | ||
| E2E_PROJECT: ${{ matrix.test-project }} | ||
| E2E_CLERK_ENCRYPTION_KEY: ${{ matrix.clerk-encryption-key }} | ||
| INTEGRATION_INSTANCE_KEYS: ${{ secrets.INTEGRATION_INSTANCE_KEYS }} | ||
| NODE_EXTRA_CA_CERTS: ${{ github.workspace }}/integration/certs/rootCA.pem | ||
| VERCEL_AUTOMATION_BYPASS_SECRET: ${{ secrets.VERCEL_AUTOMATION_BYPASS_SECRET }} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n .github/workflows/ci-fork-integration.ymlRepository: clerk/javascript
Length of output: 13598
🏁 Script executed:
grep -n "clerk-encryption-key" .github/workflows/ci.yml | head -20Repository: clerk/javascript
Length of output: 134
🏁 Script executed:
sed -n '1,150p' .github/workflows/ci.yml | grep -A 100 "strategy:" | head -80Repository: clerk/javascript
Length of output: 42
🏁 Script executed:
sed -n '80,200p' .github/workflows/ci.ymlRepository: clerk/javascript
Length of output: 3860
🏁 Script executed:
sed -n '200,500p' .github/workflows/ci.yml | grep -B 5 -A 120 "integration-tests:"Repository: clerk/javascript
Length of output: 4504
🏁 Script executed:
sed -n '400,500p' .github/workflows/ci.ymlRepository: clerk/javascript
Length of output: 3996
Add clerk-encryption-key to the matrix or remove this env var.
E2E_CLERK_ENCRYPTION_KEY references ${{ matrix.clerk-encryption-key }} (line 270), but clerk-encryption-key is not defined in any matrix entry or include block. This causes the environment variable to be empty at runtime. The same issue exists in ci.yml.
🧰 Tools
🪛 actionlint (1.7.11)
[error] 270-270: property "clerk-encryption-key" is not defined in object type {next-version: number; test-name: string; test-project: string}
(expression)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci-fork-integration.yml around lines 258 - 273, The env
var E2E_CLERK_ENCRYPTION_KEY in the "Run Integration Tests" job is populated
from matrix.clerk-encryption-key but that matrix key does not exist; either add
clerk-encryption-key to the matrix/include entries (set a value per matrix
entry) so matrix.clerk-encryption-key resolves, or remove
E2E_CLERK_ENCRYPTION_KEY from the env block; apply the same fix to the
corresponding job in ci.yml to keep both workflows consistent (search for "Run
Integration Tests" or the job id integration-tests and the env name
E2E_CLERK_ENCRYPTION_KEY).
There was a problem hiding this comment.
♻️ Duplicate comments (4)
.github/workflows/ci-fork-integration.yml (4)
51-51:⚠️ Potential issue | 🟠 MajorPin all action references to full commit SHAs in this privileged workflow.
pull_request_targetruns with elevated repo context; mutable tags (@v2,@v4,@v7) are still present on Line 51, Line 135, Line 155, Line 236, and Line 284.🔧 Suggested hardening
- uses: actions-cool/check-user-permission@v2 + uses: actions-cool/check-user-permission@<40-char-commit-sha> # v2 - uses: actions/checkout@v4 + uses: actions/checkout@<40-char-commit-sha> # v4.x - uses: actions/checkout@v4 + uses: actions/checkout@<40-char-commit-sha> # v4.x - uses: actions/github-script@v7 + uses: actions/github-script@<40-char-commit-sha> # v7.x - uses: actions/upload-artifact@v4 + uses: actions/upload-artifact@<40-char-commit-sha> # v4.x#!/bin/bash # Verify all `uses:` refs in this workflow are SHA-pinned (40 hex chars). awk ' /^[[:space:]]*uses:[[:space:]]*/ { line=$0 ref=line sub(/^[[:space:]]*uses:[[:space:]]*/, "", ref) split(ref,a,"@") if (length(a) >= 2) { r=a[2] sub(/[[:space:]]+#.*/, "", r) if (r !~ /^[0-9a-f]{40}$/) { print NR ":" line } } }' .github/workflows/ci-fork-integration.ymlAlso applies to: 135-135, 155-155, 236-236, 284-284
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci-fork-integration.yml at line 51, This workflow uses mutable action tags (e.g., the reference "actions-cool/check-user-permission@v2" shown) which must be replaced with exact commit SHAs; update every "uses:" entry that currently uses a tag (the occurrences at the shown lines and the other mutable refs) to the corresponding full 40-character commit SHA for that action (replace "@vX" with "@<full-commit-sha>") so each "uses:" reference is immutable and SHA-pinned.
245-245:⚠️ Potential issue | 🟠 MajorRemove secret debug logging.
Line 245 logs
INTEGRATION_ROOT_CAcontent to workflow logs.🛡️ Fix
- console.log('rootCa', rootCa);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci-fork-integration.yml at line 245, Remove the secret debug logging that prints the INTEGRATION_ROOT_CA; specifically delete or replace the console.log('rootCa', rootCa) statement so the certificate/secret value is not written to workflow logs. If you need visibility for debugging, log a non-sensitive placeholder or a hash/length (e.g., log rootCa.length or a short SHA) instead of the full secret, and ensure the change is applied where console.log('rootCa', rootCa) appears.
268-268:⚠️ Potential issue | 🟠 MajorFix undefined matrix property reference.
Line 268 references
${{ matrix.clerk-encryption-key }}, but this key is not defined in the matrix object.🔧 Safe immediate fix (if key is not needed)
- E2E_CLERK_ENCRYPTION_KEY: ${{ matrix.clerk-encryption-key }}#!/bin/bash # Verify matrix keys and where clerk-encryption-key is referenced. rg -n "matrix:|test-name:|test-project:|next-version:|clerk-encryption-key|E2E_CLERK_ENCRYPTION_KEY" .github/workflows/ci-fork-integration.yml🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci-fork-integration.yml at line 268, The workflow references a non-existent matrix key `${{ matrix.clerk-encryption-key }}` for the environment variable E2E_CLERK_ENCRYPTION_KEY; fix by either adding a `clerk-encryption-key` entry to the matrix definition (so matrix contains that key) or stop referencing the matrix by replacing `${{ matrix.clerk-encryption-key }}` with the correct secret/variable (e.g., `secrets.CLERK_ENCRYPTION_KEY` or a different defined matrix key) or remove the E2E_CLERK_ENCRYPTION_KEY mapping entirely if not needed; update the CI matrix block (the matrix: definition) or the env mapping where E2E_CLERK_ENCRYPTION_KEY is set accordingly.
142-142:⚠️ Potential issue | 🔴 CriticalIsolate fork-code execution from privileged setup state.
After privileged setup, fork code is overlaid and
pnpm installis executed in the same job withclean: false(Line 163). That keeps inherited workspace/job state available to untrusted code.🔒 Minimal containment direction
- clean: false + clean: true + - name: Clear privileged env before fork dependency install + run: | + unset TURBO_TOKEN + unset TURBO_REMOTE_CACHE_SIGNATURE_KEY + unset TURBO_TEAM#!/bin/bash # Verify exported privileged envs and absence of cleanup before fork install. rg -n "exportVariable|TURBO_TOKEN|TURBO_REMOTE_CACHE_SIGNATURE_KEY|TURBO_TEAM" .github/actions/init-blacksmith/action.yml rg -n "Checkout fork PR|clean:|Re-install dependencies for fork code|unset TURBO_TOKEN|unset TURBO_REMOTE_CACHE_SIGNATURE_KEY|unset TURBO_TEAM" .github/workflows/ci-fork-integration.ymlAlso applies to: 154-166
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci-fork-integration.yml at line 142, The workflow overlays fork code onto a privileged setup and runs "pnpm install" with "clean: false", which leaves inherited workspace state and exported secrets (e.g., TURBO_TOKEN, TURBO_REMOTE_CACHE_SIGNATURE_KEY, TURBO_TEAM) available to untrusted fork PR code; fix by isolating fork execution: run the fork checkout and its "pnpm install" in a separate job or at minimum a separate step that uses a fresh checkout (actions/checkout with clean:true) and/or uses "pnpm install" with a clean workspace, and explicitly unset or avoid exporting privileged envs before the fork step (ensure any previous exportVariable calls are not visible to the fork step and add explicit unset of TURBO_TOKEN, TURBO_REMOTE_CACHE_SIGNATURE_KEY, TURBO_TEAM if needed).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/workflows/ci-fork-integration.yml:
- Line 51: This workflow uses mutable action tags (e.g., the reference
"actions-cool/check-user-permission@v2" shown) which must be replaced with exact
commit SHAs; update every "uses:" entry that currently uses a tag (the
occurrences at the shown lines and the other mutable refs) to the corresponding
full 40-character commit SHA for that action (replace "@vX" with
"@<full-commit-sha>") so each "uses:" reference is immutable and SHA-pinned.
- Line 245: Remove the secret debug logging that prints the INTEGRATION_ROOT_CA;
specifically delete or replace the console.log('rootCa', rootCa) statement so
the certificate/secret value is not written to workflow logs. If you need
visibility for debugging, log a non-sensitive placeholder or a hash/length
(e.g., log rootCa.length or a short SHA) instead of the full secret, and ensure
the change is applied where console.log('rootCa', rootCa) appears.
- Line 268: The workflow references a non-existent matrix key `${{
matrix.clerk-encryption-key }}` for the environment variable
E2E_CLERK_ENCRYPTION_KEY; fix by either adding a `clerk-encryption-key` entry to
the matrix definition (so matrix contains that key) or stop referencing the
matrix by replacing `${{ matrix.clerk-encryption-key }}` with the correct
secret/variable (e.g., `secrets.CLERK_ENCRYPTION_KEY` or a different defined
matrix key) or remove the E2E_CLERK_ENCRYPTION_KEY mapping entirely if not
needed; update the CI matrix block (the matrix: definition) or the env mapping
where E2E_CLERK_ENCRYPTION_KEY is set accordingly.
- Line 142: The workflow overlays fork code onto a privileged setup and runs
"pnpm install" with "clean: false", which leaves inherited workspace state and
exported secrets (e.g., TURBO_TOKEN, TURBO_REMOTE_CACHE_SIGNATURE_KEY,
TURBO_TEAM) available to untrusted fork PR code; fix by isolating fork
execution: run the fork checkout and its "pnpm install" in a separate job or at
minimum a separate step that uses a fresh checkout (actions/checkout with
clean:true) and/or uses "pnpm install" with a clean workspace, and explicitly
unset or avoid exporting privileged envs before the fork step (ensure any
previous exportVariable calls are not visible to the fork step and add explicit
unset of TURBO_TOKEN, TURBO_REMOTE_CACHE_SIGNATURE_KEY, TURBO_TEAM if needed).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: dd106dce-522f-4b65-a0c9-5d949787c3b7
📒 Files selected for processing (1)
.github/workflows/ci-fork-integration.yml
dominic-clerk
left a comment
dominic-clerk
left a comment
There was a problem hiding this comment.
I left some mildly paranoied comments. I'll return to this again later.
| clean: false | ||
|
|
||
| - name: Re-install dependencies for fork code | ||
| run: pnpm install |
There was a problem hiding this comment.
In case the review misses a change in config that had allowed a post-install script
| run: pnpm install | |
| run: pnpm install --ignore-scripts |
| id: checkAccess | ||
| uses: actions-cool/check-user-permission@v2 | ||
| with: | ||
| require: write |
There was a problem hiding this comment.
What about doing it like
javascript/.github/workflows/release.yml
Line 276 in b80eafb
I'd like the security gate to have as little 3rd party as possible and the way we do it in the release workflow uses only dependencies provided by GitHub.
I'm also wary of bots or 3rd party integrations that might have write access (dependabot for example) that might be hijacked... somehow. This is the paranoia speaking :)
| env: | ||
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| - name: Remove unauthorized label and fail | ||
| if: ${{ steps.checkAccess.outputs.require-result == 'false' }} |
There was a problem hiding this comment.
The docs claim true | false are the only results, but I wonder what happens if it errors out for whatever reason? Let's fail on "not true" instead of "false".
| if: ${{ steps.checkAccess.outputs.require-result == 'false' }} | |
| if: ${{ steps.checkAccess.outputs.require-result != 'true' }} |
| id: config | ||
| uses: ./.github/actions/init-blacksmith | ||
| with: | ||
| turbo-signature: ${{ secrets.TURBO_REMOTE_CACHE_SIGNATURE_KEY }} |
There was a problem hiding this comment.
Can we have a separate cache entirely to make cache poisoning completely impossible? I know it shouldn't be possible already and would require a hash collision but it would make it completely impossible and protect us against a bug in turbo itself.
There was a problem hiding this comment.
We could also just not use the turbo cache. These runs don't happen often so I think it's fine if they take extra time.
2 participants
Description
This PR adds a separate workflow that can be used by maintainers to trigger the integration tests on PRs from forks manually after review determines it's safe. This happens through adding a new
safe-to-testlabel, which only works for people with write access to the repo.Adding the label triggers a workflow on
pull_request_trigger, which does have access to secrets.The label gets removed on every push, requiring re-review and adding it again to trigger another run. The run that is triggered runs on the SHA from the event, not the latest SHA in the PR to avoid TOCTOU attacks.
Note that the built in "Approve workflows to run" does not apply to
pull_request_target(source), which is why we need the label.The flow for a forked PR would be:
safe-to-testlabel to trigger integration tests viaci-fork-integration.ymlBoth workflows manually post a status to a commit context with the same name, and this is what we use for a Required check, making it pass if either of the two workflows passes (normal or fork paths).
TODO:
actionlintmainFollow ups:
Summary by CodeRabbit