Repository navigation
fix(sdk): stop toBuffer from masking a decrypt failure as a network error (DSPX-4703) - #1035
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 |
|
|
Closing as superseded: this fix already landed on That merge includes the same |
1 participant
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