Fixed ExternalRedirectListener issue parsing some invalid URLs

nelmio · Nov 28, 2024 · 9dba3b6 · 9dba3b6
1 parent 8214958
commit 9dba3b6
Show file tree

Hide file tree

Showing 2 changed files with 23 additions and 3 deletions.
diff --git a/src/EventListener/ExternalRedirectListener.php b/src/EventListener/ExternalRedirectListener.php
@@ -17,6 +17,7 @@
 use Nelmio\SecurityBundle\ExternalRedirect\ExternalRedirectResponse;
 use Nelmio\SecurityBundle\ExternalRedirect\TargetValidator;
 use Psr\Log\LoggerInterface;
+use Symfony\Component\HttpFoundation\Exception\BadRequestException;
 use Symfony\Component\HttpKernel\Event\ResponseEvent;
 use Symfony\Component\HttpKernel\Exception\HttpException;
 use Symfony\Component\Routing\Generator\UrlGeneratorInterface;
@@ -130,8 +131,15 @@ public function onKernelResponse(ResponseEvent $e): void
 
     public function isExternalRedirect(string $source, string $target): bool
     {
-        // cleanup "\rhttp://foo.com/" and other null prefixeds to be scanned as valid internal redirect
-        $target = trim($target);
+        if (false !== ($i = strpos($target, '\\')) && $i < strcspn($target, '?#')) {
+            throw new BadRequestException('Invalid URI: A URI cannot contain a backslash.');
+        }
+        if (\strlen($target) !== strcspn($target, "\r\n\t")) {
+            throw new BadRequestException('Invalid URI: A URI cannot contain CR/LF/TAB characters.');
+        }
+        if ('' !== $target && (\ord($target[0]) <= 32 || \ord($target[-1]) <= 32)) {
+            throw new BadRequestException('Invalid URI: A URI must not start nor end with ASCII control characters or spaces.');
+        }
 
         // handle protocol-relative URLs that parse_url() doesn't like
         if ('//' === substr($target, 0, 2)) {

diff --git a/tests/Listener/ExternalRedirectListenerTest.php b/tests/Listener/ExternalRedirectListenerTest.php
@@ -16,6 +16,7 @@
 use Nelmio\SecurityBundle\EventListener\ExternalRedirectListener;
 use Nelmio\SecurityBundle\ExternalRedirect\AllowListBasedTargetValidator;
 use Nelmio\SecurityBundle\ExternalRedirect\ExternalRedirectResponse;
+use Symfony\Component\HttpFoundation\Exception\BadRequestException;
 use Symfony\Component\HttpFoundation\RedirectResponse;
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpKernel\Exception\HttpException;
@@ -28,7 +29,11 @@ class ExternalRedirectListenerTest extends ListenerTestCase
     public function testRedirectMatcher(string $source, string $target, bool $expected): void
     {
         $listener = new ExternalRedirectListener(true);
-        $result = $listener->isExternalRedirect($source, $target);
+        try {
+            $result = $listener->isExternalRedirect($source, $target);
+        } catch (BadRequestException $e) {
+            $result = true;
+        }
         $this->assertSame($expected, $result);
     }
 
@@ -40,6 +45,8 @@ public function provideRedirectMatcher(): array
             ['http://test.org/', 'https://test.org/foo', false],
             ['http://test.org/', '/foo', false],
             ['http://test.org/', 'foo', false],
+            ['http://test.org/', 'http://test.org/?\\', false],
+            ['http://test.org/', 'http://test.org/#\\', false],
 
             // external
             ['http://test.org/', 'http://example.org/foo', true],
@@ -50,6 +57,11 @@ public function provideRedirectMatcher(): array
             ['http://test.org/', "\r".'http://foo.com/', true],
             ['http://test.org/', "\0\0".'http://foo.com/', true],
             ['http://test.org/', '  http://foo.com/', true],
+            ['http://test.org/', '  http://test.org/', true],
+            ['http://test.org/', "\r".'http://test.org/', true],
+            ['http://test.org/', "\0".'http://test.org/', true],
+            ['http://test.org/', 'http://\test.org/', true],
+            ['http://test.org/', 'http://evil.com\@test.org/', true],
         ];
     }