diff --git a/SECURITY_AUDIT.md b/SECURITY_AUDIT.md index 39cc25a..25a7b0f 100644 --- a/SECURITY_AUDIT.md +++ b/SECURITY_AUDIT.md @@ -11,7 +11,7 @@ | 1 | High | ~~`PrepareEliminationController` has no authorization guard~~ **Fixed 2026-07-13** | `src/Controller/Backoffice/PrepareEliminationController.php:21-65` | | 2 | High | Production PostgreSQL published to host + default-password fallback | `compose.prod.yaml:27-28`, `compose.yaml:11,30` | | 3 | Medium | ~~Spreadsheet formula injection in exports~~ **Fixed 2026-07-13** | `src/Service/QuizSpreadsheetService.php`, `src/Service/DataExportService.php` | -| 4 | Medium | No login throttling / brute-force protection | `config/packages/security.yaml:17-29` | +| 4 | Medium | ~~No login throttling / brute-force protection~~ **Fixed 2026-07-13** | `config/packages/security.yaml:17-29` | | 5 | Medium | Open self-registration grants immediate backoffice access | `src/Controller/RegistrationController.php:40-56` | | 6 | Low | Double-submit race can inflate score | `src/Controller/QuizController.php:120-128` | | 7 | Low | Answer-POST path never checks `isFinalized`/`isLocked` | `src/Controller/QuizController.php:102-131` | @@ -65,13 +65,17 @@ All user-controlled strings were written to XLSX via `setCellValue()`/`fromArray **Fix applied:** Added `src/Helpers/FormulaInjectionSafeValueBinder.php`, a `DefaultValueBinder` override that forces any string starting with `= + - @` (or a leading tab/CR) to be stored as a plain string data type instead of being auto-detected as a formula. Wired it in via `Cell::setValueBinder()` in the constructors of `QuizSpreadsheetService` and `DataExportService`, so it's active before any cell is written. Regression tests added: `tests/Helpers/FormulaInjectionSafeValueBinderTest.php` (unit-level), plus `testQuizToXlsxStoresFormulaLikeAnswerTextAsPlainString` and `testRawAnswersSheetStoresFormulaLikeAnswerTextAsPlainString` (written first, confirmed failing against the old code, now passing). -### 4. No login throttling / brute-force protection +### 4. No login throttling / brute-force protection — FIXED **File:** `config/packages/security.yaml:17-29` -`form_login` has no `login_throttling` and no rate limiter is configured anywhere. `/login` allows unlimited password guessing; the public `POST /` season-code entry (`QuizController.php:35`) is likewise an unthrottled oracle for enumerating the ~3.2M-space season codes (5 chars, 20-consonant alphabet). +`form_login` had no `login_throttling` and no rate limiter was configured anywhere. `/login` allowed unlimited password guessing; the public `POST /` season-code entry (`QuizController.php:35`) was likewise an unthrottled oracle for enumerating the ~3.2M-space season codes (5 chars, 20-consonant alphabet). -**Fix:** Add `login_throttling` and a rate limiter on the public season entry. +**Fix applied:** +- Added `symfony/rate-limiter` as a composer dependency and enabled `login_throttling` (`max_attempts: 5`) on the `main` firewall in `config/packages/security.yaml`, using Symfony's built-in per-user/global rate limiter. +- Added a dedicated `season_code` rate limiter (`framework.rate_limiter`, sliding window, 20 attempts/minute per IP) in `config/packages/framework.yaml`, enforced in `QuizController::selectSeason` — a `TooManyRequestsHttpException` (429) is thrown once the client IP exceeds the limit, before the season-code form is even validated. +- Both limiters have lower `when@test` overrides so tests run fast and deterministically. +- Regression tests added: `testLoginIsThrottledAfterTooManyFailedAttempts` (`tests/Controller/LoginControllerTest.php`) and `testSelectSeasonIsThrottledAfterTooManyAttempts` (`tests/Controller/QuizControllerTest.php`), both written first, confirmed failing against the old config, now passing. ### 5. Open self-registration grants immediate backoffice access diff --git a/composer.json b/composer.json index a5f07e3..c0219c3 100644 --- a/composer.json +++ b/composer.json @@ -36,6 +36,7 @@ "symfony/object-mapper": "8.1.*", "symfony/property-access": "8.1.*", "symfony/property-info": "8.1.*", + "symfony/rate-limiter": "8.1.*", "symfony/runtime": "8.1.*", "symfony/security-bundle": "8.1.*", "symfony/security-csrf": "8.1.*", diff --git a/composer.lock b/composer.lock index 437af3e..2080d32 100644 --- a/composer.lock +++ b/composer.lock @@ -4,7 +4,7 @@ "Read more about it at https://getcomposer.org/doc/01-basic-usage.md#installing-dependencies", "This file is @generated automatically" ], - "content-hash": "fa712d8b03b08daba40ae9f1de32e256", + "content-hash": "8be25e3d256bbcc2d3c36cecac47feed", "packages": [ { "name": "composer/pcre", @@ -6863,6 +6863,80 @@ ], "time": "2026-05-29T05:06:50+00:00" }, + { + "name": "symfony/rate-limiter", + "version": "v8.1.1", + "source": { + "type": "git", + "url": "https://github.com/symfony/rate-limiter.git", + "reference": "dd8f48286c000b8511b9413105f4118c8c2a4bd6" + }, + "dist": { + "type": "zip", + "url": "https://api.github.com/repos/symfony/rate-limiter/zipball/dd8f48286c000b8511b9413105f4118c8c2a4bd6", + "reference": "dd8f48286c000b8511b9413105f4118c8c2a4bd6", + "shasum": "" + }, + "require": { + "php": ">=8.4.1", + "symfony/options-resolver": "^7.4|^8.0" + }, + "require-dev": { + "psr/cache": "^1.0|^2.0|^3.0", + "symfony/lock": "^7.4|^8.0" + }, + "type": "library", + "autoload": { + "psr-4": { + "Symfony\\Component\\RateLimiter\\": "" + }, + "exclude-from-classmap": [ + "/Tests/" + ] + }, + "notification-url": "https://packagist.org/downloads/", + "license": [ + "MIT" + ], + "authors": [ + { + "name": "Wouter de Jong", + "email": "wouter@wouterj.nl" + }, + { + "name": "Symfony Community", + "homepage": "https://symfony.com/contributors" + } + ], + "description": "Provides a Token Bucket implementation to rate limit input and output in your application", + "homepage": "https://symfony.com", + "keywords": [ + "limiter", + "rate-limiter" + ], + "support": { + "source": "https://github.com/symfony/rate-limiter/tree/v8.1.1" + }, + "funding": [ + { + "url": "https://symfony.com/sponsor", + "type": "custom" + }, + { + "url": "https://github.com/fabpot", + "type": "github" + }, + { + "url": "https://github.com/nicolas-grekas", + "type": "github" + }, + { + "url": "https://tidelift.com/funding/github/packagist/symfony/symfony", + "type": "tidelift" + } + ], + "time": "2026-06-09T11:06:24+00:00" + }, { "name": "symfony/routing", "version": "v8.1.0", diff --git a/config/packages/framework.yaml b/config/packages/framework.yaml index fc45578..9c49989 100644 --- a/config/packages/framework.yaml +++ b/config/packages/framework.yaml @@ -1,7 +1,7 @@ # see https://symfony.com/doc/current/reference/configuration/framework.html framework: secret: '%env(APP_SECRET)%' - + # Note that the session will be started ONLY if you read or write from it. session: true form: @@ -9,6 +9,13 @@ framework: enabled: true #esi: true #fragments: true + rate_limiter: + # Throttles guesses against the public season-code entry point (config/packages/security.yaml + # handles throttling for the backoffice /login form separately). + season_code: + policy: sliding_window + limit: 20 + interval: '1 minute' when@prod: framework: # shortcut for private IP address ranges of your proxy @@ -21,3 +28,6 @@ when@test: test: true session: storage_factory_id: session.storage.factory.mock_file + rate_limiter: + season_code: + limit: 3 diff --git a/config/packages/security.yaml b/config/packages/security.yaml index 378d16d..479a740 100644 --- a/config/packages/security.yaml +++ b/config/packages/security.yaml @@ -27,6 +27,8 @@ security: remember_me: secret: '%kernel.secret%' lifetime: 604800 # 1 week in seconds + login_throttling: + max_attempts: 5 access_control: - { path: ^/admin, roles: ROLE_ADMIN } @@ -43,3 +45,7 @@ when@test: cost: 4 # Lowest possible value for bcrypt time_cost: 3 # Lowest possible value for argon memory_cost: 10 # Lowest possible value for argon + firewalls: + main: + login_throttling: + max_attempts: 2 diff --git a/config/reference.php b/config/reference.php index 31b811b..309f186 100644 --- a/config/reference.php +++ b/config/reference.php @@ -629,7 +629,7 @@ use Symfony\Component\Config\Loader\ParamConfigurator as Param; * }>, * }, * rate_limiter?: bool|array{ // Rate limiter configuration - * enabled?: bool|Param, // Default: false + * enabled?: bool|Param, // Default: true * limiters?: arraycreateForm(SelectSeasonType::class); $form->handleRequest($request); + if ($form->isSubmitted() && !$this->seasonCodeLimiter->create($request->getClientIp())->consume()->isAccepted()) { + throw new TooManyRequestsHttpException(); + } + if ($form->isSubmitted() && $form->isValid()) { $seasonCode = $form->get('season_code')->getData(); diff --git a/tests/Controller/LoginControllerTest.php b/tests/Controller/LoginControllerTest.php index 2ad24d0..b530997 100644 --- a/tests/Controller/LoginControllerTest.php +++ b/tests/Controller/LoginControllerTest.php @@ -11,6 +11,14 @@ use Tvdt\Controller\LoginController; #[CoversClass(LoginController::class)] final class LoginControllerTest extends AbstractControllerWebTestCase { + protected function setUp(): void + { + parent::setUp(); + + // Login attempts are rate-limited; start each test with a clean quota. + self::getContainer()->get('cache.rate_limiter')->clear(); + } + public function testLoginPageLoadsWhenNotAuthenticated(): void { $this->client->request(Request::METHOD_GET, '/login'); @@ -60,6 +68,30 @@ final class LoginControllerTest extends AbstractControllerWebTestCase self::assertSelectorTextContains('body', 'Ongeldige inloggegevens.'); } + public function testLoginIsThrottledAfterTooManyFailedAttempts(): void + { + for ($i = 0; $i < 2; ++$i) { + $this->client->request(Request::METHOD_GET, '/login'); + $form = $this->client->getCrawler()->filter('form')->form([ + '_username' => 'test@example.org', + '_password' => 'wrong-password', + ]); + $this->client->submit($form); + self::assertResponseRedirects('/login'); + } + + $this->client->request(Request::METHOD_GET, '/login'); + $form = $this->client->getCrawler()->filter('form')->form([ + '_username' => 'test@example.org', + '_password' => 'wrong-password', + ]); + $this->client->submit($form); + + self::assertResponseRedirects('/login'); + $this->client->followRedirect(); + self::assertSelectorTextContains('body', 'Te veel onjuiste inlogpogingen'); + } + public function testLogoutIsInterceptedByFirewall(): void { $this->loginAs('test@example.org'); diff --git a/tests/Controller/QuizControllerTest.php b/tests/Controller/QuizControllerTest.php index a5f88ef..25e3e4c 100644 --- a/tests/Controller/QuizControllerTest.php +++ b/tests/Controller/QuizControllerTest.php @@ -17,6 +17,14 @@ use Tvdt\Helpers\Base64; #[CoversClass(QuizController::class)] final class QuizControllerTest extends AbstractControllerWebTestCase { + protected function setUp(): void + { + parent::setUp(); + + // Season-code guessing is rate-limited; start each test with a clean quota. + self::getContainer()->get('cache.rate_limiter')->clear(); + } + private function answerQuestion(Question $question): void { $tomHash = Base64::base64UrlEncode('Tom'); @@ -69,6 +77,26 @@ final class QuizControllerTest extends AbstractControllerWebTestCase self::assertResponseRedirects('/krtek'); } + public function testSelectSeasonIsThrottledAfterTooManyAttempts(): void + { + for ($i = 0; $i < 3; ++$i) { + $crawler = $this->client->request(Request::METHOD_GET, '/'); + $form = $crawler->filter('form')->form([ + 'select_season[season_code]' => 'aaaaa', + ]); + $this->client->submit($form); + self::assertResponseRedirects('/'); + } + + $crawler = $this->client->request(Request::METHOD_GET, '/'); + $form = $crawler->filter('form')->form([ + 'select_season[season_code]' => 'aaaaa', + ]); + $this->client->submit($form); + + self::assertResponseStatusCodeSame(429); + } + public function testEnterNamePageLoads(): void { $this->client->request(Request::METHOD_GET, '/krtek');