Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: Add strict mode to parser #74
Changes from 33 commits
697493f
9e42db0
36ef68c
67c0b03
082bec5
0909008
042480b
10df671
e44c46c
4063751
a50e940
062bdc9
28ae7b8
e6fea72
ef06099
c228484
dd4f718
26df81c
a04d11c
ca05b43
a1c3544
fa0775b
5b89108
1d412da
a16fc7b
c4271a0
b6107bb
948ca9e
a2ea48d
38575cd
99355dd
e66a8b3
a4fc92a
12fd0e6
870cffe
694c75d
9b76d55
fcb8c8d
e56246c
b4e0daf
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As @cjihrig suggests elsewhere, could se use:
To avoid needing to introduce a new error to
errors.js
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's an early WIP with all the strict error cases updated to use
ERR_INVALID_ARG_VALUE
: 948ca9eI could use some suggestions for the error messages as the new message constraint of
ERR_INVALID_ARG_VALUE
and my current implementation doesn't read entirely accurate:Really I'm hung up on the word
property
overoption
(e.g.The option --foo must...
), but not sure if I'm overthinking it. Just want the message to be super clear to users.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, the current unknown option message isn't entirely helpful to users e.g.
$ node unknown-option.js --foo TypeError: The property option.foo must be configured, Received 'undefined'
Any suggestions to make this message more user-facing? e.g.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a user I would expect the message to be "Unknown option --foo".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, that was the original message before the rework with the addition of dynamism for long and short options e.g.
Unknown long option --foo
andUnknown short option -f
. In an effort to reduce friction merging into Node I'm trying to find an equally helpful message within the constraints ofERR_INVALID_ARG_VALUE
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the format for
ERR_INVALID_ARG_VALUE
is a particularly good match for our use here where we are referring to options on the command line, rather than arguments to a javascript function. (Valiant try though!)I see the node 18.x branch has updated code for the error which inserts property/argument depending on the name, but still not a good match for our option errors.
https://github.com/nodejs/node/blob/ec5a359ffd38b6f56a456a0b6a482b5167fbfce1/lib/internal/errors.js#L1246
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted to custom error classes and updated the error messages per: #74 (comment)
Examples based on the PR description prompts:
type:'string'
used like a boolean option e.g. lone--string
type:'boolean'
used like a string option e.g.--boolean=foo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
My only minor wording musing is for the "Option of
type:'string'
used like a boolean option". With the option I used, I was briefly confusing by the long option name reused as the option argument name. I wonder aboutvalue
orarg
? But haven't convinced myself they are better, unless you really like one, stick with what you have!The reuse does tend to work semantically. I picked a long one out of the node options to try:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! That example is excessively long. I think I'll use
value
for consistency with theparseArgs
results object.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
870cffe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likewise, if we can avoid adding any new errors it simplifies merging into Node.js.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see: #97
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#74 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, is it possible to namespace our error codes so we can provide the most accurate messages to users?
e.g.
Unknown long option --foo
Unknown short option -f
Missing value for long option --foo with type:'string'
Unexpected value bar for short option -f with type:'boolean'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There appear to be error namespaces in node core:
@bcoe @shadowspawn @ljharb What are your thoughts on using a namespace for
parseArgs
errors. For exampleERR_PARGS_*
orERR_PARSEARGS_*
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My preferences would be
ERR_PARSE_ARGS
, rather than the short hand.But yes, I can't see any reason not to use a namespace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated unique
parseArgs
error classes toERR_PARSE_ARGS_*
: 694c75dThere was a problem hiding this comment.
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] ?? {}
.There was a problem hiding this comment.
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?There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
Not sure if this opens up any bugs or areas for malicious behavior, but might as well update the condition to:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
12fd0e6
There was a problem hiding this comment.
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:
multiple
type
, lots including utils. Although that might be ok if the next issue was fixed.type
at the top ofparseArgs
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!There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.