Add persistent storage for ignored phishing sites #17463
Conversation
|
New Issues (1)Checkmarx found the following issues in this Pull Request
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17463 +/- ##
==========================================
- Coverage 41.11% 41.11% -0.01%
==========================================
Files 3544 3543 -1
Lines 101753 101859 +106
Branches 15241 15265 +24
==========================================
+ Hits 41836 41878 +42
- Misses 58157 58215 +58
- Partials 1760 1766 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Claude Code is working… I'll analyze this and get back to you. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // Load previously ignored hostnames from storage (synchronously to avoid race conditions) | ||
| const ignoredHostnamesState = globalStateProvider.get(IGNORED_PHISHING_HOSTNAMES_KEY); | ||
| firstValueFrom(ignoredHostnamesState.state$.pipe(map((hostnames) => hostnames ?? []))) | ||
| .then((initialHostnames) => { | ||
| this._ignoredHostnames = new Set(initialHostnames); | ||
| logService.debug( | ||
| `[PhishingDetectionService] Loaded ${initialHostnames.length} ignored hostnames from storage`, | ||
| ); | ||
| }) | ||
| .catch((error) => { | ||
| logService.error("[PhishingDetectionService] Failed to load ignored hostnames", error); | ||
| }); |
There was a problem hiding this comment.
❌ Race condition: The initial state load is not awaited, which means the service may start processing navigation events before _ignoredHostnames is populated from disk. This defeats the purpose of persistent storage.
If a user navigates to a previously-ignored phishing site immediately after browser startup, they'll see the warning again.
| // Load previously ignored hostnames from storage (synchronously to avoid race conditions) | |
| const ignoredHostnamesState = globalStateProvider.get(IGNORED_PHISHING_HOSTNAMES_KEY); | |
| firstValueFrom(ignoredHostnamesState.state$.pipe(map((hostnames) => hostnames ?? []))) | |
| .then((initialHostnames) => { | |
| this._ignoredHostnames = new Set(initialHostnames); | |
| logService.debug( | |
| `[PhishingDetectionService] Loaded ${initialHostnames.length} ignored hostnames from storage`, | |
| ); | |
| }) | |
| .catch((error) => { | |
| logService.error("[PhishingDetectionService] Failed to load ignored hostnames", error); | |
| }); | |
| // Load previously ignored hostnames from storage (await to avoid race conditions) | |
| const ignoredHostnamesState = globalStateProvider.get(IGNORED_PHISHING_HOSTNAMES_KEY); | |
| try { | |
| const initialHostnames = await firstValueFrom( | |
| ignoredHostnamesState.state$.pipe(map((hostnames) => hostnames ?? [])) | |
| ); | |
| this._ignoredHostnames = new Set(initialHostnames); | |
| logService.debug( | |
| `[PhishingDetectionService] Loaded ${initialHostnames.length} ignored hostnames from storage`, | |
| ); | |
| } catch (error) { | |
| logService.error("[PhishingDetectionService] Failed to load ignored hostnames", error); | |
| } |
| globalStateProvider, | ||
| ); | ||
| }).not.toThrow(); | ||
| }); |
There was a problem hiding this comment.
❌ Insufficient test coverage (30.43%): This test file only verifies that initialization doesn't throw. Critical functionality is completely untested:
- Loading ignored hostnames from storage on init
- Persisting hostnames when user clicks "Continue"
- Navigation to ignored sites (should skip phishing check)
- Error handling during storage operations
- Cleanup/unsubscribe behavior
Please add tests covering the main user flows and error scenarios.
| /** | ||
| * Key definition for storing hostnames that the user has chosen to ignore | ||
| */ | ||
| export const IGNORED_PHISHING_HOSTNAMES_KEY = new KeyDefinition<string[]>( | ||
| PHISHING_DETECTION_DISK, | ||
| "ignoredPhishingHostnames", | ||
| { | ||
| deserializer: (value: string[]) => value ?? [], | ||
| }, | ||
| ); |
There was a problem hiding this comment.
📝 Missing documentation: Please add a comment explaining:
- This is global state (shared across all accounts)
- Data persists across browser restarts
- Not synced across devices
| /** | |
| * Key definition for storing hostnames that the user has chosen to ignore | |
| */ | |
| export const IGNORED_PHISHING_HOSTNAMES_KEY = new KeyDefinition<string[]>( | |
| PHISHING_DETECTION_DISK, | |
| "ignoredPhishingHostnames", | |
| { | |
| deserializer: (value: string[]) => value ?? [], | |
| }, | |
| ); | |
| /** | |
| * Key definition for storing hostnames that the user has chosen to ignore. | |
| * This is global state (shared across all accounts) stored in disk storage. | |
| * Data persists across browser restarts but is not synced across devices. | |
| */ | |
| export const IGNORED_PHISHING_HOSTNAMES_KEY = new KeyDefinition<string[]>( | |
| PHISHING_DETECTION_DISK, | |
| "ignoredPhishingHostnames", | |
| { | |
| deserializer: (value: string[]) => value ?? [], | |
| }, | |
| ); |
|
|
| export const IGNORED_PHISHING_HOSTNAMES_KEY = new KeyDefinition<string[]>( | ||
| PHISHING_DETECTION_DISK, | ||
| "ignoredPhishingHostnames", | ||
| { | ||
| deserializer: (value: string[]) => value ?? [], | ||
| }, | ||
| ); |
There was a problem hiding this comment.
suggestion(non-blocking): you can also use the array helper
| export const IGNORED_PHISHING_HOSTNAMES_KEY = new KeyDefinition<string[]>( | |
| PHISHING_DETECTION_DISK, | |
| "ignoredPhishingHostnames", | |
| { | |
| deserializer: (value: string[]) => value ?? [], | |
| }, | |
| ); | |
| export const IGNORED_PHISHING_HOSTNAMES_KEY = KeyDefinition.array<string>( | |
| PHISHING_DETECTION_DISK, | |
| "ignoredPhishingHostnames", | |
| { | |
| deserializer: (value: string) => value, | |
| }, | |
| ); |
| .pipe( | ||
| map((hostnames) => hostnames ?? []), | ||
| tap((hostnames) => { | ||
| this._ignoredHostnames = new Set(hostnames); |
There was a problem hiding this comment.
suggestion: don't eject data from observable flows, it risks causing race-conditions. Just subscribe to it directly where it's needed
example:
const onContinueCommand$ = combineLatest([messageListener.messages$(PHISHING_DETECTION_CONTINUE_COMMAND), ignoredHostnamesState.state$]).pipe(
tap(([message, ignoredHostnames]) =>
logService.debug(`[PhishingDetectionService] user selected continue for ${message.url}`),
),| this._ignoredHostnames.add(url.hostname); | ||
| // Persist to storage | ||
| await ignoredHostnamesState.update(() => Array.from(this._ignoredHostnames)); |
There was a problem hiding this comment.
issue: this is susceptible to race-conditions because awaiting ignoredHostnamesState.update can cause the execution of this function to be pre-empted by e.g. an emission of ignoredHostnamesState.state$ which would cause this._ignoredHostnames to get overwritten before the anonymous function inside update(...) has a chance to fetch the value
Either stash a copy of the Set locally or, better, don't keep a copy of state data outside of state providers and subscribe to it instead
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-28394
📔 Objective
📸 Screenshots
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes