Add size limits to untrusted input parsing and replace unwrap on external responses#110
Conversation
WalkthroughAdds defensive input validation and size limits: caps Nostr sign_event JSON, enforces FROST animated-frame count/assembled-size and frame sanity checks, and replaces bincode deserialization with Options using fixed-int encoding, allow_trailing_bytes, and per-record size limits. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (4)
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 |
![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
🤖 Fix all issues with AI agents
In @keep-core/src/hidden/volume.rs:
- Around line 519-523: Deserialization in the records path uses
bincode::options().with_fixint_encoding(), but the corresponding serialization
calls in store_key_outer and write_hidden_records (and the other serialize call
around lines 596-600) use plain bincode::serialize() with default varint
encoding, causing a mismatch; fix by changing those bincode::serialize(...)
calls to use the same options (e.g.,
bincode::options().with_fixint_encoding().serialize(&value)) so serialization
and deserialization use identical encoding; update every serialize call
referenced (store_key_outer, write_hidden_records, and the other serialize
instances) to use bincode::options().with_fixint_encoding() to ensure
consistency with the existing deserialize(...) usage.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
keep-cli/src/server.rskeep-core/src/frost/transport.rskeep-core/src/hidden/volume.rskeep-core/src/storage.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (7)
keep-core/src/storage.rs (2)
15-21: Good defensive additions for size-limited deserialization.The
MAX_RECORD_SIZEconstant (1 MiB) andbincode::Optionsimport set up proper bounds for untrusted input parsing.
292-296: This review comment is based on an incorrect assumption about the bincode encoding behavior.The code shows that
bincode::serialize()(lines 261, 355) stores data, andbincode::options().with_fixint_encoding().deserialize()(lines 292-296, 315-319, 388-392) retrieves it. However, the testtest_storage_key_operations(lines 474-500) explicitly validates this round-trip pattern by storing a KeyRecord and then loading it back with assertions that the data matches. This test passes, which demonstrates that no encoding mismatch exists in practice. If the encoding claim were correct, the test would fail during deserialization.Likely an incorrect or invalid review comment.
keep-cli/src/server.rs (2)
17-18: Good size validation for untrusted input.The 64 KiB limit for event JSON is reasonable and the check happens before parsing, which prevents potential DoS via malformed large payloads.
Also applies to: 293-296
317-320: Proper error handling for serialization.Replacing unwrap with explicit error handling ensures graceful failure instead of panicking on serialization errors from external event data.
keep-core/src/frost/transport.rs (2)
12-13: Good constants for bounding frame assembly.MAX_FRAME_COUNT (100) and MAX_ASSEMBLED_SIZE (64 KiB) provide reasonable limits for the animated frames protocol.
253-296: Comprehensive validation for multi-frame assembly.The implementation correctly:
- Limits total frame count upfront
- Detects duplicate frame indices
- Validates frame index bounds
- Uses
checked_addfor overflow-safe size accumulation- Checks cumulative size limit incrementally
This provides robust protection against malformed or malicious frame data.
keep-core/src/hidden/volume.rs (1)
45-54: Size limit constant and Options import added correctly.Same pattern as
storage.rsfor defensive deserialization bounds.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
keep-core/src/hidden/volume.rs (1)
521-535: Document the hidden volume capacity limit and clarify the distinction between outer and hidden volume deserialization.The
MAX_RECORD_SIZE(1MB) limit applies differently depending on the volume type:
- Outer volume (
list_keys_outer): Deserializes individualKeyRecorditems from the redb table—1MB per record is reasonable- Hidden volume (
load_hidden_records): Deserializes the entireVec<KeyRecord>in a single operation—1MB for the entire collection could be restrictive if users store thousands of keysAdditionally, the
write_hidden_recordsmethod does not enforce a size limit on the serialized output, creating a potential mismatch where data could be written that exceeds the deserialization limit.The use of
allow_trailing_bytes()is consistent across the codebase and intentionally enables forward compatibility, but it does mask data corruption. Add a comment explaining this trade-off.Recommendations:
- Document the implicit hidden volume capacity (e.g., ~2,000–5,000 keys depending on key name and secret size)
- Add validation in
write_hidden_recordsto check serialized size against the limit, or use a larger limit for collection deserialization- Add comments explaining why
allow_trailing_bytes()is used despite the corruption-masking trade-off
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
keep-core/src/hidden/volume.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (4)
keep-core/src/hidden/volume.rs (4)
45-54: LGTM! Appropriate defensive size limit for untrusted input.The import and constant are correctly defined. The 1MB limit provides protection against malicious or corrupted data while being generous enough for legitimate key records.
458-460: LGTM! Consistent serialization with fixed-int encoding.Using
with_fixint_encoding()ensures a predictable binary format that will correctly roundtrip with the corresponding deserialization options.
545-547: LGTM! Consistent serialization options.The serialization options match the pattern used in
store_key_outerand will correctly roundtrip withload_hidden_records.
600-604: LGTM! Appropriate size limit for individual record deserialization.The 1MB limit is well-suited for single
KeyRecorddeserialization. The options chain correctly matches the serialization format used instore_key_outer.
2 participants
Summary
Test plan
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.