107743 - post_bug.cgi does not properly validate parameters.

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect, P1)

Description

[Matthew Tuck [:CodeMachine]]

post_bug.cgi needs some real loving.  The initial status can be RESOLVED, the
assignee can be blank, etc, etc.  Using a GET URL you can insert all manner of
broken bugs.

Comment 1

[Matthew Tuck [:CodeMachine]]

OK, we do seem to have checking, but you need to have strictvaluechecks on. 
This makes no sense to me.  We should make sure this stuff works and enforce it
always.

Comment 2

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

I agree 100%.  I can't think of a single reason why anyone would not want strict
value checks.  Still, we should check in the newsgroups to find out if there is
some legitimate reason we haven't thought of.

Comment 3

[Matthew Tuck [:CodeMachine]]

I suspect the reason is "terry couldn't be bothered testing/didn't have time to
test these work, so he put them in as an option defaulted to off".  Adding him
so we can get an idea.

Comment 4

[Dave Miller [:justdave]]

Terry verified in IRC today that this was indeed the reason.  The "strict value
checks" param can go away, and we can always use strict value checks.

Comment 5

[Bradley Baetz (:bbaetz)]

Can't do this until bugs in those options are fixed - adding dependancy to bug
106993. Although I'm not sure what happens currently in that case...

Comment 6

[Gervase Markham [:gerv]]

StrictValueChecks is now on all the time - does that solve this problem?

Gerv

Comment 7

[Matthew Tuck [:CodeMachine]]

Gerv, they are not on, that's why this bug has dependencies.

Comment 8

[Bradley Baetz (:bbaetz)]

Taking. My patch will check that the status is either $::unconfirmedstate or
'NEW', and also fixes bug 63018 in the same patch

Now that strict value checks are on by default, I think that thats all we need.
Is there anything else which needs to be checked?

Comment 9

[Bradley Baetz (:bbaetz)]

Attachment: patch

Comment 10

[Matthew Tuck [:CodeMachine]]

Easy way to find out is to test.

Comment 11

[Matthew Tuck [:CodeMachine]]

Comment on attachment 67820 [details] [diff] [review]
patch

I took a look at this, and everything appears in order, except that you can set
a bug to UNCONFIRMED when this is not allowed.	Perhaps the code from
enter_bug.cgi should be reused.

Comment 12

[Bradley Baetz (:bbaetz)]

Attachment: patch, v2

Comment 13

[Gervase Markham [:gerv]]

Comment on attachment 69190 [details] [diff] [review]
patch, v2

r=gerv.

Gerv

Comment 14

[Dave Miller [:justdave]]

Comment on attachment 69190 [details] [diff] [review]
patch, v2

r= justdave

looks good, applies cleanly, does what it says, and passes tests. :-)

Comment 15

[Bradley Baetz (:bbaetz)]

Fixed

Comment 16

[Dave Miller [:justdave]]

Or maybe not....

not sure why the tests didn't catch it, and I have letsubmitterchoosepriority
set on my install so I didn't notice this one.  But this is definitely broken...
 (thanks to Daa for catching it)

Param{'defaultpriority'} just doesn't work.  Has to be either Param() or
$::param{}.  (notice parens vs braces)

Comment 17

[Bradley Baetz (:bbaetz)]

Oops. Fixed.