Closed
Bug 268146
Opened 20 years ago
Closed 19 years ago
mod_security complain: Invalid cookie format: Cookie value is missing #2
Categories
(Bugzilla :: Bugzilla-General, defect)
Tracking
()
RESOLVED
FIXED
Bugzilla 2.18
People
(Reporter: max.odendahl, Assigned: Wurblzap)
References
Details
Attachments
(3 files, 5 obsolete files)
8.33 KB,
patch
|
kiko
:
review+
|
Details | Diff | Splinter Review |
8.20 KB,
patch
|
kiko
:
review+
|
Details | Diff | Splinter Review |
670 bytes,
patch
|
LpSolit
:
review+
wicked
:
review+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1; SV1; .NET CLR 1.1.4322) Build Identifier: 2.18R3 There is a preinstalled filter in mod_security, called SecFilterCheckCookieFormat. If this is on, bugzilla is not working anymore due to a cookie problem, the error message is: Invalid cookie format: Cookie value is missing #2 Dave mentioned a probable cause: ".. override in Bugzilla::CGI to the Cookie code to stop Perl's CGI from complaining about the value being empty.." Reproducible: Always Steps to Reproduce:
Reporter | ||
Updated•20 years ago
|
Severity: normal → critical
Comment 1•19 years ago
|
||
the correct way to do this is to set an expiration date in the past and throw some dummy value in.
Assignee: justdave → general
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking2.20+
Flags: blocking2.18.2+
OS: Linux → All
QA Contact: mattyt-bugzilla → default-qa
Hardware: PC → All
Target Milestone: --- → Bugzilla 2.18
Assignee | ||
Comment 2•19 years ago
|
||
Expiration dates in the past are already in place, so this patch doesn't more than supplying a dummy value. Oh, and it fixes what I think is a copy-and-paste error of bug 201816 in query.cgi along the way, where the expiration date of a particular cookie is in the future where it should be in the past.
Assignee | ||
Updated•19 years ago
|
Assignee | ||
Updated•19 years ago
|
Attachment #186597 -
Attachment description: Patch → HEAD Patch
Assignee | ||
Updated•19 years ago
|
Attachment #186597 -
Flags: review?
Assignee | ||
Comment 3•19 years ago
|
||
This makes cookie expiry easier by centralizing dummy value and expiry date provision.
Attachment #186597 -
Attachment is obsolete: true
Attachment #186602 -
Flags: review?
Updated•19 years ago
|
Whiteboard: [patch awaiting review]
Comment on attachment 186602 [details] [diff] [review] Patch 1.2 cvs diffs are appreciated >--- head/colchange.cgi 2005-03-17 18:21:24.000000000 +0100 >+++ patched/colchange.cgi 2005-06-17 17:21:19.766710400 +0200 >@@ -107,7 +107,7 @@ > $cgi->send_cookie(-name => 'SPLITHEADER', >- -value => $cgi->param('splitheader'), >+ -value => $cgi->param('splitheader') || '0', > -expires => 'Fri, 01-Jan-2038 00:00:00 GMT'); >--- head/query.cgi 2005-06-17 09:58:06.245454400 +0200 >+++ patched/query.cgi 2005-06-17 18:06:00.461360000 +0200 >@@ -100,8 +100,7 @@ >- $cgi->send_cookie(-name => $cookiename, >- -expires => "Fri, 01-Jan-2038 00:00:00 GMT"); >+ $cgi->send_cookie(-name => $cookiename); are you sure you mean to do this?
Assignee | ||
Comment 5•19 years ago
|
||
(In reply to comment #4) > are you sure you mean to do this? Deleting the cookie in query.cgi fixes the bug 201816 error I mentioned in comment 2. I'll look into deleting the SPLITHEADER cookie as well. Thanks for pointing that out. It should work, I'd say.
Updated•19 years ago
|
Version: unspecified → 2.18
Comment 6•19 years ago
|
||
This patch rocks! Thanks so much for doing it, this is a big pet peeve of mine.
Assignee | ||
Updated•19 years ago
|
Attachment #186602 -
Flags: review?
Assignee | ||
Comment 7•19 years ago
|
||
Expires the SPLITHEADER cookie instead of setting it to 0.
Attachment #186602 -
Attachment is obsolete: true
Attachment #187386 -
Flags: review?
Comment 8•19 years ago
|
||
Comment on attachment 187386 [details] [diff] [review] Patch 1.3 I love this patch, but I would like us to have separate API instead of overloading send_cookie. Can we have $cgi->remove_cookie()? I'm okay if it calls send_cookie() internally, but I think the API change should be explicit, not implicit. I certainly don't agree that this bug needs to ba release blocker, though..
Attachment #187386 -
Flags: review? → review-
Comment 9•19 years ago
|
||
Well, I guess the warning is a bit annoying.
Assignee | ||
Comment 10•19 years ago
|
||
Introducing remove_cookie. The drawback is that calling send_cookie without -value gives undesired results.
Attachment #187386 -
Attachment is obsolete: true
Attachment #188317 -
Flags: review?(kiko)
Comment 11•19 years ago
|
||
Comment on attachment 188317 [details] [diff] [review] Patch 1.4 Tip: use diff -p and you make life better! >Index: Bugzilla/CGI.pm >@@ -184,16 +185,36 @@ > unshift(@_, '-domain' => Param('cookiedomain')); > } > >- # Use CGI::Cookie directly, because CGI.pm's |cookie| method gives the >- # current value if there isn't a -value attribute, which happens when >- # we're expiring an entry. >- require CGI::Cookie; >- my $cookie = CGI::Cookie->new(@_); >- push @{$self->{Bugzilla_cookie_list}}, $cookie; >- >+ push(@{$self->{'Bugzilla_cookie_list'}}, $self->cookie(@_)); > return; > } Nice cleanup. I have a question: should we not print a warning if no value is supplied to this method, since it is now an error? If you think so and add that, r=kiko.
Attachment #188317 -
Flags: review?(kiko) → review+
Assignee | ||
Comment 12•19 years ago
|
||
This now throws a code error if -value is not given or empty. Any ideas how to avoid the need for the @paramlist<->%paramhash conversion in both send_cookie and remove_cookie? Minor POD change, too.
Assignee | ||
Updated•19 years ago
|
Attachment #188317 -
Attachment is obsolete: true
Attachment #188326 -
Flags: review?(kiko)
Comment 13•19 years ago
|
||
Comment on attachment 188326 [details] [diff] [review] Patch 1.5 I don't like the code duplication, either. Ah, perhaps just changing remove_cookie to take a single argument, name, is enough to solve this quagmire. I'll r+ a patch that does just that (supplying -name is a bit silly!)
Updated•19 years ago
|
Attachment #188326 -
Flags: review?(kiko) → review-
Assignee | ||
Comment 14•19 years ago
|
||
The calling convention for remove_cookie differs from the one for send_cookie now, but that can be ironed out in another bug.
Attachment #188326 -
Attachment is obsolete: true
Attachment #188330 -
Flags: review?(kiko)
Comment 15•19 years ago
|
||
Comment on attachment 188330 [details] [diff] [review] Patch 1.6 >Index: template/en/default/global/code-error.html.tmpl >+ [% ELSIF error == "cookies_need_value" %] >+ Every cookie must have a value. Maybe suggest using remove_cookie() to remove cookies, but that's a nit. r=kiko, thanks!
Attachment #188330 -
Flags: review?(kiko) → review+
Assignee | ||
Comment 16•19 years ago
|
||
Cookies needing to have a value is not only related to cookie removal. The SPLITHEADER cookie is such a case -- it was being set to either '1' or '' all the time.
Attachment #188334 -
Flags: review?(kiko)
Updated•19 years ago
|
Attachment #188334 -
Flags: review?(kiko) → review+
Assignee | ||
Updated•19 years ago
|
Flags: approval?
Flags: approval2.18?
Assignee | ||
Updated•19 years ago
|
Whiteboard: [patch awaiting review] → [patch awaiting approval]
Updated•19 years ago
|
Flags: approval?
Flags: approval2.18?
Flags: approval2.18+
Flags: approval+
Comment 17•19 years ago
|
||
Tip: Checking in buglist.cgi; /cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v <-- buglist.cgi new revision: 1.298; previous revision: 1.297 done Checking in colchange.cgi; /cvsroot/mozilla/webtools/bugzilla/colchange.cgi,v <-- colchange.cgi new revision: 1.48; previous revision: 1.47 done Checking in query.cgi; /cvsroot/mozilla/webtools/bugzilla/query.cgi,v <-- query.cgi new revision: 1.146; previous revision: 1.145 done Checking in Bugzilla/CGI.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/CGI.pm,v <-- CGI.pm new revision: 1.16; previous revision: 1.15 done Checking in Bugzilla/Auth/Login/WWW/CGI.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/Login/WWW/CGI.pm,v <-- CGI.pm new revision: 1.11; previous revision: 1.10 done Checking in template/en/default/global/code-error.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/global/code-error.html.tmpl,v <-- code-error.html.tmpl new revision: 1.50; previous revision: 1.49 done 2.18.1: Checking in buglist.cgi; /cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v <-- buglist.cgi new revision: 1.255.2.9; previous revision: 1.255.2.8 done Checking in colchange.cgi; /cvsroot/mozilla/webtools/bugzilla/colchange.cgi,v <-- colchange.cgi new revision: 1.41.2.3; previous revision: 1.41.2.2 done Checking in query.cgi; /cvsroot/mozilla/webtools/bugzilla/query.cgi,v <-- query.cgi new revision: 1.126.2.5; previous revision: 1.126.2.4 done Checking in Bugzilla/CGI.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/CGI.pm,v <-- CGI.pm new revision: 1.10.2.4; previous revision: 1.10.2.3 done Checking in Bugzilla/Auth/CGI.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/Attic/CGI.pm,v <-- CGI.pm new revision: 1.7.2.3; previous revision: 1.7.2.2 done Checking in template/en/default/global/code-error.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/global/code-error.html.tmpl,v <-- code-error.html.tmpl new revision: 1.39.2.5; previous revision: 1.39.2.4 done
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Whiteboard: [patch awaiting approval]
Comment 18•19 years ago
|
||
The 2.18 branch patch on here breaks query.cgi (at least).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Updated•19 years ago
|
Flags: blocking2.18.3+
Comment 19•19 years ago
|
||
The 2.18.2 release is broken by this! I just updated and get Software error: Can't find param named cookiedomain at Bugzilla/Config.pm line 150.
Updated•19 years ago
|
Flags: approval2.18+
Comment 20•19 years ago
|
||
Just updated to 2.18.2 (via CVS using the "Bugzilla_Stable" tag) from 2.18.1. Corrected the problem by commenting out line 193 of "Bugzilla/CGI.pm".
Comment 21•19 years ago
|
||
Cookiedomain parameter was introduced in bug 254351 that was done to 2.20 only. So in 2.18 branch patch references to this parameter should be simply removed.
Assignee | ||
Comment 22•19 years ago
|
||
:(
Attachment #188334 -
Attachment is obsolete: true
Attachment #188712 -
Flags: review?
Comment 23•19 years ago
|
||
Comment on attachment 188712 [details] [diff] [review] Additional branch patch 1.6.1 This patch fixes the problem for me. *Tested*, works. r=LpSolit
Attachment #188712 -
Flags: review? → review+
Updated•19 years ago
|
Status: REOPENED → ASSIGNED
Flags: approval2.18?
Comment 24•19 years ago
|
||
Comment on attachment 188712 [details] [diff] [review] Additional branch patch 1.6.1 I tested this patch too. Fixes the regression and cookie simple smoke tests pass (like login/logout, query, change columns).
Attachment #188712 -
Flags: review+
Updated•19 years ago
|
Flags: approval2.18? → approval2.18+
Comment 25•19 years ago
|
||
Checking in Bugzilla/CGI.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/CGI.pm,v <-- CGI.pm new revision: 1.10.2.5; previous revision: 1.10.2.4 done
Status: ASSIGNED → RESOLVED
Closed: 19 years ago → 19 years ago
Resolution: --- → FIXED
Comment 26•19 years ago
|
||
Comment on attachment 188334 [details] [diff] [review] Branch patch 1.6 This patch is not obsolete because it did get checked in, and needs to be applied in addition to the followup patch, for anyone that's following along manually.
Attachment #188334 -
Attachment is obsolete: false
You need to log in
before you can comment on or make changes to this bug.
Description
•