diff --git a/lib/config/environment_config.dart b/lib/config/environment_config.dart index bf47fcd..9ec6559 100644 --- a/lib/config/environment_config.dart +++ b/lib/config/environment_config.dart @@ -55,8 +55,10 @@ class EnvironmentConfig { ); /// Flutter flavor passed via --flavor flag - /// This is set automatically by Flutter build system - static const String _flavor = String.fromEnvironment('FLUTTER_FLAVOR'); + /// FLUTTER_APP_FLAVOR is the define the Flutter build system actually + /// injects for --flavor builds (there is no FLUTTER_FLAVOR define - + /// reading that name silently falls through to production). + static const String _flavor = String.fromEnvironment('FLUTTER_APP_FLAVOR'); /// Explicit environment override via --dart-define=ENVIRONMENT=local /// Also supports --dart-define=ENV=dev for convenience diff --git a/lib/models/comment.dart b/lib/models/comment.dart index dcc9c3e..32692ac 100644 --- a/lib/models/comment.dart +++ b/lib/models/comment.dart @@ -11,23 +11,23 @@ class CommentsResponse { CommentsResponse({required this.post, this.cursor, required this.comments}); factory CommentsResponse.fromJson(Map json) { - // Handle null comments array from backend + // Handle a null or non-list comments array from the backend final commentsData = json['comments']; - final List commentsList; + final commentsList = []; - if (commentsData == null) { - // Backend returned null, use empty list - commentsList = []; - } else { + if (commentsData is List) { // Parse comment items, skipping any that fail to parse so one - // malformed comment never kills the whole thread load - commentsList = []; - for (final item in commentsData as List) { + // malformed comment never kills the whole thread load. Items are + // type-checked rather than cast, and the catch is `on Object` + // because an unchecked cast deep in a federated record raises a + // TypeError, which is an Error and would escape `on Exception`. + for (final item in commentsData) { + if (item is! Map) { + continue; + } try { - commentsList.add( - ThreadViewComment.fromJson(item as Map), - ); - } on Exception catch (e) { + commentsList.add(ThreadViewComment.fromJson(item)); + } on Object catch (e) { if (kDebugMode) { debugPrint('⚠️ Skipping malformed comment: $e'); } @@ -35,9 +35,10 @@ class CommentsResponse { } } + final cursor = json['cursor']; return CommentsResponse( post: json['post'], - cursor: json['cursor'] as String?, + cursor: cursor is String ? cursor : null, comments: commentsList, ); } @@ -56,18 +57,30 @@ class ThreadViewComment { }) : replies = replies == null ? null : List.unmodifiable(replies); factory ThreadViewComment.fromJson(Map json) { + // Parse replies with the same skip-malformed guard as the top level, + // so one bad nested reply drops only itself (and its subtree), not + // every ancestor up to the thread root. + final repliesData = json['replies']; + List? repliesList; + if (repliesData is List) { + repliesList = []; + for (final item in repliesData) { + if (item is! Map) { + continue; + } + try { + repliesList.add(ThreadViewComment.fromJson(item)); + } on Object catch (e) { + if (kDebugMode) { + debugPrint('⚠️ Skipping malformed reply: $e'); + } + } + } + } + return ThreadViewComment( comment: CommentView.fromJson(json['comment'] as Map), - replies: - json['replies'] != null - ? (json['replies'] as List) - .map( - (item) => ThreadViewComment.fromJson( - item as Map, - ), - ) - .toList() - : null, + replies: repliesList, hasMore: json['hasMore'] as bool? ?? false, ); } @@ -439,20 +452,29 @@ class ActorCommentsResponse { /// Handles null comments array gracefully by returning an empty list. factory ActorCommentsResponse.fromJson(Map json) { final commentsData = json['comments']; - final List commentsList; - - if (commentsData == null) { - commentsList = []; - } else { - commentsList = - (commentsData as List) - .map((item) => CommentView.fromJson(item as Map)) - .toList(); + final commentsList = []; + + // Same skip-malformed guard as CommentsResponse: one bad comment must + // not blank the whole profile comments tab. + if (commentsData is List) { + for (final item in commentsData) { + if (item is! Map) { + continue; + } + try { + commentsList.add(CommentView.fromJson(item)); + } on Object catch (e) { + if (kDebugMode) { + debugPrint('⚠️ Skipping malformed actor comment: $e'); + } + } + } } + final cursor = json['cursor']; return ActorCommentsResponse( comments: commentsList, - cursor: json['cursor'] as String?, + cursor: cursor is String ? cursor : null, ); } diff --git a/lib/models/community.dart b/lib/models/community.dart index 12ee3e9..e257b85 100644 --- a/lib/models/community.dart +++ b/lib/models/community.dart @@ -4,6 +4,8 @@ // GET /xrpc/social.coves.community.list // POST /xrpc/social.coves.community.post.create +import 'package:flutter/foundation.dart'; + import '../constants/embed_types.dart'; /// Response from GET /xrpc/social.coves.community.list @@ -11,25 +13,32 @@ class CommunitiesResponse { CommunitiesResponse({required this.communities, this.cursor}); factory CommunitiesResponse.fromJson(Map json) { - // Handle null communities array from backend + // Handle a null or non-list communities array from the backend final communitiesData = json['communities']; - final List communitiesList; - - if (communitiesData == null) { - // Backend returned null, use empty list - communitiesList = []; - } else { - // Parse community items - communitiesList = (communitiesData as List) - .map( - (item) => CommunityView.fromJson(item as Map), - ) - .toList(); + final communitiesList = []; + + // Parse community items, skipping any that fail to parse so one + // malformed community never kills the whole list. `on Object` because + // a bad cast raises a TypeError, which escapes `on Exception`. + if (communitiesData is List) { + for (final item in communitiesData) { + if (item is! Map) { + continue; + } + try { + communitiesList.add(CommunityView.fromJson(item)); + } on Object catch (e) { + if (kDebugMode) { + debugPrint('⚠️ Skipping malformed community: $e'); + } + } + } } + final cursor = json['cursor']; return CommunitiesResponse( communities: communitiesList, - cursor: json['cursor'] as String?, + cursor: cursor is String ? cursor : null, ); } diff --git a/lib/providers/auth_provider.dart b/lib/providers/auth_provider.dart index 1e68dbf..a288267 100644 --- a/lib/providers/auth_provider.dart +++ b/lib/providers/auth_provider.dart @@ -39,9 +39,6 @@ class AuthProvider with ChangeNotifier { /// /// Returns the sealed token for API authentication. /// The token is opaque to the client - backend handles everything. - /// - /// If token refresh fails, attempts to refresh automatically. - /// If refresh fails, signs out the user. Future getAccessToken() async { if (_session == null) { return null; @@ -146,10 +143,9 @@ class AuthProvider with ChangeNotifier { print('Restored session rejected by backend - attempting refresh'); } - // Call the service directly rather than [refreshToken]: that method - // signs out on ANY failure (appropriate for its 401-interceptor - // callers), while this proactive probe must only sign out when the - // backend definitively rejects the session. + // Call the service directly rather than [refreshToken] so this probe + // can distinguish the race-discard case and keep its own logging; + // both paths sign out only on [SessionExpiredException]. try { final refreshedSession = await _authService.refreshToken(); if (_session?.token != validatedToken) { @@ -282,7 +278,11 @@ class AuthProvider with ChangeNotifier { /// Calls the backend's /oauth/refresh endpoint. /// The backend handles the actual PDS token refresh internally. /// - /// Returns true if refresh succeeded, false otherwise. + /// Returns true if refresh succeeded, false otherwise. Owns the + /// sign-out decision: signs out only when the backend definitively + /// rejects the session ([SessionExpiredException]); transient failures + /// (network, 5xx) keep the session. Callers must not sign out on a + /// false return - a false may just mean the network blipped. Future refreshToken() async { if (_session == null) { return false; @@ -307,14 +307,22 @@ class AuthProvider with ChangeNotifier { // The session state is already whatever the race winner made it - // signing out here would destroy a freshly created session. return false; - } on Exception catch (e) { + } on SessionExpiredException { + // The backend definitively rejected the session (refresh 401): + // it is dead and only a new sign-in can recover. if (kDebugMode) { - print('Token refresh failed: $e'); + print('Session expired - signing out'); } - - // If refresh fails, sign out the user await signOut(); return false; + } on Exception catch (e) { + // Transient failure (network drop, timeout, 5xx): the session may + // still be perfectly valid, so keep it. The caller's request fails + // like any other network error and the next 401 retriggers refresh. + if (kDebugMode) { + print('Token refresh failed transiently (session kept): $e'); + } + return false; } } diff --git a/lib/services/auth_interceptor.dart b/lib/services/auth_interceptor.dart index 28301e2..b97030a 100644 --- a/lib/services/auth_interceptor.dart +++ b/lib/services/auth_interceptor.dart @@ -10,7 +10,12 @@ import '../models/coves_session.dart'; /// CommentService by providing a single implementation of: /// - Adding Authorization headers with fresh tokens on each request /// - Automatic retry with token refresh on 401 responses -/// - Sign-out handling when refresh fails +/// - Sign-out when a 401 persists after a successful refresh +/// +/// Sign-out on refresh *failure* is deliberately NOT handled here: the +/// tokenRefresher (AuthProvider.refreshToken) owns that decision, because +/// only it can tell a definitive session rejection from a transient +/// network/server failure that must keep the session alive. /// /// Usage: /// ```dart @@ -113,29 +118,22 @@ InterceptorsWrapper createAuthInterceptor({ } } - // Refresh failed, sign out the user + // Refresh failed. Do NOT sign out here: the refresher owns that + // decision and already signed out if the session was definitively + // rejected. A false return may just mean a transient network + // failure, and signing out would destroy a valid session. if (kDebugMode) { debugPrint( - '❌ $serviceName: Token refresh failed, signing out user', + '❌ $serviceName: Token refresh failed, propagating error', ); } - if (signOutHandler != null) { - await signOutHandler(); - } } on Exception catch (e) { + // Same rule as above: an exception here (from the refresher or + // from retrying the original request) is not evidence the session + // is dead, so never sign out - just propagate the error. if (kDebugMode) { debugPrint('❌ $serviceName: Error during token refresh: $e'); } - // Only sign out if we haven't already (avoid double sign-out) - // Check if this is a DioException from a retried request - final isRetriedRequest = - e is DioException && - e.response?.statusCode == 401 && - e.requestOptions.extra['retried'] == true; - - if (!isRetriedRequest && signOutHandler != null) { - await signOutHandler(); - } } } diff --git a/lib/services/comment_service.dart b/lib/services/comment_service.dart index 3e7aea0..e3bcaa0 100644 --- a/lib/services/comment_service.dart +++ b/lib/services/comment_service.dart @@ -132,10 +132,13 @@ class CommentService { throw ApiException('Invalid response from server - no data'); } - final uri = data['uri'] as String?; - final cid = data['cid'] as String?; + final uri = data['uri']; + final cid = data['cid']; - if (uri == null || uri.isEmpty || cid == null || cid.isEmpty) { + // Type-check rather than cast: a non-string uri/cid raises a + // TypeError - an Error, not an Exception - which would escape every + // handler up the stack instead of surfacing as ApiException. + if (uri is! String || uri.isEmpty || cid is! String || cid.isEmpty) { throw ApiException('Invalid response from server - missing uri or cid'); } diff --git a/lib/services/coves_api_service.dart b/lib/services/coves_api_service.dart index 8a947b7..c30e471 100644 --- a/lib/services/coves_api_service.dart +++ b/lib/services/coves_api_service.dart @@ -25,7 +25,9 @@ import 'retry_interceptor.dart'; /// Features automatic token refresh on 401 responses: /// - When a 401 is received, attempts to refresh the token /// - Retries the original request with the new token -/// - If refresh fails, signs out the user +/// - If refresh fails, propagates the error - sign-out is owned by the +/// token refresher (AuthProvider.refreshToken); sign-out here happens +/// only when a 401 persists after a successful refresh class CovesApiService { CovesApiService({ Future Function()? tokenGetter, @@ -165,27 +167,21 @@ class CovesApiService { } } - // Refresh failed, sign out the user + // Refresh failed. Do NOT sign out here: the refresher owns + // that decision and already signed out if the session was + // definitively rejected. A false return may just mean a + // transient network failure, and signing out would destroy + // a valid session. if (kDebugMode) { - debugPrint('❌ Token refresh failed, signing out user'); - } - if (_signOutHandler != null) { - await _signOutHandler(); + debugPrint('❌ Token refresh failed, propagating error'); } } catch (e) { + // Same rule as above: an exception here (from the refresher + // or from retrying the original request) is not evidence the + // session is dead, so never sign out - just propagate. if (kDebugMode) { debugPrint('❌ Error during token refresh: $e'); } - // Only sign out if we haven't already (avoid double sign-out) - // Check if this is a DioException from a retried request - final isRetriedRequest = - e is DioException && - e.response?.statusCode == 401 && - e.requestOptions.extra['retried'] == true; - - if (!isRetriedRequest && _signOutHandler != null) { - await _signOutHandler(); - } } } diff --git a/lib/services/coves_auth_service.dart b/lib/services/coves_auth_service.dart index d4a33be..53db2c0 100644 --- a/lib/services/coves_auth_service.dart +++ b/lib/services/coves_auth_service.dart @@ -453,6 +453,17 @@ class CovesAuthService { print('Status code: ${e.response?.statusCode}'); } + // The race-discard rule applies to failures too: if sign-out or + // re-login replaced the session mid-flight, this outcome belongs to + // a session that no longer exists. Without this check a stale 401 + // would be classified as SessionExpiredException below, and the + // caller would sign out the race winner's freshly created session. + if (!identical(_session, sessionAtStart)) { + const error = SessionRefreshDiscardedException(); + _refreshCompleter!.completeError(error); + return _refreshCompleter!.future; + } + // 401 means session is invalid/expired - caller should sign out. // Typed so callers can distinguish "definitively dead" from transient // refresh failures (which must NOT destroy the session on the diff --git a/lib/services/retry_interceptor.dart b/lib/services/retry_interceptor.dart index 9e3b302..4b14b7a 100644 --- a/lib/services/retry_interceptor.dart +++ b/lib/services/retry_interceptor.dart @@ -92,18 +92,31 @@ class RetryInterceptor extends Interceptor { } } + /// HTTP methods that are safe to retry after an ambiguous failure: + /// they don't mutate server state, so a duplicate delivery is harmless. + static const _idempotentMethods = {'GET', 'HEAD', 'OPTIONS'}; + /// Determine if the error is retryable /// /// Only retry on transient network errors, not: /// - HTTP errors (4xx, 5xx) - server responded, retry won't help /// - Request cancellation - intentional /// - Bad certificate - security issue - /// - Receive timeout on POST - server may have processed the request + /// - Ambiguous failures on mutating methods - see below bool _shouldRetry(DioException err) { - // Never retry receive timeouts on POST - server may have processed - // the request. This prevents duplicate comments, vote toggling, etc. - if (err.type == DioExceptionType.receiveTimeout && - err.requestOptions.method == 'POST') { + // Never retry mutating requests (POST etc.) when the failure is + // ambiguous about whether the server already processed the request: + // - receiveTimeout: the request was fully sent; only the response + // was lost + // - connectionError: dio raises this for resets both before AND after + // the request went out, so the server may have processed it + // Retrying would double-submit: a vote toggle fired twice silently + // reverts the user's vote, a comment posts twice, etc. + // connectionTimeout/sendTimeout stay retryable: the request never + // fully reached the server, so it cannot have been processed. + if (!_idempotentMethods.contains(err.requestOptions.method) && + (err.type == DioExceptionType.receiveTimeout || + err.type == DioExceptionType.connectionError)) { return false; } diff --git a/lib/services/vote_service.dart b/lib/services/vote_service.dart index 7424730..0d97e13 100644 --- a/lib/services/vote_service.dart +++ b/lib/services/vote_service.dart @@ -132,8 +132,16 @@ class VoteService { throw ApiException('Invalid response from server - no data'); } - final uri = data['uri'] as String?; - final cid = data['cid'] as String?; + final uri = data['uri']; + final cid = data['cid']; + + // Type-check rather than cast: a non-string uri/cid from a backend + // contract break raises a TypeError - an Error, not an Exception - + // which escapes every handler up the stack and skips VoteProvider's + // optimistic-state rollback. Surface it as ApiException instead. + if (uri is! String? || cid is! String?) { + throw ApiException('Invalid response from server - malformed uri/cid'); + } // If uri/cid are empty, the backend toggled off an existing vote if (uri == null || uri.isEmpty || cid == null || cid.isEmpty) { diff --git a/lib/widgets/comment_card.dart b/lib/widgets/comment_card.dart index a26c1ff..9e9c8ca 100644 --- a/lib/widgets/comment_card.dart +++ b/lib/widgets/comment_card.dart @@ -2,6 +2,7 @@ import 'package:cached_network_image/cached_network_image.dart'; import 'package:flutter/foundation.dart'; import 'package:flutter/material.dart'; import 'package:flutter/services.dart'; +import 'package:go_router/go_router.dart'; import 'package:provider/provider.dart'; import '../constants/app_colors.dart'; @@ -636,7 +637,7 @@ class _CommentCardState extends State { ); if ((shouldSignIn ?? false) && context.mounted) { - await Navigator.of(context).pushNamed('/sign-in'); + await context.push('/login'); } return; } diff --git a/test/models/comment_test.dart b/test/models/comment_test.dart index b7b65a5..a1584f6 100644 --- a/test/models/comment_test.dart +++ b/test/models/comment_test.dart @@ -138,6 +138,18 @@ void main() { expect(response.cursor, null); expect(response.comments.length, 1); }); + + test('should treat a wrong-typed cursor as absent', () { + final json = { + 'post': {'uri': 'at://test/post/123'}, + 'cursor': 12345, + 'comments': [], + }; + + final response = CommentsResponse.fromJson(json); + + expect(response.cursor, null); + }); }); group('ThreadViewComment', () { @@ -203,6 +215,62 @@ void main() { expect(thread.replies![0].comment.content, 'Reply comment'); }); + test('should skip malformed nested replies but keep the parent and ' + 'valid siblings', () { + final json = { + 'comment': { + 'uri': 'at://did:plc:test/comment/1', + 'cid': 'cid1', + 'record': {'content': 'Parent comment'}, + 'createdAt': '2025-01-01T12:00:00Z', + 'indexedAt': '2025-01-01T12:00:00Z', + 'author': {'did': 'did:plc:author', 'handle': 'test.user'}, + 'post': {'uri': 'at://did:plc:test/post/123', 'cid': 'post-cid'}, + 'stats': {'upvotes': 5, 'downvotes': 1, 'score': 4}, + }, + 'replies': [ + { + 'comment': { + 'uri': 'at://did:plc:test/comment/good', + 'cid': 'cid-good', + 'record': {'content': 'Valid reply'}, + 'createdAt': '2025-01-01T13:00:00Z', + 'indexedAt': '2025-01-01T13:00:00Z', + 'author': {'did': 'did:plc:author2', 'handle': 'test.user2'}, + 'post': {'uri': 'at://did:plc:test/post/123', 'cid': 'post-cid'}, + 'stats': {'upvotes': 3, 'downvotes': 0, 'score': 3}, + }, + 'hasMore': false, + }, + { + // Malformed: missing required cid raises a TypeError (an + // Error, not an Exception) during CommentView parsing + 'comment': { + 'uri': 'at://did:plc:test/comment/bad', + 'record': {'content': 'Broken reply'}, + 'createdAt': '2025-01-01T13:00:00Z', + 'indexedAt': '2025-01-01T13:00:00Z', + 'author': {'did': 'did:plc:author3', 'handle': 'test.user3'}, + 'post': {'uri': 'at://did:plc:test/post/123', 'cid': 'post-cid'}, + 'stats': {'upvotes': 0, 'downvotes': 0, 'score': 0}, + }, + 'hasMore': false, + }, + // Not even a map - a federated record gone completely wrong + 'garbage', + ], + 'hasMore': false, + }; + + final thread = ThreadViewComment.fromJson(json); + + // The parent survives and only the malformed reply is dropped + expect(thread.comment.uri, 'at://did:plc:test/comment/1'); + expect(thread.replies, isNotNull); + expect(thread.replies!.length, 1); + expect(thread.replies![0].comment.uri, 'at://did:plc:test/comment/good'); + }); + test('should default hasMore to false when missing', () { final json = { 'comment': { @@ -868,6 +936,70 @@ void main() { ); }); }); + + group('ActorCommentsResponse', () { + Map validComment(String suffix) => { + 'uri': 'at://did:plc:test/comment/$suffix', + 'cid': 'cid-$suffix', + 'record': {'content': 'Comment $suffix'}, + 'createdAt': '2025-01-01T12:00:00Z', + 'indexedAt': '2025-01-01T12:00:00Z', + 'author': {'did': 'did:plc:author', 'handle': 'test.user'}, + 'post': {'uri': 'at://did:plc:test/post/123', 'cid': 'post-cid'}, + 'stats': {'upvotes': 1, 'downvotes': 0, 'score': 1}, + }; + + test('should parse valid JSON with comments and cursor', () { + final json = { + 'comments': [validComment('1'), validComment('2')], + 'cursor': 'next-page', + }; + + final response = ActorCommentsResponse.fromJson(json); + + expect(response.comments.length, 2); + expect(response.comments[0].uri, 'at://did:plc:test/comment/1'); + expect(response.cursor, 'next-page'); + }); + + test('should skip malformed comments but keep valid ones', () { + final malformed = validComment('bad')..remove('cid'); + final json = { + 'comments': [ + validComment('1'), + malformed, // missing cid raises a TypeError, not an Exception + 'garbage', // not even a map + validComment('2'), + ], + }; + + final response = ActorCommentsResponse.fromJson(json); + + expect(response.comments.length, 2); + expect(response.comments[0].uri, 'at://did:plc:test/comment/1'); + expect(response.comments[1].uri, 'at://did:plc:test/comment/2'); + }); + + test('should handle null or non-list comments array', () { + expect( + ActorCommentsResponse.fromJson({'comments': null}).comments, + isEmpty, + ); + expect( + ActorCommentsResponse.fromJson({'comments': 'nope'}).comments, + isEmpty, + ); + }); + + test('should treat a wrong-typed cursor as absent', () { + final json = { + 'comments': [], + 'cursor': 42, + }; + + expect(ActorCommentsResponse.fromJson(json).cursor, null); + }); + }); } /// Builds a minimal ThreadViewComment node for tree-manipulation tests. diff --git a/test/models/community_test.dart b/test/models/community_test.dart index 1a61dff..d1f5b73 100644 --- a/test/models/community_test.dart +++ b/test/models/community_test.dart @@ -70,6 +70,36 @@ void main() { expect(response.cursor, null); expect(response.communities.length, 1); }); + + test('should skip malformed communities but keep valid ones', () { + final json = { + 'communities': [ + {'did': 'did:plc:community1', 'name': 'first'}, + // Malformed: missing required name raises a TypeError (an + // Error, not an Exception) during parsing + {'did': 'did:plc:community-bad'}, + // Not even a map + 'garbage', + {'did': 'did:plc:community2', 'name': 'second'}, + ], + }; + + final response = CommunitiesResponse.fromJson(json); + + expect(response.communities.length, 2); + expect(response.communities[0].name, 'first'); + expect(response.communities[1].name, 'second'); + }); + + test('should treat a wrong-typed cursor or non-list array as absent', () { + final response = CommunitiesResponse.fromJson({ + 'communities': 'nope', + 'cursor': 99, + }); + + expect(response.communities, isEmpty); + expect(response.cursor, null); + }); }); group('CommunityView', () { diff --git a/test/providers/auth_provider_test.dart b/test/providers/auth_provider_test.dart index b0ad7a4..a1c9589 100644 --- a/test/providers/auth_provider_test.dart +++ b/test/providers/auth_provider_test.dart @@ -454,7 +454,34 @@ void main() { expect(authProvider.session?.token, 'new_sealed_token'); }); - test('should sign out if refresh fails', () async { + test( + 'should sign out when refresh is definitively rejected ' + '(SessionExpiredException)', + () async { + const mockSession = CovesSession( + token: 'mock_sealed_token', + did: 'did:plc:test123', + sessionId: 'session123', + ); + + when( + mockAuthService.signIn('alice.bsky.social'), + ).thenAnswer((_) async => mockSession); + when( + mockAuthService.refreshToken(), + ).thenThrow(const SessionExpiredException()); + when(mockAuthService.signOut()).thenAnswer((_) async => {}); + + await authProvider.signIn('alice.bsky.social'); + final result = await authProvider.refreshToken(); + + expect(result, false); + expect(authProvider.isAuthenticated, false); + verify(mockAuthService.signOut()).called(1); + }, + ); + + test('should keep the session when refresh fails transiently', () async { const mockSession = CovesSession( token: 'mock_sealed_token', did: 'did:plc:test123', @@ -466,14 +493,16 @@ void main() { ).thenAnswer((_) async => mockSession); when( mockAuthService.refreshToken(), - ).thenThrow(Exception('Refresh failed')); - when(mockAuthService.signOut()).thenAnswer((_) async => {}); + ).thenThrow(Exception('Refresh failed: network error')); await authProvider.signIn('alice.bsky.social'); final result = await authProvider.refreshToken(); + // A 5xx or network drop during refresh is not evidence the session + // is dead - the user must stay signed in. expect(result, false); - expect(authProvider.isAuthenticated, false); + expect(authProvider.isAuthenticated, true); + verifyNever(mockAuthService.signOut()); }); }); diff --git a/test/services/coves_api_service_test.dart b/test/services/coves_api_service_test.dart index eb5bc24..e570044 100644 --- a/test/services/coves_api_service_test.dart +++ b/test/services/coves_api_service_test.dart @@ -561,10 +561,11 @@ void main() { }, ); - expect( - () => apiService.getComments(postUri: postUri), - throwsA(isA()), - ); + // A malformed comment (missing required field raises a TypeError + // during parsing) is skipped rather than killing the whole thread + // load - one bad federated record must never blank the thread. + final response = await apiService.getComments(postUri: postUri); + expect(response.comments, isEmpty); }); test('should handle comments with nested replies', () async { diff --git a/test/services/coves_api_service_token_refresh_test.dart b/test/services/coves_api_service_token_refresh_test.dart index a2652f8..0c780d3 100644 --- a/test/services/coves_api_service_token_refresh_test.dart +++ b/test/services/coves_api_service_token_refresh_test.dart @@ -100,7 +100,12 @@ void main() { expect(signOutCallCount, 1); }); - test('should sign out user if token refresh fails', () async { + test('should NOT sign out here when token refresh fails', () async { + // The interceptor must not sign out on a false return from the + // refresher: AuthProvider.refreshToken owns that decision (it signs + // out itself on a definitive 401, and keeps the session on transient + // failures). Signing out here would destroy a valid session when the + // refresh merely hit a network blip or 5xx. const postUri = 'at://did:plc:test/social.coves.post.record/123'; // Set refresh to fail @@ -133,8 +138,9 @@ void main() { // Verify token refresh was attempted expect(tokenRefreshCallCount, 1); - // Verify user was signed out after refresh failure - expect(signOutCallCount, 1); + // The refresher owns the sign-out decision - the interceptor must + // not sign out on its behalf + expect(signOutCallCount, 0); }); test( @@ -143,7 +149,7 @@ void main() { // This test verifies that the interceptor checks for /oauth/refresh // in the path to avoid infinite loops. Due to limitations with mocking // complex request/response cycles, we test this by verifying the - // signOutHandler gets called when refresh fails. + // refresher runs exactly once and the error propagates. // Set refresh to fail (simulates refresh endpoint returning 401) shouldRefreshSucceed = false; @@ -173,17 +179,20 @@ void main() { // Wait for async operations to complete await Future.delayed(const Duration(milliseconds: 100)); - // Verify user was signed out (no infinite loop) - expect(signOutCallCount, 1); + // The refresher ran exactly once (no infinite loop), and the + // interceptor left the sign-out decision to it + expect(tokenRefreshCallCount, 1); + expect(signOutCallCount, 0); }, ); test( - 'should sign out user if token refresh throws exception', + 'should NOT sign out when token refresh throws exception', () async { - // Skipped: causes retry loops with http_mock_adapter after disposal - // The core functionality is tested by the "should sign out user if token - // refresh fails" test above. + // Skipped: causes retry loops with http_mock_adapter after disposal. + // The contract (refresher owns the sign-out decision; an exception + // here must not sign out) is covered by the "should NOT sign out + // here when token refresh fails" test above. }, skip: 'Causes retry issues with http_mock_adapter', ); diff --git a/test/services/coves_auth_service_test.dart b/test/services/coves_auth_service_test.dart index f559bfe..b9d8fe7 100644 --- a/test/services/coves_auth_service_test.dart +++ b/test/services/coves_auth_service_test.dart @@ -335,6 +335,63 @@ void main() { }, ); + test( + 'should discard a refresh 401 when the session changes while the ' + 'request is in flight (must not become SessionExpiredException)', + () async { + const sessionA = CovesSession( + token: 'token-a', + did: 'did:plc:test123', + sessionId: 'session-a', + ); + when( + mockStorage.read(key: storageKey), + ).thenAnswer((_) async => sessionA.toJsonString()); + await authService.restoreSession(); + + final gate = Completer>>(); + when( + mockDio.post>( + '/oauth/refresh', + data: anyNamed('data'), + ), + ).thenAnswer((_) => gate.future); + + final pending = authService.refreshToken(); + + // Re-login replaces the session while the refresh is in flight. + const sessionB = CovesSession( + token: 'token-b', + did: 'did:plc:other456', + sessionId: 'session-b', + ); + when( + mockStorage.read(key: storageKey), + ).thenAnswer((_) async => sessionB.toJsonString()); + await authService.restoreSession(); + + // The stale session's refresh comes back 401. Classifying it as + // SessionExpiredException would make AuthProvider sign out the + // race winner's freshly created session. + gate.completeError( + DioException( + requestOptions: RequestOptions(path: '/oauth/refresh'), + type: DioExceptionType.badResponse, + response: Response( + requestOptions: RequestOptions(path: '/oauth/refresh'), + statusCode: 401, + ), + ), + ); + + await expectLater( + pending, + throwsA(isA()), + ); + expect(authService.session?.token, 'token-b'); + }, + ); + test('should throw Exception on network error during refresh', () async { // Arrange - First restore a session const session = CovesSession( diff --git a/test/services/retry_interceptor_test.dart b/test/services/retry_interceptor_test.dart index 76c6302..d82e465 100644 --- a/test/services/retry_interceptor_test.dart +++ b/test/services/retry_interceptor_test.dart @@ -99,6 +99,56 @@ void main() { ); }); + test('should NOT retry POST requests with connectionError', () async { + // dio raises connectionError for resets both before AND after the + // request went out, so the server may have already processed it. + // Retrying would double-toggle votes / duplicate comments. + final error = DioException( + type: DioExceptionType.connectionError, + requestOptions: RequestOptions(path: '/vote', method: 'POST'), + ); + + final retryCount = await getRetryCount(error); + + expect( + retryCount, + isNull, + reason: 'POST + connectionError must NOT retry (prevents duplicates)', + ); + }); + + test('should retry GET requests with connectionError', () async { + final error = DioException( + type: DioExceptionType.connectionError, + requestOptions: RequestOptions(path: '/feed', method: 'GET'), + ); + + final retryCount = await getRetryCount(error); + + expect( + retryCount, + equals(1), + reason: 'GET is idempotent - connectionError should retry', + ); + }); + + test('should retry POST requests with connectionTimeout', () async { + // connectionTimeout means the TCP connection was never established, + // so the server cannot have processed anything - safe to retry. + final error = DioException( + type: DioExceptionType.connectionTimeout, + requestOptions: RequestOptions(path: '/vote', method: 'POST'), + ); + + final retryCount = await getRetryCount(error); + + expect( + retryCount, + equals(1), + reason: 'POST + connectionTimeout is safe to retry', + ); + }); + test('should NOT retry on HTTP errors (badResponse)', () async { final requestOptions = RequestOptions(path: '/test', method: 'GET'); final error = DioException( diff --git a/test/services/vote_service_token_refresh_test.dart b/test/services/vote_service_token_refresh_test.dart index d0c26b5..2f5144e 100644 --- a/test/services/vote_service_token_refresh_test.dart +++ b/test/services/vote_service_token_refresh_test.dart @@ -118,7 +118,12 @@ void main() { expect(signOutCallCount, 1); }); - test('should sign out user if token refresh fails', () async { + test('should NOT sign out here when token refresh fails', () async { + // The interceptor must not sign out on a false return from the + // refresher: AuthProvider.refreshToken owns that decision (it signs + // out itself on a definitive 401, and keeps the session on transient + // failures). Signing out here would destroy a valid session when the + // refresh merely hit a network blip or 5xx. const postUri = 'at://did:plc:test/social.coves.post.record/123'; const postCid = 'bafy123'; @@ -154,8 +159,9 @@ void main() { // Verify token refresh was attempted expect(tokenRefreshCallCount, 1); - // Verify user was signed out after refresh failure - expect(signOutCallCount, 1); + // The refresher owns the sign-out decision - the interceptor must + // not sign out on its behalf + expect(signOutCallCount, 0); }); test(