Changed backend email validation to match discussed HTML5 standards. Add... #3805
Conversation
…Added new tests for email validation feature. close mozilla#3546
| start_stop.addStartupBatches(suite); | ||
|
|
||
| // this test verifies that a user who has only authenticated with | ||
| // an assertion from their primary, may not call restricted apis |
| @@ -18,10 +18,12 @@ httputils = require('./httputils.js'), | |||
| check = require('validator').check; | |||
|
|
|||
| var hostnameRegex = /^(([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])\.){3}([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])$|^(([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\-]*[a-zA-Z0-9])\.)*([A-Za-z0-9]|[A-Za-z0-9][A-Za-z0-9\-]*[A-Za-z0-9])$/; | |||
| // HTML5 email regex from: http://blog.gerv.net/2011/05/html5_email_address_regexp/ | |||
| var emailRegex = /^[a-zA-Z0-9.!#$%&'*+\/=?\^_`{|}~\-]+@[a-zA-Z0-9](?:[a-zA-Z0-9]{0,253}[a-zA-Z0-9])?(?:\.[a-zA-Z0-9](?:[a-zA-Z0-9]{0,253}[a-zA-Z0-9])?)*$/; | |||
There was a problem hiding this comment.
This is missing a dash after the @ symbol. @[a-zA-Z0-9](?:[a-zA-Z0-9-]... That's what's breaking the other test, since its testing /wsapi/address_info?email=test@test-domain.com
Add a testcase for test-foo@test-foo.test-test.com as well.
There was a problem hiding this comment.
Just to be clear, test-foo@test-foo.test-test.com is expected to fail, correct?
| var invalidEmail1 = 'test@-test.com'; | ||
| var invalidEmail2 = 'test@'; | ||
| var invalidEmail3 = 'test@.org'; | ||
| var invalidEmail4 = 'test-foo@test-foo.test-test.com'; |
There was a problem hiding this comment.
Huh? No, I didn't expect that to fail. That's a valid email address, isn't it?
There was a problem hiding this comment.
This is getting silly. First off, you are making arbitrary changes to the regex from the source you quoted.
There was a problem hiding this comment.
I see what is happening. I attempted to escape some characters in the regex to avoid warnings in the jshint test. I should have recognized that this would change the regex. I will remove the escape characters, but can you give me some guidance on the resulting jshint warnings? Are they ok to leave in?
There was a problem hiding this comment.
Yeah, sorry, my comment wasn't really helpful.
You can look at https://github.com/mozilla/browserid/blob/dev/resources/static/common/js/validation.js for an example of an another regex that is backslash-escaped properly for jshint.
But anyways, I don't really like that this PR will just substitute in yet another regex for the current one on the server side, and that we still have a different way on the client side. (If the server side is a strict superset of the client side test, I'm ok with that. But the client side check we have is already strict on the DNS aspects of the regex (max sizes) that I'd be concerned about). (Side note: gerv's regex is incorrect on those limits).
Also, not to put it on you, but having comprehensive tests of valid and invalid email addresses before changing either the server or client side (preferably shared) would be good. Since HTML5 browsers now implement input[type=email] elements, they have test sets already, and they'd be a good base for that.
There was a problem hiding this comment.
Good points. I don't suppose there is any pre-established pattern in the project for sharing JS utility libraries between client and server side? If you can point me to any examples, that would be great.
I'll look into some existing HTML5 email test sets, and see about basing tests off of them, More comprehensive testing would certainly be a benefit here.
There was a problem hiding this comment.
So, we have used browserify for some parts of the code, but that's not really used in a dynamic-build way.
I think for the purposes of getting to a consistent function for valid email patterns, we can live with clearly calling out where "if you change this here, you MUST change it there" and tests.
I started work on using the tests in gecko against a trivial module that can be swapped out in ./lib/validate. But it needs more work (and even those gecko tests have some quirks).
|
this has been lingering for a while, apologies @cacois. @jrgm or @seanmonstar, can you summarize with remaining next steps so we can either close this down or fix it up and merge it? 🍻 |
|
Current status is that I still don't expect |
|
@cacois you down for fixing up the code and tests? |
|
Absolutely. Sorry, been a busy week. I'll make sure the regex matches that On Mon, Sep 9, 2013 at 7:05 PM, Jared Hirsch notifications@github.comwrote:
|
|
@cacois no worries! thanks for contributing :-) |
|
@cacois i'm closing this to prevent stuff lingering, but please reopen once you've fixed the tests. 🍻 |
...ed new tests for email validation feature. close #3546