refactor: parse options with multiple = in cli

There was a case with the hashed-password which had multiple equal signs in the value and it wasn't being parsed correctly. This uses a new function and adds a few tests.
2021-06-03 16:37:46 -07:00 · 2021-06-03 16:37:46 -07:00 · 8c2bb61af9
commit 8c2bb61af9
parent 531b7c0c25
8 changed files with 71 additions and 72 deletions
--- a/docs/FAQ.md
+++ b/docs/FAQ.md
@ -2,39 +2,38 @@
 <!-- DON'T EDIT THIS SECTION, INSTEAD RE-RUN doctoc TO UPDATE -->
 # FAQ

- [FAQ](#faq)
-  - [Questions?](#questions)
-  - [iPad Status?](#ipad-status)
-  - [Community Projects (awesome-code-server)](#community-projects-awesome-code-server)
-  - [How can I reuse my VS Code configuration?](#how-can-i-reuse-my-vs-code-configuration)
-  - [Differences compared to VS Code?](#differences-compared-to-vs-code)
+- [Questions?](#questions)
+- [iPad Status?](#ipad-status)
+- [Community Projects (awesome-code-server)](#community-projects-awesome-code-server)
+- [How can I reuse my VS Code configuration?](#how-can-i-reuse-my-vs-code-configuration)
+- [Differences compared to VS Code?](#differences-compared-to-vs-code)
  - [Installing an extension](#installing-an-extension)
-  - [How can I request a missing extension?](#how-can-i-request-a-missing-extension)
-  - [Installing an extension manually](#installing-an-extension-manually)
-  - [How do I configure the marketplace URL?](#how-do-i-configure-the-marketplace-url)
-  - [Where are extensions stored?](#where-are-extensions-stored)
-  - [How is this different from VS Code Codespaces?](#how-is-this-different-from-vs-code-codespaces)
-  - [How should I expose code-server to the internet?](#how-should-i-expose-code-server-to-the-internet)
-  - [Can I store my password hashed?](#can-i-store-my-password-hashed)
-  - [How do I securely access web services?](#how-do-i-securely-access-web-services)
+- [How can I request a missing extension?](#how-can-i-request-a-missing-extension)
+- [Installing an extension manually](#installing-an-extension-manually)
+- [How do I configure the marketplace URL?](#how-do-i-configure-the-marketplace-url)
+- [Where are extensions stored?](#where-are-extensions-stored)
+- [How is this different from VS Code Codespaces?](#how-is-this-different-from-vs-code-codespaces)
+- [How should I expose code-server to the internet?](#how-should-i-expose-code-server-to-the-internet)
+- [Can I store my password hashed?](#can-i-store-my-password-hashed)
+- [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)
+- [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)
  - [Proxying to Create React App](#proxying-to-create-react-app)
-  - [Multi-tenancy](#multi-tenancy)
-  - [Docker in code-server container?](#docker-in-code-server-container)
-  - [How can I disable telemetry?](#how-can-i-disable-telemetry)
-  - [How does code-server decide what workspace or folder to open?](#how-does-code-server-decide-what-workspace-or-folder-to-open)
-  - [How do I debug issues with code-server?](#how-do-i-debug-issues-with-code-server)
-  - [Heartbeat File](#heartbeat-file)
-  - [Healthz endpoint](#healthz-endpoint)
-  - [How does the config file work?](#how-does-the-config-file-work)
-  - [Isn't an install script piped into sh insecure?](#isnt-an-install-script-piped-into-sh-insecure)
-  - [How do I make my keyboard shortcuts work?](#how-do-i-make-my-keyboard-shortcuts-work)
-  - [How do I access my Documents/Downloads/Desktop folders in code-server on OSX?](#how-do-i-access-my-documentsdownloadsdesktop-folders-in-code-server-on-osx)
-  - [Differences compared to Theia?](#differences-compared-to-theia)
-  - [`$HTTP_PROXY`, `$HTTPS_PROXY`, `$NO_PROXY`](#http_proxy-https_proxy-no_proxy)
-  - [Enterprise](#enterprise)
+- [Multi-tenancy](#multi-tenancy)
+- [Docker in code-server container?](#docker-in-code-server-container)
+- [How can I disable telemetry?](#how-can-i-disable-telemetry)
+- [How does code-server decide what workspace or folder to open?](#how-does-code-server-decide-what-workspace-or-folder-to-open)
+- [How do I debug issues with code-server?](#how-do-i-debug-issues-with-code-server)
+- [Heartbeat File](#heartbeat-file)
+- [Healthz endpoint](#healthz-endpoint)
+- [How does the config file work?](#how-does-the-config-file-work)
+- [Isn't an install script piped into sh insecure?](#isnt-an-install-script-piped-into-sh-insecure)
+- [How do I make my keyboard shortcuts work?](#how-do-i-make-my-keyboard-shortcuts-work)
+- [How do I access my Documents/Downloads/Desktop folders in code-server on OSX?](#how-do-i-access-my-documentsdownloadsdesktop-folders-in-code-server-on-osx)
+- [Differences compared to Theia?](#differences-compared-to-theia)
+- [`$HTTP_PROXY`, `$HTTPS_PROXY`, `$NO_PROXY`](#http_proxy-https_proxy-no_proxy)
+- [Enterprise](#enterprise)

 <!-- END doctoc generated TOC please keep comment here to allow auto update -->

@ -209,7 +208,6 @@ Yes you can! Set the value of `hashed-password` instead of `password`. Generate
 ```shell
 echo -n "password" | npx argon2-cli -e
 $argon2i$v=19$m=4096,t=3,p=1$wst5qhbgk2lu1ih4dmuxvg$ls1alrvdiwtvzhwnzcm1dugg+5dto3dt1d5v9xtlws4
-
 ```

 Of course replace `thisismypassword` with your actual password and **remember to put it inside quotes**!
--- a/package.json
+++ b/package.json
@ -36,7 +36,6 @@
  "main": "out/node/entry.js",
  "devDependencies": {
    "@schemastore/package": "^0.0.6",
-    "@types/bcrypt": "^5.0.0",
    "@types/body-parser": "^1.19.0",
    "@types/compression": "^1.7.0",
    "@types/cookie-parser": "^1.4.2",
@ -90,7 +89,6 @@
  "dependencies": {
    "@coder/logger": "1.1.16",
    "argon2": "^0.28.0",
-    "bcrypt": "^5.0.1",
    "body-parser": "^1.19.0",
    "compression": "^1.7.4",
    "cookie-parser": "^1.4.5",
--- a/src/node/cli.ts
+++ b/src/node/cli.ts
@ -247,14 +247,8 @@ export function splitOnFirstEquals(str: string): string[] {
  // $argon2i$v=19$m=4096,t=3,p=1$0qR/o+0t00hsbJFQCKSfdQ$oFcM4rL6o+B7oxpuA4qlXubypbBPsf+8L531U7P9HYY
  // 2 means return two items
  // Source: https://stackoverflow.com/a/4607799/3015595
-  const split = str.split(/=(.+)/, 2)
-
-  // It should always return two elements
-  // because it's used in a place where
-  // it expected two elements
-  if (split.length === 1) {
-    split.push("")
-  }
+  // We use the ? to say the the substr after the = is optional
+  const split = str.split(/=(.+)?/, 2)

  return split
 }
@ -289,17 +283,9 @@ export const parse = (
      let key: keyof Args | undefined
      let value: string | undefined
      if (arg.startsWith("--")) {
-        // TODO fix this
-        const split = arg.replace(/^--/, "").split("=", 2)
+        const split = splitOnFirstEquals(arg.replace(/^--/, ""))
        key = split[0] as keyof Args
        value = split[1]
-      } else {
-        const short = arg.replace(/^-/, "")
-        const pair = Object.entries(options).find(([, v]) => v.short === short)
-        if (pair) {
-          key = pair[0] as keyof Args
-        }
-      }

      if (!key || !options[key]) {
        throw error(`Unknown option ${arg}`)
@ -563,7 +549,6 @@ export function parseConfigFile(configFile: string, configPath: string): ConfigA
  const config = yaml.load(configFile, {
    filename: configPath,
  })
-  console.log("what is this config", config)
  if (!config || typeof config === "string") {
    throw new Error(`invalid config: ${config}`)
  }
@ -576,11 +561,9 @@ export function parseConfigFile(configFile: string, configPath: string): ConfigA
    }
    return `--${optName}=${opt}`
  })
-  console.log("what are the configFileArgv", configFileArgv)
  const args = parse(configFileArgv, {
    configFile: configPath,
  })
-  console.log(args, "args")
  return {
    ...args,
    config: configPath,
--- a/src/node/http.ts
+++ b/src/node/http.ts
@ -63,9 +63,10 @@ export const ensureAuthenticated = async (
 */
 export const authenticated = async (req: express.Request): Promise<boolean> => {
  switch (req.args.auth) {
-    case AuthType.None:
+    case AuthType.None: {
      return true
-    case AuthType.Password:
+    }
+    case AuthType.Password: {
      // The password is stored in the cookie after being hashed.
      const hashedPasswordFromArgs = req.args["hashed-password"]
      const passwordMethod = getPasswordMethod(hashedPasswordFromArgs)
@ -77,9 +78,11 @@ export const authenticated = async (req: express.Request): Promise<boolean> => {
      }

      return await isCookieValid(isCookieValidArgs)
-    default:
+    }
+    default: {
      throw new Error(`Unsupported auth type ${req.args.auth}`)
    }
+  }
 }

 /**
--- a/src/node/util.ts
+++ b/src/node/util.ts
@ -1,15 +1,15 @@
+import { logger } from "@coder/logger"
+import * as argon2 from "argon2"
 import * as cp from "child_process"
 import * as crypto from "crypto"
-import * as argon2 from "argon2"
 import envPaths from "env-paths"
 import { promises as fs } from "fs"
 import * as net from "net"
 import * as os from "os"
 import * as path from "path"
+import safeCompare from "safe-compare"
 import * as util from "util"
 import xdgBasedir from "xdg-basedir"
-import safeCompare from "safe-compare"
-import { logger } from "@coder/logger"

 export interface Paths {
  data: string
--- a/test/package.json
+++ b/test/package.json
@ -17,7 +17,5 @@
  },
  "resolutions": {
    "@playwright/test/playwright": "^1.11.0-next-alpha-apr-13-2021"
-  },
-  "dependencies": {
  }
 }
--- a/test/unit/cli.test.ts
+++ b/test/unit/cli.test.ts
@ -349,6 +349,21 @@ describe("parser", () => {
      ],
    })
  })
+  it("should parse options with double-dash and multiple equal signs ", async () => {
+    const args = parse(
+      [
+        "--hashed-password=$argon2i$v=19$m=4096,t=3,p=1$0qr/o+0t00hsbjfqcksfdq$ofcm4rl6o+b7oxpua4qlxubypbbpsf+8l531u7p9hyy",
+      ],
+      {
+        configFile: "/pathtoconfig",
+      },
+    )
+    expect(args).toEqual({
+      _: [],
+      "hashed-password":
+        "$argon2i$v=19$m=4096,t=3,p=1$0qr/o+0t00hsbjfqcksfdq$ofcm4rl6o+b7oxpua4qlxubypbbpsf+8l531u7p9hyy",
+    })
+  })
 })

 describe("cli", () => {
@ -426,25 +441,25 @@ describe("cli", () => {

 describe("splitOnFirstEquals", () => {
  it("should split on the first equals", () => {
-    const testStr = "--enabled-proposed-api=test=value"
+    const testStr = "enabled-proposed-api=test=value"
    const actual = splitOnFirstEquals(testStr)
-    const expected = ["--enabled-proposed-api", "test=value"]
+    const expected = ["enabled-proposed-api", "test=value"]
    expect(actual).toEqual(expect.arrayContaining(expected))
  })
  it("should split on first equals regardless of multiple equals signs", () => {
    const testStr =
-      "--hashed-password=$argon2i$v=19$m=4096,t=3,p=1$0qR/o+0t00hsbJFQCKSfdQ$oFcM4rL6o+B7oxpuA4qlXubypbBPsf+8L531U7P9HYY"
+      "hashed-password=$argon2i$v=19$m=4096,t=3,p=1$0qR/o+0t00hsbJFQCKSfdQ$oFcM4rL6o+B7oxpuA4qlXubypbBPsf+8L531U7P9HYY"
    const actual = splitOnFirstEquals(testStr)
    const expected = [
-      "--hashed-password",
+      "hashed-password",
      "$argon2i$v=19$m=4096,t=3,p=1$0qR/o+0t00hsbJFQCKSfdQ$oFcM4rL6o+B7oxpuA4qlXubypbBPsf+8L531U7P9HYY",
    ]
    expect(actual).toEqual(expect.arrayContaining(expected))
  })
-  it("should always return two elements", () => {
-    const testStr = ""
+  it("should always return the first element before an equals", () => {
+    const testStr = "auth="
    const actual = splitOnFirstEquals(testStr)
-    const expected = ["", ""]
+    const expected = ["auth"]
    expect(actual).toEqual(expect.arrayContaining(expected))
  })
 })
--- a/typings/pluginapi.d.ts
+++ b/typings/pluginapi.d.ts
@ -145,7 +145,11 @@ export const proxy: ProxyServer
 /**
 * Middleware to ensure the user is authenticated. Throws if they are not.
 */
-export function ensureAuthenticated(req: express.Request, res?: express.Response, next?: express.NextFunction): Promise<void>
+export function ensureAuthenticated(
+  req: express.Request,
+  res?: express.Response,
+  next?: express.NextFunction,
+): Promise<void>

 /**
 * Returns true if the user is authenticated.