fix(lambda-at-edge): fix broken redirects with full URLs as destinati…

…ons (#638)
serverless-nextjs · Sep 30, 2020 · ee95e54 · ee95e54
1 parent c802fa4
commit ee95e54
Show file tree

Hide file tree

Showing 12 changed files with 155 additions and 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
@@ -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
@@ -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
@@ -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
@@ -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/...a-at-edge/tests/default-handler/default-basepath-routes-manifest-with-trailing-slash.json b/...a-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
@@ -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
@@ -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
@@ -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/...ibs/lambda-at-edge/tests/default-handler/default-routes-manifest-with-trailing-slash.json b/...ibs/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
@@ -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
@@ -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
@@ -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 }) => {