From 4d196ba1f39a18dc140203b33383f04d1eac2c2c Mon Sep 17 00:00:00 2001 From: Matt Friedman Date: Thu, 3 Sep 2026 15:13:00 -0700 Subject: [PATCH 1/5] configure iframe embeds preserves unsafe custom bbcode template --- service/media_manager.php | 17 +++++++++--- tests/service/media_manager_test.php | 40 ++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 4 deletions(-) diff --git a/service/media_manager.php b/service/media_manager.php index 116b4e3..83c2f01 100644 --- a/service/media_manager.php +++ b/service/media_manager.php @@ -12,6 +12,7 @@ use s9e\TextFormatter\Configurator; use s9e\TextFormatter\Configurator\Helpers\TemplateLoader; +use s9e\TextFormatter\Configurator\Items\UnsafeTemplate; class media_manager { @@ -50,7 +51,8 @@ public function configure_iframe_embeds(Configurator $configurator) foreach ($configurator->tags as $tag) { - $template_source = (string) $tag->template; + $original_template = $tag->template; + $template_source = (string) $original_template; if ($template_source === '' || stripos($template_source, 'iframe') === false) { @@ -63,9 +65,16 @@ public function configure_iframe_embeds(Configurator $configurator) continue; } - $tag->template = $template; - $configurator->templateNormalizer->normalizeTag($tag); - $configurator->templateChecker->checkTag($tag); + try + { + $tag->template = ($original_template instanceof UnsafeTemplate) ? new UnsafeTemplate($template) : $template; + $configurator->templateNormalizer->normalizeTag($tag); + $configurator->templateChecker->checkTag($tag); + } + catch (\Exception $e) + { + $tag->template = $original_template; + } } } diff --git a/tests/service/media_manager_test.php b/tests/service/media_manager_test.php index f19c57a..46f3bf3 100644 --- a/tests/service/media_manager_test.php +++ b/tests/service/media_manager_test.php @@ -150,6 +150,46 @@ public function test_configure_iframe_embeds_rewrites_custom_s9e_iframes() self::assertStringContainsString('') + ); + + $this->manager->configure_iframe_embeds($configurator); + + $template = $configurator->tags['MYFRAME']->template; + self::assertInstanceOf('\s9e\TextFormatter\Configurator\Items\UnsafeTemplate', $template); + self::assertStringContainsString('$S_CONSENTMANAGER_MEDIA_ALLOWED', (string) $template); + self::assertStringContainsString('data-consent-src="{.}"', (string) $template); + } + + public function test_configure_iframe_embeds_restores_original_template_when_validation_fails() + { + $this->expect_media_enabled(true); + + $configurator = $this->create_configurator_with_tag( + 'CUSTOM', + '' + ); + $original_template = $configurator->tags['CUSTOM']->template; + $configurator->templateChecker->append(new class extends \s9e\TextFormatter\Configurator\TemplateCheck + { + public function check(\DOMElement $template, \s9e\TextFormatter\Configurator\Items\Tag $tag) + { + throw new \RuntimeException('Rejected transformed template'); + } + }); + + $this->manager->configure_iframe_embeds($configurator); + + self::assertSame($original_template, $configurator->tags['CUSTOM']->template); + } + public function test_configure_iframe_embeds_produces_consistent_results_for_identical_templates() { $this->expect_media_enabled(true); From 386d30d76d50429a46b99c09c6722fe55473b72b Mon Sep 17 00:00:00 2001 From: Matt Friedman Date: Thu, 3 Sep 2026 15:15:05 -0700 Subject: [PATCH 2/5] Fix language key issues --- adm/style/consentmanager_acp.html | 10 +++++----- adm/style/consentmanager_acp_banner.html | 2 +- language/en/acp_consentmanager.php | 11 ++++++----- 3 files changed, 12 insertions(+), 11 deletions(-) diff --git a/adm/style/consentmanager_acp.html b/adm/style/consentmanager_acp.html index 795bb90..390d8cd 100644 --- a/adm/style/consentmanager_acp.html +++ b/adm/style/consentmanager_acp.html @@ -42,13 +42,13 @@

{{ lang('WARNING') }}

{{ lang('ACP_CONSENTMANAGER_REGISTRATIONS_EXPLAIN') }}
- {% set services = S_CONSENTMANAGER_MEDIA ? CONSENTMANAGER_SERVICES|merge([{'category': 'media', 'label': lang('CONSENTMANAGER_IFRAMES'), 'id': 'phpbb.consentmanager'}]) : CONSENTMANAGER_SERVICES %} + {% set services = S_CONSENTMANAGER_MEDIA ? CONSENTMANAGER_SERVICES|merge([{'category': 'media', 'label': lang('ACP_CONSENTMANAGER_IFRAMES'), 'id': 'phpbb.consentmanager'}]) : CONSENTMANAGER_SERVICES %} {% if services %} {% for category in [ - { id: 'necessary', label: lang('CONSENTMANAGER_CATEGORY_NECESSARY') }, - { id: 'analytics', label: lang('CONSENTMANAGER_CATEGORY_ANALYTICS') }, - { id: 'marketing', label: lang('CONSENTMANAGER_CATEGORY_MARKETING') }, - { id: 'media', label: lang('CONSENTMANAGER_CATEGORY_MEDIA') } + { id: 'necessary', label: lang('ACP_CONSENTMANAGER_CATEGORY_NECESSARY') }, + { id: 'analytics', label: lang('ACP_CONSENTMANAGER_CATEGORY_ANALYTICS') }, + { id: 'marketing', label: lang('ACP_CONSENTMANAGER_CATEGORY_MARKETING') }, + { id: 'media', label: lang('ACP_CONSENTMANAGER_CATEGORY_MEDIA') } ] %} {% set category_has_services = false %} {% for service in services %} diff --git a/adm/style/consentmanager_acp_banner.html b/adm/style/consentmanager_acp_banner.html index b7be09b..f8c6d0d 100644 --- a/adm/style/consentmanager_acp_banner.html +++ b/adm/style/consentmanager_acp_banner.html @@ -15,7 +15,7 @@

{{ lang('WARNING') }}

- + {% for field in CONSENTMANAGER_BANNER_FIELDS %} {% endfor %} diff --git a/language/en/acp_consentmanager.php b/language/en/acp_consentmanager.php index 4dfd91e..7349f96 100644 --- a/language/en/acp_consentmanager.php +++ b/language/en/acp_consentmanager.php @@ -54,11 +54,12 @@ 'ACP_CONSENTMANAGER_BANNER_FALLBACK_EXPLAIN' => 'Leave a field blank to remove the custom translation and use Consent Manager’s default text for that language.', 'ACP_CONSENTMANAGER_BANNER_TEXT_TOO_LONG' => 'Consent text values must be %d characters or fewer.', 'ACP_CONSENTMANAGER_BANNER_UPDATED' => 'Consent text updated.', - 'CONSENTMANAGER_CATEGORY_NECESSARY' => 'Necessary', - 'CONSENTMANAGER_CATEGORY_ANALYTICS' => 'Analytics', - 'CONSENTMANAGER_CATEGORY_MARKETING' => 'Marketing', - 'CONSENTMANAGER_CATEGORY_MEDIA' => 'Media', - 'CONSENTMANAGER_IFRAMES' => 'BBCode iframe embeds', + 'ACP_CONSENTMANAGER_LANGUAGE' => 'Language', + 'ACP_CONSENTMANAGER_CATEGORY_NECESSARY' => 'Necessary', + 'ACP_CONSENTMANAGER_CATEGORY_ANALYTICS' => 'Analytics', + 'ACP_CONSENTMANAGER_CATEGORY_MARKETING' => 'Marketing', + 'ACP_CONSENTMANAGER_CATEGORY_MEDIA' => 'Media', + 'ACP_CONSENTMANAGER_IFRAMES' => 'BBCode iframe embeds', // Consent logs 'ACP_CONSENTMANAGER_EXPORT_EXPLAIN' => 'Download a CSV file of stored consent log records or permanently delete matching records from the database. All fields are optional; leave them blank to work with the full log.', From c6f69b8d750fb6ad666362cca38226d132bdb64b Mon Sep 17 00:00:00 2001 From: Matt Friedman Date: Thu, 3 Sep 2026 15:31:47 -0700 Subject: [PATCH 3/5] Implement separate throttle identity --- migrations/m4_guest_throttling.php | 59 ++++++++++++++++++++++++++++++ service/log_manager.php | 30 +++++++++++++-- tests/service/log_manager_test.php | 36 ++++++++++++++++-- 3 files changed, 117 insertions(+), 8 deletions(-) create mode 100644 migrations/m4_guest_throttling.php diff --git a/migrations/m4_guest_throttling.php b/migrations/m4_guest_throttling.php new file mode 100644 index 0000000..4a693ea --- /dev/null +++ b/migrations/m4_guest_throttling.php @@ -0,0 +1,59 @@ +db_tools->sql_column_exists($this->table_prefix . 'consentmanager_logs', 'throttle_id'); + } + + public static function depends_on() + { + return [ + '\phpbb\consentmanager\migrations\m1_initial', + '\phpbb\consentmanager\migrations\m3_banner_translations' + ]; + } + + public function update_schema() + { + return [ + 'add_columns' => [ + $this->table_prefix . 'consentmanager_logs' => [ + 'throttle_id' => ['VCHAR:64', ''], + ], + ], + 'add_index' => [ + $this->table_prefix . 'consentmanager_logs' => [ + 'throttle_id' => ['throttle_id'], + ], + ], + ]; + } + + public function revert_schema() + { + return [ + 'drop_keys' => [ + $this->table_prefix . 'consentmanager_logs' => [ + 'throttle_id', + ], + ], + 'drop_columns' => [ + $this->table_prefix . 'consentmanager_logs' => [ + 'throttle_id', + ], + ], + ]; + } +} diff --git a/service/log_manager.php b/service/log_manager.php index 665907a..806782a 100644 --- a/service/log_manager.php +++ b/service/log_manager.php @@ -64,16 +64,18 @@ public function __construct(config $config, driver_interface $db, user $user, $c public function log_consent(array $categories, $version) { $anonymized_id = $this->get_anonymized_subject(); + $throttle_id = $this->get_throttle_subject($anonymized_id); $accepted_categories = json_encode(array_values($categories)); $now = time(); - if ($this->should_suppress_submission($anonymized_id, (int) $version, $accepted_categories, $now)) + if ($this->should_suppress_submission($throttle_id, (int) $version, $accepted_categories, $now)) { return false; } $record = [ 'anonymized_id' => $anonymized_id, + 'throttle_id' => $throttle_id, 'consent_version' => (int) $version, 'accepted_categories' => $accepted_categories, 'consent_time' => $now, @@ -88,18 +90,18 @@ 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 * @param int $now Current Unix timestamp * * @return bool */ - protected function should_suppress_submission($anonymized_id, $version, $accepted_categories, $now) + protected function should_suppress_submission($throttle_id, $version, $accepted_categories, $now) { $sql = 'SELECT consent_version, accepted_categories, consent_time FROM ' . $this->consent_logs_table . " - WHERE anonymized_id = '" . $this->db->sql_escape($anonymized_id) . "' + WHERE throttle_id = '" . $this->db->sql_escape($throttle_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); @@ -138,4 +140,24 @@ protected function get_anonymized_subject() return hash_hmac('sha256', $subject, $this->config['rand_seed']); } + + /** + * Build an anonymized identifier used to throttle the current subject. + * + * Guest throttling uses the requester IP so discarding the session cookie + * cannot reset the duplicate or rate-limit windows. + * + * @param string $anonymized_id Current user or guest-session identifier + * + * @return string + */ + protected function get_throttle_subject($anonymized_id) + { + if ((int) $this->user->data['user_id'] !== ANONYMOUS) + { + return $anonymized_id; + } + + return hash_hmac('sha256', 'ip:' . $this->user->ip, $this->config['rand_seed']); + } } diff --git a/tests/service/log_manager_test.php b/tests/service/log_manager_test.php index ab00aab..b1ef6b0 100644 --- a/tests/service/log_manager_test.php +++ b/tests/service/log_manager_test.php @@ -49,10 +49,11 @@ public function test_log_consent_persists_authenticated_subject() $this->assertSqlResultEquals(array( array( 'anonymized_id' => hash_hmac('sha256', 'u:42', 'random-seed'), + 'throttle_id' => hash_hmac('sha256', 'u:42', 'random-seed'), 'consent_version' => '3', 'accepted_categories' => '["necessary","analytics"]', ), - ), 'SELECT anonymized_id, consent_version, accepted_categories + ), 'SELECT anonymized_id, throttle_id, consent_version, accepted_categories FROM phpbb_consentmanager_logs'); } @@ -64,10 +65,11 @@ public function test_log_consent_uses_session_identifier_for_guests() $this->assertSqlResultEquals(array( array( 'anonymized_id' => hash_hmac('sha256', 's:guest-session', 'random-seed'), + 'throttle_id' => hash_hmac('sha256', 'ip:127.0.0.1', 'random-seed'), 'consent_version' => '9', 'accepted_categories' => '["necessary"]', ), - ), 'SELECT anonymized_id, consent_version, accepted_categories + ), 'SELECT anonymized_id, throttle_id, consent_version, accepted_categories FROM phpbb_consentmanager_logs'); } @@ -80,6 +82,16 @@ public function test_log_consent_suppresses_recent_duplicate() $this->assertLogCount(1); } + public function test_log_consent_suppresses_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); + } + public function test_log_consent_preserves_changed_decision() { $manager = $this->create_manager(ANONYMOUS, 'guest-session'); @@ -113,6 +125,22 @@ 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() + { + 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)); + } + + $same_ip_manager = $this->create_manager(ANONYMOUS, 'new-guest-session', '192.0.2.1'); + self::assertFalse($same_ip_manager->log_consent(array('necessary', 'analytics'), 999)); + + $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); + } + protected function assertLogCount($expected) { $result = $this->db->sql_query('SELECT COUNT(*) AS log_count @@ -123,7 +151,7 @@ protected function assertLogCount($expected) self::assertSame($expected, $count); } - protected function create_manager($user_id, $session_id) + protected function create_manager($user_id, $session_id, $ip = '127.0.0.1') { $config = new \phpbb\config\config(array( 'rand_seed' => 'random-seed', @@ -134,7 +162,7 @@ protected function create_manager($user_id, $session_id) 'user_id' => $user_id, ); $user->session_id = $session_id; - $user->ip = '127.0.0.1'; + $user->ip = $ip; return new \phpbb\consentmanager\service\log_manager( $config, From bd3df56e0e6109208464d65913e65e02ea997258 Mon Sep 17 00:00:00 2001 From: Matt Friedman Date: Thu, 3 Sep 2026 15:38:42 -0700 Subject: [PATCH 4/5] Implement stable extension HMAC secret --- migrations/m5_hmac_secret.php | 38 ++++++++++++++++++++++++++++++ service/acp_manager.php | 5 +--- service/log_manager.php | 4 ++-- tests/service/acp_manager_test.php | 20 +++++++++++++--- tests/service/log_manager_test.php | 23 +++++++++++++----- 5 files changed, 75 insertions(+), 15 deletions(-) create mode 100644 migrations/m5_hmac_secret.php diff --git a/migrations/m5_hmac_secret.php b/migrations/m5_hmac_secret.php new file mode 100644 index 0000000..fa8ae7f --- /dev/null +++ b/migrations/m5_hmac_secret.php @@ -0,0 +1,38 @@ +config['consentmanager_hmac_secret']); + } + + public static function depends_on() + { + return ['\phpbb\consentmanager\migrations\m4_guest_throttling']; + } + + public function update_data() + { + return [ + ['config.add', ['consentmanager_hmac_secret', (string) $this->config['rand_seed']]], + ]; + } + + public function revert_data() + { + return [ + ['config.remove', ['consentmanager_hmac_secret']], + ]; + } +} diff --git a/service/acp_manager.php b/service/acp_manager.php index 7998660..17bd388 100644 --- a/service/acp_manager.php +++ b/service/acp_manager.php @@ -281,16 +281,13 @@ public function delete_logs(array $filters = []) * Mirrors the HMAC used in log_manager::log_consent() so that admins can * filter exports by user ID without exposing raw identifiers. * - * Note: it only matches rows hashed with the current config[rand_seed]. Records - * logged before a rand_seed rotation will not be found. - * * @param int $user_id Numeric phpBB user ID (must be > 0) * * @return string 64-character hex hash */ public function hash_user_id($user_id) { - return hash_hmac('sha256', 'u:' . (int) $user_id, $this->config['rand_seed']); + return hash_hmac('sha256', 'u:' . (int) $user_id, $this->config['consentmanager_hmac_secret']); } /** diff --git a/service/log_manager.php b/service/log_manager.php index 806782a..babcfe0 100644 --- a/service/log_manager.php +++ b/service/log_manager.php @@ -138,7 +138,7 @@ protected function get_anonymized_subject() { $subject = (int) $this->user->data['user_id'] !== ANONYMOUS ? 'u:' . (int) $this->user->data['user_id'] : 's:' . $this->user->session_id; - return hash_hmac('sha256', $subject, $this->config['rand_seed']); + return hash_hmac('sha256', $subject, $this->config['consentmanager_hmac_secret']); } /** @@ -158,6 +158,6 @@ protected function get_throttle_subject($anonymized_id) return $anonymized_id; } - return hash_hmac('sha256', 'ip:' . $this->user->ip, $this->config['rand_seed']); + return hash_hmac('sha256', 'ip:' . $this->user->ip, $this->config['consentmanager_hmac_secret']); } } diff --git a/tests/service/acp_manager_test.php b/tests/service/acp_manager_test.php index 9859cc8..e0e4308 100644 --- a/tests/service/acp_manager_test.php +++ b/tests/service/acp_manager_test.php @@ -83,11 +83,23 @@ public function log_admin_action_data() public function test_hash_user_id_returns_hmac_of_user_prefix() { $manager = $this->create_manager(1, 'session'); - $expected = hash_hmac('sha256', 'u:42', 'random-seed'); + $expected = hash_hmac('sha256', 'u:42', 'consent-secret'); self::assertSame($expected, $manager->hash_user_id(42)); } + public function test_hash_user_id_is_unchanged_after_rand_seed_rotation() + { + $manager_before = $this->create_manager(1, 'session', null, null, null, null, null, [ + 'rand_seed' => 'old-random-seed', + ]); + $manager_after = $this->create_manager(1, 'session', null, null, null, null, null, [ + 'rand_seed' => 'new-random-seed', + ]); + + self::assertSame($manager_before->hash_user_id(42), $manager_after->hash_user_id(42)); + } + public function test_hash_user_id_is_consistent() { $manager = $this->create_manager(1, 'session'); @@ -491,7 +503,7 @@ public function test_stream_logs_csv_filters_by_user_id() self::assertCount(1, $rows); - $expected_hash = hash_hmac('sha256', 'u:42', 'random-seed'); + $expected_hash = hash_hmac('sha256', 'u:42', 'consent-secret'); self::assertStringContainsString($expected_hash, reset($rows)); } @@ -602,7 +614,7 @@ public function test_delete_logs_filters_by_user_id() self::assertCount(1, $rows); - $remaining_hash = hash_hmac('sha256', 'u:99', 'random-seed'); + $remaining_hash = hash_hmac('sha256', 'u:99', 'consent-secret'); self::assertStringContainsString($remaining_hash, reset($rows)); } @@ -644,6 +656,7 @@ protected function create_manager($user_id, $session_id, $log = null, $config_te $config = new \phpbb\config\config(array_merge(array( 'rand_seed' => 'random-seed', + 'consentmanager_hmac_secret' => 'consent-secret', 'consentmanager_analytics_enabled' => 1, 'consentmanager_marketing_enabled' => 1, 'consentmanager_media_enabled' => 1, @@ -804,6 +817,7 @@ protected function create_log_manager($user_id, $session_id) { $config = new \phpbb\config\config(array( 'rand_seed' => 'random-seed', + 'consentmanager_hmac_secret' => 'consent-secret', )); $user = new \phpbb\user($this->language, '\phpbb\datetime'); diff --git a/tests/service/log_manager_test.php b/tests/service/log_manager_test.php index b1ef6b0..bd4ae76 100644 --- a/tests/service/log_manager_test.php +++ b/tests/service/log_manager_test.php @@ -48,8 +48,8 @@ public function test_log_consent_persists_authenticated_subject() $this->assertSqlResultEquals(array( array( - 'anonymized_id' => hash_hmac('sha256', 'u:42', 'random-seed'), - 'throttle_id' => hash_hmac('sha256', 'u:42', 'random-seed'), + 'anonymized_id' => hash_hmac('sha256', 'u:42', 'consent-secret'), + 'throttle_id' => hash_hmac('sha256', 'u:42', 'consent-secret'), 'consent_version' => '3', 'accepted_categories' => '["necessary","analytics"]', ), @@ -64,8 +64,8 @@ public function test_log_consent_uses_session_identifier_for_guests() $this->assertSqlResultEquals(array( array( - 'anonymized_id' => hash_hmac('sha256', 's:guest-session', 'random-seed'), - 'throttle_id' => hash_hmac('sha256', 'ip:127.0.0.1', 'random-seed'), + 'anonymized_id' => hash_hmac('sha256', 's:guest-session', 'consent-secret'), + 'throttle_id' => hash_hmac('sha256', 'ip:127.0.0.1', 'consent-secret'), 'consent_version' => '9', 'accepted_categories' => '["necessary"]', ), @@ -92,6 +92,16 @@ public function test_log_consent_suppresses_guest_duplicate_across_sessions_from $this->assertLogCount(1); } + public function test_log_consent_guest_throttle_is_unchanged_after_rand_seed_rotation() + { + $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'); + + self::assertTrue($manager_before->log_consent(array('necessary'), 1)); + self::assertFalse($manager_after->log_consent(array('necessary'), 1)); + $this->assertLogCount(1); + } + public function test_log_consent_preserves_changed_decision() { $manager = $this->create_manager(ANONYMOUS, 'guest-session'); @@ -151,10 +161,11 @@ protected function assertLogCount($expected) self::assertSame($expected, $count); } - protected function create_manager($user_id, $session_id, $ip = '127.0.0.1') + protected function create_manager($user_id, $session_id, $ip = '127.0.0.1', $rand_seed = 'random-seed') { $config = new \phpbb\config\config(array( - 'rand_seed' => 'random-seed', + 'rand_seed' => $rand_seed, + 'consentmanager_hmac_secret' => 'consent-secret', )); $user = new \phpbb\user($this->language, '\phpbb\datetime'); From 3f614ab08305fa10c5a9b36ab010b79173751757 Mon Sep 17 00:00:00 2001 From: Matt Friedman Date: Thu, 3 Sep 2026 15:45:43 -0700 Subject: [PATCH 5/5] Implement HTTPS-only absolute script sources --- language/en/acp_consentmanager.php | 2 +- service/consent_manager.php | 2 +- styles/all/template/js/consentmanager.js | 6 ++---- tests/javascript/consentmanager.test.js | 11 +++++++++++ tests/service/consent_manager_test.php | 17 +++++++++++++++++ 5 files changed, 32 insertions(+), 6 deletions(-) diff --git a/language/en/acp_consentmanager.php b/language/en/acp_consentmanager.php index 7349f96..532d848 100644 --- a/language/en/acp_consentmanager.php +++ b/language/en/acp_consentmanager.php @@ -33,7 +33,7 @@ 'ACP_CONSENTMANAGER_INTEGRATIONS_REQUIRED' => 'Required properties', 'ACP_CONSENTMANAGER_INTEGRATIONS_REQUIRED_ID' => 'may only use letters, numbers, dots, underscores, colons, and hyphens.', 'ACP_CONSENTMANAGER_INTEGRATIONS_REQUIRED_CAT' => 'must be one of these values:', - 'ACP_CONSENTMANAGER_INTEGRATIONS_REQUIRED_SRC' => 'must be a valid http, https, or relative script URL.', + 'ACP_CONSENTMANAGER_INTEGRATIONS_REQUIRED_SRC' => 'must be a valid HTTPS or relative script URL.', 'ACP_CONSENTMANAGER_INTEGRATIONS_OPTIONAL' => 'Optional properties', 'ACP_CONSENTMANAGER_INTEGRATIONS_EXAMPLE_LABEL' => 'Example Analytics', 'ACP_CONSENTMANAGER_INTEGRATIONS_EXAMPLE_DESC' => 'Loads a simple analytics library after consent.', diff --git a/service/consent_manager.php b/service/consent_manager.php index 2b6e806..f574ed1 100644 --- a/service/consent_manager.php +++ b/service/consent_manager.php @@ -1023,7 +1023,7 @@ protected function is_valid_script_source($src) return false; } - return !isset($parts['scheme']) || in_array(strtolower($parts['scheme']), ['http', 'https'], true); + return !isset($parts['scheme']) || strtolower($parts['scheme']) === 'https'; } /** diff --git a/styles/all/template/js/consentmanager.js b/styles/all/template/js/consentmanager.js index 8cd4520..6d4fca8 100644 --- a/styles/all/template/js/consentmanager.js +++ b/styles/all/template/js/consentmanager.js @@ -575,11 +575,9 @@ return false; } - const link = document.createElement('a'); - link.href = src; - const protocol = (link.protocol || '').toLowerCase(); + const scheme = src.match(/^([a-z][a-z0-9+.-]*):/i); - return protocol === '' || protocol === 'http:' || protocol === 'https:'; + return !scheme || scheme[1].toLowerCase() === 'https'; } function isSafeEmbedSource(src) diff --git a/tests/javascript/consentmanager.test.js b/tests/javascript/consentmanager.test.js index 68b76df..92bbf2d 100644 --- a/tests/javascript/consentmanager.test.js +++ b/tests/javascript/consentmanager.test.js @@ -386,6 +386,11 @@ test('registerScript blocks unsafe sources and executes safe inline scripts', () src: 'javascript:alert(1)' })).toBe(false); + expect(window.consentManager.registerScript('insecure', { + category: 'analytics', + src: 'http://cdn.example.com/script.js' + })).toBe(false); + expect(window.consentManager.registerScript('safe-inline', { category: 'analytics', inline: 'window.safeInlineLoaded = true;' @@ -393,6 +398,12 @@ test('registerScript blocks unsafe sources and executes safe inline scripts', () expect(window.safeInlineLoaded).toBe(true); expect(document.head.querySelectorAll('script[src]').length).toBe(0); + + expect(window.consentManager.registerScript('safe-relative', { + category: 'analytics', + src: '/assets/analytics.js' + })).toBe(true); + expect(document.head.querySelector('script[src="/assets/analytics.js"]')).not.toBeNull(); }); test('processes deferred consent scripts and copies only safe attributes', () => { diff --git a/tests/service/consent_manager_test.php b/tests/service/consent_manager_test.php index df812b7..39f0eb5 100644 --- a/tests/service/consent_manager_test.php +++ b/tests/service/consent_manager_test.php @@ -296,6 +296,10 @@ public function invalid_script_source_data() 'category' => 'analytics', 'src' => 'javascript:alert(1)', )), + 'insecure remote source' => array(array( + 'category' => 'analytics', + 'src' => 'http://cdn.example.com/script.js', + )), 'remote source with forbidden characters' => array(array( 'category' => 'analytics', 'src' => 'https://cdn.example.com/.js', @@ -901,6 +905,19 @@ public function test_normalize_integrations_accepts_empty_json_array_string() self::assertSame(array(), $errors); } + public function test_normalize_integrations_rejects_http_script_source() + { + $manager = $this->get_manager(); + $errors = array(); + $input = '[{"id":"board.analytics","category":"analytics","src":"http://cdn.example.com/analytics.js"}]'; + + self::assertSame(array(), $manager->normalize_integrations($input, $errors)); + self::assertSame( + array($this->language->lang('ACP_CONSENTMANAGER_INVALID_INTEGRATION_ENTRY', 1)), + $errors + ); + } + public function test_validate_log_payload_accepts_valid_hash_and_normalizes_categories() { $manager = $this->get_manager(array(
{{ lang('LANGUAGE') }}{{ lang('ACP_CONSENTMANAGER_LANGUAGE') }}{{ field.LABEL }}