From 74423eaf126ea719df3ed102e459edad6be864da Mon Sep 17 00:00:00 2001 From: Fyorl Date: Thu, 9 Aug 2012 12:31:49 +0100 Subject: [PATCH 01/12] [ticket/10939] Added $_FILES handling to phpbb_request PHPBB3-10939 --- phpBB/includes/request/interface.php | 1 + phpBB/includes/request/request.php | 13 +++++++++++++ 2 files changed, 14 insertions(+) diff --git a/phpBB/includes/request/interface.php b/phpBB/includes/request/interface.php index afd53002e3..741db35917 100644 --- a/phpBB/includes/request/interface.php +++ b/phpBB/includes/request/interface.php @@ -30,6 +30,7 @@ interface phpbb_request_interface const REQUEST = 2; const COOKIE = 3; const SERVER = 4; + const FILES = 5; /**#@-*/ /** diff --git a/phpBB/includes/request/request.php b/phpBB/includes/request/request.php index 4e425dbd27..4fc9e67314 100644 --- a/phpBB/includes/request/request.php +++ b/phpBB/includes/request/request.php @@ -34,6 +34,7 @@ class phpbb_request implements phpbb_request_interface phpbb_request_interface::REQUEST => '_REQUEST', phpbb_request_interface::COOKIE => '_COOKIE', phpbb_request_interface::SERVER => '_SERVER', + phpbb_request_interface::FILES => '_FILES', ); /** @@ -283,6 +284,18 @@ class phpbb_request implements phpbb_request_interface return $this->server($var_name, $default); } + /** + * Shortcut method to retrieve $_FILES variables + * + * @param string $form_name The name of the file input form element + * + * @return array The uploaded file's information + */ + public function file($form_name) + { + return $this->variable($form_name, array(), false, phpbb_request_interface::FILES); + } + /** * Checks whether a certain variable was sent via POST. * To make sure that a request was sent using POST you should call this function From 48a0810ea593fc3ed84762ccd9076ea756cda41c Mon Sep 17 00:00:00 2001 From: Fyorl Date: Wed, 15 Aug 2012 14:20:40 +0100 Subject: [PATCH 02/12] [ticket/10939] Modified request test slightly to include $_FILES PHPBB3-10939 --- tests/request/request_test.php | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/request/request_test.php b/tests/request/request_test.php index bca5125b7a..c02fcf829f 100644 --- a/tests/request/request_test.php +++ b/tests/request/request_test.php @@ -21,6 +21,13 @@ class phpbb_request_test extends phpbb_test_case $_COOKIE['test'] = 3; $_REQUEST['test'] = 3; $_GET['unset'] = ''; + $_FILES['test'] = array( + 'name' => 'file', + 'tmp_name' => 'tmp', + 'size' => 256, + 'type' => 'application/octet-stream', + 'error' => UPLOAD_ERR_OK, + ); $_SERVER['HTTP_HOST'] = 'example.com'; $_SERVER['HTTP_ACCEPT'] = 'application/json'; @@ -42,6 +49,7 @@ class phpbb_request_test extends phpbb_test_case $this->assertEquals(2, $_GET['test'], 'Checking $_GET after enable_super_globals'); $this->assertEquals(3, $_COOKIE['test'], 'Checking $_COOKIE after enable_super_globals'); $this->assertEquals(3, $_REQUEST['test'], 'Checking $_REQUEST after enable_super_globals'); + $this->assertEquals(256, $_FILES['test']['size']); $_POST['x'] = 2; $this->assertEquals($_POST, $GLOBALS['_POST'], 'Checking whether $_POST can still be accessed via $GLOBALS[\'_POST\']'); From 91b9cc90dd078fda135d975f0e5af798535d9014 Mon Sep 17 00:00:00 2001 From: Fyorl Date: Wed, 15 Aug 2012 15:00:03 +0100 Subject: [PATCH 03/12] [ticket/10939] Modified functions_upload to not use $_FILES PHPBB3-10939 --- phpBB/includes/functions_upload.php | 45 ++++++++++++++++------------- 1 file changed, 25 insertions(+), 20 deletions(-) diff --git a/phpBB/includes/functions_upload.php b/phpBB/includes/functions_upload.php index d4c6b42cf4..b467aa93d1 100644 --- a/phpBB/includes/functions_upload.php +++ b/phpBB/includes/functions_upload.php @@ -566,10 +566,11 @@ class fileupload */ function form_upload($form_name) { - global $user; + global $user, $request; - unset($_FILES[$form_name]['local_mode']); - $file = new filespec($_FILES[$form_name], $this); + $upload = $request->file($form_name); + unset($upload['local_mode']); + $file = new filespec($upload, $this); if ($file->init_error) { @@ -578,9 +579,9 @@ class fileupload } // Error array filled? - if (isset($_FILES[$form_name]['error'])) + if (isset($upload['error'])) { - $error = $this->assign_internal_error($_FILES[$form_name]['error']); + $error = $this->assign_internal_error($upload['error']); if ($error !== false) { @@ -590,7 +591,7 @@ class fileupload } // Check if empty file got uploaded (not catched by is_uploaded_file) - if (isset($_FILES[$form_name]['size']) && $_FILES[$form_name]['size'] == 0) + if (isset($upload['size']) && $upload['size'] == 0) { $file->error[] = $user->lang[$this->error_prefix . 'EMPTY_FILEUPLOAD']; return $file; @@ -631,17 +632,17 @@ class fileupload */ function local_upload($source_file, $filedata = false) { - global $user; + global $user, $request; - $form_name = 'local'; + $upload = array(); - $_FILES[$form_name]['local_mode'] = true; - $_FILES[$form_name]['tmp_name'] = $source_file; + $upload['local_mode'] = true; + $upload['tmp_name'] = $source_file; if ($filedata === false) { - $_FILES[$form_name]['name'] = utf8_basename($source_file); - $_FILES[$form_name]['size'] = 0; + $upload['name'] = utf8_basename($source_file); + $upload['size'] = 0; $mimetype = ''; if (function_exists('mime_content_type')) @@ -655,16 +656,16 @@ class fileupload $mimetype = 'application/octetstream'; } - $_FILES[$form_name]['type'] = $mimetype; + $upload['type'] = $mimetype; } else { - $_FILES[$form_name]['name'] = $filedata['realname']; - $_FILES[$form_name]['size'] = $filedata['size']; - $_FILES[$form_name]['type'] = $filedata['type']; + $upload['name'] = $filedata['realname']; + $upload['size'] = $filedata['size']; + $upload['type'] = $filedata['type']; } - $file = new filespec($_FILES[$form_name], $this); + $file = new filespec($upload, $this); if ($file->init_error) { @@ -672,9 +673,9 @@ class fileupload return $file; } - if (isset($_FILES[$form_name]['error'])) + if (isset($upload['error'])) { - $error = $this->assign_internal_error($_FILES[$form_name]['error']); + $error = $this->assign_internal_error($upload['error']); if ($error !== false) { @@ -709,6 +710,7 @@ class fileupload } $this->common_checks($file); + $request->overwrite('local', $upload, phpbb_request_interface::FILES); return $file; } @@ -1001,7 +1003,10 @@ class fileupload */ function is_valid($form_name) { - return (isset($_FILES[$form_name]) && $_FILES[$form_name]['name'] != 'none') ? true : false; + global $request; + $upload = $request->file($form_name); + + return (!empty($upload) && $upload['name'] !== 'none'); } From 3eb88b026752512a79b641e3b55193972f221a45 Mon Sep 17 00:00:00 2001 From: Fyorl Date: Wed, 15 Aug 2012 15:06:37 +0100 Subject: [PATCH 04/12] [ticket/10939] Modified message_parser.php to not use $_FILES PHPBB3-10939 --- phpBB/includes/message_parser.php | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/phpBB/includes/message_parser.php b/phpBB/includes/message_parser.php index 6695047b56..1cd2a46fa1 100644 --- a/phpBB/includes/message_parser.php +++ b/phpBB/includes/message_parser.php @@ -1363,13 +1363,14 @@ class parse_message extends bbcode_firstpass */ function parse_attachments($form_name, $mode, $forum_id, $submit, $preview, $refresh, $is_message = false) { - global $config, $auth, $user, $phpbb_root_path, $phpEx, $db; + global $config, $auth, $user, $phpbb_root_path, $phpEx, $db, $request; $error = array(); $num_attachments = sizeof($this->attachment_data); $this->filename_data['filecomment'] = utf8_normalize_nfc(request_var('filecomment', '', true)); - $upload_file = (isset($_FILES[$form_name]) && $_FILES[$form_name]['name'] != 'none' && trim($_FILES[$form_name]['name'])) ? true : false; + $upload = $request->file($form_name); + $upload_file = (!empty($upload) && $upload['name'] !== 'none' && trim($upload['name'])); $add_file = (isset($_POST['add_file'])) ? true : false; $delete_file = (isset($_POST['delete_file'])) ? true : false; From a3b87f1e953e4f79478d8e7eb65ca9855140243a Mon Sep 17 00:00:00 2001 From: Fyorl Date: Wed, 15 Aug 2012 15:07:42 +0100 Subject: [PATCH 05/12] [ticket/10939] Modified functions_user.php to not use $_FILES PHPBB3-10939 --- phpBB/includes/functions_user.php | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/phpBB/includes/functions_user.php b/phpBB/includes/functions_user.php index 6e658b4ef4..405c5ef5d1 100644 --- a/phpBB/includes/functions_user.php +++ b/phpBB/includes/functions_user.php @@ -2059,13 +2059,14 @@ function avatar_remote($data, &$error) */ function avatar_upload($data, &$error) { - global $phpbb_root_path, $config, $db, $user, $phpEx; + global $phpbb_root_path, $config, $db, $user, $phpEx, $request; // Init upload class include_once($phpbb_root_path . 'includes/functions_upload.' . $phpEx); $upload = new fileupload('AVATAR_', array('jpg', 'jpeg', 'gif', 'png'), $config['avatar_filesize'], $config['avatar_min_width'], $config['avatar_min_height'], $config['avatar_max_width'], $config['avatar_max_height'], (isset($config['mime_triggers']) ? explode('|', $config['mime_triggers']) : false)); - if (!empty($_FILES['uploadfile']['name'])) + $uploadfile = $request->file('uploadfile'); + if (!empty($uploadfile['name'])) { $file = $upload->form_upload('uploadfile'); } @@ -2288,7 +2289,7 @@ function avatar_get_dimensions($avatar, $avatar_type, &$error, $current_x = 0, $ */ function avatar_process_user(&$error, $custom_userdata = false, $can_upload = null) { - global $config, $phpbb_root_path, $auth, $user, $db; + global $config, $phpbb_root_path, $auth, $user, $db, $request; $data = array( 'uploadurl' => request_var('uploadurl', ''), @@ -2330,7 +2331,8 @@ function avatar_process_user(&$error, $custom_userdata = false, $can_upload = nu $can_upload = ($config['allow_avatar_upload'] && file_exists($phpbb_root_path . $config['avatar_path']) && phpbb_is_writable($phpbb_root_path . $config['avatar_path']) && $change_avatar && (@ini_get('file_uploads') || strtolower(@ini_get('file_uploads')) == 'on')) ? true : false; } - if ((!empty($_FILES['uploadfile']['name']) || $data['uploadurl']) && $can_upload) + $uploadfile = $request->file('uploadfile'); + if ((!empty($uploadfile['name']) || $data['uploadurl']) && $can_upload) { list($sql_ary['user_avatar_type'], $sql_ary['user_avatar'], $sql_ary['user_avatar_width'], $sql_ary['user_avatar_height']) = avatar_upload($data, $error); } From 83a53eb985d17f203d8ef0d33291c1ad1c4db84c Mon Sep 17 00:00:00 2001 From: Fyorl Date: Wed, 15 Aug 2012 15:11:33 +0100 Subject: [PATCH 06/12] [ticket/10939] Modified ucp_groups.php to not use $_FILES PHPBB3-10939 --- phpBB/includes/ucp/ucp_groups.php | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/phpBB/includes/ucp/ucp_groups.php b/phpBB/includes/ucp/ucp_groups.php index 65ab92e78e..9652986cf2 100644 --- a/phpBB/includes/ucp/ucp_groups.php +++ b/phpBB/includes/ucp/ucp_groups.php @@ -513,7 +513,8 @@ class ucp_groups $data['height'] = request_var('height', ''); $delete = request_var('delete', ''); - if (!empty($_FILES['uploadfile']['tmp_name']) || $data['uploadurl'] || $data['remotelink']) + $uploadfile = $request->file('uploadfile'); + if (!empty($uploadfile['tmp_name']) || $data['uploadurl'] || $data['remotelink']) { // Avatar stuff $var_ary = array( @@ -527,7 +528,7 @@ class ucp_groups { $data['user_id'] = "g$group_id"; - if ((!empty($_FILES['uploadfile']['tmp_name']) || $data['uploadurl']) && $can_upload) + if ((!empty($uploadfile['tmp_name']) || $data['uploadurl']) && $can_upload) { list($submit_ary['avatar_type'], $submit_ary['avatar'], $submit_ary['avatar_width'], $submit_ary['avatar_height']) = avatar_upload($data, $error); } From 94ed66c777d22e16ba73d317889707b90f755878 Mon Sep 17 00:00:00 2001 From: Fyorl Date: Wed, 15 Aug 2012 15:14:52 +0100 Subject: [PATCH 07/12] [ticket/10939] Modified acp_groups.php to not use $_FILES PHPBB3-10939 --- phpBB/includes/acp/acp_groups.php | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/phpBB/includes/acp/acp_groups.php b/phpBB/includes/acp/acp_groups.php index f88fa76df1..b018faf968 100644 --- a/phpBB/includes/acp/acp_groups.php +++ b/phpBB/includes/acp/acp_groups.php @@ -26,6 +26,7 @@ class acp_groups { global $config, $db, $user, $auth, $template, $cache; global $phpbb_root_path, $phpbb_admin_path, $phpEx, $table_prefix, $file_uploads; + global $request; $user->add_lang('acp/groups'); $this->tpl_name = 'acp_groups'; @@ -323,7 +324,8 @@ class acp_groups $submit_ary['founder_manage'] = isset($_REQUEST['group_founder_manage']) ? 1 : 0; } - if (!empty($_FILES['uploadfile']['tmp_name']) || $data['uploadurl'] || $data['remotelink']) + $uploadfile = $request->file('uploadfile'); + if (!empty($uploadfile['tmp_name']) || $data['uploadurl'] || $data['remotelink']) { // Avatar stuff $var_ary = array( @@ -337,7 +339,7 @@ class acp_groups { $data['user_id'] = "g$group_id"; - if ((!empty($_FILES['uploadfile']['tmp_name']) || $data['uploadurl']) && $can_upload) + if ((!empty($uploadfile['tmp_name']) || $data['uploadurl']) && $can_upload) { list($submit_ary['avatar_type'], $submit_ary['avatar'], $submit_ary['avatar_width'], $submit_ary['avatar_height']) = avatar_upload($data, $error); } From 348554cc297bf906a0510894986bb60041a5db9a Mon Sep 17 00:00:00 2001 From: Fyorl Date: Wed, 15 Aug 2012 15:36:41 +0100 Subject: [PATCH 08/12] [ticket/10939] Modified mock request class to handle deactivated $_FILES PHPBB3-10939 --- tests/mock/request.php | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/tests/mock/request.php b/tests/mock/request.php index 946dfdada9..2a272fc03b 100644 --- a/tests/mock/request.php +++ b/tests/mock/request.php @@ -11,13 +11,14 @@ class phpbb_mock_request implements phpbb_request_interface { protected $data; - public function __construct($get = array(), $post = array(), $cookie = array(), $server = array(), $request = false) + public function __construct($get = array(), $post = array(), $cookie = array(), $server = array(), $request = false, $files = array()) { $this->data[phpbb_request_interface::GET] = $get; $this->data[phpbb_request_interface::POST] = $post; $this->data[phpbb_request_interface::COOKIE] = $cookie; $this->data[phpbb_request_interface::REQUEST] = ($request === false) ? $post + $get : $request; $this->data[phpbb_request_interface::SERVER] = $server; + $this->data[phpbb_request_interface::FILES] = $files; } public function overwrite($var_name, $value, $super_global = phpbb_request_interface::REQUEST) @@ -42,6 +43,12 @@ class phpbb_mock_request implements phpbb_request_interface return $this->server($var_name, $default); } + public function file($form_name) + { + $super_global = phpbb_request_interface::FILES; + return isset($this->data[$super_global][$form_name]) ? $this->data[$super_global][$form_name] : array(); + } + public function is_set_post($name) { return $this->is_set($name, phpbb_request_interface::POST); From df86f466e0245669e5cf97a162378fb653c72422 Mon Sep 17 00:00:00 2001 From: Fyorl Date: Wed, 15 Aug 2012 15:56:43 +0100 Subject: [PATCH 09/12] [ticket/10939] Modified fileupload tests to deal with new behaviour PHPBB3-10939 --- tests/upload/fileupload_test.php | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/upload/fileupload_test.php b/tests/upload/fileupload_test.php index 076855ab56..1665c493be 100644 --- a/tests/upload/fileupload_test.php +++ b/tests/upload/fileupload_test.php @@ -19,7 +19,8 @@ class phpbb_fileupload_test extends phpbb_test_case { // Global $config required by unique_id // Global $user required by several functions dealing with translations - global $config, $user; + // Global $request required by form_upload, local_upload and is_valid + global $config, $user, $request; if (!is_array($config)) { @@ -31,6 +32,9 @@ class phpbb_fileupload_test extends phpbb_test_case $user = new phpbb_mock_user(); $user->lang = new phpbb_mock_lang(); + + $request = new phpbb_mock_request(); + $this->path = __DIR__ . '/fixture/'; } From 935e717762ccf9cad47058157933b4d372a57320 Mon Sep 17 00:00:00 2001 From: Fyorl Date: Wed, 15 Aug 2012 17:03:15 +0100 Subject: [PATCH 10/12] [ticket/10939] Modified the default return for $request->file PHPBB3-10939 --- phpBB/includes/request/request.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/phpBB/includes/request/request.php b/phpBB/includes/request/request.php index 4fc9e67314..25410c27c4 100644 --- a/phpBB/includes/request/request.php +++ b/phpBB/includes/request/request.php @@ -293,7 +293,7 @@ class phpbb_request implements phpbb_request_interface */ public function file($form_name) { - return $this->variable($form_name, array(), false, phpbb_request_interface::FILES); + return $this->variable($form_name, array('name' => 'none'), false, phpbb_request_interface::FILES); } /** From aa5f6dffa5ad7e8c357006ba539885bf9814d8f5 Mon Sep 17 00:00:00 2001 From: Fyorl Date: Mon, 20 Aug 2012 21:40:53 +0100 Subject: [PATCH 11/12] [ticket/10939] Added tests for phpbb_request::file PHPBB3-10939 --- tests/request/request_test.php | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/tests/request/request_test.php b/tests/request/request_test.php index c02fcf829f..52c21abd2a 100644 --- a/tests/request/request_test.php +++ b/tests/request/request_test.php @@ -93,6 +93,23 @@ class phpbb_request_test extends phpbb_test_case $this->request->header('SOMEVAR'); } + public function test_file() + { + $file = $this->request->file('test'); + $this->assertEquals('file', $file['name']); + $this->assertEquals('tmp', $file['tmp_name']); + $this->assertEquals(256, $file['size']); + $this->assertEquals('application/octet-stream', $file['type']); + $this->assertEquals(UPLOAD_ERR_OK, $file['error']); + } + + public function test_file_not_exists() + { + $file = $this->request->file('404'); + $this->assertTrue(is_array($file)); + $this->assertTrue(empty($file)); + } + /** * Checks that directly accessing $_POST will trigger * an error. From 1f9bff2126bbec514c4a7b675723bfe7f26c432e Mon Sep 17 00:00:00 2001 From: Fyorl Date: Sat, 10 Nov 2012 08:51:12 +0000 Subject: [PATCH 12/12] [ticket/10939] Added documentation for phpbb_request::file PHPBB3-10939 --- phpBB/includes/request/request.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/phpBB/includes/request/request.php b/phpBB/includes/request/request.php index 25410c27c4..94445f232c 100644 --- a/phpBB/includes/request/request.php +++ b/phpBB/includes/request/request.php @@ -289,7 +289,8 @@ class phpbb_request implements phpbb_request_interface * * @param string $form_name The name of the file input form element * - * @return array The uploaded file's information + * @return array The uploaded file's information or an empty array if the + * variable does not exist in _FILES. */ public function file($form_name) {