Bug 466748: [SECURITY] Shared/saved searches can be deleted without user confirmation using predictable URL - Patch by Frédéric Buclin <LpSolit@gmail.com> r=mkanat a=LpSolit

4 files changed, 22 insertions, 10 deletions

diff --git a/buglist.cgi b/buglist.cgi
index f5284439c..de7e56bd7 100755
--- a/buglist.cgi
+++ b/buglist.cgi
@@ -290,7 +290,7 @@ sub LookupNamedQuery {
     $result
        || ThrowUserError("buglist_parameters_required", {'queryname' => $name});
 
-    return $result;
+    return wantarray ? ($result, $id) : $result;
 }
 
 # Inserts a Named Query (a "Saved Search") into the database, or
@@ -448,14 +448,16 @@ $filename =~ s/"/\\"/g; # escape quotes
 # Take appropriate action based on user's request.
 if ($cgi->param('cmdtype') eq "dorem") {  
     if ($cgi->param('remaction') eq "run") {
-        $buffer = LookupNamedQuery(scalar $cgi->param("namedcmd"),
-                                   scalar $cgi->param('sharer_id'));
+        my $query_id;
+        ($buffer, $query_id) = LookupNamedQuery(scalar $cgi->param("namedcmd"),
+                                                scalar $cgi->param('sharer_id'));
         # If this is the user's own query, remember information about it
         # so that it can be modified easily.
         $vars->{'searchname'} = $cgi->param('namedcmd');
         if (!$cgi->param('sharer_id') ||
             $cgi->param('sharer_id') == Bugzilla->user->id) {
             $vars->{'searchtype'} = "saved";
+            $vars->{'search_id'} = $query_id;
         }
         $params = new Bugzilla::CGI($buffer);
         $order = $params->param('order') || $order;
@@ -504,6 +506,10 @@ if ($cgi->param('cmdtype') eq "dorem") {
             # The user has no query of this name. Play along.
         }
         else {
+            # Make sure the user really wants to delete his saved search.
+            my $token = $cgi->param('token');
+            check_hash_token($token, [$query_id, $qname]);
+
             $dbh->do('DELETE FROM namedqueries
                             WHERE id = ?',
                      undef, $query_id);
@@ -557,9 +563,12 @@ elsif (($cgi->param('cmdtype') eq "doit") && defined $cgi->param('remtype')) {
             my %bug_ids;
             my $is_new_name = 0;
             if ($query_name) {
+                my ($query, $query_id) =
+                  LookupNamedQuery($query_name, undef, QUERY_LIST, !THROW_ERROR);
                 # Make sure this name is not already in use by a normal saved search.
-                if (LookupNamedQuery($query_name, undef, QUERY_LIST, !THROW_ERROR)) {
-                    ThrowUserError('query_name_exists', {'name' => $query_name});
+                if ($query) {
+                    ThrowUserError('query_name_exists', {name     => $query_name,
+                                                         query_id => $query_id});
                 }
                 $is_new_name = 1;
             }
diff --git a/template/en/default/account/prefs/saved-searches.html.tmpl b/template/en/default/account/prefs/saved-searches.html.tmpl
index 709cf49c5..280b932ba 100644
--- a/template/en/default/account/prefs/saved-searches.html.tmpl
+++ b/template/en/default/account/prefs/saved-searches.html.tmpl
@@ -108,7 +108,8 @@
             Remove from <a href="editwhines.cgi">whining</a> first
           [% ELSE %]
             <a href="buglist.cgi?cmdtype=dorem&amp;remaction=forget&amp;namedcmd=
-                     [% q.name FILTER url_quote %]">Forget</a>
+                     [% q.name FILTER url_quote %]&amp;token=
+                     [% issue_hash_token([q.id, q.name]) FILTER url_quote %]">Forget</a>
           [% END %]
         </td>
         <td align="center">
diff --git a/template/en/default/global/user-error.html.tmpl b/template/en/default/global/user-error.html.tmpl
index 0606857dd..1efee9a5e 100644
--- a/template/en/default/global/user-error.html.tmpl
+++ b/template/en/default/global/user-error.html.tmpl
@@ -1391,8 +1391,9 @@
     The name <em>[% name FILTER html %]</em> is already used by another
     saved search. You first have to
     <a href="buglist.cgi?cmdtype=dorem&amp;remaction=forget&amp;namedcmd=
-    [%- name FILTER url_quote %]">delete</a> it if you really want to use
-    this name.
+    [%- name FILTER url_quote %]&amp;token=
+    [% issue_hash_token([query_id, name]) FILTER url_quote %]">delete</a>
+    it if you really want to use this name.
 
   [% ELSIF error == "query_name_missing" %]
     [% title = "No Search Name Specified" %]
diff --git a/template/en/default/list/list.html.tmpl b/template/en/default/list/list.html.tmpl
index 4929c416d..a75f1340c 100644
--- a/template/en/default/list/list.html.tmpl
+++ b/template/en/default/list/list.html.tmpl
@@ -228,8 +228,9 @@
       <td valign="middle" nowrap="nowrap" class="bz_query_forget">
         |
         <a href="buglist.cgi?cmdtype=dorem&amp;remaction=forget&amp;namedcmd=
-                [% searchname FILTER url_quote %]">Forget&nbsp;Search&nbsp;'
-                [% searchname FILTER html %]'</a>
+                [% searchname FILTER url_quote %]&amp;token=
+                [% issue_hash_token([search_id, searchname]) FILTER url_quote %]">
+          Forget&nbsp;Search&nbsp;'[% searchname FILTER html %]'</a>
       </td>
     [% ELSE %]
       <td>&nbsp;</td>