mirror of
https://github.com/karatektus/pl_mail.git
synced 2026-10-25 01:54:10 +00:00
fix(security): refuse control characters in the calendar returnTo path
`/\t/evil.test` passed the `//` prefix check, and browsers strip tabs and newlines before parsing a URL, so it redirected to another host. The check is now one rule: a slash, not followed by a slash or backslash, and no control character, whitespace or backslash anywhere. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
1 parent
047db65450
commit
f7f591ad3f
2 files changed
+57
-5
No files matched your search
@@ -982,14 +982,16 @@ final class CalendarController extends AbstractController
|
||||
* this is an open-redirect check and not a formality. `//evil.test` is the
|
||||
* case worth naming: a browser reads it as scheme-relative to another host,
|
||||
* and it passes a naive "starts with a slash" test.
|
||||
*
|
||||
* So is `/\t/evil.test`, which passed the `//` check this used to be: the
|
||||
* URL parser strips tab, CR and LF before it looks at the string, so the
|
||||
* browser follows `//evil.test` all the same. Hence one rule rather than a
|
||||
* list of bad prefixes — a slash, then not a slash or backslash, then no
|
||||
* control character, whitespace or backslash anywhere.
|
||||
*/
|
||||
private function safeReturnTo(string $candidate): ?string
|
||||
{
|
||||
if ('' === $candidate || false === str_starts_with($candidate, '/')) {
|
||||
return null;
|
||||
}
|
||||
|
||||
if (true === str_starts_with($candidate, '//') || true === str_contains($candidate, '\\')) {
|
||||
if (1 !== preg_match('#^/(?![/\\\\])[^\x00-\x20\x7F\\\\]*$#D', $candidate)) {
|
||||
return null;
|
||||
}
|
||||
|
||||
|
||||
@@ -0,0 +1,50 @@
|
||||
<?php
|
||||
|
||||
declare(strict_types=1);
|
||||
|
||||
namespace App\Tests\Controller\Calendar;
|
||||
|
||||
use App\Controller\Calendar\CalendarController;
|
||||
use PHPUnit\Framework\Attributes\DataProvider;
|
||||
use PHPUnit\Framework\TestCase;
|
||||
use ReflectionClass;
|
||||
|
||||
/**
|
||||
* The calendar editor's `returnTo` is an open-redirect check.
|
||||
*
|
||||
* `/\t/evil.test` got past the old `//` prefix test, and a browser strips the
|
||||
* tab before parsing, so it followed `//evil.test` — another host.
|
||||
*/
|
||||
final class SafeReturnToTest extends TestCase
|
||||
{
|
||||
/**
|
||||
* @return iterable<string, array{string}>
|
||||
*/
|
||||
public static function offsite(): iterable
|
||||
{
|
||||
yield 'scheme-relative' => ['//evil.test'];
|
||||
yield 'tab smuggled' => ["/\t/evil.test"];
|
||||
yield 'newline smuggled' => ["/\n/evil.test"];
|
||||
yield 'backslash' => ['/\\evil.test'];
|
||||
yield 'absolute' => ['https://evil.test/'];
|
||||
yield 'empty' => [''];
|
||||
}
|
||||
|
||||
#[DataProvider('offsite')]
|
||||
public function testAnOffsiteTargetIsRefused(string $candidate): void
|
||||
{
|
||||
self::assertNull($this->safeReturnTo($candidate));
|
||||
}
|
||||
|
||||
public function testAPathOnThisHostIsKept(): void
|
||||
{
|
||||
self::assertSame('/mail/inbox?pane=1', $this->safeReturnTo('/mail/inbox?pane=1'));
|
||||
}
|
||||
|
||||
private function safeReturnTo(string $candidate): ?string
|
||||
{
|
||||
$controller = new ReflectionClass(CalendarController::class)->newInstanceWithoutConstructor();
|
||||
|
||||
return new \ReflectionMethod(CalendarController::class, 'safeReturnTo')->invoke($controller, $candidate);
|
||||
}
|
||||
}
|
||||
Reference in new issue
Block a user