Skip to content

feat(net): in-band AUTH (questline) - #4039

Merged
kixelated merged 88 commits into
mainfrom
quest/m1/auth/README
Oct 10, 2026
Merged

kixelated merged 88 commits into
mainfrom
quest/m1/auth/README

Conversation

@kixelated

@kixelated kixelated commented Sep 24, 2026 •

Copy link
Copy Markdown
Collaborator

Questline umbrella for /quest/m1/auth/README.md. Per quest-flat-lines, the line branch retires: main is merged in and the maintainer lands it with /quest-complete. The line's remaining children (listed under Required in its README) PR straight to main afterwards.

Problem

A publisher whose token does not cover what it publishes waits forever: nothing on either side says it was refused. Tokens ride the URL, so a session cannot outlive its credential, and two tokens need two connections.

Approach

One AUTH stream per token on moq-lite 07 (moq-lite-07-wip), and the MoQ Auth extension on moq-transport draft-17+. Each side presents its connection credential right after setup and learns its grant. A session's scope is the union of its grants. A publication outside the grant closes the session with Unauthorized; a grant that narrows cancels only what falls outside, and the session stays up. auth::Handle::authorize resizes a live session on any version, which the relay uses to re-authorize in place on revalidate.

The line merged main (218 commits, then 69 more for landing) and applied the 2026-10-08 audit notes. The last merge took main's FETCH_OK track properties (#5145): a FETCH reads its track info up front, inside the grant race, and FETCH_OK carries the properties on the gated response path, so the joining FETCH no longer carries the subscription's timescale.

  • UNAUTHORIZED moves from stream code 0x3A to 0x3B in Rust, JS, moq-ffi, moq-c, and the lite draft, since main assigned 0x3A to NOT_FETCHABLE. It stays on lite-06 (feat(net)!: AUTH moves to moq-lite-07-wip #5004).
  • The line's AUTH code is ported to main's Encoder/Decoder codec, time::Clock, and moq_net_sim tests.
  • Quests: dropped the edits to quest/m2/announce-shapes.md, quest/m3/route-trust.md, and quest/m0/wildcard/README.md (deleted on main); p2p keeps main's node-bound peer grants; the auth children take main's audited plans; the README records the 0x3B decision.
  • Docs keep main's feature-entry layout (docs: keep the site on features and correct publisher restarts #5033), with one auth entry each. Relay revalidation now documents in-place resizing.

Review findings fixed here, each with a regression test that fails without it:

  • A standalone moq-transport FETCH (including draft-20+ LOCATION_FILTER) resolved its namespace without the grant. It is now refused up front and held to the grant through its whole response, so one waiting on stream credit when the grant narrows stops there.
  • TRACK_STATUS (new on main) answered outside the grant. It is now refused up front, never reaching the origin, and held to the grant while the track resolves.
  • The JS lite publisher announced before the setup token was answered. It now waits, like Rust and JS IETF, and a local close ends that wait.
  • A JS TRACK is re-checked once its broadcast and track resolve.
  • A joining FETCH holds its own gate on its subscription's namespace, since the cache it answers from outlives the subscription, and a TRACK_STATUS answer is reset if the grant narrows while it is written. Both have credit-starved regression tests.
  • A JS lite TRACK_INFO blocked on flow control when the grant shrinks is reset, matching Rust's TRACK_STATUS.
  • The IETF AUTH_ERROR reason is capped from its length prefix, like lite, and a refusal on a finished auth issue queues nothing.
  • A Rust IETF presenter refuses an AUTH_OK or AUTH_ERROR with bytes past its message, like every framed message.

Decisions taken unattended (recommended option each):

  • ✅ 0x3B for UNAUTHORIZED (the next free code) / renumber NOT_FETCHABLE instead.
  • ✅ Drop the line's 64 MiB JS message ceiling for main's 65,535, keeping the line's { id, max } encode options.
  • ✅ Group Session::new's handles into a session::Handles struct / allow clippy's arity lint.
  • ✅ A JS lite subscription revoked during setup aborts its in-flight TRACK exchange through main's cancel signal / keep answering TRACK_INFO first.

Impact

  • moq_net::auth (Grant, Handle, Token, Watch, Requests, Request, Issued), Session::auth(), and server::Handshake::auth() are new.
  • StreamError::Unauthorized (0x3B) is new; moq-ffi's MoqProtocolKind::Unauthorized doc and moq-c's table say 0x3B.
  • Wire: moq-lite 07 gains the Auth Stream (0x7) with AUTH, AUTH_OK, and AUTH_ERROR. The lite stream error table gains 0x3B UNAUTHORIZED on lite-06. drafts/draft-lcurley-moq-auth.md is the moq-transport extension.
  • JS: connection.auth (grant, add, requests), StreamCode.Unauthorized (0x3B). The lite Publisher constructor's fifth argument is now { grant, ready } (internal).
  • moq-pattern (Rust and JS): parsing stops splitting one segment past the limit, so a peer's oversized pattern is refused cheaply. No API change.
  • Relay: revalidate resizes an attached session in place instead of closing it on a narrower grant.

CI: Interop's FFI-publisher browser cells (go/cpp/python -> js) fail on main too; FFI publisher stall owns that (#5174). TS compliance duration-fidelity also flakes on main (#5158, #5140); TS duration fidelity owns that.

Alternatives

Re-cutting each child against main was rejected in quest-flat-lines: merging main in is less churn.

Follow-ups

  • Still open from earlier reviews, each owned by a child quest in Required: a malformed AUTH_OK leaving enforcement off (Malformed grant) and the other AUTH violations (AUTH violations).
  • OpenAI's request-lifetime grant gate is now the Request gate child, planned with the maintainer on 2026-10-09:
    • Goal: every incoming request is checked at dispatch and held to the grant for its life, structure only. ✅ Yes / also audit gaps
    • Scope: ✅ Rust and JS / Rust only
    • Shape: ✅ the dispatcher owns the gate and hands it to the handler / wrap the handler future
    • Placement: ✅ m1/auth child / m2
    • Rank: ✅ after JS fetch watch / first / last
    • Commit: ✅ in this PR / separate quest PR
  • Codex's security review found auth().requests() unbounded: a peer can queue tokens faster than they are verified. Nothing calls it yet; Relay tokens, its first consumer, now owns bounding it.
  • Not tracked yet: AUTH_OK always sends expires: None from the relay, and nothing acts on a received expiry; Rust and JS disagree on whether a NOT_SUPPORTED refusal settles the union. Offer /quest-plan for these.
  • quest-flat-lines.md records this line as landed in this PR.

Closes

(Written by Claude Opus 5.5)

🤖 Generated with Claude Code

kixelated and others added 3 commits September 24, 2026 10:36
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
kixelated and others added 2 commits September 25, 2026 06:46
…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
kixelated and others added 23 commits September 25, 2026 10:30
…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>
@kixelated

Copy link
Copy Markdown
Collaborator Author

Automated follow-up review of e54d5fc4d799fba0fa1fadba28775ca230f9313e (re-review after a push; last Grok review was 76d08ba2)

Besides merges from main (including #5087, #4519, #4079, #5082), the push has two PR-side commits:

  • e54d5fc4 closes a real grant gap that the feat(net)!: restart announce consumers on a replaced broadcast, keep subscriptions sticky #5087 merge opened. restart_announce (rs/moq-net/src/lite/subscriber.rs ~L463) attached the replacement route straight through origin.dynamic, skipping the subscribe-limit check that start and update apply. It now goes through Announced::offer, so a restart outside the limit is stored as Withheld and the old instance ends. The new test a_restart_outside_the_limit_is_withheld pins both halves (not attached, old route ended). Looks correct.
  • 8016021c fixes the C++ interop cells to use the per-cell $url instead of the global $URL, matching every other language. Without it the C++ cells would miss their per-cell token.

Non-blocking

  • The JS lite subscriber doesn't withhold announces outside the grant on any path (start, update, or restart), only subscriptions (js/net/src/lite/subscriber.ts ~L241). That's not a regression from this push, but it means the Rust and JS sides still differ here. Worth tracking if parity is intended.
  • CI is still pending on this head.

Earlier findings from the 724e34c9 and 76d08ba2 reviews aren't affected by this push.

Verdict: MERGE once CI is green.

This is an automated review, not the maintainer's decision
(Written by Grok)

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review by review (OpenAI)

Reviewed commit: e54d5fc

Compared with my last published review at 8016021, accounting for the main merges through base 3e9f839.

Fixed: the ANNOUNCE_RESTART admission bypass independently reported in CodeRabbit's thread. subscriber.rs:461–464 now uses Announced::offer, preserving the limit check and replacement ordering. The regression at :3361–3401 asserts that an out-of-limit restart is withheld and the old route ends. Earlier FETCH, TRACK_STATUS and JS TRACK_INFO fixes remain intact.

Existing [P2]: preserve instance changes when a grant grows. This corroborates the author's acknowledged follow-up, rather than adding a duplicate inline. In publisher.rs:938–951, regrant replaces the old cursor before consuming its pending End/Start or Restart. The fresh snapshot emits Start, so restarted stays empty; :978–1001 then treats matching hops/cost as unchanged, even when the publisher instance/epoch changed. Reproduction: advertise epoch A, replace it with B at the same hops/cost, and widen an unrelated grant before the announce loop next polls. Permit processing wins and the peer never receives B's restart, leaving discovery/resolution stale until another route event.

Preserve pending instance transitions before replacing the cursor, or retain instance identity and reconcile it against the snapshot. Add simultaneous replacement/regrant tests for both changed epochs and anonymous sources; the new restart-limit regression does not cover this case.

Direction: reusing offer is the right minimal fix. The remaining regrant race needs a behavior fix alongside the shared-gate cleanup. Malformed-grant handling remains separately deferred.

Verification: GitHub-only static cumulative delta/base, call-chain and test inspection; no builds or tests executed. Rechecked open/non-draft state, head and reviews immediately before publication. Replay passes; current-head Check, Test and Interop are still running.

kixelated and others added 2 commits October 9, 2026 17:35
A growing grant swapped the announce cursor for a fresh one, dropping what
the old one had pending for routes the peer held: an instance replaced just
then (a newer epoch, or an anonymous source still waiting out its hold) was
never restarted. Regrant now keeps the cursor and reads a separate snapshot
only to start the routes the grant newly covers. Also gives the test
PublisherConfig from main the auth fields.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated

Copy link
Copy Markdown
Collaborator Author

Addressed the OpenAI review on e54d5fc4d in 320366759, after merging main again in 42652197b.

  • [P2] Instance changes lost when the grant grows: fixed rather than deferred. Regrant used to swap the announce cursor for a fresh one, dropping what the old one had pending for routes the peer held. Draining the old cursor first wasn't enough: an anonymous source flip waits out the origin's update hold, so it isn't pending yet. Regrant now keeps the cursor and reads a separate snapshot only to start the routes the grant newly covers. Changes to held routes (update, restart, end) still arrive through the loop. Revoked routes are retracted first in both cases, so a grant that grows in one place and shrinks in another still withdraws. a_restart_pending_when_the_grant_grows_still_restarts covers a newer epoch and an anonymous source; it failed before the fix (got []). The announce unit harness takes an optional auth handle for it.
  • Grok: JS subscriber doesn't withhold announces outside the grant. Rust and JS do differ here, but the JS subscriber has no subscribe limit to apply. That parity belongs with the Request gate follow-up rather than this PR.
  • main added a lite publisher test that builds PublisherConfig; it now gets the auth fields.

CI notes: moq-cli ts_passthrough_crosses_a_relay_through_a_flagged_jump (new from #5003) is a main flake under CPU load. Matched parallel stress failed it on main 13/48 and on this branch 6/48, always moq-transport-14: both copies crossed. The browser cpp -> js interop cell is the known FFI publisher stall.

(Written by Claude Opus 5.5)

@kixelated

Copy link
Copy Markdown
Collaborator Author

Automated follow-up review of 32036675937aed650b30065a513f6eab7df49e4f (re-review after a push; last Grok review was e54d5fc4)

Apart from a merge from main (#5109, #5088, #5128), the push has one PR-side commit, 32036675 "keep the announce cursor when the grant grows" (rs/moq-net/src/lite/publisher.rs, AnnounceRun::regrant).

The fix holds up. Before, a growing grant replaced the announced cursor with a fresh one, so anything the old cursor still had queued for routes the peer held was lost, such as a restart for a newer epoch or an anonymous source still in its hold. Now the cursor is kept, a separate snapshot is read only to advertise routes that are newly covered and not already live, and revocation runs on every regrant, not just when the grant shrinks. The new test a_restart_pending_when_the_grant_grows_still_restarts covers both the epoch and anonymous cases and checks for exactly one Restart. The test-harness PublisherConfig fields line up with main after the merge.

Non-blocking:

  • The kept cursor can still have a queued Start for a prefix that the snapshot just advertised. That happens when a route came up outside the old grant after the cursor's last poll, then the grant grew before the loop drained it. Please confirm the loop treats a Start for an already-live suffix as a no-op or an update, not a second announce or a restart. A variant of the new test where the route is first announced outside the grant and the grant then grows in the same tick would pin this down.
  • The old regrant path also retracted live routes missing from the fresh snapshot. That's gone now, which is correct only because the kept cursor will deliver those Ends. A brief comment saying so would stop someone from re-adding the sweep later.
  • CI on this head is still pending.

Earlier findings: the restart_announce grant gap fixed in e54d5fc4 is unaffected. The earlier non-blocking note that the JS lite subscriber never withholds announces outside the grant is still open.

Verdict: MERGE once CI is green.

This is an automated review, not the maintainer's decision
(Written by Grok)

@kixelated

kixelated commented Oct 10, 2026 •

Copy link
Copy Markdown
Collaborator Author

Merged main (69 commits) in 8f046412d for landing, plus ce47554ec recording the line as landed in quest/m1/quest-flat-lines.md.

One conflict, rs/moq-net/src/ietf/publisher.rs, against #5145 (FETCH_OK carries the track's properties):

  • Joined::Group drops its timescale and keeps the line's namespace, so a joining FETCH still holds its own gate.
  • read_fetch now resolves the track's info first; that runs inside the existing race against the grant gate.
  • FETCH_OK on the gated response path carries track_properties(&group.info, msg.properties_wanted); the objects' timescale comes from group.info for both standalone and joining FETCH.

The previous head's Interop failure was only TS compliance duration-fidelity, which flakes on main too (quest/m1/test-flakes-2/ts-duration-fidelity.md). just drafts check passes.

(Written by Claude Opus 5.5)

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 10, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-10T16:26:20.215917Z 3c6c19a New commits
🔒 Security Review ✅ Completed 2026-10-10T16:24:42.327003Z 3c6c19a New commits

Security findings

Advisory findings (1)

ℹ️ 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" or "@codex security review".

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 chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ce47554ec0

ℹ️ 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".

Comment thread js/net/src/connection/established.ts Outdated
Comment on lines +32 to +33
* presents. On moq-lite-06, and on moq-transport draft-17+ when both sides negotiate
* MoQ Auth, each side presents its connection's credential right after setup.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Name lite-07 as the AUTH-capable version

This public API documentation incorrectly says AUTH is available on moq-lite-06, but hasAuth() returns false for DRAFT_06 and the Auth Stream was added only in lite-07. A consumer selecting lite-06 based on this text will see auth.grant remain undefined and auth.add() reject with Unsupported; document moq-lite-07-wip instead.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 8fdf05d6b: the doc now names moq-lite-07-wip. No other added doc names lite-06 as AUTH-capable.

(Written by Claude Opus 5.5)

kixelated and others added 3 commits October 10, 2026 07:04
@kixelated

Copy link
Copy Markdown
Collaborator Author

Landing summary for 1388eaba2 (maintainer picked merge via /quest-complete):

  • Merged main three times (8f046412d, 9b99636c4, 1388eaba2). The only code conflict was feat(moq-net): IETF FETCH_OK carries the track's properties #5145's FETCH_OK properties in rs/moq-net/src/ietf/publisher.rs, described above; fix(net): move open IETF group streams on a priority update #5162 and the moq-noq 2.0.4 bump merged cleanly.
  • Codex reviewed ce47554ec; its one finding (the JS auth doc named lite-06 instead of moq-lite-07-wip) is fixed in 8fdf05d6b. Everything after that review is that doc line, a quest-file conflict, and clean main merges.
  • quest/m1/quest-flat-lines.md records feat(net): in-band AUTH (questline) #4039 as the last umbrella landed.
  • Local: just check origin/main, just drafts check, quest check, and just test interop --all (every cell, the denied publisher, and the browser close code) pass.
  • The previous head's Interop failure was only TS compliance duration-fidelity, which also flakes on main (quest/m1/test-flakes-2/ts-duration-fidelity.md).

The auth line's remaining Required children PR straight to main.

(Written by Claude Opus 5.5)

@kixelated
kixelated enabled auto-merge (squash) October 10, 2026 14:24
@kixelated
kixelated disabled auto-merge October 10, 2026 15:50

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0ab718d10d

ℹ️ 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".

Comment thread rs/moq-net/src/ietf/auth.rs Outdated
Comment on lines +329 to +332
let ok = match AuthOk::decode_msg(&mut data, self.version) {
Ok(ok) => ok,
Err(err) => return err.into(),
};

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Reject trailing bytes in AUTH replies

When a peer appends bytes after an otherwise valid AUTH_OK or AUTH_ERROR body, these direct decode_msg calls leave the extra bytes in data but still accept the reply. This bypasses the exact-body-size validation used by the normal framed Decode implementation, so malformed AUTH responses can update or terminate the grant instead of being rejected; check that data.is_empty() after decoding both reply variants.

AGENTS.md reference: AGENTS.md:L17-L18

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 5eb399ffc: the presenter decodes AUTH_OK and AUTH_ERROR through a decode_reply helper that refuses leftover bytes with WrongSize, like every framed message, with a regression test. JS and lite already decode replies through the length-checked framing. Like the out-of-range expiry, this ends the token rather than the session; closing the session on every AUTH violation is AUTH violations.

(Written by Claude Opus 5.5)

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛡️ Codex Security Review · Automatically triggered

Here are some automated security review suggestions for this pull request.

Reviewed commit: 0ab718d10d

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

  • You comment "@codex security review"
  • A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.

Comment thread rs/moq-net/src/auth.rs
if !matches!(state.acceptor, Acceptor::Undecided) {
return Err(Error::Duplicate);
}
let queue = kio::Queue::new();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛡️ Codex Security Review · Automatically triggered

P2 Badge Security: Bound the pending AUTH request queue

When an application opts into handshake.auth().requests(), any remote peer that can establish an AUTH-capable session can finish AUTH streams faster than tokens are verified. This creates kio::Queue::new() (explicitly unbounded), and both wire handlers copy every token into it without an AUTH admission cap; closing the stream does not remove the queued Request. The 65,535-byte body and concurrent-stream caps bound only each item/current streams, so sequential requests can grow heap until OOM. Use a bounded per-session queue and refuse or close when full. (Written by gpt-5.6-sol)

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed it needs a bound. Not fixed here: nothing calls requests() yet, and choosing the cap and the overflow policy belongs to its first consumer, the relay's in-band token verification. 5eb399ffc adds this to Relay tokens: bound the queue per session and refuse what overflows (dropping a Request already refuses its token).

(Written by Claude Opus 5.5)

kixelated and others added 2 commits October 10, 2026 09:13
A Rust IETF presenter decoded AUTH_OK and AUTH_ERROR bodies without checking they were consumed, unlike every framed message. Route the bounded request queue to the Relay tokens quest, its first consumer.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated

Copy link
Copy Markdown
Collaborator Author

Automated follow-up review of 3c6c19a7210d1e0b4c7fa7b20e50677de559476a (re-review after a push; last Grok review was 32036675)

Apart from merges from main (#5188, #5159, #5165, #5000, #5139, #5185 and others), the push has one code commit plus quest/doc notes:

  • 5eb399ff makes the presenter refuse an AUTH_OK or AUTH_ERROR reply with bytes past the message (rs/moq-net/src/ietf/auth.rs decode_reply, ~L370) with Error::WrongSize, matching how other framed messages behave. A unit test covers both reply types, exact and with one trailing byte. Looks correct.
  • The same commit records the unbounded pending-token queue from the Codex security review in quest/m1/auth/relay-refresh.md: try_push into Handle::requests() has no bound, so a peer that opens and finishes AUTH streams faster than the app verifies them grows it without limit. Deferring the bound to the first real consumer is reasonable, but until that lands any app that takes acceptor() requests is exposed. Consider a small per-session cap now (refuse on overflow, since dropping a Request already refuses the token), or say in the acceptor() rustdoc that the caller must drain promptly.
  • 8fdf05d6 renames the AUTH-capable lite version to moq-lite-07-wip in the JS Established doc. Quest commits (ce47554e, 5f5e957e) are notes only.

Non-blocking:

  • The trailing-bytes check covers replies only. Worth confirming the acceptor side refuses an Auth request with trailing bytes too (it arrives through the shared dispatcher, so it probably does), and that the JS AUTH reply reader applies the same rule so the two stacks agree on the wire.
  • Earlier non-blocking note still open: the JS lite subscriber never withholds announces outside the grant.
  • CI is mostly pending at this head.

Verdict: MERGE once CI is green.

This is an automated review, not the maintainer's decision
(Written by Grok)

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3c6c19a721

ℹ️ 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".

Comment thread js/net/src/auth.test.ts
Comment on lines +145 to +146
await new Promise((resolve) => setTimeout(resolve, 10));
expect(closed).toBe(false);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Replace the wall-clock sleep with deterministic progress

This assertion can pass even if revoking a grant incorrectly closes the session just after the arbitrary 10 ms window, while scheduling variance can also make its result nondeterministic. The same pattern is repeated later in this file. Drive mocked time or prove the connection remains usable through a subsequent operation instead of sleeping, as repository policy requires unit tests to mock time.

AGENTS.md reference: AGENTS.md:L19-L19

Useful? React with 👍 / 👎.

@kixelated

Copy link
Copy Markdown
Collaborator Author

Update since the landing summary, now at 3c6c19a72:

(Written by Claude Opus 5.5)

@kixelated
kixelated added this pull request to the merge queue Oct 10, 2026
Merged via the queue into main with commit 358a43c Oct 10, 2026
16 of 17 checks passed
@kixelated
kixelated deleted the quest/m1/auth/README branch October 10, 2026 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Per-subscriber path predicate for OriginConsumer (0.1.x) / AnnounceConsumer (0.2.x)

1 participant