Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit ded8335

Browse filesBrowse files
aduh95targos
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
Copy full SHA for ded8335

File tree

Expand file treeCollapse file tree

7 files changed

+87
-13
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

7 files changed

+87
-13
lines changed
Open diff view settings
Collapse file

‎lib/internal/fs/promises.js‎

Copy file name to clipboardExpand all lines: lib/internal/fs/promises.js
+7-7Lines 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 = {
Collapse file

‎lib/internal/modules/run_main.js‎

Copy file name to clipboardExpand all lines: lib/internal/modules/run_main.js
+6-3Lines 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
Collapse file

‎lib/internal/per_context/primordials.js‎

Copy file name to clipboardExpand all lines: lib/internal/per_context/primordials.js
+31Lines 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);
Collapse file

‎lib/timers/promises.js‎

Copy file name to clipboardExpand all lines: lib/timers/promises.js
+3-3Lines 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
}
Collapse file

‎test/message/esm_display_syntax_error_import.out‎

Copy file name to clipboardExpand all lines: test/message/esm_display_syntax_error_import.out
+1Lines 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:*:*)
Collapse file

‎test/message/esm_display_syntax_error_import_module.out‎

Copy file name to clipboardExpand all lines: test/message/esm_display_syntax_error_import_module.out
+1Lines 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:*:*)
Collapse file

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

Copy file name to clipboard
+38Lines 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 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.