Add optional redirect loop protection to AuthenticationService

[dereuromark]

Summary

This PR adds optional redirect validation to AuthenticationService::getLoginRedirect() to prevent redirect loop attacks that have been observed in production environments.

Problem

The current implementation validates that redirect URLs are relative (not external) but does NOT check for:

• ❌ Nested redirect parameters
• ❌ Multiple encoding levels
• ❌ Redirects to authentication pages (causing loops)
• ❌ Excessively long URLs (DOS prevention)

Real-world evidence: Production logs show bots (especially GPTBot) creating 6-7 levels of nested redirects, wasting server resources and potentially enabling security exploits.

Solution

Adds configurable redirectValidation option with:

• Opt-in design: Disabled by default for backward compatibility
• Four validation checks: Redirect depth, encoding levels, URL length, blocked patterns
• Extensible: Protected validateRedirect() method can be overridden
• Well-tested: 8 new test cases, all existing tests pass (312 total)
• Fully documented: New documentation page with security considerations

Configuration Example

$service->setConfig([
    'queryParam' => 'redirect',
    'redirectValidation' => [
        'enabled' => true,
        'maxDepth' => 1,
        'maxEncodingLevels' => 1,
        'maxLength' => 2000,
        'blockedPatterns' => ['#/login#i', '#/logout#i'],
    ],
]);

Changes

• ✅ Add validateRedirect() protected method to AuthenticationService
• ✅ Add redirectValidation configuration with sensible defaults
• ✅ Update getLoginRedirect() to call validation
• ✅ Add 8 comprehensive test cases (all pass)
• ✅ Add detailed documentation (docs/en/redirect-validation.rst)
• ✅ Maintain 100% backward compatibility (disabled by default)
• ✅ All existing tests pass (312 tests, 926 assertions)
• ✅ Code style checks pass

Validation Logic

1. Redirect depth: Count redirect= occurrences in decoded URL
2. Encoding level: Count %25 (percent-encoding of %)
3. URL length: Check total character count
4. Blocked patterns: Match against configured regex patterns

If validation fails, returns null instead of invalid URL.

Security Impact

Before:

• ⚠️ Nested redirects like /login?redirect=/login?redirect=/login...
• ⚠️ Double-encoded URLs like %252F
• ⚠️ Redirects to /logout causing loops
• ⚠️ Excessively long URLs (potential DOS)

After (when enabled):

• ✅ All nested redirects blocked
• ✅ Deep encoding detected and rejected
• ✅ Auth page loops prevented
• ✅ URL length limited

References

• Issue: [Security] Add redirect loop protection to AuthenticationService #751
• OWASP: Unvalidated Redirects and Forwards
• CWE-601: URL Redirection to Untrusted Site

Testing

# Run new tests
vendor/bin/phpunit --filter testGetLoginRedirect

# Run full test suite
vendor/bin/phpunit

# Check code style
vendor/bin/phpcs --standard=vendor/cakephp/cakephp-codesniffer/CakePHP src/

Checklist

• Tests pass
• Code style checks pass
• Documentation added
• Backward compatible (opt-in)
• Security considerations documented

---

Ready for review! Happy to make any adjustments based on feedback.

[Copilot]

Pull Request Overview

This PR adds optional redirect validation to AuthenticationService::getLoginRedirect() to prevent redirect loop attacks observed in production. The feature is disabled by default for backward compatibility and can be enabled via the redirectValidation configuration option.

• Adds validateRedirect() protected method with four validation checks (depth, encoding, length, patterns)
• Adds comprehensive test coverage with 8 new test cases
• Provides detailed documentation with security considerations and usage examples

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
src/AuthenticationService.php	Implements the validateRedirect() method and adds redirectValidation configuration with defaults
tests/TestCase/AuthenticationServiceTest.php	Adds 8 comprehensive test cases covering disabled validation, nested redirects, encoding levels, blocked patterns, URL length, custom patterns, and query parameters
docs/en/redirect-validation.rst	New documentation page explaining the feature, configuration options, validation logic, and security considerations
docs/en/index.rst	Adds link to the new redirect-validation documentation

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[dereuromark]

Thanks for the quick review @ADmad and @copilot!

I've addressed all the feedback in commit 1ae39c2:

✅ Fixed comparison operators (>= instead of >)

• Changed redirectCount > maxDepth to >=
• Changed encodingCount > maxEncodingLevels to >=
• This correctly blocks when the count meets or exceeds the threshold

✅ Replaced empty() with !

• Changed empty($config['enabled']) to !$config['enabled'] for clearer intent

All tests still pass (312 tests, 926 assertions). Ready for another review!

[markstory]

This whole thing seems quite complex for the problem it is trying to solve.

[dereuromark]

Thanks for the feedback @markstory and @ADmad!

I've simplified the implementation in commit 591ec19:

Removed

❌ blockedPatterns configuration - Removed entirely
❌ Pattern-based validation logic - Removed from core implementation
❌ 2 pattern-based tests - Removed

What Remains (Core Security Features)

✅ Nested redirect detection - Blocks /login?redirect=/login?redirect=...
✅ Deep encoding detection - Blocks double-encoded URLs like %252F
✅ URL length limits - Prevents DOS via long URLs

Custom Pattern Blocking

Applications that need pattern-based blocking (e.g., blocking redirects to /login, /logout) can extend AuthenticationService and override validateRedirect(). I've updated the documentation with an example.

---

Result: Much simpler implementation focused on the core redirect loop vulnerability.

• 310 tests pass (920 assertions)
• Code style checks pass
• Documentation updated with custom validation example

Ready for another review!

[dereuromark]

Gonna test it on the server before we tag any release.

[dereuromark]

So far so good.

Only one more case where it can run into loops. when working together with authorization - and UnauthorizedHandler.
It seems there is one important part missing that gets triggered when a user logged in has not sufficient rights, when you there try to send a user to the action they wanted to go.

This seems to solve it, but its not really too nice.

// SafeRedirectHandler extends plugin one
	public function handle(Exception $exception, ServerRequestInterface $request, array $options = []): ResponseInterface {
		// If user is already logged in but not authorized, redirect to fallback
		// instead of login page (which would cause a loop)
		$identity = $request->getAttribute('identity');
		if ($identity) {
			$message = $options['unauthorizedMessage'] ?? __('You are not authorized to access that location.');
			if ($message) {
				/** @var \Cake\Http\ServerRequest $request */
				$session = $request->getSession();
				$session->write('Flash.flash', [
					[
						'message' => $message,
						'key' => 'flash',
						'element' => 'flash/error',
						'params' => [],
					],
				]);
			}

			$fallbackUrl = Router::url(['prefix' => false, 'plugin' => false, 'controller' => 'Account', 'action' => 'index']);

			return (new Response())
				->withHeader('Location', $fallbackUrl)
				->withStatus(302);
		}