-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Accept internationalized emails. Fix uppercase and anchor bugs in email auth. #4695
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
base: main
Are you sure you want to change the base?
Changes from 4 commits
7e2347b
961c733
e50e9fb
434614b
b142069
6b3be5c
98c86c0
76ad235
7ee7fd1
2e478ec
838624d
69b41f7
34f969a
0688f92
c748460
8fc0d8e
3a6802a
a577b35
5f45b72
2f36a2f
116c1c3
a57d428
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| /** | ||
| * The syntax we accept is the HTML5 `input[type=email]` grammar (WHATWG HTML, | ||
| * "valid e-mail address"), widened to also accept Unicode letters, marks and | ||
| * digits so that internationalized addresses (RFC 6531) are not rejected. | ||
|
pullfrog[bot] marked this conversation as resolved.
Outdated
|
||
| * | ||
| * We match the HTML5 grammar because that is what browsers already enforce on | ||
| * `input[type=email]`, so the client and the server agree on what an address | ||
| * looks like. We widen it because the HTML5 grammar is deliberately ASCII-only | ||
| * (see whatwg/html#4562), which locks out anyone whose address contains, say, | ||
| * an umlaut. | ||
| * | ||
| * Syntax is all we check. Whether an address can actually receive mail is | ||
| * settled by sending it a verification email, not by a regex. | ||
| */ | ||
|
Comment on lines
+4
to
+16
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Waaaay to wordy IMO
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in fact, can we just use the zod validator? there's even a not in the previous implementation about it
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wanted to do html5 with unicode, and Zod options:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated the description. |
||
| const validEmailRegex = | ||
| /^[\p{L}\p{M}\p{N}.!#$%&'*+/=?^_`{|}~-]+@[\p{L}\p{N}](?:[\p{L}\p{M}\p{N}-]{0,61}[\p{L}\p{M}\p{N}])?(?:\.[\p{L}\p{N}](?:[\p{L}\p{M}\p{N}-]{0,61}[\p{L}\p{M}\p{N}])?)*$/u; | ||
|
pullfrog[bot] marked this conversation as resolved.
Outdated
|
||
|
|
||
| /** | ||
| * Upper bounds from RFC 5321 (4.5.3.1. Size Limits and Minimums), counted in | ||
| * octets because that is how the RFC counts them. | ||
| */ | ||
| const maxLocalPartOctets = 64; | ||
| const maxAddressOctets = 254; | ||
|
|
||
| /** | ||
| * Checks that `input` looks like an email address. | ||
| */ | ||
| export function isValidEmail(input: unknown): boolean { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO this function should have
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It actually does work with |
||
| if (typeof input !== "string") { | ||
| return false; | ||
| } | ||
|
|
||
| return ( | ||
| validEmailRegex.test(input) && | ||
| countOctets(input) <= maxAddressOctets && | ||
| countOctets(getLocalPart(input)) <= maxLocalPartOctets | ||
| ); | ||
| } | ||
|
|
||
| function getLocalPart(email: string): string { | ||
| return email.slice(0, email.lastIndexOf("@")); | ||
| } | ||
|
|
||
| function countOctets(text: string): number { | ||
| return new TextEncoder().encode(text).length; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1 @@ | ||
| /** | ||
| * NOTE: this is a placeholder file for future code exported for both runtimes. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Finally 😄 |
||
| */ | ||
| export { isValidEmail } from "./email"; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i wish this was a tested external library so that we didn't have to test it ourselves
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not too happy with it too. I've rejected
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Asked Claude to compare our solution to existing packages to make sure:
Last one passes most of the only because it does not support unicode at all so it doesn't have to deal with those cases. I myself used https://github.com/JoshData/python-email-validator as inspiration. Took some of safety features from there.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IK have to agree that it sucks that we have to implement this. Good thing is I don't see this changing much so maybe it is fine. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,109 @@ | ||
| import { describe, expect, it } from "vitest"; | ||
| import { isValidEmail } from "../src/email"; | ||
|
|
||
| describe("isValidEmail", () => { | ||
| it.each([ | ||
| "user@example.com", | ||
| "user.name+tag@example.co.uk", | ||
| "user_name@example.com", | ||
| "!#$%&'*+-/=?^_`{|}~@example.com", | ||
| "user@sub.domain.example.com", | ||
| "user@a.io", | ||
| // `input[type=email]` accepts a dotless domain, so we do too. | ||
| "user@localhost", | ||
| ])("accepts the ASCII address %j", (email) => { | ||
| expect(isValidEmail(email)).toBe(true); | ||
| }); | ||
|
|
||
| it.each([ | ||
| "", | ||
| "plainaddress", | ||
| "@example.com", | ||
| "user@", | ||
| "user@@example.com", | ||
| "user@-example.com", | ||
| "user@example-.com", | ||
| "user@exam ple.com", | ||
| "user name@example.com", | ||
| "user@example..com", | ||
| ])("rejects the malformed address %j", (email) => { | ||
| expect(isValidEmail(email)).toBe(false); | ||
| }); | ||
|
|
||
| it.each([null, undefined, 42, {}, ["user@example.com"]])( | ||
| "rejects the non-string input %j", | ||
| (input) => { | ||
| expect(isValidEmail(input)).toBe(false); | ||
| }, | ||
| ); | ||
|
|
||
| describe("internationalized addresses (RFC 6531)", () => { | ||
| it.each([ | ||
| "jürgen@example.com", | ||
| "user@münchen.de", | ||
| "jürgen@münchen.de", | ||
| "用户@例子.广告", | ||
| "θσερ@εχαμπλε.ψομ", | ||
| "अजय@डाटा.भारत", | ||
| "квіточка@пошта.укр", | ||
| ])("accepts %j", (email) => { | ||
| expect(isValidEmail(email)).toBe(true); | ||
| }); | ||
| }); | ||
|
|
||
| describe("case", () => { | ||
| // The email signup endpoint validates the raw request body and only | ||
| // lowercases the address afterwards, so the validator has to accept | ||
| // whatever casing the user typed. | ||
|
Comment on lines
+108
to
+110
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It really doesn't sound like this is concern that should be stated here! That is anothe rpart of the system and here we shouldn't be concerend about it. I would just say we accept uppercase and that is it. The rest of the system has to adapt to that, not this to it. |
||
| it.each([ | ||
| "JOHN@EXAMPLE.COM", | ||
| "John@Example.com", | ||
| "jOhN@eXaMpLe.CoM", | ||
| // Uppercase confined to the domain. | ||
| "john@Example.com", | ||
| "john@example.COM", | ||
| // Uppercase confined to the local part, running right up to the "@". | ||
| "JOHN@example.com", | ||
| "john.DOE@example.com", | ||
| ])("accepts %j", (email) => { | ||
| expect(isValidEmail(email)).toBe(true); | ||
| }); | ||
| }); | ||
|
|
||
| describe("anchoring", () => { | ||
| it.each([ | ||
| "user@example.com <script>alert(1)</script>", | ||
| "I am not an email, ask user@example.com", | ||
| "\nuser@example.com", | ||
| "user@example.com\n", | ||
| " user@example.com ", | ||
| ])("rejects %j, which merely contains an address", (input) => { | ||
| expect(isValidEmail(input)).toBe(false); | ||
| }); | ||
| }); | ||
|
Comment on lines
+126
to
+136
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A bit weird names: anchoring, and also "rejects %j, which merely contains an address". Ancorhing is impelmenation detail of regexes, not sometihng we should test here. I would instead name the test "email is substring ... and shouldn't be" or something liek that. I am not fully knowledagable about this describe/it mechanism here in whatevver test library we use so I don't know exactsly how to best name but you get the point. |
||
|
|
||
| describe("size limits (RFC 5321)", () => { | ||
| it("accepts a 64 octet local part", () => { | ||
| expect(isValidEmail(`${"a".repeat(64)}@example.com`)).toBe(true); | ||
| }); | ||
|
|
||
| it("rejects a 65 octet local part", () => { | ||
| expect(isValidEmail(`${"a".repeat(65)}@example.com`)).toBe(false); | ||
| }); | ||
|
|
||
| it("counts octets rather than characters in the local part", () => { | ||
| // "ä" is two octets in UTF-8, so 33 of them exceed the 64 octet limit | ||
| // while staying well under 64 characters. | ||
| expect(isValidEmail(`${"ä".repeat(32)}@example.com`)).toBe(true); | ||
| expect(isValidEmail(`${"ä".repeat(33)}@example.com`)).toBe(false); | ||
| }); | ||
|
|
||
| it("rejects an address longer than 254 octets", () => { | ||
| const domain = `${"a".repeat(61)}.${"b".repeat(61)}.${"c".repeat(61)}.com`; | ||
| const localPart = "d".repeat(254 - domain.length - 1); | ||
|
|
||
| expect(isValidEmail(`${localPart}@${domain}`)).toBe(true); | ||
| expect(isValidEmail(`${localPart}x@${domain}`)).toBe(false); | ||
| }); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,7 @@ import { useAuthContext } from '@wasp.sh/lib-auth/browser' | |
|
|
||
| import { requestPasswordReset } from '../../../email/actions/passwordReset.js' | ||
| import { Form, FormItemGroup, FormLabel, FormInput, SubmitButton, FormError } from '../Form' | ||
| import { emailFieldRules, emailInputProps } from '../emailField' | ||
|
|
||
|
|
||
| // PRIVATE API | ||
|
|
@@ -34,10 +35,8 @@ export const ForgotPasswordForm = () => { | |
| <FormItemGroup> | ||
| <FormLabel>E-mail</FormLabel> | ||
| <FormInput | ||
| {...register('email', { | ||
| required: 'Email is required', | ||
| })} | ||
| type="email" | ||
| {...register('email', emailFieldRules)} | ||
| {...emailInputProps} | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This feels more bothersome than adding an
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, but there is nothing simpler if we want to support unicode. |
||
| disabled={isLoading} | ||
| /> | ||
| {errors.email && <FormError>{errors.email.message}</FormError>} | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| import { isValidEmail } from '@wasp.sh/lib-auth' | ||
|
|
||
| /** | ||
| * Deliberately not `type="email"`: browsers validate that against the HTML5 | ||
| * grammar, which is ASCII-only and would reject internationalized addresses | ||
| * that the server accepts. `inputMode` keeps the email keyboard on mobile. | ||
| */ | ||
| export const emailInputProps = { | ||
| type: 'text', | ||
| inputMode: 'email', | ||
| autoComplete: 'email', | ||
| } as const | ||
|
|
||
| /** | ||
| * react-hook-form rules that mirror the server side `ensureValidEmail`. | ||
| */ | ||
| export const emailFieldRules = { | ||
| required: 'Email is required', | ||
| validate: (email: string) => | ||
| isValidEmail(email) || 'Email must be a valid email', | ||
| } |

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole comment shoujld be on topo of isValidEmail f unction, shoudln't it? Why here?