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.

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

Restrict process and Buffer globals to CommonJS #26334

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 2 additions & 1 deletion lib/internal/bootstrap/loaders.js
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,8 @@ let internalBinding;
const loaderExports = {
internalBinding,
NativeModule,
require: nativeModuleRequire
require: nativeModuleRequire,
cjsContext: { __proto__: null, process: undefined, Buffer: undefined }
Copy link
Member

Choose a reason for hiding this comment

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

Is __proto__: null necessary? I assume so, because it’s used as the context object for CJS compilation below, but I feel like it could be responsible for part of the performance impact. If it is necessary, it might also be good to explicitly test the fact that inherited methods aren’t entered into the CJS module’s scope?

Copy link
Member

Choose a reason for hiding this comment

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

To my understanding context is equivalent to regular with() so all delegated enumerable properties would be visible, JS language builtins are not enumerable however things matching web specs are. To test, you should ensure that you define an enumerable property. If context is acting like with() it should expose the enumerable properties even if added late in execution.

Copy link
Member

Choose a reason for hiding this comment

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

To my understanding context is equivalent to regular with() so all delegated enumerable properties would be visible, JS language builtins are not enumerable however things matching web specs are. To test, you should ensure that you define an enumerable property. If context is acting like with() it should expose the enumerable properties even if added late in execution.

};

const loaderId = 'internal/bootstrap/loaders';
Expand Down
17 changes: 8 additions & 9 deletions lib/internal/bootstrap/pre_execution.js
Original file line number Diff line number Diff line change
Expand Up @@ -263,28 +263,27 @@ function initializeDeprecations() {
'Please use public APIs instead.', 'DEP0111');
}

// Create global.process and global.Buffer as getters so that we have a
// deprecation path for these in ES Modules.
// See https://github.com/nodejs/node/pull/26334.
let _process = process;
// Create global.process and global.Buffer as getters to allow deprecation.
const { cjsContext } = require('internal/bootstrap/loaders');
cjsContext.process = process;
cjsContext.Buffer = Buffer;
Object.defineProperty(global, 'process', {
get() {
return _process;
return cjsContext.process;
},
set(value) {
_process = value;
cjsContext.process = value;
},
enumerable: false,
configurable: true
});

let _Buffer = Buffer;
Object.defineProperty(global, 'Buffer', {
get() {
return _Buffer;
return cjsContext.Buffer;
},
set(value) {
_Buffer = value;
cjsContext.Buffer = value;
},
enumerable: false,
configurable: true
Expand Down
4 changes: 2 additions & 2 deletions lib/internal/modules/cjs/loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@

const { JSON, Object, Reflect } = primordials;

const { NativeModule } = require('internal/bootstrap/loaders');
const { NativeModule, cjsContext } = require('internal/bootstrap/loaders');
const { pathToFileURL, fileURLToPath, URL } = require('internal/url');
const { deprecate } = require('internal/util');
const vm = require('vm');
Expand Down Expand Up @@ -713,7 +713,7 @@ function wrapSafe(filename, content) {
undefined,
false,
undefined,
[],
[cjsContext],
[
'exports',
'require',
Expand Down
45 changes: 45 additions & 0 deletions lib/internal/modules/esm/deprecate_process_buffer.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
'use strict';

const { cjsContext } = require('internal/bootstrap/loaders');
const { getStackSourceName } = internalBinding('contextify');
guybedford marked this conversation as resolved.
Show resolved Hide resolved
const { deprecate } = require('internal/util');
const { Object } = primordials;

module.exports = () => {
// Deprecate global.process and global.Buffer access in ES Modules.
const processGetter = deprecate(
() => cjsContext.process,
'global.process is deprecated in ECMAScript Modules. ' +
'Please use `import process from \'process\'` instead.', 'DEP0XXX');
Object.defineProperty(global, 'process', {
get() {
const sourceName = getStackSourceName(1);
if (sourceName && sourceName.startsWith('file://'))
return processGetter();
return cjsContext.process;
},
set(value) {
cjsContext.process = value;
},
enumerable: false,
configurable: true
});

const bufferGetter = deprecate(
() => cjsContext.Buffer,
'global.Buffer is deprecated in ECMAScript Modules. ' +
'Please use `import { Buffer } from \'buffer\'` instead.', 'DEP0YYY');
Object.defineProperty(global, 'Buffer', {
get() {
const sourceName = getStackSourceName(1);
if (sourceName && sourceName.startsWith('file://'))
return bufferGetter();
return cjsContext.Buffer;
},
set(value) {
cjsContext.Buffer = value;
},
enumerable: false,
configurable: true
});
};
7 changes: 7 additions & 0 deletions lib/internal/modules/esm/translators.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ const fs = require('fs');
const { fileURLToPath, URL } = require('url');
const { debuglog } = require('internal/util/debuglog');
const { promisify } = require('internal/util');
const deprecateProcessBufferAccess =
require('internal/modules/esm/deprecate_process_buffer');
const esmLoader = require('internal/process/esm_loader');
const {
ERR_UNKNOWN_BUILTIN_MODULE
Expand All @@ -43,7 +45,12 @@ async function importModuleDynamically(specifier, { url }) {
}

// Strategy for loading a standard JavaScript module
let firstModule = true;
translators.set('module', async function moduleStrategy(url) {
if (firstModule) {
deprecateProcessBufferAccess();
firstModule = false;
}
const source = `${await readFileAsync(new URL(url))}`;
debug(`Translating StandardModule ${url}`);
const { ModuleWrap, callbackMap } = internalBinding('module_wrap');
Expand Down
1 change: 1 addition & 0 deletions node.gyp
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,7 @@
'lib/internal/modules/esm/loader.js',
'lib/internal/modules/esm/create_dynamic_module.js',
'lib/internal/modules/esm/default_resolve.js',
'lib/internal/modules/esm/deprecate_process_buffer.js',
'lib/internal/modules/esm/module_job.js',
'lib/internal/modules/esm/module_map.js',
'lib/internal/modules/esm/translators.js',
Expand Down
27 changes: 27 additions & 0 deletions src/node_contextify.cc
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@ using v8::Script;
using v8::ScriptCompiler;
using v8::ScriptOrigin;
using v8::ScriptOrModule;
using v8::StackFrame;
using v8::StackTrace;
using v8::String;
using v8::Symbol;
using v8::Uint32;
Expand Down Expand Up @@ -235,6 +237,7 @@ void ContextifyContext::Init(Environment* env, Local<Object> target) {
env->SetMethod(target, "makeContext", MakeContext);
env->SetMethod(target, "isContext", IsContext);
env->SetMethod(target, "compileFunction", CompileFunction);
env->SetMethod(target, "getStackSourceName", GetStackSourceName);
}


Expand Down Expand Up @@ -299,6 +302,30 @@ void ContextifyContext::IsContext(const FunctionCallbackInfo<Value>& args) {
args.GetReturnValue().Set(result.FromJust());
}

void ContextifyContext::GetStackSourceName(
const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);

CHECK_EQ(args.Length(), 1);
CHECK(args[0]->IsNumber());

int32_t stack_depth = args[0].As<Integer>()->Value();

Local<StackFrame> frame;
do {
Local<StackTrace> stack =
StackTrace::CurrentStackTrace(env->isolate(),
stack_depth + 1);
if (stack_depth >= stack->GetFrameCount()) {
return;
}
frame = stack->GetFrame(env->isolate(), stack_depth++);
} while (frame->IsEval());

Local<Value> script_name = frame->GetScriptName();

args.GetReturnValue().Set(script_name);
}

void ContextifyContext::WeakCallback(
const WeakCallbackInfo<ContextifyContext>& data) {
Expand Down
2 changes: 2 additions & 0 deletions src/node_contextify.h
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@ class ContextifyContext {
private:
static void MakeContext(const v8::FunctionCallbackInfo<v8::Value>& args);
static void IsContext(const v8::FunctionCallbackInfo<v8::Value>& args);
static void GetStackSourceName(
const v8::FunctionCallbackInfo<v8::Value>& args);
static void CompileFunction(
const v8::FunctionCallbackInfo<v8::Value>& args);
static void WeakCallback(
Expand Down
7 changes: 1 addition & 6 deletions test/common/index.mjs
Original file line number Diff line number Diff line change
@@ -1,12 +1,7 @@
// Flags: --experimental-modules
/* eslint-disable node-core/require-common-first, node-core/required-modules */

import { createRequireFromPath } from 'module';
import { fileURLToPath as toPath } from 'url';

function createRequire(metaUrl) {
return createRequireFromPath(toPath(metaUrl));
}
import { createRequire } from 'module';

const require = createRequire(import.meta.url);
const common = require('./index.js');
Expand Down
1 change: 0 additions & 1 deletion test/message/async_error_sync_esm.out
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,3 @@ Error: test
at async three (*fixtures*async-error.js:20:3)
at async four (*fixtures*async-error.js:24:3)
at async main (*message*async_error_sync_esm.mjs:7:5)
(node:*) [DEP0130] DeprecationWarning: Module.createRequireFromPath() is deprecated. Use Module.createRequire() instead.
9 changes: 9 additions & 0 deletions test/parallel/test-global-buffer-dep.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// Flags: --experimental-modules
import { expectWarning } from '../common/index.mjs';

global.Buffer;

expectWarning('DeprecationWarning',
'global.Buffer is deprecated in ECMAScript Modules. ' +
'Please use `import { Buffer } from \'buffer\'` instead.',
'DEP0YYY');
9 changes: 9 additions & 0 deletions test/parallel/test-global-process-dep-eval.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// Flags: --experimental-modules
import { expectWarning } from '../common/index.mjs';

(0, eval)('process');

expectWarning('DeprecationWarning',
'global.process is deprecated in ECMAScript Modules. ' +
'Please use `import process from \'process\'` instead.',
'DEP0XXX');
9 changes: 9 additions & 0 deletions test/parallel/test-global-process-dep-nested-eval.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// Flags: --experimental-modules
import { expectWarning } from '../common/index.mjs';

new Function('return new Function("eval(process)")')()();

expectWarning('DeprecationWarning',
'global.process is deprecated in ECMAScript Modules. ' +
'Please use `import process from \'process\'` instead.',
'DEP0XXX');
9 changes: 9 additions & 0 deletions test/parallel/test-global-process-dep.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// Flags: --experimental-modules
import { expectWarning } from '../common/index.mjs';

process;

expectWarning('DeprecationWarning',
'global.process is deprecated in ECMAScript Modules. ' +
'Please use `import process from \'process\'` instead.',
'DEP0XXX');