CSP: refactor to not require style-src 'unsafe-inline and add test coverage
#746
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Also should not apply CSP two times (by header and meta tag). Report-Only for development, block violations in tests.
stylesheet.removeRule() is not part of spec. It's working in Chrome but not in Firefox.
simonihmig
approved these changes
Jan 17, 2019
You mean they are only useable with Glimmer component? You sure? I didn't read that out of the RFC, but might have missed something. I believe they should work with any component, as well as any template in general. |
Thanks.
They should work for all type of components for elements defined in template. But they won't work for the element defined by component. The three cases of dynamic styles addressed by this pull request and #738 are only dealing with elements defined by component. Therefore a refactor for outer HTML would be needed. |
|
Ah, ok, got it! yes, indeed... |
simonihmig
added a commit
that referenced
this pull request
Aug 23, 2019
Previously an inline `display: block` (to override a default `display: none`) was set in JavaScript (CSSOM) to not cause strict CSP violations (see #746). But in FastBoot that code would not run (there is no real DOM to set styles to), so an opened modal rendered in FastBoot would show only the backdrop, the actual modals would still be hidden. This change applies the default BS classes that apply `display: block` in a normal Ember-only way, so they get rendered in FastBoot. That is happening *additionally* to setting inline `display: block`, as I thought that maybe users might have omitted those utility classes from their CSS bundle, in which case modals wouldn't show up at all.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes all violations of a strict Content-Security-Policy and add test coverage to prevent regression. Tests will fail if they trigger a CSP violation.
Fixes for CSP are binding of style attribute which requires
unsafe-inlinecause Glimmer will do aelement.setAttribute('style', value). This is true forComponent.extend({ attributeBindings: ['style'], style: '...' })as well as<div style="...">in templates. The last case was often present in tests. For that one a test helper that inserts CSS rules needed for testing is added. Attribute binding was mostly refactored to methods that changes the style using CSSOM and triggered in component lifecycle hooks. There might still be room for optimization.I'm not convinced by the code needed to dynamically bind style. That doesn't feel correct in an ember environment. But as far as I know there isn't any better way currently. Hopefully there will be as soon as Element Modifiers (RFC 353) land and are supported by all targeted version. But that may need a while and would also require a refactor for outer HTML.