Bug 175247 – REGRESSION (r219013): OAuth flows are broken when redirecting back to application after authentication

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Daniel Bates]

Treat POST that redirects to a GET as a new navigation action

[Daniel Bates]

Created attachment 317370 [details]
Patch

[Daniel Bates]

Created attachment 317371 [details]
Patch

[Daniel Bates]

<rdar://problem/33679804>

[Daniel Bates]

Comment on attachment 317371 [details]
Patch

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

> Tools/TestWebKitAPI/cocoa/TestProtocol.mm:87
> +        NSMutableURLRequest* request = [NSMutableURLRequest requestWithURL:createRedirectURL(requestURL.query)];

* is on the wrong side. Will fix before landing.

[Build Bot]

Comment on attachment 317371 [details]
Patch

Attachment 317371 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4265533

New failing tests:
http/tests/misc/redirect-to-external-url.html

[Build Bot]

Created attachment 317378 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Daniel Bates]

(In reply to Build Bot from comment #5)
> Comment on attachment 317371 [details]
> Patch
> 
> Attachment 317371 [details] did not pass mac-ews (mac):
> Output: http://webkit-queues.webkit.org/results/4265533
> 
> New failing tests:
> http/tests/misc/redirect-to-external-url.html

Interesting. TestRunner can dump navigation type passed to decidePolicyForNavigationAction. So, we could use a layout test(s) as opposed to unit tests to exercise this change. When I have a moment I will look to write layout tests either in an updated patch on this bug or in a separate bug/patch.

The test fails because a clicked hyperlink redirects to a GET. The test expected that the navigation type would be "link clicked". Now, it is "other".

[Daniel Bates]

Created attachment 317380 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 317380 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Treat POST that redirects to a GET as a new navigation action

The new behavior seems very unintuitive to me. The navigation is triggered by form submission, why does it matter that internally, there was a redirect? And if we change the action on redirects, why don't we change it when redirecting POST to POST?

> Source/WebCore/loader/PolicyChecker.cpp:88
> +    if (action.isEmpty() || (didReceiveRedirectResponse && request.httpMethod() == "GET")) {

Will this also change behavior when redirecting a GET to a GET? Or when the original action is not FormSubmitted?

[Daniel Bates]

(In reply to Alexey Proskuryakov from comment #9)
> Comment on attachment 317380 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=317380&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        Treat POST that redirects to a GET as a new navigation action
> 
> The new behavior seems very unintuitive to me. 

That's unfortunate. Maybe we need to come up with a different approach to fix this bug.

> The navigation is triggered by form submission, why does it matter that internally, there was a
> redirect?

A POSTed form that redirects to a GET is no longer a form submission because a POST body is never sent with a GET and there is no concept of transcribing a POST body to a GET for a redirect. Notice that we always ask the embedding client before performing a navigation (i.e. call decidePolicyForNavigationAction: on Mac/iOS). Suppose an embedding client wants to prompt the user that a form submission is insecure, if applicable. Looking at the Cocoa WebKit API callbacks and without loss of generality, suppose an form is POSTed to an HTTP resource (an "insecure form") then we call decidePolicyForNavigationAction: with a navigation action of type WKNavigationTypeFormSubmitted to ask the embedding client if we can submit the form. If the POSTed form redirects to a GET then we call decidePolicyForNavigationAction: again with a navigation action of type WKNavigationTypeFormSubmitted (read: the same type as the original form submission) to ask the embedding client if we can perform the redirect. The form data is only sent with the first request. How do you propose the embedding client differentiate between these first and second cases such that it prompts a person exactly once to submit the insecure form submission? Do you see any issues with making this distinction in WebCore?

> And if we change the action on redirects, why don't we change it when redirecting POST to POST?

I tried to convey this in the example I gave at the end of ChangeLog description. I will update the ChangeLog description to be more explicit. A temporary redirect "does not allow changing the request method from POST to GET" (<https://tools.ietf.org/html/rfc7231#section-6.4.7>).

> 
> > Source/WebCore/loader/PolicyChecker.cpp:88
> > +    if (action.isEmpty() || (didReceiveRedirectResponse && request.httpMethod() == "GET")) {
> 
> Will this also change behavior when redirecting a GET to a GET? Or when the
> original action is not FormSubmitted?

I will add an explicit remark to the ChangeLog about the this. (It is implicit in the wording of the WebCore ChangeLog entry and the description in LayoutTests/ChangeLog). This will change the navigation action type passed to decidePolicyForNavigationAction: when asking the client whether to allow the redirect whenever we receive a redirect to a GET. For such callbacks, the navigation type will be "other". This behavior makes the navigation action type passed to decidePolicyForNavigationAction: consistent for all internally redirected GET requests.

[Daniel Bates]

Created attachment 317462 [details]
Patch and unit tests

Updated WebCore ChangeLog description to explain the behavior of HTTP 307 temporary redirects with respect to the preservation of the HTTP method as well as how this change affects the navigation type passed to clients that implement the WKNavigationDelegate protocol.

[Geoffrey Garen]

Comment on attachment 317462 [details]
Patch and unit tests

r=me

[Daniel Bates]

Created attachment 317507 [details]
Patch for landing

[Alexey Proskuryakov]

Comment on attachment 317507 [details]
Patch for landing

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

> Source/WebCore/ChangeLog:22
> +        Only HTTP 307 temporary redirects are performed without changing the HTTP method as per

Actually 308 also doesn't allow changing POST to GET.

I'm not quite sure about other codes - some of these seem to permit keeping the POST per the spec, but browsers don't actually do that. I haven't looked into the details.

[Daniel Bates]

After talking with Alexey Proskuryakov yesterday and deliberating on this patch some more I suggest that we defer changing the navigation type for navigation that redirects to GET for the moment. We need to think through the implications of such a change for WebKit and WebKitLegacy clients. For now, Alexey and I suggest adding new SPI to address this issue in Safari.

[Daniel Bates]

Created attachment 317591 [details]
Add SPI and unit tests

[Daniel Bates]

Created attachment 317639 [details]
Add SPI and unit tests

Fix the Windows build.

[Daniel Bates]

Committed r220459: <http://trac.webkit.org/changeset/220459>