fix(modal): Fix use of inline styles for Content Security Policy#310
Open
farrago wants to merge 1 commit intoyalabot:masterfrom
Open
fix(modal): Fix use of inline styles for Content Security Policy#310farrago wants to merge 1 commit intoyalabot:masterfrom
farrago wants to merge 1 commit intoyalabot:masterfrom
Conversation
### Summary Inline styles are not allowed under CSP, so this replaces the inline styles used by `$modalStack` with calls to `angular.element.css()` ### Details `$modalStack.open` creates two divs directly with angular.element, and both divs have inline style elements (i.e. `'<div style="...">'`). If [Content Security Policy](https://developer.mozilla.org/en-US/docs/Web/Security/CSP) (CSP) is enabled, these inline styles cannot be applied and cause errors. e.g. in chrome: > [Report Only] Refused to apply inline style because it violates the following Content Security Policy directive: "default-src 'self'". Either the 'unsafe-inline' keyword, a hash ('sha256-xHgdoELGuMXNdMeJ4PQkbzZpH1rlmZwpY3b56d2ZvpI='), or a nonce ('nonce-...') is required to enable inline execution. Note also that 'style-src' was not explicitly set, so 'default-src' is used as a fallback. This is fixed by replacing the inline styles with calls to `angular.element.css()` (which boils down to `HTMLElement.style.foo = bar;`) as this does not trigger the inline styles violation. - For the //faux// modal div, this is simply done immediately. - For the temporary div that is replaced by the `modal-window`, it is a bit more complex: - If you just apply to this temp div using .css() it still fails when the `modal-window` directive replaces it because Angular merges the attributes of the temp div and the new one. The temp div has an inline style (set by .css()), and trying to apply this `style='...'` as an attribute to the new div causes the same inline execution error. - Instead we pass the top position through a custom attribute (`mm-top`), then the `modal-window` applies the value to itself using `element.css()`. ### Setup Browser: Chrome `52.0.2743.116 m` (up to date), plus same result in Firefox, Edge, etc. OS: Windows 10 Server: NodeJS 4.4.3 CSP Headers: `content-security-policy-report-only: default-src 'self'; report-uri /api/v0/csp-report` (This reports but doesn't prevent violations of the policy while debugging rules). ### Notes As per the error reported by Chrome, this could theoretically be worked around by using `'unsafe-inline'` but this is, as named, unsafe. It could also be worked around with a hash for each case but that isn't well supported by browsers yet, and there are a lot of different cases that would need a hash for each one. Nonces are even harder to use with library code. Test Plan: - Run all the unit tests and check they all pass - Test the library in my own project and check no CSP warnings are given - Open and close some modals at various window scroll positions and check they still position correctly
Author
|
I should mention that the default templates also use inline styles which will cause CSP violations. For a fully CSP-compliant result you will also have to use your own replacement templates that don't use inline styles and instead set the styling through your application's own CSS. |
farrago
added a commit
to farrago/angular-foundation-bower
that referenced
this pull request
Aug 25, 2016
Fixes some CSP issues. See yalabot/angular-foundation#310
This file contains hidden or 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
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.
Summary
Inline styles are not allowed under CSP, so this replaces the inline styles used by
$modalStackwith calls toangular.element.css()Details
$modalStack.opencreates two divs directly with angular.element, and both divs have inline style elements (i.e.'<div style="...">').If Content Security Policy (CSP) is enabled, these inline styles cannot be applied and cause errors. e.g. in chrome:
This is fixed by replacing the inline styles with calls to
angular.element.css()(which boils down toHTMLElement.style.foo = bar;) as this does not trigger the inline styles violation.modal-window, it is a bit more complex:modal-windowdirective replaces it because Angular merges the attributes of the temp div and the new one. The temp div has an inline style (set by .css()), and trying to apply thisstyle='...'as an attribute to the new div causes the same inline execution error.mm-top), then themodal-windowapplies the value to itself usingelement.css().Setup
Browser: Chrome
52.0.2743.116 m(up to date), plus same result in Firefox, Edge, etc.OS: Windows 10
Server: NodeJS 4.4.3
CSP Headers:
content-security-policy-report-only: default-src 'self'; report-uri /api/v0/csp-report(This reports but doesn't prevent violations of the policy while debugging rules).Notes
As per the error reported by Chrome, this could theoretically be worked around by using
'unsafe-inline'but this is, as named, unsafe. It could also be worked around with a hash for each case but that isn't well supported by browsers yet, and there are a lot of different cases that would need a hash for each one. Nonces are even harder to use with library code.Test Plan: