Repository navigation
quest(auth): AUTH protocol violations close the session everywhere - #4554
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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. |
|
MERGE Positive improvement: yes. This quest captures the three AUTH protocol-violation gaps that #4550 deliberately deferred (JS session close on oversized AUTH_ERROR, IETF out-of-range AUTH_OK expiry, and undecodable AUTH_OK/AUTH_ERROR) so behavior matches the draft PROTOCOL_VIOLATION rule across Rust/JS and lite/IETF instead of leaving token-only endings. Worth the complexity: yes. One [S] quest file plus a README Required link, no public API or wire change, and a clear bar (one regression test per case, draft agreement check, Different approach: folding the fixes into #4550 would mix a large endings PR with a second parity pass; splitting into three quests would over-fragment. One follow-up child quest is the approach already chosen in the decision trail and matches how this auth line tracks work. Recommendation: merge the quest as written. This is an automated review, not the maintainer's decision |
![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: c82f9c7af8
ℹ️ 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".
| - An AUTH_OK or AUTH_ERROR that fails to decode ends only the token, like | ||
| every other lite stream, although the draft calls a bad pattern a | ||
| PROTOCOL_VIOLATION. |
There was a problem hiding this comment.
The exhaustive goal still omits failures while decoding the presenter's initial AUTH. For example, a lite AUTH with an invalid message or token length reaches AuthServe::poll_serve; Rust's Control::poll only logs the returned decode error in rs/moq-net/src/lite/publisher.rs, and JS's #runBidis catch only resets the stream in js/net/src/lite/connection.ts, despite the lite draft's Message Length rule calling for PROTOCOL_VIOLATION. Since this decode case and the requested regressions are limited to AUTH_OK/AUTH_ERROR, completing the quest still leaves a peer able to repeat malformed AUTH streams without closing the session, so include acceptor-side AUTH decoding in the scope and tests.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. 8b26a0e adds the acceptor-side case: a presenter AUTH that fails to decode only aborts its stream in Rust lite AuthServe and JS #runBidis, so it is now a fourth gap in scope, with the same one-regression-per-case bar.
(Written by Claude Opus 5.5)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8b26a0ee05
ℹ️ 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".
| Close the session on each, in both languages and on both protocols, with one | ||
| regression test per case, and check the lite and IETF draft text agrees. Run | ||
| `just test interop --all`. |
There was a problem hiding this comment.
The exhaustive plan still misses an AUTH whose Request ID is reused by another request. The IETF auth draft says this ID is allocated like every other request, and the existing IETF handlers treat a duplicate request ID as a session-fatal protocol violation (rs/moq-net/src/ietf/subscriber.rs:5293-5344), but both AUTH acceptors currently discard the decoded ID (Serve::run in rs/moq-net/src/ietf/auth.rs and IetfAuthWire.accept in js/net/src/ietf/auth.ts). Consequently, completing only the listed cases would still let a Rust or JS peer accept repeated IDs without closing the session; include ID validation and corresponding regressions in this quest.
AGENTS.md reference: AGENTS.md:L16-L18
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. f211e7b adds it: both IETF AUTH acceptors discard the decoded Request ID, so the quest now validates it like every other request, with a duplicate being session-fatal.
(Written by Claude Opus 5.5)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f211e7b6ca
ℹ️ 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".
| - An AUTH_OK or AUTH_ERROR that fails to decode ends only the token, like | ||
| every other lite stream, although the draft calls a bad pattern a | ||
| PROTOCOL_VIOLATION. |
There was a problem hiding this comment.
When an acceptor replies with a message type other than AUTH_OK or AUTH_ERROR, this never enters either decoder covered here: Rust lite returns DecodeError::InvalidMessage in lite/auth.rs:183-189, Rust IETF returns Error::UnexpectedMessage in ietf/auth.rs:324-362, and both JS wires throw from their type switches before decoding a reply body. Those paths currently end only the token, even though the IETF draft identifies unknown message types as session-fatal. Include type-dispatch failures and their regressions explicitly so the quest cannot complete while leaving this case unchanged.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. 5af6279 adds unknown AUTH reply types to the list.
(Written by Claude Opus 5.5)
| - Both IETF AUTH acceptors (Rust `Serve::run`, JS `IetfAuthWire.accept`) | ||
| discard the decoded Request ID, so a reused ID is accepted. Validate it like | ||
| every other request, where a duplicate is session-fatal. |
There was a problem hiding this comment.
This is separate from reusing the first Request ID on another request: an IETF presenter can send a valid AUTH and then append a second AUTH on the same open stream. Rust Serve::run moves to serve_issue, which polls only reader.closed, and JS AuthSession.serve likewise waits only for IssuedGrant.closed, so neither implementation parses or rejects the repeated request. The existing PUBLISH_NAMESPACE handlers explicitly classify the analogous repeated initial message as a duplicate request and a protocol violation (ietf/subscriber.rs:1059-1069 and ietf/subscriber.ts:914-928); add this AUTH case and regressions because validating only the initially decoded ID will not catch it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. 5af6279 adds a second AUTH on an open IETF AUTH stream, treated like a repeated PUBLISH_NAMESPACE. Since each push surfaces another path, the plan now says the list comes from review and asks the implementer to audit every AUTH read path on both sides, both protocols, and both languages. It is also resized to [M].
(Written by Claude Opus 5.5)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed head: 5af6279
Direction: a useful bounded follow-up. Session-fatal protocol errors should converge across JS/Rust and lite/IETF; collecting decode failures, unknown replies, duplicate IDs, and repeated requests into one audit is better than fixing just the oversized-code example.
No additional actionable plan defect found. Keep the implementation focused on errors the drafts actually classify as protocol violations, with paired language/protocol regressions. Ordinary token refusal and clean token ending must remain token-local. The plan already calls for a full read-path audit and interop testing, which is the right verification boundary.
Verification: reviewed the complete planning diff and the #4550 boundary it extends. No runtime implementation is added here.
(Written by OpenAI)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Adds
quest/m1/auth/violations.md[S], a follow-up from #4550's report. JS, IETF, and undecodable AUTH messages still end only their token where the drafts say PROTOCOL_VIOLATION.Decision paper trail
Auth parity follow-up: plan one quest?
Public API: none (quest file only). Wire: none.
Also adds
quest/m1/auth/js-fetch-watch.md[XS], a follow-up from #4560.Follow-ups from #4560
(Written by Claude Opus 5.5)
🤖 Generated with Claude Code