From c4c9677982b75c63d98ebf85b1904e0c341ba957 Mon Sep 17 00:00:00 2001 From: Guilhem Date: Fri, 18 Sep 2026 10:16:10 +0200 Subject: [PATCH 01/25] feat: attach files to a flow chat message (#11185) * feat: attach files to a flow chat message Co-Authored-By: Claude Opus 5 (1M context) * fix: store chat uploads under windmill_uploads and withdraw refused sends cleanly Co-Authored-By: Claude Opus 5 (1M context) * fix: refuse extra files for a single-file input and keep attachments on retry Co-Authored-By: Claude Opus 5 (1M context) * fix: clean up partial upload batches, withdraw a stopped send once, free retry payloads Co-Authored-By: Claude Opus 5 (1M context) * fix: list a chat conversation only once its run starts and require files where the flow does Co-Authored-By: Claude Opus 5 (1M context) * feat: carry uploaded attachments on the pending chat user message Co-Authored-By: Claude Opus 5 (1M context) * fix: route rich-composer file paste through the attachment lanes and tighten comments Co-Authored-By: Claude Opus 5 (1M context) * fix: keep a stopped turn's files for retry and let an explicit media type win Co-Authored-By: Claude Opus 5 (1M context) * fix: refuse chat attachments sent without text and shorten comments Co-Authored-By: Claude Opus 5 (1M context) * fix: resolve the attachments input from real flow input reads, ignoring loop iteration Co-Authored-By: Claude Opus 5 (1M context) * refactor: read flow input references through one parser for the model and attachments controls Co-Authored-By: Claude Opus 5 (1M context) * fix: withdraw an attachment send stopped after its uploads answered Co-Authored-By: Claude Opus 5 (1M context) * fix: discard uploads when a send is stopped as its attachments are announced Co-Authored-By: Claude Opus 5 (1M context) * docs: keep uploadAttachments' doc comment on uploadAttachments Co-Authored-By: Claude Opus 5 (1M context) * refactor: never delete chat uploads from workspace storage Co-Authored-By: Claude Opus 5 (1M context) * test: pin that a failed upload aborts the rest of its batch Co-Authored-By: Claude Opus 5 (1M context) --------- Co-authored-by: Claude Opus 5 (1M context) --- chat-sdk/README.md | 32 +- chat-sdk/src/api.ts | 28 +- chat-sdk/src/attachments.ts | 116 +++++ chat-sdk/src/chat.ts | 102 +++- chat-sdk/src/index.ts | 4 + chat-sdk/src/types.ts | 37 +- chat-sdk/test/attachments.test.ts | 444 ++++++++++++++++++ chat-sdk/test/support.ts | 7 +- .../InputTransformSchemaForm.svelte | 4 +- .../copilot/chat/AIChatDisplay.svelte | 71 ++- .../copilot/chat/AIChatInput.svelte | 220 ++++++++- .../copilot/chat/AIChatManager.svelte.ts | 5 + .../copilot/chat/QueuedMessageChip.svelte | 19 +- .../components/copilot/chat/blobUtils.test.ts | 19 + .../lib/components/copilot/chat/blobUtils.ts | 58 +++ .../components/copilot/chat/chatViewHost.ts | 27 +- .../copilot/chat/messageDraft.svelte.ts | 46 +- .../copilot/chat/messageDraft.test.ts | 31 +- .../flows/content/AiAgentStepInputs.svelte | 4 +- .../flows/conversations/FlowChat.svelte | 3 +- .../conversations/FlowChatInterface.svelte | 36 +- .../agentAttachmentInput.test.ts | 179 +++++++ .../conversations/agentAttachmentInput.ts | 185 ++++++++ .../conversations/agentChatInputs.test.ts | 9 + .../flows/conversations/agentChatInputs.ts | 67 +-- .../conversations/flowChatViewHost.svelte.ts | 211 +++++++-- .../conversations/flowChatViewHost.test.ts | 190 +++++++- .../components/inputTransformEnv.svelte.ts | 29 +- 28 files changed, 1996 insertions(+), 187 deletions(-) create mode 100644 chat-sdk/src/attachments.ts create mode 100644 chat-sdk/test/attachments.test.ts create mode 100644 frontend/src/lib/components/copilot/chat/blobUtils.test.ts create mode 100644 frontend/src/lib/components/copilot/chat/blobUtils.ts create mode 100644 frontend/src/lib/components/flows/conversations/agentAttachmentInput.test.ts create mode 100644 frontend/src/lib/components/flows/conversations/agentAttachmentInput.ts diff --git a/chat-sdk/README.md b/chat-sdk/README.md index 0f1082de45..1d09b68265 100644 --- a/chat-sdk/README.md +++ b/chat-sdk/README.md @@ -183,7 +183,7 @@ await chat.sendMessage('Hello') | `workspace` | Detected inside a raw app. | | `token` | A token, or a function returning one (called before every request, so it can fetch a short-lived token from your backend). Omit it inside a raw app. | | `history` | `'server'`, `'local'` or `'none'`, see [History](#history). Defaults to `'server'` with a viewer session and `'local'` with an explicit `token`. | -| `inputs` | Extra flow inputs sent with every message. `sendMessage(text, { inputs })` adds per-message ones. | +| `inputs` | Extra flow inputs sent with every message. `sendMessage(text, { inputs })` adds per-message ones, and `{ attachments, attachmentsInput }` files, see [Attachments](#attachments). | | `storageKey` | Namespace for `local` history, e.g. the signed-in user's id. Local history is per browser and per flow; without it, users sharing a browser share it. | | `fetch`, `storage` | Replacements for the globals, for tests and unusual runtimes. | | `pageSize` | Messages and conversations per page of server history. Default 50. | @@ -226,8 +226,8 @@ A turn goes `submitted` (the flow is queued) → `streaming` (the answer is arri answer, an `assistant` message with `success: false`. `status: 'error'` (with `error` set) means the turn could not run or be followed at all, such as a refused request. -Methods: `sendMessage(text, { inputs? })`, `stop()`, `newConversation()`, -`selectConversation(id)`, `loadConversations({ page?, perPage?, kind? })`, +Methods: `sendMessage(text, { inputs?, attachments?, attachmentsInput? })`, `stop()`, +`newConversation()`, `selectConversation(id)`, `loadConversations({ page?, perPage?, kind? })`, `deleteConversation(id)`, `renameConversation(id, title)`, `loadOlderMessages()`, `destroy()`. `kind` lists the flow editor's test chats (`'test'`), the deployed flow's own (`'deployed'`, the server's default) or both (`'all'`); each `Conversation` carries @@ -235,6 +235,32 @@ own (`'deployed'`, the server's default) or both (`'all'`); each `Conversation` stops following the current answer; the flow keeps running and, with server history, its answer is there when you come back. +## Attachments + +A flow whose AI agent step reads `user_attachments` from an `s3object[]` (or a single +`s3object`) flow input takes files with a message: + +```ts +await chat.sendMessage('What does this contract say?', { + attachments: [{ name: file.name, data: file }], // a Blob/File, or a `data:` URL + attachmentsInput: { name: 'files', multiple: true } +}) +``` + +Each file is uploaded to the workspace's object storage under +`windmill_uploads/chat///` and handed to the input as `{ s3, filename }` +objects (the object for a single-file input). Once the uploads return, the pending user +message lists them in `attachments`, as `{ input, s3, filename }` references. The name's +extension is corrected to the file's media type for PNG, JPEG and PDF, because the worker +reads the type off the key. +Files need message text to go with them. A failed upload rejects `sendMessage` before any +run starts, and `stop()` during the upload aborts it; both leave the transcript as it was. +The chat never deletes uploads, so files of a send that did not run stay in storage. The +workspace needs object storage set up. With Enterprise advanced storage permissions, the +user needs read and write on `windmill_uploads/*`, which the default rules grant. The upload goes through +`job_helpers`, so a restricted token needs `job_helpers:write`; a sandboxed raw app cannot +request that scope today, so attachments are not available there yet. + ## History Windmill stores every conversation of a chat-mode flow, and each Windmill user sees diff --git a/chat-sdk/src/api.ts b/chat-sdk/src/api.ts index 3ec5b0f4ff..a14e9800a1 100644 --- a/chat-sdk/src/api.ts +++ b/chat-sdk/src/api.ts @@ -230,6 +230,27 @@ export class WindmillChatApi { return (await res.json()) as FlowConversationMessage[] } + /** + * Puts bytes in the workspace's object storage under `fileKey` and returns the key they were + * stored under (the server may rewrite it). Needs the workspace to have object storage set. + */ + async uploadFile( + fileKey: string, + body: Blob, + options: { contentType?: string; signal?: AbortSignal } = {} + ): Promise<{ file_key: string }> { + const query: Record = { file_key: fileKey } + if (options.contentType) query.content_type = options.contentType + const res = await this.#request('job_helpers/upload_s3_file', { + method: 'POST', + query, + raw: body, + contentType: options.contentType || 'application/octet-stream', + signal: options.signal + }) + return (await res.json()) as { file_key: string } + } + async deleteConversation(conversationId: string): Promise { await this.#request(`flow_conversations/delete/${encodeURIComponent(conversationId)}`, { method: 'DELETE' @@ -241,7 +262,11 @@ export class WindmillChatApi { init: { method?: string query?: Record + /** JSON-encoded. */ body?: unknown + /** Sent as is, under `contentType`. */ + raw?: Blob + contentType?: string accept?: string signal?: AbortSignal } = {} @@ -252,13 +277,14 @@ export class WindmillChatApi { const headers: Record = {} if (init.accept) headers['Accept'] = init.accept if (init.body !== undefined) headers['Content-Type'] = 'application/json' + else if (init.raw !== undefined) headers['Content-Type'] = init.contentType ?? 'application/octet-stream' const token = typeof this.#token === 'function' ? await this.#token() : this.#token if (token) headers['Authorization'] = `Bearer ${token}` const res = await this.#fetch(url.toString(), { method: init.method ?? 'GET', headers, - body: init.body === undefined ? undefined : JSON.stringify(init.body), + body: init.body === undefined ? init.raw : JSON.stringify(init.body), // A token must not be paired with ambient cookies; without one, the cookie is // the credential and only rides same-origin requests. credentials: token ? 'omit' : 'same-origin', diff --git a/chat-sdk/src/attachments.ts b/chat-sdk/src/attachments.ts new file mode 100644 index 0000000000..ac5e71c7d3 --- /dev/null +++ b/chat-sdk/src/attachments.ts @@ -0,0 +1,116 @@ +import type { WindmillChatApi } from './api' +import type { AttachmentUpload } from './types' +import { abortError, isAbortError } from './utils' + +/** + * Where a chat's uploads live in the workspace's object storage. Under `windmill_uploads/` + * because the default Enterprise storage permissions grant every user write and read there + * and deny any other top-level prefix: a key outside it is refused for non-admins, both on + * upload and when the agent's job reads the file back. + */ +export const CHAT_UPLOADS_PREFIX = 'windmill_uploads/chat' + +/** What an AI agent step reads out of `user_attachments`. */ +export interface UploadedAttachment { + s3: string + filename: string +} + +/** + * The extension each type must be stored under. The worker reads an attachment's media type + * from the key's extension only (`mime_guess` in `windmill-ai/src/image_handler.rs`, falling + * back to `image/png`), never from the stored content type, so the extension must be true. + */ +const EXTENSION_BY_MEDIA_TYPE: Record = { + 'image/png': 'png', + 'image/jpeg': 'jpg', + 'application/pdf': 'pdf' +} + +/** + * The name an attachment is stored under: the picked name with the extension its media type + * needs, e.g. a `photo.webp` re-encoded to PNG becomes `photo.png`. Other types keep their name. + */ +export function storedAttachmentName(filename: string, mediaType: string): string { + const extension = EXTENSION_BY_MEDIA_TYPE[mediaType] + if (!extension) return filename + const stem = filename.replace(/\.[^./]+$/, '') + return `${stem || filename}.${extension}` +} + +/** The bytes of an attachment as a Blob carrying its media type. */ +export function attachmentBlob(attachment: AttachmentUpload): Blob { + const data = + typeof attachment.data === 'string' + ? dataUrlToBlob(attachment.data, 'application/octet-stream') + : attachment.data + return attachment.mediaType && attachment.mediaType !== data.type + ? new Blob([data], { type: attachment.mediaType }) + : data +} + +function dataUrlToBlob(dataUrl: string, fallbackType: string): Blob { + const comma = dataUrl.indexOf(',') + if (!dataUrl.startsWith('data:') || comma === -1) { + throw new Error('windmill-chat: an attachment given as a string must be a data: URL') + } + const header = dataUrl.slice(5, comma) + const isBase64 = header.endsWith(';base64') + const mediaType = (isBase64 ? header.slice(0, -';base64'.length) : header) || fallbackType + const payload = dataUrl.slice(comma + 1) + if (!isBase64) return new Blob([decodeURIComponent(payload)], { type: mediaType }) + const binary = atob(payload) + const bytes = new Uint8Array(binary.length) + for (let i = 0; i < binary.length; i++) bytes[i] = binary.charCodeAt(i) + return new Blob([bytes], { type: mediaType }) +} + +/** + * Put each attachment in the workspace's object storage and hand back what the agent reads. + * The key's turn prefix and per-file index keep two files with the same name, in this turn or + * an earlier one, from overwriting each other; the name stays the last segment. + */ +export async function uploadAttachments( + api: WindmillChatApi, + attachments: AttachmentUpload[], + turnId: string, + signal?: AbortSignal +): Promise { + const prefix = `${CHAT_UPLOADS_PREFIX}/${turnId}` + // One failed upload aborts the rest. Nothing already stored is deleted: the chat never + // removes objects from the workspace's storage, so a send that does not run leaves them. + if (signal?.aborted) throw abortError() + const batch = new AbortController() + const abortBatch = () => batch.abort() + signal?.addEventListener('abort', abortBatch, { once: true }) + try { + const results = await Promise.allSettled( + attachments.map(async (attachment, index) => { + try { + const blob = attachmentBlob(attachment) + const filename = storedAttachmentName( + attachment.name || `attachment-${index + 1}`, + blob.type + ) + const { file_key } = await api.uploadFile(`${prefix}/${index}/${filename}`, blob, { + contentType: blob.type, + signal: batch.signal + }) + return { s3: file_key, filename } + } catch (e) { + batch.abort() + throw e + } + }) + ) + const reasons = results.flatMap((r) => (r.status === 'rejected' ? [r.reason] : [])) + // A stop that lands once every upload has answered still withdraws the batch. + if (reasons.length === 0 && !signal?.aborted) { + return results.flatMap((r) => (r.status === 'fulfilled' ? [r.value] : [])) + } + // The failure that started it, not the aborts it caused in the other uploads. + throw reasons.find((reason) => !isAbortError(reason)) ?? reasons[0] ?? abortError() + } finally { + signal?.removeEventListener('abort', abortBatch) + } +} diff --git a/chat-sdk/src/chat.ts b/chat-sdk/src/chat.ts index b13fa89173..2e2c52f3df 100644 --- a/chat-sdk/src/chat.ts +++ b/chat-sdk/src/chat.ts @@ -8,6 +8,7 @@ import { import { resolveConfig, type ResolvedConfig } from './config' import { followJob } from './follow' import { createLocalHistory, type LocalHistory } from './history' +import { uploadAttachments } from './attachments' import type { AgentStreamEvent } from './stream' import type { Chat, @@ -15,6 +16,7 @@ import type { ChatOptions, ChatState, Conversation, + SendMessageOptions, ToolInvocation } from './types' import { @@ -22,6 +24,7 @@ import { truncateTitle, errorResultMessage, extractChatAnswer, + abortError, isAbortError, isErrorResult, now, @@ -41,6 +44,12 @@ interface Turn { conversationId: string /** Id of the turn's user message; the answer is whatever follows it. */ userMessageId: string + /** The turn opened the conversation; withdrawing it closes the conversation again. */ + isNew: boolean + /** The run was asked for. Before that, a failure or a stop withdraws the turn instead of failing it. */ + started: boolean + /** Both `stop()` and the send's own rejection withdraw; only the first may. */ + withdrawn: boolean jobId?: string /** The flow job and its step jobs; a persisted answer carries one of them as `job_id`. */ jobIds?: Set @@ -101,21 +110,34 @@ class ChatImpl implements Chat { } } - sendMessage = async ( - text: string, - options: { inputs?: Record } = {} - ): Promise => { + sendMessage = async (text: string, options: SendMessageOptions = {}): Promise => { const content = text.trim() - if (!content) return + if (!content) { + // A run needs a message; files alone would otherwise be dropped without a word. + if (options.attachments?.length) throw new Error('windmill-chat: attachments need a message to go with them') + return + } if (this.#turn) { throw new Error('windmill-chat: a message is already being answered; call stop() first') } + const attachments = options.attachments ?? [] + const attachmentsInput = options.attachmentsInput + if (attachments.length > 0 && !attachmentsInput) { + throw new Error('windmill-chat: attachments need `attachmentsInput`, the flow input that takes them') + } + if (attachmentsInput && !attachmentsInput.multiple && attachments.length > 1) { + // Uploading all of them would run with the first and leave the rest stranded in storage. + throw new Error(`windmill-chat: \`${attachmentsInput.name}\` holds one file; got ${attachments.length}`) + } const isNew = this.#state.conversationId === undefined const conversationId = this.#state.conversationId ?? randomId() const turn: Turn = { controller: new AbortController(), conversationId, userMessageId: `pending-${randomId()}`, + isNew, + started: false, + withdrawn: false, streamedText: false } this.#turn = turn @@ -127,7 +149,6 @@ class ChatImpl implements Chat { const touched = { ...conversation, updatedAt: timestamp } this.#set({ conversationId, - conversations: [touched, ...this.#state.conversations.filter((c) => c.id !== conversationId)], messages: [ ...this.#state.messages, { id: turn.userMessageId, role: 'user', content, success: true, createdAt: timestamp, pending: true } @@ -135,10 +156,32 @@ class ChatImpl implements Chat { status: 'submitted', error: undefined }) - this.#rememberConversation() try { - const args = { ...this.#config.inputs, ...options.inputs, user_message: content } + const args: Record = { ...this.#config.inputs, ...options.inputs, user_message: content } + if (attachmentsInput && attachments.length > 0) { + // Uploaded with the turn already shown as submitted: the message is in the transcript + // and `stop()` can abort the upload, while a second send is refused as usual. + const uploaded = await uploadAttachments(this.#api, attachments, randomId(), turn.controller.signal) + args[attachmentsInput.name] = attachmentsInput.multiple ? uploaded : uploaded[0] + // Shown on the pending message until its server row replaces it, carrying its own. + const carried = uploaded.map((u) => ({ input: attachmentsInput.name, s3: u.s3, filename: u.filename })) + if (this.#turnActive(turn)) { + this.#set({ + messages: this.#state.messages.map((m) => (m.id === turn.userMessageId ? { ...m, attachments: carried } : m)) + }) + } + } + // Nothing may start once stop() or a conversation switch has withdrawn the turn, including + // a stop from a subscriber told of the attachments just above. + if (turn.controller.signal.aborted) throw abortError() + turn.started = true + // Listed only once the run is asked for: a send that never runs (an upload that failed + // or was stopped) then has no conversation entry to take back. + this.#set({ + conversations: [touched, ...this.#state.conversations.filter((c) => c.id !== conversationId)] + }) + this.#rememberConversation() const context = { memoryId: conversationId, conversationId, signal: turn.controller.signal } turn.jobId = this.#config.run ? await this.#config.run(args, context) @@ -152,6 +195,13 @@ class ChatImpl implements Chat { } await this.#finishTurn(turn, result, isNew) } catch (e) { + if (!turn.started) { + // Nothing ran: the message is withdrawn rather than shown as a failed turn, and the + // caller gets the reason (an upload that failed, or the AbortError of a stop()). + if (this.#turn === turn) this.#turn = undefined + this.#withdrawTurn(turn) + throw e + } // stop() and a conversation switch abort the turn and settle the state themselves. if (turn.controller.signal.aborted || isAbortError(e)) return this.#failTurn(turn, e) @@ -164,6 +214,12 @@ class ChatImpl implements Chat { const turn = this.#turn if (!turn) return this.#detachTurn() + if (!turn.started) { + // Still uploading its attachments: there is no run to cancel, and the message the + // reader took back must not stay in the transcript as sent. + this.#withdrawTurn(turn) + return + } if (this.#state.conversationId === turn.conversationId) { this.#set({ messages: finalized(this.#state.messages), status: 'idle' }) this.#persistLocal() @@ -578,6 +634,36 @@ class ChatImpl implements Chat { } } + /** + * Take back the user message of a turn that never ran. A conversation it would have opened + * was never listed (see `sendMessage`), so only the message goes, and, while it is the turn + * on screen, the busy status. A switch away mid-upload has already written the message to + * local history, so it is removed there too. + */ + #withdrawTurn(turn: Turn): void { + if (turn.withdrawn) return + turn.withdrawn = true + const id = turn.conversationId + const withoutTurn = (messages: ChatMessage[]) => messages.filter((m) => m.id !== turn.userMessageId) + if (this.#state.conversationId === id) { + const messages = withoutTurn(this.#state.messages) + // A turn started since, such as a resend right after Stop, owns the status. + const newerTurn = this.#turn !== undefined && this.#turn !== turn + if (newerTurn) { + this.#set({ messages }) + } else { + const unopened = turn.isNew && messages.length === 0 + this.#set({ messages, status: 'idle', error: undefined, ...(unopened ? { conversationId: undefined } : {}) }) + } + this.#persistLocal() + } + if (this.#state.history === 'local' && this.#state.conversationId !== id) { + const stored = withoutTurn(this.#local.getMessages(id)) + if (stored.length > 0) this.#local.saveMessages(id, stored) + else if (!this.#state.conversations.some((c) => c.id === id)) this.#local.deleteConversation(id) + } + } + #failTurn(turn: Turn, e: unknown): void { if (!this.#turnActive(turn)) return const error = toError(e) diff --git a/chat-sdk/src/index.ts b/chat-sdk/src/index.ts index aedaa8b19f..9ed2ed0b7e 100644 --- a/chat-sdk/src/index.ts +++ b/chat-sdk/src/index.ts @@ -14,7 +14,10 @@ export { export { parseStreamEvents, createStreamEventParser, type AgentStreamEvent } from './stream' export { followJob, type FollowEvent } from './follow' export { extractChatAnswer, conversationIdFor } from './utils' +export { storedAttachmentName, uploadAttachments, CHAT_UPLOADS_PREFIX, type UploadedAttachment } from './attachments' export type { + AttachmentsInput, + AttachmentUpload, Chat, ChatAttachment, ChatMessage, @@ -25,6 +28,7 @@ export type { Conversation, FetchLike, HistoryMode, + SendMessageOptions, StorageLike, TokenSource, ToolInvocation diff --git a/chat-sdk/src/types.ts b/chat-sdk/src/types.ts index 87fd5a7b0a..99e16fe304 100644 --- a/chat-sdk/src/types.ts +++ b/chat-sdk/src/types.ts @@ -127,12 +127,45 @@ export interface ChatOptions { onError?: (error: Error, turn: { conversationId: string; jobId?: string }) => void } +/** A file sent with a message. It is uploaded to the workspace's object storage before the run starts. */ +export interface AttachmentUpload { + /** Kept as the last segment of the stored key, its extension corrected to the media type for PNG, JPEG and PDF. */ + name: string + /** The bytes: a Blob, or a `data:` URL of them. */ + data: Blob | string + /** The file's media type. Defaults to the Blob's own type, or the data URL's. */ + mediaType?: string +} + +/** The flow input the uploaded attachments are handed to: an `s3object` (`multiple: false`) or an `s3object[]`. */ +export interface AttachmentsInput { + name: string + multiple: boolean +} + +export interface SendMessageOptions { + /** Extra flow inputs for this message, on top of `ChatOptions.inputs`. */ + inputs?: Record + /** + * Files to upload and hand to the flow as `{ s3, filename }` objects in `attachmentsInput`, + * the way an AI agent step reads `user_attachments`. A failed upload rejects `sendMessage` + * and the run never starts; `stop()` during the upload does the same with an `AbortError`. + */ + attachments?: AttachmentUpload[] + /** Required with `attachments`, which also need message text. With `multiple: false`, more than one attachment is refused before anything uploads. */ + attachmentsInput?: AttachmentsInput +} + export interface Chat { getState(): ChatState /** Calls `listener` now and on every change; returns the unsubscribe function (Svelte store contract). */ subscribe(listener: (state: ChatState) => void): () => void - /** Sends a message in the current conversation, starting one when there is none. Resolves when the answer is complete. */ - sendMessage(text: string, options?: { inputs?: Record }): Promise + /** + * Sends a message in the current conversation, starting one when there is none. Resolves + * when the answer is complete. Rejects when the message could not be sent at all — a turn + * already running, an attachment that failed to upload — without touching the transcript. + */ + sendMessage(text: string, options?: SendMessageOptions): Promise /** Stops following the answer and asks Windmill to cancel the run. */ stop(): Promise newConversation(): void diff --git a/chat-sdk/test/attachments.test.ts b/chat-sdk/test/attachments.test.ts new file mode 100644 index 0000000000..9c2f577567 --- /dev/null +++ b/chat-sdk/test/attachments.test.ts @@ -0,0 +1,444 @@ +import { describe, expect, test } from 'bun:test' +import { WindmillChatApi } from '../src/api' +import { storedAttachmentName, uploadAttachments } from '../src/attachments' +import { createChat } from '../src/chat' +import type { ChatOptions } from '../src/types' +import { abortError } from '../src/utils' +import { fetchMock, json, memoryStorage, sse, text, type RecordedCall, type Route } from './support' + +const BASE = 'http://wm.test' +const FLOW = 'f/chat/agent' +const UPLOAD_PATH = '/api/w/ws/job_helpers/upload_s3_file' + +const run: Route = (c) => + c.method === 'POST' && c.url.pathname === `/api/w/ws/jobs/run/f/${FLOW}` + ? text('job-1') + : undefined + +/** Stores under the key it was asked to, like the server with a `file_key`. */ +const upload: Route = (c) => + c.url.pathname === UPLOAD_PATH + ? json({ file_key: c.url.searchParams.get('file_key') }) + : undefined + +const answer: Route = (c) => + c.url.pathname === '/api/w/ws/jobs_u/getupdate_sse/job-1' + ? sse([ + { + type: 'update', + completed: true, + only_result: { output: 'ok', messages: [] } + } + ]) + : undefined + +function options(fetch: ChatOptions['fetch']): ChatOptions { + return { + flowPath: FLOW, + baseUrl: BASE, + workspace: 'ws', + token: 'tok', + fetch, + storage: memoryStorage() + } +} + +const uploads = (calls: RecordedCall[]) => calls.filter((c) => c.url.pathname === UPLOAD_PATH) +const runs = (calls: RecordedCall[]) => + calls.filter((c) => c.url.pathname.startsWith('/api/w/ws/jobs/run/')) + +const png = new Blob([new Uint8Array([0x89, 0x50, 0x4e, 0x47])], { + type: 'image/png' +}) +const pdf = new Blob(['%PDF-1.7'], { type: 'application/pdf' }) + +describe('storedAttachmentName', () => { + // The worker reads the media type from the key's extension, so it has to match the bytes. + test('renames a re-encoded image and gives a bare name its extension', () => { + expect(storedAttachmentName('photo.webp', 'image/png')).toBe('photo.png') + expect(storedAttachmentName('holiday.png', 'image/jpeg')).toBe('holiday.jpg') + expect(storedAttachmentName('contract', 'application/pdf')).toBe('contract.pdf') + expect(storedAttachmentName('report.2026.final.webp', 'image/png')).toBe( + 'report.2026.final.png' + ) + }) + + test('leaves a type it does not know alone', () => { + expect(storedAttachmentName('notes.csv', 'text/csv')).toBe('notes.csv') + }) +}) + +describe('sendMessage with attachments', () => { + test('uploads each file under the turn prefix and hands the list to the input', async () => { + const { fetch, calls } = fetchMock(upload, run, answer) + const chat = createChat(options(fetch)) + + await chat.sendMessage('read these', { + inputs: { locale: 'fr' }, + attachments: [ + { name: 'photo.webp', data: png }, + { name: 'contract', data: pdf }, + // A data URL is decoded to its bytes; the mediaType names what they are. + { + name: 'photo.webp', + data: `data:image/png;base64,${btoa('\x89PNG')}` + } + ], + attachmentsInput: { name: 'files', multiple: true } + }) + + const keys = uploads(calls).map((c) => c.url.searchParams.get('file_key')!) + expect(keys).toHaveLength(3) + const prefix = keys[0].split('/').slice(0, 3).join('/') + expect(prefix).toMatch(/^windmill_uploads\/chat\/[0-9a-f-]{36}$/) + expect(keys).toEqual([ + `${prefix}/0/photo.png`, + `${prefix}/1/contract.pdf`, + `${prefix}/2/photo.png` + ]) + expect(uploads(calls).map((c) => c.url.searchParams.get('content_type'))).toEqual([ + 'image/png', + 'application/pdf', + 'image/png' + ]) + expect(uploads(calls).map((c) => c.headers['content-type'])).toEqual([ + 'image/png', + 'application/pdf', + 'image/png' + ]) + expect(new Uint8Array(await uploads(calls)[2].raw!.arrayBuffer())).toEqual( + new Uint8Array([0x89, 0x50, 0x4e, 0x47]) + ) + + expect(runs(calls)[0].body).toEqual({ + locale: 'fr', + user_message: 'read these', + files: [ + { s3: `${prefix}/0/photo.png`, filename: 'photo.png' }, + { s3: `${prefix}/1/contract.pdf`, filename: 'contract.pdf' }, + { s3: `${prefix}/2/photo.png`, filename: 'photo.png' } + ] + }) + expect(chat.getState().status).toBe('idle') + }) + + test('hands a single object to an input that holds one file', async () => { + const { fetch, calls } = fetchMock(upload, run, answer) + const chat = createChat(options(fetch)) + await chat.sendMessage('read this', { + attachments: [{ name: 'contract.pdf', data: pdf }], + attachmentsInput: { name: 'file', multiple: false } + }) + const body = runs(calls)[0].body as Record + expect(body.file).toEqual({ + s3: expect.stringMatching(/\/0\/contract\.pdf$/), + filename: 'contract.pdf' + }) + }) + + test('refuses several files for an input that holds one, before uploading any', async () => { + const { fetch, calls } = fetchMock(upload, run, answer) + const chat = createChat(options(fetch)) + await expect( + chat.sendMessage('read these', { + attachments: [ + { name: 'a.pdf', data: pdf }, + { name: 'b.png', data: png } + ], + attachmentsInput: { name: 'file', multiple: false } + }) + ).rejects.toThrow('holds one file') + expect(calls).toHaveLength(0) + expect(chat.getState().messages).toEqual([]) + }) + + test('refuses attachments without message text, before uploading', async () => { + const { fetch, calls } = fetchMock(upload, run, answer) + const chat = createChat(options(fetch)) + await expect( + chat.sendMessage(' ', { + attachments: [{ name: 'a.pdf', data: pdf }], + attachmentsInput: { name: 'files', multiple: true } + }) + ).rejects.toThrow('need a message') + expect(calls).toHaveLength(0) + }) + + test('refuses attachments without an input to put them in', async () => { + const { fetch, calls } = fetchMock(upload, run, answer) + const chat = createChat(options(fetch)) + await expect( + chat.sendMessage('hi', { attachments: [{ name: 'a.pdf', data: pdf }] }) + ).rejects.toThrow('attachmentsInput') + expect(calls).toHaveLength(0) + }) + + test('a failed upload rejects without a run, and withdraws the message', async () => { + const { fetch, calls } = fetchMock( + (c) => (c.url.pathname === UPLOAD_PATH ? text('no object storage', 500) : undefined), + run, + answer + ) + const chat = createChat(options(fetch)) + const statuses: string[] = [] + chat.subscribe((s) => statuses.push(s.status)) + + await expect( + chat.sendMessage('read this', { + attachments: [{ name: 'contract.pdf', data: pdf }], + attachmentsInput: { name: 'files', multiple: true } + }) + ).rejects.toThrow('no object storage') + + expect(runs(calls)).toHaveLength(0) + // Shown as submitted while uploading, then withdrawn whole: no message, no conversation. + expect(statuses).toContain('submitted') + const state = chat.getState() + expect(state.status).toBe('idle') + expect(state.messages).toEqual([]) + expect(state.conversationId).toBeUndefined() + expect(state.conversations).toEqual([]) + // The chat is free for the next message. + await chat.sendMessage('plain') + expect(runs(calls)).toHaveLength(1) + }) + + test('stop() during the upload aborts it and withdraws the message', async () => { + const { fetch, calls } = fetchMock( + (c) => + c.url.pathname === UPLOAD_PATH + ? new Promise((_, reject) => + c.signal!.addEventListener('abort', () => reject(abortError())) + ) + : undefined, + run, + answer + ) + const chat = createChat(options(fetch)) + const sending = chat.sendMessage('read this', { + attachments: [{ name: 'contract.pdf', data: pdf }], + attachmentsInput: { name: 'files', multiple: true } + }) + await new Promise((r) => setTimeout(r, 0)) + expect(chat.getState().status).toBe('submitted') + await chat.stop() + await expect(sending).rejects.toMatchObject({ name: 'AbortError' }) + expect(runs(calls)).toHaveLength(0) + expect(chat.getState()).toMatchObject({ + status: 'idle', + messages: [], + conversationId: undefined + }) + }) + + test('a switch away mid-upload leaves no conversation behind', async () => { + const storage = memoryStorage() + const { fetch, calls } = fetchMock( + (c) => + c.url.pathname === UPLOAD_PATH + ? new Promise((_, reject) => + c.signal!.addEventListener('abort', () => reject(abortError())) + ) + : undefined, + run, + answer + ) + const chat = createChat({ ...options(fetch), storage }) + const sending = chat.sendMessage('never runs', { + attachments: [{ name: 'contract.pdf', data: pdf }], + attachmentsInput: { name: 'files', multiple: true } + }) + await new Promise((r) => setTimeout(r, 0)) + const opened = chat.getState().conversationId! + chat.newConversation() + await expect(sending).rejects.toMatchObject({ name: 'AbortError' }) + + expect(runs(calls)).toHaveLength(0) + expect(chat.getState().conversations.map((c) => c.id)).not.toContain(opened) + const reloaded = createChat({ ...options(fetch), storage }) + expect((await reloaded.loadConversations()).map((c) => c.id)).not.toContain(opened) + }) + + test('a failed upload aborts the rest of its batch and deletes nothing', async () => { + let first: (r: Response) => void = () => {} + const { fetch, calls } = fetchMock( + (c) => { + if (c.url.pathname !== UPLOAD_PATH) return undefined + const key = c.url.searchParams.get('file_key')! + // The first file lands after the second has already failed. + if (key.includes('/0/')) return new Promise((resolve) => (first = resolve)) + setTimeout(() => first(json({ file_key: keys()[0] })), 5) + return text('quota exceeded', 507) + }, + run, + answer + ) + const keys = () => uploads(calls).map((c) => c.url.searchParams.get('file_key')!) + const chat = createChat(options(fetch)) + await expect( + chat.sendMessage('read these', { + attachments: [ + { name: 'a.pdf', data: pdf }, + { name: 'b.png', data: png } + ], + attachmentsInput: { name: 'files', multiple: true } + }) + ).rejects.toThrow('quota exceeded') + // The upload still in flight when the other failed was told to stop. + expect(uploads(calls)[0].signal?.aborted).toBe(true) + expect(calls.filter((c) => c.method === 'DELETE')).toEqual([]) + expect(runs(calls)).toHaveLength(0) + }) + + test('a send made right after stop() is not reset by the stopped upload', async () => { + let releaseRun: (r: Response) => void = () => {} + const { fetch, calls } = fetchMock( + (c) => + c.url.pathname === UPLOAD_PATH + ? new Promise((_, reject) => + c.signal!.addEventListener('abort', () => reject(abortError())) + ) + : undefined, + (c) => + c.method === 'POST' && c.url.pathname === `/api/w/ws/jobs/run/f/${FLOW}` + ? new Promise((resolve) => (releaseRun = resolve)) + : undefined, + answer + ) + const chat = createChat(options(fetch)) + // An existing conversation, so the stopped turn and the next one share it. + const first = chat.sendMessage('first') + await new Promise((r) => setTimeout(r, 0)) + releaseRun(text('job-1')) + await first + const stopped = chat.sendMessage('with a file', { + attachments: [{ name: 'a.pdf', data: pdf }], + attachmentsInput: { name: 'files', multiple: true } + }) + await new Promise((r) => setTimeout(r, 0)) + void chat.stop() + const next = chat.sendMessage('right after') + await expect(stopped).rejects.toMatchObject({ name: 'AbortError' }) + await new Promise((r) => setTimeout(r, 0)) + expect(chat.getState().status).toBe('submitted') + expect(chat.getState().messages.map((m) => m.content)).toContain('right after') + expect(chat.getState().messages.map((m) => m.content)).not.toContain('with a file') + releaseRun(text('job-1')) + await next + expect(runs(calls)).toHaveLength(2) + }) + + test('a conversation is listed only once its run starts', async () => { + let failUpload: (r: Response) => void = () => {} + const { fetch } = fetchMock( + (c) => + c.url.pathname === UPLOAD_PATH + ? new Promise((resolve) => (failUpload = resolve)) + : undefined, + run, + answer + ) + const chat = createChat(options(fetch)) + const sending = chat.sendMessage('read this', { + attachments: [{ name: 'a.pdf', data: pdf }], + attachmentsInput: { name: 'files', multiple: true } + }) + await new Promise((r) => setTimeout(r, 0)) + expect(chat.getState()).toMatchObject({ status: 'submitted', conversations: [] }) + failUpload(text('boom', 500)) + await expect(sending).rejects.toThrow('boom') + expect(chat.getState().conversations).toEqual([]) + }) + + test('an already aborted signal uploads nothing', async () => { + const { fetch, calls } = fetchMock(upload) + const api = new WindmillChatApi({ baseUrl: BASE, workspace: 'ws', token: 'tok', fetch }) + const controller = new AbortController() + controller.abort() + await expect( + uploadAttachments(api, [{ name: 'a.pdf', data: pdf }], 'turn', controller.signal) + ).rejects.toMatchObject({ name: 'AbortError' }) + expect(calls).toHaveLength(0) + }) + + test('the pending user message carries its uploaded files before the run returns', async () => { + let releaseRun: (r: Response) => void = () => {} + const { fetch, calls } = fetchMock( + upload, + (c) => + c.method === 'POST' && c.url.pathname === `/api/w/ws/jobs/run/f/${FLOW}` + ? new Promise((resolve) => (releaseRun = resolve)) + : undefined, + answer + ) + const chat = createChat(options(fetch)) + const sending = chat.sendMessage('read these', { + attachments: [ + { name: 'photo.webp', data: png }, + { name: 'contract', data: pdf } + ], + attachmentsInput: { name: 'files', multiple: true } + }) + while (runs(calls).length === 0) await new Promise((r) => setTimeout(r, 1)) + const keys = uploads(calls).map((c) => c.url.searchParams.get('file_key')!) + const pending = chat.getState().messages.find((m) => m.role === 'user')! + expect(pending.pending).toBe(true) + expect(pending.attachments).toEqual([ + { input: 'files', s3: keys[0], filename: 'photo.png' }, + { input: 'files', s3: keys[1], filename: 'contract.pdf' } + ]) + releaseRun(text('job-1')) + await sending + }) + + test('an explicit mediaType wins over the type a data URL declares', async () => { + const { fetch, calls } = fetchMock(upload, run, answer) + const chat = createChat(options(fetch)) + await chat.sendMessage('read this', { + attachments: [ + { + name: 'contract', + data: `data:application/octet-stream;base64,${btoa('%PDF')}`, + mediaType: 'application/pdf' + } + ], + attachmentsInput: { name: 'files', multiple: true } + }) + const call = uploads(calls)[0] + expect(call.url.searchParams.get('file_key')).toMatch(/\/0\/contract\.pdf$/) + expect(call.url.searchParams.get('content_type')).toBe('application/pdf') + }) + + test('stop() after the uploads land but before the run starts runs nothing', async () => { + const { fetch, calls } = fetchMock(upload, run, answer) + const chat = createChat(options(fetch)) + const sending = chat.sendMessage('read this', { + attachments: [{ name: 'contract.pdf', data: pdf }], + attachmentsInput: { name: 'files', multiple: true } + }) + // The upload responds at once; Stop lands before the send resumes after it. + while (uploads(calls).length === 0) await Promise.resolve() + await chat.stop() + await expect(sending).rejects.toMatchObject({ name: 'AbortError' }) + expect(runs(calls)).toHaveLength(0) + expect(calls.filter((c) => c.method === 'DELETE')).toEqual([]) + expect(chat.getState()).toMatchObject({ status: 'idle', messages: [], conversations: [] }) + }) + + test('a subscriber stopping when the attachments appear runs nothing', async () => { + const { fetch, calls } = fetchMock(upload, run, answer) + const chat = createChat(options(fetch)) + chat.subscribe((s) => { + if (s.messages.some((m) => m.attachments)) void chat.stop() + }) + await expect( + chat.sendMessage('read this', { + attachments: [{ name: 'contract.pdf', data: pdf }], + attachmentsInput: { name: 'files', multiple: true } + }) + ).rejects.toMatchObject({ name: 'AbortError' }) + expect(runs(calls)).toHaveLength(0) + expect(calls.filter((c) => c.method === 'DELETE')).toEqual([]) + expect(chat.getState()).toMatchObject({ status: 'idle', messages: [], conversations: [] }) + }) +}) diff --git a/chat-sdk/test/support.ts b/chat-sdk/test/support.ts index c794e50f3d..e198f19355 100644 --- a/chat-sdk/test/support.ts +++ b/chat-sdk/test/support.ts @@ -5,6 +5,9 @@ export interface RecordedCall { url: URL headers: Record body: unknown + /** A body sent as is rather than as JSON (an upload). */ + raw?: Blob + signal?: AbortSignal } export type Route = (call: RecordedCall) => Response | Promise | undefined @@ -20,7 +23,9 @@ export function fetchMock(...routes: Route[]): { fetch: FetchLike; calls: Record headers: Object.fromEntries( Object.entries((init?.headers as Record) ?? {}).map(([k, v]) => [k.toLowerCase(), v]) ), - body: typeof init?.body === 'string' ? JSON.parse(init.body) : undefined + body: typeof init?.body === 'string' ? JSON.parse(init.body) : undefined, + raw: init?.body instanceof Blob ? init.body : undefined, + signal: init?.signal ?? undefined } calls.push(call) for (const route of routes) { diff --git a/frontend/src/lib/components/InputTransformSchemaForm.svelte b/frontend/src/lib/components/InputTransformSchemaForm.svelte index 465e173f1f..31f2eb0054 100644 --- a/frontend/src/lib/components/InputTransformSchemaForm.svelte +++ b/frontend/src/lib/components/InputTransformSchemaForm.svelte @@ -8,7 +8,7 @@ import type { PickableProperties } from './flows/previousResults' import InputTransformForm from './InputTransformForm.svelte' import InputTransformPickers from './InputTransformPickers.svelte' - import { useS3StorageConfigured } from './inputTransformEnv.svelte' + import { useWorkspaceStorageConfigured } from './inputTransformEnv.svelte' import type ItemPicker from './ItemPicker.svelte' import type VariableEditor from './VariableEditor.svelte' import ResizeTransitionWrapper from './common/ResizeTransitionWrapper.svelte' @@ -86,7 +86,7 @@ let itemPicker: ItemPicker | undefined = $state(undefined) let variableEditor: VariableEditor | undefined = $state(undefined) - const s3Storage = useS3StorageConfigured(() => ws) + const s3Storage = useWorkspaceStorageConfigured(() => ws) let keys: string[] = $state([]) $effect(() => { diff --git a/frontend/src/lib/components/copilot/chat/AIChatDisplay.svelte b/frontend/src/lib/components/copilot/chat/AIChatDisplay.svelte index 82dcbc2dcd..0085ca701f 100644 --- a/frontend/src/lib/components/copilot/chat/AIChatDisplay.svelte +++ b/frontend/src/lib/components/copilot/chat/AIChatDisplay.svelte @@ -361,7 +361,15 @@ chatHost.mode === AIMode.SCRIPT || chatHost.mode === AIMode.FLOW || chatHost.mode === AIMode.APP ) - const canAttachFiles = $derived(chatHost.supportsMessageAttachments && !disabled) + // Why attaching is off, when this chat takes attachments but cannot right now. The `+` is + // kept and disabled rather than dropped: the input is the composer's either way, so the + // reader has to be able to see here why nothing can be attached. + const attachmentsOffReason = $derived( + chatHost.supportsMessageAttachments ? chatHost.attachmentsUnavailableReason : undefined + ) + const canAttachFiles = $derived( + chatHost.supportsMessageAttachments && !disabled && !attachmentsOffReason + ) // Folders are linked as session-wide assets, which only a host that reads files in // the browser can do — a host running the turn server-side takes attachments only. const canLinkFolders = $derived(chatHost.supportsLinkedFolders && !disabled) @@ -430,24 +438,32 @@ return Array.from(e.dataTransfer?.types ?? []).includes('Files') } + // A drop is claimed while attaching is off for a stated reason, too: the browser would + // otherwise navigate to the dropped file, and the reader is owed the reason instead. + const panelTakesDrops = $derived(canAttachFiles || attachmentsOffReason !== undefined) + function onPanelDragEnter(e: DragEvent) { - if (!canAttachFiles || !dragHasFiles(e)) return + if (!panelTakesDrops || !dragHasFiles(e)) return e.preventDefault() dragDepth++ } function onPanelDragOver(e: DragEvent) { - if (!canAttachFiles || !dragHasFiles(e)) return + if (!panelTakesDrops || !dragHasFiles(e)) return e.preventDefault() if (e.dataTransfer) e.dataTransfer.dropEffect = 'copy' } function onPanelDragLeave(_e: DragEvent) { - if (!canAttachFiles) return + if (!panelTakesDrops) return dragDepth = Math.max(0, dragDepth - 1) } async function onPanelDrop(e: DragEvent) { dragDepth = 0 - if (!canAttachFiles || !dragHasFiles(e)) return + if (!panelTakesDrops || !dragHasFiles(e)) return e.preventDefault() + if (attachmentsOffReason) { + sendUserToast(attachmentsOffReason, true) + return + } const dt = e.dataTransfer if (!dt) return // Images and loose text files attach to the message; folders link as session @@ -484,9 +500,8 @@ handles.length === 0 ? flatFiles : await Promise.all(handles.filter(isFileHandle).map((h) => h.getFile())) - // Loose text files attach to the message, like images. - const textFiles = looseFiles.filter((f) => !isImageFile(f)) - if (textFiles.length > 0) await aiChatInput?.addTextFiles(textFiles) + // Loose files attach to the message, like images. + await attachNonImageFiles(looseFiles.filter((f) => !isImageFile(f))) // Folders link as a live handle. const dirs = handles.filter(isDirectoryHandle) if (dirs.length > 0 && !canLinkFolders) { @@ -523,24 +538,31 @@ if (canLinkFolders) await handleAddFiles(folderEntries) else sendUserToast('Folders cannot be attached in this chat — drop individual files.', true) } - if (topLevelText.length > 0) await aiChatInput?.addTextFiles(topLevelText) + await attachNonImageFiles(topLevelText) } } async function onFileInputChange(e: Event) { const input = e.currentTarget as HTMLInputElement if (input.files && input.files.length > 0) { - const picked = Array.from(input.files) - const imageFiles = picked.filter(isImageFile) - const textFiles = picked.filter((f) => !isImageFile(f)) - // Reserved before the text work is awaited — see onPanelDrop. - const imageWork = imageFiles.length > 0 ? aiChatInput?.addImages(imageFiles) : undefined - if (textFiles.length > 0) await aiChatInput?.addTextFiles(textFiles) - await imageWork + await attachPickedFiles(Array.from(input.files)) } input.value = '' // allow re-selecting the same file } + async function attachNonImageFiles(files: File[]) { + await aiChatInput?.addNonImageFiles(files) + } + + async function attachPickedFiles(picked: File[]) { + const imageFiles = picked.filter(isImageFile) + const others = picked.filter((f) => !isImageFile(f)) + // Reserved before the other work is awaited — see onPanelDrop. + const imageWork = imageFiles.length > 0 ? aiChatInput?.addImages(imageFiles) : undefined + await attachNonImageFiles(others) + await imageWork + } + function onFolderInputChange(e: Event) { const input = e.currentTarget as HTMLInputElement // webkitdirectory files carry webkitRelativePath (`folder/sub/file`); addFiles groups @@ -622,6 +644,7 @@ const showFooterLeftControls = $derived( !footerMessageShown && (canAttachFiles || + attachmentsOffReason !== undefined || showContextPicker || showAutonomyModeSelector || (chatHost.mode === AIMode.SCRIPT && hasDiff)) @@ -991,7 +1014,21 @@ the panel, or the Escape-to-stop focus check would wrongly reject them. --> {/snippet} {/if} - {#if canAttachFiles} + {#if attachmentsOffReason} + + + {/if} +
+ {#if editable && row.id !== ADMIN_ROLE} + removeRole(row.id)} /> + {/if} + + + + {/each} + + + {#if editable && unusedRoles.length > 0} +
+ + dataTable.database.resource_type, - (resource_type) => { - dataTable.database = { - resource_type, - resource_path: - resource_type === 'instance' ? defaultInstanceDbName() : undefined + {#if dataTable.reference} +
+ Governed by + {dataTable.reference.workspace_id} + / + {dataTable.reference.datatable} + + This fork uses its parent's data table rather than a copy of it, so the database and + its roles are decided in that workspace. + +
+ {:else} +
+
+ {#if dataTable.database.resource_type === 'instance'} + + Use Windmill's PostgreSQL instance + + {/if} + does. // - // In "large mode" (viewport ≥ 1760px, where app.css bumps :root to 18px → + // In "large mode" (screen ≥ 1760px, where app.css bumps :root to 18px → // font 13.5px) headless-Chromium ink measurement showed the text sitting // ~1px low with the base leading. The residual is a fixed ~2px of line box, // so the exact centered value there is (content-box height − 2px). Scoped to - // the same 1760px breakpoint as the font-size bump; small mode is unchanged. + // the same query as the font-size bump; small mode is unchanged. export const inputLeadingClasses: Record = { - '2xs': 'leading-4 min-[1760px]:leading-[calc(1rem_-_2px)]', // h-5 − py-0.5 → 1rem - xs: 'leading-4 min-[1760px]:leading-[calc(1rem_-_2px)]', // h-5 − py-0.5 → 1rem - sm: 'leading-6 min-[1760px]:leading-[calc(1.5rem_-_2px)]', // h-7 − py-0.5 → 1.5rem - md: 'leading-8 min-[1760px]:leading-[calc(2rem_-_2px)]', // h-8, no py → 2rem - lg: 'leading-10 min-[1760px]:leading-[calc(2.5rem_-_2px)]' // h-10, no py → 2.5rem + '2xs': 'leading-4 [@media(min-device-width:1760px)]:leading-[calc(1rem_-_2px)]', // h-5 − py-0.5 → 1rem + xs: 'leading-4 [@media(min-device-width:1760px)]:leading-[calc(1rem_-_2px)]', // h-5 − py-0.5 → 1rem + sm: 'leading-6 [@media(min-device-width:1760px)]:leading-[calc(1.5rem_-_2px)]', // h-7 − py-0.5 → 1.5rem + md: 'leading-8 [@media(min-device-width:1760px)]:leading-[calc(2rem_-_2px)]', // h-8, no py → 2rem + lg: 'leading-10 [@media(min-device-width:1760px)]:leading-[calc(2.5rem_-_2px)]' // h-10, no py → 2.5rem } diff --git a/frontend/src/lib/editorFontSize.svelte.ts b/frontend/src/lib/editorFontSize.svelte.ts index 13d99915e3..96519b0aac 100644 --- a/frontend/src/lib/editorFontSize.svelte.ts +++ b/frontend/src/lib/editorFontSize.svelte.ts @@ -1,25 +1,25 @@ // Reactive Monaco font sizes that follow the global `:root` font-size -// breakpoint set in `frontend/src/lib/assets/app.css` (18px at ≥1760px). +// breakpoint set in `frontend/src/lib/assets/app.css` (18px on screens ≥1760px). // The values mirror Tailwind's `text-xs` (0.75rem) computed pixel size so // editors visually match the surrounding UI. -const LARGE_VIEWPORT_QUERY = '(min-width: 1760px)' +const LARGE_SCREEN_QUERY = '(min-device-width: 1760px)' -let isLargeViewport = $state(false) +let isLargeScreen = $state(false) if (typeof window !== 'undefined') { - const mq = window.matchMedia(LARGE_VIEWPORT_QUERY) - isLargeViewport = mq.matches + const mq = window.matchMedia(LARGE_SCREEN_QUERY) + isLargeScreen = mq.matches mq.addEventListener('change', (e) => { - isLargeViewport = e.matches + isLargeScreen = e.matches }) } export const editorFontSize = { get regular(): number { - return isLargeViewport ? 13.5 : 12 + return isLargeScreen ? 13.5 : 12 }, get small(): number { - return isLargeViewport ? 12 : 11 + return isLargeScreen ? 12 : 11 } } diff --git a/frontend/src/routes/(root)/(logged)/+layout.svelte b/frontend/src/routes/(root)/(logged)/+layout.svelte index d14cd2c2dc..58d852a2d5 100644 --- a/frontend/src/routes/(root)/(logged)/+layout.svelte +++ b/frontend/src/routes/(root)/(logged)/+layout.svelte @@ -139,8 +139,8 @@ let isCollapsed = $state(collapsePref.val) // Resizable desktop rail, sized in REM so it scales with the root font-size the - // same way the old `w-52`/`w-12` classes did — `:root` jumps to 18px past 1760px - // wide (app.css), which grows the rem-based button content; a fixed-px rail would + // same way the old `w-52`/`w-12` classes did — `:root` jumps to 18px on screens + // ≥1760px (app.css), which grows the rem-based button content; a fixed-px rail would // not grow with it and the content would overflow. SIDEBAR_MIN_REM is the default // expanded width (the old w-52); the handle only resizes when expanded and only // widens from there — collapsing is the toggle button's job, not the drag's. From 53afecd4588247bc1812d3e68a30db1f3c3b2724 Mon Sep 17 00:00:00 2001 From: hugocasa Date: Fri, 18 Sep 2026 15:15:02 +0200 Subject: [PATCH 21/25] fix: register the job token with the sensitive log masking system (#10943) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(worker): register the job token with the log masking system The masking system covered secrets fetched through `get_value_internal` and `$encrypted:` args, but not the job's own token, so a script that echoed `$WM_TOKEN` wrote it verbatim into logs that are persisted to the database and, when configured, to object storage. `run_worker` now registers the token for the job it just pulled, alongside the existing `register_running_job` call, so it is redacted like any other registered secret. That makes every job carry at least one registered value, where before the per-batch mask snapshot was skipped entirely for the majority of jobs that touched no secret. Cache the compiled Aho-Corasick automaton per job and invalidate it when a new secret is registered, so a chatty job no longer rebuilds it once per log batch. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01P316wKe2QCYNcdsx1PwAJ3 * fix(nativets): mask secrets in the in-process log path NativeTS hands `console.log` output to a task that drains a channel into `append_logs`, so it never reaches the masking in `handle_child::write_lines` and a script logging `$WM_TOKEN` persisted the raw JWT. That drain can still be flushing after the job is unregistered, so a plain per-line `snapshot` would leave the tail unmasked. `JobMasker` keeps the last masks it saw for exactly that window, and refreshes while the job is alive so secrets fetched mid-run are covered too. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01P316wKe2QCYNcdsx1PwAJ3 * fix(nativets): seed the job masker at construction A `JobMasker` that only looked up its masks on the first `mask` call had the same hole at the head of the log that its retention closes at the tail: if the drain task's first productive poll landed after the job was unregistered, the registry was already gone and every line was written raw. `new` now takes the snapshot, and its callers construct it from the job's own execution while the job is still registered. Also cover the nativets sink with an integration test, gated on `deno_core` the way the CI test build is. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01P316wKe2QCYNcdsx1PwAJ3 * fix(nativets): mask on the producing side of the log channel Masking as the drain task wrote to `append_logs` left two holes, because that task is detached and outlives the job: a secret registered mid-run could still be queued when the job was unregistered and would then be written raw, and the `windmill:job_log` tracing emission that EE forwards job logs on never went through the mask at all. Mask where the line is produced instead. That loop is joined before the job completes, so the job's secrets are always still registered, and one call now covers both the tracing mirror and the channel. The result stream keeps reading the raw text, the way `handle_child` keeps its raw `line` for results. `JobMasker` is no longer load-bearing for the post-unregistration window, so it is documented for what it now does: keep the security notice to once per set of secrets for a sink that masks line by line. Also drop the nativets test's tag override — `DEFAULT_TAGS` does advertise `nativets`, so the comment justifying it was wrong — and pin the automaton cache invalidation, whose failure mode is an unmasked secret. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01P316wKe2QCYNcdsx1PwAJ3 * fix(worker): hold the log-masking lifecycle at the job boundary Registering the job around the poller's call left every other way of running a job uncovered: the interactive worker shell and inline AI agent tools both call `handle_queued_job` directly, and a script logging `$WM_TOKEN` from either persisted the live credential. Register from inside `handle_queued_job` instead, under a drop guard, so each path is covered by construction rather than by remembering to add a call. Nothing is lost by unregistering earlier: the writes that follow go through `append_logs`, which never consulted the registry. In nativets, decide the stream/log routing before masking. `MaskSnapshot`'s notice is one-shot, so a secret-bearing `WM_STREAM:` chunk used to spend it on text that is then discarded, leaving later redactions in `job_logs` unexplained. Restore the masker's post-unregistration test: the memory-limit path never joins the producing loop, so that fallback is still load-bearing. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01P316wKe2QCYNcdsx1PwAJ3 * docs: correct the nativets masking comments The producer loop is not joined on the memory-limit path, so it does not "always" run while the job is registered — say normally, which is what `JobMasker`'s fallback is there for. Name the reason a stream chunk stays raw everywhere it goes, including the tracing mirror: it is result data that no log sink persists, so masking it would be masking a result. State the masker test's invariant without asserting a mechanism behind it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01P316wKe2QCYNcdsx1PwAJ3 * fix(masking): keep the security notice on a line of its own `mask` appended the notice as a newline plus the notice text, which assumes the caller hands it a bare log line. nativets hands it a chunk that already ends in a newline, and its sink concatenates chunks verbatim, so the notice arrived after a blank line and the next log line was welded onto the end of it. Emit the notice as its own line for either shape. `handle_child` is unaffected: its input never ends in a newline, so it keeps the original path. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01P316wKe2QCYNcdsx1PwAJ3 --------- Co-authored-by: Claude Opus 5 (1M context) --- backend/tests/job_token_log_masking.rs | 108 +++++++++ .../src/sensitive_log_masks.rs | 221 ++++++++++++++++-- backend/windmill-runtime-nativets/src/lib.rs | 23 +- backend/windmill-worker/src/worker.rs | 30 ++- 4 files changed, 350 insertions(+), 32 deletions(-) create mode 100644 backend/tests/job_token_log_masking.rs diff --git a/backend/tests/job_token_log_masking.rs b/backend/tests/job_token_log_masking.rs new file mode 100644 index 0000000000..5d7de27218 --- /dev/null +++ b/backend/tests/job_token_log_masking.rs @@ -0,0 +1,108 @@ +/* + * The job's own token (`$WM_TOKEN`) stays valid well past the job it was minted + * for, and job logs are persisted to `job_logs` and optionally to object storage, + * so a script that echoes the token would otherwise park a live credential in + * durable storage. `run_worker` registers the token with `sensitive_log_masks` + * for the job it pulled; this pins that the persisted log carries the masked form. + */ + +use sqlx::{Pool, Postgres}; +use windmill_common::{ + jobs::{JobPayload, RawCode}, + scripts::ScriptLang, +}; +use windmill_test_utils::*; + +/// Prefix of a serialized job token: `jwt_` plus the base64 of a JWT header. +/// The masked form keeps only `jwt` + the last three characters, so it never matches. +const RAW_TOKEN_PREFIX: &str = "jwt_ey"; + +#[sqlx::test(fixtures("base"))] +async fn test_job_token_masked_in_persisted_logs(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + + let job = RunJob::from(JobPayload::Code(RawCode { + hash: None, + content: "echo \"running with --token $WM_TOKEN\"".to_string(), + path: None, + lock: None, + language: ScriptLang::Bash, + cache_ttl: None, + cache_ignore_s3_path: None, + dedicated_worker: None, + concurrency_settings: windmill_common::runnable_settings::ConcurrencySettings::default() + .into(), + debouncing_settings: windmill_common::runnable_settings::DebouncingSettings::default(), + modules: None, + tag: None, + })) + .run_until_complete(&db, false, port) + .await; + assert!(job.success, "job should have succeeded"); + + let logs = + sqlx::query_scalar::<_, Option>("SELECT logs FROM job_logs WHERE job_id = $1") + .bind(job.id) + .fetch_one(&db) + .await? + .unwrap_or_default(); + + assert!( + !logs.contains(RAW_TOKEN_PREFIX), + "an unmasked job token reached the persisted logs: {logs}" + ); + assert!( + logs.contains("secret value was masked"), + "expected the masking notice in logs: {logs}" + ); + Ok(()) +} + +/// nativets runs V8 in-process and persists `console.log` output through its own +/// channel, so it is masked by a different mechanism than the bash case above and +/// needs its own guard. +#[cfg(feature = "deno_core")] +#[sqlx::test(fixtures("base"))] +async fn test_job_token_masked_in_nativets_logs(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + + let job = RunJob::from(JobPayload::Code(RawCode { + hash: None, + content: "export async function main() {\n console.log('running with --token ' + process.env.WM_TOKEN);\n return 'ok';\n}".to_string(), + path: None, + lock: None, + language: ScriptLang::Nativets, + cache_ttl: None, + cache_ignore_s3_path: None, + dedicated_worker: None, + concurrency_settings: windmill_common::runnable_settings::ConcurrencySettings::default() + .into(), + debouncing_settings: windmill_common::runnable_settings::DebouncingSettings::default(), + modules: None, + tag: None, + })) + .run_until_complete(&db, false, port) + .await; + assert!(job.success, "job should have succeeded"); + + let logs = + sqlx::query_scalar::<_, Option>("SELECT logs FROM job_logs WHERE job_id = $1") + .bind(job.id) + .fetch_one(&db) + .await? + .unwrap_or_default(); + + assert!( + !logs.contains(RAW_TOKEN_PREFIX), + "an unmasked job token reached the persisted logs: {logs}" + ); + assert!( + logs.contains("secret value was masked"), + "expected the masking notice in logs: {logs}" + ); + Ok(()) +} diff --git a/backend/windmill-common/src/sensitive_log_masks.rs b/backend/windmill-common/src/sensitive_log_masks.rs index b6f6262b77..c999297ec0 100644 --- a/backend/windmill-common/src/sensitive_log_masks.rs +++ b/backend/windmill-common/src/sensitive_log_masks.rs @@ -10,7 +10,7 @@ use std::borrow::Cow; use std::collections::{HashMap, HashSet}; -use std::sync::RwLock; +use std::sync::{Arc, RwLock}; use uuid::Uuid; /// Minimum length for a secret to be registered for masking. @@ -20,9 +20,27 @@ const MIN_SECRET_LENGTH: usize = 8; const MASKED_NOTICE: &str = "[windmill] secret value was masked for security reasons, use string transformations to display full value"; +/// The secrets registered for one job, plus the automaton compiled from them. +#[derive(Default)] +struct JobMasks { + secrets: HashSet, + /// Built on the first `snapshot` after a change and shared by every later + /// snapshot. Every job registers at least its own token, so without this + /// cache each log batch of each job would rebuild the automaton. + compiled: Option>, +} + +/// Aho-Corasick automaton for O(m) multi-pattern matching in a single pass, +/// regardless of the number of secrets registered, with the replacement +/// strings indexed to match the automaton's pattern order. +struct CompiledMasks { + ac: aho_corasick::AhoCorasick, + replacements: Vec, +} + lazy_static::lazy_static! { - /// Map of job_id -> set of secret values that should be masked in that job's logs. - static ref SENSITIVE_MASKS: RwLock>> = + /// Map of job_id -> secret values that should be masked in that job's logs. + static ref SENSITIVE_MASKS: RwLock> = RwLock::new(HashMap::new()); /// Set of currently running job IDs on this worker process. @@ -32,13 +50,8 @@ lazy_static::lazy_static! { } /// A lock-free snapshot of secrets for a job, taken once per log batch. -/// Uses Aho-Corasick for O(m) multi-pattern matching in a single pass, -/// regardless of the number of secrets registered. pub struct MaskSnapshot { - /// Aho-Corasick automaton for fast matching. - ac: aho_corasick::AhoCorasick, - /// Replacement strings, indexed to match the automaton's pattern order. - replacements: Vec, + compiled: Arc, /// Whether the security notice has already been appended for this snapshot. /// Tracked locally to avoid a global write lock on every masked line. notice_shown: std::cell::Cell, @@ -53,34 +66,104 @@ impl MaskSnapshot { } // Single-pass check + replace using the pre-built automaton - if !self.ac.is_match(text) { + if !self.compiled.ac.is_match(text) { return Cow::Borrowed(text); } - let mut result = self.ac.replace_all(text, &self.replacements); + let mut result = self + .compiled + .ac + .replace_all(text, &self.compiled.replacements); - // Append the notice only once per snapshot (i.e. per batch) + // Append the notice only once per snapshot (i.e. per batch), as its own line. + // Callers pass either a bare line (`handle_child`) or a chunk that already ends + // in a newline (nativets), and the sinks concatenate what they get verbatim: + // assuming either shape welds the notice onto a neighbouring line. if !self.notice_shown.get() { self.notice_shown.set(true); - result.push('\n'); - result.push_str(MASKED_NOTICE); + if result.ends_with('\n') { + result.push_str(MASKED_NOTICE); + result.push('\n'); + } else { + result.push('\n'); + result.push_str(MASKED_NOTICE); + } } Cow::Owned(result) } } +/// A masker for sinks that mask line by line rather than in batches, like nativets +/// masking each `console.log` chunk as V8 produces it. `snapshot` per line would +/// re-arm the security notice on every one; this keeps it to once per distinct set +/// of secrets while still picking up secrets registered mid-run. +/// +/// Masks by job id alone — the caller is the one that knows the text it passes +/// belongs to that job. +pub struct JobMasker { + job_id: Uuid, + snapshot: Option, +} + +impl JobMasker { + pub fn new(job_id: Uuid) -> Self { + JobMasker { job_id, snapshot: snapshot(&job_id) } + } + + /// Mask every secret registered for the job. Returns `Cow::Borrowed` when no match. + /// Falls back to the masks it last saw once the job is unregistered, so a sink + /// still draining past the end of a run does not start emitting secrets. + pub fn mask<'a>(&mut self, text: &'a str) -> Cow<'a, str> { + if let Some(fresh) = snapshot(&self.job_id) { + // Replacing an equivalent snapshot would re-arm the notice, so only take + // one built from a secret set we have not seen. + let unchanged = self + .snapshot + .as_ref() + .is_some_and(|cur| Arc::ptr_eq(&cur.compiled, &fresh.compiled)); + if !unchanged { + self.snapshot = Some(fresh); + } + } + match self.snapshot.as_ref() { + Some(snapshot) => snapshot.mask(text), + None => Cow::Borrowed(text), + } + } +} + /// Take a snapshot of the current secrets for a job. Returns `None` if no secrets /// are registered (the caller can then skip masking entirely for the whole batch). /// /// Call this once per log batch in `write_lines`, not per line. pub fn snapshot(job_id: &Uuid) -> Option { - let masks = SENSITIVE_MASKS.read().unwrap_or_else(|e| e.into_inner()); - let secrets = masks.get(job_id)?; - if secrets.is_empty() { - return None; + { + let masks = SENSITIVE_MASKS.read().unwrap_or_else(|e| e.into_inner()); + let job = masks.get(job_id)?; + if job.secrets.is_empty() { + return None; + } + if let Some(compiled) = job.compiled.as_ref() { + return Some(MaskSnapshot { + compiled: compiled.clone(), + notice_shown: std::cell::Cell::new(false), + }); + } } + let mut masks = SENSITIVE_MASKS.write().unwrap_or_else(|e| e.into_inner()); + let job = masks.get_mut(job_id)?; + if job.secrets.is_empty() { + return None; + } + let compiled = job + .compiled + .get_or_insert_with(|| Arc::new(compile(&job.secrets))); + Some(MaskSnapshot { compiled: compiled.clone(), notice_shown: std::cell::Cell::new(false) }) +} + +fn compile(secrets: &HashSet) -> CompiledMasks { // Sort longest-first so longer secrets are matched before shorter substrings let mut sorted: Vec<&String> = secrets.iter().collect(); sorted.sort_by(|a, b| b.len().cmp(&a.len())); @@ -106,7 +189,7 @@ pub fn snapshot(job_id: &Uuid) -> Option { .build(sorted.iter().map(|s| s.as_str())) .expect("failed to build aho-corasick automaton"); - Some(MaskSnapshot { ac, replacements, notice_shown: std::cell::Cell::new(false) }) + CompiledMasks { ac, replacements } } /// Register a job as currently running. Call this before `handle_queued_job`. @@ -148,20 +231,110 @@ pub fn register_secret_for_all_running_jobs(secret: &str) { let mut masks = SENSITIVE_MASKS.write().unwrap_or_else(|e| e.into_inner()); for job_id in job_ids { - if let Some(set) = masks.get_mut(&job_id) { - set.insert(secret.to_string()); + if let Some(job) = masks.get_mut(&job_id) { + if job.secrets.insert(secret.to_string()) { + job.compiled = None; + } } } } /// Register a secret value for a specific job. -/// Used for `$encrypted:` args where we know the job ID. +/// Used for the job's own token and for `$encrypted:` args, where we know the job ID. pub fn register_secret_for_job(job_id: Uuid, secret: &str) { if secret.len() < MIN_SECRET_LENGTH { return; } let mut masks = SENSITIVE_MASKS.write().unwrap_or_else(|e| e.into_inner()); - if let Some(set) = masks.get_mut(&job_id) { - set.insert(secret.to_string()); + if let Some(job) = masks.get_mut(&job_id) { + if job.secrets.insert(secret.to_string()) { + job.compiled = None; + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + /// The compiled automaton is cached per job, so a secret registered after the + /// first snapshot only gets masked if the cache is invalidated. + #[test] + fn snapshot_rebuilds_after_a_new_secret_is_registered() { + let job_id = Uuid::new_v4(); + register_running_job(job_id); + register_secret_for_job(job_id, "firstsecretvalue"); + let _ = snapshot(&job_id) + .expect("secret registered") + .mask("firstsecretvalue"); + + register_secret_for_job(job_id, "secondsecretvalue"); + + let snap = snapshot(&job_id).expect("secrets registered"); + let masked = snap.mask("firstsecretvalue then secondsecretvalue"); + assert!(!masked.contains("firstsecretvalue"), "{masked}"); + assert!(!masked.contains("secondsecretvalue"), "{masked}"); + unregister_running_job(job_id); + } + + /// A line-by-line sink must not repeat the notice on every line, and must still + /// pick up a secret registered after the masker was built. + #[test] + fn job_masker_notices_once_per_secret_set() { + let job_id = Uuid::new_v4(); + register_running_job(job_id); + register_secret_for_job(job_id, "firstsecretvalue"); + let mut masker = JobMasker::new(job_id); + + let first = masker.mask("saw firstsecretvalue").into_owned(); + assert!(!first.contains("firstsecretvalue"), "{first}"); + assert!(first.contains(MASKED_NOTICE), "{first}"); + + let second = masker.mask("saw firstsecretvalue again").into_owned(); + assert!(!second.contains("firstsecretvalue"), "{second}"); + assert!(!second.contains(MASKED_NOTICE), "{second}"); + + register_secret_for_job(job_id, "secondsecretvalue"); + let third = masker.mask("saw secondsecretvalue").into_owned(); + assert!(!third.contains("secondsecretvalue"), "{third}"); + unregister_running_job(job_id); + } + + /// Unregistration must not turn masking off under a sink that is still emitting: + /// the masker keeps working off the masks it last saw rather than going quiet. + #[test] + fn job_masker_masks_after_the_job_is_unregistered() { + let job_id = Uuid::new_v4(); + register_running_job(job_id); + register_secret_for_job(job_id, "supersecretvalue"); + let mut masker = JobMasker::new(job_id); + + unregister_running_job(job_id); + + let masked = masker.mask("logged supersecretvalue here"); + assert!(!masked.contains("supersecretvalue"), "{masked}"); + } + + /// The notice has to end up on a line of its own for both shapes callers pass: + /// a bare line (`handle_child`) and a newline-terminated chunk (nativets). The + /// sinks concatenate what they are given verbatim, so getting this wrong welds + /// the notice onto whichever line follows it. + #[test] + fn notice_lands_on_its_own_line_for_both_caller_shapes() { + let job_id = Uuid::new_v4(); + register_running_job(job_id); + register_secret_for_job(job_id, "supersecretvalue"); + + let line = snapshot(&job_id) + .expect("secret registered") + .mask("tok supersecretvalue"); + assert_eq!(line, format!("tok s*****e\n{MASKED_NOTICE}")); + + let chunk = snapshot(&job_id) + .expect("secret registered") + .mask("tok supersecretvalue\n"); + assert_eq!(chunk, format!("tok s*****e\n{MASKED_NOTICE}\n")); + + unregister_running_job(job_id); } } diff --git a/backend/windmill-runtime-nativets/src/lib.rs b/backend/windmill-runtime-nativets/src/lib.rs index d44213d3bc..750bf7ebac 100644 --- a/backend/windmill-runtime-nativets/src/lib.rs +++ b/backend/windmill-runtime-nativets/src/lib.rs @@ -838,6 +838,11 @@ pub async fn eval_fetch_timeout( } } let w_id_for_tracing = w_id_for_tracing; + // nativets delivers logs in-process, so they never reach the masking in + // `handle_child::write_lines` and a `console.log` of `$WM_TOKEN` would be + // persisted verbatim. Mask here rather than in the detached task draining into + // `append_logs`: this loop normally runs while the job is still registered. + let mut masker = windmill_common::sensitive_log_masks::JobMasker::new(job_id); let handle = tokio::spawn(async move { let mut result_stream = String::new(); let mut is_stream = false; @@ -845,10 +850,20 @@ pub async fn eval_fetch_timeout( use windmill_common::result_stream::extract_stream_from_logs; use windmill_common::tracing_init::{OTEL_JOB_LOGS, OTEL_PREFIX}; + let stream = extract_stream_from_logs(&log.trim_end_matches("\n")); + + // A stream chunk is result data, not a log line — it never reaches + // `job_logs`, and `merge_result_stream` can make it the job's result — + // so it stays raw wherever it goes, here and in the mirror below. + // Deliberately unlike `handle_child`, which streams the masked text. + // Routed before masking because the notice is one-shot: spent on a chunk + // no sink persists, a later redaction in `job_logs` would go unexplained. + let logged = stream.is_none().then(|| masker.mask(&log).into_owned()); + // Mirror `process_streaming_log_lines` (EE) + the OTEL_JOB_LOGS // hook from handle_child.rs, neither of which runs for nativets // since nativets delivers logs in-process via the log channel. - for line in log.lines() { + for line in logged.as_deref().unwrap_or(&log).lines() { tracing::info!( target: "windmill:job_log", job_id = ?job_id, @@ -862,7 +877,7 @@ pub async fn eval_fetch_timeout( } } - if let Some(stream) = extract_stream_from_logs(&log.trim_end_matches("\n")) { + if let Some(stream) = stream { if !is_stream { is_stream = true; if let Some(ref f) = stream_notifier_update { @@ -874,8 +889,8 @@ pub async fn eval_fetch_timeout( if let Err(e) = result_stream_sender.send(stream) { tracing::error!("failed to send result stream: {e}"); } - } else { - if let Err(e) = append_logs_sender.send(log) { + } else if let Some(logged) = logged { + if let Err(e) = append_logs_sender.send(logged) { tracing::error!("failed to send log: {e}"); } } diff --git a/backend/windmill-worker/src/worker.rs b/backend/windmill-worker/src/worker.rs index 56b4b26fb0..55a698a9dd 100644 --- a/backend/windmill-worker/src/worker.rs +++ b/backend/windmill-worker/src/worker.rs @@ -3859,8 +3859,6 @@ pub async fn run_worker( let arc_job = Arc::new(job); - windmill_common::sensitive_log_masks::register_running_job(arc_job.id); - let span = create_span_with_name(&arc_job, &worker_name, Some(hostname), "job"); let log_ctx = log_context_for_job(&arc_job, &worker_name, Some(hostname)); @@ -3976,8 +3974,6 @@ pub async fn run_worker( _ => {} } - windmill_common::sensitive_log_masks::unregister_running_job(job_id); - #[cfg(feature = "prometheus")] if let Some(duration) = _timer.map(|x| x.stop_and_record()) { register_metric( @@ -4512,6 +4508,30 @@ async fn detect_and_store_runtime_assets_from_job_args( } } +/// Holds a job's entry in the log-masking registry for as long as it executes, so +/// that secrets it fetches can be registered against it, and masks the job's own +/// token from the start: `$WM_TOKEN` stays valid well past the run, and a script +/// that echoes it would otherwise leave a live credential in the persisted logs. +/// +/// Lives here rather than at the call sites so that every way of running a job — +/// the poller, the interactive worker shell, an inline AI agent tool — is covered +/// by construction. +struct RunningJobMasks(Uuid); + +impl RunningJobMasks { + fn register(job_id: Uuid, token: &str) -> Self { + windmill_common::sensitive_log_masks::register_running_job(job_id); + windmill_common::sensitive_log_masks::register_secret_for_job(job_id, token); + RunningJobMasks(job_id) + } +} + +impl Drop for RunningJobMasks { + fn drop(&mut self) { + windmill_common::sensitive_log_masks::unregister_running_job(self.0); + } +} + pub async fn handle_queued_job( job: Arc, raw_code: Option, @@ -4533,6 +4553,8 @@ pub async fn handle_queued_job( flow_runners: Option>, #[cfg(feature = "benchmark")] _bench: &mut BenchmarkIter, ) -> windmill_common::error::Result { + let _masks = RunningJobMasks::register(job.id, &client.token); + if job.canceled_by.is_some() { return Err(Error::JsonErr(canceled_job_to_result(&job))); } From 9d335de87a4dbaa51038d55afe8d980761dcdfaf Mon Sep 17 00:00:00 2001 From: hugocasa Date: Fri, 18 Sep 2026 15:15:35 +0200 Subject: [PATCH 22/25] feat: add a workspace toggle that adds its admins and developers to new forks (#11215) * feat: add a workspace toggle that adds its admins and developers to new forks Co-Authored-By: Claude Opus 5 * fix: add members copied into a fork as manual members, not instance-group ones Co-Authored-By: Claude Opus 5 * style: keep the fork members copy comment at the query and drop the raw spacer Co-Authored-By: Claude Opus 5 --------- Co-authored-by: Claude Opus 5 --- ...83dce0c9986f3847227c2f66daa84d4109d7d.json | 15 ---- ...ce73279957d6a39fdc6b1ecb6e0a1e02f921f.json | 16 ++++ ...56ee979e4c392970278304e80368b770bb7b4.json | 22 ++++++ ...cc1011a49fc8d8ad046ac572e81cf8938995.json} | 4 +- ...d21d12fa1207b58bfc46249c250cdcdb5363.json} | 12 ++- ...78f7e6dfa9185056f7731547ff05b8176b271.json | 15 ++++ ...bf1ebed0192be267983e8fbd18f79997e6142.json | 15 ---- ...1b93db0406aaf278bccba07be00e9f937e2a.json} | 14 +++- ...52fb2b4cf7983049d2c490940df360c7e2b30.json | 15 ++++ ...dd_admins_and_developers_to_forks.down.sql | 1 + ..._add_admins_and_developers_to_forks.up.sql | 1 + backend/summarized_schema.txt | 2 +- .../tests/fork_members.rs | 75 ++++++++++++++++++ .../windmill-api-workspaces/src/workspaces.rs | 77 ++++++++++++++++++- .../src/workspaces_extra.rs | 2 +- backend/windmill-api/openapi.yaml | 40 ++++++++++ .../settings/WorkspaceUserSettings.svelte | 36 +++++++++ .../CreateWorkspaceInner.svelte | 19 +++++ 18 files changed, 336 insertions(+), 45 deletions(-) delete mode 100644 backend/.sqlx/query-1d8ccd32266637d7f7915f92a8483dce0c9986f3847227c2f66daa84d4109d7d.json create mode 100644 backend/.sqlx/query-2d6b117324eaf076a0ed06d2cb0ce73279957d6a39fdc6b1ecb6e0a1e02f921f.json create mode 100644 backend/.sqlx/query-2f39fce0ee700117f3e4c066e0b56ee979e4c392970278304e80368b770bb7b4.json rename backend/.sqlx/{query-a6decdebcd9750691f20f874b66a9c6f2ede08c33605c6a3c3dfc213a3eda76a.json => query-5ccfbd0f345b9b86ca356008def6cc1011a49fc8d8ad046ac572e81cf8938995.json} (63%) rename backend/.sqlx/{query-dc4a57df3becc610f631ef22c116450390addbfae85fecc61c991d94167e6e99.json => query-8ebe054b41793f1a7b85f1f8d29cd21d12fa1207b58bfc46249c250cdcdb5363.json} (93%) create mode 100644 backend/.sqlx/query-9bd1995747f0073b3a866d1238e78f7e6dfa9185056f7731547ff05b8176b271.json delete mode 100644 backend/.sqlx/query-b98844926ff127c528ed3e7bc63bf1ebed0192be267983e8fbd18f79997e6142.json rename backend/.sqlx/{query-ede15bff96152f209aff756830cbc76b5afa1af6ed324376989117b1054c3447.json => query-e6e31fdf705896c81f9a0f45d47c1b93db0406aaf278bccba07be00e9f937e2a.json} (78%) create mode 100644 backend/.sqlx/query-eefa0588a6a927fd9b3f65e1df652fb2b4cf7983049d2c490940df360c7e2b30.json create mode 100644 backend/migrations/20260918092041_add_admins_and_developers_to_forks.down.sql create mode 100644 backend/migrations/20260918092041_add_admins_and_developers_to_forks.up.sql create mode 100644 backend/windmill-api-integration-tests/tests/fork_members.rs diff --git a/backend/.sqlx/query-1d8ccd32266637d7f7915f92a8483dce0c9986f3847227c2f66daa84d4109d7d.json b/backend/.sqlx/query-1d8ccd32266637d7f7915f92a8483dce0c9986f3847227c2f66daa84d4109d7d.json deleted file mode 100644 index b9cbc6c382..0000000000 --- a/backend/.sqlx/query-1d8ccd32266637d7f7915f92a8483dce0c9986f3847227c2f66daa84d4109d7d.json +++ /dev/null @@ -1,15 +0,0 @@ -{ - "db_name": "PostgreSQL", - "query": "INSERT INTO workspace_settings (workspace_id, slack_team_id, slack_name, slack_command_script, slack_email, customer_id, plan, webhook, ai_config, large_file_storage, git_sync, default_app, default_scripts, deploy_ui, mute_critical_alerts, color, operator_settings, teams_command_script, teams_team_id, teams_team_name, git_app_installations, git_credentials, ducklake, dbt_warehouses, slack_oauth_client_id, slack_oauth_client_secret, datatable, teams_team_guid, auto_invite, error_handler, success_handler, public_app_execution_limit_per_minute, error_handler_fallback_to_instance_alerts, guest_access_enabled, guest_jwt_public_key, guest_jwt_jwks_url) SELECT $1, slack_team_id, slack_name, slack_command_script, slack_email, customer_id, plan, webhook, ai_config, large_file_storage, git_sync, default_app, default_scripts, deploy_ui, mute_critical_alerts, color, operator_settings, teams_command_script, teams_team_id, teams_team_name, git_app_installations, git_credentials, ducklake, dbt_warehouses, slack_oauth_client_id, slack_oauth_client_secret, datatable, teams_team_guid, auto_invite, error_handler, success_handler, public_app_execution_limit_per_minute, error_handler_fallback_to_instance_alerts, guest_access_enabled, guest_jwt_public_key, guest_jwt_jwks_url FROM workspace_settings WHERE workspace_id = $2", - "describe": { - "columns": [], - "parameters": { - "Left": [ - "Varchar", - "Text" - ] - }, - "nullable": [] - }, - "hash": "1d8ccd32266637d7f7915f92a8483dce0c9986f3847227c2f66daa84d4109d7d" -} diff --git a/backend/.sqlx/query-2d6b117324eaf076a0ed06d2cb0ce73279957d6a39fdc6b1ecb6e0a1e02f921f.json b/backend/.sqlx/query-2d6b117324eaf076a0ed06d2cb0ce73279957d6a39fdc6b1ecb6e0a1e02f921f.json new file mode 100644 index 0000000000..4b96519a06 --- /dev/null +++ b/backend/.sqlx/query-2d6b117324eaf076a0ed06d2cb0ce73279957d6a39fdc6b1ecb6e0a1e02f921f.json @@ -0,0 +1,16 @@ +{ + "db_name": "PostgreSQL", + "query": "INSERT INTO usr (workspace_id, username, email, is_admin, created_at, operator, disabled, role, is_service_account, added_via)\n SELECT $1, username, email, is_admin, created_at, operator, disabled, role, is_service_account,\n CASE WHEN $3 THEN NULL ELSE added_via END\n FROM usr WHERE workspace_id = $2\n AND (NOT $3 OR (NOT operator AND NOT disabled AND NOT is_service_account))\n ON CONFLICT DO NOTHING", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Varchar", + "Text", + "Bool" + ] + }, + "nullable": [] + }, + "hash": "2d6b117324eaf076a0ed06d2cb0ce73279957d6a39fdc6b1ecb6e0a1e02f921f" +} diff --git a/backend/.sqlx/query-2f39fce0ee700117f3e4c066e0b56ee979e4c392970278304e80368b770bb7b4.json b/backend/.sqlx/query-2f39fce0ee700117f3e4c066e0b56ee979e4c392970278304e80368b770bb7b4.json new file mode 100644 index 0000000000..99b1a9472d --- /dev/null +++ b/backend/.sqlx/query-2f39fce0ee700117f3e4c066e0b56ee979e4c392970278304e80368b770bb7b4.json @@ -0,0 +1,22 @@ +{ + "db_name": "PostgreSQL", + "query": "SELECT add_admins_and_developers_to_forks FROM workspace_settings WHERE workspace_id = $1", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "add_admins_and_developers_to_forks", + "type_info": "Bool" + } + ], + "parameters": { + "Left": [ + "Text" + ] + }, + "nullable": [ + false + ] + }, + "hash": "2f39fce0ee700117f3e4c066e0b56ee979e4c392970278304e80368b770bb7b4" +} diff --git a/backend/.sqlx/query-a6decdebcd9750691f20f874b66a9c6f2ede08c33605c6a3c3dfc213a3eda76a.json b/backend/.sqlx/query-5ccfbd0f345b9b86ca356008def6cc1011a49fc8d8ad046ac572e81cf8938995.json similarity index 63% rename from backend/.sqlx/query-a6decdebcd9750691f20f874b66a9c6f2ede08c33605c6a3c3dfc213a3eda76a.json rename to backend/.sqlx/query-5ccfbd0f345b9b86ca356008def6cc1011a49fc8d8ad046ac572e81cf8938995.json index 0532a5d3a0..7cd917d623 100644 --- a/backend/.sqlx/query-a6decdebcd9750691f20f874b66a9c6f2ede08c33605c6a3c3dfc213a3eda76a.json +++ b/backend/.sqlx/query-5ccfbd0f345b9b86ca356008def6cc1011a49fc8d8ad046ac572e81cf8938995.json @@ -1,6 +1,6 @@ { "db_name": "PostgreSQL", - "query": "\n UPDATE workspace_settings\n SET\n ai_config = source_ws.ai_config,\n large_file_storage = source_ws.large_file_storage,\n ducklake = source_ws.ducklake,\n dbt_warehouses = source_ws.dbt_warehouses,\n datatable = source_ws.datatable,\n git_app_installations = source_ws.git_app_installations\n FROM workspace_settings source_ws\n WHERE source_ws.workspace_id = $1\n AND workspace_settings.workspace_id = $2\n ", + "query": "\n UPDATE workspace_settings\n SET\n ai_config = source_ws.ai_config,\n large_file_storage = source_ws.large_file_storage,\n ducklake = source_ws.ducklake,\n dbt_warehouses = source_ws.dbt_warehouses,\n datatable = source_ws.datatable,\n git_app_installations = source_ws.git_app_installations,\n add_admins_and_developers_to_forks = source_ws.add_admins_and_developers_to_forks\n FROM workspace_settings source_ws\n WHERE source_ws.workspace_id = $1\n AND workspace_settings.workspace_id = $2\n ", "describe": { "columns": [], "parameters": { @@ -11,5 +11,5 @@ }, "nullable": [] }, - "hash": "a6decdebcd9750691f20f874b66a9c6f2ede08c33605c6a3c3dfc213a3eda76a" + "hash": "5ccfbd0f345b9b86ca356008def6cc1011a49fc8d8ad046ac572e81cf8938995" } diff --git a/backend/.sqlx/query-dc4a57df3becc610f631ef22c116450390addbfae85fecc61c991d94167e6e99.json b/backend/.sqlx/query-8ebe054b41793f1a7b85f1f8d29cd21d12fa1207b58bfc46249c250cdcdb5363.json similarity index 93% rename from backend/.sqlx/query-dc4a57df3becc610f631ef22c116450390addbfae85fecc61c991d94167e6e99.json rename to backend/.sqlx/query-8ebe054b41793f1a7b85f1f8d29cd21d12fa1207b58bfc46249c250cdcdb5363.json index 01c0fd19af..7a822a8d6a 100644 --- a/backend/.sqlx/query-dc4a57df3becc610f631ef22c116450390addbfae85fecc61c991d94167e6e99.json +++ b/backend/.sqlx/query-8ebe054b41793f1a7b85f1f8d29cd21d12fa1207b58bfc46249c250cdcdb5363.json @@ -1,6 +1,6 @@ { "db_name": "PostgreSQL", - "query": "\n SELECT\n workspace_id,\n slack_team_id,\n teams_team_id,\n teams_team_name,\n teams_team_guid,\n slack_name,\n slack_command_script,\n teams_command_script,\n slack_email,\n slack_oauth_client_id,\n slack_oauth_client_secret,\n customer_id,\n plan,\n webhook,\n ai_config,\n dbt_warehouses,\n large_file_storage,\n datatable,\n ducklake,\n git_sync,\n deploy_ui,\n default_app,\n default_scripts,\n mute_critical_alerts,\n color,\n operator_settings,\n git_app_installations,\n auto_invite,\n error_handler,\n success_handler,\n public_app_execution_limit_per_minute,\n error_handler_fallback_to_instance_alerts,\n guest_access_enabled,\n guest_jwt_public_key,\n guest_jwt_jwks_url\n FROM\n workspace_settings\n WHERE\n workspace_id = $1\n ", + "query": "\n SELECT\n workspace_id,\n slack_team_id,\n teams_team_id,\n teams_team_name,\n teams_team_guid,\n slack_name,\n slack_command_script,\n teams_command_script,\n slack_email,\n slack_oauth_client_id,\n slack_oauth_client_secret,\n customer_id,\n plan,\n webhook,\n ai_config,\n dbt_warehouses,\n large_file_storage,\n datatable,\n ducklake,\n git_sync,\n deploy_ui,\n default_app,\n default_scripts,\n mute_critical_alerts,\n color,\n operator_settings,\n git_app_installations,\n auto_invite,\n error_handler,\n success_handler,\n public_app_execution_limit_per_minute,\n error_handler_fallback_to_instance_alerts,\n guest_access_enabled,\n guest_jwt_public_key,\n guest_jwt_jwks_url,\n add_admins_and_developers_to_forks\n FROM\n workspace_settings\n WHERE\n workspace_id = $1\n ", "describe": { "columns": [ { @@ -177,6 +177,11 @@ "ordinal": 34, "name": "guest_jwt_jwks_url", "type_info": "Text" + }, + { + "ordinal": 35, + "name": "add_admins_and_developers_to_forks", + "type_info": "Bool" } ], "parameters": { @@ -219,8 +224,9 @@ false, false, true, - true + true, + false ] }, - "hash": "dc4a57df3becc610f631ef22c116450390addbfae85fecc61c991d94167e6e99" + "hash": "8ebe054b41793f1a7b85f1f8d29cd21d12fa1207b58bfc46249c250cdcdb5363" } diff --git a/backend/.sqlx/query-9bd1995747f0073b3a866d1238e78f7e6dfa9185056f7731547ff05b8176b271.json b/backend/.sqlx/query-9bd1995747f0073b3a866d1238e78f7e6dfa9185056f7731547ff05b8176b271.json new file mode 100644 index 0000000000..9c4d587c01 --- /dev/null +++ b/backend/.sqlx/query-9bd1995747f0073b3a866d1238e78f7e6dfa9185056f7731547ff05b8176b271.json @@ -0,0 +1,15 @@ +{ + "db_name": "PostgreSQL", + "query": "UPDATE workspace_settings SET add_admins_and_developers_to_forks = $1 WHERE workspace_id = $2", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Bool", + "Text" + ] + }, + "nullable": [] + }, + "hash": "9bd1995747f0073b3a866d1238e78f7e6dfa9185056f7731547ff05b8176b271" +} diff --git a/backend/.sqlx/query-b98844926ff127c528ed3e7bc63bf1ebed0192be267983e8fbd18f79997e6142.json b/backend/.sqlx/query-b98844926ff127c528ed3e7bc63bf1ebed0192be267983e8fbd18f79997e6142.json deleted file mode 100644 index 94cf77ebc5..0000000000 --- a/backend/.sqlx/query-b98844926ff127c528ed3e7bc63bf1ebed0192be267983e8fbd18f79997e6142.json +++ /dev/null @@ -1,15 +0,0 @@ -{ - "db_name": "PostgreSQL", - "query": "INSERT INTO usr (workspace_id, username, email, is_admin, created_at, operator, disabled, role, is_service_account, added_via)\n SELECT $1, username, email, is_admin, created_at, operator, disabled, role, is_service_account, added_via\n FROM usr WHERE workspace_id = $2\n ON CONFLICT DO NOTHING", - "describe": { - "columns": [], - "parameters": { - "Left": [ - "Varchar", - "Text" - ] - }, - "nullable": [] - }, - "hash": "b98844926ff127c528ed3e7bc63bf1ebed0192be267983e8fbd18f79997e6142" -} diff --git a/backend/.sqlx/query-ede15bff96152f209aff756830cbc76b5afa1af6ed324376989117b1054c3447.json b/backend/.sqlx/query-e6e31fdf705896c81f9a0f45d47c1b93db0406aaf278bccba07be00e9f937e2a.json similarity index 78% rename from backend/.sqlx/query-ede15bff96152f209aff756830cbc76b5afa1af6ed324376989117b1054c3447.json rename to backend/.sqlx/query-e6e31fdf705896c81f9a0f45d47c1b93db0406aaf278bccba07be00e9f937e2a.json index 1c247ad5b5..7462224850 100644 --- a/backend/.sqlx/query-ede15bff96152f209aff756830cbc76b5afa1af6ed324376989117b1054c3447.json +++ b/backend/.sqlx/query-e6e31fdf705896c81f9a0f45d47c1b93db0406aaf278bccba07be00e9f937e2a.json @@ -1,6 +1,6 @@ { "db_name": "PostgreSQL", - "query": "\n SELECT\n workspace_id,\n slack_team_id,\n slack_name,\n teams_team_id,\n teams_team_name,\n teams_team_guid,\n mute_critical_alerts,\n guest_access_enabled,\n deploy_ui,\n large_file_storage,\n datatable\n FROM\n workspace_settings\n WHERE\n workspace_id = $1\n ", + "query": "\n SELECT\n workspace_id,\n slack_team_id,\n slack_name,\n teams_team_id,\n teams_team_name,\n teams_team_guid,\n mute_critical_alerts,\n guest_access_enabled,\n add_admins_and_developers_to_forks,\n deploy_ui,\n large_file_storage,\n datatable\n FROM\n workspace_settings\n WHERE\n workspace_id = $1\n ", "describe": { "columns": [ { @@ -45,16 +45,21 @@ }, { "ordinal": 8, + "name": "add_admins_and_developers_to_forks", + "type_info": "Bool" + }, + { + "ordinal": 9, "name": "deploy_ui", "type_info": "Jsonb" }, { - "ordinal": 9, + "ordinal": 10, "name": "large_file_storage", "type_info": "Jsonb" }, { - "ordinal": 10, + "ordinal": 11, "name": "datatable", "type_info": "Jsonb" } @@ -73,10 +78,11 @@ true, true, false, + false, true, true, true ] }, - "hash": "ede15bff96152f209aff756830cbc76b5afa1af6ed324376989117b1054c3447" + "hash": "e6e31fdf705896c81f9a0f45d47c1b93db0406aaf278bccba07be00e9f937e2a" } diff --git a/backend/.sqlx/query-eefa0588a6a927fd9b3f65e1df652fb2b4cf7983049d2c490940df360c7e2b30.json b/backend/.sqlx/query-eefa0588a6a927fd9b3f65e1df652fb2b4cf7983049d2c490940df360c7e2b30.json new file mode 100644 index 0000000000..ec641bf32d --- /dev/null +++ b/backend/.sqlx/query-eefa0588a6a927fd9b3f65e1df652fb2b4cf7983049d2c490940df360c7e2b30.json @@ -0,0 +1,15 @@ +{ + "db_name": "PostgreSQL", + "query": "INSERT INTO workspace_settings (workspace_id, slack_team_id, slack_name, slack_command_script, slack_email, customer_id, plan, webhook, ai_config, large_file_storage, git_sync, default_app, default_scripts, deploy_ui, mute_critical_alerts, color, operator_settings, teams_command_script, teams_team_id, teams_team_name, git_app_installations, git_credentials, ducklake, dbt_warehouses, slack_oauth_client_id, slack_oauth_client_secret, datatable, teams_team_guid, auto_invite, error_handler, success_handler, public_app_execution_limit_per_minute, error_handler_fallback_to_instance_alerts, guest_access_enabled, guest_jwt_public_key, guest_jwt_jwks_url, add_admins_and_developers_to_forks) SELECT $1, slack_team_id, slack_name, slack_command_script, slack_email, customer_id, plan, webhook, ai_config, large_file_storage, git_sync, default_app, default_scripts, deploy_ui, mute_critical_alerts, color, operator_settings, teams_command_script, teams_team_id, teams_team_name, git_app_installations, git_credentials, ducklake, dbt_warehouses, slack_oauth_client_id, slack_oauth_client_secret, datatable, teams_team_guid, auto_invite, error_handler, success_handler, public_app_execution_limit_per_minute, error_handler_fallback_to_instance_alerts, guest_access_enabled, guest_jwt_public_key, guest_jwt_jwks_url, add_admins_and_developers_to_forks FROM workspace_settings WHERE workspace_id = $2", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Varchar", + "Text" + ] + }, + "nullable": [] + }, + "hash": "eefa0588a6a927fd9b3f65e1df652fb2b4cf7983049d2c490940df360c7e2b30" +} diff --git a/backend/migrations/20260918092041_add_admins_and_developers_to_forks.down.sql b/backend/migrations/20260918092041_add_admins_and_developers_to_forks.down.sql new file mode 100644 index 0000000000..4e8ac47c69 --- /dev/null +++ b/backend/migrations/20260918092041_add_admins_and_developers_to_forks.down.sql @@ -0,0 +1 @@ +ALTER TABLE workspace_settings DROP COLUMN add_admins_and_developers_to_forks; diff --git a/backend/migrations/20260918092041_add_admins_and_developers_to_forks.up.sql b/backend/migrations/20260918092041_add_admins_and_developers_to_forks.up.sql new file mode 100644 index 0000000000..bb3502d362 --- /dev/null +++ b/backend/migrations/20260918092041_add_admins_and_developers_to_forks.up.sql @@ -0,0 +1 @@ +ALTER TABLE workspace_settings ADD COLUMN add_admins_and_developers_to_forks BOOLEAN NOT NULL DEFAULT false; diff --git a/backend/summarized_schema.txt b/backend/summarized_schema.txt index e27cb2893c..47f9faca68 100644 --- a/backend/summarized_schema.txt +++ b/backend/summarized_schema.txt @@ -234,7 +234,7 @@ workspace_protection_rule: workspace_id(char), name(char), rules(int), bypass_gr FK: (workspace_id) -> workspace(id) workspace_runnable_dependencies: flow_path(char), runnable_path(char), script_hash(bigint), runnable_is_flow(bool), workspace_id(char), app_path(char), id(bigint), runnable_is_agent(bool) FK: (app_path, workspace_id) -> app(path, workspace_id) | (flow_path, workspace_id) -> flow(path, workspace_id) -workspace_settings: workspace_id(char), slack_team_id(char), slack_name(char), slack_command_script(char), slack_email(char), customer_id(char), plan(char), webhook(text), ai_config(jsonb), large_file_storage(jsonb), git_sync(jsonb), default_app(char), default_scripts(jsonb), deploy_ui(jsonb), mute_critical_alerts(bool), color(char), operator_settings(jsonb), teams_command_script(text), teams_team_id(text), teams_team_name(text), git_app_installations(jsonb), ducklake(jsonb), slack_oauth_client_id(char), slack_oauth_client_secret(char), datatable(jsonb), teams_team_guid(text), auto_invite(jsonb), error_handler(jsonb), success_handler(jsonb), public_app_execution_limit_per_minute(int), dbt_warehouses(jsonb), guest_access_enabled(bool), guest_jwt_public_key(text), guest_jwt_jwks_url(text), ai_sessions_backup_generation(int) +workspace_settings: workspace_id(char), slack_team_id(char), slack_name(char), slack_command_script(char), slack_email(char), customer_id(char), plan(char), webhook(text), ai_config(jsonb), large_file_storage(jsonb), git_sync(jsonb), default_app(char), default_scripts(jsonb), deploy_ui(jsonb), mute_critical_alerts(bool), color(char), operator_settings(jsonb), teams_command_script(text), teams_team_id(text), teams_team_name(text), git_app_installations(jsonb), ducklake(jsonb), slack_oauth_client_id(char), slack_oauth_client_secret(char), datatable(jsonb), teams_team_guid(text), auto_invite(jsonb), error_handler(jsonb), success_handler(jsonb), public_app_execution_limit_per_minute(int), dbt_warehouses(jsonb), guest_access_enabled(bool), guest_jwt_public_key(text), guest_jwt_jwks_url(text), ai_sessions_backup_generation(int), add_admins_and_developers_to_forks(bool) FK: (workspace_id) -> workspace(id) zombie_job_counter: job_id(uuid), counter(int) FK: (job_id) -> v2_job(id) diff --git a/backend/windmill-api-integration-tests/tests/fork_members.rs b/backend/windmill-api-integration-tests/tests/fork_members.rs new file mode 100644 index 0000000000..5f4baced1f --- /dev/null +++ b/backend/windmill-api-integration-tests/tests/fork_members.rs @@ -0,0 +1,75 @@ +use serde_json::json; +use sqlx::{Pool, Postgres}; + +use windmill_test_utils::*; + +/// With `add_admins_and_developers_to_forks` on, a fork starts with the parent's admins and +/// developers at their parent role, even when a developer forks it; operators are left out. The +/// copies are manual members: a parent membership that came from an instance group must not carry +/// that provenance into a fork that does not configure the group. +#[sqlx::test(migrations = "../migrations", fixtures("base"))] +async fn test_fork_adds_parent_admins_and_developers(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + let server = ApiServer::start(db.clone()).await?; + let base_url = format!( + "http://localhost:{}/api/w/test-workspace/workspaces", + server.addr.port() + ); + let client = reqwest::Client::new(); + + sqlx::query( + "UPDATE usr SET operator = true WHERE workspace_id = 'test-workspace' AND username = 'test-user-3'", + ) + .execute(&db) + .await?; + sqlx::query( + "INSERT INTO usr (workspace_id, email, username, is_admin, added_via) + VALUES ('test-workspace', 'test4@windmill.dev', 'test-user-4', false, + '{\"source\": \"instance_group\", \"group\": \"devs\"}')", + ) + .execute(&db) + .await?; + + let resp = client + .post(format!( + "{base_url}/edit_add_admins_and_developers_to_forks" + )) + .header("Authorization", "Bearer SECRET_TOKEN") + .json(&json!({ "add_admins_and_developers_to_forks": true })) + .send() + .await?; + assert!( + resp.status().is_success(), + "enabling the setting: {}", + resp.text().await? + ); + + let resp = client + .post(format!("{base_url}/create_fork")) + .header("Authorization", "Bearer SECRET_TOKEN_2") + .json(&json!({ "id": "wm-fork-team", "name": "Team fork" })) + .send() + .await?; + assert!( + resp.status().is_success(), + "creating the fork: {}", + resp.text().await? + ); + + let members: Vec<(String, bool, bool)> = sqlx::query_as( + "SELECT username, is_admin, added_via IS NULL FROM usr + WHERE workspace_id = 'wm-fork-team' ORDER BY username", + ) + .fetch_all(&db) + .await?; + assert_eq!( + members, + vec![ + ("test-user".to_string(), true, true), + ("test-user-2".to_string(), false, true), + ("test-user-4".to_string(), false, true), + ] + ); + + Ok(()) +} diff --git a/backend/windmill-api-workspaces/src/workspaces.rs b/backend/windmill-api-workspaces/src/workspaces.rs index aafba577b6..eb709c49c2 100644 --- a/backend/windmill-api-workspaces/src/workspaces.rs +++ b/backend/windmill-api-workspaces/src/workspaces.rs @@ -155,6 +155,10 @@ pub fn workspaced_service() -> Router { .route("/edit_deploy_ui_config", post(edit_deploy_ui_config)) .route("/edit_default_app", post(edit_default_app)) .route("/edit_guest_access", post(edit_guest_access)) + .route( + "/edit_add_admins_and_developers_to_forks", + post(edit_add_admins_and_developers_to_forks), + ) .route("/edit_guest_jwt_key", post(edit_guest_jwt_key)) .route("/guest_usage", get(get_guest_usage)) .route("/default_app", get(get_default_app)) @@ -338,6 +342,7 @@ pub struct WorkspaceSettings { pub guest_jwt_public_key: Option, #[serde(skip_serializing_if = "Option::is_none")] pub guest_jwt_jwks_url: Option, + pub add_admins_and_developers_to_forks: bool, } /// Subset of `WorkspaceSettings` that is safe to return to any workspace @@ -363,6 +368,8 @@ pub struct WorkspacePublicSettings { /// Not sensitive, and the app editor needs it to say whether the guest rung is /// live -- an app can be set to `guest` while the workspace has guests off. pub guest_access_enabled: bool, + /// Read by the fork dialog, which tells the forker who else the fork will include. + pub add_admins_and_developers_to_forks: bool, #[serde(skip_serializing_if = "Option::is_none")] pub deploy_ui: Option, #[serde(skip_serializing_if = "Option::is_none")] @@ -1126,7 +1133,8 @@ async fn get_settings( error_handler_fallback_to_instance_alerts, guest_access_enabled, guest_jwt_public_key, - guest_jwt_jwks_url + guest_jwt_jwks_url, + add_admins_and_developers_to_forks FROM workspace_settings WHERE @@ -1168,6 +1176,7 @@ async fn get_public_settings( teams_team_guid, mute_critical_alerts, guest_access_enabled, + add_admins_and_developers_to_forks, deploy_ui, large_file_storage, datatable @@ -5052,6 +5061,47 @@ async fn edit_guest_access( )) } +#[derive(Deserialize)] +struct EditAddAdminsAndDevelopersToForks { + add_admins_and_developers_to_forks: bool, +} + +async fn edit_add_admins_and_developers_to_forks( + authed: ApiAuthed, + Extension(db): Extension, + Path(w_id): Path, + Json(EditAddAdminsAndDevelopersToForks { add_admins_and_developers_to_forks }): Json< + EditAddAdminsAndDevelopersToForks, + >, +) -> Result { + require_admin(authed.is_admin, &authed.username)?; + + let mut tx = db.begin().await?; + sqlx::query!( + "UPDATE workspace_settings SET add_admins_and_developers_to_forks = $1 WHERE workspace_id = $2", + add_admins_and_developers_to_forks, + &w_id + ) + .execute(&mut *tx) + .await?; + + audit_log( + &mut *tx, + &authed, + "workspaces.edit_add_admins_and_developers_to_forks", + ActionKind::Update, + &w_id, + Some(&add_admins_and_developers_to_forks.to_string()), + None, + ) + .await?; + tx.commit().await?; + + Ok(format!( + "Adding admins and developers to new forks set to {add_admins_and_developers_to_forks} for workspace {w_id}" + )) +} + #[derive(Deserialize)] struct EditGuestJwtKey { /// A PEM public key (RS or ES family), or a JWKS URL, at most one. Both empty clears the @@ -6712,7 +6762,8 @@ async fn update_workspace_settings( ducklake = source_ws.ducklake, dbt_warehouses = source_ws.dbt_warehouses, datatable = source_ws.datatable, - git_app_installations = source_ws.git_app_installations + git_app_installations = source_ws.git_app_installations, + add_admins_and_developers_to_forks = source_ws.add_admins_and_developers_to_forks FROM workspace_settings source_ws WHERE source_ws.workspace_id = $1 AND workspace_settings.workspace_id = $2 @@ -6853,14 +6904,21 @@ async fn copy_workspace_members( tx: &mut Transaction<'_, Postgres>, source_workspace_id: &str, target_workspace_id: &str, + admins_and_developers_only: bool, ) -> Result<()> { + // Admins and developers join as manual members: the fork does not inherit the source's + // instance-group config, so a copied `instance_group` provenance would let the fork's + // reconciliation delete them and their data. sqlx::query!( "INSERT INTO usr (workspace_id, username, email, is_admin, created_at, operator, disabled, role, is_service_account, added_via) - SELECT $1, username, email, is_admin, created_at, operator, disabled, role, is_service_account, added_via + SELECT $1, username, email, is_admin, created_at, operator, disabled, role, is_service_account, + CASE WHEN $3 THEN NULL ELSE added_via END FROM usr WHERE workspace_id = $2 + AND (NOT $3 OR (NOT operator AND NOT disabled AND NOT is_service_account)) ON CONFLICT DO NOTHING", target_workspace_id, source_workspace_id, + admins_and_developers_only, ) .execute(&mut **tx) .await?; @@ -8651,8 +8709,19 @@ async fn create_workspace_fork( // intended. Dev creation is already admin-gated, so this is transitively admin-only too. Done before // the explicit creator insert below so the creator (a parent member) is copied with full metadata // (operator/role/is_service_account/added_via), not the bare row the insert alone would leave. + // Independently, the parent's admins can have every fork of it start with its admins and + // developers; the forker cannot opt out, since the point is that those admins can review it. if nw.copy_members && nw.is_dev_workspace { - copy_workspace_members(&mut tx, &parent_workspace_id, &forked_id).await?; + copy_workspace_members(&mut tx, &parent_workspace_id, &forked_id, false).await?; + } else if sqlx::query_scalar!( + "SELECT add_admins_and_developers_to_forks FROM workspace_settings WHERE workspace_id = $1", + parent_workspace_id + ) + .fetch_optional(&mut *tx) + .await? + .unwrap_or(false) + { + copy_workspace_members(&mut tx, &parent_workspace_id, &forked_id, true).await?; } // Ensure the creator is a member of the fork even without copy_members (or if they aren't a parent diff --git a/backend/windmill-api-workspaces/src/workspaces_extra.rs b/backend/windmill-api-workspaces/src/workspaces_extra.rs index 1cb5188e5b..45f82227f5 100644 --- a/backend/windmill-api-workspaces/src/workspaces_extra.rs +++ b/backend/windmill-api-workspaces/src/workspaces_extra.rs @@ -113,7 +113,7 @@ pub(crate) async fn change_workspace_id( // Duplicate workspace settings (keep copy in old workspace for reference) info!("Duplicating workspace_settings table"); sqlx::query!( - "INSERT INTO workspace_settings (workspace_id, slack_team_id, slack_name, slack_command_script, slack_email, customer_id, plan, webhook, ai_config, large_file_storage, git_sync, default_app, default_scripts, deploy_ui, mute_critical_alerts, color, operator_settings, teams_command_script, teams_team_id, teams_team_name, git_app_installations, git_credentials, ducklake, dbt_warehouses, slack_oauth_client_id, slack_oauth_client_secret, datatable, teams_team_guid, auto_invite, error_handler, success_handler, public_app_execution_limit_per_minute, error_handler_fallback_to_instance_alerts, guest_access_enabled, guest_jwt_public_key, guest_jwt_jwks_url) SELECT $1, slack_team_id, slack_name, slack_command_script, slack_email, customer_id, plan, webhook, ai_config, large_file_storage, git_sync, default_app, default_scripts, deploy_ui, mute_critical_alerts, color, operator_settings, teams_command_script, teams_team_id, teams_team_name, git_app_installations, git_credentials, ducklake, dbt_warehouses, slack_oauth_client_id, slack_oauth_client_secret, datatable, teams_team_guid, auto_invite, error_handler, success_handler, public_app_execution_limit_per_minute, error_handler_fallback_to_instance_alerts, guest_access_enabled, guest_jwt_public_key, guest_jwt_jwks_url FROM workspace_settings WHERE workspace_id = $2", + "INSERT INTO workspace_settings (workspace_id, slack_team_id, slack_name, slack_command_script, slack_email, customer_id, plan, webhook, ai_config, large_file_storage, git_sync, default_app, default_scripts, deploy_ui, mute_critical_alerts, color, operator_settings, teams_command_script, teams_team_id, teams_team_name, git_app_installations, git_credentials, ducklake, dbt_warehouses, slack_oauth_client_id, slack_oauth_client_secret, datatable, teams_team_guid, auto_invite, error_handler, success_handler, public_app_execution_limit_per_minute, error_handler_fallback_to_instance_alerts, guest_access_enabled, guest_jwt_public_key, guest_jwt_jwks_url, add_admins_and_developers_to_forks) SELECT $1, slack_team_id, slack_name, slack_command_script, slack_email, customer_id, plan, webhook, ai_config, large_file_storage, git_sync, default_app, default_scripts, deploy_ui, mute_critical_alerts, color, operator_settings, teams_command_script, teams_team_id, teams_team_name, git_app_installations, git_credentials, ducklake, dbt_warehouses, slack_oauth_client_id, slack_oauth_client_secret, datatable, teams_team_guid, auto_invite, error_handler, success_handler, public_app_execution_limit_per_minute, error_handler_fallback_to_instance_alerts, guest_access_enabled, guest_jwt_public_key, guest_jwt_jwks_url, add_admins_and_developers_to_forks FROM workspace_settings WHERE workspace_id = $2", &rw.new_id, &old_id ) diff --git a/backend/windmill-api/openapi.yaml b/backend/windmill-api/openapi.yaml index cba085a7e3..23390e0c67 100644 --- a/backend/windmill-api/openapi.yaml +++ b/backend/windmill-api/openapi.yaml @@ -4090,9 +4090,13 @@ paths: guest_access_enabled: type: boolean description: Whether this workspace admits guest sessions. An app's own `guest` execution mode is inert while this is false. + add_admins_and_developers_to_forks: + type: boolean + description: Whether every new fork of this workspace starts with its admins and developers as members, keeping their role. required: - workspace_id - guest_access_enabled + - add_admins_and_developers_to_forks /w/{workspace}/workspaces/get_settings: get: @@ -4183,6 +4187,9 @@ paths: guest_jwt_jwks_url: type: string description: JWKS URL a guest JWT (`jwt_guest_`) is verified against for this workspace. Mutually exclusive with `guest_jwt_public_key`. + add_admins_and_developers_to_forks: + type: boolean + description: Whether every new fork of this workspace starts with its admins and developers as members, keeping their role. /w/{workspace}/workspaces/get_deploy_to: get: @@ -6313,6 +6320,39 @@ paths: schema: type: string + /w/{workspace}/workspaces/edit_add_admins_and_developers_to_forks: + post: + summary: choose whether new forks of this workspace start with its admins and developers + description: >- + When on, every fork created from this workspace gets the workspace's admins and + developers as members, with the role they hold here; operators, disabled users and + service accounts are left out. The setting is copied into each fork, so forks of a + fork follow it too. Off by default. Workspace-admin gated. + operationId: editAddAdminsAndDevelopersToForks + tags: + - workspace + parameters: + - $ref: "#/components/parameters/WorkspaceId" + requestBody: + description: Whether new forks start with this workspace's admins and developers + required: true + content: + application/json: + schema: + type: object + properties: + add_admins_and_developers_to_forks: + type: boolean + required: + - add_admins_and_developers_to_forks + responses: + "200": + description: status + content: + text/plain: + schema: + type: string + /w/{workspace}/workspaces/edit_guest_jwt_key: post: summary: set the key guest JWTs are verified against for this workspace diff --git a/frontend/src/lib/components/settings/WorkspaceUserSettings.svelte b/frontend/src/lib/components/settings/WorkspaceUserSettings.svelte index 5c8af52683..449e9c7d31 100644 --- a/frontend/src/lib/components/settings/WorkspaceUserSettings.svelte +++ b/frontend/src/lib/components/settings/WorkspaceUserSettings.svelte @@ -46,6 +46,7 @@ let auto_invite_domain: string | undefined = $state() let operatorOnly: boolean | undefined = $state(undefined) let autoAdd: boolean | undefined = $state(false) + let addAdminsAndDevelopersToForks = $state(false) let nbDisplayed = $state(30) // Instance group auto-add settings @@ -122,6 +123,25 @@ autoAdd = autoInvite?.mode === 'add' autoAddInstanceGroups = autoInvite?.instance_groups || [] autoAddInstanceGroupsRoles = autoInvite?.instance_groups_roles || {} + addAdminsAndDevelopersToForks = settings.add_admins_and_developers_to_forks ?? false + } + + async function updateAddAdminsAndDevelopersToForks(enabled: boolean): Promise { + try { + await WorkspaceService.editAddAdminsAndDevelopersToForks({ + workspace: $workspaceStore!, + requestBody: { add_admins_and_developers_to_forks: enabled } + }) + sendUserToast( + enabled + ? 'New forks will start with the admins and developers of this workspace' + : 'New forks will start with their creator only' + ) + } catch (e) { + console.error('Failed to update the fork members setting:', e) + addAdminsAndDevelopersToForks = !enabled + sendUserToast(`Failed to update the fork members setting: ${e}`, true) + } } let getUsagePromise: CancelablePromise | undefined = undefined @@ -1067,6 +1087,22 @@
+
+ updateAddAdminsAndDevelopersToForks(e.detail)} + options={{ + right: 'Add admins and developers to new forks', + rightTooltip: + 'Admins and developers of this workspace join every new fork with the role they have here, so they can follow and review the work done in it. Forks of those forks follow the same setting.' + }} + /> +
+ {#if invites?.length > 0}
(isFork ? baseWorkspaceId : undefined), + async (ws, _prev, { signal }) => { + if (!ws) return undefined + const settings = await WorkspaceService.getPublicSettings({ workspace: ws }) + if (signal.aborted) throw new DOMException('superseded', 'AbortError') + return { ws, adds: settings.add_admins_and_developers_to_forks } + } + ) + let baseAddsAdminsAndDevelopers = $derived( + baseForkMembersResource.current?.ws === baseWorkspaceId && + !!baseForkMembersResource.current?.adds + ) // Ask the server whether a dev already exists: the caller may not be a member of this prod's dev, // so the client workspace list can't see it and would offer an invalid "create dev" action. const devWorkspaceResource = resource( @@ -929,6 +942,12 @@ disabled={createAsDevWorkspace} /> + {#if baseAddsAdminsAndDevelopers && !(createAsDevWorkspace && copyMembers)} + + {baseWorkspaceId} adds its admins and developers to every new fork, with the role they have + there. + + {/if} {/if} {#if isFork} Date: Fri, 18 Sep 2026 15:21:07 +0200 Subject: [PATCH 23/25] fix: re-encrypt git sync secrets on workspace key rotation (#11218) * fix: re-encrypt git sync credentials and webhook secrets on workspace key rotation Co-Authored-By: Claude Opus 5 * chore: point ee-repo-ref at the git sync key rotation companion Co-Authored-By: Claude Opus 5 * chore: update ee-repo-ref to bc3ef08c8e4233508c023e6ee847a3cd0b8be43b This commit updates the EE repository reference after PR #814 was merged in windmill-ee-private. Previous ee-repo-ref: 8121eac421c5d026f36e2edec140f3c79b23d2cd New ee-repo-ref: bc3ef08c8e4233508c023e6ee847a3cd0b8be43b Automated by sync-ee-ref workflow. --------- Co-authored-by: Claude Opus 5 Co-authored-by: windmill-internal-app[bot] --- backend/ee-repo-ref.txt | 2 +- .../workspace_encryption_key_git_sync.rs | 61 +++++++++++++ .../windmill-api-workspaces/src/workspaces.rs | 86 ++++++++++++++++--- 3 files changed, 135 insertions(+), 14 deletions(-) diff --git a/backend/ee-repo-ref.txt b/backend/ee-repo-ref.txt index 3d9d49412a..3dfd92f8c2 100644 --- a/backend/ee-repo-ref.txt +++ b/backend/ee-repo-ref.txt @@ -1 +1 @@ -f2fced19fcae81de7f6dac545010ce404c052e1b +bc3ef08c8e4233508c023e6ee847a3cd0b8be43b diff --git a/backend/windmill-api-integration-tests/tests/workspace_encryption_key_git_sync.rs b/backend/windmill-api-integration-tests/tests/workspace_encryption_key_git_sync.rs index 8edfd39d61..a22ad974c3 100644 --- a/backend/windmill-api-integration-tests/tests/workspace_encryption_key_git_sync.rs +++ b/backend/windmill-api-integration-tests/tests/workspace_encryption_key_git_sync.rs @@ -275,6 +275,67 @@ async fn test_encryption_key_rotation_dispatches_batched_git_sync( Ok(()) } +/// Stored repository tokens and webhook secrets are encrypted under the +/// workspace key but never synced, so a rotation has to carry them over even +/// when the caller skips re-encrypting variables. +#[sqlx::test(migrations = "../migrations", fixtures("base"))] +async fn test_encryption_key_rotation_reencrypts_git_sync_secrets( + db: Pool, +) -> anyhow::Result<()> { + use windmill_common::variables::{build_crypt, crypt_from_key_with_suffix, decrypt, encrypt}; + initialize_tracing().await; + + create_folder(&db, "28103").await?; + create_git_repo_resource(&db).await?; + let sync_script_path = "f/28103/test_sync_script_git_secrets"; + create_sync_script(&db, sync_script_path).await?; + setup_git_sync_config(&db, sync_script_path).await?; + + let mc = build_crypt(&db, "test-workspace").await?; + sqlx::query( + r#" + UPDATE workspace_settings SET + git_credentials = jsonb_build_array(jsonb_build_object( + 'token', $1::text, 'repo_identity', 'https://gitlab.example.com/grp/proj')), + git_sync = jsonb_set(git_sync, '{repositories,0,auto_pull}', jsonb_build_object( + 'enabled', true, 'mode', 'webhook', 'webhook_id', 1, 'webhook_secret', $2::text)) + WHERE workspace_id = 'test-workspace' + "#, + ) + .bind(encrypt(&mc, "stored-token")) + .bind(encrypt(&mc, "hook-secret")) + .execute(&db) + .await?; + + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + let base = format!("http://localhost:{port}/api/w/test-workspace/workspaces"); + + let new_key = "c".repeat(64); + let resp = authed(client().post(format!("{base}/encryption_key"))) + .json(&json!({"new_key": new_key, "skip_reencrypt": true})) + .send() + .await?; + assert_eq!( + resp.status(), + 200, + "set_encryption_key failed: {}", + resp.text().await? + ); + + let (token, secret): (String, String) = sqlx::query_as( + "SELECT git_credentials->0->>'token', git_sync#>>'{repositories,0,auto_pull,webhook_secret}' + FROM workspace_settings WHERE workspace_id = 'test-workspace'", + ) + .fetch_one(&db) + .await?; + let new_mc = crypt_from_key_with_suffix(&new_key, ""); + assert_eq!(decrypt(&new_mc, token)?, "stored-token"); + assert_eq!(decrypt(&new_mc, secret)?, "hook-secret"); + + Ok(()) +} + /// Regression test for the non-debouncing fallback: a workspace whose sync /// script predates hub version 28103 must still receive git-sync jobs for the /// encryption_key entry and every re-encrypted secret. Before the fallback was diff --git a/backend/windmill-api-workspaces/src/workspaces.rs b/backend/windmill-api-workspaces/src/workspaces.rs index eb709c49c2..879babff4f 100644 --- a/backend/windmill-api-workspaces/src/workspaces.rs +++ b/backend/windmill-api-workspaces/src/workspaces.rs @@ -38,7 +38,7 @@ use windmill_common::global_settings::HTTP_ROUTE_WORKSPACED_ROUTE; use windmill_common::query_builders::{render_db_quoted_identifier, DbType}; use windmill_common::users::username_to_permissioned_as; use windmill_common::variables::{ - build_crypt, decrypt, encrypt, SECRET_SALT, WORKSPACE_CRYPT_CACHE, + crypt_from_key_with_suffix, decrypt, encrypt, WORKSPACE_CRYPT_CACHE, }; use windmill_common::worker::{to_raw_value, CLOUD_HOSTED}; use windmill_common::workspaces::GitRepositorySettings; @@ -5690,9 +5690,6 @@ async fn set_encryption_key( )); } - // Build the previous cipher before the transaction (reads from cache/pool) - let previous_encryption_key = build_crypt(&db, w_id.as_str()).await?; - let mut tx = db.begin().await?; // Under the row's lock, so two rotations racing serialize and each sees the key the @@ -5726,17 +5723,14 @@ async fn set_encryption_key( None }; + // From the keys read and written under the lock, never from `build_crypt`: its + // cache can still hold a key an earlier rotation replaced, and the git-sync + // secrets below are skipped rather than failed when they do not decrypt. + let previous_encryption_key = crypt_from_key_with_suffix(&previous_key, ""); + let new_encryption_key = crypt_from_key_with_suffix(&request.new_key, ""); + let mut reencrypted_secret_paths: Vec = Vec::new(); if !request.skip_reencrypt.unwrap_or(false) { - // Build the new cipher directly from the key string, since the transaction - // hasn't committed yet and build_crypt() would read the old key from the pool. - let crypt_key = if let Some(ref salt) = SECRET_SALT.as_ref() { - format!("{}{}", request.new_key, salt) - } else { - request.new_key.clone() - }; - let new_encryption_key = magic_crypt::new_magic_crypt!(crypt_key, 256); - let mut truncated_new_key = request.new_key.clone(); truncated_new_key.truncate(8); tracing::warn!( @@ -5776,6 +5770,14 @@ async fn set_encryption_key( } } + reencrypt_git_sync_secrets( + &mut tx, + &w_id, + &previous_encryption_key, + &new_encryption_key, + ) + .await?; + tx.commit().await?; // Invalidate the cache only after the transaction has committed @@ -5813,6 +5815,64 @@ async fn set_encryption_key( return Ok(()); } +/// Move the git-sync secrets the server keeps under the workspace key (stored +/// repository tokens, webhook secrets) to the new key. They are never synced, so +/// unlike variables they are still under the old key when the caller skips +/// re-encryption. +async fn reencrypt_git_sync_secrets( + conn: &mut sqlx::PgConnection, + w_id: &str, + old: &magic_crypt::MagicCrypt256, + new: &magic_crypt::MagicCrypt256, +) -> Result<()> { + let Some((mut credentials, mut git_sync)) = + sqlx::query_as::<_, (serde_json::Value, Option)>( + "SELECT git_credentials, git_sync FROM workspace_settings WHERE workspace_id = $1 FOR UPDATE", + ) + .bind(w_id) + .fetch_optional(&mut *conn) + .await? + else { + return Ok(()); + }; + let reencrypt = |value: &mut serde_json::Value| { + let Some(ciphertext) = value.as_str() else { + return; + }; + match decrypt(old, ciphertext.to_string()) { + Ok(plain) => *value = serde_json::Value::String(encrypt(new, &plain)), + // Left by an earlier rotation and unrecoverable either way; failing here + // would block every later rotation of the workspace. + Err(e) => tracing::warn!( + "a git-sync secret of workspace {w_id} does not decrypt under its current key, leaving it as is: {e}" + ), + } + }; + for entry in credentials.as_array_mut().into_iter().flatten() { + if let Some(token) = entry.get_mut("token") { + reencrypt(token); + } + } + let repositories = git_sync + .as_mut() + .and_then(|g| g.get_mut("repositories")) + .and_then(|r| r.as_array_mut()); + for repo in repositories.into_iter().flatten() { + if let Some(secret) = repo.pointer_mut("/auto_pull/webhook_secret") { + reencrypt(secret); + } + } + sqlx::query( + "UPDATE workspace_settings SET git_credentials = $2, git_sync = $3 WHERE workspace_id = $1", + ) + .bind(w_id) + .bind(credentials) + .bind(git_sync) + .execute(&mut *conn) + .await?; + Ok(()) +} + #[derive(Serialize)] struct UsedTriggers { pub websocket_used: bool, From 37e493ae66ed5c000ecac492d60fc0fdf4bda71f Mon Sep 17 00:00:00 2001 From: hugocasa Date: Fri, 18 Sep 2026 15:24:00 +0200 Subject: [PATCH 24/25] feat: add an instance setting to refuse a token in MCP URLs (#11162) * feat: add an instance setting to refuse a token in MCP URLs MCP clients are commonly configured with the token in the URL (`/api/mcp/w/{workspace}/mcp?token=...`). A URL-borne credential ends up in browser history, proxy logs and referrers, so an instance can now turn that channel off with the `mcp_disable_token_query_param` global setting and leave the Authorization header as the only way in, which sends MCP clients through the OAuth flow the endpoints already advertise. The rejection is a middleware on both the workspaced and the gateway MCP mounts, layered outside everything that reads a token and inside the WWW-Authenticate layer, so the 401 carries the resource pointer a client needs to start OAuth discovery. Off by default. With it on, the token drawer and the home connect drawer stop offering to mint a token for an MCP URL and hand over the bare URL instead. Co-Authored-By: Claude Opus 5 * fix: read the MCP URL policy when a URL is asked for, and drop the all-workspaces option Two review findings on the token drawer: The policy was read once per page load and cached for the browser session, so a superadmin turning the setting on left every open tab handing out `?token=` URLs the server now refuses. Both entry points now read it when the user actually asks for an MCP URL: when MCP mode is entered, and when the connect drawer opens. The workspace picker offered "All workspaces / Multi-workspace", but the gateway's consent screen binds the token it issues to the one workspace picked there, so OAuth has no multi-workspace grant to hand out. That entry is now token-only. Co-Authored-By: Claude Opus 5 * fix: don't guess the MCP URL policy, and say where the switch lands on restart Review findings: The comment on the settings load claimed `MODE=mcp` as the target deployment, but that mode joins no monitor loop, so the startup pass is its only read and a change lands on restart. That is true of every global setting there, `base_url` included; the comment now says so, and the setting description tells an operator running dedicated MCP servers what to expect. A failed settings probe resolved to "tokens allowed", so with the switch on the drawer would mint a non-expiring token and hand over a URL the server refuses for as long as it exists. The probe now propagates its error and the panel reports it with a retry, creating nothing until the answer is known. The test passed a valid token, so it could not tell a rejection before authentication from one after it. It now also sends a token that was never valid and asserts the middleware's own message, which fails if the layer moves inward. Co-Authored-By: Claude Opus 5 * fix: withhold the MCP URL until a workspace is picked With no persisted workspace the store starts undefined, so opening the drawer from /user/workspaces before choosing one rendered a copyable `/api/mcp/w/undefined/mcp`. It reads like a real URL and a client pointed at it would never connect. The panel now asks for a workspace instead, matching the guard the token branch already has on its generate button. Co-Authored-By: Claude Opus 5 * docs: drop the coverage-status note from the MCP switch test It documented what the test does not reach rather than a constraint the next reader could break; that belongs in the PR, not the module doc. The layer-order rationale, which is what a future edit would break, stays. Co-Authored-By: Claude Opus 5 * refactor: fall back to the bare MCP URL instead of alerting on a failed read When the setting read fails, show the bare URL rather than an error with a retry. It works whichever way the setting is, so no alert is needed, and it still never mints a token for a URL the server may refuse. The connect drawer's wording falls back the same way so the blurb matches the panel. Co-Authored-By: Claude Opus 5 * refactor: move the MCP URL token setting to Core It sat in the Auth/OAuth/SAML list, which the settings sidebar shows under SSO, suggesting a dependency on SSO that does not exist: MCP OAuth has Windmill act as the authorization server, and any login method, password included, completes it. It is an instance-wide credential policy, so it now lives with the other ones in Core, kept out of quick setup like its neighbours. Co-Authored-By: Claude Opus 5 --------- Co-authored-by: Claude Opus 5 --- backend/src/main.rs | 9 +- backend/src/monitor.rs | 27 ++ .../tests/mcp_token_query_param.rs | 102 +++++++ backend/windmill-api-settings/src/lib.rs | 13 +- backend/windmill-api/src/lib.rs | 8 +- backend/windmill-api/src/mcp/core.rs | 30 ++- backend/windmill-api/src/mcp/mod.rs | 2 +- .../windmill-common/src/global_settings.rs | 5 + .../components/home/HomeConnectDrawer.svelte | 15 +- .../src/lib/components/instanceSettings.ts | 9 + .../components/settings/CreateToken.svelte | 255 +++++++++++------- frontend/src/lib/mcpAuth.ts | 20 ++ 12 files changed, 385 insertions(+), 110 deletions(-) create mode 100644 backend/windmill-api-integration-tests/tests/mcp_token_query_param.rs create mode 100644 frontend/src/lib/mcpAuth.ts diff --git a/backend/src/main.rs b/backend/src/main.rs index 316ed099f2..9e7a93cfcd 100644 --- a/backend/src/main.rs +++ b/backend/src/main.rs @@ -52,7 +52,8 @@ use windmill_common::{ INSTANCE_EVENTS_WEBHOOK_SETTING, INSTANCE_PYTHON_VERSION_SETTING, JOB_DEFAULT_TIMEOUT_SECS_SETTING, JOB_ISOLATION_SETTING, JWT_SECRET_SETTING, KEEP_JOB_DIR_SETTING, LICENSE_KEY_SETTING, MAVEN_REPOS_SETTING, MAVEN_SETTINGS_XML_SETTING, - MONITOR_LOGS_ON_OBJECT_STORE_SETTING, NO_DEFAULT_MAVEN_SETTING, + MCP_DISABLE_TOKEN_QUERY_PARAM_SETTING, MONITOR_LOGS_ON_OBJECT_STORE_SETTING, + NO_DEFAULT_MAVEN_SETTING, NPM_CONFIG_REGISTRY_SETTING, NSJAIL_TMPFS_SIZE_MB_SETTING, NSJAIL_TMP_BACKING_SETTING, NUGET_CONFIG_SETTING, OAUTH_SETTING, OTEL_SETTING, OTEL_TRACES_RETENTION_SECS_SETTING, OTEL_TRACING_PROXY_SETTING, PIP_INDEX_URL_SETTING, POWERSHELL_REPO_PAT_SETTING, @@ -126,7 +127,8 @@ use windmill_worker::{ use crate::monitor::{ initial_load, load_concurrency_key_max_queued, load_disable_password_login, - load_fork_workspace_tag_append_fork_suffix, load_keep_job_dir, load_metrics_debug_enabled, + load_fork_workspace_tag_append_fork_suffix, load_keep_job_dir, + load_mcp_disable_token_query_param, load_metrics_debug_enabled, load_preview_tags_override, load_require_preexisting_user, load_retention_period_overrides, load_tag_per_workspace_enabled, load_tag_per_workspace_workspaces, load_workspace_fairness_duration_secs, load_workspace_fairness_enabled, @@ -2164,6 +2166,9 @@ async fn process_notify_event( DISABLE_PASSWORD_LOGIN_SETTING => { load_disable_password_login(db).await; } + MCP_DISABLE_TOKEN_QUERY_PARAM_SETTING => { + load_mcp_disable_token_query_param(db).await; + } EXPOSE_METRICS_SETTING => { tracing::info!("Metrics setting changed, restarting"); spawn_graceful_killpill(tx, db, 30, "metrics setting change", server_mode) diff --git a/backend/src/monitor.rs b/backend/src/monitor.rs index 6b752c0ab4..750bc40560 100644 --- a/backend/src/monitor.rs +++ b/backend/src/monitor.rs @@ -65,6 +65,7 @@ use windmill_common::{ FORK_WORKSPACE_TAG_APPEND_FORK_SUFFIX_SETTING, HUB_API_SECRET_SETTING, HUB_BASE_URL_SETTING, INSTANCE_PYTHON_VERSION_SETTING, JOB_DEFAULT_TIMEOUT_SECS_SETTING, JOB_ISOLATION_SETTING, JWT_SECRET_SETTING, KEEP_JOB_DIR_SETTING, LICENSE_KEY_SETTING, + MCP_DISABLE_TOKEN_QUERY_PARAM, MCP_DISABLE_TOKEN_QUERY_PARAM_SETTING, MONITOR_LOGS_ON_OBJECT_STORE_SETTING, NPMRC_SETTING, NPM_CONFIG_REGISTRY_SETTING, NSJAIL_TMPFS_SIZE_MB_SETTING, NSJAIL_TMP_BACKING_SETTING, NUGET_CONFIG_SETTING, OTEL_SETTING, OTEL_TRACES_RETENTION_SECS_SETTING, OTEL_TRACING_PROXY_SETTING, @@ -288,6 +289,15 @@ pub async fn initial_load( ); if let Some(db) = conn.as_sql() { + // Outside the `server_mode` block below: a `MODE=mcp` process serves the MCP routes + // with `server_mode` false and would otherwise never read this at all. That mode + // joins no monitor loop, so there — as for every global setting, `base_url` + // included — this pass is the only read, and a change lands on restart. + pass.setting( + MCP_DISABLE_TOKEN_QUERY_PARAM_SETTING, + false, + |v| async move { apply_mcp_disable_token_query_param(v) }, + ); pass.setting(DEFAULT_TAGS_PER_WORKSPACE_SETTING, false, |v| async move { apply_tag_per_workspace_enabled(v) }); @@ -1617,6 +1627,23 @@ pub fn apply_disable_password_login(value: Option) { }; } +pub async fn load_mcp_disable_token_query_param(db: &DB) { + match load_value_from_global_settings(db, MCP_DISABLE_TOKEN_QUERY_PARAM_SETTING).await { + Ok(v) => apply_mcp_disable_token_query_param(v), + Err(e) => tracing::error!("Error loading mcp_disable_token_query_param setting: {e:#}"), + }; +} + +pub fn apply_mcp_disable_token_query_param(value: Option) { + match value { + Some(serde_json::Value::Bool(t)) => { + MCP_DISABLE_TOKEN_QUERY_PARAM.store(t, Ordering::Relaxed) + } + None => MCP_DISABLE_TOKEN_QUERY_PARAM.store(false, Ordering::Relaxed), + _ => (), + }; +} + struct LogFile { file_path: String, hostname: String, diff --git a/backend/windmill-api-integration-tests/tests/mcp_token_query_param.rs b/backend/windmill-api-integration-tests/tests/mcp_token_query_param.rs new file mode 100644 index 0000000000..d0d6e070f6 --- /dev/null +++ b/backend/windmill-api-integration-tests/tests/mcp_token_query_param.rs @@ -0,0 +1,102 @@ +//! The `mcp_disable_token_query_param` switch closes the URL-borne credential path. +//! +//! The rejection is a middleware layered between the `WWW-Authenticate` decorator and +//! everything that reads a token, on both the workspaced and the gateway mount. Each half of +//! that sandwich is pinned: the `WWW-Authenticate` header on the refusal catches the layer +//! being moved outward (a client would lose the pointer that starts OAuth discovery), and +//! refusing a token that was never valid catches it being moved inward past authentication +//! (the URL-borne token would be hashed and looked up before anything refused it). +#![cfg(feature = "mcp")] + +use std::sync::atomic::Ordering; + +use serde_json::json; +use sqlx::{Pool, Postgres}; + +use windmill_common::global_settings::MCP_DISABLE_TOKEN_QUERY_PARAM; +use windmill_test_utils::*; + +/// Workspace-less with an `mcp:` scope, which is what the gateway mount requires; the +/// workspaced mount takes its workspace from the path, so one token reaches both. +async fn insert_mcp_token(db: &Pool) -> anyhow::Result<()> { + sqlx::query( + "INSERT INTO token (token_hash, token_prefix, token, email, label, super_admin, scopes) + VALUES (encode(sha256('MCP_TOKEN'::bytea), 'hex'), 'MCP_TOK', 'MCP_TOKEN', 'test@windmill.dev', 'mcp token', true, ARRAY['mcp:all'])", + ) + .execute(db) + .await?; + Ok(()) +} + +/// A token that is not in `token` at all. Authentication would refuse it on its own, so a +/// refusal carrying the middleware's own wording is evidence nothing looked it up first. +const BOGUS_TOKEN: &str = "NOT_A_REAL_TOKEN"; + +async fn tools_list(url: &str) -> anyhow::Result { + Ok(reqwest::Client::new() + .post(url) + .header("Accept", "application/json, text/event-stream") + .json(&json!({ "jsonrpc": "2.0", "id": 1, "method": "tools/list", "params": {} })) + .send() + .await?) +} + +#[sqlx::test(migrations = "../migrations", fixtures("base"))] +async fn test_mcp_token_query_param_switch(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + set_jwt_secret().await; + insert_mcp_token(&db).await?; + let server = ApiServer::start_mcp(db.clone()).await?; + let port = server.addr.port(); + let workspaced = + format!("http://localhost:{port}/api/mcp/w/test-workspace/mcp?token=MCP_TOKEN"); + let gateway = format!("http://localhost:{port}/api/mcp/gateway?token=MCP_TOKEN"); + + assert_eq!( + tools_list(&workspaced).await?.status(), + 200, + "a URL-borne token is the documented default and must keep working while the switch is off" + ); + assert_eq!(tools_list(&gateway).await?.status(), 200); + + MCP_DISABLE_TOKEN_QUERY_PARAM.store(true, Ordering::Relaxed); + + for url in [&workspaced, &gateway] { + let resp = tools_list(url).await?; + assert_eq!( + resp.status(), + 401, + "{url} still admitted a token in the URL" + ); + // What sends the client into the OAuth flow rather than leaving it stuck on a 401. + assert!( + resp.headers().contains_key("www-authenticate"), + "{url} rejected without pointing at the authorization server" + ); + } + + // Refused before authentication, not after: an invalid token gets the middleware's own + // message rather than the generic 401 that looking it up would produce. + let resp = tools_list(&format!( + "http://localhost:{port}/api/mcp/w/test-workspace/mcp?token={BOGUS_TOKEN}" + )) + .await?; + assert_eq!(resp.status(), 401); + assert!( + resp.text().await?.contains("does not accept a token in the MCP URL"), + "an invalid URL token was answered by authentication, so the token was read before \ + the switch refused it" + ); + + // The header stays open: it is the channel the OAuth flow itself hands tokens over on. + let resp = reqwest::Client::new() + .post(format!("http://localhost:{port}/api/mcp/gateway")) + .header("Accept", "application/json, text/event-stream") + .header("Authorization", "Bearer MCP_TOKEN") + .json(&json!({ "jsonrpc": "2.0", "id": 1, "method": "tools/list", "params": {} })) + .send() + .await?; + assert_eq!(resp.status(), 200); + + Ok(()) +} diff --git a/backend/windmill-api-settings/src/lib.rs b/backend/windmill-api-settings/src/lib.rs index 55e22d912f..3bbaf888e2 100644 --- a/backend/windmill-api-settings/src/lib.rs +++ b/backend/windmill-api-settings/src/lib.rs @@ -64,10 +64,11 @@ use windmill_common::{ GITHUB_APP_WEBHOOK_BASE_URL_SETTING, HTTP_ROUTE_DEFAULT_ALLOWED_ORIGINS_SETTING, HTTP_ROUTE_WORKSPACED_ROUTE_SETTING, HUB_ACCESSIBLE_URL_SETTING, HUB_BASE_URL_SETTING, INSTANCE_BANNER_SETTING, MAX_RETENTION_OVERRIDE_WORKSPACES, - MAX_TOKEN_EXPIRATION_DAYS_SETTING, RETENTION_PERIOD_SECS_OVERRIDES_SETTING, - RUFF_CONFIG_SETTING, UNIQUE_ID_SETTING, WORKSPACE_FAIRNESS_DURATION_SECS_SETTING, - WORKSPACE_FAIRNESS_ENABLED_SETTING, WORKSPACE_FAIRNESS_MAX_PERCENT_SETTING, - WORKSPACE_FAIRNESS_MIN_TOTAL_SETTING, WS_BASE_URL_SETTING, + MAX_TOKEN_EXPIRATION_DAYS_SETTING, MCP_DISABLE_TOKEN_QUERY_PARAM_SETTING, + RETENTION_PERIOD_SECS_OVERRIDES_SETTING, RUFF_CONFIG_SETTING, UNIQUE_ID_SETTING, + WORKSPACE_FAIRNESS_DURATION_SECS_SETTING, WORKSPACE_FAIRNESS_ENABLED_SETTING, + WORKSPACE_FAIRNESS_MAX_PERCENT_SETTING, WORKSPACE_FAIRNESS_MIN_TOTAL_SETTING, + WS_BASE_URL_SETTING, }, instance_config::{self, ApplyMode, InstanceConfig}, server::Smtp, @@ -1364,6 +1365,10 @@ pub async fn get_global_setting( && key != INSTANCE_BANNER_SETTING // The token form reads it to stop offering expirations the server would shorten. && key != MAX_TOKEN_EXPIRATION_DAYS_SETTING + // Whoever is wiring up an MCP client reads it to know whether a URL-borne token + // would be refused, and they are usually not a superadmin. Not a secret: pointing + // any MCP client at the instance discovers the same answer. + && key != MCP_DISABLE_TOKEN_QUERY_PARAM_SETTING { require_super_admin(&db, &authed).await?; } diff --git a/backend/windmill-api/src/lib.rs b/backend/windmill-api/src/lib.rs index b70d80140a..0fd5ac31b5 100644 --- a/backend/windmill-api/src/lib.rs +++ b/backend/windmill-api/src/lib.rs @@ -560,7 +560,7 @@ pub async fn run_server( if server_mode || mcp_mode { use mcp::{ add_www_authenticate_header, add_www_authenticate_header_gateway, - extract_workspace_from_token, + extract_workspace_from_token, reject_token_query_param, }; let (mcp_router, mcp_cancellation_token) = setup_mcp_server( db.clone(), @@ -573,15 +573,17 @@ pub async fn run_server( let workspaced_mcp_router = mcp_router .clone() .route_layer(from_extractor::()) + .layer(axum::middleware::from_fn(reject_token_query_param)) .layer(axum::middleware::from_fn(add_www_authenticate_header)) .layer(axum::middleware::from_fn(extract_and_store_workspace_id)); // Gateway MCP router — resolves workspace from token let gateway_mcp_router = mcp_router .route_layer(from_extractor::()) + .layer(axum::middleware::from_fn(extract_workspace_from_token)) + .layer(axum::middleware::from_fn(reject_token_query_param)) .layer(axum::middleware::from_fn( add_www_authenticate_header_gateway, - )) - .layer(axum::middleware::from_fn(extract_workspace_from_token)); + )); ( workspaced_mcp_router, gateway_mcp_router, diff --git a/backend/windmill-api/src/mcp/core.rs b/backend/windmill-api/src/mcp/core.rs index 480e3c0841..86fbec1806 100644 --- a/backend/windmill-api/src/mcp/core.rs +++ b/backend/windmill-api/src/mcp/core.rs @@ -43,11 +43,14 @@ use axum::{ extract::{Extension, Path}, http::Request, middleware::Next, - response::Response, + response::{IntoResponse, Response}, routing::get, Json, Router, }; -use windmill_common::{auth::hash_token, db::GatewayWorkspaceId, error::JsonResult}; +use windmill_common::{ + auth::hash_token, db::GatewayWorkspaceId, error::JsonResult, + global_settings::MCP_DISABLE_TOKEN_QUERY_PARAM, +}; // McpAuth impl for ApiAuthed is in windmill-api-auth (same crate as the type) @@ -446,6 +449,29 @@ pub async fn add_www_authenticate_header( } } +/// Middleware refusing a credential carried in the MCP URL once the instance sets +/// `mcp_disable_token_query_param`. Sits outside everything that reads the token, so neither +/// the gateway lookup nor `ApiAuthed` ever sees it, and inside the `WWW-Authenticate` layer, +/// whose header is what sends the client into the OAuth flow instead. Refused rather than +/// ignored: the URL leaked the token whether or not the request used it. +pub async fn reject_token_query_param(request: Request, next: Next) -> Response { + let carries_token = MCP_DISABLE_TOKEN_QUERY_PARAM.load(std::sync::atomic::Ordering::Relaxed) + && request + .uri() + .query() + .is_some_and(|q| url::form_urlencoded::parse(q.as_bytes()).any(|(k, _)| k == "token")); + if carries_token { + return ( + axum::http::StatusCode::UNAUTHORIZED, + "This instance does not accept a token in the MCP URL. Remove the token query \ + parameter and let your client sign in through OAuth, or send the token in an \ + Authorization header.", + ) + .into_response(); + } + next.run(request).await +} + /// Extract the bearer token from either the `Authorization` header or the /// `?token=` query parameter (MCP clients commonly pass it in the URL). fn extract_gateway_token(request: &Request) -> Option { diff --git a/backend/windmill-api/src/mcp/mod.rs b/backend/windmill-api/src/mcp/mod.rs index 5f6bd5edb5..5545d59a9f 100644 --- a/backend/windmill-api/src/mcp/mod.rs +++ b/backend/windmill-api/src/mcp/mod.rs @@ -12,5 +12,5 @@ pub mod oauth_server; pub use core::{ add_www_authenticate_header, add_www_authenticate_header_gateway, extract_and_store_workspace_id, extract_workspace_from_token, list_tools_service, - setup_mcp_server, + reject_token_query_param, setup_mcp_server, }; diff --git a/backend/windmill-common/src/global_settings.rs b/backend/windmill-common/src/global_settings.rs index b022f9195e..6b9aae519d 100644 --- a/backend/windmill-common/src/global_settings.rs +++ b/backend/windmill-common/src/global_settings.rs @@ -102,6 +102,10 @@ pub const HUB_API_SECRET_SETTING: &str = "hub_api_secret"; pub const AUTOMATE_USERNAME_CREATION_SETTING: &str = "automate_username_creation"; pub const DISABLE_WORKSPACE_INVITE_EMAILS_SETTING: &str = "disable_workspace_invite_emails"; pub const DISABLE_PASSWORD_LOGIN_SETTING: &str = "disable_password_login"; +/// Refuse `?token=` on the MCP endpoints, leaving the `Authorization` header as the only way +/// in. A URL-borne credential ends up in browser history, proxy logs and referrers, so an +/// instance that cares sends MCP clients through the OAuth flow instead. +pub const MCP_DISABLE_TOKEN_QUERY_PARAM_SETTING: &str = "mcp_disable_token_query_param"; /// Ceiling, in days, on how far ahead a token minted through `POST /users/tokens/create` or /// `POST /users/tokens/impersonate` may expire; a request asking for more, or for no /// expiration at all, is shortened to it rather than refused. On those routes only: server-side @@ -407,6 +411,7 @@ use std::sync::atomic::AtomicBool; lazy_static::lazy_static! { pub static ref HTTP_ROUTE_WORKSPACED_ROUTE: AtomicBool = AtomicBool::new(false); pub static ref DISABLE_PASSWORD_LOGIN: AtomicBool = AtomicBool::new(false); + pub static ref MCP_DISABLE_TOKEN_QUERY_PARAM: AtomicBool = AtomicBool::new(false); /// Origins HTTP routes allow cross-origin when they configure none of their /// own. Empty means unset, which keeps the historical `*`. pub static ref HTTP_ROUTE_DEFAULT_ALLOWED_ORIGINS: arc_swap::ArcSwap> = diff --git a/frontend/src/lib/components/home/HomeConnectDrawer.svelte b/frontend/src/lib/components/home/HomeConnectDrawer.svelte index e7e4a306d9..e08ac2ddea 100644 --- a/frontend/src/lib/components/home/HomeConnectDrawer.svelte +++ b/frontend/src/lib/components/home/HomeConnectDrawer.svelte @@ -5,10 +5,12 @@ import CopyableCodeBlock from '$lib/components/details/CopyableCodeBlock.svelte' import { Bot, ExternalLink, Terminal } from 'lucide-svelte' import { shell } from 'svelte-highlight/languages' + import { mcpTokenUrlDisabled } from '$lib/mcpAuth' type ConnectTab = 'cli' | 'mcp' let drawer: Drawer | undefined = $state() + let tokenUrlDisabled = $state(false) let selectedTab: ConnectTab = $state('cli') let openVersion = $state(0) @@ -24,6 +26,10 @@ wmill sync pull`) export function openDrawer(tab: ConnectTab = 'cli') { selectedTab = tab openVersion += 1 + // Falls back like CreateToken below, which shows the bare URL when the read fails. + void mcpTokenUrlDisabled() + .then((v) => (tokenUrlDisabled = v)) + .catch(() => (tokenUrlDisabled = true)) drawer?.openDrawer() } @@ -96,8 +102,13 @@ wmill sync pull`)

MCP URL

- Generate an MCP server URL for the current workspace and choose which - scripts, flows, and endpoints the client can access. + {#if tokenUrlDisabled} + The MCP server URL for the current workspace. Your client signs in to + Windmill to use it. + {:else} + Generate an MCP server URL for the current workspace and choose which + scripts, flows, and endpoints the client can access. + {/if}

diff --git a/frontend/src/lib/components/instanceSettings.ts b/frontend/src/lib/components/instanceSettings.ts index 06e720a846..7335b33848 100644 --- a/frontend/src/lib/components/instanceSettings.ts +++ b/frontend/src/lib/components/instanceSettings.ts @@ -325,6 +325,15 @@ export const settings: Record = { value === null || value === '' || parseMaxTokenExpirationDays(value) !== undefined + }, + { + label: 'Disable token in MCP URLs', + description: + 'Reject the ?token= query parameter on the MCP endpoints, so MCP clients authenticate with an Authorization header or through the OAuth flow. A token in a URL is a credential that ends up in browser history, proxy logs and referrers. Existing MCP URLs carrying a token stop working. Servers and workers pick this up within a minute; dedicated MCP servers (MODE=mcp) apply it when they next restart.', + key: 'mcp_disable_token_query_param', + fieldType: 'boolean', + storage: 'setting', + hideInQuickSetup: true } ], Jobs: [ diff --git a/frontend/src/lib/components/settings/CreateToken.svelte b/frontend/src/lib/components/settings/CreateToken.svelte index acde0a557a..146f844b3f 100644 --- a/frontend/src/lib/components/settings/CreateToken.svelte +++ b/frontend/src/lib/components/settings/CreateToken.svelte @@ -6,12 +6,15 @@ workspaceStore, type UserWorkspace } from '$lib/stores' - import { Button } from '../common' + import { Alert, Button, Skeleton } from '../common' import { triggerableByAI } from '$lib/actions/triggerableByAI.svelte' import Toggle from '../Toggle.svelte' import { SettingService, UserService, type NewToken } from '$lib/gen' + import { mcpTokenUrlDisabled } from '$lib/mcpAuth' import TokenDisplay from './TokenDisplay.svelte' import ScopesPicker from './ScopesPicker.svelte' + import CopyableCodeBlock from '../details/CopyableCodeBlock.svelte' + import { shell } from 'svelte-highlight/languages' import { parseMaxTokenExpirationDays } from '$lib/tokenExpiration' import TextInput from '../text_input/TextInput.svelte' @@ -59,6 +62,22 @@ let pickedScopes = $state(null) let readOnly = $state(false) + // How this instance lets an MCP client in. `oauth` means it refuses `?token=`, so a + // generated token would not get a client in and the URL is handed over bare instead. + // A failed read lands on `oauth`: the bare URL works whichever way the setting is, + // whereas guessing `token` mints a non-expiring credential the server may refuse. + type McpUrlPolicy = 'loading' | 'token' | 'oauth' + let mcpUrlPolicy = $state('loading') + + async function loadMcpUrlPolicy() { + mcpUrlPolicy = 'loading' + try { + mcpUrlPolicy = (await mcpTokenUrlDisabled()) ? 'oauth' : 'token' + } catch (err) { + console.error('Failed to load the MCP token setting:', err) + mcpUrlPolicy = 'oauth' + } + } const DAY_SECS = 24 * 60 * 60 const EXPIRATION_CHOICES = [ @@ -117,6 +136,7 @@ function enterMcpMode() { mcpCreationMode = true + void loadMcpUrlPolicy() resetExpirationOnModeChange() newTokenWorkspace = defaultNewTokenWorkspace ?? $workspaceStore newToken = undefined @@ -224,11 +244,17 @@ const scopeWorkspaceId = $derived( isAllWorkspaces ? $workspaceStore || '' : newTokenWorkspace || $workspaceStore || '' ) - const mcpBaseUrl = $derived( + // Undefined wherever the workspace is: `/api/mcp/w/undefined/mcp` reads like a real URL + // and is copyable, so the OAuth panel withholds it rather than showing a broken one. The + // token branch guards the same case by disabling its generate button. + const mcpUrl = $derived( isAllWorkspaces - ? `${window.location.origin}/api/mcp/gateway?token=` - : `${window.location.origin}/api/mcp/w/${newTokenWorkspace}/mcp?token=` + ? `${window.location.origin}/api/mcp/gateway` + : newTokenWorkspace + ? `${window.location.origin}/api/mcp/w/${newTokenWorkspace}/mcp` + : undefined ) + const mcpBaseUrl = $derived(`${mcpUrl ?? ''}?token=`) $effect(() => { const requestedMcpMode = mcpOnly || openWithMcpMode @@ -256,7 +282,9 @@
-

{title}

+

+ {mcpCreationMode && mcpUrlPolicy !== 'token' ? 'MCP URL' : title} +

{#if showMcpMode && !mcpOnly}
{/if} - {#if scopes != undefined} -
- Scope - {#each scopes as scope (scope)} - - {/each} -
- + {:else if mcpCreationMode && mcpUrlPolicy === 'oauth'} + {#if !lockWorkspace} +
+ Workspace + + ({ label: w.name, value: w.id, subtitle: w.id })) - ]} +
+ + Paste this URL into your client. It opens a Windmill page where you approve the access + it asks for, and no token needs to be copied around. + +
+ {:else} +

Pick a workspace to get its MCP URL.

+ {/if} + + {#if !mcpOnly} +
+ +
+ {/if} + {:else} + {#if scopes != undefined} +
+ Scope + {#each scopes as scope (scope)} + + {/each} +
+ - {#if isAllWorkspaces} +
+
+ {/if} + + {#if !scopes || scopes.length === 0} + + {/if} + +
+ {#if mcpCreationMode} + {#if !lockWorkspace} +
+ Workspace + newTokenExpiration, (v) => (pickedExpiration = v)} + placeholder={maxExpirationSecs == undefined ? 'No expiration' : 'Pick an expiration'} + inputClass="w-full" + items={expirationItems} + /> + {#if maxExpirationSecs != undefined}

- This token works across every workspace you can access. Tools take a - workspace_id argument; call list_workspaces to discover them. + This instance limits tokens to {maxExpirationLabel}.

{/if}
{/if} - {/if} +
- {#if !mcpOnly} -
- Label (optional) + {#if !mcpOnly} +
- {/if} - - {#if !mcpCreationMode || maxExpirationSecs != undefined} -
- - Expires In - {#if maxExpirationSecs == undefined} - (optional) - {/if} - -