From 82ca2c8b08ca8c21b358ee8c862c639a5944977d Mon Sep 17 00:00:00 2001 From: Fyorl Date: Sat, 30 Jun 2012 10:41:54 +0100 Subject: [PATCH] [ticket/10941] Minor adjustments as per PR comments Added some comments clarifying globals and lowercased fixture filenames PHPBB3-10941 --- tests/functional/fileupload_test.php | 8 +++ tests/uploads/filespec_test.php | 100 +++++++++++++++------------ tests/uploads/fileupload_test.php | 20 +++++- tests/uploads/fixture/{GIF => gif} | Bin tests/uploads/fixture/{JPG => jpg} | Bin tests/uploads/fixture/{PNG => png} | Bin tests/uploads/fixture/{TIF => tif} | Bin tests/uploads/fixture/{TXT => txt} | 0 8 files changed, 81 insertions(+), 47 deletions(-) rename tests/uploads/fixture/{GIF => gif} (100%) rename tests/uploads/fixture/{JPG => jpg} (100%) rename tests/uploads/fixture/{PNG => png} (100%) rename tests/uploads/fixture/{TIF => tif} (100%) rename tests/uploads/fixture/{TXT => txt} (100%) diff --git a/tests/functional/fileupload_test.php b/tests/functional/fileupload_test.php index c06f7d94d7..bbcc0c37c9 100644 --- a/tests/functional/fileupload_test.php +++ b/tests/functional/fileupload_test.php @@ -51,8 +51,14 @@ class phpbb_functional_fileupload_test extends phpbb_functional_test_case public function test_remote_upload() { + // Note: we cannot check for the actual value of the error messages + // since they are passed through the translator which will result in + // blank strings within this test framework. + // Only doing this within the functional framework because we need a // URL + + // Global $config required by unique_id global $config; if (!is_array($config)) @@ -83,5 +89,7 @@ class phpbb_functional_fileupload_test extends phpbb_functional_test_case $file = $upload->remote_upload($this->root_url . 'styles/prosilver/theme/images/forum_read.gif'); $this->assertEquals(0, sizeof($file->error)); $this->assertTrue(file_exists($file->filename)); + + $config = array(); } } diff --git a/tests/uploads/filespec_test.php b/tests/uploads/filespec_test.php index 97ccb8db61..3c9eda4468 100644 --- a/tests/uploads/filespec_test.php +++ b/tests/uploads/filespec_test.php @@ -26,6 +26,7 @@ class phpbb_filespec_test extends phpbb_test_case protected function setUp() { + // Global $config required by unique_id global $config; if (!is_array($config)) @@ -37,7 +38,7 @@ class phpbb_filespec_test extends phpbb_test_case $config['rand_seed_last_update'] = time() + 600; $config['mime_triggers'] = 'body|head|html|img|plaintext|a href|pre|script|table|title'; - $this->config = $config; + $this->config = &$config; $this->path = __DIR__ . '/fixture/'; $this->init_filespec(); @@ -51,7 +52,7 @@ class phpbb_filespec_test extends phpbb_test_case } copy($fileinfo->getPathname(), $this->path . $fileinfo->getFilename() . '_copy'); - if ($fileinfo->getFilename() === 'TXT') + if ($fileinfo->getFilename() === 'txt') { copy($fileinfo->getPathname(), $this->path . $fileinfo->getFilename() . '_copy_2'); } @@ -61,22 +62,34 @@ class phpbb_filespec_test extends phpbb_test_case public function additional_checks_variables() { return array( - array('GIF', true), - array('JPG', false), - array('PNG', true), - array('TIF', false), - array('TXT', true), + array('gif', true), + array('jpg', false), + array('png', true), + array('tif', false), + array('txt', true), ); } public function check_content_variables() { return array( - array('GIF', true), - array('JPG', true), - array('PNG', true), - array('TIF', true), - array('TXT', false), + array('gif', true), + array('jpg', true), + array('png', true), + array('tif', true), + array('txt', false), + ); + } + + public function clean_filename_variables() + { + $chunks = str_split('abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ\'\\" /:*?<>|[];(){},#+=-_`', 8); + return array( + array($chunks[0] . $chunks[7]), + array($chunks[1] . $chunks[8]), + array($chunks[2] . $chunks[9]), + array($chunks[3] . $chunks[4]), + array($chunks[5] . $chunks[6]), ); } @@ -107,23 +120,23 @@ class phpbb_filespec_test extends phpbb_test_case public function is_image_variables() { return array( - array('GIF', 'image/gif', true), - array('JPG', 'image/jpg', true), - array('PNG', 'image/png', true), - array('TIF', 'image/tif', true), - array('TXT', 'text/plain', false), + array('gif', 'image/gif', true), + array('jpg', 'image/jpg', true), + array('png', 'image/png', true), + array('tif', 'image/tif', true), + array('txt', 'text/plain', false), ); } public function move_file_variables() { return array( - array('GIF_copy', 'GIF_moved', 'image/gif', 'gif', false, true), + array('gif_copy', 'gif_moved', 'image/gif', 'gif', false, true), array('non_existant', 'still_non_existant', 'text/plain', 'txt', true, false), - array('TXT_copy', 'TXT_as_img', 'image/jpg', 'txt', true, true), - array('TXT_copy_2', 'TXT_moved', 'text/plain', 'txt', false, true), - array('JPG_copy', 'JPG_moved', 'image/png', 'jpg', false, true), - array('PNG_copy', 'PNG_moved', 'image/png', 'jpg', true, true), + array('txt_copy', 'txt_as_img', 'image/jpg', 'txt', true, true), + array('txt_copy_2', 'txt_moved', 'text/plain', 'txt', false, true), + array('jpg_copy', 'jpg_moved', 'image/png', 'jpg', false, true), + array('png_copy', 'png_moved', 'image/png', 'jpg', true, true), ); } @@ -137,6 +150,8 @@ class phpbb_filespec_test extends phpbb_test_case unlink($fileinfo->getPathname()); } } + + $this->config = array(); } /** @@ -144,6 +159,7 @@ class phpbb_filespec_test extends phpbb_test_case */ public function test_additional_checks($filename, $expected) { + // Global $user required by filespec::additional_checks global $user; $user = new phpbb_mock_user(); @@ -154,6 +170,8 @@ class phpbb_filespec_test extends phpbb_test_case $this->filespec->filesize = $this->filespec->get_filesize($this->path . $filename); $this->assertEquals($expected, $this->filespec->additional_checks()); + + $user = null; } /** @@ -166,29 +184,21 @@ class phpbb_filespec_test extends phpbb_test_case $this->assertEquals($expected, $this->filespec->check_content($disallowed_content)); } - public function test_clean_filename_real() + /** + * @dataProvider clean_filename_variables + */ + public function test_clean_filename_real($filename) { - $available_chars = str_split('abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ\'\\" /:*?<>|[];(){},#+=-_`'); + $bad_chars = array("'", "\\", ' ', '/', ':', '*', '?', '"', '<', '>', '|'); - for ($tests = 0; $tests < self::TEST_COUNT; $tests++) + $this->init_filespec(array('name' => $filename)); + $this->filespec->clean_filename('real', self::PREFIX); + $name = $this->filespec->realname; + + $this->assertEquals(0, preg_match('/%(\w{2})/', $name)); + foreach ($bad_chars as $char) { - $len = mt_rand(1, self::MAX_STR_LEN); - $str = ''; - for ($j = 0; $j < $len; $j++) - { - $index = mt_rand(0, sizeof($available_chars) - 1); - $str .= $available_chars[$index]; - } - - $this->init_filespec(array('name' => $str)); - $this->filespec->clean_filename('real', self::PREFIX); - $name = $this->filespec->realname; - - $this->assertEquals(0, preg_match('/%(\w{2})/', $name)); - foreach ($bad_chars as $char) - { - $this->assertFalse(strpos($name, $char)); - } + $this->assertFalse(strpos($name, $char)); } } @@ -230,10 +240,10 @@ class phpbb_filespec_test extends phpbb_test_case */ public function test_move_file($tmp_name, $realname, $mime_type, $extension, $error, $expected) { - global $request, $phpbb_root_path, $phpEx; + // Global $phpbb_root_path and $phpEx are required by phpbb_chmod + global $phpbb_root_path, $phpEx; $phpbb_root_path = ''; $phpEx = 'php'; - $request = new phpbb_mock_request(); $upload = new phpbb_mock_fileupload(); $upload->max_filesize = self::UPLOAD_MAX_FILESIZE; @@ -250,5 +260,7 @@ class phpbb_filespec_test extends phpbb_test_case $this->assertEquals($expected, $this->filespec->move_file($this->path)); $this->assertEquals($error, (bool) sizeof($this->filespec->error)); $this->assertEquals($this->filespec->file_moved, file_exists($this->path . $realname)); + + $phpEx = ''; } } diff --git a/tests/uploads/fileupload_test.php b/tests/uploads/fileupload_test.php index 1fa628cbf9..cb346fd0a2 100644 --- a/tests/uploads/fileupload_test.php +++ b/tests/uploads/fileupload_test.php @@ -18,6 +18,8 @@ class phpbb_fileupload_test extends phpbb_test_case protected function setUp() { + // Global $config required by unique_id + // Global $user required by several functions dealing with translations global $config, $user; if (!is_array($config)) @@ -44,8 +46,20 @@ class phpbb_fileupload_test extends phpbb_test_case return $filespec; } + protected function tearDown() + { + // Clear globals + global $config, $user; + $config = array(); + $user = null; + } + public function test_common_checks() { + // Note: we cannot check for the actual value of the error messages + // since they are passed through the translator which will result in + // blank strings within this test framework. + // Test 1: Valid file $upload = new fileupload('', array('jpg'), 1000); $file = $this->gen_valid_filespec(); @@ -77,10 +91,10 @@ class phpbb_fileupload_test extends phpbb_test_case { $upload = new fileupload('', array('jpg'), 1000); - copy($this->path . 'JPG', $this->path . 'JPG.jpg'); - $file = $upload->local_upload($this->path . 'JPG.jpg'); + copy($this->path . 'jpg', $this->path . 'jpg.jpg'); + $file = $upload->local_upload($this->path . 'jpg.jpg'); $this->assertEquals(0, sizeof($file->error)); - unlink($this->path . 'JPG.jpg'); + unlink($this->path . 'jpg.jpg'); } public function test_valid_dimensions() diff --git a/tests/uploads/fixture/GIF b/tests/uploads/fixture/gif similarity index 100% rename from tests/uploads/fixture/GIF rename to tests/uploads/fixture/gif diff --git a/tests/uploads/fixture/JPG b/tests/uploads/fixture/jpg similarity index 100% rename from tests/uploads/fixture/JPG rename to tests/uploads/fixture/jpg diff --git a/tests/uploads/fixture/PNG b/tests/uploads/fixture/png similarity index 100% rename from tests/uploads/fixture/PNG rename to tests/uploads/fixture/png diff --git a/tests/uploads/fixture/TIF b/tests/uploads/fixture/tif similarity index 100% rename from tests/uploads/fixture/TIF rename to tests/uploads/fixture/tif diff --git a/tests/uploads/fixture/TXT b/tests/uploads/fixture/txt similarity index 100% rename from tests/uploads/fixture/TXT rename to tests/uploads/fixture/txt