Fix for searchParams (#817)

* fix multivalue query in searchParams for node

* fix use searchParams original encoding

* changeset

* fix middleware not using the initial query

* fix convertFromQueryString

* review

Author: conico974

.changeset/smart-ears-begin.md

@@ -0,0 +1,6 @@
+---
+"@opennextjs/aws": patch
+---
+
+fix to not decode searchParams
+fix multivalue query in searchParams for Node

packages/open-next/src/core/routing/matcher.ts

@@ -196,19 +196,16 @@ export function handleRewrites<T extends RewriteDefinition>(
     );
     // We need to use a localized path if the rewrite is not locale specific
     const pathToUse = rewrite.locale === false ? rawPath : localizedRawPath;
-    // We need to encode the "+" character with its UTF-8 equivalent "%20" to avoid 2 issues:
-    // 1. The compile function from path-to-regexp will throw an error if it finds a "+" character.
-    // https://github.com/pillarjs/path-to-regexp?tab=readme-ov-file#unexpected--or-
-    // 2. The convertToQueryString function will replace the "+" character with %2B instead of %20.
-    // %2B does not get interpreted as a space in the URL thus breaking the query string.
-    const encodePlusQueryString = queryString.replaceAll("+", "%20");
+
     debug("urlParts", { pathname, protocol, hostname, queryString });
-    const toDestinationPath = compile(escapeRegex(pathname));
+    const toDestinationPath = compile(escapeRegex(pathname, { isPath: true }));
     const toDestinationHost = compile(escapeRegex(hostname));
-    const toDestinationQuery = compile(escapeRegex(encodePlusQueryString));
+    const toDestinationQuery = compile(escapeRegex(queryString));
     const params = {
       // params for the source
-      ...getParamsFromSource(match(escapeRegex(rewrite.source)))(pathToUse),
+      ...getParamsFromSource(
+        match(escapeRegex(rewrite.source, { isPath: true })),
+      )(pathToUse),
       // params for the has
       ...rewrite.has?.reduce((acc, cur) => {
         return Object.assign(acc, computeHas(cur));
@@ -219,7 +216,7 @@ export function handleRewrites<T extends RewriteDefinition>(
       }, {}),
     };
     const isUsingParams = Object.keys(params).length > 0;
-    let rewrittenQuery = encodePlusQueryString;
+    let rewrittenQuery = queryString;
     let rewrittenHost = hostname;
     let rewrittenPath = pathname;
     if (isUsingParams) {

packages/open-next/src/core/routing/middleware.ts

@@ -10,16 +10,8 @@ import type { InternalEvent, InternalResult } from "types/open-next.js";
 import { emptyReadableStream } from "utils/stream.js";
 
 import { localizePath } from "./i18n/index.js";
-//NOTE: we should try to avoid importing stuff from next as much as possible
-// every release of next could break this
-// const { run } = require("next/dist/server/web/sandbox");
-// const { getCloneableBody } = require("next/dist/server/body-streams");
-// const {
-//   signalFromNodeResponse,
-// } = require("next/dist/server/web/spec-extension/adapters/next-request");
 import {
   convertBodyToReadableStream,
-  convertToQueryString,
   getMiddlewareMatch,
   isExternal,
 } from "./util.js";
@@ -45,14 +37,16 @@ function defaultMiddlewareLoader() {
   return import("./middleware.mjs");
 }
 
-// NOTE: As of Nextjs 13.4.13+, the middleware is handled outside the next-server.
-// OpenNext will run the middleware in a sandbox and set the appropriate req headers
-// and res.body prior to processing the next-server.
-// @returns undefined | res.end()
-
-//    if res.end() is return, the parent needs to return and not process next server
+/**
+ *
+ * @param internalEvent the internal event
+ * @param initialSearch the initial query string as it was received in the handler
+ * @param middlewareLoader Only used for unit test
+ * @returns `Promise<MiddlewareEvent | InternalResult>`
+ */
 export async function handleMiddleware(
   internalEvent: InternalEvent,
+  initialSearch: string,
   middlewareLoader: MiddlewareLoader = defaultMiddlewareLoader,
 ): Promise<MiddlewareEvent | InternalResult> {
   const headers = internalEvent.headers;
@@ -73,7 +67,7 @@ export async function handleMiddleware(
   if (!hasMatch) return internalEvent;
 
   const initialUrl = new URL(normalizedPath, internalEvent.url);
-  initialUrl.search = convertToQueryString(internalEvent.query);
+  initialUrl.search = initialSearch;
   const url = initialUrl.href;
 
   const middleware = await middlewareLoader();

packages/open-next/src/core/routing/util.ts

@@ -5,7 +5,7 @@ import { Readable } from "node:stream";
 import { BuildId, HtmlPages, NextConfig } from "config/index.js";
 import type { IncomingMessage } from "http/index.js";
 import { OpenNextNodeResponse } from "http/openNextResponse.js";
-import { parseHeaders } from "http/util.js";
+import { getQueryFromIterator, parseHeaders } from "http/util.js";
 import type {
   FunctionsConfigManifest,
   MiddlewareManifest,
@@ -38,13 +38,11 @@ export function isExternal(url?: string, host?: string) {
 export function convertFromQueryString(query: string) {
   if (query === "") return {};
   const queryParts = query.split("&");
-  return queryParts.reduce(
-    (acc, part) => {
-      const [key, value] = part.split("=");
-      acc[key] = value;
-      return acc;
-    },
-    {} as Record<string, string>,
+  return getQueryFromIterator(
+    queryParts.map((p) => {
+      const [key, value] = p.split("=");
+      return [key, value] as const;
+    }),
   );
 }
 
@@ -122,23 +120,20 @@ export function convertRes(res: OpenNextNodeResponse): InternalResult {
  * Make sure that multi-value query parameters are transformed to
  * ?key=value1&key=value2&... so that Next converts those parameters
  * to an array when reading the query parameters
+ * query should be properly encoded before using this function
  * @__PURE__
  */
 export function convertToQueryString(query: Record<string, string | string[]>) {
-  // URLSearchParams is a representation of the PARSED query.
-  // So we must decode the value before appending it to the URLSearchParams.
-  // https://stackoverflow.com/a/45516812
-  const urlQuery = new URLSearchParams();
+  const queryStrings: string[] = [];
   Object.entries(query).forEach(([key, value]) => {
     if (Array.isArray(value)) {
-      value.forEach((entry) => urlQuery.append(key, decodeURIComponent(entry)));
+      value.forEach((entry) => queryStrings.push(`${key}=${entry}`));
     } else {
-      urlQuery.append(key, decodeURIComponent(value));
+      queryStrings.push(`${key}=${value}`);
     }
   });
-  const queryString = urlQuery.toString();
 
-  return queryString ? `?${queryString}` : "";
+  return queryStrings.length > 0 ? `?${queryStrings.join("&")}` : "";
 }
 
 /**
@@ -182,11 +177,15 @@ export function getMiddlewareMatch(
  *
  * @__PURE__
  */
-export function escapeRegex(str: string) {
-  return str
+export function escapeRegex(
+  str: string,
+  { isPath }: { isPath?: boolean } = {},
+) {
+  const result = str
     .replaceAll("(.)", "_µ1_")
     .replaceAll("(..)", "_µ2_")
     .replaceAll("(...)", "_µ3_");
+  return isPath ? result : result.replaceAll("+", "_µ4_");
 }
 
 /**
@@ -197,7 +196,8 @@ export function unescapeRegex(str: string) {
   return str
     .replaceAll("_µ1_", "(.)")
     .replaceAll("_µ2_", "(..)")
-    .replaceAll("_µ3_", "(...)");
+    .replaceAll("_µ3_", "(...)")
+    .replaceAll("_µ4_", "+");
 }
 
 /**

packages/open-next/src/core/routingHandler.ts

@@ -100,7 +100,13 @@ export default async function routingHandler(
       return redirect;
     }
 
-    const eventOrResult = await handleMiddleware(internalEvent);
+    const eventOrResult = await handleMiddleware(
+      internalEvent,
+      // We need to pass the initial search without any decoding
+      // TODO: we'd need to refactor InternalEvent to include the initial querystring directly
+      // Should be done in another PR because it is a breaking change
+      new URL(event.url).search,
+    );
     const isResult = "statusCode" in eventOrResult;
     if (isResult) {
       return eventOrResult;

packages/open-next/src/http/util.ts

@@ -39,3 +39,25 @@ export function parseCookies(
     ? cookies.split(/(?<!Expires=\w+),/i).map((c) => c.trim())
     : cookies;
 }
+
+/**
+ *
+ * Get the query object from an iterable of [key, value] pairs
+ * @param it - The iterable of [key, value] pairs
+ * @returns The query object
+ */
+export function getQueryFromIterator(it: Iterable<[string, string]>) {
+  const query: Record<string, string | string[]> = {};
+  for (const [key, value] of it) {
+    if (key in query) {
+      if (Array.isArray(query[key])) {
+        query[key].push(value);
+      } else {
+        query[key] = [query[key], value];
+      }
+    } else {
+      query[key] = value;
+    }
+  }
+  return query;
+}

packages/open-next/src/overrides/converters/edge.ts

@@ -7,6 +7,7 @@ import type {
   MiddlewareResult,
 } from "types/open-next";
 import type { Converter } from "types/overrides";
+import { getQueryFromSearchParams } from "./utils.js";
 
 declare global {
   // Makes convertTo returns the request instead of fetching it.
@@ -18,18 +19,7 @@ const converter: Converter<InternalEvent, InternalResult | MiddlewareResult> = {
     const url = new URL(event.url);
 
     const searchParams = url.searchParams;
-    const query: Record<string, string | string[]> = {};
-    for (const [key, value] of searchParams.entries()) {
-      if (key in query) {
-        if (Array.isArray(query[key])) {
-          query[key].push(value);
-        } else {
-          query[key] = [query[key], value];
-        }
-      } else {
-        query[key] = value;
-      }
-    }
+    const query = getQueryFromSearchParams(searchParams);
     // Transform body into Buffer
     const body = await event.arrayBuffer();
     const headers: Record<string, string> = {};

packages/open-next/src/overrides/converters/node.ts

@@ -3,7 +3,7 @@ import type { IncomingMessage } from "node:http";
 import { parseCookies } from "http/util";
 import type { InternalResult } from "types/open-next";
 import type { Converter } from "types/overrides";
-import { extractHostFromHeaders } from "./utils";
+import { extractHostFromHeaders, getQueryFromSearchParams } from "./utils.js";
 
 const converter: Converter = {
   convertFrom: async (req: IncomingMessage) => {
@@ -26,7 +26,7 @@ const converter: Converter = {
         .filter(([key]) => key),
     );
     const url = new URL(req.url!, `http://${extractHostFromHeaders(headers)}`);
-    const query = Object.fromEntries(url.searchParams.entries());
+    const query = getQueryFromSearchParams(url.searchParams);
     return {
       type: "core",
       method: req.method ?? "GET",

packages/open-next/src/overrides/converters/utils.ts

@@ -1,3 +1,5 @@
+import { getQueryFromIterator } from "http/util.js";
+
 export function removeUndefinedFromQuery(
   query: Record<string, string | string[] | undefined>,
 ) {
@@ -21,3 +23,13 @@ export function extractHostFromHeaders(
 ): string {
   return headers["x-forwarded-host"] ?? headers.host ?? "on";
 }
+
+/**
+ * Get the query object from an URLSearchParams
+ *
+ * @param searchParams
+ * @returns
+ */
+export function getQueryFromSearchParams(searchParams: URLSearchParams) {
+  return getQueryFromIterator(searchParams.entries());
+}