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

auth/sign_up Password maxlength too small, inconsistently checked #25390

Open
AJCxZ0 opened this issue Jun 12, 2023 · 3 comments
Open

auth/sign_up Password maxlength too small, inconsistently checked #25390

AJCxZ0 opened this issue Jun 12, 2023 · 3 comments
Labels
bug Something isn't working

Comments

@AJCxZ0
Copy link

AJCxZ0 commented Jun 12, 2023

Steps to reproduce the problem

  1. Visit Sign up page
  2. Enter good long (>72 characters) password in Password field
  3. Observe Password field border turn approving green
  4. Enter good long password in Confirm password field
  5. Observe Confirm password field border turn disapproving red and field contain more •s
    ...

Expected behaviour

Long passwords in each field should be accepted

Actual behaviour

Long passwords are truncated in one field, yet accepted in the other, creating an error.

Detailed description

The code for the Password field specifies constraints which are not mentioned elsewhere, i.e. minlength="8" and maxlength="72", resulting in long passwords entered in the field being silently truncated.

<input aria-label="Password" autocomplete="new-password" minlength="8" maxlength="72" class="password required" required="required" aria-required="true" placeholder="Password" size="72" type="password" name="user[password]" id="user_password">

The code for the Confirm password field has no such limitations.

<input aria-label="Confirm password" autocomplete="new-password" class="password required" required="required" aria-required="true" placeholder="Confirm password" type="password" name="user[password_confirmation]" id="user_password_confirmation">

While it can be argued that this minimally satisfies the requirement to have the same password entered into each field, it has several shortcomings:

  1. The maximum length is arbitrarily short
  2. A long password is silently truncated
  3. The Confirm password field has no constraints
  4. The comparison fails for long passwords
  5. Determining the cause of the failure requires visual observation of the •s

While a discussion of what constitutes a "long" password in this time of long random passwords stored in a password managers and the security of these passwords depending mostly on breaches and length is outside the scope of this issue, Postel's law seems an appropriate guiding principal for any properly handled password storage.
Conversely, the lack of maxlength for the Confirm password_ field invites unnecessary opportunities for unintended consequences.

I suggest that both fields have minlength="8" and maxlength="1024", and that these constraints be explicitly advertised on the page. If the actual security of passwords and avoidance of account takeovers using breach data are desired, then I suggest checking against Pwned Passwords (which deserves a feature request).

Specifications

Mastodon v4.2.0
Mastodon v4.1.2+nightly-20230609
Mastodon v4.1.2+glitch
Mastodon v4.1.0
Mastodon v4.0.2
Mastodon v4.0.2+glitch

@AJCxZ0 AJCxZ0 added the bug Something isn't working label Jun 12, 2023
@ThisIsMissEm
Copy link
Contributor

This is a duplicate of #20653, #13152 and #24654 — essentially bcrypt which is what's used for hashing passwords doesn't accept more that 72 bytes of data, or at least only consumes the first 72 bytes of a password, anything beyond that doesn't actually increase security, hence limiting; it's an underlying issue in Devise: heartcombo/devise#5307

@AJCxZ0
Copy link
Author

AJCxZ0 commented Jul 2, 2023

@ThisIsMissEm Thank you for referencing those issues and summarising the current situation. Clearly the matter of handling maxlength is being addressed elsewhere.
Accepting that for now maximum length and how it's handled is a technical limitation, this leaves the lack of length constraints on the Confirm password field and the lack of guidance in the UI. While it's easy to describe fixes as simple when others are doing the fixing, these both appear to be trivial to resolve, e.g.

  1. Advertise constraints in the placeholder and increase the size of the input field to clearly show when the limit has been reached. This continues to ignore any concern with the hashing of characters encoded with more than one byte.
    <input aria-label="Password" autocomplete="new-password" minlength="8" maxlength="72" class="password required" required="required" aria-required="true" placeholder="Password (8 - 72 characters)" size="75" type="password" name="user[password]" id="user_password">
  2. Add missing length constraints to the input field and increase its size to match the other for the same reason.
    <input aria-label="Confirm password" autocomplete="new-password" minlength="8" maxlength="72" class="password required" required="required" aria-required="true" placeholder="Confirm password" size="75" type="password" name="user[password_confirmation]" id="user_password_confirmation">

@ThisIsMissEm
Copy link
Contributor

You could add the min & max length options here:

Making the placeholder parametised might be a little more difficult & would require updating localisations but could be doable too I think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants