From 4329572855fc14679d361ead92fa028969302542 Mon Sep 17 00:00:00 2001 From: Louis Fortunier Date: Thu, 16 Jul 2026 16:53:42 +0200 Subject: [PATCH] Docs improvements and ADR cleanup --- CHANGELOG.md | 7 ++++ adr/20260709-php-security-audit-migration.md | 39 ++++++-------------- docs/phpstan.md | 7 ++-- docs/phpunit.md | 31 ++++++++++++---- docs/psalm.md | 2 +- 5 files changed, 48 insertions(+), 38 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 708614c..7e8ee43 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,12 @@ CHANGELOG for 1.x =================== +## v1.5.2 - (2026-07-16) +### Changed +- `docs/phpunit.md` : add "Test Class management" section documenting the 1:1 mapping convention between production and test classes; rename fixture examples from `.yml` to `.yaml` and use a more meaningful fixture filename (`create_simulation.yaml` instead of `test_check_entity.yml`) +- `docs/phpstan.md` : translate `ignoreErrors` comment from French to English; clarify the `ekino/phpstan-banned-code` section title and add an explicit note that only `phpstan-disallowed-calls` is used +- `docs/psalm.md` : name `psalm-taint-stubs.php` explicitly in the custom-stub explanation +- `adr/20260709-php-security-audit-migration.md` : remove Drupal 7 rows from Table 1 (Drupal API absent in a Symfony stack); drop the "Provided by" column from Table 2 and remove rows already covered by the Symfony plugin description + ## v1.5.1 - (2026-07-10) ### Added - `docs/phpunit.md` dedicated testing documentation diff --git a/adr/20260709-php-security-audit-migration.md b/adr/20260709-php-security-audit-migration.md index eed8177..c559fbe 100644 --- a/adr/20260709-php-security-audit-migration.md +++ b/adr/20260709-php-security-audit-migration.md @@ -38,18 +38,6 @@ Status legend: βœ… covered Β· 🟑 covered by PHPStan Β· πŸ”΅ covered by compose | BadFunctions/PregReplace | `preg_replace` `/e` modifier + input | β€” | `/e` modifier removed since PHP 7 | ⏳ | | CVE/20132110 | CVE-2013-2110 (PHP < 5.3.26) | **composer audit** | dependency version scan | πŸ”΅ | | CVE/20134113 | CVE-2013-4113 (PHP < 5.3.27) | **composer audit** | dependency version scan | πŸ”΅ | -| Drupal7/SQLi | `db_query()` SQLi (Drupal) | **Psalm** (by intent) | `sql` taint via Doctrine stub (`db_query` API absent) | βœ…βšͺ | -| Drupal7/DynQueries | dynamic queries (where/having…) | **Psalm** (by intent) | `sql` taint via Doctrine stub β€” replaces the intended usage | βœ… | -| Drupal7/DbQueryAC | access control on queries (Drupal) | βšͺ | Drupal-specific access logic | βšͺ | -| Drupal7/AESModule | `db_query()` AES module (Drupal) | βšͺ | Drupal API absent | βšͺ | -| Drupal7/HttpRequest | SSRF via `drupal_http_request` | **Psalm** (by intent) | `ssrf` taint (curl) for the Symfony equivalent | βœ…βšͺ | -| Drupal7/XSSHTMLConstruct | HTML built with input | **Psalm** | `html` taint + Twig (Symfony plugin) | βœ… | -| Drupal7/XSSFormValue | XSS via `#value` (Drupal Form API) | βšͺ | Drupal API absent | βšͺ | -| Drupal7/XSSPTheme | XSS via `#theme`/html_tag (Drupal) | βšͺ | Drupal API absent | βšͺ | -| Drupal7/Cachei | cache injection (Drupal) | βšͺ | Drupal cache API absent | βšͺ | -| Drupal7/UserInputWatch | input detection (Drupal) | βšͺ | Drupal-specific heuristic | βšͺ | -| Drupal7/AdvisoriesContrib | vulnerable Drupal contrib modules | **composer audit** | dependency advisories (no Drupal here) | πŸ”΅βšͺ | -| Drupal7/AdvisoriesCore | outdated Drupal core | **composer audit** | dependency advisories (no Drupal here) | πŸ”΅βšͺ | | Misc/BadCorsHeader | `Access-Control-Allow-Origin: *` | **nelmio_security** (project) | CORS config handled by nelmio/security-bundle at project level, outside the standard-bundle | βœ…βšͺ | | Misc/IncludeMismatch | meta: extensions not scanned by PHPCS | β€” | PHPCS meta-rule, moot outside PHPCS | ❌ | @@ -64,21 +52,18 @@ Status legend: βœ… covered Β· 🟑 covered by PHPStan Β· πŸ”΅ covered by compose ### Table 2 β€” What Psalm taint analysis adds ON TOP (things we didn't have) -| Addition | Detail | Provided by | -|--------------------------------------------|-----------------------------------------------------------------------------------------------------------------------------------------------------------|--------------------------------| -| **Real data-flow analysis** | Source β†’ sink tracking across calls (controller input β†’ sink in a service several calls away). phpcs was single-file / token-based. | Psalm (engine) | -| **Near-zero false positives** | Only flags a proven flow, instead of alerting on every variable near a query (no more mass `phpcs:ignore`). | Psalm (engine) | -| **SQLi on Doctrine ORM/DQL** | `createQuery`, `QueryBuilder::where/andWhere/having/groupBy` marked as SQL sinks β€” phpcs only did pattern matching. | `psalm-taint-stubs.php` stub | -| **XSS in Twig templates** | Taint analysis of Twig templates (unescaped variables). **No** equivalent in phpcs-security-audit. | Symfony plugin | -| **XSS via Symfony Response** | `Response`/HTML content marked as `html` sink. | Symfony plugin | -| **Symfony HTTP sources modeled** | `Request`, `InputBag`, `ParameterBag`, `HeaderBag` (query/request/attributes/headers) as sources, versioned per Symfony release. | Symfony plugin | -| **LDAP injection** | `ldap_search` as `ldap` sink. | Psalm native | -| **Header injection** | `header()` as `header` sink. | Psalm native | -| **Object injection / unserialize** | `unserialize` as `unserialize` sink. | Psalm native | -| **SSRF** | `curl_init/curl_setopt/getimagesize` as `ssrf` sink. | Psalm native | -| **Path/file injection** | `file` family (fopen, file_get_contents, include…). | Psalm native | -| **Cookie injection** | `setcookie` as `cookie` sink. | Psalm native | -| **Tooled false-positive handling** | Dedicated taint baseline (`--set-baseline`/`--use-baseline`) + inline suppression via `@psalm-taint-escape sql` (not `@psalm-suppress`). | Psalm | +| Addition | Detail | +|--------------------------------------------|-----------------------------------------------------------------------------------------------------------------------------------------------------------| +| **Real data-flow analysis** | Source β†’ sink tracking across calls (controller input β†’ sink in a service several calls away). phpcs was single-file / token-based. | +| **Near-zero false positives** | Only flags a proven flow, instead of alerting on every variable near a query (no more mass `phpcs:ignore`). | +| **SQLi on Doctrine ORM/DQL** | `createQuery`, `QueryBuilder::where/andWhere/having/groupBy` marked as SQL sinks β€” phpcs only did pattern matching. | +| **LDAP injection** | `ldap_search` as `ldap` sink. | +| **Header injection** | `header()` as `header` sink. | +| **Object injection / unserialize** | `unserialize` as `unserialize` sink. | +| **SSRF** | `curl_init/curl_setopt/getimagesize` as `ssrf` sink. | +| **Path/file injection** | `file` family (fopen, file_get_contents, include…). | +| **Cookie injection** | `setcookie` as `cookie` sink. | +| **Tooled false-positive handling** | Dedicated taint baseline (`--set-baseline`/`--use-baseline`) + inline suppression via `@psalm-taint-escape sql` (not `@psalm-suppress`). | --- diff --git a/docs/phpstan.md b/docs/phpstan.md index 2cd615b..985ca47 100644 --- a/docs/phpstan.md +++ b/docs/phpstan.md @@ -53,8 +53,8 @@ Otherwise, use the config format to specify a path, for example ```yaml ignoreErrors: - # method.nonObject ignorΓ© dans tests/ : les fixtures initialisent des objets partiels - # dont certaines methodes peuvent retourner null avant d'etre completement renseignes. + # method.nonObject ignored in tests/: fixtures initialize partial objects, + # so some methods might return null before being fully defined. - identifier: method.nonObject path: tests/ @@ -70,7 +70,8 @@ See the PHPStan docs on [ignoreErrors](https://phpstan.org/user-guide/ignoring-e - Taint analysis such as injection/XSS (data-flow) is handled by [Psalm](psalm.md); - dependency CVEs check by `composer audit`. -**A note on `ekino/phpstan-banned-code`.** +**Why we do not use `ekino/phpstan-banned-code`.** +Note: This PHPStan extension is not used in this project. We rely exclusively on `phpstan-disallowed-calls`. - Also listed in the [PHPStan extension library](https://phpstan.org/user-guide/extension-library), it can look like an alternative to `phpstan-disallowed-calls` β€” it isn't: it only bans global functions and a handful of language constructs (`echo`, `print`, `eval`, `exit`, backticks), with no support for banning method calls, static calls, namespaces, constants, superglobals or attributes. diff --git a/docs/phpunit.md b/docs/phpunit.md index 5e178d0..f5c66d9 100644 --- a/docs/phpunit.md +++ b/docs/phpunit.md @@ -33,8 +33,8 @@ Each project has a `tests/Admin/AdminAvailabilityTest.php` extending `AbstractWe public function testPageIsOk(): void { $this->logIn([ - $this->getMinimalFixturesDir() . '/vendor.yml', - $this->getFixturesDir() . '/common/smart_parameter.yml', + $this->getMinimalFixturesDir() . '/vendor.yaml', + $this->getFixturesDir() . '/common/smart_parameter.yaml', ]); $urls = [ @@ -74,8 +74,8 @@ For dynamic URLs (show/edit pages needing an entity id), load a dedicated fixtur public function testDynamicOrganizationPageIsOk(): void { $this->logIn([ - $this->getMinimalFixturesDir() . '/vendor.yml', - $this->getFixturesDir() . '/Admin/AdminAvailabilityTest/organization.yml', + $this->getMinimalFixturesDir() . '/vendor.yaml', + $this->getFixturesDir() . '/Admin/AdminAvailabilityTest/organization.yaml', ]); $urls = [ @@ -96,6 +96,23 @@ public function testDynamicOrganizationPageIsOk(): void **Rule : Every new page added to the project must be added to this test.** +### Test Class management + +When [organizing your test suite in PHPUnit](https://docs.phpunit.de/en/12.5/writing-tests-for-phpunit.html), the industry standard relies on a clean **1:1 mapping** between your production classes and your test classes. Rather than creating isolated test classes for each individual method (like `MethodATest`), you should write a single test class matching your target service (e.g., `ServiceATest` for `ServiceA`). + +Inside this class, the best practice is to split your assertions so that **each test method targets exactly one specific scenario** rather than wrapping all validation logic into a single, giant method. This guarantees that if one assertion fails, PHPUnit doesn't halt early and obscure other potential bugs. + +```text +project/ +β”œβ”€β”€ src/ +β”‚ └── Manager/ +β”‚ └── CloudSimulationManager.php # Contains create() and rollback() methods +β”‚ +└── tests/ + β”œβ”€β”€ Manager/ + β”‚ └── CloudSimulationManagerTest.php # Tests both create() and rollback() +``` + ### Test fixtures management Fixtures are written as [nelmio/alice](https://github.com/nelmio/alice) YAML files and loaded with @@ -117,10 +134,10 @@ tests/ β”‚ └── user_administrator.yaml β”œβ”€β”€ Admin/ β”‚ └── AdminAvailabilityTest/ # data owned by that test class only - β”‚ └── organization.yml + β”‚ └── organization.yaml └── Manager/ └── CloudSimulationManager/ - └── test_check_entity.yml + └── create_simulation.yaml ``` Why this separation: a test must be able to evolve its data without silently breaking other tests. Sharing one big fixture set across the suite @@ -132,7 +149,7 @@ Load them in tests through the `AbstractWebTestCase` helpers: ```php $this->loadFixtureFiles([ $this->getFixturesDir() . '/common/user_administrator.yaml', - $this->getFixturesDir() . '/Manager/CloudSimulationManager/test_check_entity.yml', + $this->getFixturesDir() . '/Manager/CloudSimulationManager/create_simulation.yaml', ]); ``` diff --git a/docs/psalm.md b/docs/psalm.md index abd32c1..550ceac 100644 --- a/docs/psalm.md +++ b/docs/psalm.md @@ -18,7 +18,7 @@ Three pieces make taint analysis understand our Symfony/Doctrine stack: | [`psalm/plugin-symfony`](https://github.com/psalm/psalm-plugin-symfony) | **sources**: Symfony HTTP input (`Request`, `InputBag`, `ParameterBag`, `HeaderBag`) + XSS **sinks** (Twig, `Response`) | | `psalm-taint-stubs.php` (custom) | **sinks**: Doctrine ORM/DBAL SQL methods (`createQuery`, `QueryBuilder::where/andWhere/having/groupBy`, `Connection::executeQuery`…) | -The custom stub is required and cannot be replaced by a plugin: no Psalm plugin marks the Doctrine **ORM** query path as an SQL sink +The custom stub `psalm-taint-stubs.php` is required and cannot be replaced by a plugin: no Psalm plugin marks the Doctrine **ORM** query path as an SQL sink (`weirdan/doctrine-psalm-plugin` only covers low-level DBAL, and its type stubs even collide with ours). Native Psalm already ships SQL sinks for raw `mysqli`/`PDO`, but our code goes through Doctrine.