aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
-rw-r--r--phpBB/config/default/container/services_text_formatter.yml5
-rw-r--r--phpBB/includes/acp/acp_bbcodes.php19
-rw-r--r--phpBB/language/en/acp/posting.php3
-rw-r--r--phpBB/phpbb/textformatter/acp_utils_interface.php38
-rw-r--r--phpBB/phpbb/textformatter/s9e/acp_utils.php67
-rw-r--r--tests/functional/acp_bbcodes_test.php40
-rw-r--r--tests/text_formatter/s9e/acp_utils_test.php79
7 files changed, 246 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..9583f9a869 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'] === 'unsafe');
- if (!$warn_text && !check_form_key($form_key))
+ if ($bbcode_info['status'] === 'invalid_definition')
+ {
+ trigger_error($user->lang['BBCODE_INVALID'] . adm_back_link($this->u_action), E_USER_WARNING);
+ }
+ if ($bbcode_info['status'] === 'invalid_template')
+ {
+ trigger_error($user->lang['BBCODE_INVALID_TEMPLATE'] . 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..d1e3de9989
--- /dev/null
+++ b/phpBB/phpbb/textformatter/acp_utils_interface.php
@@ -0,0 +1,38 @@
+<?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
+{
+ /**
+ * Analyse given BBCode definition for issues and safeness
+ *
+ * Required elements in the return array:
+ * - status:
+ * - "safe" The BBCode is valid and can be safely used by anyone.
+ * - "unsafe" The BBCode is valid but may be unsafe to use.
+ * - "invalid_definition" There is an issue with the definition.
+ * - "invalid_template" There is an issue with the template.
+ *
+ * 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..981fa60813
--- /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' => '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'] = '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())) ? 'invalid_template' : '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"
+ ]
+ ],
+ ];
+ }
+}