FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

lib: make primordials Promise methods safe · nodejs/node@ded8335 · GitHub

/ node Public

Commit ded8335

Browse files
aduh95
authored and
targos
committed
lib: make primordials Promise methods safe
`catch` and `finally` methods on %Promise.prototype% looks up the `then` property of the instance, making it at risk of prototype pollution. PR-URL: #38650 Backport-PR-URL: #38878 Refs: https://tc39.es/ecma262/#sec-promise.prototype.catch Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
1 parent 864fe99 commit ded8335

7 files changed

Lines changed: 87 additions & 13 deletions

File tree

‎lib/internal/fs/promises.js‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -7,10 +7,10 @@ const {
77
MathMin,
88
NumberIsSafeInteger,
99
Promise,
10-
PromisePrototypeFinally,
1110
PromisePrototypeThen,
1211
PromiseResolve,
1312
SafeArrayIterator,
13+
SafePromisePrototypeFinally,
1414
Symbol,
1515
Uint8Array,
1616
} = primordials;
@@ -186,12 +186,12 @@ class FileHandle extends EventEmitterMixin(JSTransferable) {
186186
this[kRefs]--;
187187
if (this[kRefs] === 0) {
188188
this[kFd] = -1;
189-
this[kClosePromise] = PromisePrototypeFinally(
189+
this[kClosePromise] = SafePromisePrototypeFinally(
190190
this[kHandle].close(),
191191
() => { this[kClosePromise] = undefined; }
192192
);
193193
} else {
194-
this[kClosePromise] = PromisePrototypeFinally(
194+
this[kClosePromise] = SafePromisePrototypeFinally(
195195
new Promise((resolve, reject) => {
196196
this[kCloseResolve] = resolve;
197197
this[kCloseReject] = reject;
@@ -507,7 +507,7 @@ async function rename(oldPath, newPath) {
507507

508508
async function truncate(path, len = 0) {
509509
const fd = await open(path, 'r+');
510-
return PromisePrototypeFinally(ftruncate(fd, len), fd.close);
510+
return SafePromisePrototypeFinally(ftruncate(fd, len), fd.close);
511511
}
512512

513513
async function ftruncate(handle, len = 0) {
@@ -638,7 +638,7 @@ async function lchmod(path, mode) {
638638
throw new ERR_METHOD_NOT_IMPLEMENTED('lchmod()');
639639

640640
const fd = await open(path, O_WRONLY | O_SYMLINK);
641-
return PromisePrototypeFinally(fchmod(fd, mode), fd.close);
641+
return SafePromisePrototypeFinally(fchmod(fd, mode), fd.close);
642642
}
643643

644644
async function lchown(path, uid, gid) {
@@ -717,7 +717,7 @@ async function writeFile(path, data, options) {
717717
checkAborted(options.signal);
718718

719719
const fd = await open(path, flag, options.mode);
720-
return PromisePrototypeFinally(
720+
return SafePromisePrototypeFinally(
721721
writeFileHandle(fd, data, options.signal, options.encoding), fd.close);
722722
}
723723

@@ -742,7 +742,7 @@ async function readFile(path, options) {
742742
checkAborted(options.signal);
743743

744744
const fd = await open(path, flag, 0o666);
745-
return PromisePrototypeFinally(readFileHandle(fd, options), fd.close);
745+
return SafePromisePrototypeFinally(readFileHandle(fd, options), fd.close);
746746
}
747747

748748
module.exports = {

‎lib/internal/modules/run_main.js‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
'use strict';
22

33
const {
4-
PromisePrototypeFinally,
54
StringPrototypeEndsWith,
65
} = primordials;
76
const CJSLoader = require('internal/modules/cjs/loader');
@@ -51,7 +50,7 @@ function runMainESM(mainPath) {
5150
}));
5251
}
5352

54-
function handleMainPromise(promise) {
53+
async function handleMainPromise(promise) {
5554
// Handle a Promise from running code that potentially does Top-Level Await.
5655
// In that case, it makes sense to set the exit code to a specific non-zero
5756
// value if the main code never finishes running.
@@ -60,7 +59,11 @@ function handleMainPromise(promise) {
6059
process.exitCode = 13;
6160
}
6261
process.on('exit', handler);
63-
return PromisePrototypeFinally(promise, () => process.off('exit', handler));
62+
try {
63+
return await promise;
64+
} finally {
65+
process.off('exit', handler);
66+
}
6467
}
6568

6669
// For backwards compatibility, we have to run a bunch of

‎lib/internal/per_context/primordials.js‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -253,6 +253,8 @@ const {
253253
Map,
254254
ObjectFreeze,
255255
ObjectSetPrototypeOf,
256+
Promise,
257+
PromisePrototypeThen,
256258
Set,
257259
SymbolIterator,
258260
WeakMap,
@@ -384,5 +386,34 @@ primordials.SafeWeakRef = makeSafe(
384386
}
385387
);
386388

389+
const SafePromise = makeSafe(
390+
Promise,
391+
class SafePromise extends Promise {
392+
// eslint-disable-next-line no-useless-constructor
393+
constructor(executor) { super(executor); }
394+
}
395+
);
396+
397+
primordials.PromisePrototypeCatch = (thisPromise, onRejected) =>
398+
PromisePrototypeThen(thisPromise, undefined, onRejected);
399+
400+
/**
401+
* Attaches a callback that is invoked when the Promise is settled (fulfilled or
402+
* rejected). The resolved value cannot be modified from the callback.
403+
* Prefer using async functions when possible.
404+
* @param {Promise<any>} thisPromise
405+
* @param {() => void) | undefined | null} onFinally The callback to execute
406+
* when the Promise is settled (fulfilled or rejected).
407+
* @returns A Promise for the completion of the callback.
408+
*/
409+
primordials.SafePromisePrototypeFinally = (thisPromise, onFinally) =>
410+
// Wrapping on a new Promise is necessary to not expose the SafePromise
411+
// prototype to user-land.
412+
new Promise((a, b) =>
413+
new SafePromise((a, b) => PromisePrototypeThen(thisPromise, a, b))
414+
.finally(onFinally)
415+
.then(a, b)
416+
);
417+
387418
ObjectSetPrototypeOf(primordials, null);
388419
ObjectFreeze(primordials);

‎lib/timers/promises.js‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,8 @@
33
const {
44
FunctionPrototypeBind,
55
Promise,
6-
PromisePrototypeFinally,
76
PromiseReject,
7+
SafePromisePrototypeFinally,
88
} = primordials;
99

1010
const {
@@ -71,7 +71,7 @@ function setTimeout(after, value, options = {}) {
7171
}
7272
});
7373
return oncancel !== undefined ?
74-
PromisePrototypeFinally(
74+
SafePromisePrototypeFinally(
7575
ret,
7676
() => signal.removeEventListener('abort', oncancel)) : ret;
7777
}
@@ -115,7 +115,7 @@ function setImmediate(value, options = {}) {
115115
}
116116
});
117117
return oncancel !== undefined ?
118-
PromisePrototypeFinally(
118+
SafePromisePrototypeFinally(
119119
ret,
120120
() => signal.removeEventListener('abort', oncancel)) : ret;
121121
}

‎test/message/esm_display_syntax_error_import.out‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,3 +6,4 @@ SyntaxError: The requested module '../fixtures/es-module-loaders/module-named-ex
66
at async ModuleJob.run (node:internal/modules/esm/module_job:*:*)
77
at async Loader.import (node:internal/modules/esm/loader:*:*)
88
at async Object.loadESM (node:internal/process/esm_loader:*:*)
9+
at async handleMainPromise (node:internal/modules/run_main:*:*)

‎test/message/esm_display_syntax_error_import_module.out‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,3 +6,4 @@ SyntaxError: The requested module './module-named-exports.mjs' does not provide
66
at async ModuleJob.run (node:internal/modules/esm/module_job:*:*)
77
at async Loader.import (node:internal/modules/esm/loader:*:*)
88
at async Object.loadESM (node:internal/process/esm_loader:*:*)
9+
at async handleMainPromise (node:internal/modules/run_main:*:*)

‎test/parallel/test-primordials-promise.js‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
// Flags: --expose-internals
2+
'use strict';
3+
4+
const common = require('../common');
5+
const assert = require('assert');
6+
7+
const {
8+
PromisePrototypeCatch,
9+
PromisePrototypeThen,
10+
SafePromisePrototypeFinally,
11+
} = require('internal/test/binding').primordials;
12+
13+
Promise.prototype.catch = common.mustNotCall();
14+
Promise.prototype.finally = common.mustNotCall();
15+
Promise.prototype.then = common.mustNotCall();
16+
17+
assertIsPromise(PromisePrototypeCatch(Promise.reject(), common.mustCall()));
18+
assertIsPromise(PromisePrototypeThen(test(), common.mustCall()));
19+
assertIsPromise(SafePromisePrototypeFinally(test(), common.mustCall()));
20+
21+
async function test() {
22+
const catchFn = common.mustCall();
23+
const finallyFn = common.mustCall();
24+
25+
try {
26+
await Promise.reject();
27+
} catch {
28+
catchFn();
29+
} finally {
30+
finallyFn();
31+
}
32+
}
33+
34+
function assertIsPromise(promise) {
35+
// Make sure the returned promise is a genuine %Promise% object and not a
36+
// subclass instance.
37+
assert.strictEqual(Object.getPrototypeOf(promise), Promise.prototype);
38+
}

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL