CookieMonster::DeleteCanonicalCookie still assumes creation-time uniqueness. |
|||
Issue description
for (CookieMapItPair its = cookies_.equal_range(GetKey(cookie.Domain()));
its.first != its.second; ++its.first) {
// The creation date acts as the unique index...
if (its.first->second->CreationDate() == cookie.CreationDate()) {
InternalDeleteCookie(its.first, true, DELETE_COOKIE_EXPLICIT);
I presume this should be using something like IsEquivalent, but I don't know the story behind IsEquivalentForSecureCookieMatching and everything related to that.
,
Mar 28 2018
I think we should keep the old behavior (Require an exact match, modulo last access date) when deleting cookies, mostly out of paranoia.
,
Mar 28 2018
Is that what the business with *creation_date_to_inherit in DeleteAnyEquivalentCookie all about? I think that would still keep the old creation time if something like expiration date got changed, though, so the present DeleteCanonicalCookie would be happy to delete something that got modified like that.
,
Mar 28 2018
You're right. So to maintain behavior, we'd need to check everything but last accessed and expiration date, I think?
,
Apr 3 2018
+Victor for API discussions as well. I think changes in SameSite or Priority will currently not invalidate a delete if they happen between get and delete; and if changes to things like Secure and HttpOnly get through w/o violating security policy, they will not invalidate a delete, either. I think I'll summarize the current behavior as "delete will be fine as long as the value has never changed" (which isn't the same as it's the same, as it might have been overwritten twice).
,
Apr 6 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2896ec3f8bdb1d499f825aa79484c9a57064ef20 commit 2896ec3f8bdb1d499f825aa79484c9a57064ef20 Author: Maks Orlovich <morlovich@chromium.org> Date: Fri Apr 06 13:34:27 2018 CookieMonster::DeleteCanonicalCookie: use the proper key Creation time is no longer unique (not that it ever was); (domain,path,key) is (as it's long been). Bug: 826322 Change-Id: I433fe3ceffd2b2cfeb5c27c7af0033d69e76301c Reviewed-on: https://chromium-review.googlesource.com/984514 Reviewed-by: Matt Menke <mmenke@chromium.org> Reviewed-by: Victor Costan <pwnall@chromium.org> Commit-Queue: Maks Orlovich <morlovich@chromium.org> Cr-Commit-Position: refs/heads/master@{#548762} [modify] https://crrev.com/2896ec3f8bdb1d499f825aa79484c9a57064ef20/net/cookies/cookie_monster.cc [modify] https://crrev.com/2896ec3f8bdb1d499f825aa79484c9a57064ef20/net/cookies/cookie_monster_unittest.cc
,
Apr 6 2018
|
|||
►
Sign in to add a comment |
|||
Comment 1 by morlovich@chromium.org
, Mar 28 2018+CC people for API discussion. So straightforward change fails this testcase (from CookieStoreTest.DeleteCanonicalCookieAsync) [1]: EXPECT_TRUE(this->SetCookie(cs, this->http_www_foo_.url(), "A=C;Path=/bar")); ... // Try to delete the "/bar" cookie after overwriting it with a new cookie. cookies = this->GetCookieListWithOptions(cs, this->www_foo_bar_.url(), CookieOptions()); ASSERT_EQ(1u, cookies.size()); EXPECT_TRUE(this->SetCookie(cs, this->http_www_foo_.url(), "A=D;Path=/bar")); EXPECT_EQ(0u, this->DeleteCanonicalCookie(cs, cookies[0])); ... which seems to want an updated version of cookie to not be deleted when its value changed in between when it came from GetCookieList and the delete call. Which is fair enough, given asynchrony, but I don't see this documented anywhere, so if we want to keep this behavior, it should probably be specified first. The test seems to have been added in https://codereview.chromium.org/1666513002 [1] and also something using gmock which I will defer deciphering for as much as I can.