From 56dd940e34b146c3ca25de7959b4fb618308eb86 Mon Sep 17 00:00:00 2001 From: AlexRV12 <71396855+AlexRV12@users.noreply.github.com> Date: Mon, 14 Sep 2026 15:55:40 +0200 Subject: [PATCH 1/2] fix: keep a script draft's password marking through the chat's run form (#11110) * fix: keep a script draft's password marking through the chat's run form Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Dx9oMWHnKywoqooZBeaPCy * docs: describe what the inferArgs test mock actually lets the suite pin Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Dx9oMWHnKywoqooZBeaPCy --------- Co-authored-by: Claude Opus 5 (1M context) --- .../copilot/chat/global/core.test.ts | 42 ++++++++++++++++++- .../components/copilot/chat/global/core.ts | 22 ++++++---- .../copilot/chat/global/userDraftAdapter.ts | 1 + 3 files changed, 54 insertions(+), 11 deletions(-) diff --git a/frontend/src/lib/components/copilot/chat/global/core.test.ts b/frontend/src/lib/components/copilot/chat/global/core.test.ts index 90ff8a8f77..5b083fd9f7 100644 --- a/frontend/src/lib/components/copilot/chat/global/core.test.ts +++ b/frontend/src/lib/components/copilot/chat/global/core.test.ts @@ -321,8 +321,9 @@ vi.mock('./rawAppBundlerBridge', () => ({ vi.mock('$lib/infer', async () => ({ ...(await vi.importActual('$lib/infer')), - // Avoid the wasm parser in unit tests: the script deploy path infers the arg - // schema but tolerates failure, and these tests don't assert on the schema. + // Avoid the wasm parser in unit tests. A no-op passes the seeded schema through + // untouched, which is what lets the password-marking test pin how a schema is + // seeded in and carried out without pinning inference's own merge rules. inferArgs: vi.fn(async () => {}) })) @@ -4358,6 +4359,43 @@ describe('global AI tools', () => { expect(result).toContain('test logs') }) + // No parser emits `password`, so the stored schema is the only thing carrying it: an edit + // that rewrites the draft's schema from scratch, or a draft read that drops it, unmarks + // the field — and the form then takes the secret as a plain literal into the job's args. + it('test_run_script keeps the password marking of the script it previews', async () => { + vi.mocked(ScriptService.existsScriptByPath).mockResolvedValueOnce(true) + vi.mocked(ScriptService.getScriptByPath).mockResolvedValueOnce({ + path: 'f/scripts/secretful', + language: 'bun', + schema: { + type: 'object', + properties: { token: { type: 'string', password: true } }, + required: ['token'] + } + } as any) + + await callGlobalTool('write_script', { + path: 'f/scripts/secretful', + language: 'bun', + content: 'export async function main(token: string) { return 1 }' + }) + + let form: any + await callGlobalTool( + 'test_run_script', + { path: 'f/scripts/secretful' }, + { + ...toolCallbacks, + requestRunArgs: async (_toolId, opened) => { + form = opened + return undefined + } + } + ) + + expect(form?.schema?.properties).toMatchObject({ token: { password: true } }) + }) + it('test_run_script previews deployed script content when no draft exists', async () => { vi.mocked(ScriptService.getScriptByPath).mockResolvedValueOnce({ path: 'f/scripts/deployed-test', diff --git a/frontend/src/lib/components/copilot/chat/global/core.ts b/frontend/src/lib/components/copilot/chat/global/core.ts index 5883c5ae0d..9d459bcf71 100644 --- a/frontend/src/lib/components/copilot/chat/global/core.ts +++ b/frontend/src/lib/components/copilot/chat/global/core.ts @@ -148,6 +148,7 @@ import type { SessionArtifactsStore } from '../artifacts/artifactsState.svelte' import type { Runnable } from '$lib/components/apps/inputType' import { UserDraft } from '$lib/userDraft.svelte' import { emptySchema } from '$lib/utils' +import type { Schema } from '$lib/common' import { inferArgs } from '$lib/infer' import { resourceRequestSchema, @@ -3424,7 +3425,8 @@ export const globalTools: Tool<{}>[] = [ draftCountByType.set(draft.type, count + 1) byKey.set(getWorkspaceItemKey(draft.type, draft.path, draft.triggerKind), { ...draft, - value: undefined + value: undefined, + schema: undefined }) } } @@ -5033,11 +5035,14 @@ const SCRIPT_SPEC: WriteSpec = { language: args.language, kind: 'script' } - // Infer the arg schema from the content at save time, like the editor does, - // so the persisted draft is the single source of truth at deploy. Keep the - // previous schema (or empty) on failure rather than blanking it. + // Into the schema the base carries, as the editor does at save: `inferArgs` re-seeds + // each arg from the properties it is handed, and those are the only copy of + // `password`, enums, formats and titles — no parser emits them. A clone, so a parse + // failure leaves the previous schema rather than half of one. try { - const schema = emptySchema() + const schema = structuredClone( + draft.schema?.properties ? draft.schema : emptySchema() + ) as Schema await inferArgs(draft.language, draft.content, schema) draft.schema = schema } catch (e) { @@ -5224,10 +5229,9 @@ async function loadScriptForEdit( } } -/** The fields a test form offers, for code that may never have been deployed. A draft the - * chat wrote carries the schema it inferred at write time; anything else — a draft written - * elsewhere, a deployed script whose schema predates an edit — is inferred here from the - * content that is about to run, so the form cannot offer a field the code no longer takes. */ +/** The fields a test form offers, for code that may never have been deployed. The stored + * schema wins wherever it declares fields — a draft's or the deployed script's; only one + * declaring nothing is inferred here, from the content about to run. */ async function schemaForTestRun(script: { content: string language: ScriptLang diff --git a/frontend/src/lib/components/copilot/chat/global/userDraftAdapter.ts b/frontend/src/lib/components/copilot/chat/global/userDraftAdapter.ts index f2c3f02431..7593546114 100644 --- a/frontend/src/lib/components/copilot/chat/global/userDraftAdapter.ts +++ b/frontend/src/lib/components/copilot/chat/global/userDraftAdapter.ts @@ -141,6 +141,7 @@ function scriptDraftToWorkspaceItem(path: string, draft: NewScript): WorkspaceIt summary: draft.summary, language: draft.language, value: draft.content, + schema: draft.schema, parentHash: draft.parent_hash, isDraft: true } From 244ec132914a6e689d7d79da9cf2cb39f920aaef Mon Sep 17 00:00:00 2001 From: Guilhem Date: Mon, 14 Sep 2026 15:55:52 +0200 Subject: [PATCH 2/2] fix(ai-chat): hide other users' MCP servers from the chat unless shared (#11112) * fix(ai-chat): hide other users' MCP servers from the chat unless shared An admin's database role lets the resource listing return every user's u/ MCP resource, so the chat's "+" menu and the assistant settings tab offered servers that carry someone else's credentials. Both lists, and the tool loader behind them, now keep a u/ server only when it belongs to the current user or its extra_perms name them or one of their groups. The Resources page is unchanged. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_0131DsyTiBR6Q54qPXbTX5sC * fix(ai-chat): resolve the MCP viewer for the workspace being listed Username and groups are per workspace, and a session chat can operate on a workspace other than the one being browsed, so the filter now takes its identity from that workspace's whoami rather than from userStore. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_0131DsyTiBR6Q54qPXbTX5sC * fix(ai-chat): list the whole MCP catalog before filtering, one predicate The visibility filter runs after the server's LIMIT, so a 100-row page could drop the viewer's own servers behind foreign u/ rows. The three call sites now ask for 1000 and call the tested predicate directly. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_0131DsyTiBR6Q54qPXbTX5sC --------- Co-authored-by: Claude Fable 5.1 --- .../copilot/chat/AssistantMcpSection.svelte | 19 +++++-- .../copilot/chat/global/mcpTools.test.ts | 2 +- .../copilot/chat/global/mcpTools.ts | 18 ++++--- .../src/lib/components/mcp/mcpMenu.svelte.ts | 26 +++++---- .../src/lib/components/mcp/ownServers.test.ts | 53 +++++++++++++++++++ frontend/src/lib/components/mcp/ownServers.ts | 48 +++++++++++++++++ 6 files changed, 144 insertions(+), 22 deletions(-) create mode 100644 frontend/src/lib/components/mcp/ownServers.test.ts create mode 100644 frontend/src/lib/components/mcp/ownServers.ts diff --git a/frontend/src/lib/components/copilot/chat/AssistantMcpSection.svelte b/frontend/src/lib/components/copilot/chat/AssistantMcpSection.svelte index 0ab2812b2e..c8d360e0f9 100644 --- a/frontend/src/lib/components/copilot/chat/AssistantMcpSection.svelte +++ b/frontend/src/lib/components/copilot/chat/AssistantMcpSection.svelte @@ -14,6 +14,11 @@ switch that decides whether this chat carries its tools. import DropdownV2 from '$lib/components/DropdownV2.svelte' import Toggle from '$lib/components/Toggle.svelte' import { isMcpEnabled, setMcpEnabled } from '$lib/components/mcp/enabledServers' + import { + isOwnOrSharedMcpPath, + MCP_LIST_PER_PAGE, + mcpViewer + } from '$lib/components/mcp/ownServers' import { loadProviderIcon } from '$lib/components/mcp/providerIcon' import { cachedProviderKey, @@ -275,11 +280,15 @@ switch that decides whether this chat carries its tools. loading = true loadError = undefined try { - const resources = await ResourceService.listResource({ - workspace: target, - resourceType: 'mcp', - perPage: 100 - }) + const [listed, viewer] = await Promise.all([ + ResourceService.listResource({ + workspace: target, + resourceType: 'mcp', + perPage: MCP_LIST_PER_PAGE + }), + mcpViewer(target) + ]) + const resources = listed.filter((r) => isOwnOrSharedMcpPath(r.path, r.extra_perms, viewer)) if (seq !== loadSeq) return servers = resources.map((r) => ({ path: r.path, diff --git a/frontend/src/lib/components/copilot/chat/global/mcpTools.test.ts b/frontend/src/lib/components/copilot/chat/global/mcpTools.test.ts index e5d3af8b36..9e1b46589c 100644 --- a/frontend/src/lib/components/copilot/chat/global/mcpTools.test.ts +++ b/frontend/src/lib/components/copilot/chat/global/mcpTools.test.ts @@ -4,7 +4,7 @@ const { getMcpToolsMock, callMcpToolMock, listResourceMock, session } = vi.hoist getMcpToolsMock: vi.fn(), callMcpToolMock: vi.fn(), listResourceMock: vi.fn(), - session: { email: 'first@windmill.dev' } + session: { email: 'first@windmill.dev', workspace_id: 'test-ws', username: 'hugo', pgroups: [] } })) vi.mock('../shared', () => ({ diff --git a/frontend/src/lib/components/copilot/chat/global/mcpTools.ts b/frontend/src/lib/components/copilot/chat/global/mcpTools.ts index 6fd59e4a07..c1db1cac1b 100644 --- a/frontend/src/lib/components/copilot/chat/global/mcpTools.ts +++ b/frontend/src/lib/components/copilot/chat/global/mcpTools.ts @@ -2,6 +2,7 @@ import { z } from 'zod' import { ResourceService, type GetMcpToolsResponse } from '$lib/gen' import { createToolDef, type Tool } from '../shared' import { enabledMcpPaths } from '$lib/components/mcp/enabledServers' +import { isOwnOrSharedMcpPath, MCP_LIST_PER_PAGE, mcpViewer } from '$lib/components/mcp/ownServers' /** * Access to the MCP servers the user has connected (resources of type `mcp`) @@ -91,13 +92,18 @@ export async function loadMcpServers(workspace: string): Promise { const enabled = enabledMcpPaths(workspace) if (enabled.length === 0) return [] try { - const resources = await ResourceService.listResource({ - workspace, - resourceType: 'mcp', - perPage: 100 - }) + const [resources, viewer] = await Promise.all([ + ResourceService.listResource({ + workspace, + resourceType: 'mcp', + perPage: MCP_LIST_PER_PAGE + }), + mcpViewer(workspace) + ]) return resources - .filter((r) => enabled.includes(r.path)) + .filter( + (r) => enabled.includes(r.path) && isOwnOrSharedMcpPath(r.path, r.extra_perms, viewer) + ) .map((r) => ({ path: r.path, editedAt: r.edited_at })) } catch (e) { console.error('Failed to load MCP servers', e) diff --git a/frontend/src/lib/components/mcp/mcpMenu.svelte.ts b/frontend/src/lib/components/mcp/mcpMenu.svelte.ts index aea4a7b114..a911d71c07 100644 --- a/frontend/src/lib/components/mcp/mcpMenu.svelte.ts +++ b/frontend/src/lib/components/mcp/mcpMenu.svelte.ts @@ -8,6 +8,7 @@ import type { Item } from '$lib/utils' import type { AIChatManager } from '../copilot/chat/AIChatManager.svelte' import { isMcpEnabled, setMcpEnabled } from './enabledServers' import { cachedProviderKey, rememberProviderKey } from './iconCache' +import { isOwnOrSharedMcpPath, MCP_LIST_PER_PAGE, mcpViewer } from './ownServers' import { loadProviderIcon } from './providerIcon' import McpServerIcon from './McpServerIcon.svelte' @@ -56,17 +57,22 @@ export class McpMenu { async #load(ws: string) { const seq = ++this.#seq try { - const resources = await ResourceService.listResource({ - workspace: ws, - resourceType: 'mcp', - perPage: 100 - }) + const [resources, viewer] = await Promise.all([ + ResourceService.listResource({ + workspace: ws, + resourceType: 'mcp', + perPage: MCP_LIST_PER_PAGE + }), + mcpViewer(ws) + ]) if (seq !== this.#seq) return - this.#rows = resources.map((r) => ({ - path: r.path, - editedAt: r.edited_at, - enabled: isMcpEnabled(ws, r.path) - })) + this.#rows = resources + .filter((r) => isOwnOrSharedMcpPath(r.path, r.extra_perms, viewer)) + .map((r) => ({ + path: r.path, + editedAt: r.edited_at, + enabled: isMcpEnabled(ws, r.path) + })) this.#rowsWorkspace = ws void this.#loadIcons(ws, seq) } catch { diff --git a/frontend/src/lib/components/mcp/ownServers.test.ts b/frontend/src/lib/components/mcp/ownServers.test.ts new file mode 100644 index 0000000000..d6982790ba --- /dev/null +++ b/frontend/src/lib/components/mcp/ownServers.test.ts @@ -0,0 +1,53 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { session, roleMock } = vi.hoisted(() => ({ + session: { workspace_id: 'browsed', username: 'alice', pgroups: ['g/browsed-only'] }, + roleMock: vi.fn() +})) + +vi.mock('$lib/stores', () => ({ + userStore: { subscribe: (run: (v: unknown) => void) => (run({ ...session }), () => {}) } +})) +vi.mock('$lib/user', () => ({ getWorkspaceRole: roleMock })) + +import { isOwnOrSharedMcpPath, mcpViewer } from './ownServers' + +const alice = { username: 'alice', pgroups: ['g/eng'] } + +describe('isOwnOrSharedMcpPath', () => { + it('keeps own, folder, and explicitly shared servers; drops the rest of u/', () => { + expect(isOwnOrSharedMcpPath('u/alice/notion', {}, alice)).toBe(true) + expect(isOwnOrSharedMcpPath('f/team/notion', {}, alice)).toBe(true) + expect(isOwnOrSharedMcpPath('u/bob/notion', {}, alice)).toBe(false) + expect(isOwnOrSharedMcpPath('u/alicex/notion', {}, alice)).toBe(false) + expect(isOwnOrSharedMcpPath('u/bob/notion', { 'u/alice': false }, alice)).toBe(true) + expect(isOwnOrSharedMcpPath('u/bob/notion', { 'g/eng': false }, alice)).toBe(true) + expect(isOwnOrSharedMcpPath('u/bob/notion', { 'g/ops': true }, alice)).toBe(false) + expect(isOwnOrSharedMcpPath('u/alice/notion', {}, { username: undefined, pgroups: [] })).toBe( + false + ) + }) +}) + +describe('mcpViewer', () => { + beforeEach(() => roleMock.mockReset()) + + it('answers from the store only for the browsed workspace', async () => { + expect(await mcpViewer('browsed')).toEqual({ username: 'alice', pgroups: ['g/browsed-only'] }) + expect(roleMock).not.toHaveBeenCalled() + }) + + it('looks the identity up for another workspace instead of reusing the browsed one', async () => { + roleMock.mockResolvedValue({ + kind: 'resolved', + user: { username: 'alice_other', pgroups: ['g/eng'] } + }) + expect(await mcpViewer('other')).toEqual({ username: 'alice_other', pgroups: ['g/eng'] }) + expect(roleMock).toHaveBeenCalledWith('other') + }) + + it('yields no identity when the lookup fails', async () => { + roleMock.mockResolvedValue({ kind: 'lookup_failed' }) + expect(await mcpViewer('other')).toEqual({ username: undefined, pgroups: [] }) + }) +}) diff --git a/frontend/src/lib/components/mcp/ownServers.ts b/frontend/src/lib/components/mcp/ownServers.ts new file mode 100644 index 0000000000..31c860a039 --- /dev/null +++ b/frontend/src/lib/components/mcp/ownServers.ts @@ -0,0 +1,48 @@ +import { get } from 'svelte/store' +import { userStore } from '$lib/stores' +import { getWorkspaceRole } from '$lib/user' + +/** The principals an `extra_perms` entry can name to grant this user access. */ +export type McpViewer = { username: string | undefined; pgroups: string[] } + +/** Wide enough that a workspace's whole MCP catalog arrives at once: the filter + * below runs after the server's LIMIT, so a short page could drop the viewer's + * own rows while foreign `u/` rows fill it. */ +export const MCP_LIST_PER_PAGE = 1000 + +/** + * Whether the chat lists this MCP server. A server under another user's `u/` + * prefix carries that person's credentials, and an admin's database role lets + * the resource listing return it. Acting through it would call the provider + * as them, so the chat only offers it when its owner shared it explicitly. + */ +export function isOwnOrSharedMcpPath( + path: string, + extraPerms: Record | undefined, + viewer: McpViewer +): boolean { + if (!path.startsWith('u/')) return true + if (viewer.username !== undefined && path.startsWith(`u/${viewer.username}/`)) return true + if (!extraPerms) return false + return ( + (viewer.username !== undefined && `u/${viewer.username}` in extraPerms) || + viewer.pgroups.some((g) => g in extraPerms) + ) +} + +/** + * The viewer's identity in `workspace`. Username and groups are per workspace, + * and a session chat can operate on a workspace other than the one being + * browsed, so `userStore` only answers when it describes that same workspace. + * A failed lookup yields no username: every `u/` server is then hidden rather + * than judged against another workspace's identity. + */ +export async function mcpViewer(workspace: string): Promise { + const u = get(userStore) + if (u?.workspace_id === workspace) return { username: u.username, pgroups: u.pgroups ?? [] } + const role = await getWorkspaceRole(workspace) + if (role.kind === 'resolved') { + return { username: role.user.username, pgroups: role.user.pgroups ?? [] } + } + return { username: undefined, pgroups: [] } +}