Skip to content

Commit dfb9c4b

Browse files
cowhenclaude
andcommitted
Fix critical security and correctness issues in preview follow terminal
**P0 Fixes:** - Fix React hooks violation: move useEffect before conditional return in FollowTermDropdown - Remove broken shell type detection (cmd:shell is boolean, not shell path) **P1 Fixes:** - Fix command injection by removing vulnerable cmdEscapePath/powershellEscapePath - Revert to POSIX-only shell escaping (safe for bash/zsh/sh) - Fix race condition: remove timeout-based suppressBidir reset **P2 Fixes:** - Fix stale closure: make closeMenu a useCallback with proper dependencies - Fix linkTerm behavior: only reset bidir when linking to different terminal - Improve type safety: use proper types instead of any in followTermMenuDataAtom Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
1 parent df8483a commit dfb9c4b

2 files changed

Lines changed: 47 additions & 95 deletions

File tree

frontend/app/view/preview/preview-model.tsx

Lines changed: 13 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -514,28 +514,26 @@ export class PreviewModel implements ViewModel {
514514
this.followTermBidirAtom = atom<boolean>((get) => {
515515
return (get(this.blockAtom)?.meta?.["preview:followterm:bidir"] as boolean) ?? false;
516516
});
517-
this.followTermMenuDataAtom = atom(null) as PrimitiveAtom<{ pos: any; terms: any; currentFollowId: any; bidir: any } | null>;
517+
this.followTermMenuDataAtom = atom(null) as PrimitiveAtom<{ pos: { x: number; y: number }; terms: { blockId: string; title: string }[]; currentFollowId: string | null; bidir: boolean } | null>;
518518
}
519519

520520
showFollowTermMenu(e: React.MouseEvent<any>) {
521521
const tabData = globalStore.get(this.tabModel.tabAtom);
522522
const blockIds = tabData?.blockids ?? [];
523523

524-
const termBlockIds = blockIds
525-
.filter((bid) => {
526-
if (bid === this.blockId) return false;
527-
const block = WOS.getObjectValue<Block>(WOS.makeORef("block", bid), globalStore.get);
528-
return block?.meta?.view === "term";
529-
});
530-
531-
const terms = termBlockIds.map((bid) => {
524+
const terms: { blockId: string; title: string }[] = [];
525+
let termIndex = 1;
526+
for (const bid of blockIds) {
527+
if (bid === this.blockId) continue;
532528
const block = WOS.getObjectValue<Block>(WOS.makeORef("block", bid), globalStore.get);
533-
const termIndex = termBlockIds.indexOf(bid) + 1;
534-
return {
535-
blockId: bid,
536-
title: (block?.meta?.["frame:title"] as string) || `Terminal ${termIndex}`,
537-
};
538-
});
529+
if (block?.meta?.view === "term") {
530+
terms.push({
531+
blockId: bid,
532+
title: (block?.meta?.["frame:title"] as string) || `Terminal ${termIndex}`,
533+
});
534+
termIndex++;
535+
}
536+
}
539537

540538
const rect = (e.currentTarget as HTMLElement).getBoundingClientRect();
541539
const currentFollowId = globalStore.get(this.followTermIdAtom);

frontend/app/view/preview/preview.tsx

Lines changed: 34 additions & 80 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import { BlockModel } from "@/app/block/block-model";
1010
import * as WOS from "@/store/wos";
1111
import { fireAndForget, isBlank, makeConnRoute, stringToBase64 } from "@/util/util";
1212
import { useAtom, useAtomValue, useSetAtom } from "jotai";
13-
import { memo, useEffect, useRef } from "react";
13+
import React, { memo, useEffect, useRef } from "react";
1414
import ReactDOM from "react-dom";
1515
import { CSVView } from "./csvview";
1616
import { DirectoryPreview } from "./preview-directory";
@@ -21,61 +21,16 @@ import type { PreviewModel } from "./preview-model";
2121
import { StreamingPreview } from "./preview-streaming";
2222
import type { PreviewEnv } from "./previewenv";
2323

24-
function posixEscapePath(path: string): string {
24+
function shellEscapePath(path: string): string {
2525
if (path === "~") return "~";
2626
if (path.startsWith("~/")) {
2727
return "~/" + "'" + path.slice(2).replace(/'/g, "'\\''") + "'";
2828
}
2929
return "'" + path.replace(/'/g, "'\\''") + "'";
3030
}
3131

32-
function powershellEscapePath(path: string): string {
33-
if (path === "~") return "~";
34-
if (path.startsWith("~/")) {
35-
return "~/" + "'" + path.slice(2).replace(/'/g, "''") + "'";
36-
}
37-
return "'" + path.replace(/'/g, "''") + "'";
38-
}
39-
40-
function cmdEscapePath(path: string): string {
41-
if (path.startsWith("~/")) {
42-
return '"%USERPROFILE%\\' + path.slice(2).replace(/\//g, "\\") + '"';
43-
}
44-
if (path.includes(" ")) {
45-
return '"' + path.replace(/\//g, "\\") + '"';
46-
}
47-
return path.replace(/\//g, "\\");
48-
}
49-
50-
function getShellType(termBlockId: string): string {
51-
const termBlock = WOS.getObjectValue<Block>(WOS.makeORef("block", termBlockId), globalStore.get);
52-
const shellPath = (termBlock?.meta?.["cmd:shell"] as string) ?? "";
53-
if (shellPath.includes("powershell") || shellPath.includes("pwsh")) {
54-
return "powershell";
55-
}
56-
if (shellPath.includes("cmd.exe")) {
57-
return "cmd";
58-
}
59-
return "posix";
60-
}
61-
6232
async function sendCdToTerminal(termBlockId: string, path: string, env: import("./previewenv").PreviewEnv) {
63-
const shellType = getShellType(termBlockId);
64-
let command: string;
65-
66-
switch (shellType) {
67-
case "powershell":
68-
command = "cd " + powershellEscapePath(path) + "\r";
69-
break;
70-
case "cmd":
71-
command = "cd " + cmdEscapePath(path) + "\r";
72-
break;
73-
case "posix":
74-
default:
75-
command = "\x15cd " + posixEscapePath(path) + "\r";
76-
break;
77-
}
78-
33+
const command = "\x15cd " + shellEscapePath(path) + "\r";
7934
await env.rpc.ControllerInputCommand(TabRpcClient, { blockid: termBlockId, inputdata64: stringToBase64(command) });
8035
}
8136

@@ -162,23 +117,45 @@ function FollowTermDropdown({ model }: { model: PreviewModel }) {
162117
const menuRef = useRef<HTMLDivElement>(null);
163118
const previousActiveElement = useRef<Element | null>(null);
164119

165-
if (!menuData) return null;
166-
167-
const { pos, terms, currentFollowId, bidir } = menuData;
168-
const closeMenu = () => {
120+
const closeMenu = React.useCallback(() => {
169121
BlockModel.getInstance().setBlockHighlight(null);
170122
globalStore.set(model.followTermMenuDataAtom, null);
171123
if (previousActiveElement.current instanceof HTMLElement) {
172124
previousActiveElement.current.focus();
173125
}
174-
};
126+
}, [model.followTermMenuDataAtom]);
127+
128+
useEffect(() => {
129+
if (!menuData) return;
130+
previousActiveElement.current = document.activeElement;
131+
const handleEscape = (e: KeyboardEvent) => {
132+
if (e.key === "Escape") {
133+
closeMenu();
134+
}
135+
};
136+
document.addEventListener("keydown", handleEscape);
137+
if (menuRef.current) {
138+
const firstItem = menuRef.current.querySelector('[role="menuitem"]');
139+
if (firstItem instanceof HTMLElement) {
140+
firstItem.focus();
141+
}
142+
}
143+
return () => {
144+
document.removeEventListener("keydown", handleEscape);
145+
};
146+
}, [menuData, closeMenu]);
147+
148+
if (!menuData) return null;
149+
150+
const { pos, terms, currentFollowId, bidir } = menuData;
175151
const linkTerm = (blockId: string) => {
176152
BlockModel.getInstance().setBlockHighlight(null);
177153
fireAndForget(async () => {
178-
await model.env.services.object.UpdateObjectMeta(WOS.makeORef("block", model.blockId), {
179-
"preview:followtermid": blockId,
180-
"preview:followterm:bidir": false,
181-
});
154+
const updates: Record<string, any> = { "preview:followtermid": blockId };
155+
if (blockId !== currentFollowId) {
156+
updates["preview:followterm:bidir"] = false;
157+
}
158+
await model.env.services.object.UpdateObjectMeta(WOS.makeORef("block", model.blockId), updates);
182159
});
183160
globalStore.set(model.followTermMenuDataAtom, null);
184161
if (previousActiveElement.current instanceof HTMLElement) {
@@ -206,25 +183,6 @@ function FollowTermDropdown({ model }: { model: PreviewModel }) {
206183
}
207184
};
208185

209-
useEffect(() => {
210-
previousActiveElement.current = document.activeElement;
211-
const handleEscape = (e: KeyboardEvent) => {
212-
if (e.key === "Escape") {
213-
closeMenu();
214-
}
215-
};
216-
document.addEventListener("keydown", handleEscape);
217-
if (menuRef.current) {
218-
const firstItem = menuRef.current.querySelector('[role="menuitem"]');
219-
if (firstItem instanceof HTMLElement) {
220-
firstItem.focus();
221-
}
222-
}
223-
return () => {
224-
document.removeEventListener("keydown", handleEscape);
225-
};
226-
}, []);
227-
228186
const dropdownStyle: React.CSSProperties = {
229187
left: pos.x,
230188
top: pos.y,
@@ -352,10 +310,6 @@ function PreviewView({
352310
if (!followTermId || !followTermCwd) return;
353311
suppressBidirRef.current = true;
354312
fireAndForget(() => model.goHistory(followTermCwd));
355-
const timer = setTimeout(() => {
356-
suppressBidirRef.current = false;
357-
}, 100);
358-
return () => clearTimeout(timer);
359313
}, [followTermCwd, followTermId, model]);
360314

361315
useEffect(() => {

0 commit comments

Comments
 (0)