Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix mp security by Aabu #6403

Merged
merged 1 commit into from
Oct 19, 2022
Merged

Fix mp security by Aabu #6403

merged 1 commit into from
Oct 19, 2022

Conversation

artragis
Copy link
Member

Security breach : voici le rapport

Sous ces conditions :

  • il faut être connecté ;
  • il faut connaître l'ID d'un MP et son slug ;
  • il faut que la personne soit seule dans le MP cible ;

alors, il est possible de faire quitter le MP à la personne alors qu'on n'est pas cette personne.

J'ai pu le faire en rejouant la requête envoyée lors qu'on quitte le MP, ce qui a eu pour effet de faire quitter l'autre personne. On n'est pas obligé d'être un ancien participant, on peut reprendre n'importe quelle requête qui quitte un MP et changer la cible.

La faille a été fix sur beta et dev avec le code joint.

@coveralls
Copy link

coveralls commented Oct 18, 2022

Coverage Status

Coverage increased (+0.001%) to 88.318% when pulling 6401fa6 on artragis:fix_mps into e78c57a on zestedesavoir:dev.

@philippemilink philippemilink added this to En développement in Suivi des PR via automation Oct 18, 2022
@philippemilink philippemilink moved this from En développement to En attente de QA in Suivi des PR Oct 18, 2022
@philippemilink
Copy link
Member

Merci pour le fix (et son déploiement !)

Est-ce que tu peux ajouter un test pour ton fix et qui s'assure qu'on n'aura pas de régression, stp ?

Situphen
Situphen previously approved these changes Oct 18, 2022
Copy link
Member

@Situphen Situphen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cela corrige bien la faille de sécurité que tu as détaillé !

zds/mp/views.py Outdated Show resolved Hide resolved
Suivi des PR automation moved this from En attente de QA to Fusionnable après rebase Oct 18, 2022
@Situphen
Copy link
Member

J'avais envie donc j'ai écrit le test en question :

diff --git a/zds/mp/tests/tests_views.py b/zds/mp/tests/tests_views.py
index f1f1886d9..f3700289c 100644
--- a/zds/mp/tests/tests_views.py
+++ b/zds/mp/tests/tests_views.py
@@ -625,6 +625,7 @@ class LeaveViewTest(TestCase):
     def setUp(self):
         self.profile1 = ProfileFactory()
         self.profile2 = ProfileFactory()
+        self.profile3 = ProfileFactory()
 
         self.anonymous_account = UserFactory(username=settings.ZDS_APP["member"]["anonymous_account"])
         self.bot_group = Group()
@@ -650,6 +651,14 @@ class LeaveViewTest(TestCase):
             response, reverse("member-login") + "?next=" + reverse("mp:leave", args=[1, "private-topic"])
         )
 
+    def test_denies_leave_topic_as_random_member(self):
+        self.client.force_login(self.profile3.user)
+
+        response = self.client.post(reverse("mp:leave", args=[self.topic1.pk, self.topic1.slug()]), follow=True)
+
+        self.assertEqual(403, response.status_code)
+        self.assertEqual(1, PrivateTopic.objects.filter(pk=self.topic1.pk).count())
+
     def test_fail_leave_topic_no_exist(self):
 
         response = self.client.post(reverse("mp:leave", args=[999, "private-topic"]))

Suivi des PR automation moved this from Fusionnable après rebase to Modification demandée Oct 18, 2022
@artragis
Copy link
Member Author

Merci pour le test, c'est intégré.

@Arnaud-D Arnaud-D enabled auto-merge (squash) October 19, 2022 09:54
Copy link
Contributor

@Arnaud-D Arnaud-D left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QA OK ✔️

(J'ai fumé la moquette sur le message de commit cependant, mais trop tard ! :D)

@Arnaud-D Arnaud-D merged commit 2bc47e7 into zestedesavoir:dev Oct 19, 2022
Suivi des PR automation moved this from Modification demandée to Fusionnable après rebase Oct 19, 2022
Suivi des PR automation moved this from Fusionnable après rebase to Fusionnée Oct 19, 2022
@Arnaud-D Arnaud-D added C-Back Concerne le back-end Django hacktoberfest-accepted Pull request approuvée pour le Hacktoberfest labels Oct 20, 2022
@philippemilink philippemilink removed this from Fusionnée in Suivi des PR Dec 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-Back Concerne le back-end Django hacktoberfest-accepted Pull request approuvée pour le Hacktoberfest
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants