Node: Handle windows-style bare specifiers in exports

[jkrems]

There's currently a loophole in CJS exports:

require('x\\y.js');

The above will load y.js at least on Windows even if x uses exports and didn't whitelist the path. This happens because the specifier isn't recognized as an exports specifier.

There's three possible ways out:

1. We detect this case and iff x uses exports treat it as "not mapped", no matter what the subpath is.
2. We accept this as a loophole.
3. We normalize the specifier, turning backslashes into slashes.

There are other potential loopholes like the following:

• @scope/../any/file.js: Doesn't match the RegExp, allows access to any/file.js.
• TBD

It's not quite clear if we can plug all holes in CJS but there's likely some way of doing it.

[ljharb]

1 or 3 sounds good to me.

[zackschuster]

i think normalizing the specifier to the platform would be in the realm of "expected behavior"?

[jkrems]

Yes-ish. The problem is that we're crossing streams. It's the same configuration for both ESM and CJS. ESM operates on file URLs so "normalizing" means converting those to platform file system paths. CJS operates on file system paths so "normalizing" means first mapping relative path segments to URL segments, then mapping, and then mapping the result back to file system paths. The question is less "should we normalize" and more "what do we consider our input/output formats to be" so we can normalize correctly.

[bmeck]

1. is the general approach wrt some things like case insensitive filesystems using require(). Seems fine.

As long as it is documented, I don't have a preference on solution chosen.

If we do go with 3. however I would suggest the following types and coercions:

1. exports key is a relative URL string with leading . or .., or absolute URL string
2. exports value is a relative URL string string with leading . or .., or absolute URL string

Conversion happens lazily like most things. People can use auditing like linters to do ahead of time checks.

The URLs are resolved against the location of the package.json file using the regular URL parser:

// will ensure trailing / and remove fragment and query
// don't need this if you aren't checking location after resolve
pkgRoot = new URL('.', fileURLOfPackageJSON);
resolvedExportValue = new URL(value, pkgRoot);

// could do a pathname check to see if it stayed in the pkgRoot?
// idk, seems more of an auditing problem than a need to prevent this behavior

// only used by CJS
// this strips query/fragment but CJS doesn't support those to begin with
filename = fileURLToPath(resolvedExportValue);

fileURLToPath handles some minor oddities when converting between things and new URL handles \\ normalization in a standardized way as seen with new URL('.\\x\\y', 'file:////app/').

[jkrems]

My current thinking is that we should officially narrow the scope of exports to what it is right now: specifiers that are a package name or a package name followed by a slash.

This will mean that code can "exploit" backslash on Windows (under require) and code can "exploit" case-differences on case-insensitive file systems. But portable code will be using exports. Since we can't close the more general escape hatch (case-insensitive file systems), it feels weird to add complexity to close a very platform specific one. I don't think I've seen backslash widely used in imports or requires.

EDIT: Fixed some grammar.

[ljharb]

By “specifiers that are a package name” you mean, exports only “kicks in” when the resolved specifier started with a bare specifier?

[jkrems]

Yep. Or to be more precise: when it's a "package name specifier" which is a subset of all bare specifiers. E.g. \0%$! is a bare specifier but not something that would trigger exports (a "package name").

[MylesBorins]

Since this type of specifier resolution would affect both CJS and ESM would
it make sense for us to transfer this issue to nodejs/node?

On Tue, Oct 29, 2019 at 1:27 PM Jan Olaf Krems [e-mail]
wrote:

Yep. Or to be more precise: when it's a "package name specifier" which is
a subset of all bare specifiers. E.g. \0%$! is a bare specifier but not
something that would trigger exports (a "package name").

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/nodejs/modules/issues/356?email_source=notifications&email_token=AADZYV24EHWZ2Q5YCUGKYUDQRBXATA5CNFSM4IGGAUN2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECRMQZA#issuecomment-547539044,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AADZYV72ROVWTTQGTXVSQ2DQRBXATANCNFSM4IGGAUNQ
.

[docmaster2]

одно спотыкание и мы