Move argument defaults into setDefaults

src/node/cli.ts

@@ -5,7 +5,7 @@ import * as os from "os"
 import * as path from "path"
 import { Args as VsArgs } from "../../lib/vscode/src/vs/server/ipc"
 import { AuthType } from "./http"
-import { canConnect, generatePassword, humanPath, paths } from "./util"
+import { canConnect, generateCertificate, generatePassword, humanPath, paths } from "./util"
 
 export class Optional<T> {
   public constructor(public readonly value?: T) {}
@@ -22,33 +22,33 @@ export enum LogLevel {
 export class OptionalString extends Optional<string> {}
 
 export interface Args extends VsArgs {
-  readonly config?: string
+  config?: string
-  readonly auth?: AuthType
+  auth?: AuthType
-  readonly password?: string
+  password?: string
-  readonly cert?: OptionalString
+  cert?: OptionalString
-  readonly "cert-key"?: string
+  "cert-key"?: string
-  readonly "disable-telemetry"?: boolean
+  "disable-telemetry"?: boolean
-  readonly help?: boolean
+  help?: boolean
-  readonly host?: string
+  host?: string
-  readonly json?: boolean
+  json?: boolean
   log?: LogLevel
-  readonly open?: boolean
+  open?: boolean
-  readonly port?: number
+  port?: number
-  readonly "bind-addr"?: string
+  "bind-addr"?: string
-  readonly socket?: string
+  socket?: string
-  readonly version?: boolean
+  version?: boolean
-  readonly force?: boolean
+  force?: boolean
-  readonly "list-extensions"?: boolean
+  "list-extensions"?: boolean
-  readonly "install-extension"?: string[]
+  "install-extension"?: string[]
-  readonly "show-versions"?: boolean
+  "show-versions"?: boolean
-  readonly "uninstall-extension"?: string[]
+  "uninstall-extension"?: string[]
-  readonly "proxy-domain"?: string[]
+  "proxy-domain"?: string[]
-  readonly locale?: string
+  locale?: string
-  readonly _: string[]
+  _: string[]
-  readonly "reuse-window"?: boolean
+  "reuse-window"?: boolean
-  readonly "new-window"?: boolean
+  "new-window"?: boolean
 
-  readonly link?: OptionalString
+  link?: OptionalString
 }
 
 interface Option<T> {
@@ -325,13 +325,37 @@ export const parse = (
     args._.push(arg)
   }
 
+  // If a cert was provided a key must also be provided.
+  if (args.cert && args.cert.value && !args["cert-key"]) {
+    throw new Error("--cert-key is missing")
+  }
+
   logger.debug("parsed command line", field("args", args))
 
   return args
 }
 
-export async function setDefaults(args: Args): Promise<Args> {
+export interface DefaultedArgs extends ConfigArgs {
-  args = { ...args }
+  auth: AuthType
+  cert?: {
+    value: string
+  }
+  host: string
+  port: number
+  "proxy-domain": string[]
+  verbose: boolean
+  usingEnvPassword: boolean
+  "extensions-dir": string
+  "user-data-dir": string
+}
+
+/**
+ * Take CLI and config arguments (optional) and return a single set of arguments
+ * with the defaults set. Arguments from the CLI are prioritized over config
+ * arguments.
+ */
+export async function setDefaults(cliArgs: Args, configArgs?: ConfigArgs): Promise<DefaultedArgs> {
+  const args = Object.assign({}, configArgs || {}, cliArgs)
 
   if (!args["user-data-dir"]) {
     await copyOldMacOSDataDir()
@@ -381,7 +405,52 @@ export async function setDefaults(args: Args): Promise<Args> {
       break
   }
 
-  return args
+  // Default to using a password.
+  if (!args.auth) {
+    args.auth = AuthType.Password
+  }
+
+  const [host, port] = bindAddrFromAllSources(args, configArgs || { _: [] })
+  args.host = host
+  args.port = port
+
+  // If we're being exposed to the cloud, we listen on a random address and
+  // disable auth.
+  if (args.link) {
+    args.host = "localhost"
+    args.port = 0
+    args.socket = undefined
+    args.cert = undefined
+
+    if (args.auth !== AuthType.None) {
+      args.auth = AuthType.None
+    }
+  }
+
+  if (args.cert && !args.cert.value) {
+    const { cert, certKey } = await generateCertificate()
+    args.cert = {
+      value: cert,
+    }
+    args["cert-key"] = certKey
+  }
+
+  const usingEnvPassword = !!process.env.PASSWORD
+  if (process.env.PASSWORD) {
+    args.password = process.env.PASSWORD
+  }
+
+  // Ensure it's not readable by child processes.
+  delete process.env.PASSWORD
+
+  // Filter duplicate proxy domains and remove any leading `*.`.
+  const proxyDomains = new Set((args["proxy-domain"] || []).map((d) => d.replace(/^\*\./, "")))
+  args["proxy-domain"] = Array.from(proxyDomains)
+
+  return {
+    ...args,
+    usingEnvPassword,
+  } as DefaultedArgs // TODO: Technically no guarantee this is fulfilled.
 }
 
 async function defaultConfigFile(): Promise<string> {
@@ -392,12 +461,16 @@ cert: false
 `
 }
 
+interface ConfigArgs extends Args {
+  config: string
+}
+
 /**
  * Reads the code-server yaml config file and returns it as Args.
  *
  * @param configPath Read the config from configPath instead of $CODE_SERVER_CONFIG or the default.
  */
-export async function readConfigFile(configPath?: string): Promise<Args> {
+export async function readConfigFile(configPath?: string): Promise<ConfigArgs> {
   if (!configPath) {
     configPath = process.env.CODE_SERVER_CONFIG
     if (!configPath) {
@@ -466,7 +539,7 @@ function bindAddrFromArgs(addr: Addr, args: Args): Addr {
   return addr
 }
 
-export function bindAddrFromAllSources(cliArgs: Args, configArgs: Args): [string, number] {
+function bindAddrFromAllSources(cliArgs: Args, configArgs: Args): [string, number] {
   let addr: Addr = {
     host: "localhost",
     port: 8080,

src/node/entry.ts

@@ -12,8 +12,7 @@ import { StaticHttpProvider } from "./app/static"
 import { UpdateHttpProvider } from "./app/update"
 import { VscodeHttpProvider } from "./app/vscode"
 import {
-  Args,
+  DefaultedArgs,
-  bindAddrFromAllSources,
   optionDescriptions,
   parse,
   readConfigFile,
@@ -24,7 +23,7 @@ import {
 import { coderCloudBind } from "./coder-cloud"
 import { AuthType, HttpServer, HttpServerOptions } from "./http"
 import { loadPlugins } from "./plugin"
-import { generateCertificate, hash, humanPath, open } from "./util"
+import { hash, humanPath, open } from "./util"
 import { ipcMain, WrapperProcess } from "./wrapper"
 
 let pkg: { version?: string; commit?: string } = {}
@@ -37,7 +36,7 @@ try {
 const version = pkg.version || "development"
 const commit = pkg.commit || "development"
 
-export const runVsCodeCli = (args: Args): void => {
+export const runVsCodeCli = (args: DefaultedArgs): void => {
   logger.debug("forking vs code cli...")
   const vscode = cp.fork(path.resolve(__dirname, "../../lib/vscode/out/vs/server/fork"), [], {
     env: {
@@ -61,7 +60,7 @@ export const runVsCodeCli = (args: Args): void => {
   vscode.on("exit", (code) => process.exit(code || 0))
 }
 
-export const openInExistingInstance = async (args: Args, socketPath: string): Promise<void> => {
+export const openInExistingInstance = async (args: DefaultedArgs, socketPath: string): Promise<void> => {
   const pipeArgs: OpenCommandPipeArgs & { fileURIs: string[] } = {
     type: "open",
     folderURIs: [],
@@ -117,54 +116,26 @@ export const openInExistingInstance = async (args: Args, socketPath: string): Pr
   vscode.end()
 }
 
-const main = async (args: Args, configArgs: Args): Promise<void> => {
+const main = async (args: DefaultedArgs): Promise<void> => {
-  if (args.link) {
-    // If we're being exposed to the cloud, we listen on a random address and disable auth.
-    args = {
-      ...args,
-      host: "localhost",
-      port: 0,
-      auth: AuthType.None,
-      socket: undefined,
-      cert: undefined,
-    }
-    logger.info("link: disabling auth and listening on random localhost port for cloud agent")
-  }
-
-  if (!args.auth) {
-    args = {
-      ...args,
-      auth: AuthType.Password,
-    }
-  }
-
   logger.info(`Using user-data-dir ${humanPath(args["user-data-dir"])}`)
-
   logger.trace(`Using extensions-dir ${humanPath(args["extensions-dir"])}`)
 
-  const envPassword = !!process.env.PASSWORD
+  if (args.auth === AuthType.Password && !args.password) {
-  const password = args.auth === AuthType.Password && (process.env.PASSWORD || args.password)
-  if (args.auth === AuthType.Password && !password) {
     throw new Error("Please pass in a password via the config file or $PASSWORD")
   }
-  const [host, port] = bindAddrFromAllSources(args, configArgs)
 
   // Spawn the main HTTP server.
   const options: HttpServerOptions = {
     auth: args.auth,
     commit,
-    host: host,
+    host: args.host,
     // The hash does not add any actual security but we do it for obfuscation purposes.
-    password: password ? hash(password) : undefined,
+    password: args.password ? hash(args.password) : undefined,
-    port: port,
+    port: args.port,
     proxyDomains: args["proxy-domain"],
     socket: args.socket,
-    ...(args.cert && !args.cert.value
-      ? await generateCertificate()
-      : {
     cert: args.cert && args.cert.value,
     certKey: args["cert-key"],
-        }),
   }
 
   if (options.cert && !options.certKey) {
@@ -175,7 +146,7 @@ const main = async (args: Args, configArgs: Args): Promise<void> => {
   httpServer.registerHttpProvider(["/", "/vscode"], VscodeHttpProvider, args)
   httpServer.registerHttpProvider("/update", UpdateHttpProvider, false)
   httpServer.registerHttpProvider("/proxy", ProxyHttpProvider)
-  httpServer.registerHttpProvider("/login", LoginHttpProvider, args.config!, envPassword)
+  httpServer.registerHttpProvider("/login", LoginHttpProvider, args.config!, args.usingEnvPassword)
   httpServer.registerHttpProvider("/static", StaticHttpProvider)
   httpServer.registerHttpProvider("/healthz", HealthHttpProvider, httpServer.heart)
 
@@ -191,19 +162,18 @@ const main = async (args: Args, configArgs: Args): Promise<void> => {
   logger.info(`Using config file ${humanPath(args.config)}`)
 
   const serverAddress = await httpServer.listen()
-  logger.info(`HTTP server listening on ${serverAddress}`)
+  logger.info(`HTTP server listening on ${serverAddress} ${args.link ? "(randomized by --link)" : ""}`)
 
   if (args.auth === AuthType.Password) {
-    if (envPassword) {
+    if (args.usingEnvPassword) {
       logger.info("    - Using password from $PASSWORD")
     } else {
       logger.info(`    - Using password from ${humanPath(args.config)}`)
     }
     logger.info("    - To disable use `--auth none`")
   } else {
-    logger.info("  - No authentication")
+    logger.info(`  - No authentication ${args.link ? "(disabled by --link)" : ""}`)
   }
-  delete process.env.PASSWORD
 
   if (httpServer.protocol === "https") {
     logger.info(
@@ -215,9 +185,19 @@ const main = async (args: Args, configArgs: Args): Promise<void> => {
     logger.info("  - Not serving HTTPS")
   }
 
-  if (httpServer.proxyDomains.size > 0) {
+  if (args["proxy-domain"].length > 0) {
-    logger.info(`  - ${plural(httpServer.proxyDomains.size, "Proxying the following domain")}:`)
+    logger.info(`  - ${plural(args["proxy-domain"].length, "Proxying the following domain")}:`)
-    httpServer.proxyDomains.forEach((domain) => logger.info(`    - *.${domain}`))
+    args["proxy-domain"].forEach((domain) => logger.info(`    - *.${domain}`))
+  }
+
+  if (args.link) {
+    try {
+      await coderCloudBind(serverAddress!, args.link.value)
+      logger.info("  - Connected to cloud agent")
+    } catch (err) {
+      logger.error(err.message)
+      ipcMain.exit(1)
+    }
   }
 
   if (serverAddress && !options.socket && args.open) {
@@ -228,23 +208,12 @@ const main = async (args: Args, configArgs: Args): Promise<void> => {
     })
     logger.info(`Opened ${openAddress}`)
   }
-
-  if (args.link) {
-    try {
-      await coderCloudBind(serverAddress!, args.link.value)
-    } catch (err) {
-      logger.error(err.message)
-      ipcMain.exit(1)
-    }
-  }
 }
 
 async function entry(): Promise<void> {
   const cliArgs = parse(process.argv.slice(2))
   const configArgs = await readConfigFile(cliArgs.config)
-  // This prioritizes the flags set in args over the ones in the config file.
+  const args = await setDefaults(cliArgs, configArgs)
-  let args = Object.assign(configArgs, cliArgs)
-  args = await setDefaults(args)
 
   // There's no need to check flags like --help or to spawn in an existing
   // instance for the child process because these would have already happened in
@@ -252,7 +221,7 @@ async function entry(): Promise<void> {
   if (ipcMain.isChild) {
     await ipcMain.handshake()
     ipcMain.preventExit()
-    return main(args, configArgs)
+    return main(args)
   }
 
   if (args.help) {

src/node/http.ts

@@ -130,7 +130,7 @@ export interface HttpServerOptions {
   readonly host?: string
   readonly password?: string
   readonly port?: number
-  readonly proxyDomains?: string[]
+  readonly proxyDomains: string[]
   readonly socket?: string
 }
 
@@ -463,18 +463,12 @@ export class HttpServer {
   public readonly heart: Heart
   private readonly socketProvider = new SocketProxyProvider()
 
-  /**
-   * 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(paths.data, "heartbeat"), async () => {
       const connections = await this.getConnections()
       logger.trace(plural(connections, `${connections} active connection`))
@@ -892,7 +886,7 @@ export class HttpServer {
       return undefined
     }
 
-    this.proxyDomains.forEach((domain) => {
+    this.options.proxyDomains.forEach((domain) => {
       if (host.endsWith(domain) && domain.length < host.length) {
         host = domain
       }
@@ -922,7 +916,7 @@ export class HttpServer {
     // There must be an exact match.
     const port = parts.shift()
     const proxyDomain = parts.join(".")
-    if (!port || !this.proxyDomains.has(proxyDomain)) {
+    if (!port || !this.options.proxyDomains.includes(proxyDomain)) {
       return undefined
     }
 

test/cli.test.ts

@@ -14,17 +14,23 @@ type Mutable<T> = {
 describe("parser", () => {
   beforeEach(() => {
     delete process.env.LOG_LEVEL
+    delete process.env.PASSWORD
   })
 
   // The parser should not set any defaults so the caller can determine what
   // values the user actually set. These are only set after explicitly calling
   // `setDefaults`.
   const defaults = {
+    auth: "password",
+    host: "localhost",
+    port: 8080,
+    "proxy-domain": [],
+    usingEnvPassword: false,
     "extensions-dir": path.join(paths.data, "extensions"),
     "user-data-dir": paths.data,
   }
 
-  it("should set defaults", () => {
+  it("should parse nothing", () => {
     assert.deepEqual(parse([]), { _: [] })
   })
 
@@ -232,6 +238,71 @@ describe("parser", () => {
       "proxy-domain": ["*.coder.com", "test.com"],
     })
   })
+
+  it("should enforce cert-key with cert value or otherwise generate one", async () => {
+    const args = parse(["--cert"])
+    assert.deepEqual(args, {
+      _: [],
+      cert: {
+        value: undefined,
+      },
+    })
+    assert.throws(() => parse(["--cert", "test"]), /--cert-key is missing/)
+    assert.deepEqual(await setDefaults(args), {
+      _: [],
+      ...defaults,
+      cert: {
+        value: path.join(tmpdir, "self-signed.cert"),
+      },
+      "cert-key": path.join(tmpdir, "self-signed.key"),
+    })
+  })
+
+  it("should override with --link", async () => {
+    const args = parse("--cert test --cert-key test --socket test --host 0.0.0.0 --port 8888 --link test".split(" "))
+    assert.deepEqual(await setDefaults(args), {
+      _: [],
+      ...defaults,
+      auth: "none",
+      host: "localhost",
+      link: {
+        value: "test",
+      },
+      port: 0,
+      cert: undefined,
+      "cert-key": path.resolve("test"),
+      socket: undefined,
+    })
+  })
+
+  it("should use env var password", async () => {
+    process.env.PASSWORD = "test"
+    const args = parse([])
+    assert.deepEqual(args, {
+      _: [],
+    })
+
+    assert.deepEqual(await setDefaults(args), {
+      ...defaults,
+      _: [],
+      password: "test",
+      usingEnvPassword: true,
+    })
+  })
+
+  it("should filter proxy domains", async () => {
+    const args = parse(["--proxy-domain", "*.coder.com", "--proxy-domain", "coder.com", "--proxy-domain", "coder.org"])
+    assert.deepEqual(args, {
+      _: [],
+      "proxy-domain": ["*.coder.com", "coder.com", "coder.org"],
+    })
+
+    assert.deepEqual(await setDefaults(args), {
+      ...defaults,
+      _: [],
+      "proxy-domain": ["coder.com", "coder.org"],
+    })
+  })
 })
 
 describe("cli", () => {