Skip to content

fix(security): fail closed on ambiguous dependency-review HTTP responses - #1725

Draft
seonghobae wants to merge 15 commits into
mainfrom
fix/dependency-review-non200-fail-closed
Draft

fix(security): fail closed on ambiguous dependency-review HTTP responses#1725
seonghobae wants to merge 15 commits into
mainfrom
fix/dependency-review-non200-fail-closed

Conversation

@seonghobae

@seonghobae seonghobae commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Security owner outcome

This Draft is the canonical ContextualWisdomLab/.github owner lane for the Dependency Review admission boundary. It now consolidates the valid security deltas from protected #1724 and predecessor diagnostic #1643 without weakening the pinned Dependency Review action or any sibling scanner.

Three owner defects are repaired together:

  1. pull-request HTTP 403/404 and every other non-200 compare result previously could be normalized to an unavailable/successful state in the reusable workflow; only HTTP 200 may now authorize the action;
  2. thin reusable-workflow callers had omitted the least-privilege contents: read + pull-requests: read permission envelope that a called workflow cannot elevate itself;
  3. the compare preflight trusted repository/base/head strings before transport; both the reusable workflow and bundled Security Scan now require exact immutable base/head object IDs and one legal non-dot owner/name repository identity before curl.

Test-first and carryover lineage

The original #1725 RED/GREEN lineage remains intact for non-200 fail-closed behavior and caller permissions. Current successor commits add #1643's still-valid immutable-identity requirement test-first:

  • 3736634f95bf132bbbe208ffc80103863fe3a7c1 adds executable reusable-workflow regressions that require named/malformed revisions and malformed/dot-segment repository identities to fail before curl, while legal ContextualWisdomLab/.github reaches exactly one token-authenticated compare;
  • b1e6263d9d9626b6cfd2046ce9147ab67867beec adds the corresponding reusable-workflow production validation;
  • 8b86c0d2c6b0186538db1ed263f7cb9d222f3ca1 carries fix(security): validate immutable dependency-review identity on current main #1643's conflict-free bundled Security Scan identity preflight onto the current owner tree without force-push or destructive rebase;
  • ae128374a2e38e60ada8bf5e89a9c7a4137f864f records the decisive A/B evidence and unified security invariants in canonical doctoring;
  • 58a0b4c8ecc3073a64bd91457101229a21f020d4 adds a dedicated bundled-scan regression so the carried validation cannot silently disappear.

The temporary #1643 canary itself is deliberately not part of this publishable successor.

Decisive A/B evidence from #1643

Exact-head canary run 33589436750, job 100120235906, checked out a6a2759640e6aa1d1e1219e1cd7aacdeffef32c0 and compared exact base bb14b014eee31e6abdb5d2fffbb805aa29420eac to that head for ContextualWisdomLab/.github.

  • anonymous request: HTTP 404, curl exit 0;
  • job-token request with contents: read + pull-requests: read: HTTP 200, curl exit 0.

Therefore an anonymous response is not an availability authority. The least-privilege job token is the supported comparison boundary, and non-200 authenticated results remain fail-closed.

Current protected-main relationship — 2026-09-02

Protected base for this exact head is main@78271917b526469c559fa75cb5ee39426e5494d1. Exact current head is 58a0b4c8ecc3073a64bd91457101229a21f020d4.

Fresh comparison is ahead / behind_by=0. The effective protected-main-relative delta is exactly six owner paths:

  • .github/workflows/dependency-review.yml;
  • .github/workflows/security-scan.yml;
  • Proposed ADR-0025;
  • docs/doctoring/dependency-review-fail-closed-permission-envelope.md;
  • reusable-workflow contract tests; and
  • bundled Security Scan identity contract tests.

GitHub reports the PR Draft and mechanically mergeable. No temporary source-fix or A/B canary workflow is present in this successor.

Consumer evidence and release boundary

Before caller permission repair, immutable reusable-workflow consumers such as ContextualWisdomLab/newsdom-api#784@1623977e6c37c78cb1a94a7a48c48f6d02cac86c (33622976911) and ContextualWisdomLab/mightyETL#330@65efdf7b4064df5b9811c0403defb707e6efbc02 (33623035969) terminated startup_failure with zero jobs. After explicit caller permissions, fresh exact heads materialized Dependency Review runs in newsdom-api, mightyETL, scopeweave and Argos.

After this PR reaches protected main through ordinary protection, consumers must pin the reusable workflow to that immutable protected-main SHA. No caller returns to @main, a PR head, or another mutable owner ref.

Exact-head gate

The current source head invalidates all predecessor check/review evidence. Exact-head runs are newly queued/pending: OSV 33637850661, Secret Scan 33637849083, Security Scan 33637849313, CodeQL 33637849320, Scorecard 33637849319, SBOM 33637849219, SAST 33637849226, and Python Security 33637849275. They are non-passing until terminal.

Keep Draft and ADR-0025 Proposed until this unchanged head has terminal required checks, substantive-clean current reviews/threads, current base/mergeability and ordinary protected admission. No administrator bypass, self-approval, scanner substitution, predecessor-evidence transfer, mutable workflow pin, or 403-as-success is authorized.

Refs #810, #1150, #1643, #1724, #1728, #1731, #1734.

@coderabbitai

coderabbitai Bot commented Sep 2, 2026

Copy link
Copy Markdown

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

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 added area: ci-cd CI, GitHub Actions, checks, release, or supply chain priority: high High-priority or P1 work security status: draft Draft pull request type: bug Defect or incorrect behavior labels Sep 2, 2026 — with ChatGPT Codex Connector

Copy link
Copy Markdown
Contributor Author

Fleet handoff — a second migration defect is now live-evidenced and belongs in the central consolidation contract/doctoring before #1725 leaves Draft.

RCA: #1724's thin-caller replacements removed each caller workflow's permission envelope. A reusable workflow cannot elevate GITHUB_TOKEN permissions passed by its caller; the central workflow requests contents: read + pull-requests: read. On repositories whose default token does not already include that scope, the call fails before job creation. This is consistent with GitHub's reusable-workflow contract that permissions may only be maintained or downgraded across the call chain.

Exact RED evidence after immutable pinning (so mutable-ref resolution is no longer confounded):

  • newsdom-api fix(coverage): gate PyO3 test deferral on exact-head native peer checks #784 1623977e6c37c78cb1a94a7a48c48f6d02cac86c: Dependency Review run 33622976911 -> startup_failure, zero jobs; referenced workflow resolved exactly to .github@0bcd22d8bb07650aafb0a8f116e4c2bbb8744f03.
  • mightyETL chore(deps): bump typing-extensions from 4.15.0 to 4.16.0 #330 65efdf7b4064df5b9811c0403defb707e6efbc02: run 33623035969 -> startup_failure, zero jobs.
  • Original caller workflows prove the permissions were part of the pre-migration contract: scopeweave and newsdom-api had contents: read + pull-requests: read; Argos had the same; mightyETL had contents: read, which is insufficient once the centralized workflow itself also requires pull-requests: read.

Consumer GREEN repair is now applied without touching this owner branch: explicitly retain permissions: {contents: read, pull-requests: read} in the thin callers. Fresh exact heads now materialize instead of immediate zero-job failure: newsdom-api 9a798d5ac7b9b295a1accb2327fc76611352290f run 33623818000 queued; mightyETL 4576f863ede9fca0673d6cce5ae8a4093246f5ab run 33623854807 queued; scopeweave db8b8ed6d36a6dc6cc1d07255a7a9a86bc88bf4f run 33623761776 queued; Argos #557 ee4c5dd326977407435b0f2425fdecebc34a810f run 33623867278 pending.

Owner-path acceptance: extend #1725's central contract/ADR/doctoring/example caller so every reusable Dependency Review caller is required to pass at least contents: read and pull-requests: read; preserve the existing 403/404 fail-closed RED/GREEN; then merge normally and publish the resulting protected-main exact SHA for all four consumers to pin. No caller should return to @main.

Copy link
Copy Markdown
Contributor Author

Fresh owner-path re-read confirms the permission handoff has advanced correctly to an explicit RED at ee0f1ce544965772775b590050e40476df4ea8f6 (test(security): require caller permission envelope). The new contract requires the reusable workflow's documented thin caller to contain permissions: contents: read and pull-requests: read, while the workflow source at this exact head still shows concurrency immediately after on: and therefore does not yet satisfy that test. Keep this as RED rather than weakening the assertion.

Next owner GREEN should minimally update the canonical example/doctoring contract to include the two caller read permissions, preserve the existing non-200 fail-closed production repair, adopt protected main@8eaa65005005ac1e67e21f18f8627529d0f41f5c non-destructively, and reacquire exact-head gates before normal merge. Once that fixed protected merge SHA exists, the four consumer PRs can replace their temporary 0bcd22... pins with that immutable fixed SHA and rerun their real gates.

@seonghobae seonghobae left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fresh-base owner handoff for exact head 403ca1c4de8b3e477b5a9b1c102188278286b2c8 (read-only; no source/ref/PR-state mutation): protected ContextualWisdomLab/.github/main is now 63bf49835da44aa8257eb76a92368e6485ae6e94 via #1728, while this Draft still records base b4eec000d21084accb736d289eb64cfd78e7a91a and is currently non-mergeable. Preserve the HTTP-non-200 fail-closed and least-privilege caller-permission RED/GREEN deltas; non-force reconcile with the live protected base, then re-run focused tests and every exact-current-head required/security/provenance check. Do not transfer the prior 403ca1c4… evidence across the new integration head. Consumers must continue to wait for the resulting protected-main immutable SHA and then pin that exact SHA; no @main, PR-head, skipped/cancelled/queued, or predecessor evidence is release authority.

Copy link
Copy Markdown
Contributor Author

Fresh Naruon reproduction confirms this PR's permission-envelope RCA on a fifth consumer. ContextualWisdomLab/naruon#1539@6e7a8d8a947fec1ffdfff15f165b3a171ec2e03e pins reusable Dependency Review at protected .github@5f8e5b2a79e709c4ab1a4179a605d34c458b13a1; run 33630975578 resolves that referenced workflow but ends startup_failure with zero jobs. The Naruon thin caller currently omits caller-side permissions, matching the already-proven newsdom-api/mightyETL failure class in this PR. I am routing the consumer repair to #1539 without changing this central branch. GREEN for Naruon should include a fresh immutable central pin plus caller contents: read / pull-requests: read, followed by a real dependency-review / dependency-review job on the unchanged repaired caller head.

Copy link
Copy Markdown
Contributor Author

Current-main ancestry reconciliation rationale before write: protected main advanced to 78271917b526469c559fa75cb5ee39426e5494d1 after this Draft lane's prior reconciliation. Fresh compare is ahead_by=8 / behind_by=4 with merge base 63bf49835da44aa8257eb76a92368e6485ae6e94, but the effective tip-to-tip content delta remains exactly the four owner paths already named by this PR: reusable Dependency Review workflow, Proposed ADR-0025, doctoring, and executable contract test. The branch's latest commit 2595e246e8f4aba89fd1bbf0fe4c6980d0ee026c specifically reconciles the newer reusable-workflow contract (including comment_summary_in_pr and harden-runner tests), and every non-owner path is already content-identical to current protected main.

I will therefore preserve both histories without force/rebase by creating a two-parent reconciliation commit with the current branch tree unchanged, parents 2595e246e8f4aba89fd1bbf0fe4c6980d0ee026c and protected main@78271917b526469c559fa75cb5ee39426e5494d1, then move only this owner branch by fast-forward. This is ancestry repair only: no security behavior, ADR status, test expectation, or current protected-main intent is discarded.

Copy link
Copy Markdown
Contributor Author

Consumer owner-path acceptance from writable newsdom-api#784: current caller exact head b14586c218bb60e614136bef94e9fd8163f4d4b8 pins central workflow SHA 5f8e5b2a79e709c4ab1a4179a605d34c458b13a1. That protected-main commit still treats authenticated HTTP 403/404 compare responses as available=false and skips the hard gate; therefore it is not an acceptable security contract for the consumer. #1725 correctly owns the fail-closed repair. GREEN handoff requires: (1) unchanged #1725 protected-main descendant with non-200 fail-closed + caller permission/identity tests terminal-green, (2) ordinary protected merge, (3) canonical immutable owner release/tag for the reusable workflow (the repository currently exposes no Releases), and (4) newsdom-api caller/test constant bumped to that released owner identity followed by exact-head dependency-review / dependency-review terminal GREEN. Do not resolve by returning to @main, retaining 5f8e, or treating 403/404 as availability success.

Copy link
Copy Markdown

Evidence log — 2026-09-02

Exact current head: 58a0b4c8ecc3073a64bd91457101229a21f020d4.

  • Current protected main: 8c085835fbf77de2321b72fa6b8dd946227e523e.
  • GitHub compare: diverged, ahead_by=14, behind_by=7, merge base 78271917b526469c559fa75cb5ee39426e5494d1.
  • GitHub currently reports the PR mechanically mergeable; there is no active merge conflict reported by the API. The branch is nevertheless behind current main, so current-head validation is not yet transferable.
  • Exact-head required/security runs are queued/pending: OSV 33637850661, Secret Scan 33637849083, Security Scan 33637849313, CodeQL 33637849320, Scorecard 33637849319, SBOM 33637849219, SAST 33637849226, Python Security 33637849275.
  • Security Scan currently has four queued jobs (osv-scan, dependency-review, trivy-fs, scorecard) in run 33637849313; no terminal result exists yet.
  • Combined commit status currently exposes only CodeRabbit success; required GitHub Actions evidence is therefore non-terminal.
  • Existing independent review evidence is stale: the recorded review was against predecessor head 403ca1c4..., not this exact head, so it is not treated as current approval.

Gate decision: HOLD. Do not mark ready or merge until the branch is reconciled against current protected main, focused QA passes on the resulting exact head, all applicable required/security checks are terminal and passing, and current qualifying review/thread requirements are satisfied.

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

Labels

area: ci-cd CI, GitHub Actions, checks, release, or supply chain priority: high High-priority or P1 work security status: draft Draft pull request type: bug Defect or incorrect behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants