Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#20593 closed Bug (fixed)

Disagreement between User.set_password("") and User.check_password("")

Reported by: Jonas Kölker Owned by: Tim Graham <timograham@…>
Component: contrib.auth Version: master
Severity: Normal Keywords: blank password
Cc: bmispelon@…, eromijn@…, Jonas Kölker Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no


At, under set_unusable_password, it says "Marks the user as having no password set. This isn’t the same as having a blank string for a password", which suggests that the latter is possible. Also, set_password doesn't say anything about blank passwords not being allowed. Yet, when I run

    assert user.check_password(form.cleaned_data['new1'])

with both new passwords being empty, I get an assertion failure. According to at least two core developers*, allowing a blank password is not a bug---which would indicate that set_password does the wrong thing.

two: plus!msg/django-developers/NtpFFdYh-3g/erB1Qhd6m2UJ

Looking at, I see

def make_password(password, salt=None, hasher='default'):
    """[... If password is None or blank then UNUSABLE_PASSWORD will be returned ...]"""
    if not password:
        return UNUSABLE_PASSWORD

That looks rather deliberate. If that's so I think the documentation should be updated, reflecting the fact that User.set_password("") does something rather anti-intuitive, and that there even more anti-intuitively exists a string s such that if you do set_password(s), then check_password(s) fails.

Also, I think silently forbidding blank passwords is really bad. If set_password were to throw a ValueError with an explanation of why you rather than I should decide whether to allow blank passwords, I would dislike it but at least its anti-intuitiveness would be explicit.

As things are at present, given the silent transition (or did I look in all the wrong places for the deprecation warnings?) users of Django applications could lock themselves out when they intend to have a blank password---the 180 degrees opposite effect of what they want.

I've set version=1.4 on this ticket, because that's the one I use in my production environment, i.e. the one where I have exhibited this behavior. Version 1.3 did something different, see

I've set easy=true, because I think the fix is a one-line patch and a four-line addition to/of a test case---which I'll be happy to write---or some documentation rewriting (which I'll leave to someone else). I think the best next step is for the group of core devs to agree on what the right thing for set_password("") to do is, then add that information to this ticket.

Attachments (1)

empty-password-fix.patch (6.4 KB) - added by Jonas Kölker 5 years ago.
Proper fix, plus test cases

Download all attachments as: .zip

Change History (12)

comment:1 Changed 5 years ago by Baptiste Mispelon

Cc: bmispelon@… added
Triage Stage: UnreviewedAccepted


The backwards-incompatible change was made in commit 90e05aaeac612a4251640564aa65f103ac635e12 (which, as you noted, landed in 1.4).
There is no reference to a ticket nor is there a note in the release notes so I'm not sure the change was intentional.

I'm accepting this on the basis that this backwards-incompatible change should at least be documented, if not even reverted.

Thanks for the thorough report.

comment:2 Changed 5 years ago by wim@…

In my opinion, set_password("") should raise an exception.

Rationale: passwords are used to identify users. Allowing them to have empty passwords is unsafe; and if you want people to be able to enter a website without having a password then use a different authentication backend which does not check for an empty password.

In practice, all django authentication forms require the password field to be filled, and people have to enter characters for the form to be valid, otherwise the form raises an error.

comment:3 Changed 5 years ago by Paul McMillan

If a blank password was valid before, I agree with the reporter that it should still be valid, and I agree that set_password/check_password should be inverses. This function is not the place to enforce password conformance to any particular policy, and there are cases where an empty password is legitimately ok (e.g. when building a system authenticated by hardware tokens).

A correct patch will check for None rather than truthiness, and document the period during which an empty string was unintentionally disallowed. I would also like to see a unittest to make sure each of our included hashers correctly handles the empty string case.

comment:4 Changed 5 years ago by Sasha Romijn

Cc: eromijn@… added
Needs documentation: set
Owner: changed from nobody to Sasha Romijn
Status: newassigned

comment:5 Changed 5 years ago by Sasha Romijn

Has patch: set
Needs documentation: unset
Owner: Sasha Romijn deleted
Status: assignednew
Version: 1.4master

PaulM's suggestions make sense to me. PR:

Also added a note in the release notes about this change. Tests successfully run.

comment:6 Changed 5 years ago by Tim Graham <timograham@…>

Owner: set to Tim Graham <timograham@…>
Resolution: fixed
Status: newclosed

In 2c4fe761a0e2b28e2c5c3b4bc506ee06824a443d:

Fixed #20593 -- Allow blank passwords in check_password() and set_password()

Changed 5 years ago by Jonas Kölker

Attachment: empty-password-fix.patch added

Proper fix, plus test cases

comment:7 Changed 5 years ago by Jonas Kölker

Cc: Jonas Kölker added
Resolution: fixed
Status: closednew

Assuming my understanding of what bmispelon and PaulM think should be done is correct, the patch at doesn't fix the problem.

I've attached my own fix. With my changes to the test suite and erikr's changes to, the test cases fail---meaning that either his fix or my tests are invalid. If my tests are wrong, please explain why. Note: if after applying my patch you revert any of the hunks applied to you will introduce a test case failure.

comment:8 Changed 5 years ago by Jonas Kölker

Oh yay, I misread a summary of erikr's patch as if it were the whole patch. His patch is fine. Sorry for reopening. Nothing to see here, move along ;-)

comment:9 Changed 5 years ago by Baptiste Mispelon

Resolution: fixed
Status: newclosed

comment:10 Changed 5 years ago by Simon Charette <charette.s@…>

In 8759778185d0539bf9c11e3fda497a9486b9acab:

Fixed #20675 -- check_password should work when no password is specified.

The regression was introduced by 2c4fe761a. refs #20593.

comment:11 Changed 5 years ago by Simon Charette <charette.s@…>

In 2de0d4c4523ca3d1d6744ba0f22b8ef33bedfa03:

[1.6.x] Fixed #20675 -- check_password should work when no password is specified.

The regression was introduced by 2c4fe761a. refs #20593.

Backport of 8759778185 from master.

Note: See TracTickets for help on using tickets.
Back to Top