Closed Bug 121419 Opened 19 years ago Closed 18 years ago

If multiple cookies exist, the least significant is assigned.

Categories

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

2.15
Other
Linux
defect

Tracking

()

RESOLVED FIXED
Bugzilla 2.16

People

(Reporter: ben, Assigned: bugreport)

References

Details

(Whiteboard: [fixed in 2.16.5] [fixed in 2.17.1])

Attachments

(1 file, 3 obsolete files)

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=ben@opennms.org; \
    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.
should be applied with -p0 in the root of the bugzilla tree
Why is the cookie not being set for only that particular host?
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.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → WONTFIX
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.  :( 
OK, explicitly setting the domain to $::ENV{HTTP_HOST} I don't see any problem
with.  Lets do that.
Status: RESOLVED → REOPENED
Priority: -- → P3
Resolution: WONTFIX → ---
Target Milestone: --- → Bugzilla 2.18
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).
Attachment #66119 - Attachment is obsolete: true
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).
Attachment #68078 - Flags: review-
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.

*** Bug 165685 has been marked as a duplicate of this bug. ***
Attached patch Fix for both 121419 and 165685 (obsolete) — Splinter Review
Patch causes only the first (most-specific) key definition to be kept.

Someone who can reproduce 121419, please confirm that this covers both bugs.
Attachment #68078 - Attachment is obsolete: true
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.
Changed defined to exists in case of undefined tokens.
Attachment #97321 - Attachment is obsolete: true
Comment on attachment 97341 [details] [diff] [review]
v2 - Fix for both 121419 and 165685

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

r=bbaetz
Attachment #97341 - Flags: review+
Checking in CGI.pl;
/cvsroot/mozilla/webtools/bugzilla/CGI.pl,v  <--  CGI.pl
new revision: 1.174; previous revision: 1.173
done                                    
Status: REOPENED → RESOLVED
Closed: 19 years ago18 years ago
Resolution: --- → FIXED
low-risk high-impact fix, (see bug 220817)
nominating for the 2.16 branch.
Whiteboard: [wanted for 2.16.5][fixed in 2.17.1]
Whiteboard: [wanted for 2.16.5][fixed in 2.17.1] → [wanted for 2.16.5] [fixed in 2.17.1]
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
Whiteboard: [wanted for 2.16.5] [fixed in 2.17.1] → [fixed in 2.16.5] [fixed in 2.17.1]
Target Milestone: Bugzilla 2.18 → Bugzilla 2.16
Assignee: justdave → bugreport
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.