Repository navigation
Less throws for problematic arguments - #88
shadowspawn wants to merge 5 commits into
Conversation
- dotted overwrite of number or string - Object prototype property names - dotted modifiation of _ properties
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #88 +/- ##
==========================================
+ Coverage 98.78% 98.82% +0.03%
==========================================
Files 1 1
Lines 165 170 +5
Branches 70 73 +3
==========================================
+ Hits 163 168 +5
Misses 2 2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
I had a look at the action failure, but didn't work out the cause. It appears to be failing "Pre Harden Runner" in the "node tests". |
|
I was not sure what to produce when dotted option is added after a previous non-option value. I considered wrapping in an array to preserve previous value, but thought it was a bit too complicated to receive and went with simple overwrite. After looking again at existing minimist behaviour, and at yargs-parser behaviour (some differences), both will produce array with embedded object in various circumstances. I do not think these mixed option types are important use cases, but do prefer the behaviour to be self-consistent. I'm going to have another think about it... (Going back to draft.) Minimist example of array wrap, behaviour not affected by PR: % node plain.js --a.b=VALUE --a
{ _: [], a: [ { b: 'VALUE' }, true ] } |
|
I decided to keep the For reference, the yargs-parser array related code in the dotted notation processing is: |
Problem
Certain problematic arguments passed to
.parse()could cause Minimist to throw.#47 reported that passing Object property names such as
--toString,--hasOwnProperty,--constructor,--__proto__caused an expection.waydeshi from PayPal Cyber Security Team reported privately a couple of similar cases:
argv._properties on the parse results, particularly setting_.lengthto something invalid. (argv._holds the positionals.)Further related case noted by me:
Solution
Using option names which are also Object properties:
bools,strings, andaliasesnow use null prototypesetKeynow only considers own propertiesDotted options overwriting number/string/array in parse results:
setKeywhen traversing the dotted option keys and overwriting left-over from previous parsing, which might be a number or string or array.Dotted options overwriting
_in parse results:_and_.*insetKey. Simple and effective. Don't muck with the returned positionals.Threat Model
These are being fixed as bugs rather than a CVE. Quoting comment from @ljharb: