feat: key-based public access for anonymous subscribe/publish#1180
Merged
Conversation
The `files: ["./src"]` field was rewritten to `files: ["."]` by the build script, which caused npm to not include the compiled JS/types in the published tarball. Other packages don't use `files` and work correctly. Bump to 0.1.2. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…lish JWK keys can now encode `public_sub` and `public_pub` optional path prefixes that grant anonymous access without a token. This allows fine-grained control like requiring tokens for publishing to /demo but allowing anonymous subscribing. - Rename anon_sub/anon_pub to public_sub/public_pub (Option<String>) - Add --public, --public-subscribe, --public-publish CLI flags to key generation - Relay uses key public fields for anonymous access, deprecates --auth-public - Rename --public to --public-key for public key file output Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis pull request adds support for anonymous/guest access paths across the token generation and authentication system. CLI tools in JavaScript and Rust gain three variadic options to specify guest subscribe/publish path prefixes. Key schemas and public key output are extended to include 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rs/moq-relay/src/auth.rs (1)
268-291:⚠️ Potential issue | 🟡 MinorPublic path aggregation is not updated on JWKS refresh.
The
public_subandpublic_pubfields are computed once at startup innew_remote(), but therefresh_taskonly updates theKeySetmutex—it doesn't recompute the aggregated public paths.If a key rotation changes
public_sub/public_pubvalues in the JWKS, the relay will continue using stale values until restart.Consider either:
- Re-aggregating after each refresh (would require making
public_sub/public_pubArc<Mutex<...>>)- Computing on-demand in
verify()instead of caching- Documenting this as a known limitation
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rs/moq-relay/src/auth.rs` around lines 268 - 291, The startup code in new_remote() computes public_sub and public_pub once and never updates them when the refresh_task refreshes the KeySet, causing stale aggregated paths; update the implementation so these aggregates are recomputed after each JWKS refresh (or computed on-demand in verify())—specifically modify the refresh_task/refresh logic inside refresh_task to, after updating the KeySet mutex, also recompute Self::aggregate_public_paths(&ks) and store the results into thread-safe/writable fields (e.g., make public_sub and public_pub Arc<Mutex<...>> or Atomic/Arc-wrapped values) so verify() reads the current aggregated values; alternatively, move aggregation logic into verify() to compute from the KeySet lock at call time if you prefer on-demand semantics.
🧹 Nitpick comments (2)
js/token/package.json (1)
1-31: Removingfilesfield will publish test files and examples to npm.The
fileswhitelist was removed, so npm will now include*.test.tsfiles, theexamples/directory, andtsconfig.jsonfiles in the published package. This increases package size unnecessarily and may confuse consumers.Consider restoring a
filesfield that includes the necessary artifacts:"files": [ "dist", "src", "!src/**/*.test.ts" ]Or use
.npmignoreto exclude test files and examples.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/token/package.json` around lines 1 - 31, The package.json currently lacks a "files" whitelist which will cause test files and examples to be published; restore a "files" field in package.json (the root object shown) to include only the necessary artifacts (for example "dist" and "src") and exclude test files and examples (e.g., exclude "src/**/*.test.ts" and "examples/"), or alternatively add an .npmignore that excludes "*.test.ts", "examples/", and tsconfig.json to prevent publishing unwanted files.js/token/src/cli.ts (1)
22-31: Consider using Commander's.conflicts()method for cleaner option validation.The manual validation at lines 28-31 works, but Commander v14 supports
.conflicts()which provides more idiomatic error handling and aligns with the Rust CLI's approach. Instead of the current if-check, use.addOption()withnew Option()to declare conflicts:.addOption(new Option("--public-subscribe <path>", "Path prefix for unauthenticated subscribe access").conflicts("public")) .addOption(new Option("--public-publish <path>", "Path prefix for unauthenticated publish access").conflicts("public"))This lets Commander handle the error messaging and validation automatically.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/token/src/cli.ts` around lines 22 - 31, Replace the manual conflict check inside the .action handler by declaring conflicts at option definition time: stop using .option(...) for "--public-subscribe" and "--public-publish" and instead add them with .addOption(new Option(...).conflicts("public")) so Commander enforces that "--public" cannot be used with those flags; remove the if (options.public && (options.publicSubscribe || options.publicPublish)) { ... } block in the .action and keep existing symbols .option("--public <path>...", .action(...) and the option names publicSubscribe/publicPublish/public to locate and update the definitions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@rs/moq-relay/src/auth.rs`:
- Around line 268-291: The startup code in new_remote() computes public_sub and
public_pub once and never updates them when the refresh_task refreshes the
KeySet, causing stale aggregated paths; update the implementation so these
aggregates are recomputed after each JWKS refresh (or computed on-demand in
verify())—specifically modify the refresh_task/refresh logic inside refresh_task
to, after updating the KeySet mutex, also recompute
Self::aggregate_public_paths(&ks) and store the results into
thread-safe/writable fields (e.g., make public_sub and public_pub
Arc<Mutex<...>> or Atomic/Arc-wrapped values) so verify() reads the current
aggregated values; alternatively, move aggregation logic into verify() to
compute from the KeySet lock at call time if you prefer on-demand semantics.
---
Nitpick comments:
In `@js/token/package.json`:
- Around line 1-31: The package.json currently lacks a "files" whitelist which
will cause test files and examples to be published; restore a "files" field in
package.json (the root object shown) to include only the necessary artifacts
(for example "dist" and "src") and exclude test files and examples (e.g.,
exclude "src/**/*.test.ts" and "examples/"), or alternatively add an .npmignore
that excludes "*.test.ts", "examples/", and tsconfig.json to prevent publishing
unwanted files.
In `@js/token/src/cli.ts`:
- Around line 22-31: Replace the manual conflict check inside the .action
handler by declaring conflicts at option definition time: stop using
.option(...) for "--public-subscribe" and "--public-publish" and instead add
them with .addOption(new Option(...).conflicts("public")) so Commander enforces
that "--public" cannot be used with those flags; remove the if (options.public
&& (options.publicSubscribe || options.publicPublish)) { ... } block in the
.action and keep existing symbols .option("--public <path>...", .action(...) and
the option names publicSubscribe/publicPublish/public to locate and update the
definitions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7f200f11-4f4e-4ab6-b06c-2c85db086d30
📒 Files selected for processing (8)
js/token/package.jsonjs/token/src/cli.tsjs/token/src/generate.tsjs/token/src/key.tsrs/moq-relay/src/auth.rsrs/moq-token-cli/src/main.rsrs/moq-token/src/generate.rsrs/moq-token/src/key.rs
…nflicts - Rename public_sub/public_pub back to anon_sub/anon_pub to avoid confusion with public keys - Compute anonymous access paths on-demand in verify() from the current KeySet, so JWKS refreshes are reflected immediately - Use Commander .conflicts() for --public vs --public-subscribe/publish instead of manual validation Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…key output - Rename anon_sub/anon_pub to guest_sub/guest_pub to clearly convey "unauthenticated user" without clashing with "public key" - CLI flags: --guest, --guest-subscribe, --guest-publish for key generation - Restore --public to mean public key file output (was renamed to --public-key) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rs/moq-relay/src/auth.rs (1)
203-209:⚠️ Potential issue | 🟡 MinorMove the Rustdoc back onto
Auth::new().After inserting
aggregate_public_paths()here, the “Creates a new authenticator...” docs now attach to the private helper instead of the public constructor, so the generated docs are misleading andnew()is left undocumented.As per coding guidelines, "Document public APIs with clear docstrings or comments".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rs/moq-relay/src/auth.rs` around lines 203 - 209, The public constructor Auth::new() lost its docstring because the Rustdoc block was moved above the private helper aggregate_public_paths(); move the existing doc comment back onto Auth::new() so the public API is documented, and optionally add a brief internal comment above aggregate_public_paths() describing its purpose; ensure the docstring text about fetching JWK sets, background refresh, and public path aggregation is attached to Auth::new(), while aggregate_public_paths() remains documented only with an internal (non-Rustdoc) comment if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rs/moq-relay/src/auth.rs`:
- Around line 209-228: aggregate_public_paths currently collapses all
anon_sub/anon_pub values to a single prefix; instead collect all anon_sub and
anon_pub strings from KeySet.keys into deduplicated sets, then prune any entry
that is "covered" (i.e., has a shorter prefix present) so only the minimal
covering prefixes remain, join the remaining entries into a single string
(return Some(joined) if non-empty) for anon_sub and anon_pub respectively, and
apply the same change to the other anonymous-aggregation block that mirrors this
logic; reference aggregate_public_paths, KeySet, and the anon_sub/anon_pub
fields when locating and updating the code.
---
Outside diff comments:
In `@rs/moq-relay/src/auth.rs`:
- Around line 203-209: The public constructor Auth::new() lost its docstring
because the Rustdoc block was moved above the private helper
aggregate_public_paths(); move the existing doc comment back onto Auth::new() so
the public API is documented, and optionally add a brief internal comment above
aggregate_public_paths() describing its purpose; ensure the docstring text about
fetching JWK sets, background refresh, and public path aggregation is attached
to Auth::new(), while aggregate_public_paths() remains documented only with an
internal (non-Rustdoc) comment if needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7a3e873a-6fcf-4ba0-be1a-2128117bee1f
📒 Files selected for processing (7)
js/token/src/cli.tsjs/token/src/generate.tsjs/token/src/key.tsrs/moq-relay/src/auth.rsrs/moq-token-cli/src/main.rsrs/moq-token/src/generate.rsrs/moq-token/src/key.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- rs/moq-token/src/generate.rs
- js/token/src/key.ts
- rs/moq-token/src/key.rs
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
rs/moq-token/src/key.rs (1)
1204-1209: Consider renamingguest_publicin RSA tests to avoid terminology overlap.In these test scopes, the variable refers to RSA public key material, not guest access config. A name like
rsa_publicreads clearer next toguest_sub/guest_pubconcepts.♻️ Suggested readability tweak
- KeyType::RSA { - public: ref guest_public, + KeyType::RSA { + public: ref rsa_public, private: ref public_private, } => { assert!(public_private.is_none()); - assert_eq!(public.n, guest_public.n); - assert_eq!(public.e, guest_public.e); + assert_eq!(public.n, rsa_public.n); + assert_eq!(public.e, rsa_public.e); }Also applies to: 1242-1247
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rs/moq-token/src/key.rs` around lines 1204 - 1209, Rename the test variable guest_public to a clearer name like rsa_public wherever it appears in the RSA test match arms (e.g., the pattern binding currently written as "public: ref guest_public" and its related assertions comparing public.n/public.e) to avoid confusion with guest_* concepts; update all matching occurrences (including the other instance around the 1242-1247 block) and any subsequent assertions or uses (e.g., assert_eq!(public.n, guest_public.n); assert_eq!(public.e, guest_public.e); and comparisons alongside guest_sub/guest_pub) so the new identifier is used consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@rs/moq-token/src/key.rs`:
- Around line 1204-1209: Rename the test variable guest_public to a clearer name
like rsa_public wherever it appears in the RSA test match arms (e.g., the
pattern binding currently written as "public: ref guest_public" and its related
assertions comparing public.n/public.e) to avoid confusion with guest_*
concepts; update all matching occurrences (including the other instance around
the 1242-1247 block) and any subsequent assertions or uses (e.g.,
assert_eq!(public.n, guest_public.n); assert_eq!(public.e, guest_public.e); and
comparisons alongside guest_sub/guest_pub) so the new identifier is used
consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 64327ffc-21ad-4663-bd22-dd9b78b8bedb
📒 Files selected for processing (7)
js/token/src/cli.tsjs/token/src/generate.tsjs/token/src/key.tsrs/moq-relay/src/auth.rsrs/moq-token-cli/src/main.rsrs/moq-token/src/generate.rsrs/moq-token/src/key.rs
✅ Files skipped from review due to trivial changes (1)
- rs/moq-relay/src/auth.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- js/token/src/key.ts
- js/token/src/generate.ts
- rs/moq-token/src/generate.rs
- Change guest_sub/guest_pub JWK fields from single string to string-or-array (backwards compatible: accepts both "foo" and ["foo", "bar"]) - Add `guest` field as shorthand for both guest_sub and guest_pub - CLI flags --guest, --guest-subscribe, --guest-publish now accept multiple values - aggregate_public_paths deduplicates and removes subset paths across keys - Claims now uses serde_with::OneOrMany instead of custom string_or_vec - Rename dev key from root.jwk to dev.jwk with --guest anon --guest-sub demo Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rs/moq-relay/src/auth.rs`:
- Around line 232-237: The current warnings in auth.rs trigger whenever more
than one minimal guest prefix survives dedup (guest_sub and guest_pub) even
though multiple prefixes are a valid configuration; update the logic to stop
emitting a warning for that normal case by removing or downgrading the
tracing::warn! calls for guest_sub and guest_pub (the two if blocks checking
guest_sub.len() > 1 and guest_pub.len() > 1), or replace them with a
trace/debug-level message if you want visibility; ensure the remaining behavior
still returns/uses the deduplicated prefixes as before.
- Around line 341-355: aggregate_public_paths can return guest prefixes like
"demo" but building moq_token::Claims currently emits those as-is, causing
deeper paths (e.g. /demo/room/123) to lose access because reduction drops the
prefix instead of treating it as root-level allowance; when guest_sub or
guest_pub is non-empty, ensure you also include the empty-root permission so
subpaths below the configured prefix are allowed: modify the branch that
constructs moq_token::Claims (the code using self.key lock, calling
Self::aggregate_public_paths, and creating subscribe/publish) to insert "" into
subscribe and publish (or otherwise collapse any guest prefix into an
empty-string entry) before creating moq_token::Claims so anonymous key-based
access works for descendant paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 15e5d6fa-3d4d-4342-abdd-30ee0986d58b
📒 Files selected for processing (12)
dev/relay/justfiledev/relay/leaf0.tomldev/relay/leaf1.tomldev/relay/root.tomljs/token/src/cli.tsjs/token/src/generate.tsjs/token/src/key.tsrs/moq-relay/src/auth.rsrs/moq-token-cli/src/main.rsrs/moq-token/src/claims.rsrs/moq-token/src/generate.rsrs/moq-token/src/key.rs
✅ Files skipped from review due to trivial changes (3)
- dev/relay/leaf1.toml
- dev/relay/leaf0.toml
- dev/relay/root.toml
🚧 Files skipped from review as they are similar to previous changes (5)
- rs/moq-token/src/generate.rs
- js/token/src/generate.ts
- js/token/src/key.ts
- js/token/src/cli.ts
- rs/moq-token/src/key.rs
- Fix reduction logic to grant access when connecting under a guest prefix (e.g., /demo/room/123 with guest_sub=["demo"] now works correctly) - Reject connections that end up with no subscribe or publish permissions - Downgrade multi-prefix guest path warnings to debug level Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace the hand-rolled string_or_vec deserializer with serde_with's OneOrMany<_, PreferMany>, matching the pattern already used in claims.rs. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This was referenced Apr 1, 2026
Merged
Merged
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
public_subandpublic_puboptional path prefix fields that grant anonymous access without a token--public,--public-subscribe,--public-publishCLI flags to key generation (both Rust and TypeScript)--auth-publicis deprecated with a warning--public(public key file output) to--public-keyto avoid conflictTest plan
--public-subscribe demo, connect without token, verify subscribe works under/demo🤖 Generated with Claude Code