When inline scripts are used on a page whose CSP does not allow them (or when eval() is used), the error message posted to the console are unclear: " Warning: CSP: Directive "violated base restriction: Inline Scripts will not execute" violated " We need to fix this to be clearer in the console. Perhaps we can aim for something like this: " Warning: CSP violation: "attempted to execute inline script" "

Can we get line numbers, too?

Jesse: that's bug 600584

We should update these messages for the new 1.0 parser, but might want to wait until the old parser is deprecated -- this way we can (without making the reporting code logic overly complex) easily report inline and eval violations as part of the script-src directive being violated. e.g.: " Warning: CSP: Directive "script-src foo.com" violated, attempted to execute inline script with no 'unsafe-inline' keyword present. " or " Warning: CSP: Directive "script-src foo.com" violated, attempted to compile string into script with no 'unsafe-eval' keyword present. "

Taking this as a Q2 thing

The whole file was laid out to prefix warnings and errors with a fixed string that was out of l10n's scope. I redesigned the logging functions for error and warnings to call a common log() function, which does a localizable prefixing (assuming the prefixing is actually required). Next step would be to actually change the messaging. L10n Axel told me to change the identifier in the properties file when changing the text, so it shows up as a new thing on their side. This sentence serves as a reminder for myself. WiP1 attached :-) Can you take a look or suggest somebody else to give me some feedback? Be nice, this is my first patch ever, so it might contain some newbie mistakes.

Comment on attachment 745179 [details] [diff] [review] wip 1 Review of attachment 745179 [details] [diff] [review]: ----------------------------------------------------------------- I always try to be nice ;) Sid is a good person to ask for CSP feedback as well. Looks like a good start ! ::: browser/devtools/webconsole/webconsole.js @@ +4279,5 @@ > return CATEGORY_CSS; > > case "Mixed Content Blocker": > case "CSP": > + case "Content Security Policy": why do we have both CSP *and* Content Security Policy ? ::: content/base/src/CSPUtils.jsm @@ +891,5 @@ > + log: > + function cspd_log(aFlag, aMsg, aSource, aScriptLine, aLineNum) { > + var prefix = ""; > + if (aFlag == warnFlag) { > + //XXX do we need this? current code has this prefix always.. if we're using the right warn/error flags when we write to the dev console as your patch does, i'm fine with dropping these prefixes - Sid ? @@ +915,5 @@ > } > Components.classes["@mozilla.org/consoleservice;1"] > .getService(Ci.nsIConsoleService).logMessage(consoleMsg); > }, > love that you're deleting/centralizing code here ! i suppose you could roll in the debug stuff to log() if you wanted to as well

Comment on attachment 745179 [details] [diff] [review] wip 1 Review of attachment 745179 [details] [diff] [review]: ----------------------------------------------------------------- Sweet. Please drop the edits to webconsole.js (see my notes below) and update the patch to not need string changes, then flag me for review again. ::: content/base/src/CSPUtils.jsm @@ +877,5 @@ > > /** > + * Sends a message to the error console and web developer console. > + * @param aFlag > + * The nsIScriptError flag constant indicating severity Nit: please delete trailing whitespace. @@ +891,5 @@ > + log: > + function cspd_log(aFlag, aMsg, aSource, aScriptLine, aLineNum) { > + var prefix = ""; > + if (aFlag == warnFlag) { > + //XXX do we need this? current code has this prefix always.. I think dropping the warn v. error prefixes is fine so long as 1) they can be differentiated in the console and 2) it's clear they're CSP messages. That also avoids the need for string changes in this patch, which I endorse. @@ +914,1 @@ > "Content Security Policy"); If you replace "Content Security Policy" here with "CSP", you can avoid the edits in webconsole.js @@ +1948,1 @@ > } If you wanna go crazy and factor even more code you could rewrite this whole function as: function cspError(aCSPRep, aMessage) { (aCSPRep ? aCSPRep : new CSPRep()).log(errorFlag, aMessage); } @@ +1956,4 @@ > } > } > > + Nit: extra newline added (please don't add unnecessary whitespace here). ::: dom/locales/en-US/chrome/security/csp.properties @@ +4,5 @@ > > # CSP Warnings: > +# LOCALIZATION NODE (CspWarn): > +# This is a generic prefix for all CSP warnings. > +CspWarn = CSP Warning: This localization note isn't super helpful... maybe also add something like "CSP is a name and should not be localized" But yeah, maybe we don't need these prefixes since the web console differentiates warnings and errors for us.

This addresses all but Sid's suggestion to leave out string changes. The goal of this bug is to address awful strings, after all. ;) A patch to address the test cases that fail due to this change is in the works and will obsolete this one very soon.

This version includes a fixed test that would have been broken by my patch.

Comment on attachment 749871 [details] [diff] [review] third attempt. now with fixed test Review of attachment 749871 [details] [diff] [review]: ----------------------------------------------------------------- The messages in csp.properties will be prefixed with "CSP:" which makes it clearer what's going on (I think/hope) I'm wondering if we should even prefix them with "Content Security Policy:" to make it even clearer to developers ? I don't have strong feelings and don't want to bikeshed the text too much though :) Overall this looks great. Let's see what Sid says. ::: content/base/src/CSPUtils.jsm @@ +1946,4 @@ > } > } > > + nit: looks like an extra line of whitespace added here ::: content/base/src/contentSecurityPolicy.js @@ +25,5 @@ > const CSP_TYPE_XMLHTTPREQUEST_SPEC_COMPLIANT = "csp_type_xmlhttprequest_spec_compliant"; > const CSP_TYPE_WEBSOCKET_SPEC_COMPLIANT = "csp_type_websocket_spec_compliant"; > > +const warnFlag = Ci.nsIScriptError.warningFlag; > + Maybe we should just put in errorFlag now as well, in case we need it in the future ? @@ +166,5 @@ > // check that the policy was in fact violated before logging any violations > switch (aViolationType) { > case Ci.nsIContentSecurityPolicy.VIOLATION_TYPE_INLINE_SCRIPT: > if (!this._policy.allowsInlineScripts) > + this._asyncReportViolation('self',null, CSPLocalizer.getStr("inlineScriptBlocked"), super picky nit: space after the , between 'self' and null. you didn't introduce this. @@ +172,5 @@ > aSourceFile, aScriptSample, aLineNum); > break; > case Ci.nsIContentSecurityPolicy.VIOLATION_TYPE_EVAL: > if (!this._policy.allowsEvalInScripts) > + this._asyncReportViolation('self',null, CSPLocalizer.getStr("evalBlocked"), same super picky nit: space after the , between 'self' and null. you didn't introduce this.

Comment on attachment 749871 [details] [diff] [review] third attempt. now with fixed test Review of attachment 749871 [details] [diff] [review]: ----------------------------------------------------------------- I was hoping to avoid string changes since we're going to hit those again when inline style blocking lands. But if we do them in the same release cycle, it should be fine. ::: content/base/src/CSPUtils.jsm @@ +13,5 @@ > const Cu = Components.utils; > const Ci = Components.interfaces; > > +const warnFlag = Ci.nsIScriptError.warningFlag; > +const errorFlag = Ci.nsIScriptError.errorFlag; Please make these consts all caps like the other ones in this file (makes 'em easier to identify later on in the file). Maybe even also declare them down by the other consts. ::: content/base/src/contentSecurityPolicy.js @@ +24,5 @@ > // be removed when our original implementation is deprecated. > const CSP_TYPE_XMLHTTPREQUEST_SPEC_COMPLIANT = "csp_type_xmlhttprequest_spec_compliant"; > const CSP_TYPE_WEBSOCKET_SPEC_COMPLIANT = "csp_type_websocket_spec_compliant"; > > +const warnFlag = Ci.nsIScriptError.warningFlag; Same note here about const naming. And yeah, what ian said about including errorFlag now. ::: dom/locales/en-US/chrome/security/csp.properties @@ +54,5 @@ > +# inline script refers to JavaScript code that is embedded into the HTML document. > +inlineScriptBlocked = An attempt to execute inline scripts has been blocked > +# LOCALIZATION NOTE (evalBlocked): > +# eval is a name and should not be localized. > +evalBlocked = An attempt to call the eval() function has been blocked evalBlocked is also triggered for setTimeout and setInterval as well as a few other cases. Can you rephrase this as "eval or related functionality has been blocked" or something like that?

Ian: I want to be sure the stuff you're working on that requires string changes and this land in the same train so as to not thrash our localizers. Do you think it's doable?

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #12) > Ian: I want to be sure the stuff you're working on that requires string > changes and this land in the same train so as to not thrash our localizers. > Do you think it's doable? I think so. Very close on inline styles.

I have changed the evalBlocked thing to a different l10n name and use this string instead: > scriptFromStringBlocked = An attempt to call JavaScript from a string has been blocked (by calling a function like eval) Do you think this is OK? I'm not too passionate about how the exact wording should be :) On the other hand, I could split this up in a no-string-changes patch which could land today and a yes-string-changes patch that could land later if that's preferred

(In reply to Frederik Braun [:freddyb] from comment #14) > I have changed the evalBlocked thing to a different l10n name and use this > string instead: > > scriptFromStringBlocked = An attempt to call JavaScript from a string has been blocked (by calling a function like eval) How about we get a words person to weigh in on this? Matej: there are lots of JS functions that turn arbitrary strings into executable code. How do we best warn developers that somthing like this was blocked by CSP? My proposal is: "CSP blocked an attempt to execute script from a string (by calling a function like eval)."

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #15) > (In reply to Frederik Braun [:freddyb] from comment #14) > > I have changed the evalBlocked thing to a different l10n name and use this > > string instead: > > > scriptFromStringBlocked = An attempt to call JavaScript from a string has been blocked (by calling a function like eval) > > How about we get a words person to weigh in on this? > > Matej: there are lots of JS functions that turn arbitrary strings into > executable code. How do we best warn developers that somthing like this was > blocked by CSP? > > My proposal is: "CSP blocked an attempt to execute script from a string (by > calling a function like eval)." Unfortunately this is a bit outside my area of expertise (i.e. I don't know what CSP is). The string looks OK to me, but I admit I don't really understand what it's communicating or what the implications of that are. If it's clear and minimally jargony to a person reading it, then I'd say it's fine.

Comment on attachment 750377 [details] [diff] [review] wip 4 - addressing the second review notes Review of attachment 750377 [details] [diff] [review]: ----------------------------------------------------------------- This fixes a few strings, but other strings in csp.properties are also not-great. I'd suggest that you either add an explicit new common logging prefix, or ensure all the strings in csp.properties make their purpose obvious (eg, if someone walked up to an average web developer, would they get the gist of its meaning and be able to use Google to search for more info?). And I don't think there's any reason to be overly concerned about string length, so abbreviating to "CSP" just makes the messages less clear. eg # LOCALIZATION NOTE: do not localize "Content Security Policy" (just localize "warning") # %1$S will be one of the other strings in this file cspPrefix = Content Security Policy warning: %1$S (I now see Ian made this exact suggestion back in comment 10 :) ::: dom/locales/en-US/chrome/security/csp.properties @@ +5,5 @@ > # CSP Warnings: > +# LOCALIZATION NOTE (CSPViolation): > +# %1$S is the reason why the resource has not been loaded. > +# CSP is a name and should not be localized. > +CSPViolation = A resource has not been loaded: %1$S Might help to note that it's was the _page's_ policy. The resource at %1$S was blocked from loading by the page's Content Security Policy. or Content Security Policy warning: The resource at %1$S was blocked from loading by the page's settings. @@ +9,5 @@ > +CSPViolation = A resource has not been loaded: %1$S > +# LOCALIZATION NOTE (CSPViolationWithURI): > +# %1$S is the URI of the resource which violated the directive. > +# %2$S is the directive that has been violated. > +CSPViolationWithURI = The resource %1$S has not been loaded, because it violates the directive "%2$S" This seems jargony. Suggestion: The resource at %1$S was blocked from loading by the page's Content Security Policy ("%2$S"). or Content Security Policy warning: The resource at %1$S was blocked from loading by the page's settings ("%2$S").

Some other random comments on other strings. I also wonder if all of these actually need to be localized (especially the latter half of the file). It's probably fine to log a localized string like "couldn't parse URI: x", and leave the details unlocalized or as debug-only messages? It's not a big deal, but these messages just seem far more fine-grained that what's usually localized. (Especially considering console messages often are not localized at all!) # LOCALIZATION NOTE (triedToSendReport): # %1$S is the URI we attempted to send a report to. triedToSendReport = Tried to send report to invalid URI: "%1$S" # LOCALIZATION NOTE (errorWas): # %1$S is the error resulting from attempting to send the report errorWas = error was: "%1$S" That's not good l10n practice, you should have a full triedToSendReportWithError string (ie, where the error is %2$S). # LOCALIZATION NOTE (couldNotProcessUnknownDirective): # %1$S is the unknown directive couldNotProcessUnknownDirective = Couldn't process unknown directive '%1$S' Just "unknown directive %1$S"? (is the problem that some kind of processing failed, or that it's an unknown directive?) # LOCALIZATION NOTE (doNotUnderstandOption): # %1$S is the option that could not be understood doNotUnderstandOption = don't understand option '%1$S'. Ignoring it. "Ignoring unknown option %1$S"? # LOCALIZATION NOTE (notETLDPlus1): # %1$S is the ETLD of the report URI that could not be used notETLDPlus1 = can't use report URI from non-matching eTLD+1: %1$S "The report URI (%1$S) must be from the same eTLD+1." (eTLD+1 is pretty jargony, but I guess it's at least easily Googlable) # LOCALIZATION NOTE (notSameScheme): # %1$S is the report URI that could not be used notSameScheme = can't use report URI with different scheme from originating document: %1$S "The report URI (%1$S) must use the same scheme as the originating document." ...maybe s/scheme/protocol? ...maybe list the page's scheme/protocol too? Makes the difference clear. pageCannotSendReportsTo = page on %1$S cannot send reports to %2$S That's pretty generic. Can we say anything about _why_ when this happens? # Don't translate "allow" and "default-src" as they are keywords and part of # the CSP protocol syntax. allowDirectiveIsDeprecated = allow directive is deprecated, use the equivalent default-src directive instead I'd quote the directives, just like you did in the l10n note. :) # LOCALIZATION NOTE (nonMatchingHost): # %1$S is the URI host that does not match nonMatchingHost = can't fetch policy uri from non-matching hostname: %1$S Doesn't match what? # LOCALIZATION NOTE (nonMatchingPort): # %1$S is the URI port that does not match nonMatchingPort = can't fetch policy uri from non-matching port: %1$S Ditto. # LOCALIZATION NOTE (nonMatchingScheme): # %1$S is the URI scheme that does not match nonMatchingScheme = can't fetch policy uri from non-matching scheme: %1$S Ditto. cspSourceNotURI = Provided argument is not an nsIURI argumentIsNotString = Provided argument is not a string selfDataNotProvided = Can't use 'self' if self data is not provided Presumably these are internal errors that "shouldn't" happen in the wild? These could be more detailed. OTOH, there's probably no reason to localize them. # LOCALIZATION NOTE (notIntersectPort): # %1$S is one source we tried to intersect # %2$S is the other notIntersectPort = Could not intersect %1$S with %2$S due to port problems. Is this "intersecting" something that's part of CSP, or an internal detail? (jargon or not?)

Indeed we should change some other messages. But this bug only refers to "base restrictions" (inline JavaScript being blocked by default, for example) but my patch already covers other things. How should we go forward with this? Should I split it up into my logging cleanup and then address *all* string changes in a separate bug? - I suppose this is better than throwing lots of small string changes at l10n.. Disclaimer: Splitting it up would also support my goal in getting something patched and landed by the end of June ;)

I think it would be fine to either (1) fix all the strings _and_ tweak the logging APIs or (2) fix all the strings here but first tweak the logging APIs in some other bug.

This is my next stab at this and I have tried addressing all comments so far. A problem I have is that the awful strings like "inline script base restriction" are also what's being sent in the violation report. I don't think I should change it then..I mean, it's already wrong but I have the feeling that we should probably get this right before we start with string changes. I suppose Garrett knows the code's state best. Would you mind providing some feedback?

Comment on attachment 788903 [details] [diff] [review] bug-607067.patch Review of attachment 788903 [details] [diff] [review]: ----------------------------------------------------------------- These messages look a lot better! Most of my advice is stylistic, or suggests rewording things for clarity. The only mistake you made was in logViolationDetails, where you sent an "inlineScriptBlocked" error string to _asyncReportViolation for an inline *styles* violation. Re: Comment 19, is it correct that you're taking the "split logging changes and string cleanup" approach in this latest patch (where this patch is mostly string cleanup)? ::: content/base/src/CSPUtils.jsm @@ +365,1 @@ > [gETLDService.getBaseDomain(uri)])); Line up indentation with prevoius argument (you didn't introduce this). Same fix needed in fromStringSpecCompliant. @@ +365,5 @@ > [gETLDService.getBaseDomain(uri)])); > continue; > } > if (!uri.schemeIs(selfUri.scheme)) { > + cspWarn(aCSPR, CSPLocalizer.getFormatStr("reportURInotSameScheme", "reportURInotSameSchemeAsSelf" might be clearer. Ditto for port warning below. Also needed in fromStringSpecCompliant. @@ +2054,5 @@ > try { > result = this.stringBundle.GetStringFromName(aName); > } > catch (ex) { > + dump(aName, ex, ex.stack); You should probably remove this from the final patch (if it is for debugging, as I expect it is). ::: content/base/src/contentSecurityPolicy.js @@ +176,5 @@ > // check that the policy was in fact violated before logging any violations > switch (aViolationType) { > case Ci.nsIContentSecurityPolicy.VIOLATION_TYPE_INLINE_STYLE: > if (!this._policy.allowsInlineStyles) > + this._asyncReportViolation('self', null, CSPLocalizer.getStr("inlineScriptBlocked"), This is an inline style, not script, violation. You should report a distinct message.

Comment on attachment 788903 [details] [diff] [review] bug-607067.patch Review of attachment 788903 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/locales/en-US/chrome/security/csp.properties @@ +4,5 @@ > > # CSP Warnings: > +# LOCALIZATION NOTE (CSPViolation): > +# %1$S is the reason why the resource has not been loaded. > +# CSP is a name and should not be localized. Not sure why you mention CSP here, since it's never used inside strings. @@ +9,5 @@ > +CSPViolation = A resource was blocked from loading by the page's settings: %1$S > +# LOCALIZATION NOTE (CSPViolationWithURI): > +# %1$S is the URI of the resource which violated the directive. > +# %2$S is the directive that has been violated. > +CSPViolationWithURI = The resource at %2$S was blocked from loading by the page's settings ("%1$S"). Looking at the code, I would say that the localization notes are inverted. @@ +35,3 @@ > # LOCALIZATION NOTE (notSamePort): > # %1$S is the report URI that could not be used > +reportURInotSamePort = The report URI (%1$S) must use the same port as the originating document. If you prefer you could also squash the last two l10n comments like this # LOCALIZATION NOTE (notSameScheme, notSamePort): %1$S is the report URI that could not be used

(In reply to Garrett Robinson [:grobinson] from comment #22) > Re: Comment 19, is it correct that you're taking the "split logging changes > and string cleanup" approach in this latest patch (where this patch is > mostly string cleanup)? > Yes, I finished the logging changes in bug 879316 and am now addressing the strings.

Thanks for the comments, Francesco and Garrett. They are addressed in this patch. The only remaining problem I have is that the string changes for inline scripts/styles are affecting the violation reports we send. This could break stuff. Do you have a suggestion how to change that in a clean fashion, Garett?

Comment on attachment 789477 [details] [diff] [review] bug-607067.patch Review of attachment 789477 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/locales/en-US/chrome/security/csp.properties @@ +54,5 @@ > +# inline style refers to CSS code that is embedded into the HTML document. > +inlineStyleBlocked = An attempt to apply inline style sheets has been blocked > +# LOCALIZATION NOTE (scriptFromStringBlocked): > +# eval is a name and should not be localized. > +scriptFromStringBlocked = An attempt to call JavaScript from a string has been blocked (by calling a function like eval) I would reword this to "An attempt to call Javascript from a string (by a function like eval) has been blocked."

(In reply to Frederik Braun [:freddyb] from comment #25) > The only remaining problem I have is that the string changes for inline > scripts/styles are affecting the violation reports we send. This could break > stuff. Do you have a suggestion how to change that in a clean fashion, > Garett? AFAIK, the only things this would break would be unit tests. Have you run the CSP unit tests with your patch? A push to try wouldn't hurt either. If any tests break, it's probably becasue they're testing for specific messages, and you can just change the tests to look for your new messages. In terms of breakage for people using CSP violation reports, I would say we do not have to worry about that. See the CSP 1.0 spec, Section 5.2, which is nonnormative. Just a thought - it might be nice for us to report the directive that caused the violation (i.e., report script-src for an inline script violation) instead of a custom message.

Confirming my earlier thought about reporting the specific directive that caused the violation (instead of the custom messages that we currently report), see the webappsec CSP test suite: http://webappsec-test.info/web-platform-tests/CSP/script-src/CSP_1_1.php For the test "Inline script should not run with policy "default-src 'self'"", we get the following test failure: "violated-directive value of "inline script base restriction" did not match default-src." Our tests passing % on the webappsec suite is abysmal right now. It looks like a lot of these (at least the first few that I looked at) are due to this disagreement over how to report "base violations". I think the way webappsec doing it is correct, and we should switch to doing that. In relation to your current patch - your changes to cspWarn messages can stay. I think you should rework the violation reporting so it actually properly determines which specific directive was violated, and reports that.

I changed the message according to comment 25.

Try push (thanks for gabor): https://tbpl.mozilla.org/?tree=Try&rev=05755d81076c (the bug id is wrong in the attachment, so it links to the wrong bug. But it is the correct patch. My next revision will fix this ;))

Rebased and fixed the test breakage (browser_webconsole_bug_770099_violation.js).

Garrett, should I have carried over the r+? I wouldn't mind if you looked over this patch again, as the one in bug 909241 builds on top of this

Comment on attachment 795414 [details] [diff] [review] bug-607067.patch Review of attachment 795414 [details] [diff] [review]: ----------------------------------------------------------------- I do not like "was blocked from loading" in the messages. Can we make it active voice? I'll give some suggestions below. I know dolske approved this language, so take the string bikeshedding parts of my review as "advice" and not requirements. ::: content/base/src/CSPUtils.jsm @@ +631,5 @@ > continue; > } > if (uri.port && uri.port !== selfUri.port) { > cspWarn(aCSPR, > + CSPLocalizer.getFormatStr("reportURInotSamePortAsSelf", While you're in this bit of the code, could you remove the trailing whitespace from the lines near the ones you modify? ::: content/base/src/contentSecurityPolicy.js @@ +184,1 @@ > 'violated base restriction: Inline Stylesheets will not apply', Do you want to change these strings while you're at it? The one's about violating base restrictions are no longer aligned with what's localized. Maybe this would be a good place to clarify this string (or document what it's for - I believe it's only used in tests or consumers of the "csp-on-violate-policy" notification topic). ::: dom/locales/en-US/chrome/security/csp.properties @@ +4,5 @@ > > # CSP Warnings: > +# LOCALIZATION NOTE (CSPViolation): > +# %1$S is the reason why the resource has not been loaded. > +CSPViolation = A resource was blocked from loading by the page's settings: %1$S "was blocked from loading" is not clear enough to me. Can we use something like: "The page's settings blocked the loading of a resource: %1$S" @@ +10,3 @@ > # %1$S is the directive that has been violated. > # %2$S is the URI of the resource which violated the directive. > +CSPViolationWithURI = The resource at %2$S was blocked from loading by the page's settings ("%1$S"). Same here: "The page's settings blocked the loading of a resource at %2$S (%1$S)"

I want to agree on the active voice changes. The observer subjects will change in a tiny, subsequent bug (bug 912413). All of your other nits have been addressed. I will carry over the r+.

(Carrying over r+) Rebased and successfully ran on try: https://tbpl.mozilla.org/?tree=Try&rev=27690b1bf1c7

https://hg.mozilla.org/integration/mozilla-inbound/rev/58305759794a Friendly reminder that your commit message should be stating what the patch is actually doing, not re-stating the bug summary.