-
-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Better handle inline files in file listing #15689
Changes from all commits
c56affd
96191a5
ccd2019
c49abb6
c49921f
017884f
c01190f
02c80ff
d67addc
4933aa5
0e9b3c9
d114973
6105323
06c599c
ef9b6e3
787e651
a05c33f
db81701
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1123,6 +1123,7 @@ public static function filetype_icon($filename) | |
'png' => 'far fa-image', | ||
'webp' => 'far fa-image', | ||
'avif' => 'far fa-image', | ||
'svg' => 'fas fa-vector-square', | ||
// word | ||
'doc' => 'far fa-file-word', | ||
'docx' => 'far fa-file-word', | ||
|
@@ -1135,7 +1136,7 @@ public static function filetype_icon($filename) | |
//Text | ||
'txt' => 'far fa-file-alt', | ||
'rtf' => 'far fa-file-alt', | ||
'xml' => 'far fa-file-alt', | ||
'xml' => 'fas fa-code', | ||
// Misc | ||
'pdf' => 'far fa-file-pdf', | ||
'lic' => 'far fa-save', | ||
|
@@ -1148,41 +1149,7 @@ public static function filetype_icon($filename) | |
return 'far fa-file'; | ||
} | ||
|
||
public static function show_file_inline($filename) | ||
{ | ||
$extension = substr(strrchr($filename, '.'), 1); | ||
|
||
if ($extension) { | ||
switch ($extension) { | ||
case 'jpg': | ||
case 'jpeg': | ||
case 'gif': | ||
case 'png': | ||
case 'webp': | ||
case 'avif': | ||
return true; | ||
break; | ||
default: | ||
return false; | ||
} | ||
} | ||
|
||
return false; | ||
} | ||
|
||
/** | ||
* Generate a random encrypted password. | ||
* | ||
* @author Wes Hulette <[email protected]> | ||
* | ||
* @since 5.0.0 | ||
* | ||
* @return string | ||
*/ | ||
public static function generateEncyrptedPassword(): string | ||
{ | ||
return bcrypt(self::generateUnencryptedPassword()); | ||
} | ||
|
||
/** | ||
* Get a random unencrypted password. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,7 @@ | |
use Illuminate\Http\RedirectResponse; | ||
use Symfony\Component\HttpFoundation\BinaryFileResponse; | ||
use Symfony\Component\HttpFoundation\StreamedResponse; | ||
use Illuminate\Contracts\Filesystem\FileNotFoundException; | ||
class StorageHelper | ||
{ | ||
public static function downloader($filename, $disk = 'default') : BinaryFileResponse | RedirectResponse | StreamedResponse | ||
|
@@ -25,4 +26,64 @@ public static function downloader($filename, $disk = 'default') : BinaryFileResp | |
return Storage::disk($disk)->download($filename); | ||
} | ||
} | ||
|
||
|
||
/** | ||
* This determines the file types that should be allowed inline and checks their fileinfo extension | ||
* to determine that they are safe to display inline. | ||
* | ||
* @author <A. Gianotto> [<[email protected]]> | ||
* @since v7.0.14 | ||
* @param $file_with_path | ||
* @return bool | ||
*/ | ||
public static function allowSafeInline($file_with_path) { | ||
|
||
$allowed_inline = [ | ||
'pdf', | ||
'svg', | ||
'jpg', | ||
'gif', | ||
'svg', | ||
'avif', | ||
'webp', | ||
'png', | ||
]; | ||
|
||
|
||
// The file exists and is allowed to be displayed inline | ||
if (Storage::exists($file_with_path) && (in_array(pathinfo($file_with_path, PATHINFO_EXTENSION), $allowed_inline))) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think there's a way to do this with a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Storage:: should be doing that already - that's the point of the facade |
||
return true; | ||
} | ||
return false; | ||
|
||
} | ||
|
||
/** | ||
* Decide whether to show the file inline or download it. | ||
*/ | ||
public static function showOrDownloadFile($file, $filename) { | ||
|
||
$headers = []; | ||
|
||
if (request('inline') == 'true') { | ||
|
||
$headers = [ | ||
'Content-Disposition' => 'inline', | ||
]; | ||
|
||
// This is NOT allowed as inline - force it to be displayed as text in the browser | ||
if (self::allowSafeInline($file) != true) { | ||
$headers = array_merge($headers, ['Content-Type' => 'text/plain']); | ||
} | ||
} | ||
|
||
// Everything else seems okay, but the file doesn't exist on the server. | ||
if (Storage::missing($file)) { | ||
throw new FileNotFoundException(); | ||
} | ||
|
||
return Storage::download($file, $filename, $headers); | ||
|
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -106,50 +106,29 @@ public function destroy($accessoryId = null, $fileId = null) : RedirectResponse | |
* @param int $accessoryId | ||
* @param int $fileId | ||
*/ | ||
public function show($accessoryId = null, $fileId = null, $download = true) : View | RedirectResponse | Response | BinaryFileResponse | StreamedResponse | ||
public function show($accessoryId = null, $fileId = null) : View | RedirectResponse | Response | BinaryFileResponse | StreamedResponse | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Route-model binding is an option here, if you wanted it. Might help with the indentation level. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed, but out of scope for this PR |
||
{ | ||
|
||
Log::debug('Private filesystem is: '.config('filesystems.default')); | ||
$accessory = Accessory::find($accessoryId); | ||
|
||
|
||
|
||
// the accessory is valid | ||
if (isset($accessory->id)) { | ||
if ($accessory = Accessory::find($accessoryId)) { | ||
$this->authorize('view', $accessory); | ||
$this->authorize('accessories.files', $accessory); | ||
|
||
if (! $log = Actionlog::whereNotNull('filename')->where('item_id', $accessory->id)->find($fileId)) { | ||
return redirect()->route('accessories.index')->with('error', trans('admin/users/message.log_record_not_found')); | ||
} | ||
|
||
$file = 'private_uploads/accessories/'.$log->filename; | ||
|
||
if (Storage::missing($file)) { | ||
Log::debug('FILE DOES NOT EXISTS for '.$file); | ||
Log::debug('URL should be '.Storage::url($file)); | ||
if ($log = Actionlog::whereNotNull('filename')->where('item_id', $accessory->id)->find($fileId)) { | ||
$file = 'private_uploads/accessories/'.$log->filename; | ||
|
||
return response('File '.$file.' ('.Storage::url($file).') not found on server', 404) | ||
->header('Content-Type', 'text/plain'); | ||
} else { | ||
|
||
// Display the file inline | ||
if (request('inline') == 'true') { | ||
$headers = [ | ||
'Content-Disposition' => 'inline', | ||
]; | ||
return Storage::download($file, $log->filename, $headers); | ||
try { | ||
return StorageHelper::showOrDownloadFile($file, $log->filename); | ||
} catch (\Exception $e) { | ||
return redirect()->route('accessories.show', ['accessory' => $accessory])->with('error', trans('general.file_not_found')); | ||
} | ||
} | ||
|
||
return redirect()->route('accessories.show', ['accessory' => $accessory])->with('error', trans('general.log_record_not_found')); | ||
|
||
// We have to override the URL stuff here, since local defaults in Laravel's Flysystem | ||
// won't work, as they're not accessible via the web | ||
if (config('filesystems.default') == 'local') { // TODO - is there any way to fix this at the StorageHelper layer? | ||
return StorageHelper::downloader($file); | ||
} | ||
} | ||
} | ||
|
||
return redirect()->route('accessories.index')->with('error', trans('general.file_does_not_exist', ['id' => $fileId])); | ||
return redirect()->route('accessories.index')->with('error', trans('general.file_not_found')); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -61,43 +61,30 @@ public function store(UploadFileRequest $request, $assetId = null) : RedirectRes | |
*/ | ||
public function show($assetId = null, $fileId = null) : View | RedirectResponse | Response | StreamedResponse | BinaryFileResponse | ||
{ | ||
$asset = Asset::find($assetId); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again, you could do R-M-B here - but I could also see not wanting to muck around with the 'shape' of the code too much, so probably a bit of a 'nit' if anything. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again, that's a different PR ;) |
||
// the asset is valid | ||
if (isset($asset->id)) { | ||
$this->authorize('view', $asset); | ||
|
||
if (! $log = Actionlog::whereNotNull('filename')->where('item_id', $asset->id)->find($fileId)) { | ||
return response('No matching record for that asset/file', 500) | ||
->header('Content-Type', 'text/plain'); | ||
} | ||
|
||
$file = 'private_uploads/assets/'.$log->filename; | ||
if ($asset = Asset::find($assetId)) { | ||
|
||
if ($log->action_type == 'audit') { | ||
$file = 'private_uploads/audits/'.$log->filename; | ||
} | ||
$this->authorize('view', $asset); | ||
|
||
if (! Storage::exists($file)) { | ||
return response('File '.$file.' not found on server', 404) | ||
->header('Content-Type', 'text/plain'); | ||
} | ||
if ($log = Actionlog::whereNotNull('filename')->where('item_id', $asset->id)->find($fileId)) { | ||
$file = 'private_uploads/assets/'.$log->filename; | ||
|
||
if (request('inline') == 'true') { | ||
if ($log->action_type == 'audit') { | ||
$file = 'private_uploads/audits/'.$log->filename; | ||
} | ||
|
||
$headers = [ | ||
'Content-Disposition' => 'inline', | ||
]; | ||
try { | ||
return StorageHelper::showOrDownloadFile($file, $log->filename); | ||
} catch (\Exception $e) { | ||
return redirect()->route('hardware.show', ['hardware' => $asset])->with('error', trans('general.file_not_found')); | ||
} | ||
|
||
return Storage::download($file, $log->filename, $headers); | ||
} | ||
|
||
return StorageHelper::downloader($file); | ||
return redirect()->route('hardware.show', ['hardware' => $asset])->with('error', trans('general.log_record_not_found')); | ||
} | ||
// Prepare the error message | ||
$error = trans('admin/hardware/message.does_not_exist', ['id' => $fileId]); | ||
|
||
// Redirect to the hardware management page | ||
return redirect()->route('hardware.index')->with('error', $error); | ||
return redirect()->route('hardware.index')->with('error', trans('admin/hardware/message.does_not_exist')); | ||
|
||
} | ||
|
||
/** | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I guess we're sure that we don't need this, yeah?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Grep told me we didn't.