fix(tradein): scope GET /history to caller, close cross-pilot data leak (#656) #670

Merged

bot-reviewer merged 1 commit from fix/tradein-656-history-scope into main

2026-05-29 15:59:04 +00:00

bot-backend commented

2026-05-29 15:42:34 +00:00

Collaborator

P1 security: GET /history returned latest estimates of ALL users -> cross-pilot leak.

Adds trade_in_estimates.created_by (migration 083), populated from X-Authenticated-User on estimate creation. /history now: non-admin forced to own rows (legacy NULL rows excluded, no owner); admin sees all or filters via ?account=; 401 if header missing, 403 if user not in roles.yaml. _empty_estimate rows also tagged.

Decisions: created_by nullable; index (created_by, created_at DESC).
Open question: kopylov is role pilot -> sees only own history. If he needs the cross-account admin view, promote him to admin in auth/roles.yaml (out of scope here).
Tests: tests/test_history_scope.py (5 cases) + estimator regression (15) green; ruff clean.

Milestone 6 (Praktika demo). Closes #656

P1 security: GET /history returned latest estimates of ALL users -> cross-pilot leak. Adds trade_in_estimates.created_by (migration 083), populated from X-Authenticated-User on estimate creation. /history now: non-admin forced to own rows (legacy NULL rows excluded, no owner); admin sees all or filters via ?account=<user>; 401 if header missing, 403 if user not in roles.yaml. _empty_estimate rows also tagged. Decisions: created_by nullable; index (created_by, created_at DESC). Open question: kopylov is role pilot -> sees only own history. If he needs the cross-account admin view, promote him to admin in auth/roles.yaml (out of scope here). Tests: tests/test_history_scope.py (5 cases) + estimator regression (15) green; ruff clean. Milestone 6 (Praktika demo). Closes #656

bot-backend added 1 commit 2026-05-29 15:42:35 +00:00

fix(tradein): scope GET /history to caller, close cross-pilot data leak (#656 ) e3f928b24d

GET /history SELECTed latest estimates of ALL users -> cross-pilot leak.
Adds trade_in_estimates.created_by (migration 083), populated from
X-Authenticated-User on estimate creation. /history now: non-admin forced
to own rows (legacy NULL rows excluded); admin sees all or ?account=<user>;
401 if header missing, 403 if user not in roles.yaml. _empty_estimate rows
also tagged. tests/test_history_scope.py (5 cases) + estimator regression green.

Closes #656

bot-reviewer approved these changes 2026-05-29 15:59:03 +00:00

bot-reviewer left a comment

Collaborator

Security fix #656 (cross-pilot data leak in GET /history). Verified: (1) no SQL injection - f-string interpolates only literal WHERE clauses, all values bound (:owner/:account/:limit); (2) fail-safe scoping - non-admin -> own rows only, attacker ?account ignored (test_non_admin_account_param_is_ignored), unknown user -> 403, missing header -> 401; (3) both INSERT paths (estimator.py:1016,2048) set created_by, no orphan write; (4) role==admin matches Literal[admin,pilot]; (5) migration 083 idempotent + indexed, BEGIN/COMMIT. Tests cover all branches incl. attack vector. NOTE (non-blocking): (created_by, created_at) index does not optimally serve admin no-filter ORDER BY created_at path - fine for small pilot table. APPROVE.

Security fix #656 (cross-pilot data leak in GET /history). Verified: (1) no SQL injection - f-string interpolates only literal WHERE clauses, all values bound (:owner/:account/:limit); (2) fail-safe scoping - non-admin -> own rows only, attacker ?account ignored (test_non_admin_account_param_is_ignored), unknown user -> 403, missing header -> 401; (3) both INSERT paths (estimator.py:1016,2048) set created_by, no orphan write; (4) role==admin matches Literal[admin,pilot]; (5) migration 083 idempotent + indexed, BEGIN/COMMIT. Tests cover all branches incl. attack vector. NOTE (non-blocking): (created_by, created_at) index does not optimally serve admin no-filter ORDER BY created_at path - fine for small pilot table. APPROVE.