fix: validate next-redirect targets to block off-origin Location values (SEC_REVIEW F10)

The post-delete `next` form field on AllowlistController and
ManualBlocksController was used verbatim as the 303 Location, with
nothing to stop a forged form (CSRF, sub-XSS auto-submit) from
redirecting an authenticated operator to `//evil.example.com/phish`
or `https://evil.example.com` from the trusted IRDB origin -- a
high-quality phishing pivot.

Add SessionManager::isSafeRedirectPath that accepts only same-origin
paths: must start with `/`, second char must not be `/` or `\`, no
control characters (CR/LF for header injection, NUL). SessionManager::
safeNextOrDefault picks the candidate iff it's safe, else a hard-coded
default. setNext()/consumeNext() also drop unsafe values silently
(defense-in-depth: any future caller is safe even if it skips the
helper).

Both delete handlers now route through safeNextOrDefault, so a
malicious next falls back to the resource list rather than the
attacker URL.

Regression coverage:
- SessionManagerTest: data-provider truth table for isSafeRedirectPath
  (covers protocol-relative, absolute URLs, header injection, control
  chars), setNext drop, consumeNext sanitisation, safeNextOrDefault.
- CrudPagesTest::testManualBlockDeleteRejectsOpenRedirectInNext +
  testAllowlistDeleteRejectsOpenRedirectInNext: forged `next` 303s to
  the safe default. testManualBlockDeleteHonoursSafeNext confirms a
  legitimate same-origin next is preserved.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

ui/src/Auth/SessionManager.php

@@ -173,8 +173,17 @@ class SessionManager
         return $out;
     }
 
+    /**
+     * Stash a post-action redirect target. The value is dropped on the
+     * floor (no error) if it is not a safe local path, so callers can
+     * pass attacker-influenced input directly without an open-redirect
+     * sink (SEC_REVIEW F10).
+     */
     public function setNext(string $url): void
     {
+        if (!self::isSafeRedirectPath($url)) {
+            return;
+        }
         $_SESSION[self::KEY_NEXT] = $url;
     }
 
@@ -182,8 +191,51 @@ class SessionManager
     {
         $next = $_SESSION[self::KEY_NEXT] ?? null;
         unset($_SESSION[self::KEY_NEXT]);
+        if (!is_string($next) || !self::isSafeRedirectPath($next)) {
+            return null;
+        }
+
+        return $next;
+    }
+
+    /**
+     * True iff `$url` is a same-origin path (HTTP `Location` value with
+     * no host/scheme). Rejects:
+     *   - empty,
+     *   - anything not starting with `/`,
+     *   - protocol-relative URLs (`//evil.example.com/phish`),
+     *   - back-slash-after-slash (`/\evil` — Windows-style normalisation
+     *     some clients accept as a host),
+     *   - any control character (CR/LF for header injection, NUL).
+     *
+     * Used by `setNext()`/`consumeNext()` and by the form-`next`
+     * delete redirects in admin controllers (SEC_REVIEW F10).
+     */
+    public static function isSafeRedirectPath(string $url): bool
+    {
+        if ($url === '' || $url[0] !== '/') {
+            return false;
+        }
+        if (strlen($url) >= 2 && ($url[1] === '/' || $url[1] === '\\')) {
+            return false;
+        }
+
+        return preg_match('/[\x00-\x1f\x7f]/', $url) !== 1;
+    }
+
+    /**
+     * Convenience for callers that read a `next` form field: returns the
+     * candidate iff it is a safe local path, else returns `$default`.
+     * `$default` itself is NOT validated — callers MUST pass a known-safe
+     * literal (typically a hard-coded `/app/...` route).
+     */
+    public static function safeNextOrDefault(mixed $candidate, string $default): string
+    {
+        if (is_string($candidate) && self::isSafeRedirectPath($candidate)) {
+            return $candidate;
+        }
 
-        return is_string($next) && $next !== '' ? $next : null;
+        return $default;
     }
 
     /**

ui/src/Controllers/AllowlistController.php

@@ -123,8 +123,14 @@ final class AllowlistController
             $this->flashFromException($e);
         }
 
-        $next = $this->formBody($request)['next'] ?? '/app/allowlist';
-
-        return $response->withStatus(303)->withHeader('Location', is_string($next) && $next !== '' ? $next : '/app/allowlist');
+        // SEC_REVIEW F10: route the attacker-influenced `next` form field
+        // through the same-origin path validator before using it as a
+        // `Location` value.
+        $next = SessionManager::safeNextOrDefault(
+            $this->formBody($request)['next'] ?? null,
+            '/app/allowlist',
+        );
+
+        return $response->withStatus(303)->withHeader('Location', $next);
     }
 }

ui/src/Controllers/ManualBlocksController.php

@@ -165,8 +165,14 @@ final class ManualBlocksController
             $this->flashFromException($e);
         }
 
-        $next = $this->formBody($request)['next'] ?? '/app/manual-blocks';
-
-        return $response->withStatus(303)->withHeader('Location', is_string($next) && $next !== '' ? $next : '/app/manual-blocks');
+        // SEC_REVIEW F10: route the attacker-influenced `next` form field
+        // through the same-origin path validator before using it as a
+        // `Location` value.
+        $next = SessionManager::safeNextOrDefault(
+            $this->formBody($request)['next'] ?? null,
+            '/app/manual-blocks',
+        );
+
+        return $response->withStatus(303)->withHeader('Location', $next);
     }
 }

ui/tests/Integration/Crud/CrudPagesTest.php

@@ -363,6 +363,62 @@ final class CrudPagesTest extends AppTestCase
         self::assertStringNotContainsString('Manually block', $body);
     }
 
+    public function testManualBlockDeleteRejectsOpenRedirectInNext(): void
+    {
+        // SEC_REVIEW F10: an authenticated operator tricked into submitting
+        // a forged form (or hit by a CSRF / sub-XSS auto-submit) MUST NOT
+        // be redirected off-origin via the `next` field. The controller
+        // routes the value through SessionManager::safeNextOrDefault, so
+        // any non-local-path next falls back to the resource list.
+        $token = $this->csrfFromManualBlocks();
+        $this->enqueueApiResponse(204, []);
+
+        $body = http_build_query([
+            'csrf_token' => $token,
+            'next' => '//evil.example.com/phish',
+        ]);
+        $response = $this->request('POST', '/app/manual-blocks/42/delete', [], $body, 'application/x-www-form-urlencoded');
+
+        self::assertSame(303, $response->getStatusCode());
+        self::assertSame('/app/manual-blocks', $response->getHeaderLine('Location'));
+    }
+
+    public function testManualBlockDeleteHonoursSafeNext(): void
+    {
+        // Sanity: a legitimate same-origin next (the in-app pattern, e.g.
+        // returning to an IP detail page) is preserved.
+        $token = $this->csrfFromManualBlocks();
+        $this->enqueueApiResponse(204, []);
+
+        $body = http_build_query([
+            'csrf_token' => $token,
+            'next' => '/app/ips/203.0.113.10',
+        ]);
+        $response = $this->request('POST', '/app/manual-blocks/42/delete', [], $body, 'application/x-www-form-urlencoded');
+
+        self::assertSame(303, $response->getStatusCode());
+        self::assertSame('/app/ips/203.0.113.10', $response->getHeaderLine('Location'));
+    }
+
+    public function testAllowlistDeleteRejectsOpenRedirectInNext(): void
+    {
+        // SEC_REVIEW F10 mirror on the allowlist controller.
+        $this->enqueueApiResponse(200, ['items' => [], 'total' => 0]);
+        $this->request('GET', '/app/allowlist');
+        $token = (string) ($_SESSION[CsrfMiddleware::SESSION_KEY] ?? '');
+        self::assertNotEmpty($token);
+
+        $this->enqueueApiResponse(204, []);
+        $body = http_build_query([
+            'csrf_token' => $token,
+            'next' => 'https://evil.example.com/phish',
+        ]);
+        $response = $this->request('POST', '/app/allowlist/7/delete', [], $body, 'application/x-www-form-urlencoded');
+
+        self::assertSame(303, $response->getStatusCode());
+        self::assertSame('/app/allowlist', $response->getHeaderLine('Location'));
+    }
+
     private function csrfFromManualBlocks(): string
     {
         // GET to set the CSRF token in the session.

ui/tests/Unit/Auth/SessionManagerTest.php

@@ -6,6 +6,7 @@ namespace App\Tests\Unit\Auth;
 
 use App\Auth\SessionManager;
 use App\Auth\UserContext;
+use PHPUnit\Framework\Attributes\DataProvider;
 use PHPUnit\Framework\TestCase;
 
 /**
@@ -171,6 +172,82 @@ final class SessionManagerTest extends TestCase
         self::assertSame(7, $u->userId);
     }
 
+    /**
+     * @return iterable<string, array{0: string, 1: bool}>
+     */
+    public static function isSafeRedirectPathCases(): iterable
+    {
+        // SEC_REVIEW F10 truth table for the open-redirect guard.
+        yield 'simple absolute path' => ['/app/dashboard', true];
+        yield 'absolute path with query' => ['/app/ips/1.2.3.4?tab=a', true];
+        yield 'just the slash' => ['/', true];
+        yield 'protocol-relative URL' => ['//evil.example.com/phish', false];
+        yield 'absolute https URL' => ['https://evil.example.com', false];
+        yield 'absolute http URL' => ['http://evil.example.com', false];
+        yield 'bare hostname' => ['evil.example.com/x', false];
+        yield 'relative path' => ['app/dashboard', false];
+        yield 'empty string' => ['', false];
+        yield 'backslash after slash' => ['/\\evil.example.com', false];
+        yield 'CR header injection' => ["/app\r\nLocation: //evil", false];
+        yield 'LF header injection' => ["/app\nfoo", false];
+        yield 'NUL character' => ["/app\x00", false];
+        yield 'tab character' => ["/app\t", false];
+    }
+
+    #[DataProvider('isSafeRedirectPathCases')]
+    public function testIsSafeRedirectPathTruthTable(string $url, bool $expected): void
+    {
+        self::assertSame($expected, SessionManager::isSafeRedirectPath($url));
+    }
+
+    public function testSetNextDropsUnsafeValueSilently(): void
+    {
+        // SEC_REVIEW F10: `setNext()` is called with attacker-influenced
+        // input from form bodies; an unsafe value MUST NOT enter the
+        // session at all, so a future consumeNext() can't return it.
+        $sm = $this->mgr();
+        $sm->startSession();
+
+        $sm->setNext('//evil.example.com/phish');
+        self::assertNull($sm->consumeNext(), 'unsafe URL was stored in next');
+
+        $sm->setNext('/app/allowlist');
+        self::assertSame('/app/allowlist', $sm->consumeNext());
+    }
+
+    public function testConsumeNextRejectsPreviouslyStoredUnsafeValue(): void
+    {
+        // Defence-in-depth: even if something writes directly to
+        // $_SESSION['_next'], consumeNext() refuses to return an unsafe
+        // value (and clears it).
+        $sm = $this->mgr();
+        $sm->startSession();
+        $_SESSION['_next'] = '//evil.example.com/phish';
+
+        self::assertNull($sm->consumeNext());
+        self::assertArrayNotHasKey('_next', $_SESSION);
+    }
+
+    public function testSafeNextOrDefaultUsesDefaultOnUnsafeOrMissing(): void
+    {
+        self::assertSame(
+            '/app/allowlist',
+            SessionManager::safeNextOrDefault(null, '/app/allowlist'),
+        );
+        self::assertSame(
+            '/app/allowlist',
+            SessionManager::safeNextOrDefault('//evil', '/app/allowlist'),
+        );
+        self::assertSame(
+            '/app/allowlist',
+            SessionManager::safeNextOrDefault(123, '/app/allowlist'),
+        );
+        self::assertSame(
+            '/app/manual-blocks?id=1',
+            SessionManager::safeNextOrDefault('/app/manual-blocks?id=1', '/app/allowlist'),
+        );
+    }
+
     private function mgr(int $idle = 28800): SessionManager
     {
         return new SessionManager(secureCookie: false, idleSeconds: $idle, absoluteSeconds: 86400);