Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add strict mode to parser #74

Merged
merged 40 commits into from
Apr 13, 2022

Conversation

aaronccasanova
Copy link
Collaborator

@aaronccasanova aaronccasanova commented Mar 3, 2022

This PR introduces a strict mode parser config that is enabled by default.

Errors on:

  • Unknown option encountered
  • Option of type:'string' used like a boolean option e.g. lone --string
  • Option of type:'boolean' used like a string option e.g. --boolean=foo

Examples:

// node unknown-option.js --foo --bar
parseArgs({
  strict: true,
  options: { foo: { type: 'boolean' } },
});
// [Error]: Unknown option '--bar'
// node invalid-string-option.js --foo
parseArgs({
  strict: true,
  options: { foo: { short: 'f', type: 'string' } },
})
// [Error]: Option '-f, --foo <value>' argument missing
// node invalid-boolean-option.js --foo=bar
parseArgs({
  strict: true,
  options: { foo: { type: 'boolean' } },
})
// [Error]: Option '--foo' does not take an argument

@aaronccasanova aaronccasanova marked this pull request as draft March 3, 2022 15:10
@aaronccasanova
Copy link
Collaborator Author

Converted to draft until we get some consensus in #11

index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
shadowspawn added a commit that referenced this pull request Mar 12, 2022
…ined short and value (#75)


1) Refactor parsing to use independent blocks of code, rather than nested cascading context. This makes it easier to reason about the behaviour.

2) Split out small pieces of logic to named routines to improve readability, and allow extra documentation and examples without cluttering the parsing. (Thanks to @aaronccasanova for inspiration.)

3) Existing tests untouched to make it clear that the tested functionality has not changed.

4) Be more explicit about short option group expansion, and ready to throw error in strict mode for string option in the middle of the argument. (See #11 and #74.)

5) Add support for short option combined with value (without intervening `=`). This is what Commander and Open Group Utility Conventions do, but is _not_ what Yargs does. I don't want to block PR on this and happy to comment it out for further discussion if needed. (I have found some interesting variations in the wild.) [Edit: see also #78]

6) Add support for multiple unit tests files. Expand tests from 33 to 113, but many for internal routines rather than testing exposed API.

7) Added `.editorconfig` file


Co-authored-by: Jordan Harband <[email protected]>
Co-authored-by: Aaron Casanova <[email protected]>
@bcoe
Copy link
Collaborator

bcoe commented Apr 8, 2022

@aaronccasanova bother you for a rebase?

test/index.js Show resolved Hide resolved
@bcoe
Copy link
Collaborator

bcoe commented Apr 11, 2022

@aaronccasanova landed @shadowspawn's refactor this morning, so I think this and @bakkot's potential refactor around positionals are the last two blockers for MVP.

@shadowspawn
Copy link
Collaborator

In Commander, the error messages refer to the option with both the short and long form and the argument if applicable, rather than just what was used on the command line. So in same style as the usage might appear in the help. If the user used the short form, the long form is more informative about the option purpose.

% fab clone --branch
error: option '-b, --branch <branchname>' argument missing
% fab --silly
error: unknown option '--silly'

index.js Outdated
throw new ERR_UNKNOWN_OPTION(shortOption == null ? `--${longOption}` : `-${shortOption}`);
}

const shortOptionErr = optionConfig.short ? `-${optionConfig.short}, ` : '';
Copy link
Collaborator Author

@aaronccasanova aaronccasanova Apr 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still getting the hang of primordials. Do I need to do ObjectHasOwn(optionConfig, 'short') ? '...' : ''; here?

Copy link
Collaborator

@shadowspawn shadowspawn Apr 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was validated if present as a single character on entry, so no need for extra checks here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The initial validation only checks own properties of the option config. Couldn't immediatly think of a way to confirm, but came up with this demo:
image

Not sure if this opens up any bugs or areas for malicious behavior, but might as well update the condition to:

const shortOptionErr = ObjectHasOwn(optionConfig, 'short') ? `-${optionConfig.short}, ` : '';

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

@shadowspawn shadowspawn Apr 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arg, you are right! (Sorry, the validation wasn't doing what I thought.)

I assume the high level goal to behave the same as if the prototype pollution was not present?

That makes me question lots more of the code though. The options bag has introduced lots of property access:

  • same issue for multiple
  • and type, lots including utils. Although that might be ok if the next issue was fixed.
  • the validation of type at the top of parseArgs does not test it is not from prototype (from my recent change to make type required!)

Also, the destructing function parameters of both parseArgs and (now) storeOption receive properties from prototype if not specified by caller.

We could copy the input arguments into a safe(r) object before using them, by specifying all expected properties. Is this a pattern used in other node code? I suspect the usual pattern is just be paranoid everywhere because easier to be confident local code is being paranoid enough?!

I suggest only fix up the code introduced in this PR though. Get strict functional and separately have an more-paranoia-more-robust revisit!

Copy link
Collaborator

@shadowspawn shadowspawn Apr 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bcoe @ljharb
If my reading of this is correct, we are not currently robust against prototype pollution. Can we improve this after first version of an experimental feature?

(I have some ideas about how to improve it without littering the code and can have a go tonight, but aware clock is ticking.)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be clear, this is not a new problem. I looked back through major refactors and we have been vulnerable to prototype pollution in every iteration. Not causing pollution, but affected by prototype pollution. Opened #104 with demo.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the bigger concern for prototype pollution is untrusted input to the CLI modifying the prototype, vs., the user having mucked with proto in their application, and this affecting our parse.

This makes me think that the bigger security concern would be if we ever supported dot properties; as @shadowspawn has mentioned.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@aaronccasanova aaronccasanova marked this pull request as ready for review April 12, 2022 19:08
Copy link
Collaborator

@bcoe bcoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good to me, mostly left nits.

Only blocking from me, I prefer to check explicitly for === undefined, !== undefined, rather than a loose comparison.

Thanks for doing this work.

index.js Outdated Show resolved Hide resolved
index.js Show resolved Hide resolved
}) {
const hasOptionConfig = ObjectHasOwn(options, longOption);

const optionConfig = hasOptionConfig ? options[longOption] : {};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might simplify this to options[longOption] ?? {}.

Co-authored-by: Benjamin E. Coe <[email protected]>
@aaronccasanova
Copy link
Collaborator Author

Thanks for the reviews and feedback! I'm happy for this PR to be merged as is and willing to help with any quick follow ups. My last PR was merged by @shadowspawn and I'm not entirely sure if I should be merging into main. That being said, I don't want to block anyone, so feel free to merge and/or push up any updates!

@shadowspawn
Copy link
Collaborator

Taking a last look now. Thanks for all the hard work @aaronccasanova

@shadowspawn shadowspawn merged commit 8267d02 into pkgjs:main Apr 13, 2022
This was referenced Apr 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants