From 27d4c6ec15e59edb79d789762165682bfd51110a Mon Sep 17 00:00:00 2001 From: Andy Vandenberghe Date: Sun, 4 Oct 2026 22:02:46 +0200 Subject: [PATCH] Validate the UCP location and number preferences ucp_prefs_get_data() copied rt_location, rt_viewforum_location and rt_number from the request unchecked, and ucp_prefs_set_data() saved them. Any string up to the column length and any integer could be stored; an unknown location just hid the user's own block. Keep each location within the options the UCP offers (RT_TOP, RT_BOTTOM, RT_SIDE; no side column in the forum view), falling back to the user's stored value and then the board default. Clamp the number to 1-999, and set the form's min to 1 to match. Closes #198 Co-Authored-By: Claude Opus 5.5 --- event/ucp_listener.php | 48 +++++++++++- .../ucp_prefs_view_select_menu_append.html | 2 +- tests/event/ucp_listener_test.php | 74 +++++++++++++++++++ tests/tests.md | 4 +- 4 files changed, 123 insertions(+), 5 deletions(-) diff --git a/event/ucp_listener.php b/event/ucp_listener.php index 285416a..603687e 100644 --- a/event/ucp_listener.php +++ b/event/ucp_listener.php @@ -28,6 +28,16 @@ */ class ucp_listener implements EventSubscriberInterface { + /** Index-page block locations a user may choose */ + const LOCATIONS = ['RT_TOP', 'RT_BOTTOM', 'RT_SIDE']; + + /** Forum-view block locations a user may choose; there is no side column there */ + const VIEWFORUM_LOCATIONS = ['RT_TOP', 'RT_BOTTOM']; + + /** Range for the number of topics per page, matching the UCP form's max */ + const NUMBER_MIN = 1; + const NUMBER_MAX = 999; + /** * @var auth */ @@ -120,9 +130,17 @@ public function ucp_prefs_get_data($event) $event['data'] = array_merge( $event['data'], array( 'rt_enable' => $this->request->variable('rt_enable', (int) $this->user->data['user_rt_enable']), - 'rt_location' => $this->request->variable('rt_location', $this->user->data['user_rt_location']), - 'rt_viewforum_location' => $this->request->variable('rt_viewforum_location', $this->user->data['user_rt_viewforum_location']), - 'rt_number' => $this->request->variable('rt_number', (int) $this->user->data['user_rt_number']), + 'rt_location' => $this->valid_location( + $this->request->variable('rt_location', $this->user->data['user_rt_location']), + self::LOCATIONS, $this->user->data['user_rt_location'], $this->config['rt_location'] + ), + 'rt_viewforum_location' => $this->valid_location( + $this->request->variable('rt_viewforum_location', $this->user->data['user_rt_viewforum_location']), + self::VIEWFORUM_LOCATIONS, $this->user->data['user_rt_viewforum_location'], $this->config['rt_viewforum_location'] + ), + 'rt_number' => max(self::NUMBER_MIN, min(self::NUMBER_MAX, + $this->request->variable('rt_number', (int) $this->user->data['user_rt_number']) + )), 'rt_sort_start_time' => $this->request->variable('rt_sort_start_time', (int) $this->user->data['user_rt_sort_start_time']), 'rt_unread_only' => $this->request->variable('rt_unread_only', (int) $this->user->data['user_rt_unread_only']), ) @@ -274,6 +292,30 @@ public function ucp_prefs_set_data($event) $event['sql_ary'] = array_merge($event['sql_ary'], $sql_ary); } + /** + * Return a submitted block location if it is one of the allowed options (issue #198). + * + * Otherwise keep the user's stored location, or use the board default if that is not valid either. + * + * @param string $submitted Location from the request + * @param array $allowed Valid locations for this setting + * @param string $stored The user's current location + * @param string $default The board-wide default location + * @return string + */ + private function valid_location($submitted, array $allowed, $stored, $default) + { + foreach ([$submitted, $stored, $default] as $location) + { + if (in_array($location, $allowed, true)) + { + return $location; + } + } + + return $allowed[0]; + } + /** * set a newly registered account's Recent Topics preferences from default. * diff --git a/styles/all/template/event/ucp_prefs_view_select_menu_append.html b/styles/all/template/event/ucp_prefs_view_select_menu_append.html index ab8a2d5..f520e0b 100644 --- a/styles/all/template/event/ucp_prefs_view_select_menu_append.html +++ b/styles/all/template/event/ucp_prefs_view_select_menu_append.html @@ -36,7 +36,7 @@ {% if A_RT_NUMBER %}

{{ lang('RT_NUMBER_EXP') }}
-
+
{% endif %} diff --git a/tests/event/ucp_listener_test.php b/tests/event/ucp_listener_test.php index 1e8888f..63580d8 100644 --- a/tests/event/ucp_listener_test.php +++ b/tests/event/ucp_listener_test.php @@ -237,6 +237,80 @@ public function test_ucp_prefs_get_data_no_submit() $this->assertEquals(5, $event['data']['rt_number']); } + /** + * Submitted location / number preferences, and what must reach $event['data'] (#198). + * Stored user values: location RT_BOTTOM, viewforum location RT_TOP, number 5. + */ + public function submitted_preferences_data() + { + return array( + 'valid values pass through' => array('RT_SIDE', 'RT_BOTTOM', 20, 'RT_SIDE', 'RT_BOTTOM', 20), + 'unknown location keeps stored value' => array('RT_EVIL', 'RT_TOP', 5, 'RT_BOTTOM', 'RT_TOP', 5), + 'side is not a viewforum location' => array('RT_TOP', 'RT_SIDE', 5, 'RT_TOP', 'RT_TOP', 5), + 'number above 999 is clamped' => array('RT_TOP', 'RT_TOP', 100000, 'RT_TOP', 'RT_TOP', 999), + 'zero is raised to 1' => array('RT_TOP', 'RT_TOP', 0, 'RT_TOP', 'RT_TOP', 1), + 'negative number is raised to 1' => array('RT_TOP', 'RT_TOP', -5, 'RT_TOP', 'RT_TOP', 1), + ); + } + + /** + * @dataProvider submitted_preferences_data + */ + public function test_submitted_preferences_are_validated($location, $vf_location, $number, $expected_location, $expected_vf_location, $expected_number) + { + $this->user->data = array( + 'user_rt_enable' => 1, + 'user_rt_location' => 'RT_BOTTOM', + 'user_rt_viewforum_location' => 'RT_TOP', + 'user_rt_number' => 5, + 'user_rt_sort_start_time' => 0, + 'user_rt_unread_only' => 0, + ); + + $submitted = array('rt_location' => $location, 'rt_viewforum_location' => $vf_location, 'rt_number' => $number); + $this->request->method('variable') + ->willReturnCallback(function ($var, $default) use ($submitted) { + return $submitted[$var] ?? $default; + }); + + $this->set_listener(); + + $event = new \phpbb\event\data(array('data' => array(), 'submit' => true)); + $this->listener->ucp_prefs_get_data($event); + + $this->assertSame($expected_location, $event['data']['rt_location']); + $this->assertSame($expected_vf_location, $event['data']['rt_viewforum_location']); + $this->assertSame($expected_number, $event['data']['rt_number']); + } + + /** + * A stored location that is itself invalid falls back to the board default (#198). + */ + public function test_invalid_stored_location_falls_back_to_board_default() + { + $this->config['rt_location'] = 'RT_SIDE'; + $this->user->data = array( + 'user_rt_enable' => 1, + 'user_rt_location' => 'GARBAGE', + 'user_rt_viewforum_location' => 'RT_TOP', + 'user_rt_number' => 5, + 'user_rt_sort_start_time' => 0, + 'user_rt_unread_only' => 0, + ); + + $this->request->method('variable') + ->willReturnCallback(function ($var, $default) { + return $var === 'rt_location' ? 'RT_EVIL' : $default; + }); + + $this->set_listener(); + + $event = new \phpbb\event\data(array('data' => array(), 'submit' => true)); + $this->listener->ucp_prefs_get_data($event); + + $this->assertSame('RT_SIDE', $event['data']['rt_location']); + } + public function test_ucp_prefs_get_data_on_submit() { $this->user->data = array( diff --git a/tests/tests.md b/tests/tests.md index 45a7582..664892c 100644 --- a/tests/tests.md +++ b/tests/tests.md @@ -88,7 +88,7 @@ It creates the listener with mocked dependencies — a `config` object with real `event/ucp_listener.php` lets registered users customise their own Recent Topics experience. They can choose how many topics to show, where to position the block, whether to show only unread topics, and so on. The listener hooks into the UCP display-preferences page to show those settings and save them. There are three handlers: -- `ucp_prefs_get_data` — runs on page load AND on form submit; builds the data array and (only on page load) renders the UCP template block +- `ucp_prefs_get_data` — runs on page load AND on form submit; builds the data array, keeping the locations within their allowed options and the number within 1–999, and (only on page load) renders the UCP template block - `ucp_prefs_set_data` — maps the submitted form fields to the SQL column names used in `phpbb_users` - `ucp_register_set_data` — runs on `core.ucp_register_register_after`, after the new account is inserted, and writes the global config defaults to that user's row @@ -100,6 +100,8 @@ Creates the listener with mocks for auth, config, request, template, user, langu | `test_getSubscribedEvents` | Verifies the 3 expected event subscriptions | Accidentally removed subscription stops the UCP page from working | | `test_ucp_prefs_set_data` | Submits 5 preference fields | Each `data['rt_*']` field must map to the correct `sql_ary['user_rt_*']` column; a mismatch means preferences silently fail to save | | `test_ucp_prefs_get_data_no_submit` | Page load (submit = false) | Must: merge user DB values into `data`, call `add_lang()`, call `template->assign_vars()` | +| `test_submitted_preferences_are_validated` | Data provider: valid values; unknown location; `RT_SIDE` as viewforum location; numbers 100000, 0 and -5 | Locations outside the allowed options fall back to the user's stored value; the number is clamped to 1–999 (#198) | +| `test_invalid_stored_location_falls_back_to_board_default` | Submitted and stored locations both invalid | The board default `rt_location` is used (#198) | | `test_ucp_prefs_get_data_on_submit` | Form submit (submit = true) | Must: merge data; must NOT call `template->assign_vars()` — template must only be touched on page load, not on form processing | | `test_register_defaults_run_after_the_account_exists` | Event map | `ucp_register_set_data` must be on `core.ucp_register_register_after` (fires after `user_add()`, carries `user_id`) and not on `core.ucp_register_data_after` (fires during validation, no `user_id`, so the UPDATE hit `user_id = 0`; #196) | | `test_ucp_register_set_data` | New user registration (user_id = 3) | Must call `sql_build_array('UPDATE', ...)` with all 5 default values, then `sql_query()` with `WHERE user_id = 3` — verified by mock expectations on the db object |