Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[User Accounts] Email encoding issue (Redmine10193) #2018

Merged
merged 2 commits into from
Jul 28, 2016
Merged

[User Accounts] Email encoding issue (Redmine10193) #2018

merged 2 commits into from
Jul 28, 2016

Conversation

stellarxo
Copy link
Contributor

image

@stellarxo stellarxo added the Bug PR or issue introducing/requiring bug fixes (not mutually exclusive with the Feature label) label Jul 22, 2016
@stellarxo stellarxo added this to the 16.1 milestone Jul 22, 2016
@codecov-io
Copy link

codecov-io commented Jul 22, 2016 •
edited
Loading

Current coverage is 100% (diff: 100%)

Merging #2018 into 16.1-dev will increase coverage by 85.90%

@@           16.1-dev   #2018   diff @@
=======================================
  Files           117      22      -95   
  Lines         19843    1281   -18562   
  Methods        1104       0    -1104   
  Messages          0       0            
  Branches          0       0            
=======================================
- Hits           2797    1281    -1516   
+ Misses        17046       0   -17046   
  Partials          0       0

Sunburst

Powered by Codecov. Last update 086e39c...18d018f

ridz1208
ridz1208 reviewed Jul 25, 2016
View reviewed changes
modules/user_accounts/php/NDB_Form_user_accounts.class.inc
@@ -922,6 +922,9 @@ class NDB_Form_User_Accounts extends NDB_Form
*/
private function _getEmailError($DB, $email)
{
// remove illegal characters
$email = filter_var($email, FILTER_SANITIZE_EMAIL);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FILTER_SANITIZE_EMAIL removes illegal characters but does not fix encoding issues as described in the redmine ticket

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The email stays the same and it solves this issue. Are there any reasons why this shouldn't be used?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@driusan I hate to say I was right but... I was right.

This addition breaks email addresses that contain HTML special chars such as test@gmail.com>

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like at some point there might have been a bad merge, because here there is no if statement but in #8137 there is (resulting in filter_var being called twice)

@nirtiac nirtiac added PassedCodeReview Passed Manual Tests PR has undergone proper testing by at least one peer labels Jul 27, 2016
@driusan driusan merged commit 8e0ea13 into aces:16.1-dev Jul 28, 2016
ridz1208 added a commit that referenced this pull request Jul 12, 2022
@ridz1208
Revert "[User Accounts] Email encoding issue (Redmine10193) (#2018)"
39a71ce 
This reverts commit 8e0ea13.
ridz1208 added a commit to ridz1208/Loris that referenced this pull request Jul 12, 2022
@ridz1208
[user_accounts] Revert aces#2018 - Sanitization before validation of …
570d82c 
…email falsifies validity
driusan pushed a commit that referenced this pull request Jul 14, 2022
@ridz1208
[user_accounts] Revert #2018 - Sanitization before validation of emai…
b93f790 
…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 "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).
cmadjar pushed a commit to cmadjar/Loris that referenced this pull request Aug 10, 2022
@ridz1208 @cmadjar
[user_accounts] Revert aces#2018 - Sanitization before validation of …
6c5718b 
…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).
cmadjar pushed a commit to cmadjar/Loris that referenced this pull request Sep 20, 2022
@ridz1208 @cmadjar
[user_accounts] Revert aces#2018 - Sanitization before validation of …
80f2d94 
…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).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@driusan driusan driusan left review comments

@ridz1208 ridz1208 ridz1208 left review comments

Assignees

@ridz1208 ridz1208

Labels
Bug PR or issue introducing/requiring bug fixes (not mutually exclusive with the Feature label) Passed Manual Tests PR has undergone proper testing by at least one peer
Projects
None yet
Milestone
16.1
Development

Successfully merging this pull request may close these issues.
5 participants
@stellarxo @codecov-io @driusan @ridz1208 @nirtiac