Fix query string being double-encoding over path proxy

Instead of trying to piece together the original URL and re-encode what
needs to be re-encoded, strip out the base from the original URL.

Fixes coder#6307.

Author: code-asher

src/node/routes/pathProxy.ts

@@ -1,17 +1,14 @@
 import { Request, Response } from "express"
 import * as path from "path"
-import * as qs from "qs"
 import * as pluginapi from "../../../typings/pluginapi"
 import { HttpCode, HttpError } from "../../common/http"
 import { ensureProxyEnabled, authenticated, ensureAuthenticated, ensureOrigin, redirect, self } from "../http"
 import { proxy as _proxy } from "../proxy"
 
-const getProxyTarget = (req: Request, passthroughPath?: boolean): string => {
-  if (passthroughPath) {
-    return `http://0.0.0.0:${req.params.port}/${req.originalUrl}`
-  }
-  const query = qs.stringify(req.query)
-  return encodeURI(`http://0.0.0.0:${req.params.port}${req.params[0] || ""}${query ? `?${query}` : ""}`)
+const getProxyTarget = (req: Request): string => {
+  // If there is a base path, strip it out.
+  const base = (req as any).base || ""
+  return `http://0.0.0.0:${req.params.port}/${req.originalUrl.slice(base.length)}`
 }
 
 export async function proxy(
@@ -34,15 +31,14 @@ export async function proxy(
     throw new HttpError("Unauthorized", HttpCode.Unauthorized)
   }
 
+  // The base is used for rewriting (redirects, target).
   if (!opts?.passthroughPath) {
-    // Absolute redirects need to be based on the subpath when rewriting.
-    // See proxy.ts.
     ;(req as any).base = req.path.split(path.sep).slice(0, 3).join(path.sep)
   }
 
   _proxy.web(req, res, {
     ignorePath: true,
-    target: getProxyTarget(req, opts?.passthroughPath),
+    target: getProxyTarget(req),
   })
 }
 
@@ -55,8 +51,14 @@ export async function wsProxy(
   ensureProxyEnabled(req)
   ensureOrigin(req)
   await ensureAuthenticated(req)
+
+  // The base is used for rewriting (redirects, target).
+  if (!opts?.passthroughPath) {
+    ;(req as any).base = req.path.split(path.sep).slice(0, 3).join(path.sep)
+  }
+
   _proxy.ws(req, req.ws, req.head, {
     ignorePath: true,
-    target: getProxyTarget(req, opts?.passthroughPath),
+    target: getProxyTarget(req),
   })
 }

test/unit/node/proxy.test.ts

@@ -208,6 +208,54 @@ describe("proxy", () => {
     const json = await resp.json()
     expect(json).toBe("ほげ")
   })
+
+  it("should not double-encode query variables", async () => {
+    const spy = jest.fn()
+    e.get("*", (req, res) => {
+      spy([req.originalUrl, req.query])
+      res.end()
+    })
+    codeServer = await integration.setup(["--auth=none"], "")
+    for (const test of [
+      {
+        endpoint: proxyPath,
+        query: { foo: "bar with spaces" },
+        expected: "/wsup?foo=bar+with+spaces",
+      },
+      {
+        endpoint: absProxyPath,
+        query: { foo: "bar with spaces" },
+        expected: absProxyPath + "?foo=bar+with+spaces",
+      },
+      {
+        endpoint: proxyPath,
+        query: { foo: "with-&-ampersand" },
+        expected: "/wsup?foo=with-%26-ampersand",
+      },
+      {
+        endpoint: absProxyPath,
+        query: { foo: "with-&-ampersand" },
+        expected: absProxyPath + "?foo=with-%26-ampersand",
+      },
+      {
+        endpoint: absProxyPath,
+        query: { foo: "ほげ ほげ" },
+        expected: absProxyPath + "?foo=%E3%81%BB%E3%81%92+%E3%81%BB%E3%81%92",
+      },
+      {
+        endpoint: proxyPath,
+        query: { foo: "ほげ ほげ" },
+        expected: "/wsup?foo=%E3%81%BB%E3%81%92+%E3%81%BB%E3%81%92",
+      },
+    ]) {
+      spy.mockClear()
+      const resp = await codeServer.fetch(test.endpoint, undefined, test.query)
+      expect(resp.status).toBe(200)
+      await resp.text()
+      expect(spy).toHaveBeenCalledTimes(1)
+      expect(spy).toHaveBeenCalledWith([test.expected, test.query])
+    }
+  })
 })
 
 // NOTE@jsjoeio