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 864b8f955..f45f19cdc 100644
--- a/lib/Cake/Test/Case/Utility/DebuggerTest.php
+++ b/lib/Cake/Test/Case/Utility/DebuggerTest.php
@@ -140,8 +140,8 @@ class DebuggerTest extends CakeTestCase {
 			'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 @@ class DebuggerTest extends CakeTestCase {
 		$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 @@ class DebuggerTest extends CakeTestCase {
 
 		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 9ed57d1d9..2a9fc9c23 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 @@ class Debugger {
 		$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 @@ class Debugger {
 		$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'));
 		}