diff --git a/.env.example b/.env.example index ae94f08c7..80f043305 100644 --- a/.env.example +++ b/.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 diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 4d2a1d9c1..f2febfac9 100644 --- a/.github/workflows/tests.yml +++ b/.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 diff --git a/app/Http/Middleware/SecurityHeaders.php b/app/Http/Middleware/SecurityHeaders.php new file mode 100644 index 000000000..473e85b76 --- /dev/null +++ b/app/Http/Middleware/SecurityHeaders.php @@ -0,0 +1,48 @@ +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; + } +} diff --git a/app/Models/User.php b/app/Models/User.php index 708f85646..59edd00fc 100644 --- a/app/Models/User.php +++ b/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 $ids */ + $ids = config('app.admin_github_ids'); + + if ($ids !== []) { + return in_array((string) $this->github_id, array_map(strval(...), $ids), true); + } + /** @var list $admins */ $admins = config('app.admins'); diff --git a/app/Providers/AppServiceProvider.php b/app/Providers/AppServiceProvider.php index 87bd21f7f..0db37e468 100644 --- a/app/Providers/AppServiceProvider.php +++ b/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 { diff --git a/app/Services/RemotePush.php b/app/Services/RemotePush.php index 04da48776..41d9e0194 100644 --- a/app/Services/RemotePush.php +++ b/app/Services/RemotePush.php @@ -5,9 +5,11 @@ 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; @@ -15,6 +17,15 @@ { 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(); + } } diff --git a/bootstrap/app.php b/bootstrap/app.php index bfc82a8a5..b60c44ebf 100644 --- a/bootstrap/app.php +++ b/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']); diff --git a/config/app.php b/config/app.php index 6045938f4..c6e7bb2bd 100644 --- a/config/app.php +++ b/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 diff --git a/config/trustedproxy.php b/config/trustedproxy.php new file mode 100644 index 000000000..901f15456 --- /dev/null +++ b/config/trustedproxy.php @@ -0,0 +1,34 @@ + env('TRUSTED_PROXIES') ?: [], + +]; diff --git a/phpunit.xml b/phpunit.xml index d59b95bbe..910107a23 100644 --- a/phpunit.xml +++ b/phpunit.xml @@ -19,6 +19,7 @@ + @@ -36,6 +37,7 @@ + diff --git a/tests/Feature/AdminIdentityTest.php b/tests/Feature/AdminIdentityTest.php new file mode 100644 index 000000000..796c15168 --- /dev/null +++ b/tests/Feature/AdminIdentityTest.php @@ -0,0 +1,25 @@ + ['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(); +}); diff --git a/tests/Feature/SecurityHeadersTest.php b/tests/Feature/SecurityHeadersTest.php new file mode 100644 index 000000000..f88a26a15 --- /dev/null +++ b/tests/Feature/SecurityHeadersTest.php @@ -0,0 +1,27 @@ + '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'); +}); diff --git a/tests/Feature/TrustedProxiesTest.php b/tests/Feature/TrustedProxiesTest.php new file mode 100644 index 000000000..55b24c58b --- /dev/null +++ b/tests/Feature/TrustedProxiesTest.php @@ -0,0 +1,39 @@ + (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'); +}); diff --git a/tests/Unit/Jobs/SendRemotePushTest.php b/tests/Unit/Jobs/SendRemotePushTest.php index 051b06ecb..d2331aa92 100644 --- a/tests/Unit/Jobs/SendRemotePushTest.php +++ b/tests/Unit/Jobs/SendRemotePushTest.php @@ -225,3 +225,14 @@ new SendRemotePush($this->message->id, $this->recipient->id)->handle(resolve(RemotePush::class)); Http::assertSent(fn (Request $request): bool => str_contains($request->url(), '.notify.windows.com/') && $request->header('X-WNS-Type') === ['wns/raw'] && $request->header('Content-Type') === ['application/octet-stream'] && $request->header('Urgency') === [] && $request->body() === ''); }); + +it('never contacts a stored endpoint the allowlist no longer accepts, and forgets it', function (): void { + // A row that never went through the rule, such as one from a restored backup or a hand-written fix. + $this->subscription->update(['endpoint' => 'http://169.254.169.254/latest/meta-data/']); + + new SendRemotePush($this->message->id, $this->recipient->id)->handle(resolve(RemotePush::class)); + + Http::assertNothingSent(); + $this->assertDatabaseMissing('push_subscriptions', ['id' => $this->subscription->id]); + $this->assertDatabaseCount('jobs', 0); +});