From 7d5e604e53b47811557baf4b1a6c4a11ed70af9a Mon Sep 17 00:00:00 2001 From: Tam Nhu Tran Date: Wed, 25 Feb 2026 00:24:11 +0700 Subject: [PATCH 1/3] fix(auth): harden shared-context isolation edge cases --- src/auth/account-context.ts | 14 ++- src/auth/commands/create-command.ts | 99 ++++++++++++++++++++- src/auth/commands/list-command.ts | 2 + src/auth/commands/types.ts | 24 +++++ src/config/migration-manager.ts | 9 ++ src/management/instance-manager.ts | 79 ++++++++++++++-- src/web-server/routes/account-routes.ts | 23 +++-- tests/unit/account-context.test.ts | 31 +++++++ tests/unit/auth-command-args.test.ts | 14 +++ tests/unit/auth-list-context.test.ts | 88 ++++++++++++++++++ tests/unit/config/migration-manager.test.ts | 39 +++++++- tests/unit/shared-context-policy.test.ts | 16 ++++ 12 files changed, 417 insertions(+), 21 deletions(-) create mode 100644 tests/unit/account-context.test.ts create mode 100644 tests/unit/auth-list-context.test.ts diff --git a/src/auth/account-context.ts b/src/auth/account-context.ts index 93cb29be..f97a89b5 100644 --- a/src/auth/account-context.ts +++ b/src/auth/account-context.ts @@ -29,6 +29,8 @@ export interface ResolvedCreateAccountContext { export const DEFAULT_ACCOUNT_CONTEXT_MODE: AccountContextMode = 'isolated'; export const DEFAULT_ACCOUNT_CONTEXT_GROUP = 'default'; +export const MAX_CONTEXT_GROUP_LENGTH = 64; +export const ACCOUNT_PROFILE_NAME_PATTERN = /^[a-zA-Z][a-zA-Z0-9_-]*$/; const CONTEXT_GROUP_PATTERN = /^[a-zA-Z][a-zA-Z0-9_-]*$/; @@ -43,7 +45,14 @@ export function normalizeContextGroupName(value: string): string { * Validate context group naming constraints. */ export function isValidContextGroupName(value: string): boolean { - return CONTEXT_GROUP_PATTERN.test(value); + return value.length <= MAX_CONTEXT_GROUP_LENGTH && CONTEXT_GROUP_PATTERN.test(value); +} + +/** + * Validate account profile naming constraints. + */ +export function isValidAccountProfileName(value: string): boolean { + return ACCOUNT_PROFILE_NAME_PATTERN.test(value); } /** @@ -84,8 +93,7 @@ export function resolveCreateAccountContext( if (!isValidContextGroupName(normalizedGroup)) { return { policy: { mode: 'isolated' }, - error: - 'Invalid context group. Use letters/numbers/dash/underscore and start with a letter.', + error: `Invalid context group. Use letters/numbers/dash/underscore, start with a letter, max ${MAX_CONTEXT_GROUP_LENGTH} chars.`, }; } diff --git a/src/auth/commands/create-command.ts b/src/auth/commands/create-command.ts index 84c04518..bc40fea4 100644 --- a/src/auth/commands/create-command.ts +++ b/src/auth/commands/create-command.ts @@ -9,21 +9,27 @@ import { initUI, header, color, fail, warn, info, infoBox, warnBox } from '../.. import { getClaudeCliInfo } from '../../utils/claude-detector'; import { escapeShellArg, stripClaudeCodeEnv } from '../../utils/shell-executor'; import { isUnifiedMode } from '../../config/unified-config-loader'; +import { ProfileMetadata } from '../../types'; import { resolveCreateAccountContext, policyToAccountContextMetadata, formatAccountContextPolicy, + isValidAccountProfileName, } from '../account-context'; import { exitWithError } from '../../errors'; import { ExitCode } from '../../errors/exit-codes'; import { CommandContext, parseArgs } from './types'; +function sanitizeProfileNameForInstance(name: string): string { + return name.replace(/[^a-zA-Z0-9_-]/g, '-').toLowerCase(); +} + /** * Handle the create command */ export async function handleCreate(ctx: CommandContext, args: string[]): Promise { await initUI(); - const { profileName, force, shareContext, contextGroup } = parseArgs(args); + const { profileName, force, shareContext, contextGroup, unknownFlags } = parseArgs(args); if (!profileName) { console.log(fail('Profile name is required')); @@ -37,6 +43,21 @@ export async function handleCreate(ctx: CommandContext, args: string[]): Promise exitWithError('Profile name is required', ExitCode.PROFILE_ERROR); } + if (unknownFlags && unknownFlags.length > 0) { + const unknownList = unknownFlags.join(', '); + console.log(fail(`Unknown option(s): ${unknownList}`)); + console.log(''); + exitWithError(`Unknown option(s): ${unknownList}`, ExitCode.PROFILE_ERROR); + } + + if (!isValidAccountProfileName(profileName)) { + const error = + 'Invalid profile name. Use letters/numbers/dash/underscore and start with a letter.'; + console.log(fail(error)); + console.log(''); + exitWithError(error, ExitCode.PROFILE_ERROR); + } + // Check if profile already exists (check both legacy and unified) const existsLegacy = ctx.registry.hasProfile(profileName); const existsUnified = ctx.registry.hasAccountUnified(profileName); @@ -46,6 +67,18 @@ export async function handleCreate(ctx: CommandContext, args: string[]): Promise exitWithError(`Profile already exists: ${profileName}`, ExitCode.PROFILE_ERROR); } + const normalizedName = sanitizeProfileNameForInstance(profileName); + const collidingName = Object.keys(ctx.registry.getAllProfilesMerged()).find( + (name) => name !== profileName && sanitizeProfileNameForInstance(name) === normalizedName + ); + + if (collidingName) { + const error = `Profile "${profileName}" conflicts with existing profile "${collidingName}" on filesystem.`; + console.log(fail(error)); + console.log(''); + exitWithError(error, ExitCode.PROFILE_ERROR); + } + const resolvedContext = resolveCreateAccountContext({ shareContext: !!shareContext, contextGroup, @@ -59,14 +92,47 @@ export async function handleCreate(ctx: CommandContext, args: string[]): Promise const contextPolicy = resolvedContext.policy; const contextMetadata = policyToAccountContextMetadata(contextPolicy); + const useUnifiedConfig = isUnifiedMode(); + const createdProfile = useUnifiedConfig ? !existsUnified : !existsLegacy; + const previousLegacyProfile: ProfileMetadata | undefined = + !useUnifiedConfig && existsLegacy ? ctx.registry.getProfile(profileName) : undefined; + const previousUnifiedProfile = + useUnifiedConfig && existsUnified + ? ctx.registry.getAllAccountsUnified()[profileName] + : undefined; try { + const rollbackMetadata = (): void => { + try { + if (useUnifiedConfig) { + if (createdProfile) { + if (ctx.registry.hasAccountUnified(profileName)) { + ctx.registry.removeAccountUnified(profileName); + } + } else if (previousUnifiedProfile) { + ctx.registry.updateAccountUnified(profileName, previousUnifiedProfile); + } + return; + } + + if (createdProfile) { + if (ctx.registry.hasProfile(profileName)) { + ctx.registry.deleteProfile(profileName); + } + } else if (previousLegacyProfile) { + ctx.registry.updateProfile(profileName, previousLegacyProfile); + } + } catch { + // Best-effort rollback to avoid leaving stale accounts after failed login. + } + }; + // Create instance directory console.log(info(`Creating profile: ${profileName}`)); const instancePath = await ctx.instanceMgr.ensureInstance(profileName, contextPolicy); // Create/update profile entry based on config mode - if (isUnifiedMode()) { + if (useUnifiedConfig) { // Use unified config (config.yaml) if (existsUnified) { ctx.registry.updateAccountUnified(profileName, { @@ -96,7 +162,11 @@ export async function handleCreate(ctx: CommandContext, args: string[]): Promise console.log(info(`Instance directory: ${instancePath}`)); console.log(''); - console.log(warn('Starting Claude in isolated instance...')); + const launchDescription = + contextPolicy.mode === 'shared' + ? `Starting Claude with shared context group "${contextPolicy.group || 'default'}"...` + : 'Starting Claude in isolated instance...'; + console.log(warn(launchDescription)); console.log(warn('You will be prompted to login with your account.')); console.log(''); @@ -112,6 +182,20 @@ export async function handleCreate(ctx: CommandContext, args: string[]): Promise const { path: claudeCli, needsShell } = claudeInfo; const childEnv = stripClaudeCodeEnv({ ...process.env, CLAUDE_CONFIG_DIR: instancePath }); + // Avoid ambient provider credentials influencing account-login bootstrap behavior. + const ambientProviderPrefixes = ['ANTHROPIC_', 'OPENAI_', 'GOOGLE_', 'GEMINI_', 'MINIMAX_']; + for (const envKey of Object.keys(childEnv)) { + if (envKey === 'CLAUDE_CONFIG_DIR') { + continue; + } + + if ( + ambientProviderPrefixes.some((prefix) => envKey.startsWith(prefix)) || + envKey === 'OPENROUTER_API_KEY' + ) { + delete childEnv[envKey]; + } + } // Execute Claude in isolated instance (will auto-prompt for login if no credentials) // On Windows, .cmd/.bat/.ps1 files need shell: true to execute properly @@ -162,6 +246,11 @@ export async function handleCreate(ctx: CommandContext, args: string[]): Promise console.log(''); process.exit(0); } else { + rollbackMetadata(); + if (createdProfile) { + ctx.instanceMgr.deleteInstance(profileName); + } + console.log(''); console.log(fail('Login failed or cancelled')); console.log(''); @@ -173,6 +262,10 @@ export async function handleCreate(ctx: CommandContext, args: string[]): Promise }); child.on('error', (err: Error) => { + rollbackMetadata(); + if (createdProfile) { + ctx.instanceMgr.deleteInstance(profileName); + } exitWithError(`Failed to execute Claude CLI: ${err.message}`, ExitCode.BINARY_ERROR); }); } catch (error) { diff --git a/src/auth/commands/list-command.ts b/src/auth/commands/list-command.ts index a60dbff3..a248c2ee 100644 --- a/src/auth/commands/list-command.ts +++ b/src/auth/commands/list-command.ts @@ -30,6 +30,8 @@ export async function handleList(ctx: CommandContext, args: string[]): Promise(); + const knownBooleanFlags = new Set([ + '--force', + '--verbose', + '--json', + '--yes', + '-y', + '--share-context', + ]); + const knownValueFlags = new Set(['--context-group']); for (let i = 0; i < args.length; i++) { const arg = args[i]; @@ -83,6 +94,18 @@ export function parseArgs(args: string[]): AuthCommandArgs { } if (arg.startsWith('-')) { + const normalizedFlag = arg.includes('=') ? arg.slice(0, arg.indexOf('=')) : arg; + const isKnownFlag = + knownBooleanFlags.has(normalizedFlag) || knownValueFlags.has(normalizedFlag); + if (!isKnownFlag) { + unknownFlags.add(normalizedFlag); + // Best effort: unknown flags often take a value token. + // Skip one following non-flag token to avoid mis-parsing profile name. + const next = args[i + 1]; + if (!arg.includes('=') && next && !next.startsWith('-')) { + i++; + } + } continue; } @@ -99,5 +122,6 @@ export function parseArgs(args: string[]): AuthCommandArgs { yes: args.includes('--yes') || args.includes('-y'), shareContext: args.includes('--share-context'), contextGroup, + unknownFlags: [...unknownFlags], }; } diff --git a/src/config/migration-manager.ts b/src/config/migration-manager.ts index 3910a71a..2a94f78b 100644 --- a/src/config/migration-manager.ts +++ b/src/config/migration-manager.ts @@ -148,9 +148,18 @@ export async function migrate(dryRun = false): Promise { if (oldProfiles?.profiles) { for (const [name, meta] of Object.entries(oldProfiles.profiles)) { const metadata = meta as Record; + const rawContextMode = metadata.context_mode; + const rawContextGroup = metadata.context_group; + const contextMode = rawContextMode === 'shared' ? 'shared' : 'isolated'; + const contextGroup = + typeof rawContextGroup === 'string' && rawContextGroup.trim().length > 0 + ? rawContextGroup + : undefined; const account: AccountConfig = { created: (metadata.created as string) || new Date().toISOString(), last_used: (metadata.last_used as string) || null, + context_mode: contextMode, + context_group: contextMode === 'shared' ? contextGroup : undefined, }; unifiedConfig.accounts[name] = account; } diff --git a/src/management/instance-manager.ts b/src/management/instance-manager.ts index ab3889de..b51abae3 100644 --- a/src/management/instance-manager.ts +++ b/src/management/instance-manager.ts @@ -17,10 +17,12 @@ import { getCcsDir } from '../utils/config-manager'; */ class InstanceManager { private readonly instancesDir: string; + private readonly locksDir: string; private readonly sharedManager: SharedManager; constructor() { this.instancesDir = path.join(getCcsDir(), 'instances'); + this.locksDir = path.join(this.instancesDir, '.locks'); this.sharedManager = new SharedManager(); } @@ -33,16 +35,19 @@ class InstanceManager { ): Promise { const instancePath = this.getInstancePath(profileName); - // Lazy initialization - if (!fs.existsSync(instancePath)) { - this.initializeInstance(profileName, instancePath); - } + // Serialize context sync operations per profile across processes. + await this.withContextSyncLock(profileName, async () => { + // Lazy initialization + if (!fs.existsSync(instancePath)) { + this.initializeInstance(profileName, instancePath); + } - // Validate structure (auto-fix missing dirs) - this.validateInstance(instancePath); + // Validate structure (auto-fix missing dirs) + this.validateInstance(instancePath); - // Apply context policy (isolated by default, optional shared group). - await this.sharedManager.syncProjectContext(instancePath, contextPolicy); + // Apply context policy (isolated by default, optional shared group). + await this.sharedManager.syncProjectContext(instancePath, contextPolicy); + }); return instancePath; } @@ -195,6 +200,64 @@ class InstanceManager { // Replace unsafe characters with dash return name.replace(/[^a-zA-Z0-9_-]/g, '-').toLowerCase(); } + + private getContextSyncLockPath(profileName: string): string { + const safeName = this.sanitizeName(profileName); + return path.join(this.locksDir, `${safeName}.lock`); + } + + private async withContextSyncLock( + profileName: string, + callback: () => Promise + ): Promise { + const lockPath = this.getContextSyncLockPath(profileName); + const retryDelayMs = 50; + const timeoutMs = 5000; + const staleLockMs = 30000; + const start = Date.now(); + + fs.mkdirSync(this.locksDir, { recursive: true, mode: 0o700 }); + + while (true) { + try { + const fd = fs.openSync(lockPath, 'wx', 0o600); + fs.writeFileSync(fd, `${process.pid}`); + fs.closeSync(fd); + break; + } catch (error) { + const err = error as NodeJS.ErrnoException; + if (err.code !== 'EEXIST') { + throw error; + } + + try { + const lockStats = fs.statSync(lockPath); + if (Date.now() - lockStats.mtimeMs > staleLockMs) { + fs.unlinkSync(lockPath); + continue; + } + } catch { + // Best-effort stale lock cleanup. + } + + if (Date.now() - start > timeoutMs) { + throw new Error(`Timed out waiting for profile context lock: ${profileName}`); + } + + await new Promise((resolve) => setTimeout(resolve, retryDelayMs)); + } + } + + try { + return await callback(); + } finally { + try { + fs.unlinkSync(lockPath); + } catch { + // Best-effort cleanup. + } + } + } } export { InstanceManager }; diff --git a/src/web-server/routes/account-routes.ts b/src/web-server/routes/account-routes.ts index 8c35128a..a92422aa 100644 --- a/src/web-server/routes/account-routes.ts +++ b/src/web-server/routes/account-routes.ts @@ -7,6 +7,7 @@ import { Router, Request, Response } from 'express'; import ProfileRegistry from '../../auth/profile-registry'; +import InstanceManager from '../../management/instance-manager'; import { isUnifiedMode } from '../../config/unified-config-loader'; import { getAllAccountsSummary, @@ -21,19 +22,25 @@ import { isCLIProxyProvider } from '../../cliproxy/provider-capabilities'; const router = Router(); const registry = new ProfileRegistry(); +const instanceMgr = new InstanceManager(); /** Parse CLIProxy account key format: "provider:accountId" */ function parseCliproxyKey(key: string): { provider: CLIProxyProvider; accountId: string } | null { - const colonIndex = key.indexOf(':'); + const normalizedKey = key.startsWith('cliproxy:') ? key.slice('cliproxy:'.length) : key; + const colonIndex = normalizedKey.indexOf(':'); if (colonIndex === -1) return null; - const provider = key.slice(0, colonIndex); - const accountId = key.slice(colonIndex + 1); + const provider = normalizedKey.slice(0, colonIndex); + const accountId = normalizedKey.slice(colonIndex + 1); if (!isCLIProxyProvider(provider) || !accountId) return null; return { provider, accountId }; } +function hasAuthAccount(name: string): boolean { + return registry.hasAccountUnified(name) || registry.hasProfile(name); +} + /** * GET /api/accounts - List accounts from both profiles.json and config.yaml */ @@ -91,7 +98,8 @@ router.get('/', (_req: Request, res: Response): void => { } // Use unique ID for key to prevent collisions between accounts with same nickname/email const displayName = acct.nickname || acct.email || acct.id; - const key = `${provider}:${acct.id}`; + const rawKey = `${provider}:${acct.id}`; + const key = merged[rawKey] ? `cliproxy:${rawKey}` : rawKey; merged[key] = { type: 'cliproxy', provider, @@ -130,7 +138,7 @@ router.post('/default', (req: Request, res: Response): void => { } // Check if this is a CLIProxy account (format: "provider:accountId") - const cliproxyKey = parseCliproxyKey(name); + const cliproxyKey = !hasAuthAccount(name) ? parseCliproxyKey(name) : null; if (cliproxyKey) { const success = setCliproxyDefault(cliproxyKey.provider, cliproxyKey.accountId); if (!success) { @@ -192,7 +200,7 @@ router.delete('/:name', (req: Request, res: Response): void => { } // Check if this is a CLIProxy account (format: "provider:accountId") - const cliproxyKey = parseCliproxyKey(name); + const cliproxyKey = !hasAuthAccount(name) ? parseCliproxyKey(name) : null; if (cliproxyKey) { const success = removeCliproxyAccount(cliproxyKey.provider, cliproxyKey.accountId); if (!success) { @@ -219,6 +227,9 @@ router.delete('/:name', (req: Request, res: Response): void => { return; } + // Keep API delete behavior aligned with CLI remove command. + instanceMgr.deleteInstance(name); + res.json({ success: true, deleted: name }); } catch (error) { res.status(500).json({ error: (error as Error).message }); diff --git a/tests/unit/account-context.test.ts b/tests/unit/account-context.test.ts new file mode 100644 index 00000000..8b18b55d --- /dev/null +++ b/tests/unit/account-context.test.ts @@ -0,0 +1,31 @@ +import { describe, expect, it } from 'bun:test'; +import { + MAX_CONTEXT_GROUP_LENGTH, + isValidAccountProfileName, + resolveAccountContextPolicy, + resolveCreateAccountContext, +} from '../../src/auth/account-context'; + +describe('account context helpers', () => { + it('rejects context groups that exceed the max length', () => { + const group = `a${'x'.repeat(MAX_CONTEXT_GROUP_LENGTH)}`; + const result = resolveCreateAccountContext({ shareContext: false, contextGroup: group }); + + expect(result.error).toContain('Invalid context group'); + }); + + it('rejects profile names with unsupported characters', () => { + expect(isValidAccountProfileName('work')).toBe(true); + expect(isValidAccountProfileName('gemini:default')).toBe(false); + }); + + it('falls back to default shared group for invalid persisted metadata', () => { + const resolved = resolveAccountContextPolicy({ + context_mode: 'shared', + context_group: '###', + }); + + expect(resolved.mode).toBe('shared'); + expect(resolved.group).toBe('default'); + }); +}); diff --git a/tests/unit/auth-command-args.test.ts b/tests/unit/auth-command-args.test.ts index e91afc9c..e6ba81e7 100644 --- a/tests/unit/auth-command-args.test.ts +++ b/tests/unit/auth-command-args.test.ts @@ -31,4 +31,18 @@ describe('auth command args parsing', () => { expect(parsed.profileName).toBe('work'); expect(parsed.contextGroup).toBe(''); }); + + it('flags empty inline context group as empty string', () => { + const parsed = parseArgs(['work', '--context-group=']); + + expect(parsed.profileName).toBe('work'); + expect(parsed.contextGroup).toBe(''); + }); + + it('tracks unknown flags and keeps positional profile intact', () => { + const parsed = parseArgs(['--foo', 'bar', 'work']); + + expect(parsed.profileName).toBe('work'); + expect(parsed.unknownFlags).toEqual(['--foo']); + }); }); diff --git a/tests/unit/auth-list-context.test.ts b/tests/unit/auth-list-context.test.ts new file mode 100644 index 00000000..7f24b531 --- /dev/null +++ b/tests/unit/auth-list-context.test.ts @@ -0,0 +1,88 @@ +import { afterEach, beforeEach, describe, expect, it } from 'bun:test'; +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; +import ProfileRegistry from '../../src/auth/profile-registry'; +import InstanceManager from '../../src/management/instance-manager'; +import { handleList } from '../../src/auth/commands/list-command'; + +describe('auth list context metadata', () => { + let tempRoot = ''; + let originalCcsHome: string | undefined; + let originalCcsUnified: string | undefined; + + beforeEach(() => { + tempRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'ccs-auth-list-context-')); + originalCcsHome = process.env.CCS_HOME; + originalCcsUnified = process.env.CCS_UNIFIED_CONFIG; + + process.env.CCS_HOME = tempRoot; + process.env.CCS_UNIFIED_CONFIG = '1'; + }); + + afterEach(() => { + if (originalCcsHome !== undefined) process.env.CCS_HOME = originalCcsHome; + else delete process.env.CCS_HOME; + + if (originalCcsUnified !== undefined) process.env.CCS_UNIFIED_CONFIG = originalCcsUnified; + else delete process.env.CCS_UNIFIED_CONFIG; + + if (tempRoot && fs.existsSync(tempRoot)) { + fs.rmSync(tempRoot, { recursive: true, force: true }); + } + }); + + it('keeps unified account context metadata in JSON list output', async () => { + const ccsDir = path.join(tempRoot, '.ccs'); + fs.mkdirSync(ccsDir, { recursive: true }); + fs.writeFileSync( + path.join(ccsDir, 'config.yaml'), + [ + 'version: 8', + 'accounts:', + ' work:', + ' created: "2026-02-01T00:00:00.000Z"', + ' last_used: null', + ' context_mode: shared', + ' context_group: sprint-a', + 'profiles: {}', + 'cliproxy:', + ' oauth_accounts: {}', + ' providers: {}', + ' variants: {}', + ].join('\n'), + 'utf8' + ); + + const registry = new ProfileRegistry(); + + const instanceMgr = new InstanceManager(); + const lines: string[] = []; + const originalLog = console.log; + console.log = (...args: unknown[]) => { + lines.push(args.map(String).join(' ')); + }; + + try { + await handleList( + { + registry, + instanceMgr, + version: 'test', + }, + ['--json'] + ); + } finally { + console.log = originalLog; + } + + const payload = JSON.parse(lines.join('\n')) as { + profiles: Array<{ name: string; context_mode?: string; context_group?: string | null }>; + }; + const work = payload.profiles.find((profile) => profile.name === 'work'); + + expect(work).toBeTruthy(); + expect(work?.context_mode).toBe('shared'); + expect(work?.context_group).toBe('sprint-a'); + }); +}); diff --git a/tests/unit/config/migration-manager.test.ts b/tests/unit/config/migration-manager.test.ts index 450640d9..ec446d35 100644 --- a/tests/unit/config/migration-manager.test.ts +++ b/tests/unit/config/migration-manager.test.ts @@ -3,7 +3,7 @@ import * as fs from 'fs'; import * as os from 'os'; import * as path from 'path'; import { loadMigrationCheckData, migrate } from '../../../src/config/migration-manager'; -import { saveUnifiedConfig } from '../../../src/config/unified-config-loader'; +import { loadUnifiedConfig, saveUnifiedConfig } from '../../../src/config/unified-config-loader'; import { createEmptyUnifiedConfig } from '../../../src/config/unified-config-types'; describe('migration-manager legacy kimi compatibility', () => { @@ -132,4 +132,41 @@ describe('migration-manager legacy kimi compatibility', () => { const checkData = loadMigrationCheckData(); expect(checkData.needsMigration).toBe(false); }); + + it('migrates account context metadata from profiles.json', async () => { + fs.writeFileSync( + path.join(ccsDir, 'profiles.json'), + JSON.stringify( + { + default: 'work', + profiles: { + work: { + type: 'account', + created: '2026-02-01T00:00:00.000Z', + last_used: null, + context_mode: 'shared', + context_group: 'sprint-a', + }, + personal: { + type: 'account', + created: '2026-02-02T00:00:00.000Z', + last_used: null, + }, + }, + }, + null, + 2 + ) + ); + + const result = await migrate(false); + expect(result.success).toBe(true); + + const unified = loadUnifiedConfig(); + expect(unified).toBeTruthy(); + expect(unified?.accounts.work.context_mode).toBe('shared'); + expect(unified?.accounts.work.context_group).toBe('sprint-a'); + expect(unified?.accounts.personal.context_mode).toBe('isolated'); + expect(unified?.accounts.personal.context_group).toBeUndefined(); + }); }); diff --git a/tests/unit/shared-context-policy.test.ts b/tests/unit/shared-context-policy.test.ts index 5cf76b09..dffbd37f 100644 --- a/tests/unit/shared-context-policy.test.ts +++ b/tests/unit/shared-context-policy.test.ts @@ -3,6 +3,7 @@ import * as fs from 'fs'; import * as os from 'os'; import * as path from 'path'; import SharedManager from '../../src/management/shared-manager'; +import InstanceManager from '../../src/management/instance-manager'; import type { AccountContextPolicy } from '../../src/auth/account-context'; function getTestCcsDir(): string { @@ -118,4 +119,19 @@ describe('SharedManager context policy', () => { expect(fs.existsSync(projectFile)).toBe(true); expect(fs.readFileSync(projectFile, 'utf8')).toBe('shared history'); }); + + it('serializes concurrent context sync for the same profile', async () => { + const instanceMgr = new InstanceManager(); + const jobs = Array.from({ length: 6 }, () => + instanceMgr.ensureInstance('work', { mode: 'shared', group: 'sprint-a' }) + ); + + await Promise.all(jobs); + + const ccsDir = getTestCcsDir(); + const projectsPath = path.join(ccsDir, 'instances', 'work', 'projects'); + const stats = fs.lstatSync(projectsPath); + + expect(stats.isDirectory() || stats.isSymbolicLink()).toBe(true); + }); }); From 0b070a3f343846cb8082fcd351f0bcb3fc2f109d Mon Sep 17 00:00:00 2001 From: Tam Nhu Tran Date: Wed, 25 Feb 2026 00:55:35 +0700 Subject: [PATCH 2/3] fix(auth): close shared-context isolation edge cases --- src/auth/commands/create-command.ts | 225 +++++++++++------- src/auth/profile-registry.ts | 14 +- src/config/migration-manager.ts | 16 +- src/management/instance-manager.ts | 41 +++- src/management/shared-manager.ts | 28 +++ .../routes/account-route-helpers.ts | 45 ++++ src/web-server/routes/account-routes.ts | 57 ++--- src/web-server/routes/config-routes.ts | 54 +++++ tests/unit/account-context.test.ts | 12 + tests/unit/auth-list-context.test.ts | 74 ++++++ tests/unit/config/migration-manager.test.ts | 42 ++++ .../web-server/account-routes-context.test.ts | 133 +++++++++++ .../config-routes-account-context.test.ts | 141 +++++++++++ .../account/create-auth-profile-dialog.tsx | 8 +- 14 files changed, 767 insertions(+), 123 deletions(-) create mode 100644 src/web-server/routes/account-route-helpers.ts create mode 100644 tests/unit/web-server/account-routes-context.test.ts create mode 100644 tests/unit/web-server/config-routes-account-context.test.ts diff --git a/src/auth/commands/create-command.ts b/src/auth/commands/create-command.ts index bc40fea4..614a0ec8 100644 --- a/src/auth/commands/create-command.ts +++ b/src/auth/commands/create-command.ts @@ -15,6 +15,7 @@ import { policyToAccountContextMetadata, formatAccountContextPolicy, isValidAccountProfileName, + resolveAccountContextPolicy, } from '../account-context'; import { exitWithError } from '../../errors'; import { ExitCode } from '../../errors/exit-codes'; @@ -24,6 +25,47 @@ function sanitizeProfileNameForInstance(name: string): string { return name.replace(/[^a-zA-Z0-9_-]/g, '-').toLowerCase(); } +const AMBIENT_PROVIDER_PREFIXES = [ + 'ANTHROPIC_', + 'OPENAI_', + 'GOOGLE_', + 'GEMINI_', + 'MINIMAX_', + 'QWEN_', + 'DEEPSEEK_', + 'KIMI_', + 'AZURE_', + 'OLLAMA_', +]; +const AMBIENT_PROVIDER_EXACT_KEYS = new Set([ + 'OPENROUTER_API_KEY', + 'OPENROUTER_KEY', + 'XAI_API_KEY', + 'MISTRAL_API_KEY', + 'COHERE_API_KEY', +]); +const AMBIENT_PROVIDER_SUFFIXES = ['_API_KEY', '_AUTH_TOKEN', '_ACCESS_TOKEN', '_SECRET_KEY']; + +function stripAmbientProviderCredentials(env: NodeJS.ProcessEnv): NodeJS.ProcessEnv { + const sanitized: NodeJS.ProcessEnv = { ...env }; + + for (const envKey of Object.keys(sanitized)) { + if (envKey === 'CLAUDE_CONFIG_DIR') { + continue; + } + + if ( + AMBIENT_PROVIDER_PREFIXES.some((prefix) => envKey.startsWith(prefix)) || + AMBIENT_PROVIDER_EXACT_KEYS.has(envKey) || + AMBIENT_PROVIDER_SUFFIXES.some((suffix) => envKey.endsWith(suffix)) + ) { + delete sanitized[envKey]; + } + } + + return sanitized; +} + /** * Handle the create command */ @@ -31,6 +73,13 @@ export async function handleCreate(ctx: CommandContext, args: string[]): Promise await initUI(); const { profileName, force, shareContext, contextGroup, unknownFlags } = parseArgs(args); + if (unknownFlags && unknownFlags.length > 0) { + const unknownList = unknownFlags.join(', '); + console.log(fail(`Unknown option(s): ${unknownList}`)); + console.log(''); + exitWithError(`Unknown option(s): ${unknownList}`, ExitCode.PROFILE_ERROR); + } + if (!profileName) { console.log(fail('Profile name is required')); console.log(''); @@ -43,13 +92,6 @@ export async function handleCreate(ctx: CommandContext, args: string[]): Promise exitWithError('Profile name is required', ExitCode.PROFILE_ERROR); } - if (unknownFlags && unknownFlags.length > 0) { - const unknownList = unknownFlags.join(', '); - console.log(fail(`Unknown option(s): ${unknownList}`)); - console.log(''); - exitWithError(`Unknown option(s): ${unknownList}`, ExitCode.PROFILE_ERROR); - } - if (!isValidAccountProfileName(profileName)) { const error = 'Invalid profile name. Use letters/numbers/dash/underscore and start with a letter.'; @@ -100,33 +142,73 @@ export async function handleCreate(ctx: CommandContext, args: string[]): Promise useUnifiedConfig && existsUnified ? ctx.registry.getAllAccountsUnified()[profileName] : undefined; + const previousContextPolicy = + !createdProfile && (previousUnifiedProfile || previousLegacyProfile) + ? resolveAccountContextPolicy(previousUnifiedProfile || previousLegacyProfile) + : undefined; + + const claudeInfo = getClaudeCliInfo(); + if (!claudeInfo) { + console.log(fail('Claude CLI not found')); + console.log(''); + console.log('Please install Claude CLI first:'); + console.log(` ${color('https://claude.ai/download', 'path')}`); + exitWithError('Claude CLI not found', ExitCode.BINARY_ERROR); + } + + let rollbackCompleted = false; + const rollbackMetadata = (): void => { + try { + if (useUnifiedConfig) { + if (createdProfile) { + if (ctx.registry.hasAccountUnified(profileName)) { + ctx.registry.removeAccountUnified(profileName); + } + } else if (previousUnifiedProfile) { + ctx.registry.updateAccountUnified(profileName, previousUnifiedProfile); + } + return; + } + + if (createdProfile) { + if (ctx.registry.hasProfile(profileName)) { + ctx.registry.deleteProfile(profileName); + } + } else if (previousLegacyProfile) { + ctx.registry.updateProfile(profileName, previousLegacyProfile); + } + } catch { + // Best-effort rollback to avoid leaving stale accounts after failed login. + } + }; + + const rollbackFailedCreate = async (): Promise => { + if (rollbackCompleted) { + return; + } + rollbackCompleted = true; + + rollbackMetadata(); + + if (createdProfile) { + try { + ctx.instanceMgr.deleteInstance(profileName); + } catch { + // Best-effort cleanup. + } + return; + } + + if (previousContextPolicy) { + try { + await ctx.instanceMgr.ensureInstance(profileName, previousContextPolicy); + } catch { + // Best-effort rollback for context mode/group. + } + } + }; try { - const rollbackMetadata = (): void => { - try { - if (useUnifiedConfig) { - if (createdProfile) { - if (ctx.registry.hasAccountUnified(profileName)) { - ctx.registry.removeAccountUnified(profileName); - } - } else if (previousUnifiedProfile) { - ctx.registry.updateAccountUnified(profileName, previousUnifiedProfile); - } - return; - } - - if (createdProfile) { - if (ctx.registry.hasProfile(profileName)) { - ctx.registry.deleteProfile(profileName); - } - } else if (previousLegacyProfile) { - ctx.registry.updateProfile(profileName, previousLegacyProfile); - } - } catch { - // Best-effort rollback to avoid leaving stale accounts after failed login. - } - }; - // Create instance directory console.log(info(`Creating profile: ${profileName}`)); const instancePath = await ctx.instanceMgr.ensureInstance(profileName, contextPolicy); @@ -170,53 +252,39 @@ export async function handleCreate(ctx: CommandContext, args: string[]): Promise console.log(warn('You will be prompted to login with your account.')); console.log(''); - // Detect Claude CLI - const claudeInfo = getClaudeCliInfo(); - if (!claudeInfo) { - console.log(fail('Claude CLI not found')); - console.log(''); - console.log('Please install Claude CLI first:'); - console.log(` ${color('https://claude.ai/download', 'path')}`); - exitWithError('Claude CLI not found', ExitCode.BINARY_ERROR); - } - const { path: claudeCli, needsShell } = claudeInfo; - const childEnv = stripClaudeCodeEnv({ ...process.env, CLAUDE_CONFIG_DIR: instancePath }); - // Avoid ambient provider credentials influencing account-login bootstrap behavior. - const ambientProviderPrefixes = ['ANTHROPIC_', 'OPENAI_', 'GOOGLE_', 'GEMINI_', 'MINIMAX_']; - for (const envKey of Object.keys(childEnv)) { - if (envKey === 'CLAUDE_CONFIG_DIR') { - continue; - } - - if ( - ambientProviderPrefixes.some((prefix) => envKey.startsWith(prefix)) || - envKey === 'OPENROUTER_API_KEY' - ) { - delete childEnv[envKey]; - } - } + const childEnv = stripAmbientProviderCredentials( + stripClaudeCodeEnv({ ...process.env, CLAUDE_CONFIG_DIR: instancePath }) + ); // Execute Claude in isolated instance (will auto-prompt for login if no credentials) // On Windows, .cmd/.bat/.ps1 files need shell: true to execute properly let child: ChildProcess; - if (needsShell) { - const cmdString = escapeShellArg(claudeCli); - child = spawn(cmdString, { - stdio: 'inherit', - windowsHide: true, - shell: true, - env: childEnv, - }); - } else { - child = spawn(claudeCli, [], { - stdio: 'inherit', - windowsHide: true, - env: childEnv, - }); + try { + if (needsShell) { + const cmdString = escapeShellArg(claudeCli); + child = spawn(cmdString, { + stdio: 'inherit', + windowsHide: true, + shell: true, + env: childEnv, + }); + } else { + child = spawn(claudeCli, [], { + stdio: 'inherit', + windowsHide: true, + env: childEnv, + }); + } + } catch (error) { + await rollbackFailedCreate(); + exitWithError( + `Failed to execute Claude CLI: ${(error as Error).message}`, + ExitCode.BINARY_ERROR + ); } - child.on('exit', (code: number | null) => { + child.on('exit', async (code: number | null) => { if (code === 0) { console.log(''); console.log( @@ -246,10 +314,7 @@ export async function handleCreate(ctx: CommandContext, args: string[]): Promise console.log(''); process.exit(0); } else { - rollbackMetadata(); - if (createdProfile) { - ctx.instanceMgr.deleteInstance(profileName); - } + await rollbackFailedCreate(); console.log(''); console.log(fail('Login failed or cancelled')); @@ -261,14 +326,12 @@ export async function handleCreate(ctx: CommandContext, args: string[]): Promise } }); - child.on('error', (err: Error) => { - rollbackMetadata(); - if (createdProfile) { - ctx.instanceMgr.deleteInstance(profileName); - } + child.on('error', async (err: Error) => { + await rollbackFailedCreate(); exitWithError(`Failed to execute Claude CLI: ${err.message}`, ExitCode.BINARY_ERROR); }); } catch (error) { + await rollbackFailedCreate(); exitWithError(`Failed to create profile: ${(error as Error).message}`, ExitCode.GENERAL_ERROR); } } diff --git a/src/auth/profile-registry.ts b/src/auth/profile-registry.ts index e6d7beda..5870db61 100644 --- a/src/auth/profile-registry.ts +++ b/src/auth/profile-registry.ts @@ -159,7 +159,7 @@ export class ProfileRegistry { throw new Error(`Profile not found: ${name}`); } - return data.profiles[name]; + return this.normalizeLegacyProfileMetadata(data.profiles[name]); } /** @@ -215,7 +215,11 @@ export class ProfileRegistry { */ getAllProfiles(): Record { const data = this._read(); - return data.profiles; + const normalized: Record = {}; + for (const [name, profile] of Object.entries(data.profiles)) { + normalized[name] = this.normalizeLegacyProfileMetadata(profile); + } + return normalized; } /** @@ -357,7 +361,11 @@ export class ProfileRegistry { getAllAccountsUnified(): Record { if (!isUnifiedMode()) return {}; const config = loadOrCreateUnifiedConfig(); - return config.accounts; + const normalized: Record = {}; + for (const [name, account] of Object.entries(config.accounts)) { + normalized[name] = this.normalizeUnifiedAccountConfig(account); + } + return normalized; } /** diff --git a/src/config/migration-manager.ts b/src/config/migration-manager.ts index 2a94f78b..cf9628d2 100644 --- a/src/config/migration-manager.ts +++ b/src/config/migration-manager.ts @@ -21,6 +21,7 @@ import type { ProfileConfig, AccountConfig, CLIProxyVariantConfig } from './unif import { createEmptyUnifiedConfig } from './unified-config-types'; import { CLIPROXY_PROVIDER_IDS } from '../cliproxy/provider-capabilities'; import { saveUnifiedConfig, hasUnifiedConfig, loadUnifiedConfig } from './unified-config-loader'; +import { isValidContextGroupName, normalizeContextGroupName } from '../auth/account-context'; import { infoBox, warn } from '../utils/ui'; const BACKUP_DIR_PREFIX = 'backup-v1-'; @@ -151,10 +152,17 @@ export async function migrate(dryRun = false): Promise { const rawContextMode = metadata.context_mode; const rawContextGroup = metadata.context_group; const contextMode = rawContextMode === 'shared' ? 'shared' : 'isolated'; - const contextGroup = - typeof rawContextGroup === 'string' && rawContextGroup.trim().length > 0 - ? rawContextGroup - : undefined; + let contextGroup: string | undefined; + if (typeof rawContextGroup === 'string' && rawContextGroup.trim().length > 0) { + const normalizedGroup = normalizeContextGroupName(rawContextGroup); + if (isValidContextGroupName(normalizedGroup)) { + contextGroup = normalizedGroup; + } else { + warnings.push( + `Skipped invalid context group for account "${name}": "${rawContextGroup}" (fallback to default shared group)` + ); + } + } const account: AccountConfig = { created: (metadata.created as string) || new Date().toISOString(), last_used: (metadata.last_used as string) || null, diff --git a/src/management/instance-manager.ts b/src/management/instance-manager.ts index b51abae3..e39acbb1 100644 --- a/src/management/instance-manager.ts +++ b/src/management/instance-manager.ts @@ -8,6 +8,7 @@ import * as fs from 'fs'; import * as path from 'path'; +import { createHash } from 'crypto'; import SharedManager from './shared-manager'; import { AccountContextPolicy, DEFAULT_ACCOUNT_CONTEXT_MODE } from '../auth/account-context'; import { getCcsDir } from '../utils/config-manager'; @@ -203,7 +204,39 @@ class InstanceManager { private getContextSyncLockPath(profileName: string): string { const safeName = this.sanitizeName(profileName); - return path.join(this.locksDir, `${safeName}.lock`); + // Keep lock filenames deterministic while preventing normalized-name collisions. + const profileHash = createHash('sha1').update(profileName).digest('hex').slice(0, 8); + return path.join(this.locksDir, `${safeName}-${profileHash}.lock`); + } + + private isProcessAlive(pid: number): boolean { + if (!Number.isInteger(pid) || pid <= 0) { + return false; + } + + try { + process.kill(pid, 0); + return true; + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'EPERM') { + return true; + } + return false; + } + } + + private tryRemoveDeadOwnerLock(lockPath: string): boolean { + try { + const lockContent = fs.readFileSync(lockPath, 'utf8').trim(); + const lockPid = Number.parseInt(lockContent, 10); + if (!this.isProcessAlive(lockPid)) { + fs.unlinkSync(lockPath); + return true; + } + } catch { + // Best-effort stale lock cleanup. + } + return false; } private async withContextSyncLock( @@ -212,8 +245,8 @@ class InstanceManager { ): Promise { const lockPath = this.getContextSyncLockPath(profileName); const retryDelayMs = 50; - const timeoutMs = 5000; const staleLockMs = 30000; + const timeoutMs = staleLockMs + 5000; const start = Date.now(); fs.mkdirSync(this.locksDir, { recursive: true, mode: 0o700 }); @@ -236,6 +269,10 @@ class InstanceManager { fs.unlinkSync(lockPath); continue; } + + if (this.tryRemoveDeadOwnerLock(lockPath)) { + continue; + } } catch { // Best-effort stale lock cleanup. } diff --git a/src/management/shared-manager.ts b/src/management/shared-manager.ts index b48aeb54..e2ae5ad8 100644 --- a/src/management/shared-manager.ts +++ b/src/management/shared-manager.ts @@ -240,6 +240,7 @@ class SharedManager { if ( currentTarget && path.resolve(currentTarget) !== path.resolve(sharedProjectsPath) && + this.isSafeProjectsMergeSource(currentTarget, instanceName) && (await this.pathExists(currentTarget)) ) { await this.mergeDirectoryWithConflictCopies( @@ -247,6 +248,10 @@ class SharedManager { sharedProjectsPath, instanceName ); + } else if (currentTarget && !this.isSafeProjectsMergeSource(currentTarget, instanceName)) { + console.log( + warn(`Skipping unsafe project merge source outside CCS roots: ${currentTarget}`) + ); } await fs.promises.unlink(projectsPath); @@ -286,9 +291,14 @@ class SharedManager { if ( currentTarget && path.resolve(currentTarget) !== path.resolve(projectsPath) && + this.isSafeProjectsMergeSource(currentTarget, instanceName) && (await this.pathExists(currentTarget)) ) { await this.mergeDirectoryWithConflictCopies(currentTarget, projectsPath, instanceName); + } else if (currentTarget && !this.isSafeProjectsMergeSource(currentTarget, instanceName)) { + console.log( + warn(`Skipping unsafe project merge source outside CCS roots: ${currentTarget}`) + ); } return; @@ -678,6 +688,24 @@ class SharedManager { } } + /** + * Guard project merge operations to known CCS-managed roots only. + */ + private isSafeProjectsMergeSource(sourcePath: string, instanceName: string): boolean { + const resolvedSource = path.resolve(sourcePath); + const sharedContextRoot = path.resolve(path.join(this.sharedDir, 'context-groups')); + const instanceProjectsRoot = path.resolve( + path.join(this.instancesDir, instanceName, 'projects') + ); + + const isWithin = (child: string, root: string): boolean => + child === root || child.startsWith(`${root}${path.sep}`); + + return ( + isWithin(resolvedSource, sharedContextRoot) || isWithin(resolvedSource, instanceProjectsRoot) + ); + } + /** * Link directory with Windows fallback to recursive copy. */ diff --git a/src/web-server/routes/account-route-helpers.ts b/src/web-server/routes/account-route-helpers.ts new file mode 100644 index 00000000..645262be --- /dev/null +++ b/src/web-server/routes/account-route-helpers.ts @@ -0,0 +1,45 @@ +import type { CLIProxyProvider } from '../../cliproxy/types'; +import { isCLIProxyProvider } from '../../cliproxy/provider-capabilities'; + +export interface MergedAccountEntry { + type: string; + created: string; + last_used: string | null; + context_mode?: 'isolated' | 'shared'; + context_group?: string; + provider?: string; + displayName?: string; +} + +/** Parse CLIProxy account key format: "provider:accountId" */ +export function parseCliproxyKey( + key: string +): { provider: CLIProxyProvider; accountId: string } | null { + let normalizedKey = key; + if (key.startsWith('cliproxy:')) { + normalizedKey = key.slice('cliproxy:'.length); + } else if (key.startsWith('cliproxy+')) { + normalizedKey = key.slice('cliproxy+'.length); + } + const colonIndex = normalizedKey.indexOf(':'); + if (colonIndex === -1) return null; + + const provider = normalizedKey.slice(0, colonIndex); + const accountId = normalizedKey.slice(colonIndex + 1); + + if (!isCLIProxyProvider(provider) || !accountId) return null; + return { provider, accountId }; +} + +export function buildCliproxyAccountKey( + rawKey: string, + merged: Record +): string | null { + const candidateKeys = [rawKey, `cliproxy:${rawKey}`, `cliproxy+${rawKey}`]; + for (const key of candidateKeys) { + if (!merged[key]) { + return key; + } + } + return null; +} diff --git a/src/web-server/routes/account-routes.ts b/src/web-server/routes/account-routes.ts index a92422aa..90ace229 100644 --- a/src/web-server/routes/account-routes.ts +++ b/src/web-server/routes/account-routes.ts @@ -12,31 +12,24 @@ import { isUnifiedMode } from '../../config/unified-config-loader'; import { getAllAccountsSummary, setDefaultAccount as setCliproxyDefault, + getDefaultAccount as getCliproxyDefaultAccount, removeAccount as removeCliproxyAccount, bulkPauseAccounts, bulkResumeAccounts, soloAccount, } from '../../cliproxy/account-manager'; -import type { CLIProxyProvider } from '../../cliproxy/types'; import { isCLIProxyProvider } from '../../cliproxy/provider-capabilities'; +import { resolveAccountContextPolicy } from '../../auth/account-context'; +import { + buildCliproxyAccountKey, + parseCliproxyKey, + type MergedAccountEntry, +} from './account-route-helpers'; const router = Router(); const registry = new ProfileRegistry(); const instanceMgr = new InstanceManager(); -/** Parse CLIProxy account key format: "provider:accountId" */ -function parseCliproxyKey(key: string): { provider: CLIProxyProvider; accountId: string } | null { - const normalizedKey = key.startsWith('cliproxy:') ? key.slice('cliproxy:'.length) : key; - const colonIndex = normalizedKey.indexOf(':'); - if (colonIndex === -1) return null; - - const provider = normalizedKey.slice(0, colonIndex); - const accountId = normalizedKey.slice(colonIndex + 1); - - if (!isCLIProxyProvider(provider) || !accountId) return null; - return { provider, accountId }; -} - function hasAuthAccount(name: string): boolean { return registry.hasAccountUnified(name) || registry.hasProfile(name); } @@ -54,38 +47,29 @@ router.get('/', (_req: Request, res: Response): void => { const cliproxyAccounts = getAllAccountsSummary(); // Merge profiles: unified config takes precedence - const merged: Record< - string, - { - type: string; - created: string; - last_used: string | null; - context_mode?: 'isolated' | 'shared'; - context_group?: string; - provider?: string; - displayName?: string; - } - > = {}; + const merged: Record = {}; // Add legacy profiles first for (const [name, meta] of Object.entries(legacyProfiles)) { + const contextPolicy = resolveAccountContextPolicy(meta); merged[name] = { type: meta.type || 'account', created: meta.created, last_used: meta.last_used || null, - context_mode: meta.context_mode, - context_group: meta.context_group, + context_mode: contextPolicy.mode, + context_group: contextPolicy.group, }; } // Override with unified config accounts (takes precedence) for (const [name, account] of Object.entries(unifiedAccounts)) { + const contextPolicy = resolveAccountContextPolicy(account); merged[name] = { type: 'account', created: account.created, last_used: account.last_used, - context_mode: account.context_mode, - context_group: account.context_group, + context_mode: contextPolicy.mode, + context_group: contextPolicy.group, }; } @@ -99,7 +83,10 @@ router.get('/', (_req: Request, res: Response): void => { // Use unique ID for key to prevent collisions between accounts with same nickname/email const displayName = acct.nickname || acct.email || acct.id; const rawKey = `${provider}:${acct.id}`; - const key = merged[rawKey] ? `cliproxy:${rawKey}` : rawKey; + const key = buildCliproxyAccountKey(rawKey, merged); + if (!key) { + continue; + } merged[key] = { type: 'cliproxy', provider, @@ -202,6 +189,14 @@ router.delete('/:name', (req: Request, res: Response): void => { // Check if this is a CLIProxy account (format: "provider:accountId") const cliproxyKey = !hasAuthAccount(name) ? parseCliproxyKey(name) : null; if (cliproxyKey) { + const defaultCliproxyAccount = getCliproxyDefaultAccount(cliproxyKey.provider); + if (defaultCliproxyAccount?.id === cliproxyKey.accountId) { + res.status(400).json({ + error: `Cannot delete default CLIProxy account: ${name}. Set another default first.`, + }); + return; + } + const success = removeCliproxyAccount(cliproxyKey.provider, cliproxyKey.accountId); if (!success) { res.status(404).json({ error: `CLIProxy account not found: ${name}` }); diff --git a/src/web-server/routes/config-routes.ts b/src/web-server/routes/config-routes.ts index d00985ca..537f7348 100644 --- a/src/web-server/routes/config-routes.ts +++ b/src/web-server/routes/config-routes.ts @@ -18,9 +18,57 @@ import { getBackupDirectories, } from '../../config/migration-manager'; import { isUnifiedConfig } from '../../config/unified-config-types'; +import { isValidContextGroupName, normalizeContextGroupName } from '../../auth/account-context'; const router = Router(); +function validateAccountContextMetadata(config: unknown): string | null { + if (typeof config !== 'object' || config === null) { + return 'Invalid config payload'; + } + + const candidate = config as Record; + const accounts = candidate.accounts; + if (accounts === undefined) { + return null; + } + + if (typeof accounts !== 'object' || accounts === null || Array.isArray(accounts)) { + return 'Invalid config.accounts: expected object'; + } + + for (const [accountName, accountValue] of Object.entries(accounts as Record)) { + if (typeof accountValue !== 'object' || accountValue === null || Array.isArray(accountValue)) { + return `Invalid config.accounts.${accountName}: expected object`; + } + + const account = accountValue as Record; + const mode = account.context_mode; + const group = account.context_group; + + if (mode !== undefined && mode !== 'isolated' && mode !== 'shared') { + return `Invalid config.accounts.${accountName}.context_mode: expected isolated|shared`; + } + + if (group !== undefined && typeof group !== 'string') { + return `Invalid config.accounts.${accountName}.context_group: expected string`; + } + + if (mode !== 'shared' && group !== undefined) { + return `Invalid config.accounts.${accountName}: context_group requires context_mode=shared`; + } + + if (mode === 'shared' && typeof group === 'string' && group.trim().length > 0) { + const normalizedGroup = normalizeContextGroupName(group); + if (!isValidContextGroupName(normalizedGroup)) { + return `Invalid config.accounts.${accountName}.context_group`; + } + } + } + + return null; +} + /** * GET /api/config/format - Return current config format and migration status */ @@ -82,6 +130,12 @@ router.put('/', (req: Request, res: Response): void => { return; } + const accountContextError = validateAccountContextMetadata(config); + if (accountContextError) { + res.status(400).json({ error: accountContextError }); + return; + } + try { saveUnifiedConfig(config); res.json({ success: true }); diff --git a/tests/unit/account-context.test.ts b/tests/unit/account-context.test.ts index 8b18b55d..7e282a58 100644 --- a/tests/unit/account-context.test.ts +++ b/tests/unit/account-context.test.ts @@ -2,6 +2,7 @@ import { describe, expect, it } from 'bun:test'; import { MAX_CONTEXT_GROUP_LENGTH, isValidAccountProfileName, + policyToAccountContextMetadata, resolveAccountContextPolicy, resolveCreateAccountContext, } from '../../src/auth/account-context'; @@ -28,4 +29,15 @@ describe('account context helpers', () => { expect(resolved.mode).toBe('shared'); expect(resolved.group).toBe('default'); }); + + it('round-trips shared policy metadata with normalized context group', () => { + const metadata = policyToAccountContextMetadata({ + mode: 'shared', + group: 'Sprint-A', + }); + + const resolved = resolveAccountContextPolicy(metadata); + expect(resolved.mode).toBe('shared'); + expect(resolved.group).toBe('sprint-a'); + }); }); diff --git a/tests/unit/auth-list-context.test.ts b/tests/unit/auth-list-context.test.ts index 7f24b531..b62cb798 100644 --- a/tests/unit/auth-list-context.test.ts +++ b/tests/unit/auth-list-context.test.ts @@ -85,4 +85,78 @@ describe('auth list context metadata', () => { expect(work?.context_mode).toBe('shared'); expect(work?.context_group).toBe('sprint-a'); }); + + it('prefers unified context metadata over legacy when profile names overlap', async () => { + const ccsDir = path.join(tempRoot, '.ccs'); + fs.mkdirSync(ccsDir, { recursive: true }); + + fs.writeFileSync( + path.join(ccsDir, 'profiles.json'), + JSON.stringify( + { + version: '2.0.0', + profiles: { + work: { + type: 'account', + created: '2026-01-01T00:00:00.000Z', + last_used: null, + context_mode: 'isolated', + }, + }, + default: null, + }, + null, + 2 + ) + ); + + fs.writeFileSync( + path.join(ccsDir, 'config.yaml'), + [ + 'version: 8', + 'accounts:', + ' work:', + ' created: "2026-02-01T00:00:00.000Z"', + ' last_used: null', + ' context_mode: shared', + ' context_group: sprint-a', + 'profiles: {}', + 'cliproxy:', + ' oauth_accounts: {}', + ' providers: {}', + ' variants: {}', + ].join('\n'), + 'utf8' + ); + + const registry = new ProfileRegistry(); + const instanceMgr = new InstanceManager(); + const lines: string[] = []; + const originalLog = console.log; + console.log = (...args: unknown[]) => { + lines.push(args.map(String).join(' ')); + }; + + try { + await handleList( + { + registry, + instanceMgr, + version: 'test', + }, + ['--json'] + ); + } finally { + console.log = originalLog; + } + + const payload = JSON.parse(lines.join('\n')) as { + profiles: Array<{ name: string; context_mode?: string; context_group?: string | null }>; + }; + const work = payload.profiles.find((profile) => profile.name === 'work'); + + expect(work).toBeTruthy(); + expect(work?.context_mode).toBe('shared'); + expect(work?.context_group).toBe('sprint-a'); + }); }); diff --git a/tests/unit/config/migration-manager.test.ts b/tests/unit/config/migration-manager.test.ts index ec446d35..1e09fcaf 100644 --- a/tests/unit/config/migration-manager.test.ts +++ b/tests/unit/config/migration-manager.test.ts @@ -169,4 +169,46 @@ describe('migration-manager legacy kimi compatibility', () => { expect(unified?.accounts.personal.context_mode).toBe('isolated'); expect(unified?.accounts.personal.context_group).toBeUndefined(); }); + + it('normalizes valid legacy shared groups and drops invalid ones during migration', async () => { + fs.writeFileSync( + path.join(ccsDir, 'profiles.json'), + JSON.stringify( + { + default: 'work', + profiles: { + work: { + type: 'account', + created: '2026-02-01T00:00:00.000Z', + last_used: null, + context_mode: 'shared', + context_group: 'Sprint-A', + }, + broken: { + type: 'account', + created: '2026-02-02T00:00:00.000Z', + last_used: null, + context_mode: 'shared', + context_group: '###', + }, + }, + }, + null, + 2 + ) + ); + + const result = await migrate(false); + expect(result.success).toBe(true); + expect( + result.warnings.some((warning) => + warning.includes('Skipped invalid context group for account "broken"') + ) + ).toBe(true); + + const unified = loadUnifiedConfig(); + expect(unified?.accounts.work.context_group).toBe('sprint-a'); + expect(unified?.accounts.broken.context_mode).toBe('shared'); + expect(unified?.accounts.broken.context_group).toBeUndefined(); + }); }); diff --git a/tests/unit/web-server/account-routes-context.test.ts b/tests/unit/web-server/account-routes-context.test.ts new file mode 100644 index 00000000..0563c129 --- /dev/null +++ b/tests/unit/web-server/account-routes-context.test.ts @@ -0,0 +1,133 @@ +import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it } from 'bun:test'; +import express from 'express'; +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; +import type { Server } from 'http'; +import accountRoutes from '../../../src/web-server/routes/account-routes'; + +async function getJson(baseUrl: string, routePath: string): Promise { + const response = await fetch(`${baseUrl}${routePath}`); + expect(response.status).toBe(200); + return (await response.json()) as T; +} + +describe('web-server account-routes context normalization', () => { + let server: Server; + let baseUrl = ''; + let tempHome = ''; + let originalCcsHome: string | undefined; + let originalCcsUnified: string | undefined; + + beforeAll(async () => { + const app = express(); + app.use('/api/accounts', accountRoutes); + + await new Promise((resolve, reject) => { + server = app.listen(0, '127.0.0.1'); + const handleError = (error: Error) => reject(error); + server.once('error', handleError); + server.once('listening', () => { + server.off('error', handleError); + resolve(); + }); + }); + + const address = server.address(); + if (!address || typeof address === 'string') { + throw new Error('Unable to resolve test server port'); + } + baseUrl = `http://127.0.0.1:${address.port}`; + }); + + afterAll(async () => { + await new Promise((resolve) => server.close(() => resolve())); + }); + + beforeEach(() => { + tempHome = fs.mkdtempSync(path.join(os.tmpdir(), 'ccs-account-routes-context-')); + originalCcsHome = process.env.CCS_HOME; + originalCcsUnified = process.env.CCS_UNIFIED_CONFIG; + + process.env.CCS_HOME = tempHome; + process.env.CCS_UNIFIED_CONFIG = '1'; + }); + + afterEach(() => { + if (originalCcsHome !== undefined) process.env.CCS_HOME = originalCcsHome; + else delete process.env.CCS_HOME; + + if (originalCcsUnified !== undefined) process.env.CCS_UNIFIED_CONFIG = originalCcsUnified; + else delete process.env.CCS_UNIFIED_CONFIG; + + if (tempHome && fs.existsSync(tempHome)) { + fs.rmSync(tempHome, { recursive: true, force: true }); + } + }); + + it('normalizes invalid persisted account context metadata in API response', async () => { + const ccsDir = path.join(tempHome, '.ccs'); + fs.mkdirSync(ccsDir, { recursive: true }); + + fs.writeFileSync( + path.join(ccsDir, 'config.yaml'), + [ + 'version: 8', + 'accounts:', + ' work:', + ' created: "2026-02-01T00:00:00.000Z"', + ' last_used: null', + ' context_mode: weird', + ' context_group: "###"', + 'profiles: {}', + 'cliproxy:', + ' oauth_accounts: {}', + ' providers: {}', + ' variants: {}', + ].join('\n'), + 'utf8' + ); + + const payload = await getJson<{ + accounts: Array<{ name: string; context_mode?: string; context_group?: string }>; + }>(baseUrl, '/api/accounts'); + + const work = payload.accounts.find((account) => account.name === 'work'); + expect(work).toBeTruthy(); + expect(work?.context_mode).toBe('isolated'); + expect(work && 'context_group' in work).toBe(false); + }); + + it('falls back shared accounts with invalid groups to default shared group', async () => { + const ccsDir = path.join(tempHome, '.ccs'); + fs.mkdirSync(ccsDir, { recursive: true }); + + fs.writeFileSync( + path.join(ccsDir, 'config.yaml'), + [ + 'version: 8', + 'accounts:', + ' work:', + ' created: "2026-02-01T00:00:00.000Z"', + ' last_used: null', + ' context_mode: shared', + ' context_group: "###"', + 'profiles: {}', + 'cliproxy:', + ' oauth_accounts: {}', + ' providers: {}', + ' variants: {}', + ].join('\n'), + 'utf8' + ); + + const payload = await getJson<{ + accounts: Array<{ name: string; context_mode?: string; context_group?: string }>; + }>(baseUrl, '/api/accounts'); + + const work = payload.accounts.find((account) => account.name === 'work'); + expect(work).toBeTruthy(); + expect(work?.context_mode).toBe('shared'); + expect(work?.context_group).toBe('default'); + }); +}); diff --git a/tests/unit/web-server/config-routes-account-context.test.ts b/tests/unit/web-server/config-routes-account-context.test.ts new file mode 100644 index 00000000..850f654f --- /dev/null +++ b/tests/unit/web-server/config-routes-account-context.test.ts @@ -0,0 +1,141 @@ +import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it } from 'bun:test'; +import express from 'express'; +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; +import type { Server } from 'http'; +import configRoutes from '../../../src/web-server/routes/config-routes'; +import { createEmptyUnifiedConfig } from '../../../src/config/unified-config-types'; + +async function putJson(baseUrl: string, routePath: string, body: unknown): Promise { + return fetch(`${baseUrl}${routePath}`, { + method: 'PUT', + headers: { + 'Content-Type': 'application/json', + }, + body: JSON.stringify(body), + }); +} + +describe('web-server config-routes account context validation', () => { + let server: Server; + let baseUrl = ''; + let tempHome = ''; + let originalCcsHome: string | undefined; + + beforeAll(async () => { + const app = express(); + app.use(express.json()); + app.use('/api/config', configRoutes); + + await new Promise((resolve, reject) => { + server = app.listen(0, '127.0.0.1'); + const handleError = (error: Error) => reject(error); + server.once('error', handleError); + server.once('listening', () => { + server.off('error', handleError); + resolve(); + }); + }); + + const address = server.address(); + if (!address || typeof address === 'string') { + throw new Error('Unable to resolve test server port'); + } + baseUrl = `http://127.0.0.1:${address.port}`; + }); + + afterAll(async () => { + await new Promise((resolve) => server.close(() => resolve())); + }); + + beforeEach(() => { + tempHome = fs.mkdtempSync(path.join(os.tmpdir(), 'ccs-config-routes-context-')); + originalCcsHome = process.env.CCS_HOME; + process.env.CCS_HOME = tempHome; + fs.mkdirSync(path.join(tempHome, '.ccs'), { recursive: true }); + }); + + afterEach(() => { + if (originalCcsHome !== undefined) process.env.CCS_HOME = originalCcsHome; + else delete process.env.CCS_HOME; + + if (tempHome && fs.existsSync(tempHome)) { + fs.rmSync(tempHome, { recursive: true, force: true }); + } + }); + + it('rejects invalid account context_mode values', async () => { + const response = await putJson(baseUrl, '/api/config', { + version: 8, + accounts: { + work: { + created: '2026-01-01T00:00:00.000Z', + last_used: null, + context_mode: 'weird', + }, + }, + profiles: {}, + cliproxy: { oauth_accounts: {}, providers: [], variants: {} }, + }); + + expect(response.status).toBe(400); + const payload = (await response.json()) as { error: string }; + expect(payload.error).toContain('context_mode'); + }); + + it('rejects context_group when mode is not shared', async () => { + const response = await putJson(baseUrl, '/api/config', { + version: 8, + accounts: { + work: { + created: '2026-01-01T00:00:00.000Z', + last_used: null, + context_mode: 'isolated', + context_group: 'sprint-a', + }, + }, + profiles: {}, + cliproxy: { oauth_accounts: {}, providers: [], variants: {} }, + }); + + expect(response.status).toBe(400); + const payload = (await response.json()) as { error: string }; + expect(payload.error).toContain('context_group requires context_mode=shared'); + }); + + it('rejects invalid shared context_group names', async () => { + const response = await putJson(baseUrl, '/api/config', { + version: 8, + accounts: { + work: { + created: '2026-01-01T00:00:00.000Z', + last_used: null, + context_mode: 'shared', + context_group: '###', + }, + }, + profiles: {}, + cliproxy: { oauth_accounts: {}, providers: [], variants: {} }, + }); + + expect(response.status).toBe(400); + const payload = (await response.json()) as { error: string }; + expect(payload.error).toContain('context_group'); + }); + + it('accepts valid shared context metadata', async () => { + const config = createEmptyUnifiedConfig(); + config.accounts.work = { + created: '2026-01-01T00:00:00.000Z', + last_used: null, + context_mode: 'shared', + context_group: 'Sprint-A', + }; + const response = await putJson(baseUrl, '/api/config', config); + + expect(response.status).toBe(200); + const payload = (await response.json()) as { success: boolean }; + expect(payload.success).toBe(true); + }); +}); diff --git a/ui/src/components/account/create-auth-profile-dialog.tsx b/ui/src/components/account/create-auth-profile-dialog.tsx index a82dc72f..51b7b997 100644 --- a/ui/src/components/account/create-auth-profile-dialog.tsx +++ b/ui/src/components/account/create-auth-profile-dialog.tsx @@ -22,6 +22,8 @@ interface CreateAuthProfileDialogProps { onClose: () => void; } +const MAX_CONTEXT_GROUP_LENGTH = 64; + export function CreateAuthProfileDialog({ open, onClose }: CreateAuthProfileDialogProps) { const [profileName, setProfileName] = useState(''); const [shareContext, setShareContext] = useState(false); @@ -32,7 +34,9 @@ export function CreateAuthProfileDialog({ open, onClose }: CreateAuthProfileDial const isValidName = /^[a-zA-Z][a-zA-Z0-9_-]*$/.test(profileName); const normalizedGroup = contextGroup.trim().toLowerCase(); const isValidContextGroup = - normalizedGroup.length === 0 || /^[a-zA-Z][a-zA-Z0-9_-]*$/.test(normalizedGroup); + normalizedGroup.length === 0 || + (normalizedGroup.length <= MAX_CONTEXT_GROUP_LENGTH && + /^[a-zA-Z][a-zA-Z0-9_-]*$/.test(normalizedGroup)); const command = profileName && isValidName @@ -119,7 +123,7 @@ export function CreateAuthProfileDialog({ open, onClose }: CreateAuthProfileDial {contextGroup.trim().length > 0 && !isValidContextGroup && (

Group must start with a letter and use only letters, numbers, dashes, or - underscores. + underscores (max {MAX_CONTEXT_GROUP_LENGTH} chars).

)} From 0309630193a222c0d03dbcdad749101af1ee5433 Mon Sep 17 00:00:00 2001 From: Tam Nhu Tran Date: Wed, 25 Feb 2026 04:19:28 +0700 Subject: [PATCH 3/3] fix(auth): harden shared-context isolation and config safety --- README.md | 36 +++- docs/codebase-summary.md | 17 +- docs/dashboard-auth-cli.md | 35 +++- src/auth/account-context.ts | 2 +- src/auth/auth-commands.ts | 5 + src/auth/commands/create-command-env.ts | 57 ++++++ src/auth/commands/create-command.ts | 86 +++------ src/auth/profile-registry.ts | 32 +++- src/management/instance-manager.ts | 102 +--------- src/management/profile-context-sync-lock.ts | 177 ++++++++++++++++++ src/management/shared-manager.ts | 58 ++++-- src/web-server/routes/account-routes.ts | 23 ++- src/web-server/routes/config-routes.ts | 22 ++- tests/unit/account-context.test.ts | 11 ++ ...ile-registry-context-normalization.test.ts | 92 +++++++++ tests/unit/shared-context-policy.test.ts | 44 +++++ .../web-server/account-routes-context.test.ts | 43 +++++ .../config-routes-account-context.test.ts | 50 +++++ 18 files changed, 695 insertions(+), 197 deletions(-) create mode 100644 src/auth/commands/create-command-env.ts create mode 100644 src/management/profile-context-sync-lock.ts create mode 100644 tests/unit/auth/profile-registry-context-normalization.test.ts diff --git a/README.md b/README.md index 9c173863..a50d7a6f 100644 --- a/README.md +++ b/README.md @@ -59,7 +59,7 @@ Want to run the dashboard in Docker? See `docker/README.md`. The dashboard provides visual management for all account types: -- **Claude Accounts**: Create isolated instances (work, personal, client) +- **Claude Accounts**: Isolation-first by default (work, personal, client), with explicit shared context opt-in - **OAuth Providers**: One-click auth for Gemini, Codex, Antigravity, Kiro, Copilot - **API Profiles**: Configure GLM, Kimi with your keys - **Health Monitor**: Real-time status across all profiles @@ -221,17 +221,45 @@ ccs work "implement feature" # Terminal 1 ccs "review code" # Terminal 2 (personal account) ``` -Need continuity between two accounts for the same project? Opt in to shared context: +#### Account Context Modes (Isolation-First) + +Account profiles are isolated by default. + +| Mode | Default | Requirements | +|------|---------|--------------| +| `isolated` | Yes | No `context_group` required | +| `shared` | No (explicit opt-in) | Valid non-empty `context_group` | + +Opt in to shared context when needed: ```bash # Share context with default group ccs auth create backup --share-context -# Or isolate by named group (only accounts in this group share context) +# Share context only within named group ccs auth create backup2 --context-group sprint-a ``` -Isolation remains the default. Shared context only links project workspace data; credentials stay per-account. +Shared mode metadata in `~/.ccs/config.yaml`: + +```yaml +accounts: + work: + created: "2026-02-24T00:00:00.000Z" + last_used: null + context_mode: "shared" + context_group: "team-alpha" +``` + +`context_group` rules: + +- lowercase letters, numbers, `_`, `-` +- must start with a letter +- max length `64` +- non-empty after normalization +- normalized by trim + lowercase + whitespace collapse (`" Team Alpha "` -> `"team-alpha"`) + +Shared context links project workspace data only. Credentials remain isolated per account.
diff --git a/docs/codebase-summary.md b/docs/codebase-summary.md index 972b56c6..d138f34b 100644 --- a/docs/codebase-summary.md +++ b/docs/codebase-summary.md @@ -1,8 +1,8 @@ # CCS Codebase Summary -Last Updated: 2026-02-04 +Last Updated: 2026-02-24 -Comprehensive overview of the modularized CCS codebase structure following the Phase 9 modularization effort (Settings, Analytics, Auth Monitor splits + Test Infrastructure), v7.1 Remote CLIProxy feature, v7.2 Kiro + GitHub Copilot (ghcp) OAuth providers, v7.14 Hybrid Quota Management, and v7.34 Image Analysis Hook. +Comprehensive overview of the modularized CCS codebase structure following the Phase 9 modularization effort (Settings, Analytics, Auth Monitor splits + Test Infrastructure), v7.1 Remote CLIProxy feature, v7.2 Kiro + GitHub Copilot (ghcp) OAuth providers, v7.14 Hybrid Quota Management, v7.34 Image Analysis Hook, and account-context validation hardening. ## Repository Structure @@ -201,6 +201,19 @@ src/ | Services | `web-server/`, `api/` | HTTP server, API services | | Utilities | `utils/`, `management/` | Helpers, diagnostics | +### Account Context Metadata Flow + +- Source fields: `accounts..context_mode` and `accounts..context_group` in `~/.ccs/config.yaml`. +- Runtime policy resolver: `src/auth/account-context.ts`. +- Metadata storage normalization: `src/auth/profile-registry.ts`. +- API write validation: `PUT /api/config` in `src/web-server/routes/config-routes.ts`. +- Rules: + - mode is isolation-first (`isolated` default, `shared` opt-in) + - shared mode requires non-empty valid `context_group` + - `context_group` is normalized (trim + lowercase + whitespace collapse to `-`) + - API route rejects `context_group` when mode is not `shared` + - registry normalization drops malformed persisted `context_group` values + ### Target Adapter Module The targets module provides an extensible interface for dispatching profiles to different CLI implementations. diff --git a/docs/dashboard-auth-cli.md b/docs/dashboard-auth-cli.md index 60c442e4..85d1a7ed 100644 --- a/docs/dashboard-auth-cli.md +++ b/docs/dashboard-auth-cli.md @@ -1,6 +1,6 @@ # Dashboard Authentication CLI -Last Updated: 2026-02-04 +Last Updated: 2026-02-24 CLI commands for managing CCS dashboard authentication. @@ -10,6 +10,35 @@ The CCS dashboard (`ccs config`) can be protected with username/password authent Authentication is **disabled by default** for backward compatibility. Use the CLI to configure and enable it. +## Account Context Modes (Related Feature) + +Dashboard auth and account context metadata are separate: + +- `dashboard_auth`: protects dashboard access with username/password +- `accounts..context_mode/context_group`: controls isolated vs shared account context + +Account context is isolation-first: + +| Mode | Default | Requirement | +|------|---------|-------------| +| `isolated` | Yes | No `context_group` required | +| `shared` | No (opt-in) | Valid non-empty `context_group` | + +`context_group` normalization and validation: + +- trim + lowercase + collapse internal whitespace to `-` +- allowed characters: lowercase letters, numbers, `_`, `-` +- must start with a letter +- max length: 64 +- shared mode requires non-empty value after normalization + +`PUT /api/config` behavior for account context: + +- rejects invalid unified payloads +- rejects explicit `context_mode: shared` with invalid/empty `context_group` +- normalizes valid shared `context_group` before save +- rejects `context_group` when mode is not `shared` + ## Commands ### `ccs config auth setup` @@ -162,6 +191,10 @@ export CCS_DASHBOARD_PASSWORD_HASH='$2b$10$...' Check `session_timeout_hours` in config. Default is 24 hours. +### "Invalid ... context_group ..." + +This error comes from `PUT /api/config` when an account explicitly sets shared mode with an invalid group. Use a canonical group value (for example: `team-alpha`). + ## See Also - [Dashboard Auth Feature](https://ccs.kaitran.ca/features/dashboard-auth) - Full documentation diff --git a/src/auth/account-context.ts b/src/auth/account-context.ts index f97a89b5..c30a0f7c 100644 --- a/src/auth/account-context.ts +++ b/src/auth/account-context.ts @@ -38,7 +38,7 @@ const CONTEXT_GROUP_PATTERN = /^[a-zA-Z][a-zA-Z0-9_-]*$/; * Normalize context group names so paths and config stay consistent. */ export function normalizeContextGroupName(value: string): string { - return value.trim().toLowerCase(); + return value.trim().toLowerCase().replace(/\s+/g, '-'); } /** diff --git a/src/auth/auth-commands.ts b/src/auth/auth-commands.ts index e94c47f8..2ed849ee 100644 --- a/src/auth/auth-commands.ts +++ b/src/auth/auth-commands.ts @@ -14,6 +14,7 @@ import ProfileRegistry from './profile-registry'; import { InstanceManager } from '../management/instance-manager'; import { initUI, header, subheader, color, dim, warn, fail } from '../utils/ui'; +import { MAX_CONTEXT_GROUP_LENGTH } from './account-context'; import packageJson from '../../package.json'; // Import command handlers from modular structure @@ -127,6 +128,10 @@ class AuthCommands { console.log( ` Account profiles stay isolated unless you opt in with ${color('--share-context', 'command')}.` ); + console.log(` Shared context groups are normalized (trim + lowercase) and spaces become "-".`); + console.log( + ` ${color('context_group', 'path')} must be non-empty and <= ${MAX_CONTEXT_GROUP_LENGTH} chars in shared mode.` + ); console.log(''); } diff --git a/src/auth/commands/create-command-env.ts b/src/auth/commands/create-command-env.ts new file mode 100644 index 00000000..630e9a0e --- /dev/null +++ b/src/auth/commands/create-command-env.ts @@ -0,0 +1,57 @@ +const AMBIENT_PROVIDER_PREFIXES = [ + 'ANTHROPIC_', + 'OPENAI_', + 'GOOGLE_', + 'GEMINI_', + 'MINIMAX_', + 'QWEN_', + 'DEEPSEEK_', + 'KIMI_', + 'AZURE_', + 'OLLAMA_', + 'OPENROUTER_', + 'XAI_', + 'MISTRAL_', + 'COHERE_', + 'PERPLEXITY_', + 'TOGETHER_', + 'FIREWORKS_', +]; +const AMBIENT_PROVIDER_EXACT_KEYS = new Set([ + 'OPENROUTER_API_KEY', + 'OPENROUTER_KEY', + 'XAI_API_KEY', + 'MISTRAL_API_KEY', + 'COHERE_API_KEY', +]); +const AMBIENT_PROVIDER_SUFFIXES = [ + '_API_KEY', + '_AUTH_TOKEN', + '_ACCESS_TOKEN', + '_SECRET_KEY', + '_API_TOKEN', + '_BEARER_TOKEN', + '_SESSION_TOKEN', +]; + +export function stripAmbientProviderCredentials(env: NodeJS.ProcessEnv): NodeJS.ProcessEnv { + const sanitized: NodeJS.ProcessEnv = { ...env }; + + for (const envKey of Object.keys(sanitized)) { + const normalizedKey = envKey.toUpperCase(); + + if (normalizedKey === 'CLAUDE_CONFIG_DIR') { + continue; + } + + if ( + AMBIENT_PROVIDER_PREFIXES.some((prefix) => normalizedKey.startsWith(prefix)) || + AMBIENT_PROVIDER_EXACT_KEYS.has(normalizedKey) || + AMBIENT_PROVIDER_SUFFIXES.some((suffix) => normalizedKey.endsWith(suffix)) + ) { + delete sanitized[envKey]; + } + } + + return sanitized; +} diff --git a/src/auth/commands/create-command.ts b/src/auth/commands/create-command.ts index 614a0ec8..8c169c1d 100644 --- a/src/auth/commands/create-command.ts +++ b/src/auth/commands/create-command.ts @@ -20,52 +20,12 @@ import { import { exitWithError } from '../../errors'; import { ExitCode } from '../../errors/exit-codes'; import { CommandContext, parseArgs } from './types'; +import { stripAmbientProviderCredentials } from './create-command-env'; function sanitizeProfileNameForInstance(name: string): string { return name.replace(/[^a-zA-Z0-9_-]/g, '-').toLowerCase(); } -const AMBIENT_PROVIDER_PREFIXES = [ - 'ANTHROPIC_', - 'OPENAI_', - 'GOOGLE_', - 'GEMINI_', - 'MINIMAX_', - 'QWEN_', - 'DEEPSEEK_', - 'KIMI_', - 'AZURE_', - 'OLLAMA_', -]; -const AMBIENT_PROVIDER_EXACT_KEYS = new Set([ - 'OPENROUTER_API_KEY', - 'OPENROUTER_KEY', - 'XAI_API_KEY', - 'MISTRAL_API_KEY', - 'COHERE_API_KEY', -]); -const AMBIENT_PROVIDER_SUFFIXES = ['_API_KEY', '_AUTH_TOKEN', '_ACCESS_TOKEN', '_SECRET_KEY']; - -function stripAmbientProviderCredentials(env: NodeJS.ProcessEnv): NodeJS.ProcessEnv { - const sanitized: NodeJS.ProcessEnv = { ...env }; - - for (const envKey of Object.keys(sanitized)) { - if (envKey === 'CLAUDE_CONFIG_DIR') { - continue; - } - - if ( - AMBIENT_PROVIDER_PREFIXES.some((prefix) => envKey.startsWith(prefix)) || - AMBIENT_PROVIDER_EXACT_KEYS.has(envKey) || - AMBIENT_PROVIDER_SUFFIXES.some((suffix) => envKey.endsWith(suffix)) - ) { - delete sanitized[envKey]; - } - } - - return sanitized; -} - /** * Handle the create command */ @@ -74,9 +34,14 @@ export async function handleCreate(ctx: CommandContext, args: string[]): Promise const { profileName, force, shareContext, contextGroup, unknownFlags } = parseArgs(args); if (unknownFlags && unknownFlags.length > 0) { - const unknownList = unknownFlags.join(', '); + const unknownList = unknownFlags.map((flag) => `"${flag}"`).join(', '); console.log(fail(`Unknown option(s): ${unknownList}`)); console.log(''); + console.log( + `Usage: ${color('ccs auth create [--force] [--share-context] [--context-group ]', 'command')}` + ); + console.log(`Help: ${color('ccs auth --help', 'command')}`); + console.log(''); exitWithError(`Unknown option(s): ${unknownList}`, ExitCode.PROFILE_ERROR); } @@ -135,15 +100,17 @@ export async function handleCreate(ctx: CommandContext, args: string[]): Promise const contextPolicy = resolvedContext.policy; const contextMetadata = policyToAccountContextMetadata(contextPolicy); const useUnifiedConfig = isUnifiedMode(); - const createdProfile = useUnifiedConfig ? !existsUnified : !existsLegacy; - const previousLegacyProfile: ProfileMetadata | undefined = - !useUnifiedConfig && existsLegacy ? ctx.registry.getProfile(profileName) : undefined; - const previousUnifiedProfile = - useUnifiedConfig && existsUnified - ? ctx.registry.getAllAccountsUnified()[profileName] - : undefined; + const profileExistedBeforeCreate = existsLegacy || existsUnified; + const createdUnifiedProfile = useUnifiedConfig && !existsUnified; + const createdLegacyProfile = !useUnifiedConfig && !existsLegacy; + const previousLegacyProfile: ProfileMetadata | undefined = existsLegacy + ? ctx.registry.getProfile(profileName) + : undefined; + const previousUnifiedProfile = existsUnified + ? ctx.registry.getAllAccountsUnified()[profileName] + : undefined; const previousContextPolicy = - !createdProfile && (previousUnifiedProfile || previousLegacyProfile) + profileExistedBeforeCreate && (previousUnifiedProfile || previousLegacyProfile) ? resolveAccountContextPolicy(previousUnifiedProfile || previousLegacyProfile) : undefined; @@ -160,22 +127,21 @@ export async function handleCreate(ctx: CommandContext, args: string[]): Promise const rollbackMetadata = (): void => { try { if (useUnifiedConfig) { - if (createdProfile) { + if (createdUnifiedProfile) { if (ctx.registry.hasAccountUnified(profileName)) { ctx.registry.removeAccountUnified(profileName); } } else if (previousUnifiedProfile) { ctx.registry.updateAccountUnified(profileName, previousUnifiedProfile); } - return; - } - - if (createdProfile) { - if (ctx.registry.hasProfile(profileName)) { - ctx.registry.deleteProfile(profileName); + } else { + if (createdLegacyProfile) { + if (ctx.registry.hasProfile(profileName)) { + ctx.registry.deleteProfile(profileName); + } + } else if (previousLegacyProfile) { + ctx.registry.updateProfile(profileName, previousLegacyProfile); } - } else if (previousLegacyProfile) { - ctx.registry.updateProfile(profileName, previousLegacyProfile); } } catch { // Best-effort rollback to avoid leaving stale accounts after failed login. @@ -190,7 +156,7 @@ export async function handleCreate(ctx: CommandContext, args: string[]): Promise rollbackMetadata(); - if (createdProfile) { + if (!profileExistedBeforeCreate) { try { ctx.instanceMgr.deleteInstance(profileName); } catch { diff --git a/src/auth/profile-registry.ts b/src/auth/profile-registry.ts index 5870db61..f70f7e79 100644 --- a/src/auth/profile-registry.ts +++ b/src/auth/profile-registry.ts @@ -8,6 +8,7 @@ import { } from '../config/unified-config-loader'; import type { AccountConfig } from '../config/unified-config-types'; import { getCcsDir } from '../utils/config-manager'; +import { isValidContextGroupName, normalizeContextGroupName } from './account-context'; /** * Profile Registry (Simplified) @@ -51,13 +52,31 @@ export class ProfileRegistry { this.profilesPath = path.join(getCcsDir(), 'profiles.json'); } + private normalizeContextGroupValue(value: unknown): string | undefined { + if (typeof value !== 'string') { + return undefined; + } + + const normalized = normalizeContextGroupName(value); + if (normalized.length === 0 || !isValidContextGroupName(normalized)) { + return undefined; + } + + return normalized; + } + private normalizeLegacyProfileMetadata(metadata: ProfileMetadata): ProfileMetadata { const normalized: ProfileMetadata = { ...metadata }; if (normalized.context_mode !== 'shared') { delete normalized.context_group; - } else if (!normalized.context_group || normalized.context_group.trim().length === 0) { - delete normalized.context_group; + } else { + const normalizedGroup = this.normalizeContextGroupValue(normalized.context_group); + if (normalizedGroup) { + normalized.context_group = normalizedGroup; + } else { + delete normalized.context_group; + } } return normalized; @@ -68,8 +87,13 @@ export class ProfileRegistry { if (normalized.context_mode !== 'shared') { delete normalized.context_group; - } else if (!normalized.context_group || normalized.context_group.trim().length === 0) { - delete normalized.context_group; + } else { + const normalizedGroup = this.normalizeContextGroupValue(normalized.context_group); + if (normalizedGroup) { + normalized.context_group = normalizedGroup; + } else { + delete normalized.context_group; + } } return normalized; diff --git a/src/management/instance-manager.ts b/src/management/instance-manager.ts index e39acbb1..c0142f06 100644 --- a/src/management/instance-manager.ts +++ b/src/management/instance-manager.ts @@ -8,8 +8,8 @@ import * as fs from 'fs'; import * as path from 'path'; -import { createHash } from 'crypto'; import SharedManager from './shared-manager'; +import ProfileContextSyncLock from './profile-context-sync-lock'; import { AccountContextPolicy, DEFAULT_ACCOUNT_CONTEXT_MODE } from '../auth/account-context'; import { getCcsDir } from '../utils/config-manager'; @@ -18,13 +18,13 @@ import { getCcsDir } from '../utils/config-manager'; */ class InstanceManager { private readonly instancesDir: string; - private readonly locksDir: string; private readonly sharedManager: SharedManager; + private readonly contextSyncLock: ProfileContextSyncLock; constructor() { this.instancesDir = path.join(getCcsDir(), 'instances'); - this.locksDir = path.join(this.instancesDir, '.locks'); this.sharedManager = new SharedManager(); + this.contextSyncLock = new ProfileContextSyncLock(this.instancesDir); } /** @@ -37,7 +37,7 @@ class InstanceManager { const instancePath = this.getInstancePath(profileName); // Serialize context sync operations per profile across processes. - await this.withContextSyncLock(profileName, async () => { + await this.contextSyncLock.withLock(profileName, async () => { // Lazy initialization if (!fs.existsSync(instancePath)) { this.initializeInstance(profileName, instancePath); @@ -201,100 +201,6 @@ class InstanceManager { // Replace unsafe characters with dash return name.replace(/[^a-zA-Z0-9_-]/g, '-').toLowerCase(); } - - private getContextSyncLockPath(profileName: string): string { - const safeName = this.sanitizeName(profileName); - // Keep lock filenames deterministic while preventing normalized-name collisions. - const profileHash = createHash('sha1').update(profileName).digest('hex').slice(0, 8); - return path.join(this.locksDir, `${safeName}-${profileHash}.lock`); - } - - private isProcessAlive(pid: number): boolean { - if (!Number.isInteger(pid) || pid <= 0) { - return false; - } - - try { - process.kill(pid, 0); - return true; - } catch (error) { - if ((error as NodeJS.ErrnoException).code === 'EPERM') { - return true; - } - return false; - } - } - - private tryRemoveDeadOwnerLock(lockPath: string): boolean { - try { - const lockContent = fs.readFileSync(lockPath, 'utf8').trim(); - const lockPid = Number.parseInt(lockContent, 10); - if (!this.isProcessAlive(lockPid)) { - fs.unlinkSync(lockPath); - return true; - } - } catch { - // Best-effort stale lock cleanup. - } - return false; - } - - private async withContextSyncLock( - profileName: string, - callback: () => Promise - ): Promise { - const lockPath = this.getContextSyncLockPath(profileName); - const retryDelayMs = 50; - const staleLockMs = 30000; - const timeoutMs = staleLockMs + 5000; - const start = Date.now(); - - fs.mkdirSync(this.locksDir, { recursive: true, mode: 0o700 }); - - while (true) { - try { - const fd = fs.openSync(lockPath, 'wx', 0o600); - fs.writeFileSync(fd, `${process.pid}`); - fs.closeSync(fd); - break; - } catch (error) { - const err = error as NodeJS.ErrnoException; - if (err.code !== 'EEXIST') { - throw error; - } - - try { - const lockStats = fs.statSync(lockPath); - if (Date.now() - lockStats.mtimeMs > staleLockMs) { - fs.unlinkSync(lockPath); - continue; - } - - if (this.tryRemoveDeadOwnerLock(lockPath)) { - continue; - } - } catch { - // Best-effort stale lock cleanup. - } - - if (Date.now() - start > timeoutMs) { - throw new Error(`Timed out waiting for profile context lock: ${profileName}`); - } - - await new Promise((resolve) => setTimeout(resolve, retryDelayMs)); - } - } - - try { - return await callback(); - } finally { - try { - fs.unlinkSync(lockPath); - } catch { - // Best-effort cleanup. - } - } - } } export { InstanceManager }; diff --git a/src/management/profile-context-sync-lock.ts b/src/management/profile-context-sync-lock.ts new file mode 100644 index 00000000..c8158bc3 --- /dev/null +++ b/src/management/profile-context-sync-lock.ts @@ -0,0 +1,177 @@ +import * as fs from 'fs'; +import * as path from 'path'; +import { createHash } from 'crypto'; + +interface ContextSyncLockPayload { + version: 1; + pid: number; + nonce: string; + acquiredAtMs: number; +} + +interface ContextSyncLockSnapshot { + raw: string; + owner: { pid: number; nonce?: string } | null; +} + +class ProfileContextSyncLock { + private readonly locksDir: string; + + constructor(instancesDir: string) { + this.locksDir = path.join(instancesDir, '.locks'); + } + + private sanitizeName(name: string): string { + return name.replace(/[^a-zA-Z0-9_-]/g, '-').toLowerCase(); + } + + private getLockPath(profileName: string): string { + const safeName = this.sanitizeName(profileName); + const profileHash = createHash('sha1').update(profileName).digest('hex').slice(0, 8); + return path.join(this.locksDir, `${safeName}-${profileHash}.lock`); + } + + private isProcessAlive(pid: number): boolean { + if (!Number.isInteger(pid) || pid <= 0) { + return false; + } + + try { + process.kill(pid, 0); + return true; + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'EPERM') { + return true; + } + return false; + } + } + + private parseContextSyncLock(raw: string): { pid: number; nonce?: string } | null { + const trimmed = raw.trim(); + if (trimmed.length === 0) { + return null; + } + + try { + const parsed = JSON.parse(trimmed) as Partial; + if (typeof parsed.pid === 'number' && Number.isInteger(parsed.pid) && parsed.pid > 0) { + const nonce = + typeof parsed.nonce === 'string' && parsed.nonce.length > 0 ? parsed.nonce : undefined; + return { pid: parsed.pid, nonce }; + } + } catch { + const legacyPid = Number.parseInt(trimmed, 10); + if (Number.isInteger(legacyPid) && legacyPid > 0) { + return { pid: legacyPid }; + } + } + + return null; + } + + private readContextSyncLockSnapshot(lockPath: string): ContextSyncLockSnapshot | null { + try { + const raw = fs.readFileSync(lockPath, 'utf8'); + return { + raw, + owner: this.parseContextSyncLock(raw), + }; + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') { + return null; + } + return null; + } + } + + private tryRemoveLockIfUnchanged(lockPath: string, expectedRaw: string): boolean { + try { + const currentRaw = fs.readFileSync(lockPath, 'utf8'); + if (currentRaw !== expectedRaw) { + return false; + } + fs.unlinkSync(lockPath); + return true; + } catch { + return false; + } + } + + private tryRemoveDeadOwnerLock(lockPath: string, snapshot: ContextSyncLockSnapshot): boolean { + if (!snapshot.owner || this.isProcessAlive(snapshot.owner.pid)) { + return false; + } + + return this.tryRemoveLockIfUnchanged(lockPath, snapshot.raw); + } + + async withLock(profileName: string, callback: () => Promise): Promise { + const lockPath = this.getLockPath(profileName); + const retryDelayMs = 50; + const staleLockMs = 30000; + const timeoutMs = staleLockMs + 5000; + const start = Date.now(); + const ownerPayload: ContextSyncLockPayload = { + version: 1, + pid: process.pid, + nonce: createHash('sha1') + .update(`${process.pid}:${Date.now()}:${Math.random()}`) + .digest('hex') + .slice(0, 16), + acquiredAtMs: Date.now(), + }; + const ownerPayloadRaw = JSON.stringify(ownerPayload); + + fs.mkdirSync(this.locksDir, { recursive: true, mode: 0o700 }); + + while (true) { + try { + const fd = fs.openSync(lockPath, 'wx', 0o600); + fs.writeFileSync(fd, ownerPayloadRaw, 'utf8'); + fs.closeSync(fd); + break; + } catch (error) { + const err = error as NodeJS.ErrnoException; + if (err.code !== 'EEXIST') { + throw error; + } + + const lockSnapshot = this.readContextSyncLockSnapshot(lockPath); + if (lockSnapshot) { + if (this.tryRemoveDeadOwnerLock(lockPath, lockSnapshot)) { + continue; + } + + // For malformed lock payloads, fall back to age-based stale cleanup. + if (!lockSnapshot.owner) { + try { + const lockStats = fs.statSync(lockPath); + if (Date.now() - lockStats.mtimeMs > staleLockMs) { + if (this.tryRemoveLockIfUnchanged(lockPath, lockSnapshot.raw)) { + continue; + } + } + } catch { + // Best-effort stale lock cleanup. + } + } + } + + if (Date.now() - start > timeoutMs) { + throw new Error(`Timed out waiting for profile context lock: ${profileName}`); + } + + await new Promise((resolve) => setTimeout(resolve, retryDelayMs)); + } + } + + try { + return await callback(); + } finally { + this.tryRemoveLockIfUnchanged(lockPath, ownerPayloadRaw); + } + } +} + +export default ProfileContextSyncLock; diff --git a/src/management/shared-manager.ts b/src/management/shared-manager.ts index e2ae5ad8..4fc0e394 100644 --- a/src/management/shared-manager.ts +++ b/src/management/shared-manager.ts @@ -63,8 +63,14 @@ class SharedManager { const resolvedTarget = path.resolve(path.dirname(target), targetLink); // Check if target points back to our shared dir or link path - const sharedDir = path.join(getCcsDir(), 'shared'); - if (resolvedTarget.startsWith(sharedDir) || resolvedTarget === linkPath) { + const sharedDir = this.resolveCanonicalPath(path.join(getCcsDir(), 'shared')); + const canonicalResolvedTarget = this.resolveCanonicalPath(resolvedTarget); + const canonicalLinkPath = this.resolveCanonicalPath(linkPath); + + if ( + this.isPathWithinDirectory(canonicalResolvedTarget, sharedDir) || + canonicalResolvedTarget === canonicalLinkPath + ) { console.log(warn(`Circular symlink detected: ${target} → ${resolvedTarget}`)); return true; } @@ -692,17 +698,17 @@ class SharedManager { * Guard project merge operations to known CCS-managed roots only. */ private isSafeProjectsMergeSource(sourcePath: string, instanceName: string): boolean { - const resolvedSource = path.resolve(sourcePath); - const sharedContextRoot = path.resolve(path.join(this.sharedDir, 'context-groups')); - const instanceProjectsRoot = path.resolve( + const resolvedSource = this.resolveCanonicalPath(sourcePath); + const sharedContextRoot = this.resolveCanonicalPath( + path.join(this.sharedDir, 'context-groups') + ); + const instanceProjectsRoot = this.resolveCanonicalPath( path.join(this.instancesDir, instanceName, 'projects') ); - const isWithin = (child: string, root: string): boolean => - child === root || child.startsWith(`${root}${path.sep}`); - return ( - isWithin(resolvedSource, sharedContextRoot) || isWithin(resolvedSource, instanceProjectsRoot) + this.isPathWithinDirectory(resolvedSource, sharedContextRoot) || + this.isPathWithinDirectory(resolvedSource, instanceProjectsRoot) ); } @@ -736,7 +742,7 @@ class SharedManager { projectsPath: string, instanceName: string ): Promise { - const sharedMemoryRoot = path.resolve(path.join(this.sharedDir, 'memory')); + const sharedMemoryRoot = this.resolveCanonicalPath(path.join(this.sharedDir, 'memory')); let projectEntries: fs.Dirent[] = []; try { @@ -763,15 +769,20 @@ class SharedManager { continue; } - if (!path.resolve(memoryTarget).startsWith(sharedMemoryRoot)) { + const canonicalMemoryTarget = this.resolveCanonicalPath(memoryTarget); + if (!this.isPathWithinDirectory(canonicalMemoryTarget, sharedMemoryRoot)) { continue; } await fs.promises.unlink(memoryPath); await this.ensureDirectory(memoryPath); - if (await this.pathExists(memoryTarget)) { - await this.mergeDirectoryWithConflictCopies(memoryTarget, memoryPath, instanceName); + if (await this.pathExists(canonicalMemoryTarget)) { + await this.mergeDirectoryWithConflictCopies( + canonicalMemoryTarget, + memoryPath, + instanceName + ); } } } @@ -879,6 +890,27 @@ class SharedManager { return candidate; } + private resolveCanonicalPath(targetPath: string): string { + try { + return fs.realpathSync.native(targetPath); + } catch { + return path.resolve(targetPath); + } + } + + private isPathWithinDirectory(candidatePath: string, rootPath: string): boolean { + const normalizeForCompare = (inputPath: string): string => { + const resolved = path.resolve(inputPath); + return process.platform === 'win32' ? resolved.toLowerCase() : resolved; + }; + + const normalizedCandidate = normalizeForCompare(candidatePath); + const normalizedRoot = normalizeForCompare(rootPath); + const relative = path.relative(normalizedRoot, normalizedCandidate); + + return relative === '' || (!relative.startsWith('..') && !path.isAbsolute(relative)); + } + private async pathExists(targetPath: string): Promise { try { await fs.promises.access(targetPath); diff --git a/src/web-server/routes/account-routes.ts b/src/web-server/routes/account-routes.ts index 90ace229..b6ebc359 100644 --- a/src/web-server/routes/account-routes.ts +++ b/src/web-server/routes/account-routes.ts @@ -206,25 +206,24 @@ router.delete('/:name', (req: Request, res: Response): void => { return; } - // Delete from appropriate config (unified and/or legacy) - let deleted = false; - if (isUnifiedMode() && registry.hasAccountUnified(name)) { - registry.removeAccountUnified(name); - deleted = true; - } - if (registry.hasProfile(name)) { - registry.deleteProfile(name); - deleted = true; - } + const existsUnified = isUnifiedMode() && registry.hasAccountUnified(name); + const existsLegacy = registry.hasProfile(name); - if (!deleted) { + if (!existsUnified && !existsLegacy) { res.status(404).json({ error: `Account not found: ${name}` }); return; } - // Keep API delete behavior aligned with CLI remove command. + // Match CLI remove ordering: delete instance first, metadata second. instanceMgr.deleteInstance(name); + if (existsUnified) { + registry.removeAccountUnified(name); + } + if (existsLegacy) { + registry.deleteProfile(name); + } + res.json({ success: true, deleted: name }); } catch (error) { res.status(500).json({ error: (error as Error).message }); diff --git a/src/web-server/routes/config-routes.ts b/src/web-server/routes/config-routes.ts index 537f7348..31a25cc7 100644 --- a/src/web-server/routes/config-routes.ts +++ b/src/web-server/routes/config-routes.ts @@ -22,7 +22,7 @@ import { isValidContextGroupName, normalizeContextGroupName } from '../../auth/a const router = Router(); -function validateAccountContextMetadata(config: unknown): string | null { +function validateAndNormalizeAccountContextMetadata(config: unknown): string | null { if (typeof config !== 'object' || config === null) { return 'Invalid config payload'; } @@ -63,6 +63,15 @@ function validateAccountContextMetadata(config: unknown): string | null { if (!isValidContextGroupName(normalizedGroup)) { return `Invalid config.accounts.${accountName}.context_group`; } + account.context_group = normalizedGroup; + } + + if (mode === 'shared' && typeof group === 'string' && group.trim().length === 0) { + return `Invalid config.accounts.${accountName}.context_group: shared mode requires a non-empty value`; + } + + if (mode === 'isolated' && group !== undefined) { + delete account.context_group; } } @@ -130,7 +139,7 @@ router.put('/', (req: Request, res: Response): void => { return; } - const accountContextError = validateAccountContextMetadata(config); + const accountContextError = validateAndNormalizeAccountContextMetadata(config); if (accountContextError) { res.status(400).json({ error: accountContextError }); return; @@ -150,6 +159,15 @@ router.put('/', (req: Request, res: Response): void => { router.post('/migrate', async (req: Request, res: Response): Promise => { try { const dryRun = req.query.dryRun === 'true'; + if (!needsMigration()) { + res.json({ + success: true, + migratedFiles: [], + warnings: [], + alreadyMigrated: true, + }); + return; + } const result = await migrate(dryRun); res.json(result); } catch (error) { diff --git a/tests/unit/account-context.test.ts b/tests/unit/account-context.test.ts index 7e282a58..2c8f658f 100644 --- a/tests/unit/account-context.test.ts +++ b/tests/unit/account-context.test.ts @@ -40,4 +40,15 @@ describe('account context helpers', () => { expect(resolved.mode).toBe('shared'); expect(resolved.group).toBe('sprint-a'); }); + + it('normalizes whitespace in explicit shared context group names', () => { + const result = resolveCreateAccountContext({ + shareContext: false, + contextGroup: ' Team Alpha ', + }); + + expect(result.error).toBeUndefined(); + expect(result.policy.mode).toBe('shared'); + expect(result.policy.group).toBe('team-alpha'); + }); }); diff --git a/tests/unit/auth/profile-registry-context-normalization.test.ts b/tests/unit/auth/profile-registry-context-normalization.test.ts new file mode 100644 index 00000000..95882b35 --- /dev/null +++ b/tests/unit/auth/profile-registry-context-normalization.test.ts @@ -0,0 +1,92 @@ +import { afterEach, beforeEach, describe, expect, it } from 'bun:test'; +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; +import ProfileRegistry from '../../../src/auth/profile-registry'; + +describe('profile-registry context normalization', () => { + let tempHome = ''; + let originalCcsHome: string | undefined; + let originalUnifiedMode: string | undefined; + + beforeEach(() => { + tempHome = fs.mkdtempSync(path.join(os.tmpdir(), 'ccs-profile-registry-context-')); + originalCcsHome = process.env.CCS_HOME; + originalUnifiedMode = process.env.CCS_UNIFIED_CONFIG; + process.env.CCS_HOME = tempHome; + }); + + afterEach(() => { + if (originalCcsHome !== undefined) process.env.CCS_HOME = originalCcsHome; + else delete process.env.CCS_HOME; + + if (originalUnifiedMode !== undefined) process.env.CCS_UNIFIED_CONFIG = originalUnifiedMode; + else delete process.env.CCS_UNIFIED_CONFIG; + + if (tempHome && fs.existsSync(tempHome)) { + fs.rmSync(tempHome, { recursive: true, force: true }); + } + }); + + it('drops non-string legacy context_group values without throwing', () => { + const ccsDir = path.join(tempHome, '.ccs'); + fs.mkdirSync(ccsDir, { recursive: true }); + fs.writeFileSync( + path.join(ccsDir, 'profiles.json'), + JSON.stringify( + { + version: '2.0.0', + default: null, + profiles: { + work: { + type: 'account', + created: '2026-02-24T00:00:00.000Z', + last_used: null, + context_mode: 'shared', + context_group: { invalid: true }, + }, + }, + }, + null, + 2 + ), + 'utf8' + ); + + const registry = new ProfileRegistry(); + const profile = registry.getProfile('work'); + + expect(profile.context_mode).toBe('shared'); + expect(profile.context_group).toBeUndefined(); + }); + + it('drops non-string unified context_group values without throwing', () => { + process.env.CCS_UNIFIED_CONFIG = '1'; + const ccsDir = path.join(tempHome, '.ccs'); + fs.mkdirSync(ccsDir, { recursive: true }); + fs.writeFileSync( + path.join(ccsDir, 'config.yaml'), + [ + 'version: 8', + 'accounts:', + ' work:', + ' created: "2026-02-24T00:00:00.000Z"', + ' last_used: null', + ' context_mode: shared', + ' context_group: 123', + 'profiles: {}', + 'cliproxy:', + ' oauth_accounts: {}', + ' providers: {}', + ' variants: {}', + ].join('\n'), + 'utf8' + ); + + const registry = new ProfileRegistry(); + const accounts = registry.getAllAccountsUnified(); + + expect(accounts.work.context_mode).toBe('shared'); + expect(accounts.work.context_group).toBeUndefined(); + }); +}); diff --git a/tests/unit/shared-context-policy.test.ts b/tests/unit/shared-context-policy.test.ts index dffbd37f..b882b5d5 100644 --- a/tests/unit/shared-context-policy.test.ts +++ b/tests/unit/shared-context-policy.test.ts @@ -13,6 +13,12 @@ function getTestCcsDir(): string { return path.join(path.resolve(process.env.CCS_HOME), '.ccs'); } +function createDirectorySymlink(targetPath: string, linkPath: string): void { + const symlinkType: 'dir' | 'junction' = process.platform === 'win32' ? 'junction' : 'dir'; + const linkTarget = process.platform === 'win32' ? path.resolve(targetPath) : targetPath; + fs.symlinkSync(linkTarget, linkPath, symlinkType); +} + describe('SharedManager context policy', () => { let tempRoot = ''; let originalHome: string | undefined; @@ -134,4 +140,42 @@ describe('SharedManager context policy', () => { expect(stats.isDirectory() || stats.isSymbolicLink()).toBe(true); }); + + it('skips merge when projects symlink target is outside canonical CCS roots', async () => { + const ccsDir = getTestCcsDir(); + const instancePath = path.join(ccsDir, 'instances', 'work'); + const projectsPath = path.join(instancePath, 'projects'); + const unsafeProjectsPath = path.join(ccsDir, 'shared', 'context-groups-evil', 'projects'); + const unsafeFile = path.join(unsafeProjectsPath, '-tmp-project', 'notes.md'); + + fs.mkdirSync(path.dirname(unsafeFile), { recursive: true }); + fs.writeFileSync(unsafeFile, 'unsafe source', 'utf8'); + fs.mkdirSync(instancePath, { recursive: true }); + createDirectorySymlink(unsafeProjectsPath, projectsPath); + + const manager = new SharedManager(); + await manager.syncProjectContext(instancePath, { mode: 'isolated' }); + + expect(fs.lstatSync(projectsPath).isDirectory()).toBe(true); + expect(fs.existsSync(path.join(projectsPath, '-tmp-project', 'notes.md'))).toBe(false); + }); + + it('does not detach project memory symlink from lookalike shared path prefixes', async () => { + const ccsDir = getTestCcsDir(); + const instancePath = path.join(ccsDir, 'instances', 'work'); + const projectPath = path.join(instancePath, 'projects', '-tmp-project'); + const memoryPath = path.join(projectPath, 'memory'); + const unsafeMemoryTarget = path.join(ccsDir, 'shared', 'memory-evil', '-tmp-project'); + const unsafeMemoryFile = path.join(unsafeMemoryTarget, 'MEMORY.md'); + + fs.mkdirSync(path.dirname(unsafeMemoryFile), { recursive: true }); + fs.writeFileSync(unsafeMemoryFile, 'unsafe memory', 'utf8'); + fs.mkdirSync(projectPath, { recursive: true }); + createDirectorySymlink(unsafeMemoryTarget, memoryPath); + + const manager = new SharedManager(); + await manager.syncProjectContext(instancePath, { mode: 'isolated' }); + + expect(fs.lstatSync(memoryPath).isSymbolicLink()).toBe(true); + }); }); diff --git a/tests/unit/web-server/account-routes-context.test.ts b/tests/unit/web-server/account-routes-context.test.ts index 0563c129..c3b27999 100644 --- a/tests/unit/web-server/account-routes-context.test.ts +++ b/tests/unit/web-server/account-routes-context.test.ts @@ -5,6 +5,8 @@ import * as os from 'os'; import * as path from 'path'; import type { Server } from 'http'; import accountRoutes from '../../../src/web-server/routes/account-routes'; +import ProfileRegistry from '../../../src/auth/profile-registry'; +import { InstanceManager } from '../../../src/management/instance-manager'; async function getJson(baseUrl: string, routePath: string): Promise { const response = await fetch(`${baseUrl}${routePath}`); @@ -12,6 +14,10 @@ async function getJson(baseUrl: string, routePath: string): Promise { return (await response.json()) as T; } +async function deletePath(baseUrl: string, routePath: string): Promise { + return fetch(`${baseUrl}${routePath}`, { method: 'DELETE' }); +} + describe('web-server account-routes context normalization', () => { let server: Server; let baseUrl = ''; @@ -130,4 +136,41 @@ describe('web-server account-routes context normalization', () => { expect(work?.context_mode).toBe('shared'); expect(work?.context_group).toBe('default'); }); + + it('does not delete metadata when instance deletion fails', async () => { + const ccsDir = path.join(tempHome, '.ccs'); + fs.mkdirSync(ccsDir, { recursive: true }); + fs.writeFileSync( + path.join(ccsDir, 'config.yaml'), + [ + 'version: 8', + 'accounts:', + ' work:', + ' created: "2026-02-01T00:00:00.000Z"', + ' last_used: null', + ' context_mode: shared', + ' context_group: sprint-a', + 'profiles: {}', + 'cliproxy:', + ' oauth_accounts: {}', + ' providers: {}', + ' variants: {}', + ].join('\n'), + 'utf8' + ); + const registry = new ProfileRegistry(); + + const originalDeleteInstance = InstanceManager.prototype.deleteInstance; + InstanceManager.prototype.deleteInstance = () => { + throw new Error('simulated instance delete failure'); + }; + + try { + const response = await deletePath(baseUrl, '/api/accounts/work'); + expect(response.status).toBe(500); + expect(registry.hasAccountUnified('work')).toBe(true); + } finally { + InstanceManager.prototype.deleteInstance = originalDeleteInstance; + } + }); }); diff --git a/tests/unit/web-server/config-routes-account-context.test.ts b/tests/unit/web-server/config-routes-account-context.test.ts index 850f654f..a1bcb728 100644 --- a/tests/unit/web-server/config-routes-account-context.test.ts +++ b/tests/unit/web-server/config-routes-account-context.test.ts @@ -6,6 +6,7 @@ import * as path from 'path'; import type { Server } from 'http'; import configRoutes from '../../../src/web-server/routes/config-routes'; import { createEmptyUnifiedConfig } from '../../../src/config/unified-config-types'; +import { loadUnifiedConfig } from '../../../src/config/unified-config-loader'; async function putJson(baseUrl: string, routePath: string, body: unknown): Promise { return fetch(`${baseUrl}${routePath}`, { @@ -17,6 +18,16 @@ async function putJson(baseUrl: string, routePath: string, body: unknown): Promi }); } +async function postJson(baseUrl: string, routePath: string, body?: unknown): Promise { + return fetch(`${baseUrl}${routePath}`, { + method: 'POST', + headers: { + 'Content-Type': 'application/json', + }, + body: body === undefined ? undefined : JSON.stringify(body), + }); +} + describe('web-server config-routes account context validation', () => { let server: Server; let baseUrl = ''; @@ -124,6 +135,26 @@ describe('web-server config-routes account context validation', () => { expect(payload.error).toContain('context_group'); }); + it('rejects whitespace-only shared context_group values', async () => { + const response = await putJson(baseUrl, '/api/config', { + version: 8, + accounts: { + work: { + created: '2026-01-01T00:00:00.000Z', + last_used: null, + context_mode: 'shared', + context_group: ' ', + }, + }, + profiles: {}, + cliproxy: { oauth_accounts: {}, providers: [], variants: {} }, + }); + + expect(response.status).toBe(400); + const payload = (await response.json()) as { error: string }; + expect(payload.error).toContain('requires a non-empty value'); + }); + it('accepts valid shared context metadata', async () => { const config = createEmptyUnifiedConfig(); config.accounts.work = { @@ -137,5 +168,24 @@ describe('web-server config-routes account context validation', () => { expect(response.status).toBe(200); const payload = (await response.json()) as { success: boolean }; expect(payload.success).toBe(true); + + const savedConfig = loadUnifiedConfig(); + expect(savedConfig?.accounts.work.context_group).toBe('sprint-a'); + }); + + it('returns alreadyMigrated when migration is not needed', async () => { + const response = await postJson(baseUrl, '/api/config/migrate'); + + expect(response.status).toBe(200); + const payload = (await response.json()) as { + success: boolean; + migratedFiles: string[]; + warnings: string[]; + alreadyMigrated?: boolean; + }; + expect(payload.success).toBe(true); + expect(payload.migratedFiles).toEqual([]); + expect(payload.warnings).toEqual([]); + expect(payload.alreadyMigrated).toBe(true); }); });