Merge pull request #2563 from cdr/proxy-path-passthrough-0bb9

pathProxy.ts: Implement --proxy-path-passthrough

Author: nhooyr

doc/FAQ.md

@@ -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)
@@ -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

package.json

@@ -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",
@@ -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"
   },
@@ -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",

src/common/util.ts

@@ -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}`)
+  }
+}

src/node/app.ts

@@ -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"
 
@@ -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)

src/node/cli.ts

@@ -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
@@ -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",
@@ -239,7 +244,7 @@ export const optionDescriptions = (): string[] => {
 export const parse = (
   argv: string[],
   opts?: {
-    configFile: string
+    configFile?: string
   },
 ): Args => {
   const error = (msg: string): Error => {
@@ -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") {

src/node/heart.ts

@@ -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)
+    }
+  }
 }

src/node/routes/index.ts

@@ -55,6 +55,9 @@ export const register = async (
       })
     })
   })
+  server.on("close", () => {
+    heart.dispose()
+  })
 
   app.disable("x-powered-by")
   wsApp.disable("x-powered-by")
@@ -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()
   }

src/node/routes/pathProxy.ts

@@ -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) => {
@@ -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),
   })
 })
 
@@ -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),
   })
 })

test/httpserver.ts

@@ -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")
+  }
+}

test/integration.ts

@@ -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]
+}