[ticket/17077] Add proper locking in PHP without releasing form tokens

PHPBB3-17077
This commit is contained in:
Marc Alexander 2024-05-01 11:22:29 +02:00
parent 98929ca983
commit 6f45b46746
No known key found for this signature in database
GPG key ID: 50E0D2423696F995
5 changed files with 26 additions and 30 deletions

View file

@ -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);
}
}
}

View file

@ -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
*

View file

@ -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'));
}
}

View file

@ -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;

View file

@ -123,7 +123,6 @@ class phpbb_test_case_helpers
{
$config = array();
if (extension_loaded('sqlite3'))
{
$config = array_merge($config, array(