Skip to content

fix(auth): strip login credentials#2458

Draft
ppigazzini wants to merge 1 commit intoofficial-stockfish:masterfrom
ppigazzini:trim_usr_pwd
Draft

fix(auth): strip login credentials#2458
ppigazzini wants to merge 1 commit intoofficial-stockfish:masterfrom
ppigazzini:trim_usr_pwd

Conversation

@ppigazzini
Copy link
Copy Markdown
Collaborator

Normalize username and password inputs during login to avoid whitespace-related auth failures.

Normalize username and password inputs during login to avoid
whitespace-related auth failures.
Copilot AI review requested due to automatic review settings February 2, 2026 01:23
@ppigazzini ppigazzini added bug server server side changes gui labels Feb 2, 2026
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR normalizes username and password inputs during login by stripping leading and trailing whitespace to prevent authentication failures caused by accidental whitespace.

Changes:

  • Modified the login function to apply .strip() to username and password inputs, aligning with the existing behavior in the signup function

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread server/fishtest/views.py
@ppigazzini
Copy link
Copy Markdown
Collaborator Author

@vdbergh I have not merged yet because I'm unsure about the best practices here. GitHub login form at example strips only trail spaces and only for the username.

@Disservin
Copy link
Copy Markdown
Member

https://cheatsheetseries.owasp.org/cheatsheets/Authentication_Cheat_Sheet.html

Allow usage of all characters including unicode and whitespace. There should be no password composition rules limiting the type of characters permitted. There should be no requirement for upper or lower case or numbers or special characters.

@ppigazzini
Copy link
Copy Markdown
Collaborator Author

ppigazzini commented Feb 2, 2026

https://cheatsheetseries.owasp.org/cheatsheets/Authentication_Cheat_Sheet.html

Allow usage of all characters including unicode and whitespace. There should be no password composition rules limiting the type of characters permitted. There should be no requirement for upper or lower case or numbers or special characters.

We allow internal space ("aaa bbb"), but we strip username and password in signup before saving in DB. In my experience as user a login form usually does not strip the fields and reports an error if I add accidentally leading/trailing spaces.

def signup(request):
if request.authenticated_userid:
return home(request)
if request.method != "POST":
return {}
errors = []
signup_username = request.POST.get("username", "").strip()
signup_password = request.POST.get("password", "").strip()
signup_password_verify = request.POST.get("password2", "").strip()
signup_email = request.POST.get("email", "").strip()
tests_repo = request.POST.get("tests_repo", "").strip()

@ppigazzini
Copy link
Copy Markdown
Collaborator Author

https://cloud.google.com/blog/products/identity-security/account-authentication-and-password-management-best-practices

It's not unreasonable for a site or service to require usernames longer than two or three characters, block hidden characters, and prevent whitespace at the beginning and end of a username.

@Disservin
Copy link
Copy Markdown
Member

Disservin commented Feb 2, 2026

from that same link because your passage was about usernames

If someone wants a password made of Klingon, Emoji, and ASCII art with whitespace on both ends, you should have no technical reason to deny them.

@ppigazzini
Copy link
Copy Markdown
Collaborator Author

I checked the DB:

  • no username and no password with leading/trailing spaces/tabs/new lines etc
  • some usernames and some passwords with internal spaces
    The DB is at least consistent with the registration strip policy in master.

@vdbergh
Copy link
Copy Markdown
Contributor

vdbergh commented Feb 2, 2026

My initial reaction was that if a user wants to enter a password starting and ending with spaces then they should be allowed to do so (the user has to enter the password twice, so they are presumably aware what they are doing). Spaces are not weirder than all kinds of control characters.

But then I noticed that the server strips passwords...

@Disservin
Copy link
Copy Markdown
Member

Disservin commented Feb 2, 2026

this was only added here #1822
imo we should just revert the strip change? for the password signup but keep it for everything else

@ppigazzini
Copy link
Copy Markdown
Collaborator Author

ppigazzini commented Feb 2, 2026

On the server I have not a companion script of that PR to strip the username and the password in the DB.
After that PR a new user was not able to login after the registration with leading/trailing spaces in username and password.
If I'm not wrong dropping the strip should not hit the users.
We could have a glitch with spaces in username for a route like this, though.
"https://dfts-0.pigazzini.it/tests/user/ nostrip "

BTW something is rejecting the username with the spaces:
image

@vdbergh
Copy link
Copy Markdown
Contributor

vdbergh commented Feb 2, 2026

I don't feel strongly about this. But let me say that I don't like spaces in usernames but about passwords I feel differently.

@vdbergh
Copy link
Copy Markdown
Contributor

vdbergh commented Feb 2, 2026

BTW something is rejecting the username with the spaces:

I think Peregrine added that to the front end. But we could not do it in the back end since unfortunately there are some usernames in the userdb with spaces. And it is not trivial to fix this since these usernames appear also in the rundb and actiondb (and maybe in other collections).

@vdbergh
Copy link
Copy Markdown
Contributor

vdbergh commented Feb 3, 2026

Concerning spaces in usernames. We could enforce "no spaces" in the schema, except for some explicit list of historical usernames (perhaps contained in the key value store).

@ppigazzini ppigazzini removed the bug label Feb 3, 2026
@ppigazzini ppigazzini marked this pull request as draft February 3, 2026 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gui server server side changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants