Please look out for cookie handling during content reviews

[Jochen Eisinger]

Hey,

during the past few days, I came across two instances of new code using cookies that was not correctly hooked up to the cookie APIs.

In one instance, the CookieStore was taken from the URLRequestContext to retrieve cookies (the correct way would be to get the CookieMonster and ask the NetworkDelegate for permission first): http://codereview.chromium.org/10449017/

In the second instance, the cookie was retrieved from the CookieJar (which is hooked up to all the required permission logic - from the renderer you can hardly work around this anyway), but an incorrect first-party URL was provided (you should always pass in frame->document().firstPartyForCookies() for resources that are requested for a given frame): https://chromiumcodereview.appspot.com/10413015/

When reviewing new code that touches cookies or other site data, please review it carefully that it does the right thing[tm], or forward the review in question to someone from the owp-storage or privacy team.

thanks

-jochen

[John Abd-El-Malek]

This stuff seems tricky to catch in code reviews, i.e. it could slip by. Is there anyway to make these checks at runtime, even for just debug builds? thinking out loud, can chrome set a mode in the net layer that  causes it to force that the right object is used to retrieve cookies? can the object that't taking the URL take in a higher-level class so that it can use the correct url itself?

[Jochen Eisinger]

On Mon, May 28, 2012 at 9:47 AM, John Abd-El-Malek <j...@chromium.org> wrote:

This stuff seems tricky to catch in code reviews, i.e. it could slip by. Is there anyway to make these checks at runtime, even for just debug builds? thinking out loud, can chrome set a mode in the net layer that  causes it to force that the right object is used to retrieve cookies? can the object that't taking the URL take in a higher-level class so that it can use the correct url itself?

The problem is that e.g. for the cookie jar, we have a few locations that run outside of the context of a frame, so we could add an convenience method, and hope that people will use that method instead of the one just taking URLs.

On the browser side, I don't know the design decisions behind introducing a cookie store and only ever using the cookie monster interface. :-/

[Avi Drissman]

Is it possible to change the API to make it easier to do the right thing? Looking at the CLs that you noted, I'm pretty sure that I would have done the obvious (and wrong) thing.

Avi