fix(sandbox): address PR #250 follow-up review — XSS hardening, eviction reconcile, doc sync (#243)

Security (must-fix):
- sandbox.controller: the anonymous GET /api/sb/:id response now sets
  X-Content-Type-Options: nosniff, a restrictive CSP, and Content-Disposition=
  attachment for any mime outside a raster-image allowlist (png/jpeg/gif/webp/
  avif). entry.mime is attacker-controlled, so an evil.svg/evil.html could
  otherwise execute script inline on the Docmost origin (stored XSS). Mirrors
  the public attachment route's hardening.

Stability:
- client.stashPage: reconcile mirrors AFTER the final document put, not only
  before it. The doc blob is the newest entry and FIFO eviction drops the
  oldest = this stash's own images, so the stored doc could reference an
  evicted blob (consumer 404) and over-report images.mirrored. A bounded loop
  now reverts doc-put-evicted mirrors, drops the stale doc blob, and re-puts
  until stable. Regenerated packages/mcp/build/.
- sandbox.controller: emit Cache-Control on the 304 branch too (ttlSeconds is
  computed before the conditional check).

Docs:
- Bump the MCP tool count 39 -> 40 across all READMEs and AGENTS.md (the
  registry now exposes exactly 40 tools).

Refactor:
- SandboxStore.asSink() centralizes the {put,has,evict} sink + uri<->id
  mapping; the embedded-MCP and in-app agent-tools wiring sites share it.

Tests:
- security headers (inline vs attachment, nosniff, CSP), 304 Cache-Control,
  putAndLink URL form, has()/remove(), asSink() round-trip, getSandboxPublicUrl
  (trailing-slash trim + APP_URL fallback), and a stash test where the doc put
  itself evicts a mirrored image.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

apps/server/src/core/ai-chat/tools/ai-chat-tools.service.ts

@@ -92,20 +92,14 @@ export class AiChatToolsService {
     // Bind the stash tool to the shared in-RAM SandboxStore. The store owns the
     // anonymous-URL composition (putAndLink) and the live/evict probes the MCP
     // package needs to keep its mirror counts honest under FIFO eviction (the
-    // package never touches env or the store). The sink speaks `uri`s, so the
-    // probes map a uri back to its id (the last path segment).
-    const idOf = (uri: string) => uri.substring(uri.lastIndexOf('/') + 1);
-
+    // package never touches env or the store). asSink() centralizes the uri↔id
+    // mapping next to putAndLink, shared with the embedded-MCP wiring site.
     const { DocmostClient, sharedToolSpecs } = await loadDocmostMcp();
     const client: DocmostClientLike = new DocmostClient({
       apiUrl,
       getToken,
       getCollabToken,
-      sandbox: {
-        put: (buf, mime) => this.sandboxStore.putAndLink(buf, mime),
-        has: (uri) => this.sandboxStore.has(idOf(uri)),
-        evict: (uri) => this.sandboxStore.remove(idOf(uri)),
-      },
+      sandbox: this.sandboxStore.asSink(),
     });
 
     // Build an ai-SDK tool from a shared, zod-agnostic spec. The spec owns the

apps/server/src/integrations/environment/environment.service.spec.ts

@@ -40,4 +40,29 @@ describe('EnvironmentService', () => {
       expect(build(undefined).getSandboxTtlMs()).toBe(3_600_000);
     });
   });
+
+  describe('getSandboxPublicUrl', () => {
+    // Stub that resolves BOTH keys the public-url logic consults.
+    const build = (vals: { sandboxUrl?: string; appUrl?: string }) =>
+      new EnvironmentService({
+        get: (key: string, def?: string) =>
+          key === 'SANDBOX_PUBLIC_URL'
+            ? (vals.sandboxUrl ?? def)
+            : key === 'APP_URL'
+              ? (vals.appUrl ?? def)
+              : def,
+      } as any);
+
+    it('uses SANDBOX_PUBLIC_URL and trims a trailing slash', () => {
+      expect(
+        build({ sandboxUrl: 'https://docs.example.com/' }).getSandboxPublicUrl(),
+      ).toBe('https://docs.example.com');
+    });
+
+    it('falls back to APP_URL (origin) when SANDBOX_PUBLIC_URL is unset', () => {
+      expect(
+        build({ appUrl: 'https://app.example.com' }).getSandboxPublicUrl(),
+      ).toBe('https://app.example.com');
+    });
+  });
 });

apps/server/src/integrations/mcp/mcp.service.ts

@@ -119,15 +119,10 @@ export class McpService implements OnModuleDestroy {
   // Bind the stash tool to the shared in-RAM SandboxStore. The store owns the
   // anonymous-URL composition (putAndLink) and the live/evict probes the MCP
   // package needs to keep its mirror counts honest under FIFO eviction; the
-  // package owns neither env nor the store. The sink speaks `uri`s, so the
-  // probes map a uri back to its id (the last path segment).
+  // package owns neither env nor the store. The uri↔id mapping now lives on the
+  // store (asSink), shared with the in-app agent-tools wiring site.
   private buildSandboxConfig(): DocmostMcpConfig['sandbox'] {
-    const idOf = (uri: string) => uri.substring(uri.lastIndexOf('/') + 1);
-    return {
-      put: (buf, mime) => this.sandboxStore.putAndLink(buf, mime),
-      has: (uri) => this.sandboxStore.has(idOf(uri)),
-      evict: (uri) => this.sandboxStore.remove(idOf(uri)),
-    };
+    return this.sandboxStore.asSink();
   }
 
   // Service account the embedded MCP uses to talk back to this Docmost

apps/server/src/integrations/sandbox/sandbox.controller.spec.ts

@@ -187,4 +187,76 @@ describe('SandboxController', () => {
     expect(maxAge).toBeGreaterThanOrEqual(0);
     expect(maxAge).toBeLessThanOrEqual(60);
   });
+
+  it('emits Cache-Control alongside ETag on the 304 branch', async () => {
+    const sha = '3'.repeat(64);
+    const store = {
+      get: jest.fn().mockReturnValue(entry(Buffer.from('x'), 'application/json', sha)),
+    };
+    const controller = new SandboxController(store as any);
+    const res = makeRes();
+
+    await controller.get(VALID_ID, makeReq({ 'if-none-match': `"${sha}"` }), res);
+
+    expect(res._sent.status).toBe(304);
+    expect(res._sent.headers['cache-control']).toMatch(
+      /^private, max-age=\d+, immutable$/,
+    );
+  });
+
+  it('sets nosniff + restrictive CSP and serves an allowlisted image inline', async () => {
+    const sha = '4'.repeat(64);
+    const store = {
+      get: jest.fn().mockReturnValue(entry(Buffer.from('x'), 'image/png', sha)),
+    };
+    const controller = new SandboxController(store as any);
+    const res = makeRes();
+
+    await controller.get(VALID_ID, makeReq(), res);
+
+    expect(res._sent.status).toBe(200);
+    expect(res._sent.headers['x-content-type-options']).toBe('nosniff');
+    expect(res._sent.headers['content-security-policy']).toBe(
+      "base-uri 'none'; object-src 'self'; default-src 'self';",
+    );
+    expect(res._sent.headers['content-disposition']).toBe('inline');
+  });
+
+  it('forces an SVG to download (attachment) while keeping nosniff + CSP', async () => {
+    const sha = '5'.repeat(64);
+    const store = {
+      get: jest.fn().mockReturnValue(entry(Buffer.from('<svg/>'), 'image/svg+xml', sha)),
+    };
+    const controller = new SandboxController(store as any);
+    const res = makeRes();
+
+    await controller.get(VALID_ID, makeReq(), res);
+
+    expect(res._sent.status).toBe(200);
+    expect(res._sent.headers['content-disposition']).toBe('attachment');
+    expect(res._sent.headers['x-content-type-options']).toBe('nosniff');
+    expect(res._sent.headers['content-security-policy']).toBe(
+      "base-uri 'none'; object-src 'self'; default-src 'self';",
+    );
+  });
+
+  it('forces text/html to download (attachment) while keeping nosniff + CSP', async () => {
+    const sha = '6'.repeat(64);
+    const store = {
+      get: jest
+        .fn()
+        .mockReturnValue(entry(Buffer.from('<h1>x</h1>'), 'text/html', sha)),
+    };
+    const controller = new SandboxController(store as any);
+    const res = makeRes();
+
+    await controller.get(VALID_ID, makeReq(), res);
+
+    expect(res._sent.status).toBe(200);
+    expect(res._sent.headers['content-disposition']).toBe('attachment');
+    expect(res._sent.headers['x-content-type-options']).toBe('nosniff');
+    expect(res._sent.headers['content-security-policy']).toBe(
+      "base-uri 'none'; object-src 'self'; default-src 'self';",
+    );
+  });
 });

apps/server/src/integrations/sandbox/sandbox.controller.ts

@@ -9,6 +9,18 @@ import { SANDBOX_ROUTE_SEGMENT } from './sandbox.constants';
 const UUID_RE =
   /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/;
 
+// MIME types safe to render inline in a browser. SVG is deliberately EXCLUDED
+// (it can carry script), as are text/html and the JSON document blob — anything
+// not on this list is served as an attachment so an attacker-controlled mime can
+// never execute script on this origin (the route is anonymous + same-origin).
+const INLINE_SAFE_MIME = new Set([
+  'image/png',
+  'image/jpeg',
+  'image/gif',
+  'image/webp',
+  'image/avif',
+]);
+
 /**
  * Anonymous read endpoint for the in-RAM blob sandbox.
  *
@@ -22,6 +34,12 @@ const UUID_RE =
  * It only ever serves blobs looked up from the SandboxStore by a validated
  * UUID; `:id` is never used as a filesystem path, so there is no traversal
  * surface. Never returns tokens, never 401s.
+ *
+ * Anti-XSS hardening mirrors the public attachment route: every response sets
+ * `X-Content-Type-Options: nosniff` and a restrictive CSP, and serves any mime
+ * NOT on the inline-safe allowlist (svg/html/the JSON document blob) as an
+ * attachment, so an attacker-controlled `entry.mime` can never execute script
+ * on this same-origin anonymous route.
  */
 @Controller(SANDBOX_ROUTE_SEGMENT)
 export class SandboxController {
@@ -52,17 +70,32 @@ export class SandboxController {
     // body — the original bug this whole channel exists to fix.
     const etag = `"${entry.sha256}"`;
 
-    // Conditional request: an exact ETag match → 304 with no body. The blob is
-    // immutable, so the validator is stable for the blob's whole lifetime.
-    if (this.ifNoneMatchMatches(req.headers['if-none-match'], entry.sha256)) {
-      res.status(304).header('ETag', etag).send();
-      return;
-    }
-
+    // Compute freshness BEFORE the conditional check: a 304 conditional
+    // revalidation must not lose the Cache-Control freshness directives, or a
+    // revalidating client would forget how long the blob stays fresh.
     const ttlSeconds = Math.max(
       0,
       Math.floor((entry.expiresAt - Date.now()) / 1000),
     );
+    // Capability URL — keep it out of shared caches; immutable for its TTL.
+    const cacheControl = `private, max-age=${ttlSeconds}, immutable`;
+
+    // Conditional request: an exact ETag match → 304 with no body. The blob is
+    // immutable, so the validator is stable for the blob's whole lifetime.
+    if (this.ifNoneMatchMatches(req.headers['if-none-match'], entry.sha256)) {
+      res
+        .status(304)
+        .header('ETag', etag)
+        .header('Cache-Control', cacheControl)
+        .send();
+      return;
+    }
+
+    // Non-allowlisted mimes (svg/html/the JSON blob) are forced to download so
+    // an attacker-controlled mime can never run script inline on this origin.
+    const disposition = INLINE_SAFE_MIME.has(entry.mime)
+      ? 'inline'
+      : 'attachment';
 
     // Use @Res() + res.send(Buffer) with an explicit Content-Type so the binary
     // body bypasses the global JSON response transform/serializer.
@@ -72,8 +105,11 @@ export class SandboxController {
         'Content-Type': entry.mime,
         'Content-Length': entry.buf.length,
         ETag: etag,
-        // Capability URL — keep it out of shared caches; immutable for its TTL.
-        'Cache-Control': `private, max-age=${ttlSeconds}, immutable`,
+        'Cache-Control': cacheControl,
+        'X-Content-Type-Options': 'nosniff',
+        'Content-Security-Policy':
+          "base-uri 'none'; object-src 'self'; default-src 'self';",
+        'Content-Disposition': disposition,
       })
       .send(entry.buf);
   }

apps/server/src/integrations/sandbox/sandbox.store.spec.ts

@@ -130,4 +130,36 @@ describe('SandboxStore', () => {
       /total store cap/,
     );
   });
+
+  it('putAndLink composes the anonymous /api/sb/<id> url with matching integrity', () => {
+    store = new SandboxStore(makeEnv());
+    const buf = Buffer.from('hello link', 'utf8');
+    const expected = createHash('sha256').update(buf).digest('hex');
+
+    const res = store.putAndLink(buf, 'image/png');
+    expect(res.uri).toMatch(/^https:\/\/example\.test\/api\/sb\/[0-9a-f-]{36}$/);
+    expect(res.sha256).toBe(expected);
+    expect(res.size).toBe(buf.length);
+  });
+
+  it('has()/remove() report and free a blob by id', () => {
+    store = new SandboxStore(makeEnv());
+    const { id } = store.put(Buffer.from('x'), 'text/plain');
+
+    expect(store.has(id)).toBe(true);
+    store.remove(id);
+    expect(store.has(id)).toBe(false);
+    expect(store.bytes).toBe(0);
+  });
+
+  it('asSink() round-trips put/has/evict through the anonymous uri', () => {
+    store = new SandboxStore(makeEnv());
+    const sink = store.asSink();
+    const buf = Buffer.from('sink bytes', 'utf8');
+
+    const r = sink.put(buf, 'image/png');
+    expect(sink.has(r.uri)).toBe(true);
+    sink.evict(r.uri);
+    expect(sink.has(r.uri)).toBe(false);
+  });
 });

apps/server/src/integrations/sandbox/sandbox.store.ts

@@ -108,6 +108,26 @@ export class SandboxStore implements OnModuleDestroy {
     };
   }
 
+  /**
+   * Adapter to the package's blob-sandbox sink contract `{ put, has, evict }`.
+   * The sink speaks anonymous `uri`s while the store is keyed by `id`, so this is
+   * the ONE place that maps a sandbox uri back to its id (the last path segment).
+   * Both wiring sites (embedded MCP + in-app agent tools) use this so the uri↔id
+   * mapping and URL composition live next to putAndLink, not copy-pasted.
+   */
+  asSink(): {
+    put: (buf: Buffer, mime: string) => { uri: string; sha256: string; size: number };
+    has: (uri: string) => boolean;
+    evict: (uri: string) => void;
+  } {
+    const idOf = (uri: string) => uri.substring(uri.lastIndexOf('/') + 1);
+    return {
+      put: (buf, mime) => this.putAndLink(buf, mime),
+      has: (uri) => this.has(idOf(uri)),
+      evict: (uri) => this.remove(idOf(uri)),
+    };
+  }
+
   /** True if the blob is still live (not evicted/expired). */
   has(id: string): boolean {
     return this.get(id) !== undefined;