Security/phase 0 hardening

.env.example

@@ -4,6 +4,13 @@ APP_KEY=
 APP_DEBUG=true
 APP_URL=http://localhost:8000
 ADMINS=
+# Admins by their numeric GitHub ID, which unlike a username never changes. Set it and "ADMINS" is ignored.
+ADMIN_GITHUB_IDS=
+
+# The proxies in front of the app: a comma separated list of IPs or CIDR ranges, or "*" when the app is only
+# reachable through them. Abuse limits key on the client's address, so behind an unset proxy every client
+# shares one budget and one of them can exhaust it for all of them.
+TRUSTED_PROXIES=
 
 APP_LOCALE=en
 APP_FALLBACK_LOCALE=en

.github/workflows/tests.yml

@@ -16,6 +16,27 @@ permissions:
   contents: read
 
 jobs:
+  security:
+    runs-on: ubuntu-latest
+    timeout-minutes: 10
+
+    steps:
+      - name: Checkout
+        uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
+        with:
+          persist-credentials: false
+
+      - name: Setup
+        uses: ./.github/actions/setup
+
+      - name: Audit PHP Dependencies
+        # Abandoned packages are reported separately: one of them is a dev-only dependency of Rector, and
+        # failing every build over it would teach everybody to ignore this job.
+        run: composer audit --locked --no-interaction --abandoned=ignore
+
+      - name: Audit JavaScript Dependencies
+        run: bun audit
+
   lint:
     runs-on: ubuntu-latest
     timeout-minutes: 10

app/Http/Middleware/SecurityHeaders.php

@@ -0,0 +1,48 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Http\Middleware;
+
+use Closure;
+use Illuminate\Http\Request;
+use Symfony\Component\HttpFoundation\Response;
+
+/**
+ * The headers a browser reads before it trusts anything on the page. "PreventFraming" keeps its own concern,
+ * so this only adds what it does not already cover.
+ */
+final readonly class SecurityHeaders
+{
+    /**
+     * Only over HTTPS: browsers ignore the header on plain HTTP, and sending it there would pin local
+     * development to a scheme it does not serve. "includeSubDomains" and "preload" are left off on purpose,
+     * since either one breaks any subdomain that is not HTTPS-only yet.
+     */
+    private const string HSTS = 'max-age=31536000';
+
+    /**
+     * Recording a voice note is the one capability the app asks for, so the microphone stays available to the
+     * page itself and to nobody else. Everything it never asks for is denied outright.
+     */
+    private const string PERMISSIONS = 'camera=(self), microphone=(self), geolocation=(), payment=(), usb=()';
+
+    /**
+     * @param  Closure(Request): (Response)  $next
+     */
+    public function handle(Request $request, Closure $next): Response
+    {
+        $response = $next($request);
+        $headers = $response->headers;
+
+        if ($request->isSecure()) {
+            $headers->set('Strict-Transport-Security', self::HSTS);
+        }
+
+        $headers->set('X-Content-Type-Options', 'nosniff');
+        $headers->set('Referrer-Policy', 'strict-origin-when-cross-origin');
+        $headers->set('Permissions-Policy', self::PERMISSIONS);
+
+        return $response;
+    }
+}

app/Models/User.php

@@ -428,8 +428,20 @@ public function isOnboarded(): bool
         return $this->onboarded_at !== null;
     }
 
+    /**
+     * Admins are matched on their GitHub ID, which never changes. A handle can be renamed away and then
+     * claimed by somebody else, and "UpdateStaleHandles" actively hands released handles over, so a handle
+     * must never grant admin on its own. Handles remain the fallback only while no IDs are configured.
+     */
     public function isAdmin(): bool
     {
+        /** @var list<string> $ids */
+        $ids = config('app.admin_github_ids');
+
+        if ($ids !== []) {
+            return in_array((string) $this->github_id, array_map(strval(...), $ids), true);
+        }
+
         /** @var list<string> $admins */
         $admins = config('app.admins');
 

app/Providers/AppServiceProvider.php

@@ -18,6 +18,7 @@
 use Illuminate\Support\ServiceProvider;
 use Inertia\ExceptionResponse;
 use Inertia\Inertia;
+use LogicException;
 use Tempest\Highlight\Highlighter;
 
 final class AppServiceProvider extends ServiceProvider
@@ -30,11 +31,27 @@ public function register(): void
 
     public function boot(): void
     {
+        $this->ensureProductionIsNotDebugging();
+
         $this->bootRateLimitingDefaults();
 
         $this->bootExceptionHandling();
     }
 
+    /**
+     * A production build that still has "APP_DEBUG" on answers every error with a stack trace, the environment
+     * and the queries that led there. That is worse than being down, so the app refuses to start instead, and
+     * the deploy fails on it rather than the first visitor.
+     */
+    private function ensureProductionIsNotDebugging(): void
+    {
+        throw_if(
+            $this->app->isProduction() && config('app.debug'),
+            LogicException::class,
+            'APP_DEBUG must be false when APP_ENV is "production", as it exposes stack traces, environment variables and queries.',
+        );
+    }
+
     private function bootExceptionHandling(): void
     {
         Inertia::handleExceptionsUsing(function (ExceptionResponse $response): ExceptionResponse|RedirectResponse|null {

app/Services/RemotePush.php

@@ -5,16 +5,27 @@
 namespace App\Services;
 
 use App\Models\PushSubscription;
+use App\Rules\PushEndpoint;
 use Illuminate\Http\Client\ConnectionException;
 use Illuminate\Support\Facades\Config;
 use Illuminate\Support\Facades\Http;
+use Illuminate\Support\Facades\Validator;
 use Minishlink\WebPush\ContentEncoding;
 use Minishlink\WebPush\VAPID;
 
 final readonly class RemotePush
 {
     public function send(PushSubscription $subscription): bool
     {
+        // The endpoint was checked when it was saved, but a row can also come from a restored backup, a
+        // backfill or a hand-written fix, and the allowlist itself can change. So it is checked again here,
+        // and a row that no longer passes goes the way of one its provider answered 404 for.
+        if (! $this->isSupported($subscription->endpoint)) {
+            $subscription->delete();
+
+            return true;
+        }
+
         $auth = VAPID::validate([
             'subject' => Config::string('services.web_push.subject'),
             'publicKey' => Config::string('services.web_push.public_key'),
@@ -42,4 +53,12 @@ public function send(PushSubscription $subscription): bool
 
         return ! $response->serverError() && $response->status() !== 429;
     }
+
+    private function isSupported(string $endpoint): bool
+    {
+        return Validator::make(
+            ['endpoint' => $endpoint],
+            ['endpoint' => ['required', 'string', new PushEndpoint]],
+        )->passes();
+    }
 }

bootstrap/app.php

@@ -11,6 +11,7 @@
 use App\Http\Middleware\HandleInertiaRequests;
 use App\Http\Middleware\PreventFraming;
 use App\Http\Middleware\RedirectBannedUsers;
+use App\Http\Middleware\SecurityHeaders;
 use App\Http\Middleware\SerializePushSessions;
 use App\Http\Middleware\StripNullBytes;
 use App\Http\Middleware\UpdateLastSeenAt;
@@ -42,6 +43,7 @@
         // Before the strings are trimmed, so a value that was only NUL bytes ends up empty like any other blank one.
         $middleware->prepend(StripNullBytes::class);
         $middleware->append(PreventFraming::class);
+        $middleware->append(SecurityHeaders::class);
 
         $middleware->encryptCookies(except: ['appearance', 'sidebar_state', 'who_to_follow', 'who_to_follow_round', 'lane_dimming', 'replies_per_page']);
 

config/app.php

@@ -35,9 +35,11 @@
     | Application Admins
     |--------------------------------------------------------------------------
     |
-    | The GitHub usernames listed here are admins of the application, which
-    | allows them to moderate commits. Set them as a comma separated list
-    | in your ".env" file, for example: "ADMINS=nunomaduro,taylorotwell".
+    | The GitHub accounts listed here are admins of the application, which
+    | allows them to moderate commits. Prefer "admin_github_ids" below: an
+    | account's numeric ID never changes, while its username can be renamed
+    | away and then claimed by somebody else. This list is the fallback for
+    | as long as no IDs are configured.
     |
     */
 
@@ -46,6 +48,22 @@
         explode(',', (string) env('ADMINS', '')),
     ))),
 
+    /*
+    |--------------------------------------------------------------------------
+    | Application Admins, by GitHub ID
+    |--------------------------------------------------------------------------
+    |
+    | The same admins, identified by the numeric GitHub ID that never changes.
+    | Set them as a comma separated list in your ".env" file, for example:
+    | "ADMIN_GITHUB_IDS=1,2". Once this list is set, "admins" above is ignored.
+    |
+    */
+
+    'admin_github_ids' => array_values(array_filter(array_map(
+        trim(...),
+        explode(',', (string) env('ADMIN_GITHUB_IDS', '')),
+    ))),
+
     /*
     |--------------------------------------------------------------------------
     | Application Debug Mode

config/trustedproxy.php

@@ -0,0 +1,34 @@
+<?php
+
+declare(strict_types=1);
+
+return [
+
+    /*
+    |--------------------------------------------------------------------------
+    | Trusted Proxies
+    |--------------------------------------------------------------------------
+    |
+    | The addresses of the load balancers or proxies in front of the app, as a
+    | comma separated list of IPs or CIDR ranges in your ".env" file, or "*"
+    | when the app is only reachable through them.
+    |
+    | Abuse limits key on the client's address, so this decides what "the
+    | client's address" means. Left empty, no proxy is trusted and the
+    | connection's own address is used, which is right for a server that
+    | terminates TLS itself. Behind a proxy that is left unset, every client
+    | shares one address, so one of them can exhaust a limit for all of them.
+    |
+    | Trusting an address that is reachable from the internet lets whoever is
+    | there forge its own, so list only proxies you control.
+    |
+    | An empty list, never null: given null, the framework trusts whichever
+    | address connected whenever the app runs on Laravel Cloud, Forge or
+    | Vapor, and then every caller can set its own address through
+    | "X-Forwarded-For" and spend someone else's rate limit.
+    |
+    */
+
+    'proxies' => env('TRUSTED_PROXIES') ?: [],
+
+];

phpunit.xml

@@ -19,6 +19,7 @@
     </source>
     <php>
         <env name="ADMINS" value=""/>
+        <env name="ADMIN_GITHUB_IDS" value=""/>
         <env name="APP_ENV" value="testing"/>
         <env name="APP_MAINTENANCE_DRIVER" value="file"/>
         <env name="BCRYPT_ROUNDS" value="4"/>
@@ -36,6 +37,7 @@
         <env name="SIGNUP_REQUIRE_VERIFIED_EMAIL" value="false"/>
         <env name="TELESCOPE_ENABLED" value="false"/>
         <env name="TRENDING_TEMPERATURE" value="0"/>
+        <env name="TRUSTED_PROXIES" value=""/>
         <env name="NIGHTWATCH_ENABLED" value="false"/>
         <env name="WEB_PUSH_PUBLIC_KEY" value="" force="true"/>
         <env name="WEB_PUSH_PRIVATE_KEY" value="" force="true"/>

tests/Feature/AdminIdentityTest.php

@@ -0,0 +1,25 @@
+<?php
+
+declare(strict_types=1);
+
+use App\Models\User;
+
+it('matches an admin by GitHub ID rather than by handle', function (): void {
+    config(['app.admin_github_ids' => ['42'], 'app.admins' => []]);
+
+    expect(User::factory()->create(['github_id' => '42', 'handle' => 'renamed-away'])->isAdmin())->toBeTrue()
+        ->and(User::factory()->create(['github_id' => '43', 'handle' => 'admin'])->isAdmin())->toBeFalse();
+});
+
+it('gives nothing to whoever claims the handle a former admin renamed away from', function (): void {
+    config(['app.admin_github_ids' => ['42'], 'app.admins' => ['admin']]);
+
+    expect(User::factory()->create(['github_id' => '43', 'handle' => 'admin'])->isAdmin())->toBeFalse();
+});
+
+it('falls back to handles while no IDs are configured', function (): void {
+    config(['app.admin_github_ids' => [], 'app.admins' => ['admin']]);
+
+    expect(User::factory()->create(['handle' => 'admin'])->isAdmin())->toBeTrue()
+        ->and(User::factory()->create(['handle' => 'someone'])->isAdmin())->toBeFalse();
+});

tests/Feature/SecurityHeadersTest.php

@@ -0,0 +1,27 @@
+<?php
+
+declare(strict_types=1);
+
+use Illuminate\Support\Facades\Route;
+
+beforeEach(function (): void {
+    Route::get('_test/headers', fn (): string => 'ok');
+});
+
+it('sends the headers a browser reads before it trusts the page', function (): void {
+    $this->get('_test/headers')
+        ->assertOk()
+        ->assertHeader('X-Content-Type-Options', 'nosniff')
+        ->assertHeader('Referrer-Policy', 'strict-origin-when-cross-origin')
+        ->assertHeader('Permissions-Policy', 'camera=(self), microphone=(self), geolocation=(), payment=(), usb=()')
+        ->assertHeader('X-Frame-Options', 'DENY')
+        ->assertHeader('Content-Security-Policy', "frame-ancestors 'none'");
+});
+
+it('pins HTTPS only when the request came over it', function (): void {
+    $this->get('https://localhost/_test/headers')
+        ->assertHeader('Strict-Transport-Security', 'max-age=31536000');
+
+    $this->get('http://localhost/_test/headers')
+        ->assertHeaderMissing('Strict-Transport-Security');
+});

tests/Feature/TrustedProxiesTest.php

@@ -0,0 +1,39 @@
+<?php
+
+declare(strict_types=1);
+
+use Illuminate\Http\Request;
+use Illuminate\Support\Facades\Route;
+
+beforeEach(function (): void {
+    Route::get('_test/ip', fn (Request $request): string => (string) $request->ip());
+});
+
+it('believes the forwarded address of a proxy it trusts', function (): void {
+    config(['trustedproxy.proxies' => '10.0.0.0/8']);
+
+    $this->withServerVariables(['REMOTE_ADDR' => '10.1.2.3'])
+        ->withHeaders(['X-Forwarded-For' => '203.0.113.9'])
+        ->get('_test/ip')
+        ->assertSee('203.0.113.9');
+});
+
+it('ignores a forwarded address from anywhere else', function (): void {
+    config(['trustedproxy.proxies' => []]);
+
+    $this->withServerVariables(['REMOTE_ADDR' => '198.51.100.7'])
+        ->withHeaders(['X-Forwarded-For' => '203.0.113.9'])
+        ->get('_test/ip')
+        ->assertSee('198.51.100.7');
+});
+
+it('ignores a forwarded address on a host the framework would otherwise trust', function (): void {
+    // Given null instead of an empty list, the framework trusts whatever connected on these hosts, and a caller
+    // reaching the app directly picks its own address.
+    config(['trustedproxy.proxies' => []]);
+
+    $this->withServerVariables(['REMOTE_ADDR' => '198.51.100.7'])
+        ->withHeaders(['X-Forwarded-For' => '203.0.113.9'])
+        ->get('http://app.on-forge.com/_test/ip')
+        ->assertSee('198.51.100.7');
+});