From 9823fec1b4271c19671f22b4f3dc1e673f778b30 Mon Sep 17 00:00:00 2001
From: Patrick Radtke <patrick@cirrusidentity.com>
Date: Wed, 28 Aug 2019 13:37:13 -0500
Subject: [PATCH 1/5] Option to redirect to exact service url

Some clients will encode query parameters in their service url incorrectly.
If SSP/php's query builder utils are used then the service url is parsed,
and reconstructed differently then what is stored in the ticket. Sometimes
badly encoded parameters are lost.

Example: should space in a query param be encoded as '+' or '%20'
---
 config-templates/module_casserver.php |  3 +++
 tests/config/module_casserver.php     |  4 +++
 tests/www/LoginIntegrationTest.php    | 37 +++++++++++++++++++++++++++
 www/login.php                         | 18 +++++++++----
 4 files changed, 57 insertions(+), 5 deletions(-)

diff --git a/config-templates/module_casserver.php b/config-templates/module_casserver.php
index 8bc898eb..f4bc540a 100644
--- a/config-templates/module_casserver.php
+++ b/config-templates/module_casserver.php
@@ -32,6 +32,9 @@
             'attributes_to_transfer' => ['cn'],
         ],
     ],
+    // Don't use php or SSP's built in methods for constructing queries. Default is false
+    // Enabling this redirects the user back to the service with the exact service url provided.
+    'noReencode' => false,
 
     'legal_target_service_urls' => [
         //Any target service url string matching any of the following prefixes is accepted
diff --git a/tests/config/module_casserver.php b/tests/config/module_casserver.php
index 4297fa60..b4587560 100644
--- a/tests/config/module_casserver.php
+++ b/tests/config/module_casserver.php
@@ -30,6 +30,10 @@
         ],
         'http://changeTicketParam' => [
             'ticketName' => 'myTicket',
+        ],
+        'https://buggy.edu' => [
+            // Don't use php or SSP's built in methods for constructing queries.
+            'noReencode' => true
         ]
     ],
 
diff --git a/tests/www/LoginIntegrationTest.php b/tests/www/LoginIntegrationTest.php
index 77cfd278..09d82735 100644
--- a/tests/www/LoginIntegrationTest.php
+++ b/tests/www/LoginIntegrationTest.php
@@ -227,6 +227,43 @@ public function testValidTicketNameOverride()
         );
     }
 
+    /**
+     * Some clients don't correctly encode query parameters that are part their service
+     * urls or encode a space in a different way then SSP will in a redirect. This workaround
+     * is to allow those clients to work
+     * @dataProvider buggyClientProvider
+     * @return void
+     */
+    public function testBuggyClientBadUrlEncodingWorkAround($service_url)
+    {
+        $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']);
+
+        $this->assertStringStartsWith(
+            $service_url . '?ticket=ST-',
+            $resp['headers']['Location'],
+            'Ticket should be part of the redirect.'
+        );
+    }
+
+    public function buggyClientProvider(): array
+    {
+        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'],
+        ];
+    }
+
 
     /**
      * Test outputting user info instead of redirecting
diff --git a/www/login.php b/www/login.php
index 83e3c702..12f76e86 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\HTTP;
 
@@ -223,7 +223,15 @@
             echo '<pre>' . htmlspecialchars($casResponse) . '</pre>';
         }
     } elseif ($redirect) {
-        HTTP::redirectTrustedURL(HTTP::addURLParameters($serviceUrl, $parameters));
+        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;
+            HTTP::redirectTrustedURL($redirectUrl);
+        } else {
+            HTTP::redirectTrustedURL(HTTP::addURLParameters($_GET['service'], $parameters));
+        }
     } else {
         HTTP::submitPOSTData($serviceUrl, $parameters);
     }

From 3c8953d247d970282dc9aeeb51348e00c9cfc022 Mon Sep 17 00:00:00 2001
From: Patrick Radtke <patrick@cirrusidentity.com>
Date: Fri, 30 Aug 2019 11:39:39 -0500
Subject: [PATCH 2/5] Handle url fragment # in url

---
 tests/www/LoginIntegrationTest.php | 24 ++++++++++++++++++-----
 www/login.php                      | 31 ++++++++++++++++++++++++++----
 2 files changed, 46 insertions(+), 9 deletions(-)

diff --git a/tests/www/LoginIntegrationTest.php b/tests/www/LoginIntegrationTest.php
index 09d82735..025693f9 100644
--- a/tests/www/LoginIntegrationTest.php
+++ b/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'],
         ];
     }
 
diff --git a/www/login.php b/www/login.php
index 12f76e86..bed961df 100644
--- a/www/login.php
+++ b/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;
+}
\ No newline at end of file

From 3a818c2e8338eedd4cfd3af8a5ea9ebe86432806 Mon Sep 17 00:00:00 2001
From: Patrick Radtke <patrick@cirrusidentity.com>
Date: Thu, 16 Jan 2020 16:51:44 +1300
Subject: [PATCH 3/5] Rebase on latest master

---
 tests/www/LoginIntegrationTest.php | 5 ++++-
 www/login.php                      | 4 ++--
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/tests/www/LoginIntegrationTest.php b/tests/www/LoginIntegrationTest.php
index 025693f9..bef57489 100644
--- a/tests/www/LoginIntegrationTest.php
+++ b/tests/www/LoginIntegrationTest.php
@@ -218,7 +218,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-',
@@ -232,6 +232,9 @@ public function testValidTicketNameOverride()
      * urls or encode a space in a different way then SSP will in a redirect. This workaround
      * is to allow those clients to work
      * @dataProvider buggyClientProvider
+     * @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 testBuggyClientBadUrlEncodingWorkAround($service_url, $expectedStartsWith, $expectedEndsWith)
diff --git a/www/login.php b/www/login.php
index bed961df..0f81cfef 100644
--- a/www/login.php
+++ b/www/login.php
@@ -224,10 +224,10 @@
         }
     } elseif ($redirect) {
         if ($casconfig->getBoolean('noReencode', false)) {
-            $redirectUrl = casAddURLParameters($_GET['service'], $parameters);
+            $redirectUrl = casAddURLParameters($serviceUrl, $parameters);
             HTTP::redirectTrustedURL($redirectUrl);
         } else {
-            HTTP::redirectTrustedURL(HTTP::addURLParameters($_GET['service'], $parameters));
+            HTTP::redirectTrustedURL(HTTP::addURLParameters($serviceUrl, $parameters));
         }
     } else {
         HTTP::submitPOSTData($serviceUrl, $parameters);

From 6c4c31849c404274882333fabd8ceb8d6f6d45d6 Mon Sep 17 00:00:00 2001
From: Patrick Radtke <patrick@cirrusidentity.com>
Date: Thu, 16 Jan 2020 17:24:10 +1300
Subject: [PATCH 4/5] Add url test cases for space and for lower case
 hexadecimal

---
 tests/config/module_casserver.php  |  5 ++---
 tests/www/LoginIntegrationTest.php | 25 ++++++++++++++++---------
 2 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/tests/config/module_casserver.php b/tests/config/module_casserver.php
index b4587560..7b351e7b 100644
--- a/tests/config/module_casserver.php
+++ b/tests/config/module_casserver.php
@@ -31,9 +31,8 @@
         'http://changeTicketParam' => [
             'ticketName' => 'myTicket',
         ],
-        'https://buggy.edu' => [
-            // Don't use php or SSP's built in methods for constructing queries.
-            'noReencode' => true
+        'https://encoding.edu' => [
+            'noReencode' => true,
         ]
     ],
 
diff --git a/tests/www/LoginIntegrationTest.php b/tests/www/LoginIntegrationTest.php
index bef57489..7bee139f 100644
--- a/tests/www/LoginIntegrationTest.php
+++ b/tests/www/LoginIntegrationTest.php
@@ -228,16 +228,19 @@ public function testValidTicketNameOverride()
     }
 
     /**
-     * Some clients don't correctly encode query parameters that are part their service
-     * urls or encode a space in a different way then SSP will in a redirect. This workaround
-     * is to allow those clients to work
-     * @dataProvider buggyClientProvider
+     * 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 testBuggyClientBadUrlEncodingWorkAround($service_url, $expectedStartsWith, $expectedEndsWith)
+    public function testEncodingIssued($service_url, $expectedStartsWith, $expectedEndsWith)
     {
         $this->authenticate();
 
@@ -266,11 +269,13 @@ public function testBuggyClientBadUrlEncodingWorkAround($service_url, $expectedS
         }
     }
 
-    public function buggyClientProvider(): array
+    public function encodingIssueProvider(): 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';
+        $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';
         return [
             [$urlWithQuery, $urlWithQuery . '&ticket=ST-', ''],
             [$urlWithQuery . '#fragment', $urlWithQuery . '&ticket=ST-', '#fragment'],
@@ -278,6 +283,8 @@ public function buggyClientProvider(): array
             [$urlMultiKeys . '#fragment', $urlMultiKeys . '&ticket=ST-', '#fragment'],
             [$urlNoQuery, $urlNoQuery. '?ticket=ST-', ''],
             [$urlNoQuery . '#fragment', $urlNoQuery . '?ticket=ST-', '#fragment'],
+            [$urlWithCaseDifference, $urlWithCaseDifference. '&ticket=ST-', ''],
+            [$urlWithSpace, $urlWithSpace. '&ticket=ST-', ''],
         ];
     }
 

From 43b960b27f9159b6532cea674737f43021741050 Mon Sep 17 00:00:00 2001
From: Patrick Radtke <patrick@cirrusidentity.com>
Date: Thu, 16 Jan 2020 17:28:15 +1300
Subject: [PATCH 5/5] Remove option to use SSP's redirect; add test for
 multiple query params

There are too many cases where SSP's redirect changes the service url in ways
that make future exact matches fail. As such change is to always user the alternate
method.
---
 config-templates/module_casserver.php | 3 ---
 tests/config/module_casserver.php     | 1 -
 tests/www/LoginIntegrationTest.php    | 2 ++
 www/login.php                         | 9 +++------
 4 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/config-templates/module_casserver.php b/config-templates/module_casserver.php
index f4bc540a..8bc898eb 100644
--- a/config-templates/module_casserver.php
+++ b/config-templates/module_casserver.php
@@ -32,9 +32,6 @@
             'attributes_to_transfer' => ['cn'],
         ],
     ],
-    // Don't use php or SSP's built in methods for constructing queries. Default is false
-    // Enabling this redirects the user back to the service with the exact service url provided.
-    'noReencode' => false,
 
     'legal_target_service_urls' => [
         //Any target service url string matching any of the following prefixes is accepted
diff --git a/tests/config/module_casserver.php b/tests/config/module_casserver.php
index 7b351e7b..789cd2b1 100644
--- a/tests/config/module_casserver.php
+++ b/tests/config/module_casserver.php
@@ -32,7 +32,6 @@
             'ticketName' => 'myTicket',
         ],
         'https://encoding.edu' => [
-            'noReencode' => true,
         ]
     ],
 
diff --git a/tests/www/LoginIntegrationTest.php b/tests/www/LoginIntegrationTest.php
index 7bee139f..a49529e6 100644
--- a/tests/www/LoginIntegrationTest.php
+++ b/tests/www/LoginIntegrationTest.php
@@ -276,6 +276,7 @@ public function encodingIssueProvider(): array
         $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'],
@@ -285,6 +286,7 @@ public function encodingIssueProvider(): array
             [$urlNoQuery . '#fragment', $urlNoQuery . '?ticket=ST-', '#fragment'],
             [$urlWithCaseDifference, $urlWithCaseDifference. '&ticket=ST-', ''],
             [$urlWithSpace, $urlWithSpace. '&ticket=ST-', ''],
+            [$urlWithRepeatParams, $urlWithRepeatParams. '&ticket=ST-', ''],
         ];
     }
 
diff --git a/www/login.php b/www/login.php
index 0f81cfef..a8ca3105 100644
--- a/www/login.php
+++ b/www/login.php
@@ -223,12 +223,8 @@
             echo '<pre>' . htmlspecialchars($casResponse) . '</pre>';
         }
     } elseif ($redirect) {
-        if ($casconfig->getBoolean('noReencode', false)) {
-            $redirectUrl = casAddURLParameters($serviceUrl, $parameters);
-            HTTP::redirectTrustedURL($redirectUrl);
-        } else {
-            HTTP::redirectTrustedURL(HTTP::addURLParameters($serviceUrl, $parameters));
-        }
+        $redirectUrl = casAddURLParameters($serviceUrl, $parameters);
+        HTTP::redirectTrustedURL($redirectUrl);
     } else {
         HTTP::submitPOSTData($serviceUrl, $parameters);
     }
@@ -245,6 +241,7 @@
  * * 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