From ed83d294aef67009c7bf5822775617c402086eda Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Malte=20H=C3=BCbner?= Date: Mon, 13 Jul 2026 16:20:38 +0200 Subject: [PATCH] security: require bearer token for writing API endpoints The writing REST endpoints (PUT /api/station, POST /api/station/{code}, PUT /api/city, POST /api/{citySlug}, PUT /api/value) were completely unauthenticated, allowing anyone to create/overwrite stations, cities and measurements. Add App\EventSubscriber\ApiWriteAuthenticationSubscriber, a single kernel.request enforcement point that challenges every write method (POST/PUT/PATCH/DELETE) below /api with a shared bearer token ("Authorization: Bearer "). The token is configured via the API_WRITE_TOKEN env var and compared with hash_equals. Read endpoints (GET/HEAD, /api/doc) stay public. Fails closed: when API_WRITE_TOKEN is unset, all writes are rejected (403). Missing token => 401, wrong token => 403. Providers must be updated to send the token (tracked separately in luft-api-bundle). A full symfony/security-bundle firewall with per-client tokens remains a possible follow-up. Co-Authored-By: Claude Opus 4.8 (1M context) --- .env | 6 + CLAUDE.md | 4 +- config/services.yaml | 1 + .../ApiWriteAuthenticationSubscriber.php | 89 ++++++++++++++ .../ApiWriteAuthenticationSubscriberTest.php | 111 ++++++++++++++++++ 5 files changed, 209 insertions(+), 2 deletions(-) create mode 100644 src/EventSubscriber/ApiWriteAuthenticationSubscriber.php create mode 100644 tests/EventSubscriber/ApiWriteAuthenticationSubscriberTest.php diff --git a/.env b/.env index 0722230e..a2acf625 100644 --- a/.env +++ b/.env @@ -15,6 +15,12 @@ APP_SECRET=90f2d22c92712fa1cc32be3a26f4e599 DATABASE_URL="postgresql://symfony:ChangeMe@127.0.0.1:5432/app?serverVersion=13&charset=utf8" ###< doctrine/doctrine-bundle ### +# Shared bearer token required for writing API endpoints (PUT/POST/PATCH/DELETE +# below /api). Leave empty to reject all write requests (fail closed); set a +# strong secret in production and configure the providers to send it as +# "Authorization: Bearer ". +API_WRITE_TOKEN= + GRAPH_CACHE_DIRECTORY='/var/www/luft/public/img/graph_cache' OPENWEATHERMAP_APPID='asdf' diff --git a/CLAUDE.md b/CLAUDE.md index 255a3ca7..8446cb52 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -70,7 +70,7 @@ Custom DBAL types in `src/DBAL/Types/`: `StationType`, `AreaType`, UTC datetime REST API under `/api` with Swagger docs at `/api/doc` and OpenAPI JSON at `/api/doc.json`. Routes defined in XML files under `config/routing/` (numbered for load order: `1_static.xml` through `6_city.xml`). -**Note:** API mutation endpoints (PUT/POST) currently have no authentication. See Security section below. +**Note:** API mutation endpoints (PUT/POST/PATCH/DELETE below `/api`) require a shared bearer token, sent as `Authorization: Bearer ` and configured via the `API_WRITE_TOKEN` environment variable. Enforced centrally by `App\EventSubscriber\ApiWriteAuthenticationSubscriber` (fails closed when unset). Read endpoints stay public. ### Frontend @@ -91,7 +91,7 @@ GitHub Actions (`.github/workflows/`): PHPUnit and PHPStan run on push/PR to `ma The following items are known and should be addressed when hardening for production: -- **No API authentication**: PUT/POST endpoints (`/api/station`, `/api/city`, `/api/value`) are unauthenticated +- **API write authentication**: PUT/POST/PATCH/DELETE endpoints below `/api` require a shared bearer token (`API_WRITE_TOKEN`, enforced by `ApiWriteAuthenticationSubscriber`). A per-client token/full firewall is still worth considering. - **CSRF protection disabled**: `csrf_protection` is commented out in `config/packages/framework.yaml` - **No security headers**: CSP, HSTS, X-Frame-Options are not configured - **No rate limiting** on API endpoints diff --git a/config/services.yaml b/config/services.yaml index 3fccbfa5..e9820bf0 100644 --- a/config/services.yaml +++ b/config/services.yaml @@ -16,6 +16,7 @@ services: bind: $graphCacheDirectory: '%env(GRAPH_CACHE_DIRECTORY)%' $redisHost: 'redis://%env(REDIS_HOST)%' + $apiWriteToken: '%env(API_WRITE_TOKEN)%' # makes classes in src/ available to be used as services # this creates a service per class whose id is the fully-qualified class name diff --git a/src/EventSubscriber/ApiWriteAuthenticationSubscriber.php b/src/EventSubscriber/ApiWriteAuthenticationSubscriber.php new file mode 100644 index 00000000..145cfe96 --- /dev/null +++ b/src/EventSubscriber/ApiWriteAuthenticationSubscriber.php @@ -0,0 +1,89 @@ +" header. Read requests + * (GET/HEAD) as well as everything outside /api are left untouched. + * + * Fails closed: if no token is configured, every write request is rejected. + */ +class ApiWriteAuthenticationSubscriber implements EventSubscriberInterface +{ + private const WRITE_METHODS = ['POST', 'PUT', 'PATCH', 'DELETE']; + + public function __construct(protected readonly string $apiWriteToken) + { + } + + #[\Override] + public static function getSubscribedEvents(): array + { + // Run before the controller (and after routing) resolves the request. + return [ + KernelEvents::REQUEST => ['onRequest', 8], + ]; + } + + public function onRequest(RequestEvent $event): void + { + if (!$event->isMainRequest()) { + return; + } + + $request = $event->getRequest(); + + if (!$this->isApiWriteRequest($request)) { + return; + } + + if ('' === $this->apiWriteToken) { + throw new AccessDeniedHttpException('API write access is not configured.'); + } + + $providedToken = $this->extractBearerToken($request); + + if (null === $providedToken) { + throw new UnauthorizedHttpException('Bearer', 'Missing API token.'); + } + + if (!hash_equals($this->apiWriteToken, $providedToken)) { + throw new AccessDeniedHttpException('Invalid API token.'); + } + } + + private function isApiWriteRequest(Request $request): bool + { + $path = $request->getPathInfo(); + + if ('/api' !== $path && !str_starts_with($path, '/api/')) { + return false; + } + + return in_array($request->getMethod(), self::WRITE_METHODS, true); + } + + private function extractBearerToken(Request $request): ?string + { + $header = $request->headers->get('Authorization'); + + if (null === $header || !str_starts_with($header, 'Bearer ')) { + return null; + } + + $token = trim(substr($header, 7)); + + return '' === $token ? null : $token; + } +} diff --git a/tests/EventSubscriber/ApiWriteAuthenticationSubscriberTest.php b/tests/EventSubscriber/ApiWriteAuthenticationSubscriberTest.php new file mode 100644 index 00000000..b44414e3 --- /dev/null +++ b/tests/EventSubscriber/ApiWriteAuthenticationSubscriberTest.php @@ -0,0 +1,111 @@ +assertArrayHasKey(KernelEvents::REQUEST, ApiWriteAuthenticationSubscriber::getSubscribedEvents()); + } + + public function testReadRequestIsNotChallenged(): void + { + $event = $this->dispatch('GET', '/api/station', null); + + $this->assertFalse($event->hasResponse()); + } + + public function testNonApiWriteRequestIsNotChallenged(): void + { + $event = $this->dispatch('POST', '/contact', null); + + $this->assertFalse($event->hasResponse()); + } + + public function testApiDocGetStaysPublic(): void + { + $event = $this->dispatch('GET', '/api/doc', null); + + $this->assertFalse($event->hasResponse()); + } + + public function testWriteWithValidTokenPasses(): void + { + $event = $this->dispatch('PUT', '/api/station', 'Bearer ' . self::TOKEN); + + $this->assertFalse($event->hasResponse()); + } + + public function testWriteWithoutTokenIsUnauthorized(): void + { + $this->expectException(UnauthorizedHttpException::class); + + $this->dispatch('PUT', '/api/station', null); + } + + public function testWriteWithInvalidTokenIsDenied(): void + { + $this->expectException(AccessDeniedHttpException::class); + + $this->dispatch('POST', '/api/DENI200', 'Bearer wrong-token'); + } + + public function testWriteWithNonBearerHeaderIsUnauthorized(): void + { + $this->expectException(UnauthorizedHttpException::class); + + $this->dispatch('PUT', '/api/value', 'Basic ' . base64_encode('a:b')); + } + + public function testUnconfiguredTokenRejectsEveryWrite(): void + { + $this->expectException(AccessDeniedHttpException::class); + + $subscriber = new ApiWriteAuthenticationSubscriber(''); + $subscriber->onRequest($this->createEvent('PUT', '/api/station', 'Bearer ' . self::TOKEN)); + } + + public function testSubRequestIsIgnored(): void + { + $subscriber = new ApiWriteAuthenticationSubscriber(self::TOKEN); + $request = Request::create('/api/station', 'PUT'); + $event = new RequestEvent($this->createStub(HttpKernelInterface::class), $request, HttpKernelInterface::SUB_REQUEST); + + $subscriber->onRequest($event); + + $this->assertFalse($event->hasResponse()); + } + + private function dispatch(string $method, string $path, ?string $authorization): RequestEvent + { + $subscriber = new ApiWriteAuthenticationSubscriber(self::TOKEN); + $event = $this->createEvent($method, $path, $authorization); + + $subscriber->onRequest($event); + + return $event; + } + + private function createEvent(string $method, string $path, ?string $authorization): RequestEvent + { + $request = Request::create($path, $method); + + if (null !== $authorization) { + $request->headers->set('Authorization', $authorization); + } + + return new RequestEvent($this->createStub(HttpKernelInterface::class), $request, HttpKernelInterface::MAIN_REQUEST); + } +}