Bug 175247 - REGRESSION (r219013): OAuth flows are broken when redirecting back to application after authentication
Summary: REGRESSION (r219013): OAuth flows are broken when redirecting back to applica...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords: InRadar, Regression
Depends on:
Blocks:
 
Reported: 2017-08-06 10:52 PDT by Daniel Bates
Modified: 2017-08-09 09:30 PDT (History)
7 users (show)

See Also:


Attachments
Patch (12.38 KB, patch)
2017-08-06 11:23 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch (13.12 KB, patch)
2017-08-06 11:24 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-elcapitan (993.51 KB, application/zip)
2017-08-06 12:25 PDT, Build Bot
no flags Details
Patch (15.06 KB, patch)
2017-08-06 12:47 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch and unit tests (15.96 KB, patch)
2017-08-07 14:17 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch for landing (16.85 KB, patch)
2017-08-07 16:50 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Add SPI and unit tests (28.69 KB, patch)
2017-08-08 11:58 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Add SPI and unit tests (32.10 KB, patch)
2017-08-08 16:15 PDT, Daniel Bates
beidson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2017-08-06 10:52:56 PDT
Treat POST that redirects to a GET as a new navigation action
Comment 1 Daniel Bates 2017-08-06 11:23:16 PDT
Created attachment 317370 [details]
Patch
Comment 2 Daniel Bates 2017-08-06 11:24:20 PDT
Created attachment 317371 [details]
Patch
Comment 3 Daniel Bates 2017-08-06 11:25:10 PDT
<rdar://problem/33679804>
Comment 4 Daniel Bates 2017-08-06 11:50:05 PDT
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.
Comment 5 Build Bot 2017-08-06 12:25:53 PDT Comment hidden (obsolete)
Comment 6 Build Bot 2017-08-06 12:25:54 PDT Comment hidden (obsolete)
Comment 7 Daniel Bates 2017-08-06 12:40:20 PDT
(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".
Comment 8 Daniel Bates 2017-08-06 12:47:35 PDT
Created attachment 317380 [details]
Patch
Comment 9 Alexey Proskuryakov 2017-08-07 09:49:46 PDT
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?
Comment 10 Daniel Bates 2017-08-07 13:56:42 PDT
(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.
Comment 11 Daniel Bates 2017-08-07 14:17:04 PDT
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.
Comment 12 Geoffrey Garen 2017-08-07 14:30:24 PDT
Comment on attachment 317462 [details]
Patch and unit tests

r=me
Comment 13 Daniel Bates 2017-08-07 16:50:18 PDT
Created attachment 317507 [details]
Patch for landing
Comment 14 Alexey Proskuryakov 2017-08-07 17:09:39 PDT
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.
Comment 15 Daniel Bates 2017-08-08 11:45:00 PDT
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.
Comment 16 Daniel Bates 2017-08-08 11:58:25 PDT
Created attachment 317591 [details]
Add SPI and unit tests
Comment 17 Daniel Bates 2017-08-08 16:15:46 PDT
Created attachment 317639 [details]
Add SPI and unit tests

Fix the Windows build.
Comment 18 Daniel Bates 2017-08-09 09:30:50 PDT
Committed r220459: <http://trac.webkit.org/changeset/220459>