fix(security): fail closed on unavailable dependency review - #1042
fix(security): fail closed on unavailable dependency review#1042seonghobae wants to merge 2 commits into
Conversation
HTTP 403/404, empty or malformed status, and transport failure (including curl exit 18 with a printed 200) no longer skip the Security Scan dependency-review gate. The probe records allowlisted visibility, exact base/head SHAs, HTTP status, and curl exit, and never prints the response body. After HTTP 200 with curl exit 0 the pinned action runs unconditionally. Related to #810. Do not close #810 until an EgressWeave canary proves a non-200 cannot green the job. 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. |
|
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 |
There was a problem hiding this comment.
Stale comment
Verdict
COMMENT.
This ready-for-review copy of the fail-closed probe is weaker than #1044: it still treats curl
000as a three-digit HTTP status, and its probe harness spreads the full process environment into the extracted script.Operator next step
Prefer #1044 for integration. Do not approve or merge this head over that successor. Keep #810 open until a protected-main public consumer run proves a non-200 cannot go green.
Sent by Cursor Automation: Fix Issues
There was a problem hiding this comment.
Stale comment
Verdict
REQUEST_CHANGES on exact head
e69bd3516ee94d42bbe11335effefc5af6c57cba.The HTTP 403/404 and transport fail-closed probe is source-correct, but this ready PR still interpolates unvalidated revisions and repository names into the compare URL. An empty SHA or
../repository can therefore mintsupported=truefrom a forged HTTP 200. The success test does not prove the compare URL was requested.Do not merge this PR. Prefer #1045, which continues the #1041 canary/visibility work and adds pre-network identity validation plus compare-argv proof. Keep #810 open until a protected-main public consumer run proves a non-200 cannot go green.
Sent by Cursor Automation: Fix Issues
| case "${REPOSITORY_VISIBILITY}" in | ||
| public|private|internal) visibility="${REPOSITORY_VISIBILITY}" ;; | ||
| *) visibility="unknown" ;; | ||
| esac |
There was a problem hiding this comment.
Curl runs before any revision or repository-name check. Empty or ../ identity is interpolated into the compare URL and a forged HTTP 200 writes supported=true.
Fail closed with HTTP unavailable and curl exit uncalled unless both SHAs are 40- or 64-character hex and the repository is canonical owner/name. Use #1045.
There was a problem hiding this comment.
Verdict
COMMENT on closed head e69bd351. Do not reopen this pull request.
The production probe on this head is fail-closed for HTTP 403/404, empty or malformed status, and curl exit 18 with a printed 200. That is a real repair versus protected main. It is not the integration vehicle.
Why this head is not the landing vehicle
- The executable contract is a weaker subset of #1048: this head inherits
os.environin the probe harness and does not pin credential non-leakage, the EgressWeave #66 HTTP 403 canary, curl000, or named-ref rejection. - A later residual remains even on #1048:
../.githubandContextualWisdomLab/..still match the single-slashowner/nameregex. RFC 3986 remove-dot-segments would collapse those into a different compare URL.
Operator next step
- Prefer #1050 (
29a20baa) for integration. It continues #1048 and rejects./..path components while keepingContextualWisdomLab/.githublegal. - Keep #810 open until a protected-main public consumer run proves a non-200 cannot green Dependency Review.
- Do not merge #897, #1033, #1041, this pull request, #1044, #1045, or #1048 once #1050 is the integration vehicle.
- Wait only for exact-current-head required checks and a counted non-author formal approval on #1050. Automated review is not merge authority.
Sent by Cursor Automation: Fix Issues
There was a problem hiding this comment.
Verdict
COMMENT on closed head e69bd351. Do not reopen this pull request.
The production probe on this head is fail-closed for HTTP 403/404, empty or malformed status, and curl exit 18 with a printed 200. That is a real repair versus protected main. It is not the integration vehicle.
Why this head is not the landing vehicle
- The executable contract is a weaker subset of later heads: this harness inherits
os.environand does not pin credential non-leakage, the EgressWeave #66 HTTP 403 canary, curl000, named-ref rejection, or./..path-segment rejection. - A concurrent successor already closed those gaps.
Operator next step
- Prefer #1049 (
e4a7dffa) for integration. It rejects./..segments, keepsContextualWisdomLab/.githublegal, hides raw invalid identity, and classifies curl000as unavailable. - Treat #1050 as a historical duplicate of that same slice. Do not merge #1050.
- Keep #810 open until a protected-main public consumer run proves a non-200 cannot green Dependency Review.
- Do not merge #897, #1033, #1041, this pull request, #1044, #1045, #1048, or #1050 once #1049 is the integration vehicle.
- Wait only for exact-current-head required checks and a counted non-author formal approval on #1049. Automated review is not merge authority.
Sent by Cursor Automation: Fix Issues
Reject empty, named, or non-hex revisions and `.`/`..` repository segments before curl so a forged HTTP 200 cannot mint supported=true. Treat curl 000 as unavailable, record argv proof on the success path, and keep raw invalid identity out of logs. Related to #810. Do not close #810 until an EgressWeave canary proves a non-200 cannot green the job. Co-authored-by: Seongho Bae <[email protected]>


Purpose
Close the fail-open merge gate in the org-required Security Scan
dependency-reviewjob (#810). On currentmain, a GitHub compare/dependency-graph probe that returns HTTP 403/404 setssupported=false, skipsactions/dependency-review-action, and exits 0. Live evidence: ContextualWisdomLab/EgressWeave#66, Security Scan run 31108241013, job 92638903658 (compare10d0c51d…c038a950returned HTTP 403 and the job still succeeded). Downstream: ContextualWisdomLab/EgressWeave#76.This head now also closes the CHANGES_REQUESTED gap on #1042: empty, named, or
../identity can no longer be interpolated into the compare URL and mintsupported=truefrom a forged HTTP 200.Do not close #810 from this PR. Leave it open until an EgressWeave (or other protected-main public consumer) canary proves a non-200 or failed-transfer comparison cannot green the Dependency Review job.
What changed
|| true. Curl's transport exit is captured separately from the printed HTTP status.owner/namewithout./..segments (.githubremains legal). Failures use HTTPunavailableand curl exituncalledand do not echo raw identity.000, malformed, or any non-zero curl exit (including exit 18 with a printed200) fails the job.actions/dependency-review-actionruns unconditionally.000, named-ref rejection, dot-segment rejection, argv proof on the success path, and credential non-leakage.Verification
Local (
75d942df):pytest tests/test_required_workflow_queue_contract.py: 75 passedAcceptance
000, named refs, and./..segments