From 3fe8344104d867a0ce5ec2ba9eebddf7cdc9f143 Mon Sep 17 00:00:00 2001 From: Andreas Fischer Date: Tue, 7 Aug 2012 00:07:22 +0200 Subject: [PATCH 01/17] [feature/attach-dl] Use "else if" for precedence in case of multiple arguments. PHPBB3-11042 --- phpBB/download/file.php | 36 ++++++++++++------------------------ 1 file changed, 12 insertions(+), 24 deletions(-) diff --git a/phpBB/download/file.php b/phpBB/download/file.php index 6332095df8..d8ee4082a5 100644 --- a/phpBB/download/file.php +++ b/phpBB/download/file.php @@ -140,18 +140,6 @@ $archive = $request->variable('archive', '.tar'); $mode = request_var('mode', ''); $thumbnail = request_var('t', false); -// Ensure we're only performing one operation -if ($download_id) -{ - $topic_id = false; - $post_id = false; -} - -if ($post_id) -{ - $topic_id = false; -} - // Start session management, do not update session page. $user->session_begin(false); $auth->acl($user->data); @@ -172,6 +160,8 @@ if (!$config['allow_attachments'] && !$config['allow_pm_attach']) $attachment = ($download_id) ? array() : false; $attachments = ($topic_id || $post_id) ? array() : false; +// If multiple arguments are provided, the precedence is as follows: +// $download_id, $post_id, $topic_id if ($download_id) { $sql = 'SELECT attach_id, in_message, post_msg_id, extension, is_orphan, poster_id, filetime @@ -181,8 +171,17 @@ if ($download_id) $attachment = $db->sql_fetchrow($result); $db->sql_freeresult($result); } +else if ($post_id) +{ + $sql = 'SELECT attach_id, in_message, post_msg_id, extension, is_orphan, poster_id, filetime + FROM ' . ATTACHMENTS_TABLE . " + WHERE post_msg_id = $post_id"; -if ($topic_id) + $result = $db->sql_query($sql); + $attachments = $db->sql_fetchrowset($result); + $db->sql_freeresult($result); +} +else if ($topic_id) { $sql = 'SELECT a.attach_id, a.in_message, a.post_msg_id, a.extension, a.is_orphan, a.poster_id, a.filetime FROM ' . POSTS_TABLE . ' p, ' . ATTACHMENTS_TABLE . " a @@ -195,17 +194,6 @@ if ($topic_id) $db->sql_freeresult($result); } -if ($post_id) -{ - $sql = 'SELECT attach_id, in_message, post_msg_id, extension, is_orphan, poster_id, filetime - FROM ' . ATTACHMENTS_TABLE . " - WHERE post_msg_id = $post_id"; - - $result = $db->sql_query($sql); - $attachments = $db->sql_fetchrowset($result); - $db->sql_freeresult($result); -} - if (!$attachment && !$attachments) { send_status_line(404, 'Not Found'); From 2d32164fb074e12f12a38cc41f424611551d9573 Mon Sep 17 00:00:00 2001 From: Andreas Fischer Date: Tue, 7 Aug 2012 00:10:24 +0200 Subject: [PATCH 02/17] [feature/attach-dl] Remove unnecessary LIMIT from primary key query. PHPBB3-11042 --- phpBB/download/file.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/phpBB/download/file.php b/phpBB/download/file.php index d8ee4082a5..f540869a1e 100644 --- a/phpBB/download/file.php +++ b/phpBB/download/file.php @@ -167,7 +167,7 @@ if ($download_id) $sql = 'SELECT attach_id, in_message, post_msg_id, extension, is_orphan, poster_id, filetime FROM ' . ATTACHMENTS_TABLE . " WHERE attach_id = $download_id"; - $result = $db->sql_query_limit($sql, 1); + $result = $db->sql_query($sql); $attachment = $db->sql_fetchrow($result); $db->sql_freeresult($result); } From 862502aacd36918ba9c9d5524e625c02a3830897 Mon Sep 17 00:00:00 2001 From: Andreas Fischer Date: Tue, 7 Aug 2012 00:31:18 +0200 Subject: [PATCH 03/17] [feature/attach-dl] Use table aliases in all three cases. PHPBB3-11042 --- phpBB/download/file.php | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/phpBB/download/file.php b/phpBB/download/file.php index f540869a1e..bd96ad02f7 100644 --- a/phpBB/download/file.php +++ b/phpBB/download/file.php @@ -164,18 +164,18 @@ $attachments = ($topic_id || $post_id) ? array() : false; // $download_id, $post_id, $topic_id if ($download_id) { - $sql = 'SELECT attach_id, in_message, post_msg_id, extension, is_orphan, poster_id, filetime - FROM ' . ATTACHMENTS_TABLE . " - WHERE attach_id = $download_id"; + $sql = 'SELECT a.attach_id, a.in_message, a.post_msg_id, a.extension, a.is_orphan, a.poster_id, a.filetime + FROM ' . ATTACHMENTS_TABLE . " a + WHERE a.attach_id = $download_id"; $result = $db->sql_query($sql); $attachment = $db->sql_fetchrow($result); $db->sql_freeresult($result); } else if ($post_id) { - $sql = 'SELECT attach_id, in_message, post_msg_id, extension, is_orphan, poster_id, filetime - FROM ' . ATTACHMENTS_TABLE . " - WHERE post_msg_id = $post_id"; + $sql = 'SELECT a.attach_id, a.in_message, a.post_msg_id, a.extension, a.is_orphan, a.poster_id, a.filetime + FROM ' . ATTACHMENTS_TABLE . " a + WHERE a.post_msg_id = $post_id"; $result = $db->sql_query($sql); $attachments = $db->sql_fetchrowset($result); From 5986676f4dd59f9b5c69a37ee392575d7f7f0375 Mon Sep 17 00:00:00 2001 From: Andreas Fischer Date: Tue, 7 Aug 2012 00:33:23 +0200 Subject: [PATCH 04/17] [feature/attach-dl] Exploit the "if ... else if ..." for the error message. Use an "else" statement instead of checking everything at the top. PHPBB3-11042 --- phpBB/download/file.php | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/phpBB/download/file.php b/phpBB/download/file.php index bd96ad02f7..bddd6a27aa 100644 --- a/phpBB/download/file.php +++ b/phpBB/download/file.php @@ -145,12 +145,6 @@ $user->session_begin(false); $auth->acl($user->data); $user->setup('viewtopic'); -if (!$download_id && !$post_id && !$topic_id) -{ - send_status_line(404, 'Not Found'); - trigger_error('NO_ATTACHMENT_SELECTED'); -} - if (!$config['allow_attachments'] && !$config['allow_pm_attach']) { send_status_line(404, 'Not Found'); @@ -193,6 +187,11 @@ else if ($topic_id) $attachments = $db->sql_fetchrowset($result); $db->sql_freeresult($result); } +else +{ + send_status_line(404, 'Not Found'); + trigger_error('NO_ATTACHMENT_SELECTED'); +} if (!$attachment && !$attachments) { From 56cd7e54756b743e1e2f7587fe574c1a2c395add Mon Sep 17 00:00:00 2001 From: Andreas Fischer Date: Tue, 7 Aug 2012 01:03:32 +0200 Subject: [PATCH 05/17] [feature/attach-dl] Store query result always in $attachments first. PHPBB3-11042 --- phpBB/download/file.php | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/phpBB/download/file.php b/phpBB/download/file.php index bddd6a27aa..e50c4e18b2 100644 --- a/phpBB/download/file.php +++ b/phpBB/download/file.php @@ -151,9 +151,6 @@ if (!$config['allow_attachments'] && !$config['allow_pm_attach']) trigger_error('ATTACHMENT_FUNCTIONALITY_DISABLED'); } -$attachment = ($download_id) ? array() : false; -$attachments = ($topic_id || $post_id) ? array() : false; - // If multiple arguments are provided, the precedence is as follows: // $download_id, $post_id, $topic_id if ($download_id) @@ -162,7 +159,7 @@ if ($download_id) FROM ' . ATTACHMENTS_TABLE . " a WHERE a.attach_id = $download_id"; $result = $db->sql_query($sql); - $attachment = $db->sql_fetchrow($result); + $attachments = $db->sql_fetchrowset($result); $db->sql_freeresult($result); } else if ($post_id) @@ -193,11 +190,15 @@ else trigger_error('NO_ATTACHMENT_SELECTED'); } -if (!$attachment && !$attachments) +if (empty($attachments)) { send_status_line(404, 'Not Found'); trigger_error('ERROR_NO_ATTACHMENT'); } +else if ($download_id) +{ + $attachment = current($attachments); +} if ($attachment && ((!$attachment['in_message'] && !$config['allow_attachments']) || ($attachment['in_message'] && !$config['allow_pm_attach']))) { From 940b9e0658348be7c66b7f5375f086f2d370abb2 Mon Sep 17 00:00:00 2001 From: Andreas Fischer Date: Tue, 7 Aug 2012 01:12:19 +0200 Subject: [PATCH 06/17] [feature/attach-dl] Combine download_id and post_id queries. PHPBB3-11042 --- phpBB/download/file.php | 17 +++-------------- 1 file changed, 3 insertions(+), 14 deletions(-) diff --git a/phpBB/download/file.php b/phpBB/download/file.php index e50c4e18b2..0978f93402 100644 --- a/phpBB/download/file.php +++ b/phpBB/download/file.php @@ -153,21 +153,11 @@ if (!$config['allow_attachments'] && !$config['allow_pm_attach']) // If multiple arguments are provided, the precedence is as follows: // $download_id, $post_id, $topic_id -if ($download_id) +if ($download_id || $post_id) { $sql = 'SELECT a.attach_id, a.in_message, a.post_msg_id, a.extension, a.is_orphan, a.poster_id, a.filetime - FROM ' . ATTACHMENTS_TABLE . " a - WHERE a.attach_id = $download_id"; - $result = $db->sql_query($sql); - $attachments = $db->sql_fetchrowset($result); - $db->sql_freeresult($result); -} -else if ($post_id) -{ - $sql = 'SELECT a.attach_id, a.in_message, a.post_msg_id, a.extension, a.is_orphan, a.poster_id, a.filetime - FROM ' . ATTACHMENTS_TABLE . " a - WHERE a.post_msg_id = $post_id"; - + FROM ' . ATTACHMENTS_TABLE . ' a + WHERE ' . ($download_id ? "a.attach_id = $download_id" : "a.post_msg_id = $post_id"); $result = $db->sql_query($sql); $attachments = $db->sql_fetchrowset($result); $db->sql_freeresult($result); @@ -179,7 +169,6 @@ else if ($topic_id) WHERE p.topic_id = $topic_id AND p.post_attachment = 1 AND a.post_msg_id = p.post_id"; - $result = $db->sql_query($sql); $attachments = $db->sql_fetchrowset($result); $db->sql_freeresult($result); From 87c822b79448bf375bc76c383d7d9a3606074b19 Mon Sep 17 00:00:00 2001 From: Andreas Fischer Date: Tue, 7 Aug 2012 01:47:25 +0200 Subject: [PATCH 07/17] [feature/attach-dl] Also merge topic_id query. a.topic_id can be used. PHPBB3-11042 --- phpBB/download/file.php | 37 ++++++++++++++++++++++--------------- 1 file changed, 22 insertions(+), 15 deletions(-) diff --git a/phpBB/download/file.php b/phpBB/download/file.php index 0978f93402..75ff708b16 100644 --- a/phpBB/download/file.php +++ b/phpBB/download/file.php @@ -151,24 +151,31 @@ if (!$config['allow_attachments'] && !$config['allow_pm_attach']) trigger_error('ATTACHMENT_FUNCTIONALITY_DISABLED'); } -// If multiple arguments are provided, the precedence is as follows: -// $download_id, $post_id, $topic_id -if ($download_id || $post_id) +if ($download_id || $post_id || $topic_id) { $sql = 'SELECT a.attach_id, a.in_message, a.post_msg_id, a.extension, a.is_orphan, a.poster_id, a.filetime FROM ' . ATTACHMENTS_TABLE . ' a - WHERE ' . ($download_id ? "a.attach_id = $download_id" : "a.post_msg_id = $post_id"); - $result = $db->sql_query($sql); - $attachments = $db->sql_fetchrowset($result); - $db->sql_freeresult($result); -} -else if ($topic_id) -{ - $sql = 'SELECT a.attach_id, a.in_message, a.post_msg_id, a.extension, a.is_orphan, a.poster_id, a.filetime - FROM ' . POSTS_TABLE . ' p, ' . ATTACHMENTS_TABLE . " a - WHERE p.topic_id = $topic_id - AND p.post_attachment = 1 - AND a.post_msg_id = p.post_id"; + WHERE '; + + switch (true) + { + default: + case $download_id: + // Attachment id (only 1 attachment) + $sql .= "a.attach_id = $download_id"; + break; + + case $post_id: + // Post id or private message id (multiple attachments) + $sql .= "a.post_msg_id = $post_id"; + break; + + case $topic_id: + // Topic id (multiple attachments) + $sql .= "a.topic_id = $topic_id"; + break; + } + $result = $db->sql_query($sql); $attachments = $db->sql_fetchrowset($result); $db->sql_freeresult($result); From 60d382df4c9ce50447b07abf04ed97a6319b14b0 Mon Sep 17 00:00:00 2001 From: Andreas Fischer Date: Tue, 7 Aug 2012 02:10:15 +0200 Subject: [PATCH 08/17] [feature/attach-dl] Putting some old code under "else if ($download_id)". PHPBB3-11042 --- phpBB/download/file.php | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/phpBB/download/file.php b/phpBB/download/file.php index 75ff708b16..4f6bc6738c 100644 --- a/phpBB/download/file.php +++ b/phpBB/download/file.php @@ -193,13 +193,19 @@ if (empty($attachments)) } else if ($download_id) { + // sizeof($attachments) == 1 $attachment = current($attachments); -} -if ($attachment && ((!$attachment['in_message'] && !$config['allow_attachments']) || ($attachment['in_message'] && !$config['allow_pm_attach']))) + // in_message = 1 means it's in a private message + if (!$attachment['in_message'] && !$config['allow_attachments'] || $attachment['in_message'] && !$config['allow_pm_attach']) + { + send_status_line(404, 'Not Found'); + trigger_error('ATTACHMENT_FUNCTIONALITY_DISABLED'); + } +} +else { - send_status_line(404, 'Not Found'); - trigger_error('ATTACHMENT_FUNCTIONALITY_DISABLED'); + // sizeof($attachments) > 1 } $row = array(); From ecab0212f8dbea04cd9a9a34a597db246a5290cd Mon Sep 17 00:00:00 2001 From: Andreas Fischer Date: Tue, 7 Aug 2012 02:36:20 +0200 Subject: [PATCH 09/17] [feature/attach-dl] Putting more old code under "else if ($download_id)". PHPBB3-11042 --- phpBB/download/file.php | 193 ++++++++++++++++++++++------------------ 1 file changed, 106 insertions(+), 87 deletions(-) diff --git a/phpBB/download/file.php b/phpBB/download/file.php index 4f6bc6738c..67f315dd2f 100644 --- a/phpBB/download/file.php +++ b/phpBB/download/file.php @@ -196,123 +196,142 @@ else if ($download_id) // sizeof($attachments) == 1 $attachment = current($attachments); - // in_message = 1 means it's in a private message if (!$attachment['in_message'] && !$config['allow_attachments'] || $attachment['in_message'] && !$config['allow_pm_attach']) { send_status_line(404, 'Not Found'); trigger_error('ATTACHMENT_FUNCTIONALITY_DISABLED'); } -} -else -{ - // sizeof($attachments) > 1 -} -$row = array(); - -if ($attachment && $attachment['is_orphan']) -{ - // We allow admins having attachment permissions to see orphan attachments... - $own_attachment = ($auth->acl_get('a_attach') || $attachment['poster_id'] == $user->data['user_id']) ? true : false; - - if (!$own_attachment || ($attachment['in_message'] && !$auth->acl_get('u_pm_download')) || (!$attachment['in_message'] && !$auth->acl_get('u_download'))) + if ($attachment['is_orphan']) { - send_status_line(404, 'Not Found'); - trigger_error('ERROR_NO_ATTACHMENT'); + // We allow admins having attachment permissions to see orphan attachments... + $own_attachment = ($auth->acl_get('a_attach') || $attachment['poster_id'] == $user->data['user_id']) ? true : false; + + if (!$own_attachment || ($attachment['in_message'] && !$auth->acl_get('u_pm_download')) || (!$attachment['in_message'] && !$auth->acl_get('u_download'))) + { + send_status_line(404, 'Not Found'); + trigger_error('ERROR_NO_ATTACHMENT'); + } + + // Obtain all extensions... + $extensions = $cache->obtain_attach_extensions(true); } - - // Obtain all extensions... - $extensions = $cache->obtain_attach_extensions(true); -} -else -{ - if ($attachments || ($attachment && !$attachment['in_message'])) + else { - if ($download_id || $post_id) + if (!$attachment['in_message']) { $sql = 'SELECT p.forum_id, f.forum_password, f.parent_id FROM ' . POSTS_TABLE . ' p, ' . FORUMS_TABLE . ' f - WHERE p.post_id = ' . (($attachment) ? $attachment['post_msg_id'] : $post_id) . ' - AND p.forum_id = f.forum_id'; - } + WHERE p.post_id = ' . $attachment['post_msg_id'] . ' + AND p.forum_id = f.forum_id'; + $result = $db->sql_query_limit($sql, 1); + $row = $db->sql_fetchrow($result); + $db->sql_freeresult($result); - if ($topic_id) - { - $sql = 'SELECT t.forum_id, f.forum_password, f.parent_id - FROM ' . TOPICS_TABLE . ' t, ' . FORUMS_TABLE . " f - WHERE t.topic_id = $topic_id - AND t.forum_id = f.forum_id"; - } - - $result = $db->sql_query_limit($sql, 1); - $row = $db->sql_fetchrow($result); - $db->sql_freeresult($result); + $f_download = $auth->acl_get('f_download', $row['forum_id']); - $f_download = $auth->acl_get('f_download', $row['forum_id']); - - if ($auth->acl_get('u_download') && $f_download) - { - if ($row && $row['forum_password']) + if ($auth->acl_get('u_download') && $f_download) { - // Do something else ... ? - login_forum_box($row); + if ($row && $row['forum_password']) + { + // Do something else ... ? + login_forum_box($row); + } + } + else + { + send_status_line(403, 'Forbidden'); + trigger_error('SORRY_AUTH_VIEW_ATTACH'); } } else { - send_status_line(403, 'Forbidden'); - trigger_error('SORRY_AUTH_VIEW_ATTACH'); - } - } - else - { - $row['forum_id'] = false; - if (!$auth->acl_get('u_pm_download')) - { - send_status_line(403, 'Forbidden'); - trigger_error('SORRY_AUTH_VIEW_ATTACH'); - } - - // Check if the attachment is within the users scope... - $sql = 'SELECT user_id, author_id - FROM ' . PRIVMSGS_TO_TABLE . ' - WHERE msg_id = ' . $attachment['post_msg_id']; - $result = $db->sql_query($sql); - - $allowed = false; - while ($user_row = $db->sql_fetchrow($result)) - { - if ($user->data['user_id'] == $user_row['user_id'] || $user->data['user_id'] == $user_row['author_id']) + // Attachment is in a private message. + $row['forum_id'] = false; + if (!$auth->acl_get('u_pm_download')) { - $allowed = true; - break; + send_status_line(403, 'Forbidden'); + trigger_error('SORRY_AUTH_VIEW_ATTACH'); + } + + // Check if the attachment is within the users scope... + $sql = 'SELECT user_id, author_id + FROM ' . PRIVMSGS_TO_TABLE . ' + WHERE msg_id = ' . $attachment['post_msg_id']; + $result = $db->sql_query($sql); + + $allowed = false; + while ($user_row = $db->sql_fetchrow($result)) + { + if ($user->data['user_id'] == $user_row['user_id'] || $user->data['user_id'] == $user_row['author_id']) + { + $allowed = true; + break; + } + } + $db->sql_freeresult($result); + + if (!$allowed) + { + send_status_line(403, 'Forbidden'); + trigger_error('ERROR_NO_ATTACHMENT'); } } - $db->sql_freeresult($result); - if (!$allowed) + // disallowed? + $extensions = $cache->obtain_attach_extensions($row['forum_id']); + if ($attachment) { - send_status_line(403, 'Forbidden'); - trigger_error('ERROR_NO_ATTACHMENT'); + $ary = array($attachment); + } + else + { + $ary = &$attachments; + } + + if (!phpbb_check_attach_extensions($extensions, $ary)) + { + send_status_line(404, 'Forbidden'); + trigger_error(sprintf($user->lang['EXTENSION_DISABLED_AFTER_POSTING'], $attachment['extension'])); } } - - // disallowed? - $extensions = $cache->obtain_attach_extensions($row['forum_id']); - if ($attachment) +} +else +{ + // sizeof($attachments) > 1 + if ($post_id) { - $ary = array($attachment); + $sql = 'SELECT p.forum_id, f.forum_password, f.parent_id + FROM ' . POSTS_TABLE . ' p, ' . FORUMS_TABLE . ' f + WHERE p.post_id = ' . (($attachment) ? $attachment['post_msg_id'] : $post_id) . ' + AND p.forum_id = f.forum_id'; + } + else if ($topic_id) + { + $sql = 'SELECT t.forum_id, f.forum_password, f.parent_id + FROM ' . TOPICS_TABLE . ' t, ' . FORUMS_TABLE . " f + WHERE t.topic_id = $topic_id + AND t.forum_id = f.forum_id"; + } + + $result = $db->sql_query_limit($sql, 1); + $row = $db->sql_fetchrow($result); + $db->sql_freeresult($result); + + $f_download = $auth->acl_get('f_download', $row['forum_id']); + + if ($auth->acl_get('u_download') && $f_download) + { + if ($row && $row['forum_password']) + { + // Do something else ... ? + login_forum_box($row); + } } else { - $ary = &$attachments; - } - - if (!phpbb_check_attach_extensions($extensions, $ary)) - { - send_status_line(404, 'Forbidden'); - $ext = ($attachment) ? $attachment['extension'] : $attachments[0]['extension']; - trigger_error(sprintf($user->lang['EXTENSION_DISABLED_AFTER_POSTING'], $ext)); + send_status_line(403, 'Forbidden'); + trigger_error('SORRY_AUTH_VIEW_ATTACH'); } } From b6d4ee4244602183f99d57ee154757cd68c7eb75 Mon Sep 17 00:00:00 2001 From: Andreas Fischer Date: Tue, 7 Aug 2012 02:37:08 +0200 Subject: [PATCH 10/17] [feature/attach-dl] Move !download_allowed() check up. PHPBB3-11042 --- phpBB/download/file.php | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/phpBB/download/file.php b/phpBB/download/file.php index 67f315dd2f..23e8327a22 100644 --- a/phpBB/download/file.php +++ b/phpBB/download/file.php @@ -191,6 +191,11 @@ if (empty($attachments)) send_status_line(404, 'Not Found'); trigger_error('ERROR_NO_ATTACHMENT'); } +else if (!download_allowed()) +{ + send_status_line(403, 'Forbidden'); + trigger_error($user->lang['LINKAGE_FORBIDDEN']); +} else if ($download_id) { // sizeof($attachments) == 1 @@ -335,12 +340,6 @@ else } } -if (!download_allowed()) -{ - send_status_line(403, 'Forbidden'); - trigger_error($user->lang['LINKAGE_FORBIDDEN']); -} - if ($attachments && sizeof($attachments) < 2) { $attachments = false; From bba348d68a3114c3c6cad6f1d92855084ce8fa64 Mon Sep 17 00:00:00 2001 From: Andreas Fischer Date: Tue, 7 Aug 2012 02:47:18 +0200 Subject: [PATCH 11/17] [feature/attach-dl] phpbb_check_attach_extensions: Get rid of pass-by-reference PHPBB3-11042 --- phpBB/download/file.php | 11 ++--------- phpBB/includes/functions_download.php | 17 ++++++++--------- 2 files changed, 10 insertions(+), 18 deletions(-) diff --git a/phpBB/download/file.php b/phpBB/download/file.php index 23e8327a22..7044400f90 100644 --- a/phpBB/download/file.php +++ b/phpBB/download/file.php @@ -285,16 +285,9 @@ else if ($download_id) // disallowed? $extensions = $cache->obtain_attach_extensions($row['forum_id']); - if ($attachment) - { - $ary = array($attachment); - } - else - { - $ary = &$attachments; - } - if (!phpbb_check_attach_extensions($extensions, $ary)) + $attachments_filtered = phpbb_filter_disallowed_extensions($extensions, array($attachment)); + if (empty($attachments_filtered)) { send_status_line(404, 'Forbidden'); trigger_error(sprintf($user->lang['EXTENSION_DISABLED_AFTER_POSTING'], $attachment['extension'])); diff --git a/phpBB/includes/functions_download.php b/phpBB/includes/functions_download.php index 7d21147ab5..74c8be5f7b 100644 --- a/phpBB/includes/functions_download.php +++ b/phpBB/includes/functions_download.php @@ -618,21 +618,20 @@ function phpbb_increment_downloads($db, $ids) * Checks every attachment to see if it has an allowed extension * * @param array $extensions As generated by phpbb_cache_service::obtain_attach_extensions -* @param array &$attachments An array of attachments to check +* @param array $attachments An array of attachment row to check * -* @return bool Whether any of the attachments had allowed extensions +* @return array Array of attachment rows with allowed extension */ -function phpbb_check_attach_extensions($extensions, &$attachments) +function phpbb_filter_disallowed_extensions($extensions, $attachments) { - $new_ary = array(); - foreach ($attachments as $attach) + $result = array(); + foreach ($attachments as $row) { - if (isset($extensions['_allowed_'][$attach['extension']])) + if (isset($extensions['_allowed_'][$row['extension']])) { - $new_ary[] = $attach; + $result[] = $row; } } - $attachments = $new_ary; - return !empty($attachments); + return $result; } From 89c102a744ba9ea3eafefd47fd375342bcf6bbe4 Mon Sep 17 00:00:00 2001 From: Andreas Fischer Date: Tue, 7 Aug 2012 02:48:49 +0200 Subject: [PATCH 12/17] [feature/attach-dl] phpbb_filter_disallowed_extensions: Preserve array keys. PHPBB3-11042 --- phpBB/includes/functions_download.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/phpBB/includes/functions_download.php b/phpBB/includes/functions_download.php index 74c8be5f7b..8453469e83 100644 --- a/phpBB/includes/functions_download.php +++ b/phpBB/includes/functions_download.php @@ -625,11 +625,11 @@ function phpbb_increment_downloads($db, $ids) function phpbb_filter_disallowed_extensions($extensions, $attachments) { $result = array(); - foreach ($attachments as $row) + foreach ($attachments as $key => $row) { if (isset($extensions['_allowed_'][$row['extension']])) { - $result[] = $row; + $result[$key] = $row; } } From 8a8f48528ad9d314d2a1f289951d3c931ee672b8 Mon Sep 17 00:00:00 2001 From: Andreas Fischer Date: Tue, 7 Aug 2012 13:40:53 +0200 Subject: [PATCH 13/17] [feature/attach-dl] Correct comment for post_id and topic_id case. PHPBB3-11042 --- phpBB/download/file.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/phpBB/download/file.php b/phpBB/download/file.php index 7044400f90..11e61972b5 100644 --- a/phpBB/download/file.php +++ b/phpBB/download/file.php @@ -296,7 +296,7 @@ else if ($download_id) } else { - // sizeof($attachments) > 1 + // sizeof($attachments) >= 1 if ($post_id) { $sql = 'SELECT p.forum_id, f.forum_password, f.parent_id From d6e8fbf94ad0f2a8ef5513dc8db4660a7b490027 Mon Sep 17 00:00:00 2001 From: Andreas Fischer Date: Tue, 7 Aug 2012 13:43:31 +0200 Subject: [PATCH 14/17] [feature/attach-dl] Remove extra loop over attachments to get primary keys. PHPBB3-11042 --- phpBB/download/file.php | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/phpBB/download/file.php b/phpBB/download/file.php index 11e61972b5..c300df0235 100644 --- a/phpBB/download/file.php +++ b/phpBB/download/file.php @@ -151,6 +151,7 @@ if (!$config['allow_attachments'] && !$config['allow_pm_attach']) trigger_error('ATTACHMENT_FUNCTIONALITY_DISABLED'); } +$attachment_ids = array(); if ($download_id || $post_id || $topic_id) { $sql = 'SELECT a.attach_id, a.in_message, a.post_msg_id, a.extension, a.is_orphan, a.poster_id, a.filetime @@ -177,7 +178,13 @@ if ($download_id || $post_id || $topic_id) } $result = $db->sql_query($sql); - $attachments = $db->sql_fetchrowset($result); + while ($row = $db->sql_fetchrow($result)) + { + $attachment_id = (int) $row['attach_id']; + + $attachment_ids[$attachment_id] = $attachment_id; + $attachments[$attachment_id] = $row; + } $db->sql_freeresult($result); } else @@ -357,15 +364,9 @@ if ($attachment) if ($attachments) { - $attach_ids = array(); - foreach ($attachments as $attach) - { - $attach_ids[] = $attach['attach_id']; - } - $sql = 'SELECT attach_id, is_orphan, in_message, post_msg_id, extension, physical_filename, real_filename, mimetype, filesize, filetime FROM ' . ATTACHMENTS_TABLE . ' - WHERE ' . $db->sql_in_set('attach_id', $attach_ids); + WHERE ' . $db->sql_in_set('attach_id', $attachment_ids); $result = $db->sql_query($sql); $attachments = $db->sql_fetchrowset($result); @@ -434,7 +435,7 @@ if ($attachment) if ($attachments) { require_once $phpbb_root_path . 'includes/functions_compress.' . $phpEx; - phpbb_increment_downloads($db, $attach_ids); + phpbb_increment_downloads($db, $attachment_ids); if (!in_array($archive, compress::methods())) { From 4b06a220af23dd8888a9e7501348170746663458 Mon Sep 17 00:00:00 2001 From: Andreas Fischer Date: Tue, 7 Aug 2012 13:51:07 +0200 Subject: [PATCH 15/17] [feature/attach-dl] Use extension_allowed() again. PHPBB3-11042 --- phpBB/download/file.php | 7 ++----- phpBB/includes/functions_download.php | 22 ---------------------- 2 files changed, 2 insertions(+), 27 deletions(-) diff --git a/phpBB/download/file.php b/phpBB/download/file.php index c300df0235..b1a376155d 100644 --- a/phpBB/download/file.php +++ b/phpBB/download/file.php @@ -290,11 +290,8 @@ else if ($download_id) } } - // disallowed? - $extensions = $cache->obtain_attach_extensions($row['forum_id']); - - $attachments_filtered = phpbb_filter_disallowed_extensions($extensions, array($attachment)); - if (empty($attachments_filtered)) + $extensions = array(); + if (!extension_allowed($row['forum_id'], $attachment['extension'], $extensions)) { send_status_line(404, 'Forbidden'); trigger_error(sprintf($user->lang['EXTENSION_DISABLED_AFTER_POSTING'], $attachment['extension'])); diff --git a/phpBB/includes/functions_download.php b/phpBB/includes/functions_download.php index 8453469e83..b01712357d 100644 --- a/phpBB/includes/functions_download.php +++ b/phpBB/includes/functions_download.php @@ -613,25 +613,3 @@ function phpbb_increment_downloads($db, $ids) WHERE ' . $db->sql_in_set('attach_id', $ids); $db->sql_query($sql); } - -/** -* Checks every attachment to see if it has an allowed extension -* -* @param array $extensions As generated by phpbb_cache_service::obtain_attach_extensions -* @param array $attachments An array of attachment row to check -* -* @return array Array of attachment rows with allowed extension -*/ -function phpbb_filter_disallowed_extensions($extensions, $attachments) -{ - $result = array(); - foreach ($attachments as $key => $row) - { - if (isset($extensions['_allowed_'][$row['extension']])) - { - $result[$key] = $row; - } - } - - return $result; -} From 3de4a7e78d2420bf14675ba4c8f66d15f191885f Mon Sep 17 00:00:00 2001 From: Andreas Fischer Date: Tue, 7 Aug 2012 16:55:31 +0200 Subject: [PATCH 16/17] [feature/attach-dl] Also initialise $attachments as an array. PHPBB3-11042 --- phpBB/download/file.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/phpBB/download/file.php b/phpBB/download/file.php index b1a376155d..7e04fa2551 100644 --- a/phpBB/download/file.php +++ b/phpBB/download/file.php @@ -151,7 +151,7 @@ if (!$config['allow_attachments'] && !$config['allow_pm_attach']) trigger_error('ATTACHMENT_FUNCTIONALITY_DISABLED'); } -$attachment_ids = array(); +$attachments = $attachment_ids = array(); if ($download_id || $post_id || $topic_id) { $sql = 'SELECT a.attach_id, a.in_message, a.post_msg_id, a.extension, a.is_orphan, a.poster_id, a.filetime From 8d914e05ae8cb98fcf70f48651728f41ba7da7fa Mon Sep 17 00:00:00 2001 From: Andreas Fischer Date: Tue, 7 Aug 2012 17:03:46 +0200 Subject: [PATCH 17/17] [feature/attach-dl] Get rid of unnecessary if block. Refactor switch block. PHPBB3-11042 --- phpBB/download/file.php | 62 +++++++++++++++++++---------------------- 1 file changed, 28 insertions(+), 34 deletions(-) diff --git a/phpBB/download/file.php b/phpBB/download/file.php index 7e04fa2551..f9c611fa96 100644 --- a/phpBB/download/file.php +++ b/phpBB/download/file.php @@ -151,41 +151,20 @@ if (!$config['allow_attachments'] && !$config['allow_pm_attach']) trigger_error('ATTACHMENT_FUNCTIONALITY_DISABLED'); } -$attachments = $attachment_ids = array(); -if ($download_id || $post_id || $topic_id) +if ($download_id) { - $sql = 'SELECT a.attach_id, a.in_message, a.post_msg_id, a.extension, a.is_orphan, a.poster_id, a.filetime - FROM ' . ATTACHMENTS_TABLE . ' a - WHERE '; - - switch (true) - { - default: - case $download_id: - // Attachment id (only 1 attachment) - $sql .= "a.attach_id = $download_id"; - break; - - case $post_id: - // Post id or private message id (multiple attachments) - $sql .= "a.post_msg_id = $post_id"; - break; - - case $topic_id: - // Topic id (multiple attachments) - $sql .= "a.topic_id = $topic_id"; - break; - } - - $result = $db->sql_query($sql); - while ($row = $db->sql_fetchrow($result)) - { - $attachment_id = (int) $row['attach_id']; - - $attachment_ids[$attachment_id] = $attachment_id; - $attachments[$attachment_id] = $row; - } - $db->sql_freeresult($result); + // Attachment id (only 1 attachment) + $sql_where = "a.attach_id = $download_id"; +} +else if ($post_id) +{ + // Post id or private message id (multiple attachments) + $sql_where = "a.post_msg_id = $post_id"; +} +else if ($topic_id) +{ + // Topic id (multiple attachments) + $sql_where = "a.topic_id = $topic_id"; } else { @@ -193,6 +172,21 @@ else trigger_error('NO_ATTACHMENT_SELECTED'); } +$sql = 'SELECT a.attach_id, a.in_message, a.post_msg_id, a.extension, a.is_orphan, a.poster_id, a.filetime + FROM ' . ATTACHMENTS_TABLE . " a + WHERE $sql_where"; +$result = $db->sql_query($sql); + +$attachments = $attachment_ids = array(); +while ($row = $db->sql_fetchrow($result)) +{ + $attachment_id = (int) $row['attach_id']; + + $attachment_ids[$attachment_id] = $attachment_id; + $attachments[$attachment_id] = $row; +} +$db->sql_freeresult($result); + if (empty($attachments)) { send_status_line(404, 'Not Found');