Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lib: add Promise methods to avoid-prototype-pollution lint rule #43849

Merged
merged 2 commits into from
Jul 26, 2022
Merged

lib: add Promise methods to avoid-prototype-pollution lint rule #43849

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b173555
lib: add `Promise` methods to `avoid-prototype-pollution` lint rule
aduh95 Jul 15, 2022
f4c5d9a
Update test-primordials-promise.js
aduh95 Jul 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 23 additions & 16 deletions lib/internal/debugger/inspect.js
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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 @@ -183,7 +183,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
View file
Open in desktop
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
View file
Open in desktop
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);

Comment on lines -412 to -414
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't using this helper equally robust, and much cleaner, than forcing the whole codebase to use PromisePrototypeThen with an undefined first argument?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's been decided some time ago that primordials should keep their original value, as it's what most seem to expect. We could create a SafePromisePrototypeCatch but not sure it's really necessary (there's only a few occurrences of this).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where was that decision made?

I think it's absurd to claim the users of primordials have any expectations whatsoever about their implementation, only about what they do (especially since ALL of them are call-bound, which means NONE of them have their original value). If the lint rule would be preventing them from using PromisePrototypeCatch, how is that aligned with their expectations, but "PromisePrototypeCatch is made safe" isn't?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The linter rule is easy to workaround (you can add a // eslint-disable comment), while changing the primordials value makes it impossible to access the original %Promise.prototype.catch% reliably.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it's call-bound, you can never access it anyways. When would someone need a bind-alias to the original?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I think the disable comment should be used only when there is a good reason to override a good rule, not whenever a bad rule kicks in.

I gotta second Jordan: I would find it very unintuitive to use PromisePrototypeThen() instead of PromisePrototypeCatch(). I might use PromisePrototypeThen(), when I have both success AND failure callbacks, but never just for failure, unless I was forced to (and if I was forced to, I'd be silently cursing whoever—and I like you, so I don't wanna do that 😆).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would honestly remove all "raw" promise calls for async/await since that always produces a safe native promise as far as I'm aware.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with Benjamin, if that rule can encourage contributors to use async/await rather than chaining promises, I'm calling it a win. As I said above, we can always add a SafePromisePrototypeCatch later if necessary, and in the meantime, I'm OK with being silently cursed :)

/**
* 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
View file
Open in desktop
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
View file
Open in desktop
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 @@ -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) {
Expand Down Expand Up @@ -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);
Expand Down
20 changes: 20 additions & 0 deletions test/parallel/test-eslint-avoid-prototype-pollution.js
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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}`,
});
},
};
},
};