Fix code injection bugs (XSS) in HTML views

- Replace unnecessary uses of innerHTML with textContent.
- Prefer using the DOM over text interpolation.
- Use the appropriate library and browser functions to escape HTML.
- Rename variables to distinguish between HTML and plain text.
- Don't directly send HTML templates to the git CLI, safely
  escape and interpolate the values after parsing CLI output.

Author: nanotech

Classes/Views/GLFileView.h

@@ -25,7 +25,7 @@
 - (void)showFile;
 - (void)didLoad;
 - (NSString *)parseBlame:(NSString *)txt;
-- (NSString *)parseHTML:(NSString *)txt;
+- (NSString *)escapeHTML:(NSString *)txt;
 
 @property NSMutableArray *groups;
 @property NSString *logFormat;

Classes/Views/GLFileView.m

@@ -36,11 +36,6 @@ @implementation GLFileView
 
 - (void) awakeFromNib
 {
-	NSString *formatFile = [[NSBundle mainBundle] pathForResource:@"format" ofType:@"html" inDirectory:@"html/views/log"];
-	if(formatFile!=nil)
-		logFormat=[NSString stringWithContentsOfURL:[NSURL fileURLWithPath:formatFile] encoding:NSUTF8StringEncoding error:nil];
-	
-	
 	startFile = GROUP_ID_FILEVIEW;
 	//repository = historyController.repository;
 	[super awakeFromNib];
@@ -87,11 +82,11 @@ - (void) showFile
 
 		NSString *fileTxt = @"";
 		if([startFile isEqualToString:GROUP_ID_FILEVIEW])
-			fileTxt = [self parseHTML:[file textContents]];
+			fileTxt = [self escapeHTML:[file textContents]];
 		else if([startFile isEqualToString:GROUP_ID_BLAME])
 			fileTxt = [self parseBlame:[file blame]];
 		else if([startFile isEqualToString:GROUP_ID_LOG])
-			fileTxt = [file log:logFormat];
+			fileTxt = [self htmlHistory:file];
 
 		id script = self.view.windowScriptObject;
 		NSString *filePath = [file fullPath];
@@ -179,18 +174,15 @@ - (void)closeView
 	[super closeView];
 }
 
-- (NSString *) parseHTML:(NSString *)txt
+- (NSString *) escapeHTML:(NSString *)txt
 {
-	txt=[txt stringByReplacingOccurrencesOfString:@"&" withString:@"&"];
-	txt=[txt stringByReplacingOccurrencesOfString:@"<" withString:@"&lt;"];
-	txt=[txt stringByReplacingOccurrencesOfString:@">" withString:@"&gt;"];
-	
-	return txt;
+	CFStringRef escaped = CFXMLCreateStringByEscapingEntities(NULL, (__bridge CFStringRef)txt, NULL);
+	return (__bridge_transfer NSString *)escaped;
 }
 
 - (NSString *) parseBlame:(NSString *)txt
 {
-	txt=[self parseHTML:txt];
+	txt=[self escapeHTML:txt];
 
 	NSArray *lines = [txt componentsSeparatedByString:@"\n"];
 	NSString *line;
@@ -230,7 +222,7 @@ - (NSString *) parseBlame:(NSString *)txt
 				if([summary length]>30){
 					truncate_s=[summary substringWithRange:trunc];
 				}
-				NSString *block=[NSString stringWithFormat:@"<td><p class='author'><a href='' onclick='selectCommit(\"%@\"); return false;'>%@</a> %@</p><p class='summary'>%@</p></td>\n<td>\n",commitID,truncate_c,truncate_a,truncate_s];
+				NSString *block=[NSString stringWithFormat:@"<td><p class='author'><a class='commit-link' href='#' data-commit-id='%@'>%@</a> %@</p><p class='summary'>%@</p></td>\n<td>\n",commitID,truncate_c,truncate_a,truncate_s];
 				[headers setObject:block forKey:[header objectAtIndex:0]];
 			}
 			[res appendString:[headers objectForKey:[header objectAtIndex:0]]];
@@ -268,6 +260,38 @@ - (NSString *) parseBlame:(NSString *)txt
 	return (NSString *)res;
 }
 
+- (NSString *) htmlHistory:(PBGitTree *)file
+{
+	// \0 can't be passed as a shell argument, so use a sufficiently long random seperator instead
+	NSString *seperator = [[NSUUID UUID] UUIDString];
+	NSString *commitTerminator = [[NSUUID UUID] UUIDString];
+	NSString *logFormat = [[@"%h,%s,%aN,%ar,%H" stringByReplacingOccurrencesOfString:@"," withString:seperator] stringByAppendingString:commitTerminator];
+	NSString *output = [file log:logFormat];
+	NSArray<NSString *> *rawCommits = [output componentsSeparatedByString:commitTerminator];
+	rawCommits = [rawCommits subarrayWithRange:(NSRange){0, rawCommits.count - 1}];
+
+	NSCharacterSet *whitespaceSet = [NSCharacterSet whitespaceCharacterSet];
+
+	NSMutableString *html = [NSMutableString string];
+	for (NSString *rawCommit in rawCommits) {
+		NSArray<NSString *> *parts = [rawCommit componentsSeparatedByString:seperator];
+		[html appendFormat:
+		 @"<div id='%@' class='commit'>"
+			 "<p class='title'>%@</p>"
+			 "<table>"
+				 "<tr><td>Author:</td><td>%@</td></tr>"
+				 "<tr><td>Date:</td><td>%@</td></tr>"
+		 		 "<tr><td>Commit:</td><td><a class='commit-link' href='#'>%@</a></td></tr>"
+			 "</table>"
+		 "</div>",
+		 [self escapeHTML:[parts[0] stringByTrimmingCharactersInSet:whitespaceSet]], // trim leading newline from split
+		 [self escapeHTML:parts[1]],
+		 [self escapeHTML:parts[2]],
+		 [self escapeHTML:parts[3]],
+		 [self escapeHTML:parts[4]]];
+	}
+	return html;
+}
 
 
 #pragma mark NSSplitView delegate methods
@@ -334,6 +358,5 @@ - (void)restoreSplitViewPositiion
 
 
 @synthesize groups;
-@synthesize logFormat;
 
 @end

Resources/html/lib/GitX.js

@@ -9,13 +9,13 @@ function $(element) {
 	return document.getElementById(element);
 }
 
-String.prototype.escapeHTML = function() {
-  return this.replace(/&/g,'&amp;').replace(/</g,'&lt;').replace(/>/g,'&gt;');
-};
-
-String.prototype.unEscapeHTML = function() {
-  return this.replace(/&amp;/g,'&').replace(/&lt;/g,'<').replace(/&gt;/g,'>');
-};
+String.prototype.escapeHTML = (function() {
+    var div = document.createElement("div");
+    return function() {
+        div.textContent = this;
+        return div.innerHTML;
+    };
+})();
 
 Element.prototype.toggleDisplay = function() {
 	if (this.style.display != "")
@@ -33,10 +33,10 @@ Array.prototype.indexOf = function(item, i) {
   return -1;
 };
 
-var notify = function(text, state) {
+var notify = function(html, state) {
 	var n = $("notification");
 	n.style.display = "";
-	$("notification_message").innerHTML = text;
+	$("notification_message").innerHTML = html;
 
 	// Change color
 	if (!state) { // Busy
@@ -55,3 +55,13 @@ var notify = function(text, state) {
 var hideNotification = function() {
 	$("notification").style.display = "none";
 }
+
+var bindCommitSelectionLinks = function(el) {
+	var links = el.getElementsByClassName("commit-link");
+	for (var i = 0, n = links.length; i < n; ++i) {
+		links[i].addEventListener("click", function(e) {
+			e.preventDefault();
+			selectCommit(this.dataset.commitId || this.innerHTML);
+		}, false);
+	}
+};

Resources/html/lib/diffHighlighter.js

@@ -86,12 +86,12 @@ var highlightDiff = function(diff, element, callbacks) {
 
 		// Show file list header
 		finalContent += '<div class="file" id="file_index_' + (file_index - 1) + '">';
-		finalContent += '<div id="title_' + title + '" class="expanded fileHeader"><a href="javascript:toggleDiff(\'' + title + '\');">' + title + '</a></div>';
+		finalContent += '<div id="title_' + title + '" class="expanded fileHeader"><a href="">' + title + '</a></div>';
 
 		if (binary) {
 			// diffContent is assumed to be empty for binary files
-			if (callbacks["binaryFile"]) {
-				finalContent += callbacks["binaryFile"](binaryname);
+			if (callbacks.binaryFileHTML) {
+				finalContent += callbacks.binaryFileHTML(binaryname);
 			} else {
 				finalContent += '<div id="content_' + title + '">Binary file differs</div>';
 			}
@@ -254,6 +254,20 @@ var highlightDiff = function(diff, element, callbacks) {
 	// This takes about 7ms
 	element.innerHTML = finalContent;
 
+	var fileHeaders = element.querySelectorAll(".fileHeader a");
+	for (var i = 0, n = fileHeaders.length; i < n; ++i) {
+		fileHeaders[i].addEventListener("click", function(e) {
+			e.preventDefault();
+			toggleDiff(this.innerHTML);
+		});
+	}
+	if (callbacks.binaryFileClass && callbacks.binaryFileOnClick) {
+		var binaryFiles = element.getElementsByClassName(callbacks.binaryFileClass);
+		for (var i = 0, n = binaryFiles.length; i < n; ++i) {
+			binaryFiles[i].addEventListener("click", callbacks.binaryFileOnClick);
+		}
+	}
+
 	// TODO: Replace this with a performance pref call
 	if (false)
 		Controller.log_("Total time:" + (new Date().getTime() - start));
@@ -360,12 +374,7 @@ var inlinediff = (function () {
   };
 
   function escape(s) {
-      var n = s;
-      n = n.replace(/&/g, "&amp;");
-      n = n.replace(/</g, "&lt;");
-      n = n.replace(/>/g, "&gt;");
-      n = n.replace(/"/g, "&quot;");
-      return n;
+      return s.escapeHTML();
   }
 
   function diffString( o, n ) {

Resources/html/views/blame/blame.js

@@ -1,6 +1,8 @@
-var showFile = function(txt) {
-	$("txt").style.display = "";
-	$("txt").innerHTML="<pre>"+txt+"</pre>";
+var showFile = function(html) {
+	var el = $("txt");
+	el.style.display = "";
+	el.innerHTML = "<pre>" + html + "</pre>";
+	bindCommitSelectionLinks(el);
 
 	SyntaxHighlighter.defaults['toolbar'] = false;
 
@@ -10,4 +12,4 @@ var showFile = function(txt) {
 
 var selectCommit = function(a) {
 	Controller.selectCommit_(a);
-}
+}

Resources/html/views/commit/commit.js

@@ -5,16 +5,18 @@ var contextLines = 0;
 
 var showNewFile = function(file)
 {
-	setTitle("New file: " + file.path.escapeHTML());
+	setTitle("New file: " + file.path);
+    diff.innerHTML = "";
 
 	var contents = Index.diffForFile_staged_contextLines_(file, false, contextLines);
 	if (!contents) {
 		notify("Can not display changes (Binary file?)", -1);
-		diff.innerHTML = "";
 		return;
 	}
 
-	diff.innerHTML = "<pre>" + contents.escapeHTML() + "</pre>";
+    var pre = document.createElement("pre");
+    pre.textContent = contents;
+    diff.appendChild(pre);
 	diff.style.display = '';
 }
 
@@ -27,11 +29,11 @@ var setState = function(state) {
 	hideNotification();
 	$("state").style.display = "";
 	$("diff").style.display = "none";
-	$("state").innerHTML = state.escapeHTML();
+	$("state").textContent = state;
 }
 
 var setTitle = function(status) {
-	$("status").innerHTML = status;
+	$("status").textContent = status;
 	$("contextSize").style.display = "none";
 	$("contextTitle").style.display = "none";
 }
@@ -59,7 +61,7 @@ var showFileChanges = function(file, cached) {
 	if (file.status == 0) // New file?
 		return showNewFile(file);
 
-	setTitle((cached ? "Staged": "Unstaged") + " changes for " + file.path.escapeHTML());
+	setTitle((cached ? "Staged" : "Unstaged") + " changes for " + file.path);
 	displayContext();
 	var changes = Index.diffForFile_staged_contextLines_(file, cached, contextLines);
 

Resources/html/views/commit/multipleSelection.js

@@ -4,19 +4,18 @@ var showMultipleFilesSelection = function(files)
 	setTitle("");
 
 	var div = $("diff");
+    div.style.display = "";
+	div.innerHTML = '<div id="multiselect">' +
+		'<div class="title">Multiple Selection</div>' +
+        '<ul></ul>' +
+        '</div>';
 
-	var contents = '<div id="multiselect">' +
-		'<div class="title">Multiple Selection</div>';
-
-	contents += "<ul>";
-
+    var ul = div.getElementsByTagName("ul")[0];
 	for (var i = 0; i < files.length; ++i)
 	{
 		var file = files[i];
-		contents += "<li>" + file.path + "</li>";
+        var li = document.createElement("li");
+        li.textContent = file.path;
+        ul.appendChild(li);
 	}
-	contents += "</ul></div>";
-
-	div.innerHTML = contents;
-	div.style.display = "";
-}
+}

Resources/html/views/diff/diffWindow.js

@@ -2,7 +2,6 @@
 
 var setMessage = function(message) {
 	$("message").style.display = "";
-	$("message").innerHTML = message.escapeHTML();
+	$("message").textContent = message;
 	$("diff").style.display = "none";
-}
-
+};