Test pull_request_target control trust - #2792
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8463351de6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Independent HIGH-severity review at 8463351: no confirmed HIGH or CRITICAL findings, so no fix batch. The new test is a drift contract for the reviewed block-style smart-ci-shadow.yml boundary: it pins the only current pull_request_target workflow, its job graph, executable envelope and step shapes; it rejects a PR-head checkout and direct fetched-head pipe-to-shell fixture. It should not be read as a general YAML security proof. Non-blocking #2335 residual: declaresPullRequestTarget does not recognize flow-style on: [pull_request_target], so a future additional workflow in that syntax is outside this detector. No current workflow uses that shape, and this test is not the enforcement boundary against a reviewer-approved malicious control-plane change; track it on #2335 rather than inflating severity. Latest hosted evidence was triaged without rerun: Smart CI Plan, Required Gate, and Planner Self-Test are SUCCESS at this head; CI Required API Integration (windows-latest) remains IN_PROGRESS. Focused exact-head test passed: node --test scripts/ci/smart-ci/control-trust.test.mjs (3/3); git diff --check clean. R4 hosted qualification and maintainer control-plane review remain required before merge. |
|
Follow-up for connector finding 3953111024: confirmed HIGH and fixed at 2c3eaa3. The contract had stripped run-block comment lines from its fingerprints; expression expansion before Bash turns a multiline PR-body value in such a comment into executable later lines. The control contract now rejects expressions in parsed run-block comments and pins the exact mutant. Focused test: node --test scripts/ci/smart-ci/control-trust.test.mjs (4/4 pass). |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c3eaa3b11
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Fresh scoped verification at
The range remains test-only ( |
|
Current qualification at 2c3eaa3 is green across all hosted checks. The expression-in-comment HIGH was fixed, and fresh scoped Terra verification passed all 4 tests. The flow-style trigger coverage P2 is tracked under #2335; both review threads have explicit dispositions and are resolved. Ready for the maintainer's own CI-control review under ADR-0066; no merge authority is inferred from CI or independent review. The whole #2335 issue remains open for its broader acceptance. |
Chris0Jeky
left a comment
Chris0Jeky
left a comment
There was a problem hiding this comment.
Terra fresh-context independent review: no confirmed CRITICAL or HIGH finding in the exact head 2c3eaa3. The regression contract pins the present block-style pull_request_target workflow, protected-base checkout and step envelope; it rejects a PR-head checkout, fetched-head pipe-to-shell, and interpolation in a run-block comment. The documented flow-style-trigger residual is non-blocking because no current workflow uses it. Hosted rollup is SUCCESS with zero unresolved threads. Parked pending the maintainer own ADR-0066 SC-10 review; no merge authority is inferred.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee095e53c0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| function fingerprintStep(step) { | ||
| const executableShape = step.raw | ||
| .filter((line) => !/^\s*#/.test(line)) |
There was a problem hiding this comment.
If a full-line comment is inserted after a shell continuation such as the current node ... \\ resolver invocation, both fingerprints discard it even though Bash joins the comment to the preceding line and then executes the following --pr line as a standalone command, breaking the control workflow while this suite remains green. Fresh evidence beyond the earlier expression-comment mutant is that a plain comment containing no GitHub expression reproduces this against the current resolver block; block-scalar comment lines therefore need to remain in the fingerprint.
Useful? React with 👍 / 👎.
|
Fresh Terra base-interaction review completed against main 7ac99a7: no CRITICAL/HIGH blocker. Each current head was materialized and its merge tree reconciled cleanly. Original source/fix reviews remain applicable. For the control seams, control-trust tests passed 4/4 (#2792), nightly tests passed 35/35 with docs checks (#2791). Fresh current-head hosted CI remains required; this comment does not waive the maintainer-review gate on control paths. |
|
Maintainer ruling received in-session on 2026-09-09: "Delegate this four-PR batch; review afterward" for #2769, #2803, #2791 and #2792. The coordinator may perform final review and merge after fresh hosted CI, with maintainer review afterward. These source heads are preserved in integration PR #2815 so the complete combined tree receives one final exact-head qualification. Separate historical disclosures, repository settings and release decisions are not acknowledged or authorized by this ruling. |
1 participant
Summary
Add a regression contract for the existing pull_request_target control-plane boundary in smart-ci-shadow.yml.
The test discovers every real workflow using pull_request_target, pins the supported workflow and two-job execution graph, requires each job's sole checkout to be the default protected-base checkout, and fingerprints executable step configuration so unreviewed command changes fail closed. Synthetic mutations prove the guard rejects both an explicit PR-head checkout and piping a fetched head object into a shell.
Implementation notes
Verification
Docs
No docs changed. This adds regression evidence for an existing contract and does not change shipped behavior or workflow policy.
Risks and limits
The parser is intentionally limited to the checked-in workflow shape rather than acting as a general YAML or security scanner. Unsupported trigger, job, or executable-step changes fail closed through structural assertions and fingerprints and require deliberate review. This guards the current workflow execution seam; it does not claim that arbitrary shell syntax can be proven safe by regex alone.
Refs #2335