fix: round-L audit (parser comments, BSD pgrep, poll guard, log rotation)

src/config.ts

@@ -16,19 +16,27 @@ export interface Config {
   activeTarget?: string
 }
 
-// Reject remote names that ssh would interpret as option flags. A user-typed
-// `-oProxyCommand=…` slipping through to `spawn('ssh', [name, …])` would let
-// ssh take that arg as flags and execute the embedded command. The spawn
-// sites also use a `--` separator now, but rejecting at input gives a clear
-// error and stops typos like `-myhost` from creating a non-functional entry.
+// Allowed character set for remote names: alphanumerics plus `.`, `_`, `-`,
+// `@`. The leading character must NOT be `-` so ssh can't take it as an
+// option flag (`-oProxyCommand=…`) — even with the `--` spawn-separator
+// in monitor.ts, rejecting at config-input time gives a clear UI error
+// instead of an unparseable saved entry. Covers everything ssh accepts as
+// a hostname or user@host expression (including IPv4 dotted-quads and
+// config-file aliases like `prod-1`). An IPv6 literal in user@host form
+// would need brackets — not currently supported by the picker; users with
+// v6-only hosts can put them in `~/.ssh/config` under a name and
+// reference the alias.
+//
+// The whitelist is stricter than necessary because no current code path
+// interpolates a `remote` value into a remote shell string — pipeToRemote
+// and getHomeDir pass it as a positional spawn arg with `--` separator.
+// But a future refactor that builds an ssh-side command from `remote`
+// (e.g. a `sshshot logs <remote>` feature) would inherit the whitelist
+// as defense in depth. Cheaper to enforce now than to remember later.
+const VALID_REMOTE_NAME_RE = /^[A-Za-z0-9._@][A-Za-z0-9._@-]*$/
+
 export function isValidRemoteName(name: string): boolean {
-  if (name.length === 0) return false
-  if (name.startsWith('-')) return false
-  // No whitespace. Internal hyphens are fine — hostnames like prod-1, db-2
-  // are common. The interactive prompt that feeds this is single-line, so
-  // control-character checks are not needed here.
-  if (/\s/.test(name)) return false
-  return true
+  return VALID_REMOTE_NAME_RE.test(name)
 }
 
 export function getConfigDir(): string {
@@ -161,14 +169,22 @@ export function parseSSHConfig(content: string, loader?: SSHConfigIncludeLoader)
   return parseSSHConfigInner(content, loader ?? (() => []), 0)
 }
 
+// OpenSSH treats `#` as starting a line comment iff preceded by whitespace
+// OR at start of line. A `#` inside a token (e.g. a hostname `prod#qa`)
+// is part of the value, not a comment. Strip from there to end-of-line.
+// Exported so tests can pin the exact tokenizer behavior.
+export function stripSshConfigComment(line: string): string {
+  return line.replace(/(^|\s)#.*$/, '$1').trimEnd()
+}
+
 function parseSSHConfigInner(
   content: string,
   loader: SSHConfigIncludeLoader,
   depth: number
 ): SSHHost[] {
   if (depth > MAX_INCLUDE_DEPTH) return []
 
-  const lines = content.split('\n')
+  const lines = content.split('\n').map(stripSshConfigComment)
   const hosts: SSHHost[] = []
   // currentBlock is a list of host objects sharing one Host directive — all
   // are mutated together when we see HostName / User children. Match blocks

src/index.ts

@@ -120,7 +120,11 @@ function findSshshotProcesses(): ProcessInfo[] {
         }
       }
     } else {
-      const result = execSync("pgrep -af 'node.*[s]shshot.*--daemon'", { encoding: 'utf8' })
+      // -lf prints "PID full-cmdline" — works on both BSD pgrep (macOS,
+      // FreeBSD) and GNU/procps pgrep (Linux). The previous flag combo
+      // `-af` is GNU-only; on BSD `-a` is silently ignored, which made
+      // pgrep emit just PIDs and dropped the target-name extraction below.
+      const result = execSync("pgrep -lf 'node.*[s]shshot.*--daemon'", { encoding: 'utf8' })
       for (const line of result.trim().split('\n').filter(Boolean)) {
         const pid = parseInt(line.split(/\s+/)[0])
         if (!isNaN(pid)) {
@@ -172,6 +176,33 @@ function getVersion(): string {
   return pkg.version
 }
 
+// daemon.log captures the daemon's stdout+stderr — anything escaping the
+// monitor's own structured logger lands here. The internal logs in
+// monitor.ts rotate hourly + prune after 7 days; this one was previously
+// append-only forever. Rotate on each start when the file passes a size
+// cap so a recurring stderr error (failing ssh, missing xclip, etc.) can't
+// silently grow the file unbounded over months.
+const DAEMON_LOG_MAX_BYTES = 5 * 1024 * 1024 // 5 MB
+function rotateDaemonLogIfTooLarge(logDir: string): void {
+  const current = path.join(logDir, 'daemon.log')
+  let stat: fs.Stats
+  try {
+    stat = fs.statSync(current)
+  } catch {
+    return // doesn't exist yet
+  }
+  if (stat.size < DAEMON_LOG_MAX_BYTES) return
+  const archived = path.join(logDir, 'daemon.log.1')
+  try {
+    fs.rmSync(archived, { force: true })
+    fs.renameSync(current, archived)
+  } catch {
+    // best-effort — if rotation fails (permissions, race), the daemon will
+    // just append to the existing file and the cap effectively becomes
+    // higher this round. No reason to abort startup over it.
+  }
+}
+
 async function addRemotes(existing: string[]): Promise<string[]> {
   const remotes = [...existing]
 
@@ -244,6 +275,7 @@ function startBackground(remote: string): void {
     // processes that the daemon spawns per poll.
     const logDir = path.join(os.homedir(), '.config', 'sshshot', 'logs')
     fs.mkdirSync(logDir, { recursive: true })
+    rotateDaemonLogIfTooLarge(logDir)
     const logFd = fs.openSync(path.join(logDir, 'daemon.log'), 'a')
     child = spawn(process.execPath, [__filename, '--daemon', remote], {
       detached: true,

src/monitor.ts

@@ -596,64 +596,60 @@ async function pipeToRemote(
   })
 }
 
-function copyToClipboardWindows(text: string): void {
-  // Spawn the target binary with array args and pipe text via stdin — no
-  // shell, no quote-escaping, no shell-injection surface. Same pattern we
-  // use for pbcopy on macOS.
+// Each copy-to-clipboard helper now returns success-bool so the caller can
+// log accurately. Previously a failing xclip (no X11 display, missing
+// binary) silently returned and the daemon logged "-> Copied to clipboard"
+// regardless. Users would paste and get the previous clipboard contents.
+function copyToClipboardWindows(text: string): boolean {
   if (isWindows) {
-    // PowerShell's pipeline reads from stdin via $input
-    spawnSync(
+    const result = spawnSync(
       'powershell',
       ['-NoProfile', '-WindowStyle', 'Hidden', '-Command', '$input | Set-Clipboard'],
       { input: text, timeout: 2000, windowsHide: true }
     )
-  } else {
-    // WSL: clip.exe reads stdin directly, no echo needed
-    spawnSync('clip.exe', [], { input: text, timeout: 2000 })
+    return result.status === 0
   }
+  // WSL: clip.exe reads stdin directly, no echo needed
+  const result = spawnSync('clip.exe', [], { input: text, timeout: 2000 })
+  return result.status === 0
 }
 
-function copyToClipboardX11(text: string): void {
+function copyToClipboardX11(text: string): boolean {
   // xclip -selection clipboard reads text from stdin when no `-i <file>` is
   // given. Spawn with array args; no shell, no quote-escaping needed.
-  spawnSync('xclip', ['-selection', 'clipboard'], { input: text, timeout: 2000 })
+  const result = spawnSync('xclip', ['-selection', 'clipboard'], {
+    input: text,
+    timeout: 2000
+  })
+  return result.status === 0
 }
 
-function copyToClipboardWayland(text: string): void {
+function copyToClipboardWayland(text: string): boolean {
   // wl-copy reads stdin and copies to the clipboard. Spawn with array args;
   // no shell, no escaping. --trim-newline strips the trailing newline most
   // shells would append; we never feed one but it's defensive.
-  spawnSync('wl-copy', ['--trim-newline'], { input: text, timeout: 2000 })
+  const result = spawnSync('wl-copy', ['--trim-newline'], { input: text, timeout: 2000 })
+  return result.status === 0
 }
 
-async function copyToClipboardMac(text: string): Promise<void> {
+async function copyToClipboardMac(text: string): Promise<boolean> {
   // Spawn pbcopy and pipe text via stdin — avoids the macOS /bin/echo `-n` bug
   // (the shell builtin honors -n but the binary doesn't, so `echo -n` would
   // copy `-n FILE_PATH` into the clipboard) and skips shell-escaping entirely.
-  return new Promise((resolve) => {
+  return new Promise<boolean>((resolve) => {
     const proc = spawn('pbcopy')
-    const finish = () => resolve()
-    proc.on('error', finish)
-    proc.on('close', finish)
+    proc.on('error', () => resolve(false))
+    proc.on('close', (code) => resolve(code === 0))
     proc.stdin.write(text)
     proc.stdin.end()
   })
 }
 
-async function copyToClipboard(text: string): Promise<void> {
-  if (isWindows || isWSL()) {
-    copyToClipboardWindows(text)
-    return
-  }
-  if (isMac) {
-    await copyToClipboardMac(text)
-    return
-  }
-  if (isWaylandSession()) {
-    copyToClipboardWayland(text)
-    return
-  }
-  copyToClipboardX11(text)
+async function copyToClipboard(text: string): Promise<boolean> {
+  if (isWindows || isWSL()) return copyToClipboardWindows(text)
+  if (isMac) return copyToClipboardMac(text)
+  if (isWaylandSession()) return copyToClipboardWayland(text)
+  return copyToClipboardX11(text)
 }
 
 async function processNewImage(
@@ -670,17 +666,17 @@ async function processNewImage(
     const result = saveLocal(imageBuffer, filename)
     if (result.success) {
       log(`  -> Saved: ${result.path}`)
-      await copyToClipboard(result.path)
-      log(`  -> Copied to clipboard`)
+      const copied = await copyToClipboard(result.path)
+      log(copied ? `  -> Copied to clipboard` : `  -> Clipboard write failed`)
     } else {
       log(`  -> Failed to save locally`)
     }
   } else {
     const result = await pipeToRemote(imageBuffer, remote, filename)
     if (result.success) {
       log(`  -> Sent to ${remote}:${result.path}`)
-      await copyToClipboard(result.path)
-      log(`  -> Copied to clipboard`)
+      const copied = await copyToClipboard(result.path)
+      log(copied ? `  -> Copied to clipboard` : `  -> Clipboard write failed`)
     } else {
       log(`  -> Failed to send to ${remote}`)
       if (result.error) {
@@ -964,8 +960,19 @@ export async function startMonitor(initialRemote: string): Promise<void> {
     })
   }
 
+  // Re-entrancy guard: if a poll iteration takes longer than POLL_INTERVAL_MS
+  // (e.g. a slow ssh upload, a hanging PowerShell on Windows), setInterval
+  // would otherwise fire a second poll on top of the first — concurrent
+  // clipboard reads spawn duplicate xclip/PowerShell processes, and the
+  // upload semaphore can be flooded by the same image being submitted twice
+  // before the first lap finishes updating lastClipboardHash. The guard
+  // makes the loop self-throttling: a long iteration just delays the next
+  // tick rather than stacking work.
+  let pollInFlight = false
+
   const poll = async () => {
-    if (shutdownRequested) return
+    if (shutdownRequested || pollInFlight) return
+    pollInFlight = true
     try {
       // Re-resolve target each cycle so `sshshot target <name>` takes effect
       // without restarting the daemon.
@@ -992,6 +999,8 @@ export async function startMonitor(initialRemote: string): Promise<void> {
       await checkFileScreenshot()
     } catch (err) {
       log(`Error: ${err}`)
+    } finally {
+      pollInFlight = false
     }
   }
 

test/config.test.ts

@@ -170,6 +170,37 @@ Include selfref
     expect(result.every((h) => h.name === 'a')).toBe(true)
   })
 
+  it('strips inline `# comment` from Host lines (no bogus aliases)', () => {
+    const input = `Host prod # primary production box
+  HostName 1.2.3.4
+  User ops # ops account
+`
+    expect(parseSSHConfig(input)).toEqual([{ name: 'prod', hostname: '1.2.3.4', user: 'ops' }])
+  })
+
+  it('preserves a `#` that is part of a token (no leading whitespace)', () => {
+    // OpenSSH only treats `#` as a comment when preceded by whitespace OR
+    // at start of line. `prod#qa` is a single hostname token.
+    const input = `Host prod#qa
+  HostName q.example.com
+`
+    expect(parseSSHConfig(input)).toEqual([{ name: 'prod#qa', hostname: 'q.example.com' }])
+  })
+
+  it('strips trailing comments on Include lines', () => {
+    const input = `Include corp_config # imported from corp
+`
+    const seen: string[] = []
+    const loader = (spec: string): string[] => {
+      seen.push(spec)
+      return []
+    }
+    parseSSHConfig(input, loader)
+    // The previous parser would have asked the loader for `corp_config`,
+    // `#`, and `imported`/`from`/`corp` — three bogus filesystem lookups.
+    expect(seen).toEqual(['corp_config'])
+  })
+
   it('skips Match blocks (no Host names emitted from them)', () => {
     const input = `Host plain
   HostName plain.example.com
@@ -202,4 +233,14 @@ describe('isValidRemoteName', () => {
     expect(isValidRemoteName('host\twith\ttab')).toBe(false)
     expect(isValidRemoteName('host\nwith\nnewline')).toBe(false)
   })
+
+  it('rejects shell metacharacters (defense-in-depth whitelist)', () => {
+    expect(isValidRemoteName('myhost;touch/tmp/evil')).toBe(false)
+    expect(isValidRemoteName('host&&rm')).toBe(false)
+    expect(isValidRemoteName('host|nc')).toBe(false)
+    expect(isValidRemoteName('host`whoami`')).toBe(false)
+    expect(isValidRemoteName('host$(id)')).toBe(false)
+    expect(isValidRemoteName("host'OR'1=1")).toBe(false)
+    expect(isValidRemoteName('host/etc/passwd')).toBe(false)
+  })
 })