238865 - remove %FORM from page.cgi

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement)

Description

[Dave Miller [:justdave]]

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 -%]

Comment 1

[Christian Reis]

Attachment: kiko_v1: fix

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.

Comment 2

[Vlad Dascalu]

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.

Comment 3

[Dave Miller [:justdave]]

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.

Comment 4

[Christian Reis]

Attachment: kiko_v2: fair enough

Comment 5

[Christian Reis]

(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 6

[Vlad Dascalu]

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.

Comment 7

[Vlad Dascalu]

s/as guess/as well/
(in the previous comment)

Comment 8

[Christian Reis]

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?

Comment 9

[Myk Melez [:myk] [@mykmelez]]

> 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

Comment 10

[Christian Reis]

Attachment: from the code-error-it-is department

You guys make me work hard for the money.

Comment 11

[Christian Reis]

/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.

Comment 12

[Dave Miller [:justdave]]

(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.

Comment 13

[Dave Miller [:justdave]]

Comment on attachment 145759 [details] [diff] [review]
from the code-error-it-is department

review- per previous comment.

This patch has been backed out.

Comment 14

[Vlad Dascalu]

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.

Comment 15

[Vlad Dascalu]

Comment on attachment 145585 [details] [diff] [review]
kiko_v2: fair enough

kiko: this means you should commit this one then.

Comment 16

[Justin Wood]

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.

Comment 17

[Dave Miller [:justdave]]

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. :)

Comment 18

[Dave Miller [:justdave]]

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