From 6526ff244f01fb20cef9f60c632dd9cc36bdf790 Mon Sep 17 00:00:00 2001 From: Matt Friedman Date: Sun, 13 Sep 2026 16:37:46 -0700 Subject: [PATCH 1/5] Implement wider throttle on shared IPs (same IPs, diff sessions) --- controller/log_controller.php | 3 +- service/log_manager.php | 35 +++++++++++++++++--- tests/controller/log_controller_test.php | 29 ++++++++++++++++- tests/service/log_manager_test.php | 41 ++++++++++++++++-------- 4 files changed, 89 insertions(+), 19 deletions(-) diff --git a/controller/log_controller.php b/controller/log_controller.php index 50efd37..fe7950b 100644 --- a/controller/log_controller.php +++ b/controller/log_controller.php @@ -64,10 +64,11 @@ public function log(Request $request) ], $this->get_error_status_code($submission['error'])); } - $this->log_manager->log_consent($submission['categories'], $submission['version']); + $logged = $this->log_manager->log_consent($submission['categories'], $submission['version']); return new JsonResponse([ 'success' => true, + 'logged' => $logged, 'categories' => $submission['categories'], 'version' => $submission['version'], ]); diff --git a/service/log_manager.php b/service/log_manager.php index babcfe0..bbf22fe 100644 --- a/service/log_manager.php +++ b/service/log_manager.php @@ -22,6 +22,9 @@ class log_manager /** Maximum accepted decisions per anonymized subject during the rate-limit window. */ public const RATE_LIMIT_MAX = 20; + /** Maximum accepted guest decisions per IP address during the rate-limit window. */ + public const IP_RATE_LIMIT_MAX = 200; + /** Rate-limit window in seconds. */ public const RATE_LIMIT_WINDOW = 3600; @@ -68,7 +71,7 @@ public function log_consent(array $categories, $version) $accepted_categories = json_encode(array_values($categories)); $now = time(); - if ($this->should_suppress_submission($throttle_id, (int) $version, $accepted_categories, $now)) + if ($this->should_suppress_submission($anonymized_id, $throttle_id, (int) $version, $accepted_categories, $now)) { return false; } @@ -90,6 +93,7 @@ public function log_consent(array $categories, $version) /** * Suppress rapid duplicates and excessive submissions from one subject. * + * @param string $anonymized_id Anonymized user or guest-session identifier * @param string $throttle_id Anonymized user or guest-IP throttle identifier * @param int $version Consent version * @param string $accepted_categories JSON-encoded normalized categories @@ -97,11 +101,11 @@ public function log_consent(array $categories, $version) * * @return bool */ - protected function should_suppress_submission($throttle_id, $version, $accepted_categories, $now) + protected function should_suppress_submission($anonymized_id, $throttle_id, $version, $accepted_categories, $now) { $sql = 'SELECT consent_version, accepted_categories, consent_time FROM ' . $this->consent_logs_table . " - WHERE throttle_id = '" . $this->db->sql_escape($throttle_id) . "' + WHERE anonymized_id = '" . $this->db->sql_escape($anonymized_id) . "' AND consent_time >= " . ((int) $now - self::RATE_LIMIT_WINDOW) . ' ORDER BY consent_log_id DESC'; $result = $this->db->sql_query_limit($sql, self::RATE_LIMIT_MAX); @@ -126,7 +130,30 @@ protected function should_suppress_submission($throttle_id, $version, $accepted_ return true; } - return $count >= self::RATE_LIMIT_MAX; + if ($count >= self::RATE_LIMIT_MAX) + { + return true; + } + + if ($throttle_id === $anonymized_id) + { + return false; + } + + $sql = 'SELECT consent_log_id + FROM ' . $this->consent_logs_table . " + WHERE throttle_id = '" . $this->db->sql_escape($throttle_id) . "' + AND consent_time >= " . ((int) $now - self::RATE_LIMIT_WINDOW); + $result = $this->db->sql_query_limit($sql, self::IP_RATE_LIMIT_MAX); + $count = 0; + + while ($this->db->sql_fetchrow($result)) + { + $count++; + } + $this->db->sql_freeresult($result); + + return $count >= self::IP_RATE_LIMIT_MAX; } /** diff --git a/tests/controller/log_controller_test.php b/tests/controller/log_controller_test.php index e798a9d..d9c8360 100644 --- a/tests/controller/log_controller_test.php +++ b/tests/controller/log_controller_test.php @@ -80,7 +80,8 @@ public function test_log_persists_valid_submission() { $this->log_manager->expects(self::once()) ->method('log_consent') - ->with(['necessary', 'analytics'], 5); + ->with(['necessary', 'analytics'], 5) + ->willReturn(true); $this->consent_manager->expects(self::once()) ->method('validate_log_payload') @@ -99,11 +100,37 @@ public function test_log_persists_valid_submission() self::assertSame(200, $response->getStatusCode()); self::assertSame(array( 'success' => true, + 'logged' => true, 'categories' => array('necessary', 'analytics'), 'version' => 5, ), json_decode($response->getContent(), true)); } + public function test_log_reports_suppressed_submission() + { + $this->log_manager->expects(self::once()) + ->method('log_consent') + ->willReturn(false); + + $this->consent_manager->expects(self::once()) + ->method('validate_log_payload') + ->willReturn(array( + 'success' => true, + 'categories' => array('necessary'), + 'version' => 5, + )); + + $response = $this->controller->log(new \Symfony\Component\HttpFoundation\Request(array(), array(), array(), array(), array(), array(), '{}')); + + self::assertSame(200, $response->getStatusCode()); + self::assertSame(array( + 'success' => true, + 'logged' => false, + 'categories' => array('necessary'), + 'version' => 5, + ), json_decode($response->getContent(), true)); + } + public function invalid_submission_data() { return array( diff --git a/tests/service/log_manager_test.php b/tests/service/log_manager_test.php index bd4ae76..173a30a 100644 --- a/tests/service/log_manager_test.php +++ b/tests/service/log_manager_test.php @@ -82,24 +82,26 @@ public function test_log_consent_suppresses_recent_duplicate() $this->assertLogCount(1); } - public function test_log_consent_suppresses_guest_duplicate_across_sessions_from_same_ip() + public function test_log_consent_does_not_suppress_guest_duplicate_across_sessions_from_same_ip() { $first_manager = $this->create_manager(ANONYMOUS, 'guest-session-one', '192.0.2.1'); $second_manager = $this->create_manager(ANONYMOUS, 'guest-session-two', '192.0.2.1'); self::assertTrue($first_manager->log_consent(array('necessary'), 1)); - self::assertFalse($second_manager->log_consent(array('necessary'), 1)); - $this->assertLogCount(1); + self::assertTrue($second_manager->log_consent(array('necessary'), 1)); + $this->assertLogCount(2); } - public function test_log_consent_guest_throttle_is_unchanged_after_rand_seed_rotation() + public function test_log_consent_guest_throttle_uses_stable_extension_secret() { - $manager_before = $this->create_manager(ANONYMOUS, 'guest-session-one', '192.0.2.1', 'old-random-seed'); - $manager_after = $this->create_manager(ANONYMOUS, 'guest-session-two', '192.0.2.1', 'new-random-seed'); + $manager = $this->create_manager(ANONYMOUS, 'guest-session', '192.0.2.1', 'random-seed'); + $manager->log_consent(array('necessary'), 1); - self::assertTrue($manager_before->log_consent(array('necessary'), 1)); - self::assertFalse($manager_after->log_consent(array('necessary'), 1)); - $this->assertLogCount(1); + $this->assertSqlResultEquals(array( + array( + 'throttle_id' => hash_hmac('sha256', 'ip:192.0.2.1', 'consent-secret'), + ), + ), 'SELECT throttle_id FROM phpbb_consentmanager_logs'); } public function test_log_consent_preserves_changed_decision() @@ -135,12 +137,25 @@ public function test_log_consent_limits_submissions_per_subject() $this->assertLogCount(\phpbb\consentmanager\service\log_manager::RATE_LIMIT_MAX); } - public function test_log_consent_limits_guest_submissions_across_sessions_from_same_ip() + public function test_log_consent_does_not_share_subject_limit_across_guest_sessions() { for ($version = 1; $version <= \phpbb\consentmanager\service\log_manager::RATE_LIMIT_MAX; $version++) { - $manager = $this->create_manager(ANONYMOUS, 'guest-session-' . $version, '192.0.2.1'); - self::assertTrue($manager->log_consent(array('necessary'), $version)); + $first_manager = $this->create_manager(ANONYMOUS, 'guest-session-one', '192.0.2.1'); + self::assertTrue($first_manager->log_consent(array('necessary'), $version)); + } + + $second_manager = $this->create_manager(ANONYMOUS, 'guest-session-two', '192.0.2.1'); + self::assertTrue($second_manager->log_consent(array('necessary', 'analytics'), 999)); + $this->assertLogCount(\phpbb\consentmanager\service\log_manager::RATE_LIMIT_MAX + 1); + } + + public function test_log_consent_applies_higher_guest_ip_limit_across_sessions() + { + for ($submission = 1; $submission <= \phpbb\consentmanager\service\log_manager::IP_RATE_LIMIT_MAX; $submission++) + { + $manager = $this->create_manager(ANONYMOUS, 'guest-session-' . $submission, '192.0.2.1'); + self::assertTrue($manager->log_consent(array('necessary'), $submission)); } $same_ip_manager = $this->create_manager(ANONYMOUS, 'new-guest-session', '192.0.2.1'); @@ -148,7 +163,7 @@ public function test_log_consent_limits_guest_submissions_across_sessions_from_s $different_ip_manager = $this->create_manager(ANONYMOUS, 'another-guest-session', '192.0.2.2'); self::assertTrue($different_ip_manager->log_consent(array('necessary', 'analytics'), 999)); - $this->assertLogCount(\phpbb\consentmanager\service\log_manager::RATE_LIMIT_MAX + 1); + $this->assertLogCount(\phpbb\consentmanager\service\log_manager::IP_RATE_LIMIT_MAX + 1); } protected function assertLogCount($expected) From 651ab2c95d2b38c548b07fc2fad449e236438467 Mon Sep 17 00:00:00 2001 From: Matt Friedman Date: Sun, 13 Sep 2026 17:26:06 -0700 Subject: [PATCH 2/5] failed validation restores exact submitted textarea and checkbox values --- controller/acp_controller.php | 29 +++++++++++++++++++----- tests/controller/acp_controller_test.php | 6 ++++- 2 files changed, 28 insertions(+), 7 deletions(-) diff --git a/controller/acp_controller.php b/controller/acp_controller.php index 0d91e5a..6c95909 100644 --- a/controller/acp_controller.php +++ b/controller/acp_controller.php @@ -94,17 +94,23 @@ public function handle() { $this->validate_form_key('phpbb_consentmanager_acp'); - $errors = []; - $saved = $this->acp_manager->save_settings([ + $submitted_settings = [ 'analytics_enabled' => $this->request->variable('consentmanager_analytics_enabled', 0), 'marketing_enabled' => $this->request->variable('consentmanager_marketing_enabled', 0), 'media_enabled' => $this->request->variable('consentmanager_media_enabled', 0), - 'integrations' => trim($this->request->raw_variable('consentmanager_integrations', '')), + 'integrations' => $this->request->raw_variable('consentmanager_integrations', ''), + ]; + $errors = []; + $saved = $this->acp_manager->save_settings([ + 'analytics_enabled' => $submitted_settings['analytics_enabled'], + 'marketing_enabled' => $submitted_settings['marketing_enabled'], + 'media_enabled' => $submitted_settings['media_enabled'], + 'integrations' => trim($submitted_settings['integrations']), ], $errors); if (!$saved) { - $this->assign_template_vars($errors); + $this->assign_template_vars($errors, $submitted_settings); return; } @@ -347,10 +353,21 @@ protected function get_current_form_token_fields() ]; } - protected function assign_template_vars(array $errors = []) + protected function assign_template_vars(array $errors = [], array $submitted_settings = null) { + $template_data = $this->acp_manager->get_settings_template_data(); + if ($submitted_settings !== null) + { + $template_data = array_merge($template_data, [ + 'S_CONSENTMANAGER_ANALYTICS' => !empty($submitted_settings['analytics_enabled']), + 'S_CONSENTMANAGER_MARKETING' => !empty($submitted_settings['marketing_enabled']), + 'S_CONSENTMANAGER_MEDIA' => !empty($submitted_settings['media_enabled']), + 'CONSENTMANAGER_INTEGRATIONS' => (string) $submitted_settings['integrations'], + ]); + } + $this->template->assign_vars(array_merge( - $this->acp_manager->get_settings_template_data(), + $template_data, [ 'S_ERROR' => !empty($errors), 'ERROR_MSG' => implode('
', $errors), diff --git a/tests/controller/acp_controller_test.php b/tests/controller/acp_controller_test.php index 38a29a0..83d712c 100644 --- a/tests/controller/acp_controller_test.php +++ b/tests/controller/acp_controller_test.php @@ -145,7 +145,11 @@ public function test_handle_submit_validation_errors_reassigns_form_data() return $vars['S_ERROR'] && $vars['ERROR_MSG'] === 'Invalid integrations' && $vars['U_ACTION'] === self::ACP_URL - && isset($vars['CONSENTMANAGER_VERSION']); + && isset($vars['CONSENTMANAGER_VERSION']) + && $vars['S_CONSENTMANAGER_ANALYTICS'] === true + && $vars['S_CONSENTMANAGER_MARKETING'] === false + && $vars['S_CONSENTMANAGER_MEDIA'] === false + && $vars['CONSENTMANAGER_INTEGRATIONS'] === " invalid json \n"; })]; $this->template->expects(self::once()) From f4fac08a0e3be652c601e29aa64fbb79002423d6 Mon Sep 17 00:00:00 2001 From: Matt Friedman Date: Sun, 13 Sep 2026 17:34:18 -0700 Subject: [PATCH 3/5] Validates everything, then atomically performs one delete plus sql_multi_insert Fix also prevents partial saves when later fields fail. --- service/translation_manager.php | 92 ++++++++-------------- tests/service/translation_manager_test.php | 26 ++++++ 2 files changed, 60 insertions(+), 58 deletions(-) diff --git a/service/translation_manager.php b/service/translation_manager.php index 4c2e9fd..308110d 100644 --- a/service/translation_manager.php +++ b/service/translation_manager.php @@ -188,6 +188,9 @@ public function save_translations(array $submitted_translations, array $allowed_ $allowed_languages = array_fill_keys($installed_languages, true); $allowed_keys = array_intersect($allowed_keys, array_keys(self::BANNER_FIELDS)); $allowed_keys = array_fill_keys($allowed_keys, true); + $delete_keys_by_language = []; + $replacement_rows = []; + $updated_at = time(); foreach ($submitted_translations as $lang_iso => $translations) { @@ -202,11 +205,11 @@ public function save_translations(array $submitted_translations, array $allowed_ { continue; } + $delete_keys_by_language[$lang_iso][$translation_key] = true; $translation_text = trim((string) $translation_text); if ($translation_text === '' || $translation_text === $this->get_language_default($lang_iso, self::BANNER_FIELDS[$translation_key]['fallback'])) { - $this->delete_translation($translation_key, $lang_iso); continue; } @@ -227,7 +230,16 @@ public function save_translations(array $submitted_translations, array $allowed_ continue; } - $this->upsert_translation($translation_key, $lang_iso, $translation_text, $parsed_text, $uid, $bitfield, $options); + $replacement_rows[] = [ + 'translation_key' => $translation_key, + 'lang_iso' => $lang_iso, + 'translation_text' => utf8_encode_ucr($translation_text), + 'translation_text_parsed' => $parsed_text, + 'translation_uid' => $uid, + 'translation_bitfield' => $bitfield, + 'translation_options' => (int) $options, + 'updated_at' => $updated_at, + ]; } } @@ -236,6 +248,26 @@ public function save_translations(array $submitted_translations, array $allowed_ return false; } + if (!empty($delete_keys_by_language)) + { + $delete_conditions = []; + foreach ($delete_keys_by_language as $lang_iso => $translation_keys) + { + $delete_conditions[] = "(lang_iso = '" . $this->db->sql_escape($lang_iso) . "' + AND " . $this->db->sql_in_set('translation_key', array_keys($translation_keys)) . ')'; + } + + $this->db->sql_transaction('begin'); + $this->db->sql_query('DELETE FROM ' . $this->translations_table . ' + WHERE ' . implode(' OR ', $delete_conditions)); + + if (!empty($replacement_rows)) + { + $this->db->sql_multi_insert($this->translations_table, $replacement_rows); + } + $this->db->sql_transaction('commit'); + } + $this->translations = null; $this->consent_cache->invalidate_translations(); @@ -314,62 +346,6 @@ protected function get_custom_translation_row($translation_key, $lang_iso) return $translations[$translation_key][$lang_iso] ?? null; } - /** - * Insert or update a custom translation. - * - * @return void - */ - protected function upsert_translation($translation_key, $lang_iso, $translation_text, $parsed_text, $uid, $bitfield, $options) - { - $sql_ary = [ - 'translation_key' => $translation_key, - 'lang_iso' => $lang_iso, - 'translation_text' => utf8_encode_ucr($translation_text), - 'translation_text_parsed' => $parsed_text, - 'translation_uid' => $uid, - 'translation_bitfield' => $bitfield, - 'translation_options' => (int) $options, - 'updated_at' => time(), - ]; - - $sql = 'SELECT translation_id - FROM ' . $this->translations_table . " - WHERE translation_key = '" . $this->db->sql_escape($translation_key) . "' - AND lang_iso = '" . $this->db->sql_escape($lang_iso) . "'"; - $result = $this->db->sql_query($sql); - $translation_id = (int) $this->db->sql_fetchfield('translation_id'); - $this->db->sql_freeresult($result); - - if ($translation_id) - { - $sql = 'UPDATE ' . $this->translations_table . ' - SET ' . $this->db->sql_build_array('UPDATE', $sql_ary) . ' - WHERE translation_id = ' . $translation_id; - } - else - { - $sql = 'INSERT INTO ' . $this->translations_table . ' ' . $this->db->sql_build_array('INSERT', $sql_ary); - } - - $this->db->sql_query($sql); - } - - /** - * Delete a custom translation. - * - * @param string $translation_key Translation key - * @param string $lang_iso Language ISO - * - * @return void - */ - protected function delete_translation($translation_key, $lang_iso) - { - $sql = 'DELETE FROM ' . $this->translations_table . " - WHERE translation_key = '" . $this->db->sql_escape($translation_key) . "' - AND lang_iso = '" . $this->db->sql_escape($lang_iso) . "'"; - $this->db->sql_query($sql); - } - /** * Return an extension language-file default for a specific language. * diff --git a/tests/service/translation_manager_test.php b/tests/service/translation_manager_test.php index 867569d..90ca52f 100644 --- a/tests/service/translation_manager_test.php +++ b/tests/service/translation_manager_test.php @@ -232,6 +232,32 @@ public function test_parser_errors_abort_save() $this->assertSqlResultEquals([], 'SELECT translation_key FROM phpbb_consentmanager_translations'); } + public function test_validation_errors_do_not_partially_replace_translations() + { + $manager = $this->create_manager(); + $errors = []; + + self::assertTrue($manager->save_translations([ + 'en' => [ + 'banner_title' => 'Original title', + ], + ], ['banner_title'], $errors)); + + self::assertFalse($manager->save_translations([ + 'en' => [ + 'banner_title' => 'Changed title', + 'banner_message' => '[img]https://example.com/image.png[/img]', + ], + ], ['banner_title', 'banner_message'], $errors)); + + self::assertNotEmpty($errors); + self::assertSame('Original title', $manager->get_translation('banner_title', 'CONSENTMANAGER_DEFAULT_BANNER_TITLE', 'en')); + $this->assertSqlResultEquals( + [['translation_key' => 'banner_title', 'translation_text' => 'Original title']], + 'SELECT translation_key, translation_text FROM phpbb_consentmanager_translations' + ); + } + public function test_accepts_translation_text_at_maximum_length() { $manager = $this->create_manager(); From 94a4c72645261ea83ee7cf3e1fec2ffdf4c19412 Mon Sep 17 00:00:00 2001 From: Matt Friedman Date: Sun, 13 Sep 2026 18:46:53 -0700 Subject: [PATCH 4/5] ACP integrations only use absolute scrupt src to prevent relative path problems --- DOCUMENTATION.md | 4 ++-- language/en/acp_consentmanager.php | 4 ++-- service/consent_manager.php | 24 ++++++++++++++++++++- tests/functional/acp_test.php | 2 +- tests/functional/frontend_test.php | 2 +- tests/service/consent_manager_test.php | 30 ++++++++++++++++++++++++++ 6 files changed, 59 insertions(+), 7 deletions(-) diff --git a/DOCUMENTATION.md b/DOCUMENTATION.md index 3ceea5a..701355b 100644 --- a/DOCUMENTATION.md +++ b/DOCUMENTATION.md @@ -122,7 +122,7 @@ $accepted = $consent_manager->register(string $id, array $definition); - Registration IDs and script IDs may only use letters, numbers, `.`, `_`, `:`, and `-`, and must start with a letter or number. - Supported categories are `necessary`, `analytics`, `marketing`, and `media`. - Each `scripts` definition must use **one** of these execution sources: `src`, `asset`, or `inline`. -- `src` accepts `http`, `https`, or relative URLs. URLs such as `//example.com/...` are not allowed. +- `src` accepts HTTPS or relative URLs. URLs such as `//example.com/...` are not allowed. - `asset` must be a local phpBB asset path such as `@vendor_example/js/file.js`. - Unsafe HTML event-handler attributes such as `onclick` are ignored. @@ -172,7 +172,7 @@ Each entry inside `scripts` supports the following options. |----------------------|------------------------------|-------------------------------------------------------------------------------------------------------------------------|---------------------------------------------------| | `id` | Optional | Unique ID for this script. If omitted, Consent Manager creates one when needed. | `'id' => 'vendor.example.analytics.loader'` | | `category` | Optional | Lets this script use a different category from the main registration. Most extensions should keep the same category. | `'category' => 'marketing'` | -| `src` | Needed for remote files | URL of an external script, or a relative URL. Do not combine with `asset` or `inline`. | `'src' => 'https://cdn.example.com/analytics.js'` | +| `src` | Needed for remote files | HTTPS URL of an external script, or a path relative to the phpBB root. Do not combine with `asset` or `inline`. | `'src' => 'https://cdn.example.com/analytics.js'` | | `asset` | Needed for extension files | Local phpBB asset path. Best for JavaScript files that ship with your extension. Do not combine with `src` or `inline`. | `'asset' => '@vendor_example/js/analytics.js'` | | `inline` | Needed for inline JavaScript | JavaScript code that Consent Manager should inject after consent. Do not combine with `src` or `asset`. | `'inline' => 'window.tracker.start();'` | | `async` | Optional | Sets `