Skip to content

Commit 591ec19

Browse files
committed
Simplify implementation by removing blockedPatterns
Address maintainer feedback from @markstory and @ADmad: - Remove blockedPatterns configuration option - Remove pattern-based validation logic - Update documentation to show custom pattern validation in subclass - Remove 2 pattern-based tests (testGetLoginRedirectValidationBlockedPatterns, testGetLoginRedirectValidationCustomPatterns) Result: Simpler, focused implementation covering the core security issues: - Nested redirect detection - Deep encoding detection - URL length limits Custom pattern blocking can still be achieved by overriding validateRedirect(). All tests pass: 310 tests, 920 assertions Code style checks pass
1 parent 1ae39c2 commit 591ec19

3 files changed

Lines changed: 9 additions & 116 deletions

File tree

docs/en/redirect-validation.rst

Lines changed: 9 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -66,12 +66,6 @@ maxLength
6666
Maximum allowed length of the redirect URL in characters. This helps prevent DOS attacks
6767
via excessively long URLs.
6868

69-
blockedPatterns
70-
**Type:** ``array`` | **Default:** ``['#/login#i', '#/logout#i', '#/register#i']``
71-
72-
Array of regular expressions to match against redirect URLs. Matching URLs will be rejected.
73-
This prevents redirects to authentication-related pages that could cause loops.
74-
7569
Example Configuration
7670
=====================
7771

@@ -88,12 +82,6 @@ Here's a complete example with custom configuration:
8882
'maxDepth' => 1,
8983
'maxEncodingLevels' => 1,
9084
'maxLength' => 2000,
91-
'blockedPatterns' => [
92-
'#/admin#i', // Block admin areas
93-
'#/login#i', // Block login page
94-
'#/logout#i', // Block logout page
95-
'#/register#i', // Block registration page
96-
],
9785
],
9886
]);
9987
@@ -118,15 +106,14 @@ The validation performs the following checks in order:
118106
1. **Redirect Depth**: Counts occurrences of ``redirect=`` in the decoded URL
119107
2. **Encoding Level**: Counts occurrences of ``%25`` (percent-encoded percent sign)
120108
3. **URL Length**: Checks total character count
121-
4. **Blocked Patterns**: Matches against configured regex patterns
122109

123110
If any check fails, the URL is rejected.
124111

125112
Custom Validation
126113
=================
127114

128115
You can extend ``AuthenticationService`` and override the ``validateRedirect()`` method
129-
to implement custom validation logic:
116+
to implement custom validation logic, such as blocking specific URL patterns:
130117

131118
.. code-block:: php
132119
@@ -146,8 +133,14 @@ to implement custom validation logic:
146133
}
147134
148135
// Add your custom validation
149-
if (str_contains($redirect, 'forbidden')) {
150-
return null; // Reject this URL
136+
// Example: Block redirects to authentication pages
137+
if (preg_match('#/(login|logout|register)#i', $redirect)) {
138+
return null;
139+
}
140+
141+
// Example: Block redirects to admin areas
142+
if (str_contains($redirect, '/admin')) {
143+
return null;
151144
}
152145
153146
return $redirect;

src/AuthenticationService.php

Lines changed: 0 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -93,10 +93,6 @@ class AuthenticationService implements AuthenticationServiceInterface, Impersona
9393
* 'maxDepth' => 1, // Max nested "redirect=" parameters (default: 1)
9494
* 'maxEncodingLevels' => 1, // Max percent-encoding levels (default: 1)
9595
* 'maxLength' => 2000, // Max URL length in characters (default: 2000)
96-
* 'blockedPatterns' => [ // Regex patterns to reject (default: auth pages)
97-
* '#/login#i',
98-
* '#/logout#i',
99-
* ],
10096
* ]
10197
* ```
10298
*
@@ -126,11 +122,6 @@ class AuthenticationService implements AuthenticationServiceInterface, Impersona
126122
'maxDepth' => 1,
127123
'maxEncodingLevels' => 1,
128124
'maxLength' => 2000,
129-
'blockedPatterns' => [
130-
'#/login#i',
131-
'#/logout#i',
132-
'#/register#i',
133-
],
134125
],
135126
];
136127

@@ -526,13 +517,6 @@ protected function validateRedirect(string $redirect): ?string
526517
return null;
527518
}
528519

529-
// Check against blocked patterns
530-
foreach ($config['blockedPatterns'] as $pattern) {
531-
if (preg_match($pattern, $decodedUrl)) {
532-
return null;
533-
}
534-
}
535-
536520
return $redirect;
537521
}
538522

tests/TestCase/AuthenticationServiceTest.php

Lines changed: 0 additions & 84 deletions
Original file line numberDiff line numberDiff line change
@@ -1046,53 +1046,6 @@ public function testGetLoginRedirectValidationEncodingLevels()
10461046
$this->assertNull($service->getLoginRedirect($request));
10471047
}
10481048

1049-
/**
1050-
* testGetLoginRedirectValidationBlockedPatterns
1051-
*
1052-
* @return void
1053-
*/
1054-
public function testGetLoginRedirectValidationBlockedPatterns()
1055-
{
1056-
$service = new AuthenticationService([
1057-
'unauthenticatedRedirect' => '/users/login',
1058-
'queryParam' => 'redirect',
1059-
'redirectValidation' => [
1060-
'enabled' => true,
1061-
],
1062-
]);
1063-
1064-
// Redirect to login page (should be blocked)
1065-
$request = ServerRequestFactory::fromGlobals(
1066-
['REQUEST_URI' => '/secrets'],
1067-
['redirect' => '/users/login'],
1068-
);
1069-
$this->assertNull($service->getLoginRedirect($request));
1070-
1071-
// Redirect to logout page (should be blocked)
1072-
$request = ServerRequestFactory::fromGlobals(
1073-
['REQUEST_URI' => '/secrets'],
1074-
['redirect' => '/users/logout'],
1075-
);
1076-
$this->assertNull($service->getLoginRedirect($request));
1077-
1078-
// Redirect to register page (should be blocked)
1079-
$request = ServerRequestFactory::fromGlobals(
1080-
['REQUEST_URI' => '/secrets'],
1081-
['redirect' => '/users/register'],
1082-
);
1083-
$this->assertNull($service->getLoginRedirect($request));
1084-
1085-
// Valid redirect to non-auth page
1086-
$request = ServerRequestFactory::fromGlobals(
1087-
['REQUEST_URI' => '/secrets'],
1088-
['redirect' => '/articles/index'],
1089-
);
1090-
$this->assertSame(
1091-
'/articles/index',
1092-
$service->getLoginRedirect($request),
1093-
);
1094-
}
1095-
10961049
/**
10971050
* testGetLoginRedirectValidationMaxLength
10981051
*
@@ -1128,43 +1081,6 @@ public function testGetLoginRedirectValidationMaxLength()
11281081
$this->assertNull($service->getLoginRedirect($request));
11291082
}
11301083

1131-
/**
1132-
* testGetLoginRedirectValidationCustomPatterns
1133-
*
1134-
* @return void
1135-
*/
1136-
public function testGetLoginRedirectValidationCustomPatterns()
1137-
{
1138-
$service = new AuthenticationService([
1139-
'unauthenticatedRedirect' => '/users/login',
1140-
'queryParam' => 'redirect',
1141-
'redirectValidation' => [
1142-
'enabled' => true,
1143-
'blockedPatterns' => [
1144-
'#/admin#i',
1145-
'#/secret#i',
1146-
],
1147-
],
1148-
]);
1149-
1150-
// Redirect to admin area (should be blocked by custom pattern)
1151-
$request = ServerRequestFactory::fromGlobals(
1152-
['REQUEST_URI' => '/articles'],
1153-
['redirect' => '/admin/dashboard'],
1154-
);
1155-
$this->assertNull($service->getLoginRedirect($request));
1156-
1157-
// Redirect to normal page (should pass)
1158-
$request = ServerRequestFactory::fromGlobals(
1159-
['REQUEST_URI' => '/articles'],
1160-
['redirect' => '/public/articles'],
1161-
);
1162-
$this->assertSame(
1163-
'/public/articles',
1164-
$service->getLoginRedirect($request),
1165-
);
1166-
}
1167-
11681084
/**
11691085
* testGetLoginRedirectValidationWithQueryParameters
11701086
*

0 commit comments

Comments
 (0)