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 (8)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 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: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix 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 The OAuth exchange now validates bounded response bodies with shared timeout and cancellation behavior, with no unresolved current-head merge risk. 🚥 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🧪 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 has been marked 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
9e28465 to
4d2fc92
Compare
Ready-for-review evidenceRebased onto the current Local run — To be precise about that number rather than hide it: the same 23 failures reproduce on unmodified No unresolved Codex or CodeRabbit findings on this PR. |
4d2fc92 to
21a15c9
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 21a15c9397
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| import { OAuthCallbackFlow, type OAuthCallbackFlowOptions } from "./callback-server"; | ||
| import { generatePKCE } from "./pkce"; | ||
| import type { OAuthController, OAuthCredentials } from "./types"; | ||
| import { BOUNDED_BODY_MAX_BYTES, readBoundedResponseBytes } from "../lib/bounded-body"; |
There was a problem hiding this comment.
This changes authentication and response-transport behavior under both src/oauth/ and src/lib/, but none of the owning documents listed for those areas in structure/INDEX.md are updated. That leaves the maintainer source of truth without the new 64 KiB limit, shared deadline, and cancellation contract; update the applicable structure documents in this change as required by the repository's non-advisory ownership rule.
AGENTS.md reference: AGENTS.md:L33-L41
Useful? React with 👍 / 👎.
Correction: the local failures have a concrete cause, not "environment noise"My earlier comment called the 23 local failures a Windows environment limitation. That was imprecise, so here is the actual root cause.
Instrumented proof, two consecutive tests in this file: The second flow's own state is rejected, while the first flow's state is accepted on the same port — the stale listener is serving the request. Linux raises Supporting evidence: the failing test passes in isolation ( This looks like a real product issue rather than a test artifact — back-to-back OAuth logins on Windows can have the new callback intercepted by the retired listener. It is outside this PR's scope (bounded key-exchange responses), so I am not changing it here; I will raise it separately. |
Correcting my own root cause aboveMy previous comment blamed two coexisting listeners on a fixed port. I dug further and that explanation was wrong — the mechanism is different, so please disregard it in favour of this. I measured the listener lifecycle directly instead of inferring it: A fresh TCP connection is already refused, so the listening socket does close correctly and no second listener exists. What survives is an already-established keep-alive connection: So the earlier probe output was right, but my interpretation of it was not. Notably The real fix is to end the connection with the callback response, since exactly one callback is expected per flow. I opened #4280 for that. With it applied, this file goes from 23 failures on unmodified Nothing here changes the conclusion for this PR: the failures are pre-existing on |
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
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
All CI tests are green on my local testing.
I pushed my PR to the latest dev commit.
I resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.
Readiness base check: 7 commits behind current dev; within the repository allowance of ten.
Summary by CodeRabbit
Documentation
Bug Fixes