[user_accounts] Revert #2018 - Sanitization before validation of emai…

…l falsifies validity (#8137) When an email address is entered with invalid characters, the invalid characters are currently stripped before the address is validated, causing the validation to improperly pass when it should fail for email addresses such as "[email protected]>". The sanitation pass turns that into "[email protected]" before it's validated, which returns "true" despite the address entered being invalid. This also removes an old check for < > and " because the new check offers a clearer error message and covers a broader range of characters we don't want in emails (because of escaping issues).
aces · Jul 14, 2022 · b93f790 · b93f790
1 parent 2e32249
commit b93f790
Show file tree

Hide file tree

Showing 2 changed files with 13 additions and 19 deletions.
diff --git a/modules/login/php/requestaccount.class.inc b/modules/login/php/requestaccount.class.inc
@@ -133,6 +133,12 @@ class RequestAccount extends \NDB_Form
         }
         if (empty($values['from'])) {
             $errors['from'] = "Please provide a valid email!";
+        } else if (preg_match('/(<|>|"|&)/', $_REQUEST['from'])) {
+            // Although some of these characters are legal in emails, due to the
+            // current HTML escaping method, it is better to reject email
+            // addresses containing them
+            $errors['from'] =  'Email address can not contain the following' .
+                ' characters: <,>,& and "';
         } else if (!filter_var($_REQUEST['from'], FILTER_VALIDATE_EMAIL) ) {
             $errors['from'] = 'Please provide a valid email!';
         }
@@ -144,22 +150,6 @@ class RequestAccount extends \NDB_Form
             $errors['project'] = "Please choose a project!";
         }
 
-        // I don't think this is required because insert uses prepared statements,
-        // but I'm keeping it for now because it doesn't do any harm (unlike the
-        // len > 3 check). Little Bobby Tables will just have to sign
-        // up using a table alias. - Dave
-        //
-        // For each fields, check if quotes or if some HTML/PHP
-        // tags have been entered
-        foreach ($values as $field => $value) {
-            if (preg_match('/["]/', html_entity_decode($value))) {
-                $errors[$field] = "You can't use quotes in $field";
-            }
-            if (strlen($value) > strlen(strip_tags($value))) {
-                $errors[$field] = "You can't use tags in $field";
-            }
-        }
-
         return $errors;
     }
 

diff --git a/modules/user_accounts/php/edit_user.class.inc b/modules/user_accounts/php/edit_user.class.inc
@@ -1287,9 +1287,13 @@ class Edit_User extends \NDB_Form
      */
     private function _getEmailError(\Database $DB, string $email): ?string
     {
-        // remove illegal characters
-        $email = filter_var($email, FILTER_SANITIZE_EMAIL);
-        if (!filter_var($email, FILTER_VALIDATE_EMAIL)) {
+        if (preg_match('/(<|>|"|&)/', $email)) {
+            // Although some of these characters are legal in emails, due to the
+            // current HTML escaping method, it is better to reject email
+            // addresses containing them
+            return 'Email address can not contain any the following '.
+                'characters: <, >, & and "';
+        } elseif (!filter_var($email, FILTER_VALIDATE_EMAIL)) {
             // If email not syntactically valid
             return "Invalid email address";
         }