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.

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

Rationale for optional exception #16

Open
jugglinmike opened this issue May 21, 2021 · 13 comments
Open

Rationale for optional exception #16

jugglinmike opened this issue May 21, 2021 · 13 comments

Comments

@jugglinmike
Copy link

The proposal includes the following text:

  • If assertions has an entry entry such that entry.[[Key]] is "type", let type be entry.[[Value]]. The following requirements apply:
    • If type is "json", then this algorithm must either invoke ParseJSONModule and return the resulting Completion Record, or throw an exception.

I don't see why the proposal allows implementers to optionally throw an exception. I've summarized my understanding below to help others find the hole in my reasoning (and potentially identify an opportunity to improve the proposal's documentation).

I'm aware of the security concerns that were initially raised about an early version of this proposal, but it's not clear if/how the optional exception is related. In that issue (and the subsequent conversations at TC39 plenary throughout 2020), it seemed sufficient to give authors a means to communicate the desire, "this resource should never be evaluated as JavaScript." That allows hosts to fail the import based on additional context (e.g. web browsers receiving HTTP responses with inappropriate Content-Type headers).

The part where I get lost is why the proposal text explicitly allows an exception. Even without import assertions, web browsers are already rejecting modules at their own discretion (e.g. due to Content-Security Policy violations). I haven't been able to find any justification for that kind of rejection in ECMA262--the closest I could find was the exception for Module Records that "[do] not exist or cannot be created." My ignorance doesn't prove anything, of course (as demonstrated by the existence of square dancing), but it makes me wonder whether it's absent because it's not necessary. If it is present, I'd be curious to learn why it doesn't apply to this case.

Even if an error is specifically necessary for modules imported using this new syntax, it seems as though the expected exception (and the security guarantee) would be naturally enforced by the semantics of the ParseJSONModule abstract operation. In order to present any danger in this context, the source text would necessarily fail to parse as JSON, resulting in a SyntaxError.

Thanks for the help!

@ljharb
Copy link
Member

ljharb commented May 22, 2021

Browsers require the ability to:

  1. error when someone imports JSON without an explicit syntactic marker
  2. not error when someone imports JSON with that same marker
  3. error when someone imports non-JSON with that same marker

node requires the ability to:

  1. not error when someone imports JSON without an explicit syntactic marker
  2. not error when someone imports JSON with that same marker
  3. error when someone imports non-JSON with that same marker

The first item in each list, taken together, seems to require leaving the decision about whether to throw or not up to the host. Please let me know if I'm misunderstanding something about your OP.

@jugglinmike
Copy link
Author

Thanks, @ljharb! That's a good summary of stakeholders' requirements, but it doesn't seem to address the behavior I'm wondering about. The option to "throw an exception" is only available in the presence of the explicit syntactic marker. Since the first item in each list describes its absense, neither appear to be related.

@ljharb
Copy link
Member

ljharb commented May 27, 2021

@jugglinmike the second two items in both lists describe its presence.

@jugglinmike
Copy link
Author

@ljharb That's true! I was focused on the first item in each list because I thought your explanation concerns them specifically.

The second two items apply to the use of "type": "json", so maybe the optional error is meant to allow hosts to control their own expectation for "JSON" versus "non-JSON".

You and I talked about this in IRC; for others following along:

  • ljharb: if you import without type json today and get .json, browsers insist that must always error, to avoid the footgun referenced just prior
  • jugglinmike: Yeah, I've read some of the motivating discussion.
  • jugglinmike: But isn't the rejection (and maybe most importantly, the refusal to execute code) covered by the semantics of ParseJSONModule?
  • jugglinmike: In other words, if the server starts sending JS, won't the import naturally fail when it calls JSON.parse?
  • ljharb: not necessarily - valid json can also be js
  • ljharb: and browsers want to reject based on mime, not content
  • ljharb: in the case of something with a filesystem it’d likely be based on file extension, also not content

I'm still pressing on this error because I believe that behavior is already possible for hosts without the additional language currently in the proposal.

My working example is the web platform feature Content-Security Policy. That's a case where browsers are already selectively fail to load modules according to their own internal priorities.

As I understand it, this is allowed by ECMA262 via HostResolveImportedModule. By my reading, web browsers are essentially saying that CSP-violating module records "cannot be created." If that's true, then can't that same language also be used to justify the rejection of modules which hosts have classified to be "non-JSON"?

@ljharb
Copy link
Member

ljharb commented May 27, 2021

In this case, the module itself can't be rejected, only the importing callsite.

@jugglinmike
Copy link
Author

Would you mind giving an example of a case where the distinction is observable?

@ljharb
Copy link
Member

ljharb commented May 31, 2021

Promise.allSettled([import('path/to.json'), import('path/to.json', { assert: { type: 'json' } })]) should in a browser produce a Promise of an array of two settlement objects, one rejected and one fulfilled.

In node, for example (assuming the assertion ends up being optional, which has always been the plan), that should provide a Promise of an array of two settlement objects, both fulfilled.

@jugglinmike
Copy link
Author

This change from the Import Assertions proposal reads like it would allow hosts to create distinct module records for each of those two ImportCalls:

  • Each time this operation is called with a specific referencingScriptOrModule, moduleRequest.[[Specifier]], moduleRequest.[[Assertions]] triple as arguments it must return the same Module Record instance if it completes normally.
    • It is recommended but not required that implementations additionally conform to the following stronger constraint: each time this operation is called with a specific referencingScriptOrModule, moduleRequest.[[Specifier]] pair as arguments it must return the same Module Record instance if it completes normally.

If so, couldn't browsers reject the module record itself (i.e. without the optional error here in JSON modules) and still implement the expected behavior?

@ljharb
Copy link
Member

ljharb commented Jun 3, 2021

Yes, browsers need the capability to do that because of race conditions - two simultaneous import()s could receive different modules/MIME types, and both need to respond appropriately (rather than the first to arrive dictating the second to arrive's behavior). However, any host that doesn't fetch modules over the network wouldn't need that flexibility (browsers won't either, when the cache is already populated), and they will be able to conform to the stronger constraint.

@jugglinmike
Copy link
Author

Is it accurate to say that the exception we're discussing isn't observable and that it's present to allow hosts to conform to an optional constraint?

@ljharb
Copy link
Member

ljharb commented Jun 3, 2021

It would certainly be observable on browsers if you controlled the server and the client. Whenever it's not observable, then engines are encouraged to conform to the stronger constraint.

(It's certainly not something test262 should or can test, I think)

@jugglinmike
Copy link
Author

It would certainly be observable on browsers if you controlled the server and the client. Whenever it's not observable, then engines are encouraged to conform to the stronger constraint.

It seems that it isn't possible for authors to differentiate between the following two compliant behaviors:

  • the host tracks Module Records by the "import triple" and never uses the optional rejection in the JSON Modules proposal
  • the host tracks Module Records by the "import pair" and uses the optional rejection in the JSON Modules proposal at its discretion

Is that right?

(It's certainly not something test262 should or can test, I think)

That's been my suspicion, as well!

@ljharb
Copy link
Member

ljharb commented Jun 10, 2021

It’s not possible without a very obscure testing setup, and intimate knowledge of the engine’s internals, i think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants