Skip to content

ci: add release security-scan#4042

Merged
enyst merged 5 commits into
OpenHands:mainfrom
smolpaws:security-scan-release-workflow
Jul 9, 2026
Merged

ci: add release security-scan#4042
enyst merged 5 commits into
OpenHands:mainfrom
smolpaws:security-scan-release-workflow

Conversation

@smolpaws

@smolpaws smolpaws commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

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:

  • a PR is approved, then commits land after the approval and still get merged;
  • a PR is merged with no human approval at all;
  • a review/QA agent runs while an injected comment sits on the PR, and whatever it did ends up on main.

This is a classic TOCTOU (time-of-check → time-of-use) gap. Cutting a release from that main inherits it. The right place to catch it is the release diff — last release tag → this PR — audited as one unit.

Summary

  • Adds .github/workflows/security-scan.yml: a new opt-in job gated on a security-scan label (plus workflow_dispatch), mirroring the existing labeled-suite pattern. Posts one PR comment and fails on any blocking finding.
  • Adds check_approval_drift.py (TOCTOU): for every PR merged since the last v* 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.
  • Adds check_dependency_diff.py (supply chain): diffs uv.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).
  • Adds security_scan_common.py shared 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 plain pull_request (not pull_request_target) and no secrets beyond the default GITHUB_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:

# Supply-chain diff (OSV + non-PyPI source), stdlib only
SECURITY_SCAN_BASELINE_TAG=v1.28.0 python .github/scripts/check_dependency_diff.py

# Approval drift (needs a token to read reviews)
GITHUB_TOKEN=<token> GITHUB_REPOSITORY=OpenHands/software-agent-sdk \
  SECURITY_SCAN_BASELINE_TAG=v1.31.0 python .github/scripts/check_approval_drift.py

Or add the security-scan label to a PR / trigger via workflow_dispatch.

Verified locally against this repo's real history (evidence):

  • check_dependency_diff.py over v1.28.0..HEAD correctly surfaced a known-vulnerable transitive starlette==1.0.1 with 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; over v1.32.0..HEAD it reports ✅ no findings (only the SDK's own version bumps).
  • check_approval_drift.py over v1.31.0..HEAD flagged a real changed-after-approval merge (fix(profiles): deny-list skill selection (disabled_skills) + drop embedded skills #4018: last approval by @neubig on 1986d9562, merged head 780de42de), exit code 1; over v1.33.0..HEAD it reports ✅ no findings across 6 PRs.
  • ruff check and ruff format clean on all three scripts.

Type

  • Feature (CI)

Notes

  • Scoped to the two deterministic checks we agreed to start with. Natural follow-ups (separate PRs): a prompt-injection / agent-influence scan of PR content + surrounding comments; a general static "weirdness" pass (egress / dangerous-exec / CI-workflow tampering); and running the scanners from a trusted ref rather than PR head for extra hardening.
  • Not yet dogfooded on a live release PR — suggest enabling on the next release cut (or a follow-up integration test) before relying on it as a gate.

Opened by 🐾 smolpaws on Engel's behalf.

enyst and others added 2 commits July 9, 2026 02:47
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>
@enyst enyst added the qa-this label Jul 9, 2026
@enyst
enyst requested a review from all-hands-bot July 9, 2026 01:15
@enyst

enyst commented Jul 9, 2026

Copy link
Copy Markdown
Member

@OpenHands Do a /codereview on this PR. Post your feedback as a review with gh api.

@openhands-ai

openhands-ai Bot commented Jul 9, 2026

Copy link
Copy Markdown

I'm on it! enyst can track my progress at all-hands.dev

@enyst enyst left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.py over v1.32.0..HEAD → ✅ no findings, correctly identifies the 4 first-party version bumps (exit 0).
  • check_approval_drift.py over v1.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_lock diffing (added / bumped / removed) from two lockfile revisions in a temp repo;
  • _is_trusted_registry for {registry: pypi}, {editable}, {git}, {url}, and the empty/missing-source case (see the robustness note below);
  • merged_pr_numbers extraction, including subjects that do not match (#N);
  • audit_pr status 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 the source table 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 registry is a substring match, so https://pypi.org.evil.example/simple would 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.yml L82–108): each labeled event 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_dispatch produces no visible artifact: the "Post results" step is gated on github.event_name == 'pull_request', and the report is written to security-report.md in the workspace, not $GITHUB_STEP_SUMMARY. On manual dispatch you only get raw logs. Writing the rendered report to $GITHUB_STEP_SUMMARY would make both trigger paths legible.
  • Fork-PR caveat (security-scan.yml L82–108): on a pull_request from a fork, GITHUB_TOKEN is read-only regardless of the permissions: block, so the curl -sSf comment POST 403s and (no continue-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:

  1. Add a .agents/skills/custom-codereview-guide.md file to your branch (or edit it if one already exists) with the /codereview trigger 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.
  2. Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
  3. 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 /iterate to 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.

Comment thread .github/scripts/check_dependency_diff.py Outdated
Comment thread .github/scripts/check_approval_drift.py
Comment thread .github/workflows/security-scan.yml Outdated
@openhands-ai

openhands-ai Bot commented Jul 9, 2026

Copy link
Copy Markdown

Done. I completed the /codereview and posted it as a PR review via gh api.

Summary

I fetched PR #4042 (from the smolpaws fork), read all four changed files, and verified the code against this repo's real history before writing the review — running both scanners, inspecting the actual uv.lock source format, and confirming the pinned action versions exist.

Review posted: #4042 (review) (event COMMENT, with a summary body + 3 inline comments anchored to verified lines).

Verdict: 🟡 Acceptable — ✅ Worth merging

The design is genuinely good: deterministic, read-only, fails safe (network/OSV errors → warnings, not blockers), and correctly uses pull_request rather than pull_request_target. Both scanners ran clean against real history, matching the PR's stated evidence.

Substantive feedback raised:

  1. Testing gap (main point) — the repo has an established convention (tests/cross/test_check_*.py, e.g. test_check_version_bumps.py) of testing these .github/scripts/check_*.py scripts with real logic against temp git repos. This PR ships three new scripts with pure, testable functions and zero tests. For a security gate, that classification logic is exactly what shouldn't silently regress.
  2. _is_trusted_registry brittleness — trusts on a "pypi.org" substring; correct for the current lock (verified: 409/413 registry, 4 editable) but can produce release-blocking false positives if uv ever omits/mirrors the source, and the substring match is spoofable.
  3. merged_pr_numbers false negatives — reliance on --first-parent + the (#N) squash convention silently skips any PR that doesn't match, which in an approval-drift guard is the merge you'd most want to catch.
  4. Minor/non-blocking — duplicate comments per label event, no $GITHUB_STEP_SUMMARY output on workflow_dispatch, and a fork-PR read-only-token caveat that would 403 the comment step.

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>
@smolpaws

smolpaws commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

Pushed 48e37cc addressing the review 🐾

Robustness (the two improvement points):

  • Exact-host registry allowlistpypi.org / files.pythonhosted.org matched on parsed hostname, not substring, so pypi.org.evil.example no longer slips through; mirror / alt-index / git / url / unknown source all block.
  • Approval-drift blind spot surfacedmerged_pr_numbers returns (matched, unmapped); commits that don't fit the (#N) squash convention are now reported as a visible warning instead of silently skipped.

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 $GITHUB_STEP_SUMMARY so workflow_dispatch/fork runs are legible.

Tests (the merge condition): added tests/cross/test_check_dependency_diff.py + test_check_approval_drift.py — 17 tests over the source classifier and the matched/unmapped split, following the tests/cross/ convention. ruff check/format clean; both scanners still run green against this repo's real v1.33.0..HEAD.

@enyst

enyst commented Jul 9, 2026

Copy link
Copy Markdown
Member

@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.

@openhands-ai

openhands-ai Bot commented Jul 9, 2026

Copy link
Copy Markdown

I'm on it! enyst can track my progress at all-hands.dev

@enyst enyst left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 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 passtests/cross/test_check_approval_drift.py + tests/cross/test_check_dependency_diff.py, 17 passed. They exercise the real pure logic (merged_pr_numbers matched/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/virtual stay trusted. I also re-checked the live uv.lock: 409 registry + 4 editable, 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 on head.repo.full_name == github.repository so 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@v6 match 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:

  1. Add a .agents/skills/custom-codereview-guide.md file to your branch (or edit it if one already exists) with the /codereview trigger 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.
  2. Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
  3. 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 /iterate to 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.

@openhands-ai

openhands-ai Bot commented Jul 9, 2026

Copy link
Copy Markdown

Done. I ran a fresh /codereview on PR #4042 (re-review of 48e37cc) and posted it as a PR review via gh api.

Review posted: #4042 (review) (event COMMENT)

What I did

I fetched the PR from the smolpaws fork, read all six changed files, and read the previous review plus all three (now-resolved) inline review threads so I wasn't re-litigating settled feedback. Then I verified every claimed fix against this repo's real state rather than trusting the replies:

  • Tests now exist and passtests/cross/test_check_approval_drift.py + tests/cross/test_check_dependency_diff.py, 17 passed. They exercise real pure logic (no mocks-as-tests), closing the main gap from the prior round.
  • Supply-chain diff v1.32.0..HEAD → ✅ clean, live OSV query succeeded (exit 0).
  • Approval-drift v1.33.0..HEAD → 6 PRs clean; the new unmapped-commit warning correctly surfaces the blind spot (exit 0, non-blocking).
  • Exact-host allowlist fix confirmed: pypi.org.evil.example now blocks; mirror/git/url/empty block; editable/virtual trusted. Live uv.lock is 409 registry + 4 editable, zero unknown-source packages, so the deliberate fail-closed decision won't false-positive today.
  • Workflow hardening confirmed: upsert-by-marker comment, fork-PR guard, $GITHUB_STEP_SUMMARY output; checkout@v7/setup-python@v6 match repo convention.

Verdict

🟢 Good taste / ✅ Worth merging. All prior-round items are genuinely fixed and verified. I raised one new, non-blocking minor note: audit_pr fetches reviews as a single un-paginated per_page=100 page (a PR with >100 review objects could push the human approval off-page → a fail-safe false-positive block). Overall risk: 🟢 LOW (additive opt-in CI job, read-only scanners, no runtime code).

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>
@smolpaws

smolpaws commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

Fixed the pagination edge case in e9a989e — folding it in here rather than a follow-up, since a silently-wrong gate on a big PR is exactly the kind of thing this PR is meant to prevent.

What changed: audit_pr fetched /pulls/{n}/reviews?per_page=100 as a single page. Since that endpoint returns every review object (COMMENTED too), a human APPROVED past the first 100 would be missed → false-positive no-human-approval block. Added github_request_all() to security_scan_common which follows the Link: rel="next" header and concatenates all pages; audit_pr now uses it. A non-list response is wrapped as a single item so an error object isn't iterated element-wise.

Tests: new tests/cross/test_security_scan_common.py covers _next_link parsing and multi-page / single-page / non-list pagination. Full cross suite now 23 passed. Smoke-ran approval-drift over v1.33.0..HEAD with a real token: 6 PRs, 6 clean, exit 0.

You called it right — it fails safe, but on a security gate a silent false block is worth closing. 🐾

@enyst enyst changed the title ci: add release security-scan (approval-drift + supply-chain diff) ci: add release security-scan Jul 9, 2026
@enyst

enyst commented Jul 9, 2026

Copy link
Copy Markdown
Member

@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.

@openhands-fleet-shared-1

Copy link
Copy Markdown

@enyst it looks like you haven't created an OpenHands account yet. Please sign up at OpenHands Cloud and try again.

@openhands-ai

openhands-ai Bot commented Jul 9, 2026

Copy link
Copy Markdown

I'm on it! enyst can track my progress at all-hands.dev

@enyst enyst left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 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 passtests/cross/test_check_approval_drift.py, test_check_dependency_diff.py, and the new test_security_scan_common.py23 passed. They exercise the real pure logic (pagination _next_link / github_request_all multi-page + non-list wrapping, merged_pr_numbers matched/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_pr now calls github_request_all(.../reviews?per_page=100), which follows Link: rel="next" and concatenates pages. This closes a genuine false-positive: the /reviews endpoint returns every review object (COMMENTED included), so a human APPROVED past the first 100 would previously be missed and the PR misclassified as no-human-approval — a spurious release block. The refactor cleanly splits _github_get (one page + Link header) from github_request (single) / github_request_all (paginated), and the non-list-response guard means an error object is never iterated element-wise.
  • Lint cleanruff check and ruff format --check pass on all three scripts and the test files.
  • Pinned actions actions/checkout@v7 and actions/setup-python@v6 exist 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.

@openhands-ai

openhands-ai Bot commented Jul 9, 2026

Copy link
Copy Markdown

Done. I ran a fresh /codereview on PR #4042 (re-review of the latest commit e9a989e) and posted it as a PR review via gh api.

Review posted: #4042 (review) (event APPROVED)

What I did

  • Fetched the PR head from the smolpaws fork, read all six changed files, plus the two prior reviews and the three (now-resolved) inline threads so I wasn't re-litigating settled feedback.
  • Focused on the one commit that landed after the last 🟢 re-review — e9a989e "paginate PR reviews to avoid false-positive block" — and verified every claim against this repo's real history rather than trusting the description.

Verification (all against the current branch)

  • 23 tests pass across the three new tests/cross/ files (no mocks-as-tests; real parsing/classification/pagination logic).
  • Supply-chain diff v1.32.0..HEAD → ✅ no findings, 4 first-party bumps, live OSV query succeeded (exit 0).
  • Approval-drift v1.33.0..HEAD → 6 PRs clean, unmapped-commit blind-spot warning works (exit 0).
  • Pagination fix is correctaudit_pr now follows Link: rel="next", closing the real false-positive where a human APPROVED past the first page would misclassify a PR as unapproved. Clean _github_get/github_request/github_request_all split with a non-list guard.
  • ruff check + format clean; pinned checkout@v7 / setup-python@v6 match the repo's dominant pinning.

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 labeled but not on later synchronize pushes — fine because the real release/** gate does re-run and behavior B is explicitly best-effort.

The review was posted with an AI-disclosure line. No code changes were needed.

Comment thread .github/workflows/security-scan.yml Outdated
@enyst
enyst enabled auto-merge (squash) July 9, 2026 21:46
@enyst
enyst merged commit 0bebe7b into OpenHands:main Jul 9, 2026
27 checks passed
@github-actions github-actions Bot mentioned this pull request Jul 10, 2026
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants