Skip to content

Commit ecf2dec

Browse files
shadowspawnshadowspawn
authored
fix: resist pollution (#106)
1 parent e5d1f4b commit ecf2dec

3 files changed

Lines changed: 179 additions & 70 deletions

File tree

‎index.js‎

Lines changed: 84 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -6,8 +6,9 @@ const {
66
ArrayPrototypeShift,
77
ArrayPrototypeSlice,
88
ArrayPrototypePush,
9-
ObjectPrototypeHasOwnProperty: ObjectHasOwn,
9+
ObjectDefineProperty,
1010
ObjectEntries,
11+
ObjectPrototypeHasOwnProperty: ObjectHasOwn,
1112
StringPrototypeCharAt,
1213
StringPrototypeIncludes,
1314
StringPrototypeIndexOf,
@@ -29,7 +30,9 @@ const {
2930
isLongOptionAndValue,
3031
isOptionValue,
3132
isShortOptionAndValue,
32-
isShortOptionGroup
33+
isShortOptionGroup,
34+
objectGetOwn,
35+
optionsGetOwn
3336
} = require('./utils');
3437

3538
const {
@@ -74,61 +77,83 @@ function getMainArgs() {
7477
return ArrayPrototypeSlice(process.argv, 2);
7578
}
7679

77-
const protoKey = '__proto__';
78-
79-
function storeOption({
80-
strict,
81-
options,
82-
result,
83-
longOption,
84-
shortOption,
85-
optionValue,
86-
}) {
87-
const hasOptionConfig = ObjectHasOwn(options, longOption);
88-
const optionConfig = hasOptionConfig ? options[longOption] : {};
89-
90-
if (strict) {
91-
if (!hasOptionConfig) {
92-
throw new ERR_PARSE_ARGS_UNKNOWN_OPTION(shortOption == null ? `--${longOption}` : `-${shortOption}`);
93-
}
94-
95-
const shortOptionErr = ObjectHasOwn(optionConfig, 'short') ? `-${optionConfig.short}, ` : '';
80+
/**
81+
* In strict mode, throw for usage errors.
82+
*
83+
* @param {string} longOption - long option name e.g. 'foo'
84+
* @param {string|undefined} optionValue - value from user args
85+
* @param {Object} options - option configs, from parseArgs({ options })
86+
* @param {string} shortOrLong - option used, with dashes e.g. `-l` or `--long`
87+
* @param {boolean} strict - show errors, from parseArgs({ strict })
88+
*/
89+
function checkOptionUsage(longOption, optionValue, options,
90+
shortOrLong, strict) {
91+
// Strict and options are used from local context.
92+
if (!strict) return;
9693

97-
if (options[longOption].type === 'string' && optionValue == null) {
98-
throw new ERR_PARSE_ARGS_INVALID_OPTION_VALUE(`Option '${shortOptionErr}--${longOption} <value>' argument missing`);
99-
}
94+
if (!ObjectHasOwn(options, longOption)) {
95+
throw new ERR_PARSE_ARGS_UNKNOWN_OPTION(shortOrLong);
96+
}
10097

101-
if (options[longOption].type === 'boolean' && optionValue != null) {
102-
throw new ERR_PARSE_ARGS_INVALID_OPTION_VALUE(`Option '${shortOptionErr}--${longOption}' does not take an argument`);
103-
}
98+
const short = optionsGetOwn(options, longOption, 'short');
99+
const shortAndLong = short ? `-${short}, --${longOption}` : `--${longOption}`;
100+
const type = optionsGetOwn(options, longOption, 'type');
101+
if (type === 'string' && typeof optionValue !== 'string') {
102+
throw new ERR_PARSE_ARGS_INVALID_OPTION_VALUE(`Option '${shortAndLong} <value>' argument missing`);
103+
}
104+
// (Idiomatic test for undefined||null, expecting undefined.)
105+
if (type === 'boolean' && optionValue != null) {
106+
throw new ERR_PARSE_ARGS_INVALID_OPTION_VALUE(`Option '${shortAndLong}' does not take an argument`);
104107
}
108+
}
105109

106-
if (longOption === protoKey) {
110+
/**
111+
* Store the option value in `values`.
112+
*
113+
* @param {string} longOption - long option name e.g. 'foo'
114+
* @param {string|undefined} optionValue - value from user args
115+
* @param {Object} options - option configs, from parseArgs({ options })
116+
* @param {Object} values - option values returned in `values` by parseArgs
117+
*/
118+
function storeOption(longOption, optionValue, options, values) {
119+
if (longOption === '__proto__') {
107120
return;
108121
}
109122

110-
// Values
111-
const usedAsFlag = optionValue === undefined;
112-
const newValue = usedAsFlag ? true : optionValue;
113-
if (optionConfig.multiple) {
114-
// Always store value in array, including for flags.
115-
// result.values[longOption] starts out not present,
123+
// Can be removed when value has a null prototype
124+
const safeAssignProperty = (obj, prop, value) => {
125+
ObjectDefineProperty(obj, prop, {
126+
value,
127+
writable: true,
128+
enumerable: true,
129+
configurable: true
130+
});
131+
};
132+
133+
// We store based on the option value rather than option type,
134+
// preserving the users intent for author to deal with.
135+
const newValue = optionValue ?? true;
136+
if (optionsGetOwn(options, longOption, 'multiple')) {
137+
// Always store value in array, including for boolean.
138+
// values[longOption] starts out not present,
116139
// first value is added as new array [newValue],
117140
// subsequent values are pushed to existing array.
118-
if (result.values[longOption] !== undefined)
119-
ArrayPrototypePush(result.values[longOption], newValue);
120-
else
121-
result.values[longOption] = [newValue];
141+
if (ObjectHasOwn(values, longOption)) {
142+
ArrayPrototypePush(values[longOption], newValue);
143+
} else {
144+
safeAssignProperty(values, longOption, [newValue]);
145+
}
122146
} else {
123-
result.values[longOption] = newValue;
147+
safeAssignProperty(values, longOption, newValue);
124148
}
125149
}
126150

127-
const parseArgs = ({
128-
args = getMainArgs(),
129-
strict = true,
130-
options = {}
131-
} = {}) => {
151+
const parseArgs = (config = { __proto__: null }) => {
152+
const args = objectGetOwn(config, 'args') ?? getMainArgs();
153+
const strict = objectGetOwn(config, 'strict') ?? true;
154+
const options = objectGetOwn(config, 'options') ?? { __proto__: null };
155+
156+
// Validate input configuration.
132157
validateArray(args, 'args');
133158
validateBoolean(strict, 'strict');
134159
validateObject(options, 'options');
@@ -137,7 +162,8 @@ const parseArgs = ({
137162
({ 0: longOption, 1: optionConfig }) => {
138163
validateObject(optionConfig, `options.${longOption}`);
139164

140-
validateUnion(optionConfig.type, `options.${longOption}.type`, ['string', 'boolean']);
165+
// type is required
166+
validateUnion(objectGetOwn(optionConfig, 'type'), `options.${longOption}.type`, ['string', 'boolean']);
141167

142168
if (ObjectHasOwn(optionConfig, 'short')) {
143169
const shortOption = optionConfig.short;
@@ -183,18 +209,13 @@ const parseArgs = ({
183209
const shortOption = StringPrototypeCharAt(arg, 1);
184210
const longOption = findLongOptionForShort(shortOption, options);
185211
let optionValue;
186-
if (options[longOption]?.type === 'string' && isOptionValue(nextArg)) {
212+
if (optionsGetOwn(options, longOption, 'type') === 'string' &&
213+
isOptionValue(nextArg)) {
187214
// e.g. '-f', 'bar'
188215
optionValue = ArrayPrototypeShift(remainingArgs);
189216
}
190-
storeOption({
191-
strict,
192-
options,
193-
result,
194-
longOption,
195-
shortOption,
196-
optionValue,
197-
});
217+
checkOptionUsage(longOption, optionValue, options, arg, strict);
218+
storeOption(longOption, optionValue, options, result.values);
198219
continue;
199220
}
200221

@@ -204,7 +225,7 @@ const parseArgs = ({
204225
for (let index = 1; index < arg.length; index++) {
205226
const shortOption = StringPrototypeCharAt(arg, index);
206227
const longOption = findLongOptionForShort(shortOption, options);
207-
if (options[longOption]?.type !== 'string' ||
228+
if (optionsGetOwn(options, longOption, 'type') !== 'string' ||
208229
index === arg.length - 1) {
209230
// Boolean option, or last short in group. Well formed.
210231
ArrayPrototypePush(expanded, `-${shortOption}`);
@@ -224,26 +245,22 @@ const parseArgs = ({
224245
const shortOption = StringPrototypeCharAt(arg, 1);
225246
const longOption = findLongOptionForShort(shortOption, options);
226247
const optionValue = StringPrototypeSlice(arg, 2);
227-
storeOption({
228-
strict,
229-
options,
230-
result,
231-
longOption,
232-
shortOption,
233-
optionValue,
234-
});
248+
checkOptionUsage(longOption, optionValue, options, `-${shortOption}`, strict);
249+
storeOption(longOption, optionValue, options, result.values);
235250
continue;
236251
}
237252

238253
if (isLoneLongOption(arg)) {
239254
// e.g. '--foo'
240255
const longOption = StringPrototypeSlice(arg, 2);
241256
let optionValue;
242-
if (options[longOption]?.type === 'string' && isOptionValue(nextArg)) {
257+
if (optionsGetOwn(options, longOption, 'type') === 'string' &&
258+
isOptionValue(nextArg)) {
243259
// e.g. '--foo', 'bar'
244260
optionValue = ArrayPrototypeShift(remainingArgs);
245261
}
246-
storeOption({ strict, options, result, longOption, optionValue });
262+
checkOptionUsage(longOption, optionValue, options, arg, strict);
263+
storeOption(longOption, optionValue, options, result.values);
247264
continue;
248265
}
249266

@@ -252,7 +269,8 @@ const parseArgs = ({
252269
const index = StringPrototypeIndexOf(arg, '=');
253270
const longOption = StringPrototypeSlice(arg, 2, index);
254271
const optionValue = StringPrototypeSlice(arg, index + 1);
255-
storeOption({ strict, options, result, longOption, optionValue });
272+
checkOptionUsage(longOption, optionValue, options, `--${longOption}`, strict);
273+
storeOption(longOption, optionValue, options, result.values);
256274
continue;
257275
}
258276

‎test/prototype-pollution.js‎

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,23 @@
44
const test = require('tape');
55
const { parseArgs } = require('../index.js');
66

7+
// These tests are not synced upstream with node, in case of possible side-effects.
8+
// See index.js for tests shared with upstream.
9+
10+
function setObjectPrototype(prop, value) {
11+
const oldDescriptor = Object.getOwnPropertyDescriptor(Object.prototype, prop);
12+
Object.prototype[prop] = value;
13+
return oldDescriptor;
14+
}
15+
16+
function restoreObjectPrototype(prop, oldDescriptor) {
17+
if (oldDescriptor == null) {
18+
delete Object.prototype[prop];
19+
} else {
20+
Object.defineProperty(Object.prototype, prop, oldDescriptor);
21+
}
22+
}
23+
724
test('should not allow __proto__ key to be set on object', (t) => {
825
const passedArgs = ['--__proto__=hello'];
926
const expected = { values: {}, positionals: [] };
@@ -13,3 +30,58 @@ test('should not allow __proto__ key to be set on object', (t) => {
1330
t.deepEqual(result, expected);
1431
t.end();
1532
});
33+
34+
test('when prototype has multiple then ignored', (t) => {
35+
const args = ['--foo', '1', '--foo', '2'];
36+
const options = { foo: { type: 'string' } };
37+
const expectedResult = { values: { foo: '2' }, positionals: [] };
38+
39+
const holdDescriptor = setObjectPrototype('multiple', true);
40+
const result = parseArgs({ args, options });
41+
restoreObjectPrototype('multiple', holdDescriptor);
42+
t.deepEqual(result, expectedResult);
43+
t.end();
44+
});
45+
46+
test('when prototype has type then ignored', (t) => {
47+
const args = ['--foo', '1'];
48+
const options = { foo: { } };
49+
50+
const holdDescriptor = setObjectPrototype('type', 'string');
51+
t.throws(() => {
52+
parseArgs({ args, options });
53+
});
54+
restoreObjectPrototype('type', holdDescriptor);
55+
t.end();
56+
});
57+
58+
test('when prototype has short then ignored', (t) => {
59+
const args = ['-f', '1'];
60+
const options = { foo: { type: 'string' } };
61+
62+
const holdDescriptor = setObjectPrototype('short', 'f');
63+
t.throws(() => {
64+
parseArgs({ args, options });
65+
});
66+
restoreObjectPrototype('short', holdDescriptor);
67+
t.end();
68+
});
69+
70+
test('when prototype has strict then ignored', (t) => {
71+
const args = ['-f'];
72+
73+
const holdDescriptor = setObjectPrototype('strict', false);
74+
t.throws(() => {
75+
parseArgs({ args });
76+
});
77+
restoreObjectPrototype('strict', holdDescriptor);
78+
t.end();
79+
});
80+
81+
test('when prototype has args then ignored', (t) => {
82+
const holdDescriptor = setObjectPrototype('args', ['--foo']);
83+
const result = parseArgs({ strict: false });
84+
restoreObjectPrototype('args', holdDescriptor);
85+
t.false(result.values.foo);
86+
t.end();
87+
});

‎utils.js‎

Lines changed: 23 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
const {
44
ArrayPrototypeFind,
55
ObjectEntries,
6+
ObjectPrototypeHasOwnProperty: ObjectHasOwn,
67
StringPrototypeCharAt,
78
StringPrototypeIncludes,
89
StringPrototypeSlice,
@@ -20,6 +21,22 @@ const {
2021
//
2122
// These routines are for internal use, not for export to client.
2223

24+
/**
25+
* Return the named property, but only if it is an own property.
26+
*/
27+
function objectGetOwn(obj, prop) {
28+
if (ObjectHasOwn(obj, prop))
29+
return obj[prop];
30+
}
31+
32+
/**
33+
* Return the named options property, but only if it is an own property.
34+
*/
35+
function optionsGetOwn(options, longOption, prop) {
36+
if (ObjectHasOwn(options, longOption))
37+
return objectGetOwn(options[longOption], prop);
38+
}
39+
2340
/**
2441
* Determines if the argument may be used as an option value.
2542
* NB: We are choosing not to accept option-ish arguments.
@@ -107,7 +124,7 @@ function isShortOptionGroup(arg, options) {
107124

108125
const firstShort = StringPrototypeCharAt(arg, 1);
109126
const longOption = findLongOptionForShort(firstShort, options);
110-
return options[longOption]?.type !== 'string';
127+
return optionsGetOwn(options, longOption, 'type') !== 'string';
111128
}
112129

113130
/**
@@ -128,7 +145,7 @@ function isShortOptionAndValue(arg, options) {
128145

129146
const shortOption = StringPrototypeCharAt(arg, 1);
130147
const longOption = findLongOptionForShort(shortOption, options);
131-
return options[longOption]?.type === 'string';
148+
return optionsGetOwn(options, longOption, 'type') === 'string';
132149
}
133150

134151
/**
@@ -144,7 +161,7 @@ function findLongOptionForShort(shortOption, options) {
144161
validateObject(options, 'options');
145162
const { 0: longOption } = ArrayPrototypeFind(
146163
ObjectEntries(options),
147-
({ 1: optionConfig }) => optionConfig.short === shortOption
164+
({ 1: optionConfig }) => objectGetOwn(optionConfig, 'short') === shortOption
148165
) || [];
149166
return longOption || shortOption;
150167
}
@@ -156,5 +173,7 @@ module.exports = {
156173
isLongOptionAndValue,
157174
isOptionValue,
158175
isShortOptionAndValue,
159-
isShortOptionGroup
176+
isShortOptionGroup,
177+
objectGetOwn,
178+
optionsGetOwn
160179
};

0 commit comments

Comments
 (0)