fix(security): reject malformed dependency-review identity before compare - #1045
fix(security): reject malformed dependency-review identity before compare#1045cursor[bot] wants to merge 5 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]>
…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]>
…pare Empty or non-hex base/head revisions and non-canonical repository names no longer reach curl. A forged HTTP 200 therefore cannot write supported=true. Executable regressions now record the exact compare argv and keep the pinned action ungated after a successful probe. 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. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This head closes empty-SHA and owner/../path cases, but it is not yet the integration vehicle.
Remaining buyer-visible gaps on 6a7103a4:
repository_patternstill acceptsContextualWisdomLab/... That interpolates to/repos/ContextualWisdomLab/../dependency-graph/compare/...and can leave the intended repository (CWE-22). The special.githubname must stay legal; only complete./..segments must fail closed.- Identity-rejection diagnostics echo the raw
${BASE_SHA},${HEAD_SHA}, and${REPOSITORY}. A named ref such asmaintherefore appears in the log, and an untrusted repository string can break out of the Actions error line. - Curl
%{http_code}000with transport exit 0 is classified as HTTP000instead ofunavailable, so operators cannot distinguish a no-status sentinel from a real status.
Do not merge this head. Do not merge #1044 or #1048 for the same reason: #1048 already hides raw named refs and classifies 000, but it still accepts owner/...
Operator next step: review the successor that rejects ./.. segments, omits raw invalid identity, and locks 000 as unavailable. Keep #810 open until a protected-main public consumer run on ContextualWisdomLab/EgressWeave#76 proves a non-200 cannot go green. Automated review is not qualifying approval.
Sent by Cursor Automation: Fix Issues
| *) visibility="unknown" ;; | ||
| esac | ||
| revision_pattern='^[0-9a-fA-F]{40}$|^[0-9a-fA-F]{64}$' | ||
| repository_pattern='^[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+$' |
There was a problem hiding this comment.
^[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+$ matches ContextualWisdomLab/.. and ../.github. After interpolation the compare URL becomes /repos/ContextualWisdomLab/../dependency-graph/compare/....
Reject a complete owner or name segment that is . or .. before curl. Keep ContextualWisdomLab/.github legal. Prove this with an executable case that would otherwise print HTTP 200 and write supported=true.
| revision_pattern='^[0-9a-fA-F]{40}$|^[0-9a-fA-F]{64}$' | ||
| repository_pattern='^[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+$' | ||
| if ! [[ "${BASE_SHA}" =~ $revision_pattern ]] || ! [[ "${HEAD_SHA}" =~ $revision_pattern ]]; then | ||
| echo "::error::Dependency review evidence unavailable for ${REPOSITORY} (visibility ${visibility}) at exact base ${BASE_SHA} and head ${HEAD_SHA}: HTTP unavailable; curl exit uncalled. Malformed revision. Supply the pull request's exact 40- or 64-character hex base and head SHAs, then rerun. Failing closed." |
There was a problem hiding this comment.
This error interpolates the untrusted revision and repository. HEAD_SHA=main therefore appears in the log, and a crafted repository string can break the Actions annotation.
Name the failure class and the operator next step only: resupply the pull-request event's exact 40- or 64-character hex SHAs. Do not echo the raw invalid values. After identity is validated, echoing those now-safe SHAs on a later transport failure remains useful.
| exit 0 | ||
| fi | ||
| case "$status" in | ||
| [0-9][0-9][0-9]) http_status="$status" ;; |
There was a problem hiding this comment.
000 matches [0-9][0-9][0-9], so a no-status sentinel with curl exit 0 is recorded as HTTP 000 instead of unavailable.
Classify 000 and empty status as unavailable before the numeric arm, then fail closed. Operators should check runner egress and GitHub service health, then rerun — not treat 000 as a real HTTP status.
2 participants
Purpose
Close the remaining identity-validation hole on the #810 fail-closed dependency-review gate. #1041 and #1042 already fail closed on HTTP 403/404, empty/malformed status, transport failure, and curl exit 18 with a printed 200. They still interpolate unvalidated
BASE_SHA,HEAD_SHA, andREPOSITORYinto the compare URL. An empty revision or../repository name can therefore reach curl; a forged HTTP 200 writessupported=true.This is the single integration vehicle for that repair. It continues #1041 (same fail-closed probe, visibility diagnostics, EgressWeave #66 canary, and unskippable pinned action) and supersedes #897, #1033, and #1042 for integration. Do not open further overlapping workflow PRs.
Canary (do not treat as fixed until a post-merge consumer run)
ContextualWisdomLab/EgressWeave#66, Security Scan run 31108241013, job 92638903658 compared
10d0c51d...c038a950, received HTTP 403, skipped the pinned action, and still concluded success. Downstream: ContextualWisdomLab/EgressWeave#76.Keep #810 open until a protected-main public consumer run proves a non-200 or failed-transfer comparison cannot green this job.
What changed
owner/name, before opening a socket (curl exituncalled).supported=truepath cannot satisfy the success contract.Tests
Local
pytest tests/test_required_workflow_queue_contract.py tests/test_pr_review_autofix_nvidia_nim_contract.py tests/test_pr_governance_audit_contract.py tests/test_codeql_pr_workflow_contract.py tests/test_central_required_workflow_ruleset_audit.py tests/test_opencode_workflow_shell_syntax.py: 117 passed.Acceptance
Do not close #810 from this change alone. Merge only after exact-current-head deterministic, security, dependency, and required workflows are terminal and clean, current automated review has no valid unresolved finding, and a qualifying counted non-author formal approval exists.