fix(csrf): validate against the presented session, not the most recent (6c-3)
RequireCSRF looked up csrf_token by user_id ORDER BY created_at DESC LIMIT 1 — the newest session, not the one whose Bearer token was presented. Two consequences: created_at's 1-second resolution made two same-second sessions a coin-flip (phantom 403s), and conceptually it accepted "any valid CSRF for this user" rather than the one bound to this session. Now queried by token_hash = SHA256(bearer) AND user_id. Authenticate already located this row + checked expiry, so the expiry clause is dropped. Signature unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -668,6 +668,13 @@ prochain démarrage.
|
|||||||
|
|
||||||
### 6c-3. 🟡 Le CSRF est validé contre la mauvaise session
|
### 6c-3. 🟡 Le CSRF est validé contre la mauvaise session
|
||||||
|
|
||||||
|
> ✅ **RÉSOLU 2026-07-16.** `RequireCSRF` lit désormais `csrf_token WHERE
|
||||||
|
> token_hash = :th AND user_id = :uid` (la session présentée), plus de
|
||||||
|
> `ORDER BY created_at DESC LIMIT 1`. `ExtractBearerToken`/`SHA256Hex` déjà
|
||||||
|
> utilisés par `Authenticate` dans la même unité ; signature inchangée, aucun
|
||||||
|
> call-site touché. Le re-test d'expiry est retiré (Authenticate tourne avant
|
||||||
|
> et l'a déjà fait, désormais sur la même base UTC après §6c-2).
|
||||||
|
|
||||||
[`RequireCSRF`](delphi-backend/Source/PM.Session.pas:120) cherche le
|
[`RequireCSRF`](delphi-backend/Source/PM.Session.pas:120) cherche le
|
||||||
`csrf_token` par `user_id ORDER BY created_at DESC LIMIT 1` — la session la plus
|
`csrf_token` par `user_id ORDER BY created_at DESC LIMIT 1` — la session la plus
|
||||||
récente, **pas celle réellement présentée**. Deux conséquences :
|
récente, **pas celle réellement présentée**. Deux conséquences :
|
||||||
|
|||||||
@@ -101,7 +101,7 @@ end;
|
|||||||
procedure RequireCSRF(ARequest: TIdHTTPRequestInfo;
|
procedure RequireCSRF(ARequest: TIdHTTPRequestInfo;
|
||||||
AResponse: TIdHTTPResponseInfo; AUserId: Integer);
|
AResponse: TIdHTTPResponseInfo; AUserId: Integer);
|
||||||
var
|
var
|
||||||
LSubmitted, LStored: string;
|
LSubmitted, LStored, LTokenHash: string;
|
||||||
LQ: TFDQuery;
|
LQ: TFDQuery;
|
||||||
begin
|
begin
|
||||||
if SameText(ARequest.Command, 'GET') then Exit;
|
if SameText(ARequest.Command, 'GET') then Exit;
|
||||||
@@ -113,6 +113,14 @@ begin
|
|||||||
raise ESessionRejected.Create('missing csrf');
|
raise ESessionRejected.Create('missing csrf');
|
||||||
end;
|
end;
|
||||||
|
|
||||||
|
// Bind the CSRF check to the session actually PRESENTED (token_hash), not
|
||||||
|
// "the user's most recent session" (§6c-3). The old ORDER BY created_at
|
||||||
|
// could read a different session's token — and created_at's 1-second
|
||||||
|
// resolution made two same-second sessions a coin-flip → phantom 403s.
|
||||||
|
// user_id stays as a filter (defense in depth). Expiry isn't re-checked:
|
||||||
|
// Authenticate always runs before RequireCSRF and already did it.
|
||||||
|
LTokenHash := SHA256Hex(ExtractBearerToken(ARequest));
|
||||||
|
|
||||||
DB.Lock;
|
DB.Lock;
|
||||||
try
|
try
|
||||||
LQ := TFDQuery.Create(nil);
|
LQ := TFDQuery.Create(nil);
|
||||||
@@ -120,8 +128,8 @@ begin
|
|||||||
LQ.Connection := DB.Connection;
|
LQ.Connection := DB.Connection;
|
||||||
LQ.SQL.Text :=
|
LQ.SQL.Text :=
|
||||||
'SELECT csrf_token FROM sessions ' +
|
'SELECT csrf_token FROM sessions ' +
|
||||||
'WHERE user_id = :uid AND expires_at > datetime(''now'') ' +
|
'WHERE token_hash = :th AND user_id = :uid';
|
||||||
'ORDER BY created_at DESC LIMIT 1';
|
LQ.ParamByName('th').AsString := LTokenHash;
|
||||||
LQ.ParamByName('uid').AsInteger := AUserId;
|
LQ.ParamByName('uid').AsInteger := AUserId;
|
||||||
LQ.Open;
|
LQ.Open;
|
||||||
if LQ.IsEmpty then
|
if LQ.IsEmpty then
|
||||||
|
|||||||
Reference in New Issue
Block a user