Move proxy logic into main HTTP server

This makes the code much more internally consistent (providers just
return payloads, include the proxy provider).

src/node/app/proxy.ts

@@ -1,46 +1,12 @@
-import { logger } from "@coder/logger"
 import * as http from "http"
-import proxy from "http-proxy"
-import * as net from "net"
-import * as querystring from "querystring"
 import { HttpCode, HttpError } from "../../common/http"
-import { HttpProvider, HttpProviderOptions, HttpProxyProvider, HttpResponse, Route } from "../http"
+import { HttpProvider, HttpResponse, Route, WsResponse } from "../http"
-
-interface Request extends http.IncomingMessage {
-  base?: string
-}
 
 /**
  * Proxy HTTP provider.
  */
-export class ProxyHttpProvider extends HttpProvider implements HttpProxyProvider {
+export class ProxyHttpProvider extends HttpProvider {
-  /**
+  public async handleRequest(route: Route, request: http.IncomingMessage): Promise<HttpResponse> {
-   * Proxy domains are stored here without the leading `*.`
-   */
-  public readonly proxyDomains: Set<string>
-  private readonly proxy = proxy.createProxyServer({})
-
-  /**
-   * Domains can be provided in the form `coder.com` or `*.coder.com`. Either
-   * way, `<number>.coder.com` will be proxied to `number`.
-   */
-  public constructor(options: HttpProviderOptions, proxyDomains: string[] = []) {
-    super(options)
-    this.proxyDomains = new Set(proxyDomains.map((d) => d.replace(/^\*\./, "")))
-    this.proxy.on("error", (error) => logger.warn(error.message))
-    // Intercept the response to rewrite absolute redirects against the base path.
-    this.proxy.on("proxyRes", (response, request: Request) => {
-      if (response.headers.location && response.headers.location.startsWith("/") && request.base) {
-        response.headers.location = request.base + response.headers.location
-      }
-    })
-  }
-
-  public async handleRequest(
-    route: Route,
-    request: http.IncomingMessage,
-    response: http.ServerResponse,
-  ): Promise<HttpResponse> {
     if (!this.authenticated(request)) {
       if (this.isRoot(route)) {
         return { redirect: "/login", query: { to: route.fullPath } }
@@ -56,133 +22,22 @@ export class ProxyHttpProvider extends HttpProvider implements HttpProxyProvider
     }
 
     const port = route.base.replace(/^\//, "")
-    const base = `${this.options.base}/${port}`
+    return {
-    const payload = this.doProxy(route, request, response, port, base)
+      proxy: {
-    if (payload) {
+        base: `${this.options.base}/${port}`,
-      return payload
+        port,
+      },
     }
-
-    throw new HttpError("Not found", HttpCode.NotFound)
   }
 
-  public async handleWebSocket(
+  public async handleWebSocket(route: Route, request: http.IncomingMessage): Promise<WsResponse> {
-    route: Route,
-    request: http.IncomingMessage,
-    socket: net.Socket,
-    head: Buffer,
-  ): Promise<void> {
     this.ensureAuthenticated(request)
     const port = route.base.replace(/^\//, "")
-    const base = `${this.options.base}/${port}`
+    return {
-    this.doProxy(route, request, { socket, head }, port, base)
+      proxy: {
-  }
+        base: `${this.options.base}/${port}`,
-
+        port,
-  public getCookieDomain(host: string): string {
+      },
-    let current: string | undefined
-    this.proxyDomains.forEach((domain) => {
-      if (host.endsWith(domain) && (!current || domain.length < current.length)) {
-        current = domain
-      }
-    })
-    // Setting the domain to localhost doesn't seem to work for subdomains (for
-    // example dev.localhost).
-    return current && current !== "localhost" ? current : host
-  }
-
-  public maybeProxyRequest(
-    route: Route,
-    request: http.IncomingMessage,
-    response: http.ServerResponse,
-  ): HttpResponse | undefined {
-    const port = this.getPort(request)
-    return port ? this.doProxy(route, request, response, port) : undefined
-  }
-
-  public maybeProxyWebSocket(
-    route: Route,
-    request: http.IncomingMessage,
-    socket: net.Socket,
-    head: Buffer,
-  ): HttpResponse | undefined {
-    const port = this.getPort(request)
-    return port ? this.doProxy(route, request, { socket, head }, port) : undefined
-  }
-
-  private getPort(request: http.IncomingMessage): string | undefined {
-    // No proxy until we're authenticated. This will cause the login page to
-    // show as well as let our assets keep loading normally.
-    if (!this.authenticated(request)) {
-      return undefined
     }
-
-    // Split into parts.
-    const host = request.headers.host || ""
-    const idx = host.indexOf(":")
-    const domain = idx !== -1 ? host.substring(0, idx) : host
-    const parts = domain.split(".")
-
-    // There must be an exact match.
-    const port = parts.shift()
-    const proxyDomain = parts.join(".")
-    if (!port || !this.proxyDomains.has(proxyDomain)) {
-      return undefined
-    }
-
-    return port
-  }
-
-  private doProxy(
-    route: Route,
-    request: http.IncomingMessage,
-    response: http.ServerResponse,
-    portStr: string,
-    base?: string,
-  ): HttpResponse
-  private doProxy(
-    route: Route,
-    request: http.IncomingMessage,
-    response: { socket: net.Socket; head: Buffer },
-    portStr: string,
-    base?: string,
-  ): HttpResponse
-  private doProxy(
-    route: Route,
-    request: http.IncomingMessage,
-    response: http.ServerResponse | { socket: net.Socket; head: Buffer },
-    portStr: string,
-    base?: string,
-  ): HttpResponse {
-    const port = parseInt(portStr, 10)
-    if (isNaN(port)) {
-      return {
-        code: HttpCode.BadRequest,
-        content: `"${portStr}" is not a valid number`,
-      }
-    }
-
-    // REVIEW: Absolute redirects need to be based on the subpath but I'm not
-    // sure how best to get this information to the `proxyRes` event handler.
-    // For now I'm sticking it on the request object which is passed through to
-    // the event.
-    ;(request as Request).base = base
-
-    const isHttp = response instanceof http.ServerResponse
-    const path = base ? route.fullPath.replace(base, "") : route.fullPath
-    const options: proxy.ServerOptions = {
-      changeOrigin: true,
-      ignorePath: true,
-      target: `${isHttp ? "http" : "ws"}://127.0.0.1:${port}${path}${
-        Object.keys(route.query).length > 0 ? `?${querystring.stringify(route.query)}` : ""
-      }`,
-      ws: !isHttp,
-    }
-
-    if (response instanceof http.ServerResponse) {
-      this.proxy.web(request, response, options)
-    } else {
-      this.proxy.ws(request, response.socket, response.head, options)
-    }
-
-    return { handled: true }
   }
 }

src/node/entry.ts

@@ -43,6 +43,7 @@ const main = async (args: Args): Promise<void> => {
     host: args.host || (args.auth === AuthType.Password && typeof args.cert !== "undefined" ? "0.0.0.0" : "localhost"),
     password: originalPassword ? hash(originalPassword) : undefined,
     port: typeof args.port !== "undefined" ? args.port : process.env.PORT ? parseInt(process.env.PORT, 10) : 8080,
+    proxyDomains: args["proxy-domain"],
     socket: args.socket,
     ...(args.cert && !args.cert.value
       ? await generateCertificate()
@@ -60,11 +61,10 @@ const main = async (args: Args): Promise<void> => {
   const vscode = httpServer.registerHttpProvider("/", VscodeHttpProvider, args)
   const api = httpServer.registerHttpProvider("/api", ApiHttpProvider, httpServer, vscode, args["user-data-dir"])
   const update = httpServer.registerHttpProvider("/update", UpdateHttpProvider, !args["disable-updates"])
-  const proxy = httpServer.registerHttpProvider("/proxy", ProxyHttpProvider, args["proxy-domain"])
+  httpServer.registerHttpProvider("/proxy", ProxyHttpProvider)
   httpServer.registerHttpProvider("/login", LoginHttpProvider)
   httpServer.registerHttpProvider("/static", StaticHttpProvider)
   httpServer.registerHttpProvider("/dashboard", DashboardHttpProvider, api, update)
-  httpServer.registerProxy(proxy)
 
   ipcMain().onDispose(() => httpServer.dispose())
 
@@ -94,9 +94,9 @@ const main = async (args: Args): Promise<void> => {
     logger.info("  - Not serving HTTPS")
   }
 
-  if (proxy.proxyDomains.size > 0) {
+  if (httpServer.proxyDomains.size > 0) {
-    logger.info(`  - Proxying the following domain${proxy.proxyDomains.size === 1 ? "" : "s"}:`)
+    logger.info(`  - Proxying the following domain${httpServer.proxyDomains.size === 1 ? "" : "s"}:`)
-    proxy.proxyDomains.forEach((domain) => logger.info(`    - *.${domain}`))
+    httpServer.proxyDomains.forEach((domain) => logger.info(`    - *.${domain}`))
   }
 
   logger.info(`Automatic updates are ${update.enabled ? "enabled" : "disabled"}`)

src/node/http.ts

@@ -1,6 +1,7 @@
 import { field, logger } from "@coder/logger"
 import * as fs from "fs-extra"
 import * as http from "http"
+import proxy from "http-proxy"
 import * as httpolyglot from "httpolyglot"
 import * as https from "https"
 import * as net from "net"
@@ -18,6 +19,10 @@ import { getMediaMime, xdgLocalDir } from "./util"
 export type Cookies = { [key: string]: string[] | undefined }
 export type PostData = { [key: string]: string | string[] | undefined }
 
+interface ProxyRequest extends http.IncomingMessage {
+  base?: string
+}
+
 interface AuthPayload extends Cookies {
   key?: string[]
 }
@@ -29,6 +34,17 @@ export enum AuthType {
 
 export type Query = { [key: string]: string | string[] | undefined }
 
+export interface ProxyOptions {
+  /**
+   * A base path to strip from from the request before proxying if necessary.
+   */
+  base?: string
+  /**
+   * The port to proxy.
+   */
+  port: string
+}
+
 export interface HttpResponse<T = string | Buffer | object> {
   /*
    * Whether to set cache-control headers for this response.
@@ -78,9 +94,16 @@ export interface HttpResponse<T = string | Buffer | object> {
    */
   query?: Query
   /**
-   * Indicates the request was handled and nothing else needs to be done.
+   * Indicates the request should be proxied.
    */
-  handled?: boolean
+  proxy?: ProxyOptions
+}
+
+export interface WsResponse {
+  /**
+   * Indicates the web socket should be proxied.
+   */
+  proxy?: ProxyOptions
 }
 
 /**
@@ -104,6 +127,7 @@ export interface HttpServerOptions {
   readonly host?: string
   readonly password?: string
   readonly port?: number
+  readonly proxyDomains?: string[]
   readonly socket?: string
 }
 
@@ -156,7 +180,9 @@ export abstract class HttpProvider {
   }
 
   /**
-   * Handle web sockets on the registered endpoint.
+   * Handle web sockets on the registered endpoint. Normally the provider
+   * handles the request itself but it can return a response when necessary. The
+   * default is to throw a 404.
    */
   public handleWebSocket(
     /* eslint-disable @typescript-eslint/no-unused-vars */
@@ -165,18 +191,14 @@ export abstract class HttpProvider {
     _socket: net.Socket,
     _head: Buffer,
     /* eslint-enable @typescript-eslint/no-unused-vars */
-  ): Promise<void> {
+  ): Promise<WsResponse | void> {
     throw new HttpError("Not found", HttpCode.NotFound)
   }
 
   /**
    * Handle requests to the registered endpoint.
    */
-  public abstract handleRequest(
+  public abstract handleRequest(route: Route, request: http.IncomingMessage): Promise<HttpResponse>
-    route: Route,
-    request: http.IncomingMessage,
-    response: http.ServerResponse,
-  ): Promise<HttpResponse>
 
   /**
    * Get the base relative to the provided route. For each slash we need to go
@@ -288,7 +310,7 @@ export abstract class HttpProvider {
    * Return the provided password value if the payload contains the right
    * password otherwise return false. If no payload is specified use cookies.
    */
-  protected authenticated(request: http.IncomingMessage, payload?: AuthPayload): string | boolean {
+  public authenticated(request: http.IncomingMessage, payload?: AuthPayload): string | boolean {
     switch (this.options.auth) {
       case AuthType.None:
         return true
@@ -426,39 +448,6 @@ export interface HttpProvider3<A1, A2, A3, T> {
   new (options: HttpProviderOptions, a1: A1, a2: A2, a3: A3): T
 }
 
-export interface HttpProxyProvider {
-  /**
-   * Return a response if the request should be proxied. Anything that ends in a
-   * proxy domain and has a *single* subdomain should be proxied. Anything else
-   * should return `undefined` and will be handled as normal.
-   *
-   * For example if `coder.com` is specified `8080.coder.com` will be proxied
-   * but `8080.test.coder.com` and `test.8080.coder.com` will not.
-   */
-  maybeProxyRequest(
-    route: Route,
-    request: http.IncomingMessage,
-    response: http.ServerResponse,
-  ): HttpResponse | undefined
-
-  /**
-   * Same concept as `maybeProxyRequest` but for web sockets.
-   */
-  maybeProxyWebSocket(
-    route: Route,
-    request: http.IncomingMessage,
-    socket: net.Socket,
-    head: Buffer,
-  ): HttpResponse | undefined
-
-  /**
-   * Get the domain that should be used for setting a cookie. This will allow
-   * the user to authenticate only once. This will return the highest level
-   * domain (e.g. `coder.com` over `test.coder.com` if both are specified).
-   */
-  getCookieDomain(host: string): string | undefined
-}
-
 /**
  * An HTTP server. Its main role is to route incoming HTTP requests to the
  * appropriate provider for that endpoint then write out the response. It also
@@ -471,9 +460,19 @@ export class HttpServer {
   private readonly providers = new Map<string, HttpProvider>()
   private readonly heart: Heart
   private readonly socketProvider = new SocketProxyProvider()
-  private proxy?: HttpProxyProvider
+
+  /**
+   * Proxy domains are stored here without the leading `*.`
+   */
+  public readonly proxyDomains: Set<string>
+
+  /**
+   * Provides the actual proxying functionality.
+   */
+  private readonly proxy = proxy.createProxyServer({})
 
   public constructor(private readonly options: HttpServerOptions) {
+    this.proxyDomains = new Set((options.proxyDomains || []).map((d) => d.replace(/^\*\./, "")))
     this.heart = new Heart(path.join(xdgLocalDir, "heartbeat"), async () => {
       const connections = await this.getConnections()
       logger.trace(`${connections} active connection${plural(connections)}`)
@@ -491,6 +490,13 @@ export class HttpServer {
     } else {
       this.server = http.createServer(this.onRequest)
     }
+    this.proxy.on("error", (error) => logger.warn(error.message))
+    // Intercept the response to rewrite absolute redirects against the base path.
+    this.proxy.on("proxyRes", (response, request: ProxyRequest) => {
+      if (response.headers.location && response.headers.location.startsWith("/") && request.base) {
+        response.headers.location = request.base + response.headers.location
+      }
+    })
   }
 
   public dispose(): void {
@@ -546,14 +552,6 @@ export class HttpServer {
     return p
   }
 
-  /**
-   * Register a provider as a proxy. It will be consulted before any other
-   * provider.
-   */
-  public registerProxy(proxy: HttpProxyProvider): void {
-    this.proxy = proxy
-  }
-
   /**
    * Start listening on the specified port.
    */
@@ -602,7 +600,7 @@ export class HttpServer {
               "Set-Cookie": [
                 `${payload.cookie.key}=${payload.cookie.value}`,
                 `Path=${normalize(payload.cookie.path || "/", true)}`,
-                domain ? `Domain=${(this.proxy && this.proxy.getCookieDomain(domain)) || domain}` : undefined,
+                domain ? `Domain=${this.getCookieDomain(domain)}` : undefined,
                 // "HttpOnly",
                 "SameSite=lax",
               ]
@@ -631,9 +629,11 @@ export class HttpServer {
     try {
       const payload =
         this.maybeRedirect(request, route) ||
-        (this.proxy && this.proxy.maybeProxyRequest(route, request, response)) ||
+        (route.provider.authenticated(request) && this.maybeProxy(request)) ||
-        (await route.provider.handleRequest(route, request, response))
+        (await route.provider.handleRequest(route, request))
-      if (!payload.handled) {
+      if (payload.proxy) {
+        this.doProxy(route, request, response, payload.proxy)
+      } else {
         write(payload)
       }
     } catch (error) {
@@ -710,8 +710,13 @@ export class HttpServer {
         throw new HttpError("Not found", HttpCode.NotFound)
       }
 
-      if (!this.proxy || !this.proxy.maybeProxyWebSocket(route, request, socket, head)) {
+      // The socket proxy is so we can pass them to child processes (TLS sockets
-        await route.provider.handleWebSocket(route, request, await this.socketProvider.createProxy(socket), head)
+      // can't be transferred so we need an in-between).
+      const socketProxy = await this.socketProvider.createProxy(socket)
+      const payload =
+        this.maybeProxy(request) || (await route.provider.handleWebSocket(route, request, socketProxy, head))
+      if (payload && payload.proxy) {
+        this.doProxy(route, request, { socket: socketProxy, head }, payload.proxy)
       }
     } catch (error) {
       socket.destroy(error)
@@ -756,4 +761,106 @@ export class HttpServer {
     }
     return { base, fullPath, requestPath, query: parsedUrl.query, provider, originalPath }
   }
+
+  /**
+   * Proxy a request to the target.
+   */
+  private doProxy(
+    route: Route,
+    request: http.IncomingMessage,
+    response: http.ServerResponse,
+    options: ProxyOptions,
+  ): void
+  /**
+   * Proxy a web socket to the target.
+   */
+  private doProxy(
+    route: Route,
+    request: http.IncomingMessage,
+    response: { socket: net.Socket; head: Buffer },
+    options: ProxyOptions,
+  ): void
+  /**
+   * Proxy a request or web socket to the target.
+   */
+  private doProxy(
+    route: Route,
+    request: http.IncomingMessage,
+    response: http.ServerResponse | { socket: net.Socket; head: Buffer },
+    options: ProxyOptions,
+  ): void {
+    const port = parseInt(options.port, 10)
+    if (isNaN(port)) {
+      throw new HttpError(`"${options.port}" is not a valid number`, HttpCode.BadRequest)
+    }
+
+    // REVIEW: Absolute redirects need to be based on the subpath but I'm not
+    // sure how best to get this information to the `proxyRes` event handler.
+    // For now I'm sticking it on the request object which is passed through to
+    // the event.
+    ;(request as ProxyRequest).base = options.base
+
+    const isHttp = response instanceof http.ServerResponse
+    const path = options.base ? route.fullPath.replace(options.base, "") : route.fullPath
+    const proxyOptions: proxy.ServerOptions = {
+      changeOrigin: true,
+      ignorePath: true,
+      target: `${isHttp ? "http" : "ws"}://127.0.0.1:${port}${path}${
+        Object.keys(route.query).length > 0 ? `?${querystring.stringify(route.query)}` : ""
+      }`,
+      ws: !isHttp,
+    }
+
+    if (response instanceof http.ServerResponse) {
+      this.proxy.web(request, response, proxyOptions)
+    } else {
+      this.proxy.ws(request, response.socket, response.head, proxyOptions)
+    }
+  }
+
+  /**
+   * Get the domain that should be used for setting a cookie. This will allow
+   * the user to authenticate only once. This will return the highest level
+   * domain (e.g. `coder.com` over `test.coder.com` if both are specified).
+   */
+  private getCookieDomain(host: string): string {
+    let current: string | undefined
+    this.proxyDomains.forEach((domain) => {
+      if (host.endsWith(domain) && (!current || domain.length < current.length)) {
+        current = domain
+      }
+    })
+    // Setting the domain to localhost doesn't seem to work for subdomains (for
+    // example dev.localhost).
+    return current && current !== "localhost" ? current : host
+  }
+
+  /**
+   * Return a response if the request should be proxied. Anything that ends in a
+   * proxy domain and has a *single* subdomain should be proxied. Anything else
+   * should return `undefined` and will be handled as normal.
+   *
+   * For example if `coder.com` is specified `8080.coder.com` will be proxied
+   * but `8080.test.coder.com` and `test.8080.coder.com` will not.
+   */
+  public maybeProxy(request: http.IncomingMessage): HttpResponse | undefined {
+    // Split into parts.
+    const host = request.headers.host || ""
+    const idx = host.indexOf(":")
+    const domain = idx !== -1 ? host.substring(0, idx) : host
+    const parts = domain.split(".")
+
+    // There must be an exact match.
+    const port = parts.shift()
+    const proxyDomain = parts.join(".")
+    if (!port || !this.proxyDomains.has(proxyDomain)) {
+      return undefined
+    }
+
+    return {
+      proxy: {
+        port,
+      },
+    }
+  }
 }