diff --git a/src/config.ts b/src/config.ts index ddb58c9..80a3db1 100644 --- a/src/config.ts +++ b/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 ` 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,6 +169,14 @@ 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, @@ -168,7 +184,7 @@ function parseSSHConfigInner( ): 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 diff --git a/src/index.ts b/src/index.ts index dc21cea..37ca9b1 100644 --- a/src/index.ts +++ b/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 { 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, diff --git a/src/monitor.ts b/src/monitor.ts index 8512d19..79aeaf4 100644 --- a/src/monitor.ts +++ b/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 ` 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 { +async function copyToClipboardMac(text: string): Promise { // 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((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 { - 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 { + if (isWindows || isWSL()) return copyToClipboardWindows(text) + if (isMac) return copyToClipboardMac(text) + if (isWaylandSession()) return copyToClipboardWayland(text) + return copyToClipboardX11(text) } async function processNewImage( @@ -670,8 +666,8 @@ 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`) } @@ -679,8 +675,8 @@ async function processNewImage( 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 { }) } + // 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 ` takes effect // without restarting the daemon. @@ -992,6 +999,8 @@ export async function startMonitor(initialRemote: string): Promise { await checkFileScreenshot() } catch (err) { log(`Error: ${err}`) + } finally { + pollInFlight = false } } diff --git a/test/config.test.ts b/test/config.test.ts index b4f3859..20b110d 100644 --- a/test/config.test.ts +++ b/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) + }) })