From 3743a39700a77b9dcf87b9d884d0cb2115e29279 Mon Sep 17 00:00:00 2001
From: Paul Logston <paul.logston@gmail.com>
Date: Thu, 16 Jul 2020 00:40:47 +0800
Subject: [PATCH] Escape ampersands when creating AuthnRequests

In some cases, the IdP singleSignOnService or singleLogoutService url
values will contain a query string and that query string will contain
an ampersand ("&"). If not escaped, this ampersand will prevent valid
XML creation by python3-saml and thus the IdP will not be able to
correctly parse the received AuthnRequest.
---
 src/onelogin/saml2/authn_request.py           |  2 +-
 tests/settings/settings11.json                | 47 +++++++++++++++++++
 .../saml2_tests/authn_request_test.py         | 29 ++++++++++++
 3 files changed, 77 insertions(+), 1 deletion(-)
 create mode 100644 tests/settings/settings11.json

diff --git a/src/onelogin/saml2/authn_request.py b/src/onelogin/saml2/authn_request.py
index 73547735..b279bd98 100644
--- a/src/onelogin/saml2/authn_request.py
+++ b/src/onelogin/saml2/authn_request.py
@@ -51,7 +51,7 @@ def __init__(self, settings, force_authn=False, is_passive=False, set_nameid_pol
         self.__id = uid
         issue_instant = OneLogin_Saml2_Utils.parse_time_to_SAML(OneLogin_Saml2_Utils.now())
 
-        destination = idp_data['singleSignOnService']['url']
+        destination = idp_data['singleSignOnService']['url'].replace("&", "&amp;")
 
         provider_name_str = ''
         organization_data = settings.get_organization()
diff --git a/tests/settings/settings11.json b/tests/settings/settings11.json
new file mode 100644
index 00000000..2c134401
--- /dev/null
+++ b/tests/settings/settings11.json
@@ -0,0 +1,47 @@
+{
+    "strict": false,
+    "debug": false,
+    "custom_base_path": "../../../tests/data/customPath/",
+    "sp": {
+        "entityId": "http://stuff.com/endpoints/metadata.php",
+        "assertionConsumerService": {
+            "url": "http://stuff.com/endpoints/endpoints/acs.php"
+        },
+        "singleLogoutService": {
+            "url": "http://stuff.com/endpoints/endpoints/sls.php"
+        },
+        "NameIDFormat": "urn:oasis:names:tc:SAML:1.1:nameid-format:unspecified"
+    },
+    "idp": {
+        "entityId": "http://idp.example.com/",
+        "singleSignOnService": {
+            "url": "http://idp.example.com/SSOService.php?arg1=a&arg2=b"
+        },
+        "singleLogoutService": {
+            "url": "http://idp.example.com/SingleLogoutService.php?arg1=a&arg2=b"
+        },
+        "x509cert": "MIICgTCCAeoCCQCbOlrWDdX7FTANBgkqhkiG9w0BAQUFADCBhDELMAkGA1UEBhMCTk8xGDAWBgNVBAgTD0FuZHJlYXMgU29sYmVyZzEMMAoGA1UEBxMDRm9vMRAwDgYDVQQKEwdVTklORVRUMRgwFgYDVQQDEw9mZWlkZS5lcmxhbmcubm8xITAfBgkqhkiG9w0BCQEWEmFuZHJlYXNAdW5pbmV0dC5ubzAeFw0wNzA2MTUxMjAxMzVaFw0wNzA4MTQxMjAxMzVaMIGEMQswCQYDVQQGEwJOTzEYMBYGA1UECBMPQW5kcmVhcyBTb2xiZXJnMQwwCgYDVQQHEwNGb28xEDAOBgNVBAoTB1VOSU5FVFQxGDAWBgNVBAMTD2ZlaWRlLmVybGFuZy5ubzEhMB8GCSqGSIb3DQEJARYSYW5kcmVhc0B1bmluZXR0Lm5vMIGfMA0GCSqGSIb3DQEBAQUAA4GNADCBiQKBgQDivbhR7P516x/S3BqKxupQe0LONoliupiBOesCO3SHbDrl3+q9IbfnfmE04rNuMcPsIxB161TdDpIesLCn7c8aPHISKOtPlAeTZSnb8QAu7aRjZq3+PbrP5uW3TcfCGPtKTytHOge/OlJbo078dVhXQ14d1EDwXJW1rRXuUt4C8QIDAQABMA0GCSqGSIb3DQEBBQUAA4GBACDVfp86HObqY+e8BUoWQ9+VMQx1ASDohBjwOsg2WykUqRXF+dLfcUH9dWR63CtZIKFDbStNomPnQz7nbK+onygwBspVEbnHuUihZq3ZUdmumQqCw4Uvs/1Uvq3orOo/WJVhTyvLgFVK2QarQ4/67OZfHd7R+POBXhophSMv1ZOo"
+    },
+    "security": {
+        "authnRequestsSigned": false,
+        "wantAssertionsSigned": false,
+        "signMetadata": false
+    },
+    "contactPerson": {
+        "technical": {
+            "givenName": "technical_name",
+            "emailAddress": "technical@example.com"
+        },
+        "support": {
+            "givenName": "support_name",
+            "emailAddress": "support@example.com"
+        }
+    },
+    "organization": {
+        "en-US": {
+            "name": "sp_test",
+            "displayname": "SP test",
+            "url": "http://sp.example.com"
+        }
+    }
+}
diff --git a/tests/src/OneLogin/saml2_tests/authn_request_test.py b/tests/src/OneLogin/saml2_tests/authn_request_test.py
index 3f1262f2..bc160e07 100644
--- a/tests/src/OneLogin/saml2_tests/authn_request_test.py
+++ b/tests/src/OneLogin/saml2_tests/authn_request_test.py
@@ -6,6 +6,7 @@
 import json
 from os.path import dirname, join, exists
 import unittest
+import xml.etree.ElementTree as ET
 
 from onelogin.saml2 import compat
 from onelogin.saml2.authn_request import OneLogin_Saml2_Authn_Request
@@ -305,6 +306,34 @@ def testCreateDeflatedSAMLRequestURLParameter(self):
         inflated = compat.to_string(OneLogin_Saml2_Utils.decode_base64_and_inflate(payload))
         self.assertRegex(inflated, '^<samlp:AuthnRequest')
 
+    def testSAMLRequestURLParameterWithAmpersand(self):
+        """
+        Tests the OneLogin_Saml2_Authn_Request Constructor.
+        Test that ampersand
+        """
+        settings = OneLogin_Saml2_Settings(self.loadSettingsJSON(name='settings11.json'))
+        authn_request = OneLogin_Saml2_Authn_Request(settings)
+        parameters = {
+            'SAMLRequest': authn_request.get_request()
+        }
+        auth_url = OneLogin_Saml2_Utils.redirect('http://idp.example.com/SSOService.php', parameters, True)
+        self.assertRegex(auth_url, r'^http://idp\.example\.com\/SSOService\.php\?SAMLRequest=')
+        exploded = urlparse(auth_url)
+        exploded = parse_qs(exploded[4])
+        payload = exploded['SAMLRequest'][0]
+        inflated = compat.to_string(OneLogin_Saml2_Utils.decode_base64_and_inflate(payload))
+        self.assertRegex(inflated, '^<samlp:AuthnRequest')
+
+        try:
+            root = ET.fromstring(inflated)
+        except ET.ParseError:
+            self.fail('Unable to parse AuthnRequest')
+
+        destination = root.get('Destination')
+        # This URL includes an un-escaped ampersand as should be the case after parsing the XML.
+        url = 'http://idp.example.com/SSOService.php?arg1=a&arg2=b'
+        self.assertEqual(url, destination)
+
     def testCreateEncSAMLRequest(self):
         """
         Tests the OneLogin_Saml2_Authn_Request Constructor.