diff --git a/src/cli/commands/plugin-skills.ts b/src/cli/commands/plugin-skills.ts index 0661fec..f9f7072 100644 --- a/src/cli/commands/plugin-skills.ts +++ b/src/cli/commands/plugin-skills.ts @@ -18,7 +18,7 @@ import { } from '../../constants.js'; import { addMarketplace, - findMarketplace, + findMarketplaceRegistration, listMarketplacePlugins, updateMarketplace, } from '../../core/marketplace.js'; @@ -645,14 +645,14 @@ async function installSkillViaMarketplace(opts: { // Check if the marketplace is already registered at any scope (user or project) let marketplaceName: string | undefined; - const existingAnyScope = await findMarketplace( + const existingAnyScope = await findMarketplaceRegistration( parsed?.repo ?? from, sourceLocation, isUser ? undefined : workspacePath, ); if (existingAnyScope) { - marketplaceName = existingAnyScope.name; + marketplaceName = existingAnyScope.key; await updateMarketplace( marketplaceName, isUser ? undefined : workspacePath, @@ -1182,14 +1182,14 @@ async function selectAndInstallSkillsFromSource(opts: { const sourceLocation = parsed ? `${parsed.owner}/${parsed.repo}` : undefined; let marketplaceName: string | undefined; - const existingAnyScope = await findMarketplace( + const existingAnyScope = await findMarketplaceRegistration( parsed?.repo ?? from, sourceLocation, isUser ? undefined : workspacePath, ); if (existingAnyScope) { - marketplaceName = existingAnyScope.name; + marketplaceName = existingAnyScope.key; await updateMarketplace(marketplaceName, isUser ? undefined : workspacePath); } else { const scopeOptions = isUser @@ -1534,14 +1534,14 @@ async function installAllViaMarketplace(opts: { const sourceLocation = parsed ? `${parsed.owner}/${parsed.repo}` : undefined; let marketplaceName: string | undefined; - const existingAnyScope = await findMarketplace( + const existingAnyScope = await findMarketplaceRegistration( parsed?.repo ?? from, sourceLocation, isUser ? undefined : workspacePath, ); if (existingAnyScope) { - marketplaceName = existingAnyScope.name; + marketplaceName = existingAnyScope.key; await updateMarketplace( marketplaceName, isUser ? undefined : workspacePath, diff --git a/src/cli/commands/plugin.ts b/src/cli/commands/plugin.ts index d5303fa..849026d 100644 --- a/src/cli/commands/plugin.ts +++ b/src/cli/commands/plugin.ts @@ -6,6 +6,7 @@ import { updateMarketplace, listMarketplacePlugins, findMarketplace, + findMarketplaceRegistration, parsePluginSpec, getAllagentsDir, getMarketplaceVersion, @@ -15,6 +16,7 @@ import { loadRegistryFromPath, type ScopedMarketplaceEntry, getMarketplaceOverrides, + getMarketplaceAccessError, } from '../../core/marketplace.js'; import { syncWorkspace, syncUserWorkspace } from '../../core/sync.js'; import { loadSyncState } from '../../core/sync-state.js'; @@ -276,7 +278,7 @@ const marketplaceListCmd = command({ if (isJsonMode()) { const enriched = await Promise.all( marketplaces.map(async (mp) => { - const version = await getMarketplaceVersion(mp.path); + const version = await getMarketplaceVersion(mp); return { ...mp, ...(version && { @@ -327,7 +329,7 @@ const marketplaceListCmd = command({ console.log(` ❯ ${mp.name} (${mp.scope})`); console.log(` Source: ${sourceLabel}`); - const version = await getMarketplaceVersion(mp.path); + const version = await getMarketplaceVersion(mp); if (version) { const ts = version.date.toISOString().replace('T', ' ').slice(0, 16); console.log(` Version: ${version.hash} (${ts})`); @@ -491,12 +493,16 @@ const marketplaceRemoveCmd = command({ name, path: result.marketplace?.path, retainedUserPlugins: result.retainedUserPlugins ?? [], + warnings: result.warnings ?? [], }, }); return; } console.log(`\u2713 Marketplace '${name}' removed`); + for (const warning of result.warnings ?? []) { + console.warn(`Warning: ${warning}`); + } if (result.retainedUserPlugins && result.retainedUserPlugins.length > 0) { console.log(`\n \u26A0 ${result.retainedUserPlugins.length} plugin(s) still reference this marketplace:`); for (const p of result.retainedUserPlugins) { @@ -1408,8 +1414,9 @@ const pluginUpdateCmd = command({ return { parsePluginSpec, - getMarketplace: (name: string, sourceLocation?: string) => - findMarketplace(name, sourceLocation, workspacePath), + getMarketplaceRegistration: (name: string, sourceLocation?: string) => + findMarketplaceRegistration(name, sourceLocation, workspacePath), + validateMarketplaceAccess: getMarketplaceAccessError, parseMarketplaceManifest, updateMarketplace: async (name: string) => { // Skip if already updated in this scope during this run diff --git a/src/cli/tui/actions/plugins.ts b/src/cli/tui/actions/plugins.ts index 126be32..375e160 100644 --- a/src/cli/tui/actions/plugins.ts +++ b/src/cli/tui/actions/plugins.ts @@ -17,7 +17,8 @@ import { addMarketplace, removeMarketplace, updateMarketplace, - findMarketplace, + findMarketplaceRegistration, + getMarketplaceAccessError, parsePluginSpec, type MarketplaceEntry, type MarketplacePluginsResult, @@ -42,8 +43,9 @@ function createUpdateDeps(workspacePath?: string) { const updatedMarketplaces = new Set(); return { parsePluginSpec, - getMarketplace: (name: string, sourceLocation?: string) => - findMarketplace(name, sourceLocation, workspacePath), + getMarketplaceRegistration: (name: string, sourceLocation?: string) => + findMarketplaceRegistration(name, sourceLocation, workspacePath), + validateMarketplaceAccess: getMarketplaceAccessError, parseMarketplaceManifest, updateMarketplace: async (name: string) => { if (updatedMarketplaces.has(name)) { @@ -880,6 +882,10 @@ async function runMarketplaceDetail( continue; } + if (result.warnings && result.warnings.length > 0) { + p.note(result.warnings.join('\n'), 'Warning'); + } + cache?.invalidate(); } catch (error) { const message = error instanceof Error ? error.message : String(error); diff --git a/src/core/marketplace.ts b/src/core/marketplace.ts index 68da537..0cf19b0 100644 --- a/src/core/marketplace.ts +++ b/src/core/marketplace.ts @@ -1,5 +1,6 @@ -import { existsSync } from 'node:fs'; -import { mkdir, readFile, readdir, rm, writeFile } from 'node:fs/promises'; +import { randomUUID } from 'node:crypto'; +import { existsSync, lstatSync, realpathSync } from 'node:fs'; +import { mkdir, readFile, readdir, rename, rm, writeFile } from 'node:fs/promises'; import { basename, dirname, join, resolve } from 'node:path'; import simpleGit from 'simple-git'; import { getHomeDir } from '../constants.js'; @@ -12,7 +13,7 @@ import { parseMarketplaceManifest, resolvePluginSourcePath, } from '../utils/marketplace-manifest-parser.js'; -import { getPluginCachePath, parseGitHubUrl } from '../utils/plugin-path.js'; +import { getPluginCachePath, isFilesystemRoot, parseGitHubUrl } from '../utils/plugin-path.js'; import { GitCloneError, cloneTo, gitHubUrl, pull } from './git.js'; import { fetchPlugin } from './plugin.js'; import type { FetchResult } from './plugin.js'; @@ -56,6 +57,14 @@ export interface MarketplaceEntry { lastUpdated?: string; } +/** Exact registry identity for an entry; embedded names are not authoritative keys. */ +export interface MarketplaceRegistration { + key: string; + entry: MarketplaceEntry; + scope: MarketplaceScope; + registryPath: string; +} + /** * Marketplace registry structure */ @@ -64,6 +73,38 @@ export interface MarketplaceRegistry { marketplaces: Record; } +/** Registry aliases are untrusted keys and may overlap Object.prototype. */ +function getRegistryMarketplace( + registry: MarketplaceRegistry, + key: string, +): MarketplaceEntry | undefined { + return Object.hasOwn(registry.marketplaces, key) + ? registry.marketplaces[key] + : undefined; +} + +function setRegistryMarketplace( + registry: MarketplaceRegistry, + key: string, + entry: MarketplaceEntry, +): void { + Object.defineProperty(registry.marketplaces, key, { + value: entry, + enumerable: true, + configurable: true, + writable: true, + }); +} + +function deleteRegistryMarketplace( + registry: MarketplaceRegistry, + key: string, +): boolean { + return Object.hasOwn(registry.marketplaces, key) + ? delete registry.marketplaces[key] + : false; +} + /** * Result of marketplace operations */ @@ -79,6 +120,8 @@ export interface MarketplaceResult { removedUserPlugins?: string[]; /** User-level plugins that still reference the removed marketplace (returned when cascade is off) */ retainedUserPlugins?: string[]; + /** Non-fatal safety warnings produced by the operation. */ + warnings?: string[]; } /** @@ -95,6 +138,152 @@ export function getMarketplacesDir(): string { return join(getAllagentsDir(), 'plugins', 'marketplaces'); } +/** Keep registry aliases unambiguous without imposing filesystem rules. */ +function isValidMarketplaceAlias(name: string): boolean { + const hasControlCharacter = Array.from(name).some( + (character) => { + const codePoint = character.charCodeAt(0); + return codePoint < 32 || codePoint === 127; + }, + ); + return !( + !name || + name === '.' || + name === '..' || + /[/\\]/.test(name) || + hasControlCharacter + ); +} + +/** + * Remote marketplace names become directory names for managed caches. Apply + * portable filesystem restrictions in addition to the registry-alias rules. + */ +function isValidManagedMarketplaceName(name: string): boolean { + if ( + !isValidMarketplaceAlias(name) || + /[<>:"|?*]/.test(name) || + /[ .]$/.test(name) || + /^(con|prn|aux|nul|com[1-9]|lpt[1-9])(?:\.|$)/i.test(name) + ) { + return false; + } + + const cacheRoot = resolve(getMarketplacesDir()); + return dirname(resolve(cacheRoot, name)) === cacheRoot; +} + +/** Return the exact managed cache path for a safe marketplace name. */ +function getManagedMarketplacePath(name: string): string | null { + if (!isValidManagedMarketplaceName(name) || !hasSafeManagedMarketplaceRoot()) { + return null; + } + return resolve(getMarketplacesDir(), name); +} + +/** + * Local marketplaces are user-owned, but a filesystem root or the user's + * entire home directory is too broad to be a marketplace boundary. + */ +function isUnsafeLocalMarketplacePath(marketplacePath: string): boolean { + const resolvedPath = canonicalizeExistingPath(marketplacePath); + const homePath = canonicalizeExistingPath(getHomeDir()); + return isFilesystemRoot(resolvedPath) || resolvedPath === homePath; +} + +/** Resolve symlinks for existing paths while retaining a stable fallback. */ +function canonicalizeExistingPath(candidatePath: string): string { + try { + return realpathSync(candidatePath); + } catch { + return resolve(candidatePath); + } +} + +/** + * AllAgents creates managed remote caches as real directories. A symlink at + * that location has unknown ownership and must not be followed or removed. + */ +function isSymbolicLinkPath(candidatePath: string): boolean { + try { + return lstatSync(candidatePath).isSymbolicLink(); + } catch { + return false; + } +} + +/** Check for a filesystem entry without following a dangling symlink. */ +function pathEntryExists(candidatePath: string): boolean { + try { + lstatSync(candidatePath); + return true; + } catch { + return false; + } +} + +/** + * The complete AllAgents state directory may be relocated as one unit, but + * its internal plugin/cache directories must remain real owned directories. + */ +function hasSafeManagedMarketplaceRoot(): boolean { + const allagentsPath = canonicalizeExistingPath(getAllagentsDir()); + const homePath = canonicalizeExistingPath(getHomeDir()); + if (isFilesystemRoot(allagentsPath) || allagentsPath === homePath) { + return false; + } + return ( + !isSymbolicLinkPath(join(getAllagentsDir(), 'plugins')) && + !isSymbolicLinkPath(getMarketplacesDir()) + ); +} + +/** + * Remote marketplace paths are AllAgents-owned only at their exact managed + * cache location. Registry data is untrusted and must be checked before rm. + */ +function hasManagedRemotePath(marketplace: MarketplaceEntry): boolean { + if (marketplace.source.type === 'local') return false; + if (!isValidManagedMarketplaceName(marketplace.name)) return false; + if (!hasSafeManagedMarketplaceRoot()) return false; + const cacheRoot = resolve(getMarketplacesDir()); + const marketplacePath = resolve(marketplace.path); + if (dirname(marketplacePath) !== cacheRoot) return false; + if (isSymbolicLinkPath(marketplacePath)) return false; + + // Ancestor symlinks may intentionally relocate the entire AllAgents state + // directory. Treat the canonical cache root as the ownership boundary while + // still refusing a symlink at an individual marketplace cache location. + const canonicalCacheRoot = canonicalizeExistingPath(cacheRoot); + const canonicalMarketplacePath = pathEntryExists(marketplacePath) + ? canonicalizeExistingPath(marketplacePath) + : resolve(canonicalCacheRoot, basename(marketplacePath)); + if (dirname(canonicalMarketplacePath) !== canonicalCacheRoot) return false; + + const sourceName = marketplace.source.type === 'github' + ? parseLocation(marketplace.source.location).repo + : parseMarketplaceSource(marketplace.source.location)?.name; + const allowedNames = [marketplace.name, sourceName].filter( + (name): name is string => + name != null && isValidManagedMarketplaceName(name), + ); + return allowedNames.includes(basename(marketplacePath)); +} + +/** Return a safety error before reading, updating, or deleting registry paths. */ +export function getMarketplaceAccessError( + marketplace: MarketplaceEntry, +): string | undefined { + if (marketplace.source.type === 'local') { + return isUnsafeLocalMarketplacePath(marketplace.path) + ? `Refused to access overly broad local marketplace path: ${marketplace.path}` + : undefined; + } + return hasManagedRemotePath(marketplace) + ? undefined + : `Refused to access unmanaged marketplace path: ${marketplace.path}`; +} + /** * Get the registry file path */ @@ -264,6 +453,26 @@ export interface MarketplaceScopeOptions { workspacePath?: string; } +function getMarketplaceCloneError( + location: string, + error: unknown, +): string { + if (error instanceof GitCloneError) { + if (error.isAuthError) { + return `Authentication failed for ${location}.\n Check your SSH keys or git credentials.`; + } + if (error.isTimeout) { + return `Clone timed out for ${location}.\n Check your network connection.`; + } + } + + const message = error instanceof Error ? error.message : String(error); + if (message.toLowerCase().includes('not found') || message.includes('404')) { + return `Repository not found: ${location}`; + } + return `Failed to clone marketplace: ${message}`; +} + /** * Add a marketplace to the registry * Idempotent: returns success if marketplace is already registered by source location @@ -292,6 +501,18 @@ export async function addMarketplace( // Resolve branch: explicit --branch flag wins over URL-parsed branch const effectiveBranch = branch || parsed.branch; + let name = customName || parsed.name; + + const isRemoteMarketplace = parsed.type === 'github' || parsed.type === 'git'; + const hasValidName = isRemoteMarketplace + ? isValidManagedMarketplaceName(name) + : isValidMarketplaceAlias(name); + if (!hasValidName) { + return { + success: false, + error: `Invalid marketplace name '${name}'. Use a single directory name without path separators or traversal segments.`, + }; + } // Naming rules for non-default branches if (effectiveBranch) { @@ -309,7 +530,6 @@ export async function addMarketplace( } } - let name = customName || parsed.name; const registryPath = scopeOptions?.scope === 'project' && scopeOptions?.workspacePath ? getProjectRegistryPath(scopeOptions.workspacePath) : getRegistryPath(); @@ -331,10 +551,25 @@ export async function addMarketplace( let alreadyRegistered = !!existingBySource; let marketplacePath: string; + let clonedMarketplace = false; if (parsed.type === 'github' || parsed.type === 'git') { // Clone remote repository - marketplacePath = join(getMarketplacesDir(), name); + const managedMarketplacePath = getManagedMarketplacePath(name); + if (managedMarketplacePath === null) { + return { + success: false, + error: `Marketplace cache root is not a safe AllAgents-owned directory: ${getMarketplacesDir()}`, + }; + } + marketplacePath = managedMarketplacePath; + + if (isSymbolicLinkPath(marketplacePath)) { + return { + success: false, + error: `Remote marketplace cache cannot be a symbolic link: ${marketplacePath}`, + }; + } // Check if directory already exists (from a previous partial registration) if (existsSync(marketplacePath)) { @@ -358,31 +593,23 @@ export async function addMarketplace( // Clone repository (with branch if specified) try { await cloneTo(repoUrl, marketplacePath, effectiveBranch); + clonedMarketplace = true; } catch (error) { - if (error instanceof GitCloneError) { - if (error.isAuthError) { - return { - success: false, - error: `Authentication failed for ${parsed.location}.\n Check your SSH keys or git credentials.`, - }; - } - } - const msg = error instanceof Error ? error.message : String(error); - if (msg.toLowerCase().includes('not found') || msg.includes('404')) { - return { - success: false, - error: `Repository not found: ${parsed.location}`, - }; - } return { success: false, - error: `Failed to clone marketplace: ${msg}`, + error: getMarketplaceCloneError(parsed.location, error), }; } } } else { // Local directory - just verify it exists marketplacePath = parsed.location; + if (isUnsafeLocalMarketplacePath(marketplacePath)) { + return { + success: false, + error: `Local marketplace source must be a specific directory, not a filesystem root or the user's home directory: ${marketplacePath}`, + }; + } if (!existsSync(marketplacePath)) { return { success: false, @@ -396,9 +623,21 @@ export async function addMarketplace( const manifestResult = await parseMarketplaceManifest(marketplacePath); if (manifestResult.success && manifestResult.data.name) { const manifestName = manifestResult.data.name; + const hasValidManifestName = isRemoteMarketplace + ? isValidManagedMarketplaceName(manifestName) + : isValidMarketplaceAlias(manifestName); + if (!hasValidManifestName) { + if (clonedMarketplace) { + await rm(marketplacePath, { recursive: true, force: true }); + } + return { + success: false, + error: `Invalid marketplace name '${manifestName}' in marketplace manifest. Use a single directory name without path separators or traversal segments.`, + }; + } if (manifestName !== name) { // Track if the manifest name is already registered - if (registry.marketplaces[manifestName]) { + if (getRegistryMarketplace(registry, manifestName)) { alreadyRegistered = true; } name = manifestName; @@ -407,7 +646,7 @@ export async function addMarketplace( } // Check if already registered by name (after manifest parsing to use final name) - if (registry.marketplaces[name]) { + if (getRegistryMarketplace(registry, name)) { alreadyRegistered = true; } @@ -434,7 +673,7 @@ export async function addMarketplace( }; // Save to registry - registry.marketplaces[name] = entry; + setRegistryMarketplace(registry, name, entry); await saveRegistryToPath(registry, registryPath); return { @@ -481,16 +720,27 @@ export async function removeMarketplace( const userRegPath = options.userRegistryPath ?? getRegistryPath(); let removedEntry: MarketplaceEntry | undefined; + const warnings: string[] = []; // Remove from user scope if (scope === 'user' || scope === 'all') { const userRegistry = await loadRegistryFromPath(userRegPath); - if (userRegistry.marketplaces[name]) { - removedEntry = userRegistry.marketplaces[name]; - delete userRegistry.marketplaces[name]; + const userEntry = getRegistryMarketplace(userRegistry, name); + if (userEntry) { + removedEntry = userEntry; + deleteRegistryMarketplace(userRegistry, name); await saveRegistryToPath(userRegistry, userRegPath); - if (removedEntry.source.type !== 'local' && existsSync(removedEntry.path)) { - await rm(removedEntry.path, { recursive: true, force: true }); + if ( + removedEntry.source.type !== 'local' && + pathEntryExists(removedEntry.path) + ) { + if (hasManagedRemotePath(removedEntry)) { + await rm(removedEntry.path, { recursive: true, force: true }); + } else { + warnings.push( + `Refused to delete unmanaged marketplace path: ${removedEntry.path}`, + ); + } } } } @@ -499,12 +749,22 @@ export async function removeMarketplace( if ((scope === 'project' || scope === 'all') && options.workspacePath) { const projectRegPath = getProjectRegistryPath(options.workspacePath); const projectRegistry = await loadRegistryFromPath(projectRegPath); - if (projectRegistry.marketplaces[name]) { - removedEntry = projectRegistry.marketplaces[name]; - delete projectRegistry.marketplaces[name]; + const projectEntry = getRegistryMarketplace(projectRegistry, name); + if (projectEntry) { + removedEntry = projectEntry; + deleteRegistryMarketplace(projectRegistry, name); await saveRegistryToPath(projectRegistry, projectRegPath); - if (removedEntry.source.type !== 'local' && existsSync(removedEntry.path)) { - await rm(removedEntry.path, { recursive: true, force: true }); + if ( + removedEntry.source.type !== 'local' && + pathEntryExists(removedEntry.path) + ) { + if (hasManagedRemotePath(removedEntry)) { + await rm(removedEntry.path, { recursive: true, force: true }); + } else { + warnings.push( + `Refused to delete unmanaged marketplace path: ${removedEntry.path}`, + ); + } } } } @@ -527,6 +787,7 @@ export async function removeMarketplace( success: true, marketplace: removedEntry, removedUserPlugins, + ...(warnings.length > 0 && { warnings }), }; } @@ -540,6 +801,7 @@ export async function removeMarketplace( success: true, marketplace: removedEntry, retainedUserPlugins, + ...(warnings.length > 0 && { warnings }), }; } @@ -560,15 +822,54 @@ export async function getMarketplace( name: string, workspacePath?: string, ): Promise { - if (workspacePath) { - const { registry } = await loadMergedRegistries( - getRegistryPath(), - getProjectRegistryPath(workspacePath), - ); - return registry.marketplaces[name] || null; - } - const registry = await loadRegistry(); - return registry.marketplaces[name] || null; + return (await findMarketplaceRegistration(name, undefined, workspacePath)) + ?.entry ?? null; +} + +async function loadMarketplaceRegistrations( + workspacePath?: string, +): Promise> { + const userRegistryPath = getRegistryPath(); + const userRegistry = await loadRegistryFromPath(userRegistryPath); + const registrations = new Map(); + for (const [key, entry] of Object.entries(userRegistry.marketplaces)) { + registrations.set(key, { + key, + entry, + scope: 'user', + registryPath: userRegistryPath, + }); + } + + if (!workspacePath) return registrations; + const projectRegistryPath = getProjectRegistryPath(workspacePath); + if (resolve(projectRegistryPath) === resolve(userRegistryPath)) { + return registrations; + } + const projectRegistry = await loadRegistryFromPath(projectRegistryPath); + for (const [key, entry] of Object.entries(projectRegistry.marketplaces)) { + registrations.set(key, { + key, + entry, + scope: 'project', + registryPath: projectRegistryPath, + }); + } + return registrations; +} + +export async function findMarketplaceRegistration( + name: string, + sourceLocation?: string, + workspacePath?: string, +): Promise { + const registrations = await loadMarketplaceRegistrations(workspacePath); + const exact = registrations.get(name); + if (exact) return exact; + if (!sourceLocation) return null; + return Array.from(registrations.values()).find( + ({ entry }) => getSourceLocationKey(entry.source) === sourceLocation, + ) ?? null; } /** @@ -580,16 +881,9 @@ export async function findMarketplace( sourceLocation?: string, workspacePath?: string, ): Promise { - const registry = workspacePath - ? (await loadMergedRegistries(getRegistryPath(), getProjectRegistryPath(workspacePath))).registry - : await loadRegistry(); - if (registry.marketplaces[name]) { - return registry.marketplaces[name]; - } - if (sourceLocation) { - return findBySourceLocation(registry, sourceLocation); - } - return null; + return ( + await findMarketplaceRegistration(name, sourceLocation, workspacePath) + )?.entry ?? null; } /** @@ -611,13 +905,27 @@ export async function updateMarketplace( } // Merge for lookup, tracking which scope each entry came from - const mergedEntries = new Map(); - for (const entry of Object.values(userRegistry.marketplaces)) { - mergedEntries.set(entry.name, { entry, scope: 'user' }); + const userRegistryPath = getRegistryPath(); + const projectRegistryPath = workspacePath + ? getProjectRegistryPath(workspacePath) + : undefined; + const mergedEntries = new Map(); + for (const [key, entry] of Object.entries(userRegistry.marketplaces)) { + mergedEntries.set(key, { + key, + entry, + scope: 'user', + registryPath: userRegistryPath, + }); } if (projectRegistry) { - for (const entry of Object.values(projectRegistry.marketplaces)) { - mergedEntries.set(entry.name, { entry, scope: 'project' }); + for (const [key, entry] of Object.entries(projectRegistry.marketplaces)) { + mergedEntries.set(key, { + key, + entry, + scope: 'project', + registryPath: projectRegistryPath as string, + }); } } @@ -628,14 +936,37 @@ export async function updateMarketplace( })() : Array.from(mergedEntries.values()); - const toUpdate = toUpdateScoped.map((s) => s.entry); const results: Array<{ name: string; success: boolean; error?: string }> = []; - if (name && toUpdate.length === 0) { + if (name && toUpdateScoped.length === 0) { return [{ name, success: false, error: `Marketplace '${name}' not found` }]; } - for (const marketplace of toUpdate) { + const invalidRegistrations = new Set(); + const blockedSaveScopes = new Set(); + let userDirty = false; + let projectDirty = false; + + for (const registration of toUpdateScoped) { + const { entry: marketplace, key, scope } = registration; + const accessError = getMarketplaceAccessError(marketplace); + if (accessError) { + const registry = scope === 'user' ? userRegistry : projectRegistry; + const removal = await removeInvalidMarketplaceRegistration(registration); + if (removal.removed && registry) { + deleteRegistryMarketplace(registry, key); + } else if (!removal.removed) { + blockedSaveScopes.add(scope); + } + invalidRegistrations.add(registration); + results.push({ + name: marketplace.name, + success: false, + error: removal.error ?? 'Unsafe marketplace registration was not updated.', + }); + continue; + } + if (marketplace.source.type === 'local') { // Local marketplaces don't need updating results.push({ @@ -712,22 +1043,27 @@ export async function updateMarketplace( } // Save updated timestamps back to the appropriate registries - let userDirty = false; - let projectDirty = false; - for (const { entry, scope } of toUpdateScoped) { + for (const registration of toUpdateScoped) { + const { entry, key, scope } = registration; + if (invalidRegistrations.has(registration)) continue; if (scope === 'user') { - userRegistry.marketplaces[entry.name] = entry; + setRegistryMarketplace(userRegistry, key, entry); userDirty = true; } else if (projectRegistry) { - projectRegistry.marketplaces[entry.name] = entry; + setRegistryMarketplace(projectRegistry, key, entry); projectDirty = true; } } - if (userDirty) { + if (userDirty && !blockedSaveScopes.has('user')) { await saveRegistry(userRegistry); } - if (projectDirty && projectRegistry && workspacePath) { - await saveRegistryToPath(projectRegistry, getProjectRegistryPath(workspacePath)); + if ( + projectDirty && + !blockedSaveScopes.has('project') && + projectRegistry && + projectRegistryPath + ) { + await saveRegistryToPath(projectRegistry, projectRegistryPath); } return results; @@ -738,7 +1074,8 @@ export async function updateMarketplace( */ export async function getMarketplacePath(name: string): Promise { const marketplace = await getMarketplace(name); - return marketplace?.path || null; + if (!marketplace || getMarketplaceAccessError(marketplace)) return null; + return marketplace.path; } /** @@ -821,6 +1158,11 @@ export async function listMarketplacePlugins( return { plugins: [], warnings: [] }; } + const accessError = getMarketplaceAccessError(marketplace); + if (accessError) { + return { plugins: [], warnings: [accessError] }; + } + // Try manifest first const manifestResult = await getMarketplacePluginsFromManifest( marketplace.path, @@ -924,6 +1266,7 @@ export async function resolvePluginSpec( options: { subpath?: string; marketplaceNameOverride?: string; + /** Trusted explicit path that bypasses registry lookup (primarily for isolated resolution). */ marketplacePathOverride?: string; offline?: boolean; fetchFn?: (url: string) => Promise; @@ -951,6 +1294,9 @@ export async function resolvePluginSpec( if (!marketplace) { return null; } + if (getMarketplaceAccessError(marketplace)) { + return null; + } marketplacePath = marketplace.path; } @@ -1052,35 +1398,190 @@ export interface ResolvePluginSpecResult { error?: string; } +interface InvalidMarketplaceRemovalResult extends MarketplaceResult { + removed: boolean; +} + +async function removeInvalidMarketplaceRegistration( + registration: MarketplaceRegistration, +): Promise { + const registry = await loadRegistryFromPath(registration.registryPath); + const currentEntry = getRegistryMarketplace(registry, registration.key); + if (!currentEntry || !hasSameMarketplaceIdentity(currentEntry, registration.entry)) { + return { + success: false, + removed: false, + error: `Marketplace registration '${registration.key}' changed before unsafe cleanup. No registry entry or filesystem path was removed; retry the command.`, + }; + } + deleteRegistryMarketplace(registry, registration.key); + await saveRegistryToPath(registry, registration.registryPath); + return { + success: false, + removed: true, + error: getInvalidMarketplaceRegistrationError( + registration.key, + registration.entry, + ), + }; +} + +function hasSameMarketplaceIdentity( + current: MarketplaceEntry, + expected: MarketplaceEntry, +): boolean { + return ( + current.name === expected.name && + current.path === expected.path && + current.source.type === expected.source.type && + current.source.location === expected.source.location + ); +} + +function getInvalidMarketplaceRegistrationError( + registrationKey: string, + marketplace: MarketplaceEntry, +): string { + return `Removed invalid marketplace registration '${registrationKey}'. Refused to access or delete unmanaged path: ${marketplace.path}. Re-add the marketplace to restore it safely.`; +} + /** - * Refresh a GitHub marketplace by removing it from registry, deleting the - * cached directory, and re-adding it (fresh clone). - * Unlike removeMarketplace, this does NOT cascade-remove user plugins. + * Refresh a remote marketplace through a staged clone. Valid registrations and + * cached files survive clone failures; unsafe legacy registrations are removed + * without touching their untrusted paths. */ async function refreshMarketplace( - marketplace: MarketplaceEntry, + registration: MarketplaceRegistration, ): Promise { + const marketplace = registration.entry; if (marketplace.source.type === 'local') { return { success: true, marketplace }; } - // Remove from registry without cascade - const registry = await loadRegistry(); - delete registry.marketplaces[marketplace.name]; - await saveRegistry(registry); - - // Delete the cached directory - if (existsSync(marketplace.path)) { - await rm(marketplace.path, { recursive: true, force: true }); + const managedPath = hasManagedRemotePath(marketplace) + ? resolve(marketplace.path) + : null; + if (managedPath === null) { + return removeInvalidMarketplaceRegistration(registration); } - // Re-add with original source (will clone fresh) + let cloneUrl: string; + let branch: string | undefined; if (marketplace.source.type === 'github') { - const { owner, repo, branch } = parseLocation(marketplace.source.location); - return addMarketplace(`${owner}/${repo}`, marketplace.name, branch); + const parsed = parseLocation(marketplace.source.location); + cloneUrl = gitHubUrl(parsed.owner, parsed.repo); + branch = parsed.branch; + } else { + cloneUrl = marketplace.source.location; + } + + const cacheRoot = getMarketplacesDir(); + const refreshId = randomUUID(); + const stagingPath = join(cacheRoot, `.refresh-${refreshId}`); + const backupPath = join(cacheRoot, `.backup-${refreshId}`); + await mkdir(cacheRoot, { recursive: true }); + + try { + await cloneTo(cloneUrl, stagingPath, branch); + } catch (error) { + await rm(stagingPath, { recursive: true, force: true }); + return { + success: false, + error: `Failed to refresh marketplace '${marketplace.name}': ${getMarketplaceCloneError(marketplace.source.location, error)}\n The existing registration and any cached files were preserved.`, + }; } - // git type: use the full URL directly - return addMarketplace(marketplace.source.location, marketplace.name); + + const hadExistingCache = existsSync(managedPath); + if (hadExistingCache) { + try { + await rename(managedPath, backupPath); + } catch (error) { + await rm(stagingPath, { recursive: true, force: true }).catch(() => {}); + return { + success: false, + error: `Failed to prepare marketplace cache for '${marketplace.name}': ${error instanceof Error ? error.message : String(error)} The existing registration and cache were preserved.`, + }; + } + } + + try { + await rename(stagingPath, managedPath); + } catch (error) { + let recoveryError: unknown; + if (hadExistingCache && existsSync(backupPath)) { + try { + await rename(backupPath, managedPath); + } catch (restoreError) { + recoveryError = restoreError; + } + } + await rm(stagingPath, { recursive: true, force: true }).catch(() => {}); + const replacementError = error instanceof Error ? error.message : String(error); + if (recoveryError) { + const recoveryMessage = recoveryError instanceof Error + ? recoveryError.message + : String(recoveryError); + return { + success: false, + error: `Failed to replace marketplace cache for '${marketplace.name}': ${replacementError}. Automatic recovery also failed: ${recoveryMessage}. The original cache remains at ${backupPath}.`, + }; + } + return { + success: false, + error: `Failed to replace marketplace cache for '${marketplace.name}': ${replacementError}. The existing registration and cache were preserved.`, + }; + } + + const refreshedMarketplace: MarketplaceEntry = { + ...marketplace, + path: managedPath, + lastUpdated: new Date().toISOString(), + }; + const registry = await loadRegistryFromPath(registration.registryPath); + const currentEntry = getRegistryMarketplace(registry, registration.key); + if (!currentEntry || !hasSameMarketplaceIdentity(currentEntry, marketplace)) { + let cleanupError: unknown; + let restoreError: unknown; + try { + await rm(managedPath, { recursive: true, force: true }); + } catch (error) { + cleanupError = error; + } + if (!cleanupError && hadExistingCache && existsSync(backupPath)) { + try { + await rename(backupPath, managedPath); + } catch (error) { + restoreError = error; + } + } + + let recoveryMessage = ''; + if (cleanupError) { + const detail = cleanupError instanceof Error + ? cleanupError.message + : String(cleanupError); + recoveryMessage = hadExistingCache + ? ` Automatic recovery could not remove the replacement cache: ${detail}. The replacement remains at ${managedPath}, and the original cache remains at ${backupPath}.` + : ` Automatic cleanup failed: ${detail}. The replacement cache remains at ${managedPath}.`; + } else if (restoreError) { + const detail = restoreError instanceof Error + ? restoreError.message + : String(restoreError); + recoveryMessage = ` Automatic recovery failed: ${detail}. The original cache remains at ${backupPath}.`; + } + return { + success: false, + error: `Marketplace registration '${registration.key}' changed during refresh. The registry was not overwritten.${recoveryMessage}`, + }; + } + setRegistryMarketplace(registry, registration.key, refreshedMarketplace); + await saveRegistryToPath(registry, registration.registryPath); + + if (hadExistingCache) { + await rm(backupPath, { recursive: true, force: true }).catch(() => {}); + } + + return { success: true, marketplace: refreshedMarketplace, replaced: true }; } /** @@ -1109,11 +1610,15 @@ export async function resolvePluginSpecWithAutoRegister( // Check if marketplace is already registered (by name, then by source location) const sourceLocation = owner && repo ? `${owner}/${repo}` : undefined; - let marketplace = await findMarketplace(marketplaceName, sourceLocation, options.workspacePath); + let registration = await findMarketplaceRegistration( + marketplaceName, + sourceLocation, + options.workspacePath, + ); let didAutoRegister = false; // If not registered, try auto-registration - if (!marketplace) { + if (!registration) { const sourceToRegister = owner && repo ? `${owner}/${repo}` : marketplaceName; const autoRegResult = await autoRegisterMarketplace(sourceToRegister); @@ -1123,33 +1628,48 @@ export async function resolvePluginSpecWithAutoRegister( error: autoRegResult.error || 'Unknown error', }; } - marketplace = await getMarketplace(autoRegResult.name ?? marketplaceName, options.workspacePath); + registration = await findMarketplaceRegistration( + autoRegResult.name ?? marketplaceName, + undefined, + options.workspacePath, + ); didAutoRegister = true; } - if (!marketplace) { + if (!registration) { return { success: false, error: `Marketplace '${marketplaceName}' not found`, }; } + let marketplace = registration.entry; + const accessError = getMarketplaceAccessError(marketplace); + if (accessError) { + const invalidResult = await removeInvalidMarketplaceRegistration(registration); + return { + success: false, + error: `Plugin '${pluginName}' could not be resolved from marketplace '${marketplaceName}'.\n ${invalidResult.error}`, + }; + } + const updateCacheKey = `${registration.registryPath}:${registration.key}`; + // Pull latest marketplace if online, not freshly cloned, and not yet updated this session if ( !didAutoRegister && !options.offline && marketplace.source.type !== 'local' && - !updatedMarketplaceCache.has(marketplace.name) + !updatedMarketplaceCache.has(updateCacheKey) ) { - const results = await updateMarketplace(marketplace.name, options.workspacePath); + const results = await updateMarketplace(registration.key, options.workspacePath); const result = results[0]; if (result?.success) { - updatedMarketplaceCache.add(marketplace.name); + updatedMarketplaceCache.add(updateCacheKey); } } // Mark freshly cloned marketplaces as updated so subsequent calls skip the pull if (didAutoRegister) { - updatedMarketplaceCache.add(marketplace.name); + updatedMarketplaceCache.add(updateCacheKey); } // Determine the expected subpath for error messages @@ -1160,6 +1680,7 @@ export async function resolvePluginSpecWithAutoRegister( const resolveOpts = { ...(subpath && { subpath }), marketplaceNameOverride: marketplace.name, + marketplacePathOverride: marketplace.path, ...(options.offline != null && { offline: options.offline }), ...(options.workspacePath && { workspacePath: options.workspacePath }), }; @@ -1169,14 +1690,22 @@ export async function resolvePluginSpecWithAutoRegister( // If not found and online, refresh the marketplace (re-clone) and retry if (!resolved && !options.offline && marketplace.source.type !== 'local') { console.log( - `Plugin not found in cached marketplace, refreshing '${marketplace.name}'...`, + `Plugin '${pluginName}' not found in cached marketplace '${marketplace.name}', refreshing...`, ); - const refreshResult = await refreshMarketplace(marketplace); + const refreshResult = await refreshMarketplace(registration); + if (!refreshResult.success) { + return { + success: false, + error: `Plugin '${pluginName}' could not be resolved from marketplace '${marketplaceName}'.\n ${refreshResult.error ?? 'Marketplace refresh failed.'}`, + }; + } if (refreshResult.success && refreshResult.marketplace) { marketplace = refreshResult.marketplace; + registration = { ...registration, entry: marketplace }; resolved = await resolvePluginSpec(spec, { ...(subpath && { subpath }), marketplaceNameOverride: marketplace.name, + marketplacePathOverride: marketplace.path, ...(options.workspacePath && { workspacePath: options.workspacePath }), }); } @@ -1396,10 +1925,10 @@ export async function loadMergedRegistries( const overrides: string[] = []; for (const [name, entry] of Object.entries(projectRegistry.marketplaces)) { - if (merged.marketplaces[name]) { + if (getRegistryMarketplace(merged, name)) { overrides.push(name); } - merged.marketplaces[name] = entry; + setRegistryMarketplace(merged, name, entry); } return { registry: merged, overrides }; @@ -1467,9 +1996,9 @@ export async function listMarketplacesWithScope( const overrides: string[] = []; // Add user entries that aren't overridden by project - for (const entry of Object.values(userRegistry.marketplaces)) { - if (projectNames.has(entry.name)) { - overrides.push(entry.name); + for (const [key, entry] of Object.entries(userRegistry.marketplaces)) { + if (projectNames.has(key)) { + overrides.push(key); } else { entries.push({ ...entry, scope: 'user' }); } @@ -1487,12 +2016,17 @@ export async function listMarketplacesWithScope( } /** - * Get the short git commit hash and date for a marketplace directory. - * Returns null if the marketplace is not a git repo or has no commits. + * Get the short git commit hash and date for a safely accessible marketplace. + * Returns null if the marketplace is unsafe, not a git repo, or has no commits. */ export async function getMarketplaceVersion( - marketplacePath: string, + marketplace: MarketplaceEntry, ): Promise<{ hash: string; date: Date } | null> { + const accessError = getMarketplaceAccessError(marketplace); + if (accessError) { + return null; + } + const marketplacePath = marketplace.path; if (!existsSync(marketplacePath)) { return null; } diff --git a/src/core/plugin.ts b/src/core/plugin.ts index d95356e..e6e0c80 100644 --- a/src/core/plugin.ts +++ b/src/core/plugin.ts @@ -409,7 +409,11 @@ export interface InstalledPluginUpdateResult { */ export interface UpdatePluginDeps { parsePluginSpec: (spec: string) => { plugin: string; marketplaceName: string; owner?: string; repo?: string } | null; - getMarketplace: (name: string, sourceLocation?: string) => Promise<{ name: string; path: string; source: { type: string } } | null>; + getMarketplaceRegistration: (name: string, sourceLocation?: string) => Promise<{ + key: string; + entry: { name: string; path: string; source: { type: 'github' | 'git' | 'local'; location: string } }; + } | null>; + validateMarketplaceAccess: (marketplace: { name: string; path: string; source: { type: 'github' | 'git' | 'local'; location: string } }) => string | undefined; parseMarketplaceManifest: (path: string) => Promise<{ success: boolean; data?: { plugins: Array<{ name: string; source: string | { url: string } }> } }>; updateMarketplace: (name: string) => Promise>; /** Optional fetch function for testing - defaults to fetchPlugin */ @@ -454,8 +458,11 @@ export async function updatePlugin( // Get marketplace info (with source location fallback for owner/repo format) const sourceLocation = parsed.owner && parsed.repo ? `${parsed.owner}/${parsed.repo}` : undefined; - const marketplace = await deps.getMarketplace(parsed.marketplaceName, sourceLocation); - if (!marketplace) { + const registration = await deps.getMarketplaceRegistration( + parsed.marketplaceName, + sourceLocation, + ); + if (!registration) { return { plugin: pluginSpec, success: false, @@ -463,15 +470,27 @@ export async function updatePlugin( error: `Marketplace not found: ${parsed.marketplaceName}`, }; } + const marketplace = registration.entry; + + const accessError = deps.validateMarketplaceAccess(marketplace); + if (accessError) { + return { + plugin: pluginSpec, + success: false, + action: 'failed', + error: accessError, + }; + } - // Use the actual marketplace name (may differ from parsed name if found by source location) - const marketplaceName = marketplace.name; + // Registry keys are authoritative. Manifest names can differ for legacy or + // manually edited entries, especially when lookup fell back to source. + const marketplaceKey = registration.key; // Parse marketplace manifest to determine if plugin is embedded or external const manifestResult = await deps.parseMarketplaceManifest(marketplace.path); if (!manifestResult.success || !manifestResult.data) { // No manifest - update the marketplace itself (plugin might be in directory) - const updateResults = await deps.updateMarketplace(marketplaceName); + const updateResults = await deps.updateMarketplace(marketplaceKey); const result = updateResults[0]; return { plugin: pluginSpec, @@ -488,7 +507,7 @@ export async function updatePlugin( if (!pluginEntry) { // Plugin not in manifest - update marketplace and hope for the best - const updateResults = await deps.updateMarketplace(marketplaceName); + const updateResults = await deps.updateMarketplace(marketplaceKey); const result = updateResults[0]; return { plugin: pluginSpec, @@ -501,7 +520,7 @@ export async function updatePlugin( // Check if embedded (string path) or external (url object) if (typeof pluginEntry.source === 'string') { // Embedded plugin - update the marketplace - const updateResults = await deps.updateMarketplace(marketplaceName); + const updateResults = await deps.updateMarketplace(marketplaceKey); const result = updateResults[0]; return { plugin: pluginSpec, @@ -516,7 +535,7 @@ export async function updatePlugin( // Update the marketplace first (in case manifest changed) if (marketplace.source.type === 'github') { - await deps.updateMarketplace(marketplaceName); + await deps.updateMarketplace(marketplaceKey); } // Update the external plugin cache diff --git a/src/core/sync.ts b/src/core/sync.ts index 2463650..640acbf 100644 --- a/src/core/sync.ts +++ b/src/core/sync.ts @@ -72,6 +72,7 @@ import { getRegistryPath, getProjectRegistryPath, getMarketplace, + getMarketplaceAccessError, } from './marketplace.js'; import { loadSyncState, @@ -2524,6 +2525,7 @@ export async function seedFetchCacheFromMarketplaces( const entry = await getMarketplace(result.name); if (!entry || entry.source.type !== 'github') continue; + if (getMarketplaceAccessError(entry)) continue; // Seed the bare key (owner/repo without branch) seedFetchCache(entry.source.location, entry.path); diff --git a/src/core/workspace-modify.ts b/src/core/workspace-modify.ts index 804f10b..29fc682 100644 --- a/src/core/workspace-modify.ts +++ b/src/core/workspace-modify.ts @@ -20,6 +20,7 @@ import { } from '../utils/plugin-path.js'; import { getMarketplace, + getMarketplaceAccessError, isPluginSpec, parsePluginSpec, resolvePluginSpecWithAutoRegister, @@ -1091,6 +1092,7 @@ export async function resolveGitHubIdentity( const marketplace = await getMarketplace(parsed.marketplaceName); if (!marketplace) return null; + if (getMarketplaceAccessError(marketplace)) return null; const manifestResult = await parseMarketplaceManifest(marketplace.path); if (!manifestResult.success) return null; diff --git a/tests/unit/core/marketplace-add-branch.test.ts b/tests/unit/core/marketplace-add-branch.test.ts index a99616f..bb0d14d 100644 --- a/tests/unit/core/marketplace-add-branch.test.ts +++ b/tests/unit/core/marketplace-add-branch.test.ts @@ -1,6 +1,14 @@ import { describe, it, expect, beforeEach, afterEach, mock } from 'bun:test'; -import { mkdirSync, rmSync } from 'node:fs'; -import { join } from 'node:path'; +import { + existsSync, + lstatSync, + mkdirSync, + readFileSync, + rmSync, + symlinkSync, + writeFileSync, +} from 'node:fs'; +import { join, parse } from 'node:path'; import { tmpdir } from 'node:os'; import { stubHomeDir } from '../../helpers/env.js'; @@ -48,6 +56,8 @@ mock.module('simple-git', () => ({ })); const { addMarketplace, loadRegistry } = await import('../../../src/core/marketplace.js'); +const { cloneTo } = await import('../../../src/core/git.js'); +const cloneToMock = cloneTo as ReturnType; describe('addMarketplace branch support', () => { let restoreHomeDir: () => void; @@ -58,6 +68,13 @@ describe('addMarketplace branch support', () => { restoreHomeDir = stubHomeDir(testHome); mkdirSync(join(testHome, '.allagents'), { recursive: true }); cloneCalls.length = 0; + cloneToMock.mockImplementation( + (url: string, dest: string, ref?: string) => { + cloneCalls.push({ url, dest, ref }); + mkdirSync(dest, { recursive: true }); + return Promise.resolve(); + }, + ); }); afterEach(() => { @@ -132,4 +149,206 @@ describe('addMarketplace branch support', () => { expect(cloneCall).toBeDefined(); expect(cloneCall!.ref).toBeUndefined(); }); + + it('should reject a remote marketplace with an unsafe custom name', async () => { + const result = await addMarketplace('owner/repo', '../../..'); + + expect(result.success).toBe(false); + expect(result.error).toContain('Invalid marketplace name'); + expect(cloneCalls).toHaveLength(0); + expect((await loadRegistry()).marketplaces).toEqual({}); + }); + + it('should reject a remote marketplace with an unsafe derived name', async () => { + const result = await addMarketplace('owner/..'); + + expect(result.success).toBe(false); + expect(result.error).toContain('Invalid marketplace name'); + expect(cloneCalls).toHaveLength(0); + expect((await loadRegistry()).marketplaces).toEqual({}); + }); + + it('should reject marketplace names that alias or special-case Windows paths', async () => { + for (const name of ['CON', 'repo.', 'repo ', 'bad:name']) { + const result = await addMarketplace('owner/repo', name); + expect(result.success).toBe(false); + expect(result.error).toContain('Invalid marketplace name'); + } + + expect(cloneCalls).toHaveLength(0); + expect((await loadRegistry()).marketplaces).toEqual({}); + }); + + it('should allow local aliases that are valid registry keys but unsafe remote cache names', async () => { + const localPath = join(testHome, 'local-marketplace'); + mkdirSync(localPath, { recursive: true }); + + for (const alias of ['foo:bar', 'CON']) { + const result = await addMarketplace(localPath, alias); + expect(result.success).toBe(true); + expect(result.marketplace?.name).toBe(alias); + } + + const registry = await loadRegistry(); + expect(registry.marketplaces['foo:bar']).toBeDefined(); + expect(registry.marketplaces.CON).toBeDefined(); + expect(cloneCalls).toHaveLength(0); + }); + + it('should persist a prototype-named remote alias as an own registry entry', async () => { + const result = await addMarketplace('owner/repo', '__proto__'); + + expect(result.success).toBe(true); + const registry = await loadRegistry(); + expect(Object.hasOwn(registry.marketplaces, '__proto__')).toBe(true); + expect(registry.marketplaces['__proto__'].name).toBe('__proto__'); + expect(existsSync(registry.marketplaces['__proto__'].path)).toBe(true); + }); + + it('should allow a platform-valid local manifest name', async () => { + const localPath = join(testHome, 'local-manifest-marketplace'); + mkdirSync(join(localPath, '.claude-plugin'), { recursive: true }); + writeFileSync( + join(localPath, '.claude-plugin', 'marketplace.json'), + JSON.stringify({ name: 'foo:bar', plugins: [] }), + ); + + const result = await addMarketplace(localPath); + + expect(result.success).toBe(true); + expect(result.marketplace?.name).toBe('foo:bar'); + expect((await loadRegistry()).marketplaces['foo:bar']).toBeDefined(); + expect(cloneCalls).toHaveLength(0); + }); + + it('should reject ambiguous local aliases', async () => { + const localPath = join(testHome, 'local-marketplace'); + mkdirSync(localPath, { recursive: true }); + + for (const alias of ['.', '..', 'bad/name', 'bad\\name', 'bad\u0001name']) { + const result = await addMarketplace(localPath, alias); + expect(result.success).toBe(false); + expect(result.error).toContain('Invalid marketplace name'); + } + + expect((await loadRegistry()).marketplaces).toEqual({}); + expect(cloneCalls).toHaveLength(0); + }); + + it('should reject broad local roots without modifying them', async () => { + const markerPath = join(testHome, 'home-marker.txt'); + const homeLink = join(testHome, 'home-link'); + const rootLink = join(testHome, 'root-link'); + writeFileSync(markerPath, 'keep'); + symlinkSync(testHome, homeLink, 'dir'); + symlinkSync(parse(testHome).root, rootLink, 'dir'); + + for (const source of [ + testHome, + parse(testHome).root, + homeLink, + rootLink, + ]) { + const result = await addMarketplace(source); + expect(result.success).toBe(false); + expect(result.error).toContain( + 'must be a specific directory, not a filesystem root or the user\'s home directory', + ); + } + + expect(readFileSync(markerPath, 'utf-8')).toBe('keep'); + expect(lstatSync(homeLink).isSymbolicLink()).toBe(true); + expect(lstatSync(rootLink).isSymbolicLink()).toBe(true); + expect((await loadRegistry()).marketplaces).toEqual({}); + }); + + it('should reject a symlink at a managed remote cache path', async () => { + const targetPath = join(testHome, 'user-owned-target'); + const cachePath = join( + testHome, + '.allagents', + 'plugins', + 'marketplaces', + 'repo', + ); + mkdirSync(targetPath, { recursive: true }); + writeFileSync(join(targetPath, 'marker.txt'), 'keep'); + mkdirSync(join(cachePath, '..'), { recursive: true }); + symlinkSync(targetPath, cachePath, 'dir'); + + const result = await addMarketplace('owner/repo'); + + expect(result.success).toBe(false); + expect(result.error).toContain('cannot be a symbolic link'); + expect(lstatSync(cachePath).isSymbolicLink()).toBe(true); + expect(readFileSync(join(targetPath, 'marker.txt'), 'utf-8')).toBe('keep'); + expect(cloneCalls).toHaveLength(0); + expect((await loadRegistry()).marketplaces).toEqual({}); + }); + + it('should reject relocation of only the internal marketplace cache root', async () => { + const targetPath = join(testHome, 'user-owned-cache-root'); + const marketplaceRoot = join( + testHome, + '.allagents', + 'plugins', + 'marketplaces', + ); + mkdirSync(targetPath, { recursive: true }); + writeFileSync(join(targetPath, 'marker.txt'), 'keep'); + mkdirSync(join(marketplaceRoot, '..'), { recursive: true }); + symlinkSync(targetPath, marketplaceRoot, 'dir'); + + const result = await addMarketplace('owner/repo'); + + expect(result.success).toBe(false); + expect(result.error).toContain( + 'cache root is not a safe AllAgents-owned directory', + ); + expect(lstatSync(marketplaceRoot).isSymbolicLink()).toBe(true); + expect(readFileSync(join(targetPath, 'marker.txt'), 'utf-8')).toBe('keep'); + expect(cloneCalls).toHaveLength(0); + expect((await loadRegistry()).marketplaces).toEqual({}); + }); + + it('should remove a newly cloned cache with an unsafe manifest name', async () => { + cloneToMock.mockImplementation( + (url: string, dest: string, ref?: string) => { + cloneCalls.push({ url, dest, ref }); + mkdirSync(join(dest, '.claude-plugin'), { recursive: true }); + writeFileSync( + join(dest, '.claude-plugin', 'marketplace.json'), + JSON.stringify({ name: '../../..', plugins: [] }), + ); + return Promise.resolve(); + }, + ); + + const result = await addMarketplace('owner/unsafe-manifest'); + + expect(result.success).toBe(false); + expect(result.error).toContain('Invalid marketplace name'); + expect(existsSync(cloneCalls[0].dest)).toBe(false); + }); + + it('should not delete a pre-existing cache with an unsafe manifest name', async () => { + const cachePath = join( + testHome, + '.allagents', + 'plugins', + 'marketplaces', + 'unsafe-manifest', + ); + mkdirSync(join(cachePath, '.claude-plugin'), { recursive: true }); + writeFileSync( + join(cachePath, '.claude-plugin', 'marketplace.json'), + JSON.stringify({ name: '../../..', plugins: [] }), + ); + writeFileSync(join(cachePath, 'marker.txt'), 'keep'); + + const result = await addMarketplace('owner/unsafe-manifest'); + + expect(result.success).toBe(false); + expect(readFileSync(join(cachePath, 'marker.txt'), 'utf-8')).toBe('keep'); + }); }); diff --git a/tests/unit/core/marketplace-refresh.test.ts b/tests/unit/core/marketplace-refresh.test.ts index 0026954..8ec7c51 100644 --- a/tests/unit/core/marketplace-refresh.test.ts +++ b/tests/unit/core/marketplace-refresh.test.ts @@ -1,5 +1,13 @@ import { describe, it, expect, beforeEach, afterEach, mock } from 'bun:test'; -import { mkdirSync, writeFileSync, rmSync, existsSync } from 'node:fs'; +import { + mkdirSync, + writeFileSync, + readFileSync, + rmSync, + existsSync, + lstatSync, + symlinkSync, +} from 'node:fs'; import { join } from 'node:path'; import { tmpdir } from 'node:os'; import { stubHomeDir } from '../../helpers/env.js'; @@ -28,9 +36,13 @@ mock.module('../../../src/core/git.js', () => ({ cleanupTempDir: mock(() => Promise.resolve()), })); -const { resolvePluginSpecWithAutoRegister } = await import( - '../../../src/core/marketplace.js' -); +const { + listMarketplacePlugins, + resolvePluginSpec, + resolvePluginSpecWithAutoRegister, +} = await import('../../../src/core/marketplace.js'); +const { cloneTo } = await import('../../../src/core/git.js'); +const cloneToMock = cloneTo as ReturnType; describe('resolvePluginSpecWithAutoRegister refresh', () => { let restoreHomeDir: () => void; @@ -40,6 +52,13 @@ describe('resolvePluginSpecWithAutoRegister refresh', () => { testHome = join(tmpdir(), `marketplace-refresh-test-${Date.now()}`); restoreHomeDir = stubHomeDir(testHome); cloneToCalls.length = 0; + cloneToMock.mockImplementation( + (url: string, path: string, branch?: string) => { + cloneToCalls.push({ url, path, branch }); + mkdirSync(path, { recursive: true }); + return Promise.resolve(); + }, + ); }); afterEach(() => { @@ -98,10 +117,7 @@ describe('resolvePluginSpecWithAutoRegister refresh', () => { // Override cloneTo to create the directory with the new plugin included cloneToCalls.length = 0; - const originalCloneTo = ( - await import('../../../src/core/git.js') - ).cloneTo as ReturnType; - originalCloneTo.mockImplementation( + cloneToMock.mockImplementation( (url: string, path: string, branch?: string) => { cloneToCalls.push({ url, path, branch }); // Simulate fresh clone that now includes the missing plugin @@ -138,6 +154,81 @@ describe('resolvePluginSpecWithAutoRegister refresh', () => { expect(cloneToCalls[0].url).toContain('owner/test-mp'); }); + it('should refresh a canonical marketplace whose cache uses the repository name', async () => { + const mpPath = setupMarketplace('repo-name', []); + writeFileSync( + join(mpPath, '.claude-plugin', 'marketplace.json'), + JSON.stringify({ name: 'canonical-name', plugins: [] }), + ); + setupRegistry({ + 'canonical-name': { + name: 'canonical-name', + source: { type: 'github', location: 'owner/repo-name' }, + path: mpPath, + }, + }); + cloneToMock.mockImplementation( + (_url: string, path: string, _branch?: string) => { + mkdirSync(join(path, '.claude-plugin'), { recursive: true }); + writeFileSync( + join(path, '.claude-plugin', 'marketplace.json'), + JSON.stringify({ + name: 'canonical-name', + plugins: [{ name: 'new-plugin', source: './plugins/new-plugin' }], + }), + ); + mkdirSync(join(path, 'plugins', 'new-plugin'), { recursive: true }); + return Promise.resolve(); + }, + ); + + const result = await resolvePluginSpecWithAutoRegister( + 'new-plugin@canonical-name', + ); + + expect(result.success).toBe(true); + expect(result.pluginName).toBe('new-plugin'); + const registry = JSON.parse( + readFileSync(join(testHome, '.allagents', 'marketplaces.json'), 'utf-8'), + ); + expect(registry.marketplaces['canonical-name'].path).toBe(mpPath); + }); + + it('should refresh a malformed alias under its exact registry key', async () => { + const mpPath = setupMarketplace('repo-name', []); + setupRegistry({ + alias: { + name: 'canonical-name', + source: { type: 'github', location: 'owner/repo-name' }, + path: mpPath, + }, + }); + cloneToMock.mockImplementation( + (_url: string, path: string, _branch?: string) => { + mkdirSync(join(path, '.claude-plugin'), { recursive: true }); + writeFileSync( + join(path, '.claude-plugin', 'marketplace.json'), + JSON.stringify({ + name: 'canonical-name', + plugins: [{ name: 'new-plugin', source: './plugins/new-plugin' }], + }), + ); + mkdirSync(join(path, 'plugins', 'new-plugin'), { recursive: true }); + return Promise.resolve(); + }, + ); + + const result = await resolvePluginSpecWithAutoRegister('new-plugin@alias'); + + expect(result.success).toBe(true); + const registry = JSON.parse( + readFileSync(join(testHome, '.allagents', 'marketplaces.json'), 'utf-8'), + ); + expect(Object.keys(registry.marketplaces)).toEqual(['alias']); + expect(registry.marketplaces.alias.name).toBe('canonical-name'); + expect(registry.marketplaces.alias.path).toBe(mpPath); + }); + it('should not refresh when offline', async () => { const mpPath = setupMarketplace('test-mp', []); setupRegistry({ @@ -177,6 +268,32 @@ describe('resolvePluginSpecWithAutoRegister refresh', () => { expect(cloneToCalls.length).toBe(0); }); + it('should accept a remote cache beneath an intentionally relocated AllAgents directory', async () => { + const relocatedAllagents = join(testHome, 'relocated-allagents'); + mkdirSync(relocatedAllagents, { recursive: true }); + symlinkSync(relocatedAllagents, join(testHome, '.allagents'), 'dir'); + const mpPath = setupMarketplace('test-mp', [ + { name: 'existing-plugin', source: './plugins/existing-plugin' }, + ]); + setupRegistry({ + 'test-mp': { + name: 'test-mp', + source: { type: 'github', location: 'owner/test-mp' }, + path: mpPath, + }, + }); + + const result = await resolvePluginSpecWithAutoRegister( + 'existing-plugin@test-mp', + { offline: true }, + ); + + expect(result.success).toBe(true); + expect(result.pluginName).toBe('existing-plugin'); + expect(lstatSync(join(testHome, '.allagents')).isSymbolicLink()).toBe(true); + expect(cloneToCalls).toHaveLength(0); + }); + it('should delete old cache directory during refresh', async () => { const mpPath = setupMarketplace('test-mp', []); setupRegistry({ @@ -191,10 +308,7 @@ describe('resolvePluginSpecWithAutoRegister refresh', () => { // Create a marker file in the old directory writeFileSync(join(mpPath, 'old-marker.txt'), 'old'); - const originalCloneTo = ( - await import('../../../src/core/git.js') - ).cloneTo as ReturnType; - originalCloneTo.mockImplementation( + cloneToMock.mockImplementation( (_url: string, path: string, _branch?: string) => { // By the time clone is called, old directory should be deleted // (clone target is the new path based on marketplace name) @@ -208,4 +322,318 @@ describe('resolvePluginSpecWithAutoRegister refresh', () => { // Old directory should be gone (rm was called before clone) expect(existsSync(join(mpPath, 'old-marker.txt'))).toBe(false); }); + + it('should preserve the registry entry and cache when refresh fails', async () => { + const mpPath = setupMarketplace('test-mp', []); + writeFileSync(join(mpPath, 'old-marker.txt'), 'old'); + setupRegistry({ + 'test-mp': { + name: 'test-mp', + source: { type: 'github', location: 'owner/test-mp' }, + path: mpPath, + lastUpdated: '2024-01-01T00:00:00.000Z', + }, + }); + + cloneToMock.mockImplementation(() => + Promise.reject(new Error('clone failed')), + ); + + const result = await resolvePluginSpecWithAutoRegister( + 'missing-plugin@test-mp', + ); + + expect(result.success).toBe(false); + expect(result.error).toContain('clone failed'); + expect(readFileSync(join(mpPath, 'old-marker.txt'), 'utf-8')).toBe('old'); + const registry = JSON.parse( + readFileSync(join(testHome, '.allagents', 'marketplaces.json'), 'utf-8'), + ); + expect(registry.marketplaces['test-mp']).toEqual({ + name: 'test-mp', + source: { type: 'github', location: 'owner/test-mp' }, + path: mpPath, + lastUpdated: '2024-01-01T00:00:00.000Z', + }); + }); + + it('should restore the old cache without overwriting a concurrent registry repair', async () => { + const mpPath = setupMarketplace('test-mp', []); + writeFileSync(join(mpPath, 'old-marker.txt'), 'old'); + setupRegistry({ + 'test-mp': { + name: 'test-mp', + source: { type: 'github', location: 'owner/test-mp' }, + path: mpPath, + }, + }); + const repairedPath = join(testHome, 'repaired-local-marketplace'); + mkdirSync(repairedPath, { recursive: true }); + cloneToMock.mockImplementation((_url: string, path: string) => { + mkdirSync(path, { recursive: true }); + setupRegistry({ + 'test-mp': { + name: 'test-mp', + source: { type: 'local', location: repairedPath }, + path: repairedPath, + }, + }); + return Promise.resolve(); + }); + + const result = await resolvePluginSpecWithAutoRegister( + 'missing-plugin@test-mp', + ); + + expect(result.success).toBe(false); + expect(result.error).toContain("registration 'test-mp' changed during refresh"); + expect(readFileSync(join(mpPath, 'old-marker.txt'), 'utf-8')).toBe('old'); + const registry = JSON.parse( + readFileSync(join(testHome, '.allagents', 'marketplaces.json'), 'utf-8'), + ); + expect(registry.marketplaces['test-mp']).toEqual({ + name: 'test-mp', + source: { type: 'local', location: repairedPath }, + path: repairedPath, + }); + }); + + it('should restore the old cache when replacing the staged clone fails', async () => { + const mpPath = setupMarketplace('test-mp', []); + writeFileSync(join(mpPath, 'old-marker.txt'), 'old'); + setupRegistry({ + 'test-mp': { + name: 'test-mp', + source: { type: 'github', location: 'owner/test-mp' }, + path: mpPath, + }, + }); + cloneToMock.mockImplementation(() => Promise.resolve()); + + const result = await resolvePluginSpecWithAutoRegister( + 'missing-plugin@test-mp', + ); + + expect(result.success).toBe(false); + expect(result.error).toContain('Failed to replace marketplace cache'); + expect(readFileSync(join(mpPath, 'old-marker.txt'), 'utf-8')).toBe('old'); + }); + + it('should remove only an unsafe remote registry entry without deleting its path', async () => { + const homeMarker = join(testHome, 'home-marker.txt'); + mkdirSync(testHome, { recursive: true }); + writeFileSync(homeMarker, 'keep'); + setupRegistry({ + unsafe: { + name: 'unsafe', + source: { type: 'github', location: 'owner/unsafe' }, + path: testHome, + lastUpdated: '2024-01-01T00:00:00.000Z', + }, + unrelated: { + name: 'unrelated', + source: { type: 'local', location: '/tmp/unrelated' }, + path: '/tmp/unrelated', + }, + }); + + const result = await resolvePluginSpecWithAutoRegister('missing@unsafe'); + + expect(result.success).toBe(false); + expect(result.error).toContain('Removed invalid marketplace registration'); + expect(result.error).toContain('Refused to access or delete unmanaged path'); + expect(readFileSync(homeMarker, 'utf-8')).toBe('keep'); + const registry = JSON.parse( + readFileSync(join(testHome, '.allagents', 'marketplaces.json'), 'utf-8'), + ); + expect(registry.marketplaces.unsafe).toBeUndefined(); + expect(registry.marketplaces.unrelated).toEqual({ + name: 'unrelated', + source: { type: 'local', location: '/tmp/unrelated' }, + path: '/tmp/unrelated', + }); + }); + + it('should refuse to list plugins through an unsafe registry path', async () => { + mkdirSync(join(testHome, '.claude-plugin'), { recursive: true }); + writeFileSync( + join(testHome, '.claude-plugin', 'marketplace.json'), + JSON.stringify({ + name: 'unsafe', + plugins: [{ name: 'should-not-be-read', source: './plugin' }], + }), + ); + setupRegistry({ + unsafe: { + name: 'unsafe', + source: { type: 'github', location: 'owner/unsafe' }, + path: testHome, + }, + }); + + const result = await listMarketplacePlugins('unsafe'); + + expect(result.plugins).toEqual([]); + expect(result.warnings).toEqual([ + `Refused to access unmanaged marketplace path: ${testHome}`, + ]); + }); + + it('should refuse direct plugin resolution through an unsafe registry path', async () => { + mkdirSync(join(testHome, '.claude-plugin'), { recursive: true }); + writeFileSync( + join(testHome, '.claude-plugin', 'marketplace.json'), + JSON.stringify({ + name: 'unsafe', + plugins: [{ name: 'should-not-be-read', source: './plugin' }], + }), + ); + mkdirSync(join(testHome, 'plugin'), { recursive: true }); + setupRegistry({ + unsafe: { + name: 'unsafe', + source: { type: 'github', location: 'owner/unsafe' }, + path: testHome, + }, + }); + + const result = await resolvePluginSpec('should-not-be-read@unsafe'); + + expect(result).toBeNull(); + }); + + it('should remove an unsafe project alias without deleting a safe user entry', async () => { + const workspacePath = join(testHome, 'workspace'); + const safePath = join(testHome, 'safe-local-marketplace'); + mkdirSync(safePath, { recursive: true }); + setupRegistry({ + victim: { + name: 'victim', + source: { type: 'local', location: safePath }, + path: safePath, + }, + }); + const projectRegistryPath = join( + workspacePath, + '.allagents', + 'marketplaces.json', + ); + mkdirSync(join(projectRegistryPath, '..'), { recursive: true }); + writeFileSync( + projectRegistryPath, + JSON.stringify({ + version: 1, + marketplaces: { + alias: { + name: 'victim', + source: { type: 'github', location: 'owner/unsafe' }, + path: testHome, + }, + }, + }), + ); + + const result = await resolvePluginSpecWithAutoRegister( + 'missing@owner/unsafe', + { workspacePath }, + ); + + expect(result.success).toBe(false); + expect(result.error).toContain("registration 'alias'"); + const userRegistry = JSON.parse( + readFileSync(join(testHome, '.allagents', 'marketplaces.json'), 'utf-8'), + ); + const projectRegistry = JSON.parse( + readFileSync(projectRegistryPath, 'utf-8'), + ); + expect(userRegistry.marketplaces.victim).toBeDefined(); + expect(projectRegistry.marketplaces.alias).toBeUndefined(); + expect(cloneToCalls).toHaveLength(0); + }); + + it('should remove a broad local registration without accessing its home directory', async () => { + const homeMarker = join(testHome, 'home-marker.txt'); + mkdirSync(testHome, { recursive: true }); + writeFileSync(homeMarker, 'keep'); + setupRegistry({ + unsafe: { + name: 'unsafe', + source: { type: 'local', location: testHome }, + path: testHome, + }, + }); + + const result = await resolvePluginSpecWithAutoRegister('missing@unsafe'); + + expect(result.success).toBe(false); + expect(result.error).toContain('Removed invalid marketplace registration'); + expect(readFileSync(homeMarker, 'utf-8')).toBe('keep'); + const registry = JSON.parse( + readFileSync(join(testHome, '.allagents', 'marketplaces.json'), 'utf-8'), + ); + expect(registry.marketplaces.unsafe).toBeUndefined(); + expect(cloneToCalls).toHaveLength(0); + }); + + it('should remove a symlinked remote registration without touching the link or target', async () => { + const targetPath = join(testHome, 'user-owned-target'); + const cachePath = join( + testHome, + '.allagents', + 'plugins', + 'marketplaces', + 'unsafe', + ); + mkdirSync(targetPath, { recursive: true }); + writeFileSync(join(targetPath, 'marker.txt'), 'keep'); + mkdirSync(join(cachePath, '..'), { recursive: true }); + symlinkSync(targetPath, cachePath, 'dir'); + setupRegistry({ + unsafe: { + name: 'unsafe', + source: { type: 'github', location: 'owner/unsafe' }, + path: cachePath, + }, + }); + + const result = await resolvePluginSpecWithAutoRegister('missing@unsafe'); + + expect(result.success).toBe(false); + expect(result.error).toContain('Removed invalid marketplace registration'); + expect(lstatSync(cachePath).isSymbolicLink()).toBe(true); + expect(readFileSync(join(targetPath, 'marker.txt'), 'utf-8')).toBe('keep'); + const registry = JSON.parse( + readFileSync(join(testHome, '.allagents', 'marketplaces.json'), 'utf-8'), + ); + expect(registry.marketplaces.unsafe).toBeUndefined(); + expect(cloneToCalls).toHaveLength(0); + }); + + it('should not access a sibling marketplace cache referenced by an invalid entry', async () => { + const otherPath = setupMarketplace('other', []); + const markerPath = join(otherPath, 'other-marker.txt'); + writeFileSync(markerPath, 'keep'); + setupRegistry({ + unsafe: { + name: 'unsafe', + source: { type: 'github', location: 'owner/unsafe' }, + path: otherPath, + }, + other: { + name: 'other', + source: { type: 'github', location: 'owner/other' }, + path: otherPath, + }, + }); + + const result = await resolvePluginSpecWithAutoRegister('missing@unsafe'); + + expect(result.success).toBe(false); + expect(readFileSync(markerPath, 'utf-8')).toBe('keep'); + const registry = JSON.parse( + readFileSync(join(testHome, '.allagents', 'marketplaces.json'), 'utf-8'), + ); + expect(registry.marketplaces.unsafe).toBeUndefined(); + expect(registry.marketplaces.other).toBeDefined(); + }); }); diff --git a/tests/unit/core/marketplace-remove-cascade.test.ts b/tests/unit/core/marketplace-remove-cascade.test.ts index feae5ff..8911f18 100644 --- a/tests/unit/core/marketplace-remove-cascade.test.ts +++ b/tests/unit/core/marketplace-remove-cascade.test.ts @@ -1,5 +1,13 @@ import { describe, it, expect, beforeEach, afterEach } from 'bun:test'; -import { mkdtemp, rm, mkdir, writeFile, readFile } from 'node:fs/promises'; +import { + lstat, + mkdtemp, + rm, + mkdir, + readFile, + symlink, + writeFile, +} from 'node:fs/promises'; import { join } from 'node:path'; import { tmpdir } from 'node:os'; import { dump, load } from 'js-yaml'; @@ -122,6 +130,31 @@ describe('removeMarketplace cascade', () => { expect(result.removedUserPlugins).toBeUndefined(); }); + it('should distinguish own registry aliases from Object prototype names', async () => { + const localPath = join(testDir, 'local-marketplace'); + await mkdir(localPath, { recursive: true }); + const marketplaces = JSON.parse(JSON.stringify({ + ['__proto__']: { + name: '__proto__', + source: { type: 'local', location: localPath }, + path: localPath, + }, + })) as MarketplaceRegistry['marketplaces']; + await writeRegistry(marketplaces); + + const missingResult = await removeMarketplace('toString'); + expect(missingResult.success).toBe(false); + expect(missingResult.error).toContain("Marketplace 'toString' not found"); + + const result = await removeMarketplace('__proto__'); + expect(result.success).toBe(true); + expect(await lstat(localPath)).toBeDefined(); + const registryContent = JSON.parse( + await readFile(join(testDir, '.allagents', 'marketplaces.json'), 'utf-8'), + ); + expect(Object.hasOwn(registryContent.marketplaces, '__proto__')).toBe(false); + }); + it('should succeed when no user plugins reference the marketplace', async () => { await writeRegistry({ 'my-marketplace': { @@ -215,4 +248,61 @@ describe('removeMarketplace cascade', () => { // Local source directory must NOT be deleted expect(existsSync(localSourceDir)).toBe(true); }); + + it('should remove an unsafe remote entry without deleting its referenced path', async () => { + const markerPath = join(testDir, 'home-marker.txt'); + await writeFile(markerPath, 'keep', 'utf-8'); + await writeRegistry({ + unsafe: { + name: 'unsafe', + source: { type: 'github', location: 'owner/unsafe' }, + path: testDir, + }, + unrelated: { + name: 'unrelated', + source: { type: 'local', location: '/tmp/unrelated' }, + path: '/tmp/unrelated', + }, + }); + + const result = await removeMarketplace('unsafe'); + + expect(result.success).toBe(true); + expect(result.warnings).toEqual([ + `Refused to delete unmanaged marketplace path: ${testDir}`, + ]); + expect(await readFile(markerPath, 'utf-8')).toBe('keep'); + const registry = JSON.parse( + await readFile(join(testDir, '.allagents', 'marketplaces.json'), 'utf-8'), + ) as MarketplaceRegistry; + expect(registry.marketplaces.unsafe).toBeUndefined(); + expect(registry.marketplaces.unrelated).toBeDefined(); + }); + + it('should warn without deleting a dangling remote cache symlink', async () => { + const cachePath = join( + testDir, + '.allagents', + 'plugins', + 'marketplaces', + 'dangling', + ); + await mkdir(join(cachePath, '..'), { recursive: true }); + await symlink(join(testDir, 'missing-target'), cachePath, 'dir'); + await writeRegistry({ + dangling: { + name: 'dangling', + source: { type: 'github', location: 'owner/dangling' }, + path: cachePath, + }, + }); + + const result = await removeMarketplace('dangling'); + + expect(result.success).toBe(true); + expect(result.warnings).toEqual([ + `Refused to delete unmanaged marketplace path: ${cachePath}`, + ]); + expect((await lstat(cachePath)).isSymbolicLink()).toBe(true); + }); }); diff --git a/tests/unit/core/marketplace-scope.test.ts b/tests/unit/core/marketplace-scope.test.ts index e2ced25..c607709 100644 --- a/tests/unit/core/marketplace-scope.test.ts +++ b/tests/unit/core/marketplace-scope.test.ts @@ -341,6 +341,39 @@ describe('scope-aware registry loading and saving', () => { expect(result.entries[0].source.location).toBe('/project/shared'); expect(result.overrides).toEqual(['shared']); }); + + it('uses registry keys for project precedence when embedded names differ', async () => { + const userPath = join(tmpDir, 'user-marketplaces.json'); + const projectPath = join(tmpDir, 'project-marketplaces.json'); + + writeFileSync(userPath, JSON.stringify({ + version: 1, + marketplaces: { + alias: { + name: 'user-canonical', + source: { type: 'github', location: 'user-org/repo' }, + path: '/user/repo', + }, + }, + } satisfies MarketplaceRegistry)); + writeFileSync(projectPath, JSON.stringify({ + version: 1, + marketplaces: { + alias: { + name: 'project-canonical', + source: { type: 'local', location: '/project/repo' }, + path: '/project/repo', + }, + }, + } satisfies MarketplaceRegistry)); + + const result = await listMarketplacesWithScope(userPath, projectPath); + + expect(result.entries).toHaveLength(1); + expect(result.entries[0].name).toBe('project-canonical'); + expect(result.entries[0].scope).toBe('project'); + expect(result.overrides).toEqual(['alias']); + }); }); }); diff --git a/tests/unit/core/marketplace-update.test.ts b/tests/unit/core/marketplace-update.test.ts index 3f5c222..984674b 100644 --- a/tests/unit/core/marketplace-update.test.ts +++ b/tests/unit/core/marketplace-update.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect, beforeEach, afterEach, mock } from 'bun:test'; -import { mkdirSync, writeFileSync, rmSync } from 'node:fs'; +import { mkdirSync, readFileSync, writeFileSync, rmSync } from 'node:fs'; import { join } from 'node:path'; import { tmpdir } from 'node:os'; import { stubHomeDir } from '../../helpers/env.js'; @@ -37,11 +37,21 @@ mock.module('simple-git', () => ({ // Mock the git module's pull function mock.module('../../../src/core/git.js', () => ({ + createGitEnv: () => ({ + ...process.env, + GIT_TERMINAL_PROMPT: '0', + GIT_LFS_SKIP_SMUDGE: '1', + }), pull: mock((path: string) => { pullCalls.push({ path }); return Promise.resolve(); }), + cloneToTemp: mock(() => Promise.resolve('/tmp/fake')), cloneTo: mock(() => Promise.resolve()), + repoExists: mock(() => Promise.resolve(true)), + refExists: mock(() => Promise.resolve(true)), + cleanupTempDir: mock(() => Promise.resolve()), + classifyError: (error: Error) => error, gitHubUrl: (owner: string, repo: string) => `https://github.com/${owner}/${repo}.git`, GitCloneError: class extends Error {}, })); @@ -198,4 +208,74 @@ describe('updateMarketplace', () => { // Should pull expect(pullCalls.length).toBe(1); }); + + it('should remove an unsafe registration without opening its directory', async () => { + const markerPath = join(testHome, 'home-marker.txt'); + writeFileSync(markerPath, 'keep'); + const registryPath = join(testHome, '.allagents', 'marketplaces.json'); + writeFileSync( + registryPath, + JSON.stringify({ + version: 1, + marketplaces: { + unsafe: { + name: 'unsafe', + source: { type: 'github', location: 'owner/unsafe' }, + path: testHome, + }, + }, + }), + ); + + const results = await updateMarketplace('unsafe'); + + expect(results).toHaveLength(1); + expect(results[0].success).toBe(false); + expect(results[0].error).toContain( + 'Removed invalid marketplace registration', + ); + expect(readFileSync(markerPath, 'utf-8')).toBe('keep'); + expect(simpleGitCalls).toHaveLength(0); + expect(pullCalls).toHaveLength(0); + const registry = JSON.parse(readFileSync(registryPath, 'utf-8')); + expect(registry.marketplaces.unsafe).toBeUndefined(); + }); + + it('should remove an unsafe alias by exact key without rewriting a safe entry', async () => { + const safePath = join(testHome, 'safe-local-marketplace'); + mkdirSync(safePath, { recursive: true }); + const registryPath = join(testHome, '.allagents', 'marketplaces.json'); + writeFileSync( + registryPath, + JSON.stringify({ + version: 1, + marketplaces: { + victim: { + name: 'other', + source: { type: 'local', location: safePath }, + path: safePath, + }, + alias: { + name: 'victim', + source: { type: 'github', location: 'owner/unsafe' }, + path: testHome, + }, + }, + }), + ); + + const results = await updateMarketplace(); + + expect(results).toHaveLength(2); + const registry = JSON.parse(readFileSync(registryPath, 'utf-8')); + expect(registry.marketplaces.alias).toBeUndefined(); + expect(registry.marketplaces.victim).toEqual({ + name: 'other', + source: { type: 'local', location: safePath }, + path: safePath, + }); + expect(registry.marketplaces.other).toBeUndefined(); + expect(simpleGitCalls).toHaveLength(0); + expect(pullCalls).toHaveLength(0); + }); }); diff --git a/tests/unit/core/marketplace-version.test.ts b/tests/unit/core/marketplace-version.test.ts index 32656d3..59a6485 100644 --- a/tests/unit/core/marketplace-version.test.ts +++ b/tests/unit/core/marketplace-version.test.ts @@ -44,6 +44,12 @@ const { getMarketplaceVersion } = await import( '../../../src/core/marketplace.js' ); +const localMarketplace = (path: string) => ({ + name: 'local-test', + source: { type: 'local' as const, location: path }, + path, +}); + describe('getMarketplaceVersion', () => { it('should return hash and date for a git repo', async () => { const dir = await mkdtemp(join(tmpdir(), 'mp-version-')); @@ -56,7 +62,7 @@ describe('getMarketplaceVersion', () => { execSync('git add .', { cwd: dir, env }); execSync('git commit -m "initial"', { cwd: dir, env }); - const result = (await getMarketplaceVersion(dir)) as { + const result = (await getMarketplaceVersion(localMarketplace(dir))) as { hash: string; date: Date; } | null; @@ -71,7 +77,7 @@ describe('getMarketplaceVersion', () => { it('should return null for a non-git directory', async () => { const dir = await mkdtemp(join(tmpdir(), 'mp-version-')); try { - const result = await getMarketplaceVersion(dir); + const result = await getMarketplaceVersion(localMarketplace(dir)); expect(result).toBeNull(); } finally { await rm(dir, { recursive: true }); @@ -79,9 +85,24 @@ describe('getMarketplaceVersion', () => { }); it('should return null for a non-existent path', async () => { - const result = await getMarketplaceVersion( - '/tmp/nonexistent-marketplace-path', - ); + const path = '/tmp/nonexistent-marketplace-path'; + const result = await getMarketplaceVersion(localMarketplace(path)); expect(result).toBeNull(); }); + + it('should not inspect an unmanaged remote registry path', async () => { + const dir = await mkdtemp(join(tmpdir(), 'mp-version-')); + const env = gitEnv(dir); + try { + execSync('git init', { cwd: dir, env }); + const result = await getMarketplaceVersion({ + name: 'unsafe', + source: { type: 'github', location: 'owner/unsafe' }, + path: dir, + }); + expect(result).toBeNull(); + } finally { + await rm(dir, { recursive: true }); + } + }); }); diff --git a/tests/unit/core/plugin.test.ts b/tests/unit/core/plugin.test.ts index cde3f10..3e16c3a 100644 --- a/tests/unit/core/plugin.test.ts +++ b/tests/unit/core/plugin.test.ts @@ -133,9 +133,12 @@ describe('updatePlugin', () => { return null; }); - const mockGetMarketplace = mock(async (name: string) => { + const mockGetMarketplaceRegistration = mock(async (name: string) => { if (name === 'test-marketplace') { - return { name: 'test-marketplace', path: '/mock/marketplace/path', source: { type: 'github' } }; + return { + key: 'test-marketplace', + entry: { name: 'test-marketplace', path: '/mock/marketplace/path', source: { type: 'github' as const, location: 'owner/test-marketplace' } }, + }; } return null; }); @@ -159,7 +162,8 @@ describe('updatePlugin', () => { const updateDeps: UpdatePluginDeps = { parsePluginSpec: mockParsePluginSpec as unknown as UpdatePluginDeps['parsePluginSpec'], - getMarketplace: mockGetMarketplace as unknown as UpdatePluginDeps['getMarketplace'], + getMarketplaceRegistration: mockGetMarketplaceRegistration as unknown as UpdatePluginDeps['getMarketplaceRegistration'], + validateMarketplaceAccess: () => undefined, parseMarketplaceManifest: mockParseManifest as unknown as UpdatePluginDeps['parseMarketplaceManifest'], updateMarketplace: mockUpdateMarketplace as unknown as UpdatePluginDeps['updateMarketplace'], fetchFn: mockFetchFn as unknown as UpdatePluginDeps['fetchFn'], @@ -167,7 +171,7 @@ describe('updatePlugin', () => { beforeEach(() => { mockParsePluginSpec.mockClear(); - mockGetMarketplace.mockClear(); + mockGetMarketplaceRegistration.mockClear(); mockParseManifest.mockClear(); mockUpdateMarketplace.mockClear(); mockFetchFn.mockClear(); @@ -197,4 +201,41 @@ describe('updatePlugin', () => { expect(result.success).toBe(true); expect(result.action).toBe('updated'); }); + + it('should update the exact registry key after source fallback lookup', async () => { + const deps: UpdatePluginDeps = { + ...updateDeps, + getMarketplaceRegistration: mock(async () => ({ + key: 'legacy-alias', + entry: { + name: 'canonical-name', + path: '/mock/marketplace/path', + source: { type: 'github' as const, location: 'owner/test-marketplace' }, + }, + })), + }; + + const result = await updatePlugin( + 'embedded-plugin@owner/test-marketplace', + deps, + ); + + expect(result.success).toBe(true); + expect(mockUpdateMarketplace).toHaveBeenCalledWith('legacy-alias'); + expect(mockUpdateMarketplace).not.toHaveBeenCalledWith('canonical-name'); + }); + + it('should reject unsafe marketplace access before parsing its manifest', async () => { + const deps: UpdatePluginDeps = { + ...updateDeps, + validateMarketplaceAccess: () => 'Refused unsafe marketplace path', + }; + + const result = await updatePlugin('embedded-plugin@test-marketplace', deps); + + expect(result.success).toBe(false); + expect(result.error).toBe('Refused unsafe marketplace path'); + expect(mockParseManifest).not.toHaveBeenCalled(); + expect(mockUpdateMarketplace).not.toHaveBeenCalled(); + }); }); diff --git a/tests/unit/core/sync-marketplace-cache-safety.test.ts b/tests/unit/core/sync-marketplace-cache-safety.test.ts new file mode 100644 index 0000000..cda9365 --- /dev/null +++ b/tests/unit/core/sync-marketplace-cache-safety.test.ts @@ -0,0 +1,57 @@ +import { afterEach, beforeEach, describe, expect, it } from 'bun:test'; +import { mkdirSync, rmSync, writeFileSync } from 'node:fs'; +import { join } from 'node:path'; +import { tmpdir } from 'node:os'; +import { fetchPlugin, resetFetchCache } from '../../../src/core/plugin.js'; +import { seedFetchCacheFromMarketplaces } from '../../../src/core/sync.js'; +import { stubHomeDir } from '../../helpers/env.js'; + +describe('seedFetchCacheFromMarketplaces ownership', () => { + let testHome: string; + let restoreHomeDir: () => void; + + beforeEach(() => { + testHome = join(tmpdir(), `marketplace-seed-safety-${Date.now()}`); + restoreHomeDir = stubHomeDir(testHome); + mkdirSync(join(testHome, '.allagents'), { recursive: true }); + resetFetchCache(); + }); + + afterEach(() => { + resetFetchCache(); + restoreHomeDir(); + rmSync(testHome, { recursive: true, force: true }); + }); + + it('should not seed an unsafe marketplace registry path', async () => { + writeFileSync( + join(testHome, '.allagents', 'marketplaces.json'), + JSON.stringify({ + version: 1, + marketplaces: { + unsafe: { + name: 'unsafe', + source: { type: 'github', location: 'owner/repo' }, + path: testHome, + }, + }, + }), + ); + await seedFetchCacheFromMarketplaces([ + { source: 'owner/repo', success: true, name: 'unsafe' }, + ]); + + let cloneCalled = false; + const result = await fetchPlugin('owner/repo', {}, { + existsSync: () => false, + mkdir: async () => undefined, + cloneTo: async () => { + cloneCalled = true; + }, + pull: async () => undefined, + }); + + expect(cloneCalled).toBe(true); + expect(result.cachePath).not.toBe(testHome); + }); +});