Bug 86000 – Take referrer policy into account when clearing the referrer header

• Component: New Bugs
• Status: RESOLVED FIXED

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

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

Created attachment 141552 [details]
Patch

[Adam Barth]

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.

[Alexey Proskuryakov]

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.

(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.

[Alexey Proskuryakov]

ApplicationCacheGroup at least shouldn't take it into account. Cache group is not tied to a single document.

[Zan Dobersek]

*** Bug 86061 has been marked as a duplicate of this bug. ***

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?

[Gyuyoung Kim]

If possible, please take care EFL port as well.

http://trac.webkit.org/browser/trunk/LayoutTests/platform/efl/Skipped#L1043

(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

[Marja Hölttä]

Created attachment 182545 [details]
Patch

[Early Warning System Bot]

Comment on attachment 182545 [details]
Patch

Attachment 182545 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15867449

[Early Warning System Bot]

Comment on attachment 182545 [details]
Patch

Attachment 182545 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15870485

[EFL EWS Bot]

Comment on attachment 182545 [details]
Patch

Attachment 182545 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15875471

[Marja Hölttä]

Created attachment 182549 [details]
Patch

[Build Bot]

Comment on attachment 182549 [details]
Patch

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

[EFL EWS Bot]

Comment on attachment 182549 [details]
Patch

Attachment 182549 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15841502

[Marja Hölttä]

Created attachment 182564 [details]
Patch

[Marja Hölttä]

Created attachment 182568 [details]
Patch

[Marja Hölttä]

Created attachment 182573 [details]
Patch

[Marja Hölttä]

Comment on attachment 182573 [details]
Patch

ap@, could you have a look at the new version of this patch? Thanks!

[Build Bot]

Comment on attachment 182573 [details]
Patch

Attachment 182573 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15853788

[Marja Hölttä]

Created attachment 182719 [details]
Patch

[Marja Hölttä]

beidson@, could you review this? (ap@ redirected me to you.)

[Brady Eidson]

This touches a lot of surface area.  I can't meaningfully review this today.

[Marja Hölttä]

beidson@: A friendly ping, could you have a look at this, or suggest another reviewer?

[Adam Barth]

@beidson: Are you interested in reviewing this patch?  If not, I can try to take a look.

[Adam Barth]

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.

[Alexey Proskuryakov]

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".

[Brady Eidson]

Confirming Alexey: Please feel free to move forward without any further input from me.

Another EWS run might be important.

[Marja Hölttä]

Created attachment 186032 [details]
Patch

[Build Bot]

Comment on attachment 186032 [details]
Patch

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

[Build Bot]

Comment on attachment 186032 [details]
Patch

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

[Build Bot]

Comment on attachment 186032 [details]
Patch

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

[Marja Hölttä]

Created attachment 186350 [details]
Patch

[Marja Hölttä]

Created attachment 186354 [details]
Patch

[Build Bot]

Comment on attachment 186354 [details]
Patch

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

[Build Bot]

Comment on attachment 186354 [details]
Patch

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

[Build Bot]

Comment on attachment 186354 [details]
Patch

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

[Marja Hölttä]

Created attachment 186376 [details]
Patch

[Alexey Proskuryakov]

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).

[Build Bot]

Comment on attachment 186376 [details]
Patch

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

[Build Bot]

Comment on attachment 186376 [details]
Patch

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

[Marja Hölttä]

Created attachment 186417 [details]
Patch

[Build Bot]

Comment on attachment 186417 [details]
Patch

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

[Marja Hölttä]

Created attachment 186421 [details]
Patch

[Marja Hölttä]

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.

[Marja Hölttä]

Comment on attachment 186421 [details]
Patch

/Now with fixed build.) abarth & ap, could you have another look?

[WebKit Review Bot]

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

[Marja Hölttä]

Created attachment 186793 [details]
Patch

[WebKit Review Bot]

Comment on attachment 186793 [details]
Patch

Clearing flags on attachment: 186793

Committed r141981: <http://trac.webkit.org/changeset/141981>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.