Skip to content

Commit

Permalink
Merge pull request coder#2563 from cdr/proxy-path-passthrough-0bb9
Browse files Browse the repository at this point in the history
pathProxy.ts: Implement --proxy-path-passthrough
  • Loading branch information
nhooyr authored Jan 20, 2021
2 parents c17f3ff + c32d8b1 commit 28e98c0
Show file tree
Hide file tree
Showing 13 changed files with 277 additions and 95 deletions.
37 changes: 37 additions & 0 deletions doc/FAQ.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
- [How do I securely access web services?](#how-do-i-securely-access-web-services)
- [Sub-paths](#sub-paths)
- [Sub-domains](#sub-domains)
- [Why does the code-server proxy strip `/proxy/<port>` from the request path?](#why-does-the-code-server-proxy-strip-proxyport-from-the-request-path)
- [Multi-tenancy](#multi-tenancy)
- [Docker in code-server container?](#docker-in-code-server-container)
- [How can I disable telemetry?](#how-can-i-disable-telemetry)
Expand Down Expand Up @@ -208,6 +209,42 @@ code-server --proxy-domain <domain>
Now you can browse to `<port>.<domain>`. Note that this uses the host header so
ensure your reverse proxy forwards that information if you are using one.
## Why does the code-server proxy strip `/proxy/<port>` from the request path?
HTTP servers should strive to use relative URLs to avoid needed to be coupled to the
absolute path at which they are served. This means you must use trailing slashes on all
paths with subpaths. See https://blog.cdivilly.com/2019/02/28/uri-trailing-slashes
This is really the "correct" way things work and why the striping of the base path is the
default. If your application uses relative URLs and does not assume the absolute path at
which it is being served, it will just work no matter what port you decide to serve it off
or if you put it in behind code-server or any other proxy!
However many people prefer the cleaner aesthetic of no trailing slashes. This couples you
to the base path as you cannot use relative redirects correctly anymore. See the above
link.
For users who are ok with this tradeoff, pass `--proxy-path-passthrough` to code-server
and the path will be passed as is.
This is particularly a problem with the `start` script in create-react-app. See
[#2222](https://github.com/cdr/code-server/issues/2222). You will need to inform
create-react-app of the path at which you are serving via `homepage` field in your
`package.json`. e.g. you'd add the following for the default CRA port:
```json
"homepage": "/proxy/3000",
```

Then visit `https://my-code-server-address.io/proxy/3000` to see your app exposed through
code-server!

Unfortunately `webpack-dev-server`'s websocket connections will not go through as it
always uses `/sockjs-node`. So hot reloading will not work until the `create-react-app`
team fix this bug.

Highly recommend using the subdomain approach instead to avoid this class of issue.

## Multi-tenancy

If you want to run multiple code-servers on shared infrastructure, we recommend using virtual
Expand Down
4 changes: 2 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -38,13 +38,13 @@
"@types/js-yaml": "^3.12.3",
"@types/mocha": "^8.0.3",
"@types/node": "^12.12.7",
"@types/node-fetch": "^2.5.7",
"@types/parcel-bundler": "^1.12.1",
"@types/pem": "^1.9.5",
"@types/proxy-from-env": "^1.0.1",
"@types/safe-compare": "^1.1.0",
"@types/semver": "^7.1.0",
"@types/split2": "^2.1.6",
"@types/supertest": "^2.0.10",
"@types/tar-fs": "^2.0.0",
"@types/tar-stream": "^2.1.0",
"@types/ws": "^7.2.6",
Expand All @@ -61,7 +61,6 @@
"prettier": "^2.0.5",
"stylelint": "^13.0.0",
"stylelint-config-recommended": "^3.0.0",
"supertest": "^6.0.1",
"ts-node": "^9.0.0",
"typescript": "4.0.2"
},
Expand All @@ -81,6 +80,7 @@
"httpolyglot": "^0.1.2",
"js-yaml": "^3.13.1",
"limiter": "^1.1.5",
"node-fetch": "^2.6.1",
"pem": "^1.14.2",
"proxy-agent": "^4.0.0",
"proxy-from-env": "^1.1.0",
Expand Down
8 changes: 8 additions & 0 deletions src/common/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -112,3 +112,11 @@ export const getFirstString = (value: string | string[] | object | undefined): s

return typeof value === "string" ? value : undefined
}

export function logError(prefix: string, err: any): void {
if (err instanceof Error) {
logger.error(`${prefix}: ${err.message} ${err.stack}`)
} else {
logger.error(`${prefix}: ${err}`)
}
}
18 changes: 16 additions & 2 deletions src/node/app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import express, { Express } from "express"
import { promises as fs } from "fs"
import http from "http"
import * as httpolyglot from "httpolyglot"
import * as util from "../common/util"
import { DefaultedArgs } from "./cli"
import { handleUpgrade } from "./wsRouter"

Expand All @@ -22,8 +23,21 @@ export const createApp = async (args: DefaultedArgs): Promise<[Express, Express,
)
: http.createServer(app)

await new Promise<http.Server>(async (resolve, reject) => {
server.on("error", reject)
let resolved = false
await new Promise<http.Server>(async (resolve2, reject) => {
const resolve = () => {
resolved = true
resolve2()
}
server.on("error", (err) => {
if (!resolved) {
reject(err)
} else {
// Promise resolved earlier so this is an unrelated error.
util.logError("http server error", err)
}
})

if (args.socket) {
try {
await fs.unlink(args.socket)
Expand Down
21 changes: 19 additions & 2 deletions src/node/cli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ export interface Args extends VsArgs {
"show-versions"?: boolean
"uninstall-extension"?: string[]
"proxy-domain"?: string[]
"proxy-path-passthrough"?: boolean
locale?: string
_: string[]
"reuse-window"?: boolean
Expand Down Expand Up @@ -172,6 +173,10 @@ const options: Options<Required<Args>> = {
"uninstall-extension": { type: "string[]", description: "Uninstall a VS Code extension by id." },
"show-versions": { type: "boolean", description: "Show VS Code extension versions." },
"proxy-domain": { type: "string[]", description: "Domain used for proxying ports." },
"proxy-path-passthrough": {
type: "boolean",
description: "Whether the path proxy should leave the /proxy/<port> in the request path when proxying.",
},
"ignore-last-opened": {
type: "boolean",
short: "e",
Expand Down Expand Up @@ -239,7 +244,7 @@ export const optionDescriptions = (): string[] => {
export const parse = (
argv: string[],
opts?: {
configFile: string
configFile?: string
},
): Args => {
const error = (msg: string): Error => {
Expand Down Expand Up @@ -516,7 +521,19 @@ export async function readConfigFile(configPath?: string): Promise<ConfigArgs> {
}

const configFile = await fs.readFile(configPath)
const config = yaml.safeLoad(configFile.toString(), {
return parseConfigFile(configFile.toString(), configPath)
}

/**
* parseConfigFile parses configFile into ConfigArgs.
* configPath is used as the filename in error messages
*/
export function parseConfigFile(configFile: string, configPath: string): ConfigArgs {
if (!configFile) {
return { _: [], config: configPath }
}

const config = yaml.safeLoad(configFile, {
filename: configPath,
})
if (!config || typeof config === "string") {
Expand Down
9 changes: 9 additions & 0 deletions src/node/heart.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,4 +45,13 @@ export class Heart {
})
}, this.heartbeatInterval)
}

/**
* Call to clear any heartbeatTimer for shutdown.
*/
public dispose(): void {
if (typeof this.heartbeatTimer !== "undefined") {
clearTimeout(this.heartbeatTimer)
}
}
}
5 changes: 4 additions & 1 deletion src/node/routes/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,9 @@ export const register = async (
})
})
})
server.on("close", () => {
heart.dispose()
})

app.disable("x-powered-by")
wsApp.disable("x-powered-by")
Expand Down Expand Up @@ -165,7 +168,7 @@ export const register = async (

app.use(errorHandler)

const wsErrorHandler: express.ErrorRequestHandler = async (err, req) => {
const wsErrorHandler: express.ErrorRequestHandler = async (err, req, res, next) => {
logger.error(`${err.message} ${err.stack}`)
;(req as WebsocketRequest).ws.end()
}
Expand Down
14 changes: 7 additions & 7 deletions src/node/routes/pathProxy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,12 @@ import { Router as WsRouter } from "../wsRouter"

export const router = Router()

const getProxyTarget = (req: Request, rewrite: boolean): string => {
if (rewrite) {
const query = qs.stringify(req.query)
return `http://0.0.0.0:${req.params.port}/${req.params[0] || ""}${query ? `?${query}` : ""}`
const getProxyTarget = (req: Request, passthroughPath: boolean): string => {
if (passthroughPath) {
return `http://0.0.0.0:${req.params.port}/${req.originalUrl}`
}
return `http://0.0.0.0:${req.params.port}/${req.originalUrl}`
const query = qs.stringify(req.query)
return `http://0.0.0.0:${req.params.port}/${req.params[0] || ""}${query ? `?${query}` : ""}`
}

router.all("/(:port)(/*)?", (req, res) => {
Expand All @@ -33,7 +33,7 @@ router.all("/(:port)(/*)?", (req, res) => {

proxy.web(req, res, {
ignorePath: true,
target: getProxyTarget(req, true),
target: getProxyTarget(req, req.args["proxy-path-passthrough"] || false),
})
})

Expand All @@ -42,6 +42,6 @@ export const wsRouter = WsRouter()
wsRouter.ws("/(:port)(/*)?", ensureAuthenticated, (req) => {
proxy.ws(req, req.ws, req.head, {
ignorePath: true,
target: getProxyTarget(req, true),
target: getProxyTarget(req, req.args["proxy-path-passthrough"] || false),
})
})
72 changes: 72 additions & 0 deletions test/httpserver.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
import * as http from "http"
import * as nodeFetch from "node-fetch"
import * as util from "../src/common/util"
import { ensureAddress } from "../src/node/app"

// Perhaps an abstraction similar to this should be used in app.ts as well.
export class HttpServer {
private hs = http.createServer()

public constructor(hs?: http.Server) {
// See usage in test/integration.ts
if (hs) {
this.hs = hs
}
}

/**
* listen starts the server on a random localhost port.
* Use close to cleanup when done.
*/
public listen(fn: http.RequestListener): Promise<void> {
this.hs.on("request", fn)

let resolved = false
return new Promise((res, rej) => {
this.hs.listen(0, "localhost", () => {
res()
resolved = true
})

this.hs.on("error", (err) => {
if (!resolved) {
rej(err)
} else {
// Promise resolved earlier so this is some other error.
util.logError("http server error", err)
}
})
})
}

/**
* close cleans up the server.
*/
public close(): Promise<void> {
return new Promise((res, rej) => {
this.hs.close((err) => {
if (err) {
rej(err)
return
}
res()
})
})
}

/**
* fetch fetches the request path.
* The request path must be rooted!
*/
public fetch(requestPath: string, opts?: nodeFetch.RequestInit): Promise<nodeFetch.Response> {
return nodeFetch.default(`${ensureAddress(this.hs)}${requestPath}`, opts)
}

public port(): number {
const addr = this.hs.address()
if (addr && typeof addr === "object") {
return addr.port
}
throw new Error("server not listening or listening on unix socket")
}
}
21 changes: 21 additions & 0 deletions test/integration.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import * as express from "express"
import { createApp } from "../src/node/app"
import { parse, setDefaults, parseConfigFile, DefaultedArgs } from "../src/node/cli"
import { register } from "../src/node/routes"
import * as httpserver from "./httpserver"

export async function setup(
argv: string[],
configFile?: string,
): Promise<[express.Application, express.Application, httpserver.HttpServer, DefaultedArgs]> {
argv = ["--bind-addr=localhost:0", ...argv]

const cliArgs = parse(argv)
const configArgs = parseConfigFile(configFile || "", "test/integration.ts")
const args = await setDefaults(cliArgs, configArgs)

const [app, wsApp, server] = await createApp(args)
await register(app, wsApp, server, args)

return [app, wsApp, new httpserver.HttpServer(server), args]
}
Loading

0 comments on commit 28e98c0

Please sign in to comment.