279303 - Negative numbers are rejected as invalid sortkeys for milestones

Status: RESOLVED FIXED

(Bugzilla :: Administration, task)

Description

[Peter D. Stout]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0
Build Identifier: 2.18

We have recently upgraded to 2.18 from 2.16.1 and the editmilestones.cgi script
no longer accepts negative numbers as valid sortkeys.  We use negative numbers
for old milestones so that they will sort above the default '---' milestone and
any newly created milestones, which are assigned a sortkey of zero by default.

The problem appears to be in the use of the detaint_natural method invoked at
line 522 of editmilestones.cgi.  The pattern in detaint_natural does not permit
a leading minus sign, which is appropriate if the method is suppose to be
checking if the argument is a NATURAL, i.e non-negative, number but overly
restrictive for sortkeys, perhaps there should be a detaint_signed method.

Reproducible: Always

Steps to Reproduce:
1. Select a product that has milestones
2. Click on "Edit milestones" link
3. Click on the edit link for a milestone
4. Type -1 in the sortkey field and click the update button

Actual Results:  
An error page is displayed with the following message:

The sortkey for a milestone must be a number. Please press Back and try again.

Expected Results:  
Set the sortkey field for the milestone.

Comment 1

[Dave Miller [:justdave]]

OK, what Frederic meant to imply by adding that dependency is that this is a
regression caused by the checkin on the dependent bug.

Whether or not we want to fix this is another issue...  not allowing negative
numbers makes the error checking a lot easier...

What I do on mozilla.org is move the sortkey on the --- default value forward as
we retire older versions.  Seems like that would be easier than moving all of
the retired ones earlier...

Comment 2

[Peter D. Stout]

Attachment: Patch to add allow negative milestone sortkeys

This patch adds a detaint_signed method to the Util module that accepts signed
integers and updates editmilestones.cgi to use it.

Comment 3

[Peter D. Stout]

(In reply to comment #1)
> Whether or not we want to fix this is another issue...  not allowing negative
> numbers makes the error checking a lot easier...

The complexity of my proposed patch doesn't seem that great to me.

> What I do on mozilla.org is move the sortkey on the --- default value forward as
> we retire older versions.  Seems like that would be easier than moving all of
> the retired ones earlier...

Doesn't this create the implicit requirement that you remember to explicitly set
the sortkey when you create a new milestone, rather than using the default of
zero?  For us, using negative sortkeys for old milestones means that the person
creating a new milestone does not need to think about sortkeys.

Comment 4

[Dave Miller [:justdave]]

hmm, well, in our case, the milestones are all based on version numbers, so I
just use the version number itself as the sort key (dropping the decimal points
and tacking a few zeroes on the end if necessary so they're all the same length)

I like the idea of having a detaint_signed function available, regardless, and
since we used to allow this, it probably makes sense to put it back.

Comment 5

[Dave Miller [:justdave]]

Comment on attachment 172043 [details] [diff] [review]
Patch to add allow negative milestone sortkeys

>+sub detaint_signed {
>+    $_[0] =~ /^(-?\d+)$/;

my only question with this, is should we allow a "+" character as well?  Will a
database server choke if we pass in one with a +?

Maybe this routine should allow a + and silently remove it if present?

These are just blue sky ideas, looking for opinions from others, so don't feel
like you need to re-roll the patch or anything yet.

Comment 6

[Frédéric Buclin]

(In reply to comment #3)
> Doesn't this create the implicit requirement that you remember to explicitly set
> the sortkey when you create a new milestone, rather than using the default of
> zero?  For us, using negative sortkeys for old milestones means that the person
> creating a new milestone does not need to think about sortkeys.

On the other hand, you have to change sortkeys of milestones when they become
old. What is the easiest? Incrementing the sortkey of the default "----"
milestone or changing your milestone sortkeys to negative values?

Comment 7

[Peter D. Stout]

(In reply to comment #6)
> On the other hand, you have to change sortkeys of milestones when they become
> old. What is the easiest? Incrementing the sortkey of the default "----"
> milestone or changing your milestone sortkeys to negative values?

They are both reasonable strategies.  I have found, at least within our
organization, using "A" negative sortkey easier to explain to my users.

* When they add a milestone they don't need to think about the sortkey.
* When they want to obsolete a milestone they set the sortkey to -1.

Arguably, we are using the sortkey value more like an enabled/disabled flag. 
(In fact, that might be a nice feature to add, so that the pick list for new
bugs would be shorter.)

Comment 8

[Peter D. Stout]

(In reply to comment #5)
> (From update of attachment 172043 [details] [diff] [review] [edit])
> >+sub detaint_signed {
> >+    $_[0] =~ /^(-?\d+)$/;
> 
> my only question with this, is should we allow a "+" character as well?  Will a
> database server choke if we pass in one with a +?
> 
> Maybe this routine should allow a + and silently remove it if present?

I have tested that a "+" character is acceptable to the MySQL (versions 3.23.53a
and 4.0.23) and Oracle (9.2), which are the databases to which I have easy
access.  It is a trivial change to my patch to accept the plus sign and pass it
through.  Would you like me to resubmit my patch with the change?

Comment 9

[Frédéric Buclin]

(In reply to comment #8)
> through.  Would you like me to resubmit my patch with the change?

Please do, with detaint_signed silently removing the "+" sign if present!

Comment 10

[Frédéric Buclin]

Hum... We have some inconsistencies about milestones:

checksetup.pl specifies that the "sortkey" attribute of the "milestones" table
has the following format: "sortkey smallint not null", that means: -32768 <=
sortey <= 32767 (or 0 < sortkey <= 32767 if we don't want negative values).

But looking at administration.xml, I read:
"Enter the name of the Milestone in the "Milestone" field. You can optionally
set the "sortkey", which is a positive or negative number (-255 to 255) that
defines where in the list this particular milestone appears."

First, why between -255 and 255 only if we declare sortkey to be a smallint?
Second, why do we write here that sortkeys may be negative as we don't accept
negative values?

Independently of what we decide to do, either editmilestones.cgi or
administration.xml has to change. :)

Comment 11

[Max Kanat-Alexander]

Comment on attachment 172043 [details] [diff] [review]
Patch to add allow negative milestone sortkeys

In addition to LpSolit's comments:

> is_tainted trick_taint detaint_natural
> detaint_signed

Those need to be on one line, and you need to modify the POD documentation at
the bottom, in the next patch.

justdave, there isn't some reason that we don't already have this function, is
there?

Comment 12

[Peter D. Stout]

Attachment: Patch to add allow negative milestone sortkeys - v2

> From update of attachment 172043 [details] [diff] [review] [edit])
> In addition to LpSolit's comments:
>
>> is_tainted trick_taint detaint_natural
>> detaint_signed
>
> Those need to be on one line, and you need to modify the POD documentation at

> the bottom, in the next patch.

This version accepts, and discards, a leading plus sign.  I have also joined
the two lines in the EXPORT statement.	(Curiousity question, is this a style
issue or is there a technical subtlety?  I had been unable to identify a
pattern in the existing list so I put the new function on a new line to avoid
going past 80 characters.)  And lastly, I updated the POD documentation.

As for the range issue (-255 to 255 vs. -32768 to 32767), I did not add that to
this patch for two reasons.  First, I wasn't certain what the range was
intended to be.  Second, I wasn't certain whether it should be fixed as part of
this bug, as a new patch for 246328, or as part of a new bug.  If you want to
fix it as part of this bug, I would be happy to spin another version of patch
with a range check added to editmilestones.cgi.  For reference, MySQL 4.0
silently converts out of range values to the closest in-range value, i.e. 40000
-> 32767 and -40000 -> -32768.

Comment 13

[Frédéric Buclin]

Comment on attachment 172242 [details] [diff] [review]
Patch to add allow negative milestone sortkeys - v2

There are two places in editmilestones.cgi where we have
if (!detaint_natural($sortkey)) {

Here and when creating a new milestone. In order to be consistent, we should
modify the other one too and write detaint_signed in both places.

Comment 14

[Max Kanat-Alexander]

(In reply to comment #12)
> (Curiousity question, is this a style
> issue or is there a technical subtlety?  I had been unable to identify a
> pattern in the existing list so I put the new function on a new line to avoid
> going past 80 characters.)

  Oh. If putting it on the same line causes the line to go past 80 characters,
then it does need to be on a separate line. :-) Sorry about that.

  The technical subtlety was that all of the functions that were in one
"category" all went on the same line.

Comment 15

[Peter D. Stout]

Attachment: Patch to add allow negative milestone sortkeys - v3

I replaced the taint_natural in the "new" block with taint_signed.  The two
blocks that check the sortkey value are identical, should they be refactored
into a CheckSortkey subroutine along the lines of CheckProduct and
CheckMilestone?  This seems like a good idea to me, particularly if the code is
going to be changed to do range checking, but I wasn't sure if there might be
issues related to tainting.

I also fixed the EXPORT statement to not exceed 80 characters.

Comment 16

[Frédéric Buclin]

Comment on attachment 172272 [details] [diff] [review]
Patch to add allow negative milestone sortkeys - v3

The patch is bitrotten due to the editmilestones.cgi part. But what I have seen
looks good. Please update your patch and ask me to review it. We still have a
small chance to have it for 2.20 even if the trunk is already frozen.

justdave, if you don't want to allow negative sortkeys, please mark this bug as
wontfix...

Comment 17

[Shane H. W. Travis]

If this really is a regression, and we want to fix it, then we should block 
releases on it.

Comment 18

[Peter D. Stout]

Attachment: 2.18: Patch to allow negative milestone sortkeys - v4

This patch is against the 2.18 branch.	It adds range checking to the previous
patch and updates the documentation and the "user error template" to report
consistent values.

Comment 19

[Peter D. Stout]

Attachment: 2.20: Patch to allow negative milestone sortkeys - v1

This patch is against the TRUNK.  It adds range checking to the previous
patch and updates the documentation and the "user error template" to report
consistent values.

As part of the change I moved the check that the sortkey is valid outside of
the check that the sortkey is the same as the old sortkey.  It seemed a little
odd that we were using the sortkey value and then checking whether it was
valid.

Comment 20

[Peter D. Stout]

Comment on attachment 179187 [details] [diff] [review]
2.18: Patch to allow negative milestone sortkeys - v4

Is the user-error.html.tmpl file used in 2.18?	I fixed it in this patch, but I
wasn't sure that it was actually being used.

Comment 21

[Frédéric Buclin]

Comment on attachment 179189 [details] [diff] [review]
2.20: Patch to allow negative milestone sortkeys - v1

>-    # Need to store because detaint_natural() will delete this if
>-    # invalid
>-    my $stored_sortkey = $sortkey;
>+    $sortkey = CheckSortkey($milestone, $sortkey);
>     if ($sortkey != $sortkeyold) {
>-        if (!detaint_natural($sortkey)) {
>-            ThrowUserError('milestone_sortkey_invalid',
>-                           {'name' => $milestone,
>-                            'sortkey' => $stored_sortkey});
>-        }

Nit: $sortkey is not used outside the first IF block, so doing its check only
if its value changes is better. I can fix this point on checkin.


     The sortkey '[% sortkey FILTER html %]' for milestone '
>-    [% name FILTER html %]' is not a valid (positive) number.
>+    [% name FILTER html %]' is not in the range -32768 <= sortkey
>+    <= 32767.

Nit: Use ≤ instead of <=. Again I can fix this on checkin.

But if you prefer to submit a new patch with these nits fixed, I won't say
'no'. ;)

r=LpSolit

Comment 22

[Frédéric Buclin]

Comment on attachment 179187 [details] [diff] [review]
2.18: Patch to allow negative milestone sortkeys - v4

>+sub CheckSortkey ($$$)
>+{
>+    my ($milestone,$sortkey,$localtrailer) = @_;
>+    # Keep a copy in case detaint_signed() clears the sortkey
>+    my $stored_sortkey = $sortkey;
>+
>+    if (!detaint_signed($sortkey) || $sortkey < -32768 || $sortkey > 32767) {
>+        print "The sortkey for a milestone must be a number between -32768 ";
>+        print "and 32767 inclusive.  Please press\n";
>+        print "<b>Back</b> and try again.\n";
>+        PutTrailer($localtrailer);
>+        exit;
>+    }
>+
>+    return $sortkey;
>+}

$localtrailer is accessible from everywhere in the file. Then, there is no need
to pass it as a param.


>+    $sortkey = CheckSortkey($milestone,$sortkey,$localtrailer);
>     if ($sortkey != $sortkeyold) {
>-        if (!detaint_natural($sortkey)) {
>-            print "The sortkey for a milestone must be a number. Please press\n";
>-            print "<b>Back</b> and try again.\n";
>-            PutTrailer($localtrailer);
>-            exit;
>-        }

$sortkey is used only if its value has changed. Then you should move the
validation inside the IF block.


>   [% ELSIF error == "flag_type_sortkey_invalid" %]
>     [% title = "Flag Type Sort Key Invalid" %]
>-    The sort key must be an integer between 0 and 32767 inclusive.
>+    The sort key must be an integer between -32768 and 32767 inclusive.
>     It cannot be <em>[% sortkey FILTER html %]</em>.

This error message is for flags, not for milestones!! Please let this error
message alone. :)

Comment 23

[Peter D. Stout]

Attachment: 2.20: Patch to allow negative milestone sortkeys - v2

This fixes the couple nits that were pointed out in the first version of the
patch for 2.20.

Comment 24

[Peter D. Stout]

Attachment: 2.18: Patch to allow negative milestone sortkeys - v5

This patch removes the unnecessary parameter to CheckSortKey, moves the check
inside the check that the value changed, and removes the change to the flags
sortkey error message.

Comment 25

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

"If it's not a regression from 2.18 and it's not a critical problem with
something that's already landed, let's push it off." - Dave

Comment 26

[Peter D. Stout]

While this is certainly not a critical bug to fix, the behavior of 2.18 is a
regression from the behavior of 2.16.

Comment 27

[Frédéric Buclin]

Don't worry, your patches will be checked in on time. I will review your patches
in a few tens of minutes. ;)

Comment 28

[Frédéric Buclin]

Comment on attachment 181339 [details] [diff] [review]
2.20: Patch to allow negative milestone sortkeys - v2

r=LpSolit

Comment 29

[Frédéric Buclin]

Comment on attachment 181348 [details] [diff] [review]
2.18: Patch to allow negative milestone sortkeys - v5

Could you please submit a patch against the 2.18 branch please? This patch does
not apply at all!

Comment 30

[Peter D. Stout]

Attachment: 2.18: Patch to allow negative milestone sortkeys - v6

Oops, it never pays to try and do something in a hurry.  I forgot to change
working directories before generating the 2.18 patch.  Here is the correct
patch.

Comment 31

[Frédéric Buclin]

Comment on attachment 181713 [details] [diff] [review]
2.18: Patch to allow negative milestone sortkeys - v6

>--- editmilestones.cgi	27 Jul 2004 15:14:51 -0000	1.23.2.1
>+++ editmilestones.cgi	24 Apr 2005 21:46:30 -0000
>@@ -87,6 +87,23 @@
>+sub CheckSortkey ($$)
>+{
>+    my ($milestone,$sortkey) = @_;
>+    # Keep a copy in case detaint_signed() clears the sortkey
>+    my $stored_sortkey = $sortkey;
>+
>+    if (!detaint_signed($sortkey) || $sortkey < -32768 || $sortkey > 32767) {
>+        print "The sortkey for a milestone must be a number between -32768 ";
>+        print "and 32767 inclusive.  Please press\n";
>+        print "<b>Back</b> and try again.\n";
>+        PutTrailer($localtrailer);
>+        exit;
>+    }
>+
>+    return $sortkey;
>+}

$stored_sortkey is not used; remove it. Moreover, as CheckSortkey() uses
$localtrailer, this one has to be declared before this function.

Comment 32

[Peter D. Stout]

In the patch for 2.18, should CheckSortkey be using $localtrailer at all?  I put
it in originally, because it was used in the block of code that formed the basis
for the CheckSortkey routine.  When I started to fix the 2.18 patch, I noticed
that the other Check* routines, however, don't use it.  Before I started moving
the declaration of $localtrailer, I decide I should check whether it should
really be used at all.  Preferences?

Comment 33

[Frédéric Buclin]

You are right. If you can remove $localtrailer from CheckSortkey() but keeping
PutTrailer() in this function, that's fine. Else if you cannot, use something like:

if (!CheckSortkey($sortkey)) {
    print ...;
    PutTrailer($localtrailer);
    exit;
}

Comment 34

[Peter D. Stout]

Attachment: 2.18: Patch to allow negative milestone sortkeys - v7

Fixed issues with CheckSortkey method.

Comment 35

[Frédéric Buclin]

Comment on attachment 181871 [details] [diff] [review]
2.18: Patch to allow negative milestone sortkeys - v7

Removing $localtrailer from PutTrailer() prevents the "edit more milestones"
link from being displayed if CheckSortkey() fails. As this is the actual
behavior of both CheckProduct() and CheckMilestone(), I guess this is fine.

So r=LpSolit

Comment 36

[Frédéric Buclin]

Comment on attachment 181871 [details] [diff] [review]
2.18: Patch to allow negative milestone sortkeys - v7

>     if ($sortkey != $sortkeyold) {
>-        if (!detaint_natural($sortkey)) {
>-            print "The sortkey for a milestone must be a number. Please press\n";
>-            print "<b>Back</b> and try again.\n";
>-            PutTrailer($localtrailer);
>-            exit;
>-        }
>+        $sortkey = CheckSortkey($milestone,$sortkey);

Note that if $sortkeyold is a valid integer but $sortkey = $sortkeyold .
$anystring, the IF block is not executed because we use '!=' instead of 'ne'
for the test. Then Bugzilla behaves as if the sortkey did not change, which I
think does not hurt at this point. Both 2.18.1 and 2.19.3 are affected.

Comment 37

[Dave Miller [:justdave]]

(In reply to comment #36)
> Note that if $sortkeyold is a valid integer but $sortkey = $sortkeyold .
> $anystring, the IF block is not executed because we use '!=' instead of 'ne'
> for the test. Then Bugzilla behaves as if the sortkey did not change, which I
> think does not hurt at this point. Both 2.18.1 and 2.19.3 are affected.

Hmm, this is a good point, but I think it'd be better to tell the user they
entered something wrong than to silently ignore it...  we should fix that to use
'ne'.

Comment 38

[Frédéric Buclin]

justdave, I can change '!=' to 'ne' on checkin. Please don't hold approval for
this. ;)

Comment 39

[Dave Miller [:justdave]]

ok, go for it. Make sure we get an updated patch on the bug though, since we're
touching a branch.

Comment 40

[Frédéric Buclin]

Attachment: patch checked in, 2.20

updated patch as requested by justdave.

Comment 41

[Frédéric Buclin]

Attachment: patch checked in, 2.18

updated patch for 2.18, as requested by justdave.

Comment 42

[Frédéric Buclin]

Tip:

Checking in editmilestones.cgi;
/cvsroot/mozilla/webtools/bugzilla/editmilestones.cgi,v  <--  editmilestones.cgi
new revision: 1.38; previous revision: 1.37
done
Checking in Bugzilla/Util.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Util.pm,v  <--  Util.pm
new revision: 1.25; previous revision: 1.24
done
Checking in docs/xml/administration.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/administration.xml,v  <-- 
administration.xml
new revision: 1.48; previous revision: 1.47
done
Checking in template/en/default/global/user-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tmpl,v
 <--  user-error.html.tmpl
new revision: 1.107; previous revision: 1.106
done


2.18:

Checking in editmilestones.cgi;
/cvsroot/mozilla/webtools/bugzilla/editmilestones.cgi,v  <--  editmilestones.cgi
new revision: 1.23.2.2; previous revision: 1.23.2.1
done
Checking in Bugzilla/Util.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Util.pm,v  <--  Util.pm
new revision: 1.12.2.3; previous revision: 1.12.2.2
done
Checking in docs/xml/administration.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/administration.xml,v  <-- 
administration.xml
new revision: 1.34.2.11; previous revision: 1.34.2.10
done