diff --git a/.prettierignore b/.prettierignore index bb1bedff36..f16fe56f69 100644 --- a/.prettierignore +++ b/.prettierignore @@ -12,6 +12,7 @@ integration/helpers/**/build/ playwright-report/ test-results/ build.utils.d.ts +.github/workflows/*.lock.yml .wrangler/ .tmp/ .react-router/ diff --git a/packages/react-router/.changes/patch.linear-route-matching.md b/packages/react-router/.changes/patch.linear-route-matching.md new file mode 100644 index 0000000000..ed0107bf15 --- /dev/null +++ b/packages/react-router/.changes/patch.linear-route-matching.md @@ -0,0 +1 @@ +Improve route matching performance for long paths diff --git a/packages/react-router/.changes/patch.normalize-url-control-characters.md b/packages/react-router/.changes/patch.normalize-url-control-characters.md new file mode 100644 index 0000000000..1fcb82fe34 --- /dev/null +++ b/packages/react-router/.changes/patch.normalize-url-control-characters.md @@ -0,0 +1 @@ +Improve handling of special characters in navigation paths diff --git a/packages/react-router/.changes/patch.schemeful-action-origins.md b/packages/react-router/.changes/patch.schemeful-action-origins.md new file mode 100644 index 0000000000..28142a3697 --- /dev/null +++ b/packages/react-router/.changes/patch.schemeful-action-origins.md @@ -0,0 +1 @@ +Improve validation of action request origins diff --git a/packages/react-router/__tests__/dom/link-href-test.tsx b/packages/react-router/__tests__/dom/link-href-test.tsx index 861de56e30..589ec13300 100644 --- a/packages/react-router/__tests__/dom/link-href-test.tsx +++ b/packages/react-router/__tests__/dom/link-href-test.tsx @@ -129,6 +129,26 @@ describe(" href", () => { expect(renderer.root.findByType("a").props.href).toEqual("//remix.run"); }); + test("normalizes special characters in relative values", () => { + let renderer: TestRenderer.ReactTestRenderer; + TestRenderer.act(() => { + renderer = TestRenderer.create( + + + + } + /> + + + , + ); + }); + + expect(renderer.root.findByType("a").props.href).toEqual("/nested/path"); + }); + test(' is treated as external link', () => { let renderer: TestRenderer.ReactTestRenderer; TestRenderer.act(() => { diff --git a/packages/react-router/__tests__/router/browser-test.ts b/packages/react-router/__tests__/router/browser-test.ts index d54373171d..1febdb99ff 100644 --- a/packages/react-router/__tests__/router/browser-test.ts +++ b/packages/react-router/__tests__/router/browser-test.ts @@ -47,6 +47,12 @@ describe("a browser history", () => { expect(href).toEqual("/the/path?the=query#the-hash"); }); + it("normalizes special characters in relative hrefs", () => { + for (let char of ["\t", "\n", "\r"]) { + expect(history.createHref(`/${char}/nested/path`)).toBe("/nested/path"); + } + }); + it("does not encode the generated path", () => { const encodedHref = history.createHref({ pathname: "/%23abc", diff --git a/packages/react-router/__tests__/router/redirects-test.ts b/packages/react-router/__tests__/router/redirects-test.ts index 86d5f99d10..8060380c7d 100644 --- a/packages/react-router/__tests__/router/redirects-test.ts +++ b/packages/react-router/__tests__/router/redirects-test.ts @@ -1,6 +1,6 @@ import { createMemoryHistory } from "../../lib/router/history"; import { IDLE_NAVIGATION, createRouter } from "../../lib/router/router"; -import { replace } from "../../lib/router/utils"; +import { redirect, replace } from "../../lib/router/utils"; import type { TestRouteObject } from "./utils/data-router-setup"; import { cleanup, setup } from "./utils/data-router-setup"; import { createFormData, tick } from "./utils/utils"; @@ -482,6 +482,25 @@ describe("redirects", () => { } }); + it("normalizes special characters in redirects", async () => { + let router = createRouter({ + history: createMemoryHistory(), + routes: [ + { path: "/" }, + { path: "/start", loader: () => redirect("/\t/parent") }, + { path: "/parent" }, + ], + }); + router.initialize(); + await tick(); + + await router.navigate("/start"); + expect(router.state.location).toMatchObject({ + pathname: "/parent", + }); + router.dispose(); + }); + it("properly handles same-origin absolute URLs when using a basename", async () => { let t = setup({ routes: REDIRECT_ROUTES, basename: "/base" }); diff --git a/packages/react-router/__tests__/server-runtime/actions-test.ts b/packages/react-router/__tests__/server-runtime/actions-test.ts index 0a7762480e..d65d6035c3 100644 --- a/packages/react-router/__tests__/server-runtime/actions-test.ts +++ b/packages/react-router/__tests__/server-runtime/actions-test.ts @@ -41,9 +41,23 @@ describe("throwIfPotentialCSRFAttack", () => { }, }); expect(() => throwIfPotentialCSRFAttack(request, undefined)).toThrow( - "`request.url` host does not match `origin` header from a forwarded action request", + "`request.url` origin does not match `origin` header from a forwarded action request", ); }); + + it("should compare complete origins", () => { + for (let [origin, requestUrl] of [ + ["http://example.com", "https://example.com/action"], + ["https://example.com", "http://example.com/action"], + ]) { + let request = new Request(requestUrl, { + method: "POST", + headers: { origin }, + }); + + expect(() => throwIfPotentialCSRFAttack(request, undefined)).toThrow(); + } + }); }); describe("with allowed origins", () => { @@ -59,6 +73,18 @@ describe("throwIfPotentialCSRFAttack", () => { ).not.toThrow(); }); + it("should support explicitly allowed hosts", () => { + let request = new Request("https://example.com/action", { + method: "POST", + headers: { + origin: "http://example.com", + }, + }); + expect(() => + throwIfPotentialCSRFAttack(request, ["example.com"]), + ).not.toThrow(); + }); + it("should not throw when origin matches a wildcard pattern", () => { let request = new Request("https://example.com/action", { method: "POST", @@ -93,7 +119,7 @@ describe("throwIfPotentialCSRFAttack", () => { expect(() => throwIfPotentialCSRFAttack(request, ["trusted.com", "*.safe.com"]), ).toThrow( - "`request.url` host does not match `origin` header from a forwarded action request", + "`request.url` origin does not match `origin` header from a forwarded action request", ); }); @@ -134,7 +160,7 @@ describe("throwIfPotentialCSRFAttack", () => { }, }); expect(() => throwIfPotentialCSRFAttack(request, undefined)).toThrow( - "`request.url` host does not match `origin` header from a forwarded action request", + "`request.url` origin does not match `origin` header from a forwarded action request", ); }); @@ -181,7 +207,7 @@ describe("throwIfPotentialCSRFAttack", () => { }, }); expect(() => throwIfPotentialCSRFAttack(request, undefined)).toThrow( - "`request.url` host does not match `origin` header from a forwarded action request", + "`request.url` origin does not match `origin` header from a forwarded action request", ); }); @@ -195,7 +221,7 @@ describe("throwIfPotentialCSRFAttack", () => { expect(() => throwIfPotentialCSRFAttack(request, ["", "other.com"]), ).toThrow( - "`request.url` host does not match `origin` header from a forwarded action request", + "`request.url` origin does not match `origin` header from a forwarded action request", ); }); @@ -219,7 +245,7 @@ describe("throwIfPotentialCSRFAttack", () => { }, }); expect(() => throwIfPotentialCSRFAttack(request, undefined)).toThrow( - "`request.url` host does not match `origin` header from a forwarded action request", + "`request.url` origin does not match `origin` header from a forwarded action request", ); }); @@ -243,7 +269,7 @@ describe("throwIfPotentialCSRFAttack", () => { }, }); expect(() => throwIfPotentialCSRFAttack(request, ["*"])).toThrow( - "`request.url` host does not match `origin` header from a forwarded action request", + "`request.url` origin does not match `origin` header from a forwarded action request", ); }); diff --git a/packages/react-router/lib/actions.ts b/packages/react-router/lib/actions.ts index 778c88a454..cbfaf8441d 100644 --- a/packages/react-router/lib/actions.ts +++ b/packages/react-router/lib/actions.ts @@ -4,24 +4,30 @@ export function throwIfPotentialCSRFAttack( ) { let originHeader = request.headers.get("origin"); let originDomain: string | null = null; + let originUrl: URL | null = null; try { - originDomain = - typeof originHeader === "string" && originHeader !== "null" - ? new URL(originHeader).host - : originHeader; + if (typeof originHeader === "string" && originHeader !== "null") { + originUrl = new URL(originHeader); + originDomain = originUrl.host; + } else { + originDomain = originHeader; + } } catch { throw new Error( `\`origin\` header is not a valid URL. Aborting the action.`, ); } - let host = new URL(request.url).host; + let requestUrl = new URL(request.url); + let originMatchesRequest = originUrl + ? originUrl.origin === requestUrl.origin + : originDomain === requestUrl.host; - if (originDomain && originDomain !== host) { + if (originDomain && !originMatchesRequest) { if (!isAllowedOrigin(originDomain, allowedActionOrigins)) { // This seems to be an CSRF attack. We should not proceed with the action. throw new Error( - "The `request.url` host does not match `origin` header from a forwarded " + + "The `request.url` origin does not match `origin` header from a forwarded " + "action request. Aborting the action.", ); } diff --git a/packages/react-router/lib/dom/lib.tsx b/packages/react-router/lib/dom/lib.tsx index 32d5d93de6..65118ae3c5 100644 --- a/packages/react-router/lib/dom/lib.tsx +++ b/packages/react-router/lib/dom/lib.tsx @@ -38,13 +38,13 @@ import { defaultMapRouteProperties, ErrorResponseImpl, SUPPORTED_ERROR_TYPES, + isAbsoluteUrl, joinPaths, matchPath, parseToInfo, resolveTo, stripBasename, } from "../router/utils"; -import { ABSOLUTE_URL_REGEX } from "../router/url"; // eslint-disable-next-line @typescript-eslint/no-unused-vars import type * as _ from "./global"; @@ -1334,7 +1334,7 @@ export const Link = React.forwardRef( ) { let { basename, navigator, useTransitions } = React.useContext(NavigationContext); - let isAbsolute = typeof to === "string" && ABSOLUTE_URL_REGEX.test(to); + let isAbsolute = typeof to === "string" && isAbsoluteUrl(to); let parsed = parseToInfo(to, basename); to = parsed.to; @@ -1944,8 +1944,7 @@ export const Form = React.forwardRef( let formAction = useFormAction(action, { relative }); let formMethod: HTMLFormMethod = method.toLowerCase() === "get" ? "get" : "post"; - let isAbsolute = - typeof action === "string" && ABSOLUTE_URL_REGEX.test(action); + let isAbsolute = typeof action === "string" && isAbsoluteUrl(action); let submitHandler: React.SubmitEventHandler = (event) => { onSubmit && onSubmit(event); diff --git a/packages/react-router/lib/dom/server.tsx b/packages/react-router/lib/dom/server.tsx index 0713618bbe..1fd29f0269 100644 --- a/packages/react-router/lib/dom/server.tsx +++ b/packages/react-router/lib/dom/server.tsx @@ -24,7 +24,7 @@ import { convertRoutesToDataRoutes, isRouteErrorResponse, } from "../router/utils"; -import { ABSOLUTE_URL_REGEX } from "../router/url"; +import { ABSOLUTE_URL_REGEX, normalizeRelativeUrl } from "../router/url"; import { DataRoutes, Router } from "../components"; import { DataRouterContext, @@ -457,6 +457,7 @@ function createHref(to: To) { function encodeLocation(to: To): Path { let href = typeof to === "string" ? to : createPath(to); + href = normalizeRelativeUrl(href); // Treating this as a full URL will strip any trailing spaces so we need to // pre-encode them since they might be part of a matching splat param from // an ancestor route diff --git a/packages/react-router/lib/router/history.ts b/packages/react-router/lib/router/history.ts index 08148d6a0e..b4cc9894a0 100644 --- a/packages/react-router/lib/router/history.ts +++ b/packages/react-router/lib/router/history.ts @@ -1,4 +1,4 @@ -import { PROTOCOL_RELATIVE_URL_REGEX } from "./url"; +import { normalizeRelativeUrl, PROTOCOL_RELATIVE_URL_REGEX } from "./url"; //////////////////////////////////////////////////////////////////////////////// //#region Types and Constants @@ -407,7 +407,7 @@ export function createBrowserHistory( } function createBrowserHref(window: Window, to: To) { - return typeof to === "string" ? to : createPath(to); + return normalizeRelativeUrl(typeof to === "string" ? to : createPath(to)); } return getUrlBasedHistory( @@ -798,6 +798,7 @@ export function createBrowserURLImpl( invariant(base, "No window.location.(origin|href) available to create URL"); let href = typeof to === "string" ? to : createPath(to); + href = normalizeRelativeUrl(href); // Treating this as a full URL will strip any trailing spaces so we need to // pre-encode them since they might be part of a matching splat param from diff --git a/packages/react-router/lib/router/router.ts b/packages/react-router/lib/router/router.ts index db266eab0e..5b320d0b49 100644 --- a/packages/react-router/lib/router/router.ts +++ b/packages/react-router/lib/router/router.ts @@ -77,6 +77,7 @@ import { } from "./utils"; import { normalizeProtocolRelativeUrl, + normalizeRelativeUrl, PROTOCOL_RELATIVE_URL_REGEX, } from "./url"; @@ -6908,6 +6909,8 @@ function normalizeRedirectLocation( basename: string, historyInstance: History, ): string { + location = normalizeRelativeUrl(location); + if (isAbsoluteUrl(location)) { // Strip off the protocol+origin for same-origin + same-basename absolute redirects let normalizedLocation = location; diff --git a/packages/react-router/lib/router/url.ts b/packages/react-router/lib/router/url.ts index 078b92a292..7c2e61cf9c 100644 --- a/packages/react-router/lib/router/url.ts +++ b/packages/react-router/lib/router/url.ts @@ -1,6 +1,25 @@ export const ABSOLUTE_URL_REGEX = /^(?:[a-z][a-z0-9+.-]*:|[\\/]{2})/i; export const PROTOCOL_RELATIVE_URL_REGEX = /^[\\/]{2}/; +// Normalize characters ignored by the URL parser before determining whether a +// URL is relative or absolute. +export function normalizeRelativeUrl(url: string): string { + if (ABSOLUTE_URL_REGEX.test(url)) { + return url; + } + + let normalized = url.replace(/[\t\n\r]/g, ""); + if (!ABSOLUTE_URL_REGEX.test(normalized)) { + return normalized; + } + + if (PROTOCOL_RELATIVE_URL_REGEX.test(normalized)) { + return normalized.replace(/^[\\/]+/, "/"); + } + + return normalized.replace(/^([a-z][a-z0-9+.-]*):/i, "$1%3A"); +} + export function normalizeProtocolRelativeUrl(url: string, protocol: string) { return protocol + url.replace(/\\/g, "/"); } diff --git a/packages/react-router/lib/router/utils.ts b/packages/react-router/lib/router/utils.ts index 8f78217250..338394fe0d 100644 --- a/packages/react-router/lib/router/utils.ts +++ b/packages/react-router/lib/router/utils.ts @@ -5,6 +5,7 @@ import { invariant, parsePath, warning } from "./history"; import { ABSOLUTE_URL_REGEX, normalizeProtocolRelativeUrl, + normalizeRelativeUrl, PROTOCOL_RELATIVE_URL_REGEX, } from "./url"; @@ -1691,7 +1692,7 @@ function matchPathImpl( if (!match) return null; let matchedPathname = match[0]; - let pathnameBase = matchedPathname.replace(/(.)\/+$/, "$1"); + let pathnameBase = removeTrailingSlash(matchedPathname, 1); let captureGroups = match.slice(1); let params: Params = compiledParams.reduce>( (memo, { paramName, isOptional }, index) => { @@ -1699,9 +1700,10 @@ function matchPathImpl( // instead of using params["*"] later because it will be decoded then if (paramName === "*") { let splatValue = captureGroups[index] || ""; - pathnameBase = matchedPathname - .slice(0, matchedPathname.length - splatValue.length) - .replace(/(.)\/+$/, "$1"); + pathnameBase = removeTrailingSlash( + matchedPathname.slice(0, matchedPathname.length - splatValue.length), + 1, + ); } const value = captureGroups[index]; @@ -1852,7 +1854,8 @@ export function prependBasename({ return pathname === "/" ? basename : joinPaths([basename, pathname]); } -export const isAbsoluteUrl = (url: string) => ABSOLUTE_URL_REGEX.test(url); +export const isAbsoluteUrl = (url: string) => + ABSOLUTE_URL_REGEX.test(normalizeRelativeUrl(url)); /** * Returns a resolved {@link Path} object relative to the given pathname. @@ -1873,6 +1876,7 @@ export function resolvePath(to: To, fromPathname = "/"): Path { let pathname: string; if (toPathname) { + toPathname = normalizeRelativeUrl(toPathname); toPathname = removeDoubleSlashes(toPathname); if (toPathname.startsWith("/")) { pathname = resolvePathname(toPathname.substring(1), "/"); @@ -2051,8 +2055,14 @@ export const removeDoubleSlashes = (path: string): string => export const joinPaths = (paths: string[]): string => removeDoubleSlashes(paths.join("/")); -export const removeTrailingSlash = (path: string): string => - path.replace(/\/+$/, ""); +// Scan from the end to avoid repeated RegExp work on long paths. +export function removeTrailingSlash(path: string, minLength = 0): string { + let end = path.length; + while (end > minLength && path.charCodeAt(end - 1) === 47) { + end--; + } + return end === path.length ? path : path.slice(0, end); +} export const normalizePathname = (pathname: string): string => removeTrailingSlash(pathname).replace(/^\/*/, "/"); @@ -2400,7 +2410,9 @@ export function parseToInfo( _to: T, basename: string, ): ParsedLocationInfo { - let to = _to as string; + let to = ( + typeof _to === "string" ? normalizeRelativeUrl(_to) : _to + ) as string; if (typeof to !== "string" || !ABSOLUTE_URL_REGEX.test(to)) { return { absoluteURL: undefined, diff --git a/packages/react-router/lib/rsc/browser.tsx b/packages/react-router/lib/rsc/browser.tsx index 0daef296f5..0a38484a5e 100644 --- a/packages/react-router/lib/rsc/browser.tsx +++ b/packages/react-router/lib/rsc/browser.tsx @@ -25,7 +25,10 @@ import type { RouterContextProvider, } from "../router/utils"; import { ErrorResponseImpl, createContext, resolvePath } from "../router/utils"; -import { PROTOCOL_RELATIVE_URL_REGEX } from "../router/url"; +import { + normalizeRelativeUrl, + PROTOCOL_RELATIVE_URL_REGEX, +} from "../router/url"; import type { DecodedSingleFetchResults, FetchAndDecodeFunction, @@ -1131,6 +1134,8 @@ function isExternalLocation(location: string) { } function normalizeRedirectLocation(location: string): string { + location = normalizeRelativeUrl(location); + if (PROTOCOL_RELATIVE_URL_REGEX.test(location)) { let path = resolvePath(location); return path.pathname + path.search + path.hash;