From da1db1402be5d249990d1beb5f41390b92f7e0be Mon Sep 17 00:00:00 2001 From: "lpsolit%gmail.com" <> Date: Tue, 28 Feb 2006 20:52:31 +0000 Subject: Bug 315605: Bugzilla::Field::check_form_field() should not take a CGI object as parameter - Patch by Frédéric Buclin r=wicked a=justdave MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- Bugzilla/Field.pm | 81 ++++++++++++++++++++----------------------------------- post_bug.cgi | 27 ++++++++++--------- process_bug.cgi | 53 ++++++++++++++++++++---------------- 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 -# Frédéric Buclin +# Frédéric Buclin # Myk Melez =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 +=item C -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 - -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') . ")"); } -- cgit v1.2.3-24-g4f1b