require fallthrough behavior

[bmeck]

• 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.

[Slayer95]

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

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

[richardlau]

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

Completely or just for signed packages?

[bmeck]

@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]

@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]

@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]

@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]

I think we can close, cases in wild were found, so unsafe to remove. Warning might be good though _{refack: fixed typo and removed email quoted text}

[refack]

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]

@refack Are signed packages a thing yet?

[refack]

@refack Are signed packages a thing yet?

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

[TimothyGu]

Closing per @bmeck.