Bug 231928 - Show HTTP status code in CORS messages logged to devtools console that can indirectly result from HTTP errors
Summary: Show HTTP status code in CORS messages logged to devtools console that can in...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Enhancement
Assignee: Michael[tm] Smith
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-10-18 18:08 PDT by Michael[tm] Smith
Modified: 2021-11-03 11:42 PDT (History)
15 users (show)

See Also:


Attachments
Patch (3.29 KB, patch)
2021-10-18 18:35 PDT, Michael[tm] Smith
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (47.81 KB, patch)
2021-10-18 23:01 PDT, Michael[tm] Smith
no flags Details | Formatted Diff | Diff
Patch (47.81 KB, patch)
2021-10-18 23:06 PDT, Michael[tm] Smith
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (175.42 KB, patch)
2021-10-19 01:32 PDT, Michael[tm] Smith
no flags Details | Formatted Diff | Diff
Patch (175.42 KB, patch)
2021-10-19 02:01 PDT, Michael[tm] Smith
no flags Details | Formatted Diff | Diff
Patch (175.42 KB, patch)
2021-10-19 02:51 PDT, Michael[tm] Smith
no flags Details | Formatted Diff | Diff
Patch (175.42 KB, patch)
2021-10-19 04:11 PDT, Michael[tm] Smith
no flags Details | Formatted Diff | Diff
Patch (175.86 KB, patch)
2021-10-19 08:00 PDT, Michael[tm] Smith
no flags Details | Formatted Diff | Diff
Patch (178.87 KB, patch)
2021-10-21 04:12 PDT, Michael[tm] Smith
no flags Details | Formatted Diff | Diff
Patch (179.76 KB, patch)
2021-10-21 04:43 PDT, Michael[tm] Smith
no flags Details | Formatted Diff | Diff
Patch (180.29 KB, patch)
2021-10-21 07:56 PDT, Michael[tm] Smith
no flags Details | Formatted Diff | Diff
Patch (184.09 KB, patch)
2021-10-21 15:36 PDT, Michael[tm] Smith
no flags Details | Formatted Diff | Diff
Patch (183.14 KB, patch)
2021-10-21 16:41 PDT, Michael[tm] Smith
no flags Details | Formatted Diff | Diff
Patch (185.13 KB, patch)
2021-10-21 17:39 PDT, Michael[tm] Smith
no flags Details | Formatted Diff | Diff
Patch (188.89 KB, patch)
2021-10-21 21:00 PDT, Michael[tm] Smith
no flags Details | Formatted Diff | Diff
Patch (189.19 KB, patch)
2021-10-21 22:51 PDT, Michael[tm] Smith
no flags Details | Formatted Diff | Diff
Patch (189.14 KB, patch)
2021-11-01 16:00 PDT, Michael[tm] Smith
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael[tm] Smith 2021-10-18 18:08:06 PDT
### Problem
 When a CORS request is sent to a server and the server responds with a 4xx or 5xx HTTP status code in the response, and the response doesn’t include the Access-Control-Allow-Origin header, the browser will always log a CORS error message to the devtools console — but may not (or even usually doesn’t) also log the HTTP status code to the console.

As a result, many (or most) developers who are in the process of trying to troubleshoot the server-side CORS config (on the server the request was sent to) only see the CORS error — but don’t see the 4xx or 5xx HTTP status code — and so then often (or usually) assume that there’s a mistake in their CORS config, when in fact the actual cause is that a 4xx or 5xx error occurred.

For some evidence and discussion of that problem, see the following:

* https://stackoverflow.com/questions/54795541/503-return-from-server-is-branded-as-cors-violation-by-chrome
* https://davidtruxall.com/misleading-cors-errors/

The effect of that problem is that, every day, it causes hundreds if not thousands of developers to waste hours trying to troubleshoot and identify mistakes in their server CORS config — mistakes they never find, because in fact their existing CORS config is already working as expected and instead the real cause is some 400 or 500 or 502 or whatever.

The main reason those developers see CORS errors logged to the devtools console in those cases is that many (or most) runtimes/server systems by default do not add application-set response headers to 4xx and 5xx responses — including the Access-Control-Allow-Origin response header. (For example, Apache and nginx do not; in order to make them add the Access-Control-Allow-Origin response header to 4xx or 5xx errors, the `always` keyword needs to be added to the header-setting directive.)

### Proposed solution
Make the HTTP status be included in all CORS messages that might get logged when a 4xx or 5xx error occurs. That means the following:

* Failed to load resource: Origin foo is not allowed by Access-Control-Allow-Origin

* Failed to load resource: Preflight response is not successful

The proposal here is that in the case of, for example, a 500 error, those messages would instead be:

* Failed to load resource: Origin foo is not allowed by Access-Control-Allow-Origin. Status code: 500

* Failed to load resource: Preflight response is not successful. Status code: 500
Comment 1 Michael[tm] Smith 2021-10-18 18:11:35 PDT
I should have mentioned in the issue description that I’ve written Gecko and Blink patches for this:

* https://bugzilla.mozilla.org/show_bug.cgi?id=1736026 is the Gecko bug. The patch for that should be landing in the trunk there this week.

* https://bugs.chromium.org/p/chromium/issues/detail?id=1260776 is the Chromium bug. The patch there is going take a while longer to land.
Comment 2 Michael[tm] Smith 2021-10-18 18:35:39 PDT
Created attachment 441669 [details]
Patch
Comment 3 Michael[tm] Smith 2021-10-18 23:01:32 PDT
Created attachment 441690 [details]
Patch
Comment 4 Michael[tm] Smith 2021-10-18 23:06:21 PDT
Created attachment 441691 [details]
Patch
Comment 5 Michael[tm] Smith 2021-10-19 01:32:56 PDT
Created attachment 441699 [details]
Patch
Comment 6 Michael[tm] Smith 2021-10-19 02:01:31 PDT
Created attachment 441703 [details]
Patch
Comment 7 Michael[tm] Smith 2021-10-19 02:51:49 PDT
Created attachment 441704 [details]
Patch
Comment 8 Michael[tm] Smith 2021-10-19 04:11:31 PDT
Created attachment 441710 [details]
Patch
Comment 9 Michael[tm] Smith 2021-10-19 08:00:30 PDT
Created attachment 441726 [details]
Patch
Comment 10 Michael[tm] Smith 2021-10-20 19:25:15 PDT
I’m looking at the test failures and I see this:

https://ews-build.s3-us-west-2.amazonaws.com/Windows-EWS/r441710-110694/results.html

…and that’s failing because there’s a difference between the expected output and the actual output:

https://ews-build.s3-us-west-2.amazonaws.com/Windows-EWS/r441710-110694/http/tests/xmlhttprequest/access-control-preflight-not-successful-pretty-diff.html

-CONSOLE MESSAGE: Preflight response is not successful
+CONSOLE MESSAGE: Preflight response is not successful. Status code: 302

That is, specifically, the expected output has no `Status code: 302` while the actual output does.

But the patch does actually include a change to the expected output at http/tests/xmlhttprequest/access-control-preflight-not-successful-expected.txt — see https://bugs.webkit.org/attachment.cgi?id=441726&action=diff#a/LayoutTests/http/tests/xmlhttprequest/access-control-preflight-not-successful-expected.txt_sec1

However, for some reason the expected output that the EWS test is picking up at 
https://ews-build.s3-us-west-2.amazonaws.com/Windows-EWS/r441710-110694/http/tests/xmlhttprequest/access-control-preflight-not-successful-expected.txt doesn’t include the change.

So I don’t understand why the change from the patch isn’t reflected in the version the EWS test is using — and I don’t have an idea how to fix it…
Comment 11 Alex Christensen 2021-10-20 19:31:45 PDT
$ find LayoutTests -name access-control-preflight-not-successful-expected.txt
LayoutTests/platform/win/http/tests/xmlhttprequest/access-control-preflight-not-successful-expected.txt
LayoutTests/platform/mac-wk1/http/tests/xmlhttprequest/access-control-preflight-not-successful-expected.txt
LayoutTests/http/tests/xmlhttprequest/access-control-preflight-not-successful-expected.txt

There are different test expectations files for different platforms.  You'll probably need to update all of them.
Comment 12 Michael[tm] Smith 2021-10-21 04:12:04 PDT
Created attachment 442005 [details]
Patch
Comment 13 Michael[tm] Smith 2021-10-21 04:43:34 PDT
Created attachment 442009 [details]
Patch
Comment 14 Michael[tm] Smith 2021-10-21 07:56:38 PDT
Created attachment 442022 [details]
Patch
Comment 15 Michael[tm] Smith 2021-10-21 15:36:56 PDT
Created attachment 442076 [details]
Patch
Comment 16 Michael[tm] Smith 2021-10-21 16:41:04 PDT
Created attachment 442085 [details]
Patch
Comment 17 Michael[tm] Smith 2021-10-21 17:39:12 PDT
Created attachment 442097 [details]
Patch
Comment 18 Michael[tm] Smith 2021-10-21 21:00:31 PDT
Created attachment 442114 [details]
Patch
Comment 19 Michael[tm] Smith 2021-10-21 22:51:47 PDT
Created attachment 442125 [details]
Patch
Comment 20 Michael[tm] Smith 2021-10-22 01:55:27 PDT
(In reply to Alex Christensen from comment #11)
>
> There are different test expectations files for different platforms.  You'll
> probably need to update all of them.

Aha, thanks — I’ve now updated all the tests, and EWS is all green. So I think this is ready for review from whoever might have time.

Incidentally, the equivalent Firefox change has now landed and will ship in Firefox 95.

- https://hg.mozilla.org/mozilla-central/rev/2b65a0fbcf8b
- https://bugzilla.mozilla.org/show_bug.cgi?id=1736026
Comment 21 EWS 2021-10-25 14:33:32 PDT
ChangeLog entry in Source/WebKit/ChangeLog contains OOPS!.
Comment 22 Radar WebKit Bug Importer 2021-10-25 18:09:23 PDT
<rdar://problem/84640965>
Comment 23 Chris Dumez 2021-11-01 15:53:17 PDT
Comment on attachment 442125 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=442125&action=review

> Source/WebKit/ChangeLog:28
> +        Reviewed by NOBODY (OOPS!).

double NOBODY line.
Comment 24 Michael[tm] Smith 2021-11-01 16:00:49 PDT
Created attachment 443028 [details]
Patch
Comment 25 EWS 2021-11-01 16:33:55 PDT
Committed r285145 (243782@main): <https://commits.webkit.org/243782@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 443028 [details].
Comment 26 Eric Hutchison 2021-11-02 10:03:05 PDT
Patch broke a few tests on iOS EWS: 

http/tests/app-privacy-report/user-attribution-preflight-async.html
http/tests/app-privacy-report/user-attribution-preflight-sync.html
http/tests/app-privacy-report/app-attribution-preflight-async.html
http/tests/app-privacy-report/app-attribution-preflight-sync.html
http/tests/privateClickMeasurement/attribution-conversion-through-fetch-keepalive.html

Re-opening for a patch.
Comment 27 Kate Cheney 2021-11-02 10:12:29 PDT
Fixed test expectations in http://trac.webkit.org/changeset/285166/webkit.
Comment 28 Michael[tm] Smith 2021-11-02 18:50:55 PDT
(In reply to Eric Hutchison from comment #26)
> Patch broke a few tests on iOS EWS: 
> 
> http/tests/app-privacy-report/user-attribution-preflight-async.html
> http/tests/app-privacy-report/user-attribution-preflight-sync.html
> http/tests/app-privacy-report/app-attribution-preflight-async.html
> http/tests/app-privacy-report/app-attribution-preflight-sync.html
> http/tests/privateClickMeasurement/attribution-conversion-through-fetch-
> keepalive.html

(In reply to Kate Cheney from comment #27)
> Fixed test expectations in http://trac.webkit.org/changeset/285166/webkit.

Thanks much — and sorry for having not been more thorough myself — to have identified those tests as needing changes.
Comment 29 Kate Cheney 2021-11-03 09:05:03 PDT
(In reply to Michael[tm] Smith from comment #28)
> (In reply to Eric Hutchison from comment #26)
> > Patch broke a few tests on iOS EWS: 
> > 
> > http/tests/app-privacy-report/user-attribution-preflight-async.html
> > http/tests/app-privacy-report/user-attribution-preflight-sync.html
> > http/tests/app-privacy-report/app-attribution-preflight-async.html
> > http/tests/app-privacy-report/app-attribution-preflight-sync.html
> > http/tests/privateClickMeasurement/attribution-conversion-through-fetch-
> > keepalive.html
> 
> (In reply to Kate Cheney from comment #27)
> > Fixed test expectations in http://trac.webkit.org/changeset/285166/webkit.
> 
> Thanks much — and sorry for having not been more thorough myself — to have
> identified those tests as needing changes.

No worries, those tests don't run on open source EWS bots for various reasons so it would have been very tricky to catch them the first time around!
Comment 30 Eric Hutchison 2021-11-03 11:06:36 PDT
http/tests/privateClickMeasurement/attribution-conversion-through-fetch-keepalive.html continues to fail on iOS15 on EWS and Opensource.
Comment 31 Eric Hutchison 2021-11-03 11:08:00 PDT
Creating new bug report for failing test: http/tests/privateClickMeasurement/attribution-conversion-through-fetch-keepalive.html
Comment 32 Eric Hutchison 2021-11-03 11:42:08 PDT
Rebaselined http/tests/privateClickMeasurement/attribution-conversion-through-fetch-keepalive.html at https://trac.webkit.org/changeset/285218/webkit
Comment 33 Eric Hutchison 2021-11-03 11:42:38 PDT
(In reply to Eric Hutchison from comment #31)
> Creating new bug report for failing test:
> http/tests/privateClickMeasurement/attribution-conversion-through-fetch-
> keepalive.html

Disregard, re-baselined instead.