From e749e060421f7b6b0250cfd455f6fc0361eeb9d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20Calvo?= Date: Wed, 9 Aug 2017 12:21:19 +0200 Subject: [PATCH] [ticket/15286] Use storage in attachments PHPBB3-15286 --- .../default/container/services_attachment.yml | 1 + .../default/container/services_storage.yml | 8 ++ phpBB/docs/lighttpd.sample.conf | 27 ++-- phpBB/docs/nginx.sample.conf | 8 -- phpBB/download/file.php | 2 +- phpBB/includes/acp/acp_attachments.php | 48 ------- phpBB/includes/functions_download.php | 56 +++----- phpBB/install/schemas/schema_data.sql | 3 +- phpBB/phpbb/attachment/upload.php | 51 ++++--- .../data/v330/storage_attachment.php | 26 ++++ phpBB/phpbb/files/filespec_storage.php | 3 - phpBB/phpbb/files/types/local_storage.php | 136 ++++++++++++++++++ 12 files changed, 233 insertions(+), 136 deletions(-) create mode 100644 phpBB/phpbb/db/migration/data/v330/storage_attachment.php create mode 100644 phpBB/phpbb/files/types/local_storage.php diff --git a/phpBB/config/default/container/services_attachment.yml b/phpBB/config/default/container/services_attachment.yml index c56ced21f4..9253726496 100644 --- a/phpBB/config/default/container/services_attachment.yml +++ b/phpBB/config/default/container/services_attachment.yml @@ -36,5 +36,6 @@ services: - '@mimetype.guesser' - '@dispatcher' - '@plupload' + - '@storage.attachment' - '@user' - '%core.root_path%' diff --git a/phpBB/config/default/container/services_storage.yml b/phpBB/config/default/container/services_storage.yml index 3593b8264a..f114dc1dfe 100644 --- a/phpBB/config/default/container/services_storage.yml +++ b/phpBB/config/default/container/services_storage.yml @@ -1,6 +1,14 @@ services: # Storages + storage.attachment: + class: phpbb\storage\storage + arguments: + - '@storage.adapter.factory' + - 'attachment' + tags: + - { name: storage } + storage.avatar: class: phpbb\storage\storage arguments: diff --git a/phpBB/docs/lighttpd.sample.conf b/phpBB/docs/lighttpd.sample.conf index f5b509e002..e783c809fc 100644 --- a/phpBB/docs/lighttpd.sample.conf +++ b/phpBB/docs/lighttpd.sample.conf @@ -3,17 +3,8 @@ # from your system's lighttpd.conf. # Tested with lighttpd 1.4.35 -# If you want to use the X-Sendfile feature, -# uncomment the 'allow-x-send-file' for the fastcgi -# server below and add the following to your config.php -# -# define('PHPBB_ENABLE_X_SENDFILE', true); -# -# See http://blog.lighttpd.net/articles/2006/07/02/x-sendfile -# for the details on X-Sendfile. - # Load moules -server.modules += ( +server.modules += ( "mod_access", "mod_fastcgi", "mod_rewrite", @@ -32,11 +23,11 @@ $HTTP["host"] == "www.myforums.com" { server.name = "www.myforums.com" server.document-root = "/path/to/phpbb" server.dir-listing = "disable" - + index-file.names = ( "index.php", "index.htm", "index.html" ) accesslog.filename = "/var/log/lighttpd/access-www.myforums.com.log" - - # Deny access to internal phpbb files. + + # Deny access to internal phpbb files. $HTTP["url"] =~ "^/(config\.php|common\.php|cache|files|images/avatars/upload|includes|phpbb|store|vendor)" { url.access-deny = ( "" ) } @@ -45,12 +36,12 @@ $HTTP["host"] == "www.myforums.com" { $HTTP["url"] =~ "/\.svn|/\.git" { url.access-deny = ( "" ) } - + # Deny access to apache configuration files. $HTTP["url"] =~ "/\.htaccess|/\.htpasswd|/\.htgroups" { url.access-deny = ( "" ) } - + # The following 3 lines will rewrite URLs passed through the front controller # to not require app.php in the actual URL. In other words, a controller is # by default accessed at /app.php/my/controller, but can also be accessed at @@ -58,14 +49,14 @@ $HTTP["host"] == "www.myforums.com" { url.rewrite-if-not-file = ( "^/(.*)$" => "/app.php/$1" ) - - fastcgi.server = ( ".php" => + + fastcgi.server = ( ".php" => (( "bin-path" => "/usr/bin/php-cgi", "socket" => "/tmp/php.socket", "max-procs" => 4, "idle-timeout" => 30, - "bin-environment" => ( + "bin-environment" => ( "PHP_FCGI_CHILDREN" => "10", "PHP_FCGI_MAX_REQUESTS" => "10000" ), diff --git a/phpBB/docs/nginx.sample.conf b/phpBB/docs/nginx.sample.conf index 55c01a1fc9..4e40400b36 100644 --- a/phpBB/docs/nginx.sample.conf +++ b/phpBB/docs/nginx.sample.conf @@ -3,14 +3,6 @@ # from your system's nginx.conf. # Tested with nginx 0.8.35. -# If you want to use the X-Accel-Redirect feature, -# add the following to your config.php. -# -# define('PHPBB_ENABLE_X_ACCEL_REDIRECT', true); -# -# See http://wiki.nginx.org/XSendfile for the details -# on X-Accel-Redirect. - http { # Compression - requires gzip and gzip static modules. gzip on; diff --git a/phpBB/download/file.php b/phpBB/download/file.php index c7540c5380..a885c886cd 100644 --- a/phpBB/download/file.php +++ b/phpBB/download/file.php @@ -324,7 +324,7 @@ else } else { - send_file_to_browser($attachment, $config['upload_path'], $display_cat); + send_file_to_browser($attachment, $display_cat); file_gc(); } } diff --git a/phpBB/includes/acp/acp_attachments.php b/phpBB/includes/acp/acp_attachments.php index dc4eb66cf8..c735c6265b 100644 --- a/phpBB/includes/acp/acp_attachments.php +++ b/phpBB/includes/acp/acp_attachments.php @@ -147,7 +147,6 @@ class acp_attachments 'allow_attachments' => array('lang' => 'ALLOW_ATTACHMENTS', 'validate' => 'bool', 'type' => 'radio:yes_no', 'explain' => false), 'allow_pm_attach' => array('lang' => 'ALLOW_PM_ATTACHMENTS', 'validate' => 'bool', 'type' => 'radio:yes_no', 'explain' => false), - 'upload_path' => array('lang' => 'UPLOAD_DIR', 'validate' => 'wpath', 'type' => 'text:25:100', 'explain' => true), 'display_order' => array('lang' => 'DISPLAY_ORDER', 'validate' => 'bool', 'type' => 'custom', 'method' => 'display_order', 'explain' => true), 'attachment_quota' => array('lang' => 'ATTACH_QUOTA', 'validate' => 'string', 'type' => 'custom', 'method' => 'max_filesize', 'explain' => true), 'max_filesize' => array('lang' => 'ATTACH_MAX_FILESIZE', 'validate' => 'string', 'type' => 'custom', 'method' => 'max_filesize', 'explain' => true), @@ -223,9 +222,6 @@ class acp_attachments { $phpbb_log->add('admin', $user->data['user_id'], $user->ip, 'LOG_CONFIG_ATTACH'); - // Check Settings - $this->test_upload($error, $this->new_config['upload_path'], false); - if (!count($error)) { trigger_error($user->lang['CONFIG_UPDATED'] . adm_back_link($this->u_action)); @@ -1536,50 +1532,6 @@ class acp_attachments return $imagick; } - /** - * Test Settings - */ - function test_upload(&$error, $upload_dir, $create_directory = false) - { - global $user, $phpbb_root_path; - - // Does the target directory exist, is it a directory and writable. - if ($create_directory) - { - if (!file_exists($phpbb_root_path . $upload_dir)) - { - @mkdir($phpbb_root_path . $upload_dir, 0777); - - try - { - $this->filesystem->phpbb_chmod($phpbb_root_path . $upload_dir, CHMOD_READ | CHMOD_WRITE); - } - catch (\phpbb\filesystem\exception\filesystem_exception $e) - { - // Do nothing - } - } - } - - if (!file_exists($phpbb_root_path . $upload_dir)) - { - $error[] = sprintf($user->lang['NO_UPLOAD_DIR'], $upload_dir); - return; - } - - if (!is_dir($phpbb_root_path . $upload_dir)) - { - $error[] = sprintf($user->lang['UPLOAD_NOT_DIR'], $upload_dir); - return; - } - - if (!$this->filesystem->is_writable($phpbb_root_path . $upload_dir)) - { - $error[] = sprintf($user->lang['NO_WRITE_UPLOAD'], $upload_dir); - return; - } - } - /** * Perform operations on sites for external linking */ diff --git a/phpBB/includes/functions_download.php b/phpBB/includes/functions_download.php index b6414c40dc..b6b55ab59d 100644 --- a/phpBB/includes/functions_download.php +++ b/phpBB/includes/functions_download.php @@ -121,13 +121,15 @@ function wrap_img_in_html($src, $title) /** * Send file to browser */ -function send_file_to_browser($attachment, $upload_dir, $category) +function send_file_to_browser($attachment, $category) { - global $user, $db, $phpbb_dispatcher, $phpbb_root_path, $request; + global $user, $db, $phpbb_dispatcher, $request, $phpbb_container; - $filename = $phpbb_root_path . $upload_dir . '/' . $attachment['physical_filename']; + $storage = $phpbb_container->get('storage.attachment'); - if (!@file_exists($filename)) + $filename = $attachment['physical_filename']; + + if (!$storage->exists($filename)) { send_status_line(404, 'Not Found'); trigger_error('ERROR_NO_ATTACHMENT'); @@ -146,14 +148,22 @@ function send_file_to_browser($attachment, $upload_dir, $category) } // Now send the File Contents to the Browser - $size = @filesize($filename); + try + { + $file_info = $storage->file_info($filename); + $size = $file_info->size; + } + catch (\Exception $e) + { + $size = 0; + } + /** * Event to alter attachment before it is sent to browser. * * @event core.send_file_to_browser_before * @var array attachment Attachment data - * @var string upload_dir Relative path of upload directory * @var int category Attachment category * @var string filename Path to file, including filename * @var int size File size @@ -161,7 +171,6 @@ function send_file_to_browser($attachment, $upload_dir, $category) */ $vars = array( 'attachment', - 'upload_dir', 'category', 'filename', 'size', @@ -171,15 +180,8 @@ function send_file_to_browser($attachment, $upload_dir, $category) // To correctly display further errors we need to make sure we are using the correct headers for both (unsetting content-length may not work) // Check if headers already sent or not able to get the file contents. - if (headers_sent() || !@file_exists($filename) || !@is_readable($filename)) + if (headers_sent()) { - // PHP track_errors setting On? - if (!empty($php_errormsg)) - { - send_status_line(500, 'Internal Server Error'); - trigger_error($user->lang['UNABLE_TO_DELIVER_FILE'] . '
' . sprintf($user->lang['TRACKED_PHP_ERROR'], $php_errormsg)); - } - send_status_line(500, 'Internal Server Error'); trigger_error('UNABLE_TO_DELIVER_FILE'); } @@ -235,24 +237,6 @@ function send_file_to_browser($attachment, $upload_dir, $category) if (!set_modified_headers($attachment['filetime'], $user->browser)) { - // We make sure those have to be enabled manually by defining a constant - // because of the potential disclosure of full attachment path - // in case support for features is absent in the webserver software. - if (defined('PHPBB_ENABLE_X_ACCEL_REDIRECT') && PHPBB_ENABLE_X_ACCEL_REDIRECT) - { - // X-Accel-Redirect - http://wiki.nginx.org/XSendfile - header('X-Accel-Redirect: ' . $user->page['root_script_path'] . $upload_dir . '/' . $attachment['physical_filename']); - exit; - } - else if (defined('PHPBB_ENABLE_X_SENDFILE') && PHPBB_ENABLE_X_SENDFILE && !phpbb_http_byte_range($size)) - { - // X-Sendfile - http://blog.lighttpd.net/articles/2006/07/02/x-sendfile - // Lighttpd's X-Sendfile does not support range requests as of 1.4.26 - // and always requires an absolute path. - header('X-Sendfile: ' . dirname(__FILE__) . "/../$upload_dir/{$attachment['physical_filename']}"); - exit; - } - if ($size) { header("Content-Length: $size"); @@ -261,7 +245,7 @@ function send_file_to_browser($attachment, $upload_dir, $category) // Try to deliver in chunks @set_time_limit(0); - $fp = @fopen($filename, 'rb'); + $fp = $storage->read_stream($filename); if ($fp !== false) { @@ -291,10 +275,6 @@ function send_file_to_browser($attachment, $upload_dir, $category) } fclose($fp); } - else - { - @readfile($filename); - } flush(); } diff --git a/phpBB/install/schemas/schema_data.sql b/phpBB/install/schemas/schema_data.sql index 58a7ff4f63..c802480eea 100644 --- a/phpBB/install/schemas/schema_data.sql +++ b/phpBB/install/schemas/schema_data.sql @@ -277,7 +277,6 @@ INSERT INTO phpbb_config (config_name, config_value) VALUES ('teampage_forums', INSERT INTO phpbb_config (config_name, config_value) VALUES ('topics_per_page', '25'); INSERT INTO phpbb_config (config_name, config_value) VALUES ('tpl_allow_php', '0'); INSERT INTO phpbb_config (config_name, config_value) VALUES ('upload_icons_path', 'images/upload_icons'); -INSERT INTO phpbb_config (config_name, config_value) VALUES ('upload_path', 'files'); INSERT INTO phpbb_config (config_name, config_value) VALUES ('use_system_cron', '0'); INSERT INTO phpbb_config (config_name, config_value) VALUES ('version', '3.3.0-a1-dev'); INSERT INTO phpbb_config (config_name, config_value) VALUES ('warnings_expire_days', '90'); @@ -288,6 +287,8 @@ INSERT INTO phpbb_config (config_name, config_value) VALUES ('exts_composer_json INSERT INTO phpbb_config (config_name, config_value) VALUES ('exts_composer_vendor_dir', 'vendor-ext/'); INSERT INTO phpbb_config (config_name, config_value) VALUES ('exts_composer_enable_on_install', '0'); INSERT INTO phpbb_config (config_name, config_value) VALUES ('exts_composer_purge_on_remove', '1'); +INSERT INTO phpbb_config (config_name, config_value) VALUES ('storage\attachment\provider', 'phpbb\storage\provider\local'); +INSERT INTO phpbb_config (config_name, config_value) VALUES ('storage\attachment\config\path', 'files'); INSERT INTO phpbb_config (config_name, config_value) VALUES ('storage\avatar\provider', 'phpbb\storage\provider\local'); INSERT INTO phpbb_config (config_name, config_value) VALUES ('storage\avatar\config\path', 'images/avatars/upload'); diff --git a/phpBB/phpbb/attachment/upload.php b/phpBB/phpbb/attachment/upload.php index b9d32058db..70ce439381 100644 --- a/phpBB/phpbb/attachment/upload.php +++ b/phpBB/phpbb/attachment/upload.php @@ -20,6 +20,7 @@ use \phpbb\event\dispatcher; use \phpbb\language\language; use \phpbb\mimetype\guesser; use \phpbb\plupload\plupload; +use \phpbb\storage\storage; use \phpbb\user; /** @@ -51,6 +52,9 @@ class upload /** @var plupload Plupload */ protected $plupload; + /** @var storage */ + protected $storage; + /** @var user */ protected $user; @@ -79,7 +83,7 @@ class upload * @param user $user * @param $phpbb_root_path */ - public function __construct(auth $auth, service $cache, config $config, \phpbb\files\upload $files_upload, language $language, guesser $mimetype_guesser, dispatcher $phpbb_dispatcher, plupload $plupload, user $user, $phpbb_root_path) + public function __construct(auth $auth, service $cache, config $config, \phpbb\files\upload $files_upload, language $language, guesser $mimetype_guesser, dispatcher $phpbb_dispatcher, plupload $plupload, storage $storage, user $user, $phpbb_root_path) { $this->auth = $auth; $this->cache = $cache; @@ -89,6 +93,7 @@ class upload $this->mimetype_guesser = $mimetype_guesser; $this->phpbb_dispatcher = $phpbb_dispatcher; $this->plupload = $plupload; + $this->storage = $storage; $this->user = $user; $this->phpbb_root_path = $phpbb_root_path; } @@ -118,7 +123,7 @@ class upload return $this->file_data; } - $this->file = ($local) ? $this->files_upload->handle_upload('files.types.local', $local_storage, $local_filedata) : $this->files_upload->handle_upload('files.types.form', $form_name); + $this->file = ($local) ? $this->files_upload->handle_upload('files.types.local_storage', $local_storage, $local_filedata) : $this->files_upload->handle_upload('files.types.form_storage', $form_name); if ($this->file->init_error()) { @@ -152,25 +157,12 @@ class upload $this->file->clean_filename('unique', $this->user->data['user_id'] . '_'); - // Are we uploading an image *and* this image being within the image category? - // Only then perform additional image checks. - $this->file->move_file($this->config['upload_path'], false, !$is_image); - // Do we have to create a thumbnail? $this->file_data['thumbnail'] = ($is_image && $this->config['img_create_thumbnail']) ? 1 : 0; // Make sure the image category only holds valid images... $this->check_image($is_image); - if (count($this->file->error)) - { - $this->file->remove(); - $this->file_data['error'] = array_merge($this->file_data['error'], $this->file->error); - $this->file_data['post_attach'] = false; - - return $this->file_data; - } - $this->fill_file_data(); $filedata = $this->file_data; @@ -200,6 +192,19 @@ class upload // Create Thumbnail $this->create_thumbnail(); + // Are we uploading an image *and* this image being within the image category? + // Only then perform additional image checks. + $this->file->move_file($this->storage, false, !$is_image); + + if (count($this->file->error)) + { + $this->file->remove($this->storage); + $this->file_data['error'] = array_merge($this->file_data['error'], $this->file->error); + $this->file_data['post_attach'] = false; + + return $this->file_data; + } + return $this->file_data; } @@ -212,10 +217,18 @@ class upload { if ($this->file_data['thumbnail']) { - $source = $this->file->get('destination_file'); - $destination = $this->file->get('destination_path') . '/thumb_' . $this->file->get('realname'); + $source = $this->file->get('filename'); + $destination_name = 'thumb_' . $this->file->get('realname'); + $destination = sys_get_temp_dir() . '/' . $destination_name; - if (!create_thumbnail($source, $destination, $this->file->get('mimetype'))) + if (create_thumbnail($source, $destination, $this->file->get('mimetype'))) + { + // Move the thumbnail from temp folder to the storage + $fp = fopen($destination, 'rb'); + $this->storage->write_stream($destination_name, $fp); + fclose($fp); + } + else { $this->file_data['thumbnail'] = 0; } @@ -253,7 +266,7 @@ class upload // Make sure the image category only holds valid images... if ($is_image && !$this->file->is_image()) { - $this->file->remove(); + $this->file->remove($this->storage); if ($this->plupload && $this->plupload->is_active()) { diff --git a/phpBB/phpbb/db/migration/data/v330/storage_attachment.php b/phpBB/phpbb/db/migration/data/v330/storage_attachment.php new file mode 100644 index 0000000000..626bafed65 --- /dev/null +++ b/phpBB/phpbb/db/migration/data/v330/storage_attachment.php @@ -0,0 +1,26 @@ + +* @license GNU General Public License, version 2 (GPL-2.0) +* +* For full copyright and license information, please see +* the docs/CREDITS.txt file. +* +*/ + +namespace phpbb\db\migration\data\v330; + +class storage_attachment extends \phpbb\db\migration\migration +{ + public function update_data() + { + return array( + array('config.add', array('storage\\attachment\\provider', \phpbb\storage\provider\local::class)), + array('config.add', array('storage\\attachment\\config\\path', $this->config['upload_path'])), + array('config.remove', array('upload_path')), + ); + } +} diff --git a/phpBB/phpbb/files/filespec_storage.php b/phpBB/phpbb/files/filespec_storage.php index b370b66a4e..8b6194fe99 100644 --- a/phpBB/phpbb/files/filespec_storage.php +++ b/phpBB/phpbb/files/filespec_storage.php @@ -51,9 +51,6 @@ class filespec_storage /** @var string Destination file name */ protected $destination_file = ''; - /** @var string Destination file path */ - protected $destination_path = ''; - /** @var bool Whether file was moved */ protected $file_moved = false; diff --git a/phpBB/phpbb/files/types/local_storage.php b/phpBB/phpbb/files/types/local_storage.php new file mode 100644 index 0000000000..7485bf5362 --- /dev/null +++ b/phpBB/phpbb/files/types/local_storage.php @@ -0,0 +1,136 @@ + + * @license GNU General Public License, version 2 (GPL-2.0) + * + * For full copyright and license information, please see + * the docs/CREDITS.txt file. + * + */ + +namespace phpbb\files\types; + +use bantu\IniGetWrapper\IniGetWrapper; +use phpbb\files\factory; +use phpbb\files\filespec; +use phpbb\language\language; +use phpbb\request\request_interface; + +class local extends base +{ + /** @var factory Files factory */ + protected $factory; + + /** @var language */ + protected $language; + + /** @var IniGetWrapper */ + protected $php_ini; + + /** @var request_interface */ + protected $request; + + /** @var \phpbb\files\upload */ + protected $upload; + + /** + * Construct a form upload type + * + * @param factory $factory Files factory + * @param language $language Language class + * @param IniGetWrapper $php_ini ini_get() wrapper + * @param request_interface $request Request object + */ + public function __construct(factory $factory, language $language, IniGetWrapper $php_ini, request_interface $request) + { + $this->factory = $factory; + $this->language = $language; + $this->php_ini = $php_ini; + $this->request = $request; + } + + /** + * {@inheritdoc} + */ + public function upload() + { + $args = func_get_args(); + return $this->local_upload($args[0], isset($args[1]) ? $args[1] : false); + } + + /** + * Move file from another location to phpBB + * + * @param string $source_file Filename of source file + * @param array|bool $filedata Array with filedata or false + * + * @return filespec Object "filespec" is returned, all further operations can be done with this object + */ + protected function local_upload($source_file, $filedata = false) + { + $upload = $this->get_upload_ary($source_file, $filedata); + + /** @var filespec $file */ + $file = $this->factory->get('filespec_storage') + ->set_upload_ary($upload) + ->set_upload_namespace($this->upload); + + if ($file->init_error()) + { + $file->error[] = ''; + return $file; + } + + // PHP Upload file size check + $file = $this->check_upload_size($file); + if (count($file->error)) + { + return $file; + } + + // Not correctly uploaded + if (!$file->is_uploaded()) + { + $file->error[] = $this->language->lang($this->upload->error_prefix . 'NOT_UPLOADED'); + return $file; + } + + $this->upload->common_checks($file); + $this->request->overwrite('local', $upload, request_interface::FILES); + + return $file; + } + + /** + * Retrieve upload array + * + * @param string $source_file Source file name + * @param array $filedata File data array + * + * @return array Upload array + */ + protected function get_upload_ary($source_file, $filedata) + { + $upload = array(); + + $upload['local_mode'] = true; + $upload['tmp_name'] = $source_file; + + if ($filedata === false) + { + $upload['name'] = utf8_basename($source_file); + $upload['size'] = 0; + } + else + { + $upload['name'] = $filedata['realname']; + $upload['size'] = $filedata['size']; + $upload['type'] = $filedata['type']; + } + + return $upload; + } +}