Merge pull request #6594 from marc1706/ticket/17077

[ticket/17077] Improve handling of posting to reduce double submit possibility

phpBB/config/default/container/services_content.yml

@@ -67,6 +67,13 @@ services:
             - '@controller.helper'
             - '@dispatcher'
 
+    posting.lock:
+        class: phpbb\lock\posting
+        shared: false
+        arguments:
+            - '@cache.driver'
+            - '@config'
+
     viewonline_helper:
         class: phpbb\viewonline_helper
         arguments:

phpBB/phpbb/lock/posting.php

@@ -0,0 +1,77 @@
+<?php
+/**
+ *
+ * This file is part of the phpBB Forum Software package.
+ *
+ * @copyright (c) phpBB Limited <https://www.phpbb.com>
+ * @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;
+
+class posting
+{
+	/** @var cache_interface */
+	private $cache;
+
+	/** @var config */
+	private $config;
+
+	/** @var string */
+	private $lock_name = '';
+
+	/**
+	 * Constructor for posting lock
+	 *
+	 * @param cache_interface $cache
+	 * @param config $config
+	 */
+	public function __construct(cache_interface $cache, config $config)
+	{
+		$this->cache = $cache;
+		$this->config = $config;
+	}
+
+	/**
+	 * 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 set_lock_name(int $creation_time, string $form_token): void
+	{
+		$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(int $creation_time, string $form_token): bool
+	{
+		$this->set_lock_name($creation_time, $form_token);
+
+		// 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->cache->put($this->lock_name, true, $this->config['flood_interval']);
+
+		return true;
+	}
+}

phpBB/posting.php

@@ -1429,7 +1429,14 @@
 	// Store message, sync counters
 	if (!count($error) && $submit)
 	{
-		if ($submit)
+		/** @var \phpbb\lock\posting $posting_lock */
+		$posting_lock = $phpbb_container->get('posting.lock');
+
+		// 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'];
@@ -1620,6 +1627,11 @@
 
 			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');
+		}
 	}
 }
 

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

phpBB/styles/prosilver/template/posting_editor.html

@@ -100,7 +100,7 @@
 	<!-- IF not S_SHOW_DRAFTS and not $SIG_EDIT eq 1 -->
 	<div class="panel bg2">
 		<div class="inner">
-		<fieldset class="submit-buttons">
+		<fieldset class="submit-buttons" data-prevent-flood>
 			{S_HIDDEN_ADDRESS_FIELD}
 			{S_HIDDEN_FIELDS}
 			<!-- EVENT posting_editor_submit_buttons -->

tests/lock/posting_test.php

@@ -0,0 +1,56 @@
+<?php
+/**
+ *
+ * This file is part of the phpBB Forum Software package.
+ *
+ * @copyright (c) phpBB Limited <https://www.phpbb.com>
+ * @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->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->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'));
+	}
+}

tests/test_framework/phpbb_functional_test_case.php

@@ -96,6 +96,14 @@ protected function setUp(): void
 
 		$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 @@ protected function tearDown(): void
 
 		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 @@ public function __construct($name = NULL, array $data = [], $dataName = '')
 		$this->excludeBackupStaticAttributes($backupStaticAttributesBlacklist);
 	}
 
+	/**
+	 * @return \phpbb\db\driver\driver_interface
+	 */
 	protected function get_db()
 	{
 		global $phpbb_root_path, $phpEx;

tests/test_framework/phpbb_test_case_helpers.php

@@ -123,7 +123,6 @@ static public function get_test_config()
 	{
 		$config = array();
 
-
 		if (extension_loaded('sqlite3'))
 		{
 			$config = array_merge($config, array(