[ticket/14285] Update controllers

PHPBB3-14285

phpBB/config/default/routing/storage.yml

@@ -7,3 +7,5 @@ phpbb_storage_attachment:
     path:             /download/attachment/{file}
     defaults:
         _controller:  storage.controller.attachment:handle
+    requirements:
+        file:         \d+

phpBB/phpbb/storage/controller/attachment.php

@@ -19,9 +19,11 @@ use phpbb\config\config;
 use phpbb\content_visibility;
 use phpbb\db\driver\driver_interface;
 use phpbb\event\dispatcher;
+use phpbb\exception\http_exception;
 use phpbb\request\request;
 use phpbb\storage\storage;
 use phpbb\user;
+use Symfony\Component\HttpFoundation\RedirectResponse;
 
 class attachment extends controller
 {
@@ -61,10 +63,9 @@ class attachment extends controller
 
 	public function handle($file)
 	{
-		$attach_id = $file;
+		$attach_id = (int) $file;
 		$mode = $this->request->variable('mode', '');
 		$thumbnail = $this->request->variable('t', false);
-		global $phpbb_container;
 
 		// Start session management, do not update session page.
 		$this->user->session_begin(false);
@@ -73,14 +74,12 @@ class attachment extends controller
 
 		if (!$this->config['allow_attachments'] && !$this->config['allow_pm_attach'])
 		{
-			send_status_line(404, 'Not Found');
+			throw new http_exception(404, 'ATTACHMENT_FUNCTIONALITY_DISABLED');
-			trigger_error('ATTACHMENT_FUNCTIONALITY_DISABLED');
 		}
 
 		if (!$attach_id)
 		{
-			send_status_line(404, 'Not Found');
+			throw new http_exception(404, 'NO_ATTACHMENT_SELECTED');
-			trigger_error('NO_ATTACHMENT_SELECTED');
 		}
 
 		$sql = 'SELECT attach_id, post_msg_id, topic_id, in_message, poster_id, is_orphan, physical_filename, real_filename, extension, mimetype, filesize, filetime
@@ -92,133 +91,124 @@ class attachment extends controller
 
 		if (!$attachment)
 		{
-			send_status_line(404, 'Not Found');
+			throw new http_exception(404, 'ERROR_NO_ATTACHMENT');
-			trigger_error('ERROR_NO_ATTACHMENT');
 		}
 		else if (!$this->download_allowed())
 		{
-			send_status_line(403, 'Forbidden');
+			throw new http_exception(403, 'LINKAGE_FORBIDDEN');
-			trigger_error($this->user->lang['LINKAGE_FORBIDDEN']);
+		}
+
+		$attachment['physical_filename'] = utf8_basename($attachment['physical_filename']);
+
+		if (!$attachment['in_message'] && !$this->config['allow_attachments'] || $attachment['in_message'] && !$this->config['allow_pm_attach'])
+		{
+			throw new http_exception(404, 'ATTACHMENT_FUNCTIONALITY_DISABLED');
+		}
+
+		if ($attachment['is_orphan'])
+		{
+			// We allow admins having attachment permissions to see orphan attachments...
+			$own_attachment = ($this->auth->acl_get('a_attach') || $attachment['poster_id'] == $this->user->data['user_id']) ? true : false;
+
+			if (!$own_attachment || ($attachment['in_message'] && !$this->auth->acl_get('u_pm_download')) || (!$attachment['in_message'] && !$this->auth->acl_get('u_download')))
+			{
+				throw new http_exception(404, 'ERROR_NO_ATTACHMENT');
+			}
+
+			// Obtain all extensions...
+			$extensions = $this->cache->obtain_attach_extensions(true);
 		}
 		else
 		{
-			$attachment['physical_filename'] = utf8_basename($attachment['physical_filename']);
+			if (!$attachment['in_message'])
-
-			if (!$attachment['in_message'] && !$this->config['allow_attachments'] || $attachment['in_message'] && !$this->config['allow_pm_attach'])
 			{
-				send_status_line(404, 'Not Found');
+				$this->phpbb_download_handle_forum_auth($attachment['topic_id']);
-				trigger_error('ATTACHMENT_FUNCTIONALITY_DISABLED');
-			}
 
-			if ($attachment['is_orphan'])
+				$sql = 'SELECT forum_id, post_visibility
-			{
+					FROM ' . POSTS_TABLE . '
-				// We allow admins having attachment permissions to see orphan attachments...
+					WHERE post_id = ' . (int) $attachment['post_msg_id'];
-				$own_attachment = ($this->auth->acl_get('a_attach') || $attachment['poster_id'] == $this->user->data['user_id']) ? true : false;
+				$result = $this->db->sql_query($sql);
+				$post_row = $this->db->sql_fetchrow($result);
+				$this->db->sql_freeresult($result);
 
-				if (!$own_attachment || ($attachment['in_message'] && !$this->auth->acl_get('u_pm_download')) || (!$attachment['in_message'] && !$this->auth->acl_get('u_download')))
+				if (!$post_row || !$this->content_visibility->is_visible('post', $post_row['forum_id'], $post_row))
 				{
-					send_status_line(404, 'Not Found');
+					// Attachment of a soft deleted post and the user is not allowed to see the post
-					trigger_error('ERROR_NO_ATTACHMENT');
+					throw new http_exception(404, 'ERROR_NO_ATTACHMENT');
 				}
-
-				// Obtain all extensions...
-				$extensions = $this->cache->obtain_attach_extensions(true);
 			}
 			else
 			{
-				if (!$attachment['in_message'])
+				// Attachment is in a private message.
-				{
+				$post_row = array('forum_id' => false);
-					$this->phpbb_download_handle_forum_auth($attachment['topic_id']);
+				$this->phpbb_download_handle_pm_auth( $attachment['post_msg_id']);
-
-					$sql = 'SELECT forum_id, post_visibility
-						FROM ' . POSTS_TABLE . '
-						WHERE post_id = ' . (int) $attachment['post_msg_id'];
-					$result = $this->db->sql_query($sql);
-					$post_row = $this->db->sql_fetchrow($result);
-					$this->db->sql_freeresult($result);
-
-					if (!$post_row || !$this->content_visibility->is_visible('post', $post_row['forum_id'], $post_row))
-					{
-						// Attachment of a soft deleted post and the user is not allowed to see the post
-						send_status_line(404, 'Not Found');
-						trigger_error('ERROR_NO_ATTACHMENT');
-					}
-				}
-				else
-				{
-					// Attachment is in a private message.
-					$post_row = array('forum_id' => false);
-					$this->phpbb_download_handle_pm_auth( $attachment['post_msg_id']);
-				}
-
-				$extensions = array();
-				if (!extension_allowed($post_row['forum_id'], $attachment['extension'], $extensions))
-				{
-					send_status_line(403, 'Forbidden');
-					trigger_error(sprintf($this->user->lang['EXTENSION_DISABLED_AFTER_POSTING'], $attachment['extension']));
-				}
 			}
 
-			$display_cat = $extensions[$attachment['extension']]['display_cat'];
+			$extensions = array();
-
+			if (!extension_allowed($post_row['forum_id'], $attachment['extension'], $extensions))
-			if (($display_cat == ATTACHMENT_CATEGORY_IMAGE || $display_cat == ATTACHMENT_CATEGORY_THUMB) && !$this->user->optionget('viewimg'))
 			{
-				$display_cat = ATTACHMENT_CATEGORY_NONE;
+				throw new http_exception(403, 'EXTENSION_DISABLED_AFTER_POSTING', [$attachment['extension']]);
 			}
+		}
 
-			if ($display_cat == ATTACHMENT_CATEGORY_FLASH && !$this->user->optionget('viewflash'))
+		$display_cat = $extensions[$attachment['extension']]['display_cat'];
-			{
-				$display_cat = ATTACHMENT_CATEGORY_NONE;
-			}
 
-			if ($thumbnail)
+		if (($display_cat == ATTACHMENT_CATEGORY_IMAGE || $display_cat == ATTACHMENT_CATEGORY_THUMB) && !$this->user->optionget('viewimg'))
-			{
+		{
-				$attachment['physical_filename'] = 'thumb_' . $attachment['physical_filename'];
+			$display_cat = ATTACHMENT_CATEGORY_NONE;
-			}
+		}
-			else if ($display_cat == ATTACHMENT_CATEGORY_NONE && !$attachment['is_orphan'])
-			{
-				// Update download count
-				$this->phpbb_increment_downloads($attachment['attach_id']);
-			}
 
-			$redirect = '';
+		if ($display_cat == ATTACHMENT_CATEGORY_FLASH && !$this->user->optionget('viewflash'))
+		{
+			$display_cat = ATTACHMENT_CATEGORY_NONE;
+		}
 
-			/**
+		if ($thumbnail)
-			* Event to modify data before sending file to browser
+		{
-			*
+			$attachment['physical_filename'] = 'thumb_' . $attachment['physical_filename'];
-			* @event core.download_file_send_to_browser_before
+		}
-			* @var	int		attach_id			The attachment ID
+		else if ($display_cat == ATTACHMENT_CATEGORY_NONE && !$attachment['is_orphan'])
-			* @var	array	attachment			Array with attachment data
+		{
-			* @var	int		display_cat			Attachment category
+			// Update download count
-			* @var	array	extensions			Array with file extensions data
+			$this->phpbb_increment_downloads($attachment['attach_id']);
-			* @var	string	mode				Download mode
+		}
-			* @var	bool	thumbnail			Flag indicating if the file is a thumbnail
-			* @var	string	redirect			Do a redirection instead of reading the file
-			* @since 3.1.6-RC1
-			* @changed 3.1.7-RC1	Fixing wrong name of a variable (replacing "extension" by "extensions")
-			* @changed 3.3.0-a1		Add redirect variable
-			*/
-			$vars = array(
-				'attach_id',
-				'attachment',
-				'display_cat',
-				'extensions',
-				'mode',
-				'thumbnail',
-				'redirect',
-			);
-			extract($this->dispatcher->trigger_event('core.download_file_send_to_browser_before', compact($vars)));
 
-			if (!empty($redirect))
+		$redirect = '';
-			{
-				redirect($redirect, false, true);
-			}
-			else
-			{
-				$this->send_file_to_browser($attachment, $display_cat);
-			}
 
-			$this->file_gc();
+		/**
+		* Event to modify data before sending file to browser
+		*
+		* @event core.download_file_send_to_browser_before
+		* @var	int		attach_id			The attachment ID
+		* @var	array	attachment			Array with attachment data
+		* @var	int		display_cat			Attachment category
+		* @var	array	extensions			Array with file extensions data
+		* @var	string	mode				Download mode
+		* @var	bool	thumbnail			Flag indicating if the file is a thumbnail
+		* @var	string	redirect			Do a redirection instead of reading the file
+		* @since 3.1.6-RC1
+		* @changed 3.1.7-RC1	Fixing wrong name of a variable (replacing "extension" by "extensions")
+		* @changed 3.3.0-a1		Add redirect variable
+		*/
+		$vars = array(
+			'attach_id',
+			'attachment',
+			'display_cat',
+			'extensions',
+			'mode',
+			'thumbnail',
+			'redirect',
+		);
+		extract($this->dispatcher->trigger_event('core.download_file_send_to_browser_before', compact($vars)));
+
+		if (!empty($redirect))
+		{
+			$response = new RedirectResponse($redirect);
+			$response->send();
+		}
+		else
+		{
+			$this->send_file_to_browser($attachment, $display_cat);
 		}
 	}
 
@@ -231,8 +221,7 @@ class attachment extends controller
 
 		if (!$this->storage->exists($filename))
 		{
-			send_status_line(404, 'Not Found');
+			throw new http_exception(404, 'ERROR_NO_ATTACHMENT');
-			trigger_error('ERROR_NO_ATTACHMENT');
 		}
 
 		// Correct the mime type - we force application/octetstream for all files, except images
@@ -281,8 +270,7 @@ class attachment extends controller
 		// Check if headers already sent or not able to get the file contents.
 		if (headers_sent())
 		{
-			send_status_line(500, 'Internal Server Error');
+			throw new http_exception(500, 'UNABLE_TO_DELIVER_FILE');
-			trigger_error('UNABLE_TO_DELIVER_FILE');
 		}
 
 		// Make sure the database record for the filesize is correct
@@ -331,7 +319,7 @@ class attachment extends controller
 			$fp = $this->storage->read_stream($filename);
 
 			// Close the db connection before sending the file etc.
-			$this->file_gc(false);
+			$this->file_gc();
 
 			if ($fp !== false)
 			{
@@ -372,8 +360,7 @@ class attachment extends controller
 
 		if ($row && !$this->content_visibility->is_visible('topic', $row['forum_id'], $row))
 		{
-			send_status_line(404, 'Not Found');
+			throw new http_exception(404, 'ERROR_NO_ATTACHMENT');
-			trigger_error('ERROR_NO_ATTACHMENT');
 		}
 		else if ($row && $this->auth->acl_get('u_download') && $this->auth->acl_get('f_download', $row['forum_id']))
 		{
@@ -385,8 +372,7 @@ class attachment extends controller
 		}
 		else
 		{
-			send_status_line(403, 'Forbidden');
+			throw new http_exception(403, 'SORRY_AUTH_VIEW_ATTACH');
-			trigger_error('SORRY_AUTH_VIEW_ATTACH');
 		}
 	}
 
@@ -401,8 +387,7 @@ class attachment extends controller
 	{
 		if (!$this->auth->acl_get('u_pm_download'))
 		{
-			send_status_line(403, 'Forbidden');
+			throw new http_exception(403, 'SORRY_AUTH_VIEW_ATTACH');
-			trigger_error('SORRY_AUTH_VIEW_ATTACH');
 		}
 
 		$allowed = $this->phpbb_download_check_pm_auth($msg_id);
@@ -421,8 +406,7 @@ class attachment extends controller
 
 		if (!$allowed)
 		{
-			send_status_line(403, 'Forbidden');
+			throw new http_exception(403, 'ERROR_NO_ATTACHMENT');
-			trigger_error('ERROR_NO_ATTACHMENT');
 		}
 	}
 

phpBB/phpbb/storage/controller/controller.php

@@ -15,6 +15,7 @@ namespace phpbb\storage\controller;
 
 use phpbb\cache\service;
 use phpbb\db\driver\driver_interface;
+use phpbb\exception\http_exception;
 use phpbb\storage\storage;
 
 class controller
@@ -40,16 +41,12 @@ class controller
 	{
 		if (!$this->is_allowed($file))
 		{
-			send_status_line(403, 'Forbidden');
+			throw new http_exception(403, 'Forbidden');
-			$this->file_gc();
-			exit;
 		}
 
 		if (!$this->file_exists($file))
 		{
-			send_status_line(404, 'Not Found');
+			throw new http_exception(404, 'Not Found');
-			$this->file_gc();
-			exit;
 		}
 
 		$this->send($file);
@@ -94,7 +91,7 @@ class controller
 			$fp = $this->storage->read_stream($file);
 
 			// Close db connection
-			$this->file_gc(false);
+			$this->file_gc();
 
 			$output = fopen('php://output', 'w+b');
 
@@ -114,7 +111,7 @@ class controller
 	*
 	* @return null
 	*/
-	protected function file_gc($exit = true)
+	protected function file_gc()
 	{
 		if (!empty($this->cache))
 		{
@@ -122,10 +119,5 @@ class controller
 		}
 
 		$this->db->sql_close();
-
-		if ($exit)
-		{
-			exit;
-		}
 	}
 }