From d3a948f0d30ed5a90431a40620a21e5f36d1d5bc Mon Sep 17 00:00:00 2001 From: Akshar Patel Date: Mon, 24 Aug 2026 17:34:28 -0400 Subject: [PATCH] fix: close sync safety edge cases --- README.md | 2 +- src/cli.js | 2 +- src/core/check.js | 10 ++++- src/core/device.js | 36 ++++++++++++---- src/core/transaction.js | 7 +++- test/core.test.js | 93 +++++++++++++++++++++++++++++++++++++++++ 6 files changed, 137 insertions(+), 13 deletions(-) diff --git a/README.md b/README.md index 704ab92..d79bb40 100644 --- a/README.md +++ b/README.md @@ -366,7 +366,7 @@ Restore the most recent successful local projection apply in an emergency: skillsync rollback ``` -The next sync applies the current vault assignments again. +Rollback restores files only. It does not change assignments or target settings. Validate vault structure, registry hashes, JSON files, symlinks, and common credential formats without changing the vault: diff --git a/src/cli.js b/src/cli.js index e7f3799..07f5219 100755 --- a/src/cli.js +++ b/src/cli.js @@ -1813,7 +1813,7 @@ async function rollbackCommand(rest = []) { deviceId: config.deviceId, }); console.log(`Restored ${result.restored} skill projection${result.restored === 1 ? '' : 's'} from ${result.backupId}.`); - console.log('The next sync will apply the current vault assignments again.'); + console.log('Rollback restores files only; it does not change assignments or target settings.'); } async function doctor() { diff --git a/src/core/check.js b/src/core/check.js index f74f48a..a2e4e6f 100644 --- a/src/core/check.js +++ b/src/core/check.js @@ -4,7 +4,8 @@ import path from 'node:path'; import { assertSafePathSegment, hashDirectory } from './fs.js'; -const IGNORED_ROOT_ENTRIES = new Set(['.git', '.skillsync-local']); +const GIT_ENTRY = '.git'; +const LOCAL_STATE_ENTRY = '.skillsync-local'; const OWNERSHIP_MARKER = '.skillsync-owned.json'; const SECRET_PATTERNS = [ ['private key', /-----BEGIN (?:RSA |EC |OPENSSH )?PRIVATE KEY-----/], @@ -144,11 +145,16 @@ export async function checkVault(vaultPath, { verifyRegistry = true } = {}) { throw validationError('Vault check', [`Vault is not a directory: ${root}`]); } + const gitBacked = Boolean(await pathInfo(path.join(root, GIT_ENTRY))); + const localState = await pathInfo(path.join(root, LOCAL_STATE_ENTRY)); const inspected = await inspectTree(root, { - ignoreRootEntries: IGNORED_ROOT_ENTRIES, + ignoreRootEntries: new Set([GIT_ENTRY, LOCAL_STATE_ENTRY]), rejectOwnershipMarkers: true, }); const errors = [...inspected.errors]; + if (gitBacked && localState) { + errors.push(`Reserved local state path in Git-backed vault: ${LOCAL_STATE_ENTRY}`); + } const unsafeTree = inspected.errors.some((error) => ( error.startsWith('Symlinks are not allowed:') || error.startsWith('Unsupported file type:') diff --git a/src/core/device.js b/src/core/device.js index 96a2776..f1b247e 100644 --- a/src/core/device.js +++ b/src/core/device.js @@ -927,7 +927,9 @@ export async function planLinks({ }) { const device = await loadLocalDevice(vaultPath, deviceId); const registry = providedRegistry || await loadRegistry(vaultPath); - const approvedReplacements = new Set(replaceUnmanagedPaths.map((targetPath) => path.resolve(targetPath))); + const approvedReplacements = new Set(await Promise.all( + replaceUnmanagedPaths.map((targetPath) => canonicalTargetRoot(targetPath)), + )); const desiredByTarget = new Map(); for (const [skillName, targets] of Object.entries(device.installed)) { assertSafePathSegment(skillName, 'Skill name'); @@ -942,7 +944,8 @@ export async function planLinks({ const targetGroups = new Map(); for (const [targetName, targetConfig] of Object.entries(device.targets)) { const targetPath = path.resolve(expandHome(targetConfig.path)); - const existing = targetGroups.get(targetPath); + const canonicalPath = await canonicalTargetRoot(targetPath); + const existing = targetGroups.get(canonicalPath); if (existing && existing.mode !== targetConfig.mode) { throw new Error(`Targets sharing ${targetPath} must use the same projection mode`); } @@ -954,12 +957,12 @@ export async function planLinks({ }; group.targetNames.push(targetName); for (const skillName of desiredByTarget.get(targetName) || []) group.desired.add(skillName); - targetGroups.set(targetPath, group); + targetGroups.set(canonicalPath, group); } - const targetRoots = [...targetGroups.keys()].sort(); - for (const [index, root] of targetRoots.entries()) { - if (targetRoots.some((candidate, candidateIndex) => ( + const canonicalRoots = [...targetGroups.keys()].sort(); + for (const [index, root] of canonicalRoots.entries()) { + if (canonicalRoots.some((candidate, candidateIndex) => ( candidateIndex !== index && root.startsWith(candidate + path.sep) ))) { throw new Error(`Configured skill targets cannot overlap: ${root}`); @@ -1007,7 +1010,7 @@ export async function planLinks({ if (info.exists && !info.ownedSymlink && !info.ownedCopy - && !approvedReplacements.has(path.resolve(destination))) { + && !approvedReplacements.has(await canonicalTargetRoot(destination))) { throw new Error(`Refusing to overwrite unmanaged target path: ${destination}`); } const sourceHash = registry.skills[skillName].hash; @@ -1069,11 +1072,28 @@ export async function planLinks({ operations.sort((left, right) => left.destination.localeCompare(right.destination)); return { deviceId, - roots: targetRoots, + roots: [...new Set(Object.values(device.targets) + .map((target) => path.resolve(expandHome(target.path))))].sort(), operations, }; } +async function canonicalTargetRoot(targetPath) { + const suffix = []; + let current = path.resolve(targetPath); + while (true) { + try { + return path.join(await realpath(current), ...suffix); + } catch (error) { + if (error.code !== 'ENOENT') throw error; + const parent = path.dirname(current); + if (parent === current) return path.resolve(targetPath); + suffix.unshift(path.basename(current)); + current = parent; + } + } +} + export async function applyLinks(options) { const roots = await approvedProjectionRoots(options.vaultPath, options.deviceId || defaultDeviceId()); let preparedPlan = null; diff --git a/src/core/transaction.js b/src/core/transaction.js index 2eb655f..2ef0529 100644 --- a/src/core/transaction.js +++ b/src/core/transaction.js @@ -449,10 +449,15 @@ export async function rollbackLatestFilesystemBackup({ vaultPath, deviceId, root null, ).catch(() => null); if (manifest?.state !== 'applied') continue; + if (!Array.isArray(manifest.roots) + || manifest.roots.some((root) => typeof root !== 'string')) { + throw new Error(`Invalid projection backup manifest: ${entry.name}`); + } + // Local-only backups retain roots that were device-approved when captured. return rollbackBackup({ vaultPath, deviceId, - allowedRoots, + allowedRoots: normalizeRoots([...allowedRoots, ...manifest.roots]), backupId: entry.name, }); } diff --git a/test/core.test.js b/test/core.test.js index f5defbb..a40b355 100644 --- a/test/core.test.js +++ b/test/core.test.js @@ -274,6 +274,32 @@ test('projection plans reject overlapping target roots', async () => { ); }); +test('projection plans reject overlapping target roots through symlink aliases', async () => { + const root = await tempDir(); + const vault = path.join(root, 'vault'); + const physicalTarget = path.join(root, 'physical-skills'); + const aliasTarget = path.join(root, 'alias-skills'); + await mkdir(path.join(physicalTarget, 'nested'), { recursive: true }); + await symlink(physicalTarget, aliasTarget, 'dir'); + await addTarget({ + vaultPath: vault, + deviceId: 'macbook', + name: 'alias', + targetPath: aliasTarget, + }); + await addTarget({ + vaultPath: vault, + deviceId: 'macbook', + name: 'nested', + targetPath: path.join(physicalTarget, 'nested'), + }); + + await assert.rejects( + () => applyLinks({ vaultPath: vault, deviceId: 'macbook' }), + /Configured skill targets cannot overlap/, + ); +}); + test('installSkill tracks device-global installs without an agent target', async () => { const root = await tempDir(); const vault = path.join(root, 'vault'); @@ -1360,6 +1386,38 @@ test('approved local skill replacement is backed up without weakening unmanaged await assert.rejects(() => lstat(path.join(destination, '.skillsync-owned.json')), { code: 'ENOENT' }); }); +test('approved local replacement works through equivalent target aliases', async () => { + const root = await tempDir(); + const vault = path.join(root, 'vault'); + const physicalTarget = path.join(root, 'physical-skills'); + const firstAlias = path.join(root, 'first-skills'); + const secondAlias = path.join(root, 'second-skills'); + await makeSkill(physicalTarget, 'shared-skill', '# Local\n'); + await symlink(physicalTarget, firstAlias, 'dir'); + await symlink(physicalTarget, secondAlias, 'dir'); + await makeSkill(path.join(vault, 'skills'), 'shared-skill', '# Vault\n'); + await rebuildRegistry(vault); + await addTarget({ vaultPath: vault, deviceId: 'macbook', name: 'first', targetPath: firstAlias }); + await addTarget({ vaultPath: vault, deviceId: 'macbook', name: 'second', targetPath: secondAlias }); + await installSkill({ + vaultPath: vault, + deviceId: 'macbook', + skillName: 'shared-skill', + targets: ['second'], + }); + + await applyLinks({ + vaultPath: vault, + deviceId: 'macbook', + replaceUnmanagedPaths: [path.join(secondAlias, 'shared-skill')], + }); + + assert.equal( + await readFile(path.join(physicalTarget, 'shared-skill', 'SKILL.md'), 'utf8'), + '# Vault\n', + ); +}); + test('copy projections refuse local edits unless discarding them is explicit', async () => { const root = await tempDir(); const vault = path.join(root, 'vault'); @@ -1551,6 +1609,27 @@ test('rollback restores the previous copy projection', async () => { ); }); +test('rollback restores projections from a removed target', async () => { + const root = await tempDir(); + const vault = path.join(root, 'vault'); + const target = path.join(root, 'codex-skills'); + await makeSkill(path.join(vault, 'skills'), 'shared-skill', '# Shared\n'); + await rebuildRegistry(vault); + await addTarget({ vaultPath: vault, deviceId: 'macbook', name: 'codex', targetPath: target }); + await installSkill({ + vaultPath: vault, + deviceId: 'macbook', + skillName: 'shared-skill', + targets: ['codex'], + }); + await applyLinks({ vaultPath: vault, deviceId: 'macbook' }); + await removeTargetAndPrune({ vaultPath: vault, deviceId: 'macbook', name: 'codex' }); + + await rollbackLinks({ vaultPath: vault, deviceId: 'macbook' }); + + assert.equal((await lstat(path.join(target, 'shared-skill'))).isSymbolicLink(), true); +}); + test('vault checks reject stale registry entries, secrets, and symlinks', async () => { const root = await tempDir(); const vault = path.join(root, 'vault'); @@ -1573,6 +1652,20 @@ test('vault checks reject stale registry entries, secrets, and symlinks', async await assert.rejects(() => checkVault(vault), /Invalid JSON: state\/broken.json/); }); +test('vault checks reject local-state paths that Git can track', async () => { + const root = await tempDir(); + const vault = path.join(root, 'vault'); + await rebuildRegistry(vault); + await mkdir(path.join(vault, '.git'), { recursive: true }); + await mkdir(path.join(vault, '.skillsync-local'), { recursive: true }); + await writeFile(path.join(vault, '.skillsync-local', 'device.json'), '{}\n'); + + await assert.rejects( + () => checkVault(vault), + /Reserved local state path in Git-backed vault: \.skillsync-local/, + ); +}); + test('adding a skill rejects credentials before copying it into the vault', async () => { const root = await tempDir(); const source = await makeSkill(root, 'unsafe-skill', '# Unsafe\n');