Closed
Bug 1475391
Opened 7 years ago
Closed 7 years ago
Add mapping for CORS error types to MDN pages
Categories
(DevTools :: Console, defect, P2)
DevTools
Console
Tracking
(firefox62 fixed, firefox63 fixed)
VERIFIED
FIXED
Firefox 63
People
(Reporter: Harald, Assigned: nchevobbe)
References
(Blocks 2 open bugs)
Details
Attachments
(1 file)
46 bytes,
text/x-phabricator-request
|
bgrins
:
review+
bgrins
:
review+
lizzard
:
approval-mozilla-beta+
|
Details | Review |
Follow up work to wire up the categories added in 1475073 to the MDN pages created by https://github.com/mdn/sprints/issues/64 .
Reporter | ||
Updated•7 years ago
|
Component: General → Console
Product: Core → DevTools
Target Milestone: --- → Firefox 62
Assignee | ||
Updated•7 years ago
|
Priority: -- → P2
Comment 1•7 years ago
|
||
A list of all 14 documented error messages can be found here: https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS/Errors
Comment 2•7 years ago
|
||
Please add GA parameters to the links, so we know where the traffic is coming from, similarly to how the JS error messages are handled:
utm_source=mozilla
utm_medium=firefox-cors-errors
utm_campaign=default
Reporter | ||
Comment 3•7 years ago
|
||
Kadir, newer links use the format source=mozilla / medium=devtools-* (devtools-console-cors in this case). Would you be OK with that?
Flags: needinfo?(a.topal)
Reporter | ||
Comment 4•7 years ago
|
||
Nicolas, who could pick up the work to link the categories to the MDN errors?
Flags: needinfo?(nchevobbe)
Reporter | ||
Comment 5•7 years ago
|
||
Kadir, correction. Actual new format source = devtools, medium = console-cors.
Comment 6•7 years ago
|
||
Harald, is that going to be changed for the JS errors or do they stay in the old format? I don't have an opinion on naming, but it would be good to have it consistent, that would make reporting out all error message traffic easier for example. Either way, we can work with any format.
Flags: needinfo?(a.topal) → needinfo?(hkirschner)
Assignee | ||
Comment 7•7 years ago
|
||
I may try doing it first.
What's the timeline on this ?
Flags: needinfo?(nchevobbe)
Reporter | ||
Comment 8•7 years ago
|
||
Land 63 and uplift, given the low risk of adding some mapping. :ckerschb have his thumbs up for uplifting the added categories.
Flags: needinfo?(hkirschner) → needinfo?(nchevobbe)
Assignee | ||
Comment 9•7 years ago
|
||
Are we missing a page for https://searchfox.org/mozilla-central/rev/033d45ca70ff32acf04286244644d19308c359d5/dom/locales/en-US/chrome/security/security.properties#14 ?
Flags: needinfo?(nchevobbe)
Assignee | ||
Comment 10•7 years ago
|
||
Assignee | ||
Comment 11•7 years ago
|
||
The uploaded patch seems to work, but I'd like to have dedicated test for each of the error.
I don't think we have such thing for the other error urls right now ?
Assignee: nobody → nchevobbe
Status: NEW → ASSIGNED
Comment 12•7 years ago
|
||
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #11)
> The uploaded patch seems to work, but I'd like to have dedicated test for
> each of the error.
> I don't think we have such thing for the other error urls right now ?
Christoph, is there a test page we can copy/reuse that runs into each distinct type of CORS error?
Flags: needinfo?(ckerschb)
Comment 13•7 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #12)
> (In reply to Nicolas Chevobbe [:nchevobbe] from comment #11)
> > The uploaded patch seems to work, but I'd like to have dedicated test for
> > each of the error.
> > I don't think we have such thing for the other error urls right now ?
>
> Christoph, is there a test page we can copy/reuse that runs into each
> distinct type of CORS error?
Yeah, I am not sure we have something suitable for this usecase. All the tests we have are within dom/security/test/cors. Maybe you can tweak browser_CORS-console-warnings.js to do what you want!
Flags: needinfo?(ckerschb)
Comment 14•7 years ago
|
||
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #11)
> The uploaded patch seems to work, but I'd like to have dedicated test for
> each of the error.
> I don't think we have such thing for the other error urls right now ?
Yeah - if we don't want to simulate the actual error messages (as per Comment 13), I guess we could modify the category on a stub packet directly and confirm the right link shows up.
Assignee | ||
Comment 15•7 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #14)
> (In reply to Nicolas Chevobbe [:nchevobbe] from comment #11)
> > The uploaded patch seems to work, but I'd like to have dedicated test for
> > each of the error.
> > I don't think we have such thing for the other error urls right now ?
>
> Yeah - if we don't want to simulate the actual error messages (as per
> Comment 13), I guess we could modify the category on a stub packet directly
> and confirm the right link shows up.
We wouldn't be covered if a category name was changed in that case. We could test it with mocha, but in order to get the proper packets we would need to trigger the cors errors anyway, so it's maybe better to directly have mochitests for that.
Comment 16•7 years ago
|
||
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #15)
> (In reply to Brian Grinstead [:bgrins] from comment #14)
> > (In reply to Nicolas Chevobbe [:nchevobbe] from comment #11)
> > > The uploaded patch seems to work, but I'd like to have dedicated test for
> > > each of the error.
> > > I don't think we have such thing for the other error urls right now ?
> >
> > Yeah - if we don't want to simulate the actual error messages (as per
> > Comment 13), I guess we could modify the category on a stub packet directly
> > and confirm the right link shows up.
>
> We wouldn't be covered if a category name was changed in that case. We could
> test it with mocha, but in order to get the proper packets we would need to
> trigger the cors errors anyway, so it's maybe better to directly have
> mochitests for that.
Sure - I think it'd be handy anyway to have a CORS test page to use for other console / network features. Hopefully we can extract something relatively simple out of the test / helper files in Comment 13 that suits our needs.
Comment 17•7 years ago
|
||
Yes, we were missing a page for CORSMultipleAllowOriginNotAllowed. Now we aren't anymore: https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS/Errors/CORSMultipleAllowOriginNotAllowed.
Assignee | ||
Comment 18•7 years ago
|
||
(In reply to Eric Shepherd [:sheppy] from comment #17)
> Yes, we were missing a page for CORSMultipleAllowOriginNotAllowed. Now we
> aren't anymore:
> https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS/Errors/
> CORSMultipleAllowOriginNotAllowed.
Great, thanks !
Assignee | ||
Comment 19•7 years ago
|
||
Okay, so this is where I'm at for now: https://phabricator.services.mozilla.com/D2557#change-koRclRrhHmPO
I only managed to get a fraction of the test case to actually produce the error we want.
It's not clear for all the test cases what is missing to make them fail properly.
Christoph, if you have insights on that that would be super helpful :)
Flags: needinfo?(ckerschb)
Assignee | ||
Comment 20•7 years ago
|
||
new patch version posted on phabricator - down to 2 test case missing:
- CORSOriginHeaderNotAdded
- CORSRequestNotHttp
Not sure how to manage those. I think I'd be fine landing this patch without them and then enabling them in follow-ups.
Thoughts ?
Assignee | ||
Updated•7 years ago
|
Attachment #8996257 -
Flags: review?(bgrinstead)
Comment 21•7 years ago
|
||
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #20)
> new patch version posted on phabricator - down to 2 test case missing:
>
> - CORSOriginHeaderNotAdded
I think you could flip the pref 'network.http.sendOriginHeader'.
> - CORSRequestNotHttp
Maybe make use of a data: URI - I think that should do the trick for the test. (grepping for data: within dom/security/test/ might give you some ideas).
> Not sure how to manage those. I think I'd be fine landing this patch without
> them and then enabling them in follow-ups.
> Thoughts ?
If my two suggestions work out, then maybe you can incorporate the changes within this patch. If too complicated I think it's fine to land what you have and potentially file a follow up.
Thanks for adding those tests!
Flags: needinfo?(ckerschb)
Assignee | ||
Comment 22•7 years ago
|
||
Thanks Christoph for your suggestions !
Sadly, it looks like network.http.sendOriginHeader is somehow overridden when doing cors ? It makes sense its default is 0 and thus would make all cors request to not include the origin header. I don't see anything that would explain this in https://searchfox.org/mozilla-central/rev/d0a0ae30d534dc55e712bd3eedc1b4553ba56323/netwerk/protocol/http/nsHttpChannel.cpp#8887-8937 so I am surely missing something.
> Maybe make use of a data: URI - I think that should do the trick for the test. (grepping for data: within dom/security/test/ might give you some ideas).
It doesn't seems to work either. It's not even logged as a network request but I do get a 200 response.
Comment 23•7 years ago
|
||
Comment on attachment 8996257 [details]
Bug 1475391 - Add mapping for CORS error types to MDN pages; r=bgrins.
Brian Grinstead [:bgrins] has approved the revision.
https://phabricator.services.mozilla.com/D2557
Attachment #8996257 -
Flags: review+
Updated•7 years ago
|
Attachment #8996257 -
Flags: review?(bgrinstead) → review+
Assignee | ||
Comment 25•7 years ago
|
||
Filed Bug 1480671 & Bug 1480672 for the 2 remaining test cases.
Comment 26•7 years ago
|
||
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/484dc9b59dca
Add mapping for CORS error types to MDN pages; r=bgrins.
Comment 27•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: Firefox 62 → Firefox 63
Reporter | ||
Comment 28•7 years ago
|
||
Comment on attachment 8996257 [details]
Bug 1475391 - Add mapping for CORS error types to MDN pages; r=bgrins.
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, new tests added.
[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=8992995
[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 #8996257 -
Flags: approval-mozilla-beta?
Updated•7 years ago
|
status-firefox62:
--- → affected
Comment 29•7 years ago
|
||
Comment on attachment 8996257 [details]
Bug 1475391 - Add mapping for CORS error types to MDN pages; r=bgrins.
Useful improvement for developers, low risk to the release. Let's uplift for beta 17 along with the patch in bug 1475073.
Attachment #8996257 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 30•7 years ago
|
||
bugherder uplift |
Flags: in-testsuite+
Comment 31•7 years ago
|
||
Backed out from Beta for browser_webconsole_cors_errors.js failures.
https://treeherder.mozilla.org/logviewer.html#?job_id=193366021&repo=mozilla-beta
https://hg.mozilla.org/releases/mozilla-beta/rev/c22eb9a9315cafb62134eedfc45f2b5e4bf5afcd
Flags: needinfo?(nchevobbe)
Assignee | ||
Comment 32•7 years ago
|
||
The test relies on Bug 1471502 patch, which wasn't listed as needed for the uplift (but we do need it).
Flags: needinfo?(nchevobbe)
![]() |
||
Comment 33•7 years ago
|
||
bugherder uplift |
Comment 34•6 years ago
|
||
verifying that we are seeing this show up in MDN
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•