Repository: remix-run/remix
Pull request #5336 (2023-01-31 21:24:16)

Fix adapter request creation when double slashes exist
The root cause of this came down to us passing the origin in the optional second `base` param for the `new URL()` constructor.  When we did this, an incoming pathname of `//` got incorrectly identified as a protocol-less absolute URL.  Since we know the origin, we can just pass an absolute URL directly into the first param to avoid this:

```js
let origin = 'http://localhost';
let path = '//';

// ❌ Throws `Invalid URL` error
new URL(path, origin);

// ✅ All good in the hood
new URL(`${origin}${path}`);
```

Closes: #4422
Replace #4622

- [ ] ~~Docs~~
- [x] Tests

**Testing Strategy**:

 * Unit tests for each of the adapters
 * Manual testing for express

⚠️ Note this does not change anything about our route matching logic, it just lets the request make it through to matching.  Current behavior is as follows:

* `//` will match a root/index route
* `//todos` will 404 if a `/todos` route exists

changeset-bot[bot]: ###  🦋  Changeset detected

Latest commit: 7bb4fbe9d89cacd68a1fcb2ff753455fdf049a3a

**The changes in this PR will be included in the next version bump.**

<details><summary>This PR includes changesets to release 18 packages</summary>

  | Name                          | Type  |
| ----------------------------- | ----- |
| @remix-run/architect          | Patch |
| @remix-run/express            | Patch |
| @remix-run/netlify            | Patch |
| @remix-run/vercel             | Patch |
| @remix-run/serve              | Patch |
| @remix-run/dev                | Patch |
| create-remix                  | Patch |
| remix                         | Patch |
| @remix-run/cloudflare         | Patch |
| @remix-run/cloudflare-pages   | Patch |
| @remix-run/cloudflare-workers | Patch |
| @remix-run/css-bundle         | Patch |
| @remix-run/deno               | Patch |
| @remix-run/eslint-config      | Patch |
| @remix-run/node               | Patch |
| @remix-run/react              | Patch |
| @remix-run/server-runtime     | Patch |
| @remix-run/testing            | Patch |

</details>

Not sure what this means? [Click here  to learn what changesets are](https://github.com/changesets/changesets/blob/master/docs/adding-a-changeset.md).

[Click here if you're a maintainer who wants to add another changeset to this PR](https://github.com/remix-run/remix/new/brophdawg11/double-slashes-invalid-url?filename=.changeset/six-days-suffer.md&value=---%0A%22remix%22%3A%20patch%0A%22%40remix-run%2Farchitect%22%3A%20patch%0A%22%40remix-run%2Fexpress%22%3A%20patch%0A%22%40remix-run%2Fnetlify%22%3A%20patch%0A%22%40remix-run%2Fvercel%22%3A%20patch%0A---%0A%0AFix%20adapter%20request%20creation%20when%20double%20slashes%20exist%0A)



Patch:
diff --git a/.changeset/nine-apricots-breathe.md b/.changeset/nine-apricots-breathe.md
new file mode 100644
index 000000000..f25cd7ab6
--- /dev/null
+++ b/.changeset/nine-apricots-breathe.md
@@ -0,0 +1,8 @@
+---
+"@remix-run/architect": patch
+"@remix-run/express": patch
+"@remix-run/netlify": patch
+"@remix-run/vercel": patch
+---
+
+Fix Fetch Request creation for incoming URLs with double slashes
diff --git a/packages/remix-architect/server.ts b/packages/remix-architect/server.ts
index 45e971869..cbbfa9f1b 100644
--- a/packages/remix-architect/server.ts
+++ b/packages/remix-architect/server.ts
@@ -62,7 +62,7 @@ export function createRemixRequest(event: APIGatewayProxyEventV2): NodeRequest {
   let host = event.headers["x-forwarded-host"] || event.headers.host;
   let search = event.rawQueryString.length ? `?${event.rawQueryString}` : "";
   let scheme = process.env.ARC_SANDBOX ? "http" : "https";
-  let url = new URL(event.rawPath + search, `${scheme}://${host}`);
+  let url = new URL(`${scheme}://${host}${event.rawPath}${search}`);
   let isFormData = event.headers["content-type"]?.includes(
     "multipart/form-data"
   );
diff --git a/packages/remix-express/server.ts b/packages/remix-express/server.ts
index 4f6fe881f..009652fd3 100644
--- a/packages/remix-express/server.ts
+++ b/packages/remix-express/server.ts
@@ -93,8 +93,7 @@ export function createRemixRequest(
   req: express.Request,
   res: express.Response
 ): NodeRequest {
-  let origin = `${req.protocol}://${req.get("host")}`;
-  let url = new URL(req.url, origin);
+  let url = new URL(`${req.protocol}://${req.get("host")}${req.url}`);
 
   // Abort action/loaders once we can no longer write a response
   let controller = new NodeAbortController();
diff --git a/packages/remix-netlify/server.ts b/packages/remix-netlify/server.ts
index ccd601009..5932d3f04 100644
--- a/packages/remix-netlify/server.ts
+++ b/packages/remix-netlify/server.ts
@@ -63,7 +63,7 @@ export function createRemixRequest(event: HandlerEvent): NodeRequest {
   } else {
     let origin = event.headers.host;
     let rawPath = getRawPath(event);
-    url = new URL(rawPath, `http://${origin}`);
+    url = new URL(`http://${origin}${rawPath}`);
   }
 
   // Note: No current way to abort these for Netlify, but our router expects
diff --git a/packages/remix-vercel/server.ts b/packages/remix-vercel/server.ts
index 2827b96f6..6707693cb 100644
--- a/packages/remix-vercel/server.ts
+++ b/packages/remix-vercel/server.ts
@@ -82,7 +82,7 @@ export function createRemixRequest(
   let host = req.headers["x-forwarded-host"] || req.headers["host"];
   // doesn't seem to be available on their req object!
   let protocol = req.headers["x-forwarded-proto"] || "https";
-  let url = new URL(req.url!, `${protocol}://${host}`);
+  let url = new URL(`${protocol}://${host}${req.url}`);
 
   // Abort action/loaders once we can no longer write a response
   let controller = new NodeAbortController();


Test patch:
diff --git a/packages/remix-architect/__tests__/server-test.ts b/packages/remix-architect/__tests__/server-test.ts
index 969275754..2d02c5136 100644
--- a/packages/remix-architect/__tests__/server-test.ts
+++ b/packages/remix-architect/__tests__/server-test.ts
@@ -101,6 +101,38 @@ describe("architect createRequestHandler", () => {
         });
     });
 
+    it("handles root // requests", async () => {
+      mockedCreateRequestHandler.mockImplementation(() => async (req) => {
+        return new Response(`URL: ${new URL(req.url).pathname}`);
+      });
+
+      // We don't have a real app to test, but it doesn't matter. We won't ever
+      // call through to the real createRequestHandler
+      // @ts-expect-error
+      await lambdaTester(createRequestHandler({ build: undefined }))
+        .event(createMockEvent({ rawPath: "//" }))
+        .expectResolve((res) => {
+          expect(res.statusCode).toBe(200);
+          expect(res.body).toBe("URL: //");
+        });
+    });
+
+    it("handles nested // requests", async () => {
+      mockedCreateRequestHandler.mockImplementation(() => async (req) => {
+        return new Response(`URL: ${new URL(req.url).pathname}`);
+      });
+
+      // We don't have a real app to test, but it doesn't matter. We won't ever
+      // call through to the real createRequestHandler
+      // @ts-expect-error
+      await lambdaTester(createRequestHandler({ build: undefined }))
+        .event(createMockEvent({ rawPath: "//foo//bar" }))
+        .expectResolve((res) => {
+          expect(res.statusCode).toBe(200);
+          expect(res.body).toBe("URL: //foo//bar");
+        });
+    });
+
     it("handles null body", async () => {
       mockedCreateRequestHandler.mockImplementation(() => async () => {
         return new Response(null, { status: 200 });
diff --git a/packages/remix-express/__tests__/server-test.ts b/packages/remix-express/__tests__/server-test.ts
index dbf5cf9d5..1e50480ce 100644
--- a/packages/remix-express/__tests__/server-test.ts
+++ b/packages/remix-express/__tests__/server-test.ts
@@ -64,6 +64,30 @@ describe("express createRequestHandler", () => {
       expect(res.headers["x-powered-by"]).toBe("Express");
     });
 
+    it("handles root // URLs", async () => {
+      mockedCreateRequestHandler.mockImplementation(() => async (req) => {
+        return new Response("URL: " + new URL(req.url).pathname);
+      });
+
+      let request = supertest(createApp());
+      let res = await request.get("//");
+
+      expect(res.status).toBe(200);
+      expect(res.text).toBe("URL: //");
+    });
+
+    it("handles nested // URLs", async () => {
+      mockedCreateRequestHandler.mockImplementation(() => async (req) => {
+        return new Response("URL: " + new URL(req.url).pathname);
+      });
+
+      let request = supertest(createApp());
+      let res = await request.get("//foo//bar");
+
+      expect(res.status).toBe(200);
+      expect(res.text).toBe("URL: //foo//bar");
+    });
+
     it("handles null body", async () => {
       mockedCreateRequestHandler.mockImplementation(() => async () => {
         return new Response(null, { status: 200 });
diff --git a/packages/remix-netlify/__tests__/server-test.ts b/packages/remix-netlify/__tests__/server-test.ts
index a5f0998d0..794c088a8 100644
--- a/packages/remix-netlify/__tests__/server-test.ts
+++ b/packages/remix-netlify/__tests__/server-test.ts
@@ -38,7 +38,9 @@ function createMockEvent(event: Partial<HandlerEvent> = {}): HandlerEvent {
     rawQuery: "",
     path: "/",
     httpMethod: "GET",
-    headers: {},
+    headers: {
+      host: "localhost:3000",
+    },
     multiValueHeaders: {},
     queryStringParameters: null,
     multiValueQueryStringParameters: null,
@@ -74,6 +76,80 @@ describe("netlify createRequestHandler", () => {
         });
     });
 
+    it("handles root // requests", async () => {
+      mockedCreateRequestHandler.mockImplementation(() => async (req) => {
+        return new Response(`URL: ${new URL(req.url).pathname}`);
+      });
+
+      // We don't have a real app to test, but it doesn't matter. We won't ever
+      // call through to the real createRequestHandler
+      // @ts-expect-error
+      await lambdaTester(createRequestHandler({ build: undefined }))
+        .event(createMockEvent({ rawUrl: "http://localhost:3000//" }))
+        .expectResolve((res) => {
+          expect(res.statusCode).toBe(200);
+          expect(res.body).toBe("URL: //");
+        });
+    });
+
+    it("handles nested // requests", async () => {
+      mockedCreateRequestHandler.mockImplementation(() => async (req) => {
+        return new Response(`URL: ${new URL(req.url).pathname}`);
+      });
+
+      // We don't have a real app to test, but it doesn't matter. We won't ever
+      // call through to the real createRequestHandler
+      // @ts-expect-error
+      await lambdaTester(createRequestHandler({ build: undefined }))
+        .event(createMockEvent({ rawUrl: "http://localhost:3000//foo//bar" }))
+        .expectResolve((res) => {
+          expect(res.statusCode).toBe(200);
+          expect(res.body).toBe("URL: //foo//bar");
+        });
+    });
+
+    it("handles root // requests (development)", async () => {
+      let oldEnv = process.env.NODE_ENV;
+      process.env.NODE_ENV = "development";
+
+      mockedCreateRequestHandler.mockImplementation(() => async (req) => {
+        return new Response(`URL: ${new URL(req.url).pathname}`);
+      });
+
+      // We don't have a real app to test, but it doesn't matter. We won't ever
+      // call through to the real createRequestHandler
+      // @ts-expect-error
+      await lambdaTester(createRequestHandler({ build: undefined }))
+        .event(createMockEvent({ path: "//" }))
+        .expectResolve((res) => {
+          expect(res.statusCode).toBe(200);
+          expect(res.body).toBe("URL: //");
+        });
+
+      process.env.NODE_ENV = oldEnv;
+    });
+
+    it("handles nested // requests (development)", async () => {
+      let oldEnv = process.env.NODE_ENV;
+      process.env.NODE_ENV = "development";
+
+      mockedCreateRequestHandler.mockImplementation(() => async (req) => {
+        return new Response(`URL: ${new URL(req.url).pathname}`);
+      });
+
+      // We don't have a real app to test, but it doesn't matter. We won't ever
+      // call through to the real createRequestHandler
+      // @ts-expect-error
+      await lambdaTester(createRequestHandler({ build: undefined }))
+        .event(createMockEvent({ path: "//foo//bar" }))
+        .expectResolve((res) => {
+          expect(res.statusCode).toBe(200);
+          expect(res.body).toBe("URL: //foo//bar");
+        });
+
+      process.env.NODE_ENV = oldEnv;
+    });
+
     it("handles null body", async () => {
       mockedCreateRequestHandler.mockImplementation(() => async () => {
         return new Response(null, { status: 200 });
diff --git a/packages/remix-vercel/__tests__/server-test.ts b/packages/remix-vercel/__tests__/server-test.ts
index 33697171f..59f248e90 100644
--- a/packages/remix-vercel/__tests__/server-test.ts
+++ b/packages/remix-vercel/__tests__/server-test.ts
@@ -68,6 +68,34 @@ describe("vercel createRequestHandler", () => {
       expect(res.text).toBe("URL: /foo/bar");
     });
 
+    it("handles root // requests", async () => {
+      mockedCreateRequestHandler.mockImplementation(() => async (req) => {
+        return new Response(`URL: ${new URL(req.url).pathname}`);
+      });
+
+      let request = supertest(createApp());
+      // note: vercel's createServerWithHelpers requires a x-now-bridge-request-id
+      let res = await request.get("//").set({ "x-now-bridge-request-id": "2" });
+
+      expect(res.status).toBe(200);
+      expect(res.text).toBe("URL: //");
+    });
+
+    it("handles nested // requests", async () => {
+      mockedCreateRequestHandler.mockImplementation(() => async (req) => {
+        return new Response(`URL: ${new URL(req.url).pathname}`);
+      });
+
+      let request = supertest(createApp());
+      // note: vercel's createServerWithHelpers requires a x-now-bridge-request-id
+      let res = await request
+        .get("//foo//bar")
+        .set({ "x-now-bridge-request-id": "2" });
+
+      expect(res.status).toBe(200);
+      expect(res.text).toBe("URL: //foo//bar");
+    });
+
     it("handles null body", async () => {
       mockedCreateRequestHandler.mockImplementation(() => async () => {
         return new Response(null, { status: 200 });