Make sure that email validators don't match unintentional non-ASCII local parts - #1075
Make sure that email validators don't match unintentional non-ASCII local parts#1075tats-u wants to merge 7 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
This comment was marked as resolved.
This comment was marked as resolved.
|
I doubt you know this fact. Internationalized emails are not considered by the current regex even now. This is an unintended behavior and should be fixed.
Discuss it only in WHATWG, then Valibot will change only
|
This comment was marked as resolved.
This comment was marked as resolved.
commit: |
|
In the first place what "internationalized email addresses" match that regex? SMTPUTF8 is not supported by all MTAs. Who uses non-ASCII characters as a local part? |
|
Have you seen https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Regular_expressions/Character_class_escape#w? What "accented letters" match |
|
Oh damn it, I mixed up regex flavors again... 😖 You're right here, JS' unicode flag doesn't do anything useful here; for some reason I was thinking with PCRE regex behavior in mind... 🙃 I marked my original comment as resolved, sorry for the confusion! ❤️ I'll open a separate issue for internationalized email support, as I still think it's an important thing to address nonetheless |
|
You should confirm detailed definitions of character classes in each language unless you are used to them. |
|
Just as a quick clarification... there is still interest in merging this PR? If so, I will review in soon. |
|
I took a quick look and am not sure if I want to merge it. |
Sure
This PR also disallows uncommon cases. I don't think you assume or know |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe email regex now uses only the case-insensitive 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR updates Valibot’s email validation regex to prevent unintended matches of non-ASCII characters in the local part (a follow-up to #1068), and adds regression coverage to ensure both email and rfcEmail reject such inputs.
Changes:
- Adjust
EMAIL_REGEXto avoid Unicode-case-folding side effects that can admit non-ASCII characters. - Add a regression case (
Kſ@example.com) toemailandrfcEmailtest suites.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
library/src/regex.ts |
Updates EMAIL_REGEX flags to avoid unintended Unicode/local-part matches. |
library/src/actions/rfcEmail/rfcEmail.test.ts |
Adds a regression test for non-ASCII local-part lookalikes. |
library/src/actions/email/email.test.ts |
Adds the same regression test for the email action. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
I think this fix is good. It can be merged, no problem. On the other hand, I think this is a very rare case. I don't think any user will encounter this. |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Follow-up of #1068
EMAIL_REGEXhas the same problem asRFC_EMAIL_REGEX.Also added a test case to
emailandrfcEmail. The latter has already been assured to be passed thanks to #1068.Summary by CodeRabbit