diff --git a/app/Facades/Pdf.php b/app/Facades/Pdf.php index e2e05a2a..bb96850f 100644 --- a/app/Facades/Pdf.php +++ b/app/Facades/Pdf.php @@ -5,7 +5,7 @@ namespace App\Facades; use Illuminate\Support\Facades\Facade; /** - * @method static \Psr\Http\Message\ResponseInterface loadView(string $template) + * @method static \App\Support\Pdf\ResponseStream loadView(string $template) */ class Pdf extends Facade { diff --git a/app/Http/Controllers/Company/Report/CustomerSalesReportController.php b/app/Http/Controllers/Company/Report/CustomerSalesReportController.php index b9100f80..079f5fbe 100644 --- a/app/Http/Controllers/Company/Report/CustomerSalesReportController.php +++ b/app/Http/Controllers/Company/Report/CustomerSalesReportController.php @@ -12,6 +12,7 @@ use Carbon\Carbon; use Illuminate\Http\JsonResponse; use Illuminate\Http\Request; use Illuminate\Support\Facades\App; +use Silber\Bouncer\BouncerFacade; class CustomerSalesReportController extends Controller { @@ -23,7 +24,14 @@ class CustomerSalesReportController extends Controller */ public function __invoke(Request $request, $hash) { - $company = Company::where('unique_hash', $hash)->first(); + $company = Company::where('unique_hash', $hash)->firstOrFail(); + + // These routes carry no company header, so ScopeBouncer is not in their + // middleware stack and the ability scope was never set. 'view-financial-reports' + // is stored scoped to a company, so the unscoped check always failed and every + // report PDF answered 403. Scope to the company named in the URL: the policy + // still checks membership, so this grants nothing new. + BouncerFacade::scope()->to($company->id); $this->authorize('view report', $company); diff --git a/app/Http/Controllers/Company/Report/ExpensesReportController.php b/app/Http/Controllers/Company/Report/ExpensesReportController.php index 3743af64..62bfd3d3 100644 --- a/app/Http/Controllers/Company/Report/ExpensesReportController.php +++ b/app/Http/Controllers/Company/Report/ExpensesReportController.php @@ -13,6 +13,7 @@ use Illuminate\Contracts\View\View; use Illuminate\Http\Request; use Illuminate\Http\Response; use Illuminate\Support\Facades\App; +use Silber\Bouncer\BouncerFacade; class ExpensesReportController extends Controller { @@ -24,7 +25,14 @@ class ExpensesReportController extends Controller */ public function __invoke(Request $request, $hash) { - $company = Company::where('unique_hash', $hash)->first(); + $company = Company::where('unique_hash', $hash)->firstOrFail(); + + // These routes carry no company header, so ScopeBouncer is not in their + // middleware stack and the ability scope was never set. 'view-financial-reports' + // is stored scoped to a company, so the unscoped check always failed and every + // report PDF answered 403. Scope to the company named in the URL: the policy + // still checks membership, so this grants nothing new. + BouncerFacade::scope()->to($company->id); $this->authorize('view report', $company); diff --git a/app/Http/Controllers/Company/Report/ItemSalesReportController.php b/app/Http/Controllers/Company/Report/ItemSalesReportController.php index dc7b91dd..eb363f02 100644 --- a/app/Http/Controllers/Company/Report/ItemSalesReportController.php +++ b/app/Http/Controllers/Company/Report/ItemSalesReportController.php @@ -12,6 +12,7 @@ use Carbon\Carbon; use Illuminate\Http\JsonResponse; use Illuminate\Http\Request; use Illuminate\Support\Facades\App; +use Silber\Bouncer\BouncerFacade; class ItemSalesReportController extends Controller { @@ -23,7 +24,14 @@ class ItemSalesReportController extends Controller */ public function __invoke(Request $request, $hash) { - $company = Company::where('unique_hash', $hash)->first(); + $company = Company::where('unique_hash', $hash)->firstOrFail(); + + // These routes carry no company header, so ScopeBouncer is not in their + // middleware stack and the ability scope was never set. 'view-financial-reports' + // is stored scoped to a company, so the unscoped check always failed and every + // report PDF answered 403. Scope to the company named in the URL: the policy + // still checks membership, so this grants nothing new. + BouncerFacade::scope()->to($company->id); $this->authorize('view report', $company); diff --git a/app/Http/Controllers/Company/Report/ProfitLossReportController.php b/app/Http/Controllers/Company/Report/ProfitLossReportController.php index ebe42538..6dfdfbe6 100644 --- a/app/Http/Controllers/Company/Report/ProfitLossReportController.php +++ b/app/Http/Controllers/Company/Report/ProfitLossReportController.php @@ -13,6 +13,7 @@ use Carbon\Carbon; use Illuminate\Http\JsonResponse; use Illuminate\Http\Request; use Illuminate\Support\Facades\App; +use Silber\Bouncer\BouncerFacade; class ProfitLossReportController extends Controller { @@ -24,7 +25,14 @@ class ProfitLossReportController extends Controller */ public function __invoke(Request $request, $hash) { - $company = Company::where('unique_hash', $hash)->first(); + $company = Company::where('unique_hash', $hash)->firstOrFail(); + + // These routes carry no company header, so ScopeBouncer is not in their + // middleware stack and the ability scope was never set. 'view-financial-reports' + // is stored scoped to a company, so the unscoped check always failed and every + // report PDF answered 403. Scope to the company named in the URL: the policy + // still checks membership, so this grants nothing new. + BouncerFacade::scope()->to($company->id); $this->authorize('view report', $company); diff --git a/app/Http/Controllers/Company/Report/TaxSummaryReportController.php b/app/Http/Controllers/Company/Report/TaxSummaryReportController.php index a8b1e0a1..e19f722a 100644 --- a/app/Http/Controllers/Company/Report/TaxSummaryReportController.php +++ b/app/Http/Controllers/Company/Report/TaxSummaryReportController.php @@ -12,6 +12,7 @@ use Carbon\Carbon; use Illuminate\Http\JsonResponse; use Illuminate\Http\Request; use Illuminate\Support\Facades\App; +use Silber\Bouncer\BouncerFacade; class TaxSummaryReportController extends Controller { @@ -23,7 +24,14 @@ class TaxSummaryReportController extends Controller */ public function __invoke(Request $request, $hash) { - $company = Company::where('unique_hash', $hash)->first(); + $company = Company::where('unique_hash', $hash)->firstOrFail(); + + // These routes carry no company header, so ScopeBouncer is not in their + // middleware stack and the ability scope was never set. 'view-financial-reports' + // is stored scoped to a company, so the unscoped check always failed and every + // report PDF answered 403. Scope to the company named in the URL: the policy + // still checks membership, so this grants nothing new. + BouncerFacade::scope()->to($company->id); $this->authorize('view report', $company); diff --git a/app/Support/Pdf/DompdfDriver.php b/app/Support/Pdf/DompdfDriver.php new file mode 100644 index 00000000..e23634ff --- /dev/null +++ b/app/Support/Pdf/DompdfDriver.php @@ -0,0 +1,27 @@ +wrapper()->loadView($template)); + } + + protected function wrapper(): PDF + { + return App::make('dompdf.wrapper'); + } +} diff --git a/app/Support/Pdf/DompdfResponse.php b/app/Support/Pdf/DompdfResponse.php new file mode 100644 index 00000000..5f75ed55 --- /dev/null +++ b/app/Support/Pdf/DompdfResponse.php @@ -0,0 +1,35 @@ +pdf->stream($filename); + } + + public function download(string $filename = 'document.pdf'): Response + { + return $this->pdf->download($filename); + } + + public function output(): string + { + return $this->pdf->output(); + } +} diff --git a/app/Support/Pdf/GotenbergPdfDriver.php b/app/Support/Pdf/GotenbergPdfDriver.php index 2c5cbc05..2461436c 100644 --- a/app/Support/Pdf/GotenbergPdfDriver.php +++ b/app/Support/Pdf/GotenbergPdfDriver.php @@ -6,10 +6,23 @@ use App\Support\Net\BlockedUrlException; use App\Support\Net\PrivateNetworkGuard; use Gotenberg\Gotenberg; use Gotenberg\Stream; +use Psr\Http\Message\RequestInterface; -class GotenbergPdfDriver +class GotenbergPdfDriver implements PdfDriver { - public function loadView(string $viewname): GotenbergPdfResponse + public function loadView(string $template): ResponseStream + { + return new GotenbergPdfResponse(Gotenberg::send($this->buildRequest($template))); + } + + /** + * Assemble the Chromium request without sending it. + * + * Split out so the option wiring can actually be asserted on. Everything + * below this line used to be inlined into loadView(), which meant the only + * way to check that an option was set was to run a Gotenberg service. + */ + public function buildRequest(string $template): RequestInterface { $papersize = explode(' ', config('pdf.connections.gotenberg.papersize')); if (count($papersize) != 2) { @@ -32,18 +45,28 @@ class GotenbergPdfDriver } } - $request = Gotenberg::chromium($host) + return Gotenberg::chromium($host) ->pdf() + // Only affects the root (body/html) background: Chromium paints + // element backgrounds either way, verified against gotenberg:8, so + // no stock template changes. dompdf does paint the body background, + // so this is here to stop a custom template that sets one from + // rendering differently depending on the selected driver. + ->printBackground() + // config/dompdf.php renders as `screen`; Chromium defaults to `print`. + // Align them so a template with media queries behaves the same either + // way rather than depending on which driver is selected. + ->emulateScreenMediaType() ->margins(0, 0, 0, 0) ->paperSize($papersize[0], $papersize[1]) ->html( + // The SDK renames this to index.html regardless of what we pass + // (ChromiumPdf::html()), so name it that way rather than implying + // a choice we do not have. Stream::string( - 'document.html', - view($viewname)->render(), + 'index.html', + view($template)->render(), ) ); - $result = Gotenberg::send($request); - - return new GotenbergPdfResponse($result); } } diff --git a/app/Support/Pdf/GotenbergPdfResponse.php b/app/Support/Pdf/GotenbergPdfResponse.php index 58524cca..2f178fd3 100644 --- a/app/Support/Pdf/GotenbergPdfResponse.php +++ b/app/Support/Pdf/GotenbergPdfResponse.php @@ -5,7 +5,7 @@ namespace App\Support\Pdf; use Illuminate\Http\Response; use Psr\Http\Message\ResponseInterface; -class GotenbergPdfResponse +class GotenbergPdfResponse implements ResponseStream { protected ResponseInterface $response; @@ -16,16 +16,33 @@ class GotenbergPdfResponse public function stream(string $filename = 'document.pdf'): Response { - $output = $this->response->getBody(); + return $this->respond($filename, 'inline'); + } - return new Response($output, 200, [ - 'Content-Type' => 'application/pdf', - 'Content-Disposition' => 'inline; filename="'.$filename.'"', - ]); + public function download(string $filename = 'document.pdf'): Response + { + return $this->respond($filename, 'attachment'); } public function output(): string { - return $this->response->getBody()->getContents(); + $body = $this->response->getBody(); + + // getContents() reads from wherever the stream currently sits, so a + // second call would hand back nothing. Rewind so output() is repeatable + // and safe to mix with stream()/download() on the same instance. + if ($body->isSeekable()) { + $body->rewind(); + } + + return $body->getContents(); + } + + private function respond(string $filename, string $disposition): Response + { + return new Response($this->output(), 200, [ + 'Content-Type' => 'application/pdf', + 'Content-Disposition' => $disposition.'; filename="'.$filename.'"', + ]); } } diff --git a/app/Support/Pdf/PdfDriverFactory.php b/app/Support/Pdf/PdfDriverFactory.php index 91389ed7..0b0657df 100644 --- a/app/Support/Pdf/PdfDriverFactory.php +++ b/app/Support/Pdf/PdfDriverFactory.php @@ -2,14 +2,12 @@ namespace App\Support\Pdf; -use App; - class PdfDriverFactory { - public static function create(string $driver) + public static function create(string $driver): PdfDriver { return match ($driver) { - 'dompdf' => App::make('dompdf.wrapper'), + 'dompdf' => new DompdfDriver, 'gotenberg' => new GotenbergPdfDriver, default => throw new \InvalidArgumentException('Invalid PdfDriver requested') }; diff --git a/app/Support/Pdf/PdfService.php b/app/Support/Pdf/PdfService.php index dd45ad8d..6548ab43 100644 --- a/app/Support/Pdf/PdfService.php +++ b/app/Support/Pdf/PdfService.php @@ -4,7 +4,7 @@ namespace App\Support\Pdf; class PdfService { - public static function loadView(string $template) + public static function loadView(string $template): ResponseStream { $driver = config('pdf.driver'); diff --git a/app/Support/Pdf/ResponseStream.php b/app/Support/Pdf/ResponseStream.php index 046b15ee..3506a175 100644 --- a/app/Support/Pdf/ResponseStream.php +++ b/app/Support/Pdf/ResponseStream.php @@ -4,9 +4,18 @@ namespace App\Support\Pdf; use Illuminate\Http\Response; +/** + * The rendered-document contract every PDF driver returns. + * + * The defaults matter: callers reach for the bare `stream()` / `download()` + * (see the report controllers and GeneratesPdfTrait), so a driver that only + * accepts an explicit filename would break them. + */ interface ResponseStream { - public function stream(string $filename): Response; + public function stream(string $filename = 'document.pdf'): Response; + + public function download(string $filename = 'document.pdf'): Response; public function output(): string; } diff --git a/app/Traits/GeneratesPdfTrait.php b/app/Traits/GeneratesPdfTrait.php index 1b8871be..ccbc9d32 100644 --- a/app/Traits/GeneratesPdfTrait.php +++ b/app/Traits/GeneratesPdfTrait.php @@ -30,7 +30,12 @@ trait GeneratesPdfTrait $pdf = $this->getPDFData(); - return response()->make($pdf->stream(), 200, [ + // ->output(), not ->stream(): stream() already returns a Response, and + // nesting one inside response()->make() stringifies it, prepending the + // whole "HTTP/1.0 200 OK" preamble to the file. Readers scan the first + // kilobyte for %PDF so it looked fine, but the bytes were malformed and + // anything that validates them (PDF/A, extraction tooling) would balk. + return response()->make($pdf->output(), 200, [ 'Content-Type' => 'application/pdf', 'Content-Disposition' => 'inline; filename="'.$this[$collection_name.'_number'].'.pdf"', ]); diff --git a/tests/Feature/Pdf/PdfTemplateRenderTest.php b/tests/Feature/Pdf/PdfTemplateRenderTest.php index 1e054228..d99b0c17 100644 --- a/tests/Feature/Pdf/PdfTemplateRenderTest.php +++ b/tests/Feature/Pdf/PdfTemplateRenderTest.php @@ -37,22 +37,22 @@ function stockPdfTemplates(string $type): array } /** - * Asserts the route returned a PDF. + * Asserts the route returned a PDF, starting at byte zero. * - * Presence, not position: the body currently carries the status line and - * headers of an inner Response ahead of the PDF payload, because - * GeneratesPdfTrait wraps $pdf->stream() — already a Response — in another - * response()->make(). Readers tolerate leading bytes before %PDF, which is why - * nobody has noticed. Asserting presence keeps this test honest about what it - * covers (the render not fatalling) without baking the malformed prefix in as - * expected output. + * This used to only assert %PDF appeared somewhere in the first kilobyte: the + * body carried the status line and headers of an inner Response ahead of the + * payload, because GeneratesPdfTrait wrapped $pdf->stream() — already a Response + * — in another response()->make(). Readers scan for the header so nobody + * noticed, but the bytes were malformed. The trait now passes ->output(), so the + * position can be asserted, and a regression would be caught rather than + * tolerated. */ function assertRenderedPdf(TestResponse $response): void { $response->assertOk(); expect($response->headers->get('content-type'))->toContain('application/pdf'); - expect(substr($response->getContent(), 0, 1024))->toContain('%PDF'); + expect($response->getContent())->toStartWith('%PDF-'); } dataset('invoice templates', fn () => stockPdfTemplates('invoice')); diff --git a/tests/Feature/Pdf/ReportPdfDownloadTest.php b/tests/Feature/Pdf/ReportPdfDownloadTest.php new file mode 100644 index 00000000..f35e5b1a --- /dev/null +++ b/tests/Feature/Pdf/ReportPdfDownloadTest.php @@ -0,0 +1,63 @@ +download(), and + * GotenbergPdfResponse never had that method. Selecting the Gotenberg driver + * therefore turned every report download into a fatal undefined-method error, + * while the same page streamed fine. Nothing caught it because the factory + * returned the vendor dompdf wrapper for one driver and a bespoke class for the + * other, with no shared type between them. + * + * These run against dompdf so they need no Gotenberg service; the contract that + * keeps the two in step is asserted in tests/Unit/PdfDriverContractTest.php. + */ +beforeEach(function () { + Artisan::call('db:seed', ['--class' => 'DatabaseSeeder', '--force' => true]); + Artisan::call('db:seed', ['--class' => 'DemoSeeder', '--force' => true]); + + $user = User::find(1); + $this->company = $user->companies()->first(); + + // Bouncer scopes abilities by company, so `view report` only resolves with + // the company header set. + $this->withHeaders(['company' => $this->company->id]); + + Sanctum::actingAs($user, ['*']); + + config(['pdf.driver' => 'dompdf']); +}); + +dataset('reports', [ + 'sales/customers', + 'sales/items', + 'expenses', + 'tax-summary', + 'profit-loss', +]); + +function reportUrl(string $report, string $hash, string $extra = ''): string +{ + return "/reports/{$report}/{$hash}?from_date=2020-01-01&to_date=2030-12-31{$extra}"; +} + +test('every report streams a pdf', function (string $report) { + $response = get(reportUrl($report, $this->company->unique_hash)); + + $response->assertOk(); + expect($response->headers->get('content-type'))->toContain('application/pdf'); + expect($response->getContent())->toStartWith('%PDF-'); +})->with('reports'); + +test('every report can be downloaded as an attachment', function (string $report) { + $response = get(reportUrl($report, $this->company->unique_hash, '&download=true')); + + $response->assertOk(); + expect($response->headers->get('content-disposition'))->toContain('attachment'); + expect($response->getContent())->toStartWith('%PDF-'); +})->with('reports'); diff --git a/tests/Unit/GotenbergPdfDriverTest.php b/tests/Unit/GotenbergPdfDriverTest.php new file mode 100644 index 00000000..2278d600 --- /dev/null +++ b/tests/Unit/GotenbergPdfDriverTest.php @@ -0,0 +1,66 @@ + 'http://gotenberg.example.com:3000', + 'pdf.connections.gotenberg.papersize' => '210mm 297mm', + ]); +}); + +// A real view that renders without any shared document data, so these stay +// unit tests rather than needing a seeded invoice. +function gotenbergRequestBody(string $template = 'app.pdf.partials.fonts'): string +{ + return (string) (new GotenbergPdfDriver)->buildRequest($template)->getBody(); +} + +/** + * printBackground governs the root (body/html) background only — Chromium paints + * element backgrounds regardless, checked against gotenberg:8. dompdf paints the + * body background, so setting this keeps a custom template that styles `body` + * looking the same on either driver. No stock template sets one. + */ +test('the chromium request asks for the page background to be printed', function () { + expect(gotenbergRequestBody())->toContain('printBackground'); +}); + +/** + * config/dompdf.php renders as `screen`; Chromium's own default is `print`. The + * two drivers should not disagree about which media type a template is styled for. + */ +test('the chromium request emulates the same media type dompdf uses', function () { + expect(gotenbergRequestBody())->toContain('emulatedMediaType'); +}); + +test('the configured paper size reaches the request', function () { + config(['pdf.connections.gotenberg.papersize' => '8.5in 11in']); + + expect(gotenbergRequestBody()) + ->toContain('paperWidth') + ->toContain('8.5in') + ->toContain('11in'); +}); + +test('the rendered document is sent as the index file', function () { + expect(gotenbergRequestBody())->toContain('index.html'); +}); + +test('it throws when the papersize config has an unexpected format', function () { + config(['pdf.connections.gotenberg.papersize' => 'invalid']); + + expect(fn () => gotenbergRequestBody()) + ->toThrow(InvalidArgumentException::class, 'Invalid Gotenberg Papersize specified'); +}); + +test('it throws when the configured host targets a private network address', function () { + config(['pdf.connections.gotenberg.host' => 'http://10.0.0.1:3000']); + + expect(fn () => gotenbergRequestBody()) + ->toThrow(InvalidArgumentException::class, 'Invalid Gotenberg host'); +}); diff --git a/tests/Unit/PdfDriverContractTest.php b/tests/Unit/PdfDriverContractTest.php new file mode 100644 index 00000000..7aef54c9 --- /dev/null +++ b/tests/Unit/PdfDriverContractTest.php @@ -0,0 +1,59 @@ +download() on a Gotenberg response that had no such method: a fatal error on + * every report download, invisible because no type ever asserted the two were + * interchangeable. These tests are that assertion. + */ +test('the factory returns a PdfDriver for every supported driver', function (string $driver) { + expect(PdfDriverFactory::create($driver))->toBeInstanceOf(PdfDriver::class); +})->with(['dompdf', 'gotenberg']); + +test('the factory rejects an unknown driver', function () { + expect(fn () => PdfDriverFactory::create('wkhtmltopdf')) + ->toThrow(InvalidArgumentException::class, 'Invalid PdfDriver requested'); +}); + +test('every driver implements the driver contract', function (string $class) { + expect(is_subclass_of($class, PdfDriver::class))->toBeTrue(); +})->with([DompdfDriver::class, GotenbergPdfDriver::class]); + +/** + * The report controllers call stream(), download() and output() with no + * arguments, so each has to be callable bare on either driver's response. + */ +test('every response implements the full response contract', function (string $class) { + expect(is_subclass_of($class, ResponseStream::class))->toBeTrue(); + + foreach (['stream', 'download', 'output'] as $method) { + expect(method_exists($class, $method))->toBeTrue(); + + $required = (new ReflectionMethod($class, $method))->getNumberOfRequiredParameters(); + expect($required)->toBe(0, "{$class}::{$method}() must be callable without arguments"); + } +})->with([DompdfResponse::class, GotenbergPdfResponse::class]); + +test('dompdf renders a real pdf through the contract', function () { + $pdf = (new DompdfDriver)->loadView('app.pdf.partials.fonts'); + + expect($pdf)->toBeInstanceOf(ResponseStream::class) + ->and($pdf->output())->toStartWith('%PDF-'); +}); + +test('dompdf offers the document as an attachment when downloaded', function () { + $response = (new DompdfDriver)->loadView('app.pdf.partials.fonts')->download('expenses.pdf'); + + expect($response->headers->get('content-disposition'))->toContain('attachment') + ->and($response->headers->get('content-disposition'))->toContain('expenses.pdf'); +});