Skip to content

js: read XHR response bodies as bytes, not through text() - #755

Open
alexskinner wants to merge 1 commit into
h4ckf0r0day:mainfrom
alexskinner:fix/xhr-binary-response
Open

js: read XHR response bodies as bytes, not through text()#755
alexskinner wants to merge 1 commit into
h4ckf0r0day:mainfrom
alexskinner:fix/xhr-binary-response

Conversation

@alexskinner

Copy link
Copy Markdown
Contributor

Fixes #754.

XMLHttpRequest is implemented over fetch and read every response with
resp.text() before choosing a responseType, so the binary types were rebuilt
from a string:

case 'arraybuffer': xhr.response = new TextEncoder().encode(text).buffer;
case 'blob':        xhr.response = new Blob([text]);

text()TextEncoder().encode() is not a round-trip for bytes that are not
valid UTF-8. A lenient decode treats any high byte as a lead byte, so 82 83
becomes U+0083 and re-encodes as c2 83 — every byte ≥ 0x80 is rewritten and
the length changes with the content. fetch() was unaffected because it never
took that detour, which is why this hid: ASCII bodies, and therefore all JSON
and text over XHR, are untouched.

The change

Read the body once as an ArrayBuffer and decode to text only for the text-ish
response types.

blob had the same bug and is fixed with it. Not decoding the body for the
binary types also avoids building a multi-megabyte string that nothing reads;
responseText is not defined for those types, so it is left '' rather than
throwing — the conservative option, but say if you would rather it threw.

Test

crates/obscura/tests/xhr_binary_response.rs serves a 256-byte fixture holding
every byte value and asserts on the bytes, not just the length — a same-length
corruption would otherwise pass. Confirmed to fail without the fix:

expected all 256 bytes, got Number(255) — a lossy text round-trip changes the length

Before / after

before   fetch len 256 | xhr len 255 | diverge at 128
after    fetch len 256 | xhr len 256 | diverge at 256      (matches Chrome 147)

On a real 4,196,020-byte asset, XHR returned 4,118,955 bytes with a corrupted
header; it is now byte-exact. This matters beyond the obvious, because
Emscripten's generated loader fetches .wasm and data files with
XMLHttpRequest + responseType='arraybuffer' rather than fetch() — so WASM
applications were loading corrupted assets and failing far from the cause. One
that previously aborted on a nonsense allocation derived from a mangled header
now starts and runs, rendering to a 2D canvas and responding to
Input.dispatchKeyEvent.

Related to #716, which covers binary request bodies and the CDP fulfill
path. This is the response side of XHR and a separate code path, though likely
the same underlying habit of reaching for String where Vec<u8> belongs.

XMLHttpRequest is implemented over fetch and read every response with
resp.text() before choosing a responseType, so the binary types were built
from a string:

    case 'arraybuffer': xhr.response = new TextEncoder().encode(text).buffer;
    case 'blob':        xhr.response = new Blob([text]);

text() -> TextEncoder().encode() is not a round-trip for bytes that are not
valid UTF-8. A lenient decode treats any high byte as a lead byte, so 0x82 0x83
becomes U+0083 and re-encodes as c2 83; every byte >= 0x80 is rewritten and the
length changes with the content. fetch() was unaffected because it never took
that detour.

Read the body once as an ArrayBuffer and decode to text only for the text-ish
response types. Not decoding a multi-megabyte binary body also avoids building
a string nothing reads; responseText is not defined for the binary types, so it
is left empty rather than throwing.

A 256-byte file of every byte value now round-trips identically on both paths,
matching Chrome. A 4,196,020-byte asset that arrived as 4,118,955 bytes with a
corrupted header is now byte-exact, and an Emscripten application that aborted
while parsing it starts and runs.
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.

XMLHttpRequest with responseType='arraybuffer' corrupts binary responses (fetch() is correct)

1 participant