Skip to content

Fix body proxying, redirect proxying and add tests #2609

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Feb 1, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/node/proxy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
12 changes: 6 additions & 6 deletions src/node/routes/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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)
Expand All @@ -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)

Expand Down
6 changes: 4 additions & 2 deletions src/node/routes/pathProxy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
71 changes: 63 additions & 8 deletions test/proxy.test.ts
Original file line number Diff line number Diff line change
@@ -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()
Expand All @@ -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)
Expand All @@ -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")
})
})