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