Make email validator case-insensitive #35790
Conversation
dimagimon
added
the
Risk: High
There was a problem hiding this comment.
Tests would be pretty nice given the complexity of the regex.
If you extracted the regex into a constant somewhere, I think it'd be pretty straightforward to set up utility tests, assert_properties.js is a nice example. But I defer to you on whether or not you want to increase the scope of this, this change looks fine.
| @@ -16,7 +16,7 @@ hqDefine("hqwebapp/js/bootstrap5/validators.ko", [ | |||
| ko.validation.rules['emailRFC2822'] = { | |||
| validator: function (val) { | |||
| if (val === undefined || val.length === 0) {return true;} // do separate validation for required | |||
| var re = /(?:[a-z0-9!#$%&'*+\/=?^_`{|}~-]+(?:\.[a-z0-9!#$%&'*+\/=?^_`{|}~-]+)*|"(?:[\x01-\x08\x0b\x0c\x0e-\x1f\x21\x23-\x5b\x5d-\x7f]|\\[\x01-\x09\x0b\x0c\x0e-\x7f])*")@(?:(?:[a-z0-9](?:[a-z0-9-]*[a-z0-9])?\.)+[a-z0-9](?:[a-z0-9-]*[a-z0-9])?|\[(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.){3}(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?|[a-z0-9-]*[a-z0-9]:(?:[\x01-\x08\x0b\x0c\x0e-\x1f\x21-\x5a\x53-\x7f]|\\[\x01-\x09\x0b\x0c\x0e-\x7f])+)\])$/; | |||
| var re = /(?:[A-Za-z0-9!#$%&'*+\/=?^_`{|}~-]+(?:\.[A-Za-z0-9!#$%&'*+\/=?^_`{|}~-]+)*|"(?:[\x01-\x08\x0b\x0c\x0e-\x1f\x21\x23-\x5b\x5d-\x7f]|\\[\x01-\x09\x0b\x0c\x0e-\x7f])*")@(?:(?:[A-Za-z0-9](?:[A-Za-z0-9-]*[A-Za-z0-9])?\.)+[A-Za-z0-9](?:[A-Za-z0-9-]*[A-Za-z0-9])?|\[(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.){3}(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?|[A-Za-z0-9-]*[A-Za-z0-9]:(?:[\x01-\x08\x0b\x0c\x0e-\x1f\x21-\x5a\x53-\x7f]|\\[\x01-\x09\x0b\x0c\x0e-\x7f])+)\])$/; | |||
There was a problem hiding this comment.
Alternatively, you could run test on value.toLowerCase() and make the regex a smidge simpler. But it's already a monstrosity, so this is just a suggestion.
@orangejenny That's totally reasonable. Worked on it a bit this afternoon, setup and basic assertions have been super simple, but having a bit of a time deciphering what this regex does relative to the actual email standard (with the examples given on Wikipedia, at least). It seems like it's a fair bit less restrictive in some senses, and then more strict in others. Would you think it makes more sense to write tests according to what the expression actually does and retain most of existing behavior, or write them according to what the standard "should be" and change the expression to fit? |
|
@nospame I'd write them to document the current behavior and not change the functionality. I'm curious what kinds of differences there are, but that's just idle curiosity talking. |
nospame
force-pushed
the
em/email-validator-case-sensitivity
branch
from
02df21f to
453f37a
Compare
|
@orangejenny In general it's just not as explicit as the actual standard, which is almost impossibly comprehensive. Moved the expression to constants c39df56, Added an |
| }); | ||
|
|
||
| it('should allow specified special characters in the local part', function () { | ||
| assert.ok(testEmail('.\x01!#$%&"\'*+/=?^_`{|}[email protected]')); |
There was a problem hiding this comment.
Agreed! I think this is one of the places where, strictly speaking, some of these characters should only be allowed based on context rather than all the time, e.g. a quote should always be paired. But the existing expression doesn't test for that.
Product Description
Addresses the issue of capital letters in an email address potentially causing a validation error when inviting a web user. Previously, any email address typed with the name portion ending in a capital letter or the domain portion containing any capital letters would be marked invalid, e.g.
Both of which are now valid.
Technical Summary
SAAS-15628
Updates the regular expression for email in
validators.koto allow capital letters. Also lowercases the email address as part of the web user invitation form'sclean_email, which while not required here because of theAdminInvitesUserFormValidator, makes it consistent with otherclean_emailmethods.Safety Assurance
Safety story
This is a small change to make email addresses validation more accurate and not block the entry of some valid email addresses. I think the risks are quite low.
Automated test coverage
We don't have tests for this monster of a regular expression.
QA Plan
Not planning it.
Rollback instructions
Labels & Review