feat(net): carry path patterns in moq-lite AUTH_OK grants - #4277
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
AUTH_OK now encodes each grant pattern as its canonical text instead of a prefix, so literal and wildcard grants reach the presenter as issued rather than resetting the stream. moq-transport's MoQ Auth extension keeps prefixes. Completes the path patterns quest. 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. |
Decisions
(Written by Opus 5.5) |
# Conflicts: # doc/lib/js/net.md # quest/m1/auth/README.md # quest/m1/auth/patterns.md # quest/m1/auth/relay-refresh.md
![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: 7ce9e244b9
ℹ️ 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".
| let prefix = Path::decode(r, version)?; | ||
| let pattern = Pattern::subtree(prefix.as_str()).map_err(|_| DecodeError::InvalidValue)?; | ||
| let text = String::decode(r, version)?; | ||
| let pattern = Pattern::try_from(text.as_str()).map_err(|_| DecodeError::InvalidValue)?; |
There was a problem hiding this comment.
A peer can send a near-64 MiB pattern such as millions of a/ segments, and Pattern::try_from splits it into a Vec of individually allocated strings before Pattern::new enforces the 32-segment limit. This can amplify one permitted AUTH_OK message into enough allocations to exhaust the process; the JavaScript decoder has the same issue because Pattern.parse builds the complete split array first. Reject the 33rd segment before constructing segment objects in both parsers.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4554773: both parsers now split at most MAX_SEGMENTS + 1 segments before the constructor refuses. A shared golden vector (33 segments then a*b*c) reports too-many-segments only with the bound in place.
(Written by Claude Opus 5.5)
| A pattern has at most 32 segments and at most one `**`. | ||
| It has no leading, trailing, or repeated `/`, no segment with more than one `*`, and no `**` combined with other bytes in a segment. | ||
| It is canonical: a `**` is never immediately preceded by a `*` segment, since `*/**` and `**/*` match the same paths and only `**/*` is valid. | ||
| A pattern that breaks any of these rules is a PROTOCOL_VIOLATION. |
There was a problem hiding this comment.
When an acceptor sends an invalid or non-canonical pattern such as /room or */**, Rust's PresentToken absorbs the resulting decode error as a token termination, while JavaScript's auth loop catches it and likewise leaves the session open. The newly specified PROTOCOL_VIOLATION exists only in the session error space, so both implementations need to propagate this decode failure into a session close rather than merely dropping the offending token.
AGENTS.md reference: drafts/AGENTS.md:L1-L1
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not changing this here. Every malformed AUTH reply already ends just that token and leaves the session up in both languages, including an out-of-range Expires, which predates this PR. Closing the session on a malformed Auth Stream reply is a behavior change for the whole stream, not just patterns. It should be decided and tested on its own, so it is proposed as a follow-up quest.
(Written by Claude Opus 5.5)
The interop relay config now requires tokens from `moq auth serve` (#4181), which the TS harness never starts, so every connection was refused with 502 and `just test ts` captured nothing. Give it its own config, like the wasm harness. 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: 5f94754b09
ℹ️ 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".
| // Each pattern travels as its canonical text. | ||
| async function encodePatterns(w: Writer, patterns: Path.Patterns) { | ||
| await w.u53(patterns.size); | ||
| for (const pattern of patterns) await w.string(pattern.text); |
There was a problem hiding this comment.
When a JavaScript issuer accepts a JSON pattern containing an unpaired UTF-16 surrogate, Pattern.parse preserves it but Writer.string uses TextEncoder, which silently replaces it with U+FFFD. The AUTH_OK therefore advertises a different grant from AuthOk.publish, so an action the presenter believes is authorized can be rejected by the acceptor's original grant. Validate that pattern text round-trips through UTF-8 and fail encoding instead of changing the authorization scope.
AGENTS.md reference: AGENTS.md:L17-L18
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Disagree for this PR. @moq/net never writes a lite AUTH_OK outside tests; the relay (Rust, where strings are always valid UTF-8) is the acceptor. Lossy surrogate encoding is a general Writer.string property that applies to every path and name on the wire. If it matters, it belongs in Writer.string, not in the pattern encoder.
(Written by Claude Opus 5.5)
AUTH_OK hands pattern text from the peer to the parser, which split the whole string into segments before counting them, so one message of `a/a/...` could allocate millions of segments. Parse at most one past the limit in both languages; the shared vector with an invalid segment past the limit fails without it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Landing summary:
(Written by Claude Opus 5.5) |
1 participant
Problem
moq-lite-06 AUTH_OK carried grants as ANNOUNCE_REQUEST prefixes, so only a union of subtrees fit. A literal grant such as
b1.hang, or a wildcard such asroom/*/cam, reached the client as no grant at all (the stream was reset as unsupported) while the relay's origin enforced it correctly. AUTH has not shipped in a release, so this is the last chance to fix the encoding before it becomes a compatibility burden.Completes the
quest/m1/auth/patterns.mdquest: the matcher, algebra, golden vectors, fuzz target, and CAT/C4M notes already landed; only the AUTH wire was left.Approach
Publish Pattern (s),Subscribe Pattern (s)), in Rust and JavaScript. Decode refuses invalid or non-canonical text (*/**,/a,a*b*c), so each pattern has one encoding.#path-pattern) next to AUTH_OK.*,lit*lit, leading**, empty pattern), and enforcement of a wildcard grant (**/b.hangadmitsb.hangvia a zero-segment**,room/alice/micaborts outsideroom/*/cam). The prefix-refusal tests now run on moq-transport only.**/<broadcast>, and the refused publisher getsinterop-allowed-*.hang.moq-patternand@moq/patternparse at most one segment past the 32-segment limit, so peer-supplied pattern text cannot allocate millions of segments before being refused.just test tsruns on its own anonymous relay config (test/ts/relay.toml). The line's interop config requiresmoq auth servetokens since test(interop): assert each AUTH cell's grant and refuse a publish outside it #4181, so every TS connection was refused with 502.main's stalequest/m1/path-patterns.mdmerges into it by rename, so the line's merge leaves none.Impact
moq-pattern/@moq/pattern: no API change. Text with more than 32 segments now always reportsTooManySegments, even when a later segment is also invalid.moq-net: no public API change. Pattern grants now reach a lite presenter instead of failing withError::Unsupported.@moq/net: no public API change. Same behavior change;Lite.AuthOkencodes/decodes patterns.draft-lcurley-moq-liteAUTH_OK and new Path Pattern section; changelog bullet updated.Alternatives
Open decisions
*/**) rather than normalizing them. Recommend keeping both.mainstill hasquest/m1/path-patterns.md(edited by fix(auth): read and write legacy put/get token grants #4190) and moq-auth reading legacyput/get, while this line's moq-auth refuses them. Recommend resolving at the line's nextmainmerge by taking the deletion and fix(auth): read and write legacy put/get token grants #4190's moq-auth behavior.Checks
just check,just drafts check, andjust test interop --all(every cell and the grant checks pass with the pattern tokens).Follow-ups
🤖 Generated with Claude Code
(Written by Claude Opus 5.5)