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

Improve password-checking regex #279

Merged
merged 3 commits into from
Sep 25, 2023

Conversation

rmunn
Copy link
Contributor

@rmunn rmunn commented Sep 14, 2023

Now that we've verified that only the three symbols &, +, and % cause problems in passwords submitted through Chorus, we can loosen the password rules to allow any character except those three.

Fixes #255.

Now that we've verified that only the three symbols &, +, and % cause
problems in passwords submitted through Chorus, we can loosen the
password rules to allow any character except those three.
@rmunn rmunn requested review from myieye and removed request for myieye September 14, 2023 04:23
@rmunn rmunn marked this pull request as draft September 14, 2023 04:24
@github-actions
Copy link

github-actions bot commented Sep 14, 2023

UI unit Tests

1 tests   1 ✔️  0s ⏱️
1 suites  0 💤
1 files    0

Results for commit 0b3c5b1.

♻️ This comment has been updated with latest results.

@rmunn
Copy link
Contributor Author

rmunn commented Sep 14, 2023

Converted to draft since I just saw this part of #255:

Send/Receive didn't work with the password: test-temp-0:?"!$. I'm not sure what the culprit is exactly

I tested the Chorus "Login" button but not a full Send/Receive. I'll add a full Send/Receive to my tests to determine whether other characters besides &, % and + might cause further issues.

@rmunn
Copy link
Contributor Author

rmunn commented Sep 14, 2023

Can't confirm Tim's experience that the password test-temp-0:?"!$ caused issues; it worked fine for me, including doing a full Send/Receive. I also checked many other special characters in a full S/R and not just logging in, and they all worked. So I'm restoring this PR to non-draft, because +, &, and % are indeed the only characters we need to worry about.

@rmunn rmunn marked this pull request as ready for review September 14, 2023 06:00
Copy link
Contributor

@myieye myieye left a comment

Choose a reason for hiding this comment

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

Thanks so much for doing the researching and finishing this up!
I just think the error message should be a bit shorter

frontend/src/lib/i18n/locales/en.json Outdated Show resolved Hide resolved
Copy link
Contributor

@myieye myieye left a comment

Choose a reason for hiding this comment

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

I still find the error message a bit odd.
"can cause problems" sounds more like "You can use them if you want, but something bad might happen somewhere".

From our perspective we don't allow these characters because they break S/R.
From a user perspective, they're just not allowed. If we want an explination (which I don't think is absolutely necessary, we could say something like "for backwards compatibility purposes") 🤷

@hahn-kev hahn-kev merged commit 86138a9 into develop Sep 25, 2023
@hahn-kev hahn-kev deleted the feature/allow-more-password-characters branch July 9, 2024 07:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Indicate that special characters in passwords could cause trouble.
3 participants