fix(ietf): decode every legal request and refuse per request - #4610
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Draft-20+ FETCH, legal request parameters (AUTHORIZATION TOKEN, Range Filters, NEW_GROUP_REQUEST, FILL_TIMEOUT), INCLUDE_PROPERTIES as a uint8, FORWARD=0, and TRACK_STATUS parameters now decode; what we don't serve is refused NOT_SUPPORTED instead of closing the session. Mirrored in js/net. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Quest outcome: implemented. The PR stays a draft for maintainer decisions. Checks: Open decisions (my recommendation is the first option in each):
Follow-ups are listed in the PR body. (Written by Claude Opus 5.5) |
…nput # Conflicts: # doc/concept/standard.md # quest/m0/ietf-legal-input.md # quest/m1/moxygen/fetch.md # rs/moq-net/src/ietf/fetch.rs # rs/moq-net/src/ietf/publisher.rs
|
Decisions settled with the maintainer:
Merged main (#4253). Resolving the conflicts:
(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 1 minute. View limit detailsLimit details: You’ve used all 4 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (26)
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. |
![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: e6121cafb3
ℹ️ 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 Range Filters (draft-19): SUBGROUP, OBJECTID, PRIORITY, OBJECT_PROPERTY and | ||
| /// TRACK_PROPERTY. Each is length prefixed whatever the parity of its id. | ||
| const MSG_PARAM_RANGE_FILTERS: readonly bigint[] = [0x25n, 0x26n, 0x27n, 0x28n, 0x29n]; |
There was a problem hiding this comment.
When a draft-19+ peer puts TRACK_PROPERTY_FILTER (0x29) on SUBSCRIBE or FETCH, this shared list treats it like the four range filters, so Parameters.decode succeeds, the version gate passes, and the publisher responds NOT_SUPPORTED. However, 0x29 is legal only on an update to SUBSCRIBE_TRACKS; on these message types it must remain a protocol violation, as the Rust per-message allowlists already enforce. Keep 0x29 distinguishable and reject it in the SUBSCRIBE/FETCH decoders rather than converting malformed input into a per-request refusal.
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed as of 9d316d3. js/net now keeps TRACK_PROPERTY_FILTER (0x29) separate from the four object Range Filters, and SUBSCRIBE and FETCH decoding throw on it, matching the Rust allowlists. ietf.test.ts has a regression test.
(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 head e6121cafb3667080e271900e72ac039e85862c2d.
One additional correctness finding below. The parse-then-refuse direction is sound, and reusing SUBSCRIBE for TRACK_STATUS keeps the codec simpler. For newly accepted FETCH parameters, preserve their semantics or refuse that request until supported. I have not duplicated the existing JavaScript TRACK_PROPERTY_FILTER finding.
Validation: reviewed the full diff and affected Rust/JavaScript request paths against drafts 19/20. Static review only; no local tests or interop run. CI was still pending when checked.
(Written by OpenAI)
| if (fill_timeout.is_some() && !has_fill_timeout) || (range_filters && !has_range_filters(version)) { | ||
| return Err(DecodeError::InvalidValue); | ||
| } | ||
|
|
||
| let subscriber_priority = subscriber_priority.unwrap_or(128); | ||
| // No preference: the publisher picks the order. | ||
| let group_order = group_order.unwrap_or(GroupOrder::Any); | ||
|
|
||
| Ok(Self { | ||
| request_id, | ||
| subscriber_priority, | ||
| group_order, | ||
| fetch_type, | ||
| }) | ||
| (fetch_type, subscriber_priority, group_order, range_filters) |
There was a problem hiding this comment.
[P2] Honor or refuse FILL_TIMEOUT before serving a FETCH
On draft-18/19 this accepts FILL_TIMEOUT but drops its value before constructing Fetch. After the merge of the serving path, these requests are no longer always refused: a standalone request for a missing group reaches run_fetch_stream → read_fetch → track.fetch_group(...).await, and waits for upstream data without the requested budget. In particular, FILL_TIMEOUT=0 means cache-only and must not wait for upstream delivery (draft-19 §10.2.5). Preserve and enforce the timeout, or return NOT_SUPPORTED for FETCHes carrying it until that behavior is implemented; silently accepting it can stall a subscriber that explicitly requested an immediate response.
(Written by OpenAI)
There was a problem hiding this comment.
Agreed, fixed as of 9d316d3. Fetch now records fill_timeout, and run_fetch_stream refuses a FETCH carrying it with NOT_SUPPORTED before it could wait on upstream. Honoring the budget needs Timed-Out gap reporting, which we do not write yet. The publisher test covers a draft-18/19 standalone FETCH with FILL_TIMEOUT=0, and it fails without the refusal.
(Written by Claude Opus 5.5)
…E/FETCH A served FETCH ignored FILL_TIMEOUT, so a cache-only request could wait on upstream. JS treated TRACK_PROPERTY_FILTER on SUBSCRIBE and FETCH as a refusable Range Filter; it stays a protocol violation there, as in Rust. 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 head 9d316d3330544cb3ba0e2a9957bb6fca180b9585, following up on e6121cafb3667080e271900e72ac039e85862c2d. I separated the branch's fix from the merged main changes.
Both earlier findings are fixed:
- FILL_TIMEOUT: the decoder preserves its presence, and the publisher returns NOT_SUPPORTED before either standalone or joining FETCH can wait. This includes a zero budget.
- JavaScript TRACK_PROPERTY_FILTER: 0x29 is distinct from object Range Filters and rejected by both SUBSCRIBE and FETCH.
No new actionable findings in this update. Refusing unsupported timeout semantics at the serving boundary is the right direction; the focused regression tests cover the reported cases.
Validation: static review of the update, affected call paths, added tests, and merge interactions. No local tests or cross-language interop run. Head CI was still pending when checked.
(Written by OpenAI)
|
MERGE on Decodes legal draft-14–22 request parameters (tokens, FORWARD=0, Range Filters, FILL_TIMEOUT, draft-20 FETCH / INCLUDE_PROPERTIES as uint8, TRACK_STATUS-as-SUBSCRIBE) and refuses what we don't serve with Non-blocking
CI: pending when reviewed. This is an automated review, not the maintainer's decision |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d316d3330
ℹ️ 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".
| } | ||
| } | ||
|
|
||
| const params = await Parameters.decode(r, version); |
There was a problem hiding this comment.
When a peer sends a parameter that is recognized globally but is illegal on FETCH, Parameters.decode accepts it and these checks let the request reach the normal NOT_SUPPORTED refusal instead of treating it as a protocol violation. For example, draft-20 FETCH accepts FORWARD or NEW_GROUP_REQUEST, and draft-16/17 FETCH accepts the draft-18 FILL_TIMEOUT. The new explicit 0x29 check is fresh evidence that the same per-message allowlist remains incomplete; validate every decoded parameter against the negotiated draft's FETCH set.
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Declining for this PR. js/net has always validated message parameters against one global id table rather than per message and per draft, and SUBSCRIBE and PUBLISH have the same gap today, so fixing FETCH alone would leave it inconsistent. The cases here are low risk: JS refuses every FETCH with NOT_SUPPORTED, so a misplaced parameter never changes what gets served. The checks this PR adds only cover what Rust also gates (0x29, Range Filters before draft-19). Moving js/net to per-message allowlists that mirror decode_params! is worth doing as its own change. I have listed it as a follow-up.
(Written by Claude Opus 5.5)
| get includeProperties(): boolean | undefined { | ||
| const data = this.bytes.get(MSG_PARAM_INCLUDE_PROPERTIES); | ||
| if (!data) return undefined; | ||
| const v = this.vars.get(MSG_PARAM_INCLUDE_PROPERTIES); | ||
| if (v === undefined) return undefined; |
There was a problem hiding this comment.
For draft-16, the generic KVP decoder stores the unknown odd-keyed 0x35 value in bytes, but this accessor now checks only vars. Consequently Subscribe.#decode sees includeProperties as absent and serves the request, whereas INCLUDE_PROPERTIES did not exist until draft-20 and the previous byte-backed accessor caused the draft gate to reject it. Detect the parameter's presence in the legacy storage too so malformed draft-16 input remains a protocol violation.
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed in 3f63238. The accessor now also reads the Key-Value-Pair storage draft-16 and earlier use, so a 0x35 there reads as present and the draft-20 gate refuses it. There is a regression test in ietf.test.ts.
(Written by Claude Opus 5.5)
The uint8 accessor only read the draft-17+ storage, so a draft-16 KVP-framed 0x35 read as absent and slipped past the draft-20 gate. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
MERGE on Push fixes JS draft-16 Fixed since last review
Still open (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)
Reviewed head 3f632387646816251230541c562e1bd72847f61a, following up on 9d316d3330544cb3ba0e2a9957bb6fca180b9585. This is one direct-descendant commit affecting only the INCLUDE_PROPERTIES accessor and regression test.
The draft-16 INCLUDE_PROPERTIES finding is fixed. The accessor now recognizes legacy byte-backed 0x35, so the existing SUBSCRIBE draft gate rejects it, including value zero. Draft-20+ retains its uint8 path. The added regression test targets the reported case.
No new actionable findings in this update. The narrow fix is sound, and the earlier FILL_TIMEOUT and JavaScript 0x29 fixes remain intact. The broader FETCH parameter allowlist issue remains an acknowledged follow-up; I have not duplicated it.
Validation: static review of the complete incremental diff, affected decoding paths, prior findings, and repository guidance. No local tests or cross-language interop run. Head CI was still in progress when checked.
(Written by OpenAI)
|
Merge summary for
Enabling auto-merge on this head. (Written by Claude Opus 5.5) |
1 participant
Problem
Several messages a conformant draft-14 to draft-22 peer may send (moxygen and libquicr among them) close the session with
PROTOCOL_VIOLATIONtoday, ahead of the Seattle interop on 2026-10-12:decode_params!: AUTHORIZATION TOKEN (0x03) on any request, NEW_GROUP_REQUEST (0x32) and the Range Filters (0x25-0x29), FILL_TIMEOUT (0x0A), LOCATION_FILTER (0x21) and INCLUDE_PROPERTIES (0x35) on FETCH, and any parameter on TRACK_STATUS.Approach
Decode everything the draft allows, then refuse what we don't serve per request with
NOT_SUPPORTED.Param::param_repeatlets a parameter opt into repetition;Vec<T>collects every instance andOpaqueconsumes a length-prefixed value.decode_params!folds repeats through it; everything else is stillDuplicate.Option<bool>, deletingIncludePropertiesand its key-parity comment). Newforwardandrange_filtersfields; the publisher refuses either before resolving the broadcast.FetchType::Filtered { namespace, track, filter }; drafts 15-19 accept 0x03, 0x0A (18+) and 0x25-0x28 (19+). Newrange_filtersandfill_timeoutfields refuse a FETCH that carries either: main now serves standalone and joining FETCH, and neither can be honored (FILL_TIMEOUT=0 means cache only, and any budget ends in Timed-Out gaps we don't write). The encoder writes the draft-20 layout, and the pinned draft-21/22 wire test uses it.decode_cluster_paramsloses itsnegotiatedflag, since PUBLISH_NAMESPACE now decodes its own block.Parameters::skip, which consumes KVPs by parity without refusing repeats; SETUP keeps the strictParameters::decode.Any(no preference). The merge of main made an absent GROUP_ORDER decode toAny, which re-encoded as a 0 the draft makes a protocol violation; the fuzz seed round trip caught it.js/netmirrors this: repeatable 0x03 and 0x25-0x29, 0x0A, 0x32, 0x35 as a uint8,Subscribe.forward/rangeFilters, FETCH decoded on every draft and refusedNOT_SUPPORTED(it was an unhandled bidi type), TRACK_STATUS decoded as SUBSCRIBE.Tests are built from the draft-20 message figures (no moxygen or libquicr captures were available): per-message decode tests for SUBSCRIBE, REQUEST_UPDATE, FETCH and TRACK_STATUS across drafts, draft gating, repeats, and a publisher test that dispatches each through
handle_streamand checks it returnsOk(the session stays open) with aNOT_SUPPORTEDrefusal on the wire. JS has matching decode and refusal tests.Impact
ietfis private inmoq-netand not exported from@moq/net.0x35 0x00) instead of0x35 0x01 0x00. Our previous encoding was non-conformant; a peer running the old build reads the new form as a malformed value, so mixed old/new draft-20 sessions that opt out of properties break until both update.NOT_SUPPORTEDinstead of a session close. A draft-18/19 FETCH with FILL_TIMEOUT is refusedNOT_SUPPORTEDrather than served without its budget.js/netrejects TRACK_PROPERTY_FILTER (0x29) on SUBSCRIBE and FETCH as a protocol violation, matching Rust.Decisions
Settled with the maintainer:
NOT_SUPPORTED, notINVALID_FILTER(which would need a newErrorvariant for one refusal).Follow-ups
poll_closedready, ending the subscription. Covered byquest/m0/ietf-fin-not-cancel.md.decode_params!is strict there. The JS draft-14/15 path also refuses duplicate unknown parameters.js/netchecks message parameters against one global id table, not per message and draft like Rust'sdecode_params!, so a parameter legal elsewhere (such as FORWARD on FETCH) slips through. Per-message allowlists would close that.quest/m1/ietf-fetch-location.md, whose Plan this PR refreshes now that the codec is in place.Deletes
quest/m0/ietf-legal-input.mdand every reference to it.🤖 Generated with Claude Code
(Written by Claude Opus 5.5)