# HG changeset patch
# User Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
# Date 2019-02-26 21:50:15
# Node ID 603f5f7c323d1d128aa5d486b60f1172cd254d59
# Parent  fa3e6eda9e7cc352b5500648fe3833e62f77b412

pullrequests: prevent XSS in 'Potential Reviewers' list when first and last names cannot be trusted

If a user first or last name contains javascript, these fields need proper
escaping to avoid XSS attacks.

An example scenario is:
- the malicious user creates a repository. This will cause this user to be
  listed automatically under 'Potential Reviewers' in pull requests.
- another user creates a pull request on that repository and selects the
  suggested reviewer from the 'Potential Reviewers' list.

Reported by Bob Hogg <wombat@rwhogg.site> (thanks!).


Technical note: the other caller of addReviewMember in base.js itself does
_not_ need to be adapted to escape the input values, because the input
values (oData) are _already_ escaped (by the YUI framework).

diff --git a/kallithea/public/js/base.js b/kallithea/public/js/base.js
--- a/kallithea/public/js/base.js
+++ b/kallithea/public/js/base.js
@@ -1313,6 +1313,8 @@ var MentionsAutoComplete = function ($in
         });
 }
 
+// Important: input arguments to addReviewMember should be safe (escaped) for
+// inclusion into HTML.
 var addReviewMember = function(id,fname,lname,nname,gravatar_link,gravatar_size){
     var displayname = nname;
     if ((fname != "") && (lname != "")) {
@@ -1372,6 +1374,8 @@ var PullRequestAutoComplete = function (
             var elLI = aArgs[1]; // reference to the selected LI element
             var oData = aArgs[2]; // object literal of selected item's result data
 
+            // The fields in oData have already been escaped by the YUI
+            // framework. We thus should not add explicit escaping here anymore.
             addReviewMember(oData.id, oData.fname, oData.lname, oData.nname,
                             oData.gravatar_lnk, oData.gravatar_size);
             myAC.getInputEl().value = '';
diff --git a/kallithea/templates/pullrequests/pullrequest_show.html b/kallithea/templates/pullrequests/pullrequest_show.html
--- a/kallithea/templates/pullrequests/pullrequest_show.html
+++ b/kallithea/templates/pullrequests/pullrequest_show.html
@@ -412,7 +412,14 @@ ${self.repo_context_bar('showpullrequest
 
           $('.missing_reviewer').click(function(){
             var $this = $(this);
-            addReviewMember($this.attr('user_id'), $this.attr('fname'), $this.attr('lname'), $this.attr('nname'), $this.attr('gravatar_lnk'), $this.attr('gravatar_size'));
+            addReviewMember(
+                $this.attr('user_id'),
+                $this.attr('fname').html_escape(),
+                $this.attr('lname').html_escape(),
+                $this.attr('nname').html_escape(),
+                $this.attr('gravatar_lnk'),
+                $this.attr('gravatar_size')
+            );
           });
       });
     </script>