diff --git a/packages/web/server/__tests__/direct-terminal-ws.integration.test.ts b/packages/web/server/__tests__/direct-terminal-ws.integration.test.ts index ba10d6f5a..2cf132466 100644 --- a/packages/web/server/__tests__/direct-terminal-ws.integration.test.ts +++ b/packages/web/server/__tests__/direct-terminal-ws.integration.test.ts @@ -451,14 +451,14 @@ describe("hash-prefixed session resolution", () => { it("resolves hash-prefixed tmux session by suffix match", async () => { // Create a session that only exists with a hash prefix (no exact match) const hashOnlySession = `ao-hashtest-${process.pid}`; - const hashPrefixedName = `deadbeef-${hashOnlySession}`; + const hashPrefixedName = `deadbeef0123-${hashOnlySession}`; execFileSync(TMUX, ["new-session", "-d", "-s", hashPrefixedName, "-x", "80", "-y", "24"], { timeout: 5000 }); try { const ws = await connectWs(hashOnlySession); - // Should have resolved via suffix match and connected + // Should have resolved via hash-prefix match and connected const data = await waitForWsData(ws); expect(data.length).toBeGreaterThan(0); @@ -470,7 +470,7 @@ describe("hash-prefixed session resolution", () => { it("can send input through hash-resolved session", async () => { const hashOnlySession = `ao-hashcmd-${process.pid}`; - const hashPrefixedName = `cafebabe-${hashOnlySession}`; + const hashPrefixedName = `cafebabe0123-${hashOnlySession}`; execFileSync(TMUX, ["new-session", "-d", "-s", hashPrefixedName, "-x", "80", "-y", "24"], { timeout: 5000 }); @@ -491,7 +491,7 @@ describe("hash-prefixed session resolution", () => { it("uses user-facing ID (not hash name) as activeSessions key", async () => { const hashOnlySession = `ao-hashkey-${process.pid}`; - const hashPrefixedName = `face1234-${hashOnlySession}`; + const hashPrefixedName = `face12340000-${hashOnlySession}`; execFileSync(TMUX, ["new-session", "-d", "-s", hashPrefixedName, "-x", "80", "-y", "24"], { timeout: 5000 }); @@ -510,16 +510,16 @@ describe("hash-prefixed session resolution", () => { }); it("does NOT cross-match ao-1 to hash-ao-15 via prefix", async () => { - // Create "deadbeef-ao-test-15-PID" but NOT "ao-test-1-PID" + // Create "deadbeef0123-ao-test-15-PID" but NOT "ao-test-1-PID" // Connecting as "ao-test-1-PID" should fail (not match ao-test-15-PID) const session15 = `ao-crosstest-15-${process.pid}`; - const hashSession15 = `deadbeef-${session15}`; + const hashSession15 = `deadbeef01ab-${session15}`; const session1 = `ao-crosstest-1-${process.pid}`; execFileSync(TMUX, ["new-session", "-d", "-s", hashSession15, "-x", "80", "-y", "24"], { timeout: 5000 }); try { - // ao-crosstest-1-PID should NOT resolve to deadbeef-ao-crosstest-15-PID + // ao-crosstest-1-PID should NOT resolve to deadbeef01ab-ao-crosstest-15-PID const ws = new WebSocket(`ws://localhost:${port}/ws?session=${session1}`); const result = await waitForWsClose(ws); diff --git a/packages/web/server/__tests__/tmux-utils.test.ts b/packages/web/server/__tests__/tmux-utils.test.ts index ad8883c16..a4d3581a8 100644 --- a/packages/web/server/__tests__/tmux-utils.test.ts +++ b/packages/web/server/__tests__/tmux-utils.test.ts @@ -423,7 +423,7 @@ describe("resolveTmuxSession", () => { throw new Error("session not found"); }) .mockImplementationOnce(() => { - return "8474d6f29887-ao-15\n8474d6f29887-ao-16\nao-orchestrator\n"; + return "8474d6f29887-ao-15\na1b2c3d4e5f6-ao-16\nao-orchestrator\n"; }); const result = resolveTmuxSession("ao-15", TMUX, mockExec); @@ -431,8 +431,8 @@ describe("resolveTmuxSession", () => { expect(result).toBe("8474d6f29887-ao-15"); }); - it("uses endsWith for suffix matching (not contains)", () => { - // "ao-1" should NOT match "8474d6f29887-ao-15" because "-ao-15" doesn't end with "-ao-1" + it("uses exact match after hash prefix (not endsWith)", () => { + // "ao-1" should NOT match "8474d6f29887-ao-15" because substring(13) is "ao-15" not "ao-1" const mockExec = vi.fn() .mockImplementationOnce(() => { throw new Error("session not found"); @@ -446,6 +446,54 @@ describe("resolveTmuxSession", () => { expect(result).toBeNull(); }); + it("does NOT match ambiguous suffixes (bugbot: hash-my-app-1 vs app-1)", () => { + // This is the critical bugbot fix: "hash-my-app-1" ends with "-app-1" + // but the hash prefix is not a valid 12-char hex string, so it shouldn't match. + // A user looking up "app-1" should NOT be connected to "hash-my-app-1". + const mockExec = vi.fn() + .mockImplementationOnce(() => { + throw new Error("session not found"); + }) + .mockImplementationOnce(() => { + return "nonhexprefix-my-app-1\n8474d6f29887-app-1\n"; + }); + + // Should match the one with valid hash prefix, not the ambiguous one + expect(resolveTmuxSession("app-1", TMUX, mockExec)).toBe("8474d6f29887-app-1"); + }); + + it("rejects session names where hash prefix is not 12-char hex", () => { + const mockExec = vi.fn() + .mockImplementationOnce(() => { + throw new Error("session not found"); + }) + .mockImplementationOnce(() => { + // These look like hash-prefixed but aren't valid 12-char hex + return [ + "short-ao-15", // too short + "toolonghashprefix-ao-15", // too long + "ABCDEF123456-ao-15", // uppercase hex (not matching [a-f0-9]) + "zzzzzzzzzzzz-ao-15", // not hex chars + "8474d6f2988-ao-15", // 11 chars (one short) + "8474d6f29887a-ao-15", // 13 chars (one extra) + ].join("\n") + "\n"; + }); + + expect(resolveTmuxSession("ao-15", TMUX, mockExec)).toBeNull(); + }); + + it("only matches valid 12-char lowercase hex prefix", () => { + const mockExec = vi.fn() + .mockImplementationOnce(() => { + throw new Error("session not found"); + }) + .mockImplementationOnce(() => { + return "abcdef012345-ao-15\n"; + }); + + expect(resolveTmuxSession("ao-15", TMUX, mockExec)).toBe("abcdef012345-ao-15"); + }); + it("matches the correct session among many", () => { const mockExec = vi.fn() .mockImplementationOnce(() => { @@ -453,30 +501,30 @@ describe("resolveTmuxSession", () => { }) .mockImplementationOnce(() => { return [ - "abc123-ao-1", - "def456-ao-15", - "ghi789-ao-2", - "jkl012-ao-orchestrator", + "aabbccddeef0-ao-1", + "112233445566-ao-15", + "ffeeddccbbaa-ao-2", + "a0b1c2d3e4f5-ao-orchestrator", ].join("\n") + "\n"; }); - expect(resolveTmuxSession("ao-15", TMUX, mockExec)).toBe("def456-ao-15"); + expect(resolveTmuxSession("ao-15", TMUX, mockExec)).toBe("112233445566-ao-15"); }); - it("matches ao-1 to abc123-ao-1 (not ao-15)", () => { + it("matches ao-1 to hash-ao-1 (not hash-ao-15)", () => { const mockExec = vi.fn() .mockImplementationOnce(() => { throw new Error("session not found"); }) .mockImplementationOnce(() => { return [ - "abc123-ao-1", - "def456-ao-15", - "ghi789-ao-2", + "aabbccddeef0-ao-1", + "112233445566-ao-15", + "ffeeddccbbaa-ao-2", ].join("\n") + "\n"; }); - expect(resolveTmuxSession("ao-1", TMUX, mockExec)).toBe("abc123-ao-1"); + expect(resolveTmuxSession("ao-1", TMUX, mockExec)).toBe("aabbccddeef0-ao-1"); }); it("matches session with multiple hyphens in name", () => { @@ -485,11 +533,11 @@ describe("resolveTmuxSession", () => { throw new Error("session not found"); }) .mockImplementationOnce(() => { - return "abc123-my-long-session-name\ndef456-other-session\n"; + return "aabbccddeef0-my-long-session-name\n112233445566-other-session\n"; }); expect(resolveTmuxSession("my-long-session-name", TMUX, mockExec)) - .toBe("abc123-my-long-session-name"); + .toBe("aabbccddeef0-my-long-session-name"); }); it("matches session with underscores", () => { @@ -498,10 +546,10 @@ describe("resolveTmuxSession", () => { throw new Error("session not found"); }) .mockImplementationOnce(() => { - return "abc123-my_session\ndef456-other_session\n"; + return "aabbccddeef0-my_session\n112233445566-other_session\n"; }); - expect(resolveTmuxSession("my_session", TMUX, mockExec)).toBe("abc123-my_session"); + expect(resolveTmuxSession("my_session", TMUX, mockExec)).toBe("aabbccddeef0-my_session"); }); it("passes list-sessions format flag correctly", () => { @@ -522,8 +570,8 @@ describe("resolveTmuxSession", () => { ); }); - it("does NOT match if session name contains the ID but not as suffix", () => { - // e.g., "ao-15-extended" contains "ao-15" but doesn't end with "-ao-15" + it("does NOT match if session name contains the ID but not after hash prefix", () => { + // e.g., "ao-15-extended" has no valid hash prefix const mockExec = vi.fn() .mockImplementationOnce(() => { throw new Error("session not found"); @@ -535,6 +583,19 @@ describe("resolveTmuxSession", () => { expect(resolveTmuxSession("ao-15", TMUX, mockExec)).toBeNull(); }); + it("does NOT match hash-prefixed session with extra suffix", () => { + // "aabbccddeef0-ao-15-backup" has valid hash but substring(13) is "ao-15-backup" not "ao-15" + const mockExec = vi.fn() + .mockImplementationOnce(() => { + throw new Error("session not found"); + }) + .mockImplementationOnce(() => { + return "aabbccddeef0-ao-15-backup\n"; + }); + + expect(resolveTmuxSession("ao-15", TMUX, mockExec)).toBeNull(); + }); + it("returns first match when multiple hash-prefixed sessions exist for same ID", () => { // This shouldn't happen in practice, but test the behavior const mockExec = vi.fn() @@ -542,11 +603,11 @@ describe("resolveTmuxSession", () => { throw new Error("session not found"); }) .mockImplementationOnce(() => { - return "hash1-ao-15\nhash2-ao-15\n"; + return "aabbccddeef0-ao-15\n112233445566-ao-15\n"; }); // find() returns the first match - expect(resolveTmuxSession("ao-15", TMUX, mockExec)).toBe("hash1-ao-15"); + expect(resolveTmuxSession("ao-15", TMUX, mockExec)).toBe("aabbccddeef0-ao-15"); }); }); @@ -635,10 +696,10 @@ describe("resolveTmuxSession", () => { throw new Error("session not found"); }) .mockImplementationOnce(() => { - return "prefix-abcdef123456\n"; + return "aabbccddeef0-abcdef123456\n"; }); - expect(resolveTmuxSession("abcdef123456", TMUX, mockExec)).toBe("prefix-abcdef123456"); + expect(resolveTmuxSession("abcdef123456", TMUX, mockExec)).toBe("aabbccddeef0-abcdef123456"); }); it("handles single-char session ID", () => { @@ -647,23 +708,21 @@ describe("resolveTmuxSession", () => { throw new Error("session not found"); }) .mockImplementationOnce(() => { - return "hash-a\nhash-b\n"; + return "aabbccddeef0-a\n112233445566-b\n"; }); - expect(resolveTmuxSession("a", TMUX, mockExec)).toBe("hash-a"); + expect(resolveTmuxSession("a", TMUX, mockExec)).toBe("aabbccddeef0-a"); }); - it("does not match session that equals the ID without a hyphen prefix", () => { - // "ao-15" should not match "xao-15" (missing hyphen separator) + it("does not match session without valid hash prefix", () => { const mockExec = vi.fn() .mockImplementationOnce(() => { throw new Error("session not found"); }) .mockImplementationOnce(() => { - return "xao-15\n"; + return "xao-15\nnotahash-ao-15\n"; }); - // endsWith("-ao-15") would not match "xao-15" expect(resolveTmuxSession("ao-15", TMUX, mockExec)).toBeNull(); }); @@ -673,27 +732,29 @@ describe("resolveTmuxSession", () => { throw new Error("session not found"); }) .mockImplementationOnce(() => { - return "hash1-ao-15\r\nhash2-ao-16\r\n"; + return "aabbccddeef0-ao-15\r\n112233445566-ao-16\r\n"; }); - // \r will remain in the session name with current implementation - // This tests that the code is robust against this edge case - // The result may include \r but the important thing is it returns something - const result = resolveTmuxSession("ao-15", TMUX, mockExec); - // With .filter(Boolean) and .split("\n"), "hash1-ao-15\r" won't match "-ao-15" + // \r will remain in the session name after split("\n") + // substring(13) of "aabbccddeef0-ao-15\r" is "ao-15\r" which !== "ao-15" // This documents the current behavior — tmux shouldn't produce \r\n on unix - expect(result).toBeNull(); + expect(resolveTmuxSession("ao-15", TMUX, mockExec)).toBeNull(); }); it("handles very long session list", () => { - const sessions = Array.from({ length: 100 }, (_, i) => `hash${i}-session-${i}`).join("\n") + "\n"; + // Generate 100 sessions with valid 12-char hex prefixes + const sessions = Array.from({ length: 100 }, (_, i) => { + const hex = i.toString(16).padStart(12, "0"); + return `${hex}-session-${i}`; + }).join("\n") + "\n"; const mockExec = vi.fn() .mockImplementationOnce(() => { throw new Error("session not found"); }) .mockImplementationOnce(() => sessions); - expect(resolveTmuxSession("session-50", TMUX, mockExec)).toBe("hash50-session-50"); + const hex50 = (50).toString(16).padStart(12, "0"); + expect(resolveTmuxSession("session-50", TMUX, mockExec)).toBe(`${hex50}-session-50`); }); it("handles session list where target is last entry", () => { @@ -702,10 +763,10 @@ describe("resolveTmuxSession", () => { throw new Error("session not found"); }) .mockImplementationOnce(() => { - return "hash1-ao-1\nhash2-ao-2\nhash3-ao-3\nhash4-ao-target\n"; + return "aabbccddeef0-ao-1\n112233445566-ao-2\nffeeddccbbaa-ao-3\na0b1c2d3e4f5-ao-target\n"; }); - expect(resolveTmuxSession("ao-target", TMUX, mockExec)).toBe("hash4-ao-target"); + expect(resolveTmuxSession("ao-target", TMUX, mockExec)).toBe("a0b1c2d3e4f5-ao-target"); }); it("handles session list where target is first entry", () => { @@ -714,10 +775,10 @@ describe("resolveTmuxSession", () => { throw new Error("session not found"); }) .mockImplementationOnce(() => { - return "hash1-ao-target\nhash2-ao-2\nhash3-ao-3\n"; + return "aabbccddeef0-ao-target\n112233445566-ao-2\nffeeddccbbaa-ao-3\n"; }); - expect(resolveTmuxSession("ao-target", TMUX, mockExec)).toBe("hash1-ao-target"); + expect(resolveTmuxSession("ao-target", TMUX, mockExec)).toBe("aabbccddeef0-ao-target"); }); it("works with different tmux paths", () => { diff --git a/packages/web/server/tmux-utils.ts b/packages/web/server/tmux-utils.ts index 8e5aa1790..6a476fbe1 100644 --- a/packages/web/server/tmux-utils.ts +++ b/packages/web/server/tmux-utils.ts @@ -10,6 +10,9 @@ import { execFileSync } from "node:child_process"; /** Session ID validation regex — alphanumeric, hyphens, underscores only */ export const SESSION_ID_PATTERN = /^[a-zA-Z0-9_-]+$/; +/** Hash prefix pattern — 12-char lowercase hex, as generated by generateConfigHash */ +const HASH_PREFIX_PATTERN = /^[a-f0-9]{12}-/; + /** * Validate a session ID format. * Prevents path traversal, shell injection, and other attacks. @@ -54,8 +57,8 @@ export function findTmux( * * 1. Tries exact match first using tmux's `=` prefix syntax to prevent * prefix matching (where "ao-1" would incorrectly match "ao-15"). - * 2. Falls back to listing all sessions and finding one that ends with - * `-{sessionId}` (hash-prefixed match). + * 2. Falls back to listing all sessions and finding one with a 12-char hex + * prefix followed by the exact session ID (e.g., `{12-hex}-{sessionId}`). * * @param sessionId - User-facing session ID (e.g., "ao-15") * @param tmuxPath - Full path to tmux binary @@ -77,13 +80,17 @@ export function resolveTmuxSession( } // Search for hash-prefixed tmux session (e.g., "8474d6f29887-ao-15" for "ao-15") + // Validate the 12-char hex prefix to avoid ambiguous suffix matches where + // "hash-my-app-1" could falsely match a lookup for "app-1". try { const output = execFn(tmuxPath, ["list-sessions", "-F", "#{session_name}"], { timeout: 5000, encoding: "utf8", }) as string; const sessions = output.split("\n").filter(Boolean); - const match = sessions.find((s) => s.endsWith(`-${sessionId}`)); + const match = sessions.find((s) => + HASH_PREFIX_PATTERN.test(s) && s.substring(13) === sessionId, + ); if (match) { return match; }