From 956b79a6a37e9ac8f7d67dbfbb5c6863be31b4f1 Mon Sep 17 00:00:00 2001 From: JoaoMarcos44 Date: Wed, 28 Oct 2026 15:17:33 -0300 Subject: [PATCH 1/5] util: do not negate explicit no-* options in parseArgs When `allowNegative` is enabled in `util.parseArgs()`, `storeOption()` previously stripped the `no-` prefix unconditionally, even when the option itself was explicitly defined in `options` (for example `'no-logfile'` or `'no-warnings'`). Check `!ObjectHasOwn(options, longOption)` before stripping `no-`, matching `checkOptionUsage()`, and avoid negating configured `'string'` options or `'__proto__'` in `strict: false` mode. Fixes: https://github.com/nodejs/node/issues/67085 Signed-off-by: JoaoMarcos44 --- lib/internal/util/parse_args/parse_args.js | 23 ++++-- test/parallel/test-parse-args.mjs | 84 ++++++++++++++++++++++ 2 files changed, 100 insertions(+), 7 deletions(-) diff --git a/lib/internal/util/parse_args/parse_args.js b/lib/internal/util/parse_args/parse_args.js index e8751c6e2225..459bbcd160e6 100644 --- a/lib/internal/util/parse_args/parse_args.js +++ b/lib/internal/util/parse_args/parse_args.js @@ -127,15 +127,24 @@ function checkOptionUsage(config, token) { function storeOption(token, options, values, allowNegative) { let longOption = token.name; let optionValue = token.value; - if (longOption === '__proto__') { - return; // No. Just no. - } - if (allowNegative && StringPrototypeStartsWith(longOption, 'no-') && optionValue === undefined) { + if ( + allowNegative && + !ObjectHasOwn(options, longOption) && + StringPrototypeStartsWith(longOption, 'no-') && + optionValue === undefined + ) { // Boolean option negation: --no-foo - longOption = StringPrototypeSlice(longOption, 3); - token.name = longOption; - optionValue = false; + const positiveOption = StringPrototypeSlice(longOption, 3); + if (optionsGetOwn(options, positiveOption, 'type') !== 'string') { + longOption = positiveOption; + token.name = longOption; + optionValue = false; + } + } + + if (longOption === '__proto__') { + return; // No. Just no. } // We store based on the option value rather than option type, diff --git a/test/parallel/test-parse-args.mjs b/test/parallel/test-parse-args.mjs index 706a3b24f1d0..cd1c40991e55 100644 --- a/test/parallel/test-parse-args.mjs +++ b/test/parallel/test-parse-args.mjs @@ -1025,3 +1025,87 @@ test('auto-detect --no-foo as negated when strict:false and allowNegative', () = assert.deepStrictEqual(result, expected); process.argv = holdArgv; }); + +test('explicit option starting with "no-" is not treated as negation when allowNegative is true', () => { + const options = { + 'color': { type: 'boolean' }, + 'logfile': { type: 'string' }, + 'no-logfile': { type: 'boolean' }, + 'no-warnings': { type: 'boolean', short: 'W', default: false }, + }; + const result = parseArgs({ + args: ['--no-color', '--no-logfile', '-W'], + options, + allowNegative: true, + tokens: true, + }); + const expected = { + values: { + '__proto__': null, + 'color': false, + 'no-logfile': true, + 'no-warnings': true, + }, + positionals: [], + tokens: [ + { + kind: 'option', + name: 'color', + rawName: '--no-color', + index: 0, + value: undefined, + inlineValue: undefined, + }, + { + kind: 'option', + name: 'no-logfile', + rawName: '--no-logfile', + index: 1, + value: undefined, + inlineValue: undefined, + }, + { + kind: 'option', + name: 'no-warnings', + rawName: '-W', + index: 2, + value: undefined, + inlineValue: undefined, + }, + ], + }; + assert.deepStrictEqual(result, expected); +}); + +test('explicit boolean option starting with "no-" can itself be negated with "--no-no-"', () => { + const options = { + 'no-warnings': { type: 'boolean', default: true }, + }; + const result = parseArgs({ + args: ['--no-no-warnings'], + options, + allowNegative: true, + }); + const expected = { + values: { '__proto__': null, 'no-warnings': false }, + positionals: [], + }; + assert.deepStrictEqual(result, expected); +}); + +test('strict:false with allowNegative does not negate configured string option or __proto__', () => { + const options = { + alpha: { type: 'string' }, + }; + const result = parseArgs({ + args: ['--alpha', 'val', '--no-alpha', '--no-__proto__'], + options, + strict: false, + allowNegative: true, + }); + const expected = { + values: { '__proto__': null, 'alpha': 'val', 'no-alpha': true }, + positionals: [], + }; + assert.deepStrictEqual(result, expected); +}); From 43a57a19f5225d3fae60403f07e4e535ea307e20 Mon Sep 17 00:00:00 2001 From: JoaoMarcos44 Date: Wed, 28 Oct 2026 16:34:12 -0300 Subject: [PATCH 2/5] test: cover strict parseArgs negation targets Cover explicit negative options with string or missing destinations. Preserve boolean aliases, tokens, defaults, multiple values, literal string options, and permissive parsing. Refs: https://github.com/nodejs/node/issues/67085 Signed-off-by: JoaoMarcos44 Assisted-by: Codex --- test/parallel/test-parse-args.mjs | 177 +++++++++++++++++++----------- 1 file changed, 115 insertions(+), 62 deletions(-) diff --git a/test/parallel/test-parse-args.mjs b/test/parallel/test-parse-args.mjs index cd1c40991e55..4b72e5c8c205 100644 --- a/test/parallel/test-parse-args.mjs +++ b/test/parallel/test-parse-args.mjs @@ -1026,86 +1026,139 @@ test('auto-detect --no-foo as negated when strict:false and allowNegative', () = process.argv = holdArgv; }); -test('explicit option starting with "no-" is not treated as negation when allowNegative is true', () => { +for (const flag of ['--no-logfile', '-L']) { + for (const args of [[flag], ['--logfile', 'out.log', flag]]) { + test(`strict mode rejects negating an explicit string target: ${args.join(' ')}`, () => { + const options = { + 'logfile': { type: 'string' }, + 'no-logfile': { type: 'boolean', short: 'L' }, + }; + assert.throws(() => { + parseArgs({ args, options, allowNegative: true }); + }, { + code: 'ERR_PARSE_ARGS_UNKNOWN_OPTION' + }); + }); + } +} + +for (const flag of ['--no-alpha', '-A']) { + test(`strict mode rejects an explicit negative option without a target: ${flag}`, () => { + const options = { 'no-alpha': { type: 'boolean', short: 'A' } }; + assert.throws(() => { + parseArgs({ args: [flag], options, allowNegative: true }); + }, { + code: 'ERR_PARSE_ARGS_UNKNOWN_OPTION' + }); + }); +} + +test('explicit negative boolean options and short aliases retain negated tokens', () => { + const args = ['--alpha', '-A', '--no-alpha']; const options = { - 'color': { type: 'boolean' }, - 'logfile': { type: 'string' }, - 'no-logfile': { type: 'boolean' }, - 'no-warnings': { type: 'boolean', short: 'W', default: false }, + 'alpha': { type: 'boolean', short: 'a' }, + 'no-alpha': { type: 'boolean', short: 'A' }, }; - const result = parseArgs({ - args: ['--no-color', '--no-logfile', '-W'], - options, - allowNegative: true, - tokens: true, - }); const expected = { - values: { - '__proto__': null, - 'color': false, - 'no-logfile': true, - 'no-warnings': true, - }, + values: { __proto__: null, alpha: false }, positionals: [], tokens: [ - { - kind: 'option', - name: 'color', - rawName: '--no-color', - index: 0, - value: undefined, - inlineValue: undefined, - }, - { - kind: 'option', - name: 'no-logfile', - rawName: '--no-logfile', - index: 1, - value: undefined, - inlineValue: undefined, - }, - { - kind: 'option', - name: 'no-warnings', - rawName: '-W', - index: 2, - value: undefined, - inlineValue: undefined, - }, + { kind: 'option', name: 'alpha', rawName: '--alpha', + index: 0, value: undefined, inlineValue: undefined }, + { kind: 'option', name: 'alpha', rawName: '-A', + index: 1, value: undefined, inlineValue: undefined }, + { kind: 'option', name: 'alpha', rawName: '--no-alpha', + index: 2, value: undefined, inlineValue: undefined }, ], }; - assert.deepStrictEqual(result, expected); + assert.deepStrictEqual(parseArgs({ args, options, allowNegative: true, tokens: true }), expected); }); -test('explicit boolean option starting with "no-" can itself be negated with "--no-no-"', () => { +test('explicit negative aliases use the target multiple setting and replace its default', () => { + const args = ['-A', '--alpha', '-A']; const options = { - 'no-warnings': { type: 'boolean', default: true }, + 'alpha': { type: 'boolean', multiple: true, default: [true] }, + 'no-alpha': { type: 'boolean', short: 'A' }, }; - const result = parseArgs({ - args: ['--no-no-warnings'], - options, - allowNegative: true, + const expected = { values: { __proto__: null, alpha: [false, true, false] }, positionals: [] }; + assert.deepStrictEqual(parseArgs({ args, options, allowNegative: true }), expected); +}); + +test('explicit negative boolean options can themselves be negated', () => { + const args = ['--no-no-alpha']; + const options = { 'no-alpha': { type: 'boolean', default: true } }; + const expected = { values: { '__proto__': null, 'no-alpha': false }, positionals: [] }; + assert.deepStrictEqual(parseArgs({ args, options, allowNegative: true }), expected); +}); + +for (const args of [['--no-logfile=out.log'], ['--no-logfile', 'out.log'], ['-L', 'out.log']]) { + test(`explicit negative string options retain their values: ${args.join(' ')}`, () => { + const options = { + 'logfile': { type: 'string' }, + 'no-logfile': { type: 'string', short: 'L' }, + }; + const expected = { values: { '__proto__': null, 'no-logfile': 'out.log' }, positionals: [] }; + assert.deepStrictEqual(parseArgs({ args, options, allowNegative: true }), expected); }); - const expected = { - values: { '__proto__': null, 'no-warnings': false }, - positionals: [], +} + +for (const flag of ['--no-logfile', '-L']) { + test(`explicit negative string options still require a value: ${flag}`, () => { + const options = { + 'logfile': { type: 'string' }, + 'no-logfile': { type: 'string', short: 'L' }, + }; + assert.throws(() => { + parseArgs({ args: [flag], options, allowNegative: true }); + }, { + code: 'ERR_PARSE_ARGS_INVALID_OPTION_VALUE' + }); + }); +} + +test('explicit negative boolean options still reject values in strict mode', () => { + const args = ['--no-alpha=value']; + const options = { + 'alpha': { type: 'boolean' }, + 'no-alpha': { type: 'boolean' }, }; - assert.deepStrictEqual(result, expected); + assert.throws(() => { + parseArgs({ args, options, allowNegative: true }); + }, { + code: 'ERR_PARSE_ARGS_INVALID_OPTION_VALUE' + }); }); -test('strict:false with allowNegative does not negate configured string option or __proto__', () => { +test('allowNegative:false preserves explicitly configured negative options', () => { + const args = ['--no-alpha', '-A']; const options = { - alpha: { type: 'string' }, + 'alpha': { type: 'string', default: 'value' }, + 'no-alpha': { type: 'boolean', short: 'A', default: false }, + }; + const expected = { values: { '__proto__': null, 'alpha': 'value', 'no-alpha': true }, positionals: [] }; + assert.deepStrictEqual(parseArgs({ args, options, allowNegative: false }), expected); +}); + +test('strict:false preserves negation of explicit string and unknown targets', () => { + const args = ['--logfile', 'out.log', '--no-logfile', '-L', '--no-warnings', '-W', '--no-other']; + const options = { + 'logfile': { type: 'string' }, + 'no-logfile': { type: 'boolean', short: 'L' }, + 'no-warnings': { type: 'boolean', short: 'W' }, }; - const result = parseArgs({ - args: ['--alpha', 'val', '--no-alpha', '--no-__proto__'], - options, - strict: false, - allowNegative: true, - }); const expected = { - values: { '__proto__': null, 'alpha': 'val', 'no-alpha': true }, + values: { __proto__: null, logfile: false, warnings: false, other: false }, positionals: [], }; - assert.deepStrictEqual(result, expected); + assert.deepStrictEqual(parseArgs({ args, options, strict: false, allowNegative: true }), expected); +}); + +test('strict:false preserves explicit values on negative boolean options', () => { + const args = ['--no-alpha=value']; + const options = { + 'alpha': { type: 'boolean' }, + 'no-alpha': { type: 'boolean' }, + }; + const expected = { values: { '__proto__': null, 'no-alpha': 'value' }, positionals: [] }; + assert.deepStrictEqual(parseArgs({ args, options, strict: false, allowNegative: true }), expected); }); From 3d3d67655323637094f98718ca34f8f6a141577c Mon Sep 17 00:00:00 2001 From: JoaoMarcos44 Date: Wed, 28 Oct 2026 16:34:23 -0300 Subject: [PATCH 3/5] util: validate parseArgs negation targets in strict mode Validate the destination of an applied negative option after checking its original declaration and argument. Reject string and undeclared destinations with ERR_PARSE_ARGS_UNKNOWN_OPTION in strict mode. Keep storeOption unchanged to preserve negative boolean aliases and permissive parsing. Fixes: https://github.com/nodejs/node/issues/67085 Signed-off-by: JoaoMarcos44 Assisted-by: Codex --- lib/internal/util/parse_args/parse_args.js | 31 +++++++++++----------- 1 file changed, 15 insertions(+), 16 deletions(-) diff --git a/lib/internal/util/parse_args/parse_args.js b/lib/internal/util/parse_args/parse_args.js index 459bbcd160e6..bd3fb5c966a9 100644 --- a/lib/internal/util/parse_args/parse_args.js +++ b/lib/internal/util/parse_args/parse_args.js @@ -114,6 +114,14 @@ function checkOptionUsage(config, token) { if (type === 'boolean' && token.value != null) { throw new ERR_PARSE_ARGS_INVALID_OPTION_VALUE(`Option '${shortAndLong}' does not take an argument`); } + + if (config.allowNegative && StringPrototypeStartsWith(token.name, 'no-') && token.value === undefined) { + const targetName = StringPrototypeSlice(token.name, 3); + if (optionsGetOwn(config.options, targetName, 'type') !== 'boolean') { + throw new ERR_PARSE_ARGS_UNKNOWN_OPTION( + token.rawName, config.allowPositionals); + } + } } @@ -127,26 +135,17 @@ function checkOptionUsage(config, token) { function storeOption(token, options, values, allowNegative) { let longOption = token.name; let optionValue = token.value; - - if ( - allowNegative && - !ObjectHasOwn(options, longOption) && - StringPrototypeStartsWith(longOption, 'no-') && - optionValue === undefined - ) { - // Boolean option negation: --no-foo - const positiveOption = StringPrototypeSlice(longOption, 3); - if (optionsGetOwn(options, positiveOption, 'type') !== 'string') { - longOption = positiveOption; - token.name = longOption; - optionValue = false; - } - } - if (longOption === '__proto__') { return; // No. Just no. } + if (allowNegative && StringPrototypeStartsWith(longOption, 'no-') && optionValue === undefined) { + // Boolean option negation: --no-foo + longOption = StringPrototypeSlice(longOption, 3); + token.name = longOption; + optionValue = false; + } + // We store based on the option value rather than option type, // preserving the users intent for author to deal with. const newValue = optionValue ?? true; From cdda2e3b6801b03abf6ba8ce2c61286815a05690 Mon Sep 17 00:00:00 2001 From: JoaoMarcos44 Date: Sat, 31 Oct 2026 13:59:56 -0300 Subject: [PATCH 4/5] util: validate conflicting parseArgs configuration Reject explicit boolean no-* options that target a string option during strict configuration validation, before arguments are tokenized. Keep missing negation targets as usage errors, preserving valid double negation, boolean aliases, and permissive parsing. Check own option names including non-enumerable declarations without reading a negative getter when its own target is absent. Add regression coverage and document the strict configuration contract. Refs: https://github.com/nodejs/node/pull/67086 Assisted-by: Codex Signed-off-by: JoaoMarcos44 --- doc/api/util.md | 4 + lib/internal/util/parse_args/parse_args.js | 26 ++- test/parallel/test-parse-args.mjs | 174 ++++++++++++++++++++- 3 files changed, 201 insertions(+), 3 deletions(-) diff --git a/doc/api/util.md b/doc/api/util.md index c30831019d0e..c5cf45ae25bb 100644 --- a/doc/api/util.md +++ b/doc/api/util.md @@ -2361,6 +2361,10 @@ changes: **Default:** `false` if `strict` is `true`, otherwise `true`. * `allowNegative` {boolean} If `true`, allows explicitly setting boolean options to `false` by prefixing the option name with `--no-`. + In strict mode, the option being negated must be a declared boolean option, + including when a `no-*` option is explicitly configured with a `short` alias. + An explicitly configured boolean `no-foo` cannot coexist with a string + `foo` option in strict mode, even if neither option appears in the arguments. **Default:** `false`. * `tokens` {boolean} Return the parsed tokens. This is useful for extending the built-in behavior, from adding additional checks through to reprocessing diff --git a/lib/internal/util/parse_args/parse_args.js b/lib/internal/util/parse_args/parse_args.js index bd3fb5c966a9..5b71129bf40f 100644 --- a/lib/internal/util/parse_args/parse_args.js +++ b/lib/internal/util/parse_args/parse_args.js @@ -1,6 +1,7 @@ 'use strict'; const { + ArrayIsArray, ArrayPrototypeForEach, ArrayPrototypeMap, ArrayPrototypePush, @@ -9,6 +10,7 @@ const { ArrayPrototypeSlice, ArrayPrototypeUnshiftApply, ObjectEntries, + ObjectGetOwnPropertyNames, ObjectPrototypeHasOwnProperty: ObjectHasOwn, StringPrototypeCharAt, StringPrototypeIndexOf, @@ -115,7 +117,9 @@ function checkOptionUsage(config, token) { throw new ERR_PARSE_ARGS_INVALID_OPTION_VALUE(`Option '${shortAndLong}' does not take an argument`); } - if (config.allowNegative && StringPrototypeStartsWith(token.name, 'no-') && token.value === undefined) { + // Implicit negative options already had their target checked above. + if (tokenName === token.name && config.allowNegative && + StringPrototypeStartsWith(token.name, 'no-') && token.value === undefined) { const targetName = StringPrototypeSlice(token.name, 3); if (optionsGetOwn(config.options, targetName, 'type') !== 'boolean') { throw new ERR_PARSE_ARGS_UNKNOWN_OPTION( @@ -368,6 +372,26 @@ const parseArgs = (config = kEmptyObject) => { }, ); + if (strict && allowNegative) { + ArrayPrototypeForEach(ObjectGetOwnPropertyNames(options), (longOption) => { + if (!StringPrototypeStartsWith(longOption, 'no-')) return; + const targetName = StringPrototypeSlice(longOption, 3); + if (!ObjectHasOwn(options, targetName)) return; + const optionConfig = objectGetOwn(options, longOption); + if (optionConfig === null || typeof optionConfig !== 'object' || ArrayIsArray(optionConfig)) return; + if (objectGetOwn(optionConfig, 'type') !== 'boolean') return; + + const target = objectGetOwn(options, targetName); + if (target !== null && typeof target === 'object' && !ArrayIsArray(target) && + objectGetOwn(target, 'type') === 'string') { + throw new ERR_INVALID_ARG_VALUE( + `options.${longOption}`, optionConfig, + `conflicts with string option ${targetName} when allowNegative is true`, + ); + } + }); + } + // Phase 1: identify tokens const tokens = argsToTokens(args, options); diff --git a/test/parallel/test-parse-args.mjs b/test/parallel/test-parse-args.mjs index 4b72e5c8c205..5a3b020355aa 100644 --- a/test/parallel/test-parse-args.mjs +++ b/test/parallel/test-parse-args.mjs @@ -1028,7 +1028,7 @@ test('auto-detect --no-foo as negated when strict:false and allowNegative', () = for (const flag of ['--no-logfile', '-L']) { for (const args of [[flag], ['--logfile', 'out.log', flag]]) { - test(`strict mode rejects negating an explicit string target: ${args.join(' ')}`, () => { + test(`strict mode rejects a conflicting negative boolean option: ${args.join(' ')}`, () => { const options = { 'logfile': { type: 'string' }, 'no-logfile': { type: 'boolean', short: 'L' }, @@ -1036,12 +1036,90 @@ for (const flag of ['--no-logfile', '-L']) { assert.throws(() => { parseArgs({ args, options, allowNegative: true }); }, { - code: 'ERR_PARSE_ARGS_UNKNOWN_OPTION' + code: 'ERR_INVALID_ARG_VALUE' }); }); } } +for (const reverse of [false, true]) { + for (const args of [[], ['--logfile', 'out.log'], ['-l', 'out.log']]) { + test(`strict mode rejects a conflicting configuration before parsing: ${reverse} ${args.join(' ')}`, () => { + const entries = [ + ['logfile', { type: 'string', short: 'l' }], + ['no-logfile', { type: 'boolean', short: 'L' }], + ]; + const options = Object.fromEntries(reverse ? entries.reverse() : entries); + assert.throws(() => { + parseArgs({ args, options, allowNegative: true }); + }, { + code: 'ERR_INVALID_ARG_VALUE' + }); + }); + } +} + +for (const nonEnumerable of ['logfile', 'no-logfile']) { + for (const args of [[], ['--no-logfile']]) { + test(`strict mode detects a non-enumerable conflict: ${nonEnumerable} ${args.join(' ')}`, () => { + const options = { + 'logfile': { type: 'string' }, + 'no-logfile': { type: 'boolean' }, + }; + Object.defineProperty(options, nonEnumerable, { enumerable: false }); + assert.throws(() => { + parseArgs({ args, options, allowNegative: true }); + }, { + code: 'ERR_INVALID_ARG_VALUE' + }); + }); + } +} + +for (const [description, prototype] of [ + ['absent', null], + ['inherited', { foo: { type: 'string' } }], +]) { + test(`strict mode does not read a non-enumerable negative getter without an own target: ${description}`, () => { + const options = { __proto__: prototype }; + let reads = 0; + Object.defineProperty(options, 'no-foo', { + get() { + reads++; + throw new Error('negative getter was read'); + }, + }); + const expected = { values: { __proto__: null }, positionals: [] }; + assert.deepStrictEqual(parseArgs({ args: [], options, allowNegative: true }), expected); + assert.strictEqual(reads, 0); + }); +} + +for (const [description, option, code, message] of [ + ['null', null, 'ERR_INVALID_ARG_TYPE', /options\.invalid/], + ['array', [], 'ERR_INVALID_ARG_TYPE', /options\.invalid/], + ['type', { type: 'number' }, 'ERR_INVALID_ARG_TYPE', /options\.invalid\.type/], + ['short', { type: 'boolean', short: 'xy' }, 'ERR_INVALID_ARG_VALUE', /options\.invalid\.short/], + ['default', { type: 'boolean', default: 'invalid' }, 'ERR_INVALID_ARG_TYPE', /options\.invalid\.default/], +]) { + test(`invalid ${description} is reported before a configuration conflict`, () => { + const options = { + 'logfile': { type: 'string' }, + 'no-logfile': { type: 'boolean' }, + 'invalid': option, + }; + assert.throws(() => { + parseArgs({ args: [], options, allowNegative: true }); + }, { code, message }); + }); +} + +test('strict mode accepts an explicit negative boolean without a target when unused', () => { + const options = { 'no-alpha': { type: 'boolean', short: 'A' } }; + const expected = { values: { __proto__: null }, positionals: [] }; + assert.deepStrictEqual(parseArgs({ args: [], options, allowNegative: true }), expected); +}); + for (const flag of ['--no-alpha', '-A']) { test(`strict mode rejects an explicit negative option without a target: ${flag}`, () => { const options = { 'no-alpha': { type: 'boolean', short: 'A' } }; @@ -1153,6 +1231,15 @@ test('strict:false preserves negation of explicit string and unknown targets', ( assert.deepStrictEqual(parseArgs({ args, options, strict: false, allowNegative: true }), expected); }); +test('strict:false accepts a conflicting configuration with no arguments', () => { + const options = { + 'logfile': { type: 'string' }, + 'no-logfile': { type: 'boolean' }, + }; + const expected = { values: { __proto__: null }, positionals: [] }; + assert.deepStrictEqual(parseArgs({ args: [], options, strict: false, allowNegative: true }), expected); +}); + test('strict:false preserves explicit values on negative boolean options', () => { const args = ['--no-alpha=value']; const options = { @@ -1162,3 +1249,86 @@ test('strict:false preserves explicit values on negative boolean options', () => const expected = { values: { '__proto__': null, 'no-alpha': 'value' }, positionals: [] }; assert.deepStrictEqual(parseArgs({ args, options, strict: false, allowNegative: true }), expected); }); + +for (const flag of ['--no-no-alpha', '-N']) { + test(`explicit double negation uses the immediate boolean target: ${flag}`, () => { + const options = { + 'alpha': { type: 'boolean' }, + 'no-alpha': { type: 'boolean' }, + 'no-no-alpha': { type: 'boolean', short: 'N' }, + }; + const expected = { values: { '__proto__': null, 'no-alpha': false }, positionals: [] }; + assert.deepStrictEqual(parseArgs({ args: [flag], options, allowNegative: true }), expected); + }); +} + +for (const flag of ['--no-no-alpha', '-N']) { + test(`double negation rejects an independently conflicting configuration: ${flag}`, () => { + const options = { + 'alpha': { type: 'string' }, + 'no-alpha': { type: 'boolean' }, + 'no-no-alpha': { type: 'boolean', short: 'N' }, + }; + assert.throws(() => { + parseArgs({ args: [flag], options, allowNegative: true }); + }, { + code: 'ERR_INVALID_ARG_VALUE' + }); + }); + + test(`double negation accepts a boolean target without an alpha option: ${flag}`, () => { + const options = { + 'no-alpha': { type: 'boolean' }, + 'no-no-alpha': { type: 'boolean', short: 'N' }, + }; + const expected = { values: { '__proto__': null, 'no-alpha': false }, positionals: [] }; + assert.deepStrictEqual(parseArgs({ args: [flag], options, allowNegative: true }), expected); + }); +} + +test('inherited options do not cause a configuration conflict', () => { + const options = { + '__proto__': { 'no-alpha': { type: 'boolean' } }, + 'alpha': { type: 'string' }, + }; + const expected = { values: { __proto__: null }, positionals: [] }; + assert.deepStrictEqual(parseArgs({ args: [], options, allowNegative: true }), expected); +}); + +test('an inherited string target does not cause a configuration conflict', () => { + const options = { + '__proto__': { alpha: { type: 'string' } }, + 'no-alpha': { type: 'boolean' }, + }; + const expected = { values: { __proto__: null }, positionals: [] }; + assert.deepStrictEqual(parseArgs({ args: [], options, allowNegative: true }), expected); +}); + +for (const type of ['boolean', 'string']) { + const flags = type === 'boolean' ? ['--no-alpha'] : ['--no-alpha', '-A']; + for (const flag of flags) { + test(`explicit negative ${type} retains value errors without a target: ${flag}`, () => { + const options = { 'no-alpha': { type, short: 'A' } }; + const args = type === 'boolean' ? [`${flag}=value`] : [flag]; + assert.throws(() => { + parseArgs({ args, options, allowNegative: true }); + }, { + code: 'ERR_PARSE_ARGS_INVALID_OPTION_VALUE' + }); + }); + } +} + +for (const flag of ['--no-alpha', '-A']) { + test(`strict negation rejects an inherited boolean target: ${flag}`, () => { + const options = { + '__proto__': { alpha: { type: 'boolean' } }, + 'no-alpha': { type: 'boolean', short: 'A' }, + }; + assert.throws(() => { + parseArgs({ args: [flag], options, allowNegative: true }); + }, { + code: 'ERR_PARSE_ARGS_UNKNOWN_OPTION' + }); + }); +} From 8c8fc771e370fb00a9717445dd8ce4a103312544 Mon Sep 17 00:00:00 2001 From: JoaoMarcos44 Date: Sat, 31 Oct 2026 14:44:19 -0300 Subject: [PATCH 5/5] util: simplify parseArgs configuration validation Check conflicting negative declarations in the existing validation loop and reuse the validated option type. Remove the additional traversal of own property names while retaining the negation usage check. Follow the existing loop's enumeration scope and error ordering. Keep guards for invalid target configurations, update regression coverage, and document the enumerable declaration requirement. Refs: https://github.com/nodejs/node/pull/67086 Assisted-by: Codex Signed-off-by: JoaoMarcos44 --- doc/api/util.md | 2 +- lib/internal/util/parse_args/parse_args.js | 34 +++---- test/parallel/test-parse-args.mjs | 108 ++++++++++++++++++--- 3 files changed, 106 insertions(+), 38 deletions(-) diff --git a/doc/api/util.md b/doc/api/util.md index c5cf45ae25bb..b8f83c648c09 100644 --- a/doc/api/util.md +++ b/doc/api/util.md @@ -2363,7 +2363,7 @@ changes: options to `false` by prefixing the option name with `--no-`. In strict mode, the option being negated must be a declared boolean option, including when a `no-*` option is explicitly configured with a `short` alias. - An explicitly configured boolean `no-foo` cannot coexist with a string + An enumerable own boolean `no-foo` option cannot coexist with a string `foo` option in strict mode, even if neither option appears in the arguments. **Default:** `false`. * `tokens` {boolean} Return the parsed tokens. This is useful for extending diff --git a/lib/internal/util/parse_args/parse_args.js b/lib/internal/util/parse_args/parse_args.js index 5b71129bf40f..8e8c3c184f6b 100644 --- a/lib/internal/util/parse_args/parse_args.js +++ b/lib/internal/util/parse_args/parse_args.js @@ -10,7 +10,6 @@ const { ArrayPrototypeSlice, ArrayPrototypeUnshiftApply, ObjectEntries, - ObjectGetOwnPropertyNames, ObjectPrototypeHasOwnProperty: ObjectHasOwn, StringPrototypeCharAt, StringPrototypeIndexOf, @@ -369,28 +368,21 @@ const parseArgs = (config = kEmptyObject) => { } validator(defaultValue, `options.${longOption}.default`); } - }, - ); - if (strict && allowNegative) { - ArrayPrototypeForEach(ObjectGetOwnPropertyNames(options), (longOption) => { - if (!StringPrototypeStartsWith(longOption, 'no-')) return; - const targetName = StringPrototypeSlice(longOption, 3); - if (!ObjectHasOwn(options, targetName)) return; - const optionConfig = objectGetOwn(options, longOption); - if (optionConfig === null || typeof optionConfig !== 'object' || ArrayIsArray(optionConfig)) return; - if (objectGetOwn(optionConfig, 'type') !== 'boolean') return; - - const target = objectGetOwn(options, targetName); - if (target !== null && typeof target === 'object' && !ArrayIsArray(target) && - objectGetOwn(target, 'type') === 'string') { - throw new ERR_INVALID_ARG_VALUE( - `options.${longOption}`, optionConfig, - `conflicts with string option ${targetName} when allowNegative is true`, - ); + if (strict && allowNegative && optionType === 'boolean' && + StringPrototypeStartsWith(longOption, 'no-')) { + const targetName = StringPrototypeSlice(longOption, 3); + const target = objectGetOwn(options, targetName); + if (target !== null && typeof target === 'object' && !ArrayIsArray(target) && + objectGetOwn(target, 'type') === 'string') { + throw new ERR_INVALID_ARG_VALUE( + `options.${longOption}`, optionConfig, + `conflicts with string option ${targetName} when allowNegative is true`, + ); + } } - }); - } + }, + ); // Phase 1: identify tokens const tokens = argsToTokens(args, options); diff --git a/test/parallel/test-parse-args.mjs b/test/parallel/test-parse-args.mjs index 5a3b020355aa..35c13e3a4498 100644 --- a/test/parallel/test-parse-args.mjs +++ b/test/parallel/test-parse-args.mjs @@ -1059,23 +1059,42 @@ for (const reverse of [false, true]) { } } -for (const nonEnumerable of ['logfile', 'no-logfile']) { - for (const args of [[], ['--no-logfile']]) { - test(`strict mode detects a non-enumerable conflict: ${nonEnumerable} ${args.join(' ')}`, () => { - const options = { - 'logfile': { type: 'string' }, - 'no-logfile': { type: 'boolean' }, - }; - Object.defineProperty(options, nonEnumerable, { enumerable: false }); - assert.throws(() => { - parseArgs({ args, options, allowNegative: true }); - }, { - code: 'ERR_INVALID_ARG_VALUE' - }); +for (const args of [[], ['--no-logfile']]) { + test(`strict mode detects a conflict with a non-enumerable target: ${args.join(' ')}`, () => { + const options = { + 'logfile': { type: 'string' }, + 'no-logfile': { type: 'boolean' }, + }; + Object.defineProperty(options, 'logfile', { enumerable: false }); + assert.throws(() => { + parseArgs({ args, options, allowNegative: true }); + }, { + code: 'ERR_INVALID_ARG_VALUE' }); - } + }); } +test('strict mode accepts an unused non-enumerable conflicting negative option', () => { + const options = { + 'logfile': { type: 'string' }, + 'no-logfile': { type: 'boolean' }, + }; + Object.defineProperty(options, 'no-logfile', { enumerable: false }); + const expected = { values: { __proto__: null }, positionals: [] }; + assert.deepStrictEqual(parseArgs({ args: [], options, allowNegative: true }), expected); +}); + +test('strict mode rejects a used non-enumerable conflicting negative option', () => { + const options = { + 'logfile': { type: 'string' }, + 'no-logfile': { type: 'boolean' }, + }; + Object.defineProperty(options, 'no-logfile', { enumerable: false }); + assert.throws(() => { + parseArgs({ args: ['--no-logfile'], options, allowNegative: true }); + }, { code: 'ERR_PARSE_ARGS_UNKNOWN_OPTION' }); +}); + for (const [description, prototype] of [ ['absent', null], ['inherited', { foo: { type: 'string' } }], @@ -1102,11 +1121,11 @@ for (const [description, option, code, message] of [ ['short', { type: 'boolean', short: 'xy' }, 'ERR_INVALID_ARG_VALUE', /options\.invalid\.short/], ['default', { type: 'boolean', default: 'invalid' }, 'ERR_INVALID_ARG_TYPE', /options\.invalid\.default/], ]) { - test(`invalid ${description} is reported before a configuration conflict`, () => { + test(`invalid ${description} is reported before a later configuration conflict`, () => { const options = { + 'invalid': option, 'logfile': { type: 'string' }, 'no-logfile': { type: 'boolean' }, - 'invalid': option, }; assert.throws(() => { parseArgs({ args: [], options, allowNegative: true }); @@ -1114,6 +1133,63 @@ for (const [description, option, code, message] of [ }); } +test('configuration conflict is reported before a later invalid option', () => { + const options = { + 'logfile': { type: 'string' }, + 'no-logfile': { type: 'boolean' }, + 'invalid': null, + }; + assert.throws(() => { + parseArgs({ args: [], options, allowNegative: true }); + }, { code: 'ERR_INVALID_ARG_VALUE', message: /options\.no-logfile.*conflicts with string option logfile/ }); +}); + +for (const [description, target] of [ + ['null', null], + ['undefined', undefined], + ['array', Object.assign([], { type: 'string' })], +]) { + for (const reverse of [false, true]) { + test(`invalid ${description} target is reported in either order: ${reverse}`, () => { + const entries = [ + ['alpha', target], + ['no-alpha', { type: 'boolean' }], + ]; + const options = Object.fromEntries(reverse ? entries.reverse() : entries); + assert.throws(() => { + parseArgs({ args: [], options, allowNegative: true }); + }, { code: 'ERR_INVALID_ARG_TYPE', message: /options\.alpha/ }); + }); + } +} + +test('a negative option is validated before checking its configuration conflict', () => { + const options = { + 'no-alpha': { type: 'boolean', default: 'invalid' }, + 'alpha': { type: 'string' }, + }; + assert.throws(() => { + parseArgs({ args: [], options, allowNegative: true }); + }, { code: 'ERR_INVALID_ARG_TYPE', message: /options\.no-alpha\.default/ }); +}); + +test('negative option type getter is read once during configuration validation', () => { + let reads = 0; + const options = { + 'alpha': { type: 'boolean' }, + 'no-alpha': { + get type() { + reads++; + if (reads > 1) throw new Error('negative type was read again'); + return 'boolean'; + }, + }, + }; + const expected = { values: { __proto__: null }, positionals: [] }; + assert.deepStrictEqual(parseArgs({ args: [], options, allowNegative: true }), expected); + assert.strictEqual(reads, 1); +}); + test('strict mode accepts an explicit negative boolean without a target when unused', () => { const options = { 'no-alpha': { type: 'boolean', short: 'A' } }; const expected = { values: { __proto__: null }, positionals: [] };