Repository navigation
docs(quest): constrain per-request auth to an experimental Rust hook - #5364
Conversation
Co-Authored-By: Codex <noreply@openai.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. |
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 2286d8c
No actionable bugs found in this plans-only change. The direction is coherent: distinguish session/request acceptance in the types, use locally verified authority for inbound admission, and require bounded, cancellable verification before landing the experimental hook. The deferred relay/JS/bindings scope and retained renewal/joining-FETCH requirements are consistent across the updated quests.
Checked all 11 changed files, the completed prerequisite PRs, and relevant existing decoder/admission code. The implementation hazards in #5148 remain implementation work, not fixes delivered by this PR. Verification was static through GitHub; I did not run quest checks, builds, tests, or interoperability checks. Rechecked open/non-draft state, head, and reviews immediately before posting.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe quest documents now describe request-token verification as an experimental Rust hook with request-specific authorization rules. They distinguish the hook from deferred relay integration and exclude per-request APIs from bindings. The relay plans specify unsupported request verification until implemented, and related quest documents update their scope and references. Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The changes clarify the experimental Rust hook and keep relay verification deferred. No concrete user or operational regression is identified, so the PR appears ready to merge, subject to its reported document checks. Pre-merge checks ·
 |
| Check name | Status | Explanation |
|---|---|---|
| Description check |  |
The description clearly explains that the PR narrows per-request AUTH to an experimental Rust hook, defers relay and binding work, coordinates related quests, and changes plans only. |
| Title check | |
The title concisely and accurately identifies the main change: constraining per-request authentication to an experimental Rust hook. |
| Docstring Coverage | |
No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0… |
| 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. |
✨ 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.
![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: 2286d8ca04
ℹ️ 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".
| - **Wire scope.** Honor tokens on SUBSCRIBE, unsolicited PUBLISH_NAMESPACE, | ||
| and their renewal updates; other request tokens grant no authority. The | ||
| solicited NAMESPACE response has no request-token slot. Preserve the |
There was a problem hiding this comment.
When a draft-14/15 peer answers SUBSCRIBE_NAMESPACE, it must use PUBLISH_NAMESPACE because those drafts have no NAMESPACE message (rs/moq-net/src/ietf/publisher.rs:2543-2546,2656-2660), and the receiver explicitly cannot distinguish that reply from an unsolicited advertisement (rs/moq-net/src/ietf/subscriber.rs:1236-1238). The implementation therefore cannot honor tokens only on “unsolicited PUBLISH_NAMESPACE”: it must either reject valid solicited token authorization or honor it contrary to this stated scope. Define the legacy behavior explicitly, likely by honoring all draft-14/15 PUBLISH_NAMESPACE tokens, and cover the solicited case.
Useful? React with 👍 / 👎.
Problem
The per-request AUTH plan presents a broad feature while #5148 still has admission and API hazards: received peer grants can bypass local verification, initial verification precedes capacity/cancellation handling, and the shared acceptor can silently give session handlers request-enforcement semantics.
Approach
Constrain #5148 to an experimental Rust interoperability hook with application-owned verification, retaining renewal and joining FETCH. Record the required fixes and regressions, coordinate shared AUTH plumbing with #5282, and keep its separate open decisions open. Move relay request leases to m3 and explicitly defer per-request JS, CLI, and bindings. Keep documentation to minimal API comments.
Remove completed decoder (#5107) and setup-extension (#5106) quests and update references. The decoder's old REQUEST_UPDATE finding is already fixed on main.
Impact
Decisions
All selected options were confirmed by the maintainer:
Alternatives
The selected scope keeps the interoperability hook without making relay integration a completion requirement. Explicit acceptor variants prevent existing session handlers from silently opting into request authorization. Local-session-first admission preserves the existing intended policy while fixing the direction of authority.
Follow-ups
Implementation stays in #5148 and shared session plumbing in #5282. Relay request verification is deferred at
quest/m3/auth-request-lease.md. No new guide quest.Validation
quest check: 412 documents pass.nix develop --command just check: passes.git diff --check: passes.(Written by GPT-6)
Need help on this PR? Tag
@codesmith-botwith what you need.