From 531d9dfa1f0dddfe765a92ca40c77015091ecc5e Mon Sep 17 00:00:00 2001 From: JoshyPHP Date: Mon, 22 Jan 2018 03:34:47 +0100 Subject: [PATCH 1/2] [ticket/15531] Log malformed BBCodes PHPBB3-15531 --- .../container/services_text_formatter.yml | 1 + phpBB/language/en/acp/common.php | 1 + phpBB/phpbb/textformatter/s9e/factory.php | 44 +++++++++++++------ .../phpbb_test_case_helpers.php | 7 ++- tests/text_formatter/s9e/factory_test.php | 18 ++++++++ .../s9e/fixtures/malformed_bbcode.xml | 28 ++++++++++++ 6 files changed, 84 insertions(+), 15 deletions(-) create mode 100644 tests/text_formatter/s9e/fixtures/malformed_bbcode.xml diff --git a/phpBB/config/default/container/services_text_formatter.yml b/phpBB/config/default/container/services_text_formatter.yml index 74624ea4e4..07087cd4a9 100644 --- a/phpBB/config/default/container/services_text_formatter.yml +++ b/phpBB/config/default/container/services_text_formatter.yml @@ -39,6 +39,7 @@ services: - '@dispatcher' - '@config' - '@text_formatter.s9e.link_helper' + - '@log' - '%text_formatter.cache.dir%' - '%text_formatter.cache.parser.key%' - '%text_formatter.cache.renderer.key%' diff --git a/phpBB/language/en/acp/common.php b/phpBB/language/en/acp/common.php index de5039f047..0d5f6fee25 100644 --- a/phpBB/language/en/acp/common.php +++ b/phpBB/language/en/acp/common.php @@ -552,6 +552,7 @@ $lang = array_merge($lang, array( 'LOG_BBCODE_ADD' => 'Added new BBCode
» %s', 'LOG_BBCODE_EDIT' => 'Edited BBCode
» %s', 'LOG_BBCODE_DELETE' => 'Deleted BBCode
» %s', + 'LOG_BBCODE_CONFIGURATION_ERROR' => 'Error while configuring BBCode: %1$s
» %2$s', 'LOG_BOT_ADDED' => 'New bot added
» %s', 'LOG_BOT_DELETE' => 'Deleted bot
» %s', diff --git a/phpBB/phpbb/textformatter/s9e/factory.php b/phpBB/phpbb/textformatter/s9e/factory.php index 1e85856898..2b11a79192 100644 --- a/phpBB/phpbb/textformatter/s9e/factory.php +++ b/phpBB/phpbb/textformatter/s9e/factory.php @@ -13,6 +13,7 @@ namespace phpbb\textformatter\s9e; +use Exception; use s9e\TextFormatter\Configurator; use s9e\TextFormatter\Configurator\Items\AttributeFilters\RegexpFilter; use s9e\TextFormatter\Configurator\Items\UnsafeTemplate; @@ -131,6 +132,11 @@ class factory implements \phpbb\textformatter\cache_interface */ protected $dispatcher; + /** + * @var \phpbb\log\log_interface + */ + protected $log; + /** * Constructor * @@ -139,11 +145,12 @@ class factory implements \phpbb\textformatter\cache_interface * @param \phpbb\event\dispatcher_interface $dispatcher * @param \phpbb\config\config $config * @param \phpbb\textformatter\s9e\link_helper $link_helper + * @param \phpbb\log\log_interface $log * @param string $cache_dir Path to the cache dir * @param string $cache_key_parser Cache key used for the parser * @param string $cache_key_renderer Cache key used for the renderer */ - public function __construct(\phpbb\textformatter\data_access $data_access, \phpbb\cache\driver\driver_interface $cache, \phpbb\event\dispatcher_interface $dispatcher, \phpbb\config\config $config, \phpbb\textformatter\s9e\link_helper $link_helper, $cache_dir, $cache_key_parser, $cache_key_renderer) + public function __construct(\phpbb\textformatter\data_access $data_access, \phpbb\cache\driver\driver_interface $cache, \phpbb\event\dispatcher_interface $dispatcher, \phpbb\config\config $config, \phpbb\textformatter\s9e\link_helper $link_helper, \phpbb\log\log_interface $log, $cache_dir, $cache_key_parser, $cache_key_renderer) { $this->link_helper = $link_helper; $this->cache = $cache; @@ -153,6 +160,7 @@ class factory implements \phpbb\textformatter\cache_interface $this->config = $config; $this->data_access = $data_access; $this->dispatcher = $dispatcher; + $this->log = $log; } /** @@ -272,7 +280,7 @@ class factory implements \phpbb\textformatter\cache_interface // Add default BBCodes foreach ($this->get_default_bbcodes($configurator) as $bbcode) { - $configurator->BBCodes->addCustom($bbcode['usage'], new UnsafeTemplate($bbcode['template'])); + $this->add_bbcode($configurator, $bbcode['usage'], $bbcode['template']); } if (isset($configurator->tags['QUOTE'])) { @@ -299,17 +307,7 @@ class factory implements \phpbb\textformatter\cache_interface }, $row['bbcode_tpl'] ); - - try - { - $configurator->BBCodes->addCustom($row['bbcode_match'], new UnsafeTemplate($tpl)); - } - catch (\Exception $e) - { - /** - * @todo log an error? - */ - } + $this->add_bbcode($configurator, $row['bbcode_match'], $tpl); } // Load smilies @@ -418,6 +416,26 @@ class factory implements \phpbb\textformatter\cache_interface return array('parser' => $parser, 'renderer' => $renderer); } + /** + * Add a BBCode to given configurator + * + * @param Configurator $configurator + * @param string $usage + * @param string $template + * @return void + */ + protected function add_bbcode(Configurator $configurator, $usage, $template) + { + try + { + $configurator->BBCodes->addCustom($usage, new UnsafeTemplate($template)); + } + catch (Exception $e) + { + $this->log->add('critical', null, null, 'LOG_BBCODE_CONFIGURATION_ERROR', false, [$usage, $e->getMessage()]); + } + } + /** * Configure the Autolink / Autoemail plugins used to linkify text * diff --git a/tests/test_framework/phpbb_test_case_helpers.php b/tests/test_framework/phpbb_test_case_helpers.php index 7fb9a740b8..c792976b1e 100644 --- a/tests/test_framework/phpbb_test_case_helpers.php +++ b/tests/test_framework/phpbb_test_case_helpers.php @@ -385,7 +385,7 @@ class phpbb_test_case_helpers $mb = $this->test_case->getMockBuilder('phpbb\\textformatter\\data_access'); $mb->setMethods(array('get_bbcodes', 'get_censored_words', 'get_smilies', 'get_styles')); $mb->setConstructorArgs(array( - $this->test_case->getMock('phpbb\\db\\driver\\driver'), + $this->test_case->getMockBuilder('phpbb\\db\\driver\\driver')->getMock(), 'phpbb_bbcodes', 'phpbb_smilies', 'phpbb_styles', @@ -489,8 +489,11 @@ class phpbb_test_case_helpers $request = new phpbb_mock_request; } + // Get a log interface + $log = ($container->has('log')) ? $container->get('log') : $this->test_case->getMockBuilder('phpbb\\log\\log_interface')->getMock(); + // Create and register the text_formatter.s9e.factory service - $factory = new \phpbb\textformatter\s9e\factory($dal, $cache, $dispatcher, $config, new \phpbb\textformatter\s9e\link_helper, $cache_dir, $cache_key_parser, $cache_key_renderer); + $factory = new \phpbb\textformatter\s9e\factory($dal, $cache, $dispatcher, $config, new \phpbb\textformatter\s9e\link_helper, $log, $cache_dir, $cache_key_parser, $cache_key_renderer); $container->set('text_formatter.s9e.factory', $factory); // Create a user if none was provided, and add the common lang strings diff --git a/tests/text_formatter/s9e/factory_test.php b/tests/text_formatter/s9e/factory_test.php index d35330a975..0d780a19a9 100644 --- a/tests/text_formatter/s9e/factory_test.php +++ b/tests/text_formatter/s9e/factory_test.php @@ -56,6 +56,7 @@ class phpbb_textformatter_s9e_factory_test extends phpbb_database_test_case $this->dispatcher, new \phpbb\config\config(array('allowed_schemes_links' => 'http,https,ftp')), new \phpbb\textformatter\s9e\link_helper, + $this->getMockBuilder('phpbb\\log\\log_interface')->getMock(), $this->get_cache_dir(), '_foo_parser', '_foo_renderer' @@ -263,6 +264,23 @@ class phpbb_textformatter_s9e_factory_test extends phpbb_database_test_case $this->assertSame($expected, $renderer->render($parser->parse($original))); } + /** + * @testdox Logs malformed BBCodes + */ + public function test_malformed_bbcodes() + { + $log = $this->getMockBuilder('phpbb\\log\\log_interface')->getMock(); + $log->expects($this->once()) + ->method('add') + ->with('critical', null, null, 'LOG_BBCODE_CONFIGURATION_ERROR', false, ['[x !x]{TEXT}[/x]', 'Cannot interpret the BBCode definition']); + + $container = new phpbb_mock_container_builder; + $container->set('log', $log); + + $fixture = __DIR__ . '/fixtures/malformed_bbcode.xml'; + $this->get_test_case_helpers()->set_s9e_services($container, $fixture); + } + /** * @testdox get_configurator() triggers events before and after configuration */ diff --git a/tests/text_formatter/s9e/fixtures/malformed_bbcode.xml b/tests/text_formatter/s9e/fixtures/malformed_bbcode.xml new file mode 100644 index 0000000000..7e7aa1a39c --- /dev/null +++ b/tests/text_formatter/s9e/fixtures/malformed_bbcode.xml @@ -0,0 +1,28 @@ + + + + bbcode_id + bbcode_tag + bbcode_helpline + display_on_posting + bbcode_match + bbcode_tpl + first_pass_match + first_pass_replace + second_pass_match + second_pass_replace + + + 13 + x + + 1 + [x !x]{TEXT}[/x] + ... + + + + + +
+ From 3c5b3254d151c052070aa1e5ef2efef7d850f95b Mon Sep 17 00:00:00 2001 From: JoshyPHP Date: Mon, 22 Jan 2018 16:36:12 +0100 Subject: [PATCH 2/2] [ticket/15531] Workaround for false-positive sniff PHPBB3-15531 --- phpBB/phpbb/textformatter/s9e/factory.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/phpBB/phpbb/textformatter/s9e/factory.php b/phpBB/phpbb/textformatter/s9e/factory.php index 2b11a79192..c0bbc7b0e8 100644 --- a/phpBB/phpbb/textformatter/s9e/factory.php +++ b/phpBB/phpbb/textformatter/s9e/factory.php @@ -13,7 +13,6 @@ namespace phpbb\textformatter\s9e; -use Exception; use s9e\TextFormatter\Configurator; use s9e\TextFormatter\Configurator\Items\AttributeFilters\RegexpFilter; use s9e\TextFormatter\Configurator\Items\UnsafeTemplate; @@ -430,7 +429,7 @@ class factory implements \phpbb\textformatter\cache_interface { $configurator->BBCodes->addCustom($usage, new UnsafeTemplate($template)); } - catch (Exception $e) + catch (\Exception $e) { $this->log->add('critical', null, null, 'LOG_BBCODE_CONFIGURATION_ERROR', false, [$usage, $e->getMessage()]); }