Skip to content

fix: prevent PDF export crash on tables with nested quotes in inline styles - #86

Open
Herafia wants to merge 5 commits into
mainfrom
fix/tcpdf-nested-quotes-crash
Open

fix: prevent PDF export crash on tables with nested quotes in inline styles#86
Herafia wants to merge 5 commits into
mainfrom
fix/tcpdf-nested-quotes-crash

Conversation

@Herafia

@Herafia Herafia commented Aug 25, 2026

Copy link
Copy Markdown

Checklist before requesting a review

Please delete options that are not relevant.

  • I have performed a self-review of my code.
  • I have added tests (when available) that prove my fix is effective or that my feature works.
  • I have updated the CHANGELOG with a short functional description of the fix or new feature.
  • This change requires a documentation update.

Description

  • It fixes !45825
  • The PDF export would crash on tickets whose descriptions contained an html table with styles using url(‘...’) (nested quotes). The regular expressions in cleanTableHtml would break the html at these quotes, corrupting the table structure and causing tcpdf to lose track of the cells.

Fix: Replaced the regular expressions with DOMDocument/DOMXPath parsing in cleanTableHtml, which is robust against nested quotes.

@Herafia Herafia changed the title fix: prevent PDF export crash on tables with nested quotes in inline … fix: prevent PDF export crash on tables with nested quotes in inline styles Aug 25, 2026
@Herafia Herafia self-assigned this Aug 25, 2026
@Herafia
Herafia requested review from Rom1-B and stonebuzz August 25, 2026 12:17
Comment thread inc/simplepdf.class.php

// 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

Copy link
Copy Markdown

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-quote url('...') style value (case from !45825)? Verified locally that the old regex code produced a <table> with duplicate border/style attributes on this input, the actual crash mechanism, and that the new code doesn't; a test would pin the regression.

@Herafia
Herafia requested a review from Rom1-B August 25, 2026 14:42
Comment thread tests/bootstrap.php
Comment on lines +33 to +35
use function Safe\realpath;

$current_plugin_folder = basename(realpath(__DIR__ . '/../'));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use pluginsGLPI/empty for example

Suggested change
use function Safe\realpath;
$current_plugin_folder = basename(realpath(__DIR__ . '/../'));
$current_plugin_folder = basename(dirname(__DIR__));

Comment thread phpunit.xml
Comment on lines +1 to +7
<phpunit bootstrap="tests/bootstrap.php" colors="true" testdox="true">
<testsuites>
<testsuite name="Tests">
<directory>tests</directory>
</testsuite>
</testsuites>
</phpunit>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<phpunit bootstrap="tests/bootstrap.php" colors="true" testdox="true">
<testsuites>
<testsuite name="Tests">
<directory>tests</directory>
</testsuite>
</testsuites>
</phpunit>
<phpunit
bootstrap="tests/bootstrap.php"
colors="true"
testdox="true"
cacheDirectory="var/phpunit"
>
<source>
<include>
<directory>src</directory>
</include>
</source>
<testsuites>
<testsuite name="Tests">
<directory suffix="Test.php">tests</directory>
</testsuite>
</testsuites>
</phpunit>

And add var/ in .gitignore

Comment thread inc/simplepdf.class.php
// 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) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

explode(';', ...) also splits inside url(...), so a data URI is rebuilt with an extra space and becomes invalid: url(data:image/png;base64,X) comes out as url(data:image/png; base64,X).

Suggested change
foreach (explode(';', $style) as $declaration) {
foreach (preg_split('/;(?![^(]*\))/', $style) as $declaration) {

Comment thread inc/simplepdf.class.php
// 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);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

libxml_use_internal_errors(true) is never restored, so libxml error reporting stays silenced for the rest of the request.

Suggested change
libxml_use_internal_errors(true);
$libxml_previous_state = libxml_use_internal_errors(true);

and after libxml_clear_errors(); (line 481):

        libxml_use_internal_errors($libxml_previous_state);

Comment thread inc/simplepdf.class.php
Comment on lines +489 to +491
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);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DOMXPath is rebuilt on every table; hoist it out of the loop.

Suggested change
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);
$xpath = new DOMXPath($dom);
foreach (iterator_to_array($dom->getElementsByTagName('table')) as $table) {
// Remove width/height (attributes and styles) on the table and its rows/cells

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants