fix(crypto): tolerate unparseable EncStrings across FFI boundary - #1462
fix(crypto): tolerate unparseable EncStrings across FFI boundary#1462quexten wants to merge 3 commits into
Conversation
A malformed EncString used to error out at parse time, which for the WASM bindings meant the failure surfaced while lifting arguments across the FFI boundary, before the called method ran. A single bad item took down an entire batch decryption instead of being reported as a per-item failure. FromStr now maps anything that matches no known format to EncString::Unparseable, keeping the raw value so it round-trips unchanged. Decryption, unwrapping and to_buffer reject it with a dedicated CryptoError::UnparseableEncString, so the failure surfaces where it can be attributed. Call sites that must reject malformed input up front use the new EncString::parse_strict.
|
Claude Code is reviewing this pull request... If this comment does not update with results, check the Actions log. |
| it("reports the malformed cipher as a failure and still decrypts the rest", async () => { | ||
| const { ciphers, valid, malformed } = await setup(); | ||
|
|
||
| const result = await ciphers.decrypt_list_full_with_failures([valid, malformed]); |
There was a problem hiding this comment.
Previously this would throw since the parse fails.
quexten
changed the title
quexten
changed the title
🔍 SDK Breaking Change DetectionSDK Version:
Breaking change detection uses the build of the SDK from this branch, including any incompatibities pre-existing on or merged into this branch. Check the workflow logs to confirm. |
dani-garcia
left a comment
dani-garcia
left a comment
There was a problem hiding this comment.
This looks great, thanks!
|
|
||
| /// Never returns an error: anything that does not match a known format becomes | ||
| /// [EncString::Unparseable]. Use [EncString::parse_strict] to reject those instead. | ||
| fn from_str(s: &str) -> Result<Self, Self::Err> { |
There was a problem hiding this comment.
Non-blocking improvement: I wonder if we should provide some From<&str> and From<String> implementations, to allow conversion without having the parent do error handling. In the future it might be nice if the API bindings returned the EncString type directly for example.
There was a problem hiding this comment.
Agree. Will leave it to follow-up, but:
In the future it might be nice if the API bindings returned the EncString type directly for example. is something I'd very much like to enable.
There was a problem hiding this comment.
Yeah, it's something I've looked into the past a few times. The server already has the [EncryptedString] annotation, and I've already created a filter to make sure the data makes it into the schema: https://github.com/bitwarden/server/blob/main/src/SharedWeb/Swagger/EncryptedStringSchemaFilter.cs#L14
With that done, it's a single config like to get openapi to emit a custom type in the rust code, unfortunately the fact that server has a single EncString annotation while the SDK splits it based on Symmetric/Asymmetric makes it a bit hard as it is.
There was a problem hiding this comment.
NVM I see i missed a test and failed to run test before sending the PR :(
I'll add your suggestion now since I'll have to get re-approval anyways.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1462 +/- ##
==========================================
+ Coverage 86.01% 86.18% +0.17%
==========================================
Files 525 534 +9
Lines 77769 79831 +2062
==========================================
+ Hits 66891 68803 +1912
- Misses 10878 11028 +150 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
2 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-42212
📔 Objective
A malformed
EncStringerrored at parse time. In the WASM bindings that failure happened whilemoving arguments across the FFI boundary, before the called method ran, so one bad item took down
an entire batch decryption instead of being reported as a per-item failure. This is a problem in vault decryption, because it expects to handle failures on a per item basis (see the test that's added).
FromStrnow maps anything matching no known format toEncString::Unparseable, which keeps theraw value so it round-trips unchanged. Decryption, unwrapping and
to_bufferreject it with adedicated
CryptoError::UnparseableEncString, so the failure surfaces where it can be attributed.Call sites that must reject malformed input up front use the new
EncString::parse_strict.from_buffer(binary format) stays strict.🚨 Breaking Changes
New
EncStringenum variant. Exhaustive matches onEncStringin consumers need a new arm.