Closed Bug 8530 Opened 25 years ago Closed 25 years ago

[RFE]Enhancement to reject cookies with excessive lifetime

Categories

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

enhancement

Tracking

()

VERIFIED FIXED

People

(Reporter: GregNoel, Assigned: morse)

References

Details

Attachments

(4 files)

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.
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.
Status: NEW → ASSIGNED
Summary: Enhancement to reject cookies with excessive lifetime → [RFE]Enhancement to reject cookies with excessive lifetime
Site by site cookie settings would be great. You might want to look at bug #7380.
Target Milestone: M10
Attached file attachment received from Greg Noel (deleted) —
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
Target Milestone: M10 → M14
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?
That's not the topic of this bug report, so I'll respond to you directly.
Target Milestone: M14 → M20
Is it necessary to reject the cookies? Is it possible to keep them but limit their lifetime?
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.
Assignee: morse → nobody
Status: ASSIGNED → NEW
This enhancement is not going to get done unless somebody from the web volunteers to do it.
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...
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.
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.
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 matty@box.net.au'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.
morse: attached is the patch that Greg sent to me last night. Apparently bugzilla is not working properly for him. It compiles on Mac.
Updating Assignee from "nobody" to morse.
Assignee: nobody → 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
Target Milestone: M20 → M15
Status: NEW → ASSIGNED
Attached patch even cleaner patch (deleted) — Splinter Review
damn. greg sent me new diffs and I just remembered to put the diffs into the tree. morse: could you review and land?
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?
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.
Target Milestone: M15 → M20
.
Status: ASSIGNED → RESOLVED
Closed: 25 years ago
Resolution: --- → LATER
Reopened per management's every changing mind.
Status: RESOLVED → REOPENED
Resolution: LATER → ---
Status: REOPENED → ASSIGNED
Keywords: patch
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
Blocks: 32284
Attached patch latest diff (deleted) — Splinter Review
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.
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 msuencks@marcant.de will test this to make sure that the regression does not occur.
Status: ASSIGNED → RESOLVED
Closed: 25 years ago25 years ago
Resolution: --- → FIXED
*** Bug 32284 has been marked as a duplicate of this bug. ***
over to tever the cookies guy
QA Contact: paulmac → tever
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 ? :-)
UI is in German's hands. Get him to approve it.
*** Bug 53354 has been marked as a duplicate of this bug. ***
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.
verified
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: