Bug 86000

Summary: Take referrer policy into account when clearing the referrer header
Product: WebKit Reporter: jochen
Component: New BugsAssignee: Marja Hölttä <marja>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ap, beidson, buildbot, danw, gns, gyuyoung.kim, japhet, jochen, laszlo.gombos, marja, mifenton, mrobinson, rakuco, rniwa, rwlbuis, tonikitoo, webkit-ews, webkit.review.bot, yong.li.webkit, zan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 86061    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description jochen 2012-05-09 09:35:10 PDT
In order to correctly support the meta-referrer tag, network stacks used by the various ports need to take the referrer policy into account when clearing the Referrer header:

e.g. in Source/WebCore/platform/network/mac/ResourceHandleMac.mm the referrer header is cleared when redirecting from https to !https. This should only happen, if the referrer policy for this request as "default". For others ("always", "never", "origin") the header should not be touched.

Test is in http/tests/security/referrer-policy-redirect-link.html
Comment 1 jochen 2012-05-09 10:48:21 PDT
Alexey suggests to move the logic that strips the referrer from the platform specific ResourceHandle implementations to ResourceLoader::willSendRequest which is invoked on every redirect anyway (different from chrome which does the resource loading out of process). In ResourceLoader, we have access to the frame and thus the document's referrer policy
Comment 2 jochen 2012-05-11 23:08:12 PDT
Created attachment 141552 [details]
Patch
Comment 3 Adam Barth 2012-05-11 23:11:54 PDT
Comment on attachment 141552 [details]
Patch

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

Holy duplicated code batman.

> Source/WebCore/loader/ResourceLoader.cpp:237
> +    if (!m_frame->document() || m_frame->document()->referrerPolicy() == ReferrerPolicyDefault) {

!m_frame->document() <- shouldn't be possible.
Comment 4 Alexey Proskuryakov 2012-05-12 00:37:58 PDT
Comment on attachment 141552 [details]
Patch

I suggested this, but now I realize that this is a fragile solution. Every ResourceHandleClient now needs to implement this, and ResourceHandle may still need this when its client is null.

Specifically, BlobResourceSynchronousLoader is a ResourceHandleClient, and need to implement the same logic. And so is ApplicationCacheGroup.

I guess that we need to add a new client call, so that clients that care would be able to customize the behavior that's still in ResourceHandle. Sorry for a false lead.
Comment 5 jochen 2012-05-12 18:46:00 PDT
(In reply to comment #4)
> (From update of attachment 141552 [details])
> I suggested this, but now I realize that this is a fragile solution. Every ResourceHandleClient now needs to implement this, and ResourceHandle may still need this when its client is null.
> 
> Specifically, BlobResourceSynchronousLoader is a ResourceHandleClient, and need to implement the same logic. And so is ApplicationCacheGroup.
> 
> I guess that we need to add a new client call, so that clients that care would be able to customize the behavior that's still in ResourceHandle. Sorry for a false lead.

Right. Should we make it a pure virtual function? After all, all possible code paths should take the policy into account.
Comment 6 Alexey Proskuryakov 2012-05-13 22:01:10 PDT
ApplicationCacheGroup at least shouldn't take it into account. Cache group is not tied to a single document.
Comment 7 Zan Dobersek 2012-05-16 12:36:21 PDT
*** Bug 86061 has been marked as a duplicate of this bug. ***
Comment 8 jochen 2012-07-17 04:23:53 PDT
I revived that patch, however, I ran into another problem. E.g. ResourceHandleCFNet implements a ResourceHandleClient for synchronous loads which would need to know the referrer policy. A possible solution would be to include whether or not to apply the default referrer policy in the NetworkingContext which is passed for a synchronous load. wdyt?
Comment 9 Gyuyoung Kim 2012-07-17 04:29:19 PDT
If possible, please take care EFL port as well.

http://trac.webkit.org/browser/trunk/LayoutTests/platform/efl/Skipped#L1043
Comment 10 jochen 2012-07-17 04:40:17 PDT
(In reply to comment #9)
> If possible, please take care EFL port as well.
> 
> http://trac.webkit.org/browser/trunk/LayoutTests/platform/efl/Skipped#L1043

Sure, will do
Comment 11 Marja Hölttä 2013-01-14 04:49:03 PST
Created attachment 182545 [details]
Patch
Comment 12 Early Warning System Bot 2013-01-14 04:55:50 PST
Comment on attachment 182545 [details]
Patch

Attachment 182545 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15867449
Comment 13 Early Warning System Bot 2013-01-14 04:57:20 PST
Comment on attachment 182545 [details]
Patch

Attachment 182545 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15870485
Comment 14 EFL EWS Bot 2013-01-14 04:58:16 PST
Comment on attachment 182545 [details]
Patch

Attachment 182545 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15875471
Comment 15 Marja Hölttä 2013-01-14 05:02:14 PST
Created attachment 182549 [details]
Patch
Comment 16 Build Bot 2013-01-14 05:47:52 PST
Comment on attachment 182549 [details]
Patch

Attachment 182549 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15867462
Comment 17 EFL EWS Bot 2013-01-14 06:04:55 PST
Comment on attachment 182549 [details]
Patch

Attachment 182549 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15841502
Comment 18 Marja Hölttä 2013-01-14 06:13:35 PST
Created attachment 182564 [details]
Patch
Comment 19 Marja Hölttä 2013-01-14 06:42:10 PST
Created attachment 182568 [details]
Patch
Comment 20 Marja Hölttä 2013-01-14 07:23:42 PST
Created attachment 182573 [details]
Patch
Comment 21 Marja Hölttä 2013-01-14 08:18:36 PST
Comment on attachment 182573 [details]
Patch

ap@, could you have a look at the new version of this patch? Thanks!
Comment 22 Build Bot 2013-01-14 18:53:32 PST
Comment on attachment 182573 [details]
Patch

Attachment 182573 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15853788
Comment 23 Marja Hölttä 2013-01-15 01:46:27 PST
Created attachment 182719 [details]
Patch
Comment 24 Marja Hölttä 2013-01-15 02:07:21 PST
beidson@, could you review this? (ap@ redirected me to you.)
Comment 25 Brady Eidson 2013-01-15 12:41:13 PST
This touches a lot of surface area.  I can't meaningfully review this today.
Comment 26 Marja Hölttä 2013-01-18 07:37:15 PST
beidson@: A friendly ping, could you have a look at this, or suggest another reviewer?
Comment 27 Adam Barth 2013-01-30 00:08:33 PST
@beidson: Are you interested in reviewing this patch?  If not, I can try to take a look.
Comment 28 Adam Barth 2013-01-30 00:11:50 PST
Comment on attachment 182719 [details]
Patch

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

> Source/WebCore/platform/network/ResourceHandle.cpp:57
> -ResourceHandle::ResourceHandle(const ResourceRequest& request, ResourceHandleClient* client, bool defersLoading, bool shouldContentSniff)
> -    : d(adoptPtr(new ResourceHandleInternal(this, request, client, defersLoading, shouldContentSniff && shouldContentSniffURL(request.url()))))
> +ResourceHandle::ResourceHandle(const ResourceRequest& request, ResourceHandleClient* client, bool defersLoading, bool shouldContentSniff, NetworkingContext* context)
> +    : d(adoptPtr(new ResourceHandleInternal(this, request, client, defersLoading, shouldContentSniff && shouldContentSniffURL(request.url()), context)))

I haven't studied the full patch in detail, but it looks like the crux of the issue is whether it makes sense to change these two lines in this way.

@beidson: If you don't have time to review the entire patch, I'd certainly welcome your views on this question.
Comment 29 Alexey Proskuryakov 2013-01-30 10:43:55 PST
Comment on attachment 182719 [details]
Patch

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

I asked Marja to talk to Brady, because Brady was doing some refactoring of sync loads, and the approaches could be in conflict.

Brady's work has already landed, and it's orthogonal to this patch, so this is no longer gated on him. The only thing is that it would be useful to run the patch by EWS again, as it could be breaking the build now.

> Source/WebCore/loader/FrameNetworkingContext.h:37
> +    virtual bool shouldClearReferrerPolicyOnHttpsToHttpRedirect() const

WebKit style to to keep acronyms in upper case in function names (so, HTTPSToHTTPRedirect, not HttpsToHttpRedirect).

> Source/WebCore/platform/network/BlobResourceHandle.cpp:144
> +    : ResourceHandle(request, client, false, false, 0)

This special case is certainly not nice (but probably not worse that existing peculiarities of BlobResourceHandle).

> Source/WebCore/platform/network/NetworkingContext.h:68
> +    virtual bool shouldClearReferrerPolicyOnHttpsToHttpRedirect() const { return true; }

Is using this base class implementation ever the right behavior? It doesn't look like it is.

I think that this function should be pure virtual, or at the very least, a comment should explain that every subclass should override it, and that hasn't been done only due to implementation difficulty of fixing all ports.

>> Source/WebCore/platform/network/ResourceHandle.cpp:57
>> +    : d(adoptPtr(new ResourceHandleInternal(this, request, client, defersLoading, shouldContentSniff && shouldContentSniffURL(request.url()), context)))
> 
> I haven't studied the full patch in detail, but it looks like the crux of the issue is whether it makes sense to change these two lines in this way.
> 
> @beidson: If you don't have time to review the entire patch, I'd certainly welcome your views on this question.

Adam, please feel free to review this patch.

The only thing that worries me a little is possible reference cycles between ResourceHandle and NetworkingContext form the new RefPtr member, but they don't seem too likely.

> Source/WebCore/platform/network/ResourceHandle.h:219
> +    ResourceHandle(const ResourceRequest&, ResourceHandleClient*, bool defersLoading, bool shouldContentSniff, NetworkingContext*);

ResourceHandle::create takes NetworkingContext* as the first argument, and so should the constructor, for consistency.

> Source/WebCore/platform/network/ResourceHandleInternal.h:86
> +        ResourceHandleInternal(ResourceHandle* loader, const ResourceRequest& request, ResourceHandleClient* c, bool defersLoading, bool shouldContentSniff, NetworkingContext* context)

Now that we have both "c" and "context", it's time to rename "c" to "client".
Comment 30 Brady Eidson 2013-01-30 10:54:46 PST
Confirming Alexey: Please feel free to move forward without any further input from me.

Another EWS run might be important.
Comment 31 Marja Hölttä 2013-02-01 06:33:09 PST
Created attachment 186032 [details]
Patch
Comment 32 Build Bot 2013-02-01 07:59:43 PST
Comment on attachment 186032 [details]
Patch

Attachment 186032 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16298707
Comment 33 Build Bot 2013-02-01 09:23:21 PST
Comment on attachment 186032 [details]
Patch

Attachment 186032 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16296744
Comment 34 Build Bot 2013-02-01 10:16:06 PST
Comment on attachment 186032 [details]
Patch

Attachment 186032 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16301851
Comment 35 Marja Hölttä 2013-02-04 04:32:30 PST
Created attachment 186350 [details]
Patch
Comment 36 Marja Hölttä 2013-02-04 04:55:56 PST
Created attachment 186354 [details]
Patch
Comment 37 Build Bot 2013-02-04 05:28:42 PST
Comment on attachment 186354 [details]
Patch

Attachment 186354 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16376127
Comment 38 Build Bot 2013-02-04 06:18:20 PST
Comment on attachment 186354 [details]
Patch

Attachment 186354 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16372217
Comment 39 Build Bot 2013-02-04 06:50:21 PST
Comment on attachment 186354 [details]
Patch

Attachment 186354 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16379110
Comment 40 Marja Hölttä 2013-02-04 08:09:22 PST
Created attachment 186376 [details]
Patch
Comment 41 Alexey Proskuryakov 2013-02-04 08:34:24 PST
Comment on attachment 186376 [details]
Patch

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

WebKit2 changes look good to me.

> LayoutTests/ChangeLog:11
> +        Skipping the tests on wk2, because other referrer policy tests are
> +        skipped, too ( https://bugs.webkit.org/show_bug.cgi?id=73913 ).

I'm curious if bug 73913 is still an issue (commented in that bug).
Comment 42 Build Bot 2013-02-04 09:31:05 PST
Comment on attachment 186376 [details]
Patch

Attachment 186376 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16371301
Comment 43 Build Bot 2013-02-04 09:36:17 PST
Comment on attachment 186376 [details]
Patch

Attachment 186376 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16374231
Comment 44 Marja Hölttä 2013-02-04 10:36:20 PST
Created attachment 186417 [details]
Patch
Comment 45 Build Bot 2013-02-04 11:06:29 PST
Comment on attachment 186417 [details]
Patch

Attachment 186417 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16372329
Comment 46 Marja Hölttä 2013-02-04 11:25:34 PST
Created attachment 186421 [details]
Patch
Comment 47 Marja Hölttä 2013-02-05 00:23:53 PST
Comment on attachment 182719 [details]
Patch

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

>> Source/WebCore/loader/FrameNetworkingContext.h:37
>> +    virtual bool shouldClearReferrerPolicyOnHttpsToHttpRedirect() const
> 
> WebKit style to to keep acronyms in upper case in function names (so, HTTPSToHTTPRedirect, not HttpsToHttpRedirect).

Done.

>> Source/WebCore/platform/network/BlobResourceHandle.cpp:144
>> +    : ResourceHandle(request, client, false, false, 0)
> 
> This special case is certainly not nice (but probably not worse that existing peculiarities of BlobResourceHandle).

(Ack.)

>> Source/WebCore/platform/network/NetworkingContext.h:68
>> +    virtual bool shouldClearReferrerPolicyOnHttpsToHttpRedirect() const { return true; }
> 
> Is using this base class implementation ever the right behavior? It doesn't look like it is.
> 
> I think that this function should be pure virtual, or at the very least, a comment should explain that every subclass should override it, and that hasn't been done only due to implementation difficulty of fixing all ports.

Made the function pure virtual.

>>> Source/WebCore/platform/network/ResourceHandle.cpp:57
>>> +    : d(adoptPtr(new ResourceHandleInternal(this, request, client, defersLoading, shouldContentSniff && shouldContentSniffURL(request.url()), context)))
>> 
>> I haven't studied the full patch in detail, but it looks like the crux of the issue is whether it makes sense to change these two lines in this way.
>> 
>> @beidson: If you don't have time to review the entire patch, I'd certainly welcome your views on this question.
> 
> Adam, please feel free to review this patch.
> 
> The only thing that worries me a little is possible reference cycles between ResourceHandle and NetworkingContext form the new RefPtr member, but they don't seem too likely.

Some ports already have a RefPtr<NetworkingContext> in ResourceHandleInternal.

>> Source/WebCore/platform/network/ResourceHandle.h:219
>> +    ResourceHandle(const ResourceRequest&, ResourceHandleClient*, bool defersLoading, bool shouldContentSniff, NetworkingContext*);
> 
> ResourceHandle::create takes NetworkingContext* as the first argument, and so should the constructor, for consistency.

Done & also changed the parameter order in ResourceHandlerInternal ctor.

>> Source/WebCore/platform/network/ResourceHandleInternal.h:86
>> +        ResourceHandleInternal(ResourceHandle* loader, const ResourceRequest& request, ResourceHandleClient* c, bool defersLoading, bool shouldContentSniff, NetworkingContext* context)
> 
> Now that we have both "c" and "context", it's time to rename "c" to "client".

Done.
Comment 48 Marja Hölttä 2013-02-05 00:26:47 PST
Comment on attachment 186421 [details]
Patch

/Now with fixed build.) abarth & ap, could you have another look?
Comment 49 WebKit Review Bot 2013-02-05 16:33:22 PST
Comment on attachment 186421 [details]
Patch

Rejecting attachment 186421 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 186421, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
(offset 5 lines).
patching file LayoutTests/platform/mac/TestExpectations
patching file LayoutTests/platform/qt/TestExpectations
Hunk #1 succeeded at 1897 (offset 3 lines).
patching file LayoutTests/platform/win/TestExpectations
Hunk #1 succeeded at 1853 (offset 24 lines).
patching file LayoutTests/platform/wk2/TestExpectations

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', '--force', '--reviewer', 'Alexey Proskuryakov']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/16376933
Comment 50 Marja Hölttä 2013-02-06 01:56:10 PST
Created attachment 186793 [details]
Patch
Comment 51 WebKit Review Bot 2013-02-06 03:25:40 PST
Comment on attachment 186793 [details]
Patch

Clearing flags on attachment: 186793

Committed r141981: <http://trac.webkit.org/changeset/141981>
Comment 52 WebKit Review Bot 2013-02-06 03:25:48 PST
All reviewed patches have been landed.  Closing bug.