From 9a2a9572311639872d66f650f66edffe066dd1e4 Mon Sep 17 00:00:00 2001 From: Tom Wu Date: Mon, 11 May 2026 19:49:36 -0400 Subject: [PATCH 1/6] 9.x cleanup: resolve 6 deferred Codex P2s in one bundle MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the deferred follow-up list across Sprint 9 PRs #78, #81, #82, #84, #85 so the closeout doc (9.8) ships against a clean slate. P2 fixes: 1. /forgot-password concurrent-request race (api/routers/password_reset.py). Add with_for_update() to the Person lookup so two parallel forgot-password calls serialize on the Person row. Without it both transactions could invalidate-then-insert, leaving 2 valid reset tokens (Scenario 6 in docs/features/password-reset.md says fresh request must invalidate older links). SQLite ignores FOR UPDATE (global lock already serializes writers); PostgreSQL enforces a row lock under READ COMMITTED. 2. Settings.EMAIL_ENABLED default mismatch (api/core/config.py). Flip default from True to False to match EmailService's effective env-read gate (defaults to false per #78). Without alignment, notification_service._should_queue_email() reads True from Settings and queues Celery email jobs that the worker silently no-ops. 3. Refresh-in-flight signOut race (mobile/lib/api/api_client.dart). Before persisting tokens from /auth/refresh response, re-read the refresh slot and confirm it still matches the token we sent. If signOut/clearAll ran during the round-trip, storage is empty or replaced — drop the write. Otherwise the dio interceptor silently resurrects the prior session after explicit logout. New regression test in refresh_test.dart. 4. invitation_repository absent-refresh handling (mobile/lib/auth/invitation_repository.dart). Mirror the #82 fix from login_repository / signup_repository: clear any prior refresh token when the server omits one, so accepting an invitation from a pre-9.4b backend can't inherit the previous user's refresh credential. 5. email_smoke.py shell-env leak (scripts/email_smoke.py + docs/saas/SMOKE_TESTING_EMAIL.md). load_dotenv(override=True) so a blank SENDGRID_API_KEY in .env wins over a stale exported value; added warning when both SendGrid and Mailtrap creds are populated; runbook now prefixes Path A with `unset SENDGRID_API_KEY` as a belt-and-braces guarantee. 6. Codex review base ref (CLAUDE.md). Replace --base main with --base origin/main on both review invocations + add `git fetch origin main` prelude. Reviewing against local main runs against a stale base when local hasn't fetched recently. Co-Authored-By: Claude Opus 4.7 (1M context) --- CLAUDE.md | 11 ++++-- api/core/config.py | 7 +++- api/routers/password_reset.py | 10 ++++++ docs/saas/SMOKE_TESTING_EMAIL.md | 14 ++++++++ mobile/lib/api/api_client.dart | 9 +++++ mobile/lib/auth/invitation_repository.dart | 5 +++ mobile/test/refresh_test.dart | 42 ++++++++++++++++++++++ scripts/email_smoke.py | 17 ++++++++- 8 files changed, 111 insertions(+), 4 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 07e10802..31775fb4 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -128,13 +128,20 @@ This repository uses local Codex review through `openai/codex-plugin-cc`. Before shipping or merging a PR, run Codex review from Claude Code: ``` -/codex:review --base main +git fetch origin main +/codex:review --base origin/main ``` +Use the remote ref (`origin/main`), not local `main` — if your checkout +hasn't fetched recently, reviewing against local `main` runs the +comparison against a stale base and the verdict won't reflect the diff +GitHub will actually merge. + For larger changes, prefer background review: ``` -/codex:review --base main --background +git fetch origin main +/codex:review --base origin/main --background /codex:status /codex:result ``` diff --git a/api/core/config.py b/api/core/config.py index 8c0b30e2..bb676aba 100644 --- a/api/core/config.py +++ b/api/core/config.py @@ -36,7 +36,12 @@ class Settings(BaseSettings): # Email Service (General) EMAIL_FROM: str = "noreply@signupflow.io" EMAIL_FROM_NAME: str = "SignUpFlow" - EMAIL_ENABLED: bool = True + # Default off, matching EmailService.__init__ (api/services/email_service.py). + # Both gates must agree — otherwise notification_service._should_queue_email() + # reads True from Settings and queues Celery jobs that the worker silently + # no-ops because EmailService.enabled is False. See docs/saas/SMOKE_TESTING_EMAIL.md + # for how to flip this on (env: EMAIL_ENABLED=true). + EMAIL_ENABLED: bool = False SMS_ENABLED: bool = False # Mailtrap (Development Email Testing) diff --git a/api/routers/password_reset.py b/api/routers/password_reset.py index fdc1a36b..8cb5b086 100644 --- a/api/routers/password_reset.py +++ b/api/routers/password_reset.py @@ -111,9 +111,19 @@ def request_password_reset( # bypass invitation/onboarding and inherit the row's roles, since # /reset-password writes ``password_hash`` unconditionally. Treat them # like an unknown email: no token, generic response. + # + # ``with_for_update()`` serializes concurrent /forgot-password calls + # for the same Person. Without it, two parallel requests can each pass + # the "invalidate prior tokens" UPDATE before either INSERT commits, + # leaving two valid reset tokens in the table — the freshness contract + # in docs/features/password-reset.md Scenario 6 then doesn't hold. + # The row lock is held until db.commit() below. SQLite ignores + # FOR UPDATE (its global lock already serializes writers); PostgreSQL + # enforces a real row lock under READ COMMITTED. person = ( db.query(Person) .filter(Person.email == request.email, Person.password_hash.isnot(None)) + .with_for_update() .first() ) diff --git a/docs/saas/SMOKE_TESTING_EMAIL.md b/docs/saas/SMOKE_TESTING_EMAIL.md index 2b8abadd..8e5c1ca4 100644 --- a/docs/saas/SMOKE_TESTING_EMAIL.md +++ b/docs/saas/SMOKE_TESTING_EMAIL.md @@ -44,6 +44,20 @@ Mailtrap captures every send to a virtual inbox you can view in the browser. Nothing leaves their network — safe to use without any domain authentication. +> **Shell prelude (important):** before running the smoke, ensure no +> stale `SENDGRID_API_KEY` is exported in your shell: +> +> ```bash +> unset SENDGRID_API_KEY +> ``` +> +> The smoke script reloads `.env` with `override=True`, so a blank +> `SENDGRID_API_KEY=` line in `.env` will clear it for the run; but an +> exported value in the shell can still leak in via subprocess +> inheritance for the Python interpreter that imports SendGrid. The +> unset is a belt-and-braces guarantee that you're testing the SMTP +> path, not accidentally hitting live SendGrid. + ### 1. Get credentials 1. Sign up / log in at . diff --git a/mobile/lib/api/api_client.dart b/mobile/lib/api/api_client.dart index 110647f9..060db01b 100644 --- a/mobile/lib/api/api_client.dart +++ b/mobile/lib/api/api_client.dart @@ -113,6 +113,15 @@ Future _attemptRefresh(Dio dio, SecureTokenStorage storage) async { if (body is Map && body['token'] is String && body['refresh_token'] is String) { + // If signOut() / clearAll() ran while /auth/refresh was in flight, + // the refresh slot in storage is now empty (or replaced by a fresh + // login's token). Persisting the response would resurrect the old + // session after the user explicitly logged out. Drop the write. + final stillStored = await storage.readRefreshToken(); + if (stillStored != refreshToken) { + completer.complete(false); + return false; + } await storage.writeToken(body['token'] as String); await storage.writeRefreshToken(body['refresh_token'] as String); completer.complete(true); diff --git a/mobile/lib/auth/invitation_repository.dart b/mobile/lib/auth/invitation_repository.dart index 557c8aa6..9717ce08 100644 --- a/mobile/lib/auth/invitation_repository.dart +++ b/mobile/lib/auth/invitation_repository.dart @@ -88,9 +88,14 @@ class InvitationRepository { // Persist the refresh token (Sprint 9 PR 9.4b makes invitation accept // mint real JWT pairs like login/signup). Pre-9.4b backends omit the // field; treat null/empty as "no refresh — fall back to re-login". + // Mirror login_repository / signup_repository: clear any prior refresh + // token when the response omits one, so accepting an invitation can't + // inherit the previous session's refresh credential. final refreshToken = data.refreshToken; if (refreshToken != null && refreshToken.isNotEmpty) { await _storage.writeRefreshToken(refreshToken); + } else { + await _storage.clearRefreshToken(); } return AuthState( role: LoginRepository.resolveRole(data.roles.toList()), diff --git a/mobile/test/refresh_test.dart b/mobile/test/refresh_test.dart index 98febb53..e38b702c 100644 --- a/mobile/test/refresh_test.dart +++ b/mobile/test/refresh_test.dart @@ -130,6 +130,48 @@ void main() { expect(await storage.readToken(), isNull); }); + test('refresh in flight when storage is cleared does NOT resurrect tokens', () async { + // P2 from #82: a 401-triggered /auth/refresh that's mid-flight when + // signOut()/clearAll() runs must not persist the response, otherwise + // the user who explicitly logged out gets silently re-authed. + final storage = InMemoryTokenStorage(); + await storage.writeToken('expired_access'); + await storage.writeRefreshToken('valid_refresh'); + + final adapter = _FakeAdapter((opts) async { + if (opts.path.endsWith('/auth/refresh')) { + // Delay so we can simulate signOut clearing storage *during* the + // refresh round-trip. + await Future.delayed(const Duration(milliseconds: 30)); + return _json( + 200, + '{"token":"resurrected_access","refresh_token":"resurrected_refresh"}', + ); + } + return _json(401, '{"detail":"expired"}'); + }); + + final container = _container(storage); + addTearDown(container.dispose); + final dio = container.read(dioProvider)..httpClientAdapter = adapter; + + // Kick off the request that triggers refresh. + final pending = dio.get('/api/v1/events'); + // While refresh is in flight, simulate signOut clearing storage. + await Future.delayed(const Duration(milliseconds: 10)); + await storage.clearAll(); + + // The request itself fails (no valid auth could be restored). + await expectLater(pending, throwsA(isA())); + await Future.delayed(const Duration(milliseconds: 10)); + + // Critical: storage must remain cleared. The refresh response + // returned new tokens after clearAll(), but the interceptor must + // detect the storage mismatch and drop the write. + expect(await storage.readToken(), isNull); + expect(await storage.readRefreshToken(), isNull); + }); + test('two concurrent 401s coalesce to a single /auth/refresh call', () async { final storage = InMemoryTokenStorage(); await storage.writeToken('expired_access'); diff --git a/scripts/email_smoke.py b/scripts/email_smoke.py index b68f012c..6e615289 100644 --- a/scripts/email_smoke.py +++ b/scripts/email_smoke.py @@ -32,7 +32,22 @@ def main() -> int: ) args = parser.parse_args() - load_dotenv() + # override=True so a blank SENDGRID_API_KEY in .env actually wins over a + # stale `export SENDGRID_API_KEY=...` from the operator's shell — without + # it, dotenv silently keeps the shell value and the Mailtrap smoke would + # accidentally hit live SendGrid. + load_dotenv(override=True) + + sendgrid_set = bool(os.getenv("SENDGRID_API_KEY")) + mailtrap_set = bool(os.getenv("MAILTRAP_SMTP_USER")) + if sendgrid_set and mailtrap_set: + print( + "warning: both SENDGRID_API_KEY and MAILTRAP_SMTP_USER are set. " + "EmailService will pick SendGrid (real send). If you intended " + "the Mailtrap smoke, blank SENDGRID_API_KEY in .env or " + "`unset SENDGRID_API_KEY` in your shell.", + file=sys.stderr, + ) if os.getenv("TESTING", "").lower() == "true": print( From 6bdd770b198270bb47b6cd584f09daa539066195 Mon Sep 17 00:00:00 2001 From: Tom Wu Date: Mon, 11 May 2026 20:06:58 -0400 Subject: [PATCH 2/6] 9.x cleanup iter 2: address Codex findings on the cleanup PR itself Codex caught three issues in the first iteration that meaningfully undermined the fixes' value. Each addressed: 1. password_reset row lock was released by log_audit_event's commit (api/routers/password_reset.py). Moved with_for_update() out of the initial Person lookup (which is followed by a committing audit log) and into a tight re-acquire right before the invalidate+insert critical section. Lock is now held until the token-rotation commit. 2. Refresh-in-flight signOut race fix was wiping fresh logins (mobile/lib/api/api_client.dart). The original change made _attemptRefresh return false on stale; the interceptor then ran clearAll(), clobbering a user who'd just signed back in mid-flight. Introduced _RefreshOutcome enum with success/failure/stale. The interceptor only clearAll()s on failure; stale leaves storage alone. New regression test in refresh_test.dart covers the fresh-login case explicitly. 3. email_smoke.py override=True overrode every env var, not just the SendGrid key. A shell-set EMAIL_ENABLED=false or TESTING=true safety toggle could be silently overridden by .env values. Replaced with a narrow override: read .env via dotenv_values(), and only force SENDGRID_API_KEY-blank when .env explicitly sets it that way. Co-Authored-By: Claude Opus 4.7 (1M context) --- api/routers/password_reset.py | 23 ++++++++------- mobile/lib/api/api_client.dart | 53 ++++++++++++++++++++++------------ mobile/test/refresh_test.dart | 41 ++++++++++++++++++++++++++ scripts/email_smoke.py | 17 +++++++---- 4 files changed, 100 insertions(+), 34 deletions(-) diff --git a/api/routers/password_reset.py b/api/routers/password_reset.py index 8cb5b086..48f00920 100644 --- a/api/routers/password_reset.py +++ b/api/routers/password_reset.py @@ -111,19 +111,9 @@ def request_password_reset( # bypass invitation/onboarding and inherit the row's roles, since # /reset-password writes ``password_hash`` unconditionally. Treat them # like an unknown email: no token, generic response. - # - # ``with_for_update()`` serializes concurrent /forgot-password calls - # for the same Person. Without it, two parallel requests can each pass - # the "invalidate prior tokens" UPDATE before either INSERT commits, - # leaving two valid reset tokens in the table — the freshness contract - # in docs/features/password-reset.md Scenario 6 then doesn't hold. - # The row lock is held until db.commit() below. SQLite ignores - # FOR UPDATE (its global lock already serializes writers); PostgreSQL - # enforces a real row lock under READ COMMITTED. person = ( db.query(Person) .filter(Person.email == request.email, Person.password_hash.isnot(None)) - .with_for_update() .first() ) @@ -141,6 +131,19 @@ def request_password_reset( if not person: return generic_response + # Re-acquire the Person row with ``FOR UPDATE`` immediately before the + # critical section (invalidate prior tokens + insert new + commit). + # The initial Person lookup above can't hold the lock, because + # ``log_audit_event`` commits the audit row in between — which would + # release any row lock taken on the original query. Re-locking here + # scopes the lock tightly to the rotation block: two concurrent + # /forgot-password requests for the same person serialize on this + # SELECT, then run the UPDATE/INSERT/commit one at a time, so the + # freshness contract in docs/features/password-reset.md Scenario 6 + # holds. SQLite ignores FOR UPDATE (its global lock already serializes + # writers); PostgreSQL enforces a real row lock under READ COMMITTED. + db.query(Person).filter(Person.id == person.id).with_for_update().one() + now = utcnow() # Invalidate any prior unused reset tokens for this person before diff --git a/mobile/lib/api/api_client.dart b/mobile/lib/api/api_client.dart index 060db01b..3bd3675a 100644 --- a/mobile/lib/api/api_client.dart +++ b/mobile/lib/api/api_client.dart @@ -17,10 +17,16 @@ const String defaultApiBaseUrl = String.fromEnvironment( defaultValue: 'http://localhost:8000', ); +/// Outcome of one /auth/refresh attempt. The interceptor must distinguish +/// "refresh failed" (wipe storage) from "refresh succeeded but storage was +/// mutated mid-flight" (leave storage alone) — otherwise a signOut / fresh +/// login that races a stale refresh response gets clobbered. +enum _RefreshOutcome { success, failure, stale } + /// Pump a single concurrent /auth/refresh attempt. If two requests hit /// 401 at the same time, only one fires the refresh; the others await /// the same `Future` and replay against the new token. -Completer? _refreshInFlight; +Completer<_RefreshOutcome>? _refreshInFlight; /// dio configured with the API base URL + a token-refresh interceptor. final dioProvider = Provider((ref) { @@ -52,14 +58,21 @@ final dioProvider = Provider((ref) { return; } - final refreshed = await _attemptRefresh(dio, storage); - if (!refreshed) { + final outcome = await _attemptRefresh(dio, storage); + if (outcome == _RefreshOutcome.failure) { // No refresh token, refresh failed, or 401 from /auth/refresh // itself — wipe both tokens and let the router bounce to /login. await storage.clearAll(); handler.next(e); return; } + if (outcome == _RefreshOutcome.stale) { + // Storage was mutated (signOut / fresh login) while /auth/refresh + // was in flight. Don't replay with the dropped tokens, but also + // don't clear the user's current session — leave storage alone. + handler.next(e); + return; + } // Replay the original request with the new access token. try { @@ -81,22 +94,24 @@ final dioProvider = Provider((ref) { }); /// Fires `/auth/refresh` against the stored refresh token. Coalesces -/// concurrent calls so only one round-trip happens at a time. Returns -/// true on success (new tokens persisted), false otherwise. -Future _attemptRefresh(Dio dio, SecureTokenStorage storage) async { +/// concurrent calls so only one round-trip happens at a time. The +/// outcome tells the caller whether to (success) replay the original +/// request, (failure) clear storage and propagate 401, or (stale) just +/// propagate 401 without touching storage. +Future<_RefreshOutcome> _attemptRefresh(Dio dio, SecureTokenStorage storage) async { // Coalesce concurrent refreshes. final inFlight = _refreshInFlight; if (inFlight != null) { return inFlight.future; } - final completer = Completer(); + final completer = Completer<_RefreshOutcome>(); _refreshInFlight = completer; try { final refreshToken = await storage.readRefreshToken(); if (refreshToken == null || refreshToken.isEmpty) { - completer.complete(false); - return false; + completer.complete(_RefreshOutcome.failure); + return _RefreshOutcome.failure; } final res = await dio.post( '/api/v1/auth/refresh', @@ -116,22 +131,24 @@ Future _attemptRefresh(Dio dio, SecureTokenStorage storage) async { // If signOut() / clearAll() ran while /auth/refresh was in flight, // the refresh slot in storage is now empty (or replaced by a fresh // login's token). Persisting the response would resurrect the old - // session after the user explicitly logged out. Drop the write. + // session or clobber a brand-new login. Return `stale` so the + // interceptor neither replays the original request nor wipes the + // current session's tokens. final stillStored = await storage.readRefreshToken(); if (stillStored != refreshToken) { - completer.complete(false); - return false; + completer.complete(_RefreshOutcome.stale); + return _RefreshOutcome.stale; } await storage.writeToken(body['token'] as String); await storage.writeRefreshToken(body['refresh_token'] as String); - completer.complete(true); - return true; + completer.complete(_RefreshOutcome.success); + return _RefreshOutcome.success; } - completer.complete(false); - return false; + completer.complete(_RefreshOutcome.failure); + return _RefreshOutcome.failure; } on DioException { - completer.complete(false); - return false; + completer.complete(_RefreshOutcome.failure); + return _RefreshOutcome.failure; } finally { _refreshInFlight = null; } diff --git a/mobile/test/refresh_test.dart b/mobile/test/refresh_test.dart index e38b702c..035ac479 100644 --- a/mobile/test/refresh_test.dart +++ b/mobile/test/refresh_test.dart @@ -172,6 +172,47 @@ void main() { expect(await storage.readRefreshToken(), isNull); }); + test('refresh in flight when user signs back in does NOT clobber fresh login', () async { + // Companion to the previous test: if storage is *replaced* (not just + // cleared) by a fresh sign-in during the refresh round-trip, the + // stale refresh response must neither persist the old tokens nor + // call clearAll() on the fresh tokens. Storage must reflect the + // fresh login at the end. + final storage = InMemoryTokenStorage(); + await storage.writeToken('expired_access'); + await storage.writeRefreshToken('old_refresh'); + + final adapter = _FakeAdapter((opts) async { + if (opts.path.endsWith('/auth/refresh')) { + await Future.delayed(const Duration(milliseconds: 30)); + return _json( + 200, + '{"token":"old_session_new_access","refresh_token":"old_session_new_refresh"}', + ); + } + return _json(401, '{"detail":"expired"}'); + }); + + final container = _container(storage); + addTearDown(container.dispose); + final dio = container.read(dioProvider)..httpClientAdapter = adapter; + + final pending = dio.get('/api/v1/events'); + // Mid-flight: user signs back in with new credentials. + await Future.delayed(const Duration(milliseconds: 10)); + await storage.writeToken('fresh_login_access'); + await storage.writeRefreshToken('fresh_login_refresh'); + + await expectLater(pending, throwsA(isA())); + await Future.delayed(const Duration(milliseconds: 10)); + + // Fresh login's tokens must survive. The interceptor's "stale" + // outcome must NOT call clearAll() — that would log out a user who + // had just signed in. + expect(await storage.readToken(), 'fresh_login_access'); + expect(await storage.readRefreshToken(), 'fresh_login_refresh'); + }); + test('two concurrent 401s coalesce to a single /auth/refresh call', () async { final storage = InMemoryTokenStorage(); await storage.writeToken('expired_access'); diff --git a/scripts/email_smoke.py b/scripts/email_smoke.py index 6e615289..c90d4c59 100644 --- a/scripts/email_smoke.py +++ b/scripts/email_smoke.py @@ -19,7 +19,7 @@ import sys from datetime import datetime, timezone -from dotenv import load_dotenv +from dotenv import dotenv_values, load_dotenv def main() -> int: @@ -32,11 +32,16 @@ def main() -> int: ) args = parser.parse_args() - # override=True so a blank SENDGRID_API_KEY in .env actually wins over a - # stale `export SENDGRID_API_KEY=...` from the operator's shell — without - # it, dotenv silently keeps the shell value and the Mailtrap smoke would - # accidentally hit live SendGrid. - load_dotenv(override=True) + load_dotenv() + # Narrow override: if .env *explicitly* sets SENDGRID_API_KEY to blank, + # force-clear any stale exported value so the Mailtrap smoke path can't + # be hijacked by a leftover `export SENDGRID_API_KEY=...` in the shell. + # We deliberately don't pass override=True to load_dotenv because that + # would also overwrite operator-set safety toggles like TESTING=true or + # EMAIL_ENABLED=false in the shell. + env_file_values = dotenv_values() + if "SENDGRID_API_KEY" in env_file_values and not env_file_values["SENDGRID_API_KEY"]: + os.environ.pop("SENDGRID_API_KEY", None) sendgrid_set = bool(os.getenv("SENDGRID_API_KEY")) mailtrap_set = bool(os.getenv("MAILTRAP_SMTP_USER")) From a1b7bc56a2da75b9803c0e1e6a417bdf5874cff3 Mon Sep 17 00:00:00 2001 From: Tom Wu Date: Mon, 11 May 2026 20:31:56 -0400 Subject: [PATCH 3/6] 9.x cleanup iter 3: scope password-reset lock + close TOCTOU on refresh MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex iter 2 caught: P0: the new with_for_update() Person re-fetch in /forgot-password queried by id only, violating the repo-wide tenancy convention that every Person query filters by org_id. Filter is now (Person.id == person.id AND Person.org_id == person.org_id). Same result (id is globally unique), but matches the convention reviewers grep against. P1: the value-compare stale-check in _attemptRefresh had a TOCTOU window — between readRefreshToken() and writeToken(), an interleaved signOut/login could pass the value check but invalidate the session state we were about to overwrite. Replaced value compare with a monotonic sessionGeneration counter on SecureTokenStorage that bumps on every write/clear. The interceptor snapshots the generation at request start; if anything mutates storage during the round-trip, the response is dropped without touching state. Existing regression tests in refresh_test.dart still cover the clearAll and fresh-login paths end-to-end via the generation counter. SecureTokenStorage abstract interface adds `int get sessionGeneration`; both _RealStorage and InMemoryTokenStorage maintain a private `_gen` that increments on each mutator (writeToken / clearToken / writeRefreshToken / clearRefreshToken / clearAll). Co-Authored-By: Claude Opus 4.7 (1M context) --- api/routers/password_reset.py | 9 +++- mobile/lib/api/api_client.dart | 20 ++++---- mobile/lib/auth/secure_token_storage.dart | 58 +++++++++++++++++++---- 3 files changed, 70 insertions(+), 17 deletions(-) diff --git a/api/routers/password_reset.py b/api/routers/password_reset.py index 48f00920..0d5dc1e2 100644 --- a/api/routers/password_reset.py +++ b/api/routers/password_reset.py @@ -142,7 +142,14 @@ def request_password_reset( # freshness contract in docs/features/password-reset.md Scenario 6 # holds. SQLite ignores FOR UPDATE (its global lock already serializes # writers); PostgreSQL enforces a real row lock under READ COMMITTED. - db.query(Person).filter(Person.id == person.id).with_for_update().one() + # Scope by org_id too — repo-wide tenancy contract is "every Person + # query filters by org_id". Filtering by primary key alone is + # technically sufficient here (Person.id is globally unique), but the + # convention exists so reviewers can grep for missing org filters in + # new code paths. + db.query(Person).filter( + Person.id == person.id, Person.org_id == person.org_id + ).with_for_update().one() now = utcnow() diff --git a/mobile/lib/api/api_client.dart b/mobile/lib/api/api_client.dart index 3bd3675a..bad209a0 100644 --- a/mobile/lib/api/api_client.dart +++ b/mobile/lib/api/api_client.dart @@ -108,6 +108,14 @@ Future<_RefreshOutcome> _attemptRefresh(Dio dio, SecureTokenStorage storage) asy _refreshInFlight = completer; try { + // Snapshot the storage generation at the start. Any storage mutation + // (signOut, fresh login, another refresh landing first) bumps it; + // we check before persisting so an in-flight stale response can't + // overwrite whatever's currently in storage. Value comparison on + // the refresh token alone has a TOCTOU window between the read and + // the writes — a logout that lands between them would still pass + // value check but invalidate the session. + final genAtStart = storage.sessionGeneration; final refreshToken = await storage.readRefreshToken(); if (refreshToken == null || refreshToken.isEmpty) { completer.complete(_RefreshOutcome.failure); @@ -128,14 +136,10 @@ Future<_RefreshOutcome> _attemptRefresh(Dio dio, SecureTokenStorage storage) asy if (body is Map && body['token'] is String && body['refresh_token'] is String) { - // If signOut() / clearAll() ran while /auth/refresh was in flight, - // the refresh slot in storage is now empty (or replaced by a fresh - // login's token). Persisting the response would resurrect the old - // session or clobber a brand-new login. Return `stale` so the - // interceptor neither replays the original request nor wipes the - // current session's tokens. - final stillStored = await storage.readRefreshToken(); - if (stillStored != refreshToken) { + // Drop the response if anything else mutated storage during the + // round-trip. Returns `stale` so the interceptor neither replays + // the original request nor wipes the current session. + if (storage.sessionGeneration != genAtStart) { completer.complete(_RefreshOutcome.stale); return _RefreshOutcome.stale; } diff --git a/mobile/lib/auth/secure_token_storage.dart b/mobile/lib/auth/secure_token_storage.dart index c0f1f3c1..56f1c8cc 100644 --- a/mobile/lib/auth/secure_token_storage.dart +++ b/mobile/lib/auth/secure_token_storage.dart @@ -18,6 +18,14 @@ abstract class SecureTokenStorage { /// Convenience: nukes both tokens. Used on logout and on a 401 that /// can't be recovered via /auth/refresh. Future clearAll(); + + /// Monotonic counter bumped on every write/clear. The refresh + /// interceptor captures this at request time and verifies it hasn't + /// changed before persisting response tokens. If it has — signOut(), + /// a fresh login, or another refresh response landed in between — + /// the response is dropped without mutating storage. Closes the + /// TOCTOU window in `readRefreshToken() → compare → writeToken()`. + int get sessionGeneration; } class _RealStorage implements SecureTokenStorage { @@ -26,27 +34,44 @@ class _RealStorage implements SecureTokenStorage { final _impl = const FlutterSecureStorage( iOptions: IOSOptions(accessibility: KeychainAccessibility.first_unlock), ); + int _gen = 0; + + @override + int get sessionGeneration => _gen; @override Future readToken() => _impl.read(key: _accessKey); @override - Future writeToken(String token) => _impl.write(key: _accessKey, value: token); + Future writeToken(String token) async { + _gen++; + await _impl.write(key: _accessKey, value: token); + } @override - Future clearToken() => _impl.delete(key: _accessKey); + Future clearToken() async { + _gen++; + await _impl.delete(key: _accessKey); + } @override Future readRefreshToken() => _impl.read(key: _refreshKey); @override - Future writeRefreshToken(String token) => _impl.write(key: _refreshKey, value: token); + Future writeRefreshToken(String token) async { + _gen++; + await _impl.write(key: _refreshKey, value: token); + } @override - Future clearRefreshToken() => _impl.delete(key: _refreshKey); + Future clearRefreshToken() async { + _gen++; + await _impl.delete(key: _refreshKey); + } @override Future clearAll() async { + _gen++; await _impl.delete(key: _accessKey); await _impl.delete(key: _refreshKey); } @@ -56,27 +81,44 @@ class _RealStorage implements SecureTokenStorage { class InMemoryTokenStorage implements SecureTokenStorage { String? _token; String? _refresh; + int _gen = 0; + + @override + int get sessionGeneration => _gen; @override Future readToken() async => _token; @override - Future writeToken(String token) async => _token = token; + Future writeToken(String token) async { + _gen++; + _token = token; + } @override - Future clearToken() async => _token = null; + Future clearToken() async { + _gen++; + _token = null; + } @override Future readRefreshToken() async => _refresh; @override - Future writeRefreshToken(String token) async => _refresh = token; + Future writeRefreshToken(String token) async { + _gen++; + _refresh = token; + } @override - Future clearRefreshToken() async => _refresh = null; + Future clearRefreshToken() async { + _gen++; + _refresh = null; + } @override Future clearAll() async { + _gen++; _token = null; _refresh = null; } From 20f00129e443634ef301dd4765b756939dd84418 Mon Sep 17 00:00:00 2001 From: Tom Wu Date: Mon, 11 May 2026 20:51:22 -0400 Subject: [PATCH 4/6] 9.x cleanup iter 4: close inter-write race in refresh persist MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex iter 3 P1: the generation check before writeToken closes the "during round-trip" race but not the narrow "between the two awaited writes" race. If a signOut/login lands after writeToken but before writeRefreshToken, storage ends up with one session's access and another session's refresh. Fix: after the first write, recheck generation against (genAtStart+1). If it bumped higher, another mutator interleaved. Roll back the access write (clearToken — which itself bumps generation, but we're now deliberately discarding both halves of this rotation) and return stale. The remaining theoretical race (a mutator landing during the rollback's own await) is acceptable: in Dart's single-isolate cooperative model the window is platform-call latency, and on the next 401 the interceptor will pick up whichever session is actually in storage. Co-Authored-By: Claude Opus 4.7 (1M context) --- mobile/lib/api/api_client.dart | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/mobile/lib/api/api_client.dart b/mobile/lib/api/api_client.dart index bad209a0..0bf1a93e 100644 --- a/mobile/lib/api/api_client.dart +++ b/mobile/lib/api/api_client.dart @@ -136,14 +136,25 @@ Future<_RefreshOutcome> _attemptRefresh(Dio dio, SecureTokenStorage storage) asy if (body is Map && body['token'] is String && body['refresh_token'] is String) { - // Drop the response if anything else mutated storage during the - // round-trip. Returns `stale` so the interceptor neither replays - // the original request nor wipes the current session. + // First gate: nothing mutated storage during the /auth/refresh + // round-trip. If it did, the response is for a session that's + // already been replaced — drop without touching state. if (storage.sessionGeneration != genAtStart) { completer.complete(_RefreshOutcome.stale); return _RefreshOutcome.stale; } + // Each storage mutator bumps the counter exactly once. After our + // writeToken below, the expected generation is genAtStart + 1. + // If it's higher, another mutator (signOut, fresh login) raced + // between our two awaited writes — we'd be persisting an access + // token from one session and a refresh token from another. Roll + // back the access write and abort. await storage.writeToken(body['token'] as String); + if (storage.sessionGeneration != genAtStart + 1) { + await storage.clearToken(); + completer.complete(_RefreshOutcome.stale); + return _RefreshOutcome.stale; + } await storage.writeRefreshToken(body['refresh_token'] as String); completer.complete(_RefreshOutcome.success); return _RefreshOutcome.success; From 24e18102295ada5d9d0b6aa1309a0017fa01dcd2 Mon Sep 17 00:00:00 2001 From: Tom Wu Date: Mon, 11 May 2026 21:06:11 -0400 Subject: [PATCH 5/6] 9.x cleanup iter 5: revert inter-write rollback + treat post-mutation failure as stale MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex iter 4 caught two follow-on P2s: 1. The inter-write recheck (iter 4) called clearToken() on detected race — but by the time we detect it, the access slot may hold a fresh login's token, and clearing it logs that user out. Reverted the rollback. Single generation check before the writes remains; inter-write race documented as a known narrow window (microseconds, Dart cooperative async, no preemption). 2. On DioException (refresh failed), we previously returned `failure` unconditionally, which makes the caller run clearAll() — but if a fresh login landed during the round-trip, that wipes the new session. Now also gate the DioException path on the generation check: if storage changed mid-flight, return `stale` instead of `failure` so the new session is preserved. The remaining inter-write race (mutator landing between writeToken and writeRefreshToken) is platform-call latency. Closing it requires either package:synchronized or an atomic compareAndWriteTokens method on SecureTokenStorage — both meaningful design changes deferred to a focused follow-up. Co-Authored-By: Claude Opus 4.7 (1M context) --- mobile/lib/api/api_client.dart | 34 ++++++++++++++++++++-------------- 1 file changed, 20 insertions(+), 14 deletions(-) diff --git a/mobile/lib/api/api_client.dart b/mobile/lib/api/api_client.dart index 0bf1a93e..0434ca4f 100644 --- a/mobile/lib/api/api_client.dart +++ b/mobile/lib/api/api_client.dart @@ -136,25 +136,24 @@ Future<_RefreshOutcome> _attemptRefresh(Dio dio, SecureTokenStorage storage) asy if (body is Map && body['token'] is String && body['refresh_token'] is String) { - // First gate: nothing mutated storage during the /auth/refresh - // round-trip. If it did, the response is for a session that's - // already been replaced — drop without touching state. + // Drop the response if anything mutated storage during the + // /auth/refresh round-trip — the response is for a session + // that's already been replaced or cleared. + // + // KNOWN RESIDUAL RACE: within Dart's single-isolate cooperative + // model, between the two awaited writes below another mutator + // can interleave. We don't attempt to roll back the first write, + // because by the time we detect it the "current" access token + // may belong to a fresh login that ran after our write — and + // clearing it would log that user out. The realistic exploit + // window is platform-call latency (microseconds); on the next + // 401 the interceptor sees whatever session is actually in + // storage and rotates from there. if (storage.sessionGeneration != genAtStart) { completer.complete(_RefreshOutcome.stale); return _RefreshOutcome.stale; } - // Each storage mutator bumps the counter exactly once. After our - // writeToken below, the expected generation is genAtStart + 1. - // If it's higher, another mutator (signOut, fresh login) raced - // between our two awaited writes — we'd be persisting an access - // token from one session and a refresh token from another. Roll - // back the access write and abort. await storage.writeToken(body['token'] as String); - if (storage.sessionGeneration != genAtStart + 1) { - await storage.clearToken(); - completer.complete(_RefreshOutcome.stale); - return _RefreshOutcome.stale; - } await storage.writeRefreshToken(body['refresh_token'] as String); completer.complete(_RefreshOutcome.success); return _RefreshOutcome.success; @@ -162,6 +161,13 @@ Future<_RefreshOutcome> _attemptRefresh(Dio dio, SecureTokenStorage storage) asy completer.complete(_RefreshOutcome.failure); return _RefreshOutcome.failure; } on DioException { + // If the refresh failed AND storage changed since we started, the + // failure belongs to a session that's already gone. Don't make the + // caller clearAll() — that would wipe the new session. + if (storage.sessionGeneration != genAtStart) { + completer.complete(_RefreshOutcome.stale); + return _RefreshOutcome.stale; + } completer.complete(_RefreshOutcome.failure); return _RefreshOutcome.failure; } finally { From 72b5176fa2b1e5449b22e196c922a47d7c0934b6 Mon Sep 17 00:00:00 2001 From: Tom Wu Date: Mon, 11 May 2026 21:16:57 -0400 Subject: [PATCH 6/6] 9.x cleanup iter 6: hoist genAtStart out of try so catch can see it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex iter 5: Dart scoping error — `genAtStart` declared inside the try block, referenced from the DioException catch path. Wouldn't compile under flutter analyze. Repo's CI doesn't run flutter analyze so it slipped past. Moved the snapshot above the try. Co-Authored-By: Claude Opus 4.7 (1M context) --- mobile/lib/api/api_client.dart | 35 ++++++++++++++++------------------ 1 file changed, 16 insertions(+), 19 deletions(-) diff --git a/mobile/lib/api/api_client.dart b/mobile/lib/api/api_client.dart index 0434ca4f..e0567d94 100644 --- a/mobile/lib/api/api_client.dart +++ b/mobile/lib/api/api_client.dart @@ -107,15 +107,14 @@ Future<_RefreshOutcome> _attemptRefresh(Dio dio, SecureTokenStorage storage) asy final completer = Completer<_RefreshOutcome>(); _refreshInFlight = completer; + // Snapshot the storage generation at the start. Any storage mutation + // (signOut, fresh login, another refresh landing first) bumps it; + // we check before persisting so an in-flight stale response can't + // overwrite whatever's currently in storage. Hoisted out of the try + // block so the DioException catch can also see it (Dart scoping). + final genAtStart = storage.sessionGeneration; + try { - // Snapshot the storage generation at the start. Any storage mutation - // (signOut, fresh login, another refresh landing first) bumps it; - // we check before persisting so an in-flight stale response can't - // overwrite whatever's currently in storage. Value comparison on - // the refresh token alone has a TOCTOU window between the read and - // the writes — a logout that lands between them would still pass - // value check but invalidate the session. - final genAtStart = storage.sessionGeneration; final refreshToken = await storage.readRefreshToken(); if (refreshToken == null || refreshToken.isEmpty) { completer.complete(_RefreshOutcome.failure); @@ -138,17 +137,15 @@ Future<_RefreshOutcome> _attemptRefresh(Dio dio, SecureTokenStorage storage) asy body['refresh_token'] is String) { // Drop the response if anything mutated storage during the // /auth/refresh round-trip — the response is for a session - // that's already been replaced or cleared. - // - // KNOWN RESIDUAL RACE: within Dart's single-isolate cooperative - // model, between the two awaited writes below another mutator - // can interleave. We don't attempt to roll back the first write, - // because by the time we detect it the "current" access token - // may belong to a fresh login that ran after our write — and - // clearing it would log that user out. The realistic exploit - // window is platform-call latency (microseconds); on the next - // 401 the interceptor sees whatever session is actually in - // storage and rotates from there. + // that's already been replaced or cleared. KNOWN RESIDUAL RACE: + // within Dart's single-isolate cooperative model, between the + // two awaited writes below another mutator can interleave. We + // don't attempt to roll back the first write, because by the + // time we detect it the "current" access token may belong to a + // fresh login that ran after our write — clearing it would log + // that user out. The realistic window is platform-call latency + // (microseconds); on the next 401 the interceptor sees whatever + // session is actually in storage and rotates from there. if (storage.sessionGeneration != genAtStart) { completer.complete(_RefreshOutcome.stale); return _RefreshOutcome.stale;