From 96adadce5e5c5d10b9a55269af0ca7c3bcc86440 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20P=C3=A9pin?= Date: Sat, 5 Oct 2019 01:25:36 +0200 Subject: [PATCH 1/4] Replace some 403 by 404 to avoid trigramme leaking Fixes #224 --- kfet/views.py | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/kfet/views.py b/kfet/views.py index 4c8c6f47..dde4f24e 100644 --- a/kfet/views.py +++ b/kfet/views.py @@ -10,7 +10,6 @@ from django.contrib.auth.decorators import login_required, permission_required from django.contrib.auth.mixins import PermissionRequiredMixin from django.contrib.auth.models import Permission, User from django.contrib.messages.views import SuccessMessageMixin -from django.core.exceptions import PermissionDenied from django.db import transaction from django.db.models import Count, F, Prefetch, Sum from django.forms import formset_factory @@ -303,7 +302,7 @@ def account_read(request, trigramme): if not account.readable or ( not request.user.has_perm("kfet.is_team") and request.user != account.user ): - raise PermissionDenied + raise Http404 addcosts = ( OperationGroup.objects.filter(opes__addcost_for=account, opes__canceled_at=None) @@ -327,7 +326,7 @@ def account_update(request, trigramme): # Checking permissions if not request.user.has_perm("kfet.is_team") and request.user != account.user: - raise PermissionDenied + raise Http404 user_info_form = UserInfoForm(instance=account.user) @@ -2226,7 +2225,7 @@ class AccountStatBalanceList(PkUrlMixin, SingleResumeStat): def get_object(self, *args, **kwargs): obj = super().get_object(*args, **kwargs) if self.request.user != obj.user: - raise PermissionDenied + raise Http404 return obj @method_decorator(login_required) @@ -2345,7 +2344,7 @@ class AccountStatBalance(PkUrlMixin, JSONDetailView): def get_object(self, *args, **kwargs): obj = super().get_object(*args, **kwargs) if self.request.user != obj.user: - raise PermissionDenied + raise Http404 return obj @method_decorator(login_required) @@ -2376,7 +2375,7 @@ class AccountStatOperationList(PkUrlMixin, SingleResumeStat): def get_object(self, *args, **kwargs): obj = super().get_object(*args, **kwargs) if self.request.user != obj.user: - raise PermissionDenied + raise Http404 return obj @method_decorator(login_required) @@ -2439,7 +2438,7 @@ class AccountStatOperation(ScaleMixin, PkUrlMixin, JSONDetailView): def get_object(self, *args, **kwargs): obj = super().get_object(*args, **kwargs) if self.request.user != obj.user: - raise PermissionDenied + raise Http404 return obj @method_decorator(login_required) From e0285607a0524db2d843318937844b79100e882c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20P=C3=A9pin?= Date: Sat, 5 Oct 2019 02:25:05 +0200 Subject: [PATCH 2/4] Fix tests according to issue #224 --- kfet/tests/test_statistic.py | 5 +- kfet/tests/test_views.py | 118 ++++++++++++++++++++++++++++++++++- 2 files changed, 119 insertions(+), 4 deletions(-) diff --git a/kfet/tests/test_statistic.py b/kfet/tests/test_statistic.py index f0ed7f74..eda386b7 100644 --- a/kfet/tests/test_statistic.py +++ b/kfet/tests/test_statistic.py @@ -36,8 +36,7 @@ class TestStats(TestCase): client2 = Client() client2.login(username="Barfoo", password="barfoo") - # 1. FOO should be able to get these pages but BAR receives a Forbidden - # response + # 1. FOO should be able to get these pages but BAR receives a 404 user_urls = [ "/k-fet/accounts/FOO/stat/operations/list", "/k-fet/accounts/FOO/stat/operations?{}".format( @@ -57,7 +56,7 @@ class TestStats(TestCase): resp = client.get(url) self.assertEqual(200, resp.status_code) resp2 = client2.get(url) - self.assertEqual(403, resp2.status_code) + self.assertEqual(404, resp2.status_code) # 2. FOO is a member of the team and can get these pages but BAR # receives a Redirect response diff --git a/kfet/tests/test_views.py b/kfet/tests/test_views.py index cb6c9a0c..41e579bf 100644 --- a/kfet/tests/test_views.py +++ b/kfet/tests/test_views.py @@ -209,6 +209,25 @@ class AccountReadViewTests(ViewTestCaseMixin, TestCase): auth_user = "team" auth_forbidden = [None, "user"] + # Forbidden users should get a 404 here, to avoid leaking trigrams + # See issue #224 + def test_forbidden(self): + for user in self.auth_forbidden: + self.check_forbidden(user, self.url_expected) + self.check_forbidden(user, "/k-fet/accounts/NEX") + + def check_forbidden(self, user, url): + client = Client() + if user is None: + response = client.get(url) + self.assertRedirects( + response, "/login?next={}".format(url), fetch_redirect_response=False + ) + else: + client.login(username=user, password=user) + response = client.get(url) + self.assertEqual(response.status_code, 404) + def get_users_extra(self): return {"user1": create_user("user1", "001")} @@ -296,6 +315,27 @@ class AccountUpdateViewTests(ViewTestCaseMixin, TestCase): "team1": create_team("team1", "101", perms=["kfet.change_account"]), } + # Forbidden users should get a 404 here, to avoid leaking trigrams + # See issue #224 + def test_forbidden(self): + for method in ["get", "post"]: + for user in self.auth_forbidden: + self.check_forbidden(user, method, self.url_expected) + self.check_forbidden(user, method, "/k-fet/accounts/NEX/edit") + + def check_forbidden(self, user, method, url): + client = Client() + meth = getattr(client, method) + if user is None: + response = meth(url) + self.assertRedirects( + response, "/login?next={}".format(url), fetch_redirect_response=False + ) + else: + client.login(username=user, password=user) + response = meth(url) + self.assertEqual(response.status_code, 404) + def test_get_ok(self): r = self.client.get(self.url) self.assertEqual(r.status_code, 200) @@ -375,7 +415,7 @@ class AccountDeleteViewTests(ViewTestCaseMixin, TestCase): if Account.objects.get(trigramme=trigramme).readable: expected_code = 200 else: - expected_code = 403 + expected_code = 404 r = self.client.post( reverse(self.url_name, kwargs={"trigramme": trigramme}), {} ) @@ -555,6 +595,25 @@ class AccountStatOperationListViewTests(ViewTestCaseMixin, TestCase): def get_users_extra(self): return {"user1": create_user("user1", "001")} + # Forbidden users should get a 404 here, to avoid leaking trigrams + # See issue #224 + def test_forbidden(self): + for user in self.auth_forbidden: + self.check_forbidden(user, self.url_expected) + self.check_forbidden(user, "/k-fet/accounts/NEX/stat/operations/list") + + def check_forbidden(self, user, url): + client = Client() + if user is None: + response = client.get(url) + self.assertRedirects( + response, "/login?next={}".format(url), fetch_redirect_response=False + ) + else: + client.login(username=user, password=user) + response = client.get(url) + self.assertEqual(response.status_code, 404) + def test_ok(self): r = self.client.get(self.url) self.assertEqual(r.status_code, 200) @@ -616,6 +675,25 @@ class AccountStatOperationViewTests(ViewTestCaseMixin, TestCase): auth_user = "user1" auth_forbidden = [None, "user", "team"] + # Forbidden users should get a 404 here, to avoid leaking trigrams + # See issue #224 + def test_forbidden(self): + for user in self.auth_forbidden: + self.check_forbidden(user, self.url_expected) + self.check_forbidden(user, "/k-fet/accounts/NEX/stat/operations") + + def check_forbidden(self, user, url): + client = Client() + if user is None: + response = client.get(url) + self.assertRedirects( + response, "/login?next={}".format(url), fetch_redirect_response=False + ) + else: + client.login(username=user, password=user) + response = client.get(url) + self.assertEqual(response.status_code, 404) + def get_users_extra(self): return {"user1": create_user("user1", "001")} @@ -632,6 +710,25 @@ class AccountStatBalanceListViewTests(ViewTestCaseMixin, TestCase): auth_user = "user1" auth_forbidden = [None, "user", "team"] + # Forbidden users should get a 404 here, to avoid leaking trigrams + # See issue #224 + def test_forbidden(self): + for user in self.auth_forbidden: + self.check_forbidden(user, self.url_expected) + self.check_forbidden(user, "/k-fet/accounts/NEX/stat/balance/list") + + def check_forbidden(self, user, url): + client = Client() + if user is None: + response = client.get(url) + self.assertRedirects( + response, "/login?next={}".format(url), fetch_redirect_response=False + ) + else: + client.login(username=user, password=user) + response = client.get(url) + self.assertEqual(response.status_code, 404) + def get_users_extra(self): return {"user1": create_user("user1", "001")} @@ -677,6 +774,25 @@ class AccountStatBalanceViewTests(ViewTestCaseMixin, TestCase): auth_user = "user1" auth_forbidden = [None, "user", "team"] + # Forbidden users should get a 404 here, to avoid leaking trigrams + # See issue #224 + def test_forbidden(self): + for user in self.auth_forbidden: + self.check_forbidden(user, self.url_expected) + self.check_forbidden(user, "/k-fet/accounts/NEX/stat/balance") + + def check_forbidden(self, user, url): + client = Client() + if user is None: + response = client.get(url) + self.assertRedirects( + response, "/login?next={}".format(url), fetch_redirect_response=False + ) + else: + client.login(username=user, password=user) + response = client.get(url) + self.assertEqual(response.status_code, 404) + def get_users_extra(self): return {"user1": create_user("user1", "001")} From a4ecd344d03a8a39b21b17c1b79b58b92e60ea4c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20P=C3=A9pin?= Date: Sat, 5 Oct 2019 11:28:59 +0200 Subject: [PATCH 3/4] Update CHANGELOG --- CHANGELOG | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG b/CHANGELOG index 74278d21..a709d62c 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,3 +1,4 @@ +- Si on essaie d'accéder au compte que qqn d'autre on a une 404 (et plus une 403) - On ne peut plus modifier des comptes COF depuis l'interface K-Fêt - Le champ de paiement BdA se fait au niveau des attributions - Affiche un message d'erreur plutôt que de crasher si échec de l'envoi du mail From d37c41e99fff60fdf9c6176706517f0591756406 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20P=C3=A9pin?= Date: Sat, 5 Oct 2019 13:48:29 +0200 Subject: [PATCH 4/4] kfet/test_views: more eloquent test names --- kfet/tests/test_views.py | 56 +++++++++++++++++++++++----------------- 1 file changed, 32 insertions(+), 24 deletions(-) diff --git a/kfet/tests/test_views.py b/kfet/tests/test_views.py index 41e579bf..a5b564cb 100644 --- a/kfet/tests/test_views.py +++ b/kfet/tests/test_views.py @@ -209,14 +209,14 @@ class AccountReadViewTests(ViewTestCaseMixin, TestCase): auth_user = "team" auth_forbidden = [None, "user"] - # Forbidden users should get a 404 here, to avoid leaking trigrams + # Users with forbidden access users should get a 404 here, to avoid leaking trigrams # See issue #224 def test_forbidden(self): for user in self.auth_forbidden: - self.check_forbidden(user, self.url_expected) - self.check_forbidden(user, "/k-fet/accounts/NEX") + self.assertRedirectsToLoginOr404(user, self.url_expected) + self.assertRedirectsToLoginOr404(user, "/k-fet/accounts/NEX") - def check_forbidden(self, user, url): + def assertRedirectsToLoginOr404(self, user, url): client = Client() if user is None: response = client.get(url) @@ -315,15 +315,17 @@ class AccountUpdateViewTests(ViewTestCaseMixin, TestCase): "team1": create_team("team1", "101", perms=["kfet.change_account"]), } - # Forbidden users should get a 404 here, to avoid leaking trigrams + # Users with forbidden access users should get a 404 here, to avoid leaking trigrams # See issue #224 def test_forbidden(self): for method in ["get", "post"]: for user in self.auth_forbidden: - self.check_forbidden(user, method, self.url_expected) - self.check_forbidden(user, method, "/k-fet/accounts/NEX/edit") + self.assertRedirectsToLoginOr404(user, method, self.url_expected) + self.assertRedirectsToLoginOr404( + user, method, "/k-fet/accounts/NEX/edit" + ) - def check_forbidden(self, user, method, url): + def assertRedirectsToLoginOr404(self, user, method, url): client = Client() meth = getattr(client, method) if user is None: @@ -595,14 +597,16 @@ class AccountStatOperationListViewTests(ViewTestCaseMixin, TestCase): def get_users_extra(self): return {"user1": create_user("user1", "001")} - # Forbidden users should get a 404 here, to avoid leaking trigrams + # Users with forbidden access users should get a 404 here, to avoid leaking trigrams # See issue #224 def test_forbidden(self): for user in self.auth_forbidden: - self.check_forbidden(user, self.url_expected) - self.check_forbidden(user, "/k-fet/accounts/NEX/stat/operations/list") + self.assertRedirectsToLoginOr404(user, self.url_expected) + self.assertRedirectsToLoginOr404( + user, "/k-fet/accounts/NEX/stat/operations/list" + ) - def check_forbidden(self, user, url): + def assertRedirectsToLoginOr404(self, user, url): client = Client() if user is None: response = client.get(url) @@ -675,14 +679,16 @@ class AccountStatOperationViewTests(ViewTestCaseMixin, TestCase): auth_user = "user1" auth_forbidden = [None, "user", "team"] - # Forbidden users should get a 404 here, to avoid leaking trigrams + # Users with forbidden access users should get a 404 here, to avoid leaking trigrams # See issue #224 def test_forbidden(self): for user in self.auth_forbidden: - self.check_forbidden(user, self.url_expected) - self.check_forbidden(user, "/k-fet/accounts/NEX/stat/operations") + self.assertRedirectsToLoginOr404(user, self.url_expected) + self.assertRedirectsToLoginOr404( + user, "/k-fet/accounts/NEX/stat/operations" + ) - def check_forbidden(self, user, url): + def assertRedirectsToLoginOr404(self, user, url): client = Client() if user is None: response = client.get(url) @@ -710,14 +716,16 @@ class AccountStatBalanceListViewTests(ViewTestCaseMixin, TestCase): auth_user = "user1" auth_forbidden = [None, "user", "team"] - # Forbidden users should get a 404 here, to avoid leaking trigrams + # Users with forbidden access users should get a 404 here, to avoid leaking trigrams # See issue #224 def test_forbidden(self): for user in self.auth_forbidden: - self.check_forbidden(user, self.url_expected) - self.check_forbidden(user, "/k-fet/accounts/NEX/stat/balance/list") + self.assertRedirectsToLoginOr404(user, self.url_expected) + self.assertRedirectsToLoginOr404( + user, "/k-fet/accounts/NEX/stat/balance/list" + ) - def check_forbidden(self, user, url): + def assertRedirectsToLoginOr404(self, user, url): client = Client() if user is None: response = client.get(url) @@ -774,14 +782,14 @@ class AccountStatBalanceViewTests(ViewTestCaseMixin, TestCase): auth_user = "user1" auth_forbidden = [None, "user", "team"] - # Forbidden users should get a 404 here, to avoid leaking trigrams + # Users with forbidden access users should get a 404 here, to avoid leaking trigrams # See issue #224 def test_forbidden(self): for user in self.auth_forbidden: - self.check_forbidden(user, self.url_expected) - self.check_forbidden(user, "/k-fet/accounts/NEX/stat/balance") + self.assertRedirectsToLoginOr404(user, self.url_expected) + self.assertRedirectsToLoginOr404(user, "/k-fet/accounts/NEX/stat/balance") - def check_forbidden(self, user, url): + def assertRedirectsToLoginOr404(self, user, url): client = Client() if user is None: response = client.get(url)