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

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, enhancement)

Description

[Max Kanat-Alexander]

Now that we've created Bugzilla::Bug->check in bug 44609, we should use it everywhere.

Comment 1

[Max Kanat-Alexander]

I've discovered that there's at least one place that's too complicated to modify in this patch (_check_dependencies), so I'll save it for another patch.

Comment 2

[Max Kanat-Alexander]

Actually, I decided to just keep it simple and do things the simple way instead of trying to re-work things to be efficient. We can do that in future bugs if we want. So now I was able to eliminate all calls to ValidateBugID except one (the one inside check()) which I will handle in another bug.

Comment 3

[Max Kanat-Alexander]

Attachment: v1

As I stated in the above comment, I know that I'm not always doing things in the most efficient way possible.

This passes runtests, but I haven't checked it much beyond that. It should be fine, though--the code changes are really simple.

If you're wondering why I do a lot of changes of $bug_id to $bug->id, it's because ValidateBugID used to convert aliases to IDs, but now we're calling check() which doesn't (or at least shouldn't) modify the caller's variables.

Comment 4

[Max Kanat-Alexander]

Attachment: Actual v1

This patch requires the patch from bug 440609. The previous patch accidentally included that patch.

Comment 5

[Max Kanat-Alexander]

Attachment: v2

A few pieces were missing from v1.

Comment 6

[Frédéric Buclin]

Please also remove ValidateBugID from get_history() in WebService/Bug.pm.

Comment 7

[Max Kanat-Alexander]

Attachment: v3

Okay, here we go!

Comment 8

[Frédéric Buclin]

Comment on attachment 327062 [details] [diff] [review]
v3

>@@ -344,11 +342,10 @@
>-    ValidateBugID($bugid);
>-    validateCanChangeBug($bugid);
>+    my $bug = Bugzilla::Bug->check($bugid);
>+    validateCanChangeBug($bug->id);
>     my ($timestamp) = Bugzilla->dbh->selectrow_array("SELECT NOW()");
> 
>-    my $bug = new Bugzilla::Bug($bugid);

Due to the recent checkin of bug 422691, you will have to fix the bitrot here (easy to fix).



>Index: showdependencytree.cgi

>-my $dependson_tree = { $id => $current_bug };
>+my $dependson_tree = { $bug->id => $current_bug };

This file doesn't compile. $bug, used here and at several places below, is not defined. You probably meant $current_bug.


Both comments must be fixed, but I'm still reviewing the remaining part of the patch. More comments later.

Comment 9

[Max Kanat-Alexander]

Attachment: v4

Okay, I fixed the bitrot and also made sure everything compiles.

Comment 10

[Max Kanat-Alexander]

Attachment: v5

Use $id instead of $bug->id in showdependencytree.

Comment 11

[Max Kanat-Alexander]

Attachment: v6

Fixed $cgi->param in process_bug.

Comment 12

[Frédéric Buclin]

Comment on attachment 327270 [details] [diff] [review]
v6

>Index: attachment.cgi

> sub viewall {

>-    ValidateBugID($bugid);
>-    my $bug = new Bugzilla::Bug($bugid);
>+    my $bug = Bugzilla::Bug->check($bugid);
> 
>     my $attachments = Bugzilla::Attachment->get_attachments_by_bug($bugid);

get_attachments_by_bug() uses $bugid, which is no longer validated (taint error).


> sub enter {

>-  ValidateBugID($bugid);
>+  my $bug = Bugzilla::Bug->check($bugid);

Same here, $bugid is used in the SQL query with no validation (taint error).



>Index: buglist.cgi

>             foreach my $bug_id (split(/[\s,]+/, $cgi->param('bug_ids'))) {
>                 next unless $bug_id;
>-                ValidateBugID($bug_id);
>+                Bugzilla::Bug->check($bug_id);
>                 $bug_ids{$bug_id} = $keep_bug;

If I pass an alias, calling the tag generates a SQL crash, because $bug_id still contains a string.



>Index: enter_bug.cgi

> if ($cloned_bug_id) {
>-    ValidateBugID($cloned_bug_id);
>-    $cloned_bug = new Bugzilla::Bug($cloned_bug_id);
>+    $cloned_bug = Bugzilla::Bug->check($cloned_bug_id);

$cloned_bug_id is used in many places, but is no longer validated.



>Index: showdependencytree.cgi

>-my $id = $cgi->param('id') || ThrowUserError('improper_bug_id_field_value');
>-ValidateBugID($id);
>-my $current_bug = new Bugzilla::Bug($id);
>+my $bug = Bugzilla::Bug->check(scalar $cgi->param('id'));

If no ID is passed, which happens when you call this CGI file directly, you now get:

Undef to trick_taint at Bugzilla/Util.pm line 67

You must keep the $cgi->param('id') || ThrowUserError('improper_bug_id_field_value') check.



>Index: votes.cgi

>     foreach my $id (@buglist) {
>-      ValidateBugID($id);
>-      $votes{$id} = $cgi->param($id);
>-      detaint_natural($votes{$id}) 
>+      my $bug = Bugzilla::Bug->check($id);
>+      $votes{$bug->id} = $cgi->param($bug->id);
>+      detaint_natural($votes{$bug->id}) 

Nit: wouldn't it make more sense to write $id = $bug->id rather than repeating $bug->id everytime?

Comment 13

[Max Kanat-Alexander]

Attachment: v7

Okay, fixed everything, except the improper_bug_id_field_value thing, because that is actually handled by the next patch in this dependency chain. (And if it isn't, we should handle it there.)

Comment 14

[Frédéric Buclin]

Comment on attachment 327283 [details] [diff] [review]
v7

Seems to work fine. r=LpSolit

Comment 15

[Max Kanat-Alexander]

Checking in attachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.146; previous revision: 1.145
done
Checking in buglist.cgi;
/cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v  <--  buglist.cgi
new revision: 1.376; previous revision: 1.375
done
Checking in email_in.pl;
/cvsroot/mozilla/webtools/bugzilla/email_in.pl,v  <--  email_in.pl
new revision: 1.20; previous revision: 1.19
done
Checking in enter_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/enter_bug.cgi,v  <--  enter_bug.cgi
new revision: 1.161; previous revision: 1.160
done
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.412; previous revision: 1.411
done
Checking in show_activity.cgi;
/cvsroot/mozilla/webtools/bugzilla/show_activity.cgi,v  <--  show_activity.cgi
new revision: 1.25; previous revision: 1.24
done
Checking in show_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/show_bug.cgi,v  <--  show_bug.cgi
new revision: 1.54; previous revision: 1.53
done
Checking in showdependencygraph.cgi;
/cvsroot/mozilla/webtools/bugzilla/showdependencygraph.cgi,v  <--  showdependencygraph.cgi
new revision: 1.66; previous revision: 1.65
done
Checking in showdependencytree.cgi;
/cvsroot/mozilla/webtools/bugzilla/showdependencytree.cgi,v  <--  showdependencytree.cgi
new revision: 1.53; previous revision: 1.52
done
Checking in summarize_time.cgi;
/cvsroot/mozilla/webtools/bugzilla/summarize_time.cgi,v  <--  summarize_time.cgi
new revision: 1.24; previous revision: 1.23
done
Checking in votes.cgi;
/cvsroot/mozilla/webtools/bugzilla/votes.cgi,v  <--  votes.cgi
new revision: 1.56; previous revision: 1.55
done
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.245; previous revision: 1.244
done
Checking in Bugzilla/WebService/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService/Bug.pm,v  <--  Bug.pm
new revision: 1.14; previous revision: 1.13
done