Desktop Autotype fix feature triggering in settings menu

[neuronull]

🎟️ Tracking

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

📔 Objective

Currently if the existing shortcut is re-entered in the settings pop-up, it triggers the feature. This might be benign but we shouldn't be going down that code path at all in the settings menu, and the worst case scenario is if a vault item matches bw, the credentials for that item would be sent to the os to be typed.

easy solution to disable and re-enable.

manually verified the console logs showing feature disabled when edit is clicked, and re-enabled after shortcut is saved.

⏰ Reminders before review

• Contributor guidelines followed
• All formatters and local linters executed and passed
• Written new unit and / or integration tests where applicable
• Protected functional changes with optionality (feature flags)
• Used internationalization (i18n) for all UI strings
• CI builds passed
• Communicated to DevOps any deployment requirements
• Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 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

[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

[github-actions]

Checkmarx One – Scan Summary & Details – 23795687-0464-4615-8289-531eafb38152

Great job! No new security vulnerabilities introduced in this pull request

[codecov]

Codecov Report

❌ Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 41.53%. Comparing base (ffaf474) to head (4aea3d1).
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...pps/desktop/src/app/accounts/settings.component.ts	0.00%	2 Missing ⚠️

Additional details and impacted files

@@                                  Coverage Diff                                   @@
##           autofill/pm-27195/register-autotype-svc-with-login   #17808      +/-   ##
======================================================================================
- Coverage                                               41.53%   41.53%   -0.01%     
======================================================================================
  Files                                                    3557     3557              
  Lines                                                  102995   102997       +2     
  Branches                                                15485    15485              
======================================================================================
  Hits                                                    42778    42778              
- Misses                                                  58399    58401       +2     
  Partials                                                 1818     1818              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[claude]

Claude finished @neuronull's task —— View job

---

Code Review Complete

I've reviewed the PR changes that fix the autotype feature being triggerable in the settings menu. Here's my detailed analysis:

Overview

The fix addresses a security concern where re-entering the existing autotype shortcut in the settings dialog could trigger the autotype feature, potentially causing credentials to be typed into the settings window.

✅ Positive Aspects

1. Security Fix is Sound: Disabling the autotype feature before opening the shortcut dialog effectively prevents the feature from being triggered during settings configuration.

2. Clear Comments: The inline comments explain the rationale well - particularly noting that the feature is only available to edit when already enabled, so we know we need to re-enable it after.

3. Simple Solution: The approach is straightforward and easy to understand - disable, show dialog, re-enable.

⚠️ Concerns and Suggestions

1. Re-enable Happens Too Early (Major)

The current implementation re-enables the autotype feature before checking if the user canceled or closed the dialog:

const newShortcutArray = await firstValueFrom(dialogRef.closed);

// re-enable
await this.desktopAutotypeService.setAutotypeEnabledState(true);

if (!newShortcutArray) {
  return;
}

Issue: If dialogRef.closed emits null or undefined (user canceled), the feature is re-enabled but the function returns early without saving the new shortcut. This creates a brief window where:

• The feature is enabled with the old shortcut
• The settings UI might be in an inconsistent state

Recommendation: Move the re-enable logic to after the null check OR into a finally block:

const newShortcutArray = await firstValueFrom(dialogRef.closed);

if (!newShortcutArray) {
  // re-enable since user canceled
  await this.desktopAutotypeService.setAutotypeEnabledState(true);
  return;
}

// re-enable before saving new shortcut
await this.desktopAutotypeService.setAutotypeEnabledState(true);

this.form.controls.autotypeShortcut.setValue(
  this.getFormattedAutotypeShortcutText(newShortcutArray),
);
await this.desktopAutotypeService.setAutotypeKeyboardShortcutState(newShortcutArray);

Or use try-finally for cleaner flow:

await this.desktopAutotypeService.setAutotypeEnabledState(false);

try {
  const dialogRef = AutotypeShortcutComponent.open(this.dialogService);
  const newShortcutArray = await firstValueFrom(dialogRef.closed);

  if (!newShortcutArray) {
    return;
  }

  this.form.controls.autotypeShortcut.setValue(
    this.getFormattedAutotypeShortcutText(newShortcutArray),
  );
  await this.desktopAutotypeService.setAutotypeKeyboardShortcutState(newShortcutArray);
} finally {
  await this.desktopAutotypeService.setAutotypeEnabledState(true);
}

2. Typo in Comment (Minor)

Line 976: "avaialble" should be "available"

apps/desktop/src/app/accounts/settings.component.ts:976

3. Error Handling (Minor)

Consider what happens if:

• setAutotypeEnabledState(false) fails - should we proceed?
• firstValueFrom(dialogRef.closed) throws an error - feature remains disabled
• setAutotypeEnabledState(true) fails - feature stays disabled

The try-finally pattern would help ensure re-enabling happens even if errors occur.

4. Race Condition Consideration (Question)

What happens if the user:

1. Opens the shortcut dialog (feature disabled)
2. Opens another settings window or performs another action that expects autotype to be in its normal state
3. The feature is still disabled from step 1

Is this a concern in practice? Does the dialog block other interactions?

📝 Testing Recommendations

To verify this fix thoroughly:

1. ✅ Verify autotype doesn't trigger when re-entering existing shortcut in settings
2. ✅ Verify autotype works after successfully changing shortcut
3. ✅ Verify autotype works after canceling the shortcut dialog (current code does this, but awkwardly)
4. ⚠️ Verify autotype works if an error occurs during the dialog flow
5. ⚠️ Verify behavior if user has multiple windows open

Summary

The core fix is correct and addresses the security concern. However, the re-enable logic placement could be improved for better robustness. I'd recommend using a try-finally pattern to ensure the feature is always re-enabled, regardless of whether the user saves, cancels, or encounters an error.

The current implementation works but leaves a small window for edge cases where the feature state could be inconsistent.

---