diff --git a/tests/config/module_casserver.php b/tests/config/module_casserver.php index 351a6745..82b15748 100644 --- a/tests/config/module_casserver.php +++ b/tests/config/module_casserver.php @@ -30,6 +30,8 @@ ], 'http://changeTicketParam' => [ 'ticketName' => 'myTicket', + ], + 'https://encoding.edu' => [ ] ], diff --git a/tests/www/LoginIntegrationTest.php b/tests/www/LoginIntegrationTest.php index 6441248a..2053c2df 100644 --- a/tests/www/LoginIntegrationTest.php +++ b/tests/www/LoginIntegrationTest.php @@ -236,7 +236,7 @@ public function testValidTicketNameOverride() CURLOPT_COOKIEFILE => $this->cookies_file ] ); - $this->assertEquals(302, $resp['code']); + $this->assertEquals(302, $resp['code'], $resp['body']); $this->assertStringStartsWith( $service_url . '?myTicket=ST-', @@ -245,6 +245,69 @@ public function testValidTicketNameOverride() ); } + /** + * Some clients urls: + * 1. Incorrectly encode query parameters or fragments + * 2. Encode spaces as %20 which php rencodes to + + * 3. Special characters url encoded to lower case hexadecimal (some .net versions) are changed to upper case. + * + * Test to confirm these type of urls work + * @dataProvider encodingIssueProvider + * @param string $service_url The service url submitted by the client + * @param string $expectedStartsWith The redirect location is expected to start with this. + * @param string $expectedEndsWith The redirect location is expected to end with this. Used for testing #fragments. + * @return void + */ + public function testEncodingIssued($service_url, $expectedStartsWith, $expectedEndsWith) + { + $this->authenticate(); + + /** @var array $resp */ + $resp = $this->server->get( + self::$LINK_URL, + ['service' => $service_url], + [ + CURLOPT_COOKIEJAR => $this->cookies_file, + CURLOPT_COOKIEFILE => $this->cookies_file + ] + ); + $this->assertEquals(302, $resp['code'], $resp['body']); + + $this->assertStringStartsWith( + $expectedStartsWith, + $resp['headers']['Location'], + 'Ticket should be part of the redirect.' + ); + if (!empty($expectedEndsWith)) { + $this->assertStringEndsWith( + $expectedEndsWith, + $resp['headers']['Location'], + 'url fragments happen after the query params' + ); + } + } + + public function encodingIssueProvider(): array + { + $urlWithQuery = 'https://encoding.edu/bug/portal.do?solo&ct=Search%20Prot&curl=https://kc.edu/IRB.do?se=1875*&rs=1'; + $urlNoQuery = 'https://encoding.edu/bug'; + $urlMultiKeys = 'https://encoding.edu/bug?a=val1&a=val2'; + $urlWithCaseDifference = 'https://encoding.edu?url=https%3a%2f%2Fencoding.edu%3Fa%3Db'; + $urlWithSpace = 'https://encoding.edu?a=a%20space'; + $urlWithRepeatParams = 'https://encoding.edu?a=b&a=c'; + return [ + [$urlWithQuery, $urlWithQuery . '&ticket=ST-', ''], + [$urlWithQuery . '#fragment', $urlWithQuery . '&ticket=ST-', '#fragment'], + [$urlMultiKeys, $urlMultiKeys . '&ticket=ST-', ''], + [$urlMultiKeys . '#fragment', $urlMultiKeys . '&ticket=ST-', '#fragment'], + [$urlNoQuery, $urlNoQuery. '?ticket=ST-', ''], + [$urlNoQuery . '#fragment', $urlNoQuery . '?ticket=ST-', '#fragment'], + [$urlWithCaseDifference, $urlWithCaseDifference. '&ticket=ST-', ''], + [$urlWithSpace, $urlWithSpace. '&ticket=ST-', ''], + [$urlWithRepeatParams, $urlWithRepeatParams. '&ticket=ST-', ''], + ]; + } + /** * Test outputting user info instead of redirecting diff --git a/www/login.php b/www/login.php index c63fb74c..5c4770f4 100644 --- a/www/login.php +++ b/www/login.php @@ -28,15 +28,15 @@ * language */ +use SimpleSAML\Configuration; +use SimpleSAML\Locale\Language; +use SimpleSAML\Logger; +use SimpleSAML\Module; use SimpleSAML\Module\casserver\Cas\AttributeExtractor; use SimpleSAML\Module\casserver\Cas\Protocol\SamlValidateResponder; use SimpleSAML\Module\casserver\Cas\ServiceValidator; use SimpleSAML\Module\casserver\Cas\Ticket\TicketFactory; use SimpleSAML\Module\casserver\Cas\Ticket\TicketStore; -use SimpleSAML\Configuration; -use SimpleSAML\Locale\Language; -use SimpleSAML\Logger; -use SimpleSAML\Module; use SimpleSAML\Session; use SimpleSAML\Utils; @@ -227,7 +227,7 @@ echo '
' . htmlspecialchars($casResponse) . ''; } } elseif ($redirect) { - $httpUtils->redirectTrustedURL($httpUtils->addURLParameters($serviceUrl, $parameters)); + $httpUtils->redirectTrustedURL($this->casAddURLParameters($serviceUrl, $parameters)); } else { $httpUtils->submitPOSTData($serviceUrl, $parameters); } @@ -236,3 +236,30 @@ $httpUtils->addURLParameters(Module::getModuleURL('casserver/loggedIn.php'), $parameters) ); } + + +/** + * CAS wants to ensure that a service url provided in login matches exactly that provided in service validate. + * This method avoids SSP's built in redirect which can change that url in certain ways, such as + * * changing how a ' ' is encoded + * * not correctly handling url fragments (e.g. #) + * * not correctly handling query param keys occurring multiple times + * * changing lower case hexadecimal to upper case + * * some buggy clients don't encode query params correctly + * which results in either the wrong url being returned to the client, or a service mismatch + * @param string $url The url to adjust + * @param array $params The query parameters to add. + * @return string The url to return + */ +function casAddURLParameters($url, $params) +{ + $url_fragment = explode("#", $url); + if (strpos($url_fragment[0], "?") === false) { + $url_fragment[0] .= "?"; + } else { + $url_fragment[0] .= "&"; + } + $url_fragment[0] .= http_build_query($params); + $url = implode("#", $url_fragment); + return $url; +} \ No newline at end of file