tc39 / proposal-shadowrealm

ECMAScript Proposal, specs, and reference implementation for Realms
https://tc39.es/proposal-shadowrealm/
1.41k stars 67 forks source link

Should HostImportModuleDynamically provide a referencing script? #363

Closed mgaudet closed 5 months ago

mgaudet commented 2 years ago

Implementing ShadowRealms in Gecko seems to indicate to me that the call to HostImportModuleDynamically should actually not provide null as the referencingScriptOrModule, but actually the executing script that invokes importValue (experimentally, this works in Gecko more like how Shadow Realms module resolution seems like it ought to).

In Gecko, the referencingScriptOrModule is used to determine the imported script's base URL (functionally, expanding ./foo.js to the actual URL to be loaded); if it's left out, we currently default to the base URL of the document, which can be incorrect.

This could just be a matter of requiring compatibility change on the HTML side; The HTML spec says that if you don't provide referencingScriptOrModule, the base URL will be the base URL of the 'current settings object'. As I understand it, that probably should be the settings object of the ShadowRealm's realm; however, that poses a problem because it's never defined anywhere what that settings object ought to contain.

It seems like import and importValue sharing the same referencingScriptOrModule would make sense...

mgaudet commented 2 years ago

WebKit's implementation, if I read it right -- and I'm only about 60% sure -- does seem to pass the caller's source origin to importModule, which seems to control how the baseURL is determined as well.

caridy commented 2 years ago

From previous discussions, my intuition here is to think about r.importValue("/something", "name") as equivalent to <script type="module" src="/something"><script>. This way, you can't not observe that you're being executed inside a ShadowRealm. @rwaldron do we have any tests about this? @erights @mhofman do you have any opinion on this?

leobalter commented 2 years ago

@caridy please see https://github.com/tc39/proposal-shadowrealm/issues/364#issuecomment-1138938121

if we do r.importValue("/something") this way, we may assume importValue won't ever do any sort of serialization of the module namespace, and perhaps this could be featured in some ShadowRealm.prototype.import to return a static namespace representation.

caridy commented 2 years ago

@leobalter I have updated my example, but that's not the important part, I just forgot to add the second argument.

codehag commented 2 years ago

The null referencing script for dynamic import in html occurs in such cases:

<button onclick="import(`my-module.js`)">click me</button>

This way, you can't not observe that you're being executed inside a ShadowRealm.

I am confused by this point, can you clarify? There are other ways to detect if you are in a shadow realm, for example -- missing APIs. From my perspective, the main outcome of this choice would be to make it harder to use from the calling realm if you are importing nested siblings. The referencing script is only used to propagate the script fetch options and the directory we are "currently" in so that the import specifier can be shorter.

Script fetch options aren't exposed beyond fetch behavior, and what this would effectively do is allow a shadow realm to escape a script element's referrer policy, etc. Is that the goal? One might say that this is "detectable" but only at the point of fetch, not as the script is executing.

littledan commented 2 years ago

If I recall correctly, what's at stake here is just how relative paths in the module specifier are resolved. I think at some point we decided to be conservative here and force document-relative paths because importValue is a method rather than a piece of syntax, where syntax can logically close over the enclosing source location. On the other hand, it seems convenient to go the other way and use the context of the currently executing script. So I don't have a strong opinion here.

caridy commented 5 months ago

This is a very old issue, and the spec is not using HostImportModuleDynamically anymore. I believe this was addressed a while ago, currently, the spec is doing this following as part of the importValue routine:

1. Let _referrer_ be the Realm component of _evalContext_.
1. Perform HostLoadImportedModule(_referrer_, _specifierString_, ~empty~, _innerCapability_).

@nicolo-ribaudo probably have more details. I'm closing the issue, as it seems that when invoking HostLoadImportedModule we always have a valid referrer value pointing to the realm associated to the shadow realm instance.