Skip to content

Commit

Permalink
lib: add Promise methods to avoid-prototype-pollution lint rule
Browse files Browse the repository at this point in the history
PR-URL: #43849
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
  • Loading branch information
aduh95 authored and ruyadorno committed Aug 22, 2022
1 parent a0c5783 commit 94912bb
Show file tree
Hide file tree
Showing 10 changed files with 91 additions and 45 deletions.
39 changes: 23 additions & 16 deletions lib/internal/debugger/inspect.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ const {
FunctionPrototypeBind,
Number,
Promise,
PromisePrototypeCatch,
PromisePrototypeThen,
PromiseResolve,
Proxy,
Expand Down Expand Up @@ -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) {
Expand All @@ -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() {
Expand Down
6 changes: 3 additions & 3 deletions lib/internal/http2/core.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ const {
ObjectDefineProperty,
ObjectPrototypeHasOwnProperty,
Promise,
PromisePrototypeCatch,
PromisePrototypeThen,
Proxy,
ReflectApply,
ReflectGet,
Expand Down Expand Up @@ -2456,8 +2456,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();
}
Expand Down
4 changes: 2 additions & 2 deletions lib/internal/main/worker_thread.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ const {
ArrayPrototypePushApply,
ArrayPrototypeSplice,
ObjectDefineProperty,
PromisePrototypeCatch,
PromisePrototypeThen,
globalThis: { Atomics },
} = primordials;

Expand Down Expand Up @@ -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 {
Expand Down
4 changes: 2 additions & 2 deletions lib/internal/modules/esm/module_job.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ const {
ObjectCreate,
ObjectSetPrototypeOf,
PromiseResolve,
PromisePrototypeCatch,
PromisePrototypeThen,
ReflectApply,
RegExpPrototypeExec,
RegExpPrototypeSymbolReplace,
Expand Down Expand Up @@ -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.
Expand Down
3 changes: 0 additions & 3 deletions lib/internal/per_context/primordials.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
4 changes: 2 additions & 2 deletions lib/internal/streams/operators.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ const {
NumberIsNaN,
Promise,
PromiseReject,
PromisePrototypeCatch,
PromisePrototypeThen,
Symbol,
} = primordials;

Expand Down Expand Up @@ -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;
Expand Down
6 changes: 3 additions & 3 deletions lib/internal/webstreams/readablestream.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ const {
ObjectDefineProperties,
ObjectSetPrototypeOf,
Promise,
PromisePrototypeCatch,
PromisePrototypeThen,
PromiseResolve,
PromiseReject,
Expand Down Expand Up @@ -1334,7 +1333,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) {
Expand Down Expand Up @@ -1503,8 +1502,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);
Expand Down
20 changes: 20 additions & 0 deletions test/parallel/test-eslint-avoid-prototype-pollution.js
Original file line number Diff line number Diff line change
Expand Up @@ -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/ }]
},
]
});
18 changes: 8 additions & 10 deletions test/parallel/test-primordials-promise.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ const common = require('../common');
const assert = require('assert');

const {
PromisePrototypeCatch,
PromisePrototypeThen,
SafePromiseAll,
SafePromiseAllSettled,
Expand All @@ -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()));

Expand Down
32 changes: 28 additions & 4 deletions tools/eslint-rules/avoid-prototype-pollution.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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}`,
});
},
};
},
};

0 comments on commit 94912bb

Please sign in to comment.