test: Wave 24 - comprehensive tonic and JWT integration tests#43
test: Wave 24 - comprehensive tonic and JWT integration tests#43EffortlessSteven merged 1 commit intomainfrom
Conversation
Adds integration test suites for tonic TLS configuration and extended JWT token round-trip scenarios. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Review Summary by QodoAdd comprehensive JWT and tonic adapter integration tests
WalkthroughsDescription• Adds 37 JWT adapter integration tests covering RSA, ECDSA, Ed25519, and HMAC algorithms • Adds 44 tonic adapter integration tests for TLS/mTLS configuration and certificate chains • Tests token round-trips with complex claims, algorithm mismatches, and cross-family rejection • Tests deterministic key generation, various RSA key sizes, and negative scenarios Diagramflowchart LR
JWT["JWT Adapter Tests<br/>37 tests"]
RSA["RSA Algorithms<br/>RS256-512, PS256-512"]
ECDSA["ECDSA Algorithms<br/>ES256, ES384"]
ED["Ed25519<br/>EdDSA"]
HMAC["HMAC Algorithms<br/>HS256-512"]
CROSS["Cross-Family<br/>Rejection Tests"]
STRUCT["Token Structure<br/>Validation"]
TONIC["Tonic Adapter Tests<br/>44 tests"]
SERVER["Server TLS<br/>Config"]
CLIENT["Client TLS<br/>Config"]
MTLS["mTLS<br/>Config"]
CHAIN["Certificate Chain<br/>Handling"]
RSA_SIZES["RSA Key Sizes<br/>2048-4096 bits"]
DET["Determinism<br/>Tests"]
NEG["Negative<br/>Cases"]
JWT --> RSA
JWT --> ECDSA
JWT --> ED
JWT --> HMAC
JWT --> CROSS
JWT --> STRUCT
TONIC --> SERVER
TONIC --> CLIENT
TONIC --> MTLS
TONIC --> CHAIN
TONIC --> RSA_SIZES
TONIC --> DET
TONIC --> NEG
File Changes1. crates/uselesskey-jsonwebtoken/tests/jwt_extended.rs
|
Code Review by Qodo
1. assert_eq! on private PEM
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| assert_eq!(chain_a.chain_pem(), chain_b.chain_pem()); | ||
| assert_eq!(chain_a.root_cert_pem(), chain_b.root_cert_pem()); | ||
| assert_eq!( | ||
| chain_a.leaf_private_key_pkcs8_pem(), | ||
| chain_b.leaf_private_key_pkcs8_pem() | ||
| ); |
There was a problem hiding this comment.
1. assert_eq! on private pem 📘 Rule violation ⛨ Security
The new tonic integration tests compare private-key PEM strings using assert_eq!/assert_ne!, which will print the full key material to test output on failure. This risks leaking secret-shaped material via CI logs/debug output.
Agent Prompt
## Issue description
The tests use `assert_eq!` / `assert_ne!` to compare private key PEM strings (e.g., `leaf_private_key_pkcs8_pem()`, `private_key_pkcs8_pem()`). If these assertions fail, Rust will print the full PEM values in test output, leaking key material into CI logs.
## Issue Context
Compliance requires that debug/log/test output never exposes private keys, raw key bytes, or similar secret-shaped blobs.
## Fix Focus Areas
- crates/uselesskey-tonic/tests/tonic_comprehensive.rs[364-369]
- crates/uselesskey-tonic/tests/tonic_comprehensive.rs[382-385]
- crates/uselesskey-tonic/tests/tonic_comprehensive.rs[443-447]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
Pull request overview
Adds a large set of integration tests to exercise the TLS (tonic) and JWT adapters across supported configurations and algorithm families.
Changes:
- Added comprehensive
uselesskey-tonicintegration tests for server/client TLS, mTLS, chain handling, RSA key sizes, determinism, and negative scenarios. - Added extended
uselesskey-jsonwebtokenintegration tests covering RSA/ECDSA/Ed25519/HMAC signing + verification, token structure checks, and cross-family rejection cases.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| crates/uselesskey-tonic/tests/tonic_comprehensive.rs | New integration test suite covering TLS/mTLS config creation, X.509 chain material, determinism, and negative scenarios. |
| crates/uselesskey-jsonwebtoken/tests/jwt_extended.rs | New integration test suite covering JWT round-trips and rejection behavior across RSA/ECDSA/Ed25519/HMAC algorithms. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #[test] | ||
| fn self_signed_corrupt_cert_pem_still_builds_tonic_identity() { | ||
| let fx = fx(); | ||
| let cert = fx.x509_self_signed("neg-corrupt", X509Spec::self_signed("localhost")); | ||
|
|
||
| // A valid cert should still produce an identity | ||
| let _identity = cert.identity_tonic(); | ||
|
|
||
| // Corrupt PEM is available but tonic Identity::from_pem is infallible | ||
| // (it defers validation to the TLS handshake). Verify the corruption API exists. | ||
| let _corrupt = cert.corrupt_cert_pem(uselesskey_core::negative::CorruptPem::BadBase64); | ||
| assert_ne!( | ||
| cert.cert_pem(), | ||
| &_corrupt, | ||
| "Corrupt PEM should differ from original" | ||
| ); | ||
| } |
There was a problem hiding this comment.
This test name claims a corrupt cert PEM still builds a tonic identity, but the assertions only verify that a corruption API exists and produces a different PEM (it never attempts to construct an identity from the corrupted PEM). Either rename the test to reflect what it validates, or actually build a tonic identity from the corrupted PEM and assert construction succeeds.
| #[test] | ||
| fn deterministic_chain_produces_same_identity() { | ||
| let seed = Seed::from_env_value("tonic-det-test-v1").expect("seed"); | ||
| let fx = Factory::deterministic(seed); | ||
|
|
||
| let chain_a = fx.x509_chain("det-id", ChainSpec::new("det.example.com")); | ||
| fx.clear_cache(); | ||
| let chain_b = fx.x509_chain("det-id", ChainSpec::new("det.example.com")); | ||
|
|
||
| assert_eq!(chain_a.chain_pem(), chain_b.chain_pem()); | ||
| assert_eq!(chain_a.root_cert_pem(), chain_b.root_cert_pem()); | ||
| assert_eq!( | ||
| chain_a.leaf_private_key_pkcs8_pem(), | ||
| chain_b.leaf_private_key_pkcs8_pem() | ||
| ); | ||
| } |
There was a problem hiding this comment.
The test name says it produces the same identity, but it compares PEM/key material (and does not compare/build an identity object). Consider renaming to something like deterministic_chain_produces_same_cert_material (or similar) to match the assertions.
| #[test] | ||
| fn ecdsa_deterministic_keys_produce_same_tokens() { | ||
| let seed = Seed::from_env_value("jwt-ext-det-ec").unwrap(); | ||
| let fx1 = Factory::deterministic(seed); | ||
| let fx2 = Factory::deterministic(seed); | ||
|
|
||
| let kp1 = fx1.ecdsa("jwt-det-ec", EcdsaSpec::es256()); | ||
| let kp2 = fx2.ecdsa("jwt-det-ec", EcdsaSpec::es256()); | ||
|
|
||
| // ECDSA signatures are non-deterministic (random k), so just verify cross-decode | ||
| let claims = Claims::standard(); | ||
| let header = Header::new(Algorithm::ES256); | ||
| let token = encode(&header, &claims, &kp1.encoding_key()).unwrap(); | ||
|
|
||
| let mut validation = Validation::new(Algorithm::ES256); | ||
| validation.set_audience(&["test-api"]); | ||
| let decoded = decode::<Claims>(&token, &kp2.decoding_key(), &validation).unwrap(); | ||
|
|
||
| assert_eq!(decoded.claims, claims); | ||
| } |
There was a problem hiding this comment.
The test name says it produces the same tokens, but it does not compare token strings—only that a token signed with kp1 can be verified with kp2. Either rename to reflect cross-verification (e.g., ecdsa_deterministic_keys_cross_decode), or add a token equality assertion if identical tokens are actually expected.
| let kp1 = fx1.ecdsa("jwt-det-ec", EcdsaSpec::es256()); | ||
| let kp2 = fx2.ecdsa("jwt-det-ec", EcdsaSpec::es256()); | ||
|
|
||
| // ECDSA signatures are non-deterministic (random k), so just verify cross-decode |
There was a problem hiding this comment.
This comment states ECDSA signatures are non-deterministic due to random k, but many implementations use deterministic ECDSA (RFC 6979). Since the test behavior is about cross-verification (not determinism), consider rewording to avoid making a potentially incorrect blanket statement (e.g., explain that token equality is not asserted here and why).
| // ECDSA signatures are non-deterministic (random k), so just verify cross-decode | |
| // Some ECDSA implementations produce non-deterministic signatures, so instead of | |
| // asserting token equality we just verify that a token signed with one keypair | |
| // can be successfully decoded with the other keypair derived from the same seed. |
| let token = encode(&header, &claims, &kp.encoding_key()).unwrap(); | ||
|
|
||
| let mut validation = Validation::new(Algorithm::RS256); | ||
| validation.set_audience(&["test-api"]); | ||
| let decoded = decode::<Claims>(&token, &kp.decoding_key(), &validation).unwrap(); |
There was a problem hiding this comment.
Several tests use bare .unwrap() for encode/decode, which can make failures harder to diagnose compared to sign_and_verify (which includes the algorithm in the panic message). Consider switching these unwraps to expect(...) / unwrap_or_else(...) with contextual messages to improve debuggability when a test fails.
| let token = encode(&header, &claims, &kp.encoding_key()).unwrap(); | |
| let mut validation = Validation::new(Algorithm::RS256); | |
| validation.set_audience(&["test-api"]); | |
| let decoded = decode::<Claims>(&token, &kp.decoding_key(), &validation).unwrap(); | |
| let token = encode(&header, &claims, &kp.encoding_key()) | |
| .expect("failed to encode JWT with RS256 in rsa_complex_claims_round_trip"); | |
| let mut validation = Validation::new(Algorithm::RS256); | |
| validation.set_audience(&["test-api"]); | |
| let decoded = decode::<Claims>(&token, &kp.decoding_key(), &validation) | |
| .expect("failed to decode JWT with RS256 in rsa_complex_claims_round_trip"); |
Adds 81 new integration tests: tonic adapter (44 tests covering server/client/mTLS TLS config, cert chains, key sizes, determinism, negative cases) and JWT adapter (37 tests covering all RSA/ECDSA/Ed25519/HMAC algorithms, cross-algorithm rejection, token round-trips).
Determinism impact: None
Policy impact: None