7 files changed, 262 insertions, 5 deletions
diff --git a/phpBB/config/default/container/services_text_formatter.yml b/phpBB/config/default/container/services_text_formatter.yml
index 07087cd4a9..4e4abf6564 100644
--- a/phpBB/config/default/container/services_text_formatter.yml
+++ b/phpBB/config/default/container/services_text_formatter.yml
@@ -4,6 +4,11 @@ parameters:
     text_formatter.cache.renderer.key: _text_formatter_renderer
 
 services:
+    text_formatter.acp_utils:
+        class: phpbb\textformatter\s9e\acp_utils
+        arguments:
+            - '@text_formatter.s9e.factory'
+
     text_formatter.cache:
         alias: text_formatter.s9e.factory
 
diff --git a/phpBB/includes/acp/acp_bbcodes.php b/phpBB/includes/acp/acp_bbcodes.php
index a67f3c54f9..84dbbf02ba 100644
--- a/phpBB/includes/acp/acp_bbcodes.php
+++ b/phpBB/includes/acp/acp_bbcodes.php
@@ -157,7 +157,7 @@ class acp_bbcodes
 				* @var	string	bbcode_tpl			The bbcode HTML replacement string
 				* @var	string	bbcode_helpline		The bbcode help line string
 				* @var	array	hidden_fields		Array of hidden fields for use when
-				*									submitting form when $warn_text is true
+				*									submitting form when $warn_unsafe is true
 				* @since 3.1.0-a3
 				*/
 				$vars = array(
@@ -172,14 +172,25 @@ class acp_bbcodes
 				);
 				extract($phpbb_dispatcher->trigger_event('core.acp_bbcodes_modify_create', compact($vars)));
 
-				$warn_text = preg_match('%<[^>]*\{text[\d]*\}[^>]*>%i', $bbcode_tpl);
+				$acp_utils   = $phpbb_container->get('text_formatter.acp_utils');
+				$bbcode_info = $acp_utils->analyse_bbcode($bbcode_match, $bbcode_tpl);
+				$warn_unsafe = ($bbcode_info['status'] === $acp_utils::BBCODE_STATUS_UNSAFE);
 
-				if (!$warn_text && !check_form_key($form_key))
+				if ($bbcode_info['status'] === $acp_utils::BBCODE_STATUS_INVALID_TEMPLATE)
+				{
+					trigger_error($user->lang['BBCODE_INVALID_TEMPLATE'] . adm_back_link($this->u_action), E_USER_WARNING);
+				}
+				if ($bbcode_info['status'] === $acp_utils::BBCODE_STATUS_INVALID_DEFINITION)
+				{
+					trigger_error($user->lang['BBCODE_INVALID'] . adm_back_link($this->u_action), E_USER_WARNING);
+				}
+
+				if (!$warn_unsafe && !check_form_key($form_key))
 				{
 					trigger_error($user->lang['FORM_INVALID'] . adm_back_link($this->u_action), E_USER_WARNING);
 				}
 
-				if (!$warn_text || confirm_box(true))
+				if (!$warn_unsafe || confirm_box(true))
 				{
 					$data = $this->build_regexp($bbcode_match, $bbcode_tpl);
 
diff --git a/phpBB/language/en/acp/posting.php b/phpBB/language/en/acp/posting.php
index 1667aa6011..3bff6b9185 100644
--- a/phpBB/language/en/acp/posting.php
+++ b/phpBB/language/en/acp/posting.php
@@ -42,7 +42,7 @@ $lang = array_merge($lang, array(
 	'ACP_BBCODES_EXPLAIN'		=> 'BBCode is a special implementation of HTML offering greater control over what and how something is displayed. From this page you can add, remove and edit custom BBCodes.',
 	'ADD_BBCODE'				=> 'Add a new BBCode',
 
-	'BBCODE_DANGER'				=> 'The BBCode you are trying to add seems to use a {TEXT} token inside a HTML attribute. This is a possible XSS security issue. Try using the more restrictive {SIMPLETEXT} or {INTTEXT} types instead. Only proceed if you understand the risks involved and you consider the use of {TEXT} absolutely unavoidable.',
+	'BBCODE_DANGER'				=> 'The BBCode you are trying to add seems unsafe. If the BBCode uses a {TEXT} token in a sensitive context, try using a more restrictive type instead. Only proceed if you understand the risks involved.',
 	'BBCODE_DANGER_PROCEED'		=> 'Proceed', //'I understand the risk',
 
 	'BBCODE_ADDED'				=> 'BBCode added successfully.',
@@ -56,6 +56,7 @@ $lang = array_merge($lang, array(
 
 	'BBCODE_INVALID_TAG_NAME'	=> 'The BBCode tag name that you selected already exists.',
 	'BBCODE_INVALID'			=> 'Your BBCode is constructed in an invalid form.',
+	'BBCODE_INVALID_TEMPLATE'	=> 'Your BBCode’s template is invalid.',
 	'BBCODE_TAG'				=> 'Tag',
 	'BBCODE_TAG_TOO_LONG'		=> 'The tag name you selected is too long.',
 	'BBCODE_TAG_DEF_TOO_LONG'	=> 'The tag definition that you have entered is too long, please shorten your tag definition.',
diff --git a/phpBB/phpbb/textformatter/acp_utils_interface.php b/phpBB/phpbb/textformatter/acp_utils_interface.php
new file mode 100644
index 0000000000..cdee56f19d
--- /dev/null
+++ b/phpBB/phpbb/textformatter/acp_utils_interface.php
@@ -0,0 +1,54 @@
+<?php
+/**
+*
+* This file is part of the phpBB Forum Software package.
+*
+* @copyright (c) phpBB Limited <https://www.phpbb.com>
+* @license GNU General Public License, version 2 (GPL-2.0)
+*
+* For full copyright and license information, please see
+* the docs/CREDITS.txt file.
+*
+*/
+
+namespace phpbb\textformatter;
+
+interface acp_utils_interface
+{
+	/**
+	* There is an issue with the definition
+	*/
+	const BBCODE_STATUS_INVALID_DEFINITION = 'invalid_definition';
+
+	/**
+	* There is an issue with the template
+	*/
+	const BBCODE_STATUS_INVALID_TEMPLATE = 'invalid_template';
+
+	/**
+	* The BBCode is valid and can be safely used by anyone
+	*/
+	const BBCODE_STATUS_SAFE = 'safe';
+
+	/**
+	* The BBCode is valid but may be unsafe to use
+	*/
+	const BBCODE_STATUS_UNSAFE = 'unsafe';
+
+	/**
+	* Analyse given BBCode definition for issues and safeness
+	*
+	* Required elements in the return array:
+	*  - status: see BBCODE_STATUS_* constants
+	*
+	* Optional elements in the return array:
+	*  - name:       Name of the BBCode based on the definition. Required if status is "safe".
+	*  - error_text: Textual description of the issue in plain text or as a L_* string.
+	*  - error_html: Visual description of the issue in HTML.
+	*
+	* @param  string $definition BBCode definition, e.g. [b]{TEXT}[/b]
+	* @param  string $template   BBCode template, e.g. <b>{TEXT}</b>
+	* @return array
+	*/
+	public function analyse_bbcode(string $definition, string $template): array;
+}
diff --git a/phpBB/phpbb/textformatter/s9e/acp_utils.php b/phpBB/phpbb/textformatter/s9e/acp_utils.php
new file mode 100644
index 0000000000..c4a668020e
--- /dev/null
+++ b/phpBB/phpbb/textformatter/s9e/acp_utils.php
@@ -0,0 +1,67 @@
+<?php
+/**
+*
+* This file is part of the phpBB Forum Software package.
+*
+* @copyright (c) phpBB Limited <https://www.phpbb.com>
+* @license GNU General Public License, version 2 (GPL-2.0)
+*
+* For full copyright and license information, please see
+* the docs/CREDITS.txt file.
+*
+*/
+
+namespace phpbb\textformatter\s9e;
+
+use phpbb\textformatter\acp_utils_interface;
+use s9e\TextFormatter\Configurator\Exceptions\UnsafeTemplateException;
+
+class acp_utils implements acp_utils_interface
+{
+	/**
+	* @var factory $factory
+	*/
+	protected $factory;
+
+	/**
+	* @param factory $factory
+	*/
+	public function __construct(factory $factory)
+	{
+		$this->factory = $factory;
+	}
+
+	/**
+	* {@inheritdoc}
+	*/
+	public function analyse_bbcode(string $definition, string $template): array
+	{
+		$configurator = $this->factory->get_configurator();
+		$return       = ['status' => self::BBCODE_STATUS_SAFE];
+
+		// Capture and normalize the BBCode name manually because there's no easy way to retrieve
+		// it in TextFormatter <= 2.x
+		if (preg_match('(\\[([-\\w]++))', $definition, $m))
+		{
+			$return['name'] = strtoupper($m[1]);
+		}
+
+		try
+		{
+			$configurator->BBCodes->addCustom($definition, $template);
+		}
+		catch (UnsafeTemplateException $e)
+		{
+			$return['status']     = self::BBCODE_STATUS_UNSAFE;
+			$return['error_text'] = $e->getMessage();
+			$return['error_html'] = $e->highlightNode('<span class="highlight">');
+		}
+		catch (\Exception $e)
+		{
+			$return['status']     = (preg_match('(xml|xpath|xsl)i', $e->getMessage())) ? self::BBCODE_STATUS_INVALID_TEMPLATE : self::BBCODE_STATUS_INVALID_DEFINITION;
+			$return['error_text'] = $e->getMessage();
+		}
+
+		return $return;
+	}
+}
diff --git a/tests/functional/acp_bbcodes_test.php b/tests/functional/acp_bbcodes_test.php
index 58681dfa07..cc6397fdfd 100644
--- a/tests/functional/acp_bbcodes_test.php
+++ b/tests/functional/acp_bbcodes_test.php
@@ -43,4 +43,44 @@ class phpbb_functional_acp_bbcodes_test extends phpbb_functional_test_case
 		$this->assertContains('<div>c</div>', $html);
 		$this->assertContains('<div>d</div>', $html);
 	}
+
+	/**
+	* @dataProvider get_bbcode_error_tests
+	*/
+	public function test_bbcode_error($match, $tpl, $error)
+	{
+		$this->login();
+		$this->admin_login();
+
+		$crawler = self::request('GET', 'adm/index.php?i=acp_bbcodes&sid=' . $this->sid . '&mode=bbcodes&action=add');
+		$form = $crawler->selectButton('Submit')->form([
+			'bbcode_match' => $match,
+			'bbcode_tpl'   => $tpl
+		]);
+		$crawler = self::submit($form);
+
+		$text = $crawler->filter('.errorbox')->text();
+		$this->assertStringContainsString($error, $text);
+	}
+
+	public function get_bbcode_error_tests()
+	{
+		return [
+			[
+				'XXX',
+				'',
+				'BBCode is constructed in an invalid form'
+			],
+			[
+				'[x]{TEXT}[/x]',
+				'<xsl:invalid',
+				'template is invalid'
+			],
+			[
+				'[x]{TEXT}[/x]',
+				'<script>{TEXT}</script>',
+				'unsafe'
+			],
+		];
+	}
 }
diff --git a/tests/text_formatter/s9e/acp_utils_test.php b/tests/text_formatter/s9e/acp_utils_test.php
new file mode 100644
index 0000000000..9d84924042
--- /dev/null
+++ b/tests/text_formatter/s9e/acp_utils_test.php
@@ -0,0 +1,79 @@
+<?php
+/**
+*
+* This file is part of the phpBB Forum Software package.
+*
+* @copyright (c) phpBB Limited <https://www.phpbb.com>
+* @license GNU General Public License, version 2 (GPL-2.0)
+*
+* For full copyright and license information, please see
+* the docs/CREDITS.txt file.
+*
+*/
+
+class phpbb_textformatter_s9e_acp_utils_test extends phpbb_test_case
+{
+	/**
+	* @dataProvider get_analyse_bbcode_tests
+	*/
+	public function test_analyse_bbcode($definition, $template, $expected)
+	{
+		$container = $this->get_test_case_helpers()->set_s9e_services();
+		$factory   = $container->get('text_formatter.s9e.factory');
+		$acp_utils = new \phpbb\textformatter\s9e\acp_utils($factory);
+		$actual    = $acp_utils->analyse_bbcode($definition, $template);
+
+		$this->assertEquals($expected, $actual);
+	}
+
+	public function get_analyse_bbcode_tests()
+	{
+		return [
+			[
+				'[x]{TEXT}[/x]',
+				'<b>{TEXT}</b>',
+				[
+					'status' => 'safe',
+					'name'   => 'X'
+				]
+			],
+			[
+				'[hr]',
+				'<hr>',
+				[
+					'status' => 'safe',
+					'name'   => 'HR'
+				]
+			],
+			[
+				'[x]{TEXT}[/x]',
+				'<script>{TEXT}</script>',
+				[
+					'status'     => 'unsafe',
+					'name'       => 'X',
+					'error_text' => 'Cannot allow unfiltered data in this context',
+					'error_html' => '&lt;script&gt;
+  <span class="highlight">&lt;xsl:apply-templates/&gt;</span>
+&lt;/script&gt;'
+				]
+			],
+			[
+				'???',
+				'<hr>',
+				[
+					'status'     => 'invalid_definition',
+					'error_text' => 'Cannot interpret the BBCode definition'
+				]
+			],
+			[
+				'[x]{TEXT}[/x]',
+				'<xsl:invalid',
+				[
+					'status'     => 'invalid_template',
+					'name'       => 'X',
+					'error_text' => "Invalid XSL: Couldn't find end of Start Tag invalid line 1\n"
+				]
+			],
+		];
+	}
+}