From 29ed47d2709af300e1c9be3c644ecb0ffdc393aa Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Wed, 27 Jul 2022 00:38:24 +0200 Subject: [PATCH] lib: add `Promise` methods to `avoid-prototype-pollution` lint rule PR-URL: https://github.com/nodejs/node/pull/43849 Reviewed-By: Matteo Collina Reviewed-By: Paolo Insogna --- lib/internal/debugger/inspect.js | 39 +++++++++++-------- lib/internal/http2/core.js | 6 +-- lib/internal/main/worker_thread.js | 4 +- lib/internal/modules/esm/module_job.js | 4 +- lib/internal/per_context/primordials.js | 3 -- lib/internal/streams/operators.js | 4 +- lib/internal/webstreams/readablestream.js | 6 +-- .../test-eslint-avoid-prototype-pollution.js | 20 ++++++++++ test/parallel/test-primordials-promise.js | 18 ++++----- .../eslint-rules/avoid-prototype-pollution.js | 32 +++++++++++++-- 10 files changed, 91 insertions(+), 45 deletions(-) diff --git a/lib/internal/debugger/inspect.js b/lib/internal/debugger/inspect.js index e5211c285bd..42b5c64ab87 100644 --- a/lib/internal/debugger/inspect.js +++ b/lib/internal/debugger/inspect.js @@ -11,7 +11,6 @@ const { FunctionPrototypeBind, Number, Promise, - PromisePrototypeCatch, PromisePrototypeThen, PromiseResolve, Proxy, @@ -169,12 +168,17 @@ class NodeInspector { process.once('SIGTERM', exitCodeZero); process.once('SIGHUP', exitCodeZero); - PromisePrototypeCatch(PromisePrototypeThen(this.run(), async () => { - const repl = await startRepl(); - this.repl = repl; - this.repl.on('exit', exitCodeZero); - this.paused = false; - }), (error) => process.nextTick(() => { throw error; })); + (async () => { + try { + await this.run(); + const repl = await startRepl(); + this.repl = repl; + this.repl.on('exit', exitCodeZero); + this.paused = false; + } catch (error) { + process.nextTick(() => { throw error; }); + } + })(); } suspendReplWhile(fn) { @@ -183,16 +187,19 @@ class NodeInspector { } this.stdin.pause(); this.paused = true; - return PromisePrototypeCatch(PromisePrototypeThen(new Promise((resolve) => { - resolve(fn()); - }), () => { - this.paused = false; - if (this.repl) { - this.repl.resume(); - this.repl.displayPrompt(); + return (async () => { + try { + await fn(); + this.paused = false; + if (this.repl) { + this.repl.resume(); + this.repl.displayPrompt(); + } + this.stdin.resume(); + } catch (error) { + process.nextTick(() => { throw error; }); } - this.stdin.resume(); - }), (error) => process.nextTick(() => { throw error; })); + })(); } killChild() { diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 0f4dc9bf986..d59d4a76070 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -17,7 +17,7 @@ const { ObjectDefineProperty, ObjectPrototypeHasOwnProperty, Promise, - PromisePrototypeCatch, + PromisePrototypeThen, Proxy, ReflectApply, ReflectGet, @@ -2451,8 +2451,8 @@ function processHeaders(oldHeaders, options) { function onFileUnpipe() { const stream = this.sink[kOwner]; if (stream.ownsFd) - PromisePrototypeCatch(this.source.close(), - FunctionPrototypeBind(stream.destroy, stream)); + PromisePrototypeThen(this.source.close(), undefined, + FunctionPrototypeBind(stream.destroy, stream)); else this.source.releaseFD(); } diff --git a/lib/internal/main/worker_thread.js b/lib/internal/main/worker_thread.js index e21c1b1fe2c..8d5bc45edd5 100644 --- a/lib/internal/main/worker_thread.js +++ b/lib/internal/main/worker_thread.js @@ -8,7 +8,7 @@ const { ArrayPrototypePushApply, ArrayPrototypeSplice, ObjectDefineProperty, - PromisePrototypeCatch, + PromisePrototypeThen, globalThis: { Atomics }, } = primordials; @@ -185,7 +185,7 @@ port.on('message', (message) => { evalScript(name, filename); } else if (doEval === 'module') { const { evalModule } = require('internal/process/execution'); - PromisePrototypeCatch(evalModule(filename), (e) => { + PromisePrototypeThen(evalModule(filename), undefined, (e) => { workerOnGlobalUncaughtException(e, true); }); } else { diff --git a/lib/internal/modules/esm/module_job.js b/lib/internal/modules/esm/module_job.js index 3a2ec7343a1..2dd69b32f77 100644 --- a/lib/internal/modules/esm/module_job.js +++ b/lib/internal/modules/esm/module_job.js @@ -8,7 +8,7 @@ const { ObjectCreate, ObjectSetPrototypeOf, PromiseResolve, - PromisePrototypeCatch, + PromisePrototypeThen, ReflectApply, RegExpPrototypeExec, RegExpPrototypeSymbolReplace, @@ -88,7 +88,7 @@ class ModuleJob { this.linked = link(); // This promise is awaited later anyway, so silence // 'unhandled rejection' warnings. - PromisePrototypeCatch(this.linked, noop); + PromisePrototypeThen(this.linked, undefined, noop); // instantiated == deep dependency jobs wrappers are instantiated, // and module wrapper is instantiated. diff --git a/lib/internal/per_context/primordials.js b/lib/internal/per_context/primordials.js index 7c6f513d9cc..2ee5efb6cb7 100644 --- a/lib/internal/per_context/primordials.js +++ b/lib/internal/per_context/primordials.js @@ -409,9 +409,6 @@ const SafePromise = makeSafe( } ); -primordials.PromisePrototypeCatch = (thisPromise, onRejected) => - PromisePrototypeThen(thisPromise, undefined, onRejected); - /** * Attaches a callback that is invoked when the Promise is settled (fulfilled or * rejected). The resolved value cannot be modified from the callback. diff --git a/lib/internal/streams/operators.js b/lib/internal/streams/operators.js index a450aa78e75..d45efbad7c2 100644 --- a/lib/internal/streams/operators.js +++ b/lib/internal/streams/operators.js @@ -25,7 +25,7 @@ const { NumberIsNaN, Promise, PromiseReject, - PromisePrototypeCatch, + PromisePrototypeThen, Symbol, } = primordials; @@ -113,7 +113,7 @@ function map(fn, options) { queue.push(kEof); } catch (err) { const val = PromiseReject(err); - PromisePrototypeCatch(val, onDone); + PromisePrototypeThen(val, undefined, onDone); queue.push(val); } finally { done = true; diff --git a/lib/internal/webstreams/readablestream.js b/lib/internal/webstreams/readablestream.js index 0e7d33e9b71..e92025cc161 100644 --- a/lib/internal/webstreams/readablestream.js +++ b/lib/internal/webstreams/readablestream.js @@ -16,7 +16,6 @@ const { ObjectDefineProperties, ObjectSetPrototypeOf, Promise, - PromisePrototypeCatch, PromisePrototypeThen, PromiseResolve, PromiseReject, @@ -1329,7 +1328,7 @@ function readableStreamPipeTo( if (stream[kState].state === 'errored') action(stream[kState].storedError); else - PromisePrototypeCatch(promise, action); + PromisePrototypeThen(promise, undefined, action); } function watchClosed(stream, promise, action) { @@ -1498,8 +1497,9 @@ function readableStreamTee(stream, cloneForBranch2) { branch2 = createTeeReadableStream(nonOpStart, pullAlgorithm, cancel2Algorithm); - PromisePrototypeCatch( + PromisePrototypeThen( reader[kState].close.promise, + undefined, (error) => { readableStreamDefaultControllerError(branch1[kState].controller, error); readableStreamDefaultControllerError(branch2[kState].controller, error); diff --git a/test/parallel/test-eslint-avoid-prototype-pollution.js b/test/parallel/test-eslint-avoid-prototype-pollution.js index 26b0852c0c2..f10d6ea973b 100644 --- a/test/parallel/test-eslint-avoid-prototype-pollution.js +++ b/test/parallel/test-eslint-avoid-prototype-pollution.js @@ -203,5 +203,25 @@ new RuleTester({ code: 'new Proxy({}, { ...{ __proto__: null } })', errors: [{ message: /null-prototype/ }] }, + { + code: 'PromisePrototypeCatch(promise, ()=>{})', + errors: [{ message: /\bPromisePrototypeThen\b/ }] + }, + { + code: 'PromiseAll([])', + errors: [{ message: /\bSafePromiseAll\b/ }] + }, + { + code: 'PromiseAllSettled([])', + errors: [{ message: /\bSafePromiseAllSettled\b/ }] + }, + { + code: 'PromiseAny([])', + errors: [{ message: /\bSafePromiseAny\b/ }] + }, + { + code: 'PromiseRace([])', + errors: [{ message: /\bSafePromiseRace\b/ }] + }, ] }); diff --git a/test/parallel/test-primordials-promise.js b/test/parallel/test-primordials-promise.js index 7ff29fc0f5d..c753b4b7e79 100644 --- a/test/parallel/test-primordials-promise.js +++ b/test/parallel/test-primordials-promise.js @@ -5,7 +5,6 @@ const common = require('../common'); const assert = require('assert'); const { - PromisePrototypeCatch, PromisePrototypeThen, SafePromiseAll, SafePromiseAllSettled, @@ -14,16 +13,15 @@ const { SafePromiseRace, } = require('internal/test/binding').primordials; -Array.prototype[Symbol.iterator] = common.mustNotCall(); -Promise.all = common.mustNotCall(); -Promise.allSettled = common.mustNotCall(); -Promise.any = common.mustNotCall(); -Promise.race = common.mustNotCall(); -Promise.prototype.catch = common.mustNotCall(); -Promise.prototype.finally = common.mustNotCall(); -Promise.prototype.then = common.mustNotCall(); +Array.prototype[Symbol.iterator] = common.mustNotCall('%Array.prototype%[@@iterator]'); +Promise.all = common.mustNotCall('%Promise%.all'); +Promise.allSettled = common.mustNotCall('%Promise%.allSettled'); +Promise.any = common.mustNotCall('%Promise%.any'); +Promise.race = common.mustNotCall('%Promise%.race'); +Promise.prototype.catch = common.mustNotCall('%Promise.prototype%.catch'); +Promise.prototype.finally = common.mustNotCall('%Promise.prototype%.finally'); +Promise.prototype.then = common.mustNotCall('%Promise.prototype%.then'); -assertIsPromise(PromisePrototypeCatch(Promise.reject(), common.mustCall())); assertIsPromise(PromisePrototypeThen(test(), common.mustCall())); assertIsPromise(SafePromisePrototypeFinally(test(), common.mustCall())); diff --git a/tools/eslint-rules/avoid-prototype-pollution.js b/tools/eslint-rules/avoid-prototype-pollution.js index 1f71272bd7d..d59b62f9502 100644 --- a/tools/eslint-rules/avoid-prototype-pollution.js +++ b/tools/eslint-rules/avoid-prototype-pollution.js @@ -109,11 +109,11 @@ module.exports = { testRange.start = testRange.start + 'RegexpPrototype'.length; testRange.end = testRange.start + 'Test'.length; return [ - fixer.replaceTextRange(node.range, 'Exec'), + fixer.replaceTextRange(testRange, 'Exec'), fixer.insertTextAfter(node, ' !== null'), ]; } - }] + }], }); }, [`${CallExpression}[expression.callee.name=${/^RegExpPrototypeSymbol(Match|MatchAll|Search)$/}]`](node) { @@ -142,9 +142,33 @@ module.exports = { } context.report({ node, - message: 'Proxy handler must be a null-prototype object' + message: 'Proxy handler must be a null-prototype object', }); - } + }, + + [`${CallExpression}[expression.callee.name=PromisePrototypeCatch]`](node) { + context.report({ + node, + message: '%Promise.prototype.catch% look up the `then` property of ' + + 'the `this` argument, use PromisePrototypeThen instead', + }); + }, + + [`${CallExpression}[expression.callee.name=PromisePrototypeFinally]`](node) { + context.report({ + node, + message: '%Promise.prototype.finally% look up the `then` property of ' + + 'the `this` argument, use SafePromisePrototypeFinally or ' + + 'try/finally instead', + }); + }, + + [`${CallExpression}[expression.callee.name=${/^Promise(All(Settled)?|Any|Race)/}]`](node) { + context.report({ + node, + message: `Use Safe${node.expression.callee.name} instead of ${node.expression.callee.name}`, + }); + }, }; }, };