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

lib: promote process.binding/_tickCallback to runtime deprecation #50687

Closed
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
11 changes: 11 additions & 0 deletions doc/api/cli.md
Original file line number Diff line number Diff line change
Expand Up @@ -432,6 +432,12 @@ added: v12.0.0

Specify the file name of the CPU profile generated by `--cpu-prof`.

### `--deprecated-process-binding`

Ensure that the deprecated `process.binding(...)` API is available. This
is currently the default. A runtime deprecation warning, however, will be
emitted when `process.binding(...)` is accessed.

### `--diagnostic-dir=directory`

Set the directory to which all diagnostic output files are written.
Expand Down Expand Up @@ -1172,6 +1178,10 @@ Disable the `node-addons` exports condition as well as disable loading
native addons. When `--no-addons` is specified, calling `process.dlopen` or
requiring a native C++ addon will fail and throw an exception.

### `--no-deprecated-process-binding`

Makes the deprecated `process.binding(...)` API unavailable in the process.

### `--no-deprecation`

<!-- YAML
Expand Down Expand Up @@ -2325,6 +2335,7 @@ Node.js options that are allowed are:
* `--allow-fs-write`
* `--allow-worker`
* `--conditions`, `-C`
* `--deprecated-process-binding`
* `--diagnostic-dir`
* `--disable-proto`
* `--dns-result-order`
Expand Down
10 changes: 8 additions & 2 deletions doc/api/deprecations.md
Original file line number Diff line number Diff line change
Expand Up @@ -2203,6 +2203,9 @@ The `produceCachedData` option is deprecated. Use

<!-- YAML
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/50687
description: Upgraded to a runtime deprecation.
- version: v11.12.0
pr-url: https://github.com/nodejs/node/pull/26500
description: Added support for `--pending-deprecation`.
Expand All @@ -2211,7 +2214,7 @@ changes:
description: Documentation-only deprecation.
-->

Type: Documentation-only (supports [`--pending-deprecation`][])
Type: Runtime

`process.binding()` is for use by Node.js internal code only.

Expand Down Expand Up @@ -2604,12 +2607,15 @@ Prefer [`response.socket`][] over [`response.connection`][] and

<!-- YAML
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/50687
description: Upgraded to a runtime deprecation.
- version: v12.12.0
pr-url: https://github.com/nodejs/node/pull/29781
description: Documentation-only deprecation.
-->

Type: Documentation-only (supports [`--pending-deprecation`][])
Type: Runtime

The `process._tickCallback` property was never documented as
an officially supported API.
Expand Down
4 changes: 3 additions & 1 deletion lib/internal/bootstrap/realm.js
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,6 @@ ObjectDefineProperty(process, 'moduleLoadList', {
writable: false,
});


// processBindingAllowList contains the name of bindings that are allowed
// for access via process.binding(). This is used to provide a transition
// path for modules that are being moved over to internalBinding.
Expand Down Expand Up @@ -144,6 +143,9 @@ const experimentalModuleList = new SafeSet();
{
const bindingObj = { __proto__: null };

// TODO(@jasnell): If the --deprecated-process-binding option is false we really
Copy link
Member

Choose a reason for hiding this comment

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

You can just expose the processBindingAllowList list, which is set to empty initially, and add things back in pre-execution. We do something similar for --expose-internals for example.

// ought to skip setting this here. However, we cannot require internal/options or
// use internalBinding here so we'll have to pull it back out later.
process.binding = function binding(module) {
module = String(module);
// Deprecated specific process.binding() modules, but not all, allow
Expand Down
12 changes: 9 additions & 3 deletions lib/internal/process/policy.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,10 @@ const {
ERR_MANIFEST_TDZ,
} = require('internal/errors').codes;
const { Manifest } = require('internal/policy/manifest');

const { getOptionValue } = require('internal/options');
const deprecatedProcessBinding = getOptionValue('--deprecated-process-binding');

let manifest;
let manifestSrc;
let manifestURL;
Expand All @@ -36,9 +40,11 @@ module.exports = ObjectFreeze({

// process.binding() is deprecated (DEP0111) and trivially allows bypassing
// policies, so if policies are enabled, make this API unavailable.
process.binding = function binding(_module) {
throw new ERR_ACCESS_DENIED('process.binding');
};
if (deprecatedProcessBinding) {
process.binding = function binding(_module) {
throw new ERR_ACCESS_DENIED('process.binding');
};
}
process._linkedBinding = function _linkedBinding(_module) {
throw new ERR_ACCESS_DENIED('process._linkedBinding');
};
Expand Down
23 changes: 15 additions & 8 deletions lib/internal/process/pre_execution.js
Original file line number Diff line number Diff line change
Expand Up @@ -545,15 +545,20 @@ function initializeDeprecations() {
});
}

if (pendingDeprecation) {
if (getOptionValue('--deprecated-process-binding')) {
process.binding = deprecate(process.binding,
'process.binding() is deprecated. ' +
'Please use public APIs instead.', 'DEP0111');

process._tickCallback = deprecate(process._tickCallback,
'process._tickCallback() is deprecated',
'DEP0134');
} else {
// Ideally it wouldn't be defined at all already but realm.js does not
// provide a means of checking to see if the option is enabled or not,
// so we do it here.
delete process.binding;
Copy link
Member

Choose a reason for hiding this comment

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

This should at least throw a warning and tell users about the option --deprecated-process-binding.

}

process._tickCallback = deprecate(process._tickCallback,
'process._tickCallback() is deprecated',
'DEP0134');
}

function setupChildProcessIpcChannel() {
Expand Down Expand Up @@ -587,9 +592,11 @@ function initializeClusterIPC() {
function initializePermission() {
const experimentalPermission = getOptionValue('--experimental-permission');
if (experimentalPermission) {
process.binding = function binding(_module) {
throw new ERR_ACCESS_DENIED('process.binding');
};
if (getOptionValue('--deprecated-process-binding') && process.binding !== undefined) {
process.binding = function binding(_module) {
throw new ERR_ACCESS_DENIED('process.binding');
};
}
process.emitWarning('Permission is an experimental feature',
'ExperimentalWarning');
const { has, deny } = require('internal/process/permission');
Expand Down
5 changes: 5 additions & 0 deletions src/node_options.cc
Original file line number Diff line number Diff line change
Expand Up @@ -535,6 +535,11 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() {
"emit pending deprecation warnings",
&EnvironmentOptions::pending_deprecation,
kAllowedInEnvvar);
AddOption("--deprecated-process-binding",
"enable the deprecated process.binding(...) api",
&EnvironmentOptions::deprecated_process_binding,
kAllowedInEnvvar,
true);
AddOption("--preserve-symlinks",
"preserve symbolic links when resolving",
&EnvironmentOptions::preserve_symlinks,
Expand Down
10 changes: 10 additions & 0 deletions src/node_options.h
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,16 @@ class EnvironmentOptions : public Options {
false;
#endif // DEBUG

// TODO(@jasnell): In preparation for the eventual complete removal of
// the deprecated process.binding, this option controls whether or not
// process.binding is available. Currently the default is true, making
// the deprecated API available, albeit with a runtime deprecation message.
// Then, later, we can change the default to false, giving folks the option
// to re-enable if they really need it. Then, as the final step, we would
// remove the deprecated api entirely, in which case this option becomes
// a non-op.
bool deprecated_process_binding = true;

bool watch_mode = false;
bool watch_mode_report_to_parent = false;
bool watch_mode_preserve_output = false;
Expand Down
5 changes: 5 additions & 0 deletions test/parallel/test-no-deprecated-process-binding.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
'use strict';
// Flags: --no-deprecated-process-binding
require('../common');
const { strictEqual } = require('node:assert');
strictEqual(process.binding, undefined);