8530 - [RFE]Enhancement to reject cookies with excessive lifetime

Status: VERIFIED FIXED

(Core :: Networking: Cookies, enhancement, P3)

Description

[Greg Noel, Unix Guru]

I am totally frosted by sites that try to set cookies that won't expire for

thirty years or more.  In fact, the odds that a cookie's appliciability (or its

shape, for that matter) being valid for more than a few months is close to zero.

I consider it impolite and rude of a site to expect me to keep something around

that long---if I don't visit a site in a few months (and give it a chance to

refresh the cookie), I'm not very interested in that site.  I see no reason to

have my cookie cache cluttered up with sites that I visited only once and never

plan to visit again.



So I propose an enhancement that allows me to automatically reject a cookie with

a lifetime longer than, say, three months.  Obviously, this should be a settable

preference, but I think a default of 90 days or so is about right.



At a minimum, the existence of this preference would force sites to consider

their expiration policy and select something more reasonable, something that

reflects the actual useful lifetime of the cookie.



For a site that uses cookies in a responsible fashion, take a look at amazon.com

where the cookie retention is about a month.  They don't seem to have any problem

with tracking their users, even on a longer period.



I'm willing to put my own effort into this.  Although I know next to nothing

about the Mozilla code base, I'm an experienced programmer.  If you can tell me

where to find the code necessary to make this change, I'm willing to take a look

at it and suggest the changes.  My only network-connected platform at this time

is my wife's Macintosh, but I hope to have a MkLinux platform available in a few

weeks.

Comment 1

[Stephen P. Morse]

That's an excellent suggestion and would complement nicely the new cookie
management features that we have added in 5.0 (ability to remember a decision
about accepting or rejecting cookies on a site-by-site basis).

The places where the changes would go are in a module called nsCookie.cpp.  It
currently resides in network/protocol/http but is in the process of being moved
to extensions/cookie.  Also you'll have to look in the preference modules to see
how to add the new preference to specifies how long a cookie retention you are
willing to accept.

I strongly encourage you to get familiar with these modules and to make the
changes and test them out on your mac.  Also you might consider reassigning the
bug to yourself.  I'll be glad to assist and give you more specific pointers as
to where the changes should be made.

Comment 2

[Matthew Tuck [:CodeMachine]]

Site by site cookie settings would be great.  You might want to look at bug
#7380.

Comment 3

[Stephen P. Morse]

Attachment: attachment received from Greg Noel

Comment 4

[Stephen P. Morse]

Message received from Greg Noel (with attachment):

It's just a short time until I leave and I'm trying to get this to you
before I go.  I _think_ it's OK, but all I've been able to do is compile
it, since the problems with running the lizard haven't been sorted out yet.

(I'll be gone for four weeks; if you watch the Charger game on August 7th,
you may see me.  I won't have access to e-mail until I'm back.)

Anyway, the attached file is extensions/cookie/nsCookie.cpp with changes to
fix the subject problem.  (The file passed through a Macintosh, so the
newlines may have been changed to returns; if so, you'll have to fix that.)
Here's the story:

When I looked at the code, I found that there were at least four distinct
coding styles in use, which made the code very hard to read.  Moreover, the
indentation was irregular, as some places used spaces and others used tabs
(with tab stops every four, gack).

So, simply to make it readable, indent(1) and I had a long discussion with
it.  As a result, it's been reset in pure K&R style, with tab stops every
eight, like God and Ritchie intended.  That will probably play hob with any
diffs you do, but the only substantive change I made during that processing
was to #ifdef out code instead of commenting it out (so that indent(1) will
process the code instead of skipping it).

Then, using the same patterns already used in the code, I added two new
preference values, one to turn the new mode on and off and one for the
period in days.  I think I got all of that right; if not, I think the worst
case is that the new stuff will quietly ignored.

I put in new logic to ignore any cookies that exceed the threshold if the
new mode is set.  I can't test any of this because I still can't build a
functional lizard on this machine, but I've reviewed the code until I'm
cross-eyed, and I'm pretty confident in it.

I cleaned up the code to remove warnings and fix a couple of minor bugs:
one would affect the sort order (the display order would have alphabetic
runs but not be fully alphabetized) and another where the expiration time
wasn't retained correctly.

I then pulled the tip to see any changes that had been made; all the
changes up through 1.9 have been applied.

I ran out of time before I could make the code per-domain; sorry.  I'll
have to look at that when I'm back from holiday.

While I was working on the code, it occured to me that it could just as
well trim the expiration date down rather than discard the cookie.  That
would have almost the same effect.  I'll look at that when I get back, too.

I didn't do anything to make it configurable via the cookie preferences
panel.  Yet another thing that will have to wait until I return.

Wups, got to leave for the airport; no time to finish this note.

Hope this helps,
-- Greg Noel, retired UNIX guru

Comment 5

[Matthew Tuck [:CodeMachine]]

Steve how far are the site-by-site cookies coming along?  Are there preferences
there yet?  I haven't seen it in the UI, but is there a way to get it from
hacking prefs.js?

Comment 6

[Stephen P. Morse]

That's not the topic of this bug report, so I'll respond to you directly.

Comment 7

[Matthew Tuck [:CodeMachine]]

Is it necessary to reject the cookies?  Is it possible to keep them but limit
their lifetime?

Comment 8

[Greg Noel, Unix Guru]

Yes, it's possible, but my feeling is that if they are simply specifying an

expiration period of ten, twenty, or even thirty years, they haven't thought

through the implications of a short expiry time (refreshing the cookie when it's

nearly expired, things like that).  It's strategically better to simply reject

the cookies and thereby motivate the offending site to get its act together.

Comment 9

[Stephen P. Morse]

This enhancement is not going to get done unless somebody from the web
volunteers to do it.

Comment 10

[Greg Noel, Unix Guru]

I don't understand the statement that this needs web help.  I submitted the
changes over five months ago.  It was ignored.

About two months ago, I noticed that the files had been changed extensively
without integrating the changes, so I wrote and offered to generate the patches
again.  I received no reply, not even a "not now, we're busy."

I'm still willing to generate the patches again, if someone is willing to review
them and check them in.  But if I'm going to continue to be ignored, it'd just
be a waste of my time.

So it's up to you...

Comment 11

[Stephen P. Morse]

Greg,

I must be missing some mail here.  The only changes I ever received from you
were on 8-3 and they were incomplete.  Specifically, you stated:

"I can't test any of this because I still can't build a functional lizard on
this machine,"

"I ran out of time before I could make the code per-domain;"

"I didn't do anything to make it configurable via the cookie preferences
panel.  Yet another thing that will have to wait until I return."

With all of the above missing (especially the lack of any testing), there's no
way I could approve this for checkin.  I was waiting for you to someday complete
it.

Furthermore, in your comment today you said

  About two months ago, I noticed that the files had been changed extensively
  without integrating the changes, so I wrote and offered to generate the
  patches again.  I received no reply, not even a "not now, we're busy."

If you did write, I never received it.  I would have replied immediately if I
had.

It's important to realize that there are no resources with netscape to do any of
this.  We have our hands full just trying to get all the required features in.
So if you want this feature in, it will be up to you to develop it competely
(including changes to the preference panel), and test it thoroughly on unix,
mac, and win32.

The only help I can offer is to do a code review and to check it in if it is
acceptable.  But in order to do a code review I will need a clean set of diffs.
Don't do any extensive changes to the indentation if you expect your diffs to be
readible.  If you want to do such changes, do that in a separate codedrop with
no textual changes other than whitespace.  That will be very easy to verify as
being textually equivalent.

Comment 12

[Stephen P. Morse]

Just to clarify my last statement.  When I said:

   If you want to do such changes, do that in a separate codedrop with
   no textual changes other than whitespace.

I was referring to the indentation changes.

Comment 13

[Greg Noel, Unix Guru]

OK, I'll believe you if you say you never got the mail---I definitely sent it. 
But I'd rather get the bug fixed than point fingers.  I'll attach a patch that
fixes all the immediate concerns of this bug.  I even used the same appalling
style as was currently there, even though it drove me cross-eyed trying to read
it, to avoid issues with formatting changes.

It has been exhaustively tested on Linux.  Virtually all of the code was cloned
from other fragements and the remainder is very vanilla, so there should be no
problem with it working on other platforms.  (The delay in getting the patches
to you has been because my browser has not been able to run the browser buster
for a long enough period to constitute "exhaustive testing.")

The comments about exactly what was changed are in the attachment.

I didn't include [e-mail]'s request to make this work on a site-by-site
basis.  I started to code that and had to pull most of it back out; adding all
of it would have delayed these fixes too much.  I'll continue to work on it,
either in the context of bug#7380 or another bug.

The code can be tested by manually modifying prefs.js.  I've started working on
the cookie preferences panel, but I wanted to get this in so that people can
start testing it if they want.

Comment 14

[Chris Waterson]

Attachment: cleaner patch, courtesy Greg Noel.

Comment 15

[Chris Waterson]

morse: attached is the patch that Greg sent to me last night. Apparently
bugzilla is not working properly for him. It compiles on Mac.

Comment 16

[leger]

Updating Assignee from "nobody" to morse.

Comment 17

[Stephen P. Morse]

I have the fix in-hand from Greg Noel.  I was hoping to be able to test it and 
check it in last week but the crunch for beta has suddenly become intense.  So I 
promise to work on this just as soon as beta (M14) is finished.  Marking tfv as 
M15

Comment 18

[Chris Waterson]

Attachment: even cleaner patch

Comment 19

[Chris Waterson]

damn. greg sent me new diffs and I just remembered to put the diffs into the 
tree. morse: could you review and land?

Comment 20

[Stephen P. Morse]

Chris, I planned on reviewing all this and being prepared to put it into the 
tree just as soon as the beta1 crunch is over.  Are his latest diffs in the 
patch you just included?

Comment 21

[Stephen P. Morse]

Have requested revised diffs from Greg Noel that fix the errors that I detected 
when code reviewing.  Have never received those diffs.  Moving this out to M20 
unless the diffs come in sooner.

Comment 22

[Stephen P. Morse]

.

Comment 23

[Stephen P. Morse]

Reopened per management's every changing mind.

Comment 24

[msuencks]

here is the latest version of the
patch which also includes a special
treatment for a cookie's lifetime
limit of "0" days and which sets
every cookie's expire time to "session"
in that case.
(as discussed in bug #32284)

built and tested with linux. 
the diff works against the source
tarball fetched on Apr 2 

Comment 25

[msuencks]

Attachment: latest diff

Comment 26

[Stephen P. Morse]

Here are some comments on the latest diff.

1. In COOKIE_RegisterCookiePrefCallbacks you have changed the following

  if (NS_FAILED(prefs->GetIntPref(cookie_behaviorPref, &n))) {
    cookie_SetBehaviorPref(COOKIE_Accept);
  } else {
    cookie_SetBehaviorPref((COOKIE_BehaviorEnum)n);
  }

to be

  if (NS_FAILED(prefs->GetIntPref(cookie_behaviorPref, &n))) {
    n = COOKIE_Accept;
  }

But you never use n after that.  I believe that the correct change be

  if (NS_FAILED(prefs->GetIntPref(cookie_behaviorPref, &n))) {
    n = COOKIE_Accept;
  }
  cookie_SetBehaviorPref((COOKIE_BehaviorEnum)n);

2. Since you are making the above optimization for cookie_behaviorPref, perhaps 
you should make the same optimization for image_behaviorPref a few lines later.

Otherwise everything looks fine.  If you agree with my corrections, then please 
test the change out to make sure that it is doing what it is supposed to.  Then 
attach a revised patch and I'll check the change in.

Thanks for taking on this project.

Comment 27

[Stephen P. Morse]

I decided to check this in, with the additional fix to prevent the 
regression that I caught while code reviewing.  But I'm trusting that  
[e-mail] will test this to make sure that the regression does not 
occur.

Comment 28

[Stephen P. Morse]

*** Bug 32284 has been marked as a duplicate of this bug. ***

Comment 29

[Paul MacQuiddy]

over to tever the cookies guy

Comment 30

[msuencks]

ok i tested it although it seemed rather obvious
to me that the regression would not occur with
the fix ( i think it would have resulted
in not intializing the cookie behaviour from the prefs.js file)


upon testing I wondered about mozilla's debug output
saying
"*** set bool pref network.cookie.warnAboutCookies to undefined"

... which appeared wether warnAboutCookies was false or true,
it didn't matter. however in the prefs.js file the correct value
appeared. so it seems a bug in debug ..?

anyway .. what next to get the stuff into the preferences dialog ?

get some people to vote on this ? :-)

Comment 31

[Stephen P. Morse]

UI is in German's hands.  Get him to approve it.

Comment 32

[Stephen P. Morse]

*** Bug 53354 has been marked as a duplicate of this bug. ***

Comment 33

[Tobias Burnus]

I don't see a UI (and morse says in dup bug 53354 there is none).
I reopened my bug 53354 and changed it to request a UI.

Comment 34

[Tom Everingham]

verified