From 7ea210f58788f68369aaa8e8b91a3016038c526f Mon Sep 17 00:00:00 2001 From: Byron Jones Date: Thu, 5 Dec 2013 10:04:08 -0800 Subject: Bug 943636: SQL error in quicksearch API when providing just a bug ID --- Bugzilla/Search.pm | 68 +++++++++++++++++++++++++++++++---------- buglist.cgi | 88 +++++++++++++++++++++++------------------------------- 2 files changed, 89 insertions(+), 67 deletions(-) diff --git a/Bugzilla/Search.pm b/Bugzilla/Search.pm index bda57716a..0c52553bb 100644 --- a/Bugzilla/Search.pm +++ b/Bugzilla/Search.pm @@ -38,7 +38,6 @@ use base qw(Exporter); @Bugzilla::Search::EXPORT = qw( IsValidQueryType split_order_term - translate_old_column ); use Bugzilla::Error; @@ -598,10 +597,10 @@ sub COLUMNS { ); # Backward-compatibility for old field names. Goes new_name => old_name. - # These are here and not in translate_old_column because the rest of the + # These are here and not in _translate_old_column because the rest of the # code actually still uses the old names, while the fielddefs table uses # the new names (which is not the case for the fields handled by - # translate_old_column). + # _translate_old_column). my %old_names = ( creation_ts => 'opendate', delta_ts => 'changeddate', @@ -744,8 +743,8 @@ sub data { my $all_in_bugs_table = 1; foreach my $field (@orig_fields) { next if $self->COLUMNS->{$field}->{name} =~ /^bugs\.\w+$/; - $all_in_bugs_table = 0; $self->{fields} = ['bug_id']; + $all_in_bugs_table = 0; last; } @@ -944,6 +943,21 @@ sub boolean_charts_to_custom_search { } } +sub invalid_order_columns { + my ($self) = @_; + my @invalid_columns; + foreach my $order ($self->_input_order) { + next if defined $self->_validate_order_column($order); + push(@invalid_columns, $order); + } + return \@invalid_columns; +} + +sub order { + my ($self) = @_; + return $self->_valid_order; +} + ###################### # Internal Accessors # ###################### @@ -1009,7 +1023,7 @@ sub _extra_columns { my ($self) = @_; # Everything that's going to be in the ORDER BY must also be # in the SELECT. - push(@{ $self->{extra_columns} }, $self->_input_order_columns); + push(@{ $self->{extra_columns} }, $self->_valid_order_columns); return @{ $self->{extra_columns} }; } @@ -1079,10 +1093,32 @@ sub _sql_select { # The "order" that was requested by the consumer, exactly as it was # requested. sub _input_order { @{ $_[0]->{'order'} || [] } } -# The input order with just the column names, and no ASC or DESC. -sub _input_order_columns { +# Requested order with invalid values removed and old names translated +sub _valid_order { + my ($self) = @_; + return map { ($self->_validate_order_column($_)) } $self->_input_order; +} +# The valid order with just the column names, and no ASC or DESC. +sub _valid_order_columns { my ($self) = @_; - return map { (split_order_term($_))[0] } $self->_input_order; + return map { (split_order_term($_))[0] } $self->_valid_order; +} + +sub _validate_order_column { + my ($self, $order_item) = @_; + + # Translate old column names + my ($field, $direction) = split_order_term($order_item); + $field = $self->_translate_old_column($field); + + # Only accept valid columns + return if (!exists $self->COLUMNS->{$field}); + + # Relevance column can be used only with one or more fulltext searches + return if ($field eq 'relevance' && !$self->COLUMNS->{$field}->{name}); + + $direction = " $direction" if $direction; + return "$field$direction"; } # A hashref that describes all the special stuff that has to be done @@ -1114,7 +1150,7 @@ sub _sql_order_by { my ($self) = @_; if (!$self->{sql_order_by}) { my @order_by = map { $self->_translate_order_by_column($_) } - $self->_input_order; + $self->_valid_order; $self->{sql_order_by} = \@order_by; } return @{ $self->{sql_order_by} }; @@ -1259,7 +1295,7 @@ sub _select_order_joins { my @column_join = $self->_column_join($field); push(@joins, @column_join); } - foreach my $field ($self->_input_order_columns) { + foreach my $field ($self->_valid_order_columns) { my $join_info = $self->_special_order->{$field}->{join}; if ($join_info) { # Don't let callers modify SPECIAL_ORDER. @@ -1417,7 +1453,7 @@ sub _sql_group_by { # And all items from ORDER BY must be in the GROUP BY. The above loop # doesn't catch items that were put into the ORDER BY from SPECIAL_ORDER. - foreach my $column ($self->_input_order_columns) { + foreach my $column ($self->_valid_order_columns) { my $special_order = $self->_special_order->{$column}->{order}; next if !$special_order; push(@extra_group_by, @$special_order); @@ -3388,8 +3424,8 @@ sub split_order_term { # Used to translate old SQL fragments from buglist.cgi's "order" argument # into our modern field IDs. -sub translate_old_column { - my ($column) = @_; +sub _translate_old_column { + my ($self, $column) = @_; # All old SQL fragments have a period in them somewhere. return $column if $column !~ /\./; @@ -3403,9 +3439,9 @@ sub translate_old_column { # If it doesn't match the regexps above, check to see if the old # SQL fragment matches the SQL of an existing column - foreach my $key (%{ COLUMNS() }) { - next unless exists COLUMNS->{$key}->{name}; - return $key if COLUMNS->{$key}->{name} eq $column; + foreach my $key (%{ $self->COLUMNS }) { + next unless exists $self->COLUMNS->{$key}->{name}; + return $key if $self->COLUMNS->{$key}->{name} eq $column; } return $column; diff --git a/buglist.cgi b/buglist.cgi index 9c7281822..f9cda72ec 100755 --- a/buglist.cgi +++ b/buglist.cgi @@ -56,18 +56,19 @@ my $cgi = Bugzilla->cgi; my $dbh = Bugzilla->dbh; my $template = Bugzilla->template; my $vars = {}; -my $buffer = $cgi->query_string(); # We have to check the login here to get the correct footer if an error is # thrown and to prevent a logged out user to use QuickSearch if 'requirelogin' # is turned 'on'. my $user = Bugzilla->login(); +$cgi->redirect_search_url(); + +my $buffer = $cgi->query_string(); if (length($buffer) == 0) { ThrowUserError("buglist_parameters_required"); } -$cgi->redirect_search_url(); # Determine whether this is a quicksearch query. my $searchstring = $cgi->param('quicksearch'); @@ -330,6 +331,7 @@ sub _close_standby_message { } } + ################################################################################ # Command Execution ################################################################################ @@ -373,7 +375,7 @@ if ($cmdtype eq "dorem") { # so that it can be modified easily. $vars->{'searchname'} = $cgi->param('namedcmd'); if (!$cgi->param('sharer_id') || - $cgi->param('sharer_id') == Bugzilla->user->id) { + $cgi->param('sharer_id') == $user->id) { $vars->{'searchtype'} = "saved"; $vars->{'search_id'} = $query_id; } @@ -690,69 +692,39 @@ if (!$order || $order =~ /^reuse/i) { } } +my @order_columns; if ($order) { # Convert the value of the "order" form field into a list of columns # by which to sort the results. ORDER: for ($order) { /^Bug Number$/ && do { - $order = "bug_id"; + @order_columns = ("bug_id"); last ORDER; }; /^Importance$/ && do { - $order = "priority,bug_severity"; + @order_columns = ("priority", "bug_severity"); last ORDER; }; /^Assignee$/ && do { - $order = "assigned_to,bug_status,priority,bug_id"; + @order_columns = ("assigned_to", "bug_status", "priority", + "bug_id"); last ORDER; }; /^Last Changed$/ && do { - $order = "changeddate,bug_status,priority,assigned_to,bug_id"; + @order_columns = ("changeddate", "bug_status", "priority", + "assigned_to", "bug_id"); last ORDER; }; do { - my (@order, @invalid_fragments); - - # A custom list of columns. Make sure each column is valid. - foreach my $fragment (split(/,/, $order)) { - $fragment = trim($fragment); - next unless $fragment; - my ($column_name, $direction) = split_order_term($fragment); - $column_name = translate_old_column($column_name); - - # Special handlings for certain columns - next if $column_name eq 'relevance' && !$fulltext; - - if (exists $columns->{$column_name}) { - $direction = " $direction" if $direction; - push(@order, "$column_name$direction"); - } - else { - push(@invalid_fragments, $fragment); - } - } - if (scalar @invalid_fragments) { - $vars->{'message'} = 'invalid_column_name'; - $vars->{'invalid_fragments'} = \@invalid_fragments; - } - - $order = join(",", @order); - # Now that we have checked that all columns in the order are valid, - # detaint the order string. - trick_taint($order) if $order; + # A custom list of columns. Bugzilla::Search will validate items. + @order_columns = split(/\s*,\s*/, $order); }; } } -if (!$order) { +if (!scalar @order_columns) { # DEFAULT - $order = "bug_status,priority,assigned_to,bug_id"; -} - -my @orderstrings = split(/,\s*/, $order); - -if ($fulltext and grep { /^relevance/ } @orderstrings) { - $vars->{'message'} = 'buglist_sorted_by_relevance' + @order_columns = ("bug_status", "priority", "assigned_to", "bug_id"); } # In the HTML interface, by default, we limit the returned results, @@ -766,9 +738,20 @@ if ($format->{'extension'} eq 'html' && !defined $params->param('limit')) { # Generate the basic SQL query that will be used to generate the bug list. my $search = new Bugzilla::Search('fields' => \@selectcolumns, 'params' => scalar $params->Vars, - 'order' => \@orderstrings, + 'order' => \@order_columns, 'sharer' => $sharer_id); +$order = join(',', $search->order); + +if (scalar @{$search->invalid_order_columns}) { + $vars->{'message'} = 'invalid_column_name'; + $vars->{'invalid_fragments'} = $search->invalid_order_columns; +} + +if ($fulltext and grep { /^relevance/ } $search->order) { + $vars->{'message'} = 'buglist_sorted_by_relevance' +} + # We don't want saved searches and other buglist things to save # our default limit. $params->delete('limit') if $vars->{'default_limited'}; @@ -974,10 +957,10 @@ $vars->{'order'} = $order; $vars->{'caneditbugs'} = 1; $vars->{'time_info'} = $time_info; -if (!Bugzilla->user->in_group('editbugs')) { +if (!$user->in_group('editbugs')) { foreach my $product (keys %$bugproducts) { my $prod = new Bugzilla::Product({name => $product}); - if (!Bugzilla->user->in_group('editbugs', $prod->id)) { + if (!$user->in_group('editbugs', $prod->id)) { $vars->{'caneditbugs'} = 0; last; } @@ -985,7 +968,7 @@ if (!Bugzilla->user->in_group('editbugs')) { } my @bugowners = keys %$bugowners; -if (scalar(@bugowners) > 1 && Bugzilla->user->in_group('editbugs')) { +if (scalar(@bugowners) > 1 && $user->in_group('editbugs')) { my $suffix = Bugzilla->params->{'emailsuffix'}; map(s/$/$suffix/, @bugowners) if $suffix; my $bugowners = join(",", @bugowners); @@ -996,7 +979,10 @@ if (scalar(@bugowners) > 1 && Bugzilla->user->in_group('editbugs')) { # the list more compact. $vars->{'splitheader'} = $cgi->cookie('SPLITHEADER') ? 1 : 0; -$vars->{'quip'} = GetQuip(); +if ($user->settings->{'display_quips'}->{'value'} eq 'on') { + $vars->{'quip'} = GetQuip(); +} + $vars->{'currenttime'} = localtime(time()); # See if there's only one product in all the results (or only one product @@ -1014,7 +1000,7 @@ elsif (my @product_input = $cgi->param('product')) { } # We only want the template to use it if the user can actually # enter bugs against it. -if ($one_product && Bugzilla->user->can_enter_product($one_product)) { +if ($one_product && $user->can_enter_product($one_product)) { $vars->{'one_product'} = $one_product; } @@ -1034,7 +1020,7 @@ if ($dotweak && scalar @bugs) { $vars->{'token'} = issue_session_token('buglist_mass_change'); Bugzilla->switch_to_shadow_db(); - $vars->{'products'} = Bugzilla->user->get_enterable_products; + $vars->{'products'} = $user->get_enterable_products; $vars->{'platforms'} = get_legal_field_values('rep_platform'); $vars->{'op_sys'} = get_legal_field_values('op_sys'); $vars->{'priorities'} = get_legal_field_values('priority'); -- cgit v1.2.3-24-g4f1b