Bug 315605: Bugzilla::Field::check_form_field() should not take a CGI object as parameter - Patch by Frédéric Buclin <LpSolit@gmail.com> r=wicked a=justdave

3 files changed, 74 insertions, 87 deletions

diff --git a/Bugzilla/Field.pm b/Bugzilla/Field.pm
index 8585ff760..b6424f3df 100644
--- a/Bugzilla/Field.pm
+++ b/Bugzilla/Field.pm
@@ -13,7 +13,7 @@
 # The Original Code is the Bugzilla Bug Tracking System.
 #
 # Contributor(s): Dan Mosedale <dmose@mozilla.org>
-#                 Frédéric Buclin <LpSolit@gmail.com>
+#                 Frédéric Buclin <LpSolit@gmail.com>
 #                 Myk Melez <myk@mozilla.org>
 
 =head1 NAME
@@ -28,7 +28,7 @@ Bugzilla::Field - a particular piece of information about bugs
 
   # Display information about all fields.
   print Dumper(Bugzilla->get_fields());
-  
+
   # Display information about non-obsolete custom fields.
   print Dumper(Bugzilla->get_fields({ obsolete => 1, custom => 1 }));
 
@@ -41,11 +41,11 @@ Bugzilla::Field - a particular piece of information about bugs
   # Bugzilla->get_fields() is a wrapper around Bugzilla::Field::match(),
   # so both methods take the same arguments.
   print Dumper(Bugzilla::Field::match({ obsolete => 1, custom => 1 }));
-  
+
   # Create a custom field.
   my $field = Bugzilla::Field::create("hilarity", "Hilarity");
   print "$field->{description} is a custom field\n";
-  
+
   # Instantiate a Field object for an existing field.
   my $field = new Bugzilla::Field('qacontact_accessible');
   if ($field->{obsolete}) {
@@ -53,8 +53,7 @@ Bugzilla::Field - a particular piece of information about bugs
   }
 
   # Validation Routines
-  check_form_field($cgi, $fieldname, \@legal_values);
-  check_form_field_defined($cgi, $fieldname);
+  check_field($name, $value, \@legal_values, $no_warn);
   $fieldid = get_field_id($fieldname);
 
 =head1 DESCRIPTION
@@ -71,8 +70,7 @@ package Bugzilla::Field;
 use strict;
 
 use base qw(Exporter);
-@Bugzilla::Field::EXPORT = qw(check_form_field check_form_field_defined
-                              get_field_id);
+@Bugzilla::Field::EXPORT = qw(check_field get_field_id);
 
 use Bugzilla::Util;
 use Bugzilla::Constants;
@@ -286,66 +284,45 @@ sub match {
 
 =over
 
-=item C<check_form_field($cgi, $fieldname, \@legal_values)>
+=item C<check_field($name, $value, \@legal_values, $no_warn)>
 
-Description: Makes sure the field $fieldname is defined and its value
+Description: Makes sure the field $name is defined and its $value
              is non empty. If @legal_values is defined, this routine
              also checks whether its value is one of the legal values
-             associated with this field. If the test fails, an error
-             is thrown.
+             associated with this field. If the test is successful,
+             the function returns 1. If the test fails, an error
+             is thrown (by default), unless $no_warn is true, in which
+             case the function returns 0.
 
-Params:      $cgi          - a CGI object
-             $fieldname    - the field name to check
+Params:      $name         - the field name
+             $value        - the field value
              @legal_values - (optional) ref to a list of legal values
+             $no_warn      - (optional) do not throw an error if true
 
-Returns:     nothing
+Returns:     1 on success; 0 on failure if $no_warn is true (else an
+             error is thrown).
 
 =back
 
 =cut
 
-sub check_form_field {
-    my ($cgi, $fieldname, $legalsRef) = @_;
+sub check_field {
+    my ($name, $value, $legalsRef, $no_warn) = @_;
     my $dbh = Bugzilla->dbh;
 
-    if (!defined $cgi->param($fieldname)
-        || trim($cgi->param($fieldname)) eq ""
-        || (defined($legalsRef)
-            && lsearch($legalsRef, $cgi->param($fieldname)) < 0))
+    if (!defined($value)
+        || trim($value) eq ""
+        || (defined($legalsRef) && lsearch($legalsRef, $value) < 0))
     {
-        trick_taint($fieldname);
-        my ($result) = $dbh->selectrow_array("SELECT description FROM fielddefs
-                                              WHERE name = ?", undef, $fieldname);
-        
-        my $field = $result || $fieldname;
-        ThrowCodeError("illegal_field", { field => $field });
-    }
-}
-
-=pod
-
-=over
-
-=item C<check_form_field_defined($cgi, $fieldname)>
-
-Description: Makes sure the field $fieldname is defined and its value
-             is non empty. Else an error is thrown.
-
-Params:      $cgi       - a CGI object
-             $fieldname - the field name to check
-
-Returns:     nothing
-
-=back
-
-=cut
-
-sub check_form_field_defined {
-    my ($cgi, $fieldname) = @_;
+        return 0 if $no_warn; # We don't want an error to be thrown; return.
+        trick_taint($name);
+        my ($result) = $dbh->selectrow_array('SELECT description FROM fielddefs
+                                              WHERE name = ?', undef, $name);
 
-    if (!defined $cgi->param($fieldname)) {
-        ThrowCodeError("undefined_field", { field => $fieldname });
+        my $field = $result || $name;
+        ThrowCodeError('illegal_field', { field => $field });
     }
+    return 1;
 }
 
 =pod
diff --git a/post_bug.cgi b/post_bug.cgi
index 4d8c6a2c9..296979b79 100755
--- a/post_bug.cgi
+++ b/post_bug.cgi
@@ -197,18 +197,21 @@ if (!Param('letsubmitterchoosepriority')) {
 GetVersionTable();
 
 # Some more sanity checking
-check_form_field($cgi, 'product',      \@::legal_product);
-check_form_field($cgi, 'rep_platform', \@::legal_platform);
-check_form_field($cgi, 'bug_severity', \@::legal_severity);
-check_form_field($cgi, 'priority',     \@::legal_priority);
-check_form_field($cgi, 'op_sys',       \@::legal_opsys);
-check_form_field($cgi, 'bug_status',   ['UNCONFIRMED', 'NEW']);
-check_form_field($cgi, 'version',          $::versions{$product});
-check_form_field($cgi, 'component',        $::components{$product});
-check_form_field($cgi, 'target_milestone', $::target_milestone{$product});
-check_form_field_defined($cgi, 'assigned_to');
-check_form_field_defined($cgi, 'bug_file_loc');
-check_form_field_defined($cgi, 'comment');
+check_field('product',      scalar $cgi->param('product'),      \@::legal_product);
+check_field('rep_platform', scalar $cgi->param('rep_platform'), \@::legal_platform);
+check_field('bug_severity', scalar $cgi->param('bug_severity'), \@::legal_severity);
+check_field('priority',     scalar $cgi->param('priority'),     \@::legal_priority);
+check_field('op_sys',       scalar $cgi->param('op_sys'),       \@::legal_opsys);
+check_field('bug_status',   scalar $cgi->param('bug_status'),   ['UNCONFIRMED', 'NEW']);
+check_field('version',      scalar $cgi->param('version'),      $::versions{$product});
+check_field('component',    scalar $cgi->param('component'),    $::components{$product});
+check_field('target_milestone', scalar $cgi->param('target_milestone'),
+            $::target_milestone{$product});
+
+foreach my $field_name ('assigned_to', 'bug_file_loc', 'comment') {
+    defined($cgi->param($field_name))
+      || ThrowCodeError('undefined_field', { field => $field_name });
+}
 
 my $everconfirmed = ($cgi->param('bug_status') eq 'UNCONFIRMED') ? 0 : 1;
 $cgi->param(-name => 'everconfirmed', -value => $everconfirmed);
diff --git a/process_bug.cgi b/process_bug.cgi
index b3006565f..345bce592 100755
--- a/process_bug.cgi
+++ b/process_bug.cgi
@@ -234,10 +234,10 @@ if ($cgi->cookie("BUGLIST") && defined $cgi->param('id')) {
 
 GetVersionTable();
 
-check_form_field_defined($cgi, 'product');
-check_form_field_defined($cgi, 'version');
-check_form_field_defined($cgi, 'component');
-
+foreach my $field_name ('product', 'component', 'version') {
+    defined($cgi->param($field_name))
+      || ThrowCodeError('undefined_field', { field => $field_name });
+}
 
 # This function checks if there is a comment required for a specific
 # function and tests, if the comment was given.
@@ -325,7 +325,9 @@ if (((defined $cgi->param('id') && $cgi->param('product') ne $oldproduct)
 
     my $mok = 1;   # so it won't affect the 'if' statement if milestones aren't used
     if ( Param("usetargetmilestone") ) {
-       check_form_field_defined($cgi, 'target_milestone');
+       defined($cgi->param('target_milestone'))
+         || ThrowCodeError('undefined_field', { field => 'target_milestone' });
+
        $mok = lsearch($::target_milestone{$prod},
                       $cgi->param('target_milestone')) >= 0;
     }
@@ -595,22 +597,25 @@ if (defined $cgi->param('id')) {
     # values that have been changed instead of submitting all the new values.
     # (XXX those error checks need to happen too, but implementing them 
     # is more work in the current architecture of this script...)
-    #
-    check_form_field($cgi, 'product', \@::legal_product);
-    check_form_field($cgi, 'component', 
-                   \@{$::components{$cgi->param('product')}});
-    check_form_field($cgi, 'version', \@{$::versions{$cgi->param('product')}});
+    check_field('product', scalar $cgi->param('product'), \@::legal_product);
+    check_field('component', scalar $cgi->param('component'), 
+                \@{$::components{$cgi->param('product')}});
+    check_field('version', scalar $cgi->param('version'),
+                \@{$::versions{$cgi->param('product')}});
     if ( Param("usetargetmilestone") ) {
-        check_form_field($cgi, 'target_milestone', 
-                       \@{$::target_milestone{$cgi->param('product')}});
-    }
-    check_form_field($cgi, 'rep_platform', \@::legal_platform);
-    check_form_field($cgi, 'op_sys', \@::legal_opsys);
-    check_form_field($cgi, 'priority', \@::legal_priority);
-    check_form_field($cgi, 'bug_severity', \@::legal_severity);
-    check_form_field_defined($cgi, 'bug_file_loc');
-    check_form_field_defined($cgi, 'short_desc');
-    check_form_field_defined($cgi, 'longdesclength');
+        check_field('target_milestone', scalar $cgi->param('target_milestone'), 
+                    \@{$::target_milestone{$cgi->param('product')}});
+    }
+    check_field('rep_platform', scalar $cgi->param('rep_platform'), \@::legal_platform);
+    check_field('op_sys',       scalar $cgi->param('op_sys'),       \@::legal_opsys);
+    check_field('priority',     scalar $cgi->param('priority'),     \@::legal_priority);
+    check_field('bug_severity', scalar $cgi->param('bug_severity'), \@::legal_severity);
+
+    # Those fields only have to exist. We don't validate their value here.
+    foreach my $field_name ('bug_file_loc', 'short_desc', 'longdesclength') {
+        defined($cgi->param($field_name))
+          || ThrowCodeError('undefined_field', { field => $field_name });
+    }
     $cgi->param('short_desc', clean_text($cgi->param('short_desc')));
 
     if (trim($cgi->param('short_desc')) eq "") {
@@ -1105,7 +1110,8 @@ SWITCH: for ($cgi->param('knob')) {
     };
     /^resolve$/ && CheckonComment( "resolve" ) && do {
         # Check here, because its the only place we require the resolution
-        check_form_field($cgi, 'resolution', \@::settable_resolution);
+        check_field('resolution', scalar $cgi->param('resolution'),
+                    \@::settable_resolution);
 
         # don't resolve as fixed while still unresolved blocking bugs
         if (Param("noresolveonopenblockers")
@@ -1189,7 +1195,9 @@ SWITCH: for ($cgi->param('knob')) {
         }
 
         # Make sure we can change the original bug (issue A on bug 96085)
-        check_form_field_defined($cgi, 'dup_id');
+        defined($cgi->param('dup_id'))
+          || ThrowCodeError('undefined_field', { field => 'dup_id' });
+
         $duplicate = $cgi->param('dup_id');
         ValidateBugID($duplicate, 'dup_id');
         $cgi->param('dup_id', $duplicate);
@@ -2063,7 +2071,6 @@ foreach my $id (@idlist) {
                       " has been marked as a duplicate of this bug. ***",
                       0, $timestamp);
 
-        check_form_field_defined($cgi,'comment');
         SendSQL("INSERT INTO duplicates VALUES ($duplicate, " .
                 $cgi->param('id') . ")");
     }