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 f3179f7

Browse filesBrowse files
bmecktargos
bmeck
authored and
targos
committed
policy: ensure workers do not read fs for policy
This prevents a main thread from rewriting the policy file and loading a worker that has a different policy from the main thread. PR-URL: #25710 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
1 parent ebcdbeb commit f3179f7
Copy full SHA for f3179f7

File tree

Expand file treeCollapse file tree

5 files changed

+138
-16
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

5 files changed

+138
-16
lines changed
Open diff view settings
Collapse file

‎lib/internal/bootstrap/node.js‎

Copy file name to clipboardExpand all lines: lib/internal/bootstrap/node.js
+5-5Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -173,7 +173,7 @@ function startup() {
173173

174174
// TODO(joyeecheung): move this down further to get better snapshotting
175175
const experimentalPolicy = getOptionValue('--experimental-policy');
176-
if (experimentalPolicy) {
176+
if (isMainThread && experimentalPolicy) {
177177
process.emitWarning('Policies are experimental.',
178178
'ExperimentalWarning');
179179
const { pathToFileURL, URL } = NativeModule.require('url');
@@ -364,8 +364,6 @@ function startup() {
364364
}
365365

366366
function startWorkerThreadExecution() {
367-
prepareUserCodeExecution();
368-
369367
// If we are in a worker thread, execute the script sent through the
370368
// message port.
371369
const {
@@ -382,7 +380,9 @@ function startWorkerThreadExecution() {
382380
debug(`[${threadId}] is setting up worker child environment`);
383381

384382
const port = getEnvMessagePort();
385-
port.on('message', createMessageHandler(port));
383+
port.on('message', createMessageHandler(
384+
port,
385+
prepareUserCodeExecution));
386386
port.start();
387387

388388
// Overwrite fatalException
@@ -476,7 +476,7 @@ function prepareUserCodeExecution() {
476476

477477
// For user code, we preload modules if `-r` is passed
478478
const preloadModules = getOptionValue('--require');
479-
if (preloadModules) {
479+
if (preloadModules.length) {
480480
const {
481481
_preloadModules
482482
} = NativeModule.require('internal/modules/cjs/loader');
Collapse file

‎lib/internal/process/policy.js‎

Copy file name to clipboardExpand all lines: lib/internal/process/policy.js
+18Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,10 +5,14 @@ const {
55
} = require('internal/errors').codes;
66
const { Manifest } = require('internal/policy/manifest');
77
let manifest;
8+
let manifestSrc;
9+
let manifestURL;
810

911
module.exports = Object.freeze({
1012
__proto__: null,
1113
setup(src, url) {
14+
manifestSrc = src;
15+
manifestURL = url;
1216
if (src === null) {
1317
manifest = null;
1418
return;
@@ -31,6 +35,20 @@ module.exports = Object.freeze({
3135
return manifest;
3236
},
3337

38+
get src() {
39+
if (typeof manifestSrc === 'undefined') {
40+
throw new ERR_MANIFEST_TDZ();
41+
}
42+
return manifestSrc;
43+
},
44+
45+
get url() {
46+
if (typeof manifestURL === 'undefined') {
47+
throw new ERR_MANIFEST_TDZ();
48+
}
49+
return manifestURL;
50+
},
51+
3452
assertIntegrity(moduleURL, content) {
3553
this.manifest.matchesIntegrity(moduleURL, content);
3654
}
Collapse file

‎lib/internal/process/worker_thread_only.js‎

Copy file name to clipboardExpand all lines: lib/internal/process/worker_thread_only.js
+14-2Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -43,12 +43,24 @@ function initializeWorkerStdio() {
4343
};
4444
}
4545

46-
function createMessageHandler(port) {
46+
function createMessageHandler(port, prepareUserCodeExecution) {
4747
const publicWorker = require('worker_threads');
4848

4949
return function(message) {
5050
if (message.type === messageTypes.LOAD_SCRIPT) {
51-
const { filename, doEval, workerData, publicPort, hasStdin } = message;
51+
const {
52+
filename,
53+
doEval,
54+
workerData,
55+
publicPort,
56+
manifestSrc,
57+
manifestURL,
58+
hasStdin
59+
} = message;
60+
if (manifestSrc) {
61+
require('internal/process/policy').setup(manifestSrc, manifestURL);
62+
}
63+
prepareUserCodeExecution();
5264
publicWorker.parentPort = publicPort;
5365
publicWorker.workerData = workerData;
5466

Collapse file

‎lib/internal/worker.js‎

Copy file name to clipboardExpand all lines: lib/internal/worker.js
+4Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ const {
1212
ERR_INVALID_ARG_TYPE,
1313
} = require('internal/errors').codes;
1414
const { validateString } = require('internal/validators');
15+
const { getOptionValue } = require('internal/options');
1516

1617
const {
1718
drainMessagePort,
@@ -112,6 +113,9 @@ class Worker extends EventEmitter {
112113
doEval: !!options.eval,
113114
workerData: options.workerData,
114115
publicPort: port2,
116+
manifestSrc: getOptionValue('--experimental-policy') ?
117+
require('internal/process/policy').src :
118+
null,
115119
hasStdin: !!options.stdin
116120
}, [port2]);
117121
// Actually start the new thread now that everything is in place.
Collapse file

‎test/parallel/test-policy-integrity.js‎

Copy file name to clipboardExpand all lines: test/parallel/test-policy-integrity.js
+97-9Lines changed: 97 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,17 @@ const parentFilepath = path.join(tmpdir.path, 'parent.js');
3333
const parentURL = pathToFileURL(parentFilepath);
3434
const parentBody = 'require(\'./dep.js\')';
3535

36+
const workerSpawningFilepath = path.join(tmpdir.path, 'worker_spawner.js');
37+
const workerSpawningURL = pathToFileURL(workerSpawningFilepath);
38+
const workerSpawningBody = `
39+
const { Worker } = require('worker_threads');
40+
// make sure this is gone to ensure we don't do another fs read of it
41+
// will error out if we do
42+
require('fs').unlinkSync(${JSON.stringify(policyFilepath)});
43+
const w = new Worker(${JSON.stringify(parentFilepath)});
44+
w.on('exit', process.exit);
45+
`;
46+
3647
const depFilepath = path.join(tmpdir.path, 'dep.js');
3748
const depURL = pathToFileURL(depFilepath);
3849
const depBody = '';
@@ -49,8 +60,9 @@ if (!tmpdirURL.pathname.endsWith('/')) {
4960
}
5061
function test({
5162
shouldFail = false,
63+
preload = [],
5264
entry,
53-
onerror,
65+
onerror = undefined,
5466
resources = {}
5567
}) {
5668
const manifest = {
@@ -65,7 +77,9 @@ function test({
6577
}
6678
fs.writeFileSync(policyFilepath, JSON.stringify(manifest, null, 2));
6779
const { status } = spawnSync(process.execPath, [
68-
'--experimental-policy', policyFilepath, entry
80+
'--experimental-policy', policyFilepath,
81+
...preload.map((m) => ['-r', m]).flat(),
82+
entry
6983
]);
7084
if (shouldFail) {
7185
assert.notStrictEqual(status, 0);
@@ -74,13 +88,25 @@ function test({
7488
}
7589
}
7690

77-
const { status } = spawnSync(process.execPath, [
78-
'--experimental-policy', policyFilepath,
79-
'--experimental-policy', policyFilepath
80-
], {
81-
stdio: 'pipe'
82-
});
83-
assert.notStrictEqual(status, 0, 'Should not allow multiple policies');
91+
{
92+
const { status } = spawnSync(process.execPath, [
93+
'--experimental-policy', policyFilepath,
94+
'--experimental-policy', policyFilepath
95+
], {
96+
stdio: 'pipe'
97+
});
98+
assert.notStrictEqual(status, 0, 'Should not allow multiple policies');
99+
}
100+
{
101+
const enoentFilepath = path.join(tmpdir.path, 'enoent');
102+
try { fs.unlinkSync(enoentFilepath); } catch {}
103+
const { status } = spawnSync(process.execPath, [
104+
'--experimental-policy', enoentFilepath, '-e', ''
105+
], {
106+
stdio: 'pipe'
107+
});
108+
assert.notStrictEqual(status, 0, 'Should not allow missing policies');
109+
}
84110

85111
test({
86112
shouldFail: true,
@@ -195,6 +221,21 @@ test({
195221
}
196222
}
197223
});
224+
test({
225+
shouldFail: false,
226+
preload: [depFilepath],
227+
entry: parentFilepath,
228+
resources: {
229+
[parentURL]: {
230+
body: parentBody,
231+
match: true,
232+
},
233+
[depURL]: {
234+
body: depBody,
235+
match: true,
236+
}
237+
}
238+
});
198239
test({
199240
shouldFail: true,
200241
entry: parentFilepath,
@@ -295,3 +336,50 @@ test({
295336
}
296337
}
297338
});
339+
test({
340+
shouldFail: true,
341+
entry: workerSpawningFilepath,
342+
resources: {
343+
[workerSpawningURL]: {
344+
body: workerSpawningBody,
345+
match: true,
346+
},
347+
}
348+
});
349+
test({
350+
shouldFail: false,
351+
entry: workerSpawningFilepath,
352+
resources: {
353+
[workerSpawningURL]: {
354+
body: workerSpawningBody,
355+
match: true,
356+
},
357+
[parentURL]: {
358+
body: parentBody,
359+
match: true,
360+
},
361+
[depURL]: {
362+
body: depBody,
363+
match: true,
364+
}
365+
}
366+
});
367+
test({
368+
shouldFail: false,
369+
entry: workerSpawningFilepath,
370+
preload: [parentFilepath],
371+
resources: {
372+
[workerSpawningURL]: {
373+
body: workerSpawningBody,
374+
match: true,
375+
},
376+
[parentURL]: {
377+
body: parentBody,
378+
match: true,
379+
},
380+
[depURL]: {
381+
body: depBody,
382+
match: true,
383+
}
384+
}
385+
});

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.