[user_accounts] Revert #2018 - Sanitization before validation of email falsifies validity #8137
Conversation
…email falsifies validity
ridz1208
added
the
Critical to release
|
@jstirling91 assigned you just to take a look see if it makes sense |
kongtiaowang
added
the
Passed manual tests
|
I think this might still fail if there is a The escaping must be removed for full coverage but it would make it subject to XSS Example: This is valid but subject to XSS |
ridz1208
added
State: Needs work
ridz1208
force-pushed
the
email_sanitize_before_validate
branch
from
417314e to
90b84a4
Compare
Co-authored-by: Dave MacFarlane <driusan@gmail.com>
| // 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 any the following' . |
There was a problem hiding this comment.
This is rarely getting triggered because of other validation. < is giving the error "You can't use tags in from" from line 165. " is giving the error "You can't use quotes in from" from the same part of the code. > is failing with "Please provide a valid email" from line 137.
But there's a bigger problem with quotes in that everything after the " gets truncated. I think there's still a reflected XSS attack despite this validation because, while the error is getting triggered, smarty is not escaping it when re-putting the user input into the form with the values that were entered by the user.
There was a problem hiding this comment.
This does catch "&" though.
There was a problem hiding this comment.
The quotation mark seems to be an existing issue which #7777 attempts to fix, not related to this PR.
…into email_sanitize_before_validate
ridz1208
removed
the
State: Needs work
There was a problem hiding this comment.
I tested request accounts and this now work. I'm unable to test user_accounts until this is pushed to 24 because of schema differences between 23 and 24 making it impossible to login in my sandbox, but the code is the same so I don't see any reason it wouldn't work.
The code looks good to me too. We lose the checking for < and " in firstname/lastname but the data is escaped by the Database class on insert, so it shouldn't open any vulnerabilities and the comment deleted says it was just there for a double check.
…email falsifies validity (aces#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 "test@gmail.com>". The sanitation pass turns that into "test@gmail.com" 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).
…email falsifies validity (aces#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 "test@gmail.com>". The sanitation pass turns that into "test@gmail.com" 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).
4 participants
Brief summary of changes
The issue came up when registering a user using the User Accounts -> add user functionality. If you select the "Make user name match email address" checkbox and your email address contains HTML special characters, you end up with a 500 error.
The email address failing here is:
test@gmail.com>This address does not passFILTER_VALIDATE_EMAILcheck but because of the sanitization step removed in this PR, the email actually being validated istest@gmail.comwhich is valid.This PR 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)