diff --git a/apps/client/src/features/ai-chat/components/message-item.tsx b/apps/client/src/features/ai-chat/components/message-item.tsx index 130b15dc..878477b4 100644 --- a/apps/client/src/features/ai-chat/components/message-item.tsx +++ b/apps/client/src/features/ai-chat/components/message-item.tsx @@ -47,6 +47,13 @@ interface MessageItemProps { * agent's raw query/argument text. */ showInput?: boolean; + /** + * Forwarded to ToolCallCard: whether a failed tool card renders its raw + * errorText. Defaults to true (internal chat). The public share passes false so + * internal detail in a tool error is never painted (belt to the server-side + * byte sanitization). + */ + showErrors?: boolean; /** * Neutralize internal/relative markdown links in the rendered answer (drop * their href so they become inert text). Defaults to false (internal chat, @@ -125,6 +132,7 @@ function MessageItem({ message, showCitations = true, showInput = true, + showErrors = true, neutralizeInternalLinks = false, assistantName, turnStreaming = false, @@ -219,6 +227,7 @@ function MessageItem({ part={part as unknown as ToolUiPart} showCitations={showCitations} showInput={showInput} + showErrors={showErrors} /> ); } @@ -284,6 +293,7 @@ export function arePropsEqual( prev.signature === next.signature && prev.showCitations === next.showCitations && prev.showInput === next.showInput && + prev.showErrors === next.showErrors && prev.neutralizeInternalLinks === next.neutralizeInternalLinks && prev.assistantName === next.assistantName && // The turn-end flip re-renders every row once (cheap, terminal event) — diff --git a/apps/client/src/features/ai-chat/components/message-list.tsx b/apps/client/src/features/ai-chat/components/message-list.tsx index d5c9c377..a208361f 100644 --- a/apps/client/src/features/ai-chat/components/message-list.tsx +++ b/apps/client/src/features/ai-chat/components/message-list.tsx @@ -32,6 +32,12 @@ interface MessageListProps { * doesn't see the agent's raw query/argument text. */ showInput?: boolean; + /** + * Forwarded to MessageItem -> ToolCallCard: whether a failed tool card renders + * its raw errorText. Defaults to true (internal chat). The public share passes + * false so internal detail in a tool error is never painted. + */ + showErrors?: boolean; /** * Forwarded to MessageItem: neutralize internal/relative markdown links in * the rendered answers (drop their href so they render as inert text). @@ -127,6 +133,7 @@ export default function MessageList({ emptyState, showCitations = true, showInput = true, + showErrors = true, neutralizeInternalLinks = false, assistantName, }: MessageListProps) { @@ -217,6 +224,7 @@ export default function MessageList({ signature={messageSignature(message)} showCitations={showCitations} showInput={showInput} + showErrors={showErrors} neutralizeInternalLinks={neutralizeInternalLinks} assistantName={assistantName} // Turn-level liveness, gated to the TAIL row: only the tail message diff --git a/apps/client/src/features/ai-chat/components/tool-call-card.tsx b/apps/client/src/features/ai-chat/components/tool-call-card.tsx index 30efb19f..faaea631 100644 --- a/apps/client/src/features/ai-chat/components/tool-call-card.tsx +++ b/apps/client/src/features/ai-chat/components/tool-call-card.tsx @@ -30,6 +30,16 @@ interface ToolCallCardProps { * the extra summary line, leaving the card (the action log) intact. */ showInput?: boolean; + /** + * Whether to render the tool's raw errorText on a failed call. Defaults to true + * (the internal chat, where the operator may debug). The public share passes + * false: a tool error string can carry internal detail (an internal page title, + * a stack fragment, a provider message). This is the RENDER gate only — the + * authoritative fix also sanitizes the bytes server-side (see + * PublicShareChatToolsService.forShare), so a share reader never receives raw + * error text over the wire, not just never sees it painted (#394). + */ + showErrors?: boolean; } /** @@ -41,6 +51,7 @@ export default function ToolCallCard({ part, showCitations = true, showInput = true, + showErrors = true, }: ToolCallCardProps) { const { t } = useTranslation(); const toolName = getToolName(part); @@ -74,7 +85,7 @@ export default function ToolCallCard({ )} - {state === "error" && part.errorText && ( + {state === "error" && showErrors && part.errorText && ( {part.errorText} diff --git a/apps/client/src/features/share/components/share-ai-widget.tsx b/apps/client/src/features/share/components/share-ai-widget.tsx index 59b2efb5..6a1930c8 100644 --- a/apps/client/src/features/share/components/share-ai-widget.tsx +++ b/apps/client/src/features/share/components/share-ai-widget.tsx @@ -168,6 +168,10 @@ export default function ShareAiWidget({ // Anonymous reader: suppress the tool-argument summary line so the // agent's raw query/argument text isn't shown on the public share. showInput={false} + // Anonymous reader: never paint a tool's raw errorText (it can carry + // internal detail). This is the render gate; the bytes are also + // sanitized server-side in PublicShareChatToolsService.forShare (#394). + showErrors={false} // Anonymous reader: neutralize internal/relative links in the // assistant's markdown so internal UUIDs/auth-gated routes don't // leak as clickable links (external http(s) links are kept). diff --git a/apps/server/src/core/ai-chat/public-share-chat.error-leak.spec.ts b/apps/server/src/core/ai-chat/public-share-chat.error-leak.spec.ts new file mode 100644 index 00000000..4d6151de --- /dev/null +++ b/apps/server/src/core/ai-chat/public-share-chat.error-leak.spec.ts @@ -0,0 +1,241 @@ +// Break the editor-ext import chain (share.service -> collaboration.util -> +// @docmost/editor-ext -> @tiptap/core) that is unresolvable in this jest env and +// pre-existingly breaks these specs. jsonToMarkdown is never reached in these +// tests (the tools fail before rendering markdown). +jest.mock('../../collaboration/collaboration.util', () => ({ + jsonToMarkdown: () => '', +})); + +import { Logger } from '@nestjs/common'; +import { MockLanguageModelV3, simulateReadableStream } from 'ai/test'; +import { PublicShareChatService } from './public-share-chat.service'; +import { PublicShareChatToolsService } from './tools/public-share-chat-tools.service'; + +/** + * SECURITY integration guard for #394 (commit 5): a tool's or the provider's raw + * error text must NOT leak to an anonymous public-share reader. + * + * The render gate (ToolCallCard showErrors=false) hides the text in the DOM but + * NOT on the wire, so this test asserts on the RAW SSE BYTES the server writes — + * exactly the channel the render gate masks. We drive the real + * PublicShareChatService.stream() with a real share toolset (its underlying + * services mocked to fail) and a mock model, then inspect every byte piped to the + * fake socket. + */ + +// A minimal ServerResponse stand-in that records every written chunk. +class FakeSocket { + chunks: string[] = []; + statusCode = 200; + writableEnded = false; + destroyed = false; + headersSent = false; + writeHead(): this { + this.headersSent = true; + return this; + } + setHeader(): void {} + removeHeader(): void {} + getHeader(): undefined { + return undefined; + } + flushHeaders(): void {} + write(chunk: unknown): boolean { + this.chunks.push( + typeof chunk === 'string' ? chunk : Buffer.from(chunk as never).toString('utf8'), + ); + return true; + } + end(chunk?: unknown): void { + if (chunk) this.write(chunk); + this.writableEnded = true; + } + on(): this { + return this; + } + once(): this { + return this; + } + get body(): string { + return this.chunks.join(''); + } +} + +/** Mock model that issues one getSharePage tool call, then finishes with text. */ +function toolCallingModel(): MockLanguageModelV3 { + let call = 0; + return new MockLanguageModelV3({ + doStream: async () => { + call++; + if (call === 1) { + return { + stream: simulateReadableStream({ + chunks: [ + { type: 'stream-start' as const, warnings: [] }, + { type: 'tool-input-start' as const, id: 't1', toolName: 'getSharePage' }, + { type: 'tool-input-end' as const, id: 't1' }, + { + type: 'tool-call' as const, + toolCallId: 't1', + toolName: 'getSharePage', + input: '{"pageId":"secret-page"}', + }, + { + type: 'finish' as const, + finishReason: { unified: 'tool-calls' as const, raw: 'tool_calls' }, + usage: { + inputTokens: { total: 1, noCache: undefined, cacheRead: undefined, cacheWrite: undefined }, + outputTokens: { total: 1, text: 1, reasoning: undefined }, + }, + }, + ], + }), + }; + } + return { + stream: simulateReadableStream({ + chunks: [ + { type: 'stream-start' as const, warnings: [] }, + { type: 'text-start' as const, id: '1' }, + { type: 'text-delta' as const, id: '1', delta: 'Sorry.' }, + { type: 'text-end' as const, id: '1' }, + { + type: 'finish' as const, + finishReason: { unified: 'stop' as const, raw: 'stop' }, + usage: { + inputTokens: { total: 1, noCache: undefined, cacheRead: undefined, cacheWrite: undefined }, + outputTokens: { total: 1, text: 1, reasoning: undefined }, + }, + }, + ], + }), + }; + }, + }); +} + +/** Mock model whose stream emits a provider error carrying an internal secret. */ +function providerErrorModel(secret: string): MockLanguageModelV3 { + return new MockLanguageModelV3({ + doStream: async () => ({ + stream: simulateReadableStream({ + chunks: [ + { type: 'stream-start' as const, warnings: [] }, + { + type: 'error' as const, + error: { + statusCode: 503, + message: 'Service Unavailable', + responseBody: `upstream ${secret} model=internal-gpt`, + }, + }, + ], + }), + }), + }); +} + +function makeService(toolsService: PublicShareChatToolsService): { + svc: PublicShareChatService; + logSpy: jest.SpyInstance; +} { + const svc = Object.create(PublicShareChatService.prototype); + const logger = new Logger('test'); + const logSpy = jest.spyOn(logger, 'error').mockImplementation(() => undefined); + jest.spyOn(logger, 'warn').mockImplementation(() => undefined); + svc.tools = toolsService; + svc.logger = logger; + svc.tokenBudget = { record: jest.fn().mockResolvedValue(undefined) }; + return { svc, logSpy }; +} + +async function runStream( + svc: PublicShareChatService, + model: MockLanguageModelV3, +): Promise { + const socket = new FakeSocket(); + await svc.stream({ + workspaceId: 'ws1', + shareId: 'share1', + share: { id: 'share1', pageId: 'p1', sharedPage: { id: 'p1', title: 'Docs' } }, + openedPage: null, + messages: [ + { id: 'm1', role: 'user', parts: [{ type: 'text', text: 'read the page' }] } as never, + ], + res: { raw: socket } as never, + signal: new AbortController().signal, + model: model as never, + role: null, + }); + // Let the piped stream drain fully. + await new Promise((r) => setTimeout(r, 300)); + return socket; +} + +describe('public share chat error leak (#394)', () => { + afterEach(() => jest.restoreAllMocks()); + + it('does NOT leak a tool\'s raw internal error to the SSE bytes (generic classified string instead)', async () => { + const SECRET = 'INTERNAL_baseUrl_http://provider.internal:8080/v1'; + const shareService = { + // The canonical boundary throws a RAW internal error (with a secret). + resolveReadableSharePage: jest + .fn() + .mockRejectedValue(new Error(`db failed at ${SECRET} stack@line42`)), + }; + const tools = new PublicShareChatToolsService( + shareService as never, + {} as never, + {} as never, + ); + const { svc } = makeService(tools); + + const socket = await runStream(svc, toolCallingModel()); + + // The tool-output-error frame is present on the wire... + expect(socket.body).toContain('tool-output-error'); + // ...but it carries ONLY the generic classified string — never the secret, + // the raw driver message, or a stack fragment. + expect(socket.body).toContain('The tool could not complete the request.'); + expect(socket.body).not.toContain(SECRET); + expect(socket.body).not.toContain('stack@line42'); + expect(socket.body).not.toContain('db failed'); + }); + + it('passes a SAFE ShareToolError message (page not available) through to the bytes', async () => { + const shareService = { + // Not found in this share -> the tool throws the classified SAFE message. + resolveReadableSharePage: jest.fn().mockResolvedValue(null), + }; + const tools = new PublicShareChatToolsService( + shareService as never, + {} as never, + {} as never, + ); + const { svc } = makeService(tools); + + const socket = await runStream(svc, toolCallingModel()); + expect(socket.body).toContain('tool-output-error'); + expect(socket.body).toContain('not available in this share'); + }); + + it('does NOT leak a provider error (statusCode + response body) to the SSE bytes', async () => { + const SECRET = 'http://provider.internal:8080'; + const tools = new PublicShareChatToolsService( + {} as never, + {} as never, + {} as never, + ); + const { svc, logSpy } = makeService(tools); + + const socket = await runStream(svc, providerErrorModel(SECRET)); + + // The anon sees a fixed classified string, not the provider body/baseUrl/model. + expect(socket.body).toContain('temporarily unavailable'); + expect(socket.body).not.toContain(SECRET); + expect(socket.body).not.toContain('internal-gpt'); + // The FULL provider detail is logged server-side only. + const logged = logSpy.mock.calls.map((c) => String(c[0])).join('\n'); + expect(logged).toContain(SECRET); + }); +}); diff --git a/apps/server/src/core/ai-chat/public-share-chat.service.ts b/apps/server/src/core/ai-chat/public-share-chat.service.ts index a98e738f..236067eb 100644 --- a/apps/server/src/core/ai-chat/public-share-chat.service.ts +++ b/apps/server/src/core/ai-chat/public-share-chat.service.ts @@ -12,7 +12,10 @@ import { AiAgentRoleRepo } from '@docmost/db/repos/ai-agent-roles/ai-agent-roles import { AiAgentRole } from '@docmost/db/types/entity.types'; import { AiService } from '../../integrations/ai/ai.service'; import { AiSettingsService } from '../../integrations/ai/ai-settings.service'; -import { PublicShareChatToolsService } from './tools/public-share-chat-tools.service'; +import { + PublicShareChatToolsService, + ShareToolError, +} from './tools/public-share-chat-tools.service'; import { buildShareSystemPrompt } from './public-share-chat.prompt'; import { roleModelOverride } from './roles/role-model-config'; import { @@ -102,6 +105,30 @@ export function filterShareTranscript(messages: UIMessage[]): UIMessage[] { ); } +/** + * Fixed, classified strings an ANONYMOUS share reader may see when the assistant + * stream fails (#394). These reveal NOTHING about the internal provider, its + * baseUrl, the model name, or the raw response body — unlike describeProviderError + * (which is for the server log / the authenticated operator only). We classify by + * HTTP status where available so the reader still gets a useful hint (retry vs. + * give up) without any internal detail. + */ +export function classifyAnonStreamError(error: unknown): string { + const status = + typeof error === 'object' && error !== null + ? (error as { statusCode?: number }).statusCode + : undefined; + if (status === 429) { + return 'The assistant is receiving too many requests right now. Please try again shortly.'; + } + if (typeof status === 'number' && status >= 500) { + return 'The assistant is temporarily unavailable. Please try again.'; + } + // Any other failure (including a bare connection error with no status): a + // single neutral line. No provider identity, no config, no response body. + return 'The assistant could not complete your request. Please try again.'; +} + /** * Anonymous, read-only AI assistant for a single PUBLIC share tree. * @@ -318,11 +345,28 @@ export class PublicShareChatService { result.pipeUIMessageStreamToResponse(res.raw, { headers: { 'X-Accel-Buffering': 'no' }, onError: (error: unknown) => { - // Reuse the shared formatter so provider error formatting stays - // unified between the log line and the streamed error message — a - // share reader sees 402/429/503 causes consistently with the - // authenticated path. - return describeProviderError(error, 'AI stream error'); + // SECURITY (#394): the string this returns is written verbatim into the + // SSE error frame delivered to an ANONYMOUS reader (for a tool failure + // it becomes the atomic `tool-output-error` frame's errorText; for a + // stream/provider failure, the terminal error frame). + // + // A ShareToolError is already a classified, safe tool message (see + // PublicShareChatToolsService.wrapToolErrors) — pass it through so the + // reader still gets the useful "page not available in this share" hint. + if (error instanceof ShareToolError) { + return error.message; + } + // Anything else is a provider/stream error. describeProviderError + // bundles the provider statusCode AND response body, which can carry the + // internal baseUrl or model name — NEVER expose that to the public. Log + // the full detail server-side only and return a fixed classified string. + this.logger.error( + `Public share chat pipe error: ${describeProviderError( + error, + 'AI stream error', + )}`, + ); + return classifyAnonStreamError(error); }, }); diff --git a/apps/server/src/core/ai-chat/tools/public-share-chat-tools.service.ts b/apps/server/src/core/ai-chat/tools/public-share-chat-tools.service.ts index 2d2da79d..5467234f 100644 --- a/apps/server/src/core/ai-chat/tools/public-share-chat-tools.service.ts +++ b/apps/server/src/core/ai-chat/tools/public-share-chat-tools.service.ts @@ -7,6 +7,22 @@ import { PageRepo } from '@docmost/db/repos/page/page.repo'; import { jsonToMarkdown } from '../../../collaboration/collaboration.util'; import { modelFriendlyInput } from './model-friendly-input'; +/** + * A tool error whose message is DELIBERATELY safe to expose to an anonymous + * share reader (and to the model, for self-correction). Every OTHER thrown error + * is treated as internal and replaced with a generic string by `wrapToolErrors`, + * so a raw exception message — an internal page title, a DB/stack fragment, a + * driver detail — never rides the public UI stream (#394). + */ +export class ShareToolError extends Error {} + +// The only two classified strings an anonymous reader may ever see from a tool +// failure. The specific one keeps the model's self-correction useful ("try a +// different page"); the generic one reveals nothing about the internal fault. +const SHARE_TOOL_ERROR_NOT_AVAILABLE = + 'The requested page is not available in this share.'; +const SHARE_TOOL_ERROR_GENERIC = 'The tool could not complete the request.'; + /** * Isolated, READ-ONLY toolset for the ANONYMOUS public-share assistant. * @@ -44,7 +60,7 @@ export class PublicShareChatToolsService { * are NO write tools, NO comments/history, NO cross-space or external tools. */ forShare(shareId: string, workspaceId: string): Record { - return { + return this.wrapToolErrors({ searchSharePages: tool({ description: 'Search the pages of THIS published documentation share for a ' + @@ -96,7 +112,7 @@ export class PublicShareChatToolsService { execute: async ({ pageId }) => { const id = (pageId ?? '').trim(); if (!id) { - throw new Error('A pageId is required.'); + throw new ShareToolError('A pageId is required.'); } // Resolve via the SINGLE canonical share-access boundary: confirms the // page resolves to THIS share (recursive CTE up the tree, honouring @@ -112,7 +128,7 @@ export class PublicShareChatToolsService { workspaceId, ); if (!resolved) { - throw new Error('That page is not part of this published share.'); + throw new ShareToolError(SHARE_TOOL_ERROR_NOT_AVAILABLE); } const { page } = resolved; @@ -193,6 +209,57 @@ export class PublicShareChatToolsService { } }, }), - }; + }); + } + + /** + * Wrap every tool's `execute` so a THROWN error is sanitized in ONE place — + * closing the byte leak, the render, and the model context at once (#394). + * + * The AI SDK surfaces a tool-execution throw as an atomic `tool-output-error` + * frame on the v6 UI stream whose `errorText` is the thrown message; on the + * public share that frame goes straight to an anonymous reader. Unwrapped, a + * raw exception (an internal page title, a DB/stack fragment, a driver detail) + * would ride that frame verbatim. Here we catch it, LOG the full detail + * server-side only, and re-throw a CLASSIFIED, safe error: the tool's own + * intentional ShareToolError messages pass through (they keep the model's + * self-correction useful), everything else collapses to a generic string. + */ + private wrapToolErrors( + tools: Record, + ): Record { + const wrapped: Record = {}; + for (const [name, t] of Object.entries(tools)) { + const original = t.execute; + if (typeof original !== 'function') { + wrapped[name] = t; + continue; + } + wrapped[name] = { + ...t, + execute: async (args: unknown, options: unknown) => { + try { + return await ( + original as (a: unknown, o: unknown) => Promise + )(args, options); + } catch (err) { + const safe = + err instanceof ShareToolError + ? err.message + : SHARE_TOOL_ERROR_GENERIC; + // Full detail to the server log ONLY — never to the anon. + this.logger.warn( + `Public share tool "${name}" failed: ${ + err instanceof Error ? err.message : String(err) + }`, + ); + // This safe string is ALL that rides the tool-output-error frame, + // becomes model context, and could be rendered — one choke point. + throw new ShareToolError(safe); + } + }, + } as Tool; + } + return wrapped; } }