115800 - Short-lived cookies get "upgraded" to session cookies

Status: VERIFIED FIXED

(Core :: Networking: Cookies, defect)

Description

[Chris Underhill]

Mozilla 2001121308-trunk on Linux: Red Hat 7.1+updates

When using P3P or the preferences to downgrade cookies sent by the site to
session cookies, short-lived cookies no longer expire. This stops sites which
use this mechanism to force idle users to relogin from doing so.

See also comments 11 and 12 on bugid 114709.

Comment 1

[Stephen P. Morse]

OK, the way to fix this is to yank out all the current tests for expiring the 
cookie if status is downgrade.  Instead add a test at the time that the cookie 
is being written to the file, and if the cookie has a status of downgrade, don't 
write it.

Attaching a composite patch pertaining to bug 121311, bug 121685, and this one 
(because they all modify the same file).  The portions of the patch that are 
specific to this bug are:

1. removing the test for nsICookie::STATUS_DOWNGRADED before setting expires
   to cookie_ParseDate(date)

2. removing the test for nsICookie::STATUS_DOWNGRADED which results in setting
   gmtCookieExpires to 0

3. adding the test for nsICookie::STATUS_DOWNGRADED before writing the cookie
   to the file.

Comment 2

[Stephen P. Morse]

Attachment: composite patch for this bug, bug 121311, and bug 121685

Comment 3

[Stephen P. Morse]

alecf, sgehani, please review

Comment 4

[Alec Flett]

Comment on attachment 68499 [details] [diff] [review]
composite patch for this bug, bug 121311, and bug 121685

there's got to be a better way to get the max int..did you look in the NSPR
header files?

Comment 5

[Samir Gehani]

Comment on attachment 68499 [details] [diff] [review]
composite patch for this bug, bug 121311, and bug 121685

r=sgehani for the non-121311 (part Alec has a concern with) and non-121685
portions of this patch.

Comment 6

[Stephen P. Morse]

In prlong.h there is defined LL_MAXINT.  But that is defined as an int64 whereas 
I need it for a long.  When I tried to use it I got the warning message:

  warning C4244: '=' : conversion from '__int64' to 'long',
                       possible loss of data

The value being returned by LL_MAXINT is 0x7fffffff, 0xffffffff.  The value that 
I am generating is a leading 0 followed by all 1's, independent of the 
number of bits.

Is there something else that you know about that I should use?

Comment 7

[Alec Flett]

Comment on attachment 68499 [details] [diff] [review]
composite patch for this bug, bug 121311, and bug 121685

my concern is that you're relying on the bit format of time_t, rather than
treating it as an abstract type. What if time_t is signed? or unsigned? or what
if it IS actually 64-bit?

I think this reveals a fundamental flaw in the use of cookie_ParseDate()

you should be using the guaranteed-to-be-the-same-on-all-platforms PRTime

I just read the comments in get_current_time() and I see you've basically
painted yourself into a corner by not using PRTime "since the value is 64bits
and too hard to manipulate" (quoted fromt he comments at the top of
get_current_time)



I'm not sure what to say here, except that you're relying on assumptions about
time_t that simply won't hold true on all platforms.

Comment 8

[Stephen P. Morse]

Attachment: Separate patch, no longer a composite.

Comment 9

[Stephen P. Morse]

Alec, samir, please review.  The patch is no longer a composite patch but 
applies to this bug report only.

Comment 10

[Samir Gehani]

Comment on attachment 68966 [details] [diff] [review]
Separate patch, no longer a composite.

r=sgehani

Comment 11

[Alec Flett]

Comment on attachment 68966 [details] [diff] [review]
Separate patch, no longer a composite.

sr=alecf

Comment 12

[Stephen P. Morse]

This was checked in yesterday.  Marking fixed.

Comment 13

[Tom Everingham]

verified:  03/13/02 builds