1304645 - Pass individual CSP errors as categories to web console error messages

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P2)

Description

[Florian Scholz (Open Web Docs)]

The CSPViolation error that is thrown seems to be quite complicated [1] for web developers. Let's add a [Learn more] link pointing to MDN for this which has been very successful for JS errors [2].

Following up on bug 1296027 comment 10

> 2) If we set the errorMessageName field on these errors and modified the
> errordocs actor with a doc link we could have a 'learn more' link show up to
> point to MDN: https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/errordocs.js.  
> Florian (cc'ed) has been working on this for other errors and it could be a nice enhancement.


Looks like there are two kinds of messages here which could both point to the same MDN doc:

CSPViolation = The page’s settings blocked the loading of a resource: %1$S
CSPViolationWithURI = The page’s settings blocked the loading of a resource at %2$S (“%1$S”).

We are making progress on MDN to document HTTP and CSP, but at this point there is no dedicated page for this error yet. However, I will happily help to write up a page for this, if anyone wants to implement a [Learn more] link to the console now.

[1] bug 1267389, bug 1279894, bug 1296027
[2] https://hacks.mozilla.org/2016/06/helping-web-developers-with-javascript-errors/

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Sounds reasonable to me - putting in the backlog. It's already blocking the right Bug where we track CSP console message improvements (Bug 1242016).

Comment 2

[Brian Grinstead [:bgrins]]

Attachment: Bug 1304645 - Add [Learn more] for CSPViolation message and point to MDN docs

Review commit: https://reviewboard.mozilla.org/r/251126/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/251126/

Comment 3

[Brian Grinstead [:bgrins]]

It's probably not pointing to the right page, but pushed up a change that adds the Learn More link. Florian, is there a particular page we should be pointing to?

Comment 4

[Florian Scholz (Open Web Docs)]

Thanks so much for coming back to this! [Learn more] links continue to be a huge win for MDN, Devtools and developers troubleshooting. Happy to see more of them added. Some clarifications needed here:

Will this add [Learn more] to all CSP-related warnings and errors? (by "all" I mean the ones listed here https://dxr.mozilla.org/mozilla-central/source/dom/locales/en-US/chrome/security/csp.properties) or is this just adding [Learn more] for CSPViolation and CSPViolationWithURI?

I think [Learn more] links are most effective when it can point to a specific troubleshooting page for the exact situation the developer is in. For example, for things like "strictDynamicButNoHashOrNonce = Keyword ‘strict-dynamic’ within “%1$S” with no valid nonce or hash might block all scripts from loading" we could offer more dedicated help on a specific page.

That said, I'm a unsure if it is a good idea to point to https://developer.mozilla.org/en-US/docs/Web/HTTP/CSP for all CSP warnings/errors. For JS, we have https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors and sub pages and also aren't just pointing to say https://developer.mozilla.org/en-US/docs/Web/JavaScript.

Let me know what you think.

Comment 5

[Brian Grinstead [:bgrins]]

(In reply to Florian Scholz [:fscholz] (MDN) from comment #4)
> Thanks so much for coming back to this! [Learn more] links continue to be a
> huge win for MDN, Devtools and developers troubleshooting. Happy to see more
> of them added. Some clarifications needed here:
> 
> Will this add [Learn more] to all CSP-related warnings and errors? (by "all"
> I mean the ones listed here
> https://dxr.mozilla.org/mozilla-central/source/dom/locales/en-US/chrome/
> security/csp.properties) or is this just adding [Learn more] for
> CSPViolation and CSPViolationWithURI?

Right now it will be added to anything calling nsCSPContext::logToConsole (https://searchfox.org/mozilla-central/rev/d0a41d2e7770fc00df7844d5f840067cc35ba26f/dom/security/nsCSPContext.cpp#775) since the Learn More links are keyed on error category and everything there passes "CSP" as the category.

To change that from the platform side it shouldn't be too difficult, we'd have to change callers to pass a different category in depending on the error: https://searchfox.org/mozilla-central/search?q=nsCSPContext%3A%3AlogToConsole&path=. Then for the frontend we'd update this patch to set different URLs for the different categories.

> I think [Learn more] links are most effective when it can point to a
> specific troubleshooting page for the exact situation the developer is in.
> For example, for things like "strictDynamicButNoHashOrNonce = Keyword
> ‘strict-dynamic’ within “%1$S” with no valid nonce or hash might block all
> scripts from loading" we could offer more dedicated help on a specific page.
>
> That said, I'm a unsure if it is a good idea to point to
> https://developer.mozilla.org/en-US/docs/Web/HTTP/CSP for all CSP
> warnings/errors. For JS, we have
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors and
> sub pages and also aren't just pointing to say
> https://developer.mozilla.org/en-US/docs/Web/JavaScript.
> 
> Let me know what you think.

I agree it'd be better to have different categories for each message with an individual link (maybe an MDN page for "CSP Errors" or something, and a section for each one that we hook up through the Learn More links?).

Also just to be aware before we start documenting individual error messages - we've discussed improving the messages to include more information about what was blocked and why. I think Honza was starting to look into the existing messages and how they could be improved, so setting needinfo to make sure we are all on the same page here.

Comment 6

[:Harald Kirschner :digitarald]

Updated the title to reflect the per-type link nature.

We need categories per error type, the MDN team owns creating the content.

Christoph – who could own adding specific categories for each error type?

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Adding conversation I had with bgrins on slack:

bgrinstead [6:25 PM]
i believe what we want is for different types of messages to point to different MDN pages on the [Learn More] links in the console (rather than just a single page for all “CORS” / “CSP” errors)

in order to do that I think we need to:
1) List out all the individual error types that we want to link to (I believe this is what Harald has done)
2) Create MDN pages (or sections in a single MDN page) for each of them
3) Change the “category” on the platform side for each of these messages (this is what is requested of you)
4) Hook up categories into the learn more link map for devtools


I will provide (3) within this bug tomorrow.

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1304645_change_category_for_csp_errors.patch

@baku: We would like to pass individual CSP errors down to the console as category, so we can link to individual errors on MDN from the console.

@harald: I suppose that should be sufficient or am I missing something?

Currently I am relying on all of the errors from csp.properties [1] which should be sufficient as a differentiating factor, right?

[1] https://dxr.mozilla.org/mozilla-central/source/dom/locales/en-US/chrome/security/csp.properties

Comment 9

[Andrea Marchesini [:baku]]

Comment on attachment 8992964 [details] [diff] [review]
bug_1304645_change_category_for_csp_errors.patch

Review of attachment 8992964 [details] [diff] [review]:
-----------------------------------------------------------------

A couple of things:

1. would be better to have better category names. Such as: "CSP Ignoring Because of Directive" or anything that contains "CSP"
2. we can do something similar to JS errors where we create a MDN URL based on the error code: https://searchfox.org/mozilla-central/source/devtools/server/actors/errordocs.js#12-15

Do we already have MDN pages? What about if we link them directly?

::: dom/html/HTMLFormElement.cpp
@@ +1748,5 @@
>                          EmptyString(), // aSourceFile
>                          EmptyString(), // aScriptSample
>                          0, // aLineNumber
>                          0, // aColumnNumber
> +                        nsIScriptError::warningFlag, category,

NS_LITERAL_CSTRING("upgradeInsecureRequest")

::: dom/security/FramingChecker.cpp
@@ +196,5 @@
>                        EmptyString(), // no scriptsample
>                        0,             // no linenumber
>                        0,             // no columnnumber
>                        nsIScriptError::warningFlag,
> +                      category, innerWindowID,

NS_LITERAL_CSTRING("IgnoringSrcBecauseOfDirective")

::: dom/security/nsCSPContext.cpp
@@ +819,5 @@
>                             uint32_t aSeverityFlag)
>  {
> +  // we are passing aName as the category so we can link to the
> +  // appropriate MDN docs depending on the specific error.
> +   nsCString category(aName);

nsDependentCString category(aName)

::: dom/security/nsCSPUtils.cpp
@@ +170,4 @@
>      rv = error->Init(cspMsg, aSourceName,
>                       aSourceLine, aLineNumber,
>                       aColumnNumber, aFlags,
> +                     category.get(), aFromPrivateWindow);

can you just pass aCategory here?

::: dom/security/nsMixedContentBlocker.cpp
@@ +805,5 @@
>                          EmptyString(), // aSourceFile
>                          EmptyString(), // aScriptSample
>                          0, // aLineNumber
>                          0, // aColumnNumber
> +                        nsIScriptError::errorFlag, category,

NS_LITERAL_CSTRING(...

::: dom/websocket/WebSocket.cpp
@@ +1724,5 @@
>                          EmptyString(), // aSourceFile
>                          EmptyString(), // aScriptSample
>                          0, // aLineNumber
>                          0, // aColumnNumber
> +                        nsIScriptError::warningFlag, category,

here as well

::: netwerk/base/nsNetUtil.cpp
@@ +2930,5 @@
>                                EmptyString(), // aSourceFile
>                                EmptyString(), // aScriptSample
>                                0, // aLineNumber
>                                0, // aColumnNumber
> +                              nsIScriptError::warningFlag, category,

here too

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Andrea Marchesini [:baku] from comment #9)
> Comment on attachment 8992964 [details] [diff] [review]
> bug_1304645_change_category_for_csp_errors.patch
> 
> Review of attachment 8992964 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> A couple of things:
> 
> 1. would be better to have better category names. Such as: "CSP Ignoring
> Because of Directive" or anything that contains "CSP"
> 2. we can do something similar to JS errors where we create a MDN URL based
> on the error code:
> https://searchfox.org/mozilla-central/source/devtools/server/actors/
> errordocs.js#12-15
> 
> Do we already have MDN pages? What about if we link them directly?

Whatever is required, I let Harald answer here first. I am fine either way. But I guess updating csp.properties is out of scope fo this initial phase of the project.

Comment 11

[Andrea Marchesini [:baku]]

I'm not saying we should touch csp.properties. [Learn More] is generated by https://searchfox.org/mozilla-central/source/devtools/server/actors/errordocs.js if the category name matches with what written in that file. We should probably update that file.

Comment 12

[:Harald Kirschner :digitarald]

> 2. we can do something similar to JS errors where we create a MDN URL based on the error code: https://searchfox.org/mozilla-central/source/devtools/server/actors/errordocs.js#12-15
> Do we already have MDN pages? What about if we link them directly?

This would be the next step. I will file a follow bug. They should be drafted in an upcoming MDN sprint.

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

I am renaming this bug and let the required follow up work happen by the devtools team, because the changes here only affect the backend.

(In reply to Andrea Marchesini [:baku] from comment #9)
> ::: dom/security/nsCSPUtils.cpp
> @@ +170,4 @@
> >      rv = error->Init(cspMsg, aSourceName,
> >                       aSourceLine, aLineNumber,
> >                       aColumnNumber, aFlags,
> > +                     category.get(), aFromPrivateWindow);
> 
> can you just pass aCategory here?

I tried, compiler doesn't really like that conversion:
> error: no viable conversion from 'const nsACString' (aka 'const nsTSubstring<char>') to 'const char *'

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1304645_change_category_for_csp_errors.patch

baku, please see previous comment as well - thanks!

Comment 15

[Andrea Marchesini [:baku]]

Comment on attachment 8993388 [details] [diff] [review]
bug_1304645_change_category_for_csp_errors.patch

Review of attachment 8993388 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/html/HTMLFormElement.cpp
@@ +1748,5 @@
>                          EmptyString(), // aScriptSample
>                          0, // aLineNumber
>                          0, // aColumnNumber
> +                        nsIScriptError::warningFlag,
> +                        NS_LITERAL_CSTRING("upgradeInsecureRequest"),

I'm not particularly happy about this category names because they do not refer to CSP.
But we are going to change them when the MDN pages will be ready, I guess.

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1304645_change_category_for_csp_errors.patch

(In reply to Andrea Marchesini [:baku] from comment #15)
> I'm not particularly happy about this category names because they do not
> refer to CSP.
> But we are going to change them when the MDN pages will be ready, I guess.

Actually I thought about that again, and I guess we can do better. What if we prepend "CSP_". I have done that change within nsCSPUtils.cpp - everything else is unchanged!

What do you think? We could do the same for CORS in the other bug.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3ac68d9ead9
Pass individual CSP errors as categories to web console error messages. r=baku

Comment 18

[Andreea Pavel [:apavel]]

Backed out for failing mochitest and wpt 

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=4fbeea69b10d7487b74de22de78065d5b326dca5

Failure logs: https://treeherder.mozilla.org/logviewer.html#?job_id=189130132&repo=mozilla-inbound&lineNumber=3535
https://treeherder.mozilla.org/logviewer.html#?job_id=189130548&repo=mozilla-inbound&lineNumber=7047

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/3539fcf9699308a1a05d5cec055321bb1220b0d5

Comment 19

[Christoph Kerschbaumer [:ckerschb]]

This patch got pushed together with Bug 1475073 which caused a test failure, see:
https://bugzilla.mozilla.org/show_bug.cgi?id=1475073#c11

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b5f7ac22733
Pass individual CSP errors as categories to web console error messages. r=baku

Comment 21

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/6b5f7ac22733

Comment 22

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Hi Harald, in bug 1475073 you mention this bug is needed for uplift of that bug. Can you request uplift, please?

Comment 23

[:Harald Kirschner :digitarald]

Comment on attachment 8993400 [details] [diff] [review]
bug_1304645_change_category_for_csp_errors.patch

Approval Request Comment
[Feature/Bug causing the regression]:
No regression. MDN finished the documentation ahead of time, we just need the right categories in the console to start linking to them.
[User impact if declined]:
No major impact. CORS errors will not have links and remain less actionable
[Is this code covered by automated tests?]:
Yes
[Has the fix been verified in Nightly?]:
Yes
[Needs manual test from QE? If yes, steps to reproduce]: 
No.
[List of other uplifts needed for the feature/fix]:
https://bugzilla.mozilla.org/attachment.cgi?id=1475073
[Is the change risky?]:
Very low risk per :ckerschb
[Why is the change risky/not risky?]:
Covered by tests, only adds existing category texts to error logging.
[String changes made/needed]:
None.

Comment 24

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8993400 [details] [diff] [review]
bug_1304645_change_category_for_csp_errors.patch

One more patch for the dev tools error category links. OK for beta 17/18. This may not make it into the beta 17 build though.

Comment 25

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/31a3bae15c00