From 1599e156a4d80ec05ee1dcfe6931ed456ce1e1a6 Mon Sep 17 00:00:00 2001 From: William Jacoby Date: Mon, 12 Oct 2026 13:12:11 -0500 Subject: [PATCH] Act on the real article name after a confirm box confirm_box() posts back to an address whose path is encoded twice, so an article called "Some Article" arrived as "Some%20Article" and matched nothing. Deleting an article and switching one off reported success but changed nothing, and deleting an uploaded file said the file did not exist, for every article name containing a space. The handlers now use the name the box carried in a hidden field. Co-Authored-By: Claude Sonnet 5.5 --- config/services.yml | 1 + tests/unit/confirm_article_test.php | 53 +++++++++++++++++++++++++++++ wiki/delete.php | 9 ++++- wiki/edit.php | 2 ++ wiki/files.php | 9 ++++- wiki/functions.php | 18 ++++++++++ 6 files changed, 90 insertions(+), 2 deletions(-) create mode 100644 tests/unit/confirm_article_test.php diff --git a/config/services.yml b/config/services.yml index 1ede67a..00dd88d 100644 --- a/config/services.yml +++ b/config/services.yml @@ -118,6 +118,7 @@ services: - '@user' - '%tables.wiki.article%' - '@phpbbmodders.wiki.files' + - '@request' phpbbmodders.wiki.view: class: phpbbmodders\wiki\wiki\view arguments: diff --git a/tests/unit/confirm_article_test.php b/tests/unit/confirm_article_test.php new file mode 100644 index 0000000..2889e6e --- /dev/null +++ b/tests/unit/confirm_article_test.php @@ -0,0 +1,53 @@ +createMock(\phpbb\request\request_interface::class); + $request->method('variable')->willReturnCallback(function ($name, $default, $multibyte) use ($posted) { + $this->assertSame('article', $name); + $this->assertTrue($multibyte, 'names can hold any character'); + + return $posted ?? $default; + }); + + return $request; + } + + public function test_the_name_the_box_carried_wins_over_the_double_encoded_route_name() + { + $this->assertSame('Some Article', functions::confirmed_article($this->request('Some Article'), 'Some%20Article')); + } + + public function test_before_the_box_the_route_name_is_used() + { + $this->assertSame('Some Article', functions::confirmed_article($this->request(null), 'Some Article')); + } + + public function test_the_html_escaping_of_the_posted_value_is_undone() + { + // request->variable() escapes what it returns; article names are stored as typed + $this->assertSame('Tom & "Jerry"', functions::confirmed_article($this->request('Tom & "Jerry"'), 'x')); + } + + public function test_unicode_names_survive() + { + $this->assertSame('Übersicht ✓', functions::confirmed_article($this->request('Übersicht ✓'), 'x')); + } +} diff --git a/wiki/delete.php b/wiki/delete.php index f76dfb4..b6a6b14 100644 --- a/wiki/delete.php +++ b/wiki/delete.php @@ -22,6 +22,9 @@ class delete extends \phpbbmodders\wiki\wiki\functions /** @var \phpbb\controller\helper */ protected $helper; + /** @var \phpbb\request\request */ + protected $request; + /** @var \parse_message */ protected $message_parser; @@ -44,10 +47,12 @@ class delete extends \phpbbmodders\wiki\wiki\functions * @param \phpbb\user $user User object * @param string $article_table * @param \phpbbmodders\wiki\wiki\files $files Uploaded files + * @param \phpbb\request\request $request Request object */ - public function __construct(\phpbb\auth\auth $auth, \phpbb\db\driver\driver_interface $db, \phpbb\controller\helper $helper, \phpbb\user $user, $article_table, \phpbbmodders\wiki\wiki\files $files) + public function __construct(\phpbb\auth\auth $auth, \phpbb\db\driver\driver_interface $db, \phpbb\controller\helper $helper, \phpbb\user $user, $article_table, \phpbbmodders\wiki\wiki\files $files, \phpbb\request\request $request) { $this->files = $files; + $this->request = $request; $this->auth = $auth; $this->db = $db; $this->helper = $helper; @@ -112,6 +117,8 @@ public function article($article) if (confirm_box(true)) { + $article = self::confirmed_article($this->request, $article); + $sql = 'DELETE FROM ' . $this->article_table . " WHERE article_url = '" . $this->db->sql_escape($article) . "'"; $this->db->sql_query($sql); diff --git a/wiki/edit.php b/wiki/edit.php index 55efca8..0850f29 100644 --- a/wiki/edit.php +++ b/wiki/edit.php @@ -132,6 +132,8 @@ public function deactivate($article) if (confirm_box(true)) { + $article = self::confirmed_article($this->request, $article); + // Set all versions to not approved $sql = 'UPDATE ' . $this->article_table . " SET article_approved = 0 diff --git a/wiki/files.php b/wiki/files.php index 64470a0..9289ba8 100644 --- a/wiki/files.php +++ b/wiki/files.php @@ -491,6 +491,13 @@ public function serve($article_url, $file_id) */ public function delete($article_url, $file_id) { + // confirm_box(true) can only be asked once per request + $confirmed = confirm_box(true); + if ($confirmed) + { + $article_url = functions::confirmed_article($this->request, $article_url); + } + $file = $this->get_file($article_url, $file_id); if (!$file) { @@ -504,7 +511,7 @@ public function delete($article_url, $file_id) $back = $this->helper->route('phpbbmodders_wiki_article', array('article' => $article_url)); - if (confirm_box(true)) + if ($confirmed) { $this->remove_files(array($file)); trigger_error($this->user->lang('WIKI_FILE_DELETED', $file['file_name']) . '

' . $this->user->lang('BACK_TO_ARTICLE') . ''); diff --git a/wiki/functions.php b/wiki/functions.php index 7b1e70a..62ecde3 100644 --- a/wiki/functions.php +++ b/wiki/functions.php @@ -60,6 +60,24 @@ protected function setup_parser() } } + /** + * The article name to act on once a confirm box has been answered. + * + * confirm_box() posts back to a page address whose path is encoded a second + * time, so "Some Article" comes back from the route as "Some%20Article" and + * matches nothing. The name the box carried in a hidden field is the real one; + * on the first request, before the box, there is no such field and the route's + * name is used. + * + * @param \phpbb\request\request_interface $request + * @param string $article Name from the route + * @return string + */ + public static function confirmed_article(\phpbb\request\request_interface $request, $article) + { + return htmlspecialchars_decode($request->variable('article', $article, true), ENT_COMPAT); + } + /** * * @param int $id Version ID