Skip to content

Commit

Permalink
fix: properly handle external redirects (#9590)
Browse files Browse the repository at this point in the history 
* fix: properly handle external redirects
* Handle missing loader with 400 instead of 405
  • Loading branch information
@brophdawg11
brophdawg11 committed Nov 23, 2022
1 parent 2cd8246 commit d6a6825
Show file tree
Hide file tree
Showing 4 changed files with 136 additions and 36 deletions.
5 changes: 5 additions & 0 deletions .changeset/flat-trainers-speak.md
View file
@@ -0,0 +1,5 @@
---
"@remix-run/router": patch
---

properly handle redirects to external domains
88 changes: 83 additions & 5 deletions packages/router/__tests__/router-test.ts
View file
Expand Up @@ -5433,7 +5433,7 @@ describe("a router", () => {
it("preserves query and hash in redirects", async () => {
let t = setup({ routes: REDIRECT_ROUTES });

let nav1 = await t.fetch("/parent/child", {
let nav1 = await t.navigate("/parent/child", {
formMethod: "post",
formData: createFormData({}),
});
Expand All @@ -5459,7 +5459,7 @@ describe("a router", () => {
it("preserves query and hash in relative redirects", async () => {
let t = setup({ routes: REDIRECT_ROUTES });

let nav1 = await t.fetch("/parent/child", {
let nav1 = await t.navigate("/parent/child", {
formMethod: "post",
formData: createFormData({}),
});
Expand All @@ -5484,6 +5484,37 @@ describe("a router", () => {
errors: null,
});
});

it("processes external redirects if window is present", async () => {
let urls = [
"http://remix.run/blog",
"https://remix.run/blog",
"//remix.run/blog",
"app://whatever",
];

for (let url of urls) {
// This is gross, don't blame me, blame SO :)
// https://stackoverflow.com/a/60697570
let oldLocation = window.location;
const location = new URL(window.location.href) as unknown as Location;
location.replace = jest.fn();
delete (window as any).location;
window.location = location as unknown as Location;

let t = setup({ routes: REDIRECT_ROUTES });

let A = await t.navigate("/parent/child", {
formMethod: "post",
formData: createFormData({}),
});

await A.actions.child.redirectReturn(url);
expect(window.location.replace).toHaveBeenCalledWith(url);

window.location = oldLocation;
}
});
});

describe("scroll restoration", () => {
Expand Down Expand Up @@ -6822,7 +6853,7 @@ describe("a router", () => {
405,
"Method Not Allowed",
new Error(
'You made a post request to "/" but did not provide a `loader` ' +
'You made a post request to "/" but did not provide an `action` ' +
'for route "root", so there is no way to handle the request.'
),
true
Expand Down Expand Up @@ -10312,6 +10343,28 @@ describe("a router", () => {
expect((response as Response).headers.get("Location")).toBe("/parent");
});

it("should handle external redirect Responses", async () => {
let urls = [
"http://remix.run/blog",
"https://remix.run/blog",
"//remix.run/blog",
"app://whatever",
];

for (let url of urls) {
let handler = createStaticHandler([
{
path: "/",
loader: () => redirect(url),
},
]);
let response = await handler.query(createRequest("/"));
expect(response instanceof Response).toBe(true);
expect((response as Response).status).toBe(302);
expect((response as Response).headers.get("Location")).toBe(url);
}
});

it("should handle 404 navigations", async () => {
let { query } = createStaticHandler(SSR_ROUTES);
let context = await query(createRequest("/not/found"));
Expand Down Expand Up @@ -11311,6 +11364,29 @@ describe("a router", () => {
expect((response as Response).headers.get("Location")).toBe("/parent");
});

it("should handle external redirect Responses", async () => {
let urls = [
"http://remix.run/blog",
"https://remix.run/blog",
"//remix.run/blog",
"app://whatever",
];

for (let url of urls) {
let handler = createStaticHandler([
{
id: "root",
path: "/",
loader: () => redirect(url),
},
]);
let response = await handler.queryRoute(createRequest("/"), "root");
expect(response instanceof Response).toBe(true);
expect((response as Response).status).toBe(302);
expect((response as Response).headers.get("Location")).toBe(url);
}
});

it("should not unwrap responses returned from loaders", async () => {
let response = json({ key: "value" });
let { queryRoute } = createStaticHandler([
Expand Down Expand Up @@ -11466,13 +11542,13 @@ describe("a router", () => {
}
});

it("should handle not found action/loader submissions with a 405 Response", async () => {
it("should handle missing loaders with a 400 Response", async () => {
try {
await queryRoute(createRequest("/"), "root");
expect(false).toBe(true);
} catch (data) {
expect(isRouteErrorResponse(data)).toBe(true);
expect(data.status).toBe(405);
expect(data.status).toBe(400);
expect(data.error).toEqual(
new Error(
'You made a GET request to "/" but did not provide a `loader` ' +
Expand All @@ -11481,7 +11557,9 @@ describe("a router", () => {
);
expect(data.internal).toBe(true);
}
});

it("should handle missing actions with a 405 Response", async () => {
try {
await queryRoute(createSubmitRequest("/"), "root");
expect(false).toBe(true);
Expand Down
78 changes: 47 additions & 31 deletions packages/router/router.ts
View file
Expand Up @@ -1596,6 +1596,16 @@ export function createRouter(init: RouterInit): Router {
navigation.location,
"Expected a location on the redirect navigation"
);

if (
redirect.external &&
typeof window !== "undefined" &&
typeof window.location !== "undefined"
) {
window.location.replace(redirect.location);
return;
}

// There's no need to abort on redirects, since we don't detect the
// redirect until the action/loaders have settled
pendingNavigationController = null;
Expand Down Expand Up @@ -2185,7 +2195,7 @@ export function unstable_createStaticHandler(

// Short circuit if we have no loaders to run (queryRoute())
if (isRouteRequest && !routeMatch?.route.loader) {
throw getInternalRouterError(405, {
throw getInternalRouterError(400, {
method: request.method,
pathname: createURL(request.url).pathname,
routeId: routeMatch?.route.id,
Expand Down Expand Up @@ -2578,26 +2588,31 @@ async function callLoaderOrAction(
"Redirects returned/thrown from loaders/actions must have a Location header"
);

// Support relative routing in redirects
let activeMatches = matches.slice(0, matches.indexOf(match) + 1);
let routePathnames = getPathContributingMatches(activeMatches).map(
(match) => match.pathnameBase
);
let requestPath = createURL(request.url).pathname;
let resolvedLocation = resolveTo(location, routePathnames, requestPath);
invariant(
createPath(resolvedLocation),
`Unable to resolve redirect location: ${result.headers.get("Location")}`
);
// Check if this an external redirect that goes to a new origin
let external = createURL(location).origin !== createURL("/").origin;

// Prepend the basename to the redirect location if we have one
if (basename) {
let path = resolvedLocation.pathname;
resolvedLocation.pathname =
path === "/" ? basename : joinPaths([basename, path]);
}
// Support relative routing in internal redirects
if (!external) {
let activeMatches = matches.slice(0, matches.indexOf(match) + 1);
let routePathnames = getPathContributingMatches(activeMatches).map(
(match) => match.pathnameBase
);
let requestPath = createURL(request.url).pathname;
let resolvedLocation = resolveTo(location, routePathnames, requestPath);
invariant(
createPath(resolvedLocation),
`Unable to resolve redirect location: ${location}`
);

location = createPath(resolvedLocation);
// Prepend the basename to the redirect location if we have one
if (basename) {
let path = resolvedLocation.pathname;
resolvedLocation.pathname =
path === "/" ? basename : joinPaths([basename, path]);
}

location = createPath(resolvedLocation);
}

// Don't process redirects in the router during static requests requests.
// Instead, throw the Response and let the server handle it with an HTTP
Expand All @@ -2613,6 +2628,7 @@ async function callLoaderOrAction(
status,
location,
revalidate: result.headers.get("X-Remix-Revalidate") !== null,
external,
};
}

Expand Down Expand Up @@ -2921,7 +2937,14 @@ function getInternalRouterError(

if (status === 400) {
statusText = "Bad Request";
errorMessage = "Cannot submit binary form data using GET";
if (method && pathname && routeId) {
errorMessage =
`You made a ${method} request to "${pathname}" but ` +
`did not provide a \`loader\` for route "${routeId}", ` +
`so there is no way to handle the request.`;
} else {
errorMessage = "Cannot submit binary form data using GET";
}
} else if (status === 403) {
statusText = "Forbidden";
errorMessage = `Route "${routeId}" does not match URL "${pathname}"`;
Expand All @@ -2931,17 +2954,10 @@ function getInternalRouterError(
} else if (status === 405) {
statusText = "Method Not Allowed";
if (method && pathname && routeId) {
if (validActionMethods.has(method)) {
errorMessage =
`You made a ${method} request to "${pathname}" but ` +
`did not provide an \`action\` for route "${routeId}", ` +
`so there is no way to handle the request.`;
} else {
errorMessage =
`You made a ${method} request to "${pathname}" but ` +
`did not provide a \`loader\` for route "${routeId}", ` +
`so there is no way to handle the request.`;
}
errorMessage =
`You made a ${method} request to "${pathname}" but ` +
`did not provide an \`action\` for route "${routeId}", ` +
`so there is no way to handle the request.`;
} else {
errorMessage = `Invalid request method "${method}"`;
}
Expand Down
1 change: 1 addition & 0 deletions packages/router/utils.ts
View file
Expand Up @@ -41,6 +41,7 @@ export interface RedirectResult {
status: number;
location: string;
revalidate: boolean;
external: boolean;
}

/**
Expand Down

0 comments on commit d6a6825

Please sign in to comment.