fix(pdf): give both drivers one contract, and fix what that was hiding (#727)

PdfDriver and ResponseStream existed but nothing implemented them. The factory
returned the vendor dompdf wrapper for one driver and a bespoke class for the
other, so the two were never held to the same shape. Three things had slipped
through that gap.

Report PDFs answered 403 for everyone. The five report 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
check could not pass. They now scope to the company named in the URL. The policy
still checks membership, so this grants nothing new. Also firstOrFail() on the
hash lookup, so an unknown company is a 404 rather than a 500 on a null.

Report downloads were fatal on Gotenberg. GotenbergPdfResponse had no download(),
and the report controllers are its only callers. Added, alongside stream() and
output(), with the whole set now on the interface.

Streamed documents carried an HTTP preamble. GeneratesPdfTrait wrapped
$pdf->stream() -- already a Response -- in another response()->make(), which
stringified it and prepended "HTTP/1.0 200 OK" plus headers to the file. Readers
scan the first kilobyte for %PDF so nobody noticed, but the bytes were malformed.
Passing ->output() fixes it, and the render test now asserts the position.

Two driver-parity settings, both checked against a real gotenberg:8 rather than
inferred: emulateScreenMediaType(), because Chromium defaults to print media
while config/dompdf.php renders as screen, so a @media print rule applied on one
driver and not the other; and printBackground(), which turns out to affect only
the root background, since Chromium paints element backgrounds either way. No
stock template sets a body background, so that one changes nothing today and is
here to keep custom templates consistent across drivers.

Claude-Session: https://claude.ai/code/session_01QmECndmNZwzN65Zz9P87dF

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
 {

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);
 

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);
 

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);
 

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);
 

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);
 

app/Support/Pdf/DompdfDriver.php

@@ -0,0 +1,27 @@
+<?php
+
+namespace App\Support\Pdf;
+
+use Barryvdh\DomPDF\PDF;
+use Illuminate\Support\Facades\App;
+
+/**
+ * The dompdf half of {@see PdfDriver}.
+ *
+ * Previously the factory returned `dompdf.wrapper` straight from the container,
+ * which left no place to apply anything InvoiceShelf decides — paper size,
+ * orientation and margins are all still whatever `config/dompdf.php` baked in at
+ * construction. This class is that place.
+ */
+class DompdfDriver implements PdfDriver
+{
+    public function loadView(string $template): ResponseStream
+    {
+        return new DompdfResponse($this->wrapper()->loadView($template));
+    }
+
+    protected function wrapper(): PDF
+    {
+        return App::make('dompdf.wrapper');
+    }
+}

app/Support/Pdf/DompdfResponse.php

@@ -0,0 +1,35 @@
+<?php
+
+namespace App\Support\Pdf;
+
+use Barryvdh\DomPDF\PDF;
+use Illuminate\Http\Response;
+
+/**
+ * Adapts the dompdf wrapper to {@see ResponseStream}.
+ *
+ * The wrapper already offers stream/download/output with matching semantics,
+ * so this only exists to hold it to the same contract Gotenberg is held to.
+ * Without it the factory hands back a vendor object that happens to look right,
+ * which is how `download()` came to be missing on the Gotenberg side without
+ * anything noticing.
+ */
+class DompdfResponse implements ResponseStream
+{
+    public function __construct(protected PDF $pdf) {}
+
+    public function stream(string $filename = 'document.pdf'): Response
+    {
+        return $this->pdf->stream($filename);
+    }
+
+    public function download(string $filename = 'document.pdf'): Response
+    {
+        return $this->pdf->download($filename);
+    }
+
+    public function output(): string
+    {
+        return $this->pdf->output();
+    }
+}

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);
     }
 }

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.'"',
+        ]);
     }
 }

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')
         };

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');
 

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;
 }

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"',
         ]);

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'));

tests/Feature/Pdf/ReportPdfDownloadTest.php

@@ -0,0 +1,63 @@
+<?php
+
+use App\Models\User;
+use Illuminate\Support\Facades\Artisan;
+use Laravel\Sanctum\Sanctum;
+
+use function Pest\Laravel\get;
+
+/**
+ * The five report controllers are the only callers of ->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');

tests/Unit/GotenbergPdfDriverTest.php

@@ -0,0 +1,66 @@
+<?php
+
+use App\Support\Pdf\GotenbergPdfDriver;
+
+/**
+ * These assert against buildRequest(), which assembles the Chromium multipart
+ * body without sending it. Nothing here needs a running Gotenberg.
+ */
+beforeEach(function () {
+    config([
+        'pdf.connections.gotenberg.host' => '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');
+});

tests/Unit/PdfDriverContractTest.php

@@ -0,0 +1,59 @@
+<?php
+
+use App\Support\Pdf\DompdfDriver;
+use App\Support\Pdf\DompdfResponse;
+use App\Support\Pdf\GotenbergPdfDriver;
+use App\Support\Pdf\GotenbergPdfResponse;
+use App\Support\Pdf\PdfDriver;
+use App\Support\Pdf\PdfDriverFactory;
+use App\Support\Pdf\ResponseStream;
+
+/**
+ * PdfDriver and ResponseStream existed but nothing implemented them, so the
+ * factory could hand back the raw dompdf wrapper for one driver and a bespoke
+ * class for the other. That is how the report controllers came to call
+ * ->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');
+});