From 7e37b13aabd0d7ed8ebdedfa96cecad8e1e89796 Mon Sep 17 00:00:00 2001
From: gavrielc <gabicohen22@yahoo.com>
Date: Tue, 28 Apr 2026 13:26:44 +0300
Subject: [PATCH] Fix path traversal in attachment handling on channel-inbound
 path

---
 src/attachment-safety.ts                  | 23 ++++++++++++++
 src/host-core.test.ts                     | 37 +++++++++++++++++++++++
 src/modules/agent-to-agent/agent-route.ts | 23 ++------------
 src/router.ts                             |  9 +++++-
 src/session-manager.ts                    | 18 ++++++++++-
 5 files changed, 88 insertions(+), 22 deletions(-)
 create mode 100644 src/attachment-safety.ts

diff --git a/src/attachment-safety.ts b/src/attachment-safety.ts
new file mode 100644
index 0000000..85467f9
--- /dev/null
+++ b/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;
+}
diff --git a/src/host-core.test.ts b/src/host-core.test.ts
index 9906c4b..2bb72d4 100644
--- a/src/host-core.test.ts
+++ b/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', () => {
diff --git a/src/modules/agent-to-agent/agent-route.ts b/src/modules/agent-to-agent/agent-route.ts
index 812cb8e..613a1ed 100644
--- a/src/modules/agent-to-agent/agent-route.ts
+++ b/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
diff --git a/src/router.ts b/src/router.ts
index 3cf0192..995496d 100644
--- a/src/router.ts
+++ b/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 {
diff --git a/src/session-manager.ts b/src/session-manager.ts
index 38eaa0d..996a750 100644
--- a/src/session-manager.ts
+++ b/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;