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

# Backport This will backport the following commits from `main` to `8.8`: - [[cli] Adjust CLI to treat extra options consistently (#158257)](https://github.com/elastic/kibana/pull/158257)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Alex Szabo <alex.szabo@elastic.co>
2025-04-24 09:48:58 -04:00 · 2023-05-30 13:18:52 -04:00 · 2023-05-30 13:18:52 -04:00 · 0841742ec8
commit 0841742ec8
parent aee0c5dea9
3 changed files with 158 additions and 16 deletions
--- a/src/cli/command.js
+++ b/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);
      });
    }
--- a/src/cli/command.test.js
+++ b/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];
+}
--- a/src/cli/serve/integration_tests/serverless_config_flag.test.ts
+++ b/src/cli/serve/integration_tests/serverless_config_flag.test.ts
@ -28,7 +28,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);