From f5cf3fd33187b3fa29dc762075152435244fa8f2 Mon Sep 17 00:00:00 2001 From: Anmol Sethi Date: Wed, 20 Jan 2021 14:26:15 -0500 Subject: [PATCH 1/3] proxy.ts: Do not always rewrite redirects against the base path This breaks --proxy-path-passthrough However, we still need this when that code is disabled as many apps will issue absolute redirects and expect the proxy to rewrite as appropriate. e.g. Go's http.Redirect will rewrite relative redirects as absolute! See https://golang.org/pkg/net/http/#Redirect --- src/node/proxy.ts | 2 ++ src/node/routes/pathProxy.ts | 6 ++++-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/node/proxy.ts b/src/node/proxy.ts index da430f5b3..35fd5d81a 100644 --- a/src/node/proxy.ts +++ b/src/node/proxy.ts @@ -9,6 +9,8 @@ proxy.on("error", (error, _, res) => { }) // Intercept the response to rewrite absolute redirects against the base path. +// Is disabled when the request has no base path which means --proxy-path-passthrough has +// been enabled. proxy.on("proxyRes", (res, req) => { if (res.headers.location && res.headers.location.startsWith("/") && (req as any).base) { res.headers.location = (req as any).base + res.headers.location diff --git a/src/node/routes/pathProxy.ts b/src/node/routes/pathProxy.ts index 9554a006d..6a6b9d29c 100644 --- a/src/node/routes/pathProxy.ts +++ b/src/node/routes/pathProxy.ts @@ -28,8 +28,10 @@ router.all("/(:port)(/*)?", (req, res) => { throw new HttpError("Unauthorized", HttpCode.Unauthorized) } - // Absolute redirects need to be based on the subpath when rewriting. - ;(req as any).base = `${req.baseUrl}/${req.params.port}` + if (!req.args["proxy-path-passthrough"]) { + // Absolute redirects need to be based on the subpath when rewriting. + ;(req as any).base = `${req.baseUrl}/${req.params.port}` + } proxy.web(req, res, { ignorePath: true, From 58d72d53a1cb36fb39592bdaaea2e37106fadebf Mon Sep 17 00:00:00 2001 From: Anmol Sethi Date: Wed, 20 Jan 2021 17:32:47 -0500 Subject: [PATCH 2/3] routes/index.ts: register proxy routes before body-parser Any json or urlencoded request bodies were being consumed by body-parser before they could be proxied. That's why requests without Content-Type were proxied correctly as body-parser would not consume their body. This allows the http-proxy package to passthrough the request body correctly in all instances. Closes #2377 --- src/node/routes/index.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/node/routes/index.ts b/src/node/routes/index.ts index 006543679..a3fb8bd4f 100644 --- a/src/node/routes/index.ts +++ b/src/node/routes/index.ts @@ -65,9 +65,6 @@ export const register = async ( app.use(cookieParser()) wsApp.use(cookieParser()) - app.use(bodyParser.json()) - app.use(bodyParser.urlencoded({ extended: true })) - const common: express.RequestHandler = (req, _, next) => { // /healthz|/healthz/ needs to be excluded otherwise health checks will make // it look like code-server is always in use. @@ -106,6 +103,12 @@ export const register = async ( app.use("/", domainProxy.router) wsApp.use("/", domainProxy.wsRouter.router) + app.use("/proxy", proxy.router) + wsApp.use("/proxy", proxy.wsRouter.router) + + app.use(bodyParser.json()) + app.use(bodyParser.urlencoded({ extended: true })) + app.use("/", vscode.router) wsApp.use("/", vscode.wsRouter.router) app.use("/vscode", vscode.router) @@ -121,9 +124,6 @@ export const register = async ( }) } - app.use("/proxy", proxy.router) - wsApp.use("/proxy", proxy.wsRouter.router) - app.use("/static", _static.router) app.use("/update", update.router) From a60f61f9b36c02313d3171f2418ea0c4df1205b6 Mon Sep 17 00:00:00 2001 From: Anmol Sethi Date: Wed, 20 Jan 2021 17:19:39 -0500 Subject: [PATCH 3/3] proxy.test.ts: Add POST body test and redirection tests Closes #2377 --- test/proxy.test.ts | 71 ++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 63 insertions(+), 8 deletions(-) diff --git a/test/proxy.test.ts b/test/proxy.test.ts index b419e7929..0ef5fd796 100644 --- a/test/proxy.test.ts +++ b/test/proxy.test.ts @@ -1,28 +1,29 @@ +import bodyParser from "body-parser" import * as express from "express" import * as httpserver from "./httpserver" import * as integration from "./integration" describe("proxy", () => { - let codeServer: httpserver.HttpServer | undefined const nhooyrDevServer = new httpserver.HttpServer() + let codeServer: httpserver.HttpServer | undefined let proxyPath: string + let e: express.Express beforeAll(async () => { - const e = express.default() - await nhooyrDevServer.listen(e) - e.get("/wsup", (req, res) => { - res.json("asher is the best") + await nhooyrDevServer.listen((req, res) => { + e(req, res) }) proxyPath = `/proxy/${nhooyrDevServer.port()}/wsup` - e.get(proxyPath, (req, res) => { - res.json("joe is the best") - }) }) afterAll(async () => { await nhooyrDevServer.close() }) + beforeEach(() => { + e = express.default() + }) + afterEach(async () => { if (codeServer) { await codeServer.close() @@ -31,6 +32,9 @@ describe("proxy", () => { }) it("should rewrite the base path", async () => { + e.get("/wsup", (req, res) => { + res.json("asher is the best") + }) ;[, , codeServer] = await integration.setup(["--auth=none"], "") const resp = await codeServer.fetch(proxyPath) expect(resp.status).toBe(200) @@ -39,10 +43,61 @@ describe("proxy", () => { }) it("should not rewrite the base path", async () => { + e.get(proxyPath, (req, res) => { + res.json("joe is the best") + }) ;[, , codeServer] = await integration.setup(["--auth=none", "--proxy-path-passthrough=true"], "") const resp = await codeServer.fetch(proxyPath) expect(resp.status).toBe(200) const json = await resp.json() expect(json).toBe("joe is the best") }) + + it("should rewrite redirects", async () => { + e.post("/wsup", (req, res) => { + res.redirect(307, "/finale") + }) + e.post("/finale", (req, res) => { + res.json("redirect success") + }) + ;[, , codeServer] = await integration.setup(["--auth=none"], "") + const resp = await codeServer.fetch(proxyPath, { + method: "POST", + }) + expect(resp.status).toBe(200) + expect(await resp.json()).toBe("redirect success") + }) + + it("should not rewrite redirects", async () => { + const finalePath = proxyPath.replace("/wsup", "/finale") + e.post(proxyPath, (req, res) => { + res.redirect(307, finalePath) + }) + e.post(finalePath, (req, res) => { + res.json("redirect success") + }) + ;[, , codeServer] = await integration.setup(["--auth=none", "--proxy-path-passthrough=true"], "") + const resp = await codeServer.fetch(proxyPath, { + method: "POST", + }) + expect(resp.status).toBe(200) + expect(await resp.json()).toBe("redirect success") + }) + + it("should allow post bodies", async () => { + e.use(bodyParser.json({ strict: false })) + e.post("/wsup", (req, res) => { + res.json(req.body) + }) + ;[, , codeServer] = await integration.setup(["--auth=none"], "") + const resp = await codeServer.fetch(proxyPath, { + method: "post", + body: JSON.stringify("coder is the best"), + headers: { + "Content-Type": "application/json", + }, + }) + expect(resp.status).toBe(200) + expect(await resp.json()).toBe("coder is the best") + }) })