firebase / firebase-admin-go

Firebase Admin Go SDK
Apache License 2.0
1.12k stars 239 forks source link

Email Validation func is Improper #351

Open mohammed90 opened 4 years ago

mohammed90 commented 4 years ago

[REQUIRED] Step 2: Describe your environment

[REQUIRED] Step 3: Describe the problem

Steps to reproduce:

The email validation func checks if the split over @ results in 2 parts:

https://github.com/firebase/firebase-admin-go/blob/32728b0fc9f9486276937d8f50cdf75485c1293c/auth/user_mgt.go#L461-L463

But this risks rejecting valid email address. These are all valid email addresses:

which the library will reject. The recommendation per Stavros Korokithakis is to check if there's an @ symbol and try to send an email to it. If they click the validation link, then it's a real address. So replace the prior snippet with:

Relevant Code:

Current code: https://github.com/firebase/firebase-admin-go/blob/32728b0fc9f9486276937d8f50cdf75485c1293c/auth/user_mgt.go#L461-L463

Suggested replacement:

if !strings.Contains(email, "@") {
    return fmt.Errorf("malformed email string: %q", email)
}

References:

google-oss-bot commented 4 years ago

This issue does not seem to follow the issue template. Make sure you provide all the required information.

hiranya911 commented 4 years ago

This is based off the same validation logic we use in our Node SDK.

export function isEmail(email: any): boolean {
  if (typeof email !== 'string') {
    return false;
  }
  // There must at least one character before the @ symbol and another after.
  const re = /^[^@]+@[^@]+$/;
  return re.test(email);
}

Yes, it is certainly not RFC compliant. But the kind of edge cases you've listed are also very rare in practice. Many of the widely used email providers simply won't allow such addresses. That's probably why these implementations have been able to get by for years without anybody pointing it out :)

Having said that, if there's a straightforward fix to the above code, we'd gladly accept it. From what I can see only those email addresses with extra @ signs in the username are the problematic cases.

hiranya911 commented 2 years ago

Found a fairly robust and exhaustive regex for email validation at https://html.spec.whatwg.org/multipage/input.html#valid-e-mail-address

We might want to update our email validation to use something like that.

mohammed90 commented 2 years ago

Found a fairly robust and exhaustive regex for email validation at https://html.spec.whatwg.org/multipage/input.html#valid-e-mail-address

We might want to update our email validation to use something like that.

Please check the note in the same link:

This requirement is a willful violation of RFC 5322, which defines a syntax for email addresses

RFC 5322 is the standard spec for email: https://datatracker.ietf.org/doc/html/rfc5322