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 #6307.

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}` : ""}`)
+  // 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,47 @@ 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