From c6c6c13fc265aeedf5de7226b3cde39d185ba49d Mon Sep 17 00:00:00 2001
From: Daniel Hensby <dhensby@silverstripe.com>
Date: Fri, 13 Jan 2017 00:12:22 +0000
Subject: [PATCH] [SS-2017-001] FIX Unescaped title attribute in
 LeftAndMain_TreeNode::forTemplate

---
 admin/code/LeftAndMain.php                    | 22 +++++++++++--------
 .../Includes/LeftAndMain_TreeNode.ss          |  6 +++++
 admin/tests/LeftAndMainTest.php               |  4 +++-
 model/Hierarchy.php                           |  6 ++++-
 4 files changed, 27 insertions(+), 11 deletions(-)
 create mode 100644 admin/templates/Includes/LeftAndMain_TreeNode.ss

diff --git a/admin/code/LeftAndMain.php b/admin/code/LeftAndMain.php
index 31385675b..6ba428714 100644
--- a/admin/code/LeftAndMain.php
+++ b/admin/code/LeftAndMain.php
@@ -1005,8 +1005,7 @@ class LeftAndMain extends Controller implements PermissionProvider {
 			}
 
 			$link = Controller::join_links($recordController->Link("show"), $record->ID);
-			$html = LeftAndMain_TreeNode::create($record, $link, $this->isCurrentPage($record))
-				->forTemplate() . '</li>';
+			$html = LeftAndMain_TreeNode::create($record, $link, $this->isCurrentPage($record))->forTemplate();
 
 			$data[$id] = array(
 				'html' => $html,
@@ -1982,16 +1981,21 @@ class LeftAndMain_TreeNode extends ViewableData {
 	 *
 	 * @todo Remove hardcoded assumptions around returning an <li>, by implementing recursive tree node rendering
 	 *
-	 * @return String
+	 * @return string
 	 */
 	public function forTemplate() {
 		$obj = $this->obj;
-		return "<li id=\"record-$obj->ID\" data-id=\"$obj->ID\" data-pagetype=\"$obj->ClassName\" class=\""
-			. $this->getClasses() . "\">" . "<ins class=\"jstree-icon\">&nbsp;</ins>"
-			. "<a href=\"" . $this->getLink() . "\" title=\"("
-			. trim(_t('LeftAndMain.PAGETYPE','Page type'), " :") // account for inconsistencies in translations
-			. ": " . $obj->i18n_singular_name() . ") $obj->Title\" ><ins class=\"jstree-icon\">&nbsp;</ins><span class=\"text\">" . ($obj->TreeTitle)
-			. "</span></a>";
+
+		return (string)SSViewer::execute_template('LeftAndMain_TreeNode', $obj, array(
+			'Classes' => $this->getClasses(),
+			'Link' => $this->getLink(),
+			'Title' => sprintf(
+				'(%s: %s) %s',
+				trim(_t('LeftAndMain.PAGETYPE','Page type'), " :"),
+				$obj->i18n_singular_name(),
+				$obj->Title
+			),
+		));
 	}
 
 	/**
diff --git a/admin/templates/Includes/LeftAndMain_TreeNode.ss b/admin/templates/Includes/LeftAndMain_TreeNode.ss
new file mode 100644
index 000000000..7e4c8ebf8
--- /dev/null
+++ b/admin/templates/Includes/LeftAndMain_TreeNode.ss
@@ -0,0 +1,6 @@
+<li id="record-$ID" data-id="$ID" data-pagetype="$ClassName" class="$Classes">
+	<ins class="jstree-icon">&nbsp;</ins>
+	<a href="$Link" title="$Title.ATT"><ins class="jstree-icon">&nbsp;</ins>
+		<span class="text">$TreeTitle</span>
+	</a>
+</li>
diff --git a/admin/tests/LeftAndMainTest.php b/admin/tests/LeftAndMainTest.php
index bb4036ba0..750f40856 100644
--- a/admin/tests/LeftAndMainTest.php
+++ b/admin/tests/LeftAndMainTest.php
@@ -302,6 +302,8 @@ class LeftAndMainTest_Object extends DataObject implements TestOnly {
 		'Hierarchy'
 	);
 
-	public function CMSTreeClasses() {}
+	public function CMSTreeClasses() {
+		return '';
+	}
 
 }
diff --git a/model/Hierarchy.php b/model/Hierarchy.php
index 51f510bcc..13dedd7f4 100644
--- a/model/Hierarchy.php
+++ b/model/Hierarchy.php
@@ -101,7 +101,7 @@ class Hierarchy extends DataExtension {
 	 *
 	 * @return string
 	 */
-	public function getChildrenAsUL($attributes = "", $titleEval = '"<li>" . $child->Title', $extraArg = null,
+	public function getChildrenAsUL($attributes = "", $titleEval = '"<li>" . $child->Title . "</li>"', $extraArg = null,
 			$limitToMarked = false, $childrenMethod = "AllChildrenIncludingDeleted",
 			$numChildrenMethod = "numChildren", $rootCall = true,
 			$nodeCountThreshold = null, $nodeCountCallback = null) {
@@ -144,6 +144,10 @@ class Hierarchy extends DataExtension {
 					} else {
 						$output .= eval("return $titleEval;");
 					}
+					$output = trim($output);
+					if (substr($output, -5) == '</li>') {
+						$output = trim(substr($output, 0, -5));
+					}
 					$output .= "\n";
 
 					$numChildren = $child->$numChildrenMethod();