From df6ab1a811697d9ef5fe4a5854fa6b30fc93af4c Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Tue, 27 Jun 2023 21:00:16 +0200 Subject: [PATCH 1/5] [ticket/15687] Add attachment filename to attachment URL This will also fix the invalid requirements for the file parameter of the URL. PHPBB3-15687 --- phpBB/config/default/routing/storage.yml | 5 +++-- phpBB/includes/acp/acp_attachments.php | 16 +++++++++++++-- phpBB/includes/acp/acp_users.php | 8 +++++++- phpBB/includes/functions_content.php | 25 +++++++++++++++++++++--- phpBB/includes/functions_posting.php | 9 ++++++++- phpBB/includes/message_parser.php | 8 +++++++- phpBB/includes/ucp/ucp_attachments.php | 8 +++++++- 7 files changed, 68 insertions(+), 11 deletions(-) diff --git a/phpBB/config/default/routing/storage.yml b/phpBB/config/default/routing/storage.yml index ac135c5e6b..1149d8cf69 100644 --- a/phpBB/config/default/routing/storage.yml +++ b/phpBB/config/default/routing/storage.yml @@ -4,8 +4,9 @@ phpbb_storage_avatar: _controller: storage.controller.avatar:handle phpbb_storage_attachment: - path: /attachment/{file} + path: /attachment/{file}/{filename} defaults: + filename: '' _controller: storage.controller.attachment:handle requirements: - id: \d+ + file: \d+ diff --git a/phpBB/includes/acp/acp_attachments.php b/phpBB/includes/acp/acp_attachments.php index 329900130e..8b4806a6bf 100644 --- a/phpBB/includes/acp/acp_attachments.php +++ b/phpBB/includes/acp/acp_attachments.php @@ -1112,7 +1112,13 @@ class acp_attachments 'PHYSICAL_FILENAME' => utf8_basename($row['physical_filename']), 'ATTACH_ID' => $row['attach_id'], 'POST_ID' => (!empty($post_ids[$row['attach_id']])) ? $post_ids[$row['attach_id']] : '', - 'U_FILE' => $this->controller_helper->route('phpbb_storage_attachment', ['file' => (int) $row['attach_id']]) + 'U_FILE' => $this->controller_helper->route( + 'phpbb_storage_attachment', + [ + 'file' => (int) $row['attach_id'], + 'filename' => $row['real_filename'], + ] + ), ]); } $db->sql_freeresult($result); @@ -1302,7 +1308,13 @@ class acp_attachments 'S_IN_MESSAGE' => (bool) $row['in_message'], 'U_VIEW_TOPIC' => append_sid("{$phpbb_root_path}viewtopic.$phpEx", "p={$row['post_msg_id']}") . "#p{$row['post_msg_id']}", - 'U_FILE' => $this->controller_helper->route('phpbb_storage_attachment', ['file' => $row['attach_id']]) + 'U_FILE' => $this->controller_helper->route( + 'phpbb_storage_attachment', + [ + 'file' => $row['attach_id'], + 'filename' => $row['real_filename'], + ] + ) )); } diff --git a/phpBB/includes/acp/acp_users.php b/phpBB/includes/acp/acp_users.php index 8be78c24f2..6305a91d70 100644 --- a/phpBB/includes/acp/acp_users.php +++ b/phpBB/includes/acp/acp_users.php @@ -2306,7 +2306,13 @@ class acp_users 'S_IN_MESSAGE' => $row['in_message'], - 'U_DOWNLOAD' => $controller_helper->route('phpbb_storage_attachment', ['file' => (int) $row['attach_id']]), + 'U_DOWNLOAD' => $controller_helper->route( + 'phpbb_storage_attachment', + [ + 'file' => (int) $row['attach_id'], + 'filename' => $row['real_filename'], + ] + ), 'U_VIEW_TOPIC' => $view_topic) ); } diff --git a/phpBB/includes/functions_content.php b/phpBB/includes/functions_content.php index 349f3582c4..7c228d7a24 100644 --- a/phpBB/includes/functions_content.php +++ b/phpBB/includes/functions_content.php @@ -1285,14 +1285,26 @@ function parse_attachments($forum_id, &$message, &$attachments, &$update_count_a $display_cat = attachment_category::NONE; } - $download_link = $controller_helper->route('phpbb_storage_attachment', ['file' => (int) $attachment['attach_id']]); + $download_link = $controller_helper->route( + 'phpbb_storage_attachment', + [ + 'file' => (int) $attachment['attach_id'], + 'filename' => $attachment['real_filename'], + ] + ); $l_downloaded_viewed = 'VIEWED_COUNTS'; switch ($display_cat) { // Images case attachment_category::IMAGE: - $inline_link = $controller_helper->route('phpbb_storage_attachment', ['file' => (int) $attachment['attach_id']]); + $inline_link = $controller_helper->route( + 'phpbb_storage_attachment', + [ + 'file' => (int) $attachment['attach_id'], + 'filename' => $attachment['real_filename'], + ] + ); $block_array += array( 'S_IMAGE' => true, @@ -1304,7 +1316,14 @@ function parse_attachments($forum_id, &$message, &$attachments, &$update_count_a // Images, but display Thumbnail case attachment_category::THUMB: - $thumbnail_link = $controller_helper->route('phpbb_storage_attachment', ['file' => (int) $attachment['attach_id'], 't' => 1]); + $thumbnail_link = $controller_helper->route( + 'phpbb_storage_attachment', + [ + 'file' => (int) $attachment['attach_id'], + 'filename' => $attachment['real_filename'], + 't' => 1, + ] + ); $block_array += array( 'S_THUMBNAIL' => true, diff --git a/phpBB/includes/functions_posting.php b/phpBB/includes/functions_posting.php index d4673531cb..2a0ecd2b53 100644 --- a/phpBB/includes/functions_posting.php +++ b/phpBB/includes/functions_posting.php @@ -868,7 +868,14 @@ function posting_gen_attachment_entry($attachment_data, &$filename_data, $show_a $hidden .= ''; } - $download_link = $phpbb_container->get('controller.helper')->route('phpbb_storage_attachment', ['file' => (int) $attach_row['attach_id']]); + $download_link = $phpbb_container->get('controller.helper') + ->route( + 'phpbb_storage_attachment', + [ + 'file' => (int) $attach_row['attach_id'], + 'filename' => $attachment['real_filename'], + ] + ); $attachrow_template_vars[(int) $attach_row['attach_id']] = array( 'FILENAME' => utf8_basename($attach_row['real_filename']), diff --git a/phpBB/includes/message_parser.php b/phpBB/includes/message_parser.php index 205d0682e3..912769f70e 100644 --- a/phpBB/includes/message_parser.php +++ b/phpBB/includes/message_parser.php @@ -1716,7 +1716,13 @@ class parse_message extends bbcode_firstpass if (isset($this->plupload) && $this->plupload->is_active()) { - $download_url = $controller_helper->route('phpbb_storage_attachment', ['file' => (int) $new_entry['attach_id']]); + $download_url = $controller_helper->route( + 'phpbb_storage_attachment', + [ + 'file' => (int) $new_entry['attach_id'], + 'filename' => $attachment['real_filename'], + ] + ); // Send the client the attachment data to maintain state $json_response->send(array('data' => $this->attachment_data, 'download_url' => $download_url)); diff --git a/phpBB/includes/ucp/ucp_attachments.php b/phpBB/includes/ucp/ucp_attachments.php index 5330fdfd94..c143bea429 100644 --- a/phpBB/includes/ucp/ucp_attachments.php +++ b/phpBB/includes/ucp/ucp_attachments.php @@ -185,7 +185,13 @@ 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' => $controller_helper->route('phpbb_storage_attachment', ['file' => (int) $row['attach_id']]), + 'U_VIEW_ATTACHMENT' => $controller_helper->route( + 'phpbb_storage_attachment', + [ + 'file' => (int) $row['attach_id'], + 'filename' => $row['real_filename'], + ] + ), 'U_VIEW_TOPIC' => $view_topic) ); From b02ad26015cd96bf5d78bab77521dc51ad9515e8 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Tue, 27 Jun 2023 22:16:30 +0200 Subject: [PATCH 2/5] [ticket/15687] Fix incorrect variable names thanks to copy & paste PHPBB3-15687 --- phpBB/includes/functions_posting.php | 2 +- phpBB/includes/message_parser.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/phpBB/includes/functions_posting.php b/phpBB/includes/functions_posting.php index 2a0ecd2b53..b46a8b2a45 100644 --- a/phpBB/includes/functions_posting.php +++ b/phpBB/includes/functions_posting.php @@ -873,7 +873,7 @@ function posting_gen_attachment_entry($attachment_data, &$filename_data, $show_a 'phpbb_storage_attachment', [ 'file' => (int) $attach_row['attach_id'], - 'filename' => $attachment['real_filename'], + 'filename' => $attach_row['real_filename'], ] ); diff --git a/phpBB/includes/message_parser.php b/phpBB/includes/message_parser.php index 912769f70e..6b7fa8733c 100644 --- a/phpBB/includes/message_parser.php +++ b/phpBB/includes/message_parser.php @@ -1720,7 +1720,7 @@ class parse_message extends bbcode_firstpass 'phpbb_storage_attachment', [ 'file' => (int) $new_entry['attach_id'], - 'filename' => $attachment['real_filename'], + 'filename' => $new_entry['real_filename'], ] ); From 5cb0b267d315a5453589eb50c51dd7e375816ca7 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Tue, 27 Jun 2023 22:34:44 +0200 Subject: [PATCH 3/5] [ticket/15687] Adjust regex for attachment URL PHPBB3-15687 --- tests/functional/acp_attachments_test.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/functional/acp_attachments_test.php b/tests/functional/acp_attachments_test.php index bed79eaaf2..099a38c329 100644 --- a/tests/functional/acp_attachments_test.php +++ b/tests/functional/acp_attachments_test.php @@ -86,7 +86,7 @@ class phpbb_functional_acp_attachments_test extends phpbb_functional_test_case // Get attach id, the link looks similar to ./download/attachment/3 $attach_link = $crawler->filter('span.file-name > a')->attr('href'); $matches = []; - preg_match('/\/([0-9]+)$/', $attach_link, $matches); + preg_match('/\/([0-9]+)\/valid\.jpg$/', $attach_link, $matches); $attach_id = (int) $matches[1]; // Set file time older than 3 hours to consider it orphan From 4e9cf239ed2a26e4992efa53ca7e2625f97801ae Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Wed, 28 Jun 2023 10:51:22 +0200 Subject: [PATCH 4/5] [ticket/15687] Force supplied filename to be correct and modify route PHPBB3-15687 --- phpBB/config/default/routing/storage.yml | 6 +++--- phpBB/download/file.php | 2 +- phpBB/includes/acp/acp_attachments.php | 4 ++-- phpBB/includes/acp/acp_users.php | 2 +- phpBB/includes/functions_content.php | 6 +++--- phpBB/includes/functions_posting.php | 2 +- phpBB/includes/message_parser.php | 2 +- phpBB/includes/ucp/ucp_attachments.php | 2 +- phpBB/phpbb/storage/controller/attachment.php | 12 ++++++++---- 9 files changed, 21 insertions(+), 17 deletions(-) diff --git a/phpBB/config/default/routing/storage.yml b/phpBB/config/default/routing/storage.yml index 1149d8cf69..ec66c8f544 100644 --- a/phpBB/config/default/routing/storage.yml +++ b/phpBB/config/default/routing/storage.yml @@ -4,9 +4,9 @@ phpbb_storage_avatar: _controller: storage.controller.avatar:handle phpbb_storage_attachment: - path: /attachment/{file}/{filename} + path: /attachment/{id}/{filename} defaults: filename: '' - _controller: storage.controller.attachment:handle + _controller: storage.controller.attachment:handle_attachment requirements: - file: \d+ + id: \d+ diff --git a/phpBB/download/file.php b/phpBB/download/file.php index c71cc2274c..e1eddf5bba 100644 --- a/phpBB/download/file.php +++ b/phpBB/download/file.php @@ -47,7 +47,7 @@ $thumbnail = $request->variable('t', false); $response = new RedirectResponse( $controller_helper->route('phpbb_storage_attachment', array( - 'file' => $attach_id, + 'id' => $attach_id, 't' => $thumbnail, ), false), 301 diff --git a/phpBB/includes/acp/acp_attachments.php b/phpBB/includes/acp/acp_attachments.php index 8b4806a6bf..f8969f8007 100644 --- a/phpBB/includes/acp/acp_attachments.php +++ b/phpBB/includes/acp/acp_attachments.php @@ -1115,7 +1115,7 @@ class acp_attachments 'U_FILE' => $this->controller_helper->route( 'phpbb_storage_attachment', [ - 'file' => (int) $row['attach_id'], + 'id' => (int) $row['attach_id'], 'filename' => $row['real_filename'], ] ), @@ -1311,7 +1311,7 @@ class acp_attachments 'U_FILE' => $this->controller_helper->route( 'phpbb_storage_attachment', [ - 'file' => $row['attach_id'], + 'id' => $row['attach_id'], 'filename' => $row['real_filename'], ] ) diff --git a/phpBB/includes/acp/acp_users.php b/phpBB/includes/acp/acp_users.php index 6305a91d70..268bc16a49 100644 --- a/phpBB/includes/acp/acp_users.php +++ b/phpBB/includes/acp/acp_users.php @@ -2309,7 +2309,7 @@ class acp_users 'U_DOWNLOAD' => $controller_helper->route( 'phpbb_storage_attachment', [ - 'file' => (int) $row['attach_id'], + 'id' => (int) $row['attach_id'], 'filename' => $row['real_filename'], ] ), diff --git a/phpBB/includes/functions_content.php b/phpBB/includes/functions_content.php index 7c228d7a24..2a2e9be5de 100644 --- a/phpBB/includes/functions_content.php +++ b/phpBB/includes/functions_content.php @@ -1288,7 +1288,7 @@ function parse_attachments($forum_id, &$message, &$attachments, &$update_count_a $download_link = $controller_helper->route( 'phpbb_storage_attachment', [ - 'file' => (int) $attachment['attach_id'], + 'id' => (int) $attachment['attach_id'], 'filename' => $attachment['real_filename'], ] ); @@ -1301,7 +1301,7 @@ function parse_attachments($forum_id, &$message, &$attachments, &$update_count_a $inline_link = $controller_helper->route( 'phpbb_storage_attachment', [ - 'file' => (int) $attachment['attach_id'], + 'id' => (int) $attachment['attach_id'], 'filename' => $attachment['real_filename'], ] ); @@ -1319,7 +1319,7 @@ function parse_attachments($forum_id, &$message, &$attachments, &$update_count_a $thumbnail_link = $controller_helper->route( 'phpbb_storage_attachment', [ - 'file' => (int) $attachment['attach_id'], + 'id' => (int) $attachment['attach_id'], 'filename' => $attachment['real_filename'], 't' => 1, ] diff --git a/phpBB/includes/functions_posting.php b/phpBB/includes/functions_posting.php index b46a8b2a45..1551ba7ae9 100644 --- a/phpBB/includes/functions_posting.php +++ b/phpBB/includes/functions_posting.php @@ -872,7 +872,7 @@ function posting_gen_attachment_entry($attachment_data, &$filename_data, $show_a ->route( 'phpbb_storage_attachment', [ - 'file' => (int) $attach_row['attach_id'], + 'id' => (int) $attach_row['attach_id'], 'filename' => $attach_row['real_filename'], ] ); diff --git a/phpBB/includes/message_parser.php b/phpBB/includes/message_parser.php index 6b7fa8733c..e82b2168b4 100644 --- a/phpBB/includes/message_parser.php +++ b/phpBB/includes/message_parser.php @@ -1719,7 +1719,7 @@ class parse_message extends bbcode_firstpass $download_url = $controller_helper->route( 'phpbb_storage_attachment', [ - 'file' => (int) $new_entry['attach_id'], + 'id' => (int) $new_entry['attach_id'], 'filename' => $new_entry['real_filename'], ] ); diff --git a/phpBB/includes/ucp/ucp_attachments.php b/phpBB/includes/ucp/ucp_attachments.php index c143bea429..666170574b 100644 --- a/phpBB/includes/ucp/ucp_attachments.php +++ b/phpBB/includes/ucp/ucp_attachments.php @@ -188,7 +188,7 @@ class ucp_attachments 'U_VIEW_ATTACHMENT' => $controller_helper->route( 'phpbb_storage_attachment', [ - 'file' => (int) $row['attach_id'], + 'id' => (int) $row['attach_id'], 'filename' => $row['real_filename'], ] ), diff --git a/phpBB/phpbb/storage/controller/attachment.php b/phpBB/phpbb/storage/controller/attachment.php index 08cd012290..d9e108e098 100644 --- a/phpBB/phpbb/storage/controller/attachment.php +++ b/phpBB/phpbb/storage/controller/attachment.php @@ -86,11 +86,14 @@ class attachment extends controller } /** - * {@inheritdoc} + * Handle attachments + * + * @param int $id File ID + * @param string $filename Filename */ - public function handle(string $file): Response + public function handle_attachment(int $id, string $filename): Response { - $attach_id = (int) $file; + $attach_id = $id; $thumbnail = $this->request->variable('t', false); $this->language->add_lang('viewtopic'); @@ -109,7 +112,8 @@ class attachment extends controller is_orphan, physical_filename, real_filename, extension, mimetype, filesize, filetime FROM ' . ATTACHMENTS_TABLE . " - WHERE attach_id = $attach_id"; + WHERE attach_id = $attach_id" . + (($filename) ? " AND real_filename = '" . $this->db->sql_escape($filename) . "'" : ''); $result = $this->db->sql_query($sql); $attachment = $this->db->sql_fetchrow($result); $this->db->sql_freeresult($result); From c48092f6e76e1eb8f4d2692f9c55415938cd0426 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Thu, 20 Jul 2023 09:09:59 +0200 Subject: [PATCH 5/5] [ticket/15687] Add missing int cast PHPBB3-15687 --- phpBB/includes/acp/acp_attachments.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/phpBB/includes/acp/acp_attachments.php b/phpBB/includes/acp/acp_attachments.php index f8969f8007..4eb06bd6f8 100644 --- a/phpBB/includes/acp/acp_attachments.php +++ b/phpBB/includes/acp/acp_attachments.php @@ -1311,7 +1311,7 @@ class acp_attachments 'U_FILE' => $this->controller_helper->route( 'phpbb_storage_attachment', [ - 'id' => $row['attach_id'], + 'id' => (int) $row['attach_id'], 'filename' => $row['real_filename'], ] )