diff --git a/src/commands/vault-sync.test.ts b/src/commands/vault-sync.test.ts index cbd6639..959735e 100644 --- a/src/commands/vault-sync.test.ts +++ b/src/commands/vault-sync.test.ts @@ -50,10 +50,24 @@ const makeDeps = (over: Partial = {}): { deps: VaultSyncDeps; log }; describe('runVaultSync', () => { - it('reports "no syncable origin" for an unknown vault name', async () => { - const { deps, logs } = makeDeps(); - await runVaultSync('missing', deps); - expect(logs.join()).toContain("No syncable origin configured for vault 'missing'"); + it('errors when the named vault is not registered', async () => { + const { deps } = makeDeps(); + await expect(runVaultSync('missing', deps)).rejects.toThrow("vault 'missing' not found"); + }); + + it('reports "no syncable origin" for a registered vault with no external origin', async () => { + const { deps, logs } = makeDeps({ + loadConfig: () => ({ version: 1, vaults: { local: { path: '/tmp/local' } } }), + }); + await runVaultSync('local', deps); + expect(logs.join()).toContain("No syncable origin configured for vault 'local'"); + }); + + it('prints an upfront count and a per-vault progress line', async () => { + const { deps, logs } = makeDeps({ daemonPort: async () => null }); + await runVaultSync('v', deps); + expect(logs).toContain('Syncing 1 vault(s)...'); + expect(logs).toContain('v...'); }); it('hints when no syncable vaults exist', async () => { diff --git a/src/commands/vault-sync.ts b/src/commands/vault-sync.ts index 732d396..b768d10 100644 --- a/src/commands/vault-sync.ts +++ b/src/commands/vault-sync.ts @@ -8,6 +8,7 @@ import { runSyncCycle, type SyncResult } from '../sync/cycle.js'; import { createCouchSyncManager, type CouchSyncResult } from '../sync/couch/manager.js'; import { couchTargets } from '../sync/couch/targets.js'; import { syncTargets, type SyncTarget } from '../sync/planner.js'; +import { redactRemoteUrl } from '../sync/remote-url.js'; export interface VaultSyncDeps { loadConfig: () => VaultsConfig; @@ -46,7 +47,7 @@ const report = (log: (msg: string) => void, r: SyncRunResult): void => { log(`${r.vault} (account): ${describeCouch(r)}`); return; } - log(`${r.vault} -> ${r.remote}: ${describeGit(r)}`); + log(`${r.vault} -> ${redactRemoteUrl(r.remote)}: ${describeGit(r)}`); for (const c of r.conflicts) log(` kept remote copy: ${c}`); }; @@ -59,6 +60,8 @@ export const runVaultSync = async ( deps: VaultSyncDeps ): Promise => { const config = deps.loadConfig(); + // A named vault that is not registered is an error (mirrors `vault remove`), not a silent no-op. + if (name && !(config.vaults ?? {})[name]) throw new Error(`vault '${name}' not found`); const gitTargets = syncTargets(config).filter((t) => !name || t.vault === name); const couchVaults = couchTargets(config) .filter((t) => !name || t.vault === name) @@ -71,14 +74,24 @@ export const runVaultSync = async ( ); return; } + const vaults = [...new Set([...gitTargets.map((t) => t.vault), ...couchVaults])]; + deps.log(`Syncing ${vaults.length} vault(s)...`); const port = await deps.daemonPort(); if (port !== null) { - const vaults = [...new Set([...gitTargets.map((t) => t.vault), ...couchVaults])]; - for (const vault of vaults) report(deps.log, await deps.runViaDaemon(port, vault)); + for (const vault of vaults) { + deps.log(`${vault}...`); + report(deps.log, await deps.runViaDaemon(port, vault)); + } return; } - for (const target of gitTargets) report(deps.log, await deps.runGitInProcess(target)); - for (const vault of couchVaults) report(deps.log, await deps.runCouchInProcess(vault)); + for (const target of gitTargets) { + deps.log(`${target.vault}...`); + report(deps.log, await deps.runGitInProcess(target)); + } + for (const vault of couchVaults) { + deps.log(`${vault}...`); + report(deps.log, await deps.runCouchInProcess(vault)); + } }; const resolveDaemonPort = async (): Promise => { diff --git a/src/commands/vault.test.ts b/src/commands/vault.test.ts index 8afe8c0..54556dd 100644 --- a/src/commands/vault.test.ts +++ b/src/commands/vault.test.ts @@ -61,6 +61,22 @@ describe('vault add', () => { expect(h.logs.join()).toContain('(git)'); }); + it('rejects an unsafe --git transport-helper remote', async () => { + const h = makeDeps(); + await expect(runVaultAdd('x', { git: 'ext::sh -c "id"' }, h.deps)).rejects.toThrow( + /unsafe git remote URL/ + ); + expect(h.get().vaults?.x).toBeUndefined(); + }); + + it('redacts credentials in the echoed --git remote (stores the real URL)', async () => { + const h = makeDeps(); + await runVaultAdd('work', { git: 'https://u:tok@h/w.git' }, h.deps); + expect(h.get().vaults?.work.origin?.[0]?.remote).toBe('https://u:tok@h/w.git'); + expect(h.logs.join()).toContain('https://u:***@h/w.git'); + expect(h.logs.join()).not.toContain('tok@'); + }); + it('with no flag registers an account vault, writes the entry, then provisions', async () => { const h = makeDeps(); await runVaultAdd('acct', {}, h.deps); @@ -205,4 +221,15 @@ describe('vault list', () => { expect(out.a!.type).toBe('local'); expect(out.acct!.type).toBe('account'); }); + + it('redacts credentials in --json origin remotes', async () => { + const h = makeDeps(); + await runVaultAdd('work', { git: 'https://u:tok@h/w.git' }, h.deps); + h.logs.length = 0; + runVaultList({ json: true }, h.deps); + expect(h.logs[0]).toContain('https://u:***@h/w.git'); + expect(h.logs[0]).not.toContain('tok@'); + // The on-disk value is untouched. + expect(h.get().vaults?.work.origin?.[0]?.remote).toBe('https://u:tok@h/w.git'); + }); }); diff --git a/src/commands/vault.ts b/src/commands/vault.ts index ce3b7c7..0b43481 100644 --- a/src/commands/vault.ts +++ b/src/commands/vault.ts @@ -6,6 +6,7 @@ import { appendDiscoverIgnore, ensureVaultDir, formatVaultLine, + redactEntry, removeVault, vaultType, } from '../lib/vault-registry.js'; @@ -15,6 +16,7 @@ import { type ProvisionResult, } from '../lib/provision.js'; import { loadVaultsConfig, mutateVaultsConfig, type LoadedVaults } from '../lib/vaults.js'; +import { assertSafeRemoteUrl, redactRemoteUrl } from '../sync/remote-url.js'; import { defaultVaultSyncDeps, runVaultSync } from './vault-sync.js'; export interface VaultDeps { @@ -49,7 +51,10 @@ const buildEntry = (name: string, opts: VaultAddOptions): VaultEntry => { throw new Error('--path applies only to an account vault (drop --local and --git)'); // A --git vault is a local working copy that syncs to an external git remote (path + origin); // the daemon commits/pushes and pulls it per its interval. - if (opts.git) return { path: `~/vaults/${name}`, origin: [{ remote: opts.git }], mcp: ['local'] }; + if (opts.git) { + assertSafeRemoteUrl(opts.git); + return { path: `~/vaults/${name}`, origin: [{ remote: opts.git }], mcp: ['local'] }; + } if (hasLocal) { const path = typeof opts.local === 'string' ? opts.local : `~/vaults/${name}`; return { path, mcp: ['local'] }; @@ -69,7 +74,8 @@ export const runVaultAdd = async ( if (entry.path) deps.ensureDir(entry.path); const kind = vaultType(entry); const where = entry.path ?? entry.origin?.[0]?.remote ?? ''; - const via = kind === 'git' && entry.origin?.length ? ` <- ${entry.origin[0]!.remote}` : ''; + const via = + kind === 'git' && entry.origin?.length ? ` <- ${redactRemoteUrl(entry.origin[0]!.remote)}` : ''; deps.log(chalk.green(`Added vault '${name}' (${kind}) -> ${where}${via}`)); // Offline-first: the local entry is saved first; provisioning the account channel is never fatal. if (isAccountVault(entry)) deps.log((await deps.provision(name)).message); @@ -105,8 +111,9 @@ export const runVaultList = (opts: { json?: boolean }, deps: VaultDeps = default const names = Object.keys(vaults); if (opts.json) { // Backward-compatible: still the name-keyed map, each entry annotated with its honest type. + // Origin remotes are redacted so a credentialed URL never leaks into machine output. const out = Object.fromEntries( - names.map((n) => [n, { ...vaults[n]!, type: vaultType(vaults[n]!) }]) + names.map((n) => [n, { ...redactEntry(vaults[n]!), type: vaultType(vaults[n]!) }]) ); deps.log(JSON.stringify(out, null, 2)); return; diff --git a/src/lib/vault-registry.test.ts b/src/lib/vault-registry.test.ts index 282a3ef..bc40fb8 100644 --- a/src/lib/vault-registry.test.ts +++ b/src/lib/vault-registry.test.ts @@ -9,6 +9,7 @@ import { appendDiscoverIgnore, ensureVaultDir, formatVaultLine, + redactEntry, removeVault, vaultType, } from './vault-registry.js'; @@ -137,6 +138,30 @@ describe('formatVaultLine', () => { expect(line).toContain('account'); expect(line).not.toContain('<- agentage'); }); + + it('redacts credentials in the echoed remote', () => { + const line = formatVaultLine('work', { origin: [{ remote: 'https://u:tok@h/w.git' }] }); + expect(line).toContain('https://u:***@h/w.git'); + expect(line).not.toContain('tok'); + }); +}); + +describe('redactEntry', () => { + it('redacts every origin remote without mutating the input', () => { + const entry = { + path: '/tmp/w', + origin: [{ remote: 'https://u:tok@h/w.git' }, { remote: 'git@h:me/w.git' }], + }; + const out = redactEntry(entry); + expect(out.origin?.[0]?.remote).toBe('https://u:***@h/w.git'); + expect(out.origin?.[1]?.remote).toBe('git@h:me/w.git'); + expect(entry.origin[0].remote).toBe('https://u:tok@h/w.git'); + }); + + it('returns origin-free entries as-is', () => { + const entry = { path: '/tmp/local' }; + expect(redactEntry(entry)).toEqual(entry); + }); }); describe('ensureVaultDir', () => { diff --git a/src/lib/vault-registry.ts b/src/lib/vault-registry.ts index d4b76b9..4bf17bd 100644 --- a/src/lib/vault-registry.ts +++ b/src/lib/vault-registry.ts @@ -7,6 +7,7 @@ import { type VaultEntry, type VaultsConfig, } from '@agentage/memory-core'; +import { redactRemoteUrl } from '../sync/remote-url.js'; import { isValidVaultName } from './vaults.schema.js'; // Offline registry operations over the unified vaults.json (object map keyed by name). No @@ -31,12 +32,23 @@ export const ensureVaultDir = (path: string): void => { export const formatVaultLine = (name: string, entry: VaultEntry): string => { const kind = vaultType(entry); - const where = entry.path ? expandPath(entry.path) : (entry.origin?.[0]?.remote ?? ''); + const rawWhere = entry.path ? expandPath(entry.path) : (entry.origin?.[0]?.remote ?? ''); + const where = entry.path ? rawWhere : redactRemoteUrl(rawWhere); // Only an external git remote is worth echoing; the account channel is implied by the type. - const remote = kind === 'git' && entry.origin?.length ? ` <- ${entry.origin[0]!.remote}` : ''; + const remote = + kind === 'git' && entry.origin?.length + ? ` <- ${redactRemoteUrl(entry.origin[0]!.remote)}` + : ''; return `${name.padEnd(16)} ${kind.padEnd(8)} ${where}${remote}`; }; +// A copy of `entry` with every origin remote redacted (credentials stripped) for display/JSON; +// the on-disk value is never mutated. +export const redactEntry = (entry: VaultEntry): VaultEntry => + entry.origin?.length + ? { ...entry, origin: entry.origin.map((o) => ({ ...o, remote: redactRemoteUrl(o.remote) })) } + : entry; + // Add an entry under `name`; the first vault added also becomes the `default`. export const addVault = (config: VaultsConfig, name: string, entry: VaultEntry): VaultsConfig => { if (!isValidVaultName(name)) throw new Error(`invalid vault name: ${JSON.stringify(name)}`); diff --git a/src/sync/cycle.test.ts b/src/sync/cycle.test.ts index cc415a9..a9a815f 100644 --- a/src/sync/cycle.test.ts +++ b/src/sync/cycle.test.ts @@ -57,6 +57,15 @@ describe('runSyncCycle', () => { expect(g(bare, ['show', 'main:notes/a.md'])).toContain('hello quokka'); }); + it('skips an unsafe transport-helper remote without touching git', async () => { + writeFile(work, 'a.md', 'x'); + const result = await runSyncCycle(target({ path: work, remote: 'ext::sh -c "id"' })); + expect(result.ok).toBe(true); + expect(result.skipped).toBe('invalid-remote'); + expect(result.pushed).toBe(false); + expect(existsSync(join(work, '.git'))).toBe(false); + }); + it('is a no-op on the second cycle (nothing to commit)', async () => { writeFile(work, 'a.md', 'x'); await runSyncCycle(target({ path: work, remote: bare })); diff --git a/src/sync/cycle.ts b/src/sync/cycle.ts index b964aae..6996152 100644 --- a/src/sync/cycle.ts +++ b/src/sync/cycle.ts @@ -4,6 +4,7 @@ import { dirname, join } from 'node:path'; import { conflictName } from './conflict.js'; import { createSyncGit, GitError, type GitErrorKind, type SyncGit } from './git-exec.js'; import { type SyncTarget } from './planner.js'; +import { isSafeRemoteUrl } from './remote-url.js'; export interface SyncResult { vault: string; @@ -12,7 +13,7 @@ export interface SyncResult { committed: boolean; // a sync commit was made for a dirty working tree pushed: boolean; conflicts: string[]; // paths written as `.conflict.md` - skipped?: 'lock' | 'busy'; + skipped?: 'lock' | 'busy' | 'invalid-remote'; reason?: GitErrorKind; error?: string; } @@ -143,6 +144,8 @@ export const runSyncCycle = async ( pushed: false, conflicts: [], }; + // Defense in depth: the planner already drops unsafe remotes, but never add/set-url one here. + if (!isSafeRemoteUrl(target.remote)) return { ...base, ok: true, skipped: 'invalid-remote' }; let committed = false; let conflicts: string[] = []; try { diff --git a/src/sync/planner.test.ts b/src/sync/planner.test.ts index 70694af..81e5b1a 100644 --- a/src/sync/planner.test.ts +++ b/src/sync/planner.test.ts @@ -1,4 +1,4 @@ -import { describe, expect, it } from 'vitest'; +import { describe, expect, it, vi } from 'vitest'; import { type VaultsConfig } from '@agentage/memory-core'; import { autoSyncTargets, @@ -78,6 +78,19 @@ describe('syncTargets', () => { expect(syncTargets(cfg({ v: { path: '/tmp/v', origin: [{ remote: ' ' }] } }))).toEqual([]); }); + it('skips an unsafe transport-helper remote and warns, keeping other targets', () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + const targets = syncTargets( + cfg({ + bad: { path: '/tmp/bad', origin: [{ remote: 'ext::sh -c "id"' }] }, + good: { path: '/tmp/good', origin: [{ remote: 'git@h:me/g.git' }] }, + }) + ); + expect(targets.map((t) => t.vault)).toEqual(['good']); + expect(warn).toHaveBeenCalledWith(expect.stringContaining("unsafe remote for vault 'bad'")); + warn.mockRestore(); + }); + it('never picks up an account (agentage-origin) vault, even with a local path', () => { const config = cfg({ acct: { path: '/tmp/acct', origin: [{ remote: 'agentage' }] }, diff --git a/src/sync/planner.ts b/src/sync/planner.ts index 8607f01..3c06552 100644 --- a/src/sync/planner.ts +++ b/src/sync/planner.ts @@ -1,4 +1,5 @@ import { expandPath, type VaultsConfig } from '@agentage/memory-core'; +import { isSafeRemoteUrl, redactRemoteUrl } from './remote-url.js'; // The unified schema stores interval as a bare non-negative integer; sync treats it as SECONDS // (there is no minutes marker in the schema, so seconds is the least-surprising reading and keeps @@ -39,6 +40,13 @@ export const syncTargets = (config: VaultsConfig): SyncTarget[] => { entry.origin.forEach((origin, index) => { const remote = origin.remote.trim(); if (!remote || remote === RESERVED_REMOTE) return; + // One poisoned origin must not run code or kill the whole cycle: skip it with a warning. + if (!isSafeRemoteUrl(remote)) { + console.warn( + `agentage: skipping unsafe remote for vault '${vault}': ${redactRemoteUrl(remote)}` + ); + return; + } out.push({ vault, path: expandPath(entry.path as string), diff --git a/src/sync/remote-url.test.ts b/src/sync/remote-url.test.ts new file mode 100644 index 0000000..9e6b39d --- /dev/null +++ b/src/sync/remote-url.test.ts @@ -0,0 +1,63 @@ +import { describe, expect, it } from 'vitest'; +import { assertSafeRemoteUrl, isSafeRemoteUrl, redactRemoteUrl } from './remote-url.js'; + +describe('isSafeRemoteUrl', () => { + it('allows conventional transports', () => { + expect(isSafeRemoteUrl('https://github.com/me/repo.git')).toBe(true); + expect(isSafeRemoteUrl('ssh://git@host/me/repo.git')).toBe(true); + expect(isSafeRemoteUrl('git://host/me/repo.git')).toBe(true); + expect(isSafeRemoteUrl(' https://host/repo.git ')).toBe(true); + }); + + it('allows scp-like user@host:path', () => { + expect(isSafeRemoteUrl('git@github.com:me/repo.git')).toBe(true); + expect(isSafeRemoteUrl('user@host:path/to/repo')).toBe(true); + }); + + it('allows credentialed https (redaction handles display)', () => { + expect(isSafeRemoteUrl('https://user:token@host/repo.git')).toBe(true); + }); + + it('rejects transport-helper URLs (arbitrary command execution)', () => { + expect(isSafeRemoteUrl('ext::sh -c "id"')).toBe(false); + expect(isSafeRemoteUrl('fd::17')).toBe(false); + expect(isSafeRemoteUrl('transport::address')).toBe(false); + expect(isSafeRemoteUrl('user@host::ext')).toBe(false); + }); + + it('rejects a URL that could be read as a git option', () => { + expect(isSafeRemoteUrl('-oProxyCommand=evil')).toBe(false); + }); + + it('allows local file transports (no command execution)', () => { + expect(isSafeRemoteUrl('/srv/git/repo.git')).toBe(true); + expect(isSafeRemoteUrl('file:///srv/git/repo.git')).toBe(true); + }); + + it('rejects blank and non-transport strings', () => { + expect(isSafeRemoteUrl('')).toBe(false); + expect(isSafeRemoteUrl(' ')).toBe(false); + expect(isSafeRemoteUrl('C:\\repo')).toBe(false); + }); +}); + +describe('assertSafeRemoteUrl', () => { + it('passes a safe URL and throws on an unsafe one', () => { + expect(() => assertSafeRemoteUrl('git@host:me/r.git')).not.toThrow(); + expect(() => assertSafeRemoteUrl('ext::sh -c "id"')).toThrow('unsafe git remote URL'); + }); +}); + +describe('redactRemoteUrl', () => { + it('redacts the password in a credentialed URL, keeping the user', () => { + expect(redactRemoteUrl('https://user:token@host/repo.git')).toBe( + 'https://user:***@host/repo.git' + ); + expect(redactRemoteUrl('ssh://git:secret@host/repo')).toBe('ssh://git:***@host/repo'); + }); + + it('leaves credential-free remotes untouched', () => { + expect(redactRemoteUrl('https://host/repo.git')).toBe('https://host/repo.git'); + expect(redactRemoteUrl('git@host:me/r.git')).toBe('git@host:me/r.git'); + }); +}); diff --git a/src/sync/remote-url.ts b/src/sync/remote-url.ts new file mode 100644 index 0000000..4c55ad7 --- /dev/null +++ b/src/sync/remote-url.ts @@ -0,0 +1,33 @@ +// Git treats a remote of the form `::
` as a transport helper (`ext::sh -c ...`, +// `fd::...`) and runs it as a command on fetch/push - arbitrary code execution if a poisoned +// vaults.json origin is auto-synced. Only conventional transports are allowed so no origin can +// ever execute code. A leading `-` is also refused (git could read it as an option). +// file:// and absolute local paths are safe transports too (a bare repo on disk/mount); unlike a +// transport helper they cannot run an arbitrary command. +const ALLOWED_SCHEMES = ['https://', 'ssh://', 'git://', 'file://']; + +// scp-like short syntax without a scheme: user@host:path. +const SCP_LIKE = /^[^\s/@]+@[^\s/:]+:.+$/; + +export const isSafeRemoteUrl = (remote: string): boolean => { + const url = remote.trim(); + if (!url || url.startsWith('-')) return false; + const lower = url.toLowerCase(); + if (ALLOWED_SCHEMES.some((s) => lower.startsWith(s))) return true; + if (url.includes('::')) return false; // transport-helper syntax (ext::, fd::, ::) + if (url.startsWith('/')) return true; // absolute local path + return SCP_LIKE.test(url); +}; + +export const assertSafeRemoteUrl = (remote: string): void => { + if (!isSafeRemoteUrl(remote)) + throw new Error( + `unsafe git remote URL ${JSON.stringify(remote.trim())} ` + + '(allowed: https://, ssh://, git://, file://, user@host:path, or an absolute local path)' + ); +}; + +// Redact `user:password@host` credentials for display; the stored value is left untouched. A bare +// `user@host` (scp-like, or scheme userinfo without a password) carries no secret and is kept. +export const redactRemoteUrl = (remote: string): string => + remote.replace(/(:\/\/[^/@]+?):[^/@]+@/, '$1:***@');