fix(sdk): stop toBuffer from masking a decrypt failure as a network error (DSPX-4703) - #1035
Open
dmihalcik-virtru wants to merge 1 commit into
Open
dmihalcik-virtru wants to merge 1 commit into
dmihalcik-virtru wants to merge 1 commit into
Conversation
…rror (DSPX-4703) `streamToBuffer` drained the plaintext with `new Response(stream).arrayBuffer()`, and Chrome replaces *any* error raised while it pulls a Response body with a bare `TypeError: Failed to fetch`, discarding the original. So in the browser — and only in the browser — a file that failed its integrity check surfaced to the caller as something indistinguishable from a dropped connection. That is the one distinction a caller most needs to make here: an `IntegrityError` means the bytes were tampered with and the operation must not be retried, while a network error means it should be. Draining with a reader instead rejects with the error the stream was errored with. `toString` had the same problem via `Response.text()`; it buffers first now, which also fixes a latent bug of its own — `Response.text()` decodes per-chunk, so a multi-byte character split across a segment boundary came back corrupted. Not a regression. `streamToBuffer` has used `Response` since 2023, and karma launches the system Chrome rather than a version pinned in any lockfile, so there is no dependency bump to point at. What changed is that nothing tested it: no existing case drove a stream error through `toBuffer()` in a browser, so the masking was invisible. The new `lib/tests/mocha/unit/decorated-readable-stream.spec.ts` closes that hole with 7 cases, asserting the *identical* error instance comes back out rather than merely the right type. It lives under `tests/mocha/unit/` so webpack picks it up and it runs under karma as well as Node. Verified by reverting the implementation alone: Node passes all 7 either way, while Chrome goes from 376 passing to `4 FAILED, 372 SUCCESS` — every error-propagation case, and only those. With the fix the suite is 376 passing / 6 pending under mocha and karma alike, coverage thresholds met, lint clean. Found while working DSPX-4703, which adds decrypt-time integrity errors that this would otherwise have swallowed, but the defect and the fix are independent of that work. Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
|
Warning Review limit reachedNext included review available in 32 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
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 |
|
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
1 participant
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.
Orthogonal to the #1030 → #1031 → #1034 stack, extracted so it can land on its own. Found while working DSPX-4703; the defect and the fix predate it.
The bug
streamToBufferdrained the plaintext withnew Response(stream).arrayBuffer(). Chrome replaces any error raised while it pulls a Response body with a bareTypeError: Failed to fetch, discarding the original.So in the browser — and only in the browser — this:
A caller cannot tell a tampered file from a dropped connection, which is the one distinction that matters most here: an
IntegrityErrormeans the bytes were altered and the operation must not be retried; a network error means it should be. Any consumer with retry-on-network-error logic will happily re-fetch a file that is being tampered with, and any consumer surfacing the message to a user tells them the wrong thing.toStringhad the same problem viaResponse.text().The fix
Drain with a reader, which rejects with the error the stream was actually errored with.
toStringnow buffers first and decodes once — which incidentally fixes a latent bug of its own, sinceResponse.text()decodes per-chunk and corrupted any multi-byte character split across a segment boundary.Not a regression
streamToBufferhas usedResponsesince 2023 (lib/tdf3/src/client/DecoratedReadableStream.ts, unchanged in substance across ~15 commits).karma-chrome-launcherlaunches the system Chrome — no Chrome version is pinned in any lockfile — so there is no dependency bump to point at either.What changed is only that someone looked. No existing test drove a stream error through
toBuffer()in a browser: the oneIntegrityErrorcase inencrypt-decrypt.spec.tsis a mocked KAS response, not a stream error. The masking has been silently present the whole time.How to test
lib/tests/mocha/unit/decorated-readable-stream.spec.tsis new — 7 cases:streamToBufferconcatenates chunks in order, and handles an empty streamstreamToBufferpropagates the original error both on the first pull and after partial outputtoBufferandtoStringeach propagate rather than masktoStringdecodes utf-8 across a chunk boundary (the per-chunk-decode bug above)The error cases assert
strictEqualagainst the thrown instance, notinstanceOf— the point is that no layer substitutes an error of its own, and a type check would pass on a re-wrap.It lives under
tests/mocha/unit/sowebpack.test.config.cjspicks it up and it runs under karma as well as Node. That placement is load-bearing: the bug does not reproduce outside a browser.Baseline
Reverting the implementation alone, keeping the tests:
The 4 are exactly the error-propagation cases; the 3 that don't involve an errored stream pass either way. Node preserves the error through
Response— this is Chrome-specific, which is why a Node-only test would not have caught it.With the fix: 376 passing / 6 pending under mocha, 376 under karma, web-test-runner green, coverage thresholds met,
npm run lintclean.Risk
Decrypt read path, but narrowly. Success behaviour is unchanged — same bytes out, verified by the round-trip tests across the existing suite. The only behavioural change is which error object a caller sees on failure, and only in a browser. Callers matching on
TypeError: Failed to fetchto detect decrypt failure would break, but that string is indistinguishable from a genuine network error, so no correct caller can be doing it.DSPX-4703