Skip to content

[HttpFoundation] automatically generate safe fallback filename #16656

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 4, 2016
Merged

[HttpFoundation] automatically generate safe fallback filename #16656

merged 1 commit into from
Mar 4, 2016

Conversation

xabbuh
Copy link
Member

@xabbuh 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

@xabbuh xabbuh mentioned this pull request Nov 24, 2015
Closed
@Yannik
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
Copy link
Member

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
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?

@ghost
Copy link

ghost commented Jan 31, 2016

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

@fabpot
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
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.

nicolas-grekas
nicolas-grekas reviewed Mar 2, 2016
View reviewed changes
src/Symfony/Component/HttpFoundation/BinaryFileResponse.php
@@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@fabpot
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
Copy link
Member Author

xabbuh commented Mar 4, 2016

@fabpot @nicolas-grekas I pushed an update.

@fabpot
Copy link
Member

fabpot commented Mar 4, 2016

👍

1 similar comment
@nicolas-grekas
Copy link
Member

👍

@fabpot
Copy link
Member

fabpot commented Mar 4, 2016

Thank you @xabbuh.

@fabpot fabpot merged commit 03721e3 into symfony:2.3 Mar 4, 2016
fabpot added a commit that referenced this pull request Mar 4, 2016
@fabpot
bug #16656 [HttpFoundation] automatically generate safe fallback file…
5bbe951 
…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 issue-16603 branch March 4, 2016 09:44
@Yannik
Copy link

Yannik commented Mar 4, 2016

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

@xabbuh
Copy link
Member Author

xabbuh commented Mar 4, 2016

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

@jakzal jakzal mentioned this pull request Mar 6, 2016
Closed
@fabpot fabpot mentioned this pull request Mar 13, 2016
Merged
This was referenced Mar 25, 2016
Merged
Closed
This was referenced Mar 27, 2016
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Bug HttpFoundation Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@xabbuh @Yannik @nicolas-grekas @fabpot @javiereguiluz @carsonbot