fix(security): claim verified OIDC replay before token mint - #83
Draft
seonghobae wants to merge 8 commits into
Draft
fix(security): claim verified OIDC replay before token mint#83seonghobae wants to merge 8 commits into
seonghobae wants to merge 8 commits into
Conversation
📝 WalkthroughWalkthroughOIDC 교환 흐름이 서명·권한 검증 후 replay claim을 등록하고, 그 다음 GitHub 설치 토큰을 발급하도록 변경되었습니다. replay 충돌과 보호 기능 불가 상태는 각각 401과 503으로 처리됩니다. ChangesOIDC replay 보호
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Worker
participant ReplayGuard
participant GitHubAPI
Client->>Worker: POST /exchange
Worker->>ReplayGuard: claim verified jti and exp
ReplayGuard-->>Worker: claim accepted
Worker->>GitHubAPI: create installation token
GitHubAPI-->>Worker: token response
Worker-->>Client: 200 single-use response
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
11 tasks
Contributor
Author
|
@coderabbitai review Please perform a substantive security review of exact head |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
expected='89f1d736c04a259363e566b2d269ad37fc6f8194'
echo '=== checked-out commit ==='
git rev-parse HEAD
git show -s --format='%H%n%P%n%s%n%D' HEAD
echo '=== expected commit available ==='
git cat-file -t "$expected"
echo '=== expected commit summary ==='
git show -s --format='%H%n%P%n%s%n%D' "$expected"
echo '=== changed files from first parent ==='
git diff-tree --no-commit-id --name-status -r "$expected"
echo '=== source outlines ==='
ast-grep outline src/index.ts --items all --view expanded || true
ast-grep outline src/worker.ts --items all --view expanded || true
ast-grep outline test/replay-before-token-mint.test.ts --items all --view expanded || true
echo '=== security-relevant source references ==='
rg -n -C 6 -e 'verifyGithubOidcJwt|createInstallationToken|replay|REPLAY|x-oidc-replay-protection|targetRepository|repository' src/index.ts src/worker.ts test/replay-before-token-mint.test.ts
echo '=== exact-head patch ==='
git diff "${expected}^" "$expected" -- src/index.ts src/worker.ts test/replay-before-token-mint.test.ts
echo '=== package scripts and dependency authority surface ==='
if [ -f package.json ]; then cat package.json; fi
if [ -f wrangler.toml ]; then cat wrangler.toml; fi
if [ -f wrangler.jsonc ]; then cat wrangler.jsonc; fi
echo '=== PR metadata and exact-head checks ==='
gh pr view 83 --repo ContextualWisdomLab/noema --json number,state,isDraft,headRefOid,baseRefName,baseRefOid,mergeStateStatus,statusCheckRollup,files \
--jq '{number,state,isDraft,headRefOid,baseRefName,baseRefOid,mergeStateStatus,checks:[.statusCheckRollup[]|{name,status,conclusion,detailsUrl}],files:[.files[]|{path,additions,deletions]}]}' || trueLength of output: 52580 🏁 Script executed: #!/bin/bash
set -euo pipefail
head='89f1d736c04a259363e566b2d269ad37fc6f8194'
base=$(git merge-base "$head" "origin/docs/architecture-trust-boundaries" 2>/dev/null || git merge-base "$head" "$head^")
printf '=== merge base ===\n%s\n' "$base"
echo '=== exact verification and core exchange path ==='
sed -n '352,410p' src/index.ts
sed -n '511,612p' src/index.ts
echo '=== complete production wrapper path ==='
sed -n '59,270p' src/worker.ts
sed -n '380,555p' src/worker.ts
echo '=== replay guard implementation ==='
replay_file=$(fd -t f '^oidc-replay\.ts$' src | head -n 1)
printf '%s\n' "$replay_file"
ast-grep outline "$replay_file" --items all --view expanded
cat -n "$replay_file"
echo '=== runtime entrypoint and bindings ==='
runtime_file=$(fd -t f '^runtime-entrypoint\.ts$' src | head -n 1)
printf '%s\n' "$runtime_file"
cat -n "$runtime_file"
echo '=== focused regression test bodies ==='
sed -n '163,255p' test/replay-before-token-mint.test.ts
echo '=== replay-related tests and direct-core tests ==='
rg -n -C 4 -e 'NOEMA_OIDC_REPLAY_GUARD|verified-before-mint|claimOidcTokenUsage|createInstallationToken|ERR_AUTH_REPLAY' test src -g '*.test.ts' -g '*.ts'
echo '=== cumulative branch diff from merge base ==='
git diff --stat "$base" "$head"
git diff --find-renames --find-copies "$base" "$head" -- src/index.ts src/worker.ts src/oidc-replay.ts src/runtime-entrypoint.ts wrangler.toml package.json test/replay-before-token-mint.test.ts docs/doctoring/replay-before-token-mint.md
echo '=== requested-commit documentation ==='
cat -n docs/doctoring/replay-before-token-mint.md
echo '=== exact-head PR and check metadata ==='
gh pr view 83 --repo ContextualWisdomLab/noema --json number,state,isDraft,headRefOid,baseRefName,baseRefOid,mergeStateStatus,statusCheckRollup,filesLength of output: 92060 🏁 Script executed: #!/bin/bash
set -euo pipefail
head='89f1d736c04a259363e566b2d269ad37fc6f8194'
pr_base='52c6f2bce0f89760367807df4c316f2137ad5a97'
echo '=== actual PR-base availability and ancestry ==='
git cat-file -t "$pr_base"
git merge-base --is-ancestor "$pr_base" "$head" && echo 'PR base is an ancestor of head'
git log --oneline --decorate --ancestry-path "${pr_base}..${head}"
echo '=== complete actual PR diff ==='
git diff --stat "$pr_base" "$head"
git diff --name-status "$pr_base" "$head"
git diff --find-renames --find-copies "$pr_base" "$head" -- \
src/index.ts src/worker.ts src/oidc-replay.ts src/runtime-entrypoint.ts \
wrangler.toml package.json \
'.github/**' test/replay-before-token-mint.test.ts docs/doctoring/replay-before-token-mint.md
echo '=== production wrapper dispatch, including route gate ==='
sed -n '356,555p' src/worker.ts
echo '=== all exported fetch entrypoints and exchange delegations ==='
rg -n -C 4 -P '(export default|async fetch\s*\(|baseWorker\.fetch\s*\(|handleExchange\s*\(|createInstallationToken\s*\()' src -g '*.ts'
echo '=== authority-related changes in actual PR ==='
git diff -U4 "$pr_base" "$head" -- \
wrangler.toml package.json \
':!src/index.ts' ':!src/worker.ts' ':!test/replay-before-token-mint.test.ts' ':!docs/doctoring/replay-before-token-mint.md'Length of output: 38305 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== entrypoint imports and /exchange delegation ==='
sed -n '1,55p' src/entrypoint.ts
sed -n '300,365p' src/entrypoint.ts
echo '=== worker import and runtime route-chain references ==='
rg -n -C 3 -P '(from "./worker"|worker\.fetch\s*\(|from "./entrypoint"|entrypoint\.fetch\s*\()' src -g '*.ts'Length of output: 6145
Diagnostic verdict: no security finding identified in the requested scope.
Limitations
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/doctoring/replay-before-token-mint.md (1)
1-77: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win운영 보안 문서를 한국어 중심으로 작성하세요.
이 문서는 replay 보호의 운영 및 보안 설계를 설명합니다. 본문과 제목을 한국어로 전환하세요. 코드, HTTP 헤더, 오류 코드, API 식별자, 인용 원문은 필요한 경우 영어로 유지하세요.
As per coding guidelines, operational and sales/acquisition documentation is largely Korean.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/doctoring/replay-before-token-mint.md` around lines 1 - 77, 이 문서의 제목과 설명 문단을 한국어 중심으로 번역해 운영·보안 설계 내용을 한국어로 제공하세요. 코드 블록, HTTP 헤더, 오류 코드, API 식별자, URL 및 필요한 인용 원문은 영어로 유지하고, 기술적 의미와 보안 순서·검증 조건은 변경하지 마세요.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@docs/doctoring/replay-before-token-mint.md`:
- Around line 1-77: 이 문서의 제목과 설명 문단을 한국어 중심으로 번역해 운영·보안 설계 내용을 한국어로 제공하세요. 코드
블록, HTTP 헤더, 오류 코드, API 식별자, URL 및 필요한 인용 원문은 영어로 유지하고, 기술적 의미와 보안 순서·검증 조건은
변경하지 마세요.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 028bd60b-bad8-48bc-b52b-c1f1cd744422
📒 Files selected for processing (4)
docs/doctoring/replay-before-token-mint.mdsrc/index.tssrc/worker.tstest/replay-before-token-mint.test.ts
11 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
Test-first stacked security repair for #81. The production ordering on the stack could mint a GitHub installation token before the distributed single-use replay claim rejected a duplicate valid OIDC credential. This PR moves the atomic replay decision to the verified-and-authorized boundary before privileged token minting, without allowing unverified
jtivalues to consume replay state.This PR is stacked on #71 (
docs/architecture-trust-boundaries) because it changes the same credential-exchange source surface. It remains Draft while #71 is unintegrated and until the repair is refreshed and revalidated against the protected lineage.Test-first lineage
ed1e7c529712028c4f6e5c5bf928fb07bba48a18: RED contract requiring a valid signed replay to return 401ERR_AUTH_REPLAYwith zero POSTs to/app/installations/{id}/access_tokens.12c95f033437cfc7efbdfd61036775921267ebf2: core exposes cryptographically verifiedjti/expafter target-repository authorization and beforecreateInstallationToken().1e893dda8ba063ffaaa694521f4cfac80d1f86dc: wrapper requires the distributed replay binding before credential-bearing core execution and consumes internal pre-mint replay proof.5acd2f0f3c69a9e80f3878ab9359dd1829c061b1: first-use regression requiresreplay_claimbeforetoken_mint.89f1d736c04a259363e566b2d269ad37fc6f8194: focused APA 7 doctoring for the security-ordering rationale.82506bf4697664e94b27b0312347f0a82e5fd15d: localizes the operational/security doctoring to Korean while preserving technical semantics, identifiers, URLs and citations.1a8317867713c003cafe3f6a46fd8490ba0bf789: repairs the stale distributed-rate-limit fixture exposed by exact-head CI.9dfbe1007c43e5c3ca1381ad2a57d748776f0c88: current exact head; adds defensive wrapper coverage for bounded replay-unavailable diagnostics and unexpected replay-guard failures. The one-commit comparison from1a831...changes onlytest/worker-defensive-replay.test.ts(+37/-2), not production source.Security boundary after the change
Intended production ordering:
The unverified payload prefilter cannot reserve replay state. The claim occurs only after signature, issuer, audience, repository/workflow, time and target-repository authorization succeed. Missing replay binding, replay conflict, unavailable replay state, missing bounded verified replay claims, or malformed proof fails closed before token mint.
Current stack identity
Freshly revalidated:
docs/architecture-trust-boundaries(fix(security): bind workflow source and document MSA boundaries #71).480ea86b1dcf93508aaf1ae759b8f8f19b497c96.9dfbe1007c43e5c3ca1381ad2a57d748776f0c88.52c6f2bce0f89760367807df4c316f2137ad5a97.src/index.ts,src/worker.ts,test/replay-before-token-mint.test.ts,test/distributed-rate-limit.test.ts,test/worker-defensive-replay.test.ts, anddocs/doctoring/replay-before-token-mint.md.Refreshing this branch while #71 remains unintegrated would invalidate exact-head evidence without creating independent integration value and could race the shared credential-exchange surface. The live-base divergence is therefore retained as explicit stack evidence rather than hidden behind stale PR
base_shametadata.Exact-head CI evidence and RCA
For exact head
9dfbe1007c43e5c3ca1381ad2a57d748776f0c88:cirun31316088354: terminal failure. The full application suite passed 679/679 tests with configured production statement, branch, function, and line coverage all 100%. The only failing acceptance boundary is the repository-widenpm audit --audit-level=highgate on inheritednanoid@3.3.16(GHSA-2v37-7h3g-55p8). The root remediation is isolated in PR fix(security): update transitive nanoid for CVE-2026-67213 #76; duplicating that lockfile change into this security stack is rejected as a wrong-layer fix.reviewer-cirun31316088356: terminal success.Security Scan: absent on this feature-base stack; this isdefer_until_trigger, not passing evidence.COMMENTEDpredecessor-range evidence only; no eligible independent non-authorAPPROVEDreview.The earlier fixture defect is no longer the failing boundary: current CI shows
test/distributed-rate-limit.test.tsand the replay-order regression suite passing. No predecessor-head CI, review, model, or synthetic-merge evidence is promoted to exact-head approval or merge authority.Remediation feasibility classification
nanoidremediation on fix(security): claim verified OIDC replay before token mint #83: reject — it duplicates fix(security): update transitive nanoid for CVE-2026-67213 #76 at the wrong stack layer and increases future conflict surface.reviewer-ci/CodeRabbit as approval: reject — changes the authority contract instead of the root cause.Required acceptance before merge
No repair workflow, self-modifying Action, branch-patching
contents:write, broader App permission, audit waiver, protection bypass, merge authority, release authority or deployment authority is introduced.Closes #81 only after protected integration and exact-head/operational verification.