Bug 184948 – Don't Block First Party Cookies on Redirects

• Component: WebKit2
• Status: RESOLVED FIXED

[Brent Fulgham]

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.

[Brent Fulgham]

Created attachment 338704 [details]
Patch

[EWS Watchlist]

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.

[Brent Fulgham]

Created attachment 338741 [details]
Patch

[youenn fablet]

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?

[Brent Fulgham]

Created attachment 338743 [details]
Patch

[John Wilander]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[Brent Fulgham]

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

[Brent Fulgham]

Created attachment 338753 [details]
Patch

[John Wilander]

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.

[Brent Fulgham]

Created attachment 338761 [details]
Patch

[John Wilander]

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.

[youenn fablet]

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.

[Brent Fulgham]

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.

[Brent Fulgham]

Created attachment 338765 [details]
Patch for landing

[Daniel Bates]

We already pass information about whether a request is a top-level navigation as part of the request. See ResourceRequest::isTopSite().

[Daniel Bates]

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

[Brent Fulgham]

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

[Brent Fulgham]

Committed r231008: <https://trac.webkit.org/changeset/231008>

[Radar WebKit Bug Importer]

<rdar://problem/39730023>

[Chris Dumez]

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

[Chris Dumez]

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

[Brent Fulgham]

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

[Daniel Bates]

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.