Cookie blocking behavior for prevalent resources should not be applied to first party cookies. It looks like the redirect logic currently treats first party cookies after a redirect as third party.
Created attachment 338704 [details] Patch
Attachment 338704 [details] did not pass style-queue: ERROR: Source/WebKit/NetworkProcess/capture/NetworkCaptureReplayer.h:31: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 35 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 338741 [details] Patch
Comment on attachment 338741 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=338741&action=review > Source/WebKit/NetworkProcess/NetworkDataTask.h:160 > + bool m_dataTaskIsForNavigation { false }; I am a bit confused here. It seems m_dataTaskIsForNavigation will be equal to options.mode == FetchOptions::Mode::Navigate. But isTopLevelNavigation() should return true if m_dataTaskIsForNavigation is true and requester is main frame, right? > Source/WebKit/NetworkProcess/NetworkDataTaskBlob.cpp:75 > + : NetworkDataTask(session, client, request, StoredCredentialsPolicy::DoNotUse, false, loadIsForNavigation) Do we need this additional parameter for blobs?
Created attachment 338743 [details] Patch
See if adding && frame.isMainFrame() to your if statement in NavigationScheduler.cpp fixes the failing cookie test. If so, we should figure out why initiatedByMainFrame() isn't returning what we believe it should.
Comment on attachment 338743 [details] Patch Attachment 338743 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/7451965 New failing tests: http/tests/misc/will-send-request-returns-null-on-redirect.html http/tests/navigation/redirect-to-fragment.html http/tests/security/strip-referrer-to-origin-for-third-party-redirects-in-private-mode.html http/tests/navigation/redirect-preserves-fragment.html http/tests/security/cookies/third-party-cookie-blocking-redirect.html http/tests/loading/server-redirect-for-provisional-load-caching.html
Created attachment 338746 [details] Archive of layout-test-results from ews107 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
(In reply to youenn fablet from comment #4) > Comment on attachment 338741 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=338741&action=review > > > Source/WebKit/NetworkProcess/NetworkDataTask.h:160 > > + bool m_dataTaskIsForNavigation { false }; > > I am a bit confused here. > It seems m_dataTaskIsForNavigation will be equal to options.mode == > FetchOptions::Mode::Navigate. > But isTopLevelNavigation() should return true if m_dataTaskIsForNavigation > is true and requester is main frame, right? Yes -- I just discovered this THROUGH TESTING! My new behavior was being applied to iframe as well, which was wrong. I have a fix coming shortly. > > Source/WebKit/NetworkProcess/NetworkDataTaskBlob.cpp:75 > > + : NetworkDataTask(session, client, request, StoredCredentialsPolicy::DoNotUse, false, loadIsForNavigation) > > Do we need this additional parameter for blobs? Yes, I think so because blob URLs can be used for main frame navigation.
Created attachment 338753 [details] Patch
Comment on attachment 338753 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=338753&action=review > Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:-330 > - This seems to be a stray whitespace change. > Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:-334 > - This one too. > Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:373 > #if !RELEASE_LOG_DISABLED Couldn't this be done like so: if (isTopLevelNavigation()) request.setFirstPartyForCookies(request.url()); shouldBlockCookies = m_session->networkStorageSession().shouldBlockCookies(request); Is it because you want to keep the original block result for the use of redirectingToPrevalentResource in the referrer-to-origin code? I think it's OK to not change the referrer on top level navigations, and thus have an exact match between when we block cookies and when we change the referrer.
Created attachment 338761 [details] Patch
Please check with Dan Bates that his SameSite cookies implementation don't depend on the old "firstPartyForCookies" since there are special rules for strict SameSite cookies in navigations. Other than that, looks good to me, as long as it passes the tests.
Comment on attachment 338761 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=338761&action=review > Source/WebKit/NetworkProcess/NetworkDataTaskBlob.h:57 > + NetworkDataTaskBlob(NetworkSession&, NetworkDataTaskClient&, const WebCore::ResourceRequest&, WebCore::ContentSniffingPolicy, const Vector<RefPtr<WebCore::BlobDataFileReference>>&, bool); It does not seem isMainFrameNavigation is useful for blobs since there are no redirections in that case. > Source/WebKit/NetworkProcess/NetworkLoadParameters.h:51 > + bool isMainFrameNavigation { false }; Seems good for now. I wonder though whether we do not add some redundant information since resourceRequest.requester also has that same information. Can we try to use that information instead? At least, we should add some ASSERT in WebLoaderStrategy. It seems to me that isMainFrameNavigation is more robust since we do not mess as much with requests.
Comment on attachment 338761 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=338761&action=review >> Source/WebKit/NetworkProcess/NetworkDataTaskBlob.h:57 >> + NetworkDataTaskBlob(NetworkSession&, NetworkDataTaskClient&, const WebCore::ResourceRequest&, WebCore::ContentSniffingPolicy, const Vector<RefPtr<WebCore::BlobDataFileReference>>&, bool); > > It does not seem isMainFrameNavigation is useful for blobs since there are no redirections in that case. OK! >> Source/WebKit/NetworkProcess/NetworkLoadParameters.h:51 >> + bool isMainFrameNavigation { false }; > > Seems good for now. > I wonder though whether we do not add some redundant information since resourceRequest.requester also has that same information. > Can we try to use that information instead? > At least, we should add some ASSERT in WebLoaderStrategy. > It seems to me that isMainFrameNavigation is more robust since we do not mess as much with requests. I did try using 'requester' in an earlier version of the patch, but found that this information was lost during redirects. Take a look at the implementation of "[NetworkSession URLSession:task:willPerformHTTPRedirection:newRequest:completionHandler:]" Since it only receives base Cocoa data types, it loses the additional data members in our ResourceRequest objects (including 'isTopSite' and 'requester'). I just filed Bug 184987 to try to address this.
Created attachment 338765 [details] Patch for landing
We already pass information about whether a request is a top-level navigation as part of the request. See ResourceRequest::isTopSite().
(In reply to Brent Fulgham from comment #15) > Comment on attachment 338761 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=338761&action=review > > >> Source/WebKit/NetworkProcess/NetworkDataTaskBlob.h:57 > >> + NetworkDataTaskBlob(NetworkSession&, NetworkDataTaskClient&, const WebCore::ResourceRequest&, WebCore::ContentSniffingPolicy, const Vector<RefPtr<WebCore::BlobDataFileReference>>&, bool); > > > > It does not seem isMainFrameNavigation is useful for blobs since there are no redirections in that case. > > OK! > > >> Source/WebKit/NetworkProcess/NetworkLoadParameters.h:51 > >> + bool isMainFrameNavigation { false }; > > > > Seems good for now. > > I wonder though whether we do not add some redundant information since resourceRequest.requester also has that same information. > > Can we try to use that information instead? > > At least, we should add some ASSERT in WebLoaderStrategy. > > It seems to me that isMainFrameNavigation is more robust since we do not mess as much with requests. > > I did try using 'requester' in an earlier version of the patch, but found > that this information was lost during redirects. > > Take a look at the implementation of "[NetworkSession > URLSession:task:willPerformHTTPRedirection:newRequest:completionHandler:]" > > Since it only receives base Cocoa data types, it loses the additional data > members in our ResourceRequest objects (including 'isTopSite' and > 'requester'). > > I just filed Bug 184987 to try to address this. I see. I would suggest that we address this bug as part of this fix. Otherwise, it seems noisy to have to effectively revert the majority of this patch once we fix that bug. If you feel strongly about landing this patch as-is then feel free to override my cq-. If you do this, then please treat fixing bug #184987 with the same priority and urgency as this bug because not fixing bug #184987 has security implications.
(In reply to Daniel Bates from comment #18) > (In reply to Brent Fulgham from comment #15) > > Comment on attachment 338761 [details] > > Patch > > I just filed Bug 184987 to try to address this. > > I see. I would suggest that we address this bug as part of this fix. > Otherwise, it seems noisy to have to effectively revert the majority of this > patch once we fix that bug. If you feel strongly about landing this patch > as-is then feel free to override my cq-. If you do this, then please treat > fixing bug #184987 with the same priority and urgency as this bug because > not fixing bug #184987 has security implications. I'm going to choose to land as-is and address the follow-up separately because of scheduling issues. Please come see me if you want details.
Committed r231008: <https://trac.webkit.org/changeset/231008>
<rdar://problem/39730023>
(In reply to Radar WebKit Bug Importer from comment #21) > <rdar://problem/39730023> I think this may have broken the build: /Volumes/Data/cdumez/WebKit/OpenSource/Source/WebKit/NetworkProcess/NetworkDataTask.cpp:54:146: error: too many arguments to function call, expected 5, have 6 return NetworkDataTaskBlob::create(session, client, parameters.request, parameters.contentSniffingPolicy, parameters.blobFileReferences, parameters.isMainFrameNavigation); In file included from /Volumes/Data/cdumez/WebKit/OpenSource/Source/WebKit/NetworkProcess/NetworkDataTask.cpp:29: /Volumes/Data/cdumez/WebKit/OpenSource/Source/WebKit/NetworkProcess/NetworkDataTaskBlob.h:49:5: note: 'create' declared here static Ref<NetworkDataTask> create(NetworkSession& session, NetworkDataTaskClient& client, const WebCore::ResourceRequest& request, WebCore::ContentSniffingPolicy shouldContentSniff, const Vector<RefPtr<WebCore::BlobDataFileReference>>& fileReferences) 1 error generated.
(In reply to Chris Dumez from comment #22) > (In reply to Radar WebKit Bug Importer from comment #21) > > <rdar://problem/39730023> > > I think this may have broken the build: > /Volumes/Data/cdumez/WebKit/OpenSource/Source/WebKit/NetworkProcess/ > NetworkDataTask.cpp:54:146: error: too many arguments to function call, > expected 5, have 6 > return NetworkDataTaskBlob::create(session, client, > parameters.request, parameters.contentSniffingPolicy, > parameters.blobFileReferences, parameters.isMainFrameNavigation); > In file included from > /Volumes/Data/cdumez/WebKit/OpenSource/Source/WebKit/NetworkProcess/ > NetworkDataTask.cpp:29: > /Volumes/Data/cdumez/WebKit/OpenSource/Source/WebKit/NetworkProcess/ > NetworkDataTaskBlob.h:49:5: note: 'create' declared here > static Ref<NetworkDataTask> create(NetworkSession& session, > NetworkDataTaskClient& client, const WebCore::ResourceRequest& request, > WebCore::ContentSniffingPolicy shouldContentSniff, const > Vector<RefPtr<WebCore::BlobDataFileReference>>& fileReferences) > 1 error generated. Looks like Brent landed a build fix in https://trac.webkit.org/changeset/231010 already.
(In reply to Chris Dumez from comment #23) > > Vector<RefPtr<WebCore::BlobDataFileReference>>& fileReferences) > > 1 error generated. > > Looks like Brent landed a build fix in > https://trac.webkit.org/changeset/231010 already. Yes, sorry for the trouble! Landing fail.
Comment on attachment 338765 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=338765&action=review > Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:363 > + if (isTopLevelNavigation()) > + request.setFirstPartyForCookies(request.url()); As per our in person conversation today (04/25) we should also do a comparable fix for WebKit Legacy unless we already do this.