mirror of
https://github.com/InvoiceShelf/InvoiceShelf.git
synced 2026-06-01 07:59:01 +00:00
Sanitize PDF address fields against SSRF, not just notes
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 <img src='http://attacker/probe'>" 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.
This commit is contained in:
@@ -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()
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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('</p>', '<br />', $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);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -34,3 +34,39 @@ it('normalizes legacy closing-br markup so lines are not collapsed in PDF output
|
||||
expect($out)->toContain('<br')->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 <img src='http://attacker/probe'>" has
|
||||
// been substituted into an address template via {BILLING_ADDRESS_NAME}.
|
||||
$html = "Acme <img src='http://attacker.test/probe'><br />123 Main St<br />Springfield";
|
||||
|
||||
$out = PdfHtmlSanitizer::sanitize($html);
|
||||
|
||||
expect($out)->not->toContain('<img');
|
||||
expect($out)->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 = '<iframe src="http://attacker/x"></iframe><link rel="stylesheet" href="http://attacker/y.css">Hello';
|
||||
|
||||
$out = PdfHtmlSanitizer::sanitize($html);
|
||||
|
||||
expect($out)->not->toContain('<iframe');
|
||||
expect($out)->not->toContain('<link');
|
||||
expect($out)->not->toContain('attacker');
|
||||
expect($out)->toContain('Hello');
|
||||
});
|
||||
|
||||
it('strips on* event handler attributes from allowed tags', function () {
|
||||
$html = '<p onload="alert(1)" onclick="x">click me</p>';
|
||||
|
||||
$out = PdfHtmlSanitizer::sanitize($html);
|
||||
|
||||
expect($out)->not->toContain('onload')->not->toContain('onclick')->not->toContain('alert');
|
||||
expect($out)->toContain('click me');
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user