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
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 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.
ApplicationCacheGroup at least shouldn't take it into account. Cache group is not tied to a single document.
*** 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?
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
Created attachment 182545 [details] Patch
Comment on attachment 182545 [details] Patch Attachment 182545 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/15867449
Comment on attachment 182545 [details] Patch Attachment 182545 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/15870485
Comment on attachment 182545 [details] Patch Attachment 182545 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/15875471
Created attachment 182549 [details] Patch
Comment on attachment 182549 [details] Patch Attachment 182549 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15867462
Comment on attachment 182549 [details] Patch Attachment 182549 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/15841502
Created attachment 182564 [details] Patch
Created attachment 182568 [details] Patch
Created attachment 182573 [details] Patch
Comment on attachment 182573 [details] Patch ap@, could you have a look at the new version of this patch? Thanks!
Comment on attachment 182573 [details] Patch Attachment 182573 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/15853788
Created attachment 182719 [details] Patch
beidson@, could you review this? (ap@ redirected me to you.)
This touches a lot of surface area. I can't meaningfully review this today.
beidson@: A friendly ping, could you have a look at this, or suggest another reviewer?
@beidson: Are you interested in reviewing this patch? If not, I can try to take a look.
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 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".
Confirming Alexey: Please feel free to move forward without any further input from me. Another EWS run might be important.
Created attachment 186032 [details] Patch
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 on attachment 186032 [details] Patch Attachment 186032 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16296744
Comment on attachment 186032 [details] Patch Attachment 186032 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16301851
Created attachment 186350 [details] Patch
Created attachment 186354 [details] Patch
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 on attachment 186354 [details] Patch Attachment 186354 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16372217
Comment on attachment 186354 [details] Patch Attachment 186354 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16379110
Created attachment 186376 [details] Patch
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 on attachment 186376 [details] Patch Attachment 186376 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16371301
Comment on attachment 186376 [details] Patch Attachment 186376 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16374231
Created attachment 186417 [details] Patch
Comment on attachment 186417 [details] Patch Attachment 186417 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16372329
Created attachment 186421 [details] Patch
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 on attachment 186421 [details] Patch /Now with fixed build.) abarth & ap, could you have another look?
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
Created attachment 186793 [details] Patch
Comment on attachment 186793 [details] Patch Clearing flags on attachment: 186793 Committed r141981: <http://trac.webkit.org/changeset/141981>
All reviewed patches have been landed. Closing bug.