240963 - short cookie lifetime overridden by "limit to current session" setting

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect)

Description

[Carsten Gaebler]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.6) Gecko/20040124
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.6) Gecko/20040124

If the "limit maximum lifetime of cookies to" preference is set to "current
session", even cookies that expire before the end of the session survive until
the session is closed.

IMHO this is a bug because the name of this setting implies that no cookie will
live longer than the session, not that all cookies survive until the end of the
session.

Reproducible: Always
Steps to Reproduce:
1. Set "limit maximum lifetime of cookies to current session".
2. Install a cookie that expires afer N minutes.
3. Wait N + x minutes (x > 0).

Actual Results:  
The cookie is still there.

Expected Results:  
The cookie should have been removed.

Comment 1

[OstGote!]

Are you sure the expired cookies are "active" and accessible? Perhaps cookies
are only deleted at the session end? 

Creating a Cookie Log
http://www.mozilla.org/projects/netlib/cookies/cookie-log.html

Comment 2

[Carsten Gaebler]

Yes, the expired cookies are still active (i.e. sent to the server) after the
expiration date.

Comment 3

[Michiel van Leeuwen (email: mvl+moz@)]

from nsCookiePermission.cpp:

432       // we're not prompting, so we must be limiting the lifetime somehow
433       // if it's a session cookie, we do nothing
434       if (!*aIsSession && delta > nsInt64(0)) {

Imagine that check is removed. That would make a session cookie have a expiry
date. Would that expire it when either the session is closed or the expiry time
comes?

Comment 4

[Mike Connor [:mconnor]]

what type of logic could we use to determine whether we should limit or not?  
other than an arbitrary length of time, I'm not seeing anything that we could 
really use for this.

mvl: if its a session cookie already, it'll stay a session cookie, that check 
just skips any of the lifetime limiting/calculation code if its already a 
session cookie.  what's delta for a session cookie?

Comment 5

[[e-mail]]

this is a valid bug, confirming; although it may end up being wontfix.

there is no delta for a session cookie; they have no expiry time defined. fixing
this bug would probably involve keeping expirytime and isSession attributes
around for every cookie, and then applying the more restrictive of the two. for
instance, a cookie is set that expires in 5 seconds; the user pref to limit
lifetime to session has been set, so we "downgrade" it to a session cookie; when
we next go to send that cookie back to the server in the same session, we note
that it's already expired and dump it.

this will introduce a tad more complexity... what say you, cookie ui peer? :)

Comment 6

[Mike Connor [:mconnor]]

this brings up the issue of "what do we show in cookie viewer" if we're
maintaining both in memory.  I don't think there's a good answer for that.  One
or the other would be just plain wrong.

I guess the question is "how harmful is this really?  and in what situations?"

Comment 7

[Mike Connor [:mconnor]]

oh, and same logic necessarily applies to prompts and site perms too, so now
we're looking for some logic to deduce when "accept for session" really means
"leave the damn thing alone"

Comment 8

[Carsten Gaebler]

Could you guys explain what you mean by "session cookie"? Seems I'm missing the
point here. :-)

Comment 9

[[e-mail]]

a session cookie is one that persists until the browser is closed, and is a
valid type of cookie (contrast this to a cookie with a given expiry time). see
rfc 2109 to learn more about the various properties of cookies.

Comment 10

[Mike Connor [:mconnor]]

session cookies are generally set by servers to expire at the end of the current
session.  Any of the limit to session options apply this type of expiry to each
cookie, meaning you get the bug seen here.

Comment 11

[Carsten Gaebler]

So why does Mozilla "downgrade" a cookie with expiration date to a session
cookie? This is counter-intuitive from the end user's point of view (as you can
see :-). I'd expect the "limit to session" option to pay attention to the
expiration dates and remove all cookies when the browser is closed.

Would this really be difficult to implement? I'm not familiar with the source
code but if this option is disabled Mozilla has to keep track of expiration
dates for every cookie anyway, doesn't it?

Comment 12

[Michiel van Leeuwen (email: mvl+moz@)]

I think checking for expiry time even if the cookie is a session cookie won't be
a problem. It is just removing a few checks, like: (from nsCookieService)

575         // check if the cookie has expired
576       if (!cookie->IsSession() && cookie->Expiry() <= currentTime) {
577         continue;
578       }

Remove the IsSession() check.
The UI in the cookie mgr is a problem though, as there are two reasons for the
cookie to expire, not just one (a date or end of session)

Comment 13

[[e-mail]]

it's not quite that simple... we'd have to init the expiry time for true session
cookies to LL_MAXINT first.

Comment 14

[benc]

We kind of assume that a user selecting this has some idea of what they are
doing. In theory ANY cookie lifetime that changes could eventually outlast the
original expiration period. Sessions are theoretically unlimited. Servers should
know how to revoke stale cookies.

Comment 15

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: also check expiry for session cookies

Something like this.
The cookie manager magicly still works, due to the difference between
nsICookie::GetExpires and nsICookie2::GetExpiry. I don't know why we have two,
but it comes in handy here :) It doesn't get confused by the large expiry time
by default. GetExpires checks for IsSession first.
For a downgraded cookie, it says it will expire at the end of the session. Is
that a problem? I don't know what a user will expect. If it doesn't say that it
is a session cookie, they will think it didn't work. But it might go away
before the session is closed. mconnor?

Comment 16

[[e-mail]]

Comment on attachment 146944 [details] [diff] [review]
also check expiry for session cookies

>Index: netwerk/cookie/src/nsCookieService.cpp
>===================================================================
>   nsCookieAttributes cookieAttributes;
>+  cookieAttributes.expiryTime = LL_MAXINT;

please put a comment here explaining why the init... maybe:
// init expiryTime such that session cookies won't prematurely expire

and of course, if you get your way, this might end up being currentTime + 12
hours instead of LL_MAXINT :)

also, you missed a case at
http://lxr.mozilla.org/seamonkey/source/netwerk/cookie/src/nsCookieService.cpp#
1971, please remove the IsSession check there... it's unnecessary now.

looks good, r=me

Comment 17

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 146944 [details] [diff] [review]
also check expiry for session cookies

Consider those review comments to be fixed.

Comment 18

[Michiel van Leeuwen (email: mvl+moz@)]

Checked in.

Comment 19

[jon rekai]

Attachment: 1.7 branch fix

Seeing this in 1.7 as "expired cookies with a value of "deleted" are saved then
the site is session-whitelisted."  (Branch fix attached.)

Comment 20

[[e-mail]]

you might want to request approval1.7.2 on the patch.

Comment 21

[Mike Kaply [:mkaply]]

Comment on attachment 152864 [details] [diff] [review]
1.7 branch fix

a=mkaply for 1.7.2

Comment 22

[jon rekai]

Would someone sufficiently CVS permed mind checking the branch fix in?

Comment 23

[[e-mail]]

fixed on 1.7 branch.

Comment 24

[[e-mail]]

Attachment: update idl comments

the fix for this bug didn't update the relevant documentation in the nsICookie2 and nsICookieManager2 interfaces. i've updated these comments on trunk per this patch.