Skip to content

fix(security): fail closed on unavailable dependency review - #897

Open
seonghobae wants to merge 5 commits into
mainfrom
fix/dependency-review-fail-closed-current-main-20260809
Open

fix(security): fail closed on unavailable dependency review#897
seonghobae wants to merge 5 commits into
mainfrom
fix/dependency-review-fail-closed-current-main-20260809

Conversation

@seonghobae

Copy link
Copy Markdown
Contributor

Purpose

Replace the stale, conflicted #799 integration path with a bounded current-main repair for #810. Preserve #799 as historical evidence until this replacement proves exact-head checks; do not import its divergent removal of later Strix work.

RCA

Symptom

The central required Security Scan can conclude success when the exact dependency-review comparison returns HTTP 403 or 404.

Immediate cause

The support probe emits supported=false, exits zero, and skips the pinned dependency-review action.

Root cause and owner

A hard supply-chain evidence boundary was modeled as an optional capability optimization. Repository-owned workflow policy therefore translated unavailable evidence into a false-green result.

Remedy selection

Rejected:

  • rerunning an unchanged 403/404;
  • treating OSV, Trivy, CodeQL, Semgrep, Secret Scan, Scorecard, or Dependabot as a semantic substitute;
  • adding credentials, write authority, OIDC, model access, or a branch-controlled exception;
  • updating the 31-commit-behind conflicted fix(security): bind repository scanners to exact PR head #799 branch or importing its divergent tree.

Implemented:

  • exact current protected base 6eb06cdd08c79a06f7b390069d4ffa49e2eb7dba;
  • RED commit b06bb25c1af99b784730209dceb299cd493734f7 reverses the stale fail-open queue contract;
  • GREEN commit 71947d24828cfb42806f5ef0a8abf83f9279d0af accepts only HTTP 200, bounds connection/total time, discards the response body, binds checkout to the explicit PR head repository/SHA, and emits actionable exact-identity failure evidence;
  • read-only permissions and immutable action pins remain unchanged;
  • operator doctoring, rollback, status discipline, APA 7 primary references, and CHANGELOG evidence are included.

Local exact-head evidence

  • RED contract rejected the protected-main workflow at the missing non-200 failure branch.
  • GREEN fail-closed queue contract: pass.
  • Probe ordering/body-discard/timeout invariants: pass.
  • Python compilation of the modified contract: pass.
  • git diff --check: pass.
  • Four bounded files changed; no Strix, provider, credential, reviewer, merge, or release behavior changed.

Required acceptance

Merge only after all exact-head deterministic/security/dependency checks are clean, current automated review has no valid unresolved findings, a qualifying counted non-author formal approval exists, and repository protection permits integration. After merge, require a protected-main public-repository consumer run proving non-200 evidence cannot produce a green dependency-review gate before closing #810.

@coderabbitai

coderabbitai Bot commented Aug 9, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@seonghobae, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 31 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 11b0fec7-1fa0-4958-afa9-41c0bfa2d00b

📥 Commits

Reviewing files that changed from the base of the PR and between 6eb06cd and 8fc17de.

📒 Files selected for processing (4)
  • .github/workflows/security-scan.yml
  • CHANGELOG.md
  • docs/doctoring/dependency-review-fail-closed.md
  • tests/test_required_workflow_queue_contract.py

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@seonghobae
seonghobae enabled auto-merge (squash) August 9, 2026 12:47

Copy link
Copy Markdown
Contributor Author

/oc Refetch and abort unless exact head is still 71947d24828cfb42806f5ef0a8abf83f9279d0af. There is one blocking source defect in the current GREEN candidate: the dependency-review probe still uses status="$(curl ... || true)". That discards curl's transport exit code. A partial-transfer/body-write/TLS/transport failure can therefore emit HTTP 200 and still reach supported=true, contradicting this PR's own fail-closed contract and the historical regression already observed in predecessor work.

Fix test-first on this existing branch only. Add a permanent regression that models a fake/controlled curl which writes HTTP 200 to stdout but exits nonzero (for example curl exit 18 partial transfer) and proves the support probe fails rather than setting supported=true. Then change the shell to preserve both channels independently: capture curl output/status without || true erasing the exit code, restore set -e, and require both curl_status == 0 and exact HTTP status 200 before emitting supported=true. Any nonzero transport exit, empty/malformed status, or non-200 status must produce only bounded exact repository/base/head/status/transport diagnostics and exit 1. Continue discarding the response body, keep 10s connect/30s total timeouts, exact-head checkout, read-only permissions, immutable dependency-review action pin, and no credential/model/ruleset changes.

Strengthen tests/test_required_workflow_queue_contract.py or a focused companion to reject the literal || true support-probe pattern and require the captured transport-status branch. Run the focused fail-closed contract, complete central suite, compilation, git diff --check, Strix quick gate, then exact-head security/supply-chain gates. Do not mark this head accepted from the current queued checks; they are stale as soon as this repair lands.

Copy link
Copy Markdown
Contributor Author

Exact-head RCA and proof for 8fc17de86d1d22c9c11946dd7f70ff934c0b553a:

  • First failing boundary on predecessor 71947d24828cfb42806f5ef0a8abf83f9279d0af: the support probe used command substitution with || true, which erased curl's transport exit. A partial transfer could emit HTTP 200, exit nonzero, and still reach supported=true.
  • Technical root cause: HTTP response status and transport integrity were collapsed into one string channel. Systemic control: the permanent executable regression now supplies a controlled curl that prints 200 and exits 18, and requires fail-closed behavior with no GitHub output.
  • Remedy: capture the curl transport exit separately under a bounded set +e region, immediately restore set -e, normalize the body-free HTTP status, and require transport exit 0 plus exact HTTP 200. Diagnostics remain bounded to repository/base/head, normalized HTTP status, and numeric transport exit.
  • Preserved boundaries: discarded response body, 10-second connect/30-second total timeouts, exact-head checkout, read-only permissions, immutable dependency-review pin, and no credential/model/ruleset changes.
  • RED: focused contract failed 2 tests, including the executable HTTP-200/exit-18 counterexample.
  • GREEN: focused 2 passed; complete central suite 978 passed + 16 subtests; compile/diff checks clean; Strix self-test PASS.

Hosted exact-head security and supply-chain results remain authoritative.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant