Skip to content

Commit 67cb283

Browse files
committed
Escape stored security-list values with PFASmarty
1 parent 090ac0c commit 67cb283

4 files changed

Lines changed: 68 additions & 2 deletions

File tree

‎CHANGELOG.TXT‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
Changes in 'master' (not in a release yet) (20XX/XX/XX)
1010
--------------------------
1111

12+
- Escape stored application-password and TOTP-exception list values.
1213
- Show the configured TOTP status and Reset TOTP action without reserving space for the hidden form (see #1166, thanks @TrapoSAMA)
1314
- Distinguish unknown local alias destinations from external destinations without inspecting other administrators' domains (see discussion #886, thanks @TrapoSAMA)
1415
- Show available usage for unlimited quotas and retain neutral quota boxes. (see #1151, thanks @TrapoSAMA)

‎public/users/app-passwords.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -115,7 +115,7 @@
115115
$smarty->assign('pPassword_text', $pPassword_text, false);
116116
$smarty->assign('pUser_text', $pUser_text, false);
117117
$smarty->assign('pUser', $pUser, false);
118-
$smarty->assign('pPasswords', $passwords, false);
118+
$smarty->assign('pPasswords', $passwords);
119119
$smarty->assign('smarty_template', 'app-passwords');
120120
$smarty->display('index.tpl');
121121

‎public/users/totp-exceptions.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -130,7 +130,7 @@
130130
$smarty->assign('pUser_text', $pUser_text, false);
131131
$smarty->assign('pUser', $pUser, false);
132132
#$smarty->assign('', $, false);
133-
$smarty->assign('pExceptions', $exceptions, false);
133+
$smarty->assign('pExceptions', $exceptions);
134134
$smarty->assign('smarty_template', 'totp-exceptions');
135135
$smarty->display('index.tpl');
136136

‎tests/SecurityListOutputTest.php‎

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,65 @@
1+
<?php
2+
3+
use PHPUnit\Framework\Attributes\DataProvider;
4+
use PHPUnit\Framework\TestCase;
5+
6+
class SecurityListOutputTest extends TestCase
7+
{
8+
public static function lists(): array
9+
{
10+
return [
11+
'application passwords' => ['app-passwords', 'pPasswords', 'passwords'],
12+
'TOTP exceptions' => ['totp-exceptions', 'pExceptions', 'exceptions'],
13+
];
14+
}
15+
16+
#[DataProvider('lists')]
17+
public function testStoredListValuesAreEscapedWithoutChangingRevokeControls(string $page, string $assigned, string $input): void
18+
{
19+
$engine = new \Smarty\Smarty();
20+
$engine->registerPlugin('function', 'CSRF_Token', static fn () => '<input name="CSRF_Token" value="fixture-token">');
21+
$engine->assign('PALANG', ['pTotp_exceptions_revoke' => 'Revoke']);
22+
$reflection = new ReflectionClass(PFASmarty::class);
23+
$smarty = $reflection->newInstanceWithoutConstructor();
24+
$reflection->getProperty('template')->setValue($smarty, $engine);
25+
26+
$descriptions = [
27+
'<img data-security-fixture="injected" src=x onerror="alert(1)">',
28+
"R&D \"quoted\" 'text' <tag> UTF-8: \u{00F1}",
29+
'Legacy &lt;tag&gt; &amp; text',
30+
];
31+
$rows = [];
32+
foreach ($descriptions as $index => $description) {
33+
$rows[] = [
34+
'id' => $index + 1,
35+
'username' => 'fixture@example.invalid',
36+
'ip' => '192.0.2.1',
37+
'description' => $description,
38+
'edit' => $index === 0 ? 1 : 0,
39+
];
40+
}
41+
$$input = $rows;
42+
$controller = file_get_contents(__DIR__ . '/../public/users/' . $page . '.php');
43+
$pattern = '/\$smarty->assign\(\x27' . preg_quote($assigned, '/') . '\x27, \$' . preg_quote($input, '/') . '(?:, false)?\);/';
44+
$this->assertSame(1, preg_match($pattern, $controller, $assignment));
45+
// Execute the real controller assignment so disabling sanitization fails this regression.
46+
eval($assignment[0]);
47+
48+
$template = file_get_contents(__DIR__ . '/../templates/' . $page . '.tpl');
49+
$this->assertSame(1, preg_match('/\{foreach \$' . preg_quote($assigned, '/') . ' .*?\{\/foreach\}/s', $template, $loop));
50+
$html = $engine->fetch('eval:<table>' . $loop[0] . '</table>');
51+
$document = new DOMDocument();
52+
$document->loadHTML('<?xml encoding="UTF-8">' . $html);
53+
$xpath = new DOMXPath($document);
54+
$this->assertSame(0, $xpath->query('//*[@data-security-fixture]')->length);
55+
$this->assertSame(2, $xpath->query('//button[@disabled]')->length);
56+
$this->assertSame(3, $xpath->query('//input[@name="CSRF_Token"]')->length);
57+
$descriptionColumn = $page === 'app-passwords' ? 2 : 3;
58+
$cells = $xpath->query('//tr/td[' . $descriptionColumn . ']');
59+
$this->assertSame(3, $cells->length);
60+
foreach ($descriptions as $index => $description) {
61+
$this->assertSame(html_entity_decode($description, ENT_QUOTES, 'UTF-8'), $cells->item($index)->textContent);
62+
}
63+
$this->assertStringNotContainsString('&amp;lt;tag&amp;gt;', $html);
64+
}
65+
}

0 commit comments

Comments
 (0)