From 1fd5d0b3ce4237af75bc2d8a378204ee656913d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?P=C4=93teris=20Caune?= Date: Wed, 18 Oct 2017 15:53:08 +0300 Subject: [PATCH] More secure unsubscribe links for monthly reports. --- hc/accounts/models.py | 16 +++++++------- hc/accounts/tests/test_unsubscribe_reports.py | 21 ++++++++++++++++++- hc/accounts/urls.py | 2 +- hc/accounts/views.py | 18 ++++++++++++---- templates/bad_link.html | 4 ++-- 5 files changed, 46 insertions(+), 15 deletions(-) diff --git a/hc/accounts/models.py b/hc/accounts/models.py index 01b00e8e..03a4476e 100644 --- a/hc/accounts/models.py +++ b/hc/accounts/models.py @@ -6,7 +6,7 @@ from datetime import timedelta from django.conf import settings from django.contrib.auth.hashers import check_password, make_password from django.contrib.auth.models import User -from django.core import signing +from django.core.signing import TimestampSigner from django.db import models from django.urls import reverse from django.utils import timezone @@ -68,6 +68,12 @@ class Profile(models.Model): def notifications_url(self): return settings.SITE_ROOT + reverse("hc-notifications") + def reports_unsub_url(self): + signer = TimestampSigner(salt="reports") + signed_username = signer.sign(self.user.username) + path = reverse("hc-unsubscribe-reports", args=[signed_username]) + return settings.SITE_ROOT + path + def team(self): # compare ids to avoid SQL queries if self.current_team_id and self.current_team_id != self.id: @@ -137,10 +143,6 @@ class Profile(models.Model): if nag and num_down == 0: return False - token = signing.Signer().sign(uuid.uuid4()) - path = reverse("hc-unsubscribe-reports", args=[self.user.username]) - unsub_link = "%s%s?token=%s" % (settings.SITE_ROOT, path, token) - # Sort checks by owner. Need this because will group by owner in # template. checks = checks.order_by("user_id") @@ -149,8 +151,8 @@ class Profile(models.Model): "checks": checks, "sort": self.sort, "now": timezone.now(), - "unsub_link": unsub_link, - "notifications_url": self.notifications_url, + "unsub_link": self.reports_unsub_url(), + "notifications_url": self.notifications_url(), "nag": nag, "nag_period": self.nag_period.total_seconds(), "num_down": num_down diff --git a/hc/accounts/tests/test_unsubscribe_reports.py b/hc/accounts/tests/test_unsubscribe_reports.py index ef232dd6..e3194f54 100644 --- a/hc/accounts/tests/test_unsubscribe_reports.py +++ b/hc/accounts/tests/test_unsubscribe_reports.py @@ -6,7 +6,7 @@ from hc.test import BaseTestCase class UnsubscribeReportsTestCase(BaseTestCase): - def test_it_works(self): + def test_token_works(self): self.profile.nag_period = td(hours=1) self.profile.save() @@ -18,3 +18,22 @@ class UnsubscribeReportsTestCase(BaseTestCase): self.profile.refresh_from_db() self.assertFalse(self.profile.reports_allowed) self.assertEqual(self.profile.nag_period.total_seconds(), 0) + + def test_bad_token_gets_rejected(self): + url = "/accounts/unsubscribe_reports/alice/?token=invalid" + r = self.client.get(url) + self.assertContains(r, "Incorrect Link") + + def test_signed_username_works(self): + sig = signing.TimestampSigner(salt="reports").sign("alice") + url = "/accounts/unsubscribe_reports/%s/" % sig + r = self.client.get(url) + self.assertContains(r, "You have been unsubscribed") + + self.profile.refresh_from_db() + self.assertFalse(self.profile.reports_allowed) + + def test_bad_signature_gets_rejected(self): + url = "/accounts/unsubscribe_reports/invalid/" + r = self.client.get(url) + self.assertContains(r, "Incorrect Link") diff --git a/hc/accounts/urls.py b/hc/accounts/urls.py index ca22c54f..af6a9a69 100644 --- a/hc/accounts/urls.py +++ b/hc/accounts/urls.py @@ -18,7 +18,7 @@ urlpatterns = [ url(r'^profile/badges/$', views.badges, name="hc-badges"), url(r'^close/$', views.close, name="hc-close"), - url(r'^unsubscribe_reports/([\w-]+)/$', + url(r'^unsubscribe_reports/([\w\:-]+)/$', views.unsubscribe_reports, name="hc-unsubscribe-reports"), url(r'^set_password/([\w-]+)/$', diff --git a/hc/accounts/views.py b/hc/accounts/views.py index f600d6a1..184b7f17 100644 --- a/hc/accounts/views.py +++ b/hc/accounts/views.py @@ -348,10 +348,20 @@ def change_email_done(request): def unsubscribe_reports(request, username): - try: - signing.Signer().unsign(request.GET.get("token")) - except signing.BadSignature: - return HttpResponseBadRequest() + if ":" in username: + signer = signing.TimestampSigner(salt="reports") + try: + username = signer.unsign(username) + except signing.BadSignature: + return render(request, "bad_link.html") + else: + # Username is not signed but there should be a ?token=... parameter + # This is here for backwards compatibility and will be removed + # at some point. + try: + signing.Signer().unsign(request.GET.get("token")) + except signing.BadSignature: + return render(request, "bad_link.html") user = User.objects.get(username=username) profile = Profile.objects.for_user(user) diff --git a/templates/bad_link.html b/templates/bad_link.html index 38c5a6b7..5be559cc 100644 --- a/templates/bad_link.html +++ b/templates/bad_link.html @@ -4,10 +4,10 @@
-

Incorrect Confirmation Link

+

Incorrect Link

- The confirmation link you just used is incorrect. + The link you just used is incorrect. If you copy-pasted the link, please make sure you did not miss any characters.