Skip to content

Commit f7efd25

Browse files
committed
fix: improve JSON error handling and security measures
- fix(json): add JSON_THROW_ON_ERROR and proper encoding flags - fix(xss): add HTML escaping for element IDs - fix(memory): add chart instance tracking and cleanup - fix(error): add canvas element existence check - test: add comprehensive test coverage for security fixes
1 parent 0b30a4a commit f7efd25

File tree

2 files changed

+133
-6
lines changed

2 files changed

+133
-6
lines changed

src/Chart.php

Lines changed: 29 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -50,10 +50,11 @@ public function toArray(): array
5050
* Convert the chart to JSON
5151
*
5252
* @return string
53+
* @throws \JsonException
5354
*/
5455
public function toJson(): string
5556
{
56-
return json_encode($this->toArray(), true);
57+
return json_encode($this->toArray(), JSON_THROW_ON_ERROR | JSON_HEX_TAG | JSON_HEX_APOS | JSON_HEX_AMP | JSON_HEX_QUOT);
5758
}
5859

5960
/**
@@ -115,12 +116,34 @@ public function toHtml(string $element, Chart $chart = null): string
115116
$chart = $this;
116117
}
117118

118-
return '<canvas id="' . $element . '"></canvas>
119+
$elementId = htmlspecialchars($element, ENT_QUOTES, 'UTF-8');
120+
$chartId = 'chart_' . uniqid();
121+
122+
return '<canvas id="' . $elementId . '"></canvas>
119123
<script>
120-
new Chart(
121-
document.getElementById("' . $element . '").getContext("2d"),
122-
' . $chart->toJson() . '
123-
);
124+
(function() {
125+
const canvas = document.getElementById("' . $elementId . '");
126+
if (!canvas) {
127+
console.error("Canvas element not found: ' . $elementId . '");
128+
return;
129+
}
130+
const ctx = canvas.getContext("2d");
131+
const ' . $chartId . ' = new Chart(ctx, ' . $chart->toJson() . ');
132+
133+
// Store chart instance for potential external access
134+
if (typeof window.chartInstances === "undefined") {
135+
window.chartInstances = {};
136+
}
137+
window.chartInstances["' . $elementId . '"] = ' . $chartId . ';
138+
139+
// Cleanup on page unload to prevent memory leaks
140+
window.addEventListener("unload", function() {
141+
if (window.chartInstances["' . $elementId . '"]) {
142+
' . $chartId . '.destroy();
143+
delete window.chartInstances["' . $elementId . '"];
144+
}
145+
});
146+
})();
124147
</script>';
125148
}
126149
}

tests/ChartTest.php

Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -329,4 +329,108 @@ public function test_can_set_dataset_as_an_array()
329329

330330
$this->assertEquals($expected, $this->chart->get());
331331
}
332+
333+
/**
334+
* Test if invalid JSON throws JsonException
335+
*/
336+
public function test_invalid_json_throws_exception()
337+
{
338+
$this->expectException(\JsonException::class);
339+
340+
// Create an invalid UTF-8 sequence to force JSON encoding error
341+
$data = new Data();
342+
$dataset = new Dataset();
343+
$dataset->data = ["\xFF"]; // Invalid UTF-8 sequence
344+
$data->datasets([$dataset]);
345+
346+
$this->chart->data($data);
347+
$this->chart->toJson();
348+
}
349+
350+
/**
351+
* Test if HTML special characters are properly escaped
352+
*/
353+
public function test_html_special_chars_are_escaped()
354+
{
355+
$maliciousId = '"><script>alert("xss")</script>';
356+
$html = $this->chart->toHtml($maliciousId);
357+
358+
$this->assertStringContains('id="&quot;&gt;&lt;script&gt;alert(&quot;xss&quot;)&lt;/script&gt;"', $html);
359+
$this->assertStringNotContains($maliciousId, $html);
360+
}
361+
362+
/**
363+
* Test if chart cleanup code is present
364+
*/
365+
public function test_chart_cleanup_code_is_present()
366+
{
367+
$html = $this->chart->toHtml('test-chart');
368+
$chartId = 'chart_' . substr($html, strpos($html, 'chart_') + 6, 13); // Extract the dynamic chart ID
369+
370+
$this->assertStringContains('window.addEventListener("unload"', $html);
371+
$this->assertStringContains($chartId . '.destroy()', $html);
372+
}
373+
374+
/**
375+
* Test if JSON output properly escapes HTML characters
376+
*/
377+
public function test_json_escapes_html_characters()
378+
{
379+
$data = new Data;
380+
$datasets = [
381+
(new Dataset)->data([1])->label('<script>alert("xss")</script>')
382+
];
383+
$data->datasets($datasets);
384+
$this->chart->data($data);
385+
386+
$json = $this->chart->toJson();
387+
388+
$this->assertStringNotContains('<script>', $json);
389+
$this->assertStringContains('\u003Cscript\u003E', $json);
390+
}
391+
392+
/**
393+
* Test if chart instances are properly tracked and cleaned up
394+
*/
395+
public function test_chart_instances_are_tracked()
396+
{
397+
$html = $this->chart->toHtml('test-chart');
398+
399+
$this->assertStringContains('window.chartInstances', $html);
400+
$this->assertStringContains('window.chartInstances["test-chart"]', $html);
401+
$this->assertStringContains('delete window.chartInstances', $html);
402+
}
403+
404+
/**
405+
* Test if error handling for missing canvas is present
406+
*/
407+
public function test_canvas_error_handling()
408+
{
409+
$html = $this->chart->toHtml('test-chart');
410+
411+
$this->assertStringContains('if (!canvas)', $html);
412+
$this->assertStringContains('console.error', $html);
413+
}
414+
415+
/**
416+
* Helper method to assert string contains substring
417+
*/
418+
private function assertStringContains(string $needle, string $haystack): void
419+
{
420+
$this->assertTrue(
421+
str_contains($haystack, $needle),
422+
"Failed asserting that '$haystack' contains '$needle'"
423+
);
424+
}
425+
426+
/**
427+
* Helper method to assert string does not contain substring
428+
*/
429+
private function assertStringNotContains(string $needle, string $haystack): void
430+
{
431+
$this->assertFalse(
432+
str_contains($haystack, $needle),
433+
"Failed asserting that '$haystack' does not contain '$needle'"
434+
);
435+
}
332436
}

0 commit comments

Comments
 (0)