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

fix: Usernames should encode all characters in emails #9541

Open
wants to merge 8 commits into
base: alpha
Choose a base branch
from

Conversation

dblythy
Copy link
Member

@dblythy dblythy commented Jan 14, 2025

Pull Request

Issue

Closes: #9111

Approach

Tasks

  • Add tests

Copy link

I will reformat the title to use the proper commit message syntax.

@parse-github-assistant parse-github-assistant bot changed the title fix: usernames should encode all characters in emails fix: Usernames should encode all characters in emails Jan 14, 2025
Copy link

Thanks for opening this pull request!

@dblythy dblythy requested a review from a team January 21, 2025 10:52
Copy link

codecov bot commented Jan 21, 2025

Codecov Report

Attention: Patch coverage is 60.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 93.49%. Comparing base (3b20a6f) to head (2e0d057).

Files with missing lines Patch % Lines
src/Utils.js 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##            alpha    #9541      +/-   ##
==========================================
+ Coverage   93.15%   93.49%   +0.33%     
==========================================
  Files         186      186              
  Lines       14807    14810       +3     
==========================================
+ Hits        13794    13846      +52     
+ Misses       1013      964      -49     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/Utils.js Outdated Show resolved Hide resolved
spec/EmailVerificationToken.spec.js Outdated Show resolved Hide resolved
@mtrezza
Copy link
Member

mtrezza commented Jan 21, 2025

@mathieulb does this address the issue you described?

@mtrezza
Copy link
Member

mtrezza commented Jan 21, 2025

@dblythy Does #8488 effectively make this PR obsolete? Or does it still make senes to add this improved escaping logic, since we can't predict what URLs we'll have in the future.

@mathieulb
Copy link

@mtrezza , it addresses the specific example problem I encountered, but I confirm that it seems to not handle dot, comma, etc., as you suspect, because they are supposed to be in the regexp. PR 8488 would make this PR 9541 obsolete, but PR 9541 isn't obsolete until PR 8488 gets accepted, obviously.

If for any reason PR 8488 shouldn't be accepted instead, PR 9541 should get more tests with creative ascii art at the end of the username, to cover all ASCII characters that I listed in the comments of #9111 : !"'),.:;<>?]^}. This was tested in GMail. Codepoints outside of the pure ASCII range (32..126) are already covered by encodeURIComponent.

@mtrezza
Copy link
Member

mtrezza commented Jan 27, 2025

Then let's merge this PR as it's almost done. Do you have any additional feedback on the PR as it is now?

I confirm that it seems to not handle dot, comma, etc., as you suspect, because they are supposed to be in the regexp.

Does your comment belong to #9541 (comment)?

@mathieulb
Copy link

mathieulb commented Jan 27, 2025

mtrezza, I was mainly replying to "does this address the #9111 you described?" which was directed at me, but this also involved referring to the other thread you mention now ("How does it handle dot, comma, etc after the URL?").

I don't have additional feedback (I'm not familiar with the parse source & tests, and I'm not set up to try parse from git at the moment)

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