53352 - cookie manager not handling domain cookies correctly

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect, P2)

Description

[Jeremy M. Dolan]

I removed cookies from cookie manager and had the 'dont allow again' box
checked, clicked ok, and re-enter to verify the hosts were added.

went back to cnn, and the cookies were added again, ignoring my 'site cannot set
cookies' rule.

btw, i love this feature, host-specific cookie/image managment, cant wait till
it works... ;)

Comment 1

[Stephen P. Morse]

You left out an initial step in your description so let me try to clarify it:

1. Keep default cookie prefs (accept all cookies, no warning)
2. Visit www.cnn.com
3. Go to cookie manager and see several cookies set from cnn.com
4. Check the box saying "don't accept removed cookies later" and then remove all 
cookies
5. Reload the site
6. The cookies have been accepted again

I think the problem here has to do with the fact that the cookies are domain 
cookies for the .cnn.com domain rather than just for the www.cnn.com server.  
Perhaps the code is not handling domain cookies correctly.

Comment 2

[Tom Everingham]

that seems to be the case, changing summary from "'site cannot set cookies' 
doesn't work" to "cookie manager not handling domain cookies correctly"

Comment 3

[Marcia Knous [:marcia]]

Netscape Nav triage team: this is a Netscape beta stopper.

Comment 4

[Stephen P. Morse]

Yes, I was right, the problem is related to the fact that we are dealing with 
domain cookies.  Here's what's happening:

1. When the user rejects a cookie and says "remember this decision", an entry is 
made to the list of sites that can/cannot set cookies.  The leading periods from 
the hostname, if any, are stripped off before it is placed in the list.

2. Furthermore, when the cookie code is presented with a request for setting a 
new cookie, it strips off any leading period from the requesting hostname before 
looking for a match in the "can/cannot" list.  This is consistent with the fact 
that leading periods were stripped off when entries were made in that list, so 
everything works fine.

3. However there is another time at which entries are made in the "can/cannot" 
list.  Namely, when a return is made from the cookie-manager dialog and the "do 
not allow again" checkbox is checked.  But in this case, leading periods are not 
stripped off before entering the hostname in the list.  So if the hostname 
contains a leading period (as would be the case for domain cookies), we will 
never get a match in step 2 and therefore cookies from this hostname will again 
be accepted.

So the fix is easy.  Simply strip off the leading periods in step 3 (just like 
it was done in step 1) before making entries in the "can/cannot" list.  
Attaching patch that does just that.

Comment 5

[Stephen P. Morse]

Attachment: patch to strip off leading periods

Comment 6

[Paul Chen]

r=pchen. Would be nice to move the '.' removing code into a function since we're
doing it now in more than once place. I'll let it slide this time, but if we
need to do this code one more time somewhere else, I'd say it's time for a
function to strip leading '.', though I would think we have that code somewhere
else in this source base.

Comment 7

[Brendan Eich [:brendan]]

-          StrAllocCopy(hostname, cookie->host);

+          char * hostnameAfterDot = cookie->host;


If cookie->host can be null, what should happen here?

+          while (hostnameAfterDot && (*hostnameAfterDot == '.')) {


Since the loop can't ever cause a non-null hostnameAfterDot to become null, the
loop control need not test that condition -- hoist it into an enclosing if
statement.

+            hostnameAfterDot++;
+          }
+          StrAllocCopy(hostname, hostnameAfterDot);

Incidentally, what happens if malloc fails under StrAllocCopy?  Does all the
code that depends on hostname cope with null, silently?

/be

Comment 8

[Stephen P. Morse]

Attachment: Corrections to address brendan's comments

Comment 9

[Brendan Eich [:brendan]]

-        if (PL_strlen(block) && block[0]=='t') {

+        if (PL_strlen(block) && block[0]=='t' && cookie->host) {


If block must be NUL-terminated and PL_strlen's return value is used only to
test whether block is not the empty string, why spend cycles in PL_strlen?

        if (block[0] != '\0' && block[0] == 't' && cookie->host) {

Otherwise looks good -- fix that, and sr=[e-mail].

/be

Comment 10

[Stephen P. Morse]

Checked in with brendan's latest suggestion.  Although I must admit that it 
seems strange to test for non-zero and then test the same thing for 't'.  If you 
are dropping the strlen because you are sure it is null terminated, then it 
would make more sense to simply write

   if (block[0] == 't' && cookie->host) {

Comment 11

[Brendan Eich [:brendan]]

Good point -- I was not paying enough attention (although that could be said of
others before me when it comes to this particular if condition ;-).  You don't
need to check in my mistake if you have a better idea!  Feel free to make a
followup fix checkin to just that line.

/be

Comment 12

[Stephen P. Morse]

Line got fixed as part of the checkin for bug 60997.