Bug 21242

Summary: [CURL] Should handle redirects in WebCore
Product: WebKit Reporter: Alp Toker <alp>
Component: New BugsAssignee: Basuke Suzuki <Basuke.Suzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, alex.christensen, Basuke.Suzuki, bfulgham, buildbot, commit-queue, davidsz, don.olmstead, galpeter, ossy, peavo, webkit-bug-importer
Priority: P2 Keywords: Curl, InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
URL: http://software.hixie.ch/utilities/cgi/data/data
Bug Depends on: 175148    
Bug Blocks: 117300    
Attachments:
Description Flags
First draft
bfulgham: review+
patch
none
patch none

Description Alp Toker 2008-09-30 08:19:16 PDT
CURL's ResourceHandleManager currently sets:

curl_easy_setopt(d->m_handle, CURLOPT_FOLLOWLOCATION, 1);

This means that HTTP redirects are handled behind the scenes by CURL. This incorrectly bypasses WebCore functionality and makes it difficult to fire the right signals and policy events at the right time. Moreover, it also bypasses WebCore's resources and data URL parsing capabilities.

http://software.hixie.ch/utilities/cgi/data/data is an example of a page that redirects to a data URL that's currently not working.

Patch coming up.
Comment 1 Gustavo Noronha (kov) 2009-02-27 20:04:19 PST
Removing the Gtk keyword, since GTK+ doesn't use curl anymore.
Comment 2 Brent Fulgham 2013-09-12 11:05:54 PDT
Too bad that patch never got added!  :-)
Comment 3 Szabolcs David 2013-09-13 03:26:00 PDT
We are currently working on it. : ) I already have a working implementation, so patch is coming soon.
Comment 4 Csaba Osztrogonác 2013-09-13 03:38:48 PDT
(In reply to comment #3)
> We are currently working on it. : ) I already have a working implementation, so patch is coming soon.

Great, this bug should be assigned to you in this case.
Comment 5 Szabolcs David 2014-01-24 02:21:38 PST
Created attachment 222089 [details]
First draft

Sorry for the delay. I have worked on this, but I have been moved to another project and I won't have time work on this anymore.
Comment 6 Brent Fulgham 2014-01-30 10:05:11 PST
Comment on attachment 222089 [details]
First draft

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

This seems fine to me, but someone using the Curl backend more frequently should confirm it doesn't cause any problems.

> Source/WebCore/platform/network/curl/ResourceHandleManager.cpp:404
> +    String location = d->m_response.httpHeaderField("location");

This could probably be a const String&
Comment 7 Brent Fulgham 2014-01-30 10:05:33 PST
+peavo
Comment 8 peavo 2014-01-30 13:16:55 PST
(In reply to comment #7)
> +peavo

Will give it a test.
Comment 9 peavo 2014-01-31 09:25:42 PST
On Windows, I seem to be having some problems logging into hotmail with this patch.
There seems to be a redirect, but after that nothing happens, and the displayed page is empty, not sure why, though.
Comment 10 peavo 2014-01-31 09:51:30 PST
(In reply to comment #9)
> On Windows, I seem to be having some problems logging into hotmail with this patch.
> There seems to be a redirect, but after that nothing happens, and the displayed page is empty, not sure why, though.

I'm sorry, disregard my last comment. This behavior is exactly the same without the patch, so it must be caused by other changes.
Comment 11 peavo 2014-12-30 14:44:15 PST
Should we commit this one?
Comment 12 peavo 2015-01-06 11:03:40 PST
(In reply to comment #11)
> Should we commit this one?

I've been running with this patch, and have had some crashes in the network backend because the ResourceHandleClient has been used after deletion.
Comment 13 Basuke Suzuki 2017-07-19 11:54:04 PDT
Comment on attachment 222089 [details]
First draft

It contains different patchs
Comment 14 Basuke Suzuki 2017-07-19 11:55:40 PDT
We've start working on this issue again.
Comment 15 Basuke Suzuki 2017-09-29 15:17:17 PDT
Created attachment 322244 [details]
patch
Comment 16 Build Bot 2017-09-29 15:19:37 PDT
Attachment 322244 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 17 Basuke Suzuki 2017-09-29 15:34:08 PDT
Created attachment 322246 [details]
patch
Comment 18 Alex Christensen 2017-09-29 16:07:11 PDT
Comment on attachment 322246 [details]
patch

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

> Source/WebCore/platform/network/ResourceHandle.cpp:173
> +#if !PLATFORM(COCOA) && !USE(CFURLCONNECTION) && !USE(SOUP) && !USE(CURL)

That's everyone.  Let's just get rid of this.

> Source/WebCore/platform/network/curl/ResourceHandleCurlDelegate.cpp:281
> +    if ((request.httpMethod() == "GET") || (request.httpMethod() == "HEAD"))

unnecessary parentheses.

> Source/WebCore/platform/network/curl/ResourceHandleCurlDelegate.cpp:371
> +    if (protocolHostAndPortAreEqual(m_currentRequest.url(), response().url())) {
> +        auto credential = getCredential(m_currentRequest, true);

This is probably ok, but will likely need to be refined.  It ignores the realm of basic authentication credentials, for example.

> Source/WebCore/platform/network/curl/ResourceResponseCurl.cpp:154
> +    return (httpStatusCode() == 301);

parentheses unnecessary.
Comment 19 Basuke Suzuki 2017-09-29 16:44:47 PDT
> > Source/WebCore/platform/network/curl/ResourceHandleCurlDelegate.cpp:371
> > +    if (protocolHostAndPortAreEqual(m_currentRequest.url(), response().url())) {
> > +        auto credential = getCredential(m_currentRequest, true);
> 
> This is probably ok, but will likely need to be refined.  It ignores the
> realm of basic authentication credentials, for example.

Oh, okay. To think about realm, this place is too late for that check. Also actually the comparison of (protocol, host, port) sets was made in willSendRequest already.

We'll discuss this one again. Thank you.
Comment 20 Basuke Suzuki 2017-10-02 10:52:28 PDT
(In reply to Alex Christensen from comment #18)
> > Source/WebCore/platform/network/curl/ResourceHandleCurlDelegate.cpp:371
> > +    if (protocolHostAndPortAreEqual(m_currentRequest.url(), response().url())) {
> > +        auto credential = getCredential(m_currentRequest, true);
> 
> This is probably ok, but will likely need to be refined.  It ignores the
> realm of basic authentication credentials, for example.

Alex, I lost the point you mentioned. What is your point? We cannot compare to any realm at this point because we're about to send a new request. Authentication info will be added  only if getCredential() returns valid information and that's when the url matches previous url who resulted with credential.

Of course, getCredential() can be more clear if it returns std::optional<std::pair<String, String>>. Is that what you want?
Comment 21 Alex Christensen 2017-10-02 10:54:49 PDT
Ah, I think you're right.  returning an optional would be a little more clear, but I guess that's not critical to have in this same patch.
Comment 22 Basuke Suzuki 2017-10-02 11:00:01 PDT
(In reply to Alex Christensen from comment #21)
> Ah, I think you're right.  returning an optional would be a little more
> clear, but I guess that's not critical to have in this same patch.

Okay. Can you give cq+?
Comment 23 Alex Christensen 2017-10-02 11:43:18 PDT
Comment on attachment 322246 [details]
patch

Sure, remove extra parentheses soon.
Comment 24 WebKit Commit Bot 2017-10-02 12:00:20 PDT
Comment on attachment 322246 [details]
patch

Clearing flags on attachment: 322246

Committed r222728: <http://trac.webkit.org/changeset/222728>
Comment 25 WebKit Commit Bot 2017-10-02 12:00:22 PDT
All reviewed patches have been landed.  Closing bug.
Comment 26 Radar WebKit Bug Importer 2017-10-02 12:01:00 PDT
<rdar://problem/34772610>