[PM-32016] - make at-risk banner dismissable

[jaasen-livefront]

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-32016

📔 Objective

The at-risk password warning callout on the View Login page was persistent — it reappeared every time a user opened the item, with no way to dismiss it. For premium users this is noisy, since they may be aware of the risk and not ready to change the password immediately.

This change adds a dismiss button (X) to the at-risk password callout in the browser extension and web app only. Desktop is intentionally excluded.

Behavior:

• Dismissed state is stored locally per-client (disk, web: disk-local) and does not roam between clients.
• The dismissed state is keyed by cipher ID and the password's revision date at time of dismissal. If the user later changes the password, the revision date changes and the callout automatically reappears on next visit.
• Dismissed state is cleared on logout.
• The inline "Change at-risk password" link near the password field is unaffected and remains visible after dismissal.
• Both personal at-risk ciphers (premium) and organization-assigned security tasks show the dismissable callout on browser/web.

Implementation:

• Added VAULT_AT_RISK_VIEW_DISK_LOCAL state definition (disk, web: disk-local) and DISMISSED_AT_RISK_CIPHERS_KEY user key definition to persist dismissed cipher IDs alongside the revision date at time of dismissal.
• Added allowDismissAtRiskCallout input to CipherViewComponent (default false). Browser extension and web app pass true.
• showAtRiskCallout computed combines the existing showChangePasswordLink() condition with !atRiskBannerDismissed().
• bit-callout's built-in dismiss button is activated by binding (dismiss) on the dismissable variant only.

📸 Screenshots

Screen.Recording.2026-05-04.at.4.23.47.PM.mov

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[theMickster]

Howdy @jaasen-livefront 👋🏼

I am field testing the new multi-agent code review, I gave it a local run against the PR. The findings are below for your analysis. Please incorporate the findings that are true signals.

We are also looking closely into the difference in the way that Claude Code runs the review given different models for pros, cons, etc. We are going to continue to refine the context delivered to Claude during this reviews which may also include information from the Jira ticket itself to help ground Claude in reality.

Many Thanks!

Sonnet Code Review: [PM-32016] - make at-risk banner dismissable (#20505)

Date: 2026-05-05 | Reviewed by: Claude Code (sonnet)

Summary

Severity	Count
🛑 Blocker	0
⚠️ Important	3
♻️ Refactor	4

The change introduces a per-client dismiss capability for the at-risk password callout backed by a new UserKeyDefinition keyed on cipher ID + passwordRevisionDate. The mechanism is sound, but three issues warrant attention before merge: (1) the unconditional [allowDismissAtRiskCallout]="true" on vault-item-dialog.component.html reaches desktop because desktop renders that template — directly contradicting the PR description's "desktop excluded" claim; (2) the atRiskBannerDismissed pipeline omits the catchError(..., of(false)) guard that the sibling hadPendingChangePasswordTask pipeline uses, so a state-store error silently breaks callout visibility; (3) when passwordRevisionDate is null at dismissal and remains null thereafter, the dismissal becomes permanent for that cipher — the at-risk callout never reappears, undermining the primary in-context UI signal for at-risk passwords. Refactor-tier items focus on placement of state definitions, lock-time clearing of dismissed state, missing test coverage, and a minor TOCTOU in the dismiss handler.

Findings

⚠️ Important

vault-item-dialog unconditionally enables dismiss on desktop, contradicting PR intent

libs/vault/src/vault-item-dialog/vault-item-dialog.component.html:13
Caught by: Architecture agent

Details

VaultItemDialogComponent is a shared component consumed by desktop (apps/desktop/src/vault/app/vault-v3/vault.component.ts calls VaultItemDialogComponent.openDrawer(...)). This PR adds [allowDismissAtRiskCallout]="true" unconditionally to <app-cipher-view> inside vault-item-dialog.component.html. Because desktop renders this same template through the drawer, the dismissable callout variant will appear on desktop — directly contradicting the PR description's stated intent to exclude desktop. There is no client-detection guard or separate template for desktop.

Fix options:

• Do not modify vault-item-dialog.component.html; instead have each consuming app pass the input on the dialog opener.
• Introduce a dialog param (e.g., formConfig.allowDismissAtRiskCallout) and have browser/web set it while desktop leaves it false.

atRiskBannerDismissed pipeline lacks catchError, inconsistent with sibling signals

libs/vault/src/cipher-view/cipher-view.component.ts:319
Caught by: Code quality agent

Details

The new atRiskBannerDismissed signal is backed by a combineLatest/switchMap pipeline that reads from stateProvider.getUser(...).state$. If that observable errors (storage failure, deserialization error), the signal will propagate the error and callout visibility breaks silently. The sibling hadPendingChangePasswordTask pipeline already in this same file wraps its inner pipeline with catchError(..., of(false)), and the catchError import is already in scope.

Fix:

.pipe(
  map((dismissed) => { ... }),
  catchError((err) => {
    this.logService.error("Failed to read dismissed at-risk ciphers state", err);
    return of(false);
  }),
)

Permanent dismissal when passwordRevisionDate is null — warning may never reappear

libs/vault/src/cipher-view/cipher-view.component.ts:330
Caught by: Security & logic agent

Details

Principle / Category: VD (Vault Data — user security signal integrity)

In atRiskBannerDismissed:

const storedRevDate = dismissed[cipher.id];           // null at dismiss time
const currentRevDate = cipher.login?.passwordRevisionDate?.toISOString() ?? null; // still null
return storedRevDate === currentRevDate;              // null === null → true

When a Login cipher's passwordRevisionDate is absent (set outside Bitwarden, imported without the field, or pre-dating revision-date tracking) and remains absent, the dismissal check permanently returns true. The callout never reappears even though the password is still at-risk.

The PR description states: "If the user later changes the password, the revision date changes and the callout automatically reappears." This guarantee does not hold when passwordRevisionDate stays null. The callout is the primary in-context UI signal for at-risk passwords; permanent suppression without user awareness is a meaningful UX security gap.

Suggested fix: Refuse to persist a null revision date as a valid dismissal anchor:

// In dismissAtRiskCallout:
const revDate = cipher.login?.passwordRevisionDate?.toISOString() ?? null;
if (revDate === null) {
  return; // do not dismiss when there is no revision anchor
}

Or treat stored === null && current === null as "not dismissed" in the read path.

♻️ Refactor

DISMISSED_AT_RISK_CIPHERS_KEY UserKeyDefinition defined in component file

libs/vault/src/cipher-view/cipher-view.component.ts:64
Caught by: Architecture agent

Details

The established pattern is to define UserKeyDefinition constants in dedicated state or service files. The closest precedent is AT_RISK_PASSWORD_CALLOUT_KEY in libs/vault/src/services/at-risk-password-callout.service.ts, which uses a similar vault-risk domain. Placing DISMISSED_AT_RISK_CIPHERS_KEY (and its type alias DismissedAtRiskCipherRecord) inside cipher-view.component.ts makes the state key unreachable to any other consumer without importing from the component, inverting the expected dependency direction (state definitions should not depend on UI components; UI components should depend on state definitions).

Suggested placement: a dedicated state file (e.g., libs/vault/src/cipher-view/state/dismissed-at-risk-ciphers.state.ts) or alongside at-risk-password-callout.service.ts.

Dismissed callout state not cleared on vault lock

libs/vault/src/cipher-view/cipher-view.component.ts:67
Caught by: Code quality agent

Details

DISMISSED_AT_RISK_CIPHERS_KEY uses clearOn: ["logout"]. The map of { [cipherId]: passwordRevisionDate } survives vault lock and persists across lock/unlock cycles until logout.

The sibling AT_RISK_PASSWORD_CALLOUT_KEY uses "memory" storage (so it inherently clears on lock). The new key uses "disk"/"disk-local" and only clears on logout. The persisted data is metadata (cipher UUIDs and ISO date strings), not Vault Data per the security vocabulary, so this is not a P02 violation — but the inconsistency with the sibling pattern is worth resolving.

Suggested fix:

{ deserializer: (obj) => obj ?? {}, clearOn: ["lock", "logout"] }

Note: adding "lock" resets dismissed state on every lock/unlock, which is a UX trade-off the team should consciously decide.

No test coverage for dismiss flow, atRiskBannerDismissed, or StateProvider injection

libs/vault/src/cipher-view/cipher-view.component.ts:319
Caught by: Code quality agent

Details

The existing cipher-view.component.spec.ts has no mock or test for StateProvider, dismissAtRiskCallout, atRiskBannerDismissed, or showAtRiskCallout. The TestBed setup does not provide StateProvider, so the component will fail to construct in any test that does not use NO_ERRORS_SCHEMA or a component override.

Concretely missing:

• dismissAtRiskCallout() writes the correct { [cipherId]: revDate } record to state.
• atRiskBannerDismissed() returns true when stored revision date matches current.
• atRiskBannerDismissed() returns false when stored differs (password changed since dismissal).
• showAtRiskCallout() becomes false after dismissal.

Without these, regressions in the dismiss flow will not be caught by CI.

dismissAtRiskCallout uses firstValueFrom — userId may not match current cipher

libs/vault/src/cipher-view/cipher-view.component.ts:338
Caught by: Bug analysis agent

Details

const cipher = this.cipher();                          // synchronous read
const userId = await firstValueFrom(this.activeUserId$); // resolves async

Between the signal read and the firstValueFrom resolution, the active account could in principle change. The window is a single microtask in practice, so the race is theoretical rather than practically exploitable, but the safer pattern used elsewhere in this codebase is to read both atomically from the same reactive context:

const [cipher, userId] = await firstValueFrom(
  combineLatest([this.cipher$, this.activeUserId$])
);

This ensures cipher and userId belong to the same emission, eliminating the interleaving window.

Reviewed and Dismissed

🔍 6 initial findings dismissed after validation

Two near-duplicate bit-callout blocks instead of using built-in conditional dismiss

libs/vault/src/cipher-view/cipher-view.component.html:14
Caught by: Architecture agent
Original severity: ♻️ Refactor
Original confidence: 88/100
Dismissed at: Step 4 validation
Dismissed because: bit-callout's isDismissible flag is set in ngOnInit based on whether dismiss$ has subscribers. Angular output bindings are established at component creation time, so conditionally binding (dismiss) on the same element instance is not possible without structural directives that would recreate the element — reintroducing the duplication. The two-callout pattern is the correct Angular idiom for this conditional case.

UserKeyDefinition exported from a component file (duplicate)

libs/vault/src/cipher-view/cipher-view.component.ts:65
Caught by: Code quality agent
Original severity: ♻️ Refactor
Original confidence: 90/100
Dismissed at: Step 4 validation
Dismissed because: Duplicate of arch-3. Both findings describe the same defect; arch-3 is retained.

atRiskBannerDismissed accesses cipher.id without null-guarding cipher

libs/vault/src/cipher-view/cipher-view.component.ts:319
Caught by: Bug analysis agent
Original severity: ⚠️ Important
Original confidence: 90/100
Dismissed at: Step 4 validation
Dismissed because: cipher is provided via input.required<CipherView>(), which guarantees the input is non-null at subscription time. The combineLatest pipeline with cipher$ (toObservable of a required signal) will only emit a CipherView. Other sibling signals in the same component access cipher.id and cipher.type directly without null-guarding — established safe pattern.

Non-dismissible callout still renders when allowDismissAtRiskCallout=false even after prior dismissal

libs/vault/src/cipher-view/cipher-view.component.html:14
Caught by: Bug analysis agent
Original severity: ⚠️ Important
Original confidence: 88/100
Dismissed at: Step 4 validation
Dismissed because: Dismiss state is per-client (disk/disk-local) and is only written when the dismissable variant's button is clicked — which requires allowDismissAtRiskCallout=true. A client where allowDismissAtRiskCallout=false cannot accumulate dismiss state for itself. Cross-client roaming does not apply. The desktop-specific concern is captured separately by arch-1.

TOCTOU between cipher() signal read and firstValueFrom(activeUserId$) (duplicate)

libs/vault/src/cipher-view/cipher-view.component.ts:344
Caught by: Security & logic agent
Original severity: ♻️ Refactor
Original confidence: 85/100
Dismissed at: Step 4 validation
Dismissed because: Duplicate of bug-2; bug-2 is retained.

Dismissed record grows without bound — no cleanup on cipher deletion

libs/vault/src/cipher-view/cipher-view.component.ts:65
Caught by: Security & logic agent
Original severity: ♻️ Refactor
Original confidence: 82/100
Dismissed at: Step 4 validation
Dismissed because: Cipher IDs are opaque UUIDs — they are not Vault Data per Bitwarden's security vocabulary (passwords, usernames, secure notes, credit cards, identities, attachments). P02 protects highly sensitive vault data; a set of cipher IDs does not meet that bar. The unbounded growth is a storage hygiene concern that a senior engineer would not block this PR on.

---

Opus Code Review: [PM-32016] - make at-risk banner dismissable (#20505)

Date: 2026-05-05 | Reviewed by: Claude Code

Summary

Severity	Count
🛑 Blocker	0
⚠️ Important	1
♻️ Refactor	2

The feature logic is sound — keying dismissal state by cipher ID and revision date is correct, and the per-client storage strategy is appropriate. However, there is one meaningful functional bug: VaultItemDialogComponent is shared with the desktop app, so the unconditional [allowDismissAtRiskCallout]="true" binding will make the dismiss button appear on desktop despite the PR explicitly stating that desktop is intentionally excluded. Two refactor findings cover template duplication and a state-key definition that diverges from the established sibling pattern.

Findings

⚠️ Important

Desktop is NOT excluded — VaultItemDialog is used by desktop, contradicting PR description

libs/vault/src/vault-item-dialog/vault-item-dialog.component.html:16
Caught by: Architecture agent

Details

The PR description states: "This change adds a dismiss button (X) to the at-risk password callout in the browser extension and web app only. Desktop is intentionally excluded."

However, the diff sets [allowDismissAtRiskCallout]="true" unconditionally on <app-cipher-view> inside VaultItemDialogComponent. VaultItemDialogComponent is consumed by the desktop app at:

apps/desktop/src/vault/app/vault-v3/vault.component.ts:891

this.activeDrawerRef = await VaultItemDialogComponent.openDrawer(this.dialogService, { ... });

As written, every desktop user who views an at-risk cipher will see the dismiss button — the stated exclusion is not enforced.

Suggested fix (either approach):

Option A — Make allowDismissAtRiskCallout an input on VaultItemDialogComponent itself (default false), and set it to true only from web callers (apps/web/src/app/...). The desktop openDrawer call inherits the safe false default.

Option B — Keep the binding in the template but resolve the value inside VaultItemDialogComponent based on a runtime platform check, so desktop continues to receive false.

♻️ Refactor

Two near-identical bit-callout blocks introduced; bodies duplicated rather than parameterized

libs/vault/src/cipher-view/cipher-view.component.html:14
Caught by: Architecture agent

Details

The diff replaces one <bit-callout> with two blocks whose entire inner content — the @if (changePasswordLink()) link/text branch — is duplicated verbatim. The blocks differ only in their *ngIf condition and whether (dismiss) is bound. Future edits to the callout body (i18n key, link attributes, icon, click handler) must be made in both places, and a divergence will silently produce inconsistent UX across the non-dismissable and dismissable variants.

Suggested fix: Use a single callout:

<bit-callout
  *ngIf="showAtRiskCallout()"
  type="warning"
  [title]="''"
  (dismiss)="allowDismissAtRiskCallout() ? dismissAtRiskCallout() : null"
>
  @if (changePasswordLink()) {
    <a bitLink ...>{{ "changeAtRiskPassword" | i18n }}</a>
  } @else {
    {{ "changeAtRiskPassword" | i18n }}
  }
</bit-callout>

showAtRiskCallout() already encodes showChangePasswordLink() && !atRiskBannerDismissed(), which correctly collapses to "always shown" when allowDismissAtRiskCallout is false (because atRiskBannerDismissed only flips after dismissAtRiskCallout is called, which is only wired in the dismissable variant).

DISMISSED_AT_RISK_CIPHERS_KEY defined in component file diverges from established sibling pattern

libs/vault/src/cipher-view/cipher-view.component.ts:65
Caught by: Architecture agent

Details

The new UserKeyDefinition DISMISSED_AT_RISK_CIPHERS_KEY (and its DismissedAtRiskCipherRecord type) is declared inline in the component file. The established sibling in the same library — AT_RISK_PASSWORD_CALLOUT_KEY — lives in a dedicated service module at libs/vault/src/services/at-risk-password-callout.service.ts, alongside its data type and a service that owns reads/writes via StateProvider.

Mixing state-key declarations into UI components creates two concrete maintenance costs:

1. State keys become discoverable only by reading the component file, fragmenting where reviewers look for the at-risk feature's persistent state.
2. The component now owns both rendering and persistence logic (atRiskBannerDismissed signal + dismissAtRiskCallout writer), where the existing pattern factors persistence behind a service — making the new code harder to reuse from any future view surface without copying state plumbing.

Suggested fix: Extract DISMISSED_AT_RISK_CIPHERS_KEY, DismissedAtRiskCipherRecord, and the state read/write helpers into a service in libs/vault/src/services/ (e.g. at-risk-callout-dismissal.service.ts), matching the shape of AtRiskPasswordCalloutService.

Reviewed and Dismissed

🔍 1 initial finding dismissed after validation

Two near-identical bit-callout blocks differ only in dismiss handler

libs/vault/src/cipher-view/cipher-view.component.html:14
Caught by: Code quality agent
Original severity: ♻️ Refactor
Original confidence: 80/100
Dismissed at: Step 4 validation
Dismissed because: Duplicate of arch-3 — both flag the same template duplication of the two near-identical <bit-callout> blocks at the same file and line.