Harden external navigation and cross-platform path handling (#167)

src/main/git/status.test.ts

@@ -1,4 +1,5 @@
 import { beforeEach, describe, expect, it, vi } from 'vitest'
+import path from 'path'
 
 const { execFileAsyncMock, readFileMock, rmMock } = vi.hoisted(() => ({
   execFileAsyncMock: vi.fn(),
@@ -19,7 +20,7 @@ vi.mock('fs/promises', () => ({
   rm: rmMock
 }))
 
-import { discardChanges } from './status'
+import { discardChanges, isWithinWorktree } from './status'
 
 describe('discardChanges', () => {
   beforeEach(() => {
@@ -75,4 +76,8 @@ describe('discardChanges', () => {
     expect(execFileAsyncMock).not.toHaveBeenCalled()
     expect(rmMock).not.toHaveBeenCalled()
   })
+
+  it('accepts in-tree Windows paths when resolving containment', async () => {
+    expect(isWithinWorktree(path.win32, 'C:\\repo', 'C:\\repo\\src\\file.ts')).toBe(true)
+  })
 })

src/main/git/status.ts

@@ -1,7 +1,7 @@
 import { execFile } from 'child_process'
 import { readFile, rm } from 'fs/promises'
 import { promisify } from 'util'
-import { join, resolve } from 'path'
+import * as path from 'path'
 import type { GitStatusEntry, GitFileStatus, GitDiffResult } from '../../shared/types'
 
 const execFileAsync = promisify(execFile)
@@ -127,7 +127,7 @@ export async function getDiff(
     } else {
       // Unstaged: modified is the working tree version
       try {
-        modifiedContent = await readFile(join(worktreePath, filePath), 'utf-8')
+        modifiedContent = await readFile(path.join(worktreePath, filePath), 'utf-8')
       } catch {
         modifiedContent = ''
       }
@@ -163,9 +163,9 @@ export async function unstageFile(worktreePath: string, filePath: string): Promi
  * Discard working tree changes for a file.
  */
 export async function discardChanges(worktreePath: string, filePath: string): Promise<void> {
-  const resolvedWorktree = resolve(worktreePath)
-  const resolvedTarget = resolve(worktreePath, filePath)
-  if (!resolvedTarget.startsWith(`${resolvedWorktree}/`)) {
+  const resolvedWorktree = path.resolve(worktreePath)
+  const resolvedTarget = path.resolve(worktreePath, filePath)
+  if (!isWithinWorktree(path, resolvedWorktree, resolvedTarget)) {
     throw new Error(`Path "${filePath}" resolves outside the worktree`)
   }
 
@@ -187,3 +187,17 @@ export async function discardChanges(worktreePath: string, filePath: string): Pr
       })
     : rm(resolvedTarget, { force: true, recursive: true }))
 }
+
+export function isWithinWorktree(
+  pathApi: Pick<typeof path, 'isAbsolute' | 'relative' | 'sep'>,
+  resolvedWorktree: string,
+  resolvedTarget: string
+): boolean {
+  const relativeTarget = pathApi.relative(resolvedWorktree, resolvedTarget)
+  return !(
+    relativeTarget === '' ||
+    relativeTarget === '..' ||
+    relativeTarget.startsWith(`..${pathApi.sep}`) ||
+    pathApi.isAbsolute(relativeTarget)
+  )
+}

src/main/hooks.test.ts

@@ -7,6 +7,14 @@ vi.mock('fs', () => ({
   existsSync: vi.fn()
 }))
 
+const { execMock } = vi.hoisted(() => ({
+  execMock: vi.fn()
+}))
+
+vi.mock('child_process', () => ({
+  exec: execMock
+}))
+
 describe('parseOrcaYaml', () => {
   it('parses YAML with setup script only', () => {
     const yaml = `scripts:\n  setup: |\n    echo "setting up"\n    npm install\n`
@@ -161,3 +169,113 @@ describe('getEffectiveHooks', () => {
     expect(result).toBeNull()
   })
 })
+
+describe('runHook', () => {
+  const makeRepo = (hookSettings?: {
+    mode: 'auto' | 'override'
+    scripts: { setup: string; archive: string }
+  }) => ({
+    id: 'test-id',
+    path: '/test/repo',
+    displayName: 'Test Repo',
+    badgeColor: '#000',
+    addedAt: Date.now(),
+    hookSettings
+  })
+
+  it('uses the Windows command shell when running hooks', async () => {
+    execMock.mockImplementation((_script, _options, callback) => {
+      callback?.(null, '', '')
+      return {} as never
+    })
+
+    const originalPlatform = process.platform
+    const originalComSpec = process.env.ComSpec
+
+    Object.defineProperty(process, 'platform', {
+      configurable: true,
+      value: 'win32'
+    })
+    process.env.ComSpec = 'C:\\Windows\\System32\\cmd.exe'
+
+    try {
+      const { runHook } = await import('./hooks')
+      const result = await runHook(
+        'setup',
+        'C:\\repo\\worktree',
+        makeRepo({
+          mode: 'override',
+          scripts: { setup: 'echo hello', archive: '' }
+        })
+      )
+
+      expect(result).toEqual({ success: true, output: '' })
+      expect(execMock).toHaveBeenCalledWith(
+        'echo hello',
+        expect.objectContaining({
+          cwd: 'C:\\repo\\worktree',
+          shell: 'C:\\Windows\\System32\\cmd.exe'
+        }),
+        expect.any(Function)
+      )
+    } finally {
+      Object.defineProperty(process, 'platform', {
+        configurable: true,
+        value: originalPlatform
+      })
+      if (originalComSpec === undefined) {
+        delete process.env.ComSpec
+      } else {
+        process.env.ComSpec = originalComSpec
+      }
+    }
+  })
+
+  it('keeps bash as the hook shell on non-Windows platforms', async () => {
+    execMock.mockImplementation((_script, _options, callback) => {
+      callback?.(null, '', '')
+      return {} as never
+    })
+
+    const originalPlatform = process.platform
+    const originalShell = process.env.SHELL
+
+    Object.defineProperty(process, 'platform', {
+      configurable: true,
+      value: 'linux'
+    })
+    process.env.SHELL = '/opt/homebrew/bin/fish'
+
+    try {
+      const { runHook } = await import('./hooks')
+      const result = await runHook(
+        'setup',
+        '/repo/worktree',
+        makeRepo({
+          mode: 'override',
+          scripts: { setup: 'echo hello', archive: '' }
+        })
+      )
+
+      expect(result).toEqual({ success: true, output: '' })
+      expect(execMock).toHaveBeenCalledWith(
+        'echo hello',
+        expect.objectContaining({
+          cwd: '/repo/worktree',
+          shell: '/bin/bash'
+        }),
+        expect.any(Function)
+      )
+    } finally {
+      Object.defineProperty(process, 'platform', {
+        configurable: true,
+        value: originalPlatform
+      })
+      if (originalShell === undefined) {
+        delete process.env.SHELL
+      } else {
+        process.env.SHELL = originalShell
+      }
+    }
+  })
+})

src/main/hooks.ts

@@ -7,6 +7,14 @@ import type { OrcaHooks, Repo } from '../shared/types'
 const HOOK_TIMEOUT = 120_000 // 2 minutes
 type HookName = keyof OrcaHooks['scripts']
 
+function getHookShell(): string | undefined {
+  if (process.platform === 'win32') {
+    return process.env.ComSpec || 'cmd.exe'
+  }
+
+  return '/bin/bash'
+}
+
 /**
  * Parse a simple orca.yaml file. Handles only the `scripts:` block with
  * multiline string values (YAML block scalar `|`).
@@ -138,7 +146,7 @@ export function runHook(
       {
         cwd,
         timeout: HOOK_TIMEOUT,
-        shell: '/bin/bash',
+        shell: getHookShell(),
         env: {
           ...process.env,
           ORCA_ROOT_PATH: repo.path,

src/main/window/createMainWindow.test.ts

@@ -31,7 +31,7 @@ describe('createMainWindow', () => {
     openExternalMock.mockReset()
   })
 
-  it('enables renderer sandboxing and only opens http(s) URLs externally', () => {
+  it('enables renderer sandboxing and opens external links safely', () => {
     const windowHandlers: Record<string, (...args: any[]) => void> = {}
     const webContents = {
       on: vi.fn((event, handler) => {
@@ -64,10 +64,35 @@ describe('createMainWindow', () => {
     )
 
     expect(windowHandlers.windowOpen({ url: 'https://example.com' })).toEqual({ action: 'deny' })
+    expect(windowHandlers.windowOpen({ url: 'localhost:3000' })).toEqual({ action: 'deny' })
     expect(windowHandlers.windowOpen({ url: 'file:///etc/passwd' })).toEqual({ action: 'deny' })
     expect(windowHandlers.windowOpen({ url: 'not a url' })).toEqual({ action: 'deny' })
 
-    expect(openExternalMock).toHaveBeenCalledTimes(1)
+    expect(openExternalMock).toHaveBeenCalledTimes(2)
     expect(openExternalMock).toHaveBeenCalledWith('https://example.com')
+    expect(openExternalMock).toHaveBeenCalledWith('http://localhost:3000/')
+
+    const preventDefault = vi.fn()
+    windowHandlers['will-navigate']({ preventDefault } as never, 'https://example.com/docs')
+    expect(preventDefault).toHaveBeenCalledTimes(1)
+    expect(openExternalMock).toHaveBeenCalledTimes(3)
+    expect(openExternalMock).toHaveBeenLastCalledWith('https://example.com/docs')
+
+    const localhostPreventDefault = vi.fn()
+    windowHandlers['will-navigate'](
+      { preventDefault: localhostPreventDefault } as never,
+      'localhost:3000'
+    )
+    expect(localhostPreventDefault).toHaveBeenCalledTimes(1)
+    expect(openExternalMock).toHaveBeenCalledTimes(4)
+    expect(openExternalMock).toHaveBeenLastCalledWith('http://localhost:3000/')
+
+    const fileNavigationPreventDefault = vi.fn()
+    windowHandlers['will-navigate'](
+      { preventDefault: fileNavigationPreventDefault } as never,
+      'file:///etc/passwd'
+    )
+    expect(fileNavigationPreventDefault).toHaveBeenCalledTimes(1)
+    expect(openExternalMock).toHaveBeenCalledTimes(4)
   })
 })

src/main/window/createMainWindow.ts

@@ -5,6 +5,26 @@ import icon from '../../../resources/icon.png?asset'
 import devIcon from '../../../resources/icon-dev.png?asset'
 import type { Store } from '../persistence'
 
+const LOCAL_ADDRESS_PATTERN =
+  /^(?:localhost|127(?:\.\d{1,3}){3}|0\.0\.0\.0|\[[0-9a-f:]+\])(?::\d+)?(?:\/.*)?$/i
+
+function normalizeExternalUrl(rawUrl: string): string | null {
+  if (LOCAL_ADDRESS_PATTERN.test(rawUrl)) {
+    try {
+      return new URL(`http://${rawUrl}`).toString()
+    } catch {
+      return null
+    }
+  }
+
+  try {
+    const parsed = new URL(rawUrl)
+    return parsed.protocol === 'https:' || parsed.protocol === 'http:' ? rawUrl : null
+  } catch {
+    return null
+  }
+}
+
 export function createMainWindow(store: Store | null): BrowserWindow {
   const mainWindow = new BrowserWindow({
     width: 1200,
@@ -33,21 +53,36 @@ export function createMainWindow(store: Store | null): BrowserWindow {
   })
 
   mainWindow.webContents.setWindowOpenHandler((details) => {
-    try {
-      const parsed = new URL(details.url)
-      if (parsed.protocol === 'https:' || parsed.protocol === 'http:') {
-        shell.openExternal(details.url)
-      }
-    } catch {
-      // ignore malformed URLs
+    const externalUrl = normalizeExternalUrl(details.url)
+    if (externalUrl) {
+      shell.openExternal(externalUrl)
     }
     return { action: 'deny' }
   })
 
+  // Block ALL in-window navigations to prevent remote pages from inheriting
+  // the privileged preload bridge (PTY, filesystem, etc.).
+  // In dev mode, allow navigations to the local dev server (e.g. HMR reloads).
   mainWindow.webContents.on('will-navigate', (event, url) => {
-    if (url.startsWith('file://')) {
-      event.preventDefault()
+    const externalUrl = normalizeExternalUrl(url)
+
+    if (externalUrl) {
+      const target = new URL(externalUrl)
+      if (is.dev && process.env.ELECTRON_RENDERER_URL) {
+        try {
+          const allowed = new URL(process.env.ELECTRON_RENDERER_URL)
+          if (target.origin === allowed.origin) {
+            return // allow dev server navigations (HMR, etc.)
+          }
+        } catch {
+          // fall through to prevent
+        }
+      }
+
+      shell.openExternal(externalUrl)
     }
+
+    event.preventDefault()
   })
 
   mainWindow.webContents.on('before-input-event', (event, input) => {