[cli] Adjust CLI to treat extra options consistently (#158257)

## Summary

Addresses: #105371 
One can use `--elasticsearch.password ---my-password---`,
`--elasticsearch.password=---mypassword---` or `--elasticsearch.password
\"---mypassword\"` both in these cases. Commander is splitting
single-dash inputs, it can't be prevented, so for those cases, use
quotes or compound options.

The original problem can be reproduced like this:
```shell
yarn start --elasticsearch.password "-some-password"
```

We were using some extra argument parsing prototype extension in
(command.js)[./src/cli/command.js] - this allows for collecting extra
options, but the proprietary logic had a few incorrect assumptions.
(e.g.: extra opts will be already split by `=` by the time we get them,
shorthands will be split to individual flags).

(1) This change makes it explicit in the code, that the single-letter
flags (`-fcz`) are not supported in the extra options.

The core of the problem is now how we treat an ambiguous expression like
this:
```
kibana --password "--kebabs--are--my--favourite--" --somethingElse 283
```
Is the `"--kebabs--are--my--favourite--"` another flag, or is it a value
for the previous option?

Unfortunately, both interpretations will have shortcomings. 
- if we treat it as a boolean flag, then values starting with "--" will
be incorrectly treated as flags, and an error will be displayed (in the
case of a very unfortunate random generated string)
- if we treat it as a value always, then incorrectly ordered
options/flags will be consumed in place of values

Maybe the latter one will happen less often. For that, people would have
to incorrectly assume that `--booleanOn` type flags exist, especially
among the non-listed options. Or they'd have to incorrectly parameterize
the command, for that they can review their own usage locally, and see
their error in the ordering.

(2) This change makes it explicit, that extra options will come in
pairs.

Also, added tests for the base use case, and the _dash_ edge case.

### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [x] Checked and works on 7.17/7.13 (it was originally reported on
7.13).

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

src/cli/command.js

@@ -53,30 +53,44 @@ Command.prototype.collectUnknownOptions = function () {
   this.allowUnknownOption();
   this.getUnknownOptions = function () {
     const opts = {};
+
+    /**
+     * Commander.js already presents the unknown args split by "=",
+     * and shorthand switches already split, like -asd => -a, -s, -d
+     */
     const unknowns = this.unknownArgv();
 
+    const singleFlagArgs = unknowns.filter((flag) => {
+      return flag.match(/^-[a-zA-Z0-9]$/);
+    });
+
+    if (singleFlagArgs.length) {
+      this.error(
+        `${title} shouldn't have unknown shorthand flag arguments (${singleFlagArgs}). Possibly an argumentation error?`
+      );
+    }
+
     while (unknowns.length) {
-      const opt = unknowns.shift().split('=');
-      if (opt[0].slice(0, 2) !== '--') {
-        this.error(`${title} "${opt[0]}" must start with "--"`);
+      const optName = unknowns.shift();
+
+      if (optName.slice(0, 2) !== '--') {
+        this.error(`${title} "${optName}" must start with "--"`);
       }
 
-      if (opt.length === 1) {
-        if (!unknowns.length || unknowns[0][0] === '-') {
-          this.error(`${title} "${opt[0]}" must have a value`);
-        }
-
-        opt.push(unknowns.shift());
+      if (unknowns.length === 0) {
+        this.error(`${title} "${optName}" must have a value`);
       }
 
-      let val = opt[1];
+      const optValue = unknowns.shift();
+
+      let val = optValue;
       try {
-        val = JSON.parse(opt[1]);
-      } catch (e) {
-        val = opt[1];
+        val = JSON.parse(optValue);
+      } catch {
+        val = optValue;
       }
 
-      set(opts, opt[0].slice(2), val);
+      set(opts, optName.slice(2), val);
     }
 
     return opts;
@@ -96,7 +110,7 @@ Command.prototype.action = _.wrap(Command.prototype.action, function (action, fn
     const ret = fn.apply(this, args);
     if (ret && typeof ret.then === 'function') {
       ret.then(null, function (e) {
-        console.log('FATAL CLI ERROR', e.stack);
+        console.log('FATAL CLI ERROR', e.stack);
         process.exit(1);
       });
     }

src/cli/command.test.js

@@ -0,0 +1,128 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License
+ * 2.0 and the Server Side Public License, v 1; you may not use this file except
+ * in compliance with, at your election, the Elastic License 2.0 or the Server
+ * Side Public License, v 1.
+ */
+
+import commander from 'commander';
+
+import './command';
+
+describe('Commander extensions', () => {
+  const mockExit = jest.fn((exitCode) => {
+    // Prevent exiting from shell, let's throw something instead.
+    throw new Error('Exit ' + exitCode);
+  });
+
+  beforeAll(() => {
+    process._real_exit = process.exit;
+
+    process.exit = mockExit;
+  });
+
+  afterAll(() => {
+    process.exit = process._real_exit;
+  });
+
+  describe('regular options', () => {
+    it('collects multiple args', () => {
+      const args = makeArgs('test1', '--code', 'xoxo', '--param', 123);
+
+      const { collectedOptions, unknownOptions } = parseArgs('test1', ['code', 'param'], args);
+
+      expect(collectedOptions).toEqual({ code: 'xoxo', param: 123 });
+      expect(unknownOptions).toEqual({});
+    });
+  });
+
+  describe('unknown args', () => {
+    it('will be collected besides defined ones', () => {
+      const args = makeArgs('test2', '--code', 'xoxo', '--specialConfig', 'foobar');
+
+      const { collectedOptions, unknownOptions } = parseArgs('test2', ['code'], args);
+
+      expect(collectedOptions).toEqual({ code: 'xoxo' });
+      expect(unknownOptions).toEqual({ specialConfig: 'foobar' });
+    });
+
+    it('will be collected with dashes, when quoted', () => {
+      const args = makeArgs('test3', '--code', 'xoxo', '--specialConfig', '"---foobar"');
+
+      const { collectedOptions, unknownOptions } = parseArgs('test3', ['code'], args);
+
+      expect(collectedOptions).toEqual({ code: 'xoxo' });
+      expect(unknownOptions).toEqual({ specialConfig: '---foobar' });
+    });
+
+    it('will be collected with dashes as compound option', () => {
+      const args = makeArgs('test4', '--code', 'xoxo', '--specialConfig=----foobar');
+
+      const { collectedOptions, unknownOptions } = parseArgs('test4', ['code'], args);
+
+      expect(collectedOptions).toEqual({ code: 'xoxo' });
+      expect(unknownOptions).toEqual({ specialConfig: '----foobar' });
+    });
+
+    it('will crash if they contain bool flags', () => {
+      const args = makeArgs('test5', '--code', 'xoxo', '--secretOpt', '1234', '--useSpecial');
+
+      expect(() => parseArgs('test5', ['code'], args)).toThrow(/Exit/);
+
+      expect(mockExit).toHaveBeenCalledWith(64);
+    });
+
+    it('will crash if they contain shorthand flags', () => {
+      const args = makeArgs('test6', '--code', 'xoxo', '-xvz');
+
+      expect(() => parseArgs('test6', ['code'], args)).toThrow(/Exit/);
+
+      expect(mockExit).toHaveBeenCalledWith(64);
+    });
+  });
+});
+
+function prepareProgram(testName, options) {
+  const optionsDefinitions = options.map((optionName) => {
+    const short = optionName[0].toLowerCase();
+    return `-${short}, --${optionName} <${optionName}>`;
+  });
+
+  let command = commander.command(testName);
+
+  for (const option of optionsDefinitions) {
+    command = command.option(option);
+  }
+
+  return command;
+}
+
+function parseArgs(testName, options, args) {
+  let storedActionArgs = null;
+
+  const command = prepareProgram(testName, options)
+    .collectUnknownOptions()
+    .action((command) => {
+      const colllectedOptions = {};
+
+      options.forEach((opt) => {
+        colllectedOptions[opt] = command[opt];
+      });
+
+      storedActionArgs = colllectedOptions;
+    });
+
+  // Pass the args at the root command
+  commander.parse(args);
+
+  return {
+    collectedOptions: storedActionArgs,
+    unknownOptions: command.getUnknownOptions(),
+    command,
+  };
+}
+
+function makeArgs(...args) {
+  return ['node', 'test.js', ...args];
+}

src/cli/serve/integration_tests/serverless_config_flag.test.ts

@@ -51,7 +51,7 @@ describe('cli serverless project type', () => {
       expect(error).toBe(undefined);
 
       expect(stdout.toString('utf8')).toContain(
-        'FATAL CLI ERROR Error: invalid --serverless value, must be one of es, oblt, security'
+        'FATAL CLI ERROR Error: invalid --serverless value, must be one of es, oblt, security'
       );
 
       expect(status).toBe(1);