Fix conditions for MultiSelectOptionType
authorTim Düsterhus <duesterhus@woltlab.com>
Fri, 9 Jul 2021 14:58:42 +0000 (16:58 +0200)
committerTim Düsterhus <duesterhus@woltlab.com>
Mon, 9 Aug 2021 13:03:35 +0000 (15:03 +0200)
This cleans up the SQL conditions used for searching for users with a specific
selection and fixes the following issues:

- It avoids the use of `escapeString()` in favor of proper prepared statements.
- It avoids the use of `preg_quote()` to escape a regular expression for use in
  MySQL, which might not be safe.
- It fixes matching when the options are later reordered, as the saved value is
  not being normalized and instead reused the order of the options within the
  select.

The generated query does not look great, but is not really worse than the
regular expression either.

In the future it might be possible to migrate this option type to a JSON based
storage and to use `JSON_CONTAINS()`.

wcfsetup/install/files/lib/system/option/MultiSelectOptionType.class.php

index 8b4b4d7d540f0a1c3c178ac5341c47cff113a382..a327b4f4ed7365d5ea85d20e2d1c59b08a5d892e 100644 (file)
@@ -103,16 +103,23 @@ class MultiSelectOptionType extends SelectOptionType
         }
         $value = ArrayUtil::trim($value, false);
 
-        $value = \array_map(static function ($value) {
-            return escapeString(\preg_quote($value));
-        }, $value);
-
-        $conditions->add(
-            "option_value.userOption" . $option->optionID . " REGEXP '" . '(^|\n)' . \implode(
-                '\n([^\n]*\n)*',
-                $value
-            ) . '($|\n)' . "'"
-        );
+        foreach ($value as $entry) {
+            $escapedEntry = \addcslashes($entry, '%_');
+            $conditions->add(
+                "(
+                    option_value.userOption" . $option->optionID . " LIKE ?
+                    OR option_value.userOption" . $option->optionID . " LIKE ?
+                    OR option_value.userOption" . $option->optionID . " LIKE ?
+                    OR option_value.userOption" . $option->optionID . " = ?
+                )",
+                [
+                    "%\n{$escapedEntry}\n%",
+                    "%\n{$escapedEntry}",
+                    "{$escapedEntry}\n%",
+                    $entry,
+                ]
+            );
+        }
 
         return true;
     }
@@ -127,16 +134,23 @@ class MultiSelectOptionType extends SelectOptionType
         }
         $value = ArrayUtil::trim($value, false);
 
-        $value = \array_map(static function ($value) {
-            return escapeString(\preg_quote($value));
-        }, $value);
-
-        $userList->getConditionBuilder()->add(
-            "user_option_value.userOption" . $option->optionID . " REGEXP '" . '(^|\n)' . \implode(
-                '\n([^\n]*\n)*',
-                $value
-            ) . '($|\n)' . "'"
-        );
+        foreach ($value as $entry) {
+            $escapedEntry = \addcslashes($entry, '%_');
+            $userList->getConditionBuilder()->add(
+                "(
+                    user_option_value.userOption" . $option->optionID . " LIKE ?
+                    OR user_option_value.userOption" . $option->optionID . " LIKE ?
+                    OR user_option_value.userOption" . $option->optionID . " LIKE ?
+                    OR user_option_value.userOption" . $option->optionID . " = ?
+                )",
+                [
+                    "%\n{$escapedEntry}\n%",
+                    "%\n{$escapedEntry}",
+                    "{$escapedEntry}\n%",
+                    $entry,
+                ]
+            );
+        }
     }
 
     /**