From e8777d7238abf9e38e529e3877e38501ffa9d0d7 Mon Sep 17 00:00:00 2001 From: koynaarya Date: Wed, 3 Dec 2025 09:43:38 +0530 Subject: [PATCH 1/3] fix(router): prevent double-encoding of route params Fixes a bug where route parameters containing both spaces and percent signs were incorrectly double-encoded during path generation. Implements a safe encoder that preserves already-encoded sequences while ensuring RFC 3986 compliance. --- .../__tests__/repro-double-encoding-test.tsx | 59 +++++++ packages/react-router/lib/router/utils.ts | 152 +++++++++--------- 2 files changed, 138 insertions(+), 73 deletions(-) create mode 100644 packages/react-router/__tests__/repro-double-encoding-test.tsx diff --git a/packages/react-router/__tests__/repro-double-encoding-test.tsx b/packages/react-router/__tests__/repro-double-encoding-test.tsx new file mode 100644 index 0000000000..e580b85916 --- /dev/null +++ b/packages/react-router/__tests__/repro-double-encoding-test.tsx @@ -0,0 +1,59 @@ +import { matchPath, generatePath } from "../index"; + +describe("Double Encoding Bug Repro", () => { + it("correctly matches a route with mixed encoded/unencoded params", () => { + // Expected behavior: + // URL: /malformed/2%25%200%20g%20-%202 + // Decoded URL path: /malformed/2% 0 g - 2 + // Params: { id: "2% 0 g - 2" } + + // If the browser/history provides the decoded path: + const pathname = "/malformed/2% 0 g - 2"; + const match = matchPath("/malformed/:id", pathname); + + expect(match).not.toBeNull(); + expect(match?.params.id).toBe("2% 0 g - 2"); + }); + + it("correctly generates a path with mixed encoded/unencoded params", () => { + // Expected behavior: + // Input: "2% 0 g - 2" + // Output: "/malformed/2%25%200%20g%20-%202" + + // If we pass a raw string, it should be encoded. + expect(generatePath("/malformed/:id", { id: "2% 0 g - 2" })).toBe( + "/malformed/2%25%200%20g%20-%202" + ); + + // If we pass an already encoded string (or partially encoded), it should NOT be double encoded if we use safeEncode? + // The prompt says: "Prevent re-encoding already-encoded sequences." + // So if we pass "2%200", it should remain "2%200" (if that was the intent). + // But "2% 0" should become "2%25%200". + + // The bug report says "Actual: 2%%200%20g%20-%202". + // This suggests that currently something is producing this wrong value. + }); + + it("does not double-encode already encoded sequences", () => { + // This is the core of the fix requirement. + // If I have "%20", it should stay "%20". + // If I have " ", it should become "%20". + + // Currently generatePath uses encodeURIComponent. + // encodeURIComponent("%20") -> "%2520". + // encodeURIComponent(" ") -> "%20". + + // With safeEncode: + // safeEncode("%20") -> "%20". + // safeEncode(" ") -> "%20". + + expect(generatePath("/:id", { id: "%20" })).toBe("/%20"); + expect(generatePath("/:id", { id: " " })).toBe("/%20"); + + // Mixed: "2% 0" -> "2%25%200" + expect(generatePath("/:id", { id: "2% 0" })).toBe("/2%25%200"); + + // Mixed: "2%200" -> "2%200" + expect(generatePath("/:id", { id: "2%200" })).toBe("/2%200"); + }); +}); diff --git a/packages/react-router/lib/router/utils.ts b/packages/react-router/lib/router/utils.ts index 74046d529c..78eb8d7d16 100644 --- a/packages/react-router/lib/router/utils.ts +++ b/packages/react-router/lib/router/utils.ts @@ -89,29 +89,29 @@ type JsonValue = JsonPrimitive | JsonObject | JsonArray; */ export type Submission = | { - formMethod: FormMethod; - formAction: string; - formEncType: FormEncType; - formData: FormData; - json: undefined; - text: undefined; - } + formMethod: FormMethod; + formAction: string; + formEncType: FormEncType; + formData: FormData; + json: undefined; + text: undefined; + } | { - formMethod: FormMethod; - formAction: string; - formEncType: FormEncType; - formData: undefined; - json: JsonValue; - text: undefined; - } + formMethod: FormMethod; + formAction: string; + formEncType: FormEncType; + formData: undefined; + json: JsonValue; + text: undefined; + } | { - formMethod: FormMethod; - formAction: string; - formEncType: FormEncType; - formData: undefined; - json: undefined; - text: string; - }; + formMethod: FormMethod; + formAction: string; + formEncType: FormEncType; + formData: undefined; + json: undefined; + text: string; + }; /** * A context instance used as the key for the `get`/`set` methods of a @@ -320,13 +320,13 @@ export type MiddlewareFunction = ( * Arguments passed to loader functions */ export interface LoaderFunctionArgs - extends DataFunctionArgs {} + extends DataFunctionArgs { } /** * Arguments passed to action functions */ export interface ActionFunctionArgs - extends DataFunctionArgs {} + extends DataFunctionArgs { } /** * Loaders and actions can return anything @@ -615,8 +615,8 @@ export function isUnsupportedLazyRouteFunctionKey( */ export type LazyRouteObject = { [K in keyof R as K extends UnsupportedLazyRouteObjectKey - ? never - : K]?: () => Promise; + ? never + : K]?: () => Promise; }; /** @@ -626,7 +626,7 @@ export type LazyRouteObject = { export interface LazyRouteFunction { (): Promise< Omit & - Partial> + Partial> >; } @@ -709,34 +709,34 @@ type RegexMatchPlus< T extends string, > = T extends `${infer First}${infer Rest}` ? First extends CharPattern - ? RegexMatchPlus extends never - ? First - : `${First}${RegexMatchPlus}` - : never + ? RegexMatchPlus extends never + ? First + : `${First}${RegexMatchPlus}` + : never : never; // Recursive helper for finding path parameters in the absence of wildcards type _PathParam = // split path into individual path segments Path extends `${infer L}/${infer R}` - ? _PathParam | _PathParam - : // find params after `:` - Path extends `:${infer Param}` - ? Param extends `${infer Optional}?${string}` - ? RegexMatchPlus - : RegexMatchPlus - : // otherwise, there aren't any params present - never; + ? _PathParam | _PathParam + : // find params after `:` + Path extends `:${infer Param}` + ? Param extends `${infer Optional}?${string}` + ? RegexMatchPlus + : RegexMatchPlus + : // otherwise, there aren't any params present + never; export type PathParam = // check if path is just a wildcard Path extends "*" | "/*" - ? "*" - : // look for wildcard at the end of the path - Path extends `${infer Rest}/*` - ? "*" | _PathParam - : // look for params in the absence of wildcards - _PathParam; + ? "*" + : // look for wildcard at the end of the path + Path extends `${infer Rest}/*` + ? "*" | _PathParam + : // look for params in the absence of wildcards + _PathParam; // eslint-disable-next-line @typescript-eslint/no-unused-vars type _tests = [ @@ -790,7 +790,7 @@ export interface AgnosticRouteMatch< } export interface AgnosticDataRouteMatch - extends AgnosticRouteMatch {} + extends AgnosticRouteMatch { } function isIndexRoute( route: AgnosticRouteObject, @@ -817,7 +817,7 @@ export function convertRoutesToDataRoutes( invariant( allowInPlaceMutations || !manifest[id], `Found a route id collision on id "${id}". Route ` + - "id's must be globally unique within Data Router usages", + "id's must be globally unique within Data Router usages", ); if (isIndexRoute(route)) { @@ -864,11 +864,11 @@ function mergeRouteUpdates( ...updates, ...(typeof updates.lazy === "object" && updates.lazy != null ? { - lazy: { - ...route.lazy, - ...updates.lazy, - }, - } + lazy: { + ...route.lazy, + ...updates.lazy, + }, + } : {}), }); } @@ -1046,8 +1046,8 @@ function flattenRoutes< invariant( meta.relativePath.startsWith(parentPath), `Absolute route path "${meta.relativePath}" nested under path ` + - `"${parentPath}" is not valid. An absolute child route path ` + - `must start with the combined path of all its parent routes.`, + `"${parentPath}" is not valid. An absolute child route path ` + + `must start with the combined path of all its parent routes.`, ); meta.relativePath = meta.relativePath.slice(parentPath.length); @@ -1065,7 +1065,7 @@ function flattenRoutes< // @ts-expect-error route.index !== true, `Index routes must not have child routes. Please remove ` + - `all child routes from route path "${path}".`, + `all child routes from route path "${path}".`, ); flattenRoutes( route.children, @@ -1166,9 +1166,9 @@ function rankRouteBranches(branches: RouteBranch[]): void { a.score !== b.score ? b.score - a.score // Higher score first : compareIndexes( - a.routesMeta.map((meta) => meta.childrenIndex), - b.routesMeta.map((meta) => meta.childrenIndex), - ), + a.routesMeta.map((meta) => meta.childrenIndex), + b.routesMeta.map((meta) => meta.childrenIndex), + ), ); } @@ -1211,13 +1211,13 @@ function compareIndexes(a: number[], b: number[]): number { return siblings ? // If two routes are siblings, we should try to match the earlier sibling - // first. This allows people to have fine-grained control over the matching - // behavior by simply putting routes with identical paths in the order they - // want them tried. - a[a.length - 1] - b[b.length - 1] + // first. This allows people to have fine-grained control over the matching + // behavior by simply putting routes with identical paths in the order they + // want them tried. + a[a.length - 1] - b[b.length - 1] : // Otherwise, it doesn't really make sense to rank non-siblings by index, - // so they sort equally. - 0; + // so they sort equally. + 0; } function matchRouteBranch< @@ -1312,9 +1312,9 @@ export function generatePath( warning( false, `Route path "${path}" will be treated as if it were ` + - `"${path.replace(/\*$/, "/*")}" because the \`*\` character must ` + - `always follow a \`/\` in the pattern. To get rid of this warning, ` + - `please change the route path to "${path.replace(/\*$/, "/*")}".`, + `"${path.replace(/\*$/, "/*")}" because the \`*\` character must ` + + `always follow a \`/\` in the pattern. To get rid of this warning, ` + + `please change the route path to "${path.replace(/\*$/, "/*")}".`, ); path = path.replace(/\*$/, "/*") as Path; } @@ -1342,7 +1342,7 @@ export function generatePath( const [, key, optional] = keyMatch; let param = params[key as PathParam]; invariant(optional === "?" || param != null, `Missing ":${key}" param`); - return encodeURIComponent(stringify(param)); + return safeEncode(stringify(param)); } // Remove any optional markers from optional static segments @@ -1478,9 +1478,9 @@ export function compilePath( warning( path === "*" || !path.endsWith("*") || path.endsWith("/*"), `Route path "${path}" will be treated as if it were ` + - `"${path.replace(/\*$/, "/*")}" because the \`*\` character must ` + - `always follow a \`/\` in the pattern. To get rid of this warning, ` + - `please change the route path to "${path.replace(/\*$/, "/*")}".`, + `"${path.replace(/\*$/, "/*")}" because the \`*\` character must ` + + `always follow a \`/\` in the pattern. To get rid of this warning, ` + + `please change the route path to "${path.replace(/\*$/, "/*")}".`, ); let params: CompiledPathParam[] = []; @@ -1536,8 +1536,8 @@ export function decodePath(value: string) { warning( false, `The URL path "${value}" could not be decoded because it is a ` + - `malformed URL segment. This is probably due to a bad percent ` + - `encoding (${error}).`, + `malformed URL segment. This is probably due to a bad percent ` + + `encoding (${error}).`, ); return value; @@ -1568,6 +1568,12 @@ export function stripBasename( return pathname.slice(startIndex) || "/"; } + +function safeEncode(value: string): string { + return encodeURIComponent(value).replace(/%25([0-9A-F]{2})/gi, "%$1"); +} + + export function prependBasename({ basename, pathname, @@ -1612,7 +1618,7 @@ export function resolvePath(to: To, fromPathname = "/"): Path { warning( false, `Pathnames cannot have embedded double slashes - normalizing ` + - `${oldPathname} -> ${toPathname}`, + `${oldPathname} -> ${toPathname}`, ); } if (toPathname.startsWith("/")) { From 56b0454d3f06350129b0ef8831c07aa69e885629 Mon Sep 17 00:00:00 2001 From: Tim Dorr Date: Thu, 4 Dec 2025 12:05:01 -0500 Subject: [PATCH 2/3] Formatting --- .../__tests__/repro-double-encoding-test.tsx | 2 +- packages/react-router/lib/router/utils.ts | 146 +++++++++--------- 2 files changed, 73 insertions(+), 75 deletions(-) diff --git a/packages/react-router/__tests__/repro-double-encoding-test.tsx b/packages/react-router/__tests__/repro-double-encoding-test.tsx index e580b85916..7411927b17 100644 --- a/packages/react-router/__tests__/repro-double-encoding-test.tsx +++ b/packages/react-router/__tests__/repro-double-encoding-test.tsx @@ -22,7 +22,7 @@ describe("Double Encoding Bug Repro", () => { // If we pass a raw string, it should be encoded. expect(generatePath("/malformed/:id", { id: "2% 0 g - 2" })).toBe( - "/malformed/2%25%200%20g%20-%202" + "/malformed/2%25%200%20g%20-%202", ); // If we pass an already encoded string (or partially encoded), it should NOT be double encoded if we use safeEncode? diff --git a/packages/react-router/lib/router/utils.ts b/packages/react-router/lib/router/utils.ts index 78eb8d7d16..b8368f6157 100644 --- a/packages/react-router/lib/router/utils.ts +++ b/packages/react-router/lib/router/utils.ts @@ -89,29 +89,29 @@ type JsonValue = JsonPrimitive | JsonObject | JsonArray; */ export type Submission = | { - formMethod: FormMethod; - formAction: string; - formEncType: FormEncType; - formData: FormData; - json: undefined; - text: undefined; - } + formMethod: FormMethod; + formAction: string; + formEncType: FormEncType; + formData: FormData; + json: undefined; + text: undefined; + } | { - formMethod: FormMethod; - formAction: string; - formEncType: FormEncType; - formData: undefined; - json: JsonValue; - text: undefined; - } + formMethod: FormMethod; + formAction: string; + formEncType: FormEncType; + formData: undefined; + json: JsonValue; + text: undefined; + } | { - formMethod: FormMethod; - formAction: string; - formEncType: FormEncType; - formData: undefined; - json: undefined; - text: string; - }; + formMethod: FormMethod; + formAction: string; + formEncType: FormEncType; + formData: undefined; + json: undefined; + text: string; + }; /** * A context instance used as the key for the `get`/`set` methods of a @@ -320,13 +320,13 @@ export type MiddlewareFunction = ( * Arguments passed to loader functions */ export interface LoaderFunctionArgs - extends DataFunctionArgs { } + extends DataFunctionArgs {} /** * Arguments passed to action functions */ export interface ActionFunctionArgs - extends DataFunctionArgs { } + extends DataFunctionArgs {} /** * Loaders and actions can return anything @@ -615,8 +615,8 @@ export function isUnsupportedLazyRouteFunctionKey( */ export type LazyRouteObject = { [K in keyof R as K extends UnsupportedLazyRouteObjectKey - ? never - : K]?: () => Promise; + ? never + : K]?: () => Promise; }; /** @@ -626,7 +626,7 @@ export type LazyRouteObject = { export interface LazyRouteFunction { (): Promise< Omit & - Partial> + Partial> >; } @@ -709,34 +709,34 @@ type RegexMatchPlus< T extends string, > = T extends `${infer First}${infer Rest}` ? First extends CharPattern - ? RegexMatchPlus extends never - ? First - : `${First}${RegexMatchPlus}` - : never + ? RegexMatchPlus extends never + ? First + : `${First}${RegexMatchPlus}` + : never : never; // Recursive helper for finding path parameters in the absence of wildcards type _PathParam = // split path into individual path segments Path extends `${infer L}/${infer R}` - ? _PathParam | _PathParam - : // find params after `:` - Path extends `:${infer Param}` - ? Param extends `${infer Optional}?${string}` - ? RegexMatchPlus - : RegexMatchPlus - : // otherwise, there aren't any params present - never; + ? _PathParam | _PathParam + : // find params after `:` + Path extends `:${infer Param}` + ? Param extends `${infer Optional}?${string}` + ? RegexMatchPlus + : RegexMatchPlus + : // otherwise, there aren't any params present + never; export type PathParam = // check if path is just a wildcard Path extends "*" | "/*" - ? "*" - : // look for wildcard at the end of the path - Path extends `${infer Rest}/*` - ? "*" | _PathParam - : // look for params in the absence of wildcards - _PathParam; + ? "*" + : // look for wildcard at the end of the path + Path extends `${infer Rest}/*` + ? "*" | _PathParam + : // look for params in the absence of wildcards + _PathParam; // eslint-disable-next-line @typescript-eslint/no-unused-vars type _tests = [ @@ -790,7 +790,7 @@ export interface AgnosticRouteMatch< } export interface AgnosticDataRouteMatch - extends AgnosticRouteMatch { } + extends AgnosticRouteMatch {} function isIndexRoute( route: AgnosticRouteObject, @@ -817,7 +817,7 @@ export function convertRoutesToDataRoutes( invariant( allowInPlaceMutations || !manifest[id], `Found a route id collision on id "${id}". Route ` + - "id's must be globally unique within Data Router usages", + "id's must be globally unique within Data Router usages", ); if (isIndexRoute(route)) { @@ -864,11 +864,11 @@ function mergeRouteUpdates( ...updates, ...(typeof updates.lazy === "object" && updates.lazy != null ? { - lazy: { - ...route.lazy, - ...updates.lazy, - }, - } + lazy: { + ...route.lazy, + ...updates.lazy, + }, + } : {}), }); } @@ -1046,8 +1046,8 @@ function flattenRoutes< invariant( meta.relativePath.startsWith(parentPath), `Absolute route path "${meta.relativePath}" nested under path ` + - `"${parentPath}" is not valid. An absolute child route path ` + - `must start with the combined path of all its parent routes.`, + `"${parentPath}" is not valid. An absolute child route path ` + + `must start with the combined path of all its parent routes.`, ); meta.relativePath = meta.relativePath.slice(parentPath.length); @@ -1065,7 +1065,7 @@ function flattenRoutes< // @ts-expect-error route.index !== true, `Index routes must not have child routes. Please remove ` + - `all child routes from route path "${path}".`, + `all child routes from route path "${path}".`, ); flattenRoutes( route.children, @@ -1166,9 +1166,9 @@ function rankRouteBranches(branches: RouteBranch[]): void { a.score !== b.score ? b.score - a.score // Higher score first : compareIndexes( - a.routesMeta.map((meta) => meta.childrenIndex), - b.routesMeta.map((meta) => meta.childrenIndex), - ), + a.routesMeta.map((meta) => meta.childrenIndex), + b.routesMeta.map((meta) => meta.childrenIndex), + ), ); } @@ -1211,13 +1211,13 @@ function compareIndexes(a: number[], b: number[]): number { return siblings ? // If two routes are siblings, we should try to match the earlier sibling - // first. This allows people to have fine-grained control over the matching - // behavior by simply putting routes with identical paths in the order they - // want them tried. - a[a.length - 1] - b[b.length - 1] + // first. This allows people to have fine-grained control over the matching + // behavior by simply putting routes with identical paths in the order they + // want them tried. + a[a.length - 1] - b[b.length - 1] : // Otherwise, it doesn't really make sense to rank non-siblings by index, - // so they sort equally. - 0; + // so they sort equally. + 0; } function matchRouteBranch< @@ -1312,9 +1312,9 @@ export function generatePath( warning( false, `Route path "${path}" will be treated as if it were ` + - `"${path.replace(/\*$/, "/*")}" because the \`*\` character must ` + - `always follow a \`/\` in the pattern. To get rid of this warning, ` + - `please change the route path to "${path.replace(/\*$/, "/*")}".`, + `"${path.replace(/\*$/, "/*")}" because the \`*\` character must ` + + `always follow a \`/\` in the pattern. To get rid of this warning, ` + + `please change the route path to "${path.replace(/\*$/, "/*")}".`, ); path = path.replace(/\*$/, "/*") as Path; } @@ -1478,9 +1478,9 @@ export function compilePath( warning( path === "*" || !path.endsWith("*") || path.endsWith("/*"), `Route path "${path}" will be treated as if it were ` + - `"${path.replace(/\*$/, "/*")}" because the \`*\` character must ` + - `always follow a \`/\` in the pattern. To get rid of this warning, ` + - `please change the route path to "${path.replace(/\*$/, "/*")}".`, + `"${path.replace(/\*$/, "/*")}" because the \`*\` character must ` + + `always follow a \`/\` in the pattern. To get rid of this warning, ` + + `please change the route path to "${path.replace(/\*$/, "/*")}".`, ); let params: CompiledPathParam[] = []; @@ -1536,8 +1536,8 @@ export function decodePath(value: string) { warning( false, `The URL path "${value}" could not be decoded because it is a ` + - `malformed URL segment. This is probably due to a bad percent ` + - `encoding (${error}).`, + `malformed URL segment. This is probably due to a bad percent ` + + `encoding (${error}).`, ); return value; @@ -1568,12 +1568,10 @@ export function stripBasename( return pathname.slice(startIndex) || "/"; } - function safeEncode(value: string): string { return encodeURIComponent(value).replace(/%25([0-9A-F]{2})/gi, "%$1"); } - export function prependBasename({ basename, pathname, @@ -1618,7 +1616,7 @@ export function resolvePath(to: To, fromPathname = "/"): Path { warning( false, `Pathnames cannot have embedded double slashes - normalizing ` + - `${oldPathname} -> ${toPathname}`, + `${oldPathname} -> ${toPathname}`, ); } if (toPathname.startsWith("/")) { From 2e6107929b4e0390ef0ac1aa3aeaac6d32c45ff8 Mon Sep 17 00:00:00 2001 From: koynaarya Date: Thu, 4 Dec 2025 22:58:40 +0530 Subject: [PATCH 3/3] chore: add contributor ynakoo --- contributors.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/contributors.yml b/contributors.yml index 64f04cec93..66fcd3c9ac 100644 --- a/contributors.yml +++ b/contributors.yml @@ -453,6 +453,7 @@ - xcsnowcity - xdaxer - yionr +- ynakoo - yracnet - ytori - yuhwan-park