Closed Bug 268146 Opened 15 years ago Closed 15 years ago

mod_security complain: Invalid cookie format: Cookie value is missing #2

Categories

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

2.18
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Bugzilla 2.18

People

(Reporter: max.odendahl, Assigned: Wurblzap)

References

Details

Attachments

(3 files, 5 obsolete files)

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:
Severity: normal → critical
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
Attached patch HEAD Patch (obsolete) — Splinter Review
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: general → wurblzap
Status: NEW → ASSIGNED
Attachment #186597 - Flags: review?
Attachment #186597 - Attachment description: Patch → HEAD Patch
Attachment #186597 - Flags: review?
Attached patch Patch 1.2 (obsolete) — Splinter Review
This makes cookie expiry easier by centralizing dummy value and expiry date
provision.
Attachment #186597 - Attachment is obsolete: true
Attachment #186602 - Flags: review?
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?
(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.
Version: unspecified → 2.18
This patch rocks! Thanks so much for doing it, this is a big pet peeve of mine.
Attachment #186602 - Flags: review?
Attached patch Patch 1.3 (obsolete) — Splinter Review
Expires the SPLITHEADER cookie instead of setting it to 0.
Attachment #186602 - Attachment is obsolete: true
Attachment #187386 - Flags: review?
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-
Well, I guess the warning is a bit annoying.
Attached patch Patch 1.4 (obsolete) — Splinter Review
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 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+
Attached patch Patch 1.5 (obsolete) — Splinter Review
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.
Attachment #188317 - Attachment is obsolete: true
Attachment #188326 - Flags: review?(kiko)
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!)
Attachment #188326 - Flags: review?(kiko) → review-
Attached patch Patch 1.6Splinter Review
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 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+
Attached patch Branch patch 1.6Splinter Review
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)
Attachment #188334 - Flags: review?(kiko) → review+
Flags: approval?
Flags: approval2.18?
Whiteboard: [patch awaiting review] → [patch awaiting approval]
Flags: approval?
Flags: approval2.18?
Flags: approval2.18+
Flags: approval+
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: 15 years ago
Resolution: --- → FIXED
Whiteboard: [patch awaiting approval]
The 2.18 branch patch on here breaks query.cgi (at least).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Flags: blocking2.18.3+
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.
Flags: approval2.18+
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".
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.
:(
Attachment #188334 - Attachment is obsolete: true
Attachment #188712 - Flags: review?
Blocks: 300138
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+
Status: REOPENED → ASSIGNED
Flags: approval2.18?
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+
Flags: approval2.18? → approval2.18+
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: 15 years ago15 years ago
Resolution: --- → FIXED
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.