fix(ietf): discard padding streams and close on unknown uni types - #4603
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Classify each unidirectional stream by the negotiated draft before dispatching it. PADDING (draft-18+) is stopped with CANCELLED instead of INTERNAL_ERROR, and a type the draft does not define closes the session with PROTOCOL_VIOLATION, as every supported draft requires. Mirrored in js/net. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Local validation: Decisions (accepted by the user as built):
Follow-ups: (Written by Claude Opus 5.5) |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (9)
💤 Files with no reviewable changes (3)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review. WalkthroughRust and JavaScript now classify incoming unidirectional streams according to the negotiated draft. They cancel valid padding streams and treat unknown or draft-invalid types as protocol violations. Rust also stops failed subgroup and FETCH streams without ending the session. Tests and documentation cover these behaviors. Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to No concrete merge-blocking issue remains identified. The change adds draft-aware stream validation and explicit cancellation and closure behavior; merge after normal checks pass on the latest revision. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change strengthens rejection of invalid peer streams while keeping valid padding cancellation local. No material security regression was established, but startup handling and complete transport cleanup remain incompletely verified. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches✨ Simplify code
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 |
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. |
…eam-types # Conflicts: # rs/moq-net/src/ietf/session.rs
![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: d72500dce0
ℹ️ 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".
| function timeout(message: string): Promise<never> { | ||
| return new Promise((_resolve, reject) => setTimeout(() => reject(new Error(message)), WAIT)); |
There was a problem hiding this comment.
This helper makes the unit tests depend on a real 500 ms deadline and, when the expected event wins, leaves the losing timer alive; under loaded CI the deadline can also expire before otherwise-correct asynchronous work runs. Use mocked time and the scoped race/timeout helpers so listeners and timers are released when the race completes.
AGENTS.md reference: js/AGENTS.md:L16-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6311a1b: the tests no longer use a timer at all. They await the stream stop and the session close directly (bun's own test timeout covers a hang), and the padding case checks the session is still open after a macrotask yield.
(Written by Claude Opus 5.5)
| // An unknown stream type MUST close the session, not just the stream. | ||
| if (err instanceof ProtocolViolation) this.close(); |
There was a problem hiding this comment.
When a peer opens an unknown unidirectional stream, this calls the no-argument close(), which ultimately invokes WebTransport.close() without close info. The peer therefore receives code 0 (Cancel) rather than SessionCode.ProtocolViolation (0x3), and the violation is reported as a clean shutdown. Send the registered protocol-violation close code here and assert the received code in the new test rather than merely awaiting closed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6311a1b: a protocol violation now closes the WebTransport session with SessionCode.ProtocolViolation (0x3), on both the uni and bidi paths, and the transport closes before NativeSession.close() can send a clean code first. The test asserts the peer receives 0x3.
(Written by Claude Opus 5.5)
Verdict: MERGEReviewed head: Fixes the interop bug: padding (draft-18+) is cancelled stream-only, and an unknown uni type closes the session with PROTOCOL_VIOLATION. Rust BlockingNone. Non-blocking
CI was still queued at review time (Check/Test/Platform/WASM/Android). Author notes local This is an automated review, not the maintainer's decision |
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
(Written by OpenAI)
Reviewed moq-dev/moq at 36079df09fe962e34a532a7b7b07aa1bec820100.
- P2: Reject reserved subgroup modes before stream-only dispatch (session.rs:634-641, connection.ts:325-328). Type 0x56 matches this mask but has SUBGROUP_ID_MODE=0b11. Rust rejects its flags inside the group handler and downgrades that failure to a stream stop; JS also dispatches it as a group. Contrary to the Alternatives rationale, draft-21 §11.3.1 explicitly requires a PROTOCOL_VIOLATION session close for this mode. Validate these type bits at classification and add a session-close regression in both languages; ordinary group failures can remain stream-scoped.
- P2: Classify the full-width JS stream type (connection.ts:321-323). A valid varint carrying an unknown type such as 2^53 makes
u53()throw RangeError before reaching the ProtocolViolation branch. The catch then stops only the stream and leaves the session open, unlike Rust and the unknown-type requirement. Read the full-width value before classification, or specifically convert type-value overflow into a protocol violation while preserving cancellation/reset handling. Test an unknown type above Number.MAX_SAFE_INTEGER.
Direction: the padding/classifier split is appropriately small, but the two classification gaps should be closed. I also independently confirm the existing JS close-code finding; no duplicate inline comment added.
Limits: source and specification review only; I did not execute tests, interop, or repository gates.
…N from JS Draft-18+ require a PROTOCOL_VIOLATION session close for SUBGROUP_HEADER types with the reserved SUBGROUP_ID_MODE (0b11), so the uni classifier now validates the flags instead of matching the form alone. JS reads the full-width stream type, rejects the same reserved modes, and closes the session with the PROTOCOL_VIOLATION code rather than a clean close. The JS tests no longer race wall-clock timers. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Review follow-up (6311a1b):
(Written by Claude Opus 5.5) |
Verdict: MERGE (follow-up)Reviewed head: Push closes the draft gaps called out after the first pass: reserved Prior findings
BlockingNone. Non-blocking
This is an automated review, not the maintainer's decision |
…eam-types # Conflicts: # quest/m0/README.md # rs/moq-net/src/ietf/session.rs
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
(Written by OpenAI)
Reviewed moq-dev/moq at 603168241eacdc78b5621c35ed9fc66d8843911e, including the merge from main.
No new actionable bugs found in the delta since my previous review.
- Both earlier P2s are addressed: reserved subgroup modes now become session violations (Rust, JS), and JS classifies the full-width type.
- The existing JS close-code finding is also addressed: the first transport close carries 0x3, and the regression checks the peer-visible code.
Direction: the classification boundary is sound; padding and ordinary subgroup failures remain stream-scoped. The merge preserves main's reset handling and nonzero-subgroup regression.
Verification limits: source/specification review and a focused Node harness of extracted JS methods only. I did not run the repository's Bun/Rust suites, interop, or just check; their tooling is unavailable here.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Verdict: MERGE (follow-up)Reviewed head: Delta since Prior findings
BlockingNone. Non-blocking
This is an automated review, not the maintainer's decision |
Read a draft-18+ PADDING stream to the end and drop it, with no STOP_SENDING, so a peer probing for bandwidth gets the throughput it measures. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
# Conflicts: # js/net/src/ietf/connection.ts # quest/m0/ietf-legal-input.md
kixelated
changed the title
|
Maintainer decision applied: PADDING streams are now discarded (read to the end and dropped), not cancelled. No STOP_SENDING is sent.
Unknown and invalid uni types still close the session with PROTOCOL_VIOLATION. (Written by Claude Opus 5.5) |
Verdict: MERGE (follow-up)Reviewed head: Notable delta since prior review: maintainer decision applied — draft-18+ PADDING is read to EOF and dropped with no STOP_SENDING (was CANCELLED). Merge-from-main ( Delta review
Prior findings
BlockingNone. Non-blocking
This is an automated review, not the maintainer's decision |
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
(Written by OpenAI)
Reviewed ebafcf1de5d342672bcaf584f47c204830698f8c, focusing on the padding-drain change since 603168241eacdc78b5621c35ed9fc66d8843911e and its merge with main.
No new actionable bugs found.
- Direction: draining padding incrementally fits the bandwidth-probe goal. The merge preserves main's FETCH dispatch and the earlier reserved-mode, full-width-type, and protocol-close-code fixes.
- The post-EOF stop concern does not require a change: after
discard()reaches EOF, the reader is closed. ReadableStreamCancel then resolves without invoking the underlying cancellation operation. A focused check of the extractedReaderconfirmed no cancel callback after draining.
Verification: extracted-source Node checks passed for classification/close behavior, buffered and multi-chunk padding, waiting for FIN, reset propagation, and cancellation after EOF. Rust was source-reviewed only. Bun/Rust suites, browser interop, and just check were not run; the required tooling is unavailable here.
|
Ready to merge at On Grok's non-blocking note about the success-path Summary:
(Written by Claude Opus 5.5) |
1 participant
Problem
The IETF uni-stream loop sent every non-SETUP stream to the group handler. That handler refused padding and unknown types alike with INTERNAL_ERROR and left the session up. Every draft we negotiate says an unknown stream type MUST close the session. Draft-18+ also define PADDING (0x132B3E28), which the receiver MUST discard, and list invalid SUBGROUP_HEADER types (the reserved SUBGROUP_ID_MODE 0b11) that MUST close the session. So a legal padding stream was answered as an internal error, and real protocol violations were shrugged off.
js/nethad the same bug: any non-group uni failedGroup.decodeand was stopped as an error.Approach
UniType::classify(kind, version)inrs/moq-net/src/ietf/session.rsnames the stream per draft before any handler runs.GroupFlags::decodeaccepts it for the negotiated draft. That rejects the 0b11 modes, and FIRST_OBJECT before draft-18.Error::UnexpectedStreamfromrun_unis, which the driver turns into a PROTOCOL_VIOLATION session close. It stops nothing on its own.Connection.#runUnireads the full-width type first and applies the same split.Reader.discard(), which keeps no bytes buffered.Group.decodethrowsProtocolViolationfor invalid subgroup types.ProtocolViolation.ProtocolViolationon the uni or bidi path now closes the WebTransport session withSessionCode.ProtocolViolation(0x3) instead of a clean close.unknown_uni_type_does_not_claim_the_session_closedflips toan_unknown_uni_type_closes_the_session, which now covers invalid subgroup types too. There is a newa_padding_stream_is_discarded(Rust, draft-18..22, asserts no STOP_SENDING) and a newjs/net/src/ietf/connection.test.tscovering both cases (the padding writer's close resolves only once every byte is read), including a type at 2^53 and the peer-visible close code.doc/concept/standard.mdnotes the behavior.Impact
Group.decode(r, version, type?)gains an optional trailing parameter (additive), and throwsProtocolViolationfor invalid types.Alternatives
Follow-ups
accept_setup(gated server accept) still aborts every pre-SETUP uni with INTERNAL_ERROR, padding included, and never closes on an unknown type.Completes and deletes quest
quest/m0/ietf-uni-stream-types.md.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code