Repository navigation
Validate exports keys and values #359
Description
Activity
Previous discussion: hybrist/proposal-pkg-exports#6
There's 3 different "data types" at work:
- (Bare)
specifier, maybe the concatenation ofnameandexpansion. - The
pattern(or key). - The
target(or value). - The
resolvedURL.
An approach that seems consistent with how import map validation works could be:
- optional Normalize
specifier, e.g. by collapsing directory separators or converting\\into/.
This step is mostly relevant to CJS and Windows where we have to support certain existing specifier formats. - Parse
specifierintonameandexpansion. Validatenameandexpansion. - Match
expansionto apattern, resulting in atarget. Validatetarget. - Apply the
pattern, yielding aresolvedURL. ValidateresolvedURL.
The nature of the validation (and what a failed validation means) is TBD here.
- (Bare)
Known possible
targetvalues:string, relative URL starting with./.string, relative URL starting with../. Current consensus: Disallowed. May throw?string, relative URL starting with/. Current consensus: Disallowed. May throw?string, absolute URL with a supported scheme.string, absolute URL with an unsupported scheme.array, each element a non-arrayand non-nullvalue.null, alias for[].boolean. TBD.number. TBD.object. TBD.
For
patternvalues, there are few restrictions. Unrecognized patterns would just not do anything helpful. In a way the definition of a usefulpatternvalue is "anything that could be matched by a validexpansion". The only restriction is:targetandpatternhave to either both end in a slash ("/") or both not end in a slash ("/").If we want to prevent easy loopholes, especially on Windows, we would have to do some limited normalization on
specifiervalues before determining thename. We can protect theexportsin the following cases, specifically when thespecifieris coming from CJS:non-exports-pkg/../exports-pkg/foo.js(path traversal loophole)exports-pkg\foo.js(Windows loophole)@scope\name/foo.js(Windows loophole, scope edition)
In ESM we'd reject those as invalid specifiers, so the issue doesn't exist. But in CJS we generally fall back on the old resolution when no
exportsare found.Why is escaping the current package boundary a "loophole"?
It's a "loophole" as in "you're still using bare specifiers but you circumvent exports". But I scaled down the language to call out that preventing those circumventions is optional.
if it is a concern couldn't you negotiate whenever you cross into a package boundary? would be a lot of IO though.
Yeah, it would also muddy the waters of what
exportsare. Because right now they don't really apply to "directories" but to "package names". Since this is only an issue in CJS (where we have to support old behavior), I'd rather keep it simple. E.g. "we accept that it's possible" or "we normalize and prevent these known patterns".How does this not have the same problem in ESM?
I thought somebody (@guybedford?) mentioned that we'd throw a
ERR_INVALID_MODULE_SPECIFIERfor those kinds of specifiers but I can't find any mention of it now. :( So I guess we still have to define behavior for it, either accepting the same kinds of issues as with CJS or making those illegal in ESM (e.g...segments and backslashes).EDIT: The only thing we do reject is backslashes in scopes (
Invalid package name '@foo\bar').@jkrems of note,
|is also a character that seems to be in conflict with import maps/layered API fallback strings@jkrems the conflict actually comes from https://github.com/drufball/layered-apis#solution but the specifiers from layered APIs would be split on
|prior to being import mapped13 remaining items
There seem to be three main phases we can apply validations:
- Validate the exports target before applying it, eg to ensure it has no
/../segments, or percent-encoded/../segments (which only get discovered moving from URL -> path, and wouldn't necessarily be caught at the URL restriction phase! - Validate the exports subpath the user themselves provide - similar to the validation above, but instead applied to the user-supplied subpath after the
pkg/...part. - Validate the final resolved URL to ensure it is within the expected package boundary. The issue here is that it might be difficult to debug, as we don't necessarily know whether it was the user-supplied part, or the "exports" part that resulted in the backtracking - maybe we can carefully design the error message to handle both. In addition, percent-encoded separators would still slip through to the file system here unless we explicitly have a phase to detect that too.
Since we know this is not a secure system - I'm tempted to say the simplest approach should win... instead of trying to comprehensively tackle every scenario.
The absolute simplest seems to me just to do nothing and have URL resolution, as @jkrems says no one has complained about this for
"main".But if we must introduce validation, please lets find a simple model for it with good errors, so we maintain a good developer experience.
Reacted by Jordan Harband- Validate the exports target before applying it, eg to ensure it has no
As others have said, we're not aiming for a security boundary here. This is about preventing bugs and conveying intent (that this feature is to be used to deliver portable package encapsulation).
For @guybedford 's list, (1) is the most valuable validation because there is no known use-case for losing portability.
(2) is also valuable - not least because it would prevent any user confusion over whether back-tracking in a sub-path is resolved before or after pattern-matching. If a pkg is using exports, it means that importers ought to entering via well-defined sub-paths.
Reacted by Jordan HarbandIf we do apply these validation rules, we should expect to be able to relax them for things if we find use cases around things outside of the validation rules. This could also be linted ahead of time it seems and we don't apply the same restrictions to things like
fs. Is the frequency of these specifiers resolving outside of the package common / in error? Or is there some use case?I would definitely prefer we spend time researching things here rather than banning outright to begin with.
Per (1), the path could still be escaped using symlinks or the like. I don't think
..is a reliable means to know if something escapes the path boundary. I do think since (2) is a request it could be applied after normalizing, but think that..should be normalized before validation.Overall, I think adding these restrictions adds a lot of complexity for validations that I don't see users being able to reason about well on their own. I would prefer we leave this to linters given the complexity of understanding the restrictions, lack of data, and potential need to remove the errors in the future as use cases like loading different protocols is exposed.
I think after reading these comments, I am +1 to some naive validation (properly formatted strings), but -1 to complex package boundary enforcement.
fs isn’t package-related; the lack of these validations in main/bin has caused tons of bugs in the ecosystem over the years - it would be a shame to miss the opportunity to maximally validate. Anything correctness-related (as opposed to style-related) that the language or platform leaves to linters is imo also a failure, as it passes burden to a large number of downstream humans.
I think normalizing before validating is the proper approach; as long as the error message indicates which exports mapping is causing the problem it’ll be very useful.
@ljharb I'm unsure that these restrictions are correctness. What examples do you have of this in the ecosystem? Using linters alleviates the usefulness of doing things across package boundaries and runtime costs in both CPU and complexity. These fields are statically analyzable and do not have the complexity of JS linting even. I am just unsure of why we are applying these restrictions without clear reasoning. It seems an effort to contain packages within themselves, but packages can cross boundaries in all sorts of ways, not just to outer paths, but to inner packages as well. The validation seems arbitrary without evidence of common mistakes and reasons for people using code outside of their own package.
I have no concrete examples available; but my anecdotal data is that these sort of bugs do happen - in main occasionally, but more often in the browser field which is the closest analogue to exports.
We can always relax restrictions later; we can never tighten them. Nows the time to start with them because we have no other opportunity.
That there are other ways to cause problems doesn’t mean we have to let this abstraction be a leaky sieve too.
It seems fine if we are doing this purely precautionary but we should probably try to lay out the goals a bit clearer.
Are we trying to prevent any package boundary crossing ?
- Inner packages can be access via things like
./node_modules/dep - Outer packages can be access via things like
../peerpeerDependencies?
- Vendored packages with a
package.jsonbundledDependencies?
- Absolute specifiers?
- If they resolve inside the current package ?
- These are not altered across installations ? Is this really about problems with installation graphs or about package boundaries in general ?
At what points are validation occuring:
- Specifiers prior to resolution
import("foo/../bar")"foo/../bar" - While doing path traversal?
- If
pkg's main exports../peerbut../peerredirects topkg/innerwhat happens since we still end up in the current package ? Same forpeerinstead of../peer- Can these cause infinite loops ?
- Symlinks behavior around
--preserve-symlinksbeing on or off ?
- If
Is this all done eagerly when searching for
package.jsonor lazily upon importing a specificexportskey? For most things I think lazy is good as it prevents slow startup, especially if it can be analyzed ahead of time.If we can reach consensus on points like those above it seems fine since we can design the validation around the goals rather than ad-hoc.
- Inner packages can be access via things like
It does seem that some frameworks at least are using
../to escape their package boundaries on purpose such aspolymer@3.- import the Polymer library from
./node_modules/@polymer/polymer/polymer-element.jsin apps, and from../@polymer/polymer/polymer-element.jsin reusable elements. Now, apps and reusable elements can both import from@polymer/polymer/polymer-element.js.
- import the Polymer library from
Just because I had to do a double-take in shock: It looks like polymer did stop asking people to do that in v3 but yes, the implication is that before then people did routinely reach out to files outside of package boundaries.
To be clear, I'm fine banning w/e behavior we can reach consensus on even if there are some uses of it; however, we should do some research on usages.
Shall we add this as an agenda item to the meeting then?
Reacted by Jordan Harband and Jan Olaf MartinA set of validations has now landed in the implementation via nodejs/node#28949.
We currently do not put any restrictions on keys and values beyond "whatever works in the current implementation". There should be a clear, non-ambiguous definition what happens for every possible key and value.
Worth noting that import maps explicitly treat the
patternandspecifiervalues as arbitrary data. They are not restricted to valid filesystem or URL characters: