Skip to content

Commit d6d2d28

Browse files
iliaaliliaal
committed
Escape remaining HTML error fragment fields
File, Type, Code, and Line were interpolated into the details fragment without escaping. File paths and anonymous class names can contain HTML metacharacters, and Throwable::getCode() is untyped. Route every fragment field through the same htmlspecialchars() flags as title and description.
1 parent 2d982b0 commit d6d2d28

2 files changed

Lines changed: 74 additions & 28 deletions

File tree

‎Slim/Error/Renderers/HtmlErrorRenderer.php‎

Lines changed: 13 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -32,11 +32,7 @@ public function __invoke(Throwable $exception, bool $displayErrorDetails): strin
3232
$html .= '<h2>Details</h2>';
3333
$html .= $this->renderExceptionFragment($exception);
3434
} else {
35-
$description = htmlspecialchars(
36-
$this->getErrorDescription($exception),
37-
ENT_QUOTES | ENT_SUBSTITUTE,
38-
'UTF-8'
39-
);
35+
$description = $this->escapeHtml($this->getErrorDescription($exception));
4036
$html = "<p>{$description}</p>";
4137
}
4238

@@ -45,32 +41,25 @@ public function __invoke(Throwable $exception, bool $displayErrorDetails): strin
4541

4642
private function renderExceptionFragment(Throwable $exception): string
4743
{
48-
$html = sprintf('<div><strong>Type:</strong> %s</div>', get_class($exception));
49-
50-
$code = $exception->getCode();
51-
$html .= sprintf('<div><strong>Code:</strong> %s</div>', $code);
52-
53-
$html .= sprintf(
54-
'<div><strong>Message:</strong> %s</div>',
55-
htmlspecialchars($exception->getMessage(), ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8')
56-
);
57-
58-
$html .= sprintf('<div><strong>File:</strong> %s</div>', $exception->getFile());
59-
60-
$html .= sprintf('<div><strong>Line:</strong> %s</div>', $exception->getLine());
61-
44+
$html = sprintf('<div><strong>Type:</strong> %s</div>', $this->escapeHtml(get_class($exception)));
45+
$html .= sprintf('<div><strong>Code:</strong> %s</div>', $this->escapeHtml((string) $exception->getCode()));
46+
$html .= sprintf('<div><strong>Message:</strong> %s</div>', $this->escapeHtml($exception->getMessage()));
47+
$html .= sprintf('<div><strong>File:</strong> %s</div>', $this->escapeHtml($exception->getFile()));
48+
$html .= sprintf('<div><strong>Line:</strong> %s</div>', $this->escapeHtml((string) $exception->getLine()));
6249
$html .= '<h2>Trace</h2>';
63-
$html .= sprintf(
64-
'<pre>%s</pre>',
65-
htmlspecialchars($exception->getTraceAsString(), ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8')
66-
);
50+
$html .= sprintf('<pre>%s</pre>', $this->escapeHtml($exception->getTraceAsString()));
6751

6852
return $html;
6953
}
7054

55+
private function escapeHtml(string $text): string
56+
{
57+
return htmlspecialchars($text, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8');
58+
}
59+
7160
public function renderHtmlBody(string $title = '', string $html = ''): string
7261
{
73-
$title = htmlspecialchars($title, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8');
62+
$title = $this->escapeHtml($title);
7463

7564
return sprintf(
7665
'<!doctype html>' .

‎tests/Error/AbstractErrorRendererTest.php‎

Lines changed: 61 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -21,10 +21,18 @@
2121
use Slim\Tests\TestCase;
2222
use stdClass;
2323

24+
use function bin2hex;
25+
use function file_put_contents;
26+
use function get_class;
2427
use function htmlspecialchars;
2528
use function json_decode;
2629
use function json_encode;
30+
use function mkdir;
31+
use function random_bytes;
32+
use function rmdir;
2733
use function simplexml_load_string;
34+
use function sys_get_temp_dir;
35+
use function unlink;
2836

2937
use const ENT_QUOTES;
3038
use const ENT_SUBSTITUTE;
@@ -76,19 +84,68 @@ public function testHTMLErrorRendererRenderFragmentMethod()
7684

7785
public function testHTMLErrorRendererEscapesQuotesInErrorDetails()
7886
{
79-
$exception = new Exception("O'Brien <script>");
87+
$unsafe = "O'Brien <script>";
88+
$escaped = htmlspecialchars($unsafe, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8');
89+
90+
$exception = new class ($unsafe) extends Exception {
91+
public function __construct(string $unsafe)
92+
{
93+
parent::__construct($unsafe, 0);
94+
$this->file = '/tmp/' . $unsafe . '.php';
95+
$this->code = "SQL'" . $unsafe;
96+
$this->line = 7;
97+
}
98+
};
99+
80100
$renderer = new HtmlErrorRenderer();
81101
$reflectionRenderer = new ReflectionClass(HtmlErrorRenderer::class);
82102

83103
$method = $reflectionRenderer->getMethod('renderExceptionFragment');
84104
$this->setAccessible($method);
85105
$output = $method->invoke($renderer, $exception);
86106

107+
$this->assertStringNotContainsString($unsafe, $output);
108+
$this->assertStringContainsString($escaped, $output);
87109
$this->assertStringContainsString(
88-
htmlspecialchars("O'Brien <script>", ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8'),
89-
$output,
90-
'Message must be HTML-escaped including quotes'
110+
htmlspecialchars('/tmp/' . $unsafe . '.php', ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8'),
111+
$output
91112
);
113+
$this->assertStringContainsString(
114+
htmlspecialchars("SQL'" . $unsafe, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8'),
115+
$output
116+
);
117+
$this->assertStringContainsString('<div><strong>Line:</strong> 7</div>', $output);
118+
}
119+
120+
public function testHTMLErrorRendererEscapesAnonymousClassType()
121+
{
122+
$unsafe = "O'Brien <script>";
123+
$dir = sys_get_temp_dir() . '/slim-html-' . bin2hex(random_bytes(4));
124+
$unsafeDir = $dir . '/' . $unsafe;
125+
mkdir($unsafeDir, 0700, true);
126+
$file = $unsafeDir . '/anon.php';
127+
file_put_contents($file, '<?php return new class extends Exception {};');
128+
129+
try {
130+
/** @var Exception $exception */
131+
$exception = require $file;
132+
$renderer = new HtmlErrorRenderer();
133+
$reflectionRenderer = new ReflectionClass(HtmlErrorRenderer::class);
134+
135+
$method = $reflectionRenderer->getMethod('renderExceptionFragment');
136+
$this->setAccessible($method);
137+
$output = $method->invoke($renderer, $exception);
138+
139+
$this->assertStringNotContainsString($unsafe, $output);
140+
$this->assertStringContainsString(
141+
htmlspecialchars(get_class($exception), ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8'),
142+
$output
143+
);
144+
} finally {
145+
unlink($file);
146+
rmdir($unsafeDir);
147+
rmdir($dir);
148+
}
92149
}
93150

94151
public function testHTMLErrorRendererRenderHttpException()

0 commit comments

Comments
 (0)