From ce639629ddae13932c9ca31da376f29fae4b37ab Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Wed, 17 Aug 2022 23:57:09 +0200 Subject: [PATCH 1/2] lib: use safe `Promise` alternatives when available PR-URL: https://github.com/nodejs/node/pull/43476 Reviewed-By: Moshe Atlow --- lib/internal/debugger/inspect_repl.js | 5 ++--- lib/internal/fs/cp/cp.js | 10 ++++------ lib/internal/modules/esm/loader.js | 5 ++--- lib/internal/webstreams/adapters.js | 23 ++++++++++------------ lib/internal/webstreams/readablestream.js | 4 ++-- lib/internal/webstreams/transformstream.js | 17 +++++++--------- 6 files changed, 27 insertions(+), 37 deletions(-) diff --git a/lib/internal/debugger/inspect_repl.js b/lib/internal/debugger/inspect_repl.js index 1d1cd6ef276bbb..965aa64b0fbcc6 100644 --- a/lib/internal/debugger/inspect_repl.js +++ b/lib/internal/debugger/inspect_repl.js @@ -23,7 +23,6 @@ const { ObjectKeys, ObjectValues, Promise, - PromisePrototypeCatch, PromisePrototypeThen, PromiseResolve, ReflectGetOwnPropertyDescriptor, @@ -653,8 +652,8 @@ function createRepl(inspector) { } const inspectValue = (expr) => - PromisePrototypeCatch(evalInCurrentContext(expr), - (error) => `<${error.message}>`); + PromisePrototypeThen(evalInCurrentContext(expr), undefined, + (error) => `<${error.message}>`); const lastIndex = watchedExpressions.length - 1; const values = await SafePromiseAll(watchedExpressions, inspectValue); diff --git a/lib/internal/fs/cp/cp.js b/lib/internal/fs/cp/cp.js index bcbc8aa3279a50..e0b7a74b1c799a 100644 --- a/lib/internal/fs/cp/cp.js +++ b/lib/internal/fs/cp/cp.js @@ -6,11 +6,9 @@ const { ArrayPrototypeEvery, ArrayPrototypeFilter, Boolean, - PromiseAll, - PromisePrototypeCatch, PromisePrototypeThen, PromiseReject, - SafeArrayIterator, + SafePromiseAll, StringPrototypeSplit, } = primordials; const { @@ -128,13 +126,13 @@ function getStats(src, dest, opts) { const statFunc = opts.dereference ? (file) => stat(file, { bigint: true }) : (file) => lstat(file, { bigint: true }); - return PromiseAll(new SafeArrayIterator([ + return SafePromiseAll([ statFunc(src), - PromisePrototypeCatch(statFunc(dest), (err) => { + PromisePrototypeThen(statFunc(dest), undefined, (err) => { if (err.code === 'ENOENT') return null; throw err; }), - ])); + ]); } async function checkParentDir(destStat, src, dest, opts) { diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index 79e9b2446c6bbf..cc7f64a9980d86 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -13,9 +13,8 @@ const { ObjectCreate, ObjectDefineProperty, ObjectSetPrototypeOf, - PromiseAll, RegExpPrototypeExec, - SafeArrayIterator, + SafePromiseAll, SafeWeakMap, StringPrototypeSlice, StringPrototypeToUpperCase, @@ -525,7 +524,7 @@ class ESMLoader { .then(({ module }) => module.getNamespace()); } - const namespaces = await PromiseAll(new SafeArrayIterator(jobs)); + const namespaces = await SafePromiseAll(jobs); if (!wasArr) { return namespaces[0]; } // We can skip the pairing below diff --git a/lib/internal/webstreams/adapters.js b/lib/internal/webstreams/adapters.js index 402a93342795b2..0c5cbc5ea1db9d 100644 --- a/lib/internal/webstreams/adapters.js +++ b/lib/internal/webstreams/adapters.js @@ -1,11 +1,10 @@ 'use strict'; const { - ArrayPrototypeMap, - PromiseAll, PromisePrototypeThen, - PromisePrototypeFinally, PromiseResolve, + SafePromiseAll, + SafePromisePrototypeFinally, Uint8Array, } = primordials; @@ -165,7 +164,7 @@ function newWritableStreamFromStreamWritable(streamWritable) { async write(chunk) { if (streamWritable.writableNeedDrain || !streamWritable.write(chunk)) { backpressurePromise = createDeferredPromise(); - return PromisePrototypeFinally( + return SafePromisePrototypeFinally( backpressurePromise.promise, () => { backpressurePromise = undefined; }); @@ -246,10 +245,9 @@ function newStreamWritableFromWritableStream(writableStream, options = kEmptyObj writer.ready, () => { return PromisePrototypeThen( - PromiseAll( - ArrayPrototypeMap( - chunks, - (data) => writer.write(data.chunk))), + SafePromiseAll( + chunks, + (data) => writer.write(data.chunk)), done, done); }, @@ -668,10 +666,9 @@ function newStreamDuplexFromReadableWritablePair(pair = kEmptyObject, options = writer.ready, () => { return PromisePrototypeThen( - PromiseAll( - ArrayPrototypeMap( - chunks, - (data) => writer.write(data.chunk))), + SafePromiseAll( + chunks, + (data) => writer.write(data.chunk)), done, done); }, @@ -767,7 +764,7 @@ function newStreamDuplexFromReadableWritablePair(pair = kEmptyObject, options = if (!writableClosed || !readableClosed) { PromisePrototypeThen( - PromiseAll([ + SafePromiseAll([ closeWriter(), closeReader(), ]), diff --git a/lib/internal/webstreams/readablestream.js b/lib/internal/webstreams/readablestream.js index c3024a350aa84e..3d8980b24e359b 100644 --- a/lib/internal/webstreams/readablestream.js +++ b/lib/internal/webstreams/readablestream.js @@ -19,8 +19,8 @@ const { PromisePrototypeThen, PromiseResolve, PromiseReject, - PromiseAll, ReflectConstruct, + SafePromiseAll, Symbol, SymbolAsyncIterator, SymbolToStringTag, @@ -1334,7 +1334,7 @@ function readableStreamPipeTo( } shutdownWithAnAction( - async () => PromiseAll(actions.map((action) => action())), + () => SafePromiseAll(actions, (action) => action()), true, error); } diff --git a/lib/internal/webstreams/transformstream.js b/lib/internal/webstreams/transformstream.js index 457c9eb8fb338a..b7366fb1ba2576 100644 --- a/lib/internal/webstreams/transformstream.js +++ b/lib/internal/webstreams/transformstream.js @@ -4,7 +4,6 @@ const { FunctionPrototypeBind, FunctionPrototypeCall, ObjectDefineProperties, - PromisePrototypeCatch, PromisePrototypeThen, PromiseResolve, ReflectConstruct, @@ -496,19 +495,17 @@ function transformStreamDefaultControllerError(controller, error) { transformStreamError(controller[kState].stream, error); } -function transformStreamDefaultControllerPerformTransform(controller, chunk) { - const transformPromise = - ensureIsPromise( +async function transformStreamDefaultControllerPerformTransform(controller, chunk) { + try { + return await ensureIsPromise( controller[kState].transformAlgorithm, controller, chunk, controller); - return PromisePrototypeCatch( - transformPromise, - (error) => { - transformStreamError(controller[kState].stream, error); - throw error; - }); + } catch (error) { + transformStreamError(controller[kState].stream, error); + throw error; + } } function transformStreamDefaultControllerTerminate(controller) { From 14000517b49371cfd3bc012898d730179e6c6d97 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Fri, 17 Jun 2022 19:12:42 +0200 Subject: [PATCH 2/2] tools: refactor `avoid-prototype-pollution` lint rule The lint rule was not catching all occurences of unsafe primordials use, and was too strict on some methods. PR-URL: https://github.com/nodejs/node/pull/43476 Reviewed-By: Moshe Atlow --- lib/internal/net.js | 6 ++ .../test-eslint-avoid-prototype-pollution.js | 23 +++++++ .../eslint-rules/avoid-prototype-pollution.js | 65 ++++++++++++------- 3 files changed, 71 insertions(+), 23 deletions(-) diff --git a/lib/internal/net.js b/lib/internal/net.js index 8ae3170228dc32..625377acd57caa 100644 --- a/lib/internal/net.js +++ b/lib/internal/net.js @@ -29,10 +29,16 @@ const IPv6Reg = new RegExp('^(' + ')(%[0-9a-zA-Z-.:]{1,})?$'); function isIPv4(s) { + // TODO(aduh95): Replace RegExpPrototypeTest with RegExpPrototypeExec when it + // no longer creates a perf regression in the dns benchmark. + // eslint-disable-next-line node-core/avoid-prototype-pollution return RegExpPrototypeTest(IPv4Reg, s); } function isIPv6(s) { + // TODO(aduh95): Replace RegExpPrototypeTest with RegExpPrototypeExec when it + // no longer creates a perf regression in the dns benchmark. + // eslint-disable-next-line node-core/avoid-prototype-pollution return RegExpPrototypeTest(IPv6Reg, s); } diff --git a/test/parallel/test-eslint-avoid-prototype-pollution.js b/test/parallel/test-eslint-avoid-prototype-pollution.js index f10d6ea973b347..c30928f56ce91a 100644 --- a/test/parallel/test-eslint-avoid-prototype-pollution.js +++ b/test/parallel/test-eslint-avoid-prototype-pollution.js @@ -45,6 +45,9 @@ new RuleTester({ 'ReflectDefineProperty({}, "key", { "__proto__": null })', 'ObjectDefineProperty({}, "key", { \'__proto__\': null })', 'ReflectDefineProperty({}, "key", { \'__proto__\': null })', + 'StringPrototypeReplace("some string", "some string", "some replacement")', + 'StringPrototypeReplaceAll("some string", "some string", "some replacement")', + 'StringPrototypeSplit("some string", "some string")', 'new Proxy({}, otherObject)', 'new Proxy({}, someFactory())', 'new Proxy({}, { __proto__: null })', @@ -167,18 +170,38 @@ new RuleTester({ code: 'StringPrototypeMatch("some string", /some regex/)', errors: [{ message: /looks up the Symbol\.match property/ }], }, + { + code: 'let v = StringPrototypeMatch("some string", /some regex/)', + errors: [{ message: /looks up the Symbol\.match property/ }], + }, + { + code: 'let v = StringPrototypeMatch("some string", new RegExp("some regex"))', + errors: [{ message: /looks up the Symbol\.match property/ }], + }, { code: 'StringPrototypeMatchAll("some string", /some regex/)', errors: [{ message: /looks up the Symbol\.matchAll property/ }], }, + { + code: 'let v = StringPrototypeMatchAll("some string", new RegExp("some regex"))', + errors: [{ message: /looks up the Symbol\.matchAll property/ }], + }, { code: 'StringPrototypeReplace("some string", /some regex/, "some replacement")', errors: [{ message: /looks up the Symbol\.replace property/ }], }, + { + code: 'StringPrototypeReplace("some string", new RegExp("some regex"), "some replacement")', + errors: [{ message: /looks up the Symbol\.replace property/ }], + }, { code: 'StringPrototypeReplaceAll("some string", /some regex/, "some replacement")', errors: [{ message: /looks up the Symbol\.replace property/ }], }, + { + code: 'StringPrototypeReplaceAll("some string", new RegExp("some regex"), "some replacement")', + errors: [{ message: /looks up the Symbol\.replace property/ }], + }, { code: 'StringPrototypeSearch("some string", /some regex/)', errors: [{ message: /looks up the Symbol\.search property/ }], diff --git a/tools/eslint-rules/avoid-prototype-pollution.js b/tools/eslint-rules/avoid-prototype-pollution.js index d59b62f95028cc..d26a410b1d9f67 100644 --- a/tools/eslint-rules/avoid-prototype-pollution.js +++ b/tools/eslint-rules/avoid-prototype-pollution.js @@ -1,5 +1,7 @@ 'use strict'; +const CallExpression = (fnName) => `CallExpression[callee.name=${fnName}]`; + function checkProperties(context, node) { if ( node.type === 'CallExpression' && @@ -64,8 +66,10 @@ function checkPropertyDescriptor(context, node) { } function createUnsafeStringMethodReport(context, name, lookedUpProperty) { + const lastDotPosition = '$String.prototype.'.length; + const unsafePrimordialName = `StringPrototype${name.charAt(lastDotPosition).toUpperCase()}${name.slice(lastDotPosition + 1, -1)}`; return { - [`${CallExpression}[expression.callee.name=${JSON.stringify(name)}]`](node) { + [CallExpression(unsafePrimordialName)](node) { context.report({ node, message: `${name} looks up the ${lookedUpProperty} property on the first argument`, @@ -74,31 +78,46 @@ function createUnsafeStringMethodReport(context, name, lookedUpProperty) { }; } -const CallExpression = 'ExpressionStatement[expression.type="CallExpression"]'; +function createUnsafeStringMethodOnRegexReport(context, name, lookedUpProperty) { + const dotPosition = 'Symbol.'.length; + const safePrimordialName = `RegExpPrototypeSymbol${lookedUpProperty.charAt(dotPosition).toUpperCase()}${lookedUpProperty.slice(dotPosition + 1)}`; + const lastDotPosition = '$String.prototype.'.length; + const unsafePrimordialName = `StringPrototype${name.charAt(lastDotPosition).toUpperCase()}${name.slice(lastDotPosition + 1, -1)}`; + return { + [[ + `${CallExpression(unsafePrimordialName)}[arguments.1.type=Literal][arguments.1.regex]`, + `${CallExpression(unsafePrimordialName)}[arguments.1.type=NewExpression][arguments.1.callee.name=RegExp]`, + ].join(',')](node) { + context.report({ + node, + message: `${name} looks up the ${lookedUpProperty} property of the passed regex, use ${safePrimordialName} directly`, + }); + } + }; +} + module.exports = { meta: { hasSuggestions: true }, create(context) { return { - [`${CallExpression}[expression.callee.name=${/^(Object|Reflect)DefinePropert(ies|y)$/}]`]( - node - ) { - switch (node.expression.callee.name) { + [CallExpression(/^(Object|Reflect)DefinePropert(ies|y)$/)](node) { + switch (node.callee.name) { case 'ObjectDefineProperties': - checkProperties(context, node.expression.arguments[1]); + checkProperties(context, node.arguments[1]); break; case 'ReflectDefineProperty': case 'ObjectDefineProperty': - checkPropertyDescriptor(context, node.expression.arguments[2]); + checkPropertyDescriptor(context, node.arguments[2]); break; default: throw new Error('Unreachable'); } }, - [`${CallExpression}[expression.callee.name="ObjectCreate"][expression.arguments.length=2]`](node) { - checkProperties(context, node.expression.arguments[1]); + [`${CallExpression('ObjectCreate')}[arguments.length=2]`](node) { + checkProperties(context, node.arguments[1]); }, - [`${CallExpression}[expression.callee.name="RegExpPrototypeTest"]`](node) { + [CallExpression('RegExpPrototypeTest')](node) { context.report({ node, message: '%RegExp.prototype.test% looks up the "exec" property of `this` value', @@ -116,18 +135,18 @@ module.exports = { }], }); }, - [`${CallExpression}[expression.callee.name=${/^RegExpPrototypeSymbol(Match|MatchAll|Search)$/}]`](node) { + [CallExpression(/^RegExpPrototypeSymbol(Match|MatchAll|Search)$/)](node) { context.report({ node, - message: node.expression.callee.name + ' looks up the "exec" property of `this` value', + message: node.callee.name + ' looks up the "exec" property of `this` value', }); }, - ...createUnsafeStringMethodReport(context, 'StringPrototypeMatch', 'Symbol.match'), - ...createUnsafeStringMethodReport(context, 'StringPrototypeMatchAll', 'Symbol.matchAll'), - ...createUnsafeStringMethodReport(context, 'StringPrototypeReplace', 'Symbol.replace'), - ...createUnsafeStringMethodReport(context, 'StringPrototypeReplaceAll', 'Symbol.replace'), - ...createUnsafeStringMethodReport(context, 'StringPrototypeSearch', 'Symbol.search'), - ...createUnsafeStringMethodReport(context, 'StringPrototypeSplit', 'Symbol.split'), + ...createUnsafeStringMethodReport(context, '%String.prototype.match%', 'Symbol.match'), + ...createUnsafeStringMethodReport(context, '%String.prototype.matchAll%', 'Symbol.matchAll'), + ...createUnsafeStringMethodOnRegexReport(context, '%String.prototype.replace%', 'Symbol.replace'), + ...createUnsafeStringMethodOnRegexReport(context, '%String.prototype.replaceAll%', 'Symbol.replace'), + ...createUnsafeStringMethodReport(context, '%String.prototype.search%', 'Symbol.search'), + ...createUnsafeStringMethodOnRegexReport(context, '%String.prototype.split%', 'Symbol.split'), 'NewExpression[callee.name="Proxy"][arguments.1.type="ObjectExpression"]'(node) { for (const { key, value } of node.arguments[1].properties) { @@ -146,7 +165,7 @@ module.exports = { }); }, - [`${CallExpression}[expression.callee.name=PromisePrototypeCatch]`](node) { + [CallExpression('PromisePrototypeCatch')](node) { context.report({ node, message: '%Promise.prototype.catch% look up the `then` property of ' + @@ -154,7 +173,7 @@ module.exports = { }); }, - [`${CallExpression}[expression.callee.name=PromisePrototypeFinally]`](node) { + [CallExpression('PromisePrototypeFinally')](node) { context.report({ node, message: '%Promise.prototype.finally% look up the `then` property of ' + @@ -163,10 +182,10 @@ module.exports = { }); }, - [`${CallExpression}[expression.callee.name=${/^Promise(All(Settled)?|Any|Race)/}]`](node) { + [CallExpression(/^Promise(All(Settled)?|Any|Race)/)](node) { context.report({ node, - message: `Use Safe${node.expression.callee.name} instead of ${node.expression.callee.name}`, + message: `Use Safe${node.callee.name} instead of ${node.callee.name}`, }); }, };