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)
Core
Networking: Cookies
Tracking
()
VERIFIED
FIXED
People
(Reporter: GregNoel, Assigned: morse)
References
Details
Attachments
(4 files)
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•25 years ago
|
||
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.
Assignee | ||
Updated•25 years ago
|
Status: NEW → ASSIGNED
Summary: Enhancement to reject cookies with excessive lifetime → [RFE]Enhancement to reject cookies with excessive lifetime
Comment 2•25 years ago
|
||
Site by site cookie settings would be great. You might want to look at bug
#7380.
Assignee | ||
Updated•25 years ago
|
Target Milestone: M10
Assignee | ||
Comment 3•25 years ago
|
||
Assignee | ||
Comment 4•25 years ago
|
||
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
Assignee | ||
Updated•25 years ago
|
Target Milestone: M10 → M14
Comment 5•25 years ago
|
||
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?
Assignee | ||
Comment 6•25 years ago
|
||
That's not the topic of this bug report, so I'll respond to you directly.
Assignee | ||
Updated•25 years ago
|
Target Milestone: M14 → M20
Comment 7•25 years ago
|
||
Is it necessary to reject the cookies? Is it possible to keep them but limit
their lifetime?
Reporter | ||
Comment 8•25 years ago
|
||
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 | ||
Updated•25 years ago
|
Assignee: morse → nobody
Status: ASSIGNED → NEW
Assignee | ||
Comment 9•25 years ago
|
||
This enhancement is not going to get done unless somebody from the web
volunteers to do it.
Reporter | ||
Comment 10•25 years ago
|
||
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...
Assignee | ||
Comment 11•25 years ago
|
||
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.
Assignee | ||
Comment 12•25 years ago
|
||
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.
Reporter | ||
Comment 13•25 years ago
|
||
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.
Comment 14•25 years ago
|
||
Comment 15•25 years ago
|
||
morse: attached is the patch that Greg sent to me last night. Apparently
bugzilla is not working properly for him. It compiles on Mac.
Assignee | ||
Comment 17•25 years ago
|
||
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
Assignee | ||
Updated•25 years ago
|
Status: NEW → ASSIGNED
Comment 18•25 years ago
|
||
Comment 19•25 years ago
|
||
damn. greg sent me new diffs and I just remembered to put the diffs into the
tree. morse: could you review and land?
Assignee | ||
Comment 20•25 years ago
|
||
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?
Assignee | ||
Comment 21•25 years ago
|
||
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
Assignee | ||
Comment 22•25 years ago
|
||
.
Status: ASSIGNED → RESOLVED
Closed: 25 years ago
Resolution: --- → LATER
Assignee | ||
Comment 23•25 years ago
|
||
Reopened per management's every changing mind.
Status: RESOLVED → REOPENED
Resolution: LATER → ---
Assignee | ||
Updated•25 years ago
|
Status: REOPENED → ASSIGNED
Comment 24•25 years ago
|
||
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
Comment 25•25 years ago
|
||
Assignee | ||
Comment 26•25 years ago
|
||
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.
Assignee | ||
Comment 27•25 years ago
|
||
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 ago → 25 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 28•25 years ago
|
||
*** Bug 32284 has been marked as a duplicate of this bug. ***
Comment 30•25 years ago
|
||
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 ? :-)
Assignee | ||
Comment 31•25 years ago
|
||
UI is in German's hands. Get him to approve it.
Assignee | ||
Comment 32•24 years ago
|
||
*** Bug 53354 has been marked as a duplicate of this bug. ***
Comment 33•24 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•