[ticket/15286] Use storage in attachments

PHPBB3-15286

phpBB/config/default/container/services_attachment.yml

@@ -36,5 +36,6 @@ services:
             - '@mimetype.guesser'
             - '@dispatcher'
             - '@plupload'
+            - '@storage.attachment'
             - '@user'
             - '%core.root_path%'

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:

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"
 			),

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;

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();
 		}
 	}

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
 	*/

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'] . '<br />' . 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();
 	}

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');
 

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');
+			$source = $this->file->get('filename');
-			$destination = $this->file->get('destination_path') . '/thumb_' . $this->file->get('realname');
+			$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())
 			{

phpBB/phpbb/db/migration/data/v330/storage_attachment.php

@@ -0,0 +1,26 @@
+<?php
+/**
+*
+* This file is part of the phpBB Forum Software package.
+*
+* @copyright (c) phpBB Limited <https://www.phpbb.com>
+* @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')),
+		);
+	}
+}

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;
 

phpBB/phpbb/files/types/local_storage.php

@@ -0,0 +1,136 @@
+<?php
+/**
+ *
+ * This file is part of the phpBB Forum Software package.
+ *
+ * @copyright (c) phpBB Limited <https://www.phpbb.com>
+ * @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;
+	}
+}