AcadeNice/corentin_wakdo
10
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

fix: audit de la documentation du 30/09 (8 correctifs de securite, documentation recalee) #199

Merged
Corentin merged 39 commits from docs/audit-3009 into dev 2026-09-30 14:11:08 +02:00
Conversation 0 Commits 39 Files changed 69 +3628 -324
Corentin commented 2026-09-30 14:05:06 +02:00
Owner
Copy link

Audit independant de la documentation du 30/09 (5 angles : metier, PHP, front, securite, API) et corrections qui en decoulent.

Code (8 defauts, corriges en TDD, deux revues adverses independantes)

  • A-2 : POST /api/orders/{n}/pay (public) ne repond que pour une commande de la borne ; comptoir/drive = 404 identique a un numero inconnu.
  • D-1 : reverification du mot de passe du profil limitee (compteur de connexion du compte) et tracee (auth.reauth_failed).
  • D-2 : une connexion reussie ne remet plus a zero le compteur de l'IP.
  • D-3 : compteur par compte incremente en SQL atomique (une seule implementation, AccountLockout).
  • D-4 : attribuer un role ou agir sur un compte exige de porter deja ses permissions et ses canaux visibles (sauf role.manage) ; pas de changement de son propre role ; role desactive protege ; listes filtrees.
  • D-5 : courriel de mot de passe oublie envoye apres la reponse, session fermee avant.
  • D-6 : default_route limite a un chemin local, a la saisie et a la redirection.
  • D-7 : jeton de reinitialisation absent des journaux (journal Apache sans chaine de requete, Referrer-Policy: no-referrer sur la page) ; Referrer-Policy de nouveau pose sur les fichiers statiques et erreurs Apache de l'hote admin.

Documentation

53 constats corriges (TVA par produit et non par mode de service, 88 renvois au code recontroles, doc API pour la collection Insomnia, doc securite avec limites connues, chiffres finaux, journal du 30/09), contre-audites par deux relecteurs independants.

Verification (code 17c8fe5)

PHPStan niveau 6 : 0 erreur. PHPUnit : 2 610 tests, 8 924 assertions, 0 echec, 0 depreciation. JS borne : 492. Navigateur : 212 reussis, 11 sautes, 0 echec ; balayage 4 812, 0 echec. Securite 3 phases : 117 reussis, 0 echec. gitleaks : aucune fuite.

Fusion en commit de fusion (pas de squash) pour que les commits cites dans la documentation restent dans l'historique de main.

Audit independant de la documentation du 30/09 (5 angles : metier, PHP, front, securite, API) et corrections qui en decoulent. ## Code (8 defauts, corriges en TDD, deux revues adverses independantes) - A-2 : `POST /api/orders/{n}/pay` (public) ne repond que pour une commande de la borne ; comptoir/drive = 404 identique a un numero inconnu. - D-1 : reverification du mot de passe du profil limitee (compteur de connexion du compte) et tracee (`auth.reauth_failed`). - D-2 : une connexion reussie ne remet plus a zero le compteur de l'IP. - D-3 : compteur par compte incremente en SQL atomique (une seule implementation, `AccountLockout`). - D-4 : attribuer un role ou agir sur un compte exige de porter deja ses permissions et ses canaux visibles (sauf `role.manage`) ; pas de changement de son propre role ; role desactive protege ; listes filtrees. - D-5 : courriel de mot de passe oublie envoye apres la reponse, session fermee avant. - D-6 : `default_route` limite a un chemin local, a la saisie et a la redirection. - D-7 : jeton de reinitialisation absent des journaux (journal Apache sans chaine de requete, `Referrer-Policy: no-referrer` sur la page) ; `Referrer-Policy` de nouveau pose sur les fichiers statiques et erreurs Apache de l'hote admin. ## Documentation 53 constats corriges (TVA par produit et non par mode de service, 88 renvois au code recontroles, doc API pour la collection Insomnia, doc securite avec limites connues, chiffres finaux, journal du 30/09), contre-audites par deux relecteurs independants. ## Verification (code 17c8fe5) PHPStan niveau 6 : 0 erreur. PHPUnit : 2 610 tests, 8 924 assertions, 0 echec, 0 depreciation. JS borne : 492. Navigateur : 212 reussis, 11 sautes, 0 echec ; balayage 4 812, 0 echec. Securite 3 phases : 117 reussis, 0 echec. gitleaks : aucune fuite. Fusion en commit de fusion (pas de squash) pour que les commits cites dans la documentation restent dans l'historique de `main`. 
POST /api/orders/{number}/pay is anonymous and public, and order numbers
are sequential and channel-prefixed (K/C/D + id). OrderRepository::pay()
did not filter by source, so anyone could pay a counter/drive order still
in pending_payment (it would transition to preparing, go to the kitchen,
and decrement stock) or read the status and total of an already-encashed
one, even though those orders are entered by an identified staff member,
not anonymous.

pay() gains an optional restrictToSource parameter, checked before any
status read or write; a mismatch throws ORDER_NOT_FOUND, the same response
as an unknown number, mirroring show()'s existing anti-enumeration guard.
OrderController::pay() (the public route) now restricts to 'kiosk'.
createStaffOrder() keeps calling pay() without a restriction, since it has
just set the order's source itself (already-trusted channel), so the
back-office counter/drive checkout flow is unaffected.
POST /admin/profile/pin re-verified the session user's current password
with no rate limit and no trace: on a shared, already-open session,
anyone could try passwords against the logged-in user without ever
arming the login lockout.

Reuse PinThrottle (pin_throttle, RG-T22) keyed on the session user id:
the gate is evaluated before verify() (a locked account skips argon2id
entirely and gets a generic message), a failed check increments the
throttle and writes an `auth.reauth_failed` audit row in one
transaction, and a success resets the counter.

Adapts ProfileControllerTest (RED first) and the HtmlRoutePinTest /
HtmlRouteHarness shared assertions that encoded the old "no write on
refusal" behavior.
AuthService::recordSuccess() reset login_throttle for the client IP on
every successful login, exactly like the per-account counter. With one
known-valid credential pair, an attacker could alternate 19 failures
and 1 success and the IP cap of 20 would never be reached, leaving
only the per-account counter as a defense.

A success now only resets the account's own counter; the IP row is
left alone and expires on its own via its sliding window
(IP_THROTTLE_WINDOW_SECONDS) or the next failed attempt, which already
resets it in SQL once the window is stale.
AuthService::recordFailure() read failed_login_attempts outside any
transaction (the RG-1 lookup) and wrote back a PHP-computed
currentAttempts + 1, unlike the IP dimension, which already increments
in SQL. Concurrent failed attempts on the same account could read the
same stale value and only advance the counter by one instead of N,
multiplying the number of tries available before the lockout by up to
the size of the PHP-FPM pool.

The account counter is now incremented with an atomic
`failed_login_attempts = failed_login_attempts + 1`, then re-read
under the row lock held by that same transaction to compute the
backoff, mirroring the IP dimension. The unknown-email / locked-account
decoy path keeps emitting the exact same query shape (target id 0) for
timing parity.

Adds a unit test pinning the atomic SQL shape and an integration test
against real MariaDB that replays two failures sharing the same stale
read (via reflection on the now-parameterless recordFailure) and
proves the counter lands on 2, not 1.
UserController::update()/store() only checked that the target role_id
existed and was active (activeRoleExists), never comparing it to the
acting user's own permissions. A role holding only user.update could
therefore assign, remove or keep the role.manage permission on any
account, change an admin's password, deactivate an admin account, reset
an admin's PIN, or anonymise an admin account -- without holding
role.manage itself. The same gap existed in UserApiController.

Add two guards, applied identically to the HTML controller and its JSON
API twin, before any PIN resolution: assignsAdminRoleWithoutPermission()
blocks assigning a role that carries role.manage unless the actor holds
role.manage; targetHoldsAdminRoleWithoutPermission() blocks any mutation
(update, deactivate, reset-pin, erase) on an account whose current role
already carries role.manage, under the same condition. Both query the
Authorizer live (no cached role list), so a custom role that gains
role.manage later falls under the guard immediately.

Also close the related gap named in the same report: a user could change
their own role_id or deactivate their own account through the edit
screen (the deactivate screen already blocked self-deactivation, but the
edit form did not enforce the same rule via the is_active checkbox).

FakeDatabase gains an optional per-role permission override (canByRole)
so tests can express "the actor's role lacks X" and "the target role
carries X" independently -- something the existing role-blind
canResult/grantedCodes fixtures could not express in a single test.

A browser test in security-access.spec.js proves this against a real
MariaDB-backed stack: a purpose-built role holding only user.update
cannot promote a third-party account to the admin role.

Demo impact: none. Per db/seeds/0001_rbac_and_reference.sql, manager
holds user.read only (never user.update/create/deactivate nor
role.manage); admin is the only seeded role with user.update, and it
already holds role.manage. The new guard is a no-op on the shipped
demo accounts.
RoleController::validate() only bounded default_route to 120 characters.
That value is later used, unvalidated, as the automatic post-login
redirect target in AuthService::authenticate(). A value such as
"https://evil.example" or "//evil.example" (protocol-relative, no
scheme needed) would send every account of that role off-site right
after a successful login.

Add App\Auth\RedirectPath, a small shared validator (isLocal/sanitize)
used at two points, as the report asked:
 - at input, RoleController::validate() now rejects any default_route
   that is not a single-leading-slash local path (no "//", no
   backslash, no scheme, no control characters);
 - at redirect time, AuthService::authenticate() falls back to '/' if
   the stored value is not local, as defense in depth for any row
   written before this fix.

The AuthService change is a single line (the defaultRoute computation)
to stay out of the way of the login-throttle work in progress on that
same file, per instructions.
PasswordResetService::requestReset() called the SMTP mailer synchronously,
inline in the request, only when the submitted email resolved to an
active account (the unknown-email path only pays a CPU decoy, no
network call). Under a configured SMTP relay, this let the response
time of POST /forgot_password reveal whether an account exists.

Add App\Core\DeferredActions, a small per-process queue of callables
run after the response is sent. requestReset() now queues the mailer
call instead of invoking it; the admin front controller flushes the
queue in a finally block after Response::send(), calling
fastcgi_finish_request() first when available so the client's
connection already closed before the mail goes out.
GET /reset_password?token=<raw token> is the link sent to reset a
password; the combined LogFormat writes the full request line (%r),
query string included, so the token landed in the access log until it
is consumed or expires (1h).

Add a combined_no_query LogFormat that rebuilds the request line from
the method, the bare path (%U, no query string) and the HTTP version
instead of %r, and route the admin vhost's /reset_password requests to
it via SetEnvIf while every other request keeps the regular combined
format. The access itself (IP, method, path, status, size) is still
logged, only the token is gone.

Also corrects the vhost's stale comment claiming X-Forwarded-For is
written into the combined log: it isn't (no %{X-Forwarded-For}i field
in the format), so %h still logs Traefik's own address, not the real
client's; only the PHP layer (Request::clientIp()) reads that header.

Verified with `httpd -t` against an image built from this Dockerfile,
and end-to-end against a disposable docker-compose stack: before this
change, `docker logs <web>` contained the raw token after a GET to
/reset_password?token=...; after, the same line is logged without it.
Extends the existing brute-force/enumeration suite with two scenarios
run against the disposable e2e stack:
- the profile PIN password re-check (D-1) locks a disposable account
  after 5 wrong current_password attempts and then rejects the right
  one too, without ever saying "Mot de passe actuel incorrect";
- the per-IP login counter (D-2) is not reset by an interleaved
  successful login: 19 failures + 1 success + 1 more failure still
  trips the 20-attempt IP lockout.

Both were verified passing against the full three-phase security suite
(tests/e2e/run-security.sh) alongside the rest of the pre-existing
109 specs, including the reset-link phase that exercises D-5's
deferred mail path end to end.
Adversarial review found that D-5's finally block called
fastcgi_finish_request() then DeferredActions::flush() while the PHP
session (files handler) was still open. The file-based session lock
is only released by session_write_close(), never called anywhere in
this codebase, so a second request carrying the same cookie blocked on
that lock until the deferred SMTP send finished (measured: ~3s versus
0.00s once the session is closed first). The attacker already holds
the cookie (needed for the form's CSRF token), so this second request
reopened the exact timing channel D-5 was meant to close.

Add DeferredActions::finishRequest(), which closes an active session
(session_write_close(), skipped when no session is active), then
finishes the request (fastcgi_finish_request() when the SAPI provides
it), then flushes the queue -- in that order. The three steps take
injectable seams so the ordering is unit-tested without depending on a
real PHP-FPM process. src/public/admin/index.php now calls this single
method instead of inlining the (previously incomplete) sequence.

Reproduced the defect and the fix with two CLI processes sharing a
session save path in a disposable container: closing the session
before the 3s simulated deferred work drops the second reader's wait
from ~3s to 0.00s.
Fix 4 FAUX findings (A-1 to A-5): menu slot fields live under data.slots
(not detail.slots) and now include option_is_orderable_maxi; document the
POST /api/orders/{number}/pay kiosk-only restriction (A-2 code fix already
merged); correct the /api/* auth row (no session, not "public or session");
clarify INVALID_ITEM_TYPE/INVALID_IDEMPOTENCY_KEY are kiosk-only, not shared
with counter/drive order creation.

Document the full POST /api/orders request body (A-4): every field verified
against OrderRepository::resolveHeader/resolveAndTotal/resolveLine, a valid
example checked against live production catalogue data, and the 9 missing
422 codes plus 404 ORDER_NOT_FOUND added to the error code table. Add the
400 INVALID_JSON row to both error tables in demo-api.md (A-12).

Fix the remaining IMPRECIS/COSMETIQUE findings: production status corrected
to "en production depuis la release du 29/09 (aab4e96)" everywhere it was
stale (A-6); commit citations for the docs/contre-audit branch corrected to
point at squash 9e22a21 / PR #197 on main instead of a branch name that
never existed there (A-7); menu slot reconciliation documented as
name-first then position (A-8); DELETE /admin/api/users/{id} 403 on own
account documented, plus a new note generalizing the privilege-escalation
403 guard across all /admin/api/users/* routes (A-9); missing role fields
listed (A-10); price/VAT PIN wording and CSV import PIN field corrected
(A-11); import 409 CONFLICT / 500 IMPORT_FAILED / 403 FORBIDDEN documented,
the "plan" response key added to the example, and the Bruno/HTML-only
extension check clarified (A-15, A-16); the hypothetical order_item route
example marked as such (A-23); PROJECT_CONTEXT.md order-tracking endpoint
description corrected to kiosk-only (A-24); demo-api.md's admin email
claim reconciled with the intentionally-public demo account (A-25).

Not corrected (out of scope, not in the assigned target files): A-13/A-14
(docs/soutenance/oral-blanc-plan-40min.md), A-20 (wiki Comparatif-Back.md),
A-21/A-22 (docs/uml/*). No defect found in the Postman/Bruno collections
themselves, so they are left untouched.
The flux-borne-selection-vers-commande.md v0.5 changelog claimed a full
git-grep-n recalibration that was never actually done on OrderRepository.php
(P-1 to P-9), and a second recalibration was needed after the A-2 kiosk-only
payment fix shifted OrderRepository.php and OrderController.php again. A full
function-by-function re-read of every chemin:ligne citation in the document
also turned up drift in CatalogueController.php, product-options.js,
page-payment.js and state.js that prior recalibrations had missed.

Also corrects the "TVA is tied to service mode" misconception repeated in the
oral plan and slides (TVA is a product attribute, service_mode has no fiscal
role), adds a dated erratum to ADR-0016 for the same misconception, notes the
main-branch commit that carries behavior documented via branch-only commit
hashes in commande.md/catalogue.md, and fixes the stale accessibility contrast
count (934, not 935) and the OpenDyslexic font name.
Adversarial review (revue-code.md, D-4 minor): the guard added in the
previous pass only checked role.manage. A custom role holding just
user.update could still assign a NON-admin role to a user, or keep
editing an account whose current role, holds MORE permissions than the
actor's own role -- as long as role.manage itself was not one of them.
That is still an elevation of privilege, just below the admin
threshold.

Replace the role.manage-specific check with a general rule: an actor
may assign a role, or mutate an account (update/deactivate/reset-pin/
erase), only if the target role's entire permission set is a subset of
the actor's own. An actor holding role.manage is exempt (they can edit
any role anyway, so the comparison would be moot, but role.manage does
not mechanically imply every other permission, hence the explicit
short-circuit rather than relying on the set comparison alone).

assignsAdminRoleWithoutPermission()/targetHoldsAdminRoleWithoutPermission()
are renamed to assignsRoleBeyondActorPermissions()/
targetHoldsRoleBeyondActorPermissions() to describe what they now do;
the user-facing refusal message is untouched. Same logic in
UserController (HTML) and UserApiController (JSON), guard evaluated
before PIN resolution in both, as before.

FakeDatabase gains permissionCodesByRole, a per-role override for the
permission-list queries (Authorizer::permissionsFor /
RoleRepository::permissionCodesFor share one query shape in the double),
needed because canByRole (single permission) cannot express "this
role's whole permission set" for the new set-inclusion check.

Tests (red before, green after): a custom role can no longer be
assigned, or keep touching an account, when it carries a permission
(e.g. stock.manage) the actor lacks, even with role.manage absent from
both sides; a role that is a strict subset of the actor's permissions
is still assignable. Existing role.manage-specific tests are kept and
extended with the matching permissionCodesByRole fixture -- they now
exercise the general rule as its role.manage special case, per
instruction.

Demo unaffected: admin is the only seeded role with user.update, and it
already holds role.manage (all 23 permissions), so it is exempt from
the comparison; manager holds user.read only, never reaching this
guard.
Adversarial review (revue-code.md, D-4 minor): permissionCodesOfRole()
used Authorizer::can()/permissionsFor(), which both filter
role.is_active = 1. That filter is correct for route authorization
(SessionGuard/guard()), but wrong here: this guard protects a TARGET
ACCOUNT, not a route. If the account's role is deactivated, the old
check found no permissions for it and let the mutation through --
reactivating the role afterwards restores those permissions intact, so
nothing was actually protected.

permissionCodesOfRole() now reads role_permission directly through
RoleRepository::permissionCodesFor(), which has no join on role and
therefore no is_active filter. Authorizer::can() itself is untouched:
route authorization still correctly denies everything to a deactivated
role.

FakeDatabase's explicit per-role overrides (canByRole, and the new
permissionCodesByRole from the previous commit) now take precedence
over the roleActive toggle: an explicit statement from the test author
is more specific than the blanket "everything is inactive" fixture
default, which is needed to simulate "the target role is deactivated"
while keeping the acting user authorized on their own permissions in
the same test.

Test (red before, green after): a target account whose current role is
simulated deactivated (roleActive = false) but still carries a
permission the actor lacks is still refused with 403, both on the HTML
controller and the JSON API.
Adversarial review (revue-code.md, D-4 minor): the create/edit user
screen listed every active role in the "Role" dropdown, including ones
the guard would refuse at submit time (roleExceedsPermissions). The
server was already the authority -- submitting one of those roles was
already rejected with 403 -- but the form still suggested a choice
doomed to fail, which is confusing for a non-technical user.

rolesForSelect() now takes the acting GuardResult and the role already
selected in the form (or the account's real current role on the edit
screen), and drops any role whose permission set exceeds the actor's,
unless the actor holds role.manage (sees everything, as before) or the
role is the one already selected -- so the current selection never
silently disappears from the list on a re-render or on the edit screen.

Test (red before, green after): the create form no longer lists a role
that exceeds the actor's own permissions, still lists one that does
not, and an actor with role.manage still sees every role; the edit
screen keeps showing the account's real current role even when it is
one the actor could not have assigned.
Adversarial review (revue-code.md, D-6 minor, two items):

1. RoleController.php: the default_route refusal message spoke of a
   "chemin local" and quoted "//", technical vocabulary for a field
   that is actually a dropdown list ("Page d'accueil après connexion").
   Reworded to "Page d'accueil après connexion invalide : choisissez
   une page de la liste." -- no path/URL jargon, matches the reviewer's
   suggested wording.

2. RedirectPath.php: the docblock above isLocal() described the colon
   check as scheme detection ("sans schema ... deja exclu par la regle
   precedente"), which both misstates what the rule matches (a colon
   right after the first path segment, e.g. /javascript:alert(1), is
   not a URL scheme) and, taken literally, suggests the check is
   redundant. Rewritten to describe the actual bypass pattern it
   guards against.

Test: RoleControllerTest's two default_route refusal assertions now
check for the new plain-language phrase instead of the old technical
wording.
Adversarial review found that closing the raw reset token out of the
access log (D-7) was not enough: the vhost's default
Referrer-Policy: strict-origin-when-cross-origin still lets a browser
attach the full URL, token included, as Referer on every same-origin
request the /reset_password page makes -- its own stylesheet, script,
logo, and its own form submission -- so the token kept landing in the
access log (and in Apache's error log on a proxy failure) via
%{Referer}i, valid until consumed or expired (up to 1h).

Four Apache-side ways to scope a different value to just this one
route were tried and rejected, each verified against a disposable
container: a <Location> block never matches this vhost's rewritten
request; Header setifempty fails to detect a value the backend already
sent and adds a second, ambiguous header; Header ... env= conditions
tied to a SetEnvIf or a RewriteRule [E=...] on the request URI are
never seen as true by mod_headers even though the identical variable
is read correctly by CustomLog on the same request.

The admin vhost no longer sets Referrer-Policy at all; App\Core\
Response::headers() (what send() actually emits) now applies
"strict-origin-when-cross-origin" as a default whenever a controller
hasn't already set the header, so PasswordResetController::
renderConfirm() setting "no-referrer" before send() reliably wins for
/reset_password while every other route keeps the prior value. The
kiosk vhost, purely static, keeps its own unconditional header.

Adds a real-browser Playwright test (security-reset.spec.js) that
loads /reset_password, asserts the response header, then loads the
page's resources and submits its form while listening for any
outgoing Referer -- none is observed.
Adversarial review found that D-1's fix reused PinThrottle (pin_throttle,
keyed on the session user) to rate-limit the /admin/profile/pin password
re-check, sharing it with the sensitive-action PIN gate. Any successful
PIN-gated action resets that same counter to zero for the session user,
even one authorized with a THIRD PARTY's email+PIN
(PinVerifier::resolveActingUser() accepts any active account). On a
shared workstation, a colleague could alternate wrong-password attempts
with harmless PIN-gated actions using their own credentials, and the
password counter would never reach its threshold.

Add App\Auth\AccountLockout, replicating the account dimension of
AuthService's login lockout (same atomic SQL increment, same
ThrottlePolicy, on user.failed_login_attempts/lockout_until) as its own
service. ProfileController::updatePin() now gates, records failures on,
and resets this account-level lockout instead of pin_throttle -- the
same secret (the account password) now has a single budget that only
knowledge of that password can reset.

Adds a unit test that reproduces the exact bypass (a PinThrottle::reset()
call, as any other successful PIN action would trigger, no longer
touches the account lockout) and an end-to-end browser test that
performs the full scenario: 4 wrong-password attempts, a real order
cancellation authorized with a different account's PIN, then proves the
5th attempt is still locked.
Adversarial review found that the existing atomicity test called
AuthService::recordFailure() twice in a row via reflection: two
sequential calls add up under any implementation, including the
original non-atomic one, so the test didn't actually exercise
concurrency.

Replace it with two real PDO connections against the same disposable
MariaDB. Connection B opens a transaction and reads
failed_login_attempts (0, a REPEATABLE READ snapshot); while that
transaction stays open, connection A runs a complete, real
AuthService::authenticate() failure and commits, moving the counter to
1; B then runs the same atomic UPDATE pattern AuthService uses,
inside its own already-open transaction. InnoDB always performs a
current read for UPDATE regardless of the transaction's own snapshot,
so B's increment lands on 2 -- proving the pattern survives a stale
read under a real concurrent commit, something the non-atomic
PHP-computed version would have lost (B would have overwritten A's
commit with 0+1=1).

The docblock is explicit about scope: B replicates the SQL shape
rather than calling the private recordFailure() (which opens its own
transaction and can't nest on the same connection); the sibling unit
test (AuthServiceTest::testAccountCounterUsesAtomicSqlIncrementNot
APhpComputedValue) pins that AuthService actually emits this exact
shape, so the two together cover the full contract.
Second adversarial review found that AuthService::recordFailure()
still inlined its own copy of the account-dimension SQL (atomic
increment, re-read under the row lock, lockout write) even though
AccountLockout::recordFailureWithin() already exists and takes an
open transaction as a parameter -- the exact shape recordFailure()
needed. Two copies of the same security-critical increment risked
drifting apart.

recordFailure() now calls accountLockout()->recordFailureWithin()
inside its own transaction, passing the same target id (0 for an
unknown email or an already-locked account, preserving the timing
parity between the two). The account ThrottlePolicy parameter is no
longer threaded through recordFailure() since AccountLockout builds
its own from the same Config. No SQL text or call count changes, so
every existing unit and integration test (AuthServiceTest,
AuthServiceDbTest, ProfileControllerTest) still passes unmodified.
Second adversarial review (revue-code-2.md, D-4 minor): the D-4 guard
compared permission sets, but not role_visible_source (which order
channels a role's dashboard/API queries can see). A custom role
restricted to the drive channel, holding user.update, could still
assign -- or keep editing -- an account whose role sees more channels
(up to the global view admin/manager have). It gains no permission,
but it widens what it can make a third-party account see, or take over
by logging into it.

An absent role_visible_source row means "sees every channel" (seed
0001 comment: "admin/manager: no rows -> global view"), confirmed by
the existing convention in OrderQueryRepository::visibleSources()
(same normalization, used for the KDS/order queues). UserController's
new visibleSourcesOfRole() applies that same normalization -- empty
rows become the three channels -- without touching
RoleRepository::visibleSources() itself, which the RBAC screen still
needs raw (it renders the actual stored checkboxes).

roleExceedsActorPermissions() now also refuses when the target/new
role's visible-source set is not a subset of the actor's; same
short-circuit for a role.manage holder (exempt, as before).
rolesForSelect() applies the identical comparison so the role dropdown
does not offer a broader-scoped role either. Same code path on the
HTML controller and the JSON API (both call the same protected
methods), guard evaluated before PIN resolution as before.

FakeDatabase gains visibleSourcesByRole, a per-role override for
role_visible_source reads, for the same reason permissionCodesByRole
was needed: expressing "the actor sees only X" and "the target role
sees Y" in a single test.

Tests (red before, green after): a role that sees one extra channel,
or has no visible-source row at all (global view), cannot be assigned
or kept on an account by an actor restricted to fewer channels, even
when permissions are otherwise identical on both sides; a role whose
visible sources are a strict subset is still assignable; the create
form no longer lists a broader-scoped role in the dropdown.

Demo unaffected: no seeded role other than admin holds user.*, and
admin already holds role.manage (exempt from the whole comparison).
Second adversarial review (revue-code-2.md, D-4 minor): ROLE_MANAGE_REQUIRED
said "elle concerne un compte ou un rôle administrateur", written back
when the guard only checked role.manage. Since the guard was
generalized to compare full permission (and now visible-source) sets,
the same message also fires for a non-admin role that is simply more
dotted than the actor -- the wording no longer matched what triggered
it.

Reworded to "Vous ne pouvez pas attribuer ce rôle ni modifier ce
compte : il donne des droits que votre propre rôle n'a pas." -- states
the actual condition (the target has rights the actor lacks) without
claiming "admin" and without naming a raw permission code.

Test: the three UserControllerTest assertions that looked for the old
"gérer les rôles" fragment now look for the new wording.
Second adversarial review (revue-code-2.md, D-6 minor), two leftover
imprecisions in the docblock above isLocal():

- "isLocal() l'a déjà exclu avant d'arriver ici" read as if "ici" were
  somewhere else; the exclusion happens in the very same method's
  first rule. Now says "la première règle de cette méthode
  ($path[0] !== '/')".
- "un motif de contournement connu" claimed a documented bypass
  pattern without citing one. Reworded to say the check is precautionary,
  without the unsourced "connu" (known) qualifier.

No behavior change; RedirectPathTest and RoleControllerTest still pass
unchanged.
Second adversarial review found three related minor gaps:

- testUpdatePinReauthLockoutSurvivesAThirdPartyPinThrottleReset set an
  already-locked account state directly and never actually exercised
  the reset path, so it passed for the wrong reason and could not have
  caught a regression back to pin_throttle. Replaced with a test that
  starts from a prior-attempt count on the account dimension, replays
  a third-party PinThrottle::reset() (a distinct table, no effect by
  construction), submits one more failure, and asserts the account
  lockout is computed from that count -- confirmed to fail against the
  pre-D-1.a ProfileController (reverted locally, reran, restored).
  The true cross-table proof remains the browser test
  (security-bruteforce.spec.js).

- The D-3 integration test's connection B now calls
  AccountLockout::recordFailureWithin() -- real application code, not
  a hand-copied SQL statement -- and also asserts lockout_until stays
  NULL (2 attempts under the threshold), proving the lockout
  computation itself ran correctly under the stale-read interleaving.
  The unsourced [CLAIM L2] now names the MySQL Reference Manual's
  "Consistent Nonlocking Reads" section instead of an unnamed claim.

- DeferredActions' class docblock still described the pre-D-5.a order
  (fastcgi_finish_request() first); it now points to finishRequest()
  for the actual order and why it matters.

- ErrorResponse's HTML error pages (404/405/500) now set
  Referrer-Policy: no-referrer. An uncaught exception on
  /reset_password?token=... would otherwise serve the 500 page with
  the default policy, letting its own stylesheet request carry the
  token in Referer.
Address findings S-1 to S-15 from the independent security documentation
audit (secu.md) and reflect the D-1 to D-7 / A-2 code fixes reviewed twice
adversarially (revue-code.md, revue-code-2.md):

- SECURITY.md: brute-force line now describes the shared account/profile
  lockout budget (D-1) and the IP counter no longer reset on success (D-2);
  Secure cookie described as HTTPS-conditional, not unconditional (S-15);
  RGPD retention corrected (no order purge exists, S-2); new "Limites
  connues" section (NAT-shared IP throttle, unverified client IP, SMTP
  fallback via LogMailer, timing channel).
- PROJECT_CONTEXT.md: R3/R8 risk rows updated with the 30/09 fixes and
  residual limits; PIN reworded "re-authentifie" not "re-autorise" (S-3);
  classification matrix fixed (password_reset_token_hash is SHA-256, not
  argon2id, S-6; password_reset_throttle stores a SHA-256 fingerprint, a
  pseudonymisation not an anonymisation, S-7).
- ARCHITECTURE.md, mlt.md, dictionary.md: audit_log described as
  application-level INSERT-only, not DB-enforced immutability (S-4); PIN
  re-authentication precision propagated (S-3); RG-9 rewritten to match the
  D-2 fix; RG-6 and conventions.md corrected: a role change does not close
  the session, only account deactivation and password reset do (S-13).
- domaines/{auth,rbac,users}.md: document the D-1/D-2/D-3 throttle fixes,
  the D-4 permission-inclusion guard, and the D-6 default_route hardening.
- preuves/10-tests-securite.md: production status corrected to "depuis"
  not "apres" the 29/09 release (S-8); the disabled-role access claim
  narrowed to match SessionGuard's actual behaviour (S-11); stale file:line
  citations on files still in motion replaced with function names; new
  section 8 documents A-2 and D-1 to D-7 with their adversarial-review
  evidence, including the open D-7.b Referrer-Policy regression on the
  admin vhost's Apache-served responses.
- uml/security-sequence.md, uml/sequence-passer-commande.md: add the
  missing JSON-body-read step (A-22) and the "id" field in the payment
  response (A-21).
- ADR-0004, ADR-0005: dated errata on audit_log's real immutability
  guarantee and on the profile-reauth budget that used to wrongly share
  pin_throttle.
- wiki Comparatif-Back.md: the two login throttle counters no longer
  conflated (S-14).

docs/notes/rbac-roles-permissions.md, cited by the audit and listed as a
target, does not exist anywhere in this repository's history; the RBAC
figures it would have carried already live correctly in
docs/demo/matrice-rbac.md.
Second adversarial review found that D-7.a's fix removed
Referrer-Policy from the admin vhost entirely to let PHP set
no-referrer on /reset_password, but that also silently dropped the
header from every response Apache serves on its own: static files
under /assets/*, and Apache-generated errors (403 on .env, 502/503
when PHP-FPM is unreachable) -- a regression versus 9e22a21, where the
header was set unconditionally at server level.

Add back Header always set Referrer-Policy "strict-origin-when-
cross-origin" "expr=-z resp('Referrer-Policy')" to the admin vhost:
the expr condition only fires when the response doesn't already carry
the header, so it covers Apache-only responses without ever competing
with App\Core\Response::headers()'s own default or with
PasswordResetController::renderConfirm()'s no-referrer override.
Verified in a disposable container (httpd -t, then a single compose
stack via _byan-output/outils/e2e.sh) that neither /login nor
/reset_password ever carries two Referrer-Policy lines
(headersArray(), which -- unlike headers() -- exposes duplicate
header occurrences instead of merging them).

Adds two targets to security-headers.spec.js's TARGETS list
(${ADMIN}/assets/css/admin.css and ${ADMIN}/.env) so the suite
actually exercises the response classes that regressed, plus a
dedicated no-duplicate assertion.

Also rewrites the stale D-7.a comments in docker/apache/httpd.conf and
vhost.conf that described the abandoned env=/SetEnvIf mechanism or
claimed the admin vhost set no Referrer-Policy at all, and narrows the
PasswordResetController docblock to point at vhost.conf rather than
repeating the full list of rejected Apache mechanisms a third time.
Close the last untreated findings from the 30/09 documentation audits
(metier, PHP, front, security, API) and the two adversarial code reviews
that followed (D-1 to D-7, A-2):

- security doc leftovers not covered by the earlier security pass (85eb9dc):
  S-3 (PIN "re-authentifie" not "re-autorise" in mct.md and the oral plan/
  slides, the remaining occurrences), S-5 (mlt.md RG-T15 and the oral plan
  Q2.8 now describe the kiosk's actual innerHTML + escHtml() templating,
  not textContent), S-8 (stale "en production apres la release" headers in
  mcd/mld/mct.md, preuves/README.md, the oral plan), S-9 (nuance the "prete"
  transition has no dedicated permission), S-15 (the timing-channel
  measurement is now described as manual and unversioned, not silently
  reproducible).
- API findings outside the API writer's original scope: A-13 (Content-Type
  applies to demo requests 1/3/5, not just 4/6), A-14 (the Insomnia
  collection is built by hand, Postman is only a starting point), A-20
  (wiki Comparatif-Back's stale production banner).
- D-7.b reserve in preuves/10 closed with the a8af3d2 fix detail
  (Referrer-Policy restored on Apache-only admin responses); conventions.md
  updated to match; D-4's role_visible_source residual limit closed
  (c5a8fc4) in preuves/10, PROJECT_CONTEXT.md and domaines/users.md.
- Final test counts (PHPStan 0 errors, PHPUnit 2610/8924, JS 492, browser
  suite 212/11/0, sweep 4812, security suite 117/0) replace the 29/09
  figures wherever they were cited as current, with dated "non rejoue"
  notes for accessibility and the health-page capture; production status
  clarified as still aab4e96, pending the 30/09 release.
- New journal entry for the day's five audits, eight code fixes and two
  adversarial review rounds, plus the day's incidents.

Wiki-next pages (Home, Comparatif-Synthese, Comparatif-Back, Accessibilite,
Journal) updated to match; published separately by the author.
docs(release): apply counter-audit fixes and settle production status for the 30/09 release
All checks were successful
CI / secret-scan (pull_request) Successful in 30s
Details
CI / php-lint (pull_request) Successful in 30s
Details
CI / static-tests (pull_request) Successful in 3m18s
Details
CI / js-tests (pull_request) Successful in 51s
Details
CI / shell-tests (pull_request) Successful in 9s
Details
64e6af701f
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
auto-merge
Fusion automatique sur CI verte

No labels
auto-merge
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
AcadeNice/corentin_wakdo!199
No description provided.