Skip to content

Commit 1ee4594

Browse files
pradtkepradtke
committed
Merge branch 'master' into SSP-2167-gateway_fix
2 parents 526159d + b0ac520 commit 1ee4594

8 files changed

Lines changed: 85 additions & 69 deletions

File tree

‎.github/workflows/php.yml‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ jobs:
2020
matrix:
2121
php-version: ['8.1', '8.2', '8.3', '8.4']
2222

23-
uses: simplesamlphp/simplesamlphp-test-framework/.github/workflows/reusable_phplinter.yml@v1.10.0
23+
uses: simplesamlphp/simplesamlphp-test-framework/.github/workflows/reusable_phplinter.yml@v1.10.3
2424
with:
2525
php-version: ${{ matrix.php-version }}
2626

@@ -29,7 +29,7 @@ jobs:
2929
strategy:
3030
fail-fast: false
3131

32-
uses: simplesamlphp/simplesamlphp-test-framework/.github/workflows/reusable_linter.yml@v1.10.0
32+
uses: simplesamlphp/simplesamlphp-test-framework/.github/workflows/reusable_linter.yml@v1.10.3
3333
with:
3434
enable_eslinter: true
3535
enable_jsonlinter: true
@@ -94,7 +94,7 @@ jobs:
9494

9595
- name: Save coverage data
9696
if: ${{ matrix.php-versions == '8.4' }}
97-
uses: actions/upload-artifact@v4
97+
uses: actions/upload-artifact@v5
9898
with:
9999
name: coverage-data
100100
path: ${{ github.workspace }}/build
@@ -253,7 +253,7 @@ jobs:
253253
steps:
254254
- uses: actions/checkout@v5
255255

256-
- uses: actions/download-artifact@v5
256+
- uses: actions/download-artifact@v6
257257
with:
258258
name: coverage-data
259259
path: ${{ github.workspace }}/build

‎README.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -101,7 +101,7 @@ docker run --name ssp-casserver-dev \
101101
--mount type=bind,source="$(pwd)/docker/ssp/authsources.php",target=/var/simplesamlphp/config/authsources.php,readonly \
102102
--mount type=bind,source="$(pwd)/docker/ssp/config-override.php",target=/var/simplesamlphp/config/config-override.php,readonly \
103103
--mount type=bind,source="$(pwd)/docker/apache-override.cf",target=/etc/apache2/sites-enabled/ssp-override.cf,readonly \
104-
-p 443:443 cirrusid/simplesamlphp:v2.3.5
104+
-p 443:443 cirrusid/simplesamlphp:v2.4.2
105105
```
106106

107107
Visit [https://localhost/simplesaml/](https://localhost/simplesaml/) and confirm you get the default page.

‎composer.json‎

Lines changed: 8 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -30,18 +30,19 @@
3030
},
3131
"require": {
3232
"php": "^8.1",
33+
"ext-ctype": "*",
3334
"ext-dom": "*",
3435
"ext-filter": "*",
3536
"ext-libxml": "*",
3637
"ext-SimpleXML": "*",
3738
"ext-session": "*",
3839

39-
"simplesamlphp/assert": "~1.8.2",
40+
"simplesamlphp/assert": "~1.8.1",
4041
"simplesamlphp/composer-module-installer": "~1.4.0",
4142
"simplesamlphp/simplesamlphp": "~2.4.2",
42-
"simplesamlphp/xml-cas": "~2.0.0",
43-
"simplesamlphp/xml-common": "~2.0.0",
44-
"simplesamlphp/xml-soap": "~2.0.0",
43+
"simplesamlphp/xml-cas": "~1.3",
44+
"simplesamlphp/xml-common": "~1.17",
45+
"simplesamlphp/xml-soap": "~1.5",
4546
"symfony/http-foundation": "~6.4.0",
4647
"symfony/http-kernel": "~6.4.0",
4748
"simplesamlphp/saml11": "~1.2.5"
@@ -54,12 +55,10 @@
5455
"source": "https://github.com/simplesamlphp/simplesamlphp-module-casserver"
5556
},
5657
"scripts": {
57-
"validate": [
58+
"verify": [
5859
"vendor/bin/phpcs -p",
59-
"vendor/bin/composer-require-checker check --config-file=tools/composer-require-checker.json composer.json",
60-
"vendor/bin/psalm -c psalm-dev.xml",
61-
"vendor/bin/composer-unused",
62-
"vendor/bin/phpunit --no-coverage --testdox"
60+
"vendor/bin/phpunit --no-coverage --testdox",
61+
"vendor/bin/phpstan --memory-limit=256M analyze -c phpstan.neon"
6362
],
6463
"tests": [
6564
"vendor/bin/phpunit --no-coverage"

‎src/Cas/ServiceValidator.php‎

Lines changed: 35 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -53,35 +53,9 @@ public function checkServiceURL(string $service): ?Configuration
5353

5454
$configOverride = \is_int($index) ? null : $value;
5555

56-
// URL String
57-
if (str_starts_with($service, $legalUrl)) {
58-
$isValidService = true;
56+
if ($isValidService = $this->validateServiceIsLegal($legalUrl, $service)) {
5957
break;
6058
}
61-
62-
// Regex
63-
// Since "If the regex pattern passed does not compile to a valid regex, an E_WARNING is emitted. "
64-
// we will throw an exception if the warning is emitted and use try-catch to handle it
65-
set_error_handler(static function ($severity, $message, $file, $line) {
66-
throw new \ErrorException($message, $severity, $severity, $file, $line);
67-
}, E_WARNING);
68-
69-
try {
70-
$result = preg_match($legalUrl, $service);
71-
if ($result !== 1) {
72-
throw new \RuntimeException('Service URL does not match legal service URL.');
73-
}
74-
$isValidService = true;
75-
break;
76-
} catch (\RuntimeException $e) {
77-
// do nothing
78-
Logger::warning($e->getMessage());
79-
} catch (\Exception $e) {
80-
// do nothing
81-
Logger::warning("Invalid CAS legal service url '$legalUrl'. Error " . preg_last_error());
82-
} finally {
83-
restore_error_handler();
84-
}
8559
}
8660

8761
if (!$isValidService) {
@@ -107,4 +81,38 @@ public function checkServiceURL(string $service): ?Configuration
10781
}
10882
return Configuration::loadFromArray($serviceConfig);
10983
}
84+
85+
/**
86+
* @param string $legalUrl The string or regex to use for comparison
87+
* @param string $service The service to compare
88+
*
89+
* @return bool Whether the service is legal
90+
* @throws \ErrorException
91+
*/
92+
protected function validateServiceIsLegal(string $legalUrl, string $service): bool
93+
{
94+
$isValid = false;
95+
if (!ctype_alnum($legalUrl[0])) {
96+
// Since "If the regex pattern passed does not compile to a valid regex, an E_WARNING is emitted. "
97+
// we will throw an exception if the warning is emitted and use try-catch to handle it
98+
set_error_handler(static function ($severity, $message, $file, $line) {
99+
throw new \ErrorException($message, $severity, $severity, $file, $line);
100+
}, E_WARNING);
101+
102+
try {
103+
if (preg_match($legalUrl, $service) === 1) {
104+
$isValid = true;
105+
}
106+
} catch (\ErrorException $e) {
107+
// do nothing
108+
Logger::warning("Invalid CAS legal service url '$legalUrl'. Error " . preg_last_error_msg());
109+
} finally {
110+
restore_error_handler();
111+
}
112+
} elseif (str_starts_with($service, $legalUrl)) {
113+
$isValid = true;
114+
}
115+
116+
return $isValid;
117+
}
110118
}

‎src/Controller/Cas20Controller.php‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@
44

55
namespace SimpleSAML\Module\casserver\Controller;
66

7-
use Exception;
87
use SimpleSAML\CAS\Constants as C;
98
use SimpleSAML\Configuration;
109
use SimpleSAML\Logger;

‎src/Controller/LoginController.php‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -28,9 +28,9 @@
2828
use Symfony\Component\HttpKernel\Attribute\AsController;
2929
use Symfony\Component\HttpKernel\Attribute\MapQueryParameter;
3030

31-
use in_array;
32-
use http_build_query;
33-
use var_export;
31+
use function http_build_query;
32+
use function in_array;
33+
use function var_export;
3434

3535
#[AsController]
3636
class LoginController

‎src/Controller/Traits/TicketValidatorTrait.php‎

Lines changed: 24 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -74,21 +74,7 @@ public function validate(
7474
$message = '';
7575
// Below, we do not have a ticket or the ticket does not meet the very basic criteria that allow
7676
// any further handling
77-
if (empty($serviceTicket)) {
78-
// No ticket
79-
$message = 'Ticket ' . var_export($ticket, true) . ' not recognized';
80-
$failed = true;
81-
} elseif ($method === 'proxyValidate' && !$this->ticketFactory->isProxyTicket($serviceTicket)) {
82-
// proxyValidate but not a proxy ticket
83-
$message = 'Ticket ' . var_export($ticket, true) . ' is not a proxy ticket.';
84-
$failed = true;
85-
} elseif ($method === 'serviceValidate' && !$this->ticketFactory->isServiceTicket($serviceTicket)) {
86-
// serviceValidate but not a service ticket
87-
$message = 'Ticket ' . var_export($ticket, true) . ' is not a service ticket.';
88-
$failed = true;
89-
}
90-
91-
if ($failed) {
77+
if ($message = $this->validateServiceTicket($serviceTicket, $ticket, $method)) {
9278
$finalMessage = 'casserver:validate: ' . $message;
9379
Logger::error(__METHOD__ . '::' . $finalMessage);
9480

@@ -185,4 +171,27 @@ public function validate(
185171
Response::HTTP_OK,
186172
);
187173
}
174+
175+
/**
176+
* @param array{'id': string}|null $serviceTicket
177+
*
178+
* @return ?string Message on failure, null on success
179+
*/
180+
private function validateServiceTicket(?array $serviceTicket, string $ticket, string $method): ?string
181+
{
182+
if (empty($serviceTicket)) {
183+
return 'Ticket ' . var_export($ticket, true) . ' not recognized';
184+
}
185+
186+
$isServiceTicket = $this->ticketFactory->isServiceTicket($serviceTicket);
187+
if ($method === 'serviceValidate' && !$isServiceTicket) {
188+
return 'Ticket ' . var_export($ticket, true) . ' is not a service ticket.';
189+
}
190+
191+
if ($method === 'proxyValidate' && !$isServiceTicket && !$this->ticketFactory->isProxyTicket($serviceTicket)) {
192+
return 'Ticket ' . var_export($ticket, true) . ' is not a proxy ticket.';
193+
}
194+
195+
return null;
196+
}
188197
}

‎tests/src/Controller/Cas20ControllerTest.php‎

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -565,14 +565,6 @@ public static function validateOnDifferentQueryParameterCombinationsProxyValidat
565565
"Ticket 'PT-{$sessionId}' has expired",
566566
'PT-' . $sessionId,
567567
],
568-
'Returns Bad Request on Ticket is A Service Ticket' => [
569-
[
570-
'ticket' => 'ST-' . $sessionId,
571-
'service' => 'https://myservice.com/abcd',
572-
],
573-
"Ticket 'ST-{$sessionId}' is not a proxy ticket.",
574-
'ST-' . $sessionId,
575-
],
576568
'Returns Bad Request on Ticket Issued By Single SignOn Session' => [
577569
[
578570
'ticket' => 'PT-' . $sessionId,
@@ -583,7 +575,7 @@ public static function validateOnDifferentQueryParameterCombinationsProxyValidat
583575
'PT-' . $sessionId,
584576
9999999999,
585577
],
586-
'Returns Success' => [
578+
'Returns Success with Proxy Ticket' => [
587579
[
588580
'ticket' => 'PT-' . $sessionId,
589581
'service' => 'https://myservice.com/abcd',
@@ -592,6 +584,15 @@ public static function validateOnDifferentQueryParameterCombinationsProxyValidat
592584
'PT-' . $sessionId,
593585
9999999999,
594586
],
587+
'Returns Success with Service Ticket' => [
588+
[
589+
'ticket' => 'ST-' . $sessionId,
590+
'service' => 'https://myservice.com/abcd',
591+
],
592+
'username@google.com',
593+
'ST-' . $sessionId,
594+
9999999999,
595+
],
595596
];
596597
}
597598

0 commit comments

Comments
 (0)