/CC @pantherale0

Worth just mentioning the background to the limit for those who may be unaware, this sums it up nicely: https://security.stackexchange.com/a/184090

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

@frenck masking it silently was the case before too. I think we should address that separately.

masking it silently was the case before too. I think we should address that separately.

The goal with this PR was to just restore the previous state as fast as possible. Yes, we would tell our users that their password is too long. However that might require a bit more thought and might be best for a separate PR.

E.g. I believe the login form is used for both user creation and login itself. So while we could certainly add a length validator, that would block existing users with these long passwords. Not sure we can do much more than raising an issue and wait for users to change their passwords themselves. If someone has a better idea for a migration, I'm all ears.

Please add a test with a password that is longer than 72 characters.

My added test already covers it or am I missing something? Maybe I should rename the test case to test_long_passwords?

    pwd_truncated = "hWwjDpFiYtDTaaMbXdjzeuKAPI3G4Di2mC92" * 4  # 72 chars
    long_pwd = pwd_truncated * 2  # 144 chars
    data.add_auth("test-user", long_pwd)
    data.validate_login("test-user", long_pwd)

I'm wondering if a migration to argon2 would be appropriate. A period of time where both bcrypt and argon2 is supported with HA, as users sign in etc, the existing bcrypt hash stored is replaced with an argon hash instead. Perhaps that's an architectural discussion.

I keep seeing mentions of bcrypt being supported in python for "legacy purposes". Although long term, either way some length validation on the frontend would probably be a requirement to prevent a denial of service attack.

@cdce8p sorry, I must have missed that you added a test already.

To be clear, I don't think we should solve anything more in this PR but restore the earlier behavior that trunkated the password silently. We definitely want to improve this but that's for a different PR.

Can you shed some light why we didn't catch the breaking change in version 5 of bcrypt when working on the bump PR?

Can you shed some light why we didn't catch the breaking change in version 5 of bcrypt when working on the bump PR?

I probably just missed it or didn't recognize at the time that this would cause issues. The tests all passed so there wasn't any indication to spend more time investigating it and it wasn't caught by the PR review either.

To be fair, there are users with really long passwords but they are definitely the exception not the norm. AFAIK we didn't receive any reports about this during the beta either.

But did you note that it was a major version change and that it then should contain at least one breaking change, and read the changelog? I'm digging a bit here, since I was under the impression that this is our process when we do library bumps. Especially when it's a library that we don't maintain ourselves and may not be familiar with the latest changes.

But did you note that it was a major version change and that it then should contain at least one breaking change, and read the changelog?

A lot of breaking changes aren't actually relevant though. E.g. a lot of project will do a new major release when dropping support for an older Python version.

I do read the changelogs and almost always also scan the full diff when opening a dependency bump. Sometimes things can slip through. Ideally they are caught during the review but if not, the best I can do is to revert / fix the issue once I'm made aware of it. Not sure there is more I can tell you about it.

Good! Yes, things can slip through. It's important that we follow a good process. Thanks for confirming that was done.