Merge commit from fork

* Strip token from cookies before proxying

Since this functionality requires information placed onto the request by
code-server (req.args) and Express (req.cookies), move the standalone
tests into the integration tests as the proxy can no longer run
correctly on its own without that context.

We could strip the header elsewhere or refactor in some way (pass in a
callback function for the stripping or something) but this seems like
the simplest and safest place at the moment to ensure we catch all uses
of the proxy.

In any case, I think it does lend more confidence to know we are testing
the proxy the way it will be used in practice.  The downside is some
additional complexity when setting up tests, but at the moment I do not
think that exchange is overly burdensome.

* Properly stringify the cookie

Cookie values need to be encoded, and they come off the req.cookies
already decoded.

package-lock.json

@@ -13,6 +13,7 @@
         "@coder/logger": "^3.0.1",
         "argon2": "^0.44.0",
         "compression": "^1.7.4",
+        "cookie": "^1.1.1",
         "cookie-parser": "^1.4.6",
         "env-paths": "^2.2.1",
         "express": "^5.0.1",
@@ -1936,12 +1937,16 @@
       }
     },
     "node_modules/cookie": {
-      "version": "0.7.2",
-      "resolved": "https://registry.npmjs.org/cookie/-/cookie-0.7.2.tgz",
-      "integrity": "sha512-yki5XnKuf750l50uGTllt6kKILY4nQ1eNIQatoXEByZ5dWgnKqbnqmTrBE5B4N7lrMJKQ2ytWMiTO2o0v6Ew/w==",
+      "version": "1.1.1",
+      "resolved": "https://registry.npmjs.org/cookie/-/cookie-1.1.1.tgz",
+      "integrity": "sha512-ei8Aos7ja0weRpFzJnEA9UHJ/7XQmqglbRwnf2ATjcB9Wq874VKH9kfjjirM6UhU2/E5fFYadylyhFldcqSidQ==",
       "license": "MIT",
       "engines": {
-        "node": ">= 0.6"
+        "node": ">=18"
+      },
+      "funding": {
+        "type": "opencollective",
+        "url": "https://opencollective.com/express"
       }
     },
     "node_modules/cookie-parser": {
@@ -1957,6 +1962,15 @@
         "node": ">= 0.8.0"
       }
     },
+    "node_modules/cookie-parser/node_modules/cookie": {
+      "version": "0.7.2",
+      "resolved": "https://registry.npmjs.org/cookie/-/cookie-0.7.2.tgz",
+      "integrity": "sha512-yki5XnKuf750l50uGTllt6kKILY4nQ1eNIQatoXEByZ5dWgnKqbnqmTrBE5B4N7lrMJKQ2ytWMiTO2o0v6Ew/w==",
+      "license": "MIT",
+      "engines": {
+        "node": ">= 0.6"
+      }
+    },
     "node_modules/cookie-signature": {
       "version": "1.0.6",
       "resolved": "https://registry.npmjs.org/cookie-signature/-/cookie-signature-1.0.6.tgz",
@@ -2953,6 +2967,15 @@
         "url": "https://opencollective.com/express"
       }
     },
+    "node_modules/express/node_modules/cookie": {
+      "version": "0.7.2",
+      "resolved": "https://registry.npmjs.org/cookie/-/cookie-0.7.2.tgz",
+      "integrity": "sha512-yki5XnKuf750l50uGTllt6kKILY4nQ1eNIQatoXEByZ5dWgnKqbnqmTrBE5B4N7lrMJKQ2ytWMiTO2o0v6Ew/w==",
+      "license": "MIT",
+      "engines": {
+        "node": ">= 0.6"
+      }
+    },
     "node_modules/express/node_modules/cookie-signature": {
       "version": "1.2.2",
       "resolved": "https://registry.npmjs.org/cookie-signature/-/cookie-signature-1.2.2.tgz",

package.json

@@ -70,6 +70,7 @@
     "@coder/logger": "^3.0.1",
     "argon2": "^0.44.0",
     "compression": "^1.7.4",
+    "cookie": "^1.1.1",
     "cookie-parser": "^1.4.6",
     "env-paths": "^2.2.1",
     "express": "^5.0.1",

src/node/proxy.ts

@@ -1,5 +1,7 @@
+import * as cookie from "cookie"
+import type { Request } from "express"
 import proxyServer from "http-proxy"
-import { HttpCode } from "../common/http"
+import { getCookieSessionName, HttpCode } from "../common/http"
 
 export const proxy = proxyServer.createProxyServer({})
 
@@ -18,6 +20,16 @@ proxy.on("error", (error, _, res) => {
   }
 })
 
+// Strip the code-server cookie if it exists to avoid transmitting the cookie
+// to potentially malicious local ports.
+proxy.on("proxyReq", (preq, req) => {
+  const cookieSessionName = getCookieSessionName((req as Request).args["cookie-suffix"])
+  preq.setHeader("Cookie", cookie.stringifyCookie({
+    ...(req as Request).cookies,
+    [cookieSessionName]: undefined,
+  }))
+})
+
 // Intercept the response to rewrite absolute redirects against the base path.
 // Is disabled when the request has no base path which means /absproxy is in use.
 proxy.on("proxyRes", (res, req) => {

test/unit/node/proxy.test.ts

@@ -1,15 +1,12 @@
 import * as express from "express"
-import * as http from "http"
-import nodeFetch from "node-fetch"
 import { HttpCode } from "../../../src/common/http"
-import { proxy } from "../../../src/node/proxy"
 import { wss, Router as WsRouter } from "../../../src/node/wsRouter"
-import { getAvailablePort, mockLogger } from "../../utils/helpers"
+import { mockLogger } from "../../utils/helpers"
 import * as httpserver from "../../utils/httpserver"
 import * as integration from "../../utils/integration"
 
 describe("proxy", () => {
-  const nhooyrDevServer = new httpserver.HttpServer()
+  const proxyTarget = new httpserver.HttpServer()
   const wsApp = express.default()
   const wsRouter = WsRouter()
   let codeServer: httpserver.HttpServer | undefined
@@ -19,21 +16,22 @@ describe("proxy", () => {
 
   beforeAll(async () => {
     wsApp.use("/", wsRouter.router)
-    await nhooyrDevServer.listen((req, res) => {
+    await proxyTarget.listen((req, res) => {
       e(req, res)
     })
-    nhooyrDevServer.listenUpgrade(wsApp)
-    proxyPath = `/proxy/${nhooyrDevServer.port()}/wsup`
+    proxyTarget.listenUpgrade(wsApp)
+    proxyPath = `/proxy/${proxyTarget.port()}/wsup`
     absProxyPath = proxyPath.replace("/proxy/", "/absproxy/")
   })
 
   afterAll(async () => {
-    await nhooyrDevServer.dispose()
+    await proxyTarget.dispose()
   })
 
   beforeEach(() => {
     e = express.default()
     mockLogger()
+    delete process.env.PASSWORD
   })
 
   afterEach(async () => {
@@ -283,65 +281,42 @@ describe("proxy", () => {
     const resp = await codeServer.fetch(proxyPath, { method: "OPTIONS" })
     expect(resp.status).toBe(200)
   })
-})
 
-// NOTE@jsjoeio
-// Both this test suite and the one above it are very similar
-// The main difference is this one uses http and node-fetch
-// and specifically tests the proxy in isolation vs. using
-// the httpserver abstraction we've built.
-//
-// Leaving this as a separate test suite for now because
-// we may consider refactoring the httpserver abstraction
-// in the future.
-//
-// If you're writing a test specifically for code in
-// src/node/proxy.ts, you should probably add it to
-// this test suite.
-describe("proxy (standalone)", () => {
-  let URL = ""
-  let PROXY_URL = ""
-  let testServer: http.Server
-  let proxyTarget: http.Server
+  it("should return a 500 when no target is running ", async () => {
+    const target = new httpserver.HttpServer()
+    await target.listen(() => {})
+    const port = target.port()
+    target.dispose()
+    codeServer = await integration.setup(["--auth=none"], "")
+    const resp = await codeServer.fetch(`/proxy/${port}/wsup`)
+    expect(resp.status).toBe(HttpCode.ServerError)
+    expect(resp.statusText).toBe("Internal Server Error")
+  })
 
-  beforeEach(async () => {
-    const PORT = await getAvailablePort()
-    const PROXY_PORT = await getAvailablePort()
-    URL = `http://localhost:${PORT}`
-    PROXY_URL = `http://localhost:${PROXY_PORT}`
-    // Define server and a proxy server
-    testServer = http.createServer((req, res) => {
-      proxy.web(req, res, {
-        target: PROXY_URL,
-      })
+  it("should strip token cookie", async () => {
+    const token = "my-super-secure-token"
+    process.env.HASHED_PASSWORD = token
+    codeServer = await integration.setup(["--auth=password"])
+
+    // Set up a listener that just prints the cookies it got.
+    e.get("/wsup/cookies", (req, res) => {
+      res.writeHead(HttpCode.Ok, { "Content-Type": "text/plain" })
+      res.end(req.headers.cookie)
     })
 
-    proxyTarget = http.createServer((req, res) => {
-      res.writeHead(200, { "Content-Type": "text/plain" })
-      res.end()
+    // Send the token along with other cookies which should be preserved.
+    // Encode one to make sure they are being re-encoded properly.
+    const value = "hello=there"
+    const encodedValue = encodeURIComponent(value)
+    const resp = await codeServer.fetch(proxyPath + "/cookies", {
+      headers: {
+        cookie: `cookie1=${encodedValue}; code-server-session=${token}; cookie2=hello;`,
+      },
     })
 
-    // Start both servers
-    proxyTarget.listen(PROXY_PORT)
-    testServer.listen(PORT)
-  })
-
-  afterEach(async () => {
-    testServer.close()
-    proxyTarget.close()
-  })
-
-  it("should return a 500 when proxy target errors ", async () => {
-    // Close the proxy target so that proxy errors
-    proxyTarget.close()
-    const errorResp = await nodeFetch(`${URL}/error`)
-    expect(errorResp.status).toBe(HttpCode.ServerError)
-    expect(errorResp.statusText).toBe("Internal Server Error")
-  })
-
-  it("should proxy correctly", async () => {
-    const resp = await nodeFetch(`${URL}/route`)
+    // The proxied listener should not have printed the code-server token.
     expect(resp.status).toBe(200)
-    expect(resp.statusText).toBe("OK")
+    const text = await resp.text()
+    expect(text).toBe(`cookie1=${encodedValue}; cookie2=hello`)
   })
 })