From c07c6816fca9b82d7009d1ab6b8d94f95f7876e5 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Mon, 8 Apr 2024 21:50:27 +0200 Subject: [PATCH 1/4] [ticket/17077] Improve handling of accidental double submission of posts PHPBB3-17077 --- .../default/container/services_content.yml | 8 ++ phpBB/phpbb/lock/posting.php | 82 +++++++++++++++++++ phpBB/posting.php | 10 ++- phpBB/styles/prosilver/template/ajax.js | 23 ++++++ .../prosilver/template/posting_editor.html | 2 +- 5 files changed, 123 insertions(+), 2 deletions(-) create mode 100644 phpBB/phpbb/lock/posting.php diff --git a/phpBB/config/default/container/services_content.yml b/phpBB/config/default/container/services_content.yml index 8b240067e4..e2d53784c2 100644 --- a/phpBB/config/default/container/services_content.yml +++ b/phpBB/config/default/container/services_content.yml @@ -67,6 +67,14 @@ services: - '@controller.helper' - '@dispatcher' + posting.lock: + class: phpbb\lock\posting + shared: false + arguments: + - '@cache.driver' + - '@config' + - '@request' + viewonline_helper: class: phpbb\viewonline_helper arguments: diff --git a/phpBB/phpbb/lock/posting.php b/phpBB/phpbb/lock/posting.php new file mode 100644 index 0000000000..d912475890 --- /dev/null +++ b/phpBB/phpbb/lock/posting.php @@ -0,0 +1,82 @@ + + * @license GNU General Public License, version 2 (GPL-2.0) + * + * For full copyright and license information, please see + * the docs/CREDITS.txt file. + * + */ + +namespace phpbb\lock; + +use phpbb\cache\driver\driver_interface as cache_interface; +use phpbb\config\config; +use phpbb\request\request_interface; + +class posting +{ + /** @var cache_interface */ + private $cache; + + /** @var config */ + private $config; + + /** @var request_interface */ + private $request; + + /** @var string */ + private $lock_name = ''; + + /** + * Constructor for posting lock + * + * @param cache_interface $cache + * @param config $config + * @param request_interface $request + */ + public function __construct(cache_interface $cache, config $config, request_interface $request) + { + $this->cache = $cache; + $this->config = $config; + $this->request = $request; + } + + /** + * Get lock name + * @return string Lock name + */ + private function lock_name(): string + { + if ($this->lock_name) + { + return $this->lock_name; + } + + $creation_time = abs($this->request->variable('creation_time', 0)); + $token = $this->request->variable('form_token', ''); + + return sha1(((string) $creation_time) . $token) . '_posting_lock'; + } + + /** + * Acquire lock for current posting form submission + * + * @return bool True if lock could be acquired, false if not + */ + public function acquire(): bool + { + // Lock is held for session, cannot acquire it + if ($this->cache->_exists($this->lock_name())) + { + return false; + } + + $this->cache->put($this->lock_name(), true, $this->config['flood_interval']); + + return true; + } +} \ No newline at end of file diff --git a/phpBB/posting.php b/phpBB/posting.php index 724d8a8df1..7b3d2243ab 100644 --- a/phpBB/posting.php +++ b/phpBB/posting.php @@ -1429,7 +1429,10 @@ if ($submit || $preview || $refresh) // Store message, sync counters if (!count($error) && $submit) { - if ($submit) + /** @var \phpbb\lock\posting $posting_lock */ + $posting_lock = $phpbb_container->get('posting.lock'); + + if ($posting_lock->acquire()) { // Lock/Unlock Topic $change_topic_status = $post_data['topic_status']; @@ -1620,6 +1623,11 @@ if ($submit || $preview || $refresh) redirect($redirect_url); } + else + { + // Posting was already locked before, hence form submission was already attempted once and is now invalid + $error[] = $language->lang('FORM_INVALID'); + } } } diff --git a/phpBB/styles/prosilver/template/ajax.js b/phpBB/styles/prosilver/template/ajax.js index 79187470d8..e01e7eb4d4 100644 --- a/phpBB/styles/prosilver/template/ajax.js +++ b/phpBB/styles/prosilver/template/ajax.js @@ -337,6 +337,29 @@ $('[data-ajax]').each(function() { } }); +// Prevent accidental double submission of form +$('[data-prevent-flood] input[type=submit]').click(function(event) { + const $submitButton = $(this); // Store the button element + const $form = $submitButton.closest('form'); + + // Always add the disabled class for visual feedback + $submitButton.addClass('disabled'); + + // Submit form if it hasn't been submitted yet + if (!$form.prop('data-form-submitted')) { + $form.prop('data-form-submitted', true); + + return; + } + + // Prevent default submission for subsequent clicks within 5 seconds + event.preventDefault(); + + setTimeout(() => { + $form.prop('removeProp', 'data-form-submitted'); + $submitButton.removeClass('disabled'); // Re-enable after 5 seconds + }, 5000); +}); /** * This simply appends #preview to the action of the diff --git a/phpBB/styles/prosilver/template/posting_editor.html b/phpBB/styles/prosilver/template/posting_editor.html index 12790360d6..d3b2ecd68e 100644 --- a/phpBB/styles/prosilver/template/posting_editor.html +++ b/phpBB/styles/prosilver/template/posting_editor.html @@ -100,7 +100,7 @@
-
+
{S_HIDDEN_ADDRESS_FIELD} {S_HIDDEN_FIELDS} From 3b5777f9008b49b7ceb4b80b75a091782a6e9b86 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Tue, 9 Apr 2024 20:53:05 +0200 Subject: [PATCH 2/4] [ticket/17077] Move request handling outside locking and add release PHPBB3-17077 --- .../default/container/services_content.yml | 1 - phpBB/phpbb/lock/posting.php | 59 +++++++++++-------- phpBB/posting.php | 9 ++- 3 files changed, 44 insertions(+), 25 deletions(-) diff --git a/phpBB/config/default/container/services_content.yml b/phpBB/config/default/container/services_content.yml index e2d53784c2..0dc0829d9e 100644 --- a/phpBB/config/default/container/services_content.yml +++ b/phpBB/config/default/container/services_content.yml @@ -73,7 +73,6 @@ services: arguments: - '@cache.driver' - '@config' - - '@request' viewonline_helper: class: phpbb\viewonline_helper diff --git a/phpBB/phpbb/lock/posting.php b/phpBB/phpbb/lock/posting.php index d912475890..1fee36e63f 100644 --- a/phpBB/phpbb/lock/posting.php +++ b/phpBB/phpbb/lock/posting.php @@ -15,7 +15,6 @@ namespace phpbb\lock; use phpbb\cache\driver\driver_interface as cache_interface; use phpbb\config\config; -use phpbb\request\request_interface; class posting { @@ -25,58 +24,72 @@ class posting /** @var config */ private $config; - /** @var request_interface */ - private $request; - /** @var string */ private $lock_name = ''; + /** @var bool Lock state */ + private $locked = false; + /** * Constructor for posting lock * * @param cache_interface $cache * @param config $config - * @param request_interface $request */ - public function __construct(cache_interface $cache, config $config, request_interface $request) + public function __construct(cache_interface $cache, config $config) { $this->cache = $cache; $this->config = $config; - $this->request = $request; } /** - * Get lock name - * @return string Lock name + * Set lock name + * + * @param int $creation_time Creation time of form, must be checked already + * @param string $form_token Form token used for form, must be checked already + * + * @return void */ - private function lock_name(): string + private function set_lock_name(int $creation_time, string $form_token): void { - if ($this->lock_name) - { - return $this->lock_name; - } - - $creation_time = abs($this->request->variable('creation_time', 0)); - $token = $this->request->variable('form_token', ''); - - return sha1(((string) $creation_time) . $token) . '_posting_lock'; + $this->lock_name = sha1(((string) $creation_time) . $form_token) . '_posting_lock'; } /** * Acquire lock for current posting form submission * + * @param int $creation_time Creation time of form, must be checked already + * @param string $form_token Form token used for form, must be checked already + * * @return bool True if lock could be acquired, false if not */ - public function acquire(): bool + public function acquire(int $creation_time, string $form_token): bool { + $this->set_lock_name($creation_time, $form_token); + // Lock is held for session, cannot acquire it - if ($this->cache->_exists($this->lock_name())) + if ($this->cache->_exists($this->lock_name)) { return false; } - $this->cache->put($this->lock_name(), true, $this->config['flood_interval']); + $this->locked = true; + + $this->cache->put($this->lock_name, true, $this->config['flood_interval']); return true; } -} \ No newline at end of file + + /** + * 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 7b3d2243ab..bea75081c1 100644 --- a/phpBB/posting.php +++ b/phpBB/posting.php @@ -1432,7 +1432,11 @@ if ($submit || $preview || $refresh) /** @var \phpbb\lock\posting $posting_lock */ $posting_lock = $phpbb_container->get('posting.lock'); - if ($posting_lock->acquire()) + // Get creation time and form token, must be already checked at this point + $creation_time = abs($request->variable('creation_time', 0)); + $form_token = $request->variable('form_token', ''); + + if ($posting_lock->acquire($creation_time, $form_token)) { // Lock/Unlock Topic $change_topic_status = $post_data['topic_status']; @@ -1561,6 +1565,9 @@ 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 * From 98929ca9838a60d5dc8a5f454ab101170f1975bc Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Tue, 9 Apr 2024 21:13:28 +0200 Subject: [PATCH 3/4] [ticket/17077] Add test for posting lock PHPBB3-17077 --- tests/lock/posting_test.php | 54 +++++++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) create mode 100644 tests/lock/posting_test.php diff --git a/tests/lock/posting_test.php b/tests/lock/posting_test.php new file mode 100644 index 0000000000..e9a9cdfffb --- /dev/null +++ b/tests/lock/posting_test.php @@ -0,0 +1,54 @@ + + * @license GNU General Public License, version 2 (GPL-2.0) + * + * For full copyright and license information, please see + * the docs/CREDITS.txt file. + * + */ + +use phpbb\cache\driver\file as file_cache; +use phpbb\config\config; +use phpbb\lock\posting; + +class phpbb_lock_posting_test extends phpbb_test_case +{ + /** @var \phpbb\cache\driver\file */ + protected $cache; + + /** @var config */ + protected $config; + + /** @var posting */ + protected $lock; + + public function setUp(): void + { + $this->cache = new file_cache(__DIR__ . '/../tmp/'); + $this->cache->purge(); // ensure cache is clean + $this->config = new config([ + 'flood_interval' => 15, + ]); + $this->lock = new posting($this->cache, $this->config); + } + + public function test_lock_acquire() + { + $this->assertTrue($this->lock->acquire(100, 'foo')); + $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->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')); + } +} \ No newline at end of file From 6f45b467466c963d874bad1a442ce3c26b2e34dd Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Wed, 1 May 2024 11:22:29 +0200 Subject: [PATCH 4/4] [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(