fix(oauth): bound OrcaRouter key-exchange responses - #4159
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (12)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughThe OrcaRouter OAuth key exchange now limits response bodies to 64 KiB, applies shared timeout and cancellation handling, validates UTF-8 and JSON, masks body-read errors, and documents the behavior in localized provider guides. ChangesOrcaRouter OAuth validation
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant OrcaRouterOAuthFlow
participant fetch
participant readBoundedResponseBytes
participant TextDecoder
OrcaRouterOAuthFlow->>fetch: Send key exchange with shared 30-second signal
fetch-->>OrcaRouterOAuthFlow: Return headers and response body
OrcaRouterOAuthFlow->>readBoundedResponseBytes: Read at most 65536 bytes
readBoundedResponseBytes-->>OrcaRouterOAuthFlow: Return bytes or abort/error
OrcaRouterOAuthFlow->>TextDecoder: Decode with fatal UTF-8 validation
TextDecoder-->>OrcaRouterOAuthFlow: Return JSON text
OrcaRouterOAuthFlow->>OrcaRouterOAuthFlow: Parse JSON before saving the key
Merge Risk: ⚪ Minimal · up to OrcaRouter browser login now safely limits and validates token-exchange responses before saving credentials, while preserving cancellation behavior. The current change is ready to merge. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 4 files. (8 skipped: 8 unsupported.)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
github-actions
Bot
added
bug
|
|
@coderabbitai review Please review immutable head |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
head_commit='d9fbb1e2a6804d117f316ccc35fb50a30252f63f'
base_commit='a7509fe00032f74d38ca073dcf701b90353b8283'
printf '%s\n' '== checked-out revision =='
git rev-parse HEAD
printf '%s\n' '== requested commits =='
git cat-file -t "$head_commit"
git cat-file -t "$base_commit"
printf '%s\n' '== changed files =='
git diff --name-status "$base_commit" "$head_commit"
printf '%s\n' '== changed-file diff with zero context =='
git diff --no-ext-diff --unified=0 "$base_commit" "$head_commit" -- .
printf '%s\n' '== candidate implementation and test files =='
fd -t f . | rg -i '(orca|oauth|response|bounded|byte|reader|body|callback)'
printf '%s\n' '== symbol references =='
rg -n -i -C 3 \
'readBoundedResponseBytes|OrcaRouter|TextDecoder|AbortSignal|timeout|key.exchange|keyExchange|scope|reauth' \
. \
-g '!node_modules/**' -g '!dist/**' -g '!build/**'Length of output: 193112 🏁 Script executed: #!/bin/bash
set -euo pipefail
head_commit='d9fbb1e2a6804d117f316ccc35fb50a30252f63f'
base_commit='a7509fe00032f74d38ca073dcf701b90353b8283'
printf '%s\n' '== revision check =='
printf 'HEAD: '; git rev-parse HEAD
printf 'base ancestor: '; git merge-base --is-ancestor "$base_commit" "$head_commit" && printf 'yes\n'
printf '%s\n' '== commits =='
git log --format='%H %s' --reverse "$base_commit..$head_commit"
printf '%s\n' '== changed paths =='
git diff --name-status "$base_commit" "$head_commit"
printf '%s\n' '== changed TypeScript paths =='
git diff --name-only "$base_commit" "$head_commit" -- '*.ts' '*.tsx'
printf '%s\n' '== TypeScript diff =='
git diff --no-ext-diff --unified=80 "$base_commit" "$head_commit" -- '*.ts' '*.tsx'
printf '%s\n' '== direct helper and OrcaRouter references outside devlog =='
rg -n -C 8 \
'readBoundedResponseBytes|OrcaRouter|orca.?router|new TextDecoder|TextDecoder\(' \
src tests \
-g '*.ts' -g '*.tsx' 2>/dev/null || trueLength of output: 201430 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== source outlines =='
ast-grep outline src/lib/bounded-body.ts --items all
ast-grep outline src/oauth/orcarouter.ts --items all
printf '%s\n' '== bounded byte reader implementation =='
match_line=$(rg -n 'function readBoundedResponseBytes|export async function readBoundedResponseBytes' src/lib/bounded-body.ts | head -1 | cut -d: -f1)
start=$((match_line > 30 ? match_line - 30 : 1))
end=$((match_line + 240))
sed -n "${start},${end}p" src/lib/bounded-body.ts
printf '%s\n' '== OrcaRouter exchange implementation =='
match_line=$(rg -n 'exchangeToken\s*\(' src/oauth/orcarouter.ts | head -1 | cut -d: -f1)
start=$((match_line > 60 ? match_line - 60 : 1))
end=$((match_line + 240))
sed -n "${start},${end}p" src/oauth/orcarouter.ts
printf '%s\n' '== exact source diff =='
git diff --no-ext-diff --unified=20 \
a7509fe00032f74d38ca073dcf701b90353b8283 \
d9fbb1e2a6804d117f316ccc35fb50a30252f63f \
-- src/lib/bounded-body.ts src/oauth/orcarouter.ts
printf '%s\n' '== relevant test ranges =='
sed -n '440,575p' tests/server/bounded-body.test.ts
sed -n '240,405p' tests/providers/orcarouter-provider.test.tsLength of output: 38517 🏁 Script executed: #!/bin/bash
set -euo pipefail
head_commit='d9fbb1e2a6804d117f316ccc35fb50a30252f63f'
base_commit='a7509fe00032f74d38ca073dcf701b90353b8283'
printf '%s\n' '== OrcaRouter deadline helper and imports =='
sed -n '1,55p' src/oauth/orcarouter.ts
printf '%s\n' '== relevant exact line locations =='
rg -n \
'cancelBodyWithoutWaiting|readBoundedResponseBytes|cancelWithoutWaiting|requestSignal|TOKEN_REQUEST_TIMEOUT_MS|BOUNDED_BODY_MAX_BYTES|TextDecoder\("utf-8", \{ fatal: true \}\)' \
src/lib/bounded-body.ts src/oauth/orcarouter.ts
printf '%s\n' '== complete changed path list =='
git diff --name-status "$base_commit" "$head_commit"
printf '%s\n' '== whitespace validation =='
git diff --check "$base_commit" "$head_commit"
printf '%s\n' '== source-only change summary =='
git diff --stat "$base_commit" "$head_commit" -- src/lib/bounded-body.ts src/oauth/orcarouter.ts tests/server/bounded-body.test.ts tests/providers/orcarouter-provider.test.tsLength of output: 6501
I found no actionable issue in the inspected changes.
The full CI result is still pending. Maintainer security review is still required before merge readiness. ✅ Action performedReview finished.
|
리뷰 · 우선순위 55 / 80이 PR은 OrcaRouter 브라우저 로그인 키 교환이 성공 응답을 같이 고친 현재 src/oauth/orcarouter.ts (exchangeToken catch) - 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
✅ READY
Review readiness checklist
✅ 4/4 boxes ticked. This pull request is already Ready for Review. |
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
Completed the requested bounded authentication-response review at this exact head. The successful key exchange is admitted by raw byte count before UTF-8 decode/JSON parse, uses one header/body deadline, and retains the key/user/scope validators. Non-abort read failures use a fixed message; pre-aborted reads cancel without acquiring a reader or waiting on cancellation. The fixture matrix actually observes byte boundaries, cancellation reason identity, lock release and non-reflection. No change was found to the credential destination, PKCE, or omitted-versus-invalid scope contract.
Author CI 34412474002 is successful for this head. The changed production files also have no base-to-current-dev delta at inspection. I am applying maintainer-sponsored for this reviewed authentication change. This is a scoped security review, not a live-provider login test, blanket repository audit, or merge. Keep required repository checks and the readiness/independent integration decision separate.
Ingwannu
added
the
maintainer-sponsored
github-actions
Bot
removed
the
intake: hygiene-blocked
d9fbb1e to
4f709f1
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
4f709f1 to
9e28465
Compare
3 participants
Summary
OrcaRouter's successful browser-login key exchange used
response.json(), materializing the whole body before validating the key and accepting replacement decoding for malformed UTF-8. Read at most 64 KiB through the existing raw-byte primitive, then require valid UTF-8 JSON. Reuse the same existing 30-second signal for response headers and body consumption. Non-abort body failures retain a fixed error without reflecting upstream text or an error cause; cancellation preserves the original reason.The first commit also closes the previously documented shared byte-reader follow-up needed by this consumer: when a signal is already aborted,
readBoundedResponseBytesnow attempts to cancel the original body before rejecting, matching the existing text-body reader. It does not attach a reader or wait for cancellation that rejects or never settles. The second commit adds the OrcaRouter limit, its regression matrix and eight localized documentation paragraphs. That public report establishes prior disclosure, not maintainer approval of this implementation.Existing PKCE, origin/private-network consent, key/user identity, omitted-versus-invalid scope and terminal reauthentication behavior are retained. The new size limit applies to the successful login key-exchange body, not inference payloads. Tests use synthetic responses and local callback fixtures; this PR makes no claim of a live provider authentication test or a proven native-fetch connection leak.
Verification
9e28465c26d4429cbfcb764152e904a68b1c807f, based ondev 386b6a0d9a8acef818b9c40ebd472e4974750199.34439096989: 26/26 jobs passed, bound to9e28465c26d4429cbfcb764152e904a68b1c807f. The checklist CI attestation refers to this completed matrix; local focused results are listed separately.Security review
The maintainer-sponsored label is applied. Independent source review found no blocker. Maintainer merge approval remains separate.
Checklist
Review readiness checklist
Readiness base check: 7 commits behind current dev; within the repository allowance of ten.