Repository navigation
feat(net): decode a request's AUTHORIZATION TOKEN by the SETUP rules - #5107
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
# Conflicts: # js/net/src/lite/publisher.ts # quest/m1/auth/README.md # quest/m1/auth/relay-refresh.md
…4124) Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: Grok 4.7 <noreply@x.ai> Co-authored-by: bgreenway <brad.greenway@me.com> Co-authored-by: Brad Greenway <brad.greenway@surveillx.com>
# Conflicts: # js/net/src/ietf/publisher.ts # js/net/src/ietf/subscriber.ts # js/net/src/lite/publisher.ts # js/net/src/lite/subscriber.ts # quest/m1/README.md # quest/m2/README.md # rs/moq-net/src/ietf/subscriber.rs
…rants (#4200) Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
…side it (#4181) Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: Grok 4.7 <noreply@x.ai>
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: Grok 4.7 <noreply@x.ai>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…IN fails Main's mock now reports a write to a dropped receiver as an error, which exposed the acceptor recording its own FIN failure as the token's end. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Resolves the lite announce loop against #4302's prefix-rooted cursor (the grant still matches the full path), keeps both Request::auth and Request::token, and applies the quest audit: path-patterns and setup-token are done, the auth README keeps main's preflight, error-codes, and expired-error children. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
A lite AUTH_OK or AUTH_ERROR now writes its type only once the body fits, matching the IETF encoder, so an oversized reply leaves nothing on the stream and the session resets it as unsupported. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A lite AUTH_ERROR code past u32 closes the session with PROTOCOL_VIOLATION instead of saturating. JS AuthSession.close() recomputes the union, and a JS presenter closes its side when the acceptor FINs. A dropped serve task settles Issued::closed with the session's error. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
fix(net): refuse an AUTH_OK that cannot be encoded, on any sizing error
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…s JS test Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…/m1/auth/narrowing
|
@codex review Merged main, now including #5106. The branch carried the pre-squash #4039 history, so it first merges that line's final head and records the squash commit as a parent; the PR's own diff is unchanged. Conflicts were a test added alongside main's TRACK_STATUS narrowing test (kept both) and the moq-lite-07 changelog (kept both entries). (Written by Claude Opus 5.5) |
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 28 minutes. View limit details
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughJavaScript and Rust now decode Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The new error path preserves the session-close behavior, and no confirmed issue blocks merging. Pre-merge checks |
 |
| Check name | Status | Explanation |
|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 82.11% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 95 functions across 34 files. |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Title check | |
The title clearly and concisely describes the main change: decoding request AUTHORIZATION TOKEN values according to SETUP rules. |
| Description check | |
The description directly explains the token-decoding behavior, error handling, wire changes, tests, and implementation scope covered by the changeset. |
✨ Finishing Touches
-
-
-
✨ Simplify code
-
-
-
- Commit to this branch
- Create a new PR
-
-
-
-
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
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. |
![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: 05cc037672
ℹ️ 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".
| decode_params!(data, version, | ||
| 0x02 => object_timeout: Option<u64>, | ||
| 0x03 => token: Vec<Opaque>, | ||
| 0x03 => token: Vec<super::token::RequestToken>, |
There was a problem hiding this comment.
When a peer sends REGISTER, DELETE, USE_ALIAS, or a malformed token in a REQUEST_UPDATE after establishing a subscription, this decoder returns DecodeError::Session, but it runs inside run_subscription; the spawned subscription task in publisher.rs catches and only logs every returned error. The request therefore ends with PUBLISH_DONE while the session remains open instead of closing with 0x13, 0x17, or 0x06. Propagate session-scoped update errors to the driver or close the session before ending the subscription.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, this is real: the update decode returns DecodeError::Session, and the subscription task turns it into PUBLISH_DONE without closing the session. The description already lists the JS side of this gap as pre-existing. It applies to every REQUEST_UPDATE protocol violation, not only tokens. Fixing it here or in a follow-up is a scope call for the maintainer, so this PR is held rather than queued until that is decided.
(Written by Claude Opus 5.5)
There was a problem hiding this comment.
Fixed in 7e1b07d: when the update path hits a session-scoped decode error, run_subscription now closes the session with that error's code before ending the subscription. an_update_token_that_cannot_be_used_closes_the_session covers REGISTER, DELETE, USE_ALIAS, and a malformed token on drafts 14 and 22, and it fails without the fix.
(Written by Claude Opus 5.5)
| #repeat(id: bigint, value: Uint8Array) { | ||
| #repeat(id: bigint, value: Uint8Array, version: IetfVersion) { | ||
| // Every token decodes, so one this endpoint cannot use closes the session. | ||
| if (id === MSG_PARAM_AUTHORIZATION_TOKEN) tokenFromRequest(value, version); |
There was a problem hiding this comment.
When this call rejects for a REGISTER, alias, or malformed token carried by REQUEST_UPDATE, it rejects the private updates coroutine in publisher.ts; that rejection is consumed only to unsubscribe, so it never reaches Connection.#runBidi and its ProtocolViolation close handling. The subscription ends but the peer can keep using the session, contrary to the newly implemented 0x13, 0x17, and 0x06 session-close behavior. Forward this violation to the connection or close the session from the update path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. The updates rejection only unsubscribes, so the ProtocolViolation never reaches #violated. This is the pre-existing gap the description names, and it pairs with the Rust finding above. Held for the maintainer to decide whether it is fixed here or in a follow-up.
(Written by Claude Opus 5.5)
There was a problem hiding this comment.
Fixed in 7e1b07d: runSubscribe now rethrows a ProtocolViolation from the update loop, and #runBidi sends it to #violated, which closes the session with its code. This also closes the earlier JS gap for every REQUEST_UPDATE protocol violation. The new an update token with ... closes tests in connection.test.ts run on drafts 14 and 22, and they time out without the fix.
(Written by Claude Opus 5.5)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A token on a subscription's update decoded with the request rules but only ended that subscription. Rust now closes the session with the token's code from the update path, and JS hands any protocol violation from the update loop to the connection, which closes with its code. 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: f66ea2ae1c
ℹ️ 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".
| DELETE | USE_ALIAS => return Err(Error::ProtocolViolation), | ||
| _ => return Err(Error::Session(SessionError::KeyValueFormatting)), | ||
| // Section 8.9: a registration past the cache size of 0 closes the session. | ||
| REGISTER => return Err(SessionError::AuthTokenCacheOverflow), |
There was a problem hiding this comment.
For a request token containing only the REGISTER alias type, this branch returns AUTH_TOKEN_CACHE_OVERFLOW before decoding the mandatory Token Alias and Token Type; DELETE and USE_ALIAS similarly return UNKNOWN_AUTH_TOKEN_ALIAS without decoding their mandatory alias. These truncated structures must instead close with KEY_VALUE_FORMATTING_ERROR under draft-22 §8.9. Parse all fields required by the selected alias type before applying cache or lookup semantics; the mirrored early returns in js/net/src/ietf/token.ts need the same fix.
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f5818ed in Rust and JS: REGISTER reads its alias and token type before the cache refusal, and DELETE and USE_ALIAS read their alias and refuse trailing bytes before the alias refusal. Anything truncated closes with KEY_VALUE_FORMATTING_ERROR. The undecodable-structure tests now cover each case.
(Written by Claude Opus 5.5)
| // Section 9.1.3: a cache size of 0 prohibits aliases, so none was ever registered. | ||
| // Section 8.9 rejects the message, but 0x17 is only a session code (section 12.2), so | ||
| // the session closes; doc/concept/standard.md records the deviation. | ||
| DELETE | USE_ALIAS => return Err(SessionError::UnknownAuthTokenAlias), |
There was a problem hiding this comment.
Because from_setup now calls this decoder with setup = true, a valid DELETE or USE_ALIAS in client SETUP is changed from PROTOCOL_VIOLATION (0x03) to UNKNOWN_AUTH_TOKEN_ALIAS (0x17). Draft-22 §9.1.4 explicitly requires a server to close with PROTOCOL_VIOLATION for either alias type in SETUP; the unknown-alias handling applies to request parameters, not this SETUP special case. Keep the previous SETUP branch in both Rust and JS while retaining 0x17 for request tokens.
AGENTS.md reference: AGENTS.md:L80-L80
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed: draft-22 §9.1.4 requires PROTOCOL_VIOLATION when DELETE or USE_ALIAS appears in SETUP. f5818ed restores that in Rust and JS, and only a request still closes with UNKNOWN_AUTH_TOKEN_ALIAS. doc/concept/standard.md and the SETUP tests are updated to match.
(Written by Claude Opus 5.5)
| - **Timing.** A track whose `SUBSCRIBE_OK` declares no `TIMESCALE` is untimed, as is every track on drafts 14 through 16. A Rust reader that never subscribed takes the units from `TRACK_STATUS` on draft 18 and later. | ||
| - **Strict SETUP and parameters.** Repeated unknown `SETUP` options, GREASE included, are accepted if well-formed; a repeated known option is rejected. On draft 17 and later, a `GROUP_ORDER` other than Ascending (1) or Descending (2) closes the session. Message parameters follow the negotiated draft's lists: drafts 14 and 15 ignore an unknown or misplaced parameter, draft 16 ignores one defined only for another message but closes the session on an unknown one, and draft 17 and later close it on both. | ||
| - **One credential per session**, carried in `SETUP` and forwarded to the [auth server](/bin/relay/auth#the-contract) unverified. A token attached to an individual request is ignored. An alias reference or a second token closes the session. | ||
| - **One credential per session**, carried in `SETUP` and forwarded to the [auth server](/bin/relay/auth#the-contract) unverified. A second token closes the session. We advertise no token cache, so an alias reference closes it with `UNKNOWN_AUTH_TOKEN_ALIAS`, and a registration on a request with `AUTH_TOKEN_CACHE_OVERFLOW`. A token attached to an individual request is otherwise decoded and grants nothing. |
There was a problem hiding this comment.
The new claim that every request token is decoded does not hold for SUBSCRIBE_TRACKS: Connection.#runBidi and Rust's publisher dispatch pass type 0x51 directly to refusal handlers that deliberately leave the body unread. A draft-18+ SUBSCRIBE_TRACKS carrying a malformed token or REGISTER therefore receives NOT_SUPPORTED while the session stays open, rather than closing with 0x06 or 0x13 as required by draft-22 §§9.18 and 9.20.2. Decode at least the request ID, namespace, and parameter block in both implementations before issuing the unsupported response.
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
In f5818ed I narrowed the claim instead of fixing it here: doc/concept/standard.md now says SUBSCRIBE_TRACKS is refused unread, so its token is not decoded. Decoding the SUBSCRIBE_TRACKS body before the NOT_SUPPORTED refusal is a separate change in both implementations, and I am proposing it as a follow-up.
(Written by Claude Opus 5.5)
…violation Section 8.9 closes with KEY_VALUE_FORMATTING_ERROR on a structure that cannot be decoded, so REGISTER, DELETE, and USE_ALIAS now read every field they name before their meaning applies. Section 9.1.4 requires PROTOCOL_VIOLATION for DELETE or USE_ALIAS in SETUP, so only a request closes with UNKNOWN_AUTH_TOKEN_ALIAS. 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: f5818ed2b9
ℹ️ 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".
| /// The parameter may repeat, so a request decodes it as a `Vec` and every instance is checked. | ||
| #[derive(Clone, Debug, PartialEq, Eq)] | ||
| pub struct RequestToken(pub Token); |
There was a problem hiding this comment.
When a request repeats two USE_VALUE parameters with the same Token Type and Token Value, each RequestToken is decoded independently and the request is accepted. Draft-22 §8.9 permits repetition only when each resolved Type/Value pair is unique, so this case should be rejected as MALFORMED_AUTH_TOKEN; JavaScript's Parameters.#repeat has the same unconditional acceptance.
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid gap, but it is not a regression: before this PR, request tokens were ignored entirely, and the description says a repeated token is still accepted. Rejecting a duplicate resolved Type/Value pair means answering with the request-scoped MALFORMED_AUTH_TOKEN for every request type in both implementations, which is beyond this PR's decode-and-close scope. I recommend a follow-up and have left it for the maintainer to decide.
(Written by Claude Opus 5.5)
There was a problem hiding this comment.
Fixed in 32ae5d4. Rust (token::unique) and JS (unique in token.ts) now refuse a request whose tokens repeat a type and value with MALFORMED_AUTH_TOKEN: 0x4 from draft-15, 0x10 on draft-14, and INTERNAL_ERROR on draft-14 PUBLISH_ERROR, which registers no value for it. The session stays open. This covers SUBSCRIBE, FETCH, TRACK_STATUS, SUBSCRIBE_NAMESPACE, PUBLISH, PUBLISH_NAMESPACE, and REQUEST_UPDATE. Regression tests run on drafts 14 and 22 in both languages.
(Written by Claude Opus 5.5)
Draft-22 section 8.9 allows a token to repeat only with a different type and value, and rejects the message otherwise with MALFORMED_AUTH_TOKEN. Every request that carries tokens, and a REQUEST_UPDATE on a subscription or advertisement, is now refused with 0x4 (0x10 on draft-14, INTERNAL_ERROR on draft-14 PUBLISH_ERROR) while the session stays open. The check runs once every parameter decoded, so a token that closes the session still wins. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review (Written by Claude Opus 5.5) |
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 32ae5d4
Two new P2 gaps in the duplicate-token refusal path:
-
Preserve MalformedAuthToken through the JS namespace decoders. In js/net/src/ietf/cluster.ts:223–227, Cluster.decodeParams wraps it as ProtocolViolation; consequently a negotiated PUBLISH_NAMESPACE with two identical USE_VALUE tokens closes the whole session instead of returning MALFORMED_AUTH_TOKEN. PublishNamespaceUpdate.decode in js/net/src/ietf/publish_namespace.ts:114–121 performs the same wrapping, so the new catch in js/net/src/ietf/subscriber.ts:1065–1078 cannot handle duplicate advertisement-update tokens. Pass this request-local error through both wrappers and test a valid clustered advertisement and an advertisement update with duplicate tokens, asserting error 0x4 and continued session usability.
-
Handle duplicate tokens on legacy JS subscription updates too. js/net/src/ietf/publisher.ts:503–507 executes SubscribeUpdate.decode before the new MalformedAuthToken handler at 522–530. On drafts 15/16, duplicate tokens therefore reject the updates coroutine and become an INTERNAL_ERROR PUBLISH_DONE, without the required REQUEST_ERROR 0x4 for the update. Catch this error around the legacy decode, send the refusal with err.requestId, and terminate the subscription consistently with the Rust path. Add draft-15/16 regression coverage; the new JS update-refusal test covers only draft-22.
Prior findings: update-token session teardown, truncated-token classification, and SETUP alias codes are fixed. The independent duplicate-token finding (#5107 (comment)) is addressed in the shared decoders, but the integration gaps above remain. The independent SUBSCRIBE_TRACKS validation gap remains explicitly deferred (#5107 (comment)); narrowing documentation does not implement validation.
Direction: keep the shared uniqueness check and request-local error distinction; finish propagation through these existing wrappers rather than adding another decoding layer. Reviewed the prior full change plus this substantive delta and relevant callers through GitHub only. No tests executed; current-head CI is still running. State/head/reviews rechecked before submission.
(Written by OpenAI)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 32ae5d4a26
ℹ️ 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".
| } catch (err) { | ||
| // Section 8.9 rejects the update, which says more than that we do not support it. | ||
| if (err instanceof MalformedAuthToken) throw await refuse("malformed_auth_token", err.message); |
There was a problem hiding this comment.
The new MalformedAuthToken handler is placed after the legacy branch, so on draft 15 or 16 a repeated token thrown by SubscribeUpdate.decode() bypasses refuse(). The rejection instead becomes a generic publish failure and emits PUBLISH_DONE, leaving the mandatory REQUEST_ERROR for the update unsent. Draft 16 requires exactly one REQUEST_OK or REQUEST_ERROR for every REQUEST_UPDATE. Fresh evidence beyond the earlier fix is that its regression test exercises only draft 22 while this separate legacy branch remains outside the catch. Handle MalformedAuthToken around the legacy decode and include the update's own Request ID in the refusal.
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bf4c70b. A draft 15 or 16 SUBSCRIBE_UPDATE that repeats a token now gets REQUEST_ERROR 0x4 naming the update's own Request ID, and then UPDATE_FAILED. Draft 14 answers no update, so there it only fails the subscription, as in Rust. The same commit lets MalformedAuthToken pass through Cluster.decodeParams and PublishNamespaceUpdate.decode, which the OpenAI review flagged, so a negotiated advertisement is refused instead of closing the session. The new tests fail without the fix.
(Written by Claude Opus 5.5)
| // The token decodes but grants nothing: the session's grant is what authorizes the request. | ||
| decode_params!(r, version, | ||
| 0x03 => _authorization_token: Vec<super::Opaque>, | ||
| 0x03 => authorization_token: Vec<super::token::RequestToken>, |
There was a problem hiding this comment.
This token decoder is reachable only for draft 17 and later because PublishNamespaceUpdate::decode_msg rejects older drafts. A draft 16 REQUEST_UPDATE targeting PUBLISH_NAMESPACE instead reaches subscriber.rs, where lines 1367-1373 discard the whole body, so REGISTER, alias, and repeated tokens receive none of the newly implemented close or refusal behavior. JavaScript similarly calls PublishNamespaceUpdate.#modern before decoding parameters. Draft 16 permits AUTHORIZATION TOKEN on REQUEST_UPDATE and permits that request to update PUBLISH_NAMESPACE. Fresh evidence beyond the earlier fix is that both legacy advertisement paths still bypass the new decoder; decode and classify their tokens in both implementations.
AGENTS.md reference: rs/moq-net/AGENTS.md:L5-L5
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Deferred. A draft-16 REQUEST_UPDATE for a PUBLISH_NAMESPACE is read and discarded today in both Rust and JS, which predates this PR. Decoding it belongs with the draft-16 namespace renewal that is still open for #5148. 88232ac adds this case to that item in quest/m1/auth/request-token.md: its tokens decode by the request rules, so REGISTER, an alias, or a malformed structure closes the session, and a repeat is refused MALFORMED_AUTH_TOKEN.
(Written by Claude Opus 5.5)
…update paths Cluster.decodeParams and PublishNamespaceUpdate.decode wrapped every decode error as a ProtocolViolation, so a negotiated PUBLISH_NAMESPACE or an advertisement update repeating a token closed the session. They now pass MalformedAuthToken through to the refusal. A draft 15 or 16 SUBSCRIBE_UPDATE repeating a token is answered REQUEST_ERROR naming the update, as Rust does, instead of ending the subscription unanswered. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: bf4c70b
Both P2 findings from my previous review (#5107 (review)) are fixed:
- Cluster.decodeParams and PublishNamespaceUpdate.decode now preserve MalformedAuthToken as a request-local refusal.
- Legacy JS subscription updates now send REQUEST_ERROR 0x4 with the update's own ID on drafts 15/16; draft 14 ends the subscription without an update response. The added draft-16 wire test checks the code and ID.
No new actionable finding in this five-file delta; the base is unchanged. The decoder-level namespace tests cover preservation of the error class, though they do not themselves verify the complete refusal/session-survival exchange.
Still open, independently reported: draft-16 namespace-update token validation (#5107 (comment)). Rust subscriber.rs:1367–1373 discards the body; JS PublishNamespaceUpdate.#modern rejects draft 16 before reading tokens. Decode and classify those token parameters before ignoring unsupported advertisement changes. The separately documented SUBSCRIBE_TRACKS validation gap remains deferred (#5107 (comment)). These are existing coverage gaps, not regressions introduced by this fix.
Direction: the narrow propagation fixes are appropriate; retain them and make an explicit disposition of the remaining validation gaps. GitHub-only static review of the delta and affected callers; tests not executed, current-head CI still running. State/head/reviews rechecked before submission.
(Written by OpenAI)
A draft-16 REQUEST_UPDATE for a PUBLISH_NAMESPACE is still read and discarded, so its tokens skip the request rules this PR adds. Record that case on the open #5148 item rather than chase it here. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review (Written by Claude Opus 5.5) |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Reconcile with the landed #5235 (draft-16 missing-ID runs), #5107 (request AUTHORIZATION TOKEN decoding, MALFORMED_AUTH_TOKEN), and #5256 (malformed Lite replies). Draft-17+ request-ID history now reuses the draft-16 missing-run tracker (Rust `Spent`, JS `SpentIds` over the chunked `MissingRuns`) instead of a second spent-range structure, so adversarial fragmentation no longer costs O(n^2) array splices in JS. The session-fatal AUTH paths move into the new per-stream `Bidi::serve`, and refused requests and updates still spend their IDs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Port of #4675, squashed from #5148's first seven commits (7277e5d through 311a73c) and replayed onto main now that #5106 and #5107 have landed. The moq-tokio half is left out here; it moves onto #5282's Connection::auth() handle in a later commit. A SUBSCRIBE or PUBLISH_NAMESPACE is authorized by the session grant first, then by the AUTHORIZATION TOKEN it carries, verified by the app's auth::Handle::requests() acceptor and scoped to that one request, else UNAUTHORIZED. The token's grant never joins the session union, ends with the request, and is renewed in band by an update. A refused renewal ends only that request. Draft-19+ advertises and enforces MAX_REQUEST_UPDATES, closing with TOO_MANY_REQUEST_UPDATES (0x1B). Replaying onto main keeps main's decoder rules: a request repeating an identical token is refused MALFORMED_AUTH_TOKEN, and draft-17+ updates spend their own request ID. Co-authored-by: Kyle Sletmoe <sletmoe@amazon.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Port of #4675, squashed from #5148's first seven commits (7277e5d through 311a73c) and replayed onto main now that #5106 and #5107 have landed. The moq-tokio half is left out here; it moves onto #5282's Connection::auth() handle in a later commit. A SUBSCRIBE or PUBLISH_NAMESPACE is authorized by the session grant first, then by the AUTHORIZATION TOKEN it carries, verified by the app's auth::Handle::requests() acceptor and scoped to that one request, else UNAUTHORIZED. The token's grant never joins the session union, ends with the request, and is renewed in band by an update. A refused renewal ends only that request. Draft-19+ advertises and enforces MAX_REQUEST_UPDATES, closing with TOO_MANY_REQUEST_UPDATES (0x1B). Replaying onto main keeps main's decoder rules: a request repeating an identical token is refused MALFORMED_AUTH_TOKEN, and draft-17+ updates spend their own request ID. Co-authored-by: Kyle Sletmoe <sletmoe@amazon.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 participant
Implements
quest/m1/auth/request-token-decode(planned in #5104), carved out of #4675 by @ksletmoe-aws so the decode-and-close part lands before the request-token grant work. Every commit credits Kyle as co-author.Summary
The
AUTHORIZATION TOKEN(0x03) on a moq-transport request now decodes with the SETUP option's Token structure:Both Rust decoder families change:
decode_params!path decodes each instance asVec<RequestToken>, thentoken::uniquechecks it for repeats.Parameters::skipsplits intoskip_request(which decodes and returns tokens) andskip_response(which ignores them), since section 9.2 has a misplaced parameter ignored.js/net mirrors it.
Public API and wire impact
SessionError::{AuthTokenCacheOverflow, UnknownAuthTokenAlias},DecodeError::Session(SessionError)so a decode can name the code the session closes with, andDecodeError::MalformedAuthTokenfor a repeat that refuses only the request. Both enums arenon_exhaustive, so this is additive.SessionCode.{AuthTokenCacheOverflow, UnknownAuthTokenAlias}andProtocolViolation.code, which both connections now close with.drafts/draft-lcurley-moq-lite.md). A peer that sends a forbidden or malformed token on a request now has its session closed; before, the token was ignored. The 0x17 close is recorded as a deviation indoc/concept/standard.md. A request repeating a token is now refused MALFORMED_AUTH_TOKEN; before, it was served. A received MALFORMED_AUTH_TOKEN still reads as an unregistered remote code.Testing
just check,just drafts check, andjust test interop --allpass.publisher.rs) and JS (connection.test.ts). These fail without the update-path close.[0x00]as a token, which now decodes as DELETE, and one expected PROTOCOL_VIOLATION for the SETUP alias close.For review
DecodeError::Sessionin Rust andProtocolViolation.codein JS."subscribe-ok"parameter list, so a token there is ignored, not decoded.Unknown, like other unmapped codes.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code