"TypeError: Cannot convert argument to a ByteString" when giving multibyte product name in header

nobu-shopify commented 1 year ago

What is the location of your example repository?

https://github.com/nobu-shopify/hydrogen-demo-customerapi-2023-07-28

Which package or tool is having this issue?

Hydrogen

What version of that package or tool are you using?

"@shopify/hydrogen": "^2023.7.0", "@shopify/remix-oxygen": "^1.1.1",

What version of Remix are you using?

"@remix-run/react": "1.19.1",

Steps to Reproduce

Open Hydrogen demo store - https://hydrogen-demo-customerapi-2023-07-28-ff4270a546f444d678ae.o2.myshopify.dev/
Click "一期一会 (ICHIGO ICHIE)" <= the beer with yellow label

Expected Behavior

The product page should be displayed, like this 一期一会 (ICHIGO ICHIE)

Actual Behavior

The following error is shown:

We’ve lost this product We couldn’t find the product you’re looking for. Try checking the URL or heading back to the home page.
URL shows corrupted characters in multibyte part:

https://hydrogen-demo-customerapi-2023-07-28-ff4270a546f444d678ae.o2.myshopify.dev/products/%C3%A4%C2%B8%C2%80%C3%A6%C2%9C%C2%9F%C3%A4%C2%B8%C2%80%C3%A4%C2%BC%C2%9A-ichigo-ichie?Title=Default+Title
Doing the same repro steps in my local env, the following error is observed:

TypeError: Cannot convert argument to a ByteString because the character at index 10 has a value of 20908 which is greater than 255. at Object.webidl.converters.ByteString (/Users/nobu/dev/shopify/hydrogen/hydrogen-demo-customerapi-2023-07-28/node_modules/undici/lib/fetch/webidl.js:426:13) at Headers.set (/Users/nobu/dev/shopify/hydrogen/hydrogen-demo-customerapi-2023-07-28/node_modules/undici/lib/fetch/headers.js:348:31) at redirect (/Users/nobu/dev/shopify/hydrogen/hydrogen-demo-customerapi-2023-07-28/node_modules/@remix-run/router/utils.ts:1479:11) at redirect (/Users/nobu/dev/shopify/hydrogen/hydrogen-demo-customerapi-2023-07-28/node_modules/@remix-run/server-runtime/dist/esm/responses.js:41:10) at redirectToFirstVariant (/Users/nobu/dev/shopify/hydrogen/hydrogen-demo-customerapi-2023-07-28/app/routes/($locale).products.$productHandle.jsx:113:9) at loader4 (/Users/nobu/dev/shopify/hydrogen/hydrogen-demo-customerapi-2023-07-28/app/routes/($locale).products.$productHandle.jsx:68:12) at processTicksAndRejections (node:internal/process/task_queues:95:5) at callRouteLoaderRR (/Users/nobu/dev/shopify/hydrogen/hydrogen-demo-customerapi-2023-07-28/node_modules/@remix-run/server-runtime/dist/esm/data.js:48:16) at callLoaderOrAction (/Users/nobu/dev/shopify/hydrogen/hydrogen-demo-customerapi-2023-07-28/node_modules/@remix-run/router/router.ts:3671:16) at async Promise.all (index 0) GET 500 loader /products/%E5%86%AC%E3%81%AE%E6%B0%97%E3%81%BE%E3%81%90%E3%82%8C-2022-fuyu-no-kimagure-2022 [($locale).products.$productHandle] (prefetch)

Looking at https://github.com/nodejs/undici/issues/1590, it appears that we are giving Unicode header to node_modules/undici/lib/fetch/headers.js

wizardlyhel commented 1 year ago

Thank you for finding this bug

wizardlyhel commented 1 year ago

@nobu-shopify I am assumming you are using the demo store project as a base.

In the function redirectToFirstVariant, update the redirect url path so that it is url encoded.

function redirectToFirstVariant({
  product,
  request,
}: {
  product: ProductQuery['product'];
  request: Request;
}) {
  const searchParams = new URLSearchParams(new URL(request.url).search);
  const firstVariant = product!.variants.nodes[0];
  for (const option of firstVariant.selectedOptions) {
    searchParams.set(option.name, option.value);
  }

-  throw redirect(
-    `/products/${product!.handle}?${searchParams.toString()}`,
-    302,
-  );

+  // Use URL to avoid accidental double encoding
+  const newUrl = new URL(`/products/${product!.handle}?${searchParams.toString()}`, 'http://example.com');
+  throw redirect(newUrl.pathname + newUrl.search, 302);
}

We'll update demo store code to reflect this change as well.

nobu-shopify commented 1 year ago

Thanks @wizardlyhel !!

Verified here that your proposed fix works on my Hydrogen demostore JS code, after removing non-null assertion in $(product!.handle) (which appears TS-only feature):

  // Fix from https://github.com/Shopify/hydrogen/issues/1209
  // Use URL to avoid accidental double encoding
  const newUrl = new URL(`/products/${product.handle}?${searchParams.toString()}`, 'http://example.com');
  throw redirect(newUrl.pathname + newUrl.search, 302);

Shopify / hydrogen