Skip to content

Desktop autotype remove SHIFT from valid modifier keys #17347

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: autofill/pm-26227/autotype-strict-type-keyboard-input
Choose a base branch
from
+131 −94
Open

Desktop autotype remove SHIFT from valid modifier keys #17347

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2e1dfea
Desktop autotype remove SHIFT from valid modifier keys
neuronull Nov 12, 2025
082bd43
update code comment
neuronull Nov 12, 2025
7f04588
better error for unsupported modifier key
neuronull Nov 12, 2025
5701b04
unecessary return
neuronull Nov 12, 2025
f0fd4da
revert the string to meta
neuronull Nov 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions apps/desktop/desktop_native/autotype/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
use anyhow::Result;

#[cfg(target_os = "windows")]
mod modifier_keys;

#[cfg(target_os = "windows")]
pub(crate) use modifier_keys::*;

#[cfg_attr(target_os = "linux", path = "linux.rs")]
#[cfg_attr(target_os = "macos", path = "macos.rs")]
#[cfg_attr(target_os = "windows", path = "windows/mod.rs")]
Expand Down
44 changes: 44 additions & 0 deletions apps/desktop/desktop_native/autotype/src/modifier_keys.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
use std::{collections::HashMap, sync::OnceLock};

// Electron modifier keys
// <https://www.electronjs.org/docs/latest/tutorial/keyboard-shortcuts#cross-platform-modifiers>
pub(crate) const CONTROL_KEY_STR: &str = "Control";
pub(crate) const ALT_KEY_STR: &str = "Alt";
pub(crate) const SUPER_KEY_STR: &str = "Super";

// numeric values for modifier keys
pub(crate) const CONTROL_KEY: u16 = 0x11;
pub(crate) const ALT_KEY: u16 = 0x12;
pub(crate) const SUPER_KEY: u16 = 0x5B;

/// A mapping of <Electron modifier key string> to <numeric representation>
static MODIFIER_KEYS: OnceLock<HashMap<&str, u16>> = OnceLock::new();

/// Provides a mapping of the valid modifier keys' electron
/// string representation to the numeric representation.
pub(crate) fn get_modifier_keys() -> &'static HashMap<&'static str, u16> {
MODIFIER_KEYS.get_or_init(|| {
HashMap::from([
(CONTROL_KEY_STR, CONTROL_KEY),
(ALT_KEY_STR, ALT_KEY),
(SUPER_KEY_STR, SUPER_KEY),
])
})
}

#[cfg(test)]
mod test {
use super::get_modifier_keys;

#[test]
fn valid_modifier_keys() {
assert_eq!(get_modifier_keys().get("Control").unwrap(), &0x11);
assert_eq!(get_modifier_keys().get("Alt").unwrap(), &0x12);
assert_eq!(get_modifier_keys().get("Super").unwrap(), &0x5B);
}

#[test]
fn does_not_contain_invalid_modifier_keys() {
assert!(!get_modifier_keys().contains_key("Shift"));
}
}
1 change: 0 additions & 1 deletion apps/desktop/desktop_native/autotype/src/windows/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@ pub fn get_foreground_window_title() -> Result<String> {
/// - Control
/// - Alt
/// - Super
/// - Shift
/// - \[a-z\]\[A-Z\]
struct KeyboardShortcutInput(INPUT);

Expand Down
48 changes: 27 additions & 21 deletions apps/desktop/desktop_native/autotype/src/windows/type_input.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,9 @@ use windows::Win32::UI::Input::KeyboardAndMouse::{
VIRTUAL_KEY,
};

use super::{ErrorOperations, KeyboardShortcutInput, Win32ErrorOperations};
use crate::get_modifier_keys;

const SHIFT_KEY_STR: &str = "Shift";
const CONTROL_KEY_STR: &str = "Control";
const ALT_KEY_STR: &str = "Alt";
const LEFT_WINDOWS_KEY_STR: &str = "Super";
use super::{ErrorOperations, KeyboardShortcutInput, Win32ErrorOperations};

const IS_VIRTUAL_KEY: bool = true;
const IS_REAL_KEY: bool = false;
Expand Down Expand Up @@ -92,22 +89,19 @@ impl TryFrom<&str> for KeyboardShortcutInput {
type Error = anyhow::Error;

fn try_from(key: &str) -> std::result::Result<Self, Self::Error> {
const SHIFT_KEY: u16 = 0x10;
const CONTROL_KEY: u16 = 0x11;
const ALT_KEY: u16 = 0x12;
const LEFT_WINDOWS_KEY: u16 = 0x5B;

// not modifier key
if key.len() == 1 {
let input = build_unicode_input(InputKeyPress::Up, get_alphabetic_hotkey(key)?);
return Ok(KeyboardShortcutInput(input));
}
// the modifier keys are using the Up keypress variant because the user has already
// pressed those keys in order to trigger the feature.
let input = match key {
SHIFT_KEY_STR => build_virtual_key_input(InputKeyPress::Up, SHIFT_KEY),
CONTROL_KEY_STR => build_virtual_key_input(InputKeyPress::Up, CONTROL_KEY),
ALT_KEY_STR => build_virtual_key_input(InputKeyPress::Up, ALT_KEY),
LEFT_WINDOWS_KEY_STR => build_virtual_key_input(InputKeyPress::Up, LEFT_WINDOWS_KEY),
_ => build_unicode_input(InputKeyPress::Up, get_alphabetic_hotkey(key)?),
};

Ok(KeyboardShortcutInput(input))
if let Some(numeric_modifier_key) = get_modifier_keys().get(key) {
let input = build_virtual_key_input(InputKeyPress::Up, *numeric_modifier_key);
return Ok(KeyboardShortcutInput(input));
} else {
return Err(anyhow!("Unsupported modifier key: {key}"));
}
}
}

Expand Down Expand Up @@ -279,7 +273,7 @@ mod tests {
#[test]
#[serial]
fn keyboard_shortcut_conversion_succeeds() {
let keyboard_shortcut = [CONTROL_KEY_STR, SHIFT_KEY_STR, "B"];
let keyboard_shortcut = ["Control", "Alt", "B"];
let _: Vec<KeyboardShortcutInput> = keyboard_shortcut
.iter()
.map(|s| KeyboardShortcutInput::try_from(*s))
Expand All @@ -291,7 +285,19 @@ mod tests {
#[serial]
#[should_panic = "Letter is not ASCII Alphabetic ([a-z][A-Z]): '1'"]
fn keyboard_shortcut_conversion_fails_invalid_key() {
let keyboard_shortcut = [CONTROL_KEY_STR, SHIFT_KEY_STR, "1"];
let keyboard_shortcut = ["Control", "Alt", "1"];
let _: Vec<KeyboardShortcutInput> = keyboard_shortcut
.iter()
.map(|s| KeyboardShortcutInput::try_from(*s))
.try_collect()
.unwrap();
}

#[test]
#[serial]
#[should_panic(expected = "Unsupported modifier key: Shift")]
fn keyboard_shortcut_conversion_fails_with_shift() {
let keyboard_shortcut = ["Control", "Shift", "B"];
let _: Vec<KeyboardShortcutInput> = keyboard_shortcut
.iter()
.map(|s| KeyboardShortcutInput::try_from(*s))
Expand Down
2 changes: 1 addition & 1 deletion apps/desktop/src/autofill/components/autotype-shortcut.component.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
</div>
<div bitDialogContent>
<p>
{{ "editAutotypeShortcutDescription" | i18n }}
{{ "editAutotypeKeyboardModifiersDescription" | i18n }}
Copy link

@claude claude bot Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ The i18n key changed from editAutotypeShortcutDescription to editAutotypeKeyboardModifiersDescription. Please verify that:

  1. All language translation files (not just en/messages.json) have been updated with this new key
  2. The old key has been removed from all language files to avoid confusion

Otherwise, non-English users may see missing translations.

Copy link
Contributor Author

@neuronull neuronull Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this happens as a follow up in some shape or form

</p>
<bit-form-field>
<bit-label>{{ "typeShortcut" | i18n }}</bit-label>
Expand Down
61 changes: 15 additions & 46 deletions apps/desktop/src/autofill/components/autotype-shortcut.component.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,9 @@ describe("AutotypeShortcutComponent", () => {
const validShortcuts = [
"Control+A",
"Alt+B",
"Shift+C",
"Win+D",
"control+e", // case insensitive
"ALT+F",
"SHIFT+G",
"WIN+H",
];

Expand All @@ -46,14 +44,7 @@ describe("AutotypeShortcutComponent", () => {
});

it("should accept two modifiers with letter", () => {
const validShortcuts = [
"Control+Alt+A",
"Control+Shift+B",
"Control+Win+C",
"Alt+Shift+D",
"Alt+Win+E",
"Shift+Win+F",
];
const validShortcuts = ["Control+Alt+A", "Control+Win+C", "Alt+Win+D", "Alt+Win+E"];

validShortcuts.forEach((shortcut) => {
const control = createControl(shortcut);
Expand All @@ -63,7 +54,7 @@ describe("AutotypeShortcutComponent", () => {
});

it("should accept modifiers in different orders", () => {
const validShortcuts = ["Alt+Control+A", "Shift+Control+B", "Win+Alt+C"];
const validShortcuts = ["Alt+Control+A", "Win+Control+B", "Win+Alt+C"];

validShortcuts.forEach((shortcut) => {
const control = createControl(shortcut);
Expand All @@ -88,15 +79,14 @@ describe("AutotypeShortcutComponent", () => {
const invalidShortcuts = [
"Control+1",
"Alt+2",
"Shift+3",
"Win+4",
"Control+!",
"Alt+@",
"Shift+#",
"Alt+#",
"Win+$",
"Control+Space",
"Alt+Enter",
"Shift+Tab",
"Control+Tab",
"Win+Escape",
];

Expand All @@ -111,12 +101,10 @@ describe("AutotypeShortcutComponent", () => {
const invalidShortcuts = [
"Control",
"Alt",
"Shift",
"Win",
"Control+Alt",
"Control+Shift",
"Alt+Shift",
"Control+Alt+Shift",
"Control+Win",
"Control+Alt+Win",
];

invalidShortcuts.forEach((shortcut) => {
Expand All @@ -127,7 +115,7 @@ describe("AutotypeShortcutComponent", () => {
});

it("should reject shortcuts with invalid modifier names", () => {
const invalidShortcuts = ["Ctrl+A", "Command+A", "Super+A", "Meta+A", "Cmd+A", "Invalid+A"];
const invalidShortcuts = ["Ctrl+A", "Command+A", "Meta+A", "Cmd+A", "Invalid+A"];

invalidShortcuts.forEach((shortcut) => {
const control = createControl(shortcut);
Expand All @@ -137,7 +125,7 @@ describe("AutotypeShortcutComponent", () => {
});

it("should reject shortcuts with multiple base keys", () => {
const invalidShortcuts = ["Control+A+B", "Alt+Ctrl+Shift"];
const invalidShortcuts = ["Control+A+B", "Alt+Ctrl+Win"];

invalidShortcuts.forEach((shortcut) => {
const control = createControl(shortcut);
Expand All @@ -148,11 +136,10 @@ describe("AutotypeShortcutComponent", () => {

it("should reject shortcuts with more than two modifiers", () => {
const invalidShortcuts = [
"Control+Alt+Shift+A",
"Control+Alt+Win+A",
"Control+Alt+Win+B",
"Control+Shift+Win+C",
"Alt+Shift+Win+D",
"Control+Alt+Shift+Win+E",
"Control+Alt+Win+C",
"Alt+Control+Win+D",
];

invalidShortcuts.forEach((shortcut) => {
Expand Down Expand Up @@ -221,7 +208,7 @@ describe("AutotypeShortcutComponent", () => {
});

it("should handle very long strings", () => {
const longString = "Control+Alt+Shift+Win+A".repeat(100);
const longString = "Control+Alt+Win+A".repeat(100);
const control = createControl(longString);
const result = validator(control);
expect(result).toEqual({ invalidShortcut: { message: "Invalid shortcut" } });
Expand All @@ -230,7 +217,7 @@ describe("AutotypeShortcutComponent", () => {

describe("modifier combinations", () => {
it("should accept all possible single modifier combinations", () => {
const modifiers = ["Control", "Alt", "Shift", "Win"];
const modifiers = ["Control", "Alt", "Win"];

modifiers.forEach((modifier) => {
const control = createControl(`${modifier}+A`);
Expand All @@ -240,14 +227,7 @@ describe("AutotypeShortcutComponent", () => {
});

it("should accept all possible two-modifier combinations", () => {
const combinations = [
"Control+Alt+A",
"Control+Shift+A",
"Control+Win+A",
"Alt+Shift+A",
"Alt+Win+A",
"Shift+Win+A",
];
const combinations = ["Control+Alt+A", "Control+Win+A", "Alt+Win+A"];

combinations.forEach((shortcut) => {
const control = createControl(shortcut);
Expand All @@ -257,25 +237,14 @@ describe("AutotypeShortcutComponent", () => {
});

it("should reject all three-modifier combinations", () => {
const combinations = [
"Control+Alt+Shift+A",
"Control+Alt+Win+A",
"Control+Shift+Win+A",
"Alt+Shift+Win+A",
];
const combinations = ["Control+Alt+Win+A", "Alt+Control+Win+A", "Win+Alt+Control+A"];

combinations.forEach((shortcut) => {
const control = createControl(shortcut);
const result = validator(control);
expect(result).toEqual({ invalidShortcut: { message: "Invalid shortcut" } });
});
});

it("should reject all four modifiers combination", () => {
const control = createControl("Control+Alt+Shift+Win+A");
const result = validator(control);
expect(result).toEqual({ invalidShortcut: { message: "Invalid shortcut" } });
});
});
});
});
28 changes: 15 additions & 13 deletions apps/desktop/src/autofill/components/autotype-shortcut.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,25 +77,31 @@ export class AutotypeShortcutComponent {
}
}

// <https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent>
private buildShortcutFromEvent(event: KeyboardEvent): string | null {
const hasCtrl = event.ctrlKey;
const hasAlt = event.altKey;
const hasShift = event.shiftKey;
const hasMeta = event.metaKey; // Windows key on Windows, Command on macOS
const hasSuper = event.metaKey; // Windows key on Windows, Command on macOS

// Require at least one modifier (Control, Alt, Shift, or Super)
if (!hasCtrl && !hasAlt && !hasShift && !hasMeta) {
// Require at least one valid modifier (Control, Alt, Super)
if (!hasCtrl && !hasAlt && !hasSuper) {
return null;
}

const key = event.key;

// Ignore pure modifier keys themselves
if (key === "Control" || key === "Alt" || key === "Shift" || key === "Meta") {
// disallow pure modifier keys themselves
if (key === "Control" || key === "Alt" || key === "Super") {
return null;
}

// Accept a single alphabetical letter as the base key
// disallow shift modifier
if (hasShift) {
return null;
}

// require a single alphabetical letter as the base key
const isAlphabetical = typeof key === "string" && /^[a-z]$/i.test(key);
if (!isAlphabetical) {
return null;
Expand All @@ -108,10 +114,7 @@ export class AutotypeShortcutComponent {
if (hasAlt) {
parts.push("Alt");
}
if (hasShift) {
parts.push("Shift");
}
if (hasMeta) {
if (hasSuper) {
parts.push("Super");
}
parts.push(key.toUpperCase());
Expand All @@ -129,10 +132,9 @@ export class AutotypeShortcutComponent {
}

// Must include exactly 1-2 modifiers and end with a single letter
// Valid examples: Ctrl+A, Shift+Z, Ctrl+Shift+X, Alt+Shift+Q
// Valid examples: Ctrl+A, Alt+B, Ctrl+Alt+X, Alt+Control+Q, Win+B, Ctrl+Win+A
// 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;
Copy link

@claude claude bot Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ This regex pattern will accept duplicate modifiers like 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") } };

Copy link
Contributor Author

@neuronull neuronull Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't introduce this, it was existing, but it seems legit. If so, I'd vote to open a separate bug

Copy link
Contributor Author

@neuronull neuronull Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

@claude claude bot Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ 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.

Copy link

@claude claude bot Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Validation gap: Regex allows duplicate modifiers

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;

return pattern.test(value)
? null
: { invalidShortcut: { message: this.i18nService.t("invalidShortcut") } };
Expand Down
Loading
Loading