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..0d5dc1e2 100644 --- a/api/routers/password_reset.py +++ b/api/routers/password_reset.py @@ -131,6 +131,26 @@ 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. + # 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() # Invalidate any prior unused reset tokens for this person before 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..e0567d94 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,31 @@ 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; + // 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 { 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', @@ -113,16 +135,38 @@ Future _attemptRefresh(Dio dio, SecureTokenStorage storage) async { if (body is Map && body['token'] is String && 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 — 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; + } 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; + // 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 { _refreshInFlight = null; } 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/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; } diff --git a/mobile/test/refresh_test.dart b/mobile/test/refresh_test.dart index 98febb53..035ac479 100644 --- a/mobile/test/refresh_test.dart +++ b/mobile/test/refresh_test.dart @@ -130,6 +130,89 @@ 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('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 b68f012c..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: @@ -33,6 +33,26 @@ def main() -> int: args = parser.parse_args() 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")) + 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(