-
Notifications
You must be signed in to change notification settings - Fork 701
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
Core: Removing leading/trailing whitespace from passwords #4359
Conversation
@@ -35,7 +35,8 @@ def get_meta(options_source: dict, race: bool = False) -> Dict[str, Union[List[s | |||
"remaining_mode": str(options_source.get("remaining_mode", ServerOptions.remaining_mode)), | |||
"collect_mode": str(options_source.get("collect_mode", ServerOptions.collect_mode)), | |||
"item_cheat": bool(int(options_source.get("item_cheat", not ServerOptions.disable_item_cheat))), | |||
"server_password": str(options_source.get("server_password", None)), | |||
# None if None else str.strip() | |||
"server_password": str(options_source.get("server_password") and options_source["server_password"].strip()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like it silently changing it to something we didn't ask for.
I think it would be better to throw an exception if it has spaces.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is how player names and other things are already handled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say it's much more visible to all kinds of users & easier to figure out what the change was in case of player names (Web tracker / Room, !players
from Text Client) , if my password doesn't work I'd probably panic quite a bit
Apparently, /option password
is a thing for the server console, which is at least one way to rectify it
but yeah this change is pretty spooky I think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know the proper way to make this error on webhost or common client, so I'm probably just going to close this PR
What is this fixing or adding?
Like the title says. Motivation for this is that various clients (including the TextClient) cannot connect to rooms with passwords with trailing and/or leading whitespace.
How was this tested?
Running MultiServer and localhost generations without passwords and also with passwords with leading/trailing whitespace. Checked the room passwords through
/options
and connected to them using the TextClient.