1475073 - Pass individual CORS errors as categories to web console error messages

Status: RESOLVED FIXED

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

Description

[:Harald Kirschner :digitarald]

The MDN team is creating CORS error pages that are meant to be linked to from Console: https://github.com/mdn/sprints/issues/64

To link pages to each individual error type the logs need to have individual categories (similar to JS errors).

As the MDN content will be ready soon we should consider getting this uplifted into 62 if it lands in time and turns out low risk.

Comment 1

[:Harald Kirschner :digitarald]

Christoph, follow up on our discussions, this would be a low effort/high impact project to improve messages. Who could help with adding log caregories?

Comment 2

[:Harald Kirschner :digitarald]

The errors that need categories would be the following (from the linked Github issue)

CORSDisabled
CORSDidNotSucceed
CORSOriginHeaderNotAdded
CORSExternalRedirectNotAllowed
CORSRequestNotHttp
CORSMissingAllowOrigin
CORSAllowOriginNotMatchingOrigin
CORSNotSupportingCredentials
CORSMethodNotFound
CORSMissingAllowCredentials
CORSPreflightDidNotSucceed
CORSInvalidAllowMethod
CORSInvalidAllowHeader
CORSMissingAllowHeaderFromPreflight

The easiest approach would be just re-using the localization keys above as categories.

Implementation wise, the key would be handed through from `LogBlockedRequest` to `nsCORSListenerProxy::LogBlockedCORSRequest` (which right now always sends "CORS" as the category).

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Just fore the refernece, I am assigning this bug to myself, similar to:
https://bugzilla.mozilla.org/show_bug.cgi?id=1304645#c7

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1304645_change_category_for_cors_errors.patch

This patch passes all the CORS properties along as categories to the console. Whatever we decide to do within Bug 1304645, we should do the same here with this bug.

Comment 5

[:Harald Kirschner :digitarald]

This looks good.

Within the scope of this bug should also be to actually add the categories. Follow up work to link the bugs to MDN pages should happen in bug 1475391, as far as I understood :bgrins.

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Same as in Bug 1304645 - renaming and only doing backend work within this bug - let's do all frontend/devtools related work in a follow up bug.

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Please note that all of those messages are prefixed with "CORS" anyway.

Comment 8

[Andrea Marchesini [:baku]]

Comment on attachment 8992995 [details] [diff] [review]
bug_1304645_change_category_for_cors_errors.patch

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

::: netwerk/protocol/http/nsCORSListenerProxy.cpp
@@ +85,5 @@
>      return;
>    }
>  
>    nsAutoString msg(blockedMessage.get());
> +  nsCString category(aProperty);

nsDependentCString category(aProperty)

Comment 9

[Pulsebot]

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

Comment 10

[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 11

[Christoph Kerschbaumer [:ckerschb]]

Ah, there is  dom/base/test/test_warning_for_blocked_cross_site_request.html which checks the category in the console. Actually good that we have such tests. I updated that test to match the new behavior - should be fine now:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=921326d8e8327c5c22dcd40a06963686c4e658a1

Comment 12

[Pulsebot]

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

Comment 13

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/7dbaf50e83c6

Comment 14

[:Harald Kirschner :digitarald]

Comment on attachment 8992995 [details] [diff] [review]
bug_1304645_change_category_for_cors_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=8996257
[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 15

[Liz Henry (:lizzard)]

Comment on attachment 8992995 [details] [diff] [review]
bug_1304645_change_category_for_cors_errors.patch

Looks useful for developers, low risk change adding categories to error messages. Let's uplift for beta 17 along with the patch in bug 1475391.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/40006339de0c