From ee95e549cdb620865d536435e74e1ea3026ecd20 Mon Sep 17 00:00:00 2001 From: Daniel Phang Date: Wed, 30 Sep 2020 02:29:27 -0700 Subject: [PATCH] fix(lambda-at-edge): fix broken redirects with full URLs as destinations (#638) --- .../cypress/integration/redirects.test.ts | 18 +++++++++++ packages/e2e-tests/next-app/next.config.js | 15 +++++++++ .../lambda-at-edge/src/routing/matcher.ts | 32 +++++++++++++++++-- .../lambda-at-edge/src/routing/redirector.ts | 11 ++++++- ...h-routes-manifest-with-trailing-slash.json | 6 ++++ .../default-basepath-routes-manifest.json | 6 ++++ .../default-handler-with-basepath.test.ts | 2 ++ .../default-handler/default-handler.test.ts | 18 ++++++----- ...t-routes-manifest-with-trailing-slash.json | 6 ++++ .../default-routes-manifest.json | 6 ++++ .../tests/routing/matcher.test.ts | 27 ++++++++++++++++ .../tests/routing/redirector.test.ts | 26 +++++++++++---- 12 files changed, 155 insertions(+), 18 deletions(-) diff --git a/packages/e2e-tests/next-app/cypress/integration/redirects.test.ts b/packages/e2e-tests/next-app/cypress/integration/redirects.test.ts index f94a8c2c8e..98ce4540e5 100644 --- a/packages/e2e-tests/next-app/cypress/integration/redirects.test.ts +++ b/packages/e2e-tests/next-app/cypress/integration/redirects.test.ts @@ -136,6 +136,24 @@ describe("Redirects Tests", () => { expectedRedirect: "/ssr-page", expectedStatus: 200, expectedRedirectStatus: 302 + }, + { + path: "/external-redirect-1", + expectedRedirect: "https://api.github.com", + expectedStatus: 200, + expectedRedirectStatus: 308 + }, + { + path: "/external-redirect-2/abcd", + expectedRedirect: "https://api.github.com/abcd", + expectedStatus: 404, + expectedRedirectStatus: 308 + }, + { + path: "/external-redirect-3/abcd", + expectedRedirect: "https://api.github.com/abcd/", + expectedStatus: 404, + expectedRedirectStatus: 308 } ].forEach( ({ path, expectedRedirect, expectedStatus, expectedRedirectStatus }) => { diff --git a/packages/e2e-tests/next-app/next.config.js b/packages/e2e-tests/next-app/next.config.js index 954f808467..5381687d5c 100644 --- a/packages/e2e-tests/next-app/next.config.js +++ b/packages/e2e-tests/next-app/next.config.js @@ -35,6 +35,21 @@ module.exports = { source: "/regex-redirect-2/:slug(\\d{1,})", destination: "/regex-redirect-2-dest/:slug", permanent: true + }, + { + source: "/external-redirect-1", + destination: "https://api.github.com", + permanent: true + }, + { + source: "/external-redirect-2/:id", + destination: "https://api.github.com/:id", + permanent: true + }, + { + source: "/external-redirect-3/:id", + destination: "https://api.github.com/:id/", + permanent: true } ]; } diff --git a/packages/libs/lambda-at-edge/src/routing/matcher.ts b/packages/libs/lambda-at-edge/src/routing/matcher.ts index 2734ee6228..daa82078a7 100644 --- a/packages/libs/lambda-at-edge/src/routing/matcher.ts +++ b/packages/libs/lambda-at-edge/src/routing/matcher.ts @@ -22,7 +22,33 @@ export function matchPath(path: string, source: string): Match { export function compileDestination( destination: string, params: object -): string { - const toPath = compile(destination, { encode: encodeURIComponent }); - return toPath(params); +): string | null { + try { + const destinationLowerCase = destination.toLowerCase(); + if ( + destinationLowerCase.startsWith("https://") || + destinationLowerCase.startsWith("http://") + ) { + // Handle external URLs + const { origin, pathname } = new URL(destination); + const toPath = compile(pathname, { encode: encodeURIComponent }); + const compiledDestination = `${origin}${toPath(params)}`; + + // Remove trailing slash if original destination didn't have it + if (!destination.endsWith("/") && compiledDestination.endsWith("/")) { + return compiledDestination.slice(0, -1); + } else { + return compiledDestination; + } + } else { + // Handle all other paths + const toPath = compile(destination, { encode: encodeURIComponent }); + return toPath(params); + } + } catch (error) { + console.error( + `Could not compile destination ${destination}, returning null instead. Error: ${error}` + ); + return null; + } } diff --git a/packages/libs/lambda-at-edge/src/routing/redirector.ts b/packages/libs/lambda-at-edge/src/routing/redirector.ts index a8ab15a61d..f2a59b48aa 100644 --- a/packages/libs/lambda-at-edge/src/routing/redirector.ts +++ b/packages/libs/lambda-at-edge/src/routing/redirector.ts @@ -55,8 +55,17 @@ export function getRedirectPath( const match = matchPath(path, redirect.source); if (match) { + const compiledDestination = compileDestination( + redirect.destination, + match.params + ); + + if (!compiledDestination) { + return null; + } + return { - redirectPath: compileDestination(redirect.destination, match.params), + redirectPath: compiledDestination, statusCode: redirect.statusCode }; } diff --git a/packages/libs/lambda-at-edge/tests/default-handler/default-basepath-routes-manifest-with-trailing-slash.json b/packages/libs/lambda-at-edge/tests/default-handler/default-basepath-routes-manifest-with-trailing-slash.json index 929e9c4264..e83780488a 100644 --- a/packages/libs/lambda-at-edge/tests/default-handler/default-basepath-routes-manifest-with-trailing-slash.json +++ b/packages/libs/lambda-at-edge/tests/default-handler/default-basepath-routes-manifest-with-trailing-slash.json @@ -20,6 +20,12 @@ "destination": "/basepath/users/:post/", "statusCode": 307, "regex": "^/basepath/old-users(?:/(\\d{1,}))/$" + }, + { + "source": "/basepath/external/", + "destination": "https://example.com", + "statusCode": 308, + "regex": "^/basepath/external/" } ], "rewrites": [], diff --git a/packages/libs/lambda-at-edge/tests/default-handler/default-basepath-routes-manifest.json b/packages/libs/lambda-at-edge/tests/default-handler/default-basepath-routes-manifest.json index d8218573f2..894f602db8 100644 --- a/packages/libs/lambda-at-edge/tests/default-handler/default-basepath-routes-manifest.json +++ b/packages/libs/lambda-at-edge/tests/default-handler/default-basepath-routes-manifest.json @@ -20,6 +20,12 @@ "destination": "/basepath/users/:post", "statusCode": 307, "regex": "^/basepath/old-users(?:/(\\d{1,}))$" + }, + { + "source": "/basepath/external", + "destination": "https://example.com", + "statusCode": 308, + "regex": "^/basepath/external" } ], "rewrites": [], diff --git a/packages/libs/lambda-at-edge/tests/default-handler/default-handler-with-basepath.test.ts b/packages/libs/lambda-at-edge/tests/default-handler/default-handler-with-basepath.test.ts index 6054a327f5..0776fdd248 100644 --- a/packages/libs/lambda-at-edge/tests/default-handler/default-handler-with-basepath.test.ts +++ b/packages/libs/lambda-at-edge/tests/default-handler/default-handler-with-basepath.test.ts @@ -625,6 +625,7 @@ describe("Lambda@Edge", () => { ${"/basepath/terms-new/"} | ${"/basepath/terms/"} | ${308} ${"/basepath/old-blog/abc/"} | ${"/basepath/news/abc/"} | ${308} ${"/basepath/old-users/1234/"} | ${"/basepath/users/1234/"} | ${307} + ${"/basepath/external/"} | ${"https://example.com"} | ${308} `( "redirects path $path to $expectedRedirect, expectedRedirectStatusCode: $expectedRedirectStatusCode", async ({ path, expectedRedirect, expectedRedirectStatusCode }) => { @@ -641,6 +642,7 @@ describe("Lambda@Edge", () => { ${"/basepath/terms-new"} | ${"/basepath/terms"} | ${308} ${"/basepath/old-blog/abc"} | ${"/basepath/news/abc"} | ${308} ${"/basepath/old-users/1234"} | ${"/basepath/users/1234"} | ${307} + ${"/basepath/external"} | ${"https://example.com"} | ${308} `( "redirects path $path to $expectedRedirect, expectedRedirectStatusCode: $expectedRedirectStatusCode", async ({ path, expectedRedirect, expectedRedirectStatusCode }) => { diff --git a/packages/libs/lambda-at-edge/tests/default-handler/default-handler.test.ts b/packages/libs/lambda-at-edge/tests/default-handler/default-handler.test.ts index d245a5c840..35d9ab98d0 100644 --- a/packages/libs/lambda-at-edge/tests/default-handler/default-handler.test.ts +++ b/packages/libs/lambda-at-edge/tests/default-handler/default-handler.test.ts @@ -641,10 +641,11 @@ describe("Lambda@Edge", () => { describe("Custom Redirects", () => { if (trailingSlash) { it.each` - path | expectedRedirect | expectedRedirectStatusCode - ${"/terms-new/"} | ${"/terms/"} | ${308} - ${"/old-blog/abc/"} | ${"/news/abc/"} | ${308} - ${"/old-users/1234/"} | ${"/users/1234/"} | ${307} + path | expectedRedirect | expectedRedirectStatusCode + ${"/terms-new/"} | ${"/terms/"} | ${308} + ${"/old-blog/abc/"} | ${"/news/abc/"} | ${308} + ${"/old-users/1234/"} | ${"/users/1234/"} | ${307} + ${"/external/"} | ${"https://example.com"} | ${308} `( "redirects path $path to $expectedRedirect, expectedRedirectStatusCode: $expectedRedirectStatusCode", async ({ path, expectedRedirect, expectedRedirectStatusCode }) => { @@ -657,10 +658,11 @@ describe("Lambda@Edge", () => { ); } else { it.each` - path | expectedRedirect | expectedRedirectStatusCode - ${"/terms-new"} | ${"/terms"} | ${308} - ${"/old-blog/abc"} | ${"/news/abc"} | ${308} - ${"/old-users/1234"} | ${"/users/1234"} | ${307} + path | expectedRedirect | expectedRedirectStatusCode + ${"/terms-new"} | ${"/terms"} | ${308} + ${"/old-blog/abc"} | ${"/news/abc"} | ${308} + ${"/old-users/1234"} | ${"/users/1234"} | ${307} + ${"/external"} | ${"https://example.com"} | ${308} `( "redirects path $path to $expectedRedirect, expectedRedirectStatusCode: $expectedRedirectStatusCode", async ({ path, expectedRedirect, expectedRedirectStatusCode }) => { diff --git a/packages/libs/lambda-at-edge/tests/default-handler/default-routes-manifest-with-trailing-slash.json b/packages/libs/lambda-at-edge/tests/default-handler/default-routes-manifest-with-trailing-slash.json index fd63eea0f0..58fe364ddf 100644 --- a/packages/libs/lambda-at-edge/tests/default-handler/default-routes-manifest-with-trailing-slash.json +++ b/packages/libs/lambda-at-edge/tests/default-handler/default-routes-manifest-with-trailing-slash.json @@ -20,6 +20,12 @@ "destination": "/users/:id/", "statusCode": 307, "regex": "^/old-users(?:/(\\d{1,}))/$" + }, + { + "source": "/external/", + "destination": "https://example.com", + "statusCode": 308, + "regex": "^/external/" } ], "rewrites": [], diff --git a/packages/libs/lambda-at-edge/tests/default-handler/default-routes-manifest.json b/packages/libs/lambda-at-edge/tests/default-handler/default-routes-manifest.json index 1a2a83cb47..be335d3962 100644 --- a/packages/libs/lambda-at-edge/tests/default-handler/default-routes-manifest.json +++ b/packages/libs/lambda-at-edge/tests/default-handler/default-routes-manifest.json @@ -20,6 +20,12 @@ "destination": "/users/:id", "statusCode": 307, "regex": "^/old-users(?:/(\\d{1,}))$" + }, + { + "source": "/external", + "destination": "https://example.com", + "statusCode": 308, + "regex": "^/external" } ], "rewrites": [], diff --git a/packages/libs/lambda-at-edge/tests/routing/matcher.test.ts b/packages/libs/lambda-at-edge/tests/routing/matcher.test.ts index d717004348..eccbff0293 100644 --- a/packages/libs/lambda-at-edge/tests/routing/matcher.test.ts +++ b/packages/libs/lambda-at-edge/tests/routing/matcher.test.ts @@ -51,5 +51,32 @@ describe("Matcher Tests", () => { const match = compileDestination("/about/:a/:b", { a: "123", b: "456" }); expect(match).toEqual("/about/123/456"); }); + + it("compiles http URL", () => { + const match = compileDestination("http://example.com", {}); + expect(match).toEqual("http://example.com"); + }); + + it("compiles https URL", () => { + const match = compileDestination("https://example.com", {}); + expect(match).toEqual("https://example.com"); + }); + + it("compiles https URL with trailing slash", () => { + const match = compileDestination("https://example.com/", {}); + expect(match).toEqual("https://example.com/"); + }); + + it("compiles parameterized https URL", () => { + const match = compileDestination("https://example.com/:id", { + id: "123" + }); + expect(match).toEqual("https://example.com/123"); + }); + + it("invalid destination returns null", () => { + const match = compileDestination("abc://123", {}); + expect(match).toBeNull(); + }); }); }); diff --git a/packages/libs/lambda-at-edge/tests/routing/redirector.test.ts b/packages/libs/lambda-at-edge/tests/routing/redirector.test.ts index 9091153079..dab3d702fb 100644 --- a/packages/libs/lambda-at-edge/tests/routing/redirector.test.ts +++ b/packages/libs/lambda-at-edge/tests/routing/redirector.test.ts @@ -25,18 +25,32 @@ describe("Redirector Tests", () => { destination: "/users/:id", statusCode: 307, regex: "^/old-users(?:/(\\d{1,}))$" + }, + { + source: "/external", + destination: "https://example.com", + statusCode: 308, + regex: "^/external$" + }, + { + source: "/invalid-destination", + destination: "ftp://example.com", + statusCode: 308, + regex: "^/invalid-destination$" } ] }; }); it.each` - path | expectedRedirect | expectedStatusCode - ${"/a"} | ${"/b"} | ${308} - ${"/c"} | ${"/d"} | ${302} - ${"/old-blog/abc"} | ${"/news/abc"} | ${308} - ${"/old-users/1234"} | ${"/users/1234"} | ${307} - ${"/old-users/abc"} | ${null} | ${null} + path | expectedRedirect | expectedStatusCode + ${"/a"} | ${"/b"} | ${308} + ${"/c"} | ${"/d"} | ${302} + ${"/old-blog/abc"} | ${"/news/abc"} | ${308} + ${"/old-users/1234"} | ${"/users/1234"} | ${307} + ${"/old-users/abc"} | ${null} | ${null} + ${"/external"} | ${"https://example.com"} | ${308} + ${"/invalid-destination"} | ${null} | ${null} `( "redirects path $path to $expectedRedirect", ({ path, expectedRedirect, expectedStatusCode }) => {