From c74f8363e21e117bff8e2a72273580e2c2f9f5a1 Mon Sep 17 00:00:00 2001
From: Mateusz Mandera <mateusz.mandera@zulip.com>
Date: Fri, 22 May 2020 15:26:17 +0200
Subject: [PATCH] saml: Gracefully handle bad SAMLResponses.

---
 zerver/tests/test_auth_backends.py | 30 ++++++++++++++++++++++++++----
 zproject/backends.py               |  8 +++++---
 2 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/zerver/tests/test_auth_backends.py b/zerver/tests/test_auth_backends.py
index f07e79a06c..1b974211bc 100644
--- a/zerver/tests/test_auth_backends.py
+++ b/zerver/tests/test_auth_backends.py
@@ -1543,10 +1543,32 @@ class SAMLAuthBackendTest(SocialAuthBase):
             result = self.client_post('/complete/saml/',  post_params)
             self.assertEqual(result.status_code, 302)
             self.assertIn('login', result.url)
-            m.assert_called_with(
-                # OneLogin_Saml2_Error exception:
-                "SAML Response not found, Only supported HTTP_POST Binding"
-            )
+            m.assert_called_once()
+
+        # Now test bad SAMLResponses.
+        with mock.patch('zproject.backends.logging.info') as m:
+            relay_state = SAMLAuthBackend.put_data_in_redis({"idp": "test_idp", "subdomain": "zulip"})
+            post_params = {"RelayState": relay_state, 'SAMLResponse': ''}
+            result = self.client_post('/complete/saml/',  post_params)
+            self.assertEqual(result.status_code, 302)
+            self.assertIn('login', result.url)
+            m.assert_called_once()
+
+        with mock.patch('zproject.backends.logging.info') as m:
+            relay_state = SAMLAuthBackend.put_data_in_redis({"idp": "test_idp", "subdomain": "zulip"})
+            post_params = {"RelayState": relay_state, 'SAMLResponse': 'b'}
+            result = self.client_post('/complete/saml/',  post_params)
+            self.assertEqual(result.status_code, 302)
+            self.assertIn('login', result.url)
+            m.assert_called_once()
+
+        with mock.patch('zproject.backends.logging.info') as m:
+            relay_state = SAMLAuthBackend.put_data_in_redis({"idp": "test_idp", "subdomain": "zulip"})
+            post_params = {"RelayState": relay_state, 'SAMLResponse': 'dGVzdA=='}  # base64 encoded 'test'
+            result = self.client_post('/complete/saml/',  post_params)
+            self.assertEqual(result.status_code, 302)
+            self.assertIn('login', result.url)
+            m.assert_called_once()
 
         with mock.patch('zproject.backends.logging.info') as m:
             relay_state = SAMLAuthBackend.put_data_in_redis({"idp": "test_idp", "subdomain": "zulip"})
diff --git a/zproject/backends.py b/zproject/backends.py
index 9e20ac44ef..46bc358bfc 100644
--- a/zproject/backends.py
+++ b/zproject/backends.py
@@ -12,6 +12,7 @@
 # call the authenticate methods of all backends registered in
 # settings.AUTHENTICATION_BACKENDS that have a function signature
 # matching the args/kwargs passed in the authenticate() call.
+import binascii
 import copy
 import logging
 import magic
@@ -35,6 +36,7 @@ from django.http import HttpResponse, HttpResponseRedirect, HttpRequest
 from django.shortcuts import render
 from django.urls import reverse
 from django.utils.translation import ugettext as _
+from lxml.etree import XMLSyntaxError
 from requests import HTTPError
 from onelogin.saml2.errors import OneLogin_Saml2_Error
 from social_core.backends.github import GithubOAuth2, GithubOrganizationOAuth2, \
@@ -1615,9 +1617,9 @@ class SAMLAuthBackend(SocialAuthMixin, SAMLAuth):
 
             # Call the auth_complete method of SocialAuthMixIn
             result = super().auth_complete(*args, **kwargs)
-        except OneLogin_Saml2_Error as e:
-            # This will be raised if SAMLResponse is missing.
-            logging.info(str(e))
+        except (OneLogin_Saml2_Error, binascii.Error, XMLSyntaxError) as e:
+            # These can be raised if SAMLResponse is missing or badly formatted.
+            logging.info("/complete/saml/: %s: %s", e.__class__.__name__, e)
             # Fall through to returning None.
         finally:
             if result is None: