From 61d89570cdaf780a9a5ea3f8e50e745670b1e7a1 Mon Sep 17 00:00:00 2001 From: EchoOfZion Date: Mon, 6 Apr 2026 18:31:33 +0900 Subject: [PATCH] security: comprehensive hardening of adapter layer and core engine MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses security findings from audit without introducing architectural changes. Focused on defense-in-depth for the existing adapter/engine stack. Security hardening: - Fail-closed engine: errors now deny (balanced) or ask (permissive) instead of silently allowing — ENGINE_ERROR logged at high severity - Prototype pollution guard: containsProtoKeys() rejects payloads with __proto__, constructor, or prototype keys before parsing - Audit log redaction: Bearer tokens, AWS keys, hex private keys, SSH keys, and generic secret patterns are stripped before writing to audit.jsonl - Audit log rotation: 10 MB max per file, 3 rotated backups, 0o600 perms - Config validation: loadConfig() rejects unknown levels, falls back to balanced instead of trusting arbitrary input - Path traversal fix: isSensitivePath() now uses resolve(normalize()) to prevent ../ bypass of sensitive path checks - File descriptor leak fix: inferInitiatingSkill() uses try/finally to guarantee closeSync() even on parse errors - Registry concurrency safety: RegistryStorage.save() serialized through write lock with atomic rename (write to .tmp then rename) Type safety improvements: - Adapters use ActionType, ActionContext, SkillIdentity types instead of inline Record and `as unknown as` casts - getString() extracted to common.ts (single source, no duplication) - Null/non-object input guards on parseInput() in both adapters Feature additions: - Read tool mapping: ClaudeCodeAdapter now maps Read → read_file, enabling ActionScanner path policy checks on file reads - Expanded sensitive paths: .env.staging, .env.development, .gnupg/, .docker/config.json, .git-credentials, .bitcoin/wallet.dat, keystore/, id_ecdsa, id_dsa, .yarnrc - Fail-closed capabilities: unknown action types denied by default in isActionAllowedByCapabilities() Tests: 154 total (was 134), all passing. Co-Authored-By: Claude --- src/adapters/claude-code.ts | 132 ++++++++++++++++++++------------ src/adapters/common.ts | 137 ++++++++++++++++++++++++++++++---- src/adapters/engine.ts | 35 ++++++--- src/adapters/index.ts | 2 + src/adapters/openclaw.ts | 103 ++++++++++++++----------- src/registry/storage.ts | 41 +++++++--- src/tests/adapter.test.ts | 94 ++++++++++++++++++++++- src/tests/integration.test.ts | 57 +++++++++++++- 8 files changed, 471 insertions(+), 130 deletions(-) diff --git a/src/adapters/claude-code.ts b/src/adapters/claude-code.ts index c5977af..4c80571 100644 --- a/src/adapters/claude-code.ts +++ b/src/adapters/claude-code.ts @@ -1,14 +1,17 @@ import { openSync, readSync, closeSync, fstatSync } from 'node:fs'; -import type { ActionEnvelope } from '../types/action.js'; +import type { ActionEnvelope, ActionType, ActionContext } from '../types/action.js'; +import type { SkillIdentity } from '../types/skill.js'; import type { HookAdapter, HookInput } from './types.js'; +import { getString } from './common.js'; /** - * Tool name → action type mapping for Claude Code + * Tool name -> action type mapping for Claude Code */ -const TOOL_ACTION_MAP: Record = { +const TOOL_ACTION_MAP: Record = { Bash: 'exec_command', Write: 'write_file', Edit: 'write_file', + Read: 'read_file', WebFetch: 'network_request', WebSearch: 'network_request', }; @@ -23,92 +26,120 @@ export class ClaudeCodeAdapter implements HookAdapter { readonly name = 'claude-code'; parseInput(raw: unknown): HookInput { - const data = raw as Record; - const hookEvent = (data.hook_event_name as string) || ''; + const data = (raw !== null && typeof raw === 'object') ? raw as Record : {}; + const hookEvent = getString(data, 'hook_event_name'); + const toolInput = (data.tool_input !== null && typeof data.tool_input === 'object') + ? data.tool_input as Record + : {}; return { - toolName: (data.tool_name as string) || '', - toolInput: (data.tool_input as Record) || {}, + toolName: getString(data, 'tool_name'), + toolInput, eventType: hookEvent.startsWith('Post') ? 'post' : 'pre', - sessionId: data.session_id as string | undefined, - cwd: data.cwd as string | undefined, + sessionId: getString(data, 'session_id') || undefined, + cwd: getString(data, 'cwd') || undefined, raw: data, }; } - mapToolToActionType(toolName: string): string | null { - return TOOL_ACTION_MAP[toolName] || null; + mapToolToActionType(toolName: string): ActionType | null { + return TOOL_ACTION_MAP[toolName] ?? null; } buildEnvelope(input: HookInput, initiatingSkill?: string | null): ActionEnvelope | null { const actionType = this.mapToolToActionType(input.toolName); if (!actionType) return null; - const actor = { - skill: { - id: initiatingSkill || 'claude-code-session', - source: initiatingSkill || 'claude-code', - version_ref: '0.0.0', - artifact_hash: '', - }, + const skill: SkillIdentity = { + id: initiatingSkill || 'claude-code-session', + source: initiatingSkill || 'claude-code', + version_ref: '0.0.0', + artifact_hash: '', }; - const context = { + const context: ActionContext = { session_id: input.sessionId || `hook-${Date.now()}`, user_present: true, - env: 'prod' as const, + env: 'prod', time: new Date().toISOString(), initiating_skill: initiatingSkill || undefined, }; - // Build action data based on type - let actionData: Record; - switch (actionType) { case 'exec_command': - actionData = { - command: (input.toolInput.command as string) || '', - args: [], - cwd: input.cwd, + return { + actor: { skill }, + action: { + type: actionType, + data: { + command: getString(input.toolInput as Record, 'command'), + args: [], + cwd: input.cwd, + }, + }, + context, }; - break; case 'write_file': - actionData = { - path: (input.toolInput.file_path as string) || '', + return { + actor: { skill }, + action: { + type: actionType, + data: { + path: getString(input.toolInput as Record, 'file_path'), + }, + }, + context, + }; + + case 'read_file': + return { + actor: { skill }, + action: { + type: actionType, + data: { + path: getString(input.toolInput as Record, 'file_path'), + }, + }, + context, }; - break; - case 'network_request': - actionData = { - method: 'GET', - url: (input.toolInput.url as string) || (input.toolInput.query as string) || '', + case 'network_request': { + const ti = input.toolInput as Record; + return { + actor: { skill }, + action: { + type: actionType, + data: { + method: 'GET' as const, + url: getString(ti, 'url') || getString(ti, 'query'), + }, + }, + context, }; - break; + } default: return null; } - - return { - actor, - action: { type: actionType, data: actionData }, - context, - } as unknown as ActionEnvelope; } async inferInitiatingSkill(input: HookInput): Promise { - const data = input.raw as Record; - const transcriptPath = data.transcript_path as string | undefined; + const data = (input.raw !== null && typeof input.raw === 'object') + ? input.raw as Record + : {}; + const transcriptPath = getString(data, 'transcript_path'); if (!transcriptPath) return null; + let fd: number | null = null; try { - const fd = openSync(transcriptPath, 'r'); + fd = openSync(transcriptPath, 'r'); const stat = fstatSync(fd); const TAIL_SIZE = 4096; const start = Math.max(0, stat.size - TAIL_SIZE); const buf = Buffer.alloc(Math.min(TAIL_SIZE, stat.size)); readSync(fd, buf, 0, buf.length, start); closeSync(fd); + fd = null; const tail = buf.toString('utf-8'); const lines = tail.split('\n').filter(Boolean); @@ -116,22 +147,27 @@ export class ClaudeCodeAdapter implements HookAdapter { for (let i = lines.length - 1; i >= 0; i--) { try { const entry = JSON.parse(lines[i]); - if (entry.type === 'tool_use' && entry.name === 'Skill' && entry.input?.skill) { + if (entry.type === 'tool_use' && entry.name === 'Skill' && typeof entry.input?.skill === 'string') { return entry.input.skill; } if (entry.role === 'assistant' && Array.isArray(entry.content)) { for (const block of entry.content) { - if (block.type === 'tool_use' && block.name === 'Skill' && block.input?.skill) { + if (block.type === 'tool_use' && block.name === 'Skill' && typeof block.input?.skill === 'string') { return block.input.skill; } } } } catch { - // Not valid JSON + // Not valid JSON line — skip } } } catch { // Can't read transcript + } finally { + // Ensure file descriptor is always closed + if (fd !== null) { + try { closeSync(fd); } catch { /* ignore */ } + } } return null; } diff --git a/src/adapters/common.ts b/src/adapters/common.ts index 9dda5fa..36285c1 100644 --- a/src/adapters/common.ts +++ b/src/adapters/common.ts @@ -1,5 +1,5 @@ -import { readFileSync, appendFileSync, mkdirSync, existsSync } from 'node:fs'; -import { join } from 'node:path'; +import { readFileSync, appendFileSync, mkdirSync, existsSync, statSync, renameSync, unlinkSync } from 'node:fs'; +import { join, resolve, normalize } from 'node:path'; import { homedir } from 'node:os'; import type { HookInput, HookOutput } from './types.js'; @@ -21,30 +21,90 @@ function ensureDir(): void { // Config // --------------------------------------------------------------------------- +const VALID_LEVELS = new Set(['strict', 'balanced', 'permissive']); + export function loadConfig(): { level: string } { try { - return JSON.parse(readFileSync(CONFIG_PATH, 'utf-8')); + const parsed = JSON.parse(readFileSync(CONFIG_PATH, 'utf-8')); + const level = typeof parsed?.level === 'string' && VALID_LEVELS.has(parsed.level) + ? parsed.level + : 'balanced'; + return { level }; } catch { return { level: 'balanced' }; } } +// --------------------------------------------------------------------------- +// Prototype pollution guard +// --------------------------------------------------------------------------- + +const DANGEROUS_KEYS = new Set(['__proto__', 'constructor', 'prototype']); + +/** + * Check whether an object (or any nested descendant) contains keys + * that could trigger prototype pollution when merged or spread. + */ +export function containsProtoKeys(obj: unknown): boolean { + if (obj === null || typeof obj !== 'object') return false; + + if (Array.isArray(obj)) { + return obj.some(containsProtoKeys); + } + + for (const key of Object.getOwnPropertyNames(obj)) { + if (DANGEROUS_KEYS.has(key)) return true; + if (containsProtoKeys((obj as Record)[key])) return true; + } + return false; +} + +// --------------------------------------------------------------------------- +// Safe string extraction +// --------------------------------------------------------------------------- + +/** + * Safely extract a string from unknown data. + */ +export function getString(obj: Record, key: string): string { + const val = obj[key]; + return typeof val === 'string' ? val : ''; +} + // --------------------------------------------------------------------------- // Sensitive path detection // --------------------------------------------------------------------------- const SENSITIVE_PATHS = [ - '.env', '.env.local', '.env.production', - '.ssh/', 'id_rsa', 'id_ed25519', + // Environment / dotenv + '.env', '.env.local', '.env.production', '.env.staging', '.env.development', + // SSH + '.ssh/', 'id_rsa', 'id_ed25519', 'id_ecdsa', 'id_dsa', + // AWS '.aws/credentials', '.aws/config', - '.npmrc', '.netrc', + // npm / package managers + '.npmrc', '.yarnrc', + // Network auth + '.netrc', + // GCP 'credentials.json', 'serviceAccountKey.json', + // Kubernetes '.kube/config', + // GPG + '.gnupg/', + // Docker + '.docker/config.json', + // Git credentials + '.git-credentials', + // Wallet / crypto + '.bitcoin/wallet.dat', + 'keystore/', ]; export function isSensitivePath(filePath: string): boolean { if (!filePath) return false; - const normalized = filePath.replace(/\\/g, '/'); + // Resolve to absolute and normalize to prevent traversal bypasses + const normalized = resolve(normalize(filePath)).replace(/\\/g, '/'); return SENSITIVE_PATHS.some( (p) => normalized.includes(`/${p}`) || normalized.endsWith(p) ); @@ -104,6 +164,51 @@ export function shouldAskAtLevel( // Audit logging // --------------------------------------------------------------------------- +const MAX_AUDIT_SIZE = 10 * 1024 * 1024; // 10 MB +const MAX_AUDIT_FILES = 3; + +function rotateAuditLogIfNeeded(): void { + try { + if (!existsSync(AUDIT_PATH)) return; + const stat = statSync(AUDIT_PATH); + if (stat.size < MAX_AUDIT_SIZE) return; + + // Rotate: .3 -> delete, .2 -> .3, .1 -> .2, current -> .1 + const oldest = `${AUDIT_PATH}.${MAX_AUDIT_FILES}`; + if (existsSync(oldest)) { + try { unlinkSync(oldest); } catch { /* ok */ } + } + for (let i = MAX_AUDIT_FILES - 1; i >= 1; i--) { + const from = `${AUDIT_PATH}.${i}`; + const to = `${AUDIT_PATH}.${i + 1}`; + if (existsSync(from)) { + try { renameSync(from, to); } catch { /* ok */ } + } + } + renameSync(AUDIT_PATH, `${AUDIT_PATH}.1`); + } catch { + // Non-critical -- rotation failure should not block logging + } +} + +/** + * Redact common secret patterns from a string before logging. + */ +function redactSecrets(value: string): string { + return value + // Bearer / Authorization tokens + .replace(/Bearer\s+[A-Za-z0-9\-_.]+/gi, 'Bearer [REDACTED]') + // AWS keys + .replace(/(AKIA|ABIA|ACCA|ASIA)[0-9A-Z]{16}/g, '$1[REDACTED]') + // Generic key=value secrets + .replace(/(api[_-]?key|api[_-]?secret|secret[_-]?key|password|token|passwd|pwd)\s*[:=]\s*['"]?[^\s'"]{8,}/gi, + (match) => match.slice(0, match.indexOf('=') + 1 || match.indexOf(':') + 1) + '[REDACTED]') + // Hex private keys (64 chars) + .replace(/0x[a-fA-F0-9]{64}/g, '0x[REDACTED_KEY]') + // SSH keys + .replace(/-----BEGIN\s+\w+\s+PRIVATE\s+KEY-----/g, '[REDACTED_SSH_KEY]'); +} + export function writeAuditLog( input: HookInput, decision: { decision?: string; risk_level?: string; risk_tags?: string[] } | null, @@ -111,6 +216,7 @@ export function writeAuditLog( ): void { try { ensureDir(); + rotateAuditLogIfNeeded(); const entry: Record = { timestamp: new Date().toISOString(), tool_name: input.toolName, @@ -122,7 +228,7 @@ export function writeAuditLog( if (initiatingSkill) { entry.initiating_skill = initiatingSkill; } - appendFileSync(AUDIT_PATH, JSON.stringify(entry) + '\n'); + appendFileSync(AUDIT_PATH, JSON.stringify(entry) + '\n', { mode: 0o600 }); } catch { // Non-critical } @@ -132,15 +238,15 @@ function summarizeToolInput(input: HookInput): string { const toolInput = input.toolInput; if (typeof toolInput === 'object' && toolInput !== null) { const cmd = (toolInput as Record).command; - if (typeof cmd === 'string') return cmd.slice(0, 200); + if (typeof cmd === 'string') return redactSecrets(cmd.slice(0, 200)); const fp = (toolInput as Record).file_path || - (toolInput as Record).path; - if (typeof fp === 'string') return fp; + (toolInput as Record).path; + if (typeof fp === 'string') return fp.slice(0, 200); const url = (toolInput as Record).url || - (toolInput as Record).query; - if (typeof url === 'string') return url; + (toolInput as Record).query; + if (typeof url === 'string') return redactSecrets(url.slice(0, 200)); } - return JSON.stringify(toolInput).slice(0, 200); + return redactSecrets(JSON.stringify(toolInput).slice(0, 200)); } // --------------------------------------------------------------------------- @@ -189,6 +295,7 @@ export function isActionAllowedByCapabilities( case 'web3_sign': return capabilities.can_web3 !== false; default: - return true; + // Fail-closed: unknown action types are denied by default + return false; } } diff --git a/src/adapters/engine.ts b/src/adapters/engine.ts index cd44412..2043a9b 100644 --- a/src/adapters/engine.ts +++ b/src/adapters/engine.ts @@ -6,6 +6,7 @@ import { writeAuditLog, getSkillTrustPolicy, isActionAllowedByCapabilities, + containsProtoKeys, } from './common.js'; /** @@ -19,9 +20,19 @@ export async function evaluateHook( rawInput: unknown, options: EngineOptions ): Promise { + // Guard: reject null / non-object payloads + if (rawInput === null || typeof rawInput !== 'object') { + return { decision: 'deny', reason: 'GoPlus AgentGuard: invalid input (not an object)' }; + } + + // Guard: prototype pollution in raw input + if (containsProtoKeys(rawInput)) { + return { decision: 'deny', reason: 'GoPlus AgentGuard: input rejected — contains dangerous keys (__proto__ / constructor / prototype)' }; + } + const input = adapter.parseInput(rawInput); - // Post-tool events → audit only + // Post-tool events -> audit only if (input.eventType === 'post') { const skill = await adapter.inferInitiatingSkill(input); writeAuditLog(input, null, skill); @@ -36,12 +47,12 @@ export async function evaluateHook( return { decision: 'allow' }; } - // Fast check: sensitive file paths (Write/Edit) + // Fast check: sensitive file paths (Write/Edit/Read) const actionType = adapter.mapToolToActionType(input.toolName); - if (actionType === 'write_file') { - const filePath = (input.toolInput.file_path as string) || - (input.toolInput.path as string) || ''; - if (isSensitivePath(filePath)) { + if (actionType === 'write_file' || actionType === 'read_file') { + const filePath = (typeof input.toolInput.file_path === 'string' ? input.toolInput.file_path : '') || + (typeof input.toolInput.path === 'string' ? input.toolInput.path : ''); + if (actionType === 'write_file' && isSensitivePath(filePath)) { const skillTag = initiatingSkill ? ` (via skill: ${initiatingSkill})` : ''; const reason = `GoPlus AgentGuard: blocked write to sensitive path "${filePath}"${skillTag}`; writeAuditLog(input, { decision: 'deny', risk_level: 'critical', risk_tags: ['SENSITIVE_PATH'] }, initiatingSkill); @@ -110,9 +121,13 @@ export async function evaluateHook( } return { decision: 'allow', initiatingSkill }; - } catch { - // Engine error → fail open - writeAuditLog(input, { decision: 'error', risk_level: 'low', risk_tags: ['ENGINE_ERROR'] }, initiatingSkill); - return { decision: 'allow' }; + } catch (err: unknown) { + // Engine error -> fail closed (security-critical: never allow on error) + const errMsg = err instanceof Error ? err.message : 'unknown error'; + writeAuditLog(input, { decision: 'error', risk_level: 'high', risk_tags: ['ENGINE_ERROR'] }, initiatingSkill); + if (options.config.level === 'permissive') { + return { decision: 'ask', reason: `GoPlus AgentGuard: internal error (${errMsg}) — please confirm action manually` }; + } + return { decision: 'deny', reason: `GoPlus AgentGuard: internal error — action blocked for safety` }; } } diff --git a/src/adapters/index.ts b/src/adapters/index.ts index 7380137..099e38a 100644 --- a/src/adapters/index.ts +++ b/src/adapters/index.ts @@ -16,4 +16,6 @@ export { writeAuditLog, getSkillTrustPolicy, isActionAllowedByCapabilities, + containsProtoKeys, + getString, } from './common.js'; diff --git a/src/adapters/openclaw.ts b/src/adapters/openclaw.ts index 1c84fd5..670c701 100644 --- a/src/adapters/openclaw.ts +++ b/src/adapters/openclaw.ts @@ -1,10 +1,12 @@ -import type { ActionEnvelope } from '../types/action.js'; +import type { ActionEnvelope, ActionType, ActionContext } from '../types/action.js'; +import type { SkillIdentity } from '../types/skill.js'; import type { HookAdapter, HookInput } from './types.js'; +import { getString } from './common.js'; /** - * Tool name → action type mapping for OpenClaw + * Tool name -> action type mapping for OpenClaw */ -const TOOL_ACTION_MAP: Record = { +const TOOL_ACTION_MAP: Record = { exec: 'exec_command', write: 'write_file', read: 'read_file', @@ -28,21 +30,24 @@ export class OpenClawAdapter implements HookAdapter { readonly name = 'openclaw'; parseInput(raw: unknown): HookInput { - const event = raw as Record; + const event = (raw !== null && typeof raw === 'object') ? raw as Record : {}; + const toolInput = (event.params !== null && typeof event.params === 'object') + ? event.params as Record + : {}; return { - toolName: (event.toolName as string) || '', - toolInput: (event.params as Record) || {}, + toolName: getString(event, 'toolName'), + toolInput, eventType: 'pre', // before_tool_call = pre raw: event, }; } - mapToolToActionType(toolName: string): string | null { + mapToolToActionType(toolName: string): ActionType | null { // Direct match if (TOOL_ACTION_MAP[toolName]) { return TOOL_ACTION_MAP[toolName]; } - // Prefix match for tool families (e.g. "exec_python" → "exec_command") + // Prefix match for tool families (e.g. "exec_python" -> "exec_command") for (const [prefix, actionType] of Object.entries(TOOL_ACTION_MAP)) { if (toolName.startsWith(prefix)) { return actionType; @@ -55,68 +60,82 @@ export class OpenClawAdapter implements HookAdapter { const actionType = this.mapToolToActionType(input.toolName); if (!actionType) return null; - const actor = { - skill: { - id: initiatingSkill || 'openclaw-session', - source: initiatingSkill || 'openclaw', - version_ref: '0.0.0', - artifact_hash: '', - }, + const skill: SkillIdentity = { + id: initiatingSkill || 'openclaw-session', + source: initiatingSkill || 'openclaw', + version_ref: '0.0.0', + artifact_hash: '', }; - const context = { + const context: ActionContext = { session_id: `openclaw-${Date.now()}`, user_present: true, - env: 'prod' as const, + env: 'prod', time: new Date().toISOString(), initiating_skill: initiatingSkill || undefined, }; - let actionData: Record; + const ti = input.toolInput as Record; switch (actionType) { case 'exec_command': - actionData = { - command: (input.toolInput.command as string) || '', - args: [], + return { + actor: { skill }, + action: { + type: actionType, + data: { + command: getString(ti, 'command'), + args: [], + }, + }, + context, }; - break; case 'write_file': - actionData = { - path: (input.toolInput.path as string) || - (input.toolInput.file_path as string) || '', + return { + actor: { skill }, + action: { + type: actionType, + data: { + path: getString(ti, 'path') || getString(ti, 'file_path'), + }, + }, + context, }; - break; case 'read_file': - actionData = { - path: (input.toolInput.path as string) || - (input.toolInput.file_path as string) || '', + return { + actor: { skill }, + action: { + type: actionType, + data: { + path: getString(ti, 'path') || getString(ti, 'file_path'), + }, + }, + context, }; - break; case 'network_request': - actionData = { - method: (input.toolInput.method as string) || 'GET', - url: (input.toolInput.url as string) || '', - body_preview: input.toolInput.body as string | undefined, + return { + actor: { skill }, + action: { + type: actionType, + data: { + method: (getString(ti, 'method') || 'GET') as 'GET' | 'POST' | 'PUT' | 'DELETE' | 'PATCH', + url: getString(ti, 'url'), + body_preview: getString(ti, 'body') || undefined, + }, + }, + context, }; - break; default: return null; } - - return { - actor, - action: { type: actionType, data: actionData }, - context, - } as unknown as ActionEnvelope; } async inferInitiatingSkill(input: HookInput): Promise { - // Try to get plugin ID from tool → plugin mapping + // Try to get plugin ID from tool -> plugin mapping try { const { getPluginIdFromTool } = await import('./openclaw-plugin.js'); return getPluginIdFromTool(input.toolName); diff --git a/src/registry/storage.ts b/src/registry/storage.ts index cd35f9d..f397302 100644 --- a/src/registry/storage.ts +++ b/src/registry/storage.ts @@ -26,6 +26,7 @@ export interface StorageOptions { export class RegistryStorage { private filePath: string; private data: RegistryData | null = null; + private writeLock: Promise = Promise.resolve(); constructor(options: StorageOptions = {}) { this.filePath = @@ -33,6 +34,21 @@ export class RegistryStorage { path.join(homedir(), '.agentguard', 'registry.json'); } + /** + * Serialize writes to prevent concurrent file corruption + */ + private async withWriteLock(fn: () => Promise): Promise { + const prev = this.writeLock; + let resolve!: () => void; + this.writeLock = new Promise((r) => { resolve = r; }); + try { + await prev; + return await fn(); + } finally { + resolve(); + } + } + /** * Ensure data directory exists */ @@ -74,19 +90,24 @@ export class RegistryStorage { * Save registry data to file */ async save(): Promise { - if (!this.data) { - throw new Error('No data to save'); - } + return this.withWriteLock(async () => { + if (!this.data) { + throw new Error('No data to save'); + } - await this.ensureDirectory(); + await this.ensureDirectory(); - this.data.updated_at = new Date().toISOString(); + this.data.updated_at = new Date().toISOString(); - await fs.writeFile( - this.filePath, - JSON.stringify(this.data, null, 2), - { encoding: 'utf-8', mode: 0o600 } - ); + // Atomic write: write to temp file then rename + const tmpPath = `${this.filePath}.tmp`; + await fs.writeFile( + tmpPath, + JSON.stringify(this.data, null, 2), + { encoding: 'utf-8', mode: 0o600 } + ); + await fs.rename(tmpPath, this.filePath); + }); } /** diff --git a/src/tests/adapter.test.ts b/src/tests/adapter.test.ts index f4d77cf..a599943 100644 --- a/src/tests/adapter.test.ts +++ b/src/tests/adapter.test.ts @@ -7,6 +7,8 @@ import { shouldDenyAtLevel, shouldAskAtLevel, isActionAllowedByCapabilities, + containsProtoKeys, + getString, } from '../adapters/common.js'; // ───────────────────────────────────────────────────────────────────────────── @@ -77,8 +79,11 @@ describe('ClaudeCodeAdapter', () => { assert.equal(adapter.mapToolToActionType('WebSearch'), 'network_request'); }); + it('should map Read to read_file', () => { + assert.equal(adapter.mapToolToActionType('Read'), 'read_file'); + }); + it('should return null for unknown tools', () => { - assert.equal(adapter.mapToolToActionType('Read'), null); assert.equal(adapter.mapToolToActionType('UnknownTool'), null); }); }); @@ -135,13 +140,25 @@ describe('ClaudeCodeAdapter', () => { assert.equal((envelope!.action.data as unknown as Record).url, 'test query'); }); - it('should return null for unmapped tools', () => { + it('should build read_file envelope from Read', () => { const input = adapter.parseInput({ hook_event_name: 'PreToolUse', tool_name: 'Read', tool_input: { file_path: '/tmp/test.txt' }, }); const envelope = adapter.buildEnvelope(input); + assert.ok(envelope); + assert.equal(envelope!.action.type, 'read_file'); + assert.equal((envelope!.action.data as unknown as Record).path, '/tmp/test.txt'); + }); + + it('should return null for unmapped tools', () => { + const input = adapter.parseInput({ + hook_event_name: 'PreToolUse', + tool_name: 'TodoWrite', + tool_input: {}, + }); + const envelope = adapter.buildEnvelope(input); assert.equal(envelope, null); }); @@ -353,6 +370,23 @@ describe('Adapter Common Utilities', () => { assert.ok(isSensitivePath('/home/user/.kube/config')); }); + it('should detect .docker/config.json', () => { + assert.ok(isSensitivePath('/home/user/.docker/config.json')); + }); + + it('should detect .git-credentials', () => { + assert.ok(isSensitivePath('/home/user/.git-credentials')); + }); + + it('should detect .gnupg directory', () => { + assert.ok(isSensitivePath('/home/user/.gnupg/secring.gpg')); + }); + + it('should detect .env.staging and .env.development', () => { + assert.ok(isSensitivePath('/project/.env.staging')); + assert.ok(isSensitivePath('/project/.env.development')); + }); + it('should allow normal paths', () => { assert.ok(!isSensitivePath('/project/src/index.ts')); assert.ok(!isSensitivePath('/project/package.json')); @@ -459,8 +493,60 @@ describe('Adapter Common Utilities', () => { assert.ok(!isActionAllowedByCapabilities('web3_sign', { can_web3: false })); }); - it('should allow unknown action types by default', () => { - assert.ok(isActionAllowedByCapabilities('unknown_action', {})); + it('should deny unknown action types (fail-closed)', () => { + assert.ok(!isActionAllowedByCapabilities('unknown_action', {})); + }); + }); + + describe('containsProtoKeys', () => { + it('should detect __proto__ key', () => { + // Object literals with __proto__ set the prototype, so use JSON.parse + const obj = JSON.parse('{"__proto__": {"admin": true}}'); + assert.ok(containsProtoKeys(obj)); + }); + + it('should detect constructor key', () => { + assert.ok(containsProtoKeys({ 'constructor': { prototype: {} } })); + }); + + it('should detect prototype key', () => { + assert.ok(containsProtoKeys({ 'prototype': {} })); + }); + + it('should detect nested dangerous keys', () => { + const obj = { a: { b: JSON.parse('{"__proto__": {}}') } }; + assert.ok(containsProtoKeys(obj)); + }); + + it('should allow safe objects', () => { + assert.ok(!containsProtoKeys({ foo: 'bar', baz: 123 })); + }); + + it('should handle null and primitives', () => { + assert.ok(!containsProtoKeys(null)); + assert.ok(!containsProtoKeys('string')); + assert.ok(!containsProtoKeys(42)); + }); + + it('should detect in arrays', () => { + const obj = [JSON.parse('{"__proto__": {}}')]; + assert.ok(containsProtoKeys(obj)); + }); + }); + + describe('getString', () => { + it('should extract string values', () => { + assert.equal(getString({ name: 'test' }, 'name'), 'test'); + }); + + it('should return empty string for non-string values', () => { + assert.equal(getString({ count: 42 }, 'count'), ''); + assert.equal(getString({ flag: true }, 'flag'), ''); + assert.equal(getString({ data: null }, 'data'), ''); + }); + + it('should return empty string for missing keys', () => { + assert.equal(getString({}, 'missing'), ''); }); }); }); diff --git a/src/tests/integration.test.ts b/src/tests/integration.test.ts index e75b630..4b44e53 100644 --- a/src/tests/integration.test.ts +++ b/src/tests/integration.test.ts @@ -76,15 +76,70 @@ describe('Integration: Claude Code evaluateHook', () => { assert.equal(result.decision, 'allow'); }); - it('should ALLOW unmapped tool (Read)', async () => { + it('should evaluate Read tool through ActionScanner (not auto-allow)', async () => { ctx = createTestContext('balanced'); const result = await evaluateHook(ctx.claudeAdapter, { hook_event_name: 'PreToolUse', tool_name: 'Read', tool_input: { file_path: '/tmp/test.txt' }, }, ctx.options); + // Read is now mapped to read_file and goes through ActionScanner + // ActionScanner may allow or deny based on path policy + assert.notEqual(result.decision, undefined, 'Should return a decision'); + assert.ok(['allow', 'deny', 'ask'].includes(result.decision)); + }); + + it('should ALLOW unmapped tool (TodoWrite)', async () => { + ctx = createTestContext('balanced'); + const result = await evaluateHook(ctx.claudeAdapter, { + hook_event_name: 'PreToolUse', + tool_name: 'TodoWrite', + tool_input: {}, + }, ctx.options); assert.equal(result.decision, 'allow'); }); + + it('should DENY input with __proto__ (prototype pollution guard)', async () => { + ctx = createTestContext('balanced'); + // Use JSON.parse to create an actual __proto__ own property + const malicious = JSON.parse('{"hook_event_name":"PreToolUse","tool_name":"Bash","tool_input":{"command":"echo hi"},"__proto__":{"admin":true}}'); + const result = await evaluateHook(ctx.claudeAdapter, malicious, ctx.options); + assert.equal(result.decision, 'deny'); + assert.ok(result.reason?.includes('dangerous keys')); + }); + + it('should DENY on engine error (fail-closed)', async () => { + ctx = createTestContext('balanced'); + // Replace actionScanner.decide to throw + const original = ctx.agentguard.actionScanner.decide; + ctx.agentguard.actionScanner.decide = async () => { throw new Error('test engine error'); }; + try { + const result = await evaluateHook(ctx.claudeAdapter, { + hook_event_name: 'PreToolUse', + tool_name: 'Bash', + tool_input: { command: 'echo hello' }, + }, ctx.options); + assert.equal(result.decision, 'deny'); + } finally { + ctx.agentguard.actionScanner.decide = original; + } + }); + + it('should ASK on engine error in permissive mode', async () => { + ctx = createTestContext('permissive'); + const original = ctx.agentguard.actionScanner.decide; + ctx.agentguard.actionScanner.decide = async () => { throw new Error('test engine error'); }; + try { + const result = await evaluateHook(ctx.claudeAdapter, { + hook_event_name: 'PreToolUse', + tool_name: 'Bash', + tool_input: { command: 'echo hello' }, + }, ctx.options); + assert.equal(result.decision, 'ask'); + } finally { + ctx.agentguard.actionScanner.decide = original; + } + }); }); // ─────────────────────────────────────────────────────────────────────────────