From c6258fa68c09e785bedbc6e517b61c869f25f727 Mon Sep 17 00:00:00 2001
From: mark_story <mark@mark-story.com>
Date: Wed, 16 May 2012 21:07:45 -0400
Subject: [PATCH] HTML escape context variables.

When creating HTML or js errors the context should be
escaped, as it is very possible that the context vars will contain
HTML.

Clean up some internals in Debugger::outputError().  There were
a few duplicate data structures, and $$ variables.

Fixes #2884
---
 lib/Cake/Test/Case/Utility/DebuggerTest.php |  9 +++---
 lib/Cake/Utility/Debugger.php               | 33 +++++++++++----------
 2 files changed, 22 insertions(+), 20 deletions(-)

diff --git a/lib/Cake/Test/Case/Utility/DebuggerTest.php b/lib/Cake/Test/Case/Utility/DebuggerTest.php
index 864b8f955de..f45f19cdcde 100644
--- a/lib/Cake/Test/Case/Utility/DebuggerTest.php
+++ b/lib/Cake/Test/Case/Utility/DebuggerTest.php
@@ -140,8 +140,8 @@ public function testOutput() {
 			'a' => array(
 				'href' => "javascript:void(0);",
 				'onclick' => "preg:/document\.getElementById\('cakeErr[a-z0-9]+\-trace'\)\.style\.display = " .
-							 "\(document\.getElementById\('cakeErr[a-z0-9]+\-trace'\)\.style\.display == 'none'" .
-							 " \? '' \: 'none'\);/"
+					 "\(document\.getElementById\('cakeErr[a-z0-9]+\-trace'\)\.style\.display == 'none'" .
+					 " \? '' \: 'none'\);/"
 			),
 			'b' => array(), 'Notice', '/b', ' (8)',
 		));
@@ -149,6 +149,7 @@ public function testOutput() {
 		$this->assertRegExp('/Undefined variable:\s+buzz/', $result[1]);
 		$this->assertRegExp('/<a[^>]+>Code/', $result[1]);
 		$this->assertRegExp('/<a[^>]+>Context/', $result[2]);
+		$this->assertContains('$wrong = &#039;&#039;', $result[3], 'Context should be HTML escaped.');
 	}
 
 /**
@@ -162,14 +163,14 @@ public function testChangeOutputFormats() {
 
 		Debugger::output('js', array(
 			'traceLine' => '{:reference} - <a href="txmt://open?url=file://{:file}' .
-							 '&line={:line}">{:path}</a>, line {:line}'
+				'&line={:line}">{:path}</a>, line {:line}'
 		));
 		$result = Debugger::trace();
 		$this->assertRegExp('/' . preg_quote('txmt://open?url=file://', '/') . '(\/|[A-Z]:\\\\)' . '/', $result);
 
 		Debugger::output('xml', array(
 			'error' => '<error><code>{:code}</code><file>{:file}</file><line>{:line}</line>' .
-						 '{:description}</error>',
+				 '{:description}</error>',
 			'context' => "<context>{:context}</context>",
 			'trace' => "<stack>{:trace}</stack>",
 		));
diff --git a/lib/Cake/Utility/Debugger.php b/lib/Cake/Utility/Debugger.php
index 9ed57d1d91c..2a9fc9c230a 100644
--- a/lib/Cake/Utility/Debugger.php
+++ b/lib/Cake/Utility/Debugger.php
@@ -63,11 +63,13 @@ class Debugger {
 			'trace' => '<pre class="stack-trace">{:trace}</pre>',
 			'code' => '',
 			'context' => '',
-			'links' => array()
+			'links' => array(),
+			'escapeContext' => true,
 		),
 		'html' => array(
 			'trace' => '<pre class="cake-error trace"><b>Trace</b> <p>{:trace}</p></pre>',
-			'context' => '<pre class="cake-error context"><b>Context</b> <p>{:context}</p></pre>'
+			'context' => '<pre class="cake-error context"><b>Context</b> <p>{:context}</p></pre>',
+			'escapeContext' => true,
 		),
 		'txt' => array(
 			'error' => "{:error}: {:code} :: {:description} on line {:line} of {:path}\n{:info}",
@@ -716,7 +718,7 @@ public function outputError($data) {
 		$info = '';
 
 		foreach ((array)$data['context'] as $var => $value) {
-			$context[] = "\${$var}\t=\t" . $this->exportVar($value, 1);
+			$context[] = "\${$var} = " . $this->exportVar($value, 1);
 		}
 
 		switch ($this->_outputFormat) {
@@ -731,30 +733,29 @@ public function outputError($data) {
 		$data['trace'] = $trace;
 		$data['id'] = 'cakeErr' . uniqid();
 		$tpl = array_merge($this->_templates['base'], $this->_templates[$this->_outputFormat]);
-		$insert = array('context' => join("\n", $context)) + $data;
-
-		$detect = array('context');
 
 		if (isset($tpl['links'])) {
 			foreach ($tpl['links'] as $key => $val) {
-				if (in_array($key, $detect) && empty($insert[$key])) {
-					continue;
-				}
-				$links[$key] = String::insert($val, $insert, $insertOpts);
+				$links[$key] = String::insert($val, $data, $insertOpts);
 			}
 		}
 
-		foreach (array('code', 'context', 'trace') as $key) {
-			if (empty($$key) || !isset($tpl[$key])) {
+		if (!empty($tpl['escapeContext'])) {
+			$context = h($context);
+		}
+
+		$infoData = compact('code', 'context', 'trace');
+		foreach ($infoData as $key => $value) {
+			if (empty($value) || !isset($tpl[$key])) {
 				continue;
 			}
-			if (is_array($$key)) {
-				$$key = join("\n", $$key);
+			if (is_array($value)) {
+				$value = join("\n", $value);
 			}
-			$info .= String::insert($tpl[$key], compact($key) + $insert, $insertOpts);
+			$info .= String::insert($tpl[$key], array($key => $value) + $data, $insertOpts);
 		}
 		$links = join(' ', $links);
-		unset($data['context']);
+
 		if (isset($tpl['callback']) && is_callable($tpl['callback'])) {
 			return call_user_func($tpl['callback'], $data, compact('links', 'info'));
 		}