Configuration file bug fixes based on @code-asher's review

nhooyr · nhooyr · commit d6ea9d78f6e5 · 2020-05-12T19:59:54.000-04:00
diff --git a/doc/FAQ.md b/doc/FAQ.md
@@ -168,8 +168,10 @@ code-server crashes can be helpful.
 ### Where is the data directory?
 
 If the `XDG_DATA_HOME` environment variable is set the data directory will be
-`$XDG_DATA_HOME/code-server`. Otherwise the default is `~/.local/share/code-server`.
-On Windows, it will be `%APPDATA%\Local\code-server\Data`.
+`$XDG_DATA_HOME/code-server`. Otherwise:
+
+1. Unix: `~/.local/share/code-server`
+1. Windows: `%APPDATA%\Local\code-server\Data`
 
 ## Enterprise
 
diff --git a/src/node/cli.ts b/src/node/cli.ts
@@ -1,10 +1,10 @@
+import { field, Level, logger } from "@coder/logger"
 import * as fs from "fs-extra"
 import yaml from "js-yaml"
 import * as path from "path"
-import { field, logger, Level } from "@coder/logger"
 import { Args as VsArgs } from "../../lib/vscode/src/vs/server/ipc"
 import { AuthType } from "./http"
-import { paths, uxPath } from "./util"
+import { generatePassword, humanPath, paths } from "./util"
 
 export class Optional<T> {
   public constructor(public readonly value?: T) {}
@@ -84,7 +84,10 @@ type Options<T> = {
 
 const options: Options<Required<Args>> = {
   auth: { type: AuthType, description: "The type of authentication to use." },
-  password: { type: "string", description: "The password for password authentication." },
+  password: {
+    type: "string",
+    description: "The password for password authentication (can only be passed in via $PASSWORD or the config file).",
+  },
   cert: {
     type: OptionalString,
     path: true,
@@ -96,11 +99,14 @@ const options: Options<Required<Args>> = {
   json: { type: "boolean" },
   open: { type: "boolean", description: "Open in browser on startup. Does not work remotely." },
 
-  "bind-addr": { type: "string", description: "Address to bind to in host:port." },
+  "bind-addr": {
+    type: "string",
+    description: "Address to bind to in host:port. You can also use $PORT to override the port.",
+  },
 
   config: {
     type: "string",
-    description: "Path to yaml config file. Every flag maps directory to a key in the config file.",
+    description: "Path to yaml config file. Every flag maps directly to a key in the config file.",
   },
 
   // These two have been deprecated by bindAddr.
@@ -145,7 +151,19 @@ export const optionDescriptions = (): string[] => {
   )
 }
 
-export const parse = (argv: string[]): Args => {
+export const parse = (
+  argv: string[],
+  opts?: {
+    configFile: string
+  },
+): Args => {
+  const error = (msg: string): Error => {
+    if (opts?.configFile) {
+      msg = `error reading ${opts.configFile}: ${msg}`
+    }
+    return new Error(msg)
+  }
+
   const args: Args = { _: [] }
   let ended = false
 
@@ -175,7 +193,11 @@ export const parse = (argv: string[]): Args => {
       }
 
       if (!key || !options[key]) {
-        throw new Error(`Unknown option ${arg}`)
+        throw error(`Unknown option ${arg}`)
+      }
+
+      if (key === "password" && !opts?.configFile) {
+        throw new Error("--password can only be set in the config file or passed in via $PASSWORD")
       }
 
       const option = options[key]
@@ -194,7 +216,11 @@ export const parse = (argv: string[]): Args => {
         ;(args[key] as OptionalString) = new OptionalString(value)
         continue
       } else if (!value) {
-        throw new Error(`--${key} requires a value`)
+        throw error(`--${key} requires a value`)
+      }
+
+      if (option.type == OptionalString && value == "false") {
+        continue
       }
 
       if (option.path) {
@@ -214,15 +240,15 @@ export const parse = (argv: string[]): Args => {
         case "number":
           ;(args[key] as number) = parseInt(value, 10)
           if (isNaN(args[key] as number)) {
-            throw new Error(`--${key} must be a number`)
+            throw error(`--${key} must be a number`)
           }
           break
         case OptionalString:
           ;(args[key] as OptionalString) = new OptionalString(value)
           break
         default: {
           if (!Object.values(option.type).includes(value)) {
-            throw new Error(`--${key} valid values: [${Object.values(option.type).join(", ")}]`)
+            throw error(`--${key} valid values: [${Object.values(option.type).join(", ")}]`)
           }
           ;(args[key] as string) = value
           break
@@ -284,53 +310,93 @@ export const parse = (argv: string[]): Args => {
   return args
 }
 
-const defaultConfigFile = `
+async function defaultConfigFile(): Promise<string> {
+  return `bind-addr: 127.0.0.1:8080
 auth: password
-bind-addr: 127.0.0.1:8080
-`.trimLeft()
-
-// readConfigFile reads the config file specified in the config flag
-// and loads it's configuration.
-//
-// Flags set on the CLI take priority.
-//
-// The config file can also be passed via $CODE_SERVER_CONFIG and defaults
-// to ~/.config/code-server/config.yaml.
-export async function readConfigFile(args: Args): Promise<Args> {
-  const configPath = getConfigPath(args)
+password: ${await generatePassword()}
+cert: false
+`
+}
+
+/**
+ * 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> {
+  if (!configPath) {
+    configPath = process.env.CODE_SERVER_CONFIG
+    if (!configPath) {
+      configPath = path.join(paths.config, "config.yaml")
+    }
+  }
 
   if (!(await fs.pathExists(configPath))) {
-    await fs.outputFile(configPath, defaultConfigFile)
-    logger.info(`Wrote default config file to ${uxPath(configPath)}`)
+    await fs.outputFile(configPath, await defaultConfigFile())
+    logger.info(`Wrote default config file to ${humanPath(configPath)}`)
   }
 
-  logger.info(`Using config file from ${uxPath(configPath)}`)
+  logger.info(`Using config file from ${humanPath(configPath)}`)
 
   const configFile = await fs.readFile(configPath)
   const config = yaml.safeLoad(configFile.toString(), {
-    filename: args.config,
+    filename: configPath,
   })
 
   // We convert the config file into a set of flags.
   // This is a temporary measure until we add a proper CLI library.
   const configFileArgv = Object.entries(config).map(([optName, opt]) => {
-    if (opt === null) {
+    if (opt === true) {
       return `--${optName}`
     }
     return `--${optName}=${opt}`
   })
-  const configFileArgs = parse(configFileArgv)
+  const args = parse(configFileArgv, {
+    configFile: configPath,
+  })
+  return {
+    ...args,
+    config: configPath,
+  }
+}
 
-  // This prioritizes the flags set in args over the ones in the config file.
-  return Object.assign(configFileArgs, args)
+function parseBindAddr(bindAddr: string): [string, number] {
+  const u = new URL(`http://${bindAddr}`)
+  return [u.hostname, parseInt(u.port, 10)]
 }
 
-function getConfigPath(args: Args): string {
-  if (args.config !== undefined) {
-    return args.config
+interface Addr {
+  host: string
+  port: number
+}
+
+function bindAddrFromArgs(addr: Addr, args: Args): Addr {
+  addr = { ...addr }
+  if (args["bind-addr"]) {
+    ;[addr.host, addr.port] = parseBindAddr(args["bind-addr"])
   }
-  if (process.env.CODE_SERVER_CONFIG !== undefined) {
-    return process.env.CODE_SERVER_CONFIG
+  if (args.host) {
+    addr.host = args.host
   }
-  return path.join(paths.config, "config.yaml")
+  if (args.port !== undefined) {
+    addr.port = args.port
+  }
+  return addr
+}
+
+export function bindAddrFromAllSources(cliArgs: Args, configArgs: Args): [string, number] {
+  let addr: Addr = {
+    host: "localhost",
+    port: 8080,
+  }
+
+  addr = bindAddrFromArgs(addr, configArgs)
+
+  if (process.env.PORT) {
+    addr.port = parseInt(process.env.PORT, 10)
+  }
+
+  addr = bindAddrFromArgs(addr, cliArgs)
+
+  return [addr.host, addr.port]
 }
diff --git a/src/node/entry.ts b/src/node/entry.ts
@@ -9,9 +9,9 @@ import { ProxyHttpProvider } from "./app/proxy"
 import { StaticHttpProvider } from "./app/static"
 import { UpdateHttpProvider } from "./app/update"
 import { VscodeHttpProvider } from "./app/vscode"
-import { Args, optionDescriptions, parse, readConfigFile } from "./cli"
+import { Args, bindAddrFromAllSources, optionDescriptions, parse, readConfigFile } from "./cli"
 import { AuthType, HttpServer, HttpServerOptions } from "./http"
-import { generateCertificate, generatePassword, hash, open, uxPath } from "./util"
+import { generateCertificate, hash, open, humanPath } from "./util"
 import { ipcMain, wrap } from "./wrapper"
 
 process.on("uncaughtException", (error) => {
@@ -31,35 +31,24 @@ try {
 const version = pkg.version || "development"
 const commit = pkg.commit || "development"
 
-const main = async (args: Args): Promise<void> => {
-  args = await readConfigFile(args)
+const main = async (cliArgs: Args): Promise<void> => {
+  const configArgs = await readConfigFile(cliArgs.config)
+  // This prioritizes the flags set in args over the ones in the config file.
+  let args = Object.assign(configArgs, cliArgs)
 
-  if (args.verbose === true) {
-    // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
-    logger.info(`Using extensions-dir at ${uxPath(args["extensions-dir"]!)}`)
-    // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
-    logger.info(`Using user-data-dir at ${uxPath(args["user-data-dir"]!)}`)
-  }
-
-  const auth = args.auth || AuthType.Password
-  const generatedPassword = (args.password || process.env.PASSWORD) !== ""
-  const password = auth === AuthType.Password && (args.password || process.env.PASSWORD || (await generatePassword()))
+  logger.trace(`Using extensions-dir at ${humanPath(args["extensions-dir"])}`)
+  logger.trace(`Using user-data-dir at ${humanPath(args["user-data-dir"])}`)
 
-  let host = args.host
-  let port = args.port
-  if (args["bind-addr"] !== undefined) {
-    const u = new URL(`http://${args["bind-addr"]}`)
-    host = u.hostname
-    port = parseInt(u.port, 10)
-  }
+  const password = args.auth === AuthType.Password && (process.env.PASSWORD || args.password)
+  const [host, port] = bindAddrFromAllSources(cliArgs, configArgs)
 
   // Spawn the main HTTP server.
   const options: HttpServerOptions = {
-    auth,
+    auth: args.auth,
     commit,
-    host: host || (args.auth === AuthType.Password && args.cert !== undefined ? "0.0.0.0" : "localhost"),
+    host: host,
     password: password ? hash(password) : undefined,
-    port: port !== undefined ? port : process.env.PORT ? parseInt(process.env.PORT, 10) : 8080,
+    port: port,
     proxyDomains: args["proxy-domain"],
     socket: args.socket,
     ...(args.cert && !args.cert.value
@@ -77,7 +66,7 @@ const main = async (args: Args): Promise<void> => {
   const httpServer = new HttpServer(options)
   const vscode = httpServer.registerHttpProvider("/", VscodeHttpProvider, args)
   const api = httpServer.registerHttpProvider("/api", ApiHttpProvider, httpServer, vscode, args["user-data-dir"])
-  const update = httpServer.registerHttpProvider("/update", UpdateHttpProvider, true)
+  const update = httpServer.registerHttpProvider("/update", UpdateHttpProvider, false)
   httpServer.registerHttpProvider("/proxy", ProxyHttpProvider)
   httpServer.registerHttpProvider("/login", LoginHttpProvider)
   httpServer.registerHttpProvider("/static", StaticHttpProvider)
@@ -89,14 +78,20 @@ const main = async (args: Args): Promise<void> => {
   const serverAddress = await httpServer.listen()
   logger.info(`HTTP server listening on ${serverAddress}`)
 
-  if (auth === AuthType.Password && generatedPassword) {
-    logger.info(`  - Password is ${password}`)
-    logger.info("    - To use your own password set it in the config file with the password key or use $PASSWORD")
-    if (!args.auth) {
-      logger.info("    - To disable use `--auth none`")
+  if (!args.auth) {
+    args = {
+      ...args,
+      auth: AuthType.Password,
     }
-  } else if (auth === AuthType.Password) {
-    logger.info("  - Using custom password for authentication")
+  }
+
+  if (args.auth === AuthType.Password) {
+    if (process.env.PASSWORD) {
+      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")
   }
@@ -117,8 +112,6 @@ const main = async (args: Args): Promise<void> => {
     httpServer.proxyDomains.forEach((domain) => logger.info(`    - *.${domain}`))
   }
 
-  // logger.info(`Automatic updates are ${update.enabled ? "enabled" : "disabled"}`)
-
   if (serverAddress && !options.socket && args.open) {
     // The web socket doesn't seem to work if browsing with 0.0.0.0.
     const openAddress = serverAddress.replace(/:\/\/0.0.0.0/, "://localhost")
diff --git a/src/node/util.ts b/src/node/util.ts
@@ -16,22 +16,20 @@ interface Paths {
 
 export const paths = getEnvPaths()
 
-// getEnvPaths gets the config and data paths for the current platform/configuration.
-//
-// On MacOS this function gets the standard XDG directories instead of using the native macOS
-// ones. Most CLIs do this as in practice only GUI apps use the standard macOS directories.
+/**
+ * Gets the config and data paths for the current platform/configuration.
+ * On MacOS this function gets the standard XDG directories instead of using the native macOS
+ * ones. Most CLIs do this as in practice only GUI apps use the standard macOS directories.
+ */
 function getEnvPaths(): Paths {
   let paths: Paths
   if (process.platform === "win32") {
     paths = envPaths("code-server", {
       suffix: "",
     })
   } else {
-    if (xdgBasedir.data === undefined) {
-      throw new Error("Missing data directory?")
-    }
-    if (xdgBasedir.config === undefined) {
-      throw new Error("Missing config directory?")
+    if (xdgBasedir.data === undefined || xdgBasedir.config === undefined) {
+      throw new Error("No home folder?")
     }
     paths = {
       data: path.join(xdgBasedir.data, "code-server"),
@@ -42,8 +40,16 @@ function getEnvPaths(): Paths {
   return paths
 }
 
-// uxPath replaces the home directory in p with ~.
-export function uxPath(p: string): string {
+/**
+ * humanPath replaces the home directory in p with ~.
+ * Makes it more readable.
+ *
+ * @param p
+ */
+export function humanPath(p?: string): string {
+  if (!p) {
+    return ""
+  }
   return p.replace(os.homedir(), "~")
 }
 
