diff --git a/deployer/aws-lambda/content-origin-request/index.js b/deployer/aws-lambda/content-origin-request/index.js index 46704ff17d64..9f962f408d43 100644 --- a/deployer/aws-lambda/content-origin-request/index.js +++ b/deployer/aws-lambda/content-origin-request/index.js @@ -78,6 +78,27 @@ exports.handler = async (event) => { const host = request.headers.host[0].value.toLowerCase(); const qs = request.querystring ? `?${request.querystring}` : ""; + // If the URL was something like `https://domain/en-US/search/`, our code + // would make a that a redirect to `/en-US/search` (stripping the trailing slash). + // But if it was `https://domain/en-US/search/` it'd make a redirect + // to `//en-US/search`. + // However, if pathname starts with `//` the Location header might look + // relative but it's actually an absolute URL. + // A 302 redirect from `https://domain//evil.com/` actually ends open + // opening `https://evil.com/` in the browser, because the browser will + // treat `//evil.com/ == https://evil.com/`. + // Prevent any pathnames that start with a double //. + if (request.uri.startsWith("//")) { + return { + status: 404, + statusDescription: "Not found", + headers: { + "content-type": [{ key: "Content-Type", value: "text/plain" }], + }, + body: "URL pathname can't start with //\n", + }; + } + const { url, status } = resolveFundamental(request.uri); if (url) { // TODO: Do we want to add the query string to the redirect? diff --git a/deployer/aws-lambda/content-origin-request/server.js b/deployer/aws-lambda/content-origin-request/server.js index c082f4806a7a..03c9269a7e76 100644 --- a/deployer/aws-lambda/content-origin-request/server.js +++ b/deployer/aws-lambda/content-origin-request/server.js @@ -43,9 +43,6 @@ async function catchall(req, res) { }; event.Records = [{ cf }]; - // console.log("EVENT..."); - // console.log(event); - // console.log(JSON.stringify(event, null, 3)); const handle = await handler(event); if (handle === cf.request) { // The request is allowed to pass through. @@ -59,26 +56,39 @@ async function catchall(req, res) { res.end(`${msg}\n`); } else if (handle.status) { // It's a redirect. - // console.log(JSON.stringify(handle, null, 3)); - let path = null; - for (const headers of Object.values(handle.headers)) { + let location = null; + for (const headers of Object.values(handle.headers || {})) { for (const header of headers) { res.setHeader(header.key, header.value); if (header.key.toLowerCase() === "location") { - path = header.value; + location = header.value; } } } - console.log( - `${ - handle.status === 302 - ? kleur.green(handle.status) - : kleur.yellow(handle.status) - } redirect to ${kleur.bold(path)}` - ); + if (handle.status >= 300 && handle.status < 400) { + console.log( + `${ + handle.status === 302 + ? kleur.green(handle.status) + : kleur.yellow(handle.status) + } redirect to ${kleur.bold(location)}` + ); + if (location) { + res.setHeader("Location", location); + } + } else if (handle.status >= 400) { + console.log( + `${ + handle.status >= 500 + ? kleur.red(handle.status) + : kleur.yellow(handle.status) + } '${handle.body.trim()}'` + ); + } else { + console.log(`${kleur.gree(handle.status)} '${handle.body.trim()}'`); + } res.statusCode = handle.status; - res.setHeader("Location", path); - res.end(""); + res.end(handle.body || ""); } else { console.warn(JSON.stringify(handle, null, 3)); res.statusCode = 500; diff --git a/deployer/aws-lambda/content-origin-request/server.test.js b/deployer/aws-lambda/content-origin-request/server.test.js index 8d51b84bd75f..15d2bb580803 100644 --- a/deployer/aws-lambda/content-origin-request/server.test.js +++ b/deployer/aws-lambda/content-origin-request/server.test.js @@ -13,6 +13,7 @@ async function get(uri, headers = {}) { headers, followRedirect: false, retry: 0, + throwHttpErrors: false, }); return response; } @@ -186,3 +187,12 @@ describe("always check for fundamental redirects first", () => { } }); }); + +describe("avoid double-slash redirects", () => { + it("should 404 on any pathname that starts with //", async () => { + const r = await get(`//en-US/search/`); + expect(r.statusCode).toBe(404); + expect(r.headers["location"]).toBeFalsy(); + expect(r.body).toContain("URL pathname can't start with //"); + }); +});