feat: Add strict mode to parser #74

aaronccasanova · 2022-03-03T14:52:48Z

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

…to feat/restructure-options-api

…onccasanova/parseargs into feat/strict-mode

aaronccasanova · 2022-03-03T15:11:45Z

Converted to draft until we get some consensus in #11

index.js

@aaronccasanova

…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 <ljharb@gmail.com> Co-authored-by: Aaron Casanova <32409546+aaronccasanova@users.noreply.github.com>

bcoe · 2022-04-08T14:47:25Z

@aaronccasanova bother you for a rebase?

test/index.js

…to feat/strict-mode

bcoe · 2022-04-11T12:42:17Z

@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.

…to feat/strict-mode

shadowspawn · 2022-04-11T21:10:38Z

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'

…ict:false

aaronccasanova · 2022-04-12T03:53:07Z

index.js

+      throw new ERR_UNKNOWN_OPTION(shortOption == null ? `--${longOption}` : `-${shortOption}`);
+    }
+
+    const shortOptionErr = optionConfig.short ? `-${optionConfig.short}, ` : '';


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

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

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:

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}, ` : '';

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!

@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.)

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.

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

test/index.js

…to feat/strict-mode

bcoe

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

bcoe · 2022-04-13T02:01:27Z

index.js

+}) {
+  const hasOptionConfig = ObjectHasOwn(options, longOption);
+
+  const optionConfig = hasOptionConfig ? options[longOption] : {};


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

Co-authored-by: Benjamin E. Coe <bencoe@google.com>

aaronccasanova · 2022-04-13T05:58:18Z

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 · 2022-04-13T08:03:49Z

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

aaronccasanova and others added 19 commits February 8, 2022 17:34

feat: Restructure options API

697493f

chore: Remove debug comments

9e42db0

chore: Remove debug comments

36ef68c

chore: Alias args to argv to introduce less changes

67c0b03

feat: Replace option with

082bec5

docs: Update README to reflect updated implementation

0909008

chore: Revert args options to argv

042480b

feat: Add strict mode to parser

10df671

docs: Update README from PR feedback

e44c46c

Merge branch 'main' of https://github.com/aaronccasanova/parseargs in…

4063751

…to feat/restructure-options-api

chore: Reduce changes

a50e940

chore: Tidy up naming conventions

062bdc9

Merge branch 'main' into feat/restructure-options-api

28ae7b8

Merge branch 'feat/restructure-options-api' of https://github.com/aar…

e6fea72

…onccasanova/parseargs into feat/strict-mode

fix: Guard against prototype member access

ef06099

Merge branch 'feat/restructure-options-api' of https://github.com/aar…

c228484

…onccasanova/parseargs into feat/strict-mode

feat: Add strict mode type validation

dd4f718

chore: Remove custom unknown option error

26df81c

chore: Remove unknown option error from test

a04d11c

aaronccasanova marked this pull request as draft March 3, 2022 15:10

aaronccasanova commented Mar 3, 2022

View reviewed changes

index.js Outdated Show resolved Hide resolved

shadowspawn reviewed Mar 3, 2022

View reviewed changes

index.js Outdated Show resolved Hide resolved

fix: Update value validation to check against undefined

ca05b43

ljharb reviewed Mar 3, 2022

View reviewed changes

index.js Outdated Show resolved Hide resolved

shadowspawn mentioned this pull request Mar 4, 2022

refactor!: parsing, revisit short option groups, add support for combined short and value #75

Merged

shadowspawn mentioned this pull request Apr 3, 2022

feat!: switch type:string option arguments to greedy, but with error for suspect cases in strict mode #88

Merged

bcoe reviewed Apr 8, 2022

View reviewed changes

test/index.js Show resolved Hide resolved

aaronccasanova added 2 commits April 10, 2022 09:44

fix(WIP): Update checks to use the ERR_INVALID_ARG_VALUE class

948ca9e

Merge branch 'main' of https://github.com/aaronccasanova/parseargs in…

a2ea48d

…to feat/strict-mode

Merge branch 'main' of https://github.com/aaronccasanova/parseargs in…

38575cd

…to feat/strict-mode

aaronccasanova added 3 commits April 11, 2022 17:43

fix: Revert back to custom error classes and improve error messages

99355dd

WIP: Default parseArgs to strict:true and update failing tests to str…

e66a8b3

…ict:false

Update README to reflect strict:true behavior

a4fc92a

aaronccasanova commented Apr 12, 2022

View reviewed changes

shadowspawn reviewed Apr 12, 2022

View reviewed changes

README.md Outdated Show resolved Hide resolved

shadowspawn reviewed Apr 12, 2022

View reviewed changes

README.md Outdated Show resolved Hide resolved

shadowspawn reviewed Apr 12, 2022

View reviewed changes

test/index.js Show resolved Hide resolved

shadowspawn reviewed Apr 12, 2022

View reviewed changes

test/index.js Show resolved Hide resolved

aaronccasanova added 6 commits April 12, 2022 11:52

fix: Improve robustness of the short option error message

12fd0e6

fix: Improve error message for type:string options missing an argument

870cffe

fix: Namespace unique parseArgs error classes

694c75d

docs: Update README examples

9b76d55

Merge branch 'main' of https://github.com/aaronccasanova/parseargs in…

fcb8c8d

…to feat/strict-mode

docs: Update remaing examples for consistency

e56246c

aaronccasanova marked this pull request as ready for review April 12, 2022 19:08

bcoe requested changes Apr 13, 2022

View reviewed changes

bcoe approved these changes Apr 13, 2022

View reviewed changes

shadowspawn approved these changes Apr 13, 2022

View reviewed changes

Update index.js

b4e0daf

Co-authored-by: Benjamin E. Coe <bencoe@google.com>

shadowspawn merged commit 8267d02 into pkgjs:main Apr 13, 2022

github-actions bot mentioned this pull request Apr 13, 2022

chore(main): release 0.7.0 #103

Merged

This was referenced Apr 13, 2022

More pollution protection? #104

Closed

Is strict actually a goal? #11

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add strict mode to parser #74

feat: Add strict mode to parser #74

aaronccasanova commented Mar 3, 2022 •

edited

Loading

aaronccasanova commented Mar 3, 2022

bcoe commented Apr 8, 2022

bcoe commented Apr 11, 2022

shadowspawn commented Apr 11, 2022

aaronccasanova Apr 12, 2022 •

edited

Loading

shadowspawn Apr 12, 2022 •

edited

Loading

aaronccasanova Apr 12, 2022

aaronccasanova Apr 12, 2022

shadowspawn Apr 12, 2022 •

edited

Loading

shadowspawn Apr 13, 2022 •

edited

Loading

shadowspawn Apr 13, 2022

bcoe Apr 13, 2022

bcoe left a comment

bcoe Apr 13, 2022

aaronccasanova commented Apr 13, 2022

shadowspawn commented Apr 13, 2022

feat: Add strict mode to parser #74

feat: Add strict mode to parser #74

Conversation

aaronccasanova commented Mar 3, 2022 • edited Loading

Errors on:

Examples:

aaronccasanova commented Mar 3, 2022

bcoe commented Apr 8, 2022

bcoe commented Apr 11, 2022

shadowspawn commented Apr 11, 2022

aaronccasanova Apr 12, 2022 • edited Loading

Choose a reason for hiding this comment

shadowspawn Apr 12, 2022 • edited Loading

Choose a reason for hiding this comment

aaronccasanova Apr 12, 2022

Choose a reason for hiding this comment

aaronccasanova Apr 12, 2022

Choose a reason for hiding this comment

shadowspawn Apr 12, 2022 • edited Loading

Choose a reason for hiding this comment

shadowspawn Apr 13, 2022 • edited Loading

Choose a reason for hiding this comment

shadowspawn Apr 13, 2022

Choose a reason for hiding this comment

bcoe Apr 13, 2022

Choose a reason for hiding this comment

bcoe left a comment

Choose a reason for hiding this comment

bcoe Apr 13, 2022

Choose a reason for hiding this comment

aaronccasanova commented Apr 13, 2022

shadowspawn commented Apr 13, 2022

aaronccasanova commented Mar 3, 2022 •

edited

Loading

aaronccasanova Apr 12, 2022 •

edited

Loading

shadowspawn Apr 12, 2022 •

edited

Loading

shadowspawn Apr 12, 2022 •

edited

Loading

shadowspawn Apr 13, 2022 •

edited

Loading