From 17bf0d109ee9dd764f79a0f61d24b643ea808eb2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?P=C4=93teris=20Caune?= Date: Fri, 30 Dec 2016 18:28:32 +0200 Subject: [PATCH] Fix CSRF in Slack, Pushbullet and Discord callbacks --- hc/front/tests/test_add_discord.py | 23 ++++++++++++- hc/front/tests/test_add_pushbullet.py | 23 ++++++++++++- hc/front/tests/test_add_slack_btn.py | 29 ++++++++++++++-- hc/front/views.py | 48 +++++++++++++++++++++------ templates/front/welcome.html | 6 ++-- templates/integrations/add_slack.html | 2 +- 6 files changed, 113 insertions(+), 18 deletions(-) diff --git a/hc/front/tests/test_add_discord.py b/hc/front/tests/test_add_discord.py index 4eab11fe..dbdbcec7 100644 --- a/hc/front/tests/test_add_discord.py +++ b/hc/front/tests/test_add_discord.py @@ -16,6 +16,9 @@ class AddDiscordTestCase(BaseTestCase): self.assertContains(r, "Connect Discord", status_code=200) self.assertContains(r, "discordapp.com/api/oauth2/authorize") + # There should now be a key in session + self.assertTrue("discord" in self.client.session) + @override_settings(DISCORD_CLIENT_ID=None) def test_it_requires_client_id(self): self.client.login(username="alice@example.org", password="password") @@ -24,6 +27,10 @@ class AddDiscordTestCase(BaseTestCase): @patch("hc.front.views.requests.post") def test_it_handles_oauth_response(self, mock_post): + session = self.client.session + session["discord"] = "foo" + session.save() + oauth_response = { "access_token": "test-token", "webhook": { @@ -35,7 +42,7 @@ class AddDiscordTestCase(BaseTestCase): mock_post.return_value.text = json.dumps(oauth_response) mock_post.return_value.json.return_value = oauth_response - url = self.url + "?code=12345678" + url = self.url + "?code=12345678&state=foo" self.client.login(username="alice@example.org", password="password") r = self.client.get(url, follow=True) @@ -44,3 +51,17 @@ class AddDiscordTestCase(BaseTestCase): ch = Channel.objects.get() self.assertEqual(ch.discord_webhook_url, "foo") + + # Session should now be clean + self.assertFalse("discord" in self.client.session) + + def test_it_avoids_csrf(self): + session = self.client.session + session["discord"] = "foo" + session.save() + + url = self.url + "?code=12345678&state=bar" + + self.client.login(username="alice@example.org", password="password") + r = self.client.get(url) + self.assertEqual(r.status_code, 400) diff --git a/hc/front/tests/test_add_pushbullet.py b/hc/front/tests/test_add_pushbullet.py index c7ab0211..6356a046 100644 --- a/hc/front/tests/test_add_pushbullet.py +++ b/hc/front/tests/test_add_pushbullet.py @@ -16,6 +16,9 @@ class AddPushbulletTestCase(BaseTestCase): self.assertContains(r, "www.pushbullet.com/authorize", status_code=200) self.assertContains(r, "Connect Pushbullet") + # There should now be a key in session + self.assertTrue("pushbullet" in self.client.session) + @override_settings(PUSHBULLET_CLIENT_ID=None) def test_it_requires_client_id(self): self.client.login(username="alice@example.org", password="password") @@ -24,12 +27,16 @@ class AddPushbulletTestCase(BaseTestCase): @patch("hc.front.views.requests.post") def test_it_handles_oauth_response(self, mock_post): + session = self.client.session + session["pushbullet"] = "foo" + session.save() + oauth_response = {"access_token": "test-token"} mock_post.return_value.text = json.dumps(oauth_response) mock_post.return_value.json.return_value = oauth_response - url = self.url + "?code=12345678" + url = self.url + "?code=12345678&state=foo" self.client.login(username="alice@example.org", password="password") r = self.client.get(url, follow=True) @@ -38,3 +45,17 @@ class AddPushbulletTestCase(BaseTestCase): ch = Channel.objects.get() self.assertEqual(ch.value, "test-token") + + # Session should now be clean + self.assertFalse("pushbullet" in self.client.session) + + def test_it_avoids_csrf(self): + session = self.client.session + session["pushbullet"] = "foo" + session.save() + + url = self.url + "?code=12345678&state=bar" + + self.client.login(username="alice@example.org", password="password") + r = self.client.get(url) + self.assertEqual(r.status_code, 400) diff --git a/hc/front/tests/test_add_slack_btn.py b/hc/front/tests/test_add_slack_btn.py index e82a0e7f..a564ab34 100644 --- a/hc/front/tests/test_add_slack_btn.py +++ b/hc/front/tests/test_add_slack_btn.py @@ -14,6 +14,9 @@ class AddSlackBtnTestCase(BaseTestCase): self.assertContains(r, "Before adding Slack integration", status_code=200) + # There should now be a key in session + self.assertTrue("slack" in self.client.session) + @override_settings(SLACK_CLIENT_ID="foo") def test_slack_button(self): self.client.login(username="alice@example.org", password="password") @@ -22,6 +25,10 @@ class AddSlackBtnTestCase(BaseTestCase): @patch("hc.front.views.requests.post") def test_it_handles_oauth_response(self, mock_post): + session = self.client.session + session["slack"] = "foo" + session.save() + oauth_response = { "ok": True, "team_name": "foo", @@ -34,7 +41,7 @@ class AddSlackBtnTestCase(BaseTestCase): mock_post.return_value.text = json.dumps(oauth_response) mock_post.return_value.json.return_value = oauth_response - url = "/integrations/add_slack_btn/?code=12345678" + url = "/integrations/add_slack_btn/?code=12345678&state=foo" self.client.login(username="alice@example.org", password="password") r = self.client.get(url, follow=True) @@ -46,8 +53,26 @@ class AddSlackBtnTestCase(BaseTestCase): self.assertEqual(ch.slack_channel, "bar") self.assertEqual(ch.slack_webhook_url, "http://example.org") + # Session should now be clean + self.assertFalse("slack" in self.client.session) + + def test_it_avoids_csrf(self): + session = self.client.session + session["slack"] = "foo" + session.save() + + url = "/integrations/add_slack_btn/?code=12345678&state=bar" + + self.client.login(username="alice@example.org", password="password") + r = self.client.get(url) + self.assertEqual(r.status_code, 400) + @patch("hc.front.views.requests.post") def test_it_handles_oauth_error(self, mock_post): + session = self.client.session + session["slack"] = "foo" + session.save() + oauth_response = { "ok": False, "error": "something went wrong" @@ -56,7 +81,7 @@ class AddSlackBtnTestCase(BaseTestCase): mock_post.return_value.text = json.dumps(oauth_response) mock_post.return_value.json.return_value = oauth_response - url = "/integrations/add_slack_btn/?code=12345678" + url = "/integrations/add_slack_btn/?code=12345678&state=foo" self.client.login(username="alice@example.org", password="password") r = self.client.get(url, follow=True) diff --git a/hc/front/views.py b/hc/front/views.py index fc58a973..4492ae8c 100644 --- a/hc/front/views.py +++ b/hc/front/views.py @@ -91,7 +91,9 @@ def index(request): "page": "welcome", "check": check, "ping_url": check.url(), - "enable_pushover": settings.PUSHOVER_API_TOKEN is not None + "enable_pushbullet": settings.PUSHBULLET_CLIENT_ID is not None, + "enable_pushover": settings.PUSHOVER_API_TOKEN is not None, + "enable_discord": settings.DISCORD_CLIENT_ID is not None } return render(request, "front/welcome.html", ctx) @@ -430,6 +432,24 @@ def add_pd(request): return render(request, "integrations/add_pd.html", ctx) +def _prepare_state(request, session_key): + state = get_random_string() + request.session[session_key] = state + return state + + +def _get_validated_code(request, session_key): + if session_key not in request.session: + return None + + session_state = request.session.pop(session_key) + request_state = request.GET.get("state") + if session_state is None or session_state != request_state: + return None + + return request.GET.get("code") + + def add_slack(request): if not settings.SLACK_CLIENT_ID and not request.user.is_authenticated: return redirect("hc-login") @@ -452,13 +472,16 @@ def add_slack(request): "slack_client_id": settings.SLACK_CLIENT_ID } + if settings.SLACK_CLIENT_ID: + ctx["state"] = _prepare_state(request, "slack") + return render(request, "integrations/add_slack.html", ctx) @login_required def add_slack_btn(request): - code = request.GET.get("code", "") - if len(code) < 8: + code = _get_validated_code(request, "slack") + if code is None: return HttpResponseBadRequest() result = requests.post("https://slack.com/api/oauth.access", { @@ -507,8 +530,8 @@ def add_pushbullet(request): raise Http404("pushbullet integration is not available") if "code" in request.GET: - code = request.GET.get("code", "") - if len(code) < 8: + code = _get_validated_code(request, "pushbullet") + if code is None: return HttpResponseBadRequest() result = requests.post("https://api.pushbullet.com/oauth2/token", { @@ -536,7 +559,8 @@ def add_pushbullet(request): authorize_url = "https://www.pushbullet.com/authorize?" + urlencode({ "client_id": settings.PUSHBULLET_CLIENT_ID, "redirect_uri": redirect_uri, - "response_type": "code" + "response_type": "code", + "state": _prepare_state(request, "pushbullet") }) ctx = { @@ -553,8 +577,8 @@ def add_discord(request): redirect_uri = settings.SITE_ROOT + reverse("hc-add-discord") if "code" in request.GET: - code = request.GET.get("code", "") - if len(code) < 8: + code = _get_validated_code(request, "discord") + if code is None: return HttpResponseBadRequest() result = requests.post("https://discordapp.com/api/oauth2/token", { @@ -579,17 +603,19 @@ def add_discord(request): return redirect("hc-channels") - authorize_url = "https://discordapp.com/api/oauth2/authorize?" + urlencode({ + auth_url = "https://discordapp.com/api/oauth2/authorize?" + urlencode({ "client_id": settings.DISCORD_CLIENT_ID, "scope": "webhook.incoming", "redirect_uri": redirect_uri, - "response_type": "code" + "response_type": "code", + "state": _prepare_state(request, "discord") }) ctx = { "page": "channels", - "authorize_url": authorize_url + "authorize_url": auth_url } + return render(request, "integrations/add_discord.html", ctx) diff --git a/templates/front/welcome.html b/templates/front/welcome.html index 65ce1d5b..a4207209 100644 --- a/templates/front/welcome.html +++ b/templates/front/welcome.html @@ -248,12 +248,14 @@ Notifications in Slack channel. + {% if enable_discord %} Discord icon Notifications in Discord channel. + {% endif %} @@ -283,14 +285,14 @@ Open and resolve incidents in VictorOps. - - + {% if enable_pushbullet %} Pushbullet icon Instant push notifications with Pushbullet. + {% endif %} {% if enable_pushover %} diff --git a/templates/integrations/add_slack.html b/templates/integrations/add_slack.html index 0738bbbd..5d27ed50 100644 --- a/templates/integrations/add_slack.html +++ b/templates/integrations/add_slack.html @@ -16,7 +16,7 @@ Slack channel.

- + Add to Slack