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

HTML, CSS, and JSON modules shouldn't solely rely on MIME type to change parsing behavior #839

Closed
rniwa opened this issue Sep 18, 2019 · 107 comments
Labels

Comments

@rniwa
Copy link
Collaborator

rniwa commented Sep 18, 2019

As we discussed in TPAC 2019 Web Components session, the current proposal / spec of HTML, CSS, and JSON modules do not specify the type of content in the import statement.

This is problematic because an import statement that intended to load CSS or JSON and not execute arbitrary scripts could end up executing scripts if the destination server's MIME type got changed or the destination server get compromised.

In general, we've made so that the importer of any content can specify how the imported content should be parsed & processed. This is one of the motivations for adding CORS fetch for JSON as opposed to JSONP for example.

@rniwa
Copy link
Collaborator Author

rniwa commented Sep 18, 2019

@annevk @dandclark @domenic

@Jamesernator
Copy link

Jamesernator commented Sep 18, 2019

I think there's two separate parts to this:

  • Treating some response as another/specific format
  • Preventing evaluation of unexpected formats

I personally think the second one should be part of Content-Security-Policy rather than changing anything about the current module loading e.g. Content-Security-Policy: modules self *, https://some.config.tld json, https://fonts.somewhere.tld css.

For the first one maybe we could extend the import: protocol to have import+css:/import+html/etc to force it into a specific format.

@trotyl
Copy link

trotyl commented Sep 18, 2019

This is problematic because an import statement that intended to load CSS or JSON and not execute arbitrary scripts could end up executing scripts if the destination server's MIME type got changed or the destination server get compromised.

I believe this functionality is required in polyfilling, to support browsers not yet supports HTML/JSON/CSS modules, a server can just respond the corresponding JavaScript file that wrapping original content to default export, which could provide a consistent code style at consumer-side.

@rniwa
Copy link
Collaborator Author

rniwa commented Sep 18, 2019

This is problematic because an import statement that intended to load CSS or JSON and not execute arbitrary scripts could end up executing scripts if the destination server's MIME type got changed or the destination server get compromised.

I believe this functionality is required in polyfilling, to support browsers not yet supports HTML/JSON/CSS modules, a server can just respond the corresponding JavaScript file that wrapping original content to default export, which could provide a consistent code style at consumer-side.

Weakening the security model for the sake of polyfilling is an unacceptable trade off in our view.

@justinfagnani
Copy link
Contributor

justinfagnani commented Sep 18, 2019

Imports are inherently dangerous, and this is why CSP allows for restricting the origin for scripts. That should apply to all imports, regardless of type.

Importing JSON should only be done for trusted sources, and shouldn't in general be done for calling third party APIs (and arguably shouldn't be done for 1st party APIs if you don't want your module graph to fail to load if the API call fails). fetch() is much more appropriate for that.

Regarding server-side polyfills, this should still work if the client sends the appropriate Accept header.

@rniwa
Copy link
Collaborator Author

rniwa commented Sep 18, 2019

Imports as inherently dangerous, and this is why CSP allows for restricting the origin for scripts. That should apply to all imports, regardless of type.

Given that many websites don't use CSP correctly, relying on websites to correctly deploy CSP to get the right security behavior is not a great plan.

Furthermore, today, if you were to fetch JSON via XHR or fetch API and parse it via JSON.parse, there is no chance of the fetched JSON suddenly executing as scripts. This new module loading mechanism, therefore, is a functional regression from existing loading mechanisms.

We believe this security issue is a show stopper issue for HTML, CSS, and JSON modules.

@annevk
Copy link
Collaborator

annevk commented Sep 19, 2019

Yeah, Mozilla does as well. Importing non-scripts should be safe by default. (If HTML modules end up executing script they might not necessarily be problematic.)

@matthewp
Copy link

matthewp commented Sep 19, 2019

What is the counter proposal?

@dandclark
Copy link
Contributor

dandclark commented Sep 19, 2019

I think @justinfagnani's point here is interesting and I want to second it. Even aside from security concerns, JSON imports seem like the wrong tool for consuming non-first-party JSON/CSS. If the request 404's or if, say, the JSON has a parse error, the entire module graph will fail to instantiate/execute. fetch() is the right tool in this scenario, with import being reserved for content that is directly controlled by the importer. Otherwise all your module scripts could fail to run because of a missing { in third-party JSON...

@annevk
Copy link
Collaborator

annevk commented Sep 19, 2019

That doesn't apply to import() afaik.

@Jamesernator
Copy link

Jamesernator commented Sep 20, 2019

If the request 404's or if, say, the JSON has a parse error, the entire module graph will fail to instantiate/execute.

This might actually be what you want in some cases, e.g. if a critical resource does fail to load then you don't want to waste resources evaluating a bunch of modules you don't need. You can always use import() to catch errors in that particularly and do some recovery code or retry.

@rniwa
Copy link
Collaborator Author

rniwa commented Sep 20, 2019

I think @justinfagnani's point here is interesting and I want to second it. Even aside from security concerns, JSON imports seem like the wrong tool for consuming non-first-party JSON/CSS.

Regardless of whether it's a good idea or not, some web developers are inevitably going to do it. We shouldn't be adding a new foot gun to the Web so that authors can avoid using it in certain situations to avoid creating a new security surface.

@caridy
Copy link

caridy commented Sep 20, 2019

I can only second @rniwa here, @justinfagnani's point included a lot of "should/shouldn't", and we all know how that goes. Importing non-scripts must be safe by default.

@littledan
Copy link

littledan commented Sep 20, 2019

Is it OK for WebAssembly modules to have parsing behavior based on their MIME type? That's what the current proposal does.

@joeldenning
Copy link

joeldenning commented Sep 20, 2019

HTML, CSS, and JSON modules do not specify the type of content in the import statement.

Are there any proposals / ideas that could solve these issues?

At the risk of suggesting something naive, could the file extension be the way that the import statement specifies its expected module type?

 // both the file extension and mime type must be present for the non-script module to load
import 'file.json'
import 'file.css'
import 'file.html

The obvious limitation of this approach is that you can't load json, css, and html modules from urls without the proper file extension. The advantage is that it doesn't require a rework of import statements and import maps to make html, css, and json modules work.

@annevk
Copy link
Collaborator

annevk commented Sep 21, 2019

Apart from plugins there's no precedent for putting meaning in file extensions within the web, I don't think we should start here.

@rniwa
Copy link
Collaborator Author

rniwa commented Sep 21, 2019

Is it OK for WebAssembly modules to have parsing behavior based on their MIME type? That's what the current proposal does.

That’s not ideal but less problematic because the expectation of loading WASM & JS are similar: they execute arbitrary scripts. It’s not so with CSS & JSON.

annevk added a commit to whatwg/html that referenced this issue Sep 28, 2019
As explained at WICG/webcomponents#839 the current setup is insecure.

This reverts db03474.
@littledan
Copy link

littledan commented Sep 28, 2019

If we had syntax in JavaScript for asserting the MIME type (mandatory for JSON modules, and optional for JavaScript), would that address this concern? If so, we can look into this issue in TC39; I don't think we have considered it before. As a strawperson, it could look like this:

import document from "./foo.json" with mime: "application/json";

This could be a basis for adding sending metadata to the host environment (HTML, Node.js, etc). Thoughts? Did people have other specific ideas for what this should look like?

@annevk
Copy link
Collaborator

annevk commented Sep 28, 2019

Nothing concrete was discussed, but yeah, something like that would address the concern. And equivalent for import(). Instead of the MIME type it might be nicer to assert it being "json" or "css" or some such.

@ljharb
Copy link

ljharb commented Sep 28, 2019

The author is the only true arbiter of the parse goal of content; would this be an assertion, or would it change the parsing like script type does, for example?

@littledan
Copy link

littledan commented Sep 28, 2019

@ljharb I imagine we would continue to use MIME type in conjunction with the declaration within JS. That's why I used the word "assertion".

@devsnek
Copy link

devsnek commented Sep 29, 2019

how about the CSP idea but the default is disallow instead of allow? I think the layering of extending import syntax is a bit iffy, since it adds host-specific semantics to an otherwise generic bit of code, and then random hosts have to be like "wait what do i do with this"

@annevk
Copy link
Collaborator

annevk commented Sep 29, 2019

It doesn't really seem host-specific to be able to import JSON without that resulting in script execution later on.

@devsnek
Copy link

devsnek commented Sep 29, 2019

as an example, node requires you to write .json in the import specifier. we would have no use for such a syntactic extension. jumping a bit further, if someone writes import x as 'application/json', but a host doesn't use mine types, what do they do with that?

@littledan
Copy link

littledan commented Sep 29, 2019

Right, somehow, this is syntactically redundant in environments where the interpretation is implied by the module specifier's suffix already. The web doesn't have a tradition of making such judgements.

I suppose the analogous (and unprecedented?) thing here would be something like requiring that, if the MIME type is application/json, then the module specifier must end in .json, and prohibiting JS modules with this suffix. But such a scheme faces web compatibility issues with growing over time.

If we require this syntax for JSON modules on the web, I think there is some chance that a common authoring format will omit the assertions, and tools will insert it when generating web output as part of a build process. But there is also a chance that we can convince most people to write this directly.

@devsnek
Copy link

devsnek commented Sep 29, 2019

sorry, to be more direct with what i'm trying to say: given security is host specific, shouldn't this assertion be out-of-band from the import? at worst you would have hosts ignoring a check people expected would be enforced.

@annevk
Copy link
Collaborator

annevk commented Sep 30, 2019

Inferring meaning from a file extension is incompatible with the web's architecture. Requiring out-of-band annotations seems like it would create such bad ergonomics the feature would effectively not be used on the web.

@justinfagnani
Copy link
Contributor

justinfagnani commented Nov 19, 2019

Oh wow, I didn't realize we were that close. So, my above would be replaced with const { supportsBar } = await fetch("./config.json").then(x=>x.json())? That really is the only use case I can think of. Maybe we don't need HTML/CSS/JSON modules after all?

Top-level await is absolutely not a replacement for CSS and HTML modules.

Wide-spread use of TLA will slow down module loading unacceptably. If every component in a large module graph of components uses TLA to load it's styles, then we get a waterfall of awaited fetches from deeper modules in the graph on up. If a module loads multiple resources, it has to take care to not have a waterfall locally. The ergonomics are terrible compared to import. TLA is dangerous enough that there is movement to ban them in ServiceWorker context, and I would expect linters and build tools to offer mode to ban them in regular modules as well.

Also, TLA+fetch doesn't offer the additional and extremely useful module semantics like deduplication and import maps. Imagine a CSS file that's loaded into every component in an app:

const baseStyles = new CSSStyleSheet();
await baseStyles.replace(await (await fetch('../base/styles.css')).text());

First, that's absolutely horrific DX compared to import baseStyles from '../base/styles.css' as css;. Miss that first await and things break in a subtle way (the stylesheet updates in the background after the app logic has run, causing a flash of styling and a re-layout). Second, it doesn't benefit from deduplication at all. Every module that does this will cause a fetch of the base styles.

So now we have to add a cache, which further decreases the DX, so we add a wrapper library to load CSS and hope that every developer in the world uses the same CSS loader so they use the same cache. This screams for being a built-in due to all the pitfalls here.

Finally, this would leave the specific module semantics undefined and unimplemented, which leaves significant complexity to userland in the case of HTML modules, and precludes next steps in standardizing in-demand patterns around CSS modules (exporting class names, CSS references, potentially mixins, etc.).

@devsnek
Copy link

devsnek commented Nov 19, 2019

Top-level await is absolutely not a replacement for CSS and HTML modules.

Don't worry, I wasn't suggesting that it was. Just for JSON resources.

TLA is dangerous enough that there is movement to ban them in ServiceWorker context, and I would expect linters and build tools to offer mode to ban them in regular modules as well.

That's because ServiceWorker stops registering listeners after the first tick, not because TLA is inherently evil. In fact, you can still dynamically import a TLA graph in ServiceWorker.

@tilgovi
Copy link

tilgovi commented Nov 19, 2019

Is there a solution discussed anywhere already that can take inspiration from import maps to provide loaders declared out of band?

@justinfagnani
Copy link
Contributor

justinfagnani commented Nov 19, 2019

@tilgovi I think OOB has significant DX and usability downsides. See my comment here: tc39/proposal-import-assertions#13 (comment)

@justinfagnani
Copy link
Contributor

justinfagnani commented Dec 6, 2019

Is it OK for WebAssembly modules to have parsing behavior based on their MIME type? That's what the current proposal does.

That’s not ideal but less problematic because the expectation of loading WASM & JS are similar: they execute arbitrary scripts. It’s not so with CSS & JSON.

In think about this a bit today... WASM doesn't have access to the DOM, correct? So an author could assume a WASM module has restricted access if it's not explicitly passed functions to allow it DOM access. If a file previously served as application/wasm was later served with application/javascript, would this present a similar security concern?

@littledan
Copy link

littledan commented Dec 7, 2019

The not-yet-implemented-in-any-browser Wasm/ESM integration proposal gives Wasm the same level of privilege as JavaScript by design. This proposal allows importing arbitrary JS modules (including cross-origin), which could export functions that manipulate the DOM but have signatures which are just based on numerics, so it would be importable and usable from Wasm. The goal is to allow transparent interaction.

@Ciantic
Copy link

Ciantic commented Jun 14, 2020

If this is about validating the input given by import, then could the import be augmented with function?

import someJson from "./some.json" with JSON.Parse;
import someTextFile from "./some.txt" with String;

// Going further, one could define own parser:
function MyParser(content, url, mimetype) {
  let sheet = new CSSStyleSheet();
  sheet.replaceSync(content);
  return sheet;
}
import someCssStylesheet from "./someother.css" with MyParser;

Having the parser as a plain function one can define, would allow to introduce some other logic easily in future. It's not uncommon to have different ways to parse e.g. *.css imports.

It would even allow returning a module like result, because you could return object from your parser function e.g.

function AnotherParser(content, url, mimetype) {
  // ...
  return { justThis: "Hello world", orThis: 5 };
}
import { justThis, orThis } from "./interesting-url" with AnotherParser;

It's of course debatable what argument should the parser get? Some object with more details including mime type, a string, and possibly the url?

@dandclark
Copy link
Contributor

dandclark commented Sep 29, 2020

With the advancement of import assertions to Stage 3 in TC39, can we consider this issue resolved?

@jerrygreen
Copy link

jerrygreen commented Sep 29, 2020

@dandclark I personally still don't see any info of how that would be resolved when there's no assertion. Will this:

import json from "./foo.json"

still use MIME-type?

I opened this question in the proposal, calling it default assertions: tc39/proposal-import-assertions#101

@justinfagnani
Copy link
Contributor

justinfagnani commented Sep 29, 2020

@jerrygreen import json from "./foo.json" will indeed use the mime-type of the response and fail if it's not a JavaScript type. Edit: in browsers.

@annevk
Copy link
Collaborator

annevk commented Sep 29, 2020

Yeah, let's close this. In response to this issue:

  1. We removed JSON modules from HTML for the time being: Revert JSON modules whatwg/html#4943.
  2. TC39 worked out import assertions, currently stage 3.
  3. Integration with import-attributes whatwg/html#5640 discusses integration of that with HTML and links PRs for that as well as JSON modules.

@annevk annevk closed this as completed Sep 29, 2020
@tclzcja
Copy link

tclzcja commented Sep 29, 2020

Sorry for commenting on a closed issue, but does that mean HTML/JSON/CSS Modules can only land after import assertion gets to Stage 4?

In other words, is Import Assertion the blocker on HTML/JSON/CSS Modules?

@annevk
Copy link
Collaborator

annevk commented Sep 29, 2020

Stage 3 is fine for integration with the HTML Standard.

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

No branches or pull requests