Skip to content

Commit d3f76a1

Browse files
dereuromarkdereuromark
committed
Fix deprecations panel empty check, escape variables panel error, reset $data in serialize loop
* DeprecationsPanel template: the empty-check sum was missing count($other), and the surrounding condition was inverted -- the "No deprecations" flash showed up exactly when deprecations existed. Include $other and flip to `if (!$hasAny)`. * Variables panel template: $error came through `printf %s` unescaped. The message can carry serializer-exception text that echoes back data from whatever variable failed to serialize, so escape it via h(). * ToolbarService::saveData: $data was declared inside `try`, so when a panel's data() itself threw, the catch block's `gettype($data ?? null)` diagnostic reported the previous iteration's panel data type instead of NULL. Reset $data = null per iteration. Adds integration tests covering each fix (deprecations empty / non-empty / other-only, variables panel error escaping, panel data() throwing).
1 parent 72cc1b7 commit d3f76a1

6 files changed

Lines changed: 180 additions & 3 deletions

File tree

‎src/ToolbarService.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -290,6 +290,7 @@ public function saveData(ServerRequest $request, ResponseInterface $response): R
290290

291291
foreach ($this->registry->loaded() as $name) {
292292
$panel = $this->registry->{$name};
293+
$data = null;
293294
try {
294295
$data = $panel->data();
295296

‎templates/element/deprecations_panel.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@
2323

2424
use function Cake\Core\h;
2525

26-
$hasAny = count($app) + count($plugins) + count($cake) + count($vendor);
26+
$hasAny = count($app) + count($plugins) + count($cake) + count($vendor) + count($other);
2727

2828
$printer = function ($section, $data) {
2929
?>
@@ -41,7 +41,7 @@
4141
}
4242
?>
4343
<div class="c-deprecations-panel">
44-
<?php if ($hasAny) : ?>
44+
<?php if (!$hasAny) : ?>
4545
<p class="c-flash c-flash--info">No deprecations</p>
4646
<?php
4747
endif;

‎templates/element/variables_panel.php‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,11 +18,14 @@
1818
* @var array $variables
1919
* @var array $errors
2020
*/
21+
22+
use function Cake\Core\h;
23+
2124
?>
2225
<div class="c-variables-panel">
2326
<?php
2427
if (isset($error)) :
25-
printf('<p class="c-flash c-flash--warning">%s</p>', $error);
28+
printf('<p class="c-flash c-flash--warning">%s</p>', h($error));
2629
endif;
2730

2831
if (isset($varsMaxDepth)) {

‎tests/TestCase/Controller/PanelsControllerTest.php‎

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,104 @@ public function testViewNotExists()
9898
$this->assertResponseContains('Error page');
9999
}
100100

101+
/**
102+
* Deprecations panel renders the "No deprecations" flash only when
103+
* every category is empty (including the `other` bucket, which was
104+
* previously missing from the check).
105+
*
106+
* @return void
107+
*/
108+
public function testViewDeprecationsPanelEmpty()
109+
{
110+
$request = $this->makeRequest();
111+
$panel = $this->makePanel(
112+
$request,
113+
'DebugKit.Deprecations',
114+
'Deprecations',
115+
'DebugKit.deprecations_panel',
116+
['app' => [], 'cake' => [], 'vendor' => [], 'plugins' => [], 'other' => []],
117+
);
118+
119+
$this->get("/debug-kit/panels/view/{$panel->id}");
120+
121+
$this->assertResponseOk();
122+
$this->assertResponseContains('No deprecations');
123+
}
124+
125+
/**
126+
* @return void
127+
*/
128+
public function testViewDeprecationsPanelWithEntries()
129+
{
130+
$request = $this->makeRequest();
131+
$entry = ['niceFile' => 'src/Foo.php', 'line' => 1, 'message' => 'deprecated thing'];
132+
$panel = $this->makePanel(
133+
$request,
134+
'DebugKit.Deprecations',
135+
'Deprecations',
136+
'DebugKit.deprecations_panel',
137+
['app' => [$entry], 'cake' => [], 'vendor' => [], 'plugins' => [], 'other' => []],
138+
);
139+
140+
$this->get("/debug-kit/panels/view/{$panel->id}");
141+
142+
$this->assertResponseOk();
143+
$this->assertResponseNotContains('No deprecations');
144+
$this->assertResponseContains('deprecated thing');
145+
}
146+
147+
/**
148+
* Deprecations only present in the `other` bucket should also suppress
149+
* the "No deprecations" flash. Guards against a regression of M3 where
150+
* `count($other)` was missing from the empty-check sum.
151+
*
152+
* @return void
153+
*/
154+
public function testViewDeprecationsPanelOtherOnly()
155+
{
156+
$request = $this->makeRequest();
157+
$entry = ['niceFile' => 'src/Bar.php', 'line' => 2, 'message' => 'only-other deprecation'];
158+
$panel = $this->makePanel(
159+
$request,
160+
'DebugKit.Deprecations',
161+
'Deprecations',
162+
'DebugKit.deprecations_panel',
163+
['app' => [], 'cake' => [], 'vendor' => [], 'plugins' => [], 'other' => [$entry]],
164+
);
165+
166+
$this->get("/debug-kit/panels/view/{$panel->id}");
167+
168+
$this->assertResponseOk();
169+
$this->assertResponseNotContains('No deprecations');
170+
$this->assertResponseContains('only-other deprecation');
171+
}
172+
173+
/**
174+
* The Variables panel surfaces serialization-error messages from
175+
* `ToolbarService`; those messages can echo back attacker-controlled
176+
* data (e.g. via `__sleep` exceptions), so the template must escape
177+
* them. Guards against a regression of M4.
178+
*
179+
* @return void
180+
*/
181+
public function testViewVariablesPanelErrorIsEscaped()
182+
{
183+
$request = $this->makeRequest();
184+
$panel = $this->makePanel(
185+
$request,
186+
'DebugKit.Variables',
187+
'Variables',
188+
'DebugKit.variables_panel',
189+
['error' => '<script>alert(1)</script>', 'variables' => [], 'errors' => []],
190+
);
191+
192+
$this->get("/debug-kit/panels/view/{$panel->id}");
193+
194+
$this->assertResponseOk();
195+
$this->assertResponseNotContains('<script>alert(1)</script>');
196+
$this->assertResponseContains('&lt;script&gt;alert(1)&lt;/script&gt;');
197+
}
198+
101199
/**
102200
* @return void
103201
*/

‎tests/TestCase/ToolbarServiceTest.php‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
use DebugKit\Model\Entity\Request as RequestEntity;
2828
use DebugKit\Panel\SqlLogPanel;
2929
use DebugKit\TestApp\Panel\SimplePanel;
30+
use DebugKit\TestApp\Panel\ThrowingDataPanel;
3031
use DebugKit\ToolbarService;
3132
use PHPUnit\Framework\Attributes\DataProvider;
3233

@@ -559,4 +560,52 @@ public function testSaveDataSerializationError()
559560
$this->assertArrayHasKey('error', $content, 'Should have error key');
560561
$this->assertStringContainsString('SimplePanel', $content['error'], 'Error should mention panel name');
561562
}
563+
564+
/**
565+
* Test that saveData survives a panel whose data() method itself throws,
566+
* not just one whose serialize() fails.
567+
*
568+
* @return void
569+
*/
570+
public function testSaveDataPanelDataThrows()
571+
{
572+
$request = new Request([
573+
'url' => '/articles',
574+
'environment' => ['REQUEST_METHOD' => 'GET'],
575+
]);
576+
$response = new Response([
577+
'statusCode' => 200,
578+
'type' => 'text/html',
579+
'body' => '<html><title>test</title><body><p>some text</p></body>',
580+
]);
581+
582+
$bar = new ToolbarService($this->events, []);
583+
$bar->loadPanels();
584+
$bar->registry()->load('DebugKit.TestApp\Panel\ThrowingDataPanel', [
585+
'className' => ThrowingDataPanel::class,
586+
]);
587+
588+
$row = $bar->saveData($request, $response);
589+
$this->assertNotEmpty($row, 'Should save data even when a panel data() throws');
590+
591+
$requests = $this->getTableLocator()->get('DebugKit.Requests');
592+
$result = $requests->find()
593+
->orderBy(['Requests.requested_at' => 'DESC'])
594+
->contain('Panels')
595+
->first();
596+
597+
$throwingPanel = null;
598+
foreach ($result->panels as $p) {
599+
if ($p->panel === 'DebugKit.TestApp\Panel\ThrowingDataPanel') {
600+
$throwingPanel = $p;
601+
break;
602+
}
603+
}
604+
605+
$this->assertNotNull($throwingPanel, 'ThrowingDataPanel should be present');
606+
$content = unserialize($throwingPanel->content);
607+
$this->assertArrayHasKey('error', $content);
608+
$this->assertStringContainsString('ThrowingDataPanel', $content['error']);
609+
$this->assertStringContainsString('Simulated data() failure', $content['error']);
610+
}
562611
}

‎tests/test_app/Panel/ThrowingDataPanel.php‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
<?php
2+
declare(strict_types=1);
3+
4+
/**
5+
* Licensed under The MIT License
6+
* For full copyright and license information, please see the LICENSE.txt
7+
*
8+
* @license https://opensource.org/licenses/MIT MIT License
9+
*/
10+
namespace DebugKit\TestApp\Panel;
11+
12+
use DebugKit\DebugPanel;
13+
use RuntimeException;
14+
15+
/**
16+
* Test panel whose data() method throws, used to verify that
17+
* ToolbarService::saveData() does not crash and produces a clean
18+
* diagnostic when a panel's data collection itself fails.
19+
*/
20+
class ThrowingDataPanel extends DebugPanel
21+
{
22+
public function data(): array
23+
{
24+
throw new RuntimeException('Simulated data() failure');
25+
}
26+
}

0 commit comments

Comments
 (0)