Skip to content

fix(editor): resolve the ranges input services send against the document before using them - #2351

Merged
datlechin merged 1 commit into
mainfrom
fix/nstextinput-range-guards
Aug 21, 2026
Merged

fix(editor): resolve the ranges input services send against the document before using them#2351
datlechin merged 1 commit into
mainfrom
fix/nstextinput-range-guards

Conversation

@datlechin

Copy link
Copy Markdown
Member

Fixes #2339.

The problem

TextView's NSTextInputClient conformance treated a caller-supplied NSRange as trusted. The callers are macOS system services, and they are documented to send ranges that fall outside the document: the doc comment sitting directly above the method that crashes is Apple's own text, and it says an implementation "should be prepared for aRange to be out of bounds", should return the intersection, and should return nil when the location is completely outside. Ghostty ships a comment on the same method saying "a lot of macOS system behaviors request bogus ranges", naming Look Up and QuickLook, so an IME is not even needed to reach this.

The write half is worse than the read half, and it is not an Objective-C exception at all:

  • Read half. attributedSubstring(forProposedRange:actualRange:) and firstRect(forCharacterRange:actualRange:) passed the raw range to rangeOfComposedCharacterSequences(for:) and attributedSubstring(from:). Measured on macOS 27: NSInvalidArgumentException ("The index N is invalid") and NSRangeException. Whether that kills the app depends on which dispatch path the callback arrived on. Measured: an exception under sendEvent, a Timer, or a CFRunLoopSource0 callout is swallowed; one inside a main-queue block is fatal; and NSApplicationCrashOnExceptions=YES, which any crash-reporting SDK sets, makes it always fatal. Even swallowed it unwinds AppKit's input-manager frames mid-operation and skips Swift deinits, so the input session is left inconsistent.
  • Write half. insertText(_:replacementRange:) and setMarkedText(_:selectedRange:replacementRange:) forwarded the range into replaceCharacters(in:with:), which registers undo before it validates anything. Building the inverse mutation slices the storage, and TextStory answers an out-of-range slice with fatalError("Range invalid for string"). Measured: insertText("X", replacementRange: NSRange(location: 4, length: 99)) on a five-character document exits with signal 5. No exception handler can reach that one and no dispatch path swallows it.

unmarkText() and the marked-text branch of insertText feed stored ranges back in, and MarkedTextManager has no hook for edits made through any other path, so its ranges outlive the text they were computed against.

The fix

The clamp sits at the NSTextInputClient boundary, which is where the untrusted values enter, and replaceCharacters stays strict for first-party callers such as Vim and the inline suggestions. Every peer engine that implements this protocol without NSTextView does the same: STTextView, Chromium, WebKit, Zed and Ghostty all resolve the range against the document before touching storage.

  • NSRange.resolved(inDocumentOfLength:) composes the package's existing clamped(toLength:) and returns nil for the cases a clamp cannot express: NSNotFound, a negative location or length, and a location plus length that overflows. That last one also protects clamped(toLength:) itself, whose max traps on those inputs.
  • attributedSubstring(forProposedRange:actualRange:) presets actualRange to {NSNotFound, 0}, returns nil when the range names nothing in the document, and otherwise writes the resolved range and returns that substring.
  • firstRect(forCharacterRange:actualRange:) resolves outside the actualRange != nil branch. The raising call used to sit inside it, so the old code only crashed when the caller wanted the adjusted range back. A zero-length range still resolves, because that is how an input method asks where to put its candidate window.
  • The three sites that replace text on an input service's behalf resolve their ranges first, and two ranges that resolve to the same position collapse to one, so a multi-cursor composition cannot insert the same text once per cursor.
  • MarkedTextManager resolves its own stored ranges before they are used. Resolving only what gets replaced would stop the crash and leave the bookkeeping stale, which corrupts the composition instead: measured, marking "n" into "Hello", replacing the document with "Hi" from elsewhere, then typing the composition gave "Hini" and then "Hininih" rather than "Hinih".
  • firstRect falls back to the end-of-document rect, converted through the window, for a range it cannot resolve. Returning a bare NSZeroRect would have put the candidate window, the press-and-hold accent popover and the dictation indicator in the corner of the display, because that value is read as screen coordinates.
  • attributedSubstring returns nil only when the location itself is outside the document, which is the case the protocol documents. A caret position inside it answers with an empty string, so a client reading the caret's context can still tell "no characters selected" from "no text here".

Verification

  • swift test --package-path LocalPackages/CodeEditTextView: 165 tests in 16 suites pass. Against the unfixed code the new suite dies with NSInvalidArgumentException: The index 3 is invalid, which is the reported failure.
  • 14 new cases cover resolution itself, both read methods (past the end, entirely past the end, NSNotFound, Int.max length, empty document, grapheme rounding, caret positions), the caret rect, the NULL actualRange path, both write methods, the multi-cursor collapse, and a composition that survives an edit made outside it.
  • xcodebuild -scheme TablePro build: passes.
  • SwiftLint on the changed files adds no violations; the four it reports in TextView+NSTextInput.swift are present at HEAD as well.

No UI automation: reaching this needs a real input service to send a stale range, which no deterministic XCUITest can arrange.

Found while investigating, not fixed here

All four were reproduced with compiled probes. None is required for this fix to be correct.

  1. unmarkText() deletes the composition instead of accepting it, contradicting the docstring three lines above it. Measured side by side against NSTextView: on "alpha " with "ceshi" marked, AppKit keeps "alpha ceshi" and drops only the marking, while this view leaves "alpha ". Shift-click mid-composition, at TextView+Mouse.swift:52, throws away what the user typed.
  2. MarkedTextManager.updateForNewSelections returns the inverse of what its own comment says, so collapsing to one cursor mid-composition leaves both marked ranges live and the commit writes two edits.
  3. CEUndoManager.registerMutation never consults isDisabled, so every IME keystroke becomes its own undo step and Cmd+Z walks back through romaji that was never committed.
  4. MarkedTextManager still has no edit-tracking hook of the kind selectionManager.didReplaceCharacters gives selections. This change resolves its ranges every time they are used, which is enough for the composition to stay correct, but the ranges still go stale in between.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@datlechin
datlechin merged commit c2d86e1 into main Aug 21, 2026
7 checks passed
@datlechin
datlechin deleted the fix/nstextinput-range-guards branch August 21, 2026 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Editor: IME and dictation can crash the app through unguarded ranges in TextView+NSTextInput

1 participant