Desktop autotype remove SHIFT from valid modifier keys #17347
Conversation
|
New Issues (8)Checkmarx found the following issues in this Pull Request
Fixed Issues (1)Great job! The following issues were fixed in this Pull Request
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## autofill/pm-26227/autotype-strict-type-keyboard-input #17347 +/- ##
=========================================================================================
+ Coverage 40.51% 40.52% +0.01%
=========================================================================================
Files 3532 3532
Lines 101021 101024 +3
Branches 15124 15124
=========================================================================================
+ Hits 40929 40943 +14
+ Misses 58363 58352 -11
Partials 1729 1729 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Claude finished @neuronull's task —— View job Code Review Complete
Summary of Changes Since Last ReviewThree commits were added addressing previous feedback:
Previous Issues - Status UpdateFinding 1 (Key name check): ✅ FIXED - Changed back to Critical FindingsFinding 1: Inconsistent modifier terminology creates confusion The codebase mixes "Super" (Electron/internal) with "Win" (user-facing) and "Meta" (DOM API):
Risk: Future maintainers may introduce bugs due to this three-way mapping. The line 124 string replacement is fragile. Recommended refactoringConsider normalizing earlier in the pipeline: if (hasSuper) {
parts.push("Win"); // Store display name directly
}
// Remove the .replace() call
return parts.join("+");This eliminates the Super↔Win conversion and reduces cognitive load. The Finding 2: Regex validator accepts duplicate modifiers (apps/desktop/src/autofill/components/autotype-shortcut.component.ts:137) Pattern While physically impossible to trigger, this violates data integrity. Recommended: Add explicit duplicate check or open bug ticket. Test Coverage GapsCodecov reports 41.17% patch coverage with 10 missing lines: autotype-shortcut.component.ts (6 missing):
main-autotype-keyboard-shortcut.ts (2 missing):
desktop-autotype.service.ts (2 missing):
Recommended test additions// Test Shift rejection via keyboard event
it("should reject shortcut when shift is pressed", () => {
const event = new KeyboardEvent("keydown", {
key: "A",
ctrlKey: true,
shiftKey: true, // Should be rejected
});
component.onShortcutKeydown(event);
expect(component.setShortcutForm.controls.shortcut.value).toBe("");
});
// Test Meta key rejection
it("should reject pure Meta key press", () => {
const event = new KeyboardEvent("keydown", {
key: "Meta",
metaKey: true,
});
component.onShortcutKeydown(event);
expect(component.setShortcutForm.controls.shortcut.value).toBe("");
});Security & Correctness✅ Shift removal correctly prevents capitalization blocking Good Practices Observed
Action ItemsFor this PR:
Follow-up work: Overall Assessment: The PR successfully removes Shift from valid modifiers and updates the default shortcut. The recent commits addressed key previous feedback. The remaining issues are either acknowledged as future work or lower-priority improvements. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // Allow modifiers in any order, but only 1-2 modifiers total | ||
| const pattern = | ||
| /^(?=.*\b(Control|Alt|Shift|Win)\b)(?:Control\+|Alt\+|Shift\+|Win\+){1,2}[A-Z]$/i; | ||
| const pattern = /^(?=.*\b(Control|Alt|Win)\b)(?:Control\+|Alt\+|Win\+){1,2}[A-Z]$/i; |
There was a problem hiding this comment.
Control+Control+A as valid. While this may not cause immediate issues, it's semantically incorrect.
Suggested improvement
Consider adding validation to reject duplicate modifiers:
const pattern = /^(?:(?:Control|Alt|Win)\+){1,2}[A-Z]$/i;
if (!pattern.test(value)) {
return { invalidShortcut: { message: this.i18nService.t("invalidShortcut") } };
}
// Validate no duplicate modifiers
const parts = value.split('+');
const modifiers = parts.slice(0, -1);
const uniqueModifiers = new Set(modifiers.map(m => m.toLowerCase()));
return uniqueModifiers.size === modifiers.length
? null
: { invalidShortcut: { message: this.i18nService.t("invalidShortcut") } };There was a problem hiding this comment.
I didn't introduce this, it was existing, but it seems legit. If so, I'd vote to open a separate bug
There was a problem hiding this comment.
I mean, it shouldn't be physically possible to do a double modifier key, because the first is already pressed down. but it doesn't hurt to guard against or at least comment on
| <div bitDialogContent> | ||
| <p> | ||
| {{ "editAutotypeShortcutDescription" | i18n }} | ||
| {{ "editAutotypeKeyboardModifiersDescription" | i18n }} |
There was a problem hiding this comment.
editAutotypeShortcutDescription to editAutotypeKeyboardModifiersDescription. Please verify that:
- All language translation files (not just
en/messages.json) have been updated with this new key - The old key has been removed from all language files to avoid confusion
Otherwise, non-English users may see missing translations.
There was a problem hiding this comment.
I think this happens as a follow up in some shape or form
| // Allow modifiers in any order, but only 1-2 modifiers total | ||
| const pattern = | ||
| /^(?=.*\b(Control|Alt|Shift|Win)\b)(?:Control\+|Alt\+|Shift\+|Win\+){1,2}[A-Z]$/i; | ||
| const pattern = /^(?=.*\b(Control|Alt|Win)\b)(?:Control\+|Alt\+|Win\+){1,2}[A-Z]$/i; |
There was a problem hiding this comment.
❌ Critical: Regex allows duplicate modifiers
While you noted this was pre-existing, the current regex pattern accepts invalid shortcuts like Control+Control+A or Alt+Alt+B. This is semantically incorrect and should be fixed.
Why it happens:
The pattern (?:Control\+|Alt\+|Win\+){1,2} matches any sequence of 1-2 modifier+plus combinations without checking for uniqueness.
Recommended fix:
const pattern = /^(?=.*\b(Control|Alt|Win)\b)(?:Control\+|Alt\+|Win\+){1,2}[A-Z]$/i;
if (!pattern.test(value)) {
return { invalidShortcut: { message: this.i18nService.t("invalidShortcut") } };
}
// Additional check: ensure no duplicate modifiers
const parts = value.split('+');
const modifiers = parts.slice(0, -1).map(m => m.toLowerCase());
const uniqueModifiers = new Set(modifiers);
if (uniqueModifiers.size !== modifiers.length) {
return { invalidShortcut: { message: this.i18nService.t("invalidShortcut") } };
}
return null;While physically triggering duplicate modifiers may be impossible, validating against it prevents edge cases and maintains data integrity.
apps/desktop/src/autofill/components/autotype-shortcut.component.ts
Outdated
Show resolved
Hide resolved
| // Allow modifiers in any order, but only 1-2 modifiers total | ||
| const pattern = | ||
| /^(?=.*\b(Control|Alt|Shift|Win)\b)(?:Control\+|Alt\+|Shift\+|Win\+){1,2}[A-Z]$/i; | ||
| const pattern = /^(?=.*\b(Control|Alt|Win)\b)(?:Control\+|Alt\+|Win\+){1,2}[A-Z]$/i; |
There was a problem hiding this comment.
While you noted this is pre-existing and physically impossible to trigger, the regex accepts Control+Control+A or Alt+Alt+B. Consider adding explicit duplicate detection for data integrity:
const pattern = /^(?=.*\b(Control|Alt|Win)\b)(?:Control\+|Alt\+|Win\+){1,2}[A-Z]$/i;
if (!pattern.test(value)) {
return { invalidShortcut: { message: this.i18nService.t("invalidShortcut") } };
}
// Validate no duplicate modifiers
const parts = value.split('+');
const modifiers = parts.slice(0, -1).map(m => m.toLowerCase());
const uniqueModifiers = new Set(modifiers);
if (uniqueModifiers.size !== modifiers.length) {
return { invalidShortcut: { message: this.i18nService.t("invalidShortcut") } };
}
return null;
|
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-28076
📔 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