From 506d3f43ed86dab20f9c7ab2e2e3b8713827dbe4 Mon Sep 17 00:00:00 2001
From: Joe Previte <jjprevite@gmail.com>
Date: Tue, 1 Mar 2022 12:11:56 -0700
Subject: [PATCH] feat(http): keep slashes in queryParams in redirects (#4928)

* 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.

* feat(testing): add tests for constructRedirectPath

Co-authored-by: Asher <ash@coder.com>
---
 src/node/http.ts            | 19 ++++++++++++---
 test/package.json           |  1 +
 test/unit/node/http.test.ts | 46 ++++++++++++++++++++++++++++++++++++-
 test/yarn.lock              |  5 ++++
 4 files changed, 67 insertions(+), 4 deletions(-)

diff --git a/src/node/http.ts b/src/node/http.ts
index f006036d3..fa9a84255 100644
--- a/src/node/http.ts
+++ b/src/node/http.ts
@@ -138,6 +138,21 @@ 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)
+  // %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
  * preserved.  `to` should be a simple path without any query parameters
@@ -156,9 +171,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)
 }
diff --git a/test/package.json b/test/package.json
index d50371666..726251353 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 87e8e0419..b5dc20402 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 35377913d..38e8e7cd9 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"