Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

layer 0: HostEnsureCanCompileStrings integration #76

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

layer 0: HostEnsureCanCompileStrings integration #76

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions 0-module-and-module-source.emu
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,8 +291,9 @@ location: https://tc39.es/proposal-compartments/
<p>When the `ModuleSource` function is called with argument _sourceText_, the following steps are taken:</p>
<emu-alg>
1. If NewTarget is *undefined*, throw a *TypeError* exception.
1. Let _sourceTextString_ be ? ToString(_sourceText_).
1. Let _body_ be ParseText(StringToCodePoints(_sourceTextString_), |Module|).
1. Let _O_ be ? OrdinaryCreateFromConstructor(NewTarget, *"%ModuleSource.prototype%"*, « [[ModuleSource]] »).
1. Let _body_ be ParseText(_sourceText_, |Module|).
1. If _body_ is a List of errors, throw a *SyntaxError* exception.
1. Set _O_.[[ModuleSource]] to ? CreateModuleSourceRecord(_body_).
1. Return _O_.
Expand Down Expand Up @@ -367,6 +368,7 @@ location: https://tc39.es/proposal-compartments/
<emu-clause id="sec-createmodulerecord" type="abstract operation">
<h1>
CreateModuleRecord (
_realm_: a Realm Record,
_moduleSource_: a Module Source Record
): a Source Text Module Record
</h1>
Expand All @@ -375,6 +377,7 @@ location: https://tc39.es/proposal-compartments/
<dd>It creates a Source Text Module Record based upon the result of a previously parsed _sourceText_ as a |Module| associated to the provided Module Source Record.</dd>
</dl>
<emu-alg>
1. Assert: _realm_ is a Realm Record.
1. Assert: _moduleSource_ is a Module Source Record.
1. Let _async_ be _sourceModule_.[[HasTLA]].
1. Let _body_ be _sourceModule_.[[ECMAScriptCode]].
Expand All @@ -384,7 +387,6 @@ location: https://tc39.es/proposal-compartments/
1. Let _indirectExportEntries_ be _sourceModule_.[[IndirectExportEntries]].
1. Let _starExportEntries_ be _sourceModule_.[[StarExportEntries]].
1. Let _hostDefined_ be _sourceModule_.[[HostDefined]].
1. Let _realm_ be the current Realm Record.
1. Return Source Text Module Record { [[Realm]]: _realm_, [[Environment]]: ~empty~, [[Namespace]]: ~empty~, [[CycleRoot]]: ~empty~, [[HasTLA]]: _async_, [[AsyncEvaluation]]: *false*, [[TopLevelCapability]]: ~empty~, [[AsyncParentModules]]: &laquo; &raquo;, [[PendingAsyncDependencies]]: ~empty~, [[Status]]: ~unlinked~, [[EvaluationError]]: ~empty~, [[HostDefined]]: _hostDefined_, [[ECMAScriptCode]]: _body_, [[Context]]: ~empty~, [[ImportMeta]]: ~empty~, [[RequestedModules]]: _requestedModules_, [[ImportEntries]]: _importEntries_, [[LocalExportEntries]]: _localExportEntries_, [[IndirectExportEntries]]: _indirectExportEntries_, [[StarExportEntries]]: _starExportEntries_, [[DFSIndex]]: ~empty~, [[DFSAncestorIndex]]: ~empty~, [[ModuleInstance]]: *undefined* }.
</emu-alg>
</emu-clause>
Expand Down Expand Up @@ -548,8 +550,11 @@ location: https://tc39.es/proposal-compartments/
1. Else, if Type(_importMeta_) is not Object, then
1. Throw a *TypeError* exception.
1. Perform ? RequireInternalSlot(_moduleSource_, [[ModuleSource]]).
1. Let _evalRealm_ be the current Realm Record.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean the [[Realm]] from the current Execution Context?

My feeling is that we should be getting [[Realm]] from an internal slot of the %Module% constructor, patterned after %Function%.

Copy link
Collaborator Author

@caridy caridy Aug 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is the pattern used by Function constructor (https://tc39.es/ecma262/#sec-function-p1-p2-pn-body), which just calls https://tc39.es/ecma262/#sec-createdynamicfunction, and this abstract operation gets the current realm.

1. NOTE: Module Instances created from this constructor must use a valid source.
1. Perform ? HostEnsureCanCompileStrings(_evalRealm_).
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Creating a ModuleSource is not problematic, but creating a Module Instance from that source is, in which case we call the host.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nicolo-ribaudo this is important so we can serialize and share a ModuleSource instance with another realm, and evaluate it there if possible without requiring string evaluation privileges on the realm creating the source.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I believe the Host operation has the wrong (or misleading) name since the problem is not really compilation IMO, but evaluation. We should get some clarifications from implementers.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @Jack-Works

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is somehow like you create a new Function via eval and call it in a CSP-enabled Realm. It's OK. You should put the check in the ModuleSource constructor.

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo Sep 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Creating a ModuleSource is not problematic

I just realized that this is problematic: for layer 1, hosts must parse the string passed to ModuleSource to provide the static analysis info, and thus it requires them to ship a complete parser.

This host hook is not just about security, it's about avoiding shipping big chunks of code (parser, interpreter) that are almost never used on resource-constrained engines.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be called in ModuleSource constructor, because create a new module instance from a compiled source is safe, but convert a string to a ModuleSource is unsafe.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good point... but the problem that I see here is that csp can completely block the parsing of a source... let's chat more about it. Ideally, we can decorate the source record to conditionally call this host hook. cc @nicolo-ribaudo

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

csp can completely block the parsing of a source

Yes, that's how CSP works.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I understand that's how it works today, but if we are going to extend the ModuleSource API to expose the metadata of the parsed source, you will not be able to parse it in the first place even though you don't plan to evaluate it. E.g.: a bundler.

1. Let _O_ be ? OrdinaryCreateFromConstructor(NewTarget, *"%Module.prototype%"*, « [[Module]], [[ModuleSourceInstance]], [[ImportHook]] »).
1. Let _moduleRecord_ to ! CreateModuleRecord(_moduleSource_.[[ModuleSource]]).
1. Let _moduleRecord_ to ! CreateModuleRecord(_evalRealm_, _moduleSource_.[[ModuleSource]]).
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reusing the _evalRealm_ here, but also it makes sense that when a module record is created, it can be associated to a realm, and the caller of the abstract operation to be responsible for providing such realm. This opens the door for more advanced use-cases like transferring instances across realms.

1. Set _moduleRecord_.[[ModuleInstance]] to _O_.
1. Set _moduleRecord_.[[ImportMeta]] to _importMeta_.
1. Let _importHookClosure_ be a new Abstract Closure with parameters (_specifier_) that captures _importHook_ and _referral_ and performs the following steps when called
Expand Down