From 19f48180dce79cbcd5c6a22f254e98e3d83104dd Mon Sep 17 00:00:00 2001 From: Nick Liu Date: Wed, 6 Oct 2021 13:03:08 -0500 Subject: [PATCH 1/3] =?UTF-8?q?Restore=20`htmlspecialchars()`=20for=20`e?= =?UTF-8?q?=5Fparse::filter(=E2=80=A6,=20'str')`?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes regression from 20882920a0b68937570264949512acc0c4841dbd where data would get inserted into the database with literal quotation marks, but e107 has always expected `"` and `'` to come directly from the database --- e107_handlers/e_parse_class.php | 16 ++++++++++++++-- e107_tests/tests/unit/e_parseTest.php | 15 +++++++++++++++ 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/e107_handlers/e_parse_class.php b/e107_handlers/e_parse_class.php index 867afdbb0..f93a1a69b 100644 --- a/e107_handlers/e_parse_class.php +++ b/e107_handlers/e_parse_class.php @@ -4800,8 +4800,17 @@ class e_parse /** * Filters/Validates using the PHP5 filter_var() method. + * * @param string|array $text - * @param string $type string str|int|email|url|w|wds|file + * @param string $type str|int|email|url|w|wds|file + * + * If the type is "str" (default), HTML tags are stripped, and quotation marks are escaped for + * HTML with the intention of making the string safe to use in both concatenated SQL queries and + * HTML code. + * + * Despite the intention, strings returned by this function should still be specified as values + * in SQL prepared statements or surrounded by {@see mysqli_real_escape_string()} if the string + * is to be written to the database. * @return string|boolean| array */ public function filter($text, $type = 'str', $validate = false) @@ -4859,7 +4868,10 @@ class e_parse { $filterTypes = array( 'int' => FILTER_SANITIZE_NUMBER_INT, - 'str' => function($input) { return strip_tags($input); }, + 'str' => function($input) + { + return htmlspecialchars(strip_tags($input), ENT_QUOTES); + }, 'email' => FILTER_SANITIZE_EMAIL, 'url' => FILTER_SANITIZE_URL, 'enc' => FILTER_SANITIZE_ENCODED diff --git a/e107_tests/tests/unit/e_parseTest.php b/e107_tests/tests/unit/e_parseTest.php index 18aedb6ec..76f5b9e68 100644 --- a/e107_tests/tests/unit/e_parseTest.php +++ b/e107_tests/tests/unit/e_parseTest.php @@ -2589,7 +2589,22 @@ Your browser does not support the audio tag. } + /** + * e107 v0.6.0 requires strings to be passed around with quotation marks escaped for HTML as a way to prevent + * both SQL injection and cross-site scripting. Although {@see e_parse::toDB()} is supposed to do that, some + * usages, specifically {@see e_front_model::sanitizeValue()} call {@see e_parse::filter()} instead. + * + * @version 2.3.1 + */ + public function testFilterStr() + { + $input = "\"e107's\""; + $expected = ""e107's""; + $actual = $this->tp->filter($input, 'str'); + + $this->assertEquals($expected, $actual); + } public function testCleanHtml() { From 2080c772c1be34651a880566e0bb336068368181 Mon Sep 17 00:00:00 2001 From: Nick Liu Date: Wed, 6 Oct 2021 12:30:39 -0500 Subject: [PATCH 2/3] Simplify `e_form::get_attributes()` Reduce code duplication without changing behavior Introduce helper `e_form::attributes()` to generate HTML attributes --- e107_handlers/form_handler.php | 153 ++++++++++----------------------- 1 file changed, 45 insertions(+), 108 deletions(-) diff --git a/e107_handlers/form_handler.php b/e107_handlers/form_handler.php index 399189d31..632a6ab8f 100644 --- a/e107_handlers/form_handler.php +++ b/e107_handlers/form_handler.php @@ -848,7 +848,7 @@ class e_form //never allow id in format name-value for text fields - return "get_attributes($options, $name). ' />'; + return "get_attributes($options, $name). ' />'; } @@ -3901,6 +3901,27 @@ var_dump($select_options);*/ $this->_tabindex_counter = $reset; } + /** + * Build a series of HTML attributes from the provided array + * + * @param array $attributes Key-value pairs of HTML attributes. The value must not be HTML-encoded. If the value is + * boolean true, the value will be set to the key (e.g. `['required' => true]` becomes + * "required='required'"). + * @return string The HTML attributes to concatenate inside an HTML tag + */ + private function attributes($attributes) + { + $stringifiedAttributes = []; + + foreach ($attributes as $key => $value) + { + if ($value === true) $value = $key; + if (!empty($value)) $stringifiedAttributes[] = $key . "='" . htmlspecialchars($value) . "'"; + } + + return count($stringifiedAttributes) > 0 ? " ".implode(" ", $stringifiedAttributes) : ""; + } + public function get_attributes($options, $name = '', $value = '') { $ret = ''; @@ -3909,7 +3930,7 @@ var_dump($select_options);*/ { if ($option !== 'other') { - $optval = htmlspecialchars(trim((string) $optval), ENT_COMPAT | ENT_HTML401, 'UTF-8', false); + $optval = html_entity_decode(trim((string) $optval)); } switch ($option) { @@ -3919,30 +3940,30 @@ var_dump($select_options);*/ break; case 'class': - if(!empty($optval)) - { - $ret .= " class='{$optval}'"; - } - break; - case 'size': - if($optval) - { - $ret .= " size='{$optval}'"; - } - break; - case 'title': - if($optval) - { - $ret .= " title='{$optval}'"; - } + case 'label': + case 'maxlength': + case 'wrap': + case 'autocomplete': + case 'pattern': + $ret .= $this->attributes([$option => $optval]); break; - case 'label': - if($optval) - { - $ret .= " label='{$optval}'"; + case 'readonly': + case 'multiple': + case 'selected': + case 'checked': + case 'disabled': + case 'required': + case 'autofocus': + $ret .= $this->attributes([$option => (bool) $optval]); + break; + + case 'placeholder': + if($optval) { + $optval = deftrue($optval, $optval); + $ret .= $this->attributes([$option => $optval]); } break; @@ -3958,91 +3979,7 @@ var_dump($select_options);*/ else { ++$this->_tabindex_counter; - $ret .= " tabindex='".$this->_tabindex_counter."'"; - } - break; - - case 'readonly': - if($optval) - { - $ret .= " readonly='readonly'"; - } - break; - - case 'multiple': - if($optval) - { - $ret .= " multiple='multiple'"; - } - break; - - case 'selected': - if($optval) - { - $ret .= " selected='selected'"; - } - break; - - case 'maxlength': - if($optval) - { - $ret .= " maxlength='{$optval}'"; - } - break; - - case 'checked': - if($optval) - { - $ret .= " checked='checked'"; - } - break; - - case 'disabled': - if($optval) - { - $ret .= " disabled='disabled'"; - } - break; - - case 'required': - if($optval) - { - $ret .= " required='required'"; - } - break; - - case 'autofocus': - if($optval) - { - $ret .= " autofocus='autofocus'"; - } - break; - - case 'placeholder': - if($optval) { - $optval = deftrue($optval, $optval); - $ret .= " placeholder='{$optval}'"; - } - break; - - case 'wrap': - if($optval) - { - $ret .= " wrap='{$optval}'"; - } - break; - - case 'autocomplete': - if($optval) - { - $ret .= " autocomplete='{$optval}'"; - } - break; - - case 'pattern': - if($optval) - { - $ret .= " pattern='{$optval}'"; + $ret .= $this->attributes([$option => $this->_tabindex_counter]); } break; @@ -4056,7 +3993,7 @@ var_dump($select_options);*/ default: if(strpos($option,'data-') === 0) { - $ret .= ' ' .$option."='{$optval}'"; + $ret .= $this->attributes([$option => $optval]); } break; } From 2fa9d10c575a5ac764d1c201d106f7bcbd47b774 Mon Sep 17 00:00:00 2001 From: Nick Liu Date: Wed, 6 Oct 2021 20:14:51 -0500 Subject: [PATCH 3/3] `e_form`: Handle attribute HTML entities properly for most form elements The result is a much more consistent form experience with less fear that some values put into an `e_form` method will break the web page. This commit covers the most common uses of `e_form` with HTML attribute quoting via `e_form::attributes()`. --- e107_handlers/form_handler.php | 768 +++++++++++++++++---------- e107_tests/tests/unit/e_formTest.php | 65 ++- 2 files changed, 527 insertions(+), 306 deletions(-) diff --git a/e107_handlers/form_handler.php b/e107_handlers/form_handler.php index 632a6ab8f..2d7c9491e 100644 --- a/e107_handlers/form_handler.php +++ b/e107_handlers/form_handler.php @@ -151,35 +151,46 @@ class e_form if(!empty($options['class'])) { - $class = "class='".$options['class']."'"; + $class = $options['class']; } else // default { - $class= "class='form-horizontal'"; + $class = "form-horizontal"; } if(isset($options['autocomplete'])) // leave as isset() { - $autoComplete = " autocomplete='".($options['autocomplete'] ? 'on' : 'off')."'"; + $autoComplete = $options['autocomplete'] ? 'on' : 'off'; } if($method === 'get' && strpos($target,'=')) { - list($url,$qry) = explode('?',$target); - $text = "\n
\n"; - - parse_str($qry,$m); - foreach($m as $k=>$v) + list($url, $qry) = explode('?', $target); + $text = "\nattributes([ + 'class' => $class, + 'action' => $url, + 'id' => $this->name2id($name), + 'method' => $method, + 'autocomplete' => $autoComplete, + ]) . ">\n"; + + parse_str($qry, $m); + foreach ($m as $k => $v) { - $text .= $this->hidden($k, $v); + $text .= $this->hidden($k, $v); } - - } - else + + } + else { - $target = str_replace('&', '&', $target); - $text = "\n\n"; + $text = "\nattributes([ + 'class' => $class, + 'action' => $target, + 'id' => $this->name2id($name), + 'method' => $method, + 'autocomplete' => $autoComplete, + ]) . ">\n"; } return $text; } @@ -630,37 +641,27 @@ class e_form */ public function carousel($name= 'e-carousel', $array=array(), $options = null) { - $interval = null; - $wrap = null; - $pause = null; $indicators = ''; $controls = ''; $act = varset($options['default'], 0); - - if(isset($options['wrap'])) - { - $wrap = 'data-wrap="'.$options['wrap'].'"'; - } - - if(isset($options['interval'])) - { - $interval = 'data-interval="'.$options['interval'].'"'; - } - - if(isset($options['pause'])) - { - $pause = 'data-pause="'.$options['pause'].'"'; - } $navigation = isset($options['navigation']) ? $options['navigation'] : true; $indicate = isset($options['indicators']) ? $options['indicators'] : true; - $start =' + $start = ' -