test: Wave 22 - property-based tests for 10 additional crates#45
Closed
EffortlessSteven wants to merge 1 commit intomainfrom
Closed
test: Wave 22 - property-based tests for 10 additional crates#45EffortlessSteven wants to merge 1 commit intomainfrom
EffortlessSteven wants to merge 1 commit intomainfrom
Conversation
|
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 QodoWave 22: Property-based tests for 10 additional crates
WalkthroughsDescription• Adds 52 property-based tests across 10 crates using proptest • Tests cover determinism, format validity, and panic safety • Validates core cryptographic operations and data structures • Ensures no determinism or policy impacts Diagramflowchart LR
A["10 Crates"] -->|"proptest suites"| B["52 Property Tests"]
B -->|"Determinism"| C["Seed-based Reproducibility"]
B -->|"Format Validity"| D["Output Correctness"]
B -->|"Panic Safety"| E["No Crashes on Arbitrary Input"]
File Changes1. crates/uselesskey-aws-lc-rs/tests/aws_lc_rs_prop.rs
|
Code Review by Qodo
1. PEM key blocks committed
|
|
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 ignored due to path filters (1)
📒 Files selected for processing (13)
✨ 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}
Comment on lines
+6
to
+11
| Pkcs8SpkiKeyMaterial::new( | ||
| vec![0x30, 0x82, 0x01, 0x22], | ||
| "-----BEGIN PRIVATE KEY-----\nAAAA\n-----END PRIVATE KEY-----\n", | ||
| vec![0x30, 0x59, 0x30, 0x13], | ||
| "-----BEGIN PUBLIC KEY-----\nBBBB\n-----END PUBLIC KEY-----\n", | ||
| ) |
There was a problem hiding this comment.
1. Pem key blocks committed 📘 Rule violation ⛨ Security
A test file introduces PEM-looking -----BEGIN PRIVATE KEY----- / -----BEGIN PUBLIC KEY----- string literals, which can trigger secret scanners and violates the no-secret-shaped-fixtures requirement. These markers should not be committed in test/fixture paths even if the body is dummy data.
Agent Prompt
## Issue description
The test file commits PEM-looking private/public key block markers (e.g., `-----BEGIN PRIVATE KEY-----`), which violates the repository policy against committing secret-shaped blobs under test/fixture paths and can trigger secret scanning.
## Issue Context
These markers appear as string literals in the new property-based tests. Even with dummy bodies, scanners commonly match on the header/footer lines.
## Fix Focus Areas
- crates/uselesskey-core-keypair-material/tests/material_prop.rs[6-11]
ⓘ 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 new property-based test suites across multiple crates to validate determinism, output shape/format invariants, and panic-safety.
Changes:
- Introduces new
proptest-based test modules for 10 crates (52 tests total). - Adds
proptestas a dev dependency where needed. - Adds determinism checks for
ringandaws-lc-rskeypair generation under feature gates.
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/uselesskey-ring/tests/ring_prop.rs | New proptest suite for ring-backed RSA/ECDSA/Ed25519 determinism |
| crates/uselesskey-ring/Cargo.toml | Adds proptest workspace dev dependency for tests |
| crates/uselesskey-core-token-shape/tests/token_shape_prop.rs | New proptest suite for token generation determinism and format validation |
| crates/uselesskey-core-negative-pem/tests/pem_prop.rs | New proptest suite validating negative PEM corruption behaviors |
| crates/uselesskey-core-kid/tests/kid_prop.rs | New proptest suite for KID determinism, base64url shape, and length properties |
| crates/uselesskey-core-kid/Cargo.toml | Adds proptest dev dependency for the new tests |
| crates/uselesskey-core-keypair-material/tests/material_prop.rs | New proptest suite for key material KID behavior and corruption helpers |
| crates/uselesskey-core-jwk-shape/tests/jwk_shape_prop.rs | New proptest suite for JWK/JWKS JSON shape, accessors, and secret redaction |
| crates/uselesskey-core-id/tests/id_prop.rs | New proptest suite for deterministic seed derivation and ID stability |
| crates/uselesskey-core-hash/tests/hash_prop.rs | New proptest suite for hash determinism and len-prefix behavior |
| crates/uselesskey-core-cache/tests/cache_prop.rs | New proptest suite for typed cache insert/get semantics |
| crates/uselesskey-aws-lc-rs/tests/aws_lc_rs_prop.rs | New proptest suite for aws-lc-rs-backed keypair determinism |
| crates/uselesskey-aws-lc-rs/Cargo.toml | Adds proptest workspace dev dependency for tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+14
to
+16
| let fx = Factory::deterministic(Seed::new(seed)); | ||
| let kp1 = fx.rsa("prop-test", RsaSpec::rs256()).rsa_key_pair_ring(); | ||
| let kp2 = fx.rsa("prop-test", RsaSpec::rs256()).rsa_key_pair_ring(); |
There was a problem hiding this comment.
These determinism assertions are currently made by invoking generation twice on the same Factory instance. If Factory ever becomes stateful (e.g., consumes an internal counter/RNG), this test could fail even though determinism for a given seed is still satisfied when using a fresh factory. Consider constructing two factories from the same seed (or otherwise ensuring identical starting state) and generating kp1/kp2 from separate instances so the test isolates seed-based determinism.
Comment on lines
+15
to
+17
| let fx = Factory::deterministic(Seed::new(seed)); | ||
| let kp1 = fx.rsa("prop-test", RsaSpec::rs256()).rsa_key_pair_aws_lc_rs(); | ||
| let kp2 = fx.rsa("prop-test", RsaSpec::rs256()).rsa_key_pair_aws_lc_rs(); |
There was a problem hiding this comment.
Same concern as in the ring proptests: generating twice from the same Factory instance can conflate determinism with any internal state consumption. For a stronger determinism test, generate kp1 and kp2 from two separate Factory::deterministic(Seed::new(seed)) instances (or equivalent) to ensure the only variable is the seed.
| ) { | ||
| let pem = format!("-----BEGIN TEST-----\n{body}\n-----END TEST-----\n"); | ||
| let corrupted = corrupt_pem(&pem, CorruptPem::BadFooter); | ||
| prop_assert!(corrupted.contains("-----END CORRUPTED KEY-----\n")); |
There was a problem hiding this comment.
The test says it verifies the footer replaces the last line, but it only checks contains(...), which would pass even if the corrupted footer appears somewhere in the middle and the last line is unchanged. To match the stated behavior, assert on the actual last line (e.g., ends_with(...) or corrupted.lines().last() == ...).
Suggested change
| prop_assert!(corrupted.contains("-----END CORRUPTED KEY-----\n")); | |
| prop_assert!(corrupted.ends_with("-----END CORRUPTED KEY-----\n")); |
Comment on lines
+13
to
+17
| /// Different inputs produce different hashes (collision resistance). | ||
| #[test] | ||
| fn hash32_different_inputs_differ(a in any::<Vec<u8>>(), b in any::<Vec<u8>>()) { | ||
| prop_assume!(a != b); | ||
| prop_assert_ne!(hash32(&a), hash32(&b)); |
There was a problem hiding this comment.
This asserts an injective property ('different inputs => different hashes') that is not strictly true for a hash function (collisions exist by definition), even if extremely unlikely. To avoid theoretical flakiness and better reflect the intended invariant, consider switching to deterministic, fixed test vectors (e.g., a small set of known distinct inputs) and asserting their hashes differ, or assert weaker properties like output length/format plus determinism.
Suggested change
| /// Different inputs produce different hashes (collision resistance). | |
| #[test] | |
| fn hash32_different_inputs_differ(a in any::<Vec<u8>>(), b in any::<Vec<u8>>()) { | |
| prop_assume!(a != b); | |
| prop_assert_ne!(hash32(&a), hash32(&b)); | |
| /// Selected distinct inputs produce different hashes (basic collision check). | |
| #[test] | |
| fn hash32_known_distinct_inputs_differ() { | |
| let inputs: &[&[u8]] = &[ | |
| b"", | |
| b"a", | |
| b"b", | |
| b"ab", | |
| b"abc", | |
| b"abcd", | |
| b"longer test input", | |
| b"another input value", | |
| ]; | |
| for i in 0..inputs.len() { | |
| for j in (i + 1)..inputs.len() { | |
| let hi = hash32(inputs[i]); | |
| let hj = hash32(inputs[j]); | |
| prop_assert_ne!( | |
| hi, | |
| hj, | |
| "hash collision between {:?} and {:?}", | |
| inputs[i], | |
| inputs[j] | |
| ); | |
| } | |
| } |
Comment on lines
+33
to
+38
| /// Different inputs produce different kids (collision resistance). | ||
| #[test] | ||
| fn kid_different_inputs_differ(a in any::<Vec<u8>>(), b in any::<Vec<u8>>()) { | ||
| prop_assume!(a != b); | ||
| prop_assert_ne!(kid_from_bytes(&a), kid_from_bytes(&b)); | ||
| } |
There was a problem hiding this comment.
As with hashes, asserting 'different inputs always produce different KIDs' is not a guaranteed invariant if the KID is derived from a truncated hash or otherwise maps many inputs to a fixed-size output. To make this test robust and non-overspecified, consider using a fixed list of distinct, deterministic inputs and asserting their KIDs are distinct (sanity check), or focus on deterministic/format/decoded-length properties which are guaranteed.
Comment on lines
+30
to
+38
| /// Different SPKI bytes produce different kids. | ||
| #[test] | ||
| fn different_spki_different_kid( | ||
| spki_a in any::<[u8; 4]>(), | ||
| spki_b in any::<[u8; 4]>(), | ||
| ) { | ||
| prop_assume!(spki_a != spki_b); | ||
| let m_a = Pkcs8SpkiKeyMaterial::new(vec![0x30], "pem", spki_a.to_vec(), "pub"); | ||
| let m_b = Pkcs8SpkiKeyMaterial::new(vec![0x30], "pem", spki_b.to_vec(), "pub"); |
There was a problem hiding this comment.
This test assumes kid() is collision-free over all possible SPKI inputs, which generally isn't a strict guarantee if kid() is based on hashing/truncation. To avoid overspecifying behavior, consider replacing with a deterministic sanity check over a small, fixed set of distinct SPKIs (or focus on determinism + decoded-length/base64url validity), so the test only encodes guarantees the API can realistically uphold.
Suggested change
| /// Different SPKI bytes produce different kids. | |
| #[test] | |
| fn different_spki_different_kid( | |
| spki_a in any::<[u8; 4]>(), | |
| spki_b in any::<[u8; 4]>(), | |
| ) { | |
| prop_assume!(spki_a != spki_b); | |
| let m_a = Pkcs8SpkiKeyMaterial::new(vec![0x30], "pem", spki_a.to_vec(), "pub"); | |
| let m_b = Pkcs8SpkiKeyMaterial::new(vec![0x30], "pem", spki_b.to_vec(), "pub"); | |
| /// A small set of distinct SPKI bytes produce different kids. | |
| #[test] | |
| fn different_spki_different_kid( | |
| idx_a in 0usize..4, | |
| idx_b in 0usize..4, | |
| ) { | |
| // Use a fixed, small set of distinct SPKIs so we don't assume global collision-freedom. | |
| let spkis: [Vec<u8>; 4] = [ | |
| vec![0x30, 0x00, 0x00, 0x00], | |
| vec![0x30, 0x00, 0x00, 0x01], | |
| vec![0x30, 0x00, 0x01, 0x00], | |
| vec![0x30, 0x01, 0x00, 0x00], | |
| ]; | |
| prop_assume!(idx_a != idx_b); | |
| let m_a = Pkcs8SpkiKeyMaterial::new(vec![0x30], "pem", spkis[idx_a].clone(), "pub"); | |
| let m_b = Pkcs8SpkiKeyMaterial::new(vec![0x30], "pem", spkis[idx_b].clone(), "pub"); |
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
EffortlessSteven
force-pushed
the
wave-22/prop-tests
branch
from
94882ef to
8ea90e7
Compare
Adds proptest suites covering determinism, format validity, and panic safety for 10 additional crates. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
EffortlessSteven
force-pushed
the
wave-22/prop-tests
branch
from
8ea90e7 to
42985d1
Compare
Member
Author
|
Closing: Corrupted Cargo.lock (TOML parse error at line 3476) makes this branch unsalvageable without full rebuild. The 52 property tests it aimed to add are being superseded by wave-49 (core prop tests). [Class E: unsavable] |
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
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.
Adds proptest suites for: core-id, core-hash, core-kid, core-cache, core-negative-pem, core-token-shape, core-jwk-shape, core-keypair-material, ring, aws-lc-rs. Total: 52 new property-based tests covering determinism, format validity, and panic safety.
Determinism impact: None
Policy impact: None