From dd66a434616edde9f8dd7c7cfabbd961dcac0c74 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Thu, 4 Jun 2026 10:52:06 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=92=BC=20fix:=20Harden=20Shared-Link=20Me?= =?UTF-8?q?ssage=20Sanitization=20with=20an=20Allowlist=20(#13510)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: harden shared-link message sanitization with an allowlist Public shared links built their message payload via `{ ...message }` and `{ ...attachment }` spreads, which exposed internal fields that are never needed by the shared view: - message: endpoint, conversationSignature, clientId, plugin(s), metadata - attachments: filepath, storageKey, metadata, and other internal keys Replace the passthrough with an allowlist so only render-relevant fields (sender, text, content, token/feedback/error flags, and sanitized attachments) are surfaced. Assistant model ids remain anonymized; other model names are omitted rather than disclosed. Also add `tenantId?: string` to ISharedLink, matching the field already read by the shared-link access middleware for multi-tenant deployments. * fix: preserve shared render data; sanitize by denylist (review feedback) Address Codex/Copilot review on the shared-link sanitization: - The tight attachment allowlist dropped tool-call render data. Switch to a denylist of storage/identity-internal fields (filepath, storageKey, user, tenantId, source, metadata, …) so toolCallId, tool payloads (web_search / file_search / etc.), and dimensions are preserved while internals are stripped. - Preserve user-uploaded message.files (previously dropped entirely) via the same denylist sanitizer, so shared links keep uploaded images/documents. - Introduce a dedicated SharedMessage / SharedFile type and use it for SharedMessagesResult.messages instead of casting the allowlisted object to IMessage, so omitted fields are caught at compile time. Extends the regression test to assert toolCallId, web_search payload, and files survive while filepath/storageKey/user/tenantId/metadata are removed. * fix: keep render URLs + skill badges, drop private feedback (review round 2) Address Codex round-2 findings on shared-link sanitization: - filepath is the URL the share renderer loads (Files.tsx uses file.preview ?? file.filepath; image attachments render only when filepath is set). Drop it from the denylist so shared images/downloads still render; storageKey (the raw object key) stays stripped. - Preserve manualSkills / alwaysAppliedSkills so SkillPills still render for skill-assisted turns (non-sensitive UI metadata). - Remove feedback from the shared projection — it is the owner's private rating/notes, is never rendered in the share view, and must not be exposed to anyone holding the share URL. Regression test updated to assert filepath/skills survive and feedback is omitted. * fix: anonymize file ids + preserve message iconURL (review round 3) - Persisted message.files records can carry the original conversationId/messageId. Attachments were already rewritten to the anonymized ids; apply the same rewrite to files so shared user-uploaded files don't expose the real ids. - Preserve message.iconURL (read by Share/MessageIcon.tsx) so shared assistant/ custom-endpoint turns keep their custom avatar instead of falling back to the generic icon. Regression test asserts the shared file's conversationId is the anonymized id and that iconURL survives. --- .../data-schemas/src/methods/share.test.ts | 112 ++++++++++++++- packages/data-schemas/src/methods/share.ts | 128 +++++++++++++++--- packages/data-schemas/src/types/share.ts | 40 +++++- 3 files changed, 256 insertions(+), 24 deletions(-) diff --git a/packages/data-schemas/src/methods/share.test.ts b/packages/data-schemas/src/methods/share.test.ts index 20b4140461..d1ffc78c83 100644 --- a/packages/data-schemas/src/methods/share.test.ts +++ b/packages/data-schemas/src/methods/share.test.ts @@ -40,8 +40,18 @@ describe('Share Methods', () => { text: String, isCreatedByUser: Boolean, model: String, + iconURL: String, + endpoint: String, + conversationSignature: String, + clientId: String, + plugin: mongoose.Schema.Types.Mixed, + metadata: mongoose.Schema.Types.Mixed, + feedback: mongoose.Schema.Types.Mixed, + manualSkills: [String], + alwaysAppliedSkills: [String], parentMessageId: String, attachments: [mongoose.Schema.Types.Mixed], + files: [mongoose.Schema.Types.Mixed], content: [mongoose.Schema.Types.Mixed], }, { timestamps: true }, @@ -340,7 +350,7 @@ describe('Share Methods', () => { expect(msg.messageId).toMatch(/^msg_/); // Should be anonymized with msg_ prefix expect(msg.messageId).not.toBe(messages[0].messageId); // Should be different from original expect(msg.conversationId).toBe(result.conversationId); - expect(msg.user).toBeUndefined(); // User should be removed + expect((msg as Record).user).toBeUndefined(); // User should be removed }); }); @@ -400,6 +410,106 @@ describe('Share Methods', () => { (result?.messages[0].attachments?.[0] as unknown as t.IMessage | undefined)?.conversationId, ).toBe(result?.conversationId); }); + + test('strips storage-internal fields while preserving shared render data', async () => { + const userId = new mongoose.Types.ObjectId().toString(); + const conversationId = `conv_${nanoid()}`; + const shareId = `share_${nanoid()}`; + + const message = await Message.create({ + messageId: `msg_${nanoid()}`, + conversationId, + user: userId, + text: 'safe text', + isCreatedByUser: false, + model: 'gpt-4', + iconURL: 'https://cdn.example.com/icon.png', + endpoint: 'openAI', + conversationSignature: 'signature', + clientId: 'client-id', + plugin: { latest: 'internal' }, + metadata: { codeEnvRef: 'internal-ref' }, + feedback: { rating: 'thumbsDown', tag: { key: 'inaccurate' }, text: 'private note' }, + manualSkills: ['research'], + alwaysAppliedSkills: ['brand-voice'], + files: [ + { + file_id: 'file123', + filename: 'upload.png', + type: 'image/png', + width: 100, + height: 100, + filepath: '/images/upload.png', + conversationId, + user: userId, + tenantId: 'tenant-a', + storageKey: 'private/upload.png', + source: 's3', + }, + ], + attachments: [ + { + toolCallId: 'call_abc', + type: 'web_search', + web_search: { results: [{ title: 'Cited source', link: 'https://example.com' }] }, + filename: 'result.json', + filepath: '/images/result.json', + storageKey: 'private/result.json', + metadata: { codeEnvRef: 'internal-ref' }, + }, + ], + }); + + await SharedLink.create({ + shareId, + conversationId, + user: userId, + messages: [message._id], + }); + + const result = await shareMethods.getSharedMessages(shareId); + const shared = result?.messages[0]; + + expect(shared?.text).toBe('safe text'); + // Custom message icon is render metadata and should be preserved. + expect(shared?.iconURL).toBe('https://cdn.example.com/icon.png'); + // Non-assistant model and internal message fields must not be disclosed. + expect(shared?.model).toBeUndefined(); + expect(shared).not.toHaveProperty('endpoint'); + expect(shared).not.toHaveProperty('conversationSignature'); + expect(shared).not.toHaveProperty('clientId'); + expect(shared).not.toHaveProperty('plugin'); + expect(shared).not.toHaveProperty('metadata'); + // Private owner feedback is not render data and must not leak publicly. + expect(shared).not.toHaveProperty('feedback'); + // Skill badges are non-sensitive UI metadata and should still render. + expect(shared?.manualSkills).toEqual(['research']); + expect(shared?.alwaysAppliedSkills).toEqual(['brand-voice']); + + // User-uploaded files keep their render URL (filepath/preview) but drop storage internals. + const file = shared?.files?.[0]; + expect(file).toMatchObject({ filename: 'upload.png', type: 'image/png' }); + expect(file?.filepath).toBe('/images/upload.png'); + expect(file).not.toHaveProperty('storageKey'); + expect(file).not.toHaveProperty('user'); + expect(file).not.toHaveProperty('tenantId'); + expect(file).not.toHaveProperty('source'); + // The file's conversation id is rewritten to the anonymized id, not the original. + expect(file?.conversationId).toBe(shared?.conversationId); + expect(file?.conversationId).not.toBe(conversationId); + + // Tool-call attachments keep their correlation id, payload, and render URL so + // citations still render, while storage-only fields are removed. + const attachment = shared?.attachments?.[0]; + expect(attachment).toMatchObject({ + toolCallId: 'call_abc', + type: 'web_search', + web_search: { results: [{ title: 'Cited source', link: 'https://example.com' }] }, + filepath: '/images/result.json', + }); + expect(attachment).not.toHaveProperty('storageKey'); + expect(attachment).not.toHaveProperty('metadata'); + }); }); describe('getSharedLinks', () => { diff --git a/packages/data-schemas/src/methods/share.ts b/packages/data-schemas/src/methods/share.ts index ca77cc028c..2fb78bc9a6 100644 --- a/packages/data-schemas/src/methods/share.ts +++ b/packages/data-schemas/src/methods/share.ts @@ -42,7 +42,82 @@ function anonymizeConvo(conversation: Partial & Partial)) { + if (!SENSITIVE_SHARED_FILE_FIELDS.has(key)) { + result[key] = fieldValue; + } + } + + return Object.keys(result).length > 0 ? result : null; +} + +function sanitizeSharedFiles(files: unknown): t.SharedFile[] | undefined { + if (!Array.isArray(files)) { + return undefined; + } + + const sanitized = files + .map(sanitizeSharedFile) + .filter((file): file is t.SharedFile => file != null); + + return sanitized.length > 0 ? sanitized : undefined; +} + +/** + * Only surface a model name when it is an (already-anonymized) assistant id; + * otherwise omit it so the underlying provider/model is not disclosed. + */ +function anonymizeSharedModel(model?: string): string | undefined { + if (!model?.startsWith('asst_')) { + return undefined; + } + return anonymizeAssistantId(model); +} + +/** + * Build the public, anonymized view of shared messages. An allowlist of + * render-relevant fields keeps internal message fields (endpoint, + * conversationSignature, clientId, plugin(s), metadata, etc.) out of the + * payload, while user files and tool-call attachments are sanitized field by + * field so render data (uploaded files, `toolCallId`, search results, generated + * outputs) is preserved without leaking storage internals. + */ +function anonymizeMessages(messages: t.IMessage[], newConvoId: string): t.SharedMessage[] { if (!Array.isArray(messages)) { return []; } @@ -52,34 +127,43 @@ function anonymizeMessages(messages: t.IMessage[], newConvoId: string): t.IMessa const newMessageId = anonymizeMessageId(message.messageId); idMap.set(message.messageId, newMessageId); - type MessageAttachment = { - messageId?: string; - conversationId?: string; - [key: string]: unknown; - }; - - const anonymizedAttachments = (message.attachments as MessageAttachment[])?.map( - (attachment) => { - return { - ...attachment, - messageId: newMessageId, - conversationId: newConvoId, - }; - }, - ); + const attachments = sanitizeSharedFiles(message.attachments)?.map((attachment) => ({ + ...attachment, + messageId: newMessageId, + conversationId: newConvoId, + })); + // Persisted file records can carry the original conversation/message ids; + // rewrite them to the anonymized ids so shared files don't expose them. + const files = sanitizeSharedFiles(message.files)?.map((file) => ({ + ...file, + ...(file.conversationId !== undefined && { conversationId: newConvoId }), + ...(file.messageId !== undefined && { messageId: newMessageId }), + })); + const model = anonymizeSharedModel(message.model); return { - ...message, messageId: newMessageId, parentMessageId: idMap.get(message.parentMessageId || '') || anonymizeMessageId(message.parentMessageId || ''), conversationId: newConvoId, - model: message.model?.startsWith('asst_') - ? anonymizeAssistantId(message.model) - : message.model, - attachments: anonymizedAttachments, - } as t.IMessage; + sender: message.sender, + text: message.text, + content: message.content, + ...(message.iconURL && { iconURL: message.iconURL }), + ...(model && { model }), + isCreatedByUser: message.isCreatedByUser, + createdAt: message.createdAt, + updatedAt: message.updatedAt, + tokenCount: message.tokenCount, + unfinished: message.unfinished, + error: message.error, + finish_reason: message.finish_reason, + ...(message.manualSkills && { manualSkills: message.manualSkills }), + ...(message.alwaysAppliedSkills && { alwaysAppliedSkills: message.alwaysAppliedSkills }), + ...(files && { files }), + ...(attachments && { attachments }), + }; }); } diff --git a/packages/data-schemas/src/types/share.ts b/packages/data-schemas/src/types/share.ts index da1adc6219..3d5de455d1 100644 --- a/packages/data-schemas/src/types/share.ts +++ b/packages/data-schemas/src/types/share.ts @@ -12,12 +12,50 @@ export interface ISharedLink { expiredAt?: Date; createdAt?: Date; updatedAt?: Date; + /** Owning tenant for multi-tenant deployments (read by the shared-link access middleware). */ + tenantId?: string; } export interface ShareServiceError extends Error { code: string; } +/** + * A file or attachment as exposed through a public shared link: storage- and + * identity-internal fields are stripped, but render-relevant data (including + * dynamic tool-call payloads keyed by tool name) is preserved. + */ +export type SharedFile = Record; + +/** + * Public, anonymized projection of a message returned by a shared link. Only + * render-relevant fields are surfaced; internal fields (user, endpoint, + * conversationSignature, clientId, plugin(s), metadata, etc.) are omitted. + */ +export type SharedMessage = Pick< + IMessage, + | 'messageId' + | 'parentMessageId' + | 'conversationId' + | 'sender' + | 'text' + | 'content' + | 'iconURL' + | 'isCreatedByUser' + | 'createdAt' + | 'updatedAt' + | 'tokenCount' + | 'unfinished' + | 'error' + | 'finish_reason' + | 'manualSkills' + | 'alwaysAppliedSkills' +> & { + model?: string; + files?: SharedFile[]; + attachments?: SharedFile[]; +}; + export interface SharedLinksResult { links: Array<{ shareId: string; @@ -31,7 +69,7 @@ export interface SharedLinksResult { export interface SharedMessagesResult { conversationId: string; - messages: Array; + messages: Array; shareId: string; title?: string; createdAt?: Date;