Bug 131026 – [SOUP] Control cookie management according ResourceRequest.allowCookies()

• Component: Platform
• Status: RESOLVED FIXED

[youenn fablet]

ResourceRequest.allowCookies() is used to control the sending and setting of cookies by the HTTP layer.
For instance, cookies are disabled for cross-origin requests for which stored credentials are not allowed.

The soup layer does not seem to handle that correctly.
One reason may be that cookies handling is set at the soup session level.

Test http/tests/xmlhttprequest/cross-origin-cookie-storage.html is probably related to that issue.

[youenn fablet]

Created attachment 228307 [details]
Temp fix working for wk1 but not for wk2

[youenn fablet]

The temp fix passes http/tests/xmlhttprequest/cross-origin-cookie-storage.html on WK1 (EFL and GTK) but not WK2 (EFL and GTK also).

http/tests/xmlhttprequest/cross-origin-cookie-storage.html uses testRunner.setAlwaysAcceptCookies to allow all cookies.

On WK1, the cookie jar policy of the soup session is correctly set to SOUP_COOKIE_JAR_ACCEPT_ALWAYS .
On WK2, the cookie jar policy remains to SOUP_COOKIE_JAR_ACCEPT_NO_THIRD_PARTY, defeating the purpose of the test.

[youenn fablet]

Created attachment 228506 [details]
Fixing WK2

[youenn fablet]

Second patch runs successfully for GTK/WK1, GTK/WK2 and EFL/WK1.
It should also work for EFL/WK2 but I am not able to test it.

[Sergio Villar Senin]

Comment on attachment 228506 [details]
Fixing WK2

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

> Source/WebKit2/WebProcess/Cookies/soup/WebCookieManagerSoup.cpp:64
>  }

I'm not sure about this. Is it really required for the fix?

Also checking mac's implementation, they set that accept policy also for all sessions in SessionTracker. I think we need to research a bit more on this.

[youenn fablet]

(In reply to comment #5)
> (From update of attachment 228506 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=228506&action=review
> 
> > Source/WebKit2/WebProcess/Cookies/soup/WebCookieManagerSoup.cpp:64
> >  }
> 
> I'm not sure about this. Is it really required for the fix?

It is needed to pass the test, as the following happens:
1. the default session switches to a testing session, with the creation of a private cookie jar.
2. The cookie policy is changed, changing the default cookie jar but not the cookie jar of the default session (the testing session).
 
> Also checking mac's implementation, they set that accept policy also for all sessions in SessionTracker. I think we need to research a bit more on this.

Mac implementation sets the cookie policy of the default session cookie jar, which is similar to this patch.
Mac implementation apparently has no notion of default cookie jar as for soup.

That said, it makes sense to set the accept policy to all existing sessions, including private sessions. I will upload a new patch accordingly.

[youenn fablet]

Humm, having another look at SoupNetworkSession, is there any reason why private browsing sessions reuse the default cookie jar? Should they not create their own private cookie jar?

[Sergio Villar Senin]

(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 228506 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=228506&action=review

> Mac implementation sets the cookie policy of the default session cookie jar, which is similar to this patch.
> Mac implementation apparently has no notion of default cookie jar as for soup.

They do. Check WebFrameNetworkingContext:::setCookieAcceptPolicyForAllContexts() in mac code. They set the policy for the shared cookie jar and the default session cookie jar and then all existing sessions.
 
> That said, it makes sense to set the accept policy to all existing sessions, including private sessions. I will upload a new patch accordingly.

We should probably use the same approach as Mac port does, moving that code to the WebFrameNetworkingContext.

[youenn fablet]

Created attachment 228595 [details]
Adding private session cookie policy setting

[Sergio Villar Senin]

Comment on attachment 228595 [details]
Adding private session cookie policy setting

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

Thanks, I believe this is indeed the way to go. I'll let kov and mrobinson to take a look at this before giving the r+ though.

> Source/WebKit2/WebProcess/WebCoreSupport/soup/WebFrameNetworkingContext.cpp:57
> +    SoupCookieJarAcceptPolicy soupPolicy;

Nit: move cookieJar declaration and initialization after the switch as you use it there for the first time. You can also initialize soupPolicy to ACCEPT_ALWAYS here and remove the line bellow.

[Gustavo Noronha (kov)]

Comment on attachment 228595 [details]
Adding private session cookie policy setting

This looks sane to me, yeah

[youenn fablet]

Created attachment 228812 [details]
Improved style

[Sergio Villar Senin]

Comment on attachment 228812 [details]
Improved style

Awesome! Thanks!

[WebKit Commit Bot]

Comment on attachment 228812 [details]
Improved style

Clearing flags on attachment: 228812

Committed r166924: <http://trac.webkit.org/changeset/166924>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.