[HttpFoundation] Generate safe fallback filename for wrongly encoded filename #23658
Conversation
| @@ -164,6 +166,10 @@ public function setContentDisposition($disposition, $filename = '', $filenameFal | |||
| if ('' === $filenameFallback && (!preg_match('/^[\x20-\x7e]*$/', $filename) || false !== strpos($filename, '%'))) { | |||
| $encoding = mb_detect_encoding($filename, null, true); | |||
There was a problem hiding this comment.
since this is a cleaning code, what about doing something like that instead?
$encoding = mb_detect_encoding($filename, null, true) ?: '8bit';
There was a problem hiding this comment.
This would indeed help with generating the filenameFallback no matter how weird the $filename is. But considering ResponseHeaderbag::makeDisposition the $filename parameter is described and expected to be A unicode string and placed in the header field parameter filename* defined as utf-8 encoded without further processing. If passing e.g. an ISO-8859-1 encoded $filename with german umlauts and not throwing this exception would lead to a bad or invalid Content-Disposition response header. Maybe setContentDisposition should actually ensure the $filename to be utf-8 encoded in any case. But this could be a BC break, although I think everything but utf-8 seems to be a misusage. What do you think?
There was a problem hiding this comment.
I think we have no need to be stricter now than we were already. If people put non-utf8 string, that's going to be sent and displayed in some way by browsers, so there is already a "reporting" effect.
throwing an exception would just break code that work "fine" now. So, still on the "8bit" side :)
| @@ -164,6 +166,10 @@ public function setContentDisposition($disposition, $filename = '', $filenameFal | |||
| if ('' === $filenameFallback && (!preg_match('/^[\x20-\x7e]*$/', $filename) || false !== strpos($filename, '%'))) { | |||
| $encoding = mb_detect_encoding($filename, null, true); | |||
|
|
|||
| if (false === $encoding) { | |||
| throw new \InvalidArgumentException('The filename encoding cannot be detected.'); | |||
There was a problem hiding this comment.
Should we use this error message instead?
throw new \InvalidArgumentException(sprintf('The "%s" filename encoding cannot be detected.', $filename));There was a problem hiding this comment.
When $filename is not a "clean" utf-8 string, but have a broken encoding, this could break the output wherever the exception message will be used. Actually we had an exception from the mongodb related code not showing up in the first place, cause its message included the broken non utf-8 encoded filename which in turn caused an Exception in the ExceptionListener, while trying to generate a JSON response with an invalid utf-8 string. So I suggest not to include the filename here.
9762238 to
8fd5569
Compare
|
The PR was updated as suggested by @nicolas-grekas. |
|
Thank you @xelaris. |
…ly encoded filename (xelaris) This PR was merged into the 2.7 branch. Discussion ---------- [HttpFoundation] Generate safe fallback filename for wrongly encoded filename | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | This handles the case where the encoding of a random string cannot be detected. Until now this causes a PHP Warning `mb_strlen(): Unknown encoding ""`. Commits ------- 8fd5569 [HttpFoundation] Generate safe fallback filename for wrongly encoded filename
This handles the case where the encoding of a random string cannot be detected. Until now this causes a PHP Warning
mb_strlen(): Unknown encoding "".