fix: percent-encode X-Vinext-Params to allow non-ASCII route params#677
fix: percent-encode X-Vinext-Params to allow non-ASCII route params#677southpolesteve merged 1 commit intomainfrom
Conversation
commit: |
|
…676) Headers.set() requires ByteString values (chars <= U+00FF). When dynamic route params contain non-ASCII characters (Korean, Japanese, etc.), JSON.stringify preserves them verbatim which causes a TypeError on the Headers.set() call in buildAppPageRscResponse. Fix: encodeURIComponent the JSON on the server side before setting the header, and decodeURIComponent on the client side when reading it back during initial hydration and client-side navigation. Closes #676
049fb18 to
edc637b
Compare
|
/bigbonk review please |
There was a problem hiding this comment.
Clean, well-scoped fix. The diagnosis is correct: Headers.set() requires ByteString values (each char ≤ U+00FF per the Fetch spec), and JSON.stringify of non-ASCII params like Korean characters produces code points well above that limit.
What I verified:
-
All read/write sites are covered. There is exactly one write site (
app-page-response.ts:172) and two read sites (app-browser-entry.ts:104for initial hydration fallback,:243for client-side navigation). All three are updated symmetrically withencodeURIComponent/decodeURIComponent. -
No other consumers exist. Grepped the entire
packages/vinext/src/tree forVinext-Params— only the three sites above. The generated entry code and prod server don't read this header directly. -
The encoding choice is appropriate.
encodeURIComponentproduces only ASCII characters (letters, digits, and percent-encoded sequences), which trivially satisfies the ByteString constraint. It round-trips cleanly throughdecodeURIComponent. -
Existing test updated correctly. The ASCII-params test now asserts the encoded form (
encodeURIComponent('{"slug":"test"}')), and the new non-ASCII test validates both the ASCII-safety invariant and the encode/decode round-trip with Korean text. -
Error handling is preserved. Both read sites already had
try/catchblocks that gracefully handle malformed headers —decodeURIComponenton a corrupted value will throw, which falls into those existing catch blocks.
LGTM.
|
Reviewed and approved PR #677. The fix is correct and complete:
|
|
Ah, I was struggling with my local setup and just uploaded the PR, but I see you already fixed it! Thanks for the quick response. I'll close my PR. |
Summary
buildAppPageRscResponsewas callingHeaders.set("X-Vinext-Params", JSON.stringify(params))with raw Unicode characters in the valueTypeError: Cannot convert argument to a ByteStringbuildAppPageHtmlResponse) never set this headerFix
encodeURIComponentwhen settingX-Vinext-Paramsinapp-page-response.tsdecodeURIComponentin both read sites inapp-browser-entry.ts(initial hydration and client-side navigation)Test
Added a unit test to
tests/app-page-response.test.tsthat verifies:Closes #676