WICG / import-maps

How to control the behavior of JavaScript imports
https://html.spec.whatwg.org/multipage/webappapis.html#import-maps
Other
2.68k stars 69 forks source link

Add access check of module URLs #152

Closed hiroshige-g closed 5 years ago

hiroshige-g commented 5 years ago

This commit reject the following URLs as request URLs of module scripts in all cases.

These can't be the output of import maps since before (i.e. the result of resolve an imports match), but were allowed if directly specified in <script> or not mapped by import maps.

This commit also adds a non-normative placeholder for referringScript-dependent access controls, which is helpful for Chromium implementation.

This commit also adds support for the case where referringScript is null in resolve a module specifier.


Preview | Diff

hiroshige-g commented 5 years ago

@domenic, PTAL when you have time. Particularly, is this mechanism sufficient for getOriginals access restriction?

guybedford commented 5 years ago

Does this mean that import maps cannot be used to map non-existent std:[builtin] URLs? That seems overly restrictive in a future where import maps are a baseline and this becomes a polyfilling technique as users would need to perpetually embed valid URLs to polyfill new builtin modules...

hiroshige-g commented 5 years ago

Does this mean that import maps cannot be used to map non-existent std:[builtin] URLs?

Mapping FROM non-existent std:[builtin] URLs: Can be used. We can use non-existent std:[builtin] URLs as import map keys (before and after this PR).

Mapping TO non-existent std:[builtin] URLs: Can't be used. In the existing spec https://wicg.github.io/import-maps/#resolve-an-imports-match there are no paths that can return non-existent std:[builtin] URLs. Access check in this PR is applied to the OUTPUT of import maps, resolve a module specifier, not about INPUT or import map keys. (Updated PR description)

domenic commented 5 years ago

This commit also adds support for the case where referringScript is null in resolve a module specifier.

This was a good catch, but I would like to land it separately and then put the new stuff here on top of it. I will post a PR shortly that extracts your work and also explains some other spec modifications needed.

domenic commented 5 years ago

Overall this makes sense. I will rebase and rename "check access" to something like "validate module script URL".

Also note that we need to loosen the / restriction on built-ins (#129) especially now that there are proposals like std:elements/switch. But for now we should keep the restriction.

Let me rebase it and stare a bit more, then I might merge or might have more comments.

domenic commented 5 years ago

I spotted another preexisting problem that we should fix before inserting more code here. "resolve an imports match" throws an error but several of its callers assume that resolution failure results in a null return value.

I will work on another PR...

hiroshige-g commented 5 years ago

Er, I was confused during rebasing. Still working on this and other PRs to fix commit chains.

hiroshige-g commented 5 years ago

Also note that we need to loosen the / restriction on built-ins (#129) especially now that there are proposals like std:elements/switch. But for now we should keep the restriction.

BTW I think the checks against URLs/specifiers should be centralized and simplified. Currently we have many kinds of URL/specifier checks in many places, which is hard to grasp for me. (I created a table: https://docs.google.com/document/d/1m4wSpE0KyioTHtYK927RQoaANB_v1gRfyD1jitxmO7s/edit#heading=h.lf4qh3xpzry2 )

domenic commented 5 years ago

I agree with the drive for simplification. I'm unsure whether we should work to simplify before or after merging this patch.

hiroshige-g commented 5 years ago

I think simplification should be done after this PR, to reduce ongoing PRs in parallel.