fix(browser): decode response bodies as UTF-8 text in the run sandbox - #357
Merged
Conversation
Response.text() called Buffer.prototype.toString('utf8') on the body, but
the QuickJS sandbox has no Buffer: the body is a plain Uint8Array, whose
inherited Array.prototype.toString comma-joins the elements and discards
the encoding argument. Every response.text() returned "123,34,105,..."
and response.json() failed with "unexpected data at the end".
Decode through quickjsEncoding.decodeText instead, the same injected UTF-8
codec the vendored protocol primitives already use. Request.postData() had
the identical defect and is fixed alongside it. APIResponse.text() in
fetch.ts shares the bug but is unreachable today, so it is left alone.
Closes #354
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
🟠 Maintainer review suggested — low confidenceThe automated review could not reach a fully supported conclusion. Limitations
This review is advisory and does not block merging. |
`_innerFetch` evaluated `options.params instanceof URLSearchParams`
unconditionally, and QuickJS has no `URLSearchParams`, so every
`page.request.*` call threw `ReferenceError` before a response existed.
Guard the check behind `globalThis`, matching the `globalThis.FormData &&`
idiom upstream already uses a few lines below.
With the guard in place the rest of the path becomes reachable, so also
decode `APIResponse.text()` through the injected UTF-8 codec instead of
`Uint8Array.prototype.toString('utf8')` (which comma-joins bytes), and
treat request/multipart payloads as `Uint8Array` rather than `Buffer`,
which the sandbox also lacks.
Closes #359
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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
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.
Closes #354
What was wrong
Response.text()in the vendored Playwright client didcontent.toString('utf8')on the body. Under QuickJS there is no NodeBuffer, so the body is a plainUint8ArrayandUint8Array.prototype.toStringfalls through toArray.prototype.toString— comma-joining the byte values and discarding the'utf8'argument.response.json()thenJSON.parsed that string and always failed.Request.postData()had the identical defect (?.toString('utf-8')), so it is fixed in the same commit.The fix
quickjsEncoding.decodeText(...)fromsrc/browser/run/playwright-client/quickjs-platform.ts, the same injected UTF-8 codecvendor/protocol/validatorPrimitives.tsalready uses for base64.Note: the issue suggested
this._platform.decodeText(content). That would have beenundefinedat runtime —decodeTextlives on the exportedquickjsEncodingobject, not on thePlatformtype. ImportingquickjsEncodingis the pattern the vendored protocol files already follow.src/browser/run/generated/playwright-client.jsis regenerated withnode scripts/build-playwright-sandbox-client.mjs,vendor-manifest.json'svendorSha256is updated for the vendor edit, and the README's "Local patches" list documents it.playwright-client-build.test.tsneeded no change; its--checkpasses.Before / after
Same repro, same daemon, only the bundle swapped between runs.
Before:
{ "type": "string", "first80": "123,34,105,100,34,58,49,44,34,116,105,116,108,101,34,58,34,69,115,115,101,110,99", "len": 5329, "jsonError": "unexpected data at the end" }After:
{ "type": "string", "first80": "{\"id\":1,\"title\":\"Essence Mascara Lash Princess\",\"description\":\"The Essence Masca", "len": 1510, "id": 1, "title": "Essence Mascara Lash Princess" }Verified
https://dummyjson.com/products/1through a daemon restarted from this branch;response.json()returns real fields.src/browser/run/runner.test.tscoveringpostData(),text(), andjson()against the in-testexample.testroutes (no network). It fails with the old bundle (unexpected data at the end) and passes with the new one.npx vitest run --project unit: 117 failures both before and after on the same checkout — identical set, all pre-existing/environment-related; no new failures, and the suite gains the one new passing test.npx tsc --noEmit: clean.node scripts/build-playwright-sandbox-client.mjs --check: clean.Out of scope, noticed in passing
APIResponse.text()invendor/client/fetch.tshas the sametoString('utf8')bug, but it is unreachable: everyAPIResponsecomes from_innerFetch, which evaluatesoptions.params instanceof URLSearchParamsunconditionally, andURLSearchParamsis not defined in the sandbox — sopage.request.get(...)throwsReferenceErrorbefore any response exists. Left untouched per browser run: response.text() returns comma-joined bytes, breaking response.json() #354's scope; theURLSearchParamsgap is a separate bug worth its own issue.Buffer.byteLength/Buffer.fromremain innetwork.tsfulfill/override paths — same missing-Bufferfamily, different symptom, not touched here./syntaxerror/imisclassification this bug triggers is browser run: syntax-error classifier mislabels runtime JSON.parse failures #355 and is deliberately untouched.🤖 Generated with Claude Code