Skip to content
Permalink
Browse files Browse the repository at this point in the history
Fix login and signup views to make email enumeration harder
  • Loading branch information
cuu508 committed Jan 23, 2023
1 parent e8c2262 commit 359edbd
Show file tree
Hide file tree
Showing 7 changed files with 37 additions and 28 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -17,6 +17,7 @@ All notable changes to this project will be documented in this file.
- Fix special character encoding in project invite emails
- Fix check transfer between same account's projects when at check limit
- Fix wording in the invite email when inviting read-only users
- Fix login and signup views to make email enumeration harder

## v2.5 - 2022-12-14

Expand Down
7 changes: 1 addition & 6 deletions hc/accounts/forms.py
Expand Up @@ -30,11 +30,6 @@ def clean_identity(self):
if len(v) > 254:
raise forms.ValidationError("Address is too long.")

if User.objects.filter(email=v).exists():
raise forms.ValidationError(
"An account with this email address already exists."
)

return v

def clean_tz(self):
Expand All @@ -60,7 +55,7 @@ def clean_identity(self):
try:
self.user = User.objects.get(email=v)
except User.DoesNotExist:
raise forms.ValidationError("Unknown email address.")
self.user = None

return v

Expand Down
10 changes: 10 additions & 0 deletions hc/accounts/tests/test_login.py
Expand Up @@ -57,6 +57,16 @@ def test_it_sends_link_with_next(self):
body = mail.outbox[0].body
self.assertTrue("/?next=" + self.channels_url in body)

def test_it_handles_unknown_email(self):
form = {"identity": "surprise@example.org"}

r = self.client.post("/accounts/login/", form)
self.assertRedirects(r, "/accounts/login_link_sent/")
self.assertIn("auto-login", r.cookies)

# There should be no sent emails.
self.assertEqual(len(mail.outbox), 0)

@override_settings(SECRET_KEY="test-secret")
def test_it_rate_limits_emails(self):
# "d60d..." is sha1("alice@example.orgtest-secret")
Expand Down
12 changes: 8 additions & 4 deletions hc/accounts/tests/test_signup.py
Expand Up @@ -16,7 +16,7 @@ def test_it_works(self):
form = {"identity": "alice@example.org", "tz": "Europe/Riga"}

r = self.client.post("/accounts/signup/", form)
self.assertContains(r, "Account created")
self.assertContains(r, "check your email")
self.assertIn("auto-login", r.cookies)

# An user should have been created
Expand Down Expand Up @@ -75,13 +75,17 @@ def test_it_ignores_case(self):
q = User.objects.filter(email="alice@example.org")
self.assertTrue(q.exists)

def test_it_checks_for_existing_users(self):
def test_it_handles_existing_users(self):
alice = User(username="alice", email="alice@example.org")
alice.save()

form = {"identity": "alice@example.org", "tz": ""}
r = self.client.post("/accounts/signup/", form)
self.assertContains(r, "already exists")
self.assertContains(r, "check your email")
self.assertIn("auto-login", r.cookies)

# It should not send an email
self.assertEqual(len(mail.outbox), 0)

def test_it_checks_syntax(self):
form = {"identity": "alice at example org", "tz": ""}
Expand All @@ -101,7 +105,7 @@ def test_it_ignores_bad_tz(self):
form = {"identity": "alice@example.org", "tz": "Foo/Bar"}

r = self.client.post("/accounts/signup/", form)
self.assertContains(r, "Account created")
self.assertContains(r, "check your email")
self.assertIn("auto-login", r.cookies)

profile = Profile.objects.get()
Expand Down
19 changes: 10 additions & 9 deletions hc/accounts/views.py
Expand Up @@ -161,10 +161,11 @@ def login(request):
if not _allow_redirect(redirect_url):
redirect_url = None

profile = Profile.objects.for_user(magic_form.user)
profile.send_instant_login_link(redirect_url=redirect_url)
response = redirect("hc-login-link-sent")
if magic_form.user:
profile = Profile.objects.for_user(magic_form.user)
profile.send_instant_login_link(redirect_url=redirect_url)

response = redirect("hc-login-link-sent")
# check_token looks for this cookie to decide if
# it needs to do the extra POST step.
response.set_cookie("auto-login", "1", max_age=300, httponly=True)
Expand Down Expand Up @@ -201,16 +202,16 @@ def signup(request):
form = forms.SignupForm(request.POST)
if form.is_valid():
email = form.cleaned_data["identity"]
tz = form.cleaned_data["tz"]
user = _make_user(email, tz)
profile = Profile.objects.for_user(user)
profile.send_instant_login_link()
ctx["created"] = True
if not User.objects.filter(email=email).exists():
tz = form.cleaned_data["tz"]
user = _make_user(email, tz)
profile = Profile.objects.for_user(user)
profile.send_instant_login_link()
else:
ctx = {"form": form}

response = render(request, "accounts/signup_result.html", ctx)
if ctx.get("created"):
if "form" not in ctx:
response.set_cookie("auto-login", "1", max_age=300, httponly=True)

return response
Expand Down
8 changes: 4 additions & 4 deletions templates/accounts/login_link_sent.html
@@ -1,16 +1,16 @@
{% extends "base.html" %}
{% load hc_extras %}

{% block content %}
<div class="row">
<div class="col-sm-6 col-sm-offset-3">
<div class="hc-dialog">
<h1>Login Link Sent!</h1>
<h1>Check Your Email!</h1>
<br />
<p>
We've sent you an email with login instructions.
Please check your inbox!
If a {% site_name %} account exists for this email address,
you will receive a login link in your email shortly.
</p>

</div>
</div>
</div>
Expand Down
8 changes: 3 additions & 5 deletions templates/accounts/signup_result.html
@@ -1,7 +1,5 @@
{% for error in form.identity.errors %}
<p class="text-danger">{{ error }}</p>
{% endfor %}

{% if created %}
<p class="text-success">Account created, please check your email!</p>
{% endif %}
{% empty %}
<p class="text-success">Please check your email!</p>
{% endfor %}

0 comments on commit 359edbd

Please sign in to comment.