Skip to content

Decode stream bodies once instead of per chunk - #10877

Open
Zuhef wants to merge 1 commit into
firebase:mainfrom
Zuhef:fix/alpha-streamtostring-multibyte
Open

Decode stream bodies once instead of per chunk#10877
Zuhef wants to merge 1 commit into
firebase:mainfrom
Zuhef:fix/alpha-streamtostring-multibyte

Conversation

@Zuhef

@Zuhef Zuhef commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Description

streamToString decodes each chunk on its own:

s.on("data", (d: Buffer | string) => {
  b += d.toString();
});

A UTF-8 character that straddles a chunk boundary is therefore split across two independent toString() calls, and each half decodes to replacement characters. Code points, so the difference is unambiguous:

input boundary falls inside result
café é (2 bytes) U+FFFD U+FFFD instead of U+00E9
a中b (3 bytes) U+FFFD U+FFFD U+FFFD instead of U+4E2D
hi😀there emoji (4 bytes) U+FFFD U+FFFD U+FFFD instead of U+1F600

Input that happens to be split on a character boundary, or that is pure ASCII, is unaffected — which is why the existing tests ("hello world", undefined, null) pass.

This buffers the chunks and decodes once at the end.

No known user-visible impact today

Worth being explicit, since it affects how you'd prioritise this. I could not reach the bug through a real command, and I looked:

  • The responseType: "json" path calls res.text()streamToString, but only when the undici branch is taken, which requires compress: false.
  • Both callers that pass compress: falsehosting/initMiddleware.ts and hosting/proxy.ts — use responseType: "stream" and pipe the body, so they never call text().
  • The direct callers in database-get.ts, downloadUtils.ts, profiler.ts and emulator/hubExport.ts all read the body only behind status >= 400, and error bodies are normally small enough to arrive in one chunk.

So this is a latent bug in a shared helper (also re-exported from utils.ts) rather than something failing in the field. It would bite the first caller that reads a large non-ASCII body.

Verification

Beyond the new unit test, I checked the behaviour against a real undici response: a local server that writes a body in two write() calls split inside é. undici delivered two data events ([20, 15]), and streamToString returned the corrupted string before this change and the correct one after.

Also covered, all passing: an empty stream, a single chunk, a stream with setEncoding("utf8") (so chunks arrive as strings), every byte pushed as its own chunk, and error events still rejecting. I ran the split-character case 20 times to confirm it isn't timing dependent.

Buffer.isBuffer(d) ? d : Buffer.from(String(d), "utf8") keeps the previous behaviour for anything that isn't a Buffer, so an object-mode stream still stringifies rather than throwing in Buffer.concat.

$ npx mocha src/utils.spec.ts src/apiv2.spec.ts src/downloadUtils.spec.ts
110 passing, 1 failing

That one failure is pre-existing and unrelated — a timezone-dependent assertion (expected '2020-02-23 00:05:45' to match /^2020-02-22 \d\d:35:45$/); I'm on UTC+5:30. It fails identically on an unmodified checkout. With src/streamUtils.ts reverted the new test fails, so it does pin the fix.

npm run test:compile is clean, and eslint reports no errors on either changed file.

streamToString called toString() on every chunk, so a UTF-8 character split across a chunk boundary decoded as replacement characters. Buffer the chunks and decode once at the end.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request fixes an issue in streamToString where multi-byte UTF-8 characters spanning across stream chunk boundaries were corrupted during decoding. The fix buffers raw chunks and decodes them all at once at the end. A unit test was also added to verify this behavior. The reviewer pointed out a potential issue where Uint8Array chunks (common in modern Node.js) would fail the Buffer.isBuffer check and be incorrectly converted to a comma-separated string of numbers via String(d), leading to corruption. A robust type-checking approach was suggested to handle Uint8Array, string, and other types safely.

Comment thread src/streamUtils.ts
Comment on lines +28 to 35
const chunks: Buffer[] = [];
s.on("error", reject);
s.on("data", (d: Buffer | string) => {
b += d.toString();
// Buffer the raw chunks and decode once at the end. Decoding each chunk
// on its own corrupts any multi-byte character that straddles a chunk
// boundary, turning it into replacement characters.
chunks.push(Buffer.isBuffer(d) ? d : Buffer.from(String(d), "utf8"));
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

If the stream emits Uint8Array chunks (which is common in modern Node.js and web-compatible streams), Buffer.isBuffer(d) will return false. This falls back to Buffer.from(String(d), "utf8"). Calling String() on a Uint8Array converts it to a comma-separated string of numbers (e.g., "104,101,108,108,111"), which will corrupt the decoded string.

Since Buffer.concat natively accepts both Buffer and Uint8Array instances, we can safely push them directly. We should explicitly check for Uint8Array (which also covers Buffer since Buffer inherits from Uint8Array in Node.js) and string to handle all cases correctly.

    const chunks: Uint8Array[] = [];
    s.on("error", reject);
    s.on("data", (d: Buffer | string | Uint8Array) => {
      // Buffer the raw chunks and decode once at the end. Decoding each chunk
      // on its own corrupts any multi-byte character that straddles a chunk
      // boundary, turning it into replacement characters.
      if (d instanceof Uint8Array) {
        chunks.push(d);
      } else if (typeof d === "string") {
        chunks.push(Buffer.from(d, "utf8"));
      } else {
        chunks.push(Buffer.from(String(d), "utf8"));
      }
    });

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants