246328 - editmilestone doesn't check for invalid sortkey

Status: RESOLVED FIXED

(Bugzilla :: Administration, task)

Description

[:glob ✱]

when editing a milestone, a non-numeric sortkey results in a sql error.

there's also a taint issue

Comment 1

[:glob ✱]

Attachment: trap invalid sortkeys and detaint

Comment 2

[Vlad Dascalu]

Comment on attachment 150519 [details] [diff] [review]
trap invalid sortkeys and detaint

Looks good to me.

-> regarding the redundancy and the duplication of the error message inside the
.cgi, I think only templatization will offer a real fix for that one. Until
then it's pretty useless to remove this redundancy since the code will get
rewritten anyway.

-> regarding previous invalid (non-numerical) sortkeys, we could add a check
for them in sanitycheck.cgi; it should be something trivial to code; at least a
bug should be opened about this. :-)

-> once we modify sanitycheck.cgi in order to enforce this new policy on old,
invalid, non-numerical sortkeys, we can be sure that $sortkeyold will contain
only numbers, so we can ditch away that unrequired trimming located a couple of
lines above:

my $sortkeyold = trim($::FORM{sortkeyold} || '0');

Since those potential issues above require either sanitycheck.cgi modification
(which is another bug) or templatization, this looks good to go for 2.18 in my
opinion. r=vladd.

Comment 3

[:glob ✱]

> -> regarding previous invalid (non-numerical) sortkeys, we could add a check
> for them in sanitycheck.cgi;

the sortkey field is numeric, so there won't be any invalid sortkeys (you'd get
a sql error when submitting the update).

Comment 4

[:glob ✱]

can someone please check this patch in for me.

Comment 5

[Vlad Dascalu]

Checking in editmilestones.cgi;
/cvsroot/mozilla/webtools/bugzilla/editmilestones.cgi,v  <--  editmilestones.cgi

new revision: 1.23; previous revision: 1.22
done