diff --git a/.changeset/smart-ears-begin.md b/.changeset/smart-ears-begin.md new file mode 100644 index 000000000..2d1de0e6e --- /dev/null +++ b/.changeset/smart-ears-begin.md @@ -0,0 +1,6 @@ +--- +"@opennextjs/aws": patch +--- + +fix to not decode searchParams +fix multivalue query in searchParams for Node \ No newline at end of file diff --git a/packages/open-next/src/core/routing/matcher.ts b/packages/open-next/src/core/routing/matcher.ts index d5ffc1418..24748634a 100644 --- a/packages/open-next/src/core/routing/matcher.ts +++ b/packages/open-next/src/core/routing/matcher.ts @@ -196,19 +196,16 @@ export function handleRewrites( ); // 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( }, {}), }; const isUsingParams = Object.keys(params).length > 0; - let rewrittenQuery = encodePlusQueryString; + let rewrittenQuery = queryString; let rewrittenHost = hostname; let rewrittenPath = pathname; if (isUsingParams) { diff --git a/packages/open-next/src/core/routing/middleware.ts b/packages/open-next/src/core/routing/middleware.ts index b6485cab0..e79e793ab 100644 --- a/packages/open-next/src/core/routing/middleware.ts +++ b/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` + */ export async function handleMiddleware( internalEvent: InternalEvent, + initialSearch: string, middlewareLoader: MiddlewareLoader = defaultMiddlewareLoader, ): Promise { 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(); diff --git a/packages/open-next/src/core/routing/util.ts b/packages/open-next/src/core/routing/util.ts index a97953a33..70ae53027 100644 --- a/packages/open-next/src/core/routing/util.ts +++ b/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, + 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) { - // 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_", "+"); } /** diff --git a/packages/open-next/src/core/routingHandler.ts b/packages/open-next/src/core/routingHandler.ts index 3be5bcfb2..ed3f926e1 100644 --- a/packages/open-next/src/core/routingHandler.ts +++ b/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; diff --git a/packages/open-next/src/http/util.ts b/packages/open-next/src/http/util.ts index 5e08c5c40..af5bb2738 100644 --- a/packages/open-next/src/http/util.ts +++ b/packages/open-next/src/http/util.ts @@ -39,3 +39,25 @@ export function parseCookies( ? cookies.split(/(? 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 = {}; + 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; +} diff --git a/packages/open-next/src/overrides/converters/edge.ts b/packages/open-next/src/overrides/converters/edge.ts index 7ac5983a8..bc1f2f677 100644 --- a/packages/open-next/src/overrides/converters/edge.ts +++ b/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 = { const url = new URL(event.url); const searchParams = url.searchParams; - const query: Record = {}; - 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 = {}; diff --git a/packages/open-next/src/overrides/converters/node.ts b/packages/open-next/src/overrides/converters/node.ts index ef9cf9be5..3eed47f2c 100644 --- a/packages/open-next/src/overrides/converters/node.ts +++ b/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", diff --git a/packages/open-next/src/overrides/converters/utils.ts b/packages/open-next/src/overrides/converters/utils.ts index 176ce298b..252b10975 100644 --- a/packages/open-next/src/overrides/converters/utils.ts +++ b/packages/open-next/src/overrides/converters/utils.ts @@ -1,3 +1,5 @@ +import { getQueryFromIterator } from "http/util.js"; + export function removeUndefinedFromQuery( query: Record, ) { @@ -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()); +} diff --git a/packages/tests-unit/tests/core/routing/middleware.test.ts b/packages/tests-unit/tests/core/routing/middleware.test.ts index 149b887ff..fdbedf213 100644 --- a/packages/tests-unit/tests/core/routing/middleware.test.ts +++ b/packages/tests-unit/tests/core/routing/middleware.test.ts @@ -84,7 +84,7 @@ describe("handleMiddleware", () => { "x-prerender-revalidate": "preview", }, }); - const result = await handleMiddleware(event, middlewareLoader); + const result = await handleMiddleware(event, "", middlewareLoader); expect(middlewareLoader).not.toHaveBeenCalled(); expect(result).toEqual(event); @@ -103,7 +103,7 @@ describe("handleMiddleware", () => { location: "/redirect", }), }); - const result = await handleMiddleware(event, middlewareLoader); + const result = await handleMiddleware(event, "", middlewareLoader); expect(middlewareLoader).toHaveBeenCalled(); expect(result.statusCode).toEqual(302); @@ -122,7 +122,7 @@ describe("handleMiddleware", () => { location: "/redirect", }), }); - const result = await handleMiddleware(event, middlewareLoader); + const result = await handleMiddleware(event, "", middlewareLoader); expect(middlewareLoader).toHaveBeenCalled(); expect(result.statusCode).toEqual(302); @@ -137,7 +137,7 @@ describe("handleMiddleware", () => { location: "/redirect", }), }); - const result = await handleMiddleware(event, middlewareLoader); + const result = await handleMiddleware(event, "", middlewareLoader); expect(middlewareLoader).toHaveBeenCalled(); expect(result.statusCode).toEqual(302); @@ -152,7 +152,7 @@ describe("handleMiddleware", () => { location: "http://external/redirect", }), }); - const result = await handleMiddleware(event, middlewareLoader); + const result = await handleMiddleware(event, "", middlewareLoader); expect(middlewareLoader).toHaveBeenCalled(); expect(result.statusCode).toEqual(302); @@ -170,7 +170,7 @@ describe("handleMiddleware", () => { "x-middleware-rewrite": "http://localhost/rewrite", }), }); - const result = await handleMiddleware(event, middlewareLoader); + const result = await handleMiddleware(event, "", middlewareLoader); expect(middlewareLoader).toHaveBeenCalled(); expect(result).toEqual({ @@ -196,7 +196,7 @@ describe("handleMiddleware", () => { "x-middleware-rewrite": "http://localhost/rewrite?newKey=value", }), }); - const result = await handleMiddleware(event, middlewareLoader); + const result = await handleMiddleware(event, "", middlewareLoader); expect(middlewareLoader).toHaveBeenCalled(); expect(result).toEqual({ @@ -225,7 +225,7 @@ describe("handleMiddleware", () => { "x-middleware-rewrite": "http://external/rewrite", }), }); - const result = await handleMiddleware(event, middlewareLoader); + const result = await handleMiddleware(event, "", middlewareLoader); expect(middlewareLoader).toHaveBeenCalled(); expect(result).toEqual({ @@ -246,7 +246,7 @@ describe("handleMiddleware", () => { "x-middleware-request-custom-header": "value", }), }); - const result = await handleMiddleware(event, middlewareLoader); + const result = await handleMiddleware(event, "", middlewareLoader); expect(middlewareLoader).toHaveBeenCalled(); expect(result).toEqual({ @@ -268,7 +268,7 @@ describe("handleMiddleware", () => { headers: new Headers(), body, }); - const result = await handleMiddleware(event, middlewareLoader); + const result = await handleMiddleware(event, "", middlewareLoader); expect(middlewareLoader).toHaveBeenCalled(); expect(result).toEqual({ @@ -291,7 +291,7 @@ describe("handleMiddleware", () => { }), body, }); - const result = await handleMiddleware(event, middlewareLoader); + const result = await handleMiddleware(event, "", middlewareLoader); expect(middlewareLoader).toHaveBeenCalled(); expect(result).toEqual({ @@ -312,7 +312,7 @@ describe("handleMiddleware", () => { host: "test.me", }, }); - await handleMiddleware(event, middlewareLoader); + await handleMiddleware(event, "", middlewareLoader); expect(middleware).toHaveBeenCalledWith( expect.objectContaining({ url: "http://test.me/path", @@ -327,7 +327,7 @@ describe("handleMiddleware", () => { host: "test.me/path", }, }); - await handleMiddleware(event, middlewareLoader); + await handleMiddleware(event, "", middlewareLoader); expect(middleware).toHaveBeenCalledWith( expect.objectContaining({ url: "https://test.me/path", @@ -342,11 +342,30 @@ describe("handleMiddleware", () => { host: "test.me", }, }); - await handleMiddleware(event, middlewareLoader); + await handleMiddleware(event, "", middlewareLoader); expect(middleware).toHaveBeenCalledWith( expect.objectContaining({ url: "https://test.me/path", }), ); }); + + it("should use the initial search query", async () => { + const event = createEvent({ + url: "https://test.me/path?something=General%2520Banner", + headers: { + host: "test.me", + }, + }); + await handleMiddleware( + event, + "?something=General%2520Banner", + middlewareLoader, + ); + expect(middleware).toHaveBeenCalledWith( + expect.objectContaining({ + url: "https://test.me/path?something=General%2520Banner", + }), + ); + }); }); diff --git a/packages/tests-unit/tests/core/routing/util.test.ts b/packages/tests-unit/tests/core/routing/util.test.ts index 0a2ab151d..908057499 100644 --- a/packages/tests-unit/tests/core/routing/util.test.ts +++ b/packages/tests-unit/tests/core/routing/util.test.ts @@ -107,9 +107,9 @@ describe("convertFromQueryString", () => { }); }); - it("converts query string with multiple keys (last one wins)", () => { + it("converts query string with multiple keys", () => { expect(convertFromQueryString("search=value&search=other")).toEqual({ - search: "other", + search: ["value", "other"], }); }); }); @@ -308,6 +308,16 @@ describe("convertToQueryString", () => { "?key=value1&key=value2&another=value3", ); }); + + it("should respect existing query encoding", () => { + const query = { + key: ["value%201", "value2+something+else"], + another: "value3", + }; + expect(convertToQueryString(query)).toBe( + "?key=value%201&key=value2+something+else&another=value3", + ); + }); }); describe("convertToQuery", () => { @@ -383,13 +393,18 @@ describe("regex", () => { ["/a/b/(..)(..)c", "/a/b/_µ2__µ2_c"], ["/a/(...)b", "/a/_µ3_b"], ["/feed/(..)photo/[id]", "/feed/_µ2_photo/[id]"], + ["?something=a+b", "?something=a_µ4_b"], ])( - "should escape (.), (..), (...) with _µ1_, _µ2_, _µ3_ - %s", + "should escape (.), (..), (...), + with _µ1_, _µ2_, _µ3_, _µ4_ - %s", (input, expected) => { const result = escapeRegex(input); expect(result).toBe(expected); }, ); + + it("should not escape plus for a path", () => { + expect(escapeRegex("/:path+", { isPath: true })).toBe("/:path+"); + }); }); describe("unescapeRegex", () => { @@ -399,6 +414,7 @@ describe("regex", () => { ["/a/b/_µ2__µ2_c", "/a/b/(..)(..)c"], ["/a/_µ3_b", "/a/(...)b"], ["/feed/_µ2_photo/[id]", "/feed/(..)photo/[id]"], + ["?something=a_µ4_b", "?something=a+b"], ])( "should unescape _µ1_, _µ2_, _µ3_ with (.), (..), (...) - %s", (input, expected) => {