Merge pull request #124 from nanotech/xss

Fix and mitigate code injection (XSS) in HTML views

Author: tiennou

Classes/Views/GLFileView.h

@@ -17,17 +17,15 @@
 	__weak IBOutlet PBGitHistoryController* historyController;
 	__weak IBOutlet MGScopeBar *typeBar;
 	NSMutableArray *groups;
-	NSString *logFormat;
 	__weak IBOutlet NSView *accessoryView;
 	__weak IBOutlet NSSplitView *fileListSplitView;
 }
 
 - (void)showFile;
 - (void)didLoad;
 - (NSString *)parseBlame:(NSString *)txt;
-- (NSString *)parseHTML:(NSString *)txt;
+- (NSString *)escapeHTML:(NSString *)txt;
 
 @property NSMutableArray *groups;
-@property NSString *logFormat;
 
 @end

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/css/GitX.css

@@ -31,3 +31,7 @@ table {
 	font-family: Menlo, Monaco, monospace;
 	text-decoration: none;
 }
+
+.hidden {
+	display: none;
+}

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,25 +33,36 @@ 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;
+	n.classList.remove("hidden");
+	$("notification_message").innerHTML = html;
 
 	// Change color
 	if (!state) { // Busy
-		$("spinner").style.display = "";
-		n.setAttribute("class", "");
+		$("spinner").classList.remove("hidden");
+		n.classList.remove("success");
+		n.classList.remove("fail");
 	}
 	else if (state == 1) { // Success
-		$("spinner").style.display = "none";
-		n.setAttribute("class", "success");
+		$("spinner").classList.add("hidden");
+		n.classList.add("success");
 	} else if (state == -1) {// Fail
-		$("spinner").style.display = "none";
-		n.setAttribute("class", "fail");
+		$("spinner").classList.add("hidden");
+		n.classList.add("fail");
 	}
 }
 
 var hideNotification = function() {
-	$("notification").style.display = "none";
+	$("notification").classList.add("hidden");
 }
+
+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/blame/index.html

@@ -1,5 +1,7 @@
 <html>
 	<head>
+		<meta charset="utf-8">
+		<meta http-equiv="Content-Security-Policy" content="default-src 'none'; script-src 'self'; style-src 'self';">
 		<script src="../../lib/GitX.js" type="text/javascript" charset="utf-8"></script>
 		<script src="../../lib/syntaxhighlighter/scripts/shCore.js" type="text/javascript" charset="utf-8"></script>
 		<script src="../../lib/syntaxhighlighter/scripts/shBrushObjC.js" type="text/javascript" charset="utf-8"></script>
@@ -11,4 +13,4 @@
 	<body>
 		<div id="txt">hola</pre>
 	</body>
-</html>
+</html>