-
Notifications
You must be signed in to change notification settings - Fork 14
fix: prevent PDF export crash on tables with nested quotes in inline styles #86
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -468,30 +468,89 @@ private function cleanTableHtml($html) | |||||||||||||
| // Remove colgroup entirely (causes fixed widths) | ||||||||||||||
| $html = preg_replace('/<colgroup\b[^>]*>.*?<\/colgroup>/is', '', $html); | ||||||||||||||
|
|
||||||||||||||
| // Remove table-layout:fixed style (prevents auto-sizing) | ||||||||||||||
| $html = preg_replace('/table-layout\s*:\s*fixed\s*;?/i', '', $html); | ||||||||||||||
| // Parse with DOMDocument rather than regexes: style/attribute values coming | ||||||||||||||
| // from pasted web content can contain nested quotes (e.g. style="...url('...')..."), | ||||||||||||||
| // which regex-based quote matching cannot handle reliably and ends up corrupting | ||||||||||||||
| // the markup fed to TCPDF (causing crashes on malformed HTML). | ||||||||||||||
| $dom = new DOMDocument(); | ||||||||||||||
| libxml_use_internal_errors(true); | ||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
and after |
||||||||||||||
| $dom->loadHTML( | ||||||||||||||
| '<?xml encoding="utf-8" ?><div>' . $html . '</div>', | ||||||||||||||
| LIBXML_HTML_NOIMPLIED | LIBXML_HTML_NODEFDTD, | ||||||||||||||
| ); | ||||||||||||||
| libxml_clear_errors(); | ||||||||||||||
|
|
||||||||||||||
| $wrapper = $dom->getElementsByTagName('div')->item(0); | ||||||||||||||
| if ($wrapper === null) { | ||||||||||||||
| // Fallback: parsing failed unexpectedly, keep original content rather than losing it | ||||||||||||||
| return $html; | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| foreach (iterator_to_array($dom->getElementsByTagName('table')) as $table) { | ||||||||||||||
| // Remove width/height (attributes and styles) on the table and its rows/cells | ||||||||||||||
| $xpath = new DOMXPath($dom); | ||||||||||||||
|
Comment on lines
+489
to
+491
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||
| foreach ($xpath->query('.//td | .//th | .//tr | .', $table) as $node) { | ||||||||||||||
| if (!($node instanceof DOMElement)) { | ||||||||||||||
| continue; | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| $node->removeAttribute('width'); | ||||||||||||||
| $node->removeAttribute('height'); | ||||||||||||||
|
|
||||||||||||||
| $style = $this->removeStyleProperties($node->getAttribute('style'), ['width', 'height', 'table-layout']); | ||||||||||||||
| if ($style === '') { | ||||||||||||||
| $node->removeAttribute('style'); | ||||||||||||||
| } else { | ||||||||||||||
| $node->setAttribute('style', $style); | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| // Add border to table if missing (for visibility) | ||||||||||||||
| if (!$table->hasAttribute('border') || (int) $table->getAttribute('border') < 1) { | ||||||||||||||
| $table->setAttribute('border', '1'); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| // Remove width/height styles only from table elements (table, td, th, tr) | ||||||||||||||
| $html = preg_replace('/(<(?:table|td|th|tr)\b[^>]*)\s+style\s*=\s*["\']([^"\']*)\bwidth\s*:\s*[^;"\'>]+;?([^"\']*)["\']/', '$1 style="$2$3"', $html); | ||||||||||||||
| $html = preg_replace('/(<(?:table|td|th|tr)\b[^>]*)\s+style\s*=\s*["\']([^"\']*)\bheight\s*:\s*[^;"\'>]+;?([^"\']*)["\']/', '$1 style="$2$3"', $html); | ||||||||||||||
| // Force table to 100% width for PDF (do this LAST) | ||||||||||||||
| $style = trim($table->getAttribute('style') . ';width:100%;', ';'); | ||||||||||||||
| $table->setAttribute('style', $style); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| // Remove width/height attributes only from table elements (table, td, th, tr) | ||||||||||||||
| $html = preg_replace('/(<(?:table|td|th|tr)\b[^>]+)\s+width\s*=\s*["\']?[^"\'\s>]+["\']?/i', '$1', $html); | ||||||||||||||
| $html = preg_replace('/(<(?:table|td|th|tr)\b[^>]+)\s+height\s*=\s*["\']?[^"\'\s>]+["\']?/i', '$1', $html); | ||||||||||||||
| $output = ''; | ||||||||||||||
| foreach (iterator_to_array($wrapper->childNodes) as $child) { | ||||||||||||||
| $output .= $dom->saveHTML($child); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| // Clean up empty style attributes and double spaces | ||||||||||||||
| $html = preg_replace('/\s+style\s*=\s*["\'][\s]*["\']/', '', $html); | ||||||||||||||
| $html = preg_replace('/\s+/', ' ', $html); | ||||||||||||||
| return $output; | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| // Add border to table if missing (for visibility) | ||||||||||||||
| if (!preg_match('/border\s*=\s*["\']?[1-9]/i', $html)) { | ||||||||||||||
| $html = preg_replace('/<table/i', '<table border="1"', $html, 1); | ||||||||||||||
| /** | ||||||||||||||
| * Remove the given CSS properties from an inline style declaration. | ||||||||||||||
| * | ||||||||||||||
| * @param $style string inline style attribute content | ||||||||||||||
| * @param $properties array lowercase property names to strip | ||||||||||||||
| * | ||||||||||||||
| * @return string | ||||||||||||||
| **/ | ||||||||||||||
| private function removeStyleProperties($style, array $properties) | ||||||||||||||
| { | ||||||||||||||
| if (trim($style) === '') { | ||||||||||||||
| return ''; | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| // Force table to 100% width for PDF (do this LAST) | ||||||||||||||
| $html = preg_replace('/<table([^>]*)>/i', '<table$1 style="width:100%">', $html, 1); | ||||||||||||||
| $kept = []; | ||||||||||||||
| foreach (explode(';', $style) as $declaration) { | ||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||
| $declaration = trim($declaration); | ||||||||||||||
| if ($declaration === '') { | ||||||||||||||
| continue; | ||||||||||||||
| } | ||||||||||||||
| $property = strtolower(trim(explode(':', $declaration, 2)[0])); | ||||||||||||||
| if (in_array($property, $properties, true)) { | ||||||||||||||
| continue; | ||||||||||||||
| } | ||||||||||||||
| $kept[] = $declaration; | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| return $html; | ||||||||||||||
| return implode('; ', $kept); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| /** | ||||||||||||||
|
|
||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,7 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| <phpunit bootstrap="tests/bootstrap.php" colors="true" testdox="true"> | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| <testsuites> | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| <testsuite name="Tests"> | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| <directory>tests</directory> | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| </testsuite> | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| </testsuites> | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| </phpunit> | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+1
to
+7
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
And add |
||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,78 @@ | ||
| <?php | ||
|
|
||
| /** | ||
| * ------------------------------------------------------------------------- | ||
| * LICENSE | ||
| * | ||
| * This file is part of PDF plugin for GLPI. | ||
| * | ||
| * PDF is free software: you can redistribute it and/or modify | ||
| * it under the terms of the GNU Affero General Public License as published by | ||
| * the Free Software Foundation, either version 3 of the License, or | ||
| * (at your option) any later version. | ||
| * | ||
| * PDF is distributed in the hope that it will be useful, | ||
| * but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
| * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
| * GNU Affero General Public License for more details. | ||
| * | ||
| * You should have received a copy of the GNU Affero General Public License | ||
| * along with Reports. If not, see <http://www.gnu.org/licenses/>. | ||
| * | ||
| * @author Nelly Mahu-Lasson, Remi Collet, Teclib | ||
| * @copyright Copyright (c) 2009-2022 PDF plugin team | ||
| * @license AGPL License 3.0 or (at your option) any later version | ||
| * @link https://github.com/pluginsGLPI/pdf/ | ||
| * @link http://www.glpi-project.org/ | ||
| * @package pdf | ||
| * @since 2009 | ||
| * http://www.gnu.org/licenses/agpl-3.0-standalone.html | ||
| * -------------------------------------------------------------------------- | ||
| */ | ||
|
|
||
| declare(strict_types=1); | ||
|
|
||
| namespace GlpiPlugin\Pdf\Tests\Units; | ||
|
|
||
| use Glpi\Tests\GLPITestCase; | ||
| use PluginPdfSimplePDF; | ||
| use ReflectionMethod; | ||
|
|
||
| class SimplePdfTest extends GLPITestCase | ||
| { | ||
| /** | ||
| * Test nested quote url in a style attribute | ||
| * used to break cleanTableHtml regex and crash TCPDF | ||
| */ | ||
| public function testCleanTableHtmlWithNestedQuoteUrlStyle(): void | ||
| { | ||
| $html = <<<HTML | ||
| <table style="width: 614px; border-collapse: collapse;" border="0" width="614"> | ||
| <tbody> | ||
| <tr> | ||
| <td style="width: 51.8pt; height: 15.75pt; background-image: url('https://example.com/pics/a.jpg');">Content</td> | ||
| </tr> | ||
| </tbody> | ||
| </table> | ||
| HTML; | ||
|
|
||
| $pdf = new PluginPdfSimplePDF(); | ||
|
|
||
| $method = new ReflectionMethod(PluginPdfSimplePDF::class, 'cleanTableHtml'); | ||
| $cleaned = $method->invoke($pdf, $html); | ||
|
|
||
| // no duplicated attribute on <table> | ||
| $this->assertMatchesRegularExpression('/^<table[^>]*>/', $cleaned); | ||
| preg_match('/^<table([^>]*)>/', $cleaned, $matches); | ||
| $tableAttributes = $matches[1]; | ||
|
|
||
| $this->assertSame(1, substr_count($tableAttributes, 'border='), 'the <table> tag must have a single border attribute'); | ||
| $this->assertSame(1, substr_count($tableAttributes, 'style='), 'the <table> tag must have a single style attribute'); | ||
|
|
||
| // width/height stripped, nested url untouched | ||
| $this->assertStringNotContainsStringIgnoringCase('width: 614px', $cleaned); | ||
| $this->assertStringNotContainsStringIgnoringCase('width: 51.8pt', $cleaned); | ||
| $this->assertStringNotContainsStringIgnoringCase('height: 15.75pt', $cleaned); | ||
| $this->assertStringContainsString("url('https://example.com/pics/a.jpg')", $cleaned); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,42 @@ | ||||||||||
| <?php | ||||||||||
|
|
||||||||||
| /** | ||||||||||
| * ------------------------------------------------------------------------- | ||||||||||
| * LICENSE | ||||||||||
| * | ||||||||||
| * This file is part of PDF plugin for GLPI. | ||||||||||
| * | ||||||||||
| * PDF is free software: you can redistribute it and/or modify | ||||||||||
| * it under the terms of the GNU Affero General Public License as published by | ||||||||||
| * the Free Software Foundation, either version 3 of the License, or | ||||||||||
| * (at your option) any later version. | ||||||||||
| * | ||||||||||
| * PDF is distributed in the hope that it will be useful, | ||||||||||
| * but WITHOUT ANY WARRANTY; without even the implied warranty of | ||||||||||
| * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||||||||||
| * GNU Affero General Public License for more details. | ||||||||||
| * | ||||||||||
| * You should have received a copy of the GNU Affero General Public License | ||||||||||
| * along with Reports. If not, see <http://www.gnu.org/licenses/>. | ||||||||||
| * | ||||||||||
| * @author Nelly Mahu-Lasson, Remi Collet, Teclib | ||||||||||
| * @copyright Copyright (c) 2009-2022 PDF plugin team | ||||||||||
| * @license AGPL License 3.0 or (at your option) any later version | ||||||||||
| * @link https://github.com/pluginsGLPI/pdf/ | ||||||||||
| * @link http://www.glpi-project.org/ | ||||||||||
| * @package pdf | ||||||||||
| * @since 2009 | ||||||||||
| * http://www.gnu.org/licenses/agpl-3.0-standalone.html | ||||||||||
| * -------------------------------------------------------------------------- | ||||||||||
| */ | ||||||||||
|
|
||||||||||
| use function Safe\realpath; | ||||||||||
|
|
||||||||||
| $current_plugin_folder = basename(realpath(__DIR__ . '/../')); | ||||||||||
|
Comment on lines
+33
to
+35
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please use pluginsGLPI/empty for example
Suggested change
|
||||||||||
|
|
||||||||||
| require __DIR__ . '/../../../tests/bootstrap.php'; | ||||||||||
| require dirname(__DIR__) . '/vendor/autoload.php'; | ||||||||||
|
|
||||||||||
| if (!Plugin::isPluginActive($current_plugin_folder)) { | ||||||||||
| throw new RuntimeException(sprintf('Plugin %s is not active in the test database', $current_plugin_folder)); | ||||||||||
| } | ||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a unit test for
cleanTableHtml()with a nested-quoteurl('...')style value (case from !45825)? Verified locally that the old regex code produced a<table>with duplicateborder/styleattributes on this input, the actual crash mechanism, and that the new code doesn't; a test would pin the regression.