fix: round-K audit follow-ups (input hardening + config robustness)

src/config.ts

@@ -16,6 +16,21 @@ 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.
+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
+}
+
 export function getConfigDir(): string {
   return path.join(os.homedir(), '.config', 'sshshot')
 }
@@ -39,11 +54,33 @@ export function getPidFile(): string {
 let lastReadErrorMessage: string | null = null
 let lastParseErrorMessage: string | null = null
 
+// mtime-gated config cache — the daemon's poll loop calls loadConfig 5x/sec
+// just to pick up `sshshot target <name>` changes, but the file rarely
+// actually changes. Stat-and-compare keeps disk reads + JSON.parse cost
+// near zero on the steady-state hot path; the actual read+parse only fires
+// when mtimeMs differs from the previous successful load. Saving via
+// saveConfig invalidates the cache (sets cachedConfigMtime back to null).
+let cachedConfig: Config | null = null
+let cachedConfigMtime: number | null = null
+
 export function loadConfig(): Config | null {
   const configPath = getConfigPath()
-  if (!fs.existsSync(configPath)) {
+
+  // Stat first. If the file is gone, drop the cache and return null. If the
+  // mtime matches what we last loaded, return the cached value without
+  // re-reading or re-parsing.
+  let stat: fs.Stats
+  try {
+    stat = fs.statSync(configPath)
+  } catch {
+    cachedConfig = null
+    cachedConfigMtime = null
     return null
   }
+  if (cachedConfig !== null && cachedConfigMtime === stat.mtimeMs) {
+    return cachedConfig
+  }
+
   let content: string
   try {
     content = fs.readFileSync(configPath, 'utf-8')
@@ -55,12 +92,12 @@ export function loadConfig(): Config | null {
     }
     return null
   }
-  // Successful read — clear any prior read-error suppression so a future
-  // failure with the same message does log again.
   lastReadErrorMessage = null
   try {
     const parsed = JSON.parse(content) as Config
     lastParseErrorMessage = null
+    cachedConfig = parsed
+    cachedConfigMtime = stat.mtimeMs
     return parsed
   } catch (err) {
     // The previous behavior was to throw, which crashed the daemon's poll
@@ -75,6 +112,9 @@ export function loadConfig(): Config | null {
       )
       lastParseErrorMessage = msg
     }
+    // Don't cache a parse failure — every subsequent stat will re-attempt
+    // until the user fixes the file, which is what we want for a transient
+    // mid-write read.
     return null
   }
 }
@@ -86,21 +126,56 @@ export function saveConfig(config: Config): void {
     fs.mkdirSync(configDir, { recursive: true })
   }
   fs.writeFileSync(configPath, JSON.stringify(config, null, 2) + '\n')
+  // Invalidate the load cache so the next loadConfig() picks up the value
+  // we just wrote (in case mtime resolution rounded equal — rare on macOS,
+  // where APFS gives ns-resolution stat, but cheap insurance).
+  cachedConfig = null
+  cachedConfigMtime = null
 }
 
+// Loader signature for Include directives. Given a raw path spec (which may
+// be relative, contain `~`, or a basic `*`/`?` glob), return the contents
+// of every file it resolves to. Empty array on no matches / unreadable.
+// Tests inject a deterministic loader; the real loader uses fs.
+export type SSHConfigIncludeLoader = (pathSpec: string) => string[]
+
+// OpenSSH's documented Include nesting limit. A misconfigured include cycle
+// will short-circuit at this depth instead of stack-overflowing the parser.
+const MAX_INCLUDE_DEPTH = 16
+
 // Pure parser, exported for tests. Takes the raw text of an ssh_config and
 // returns the Host blocks (excluding wildcard patterns).
 //
 // `Host a b c` declares three aliases sharing the same block — we emit one
 // SSHHost per alias so all of them show up in the auto-detect picker. The
 // previous parser dropped b and c entirely, so users with shorthand aliases
 // like `Host prod-1 prod` had to type the long form by hand.
-export function parseSSHConfig(content: string): SSHHost[] {
+//
+// `Include <pathspec>` directives recursively pull in additional config
+// files. Most modern setups (especially corp/devbox boxes that ship a
+// `~/.ssh/config.d/*` drop-in dir) put almost all hosts in includes; without
+// this support, first-run auto-detect appeared empty. `Match` blocks are
+// skipped — they don't introduce new Host names, only conditionally apply
+// to existing ones.
+export function parseSSHConfig(content: string, loader?: SSHConfigIncludeLoader): SSHHost[] {
+  return parseSSHConfigInner(content, loader ?? (() => []), 0)
+}
+
+function parseSSHConfigInner(
+  content: string,
+  loader: SSHConfigIncludeLoader,
+  depth: number
+): SSHHost[] {
+  if (depth > MAX_INCLUDE_DEPTH) return []
+
   const lines = content.split('\n')
   const hosts: SSHHost[] = []
   // currentBlock is a list of host objects sharing one Host directive — all
-  // are mutated together when we see HostName / User children.
+  // are mutated together when we see HostName / User children. Match blocks
+  // null this out so subsequent HostName/User lines don't leak into a Host
+  // block that already finished.
   let currentBlock: SSHHost[] | null = null
+  let inMatchBlock = false
 
   const flush = () => {
     if (currentBlock) {
@@ -111,11 +186,11 @@ export function parseSSHConfig(content: string): SSHHost[] {
 
   for (const line of lines) {
     const trimmed = line.trim()
-    if (trimmed.toLowerCase().startsWith('host ')) {
+    const lower = trimmed.toLowerCase()
+
+    if (lower.startsWith('host ')) {
       flush()
-      // Split on whitespace, drop wildcard patterns. A line like
-      // `Host * !bad ok` would yield a block with just { name: 'ok' } —
-      // we accept the non-wildcard aliases and ignore the rest.
+      inMatchBlock = false
       const aliases = trimmed
         .slice(5)
         .trim()
@@ -124,11 +199,30 @@ export function parseSSHConfig(content: string): SSHHost[] {
       if (aliases.length > 0) {
         currentBlock = aliases.map((name) => ({ name }))
       }
-    } else if (currentBlock) {
-      if (trimmed.toLowerCase().startsWith('hostname ')) {
+    } else if (lower.startsWith('match ')) {
+      // Skip the entire Match block — close the previous Host so its
+      // HostName/User children don't get inherited.
+      flush()
+      inMatchBlock = true
+    } else if (lower.startsWith('include ')) {
+      flush()
+      inMatchBlock = false
+      // Multiple path specs can appear on one line, whitespace-separated.
+      const specs = trimmed
+        .slice(8)
+        .trim()
+        .split(/\s+/)
+        .filter((s) => s.length > 0)
+      for (const spec of specs) {
+        for (const includedContent of loader(spec)) {
+          hosts.push(...parseSSHConfigInner(includedContent, loader, depth + 1))
+        }
+      }
+    } else if (currentBlock && !inMatchBlock) {
+      if (lower.startsWith('hostname ')) {
         const hostname = trimmed.slice(9).trim()
         for (const h of currentBlock) h.hostname = hostname
-      } else if (trimmed.toLowerCase().startsWith('user ')) {
+      } else if (lower.startsWith('user ')) {
         const user = trimmed.slice(5).trim()
         for (const h of currentBlock) h.user = user
       }
@@ -139,6 +233,58 @@ export function parseSSHConfig(content: string): SSHHost[] {
   return hosts
 }
 
+// Real-world Include loader: resolves the path spec against the filesystem,
+// expanding `~` and treating relative paths as anchored at `~/.ssh/`. Basic
+// glob expansion (`*`, `?`) is supported in the basename only — sufficient
+// for the canonical `Include ~/.ssh/config.d/*` pattern. Unreadable files
+// are silently skipped (the user's .ssh dir might have permission quirks).
+export function makeFsIncludeLoader(home: string): SSHConfigIncludeLoader {
+  return (spec: string): string[] => {
+    let resolved = spec
+    if (resolved.startsWith('~/')) {
+      resolved = path.join(home, resolved.slice(2))
+    }
+    if (!path.isAbsolute(resolved)) {
+      resolved = path.join(home, '.ssh', resolved)
+    }
+
+    let matches: string[]
+    if (resolved.includes('*') || resolved.includes('?')) {
+      const dir = path.dirname(resolved)
+      const pattern = path.basename(resolved)
+      const re = globToRegExp(pattern)
+      try {
+        matches = fs
+          .readdirSync(dir)
+          .filter((name) => re.test(name))
+          .map((name) => path.join(dir, name))
+      } catch {
+        matches = []
+      }
+    } else {
+      matches = [resolved]
+    }
+
+    const contents: string[] = []
+    for (const file of matches) {
+      try {
+        contents.push(fs.readFileSync(file, 'utf-8'))
+      } catch {
+        // include target missing or unreadable; skip
+      }
+    }
+    return contents
+  }
+}
+
+function globToRegExp(pattern: string): RegExp {
+  const escaped = pattern
+    .replace(/[.+^${}()|[\]\\]/g, '\\$&')
+    .replace(/\*/g, '.*')
+    .replace(/\?/g, '.')
+  return new RegExp(`^${escaped}$`)
+}
+
 export function detectSSHRemotes(): SSHHost[] {
   const home = os.homedir()
   const sshConfigPath = path.join(home, '.ssh', 'config')
@@ -147,5 +293,5 @@ export function detectSSHRemotes(): SSHHost[] {
     return []
   }
 
-  return parseSSHConfig(fs.readFileSync(sshConfigPath, 'utf-8'))
+  return parseSSHConfig(fs.readFileSync(sshConfigPath, 'utf-8'), makeFsIncludeLoader(home))
 }

src/index.ts

@@ -4,7 +4,14 @@ import * as fs from 'fs'
 import * as path from 'path'
 import * as os from 'os'
 import { spawn, spawnSync, execSync } from 'child_process'
-import { Config, getPidFile, loadConfig, saveConfig, detectSSHRemotes } from './config'
+import {
+  Config,
+  detectSSHRemotes,
+  getPidFile,
+  isValidRemoteName,
+  loadConfig,
+  saveConfig
+} from './config'
 import { promptConfirm, promptSelect, promptInput, promptMultiSelect } from './prompts'
 import { startMonitor } from './monitor'
 
@@ -191,9 +198,18 @@ async function addRemotes(existing: string[]): Promise<string[]> {
   let addMore = await promptConfirm('Add a custom SSH remote?')
   while (addMore) {
     const remoteName = await promptInput('Enter SSH remote (e.g., user@host)')
-    if (remoteName && !remotes.includes(remoteName)) {
-      remotes.push(remoteName)
-      console.log(`Added: ${remoteName}`)
+    if (remoteName) {
+      if (!isValidRemoteName(remoteName)) {
+        // Block names that ssh would parse as flags (`-oProxyCommand=…`),
+        // names with whitespace, and control chars. Empty input is also
+        // covered by isValidRemoteName.
+        console.log(
+          `Rejected: ${JSON.stringify(remoteName)} (must not start with '-' or contain whitespace)`
+        )
+      } else if (!remotes.includes(remoteName)) {
+        remotes.push(remoteName)
+        console.log(`Added: ${remoteName}`)
+      }
     }
     addMore = await promptConfirm('Add another?')
   }