Repository navigation
Potential fix for code scanning alert no. 41: Checkout of untrusted code in a non-privileged context - #2318
Potential fix for code scanning alert no. 41: Checkout of untrusted code in a non-privileged context#2318don-petry wants to merge 6 commits into
Conversation
…ode in a non-privileged context Copilot Autofix for this issue Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Dev-Lead — waiting on PR blockers (intent: review-changes)PR: #2318 |
|
Note @don-petry I reviewed this PR and no code changes were needed, but I can't mark it done yet: required check |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
No description provided. |
|
This is an informational notice indicating that Gemini cannot review this PR due to unsupported file types. However, all CI checks pass and there are no code findings to address. The underlying changes (scripts and workflow configurations) have been validated by all automated checks. |
Dev-Lead — fix-bot-comment (no-changes)Agent reasoning |
|
No description provided. |
…erpolation Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Dev-Lead — review-changes (applied)Changes committed and pushed. Requested items addressed:
|
|
No description provided. |
This comment has been minimized.
This comment has been minimized.
Dev-Lead — fix-bot-comment (no-changes)Agent reasoning |
Updates this branch onto the renamed main and rewrites its petry-projects references to combsmith (GitHub Actions does not redirect uses: after an org rename). Rename-aware merge: base and branch were rewritten first, so rename-only differences do not conflict.
|
No description provided. |
|
No description provided. |
|
Re-read the current CodeRabbit body: it only says the draft PR was not reviewed and offers a manual-review checkbox and an autofix toggle. It has no findings, actionable comments, or architecture-review items, so no action is needed. |
Dev-Lead — fix-reviews (applied)Changes committed and pushed. Requested items addressed:
|
|
Automated review — escalated to humanThe automated review cascade escalated this PR to a human reviewer at review cycle 0/3 (risk: HIGH, reviewed commit Why: the cascade could neither approve the PR nor auto-request fixes, so it requested human review via CODEOWNERS and set the Reviewer summary: This Copilot Autofix PR makes the push job of dev-lead-reusable.yml reject any agent_ref that is not a 40-hex SHA. The input defaults to "main" and the only real caller (dev-lead.yml:74) passes the channel tag dev-lead/v139-ring0, so the push job would fail for every consumer, and the PR adds no real security benefit. The new step does not open an injection or secret-exposure risk. However, it contradicts the documented first-party channel-tag exception in AGENTS.md, leaves the other two checkouts unchanged, and should not merge. The better path is to dismiss alert #41 with a written justification, or to allowlist main and dev-lead/v- refs consistently across all three checkouts. This note is updated in place on re-escalation; it is not re-posted. |
|
dev-lead is withholding action on this item. It is labeled To re-enable automated pickup: remove the |
| echo "::error::inputs.agent_ref must be provided as a pinned commit SHA." | ||
| exit 1 | ||
| fi | ||
| if ! [[ "$ref" =~ ^[0-9a-fA-F]{40}$ ]]; then |
There was a problem hiding this comment.
[High] Requiring inputs.agent_ref to be a 40-character commit SHA (! [[ "$ref" =~ ^[0-9a-fA-F]{40}$ ]]) and rejecting empty, branch, or tag refs breaks every dev-lead caller across the organization. Per standards/ci-standards.md, tests/test_sc2_self_review_channel.bats, and tests/dev-lead/integration/test_reusable_pin_compliance.py, caller stubs must pass a moving channel tag (dev-lead/v139-stable, dev-lead/v139-ring0, etc., or omit it to default to main) and are forbidden from SHA-pinning agent_ref. With this check in place, dispatch runs at line 256 using the channel tag, and this job then unconditionally fails at line 802. Validate against the allowed dev-lead channel/tag pattern (^main$|^dev-lead/(v[0-9]+(\.[0-9]+\.[0-9]+|-(stable|next|ring[0-9]+))|stable|next|ring[0-9]+)$, defaulting empty input to main) instead of requiring a 40-hex SHA.
|
Potential fix for https://github.com/petry-projects/.github-private/security/code-scanning/41
To fix this safely without changing intended functionality, enforce that
inputs.agent_refis a pinned commit SHA before checkout, and use that validated value for the.github-privatecheckout. This preserves the existing behavior (still caller-selectable ref) while preventing mutable/untrusted refs like branch names or tags from being used.Best single fix in this file:
inputs.agent_refis emptyGITHUB_ENV(e.g.,SAFE_AGENT_REF)ref: ${{ env.SAFE_AGENT_REF }}instead of raw input.Edits are only in
.github/workflows/dev-lead-reusable.yml, around lines 791–797.Suggested fixes powered by Copilot Autofix. Review carefully before merging.
Problem
See the linked issue.
Risk
Medium — changes GitHub Actions workflow behavior, which is exercised only post-merge; verify via the affected workflow runs.
Test plan
No test files were added or updated. Verification:
bash scripts/dev-lead-lint.sh(shellcheck --severity=warning) ran pre-commit; the existing CI (bats + lint) guards the change.Rollback
Revert this PR. No non-revertible side effects (no tags, migrations, or external state).
Monitoring
Watch the affected workflow run(s) in the Actions tab and this PR's Lint check for regressions.