[ticket/14285] Use route service for download routes

PHPBB3-14285

phpBB/config/default/container/services_avatar.yml

@@ -60,6 +60,7 @@ services:
         class: phpbb\avatar\driver\upload
         arguments:
             - '@config'
+            - '@controller.helper'
             - '%core.root_path%'
             - '%core.php_ext%'
             - '@storage.avatar'

phpBB/includes/acp/acp_attachments.php

@@ -14,6 +14,16 @@
 /**
 * @ignore
 */
+
+use phpbb\attachment\manager;
+use phpbb\config\config;
+use phpbb\controller\helper;
+use phpbb\db\driver\driver_interface;
+use phpbb\filesystem\filesystem_interface;
+use phpbb\language\language;
+use phpbb\template\template;
+use phpbb\user;
+
 if (!defined('IN_PHPBB'))
 {
 	exit;
@@ -21,30 +31,33 @@ if (!defined('IN_PHPBB'))
 
 class acp_attachments
 {
-	/** @var \phpbb\db\driver\driver_interface */
+	/** @var driver_interface */
 	protected $db;
 
-	/** @var \phpbb\config\config */
+	/** @var config */
 	protected $config;
 
-	/** @var \phpbb\language\language */
+	/** @var language */
 	protected $language;
 
 	/** @var ContainerBuilder */
 	protected $phpbb_container;
 
-	/** @var \phpbb\template\template */
+	/** @var template */
 	protected $template;
 
-	/** @var \phpbb\user */
+	/** @var user */
 	protected $user;
 
-	/** @var  \phpbb\filesystem\filesystem_interface */
+	/** @var  filesystem_interface */
 	protected $filesystem;
 
-	/** @var \phpbb\attachment\manager */
+	/** @var manager */
 	protected $attachment_manager;
 
+	/** @var helper */
+	protected $controller_helper;
+
 	public $id;
 	public $u_action;
 	protected $new_config;
@@ -63,6 +76,7 @@ class acp_attachments
 		$this->phpbb_container = $phpbb_container;
 		$this->filesystem = $phpbb_filesystem;
 		$this->attachment_manager = $phpbb_container->get('attachment.manager');
+		$this->controller_helper = $phpbb_container->get('controller.helper');
 
 		$user->add_lang(array('posting', 'viewtopic', 'acp/attachments'));
 
@@ -1082,8 +1096,8 @@ class acp_attachments
 						'PHYSICAL_FILENAME'	=> utf8_basename($row['physical_filename']),
 						'ATTACH_ID'			=> $row['attach_id'],
 						'POST_IDS'			=> (!empty($post_ids[$row['attach_id']])) ? $post_ids[$row['attach_id']] : '',
-						'U_FILE'			=> append_sid($phpbb_root_path . 'download/file.' . $phpEx, 'mode=view&id=' . $row['attach_id']))
+						'U_FILE'			=> $this->controller_helper->route('phpbb_storage_attachment', ['id' => (int) $row['attach_id']])
-					);
+					));
 				}
 				$db->sql_freeresult($result);
 
@@ -1270,8 +1284,8 @@ class acp_attachments
 						'S_IN_MESSAGE'		=> (bool) $row['in_message'],
 
 						'U_VIEW_TOPIC'		=> append_sid("{$phpbb_root_path}viewtopic.$phpEx", "t={$row['topic_id']}&p={$row['post_msg_id']}") . "#p{$row['post_msg_id']}",
-						'U_FILE'			=> append_sid($phpbb_root_path . 'download/file.' . $phpEx, 'mode=view&id=' . $row['attach_id']))
+						'U_FILE'			=> $this->controller_helper->route('phpbb_storage_attachment', ['id' => $row['attach_id']])
-					);
+					));
 				}
 
 			break;

phpBB/includes/acp/acp_users.php

@@ -14,6 +14,9 @@
 /**
 * @ignore
 */
+
+use phpbb\controller\helper;
+
 if (!defined('IN_PHPBB'))
 {
 	exit;
@@ -36,6 +39,9 @@ class acp_users
 		global $phpbb_dispatcher, $request;
 		global $phpbb_container, $phpbb_log;
 
+		/** @var helper $controller_helper */
+		$controller_helper = $phpbb_container->get('controller.helper');
+
 		$user->add_lang(array('posting', 'ucp', 'acp/users'));
 		$this->tpl_name = 'acp_users';
 
@@ -2123,9 +2129,6 @@ class acp_users
 					$decoded_message = generate_text_for_edit($signature, $bbcode_uid, $bbcode_flags);
 				}
 
-				/** @var \phpbb\controller\helper $controller_helper */
-				$controller_helper = $phpbb_container->get('controller.helper');
-
 				$template->assign_vars(array(
 					'S_SIGNATURE'		=> true,
 
@@ -2295,7 +2298,7 @@ class acp_users
 
 						'S_IN_MESSAGE'		=> $row['in_message'],
 
-						'U_DOWNLOAD'		=> append_sid("{$phpbb_root_path}download/file.$phpEx", 'mode=view&id=' . $row['attach_id']),
+						'U_DOWNLOAD'		=> $controller_helper->route('phpbb_storage_attachment', ['id' => (int) $row['attach_id']]),
 						'U_VIEW_TOPIC'		=> $view_topic)
 					);
 				}

phpBB/includes/functions_compatibility.php

@@ -894,11 +894,13 @@ function parse_cfg_file($filename, $lines = false)
 {
 	$parsed_items = array();
 
-	if ($lines === false) {
+	if ($lines === false)
+	{
 		$lines = file($filename);
 	}
 
-	foreach ($lines as $line) {
+	foreach ($lines as $line)
+	{
 		$line = trim($line);
 
 		if (!$line || $line[0] == '#' || ($delim_pos = strpos($line, '=')) === false)
@@ -934,7 +936,8 @@ function parse_cfg_file($filename, $lines = false)
 		$parsed_items[$key] = $value;
 	}
 
-	if (isset($parsed_items['parent']) && isset($parsed_items['name']) && $parsed_items['parent'] == $parsed_items['name']) {
+	if (isset($parsed_items['parent']) && isset($parsed_items['name']) && $parsed_items['parent'] == $parsed_items['name'])
+	{
 		unset($parsed_items['parent']);
 	}
 

phpBB/includes/functions_content.php

@@ -1124,6 +1124,9 @@ function parse_attachments($forum_id, &$message, &$attachments, &$update_count_a
 
 	$storage_attachment = $phpbb_container->get('storage.attachment');
 
+	/** @var \phpbb\controller\helper */
+	$controller_helper = $phpbb_container->get('controller.helper');
+
 	//
 	$compiled_attachments = array();
 
@@ -1283,15 +1286,14 @@ function parse_attachments($forum_id, &$message, &$attachments, &$update_count_a
 				$display_cat = ATTACHMENT_CATEGORY_NONE;
 			}
 
-			$download_link = append_sid("{$phpbb_root_path}download/file.$phpEx", 'id=' . $attachment['attach_id']);
+			$download_link = $controller_helper->route('phpbb_storage_attachment', ['id' => (int) $attachment['attach_id']]);
 			$l_downloaded_viewed = 'VIEWED_COUNTS';
 
 			switch ($display_cat)
 			{
 				// Images
 				case ATTACHMENT_CATEGORY_IMAGE:
-					$inline_link = append_sid("{$phpbb_root_path}download/file.$phpEx", 'id=' . $attachment['attach_id']);
+					$inline_link = $controller_helper->route('phpbb_storage_attachment', ['id' => (int) $attachment['attach_id']]);
-					$download_link .= '&mode=view';
 
 					$block_array += array(
 						'S_IMAGE'		=> true,
@@ -1303,8 +1305,7 @@ function parse_attachments($forum_id, &$message, &$attachments, &$update_count_a
 
 				// Images, but display Thumbnail
 				case ATTACHMENT_CATEGORY_THUMB:
-					$thumbnail_link = append_sid("{$phpbb_root_path}download/file.$phpEx", 'id=' . $attachment['attach_id'] . '&t=1');
+					$thumbnail_link = $controller_helper->route('phpbb_storage_attachment', ['id' => (int) $attachment['attach_id'], 't' => 1]);
-					$download_link .= '&mode=view';
 
 					$block_array += array(
 						'S_THUMBNAIL'		=> true,

phpBB/includes/functions_posting.php

@@ -779,7 +779,7 @@ function posting_gen_inline_attachments(&$attachment_data)
 */
 function posting_gen_attachment_entry($attachment_data, &$filename_data, $show_attach_box = true)
 {
-	global $template, $config, $phpbb_root_path, $phpEx, $user, $phpbb_dispatcher;
+	global $template, $config, $phpbb_root_path, $phpEx, $user, $phpbb_dispatcher, $phpbb_container;
 
 	// Some default template variables
 	$template->assign_vars(array(
@@ -807,7 +807,7 @@ function posting_gen_attachment_entry($attachment_data, &$filename_data, $show_a
 				$hidden .= '<input type="hidden" name="attachment_data[' . $count . '][' . $key . ']" value="' . $value . '" />';
 			}
 
-			$download_link = append_sid("{$phpbb_root_path}download/file.$phpEx", 'mode=view&amp;id=' . (int) $attach_row['attach_id'], true, ($attach_row['is_orphan']) ? $user->session_id : false);
+			$download_link = $phpbb_container->get('controller.helper')->route('phpbb_storage_attachment', ['id' => (int) $attach_row['attach_id']]);
 
 			$attachrow_template_vars[(int) $attach_row['attach_id']] = array(
 				'FILENAME'			=> utf8_basename($attach_row['real_filename']),

phpBB/includes/message_parser.php

@@ -1073,6 +1073,7 @@ class bbcode_firstpass extends bbcode
 			if ($pos_domain !== false && $pos_path >= $pos_domain && $pos_ext >= $pos_path)
 			{
 				// Ok, actually we allow linking to some files (this may be able to be extended in some way later...)
+				// @deprecated
 				if (strpos($url, '/' . $check_path . '/download/file.' . $phpEx) !== 0)
 				{
 					return false;
@@ -1534,6 +1535,8 @@ class parse_message extends bbcode_firstpass
 		global $config, $auth, $user, $phpbb_root_path, $phpEx, $db, $request;
 		global $phpbb_container, $phpbb_dispatcher;
 
+		$controller_helper = $phpbb_container->get('controller.helper');
+
 		$error = array();
 
 		$num_attachments = count($this->attachment_data);
@@ -1776,7 +1779,7 @@ class parse_message extends bbcode_firstpass
 
 						if (isset($this->plupload) && $this->plupload->is_active())
 						{
-							$download_url = append_sid("{$phpbb_root_path}download/file.{$phpEx}", 'mode=view&id=' . $new_entry['attach_id']);
+							$download_url = $controller_helper->route('phpbb_storage_attachment', ['id' => (int) $new_entry['attach_id']]);
 
 							// Send the client the attachment data to maintain state
 							$json_response->send(array('data' => $this->attachment_data, 'download_url' => $download_url));

phpBB/includes/ucp/ucp_attachments.php

@@ -14,6 +14,9 @@
 /**
 * @ignore
 */
+
+use phpbb\controller\helper;
+
 if (!defined('IN_PHPBB'))
 {
 	exit;
@@ -31,6 +34,9 @@ class ucp_attachments
 	{
 		global $template, $user, $db, $config, $phpEx, $phpbb_root_path, $phpbb_container, $request, $auth;
 
+		/** @var helper $controller_helper */
+		$controller_helper = $phpbb_container->get('controller.helper');
+
 		$start		= $request->variable('start', 0);
 		$sort_key	= $request->variable('sk', 'a');
 		$sort_dir	= $request->variable('sd', 'a');
@@ -179,7 +185,7 @@ class ucp_attachments
 					'S_IN_MESSAGE'		=> $row['in_message'],
 					'S_LOCKED'			=> !$row['in_message'] && !$auth->acl_get('m_edit', $row['forum_id']) && ($row['forum_status'] == ITEM_LOCKED || $row['topic_status'] == ITEM_LOCKED || $row['post_edit_locked']),
 
-					'U_VIEW_ATTACHMENT'	=> append_sid("{$phpbb_root_path}download/file.$phpEx", 'id=' . $row['attach_id']),
+					'U_VIEW_ATTACHMENT'	=> $controller_helper->route('phpbb_storage_attachment', ['id' => (int) $row['attach_id']]),
 					'U_VIEW_TOPIC'		=> $view_topic)
 				);
 

phpBB/phpbb/avatar/driver/upload.php

@@ -14,7 +14,13 @@
 namespace phpbb\avatar\driver;
 
 use bantu\IniGetWrapper\IniGetWrapper;
+use phpbb\config\config;
+use phpbb\controller\helper;
+use phpbb\event\dispatcher_interface;
+use phpbb\files\factory;
+use phpbb\path_helper;
 use phpbb\storage\exception\exception as storage_exception;
+use phpbb\storage\storage;
 
 /**
 * Handles avatars uploaded to the board
@@ -22,17 +28,22 @@ use phpbb\storage\exception\exception as storage_exception;
 class upload extends \phpbb\avatar\driver\driver
 {
 	/**
-	 * @var \phpbb\storage\storage
+	 * @var helper
+	 */
+	private $controller_helper;
+
+	/**
+	 * @var storage
 	 */
 	protected $storage;
 
 	/**
-	* @var \phpbb\event\dispatcher_interface
+	* @var dispatcher_interface
 	*/
 	protected $dispatcher;
 
 	/**
-	 * @var \phpbb\files\factory
+	 * @var factory
 	 */
 	protected $files_factory;
 
@@ -44,18 +55,20 @@ class upload extends \phpbb\avatar\driver\driver
 	/**
 	 * Construct a driver object
 	 *
-	* @param \phpbb\config\config $config phpBB configuration
+	 * @param config $config phpBB configuration
+	 * @param helper $controller_helper
 	 * @param string $phpbb_root_path Path to the phpBB root
 	 * @param string $php_ext PHP file extension
-	* @param \phpbb\storage\storage phpBB avatar storage
+	 * @param storage $storage phpBB avatar storage
-	* @param \phpbb\path_helper $path_helper phpBB path helper
+	 * @param path_helper $path_helper phpBB path helper
-	* @param \phpbb\event\dispatcher_interface $dispatcher phpBB Event dispatcher object
+	 * @param dispatcher_interface $dispatcher phpBB Event dispatcher object
-	* @param \phpbb\files\factory $files_factory File classes factory
+	 * @param factory $files_factory File classes factory
 	 * @param IniGetWrapper $php_ini ini_get() wrapper
 	 */
-	public function __construct(\phpbb\config\config $config, $phpbb_root_path, $php_ext, \phpbb\storage\storage $storage, \phpbb\path_helper $path_helper, \phpbb\event\dispatcher_interface $dispatcher, \phpbb\files\factory $files_factory, IniGetWrapper $php_ini)
+	public function __construct(config $config, helper $controller_helper, string $phpbb_root_path, string $php_ext, storage $storage, path_helper $path_helper, dispatcher_interface $dispatcher, factory $files_factory, IniGetWrapper $php_ini)
 	{
 		$this->config = $config;
+		$this->controller_helper = $controller_helper;
 		$this->phpbb_root_path = $phpbb_root_path;
 		$this->php_ext = $php_ext;
 		$this->storage = $storage;
@@ -73,7 +86,7 @@ class upload extends \phpbb\avatar\driver\driver
 		$root_path = (defined('PHPBB_USE_BOARD_URL_PATH') && PHPBB_USE_BOARD_URL_PATH) ? generate_board_url() . '/' : $this->path_helper->get_web_root_path();
 
 		return array(
-			'src' => $root_path . 'download/file.' . $this->php_ext . '?avatar=' . $row['avatar'],
+			'src' => $root_path . $this->controller_helper->route('phpbb_storage_avatar', ['file' => $row['avatar']]),
 			'width' => $row['avatar_width'],
 			'height' => $row['avatar_height'],
 		);

phpBB/phpbb/feed/helper.php

@@ -167,7 +167,9 @@ class helper
 			$content .= implode('<br />', $post_attachments);
 
 			// Convert attachments' relative path to absolute path
-			$content = str_replace($this->path_helper->get_web_root_path() . 'download/file.' . $this->path_helper->get_php_ext(), $this->get_board_url() . '/download/file.' . $this->path_helper->get_php_ext(), $content);
+			$pattern = '#(/app.php)/?download/attachment/#';
+			$replacement = $this->get_board_url() . '\1/download/attachment/';
+			$content = preg_replace($pattern, $replacement, $content);
 		}
 
 		// Remove Comments from inline attachments [ia]

tests/avatar/manager_test.php

@@ -56,6 +56,8 @@ class phpbb_avatar_manager_test extends \phpbb_database_test_case
 
 		$dispatcher = new phpbb_mock_event_dispatcher();
 
+		$controller_helper = $this->createMock('\phpbb\controller\helper');
+
 		// $this->avatar_foobar will be needed later on
 		$this->avatar_foobar = $this->getMockBuilder('\phpbb\avatar\driver\foobar')
 			->setMethods(array('get_name'))
@@ -93,7 +95,7 @@ class phpbb_avatar_manager_test extends \phpbb_database_test_case
 			{
 				$cur_avatar = $this->getMockBuilder('\phpbb\avatar\driver\\' . $driver)
 				->setMethods(array('get_name'))
-				->setConstructorArgs(array($this->config, $phpbb_root_path, $phpEx, $storage, $path_helper, $dispatcher, $files_factory, $php_ini))
+				->setConstructorArgs(array($this->config, $controller_helper, $phpbb_root_path, $phpEx, $storage, $path_helper, $dispatcher, $files_factory, $php_ini))
 				->getMock();
 			}
 			$cur_avatar->expects($this->any())

tests/template/extension_test.php

@@ -11,6 +11,8 @@
  *
  */
 
+use phpbb\controller\helper;
+
 require_once __DIR__ . '/template_test_case.php';
 
 class phpbb_template_extension_test extends phpbb_template_template_test_case
@@ -66,10 +68,17 @@ class phpbb_template_extension_test extends phpbb_template_template_test_case
 			->disableOriginalConstructor()
 			->getMock();
 
+		$controller_helper = $this->createMock(helper::class);
+		$controller_helper
+			->method('route')
+			->willReturnCallback(function($route, $params) {
+				return 'download/avatar/' . $params['file'];
+			});
+
 		$phpbb_dispatcher = new phpbb_mock_event_dispatcher();
 		$phpbb_container = new phpbb_mock_container_builder();
 		$files = new phpbb\files\factory($phpbb_container);
-		$upload_avatar_driver = new phpbb\avatar\driver\upload($config, $phpbb_root_path, $phpEx, $storage, $phpbb_path_helper, $phpbb_dispatcher, $files, new \bantu\IniGetWrapper\IniGetWrapper());
+		$upload_avatar_driver = new phpbb\avatar\driver\upload($config, $controller_helper, $phpbb_root_path, $phpEx, $storage, $phpbb_path_helper, $phpbb_dispatcher, $files, new \bantu\IniGetWrapper\IniGetWrapper());
 		$upload_avatar_driver->set_name('avatar.driver.upload');
 		$phpbb_container->set('avatar.manager', new \phpbb\avatar\manager($config, $phpbb_dispatcher, [
 			$upload_avatar_driver,
@@ -141,7 +150,7 @@ class phpbb_template_extension_test extends phpbb_template_template_test_case
 				],
 				[],
 				[],
-				'<img class="avatar" src="phpBB/download/file.php?avatar=great_avatar.png" width="90" height="90" alt="foo" />',
+				'<img class="avatar" src="phpBB/download/avatar/great_avatar.png" width="90" height="90" alt="foo" />',
 				[]
 			],
 			[
@@ -159,7 +168,7 @@ class phpbb_template_extension_test extends phpbb_template_template_test_case
 				],
 				[],
 				[],
-				'<img class="avatar" src="phpBB/styles//theme/images/no_avatar.gif" data-src="phpBB/download/file.php?avatar=great_avatar.png" width="90" height="90" alt="foo" />',
+				'<img class="avatar" src="phpBB/styles//theme/images/no_avatar.gif" data-src="phpBB/download/avatar/great_avatar.png" width="90" height="90" alt="foo" />',
 				[]
 			],
 			[