From 7bb4fbe9d89cacd68a1fcb2ff753455fdf049a3a Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Tue, 31 Jan 2023 16:17:40 -0500 Subject: [PATCH] Fix adapter request creation when double slashes exist --- .changeset/nine-apricots-breathe.md | 8 ++ .../remix-architect/__tests__/server-test.ts | 32 ++++++++ packages/remix-architect/server.ts | 2 +- .../remix-express/__tests__/server-test.ts | 24 ++++++ packages/remix-express/server.ts | 3 +- .../remix-netlify/__tests__/server-test.ts | 78 ++++++++++++++++++- packages/remix-netlify/server.ts | 2 +- .../remix-vercel/__tests__/server-test.ts | 28 +++++++ packages/remix-vercel/server.ts | 2 +- 9 files changed, 173 insertions(+), 6 deletions(-) create mode 100644 .changeset/nine-apricots-breathe.md diff --git a/.changeset/nine-apricots-breathe.md b/.changeset/nine-apricots-breathe.md new file mode 100644 index 00000000000..f25cd7ab641 --- /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/__tests__/server-test.ts b/packages/remix-architect/__tests__/server-test.ts index 96927575444..2d02c513697 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-architect/server.ts b/packages/remix-architect/server.ts index 45e97186931..cbbfa9f1bd6 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/__tests__/server-test.ts b/packages/remix-express/__tests__/server-test.ts index dbf5cf9d5a4..1e50480ce64 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-express/server.ts b/packages/remix-express/server.ts index 4f6fe881feb..009652fd36c 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/__tests__/server-test.ts b/packages/remix-netlify/__tests__/server-test.ts index a5f0998d038..794c088a85b 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 { 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-netlify/server.ts b/packages/remix-netlify/server.ts index ccd60100950..5932d3f041d 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/__tests__/server-test.ts b/packages/remix-vercel/__tests__/server-test.ts index 33697171ff5..59f248e906f 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 }); diff --git a/packages/remix-vercel/server.ts b/packages/remix-vercel/server.ts index 2827b96f69d..6707693cbe5 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();