ci: add release security-scan#4042
Conversation
Adds a fourth release-PR check, alongside integration-test / behavior-test / test-examples, triggered by a 'security-scan' label. It guards the release diff (last v* tag -> this PR) against two supply-chain / integrity risks: 1. Approval drift (TOCTOU): flags PRs merged since the last release with no human approval, or where commits landed after the last human approval — the window where an auto-merge or an injected review comment could ship code the human never actually reviewed. 2. Supply-chain dependency diff: diffs uv.lock, then checks new/bumped packages against OSV for known vulnerabilities and flags any resolved from a non-PyPI source (git/url/alt-index — dependency-confusion surface). Deterministic by design: the scanners only read git history and query read-only APIs (GitHub REST, OSV). They never install or execute the code under review, so the gate itself is not an injection target — hence plain pull_request (not pull_request_target) and no extra secrets. Co-authored-by: smolpaws <engel@enyst.org>
Co-authored-by: smolpaws <engel@enyst.org>
|
@OpenHands Do a /codereview on this PR. Post your feedback as a review with gh api. |
|
I'm on it! enyst can track my progress at all-hands.dev |
enyst
left a comment
There was a problem hiding this comment.
🟡 Code Review — Taste Rating: Acceptable
Solid, well-scoped work. The design is genuinely good: both scanners are deterministic (read git history + read-only APIs, never execute PR code), fail safe (network/OSV errors and un-auditable PRs degrade to warnings, not blockers), and the pull_request-not-pull_request_target choice is correct and well-justified. I fetched the branch and ran both scanners against this repo's real history:
check_dependency_diff.pyoverv1.32.0..HEAD→ ✅ no findings, correctly identifies the 4 first-party version bumps (exit 0).check_approval_drift.pyoverv1.33.0..HEAD→ ✅ 6 PRs, 6 clean, 0 flagged (exit 0).
Both match the evidence in the PR description. actions/checkout@v7 and actions/setup-python@v6 both exist and match the repo's dominant pinning (41× checkout@v7, 9× setup-python@v6).
The blocking-vs-acceptable items below are about robustness and test coverage, not fundamental design.
[TESTING GAPS] — the main thing to address
This repo has a clear, established convention: .github/scripts/check_*.py scripts get real unit tests in tests/cross/ — e.g. tests/cross/test_check_version_bumps.py and tests/cross/test_check_deprecations.py, which exercise the real logic against temp git repos and real parsing (not mocks). This PR adds three new scripts with pure, highly-testable functions (_load_lock, _is_trusted_registry, merged_pr_numbers, audit_pr split from I/O) and ships zero tests.
For a security gate, the parsing/classification logic is exactly what must not silently regress. Please add tests/cross/test_check_dependency_diff.py and tests/cross/test_check_approval_drift.py following the existing pattern, covering at minimum:
_load_lockdiffing (added / bumped / removed) from two lockfile revisions in a temp repo;_is_trusted_registryfor{registry: pypi},{editable},{git},{url}, and the empty/missing-source case (see the robustness note below);merged_pr_numbersextraction, including subjects that do not match(#N);audit_prstatus derivation (no-human-approval,changed-after-approval,ok,trusted-bot) from small fixture dicts — no network needed since it's pure once the two API payloads are passed in.
The PR description evidence (real commands + outputs) is good and appreciated, but per this repo's convention it doesn't substitute for the missing tests.
[IMPROVEMENT OPPORTUNITIES]
1. _is_trusted_registry is brittle and can produce release-blocking false positives — .github/scripts/check_dependency_diff.py L69–76.
It trusts a package only if source["registry"] contains the substring "pypi.org", else (for non-registry dicts) only virtual/editable. Two concerns:
- I verified this repo's current
uv.lock: all 413 packages carry either{registry: "https://pypi.org/simple"}(409) or{editable: ...}(4), so it works today. But uv can omit thesourcetable for default-index packages (or emit a mirror/alt-index registry) depending on[tool.uv]index config. If that ever happens, every normal added/bumped dep falls through to "non-PyPI source" →report.block(...)→ the release gate blocks on ordinary PyPI packages. That's a false-positive that fails closed on the wrong signal. "pypi.org" in registryis a substring match, sohttps://pypi.org.evil.example/simplewould be treated as trusted. An==/host-parse against the known-good index URL(s) is safer and clearer.
Suggest: match the registry host exactly (allowlist the canonical PyPI simple URL), and decide deliberately what an empty/missing source means — treat it as default registry = trusted rather than unknown = block, or at least emit a warning instead of a hard block for the "unrecognized source shape" case.
2. merged_pr_numbers can silently under-report — a false negative in a security gate — .github/scripts/check_approval_drift.py L48–61.
Coverage depends on --first-parent + the squash-merge subject ending in (#N). Any PR merged without that convention (a rebase/merge-commit merge, or a squash whose title lost the (#N) suffix) is skipped entirely and never audited. For an approval-drift guard, a skipped merge is exactly the merge you'd want to catch. Consider cross-checking the count of first-parent commits in range against the number that matched, and surfacing "N commits in range could not be mapped to a PR" as a warning so the blind spot is visible rather than silent.
[STYLE / MINOR — non-blocking]
- Comment posting is fire-and-new every time (
security-scan.ymlL82–108): eachlabeledevent POSTs a fresh issue comment with no upsert/dedup. Re-adding the label (or multiple runs) spams duplicate comments. Consider updating a single marker comment (find-by-marker → PATCH) as some of the existing labeled suites may already do. workflow_dispatchproduces no visible artifact: the "Post results" step is gated ongithub.event_name == 'pull_request', and the report is written tosecurity-report.mdin the workspace, not$GITHUB_STEP_SUMMARY. On manual dispatch you only get raw logs. Writing the rendered report to$GITHUB_STEP_SUMMARYwould make both trigger paths legible.- Fork-PR caveat (
security-scan.ymlL82–108): on apull_requestfrom a fork,GITHUB_TOKENis read-only regardless of thepermissions:block, so thecurl -sSfcomment POST 403s and (nocontinue-on-error) fails the job. Not a problem for the intended use — release PRs are cut from internal branches — but worth a one-line note/guard so a stray label on a fork PR doesn't produce a confusing red check.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟢 LOW
New opt-in CI job, label-gated, additive only (639 insertions, 0 deletions), touches no product/runtime code. Scanners are read-only and never execute PR content. Worst realistic failure mode is a false-positive block from the _is_trusted_registry edge case (fails closed — annoying, not dangerous) or a false-negative skip in merged_pr_numbers (a coverage gap in a brand-new, not-yet-relied-upon gate). No secrets beyond the default token. The PR itself already flags it isn't dogfooded on a live release yet — enabling it on the next release cut before treating it as a hard gate is the right call.
VERDICT: ✅ Worth merging once the tests are added (this repo's own convention) and the _is_trusted_registry missing/mirror-source case is made deliberate. The two [IMPROVEMENT OPPORTUNITIES] are robustness fixes, not redesigns.
KEY INSIGHT: The design is sound and fails safe; the remaining risk is entirely in two input-classification assumptions (uv.lock source shape, and the squash (#N) merge convention) that are correct for this repo today but silently degrade the gate if either drifts — which is precisely why the missing tests/cross/ coverage matters most.
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:
- Add a
.agents/skills/custom-codereview-guide.mdfile to your branch (or edit it if one already exists) with the/codereviewtrigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.- Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
- When your PR is merged, the guideline file goes through normal code review by repository maintainers.
Resolve with AI? Install the iterate skill in your agent and run
/iterateto automatically drive this PR through CI, review, and QA until it's merge-ready.Was this review helpful? React with 👍 or 👎 to give feedback.
This review was generated by an AI agent (OpenHands) on behalf of the requester.
|
Done. I completed the SummaryI fetched PR #4042 (from the Review posted: #4042 (review) (event Verdict: 🟡 Acceptable — ✅ Worth mergingThe design is genuinely good: deterministic, read-only, fails safe (network/OSV errors → warnings, not blockers), and correctly uses Substantive feedback raised:
Risk: 🟢 LOW — additive, opt-in, label-gated CI only; no product code; no secrets beyond the default token. I added the standard AI-disclosure note to the review. |
Address review on OpenHands#4042: - check_dependency_diff: exact-host registry allowlist (pypi.org / files.pythonhosted.org) replacing the substring match, so a lookalike host (pypi.org.evil.example) and any mirror/alt-index/git/url/unknown source fail closed and block. OSV lookup failure now blocks instead of warns — a release diff that can't be verified shouldn't ship; a human re-runs or clears it. - check_approval_drift: merged_pr_numbers returns (matched, unmapped) and the report surfaces "N commits could not be mapped to a PR" so a merge that skips the (#N) squash convention is a visible blind spot, not a silent false negative. - security-scan.yml: always-on for release/** PRs; comment upsert by marker (no duplicate spam); write the report to $GITHUB_STEP_SUMMARY so workflow_dispatch is legible; guard the comment POST to non-fork PRs. - Add tests/cross/test_check_dependency_diff.py and test_check_approval_drift.py covering the source classifier and the matched/unmapped split (repo convention: tests/cross for .github/scripts). Co-authored-by: smolpaws <engel@enyst.org>
|
Pushed Robustness (the two improvement points):
Deliberate design call (flags a divergence from the review): the review suggested degrading the unknown-source and OSV-failure cases to warnings (fail-safe). I went fail-closed instead — both now block. On a release gate, "can't verify ⇒ don't ship" is the safer default, and a BLOCKED is cheap: it just parks the release for a human to look at and re-run/clear on the fly. Updated the PR body to match. (Transient GitHub-API errors in approval-drift stay warnings, so a flaky call doesn't wedge a release.) Style notes: comment POST is now upsert-by-marker (no dup spam), guarded to non-fork PRs, and the report always goes to Tests (the merge condition): added |
|
@OpenHands Do a /codereview on this PR. Post your feedback as a review with gh api. Feel free to read all previous discussions / inline threads here too. |
|
I'm on it! enyst can track my progress at all-hands.dev |
enyst
left a comment
There was a problem hiding this comment.
🟢 Code Review (re-review of 48e37cc) — Taste Rating: Good taste
The three items from the previous review are all addressed, and I re-verified each against this repo's real state rather than taking the reply at face value:
Verification I ran on the current branch:
- Tests exist and pass —
tests/cross/test_check_approval_drift.py+tests/cross/test_check_dependency_diff.py, 17 passed. They exercise the real pure logic (merged_pr_numbersmatched/unmapped split,_is_trusted_registry,_registry_host,_source_label,_is_bot) with no mocks-as-tests — exactly the parsing/classification surface a security gate must not silently regress. This closes the main gap from last round. - Supply-chain diff over
v1.32.0..HEAD→ ✅ no findings, 4 first-party bumps, live OSV query succeeded (exit 0). - Approval-drift over
v1.33.0..HEAD→ 6 PRs, 6 clean, 0 flagged, and the new unmapped-commit warning correctly surfaces the 3 not-yet-squash-merged commits in range as a visible blind-spot warning (exit 0, non-blocking) rather than silently dropping them. - Exact-host allowlist fix confirmed:
_is_trusted_registry({"registry": "https://pypi.org.evil.example/simple"})→False; mirror / git / url / empty all block;editable/virtualstay trusted. I also re-checked the liveuv.lock: 409registry+ 4editable, zero<no source>packages, so the deliberate "unknown source fails closed" decision won't false-positive on ordinary PyPI deps today. - Workflow hardening confirmed: comment step is now an upsert-by-marker (
<!-- release-security-scan -->), guarded onhead.repo.full_name == github.repositoryso fork PRs skip the POST instead of 403-ing, and the full report is always written to$GITHUB_STEP_SUMMARY.actions/checkout@v7/actions/setup-python@v6match the repo's dominant pinning.
I want to explicitly endorse the one place smolpaws deliberately went against the prior suggestion: making an unknown/empty uv.lock source fail closed (block) rather than defaulting to trusted. On a release diff that's the correct call — an unclassifiable source is precisely the supply-chain signal, and a block is cheap (it parks the release for a maintainer) whereas silent trust is the dangerous default. Good taste.
[IMPROVEMENT OPPORTUNITIES] — non-blocking
1. Reviews are fetched as a single un-paginated page — .github/scripts/check_approval_drift.py L88–89.
audit_pr requests /pulls/{number}/reviews?per_page=100 with no pagination loop. The reviews endpoint returns every review object (COMMENTED included, not just APPROVED), so a heavily-discussed PR can exceed 100. If the human APPROVED review sits past the first 100, the PR is misclassified as no-human-approval → a false-positive block. Rare, and it fails safe (blocks a clean release rather than passing a dirty one), but on a security gate it's worth following the Link: rel="next" header or looping until a short page. Not blocking for merge.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟢 LOW
New opt-in/label-gated CI job, additive only (946 insertions, 0 deletions), no product/runtime code touched. Scanners are read-only, never execute PR content, and use pull_request (not pull_request_target) with only the default GITHUB_TOKEN. Both fail safe (network/OSV errors and un-auditable PRs degrade to warnings/blocks, never to a silent pass). Worst realistic failure is a false-positive block (annoying, not dangerous) from the pagination edge above. The PR is honest that it isn't dogfooded on a live release yet — enabling it on the next release cut before treating it as a hard gate remains the right sequencing.
VERDICT: ✅ Worth merging. The prior round's testing gap and both robustness items are genuinely fixed and verified against real history. The single remaining note (reviews pagination) is a minor, fail-safe edge case that can land here or as a follow-up.
KEY INSIGHT: The gate's integrity now rests on well-tested, deterministic input classification, and every failure mode is fail-closed/fail-safe — the design's original strength, now backed by the tests/cross/ coverage that makes those assumptions regression-proof.
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:
- Add a
.agents/skills/custom-codereview-guide.mdfile to your branch (or edit it if one already exists) with the/codereviewtrigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.- Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
- When your PR is merged, the guideline file goes through normal code review by repository maintainers.
Resolve with AI? Install the iterate skill in your agent and run
/iterateto automatically drive this PR through CI, review, and QA until it's merge-ready.Was this review helpful? React with 👍 or 👎 to give feedback.
This review was generated by an AI agent (OpenHands) on behalf of the requester.
|
Done. I ran a fresh Review posted: #4042 (review) (event What I didI fetched the PR from the
Verdict🟢 Good taste / ✅ Worth merging. All prior-round items are genuinely fixed and verified. I raised one new, non-blocking minor note: No code was modified — this was a review-only task. |
Address re-review on OpenHands#4042: audit_pr fetched /pulls/{n}/reviews with a single per_page=100 page. The reviews endpoint returns every review object (COMMENTED included), so on a heavily-discussed PR a human APPROVED past the first page would be missed and the PR misclassified as no-human-approval — a false-positive release block. - Add github_request_all() to security_scan_common: follows the Link: rel="next" header and concatenates all pages; a non-list response is wrapped as a single item. - audit_pr now uses it for the reviews fetch. - Add tests/cross/test_security_scan_common.py covering _next_link parsing and multi-page / single-page / non-list pagination. Co-authored-by: smolpaws <engel@enyst.org>
|
Fixed the pagination edge case in What changed: Tests: new You called it right — it fails safe, but on a security gate a silent false block is worth closing. 🐾 |
|
@OpenHands Do a /codereview on this PR. Post your feedback as a review with gh api. Feel free to read all previous discussions / inline threads here too. Post as review with approved or comment or request changes as you think fit. |
|
@enyst it looks like you haven't created an OpenHands account yet. Please sign up at OpenHands Cloud and try again. |
|
I'm on it! enyst can track my progress at all-hands.dev |
enyst
left a comment
There was a problem hiding this comment.
🟢 Code Review (re-review of e9a989e) — Taste Rating: Good taste
This is the third pass, focused on the one commit that landed after the last 🟢 re-review: e9a989e — "paginate PR reviews to avoid false-positive block." I read all six changed files at PR head, re-read the two prior reviews and the three (now-resolved) inline threads so I'm not re-litigating settled feedback, and verified every claim against this repo's real history rather than taking the description at face value.
What I verified on the current branch (e9a989e)
- Tests pass —
tests/cross/test_check_approval_drift.py,test_check_dependency_diff.py, and the newtest_security_scan_common.py→ 23 passed. They exercise the real pure logic (pagination_next_link/github_request_allmulti-page + non-list wrapping,merged_pr_numbersmatched/unmapped split,_is_trusted_registry,_registry_host,_source_label,_is_bot) with no mocks-as-tests — exactly the parsing/classification surface a security gate must not silently regress. - Supply-chain diff over
v1.32.0..HEAD→ ✅ no findings; correctly identifies the 4 first-party version bumps; live OSV query succeeded (exit 0). - Approval-drift over
v1.33.0..HEAD→ 6 PRs, 6 clean, 0 flagged, 0 un-auditable; the unmapped-commit warning correctly surfaces this PR's own 4 not-yet-squash-merged commits as a visible blind-spot warning (exit 0, non-blocking). - Pagination fix is correct.
audit_prnow callsgithub_request_all(.../reviews?per_page=100), which followsLink: rel="next"and concatenates pages. This closes a genuine false-positive: the/reviewsendpoint returns every review object (COMMENTED included), so a humanAPPROVEDpast the first 100 would previously be missed and the PR misclassified asno-human-approval— a spurious release block. The refactor cleanly splits_github_get(one page + Link header) fromgithub_request(single) /github_request_all(paginated), and the non-list-response guard means an error object is never iterated element-wise. - Lint clean —
ruff checkandruff format --checkpass on all three scripts and the test files. - Pinned actions
actions/checkout@v7andactions/setup-python@v6exist and match the repo's dominant pinning (41× checkout@v7, 9× setup-python@v6).
Design remains sound
Both scanners are deterministic (read git history + read-only GitHub REST / OSV, never execute PR code), which correctly justifies plain pull_request over pull_request_target and no secrets beyond the default GITHUB_TOKEN. The fail-closed choices (OSV lookup failure and unrecognized dep source both block; transient per-PR API errors degrade to warnings so a flaky call can't wedge a release) are deliberate, documented, and owner-endorsed. No data-structure smells, no >3-level nesting, no breaking changes to existing workflows.
One minor, non-blocking note (no action required to merge)
The opt-in path (behavior B: a security-scan label on a non-release/** PR) runs on the labeled event but not on subsequent synchronize pushes — so a labeled feature PR that gets new commits won't automatically re-scan until re-labeled or re-dispatched. This is fine because the actual gate (behavior A, release/**) does re-run on synchronize, and behavior B is explicitly "best-effort opt-in." Flagging only so it's a conscious choice; not worth changing here.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟢 LOW
New CI-only, opt-in/label-gated workflow plus three read-only stdlib scripts and their tests. No production code paths, no runtime dependencies added, no execution of PR-controlled content, no new secrets. Worst-case failure mode is a spurious release-gate block that a maintainer re-runs or clears — no data exposure or privilege surface. Not yet dogfooded on a live release PR (author notes this); enabling on the next release cut is the natural validation.
VERDICT:
✅ Worth merging — core logic is sound, well-tested against real history, and all prior review feedback is addressed and verified.
KEY INSIGHT:
Auditing the release diff (last tag → PR) as one unit, with read-only deterministic scanners, is the right place and the right mechanism to catch approval-drift/TOCTOU and supply-chain risk without turning the gate itself into an injection target.
Reviewed by an AI agent (OpenHands) on behalf of @enyst.
|
Done. I ran a fresh Review posted: #4042 (review) (event APPROVED) What I did
Verification (all against the current branch)
Verdict🟢 Good taste — Approved. Deterministic, read-only, well-tested; all prior feedback addressed. Risk: 🟢 LOW (CI-only, opt-in/label-gated, no PR-code execution, no new secrets/deps). I left one minor non-blocking note: the label-based opt-in path runs on The review was posted with an AI-disclosure line. No code changes were needed. |
HUMAN:
I asked smolpaws to add a fourth release-PR check for security: an approval-drift (TOCTOU) guard and a supply-chain dependency diff. Starting with these two deterministic checks; more to follow.
AGENT:
Why
We run three labeled suites on release PRs today —
integration-test,behavior-test,test-examples. This adds a fourth,security-scan, for a gap those don't cover: the integrity of what actually ships versus what a human reviewed.As agents (ours included) get faster and some PRs start merging with lighter or automated review, a real window opens up:
main.This is a classic TOCTOU (time-of-check → time-of-use) gap. Cutting a release from that
maininherits it. The right place to catch it is the release diff — last release tag → this PR — audited as one unit.Summary
.github/workflows/security-scan.yml: a new opt-in job gated on asecurity-scanlabel (plusworkflow_dispatch), mirroring the existing labeled-suite pattern. Posts one PR comment and fails on any blocking finding.check_approval_drift.py(TOCTOU): for every PR merged since the lastv*tag, flags those merged with no human approval, or where commits landed after the last human approval (reviewed sha ≠ merged sha). Trusted bots (e.g.dependabot[bot]) are configurable and exempt.check_dependency_diff.py(supply chain): diffsuv.lock, checks new/bumped packages against OSV for known vulns, and flags any resolved from a non-PyPI source (git/url/alt-index — dependency-confusion surface).security_scan_common.pyshared helpers. Deterministic by design: the scanners only read git history and query read-only APIs (GitHub REST, OSV); they never install or execute PR code, so the gate itself is not an injection target — hence plainpull_request(notpull_request_target) and no secrets beyond the defaultGITHUB_TOKEN. An OSV lookup failure blocks (fail-closed: a release diff that can't be verified against known vulns shouldn't ship — a maintainer re-runs or clears it), as does any non-PyPI / mirror / unrecognized source. Un-auditable PRs in the approval-drift check (transient GitHub API errors) remain warnings so a flaky call doesn't wedge a release.Issue Number
N/A (follow-up to the OpenHands/release-actions#18 freeze-on-ready discussion).
How to Test
Run either scanner locally against real history:
Or add the
security-scanlabel to a PR / trigger viaworkflow_dispatch.Verified locally against this repo's real history (evidence):
check_dependency_diff.pyoverv1.28.0..HEADcorrectly surfaced a known-vulnerable transitivestarlette==1.0.1with real advisory IDs (GHSA-82w8-qh3p-5jfq, GHSA-jp82-jpqv-5vv3, GHSA-wqp7-x3pw-xc5r, GHSA-x746-7m8f-x49c, PYSEC-2026-248, PYSEC-2026-249) and listed 9 added / 11 bumped deps; overv1.32.0..HEADit reports ✅ no findings (only the SDK's own version bumps).check_approval_drift.pyoverv1.31.0..HEADflagged a realchanged-after-approvalmerge (fix(profiles): deny-list skill selection (disabled_skills) + drop embedded skills #4018: last approval by @neubig on1986d9562, merged head780de42de), exit code 1; overv1.33.0..HEADit reports ✅ no findings across 6 PRs.ruff checkandruff formatclean on all three scripts.Type
Notes
Opened by 🐾 smolpaws on Engel's behalf.