From 3d7371ad1a65287851bec5f2be7554334a0978bc Mon Sep 17 00:00:00 2001 From: Michael Ramos Date: Sun, 9 Aug 2026 17:39:35 -0700 Subject: [PATCH] fix: address the three pre-release sweep findings Three real, new-in-range bugs from the free-hunt regression sweep: - The pinpoint anchor builder ran a document-wide uniqueness query per ancestor against a growing selector with no depth cap, so one click on a deeply wrapped document froze the tab synchronously (measured 58s at depth 800). The walk now abandons the anchor past 40 ancestors (fail closed: text-search restoration takes over), bounding the work to interactive time. Regression test uses two identical depth-60 chains so uniqueness cannot short-circuit before the cap. - The 10k selection-text cap sliced UTF-16 code units and could split a surrogate pair at the boundary, silently corrupting the annotation tail into U+FFFD downstream. Both sides of the bridge now back the cut off one unit when it would land mid-pair. - The old-git sparse fallback matched git's "unknown option" error literally, which localized git builds translate, so non-English users on old git hit the exact hard failure #1239 was written to fix. All three installers now pin LC_ALL=C around the probe clone (saved and restored; kept single-line in install.ps1 for the scoped-call scanner, whose expectation is updated to the new prefix). --- .../components/html-viewer/bridge-script.ts | 19 +++++++- .../html-viewer/htmlPinpointProtocol.test.tsx | 24 ++++++++++ .../ui/components/html-viewer/srcdoc.test.ts | 44 +++++++++++++++++++ .../html-viewer/useHtmlAnnotation.ts | 14 ++++-- scripts/install.cmd | 8 ++++ scripts/install.ps1 | 8 +++- scripts/install.sh | 7 ++- scripts/install.test.ts | 5 ++- 8 files changed, 122 insertions(+), 7 deletions(-) diff --git a/packages/ui/components/html-viewer/bridge-script.ts b/packages/ui/components/html-viewer/bridge-script.ts index 1dc7c49489..473476d352 100644 --- a/packages/ui/components/html-viewer/bridge-script.ts +++ b/packages/ui/components/html-viewer/bridge-script.ts @@ -336,7 +336,13 @@ export const BRIDGE_SCRIPT = `(function() { var MAX_SELECTION_TEXT = 10000; function capSelectionText(text) { - return text.length > MAX_SELECTION_TEXT ? text.slice(0, MAX_SELECTION_TEXT) : text; + if (text.length <= MAX_SELECTION_TEXT) return text; + var cut = MAX_SELECTION_TEXT; + var last = text.charCodeAt(cut - 1); + // Never split a surrogate pair: a lone high surrogate at the cut point + // becomes U+FFFD the moment the string is UTF-8 encoded downstream. + if (last >= 0xd800 && last <= 0xdbff) cut -= 1; + return text.slice(0, cut); } function handleSelection(modeOverride, extras) { @@ -1007,10 +1013,21 @@ export const BRIDGE_SCRIPT = `(function() { return null; } + // Each ancestor step runs a document-wide uniqueness query against a + // selector that grows with the path, so cost is quadratic in depth. Real + // documents anchor within a few levels; a degenerate deeply-wrapped chain + // (templated exports, generated markup) must not freeze the tab on a + // single pinpoint click. Past the cap the anchor is abandoned (fail + // closed): text-search restoration still works, and no anchor beats one + // that costs seconds of synchronous main-thread time. + var MAX_ANCHOR_PATH_DEPTH = 40; + function buildAnchorSelector(el) { var path = []; var current = el; + var depth = 0; while (current && current.nodeType === 1 && current !== document.body && current !== document.documentElement) { + if (++depth > MAX_ANCHOR_PATH_DEPTH) return null; var semantic = semanticSelectorFor(current); if (semantic) { path.unshift(semantic); diff --git a/packages/ui/components/html-viewer/htmlPinpointProtocol.test.tsx b/packages/ui/components/html-viewer/htmlPinpointProtocol.test.tsx index ca95744eee..d8754d855d 100644 --- a/packages/ui/components/html-viewer/htmlPinpointProtocol.test.tsx +++ b/packages/ui/components/html-viewer/htmlPinpointProtocol.test.tsx @@ -101,6 +101,30 @@ describe.if(hasDom)('parseBridgeMessage selection additions', () => { expect((parsed as { anchor?: unknown }).anchor).toBeUndefined(); }); + test('truncation never splits a surrogate pair at the cap boundary', () => { + // An astral character straddling the cut would leave a lone high + // surrogate that turns into U+FFFD once UTF-8-encoded (drafts, feedback, + // share URLs) — the cut must back off one unit instead. + const cap = hookModule!.MAX_SELECTION_TEXT_LENGTH; + const straddling = 'x'.repeat(cap - 1) + '\u{1F600}' + 'tail'; + const parsed = hookModule!.parseBridgeMessage({ + type: 'plannotator-bridge-selection', + text: straddling, + rect, + }) as { text: string }; + expect(parsed.text.length).toBe(cap - 1); + expect(parsed.text.endsWith('x')).toBe(true); + expect(/[\uD800-\uDBFF]$/.test(parsed.text)).toBe(false); + // A pair that fits entirely under the cap is untouched. + const fitting = 'y'.repeat(cap - 2) + '\u{1F600}'; + const kept = hookModule!.parseBridgeMessage({ + type: 'plannotator-bridge-selection', + text: fitting, + rect, + }) as { text: string }; + expect(kept.text).toBe(fitting); + }); + test('selection text is truncated at the parse boundary, not rejected', () => { // The page controls element text entirely, so one pinpoint click on a huge // 
 could otherwise ship an unbounded string into React state, drafts,
diff --git a/packages/ui/components/html-viewer/srcdoc.test.ts b/packages/ui/components/html-viewer/srcdoc.test.ts
index 66be108e22..b6bc4233fb 100644
--- a/packages/ui/components/html-viewer/srcdoc.test.ts
+++ b/packages/ui/components/html-viewer/srcdoc.test.ts
@@ -881,6 +881,50 @@ describe.if(hasDom)("bridge theme handler (DOM)", () => {
     document.body.replaceChildren();
   });
 
+  test("deeply nested targets get no anchor instead of a quadratic selector walk", async () => {
+    // Each ancestor step costs a document-wide uniqueness query against a
+    // growing selector, so unbounded depth freezes the tab on one click
+    // (measured ~58s at depth 800 pre-cap). Past MAX_ANCHOR_PATH_DEPTH the
+    // anchor is abandoned and restoration falls back to text search.
+    // Two structurally identical chains: every positional selector along the
+    // walk matches both branches, so uniqueness cannot short-circuit before
+    // the depth cap fires (the branch point sits above it).
+    const DEPTH = 60;
+    let chainA = "
Deeply buried text
";
+    let chainB = "
Other branch text
";
+    for (let i = 0; i < DEPTH; i++) {
+      chainA = `
${chainA}
`;
+      chainB = `
${chainB}
`;
+    }
+    document.body.innerHTML = chainA + chainB;
+    const target = document.querySelector("p");
+    if (!target || target.textContent !== "Deeply buried text") throw new Error("deep target missing");
+    postBridge({ type: "plannotator-bridge-set-input-method", method: "pinpoint" });
+
+    const messages: Array> = [];
+    const collect = (event: MessageEvent) => {
+      const data = bridgeMessageData(event);
+      if (data?.type === "plannotator-bridge-selection") messages.push(data);
+    };
+    window.addEventListener("message", collect);
+    const started = performance.now();
+    target.dispatchEvent(new MouseEvent("click", { bubbles: true, cancelable: true }));
+    const elapsed = performance.now() - started;
+    await new Promise((resolve) => setTimeout(resolve, 0));
+    window.removeEventListener("message", collect);
+
+    expect(messages.length).toBe(1);
+    expect(messages[0]!.text).toBe("Deeply buried text");
+    expect((messages[0] as { anchor?: unknown }).anchor).toBeUndefined();
+    // Bounded work: the capped walk must complete in interactive time even
+    // under happy-dom's slow selector engine.
+    expect(elapsed).toBeLessThan(2000);
+
+    postBridge({ type: "plannotator-bridge-cancel-selection" });
+    postBridge({ type: "plannotator-bridge-set-input-method", method: "drag" });
+    document.body.replaceChildren();
+  });
+
   test("behavioral-attribute anchors never bypass the text check", () => {
     // Regenerated page: the button kept its role but its meaning flipped; the
     // annotated text moved into a sibling paragraph. The role anchor must NOT
diff --git a/packages/ui/components/html-viewer/useHtmlAnnotation.ts b/packages/ui/components/html-viewer/useHtmlAnnotation.ts
index 1fed25c4d3..771390e46a 100644
--- a/packages/ui/components/html-viewer/useHtmlAnnotation.ts
+++ b/packages/ui/components/html-viewer/useHtmlAnnotation.ts
@@ -87,6 +87,16 @@ const MAX_ANCHOR_TEXT_LENGTH = 400;
 // MAX_SELECTION_TEXT in bridge-script.ts; this side is the authoritative one.
 export const MAX_SELECTION_TEXT_LENGTH = 10000;
 
+/** Truncate to the cap without ever splitting a UTF-16 surrogate pair (a
+ * lone high surrogate becomes U+FFFD once UTF-8-encoded downstream). */
+export function capSelectionText(text: string): string {
+  if (text.length <= MAX_SELECTION_TEXT_LENGTH) return text;
+  let cut = MAX_SELECTION_TEXT_LENGTH;
+  const last = text.charCodeAt(cut - 1);
+  if (last >= 0xd800 && last <= 0xdbff) cut -= 1;
+  return text.slice(0, cut);
+}
+
 /** Validate a bridge-posted element anchor. Exported for protocol tests. */
 export function parseHtmlElementAnchor(value: unknown): HtmlElementAnchor | null {
   if (!isRecord(value)) return null;
@@ -127,9 +137,7 @@ export function parseBridgeMessage(value: unknown): BridgeMessage | null {
       if (typeof value.text !== "string" || !rect) return null;
       return {
         type: value.type,
-        text: value.text.length > MAX_SELECTION_TEXT_LENGTH
-          ? value.text.slice(0, MAX_SELECTION_TEXT_LENGTH)
-          : value.text,
+        text: capSelectionText(value.text),
         rect,
         modeOverride: parseEditorMode(value.modeOverride),
         anchor: parseHtmlElementAnchor(value.anchor) ?? undefined,
diff --git a/scripts/install.cmd b/scripts/install.cmd
index 08b2b1d4ff..552ad1841c 100644
--- a/scripts/install.cmd
+++ b/scripts/install.cmd
@@ -1137,8 +1137,16 @@ if "!SKIP_SKILLS!"=="1" goto skills_checkout_done
 
 set "CLONE_OK=0"
 set "SPARSE_CLONE=1"
+REM LC_ALL=C pins git's error strings to English for the capability probe
+REM below: a localized git would emit a translated "unknown option" message
+REM the findstr match misses, sending old-git non-English users to a hard
+REM failure instead of the fallback. Saved and restored around the probe.
+set "PLANNOTATOR_SAVED_LC_ALL=!LC_ALL!"
+set "LC_ALL=C"
 git clone --depth 1 --filter=blob:none --sparse "https://github.com/!REPO!.git" --branch "!TAG!" "!SKILLS_TMP!\repo" >nul 2>"!GIT_ERR_FILE!"
 if !ERRORLEVEL! equ 0 set "CLONE_OK=1"
+set "LC_ALL=!PLANNOTATOR_SAVED_LC_ALL!"
+set "PLANNOTATOR_SAVED_LC_ALL="
 
 REM Capability probe, not a version parse (same philosophy as the GitButler
 REM flag probing in packages/shared/gitbutler-core.ts): `git clone --sparse`
diff --git a/scripts/install.ps1 b/scripts/install.ps1
index f93d62169c..6e2e232dfa 100644
--- a/scripts/install.ps1
+++ b/scripts/install.ps1
@@ -1065,7 +1065,13 @@ try {
     # $null (#1238) so failures can be diagnosed.
     $gitErrFile = Join-Path $skillsTmp "git-stderr.txt"
     if (-not $skipSkillsResolved) {
-        & { $local:ErrorActionPreference = 'Continue'; git clone --depth 1 --filter=blob:none --sparse "https://github.com/$repo.git" --branch $latestTag "$skillsTmp\repo" 2>$gitErrFile }
+        # LC_ALL=C pins git's error strings to English for the capability
+        # probe below: a localized git would emit a translated "unknown
+        # option" message the match misses, sending old-git non-English
+        # users to a hard failure instead of the fallback. Saved/restored
+        # around the call because $env: changes are process-wide. Kept on
+        # one line for the install.test.ts scoped-git-call scanner.
+        & { $local:ErrorActionPreference = 'Continue'; $prevLcAll = $env:LC_ALL; $env:LC_ALL = 'C'; git clone --depth 1 --filter=blob:none --sparse "https://github.com/$repo.git" --branch $latestTag "$skillsTmp\repo" 2>$gitErrFile; if ($null -eq $prevLcAll) { Remove-Item Env:LC_ALL -ErrorAction SilentlyContinue } else { $env:LC_ALL = $prevLcAll } }
         if (-not (Test-Path "$skillsTmp\repo")) {
             $cloneErr = ""
             if (Test-Path $gitErrFile) { $cloneErr = [System.IO.File]::ReadAllText($gitErrFile) }
diff --git a/scripts/install.sh b/scripts/install.sh
index b827ee8c63..95b8f8b51b 100644
--- a/scripts/install.sh
+++ b/scripts/install.sh
@@ -1638,7 +1638,12 @@ checkout_failed=0
         tail -n 5 "$git_err" >&2
     }
     sparse_clone=1
-    if ! git clone --depth 1 --filter=blob:none --sparse \
+    # LC_ALL=C pins git's error strings to English: the capability probe below
+    # matches the literal "unknown option ... sparse" text, and a localized
+    # git (standard Linux NLS builds) would otherwise emit a translated
+    # message the match misses, sending old-git non-English users to a hard
+    # failure instead of the fallback.
+    if ! LC_ALL=C LANGUAGE=C git clone --depth 1 --filter=blob:none --sparse \
         "https://github.com/${REPO}.git" --branch "$latest_tag" repo 2>"$git_err"; then
         # Capability probe, not a version parse (same philosophy as the
         # GitButler flag probing in packages/shared/gitbutler-core.ts):
diff --git a/scripts/install.test.ts b/scripts/install.test.ts
index 732361f85f..74225be0c8 100644
--- a/scripts/install.test.ts
+++ b/scripts/install.test.ts
@@ -1909,8 +1909,11 @@ describe("install shared behavior", () => {
     // on stderr, so without the scope the skill install failed on the line
     // announcing the clone had started. See #1162. Failure detection must
     // stay exit-code/Test-Path based, never throw-based.
+    // The sparse-probe clone also pins LC_ALL=C inside the same scoped
+    // block (the "unknown option" capability match is English-only), so its
+    // expected prefix carries the env save alongside the preference.
     expect(ps).toContain(
-      "& { $local:ErrorActionPreference = 'Continue'; git clone",
+      "& { $local:ErrorActionPreference = 'Continue'; $prevLcAll = $env:LC_ALL; $env:LC_ALL = 'C'; git clone",
     );
     expect(ps).toContain(
       "& { $local:ErrorActionPreference = 'Continue'; git sparse-checkout set",