Closed Bug 1475391 Opened Last year Closed Last year

Add mapping for CORS error types to MDN pages

Categories

(DevTools :: Console, defect, P2)

defect

Tracking

(firefox62 fixed, firefox63 fixed)

VERIFIED FIXED
Firefox 63
Tracking Status
firefox62 --- fixed
firefox63 --- fixed

People

(Reporter: Harald, Assigned: nchevobbe)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

Follow up work to wire up the categories added in 1475073 to the MDN pages created by https://github.com/mdn/sprints/issues/64 .
Component: General → Console
Product: Core → DevTools
Target Milestone: --- → Firefox 62
Priority: -- → P2
A list of all 14 documented error messages can be found here: https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS/Errors
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
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)
Nicolas, who could pick up the work to link the categories to the MDN errors?
Flags: needinfo?(nchevobbe)
Kadir, correction. Actual new format source = devtools, medium = console-cors.
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)
I may try doing it first.
What's the timeline on this ?
Flags: needinfo?(nchevobbe)
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)
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
(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)
(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)
(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.
(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.
(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.
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.
(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 !
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)
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 ?
Attachment #8996257 - Flags: review?(bgrinstead)
(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)
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 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+
Attachment #8996257 - Flags: review?(bgrinstead) → review+
Duplicate of this bug: 1115151
Blocks: 1480671
Blocks: 1480672
Filed Bug 1480671 & Bug 1480672 for the 2 remaining test cases.
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/484dc9b59dca
Add mapping for CORS error types to MDN pages; r=bgrins.
https://hg.mozilla.org/mozilla-central/rev/484dc9b59dca
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: Firefox 62 → Firefox 63
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?
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+
The test relies on Bug 1471502 patch, which wasn't listed as needed for the uplift (but we do need it).
Flags: needinfo?(nchevobbe)
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.