121419 - If multiple cookies exist, the least significant is assigned.

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect, P3)

Description

[Benjamin Reed]

Our group has 2 separate bugzilla installations, one public, at http://www.opennms.org/bugzilla/ and the other on our intranet at http://www.internal.opennms.org/bugzilla/.

When a user logs into the opennms.org public bugzilla, the "Bugzilla_logincookie" cookie is set.  When a user logs into the internal.opennms.org private bugzilla, it is set again, with a different cookie ID.

From that point on, logging into the internal.opennms.org bugzilla works, but only once.  What happens is the browser sends something similar to the following:

  Cookie: Bugzilla_logincookie=603; Bugzilla_login=[e-mail]; \
    Bugzilla_logincookie=246;

The first logincookie is from *.internal.opennms.org (most significant), the second from *.opennms.org.

The simple fix is to only accept the first cookie, since it should always be the most correct anyways.

Comment 1

[Benjamin Reed]

Attachment: Patch to CGI.pl that makes $::COOKIE{key} be set only if it doesn't exist

should be applied with -p0 in the root of the bugzilla tree

Comment 2

[Bradley Baetz (:bbaetz)]

Why is the cookie not being set for only that particular host?

Comment 3

[Dave Miller [:justdave]]

This looks like your browser is broken.

Bugzilla uses a fully-qualified domain name when setting cookies.  The cookie
shouldn't go to any server but the one it was set for, OR a subdomain of that
fully qualified domain (which the examples you gave aren't).

For example, if your external URL is http://www.opennms.org/ in order for the
situation you described to work, your internal URL would have to be
http://internal.www.opennms.org/

Doing this is poor site design for exactly that reason.  Bugzilla is not the
only cookie-using web application that would be affected by this problem.  If
you really want them to be separate, the one machine should not be a subdomain
of the other, they should both be subdomains of the same common root domain. 
(The examples you actually gave us for domain names fit this model).

The patch you provided has no guarantee that it will work, since you don't know
which cookie was from which side of the wall.  And the patch on bug 95732 almost
guarantees that it won't.

Comment 4

[Benjamin Reed]

Looking more closely at the spec, yes it does appear my browser is broken, but I've seen it happen in both Konqueror and IE, so it's definitely affecting a decent amount of browsers, albeit only with very specific domain naming conventions.  As for our domain naming being "broken", it's a matter of opinion, but as long as the domain names aren't the *same* I can't see how it can reasonably be thought that it's not worth working around.  How about, instead, explicitly setting domain=$ENV{'HTTP_HOST'} in all of the Set-Cookie operations?  This fixes the issue (I have confirmed on my own bugzilla installation) *and* is still 100% "correct" to the cookie spec (it would just be forcibly setting the default).  If you agree, I can attach the patch I made against current CVS to accomplish this, otherwise I guess I'll have to maintain it myself since it's not feasible to rename our entire inter/intra-net just because one app doesn't like it.  We have a number of other cookie-based apps that work just fine.  :( 

Comment 5

[Dave Miller [:justdave]]

OK, explicitly setting the domain to $::ENV{HTTP_HOST} I don't see any problem
with.  Lets do that.

Comment 6

[Benjamin Reed]

Attachment: patch to explicitly set domain= to $ENV{'HTTP_HOST'} for cookies to work around browser bugs

Sorry about the non-wrapping previous post.  Fix one browser bug, find another.
 ;)

This is the patch that sets domain= on all Set-Cookies.

The old patch can be marked obsolete, if you care (I don't have privs to do
it).

Comment 7

[Jouni Heikniemi]

Comment on attachment 68078 [details] [diff] [review]
patch to explicitly set domain= to $ENV{'HTTP_HOST'} for cookies to work around browser bugs

Needs an update to the current codebase (templatization related changes).

Comment 8

[Joel Peshkin]

See 
http://bugzilla.mozilla.org/show_bug.cgi?id=165685#c5

The right solution for this is to grab only the first key=value pair for each key.

Comment 9

[Joel Peshkin]

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

Comment 10

[Joel Peshkin]

Attachment: Fix for both 121419 and 165685

Patch causes only the first (most-specific) key definition to be kept.

Someone who can reproduce 121419, please confirm that this covers both bugs.

Comment 11

[Dave Miller [:justdave]]

Comment on attachment 97321 [details] [diff] [review]
Fix for both 121419 and 165685

while in theory either way probably works, would it be more correct to say "if
(!exists($::COOKIE{$1}))" instead of !defined?	We're checking to see whether
this key has already been used, not whether a key we're assuming already exists
has a value or not.

Comment 12

[Joel Peshkin]

Attachment: v2 - Fix for both 121419 and 165685

Changed defined to exists in case of undefined tokens.

Comment 13

[Bradley Baetz (:bbaetz)]

Comment on attachment 97341 [details] [diff] [review]
v2 - Fix for both 121419 and 165685

Yeah, this look fine - bring on CGI.pm!

r=bbaetz

Comment 14

[Joel Peshkin]

Checking in CGI.pl;
/cvsroot/mozilla/webtools/bugzilla/CGI.pl,v  <--  CGI.pl
new revision: 1.174; previous revision: 1.173
done                                    

Comment 15

[Dave Miller [:justdave]]

low-risk high-impact fix, (see bug 220817)
nominating for the 2.16 branch.

Comment 16

[Dave Miller [:justdave]]

Patch applied without modification to the 2.16 branch (this section of CGI.pl
had not yet been modified when this was fixed in 2.17.1).

Checking in CGI.pl;
/cvsroot/mozilla/webtools/bugzilla/CGI.pl,v  <--  CGI.pl
new revision: 1.153.2.6; previous revision: 1.153.2.5
done