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.
SAML-Toolkits · Jul 15, 2020 · b4c7cb9 · b4c7cb9
1 parent c660f6f
commit b4c7cb9
Show file tree

Hide file tree

Showing 3 changed files with 77 additions and 1 deletion.
diff --git 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/settings10.json b/tests/settings/settings10.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": "[email protected]"
+        },
+        "support": {
+            "givenName": "support_name",
+            "emailAddress": "[email protected]"
+        }
+    },
+    "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
@@ -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='settings10.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.