:Harald Kirschner :digitarald Reporter
	Description • 6 years ago

Follow up work to wire up the categories added in 1475073 to the MDN pages created by https://github.com/mdn/sprints/issues/64 .

	Kadir Topal [:atopal]
	Comment 1 • 6 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

	Kadir Topal [:atopal]
	Comment 2 • 6 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

	:Harald Kirschner :digitarald Reporter
	Comment 3 • 6 years ago

Kadir, newer links use the format source=mozilla / medium=devtools-* (devtools-console-cors in this case). Would you be OK with that?

	:Harald Kirschner :digitarald Reporter
	Comment 4 • 6 years ago

Nicolas, who could pick up the work to link the categories to the MDN errors?

	:Harald Kirschner :digitarald Reporter
	Comment 5 • 6 years ago

Kadir, correction. Actual new format source = devtools, medium = console-cors.

	Kadir Topal [:atopal]
	Comment 6 • 6 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.

	Nicolas Chevobbe [:nchevobbe] Assignee
	Comment 7 • 6 years ago

I may try doing it first.
What's the timeline on this ?

	:Harald Kirschner :digitarald Reporter
	Comment 8 • 6 years ago

Land 63 and uplift, given the low risk of adding some mapping. :ckerschb have his thumbs up for uplifting the added categories.

	Nicolas Chevobbe [:nchevobbe] Assignee
	Comment 9 • 6 years ago

Are we missing a page for https://searchfox.org/mozilla-central/rev/033d45ca70ff32acf04286244644d19308c359d5/dom/locales/en-US/chrome/security/security.properties#14 ?

	Nicolas Chevobbe [:nchevobbe] Assignee
	Comment 11 • 6 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 ?

	Brian Grinstead [:bgrins]
	Comment 12 • 6 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?

	Christoph Kerschbaumer [:ckerschb]
	Comment 13 • 6 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!

	Brian Grinstead [:bgrins]
	Comment 14 • 6 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.

	Nicolas Chevobbe [:nchevobbe] Assignee
	Comment 15 • 6 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.

	Brian Grinstead [:bgrins]
	Comment 16 • 6 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.

	Eric Shepherd [:sheppy]
	Comment 17 • 6 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.

	Nicolas Chevobbe [:nchevobbe] Assignee
	Comment 18 • 6 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 !

	Nicolas Chevobbe [:nchevobbe] Assignee
	Comment 19 • 6 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 :)

	Nicolas Chevobbe [:nchevobbe] Assignee
	Comment 20 • 6 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 ?

	Christoph Kerschbaumer [:ckerschb]
	Comment 21 • 6 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!

	Nicolas Chevobbe [:nchevobbe] Assignee
	Comment 22 • 6 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.

	Phabricator Automation
	Comment 23 • 6 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

	Nicolas Chevobbe [:nchevobbe] Assignee
	Comment 25 • 6 years ago

Filed Bug 1480671 & Bug 1480672 for the 2 remaining test cases.

	Pulsebot
	Comment 26 • 6 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.

	Bogdan Tara[:bogdan_tara | bogdant]
	Comment 27 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/484dc9b59dca

	:Harald Kirschner :digitarald Reporter
	Comment 28 • 6 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.

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 29 • 6 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.

	Ryan VanderMeulen [:RyanVM]
	Comment 30 • 6 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/4471d22def67

	Ryan VanderMeulen [:RyanVM]
	Comment 31 • 6 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

	Nicolas Chevobbe [:nchevobbe] Assignee
	Comment 32 • 6 years ago

The test relies on Bug 1471502 patch, which wasn't listed as needed for the uplift (but we do need it).

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 33 • 6 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/7fa4748a0d14

	Kadir Topal [:atopal]
	Comment 34 • 6 years ago

verifying that we are seeing this show up in MDN