Fix R01-N20: Response::redirect rejects non-path locations

All callers in the app pass a path-only string (`/foo?error=…`,
`/sprints/{id}`, etc.) — but `Response::redirect` accepted any string at
the type system level, so a future caller passing `?next=` from query
input would happily emit an off-origin `Location:` header.

`Response::redirect` now refuses anything that is not a single `/`
followed by something other than `/`, and refuses CR/LF/NUL (header
injection). Protocol-relative URLs (`//evil.example.com/x`) — the
dangerous shape that *looks* like a path — are caught by the second
test (without it, `str_starts_with($location, '/')` would happily wave
them through).

The HTTP→HTTPS canonical redirect in `public/index.php` (the one place
that genuinely needs to leave the origin) moves to the new
`Response::external($url, $status)` helper, which restricts the scheme
to `http`/`https` and requires a non-empty host so a stray
`javascript:` / `data:` payload cannot wind up in the `Location:`
header. Both helpers throw `\InvalidArgumentException` on misuse — loud
in dev/test, harmless under the existing `FatalErrorHandler` safety net
in production. Pure-static `isPathOnly()` and `isExternalUrl()` back
the contract for direct unit tests.

All 56 existing callers were audited: every one passes a `/`-prefixed
literal or a `'/'`-prefixed template, dynamic suffixes are integer IDs /
tokens / `urlencode()` output. None regress under the new guard.

Tests: 302 / 791 (was 266 / 721). New `tests/Http/ResponseTest.php`
(15 cases) pins the accept and reject matrices for both helpers and
the pure helpers separately.

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

SPEC.md

@@ -1475,6 +1475,31 @@ is gone — see `src/Auth/BootstrapAdmin.php`.
       Tests: 227 / 590 (was 211 / 562). Eleventh fix from
       `doc/REVIEW_01.md`.
 
+- [x] **R01-N20 — `Response::redirect` rejects non-path locations**
+      All callers in the app pass a path-only string (`/foo?error=…`,
+      `/sprints/{id}`, etc.) — but `Response::redirect` accepted any
+      string at the type system level, so a future caller passing
+      `?next=` from query input would happily emit an off-origin
+      `Location:` header. `Response::redirect` now refuses anything
+      that is not a single `/` followed by something other than `/`,
+      and also refuses CR/LF/NUL (header injection). Protocol-relative
+      URLs (`//evil.example.com/x`) — the dangerous shape that *looks*
+      like a path — are caught by the second test.
+      The HTTP→HTTPS canonical redirect in `public/index.php` (the
+      one place that genuinely needs to leave the origin) moves to
+      the new `Response::external($url, $status)` helper, which
+      restricts the scheme to `http`/`https` and requires a non-empty
+      host (so a stray `javascript:` or `data:` payload cannot wind up
+      in the header). Both helpers throw
+      `\InvalidArgumentException` on misuse — loud in dev/test, but
+      harmless under the existing `FatalErrorHandler` safety net in
+      production. Pure-static `isPathOnly()` and `isExternalUrl()`
+      back the contract for direct unit tests.
+      New `tests/Http/ResponseTest.php` (15 cases, 36 assertions)
+      pins the accept and reject matrices for both helpers and the
+      pure helpers separately. Tests: 302 / 791 (was 266 / 721).
+      Seventeenth fix from `doc/REVIEW_01.md`.
+
 - [x] **R01-N19 — CSP `report-uri /csp-report` + audit endpoint**
       The strict CSP in `App\Http\FatalErrorHandler::CSP` was silent —
       operators wouldn't notice a future view inadvertently introducing

public/index.php

@@ -263,8 +263,11 @@ $knownHttp = !$requestIsHttps && (
 );
 
 if ($baseIsHttps && $knownHttp && $request->path !== '/healthz') {
+    // Cross-origin (scheme-changing) redirect — must go through
+    // Response::external() because Response::redirect() only accepts
+    // path-only locations now (R01-N20).
     $target = rtrim($baseUrl, '/') . ($_SERVER['REQUEST_URI'] ?? '/');
-    Response::redirect($target, 308)->send();
+    Response::external($target, 308)->send();
     if (ob_get_level() > 0) {
         @ob_end_flush();
     }

src/Http/Response.php

@@ -51,11 +51,89 @@ final class Response
         return self::json(['ok' => false, 'error' => $error], $status);
     }
 
+    /**
+     * Same-origin redirect. The contract is path-only — the location MUST
+     * begin with a single `/` and continue with something other than `/`.
+     * That rules out:
+     *
+     *   - relative paths (`foo`, `?bar=1`) — ambiguous resolution rules,
+     *   - protocol-relative URLs (`//evil.example.com/x`) — silently
+     *     leaves the origin while *looking* like a path,
+     *   - absolute URLs (`https://evil/`) — ditto, more obvious.
+     *
+     * Callers that genuinely need to leave the origin (e.g. the canonical
+     * HTTPS redirect from `public/index.php`) must use `external()`
+     * instead. Refusing the wrong shape at construction time means a
+     * future caller passing user-supplied input by accident gets a
+     * loud `InvalidArgumentException` in dev/test instead of an
+     * undetected open redirect in production (R01-N20).
+     */
     public static function redirect(string $location, int $status = 302): self
     {
+        if (!self::isPathOnly($location)) {
+            throw new \InvalidArgumentException(
+                "Response::redirect requires a path-only location starting with '/' "
+                . "and not '//'. Got: " . self::previewForError($location) . ". "
+                . "Use Response::external() for cross-origin redirects."
+            );
+        }
         return new self($status, '', ['Location' => $location]);
     }
 
+    /**
+     * Cross-origin redirect. Use only when leaving the origin is the
+     * point of the redirect — currently the HTTP→HTTPS canonical hop in
+     * `public/index.php`. Schemes are restricted to `http` / `https` so
+     * a stray `javascript:` / `data:` payload cannot wind up in the
+     * `Location:` header.
+     */
+    public static function external(string $url, int $status = 302): self
+    {
+        if (!self::isExternalUrl($url)) {
+            throw new \InvalidArgumentException(
+                "Response::external requires an http(s):// URL. "
+                . "Got: " . self::previewForError($url)
+            );
+        }
+        return new self($status, '', ['Location' => $url]);
+    }
+
+    public static function isPathOnly(string $location): bool
+    {
+        if ($location === '') {
+            return false;
+        }
+        if ($location[0] !== '/') {
+            return false;
+        }
+        // Reject protocol-relative URLs (`//host/path`).
+        if (isset($location[1]) && $location[1] === '/') {
+            return false;
+        }
+        // Reject control characters / CR-LF (would corrupt the
+        // `Location:` header).
+        return preg_match('/[\r\n\0]/', $location) === 0;
+    }
+
+    public static function isExternalUrl(string $url): bool
+    {
+        if ($url === '' || preg_match('/[\r\n\0]/', $url) !== 0) {
+            return false;
+        }
+        $parts = parse_url($url);
+        if ($parts === false || !isset($parts['scheme'], $parts['host'])) {
+            return false;
+        }
+        $scheme = strtolower($parts['scheme']);
+        return ($scheme === 'http' || $scheme === 'https') && $parts['host'] !== '';
+    }
+
+    private static function previewForError(string $value): string
+    {
+        $clipped = substr($value, 0, 80);
+        return $clipped === $value ? "'{$clipped}'" : "'{$clipped}…'";
+    }
+
     public static function text(string $text, int $status = 200): self
     {
         return new self($status, $text, ['Content-Type' => 'text/plain; charset=utf-8']);

tests/Http/ResponseTest.php

@@ -0,0 +1,139 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Tests\Http;
+
+use App\Http\Response;
+use InvalidArgumentException;
+use PHPUnit\Framework\Attributes\DataProvider;
+use PHPUnit\Framework\TestCase;
+
+/**
+ * R01-N20: pin the redirect contract.
+ *
+ * `Response::redirect` is path-only. `Response::external` is the explicit
+ * cross-origin helper. Both reject CR/LF (header injection) and protocol-
+ * relative shapes (`//host/path`) that look like paths but leave the
+ * origin.
+ */
+final class ResponseTest extends TestCase
+{
+    /** @return list<array{0:string,1:string}> */
+    public static function pathOnlyAccepted(): array
+    {
+        return [
+            ['/',                        'root'],
+            ['/foo',                     'simple path'],
+            ['/users?error=db',          'path with query string'],
+            ['/sprints/1#tab=tasks',     'path with fragment'],
+            ['/users?flash=demoted',     'flash query'],
+            ['/sprints/import/abc-123',  'token-style segment'],
+        ];
+    }
+
+    #[DataProvider('pathOnlyAccepted')]
+    public function testRedirectAcceptsPathOnlyLocations(string $location, string $label): void
+    {
+        $resp = Response::redirect($location);
+        self::assertSame(302, $resp->status, $label);
+        self::assertSame($location, $resp->headers['Location'], $label);
+    }
+
+    public function testRedirectAcceptsCustomStatus(): void
+    {
+        $resp = Response::redirect('/foo', 303);
+        self::assertSame(303, $resp->status);
+    }
+
+    /** @return list<array{0:string,1:string}> */
+    public static function nonPathLocations(): array
+    {
+        return [
+            ['',                                  'empty string'],
+            ['foo',                               'relative path'],
+            ['foo/bar',                           'multi-segment relative'],
+            ['?next=/x',                          'query-only (relative)'],
+            ['#anchor',                           'fragment-only'],
+            ['//evil.example.com/x',              'protocol-relative URL → off-origin redirect'],
+            ['///evil.example.com/x',             'triple-slash variant'],
+            ['https://evil.example.com/x',        'absolute https URL'],
+            ['http://evil.example.com/',          'absolute http URL'],
+            ['javascript:alert(1)',               'javascript: scheme'],
+            ['data:text/html,<script>',           'data: scheme'],
+            ["/foo\r\nX-Injected: yes",           'CR/LF — header injection'],
+            ["/foo\nLocation: https://evil/",     'LF — header injection'],
+            ["/foo\0bar",                         'null byte'],
+        ];
+    }
+
+    #[DataProvider('nonPathLocations')]
+    public function testRedirectRejectsNonPathLocations(string $location, string $label): void
+    {
+        $this->expectException(InvalidArgumentException::class);
+        $this->expectExceptionMessage('path-only');
+        Response::redirect($location);
+    }
+
+    /** @return list<array{0:string,1:string}> */
+    public static function externalAccepted(): array
+    {
+        return [
+            ['https://app.example.com/',                            'https + path'],
+            ['https://app.example.com/sprints/1?tab=foo',           'https + query'],
+            ['http://localhost:8080/healthz',                       'http on localhost with port'],
+            ['HTTPS://APP.EXAMPLE.COM/x',                           'uppercase scheme is normalised at compare'],
+        ];
+    }
+
+    #[DataProvider('externalAccepted')]
+    public function testExternalAcceptsHttpAndHttpsUrls(string $url, string $label): void
+    {
+        $resp = Response::external($url, 308);
+        self::assertSame(308, $resp->status, $label);
+        self::assertSame($url, $resp->headers['Location'], $label);
+    }
+
+    /** @return list<array{0:string,1:string}> */
+    public static function externalRejected(): array
+    {
+        return [
+            ['',                                'empty'],
+            ['/foo',                            'path-only — must use redirect()'],
+            ['//evil.example.com/x',            'protocol-relative'],
+            ['javascript:alert(1)',             'javascript: scheme'],
+            ['file:///etc/passwd',              'file: scheme'],
+            ['ftp://files.example.com/x',       'ftp: scheme'],
+            ['data:text/html,<x>',              'data: scheme'],
+            ['https://',                        'no host'],
+            ['just text',                       'unparseable'],
+            ["https://evil.example.com/\r\n",   'CR/LF in URL'],
+        ];
+    }
+
+    #[DataProvider('externalRejected')]
+    public function testExternalRejectsNonHttpSchemesAndMalformed(string $url, string $label): void
+    {
+        $this->expectException(InvalidArgumentException::class);
+        Response::external($url);
+    }
+
+    public function testIsPathOnlyHelperMatchesRedirectContract(): void
+    {
+        // Drift fence: if these helpers ever diverge from what redirect()
+        // / external() accept, the test above will fail too — but pin the
+        // pure helper independently so refactors are caught instantly.
+        self::assertTrue(Response::isPathOnly('/'));
+        self::assertTrue(Response::isPathOnly('/users'));
+        self::assertFalse(Response::isPathOnly('//evil'));
+        self::assertFalse(Response::isPathOnly(''));
+        self::assertFalse(Response::isPathOnly('users'));
+        self::assertFalse(Response::isPathOnly("/x\nfoo"));
+
+        self::assertTrue(Response::isExternalUrl('https://x.example/'));
+        self::assertTrue(Response::isExternalUrl('http://x.example/'));
+        self::assertFalse(Response::isExternalUrl('/foo'));
+        self::assertFalse(Response::isExternalUrl('javascript:x'));
+        self::assertFalse(Response::isExternalUrl('//x.example/'));
+    }
+}