Add a token interop smoke test for the published moq-token tooling#4
Merged
Conversation
The media matrix runs the relay anonymously, so the entire JWT auth path shipped by moq-token went untested: a token minted by one published implementation has to verify under the implementation a relay was keyed with, and nothing proved that held for the real artifacts. token.sh is a second, independent smoke test that installs each published flavour and cross-verifies them: - rust : the moq-token-cli binary (cargo / brew / apt / nix), on PATH - js-node : @moq/token's moq-token CLI under node - js-bun : the same published npm package under bun For every (generator x verifier x algorithm) cell the generator mints a key and signs a token and the verifier checks it, across symmetric (HS256) and asymmetric (EdDSA/ES256/RS256) keys, and across the two key encodings (the Rust CLI writes base64url-JSON, @moq/token writes plain JSON). A negative pass confirms each verifier rejects a tampered token and the wrong key, so a green cell means "accepts the valid one AND refuses the bad ones". A broken published package fails only its own cells, matching smoke.sh's mark_broken semantics. Wires it into CI (moq-token-cli installed on every channel + a Token interop step), the freshness policy (@moq/token requested at latest), the justfile (just token / token-full), and the lint check. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 54 minutes and 33 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. WalkthroughThis PR introduces cross-implementation JWT token interop testing to the moq smoke test suite. A new 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ 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 |
The harness (smoke.sh / token.sh / clients) is what changes in this repo, so run the matrix on every PR to catch a broken cell before it merges. The nightly + workflow_dispatch triggers stay; per-ref concurrency with cancel-in-progress keeps PR pushes from piling up. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
README.md (1)
109-109: 💤 Low valueConsider using official "GitHub" capitalization in the URL context.
While this is in a URL path where casing is functional, the official brand name is "GitHub" with capital "H". For consistency with other references in the document (e.g., line 19 correctly uses "GitHub"), consider this minor style improvement.
✨ Proposed capitalization fix
-| `rust` | the `moq-token-cli` binary (crates.io / Homebrew tap / apt repo / the moq flake) | `cargo install moq-token-cli`, `brew install moq-dev/tap/moq-token-cli`, `apt install`, `nix run github:moq-dev/moq#moq-token-cli` | +| `rust` | the `moq-token-cli` binary (crates.io / Homebrew tap / apt repo / the moq flake) | `cargo install moq-token-cli`, `brew install moq-dev/tap/moq-token-cli`, `apt install`, `nix run GitHub:moq-dev/moq#moq-token-cli` |🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` at line 109, Update the README table row containing the `rust` entry so the URL-like token `github:moq-dev/moq#moq-token-cli` uses the official "GitHub" capitalization; i.e. replace the lowercase `github:` portion with `GitHub:` (or otherwise capitalize "GitHub") in the string shown in the table so it matches other references like the one on line 19.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@token.sh`:
- Around line 28-30: The ALGORITHMS default currently set in the token.sh script
(ALGORITHMS="${TOKEN_ALGORITHMS:-HS256,EdDSA}") omits ES256 and RS256; update
the default to include those asymmetric algorithms (for example
ALGORITHMS="${TOKEN_ALGORITHMS:-HS256,EdDSA,ES256,RS256}") or ensure the
full-matrix callers override TOKEN_ALGORITHMS when invoking token.sh (modify the
justfile and CI workflow invocations to pass --algorithms or set
TOKEN_ALGORITHMS) so ES256 and RS256 are exercised.
- Around line 296-320: The negative-pass assumes the canonical generator
produced a token for every algorithm, but canon only means "not marked broken"
and the token file "$TMP/$canon-$algo/token.jwt" may be missing which causes the
script to exit under set -e; update the loop over ALGO_LIST to check that the
token file exists and is non-empty before attempting to create tampered.jwt
(i.e., test the token path referenced by token="$keydir/token.jwt" and if
missing or unreadable log a warning and continue), so the negative pass skips
absent positive-pass artifacts instead of aborting the whole matrix.
---
Nitpick comments:
In `@README.md`:
- Line 109: Update the README table row containing the `rust` entry so the
URL-like token `github:moq-dev/moq#moq-token-cli` uses the official "GitHub"
capitalization; i.e. replace the lowercase `github:` portion with `GitHub:` (or
otherwise capitalize "GitHub") in the string shown in the table so it matches
other references like the one on line 19.
🪄 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: e496b44d-7c65-4549-9974-4dc619f9e7be
📒 Files selected for processing (8)
.github/workflows/smoke.yml.gitignoreREADME.mdclients/token/js/package.jsonclients/token/js/resolve-bin.mjsfreshness.shjustfiletoken.sh
…-cli The apt/rpm packages install the CLI as /usr/bin/moq (its real binary name), but smoke.sh hard-coded moq-cli (the name cargo derives from the crate), so require_tools aborted with "missing required tools: moq-cli" on the apt channel before any cell ran -- red on every recent nightly. Resolve the binary from PATH (prefer the packaged moq, fall back to the cargo-installed moq-cli) unless MOQ_BIN pins it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Three robustness fixes, two from PR review: - Probe moq-token-cli at startup (run `generate` once) instead of only checking it's on PATH. The published Homebrew bottle of moq-token-cli 0.5.28 aborts on launch -- it baked in a /nix/store libiconv rpath that isn't present on a user's Mac (dyld: Library not loaded) -- so every invocation crashed mid-matrix. Now a binary that won't run marks the rust row unavailable (mark_broken) and the JS cells still report. - Default ALGORITHMS to HS256,EdDSA,ES256,RS256 (was HS256,EdDSA). The full-matrix callers (just token-full, CI) don't pass --algorithms, so ES256/RS256 were never exercised despite being supported. - Mint the negative pass's canonical key+token in its own dir with error handling, instead of reusing positive-pass artifacts. `canon` only means "not marked broken"; if its positive gen/sign failed, the token file was missing and the negative pass aborted the whole script under set -e. README notes the broken Homebrew bottle as a known red, like the other published-package failures. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
kixelated
force-pushed
the
token-interop-smoke
branch
from
e892c64 to
237787a
Compare
Docker Hub is a first-class published channel for the moq binaries (moqdev/moq-relay, moqdev/moq-cli, moqdev/moq-token-cli) but nothing exercised the images. Add a `rust-docker` token cell: it `docker pull`s moqdev/moq-token-cli:latest fresh each run and drives the CLI in a throwaway container (scratch dir bind-mounted) through the same generate/sign/verify adapter as the other impls. The image is built FROM nixos/nix and ships the nix store, so it's a genuinely different artifact from the cargo/brew/apt binaries -- and it carries the libiconv the broken Homebrew bottle leaks, so it runs cleanly. - cli_for() now returns the command prefix for every impl, collapsing the rust/rust-docker paths (identical flags) onto one branch. - A startup probe pulls + runs the image once; a missing daemon / failed pull marks rust-docker unavailable instead of failing mid-matrix. - Container runtime is configurable: TOKEN_DOCKER=podman works too (verified locally: full rust/js-node/js-bun/rust-docker matrix, 96/96). - CI runs the cell on the Linux runners only (macOS runners have no Docker daemon); freshness asserts the image stays unpinned (:latest) + pulled fresh; `just token-full` includes it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Around line 166-167: Update the README wording for the "token-full" matrix to
explicitly include the rust-docker cell (Docker image) alongside the rust binary
and the two JS runtimes: state that "token-full" runs rust (binary +
rust-docker), js-node, and js-bun; also mention that token.sh exercises the
binary and marks the rust cell unavailable if the binary fails to launch so
readers understand Docker is part of the full matrix and not excluded. Reference
the existing tokens: token-full, rust-docker, token.sh, rust, js-node, and
js-bun when making the edit.
🪄 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: 10a1b326-99dd-4f50-b7be-f28eea7f8c77
📒 Files selected for processing (6)
.github/workflows/smoke.ymlREADME.mdfreshness.shjustfilesmoke.shtoken.sh
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/workflows/smoke.yml
- justfile
- token.sh
The 'Running locally' block listed just token-full as rust/js-node/js-bun only; spell out that it also runs the rust-docker cell (the moqdev/moq-token-cli image) where a container runtime is available. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
Why
The media matrix runs the relay anonymously (smoke.toml sets
public = ""), so the entire JWT auth path shipped bymoq-tokenwent untested.moq-relayis keyed with a JWK and verifies the tokens publishers/subscribers present, so a token minted by one published implementation has to verify under the implementation a relay was keyed with — and nothing proved that held for the real artifacts.moq-token-clieven rides the same channels asmoq-relay/moq-cli(and is a dependency of the relay.deb), with zero coverage.What
token.sh— a second, independent smoke test that installs each published flavour and cross-verifies them:rustmoq-token-clibinaryPATHorTOKEN_BIN)js-node@moq/token'smoq-tokenCLI, under nodenpm i @moq/tokenjs-bunnpm i @moq/tokenFor every (generator × verifier × algorithm) cell the generator mints a key and signs a token and the verifier checks it — across symmetric (
HS256) and asymmetric (EdDSA/ES256/RS256) keys, and across the two key encodings (the Rust CLI writes base64url-JSON;@moq/tokenwrites plain JSON, and each side loads the other's). A negative pass confirms each verifier rejects a tampered token and the wrong key, so a green cell means "accepts the valid one and refuses the bad ones", not "accepts everything".Like
smoke.sh, the Rust binary is taken fromPATHso the install channel is what's under test, and a broken published package fails only its own cells (mark_brokensemantics) rather than aborting the run.This complements moq's in-tree token unit tests: those run against workspace source with hardcoded fixtures; this runs the real published CLIs, live on both sides, so a packaging break (a missing bin in the
.deb, a stale formula, an export that didn't survivetsc) shows up as a red cell.Wiring
moq-token-cliinstalled on every channel (cargo/apt/brew/nix) + a "Token interop" step.@moq/tokenis requested atlatest;node_modules/bun.lockgitignored.just token(default: rust roundtrip + negatives) /just token-full(full matrix);token.shadded tojust check.Validation (run locally)
HS256+EdDSA: 30/30 PASS, exit 0. Also verifiedES256+RS256cross-impl.TOKEN_BIN=/nonexistent): only rust cells fail,js-bun→js-bunstays green, exit 1.shellcheck/shfmt/actionlint/freshness.shclean for the new + changed files.Scope
Rust + the two JS runtimes is the whole matrix because only Rust and JS ship token tooling today — Python/Go/Swift/Kotlin/C expose no token generate/verify surface yet. A natural follow-up is a relay-as-verifier check (key the relay with a JWK and assert a minted token authorizes a real connection while an anonymous/bad one is refused).
🤖 Generated with Claude Code