Skip to content

fix: prevent Cmd+Shift+U escape sequence leak in kitty protocol mode #2237

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

Closed
anthhub wants to merge 1 commit into from
Closed

fix: prevent Cmd+Shift+U escape sequence leak in kitty protocol mode #2237

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
30 changes: 16 additions & 14 deletions Sources/AppDelegate.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10671,24 +10671,15 @@ final class AppDelegate: NSObject, NSApplicationDelegate, UNUserNotificationCent
return true
}

// For command-based shortcuts, trust AppKit's layout-aware characters when present.
// Keep this strict for letter shortcuts to avoid physical-key collisions across layouts,
// while still allowing keyCode fallback for digit/punctuation shortcuts on non-US layouts.
// When a non-Latin input source is active (Russian, Korean, Chinese, Japanese, etc.),
// charactersIgnoringModifiers returns non-ASCII characters that can never match
// a Latin shortcut key — skip this guard and fall through to layout-based matching.
let hasEventChars = !(eventCharsIgnoringModifiers?.isEmpty ?? true)
let eventCharsAreASCII = eventCharsIgnoringModifiers?.allSatisfy(\.isASCII) ?? true
if hasEventChars,
eventCharsAreASCII,
flags.contains(.command),
!flags.contains(.control),
shouldRequireCharacterMatchForCommandShortcut(shortcutKey: shortcutKey) {
return false
}

// Match using the current keyboard layout so Command shortcuts stay character-based
// across layouts (QWERTY, Dvorak, etc.) instead of being tied to ANSI physical keys.
// This also provides a reliable fallback when the kitty keyboard protocol is active:
// in that mode, charactersIgnoringModifiers may be empty or carry a synthetic value
// that does not match the shortcut key, but the keyCode-based layout translation
// still correctly identifies the physical key (e.g. keyCode 32 → "u").
let layoutCharacter = shortcutLayoutCharacterProvider(event.keyCode, event.modifierFlags)
if shortcutCharacterMatches(
eventCharacter: layoutCharacter,
Expand All @@ -10699,6 +10690,18 @@ final class AppDelegate: NSObject, NSApplicationDelegate, UNUserNotificationCent
return true
}

// For command-based shortcuts, once both character sources have been tried, block
// further keyCode fallback for letter shortcuts to avoid physical-key collisions
// across layouts. Non-ASCII characters (Russian, Korean, CJK, etc.) cannot match
// a Latin shortcut key, so always allow keyCode fallback in that case.
if hasEventChars,
eventCharsAreASCII,
flags.contains(.command),
!flags.contains(.control),
shouldRequireCharacterMatchForCommandShortcut(shortcutKey: shortcutKey) {
return false
}
Comment on lines 10683 to +10703

@greptile-apps greptile-apps Bot Mar 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Missing regression test for kitty protocol fix

CLAUDE.md requires a two-commit regression test structure for bug fixes: the test is added first (CI goes red), then the fix (CI goes green). This scenario is directly unit-testable using the existing harness — AppDelegateShortcutRoutingTests already overrides shortcutLayoutCharacterProvider and constructs synthetic NSEvents (see the Cyrillic-layout tests around line 3022). A companion test could look like:

// Kitty keyboard protocol: charactersIgnoringModifiers carries a synthetic ASCII
// escape sequence, but the keyCode-based layout translation resolves the physical key.
appDelegate.shortcutLayoutCharacterProvider = { keyCode, _ in
    keyCode == 32 ? "u" : nil  // kVK_ANSI_U
}
guard let event = NSEvent.keyEvent(
    with: .keyDown,
    location: .zero,
    modifierFlags: [.command, .shift],
    timestamp: 0,
    windowNumber: window.windowNumber,
    context: nil,
    characters: "[12629;10u",
    charactersIgnoringModifiers: "[12629;10u",  // synthetic kitty value
    isARepeat: false,
    keyCode: 32
) else { return }

// matchShortcut should return true (event consumed, no escape leak)
XCTAssertTrue(appDelegate.matchShortcut(event: event, shortcut: /* jumpToUnread shortcut */))

Without a regression test, a future refactor could silently reintroduce the ordering bug.

Context Used: CLAUDE.md (source)


// Control-key combos can surface as ASCII control characters (e.g. Ctrl+H => backspace),
// so keep ANSI keyCode fallback for control-modified shortcuts. Also allow fallback for
// command punctuation shortcuts, since some non-US layouts report different characters
Expand All @@ -10709,7 +10712,6 @@ final class AppDelegate: NSObject, NSApplicationDelegate, UNUserNotificationCent
// physical key code is the definitive identifier for the intended shortcut.
// For empty-character events (synthetic/browser key equivalents), preserve the original
// behavior: only fall back when the layout translation also failed.
let hasUsableEventChars = hasEventChars && eventCharsAreASCII
let allowANSIKeyCodeFallback = flags.contains(.control)
|| (flags.contains(.command)
&& !flags.contains(.control)
Expand Down