Repository navigation
Conversation
🦋 Changeset detectedLatest commit: b684a21 The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the
|
Mutation testing (cargo-mutants,
|
| caught | missed | unviable | timeout |
|---|---|---|---|
| 10 | 0 | 31 | 0 |
Every mutant in the changed lines was caught by a test.
Two-axis review (standards and spec)This reviews commit StandardsHard violations
Documented-standard concerns (judgement calls)
Smells (judgement calls)
SpecMatches: the keyset method names and shapes, Missing or partial
Implemented, but looks wrong
Beyond the spec
CIThe mutants gate missed three mutants:
|
BREAKING CHANGE: keyset encrypt/decrypt use opaque Ciphertext and separate context methods. Structural calls move to encrypt_node/decrypt_node; borrowed conversion helpers move to target::legacy. Declare owned conversions with EncryptTarget/DecryptSource and explicit context capabilities. Guard plaintext during planning and preserve the existing sealed-leaf bytes and Go/dynamic execution paths.
- Rename the structural entry points encrypt_node / decrypt_node to encrypt_tree / decrypt_tree. - Rename the sealed Scalar trait to OpaqueEncode / OpaqueDecode, the names the API plan accepted. Both stay sealed. - Rename Ciphertext::tag to key_tag, so it cannot be read as the AEAD tag. - Give Ciphertext Serde: padded Base64 in human-readable formats, raw bytes in binary ones. - Report a non-single tree as Error::OpenedType, not Error::Aead (ADR-0010). - Restore NonEmpty on the shared encrypt/open core. The scalar path reaches the core through its own entry that allows empty context, and ADR-0003 records the exception. - Open bytes written by the pre-scalar build through Ciphertext. - Kill the three mutants CI missed. The NoCallerContext conversion is equivalent and is excluded. - Add the CHANGELOG Breaking entries and drop roadmap prose from rustdoc.
9a44074 to
5b9f4bd
Compare
|
Pushed 5b9f4bd, rebased onto the latest
Not changed: |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
cipherstash-agent
left a comment
There was a problem hiding this comment.
Review attestation
Source: Claude Code /code-review medium with cross-check of findings against existing PR feedback, run on head 302aaf9.
Critical or blocking issues: none.
Findings: 1 minor (inline on scalar.rs:126: uncapped Vec::with_capacity from the deserializer's size_hint in Ciphertext's visit_seq; the crate's existing reserved() cap shows the convention). It was verified at head and is not raised in the existing feedback.
Existing feedback: the earlier two-axis review's findings were addressed in 5b9f4bdf6. I found nothing further on those points.
Not covered: I did not run the test suite or CI. Codex review was still running when this review was posted.
Generated by Claude Code
| self, | ||
| mut seq: A, | ||
| ) -> Result<Ciphertext, A::Error> { | ||
| let mut bytes = Vec::with_capacity(seq.size_hint().unwrap_or(0)); |
There was a problem hiding this comment.
Minor: Vec::with_capacity(seq.size_hint().unwrap_or(0)) trusts the deserializer's length hint. For untrusted input, a few bytes can declare billions of elements, so this can abort on allocation before from_bytes ever validates the envelope.
The crate already guards against this elsewhere: packages/stack-encrypt/src/dynamic/context.rs:187 has reserved(hint), which caps the reservation at 64 and lets the Vec grow as elements are read. Cap the reservation here the same way, for example .min(…) the hint, or use a small fixed bound.
Source: Claude Code /code-review medium, verified at 302aaf9.
Generated by Claude Code
There was a problem hiding this comment.
Fixed in b684a21. The reservation is now capped at 256 bytes (.min(256)), and a longer sequence grows as it's read. I inlined the cap because reserved() is in the feature-gated dynamic module. A new test uses a sequence that holds three bytes but claims usize::MAX. Without the cap it panics with "capacity overflow"; with the cap it returns an envelope error.
The direct Rust API previously required an AAD argument and returned a structural node even for one integer. This implements the first scalar slice of the API redesign in #1365:
EncryptTargetandDecryptSourcedeclare operations; the ownedEncryptIntoand destination-directedDecryptFromhelpers execute them through the existing batched engine.RequiredContext,OptionalContext, andNoCallerContextcontrol method availability explicitly:Defaultnever grants permission to omit context. Encryption guards the owned source withZeroizing, plans synchronously through a borrow, and wipes the source before returning the pending request. This imposes no blanketClonebound and also covers discarded requests, planning errors and context-conversion unwinding. Returned plaintext belongs to the caller; this is not a claim of complete compiler-copy erasure or constant-time serialization.Ciphertextexposes canonical byte encoding/decoding and provider-tag access over the unchanged SealedValue v1 envelope. Cross-opening tests exercise both old-writer/new-reader and new-writer/old-reader paths. No Go/dynamic grammar, stored encoding, SQL, or cryptographic derivation changes.Migration and scope
encrypt_node/decrypt_node. Existing plans and derives retain their declaration engine; their borrowed conversion helpers move totarget::legacy. Existing consumers and tests are updated explicitly.Validation
No live ZeroKMS or PostgreSQL tests were run. Based on latest next at ffeed7a.