diff --git a/package-lock.json b/package-lock.json index b3ef6a0f2..eac3f50dd 100644 --- a/package-lock.json +++ b/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", diff --git a/package.json b/package.json index b229efb3f..35fae5970 100644 --- a/package.json +++ b/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", diff --git a/src/node/proxy.ts b/src/node/proxy.ts index afa964ae5..243d291d7 100644 --- a/src/node/proxy.ts +++ b/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) => { diff --git a/test/unit/node/proxy.test.ts b/test/unit/node/proxy.test.ts index b3509ed64..94945cb94 100644 --- a/test/unit/node/proxy.test.ts +++ b/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`) }) })