#29952 closed Bug (fixed)
All passwords in contrib/auth/common-passwords.txt.gz should be lowercased
Reported by: | Mathew Payne | Owned by: | Mathew Payne |
---|---|---|---|
Component: | contrib.auth | Version: | 2.1 |
Severity: | Release blocker | Keywords: | password, validation |
Cc: | Triage Stage: | Accepted | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description
There is a bug with the "CommonPasswordValidator" where the password validation step is done using password.lower()
to make sure that case sensitivity isn't an issue in passwords.
497 of the passwords stored in the Django provided file (django/contrib/auth/common-passwords.txt.gz
) are stored containing uppercase characters which means that 497 of the passwords are not checked correctly when validating.
Here are some examples of passwords in the file:
- VQsaBLPzLa
- FQRG7CS493
- DIOSESFIEL
- 3rJs1la7qE
- ...
Here is a small test to prove that the the password validation is not working as it should:
tests/auth_tests/test_validators.py
class CommonPasswordValidatorTest(TestCase): def test_password_validation_issue(self): # using standard list validator = CommonPasswordValidator() # is the password in the list? Yes. self.assertIn('VQsaBLPzLa', validator.passwords) # check if the validation function throws an error. It doesn't with self.assertRaises(ValidationError) as cm: self.assertIsNone(validator.validate('VQsaBLPzLa'))
And I get this Unit test error:
====================================================================== FAIL: test_password_validation_issue (auth_tests.test_validators.CommonPasswordValidatorTest) ---------------------------------------------------------------------- Traceback (most recent call last): File "/usr/lib/python3.6/unittest/case.py", line 59, in testPartExecutor yield File "/usr/lib/python3.6/unittest/case.py", line 605, in run testMethod() File "/user/geekmasher/django/tests/auth_tests/test_validators.py", line 193, in test_password_validation_issue self.assertIsNone(validator.validate('VQsaBLPzLa')) File "/usr/lib/python3.6/unittest/case.py", line 203, in __exit__ self._raiseFailure("{} not raised".format(exc_name)) File "/usr/lib/python3.6/unittest/case.py", line 135, in _raiseFailure raise self.test_case.failureException(msg) AssertionError: ValidationError not raised
Recommendation:
- All passwords in the Django supplied list should be lowered to match the validation phase.
- Duplication's should be removed from the Django list, if any.
- If a user supplied path is given, the user can request to
auto_lower
the lists items on init to match the validation.- If it's lowered every time a user supplied path is provided (even if they are lowered already), the performance of the function could be significant slower.
I will be submitting a Pull Request shortly.
Change History (5)
comment:1 by , 6 years ago
Owner: | changed from | to
---|
comment:2 by , 6 years ago
Patch needs improvement: | set |
---|---|
Severity: | Normal → Release blocker |
Summary: | CommonPasswordValidator with uppercase passwords in common-passwords.txt.gz → All passwords in contrib/auth/common-passwords.txt.gz should be lowercased |
Triage Stage: | Unreviewed → Accepted |
Version: | master → 2.1 |
comment:3 by , 6 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
comment:5 by , 6 years ago
Great work Mathew. You also discovered there is some duplication in the file too right? That could be removed too, and a test added to check that it isn't re-added, in a new ticket now this is closed.
In 26bb261: