Bug 222653

Created attachment 422076 [details] Testcase

<rdar://problem/74983521>

@Michael, I believe this is a test case reproducing the issue you have with the websites you mentioned offline. Can you validate this?

Created attachment 422088 [details] Patch

Comment on attachment 422088 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=422088&action=review I'm pretty sure this will fix the case I discussed with you yesterday. I will test it to be certain. > Source/WebCore/loader/DocumentLoader.cpp:500 > + return oldRequest.httpMethod() == "POST" && newRequest.httpMethod() == "GET"; Hmm, I'm a little suspicious that the fix cares so much about the method used by the original request, and specifically that it cares whether it is POST. My test case happened to use POST, yes, but what if we were redirecting from a PUT instead of a POST? There are really two cases: * Method is not GET or HEAD. Origin header must always be sent. * Method is GET or HEAD. Origin header must be sent if the request is a CORS request. I think the method used by the original request or previous redirected requests should not matter. So maybe what we really want to do in case of a redirect is: if newRequest.httpMethod() == "GET" || newRequest.httpMethod() == "HEAD": if newRequest is NOT a CORS request: <-- behavior difference is here newRequest.clearHTTPOrigin() The difference vs. Firefox/Chrome is that they compare newRequest to the *most recent* redirected request to decide whether it is a CORS request. WebKit seems to be comparing it to the *original* request. Right?

(In reply to Michael Catanzaro from comment #5) > Comment on attachment 422088 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=422088&action=review > > I'm pretty sure this will fix the case I discussed with you yesterday. I > will test it to be certain. > > > Source/WebCore/loader/DocumentLoader.cpp:500 > > + return oldRequest.httpMethod() == "POST" && newRequest.httpMethod() == "GET"; > > Hmm, I'm a little suspicious that the fix cares so much about the method > used by the original request, and specifically that it cares whether it is > POST. My test case happened to use POST, yes, but what if we were > redirecting from a PUT instead of a POST? There are really two cases: Navigation loads can only be POST or GET I think. > I think the method used by the original request or previous redirected > requests should not matter. So maybe what we really want to do in case of a > redirect is: > > if newRequest.httpMethod() == "GET" || newRequest.httpMethod() == "HEAD": > if newRequest is NOT a CORS request: <-- behavior difference is here > newRequest.clearHTTPOrigin() > > The difference vs. Firefox/Chrome is that they compare newRequest to the > *most recent* redirected request to decide whether it is a CORS request. > WebKit seems to be comparing it to the *original* request. Right? There is no CORS in navigation load. The patch does not check the original request but the last previous request so this should be fine. Or did I misunderstand something?

Hm, looks like EWS is failing because the helper script echo-origin.py is being considered a layout test that is expected to have results. :O (In reply to youenn fablet from comment #6) > Navigation loads can only be POST or GET I think. OK. > There is no CORS in navigation load. Hmm, OK. > The patch does not check the original request but the last previous request > so this should be fine. > Or did I misunderstand something? No, you're right!

(In reply to Michael Catanzaro from comment #7) > Hm, looks like EWS is failing because the helper script echo-origin.py is > being considered a layout test that is expected to have results. :O Oh that is sad, I thought we would only allow this on http/tests, and not in http/wpt.

Comment on attachment 422088 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=422088&action=review > LayoutTests/http/wpt/fetch/echo-origin.py:1 > +def main(request, response): The CGI script for this should look something like this: import os import sys sys.stdout.write( 'Content-Type: text/html\r\n\r\n' "<script>parent.postMessage('{}')</script>".format(os.environ.get('HTTP_ORIGIN', 'no header')) )

(In reply to youenn fablet from comment #8) > (In reply to Michael Catanzaro from comment #7) > > Hm, looks like EWS is failing because the helper script echo-origin.py is > > being considered a layout test that is expected to have results. :O > > Oh that is sad, I thought we would only allow this on http/tests, and not in > http/wpt. Also, .py files in resource directories are not considered layout tests

(In reply to Jonathan Bedard from comment #10) > (In reply to youenn fablet from comment #8) > > (In reply to Michael Catanzaro from comment #7) > > > Hm, looks like EWS is failing because the helper script echo-origin.py is > > > being considered a layout test that is expected to have results. :O > > > > Oh that is sad, I thought we would only allow this on http/tests, and not in > > http/wpt. > > Also, .py files in resource directories are not considered layout tests Ressources in LayoutTests/http/wpt are served by the WPT server not by httpd. We also want to follow WPT rules in LayoutTests/http/wpt. That means that .py scripts should probably not considered from being layout tests.

(In reply to youenn fablet from comment #11) > (In reply to Jonathan Bedard from comment #10) > > (In reply to youenn fablet from comment #8) > > > (In reply to Michael Catanzaro from comment #7) > > > > Hm, looks like EWS is failing because the helper script echo-origin.py is > > > > being considered a layout test that is expected to have results. :O > > > > > > Oh that is sad, I thought we would only allow this on http/tests, and not in > > > http/wpt. > > > > Also, .py files in resource directories are not considered layout tests > > Ressources in LayoutTests/http/wpt are served by the WPT server not by httpd. > We also want to follow WPT rules in LayoutTests/http/wpt. That means that > .py scripts should probably not considered from being layout tests. We can make that change pretty easily, the reason I hadn't done so originally is because PHP follows the same set of rules in http/wpt and http/tests

(In reply to Michael Catanzaro from comment #5) > I'm pretty sure this will fix the case I discussed with you yesterday. I > will test it to be certain. Confirmed.

Created attachment 422198 [details] Patch

Comment on attachment 422198 [details] Patch OK, EWS are green now, yay! I could give rs=me if needed, but ideally this would be reviewed by someone more familiar with navigations and DocumentLoader.

Comment on attachment 422198 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=422198&action=review > Source/WebCore/loader/DocumentLoader.cpp:500 > + return oldRequest.httpMethod() == "POST" && newRequest.httpMethod() == "GET"; This should probably be a case insensitive comparison. > LayoutTests/ChangeLog:3 > + In case of POST navigation redirected by a 302, the 'Origin' header is kept in the redirected request This mentions 302. We don't check the status code. Should we? Should we also remove the Origin headers for other redirect codes?

(In reply to Alex Christensen from comment #16) > Comment on attachment 422198 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=422198&action=review > > > Source/WebCore/loader/DocumentLoader.cpp:500 > > + return oldRequest.httpMethod() == "POST" && newRequest.httpMethod() == "GET"; > > This should probably be a case insensitive comparison. It might not hurt though DocumentLoader::isPostOrRedirectAfterPost or isOnAccessControlSimpleRequestMethodAllowlist are doing the same thing here. > > LayoutTests/ChangeLog:3 > > + In case of POST navigation redirected by a 302, the 'Origin' header is kept in the redirected request > > This mentions 302. We don't check the status code. Should we? Should we > also remove the Origin headers for other redirect codes? Only 302 and 303 AFAIK will change from POST to GET. And we want to cover both cases since the issue is really redirection triggering POST to GET transition. The bug title could be improved.

Committed r273905: <https://commits.webkit.org/r273905> All reviewed patches have been landed. Closing bug and clearing flags on attachment 422198 [details].