From d92a64b8f35bbbf5cb21d434f8966eac708840bf Mon Sep 17 00:00:00 2001 From: Fredrik Sundblom Date: Thu, 22 Apr 2021 16:57:44 +0200 Subject: [PATCH 1/8] Wrapped loadXML method within a try-catch and return false instead of throwing an error. --- src/Saml2/Utils.php | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/Saml2/Utils.php b/src/Saml2/Utils.php index 582c117b..ce78324c 100644 --- a/src/Saml2/Utils.php +++ b/src/Saml2/Utils.php @@ -87,7 +87,11 @@ public static function loadXML(DOMDocument $dom, $xml) $oldEntityLoader = libxml_disable_entity_loader(true); } - $res = $dom->loadXML($xml); + try { + $res = $dom->loadXML($xml); + } catch (\Exception $e) { + return false; + } if (PHP_VERSION_ID < 80000) { libxml_disable_entity_loader($oldEntityLoader); From b8561d9b0007118715c50263362a6ef282df86c7 Mon Sep 17 00:00:00 2001 From: Fredrik Sundblom Date: Thu, 22 Apr 2021 17:41:18 +0200 Subject: [PATCH 2/8] Fixed broken tests --- tests/src/OneLogin/Saml2/AuthTest.php | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/tests/src/OneLogin/Saml2/AuthTest.php b/tests/src/OneLogin/Saml2/AuthTest.php index f0bf65b1..a44358f1 100644 --- a/tests/src/OneLogin/Saml2/AuthTest.php +++ b/tests/src/OneLogin/Saml2/AuthTest.php @@ -777,7 +777,7 @@ public function testProcessSLORequestRelayState() $_GET['RelayState'] = 'http://relaystate.com'; $this->_auth->setStrict(true); - $targetUrl = $this->_auth->processSLO(false, null, fase, null, null, true); + $targetUrl = $this->_auth->processSLO(false, null, false, null, null, true); $parsedQuery = getParamsFromUrl($targetUrl); $sloResponseUrl = $this->_settingsInfo['idp']['singleLogoutService']['responseUrl']; @@ -815,7 +815,7 @@ public function testProcessSLORequestSignedResponse() $_GET['RelayState'] = 'http://relaystate.com'; $auth->setStrict(true); - $targetUrl = $this->_auth->processSLO(false, null, fase, null, null, true); + $targetUrl = $this->_auth->processSLO(false, null, false, null, null, true); $parsedQuery = getParamsFromUrl($targetUrl); @@ -918,7 +918,7 @@ public function testLoginSigned() $this->assertArrayHasKey('SigAlg', $parsedQuery); $this->assertArrayHasKey('Signature', $parsedQuery); $this->assertEquals($parsedQuery['RelayState'], $returnTo); - $this->assertEquals(XMLSecurityKey::RSA_SHA1, $parsedQuery['SigAlg']); + $this->assertEquals(XMLSecurityKey::RSA_SHA256, $parsedQuery['SigAlg']); } /** @@ -946,7 +946,7 @@ public function testLoginForceAuthN() $encodedRequest = $parsedQuery['SAMLRequest']; $decoded = base64_decode($encodedRequest); $request = gzinflate($decoded); - $this->assertNotContains('ForceAuthn="true"', $request); + $this->assertStringNotContainsString('ForceAuthn="true"', $request); $returnTo = 'http://example.com/returnto'; @@ -959,7 +959,7 @@ public function testLoginForceAuthN() $encodedRequest2 = $parsedQuery2['SAMLRequest']; $decoded2 = base64_decode($encodedRequest2); $request2 = gzinflate($decoded2); - $this->assertNotContains('ForceAuthn="true"', $request2); + $this->assertStringNotContainsString('ForceAuthn="true"', $request2); $returnTo = 'http://example.com/returnto'; $targetUrl3 = $auth->login($returnTo, [], true, false, true); @@ -1000,7 +1000,7 @@ public function testLoginIsPassive() $encodedRequest = $parsedQuery['SAMLRequest']; $decoded = base64_decode($encodedRequest); $request = gzinflate($decoded); - $this->assertNotContains('IsPassive="true"', $request); + $this->assertStringNotContainsString('IsPassive="true"', $request); $returnTo = 'http://example.com/returnto'; $targetUrl2 = $auth->login($returnTo, [], false, false, true); @@ -1012,7 +1012,7 @@ public function testLoginIsPassive() $encodedRequest2 = $parsedQuery2['SAMLRequest']; $decoded2 = base64_decode($encodedRequest2); $request2 = gzinflate($decoded2); - $this->assertNotContains('IsPassive="true"', $request2); + $this->assertStringNotContainsString('IsPassive="true"', $request2); $returnTo = 'http://example.com/returnto'; $targetUrl3 = $auth->login($returnTo, [], false, true, true); @@ -1048,7 +1048,7 @@ public function testLoginNameIDPolicy() $encodedRequest = $parsedQuery['SAMLRequest']; $decoded = base64_decode($encodedRequest); $request = gzinflate($decoded); - $this->assertNotContains('assertStringNotContainsString('login($returnTo, [], false, false, true, true); @@ -1095,7 +1095,7 @@ public function testLoginSubject() $encodedRequest = $parsedQuery['SAMLRequest']; $decoded = base64_decode($encodedRequest); $request = gzinflate($decoded); - $this->assertNotContains('assertStringNotContainsString('login($returnTo, [], false, false, true, true, "testuser@example.com"); @@ -1114,7 +1114,7 @@ public function testLoginSubject() $returnTo = 'http://example.com/returnto'; $settingsInfo['sp']['NameIDFormat'] = "urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress"; $auth2 = new Auth($settingsInfo); - $targetUrl3 = $auth2->login($returnTo, [], false, false, true); + $targetUrl3 = $auth2->login($returnTo, [], false, false, true, false, "testuser@example.com"); $parsedQuery3 = getParamsFromUrl($targetUrl3); $ssoUrl3 = $settingsInfo['idp']['singleSignOnService']['url']; From 1935f592d8932aafc33a88d9f19aceba07f3c08f Mon Sep 17 00:00:00 2001 From: Fredrik Sundblom Date: Sat, 1 May 2021 11:20:45 +0200 Subject: [PATCH 3/8] Revert "Wrapped loadXML method within a try-catch and return false instead of throwing an error." This reverts commit d92a64b8f35bbbf5cb21d434f8966eac708840bf. --- src/Saml2/Utils.php | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/Saml2/Utils.php b/src/Saml2/Utils.php index ce78324c..582c117b 100644 --- a/src/Saml2/Utils.php +++ b/src/Saml2/Utils.php @@ -87,11 +87,7 @@ public static function loadXML(DOMDocument $dom, $xml) $oldEntityLoader = libxml_disable_entity_loader(true); } - try { - $res = $dom->loadXML($xml); - } catch (\Exception $e) { - return false; - } + $res = $dom->loadXML($xml); if (PHP_VERSION_ID < 80000) { libxml_disable_entity_loader($oldEntityLoader); From 6ae5b8e386dc9a9a4fea608cc6d9cfe2091334dc Mon Sep 17 00:00:00 2001 From: Fredrik Sundblom Date: Sat, 1 May 2021 11:30:36 +0200 Subject: [PATCH 4/8] Fixed unit test: Catch exception when providing invalid xml --- tests/src/OneLogin/Saml2/UtilsTest.php | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/tests/src/OneLogin/Saml2/UtilsTest.php b/tests/src/OneLogin/Saml2/UtilsTest.php index 5c8a0c4c..8a0e3853 100644 --- a/tests/src/OneLogin/Saml2/UtilsTest.php +++ b/tests/src/OneLogin/Saml2/UtilsTest.php @@ -31,8 +31,12 @@ public function testLoadXML() $dom = new DOMDocument(); $metadataUnloaded = ''; - $res1 = Utils::loadXML($dom, $metadataUnloaded); - $this->assertFalse($res1); + try { + $res1 = Utils::loadXML($dom, $metadataUnloaded); + $this->assertFalse($res1); + } catch (\Exception $e) { + $this->assertEquals('DOMDocument::loadXML(): Premature end of data in tag EntityDescriptor line 1 in Entity, line: 1', $e->getMessage()); + } $metadataInvalid = file_get_contents(TEST_ROOT .'/data/metadata/noentity_metadata_settings1.xml'); $res2 = Utils::loadXML($dom, $metadataInvalid); From 210cdb76483863af42eafe241c3497462400746c Mon Sep 17 00:00:00 2001 From: gr8b Date: Mon, 18 Dec 2023 16:24:41 -0600 Subject: [PATCH 5/8] fix: skip comments in .crt file --- src/Saml2/Utils.php | 23 +++++++++++------------ tests/certs/with.comment.crt | 17 +++++++++++++++++ tests/src/OneLogin/Saml2/UtilsTest.php | 5 +++++ 3 files changed, 33 insertions(+), 12 deletions(-) create mode 100644 tests/certs/with.comment.crt diff --git a/src/Saml2/Utils.php b/src/Saml2/Utils.php index 582c117b..101a8b8c 100644 --- a/src/Saml2/Utils.php +++ b/src/Saml2/Utils.php @@ -214,24 +214,23 @@ public static function treeCopyReplace(DomNode $targetNode, DomNode $sourceNode, /** * Returns a x509 cert (adding header & footer if required). * - * @param string $cert A x509 unformated cert - * @param bool $heads True if we want to include head and footer + * @param string $x509cert A x509 unformated cert + * @param bool $heads True if we want to include head and footer * * @return string $x509 Formatted cert */ - public static function formatCert($cert, $heads = true) + public static function formatCert($x509cert, $heads = true) { - $x509cert = str_replace(array("\x0D", "\r", "\n"), "", $cert); - if (!empty($x509cert)) { - $x509cert = str_replace('-----BEGIN CERTIFICATE-----', "", $x509cert); - $x509cert = str_replace('-----END CERTIFICATE-----', "", $x509cert); - $x509cert = str_replace(' ', '', $x509cert); - - if ($heads) { - $x509cert = "-----BEGIN CERTIFICATE-----\n".chunk_split($x509cert, 64, "\n")."-----END CERTIFICATE-----\n"; - } + if (strpos($x509cert, '-----BEGIN CERTIFICATE-----') !== false) { + $x509cert = static::getStringBetween($x509cert, '-----BEGIN CERTIFICATE-----', '-----END CERTIFICATE-----'); + } + $x509cert = strtr($x509cert, "\x0d\r\n ", ''); + + if ($heads && $x509cert !== '') { + $x509cert = "-----BEGIN CERTIFICATE-----\n".chunk_split($x509cert, 64, "\n")."-----END CERTIFICATE-----\n"; } + return $x509cert; } diff --git a/tests/certs/with.comment.crt b/tests/certs/with.comment.crt new file mode 100644 index 00000000..ed0e9729 --- /dev/null +++ b/tests/certs/with.comment.crt @@ -0,0 +1,17 @@ +# certificate comments should be ignored +-----BEGIN CERTIFICATE----- +MIICgTCCAeoCCQCbOlrWDdX7FTANBgkqhkiG9w0BAQUFADCBhDELMAkGA1UEBhMC +Tk8xGDAWBgNVBAgTD0FuZHJlYXMgU29sYmVyZzEMMAoGA1UEBxMDRm9vMRAwDgYD +VQQKEwdVTklORVRUMRgwFgYDVQQDEw9mZWlkZS5lcmxhbmcubm8xITAfBgkqhkiG +9w0BCQEWEmFuZHJlYXNAdW5pbmV0dC5ubzAeFw0wNzA2MTUxMjAxMzVaFw0wNzA4 +MTQxMjAxMzVaMIGEMQswCQYDVQQGEwJOTzEYMBYGA1UECBMPQW5kcmVhcyBTb2xi +ZXJnMQwwCgYDVQQHEwNGb28xEDAOBgNVBAoTB1VOSU5FVFQxGDAWBgNVBAMTD2Zl +aWRlLmVybGFuZy5ubzEhMB8GCSqGSIb3DQEJARYSYW5kcmVhc0B1bmluZXR0Lm5v +MIGfMA0GCSqGSIb3DQEBAQUAA4GNADCBiQKBgQDivbhR7P516x/S3BqKxupQe0LO +NoliupiBOesCO3SHbDrl3+q9IbfnfmE04rNuMcPsIxB161TdDpIesLCn7c8aPHIS +KOtPlAeTZSnb8QAu7aRjZq3+PbrP5uW3TcfCGPtKTytHOge/OlJbo078dVhXQ14d +1EDwXJW1rRXuUt4C8QIDAQABMA0GCSqGSIb3DQEBBQUAA4GBACDVfp86HObqY+e8 +BUoWQ9+VMQx1ASDohBjwOsg2WykUqRXF+dLfcUH9dWR63CtZIKFDbStNomPnQz7n +bK+onygwBspVEbnHuUihZq3ZUdmumQqCw4Uvs/1Uvq3orOo/WJVhTyvLgFVK2Qar +Q4/67OZfHd7R+POBXhophSMv1ZOo +-----END CERTIFICATE----- diff --git a/tests/src/OneLogin/Saml2/UtilsTest.php b/tests/src/OneLogin/Saml2/UtilsTest.php index 8a0e3853..f2f9c9bb 100644 --- a/tests/src/OneLogin/Saml2/UtilsTest.php +++ b/tests/src/OneLogin/Saml2/UtilsTest.php @@ -194,6 +194,11 @@ public function testFormatCert() $this->assertStringNotContainsString('-----END CERTIFICATE-----', $formatedCert6); $this->assertEquals(strlen($cert2), 860); + $cert = file_get_contents(TEST_ROOT.'/certs/with.comment.crt'); + $formatedCert7 = Utils::formatCert($cert, true); + $this->assertStringContainsString('-----BEGIN CERTIFICATE-----', $formatedCert7); + $this->assertStringContainsString('-----END CERTIFICATE-----', $formatedCert7); + $this->assertStringNotContainsString('comments', $formatedCert7); } /** From 30a0d4f43fb47b340b7b88fbff8984962f9d0687 Mon Sep 17 00:00:00 2001 From: gr8b Date: Mon, 18 Dec 2023 17:00:08 -0600 Subject: [PATCH 6/8] fix: replaced incorrect strtr usage with str_replace --- src/Saml2/Utils.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Saml2/Utils.php b/src/Saml2/Utils.php index 101a8b8c..e7fa7ed8 100644 --- a/src/Saml2/Utils.php +++ b/src/Saml2/Utils.php @@ -225,7 +225,7 @@ public static function formatCert($x509cert, $heads = true) $x509cert = static::getStringBetween($x509cert, '-----BEGIN CERTIFICATE-----', '-----END CERTIFICATE-----'); } - $x509cert = strtr($x509cert, "\x0d\r\n ", ''); + $x509cert = str_replace(["\x0d", "\r", "\n", " "], '', $x509cert); if ($heads && $x509cert !== '') { $x509cert = "-----BEGIN CERTIFICATE-----\n".chunk_split($x509cert, 64, "\n")."-----END CERTIFICATE-----\n"; From 944efe96a8e32d78d8da96bfbfa6c3cfa6419d8c Mon Sep 17 00:00:00 2001 From: Sixto Martin Date: Wed, 15 May 2024 17:46:06 +0200 Subject: [PATCH 7/8] Fix typo Utils.php --- src/Saml2/Utils.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Saml2/Utils.php b/src/Saml2/Utils.php index 721ede1d..3a6f77b3 100644 --- a/src/Saml2/Utils.php +++ b/src/Saml2/Utils.php @@ -219,7 +219,7 @@ public static function treeCopyReplace(DomNode $targetNode, DomNode $sourceNode, */ public static function formatCert($x509cert, $heads = true) { - if (is_null($cert)) { + if (is_null($x509cert)) { return; } From 0e2097216178673c26c0f7615849fdfefa25dacc Mon Sep 17 00:00:00 2001 From: Sixto Martin Date: Sun, 19 May 2024 23:41:41 +0200 Subject: [PATCH 8/8] Fix tests --- tests/src/OneLogin/Saml2/AuthTest.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/src/OneLogin/Saml2/AuthTest.php b/tests/src/OneLogin/Saml2/AuthTest.php index dace87f7..0cede8eb 100644 --- a/tests/src/OneLogin/Saml2/AuthTest.php +++ b/tests/src/OneLogin/Saml2/AuthTest.php @@ -819,7 +819,7 @@ public function testProcessSLORequestSignedResponse() $_GET['RelayState'] = 'http://relaystate.com'; $auth->setStrict(true); - $targetUrl = $this->_auth->processSLO(false, null, false, null, true); + $targetUrl = $auth->processSLO(false, null, false, null, true); $parsedQuery = getParamsFromUrl($targetUrl); @@ -1118,7 +1118,7 @@ public function testLoginSubject() $returnTo = 'http://example.com/returnto'; $settingsInfo['sp']['NameIDFormat'] = "urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress"; $auth2 = new Auth($settingsInfo); - $targetUrl3 = $auth2->login($returnTo, [], false, false, true, false, "testuser@example.com"); + $targetUrl3 = $auth2->login($returnTo, [], false, false, true); $parsedQuery3 = getParamsFromUrl($targetUrl3); $ssoUrl3 = $settingsInfo['idp']['singleSignOnService']['url'];