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

Improved uploaded file sanitizer #7217

Merged
merged 1 commit into from Dec 8, 2017

Conversation

Projects
None yet
4 participants
@Boorj
Contributor

Boorj commented Nov 28, 2017

translates non-ascii European symbols to ascii-latin + transliteration
Related to #7198

@GawainLynch

Why reimplement Slugify here? 😉

@Boorj

This comment has been minimized.

Contributor

Boorj commented Dec 5, 2017

@GawainLynch , 😆 really.
will $app['slugify']->slugify($filename) be a solution?

@GawainLynch

This comment has been minimized.

@Boorj

This comment has been minimized.

Contributor

Boorj commented Dec 5, 2017

yes, this is the file that i researched, before posting previous post 😉
So i'll assume that you meant yes for my question).
I'll do that.

@Boorj

This comment has been minimized.

Contributor

Boorj commented Dec 5, 2017

@GawainLynch

This comment has been minimized.

Contributor

GawainLynch commented Dec 5, 2017

Better. On quick first look though, don't use the $app['config'] object outside of the closure(s)

@Boorj

This comment has been minimized.

Contributor

Boorj commented Dec 6, 2017

@GawainLynch i updated gist, is it ok now?

@@ -18,6 +19,14 @@ class UploadServiceProvider implements ServiceProviderInterface
{
public function register(Application $app)
{
$app['upload.sanitizer'] = $app->share(function ($app)
{

This comment has been minimized.

@Nitpick-CI

Nitpick-CI Dec 8, 2017

Opening brace should be on the same line as the declaration

@GawainLynch

This comment has been minimized.

Contributor

GawainLynch commented Dec 8, 2017

@Boorj 'cause we're mates and both really busy, I implemented the correct parts of your gist and squashed the commits. Now $app['upload.sanitizer'] is an overridable service using Slugify and everyone's needs should be able to be met 👍

@GawainLynch GawainLynch changed the base branch from 3.4 to 3.5 Dec 8, 2017

@GawainLynch

This comment has been minimized.

Contributor

GawainLynch commented Dec 8, 2017

Also as this adds features, not directly fixes bugs, I've moved the branch to 3.5 … which is going to be released a.s.a.p.

@GawainLynch GawainLynch added this to the Bolt 3.5 - Feature release milestone Dec 8, 2017

Add & use Slugify uploaded file sanitizer service
Converts non-ASCII European symbols to ASCII-latin via transliteration

@GawainLynch GawainLynch merged commit 188ace8 into bolt:3.5 Dec 8, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Boorj Boorj deleted the Boorj:patch-5 branch Dec 8, 2017

$pattern = $app['config']->get('general/upload/pattern', '[^A-Za-z0-9\.]+');
$lowercase = $app['config']->get('general/upload/lowercase', true);
return new Slugify("/$pattern/", ['lowercase' => $lowercase]);

This comment has been minimized.

@Boorj

Boorj Dec 8, 2017

Contributor

@GawainLynch i wasn't sure, how much memory does Slugify take and is it only 1 instance per PHP response called. I mean, if multiple files and one php session, and heavy Slugify. So that's why i created distinct variables. This solution was my first one. And thanks for using that.

This comment has been minimized.

@GawainLynch

GawainLynch Dec 8, 2017

Contributor

Yeah, Slugify is tiny … there are much bigger maps in use. But also this is the DI service model, they only get instantiated once per request as you mention 😄

@ntomka

This comment has been minimized.

Contributor

ntomka commented on src/Provider/UploadServiceProvider.php in 6a23742 Dec 11, 2017

This made it's way to master, but Silex 2.x is used there, where Pimple 3.x doesn't have share method anymore, because every services is shared since Pimple 2.x: https://github.com/silexphp/Pimple/blob/master/CHANGELOG#L54

This comment has been minimized.

Contributor

GawainLynch replied Dec 11, 2017

Ugh … bad merge job on my part 😞

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment