Add encrypt_mimes config option #88
Conversation
…om/Laravel-Backpack/FileManager into fix-mime-type-tampering-protection
There was a problem hiding this comment.
Thanks @pxpm for the quick fix! I think the disable flag is nice, especially for those willing to override or call the route from some other view of their own. I've tested this locally and it seems to work great for me.
Just for reference, that is also my use case: I have a custom HTML editor field, where I manually call the elFinder popup for integration, then pass along any elFinder option in JS from there. With this config flag I no longer need to 'hack' and add Crypt::encrypt('') as a parameter just to consume the view.
Edit: I've left some small suggestion for an IMO cleaner way to generate the URL that I use.
| $field['attributes']['data-elfinder-trigger-url'] = $field['attributes']['data-elfinder-trigger-url'] ?? url(config('elfinder.route.prefix').'/popup/'.$field['name']); | ||
| $field['attributes']['data-elfinder-trigger-url'] .= '?mimes='.urlencode(Crypt::encrypt($field['mime_types'] ?? '')); | ||
| $mimeTypes = $field['mime_types'] ?? ''; | ||
| $field['attributes']['data-elfinder-trigger-url'] .= '?mimes='.urlencode( | ||
| config('elfinder.encrypt_mimes', true) ? Crypt::encrypt($mimeTypes) : json_encode($mimeTypes, JSON_UNESCAPED_SLASHES) | ||
| ); |
There was a problem hiding this comment.
I personally use the route helper here with array parameters, which I both prefer for readability and because it neatly handles URL parsing for both 'required' and 'optional' parameters.
$mimeTypes = $field['mime_types'] ?? '';
$field['attributes']['data-elfinder-trigger-url'] ??= route(
'elfinder.popup',
[
'input_id' => $field['name'],
'mimes' => config('elfinder.encrypt_mimes', true) ? Crypt::encrypt($mimeTypes) : json_encode($mimeTypes, JSON_UNESCAPED_SLASHES),
],
);There was a problem hiding this comment.
I agree it's cleaner, but at this point we can't be sure if people are using elfinder.popup as their url, if they changed it or not.
since there is nothing "wrong" per se as it is now, I think I would rather keep it for now 👍
There was a problem hiding this comment.
To be fair I would be slightly more worried about the actual url changing than the alias (alias comes from https://github.com/barryvdh/laravel-elfinder/blob/30700116a94828202b777da33a6d5c1ee7d004a6/src/ElfinderServiceProvider.php#L59). But both work fine, so perfectly fine to skip this.
Thanks for merging 🥂
| $field['wrapper']['data-elfinder-trigger-url'] = $field['wrapper']['data-elfinder-trigger-url'] ?? url(config('elfinder.route.prefix').'/popup/'.$field['name'].'?multiple=1'); | ||
|
|
||
| $field['wrapper']['data-elfinder-trigger-url'] .= '&mimes='.urlencode(Crypt::encrypt($field['mime_types'] ?? '')); | ||
| $mimeTypes = $field['mime_types'] ?? ''; |
There was a problem hiding this comment.
Same as above, but with multiple => 1 in the array
This allow developers to disable the encrypt/decrypt of mime types.
IMO, it shouldn't hurt having this little obfuscation, but for some developers they might prefer to have the clean mime types and that's ok too.