Validate pre-created PR branch before privileged checkout - #55481
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot update safe outputs specification using w3c spec writer |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed in 7174596 by updating the Safe Outputs specification with W3C-style normative requirements for pre-created PR branch provenance, validation before downstream privileged use, and deterministic checkout refs. I also updated the pull-request reference docs to match. |
|
@copilot the user should be able to configure the branch prefix as part of the safe-outputs.create-pull-request configuration. |
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
There was a problem hiding this comment.
Verdict
REQUEST_CHANGES — this closes the original checkout injection hole, but it hard-codes a branch name that the feature already allows users to customize.
The blocking issue is a config/behavior mismatch in pre-created PR mode.
The new compiler path always checks out gh-aw/pre-created/<run_id>-<run_attempt> directly in downstream privileged jobs and the validator only accepts that exact ref. That is fine for the default case, but create-pull-request already exposes branch-prefix, and this PR now documents/validates steer as incompatible with branch-prefix instead of preserving the existing branch-prefix contract.
Right now a workflow author who configured branch naming policy cannot opt into pre-created PRs without losing that configuration surface. Either pre-created branch generation/validation needs to derive the trusted ref from configured branch-prefix, or the feature needs a much more deliberate API change than silently making the option unusable for this mode.
🔎 Code quality review by PR Code Quality Reviewer · pi · gpt54 · 41 AIC · ⌖ 9.08 AIC · ⊞ 7K
Comment /review to run again
|
|
||
| const preCreatePullRequestAppTokenStepID = "pre-create-pull-request-app-token" | ||
|
|
||
| func preCreatedPullRequestBranchRef() string { |
There was a problem hiding this comment.
This change hard-codes the trusted pre-created branch to gh-aw/pre-created/..., which breaks the existing create-pull-request.branch-prefix contract for anyone trying to use steer and branch naming policy together.
💡 Why this blocks
create-pull-request already supports branch-prefix, and the JS handler still understands it for normal PR creation. This patch makes pre-created PR mode special by forcing downstream privileged checkouts to a fixed ref and by rejecting steer + branch-prefix in validatePreCreatePullRequest().
That turns a security fix into a behavior regression: users who rely on branch naming policy cannot adopt steer at all. Either the trusted ref derivation needs to incorporate the configured prefix, or the product decision to drop branch-prefix in pre-created mode needs to be handled as an explicit API change with matching implementation/docs elsewhere.
A minimal fix would be to derive the expected trusted branch from configuration instead of returning a hard-coded literal here.
There was a problem hiding this comment.
Implemented configured branch-prefix support for steer-mode pre-created branches. The compiler now derives deterministic pre-created refs from that configuration in e066cf2.
There was a problem hiding this comment.
Review: Validate pre-created PR branch before privileged checkout
This is a well-structured security fix for the CodeQL finding. The two-layer defence is correct:
- Validation step —
validate-pre-created-pull-requestchecks the branch name against the deterministicgh-aw/pre-created/<run_id>-<attempt>pattern before it can flow into any downstream step. - Hardcoded
refat checkout — allactions/checkoutcalls now use the literal expected name instead of the activation output, so even if an attacker somehow altered the output, the checkout ignores it.
The pattern is applied consistently across all lock files. No issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet46 · 24.1 AIC · ⌖ 9.05 AIC · ⊞ 6.2K
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — two issues found; one requesting changes.
📋 Key Themes & Highlights
Key Themes
- Empty-branch bypass (security, line 7): If
GH_AW_EXPECTED_PRE_CREATED_PULL_REQUEST_BRANCHis empty, validation silently passes and an empty trusted-branch output is emitted. Add an explicit!expectedBranchguard. - Missing null-repo test (test coverage): The
?.optional chain on line 22 handles a nullhead.repocorrectly, but no test pins this behaviour. Add a test for the fork-deleted scenario.
Correction — inline comment #3
The concern about GitHub expression expansion in the Go compiler is a false alarm: fmt.Sprintf("...: %s\n", preCreatedPullRequestBranchRef()) emits the expression unquoted in YAML (confirmed in q.lock.yml), so ${{ github.run_id }} is correctly expanded by Actions at runtime. Disregard that comment.
Positive Highlights
- ✅ Validation step correctly placed after pre-creation and before the branch output is trusted by downstream jobs — good defence-in-depth ordering.
- ✅
pre_created_pull_request_branchoutput is now sourced from the validated step, not the raw pre-create output — this is the right fix for the CodeQL finding. - ✅ Deterministic
preCreatedPullRequestBranchRef()used for checkout ref instead of trusting the activation output — clean and correct. - ✅ Test suite is well-structured (Arrange/Act/Assert) and covers the main attack scenarios.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet46 · 56.6 AIC · ⌖ 10.3 AIC · ⊞ 7.6K
Comment /matt to run again
| async function main() { | ||
| const expectedBranch = process.env.GH_AW_EXPECTED_PRE_CREATED_PULL_REQUEST_BRANCH || ""; | ||
| const branch = process.env.GH_AW_PRE_CREATED_PULL_REQUEST_BRANCH || ""; | ||
| if (branch !== expectedBranch) { |
There was a problem hiding this comment.
[/diagnosing-bugs] Empty expectedBranch silently bypasses validation — if GH_AW_EXPECTED_PRE_CREATED_PULL_REQUEST_BRANCH is unset or empty, both expectedBranch and branch default to "", the equality check passes, and core.setOutput("branch", "") is emitted without an error.
💡 Suggested guard
Add an explicit guard before the equality check:
if (!expectedBranch) {
throw new Error("GH_AW_EXPECTED_PRE_CREATED_PULL_REQUEST_BRANCH is not set");
}And a corresponding test:
it("rejects empty expectedBranch to prevent misconfiguration bypass", async () => {
process.env.GH_AW_EXPECTED_PRE_CREATED_PULL_REQUEST_BRANCH = "";
process.env.GH_AW_PRE_CREATED_PULL_REQUEST_BRANCH = "";
const { main } = await import("./validate_pre_created_pull_request.cjs");
await expect(main()).rejects.toThrow(/not set/);
});Without this guard, a misconfigured job would accept any branch that also produces empty output.
@copilot please address this.
There was a problem hiding this comment.
Added a fail-closed guard for missing expected branch metadata, with coverage, in 1fbd7c3.
| pull_number: pullNumber, | ||
| }); | ||
| const expectedRepo = `${context.repo.owner}/${context.repo.repo}`.toLowerCase(); | ||
| if (pullRequest.head.ref !== expectedBranch || pullRequest.head.repo?.full_name?.toLowerCase() !== expectedRepo || pullRequest.base.repo?.full_name?.toLowerCase() !== expectedRepo) { |
There was a problem hiding this comment.
[/tdd] No test covers the case where pullRequest.head.ref matches but head.repo is null — the ?. optional chain on line 22 silently swallows this and produces undefined !== expectedRepo, which throws the right error, but there's no test to pin that behaviour. If the logic is ever refactored, a null repo could be silently accepted.
💡 Suggested test
it("rejects when head repo is null (fork-deleted scenario)", async () => {
global.github.rest.pulls.get.mockResolvedValue({
data: {
head: { ref: "gh-aw/pre-created/123-2", repo: null },
base: { repo: { full_name: "owner/repo" } },
},
});
const { main } = await import("./validate_pre_created_pull_request.cjs");
await expect(main()).rejects.toThrow(/does not target the expected trusted repository branch/);
});@copilot please address this.
There was a problem hiding this comment.
Added coverage for a null PR head repository; it is rejected before emitting the trusted branch in 1fbd7c3.
| " GH_AW_PRE_CREATED_PULL_REQUEST_BRANCH: ${{ steps.pre-create-pull-request.outputs.branch }}\n", | ||
| fmt.Sprintf(" GH_AW_EXPECTED_PRE_CREATED_PULL_REQUEST_BRANCH: %s\n", preCreatedPullRequestBranchRef()), | ||
| " with:\n", | ||
| fmt.Sprintf(" github-token: %s\n", token), |
There was a problem hiding this comment.
[/diagnosing-bugs] GH_AW_EXPECTED_PRE_CREATED_PULL_REQUEST_BRANCH is set to a plain string "gh-aw/pre-created/${{ github.run_id }}-${{ github.run_attempt }}" (unexpanded) in the env: block. This means the equality check on line 7 compares the raw template literal string against the actual branch name, which will never match at runtime — validation will always fail.
The fmt.Sprintf call inserts the literal GitHub expression ${{ github.run_id }}-${{ github.run_attempt }} into the YAML, which GitHub Actions will expand before the step runs. Verify in a generated lock file that the env: block reads:
GH_AW_EXPECTED_PRE_CREATED_PULL_REQUEST_BRANCH: gh-aw/pre-created/${{ github.run_id }}-${{ github.run_attempt }}and NOT a quoted Go string. If the value is double-quoted in YAML it will be treated as literal text by the shell and the ${{ expansion will not happen.
@copilot please address this.
There was a problem hiding this comment.
The generated environment value is a GitHub Actions expression and is evaluated before the github-script step runs. Generated lockfiles contain the deterministic run-id/run-attempt ref.
There was a problem hiding this comment.
Pull request overview
Hardens pre-created pull request workflows by validating branch provenance and using deterministic checkout refs.
Changes:
- Adds activation-time PR branch and repository validation.
- Uses workflow-derived refs for agent and safe-output checkouts.
- Updates tests, documentation, schema, and generated workflows.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/create_pull_request.go |
Validates configured pre-created branch prefixes. |
pkg/workflow/compiler_yaml_checkout.go |
Uses deterministic agent checkout refs. |
pkg/workflow/compiler_safe_outputs_steps.go |
Uses deterministic safe-output checkout refs. |
pkg/workflow/compiler_precreate_pull_request.go |
Generates branch refs and validation steps. |
pkg/workflow/compiler_precreate_pull_request_test.go |
Tests generation and prefix validation. |
pkg/workflow/compiler_precreate_pull_request_integration_test.go |
Tests compiled checkout behavior. |
pkg/parser/schemas/main_workflow_schema.json |
Documents steer-mode prefix constraints. |
docs/src/content/docs/specs/safe-outputs-specification.md |
Specifies branch-provenance requirements. |
docs/src/content/docs/reference/safe-outputs-pull-requests.md |
Documents secure pre-creation behavior. |
docs/src/content/docs/reference/frontmatter-full.md |
Updates branch-prefix reference text. |
actions/setup/js/validate_pre_created_pull_request.test.cjs |
Tests PR provenance validation. |
actions/setup/js/validate_pre_created_pull_request.cjs |
Validates branch, PR number, and repositories. |
actions/setup/js/pre_create_pull_request.test.cjs |
Tests custom branch prefixes. |
actions/setup/js/pre_create_pull_request.cjs |
Creates deterministic prefixed branches. |
.github/workflows/weekly-safe-outputs-spec-review.lock.yml |
Regenerates validation and deterministic refs. |
.github/workflows/weekly-editors-health-check.lock.yml |
Regenerates validation and deterministic refs. |
.github/workflows/weekly-blog-post-writer.lock.yml |
Regenerates validation and deterministic refs. |
.github/workflows/update-astro.lock.yml |
Regenerates validation and deterministic refs. |
.github/workflows/unbloat-docs.lock.yml |
Regenerates validation and deterministic refs. |
.github/workflows/ubuntu-image-analyzer.lock.yml |
Regenerates validation and deterministic refs. |
.github/workflows/tidy.lock.yml |
Regenerates validation and deterministic refs. |
.github/workflows/technical-doc-writer.lock.yml |
Regenerates validation and deterministic refs. |
.github/workflows/spec-extractor.lock.yml |
Regenerates validation and deterministic refs. |
.github/workflows/spec-enforcer.lock.yml |
Regenerates validation and deterministic refs. |
.github/workflows/smoke-project.lock.yml |
Regenerates validation and deterministic refs. |
.github/workflows/slide-deck-maintainer.lock.yml |
Regenerates validation and deterministic refs. |
.github/workflows/ruflo-backed-task.lock.yml |
Regenerates validation and deterministic refs. |
.github/workflows/refiner.lock.yml |
Regenerates validation and deterministic refs. |
.github/workflows/q.lock.yml |
Removes attacker-influenced checkout refs. |
.github/workflows/purelock.lock.yml |
Regenerates validation and deterministic refs. |
.github/workflows/linter-miner.lock.yml |
Regenerates validation and deterministic refs. |
.github/workflows/layout-spec-maintainer.lock.yml |
Regenerates validation and deterministic refs. |
.github/workflows/instructions-janitor.lock.yml |
Regenerates validation and deterministic refs. |
.github/workflows/hourly-ci-cleaner.lock.yml |
Regenerates validation and deterministic refs. |
.github/workflows/go-logger.lock.yml |
Regenerates validation and deterministic refs. |
.github/workflows/glossary-maintainer.lock.yml |
Regenerates validation and deterministic refs. |
.github/workflows/github-mcp-tools-report.lock.yml |
Regenerates validation and deterministic refs. |
.github/workflows/functional-pragmatist.lock.yml |
Regenerates validation and deterministic refs. |
.github/workflows/evoskill-evolver.lock.yml |
Regenerates validation and deterministic refs. |
.github/workflows/eslint-miner.lock.yml |
Regenerates validation and deterministic refs. |
.github/workflows/dictation-prompt.lock.yml |
Regenerates validation and deterministic refs. |
.github/workflows/developer-docs-consolidator.lock.yml |
Regenerates validation and deterministic refs. |
.github/workflows/dependabot-burner.lock.yml |
Regenerates validation and deterministic refs. |
.github/workflows/dead-code-remover.lock.yml |
Regenerates validation and deterministic refs. |
.github/workflows/daily-yamllint-fixer.lock.yml |
Regenerates validation and deterministic refs. |
.github/workflows/daily-workflow-updater.lock.yml |
Regenerates validation and deterministic refs. |
.github/workflows/daily-trajectory-grader-implementer.lock.yml |
Regenerates validation and deterministic refs. |
.github/workflows/daily-safeoutputs-git-simulator.lock.yml |
Regenerates validation and deterministic refs. |
.github/workflows/daily-safe-output-integrator.lock.yml |
Regenerates validation and deterministic refs. |
.github/workflows/daily-rendering-scripts-verifier.lock.yml |
Regenerates validation and deterministic refs. |
.github/workflows/daily-go-test-stubs-aider.lock.yml |
Regenerates validation and deterministic refs. |
.github/workflows/daily-go-test-parallelizer.lock.yml |
Regenerates validation and deterministic refs. |
.github/workflows/daily-elixir-credo-snippet-audit.lock.yml |
Regenerates validation and deterministic refs. |
.github/workflows/daily-documentation-diagram.lock.yml |
Regenerates validation and deterministic refs. |
.github/workflows/daily-doc-updater.lock.yml |
Regenerates validation and deterministic refs. |
.github/workflows/daily-doc-healer.lock.yml |
Regenerates validation and deterministic refs. |
.github/workflows/daily-compiler-threat-spec-optimizer.lock.yml |
Regenerates validation and deterministic refs. |
.github/workflows/daily-community-attribution.lock.yml |
Regenerates validation and deterministic refs. |
.github/workflows/daily-code-debt-aider.lock.yml |
Regenerates validation and deterministic refs. |
.github/workflows/daily-caveman-optimizer.lock.yml |
Regenerates validation and deterministic refs. |
.github/workflows/daily-astrostylelite-markdown-spellcheck.lock.yml |
Regenerates validation and deterministic refs. |
.github/workflows/daily-architecture-diagram.lock.yml |
Regenerates validation and deterministic refs. |
.github/workflows/daily-agent-of-the-day-blog-writer.lock.yml |
Regenerates validation and deterministic refs. |
.github/workflows/code-simplifier.lock.yml |
Regenerates validation and deterministic refs. |
.github/workflows/code-scanning-fixer.lock.yml |
Regenerates validation and deterministic refs. |
.github/workflows/cloclo.lock.yml |
Regenerates validation and deterministic refs. |
.github/workflows/ci-coach.lock.yml |
Regenerates validation and deterministic refs. |
.github/workflows/avenger.lock.yml |
Regenerates validation and deterministic refs. |
Review details
- Files reviewed: 68/68 changed files
- Comments generated: 2
- Review effort level: Balanced
| normalized := normalizePreCreatedPullRequestBranchPrefix(prefix) | ||
| if normalized != prefix || normalized == "" { | ||
| return fmt.Errorf("safe-outputs.create-pull-request.steer branch-prefix must be a valid git branch prefix; normalized form would be %q", normalized) | ||
| } |
There was a problem hiding this comment.
Pre-created branch prefixes now validate the complete deterministic branch ref and reject reserved or structurally invalid forms in 1c25d85.
| const pullNumber = Number.parseInt(process.env.GH_AW_PRE_CREATED_PULL_REQUEST_NUMBER || "", 10); | ||
| if (!Number.isSafeInteger(pullNumber) || pullNumber <= 0) { | ||
| throw new Error("Pre-created pull request number is invalid"); | ||
| } |
There was a problem hiding this comment.
PR numbers now require a canonical positive decimal string before safe-integer conversion, with malformed-value coverage, in 1c25d85.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed in 281333e by allowing |
Test Quality Sentinel ReportScore: 87/100 ✅ Excellent
23 tests added with strong security focus and comprehensive edge-case coverage.
|
There was a problem hiding this comment.
✅ Test Quality Sentinel: 87/100 — Excellent
Results: 78% design tests (threshold: 70%), 0% implementation tests (threshold: <30%), strong edge-case coverage with security-focused validation, no violations.
Recommendation: Approve — comprehensive test coverage for pre-created PR branch validation feature.
|
Please do one focused follow-up pass:
I also attempted a branch refresh for this PR in this run. Run: https://github.com/github/gh-aw/actions/runs/32780228606
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Documents the already-implemented validate-pre-created-pull-request activation-job step (#55481) that re-validates the pre-created steer PR's branch and repository identity before privileged agent/safe-outputs checkout, and the branch-prefix static/valid-git-ref-prefix validation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
CodeQL flagged
q.lock.ymlfor checking out an attacker-influenced pre-created PR branch in a privileged workflow path. The risk was thatissue_comment-reachable input could flow intoactions/checkoutbefore the branch was proven to be the workflow-created branch.Branch validation
gh-aw/pre-created/<run_id>-<attempt>pattern.Safer checkout ref
.github/workflows/q.lock.ymlfrom the updated compiler output.gh-aw-pr-sous-chef: attempted branch refresh in run https://github.com/github/gh-aw/actions/runs/32780228606