Parsing cookie expiration time should saturate rather than fail for large dates |
|||||||||
Issue descriptionCookie expiration times are stored internally by base::Time, meaning they have a maximum supported range. Even more limiting, is the fact that we use the platform's exploded time utilities, which may limit to say 2038. The consequence of this is that if a Set-Cookie sets an expiration far in the future, and beyond either the internal range of base::Time, or the platform supported maximum, the network stack treats the Set-Cookie as if it had not specified an expiration. It would be better in these cases to treat it as if the the expiration was simply Time::Max() -- for the purposes of cookie expiration this is good enough, even if it does lose some precision. This interpretation is consistent with: https://tools.ietf.org/html/rfc6265#section-5.2.1
,
Sep 22 2016
,
Sep 22 2016
,
Sep 23 2016
Tentative hack: https://codereview.chromium.org/2358343004
,
Sep 27 2016
Should we be doing this only for cookies, or should it be a general behavior? Your hack only targets cookies, and I'm a bit worried we'll end up having to extend it to other cases as well. Hrm...So I guess we need 32-bit Linux support for Chromecast, ChromeOS, and Android (?)...All platforms we control. Does seem unfortunate to add a hack instead of fixing the underlying platforms, all of which we control.
,
Sep 27 2016
,
Sep 27 2016
What other cases are you thinking? I don't think putting this hack in the general date parsing would be wise, since whether to saturate or not depends on the consumer. Consumers requiring exactness (like certificates, or server time) would not want this. And adding separate saturating date imploding into base:: is fine, but I don't think I would propose that unless there is at least another consumer for it than cookie expirations
,
Sep 27 2016
Cache times? For file modification times saturating would probably be better than returning base::Time(0) or hard failing, more often than not. Code wanting server time probably doesn't want this, but this is certainly better than Time(0), unless it checks for parse failures (Does it?)
,
Oct 4 2016
Issue 652627 has been merged into this issue.
,
Oct 5 2016
There were some issues with time rounding on mac (1 msec was lost), which I haven't resolved yet.
,
Oct 11 2016
Hrm...When talking about a time after year 2037, how much do we care about the details? That is, leap seconds and leap years? If we don't care too much about either, we could just subtract out the years after 2037, subtract out the extra years (And subtract a day if it's Feb 29th), and create a 32-bit time_t, and then add base::TimeDelta::FromDays(extra_years * 365). That's an underestimate, but maybe it's good enough? Or we could make a separate method that does that magic. Seems silly, but then, so does the fact there's no 64-bit time_t option on some 32-bit OSes.
,
Oct 14 2016
I'm going to take this on. Current plan is to start with eroman's hack, and only handle the very specific case of Cookies. If we need to broaden it later, so be it.
,
Oct 19 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/93cde27c80e7119c5a076ea61f59cc5bd473d219 commit 93cde27c80e7119c5a076ea61f59cc5bd473d219 Author: mmenke <mmenke@chromium.org> Date: Wed Oct 19 16:07:21 2016 When parsing cookie expiration times, saturate out of range dates rather than reject them. Roughly this means cookie expiration times prior to 1970 on (non-OSX) POSIX, or after 2038 on 32-bit (non-OSX) POSIX, will now be interpreted as either very small or very large base::Time values. BUG= 649416 Review-Url: https://chromiumcodereview.appspot.com/2424443002 Cr-Commit-Position: refs/heads/master@{#426213} [modify] https://crrev.com/93cde27c80e7119c5a076ea61f59cc5bd473d219/extensions/browser/api/web_request/web_request_api_helpers.cc [modify] https://crrev.com/93cde27c80e7119c5a076ea61f59cc5bd473d219/net/cookies/canonical_cookie.cc [modify] https://crrev.com/93cde27c80e7119c5a076ea61f59cc5bd473d219/net/cookies/canonical_cookie.h [modify] https://crrev.com/93cde27c80e7119c5a076ea61f59cc5bd473d219/net/cookies/cookie_monster_store_test.cc [modify] https://crrev.com/93cde27c80e7119c5a076ea61f59cc5bd473d219/net/cookies/cookie_util.cc [modify] https://crrev.com/93cde27c80e7119c5a076ea61f59cc5bd473d219/net/cookies/cookie_util.h [modify] https://crrev.com/93cde27c80e7119c5a076ea61f59cc5bd473d219/net/cookies/cookie_util_unittest.cc
,
Oct 19 2016
,
Oct 19 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1b3ea5872041478fcd1e4fcec182961cd2d6e06d commit 1b3ea5872041478fcd1e4fcec182961cd2d6e06d Author: huangs <huangs@chromium.org> Date: Wed Oct 19 17:50:38 2016 Revert of When parsing cookie expiration times, saturate out of range dates (patchset #6 id:100001 of https://chromiumcodereview.appspot.com/2424443002/ ) Reason for revert: The new test CookieUtilTest.ParseCookieExpirationTimeBeyond2038 is failing on "Linux test dbg" bot. Original issue's description: > When parsing cookie expiration times, saturate out of range dates > rather than reject them. > > Roughly this means cookie expiration times prior to 1970 on (non-OSX) > POSIX, or after 2038 on 32-bit (non-OSX) POSIX, will now be interpreted > as either very small or very large base::Time values. > > BUG= 649416 TBR=eroman@chromium.org,rdevlin.cronin@chromium.org,mmenke@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG= 649416 Review-Url: https://chromiumcodereview.appspot.com/2433193002 Cr-Commit-Position: refs/heads/master@{#426240} [modify] https://crrev.com/1b3ea5872041478fcd1e4fcec182961cd2d6e06d/extensions/browser/api/web_request/web_request_api_helpers.cc [modify] https://crrev.com/1b3ea5872041478fcd1e4fcec182961cd2d6e06d/net/cookies/canonical_cookie.cc [modify] https://crrev.com/1b3ea5872041478fcd1e4fcec182961cd2d6e06d/net/cookies/canonical_cookie.h [modify] https://crrev.com/1b3ea5872041478fcd1e4fcec182961cd2d6e06d/net/cookies/cookie_monster_store_test.cc [modify] https://crrev.com/1b3ea5872041478fcd1e4fcec182961cd2d6e06d/net/cookies/cookie_util.cc [modify] https://crrev.com/1b3ea5872041478fcd1e4fcec182961cd2d6e06d/net/cookies/cookie_util.h [modify] https://crrev.com/1b3ea5872041478fcd1e4fcec182961cd2d6e06d/net/cookies/cookie_util_unittest.cc
,
Oct 19 2016
The failure: [ RUN ] CookieUtilTest.ParseCookieExpirationTimeBeyond2038 ../../net/cookies/cookie_util_unittest.cc:173: Failure Expected: (parsed_time) < (almost_jan_32000), actual: 1939-01-12 19:28:54.775 UTC vs 1994-09-28 15:17:20.000 UTC ../../net/cookies/cookie_util_unittest.cc:173: Failure Expected: (parsed_time) < (almost_jan_32000), actual: 1939-01-12 19:28:54.775 UTC vs 1994-09-28 15:17:20.000 UTC ../../net/cookies/cookie_util_unittest.cc:173: Failure Expected: (parsed_time) < (almost_jan_32000), actual: 1939-01-12 19:28:54.775 UTC vs 1994-09-28 15:17:20.000 UTC ../../net/cookies/cookie_util_unittest.cc:173: Failure Expected: (parsed_time) < (almost_jan_32000), actual: 1939-01-12 19:28:54.775 UTC vs 1994-09-28 15:17:20.000 UTC [ FAILED ] CookieUtilTest.ParseCookieExpirationTimeBeyond2038 (1 ms) Note that due to the issue the CL is trying to work around, the output times are bogus.
,
Oct 20 2016
Should stick this time.
,
Oct 20 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b6e737731c0b73340e969939ab1bfa2ffc7cf648 commit b6e737731c0b73340e969939ab1bfa2ffc7cf648 Author: mmenke <mmenke@chromium.org> Date: Thu Oct 20 18:40:32 2016 When parsing cookie expiration times, saturate out of range dates rather than reject them. Roughly this means cookie expiration times prior to 1970 on (non-OSX) POSIX, or after 2038 on 32-bit (non-OSX) POSIX, will now be interpreted as either very small or very large base::Time values. This is a reland of https://codereview.chromium.org/2424443002/, which ran into issues due to base::Time::FromTimeT() returning Time::Max() when passed the maximum time_t value. As a workaround, ParseCookieExpirationTime now returns the min/max base::Time() values for times outside the supported range, instad of the min/max base::Time() values supported by Time::FromUTCExploded() on the current platform. BUG= 649416 Review-Url: https://chromiumcodereview.appspot.com/2438513003 Cr-Commit-Position: refs/heads/master@{#426539} [modify] https://crrev.com/b6e737731c0b73340e969939ab1bfa2ffc7cf648/extensions/browser/api/web_request/web_request_api_helpers.cc [modify] https://crrev.com/b6e737731c0b73340e969939ab1bfa2ffc7cf648/net/cookies/canonical_cookie.cc [modify] https://crrev.com/b6e737731c0b73340e969939ab1bfa2ffc7cf648/net/cookies/canonical_cookie.h [modify] https://crrev.com/b6e737731c0b73340e969939ab1bfa2ffc7cf648/net/cookies/cookie_monster_store_test.cc [modify] https://crrev.com/b6e737731c0b73340e969939ab1bfa2ffc7cf648/net/cookies/cookie_util.cc [modify] https://crrev.com/b6e737731c0b73340e969939ab1bfa2ffc7cf648/net/cookies/cookie_util.h [modify] https://crrev.com/b6e737731c0b73340e969939ab1bfa2ffc7cf648/net/cookies/cookie_util_unittest.cc |
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by eroman@chromium.org
, Sep 22 2016