Improve Password Length Validation for BCrypt Compatibility #47708

guilleiguaran · 2023-03-18T01:53:35Z

Motivation / Background

This Pull Request has been created because we identified a need to improve password length validation for BCrypt compatibility in the ActiveModel::SecurePassword module. The current validation only considers the character count, which may not accurately reflect the byte size limit imposed by BCrypt.

Detail

This Pull Request changes the password length validation by:

Updating the validation to consider byte size. This ensures that passwords adhere to the 72-byte limit imposed by BCrypt.
Adding a custom validation error message key :password_too_long for the byte size validation. This allows for better internationalization (i18n) support and customization of the error message when the byte size limit is exceeded.
Updating the existing test cases and adding new test cases to cover the updated validation logic, ensuring that both character count and byte size validations work as expected.
Providing clear comments and explanations in the code to describe the purpose of the validation and the rationale behind the implementation.

These changes improve the overall user experience and maintain compatibility with BCrypt's limitations.

Checklist

Before submitting the PR make sure the following are checked:

This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
Tests are added or updated if you fix a bug or add a feature.
CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

Note for reviewers

Everything in this pull request, including the code, tests, changelog, commit message, pull request title and description has been created by ChatGPT with some guidance. If you believe ChatGPT is infringing your copyright please let me know.

guilleiguaran · 2023-03-18T02:05:45Z

Note for reviewers

Everything in this pull request, including the code, tests, changelog, commit message, pull request title and description has been created by ChatGPT with some guidance. If you believe ChatGPT is infringing your copyright please let me know.

activemodel/CHANGELOG.md

savonarola · 2023-03-21T10:40:00Z

@guilleiguaran awesome demonstration of ChatGPT capabilities!
But ChatGPT is a proprietary tool, couldn't there be any legal issues with code ownership after accepting such a patch?

fig · 2023-03-21T19:48:54Z

@savonarola I guess we could just ask ChatGPT for permission to use the code it wrote? Or ask it to release the code under MIT?

guilleiguaran · 2023-03-21T21:15:59Z

@savonarola

Thank you for your kind words and for raising a valid concern. As an AI language model, my purpose is to assist and support developers in their tasks. While I may provide suggestions and guidance, the final decisions and implementation are made by the developers themselves.

Regarding code ownership, it is important to remember that the code produced through our collaboration has been reviewed, modified, and ultimately submitted by a human developer. As a result, the ownership and responsibility for the code belong to the person who submitted the changes

activemodel/test/cases/secure_password_test.rb

activemodel/lib/active_model/secure_password.rb

petergeneric · 2023-03-22T00:44:36Z

@guilleiguaran OT slightly but will you have a writeup of the process you followed here? It would be very interesting to see the prompts and what feedback you provided

openbl · 2023-03-23T14:55:25Z

My $0.02: I suspect there might be a benefit to having a distinction between "character length" vs "byte length" validations and wonder if this should be its own validation, validates_byte_size_of, instead. If not we could just collapse this to check the #bytesize and not the #length at all, right?

jlestavel · 2023-03-23T15:25:47Z

@guilleiguaran

Everything in this pull request, including the code, tests, changelog, commit message, pull request title and description has been created by ChatGPT with some guidance. If you believe ChatGPT is infringing your copyright please let me know.

that's amazing! Just out of curiosity: would it be possible to get the transcript of your conversation with the bot? just to understand how you get it do this quite elaborate work. Thanks a lot!

silva96 · 2023-03-23T16:12:30Z

🍿 coming from rubyweekly

abdullaachilov · 2023-03-23T16:46:18Z

What a time to be alive 🤖

al3rez · 2023-03-23T16:56:32Z

seems chatgpt loves nested ifs

activemodel/lib/active_model/secure_password.rb

abdullaachilov · 2023-03-23T17:02:52Z

activemodel/lib/active_model/secure_password.rb

+          validate do |record|
+            password_value = record.public_send(attribute)
+            if password_value.present?
+              if password_value.length > ActiveModel::SecurePassword::MAX_PASSWORD_LENGTH_ALLOWED
+                record.errors.add(attribute, :too_long, count: ActiveModel::SecurePassword::MAX_PASSWORD_LENGTH_ALLOWED)
+              elsif password_value.bytesize > ActiveModel::SecurePassword::MAX_PASSWORD_LENGTH_ALLOWED
+                record.errors.add(attribute, :too_long_in_bytes, count: ActiveModel::SecurePassword::MAX_PASSWORD_LENGTH_ALLOWED)
+              end
+            end
+          end


Suggested change

validate do |record|

password_value = record.public_send(attribute)

if password_value.present?

if password_value.length > ActiveModel::SecurePassword::MAX_PASSWORD_LENGTH_ALLOWED

record.errors.add(attribute, :too_long, count: ActiveModel::SecurePassword::MAX_PASSWORD_LENGTH_ALLOWED)

elsif password_value.bytesize > ActiveModel::SecurePassword::MAX_PASSWORD_LENGTH_ALLOWED

record.errors.add(attribute, :too_long_in_bytes, count: ActiveModel::SecurePassword::MAX_PASSWORD_LENGTH_ALLOWED)

end

end

end

validate do |record|

password_value = record.public_send(attribute)

return unless password_value.present?

if password_value.length > ActiveModel::SecurePassword::MAX_PASSWORD_LENGTH_ALLOWED

record.errors.add(attribute, :too_long, count: ActiveModel::SecurePassword::MAX_PASSWORD_LENGTH_ALLOWED)

elsif password_value.bytesize > ActiveModel::SecurePassword::MAX_PASSWORD_LENGTH_ALLOWED

record.errors.add(attribute, :too_long_in_bytes, count: ActiveModel::SecurePassword::MAX_PASSWORD_LENGTH_ALLOWED)

end

end

Refactor proposed by chatGPT as well

In this refactored version, I've made the following changes: Added a newline between method definition and code block for better readability. Removed one level of indentation by using a guard clause (return unless password_value.present?) instead of wrapping the entire block in an if statement.

jomunoz · 2023-03-23T19:57:32Z

activemodel/lib/active_model/secure_password.rb

+            if password_value.present?
+              if password_value.length > ActiveModel::SecurePassword::MAX_PASSWORD_LENGTH_ALLOWED
+                record.errors.add(attribute, :too_long, count: ActiveModel::SecurePassword::MAX_PASSWORD_LENGTH_ALLOWED)
+              elsif password_value.bytesize > ActiveModel::SecurePassword::MAX_PASSWORD_LENGTH_ALLOWED


since it is for Bcrypt, shouldn't these lines be:

elsif password_value.bytesize > BCrypt::Engine::MAX_SECRET_BYTESIZE record.errors.add(attribute, :too_long_in_bytes, count: BCrypt::Engine::MAX_SECRET_BYTESIZE)

?

scarroll32 · 2023-03-23T20:53:54Z

@guilleiguaran awesome work! Would you care to share the guidance you gave ChatGPT to create this PR?

jdnichollsc · 2023-03-24T13:50:29Z

Hey folks, why not creating a repo with useful prompts like this? @guilleiguaran awesome job! <3

emceeaich · 2023-03-24T20:24:29Z

Allowing LLM generated pull requests establishes a dangerous precedent for the Rails project, and I ask the maintainers to consider the long term harms this will cause.

khasinski · 2023-03-29T10:24:32Z

activemodel/lib/active_model/locale/en.yml

@@ -18,6 +18,7 @@ en:
      too_long:
        one: "is too long (maximum is 1 character)"
        other: "is too long (maximum is %{count} characters)"
+      too_long_in_bytes: "is too long (maximum is %{count} bytes)"


"is too long (maximum is %{count} bytes)" seems too technical for an end-user readable error message, perhaps just "is too long"?

but the whole point of this check is byte length, string itself can be short, but because of fat characters it cannot be proceeded.

… BCrypt compatibility - Validate password length in both characters and bytes - Provide user-friendly error message for character length - Add byte size validation due to BCrypt's 72-byte limit Co-authored-by: ChatGPT [Fix #47600]

Co-authored-by: Petrik de Heus <petrik@deheus.net>

- Simplify password validation to only check byte size for BCrypt limit (72 bytes) - Replace specific error messages with a single "is too long" message - Update test cases to reflect new error message Co-authored-by: ChatGPT

CHANGELOG was not updated with subsequent changes to the PR rails#47708

davidauza-engineer

Nice!

donno2048 · 2023-08-23T19:43:24Z

activemodel/CHANGELOG.md

+    ```ruby
+      user = User.new(password: "a" * 73)  # 73 characters
+      user.valid? # => false
+      user.errors[:password] # => ["is too long (maximum is 72 characters)"]


user.errors[:password] # => ["is too long (maximum is 72 characters)"]

Should be

user.errors[:password] # => ["is too long"]

donno2048 · 2023-08-23T19:46:06Z

activemodel/CHANGELOG.md

+      user.errors[:password] # => ["is too long (maximum is 72 bytes)"]
+    ```
+
+    *ChatGPT*, *Guillermo Iguaran*


Although interesting I don't think it should be listed as an author, just like you wouldn't list any other tool as an author (e.g. copilot or simple autocomplete)

Restore detailed error message for password length validation This commit reverts a change introduced in PR rails#47708 that led to the default error message for an excessively long password becoming less informative. The previous behavior included a dynamic count of the maximum allowed characters in the error message, guiding developers and users more clearly on the validation constraints. Changes: - Reintroduced the `count` parameter in the locale file `activemodel/lib/active_model/locale/en.yml` for the `:too_long` key to display maximum character count. - Updated the error symbol in `activemodel/lib/active_model/secure_password.rb` from `:password_too_long` to `:too_long`, aligning with the standard error message format. - Removed the `:password_too_long` key from `activemodel/lib/active_model/locale/en.yml` since the `:too_long` key is now correctly used again. These changes ensure that users receive a detailed and helpful error message, indicating the exact character limit for password fields. See also commit a60785c for further details. --- Co-authored by ChatGPT.

rails-bot bot added the activemodel label Mar 18, 2023

guilleiguaran changed the title ~~Improve password length validation in ActiveModel::SecurePassword for BCrypt compatibility~~ Improve Password Length Validation for BCrypt Compatibility Mar 18, 2023

p8 reviewed Mar 18, 2023

View reviewed changes

activemodel/CHANGELOG.md Outdated Show resolved Hide resolved

p8 reviewed Mar 18, 2023

View reviewed changes

activemodel/CHANGELOG.md Outdated Show resolved Hide resolved

BrianWilkinsFL reviewed Mar 22, 2023

View reviewed changes

activemodel/test/cases/secure_password_test.rb Show resolved Hide resolved

activemodel/test/cases/secure_password_test.rb Show resolved Hide resolved

activemodel/lib/active_model/secure_password.rb Outdated Show resolved Hide resolved

rafaelfranca reviewed Mar 23, 2023

View reviewed changes

activemodel/lib/active_model/secure_password.rb Outdated Show resolved Hide resolved

abdullaachilov reviewed Mar 23, 2023

View reviewed changes

jomunoz reviewed Mar 23, 2023

View reviewed changes

khasinski reviewed Mar 29, 2023

View reviewed changes

guilleiguaran and others added 3 commits April 19, 2023 15:56

Update CHANGELOG examples

63f0914

Co-authored-by: Petrik de Heus <petrik@deheus.net>

Update password validation and error messages

a60785c

- Simplify password validation to only check byte size for BCrypt limit (72 bytes) - Replace specific error messages with a single "is too long" message - Update test cases to reflect new error message Co-authored-by: ChatGPT

guilleiguaran force-pushed the fix-am-secure-password-length-validation branch from 807cda8 to a60785c Compare April 19, 2023 22:59

guilleiguaran merged commit 836fdec into main Apr 20, 2023

guilleiguaran deleted the fix-am-secure-password-length-validation branch April 20, 2023 04:49

chhhris added a commit to chhhris/rails that referenced this pull request Apr 21, 2023

Update CHANGELOG example

42a4fce

CHANGELOG was not updated with subsequent changes to the PR rails#47708

chhhris mentioned this pull request Apr 21, 2023

Update CHANGELOG example related to Improve Password Length Validation for BCrypt Compatibility #48017

Merged

4 tasks

danielvdao pushed a commit to danielvdao/rails that referenced this pull request May 1, 2023

Update CHANGELOG example

c973b23

CHANGELOG was not updated with subsequent changes to the PR rails#47708

danielvdao pushed a commit to danielvdao/rails that referenced this pull request May 1, 2023

Update CHANGELOG example

efbedd3

CHANGELOG was not updated with subsequent changes to the PR rails#47708

danielvdao pushed a commit to danielvdao/rails that referenced this pull request May 1, 2023

Update CHANGELOG example

cc50758

CHANGELOG was not updated with subsequent changes to the PR rails#47708

davidauza-engineer reviewed May 17, 2023

View reviewed changes

donno2048 reviewed Aug 23, 2023

View reviewed changes

cabgfx mentioned this pull request Nov 20, 2023

Restore detailed error message for excessive password length #50109

Open

4 tasks

arg mentioned this pull request Nov 22, 2023

Rails 7.1 updated password validation changed rendering of password inputs heartcombo/simple_form#1825

Closed

floehopper mentioned this pull request Jan 16, 2024

Cannot use a 128-character password freerange/jam-coop#149

Closed

wakairo mentioned this pull request Sep 28, 2024

Support for password_too_long svenfuchs/rails-i18n#1129

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Password Length Validation for BCrypt Compatibility #47708

Improve Password Length Validation for BCrypt Compatibility #47708

guilleiguaran commented Mar 18, 2023 •

edited

Loading

guilleiguaran commented Mar 18, 2023

savonarola commented Mar 21, 2023

fig commented Mar 21, 2023

guilleiguaran commented Mar 21, 2023

petergeneric commented Mar 22, 2023

openbl commented Mar 23, 2023

jlestavel commented Mar 23, 2023

silva96 commented Mar 23, 2023

abdullaachilov commented Mar 23, 2023

al3rez commented Mar 23, 2023

abdullaachilov Mar 23, 2023

jomunoz Mar 23, 2023 •

edited

Loading

scarroll32 commented Mar 23, 2023

jdnichollsc commented Mar 24, 2023

emceeaich commented Mar 24, 2023

khasinski Mar 29, 2023 •

edited

Loading

abdullaachilov Mar 29, 2023

davidauza-engineer left a comment

donno2048 Aug 23, 2023

donno2048 Aug 23, 2023

Improve Password Length Validation for BCrypt Compatibility #47708

Improve Password Length Validation for BCrypt Compatibility #47708

Conversation

guilleiguaran commented Mar 18, 2023 • edited Loading

Motivation / Background

Detail

Checklist

Note for reviewers

guilleiguaran commented Mar 18, 2023

Note for reviewers

savonarola commented Mar 21, 2023

fig commented Mar 21, 2023

guilleiguaran commented Mar 21, 2023

petergeneric commented Mar 22, 2023

openbl commented Mar 23, 2023

jlestavel commented Mar 23, 2023

silva96 commented Mar 23, 2023

abdullaachilov commented Mar 23, 2023

al3rez commented Mar 23, 2023

abdullaachilov Mar 23, 2023

Choose a reason for hiding this comment

jomunoz Mar 23, 2023 • edited Loading

Choose a reason for hiding this comment

scarroll32 commented Mar 23, 2023

jdnichollsc commented Mar 24, 2023

emceeaich commented Mar 24, 2023

khasinski Mar 29, 2023 • edited Loading

Choose a reason for hiding this comment

abdullaachilov Mar 29, 2023

Choose a reason for hiding this comment

davidauza-engineer left a comment

Choose a reason for hiding this comment

donno2048 Aug 23, 2023

Choose a reason for hiding this comment

donno2048 Aug 23, 2023

Choose a reason for hiding this comment

guilleiguaran commented Mar 18, 2023 •

edited

Loading

jomunoz Mar 23, 2023 •

edited

Loading

khasinski Mar 29, 2023 •

edited

Loading