fix: don't inline css in csp mode. #4411

tbosch · 2013-10-14T19:09:00Z

Also add angular-csp.css to the resulting build.

mary-poppins · 2013-10-14T19:09:03Z

Thanks for the PR!

Contributor signed CLA now or in the past
- If you just signed, leave a comment here with your real name
PR's commit messages follow the commit message format

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

vojtajina · 2013-10-14T21:20:01Z

lib/grunt/utils.js

@@ -70,12 +71,17 @@ module.exports = {


  addStyle: function(src, styles, minify){


I would change the addStyle function to this:

addStyle: function(src, styles, minify) { return styles.reduce(function(result, file) { var css = fs.readFileSync(file).toString(); result.concatenatedCss += css + '\n'; result.js += processCSS(css, minify); return result; }, {js: src, concatenatedCss: ''}); function processCSS(css, minify) { // minify css = css .replace(/\r?\n/g, '') .replace(/\/\*.*?\*\//g, '') .replace(/:\s+/g, ':') .replace(/\s*\{\s*/g, '{') .replace(/\s*\}\s*/g, '}') .replace(/\s*\,\s*/g, ',') .replace(/\s*\;\s*/g, ';'); // escape for JS css = css .replace(/\\/g, '\\\\') .replace(/'/g, "\\'") .replace(/\r?\n/g, '\\n'); return "if (!document.securityPolicy || !document.securityPolicy.isActive) { angular.element(document).find('head').prepend('<style type=\"text/css\">" + css + "</style>'); }"; } },

vojtajina · 2013-10-14T21:20:24Z

LGTM

tbosch · 2013-10-15T00:04:37Z

@vojtajina Thanks for the review.

Actually tested the condition document.securityPolicy in Firefox (24.0), Chrome (30.0) and Chrome Canary (32.0). And none of these seem to support it. To check, I uncommented util.csp() in the Gruntfile and created a dummy html file like this:

<!DOCTYPE html>
<html>
    <head>
        <script src="../build/angular.js"></script>
    </head>
</html>

This shows warnings in the javascript console regarding csp, and when I check document.securityPolicy manually it is undefined (also "securityPolicy" in document) returns undefined. If this does not work, the line in $sniffer also does not seem to work either...

tbosch · 2013-10-15T00:59:05Z

Here is an idea for another solution, as checking document.securityPolicy is not working:

when using CSP, people have to add angular-csp.css (which contains the angular built-in styles for ngCloak, ...) manually before they add angular.js
angular.js checks if there are styles for ngCloak. If so, it does not add styles itself.
Otherwise, it tries to add the styles and checks if it succeeded. If not, it logs a nice error telling people that they are probably using CSP and that they should add the angular-csp.css manually before angular.js to their html file.

Integrating this with the ng-csp directive or $sniffer.csp property does not work as waiting until angular finished it's bootstrap is too late to add the styles to the document, specially for ngCloak.

@IgorMinar What do you think about this?

IgorMinar · 2013-10-15T22:04:54Z

I created #4444 which fixes some of the issues also addressed in this PR. I think that we should merge #4444 first as it fixes a more general issue with CSP and then rebase this PR on top of master and get it in. The only colision should be in the grunt file which constructs the javascript string that injects the stylesheet.

IgorMinar · 2013-10-15T22:22:42Z

lib/grunt/utils.js

@@ -91,7 +97,7 @@ module.exports = {
        .replace(/\\/g, '\\\\')
        .replace(/'/g, "\\'")
        .replace(/\r?\n/g, '\\n');
-      return "angular.element(document).find('head').prepend('<style type=\"text/css\">" + css + "</style>');";
+      return "if (!document.securityPolicy || !document.securityPolicy.isActive) { angular.element(document).find('head').prepend('<style type=\"text/css\">" + css + "</style>'); }";


I made some changes that will make the changes on this line obsolte. see #4444

Also add `angular-csp.css` to the resulting build.

tbosch · 2013-10-16T00:39:40Z

Refactored according to @vojtajina idea using reduce, a little differently though:

  addStyle: function(src, styles, minify){
    styles = styles.reduce(processCSS.bind(this), {
      js: [src],
      css: []
    });
    return {
      js: styles.js.join('\n'),
      css: styles.css.join('\n')
    };

    function processCSS(state, file) {
      var css = fs.readFileSync(file).toString(),
        js;
      state.css.push(css);

      if(minify){
        css = css
          .replace(/\r?\n/g, '')
          .replace(/\/\*.*?\*\//g, '')
          .replace(/:\s+/g, ':')
          .replace(/\s*\{\s*/g, '{')
          .replace(/\s*\}\s*/g, '}')
          .replace(/\s*\,\s*/g, ',')
          .replace(/\s*\;\s*/g, ';');
      }
      //escape for js
      js = css
        .replace(/\\/g, '\\\\')
        .replace(/'/g, "\\'")
        .replace(/\r?\n/g, '\\n');
      js = "if (!document.securityPolicy || !document.securityPolicy.isActive) { angular.element(document).find('head').prepend('<style type=\"text/css\">" + js + "</style>'); }";
      state.js.push(js);

      return state;
    }
  },

When we refactored , we broke the csp mode because the previous implementation relied on the fact that it was ok to lazy initialize the .csp property, this is not the case any more. Besides, we need to know about csp mode during bootstrap and avoid injecting the stylesheet when csp is active, so I refactored the code to fix both issues. PR angular#4411 will follow up on this commit and add more improvements. Closes angular#917 Closes angular#2963 Closes angular#4394 Closes angular#4444 BREAKING CHANGE: triggering ngCsp directive via `ng:csp` attribute is not supported any more. Please use data-ng-csp instead.

When we refactored , we broke the csp mode because the previous implementation relied on the fact that it was ok to lazy initialize the .csp property, this is not the case any more. Besides, we need to know about csp mode during bootstrap and avoid injecting the stylesheet when csp is active, so I refactored the code to fix both issues. PR #4411 will follow up on this commit and add more improvements. Closes #917 Closes #2963 Closes #4394 Closes #4444 BREAKING CHANGE: triggering ngCsp directive via `ng:csp` attribute is not supported any more. Please use data-ng-csp instead.

IgorMinar · 2013-10-22T22:35:47Z

landed as a86cf20

When we refactored , we broke the csp mode because the previous implementation relied on the fact that it was ok to lazy initialize the .csp property, this is not the case any more. Besides, we need to know about csp mode during bootstrap and avoid injecting the stylesheet when csp is active, so I refactored the code to fix both issues. PR angular#4411 will follow up on this commit and add more improvements. Closes angular#917 Closes angular#2963 Closes angular#4394 Closes angular#4444 BREAKING CHANGE: triggering ngCsp directive via `ng:csp` attribute is not supported any more. Please use data-ng-csp instead.

tbosch closed this Oct 14, 2013

tbosch reopened this Oct 14, 2013

tbosch mentioned this pull request Oct 14, 2013

Latest 1.2 does not work with ng-csp mode. #4394

Closed

vojtajina reviewed Oct 14, 2013
View reviewed changes

IgorMinar mentioned this pull request Oct 15, 2013

fix(csp): fix csp auto-detection and stylesheet injection #4444

Closed

IgorMinar reviewed Oct 15, 2013
View reviewed changes

fix: don't inline css in csp mode.

dca87c9

Also add `angular-csp.css` to the resulting build.

IgorMinar closed this Oct 22, 2013

tbosch deleted the issue4394 branch February 6, 2014 19:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: don't inline css in csp mode. #4411

fix: don't inline css in csp mode. #4411

tbosch commented Oct 14, 2013

mary-poppins commented Oct 14, 2013

vojtajina Oct 14, 2013

vojtajina commented Oct 14, 2013

tbosch commented Oct 15, 2013

tbosch commented Oct 15, 2013

IgorMinar commented Oct 15, 2013

IgorMinar Oct 15, 2013

tbosch commented Oct 16, 2013

IgorMinar commented Oct 22, 2013

		@@ -70,12 +71,17 @@ module.exports = {


		addStyle: function(src, styles, minify){

fix: don't inline css in csp mode. #4411

fix: don't inline css in csp mode. #4411

Conversation

tbosch commented Oct 14, 2013

mary-poppins commented Oct 14, 2013

vojtajina Oct 14, 2013

Choose a reason for hiding this comment

vojtajina commented Oct 14, 2013

tbosch commented Oct 15, 2013

tbosch commented Oct 15, 2013

IgorMinar commented Oct 15, 2013

IgorMinar Oct 15, 2013

Choose a reason for hiding this comment

tbosch commented Oct 16, 2013

IgorMinar commented Oct 22, 2013