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

CSS and JSON module scripts and CSP #7233

Closed
annevk opened this issue Oct 18, 2021 · 24 comments · Fixed by #9486
Closed

CSS and JSON module scripts and CSP #7233

annevk opened this issue Oct 18, 2021 · 24 comments · Fixed by #9486
Labels
integration security/privacy There are security or privacy implications topic: fetch

Comments

@annevk
Copy link
Member

annevk commented Oct 18, 2021

WebAssembly/esm-integration#56 made me wonder what happens for CSS and JSON module scripts and it seems that since destination is "script", CSP will use script-src for them. Is that really what we want?

cc @lweichselbaum @antosart @mikewest @guybedford @dandclark @whatwg/modules

@guybedford
Copy link

This definitely seems strange to me and like something that should be fixed - that CSS imports should be style-src and JSON imports should be something else but not script-src since they don't represent any execution. There is also no escalation possibility here by design since the import './data.json' assert { type: 'json' } assertion syntax being mandated guarantees that we know the policy in play.

Given that import assertions are mandated perhaps that gives us the flexibility to use that to infer the CSP policy as well?

Alternatively I see the CSP spec does have a response phase, so the alternative could be do perform all module CSP checking in the response phase perhaps?

I hope we can solve this as it would be highly beneficial to lay the way for wasm-src as well and avoid the permanent need for a non-granular unsafe-inline-wasm in policies.

@bathos
Copy link

bathos commented Oct 18, 2021

All of them end up "being" JS modules, right? What are the benefits of distinguishing them based on the source media type rather than the destination type?

@rniwa
Copy link

rniwa commented Oct 18, 2021

What are the benefits of distinguishing them based on the source media type rather than the destination type?

So that some file allowed to be imported as a CSS module can't be simultaneously imported as JS and start running arbitrary scripts. The same way we wanted to specify the type of module where it's used, it's important for CSP to be able to specify & restrict how a given content can be interpreted.

@ljharb
Copy link
Contributor

ljharb commented Oct 19, 2021

If CSP can restrict it, then is there still a need for an import assertion?

@bathos
Copy link

bathos commented Oct 19, 2021

@ljharb AFAIK a CSP can only express constraints which apply after the type/usage is known - e.g. style-src 'self' says that all self-origin URL resources can be interpreted as stylesheets, not that they will be / must be.

@guybedford
Copy link

Does anyone have any reasonable objections for treating the import assertion as a CSP policy type indicator?

@rniwa
Copy link

rniwa commented Oct 19, 2021

If CSP can restrict it, then is there still a need for an import assertion?

Yes. There are plenty of websites where CSP isn't deployed or can't be deployed for various reasons. We need a way to ensure CSS or JSON files wouldn't start parsing as JS specially on a server with content based MIME type detection.

@josepharhar
Copy link
Contributor

I'm not super familiar with this feature but I'd like to see it get included in interop23: web-platform-tests/interop#236
Is there any rough consensus in this issue?
Is there anything I can change in chromium that would help?

@annevk
Copy link
Member Author

annevk commented Nov 1, 2022

It would be helpful to get some input from those intimately familiar with CSP and module scripts. I think what would end up doing the right thing is that the Import Assertion changes the request destination. That in turn will impact which CSP policy ends up applying.

@antosart @mikewest @arturjanc do you think that's workable?

@arturjanc
Copy link

Just to double-check my assumptions: my understanding is that importing a module requires specifying a type assertion, and that this assertion ensures that a module with a declared CSS/JSON type will not be executed as script. If so, then it would make sense for non-script imports to be governed by other CSP directives (probably style-src and connect-src for CSS and JSON respectively?).

If changing the request destination has this effect, then this sounds quite reasonable to me.

@mikewest
Copy link
Member

mikewest commented Nov 9, 2022

I agree with Artur's take. Shifting the destination of the requests sounds like a reasonable way of reflecting the risk associated with each request type if each request type solidly enforces content-type matches. I'd be comfortable with that approach.

@hiroshige-g
Copy link
Contributor

I'm relatively neutral, but given that CSS/JSON modules

  • have Content-Type enforcement,
  • are always parsed as CSS/JSON, and
  • don't trigger another module to load (e.g. CSS/JSON modules can't have any submodules and thus can't trigger any JS loading),

it somehow looks reasonable to load them as CSS/JSON, not as scripts.

As for implementation, I suppose in Chromium this is to load CSS/JSON modules using e.g. CSSStylesheetResource instead of ScriptResource.
This is more consistent, not only in that applying style-src CSP to CSS modules, but also in that ScriptResource always loads scripts (currently ScriptResource can load CSS modules).
This makes module loading code path less uniform though: module loading code should switch between resource types and be careful about subtle differences (if any) between them.

@annevk
Copy link
Member Author

annevk commented Nov 30, 2022

It was pointed out to me that this really stretches the meaning of assertion. As now the assertion directly drives the way we fetch. And there's other features people brought up that would potentially stretch it even further (such as importing text as a string, an ArrayBuffer, or a Blob). Perhaps changes on the TC39 side are still possible here that would make this less jarring?

@littledan
Copy link
Contributor

Wouldn't changing the destination for CSS module scripts to "style" change the Accept header (in fetch step 13.2)? If the assertion changes how the request is made or how the response is processed (beyond simply rejecting or accepting the response), then we've gone beyond "assertions" and are violating part of the specification

moduleRequest.[[Assertions]] must not influence the interpretation of the module or the module specifier; instead, it may be used to determine whether the algorithm completes normally or with an abrupt completion.

If browsers feel that CSS modules should have a "style" destination (which seems totally reasonable to me on the face of it!) and if this contradicts the import assertions specification, we may need to go back and think more about the definition of import assertions at TC39.

I filed tc39/proposal-import-attributes#125 to discuss this topic. I hope that we don't need to make changes to import assertions, given that they are already shipping in a browser and at Stage 3, but this question is really core to the use cases import assertions were initially designed for, and the syntax and semantics that were designed based on that.

We're going to discuss this HTML integration issue in TC39's next fortnightly call around modules. If any fetch/HTML experts are available for this discussion, it would be very helpful; I'm happy to forward the invitation.

@annevk
Copy link
Member Author

annevk commented Dec 1, 2022

Depending on the time I might be able to attend.

@annevk annevk added the agenda+ To be discussed at a triage meeting label Jan 17, 2023
@annevk
Copy link
Member Author

annevk commented Jan 17, 2023

I put this on the agenda to discuss more officially requesting TC39 changing the syntax as this change would no longer make assertion an applicable term.

@ljharb
Copy link
Contributor

ljharb commented Jan 17, 2023

It also seems to undermine the use case that went to stages 2 and 3 - so the entire feature may need to be reevaluated.

@Constellation
Copy link
Member

Constellation commented Jan 17, 2023

Yes, this is fundamental change. So if it happens, we should at least move the stage back to stage-2. Or given that the thing becomes changed from assertion, we may need a new proposal instead.

@past past removed the agenda+ To be discussed at a triage meeting label Jan 26, 2023
@syg
Copy link
Contributor

syg commented Jan 27, 2023

Hi folks, after discussing internally, the chromium and V8 position is:

  • We prefer the proposed HTML semantics, i.e. allow Accept headers and have CSP integration work for CSS modules. These semantics seem intuitive and do the right thing to us -- the "no reinterpretation" ECMAScript restriction ought to be relaxed to allow for CSP integration, but still be strong enough for the security requirements.
  • This feature has been Stage 3 in TC39 for a while, and has been shipping in Chrome since 91 and V8 9.1 (which was released May 2021). Given how long it's been out in Chrome as well as the tooling ecosystem in Node/Deno and bundlers, we are not going to unship the feature on short notice. Moreover we prefer to not unship, but to change the behavior to the proposed semantics.
  • For the same reason above, it'd be ideal if we didn't have to change the assert keyword either. If the developer response to the assert keyword is overwhelmingly negative, we think the next step should be to assess how widespread the use is in the tooling ecosystem and possibilities of migration.

Our thinking is that it's important to recall that when we did import assertions, there was a real risk that without a proper syntax proposal, the demand for the capability was strong enough that the ecosystem was going to invent micro DSLs within the module specifier to convey type information. This demand is still as strong now as it was then, evidenced by the tools' quick adoption of this feature and some apparent violations of the "no reinterpretation" restriction. Similarly, a micro DSL world is just as undesirable now as it was then, and unshipping the feature puts us at risk of that again (on top of the usual pains of possibly breaking users). We interpret the strong demand and the tooling ecosystem's disregard of the "no reinterpretation" restriction as a solid signal that the ideal path forward is to relax that restriction instead of disabling the feature.

Finally, I wonder if we're overindexing on one particular prescriptive meaning of the word "assert". The proposed HTML behavior is actually well within the lay English definition of "assert", though it deviates from its common usage in programming as a function name that aborts execution when checking for a condition. Given that this is novel syntax, I'm inclined to squint and consider it a different shade of meaning of the word "assert".

@clshortfuse
Copy link

clshortfuse commented Feb 16, 2023

Would it not be enough for CSS to be loaded in an inactive document, which is already guarded from execution, IIRC?

For example, with DocumentFragment imports, I use document.implementation.createHTMLDocument(). because <img src={url}> would be parsed and cause network fetchs before the template can be interpolated. If HTML Imports becomes finalized, I hope that becomes standard because I wouldn't be able to use it for importing uninterpolated templates if it'll start parsing on the current (active) document.

For CSS modules, my transcompilation workaround for Webkit and Firefox both use an inactive document to create the CSSStyleSheet. CSP throws up errors when I try to add an HTMLStyleElement to a shadowRoot as it should. And I no problems with adoptedStyleSheets. I'm not sure where the chain of trust would be broken. Pardon my ignorance, but are we trying to guard against CSS Modules being loaded before it can be applied to adoptedStyleSheets? Is that the goal?

Edit: Related w3c/DOM-Parsing#65

domenic pushed a commit that referenced this issue Mar 24, 2023
See recent changes to https://github.com/tc39/proposal-import-attributes. Notably:

* Import attributes can affect how a module is loaded and can be part of the cache key. This removes the willful violation HTML had in its usage of import assertions, and unblocks #7233.

* The keyword has been changed from assert to with. Due to existing support in Chrome/Node.js/Deno the assert keyword will stick around for a while, and the V8 team will investigate the possibility of removing it.
@GeoffreyBooth
Copy link

after discussing internally, the chromium and V8 position

@syg After this week's TC39 meeting can you share the Chromium/V8 team's current thinking? I'm assuming V8 will add support for with; any idea of a rough timeline for that? And any idea on when or whether V8 will drop assert?

@jridgewell
Copy link

Chrome is investigating whether it can even remove it based on useage: https://bugs.chromium.org/p/v8/issues/detail?id=13856.

@clshortfuse
Copy link

I've honestly never pushed anything publicly that didn't transcompile CSS imports to string => CSSStyleSheet/HTMLSyleElement. Unless Chromium's counter checks source maps (if they even exist), I doubt the counter will tell us much. I find CSS imports as mostly a pre-bundled thing currently. I can't imagine anything public-facing outside of maybe electron / chrome apps.

Since I've been having a rough time trying to get external tools (eg: bundlephobia, webpack, esbuild) to work with them, I'm in the process of dropping them all in my dev code since. Since we're switching actual syntax, then I feel it leads to an appearance of instability. I will likely wait for it to ship on Firefox, before proselytizing their usage around the JS community. Back to template literals, for now.

@nicolo-ribaudo
Copy link
Contributor

I opened #9486 implementing a fix for this, if anyone wants to review it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration security/privacy There are security or privacy implications topic: fetch
Development

Successfully merging a pull request may close this issue.