Skip to content

Commit 81a4244

Browse files
lauriiilauriii
committed
Issue #3274648 by nod_, Wim Leers: HTMLRestrictions::merge() and ::toGeneralHtmlSupportConfig() fail on allowed attribute values that can be interpreted as integers
(cherry picked from commit 94b5d57502c45cef2954de9885075d88b6ca56cf) (cherry picked from commit f7d93687171d120f797f8fb85e80b0aec10c2413) (cherry picked from commit 15be5c7fc1da3d5193fddb579516f06f3ef2e45e)
1 parent 93b76bb commit 81a4244

File tree

2 files changed

+123
-98
lines changed

2 files changed

+123
-98
lines changed

‎modules/ckeditor5/src/HTMLRestrictions.php‎

Lines changed: 66 additions & 98 deletions
Original file line numberDiff line numberDiff line change
@@ -693,6 +693,67 @@ public function doIntersect(HTMLRestrictions $other): HTMLRestrictions {
693693
return new self($intersection);
694694
}
695695

696+
/**
697+
* Merge arrays of allowed elements according to HTMLRestrictions rules.
698+
*
699+
* @param array $array1
700+
* The first array of allowed elements.
701+
* @param array $array2
702+
* The second array of allowed elements.
703+
*
704+
* @return array
705+
* Merged array of allowed elements.
706+
*/
707+
private static function mergeAllowedElementsLevel(array $array1, array $array2): array {
708+
$union = [];
709+
$array1_keys = array_keys($array1);
710+
$array2_keys = array_keys($array2);
711+
$common_keys = array_intersect($array1_keys, $array2_keys);
712+
if (count($common_keys) === 0) {
713+
// There are no keys in common, simply append the arrays.
714+
$union = $array1 + $array2;
715+
}
716+
else {
717+
// For all the distinct keys, append them to the result.
718+
$filter_keys = array_flip($common_keys);
719+
// Add all unique keys from $array1.
720+
$union += array_diff_key($array1, $filter_keys);
721+
// Add all unique keys from $array2.
722+
$union += array_diff_key($array2, $filter_keys);
723+
724+
// There are some keys in common that need to be merged.
725+
foreach ($common_keys as $key) {
726+
$value1 = $array1[$key];
727+
$value2 = $array2[$key];
728+
$value1_is_bool = is_bool($value1);
729+
$value2_is_bool = is_bool($value2);
730+
731+
// When both values are boolean, combine the two.
732+
if ($value1_is_bool && $value2_is_bool) {
733+
$union[$key] = $value1 || $value2;
734+
}
735+
// When only one value is a boolean, take the most permissive result:
736+
// - when the value it TRUE, keep TRUE as it is the most permissive
737+
// - when the value is FALSE, take the other value.
738+
elseif ($value1_is_bool) {
739+
$union[$key] = $value1 ?: $value2;
740+
}
741+
elseif ($value2_is_bool) {
742+
$union[$key] = $value2 ?: $value1;
743+
}
744+
// Process nested arrays, in this case it correspond to tag attributes
745+
// configuration.
746+
elseif (is_array($value1) && is_array($value2)) {
747+
$union[$key] = self::mergeAllowedElementsLevel($value1, $value2);
748+
}
749+
}
750+
}
751+
// Make sure the order of the union array matches the order of the keys in
752+
// the arrays provided.
753+
$keys_order = array_merge($array1_keys, $array2_keys);
754+
return array_merge(array_flip($keys_order), $union);
755+
}
756+
696757
/**
697758
* Computes set union of two HTML restrictions, with wildcard support.
698759
*
@@ -704,103 +765,7 @@ public function doIntersect(HTMLRestrictions $other): HTMLRestrictions {
704765
* are either allowed in $this or in $other.
705766
*/
706767
public function merge(HTMLRestrictions $other): HTMLRestrictions {
707-
$union = array_merge_recursive($this->elements, $other->elements);
708-
// When recursively merging elements arrays, unkeyed boolean values can
709-
// appear in attribute config arrays. This removes them.
710-
foreach ($union as $tag => $tag_config) {
711-
if (is_array($tag_config)) {
712-
// If the HTML tag restrictions for both operands were both booleans,
713-
// then the result of array_merge_recursive() is an array containing two
714-
// booleans (because it is designed for arrays, not for also merging
715-
// booleans) under the first two numeric keys: 0 and 1. This does not
716-
// match the structure expected of HTML restrictions. Combine the two
717-
// booleans.
718-
if (array_key_exists(0, $tag_config) && array_key_exists(1, $tag_config) && is_bool($tag_config[0]) && is_bool($tag_config[1])) {
719-
// Twice FALSE.
720-
if ($tag_config === [FALSE, FALSE]) {
721-
$union[$tag] = FALSE;
722-
}
723-
// Once or twice TRUE.
724-
else {
725-
$union[$tag] = TRUE;
726-
}
727-
continue;
728-
}
729-
730-
// If the HTML tag restrictions for only one of the two operands was a
731-
// boolean, then the result of array_merge_recursive() is an array
732-
// containing the complete contents of the non-boolean operand plus an
733-
// additional key-value pair with the first numeric key: 0.
734-
if (array_key_exists(0, $tag_config)) {
735-
// If the boolean was FALSE (meaning: "no attributes allowed"), then
736-
// the other operand's values should be used in an union: this yields
737-
// the most permissive result.
738-
if ($tag_config[0] === FALSE) {
739-
unset($union[$tag][0]);
740-
}
741-
// If the boolean was TRUE (meaning: "all attributes allowed"), then
742-
// the other operand's values should be ignored in an union: this
743-
// yields the most permissive result.
744-
elseif ($tag_config[0] === TRUE) {
745-
$union[$tag] = TRUE;
746-
}
747-
continue;
748-
}
749-
750-
// If the HTML tag restrictions are arrays for both operands, similar
751-
// logic needs to be applied to the attribute-level restrictions.
752-
foreach ($tag_config as $html_tag_attribute_name => $html_tag_attribute_restrictions) {
753-
if (is_bool($html_tag_attribute_restrictions)) {
754-
continue;
755-
}
756-
757-
if (array_key_exists(0, $html_tag_attribute_restrictions)) {
758-
// Special case: the global attribute `*` HTML tag.
759-
// @see https://html.spec.whatwg.org/multipage/dom.html#global-attributes
760-
// @see validateAllowedRestrictionsPhase2()
761-
// @see validateAllowedRestrictionsPhase4()
762-
if ($tag === '*') {
763-
assert(is_bool($html_tag_attribute_restrictions[0]) || is_bool($html_tag_attribute_restrictions[1]));
764-
// When both are boolean, pick the most permissive value.
765-
if (is_bool($html_tag_attribute_restrictions[0]) && isset($html_tag_attribute_restrictions[1]) && is_bool($html_tag_attribute_restrictions[1])) {
766-
$value = $html_tag_attribute_restrictions[0] || $html_tag_attribute_restrictions[1];
767-
}
768-
else {
769-
$value = is_bool($html_tag_attribute_restrictions[0])
770-
? $html_tag_attribute_restrictions[0]
771-
: $html_tag_attribute_restrictions[1];
772-
}
773-
$union[$tag][$html_tag_attribute_name] = $value;
774-
continue;
775-
}
776-
777-
// The "twice FALSE" case cannot occur for attributes, because
778-
// attribute restrictions either have "TRUE" (to indicate any value
779-
// is allowed for the attribute) or a list of allowed attribute
780-
// values. If there is a numeric key, then one of the two operands
781-
// must allow all attribute values (the "TRUE" case). Otherwise, an
782-
// array merge would have happened, and no numeric key would exist.
783-
// Therefore, this is always once or twice TRUE.
784-
// e.g.: <foo bar> and <foo bar>, or <foo bar> and <foo bar="baz">
785-
assert($html_tag_attribute_restrictions[0] === TRUE || $html_tag_attribute_restrictions[1] === TRUE);
786-
$union[$tag][$html_tag_attribute_name] = TRUE;
787-
}
788-
else {
789-
// Finally, when both operands list the same allowed attribute
790-
// values, then the result provided by array_merge_recursive() for
791-
// those allowed attribute values is an array containing two times
792-
// `TRUE` (because it is designed for arrays, not for also merging
793-
// booleans) under the first two numeric keys: 0 and 1.
794-
// e.g.: <foo bar="baz qux"> merged with <foo bar="baz quux">.
795-
foreach ($html_tag_attribute_restrictions as $allowed_attribute_value => $merged_result) {
796-
if ($merged_result === [0 => TRUE, 1 => TRUE]) {
797-
$union[$tag][$html_tag_attribute_name][$allowed_attribute_value] = TRUE;
798-
}
799-
}
800-
}
801-
}
802-
}
803-
}
768+
$union = self::mergeAllowedElementsLevel($this->elements, $other->elements);
804769

805770
// Special case: wildcard attributes, and the ability to define restrictions
806771
// for all concrete attributes matching them using:
@@ -1109,7 +1074,10 @@ public function toGeneralHtmlSupportConfig(): array {
11091074
// that this class expects to the `['en', 'fr']` structure that the
11101075
// GHS functionality in CKEditor 5 expects.
11111076
if (is_array($value)) {
1112-
$value = array_keys($value);
1077+
// Ensure that all values are strings, this is necessary since PHP
1078+
// transforms the "1" string into 1 the number when it is used as
1079+
// an array key.
1080+
$value = array_map('strval', array_keys($value));
11131081
}
11141082
// Drupal never allows style attributes due to security concerns.
11151083
// @see \Drupal\Component\Utility\Xss

‎modules/ckeditor5/tests/src/Unit/HTMLRestrictionsTest.php‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -294,6 +294,14 @@ public function providerConvenienceConstructors(): \Generator {
294294
'<a target class>',
295295
['a' => ['target' => TRUE, 'class' => TRUE]],
296296
];
297+
yield 'tag with allowed attribute value that happen to be numbers' => [
298+
'<ol type="1 A I">',
299+
['ol' => ['type' => [1 => TRUE, 'A' => TRUE, 'I' => TRUE]]],
300+
];
301+
yield 'tag with allowed attribute value that happen to be numbers (reversed)' => [
302+
'<ol type="I A 1">',
303+
['ol' => ['type' => ['I' => TRUE, 'A' => TRUE, 1 => TRUE]]],
304+
];
297305

298306
// Multiple tag cases.
299307
yield 'two tags' => [
@@ -682,6 +690,27 @@ public function providerRepresentations(): \Generator {
682690
],
683691
],
684692
];
693+
694+
yield '<ol type="1 A">' => [
695+
new HTMLRestrictions(['ol' => ['type' => ['1' => TRUE, 'A' => TRUE]]]),
696+
['<ol type="1 A">'],
697+
'<ol type="1 A">',
698+
[
699+
[
700+
'name' => 'ol',
701+
'attributes' => [
702+
[
703+
'key' => 'type',
704+
'value' => [
705+
'regexp' => [
706+
'pattern' => '/^(1|A)$/',
707+
],
708+
],
709+
],
710+
],
711+
],
712+
],
713+
];
685714
}
686715

687716
/**
@@ -906,6 +935,34 @@ public function providerOperands(): \Generator {
906935
'intersection' => 'a',
907936
'union' => 'b',
908937
];
938+
yield 'attribute restrictions are different: <ol type=*> vs <ol type="A">' => [
939+
'a' => new HTMLRestrictions(['ol' => ['type' => TRUE]]),
940+
'b' => new HTMLRestrictions(['ol' => ['type' => ['A' => TRUE]]]),
941+
'diff' => 'a',
942+
'intersection' => 'b',
943+
'union' => 'a',
944+
];
945+
yield 'attribute restrictions are different: <ol type=*> vs <ol type="A"> — vice versa' => [
946+
'b' => new HTMLRestrictions(['ol' => ['type' => ['A' => TRUE]]]),
947+
'a' => new HTMLRestrictions(['ol' => ['type' => TRUE]]),
948+
'diff' => HTMLRestrictions::emptySet(),
949+
'intersection' => 'a',
950+
'union' => 'b',
951+
];
952+
yield 'attribute restrictions are different: <ol type=*> vs <ol type="1">' => [
953+
'a' => new HTMLRestrictions(['ol' => ['type' => TRUE]]),
954+
'b' => new HTMLRestrictions(['ol' => ['type' => ['1' => TRUE]]]),
955+
'diff' => 'a',
956+
'intersection' => 'b',
957+
'union' => 'a',
958+
];
959+
yield 'attribute restrictions are different: <ol type=*> vs <ol type="1"> — vice versa' => [
960+
'b' => new HTMLRestrictions(['ol' => ['type' => ['1' => TRUE]]]),
961+
'a' => new HTMLRestrictions(['ol' => ['type' => TRUE]]),
962+
'diff' => HTMLRestrictions::emptySet(),
963+
'intersection' => 'a',
964+
'union' => 'b',
965+
];
909966

910967
// Complex cases.
911968
yield 'attribute restrictions are different: <a hreflang="en"> vs <strong>' => [

0 commit comments

Comments
 (0)