NodeJS returns invalid for valid email address regex

Question

EDIT NodeJS route handler

// require() statements above
let error = {};

module.exports = {
  authorize: (req, res, next) => {
    const USERNAME  = req.body.username,
          PASSWORD  = req.body.password,
          SCOPES    = req.body.scopes;

    console.log(req.body);

    const SCOPE_LOOKUP = ['read', 'write', 'admin'];
    if(!VALIDATE_EMAIL(USERNAME)) {
      error.message = 'Invalid username.';
    }

    if(error.message) { return next(error) };
    return res.status(200).json(req.body);
  }
};

The code below runs on a NodeJS application I am working on. The email address const is populated with the contents of req.body.email and I am using Postman to make the API calls.

Running the code below and passing a valid email address will work as expected. However if I pass an invalid email address the code also works as expected, but when I pass in another valid email address I end up with Invalid email. This occurs with no restart of the server.

Is there an issue with execution order or scope, which I have missed?

const VALIDATE_EMAIL = email => {
    const EXP    = /^(([^<>()[\]\\.,;:\s@"]+(\.[^<>()[\]\\.,;:\s@"]+)*)|(".+"))@((\[[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}])|(([a-zA-Z\-0-9]+\.)+[a-zA-Z]{2,}))$/;
    const DOMAIN = '@example.com';
    const OUTPUT = (EXP.test(email) && email.indexOf(DOMAIN, email.length - DOMAIN.length) !== -1) ? true : false;
    return OUTPUT;
};
(() => {
    let error = {};
    const EMAIL = 'joebloggs@example.com';

    if(!VALIDATE_EMAIL(EMAIL)) {
        error.message = 'Invalid email.';
    }

    if(error.message) { console.log(error.message); return };
    console.log(EMAIL);
})();

You don't have any variables named `USERNAME` in the code you posted above. Can you give an example of an email you expect to pass the test? — Mike Cluck, May 23 '16 at 17:58
The code you posted is working as expected. Are you sure you don't have some whitespace in the email you're receiving on your server? `EMAIL = email.trim()` — Mike Cluck, May 23 '16 at 18:08
you're supposed to provide a failing case, not a passing one. — Evan Carroll, May 23 '16 at 18:11
@KarlBateman As far as anyone can tell, your code works just fine. Can you edit your question to have an example where it is **not** doing what you expect it to do? — Mike Cluck, May 23 '16 at 18:15
Sorry it's difficult as this is running on NodeJS, when I use Postman to modify the `req.body.email` at first it works, when I change to an invalid email address it works, but when I change it back to a valid one.. it keeps failing? I wondered if I am doing something wrong with my if else structure? — stackunderflow, May 23 '16 at 18:19
you should post the route handler since this seems to be specific that and not the above code :) — Seth, May 23 '16 at 18:24
Does the same problem occur if you use `var` instead of `const`? — theaccordance, May 23 '16 at 18:49
@KarlBateman, I've posted an answer for you. It was in fact a scope issue — Seth, May 23 '16 at 19:11

score 3 · Accepted Answer · answered May 23 '16 at 18:49

Your problem is that you're persisting your error message throughout the lifecycle of your application. Don't declare the error object outside the scope of the handler... You need to declare the error object within the request handler so that each request has a fresh error object (and subsequent error message).

module.exports = {
  authorize: (req, res, next) => {
    const error = {
      message: '',
      something: '',
      foo: ''
    };

    const USERNAME  = req.body.username,
          PASSWORD  = req.body.password,
          SCOPES    = req.body.scopes;

    console.log(req.body);

    const SCOPE_LOOKUP = ['read', 'write', 'admin'];
    if(!VALIDATE_EMAIL(USERNAME)) {
      error.message = 'Invalid username.';
    }

    if(error.message) { return next(error) };
    return res.status(200).json(req.body);
  }
};

@KarlBateman: `const` in JavaScript merely means that the reference to the value it holds is read-only. This doesn't provide immutability. It just ensures that the variable cannot be reassigned. If you want immutability, checkout https://facebook.github.io/immutable-js/. So in this case, const is semantically sufficient as you don't indent to reassign `error`. — Seth, May 23 '16 at 20:00

score 1 · Answer 2 · answered May 23 '16 at 18:16

1

On principle, don't ever do what you're doing (though it seems to work).. Use a library like email-addresses.

npm install email-addresses;

const parseEmail = require('email-addresses').parseOneAddress;
let parseResult = parseEmail(EMAIL);
console.log(parseResult);

That will output...

{ parts: 
   { name: null,
     address: 
      { name: 'addr-spec',
        tokens: 'joebloggs@example.com',
        semantic: 'joebloggs@example.com',
        children: [Object] },
     local: 
      { name: 'local-part',
        tokens: 'joebloggs',
        semantic: 'joebloggs',
        children: [Object] },
     domain: 
      { name: 'domain',
        tokens: 'example.com',
        semantic: 'example.com',
        children: [Object] } },
  name: null,
  address: 'joebloggs@example.com',
  local: 'joebloggs',
  domain: 'example.com' }

So if you want if you need the domain, get parseResult.domain

answered May 23 '16 at 18:16

Evan Carroll

78,363
46
261
468

What be the reason for not using a REGEX? I appreciate the pro's to using a library, but I am trying to reduce my amount of dependancies. I am also concerned about the `if else` structure, could that be causing an issue? – stackunderflow May 23 '16 at 18:21
Your regex is stupid and not tested. It's amateur to parse email addresses with a regex. You want to see what a regex that parses and validates an email address looks like.. Check out [this question](http://stackoverflow.com/q/2953197/124486). There is no reason to not use a dependency. It's code you don't have to maintain. Not using dependencies is a mark of being an amateur. Dependencies in node get pulled down locally. There is no dependency hell. It's code in the same project directory that you didn't have to write, that comes tested. – Evan Carroll May 23 '16 at 18:25
1

This is slightly off-topic, some constructive advice would be appreciated.. stating that ones code is stupid is not. I try to exercise caution when it comes to dependancies e.g. http://qz.com/646467/how-one-programmer-broke-the-internet-by-deleting-a-tiny-piece-of-code/.. but that's another discussion perhaps. – stackunderflow May 23 '16 at 18:30
@KarlBateman I'm showing you what your code doesn't catch for and you're professing an expertise that I don't have. Know your limits. You can use a library. I can use a library. Only a god can parse html and email addresses with regexes. There is just far too much skill involved. – Evan Carroll May 23 '16 at 18:35
BTW, that article is wrong. No programmer broke the internet. NPM broke their own repository. That's not a big deal though, they fixed it and the world moves on. You can even ship your `node_modules` in your package. Then nothing will break ever! – Evan Carroll May 23 '16 at 18:37
I will no doubt opt for a library at some point, however it covers the basics prior to sending a confirmation email. Thanks for your help :) – stackunderflow May 23 '16 at 18:37
If you just want to validate the email and not parse it, use [`isemail`](https://github.com/hapijs/isemail) which even has the ability to check dns stuff. – Evan Carroll May 23 '16 at 18:39
2

Thanks, your advice has been taken onboard.. less hostile ways of explaining your point, would be my advice to you. We don't know everything, and what I don't know.. I enjoy learning :) – stackunderflow May 23 '16 at 18:41
1

Constructively, if this application is anything other than a test of your abilities to write an HTTP API with node.js, then you really should consider @EvanCarroll's point. Whether you agree with the way he conveyed it or not. Sure his wording could have been a bit more elegant, but I believe the tone was trying to convey the fact that relying on email regex is potentially opening you up to security vulnerabilities. – Seth May 23 '16 at 19:09
I will be using @EvanCarroll's suggestions but want to get the basics of this thing put together quickly. Thank you both for your time and assistance. – stackunderflow May 23 '16 at 19:43
@EvanCarroll any suggestions on a password validation module? – stackunderflow May 23 '16 at 19:48
@KarlBateman against smpt/pop/imap? Generally the best bet is to follow the npm links to github and use the project with the most stars. If one project has 10x the stars, it's the dominant module in that space. – Evan Carroll May 23 '16 at 19:55
1

@EvanCarroll Thank you, will be avoiding regex :) – stackunderflow May 23 '16 at 19:57

NodeJS returns invalid for valid email address regex

2 Answers2