368964 - cookie "expires" attribute should be absolute, not relative to server time

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect)

Description

[Marek]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; pl; rv:1.8.1.1) Gecko/20061204 Firefox/2.0.0.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; pl; rv:1.8.1.1) Gecko/20061204 Firefox/2.0.0.1

When sending php header eg.: header('Set-Cookie: a=b;expires=Tue, 30-Jan-2007 10:50:40 GMT;'); Opera and IE sets expiration date correctly but firefox does not accept proper 'expires':

0[34538]: ===== COOKIE ACCEPTED =====
0[34538]: request URL: http://somedomain.tld/a.php
0[34538]: cookie string: a=b;expires=Tue, 30-Jan-2007 10:50:40 GMT;
0[34538]: current time: Tue Jan 30 07:40:11 2007 GMT
0[34538]: ----------------
0[34538]: name: a
0[34538]: value: b
0[34538]: host: somedomain.tld
0[34538]: path: /
0[34538]: expires: Tue Jan 30 10:50:17 2007 GMT
0[34538]: is secure: false

When I add path=/, expires is set to 10:50:16 (always -1 second). When change date locally to +1 day expires is set to Jan 31 10:50:17 (with +1 day, in Opera and IE cookie doesn't exists).

I know it's usefull when client's time is different than server time but expire should mean exact date.
I could also use max-age but ie can't handle this. So it's a multi-browser problem.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[WADA:World Anti-bad-Duping Agency]

RFC 2109 (http://www.faqs.org/rfcs/rfc2109.html) defines Set-Cookie as follows.
> 4.2.2  Set-Cookie Syntax
>   The syntax for the Set-Cookie response header is
>   set-cookie      =       "Set-Cookie:" cookies
>   cookies         =       1#cookie
>   cookie          =       NAME "=" VALUE *(";" cookie-av)
>   NAME            =       attr
>   VALUE           =       value
>   cookie-av       =       "Comment" "=" value
>                   |       "Domain" "=" value
>                   |       "Max-Age" "=" value
>                   |       "Path" "=" value
>                   |       "Secure"
>                   |       "Version" "=" 1*DIGIT

And RFC 2965 (http://www.faqs.org/rfcs/rfc2965.html) which obsoletes RFC2109 defines Set-Cookie2: and Cookie: only. See also "9. HISTORICAL", "9.1 Compatibility with Existing Implementations" section. 

What will happen when setcookie() function is used instead of header() function?
http://www.php.net/manual/en/function.setcookie.php

Comment 2

[[e-mail]]

updating summary to more closely reflect bug.

Comment 5

[Boris Zbarsky [:bzbarsky]]

http://tools.ietf.org/html/draft-ietf-httpstate-cookie-23

says that this bug needs fixing, basically.  ;)

Comment 6

[Marcin]

(In reply to comment #5)
> http://tools.ietf.org/html/draft-ietf-httpstate-cookie-23
> 
> says that this bug needs fixing, basically.  ;)

I agree with Boris, this bug needs to be fixed :-)

Comment 7

[Jo Hermans]

I'm not reading that requirement in the draft, except that the client can delete cookies that expire in the past.

Obviously, the real error is that the server-clock is not set correctly (see example in bug 650221). And also that the designer of the original cookie-spec used an absolute timestamp instead of a relative one (more or less corrected later with the max-age attribute). Really, that's just asking for trouble.

Comment 8

[Marcin]

Hi guys, any decisions on that bug maybe?

cheers

Comment 9

[theimp]

I found this bug in a completely different way. If you use Max-Age greater than zero, it's also relative to the server time (as received in the HTTP Date header) and not the client time.

This creates the bizarre situation that a Max-Age of 0 (illegal to send per the new RFC6265, but unambiguous to process and legal under RFC2109) will expire after zero seconds (i.e. immediately), but a Max-Age of 1 (the lowest legal value) could take an arbitrary amount of time to expire!

I can see logic both for using server-relative-time or client-relative-time, but a value of 1 should take only one second longer than a value of zero!

Yes, yes, servers should set their clocks right; but clients - not servers - decide when and under what conditions to evict cookies, and so they should decide what "date and time" means just the same as they decide what "excess cookies" means or any other condition by which they will manage cookies.

Example #1 (Client local time: Wed, 27 Jul 2011 00:00:00 GMT)

Client:
GET / HTTP/1.0
Cookie: x=y

Server:
200 OK
Set-Cookie: x=y; Max-Age=0
Date: Wed, 27 Jul 2012 00:00:00 GMT

 Cookie expires immediately (as it should).

Example #2 (Client local time: Wed, 27 Jul 2011 00:00:00 GMT)

Client:
GET / HTTP/1.0
Cookie: x=y

Server:
200 OK
Set-Cookie: x=y; Max-Age=1
Date: Wed, 27 Jul 2012 00:00:00 GMT

 Cookie won't expire for a year plus one second!

Comment 10

[Shian-Yow Wu [:swu]]

Amy, since you are looking at cookie expires issues, you might be also interested to look at this.

Comment 11

[Amy Chung [:Amy]]

can't reproduce on 47.0b7.

Comment 12

[Amy Chung [:Amy]]

https://dxr.mozilla.org/mozilla-central/source/netwerk/cookie/nsCookieService.cpp#4034
This statement needs to modify to aCookieAttributes.expiryTime = aServerTime + delta;

Comment 13

[Amy Chung [:Amy]]

Attachment: Modified the GetExpiry() function.

Hi Jason,
I found the purpose of GetExpiry() that avoiding Client and Servers' timezone are different.
But if Client set to future time, and user set absolute time to the expiration of cookie, above situation cause the GetExpiry() function would be to set the expiration of cookie to Client time + (the absolute expiration - Server time). 
I thought the cookie of expiration that still needs to base on server time by user setting absolute expiration time.

Please help me to review this path, thanks!

Comment 14

[Amy Chung [:Amy]]

Hi Jason,
I have tried the behavior of this bug on Google Chrome and Safari, and the testing result as below:
1. I got same behavior on Google Chrome.
2. Safari would be saving the expiration of cookie as absolute time, but it didn't confirm the expiration of cookie earlier than client time that still saves this cookie to database.
Would I need to fix this bug?

Comment 15

[Jason Duell]

It seems pretty clear that if a cookie is set with an absolute GMT expiration date, then that's the time when it should expire (and local time should adjust to it).  So even if Chrome is also doing the wrong thing here, I think we should fix this. If I thought it would break the web I'd worry about compat here, but we've already got different browsers doing different things.

Comment 16

[Jason Duell]

Comment on attachment 8757263 [details] [diff] [review]
Modified the GetExpiry() function.

Ehsan: if you're busy, maybe jdm can take this instead.

Comment 17

[(no longer active)]

Comment on attachment 8757263 [details] [diff] [review]
Modified the GetExpiry() function.

Review of attachment 8757263 [details] [diff] [review]:
-----------------------------------------------------------------

Apologies for the delay.

In addition to the comments below, please also add tests.  Be careful to test both the case where the Expires field sets a date into the past and the future.

Thanks!

::: netwerk/cookie/nsCookieService.cpp
@@ +4032,5 @@
> +    // expiration.
> +    // Because if current time be set in the future, but the cookie expire time be set less than current
> +    // time and more than server time.
> +    // The cookie item have to be used to the expired cookie.
> +    base_time = aServerTime;

I think this is the right fix, but it's a roundabout way of fixing this, since |delta| subtracts aServerTime and then the line below the conditionals adds aServerTime back, essentially canceling itself out.

How about:

* Get rid of the delay variable and the generalized code that sets expiryTime below.
* In the branch above, set expiryTime to aCurrentTime + max-age.
* In this branch, set expiryTime to |expires/PR_USEC_PER_SEC|.

@@ -4029,5 @@
>      return true;
>    }
>  
> -  // if this addition overflows, expiryTime will be less than currentTime
> -  // and the cookie will be expired - that's okay.

Why are you removing this comment?  I think you need to keep it (and move it to the first branch.)

@@ +4044,2 @@
>  
>    return false;

With this change, returning false here would be wrong (a false return means the cookie is expired).  In the second branch, you would need to check the absolute time coming from the server and only return true if it's greater than or equal to the current time.

Comment 18

[Amy Chung [:Amy]]

Attachment: Modified GetExpiry() from suggestion of Eshan.

Hi Ehsan,
I already modified the GetExpiry() from your suggestion,
and this function return true by cookie is expired so I didn't modify the return value.
In addition to this, do I need to adjus other code from this modification?

Thanks!

Comment 19

[(no longer active)]

Comment on attachment 8773594 [details] [diff] [review]
Modified GetExpiry() from suggestion of Eshan.

Review of attachment 8773594 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

::: cookie
@@ +1,1 @@
> +browser/components/contextualidentity/test/browser/file_reflect_cookie_into_title.html

Err, not sure what this is...

::: netwerk/cookie/nsCookieService.cpp
@@ +4010,5 @@
>      if (numInts != 1) {
>        return true;
>      }
>  
>      delta = maxage;

You can declare and initialize delta in one go here.  No need to declare it above.

@@ +4011,5 @@
>        return true;
>      }
>  
>      delta = maxage;
> +    

Nit: trailing whitespace!

Comment 20

[(no longer active)]

(In reply to Amy Chung [:Amy] from comment #18)
> In addition to this, do I need to adjus other code from this modification?

No, this looks good, but it still needs tests.

Comment 21

[Amy Chung [:Amy]]

Attachment: add cookie tests.

Hi Ehsan,
I found one situation I didn't consider, and the situation is when client time set earlier than server time and expiry time, we need to compare server time and expiry time for letting browser decides to save the cookie or not. 
For this reason, I modified nsCookieService::GetExpiry().

For added test cases, I divided them into four categories as below:
C:client time, S: server time, E: expiry time
+-------+-----+-----+------------------+---------------------+
| C&S   | C&E | S&E | Save Cookie      | Save Cookie         |
|       |     |     | in modified code |  in original code   |
+-------+-----+-----+------------------+---------------------+
| C > S | C>E | S>E | False            | False               |
+       +-----+-----+------------------+---------------------+
|       | C>E | S<E | False            | True, E = C + (E-S) |
+-------+-----+-----+------------------+---------------------+
| C < S | C<E | S>E | False            | False               |
+       +-----+-----+------------------+---------------------+
|       | C<E | S<E | True, E = E      | True, E = C + (E-S) |
+-------+-----+-----+------------------+---------------------+

If you have any idea or suggestion, please feel free to let me know. 
Thanks!

Comment 22

[(no longer active)]

(In reply to Amy Chung [:Amy] from comment #21)
> Created attachment 8775695 [details] [diff] [review]
> add cookie tests.
> 
> Hi Ehsan,
> I found one situation I didn't consider, and the situation is when client
> time set earlier than server time and expiry time, we need to compare server
> time and expiry time for letting browser decides to save the cookie or not. 

Why is this correct?

See <https://tools.ietf.org/html/draft-ietf-httpstate-cookie-23>.  Specifically first section 5.2.1 which tells us we should use the expiry attribute if it parses as a date that's within a date range we can represent, and section 5.3 bullet point 12 where it says "A cookie is "expired" if the cookie has an expiry date in the past."

The server time is irrelevant in all of this.

Comment 23

[Amy Chung [:Amy]]

Attachment: add cookie tests.

Hi Ehsan,
I have roll-backed the nsCookieService.cpp and added test cases.
Please help me to review my code.

Thanks!

Comment 24

[Amy Chung [:Amy]]

Updated the test case and excepted result.

+-------+-----+-----+------------------+---------------------+
| C&S   | C&E | S&E | Save Cookie      | Save Cookie         |
|       |     |     | in modified code |  in original code   |
+-------+-----+-----+------------------+---------------------+
| C > S | C>E | S>E | False            | False               |
+       +-----+-----+------------------+---------------------+
|       | C>E | S<E | False            | True, E = C + (E-S) |
+-------+-----+-----+------------------+---------------------+
| C < S | C<E | S>E | True, E = E      | False               |
+       +-----+-----+------------------+---------------------+
|       | C<E | S<E | True, E = E      | True, E = C + (E-S) |
+-------+-----+-----+------------------+---------------------+

Comment 25

[Amy Chung [:Amy]]

Attachment: Rollback previous modification and add test cases.

Comment 26

[Amy Chung [:Amy]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=34abbe0404b4

Comment 27

[Amy Chung [:Amy]]

Comment on attachment 8777227 [details] [diff] [review]
Rollback previous modification and add test cases.

Review of attachment 8777227 [details] [diff] [review]:
-----------------------------------------------------------------

I have to modify for try server mistake.

Comment 28

[(no longer active)]

Comment on attachment 8777227 [details] [diff] [review]
Rollback previous modification and add test cases.

Review of attachment 8777227 [details] [diff] [review]:
-----------------------------------------------------------------

I reviewed this yesterday and forgot to submit it!

::: netwerk/cookie/nsCookieService.cpp
@@ +4014,5 @@
>      delta = maxage;
>  
> +    // if this addition overflows, expiryTime will be less than currentTime
> +    // and the cookie will be expired - that's okay.
> +    aCookieAttributes.expiryTime = aCurrentTime + delta;

Please delete the delta variable and use maxage here directly.  Also get rid of the assignment above.

::: netwerk/test/TestCookie.cpp
@@ +46,5 @@
> +        int64_t SetTime = CurrentTime + offset_time;
> +
> +        memset(timeString, 0, len);
> +        PRExplodedTime explodedTime;
> +        PR_ExplodeTime(SetTime , PR_GMTParameters, &explodedTime);

Nit: no space before comma.

@@ +50,5 @@
> +        PR_ExplodeTime(SetTime , PR_GMTParameters, &explodedTime);
> +        PR_FormatTimeUSEnglish(timeString_preset, len, "%c GMT", &explodedTime);
> +
> +        if ( cookieString != nullptr) {
> +            if ( strcmp(cookieString, "test=expiry; expires=") ) {

Nit: no space after ( and before ) here and elsewhere.

@@ +51,5 @@
> +        PR_FormatTimeUSEnglish(timeString_preset, len, "%c GMT", &explodedTime);
> +
> +        if ( cookieString != nullptr) {
> +            if ( strcmp(cookieString, "test=expiry; expires=") ) {
> +              memset(cookieString, 0, strlen(cookieString));

This isn't needed.

@@ +52,5 @@
> +
> +        if ( cookieString != nullptr) {
> +            if ( strcmp(cookieString, "test=expiry; expires=") ) {
> +              memset(cookieString, 0, strlen(cookieString));
> +              memcpy(cookieString, "test=expiry; expires=", strlen("test=expiry; expires=") + 1);

You can use strcpy instead.

Also, is there any reason why you don't strcpy directly without checking the existing contents of the buffer?

@@ +57,5 @@
> +            }
> +            memcpy(timeString, timeString_preset, len);
> +            strcat(cookieString, timeString);
> +        } else {
> +            memcpy(timeString, timeString_preset, len);

OK, so I don't really like any of these raw memory manipulations!

Please create an nsAutoCString in the caller and pass that here instead of the raw buffer, and use its helpers for setting the string, appending, etc.  We have those classes so that we don't have to write code like this.  :-)

Comment 29

[Amy Chung [:Amy]]

Attachment: Modified SetTime() function on TestCookie.cpp

Hi Ehsan,
I have modified the arguments on SetTime() function to nsAutoCString and removed all memset().

Please help me to review my patch, thanks!

Comment 30

[Amy Chung [:Amy]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=78d321f60b1a3d8046407e0b1c4609fa7b577a4b

Comment 31

[(no longer active)]

Comment on attachment 8781584 [details] [diff] [review]
Modified SetTime() function on TestCookie.cpp

Review of attachment 8781584 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the comments below addressed.  Thanks!

::: netwerk/test/TestCookie.cpp
@@ +34,5 @@
>  static char *sBuffer;
>  
> +//Set server time or expiry time
> +void
> +SetTime(PRTime offsetTime, nsAutoCString* timeString, nsAutoCString* cookieString)

Both of these should be nsACString&.  The "A" there means "abstract".  We typically do not pass strings around with their concrete type.  And we almost never pass them using pointers!

@@ +48,5 @@
> +
> +      if (cookieString != nullptr) {
> +        cookieString->Replace(0, strlen("test=expiry; expires=") + strlen(timeStringPreset) + 1, "test=expiry; expires=");
> +        timeString->Assign(timeStringPreset);
> +        cookieString->Append(timeString->get());

Nit: drop the call to get()

Comment 32

[Amy Chung [:Amy]]

Attachment: Modified GetExpiry() and added test cases.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7dd55746285d

Comment 33

[Amy Chung [:Amy]]

(In reply to :Ehsan Akhgari from comment #31)
> Comment on attachment 8781584 [details] [diff] [review]
> Modified SetTime() function on TestCookie.cpp
> 
> Review of attachment 8781584 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with the comments below addressed.  Thanks!
> 
> ::: netwerk/test/TestCookie.cpp
> @@ +34,5 @@
> >  static char *sBuffer;
> >  
> > +//Set server time or expiry time
> > +void
> > +SetTime(PRTime offsetTime, nsAutoCString* timeString, nsAutoCString* cookieString)
> 
> Both of these should be nsACString&.  The "A" there means "abstract".  We
> typically do not pass strings around with their concrete type.  And we
> almost never pass them using pointers!
> 
> @@ +48,5 @@
> > +
> > +      if (cookieString != nullptr) {
> > +        cookieString->Replace(0, strlen("test=expiry; expires=") + strlen(timeStringPreset) + 1, "test=expiry; expires=");
> > +        timeString->Assign(timeStringPreset);
> > +        cookieString->Append(timeString->get());
> 
> Nit: drop the call to get()

Thanks for your help, I have modified the arguments of SetTime() in TestCookie.cpp.

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3df16b2621d
Cookie expires attribute should be absolute, not relative to server time. r=ehsan

Comment 35

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d3df16b2621d

Comment 36

[Chris Mills [:cmills]]

This is a bug, not really a new feature that needs to be announced or documented. However, I did think it useful to add in a note saying that set expiry dates are relative to the client rather than the server:

https://developer.mozilla.org/en-US/docs/Web/HTTP/Cookies#Permanent_cookies
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie#Directives

Let me know if you think anything else is needed.