Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Spaces in email addresses are accepted #3

Closed
snoyberg opened this issue Feb 1, 2013 · 6 comments
Closed

Spaces in email addresses are accepted #3

snoyberg opened this issue Feb 1, 2013 · 6 comments
Assignees

Comments

@snoyberg
Copy link

snoyberg commented Feb 1, 2013

Example: michael @snoyman.com is an accepted email address. I ran into an issue where our validated email addresses were rejected by Amazon SES.

@Porges
Copy link
Owner

Porges commented Feb 1, 2013

Unfortunately this is a legitimate email address.

I think that stripping whitespace & comments should be permitted by the spec, so I'm thinking of doing that by default - very few people will need access to that information.

I have an experimental version that I can upload to GitHub in an hour or so.

By the way, I presume that you're using ByteStrings internally? If I swap out Parsec for Attoparsec I will also be able to drop the dependency on 'ranges'.

@ghost ghost assigned Porges Feb 1, 2013
@Porges
Copy link
Owner

Porges commented Feb 1, 2013

I've committed the experimental version in a new branch: d6083cb

Changes are:

  • Attoparsec is now the dependency instead of Parsec - ByteStrings are now the input
  • EmailAddress is now an abstract type, emailAddress is its smart constructor
  • comments and whitespace are dropped from the input email as it is parsed into an EmailAddress
  • there is a utility method canonicalizeEmail that will give you an email as a ByteString without comments or whitespace (if it is valid)
  • the Ranges dependency has been removed

Please let me know if this suits your use case, or if the dependency on Attoparsec is problematic. In the mean time I'm going make a proper test suite before I merge this back in. This will be a new major version as it is backwards-incompatible.

@Porges
Copy link
Owner

Porges commented Feb 1, 2013

Example:

 > canonicalizeEmail $ BS.pack "michael @snoyman.com"
Just "michael@snoyman.com"

@snoyberg
Copy link
Author

snoyberg commented Feb 2, 2013

This seems to work perfectly, thanks! FWIW, I'm actually using Text, not ByteString, but I can handle character encoding issues.

Thank you for getting this patch out so quickly.

@snoyberg snoyberg closed this as completed Feb 2, 2013
@Porges
Copy link
Owner

Porges commented Feb 2, 2013

Yes, I'm using ByteString instead of Text to indicate that it's ASCII. It also avoids putting a dependency on Text. (For the benefit of anyone else who might read this: you'll need to convert it using decodeUtf8.)

There should really be a separate type for this purpose...

I'm not sure if you found it already, but this version is on Hackage as email-validate-1.0.0.

@snoyberg
Copy link
Author

snoyberg commented Feb 3, 2013

Yep, I've found it and am already using it :).

Yes, the age-old ASCII issue. I tried to address this once with an Ascii newtype wrapper, but it was such a pain and there was so much pushback that I gave up and I just do exactly what you did here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants