fix: reject email with invisible chars - issue #2036

[meeraj257]

IsEmail() considers invisible characters in email as valid.

Expected Behavior:
IsEmail should deny the validation of the email when it contains an invisible character.

[codecov]

Codecov Report

Base: 100.00% // Head: 100.00% // No change to project coverage 👍

Coverage data is based on head (88eed73) compared to base (86a07ba).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##            master     #2049    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files          104       105     +1     
  Lines         2203      2318   +115     
  Branches       477       580   +103     
==========================================
+ Hits          2203      2318   +115     

Impacted Files	Coverage Δ
src/index.js	100.00% <100.00%> (ø)
src/lib/hasInvisibleChars.js	100.00% <100.00%> (ø)
src/lib/isEmail.js	100.00% <100.00%> (ø)
src/lib/isIP.js	100.00% <0.00%> (ø)
src/lib/isIn.js	100.00% <0.00%> (ø)
src/lib/trim.js	100.00% <0.00%> (ø)
src/lib/alpha.js	100.00% <0.00%> (ø)
src/lib/isBIC.js	100.00% <0.00%> (ø)
src/lib/isEAN.js	100.00% <0.00%> (ø)
src/lib/isHSL.js	100.00% <0.00%> (ø)
... and 85 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[meeraj257]

#2036

[tux-tn]

Hello @meeraj257 ,
Thank you for your PR. I have mainly two issues with your proposal:

• I double checked RFCs concerning email addresses and according to RFC 6531 and RFC 6532 introducing internationalized emails, all UTF-8 characters above U+007F are allowed in the user part of the email address. Since U+0080 -> U+009F are reserved for control codes, the existing regex is correctly starting from U+00AD and is RFC compliant.
• You added and exposed a new validator hasInvisibleChars without adding an entry to the README or writing tests. I'm not sure about the fact this new validator is needed or not.

[meeraj257]

@tux-tn - i saw your comments. The email RFC is broad and includes the whole range. I think having invisible characters in strings in general is very confusing and does cause issues like when you use it to send emails by typing it or you have blocked or allowed lists in applications. So i think its ok to not allow certain subset . we can add a flag to override this default behavior if anyone needs emails with invisible characters to be allowed. I think this change is actually beneficial and not taking away any useful functionality. Like to see inputs from issue owner @Scarus
Also I will make ReadMe updates once this discussion is resolved

[braaar]

I'm sure there are valid use cases for either case - allowing or disallowing invisible characters.

But since that is what we are looking to implement, this PR shouldn't be considered merely a bugfix as it is, but a breaking change.

I think we should add an option as @meeraj257 suggests, but that it should have a default value that preserves the current behaviour without breaking. The option can be available for when it's needed, and we can choose to break the feature by changing the default option if we wish to do so in a future release.

[meeraj257]

@braaar - thanks for your input. I like your suggestion. I can add an option for this new feature to be effective. This way the current behavior will be the default. @tux-tn - what are your thoughts?

[stuurman]

Hi, could this one be merged back? I also have the need for this fix? Thanks already!

[tux-tn]

@meeraj257 I agree on the fact that this can be useful as an option and to keep the current behavior as default. Do you want to work on the change?

[meeraj257]

@tux-tn - yes . I will work on this change this week. Sorry for delay in response. Was traveling on vacation past 2 weeks.

[chriskimgts]

When can this hidden characters validation check be merged? It could give us more validation room to play with...

[profnandaa]

Kindly let us schedule this for the release after the next one, coming out in 2 days.

[chriskimgts]

Thank you. Get Outlook for Android<https://aka.ms/AAb9ysg>

…

________________________________ From: Anthony Nandaa ***@***.***> Sent: Monday, January 30, 2023 4:47:38 AM To: validatorjs/validator.js ***@***.***> Cc: chriskimgts ***@***.***>; Comment ***@***.***> Subject: Re: [validatorjs/validator.js] fix: reject email with invisible chars - issue #2036 (PR #2049) Kindly let us schedule this for the release after the next one, coming out in 2 days. — Reply to this email directly, view it on GitHub<#2049 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/APV3GN7D4VYEKHTOVYCVTL3WU3XQVANCNFSM6AAAAAAQW7QJ5U>. You are receiving this because you commented.Message ID: ***@***.***>

[stuurman]

Hi, is it possible merge this back soon? I see it is already scheduled for some time? Could I help with something?

[braaar]

@meeraj257 – any progress with preserving the current behaviour as the default?

[stuurman]

any updates on this one?

[stuurman]

bump?

[stuurman]

@profnandaa @meeraj257
Is there any date that these proposed changes are planned? Or this is a not important change for the project?