Closed Bug 238865 Opened 16 years ago Closed 16 years ago

remove %FORM from page.cgi

Categories

(Bugzilla :: Bugzilla-General, enhancement)

2.17.7
enhancement
Not set

Tracking

()

RESOLVED FIXED
Bugzilla 2.18

People

(Reporter: justdave, Assigned: kiko)

References

Details

Attachments

(1 file, 2 obsolete files)

This should be another easy one

page.cgi:
45:if ($::FORM{'id'}) {
47:    $::FORM{'id'} =~ s/[^\w\-\.]//g;
48:    $::FORM{'id'} =~ /(.*)\.(.*)/;
52:    $vars->{'form'} = \%::FORM; 

Line 52 can just go away.  Make the template use the cgi stuff directly.

template/en/default/pages/linked.html.tmpl:
33:[%- form.text FILTER quoteUrls FILTER html -%]
48:[%- form.text FILTER quoteUrls -%]
Summary: remove %FORM and %COOKIE from page.cgi → remove %FORM from page.cgi
Attached patch kiko_v1: fix (obsolete) — Splinter Review
I decided not to manipulate (via cgi->param(X,Y)) the original value of
$FORM{id}, but used a local variable instead; check to see if that's what we
want.
Assignee: nobody → kiko
Status: NEW → ASSIGNED
Attachment #144984 - Flags: review?(justdave)
Target Milestone: --- → Bugzilla 2.18
Comment on attachment 144984 [details] [diff] [review]
kiko_v1: fix

We used to do:

-    $::FORM{'id'} =~ s/[^\w\-\.]//g;

which changed the id CGI param in order to eliminate dodgy characters. After
applying this patch, the template will get the "id" param with the dodgy chars
included, because the patch modifies the replacement, which is now applied to
the $id variable and not to the "id" CGI param.

So we could end up (after applying the patch) with a situation where the id
would be "!@#!~page.html", and $id would be "page.html", but in the template,
Bugzilla.cgi.param will still return "!@#!~page.html".

I've talked with justdave and he said that using cgi.param('x', 'y') in the CGI
file in order to do the modification changes what cgi.param() sees from within
the template, so we should probably use that somewhere. For example doing:

$cgi->param('id', $id);

right after the XXX could solve this.
Attachment #144984 - Flags: review-
Comment on attachment 144984 [details] [diff] [review]
kiko_v1: fix

Besides concurring with vladd on the above...

>Index: page.cgi
>+    $id =~ /(.*)\.(.*)/;
>+    # XXX if this regexp fails to match, raise a user error

It's not that hard to throw an error, so as long as we know this, why not do it
now?

>Index: template/en/default/pages/linked.html.tmpl
>+[% USE Bugzilla %]
>+[%- Bugzilla.cgi.param("text") FILTER quoteUrls FILTER html -%]
>+[%- Bugzilla.cgi.param("text") FILTER quoteUrls -%]

Since we're using it more than once, it might be worth doing 
[% cgi = Bugzilla.cgi %] under the [% USE Bugzilla %] and then nuking the
"Bugzilla." from the other two instances.  This is probably a good idea anyway
since this is the only template in pages using this trick currently and
aspiring programmers are probably going to use it as an example.
Attachment #144984 - Flags: review?(justdave)
Severity: normal → enhancement
Attached patch kiko_v2: fair enough (obsolete) — Splinter Review
Attachment #144984 - Attachment is obsolete: true
Attachment #145585 - Flags: review?(vlad)
(In reply to comment #2)
> -    $::FORM{'id'} =~ s/[^\w\-\.]//g;
> 
> which changed the id CGI param in order to eliminate dodgy characters.

Note that the ID itself isn't used in any existing template itself; it could be
used via a [% cgi.param("id") %] block, however.
Comment on attachment 145585 [details] [diff] [review]
kiko_v2: fair enough

+++ template/en/default/global/user-error.html.tmpl	7 Apr 2004 03:59:31
-0000

+	 ThrowUserError("bad_page_cgi_id", { "page_id" => $id });


Weren't we using ThrowCodeError for that kind of stuff? I guess UserError works
as guess.

I guess justdave can comment on that one when the does the approval thing and
we can change "User" --> "Code" upon checkin.
Attachment #145585 - Flags: review?(vlad) → review+
s/as guess/as well/
(in the previous comment)
Flags: approval?
The rationale for using a UserError and not a CodeError is that some of the
pages aren't linked from anywhere (for instance, linkify.html) -- I guess we're
not expecting the user to be typing URLs in, but it's not strictly a CodeError
(I think, IOW, a bug in Bugzilla) if somebody offers a broken link to
pinkify.html, right?
> Weren't we using ThrowCodeError for that kind of stuff? I guess UserError
> works as guess.

When in doubt, it's a code error, since it's better for usability to blame the
program when it's the user's fault than blame the user when it's the program's
fault.  Programs don't have feelings, but users do, and blaming users wrongly
hurts and frustrates them.

> it's not strictly a CodeError
> (I think, IOW, a bug in Bugzilla) if somebody offers a broken link to
> pinkify.html, right?

It's a code error in the HTML code containing the broken link, so it's not in
Bugzilla itself, but that's no reason to blame the user for it.

a=myk if this is made a code error
Flags: approval? → approval+
You guys make me work hard for the money.
Attachment #145585 - Attachment is obsolete: true
Attachment #145759 - Flags: review?
Attachment #145759 - Flags: review? → review+
/cvsroot/mozilla/webtools/bugzilla/page.cgi,v  <--  page.cgi
new revision: 1.13; previous revision: 1.12
/cvsroot/mozilla/webtools/bugzilla/template/en/default/pages/linked.html.tmpl,v
 <--  linked.html.tmpl
new revision: 1.5; previous revision: 1.4
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/code-error.html.tmpl,v
 <--  code-error.html.tmpl
new revision: 1.37; previous revision: 1.36

Thanks.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
(In reply to comment #9)
> When in doubt, it's a code error, since it's better for usability to blame the
> program when it's the user's fault than blame the user when it's the program's
> fault.  Programs don't have feelings, but users do, and blaming users wrongly
> hurts and frustrates them.

I disagree.  Nothing in ThrowUserError implies that the user is at fault unless
the error message itself says so.  Using ThrowUserError does not mean we are
blaming the user for the problem.  Make sure the error message is constructively
worded such that it's clear that we're not blaming the user, and there's no problem.

Using ThrowCodeError in a situation in which Bugzilla itself is NOT broken is a
mistake.  This makes the user think Bugzilla *is* broken, which can cause
serious problems in a corporate environment when you have PHBs using the system
who are ready to toss blame all over the place for leaving a bug in the system.
 Trust me, I've lived through it.  Those words "Bugzilla has suffered an
internal error" are downright scary to the PHBs.  They consider an error worded
like that to be comparable to the program crashing.  Also, ThrowCodeError
instructs the user to screenshot (or otherwise save) the page and mail it to the
maintainer.  You don't want someone mailing the maintainer every time they typo
a hand-crafted URL.  For places where this is likely to be common (like
page.cgi) it's better to use ThrowUserError and craft the error message such
that "if you clicked a link to get to this message, please contact the author of
the web page containing the link" or something to that effect.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 145759 [details] [diff] [review]
from the code-error-it-is department

review- per previous comment.

This patch has been backed out.
Attachment #145759 - Flags: review-
Flags: approval+
The patch shouldn't have been backed out in the first place in my opinion.

Anyway, I think we should stick with ThrowCodeError in my opinion. Linking to a
Bugzilla page and obtaining ThrowCodeError because the URL was wrong is not
something new. For example, see:

http://bugzilla.mozilla.org/votes.cgi?action=sh3ow_user

which displays a CodeError because the URL doesn't contain a valid action. Is
Bugzilla broken in this case? No. The URL is invalid, which caused an internal
code error.

If we want to change this behaviour, we have a lot of places to do that, and
probably it's outside the scope of this bug anyway.
Attachment #145759 - Attachment is obsolete: true
Comment on attachment 145585 [details] [diff] [review]
kiko_v2: fair enough

kiko: this means you should commit this one then.
Attachment #145585 - Attachment is obsolete: false
Flags: approval?
Well, Vlad, I disagree with you, typing in an invalid action *should* be a 
code error, since most of the time, action in urls is from forms, the part 
this patch checks against is entirely possible for an "End User" to look and 
go, "hey, I know where I need...*type* *type*...internal error, what?? damn 
why'd they leave that bug in there"

where-as if they try to change an action, well its perfectly 
believable "internal error" will show up.
yeah, you're right, I probably shouldn't have backed it out.  It was late and I
was tired.  See the discussion on the subject on the developers list.  Let's get
this in, we can clean it up later after the discussion on the list is resolved.

I don't care which of the two you check in...  Although with the current
wording, probably better to stick with a CodeError for the reasons Myk
mentioned.  If this goes in as a UserError (which as mentioned, I think is
better for this situation) then wording needs to be clarified some to explain
the error in "layman's English" so the user can have a clue and not feel like
they broke it. :)
Flags: approval? → approval+
Cleaning up the mess I made:

Checking in page.cgi;
/cvsroot/mozilla/webtools/bugzilla/page.cgi,v  <--  page.cgi
new revision: 1.15; previous revision: 1.14
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; previous revision: 1.38
done
Checking in template/en/default/pages/linked.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/pages/linked.html.tmpl,v
 <--  linked.html.tmpl
new revision: 1.7; previous revision: 1.6
done
Status: REOPENED → RESOLVED
Closed: 16 years ago16 years ago
Resolution: --- → FIXED
Attachment #145585 - Attachment is obsolete: true
Attachment #145759 - Attachment is obsolete: false
Attachment #145759 - Flags: review-
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.