Merged
Conversation
zew-rgn
added a commit
that referenced
this pull request
Feb 3, 2026
Addresses Issue #2 from code review - Credentials/API keys logged. Changes: - Replace truncated API key logging with "[REDACTED, length=N]" - Add _redactUri() helper to deeplink_service.dart - Redact sensitive params (api_key, token, etc) from URI logs - Remove URL logging in rest_image_upload_service (contains api_key) Files changed: - lib/features/splash/presentation/screens/splash_screen.dart - lib/features/auth/presentation/providers/auth_notifier.dart - lib/features/auth/presentation/screens/auth_screen.dart - lib/core/services/deeplink_service.dart - lib/features/devices/data/services/rest_image_upload_service.dart Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
5 tasks
zew-rgn
added a commit
that referenced
this pull request
Feb 3, 2026
…nd bug fixes (#47) * fix(security): implement code review council fixes (phases 1-10) Addresses concerns from code review council (issue #46): Phase 1 - Debug route gated behind production check Phase 2 - Rooms provider invalidation on sign-out to prevent state leak Phase 3 - Remove keepAlive from family providers to prevent memory leaks Phase 4 - Break circular dependency between provider files Phase 5 - Replace mock data with real device status in room statistics Phase 6 - Add error logging instead of silent error swallowing Phase 7 - Show error UI for SharedPreferences initialization failure Phase 8 - Document async onDispose behavior with unawaited() Phase 9 - Enable riverpod_lint and custom_lint Phase 10 - Move stream subscription after auth check Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix(providers): address Codex review feedback - Remove re-export from websocket_providers.dart to break circular dependency - Add direct import of websocket_sync_providers in repository_providers.dart - Add custom_lint plugin to analysis_options.yaml for riverpod_lint - Sort dev_dependencies alphabetically in pubspec.yaml Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> Co-Authored-By: OpenAI Codex <noreply@openai.com> * fix(imports): add websocket_sync_providers imports to all consumers After removing the re-export from websocket_providers.dart, all files that use webSocketCacheIntegrationProvider, webSocketDataSyncServiceProvider, or webSocketDeviceLastUpdateProvider need to directly import websocket_sync_providers.dart. Updated files: - main.dart - initialization_provider.dart - settings_screen.dart - speed_test_providers.dart - image_upload_provider.dart - settings_riverpod_provider.dart - device_onboarding_provider.dart - health_notices_provider.dart - auth_notifier.dart - speed_test_card.dart - room_speed_test_selector.dart - device_speed_test_section.dart - network_overview_section.dart Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * docs: update plan with Phase 9 details Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix(security): migrate credentials to secure storage Addresses Issue #1 from code review - Insecure Credential Storage. Changes: - Add flutter_secure_storage package for platform-native encryption - Create SecureStorageService with iOS Keychain and Android Keystore - Migrate sensitive data (token, sessionToken, authSignature) to secure storage - Keep non-sensitive settings (theme, filters) in SharedPreferences - Add atomic migration: read -> write secure -> verify -> delete plaintext - Add migration flag to prevent re-migration - Update all callers to use async getToken() instead of sync getter - Handle FutureProvider properly for apiKeyProvider Security improvements: - iOS: Uses Keychain with afterFirstUnlock accessibility - Android: Uses EncryptedSharedPreferences backed by Keystore - Migration verifies writes before deleting plaintext - Failed migration allows retry on next launch See: #46 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix(security): address Codex review feedback on secure storage Fixes from Codex review: 1. [High] Fix authSignature async access - auth_notifier.dart:157 now uses `await storage.getAuthSignature()` - Previously used deprecated sync getter that returned null 2. [Medium] Add verification to ATT legacy migration - Verify secure storage write before deleting legacy keys - Prevents credential loss if write fails 3. [High] Add migration for legacy api_url/api_token keys - Previously just deleted without migrating to secure storage - Now migrates token to secure storage before deletion - Verifies write before removing legacy keys Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix(security): remove credential logging (Issue #2) Addresses Issue #2 from code review - Credentials/API keys logged. Changes: - Replace truncated API key logging with "[REDACTED, length=N]" - Add _redactUri() helper to deeplink_service.dart - Redact sensitive params (api_key, token, etc) from URI logs - Remove URL logging in rest_image_upload_service (contains api_key) Files changed: - lib/features/splash/presentation/screens/splash_screen.dart - lib/features/auth/presentation/providers/auth_notifier.dart - lib/features/auth/presentation/screens/auth_screen.dart - lib/core/services/deeplink_service.dart - lib/features/devices/data/services/rest_image_upload_service.dart Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix(memory): remove keepAlive from family providers (Issue #3) Addresses Issue #3 from code review - keepAlive on family providers. Family providers with keepAlive: true create persistent instances for every unique parameter, causing unbounded memory growth. Changed from @riverpod(keepAlive: true) to @riverpod: - SpeedTestResultsNotifier (speed_test_providers.dart:98) - Parameters: speedTestId, accessPointId - RoomDeviceNotifier (room_device_view_model.dart:43) - Parameters: roomId Now auto-disposes when no longer watched, preventing memory leaks. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix(logging): replace print() with LoggerService (Issue #4) Addresses Issue #4 from code review - print() statements in production. print() calls bypass LoggerService and don't log in release builds. Replaced all print() statements with LoggerService calls or removed redundant debug logging. Files changed: - lib/features/rooms/presentation/providers/room_device_view_model.dart - Removed verbose device filtering debug logs - lib/features/devices/data/repositories/device_repository.dart - Removed redundant print() (already had logger.i()) - lib/features/issues/presentation/providers/health_notices_provider.dart - Replaced print() with LoggerService.debug() - lib/features/issues/data/datasources/health_notices_remote_data_source.dart - Rewrote with proper LoggerService calls Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix(auth): clear room cache on sign-out (Issue #6) Added roomLocalDataSourceProvider cache clearing to the sign-out cleanup to ensure room data from previous user doesn't persist after logout. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix(config): address medium-priority review issues - Pin flutter_cache_manager version to ^3.4.1 (Phase 11) - Use useSyntheticData flag instead of isDevelopment for mock data (Phase 12) - Remove analysis exclusions for mock data files (Phase 13) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix(cache): add periodic cleanup and max size limit (Issue #9) CacheManager now: - Runs periodic cleanup every 5 minutes to remove expired entries - Enforces max 100 entries with LRU eviction when exceeded - Properly disposes timer when provider is disposed Prevents unbounded memory growth from cache accumulation. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * chore: remove unused imports Clean up unused imports identified by flutter analyze. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix(errors): add logging/comments to empty catch blocks (Issue #7) - Add debug logging to websocket_data_sync_service when date parsing fails - Add explanatory comments to defensive catch blocks in room_readiness_data_source The defensive catches handle dynamic type access where objects may not have expected properties - this is intentional behavior, now documented. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix(websocket): guard send() calls against connection race condition Add try-catch around WebSocket send() calls to handle race condition where connection can close between isConnected check and send() call. Note: Catches StateError (triggers lint info) because WebSocketService throws StateError for "not connected" - a future refactor could change this to a regular Exception. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix(websocket): add callback removal methods to prevent memory leaks Add removeXxxCallback() methods to WebSocketCacheIntegration for: - deviceDataCallbacks - roomDataCallbacks - speedTestConfigCallbacks - speedTestResultCallbacks Previously callbacks could only be added (append-only), potentially causing memory issues if added repeatedly without cleanup. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * chore: remove unused code elements - Remove unused _reconstructDeviceId function from image_upload_provider - Remove unused _logger getter from device_onboarding_provider - Remove unused logger import Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * chore: remove unused and duplicate imports - Remove unused shared_preferences import from device_onboarding_provider - Remove unused websocket_providers import from settings_screen - Remove unused websocket_providers import from main.dart - Remove duplicate logger_service imports from main_development.dart - Remove duplicate logger_service imports from main_production.dart Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix(devices): use REST API for image deletion instead of WebSocket The WebSocket update_resource action silently ignores the images parameter, so delete requests never reached the backend. Switch to HTTP PUT via RestImageUploadService, matching ATT-FE-Tool's approach. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com> Co-authored-by: OpenAI Codex <noreply@openai.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds websocket login