Skip to content

Commit c6258fa

Browse files
committed
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
1 parent 0faaedf commit c6258fa

File tree

2 files changed

+22
-20
lines changed

2 files changed

+22
-20
lines changed

lib/Cake/Test/Case/Utility/DebuggerTest.php

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -140,15 +140,16 @@ public function testOutput() {
140140
'a' => array(
141141
'href' => "javascript:void(0);",
142142
'onclick' => "preg:/document\.getElementById\('cakeErr[a-z0-9]+\-trace'\)\.style\.display = " .
143-
"\(document\.getElementById\('cakeErr[a-z0-9]+\-trace'\)\.style\.display == 'none'" .
144-
" \? '' \: 'none'\);/"
143+
"\(document\.getElementById\('cakeErr[a-z0-9]+\-trace'\)\.style\.display == 'none'" .
144+
" \? '' \: 'none'\);/"
145145
),
146146
'b' => array(), 'Notice', '/b', ' (8)',
147147
));
148148

149149
$this->assertRegExp('/Undefined variable:\s+buzz/', $result[1]);
150150
$this->assertRegExp('/<a[^>]+>Code/', $result[1]);
151151
$this->assertRegExp('/<a[^>]+>Context/', $result[2]);
152+
$this->assertContains('$wrong = &#039;&#039;', $result[3], 'Context should be HTML escaped.');
152153
}
153154

154155
/**
@@ -162,14 +163,14 @@ public function testChangeOutputFormats() {
162163

163164
Debugger::output('js', array(
164165
'traceLine' => '{:reference} - <a href="txmt://open?url=file://{:file}' .
165-
'&line={:line}">{:path}</a>, line {:line}'
166+
'&line={:line}">{:path}</a>, line {:line}'
166167
));
167168
$result = Debugger::trace();
168169
$this->assertRegExp('/' . preg_quote('txmt://open?url=file://', '/') . '(\/|[A-Z]:\\\\)' . '/', $result);
169170

170171
Debugger::output('xml', array(
171172
'error' => '<error><code>{:code}</code><file>{:file}</file><line>{:line}</line>' .
172-
'{:description}</error>',
173+
'{:description}</error>',
173174
'context' => "<context>{:context}</context>",
174175
'trace' => "<stack>{:trace}</stack>",
175176
));

lib/Cake/Utility/Debugger.php

Lines changed: 17 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -63,11 +63,13 @@ class Debugger {
6363
'trace' => '<pre class="stack-trace">{:trace}</pre>',
6464
'code' => '',
6565
'context' => '',
66-
'links' => array()
66+
'links' => array(),
67+
'escapeContext' => true,
6768
),
6869
'html' => array(
6970
'trace' => '<pre class="cake-error trace"><b>Trace</b> <p>{:trace}</p></pre>',
70-
'context' => '<pre class="cake-error context"><b>Context</b> <p>{:context}</p></pre>'
71+
'context' => '<pre class="cake-error context"><b>Context</b> <p>{:context}</p></pre>',
72+
'escapeContext' => true,
7173
),
7274
'txt' => array(
7375
'error' => "{:error}: {:code} :: {:description} on line {:line} of {:path}\n{:info}",
@@ -716,7 +718,7 @@ public function outputError($data) {
716718
$info = '';
717719

718720
foreach ((array)$data['context'] as $var => $value) {
719-
$context[] = "\${$var}\t=\t" . $this->exportVar($value, 1);
721+
$context[] = "\${$var} = " . $this->exportVar($value, 1);
720722
}
721723

722724
switch ($this->_outputFormat) {
@@ -731,30 +733,29 @@ public function outputError($data) {
731733
$data['trace'] = $trace;
732734
$data['id'] = 'cakeErr' . uniqid();
733735
$tpl = array_merge($this->_templates['base'], $this->_templates[$this->_outputFormat]);
734-
$insert = array('context' => join("\n", $context)) + $data;
735-
736-
$detect = array('context');
737736

738737
if (isset($tpl['links'])) {
739738
foreach ($tpl['links'] as $key => $val) {
740-
if (in_array($key, $detect) && empty($insert[$key])) {
741-
continue;
742-
}
743-
$links[$key] = String::insert($val, $insert, $insertOpts);
739+
$links[$key] = String::insert($val, $data, $insertOpts);
744740
}
745741
}
746742

747-
foreach (array('code', 'context', 'trace') as $key) {
748-
if (empty($$key) || !isset($tpl[$key])) {
743+
if (!empty($tpl['escapeContext'])) {
744+
$context = h($context);
745+
}
746+
747+
$infoData = compact('code', 'context', 'trace');
748+
foreach ($infoData as $key => $value) {
749+
if (empty($value) || !isset($tpl[$key])) {
749750
continue;
750751
}
751-
if (is_array($$key)) {
752-
$$key = join("\n", $$key);
752+
if (is_array($value)) {
753+
$value = join("\n", $value);
753754
}
754-
$info .= String::insert($tpl[$key], compact($key) + $insert, $insertOpts);
755+
$info .= String::insert($tpl[$key], array($key => $value) + $data, $insertOpts);
755756
}
756757
$links = join(' ', $links);
757-
unset($data['context']);
758+
758759
if (isset($tpl['callback']) && is_callable($tpl['callback'])) {
759760
return call_user_func($tpl['callback'], $data, compact('links', 'info'));
760761
}

0 commit comments

Comments
 (0)