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

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
10 participants
@guybedford
Copy link
Contributor

guybedford commented Feb 27, 2019

This PR adds the process and Buffer globals into the CommonJS wrapper context using the new compileFunction approach.

This allows us to then deprecate the global.process and global.Buffer access in ECMAScript modules, while ensuring they continue to behave fine for all CommonJS modules and contexts providing comprehensive backwards compatibility.

The benefits of this are that then ECMAScript modules don't by default have to assume access to all the root-level-security functions and properties on process allowing access control to be added in future, as well as helping towards browser compatibility by making process an import.

ECMAScript modules share the same realm global, so there isn't a way to do this otherwise. In addition once users start running and writing and publishing ECMAScript modules in Node.js that assume the process and Buffer globals exist, it becomes very difficult to change this then.

For these reasons I think it is quite important to land this with the ECMAScript modules implementation in Node.js to provide these properties, or risk never being able to provide them at all in Node.js due to ecosystem compatibility constraints.

Previous discussion: nodejs/modules#235.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
@nodejs-github-bot

This comment has been minimized.

Copy link

nodejs-github-bot commented Feb 27, 2019

@guybedford sadly an error occured when I tried to trigger a build :(

@guybedford guybedford force-pushed the guybedford:module-globals branch 3 times, most recently from 4b94aef to 10e7988 Feb 27, 2019

@devsnek

This comment has been minimized.

Copy link
Member

devsnek commented Feb 27, 2019

can you undo all the unrelated changes to context.js?

@ljharb

This comment has been minimized.

Copy link
Contributor

ljharb commented Feb 27, 2019

While Buffer is fine, process is necessary for environment detection, and will continue to be necessary in ESM. I don’t see what the benefit is of denying it as a global there.

@ljharb

This comment has been minimized.

Copy link
Contributor

ljharb commented Feb 27, 2019

In other words, this PR provides a valid technical means to remove these globals from ESM, but it will still create a world where many kinds of CJS modules will be unable to be refactored to ESM, or will break if they are naively refactored (ie, will silently start going down browser code paths, because the use of typeof won’t trigger any deprecation warnings)

@guybedford

This comment has been minimized.

Copy link
Contributor Author

guybedford commented Feb 27, 2019

While Buffer is fine, process is necessary for environment detection, and will continue to be necessary in ESM.

import.meta.NODE_ENV or similar could be provided as an alternative for environment detections.

I don’t see what the benefit is of denying it as a global there.

process.binding provides access to all native bindings. process.hrtime provides high resolution timers. process provides a hugely complex interface without any encapsulation or access control. Including it by default in ES Modules means living with those design decisions for the rest of Node.js's lifetime. If that is what we want then sure, but at least it should be an explicit decision and not something that just happens by default.

@joyeecheung

This comment has been minimized.

Copy link
Member

joyeecheung commented Feb 27, 2019

Process-related: if we are going to do this, we need to doc-deprecate first before emitting warnings.

@ljharb

This comment has been minimized.

Copy link
Contributor

ljharb commented Feb 27, 2019

Using import.meta (would that work in CJS too? It’s important imo that both cjs and ESM have the same mechanisms available to them) would force a build tool on what was a simple typeof check.

It sounds like what you really want is to deprecate process.binding and process.hrtime, and perhaps some others - can we focus on deprecating those, instead of throwing the baby out with the bathwater by getting rid of the entire useful global?

@guybedford

This comment has been minimized.

Copy link
Contributor Author

guybedford commented Feb 27, 2019

It sounds like what you really want is to deprecate process.binding and process.hrtime, and perhaps some others - can we focus on deprecating those, instead of throwing the baby out with the bathwater by getting rid of the entire useful global?

"Fixing" process to be a secure global is what we'd be left with otherwise certainly. It would also mean individually replacing all of these APIs with alternatives, so we'd be looking at dozens of individual API changes instead. It won't be pretty, but yes that is the alternative to work towards these properties.

The other side of that is that we wouldn't want ES module users to use process.binding to load native modules as if that gets traction in the ecosystem then we can never remove it.

@devsnek

This comment has been minimized.

Copy link
Member

devsnek commented Feb 27, 2019

fwiw process.binding is being deprecated anyway.

@guybedford

This comment has been minimized.

Copy link
Contributor Author

guybedford commented Feb 27, 2019

@devsnek do you have a link? What is it being replaced with?

@devsnek

This comment has been minimized.

Copy link
Member

devsnek commented Feb 27, 2019

#22160

it's not being replaced with anything

@guybedford

This comment has been minimized.

Copy link
Contributor Author

guybedford commented Feb 27, 2019

@devsnek interesting - that is important to know as we then effectively have no means of loading binaries from within ECMAScript modules, without using createRequireFromPath. Edit: mixed this up with process.dlopen!

Note also what a huge effort changing process.binding is. Try multiplying that by dozens to understand what it would mean to make process safe. ES Modules give us a chance to encapsulate. It's certainly not the end of the world if we can't, but it's an opportunity we have that I think is important to consider carefully.

@zenparsing

This comment has been minimized.

Copy link
Contributor

zenparsing commented Feb 27, 2019

A couple of questions that come to mind:

  • How does requiring an import versus using a global property help to create an attenuated privelige environment? It seems to be equivalent at first glance.
  • Does removing things from the global object actually make code more cross-platform? There are tons of platform APIs on the web that are exposed via global properties and I see no effort to move all web APIs to modules.
  • Can not sandboxes/realms be used to create the attenuated environment that you are interested in? Won't you have to use sandboxes anyway in order to intercede on imports?

Thanks!

@bmeck

This comment has been minimized.

Copy link
Member

bmeck commented Feb 27, 2019

@zenparsing

  • How does requiring an import versus using a global property help to create an attenuated privelige environment? It seems to be equivalent at first glance.

Different hooks for attenuation / different analysis of dynamic access. Dynamic global access is quite hard to check for and you get lots of bailouts as can be seen in some tools for auditing like https://github.com/bmeck/tofu .

  • Does removing things from the global object actually make code more cross-platform? There are tons of platform APIs on the web that are exposed via global properties and I see no effort to move all web APIs to modules.

Yes and no; by moving it to an import, it still allows it to be polyfilled via means like import hooks or redirection. It however does help show when something is used that is not present in all environments more easily per the point above.

  • Can not sandboxes/realms be used to create the attenuated environment that you are interested in? Won't you have to use sandboxes anyway in order to intercede on imports?

You don't need sandboxes for imports if you have import hooks. You do need sandboxes for attenuating globals (which is also complex once you start talking about shared intrinsics). Realms are not going to be ready anytime soon for this that I can tell. The underpinnings to even allow Realms as often talked about is still somewhat up in the air per tc39/ecma262#1420

Overall I don't think we should equate global attenuation with import attenuation.

@guybedford

This comment has been minimized.

Copy link
Contributor Author

guybedford commented Feb 28, 2019

Process-related: if we are going to do this, we need to doc-deprecate first before emitting warnings.

Ok I will update along these lines. @joyeecheung does that mean the deprecation warnings can be added as a semver minor?

@guybedford guybedford force-pushed the guybedford:module-globals branch 5 times, most recently from 63618d0 to 8968f53 Feb 28, 2019

@guybedford guybedford force-pushed the guybedford:module-globals branch from 8968f53 to 0f7e94f Mar 1, 2019

@guybedford guybedford force-pushed the guybedford:module-globals branch 2 times, most recently from b609a9b to 8caca93 Mar 1, 2019

@jdalton

This comment has been minimized.

Copy link
Member

jdalton commented Mar 1, 2019

Why is this PR opened here instead of https://github.com/nodejs/ecmascript-modules ?

@guybedford

This comment has been minimized.

Copy link
Contributor Author

guybedford commented Mar 1, 2019

I've updated the approach here to no longer deprecate global.process and global.Buffer access in CommonJS. The deprecation path is now entirely constrained to the --experimental-modules flag behaviours at this point.

@devsnek I've removed all parts of the code touching context and VM, and dismissed your previous review as resolved on this.

VM components fixed, and this no longer deprecates global access in CJS.

@guybedford

This comment has been minimized.

Copy link
Contributor Author

guybedford commented Mar 1, 2019

@jdalton the tracking issue on nodejs/modules is nodejs/modules#235, where it was advised to take this to core for approval.

@ljharb
Copy link
Contributor

ljharb left a comment

Now we’ve got a larger difference in environment between ESM and CJS - and any CJS module can still expose these values indirectly to ESM. What value does this provide?

Show resolved Hide resolved doc/api/deprecations.md Outdated

@guybedford guybedford force-pushed the guybedford:module-globals branch from 8caca93 to 1a40061 Mar 1, 2019

@jdalton
Copy link
Member

jdalton left a comment

I'm 👎 on this for now. This feels like more than a bug fix and moves into Module WG territory. I think we should get consensus on it and apply to the appropriate repo/fork. There's already some -1 inclinations from WG members so continuing the discussion as an agenda item might be a good action before continuing with this PR.

@guybedford

This comment has been minimized.

Copy link
Contributor Author

guybedford commented Mar 1, 2019

@ljharb in CJS global.process and global.Buffer are still available fine, and typeof process checks still work.

In terms of the motivation here, it's about the fact that process being assumed in all modules leads to an ecosystem where we can never ever remove the process global. Module-level security might be introduced in future based on resolver permissions - only certain modules having access to resolve import 'process'. These possibilities are only possible in Node.js in future if we can deprecate these globals now. Otherwise only projects other than Node.js will be capable of these kinds of security properties in future.

@bmeck

This comment has been minimized.

Copy link
Member

bmeck commented Mar 1, 2019

@ljharb we have a few things listed in #26334 (comment) as well as concerns about lack of ability to analyze and/or encapsulate things with the power described in #26334 (comment) . globals have distinctly different level of auditability/encapsulation from globals and can easily be represented by the ability to deny imports to a powerful value such as process at the module level, but not to globals.

@jdalton this was moved to core explicitly because the WG felt it was more of a core concern than that of the modules WG though. We shouldn't thrash on where this is supposed to happen, do you feel that it should never have been moved here / do you feel that the modules WG should expand in scope to cover this? If it does get moved to the modules WG do you think it is relevant to the current work there? I personally think it relates to the global and ability to constrain power APIs more than how modules are loaded.

@ljharb

This comment has been minimized.

Copy link
Contributor

ljharb commented Mar 1, 2019

We already have an ecosystem where we can never remove the process global. ESM isn’t an opportunistic moment where that can/should be changed.

I don’t mind deprecating Buffer in just ESM, because that’s not used for environment detection.

@bmeck it was moved to core partially to deprecate it in CJS as well, which seems to no longer be possible - if a non-module-related feature can’t be deprecated there, i don’t think it should be deprecated anywhere.

@zenparsing

This comment has been minimized.

Copy link
Contributor

zenparsing commented Mar 1, 2019

My 2 cents: Whether or not node exposes priveledged functionality on the global object is an architectural question that encompasses the platform as a whole. It should probably be debated as such rather than as an incremental change in a PR. But with my limited knowledge, I'm not sure where the correct forum for that debate is.

@bmeck

This comment has been minimized.

Copy link
Member

bmeck commented Mar 1, 2019

@bmeck it was moved to core partially to deprecate it in CJS as well, which seems to no longer be possible - if a non-module-related feature can’t be deprecated there, i don’t think it should be deprecated anywhere

This directly relates to the SES/Realms style of encapsulating different views/constraining of globals. Based upon tc39/ecma262#1420 there is some request of people to look at this at the host level before looking at the language level. Even if we cannot constrain it to CJS, that does not mean that ESM necessarily must shared the same view of the global.

CC: @erights @dtribble , it might be good to explain differing views of globals while having same intrinsics using compartments sometime again here.

Show resolved Hide resolved lib/internal/bootstrap/node.js Outdated
Show resolved Hide resolved doc/api/deprecations.md Outdated
'global.Buffer is deprecated in ECMAScript Modules. ' +
'Please use `import { Buffer } from \'buffer\'` instead.',
'DEP0126');

Object.defineProperty(global, 'Buffer', {

This comment has been minimized.

@devsnek

devsnek Mar 1, 2019

Member

this could cause some pretty serious performance regressions if people are generally used to accessing Buffer without overhead. are there some benchmarks we can run to check if this is okay on that front?

This comment has been minimized.

@guybedford

guybedford Mar 1, 2019

Author Contributor

Note that only global.Buffer is affected here, not Buffer. global.Buffer access in a hotpath seems unlikely (most people do just Buffer), but it could be worth verifying that certainly.

This comment has been minimized.

@ljharb

ljharb Mar 1, 2019

Contributor

Unless the global Buffer is shadowed per module, a bare global variable reference is supposed to exercise a getter. Is it shadowed?

This comment has been minimized.

@guybedford

guybedford Mar 3, 2019

Author Contributor

Yes we shadow the global reference in CommonJS with the direct Buffer and process references, with the shadowed reference being synchronized with the global.Buffer and global.process versions. This ensures the fast path in the majority of use cases.

@guybedford guybedford force-pushed the guybedford:module-globals branch from 1a40061 to 8740544 Mar 3, 2019

@guybedford guybedford requested a review from joyeecheung Mar 3, 2019

@guybedford guybedford force-pushed the guybedford:module-globals branch from 8740544 to cbd338b Mar 3, 2019

@guybedford

This comment has been minimized.

Copy link
Contributor Author

guybedford commented Mar 3, 2019

Is this still semver major if it is only affecting ESM at this point? The only effect on non-module code is having global.Buffer / global.process now being getters and setters instead of value properties.

It would help a lot to know if such a change would be deemed major as then this PR needs to be prioritized by the modules group now before the current major.

@guybedford guybedford force-pushed the guybedford:module-globals branch from cbd338b to add74bc Mar 3, 2019

const processGetter = deprecate(
() => cjsContext.process,
'global.process is deprecated in ECMAScript Modules. ' +
'Please use `import process from \'process\'` instead.');

This comment has been minimized.

@BridgeAR

BridgeAR Mar 5, 2019

Member

Please add a deprecation code. This is done for all deprecations so far.


setImmediate(() => {
assert.strictEqual(sawWarning, true);
});

This comment has been minimized.

@BridgeAR

BridgeAR Mar 5, 2019

Member

Please use common.expectWarning() instead of the manual handling here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.