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

Trailing whitespace for usernames not removed #7111

Closed
sbg97 opened this issue Sep 14, 2022 · 3 comments · Fixed by #7432
Closed

Trailing whitespace for usernames not removed #7111

sbg97 opened this issue Sep 14, 2022 · 3 comments · Fixed by #7432
Labels
A-Signin O-Occasional Affects or can be seen by some users regularly or most users rarely S-Minor Impairs non-critical functionality or suitable workarounds exist T-Defect Something isn't working: bugs, crashes, hangs and other reported problems

Comments

@sbg97
Copy link

sbg97 commented Sep 14, 2022

Steps to reproduce

  1. I made an account. Ex Username: "test" Password: "test"
  2. Sign into the account with Username: "test " Password: "test"

Notice the trailing whitespace. This can be added automatically without the user noticing if you use autocorrect to complete your name.

Outcome

What did you expect?

I would expect the trailing whitespace to be automatically removed before processing since I'm pretty sure usernames can't have trailing whitespace.

What happened instead?

I get a popup saying "Invalid username or password"

Your phone model

moto g power

Operating system version

Android 11

Application version and app store

1.4.34 installed from F-Droid

Homeserver

Synapse

Will you send logs?

No

Are you willing to provide a PR?

No

@sbg97 sbg97 added the T-Defect Something isn't working: bugs, crashes, hangs and other reported problems label Sep 14, 2022
@mnaturel mnaturel added S-Minor Impairs non-critical functionality or suitable workarounds exist O-Occasional Affects or can be seen by some users regularly or most users rarely A-Signin labels Sep 14, 2022
@mnaturel
Copy link
Contributor

Thanks for raising the issue. I guess we should add a verification on the username field like we do on the signup screen to warn about invalid characters.

@ByeongsuPark
Copy link
Contributor

ByeongsuPark commented Oct 19, 2022

Hi, I'd like to take this issue.
Before that, I have a question about this issue's solution.
How about adding trim to the username input?
Considering that autocomplete is for the user's convenience, Adding a verification on the username's field as we do on the signup screen to warn about invalid characters leads to the user's inconvenience because it requires the user to do further action like deleting whitespace which is not needed if we just trim the user's input

@mnaturel
Copy link
Contributor

Hi, I'd like to take this issue. Before that, I have a question about this issue's solution. How about adding trim to the username input? Considering that autocomplete is for the user's convenience, Adding a verification on the username's field as we do on the signup screen to warn about invalid characters leads to the user's inconvenience because it requires the user to do further action like deleting whitespace which is not needed if we just trim the user's input

Hi @ByeongsuPark, adding a trim to remove any trailing whitespace is okay as solution. I think it should be done when pressing the signin button on app side. I guess we should also add it on SDK side so that every client benefits from this.

I have done some checks and it seems this trim was done in the previous version of the signin screen. Plus, I have also checked the Matrix specs about userId and whitespace is not allowed on any server so it should not block anyone from signin.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Signin O-Occasional Affects or can be seen by some users regularly or most users rarely S-Minor Impairs non-critical functionality or suitable workarounds exist T-Defect Something isn't working: bugs, crashes, hangs and other reported problems
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants