Skip to content

Commit 63dfc3a

Browse files
committed
fix: address Copilot review comments
- Use execFileSync with args array in toPosixPath to avoid shell injection - Add 5s timeout to app dispose to ensure quit even if disposal hangs - Log warning when SSH /home/ethan resolution fails and falls back to /tmp - Add random suffix to test markers to prevent collision
1 parent 13ed15c commit 63dfc3a

File tree

4 files changed

+22
-12
lines changed

4 files changed

+22
-12
lines changed

src/desktop/main.ts

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -577,14 +577,15 @@ if (gotTheLock) {
577577
event.preventDefault();
578578
isDisposing = true;
579579

580-
services
581-
.dispose()
582-
.catch((err) => {
583-
console.error("Error during ServiceContainer dispose:", err);
584-
})
585-
.finally(() => {
586-
app.quit();
587-
});
580+
// Race dispose against timeout to ensure app quits even if disposal hangs
581+
const disposePromise = services.dispose().catch((err) => {
582+
console.error("Error during ServiceContainer dispose:", err);
583+
});
584+
const timeoutPromise = new Promise<void>((resolve) => setTimeout(resolve, 5000));
585+
586+
void Promise.race([disposePromise, timeoutPromise]).finally(() => {
587+
app.quit();
588+
});
588589
});
589590

590591
app.on("window-all-closed", () => {

src/node/runtime/SSHRuntime.ts

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,15 @@ export class SSHRuntime implements Runtime {
106106

107107
if (dir === "~" || dir.startsWith("~/")) {
108108
const result = await execBuffered(this, 'echo "$HOME"', { cwd: "/", timeout: 10 });
109-
const home = result.exitCode === 0 && result.stdout.trim() ? result.stdout.trim() : "/tmp";
109+
let home: string;
110+
if (result.exitCode === 0 && result.stdout.trim()) {
111+
home = result.stdout.trim();
112+
} else {
113+
log.warn(
114+
`SSHRuntime: Failed to resolve $HOME (exitCode=${result.exitCode}). Falling back to /tmp.`
115+
);
116+
home = "/tmp";
117+
}
110118
dir = dir === "~" ? home : `${home}/${dir.slice(2)}`;
111119
}
112120

src/node/utils/paths.ts

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { execSync } from "child_process";
1+
import { execFileSync } from "child_process";
22

33
/**
44
* Convert a path to POSIX format for Git Bash on Windows.
@@ -11,7 +11,8 @@ export function toPosixPath(windowsPath: string): string {
1111
if (process.platform !== "win32") return windowsPath;
1212
try {
1313
// cygpath converts Windows paths to POSIX format for Git Bash / MSYS2
14-
return execSync(`cygpath -u "${windowsPath}"`, { encoding: "utf8" }).trim();
14+
// Use execFileSync with args array to avoid shell injection
15+
return execFileSync("cygpath", ["-u", windowsPath], { encoding: "utf8" }).trim();
1516
} catch {
1617
// Fallback if cygpath unavailable (shouldn't happen with Git Bash)
1718
return windowsPath;

tests/ipc/backgroundBash.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -278,7 +278,7 @@ describeIntegration("Background Bash Execution", () => {
278278

279279
try {
280280
// Start a background process that outputs a unique marker then exits
281-
const marker = `BGTEST_${Date.now()}`;
281+
const marker = `BGTEST_${Date.now()}_${Math.random().toString(36).slice(2)}`;
282282
const startEvents = await sendMessageAndWait(
283283
env,
284284
workspaceId,

0 commit comments

Comments
 (0)