Merge branch 'fix/credential-failure-ux' of https://github.com/qwibitai/nanoclaw into fix/credential-failure-ux

src/attachment-safety.ts

@@ -0,0 +1,23 @@
+import path from 'path';
+
+/**
+ * Is `name` safe to use as the last segment of a path inside an
+ * attachment-staging directory? Filenames originate from untrusted sources —
+ * channel messages from any chat participant, agent-to-agent forwards from
+ * a possibly-compromised peer agent — and land in `path.join(dir, name)`
+ * sinks on the host. Without this guard, a `..`-laden name escapes the
+ * inbox and writes anywhere the host process has filesystem permission.
+ *
+ * Rejects:
+ *   - non-string / empty
+ *   - `.` / `..` (traversal sentinels that path.basename returns as-is)
+ *   - anything containing a path separator (`/` or `\`) or NUL
+ *   - any value where `path.basename(name) !== name`, catching OS-specific
+ *     separators and covering drives/prefixes on Windows runtimes
+ */
+export function isSafeAttachmentName(name: string): boolean {
+  if (typeof name !== 'string' || name.length === 0) return false;
+  if (name === '.' || name === '..') return false;
+  if (/[\\/\0]/.test(name)) return false;
+  return path.basename(name) === name;
+}

src/circuit-breaker.test.ts

@@ -0,0 +1,197 @@
+/**
+ * Unit tests for the startup circuit breaker.
+ *
+ * Covers state transitions, the documented backoff schedule, and the
+ * fresh-install case where DATA_DIR doesn't exist yet (the breaker runs
+ * before initDb, so it has to create the dir itself).
+ */
+import fs from 'fs';
+import os from 'os';
+import path from 'path';
+import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest';
+
+// vi.mock factories are hoisted above imports, so they can't close over local
+// consts. vi.hoisted is hoisted alongside the mock and runs before any
+// `import` — so it can only use globals (no path/os modules). Use require()
+// inside the callback to compute the test dir.
+const { TEST_DIR } = vi.hoisted(() => {
+  const nodePath = require('path') as typeof import('path');
+  const nodeOs = require('os') as typeof import('os');
+  return { TEST_DIR: nodePath.join(nodeOs.tmpdir(), 'nanoclaw-cb-test') };
+});
+const CB_PATH = path.join(TEST_DIR, 'circuit-breaker.json');
+
+vi.mock('./config.js', async () => {
+  const actual = await vi.importActual<typeof import('./config.js')>('./config.js');
+  return { ...actual, DATA_DIR: TEST_DIR };
+});
+
+vi.mock('./log.js', () => ({
+  log: {
+    debug: vi.fn(),
+    info: vi.fn(),
+    warn: vi.fn(),
+    error: vi.fn(),
+    fatal: vi.fn(),
+  },
+}));
+
+import { enforceStartupBackoff, resetCircuitBreaker } from './circuit-breaker.js';
+
+function readState(): { attempt: number; timestamp: string } {
+  return JSON.parse(fs.readFileSync(CB_PATH, 'utf-8'));
+}
+
+function seedState(attempt: number, timestamp = new Date().toISOString()): void {
+  fs.writeFileSync(CB_PATH, JSON.stringify({ attempt, timestamp }));
+}
+
+beforeEach(() => {
+  if (fs.existsSync(TEST_DIR)) fs.rmSync(TEST_DIR, { recursive: true });
+  fs.mkdirSync(TEST_DIR, { recursive: true });
+});
+
+afterEach(() => {
+  vi.useRealTimers();
+  if (fs.existsSync(TEST_DIR)) fs.rmSync(TEST_DIR, { recursive: true });
+});
+
+describe('resetCircuitBreaker', () => {
+  it('deletes the state file', () => {
+    seedState(3);
+    expect(fs.existsSync(CB_PATH)).toBe(true);
+    resetCircuitBreaker();
+    expect(fs.existsSync(CB_PATH)).toBe(false);
+  });
+
+  it('is a no-op when the file does not exist', () => {
+    expect(fs.existsSync(CB_PATH)).toBe(false);
+    expect(() => resetCircuitBreaker()).not.toThrow();
+  });
+});
+
+describe('enforceStartupBackoff — state transitions', () => {
+  it('first run writes attempt=1 and does not delay', async () => {
+    vi.useFakeTimers();
+    const start = Date.now();
+    await enforceStartupBackoff();
+    // No timers should have been queued — clean first start is 0s.
+    expect(Date.now() - start).toBe(0);
+    expect(readState().attempt).toBe(1);
+  });
+
+  it('within reset window, attempt is incremented', async () => {
+    seedState(1);
+    vi.useFakeTimers();
+    const promise = enforceStartupBackoff();
+    await vi.runAllTimersAsync();
+    await promise;
+    expect(readState().attempt).toBe(2);
+  });
+
+  it('outside reset window (>1h), attempt resets to 1', async () => {
+    const longAgo = new Date(Date.now() - 2 * 60 * 60 * 1000).toISOString();
+    seedState(5, longAgo);
+    await enforceStartupBackoff();
+    expect(readState().attempt).toBe(1);
+  });
+
+  it('exactly at the reset window boundary still counts as "within"', async () => {
+    // RESET_WINDOW_MS = 60min. Use 59min59s to stay inside even if the test
+    // takes a few ms to execute.
+    const justInside = new Date(Date.now() - (60 * 60 * 1000 - 1000)).toISOString();
+    seedState(2, justInside);
+    vi.useFakeTimers();
+    const promise = enforceStartupBackoff();
+    await vi.runAllTimersAsync();
+    await promise;
+    expect(readState().attempt).toBe(3);
+  });
+
+  it('treats a malformed state file as no prior state', async () => {
+    fs.writeFileSync(CB_PATH, '{ this is not json');
+    await enforceStartupBackoff();
+    expect(readState().attempt).toBe(1);
+  });
+
+  it('resetCircuitBreaker after a startup actually clears the counter for the next startup', async () => {
+    // Simulate: crash, restart (attempt=2), graceful shutdown, restart again.
+    seedState(1);
+    vi.useFakeTimers();
+    const p1 = enforceStartupBackoff();
+    await vi.runAllTimersAsync();
+    await p1;
+    expect(readState().attempt).toBe(2);
+
+    resetCircuitBreaker();
+    expect(fs.existsSync(CB_PATH)).toBe(false);
+
+    await enforceStartupBackoff();
+    expect(readState().attempt).toBe(1);
+  });
+});
+
+describe('enforceStartupBackoff — backoff schedule', () => {
+  /**
+   * Documented schedule:
+   *
+   *   clean start → 1 crash → 2 crash → 3 crash → 4 crash → 5 crash → 6+ crash
+   *      0s    →    0s    →   10s   →   30s   →   2min  →   5min  →   15min cap
+   *
+   * Each row is [priorAttempt seeded in the file, expected delay this run
+   * produces in seconds]. priorAttempt=null = no file = very first start.
+   *
+   * To assert the *requested* delay (not just observed elapsed real time),
+   * we spy on global.setTimeout and look at the longest call. runAllTimersAsync
+   * lets the function complete so we can move on.
+   */
+  const cases: Array<{ label: string; priorAttempt: number | null; expectedDelaySec: number }> = [
+    { label: 'clean first start (no file)', priorAttempt: null, expectedDelaySec: 0 },
+    { label: 'first crash (attempt=2)', priorAttempt: 1, expectedDelaySec: 0 },
+    { label: 'second crash (attempt=3)', priorAttempt: 2, expectedDelaySec: 10 },
+    { label: 'third crash (attempt=4)', priorAttempt: 3, expectedDelaySec: 30 },
+    { label: 'fourth crash (attempt=5)', priorAttempt: 4, expectedDelaySec: 120 },
+    { label: 'fifth crash (attempt=6)', priorAttempt: 5, expectedDelaySec: 300 },
+    { label: 'sixth crash (attempt=7) — cap', priorAttempt: 6, expectedDelaySec: 900 },
+    { label: 'far past cap (attempt=20)', priorAttempt: 19, expectedDelaySec: 900 },
+  ];
+
+  for (const { label, priorAttempt, expectedDelaySec } of cases) {
+    it(`${label}: delays ${expectedDelaySec}s`, async () => {
+      if (priorAttempt !== null) seedState(priorAttempt);
+
+      vi.useFakeTimers();
+      const setTimeoutSpy = vi.spyOn(global, 'setTimeout');
+
+      const promise = enforceStartupBackoff();
+      await vi.runAllTimersAsync();
+      await promise;
+
+      // enforceStartupBackoff only calls setTimeout when delaySec > 0. Pick
+      // the longest delay it requested (vitest may queue small internal
+      // timers we don't care about).
+      const requestedDelays = setTimeoutSpy.mock.calls.map((c) => c[1] ?? 0);
+      const maxDelayMs = requestedDelays.length ? Math.max(...requestedDelays) : 0;
+
+      expect(maxDelayMs).toBe(expectedDelaySec * 1000);
+    });
+  }
+});
+
+describe('enforceStartupBackoff — fresh install (DATA_DIR missing)', () => {
+  /**
+   * The breaker runs before initDb (which is what creates DATA_DIR). On a
+   * fresh checkout the dir doesn't exist yet, so write() must create it
+   * before writing the state file — otherwise the host crashes on its very
+   * first start.
+   */
+  it('creates DATA_DIR on demand and does not throw', async () => {
+    fs.rmSync(TEST_DIR, { recursive: true });
+    expect(fs.existsSync(TEST_DIR)).toBe(false);
+
+    await expect(enforceStartupBackoff()).resolves.toBeUndefined();
+    expect(fs.existsSync(TEST_DIR)).toBe(true);
+    expect(fs.existsSync(CB_PATH)).toBe(true);
+    expect(readState().attempt).toBe(1);
+  });
+});

src/circuit-breaker.ts

@@ -0,0 +1,84 @@
+import fs from 'fs';
+import path from 'path';
+
+import { DATA_DIR } from './config.js';
+import { log } from './log.js';
+
+const CB_PATH = path.join(DATA_DIR, 'circuit-breaker.json');
+const RESET_WINDOW_MS = 60 * 60 * 1000; // 1 hour
+// Index = number of consecutive crashes (0 = clean start, attempt 1).
+// 6+ crashes capped at 15min.
+const BACKOFF_SCHEDULE_S = [0, 0, 10, 30, 120, 300, 900];
+
+interface CircuitBreakerState {
+  attempt: number;
+  timestamp: string;
+}
+
+function read(): CircuitBreakerState | null {
+  try {
+    const raw = fs.readFileSync(CB_PATH, 'utf-8');
+    return JSON.parse(raw) as CircuitBreakerState;
+  } catch {
+    return null;
+  }
+}
+
+function write(state: CircuitBreakerState): void {
+  // The breaker runs before initDb (which is what creates DATA_DIR), so on a
+  // fresh checkout the dir may not exist yet.
+  fs.mkdirSync(DATA_DIR, { recursive: true });
+  fs.writeFileSync(CB_PATH, JSON.stringify(state, null, 2) + '\n');
+}
+
+function getDelay(attempt: number): number {
+  const idx = Math.min(attempt - 1, BACKOFF_SCHEDULE_S.length - 1);
+  return BACKOFF_SCHEDULE_S[idx];
+}
+
+export function resetCircuitBreaker(): void {
+  try {
+    fs.unlinkSync(CB_PATH);
+    log.info('Circuit breaker reset on clean shutdown');
+  } catch {}
+}
+
+export async function enforceStartupBackoff(): Promise<void> {
+  const now = new Date();
+  const prev = read();
+
+  let attempt: number;
+  if (!prev) {
+    attempt = 1;
+  } else {
+    const elapsedMs = now.getTime() - new Date(prev.timestamp).getTime();
+    if (elapsedMs < RESET_WINDOW_MS) {
+      attempt = prev.attempt + 1;
+      log.warn('Previous startup was not a clean shutdown', {
+        previousAttempt: prev.attempt,
+        previousTimestamp: prev.timestamp,
+        elapsedSec: Math.round(elapsedMs / 1000),
+      });
+    } else {
+      attempt = 1;
+      log.info('Circuit breaker reset — last startup was over 1h ago', {
+        previousAttempt: prev.attempt,
+        previousTimestamp: prev.timestamp,
+      });
+    }
+  }
+
+  write({ attempt, timestamp: now.toISOString() });
+
+  const delaySec = getDelay(attempt);
+  if (delaySec > 0) {
+    const resumeAt = new Date(now.getTime() + delaySec * 1000).toISOString();
+    log.warn('Circuit breaker: delaying startup due to repeated crashes', {
+      attempt,
+      delaySec,
+      resumeAt,
+    });
+    await new Promise((resolve) => setTimeout(resolve, delaySec * 1000));
+    log.info('Circuit breaker: backoff complete, resuming startup', { attempt });
+  }
+}

src/host-core.test.ts

@@ -173,6 +173,43 @@ describe('session manager', () => {
 
     expect(getSession(session.id)!.last_active).not.toBeNull();
   });
+
+  it('should refuse path-traversal in attachment filenames', () => {
+    // Regression: attachment.name comes from untrusted senders (E2EE-protected
+    // chat platforms can't sanitize it server-side). Without the guard, a
+    // `../../../tmp/pwned` filename escapes the inbox dir and writes anywhere
+    // the host process can reach.
+    const { session } = resolveSession('ag-1', 'mg-1', null, 'shared');
+    const inboxBase = path.join(sessionDir('ag-1', session.id), 'inbox');
+    const escapeTarget = path.join('/tmp', 'nanoclaw-traversal-canary');
+    if (fs.existsSync(escapeTarget)) fs.rmSync(escapeTarget);
+
+    writeSessionMessage('ag-1', session.id, {
+      id: 'msg-attack',
+      kind: 'chat',
+      timestamp: now(),
+      content: JSON.stringify({
+        text: 'pwn',
+        attachments: [
+          {
+            type: 'document',
+            name: '../../../../../../../../tmp/nanoclaw-traversal-canary',
+            data: Buffer.from('owned').toString('base64'),
+          },
+        ],
+      }),
+    });
+
+    expect(fs.existsSync(escapeTarget)).toBe(false);
+    // The bytes should still land — under a synthesized safe name inside the
+    // inbox — so the agent doesn't lose data on a malicious filename.
+    const inboxDir = path.join(inboxBase, 'msg-attack');
+    expect(fs.existsSync(inboxDir)).toBe(true);
+    const written = fs.readdirSync(inboxDir);
+    expect(written).toHaveLength(1);
+    expect(written[0]).not.toContain('/');
+    expect(written[0]).not.toContain('..');
+  });
 });
 
 describe('router', () => {

src/index.ts

@@ -7,6 +7,7 @@
 import path from 'path';
 
 import { DATA_DIR } from './config.js';
+import { enforceStartupBackoff, resetCircuitBreaker } from './circuit-breaker.js';
 import { migrateGroupsToClaudeLocal } from './claude-md-compose.js';
 import { initDb } from './db/connection.js';
 import { runMigrations } from './db/migrations/index.js';
@@ -58,6 +59,9 @@ import { initChannelAdapters, teardownChannelAdapters, getChannelAdapter } from
 async function main(): Promise<void> {
   log.info('NanoClaw starting');
 
+  // 0. Circuit breaker — backoff on rapid restarts
+  await enforceStartupBackoff();
+
   // 1. Init central DB
   const dbPath = path.join(DATA_DIR, 'v2.db');
   const db = initDb(dbPath);
@@ -174,8 +178,15 @@ async function shutdown(signal: string): Promise<void> {
   }
   stopDeliveryPolls();
   stopHostSweep();
-  await teardownChannelAdapters();
-  process.exit(0);
+  try {
+    await teardownChannelAdapters();
+  } finally {
+    // Always reset on graceful shutdown — even if teardown threw, we got here
+    // via SIGTERM/SIGINT, not a crash, so the next start shouldn't be counted
+    // as one.
+    resetCircuitBreaker();
+    process.exit(0);
+  }
 }
 
 process.on('SIGTERM', () => shutdown('SIGTERM'));

src/modules/agent-to-agent/agent-route.ts

@@ -21,6 +21,7 @@
 import fs from 'fs';
 import path from 'path';
 
+import { isSafeAttachmentName } from '../../attachment-safety.js';
 import { getAgentGroup } from '../../db/agent-groups.js';
 import { getSession } from '../../db/sessions.js';
 import { wakeContainer } from '../../container-runner.js';
@@ -29,6 +30,8 @@ import { resolveSession, sessionDir, writeSessionMessage } from '../../session-m
 import type { Session } from '../../types.js';
 import { hasDestination } from './db/agent-destinations.js';
 
+export { isSafeAttachmentName };
+
 export interface ForwardedAttachment {
   name: string;
   filename: string;
@@ -36,26 +39,6 @@ export interface ForwardedAttachment {
   localPath: string;
 }
 
-/**
- * Is `name` safe to use as the last segment of a path inside the target
- * agent's inbox directory? Filenames arrive in messages_out content from
- * the source agent — under a multi-agent setup with heterogenous providers
- * (or a compromised / hallucinating sub-agent) they can't be trusted.
- *
- * Rejects:
- *   - empty string
- *   - `.` / `..` (traversal sentinels that path.basename returns as-is)
- *   - anything containing a path separator (`/` or `\`) or NUL
- *   - any value where `path.basename(name) !== name`, catching OS-specific
- *     separators and covering drives/prefixes on Windows runtimes
- */
-export function isSafeAttachmentName(name: string): boolean {
-  if (typeof name !== 'string' || name.length === 0) return false;
-  if (name === '.' || name === '..') return false;
-  if (/[\\/\0]/.test(name)) return false;
-  return path.basename(name) === name;
-}
-
 /**
  * Copy file attachments from the source agent's outbox into the target
  * agent's inbox. Returns attachments using the formatter's existing

src/router.ts

@@ -289,7 +289,14 @@ export async function routeInbound(event: InboundEvent): Promise<void> {
           log.warn('adapter.subscribe failed', { channelType: event.channelType, threadId: event.threadId, err });
         });
       }
-    } else if (agent.ignored_message_policy === 'accumulate') {
+    } else if (agent.ignored_message_policy === 'accumulate' && !(engages && (!accessOk || !scopeOk))) {
+      // Accumulate stores the message as silent context. We allow it when
+      // engagement simply didn't fire, but NOT when engagement fired and
+      // the access/scope gate refused — those refusals are security
+      // decisions about an untrusted sender, and silently storing their
+      // message (which also stages their attachments to disk via
+      // writeSessionMessage → extractAttachmentFiles) is exactly what the
+      // gate is meant to prevent.
       await deliverToAgent(agent, agentGroup, mg, event, userId, adapter?.supportsThreads === true, false);
       accumulatedCount++;
     } else {

src/session-manager.ts

@@ -14,6 +14,7 @@ import type Database from 'better-sqlite3';
 import fs from 'fs';
 import path from 'path';
 
+import { isSafeAttachmentName } from './attachment-safety.js';
 import type { OutboundFile } from './channels/adapter.js';
 import { DATA_DIR } from './config.js';
 import { getMessagingGroup } from './db/messaging-groups.js';
@@ -252,11 +253,26 @@ function extractAttachmentFiles(
   let changed = false;
   for (const att of attachments) {
     if (typeof att.data === 'string') {
+      // The name field is attacker-controlled: chat platforms with E2E
+      // attachment encryption (WhatsApp, Matrix) cannot sanitize filename
+      // server-side, and other adapters pass att.name through raw. Without
+      // this guard, `path.join(inboxDir, '../../...')` writes anywhere the
+      // host process has fs permission — see Signal Desktop's Nov 2025
+      // attachment-fileName advisory for the same archetype.
+      const rawName = (att.name as string | undefined) ?? `attachment-${Date.now()}`;
+      const filename = isSafeAttachmentName(rawName) ? rawName : `attachment-${Date.now()}`;
+      if (filename !== rawName) {
+        log.warn('Refused unsafe attachment filename — would escape inbox', {
+          messageId,
+          rawName,
+          replacement: filename,
+        });
+      }
       const inboxDir = path.join(sessionDir(agentGroupId, sessionId), 'inbox', messageId);
       fs.mkdirSync(inboxDir, { recursive: true });
-      const filename = (att.name as string) || `attachment-${Date.now()}`;
       const filePath = path.join(inboxDir, filename);
       fs.writeFileSync(filePath, Buffer.from(att.data as string, 'base64'));
+      att.name = filename;
       att.localPath = `inbox/${messageId}/${filename}`;
       delete att.data;
       changed = true;