From 427b65f9238c17f0e8157b9c40da4101e165b8e5 Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Tue, 1 Mar 2022 09:43:02 -0700 Subject: [PATCH 1/2] refactor(http): extract logic into constructRedirectPath This allows us to easily test our redirect path construction logic where we get the relative path, the query string and construct a redirect path. By extracting this from `redirect`, we can easily test this logic in a unit test. I did this so we could test some logic where slashes in query strings should be made human-friendly for users. --- src/node/http.ts | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/src/node/http.ts b/src/node/http.ts index f006036d3924..94eb0e33389e 100644 --- a/src/node/http.ts +++ b/src/node/http.ts @@ -138,6 +138,20 @@ export const relativeRoot = (originalUrl: string): string => { return normalize("./" + (depth > 1 ? "../".repeat(depth - 1) : "")) } +/** + * A helper function to construct a redirect path based on + * an Express Request, query and a path to redirect to. + * + * Redirect path is relative to `/${to}`. + */ +export const constructRedirectPath = (req: express.Request, query: qs.ParsedQs, to: string): string => { + const relativePath = normalize(`${relativeRoot(req.originalUrl)}/${to}`, true) + const queryString = qs.stringify(query) + const redirectPath = `${relativePath}${queryString ? `?${queryString}` : ""}` + + return redirectPath +} + /** * Redirect relatively to `/${to}`. Query variables on the current URI will be * preserved. `to` should be a simple path without any query parameters @@ -156,9 +170,7 @@ export const redirect = ( } }) - const relativePath = normalize(`${relativeRoot(req.originalUrl)}/${to}`, true) - const queryString = qs.stringify(query) - const redirectPath = `${relativePath}${queryString ? `?${queryString}` : ""}` + const redirectPath = constructRedirectPath(req, query, to) logger.debug(`redirecting from ${req.originalUrl} to ${redirectPath}`) res.redirect(redirectPath) } From 31e131b8a92ae5c0d68cf314d644eb162bd7b52b Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Tue, 1 Mar 2022 09:49:59 -0700 Subject: [PATCH 2/2] feat(testing): add tests for constructRedirectPath --- src/node/http.ts | 7 +++--- test/package.json | 1 + test/unit/node/http.test.ts | 46 ++++++++++++++++++++++++++++++++++++- test/yarn.lock | 5 ++++ 4 files changed, 55 insertions(+), 4 deletions(-) diff --git a/src/node/http.ts b/src/node/http.ts index 94eb0e33389e..fa9a84255191 100644 --- a/src/node/http.ts +++ b/src/node/http.ts @@ -141,16 +141,17 @@ export const relativeRoot = (originalUrl: string): string => { /** * A helper function to construct a redirect path based on * an Express Request, query and a path to redirect to. - * + * * Redirect path is relative to `/${to}`. */ export const constructRedirectPath = (req: express.Request, query: qs.ParsedQs, to: string): string => { const relativePath = normalize(`${relativeRoot(req.originalUrl)}/${to}`, true) - const queryString = qs.stringify(query) + // %2f or %2F are both equalivent to an encoded slash / + const queryString = qs.stringify(query).replace(/%2[fF]/g, "/") const redirectPath = `${relativePath}${queryString ? `?${queryString}` : ""}` return redirectPath -} +} /** * Redirect relatively to `/${to}`. Query variables on the current URI will be diff --git a/test/package.json b/test/package.json index d50371666ba4..726251353eee 100644 --- a/test/package.json +++ b/test/package.json @@ -2,6 +2,7 @@ "license": "MIT", "#": "We must put jest in a sub-directory otherwise VS Code somehow picks up the types and generates conflicts with mocha.", "devDependencies": { + "@jest-mock/express": "^1.4.5", "@playwright/test": "^1.16.3", "@types/jest": "^27.0.2", "@types/jsdom": "^16.2.13", diff --git a/test/unit/node/http.test.ts b/test/unit/node/http.test.ts index 87e8e04199b1..b5dc20402d67 100644 --- a/test/unit/node/http.test.ts +++ b/test/unit/node/http.test.ts @@ -1,4 +1,5 @@ -import { relativeRoot } from "../../../src/node/http" +import { getMockReq } from "@jest-mock/express" +import { constructRedirectPath, relativeRoot } from "../../../src/node/http" describe("http", () => { it("should construct a relative path to the root", () => { @@ -9,3 +10,46 @@ describe("http", () => { expect(relativeRoot("/foo/bar/")).toStrictEqual("./../..") }) }) + +describe("constructRedirectPath", () => { + it("should preserve slashes in queryString so they are human-readable", () => { + const mockReq = getMockReq({ + originalUrl: "localhost:8080", + }) + const mockQueryParams = { folder: "/Users/jp/dev/coder" } + const mockTo = "" + const actual = constructRedirectPath(mockReq, mockQueryParams, mockTo) + const expected = "./?folder=/Users/jp/dev/coder" + expect(actual).toBe(expected) + }) + it("should use an empty string if no query params", () => { + const mockReq = getMockReq({ + originalUrl: "localhost:8080", + }) + const mockQueryParams = {} + const mockTo = "" + const actual = constructRedirectPath(mockReq, mockQueryParams, mockTo) + const expected = "./" + expect(actual).toBe(expected) + }) + it("should append the 'to' path relative to the originalUrl", () => { + const mockReq = getMockReq({ + originalUrl: "localhost:8080", + }) + const mockQueryParams = {} + const mockTo = "vscode" + const actual = constructRedirectPath(mockReq, mockQueryParams, mockTo) + const expected = "./vscode" + expect(actual).toBe(expected) + }) + it("should append append queryParams after 'to' path", () => { + const mockReq = getMockReq({ + originalUrl: "localhost:8080", + }) + const mockQueryParams = { folder: "/Users/jp/dev/coder" } + const mockTo = "vscode" + const actual = constructRedirectPath(mockReq, mockQueryParams, mockTo) + const expected = "./vscode?folder=/Users/jp/dev/coder" + expect(actual).toBe(expected) + }) +}) diff --git a/test/yarn.lock b/test/yarn.lock index 35377913d710..38e8e7cd93aa 100644 --- a/test/yarn.lock +++ b/test/yarn.lock @@ -478,6 +478,11 @@ resolved "https://registry.yarnpkg.com/@istanbuljs/schema/-/schema-0.1.3.tgz#e45e384e4b8ec16bce2fd903af78450f6bf7ec98" integrity sha512-ZXRY4jNvVgSVQ8DL3LTcakaAtXwTVUxE81hslsyD2AtoXW/wVob10HkOJ1X/pAlcI7D+2YoZKg5do8G/w6RYgA== +"@jest-mock/express@^1.4.5": + version "1.4.5" + resolved "https://registry.yarnpkg.com/@jest-mock/express/-/express-1.4.5.tgz#437db24ccd505d88f8c0d73e8593fa3cd6eb273b" + integrity sha512-bERM1jnutyH7VMahdaOHAKy7lgX47zJ7+RTz2eMz0wlCttd9CkhsKFEyoWmJBSz/ow0nVj3lCuRqLem4QDYFkQ== + "@jest/console@^27.4.6": version "27.4.6" resolved "https://registry.yarnpkg.com/@jest/console/-/console-27.4.6.tgz#0742e6787f682b22bdad56f9db2a8a77f6a86107"