Handle url fragment # in url

Author: pradtke

tests/www/LoginIntegrationTest.php

@@ -234,7 +234,7 @@ public function testValidTicketNameOverride()
      * @dataProvider buggyClientProvider
      * @return void
      */
-    public function testBuggyClientBadUrlEncodingWorkAround($service_url)
+    public function testBuggyClientBadUrlEncodingWorkAround($service_url, $expectedStartsWith, $expectedEndsWith)
     {
         $this->authenticate();
 
@@ -247,20 +247,34 @@ public function testBuggyClientBadUrlEncodingWorkAround($service_url)
                 CURLOPT_COOKIEFILE => $this->cookies_file
             ]
         );
-        $this->assertEquals(302, $resp['code']);
+        $this->assertEquals(302, $resp['code'], $resp['body']);
 
         $this->assertStringStartsWith(
-            $service_url . '?ticket=ST-',
+            $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 buggyClientProvider(): array
     {
+        $urlWithQuery = 'https://buggy.edu/kc/portal.do?solo&ct=Search%20Prot&curl=https://kc.edu/kc/IRB.do?se=1875*&runSearch=1';
+        $urlNoQuery = 'https://buggy.edu/kc';
+        $urlMultiKeys = 'https://buggy.edu/kc?a=val1&a=val2';
         return [
-            ['https://buggy.edu/kc/portal.do?solo&ct=Search%20Prot&curl=https://kc.edu/kc/IRB.do?se=1875*&runSearch=1'],
-            ['https://buggy.edu/kc'],
+            [$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'],
         ];
     }
 

www/login.php

@@ -224,10 +224,7 @@
         }
     } elseif ($redirect) {
         if ($casconfig->getBoolean('noReencode', false)) {
-            // Some client encode query params wrong, and calling HTTP::addURLParameters
-            // will reencode them resulting in service mismatches
-            $extraParams = http_build_query($parameters);
-            $redirectUrl = $_GET['service'] . (strpos('?', $_GET['service']) === false ? '?' : '&') . $extraParams;
+            $redirectUrl = casAddURLParameters($_GET['service'], $parameters);
             HTTP::redirectTrustedURL($redirectUrl);
         } else {
             HTTP::redirectTrustedURL(HTTP::addURLParameters($_GET['service'], $parameters));
@@ -240,3 +237,29 @@
         HTTP::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
+ * * 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;
+}