From 34db4b7897e4cd6fc24d81384a88e6a0a4892a63 Mon Sep 17 00:00:00 2001 From: Darko Gjorgjijoski Date: Tue, 7 Apr 2026 20:33:15 +0200 Subject: [PATCH] Sanitize PDF address fields against SSRF, not just notes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the residual surface from the three published SSRF advisories (GHSA-pc5v-8xwc-v9xq, GHSA-38hf-fq8x-q49r, GHSA-q9wx-ggwq-mcgh / CVE-2026-34365 to 34367). The original fix in 07757e74 only sanitized the Notes field via Invoice/Estimate/Payment::getNotes(), but the same blade templates also render company/billing/shipping address fields with {!! !!} (Blade unescaped output). Those address strings are produced by getCompanyAddress(), getCustomerBillingAddress(), getCustomerShippingAddress() which feed into GeneratesPdfTrait::getFormattedString() — and that method does not call PdfHtmlSanitizer. Customer-controlled fields (name, street, phone, custom field values) are substituted into address templates via getFieldsArray() without HTML-escaping. A malicious customer name like "Acme " therefore reaches Dompdf as raw HTML through the address path, exactly the same CWE-918 SSRF pattern the advisories describe — only blocked today by the secondary defense of dompdf's enable_remote=false. If a self-hoster sets DOMPDF_ENABLE_REMOTE=true for legitimate remote logos, the address surface immediately re-opens. Move the PdfHtmlSanitizer::sanitize() call into the chokepoint at GeneratesPdfTrait::getFormattedString(), so all four sinks — notes plus the three address fields, on all three models — get the same treatment via a single call site. The explicit wrapper in each model's getNotes() becomes redundant and is removed (along with the now-unused App\Support\PdfHtmlSanitizer imports). Verified getFormattedString() is only called from PDF code paths (no email body callers, which use strtr() directly) so there is no risk of stripping useful HTML from a non-PDF context. Extends tests/Unit/PdfHtmlSanitizerTest.php with three new cases covering the address-template scenario, iframe/link tag stripping, and on* event handler removal. All 8 tests pass via vendor/bin/pest tests/Unit/PdfHtmlSanitizerTest.php. --- app/Models/Estimate.php | 3 +-- app/Models/Invoice.php | 3 +-- app/Models/Payment.php | 3 +-- app/Traits/GeneratesPdfTrait.php | 7 +++++- tests/Unit/PdfHtmlSanitizerTest.php | 36 +++++++++++++++++++++++++++++ 5 files changed, 45 insertions(+), 7 deletions(-) diff --git a/app/Models/Estimate.php b/app/Models/Estimate.php index cafacfc1..353996fc 100644 --- a/app/Models/Estimate.php +++ b/app/Models/Estimate.php @@ -8,7 +8,6 @@ use App\Facades\PDF; use App\Mail\SendEstimateMail; use App\Services\SerialNumberFormatter; use App\Space\PdfTemplateUtils; -use App\Support\PdfHtmlSanitizer; use App\Traits\GeneratesPdfTrait; use App\Traits\HasCustomFieldsTrait; use Carbon\Carbon; @@ -476,7 +475,7 @@ class Estimate extends Model implements HasMedia public function getNotes() { - return PdfHtmlSanitizer::sanitize($this->getFormattedString($this->notes)); + return $this->getFormattedString($this->notes); } public function getEmailAttachmentSetting() diff --git a/app/Models/Invoice.php b/app/Models/Invoice.php index a56e035b..fb13ce52 100644 --- a/app/Models/Invoice.php +++ b/app/Models/Invoice.php @@ -8,7 +8,6 @@ use App\Facades\PDF; use App\Mail\SendInvoiceMail; use App\Services\SerialNumberFormatter; use App\Space\PdfTemplateUtils; -use App\Support\PdfHtmlSanitizer; use App\Traits\GeneratesPdfTrait; use App\Traits\HasCustomFieldsTrait; use Carbon\Carbon; @@ -657,7 +656,7 @@ class Invoice extends Model implements HasMedia public function getNotes() { - return PdfHtmlSanitizer::sanitize($this->getFormattedString($this->notes)); + return $this->getFormattedString($this->notes); } public function getEmailString($body) diff --git a/app/Models/Payment.php b/app/Models/Payment.php index e3b3193f..b52a1e58 100644 --- a/app/Models/Payment.php +++ b/app/Models/Payment.php @@ -6,7 +6,6 @@ use App\Facades\Hashids; use App\Jobs\GeneratePaymentPdfJob; use App\Mail\SendPaymentMail; use App\Services\SerialNumberFormatter; -use App\Support\PdfHtmlSanitizer; use App\Traits\GeneratesPdfTrait; use App\Traits\HasCustomFieldsTrait; use Barryvdh\DomPDF\Facade\Pdf as PDF; @@ -434,7 +433,7 @@ class Payment extends Model implements HasMedia public function getNotes() { - return PdfHtmlSanitizer::sanitize($this->getFormattedString($this->notes)); + return $this->getFormattedString($this->notes); } public function getEmailBody($body) diff --git a/app/Traits/GeneratesPdfTrait.php b/app/Traits/GeneratesPdfTrait.php index 623d33e8..3c03ff6c 100644 --- a/app/Traits/GeneratesPdfTrait.php +++ b/app/Traits/GeneratesPdfTrait.php @@ -5,6 +5,7 @@ namespace App\Traits; use App\Models\Address; use App\Models\CompanySetting; use App\Models\FileDisk; +use App\Support\PdfHtmlSanitizer; use Carbon\Carbon; use Illuminate\Support\Facades\App; @@ -182,6 +183,10 @@ trait GeneratesPdfTrait $str = str_replace('

', '
', $str); - return $str; + // Sanitize the assembled HTML to strip any SSRF vectors that may have + // entered through user-supplied address fields, customer names, or + // custom field values. Notes also pass through this method, so they + // get the same treatment without needing a separate wrapper. + return PdfHtmlSanitizer::sanitize($str); } } diff --git a/tests/Unit/PdfHtmlSanitizerTest.php b/tests/Unit/PdfHtmlSanitizerTest.php index d94120f4..12f8f152 100644 --- a/tests/Unit/PdfHtmlSanitizerTest.php +++ b/tests/Unit/PdfHtmlSanitizerTest.php @@ -34,3 +34,39 @@ it('normalizes legacy closing-br markup so lines are not collapsed in PDF output expect($out)->toContain('toContain('line1')->toContain('line2'); expect($out)->not->toBe('line1line2'); }); + +it('strips SSRF vectors injected via address-template placeholders', function () { + // Simulates the output of GeneratesPdfTrait::getFormattedString() after a + // malicious customer name like "Acme " has + // been substituted into an address template via {BILLING_ADDRESS_NAME}. + $html = "Acme
123 Main St
Springfield"; + + $out = PdfHtmlSanitizer::sanitize($html); + + expect($out)->not->toContain('not->toContain('src='); + expect($out)->not->toContain('attacker.test'); + expect($out)->toContain('Acme'); + expect($out)->toContain('123 Main St'); + expect($out)->toContain('Springfield'); +}); + +it('strips iframe and link tags that could trigger SSRF', function () { + $html = 'Hello'; + + $out = PdfHtmlSanitizer::sanitize($html); + + expect($out)->not->toContain('not->toContain('not->toContain('attacker'); + expect($out)->toContain('Hello'); +}); + +it('strips on* event handler attributes from allowed tags', function () { + $html = '

click me

'; + + $out = PdfHtmlSanitizer::sanitize($html); + + expect($out)->not->toContain('onload')->not->toContain('onclick')->not->toContain('alert'); + expect($out)->toContain('click me'); +});