Commit c1ca8605 authored by mkanat%bugzilla.org's avatar mkanat%bugzilla.org

Bug 440612 – Use Bugzilla::Bug->check everywhere instead of ValidateBugID

Patch By Max Kanat-Alexander <mkanat@bugzilla.org> r=LpSolit, a=LpSolit
parent 9ed763d9
...@@ -49,7 +49,7 @@ use Storable qw(dclone); ...@@ -49,7 +49,7 @@ use Storable qw(dclone);
use base qw(Bugzilla::Object Exporter); use base qw(Bugzilla::Object Exporter);
@Bugzilla::Bug::EXPORT = qw( @Bugzilla::Bug::EXPORT = qw(
bug_alias_to_id ValidateBugID bug_alias_to_id
RemoveVotes CheckIfVotedConfirmed RemoveVotes CheckIfVotedConfirmed
LogActivityEntry LogActivityEntry
editable_bug_fields editable_bug_fields
...@@ -1091,8 +1091,8 @@ sub _check_dependencies { ...@@ -1091,8 +1091,8 @@ sub _check_dependencies {
my @bug_ids = split(/[\s,]+/, $deps_in{$type}); my @bug_ids = split(/[\s,]+/, $deps_in{$type});
# Eliminate nulls. # Eliminate nulls.
@bug_ids = grep {$_} @bug_ids; @bug_ids = grep {$_} @bug_ids;
# We do Validate up here to make sure all aliases are converted to IDs. # We do this up here to make sure all aliases are converted to IDs.
ValidateBugID($_, $type) foreach @bug_ids; @bug_ids = map { $invocant->check($_, $type)->id } @bug_ids;
my @check_access = @bug_ids; my @check_access = @bug_ids;
# When we're updating a bug, only added or removed bug_ids are # When we're updating a bug, only added or removed bug_ids are
...@@ -1114,11 +1114,10 @@ sub _check_dependencies { ...@@ -1114,11 +1114,10 @@ sub _check_dependencies {
my $user = Bugzilla->user; my $user = Bugzilla->user;
foreach my $modified_id (@check_access) { foreach my $modified_id (@check_access) {
ValidateBugID($modified_id); my $delta_bug = $invocant->check($modified_id);
# Under strict isolation, you can't modify a bug if you can't # Under strict isolation, you can't modify a bug if you can't
# edit it, even if you can see it. # edit it, even if you can see it.
if (Bugzilla->params->{"strict_isolation"}) { if (Bugzilla->params->{"strict_isolation"}) {
my $delta_bug = new Bugzilla::Bug($modified_id);
if (!$user->can_edit_product($delta_bug->{'product_id'})) { if (!$user->can_edit_product($delta_bug->{'product_id'})) {
ThrowUserError("illegal_change_deps", {field => $type}); ThrowUserError("illegal_change_deps", {field => $type});
} }
...@@ -1142,7 +1141,7 @@ sub _check_dup_id { ...@@ -1142,7 +1141,7 @@ sub _check_dup_id {
$dupe_of = trim($dupe_of); $dupe_of = trim($dupe_of);
$dupe_of || ThrowCodeError('undefined_field', { field => 'dup_id' }); $dupe_of || ThrowCodeError('undefined_field', { field => 'dup_id' });
# Make sure we can change the original bug (issue A on bug 96085) # Make sure we can change the original bug (issue A on bug 96085)
ValidateBugID($dupe_of, 'dup_id'); my $dupe_of_bug = $self->check($dupe_of, 'dup_id');
# Make sure a loop isn't created when marking this bug # Make sure a loop isn't created when marking this bug
# as duplicate. # as duplicate.
...@@ -1174,7 +1173,6 @@ sub _check_dup_id { ...@@ -1174,7 +1173,6 @@ sub _check_dup_id {
# Should we add the reporter to the CC list of the new bug? # Should we add the reporter to the CC list of the new bug?
# If he can see the bug... # If he can see the bug...
if ($self->reporter->can_see_bug($dupe_of)) { if ($self->reporter->can_see_bug($dupe_of)) {
my $dupe_of_bug = new Bugzilla::Bug($dupe_of);
# We only add him if he's not the reporter of the other bug. # We only add him if he's not the reporter of the other bug.
$self->{_add_dup_cc} = 1 $self->{_add_dup_cc} = 1
if $dupe_of_bug->reporter->id != $self->reporter->id; if $dupe_of_bug->reporter->id != $self->reporter->id;
...@@ -1199,9 +1197,7 @@ sub _check_dup_id { ...@@ -1199,9 +1197,7 @@ sub _check_dup_id {
my $vars = {}; my $vars = {};
my $template = Bugzilla->template; my $template = Bugzilla->template;
# Ask the user what they want to do about the reporter. # Ask the user what they want to do about the reporter.
$vars->{'cclist_accessible'} = $dbh->selectrow_array( $vars->{'cclist_accessible'} = $dupe_of_bug->cclist_accessible;
q{SELECT cclist_accessible FROM bugs WHERE bug_id = ?},
undef, $dupe_of);
$vars->{'original_bug_id'} = $dupe_of; $vars->{'original_bug_id'} = $dupe_of;
$vars->{'duplicate_bug_id'} = $self->id; $vars->{'duplicate_bug_id'} = $self->id;
print $cgi->header(); print $cgi->header();
......
...@@ -123,7 +123,8 @@ sub get_history { ...@@ -123,7 +123,8 @@ sub get_history {
my @return; my @return;
foreach my $bug_id (@$ids) { foreach my $bug_id (@$ids) {
my %item; my %item;
ValidateBugID($bug_id); my $bug = Bugzilla::Bug->check($bug_id);
$bug_id = $bug->id;
my ($activity) = Bugzilla::Bug::GetBugActivity($bug_id); my ($activity) = Bugzilla::Bug::GetBugActivity($bug_id);
$item{$bug_id} = []; $item{$bug_id} = [];
...@@ -155,7 +156,6 @@ sub get_history { ...@@ -155,7 +156,6 @@ sub get_history {
# alias is returned in case users passes a mixture of ids and aliases # alias is returned in case users passes a mixture of ids and aliases
# then they get to know which bug activity relates to which value # then they get to know which bug activity relates to which value
# they passed # they passed
my $bug = new Bugzilla::Bug($bug_id);
if (Bugzilla->params->{'usebugaliases'}) { if (Bugzilla->params->{'usebugaliases'}) {
$item{alias} = type('string')->value($bug->alias); $item{alias} = type('string')->value($bug->alias);
} }
......
...@@ -165,8 +165,10 @@ sub validateID { ...@@ -165,8 +165,10 @@ sub validateID {
|| ThrowUserError("invalid_attach_id", { attach_id => $attach_id }); || ThrowUserError("invalid_attach_id", { attach_id => $attach_id });
# Make sure the user is authorized to access this attachment's bug. # Make sure the user is authorized to access this attachment's bug.
ValidateBugID($attachment->bug_id); Bugzilla::Bug->check($attachment->bug_id);
if ($attachment->isprivate && $user->id != $attachment->attacher->id && !$user->is_insider) { if ($attachment->isprivate && $user->id != $attachment->attacher->id
&& !$user->is_insider)
{
ThrowUserError('auth_failure', {action => 'access', ThrowUserError('auth_failure', {action => 'access',
object => 'attachment'}); object => 'attachment'});
} }
...@@ -281,9 +283,8 @@ sub diff { ...@@ -281,9 +283,8 @@ sub diff {
# HTML page. # HTML page.
sub viewall { sub viewall {
# Retrieve and validate parameters # Retrieve and validate parameters
my $bugid = $cgi->param('bugid'); my $bug = Bugzilla::Bug->check(scalar $cgi->param('bugid'));
ValidateBugID($bugid); my $bugid = $bug->id;
my $bug = new Bugzilla::Bug($bugid);
my $attachments = Bugzilla::Attachment->get_attachments_by_bug($bugid); my $attachments = Bugzilla::Attachment->get_attachments_by_bug($bugid);
...@@ -301,13 +302,12 @@ sub viewall { ...@@ -301,13 +302,12 @@ sub viewall {
# Display a form for entering a new attachment. # Display a form for entering a new attachment.
sub enter { sub enter {
# Retrieve and validate parameters # Retrieve and validate parameters
my $bugid = $cgi->param('bugid'); my $bug = Bugzilla::Bug->check(scalar $cgi->param('bugid'));
ValidateBugID($bugid); my $bugid = $bug->id;
validateCanChangeBug($bugid); validateCanChangeBug($bugid);
my $dbh = Bugzilla->dbh; my $dbh = Bugzilla->dbh;
my $user = Bugzilla->user; my $user = Bugzilla->user;
my $bug = new Bugzilla::Bug($bugid, $user->id);
# Retrieve the attachments the user can edit from the database and write # Retrieve the attachments the user can edit from the database and write
# them into an array of hashes where each hash represents one attachment. # them into an array of hashes where each hash represents one attachment.
my $canEdit = ""; my $canEdit = "";
...@@ -344,8 +344,8 @@ sub insert { ...@@ -344,8 +344,8 @@ sub insert {
$dbh->bz_start_transaction; $dbh->bz_start_transaction;
# Retrieve and validate parameters # Retrieve and validate parameters
my $bugid = $cgi->param('bugid'); my $bug = Bugzilla::Bug->check(scalar $cgi->param('bugid'));
ValidateBugID($bugid); my $bugid = $bug->id;
validateCanChangeBug($bugid); validateCanChangeBug($bugid);
my ($timestamp) = Bugzilla->dbh->selectrow_array("SELECT NOW()"); my ($timestamp) = Bugzilla->dbh->selectrow_array("SELECT NOW()");
...@@ -373,7 +373,6 @@ sub insert { ...@@ -373,7 +373,6 @@ sub insert {
} }
} }
my $bug = new Bugzilla::Bug($bugid);
my $attachment = my $attachment =
Bugzilla::Attachment->insert_attachment_for_bug(THROW_ERROR, $bug, $user, Bugzilla::Attachment->insert_attachment_for_bug(THROW_ERROR, $bug, $user,
$timestamp, $vars); $timestamp, $vars);
......
...@@ -562,8 +562,8 @@ elsif (($cgi->param('cmdtype') eq "doit") && defined $cgi->param('remtype')) { ...@@ -562,8 +562,8 @@ elsif (($cgi->param('cmdtype') eq "doit") && defined $cgi->param('remtype')) {
my $changes = 0; my $changes = 0;
foreach my $bug_id (split(/[\s,]+/, $cgi->param('bug_ids'))) { foreach my $bug_id (split(/[\s,]+/, $cgi->param('bug_ids'))) {
next unless $bug_id; next unless $bug_id;
ValidateBugID($bug_id); my $bug = Bugzilla::Bug->check($bug_id);
$bug_ids{$bug_id} = $keep_bug; $bug_ids{$bug->id} = $keep_bug;
$changes = 1; $changes = 1;
} }
ThrowUserError('no_bug_ids', ThrowUserError('no_bug_ids',
......
...@@ -41,7 +41,7 @@ use Pod::Usage; ...@@ -41,7 +41,7 @@ use Pod::Usage;
use Encode; use Encode;
use Bugzilla; use Bugzilla;
use Bugzilla::Bug qw(ValidateBugID); use Bugzilla::Bug;
use Bugzilla::Constants qw(USAGE_MODE_EMAIL); use Bugzilla::Constants qw(USAGE_MODE_EMAIL);
use Bugzilla::Error; use Bugzilla::Error;
use Bugzilla::Mailer; use Bugzilla::Mailer;
...@@ -172,8 +172,7 @@ sub process_bug { ...@@ -172,8 +172,7 @@ sub process_bug {
debug_print("Updating Bug $fields{id}..."); debug_print("Updating Bug $fields{id}...");
ValidateBugID($bug_id); my $bug = Bugzilla::Bug->check($bug_id);
my $bug = new Bugzilla::Bug($bug_id);
if ($fields{'bug_status'}) { if ($fields{'bug_status'}) {
$fields{'knob'} = $fields{'bug_status'}; $fields{'knob'} = $fields{'bug_status'};
......
...@@ -350,8 +350,8 @@ my $has_canconfirm = $user->in_group('canconfirm', $product->id); ...@@ -350,8 +350,8 @@ my $has_canconfirm = $user->in_group('canconfirm', $product->id);
$cloned_bug_id = $cgi->param('cloned_bug_id'); $cloned_bug_id = $cgi->param('cloned_bug_id');
if ($cloned_bug_id) { if ($cloned_bug_id) {
ValidateBugID($cloned_bug_id); $cloned_bug = Bugzilla::Bug->check($cloned_bug_id);
$cloned_bug = new Bugzilla::Bug($cloned_bug_id); $cloned_bug_id = $cloned_bug->id;
} }
if (scalar(@{$product->components}) == 1) { if (scalar(@{$product->components}) == 1) {
......
...@@ -112,23 +112,16 @@ sub should_set { ...@@ -112,23 +112,16 @@ sub should_set {
# Create a list of objects for all bugs being modified in this request. # Create a list of objects for all bugs being modified in this request.
my @bug_objects; my @bug_objects;
if (defined $cgi->param('id')) { if (defined $cgi->param('id')) {
my $id = $cgi->param('id'); my $bug = Bugzilla::Bug->check(scalar $cgi->param('id'));
ValidateBugID($id); $cgi->param('id', $bug->id);
push(@bug_objects, $bug);
# Store the validated, and detainted id back in the cgi data, as
# lots of later code will need it, and will obtain it from there
$cgi->param('id', $id);
push(@bug_objects, new Bugzilla::Bug($id));
} else { } else {
my @ids;
foreach my $i ($cgi->param()) { foreach my $i ($cgi->param()) {
if ($i =~ /^id_([1-9][0-9]*)/) { if ($i =~ /^id_([1-9][0-9]*)/) {
my $id = $1; my $id = $1;
ValidateBugID($id); push(@bug_objects, Bugzilla::Bug->check($id));
push(@ids, $id);
} }
} }
@bug_objects = @{Bugzilla::Bug->new_from_list(\@ids)};
} }
# Make sure there are bugs to process. # Make sure there are bugs to process.
......
...@@ -43,17 +43,17 @@ Bugzilla->login(); ...@@ -43,17 +43,17 @@ Bugzilla->login();
# Make sure the bug ID is a positive integer representing an existing # Make sure the bug ID is a positive integer representing an existing
# bug that the user is authorized to access. # bug that the user is authorized to access.
my $bug_id = $cgi->param('id'); my $id = $cgi->param('id');
ValidateBugID($bug_id); my $bug = Bugzilla::Bug->check($id);
############################################################################### ###############################################################################
# End Data/Security Validation # End Data/Security Validation
############################################################################### ###############################################################################
($vars->{'operations'}, $vars->{'incomplete_data'}) = ($vars->{'operations'}, $vars->{'incomplete_data'}) =
Bugzilla::Bug::GetBugActivity($bug_id); Bugzilla::Bug::GetBugActivity($bug->id);
$vars->{'bug'} = new Bugzilla::Bug($bug_id); $vars->{'bug'} = $bug;
print $cgi->header(); print $cgi->header();
......
...@@ -57,10 +57,7 @@ my %marks; ...@@ -57,10 +57,7 @@ my %marks;
if ($single) { if ($single) {
my $id = $cgi->param('id'); my $id = $cgi->param('id');
# Its a bit silly to do the validation twice - that functionality should push @bugs, Bugzilla::Bug->check($id);
# probably move into Bug.pm at some point
ValidateBugID($id);
push @bugs, new Bugzilla::Bug($id);
if (defined $cgi->param('mark')) { if (defined $cgi->param('mark')) {
foreach my $range (split ',', $cgi->param('mark')) { foreach my $range (split ',', $cgi->param('mark')) {
if ($range =~ /^(\d+)-(\d+)$/) { if ($range =~ /^(\d+)-(\d+)$/) {
......
...@@ -135,8 +135,8 @@ if ($display eq 'doall') { ...@@ -135,8 +135,8 @@ if ($display eq 'doall') {
} }
} else { } else {
foreach my $i (split('[\s,]+', $cgi->param('id'))) { foreach my $i (split('[\s,]+', $cgi->param('id'))) {
ValidateBugID($i); my $bug = Bugzilla::Bug->check($i);
$baselist{$i} = 1; $baselist{$bug->id} = 1;
} }
my @stack = keys(%baselist); my @stack = keys(%baselist);
......
...@@ -49,9 +49,8 @@ my $dbh = Bugzilla->switch_to_shadow_db(); ...@@ -49,9 +49,8 @@ my $dbh = Bugzilla->switch_to_shadow_db();
# Make sure the bug ID is a positive integer representing an existing # Make sure the bug ID is a positive integer representing an existing
# bug that the user is authorized to access. # bug that the user is authorized to access.
my $id = $cgi->param('id') || ThrowUserError('improper_bug_id_field_value'); my $bug = Bugzilla::Bug->check(scalar $cgi->param('id'));
ValidateBugID($id); my $id = $bug->id;
my $current_bug = new Bugzilla::Bug($id);
local our $hide_resolved = $cgi->param('hide_resolved') ? 1 : 0; local our $hide_resolved = $cgi->param('hide_resolved') ? 1 : 0;
...@@ -67,7 +66,7 @@ local our $realdepth = 0; ...@@ -67,7 +66,7 @@ local our $realdepth = 0;
# Generate the tree of bugs that this bug depends on and a list of IDs # Generate the tree of bugs that this bug depends on and a list of IDs
# appearing in the tree. # appearing in the tree.
my $dependson_tree = { $id => $current_bug }; my $dependson_tree = { $id => $bug };
my $dependson_ids = {}; my $dependson_ids = {};
GenerateTree($id, "dependson", 1, $dependson_tree, $dependson_ids); GenerateTree($id, "dependson", 1, $dependson_tree, $dependson_ids);
$vars->{'dependson_tree'} = $dependson_tree; $vars->{'dependson_tree'} = $dependson_tree;
...@@ -75,7 +74,7 @@ $vars->{'dependson_ids'} = [keys(%$dependson_ids)]; ...@@ -75,7 +74,7 @@ $vars->{'dependson_ids'} = [keys(%$dependson_ids)];
# Generate the tree of bugs that this bug blocks and a list of IDs # Generate the tree of bugs that this bug blocks and a list of IDs
# appearing in the tree. # appearing in the tree.
my $blocked_tree = { $id => $current_bug }; my $blocked_tree = { $id => $bug };
my $blocked_ids = {}; my $blocked_ids = {};
GenerateTree($id, "blocked", 1, $blocked_tree, $blocked_ids); GenerateTree($id, "blocked", 1, $blocked_tree, $blocked_ids);
$vars->{'blocked_tree'} = $blocked_tree; $vars->{'blocked_tree'} = $blocked_tree;
......
...@@ -251,7 +251,7 @@ $user->in_group(Bugzilla->params->{"timetrackinggroup"}) ...@@ -251,7 +251,7 @@ $user->in_group(Bugzilla->params->{"timetrackinggroup"})
object => "timetracking_summaries"}); object => "timetracking_summaries"});
my @ids = split(",", $cgi->param('id')); my @ids = split(",", $cgi->param('id'));
map { ValidateBugID($_) } @ids; @ids = map { Bugzilla::Bug->check($_)->id } @ids;
scalar(@ids) || ThrowUserError('no_bugs_chosen', {action => 'view'}); scalar(@ids) || ThrowUserError('no_bugs_chosen', {action => 'view'});
my $group_by = $cgi->param('group_by') || "number"; my $group_by = $cgi->param('group_by') || "number";
......
...@@ -67,7 +67,10 @@ else { ...@@ -67,7 +67,10 @@ else {
# Make sure the bug ID is a positive integer representing an existing # Make sure the bug ID is a positive integer representing an existing
# bug that the user is authorized to access. # bug that the user is authorized to access.
ValidateBugID($bug_id) if defined $bug_id; if (defined $bug_id) {
my $bug = Bugzilla::Bug->check($bug_id);
$bug_id = $bug->id;
}
################################################################################ ################################################################################
# End Data/Security Validation # End Data/Security Validation
...@@ -244,14 +247,15 @@ sub record_votes { ...@@ -244,14 +247,15 @@ sub record_votes {
} }
} }
# Call ValidateBugID on each bug ID to make sure it is a positive # Call check() on each bug ID to make sure it is a positive
# integer representing an existing bug that the user is authorized # integer representing an existing bug that the user is authorized
# to access, and make sure the number of votes submitted is also # to access, and make sure the number of votes submitted is also
# a non-negative integer (a series of digits not preceded by a # a non-negative integer (a series of digits not preceded by a
# minus sign). # minus sign).
my %votes; my %votes;
foreach my $id (@buglist) { foreach my $id (@buglist) {
ValidateBugID($id); my $bug = Bugzilla::Bug->check($id);
$id = $bug->id;
$votes{$id} = $cgi->param($id); $votes{$id} = $cgi->param($id);
detaint_natural($votes{$id}) detaint_natural($votes{$id})
|| ThrowUserError("votes_must_be_nonnegative"); || ThrowUserError("votes_must_be_nonnegative");
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment