Commander.js: When arguments() argument is provided, unrecognized flags are silently deleted (no error prints)

[mcclure]

I am using commander 2.9.0 (graceful-readlink 1.0.1) on node v4.4.7 (on Windows, running in MSYS2, if it matters).

I run the following test program:

var commander = require("commander")

commander
    .version("0.0.1")
    .option('-x, --x-flag', 'A flag')
    .arguments("<file>")
    .parse(process.argv)

if (commander.xFlag)
    console.log("Got -x")
console.log("Args:", commander.args)

I find that if I do not supply the <file> argument, if I pass a nonsense flag like --nonsense, I will get a nice error message. If I _do_ supply the <file> argument, then any nonsense flags included will be silently discarded. Moreover, commander appears to assume each nonsense flag takes one option-argument, and it silently discards that, too. These flags are not appended to commander.args or anything. They just disappear.

A demonstration:

$ node testcommander.js
Args: []

$ node testcommander.js -x
Got -x
Args: []

$ node testcommander.js --nonsense -x

  error: unknown option `--nonsense'

$ node testcommander.js -x 1 2 3
Got -x
Args: [ '1', '2', '3' ]

$ node testcommander.js --NONSENSE 1 2 3
Args: [ '2', '3' ]

$ node testcommander.js 1 2 3 --NON-SENSE
Args: [ '1', '2', '3' ]

$ node testcommander.js 1 2 3 --NON-SENSE QQDFFR -x
Got -x
Args: [ '1', '2', '3' ]

Expected behavior: A --flag which is not recognized should result in commander printing an error.

This is a pretty bad bug. Not only does commander allow something which should be an error, and not only does it do it inconsistently for the same script depending on input, but since commander does this totally silently I cannot even write my own recovery code. If the lost arguments had been appended to commander.args at least I could detect it and raise my own error.

[kachkaev]

@mcclure have you found a workaround for this? I agree that not throwing when invalid arguments or options are given can be very dangerous. It's so easy to just type a wrong command and accidentally delete something important!

[matsaleh13]

I'll add that I just got bitten by this today, spending a while debugging my code only to find my command line inputs were wrong. Expected valid option was --concurrency, but I fatfingered it as --condurrency, and because I also had provided an expected argument at the end, everything went forward as if all inputs were valid.

Just for the sake of clarity, my invalid command line was:

node loadtester.js --condurrency 100 --users 1000 devtest_noop_follower.js

but it should have been:

node loadtester.js --concurrency 100 --users 1000 devtest_noop_follower.js

My code that uses commander is something like this:

const program = require('commander')

program
    .usage('[options] <file>')
    .arguments('<file>')
    .option('-c, --concurrency <number>', 'Max number of concurrent users.', parseInt)
    .option('-u, --users <number>', 'Max number of users.', parseInt)
    .action(file => {
        testPlanFile = file;
    })
    .parse(process.argv);

If I do not pass the argument, the invalid option --condurrency is detected and I get the appropriate error:

error: unknown option `--condurrency'

[matsaleh13]

I think I found the place in the commander.js code that controls this, but unfortunately it seems to be deliberate (from index.js):

Command.prototype.parseArgs = function(args, unknown) {
  var name;

  if (args.length) {
    name = args[0];
    if (this.listeners(name).length) {
      this.emit(args.shift(), args, unknown);
    } else {
      this.emit('*', args);
    }
  } else {
    outputHelpIfNecessary(this, unknown);

    // If there were no args and we have unknown options,
    // then they are extraneous and we need to error.
    if (unknown.length > 0) {
      this.unknownOption(unknown[0]);
    }
  }

  return this;
};

However, I'm not sure which use case this should really be valid for.

By this point in the code, the unknown argument contains the --condurrency (invalid) option, and the args array contains the correct args passed to the program.

Also, this implementation changes behavior depending on whether or not args is populated, rather than on the rules specified by the programmer. If args contains data, then ignore unknown options. I should think unknown options would handled the same regardless of whether args were passed.

As an experiment, I changed the code to:

Command.prototype.parseArgs = function(args, unknown) {
  var name;

  if (args.length) {
    name = args[0];
    if (this.listeners(name).length) {
      this.emit(args.shift(), args, unknown);
    } else {
      this.emit('*', args);
    }
  } else {
    outputHelpIfNecessary(this, unknown);
  }

  // Handle unknown options whether or not args are passed.
  if (unknown.length > 0) {
      this.unknownOption(unknown[0]);
  }

  return this;
};

This had the desired effect (desired by me anyway ;)): Both of the following command lines produced a useful error message:

node loadtester.js --condurrency 100

  error: unknown option `--condurrency'

and

node loadtester.js --condurrency 100 path_to_file

  error: unknown option `--condurrency'

I'd love to submit a pull request to fix this, but I don't know enough about the rest of the project (at the moment) to know whether I'm on the right track, or missing something important.

Anyone care to guide me at this point?

Thanks!

[charlierudolph]

@vanesyan @abetomo you two appear to be the last two to commit code to this library. I would really like to have this fixed and would be willing to send a PR. Any feedback on this before I make a PR?

[kingerking]

In my case the command works without any options being passed but when i try to pass a option it gives me this error 😕 This is my code:
program.command('edit <fileURL> <key> <value>') .description("Will edit a json file.") .option('-s, --spaces <number>', "Number of spaces to re-parse the file with.") .action((url, key, value, cmd) => {

[shadowspawn]

This has not seen a lot of activity, but it is a clear bug, and I have reproduced it on Mac.

[SlurpTheo]

Had a script passing accidentally -arg <argVal> <someVal> instead of --arg <argVal> <someVal>.

// test.ts
import * as commander from "commander";
const fn = (opt: string) => (a: string) => console.log(`in ${opt}${a ? ` -- got: '${a}'` : ""}`);
commander
   .option("-a, --abc <arg>", "Pass me a value", fn("abc"))
   .option("-x", "Dummy x", fn("x"))
   .option("-y", "Dummy y", fn("y"))
   .parse(process.argv);

Fine/good:

$ node test --help
Usage: test [options]

Options:
  -a, --abc <arg>  Pass me a value
  -x               Dummy x
  -y               Dummy y
  -h, --help       output usage information

$ node test -xy -y --abc ./test.js
in x
in y
in y
in abc -- got: './test.js'

$ node test -xy -y --abc ./test.js someVal
in x
in y
in y
in abc -- got: './test.js'

$

I guess this is okay (at least there's an error -- _after_ my option-handling function is called with "garbage"):

$ node test -xy -y -abc ./test.js
in x
in y
in y
in abc -- got: '-b'
error: unknown option `-c'

$

But ICK!!

$ node test -xy -y -abc ./test.js someVal
in x
in y
in y
in abc -- got: '-b'

$

[shadowspawn]

965 will fix the detection of unknown options in the case the program has an action handler. The original report in this issue (https://github.com/tj/commander.js/issues/561#issue-172306010) does not have an action handler so won't be changed by #965.

[tageecc]

use allowUnknownOption

    .command('cli')
    .description('123456.')
    .allowUnknownOption()
    .action(() => {
        const cli = ['otherCli', ...process.argv.slice(2)];
        spawn.sync('npx', cli, { stdio: 'inherit' });
    });

[mcclure]

@tageecc , what does that do? I don't find it in the doc.

In general I do not want to allow unknown options. I want unknown options to throw an error.

[shadowspawn]

Opened a PR to rework option parsing to improve option parsing: #1138

Edit: #1138 only fixes some of the cases in original post, and somewhat by accident.

[shadowspawn]

The invalid option will get detected if the program has an action handler, even an empty one. This is a limitation due to the current implementation rather than by design.

This is actually mentioned in the README, although easily missed!

However, if an action-based command does not define an action, then the options are not validated.

[shadowspawn]

Unknown options are reliably detected whether or not there is an action handler in the published pre-release of Commander v5.0.0 (#1163)

[shadowspawn]

Commander v5.0.0 has been released with significant improvements to argument parsing.

https://github.com/tj/commander.js/releases/tag/v5.0.0