Closed Bug 1304645 Opened 3 years ago Closed Last year

Pass individual CSP errors as categories to web console error messages

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox62 --- fixed
firefox63 --- fixed

People

(Reporter: fscholz, Assigned: ckerschb)

References

(Blocks 1 open bug)

Details

(Whiteboard: [domsecurity-active])

User Story

As a user I want to click through on CSP errors to read about that specific error type.

Attachments

(1 file, 3 obsolete files)

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/
Sounds reasonable to me - putting in the backlog. It's already blocking the right Bug where we track CSP console message improvements (Bug 1242016).
Priority: -- → P3
Whiteboard: [domsecurity-backlog1]
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?
Flags: needinfo?(fscholz)
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.
Flags: needinfo?(fscholz)
(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.
Flags: needinfo?(odvarko)
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?
User Story: (updated)
Flags: needinfo?(ckerschb)
Summary: Add [Learn more] for CSPViolation message and point to MDN docs → Add [Learn more] for CSPViolation message and point to MDN docs per error
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.
Assignee: nobody → ckerschb
Flags: needinfo?(ckerschb)
Priority: P3 → P2
Whiteboard: [domsecurity-backlog1] → [domsecurity-active]
@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
Flags: needinfo?(hkirschner)
Attachment #8992964 - Flags: review?(amarchesini)
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
Attachment #8992964 - Flags: review?(amarchesini)
(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.
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.
Status: NEW → ASSIGNED
> 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.
Flags: needinfo?(hkirschner)
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 *'
Summary: Add [Learn more] for CSPViolation message and point to MDN docs per error → Pass individual CSP errors as categories to web console error messages
baku, please see previous comment as well - thanks!
Attachment #8985519 - Attachment is obsolete: true
Attachment #8992964 - Attachment is obsolete: true
Attachment #8993388 - Flags: review?(amarchesini)
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.
Attachment #8993388 - Flags: review?(amarchesini) → review+
(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.
Attachment #8993388 - Attachment is obsolete: true
Attachment #8993400 - Flags: review?(amarchesini)
Attachment #8993400 - Flags: review?(amarchesini) → review+
Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3ac68d9ead9
Pass individual CSP errors as categories to web console error messages. r=baku
This patch got pushed together with Bug 1475073 which caused a test failure, see:
https://bugzilla.mozilla.org/show_bug.cgi?id=1475073#c11
Flags: needinfo?(ckerschb)
Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b5f7ac22733
Pass individual CSP errors as categories to web console error messages. r=baku
https://hg.mozilla.org/mozilla-central/rev/6b5f7ac22733
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Hi Harald, in bug 1475073 you mention this bug is needed for uplift of that bug. Can you request uplift, please?
Flags: needinfo?(hkirschner)
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.
Flags: needinfo?(hkirschner)
Attachment #8993400 - Flags: approval-mozilla-beta?
Flags: needinfo?(odvarko)
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.
Attachment #8993400 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.