barryvdh / laravel-dompdf

A DOMPDF Wrapper for Laravel
MIT License
6.57k stars 965 forks source link

Encode filename when downloading #1026

Closed rico closed 1 month ago

rico commented 6 months ago

Encode the filename according to RFC 3986 when downloading the file.

We had problems with umlauts in the filename, when we deployed a project with Vapor. Locally it worked fine, but when deployed, all umlauts got replaced with underscores.

We tried a few things that did not work, so we went on and asked Chat GPT which suggested using rawurlencode. This fixed the Problem.

rawurlencode can also be used on environments that did not show this problem.

parallels999 commented 6 months ago

any change to download must be done for stream too https://github.com/barryvdh/laravel-dompdf/blob/81227b1409164128c93fe42f4b748878afeda399/src/PDF.php#L221-L226

rico commented 6 months ago

@parallels999 thanks & sure, can expand this pull request if @barryvdh is considering to merge this.

barryvdh commented 6 months ago

I'll consider it. But was just looking, I think there was something like this in Laravel. But perhaps we just used Str::ascii there.

barryvdh commented 6 months ago

Ah yeah Symfony does something like this: https://github.com/symfony/symfony/blob/973495d1670dc40fb8dec47082aad9bdb50f4d73/src/Symfony/Component/HttpFoundation/BinaryFileResponse.php#L151-L174 And this: https://github.com/symfony/symfony/blob/973495d1670dc40fb8dec47082aad9bdb50f4d73/src/Symfony/Component/HttpFoundation/HeaderUtils.php#L165-L196

Maybe we could use the HeaderUtils better? And just use the Ascii version for the fallback? Because unicode (even with rawurlencode) should be only used for filename* right? https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Disposition#filename_2

barryvdh commented 6 months ago

So something like

'Content-Disposition' =>  HeaderUtils::makeDisposition('inline', $filename, Str::ascii($filename))
rico commented 6 months ago

@barryvdh seems like that would make sense :-)

parallels999 commented 6 months ago

@barryvdh at another point, in certain locale

BinaryFileResponse/StreamedResponse seems to be safer symfony/symfony/Symfony/Component/HttpFoundation/BinaryFileResponse.php symfony/symfony/Symfony/Component/HttpFoundation/StreamedResponse.php Laravel uses that laravel/framework/Illuminate/Routing/ResponseFactory.php#L173-L175 laravel/framework/Illuminate/Routing/ResponseFactory.php#L141-L151 But on that case return type must be \Symfony\Component\HttpFoundation\Response

barryvdh commented 6 months ago

ah that fallback is better indeed.

    protected function fallbackName($name)
    {
        return str_replace('%', '', Str::ascii($name));
    }

But binary is not that easy I think, because it's not an actual file (which the binary response expects)

barryvdh commented 6 months ago

And streamed could work, but I think I had that in the past but was causing issues. Because then you would postpone the rendering of HTML and any errors would be swallowed in the stream.

parallels999 commented 6 months ago

~Ok forget stream, but BinaryFileResponse as inline(like a fake stream)?~

maybe it would be necessary many tests, if it would be a risk, forget it

rico commented 6 months ago

What about: 'attachment; filename="' . $filename . '";filename*="UTF-8""' . rawurlencode($filename) . '"'? UTF-8 encoded with fallback ...

barryvdh commented 1 month ago

See https://github.com/barryvdh/laravel-dompdf/pull/1050 on the 3.x branch