Signing in by email code: the guess-limit, enumeration and second-factor tests cannot fail #631

New issue

Closed

opened 2026-10-01 10:48:31 +00:00 by tiagoagueda · 0 comments

tiagoagueda commented

2026-10-01 10:48:31 +00:00

Owner

Found on 2026-09-29 in a read-only review of postulo at 95348908d, running the assertions of tests/security/test_email_sign_in.py against altered settings.

Related to #153.

Checked again on 2026-10-01 against main at 0a5cad587: still present. The code is now at tests/security/test_email_sign_in.py:102-113 (test_a_code_does_not_skip_an_authenticator), :143-151 (test_asking_about_an_address_nobody_has_says_the_same_thing), :154-161 (test_guessing_the_code_runs_out) and :199-201 (_asked_for_a_code): two lines up, the three tests themselves unchanged.

Three tests in tests/security/test_email_sign_in.py pass whether or not the property they name holds.

1. The guess limit

test_guessing_the_code_runs_out (:156-163) posts 000000 three times and asserts that the person is not signed in. Any wrong code leaves the person signed out, limit or none, and the test never sends the right code afterwards. With ACCOUNT_LOGIN_BY_CODE_MAX_ATTEMPTS=1000 its assertion still holds, and the real code then signs the person in. With the limit at 2, the real code is refused after three guesses. The test cannot tell these two cases apart.

2. Enumeration

test_asking_about_an_address_nobody_has_says_the_same_thing (:145-153) compares status codes and _asked_for_a_code (:201-203), which is "code" in content. The request page contains "code" too. With allauth's ACCOUNT_PREVENT_ENUMERATION=False, a fresh client asking about nobody@example.org gets the form back with "The email address is not assigned to any user account." and status 200, and both of the test's comparisons are still equal. The test also uses one client for both requests. After the known address, the session holds a pending code login and the second request is redirected to the confirm page before the address is looked at, so the unknown address is never really asked.

3. The second factor

test_a_code_does_not_skip_an_authenticator (:104-115) ends with "authenticator" in ... or response.redirect_chain, which any redirect satisfies. Today the chain ends at /accounts/2fa/authenticate/, but the assertion would accept any redirect. The first assertion (_auth_user_id not in the session) still guards the main property.

Where: tests/security/test_email_sign_in.py:104-115, :145-153, :156-163, :201-203.
What happens: an allauth upgrade or a settings change disables the attempt counter, or lets the request form say which addresses have no account. Either would allow brute-forcing a code or listing which addresses have accounts here, and the security suite would stay green.
Why it matters: these are the two decisions the file's docstring calls the reason it lives in tests/security/. The tests that represent them prove neither.
Fix: (1) Read the real code, make MAX_ATTEMPTS wrong guesses, post the real code and assert that it is refused, with a control showing that the real code works without the guesses. (2) Use two separate clients, or flush the session between the requests. Assert that both end at reverse("account_confirm_login_code"), that neither response carries form errors, and that only the known address produced a message in mail.outbox. (3) Assert that the last entry of redirect_chain is reverse("mfa_authenticate").

Done when

Under override_settings(ACCOUNT_LOGIN_BY_CODE_MAX_ATTEMPTS=1000) the rewritten guess-limit test fails, and under ACCOUNT_PREVENT_ENUMERATION=False the rewritten enumeration test fails. Both pass with the shipped settings.

Found on 2026-09-29 in a read-only review of postulo at 95348908d, running the assertions of `tests/security/test_email_sign_in.py` against altered settings. Related to #153. Checked again on 2026-10-01 against `main` at `0a5cad587`: still present. The code is now at `tests/security/test_email_sign_in.py:102-113` (`test_a_code_does_not_skip_an_authenticator`), `:143-151` (`test_asking_about_an_address_nobody_has_says_the_same_thing`), `:154-161` (`test_guessing_the_code_runs_out`) and `:199-201` (`_asked_for_a_code`): two lines up, the three tests themselves unchanged. Three tests in `tests/security/test_email_sign_in.py` pass whether or not the property they name holds. ## 1. The guess limit `test_guessing_the_code_runs_out` (`:156-163`) posts `000000` three times and asserts that the person is not signed in. Any wrong code leaves the person signed out, limit or none, and the test never sends the right code afterwards. With `ACCOUNT_LOGIN_BY_CODE_MAX_ATTEMPTS=1000` its assertion still holds, and the real code then signs the person in. With the limit at 2, the real code is refused after three guesses. The test cannot tell these two cases apart. ## 2. Enumeration `test_asking_about_an_address_nobody_has_says_the_same_thing` (`:145-153`) compares status codes and `_asked_for_a_code` (`:201-203`), which is `"code" in content`. The request page contains "code" too. With allauth's `ACCOUNT_PREVENT_ENUMERATION=False`, a fresh client asking about `nobody@example.org` gets the form back with "The email address is not assigned to any user account." and status 200, and both of the test's comparisons are still equal. The test also uses one client for both requests. After the known address, the session holds a pending code login and the second request is redirected to the confirm page before the address is looked at, so the unknown address is never really asked. ## 3. The second factor `test_a_code_does_not_skip_an_authenticator` (`:104-115`) ends with `"authenticator" in ... or response.redirect_chain`, which any redirect satisfies. Today the chain ends at `/accounts/2fa/authenticate/`, but the assertion would accept any redirect. The first assertion (`_auth_user_id` not in the session) still guards the main property. - **Where:** `tests/security/test_email_sign_in.py:104-115`, `:145-153`, `:156-163`, `:201-203`. - **What happens:** an allauth upgrade or a settings change disables the attempt counter, or lets the request form say which addresses have no account. Either would allow brute-forcing a code or listing which addresses have accounts here, and the security suite would stay green. - **Why it matters:** these are the two decisions the file's docstring calls the reason it lives in `tests/security/`. The tests that represent them prove neither. - **Fix:** (1) Read the real code, make `MAX_ATTEMPTS` wrong guesses, post the real code and assert that it is refused, with a control showing that the real code works without the guesses. (2) Use two separate clients, or flush the session between the requests. Assert that both end at `reverse("account_confirm_login_code")`, that neither response carries form errors, and that only the known address produced a message in `mail.outbox`. (3) Assert that the last entry of `redirect_chain` is `reverse("mfa_authenticate")`. ## Done when - Under `override_settings(ACCOUNT_LOGIN_BY_CODE_MAX_ATTEMPTS=1000)` the rewritten guess-limit test fails, and under `ACCOUNT_PREVENT_ENUMERATION=False` the rewritten enumeration test fails. Both pass with the shipped settings.

tiagoagueda added this to the 0.6.0 milestone

2026-10-01 10:48:31 +00:00

tiagoagueda added the

bug

tier

labels

2026-10-01 10:48:31 +00:00

tiagoagueda referenced this issue from a commit

2026-10-04 07:56:39 +00:00

Make the email sign-in security tests able to fail

tiagoagueda closed this issue

2026-10-04 07:56:39 +00:00