diff --git a/.changeset/assistant-attachment-malware-scan.md b/.changeset/assistant-attachment-malware-scan.md new file mode 100644 index 0000000000..a9d6e44570 --- /dev/null +++ b/.changeset/assistant-attachment-malware-scan.md @@ -0,0 +1,6 @@ +--- +"@aragon/assistant": minor +"@aragon/assistant-contracts": minor +--- + +Scan chat attachments for malware before they are queued: `/files/confirm` now calls the file-malware-scanner worker (VirusTotal + Claude) after the format gate, deletes the blob of a flagged file and rejects it with `malicious_file`. Per-engine policy (`off` / `optional` / `mandatory`) and a kill switch live in the assistant config, with `ASSISTANT_MALWARE_SCAN_*` env overrides. diff --git a/.changeset/assistant-chat-block-rejected-send.md b/.changeset/assistant-chat-block-rejected-send.md new file mode 100644 index 0000000000..794fe2ef2b --- /dev/null +++ b/.changeset/assistant-chat-block-rejected-send.md @@ -0,0 +1,5 @@ +--- +"@aragon/assistant-chat": patch +--- + +Prevent sending a message with an attachment the service rejected: a file blocked by validation or the malware scan can no longer ride along with the message as if it had been delivered. diff --git a/.changeset/assistant-chat-malware-alerts.md b/.changeset/assistant-chat-malware-alerts.md new file mode 100644 index 0000000000..e911014557 --- /dev/null +++ b/.changeset/assistant-chat-malware-alerts.md @@ -0,0 +1,5 @@ +--- +"@aragon/assistant-chat": minor +--- + +Show a dedicated alert on an attachment the malware scan rejected ("Malicious code detected. The file was not attached.") and a retry hint when the scan is unavailable. diff --git a/.changeset/assistant-chat-send-idempotent.md b/.changeset/assistant-chat-send-idempotent.md new file mode 100644 index 0000000000..78b69c45fa --- /dev/null +++ b/.changeset/assistant-chat-send-idempotent.md @@ -0,0 +1,5 @@ +--- +"@aragon/assistant-chat": patch +--- + +Keep an attachment sendable after another attachment of the same message failed. The composer restores and re-sends every attachment when one throws, and dropping the entry on the first send made the surviving file look like one that never uploaded: the message could no longer be sent, and removing the file skipped its server-side deletion so it still reached the ticket. diff --git a/.changeset/assistant-scan-review-fixes.md b/.changeset/assistant-scan-review-fixes.md new file mode 100644 index 0000000000..1a3e45e766 --- /dev/null +++ b/.changeset/assistant-scan-review-fixes.md @@ -0,0 +1,5 @@ +--- +"@aragon/assistant": patch +--- + +Percent-encode the filename sent to the malware scanner: a name with any character above U+00FF (a Japanese or Cyrillic screenshot name, an emoji) made the scan request throw before any network call and permanently blocked the attachment behind a retriable "scan unavailable" error. A request the service fails to construct is now logged instead of being reported as a scanner outage. diff --git a/.changeset/assistant-strip-bidi-filenames.md b/.changeset/assistant-strip-bidi-filenames.md new file mode 100644 index 0000000000..e23f086b84 --- /dev/null +++ b/.changeset/assistant-strip-bidi-filenames.md @@ -0,0 +1,5 @@ +--- +"@aragon/assistant": patch +--- + +Strip invisible format characters from attachment filenames. A right-to-left override made `invoicegnp.exe` render as `invoiceexe.png` to whoever opened the support ticket; zero-width characters could likewise make two files carry the same visible name. diff --git a/.github/workflows/app-preview.yml b/.github/workflows/app-preview.yml index d5367ea3a7..975ae3aa04 100644 --- a/.github/workflows/app-preview.yml +++ b/.github/workflows/app-preview.yml @@ -55,7 +55,7 @@ jobs: vault-prefix: "kv_assistant" patch-root-directory: false # Serverless functions only see runtime env vars (the .env file is build-time only). - runtime-env-keys: "AI_GATEWAY_API_KEY LINEAR_API_KEY LINEAR_TEAM_ID UPSTASH_REDIS_REST_URL UPSTASH_REDIS_REST_TOKEN BLOB_READ_WRITE_TOKEN CRON_SECRET SENTRY_DSN" + runtime-env-keys: "AI_GATEWAY_API_KEY LINEAR_API_KEY LINEAR_TEAM_ID UPSTASH_REDIS_REST_URL UPSTASH_REDIS_REST_TOKEN BLOB_READ_WRITE_TOKEN CRON_SECRET SENTRY_DSN MALWARE_SCANNER_ACCESS_CLIENT_ID MALWARE_SCANNER_ACCESS_CLIENT_SECRET" upload-sentry-source-maps: true deploy: diff --git a/.github/workflows/assistant-development.yml b/.github/workflows/assistant-development.yml index ec2e4077a5..1bf91969d3 100644 --- a/.github/workflows/assistant-development.yml +++ b/.github/workflows/assistant-development.yml @@ -50,5 +50,5 @@ jobs: # dev.assistant is an aliased preview deployment, so VERCEL_ENV reports 'preview' at # runtime — ASSISTANT_ENV pins the actual environment for src/lib/env.ts. runtime-env: "ASSISTANT_ENV=development" - runtime-env-keys: "AI_GATEWAY_API_KEY LINEAR_API_KEY LINEAR_TEAM_ID UPSTASH_REDIS_REST_URL UPSTASH_REDIS_REST_TOKEN BLOB_READ_WRITE_TOKEN CRON_SECRET SENTRY_DSN" + runtime-env-keys: "AI_GATEWAY_API_KEY LINEAR_API_KEY LINEAR_TEAM_ID UPSTASH_REDIS_REST_URL UPSTASH_REDIS_REST_TOKEN BLOB_READ_WRITE_TOKEN CRON_SECRET SENTRY_DSN MALWARE_SCANNER_ACCESS_CLIENT_ID MALWARE_SCANNER_ACCESS_CLIENT_SECRET" upload-sentry-source-maps: true diff --git a/.github/workflows/assistant-production.yml b/.github/workflows/assistant-production.yml index 566f9b46e9..fd572e128c 100644 --- a/.github/workflows/assistant-production.yml +++ b/.github/workflows/assistant-production.yml @@ -47,5 +47,5 @@ jobs: vault-prefix: "kv_assistant" patch-root-directory: false # VERCEL_ENV=production maps correctly at runtime; only the secrets need lifting. - runtime-env-keys: "AI_GATEWAY_API_KEY LINEAR_API_KEY LINEAR_TEAM_ID UPSTASH_REDIS_REST_URL UPSTASH_REDIS_REST_TOKEN BLOB_READ_WRITE_TOKEN CRON_SECRET SENTRY_DSN" + runtime-env-keys: "AI_GATEWAY_API_KEY LINEAR_API_KEY LINEAR_TEAM_ID UPSTASH_REDIS_REST_URL UPSTASH_REDIS_REST_TOKEN BLOB_READ_WRITE_TOKEN CRON_SECRET SENTRY_DSN MALWARE_SCANNER_ACCESS_CLIENT_ID MALWARE_SCANNER_ACCESS_CLIENT_SECRET" upload-sentry-source-maps: true diff --git a/.gitleaksignore b/.gitleaksignore index 114d80722b..8ef4055021 100644 --- a/.gitleaksignore +++ b/.gitleaksignore @@ -37,3 +37,11 @@ f5f578dfd4020c4a38f54eb5980a2310eefe94c3:.yarn/releases/yarn-4.3.0.cjs:generic-a 59da625af2ff0be48f582d288d73cbf4c52bf105:.yarn/releases/yarn-4.1.1.cjs:generic-api-key:567 73f7080432457e358b3678fb02d4b5c466e763eb:.yarn/releases/yarn-4.9.1.cjs:generic-api-key:589 140074ec8d53b2cac8c8bf84f7eecd5522f53201:.yarn/releases/yarn-4.4.0.cjs:generic-api-key:567 + +# Public ERC-20 contract address used as a test fixture in a variable named `token` +# (blockchain address, not a credential) — gitleaks reads it as a generic API key. Same +# finding as the permissionsList entry above, re-fingerprinted after the files moved to +# apps/app in the monorepo migration. +8f5ede4daeaaaa56ec7747fbf14a3c527b4c3194:apps/app/src/modules/settings/components/permissionCondition/permissionCondition.test.tsx:generic-api-key:41 +8f5ede4daeaaaa56ec7747fbf14a3c527b4c3194:apps/app/src/modules/settings/components/votingPowerConditionSlot/votingPowerConditionSlot.test.tsx:generic-api-key:46 +8f5ede4daeaaaa56ec7747fbf14a3c527b4c3194:apps/app/src/modules/settings/components/permissionsList/permissionsList.test.tsx:generic-api-key:381 diff --git a/apps/assistant/.env.example b/apps/assistant/.env.example index 5879dd541f..8ee6b7d3e2 100644 --- a/apps/assistant/.env.example +++ b/apps/assistant/.env.example @@ -20,6 +20,14 @@ PORT=4000 # ASSISTANT_RATE_LIMIT_RPM=10 # ASSISTANT_RATE_LIMIT_SESSIONS_PER_DAY=20 +# Malware scanning of attachments (defaults live in src/lib/config.ts). The kill switch and the +# per-engine policy: off (ignore the engine) | optional (only a detection blocks) | mandatory +# (a missing verdict blocks too, retriably). +# ASSISTANT_MALWARE_SCAN_ENABLED=true +# ASSISTANT_MALWARE_SCANNER_URL=https://file-malware-scanner-0.aragon-project.workers.dev +# ASSISTANT_MALWARE_SCAN_CLAUDE=mandatory +# ASSISTANT_MALWARE_SCAN_VIRUSTOTAL=optional + # --- Secrets (1Password: kv_assistant_) --- # Vercel AI Gateway key (dedicated key with a spend budget) @@ -46,3 +54,8 @@ CRON_SECRET= # Sentry DSN of the assistant project SENTRY_DSN= + +# Cloudflare Access service token of the file-malware-scanner worker (both halves required; +# without them the scan call is unauthenticated and Access rejects it) +MALWARE_SCANNER_ACCESS_CLIENT_ID= +MALWARE_SCANNER_ACCESS_CLIENT_SECRET= diff --git a/apps/assistant/README.md b/apps/assistant/README.md index d3de144fd9..2f48d6ad40 100644 --- a/apps/assistant/README.md +++ b/apps/assistant/README.md @@ -23,11 +23,13 @@ All chat/assistant copy is centralized in two places: service-side texts live in Users can attach files (images, text/log, PDF) to a support request. Bytes go **client → Vercel Blob directly**, are validated server-side by magic bytes + size (`src/files/validateFile.ts`), queued per session, and move to the **private** Linear ticket only when the ticket is created; abandoned blobs are swept by the daily `/internal/cleanup` cron. -**Current moderation posture (p4): deterrence + reactive.** There is intentionally **no** automated NSFW/illegal-content scanning. The risk is bounded by: a private end-to-end path (Blob → private Linear queue, never public), the type/size allowlist, per-IP rate + session limits (`src/lib/rateLimit.ts`, `src/lib/config.ts`), a small per-session file cap, a client-side upload disclaimer, and quick deletion. Uploaded content is reviewed reactively by the support team. +**Malware scanning.** Every confirmed upload is scanned before it is queued: `/files/confirm` downloads the blob, runs the magic-byte gate, then calls the `file-malware-scanner` Cloudflare worker (VirusTotal + Claude, see the `cloudflare-management` repo). A flagged file is **deleted from the blob store and never queued** — it reaches neither the ticket nor the support team — and the widget shows "Malicious code detected". Each engine has its own policy in `src/lib/config.ts` (`malwareScan`): `off` ignores it, `optional` blocks only on a positive detection, `mandatory` also blocks when the engine returns no verdict (retriable `scan_unavailable`). Defaults: Claude `mandatory`, VirusTotal `optional` (its free tier rate-limits at 4 req/min and knows no hash for freshly created files). The whole feature is behind `malwareScan.enabled` — off in production until validated on dev/preview — and every setting has an `ASSISTANT_MALWARE_SCAN_*` env override so a single deployment can flip it without a code change. + +**Content moderation posture (p4): deterrence + reactive.** Beyond malware, there is intentionally **no** automated NSFW/illegal-content scanning. The risk is bounded by: a private end-to-end path (Blob → private Linear queue, never public), the type/size allowlist, per-IP rate + session limits (`src/lib/rateLimit.ts`, `src/lib/config.ts`), a small per-session file cap, a client-side upload disclaimer, and quick deletion. Uploaded content is reviewed reactively by the support team. **Deferred (not in p4), tracked as follow-ups:** -- **Automated vision moderation** — a safety classification of accepted images at `/files/confirm` via the existing AI Gateway, before the file is queued. The hook point is marked with a `TODO(assistant)` in `src/files/validateFile.ts`. +- **Automated NSFW/vision moderation** — a safety classification of accepted images, orthogonal to the malware scan above (which targets malicious payloads, not explicit content). - **CSAM / illegal content** — needs a dedicated provider (hash-matching / reporting obligations); a policy + provider decision outside this codebase, not a model call. - **Console-log ring buffer** — an app-side `console.*` interceptor (last N lines, privacy-scrubbed) attached to the ticket as a `.log`. The next debug-signal increment after the cheap context already attached (chainId, recent transactions, Sentry `user.id` replay pointer). diff --git a/apps/assistant/src/files/malwareScanner.test.ts b/apps/assistant/src/files/malwareScanner.test.ts new file mode 100644 index 0000000000..75838f98df --- /dev/null +++ b/apps/assistant/src/files/malwareScanner.test.ts @@ -0,0 +1,265 @@ +import { createMalwareScanner } from './malwareScanner'; + +const buildScanResponse = (params: { + allowed: boolean; + claudeStatus: string; + virusTotalStatus: string; + reason?: string | null; +}) => + new Response( + JSON.stringify({ + apiVersion: 'v1', + allowed: params.allowed, + reason: params.reason ?? null, + verdicts: { + claude: { + status: params.claudeStatus, + reason: 'Analyzer reason.', + }, + virustotal: { status: params.virusTotalStatus }, + }, + }), + { status: params.allowed ? 200 : 422 }, + ); + +const mockFetch = (response: Response | Error) => { + const fetchMock = jest.fn(() => + response instanceof Error + ? Promise.reject(response) + : Promise.resolve(response), + ); + global.fetch = fetchMock as unknown as typeof fetch; + + return fetchMock; +}; + +const scanPng = () => + createMalwareScanner().scan({ + data: new Uint8Array([0x89, 0x50, 0x4e, 0x47]), + filename: 'screenshot.png', + }); + +// The per-engine policy (off / optional / mandatory) is the whole point of this seam: it decides +// whether an engine's silence costs the user their attachment. +describe('malwareScanner', () => { + const originalFetch = global.fetch; + + afterEach(() => { + global.fetch = originalFetch; + process.env.ASSISTANT_MALWARE_SCAN_CLAUDE = undefined; + process.env.ASSISTANT_MALWARE_SCAN_VIRUSTOTAL = undefined; + }); + + it('passes a file both engines consider clean', async () => { + mockFetch( + buildScanResponse({ + allowed: true, + claudeStatus: 'clean', + virusTotalStatus: 'clean', + }), + ); + + await expect(scanPng()).resolves.toEqual({ status: 'clean' }); + }); + + it('flags a file the AI analyzer marked malicious', async () => { + mockFetch( + buildScanResponse({ + allowed: false, + claudeStatus: 'malicious', + virusTotalStatus: 'unknown', + }), + ); + + await expect(scanPng()).resolves.toEqual({ + status: 'malicious', + reason: 'Analyzer reason.', + }); + }); + + it('treats an analyzer refusal as a detection', async () => { + mockFetch( + buildScanResponse({ + allowed: false, + claudeStatus: 'refused', + virusTotalStatus: 'unknown', + }), + ); + + await expect(scanPng()).resolves.toMatchObject({ + status: 'malicious', + }); + }); + + it('passes when an optional engine has no verdict', async () => { + // Default policy: VirusTotal is optional, so a hash it has never seen must not block. + mockFetch( + buildScanResponse({ + allowed: true, + claudeStatus: 'clean', + virusTotalStatus: 'unknown', + }), + ); + + await expect(scanPng()).resolves.toEqual({ status: 'clean' }); + }); + + it('reports unavailable when a mandatory engine has no verdict', async () => { + mockFetch( + buildScanResponse({ + allowed: true, + claudeStatus: 'error', + virusTotalStatus: 'clean', + }), + ); + + await expect(scanPng()).resolves.toMatchObject({ + status: 'unavailable', + }); + }); + + it('ignores an engine turned off, even on a detection', async () => { + process.env.ASSISTANT_MALWARE_SCAN_VIRUSTOTAL = 'off'; + mockFetch( + buildScanResponse({ + allowed: false, + claudeStatus: 'clean', + virusTotalStatus: 'malicious', + }), + ); + + await expect(scanPng()).resolves.toEqual({ status: 'clean' }); + }); + + it('blocks on a mandatory VirusTotal detection when configured that way', async () => { + process.env.ASSISTANT_MALWARE_SCAN_VIRUSTOTAL = 'mandatory'; + mockFetch( + buildScanResponse({ + allowed: false, + claudeStatus: 'clean', + virusTotalStatus: 'malicious', + reason: 'Flagged by 12 engines.', + }), + ); + + await expect(scanPng()).resolves.toEqual({ + status: 'malicious', + reason: 'Flagged by 12 engines.', + }); + }); + + it('prefers a detection over an unavailable engine', async () => { + mockFetch( + buildScanResponse({ + allowed: false, + claudeStatus: 'error', + virusTotalStatus: 'malicious', + }), + ); + process.env.ASSISTANT_MALWARE_SCAN_VIRUSTOTAL = 'mandatory'; + + await expect(scanPng()).resolves.toMatchObject({ + status: 'malicious', + }); + }); + + it('reports unavailable when the scanner itself is unreachable', async () => { + mockFetch(new Error('network down')); + + await expect(scanPng()).resolves.toMatchObject({ + status: 'unavailable', + }); + }); + + it('passes when the scanner is unreachable but no engine is mandatory', async () => { + process.env.ASSISTANT_MALWARE_SCAN_CLAUDE = 'optional'; + mockFetch(new Error('network down')); + + await expect(scanPng()).resolves.toEqual({ status: 'clean' }); + }); + + it('reports unavailable on an unexpected scanner status code', async () => { + mockFetch(new Response('nope', { status: 500 })); + + await expect(scanPng()).resolves.toMatchObject({ + status: 'unavailable', + }); + }); + it('reports unavailable when an engine verdict is missing from the response', async () => { + // A payload shaped like a verdict but missing an engine says nothing about the file: + // reading it must not throw halfway through the decision. + mockFetch( + new Response( + JSON.stringify({ + apiVersion: 'v1', + allowed: true, + reason: null, + verdicts: { claude: { status: 'clean' } }, + }), + { status: 200 }, + ), + ); + + await expect(scanPng()).resolves.toMatchObject({ + status: 'unavailable', + }); + }); + + it('reports unavailable when an engine reports no status', async () => { + mockFetch( + new Response( + JSON.stringify({ + verdicts: { claude: {}, virustotal: { status: 'clean' } }, + }), + { status: 200 }, + ), + ); + + await expect(scanPng()).resolves.toMatchObject({ + status: 'unavailable', + }); + }); + + it('reports unavailable when the response carries no verdicts at all', async () => { + mockFetch( + new Response(JSON.stringify({ allowed: true }), { status: 200 }), + ); + + await expect(scanPng()).resolves.toMatchObject({ + status: 'unavailable', + }); + }); + it('sends a non-Latin-1 filename the scanner can receive', async () => { + // Header values are ByteStrings: an unencoded Japanese or Cyrillic name (the default + // screenshot name in those locales) makes fetch throw before any network call, which the + // catch would report as a scanner outage — blocking the file on every retry. The mock + // below builds real Headers so that validation actually runs. + const fetchMock = jest.fn((_url: string, init: RequestInit) => { + new Headers(init.headers); + + return Promise.resolve( + buildScanResponse({ + allowed: true, + claudeStatus: 'clean', + virusTotalStatus: 'clean', + }), + ); + }); + global.fetch = fetchMock as unknown as typeof fetch; + + const verdict = await createMalwareScanner().scan({ + data: new Uint8Array([0x89, 0x50, 0x4e, 0x47]), + filename: 'スクリーンショット.png', + }); + + expect(verdict).toEqual({ status: 'clean' }); + + const headers = fetchMock.mock.calls[0]?.[1].headers as Record< + string, + string + >; + expect(headers['x-filename']).toEqual( + encodeURIComponent('スクリーンショット.png'), + ); + }); +}); diff --git a/apps/assistant/src/files/malwareScanner.ts b/apps/assistant/src/files/malwareScanner.ts new file mode 100644 index 0000000000..bca604be95 --- /dev/null +++ b/apps/assistant/src/files/malwareScanner.ts @@ -0,0 +1,201 @@ +import { getConfig, type IMalwareEngineMode } from '../lib/config'; +import { env } from '../lib/env'; +import { observability } from '../lib/observability'; + +export interface IScanFileInput { + data: Uint8Array; + filename: string; +} + +export type IScanVerdict = + // The file passed every enabled engine. + | { status: 'clean' } + // An engine flagged the file: it must be deleted and never queued. + | { status: 'malicious'; reason: string } + // A mandatory engine could not deliver a verdict; the caller should let the user retry. + | { status: 'unavailable'; reason: string }; + +// Seam over the scanner service (the file-malware-scanner Cloudflare worker): routes and tests +// only consume this interface. The worker runs VirusTotal and Claude in parallel and answers with +// the v1 contract documented in cloudflare-management (POST /v1/scan). +export interface IMalwareScanner { + scan(input: IScanFileInput): Promise; +} + +// Shape of the worker's v1 response. Only the fields we act on are modelled; unknown fields are +// ignored by design, and unknown `status` values are treated conservatively (see below). Every +// field is optional because this is an untrusted payload: a partial response must fail the scan, +// never throw halfway through the decision. +interface IScanEngineVerdict { + status?: string; + reason?: string; + detail?: string; +} + +interface IScanResponse { + allowed?: boolean; + reason?: string | null; + verdicts?: { + virustotal?: IScanEngineVerdict; + claude?: IScanEngineVerdict; + }; +} + +// Engine verdicts that mean "this engine flagged the file". `refused` is Claude declining to +// analyze the content, which the worker treats as a detection signal. +const detectionStatuses = new Set(['malicious', 'refused']); +// Engine verdicts that mean "this engine delivered a clean result". +const cleanStatuses = new Set(['clean']); + +// A single engine's contribution to the decision, given how strictly it is configured. Anything +// that is neither a detection nor a clean result (`unknown`, `pending`, `error`, or a status this +// build does not know yet) counts as "no verdict". +const evaluateEngine = (params: { + mode: IMalwareEngineMode; + status: string; + reason: string; + engine: string; +}): IScanVerdict => { + const { mode, status, reason, engine } = params; + + if (mode === 'off') { + return { status: 'clean' }; + } + + if (detectionStatuses.has(status)) { + return { status: 'malicious', reason }; + } + + if (cleanStatuses.has(status) || mode === 'optional') { + return { status: 'clean' }; + } + + return { + status: 'unavailable', + reason: `${engine} could not verify the file (${status}).`, + }; +}; + +export const createMalwareScanner = (): IMalwareScanner => { + const { malwareScan } = getConfig(); + const apiKey = env.malwareScannerApiKey(); + + return { + scan: async ({ data, filename }) => { + const controller = new AbortController(); + const timeout = setTimeout( + () => controller.abort(), + malwareScan.timeoutMs, + ); + + let response: Response; + try { + response = await fetch( + `${malwareScan.serviceUrl}/v1/scan?vtUpload=${String(malwareScan.virusTotalUpload)}`, + { + method: 'POST', + headers: { + 'content-type': 'application/octet-stream', + // Percent-encoded: header values are ByteStrings, so a name carrying + // any code point above U+00FF (a Japanese or Cyrillic screenshot + // name, an emoji) makes fetch throw before any network call. The + // scanner only uses this as a display name. + 'x-filename': encodeURIComponent(filename), + // Cloudflare Access service token; the worker itself is unauthenticated. + ...(apiKey == null + ? {} + : { + 'cf-access-client-id': apiKey.clientId, + 'cf-access-client-secret': + apiKey.clientSecret, + }), + }, + body: new Uint8Array(data), + signal: controller.signal, + }, + ); + } catch (error) { + // Logged rather than silently folded into "unreachable": a request we failed to + // even construct is our bug, and it would otherwise be indistinguishable from a + // scanner outage in both the logs and the user-facing copy. + observability.logError(error, { step: 'scanFile' }); + + return unavailableOrClean( + malwareScan, + 'The scanner is unreachable.', + ); + } finally { + clearTimeout(timeout); + } + + // 200 (allowed) and 422 (blocked) both carry a verdict body; anything else is a + // scanner failure, not a verdict about the file. + if (response.status !== 200 && response.status !== 422) { + return unavailableOrClean( + malwareScan, + `The scanner returned status ${String(response.status)}.`, + ); + } + + const body = (await response + .json() + .catch(() => null)) as IScanResponse | null; + + const claude = body?.verdicts?.claude; + const virustotal = body?.verdicts?.virustotal; + + // Both engines must report a status. A payload missing either one says nothing about + // the file, so it is a scanner failure — not a verdict — and must be reported as such + // instead of throwing mid-decision (which would leave the file's claim taken). + if ( + typeof claude?.status !== 'string' || + typeof virustotal?.status !== 'string' + ) { + return unavailableOrClean( + malwareScan, + 'The scanner returned an unexpected response.', + ); + } + + const results = [ + evaluateEngine({ + mode: malwareScan.claude, + status: claude.status, + reason: + claude.reason ?? + body?.reason ?? + 'Flagged by the AI analyzer.', + engine: 'The AI analyzer', + }), + evaluateEngine({ + mode: malwareScan.virusTotal, + status: virustotal.status, + reason: + virustotal.reason ?? + body?.reason ?? + 'Flagged by VirusTotal.', + engine: 'VirusTotal', + }), + ]; + + // A detection always wins over an unavailable engine: a flagged file is blocked for + // good, not offered as retriable. + return ( + results.find((result) => result.status === 'malicious') ?? + results.find((result) => result.status === 'unavailable') ?? { + status: 'clean', + } + ); + }, + }; +}; + +// A transport-level failure blocks only when at least one engine is mandatory; with every engine +// optional the scan is advisory and the file passes. +const unavailableOrClean = ( + malwareScan: { claude: IMalwareEngineMode; virusTotal: IMalwareEngineMode }, + reason: string, +): IScanVerdict => + malwareScan.claude === 'mandatory' || malwareScan.virusTotal === 'mandatory' + ? { status: 'unavailable', reason } + : { status: 'clean' }; diff --git a/apps/assistant/src/files/validateFile.test.ts b/apps/assistant/src/files/validateFile.test.ts index 76e0bd7e3d..f53588e822 100644 --- a/apps/assistant/src/files/validateFile.test.ts +++ b/apps/assistant/src/files/validateFile.test.ts @@ -32,6 +32,16 @@ describe('sanitizeFilename', () => { expect(sanitizeFilename('re\u0007port.txt')).toEqual('report.txt'); }); + it('strips invisible format characters used to disguise the extension', () => { + // A right-to-left override makes the rest of the name render reversed, so this shows up + // as "invoiceexe.png" to whoever reads the ticket. + expect(sanitizeFilename('invoice\u202Egnp.exe')).toEqual( + 'invoicegnp.exe', + ); + // Zero-width characters would otherwise let two files carry the same visible name. + expect(sanitizeFilename('report\u200B.pdf')).toEqual('report.pdf'); + }); + it('falls back for empty or dot-only names', () => { expect(sanitizeFilename('..')).toEqual('file'); expect(sanitizeFilename('')).toEqual('file'); diff --git a/apps/assistant/src/files/validateFile.ts b/apps/assistant/src/files/validateFile.ts index 228351ed18..1960717815 100644 --- a/apps/assistant/src/files/validateFile.ts +++ b/apps/assistant/src/files/validateFile.ts @@ -28,7 +28,11 @@ const maxFilenameLength = 120; export const sanitizeFilename = (raw: string): string => { const withoutPath = raw.normalize('NFC').split(/[/\\]/).pop() ?? ''; - const cleaned = withoutPath.replace(/\p{Cc}/gu, '').trim(); + // Control chars (Cc) and invisible format chars (Cf) both go: a support engineer reads this + // name in the ticket, and a bidi override (U+202E) renders `invoicegnp.exe` as + // `invoiceexe.png`. Cf also covers zero-width characters used to make two names look + // identical. Side effect: emoji joined by ZWJ render as their separate parts. + const cleaned = withoutPath.replace(/[\p{Cc}\p{Cf}]/gu, '').trim(); if (cleaned.length === 0 || cleaned === '.' || cleaned === '..') { return 'file'; @@ -77,11 +81,9 @@ export const validateFile = async ( if (magicType != null) { const contentType = allowedMagicTypes[magicType.ext]; - // Everything sniffable but not allowlisted (svg/xml included) is rejected here. - // TODO(assistant): automated content moderation hook. This is where a vision-model safety - // scan for accepted images (contentType.startsWith('image/')) would slot in before the - // file is queued — see the moderation posture in README.md. Not implemented in p4; current - // posture is deterrence + reactive review of the private Linear queue. + // Everything sniffable but not allowlisted (svg/xml included) is rejected here. Accepted + // files then go through the malware scan in /files/confirm (see files.ts); NSFW/illegal + // content moderation stays deterrence + reactive review — see README.md. return contentType == null ? { error: 'unsupported_file' } : { data, filename, contentType, size: data.byteLength }; diff --git a/apps/assistant/src/index.ts b/apps/assistant/src/index.ts index 8c07947cec..2ed9a4aa98 100644 --- a/apps/assistant/src/index.ts +++ b/apps/assistant/src/index.ts @@ -6,6 +6,7 @@ import { Hono } from 'hono'; import { cors } from 'hono/cors'; import { getChatModel } from './chat/models'; import { createVercelBlobStore } from './files/blobStore'; +import { createMalwareScanner } from './files/malwareScanner'; import { type IAppDependencies, lazy } from './lib/appDependencies'; import { getConfig } from './lib/config'; import { resolveCorsOrigin } from './lib/cors'; @@ -32,6 +33,7 @@ const buildDefaultDependencies = (): IAppDependencies => { getLinear: lazy(() => createLinearGateway()), getChatModel, getBlobStore: lazy(() => createVercelBlobStore()), + getMalwareScanner: lazy(() => createMalwareScanner()), }; }; diff --git a/apps/assistant/src/lib/appDependencies.ts b/apps/assistant/src/lib/appDependencies.ts index d28f8f4e4e..415f9002e5 100644 --- a/apps/assistant/src/lib/appDependencies.ts +++ b/apps/assistant/src/lib/appDependencies.ts @@ -1,6 +1,7 @@ import type { Redis } from '@upstash/redis'; import type { LanguageModel } from 'ai'; import type { IBlobStore } from '../files/blobStore'; +import type { IMalwareScanner } from '../files/malwareScanner'; import type { ILinearGateway } from '../linear/linearGateway'; import type { ISessionStore } from './sessionStore'; @@ -14,6 +15,7 @@ export interface IAppDependencies { getLinear: () => ILinearGateway; getChatModel: () => LanguageModel; getBlobStore: () => IBlobStore; + getMalwareScanner: () => IMalwareScanner; } export const lazy = (factory: () => TValue): (() => TValue) => { diff --git a/apps/assistant/src/lib/config.ts b/apps/assistant/src/lib/config.ts index 2a48d24b60..b0a199b62b 100644 --- a/apps/assistant/src/lib/config.ts +++ b/apps/assistant/src/lib/config.ts @@ -1,5 +1,42 @@ import { type AssistantEnvironment, env } from './env'; +/** + * How strictly one scan engine gates an upload: + * - `off`: the engine's verdict is ignored entirely. + * - `optional`: only a positive detection blocks; an engine failure or a missing verdict passes. + * - `mandatory`: a detection blocks, and so does a missing verdict (fail-closed, retriable). + */ +export type IMalwareEngineMode = 'off' | 'optional' | 'mandatory'; + +export interface IMalwareScanConfig { + /** + * Master switch. When false no scan runs and uploads behave exactly as before. + */ + enabled: boolean; + /** + * Base URL of the file-malware-scanner worker (its /v1/scan contract). + */ + serviceUrl: string; + /** + * Strictness of the AI analyzer. + */ + claude: IMalwareEngineMode; + /** + * Strictness of VirusTotal. Kept `optional` by default: the free tier allows 4 requests per + * minute and does not know hashes of freshly created files, so its failures must not block. + */ + virusTotal: IMalwareEngineMode; + /** + * Whether the scanner may upload hash-unknown files to VirusTotal. Off for user attachments: + * uploaded files are shared with the VirusTotal community. + */ + virusTotalUpload: boolean; + /** + * Budget for the scan call; a timeout counts as a missing verdict. + */ + timeoutMs: number; +} + export interface IAssistantConfig { /** * Origins allowed to call the API: exact origins or *suffix patterns. All environments accept @@ -25,6 +62,10 @@ export interface IAssistantConfig { agentModel: string; fallbackModels: string[]; }; + /** + * Malware scanning of confirmed uploads, before the file is queued for the ticket. + */ + malwareScan: IMalwareScanConfig; } const appOrigins = ['https://app.aragon.org', '*.app.aragon.org']; @@ -51,6 +92,20 @@ const defaultChat = { fallbackModels: ['google/gemini-2.5-flash-lite'], }; +// Attachments are scanned by the file-malware-scanner worker (see the cloudflare-management repo) +// between the blob download and the queueing of the file. Claude is mandatory — it inspects the +// content itself and its verdict is the reason we scan at all — while VirusTotal stays optional: +// on the free tier it rate-limits at 4 requests/minute and returns no verdict for files it has +// never seen, neither of which should cost an honest user their attachment. +const defaultMalwareScan: IMalwareScanConfig = { + enabled: true, + serviceUrl: 'https://file-malware-scanner-0.aragon-project.workers.dev', + claude: 'mandatory', + virusTotal: 'optional', + virusTotalUpload: false, + timeoutMs: 20_000, +}; + // Non-secret per-environment configuration. Kept as a checked-in typed module because Vercel // functions receive no .env file at runtime; secrets stay in 1Password and reach the runtime as // environment variables (see .env.example). @@ -60,24 +115,30 @@ const configByEnvironment: Record = { docsSearchEnabled: false, rateLimit: defaultRateLimit, chat: defaultChat, + malwareScan: defaultMalwareScan, }, development: { corsAllowedOrigins: [...appOrigins, ...previewOrigins], docsSearchEnabled: false, rateLimit: defaultRateLimit, chat: defaultChat, + malwareScan: defaultMalwareScan, }, preview: { corsAllowedOrigins: [...appOrigins, ...previewOrigins], docsSearchEnabled: false, rateLimit: defaultRateLimit, chat: defaultChat, + malwareScan: defaultMalwareScan, }, production: { corsAllowedOrigins: appOrigins, docsSearchEnabled: false, rateLimit: defaultRateLimit, chat: defaultChat, + // Rolled out to dev/preview first; flip `enabled` once the scanner has been observed + // there (the ASSISTANT_MALWARE_SCAN_ENABLED override can also enable it per deployment). + malwareScan: { ...defaultMalwareScan, enabled: false }, }, }; @@ -93,5 +154,17 @@ export const getConfig = (): IAssistantConfig => { env.rateLimitSessionsPerDay() ?? config.rateLimit.sessionsPerDay, }, + // Env overrides let a single deployment flip the scan (or loosen one engine) without a + // code change — the kill switch when the scanner misbehaves in production. + malwareScan: { + ...config.malwareScan, + enabled: env.malwareScanEnabled() ?? config.malwareScan.enabled, + serviceUrl: + env.malwareScannerUrl() ?? config.malwareScan.serviceUrl, + claude: env.malwareScanClaudeMode() ?? config.malwareScan.claude, + virusTotal: + env.malwareScanVirusTotalMode() ?? + config.malwareScan.virusTotal, + }, }; }; diff --git a/apps/assistant/src/lib/env.ts b/apps/assistant/src/lib/env.ts index 6be9623567..18f0876318 100644 --- a/apps/assistant/src/lib/env.ts +++ b/apps/assistant/src/lib/env.ts @@ -28,6 +28,23 @@ const parseOptionalNumber = (value?: string): number | undefined => { return Number.isNaN(parsed) ? undefined : parsed; }; +// Only an explicit "true"/"false" overrides the per-environment config; anything else (unset, +// empty, typo) leaves the checked-in default in place. +const parseOptionalBoolean = (value?: string): boolean | undefined => { + if (value === 'true') { + return true; + } + + return value === 'false' ? false : undefined; +}; + +const malwareEngineModes = ['off', 'optional', 'mandatory'] as const; + +const parseEngineMode = ( + value?: string, +): (typeof malwareEngineModes)[number] | undefined => + malwareEngineModes.find((mode) => mode === value); + export const env = { // ASSISTANT_ENV is set locally through .env.local; on Vercel the runtime only exposes the // automatic VERCEL_ENV (production | preview | development), which maps 1:1 onto our values @@ -52,4 +69,23 @@ export const env = { parseOptionalNumber(process.env.ASSISTANT_RATE_LIMIT_RPM), rateLimitSessionsPerDay: (): number | undefined => parseOptionalNumber(process.env.ASSISTANT_RATE_LIMIT_SESSIONS_PER_DAY), + malwareScanEnabled: (): boolean | undefined => + parseOptionalBoolean(process.env.ASSISTANT_MALWARE_SCAN_ENABLED), + malwareScannerUrl: (): string | undefined => + process.env.ASSISTANT_MALWARE_SCANNER_URL || undefined, + malwareScanClaudeMode: () => + parseEngineMode(process.env.ASSISTANT_MALWARE_SCAN_CLAUDE), + malwareScanVirusTotalMode: () => + parseEngineMode(process.env.ASSISTANT_MALWARE_SCAN_VIRUSTOTAL), + // Cloudflare Access service token for the scanner worker; both halves must be present. + malwareScannerApiKey: (): + | { clientId: string; clientSecret: string } + | undefined => { + const clientId = process.env.MALWARE_SCANNER_ACCESS_CLIENT_ID; + const clientSecret = process.env.MALWARE_SCANNER_ACCESS_CLIENT_SECRET; + + return clientId && clientSecret + ? { clientId, clientSecret } + : undefined; + }, }; diff --git a/apps/assistant/src/lib/observability.ts b/apps/assistant/src/lib/observability.ts index cada4fb207..83b537e6f1 100644 --- a/apps/assistant/src/lib/observability.ts +++ b/apps/assistant/src/lib/observability.ts @@ -4,6 +4,7 @@ export type IAssistantStep = | 'respond' | 'createTicket' | 'confirmFile' + | 'scanFile' | 'removeFile' | 'transferFiles' | 'cleanupBlobs' @@ -32,6 +33,9 @@ export interface IStepLogEntry { // Model stop reason ('stop', 'length', 'tool-calls', …): a 'length' means the reply or the // tool arguments were truncated before completing. finishReason?: string; + // Outcome of the malware scan on a scanFile event: a verdict category, never the reason + // text (which describes what was detected in the user's file). + scanVerdict?: 'clean' | 'malicious' | 'unavailable'; // Error name/code only, never a message that could carry user content. error?: string; } diff --git a/apps/assistant/src/routes/files.test.ts b/apps/assistant/src/routes/files.test.ts index fb074c4a13..264439df9c 100644 --- a/apps/assistant/src/routes/files.test.ts +++ b/apps/assistant/src/routes/files.test.ts @@ -284,4 +284,105 @@ describe('files routes', () => { expect(await deps.sessionStore.listFiles(sessionId)).toEqual([]); expect(deps.blobStore.deletedUrls).toEqual([blobUrl]); }); + it('deletes the blob and never queues a file the scanner flagged as malicious', async () => { + const deps = createTestDependencies(createMockChatModel({})); + const blobUrl = buildBlobUrl(); + deps.blobStore.blobs.set(blobUrl, pngBytes); + deps.malwareScanner.nextVerdict = { + status: 'malicious', + reason: 'Embedded script detected.', + }; + + const response = await confirmUpload(buildApp(deps), { blobUrl }); + + expect(response.status).toEqual(422); + const body = (await response.json()) as IAssistantError; + expect(body.error.code).toEqual('malicious_file'); + expect(deps.blobStore.deletedUrls).toEqual([blobUrl]); + expect(await deps.sessionStore.listFiles(sessionId)).toEqual([]); + }); + + it('blocks retriably when a mandatory scan engine has no verdict', async () => { + const deps = createTestDependencies(createMockChatModel({})); + const blobUrl = buildBlobUrl(); + deps.blobStore.blobs.set(blobUrl, pngBytes); + deps.malwareScanner.nextVerdict = { + status: 'unavailable', + reason: 'The scanner is unreachable.', + }; + + const response = await confirmUpload(buildApp(deps), { blobUrl }); + + expect(response.status).toEqual(503); + const body = (await response.json()) as IAssistantError; + expect(body.error.code).toEqual('scan_unavailable'); + // The slot is freed and the blob removed, so a retry starts from a clean state. + expect(deps.blobStore.deletedUrls).toEqual([blobUrl]); + expect(await deps.sessionStore.listFiles(sessionId)).toEqual([]); + }); + + it('scans the file before queueing it and passes the sanitized filename', async () => { + const deps = createTestDependencies(createMockChatModel({})); + const blobUrl = buildBlobUrl(); + deps.blobStore.blobs.set(blobUrl, pngBytes); + + const response = await confirmUpload(buildApp(deps), { blobUrl }); + + expect(response.status).toEqual(201); + expect(deps.malwareScanner.scanCalls).toEqual([ + { filename: 'screenshot.png', size: pngBytes.byteLength }, + ]); + expect(await deps.sessionStore.listFiles(sessionId)).toHaveLength(1); + }); + + it('does not scan files rejected by the format gate', async () => { + const deps = createTestDependencies(createMockChatModel({})); + const blobUrl = buildBlobUrl({ filename: 'renamed.png' }); + deps.blobStore.blobs.set( + blobUrl, + new Uint8Array([0x4d, 0x5a, 0x90, 0, 3, 0, 0, 0]), + ); + + await confirmUpload(buildApp(deps), { blobUrl }); + + expect(deps.malwareScanner.scanCalls).toEqual([]); + }); + + it('skips the scan entirely when it is disabled', async () => { + process.env.ASSISTANT_MALWARE_SCAN_ENABLED = 'false'; + const deps = createTestDependencies(createMockChatModel({})); + const blobUrl = buildBlobUrl(); + deps.blobStore.blobs.set(blobUrl, pngBytes); + deps.malwareScanner.nextVerdict = { + status: 'malicious', + reason: 'Embedded script detected.', + }; + + const response = await confirmUpload(buildApp(deps), { blobUrl }); + + expect(response.status).toEqual(201); + expect(deps.malwareScanner.scanCalls).toEqual([]); + process.env.ASSISTANT_MALWARE_SCAN_ENABLED = undefined; + }); + it('releases the claim when the scan throws, so the upload can be retried', async () => { + const deps = createTestDependencies(createMockChatModel({})); + const blobUrl = buildBlobUrl(); + deps.blobStore.blobs.set(blobUrl, pngBytes); + deps.malwareScanner.failNextScan = true; + const app = buildApp(deps); + + const failed = await confirmUpload(app, { blobUrl }); + + expect(failed.status).toEqual(503); + const body = (await failed.json()) as IAssistantError; + expect(body.error.code).toEqual('scan_unavailable'); + + // The claim must not stay taken: a retry of the same blob has to reach the scanner + // again rather than bouncing off "already being confirmed" forever. + deps.blobStore.blobs.set(blobUrl, pngBytes); + const retry = await confirmUpload(app, { blobUrl }); + + expect(retry.status).toEqual(201); + expect(await deps.sessionStore.listFiles(sessionId)).toHaveLength(1); + }); }); diff --git a/apps/assistant/src/routes/files.ts b/apps/assistant/src/routes/files.ts index 35d8190cbe..bbaacc0005 100644 --- a/apps/assistant/src/routes/files.ts +++ b/apps/assistant/src/routes/files.ts @@ -17,6 +17,7 @@ import { } from '../files/blobPath'; import { validateFile } from '../files/validateFile'; import type { IAppDependencies } from '../lib/appDependencies'; +import { getConfig } from '../lib/config'; import { env } from '../lib/env'; import { observability } from '../lib/observability'; import type { ISessionFile } from '../lib/sessionStore'; @@ -27,6 +28,8 @@ const errorStatusByCode: Partial< file_too_large: 413, unsupported_file: 415, file_limit: 429, + malicious_file: 422, + scan_unavailable: 503, internal: 500, }; @@ -263,12 +266,10 @@ export const buildFilesRoute = (deps: IAppDependencies) => return context.json(body, status); } - const validated = await validateFile(data, parsedPath.filename); - - if ('error' in validated) { + // Releases the slot and deletes the blob of a file we are not going to queue + // (best effort on the blob — the cleanup cron sweeps leftovers). + const rejectFile = async () => { await sessionStore.releaseFileClaim(sessionId, fileId); - // The rejected blob is deleted right away (best effort — the cleanup cron - // sweeps leftovers). await deps .getBlobStore() .delete([blobUrl]) @@ -278,6 +279,12 @@ export const buildFilesRoute = (deps: IAppDependencies) => step: 'confirmFile', }), ); + }; + + const validated = await validateFile(data, parsedPath.filename); + + if ('error' in validated) { + await rejectFile(); const { body, status } = buildError( validated.error, validated.error === 'file_too_large' @@ -288,6 +295,51 @@ export const buildFilesRoute = (deps: IAppDependencies) => return context.json(body, status); } + // Malware scan of the content itself, after the format gate and before the file is + // queued: a flagged file is deleted here and never reaches the queue, the ticket or + // the support team. A mandatory engine without a verdict blocks too, but as a + // retriable error — see the malwareScan config for the per-engine policy. + if (getConfig().malwareScan.enabled) { + const scanStartTime = Date.now(); + // An unexpected throw here would skip the rollback below and leave the file's + // claim taken, so the user could never re-confirm that upload. Any failure is + // downgraded to a retriable verdict instead. + const verdict = await deps + .getMalwareScanner() + .scan({ data, filename: validated.filename }) + .catch((error: unknown) => { + observability.logError(error, { + sessionId, + step: 'scanFile', + }); + + return { + status: 'unavailable' as const, + reason: 'The file could not be scanned.', + }; + }); + + // Every scan is logged, clean ones included: without it a passing scan leaves no + // trace and there is no way to tell scanning ran at all from the logs. + observability.logStep({ + sessionId, + step: 'scanFile', + scanVerdict: verdict.status, + latencyMs: Date.now() - scanStartTime, + }); + + if (verdict.status !== 'clean') { + await rejectFile(); + + const { body, status } = + verdict.status === 'malicious' + ? buildError('malicious_file', verdict.reason) + : buildError('scan_unavailable', verdict.reason); + + return context.json(body, status); + } + } + // Re-attached identical bytes (e.g. the same screenshot uploaded twice) queue as // their own entry: every composer tile owns its fileId and blob, so removing one // never orphans another. The ticket still carries the content once — the transfer diff --git a/apps/assistant/src/test/testDependencies.ts b/apps/assistant/src/test/testDependencies.ts index 8a2a9b9b17..dd9fe5e095 100644 --- a/apps/assistant/src/test/testDependencies.ts +++ b/apps/assistant/src/test/testDependencies.ts @@ -1,5 +1,6 @@ import type { LanguageModel } from 'ai'; import type { IBlobInfo, IBlobStore } from '../files/blobStore'; +import type { IMalwareScanner, IScanVerdict } from '../files/malwareScanner'; import type { IAppDependencies } from '../lib/appDependencies'; import { createSessionStore, type ISessionStore } from '../lib/sessionStore'; import type { ILinearGateway } from '../linear/linearGateway'; @@ -96,11 +97,41 @@ export const createTestBlobStore = (): ITestBlobStore => { return store; }; +export interface ITestMalwareScanner extends IMalwareScanner { + scanCalls: Array<{ filename: string; size: number }>; + // Verdict returned by the next scan; defaults to clean. + nextVerdict: IScanVerdict; + // Makes the next scan reject, standing in for an unexpected failure inside the scanner. + failNextScan: boolean; +} + +export const createTestMalwareScanner = (): ITestMalwareScanner => { + const scanner: ITestMalwareScanner = { + scanCalls: [], + nextVerdict: { status: 'clean' }, + failNextScan: false, + scan: ({ data, filename }) => { + scanner.scanCalls.push({ filename, size: data.byteLength }); + + if (scanner.failNextScan) { + scanner.failNextScan = false; + + return Promise.reject(new Error('Scanner exploded')); + } + + return Promise.resolve(scanner.nextVerdict); + }, + }; + + return scanner; +}; + export interface ITestDependencies extends IAppDependencies { redis: IMockRedis; sessionStore: ISessionStore; linear: ITestLinearGateway; blobStore: ITestBlobStore; + malwareScanner: ITestMalwareScanner; } export const createTestDependencies = ( @@ -110,16 +141,19 @@ export const createTestDependencies = ( const sessionStore = createSessionStore(asRedis(redis)); const linear = createTestLinearGateway(); const blobStore = createTestBlobStore(); + const malwareScanner = createTestMalwareScanner(); return { redis, sessionStore, linear, blobStore, + malwareScanner, getRedis: () => asRedis(redis), getSessionStore: () => sessionStore, getLinear: () => linear, getChatModel: () => chatModel, getBlobStore: () => blobStore, + getMalwareScanner: () => malwareScanner, }; }; diff --git a/packages/assistant-chat/src/copy.ts b/packages/assistant-chat/src/copy.ts index 3486fd5dfd..00a86d21e6 100644 --- a/packages/assistant-chat/src/copy.ts +++ b/packages/assistant-chat/src/copy.ts @@ -67,8 +67,16 @@ export const chatCopy = { file_limit: `You can attach up to ${assistantLimits.maxFilesPerMessage} files per message.`, uploadFailed: 'Upload failed. Please try again.', removeFailed: "Couldn't remove the file. Please try again.", + // The scanner flagged the file; it was deleted server-side and never reaches the team. + maliciousFile: 'Malicious code detected. The file was not attached.', + scanUnavailable: + "We couldn't scan this file right now. Please try again in a moment.", } satisfies Record< - FileRejectReason | 'uploadFailed' | 'removeFailed', + | FileRejectReason + | 'uploadFailed' + | 'removeFailed' + | 'maliciousFile' + | 'scanUnavailable', string >, ticketCard: { diff --git a/packages/assistant-chat/src/files/attachmentAdapter.test.ts b/packages/assistant-chat/src/files/attachmentAdapter.test.ts new file mode 100644 index 0000000000..14eddb2055 --- /dev/null +++ b/packages/assistant-chat/src/files/attachmentAdapter.test.ts @@ -0,0 +1,169 @@ +import type { PendingAttachment } from '@assistant-ui/react'; +import { chatCopy } from '../copy'; +import { createAttachmentAdapter } from './attachmentAdapter'; + +// The blob client uploads straight to storage; the adapter's behaviour under test starts at the +// /files/confirm response, so the upload itself is stubbed to a successful blob. +jest.mock('@vercel/blob/client', () => ({ + upload: jest.fn(() => + Promise.resolve({ url: 'https://store.test/assistant/blob' }), + ), +})); + +const sessionId = 'b3b8f8a2-6c9d-4c9e-8f6a-2d1e0c9b8a7f'; + +const buildAdapter = () => + createAttachmentAdapter({ + assistantUrl: 'https://assistant.test', + getSessionId: () => sessionId, + logError: jest.fn(), + }); + +const pngFile = () => + new File([new Uint8Array([0x89, 0x50, 0x4e, 0x47])], 'screenshot.png', { + type: 'image/png', + }); + +// Drives the adapter's `add` generator to completion, returning the thrown error (if any) the +// way the runtime surfaces it on the attachment tile. +const runAdd = async ( + adapter: ReturnType, + file: File, +): Promise<{ attachment: PendingAttachment; error?: Error }> => { + // `add` is typed as a generator-or-promise union; this adapter always returns the generator. + const generator = adapter.add({ file }) as AsyncGenerator< + PendingAttachment, + void + >; + const first = await generator.next(); + const attachment = first.value as PendingAttachment; + + try { + await generator.next(); + + return { attachment }; + } catch (error) { + return { attachment, error: error as Error }; + } +}; + +// The service rejects the upload at /files/confirm (422 malicious_file): the blob is deleted +// server-side and nothing is queued for the ticket. +const mockConfirmRejection = () => { + global.fetch = jest.fn(() => + Promise.resolve( + new Response( + JSON.stringify({ + error: { + code: 'malicious_file', + message: 'Malicious content detected.', + }, + }), + { + status: 422, + headers: { 'content-type': 'application/json' }, + }, + ), + ), + ) as unknown as typeof fetch; +}; + +// The service accepts the upload and queues the file for the ticket. +const mockConfirmAccepted = () => { + global.fetch = jest.fn((_input: RequestInfo | URL, init?: RequestInit) => { + if (init?.method === 'DELETE') { + return Promise.resolve(new Response(null, { status: 204 })); + } + + return Promise.resolve( + new Response( + JSON.stringify({ + id: 'file-1', + filename: 'screenshot.png', + contentType: 'image/png', + size: 4, + }), + { + status: 201, + headers: { 'content-type': 'application/json' }, + }, + ), + ); + }) as unknown as typeof fetch; +}; + +describe('createAttachmentAdapter', () => { + const originalFetch = global.fetch; + + afterEach(() => { + global.fetch = originalFetch; + }); + + it('refuses to send an attachment whose upload the service rejected', async () => { + mockConfirmRejection(); + const adapter = buildAdapter(); + + const { attachment, error } = await runAdd(adapter, pngFile()); + + // The tile reports the rejection with our own wording… + expect(error?.message).toEqual(chatCopy.fileAlerts.maliciousFile); + + // …and the message cannot carry the file: the server holds no bytes for it, so sending + // it would show the user an attachment the support team never receives. + await expect(adapter.send(attachment)).rejects.toThrow(); + }); + + it('frees the composer slot of a rejected attachment', async () => { + mockConfirmRejection(); + const adapter = buildAdapter(); + + // Rejected uploads must not consume the per-message slots: several rejections in a row + // still leave room to attach a valid file. + for (let index = 0; index < 3; index++) { + await runAdd(adapter, pngFile()); + } + + const { error } = await runAdd(adapter, pngFile()); + + // Still the upload rejection, never the "too many files" alert. + expect(error?.message).toEqual(chatCopy.fileAlerts.maliciousFile); + }); + it('stays sendable when another attachment of the same message failed', async () => { + // The composer sends every attachment concurrently and, when one throws, restores them + // all and re-sends. A file that already went must survive that second round: otherwise + // the message is blocked forever and `remove` can no longer delete it server-side. + mockConfirmAccepted(); + const adapter = buildAdapter(); + const { attachment } = await runAdd(adapter, pngFile()); + + await expect(adapter.send(attachment)).resolves.toMatchObject({ + status: { type: 'complete' }, + }); + + await expect(adapter.send(attachment)).resolves.toMatchObject({ + status: { type: 'complete' }, + }); + }); + + it('still deletes a sent file server-side when it is removed', async () => { + mockConfirmAccepted(); + const adapter = buildAdapter(); + const { attachment } = await runAdd(adapter, pngFile()); + await adapter.send(attachment); + + await adapter.remove(attachment); + + // The DELETE must reach the service: a file dropped from the composer cannot be left in + // the session queue, where ticket creation would still attach it. + const calls = (global.fetch as jest.Mock).mock.calls.map((call) => ({ + url: String(call[0]), + method: (call[1] as RequestInit | undefined)?.method, + })); + expect( + calls.some( + (call) => + call.method === 'DELETE' && call.url.includes('/files/'), + ), + ).toBe(true); + }); +}); diff --git a/packages/assistant-chat/src/files/attachmentAdapter.ts b/packages/assistant-chat/src/files/attachmentAdapter.ts index 7ebe4f820e..9f1753d4cb 100644 --- a/packages/assistant-chat/src/files/attachmentAdapter.ts +++ b/packages/assistant-chat/src/files/attachmentAdapter.ts @@ -46,6 +46,18 @@ interface IAdapterEntry { * Server identifier of the queued file once confirmed, used to delete it again. */ serverId?: string; + /** + * Set when the upload was rejected (validation, malware scan) or failed. The entry is kept + * so `send` can refuse to attach a file the server does not hold, but it no longer occupies + * a composer slot. + */ + uploadError?: unknown; + /** + * Set once the file has ridden along with a message. Kept (rather than dropping the entry) so + * a repeated `send` is idempotent: the composer re-sends every attachment when one of them + * fails, and a file that already went must not look like one that never uploaded. + */ + sent?: boolean; } const toAttachmentType = (file: File): string => @@ -59,14 +71,28 @@ const toFileDataUrl = (file: File): Promise => reader.readAsDataURL(file); }); -const toUploadErrorText = (error: unknown): string => - // Service rejections (magic-byte validation, session limits) carry a human-readable message; - // anything else (network failures, unexpected shapes) falls back to the generic wording. - error instanceof UploadFileError && - error.code !== 'network' && - error.code !== 'internal' - ? error.message - : chatCopy.fileAlerts.uploadFailed; +const toUploadErrorText = (error: unknown): string => { + if (!(error instanceof UploadFileError)) { + return chatCopy.fileAlerts.uploadFailed; + } + + // Scan outcomes get our own wording: the service message describes what was detected, which + // is neither actionable for the user nor safe to echo verbatim into the chat. + if (error.code === 'malicious_file') { + return chatCopy.fileAlerts.maliciousFile; + } + + if (error.code === 'scan_unavailable') { + return chatCopy.fileAlerts.scanUnavailable; + } + + // Other service rejections (magic-byte validation, session limits) carry a human-readable + // message; anything else (network failures, unexpected shapes) falls back to the generic + // wording. + return error.code === 'network' || error.code === 'internal' + ? chatCopy.fileAlerts.uploadFailed + : error.message; +}; /** * assistant-ui attachment adapter over the widget's file endpoints. Files travel out-of-band: the @@ -85,8 +111,12 @@ export const createAttachmentAdapter = ( // Files sitting in the composer: entries are dropped again when their message is sent, so the // cap applies per message, not per session. const usedSlots = (sessionId: string): number => - [...entries.values()].filter((entry) => entry.sessionId === sessionId) - .length; + [...entries.values()].filter( + (entry) => + entry.sessionId === sessionId && + entry.uploadError == null && + !entry.sent, + ).length; return { accept: attachmentAccept, @@ -129,7 +159,10 @@ export const createAttachmentAdapter = ( entry.handle = undefined; }) .catch((error: unknown) => { - entries.delete(id); + // Kept (not deleted) so `send` can tell a rejected file apart from one that + // was never picked: the tile stays in the composer showing its error, and the + // message cannot be sent until the user removes it. + entry.uploadError = error; throw error; }); @@ -185,13 +218,30 @@ export const createAttachmentAdapter = ( entries.delete(attachment.id); }, send: async (attachment: PendingAttachment) => { - // Sending while the upload is still in flight simply waits for it; a failed upload - // keeps the composer intact so the user can remove the broken tile. - await entries.get(attachment.id)?.uploadPromise; + const entry = entries.get(attachment.id); - // The message takes the file with it: the entry no longer occupies a composer slot, - // and the server queue (bounded by its own session cap) holds it for the ticket. - entries.delete(attachment.id); + // The server holds bytes only for uploads it accepted. A rejected one (unsupported + // type, session limit, malware scan) must never ride along with the message: the + // send fails so the user removes the tile first, instead of the transcript showing + // an attachment the support team will never receive. + if (entry == null) { + throw new Error(chatCopy.fileAlerts.uploadFailed); + } + + // Sending while the upload is still in flight waits for it; a rejection landing at + // this point fails the send for the same reason. + try { + await entry.uploadPromise; + } catch (error) { + throw new Error(toUploadErrorText(error)); + } + + // The message takes the file with it, so the entry stops occupying a composer slot — + // but it is marked rather than dropped: when one attachment of a message fails, the + // composer restores them all and re-sends, and a dropped entry would then look like a + // file that never uploaded (blocking the message, and letting `remove` skip the + // server-side deletion so a removed file still reached the ticket). + entry.sent = true; // The content part exists only for the local transcript: assistant-ui rebuilds the // sent message's attachment tiles from it, the chat transport strips file parts from diff --git a/packages/assistant-contracts/src/error.ts b/packages/assistant-contracts/src/error.ts index f343ba6937..49020daf04 100644 --- a/packages/assistant-contracts/src/error.ts +++ b/packages/assistant-contracts/src/error.ts @@ -10,6 +10,10 @@ export const assistantErrorCodeSchema = z.enum([ 'file_too_large', 'file_limit', 'unsupported_file', + // The malware scan flagged the file: it was deleted and never reaches the ticket queue. + 'malicious_file', + // A mandatory scan engine could not deliver a verdict (outage, timeout): retriable. + 'scan_unavailable', 'internal', ]);