-
Notifications
You must be signed in to change notification settings - Fork 3k
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
[HOLD for payment 2023-09-07] [$4000] Inconsistent front-end email validation across the product #17387
Comments
Triggered auto assignment to @adelekennedy ( |
Bug0 Triage Checklist (Main S/O)
|
@adelekennedy Whoops! This issue is 2 days overdue. Let's get this updated quick! |
I can reproduce! Seems very similar to #17388 |
Job added to Upwork: https://www.upwork.com/jobs/~01878e05ad15a14027 |
Current assignee @adelekennedy is eligible for the External assigner, not assigning anyone new. |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @aimane-chnaif ( |
Triggered auto assignment to @NikkiWines ( |
ProposalPlease re-state the problem that we are trying to solve in this issue.Invalid email validation in chats, leading to the wrong strings markdowns being transformed to email anchor tags by using After the transformation, when we click the link rendered in chats, it also opens that invalid email in external email app. What is the root cause of that problem?Root cause of the problem is wrong regex applied in expensify's helper library called Expensify-common. Here the regex expressions are defined in the file Since a valid email can have at most 254 characters, this regex failed to put a limit on length of total string which is causing the problem What changes do you think we should make in order to solve the problem?We should add a clause to the regex above which can limit the total length of valid email. After adding the length condition: Before that(in production right now): Thankyou! EDIT: grammar |
ProposalPlease re-state the problem that we are trying to solve in this issue.We want email validation to be consistent across the app and between FE and BE. What is the root cause of that problem?We are using two different sets of regex patterns on the FE, neither of which fully matches the behaviour of the BE. The one that is used when validating emails when starting a new chat can be traced back to and then there's the markdown one here, which eventually is used to converts emails to links in-chat: This duplicating of regexes appears to be a result of this PR. What changes do you think we should make in order to solve the problem?I think it makes sense to have one master rule which fully matches BE behaviour, which would be used both for starting chats and for figuring out when a string in a message needs to be converted to an email link. This just keeps things simple going forwards, but I can see there could be an argument for looser constraints for the chat markdown conversion (as it's less critical if it gets it wrong). The BE uses a combination of If we look at the regex of
We don't need to care about the details of the filter's character validation as the additional regex the BE uses is more restrictive (e.g. BE won't accept "{" as a valid character, whereas the PHP filter would), so we just want to apply the BE's additional regex when it comes to which characters we accept. In order to match all the above behaviour, I think we should update New regex This combines the length limits in The The
This ensures that the length count starts at either whitespace, start of line or non-email character. Without this you'd get the part of the email within the length limit converted and the rest left as plain text. The BE considers Show fix for emails with leading underscoresIt's easy enough to handle simple cases like
where it becomes less clear which parts should be italicised and we can't simply handle this in one regex lookup. The easiest solution is just not to accept leading underscores in markdown since it's really rare to actually see an email starting with an underscore. Slack gets around this by just ignoring leading underscores, for example: However if we want to fix it, we need to add a
This callback will run the italic markdown rule to find any italic markdown underscores, and then run the email replace rule within the italics, before running the email replace on the rest of the text. We can move the existing markdown italic regex into a constant at the top of
Then the callback function is:
TestsAll the following test suite emails can be validated against the PHP filter + BE regex in this quick PHP script I threw together. Fixes to existing test suitesThere are several tests that need to be removed as they test for emails that the BE considers invalid. The following email patterns need to be removed from this test:
There are also several tests that include the following invalid emails in the same file:
We would also need to modify the result of this test as
This test also needs to be updated toBeFalsy as New test casesSeveral new tests need to be added to Str-test.js. These can replace Show Str-test.js new tests
We can also add similar tests to ExpensiMark-HTML-test.js, as well as a new test for the more complex italic markdown underscore case mentioned above: Show ExpensiMark-HTML-test.js new tests
|
Hi @jjcoffee , |
I agree that it's inconsistency issue to validate emails differently in various parts (login, chat, add contact, search, etc) with different regexes. |
I am coming from the PR which introduced MARKDOWN_EMAIL regex. I am not sure if understand the problem statement correctly or what is trying to be fixed here.
I am confused here, Are we trying to solve all 3 above or something specifically? Could you kindly provide this information so I can provide better information with regards to the original MARKDOWN_EMAIL regex. @aimane-chnaif |
@abdulrahuman5196 abc@abc.abc is valid email. What the issue mentioned is very long email: @chiragxarora as you're a reporter, can you share example invalid email here so that other can copy&paste and reproduce easily? |
Sure @aimane-chnaif |
@adelekennedy I've sent again |
@adelekennedy Thanks, applied to the job! |
Hello, Expensive Team
In this example, we're using the validator.js library to validate the email. Make sure to install it using npm install validator. The handleInputChange function updates the email state as the user types. When the user presses the submit button, the handleSubmit function is called. It checks if the email is valid using validator.isEmail(email). On a successful validation, you can then proceed to submit the email to the server. Make sure that server-side validation is also implemented to ensure that only valid emails are accepted. Remember, this is a basic example and you may need to adapt it to fit your specific application structure and requirements. Additionally, you should handle edge cases and potential error scenarios appropriately. For server-side validation, you can use PHP's FILTER_VALIDATE_EMAIL or the provided regex in your backend code to further ensure that only valid emails are accepted. |
📣 @EliteDevSolution! 📣
|
✅ Contributor details stored successfully. Thank you for contributing to Expensify! |
@EliteDevSolution as a new contributor, please review our contributing guideline. This issue doesn't have |
Hello, Expensive Team
I think that no problem with phone number verification too. |
@adelekennedy 24 hours passed. Still didn't get invitation on my email. Could you please ping the inbox person again for me? |
@Antasel did you respond to Ben? |
@adelekennedy 24 hours passed. Still didn't get invitation on my email. Could you please ping the inbox person again for me? |
Have you replied to Ben? I added the screenshot of the email exchange above |
|
Payouts due:Issue Reporter: $250 @chiragxarora (Upwork) Eligible for 50% #urgency bonus? N Upwork job is here. |
@chiragxarora will you apply to the Upwork job for the reporting bonus? |
Pls send offer, I don't have connects:( |
What's your Upwork information? |
@chiragxarora please confirm your upwork information |
@adelekennedy Thanks, offer accepted! |
@adelekennedy accepted offer, thanks |
Yes this is it @adelekennedy , pls send the offer here |
Offer sent @chiragxarora |
Offer accepted! @adelekennedy |
@adelekennedy I haven't received payment yet. Can you please check? |
oh @aimane-chnaif I'm so sorry I thought you were in the NewDot payment beta as well, I'll update the summary above and send you an offer through Upwork |
I already accepted Melvin offer |
@adelekennedy bump ^ |
@aimane-chnaif just ended the contract |
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Action Performed:
(Chat)
(Login/SignUp)
(Add Contact Method)
Expected result:
Front-end validation should prevent these emails from being added and creating unnecessary network requests. Additionally, consistent validation should be used in all areas where we validate emails in the product (e.g. pasting an invalid email into an existing chat still formats it like a valid email)
Backend email validation uses PHP's email validation filter FILTER_VALIDATE_EMAIL as well as the following regex (for emails and phone numbers) [\w.'#%+-]+@[a-zA-Z\d.-]+.[a-zA-Z]{2,}|^+?[1-9]\d{1,14}$)
Actual results:
Emails are successfully submitted, only to be subsequently rejected by the backend
Workaround:
N/A
Platforms:
Which of our officially supported platforms is this issue occurring on?
Version Number: v1.3.0-0
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
bandicam.2023-04-13.13-58-25-321.mp4
Expensify/Expensify Issue URL:
Issue reported by: @chiragxarora
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1681358100970919
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: