chore(auth): bump jsonwebtoken 9→10 + dedupe harness + ES256 coverage - #15
Merged
Conversation
Three follow-ups in one PR — same area (auth tests + crypto deps), same review surface. 1. **jsonwebtoken 9 → 10** (supersedes Dependabot #13) - v10 splits the crypto backend behind a `CryptoProvider` (same pattern rustls uses). Exactly one of `rust_crypto` / `aws_lc_rs` must be enabled or every signature verify panics. We pick `rust_crypto` — pure Rust, matches the `rustls-tls` feature we use on reqwest, no OpenSSL / aws-lc native deps to ship. - v10's default features include `use_pem` (~20 KB of pem + simple_asn1). Production code uses `from_rsa_components(n, e)` off the JWKS — PEM is only needed by the test harness for signing. Cargo's feature unification per build target lets us re-enable `use_pem` as a dev-dep override, keeping the release binary lean. - Dependabot's #13 only bumped the version string; it didn't fix the broken feature config OR sign-off the commits. This commit does both, so we close #13 in favour of this PR. 2. **Dedupe inline harness in tests/auth.rs** - The mock JWKS server + sign-and-mint helpers lived inline in tests/auth.rs AND in the shared tests/jwks_harness.rs that tests/jwt_middleware.rs uses. tests/auth.rs now does `mod jwks_harness;` like the other consumer — single source of truth for the harness, future API tweaks touch one file. - Net: -170 LOC in tests/auth.rs. 3. **ES256 / elliptic-curve coverage** - `build_cached_key` in src/auth.rs has separate branches for `AlgorithmParameters::RSA` and `AlgorithmParameters::EllipticCurve`, but only the RSA branch had tests. A future refactor could silently break the EC path without anyone noticing until a real Better Auth deploy with ES256 keys failed verify. - New `JwksHarness::spawn_es256()` mirrors `spawn()` against a P-256 keypair. Per-test keygen is ~1 ms (vs ~50 ms for the RSA path) so the EC sweep is essentially free. - 4 new tests on the EC branch: happy path, wrong-key reject, unknown-kid reject, cross-algorithm reject (RS256 token against ES256 JWKS — confused-deputy defense). - New `HarnessAlg` enum + `JwksHarness::header_with_kid(&self)` so a test that switches algorithms doesn't have to thread the algorithm through manually; the harness knows. Deps: - p256 = "0.13" (dev-dep, features = ["ecdsa", "pkcs8"]) for the ES256 keypair generation. - No new runtime deps. Test plan: - cargo check --all-targets ✅ - cargo fmt --all --check ✅ - cargo clippy --all-targets -- -D warnings ✅ - cargo test --test auth (17 passed, 13 RS256 + 4 ES256) ✅ - CI will run the rest against Postgres. Signed-off-by: InstaZDLL <github.105mh@8shield.net>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughMise à jour de jsonwebtoken v9→v10 (rust_crypto backend), ajout de p256 en dev, refactor du harness de tests ( ChangesJWT Upgrade and Multi-Algorithm Test Support
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@tests/auth.rs`:
- Around line 326-357: The test es256_jwk_rejects_rs256_token currently expects
AuthError::KeyNotFound or AlgorithmMismatch but the spawn_es256 harness reuses
TEST_KID so verify_token() resolves the kid and returns
AuthError::AlgorithmMismatch; update the test to assert exclusively for
AuthError::AlgorithmMismatch, update the header comment to reflect that the kid
is shared and the mismatch comes from header.alg vs cached.algorithm, and remove
or adjust any lines referencing KeyNotFound; target the test function
es256_jwk_rejects_rs256_token, the JwksHarness::spawn_es256 setup, and the
verify_token()/AuthError matching to make this change.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 01ad42d0-5724-474e-8946-0d8989289382
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!Cargo.lock,!*.lock
📒 Files selected for processing (3)
Cargo.tomltests/auth.rstests/jwks_harness.rs
CR caught an overly-permissive assertion. The disjunction `AuthError::KeyNotFound | AuthError::AlgorithmMismatch` was hiding which path the verifier actually took — and the `KeyNotFound` branch is unreachable because both harnesses publish their JWK under the same `TEST_KID`. The verifier's `resolve_kid` finds the cached EC key, then the explicit `if header.alg != cached.algorithm` cross-check fires on `RS256 != ES256` and returns `AlgorithmMismatch`. Test now asserts exclusively `AlgorithmMismatch`. The doc-comment is rewritten to call out the shared-kid setup + the alg cross-check path, so the next reader doesn't have to reverse-engineer it. Verified by `cargo test --test auth es256_jwk_rejects_rs256_token` — still passes, confirming the empirical path. Signed-off-by: InstaZDLL <github.105mh@8shield.net>
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
Three follow-ups bundled — same area (auth tests + crypto deps), so single review surface.
Supersedes Dependabot #13 which only bumped the version string without fixing the v10 breaking changes (CryptoProvider feature gate) or DCO sign-off. I'll close #13 when this lands.
1. jsonwebtoken 9 → 10
CryptoProviderindirection (rustls pattern). Exactly one ofrust_crypto/aws_lc_rsmust be enabled or every signature verify panics at runtime.rust_crypto— pure Rust, matchesrustls-tlson reqwest, no native deps.use_pem(~20 KB pem + asn.1 parser). Production code reads keys viafrom_rsa_components(n, e)off the JWKS — PEM is only needed by the test harness for signing. Cargo feature unification per build target lets us re-enableuse_pemas a dev-dep override, keeping the release binary lean.2. Dedupe inline harness in
tests/auth.rstests/auth.rsAND intests/jwks_harness.rs(used bytests/jwt_middleware.rs).tests/auth.rsnow doesmod jwks_harness;like the other consumer. Single source of truth.3. ES256 / elliptic-curve coverage
build_cached_keyinsrc/auth.rshas separate branches for RSA and EllipticCurve, but only RSA was tested. A future refactor could silently break the EC path until a real Better Auth deploy with ES256 keys failed.JwksHarness::spawn_es256()mirrorsspawn()against a P-256 keypair. Per-test keygen ~1 ms (vs ~50 ms RSA), so the sweep is essentially free.HarnessAlgenum +JwksHarness::header_with_kid(&self)so a test that switches algos doesn't thread the algorithm manually.Deps
jsonwebtokendefault-features=false+rust_cryptojsonwebtokenuse_pemfor the test harnessp256 = "0.13"(features: ecdsa, pkcs8)No new runtime deps.
Test plan
cargo check --all-targetscargo fmt --all --checkcargo clippy --all-targets -- -D warningscargo test --test auth(17 passed — 13 RS256 + 4 ES256)Summary by CodeRabbit
Notes de version
New Features
Chores
Tests