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

require fallthrough behavior #11675

Closed
bmeck opened this issue Mar 3, 2017 · 11 comments
Closed

require fallthrough behavior #11675

bmeck opened this issue Mar 3, 2017 · 11 comments
Labels
module Issues and PRs related to the module subsystem.

Comments

@bmeck
Copy link
Member

bmeck commented Mar 3, 2017

  • Version: all
  • Platform: all
  • Subsystem: module

require() has the ability to "fall through" multiple directories while searching for files.

https://github.com/bmeck/node-require-fallthrough-example/blob/master/nested/index.js

Given a dir structure of:

root
 \- node_modules/foo/package.json
 \- node_modules/foo/root.js
 \- child
     \- node_modules/foo/package.json
     \- node_modules/foo/child.js
     \- dependent.js

dependent.js has a surprising behavior of

require.resolve('foo/child'); // root/child/node_modules/child.js
require.resolve('foo/root'); // root/node_modules/root.js

If any error in resolving the "main" of child occurs, the main of root is used.

EPERM on child also has fallthrough behavior.

This means that you can place directories in parent node_modules to intercept requests for resources within a package. This should be discussed WRT signing packages and if we can remove this behavior.

The main concern here is breakage vs falling through a potentially secure context into an unsigned context and vice versa.

@bmeck bmeck added module Issues and PRs related to the module subsystem. notable-change PRs with changes that should be highlighted in changelogs. labels Mar 3, 2017
@mscdex mscdex removed the notable-change PRs with changes that should be highlighted in changelogs. label Mar 3, 2017
@Slayer95
Copy link

Slayer95 commented Mar 4, 2017

This behavior is the actual premise of npm@3 and npm dedupe...

Prior discussion:
nodejs/node-v0.x-archive#8830
#176

@richardlau
Copy link
Member

This should be discussed WRT signing packages and if we can remove this behavior.

Completely or just for signed packages?

@bmeck
Copy link
Member Author

bmeck commented Mar 4, 2017

@richardlau completely, but in particular this is an actual problem for signing.

@Slayer95 not exactly, this is about intentional injection of files within a package's namespace, not intercepting the name of a package. If you look at the example and my comment the focus here differs since it is essentially preventing injection of package/${inner_file} from having a different package when required/imported from the same file. My suggestion here is that node should ensure that when you make package:

  • no parent node_modules can intercept loading operations that miss, and no
  • package will not see any loading operations that would be in the same namespace as a child node_module

This is not like npm depupe as the intention is wrt files within packages, not preventing packages with the same name.

@richardlau
Copy link
Member

@bmeck Can you point to any information re. signing packages? I wrote the following but suspect I'm probably missing some context.

What is a package? As far as Node.js is concerned (https://nodejs.org/dist/latest-v7.x/docs/api/modules.html#modules_modules):

In Node.js, files and modules are in one-to-one correspondence (each file is treated as a separate module).

Which is to say that currently Node.js has no notion of namespaces. One might assume a package is defined by package.json, but at the moment Node.js only reads the main field from it: https://nodejs.org/dist/latest-v7.x/docs/api/modules.html#modules_all_together

@bmeck
Copy link
Member Author

bmeck commented Mar 4, 2017

@richardlau correct, currently this is a bit ambiguous in core; my use of the term mostly comes from npm not core here where a "package" is a directory containing a package.json (technically not entirely true as you can sneakily create things in the registry without a package.json )

As per signing, I am looking at various approaches but am leaning heavily towards https://github.com/dimich-g/webpackage . This is what I will be tackling after ESM.

I bring up this issue now since it relates to the ESM import resolution algorithm.

@Trott
Copy link
Member

Trott commented Jul 30, 2017

@bmeck This is still a thing that needs addressing and should remain open? Is there anything anyone who is not you could and should be doing on this at this time?

@bmeck
Copy link
Member Author

bmeck commented Jul 30, 2017 via email

@refack
Copy link
Contributor

refack commented Jul 30, 2017

I think we can close, cases in wild were found, so unsafe to remove.
Warning might be good though

Maybe consider as a breaking change only WRT to signed packages, that is - signed packages will not resolve with fall through?

@TimothyGu
Copy link
Member

@refack Are signed packages a thing yet?

@refack
Copy link
Contributor

refack commented Jul 31, 2017

@refack Are signed packages a thing yet?

I believe they have been cooking In @bmeck brains for a while (but AFAIK only there).

@TimothyGu
Copy link
Member

Closing per @bmeck.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module Issues and PRs related to the module subsystem.
Projects
None yet
Development

No branches or pull requests

7 participants