From 6f45b467466c963d874bad1a442ce3c26b2e34dd Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Wed, 1 May 2024 11:22:29 +0200 Subject: [PATCH] [ticket/17077] Add proper locking in PHP without releasing form tokens PHPBB3-17077 --- phpBB/phpbb/lock/posting.php | 22 ++----------------- phpBB/posting.php | 3 --- tests/lock/posting_test.php | 14 +++++++----- .../phpbb_functional_test_case.php | 16 ++++++++++++++ .../phpbb_test_case_helpers.php | 1 - 5 files changed, 26 insertions(+), 30 deletions(-) diff --git a/phpBB/phpbb/lock/posting.php b/phpBB/phpbb/lock/posting.php index 1fee36e63f..569cad9971 100644 --- a/phpBB/phpbb/lock/posting.php +++ b/phpBB/phpbb/lock/posting.php @@ -27,9 +27,6 @@ class posting /** @var string */ private $lock_name = ''; - /** @var bool Lock state */ - private $locked = false; - /** * Constructor for posting lock * @@ -67,29 +64,14 @@ class posting { $this->set_lock_name($creation_time, $form_token); - // Lock is held for session, cannot acquire it - if ($this->cache->_exists($this->lock_name)) + // Lock is held for session, cannot acquire it unless special flag for testing is set + if ($this->cache->_exists($this->lock_name) && !$this->config->offsetExists('ci_tests_no_lock_posting')) { return false; } - $this->locked = true; - $this->cache->put($this->lock_name, true, $this->config['flood_interval']); return true; } - - /** - * Release lock - * - * @return void - */ - public function release(): void - { - if ($this->locked) - { - $this->cache->destroy($this->lock_name); - } - } } diff --git a/phpBB/posting.php b/phpBB/posting.php index bea75081c1..f99b0db006 100644 --- a/phpBB/posting.php +++ b/phpBB/posting.php @@ -1565,9 +1565,6 @@ if ($submit || $preview || $refresh) // The last parameter tells submit_post if search indexer has to be run $redirect_url = submit_post($mode, $post_data['post_subject'], $post_author_name, $post_data['topic_type'], $poll, $data, $update_message, ($update_message || $update_subject) ? true : false); - // Release lock after submitting post - $posting_lock->release(); - /** * This event allows you to define errors after the post action is performed * diff --git a/tests/lock/posting_test.php b/tests/lock/posting_test.php index e9a9cdfffb..75716dd2ba 100644 --- a/tests/lock/posting_test.php +++ b/tests/lock/posting_test.php @@ -42,13 +42,15 @@ class phpbb_lock_posting_test extends phpbb_test_case $this->assertFalse($this->lock->acquire(100, 'foo')); $this->assertTrue($this->cache->_exists(sha1('100foo') . '_posting_lock')); - $this->lock->release(); - $this->assertFalse($this->cache->_exists(sha1('100foo') . '_posting_lock')); + $this->assertFalse($this->lock->acquire(100, 'foo')); + $this->cache->put(sha1('100foo') . '_posting_lock', 'foo', -30); $this->assertTrue($this->lock->acquire(100, 'foo')); $this->assertTrue($this->cache->_exists(sha1('100foo') . '_posting_lock')); - $this->lock->release(); - $this->lock->release(); // double release has no effect - $this->assertFalse($this->cache->_exists(sha1('100foo') . '_posting_lock')); + $this->config->offsetSet('ci_tests_no_lock_posting', true); + $this->assertTrue($this->lock->acquire(100, 'foo')); + $this->assertTrue($this->cache->_exists(sha1('100foo') . '_posting_lock')); + // Multiple acquires possible due to special ci test flag + $this->assertTrue($this->lock->acquire(100, 'foo')); } -} \ No newline at end of file +} diff --git a/tests/test_framework/phpbb_functional_test_case.php b/tests/test_framework/phpbb_functional_test_case.php index a90008c22e..9e4cb364d0 100644 --- a/tests/test_framework/phpbb_functional_test_case.php +++ b/tests/test_framework/phpbb_functional_test_case.php @@ -96,6 +96,14 @@ class phpbb_functional_test_case extends phpbb_test_case $db = $this->get_db(); + // Special flag for testing without possibility to run into lock scenario. + // Unset entry and add it back if lock behavior for posting should be tested. + // Unset ci_tests_no_lock_posting from config + $db->sql_return_on_error(true); + $sql = 'INSERT INTO ' . CONFIG_TABLE . " (config_name, config_value) VALUES ('ci_tests_no_lock_posting', '1')"; + $this->db->sql_query($sql); + $db->sql_return_on_error(false); + foreach (static::setup_extensions() as $extension) { $this->purge_cache(); @@ -120,6 +128,11 @@ class phpbb_functional_test_case extends phpbb_test_case if ($this->db instanceof \phpbb\db\driver\driver_interface) { + // Unset ci_tests_no_lock_posting from config + $sql = 'DELETE FROM ' . CONFIG_TABLE . " + WHERE config_name = 'ci_tests_no_lock_posting'"; + $this->db->sql_query($sql); + // Close the database connections again this test $this->db->sql_close(); } @@ -193,6 +206,9 @@ class phpbb_functional_test_case extends phpbb_test_case $this->excludeBackupStaticAttributes($backupStaticAttributesBlacklist); } + /** + * @return \phpbb\db\driver\driver_interface + */ protected function get_db() { global $phpbb_root_path, $phpEx; diff --git a/tests/test_framework/phpbb_test_case_helpers.php b/tests/test_framework/phpbb_test_case_helpers.php index c8cddb4352..d22b1cb8fa 100644 --- a/tests/test_framework/phpbb_test_case_helpers.php +++ b/tests/test_framework/phpbb_test_case_helpers.php @@ -123,7 +123,6 @@ class phpbb_test_case_helpers { $config = array(); - if (extension_loaded('sqlite3')) { $config = array_merge($config, array(