Validate fork PR plugins via pull_request_target#100
Draft
andrewmumblebee wants to merge 5 commits into
Draft
Conversation
The CI workflow logged in with --apiKey before validating, but
GitHub Actions withholds repository secrets from pull_request runs
triggered by a fork PR. The API key resolved to an empty string,
and the CLI silently fell through to the OAuth device flow, hanging
for its full 5-minute timeout before failing instead of erroring
immediately.
Switched the trigger to pull_request_target, which runs with the
base repo's secrets regardless of where the PR originates, and
pinned checkout to the PR's head commit so a later push can't swap
in different code after review. The workflow file itself is always
read from the base branch under pull_request_target, so a fork
editing pr-run.yaml has no effect — but the checked-out plugin
files are still attacker-controlled data. Three spots spliced
${{ steps.detect.outputs.plugin_paths }} directly into shell
scripts, a script-injection hole that was harmless without secrets
but not with them; that data now flows through an env var instead.
The CLI install also moved outside the checkout, since npm reads a
project .npmrc from the working directory even for global installs,
and a fork PR could otherwise redirect the install to a package it
controls in the same step the API key is in scope.
Deploy still only runs for same-repo PRs — validating fork-submitted
plugin code is one thing, but auto-deploying it to the live tenant
needs a maintainer to decide that deliberately. Also logs the
installed CLI version, since not knowing which version had actually
run was part of what made this issue hard to diagnose.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThe pull request workflow adds ChangesPlugin workflow
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
andrewmumblebee
marked this pull request as ready for review
July 22, 2026 13:38
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/pr-run.yaml:
- Around line 41-44: Update the SquaredUp CLI installation step around the
squaredup version check to install an approved exact `@squaredup/cli` version
instead of resolving the moving latest version, and verify the installed
package’s integrity before proceeding. Keep the existing version output while
ensuring the workflow uses the pinned version consistently.
- Line 4: Add concurrency configuration to the pull_request_target workflow so
runs sharing the same pull request are serialized, using a group keyed by the PR
number and cancelling or queuing previous runs according to the required
deployment behavior. Ensure the existing deployment job reuses this
workflow-level or job-level concurrency setting.
- Line 4: Replace the privileged pull_request_target trigger and checkout flow
with an unprivileged pull_request validation workflow, ensuring the PR head code
cannot access repository secrets. Pin the `@squaredup/cli` version before
retaining or introducing any separate secret-backed deployment path, and keep
secret-based authentication out of the PR checkout job.
🪄 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 UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9dce5589-b86d-4fb8-b187-513e695eae57
📒 Files selected for processing (1)
.github/workflows/pr-run.yaml
Member
|
I'll leave this one to @shaswot77 - I don't have good visibility of whether this would expose secrets in any logs etc. |
pull_request_target locks execution to the workflow file on the PR's base ref, which is exactly what makes it safe against a fork editing pr-run.yaml — but it also means a PR can never be used to test changes to this file itself; the old version keeps running until the change lands on main. Adding workflow_dispatch closes that gap: it lets anyone with write access run this workflow directly against a branch (gh workflow run "Validate & Deploy Plugins" --ref <branch>), executing that branch's own copy of the file. Two steps assumed a pull_request payload that doesn't exist for a manual run — checkout's ref now falls back to the dispatched commit, and the PR-comment step (which has nothing to comment on) is skipped outside pull_request_target. workflow_dispatch has a different safety model than pull_request_target though: it runs whatever copy of the workflow lives on the ref you select, not always the default branch's. A fork can't trigger it at all (dispatching requires write access to this repo), but a maintainer manually dispatching against a PR/fork ref rather than a real branch would run that PR's own version of the workflow with the real secret in scope. Left an explicit warning against doing that next to the trigger. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/pr-run.yaml (1)
47-57: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSplit the CLI install from the secret-bearing login step.
SQUAREDUP_API_KEYis inherited bynpm install, so any lifecycle script in@squaredup/clior its dependencies can read it beforesquaredup loginruns. Install the CLI in a secret-free step, then pass the API key only to the login step.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/pr-run.yaml around lines 47 - 57, Split the “Install & Configure SquaredUp CLI” workflow step into separate install and login steps. Keep the npm installation and version check in a step without SQUAREDUP_API_KEY in its environment, then place squaredup login in a subsequent conditional step that exposes the secret only through its env configuration.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.github/workflows/pr-run.yaml:
- Around line 47-57: Split the “Install & Configure SquaredUp CLI” workflow step
into separate install and login steps. Keep the npm installation and version
check in a step without SQUAREDUP_API_KEY in its environment, then place
squaredup login in a subsequent conditional step that exposes the secret only
through its env configuration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: b2ebf0c4-b6dc-47fc-a5c9-7f60489150bf
📒 Files selected for processing (1)
.github/workflows/pr-run.yaml
andrewmumblebee
marked this pull request as draft
July 23, 2026 09:52
validate-and-deploy is a required status check on main, and this PR can't satisfy it against itself: its own branch removed pull_request (the only trigger that ever evaluates a PR's own workflow content), and pull_request_target always resolves from main, which doesn't have that trigger yet. Neither one fires, so the required check never reports anything and the merge stays blocked — confirmed via `gh run list`, which shows no pull_request/pull_request_target run has ever fired on this branch, and the repo's ruleset has no bypass actors, so there's no way around it either. Restoring pull_request here just long enough for this PR to report its own required check. Once merged, main has pull_request_target and every subsequent PR is covered by it, so pull_request comes back out in an immediate follow-up. Both the checkout ref fallback and the PR-comment gate were already written in terms of what data is present (a pull_request payload, or not workflow_dispatch) rather than a specific event name, so they keep working through this transition without further changes, including once pull_request is removed again. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The prior wording didn't say who could actually act on it, which read as if a fork PR author had some way to leak the secret directly. They don't — dispatching requires write access to this repo, so only a maintainer can trigger it at all. The real risk is a maintainer being asked (or absent-mindedly choosing) to dispatch against a PR/fork ref rather than a branch, which is a social-engineering risk against a trusted human, not a bypass available to the fork author. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/pr-run.yaml:
- Around line 4-15: Restrict the deploy step around the visible workflow job or
step near lines 105–111 to run only when github.event_name is
pull_request_target. Preserve the existing same-repository check, while ensuring
pull_request executions remain validation-only and cannot deploy or race with
the target-triggered run.
- Around line 16-26: Remove the workflow_dispatch trigger and its associated
manual-dispatch guidance from the secret-bearing workflow, while preserving the
pull_request_target behavior and SQUAREDUP_API_KEY usage. If manual testing must
remain available, move it to a separate workflow that does not receive secrets.
🪄 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 UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6baa0edd-782c-4af6-b3de-457a563ac933
📒 Files selected for processing (1)
.github/workflows/pr-run.yaml
Two real issues from CodeRabbit's review, both against the current transitional dual-trigger setup: with pull_request and pull_request_target both active, a same-repo PR touching plugin files would run deploy twice per push (once per trigger), racing on the same --suffix. Restricted deploy to pull_request_target specifically to fix that at the root, and added a concurrency group per PR number so a rapid follow-up push can't independently overlap and finish out of order either — both were live gaps, not just theoretical. Also dropping workflow_dispatch. It was added so future changes to this file could be tested pre-merge, but it also puts the real SQUAREDUP_API_KEY in scope for a maintainer-triggered run, and we don't have an actual need for it yet — easier to add back once we do than to carry the extra secret-bearing trigger on spec. Checkout's ref and the PR-comment step's condition were only written the way they were to account for workflow_dispatch's missing pull_request payload, so both simplify back now that it's gone. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
🧩 Plugin PR Summaryℹ️ No plugins were modified in this PR. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/pr-run.yaml (2)
54-64: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winKeep
SQUAREDUP_API_KEYout of the install step.
npm install -g@squaredup/cli`` runs with the secret in the step environment, so any npm lifecycle script can access it. Install the CLI in a separate step with no secrets, then passSQUAREDUP_API_KEYonly to `squaredup login`.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/pr-run.yaml around lines 54 - 64, Split the “Install & Configure SquaredUp CLI” workflow step into separate install and login steps. Keep npm installation and version reporting in a step without SQUAREDUP_API_KEY, then provide the secret only in the login step that runs squaredup login.Source: MCP tools
175-177: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winGate the comment step to
pull_request_target(.github/workflows/pr-run.yaml:175-177)
GITHUB_TOKENis read-only on fork-originatedpull_requestruns, soissues.deleteComment/createCommentcan 403 and turn a passing validation into a failed required check. Gate this step topull_request_targetor skip forks here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/pr-run.yaml around lines 175 - 177, Update the “Post PR comment” step to run only for pull_request_target events, or explicitly exclude fork-originated pull_request runs while preserving execution for trusted pull request contexts. Keep the existing always() behavior within the permitted event scope.Source: MCP tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/pr-run.yaml:
- Around line 17-22: Update the workflow concurrency configuration around the
pr-run concurrency group to separate validation and deployment lanes by event,
or disable cancellation for pull_request_target deploy runs. Ensure a newer
pull_request validation cannot cancel an in-flight pull_request_target
deployment, while preserving serialization for runs that reuse the same PR
suffix.
---
Outside diff comments:
In @.github/workflows/pr-run.yaml:
- Around line 54-64: Split the “Install & Configure SquaredUp CLI” workflow step
into separate install and login steps. Keep npm installation and version
reporting in a step without SQUAREDUP_API_KEY, then provide the secret only in
the login step that runs squaredup login.
- Around line 175-177: Update the “Post PR comment” step to run only for
pull_request_target events, or explicitly exclude fork-originated pull_request
runs while preserving execution for trusted pull request contexts. Keep the
existing always() behavior within the permitted event scope.
🪄 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 UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1d90fa6e-10ee-4281-9cb4-3cd1733a49db
📒 Files selected for processing (1)
.github/workflows/pr-run.yaml
Comment on lines
+17
to
+22
| # Serialize runs per PR so an older run (from a rapid follow-up push, or the transitional | ||
| # pull_request/pull_request_target overlap above) can't finish last and overwrite a newer | ||
| # deploy that reuses the same --suffix. | ||
| concurrency: | ||
| group: pr-run-${{ github.event.pull_request.number }} | ||
| cancel-in-progress: true |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the workflow file structure first, then inspect the relevant slices.
ast-grep outline .github/workflows/pr-run.yaml --view expanded || true
printf '\n--- workflow excerpt ---\n'
cat -n .github/workflows/pr-run.yaml | sed -n '1,260p'Repository: squaredup/plugins
Length of output: 10060
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where the workflow uses pull_request / pull_request_target and where deploy happens.
rg -n "pull_request_target|pull_request|concurrency|cancel-in-progress|squaredup deploy|PLUGIN_PATHS|comments|API_KEY|token" .github/workflows/pr-run.yaml .github/workflows -g '*.yaml' -g '*.yml'Repository: squaredup/plugins
Length of output: 5382
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- pr-cleanup workflow ---\n'
if [ -f .github/workflows/pr-cleanup.yaml ]; then
cat -n .github/workflows/pr-cleanup.yaml
else
echo "missing"
fi
printf '\n--- deploy-related references ---\n'
rg -n "squaredup deploy|cleanup|suffix|cancel-in-progress|concurrency" .github/workflows -g '*.yml' -g '*.yaml'Repository: squaredup/plugins
Length of output: 1990
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect any docs or code that describe squaredup deploy behavior or suffix cleanup.
rg -n "squaredup deploy|--suffix|pr-cleanup|cleanup old|remove.*deployment|undeploy|delete.*deployment|tenant" . -g '*.md' -g '*.yaml' -g '*.yml' -g '*.js' -g '*.ts' -g '*.sh'Repository: squaredup/plugins
Length of output: 541
Separate validation from deployment concurrency
.github/workflows/pr-run.yaml:20-22 uses one PR-wide group for both events, so a newer pull_request validation can cancel an in-flight pull_request_target deploy. Since deploy writes to the live tenant and reuses the same --suffix, that can leave partial PR state. Split the group by event, or disable cancellation for the deploy lane.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/pr-run.yaml around lines 17 - 22, Update the workflow
concurrency configuration around the pr-run concurrency group to separate
validation and deployment lanes by event, or disable cancellation for
pull_request_target deploy runs. Ensure a newer pull_request validation cannot
cancel an in-flight pull_request_target deployment, while preserving
serialization for runs that reuse the same PR suffix.
Source: MCP tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📋 Summary
The plugin PR workflow logged in with
--apiKeybefore validating, but GitHub Actions withholds repository secrets frompull_requestruns triggered by a fork PR — soSQUAREDUP_API_KEYresolved to an empty string.In practice this meant
validatenever ran for external contributors.This switches the trigger to
pull_request_target, which runs with the base repo's secrets regardless of where the PR originates, and pins checkout to the PR's head commit. Because the workflow file itself is always read from the base branch underpull_request_target, a fork PR editingpr-run.yamlhas no effect on what actually runs — but thechecked-out plugin files are still attacker-controlled data, so this also:
plugin_pathsthrough anenv:var instead of splicing it directly into shell scripts via${{ }}- a script-injection hole that was harmless without secrets in scope, but wouldn't have been once they were.$RUNNER_TEMP), since npm reads a project.npmrcfrom the working directory even for global installs, and a fork PR could otherwise redirect the install to a package it controls in the same step the API key is exposed.deployto same-repo PRs only - validation always determines that a plugin is deployable, we only need to deploy if we want to test ourselves. Happy to change as needed.Transitional:
pull_requestis still here toovalidate-and-deployis a required status check, and this PR can't satisfy that check against itself throughpull_request_targetalone — that trigger always resolves the workflow file frommain, which doesn't have it until this merges.pull_requestis kept temporarily so this PR's own check can actually run; it comes back out in an immediate follow-up PR once this lands, since every PR after that is covered bypull_request_targetonmain.While both triggers are active, a same-repo PR touching plugin files would otherwise run
deploytwice per push (once per trigger), racing on the same--suffix.deployis now also restricted togithub.event_name == 'pull_request_target'specifically, and aconcurrencygroup (keyed on PR number) serializes runs so a rapid follow-up push can't independently overlap and land out of order either.🔍 Scope of change
📚 Checklist
Summary by CodeRabbit
pull_request_targethandling while retaining PR-scoped concurrency.pull_request_target.