Found by the #239 authorization ceremony review (finding F20).
Two tables, one shape: a read-then-insert with nothing serialising it, over a natural key the schema does not constrain.
user_session_clients(user_session_id, client_id)
BumpUserSession loops the loaded clients, concludes the current one is absent, and appends. Two concurrent /auth/completed requests both conclude that and both insert.
user_consents(user_id, client_id)
HandleConsentPost calls GetConsentByUserIdAndClientId and creates when it finds nothing. Two concurrent submissions of the same consent form both find nothing and both call CreateUserConsent.
The lasting consequence is on the read side. GetConsentByUserIdAndClientId builds a plain WHERE user_id = ? AND client_id = ? with no ORDER BY and no LIMIT, and getUserConsentCommon takes the first row the driver yields. With two rows carrying different scopes, which one governs is whatever the engine happens to return, and it can differ between the consent screen, handlePromptNone's consent check, and the account consents page. A later UpdateUserConsent then edits one row and leaves the other stale, which is also the territory #115 is in.
Verification notes
Both were checked against the migrations rather than the schema.sql snapshots, since the snapshots are documentation only, and across all four engines. user_sessions does carry UNIQUE (session_identifier), so the omission is specific to these two tables rather than a project-wide habit.
Fix
A unique index on each natural key across all four engines, which turns a silent duplicate into a caught error, plus an upsert or a retry at the two insert sites.
Both tables need a duplicate sweep in the same migration, because existing deployments may already hold pairs. For user_consents the sweep has to decide which row survives; the most recently granted is the defensible choice, and #115 is relevant because granted_at is not refreshed on update today.
Practical notes for whoever writes it
- This is a migration, so it costs more than its Low severity suggests. Four engines, a data-layer test each, and the sweep.
- Next available migration number is 000030 (verify with
ls src/core/data/sqlitedb/migrations | tail rather than trusting this line).
- Only MySQL declares indexes inline; PostgreSQL, SQLite and SQL Server use separate
CREATE UNIQUE INDEX statements. Reading the CREATE TABLE block alone will find a constraint on one engine and miss it on three.
- Per the project's own rule, a migration and a new constraint want data-layer tests, because that tier is the only one that runs against all four engines.
Out of scope
The two-session-rows half of the same concurrency analysis is harder and probably not worth chasing on its own; it is the same territory as #198 and could be folded there.
Related
Found by the #239 authorization ceremony review (finding F20).
Two tables, one shape: a read-then-insert with nothing serialising it, over a natural key the schema does not constrain.
user_session_clients(user_session_id, client_id)BumpUserSessionloops the loaded clients, concludes the current one is absent, and appends. Two concurrent/auth/completedrequests both conclude that and both insert.user_consents(user_id, client_id)HandleConsentPostcallsGetConsentByUserIdAndClientIdand creates when it finds nothing. Two concurrent submissions of the same consent form both find nothing and both callCreateUserConsent.The lasting consequence is on the read side.
GetConsentByUserIdAndClientIdbuilds a plainWHERE user_id = ? AND client_id = ?with noORDER BYand noLIMIT, andgetUserConsentCommontakes the first row the driver yields. With two rows carrying different scopes, which one governs is whatever the engine happens to return, and it can differ between the consent screen,handlePromptNone's consent check, and the account consents page. A laterUpdateUserConsentthen edits one row and leaves the other stale, which is also the territory #115 is in.Verification notes
Both were checked against the migrations rather than the
schema.sqlsnapshots, since the snapshots are documentation only, and across all four engines.user_sessionsdoes carryUNIQUE (session_identifier), so the omission is specific to these two tables rather than a project-wide habit.Fix
A unique index on each natural key across all four engines, which turns a silent duplicate into a caught error, plus an upsert or a retry at the two insert sites.
Both tables need a duplicate sweep in the same migration, because existing deployments may already hold pairs. For
user_consentsthe sweep has to decide which row survives; the most recently granted is the defensible choice, and #115 is relevant becausegranted_atis not refreshed on update today.Practical notes for whoever writes it
ls src/core/data/sqlitedb/migrations | tailrather than trusting this line).CREATE UNIQUE INDEXstatements. Reading theCREATE TABLEblock alone will find a constraint on one engine and miss it on three.Out of scope
The two-session-rows half of the same concurrency analysis is harder and probably not worth chasing on its own; it is the same territory as #198 and could be folded there.
Related
user_consents.granted_atis not refreshed on update) is in the same table and is relevant to the sweep's tie-break.