Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HttpFoundation] automatically generate safe fallback filename #16656

Merged
merged 1 commit into from Mar 4, 2016

Conversation

@xabbuh
Copy link
Member

xabbuh commented Nov 24, 2015

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #16603
License MIT
Doc PR
@Yannik

This comment has been minimized.

Copy link

Yannik commented Nov 24, 2015

Hi Christian,
thanks a lot for addressing this!
Maybe it's possible to convert the default filename to the nearest ascii equivalent instead of just replacing it with an underscore.
For example: 'föö.html' => 'foo.html' instead of 'f__.html',
'fòôbàř.txt' => 'foobar.txt' instead of 'f_____.txt'.
I am not confident in what's the best way to do this - I use https://github.com/danielstjules/Stringy#toascii for such purposes in my own applications.

Best regards, Yannik

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Nov 25, 2015

Why couldn't we use RFC 2231?
See also this old "paper" on the topic: http://greenbytes.de/tech/tc2231/
Doing transliterations works for western languages but is quite limited in the general case, I'm not really fan of this approach...

@xabbuh

This comment has been minimized.

Copy link
Member Author

xabbuh commented Nov 25, 2015

@nicolas-grekas Encoding a string according to RFC 2231 leads to % characters in the resulting string, doesn't it? This would conflict with how the makeDisposition() method of the ResponseHeaderBag is working (see https://github.com/symfony/symfony/blob/2.8/src/Symfony/Component/HttpFoundation/ResponseHeaderBag.php#L256) (but maybe we need to fix it there). What do you think?

@JHGitty

This comment has been minimized.

Copy link

JHGitty commented Jan 31, 2016

@xabbuh Your PR is awesome! Hopefully this will be merged into Symfony.

@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Feb 29, 2016

@nicolas-grekas @xabbuh Can we find something that works for everyone here? I'd like to merge this one ASAP if possible.

@xabbuh

This comment has been minimized.

Copy link
Member Author

xabbuh commented Mar 1, 2016

If we removed the limitation of not being able to have the percent character in the fallback filename, we could use RFC 2231 afaics as @nicolas-grekas proposed. But I am not sure why this check was added initially.

@@ -157,6 +157,20 @@ public function setContentDisposition($disposition, $filename = '', $filenameFal
$filename = $this->file->getFilename();
}
if ('' === $filenameFallback && (!preg_match('/^[\x20-\x7e]*$/', $filename) || false !== strpos($filename, '%'))) {
$encoding = mb_detect_encoding($filename);

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Mar 2, 2016

Member

mb_detect_encoding($string, null, true) to enable strict mode (non-strict is useless)

for ($i = 0; $i < mb_strlen($filename, $encoding); $i++) {
$char = mb_substr($filename, $i, 1, $encoding);
if (ord($char) < 32 || ord($char) > 126) {

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Mar 2, 2016

Member

missing handling of %

@@ -157,6 +157,20 @@ public function setContentDisposition($disposition, $filename = '', $filenameFal
$filename = $this->file->getFilename();
}
if ('' === $filenameFallback && (!preg_match('/^[\x20-\x7e]*$/', $filename) || false !== strpos($filename, '%'))) {

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Mar 2, 2016

Member

the if block should be moved in ResponseHeaderBag::makeDisposition IMO, isn't it?

This comment has been minimized.

Copy link
@xabbuh

xabbuh Mar 4, 2016

Author Member

The thing is that people will expect the ResponseHeaderBag::makeDisposition() method to throw an exception when invalid characters are passed. Silently convertin the fallback filename is a BC break imo.

This comment has been minimized.

Copy link
@fabpot

fabpot Mar 4, 2016

Member

That's a good point.

@javiereguiluz javiereguiluz added the Bug label Mar 3, 2016
@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Mar 4, 2016

@xabbuh Do you have time to finish this one? If not, just let me know and I will finish it for you.

@xabbuh xabbuh force-pushed the xabbuh:issue-16603 branch from 80494dd to 67ca870 Mar 4, 2016
@xabbuh xabbuh force-pushed the xabbuh:issue-16603 branch from 67ca870 to 03721e3 Mar 4, 2016
@xabbuh

This comment has been minimized.

Copy link
Member Author

xabbuh commented Mar 4, 2016

@fabpot @nicolas-grekas I pushed an update.

@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Mar 4, 2016

👍

1 similar comment
@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Mar 4, 2016

👍

@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Mar 4, 2016

Thank you @xabbuh.

@fabpot fabpot merged commit 03721e3 into symfony:2.3 Mar 4, 2016
1 of 3 checks passed
1 of 3 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details
fabpot added a commit that referenced this pull request Mar 4, 2016
…name (xabbuh)

This PR was merged into the 2.3 branch.

Discussion
----------

[HttpFoundation] automatically generate safe fallback filename

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #16603
| License       | MIT
| Doc PR        |

Commits
-------

03721e3 automatically generate safe fallback filename
@xabbuh xabbuh deleted the xabbuh:issue-16603 branch Mar 4, 2016
@Yannik

This comment has been minimized.

Copy link

Yannik commented Mar 4, 2016

@fabpot Will this be merged into the more current releases of symfony?

@xabbuh

This comment has been minimized.

Copy link
Member Author

xabbuh commented Mar 4, 2016

@Yannik Yes, we regularly merge lower branches into all higher maintained branches.

This was referenced Mar 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.