[webkit-reviews] review denied: [Bug 68045] Generate WebKit team's page out of committers.py : [Attachment 107439] Updated per Leandro's comment

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has denied Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 68045: Generate WebKit team's page out of committers.py
https://bugs.webkit.org/show_bug.cgi?id=68045

Attachment 107439: Updated per Leandro's comment
https://bugs.webkit.org/attachment.cgi?id=107439&action=review

------- Additional Comments from Daniel Bates <dbates at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=107439&action=review


This patch has at least one cross-site scripting vulnerability. In particular,
when the page is passed the search string "?annotate" the script includes
untrusted content from <http://trac.webkit.org>. See my remarks for
annotateForContributor() for more details.

> Websites/webkit.org/team.html:47
> +	   var contributorLine =
/^\s+(Reviewer|Committer|Contributor)\((.+)\),?$/;

It seems sufficient to define this regular expression once instead of on each
iteration of this loop. I suggest hoisting this declaration outside of this
for-loop.

> Websites/webkit.org/team.html:50
> +		   var nameEmailsNicks = eval('[' +
match[2].replace(/^u"/,'"').replace('None, ', 'null, ') + ']');

I suggest adding a comment above this line to explain that we massage the
Python code extracted from committer.py into a JavaScript-compatible form so as
to utilize the JavaScript parser to actually parse the committer.py data and
build a data structure to facilitate extracting the
Committer/Contributor/Reviewer details.

On another note, the usage of eval() here is OK. Clearly the usage of eval()
here assumes that people with Subversion credentials aren't malicious and that
such credentials won't be stolen. That being said, if an attacker controls such
credentials then they control the repository, including the web site.

> Websites/webkit.org/team.html:150
> +    function annotateForContributor(contributor) {
> +	   var listItem = findListChildForContributor(contributor);
> +	   if (!listItem) {
> +	       var listElement = document.getElementById(contributor.kind +
's');
> +	       var listItem = document.createElement('li');
> +	       listElement.appendChild(listItem);
> +	       listItem.innerHTML = '<span style="background-color:red;">' +
createMarkupForContributor(contributor) + '</span>';
> +	   } else {
> +	       var nicksInContainer = nicksInListItem(listItem);
> +	       if (contributor.nicks) {
> +		   if (nicksInContainer)
> +		       nicksDiff = contributor.nicks.filter(function (nick) {
return nicksInContainer.indexOf(nick) < 0; })
> +		   if (!nicksInContainer || nicksDiff.length)
> +		       listItem.innerHTML += ' <span
style="background-color:red;">(' + nicksDiff + ')</span>';
> +	       }
> +
> +	       var affiliationContainer =
listItem.getElementsByClassName('affiliation')[0];
> +	       var affiliation = formatAffiliation(contributor);
> +	       if (affiliation && (!affiliationContainer ||
affiliationContainer.textContent != affiliation))
> +		   listItem.innerHTML += ' <span
style="background-color:red;"><em>' + affiliation + '</em></span>';
> +	   }
> +    }
> +

Content from <http://trac.webkit.org> should be considered untrusted since
anyone can register for a trac account at
<https://trac.webkit.org/auth/register/> and edit any wiki page. In particular,
an attacker can edit the wiki page <http://trac.webkit.org/wiki/WebKit%20Team>.


Notice that the value of innerHTML is parsed for markup (*) and that
annotateForContributor() modifies the DOM via innerHTML with content from
<http://trac.webkit.org/wiki/WebKit%20Team> without escaping it.

Without loss of generality, suppose the following markup was appended to the
Reviewers section of <http://trac.webkit.org/wiki/WebKit%20Team>:

 * '''<a href='#' onmouseover='alert(1)'>Bob</a>''' (bob) ''ACompany''

Then we visit <http://www.webkit.org/team.html?annotate>. Notice that
annotateForContributor() will ultimately add a Reviewer entry for Bob. Hovering
over the name Bob will display a JavaScript alert with the message "1" because
the HTML markup embedded in the Trac formatting syntax is written verbatim to
the page.

> Websites/webkit.org/team.html:153
> +	   if (this.status != 200)

Can we strengthen the inequality to "!=="?

> Websites/webkit.org/team.html:177
> +    xhr.onerror = function () { alert('Could not obtain
http://trac.webkit.org/wiki/WebKit%20Team'); };
> +    xhr.open('GET', 'http://trac.webkit.org/wiki/WebKit%20Team?format=txt');


The URL "http://trac.webkit.org/wiki/WebKit%20Team" is referenced twice. I
suggest defining a variable, say webkitTeamWikiURL, for this value so that we
can reference this URL in both XMLHttpRequest.onerror handler and
XMLHttpRequest.open(). We may also want to consider making this variable global
and having it at the top of this script to both increase its visibility and
facilitate changes.

> Websites/webkit.org/team.html:183
> +    if (this.status != 200)

Can we strengthen the inequality to "!=="?

> Websites/webkit.org/team.html:195
> +xhr.open('GET',
'http://svn.webkit.org/repository/webkit/trunk/Tools/Scripts/webkitpy/common/co
nfig/committers.py');

For your consideration, I suggest defining a global variable for the SVN URL
and putting this variable at the top of this script as a means to centralize
this configuration data and facilitate changes.