Closed Bug 1475073 Opened Last year Closed Last year

Pass individual CORS 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: Harald, Assigned: ckerschb)

References

Details

(Whiteboard: [domsecurity-active])

Attachments

(1 file)

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.
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?
Flags: needinfo?(ckerschb)
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).
Blocks: 1475391
Just fore the refernece, I am assigning this bug to myself, similar to:
https://bugzilla.mozilla.org/show_bug.cgi?id=1304645#c7
Assignee: nobody → ckerschb
Status: NEW → ASSIGNED
Flags: needinfo?(ckerschb)
Priority: -- → P2
Whiteboard: [domsecurity-active]
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.
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.
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.
Summary: Add per-error-type categories for CORS console errors → Pass individual CORS errors as categories to web console error messages
Attachment #8992995 - Flags: review?(amarchesini)
Please note that all of those messages are prefixed with "CORS" anyway.
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)
Attachment #8992995 - Flags: review?(amarchesini) → review+
Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4fbeea69b10d
Pass individual CORS errors as categories to web console error messages. r=baku
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
Flags: needinfo?(ckerschb)
Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7dbaf50e83c6
Pass individual CORS errors as categories to web console error messages. r=baku
https://hg.mozilla.org/mozilla-central/rev/7dbaf50e83c6
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
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.
Attachment #8992995 - Flags: approval-mozilla-beta?
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.
Attachment #8992995 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Target Milestone: mozilla62 → mozilla63
You need to log in before you can comment on or make changes to this bug.