fix(security): fail closed when dependency-review compare is unavailable - #1041
fix(security): fail closed when dependency-review compare is unavailable#1041seonghobae wants to merge 6 commits into
Conversation
Replay unique #897 source onto current origin/main. Skip shared ARCHITECTURE/CLAUDE trees. Treat non-200 or failed transport as unavailable evidence rather than a clean skip.
Close the remaining #810 diagnostic gap: allowlist public/private/internal/unknown visibility in probe diagnostics, execute 403/404/empty/malformed regressions, and stop making the pinned action independently skippable after a successful probe. Co-authored-by: Seongho Bae <[email protected]>
Add executable regressions for the EgressWeave #66 canary (HTTP 403 skip-was-success), a bare transport failure, and curl exit 18 with a printed 200. Record exact SHAs and allowlisted visibility without leaking the probe token. Co-authored-by: Seongho Bae <[email protected]>
|
Important Review available on request
Reviews should be triggered manually for repositories with fewer than 10 stars. Select Trigger review above or comment ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…arness Invoke the extracted support probe with an absolute bash path and keep the fake curl first on PATH so isolated executable regressions can run without calling the real binary. Co-authored-by: Seongho Bae <[email protected]>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict
COMMENT on exact head fbb8432345ac6565724668afdbf9e3259475b94b.
The 403/404/transport fail-closed path, allowlisted visibility, exact-head checkout, discarded bodies, and unskippable pinned action are source-correct for #810. This draft still interpolates unvalidated BASE_SHA, HEAD_SHA, and REPOSITORY into the compare URL, so an empty revision or ../ repository can mint supported=true from a forged HTTP 200. The success test also does not record curl argv, so a hardcoded supported=true would pass.
Prefer #1045 (6a7103a4) for integration. It rejects malformed identity before the socket, records the exact compare argv, and keeps the pinned action ungated. Keep this draft and #897/#1033/#1042 out of the merge queue. Keep #810 open until a protected-main public consumer run proves a non-200 cannot go green.
Do not self-approve. Do not merge this draft.
Sent by Cursor Automation: Fix Issues
| public|private|internal) visibility="${REPOSITORY_VISIBILITY}" ;; | ||
| *) visibility="unknown" ;; | ||
| esac | ||
| set +e |
There was a problem hiding this comment.
The probe starts curl immediately after the visibility allowlist. An empty BASE_SHA, a 39-character hex string, or ../ in HEAD_SHA is interpolated into /dependency-graph/compare/{base}...{head}. The executable harness then returns HTTP 200 and the step writes supported=true.
Reject 40- or 64-character hex revisions and canonical owner/name before set +e, and fail with HTTP unavailable / curl exit uncalled. Landed on #1045.
| combined = f"{result.stdout}{result.stderr}" | ||
| assert result.returncode == 0 | ||
| assert (tmp_path / "github-output").read_text(encoding="utf-8") == ( | ||
| "supported=true\n" |
There was a problem hiding this comment.
This success contract only checks return code and supported=true. It does not record curl argv, so a probe that skips the compare URL and hardcodes the output still passes.
Record the fake curl argv and require https://api.example.invalid/repos/{owner}/{name}/dependency-graph/compare/{base}...{head} plus -o /dev/null. Landed on #1045.
There was a problem hiding this comment.
Verdict
COMMENT on exact head 6a7103a48a814ab4892cf9a4aabc9c07177c8147.
The pre-network 40/64-hex and owner/name checks close the forged-200 hole that blocked #1041. A remaining identity case still matches that regex: ContextualWisdomLab/.. is syntactically owner/name. Named refs such as main are also rejected only as non-hex, and rejection logs still echoed the raw values.
Prefer #1049, which continues this head, rejects ./.. segments, treats curl 000 as unavailable, and stops echoing raw invalid identity. Do not merge #1045. Keep #810 open until a protected-main public consumer run proves a non-200 cannot go green.
Sent by Cursor Automation: Fix Issues
…pare Validate 40- or 64-character hex revisions and canonical owner/name without . or .. segments before opening a socket. Treat curl's 000 no-status sentinel as unavailable. Identity failures name the class only and do not echo raw invalid values. Record compare argv so a hardcoded supported=true path cannot satisfy the success contract. Co-authored-by: Seongho Bae <[email protected]>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
Keep malformed-revision and malformed-repository errors from interpolating untrusted BASE_SHA, HEAD_SHA, or REPOSITORY into the Actions annotation. A new head also retires the stale same-SHA Noema failure on #1041. Co-authored-by: Seongho Bae <[email protected]>
2 participants
Purpose
Close the fail-open hole in the org-required Security Scan
dependency-reviewjob tracked by #810. Current protectedmain(c47afc2d) still probesGET /repos/{repo}/dependency-graph/compare/{base}...{head}, treats HTTP 403/404 assupported=false, skipsactions/dependency-review-action, and exits 0.This is the single integration vehicle for that repair. It continues #1033 and supersedes #897 (which still left the action independently skippable via
if: supported == 'true'). It also absorbs the identity-validation follow-up that #1045 started, including the later review findings on./..segments, HTTP000, and not echoing raw invalid values.Canary (do not treat as fixed until a post-merge consumer run)
ContextualWisdomLab/EgressWeave#66, Security Scan run
31108241013, job92638903658:10d0c51daf2ad278d66f43be479df8cf6b08ba6d...c038a9509d1a8eae8561cc9081e67e12bd373d42curl: (22) The requested URL returned error: 403Dependency review is unavailable for ContextualWisdomLab/EgressWeave; skipping dependency-review hard gate.Downstream: ContextualWisdomLab/EgressWeave#76.
What changed
0plus exact HTTP200may reach the pinnedactions/dependency-review-action@a1d282b36b6f3519aa1f3fc636f609c47dddb294(fail-on-severity: moderate, unchanged).000, transport failure (including curl exit 18 with a printed 200), and timeouts fail the job.owner/namewithout./..segments, before curl runs (curl exituncalled). Identity failures name the class only and do not echo raw invalid values.Tests
Local
pytest tests/test_required_workflow_queue_contract.py: 72 passed.Executable regressions cover the EgressWeave #66 403 skip-was-success canary, 404/empty/malformed/
000, transport failure, curl exit 18 + printed 200, malformed identity before curl, and argv recording on the success path.CI note
The required Noema Review failure on
22a23981(run32046375681) was a same-SHA flake: a laterpull_request_targetrun on that head succeeded, and this head (051ba5e0) retires that stale check. Noema is apull_request_targetgate from trustedmainand is not part of this workflow change.Acceptance
Do not close #810 from this change alone. Keep it open until a protected-main public-repository consumer run proves a non-200 or failed-transfer comparison cannot produce a green Dependency Review gate.