diff --git a/packages/@n8n/ai-utilities/src/__tests__/utils/n8n-llm-tracing.test.ts b/packages/@n8n/ai-utilities/src/__tests__/utils/n8n-llm-tracing.test.ts index 58a463e63fd..b16ac5a60e8 100644 --- a/packages/@n8n/ai-utilities/src/__tests__/utils/n8n-llm-tracing.test.ts +++ b/packages/@n8n/ai-utilities/src/__tests__/utils/n8n-llm-tracing.test.ts @@ -156,6 +156,244 @@ describe('N8nLlmTracing', () => { expect(estimateTokensFromStringList).toHaveBeenCalledWith(prompts, 'gpt-4o'); }); + it('should mask declared header values in persisted input data', async () => { + const tracer = new N8nLlmTracing(mockExecutionFunctions, { + redactedHeaders: ['x-secret-header'], + }); + + const llm: Serialized = { + lc: 1, + type: 'constructor', + id: ['langchain', 'chat_models', 'openai'], + kwargs: { + model: 'gpt-4', + configuration: { + baseURL: 'https://api.openai.com/v1', + defaultHeaders: { + 'User-Agent': 'n8n', + 'x-secret-header': 'My_secret_API_key123456789', + }, + }, + }, + }; + + await tracer.handleLLMStart(llm, ['hello'], 'run-123'); + + const inputArg = mockExecutionFunctions.addInputData.mock.calls[0][1] as Array< + Array<{ json: { options: { configuration: { defaultHeaders: Record } } } }> + >; + const persistedHeaders = inputArg[0][0].json.options.configuration.defaultHeaders; + + // declared header name stays, value is masked + expect(persistedHeaders['x-secret-header']).toBe('**********'); + expect(persistedHeaders['x-secret-header']).not.toBe('My_secret_API_key123456789'); + // non-declared header is untouched + expect(persistedHeaders['User-Agent']).toBe('n8n'); + + // stored run details are masked the same way + const storedOptions = tracer.runsMap['run-123'].options as { + configuration: { defaultHeaders: Record }; + }; + expect(storedOptions.configuration.defaultHeaders['x-secret-header']).toBe('**********'); + + // The original serialized object is not mutated + expect( + (llm.kwargs.configuration as { defaultHeaders: Record }).defaultHeaders[ + 'x-secret-header' + ], + ).toBe('My_secret_API_key123456789'); + }); + + it('should mask declared header values inside clientOptions', async () => { + const tracer = new N8nLlmTracing(mockExecutionFunctions, { + redactedHeaders: ['x-secret-header'], + }); + + const llm: Serialized = { + lc: 1, + type: 'constructor', + id: ['langchain', 'chat_models', 'anthropic'], + kwargs: { + model: 'claude', + clientOptions: { + defaultHeaders: { + 'User-Agent': 'n8n', + 'x-secret-header': 'My_secret_API_key123456789', + }, + }, + }, + }; + + await tracer.handleLLMStart(llm, ['hello'], 'run-123'); + + const inputArg = mockExecutionFunctions.addInputData.mock.calls[0][1] as Array< + Array<{ json: { options: { clientOptions: { defaultHeaders: Record } } } }> + >; + const persistedHeaders = inputArg[0][0].json.options.clientOptions.defaultHeaders; + + expect(persistedHeaders['x-secret-header']).toBe('**********'); + expect(persistedHeaders['User-Agent']).toBe('n8n'); + }); + + it('should always mask the Authorization header value', async () => { + const tracer = new N8nLlmTracing(mockExecutionFunctions); + + const llm: Serialized = { + lc: 1, + type: 'constructor', + id: ['langchain', 'chat_models', 'openai'], + kwargs: { + model: 'llama3', + // top-level header map and nested wrapper + headers: { authorization: 'Bearer top-secret', Cookie: 'session=abc' }, + configuration: { + defaultHeaders: { Authorization: 'Bearer nested-secret', 'x-api-key': 'sk-123' }, + }, + }, + }; + + await tracer.handleLLMStart(llm, ['hello'], 'run-123'); + + const inputArg = mockExecutionFunctions.addInputData.mock.calls[0][1] as Array< + Array<{ + json: { + options: { + headers: Record; + configuration: { defaultHeaders: Record }; + }; + }; + }> + >; + const persisted = inputArg[0][0].json.options; + // Matched case-insensitively, in both container shapes + expect(persisted.headers.authorization).toBe('**********'); + expect(persisted.headers.Cookie).toBe('**********'); + expect(persisted.configuration.defaultHeaders.Authorization).toBe('**********'); + expect(persisted.configuration.defaultHeaders['x-api-key']).toBe('**********'); + }); + + it('should mask the Authorization header value at any nesting depth', async () => { + const tracer = new N8nLlmTracing(mockExecutionFunctions); + + const llm: Serialized = { + lc: 1, + type: 'constructor', + id: ['langchain', 'chat_models', 'openai'], + kwargs: { + configuration: { httpAgent: { headers: { Authorization: 'Bearer deep-secret' } } }, + }, + }; + + await tracer.handleLLMStart(llm, ['hello'], 'run-123'); + + const inputArg = mockExecutionFunctions.addInputData.mock.calls[0][1] as Array< + Array<{ + json: { options: { configuration: { httpAgent: { headers: Record } } } }; + }> + >; + expect(inputArg[0][0].json.options.configuration.httpAgent.headers.Authorization).toBe( + '**********', + ); + }); + + it('should redact header values inside an array element', async () => { + const tracer = new N8nLlmTracing(mockExecutionFunctions); + + const llm: Serialized = { + lc: 1, + type: 'constructor', + id: ['langchain', 'chat_models', 'openai'], + kwargs: { + transports: [{ headers: { Authorization: 'Bearer array-secret' } }], + }, + }; + + await tracer.handleLLMStart(llm, ['hello'], 'run-123'); + + const inputArg = mockExecutionFunctions.addInputData.mock.calls[0][1] as Array< + Array<{ json: { options: { transports: Array<{ headers: Record }> } } }> + >; + expect(inputArg[0][0].json.options.transports[0].headers.Authorization).toBe('**********'); + }); + + it('should match a declared header name case-insensitively', async () => { + const tracer = new N8nLlmTracing(mockExecutionFunctions, { + redactedHeaders: ['X-Secret-Header'], + }); + + const llm: Serialized = { + lc: 1, + type: 'constructor', + id: ['langchain', 'chat_models', 'openai'], + kwargs: { + configuration: { defaultHeaders: { 'x-secret-header': 'My_secret_API_key123456789' } }, + }, + }; + + await tracer.handleLLMStart(llm, ['hello'], 'run-123'); + + const inputArg = mockExecutionFunctions.addInputData.mock.calls[0][1] as Array< + Array<{ json: { options: { configuration: { defaultHeaders: Record } } } }> + >; + expect(inputArg[0][0].json.options.configuration.defaultHeaders['x-secret-header']).toBe( + '**********', + ); + }); + + it('should not mask a non-header field that shares a declared name', async () => { + const tracer = new N8nLlmTracing(mockExecutionFunctions, { + redactedHeaders: ['model'], + }); + + const llm: Serialized = { + lc: 1, + type: 'constructor', + id: ['langchain', 'chat_models', 'openai'], + kwargs: { + model: 'gpt-4', + configuration: { defaultHeaders: { model: 'header-value' } }, + }, + }; + + await tracer.handleLLMStart(llm, ['hello'], 'run-123'); + + const inputArg = mockExecutionFunctions.addInputData.mock.calls[0][1] as Array< + Array<{ + json: { + options: { model: string; configuration: { defaultHeaders: Record } }; + }; + }> + >; + const persisted = inputArg[0][0].json.options; + // Top-level model field stays; only the header-container occurrence is masked + expect(persisted.model).toBe('gpt-4'); + expect(persisted.configuration.defaultHeaders.model).toBe('**********'); + }); + + it('should leave headers unchanged when nothing is declared', async () => { + const tracer = new N8nLlmTracing(mockExecutionFunctions); + + const llm: Serialized = { + lc: 1, + type: 'constructor', + id: ['langchain', 'chat_models', 'openai'], + kwargs: { + configuration: { + defaultHeaders: { 'x-secret-header': 'My_secret_API_key123456789' }, + }, + }, + }; + + await tracer.handleLLMStart(llm, ['hello'], 'run-123'); + + const inputArg = mockExecutionFunctions.addInputData.mock.calls[0][1] as Array< + Array<{ json: { options: { configuration: { defaultHeaders: Record } } } }> + >; + expect(inputArg[0][0].json.options.configuration.defaultHeaders['x-secret-header']).toBe( + 'My_secret_API_key123456789', + ); + }); + it('should use parent run index when set', async () => { const tracer = new N8nLlmTracing(mockExecutionFunctions); tracer.setParentRunIndex(5); diff --git a/packages/@n8n/ai-utilities/src/utils/n8n-llm-tracing.ts b/packages/@n8n/ai-utilities/src/utils/n8n-llm-tracing.ts index 23c285e7a2e..f9fb8009f10 100644 --- a/packages/@n8n/ai-utilities/src/utils/n8n-llm-tracing.ts +++ b/packages/@n8n/ai-utilities/src/utils/n8n-llm-tracing.ts @@ -13,6 +13,7 @@ import type { IDataObject, ISupplyDataFunctions, JsonObject } from 'n8n-workflow import { NodeConnectionTypes, NodeError, NodeOperationError } from 'n8n-workflow'; import { logAiEvent } from './log-ai-event'; +import { redactHeaderValues } from './redact-headers'; import { estimateTokensFromStringList } from './tokenizer/token-estimator'; /** Normalized token usage returned by TokensUsageParser. */ @@ -87,6 +88,7 @@ export class N8nLlmTracing extends BaseCallbackHandler { options: { tokensUsageParser: TokensUsageParser; errorDescriptionMapper: (error: NodeError) => string | null | undefined; + redactedHeaders?: string[]; } = { // Default(OpenAI format) parser tokensUsageParser: (result: LLMResult) => { @@ -112,6 +114,7 @@ export class N8nLlmTracing extends BaseCallbackHandler { options?: { tokensUsageParser?: TokensUsageParser; errorDescriptionMapper?: (error: NodeError) => string; + redactedHeaders?: string[]; }, ) { super(); @@ -216,7 +219,10 @@ export class N8nLlmTracing extends BaseCallbackHandler { ? this.#parentRunIndex + this.executionFunctions.getNextRunIndex() : undefined; - const options = llm.type === 'constructor' ? llm.kwargs : llm; + const options = redactHeaderValues( + llm.type === 'constructor' ? llm.kwargs : llm, + this.options.redactedHeaders ?? [], + ); const { index } = this.executionFunctions.addInputData( this.connectionType, [ diff --git a/packages/@n8n/ai-utilities/src/utils/redact-headers.ts b/packages/@n8n/ai-utilities/src/utils/redact-headers.ts new file mode 100644 index 00000000000..50fa81fa49d --- /dev/null +++ b/packages/@n8n/ai-utilities/src/utils/redact-headers.ts @@ -0,0 +1,67 @@ +import type { SerializedFields } from '@langchain/core/dist/load/map_keys'; +import type { SerializedNotImplemented, SerializedSecret } from '@langchain/core/load/serializable'; +import { isRecord } from '@n8n/utils/is-record'; + +type SerializedOptions = SerializedSecret | SerializedNotImplemented | SerializedFields; + +const REDACTED_HEADER_VALUE = '**********'; + +const HEADER_CONTAINER_KEYS = ['defaultheaders', 'headers']; + +const ALWAYS_REDACTED_HEADERS = ['authorization', 'x-api-key', 'api-key', 'cookie']; + +function maskHeaderContainer( + headers: Record, + lowerHeaderNames: string[], +): Record { + const masked = { ...headers }; + let changed = false; + for (const key of Object.keys(headers)) { + const lowerKey = key.toLowerCase(); + if (lowerHeaderNames.includes(lowerKey) || ALWAYS_REDACTED_HEADERS.includes(lowerKey)) { + masked[key] = REDACTED_HEADER_VALUE; + changed = true; + } + } + return changed ? masked : headers; +} + +function redactValue(value: unknown, lowerHeaderNames: string[]): unknown { + if (Array.isArray(value)) { + let result: unknown[] | undefined; + value.forEach((item, index) => { + const masked = redactValue(item, lowerHeaderNames); + if (masked !== item) { + result = result ?? [...value]; + result[index] = masked; + } + }); + return result ?? value; + } + + if (!isRecord(value)) return value; + + let result: Record | undefined; + for (const [key, child] of Object.entries(value)) { + const masked = + HEADER_CONTAINER_KEYS.includes(key.toLowerCase()) && isRecord(child) + ? maskHeaderContainer(child, lowerHeaderNames) + : redactValue(child, lowerHeaderNames); + if (masked !== child) { + result = result ?? { ...value }; + result[key] = masked; + } + } + return result ?? value; +} + +export function redactHeaderValues( + options: SerializedOptions, + headerNames: string[], +): SerializedOptions { + const redacted = redactValue( + options, + headerNames.map((name) => name.toLowerCase()), + ); + return isRecord(redacted) ? redacted : options; +} diff --git a/packages/@n8n/backend-common/src/index.ts b/packages/@n8n/backend-common/src/index.ts index dd948818ebd..65fc48d6f30 100644 --- a/packages/@n8n/backend-common/src/index.ts +++ b/packages/@n8n/backend-common/src/index.ts @@ -7,7 +7,13 @@ export { Logger } from './logging/logger'; export { ModuleRegistry } from './modules/module-registry'; export type { ModuleName } from './modules/modules.config'; export { ModulesConfig } from './modules/modules.config'; -export { isContainedWithin, safeJoinPath } from './utils/path-util'; +export { + isContainedWithin, + safeJoinPath, + pathComponents, + pathSegmentsBetween, + containsSymlinkedComponent, +} from './utils/path-util'; export { assertDir, exists } from './utils/fs'; export { parseFlatted } from './utils/parse-flatted'; export { CliParser } from './cli-parser'; diff --git a/packages/@n8n/backend-common/src/utils/__tests__/path-util.test.ts b/packages/@n8n/backend-common/src/utils/__tests__/path-util.test.ts index 9eb11347a94..699d2a30883 100644 --- a/packages/@n8n/backend-common/src/utils/__tests__/path-util.test.ts +++ b/packages/@n8n/backend-common/src/utils/__tests__/path-util.test.ts @@ -1,4 +1,14 @@ -import { isContainedWithin, safeJoinPath } from '../path-util'; +import { mkdtemp, mkdir, symlink, rm, realpath, chmod } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; + +import { + isContainedWithin, + safeJoinPath, + pathComponents, + pathSegmentsBetween, + containsSymlinkedComponent, +} from '../path-util'; describe('isContainedWithin', () => { it('should return true when parent and child paths are the same', () => { @@ -51,3 +61,84 @@ describe('safeJoinPath', () => { expect(() => safeJoinPath('/foo/bar', '..')).toThrow('Path traversal detected'); }); }); + +describe('pathComponents', () => { + it('returns each component of an absolute path from the root down', () => { + expect(pathComponents('/a/b/c')).toEqual(['/a', '/a/b', '/a/b/c']); + }); + + it('ignores empty segments and trailing slashes', () => { + expect(pathComponents('/a//b/')).toEqual(['/a', '/a/b']); + }); + + it('returns an empty list for the root path', () => { + expect(pathComponents('/')).toEqual([]); + }); + + it('builds each component of a relative path', () => { + expect(pathComponents('a/b')).toEqual(['a', 'a/b']); + }); +}); + +describe('pathSegmentsBetween', () => { + it('returns the segments of a descendant below an ancestor', () => { + expect(pathSegmentsBetween('/a', '/a/b/c')).toEqual(['b', 'c']); + }); + + it('returns an empty list when the paths are equal', () => { + expect(pathSegmentsBetween('/a/b', '/a/b')).toEqual([]); + }); + + it('returns null when the descendant is not contained within the ancestor', () => { + expect(pathSegmentsBetween('/a/b', '/a/c')).toBeNull(); + expect(pathSegmentsBetween('/a/b', '/a/b_but_not_really')).toBeNull(); + }); + + it('normalises traversal and trailing separators', () => { + expect(pathSegmentsBetween('/a', '/a/b/../b/c/')).toEqual(['b', 'c']); + }); +}); + +describe('containsSymlinkedComponent', () => { + let base: string; + + beforeEach(async () => { + // realpath so the temp dir is canonical (e.g. macOS /var -> /private/var is a symlink). + base = await realpath(await mkdtemp(join(tmpdir(), 'pathutil-'))); + }); + + afterEach(async () => { + await rm(base, { recursive: true, force: true }); + }); + + it('returns false when no component is a symlink', async () => { + const dir = join(base, 'a', 'b'); + await mkdir(dir, { recursive: true }); + expect(await containsSymlinkedComponent(dir)).toBe(false); + }); + + it('returns true when an ancestor component is a symlink', async () => { + const real = join(base, 'real'); + await mkdir(real, { recursive: true }); + const link = join(base, 'link'); + await symlink(real, link); + expect(await containsSymlinkedComponent(join(link, 'child'))).toBe(true); + }); + + it('ignores missing components', async () => { + expect(await containsSymlinkedComponent(join(base, 'does', 'not', 'exist'))).toBe(false); + }); + + it('propagates errors other than a missing component', async () => { + // Running as root bypasses permission checks, so the error path can't be exercised there. + if (process.getuid?.() === 0) return; + const unreadable = join(base, 'unreadable'); + await mkdir(unreadable); + await chmod(unreadable, 0o000); + try { + await expect(containsSymlinkedComponent(join(unreadable, 'child'))).rejects.toThrow(); + } finally { + await chmod(unreadable, 0o755); + } + }); +}); diff --git a/packages/@n8n/backend-common/src/utils/path-util.ts b/packages/@n8n/backend-common/src/utils/path-util.ts index f55a4ea1705..06944ec7dcb 100644 --- a/packages/@n8n/backend-common/src/utils/path-util.ts +++ b/packages/@n8n/backend-common/src/utils/path-util.ts @@ -1,4 +1,5 @@ import { UnexpectedError } from 'n8n-workflow'; +import { lstat } from 'node:fs/promises'; import * as path from 'node:path'; /** @@ -34,3 +35,49 @@ export function safeJoinPath(parentPath: string, ...paths: string[]): string { return candidate; } + +/** Components of an absolute path, root-first: `/a/b/c` -> [`/a`, `/a/b`, `/a/b/c`]. */ +export function pathComponents(targetPath: string): string[] { + // Start from the path root (`/` on POSIX, `C:\` on Windows) so drive-qualified + // paths build correctly instead of being re-rooted at the bare separator. + const root = path.parse(targetPath).root; + const segments = targetPath.slice(root.length).split(path.sep).filter(Boolean); + return segments.reduce( + (components, segment) => [...components, path.join(components.at(-1) ?? root, segment)], + [], + ); +} + +/** + * Path segments of `descendant` below `ancestor`, or `null` if `descendant` is + * not contained within `ancestor`. `/a/b` below `/a` -> [`b`]; equal paths -> []. + */ +export function pathSegmentsBetween(ancestor: string, descendant: string): string[] | null { + if (!isContainedWithin(ancestor, descendant)) { + return null; + } + const relative = path.relative(ancestor, descendant); + return relative.split(path.sep).filter(Boolean); +} + +export async function containsSymlinkedComponent(resolvedPath: string): Promise { + for (const component of pathComponents(resolvedPath)) { + let stats; + try { + stats = await lstat(component); + } catch (error) { + if ( + error instanceof Error && + 'code' in error && + (error.code === 'ENOENT' || error.code === 'ENOTDIR') + ) { + continue; + } + throw error; + } + if (stats.isSymbolicLink()) { + return true; + } + } + return false; +} diff --git a/packages/@n8n/computer-use/README.md b/packages/@n8n/computer-use/README.md index 343951ddbd8..4cc7d002a88 100644 --- a/packages/@n8n/computer-use/README.md +++ b/packages/@n8n/computer-use/README.md @@ -18,13 +18,15 @@ remotely over a secure SSE connection. |------------|-------|----------|--------------------| | **Filesystem (read)** | `read_file`, `list_files`, `get_file_tree`, `search_files` | All | `allow` | | **Filesystem (write)** | `write_file`, `edit_file`, `create_directory`, `delete`, `move`, `copy_file` | All | `ask` | -| **Shell** | `shell_execute` | All | `deny` | +| **Shell** | `shell_execute` | macOS · Linux/WSL2 | `deny` | | **Computer** | `screen_screenshot`, `screen_screenshot_region`, `mouse_move`, `mouse_click`, `mouse_double_click`, `mouse_drag`, `mouse_scroll`, `keyboard_type`, `keyboard_key_tap`, `keyboard_shortcut` | macOS, Linux (X11), Windows | `deny` | | **Browser** | 32 browser automation tools | All | `ask` | Modules that require native dependencies (screenshot, mouse/keyboard) are automatically disabled when their platform requirements aren't met. +Shell execution runs inside an OS-level sandbox, see [Shell sandboxing](#shell-sandboxing). + ## Quick start ### Daemon mode (recommended) @@ -96,6 +98,7 @@ Each capability has an independent permission mode (`deny` \| `ask` \| `allow`): | Flag | Default | Description | |------|---------|-------------| | `--computer-shell-timeout ` | `30000` | Shell command timeout | +| `--dangerously-disable-shell-sandbox` | | Run `shell_execute` without the OS sandbox (**insecure** — see [Shell sandboxing](#shell-sandboxing)) | #### Browser @@ -113,6 +116,7 @@ take precedence. | `N8N_GATEWAY_LOG_LEVEL` | `--log-level` | | `N8N_GATEWAY_FILESYSTEM_DIR` | `--dir` | | `N8N_GATEWAY_COMPUTER_SHELL_TIMEOUT` | `--computer-shell-timeout` | +| `N8N_GATEWAY_DANGEROUSLY_DISABLE_SHELL_SANDBOX` | `--dangerously-disable-shell-sandbox` (set to `true`) | | `N8N_GATEWAY_BROWSER_DEFAULT` | `--browser-default` | | `N8N_GATEWAY_AUTO_CONFIRM` | `--auto-confirm` (set to `true`) | | `N8N_GATEWAY_NON_INTERACTIVE` | `--non-interactive` (set to `true`) | @@ -160,7 +164,8 @@ operations — paths outside the configured root are rejected. ### Shell -Execute shell commands with configurable timeout. +Execute shell commands with configurable timeout. Commands run inside an +OS-level sandbox — see [Shell sandboxing](#shell-sandboxing). | Tool | Description | |------|-------------| @@ -236,10 +241,15 @@ resource is rejected even when the capability is set to `allow`. ## Prerequisites -### Filesystem & shell +### Filesystem No extra dependencies — works on all platforms. +### Shell + +macOS works out of the box. Linux and Windows (WSL2) need `bubblewrap` and +`socat`; native Windows is not supported. See [Shell sandboxing](#shell-sandboxing). + ### Screenshot Requires a display server. Automatically disabled when no monitors are @@ -266,6 +276,87 @@ npx playwright install chromium firefox For local browser modes, see the [@n8n/mcp-browser prerequisites](../mcp-browser/README.md#prerequisites). +## Shell sandboxing + +`shell_execute` runs every command inside an OS-level sandbox provided by +[`@anthropic-ai/sandbox-runtime`](https://github.com/anthropic-experimental/sandbox-runtime), +so a command, and every child process it spawns can only touch the files and +network the sandbox permits. + +### What it restricts + +- **Writes** are confined to the working directory. The gateway settings directory is denied for both read and write. +- **Reads** are permitted except for some sensitive directories. +- **Outbound network** is blocked, no domains are allowed. + +These boundaries are enforced by the OS, so they apply to the shell command and all of its subprocesses. + +### OS-level enforcement + +| Platform | Backend | Setup | +|----------|---------|-------| +| macOS | Seatbelt (built in) | None | +| Linux | bubblewrap + socat | Install packages (below) | +| WSL2 | bubblewrap + socat | Install packages (below) | +| WSL1 | — | Not supported — upgrade to WSL2 | +| Native Windows | — | Not supported — run inside WSL2 | + +The native Windows backend is not a boundary against a deliberately malicious +process, so it is treated as unavailable. On Windows, run the gateway inside +WSL2. + +### Linux / WSL2 setup + +The sandbox needs two packages: + +- [`bubblewrap`](https://github.com/containers/bubblewrap) — unprivileged + sandbox that enforces filesystem isolation (`bwrap`) +- [`socat`](http://www.dest-unreach.org/socat/) — relays network traffic + through the sandbox proxy + +```bash +# Ubuntu/Debian +sudo apt-get install bubblewrap socat + +# Fedora +sudo dnf install bubblewrap socat +``` + +> **Ubuntu 24.04+ (including WSL2):** the default AppArmor policy prevents +> bubblewrap from creating the user namespaces it needs. Check with +> `sysctl kernel.apparmor_restrict_unprivileged_userns`; if it returns `1`, +> grant `bwrap` the capability and reload AppArmor: +> +> ```bash +> sudo tee /etc/apparmor.d/bwrap > /dev/null <<'EOF' +> abi , +> include +> +> profile bwrap /usr/bin/bwrap flags=(unconfined) { +> userns, +> include if exists +> } +> EOF +> sudo systemctl reload apparmor +> ``` + +### Disabling the sandbox + +`--dangerously-disable-shell-sandbox` (or +`N8N_GATEWAY_DANGEROUSLY_DISABLE_SHELL_SANDBOX=true`) runs shell commands +**without** any OS isolation. + +> **Warning:** this removes the filesystem and network boundaries entirely: a +> command can read your SSH keys, write anywhere your user can, and reach any +> host. Only use it inside an already-isolated, trusted environment such as a +> disposable container or VM. + +### Limitations + +The sandbox is an OS-level guardrail, not a full VM. Treat it as protection against accidental +or opportunistic access rather than containment for fully untrusted code; for +stronger isolation, run the gateway in a container or VM. + ## Development ```bash diff --git a/packages/@n8n/computer-use/package.json b/packages/@n8n/computer-use/package.json index d466924e4b1..f53c602eb57 100644 --- a/packages/@n8n/computer-use/package.json +++ b/packages/@n8n/computer-use/package.json @@ -38,7 +38,7 @@ "dist/**/*" ], "dependencies": { - "@anthropic-ai/sandbox-runtime": "^0.0.42", + "@anthropic-ai/sandbox-runtime": "^0.0.61", "@inquirer/prompts": "^8.3.2", "@jitsi/robotjs": "^0.6.21", "@modelcontextprotocol/sdk": "catalog:", diff --git a/packages/@n8n/computer-use/src/cli.ts b/packages/@n8n/computer-use/src/cli.ts index d6c14af91ad..85a770208bd 100644 --- a/packages/@n8n/computer-use/src/cli.ts +++ b/packages/@n8n/computer-use/src/cli.ts @@ -13,6 +13,7 @@ import { printBanner, printConnected, printInvalidToken, + printModuleDiagnostics, printModuleStatus, printToolList, } from './logger'; @@ -135,6 +136,11 @@ Permissions (deny | ask | allow): Computer use: --computer-shell-timeout Shell command timeout (default: 30000) + --dangerously-disable-shell-sandbox + Run shell commands WITHOUT the OS sandbox. + Insecure — only use in a trusted, isolated + environment. Without a sandbox the shell + tool is disabled by default. Browser: --no-browser Disable browser tools @@ -212,13 +218,6 @@ async function main( process.exit(1); } - // printModuleStatus expects a GatewayConfig shape — derive one from the session. - printModuleStatus({ - ...parsed.config, - permissions: session.getAllPermissions(), - filesystem: { dir: session.dir }, - }); - const client = new GatewayClient({ url, apiKey, @@ -250,6 +249,15 @@ async function main( } printConnected(url); + printModuleStatus( + { + ...parsed.config, + permissions: session.getAllPermissions(), + filesystem: { dir: session.dir }, + }, + client.toolCategories, + ); + printModuleDiagnostics(client.disabledModules); printToolList(client.tools); } diff --git a/packages/@n8n/computer-use/src/config.ts b/packages/@n8n/computer-use/src/config.ts index 878991c7c77..9e3ed01bb75 100644 --- a/packages/@n8n/computer-use/src/config.ts +++ b/packages/@n8n/computer-use/src/config.ts @@ -58,7 +58,7 @@ export interface GatewayConfig { logLevel: 'silent' | 'error' | 'warn' | 'info' | 'debug'; allowedOrigins: string[]; filesystem: { dir: string }; - computer: { shell: { timeout: number } }; + computer: { shell: { timeout: number; dangerouslyDisableSandbox: boolean } }; browser: { defaultBrowser: string; }; @@ -104,7 +104,12 @@ const structuralConfigSchema = z.object({ filesystem: z.object({ dir: z.string().default('.') }).default({}), computer: z .object({ - shell: z.object({ timeout: z.number().int().positive().default(30_000) }).default({}), + shell: z + .object({ + timeout: z.number().int().positive().default(30_000), + dangerouslyDisableSandbox: z.boolean().default(false), + }) + .default({}), }) .default({}), browser: z @@ -165,8 +170,12 @@ function buildEnvConfig(): PartialStructural { const fsDir = envString('FILESYSTEM_DIR'); if (fsDir) config.filesystem = { dir: fsDir }; + const shell: Record = {}; const shellTimeout = envNumber('COMPUTER_SHELL_TIMEOUT'); - if (shellTimeout !== undefined) config.computer = { shell: { timeout: shellTimeout } }; + if (shellTimeout !== undefined) shell.timeout = shellTimeout; + const disableSandbox = envBoolean('DANGEROUSLY_DISABLE_SHELL_SANDBOX'); + if (disableSandbox !== undefined) shell.dangerouslyDisableSandbox = disableSandbox; + if (Object.keys(shell).length > 0) config.computer = { shell }; const defaultBrowser = envString('BROWSER_DEFAULT'); if (defaultBrowser) config.browser = { defaultBrowser }; @@ -195,8 +204,11 @@ function buildCliConfig(args: yargsParser.Arguments): PartialStructural { const dir = args.dir as string; if (dir) config.filesystem = { dir }; - const timeout = args['computer-shell-timeout'] as number; - if (timeout !== undefined) config.computer = { shell: { timeout } }; + const shell: Record = {}; + const timeout = args['computer-shell-timeout'] as number | undefined; + if (timeout !== undefined) shell.timeout = timeout; + if (args['dangerously-disable-shell-sandbox'] === true) shell.dangerouslyDisableSandbox = true; + if (Object.keys(shell).length > 0) config.computer = { shell }; if (args['browser-default']) config.browser = { defaultBrowser: args['browser-default'] as string }; @@ -303,7 +315,7 @@ export function parseConfig(argv = process.argv.slice(2)): ParsedArgs { 'permission-confirmation', ...permissionFlags, ], - boolean: ['auto-confirm', 'non-interactive', 'help'], + boolean: ['auto-confirm', 'non-interactive', 'help', 'dangerously-disable-shell-sandbox'], number: ['computer-shell-timeout'], alias: { h: 'help', d: 'dir' }, }); diff --git a/packages/@n8n/computer-use/src/gateway-client.test.ts b/packages/@n8n/computer-use/src/gateway-client.test.ts index 3b078a73ded..1db29b52e60 100644 --- a/packages/@n8n/computer-use/src/gateway-client.test.ts +++ b/packages/@n8n/computer-use/src/gateway-client.test.ts @@ -24,21 +24,42 @@ vi.mock('./logger', () => ({ })); // Mock tool modules that pull in native/ESM-only dependencies +// Mock modules as unavailable so the client under test doesn't load native deps. vi.mock('./tools/shell', () => ({ - ['ShellModule']: { isSupported: vi.fn().mockResolvedValue(false), definitions: [] }, + ['ShellModule']: { + name: 'Shell', + category: 'shell', + permissionGroup: 'shell', + activate: vi.fn().mockResolvedValue({ supported: false, reason: 'mocked' }), + }, })); vi.mock('./tools/filesystem', () => ({ filesystemReadTools: [], filesystemWriteTools: [], })); vi.mock('./tools/screenshot', () => ({ - ['ScreenshotModule']: { isSupported: vi.fn().mockResolvedValue(false), definitions: [] }, + ['ScreenshotModule']: { + name: 'Screenshot', + category: 'screenshot', + permissionGroup: 'computer', + activate: vi.fn().mockResolvedValue({ supported: false, reason: 'mocked' }), + }, })); vi.mock('./tools/mouse-keyboard', () => ({ - ['MouseKeyboardModule']: { isSupported: vi.fn().mockResolvedValue(false), definitions: [] }, + ['MouseKeyboardModule']: { + name: 'MouseKeyboard', + category: 'mouse-keyboard', + permissionGroup: 'computer', + activate: vi.fn().mockResolvedValue({ supported: false, reason: 'mocked' }), + }, })); vi.mock('./tools/browser', () => ({ - ['BrowserModule']: { create: vi.fn().mockResolvedValue(null) }, + ['BrowserModule']: { + name: 'Browser', + category: 'browser', + permissionGroup: 'browser', + activate: vi.fn().mockResolvedValue({ supported: false, reason: 'mocked' }), + }, })); vi.mock('@vscode/ripgrep', () => ({ rgPath: '/usr/bin/rg' })); vi.mock('@anthropic-ai/sandbox-runtime', () => ({ @@ -51,7 +72,8 @@ vi.mock('@anthropic-ai/sandbox-runtime', () => ({ import type { GatewayConfig } from './config'; import { GatewayAuthError, GatewayClient } from './gateway-client'; import type { GatewaySession } from './gateway-session'; -import { shellExecuteTool } from './tools/shell/shell-execute'; +import { logger } from './logger'; +import { createShellExecuteTool } from './tools/shell/shell-execute'; import type { AffectedResource, ConfirmResourceAccess, ToolDefinition } from './tools/types'; import { INSTANCE_RESOURCE_DECISION_KEYS } from './tools/types'; @@ -64,7 +86,7 @@ function makeConfig(permissionConfirmation: 'client' | 'instance' = 'client'): G logLevel: 'silent', allowedOrigins: [], filesystem: { dir: '/' }, - computer: { shell: { timeout: 30_000 } }, + computer: { shell: { timeout: 30_000, dangerouslyDisableSandbox: false } }, browser: { defaultBrowser: 'chrome' }, permissions: {}, permissionConfirmation, @@ -238,7 +260,7 @@ describe('GatewayClient.checkPermissions', () => { it('does not reuse a bare shell command session allow for shell_execute with explicit cwd', async () => { const execute = vi.fn().mockResolvedValue({ content: [{ type: 'text', text: 'ok' }] }); - const shellTool = { ...shellExecuteTool, execute } as ToolDefinition; + const shellTool = { ...createShellExecuteTool('unsandboxed'), execute } as ToolDefinition; const session = makeSession({ check: vi.fn((_toolGroup, resource) => (resource === 'ls' ? 'allow' : 'ask')), }); @@ -400,3 +422,45 @@ describe('GatewayClient.uploadCapabilities', () => { await expect(promise).rejects.toThrow(/Failed to upload capabilities: 500/); }); }); + +describe('GatewayClient.runTeardowns', () => { + function makeMinimalClient(): GatewayClient { + return new GatewayClient({ + url: 'http://localhost:5678', + apiKey: 'tok', + config: makeConfig(), + session: makeSession(), + confirmResourceAccess: vi.fn(), + }); + } + + beforeEach(() => { + vi.mocked(logger.error).mockClear(); + }); + + it('runs every teardown even when an earlier one rejects', async () => { + const first = vi.fn().mockRejectedValue(new Error('boom')); + const second = vi.fn().mockResolvedValue(undefined); + const third = vi.fn().mockRejectedValue('bad'); + const client = makeMinimalClient(); + client['teardowns'] = [first, second, third]; + + await client['runTeardowns'](); + + expect(first).toHaveBeenCalledOnce(); + expect(second).toHaveBeenCalledOnce(); + expect(third).toHaveBeenCalledOnce(); + expect(logger.error).toHaveBeenCalledTimes(2); + }); + + it('clears teardowns so a second run is a no-op', async () => { + const teardown = vi.fn().mockResolvedValue(undefined); + const client = makeMinimalClient(); + client['teardowns'] = [teardown]; + + await client['runTeardowns'](); + await client['runTeardowns'](); + + expect(teardown).toHaveBeenCalledOnce(); + }); +}); diff --git a/packages/@n8n/computer-use/src/gateway-client.ts b/packages/@n8n/computer-use/src/gateway-client.ts index 2805fdad7eb..33284e796f0 100644 --- a/packages/@n8n/computer-use/src/gateway-client.ts +++ b/packages/@n8n/computer-use/src/gateway-client.ts @@ -14,8 +14,10 @@ import { printToolCall, printToolResult, } from './logger'; -import type { BrowserModule } from './tools/browser'; +import { BrowserModule } from './tools/browser'; import { filesystemReadTools, filesystemWriteTools } from './tools/filesystem'; +import { MouseKeyboardModule } from './tools/mouse-keyboard'; +import { ScreenshotModule } from './tools/screenshot'; import { ShellModule } from './tools/shell'; import { type AffectedResource, @@ -25,6 +27,7 @@ import { type McpTool, type ResourceDecision, type ToolDefinition, + type ToolModule, GATEWAY_CONFIRMATION_REQUIRED_PREFIX, INSTANCE_RESOURCE_DECISION_KEYS, } from './tools/types'; @@ -104,13 +107,34 @@ export class GatewayClient { private definitionMap: Map = new Map(); - private browserModule: BrowserModule | null = null; + private readonly modules: ToolModule[] = [ + ShellModule, + ScreenshotModule, + MouseKeyboardModule, + BrowserModule, + ]; + + /** Teardowns for modules that hold resources; collected as modules activate. */ + private teardowns: Array<() => Promise> = []; + + /** Diagnostics for permitted-but-unavailable modules; surfaced after connect. */ + private disabledModuleReports: Array<{ name: string; reason: string; hint?: string }> = []; /** Get all registered tool definitions (populated after start). */ get tools(): ToolDefinition[] { return this.allDefinitions ?? []; } + /** Resolved per-category enabled state (populated after start). */ + get toolCategories(): Array<{ name: string; enabled: boolean; writeAccess?: boolean }> { + return this.activeToolCategories; + } + + /** Permitted modules that couldn't activate, with why + how to fix (after start). */ + get disabledModules(): Array<{ name: string; reason: string; hint?: string }> { + return this.disabledModuleReports; + } + constructor(private readonly options: GatewayClientOptions) {} /** Session key when the server has upgraded the pairing token; otherwise the original token. */ @@ -135,7 +159,7 @@ export class GatewayClient { this.eventSource.close(); this.eventSource = null; } - if (this.browserModule) await this.browserModule.shutdown(); + await this.runTeardowns(); } /** @@ -185,11 +209,25 @@ export class GatewayClient { this.eventSource.close(); this.eventSource = null; } - if (this.browserModule) await this.browserModule.shutdown(); + await this.runTeardowns(); this.options.onDisconnected?.(); } + /** Run and clear pending module teardowns. Idempotent. */ + private async runTeardowns(): Promise { + const teardowns = this.teardowns; + this.teardowns = []; + const results = await Promise.allSettled(teardowns.map(async (teardown) => await teardown())); + for (const result of results) { + if (result.status === 'rejected') { + logger.error('Module teardown failed', { + error: result.reason instanceof Error ? result.reason.message : String(result.reason), + }); + } + } + } + private async getAllDefinitions(): Promise { if (this.allDefinitions) return this.allDefinitions; @@ -212,71 +250,32 @@ export class GatewayClient { writeAccess: fsWriteEnabled, }); - // Computer use modules — check permission mode and platform support - // Lazy-load Screenshot and MouseKeyboard to avoid eager native module imports - const { ScreenshotModule } = await import('./tools/screenshot'); - const { MouseKeyboardModule } = await import('./tools/mouse-keyboard'); - - const computerModules: Array<{ - name: string; - category: string; - enabled: boolean; - module: { isSupported(): boolean | Promise; definitions: ToolDefinition[] }; - }> = [ - { - name: 'Shell', - category: 'shell', - enabled: session.getGroupMode('shell') !== 'deny', - module: ShellModule, - }, - { - name: 'Screenshot', - category: 'screenshot', - enabled: session.getGroupMode('computer') !== 'deny', - module: ScreenshotModule, - }, - { - name: 'MouseKeyboard', - category: 'mouse-keyboard', - enabled: session.getGroupMode('computer') !== 'deny', - module: MouseKeyboardModule, - }, - ]; - - for (const { name, category, enabled, module } of computerModules) { - if (!enabled) { - logger.debug('Module denied by permission, skipping', { module: name }); - categories.push({ name: category, enabled: false }); + for (const module of this.modules) { + if (session.getGroupMode(module.permissionGroup) === 'deny') { + logger.debug('Module denied by permission, skipping', { module: module.name }); + categories.push({ name: module.category, enabled: false }); continue; } - if (await module.isSupported()) { - defs.push(...tagCategory(module.definitions, category)); - categories.push({ name: category, enabled: true }); - } else { - logger.debug('Module not supported on this platform, skipping', { module: name }); - categories.push({ name: category, enabled: false }); - } - } - // Browser - if (session.getGroupMode('browser') !== 'deny') { - const { BrowserModule: BrowserModuleClass } = await import('./tools/browser'); - this.browserModule = await BrowserModuleClass.create({ - ...config.browser, - logLevel: config.logLevel, - }); - if (this.browserModule) { - defs.push(...tagCategory(this.browserModule.definitions, 'browser')); - categories.push({ name: 'browser', enabled: true }); - } else { - logger.debug('Module not supported on this platform, skipping', { - module: 'Browser', + const activation = await module.activate({ config, dir: session.dir }); + if (!activation.supported) { + logger.debug('Module disabled', { + module: module.name, + reason: activation.reason, + ...(activation.hint ? { hint: activation.hint } : {}), }); - categories.push({ name: 'browser', enabled: false }); + this.disabledModuleReports.push({ + name: module.name, + reason: activation.reason, + ...(activation.hint ? { hint: activation.hint } : {}), + }); + categories.push({ name: module.category, enabled: false }); + continue; } - } else { - logger.debug('Module denied by permission, skipping', { module: 'Browser' }); - categories.push({ name: 'browser', enabled: false }); + + defs.push(...tagCategory(activation.tools, module.category)); + if (activation.shutdown) this.teardowns.push(activation.shutdown); + categories.push({ name: module.category, enabled: true }); } for (const def of defs) { diff --git a/packages/@n8n/computer-use/src/logger.test.ts b/packages/@n8n/computer-use/src/logger.test.ts index 74e5bc06bd8..89acc039641 100644 --- a/packages/@n8n/computer-use/src/logger.test.ts +++ b/packages/@n8n/computer-use/src/logger.test.ts @@ -7,7 +7,7 @@ const BASE_CONFIG: GatewayConfig = { logLevel: 'info', allowedOrigins: [], filesystem: { dir: '/test' }, - computer: { shell: { timeout: 30_000 } }, + computer: { shell: { timeout: 30_000, dangerouslyDisableSandbox: false } }, browser: { defaultBrowser: 'chrome', }, diff --git a/packages/@n8n/computer-use/src/logger.ts b/packages/@n8n/computer-use/src/logger.ts index 0da00cb7f7c..d32efecea57 100644 --- a/packages/@n8n/computer-use/src/logger.ts +++ b/packages/@n8n/computer-use/src/logger.ts @@ -165,54 +165,95 @@ function permissionIcon(mode: PermissionMode): string { return pc.dim('✗'); } -export function printModuleStatus(config: GatewayConfig): void { +/** + * Print the module overview. Pass resolved `categories` (from the gateway) so + * runtime-gated modules (shell sandbox, native computer modules) reflect their + * actual state; without them it falls back to the permission modes alone. + */ +export function printModuleStatus( + config: GatewayConfig, + categories?: Array<{ name: string; enabled: boolean; writeAccess?: boolean }>, +): void { const { permissions } = config; + const isOn = (category: string, fallback: boolean) => + categories ? (categories.find((c) => c.name === category)?.enabled ?? false) : fallback; + const icon = (mode: PermissionMode, enabled: boolean) => + enabled ? permissionIcon(mode) : pc.dim('✗'); + const row = (iconStr: string, label: string, value: string) => + ` ${iconStr} ${label.padEnd(16)} ${value}`; + const disabled = pc.dim('(disabled)'); + const dir = pc.dim(formatPath(config.filesystem.dir)); - // Filesystem — read and write are separate permission groups + // Filesystem — permission-only, no runtime gating const fsRead = permissions.filesystemRead ?? 'deny'; const fsWrite: PermissionMode = fsRead === 'deny' ? 'deny' : (permissions.filesystemWrite ?? 'deny'); - const dir = pc.dim(formatPath(config.filesystem.dir)); + logger.info(row(permissionIcon(fsRead), 'Filesystem read', fsRead !== 'deny' ? dir : disabled), { + module: 'FilesystemRead', + }); logger.info( - ` ${permissionIcon(fsRead)} Filesystem read ${fsRead !== 'deny' ? dir : pc.dim('(disabled)')}`, - { module: 'FilesystemRead' }, - ); - logger.info( - ` ${permissionIcon(fsWrite)} Filesystem write ${fsWrite !== 'deny' ? dir : pc.dim('(disabled)')}`, + row(permissionIcon(fsWrite), 'Filesystem write', fsWrite !== 'deny' ? dir : disabled), { module: 'FilesystemWrite' }, ); - // Shell + // Shell — gated by the OS sandbox at runtime const shellMode = permissions.shell ?? 'deny'; - const shellDetail = - shellMode === 'deny' - ? pc.dim('(disabled)') - : pc.dim(`timeout: ${config.computer.shell.timeout / 1000}s`); - logger.info(` ${permissionIcon(shellMode)} Shell ${shellDetail}`, { module: 'Shell' }); - - // Computer — Screenshot + Mouse/keyboard share the same group - const computerMode = permissions.computer ?? 'deny'; - const computerDisabled = pc.dim('(disabled)'); + const shellOn = isOn('shell', shellMode !== 'deny'); logger.info( - ` ${permissionIcon(computerMode)} Screenshot ${computerMode === 'deny' ? computerDisabled : ''}`, - { module: 'Screenshot' }, + row( + icon(shellMode, shellOn), + 'Shell', + shellOn ? pc.dim(`timeout: ${config.computer.shell.timeout / 1000}s`) : disabled, + ), + { module: 'Shell' }, ); + + // Computer — Screenshot + Mouse/keyboard, gated by native modules at runtime + const computerMode = permissions.computer ?? 'deny'; + const screenshotOn = isOn('screenshot', computerMode !== 'deny'); + const mouseKeyboardOn = isOn('mouse-keyboard', computerMode !== 'deny'); + logger.info(row(icon(computerMode, screenshotOn), 'Screenshot', screenshotOn ? '' : disabled), { + module: 'Screenshot', + }); logger.info( - ` ${permissionIcon(computerMode)} Mouse/keyboard ${computerMode === 'deny' ? computerDisabled : ''}`, + row(icon(computerMode, mouseKeyboardOn), 'Mouse/keyboard', mouseKeyboardOn ? '' : disabled), { module: 'MouseKeyboard' }, ); // Browser const browserMode = permissions.browser ?? 'deny'; - const browserDetail = - browserMode === 'deny' ? pc.dim('(disabled)') : pc.dim(config.browser.defaultBrowser); - logger.info(` ${permissionIcon(browserMode)} Browser ${browserDetail}`, { - module: 'Browser', - }); + const browserOn = isOn('browser', browserMode !== 'deny'); + logger.info( + row( + icon(browserMode, browserOn), + 'Browser', + browserOn ? pc.dim(config.browser.defaultBrowser) : disabled, + ), + { module: 'Browser' }, + ); logger.info(''); } +/** + * Report permitted modules that couldn't activate, with why + how to fix. + * Printed as visible lines (the logger only surfaces metadata at debug level); + * hints may be multi-line and each line is shown indented. + */ +export function printModuleDiagnostics( + modules: Array<{ name: string; reason: string; hint?: string }>, +): void { + if (modules.length === 0) return; + for (const { name, reason, hint } of modules) { + logger.warn(` ${pc.yellow('⚠')} ${name}`, { module: name, reason, ...(hint ? { hint } : {}) }); + logger.warn(` ${pc.dim(reason)}`); + for (const line of hint?.split('\n') ?? []) { + logger.warn(` ${pc.dim(line)}`); + } + } + logger.warn(''); +} + export function printToolList(tools: ToolDefinition[]): void { if (tools.length === 0) return; @@ -243,7 +284,7 @@ export function printWaiting(): void { } export function printConnected(url: string): void { - logger.info(` ${pc.green('●')} Connected to ${pc.bold(url)}`, { url }); + logger.info(`\n ${pc.green('●')} Connected to ${pc.bold(url)}\n`, { url }); } export function printDisconnected(): void { @@ -260,9 +301,9 @@ export function printAuthFailure(): void { } export function printInvalidToken(url: string): void { - logger.error(` ${pc.red('✗')} Connection token invalid`); + logger.error(`\n ${pc.red('✗')} Connection token invalid`); logger.error( - ` ${pc.dim(`Go to ${url} and reconnect n8n Computer Use using a new connection token`)}`, + ` ${pc.dim(`Go to ${url} and reconnect n8n Computer Use using a new connection token`)}\n`, ); } diff --git a/packages/@n8n/computer-use/src/settings-store.test.ts b/packages/@n8n/computer-use/src/settings-store.test.ts index 33199762181..6dbede9c468 100644 --- a/packages/@n8n/computer-use/src/settings-store.test.ts +++ b/packages/@n8n/computer-use/src/settings-store.test.ts @@ -27,7 +27,7 @@ const BASE_CONFIG: GatewayConfig = { logLevel: 'info', allowedOrigins: [], filesystem: { dir: process.cwd() }, - computer: { shell: { timeout: 30_000 } }, + computer: { shell: { timeout: 30_000, dangerouslyDisableSandbox: false } }, browser: { defaultBrowser: 'chrome' }, permissions: {}, permissionConfirmation: 'instance', diff --git a/packages/@n8n/computer-use/src/startup-config-cli.test.ts b/packages/@n8n/computer-use/src/startup-config-cli.test.ts index ba1455f66eb..42e8f14ce46 100644 --- a/packages/@n8n/computer-use/src/startup-config-cli.test.ts +++ b/packages/@n8n/computer-use/src/startup-config-cli.test.ts @@ -28,7 +28,7 @@ const BASE_CONFIG: GatewayConfig = { logLevel: 'info', allowedOrigins: [], filesystem: { dir: '/tmp' }, - computer: { shell: { timeout: 30_000 } }, + computer: { shell: { timeout: 30_000, dangerouslyDisableSandbox: false } }, browser: { defaultBrowser: 'chrome', }, diff --git a/packages/@n8n/computer-use/src/tools/browser/index.ts b/packages/@n8n/computer-use/src/tools/browser/index.ts index a81d87ae5d6..7fb53ef124c 100644 --- a/packages/@n8n/computer-use/src/tools/browser/index.ts +++ b/packages/@n8n/computer-use/src/tools/browser/index.ts @@ -1,64 +1,38 @@ import type { Config as BrowserConfig } from '@n8n/mcp-browser'; -import { logger, type LogLevel } from '../../logger'; -import type { ToolDefinition, ToolModule } from '../types'; +import { ModuleActivation, type ToolModule } from '../types'; -export interface BrowserModuleConfig { - defaultBrowser?: string; - logLevel?: LogLevel; - adapter?: 'playwright' | 'agent-browser'; -} - -function toBrowserConfig(config: BrowserModuleConfig): Partial { - const browserConfig: Partial = { - adapter: config.adapter ?? 'agent-browser', - }; - if (config.defaultBrowser) { - browserConfig.defaultBrowser = config.defaultBrowser as BrowserConfig['defaultBrowser']; +function toBrowserConfig(defaultBrowser: string): Partial { + const browserConfig: Partial = { adapter: 'agent-browser' }; + if (defaultBrowser) { + browserConfig.defaultBrowser = defaultBrowser as BrowserConfig['defaultBrowser']; } return browserConfig; } -/** - * ToolModule that exposes @n8n/mcp-browser tools through the gateway. - * - * Use `BrowserModule.create()` to construct — it dynamically imports - * `@n8n/mcp-browser` and initialises the BrowserConnection and tools. - */ -export class BrowserModule implements ToolModule { - private connection: { shutdown(): Promise }; - - definitions: ToolDefinition[]; - - private constructor(definitions: ToolDefinition[], connection: { shutdown(): Promise }) { - this.definitions = definitions; - this.connection = connection; - } - - /** - * Create a BrowserModule if `@n8n/mcp-browser` is available. - * Returns `null` when the package cannot be imported. - */ - static async create(config: BrowserModuleConfig = {}): Promise { - try { - const { createBrowserTools, configureLogger } = await import('@n8n/mcp-browser'); - if (config.logLevel) { - configureLogger({ level: config.logLevel }); - } - const { tools, connection } = createBrowserTools(toBrowserConfig(config)); - return new BrowserModule(tools, connection); - } catch { - logger.info('Browser module not supported', { reason: '@n8n/mcp-browser not available' }); - return null; - } - } - - isSupported() { - return true; - } - - /** Shut down the BrowserConnection and close the browser. */ - async shutdown(): Promise { - await this.connection.shutdown(); +async function loadMcpBrowser() { + try { + return await import('@n8n/mcp-browser'); + } catch { + return null; } } + +export const BrowserModule: ToolModule = { + name: 'Browser', + category: 'browser', + permissionGroup: 'browser', + async activate({ config }) { + const mcpBrowser = await loadMcpBrowser(); + if (!mcpBrowser) { + return ModuleActivation.unsupported('@n8n/mcp-browser is not available'); + } + if (config.logLevel) { + mcpBrowser.configureLogger({ level: config.logLevel }); + } + const { tools, connection } = mcpBrowser.createBrowserTools( + toBrowserConfig(config.browser.defaultBrowser), + ); + return ModuleActivation.supported(tools, async () => await connection.shutdown()); + }, +}; diff --git a/packages/@n8n/computer-use/src/tools/mouse-keyboard/index.ts b/packages/@n8n/computer-use/src/tools/mouse-keyboard/index.ts index aadf6bc62ba..976ef79a2a4 100644 --- a/packages/@n8n/computer-use/src/tools/mouse-keyboard/index.ts +++ b/packages/@n8n/computer-use/src/tools/mouse-keyboard/index.ts @@ -1,5 +1,4 @@ -import { logger } from '../../logger'; -import type { ToolModule } from '../types'; +import { ModuleActivation, type ToolModule } from '../types'; import { mouseMoveTool, mouseClickTool, @@ -12,33 +11,28 @@ import { } from './mouse-keyboard'; export const MouseKeyboardModule: ToolModule = { - async isSupported() { - // Linux Wayland: no X display available for robotjs + name: 'MouseKeyboard', + category: 'mouse-keyboard', + permissionGroup: 'computer', + async activate() { if (process.env.WAYLAND_DISPLAY && !process.env.DISPLAY) { - logger.info('Mouse/keyboard module not supported', { - reason: 'Wayland without X11 compatibility layer', - }); - return false; + return ModuleActivation.unsupported('Wayland without X11 compatibility layer'); } try { const robot = await import('@jitsi/robotjs'); robot.default.getMousePos(); - return true; + return ModuleActivation.supported([ + mouseMoveTool, + mouseClickTool, + mouseDoubleClickTool, + mouseDragTool, + mouseScrollTool, + keyboardTypeTool, + keyboardKeyTapTool, + keyboardShortcutTool, + ]); } catch (error) { - logger.info('Mouse/keyboard module not supported', { - error: error instanceof Error ? error.message : String(error), - }); - return false; + return ModuleActivation.unsupported(error instanceof Error ? error.message : String(error)); } }, - definitions: [ - mouseMoveTool, - mouseClickTool, - mouseDoubleClickTool, - mouseDragTool, - mouseScrollTool, - keyboardTypeTool, - keyboardKeyTapTool, - keyboardShortcutTool, - ], }; diff --git a/packages/@n8n/computer-use/src/tools/mouse-keyboard/mouse-keyboard.test.ts b/packages/@n8n/computer-use/src/tools/mouse-keyboard/mouse-keyboard.test.ts index 9069dc077bf..120eb8c3521 100644 --- a/packages/@n8n/computer-use/src/tools/mouse-keyboard/mouse-keyboard.test.ts +++ b/packages/@n8n/computer-use/src/tools/mouse-keyboard/mouse-keyboard.test.ts @@ -1,6 +1,7 @@ import robot from '@jitsi/robotjs'; import type { Mock, Mocked } from 'vitest'; +import type { ModuleContext } from '../types'; import { MouseKeyboardModule } from './index'; import { mouseMoveTool, @@ -253,7 +254,8 @@ describe('keyboard_shortcut', () => { ); }); -describe('MouseKeyboardModule.isSupported', () => { +describe('MouseKeyboardModule.activate', () => { + const MODULE_CTX = {} as ModuleContext; const originalWaylandDisplay = process.env.WAYLAND_DISPLAY; const originalDisplay = process.env.DISPLAY; @@ -276,19 +278,19 @@ describe('MouseKeyboardModule.isSupported', () => { process.env.WAYLAND_DISPLAY = 'wayland-0'; delete process.env.DISPLAY; - const result = await MouseKeyboardModule.isSupported(); + const result = await MouseKeyboardModule.activate(MODULE_CTX); - expect(result).toBe(false); + expect(result.supported).toBe(false); }); - it('returns true when robot loads successfully', async () => { + it('is supported when robot loads successfully', async () => { delete process.env.WAYLAND_DISPLAY; process.env.DISPLAY = ':0'; // The mock is already set up with getMousePos returning undefined (no throw) - const result = await MouseKeyboardModule.isSupported(); + const result = await MouseKeyboardModule.activate(MODULE_CTX); - expect(result).toBe(true); + expect(result.supported).toBe(true); }); it('returns false when robot native bindings fail to load', async () => { @@ -306,10 +308,10 @@ describe('MouseKeyboardModule.isSupported', () => { })); const { MouseKeyboardModule: IsolatedModule } = await import('./index'); - const result = await IsolatedModule.isSupported(); + const result = await IsolatedModule.activate(MODULE_CTX); vi.doUnmock('@jitsi/robotjs'); - expect(result).toBe(false); + expect(result.supported).toBe(false); }); }); diff --git a/packages/@n8n/computer-use/src/tools/screenshot/index.ts b/packages/@n8n/computer-use/src/tools/screenshot/index.ts index 992de6eac4a..8e47958d4b4 100644 --- a/packages/@n8n/computer-use/src/tools/screenshot/index.ts +++ b/packages/@n8n/computer-use/src/tools/screenshot/index.ts @@ -1,23 +1,19 @@ -import { logger } from '../../logger'; -import type { ToolModule } from '../types'; +import { ModuleActivation, type ToolModule } from '../types'; import { screenshotRegionTool, screenshotTool } from './screenshot'; export const ScreenshotModule: ToolModule = { - async isSupported() { + name: 'Screenshot', + category: 'screenshot', + permissionGroup: 'computer', + async activate() { try { const { Monitor } = await import('node-screenshots'); - const monitors = Monitor.all(); - if (monitors.length === 0) { - logger.info('Screenshot module not supported', { reason: 'no monitors detected' }); - return false; + if (Monitor.all().length === 0) { + return ModuleActivation.unsupported('No monitors detected'); } - return true; + return ModuleActivation.supported([screenshotTool, screenshotRegionTool]); } catch (error) { - logger.info('Screenshot module not supported', { - error: error instanceof Error ? error.message : String(error), - }); - return false; + return ModuleActivation.unsupported(error instanceof Error ? error.message : String(error)); } }, - definitions: [screenshotTool, screenshotRegionTool], }; diff --git a/packages/@n8n/computer-use/src/tools/screenshot/screenshot.test.ts b/packages/@n8n/computer-use/src/tools/screenshot/screenshot.test.ts index 0cc2905eeda..d60d75450ef 100644 --- a/packages/@n8n/computer-use/src/tools/screenshot/screenshot.test.ts +++ b/packages/@n8n/computer-use/src/tools/screenshot/screenshot.test.ts @@ -1,6 +1,7 @@ import { Monitor } from 'node-screenshots'; import type { Mock, MockedClass } from 'vitest'; +import type { ModuleContext } from '../types'; import { ScreenshotModule } from './index'; import { screenshotTool, screenshotRegionTool } from './screenshot'; @@ -20,6 +21,7 @@ vi.mock('@napi-rs/image', () => ({ const MockMonitor = Monitor as MockedClass; const DUMMY_CONTEXT = { dir: '/test/base' }; +const MODULE_CTX = {} as ModuleContext; interface MockImage { width: number; @@ -260,7 +262,7 @@ describe('screen_screenshot_region tool', () => { }); }); -describe('ScreenshotModule.isSupported', () => { +describe('ScreenshotModule.activate', () => { afterEach(() => { vi.clearAllMocks(); }); @@ -268,15 +270,17 @@ describe('ScreenshotModule.isSupported', () => { it.each([ ['has monitors', [{}], true], ['returns empty array', [], false], - ])('returns %s -> %s', async (_label, monitorList, expected) => { + ])('%s -> supported %s', async (_label, monitorList, expected) => { (MockMonitor.all as Mock).mockReturnValue(monitorList); - await expect(ScreenshotModule.isSupported()).resolves.toBe(expected); + const result = await ScreenshotModule.activate(MODULE_CTX); + expect(result.supported).toBe(expected); }); - it('returns false when Monitor.all() throws', async () => { + it('is unsupported when Monitor.all() throws', async () => { (MockMonitor.all as Mock).mockImplementation(() => { throw new Error('Display server unavailable'); }); - await expect(ScreenshotModule.isSupported()).resolves.toBe(false); + const result = await ScreenshotModule.activate(MODULE_CTX); + expect(result.supported).toBe(false); }); }); diff --git a/packages/@n8n/computer-use/src/tools/shell/index.ts b/packages/@n8n/computer-use/src/tools/shell/index.ts index ff9862502c9..c85884a6309 100644 --- a/packages/@n8n/computer-use/src/tools/shell/index.ts +++ b/packages/@n8n/computer-use/src/tools/shell/index.ts @@ -1,9 +1,16 @@ -import type { ToolModule } from '../types'; -import { shellExecuteTool } from './shell-execute'; +import { ModuleActivation, type ToolModule } from '../types'; +import { resetShellSandbox, resolveShellSandbox } from './sandbox'; +import { createShellExecuteTool } from './shell-execute'; export const ShellModule: ToolModule = { - isSupported() { - return true; + name: 'Shell', + category: 'shell', + permissionGroup: 'shell', + async activate({ config, dir }) { + const status = await resolveShellSandbox({ config, dir }); + if (!status.enabled) return ModuleActivation.unsupported(status.reason, status.hint); + // Only the sandboxed mode initializes the runtime, so only it needs teardown. + const shutdown = status.mode === 'sandboxed' ? resetShellSandbox : undefined; + return ModuleActivation.supported([createShellExecuteTool(status.mode)], shutdown); }, - definitions: [shellExecuteTool], }; diff --git a/packages/@n8n/computer-use/src/tools/shell/sandbox.test.ts b/packages/@n8n/computer-use/src/tools/shell/sandbox.test.ts new file mode 100644 index 00000000000..eae29e460b7 --- /dev/null +++ b/packages/@n8n/computer-use/src/tools/shell/sandbox.test.ts @@ -0,0 +1,203 @@ +import { SandboxManager } from '@anthropic-ai/sandbox-runtime'; +import { spawn } from 'child_process'; +import type { Mocked, MockedFunction } from 'vitest'; + +import { resetShellSandbox, resolveShellSandbox, spawnShell } from './sandbox'; +import type { GatewayConfig } from '../../config'; +import { getSettingsDir } from '../../config'; +import { logger } from '../../logger'; + +vi.mock('@vscode/ripgrep', () => ({ rgPath: '/usr/bin/rg' })); +vi.mock('child_process'); +vi.mock('@anthropic-ai/sandbox-runtime', () => ({ + ['SandboxManager']: { + isSupportedPlatform: vi.fn().mockReturnValue(true), + checkDependencies: vi.fn().mockReturnValue({ errors: [], warnings: [] }), + initialize: vi.fn().mockResolvedValue(undefined), + reset: vi.fn().mockResolvedValue(undefined), + wrapWithSandbox: vi + .fn() + .mockImplementation(async (cmd: string) => await Promise.resolve(`sbx(${cmd})`)), + }, +})); +vi.mock('../../logger', () => ({ + logger: { warn: vi.fn(), info: vi.fn(), debug: vi.fn(), error: vi.fn() }, +})); + +const mockSandbox = SandboxManager as Mocked; +const mockSpawn = spawn as MockedFunction; + +function makeConfig(dangerouslyDisableSandbox = false): GatewayConfig { + return { + logLevel: 'silent', + allowedOrigins: [], + filesystem: { dir: '/work' }, + computer: { shell: { timeout: 30_000, dangerouslyDisableSandbox } }, + browser: { defaultBrowser: 'chrome' }, + permissions: {}, + permissionConfirmation: 'client', + }; +} + +const originalPlatform = Object.getOwnPropertyDescriptor(process, 'platform')!; +function setPlatform(value: NodeJS.Platform) { + Object.defineProperty(process, 'platform', { value, configurable: true }); +} + +afterEach(() => { + vi.clearAllMocks(); + Object.defineProperty(process, 'platform', originalPlatform); + mockSandbox.isSupportedPlatform.mockReturnValue(true); + mockSandbox.checkDependencies.mockReturnValue({ errors: [], warnings: [] }); + mockSandbox.initialize.mockResolvedValue(undefined); +}); + +describe('resolveShellSandbox', () => { + it('enables the sandbox on macOS without extra dependencies', async () => { + setPlatform('darwin'); + + const status = await resolveShellSandbox({ config: makeConfig(), dir: '/work' }); + + expect(status).toEqual({ enabled: true, mode: 'sandboxed' }); + expect(mockSandbox.initialize).toHaveBeenCalledTimes(1); + }); + + it('enables the sandbox on Linux when dependencies are present', async () => { + setPlatform('linux'); + + const status = await resolveShellSandbox({ config: makeConfig(), dir: '/work' }); + + expect(status).toEqual({ enabled: true, mode: 'sandboxed' }); + expect(mockSandbox.checkDependencies).toHaveBeenCalled(); + expect(mockSandbox.initialize).toHaveBeenCalledTimes(1); + }); + + it('initializes with ~/.ssh and the settings dir denied, and the working dir writable', async () => { + setPlatform('linux'); + + await resolveShellSandbox({ config: makeConfig(), dir: '/work' }); + + const config = mockSandbox.initialize.mock.calls[0][0]; + expect(config.filesystem?.denyRead).toEqual( + expect.arrayContaining(['~/.ssh', getSettingsDir()]), + ); + expect(config.filesystem?.denyWrite).toContain(getSettingsDir()); + expect(config.filesystem?.allowWrite).toEqual(['/work']); + expect(config.ripgrep?.command).toBe('/usr/bin/rg'); + }); + + it('disables the sandbox (fail closed) when Linux dependencies are missing', async () => { + setPlatform('linux'); + mockSandbox.checkDependencies.mockReturnValue({ + errors: ['bubblewrap (bwrap) not installed'], + warnings: [], + }); + + const status = await resolveShellSandbox({ config: makeConfig(), dir: '/work' }); + + expect(status.enabled).toBe(false); + if (!status.enabled) expect(status.reason).toContain('bubblewrap'); + expect(mockSandbox.initialize).not.toHaveBeenCalled(); + }); + + it('disables the sandbox on native Windows', async () => { + setPlatform('win32'); + + const status = await resolveShellSandbox({ config: makeConfig(), dir: '/work' }); + + expect(status.enabled).toBe(false); + if (!status.enabled) expect(status.reason).toContain('Windows'); + expect(mockSandbox.initialize).not.toHaveBeenCalled(); + expect(mockSandbox.checkDependencies).not.toHaveBeenCalled(); + }); + + it('disables the sandbox on unsupported platforms (e.g. WSL1)', async () => { + setPlatform('linux'); + mockSandbox.isSupportedPlatform.mockReturnValue(false); + + const status = await resolveShellSandbox({ config: makeConfig(), dir: '/work' }); + + expect(status.enabled).toBe(false); + expect(mockSandbox.initialize).not.toHaveBeenCalled(); + }); + + it('disables the sandbox (fail closed) when initialization throws', async () => { + setPlatform('linux'); + mockSandbox.initialize.mockRejectedValue(new Error('bwrap: user namespaces disabled')); + + const status = await resolveShellSandbox({ config: makeConfig(), dir: '/work' }); + + expect(status.enabled).toBe(false); + if (!status.enabled) expect(status.reason).toContain('initialize'); + }); + + describe('explicit opt-out (--dangerously-disable-shell-sandbox)', () => { + it('runs unsandboxed and warns, without checking deps or initializing', async () => { + setPlatform('linux'); + + const status = await resolveShellSandbox({ config: makeConfig(true), dir: '/work' }); + + expect(status).toEqual({ enabled: true, mode: 'unsandboxed' }); + expect(mockSandbox.checkDependencies).not.toHaveBeenCalled(); + expect(mockSandbox.initialize).not.toHaveBeenCalled(); + expect(logger.warn).toHaveBeenCalledWith( + expect.stringContaining('--dangerously-disable-shell-sandbox'), + ); + }); + + it('opts out even on native Windows', async () => { + setPlatform('win32'); + + const status = await resolveShellSandbox({ config: makeConfig(true), dir: '/work' }); + + expect(status).toEqual({ enabled: true, mode: 'unsandboxed' }); + }); + }); +}); + +describe('spawnShell', () => { + it('wraps the command with the sandbox when sandboxed', async () => { + await spawnShell('cat /etc/passwd', { cwd: '/work', mode: 'sandboxed' }); + + expect(mockSandbox.wrapWithSandbox).toHaveBeenCalledWith('cat /etc/passwd'); + const [executable, options] = mockSpawn.mock.calls[0]; + expect(executable).toBe('sbx(cat /etc/passwd)'); + expect(options).toMatchObject({ shell: true, cwd: '/work' }); + }); + + it('fails closed (never spawns) when the sandbox wrap throws', async () => { + mockSandbox.wrapWithSandbox.mockRejectedValue(new Error('bwrap: user namespaces disabled')); + + await expect(spawnShell('cat /etc/passwd', { mode: 'sandboxed' })).rejects.toThrow('bwrap'); + expect(mockSpawn).not.toHaveBeenCalled(); + }); + + it('runs bare sh on non-Windows when unsandboxed', async () => { + setPlatform('linux'); + + await spawnShell('id', { cwd: '/work', mode: 'unsandboxed' }); + + expect(mockSandbox.wrapWithSandbox).not.toHaveBeenCalled(); + const [executable, args] = mockSpawn.mock.calls[0]; + expect(executable).toBe('sh'); + expect(args).toEqual(['-c', 'id']); + }); + + it('runs cmd.exe on Windows when unsandboxed', async () => { + setPlatform('win32'); + + await spawnShell('dir', { mode: 'unsandboxed' }); + + const [executable, args] = mockSpawn.mock.calls[0]; + expect(executable).toBe('cmd.exe'); + expect(args).toEqual(['/C', 'dir']); + }); +}); + +describe('resetShellSandbox', () => { + it('tears down the sandbox', async () => { + await resetShellSandbox(); + + expect(mockSandbox.reset).toHaveBeenCalledTimes(1); + }); +}); diff --git a/packages/@n8n/computer-use/src/tools/shell/sandbox.ts b/packages/@n8n/computer-use/src/tools/shell/sandbox.ts new file mode 100644 index 00000000000..834e5e0e6e0 --- /dev/null +++ b/packages/@n8n/computer-use/src/tools/shell/sandbox.ts @@ -0,0 +1,128 @@ +import type { SandboxRuntimeConfig } from '@anthropic-ai/sandbox-runtime'; +import { spawn, type ChildProcess } from 'node:child_process'; + +import { getSettingsDir, type GatewayConfig } from '../../config'; +import { logger } from '../../logger'; + +export type ShellSandboxMode = 'sandboxed' | 'unsandboxed'; + +export type ShellSandboxStatus = + | { enabled: true; mode: ShellSandboxMode } + | { enabled: false; reason: string; hint?: string }; + +const SANDBOX_DOCS = 'https://www.npmjs.com/package/@n8n/computer-use#shell-sandboxing'; + +function buildSandboxConfig({ + dir, + rgPath, +}: { dir: string; rgPath: string }): SandboxRuntimeConfig { + return { + // Use the bundled ripgrep so a system rg isn't required. + ripgrep: { command: rgPath }, + network: { + allowedDomains: [], + deniedDomains: [], + }, + filesystem: { + denyRead: ['~/.ssh', getSettingsDir()], + allowRead: [], + allowWrite: [dir], + denyWrite: [getSettingsDir()], + }, + }; +} + +/** + * Resolve whether shell execution can run inside the OS sandbox. Fail-closed: + * when the sandbox is unavailable the shell tool is not registered, unless + * `--dangerously-disable-shell-sandbox` opts out. + */ +export async function resolveShellSandbox({ + config, + dir, +}: { + config: GatewayConfig; + dir: string; +}): Promise { + if (config.computer.shell.dangerouslyDisableSandbox) { + logger.warn( + 'Shell sandbox disabled via --dangerously-disable-shell-sandbox: commands run without OS-level isolation. Only use this in a trusted, isolated environment.', + ); + return { enabled: true, mode: 'unsandboxed' }; + } + + // The native Windows backend isn't a real security boundary; require WSL2. + if (process.platform === 'win32') { + return { + enabled: false, + reason: 'Shell sandbox is not available on native Windows', + hint: `Run inside WSL2 (needs bubblewrap + socat), or pass --dangerously-disable-shell-sandbox to run without isolation.\nDocs: ${SANDBOX_DOCS}`, + }; + } + + const { SandboxManager } = await import('@anthropic-ai/sandbox-runtime'); + const { rgPath } = await import('@vscode/ripgrep'); + + if (!SandboxManager.isSupportedPlatform()) { + return { + enabled: false, + reason: 'Shell sandbox is not supported on this platform', + hint: `Use macOS, Linux, or WSL2 (WSL1 is not supported), or pass --dangerously-disable-shell-sandbox to run without isolation.\nDocs: ${SANDBOX_DOCS}`, + }; + } + + // Checks for required binaries (bwrap/socat on Linux); can't verify the kernel + // allows user namespaces, so initialize() below may still fail closed. + const deps = SandboxManager.checkDependencies({ command: rgPath }); + if (deps.errors.length > 0) { + return { + enabled: false, + reason: `Shell sandbox dependencies missing: ${deps.errors.join(', ')}`, + hint: + process.platform === 'linux' + ? `Debian/Ubuntu: sudo apt-get install bubblewrap socat\nFedora: sudo dnf install bubblewrap socat\nUbuntu 24.04+: also allow bwrap user namespaces\nDocs: ${SANDBOX_DOCS}` + : undefined, + }; + } + for (const warning of deps.warnings) { + logger.warn('Shell sandbox degraded', { warning }); + } + + try { + await SandboxManager.initialize(buildSandboxConfig({ dir, rgPath })); + } catch (error) { + return { + enabled: false, + reason: `Shell sandbox failed to initialize: ${error instanceof Error ? error.message : String(error)}`, + hint: `Sandbox tools are installed but couldn't start, usually blocked user namespaces.\nUbuntu 24.04+: allow bwrap user namespaces. Docker: run with --privileged.\nDocs: ${SANDBOX_DOCS}`, + }; + } + + return { enabled: true, mode: 'sandboxed' }; +} + +/** Tear down the sandbox and its proxy processes. */ +export async function resetShellSandbox(): Promise { + const { SandboxManager } = await import('@anthropic-ai/sandbox-runtime'); + await SandboxManager.reset(); +} + +/** + * Launch a shell command. 'sandboxed' wraps it with the OS sandbox; a wrap + * failure throws so the caller fails closed rather than running unsandboxed. + */ +export async function spawnShell( + command: string, + { cwd, mode }: { cwd?: string; mode: ShellSandboxMode }, +): Promise { + if (mode === 'sandboxed') { + const { SandboxManager } = await import('@anthropic-ai/sandbox-runtime'); + const sandboxedCommand = await SandboxManager.wrapWithSandbox(command); + return spawn(sandboxedCommand, { shell: true, cwd }); + } + + if (process.platform === 'win32') { + return spawn('cmd.exe', ['/C', command], { cwd }); + } + return spawn('sh', ['-c', command], { cwd }); +} diff --git a/packages/@n8n/computer-use/src/tools/shell/shell-execute.test.ts b/packages/@n8n/computer-use/src/tools/shell/shell-execute.test.ts index 131e7e47a8f..7be08f9fbe8 100644 --- a/packages/@n8n/computer-use/src/tools/shell/shell-execute.test.ts +++ b/packages/@n8n/computer-use/src/tools/shell/shell-execute.test.ts @@ -3,12 +3,10 @@ import { spawn } from 'child_process'; import { EventEmitter } from 'events'; import type { Mock, Mocked, MockedFunction } from 'vitest'; -import { getSettingsDir } from '../../config'; import { textOf } from '../test-utils'; import type { AffectedResource } from '../types'; import { buildShellResource } from './build-shell-resource'; -import { ShellModule } from './index'; -import { shellExecuteTool } from './shell-execute'; +import { createShellExecuteTool } from './shell-execute'; vi.mock('child_process'); vi.mock('@vscode/ripgrep', () => ({ rgPath: '/usr/bin/rg' })); @@ -24,6 +22,10 @@ const mockSandboxManager = SandboxManager as Mocked; const mockSpawn = spawn as MockedFunction; +// Plumbing tests use the unsandboxed tool (sh -c) so spawn is called with the +// classic (executable, args, options) shape. Sandbox-mode behavior is covered in +// the cross-platform block below and in sandbox.test.ts. +const shellExecuteTool = createShellExecuteTool('unsandboxed'); const DUMMY_CONTEXT = { dir: '/test/base' }; function makeMockChild( @@ -59,6 +61,9 @@ function getErrorHandler(on: Mock): ((error: Error) => void) | undefined { async function flushMicrotasks(ticks = 1) { for (let i = 0; i < ticks; i++) await Promise.resolve(); } +async function flushUntil(predicate: () => boolean, maxTicks = 50) { + for (let i = 0; i < maxTicks && !predicate(); i++) await Promise.resolve(); +} describe('shell_execute tool', () => { const originalPlatform = Object.getOwnPropertyDescriptor(process, 'platform'); @@ -337,56 +342,28 @@ describe('shell_execute tool', () => { expect(args).toEqual(['-c', 'ls']); }); - it('wraps command with SandboxManager on darwin', async () => { - Object.defineProperty(process, 'platform', { value: 'darwin', configurable: true }); + it('wraps the command with the sandbox when sandboxed, regardless of platform', async () => { mockSandboxManager.wrapWithSandbox.mockResolvedValue('sandboxed-ls'); const child = makeMockChild(); mockSpawn.mockReturnValue(child as unknown as ReturnType); - const resultPromise = shellExecuteTool.execute( - { command: 'ls', timeout: 5000 }, - DUMMY_CONTEXT, - ); + const sandboxedTool = createShellExecuteTool('sandboxed'); + const resultPromise = sandboxedTool.execute({ command: 'ls', timeout: 5000 }, DUMMY_CONTEXT); - // darwin path has extra async depth: initializeSandbox (×2 awaits) + wrapWithSandbox + return + .then() - await flushMicrotasks(5); + // The sandboxed path lazy-imports the runtime and awaits the wrap, so wait + // for the spawn rather than counting microtasks. + await flushUntil(() => mockSpawn.mock.calls.length > 0); const closeHandler = getCloseHandler(child.on); closeHandler?.(0); await resultPromise; - expect(mockSandboxManager.initialize).toHaveBeenCalled(); expect(mockSandboxManager.wrapWithSandbox).toHaveBeenCalledWith('ls'); const [executable, spawnOptions] = mockSpawn.mock.calls[0]; expect(executable).toBe('sandboxed-ls'); expect(spawnOptions).toMatchObject({ shell: true }); }); - - it('includes settings dir in sandbox denyWrite and denyRead on darwin', async () => { - Object.defineProperty(process, 'platform', { value: 'darwin', configurable: true }); - mockSandboxManager.wrapWithSandbox.mockResolvedValue('sandboxed-ls'); - - const child = makeMockChild(); - mockSpawn.mockReturnValue(child as unknown as ReturnType); - - const resultPromise = shellExecuteTool.execute( - { command: 'ls', timeout: 5000 }, - DUMMY_CONTEXT, - ); - - await flushMicrotasks(5); - - const closeHandler = getCloseHandler(child.on); - closeHandler?.(0); - await resultPromise; - - const initCall = mockSandboxManager.initialize.mock.calls[0][0] as { - filesystem: { denyWrite: string[]; denyRead: string[] }; - }; - expect(initCall.filesystem.denyWrite).toContain(getSettingsDir()); - expect(initCall.filesystem.denyRead).toContain(getSettingsDir()); - }); }); describe('result JSON structure', () => { @@ -446,12 +423,6 @@ describe('shell_execute tool', () => { }); }); -describe('ShellModule', () => { - it('isSupported returns true', () => { - expect(ShellModule.isSupported()).toBe(true); - }); -}); - describe('getAffectedResources', () => { it('uses buildShellResource for the resource and includes the full command in description', () => { const resources = shellExecuteTool.getAffectedResources( diff --git a/packages/@n8n/computer-use/src/tools/shell/shell-execute.ts b/packages/@n8n/computer-use/src/tools/shell/shell-execute.ts index 717435cef8a..7f49102ef1c 100644 --- a/packages/@n8n/computer-use/src/tools/shell/shell-execute.ts +++ b/packages/@n8n/computer-use/src/tools/shell/shell-execute.ts @@ -1,32 +1,10 @@ -import { SandboxManager, type SandboxRuntimeConfig } from '@anthropic-ai/sandbox-runtime'; -import { rgPath } from '@vscode/ripgrep'; -import { spawn } from 'child_process'; import path from 'node:path'; import { z } from 'zod'; -import { getSettingsDir } from '../../config'; import type { CallToolResult, ToolDefinition } from '../types'; import { formatCallToolResult, formatErrorResult } from '../utils'; import { buildShellResource } from './build-shell-resource'; - -async function initializeSandbox({ dir }: { dir: string }) { - const config: SandboxRuntimeConfig = { - ripgrep: { - command: rgPath, - }, - network: { - allowedDomains: [], - deniedDomains: [], - }, - filesystem: { - denyRead: ['~/.ssh', getSettingsDir()], - allowRead: [], - allowWrite: [dir], - denyWrite: [getSettingsDir()], - }, - }; - await SandboxManager.initialize(config); -} +import { spawnShell, type ShellSandboxMode } from './sandbox'; const inputSchema = z.object({ command: z.string().describe('Shell command to execute'), @@ -38,81 +16,73 @@ function resolveCommandPath(dir: string, cwd: string | undefined) { return path.resolve(dir, cwd ?? '.'); } -export const shellExecuteTool: ToolDefinition = { - name: 'shell_execute', - description: 'Execute a shell command and return stdout, stderr, and exit code', - inputSchema, - annotations: { destructiveHint: true }, - getAffectedResources({ command, cwd }, { dir }) { - const resolvedPath = resolveCommandPath(dir, cwd); - const resource = buildShellResource(command, resolvedPath); - const description = `Execute shell command: ${command} in ${resolvedPath}`; - return [ - { - toolGroup: 'shell' as const, - resource, - description, - }, - ]; - }, - async execute({ command, timeout = 30_000, cwd }, { dir }) { - return await runCommand(command, { timeout, dir, cwd: resolveCommandPath(dir, cwd) }); - }, -}; - -async function spawnCommand(command: string, { dir, cwd }: { dir: string; cwd?: string }) { - const isWindows = process.platform === 'win32'; - const isMac = process.platform === 'darwin'; - - if (isWindows) { - return spawn('cmd.exe', ['/C', command], { cwd }); - } - - if (isMac) { - await initializeSandbox({ dir }); - const sandboxedCommand = await SandboxManager.wrapWithSandbox(command); - return spawn(sandboxedCommand, { shell: true, cwd }); - } - - return spawn('sh', ['-c', command], { cwd }); +/** Mode is baked in at creation, so an unsandboxed shell is only reachable when explicitly built as such. */ +export function createShellExecuteTool(mode: ShellSandboxMode): ToolDefinition { + return { + name: 'shell_execute', + description: 'Execute a shell command and return stdout, stderr, and exit code', + inputSchema, + annotations: { destructiveHint: true }, + getAffectedResources({ command, cwd }, { dir }) { + const resolvedPath = resolveCommandPath(dir, cwd); + const resource = buildShellResource(command, resolvedPath); + const description = `Execute shell command: ${command} in ${resolvedPath}`; + return [ + { + toolGroup: 'shell' as const, + resource, + description, + }, + ]; + }, + async execute({ command, timeout = 30_000, cwd }, { dir }) { + return await runCommand(command, { timeout, cwd: resolveCommandPath(dir, cwd), mode }); + }, + }; } async function runCommand( command: string, - { timeout, cwd, dir }: { timeout: number; dir: string; cwd?: string }, + { timeout, cwd, mode }: { timeout: number; cwd?: string; mode: ShellSandboxMode }, ): Promise { - return await new Promise((resolve, reject) => { - spawnCommand(command, { dir, cwd }) - .then((child) => { - let stdout = ''; - let stderr = ''; + let child; + try { + child = await spawnShell(command, { cwd, mode }); + } catch (error) { + // Fail closed: never run unsandboxed. + return formatErrorResult( + `Failed to start sandboxed process: ${error instanceof Error ? error.message : String(error)}`, + ); + } - child.stdout?.on('data', (chunk: Buffer) => { - stdout += String(chunk); - }); - child.stderr?.on('data', (chunk: Buffer) => { - stderr += String(chunk); - }); + return await new Promise((resolve) => { + let stdout = ''; + let stderr = ''; - const timer = setTimeout(() => { - child.kill(); - resolve(formatCallToolResult({ stdout, stderr, exitCode: null, timedOut: true })); - }, timeout); + child.stdout?.on('data', (chunk: Buffer) => { + stdout += String(chunk); + }); + child.stderr?.on('data', (chunk: Buffer) => { + stderr += String(chunk); + }); - child.on('close', (code) => { - clearTimeout(timer); - const result = formatCallToolResult({ stdout, stderr, exitCode: code }); - if (code !== 0) { - result.isError = true; - } - resolve(result); - }); + const timer = setTimeout(() => { + child.kill(); + resolve(formatCallToolResult({ stdout, stderr, exitCode: null, timedOut: true })); + }, timeout); - child.on('error', (error) => { - clearTimeout(timer); - resolve(formatErrorResult(`Failed to start process: ${error.message}`)); - }); - }) - .catch(reject); + child.on('close', (code) => { + clearTimeout(timer); + const result = formatCallToolResult({ stdout, stderr, exitCode: code }); + if (code !== 0) { + result.isError = true; + } + resolve(result); + }); + + child.on('error', (error) => { + clearTimeout(timer); + resolve(formatErrorResult(`Failed to start process: ${error.message}`)); + }); }); } diff --git a/packages/@n8n/computer-use/src/tools/types.ts b/packages/@n8n/computer-use/src/tools/types.ts index ab433428c33..dd71e5888a3 100644 --- a/packages/@n8n/computer-use/src/tools/types.ts +++ b/packages/@n8n/computer-use/src/tools/types.ts @@ -2,7 +2,7 @@ import type { CallToolResult } from '@modelcontextprotocol/sdk/types.js'; import type { CreateCredentialPayload, SecretsBuffer } from '@n8n/mcp-browser'; import type { z } from 'zod'; -import type { ToolGroup } from '../config'; +import type { GatewayConfig, ToolGroup } from '../config'; export type { CallToolResult, CreateCredentialPayload, SecretsBuffer }; @@ -85,9 +85,31 @@ export interface ToolDefinition { ): AffectedResource[] | Promise; } -export interface ToolModule { - /** Return false if this module cannot run on the current platform or lacks required permissions */ - isSupported(): boolean | Promise; - /** Tool definitions provided by this module */ - definitions: ToolDefinition[]; +/** Inputs a module needs to resolve availability and build its tools. */ +export interface ModuleContext { + config: GatewayConfig; + dir: string; +} + +/** Result of activating a module: available with tools (+ optional teardown), or unavailable with a reason. */ +export type ModuleActivation = + | { supported: true; tools: ToolDefinition[]; shutdown?: () => Promise } + | { supported: false; reason: string; hint?: string }; + +export const ModuleActivation = { + supported(tools: ToolDefinition[], shutdown?: () => Promise): ModuleActivation { + return { supported: true, tools, ...(shutdown ? { shutdown } : {}) }; + }, + unsupported(reason: string, hint?: string): ModuleActivation { + return { supported: false, reason, ...(hint ? { hint } : {}) }; + }, +} as const; + +export interface ToolModule { + readonly name: string; + /** Annotation used to route tools to the correct sub-agent (e.g. 'browser'). */ + readonly category: string; + readonly permissionGroup: ToolGroup; + /** Resolve availability once at startup; may do one-time init (native imports, sandbox setup). */ + activate(context: ModuleContext): ModuleActivation | Promise; } diff --git a/packages/@n8n/instance-ai/src/parsers/__tests__/ooxml-guard.test.ts b/packages/@n8n/instance-ai/src/parsers/__tests__/ooxml-guard.test.ts new file mode 100644 index 00000000000..e55a2dd842f --- /dev/null +++ b/packages/@n8n/instance-ai/src/parsers/__tests__/ooxml-guard.test.ts @@ -0,0 +1,68 @@ +import { + assertOoxmlWithinBounds, + MAX_OOXML_ENTRIES, + MAX_OOXML_UNCOMPRESSED_BYTES, +} from '../ooxml-guard'; + +// ── Minimal ZIP builders ───────────────────────────────────────────────────── +// `assertOoxmlWithinBounds` only reads the central directory + EOCD, so the tests +// build just those records (no local file data) and point the EOCD at offset 0. + +function centralHeader(name: string, uncompressedSize: number): Buffer { + const nameBuf = Buffer.from(name, 'utf-8'); + const header = Buffer.alloc(46 + nameBuf.length); + header.writeUInt32LE(0x02014b50, 0); // central file header signature + header.writeUInt32LE(uncompressedSize >>> 0, 24); // uncompressed size + header.writeUInt16LE(nameBuf.length, 28); // file name length + nameBuf.copy(header, 46); + return header; +} + +function makeZip(entries: Array<{ name: string; uncompressedSize: number }>): Buffer { + const cd = Buffer.concat(entries.map((e) => centralHeader(e.name, e.uncompressedSize))); + const eocd = Buffer.alloc(22); + eocd.writeUInt32LE(0x06054b50, 0); // EOCD signature + eocd.writeUInt16LE(entries.length, 8); // entries on this disk + eocd.writeUInt16LE(entries.length, 10); // total entries + eocd.writeUInt32LE(cd.length, 12); // central directory size + eocd.writeUInt32LE(0, 16); // central directory offset + return Buffer.concat([cd, eocd]); +} + +describe('assertOoxmlWithinBounds', () => { + it('accepts a normal archive within bounds', () => { + const zip = makeZip([ + { name: 'word/document.xml', uncompressedSize: 50_000 }, + { name: '[Content_Types].xml', uncompressedSize: 1_000 }, + ]); + expect(() => assertOoxmlWithinBounds(zip, 'doc.docx')).not.toThrow(); + }); + + it('rejects when total declared uncompressed size exceeds the cap', () => { + const zip = makeZip([ + { name: 'sheet1.xml', uncompressedSize: MAX_OOXML_UNCOMPRESSED_BYTES }, + { name: 'sheet2.xml', uncompressedSize: 1 }, + ]); + expect(() => assertOoxmlWithinBounds(zip, 'bomb.xlsx')).toThrow(/decompresses to more than/); + }); + + it('rejects when entry count exceeds the cap', () => { + const entries = Array.from({ length: MAX_OOXML_ENTRIES + 1 }, (_, i) => ({ + name: `part${i}.xml`, + uncompressedSize: 1, + })); + expect(() => assertOoxmlWithinBounds(makeZip(entries), 'bomb.xlsx')).toThrow( + /too many archive entries/, + ); + }); + + it('rejects a ZIP64-sized entry without decoding the extra field', () => { + const zip = makeZip([{ name: 'huge.bin', uncompressedSize: 0xffffffff }]); + expect(() => assertOoxmlWithinBounds(zip, 'bomb.xlsx')).toThrow(/ZIP64/); + }); + + it('passes through a buffer with no central directory (downstream parser will reject)', () => { + const notAZip = Buffer.from('this is plain text, not an archive', 'utf-8'); + expect(() => assertOoxmlWithinBounds(notAZip, 'plain.txt')).not.toThrow(); + }); +}); diff --git a/packages/@n8n/instance-ai/src/parsers/docx-parser.ts b/packages/@n8n/instance-ai/src/parsers/docx-parser.ts index 62cf8908392..cb66380a6ff 100644 --- a/packages/@n8n/instance-ai/src/parsers/docx-parser.ts +++ b/packages/@n8n/instance-ai/src/parsers/docx-parser.ts @@ -1,3 +1,4 @@ +import { assertOoxmlWithinBounds } from './ooxml-guard'; import { MAX_DECODED_SIZE_BYTES, MAX_RESULT_CHARS, @@ -18,6 +19,7 @@ export async function extractDocxText(attachment: AttachmentInfo): Promise MAX_DECODED_SIZE_BYTES) { throw new Error(formatSizeLimitMessage(decoded.length)); } + assertOoxmlWithinBounds(decoded, attachment.fileName); const mammoth = await import('mammoth'); const extractRawText = mammoth.extractRawText ?? mammoth.default?.extractRawText; diff --git a/packages/@n8n/instance-ai/src/parsers/ooxml-guard.ts b/packages/@n8n/instance-ai/src/parsers/ooxml-guard.ts new file mode 100644 index 00000000000..b17b339d1f1 --- /dev/null +++ b/packages/@n8n/instance-ai/src/parsers/ooxml-guard.ts @@ -0,0 +1,83 @@ +/** + * Decompression-bomb guard for OOXML attachments (`.docx`, `.xlsx`, …), which are + * ZIP containers. The compressed payload is already capped (MAX_DECODED_SIZE_BYTES), + * but a small archive can still inflate to many gigabytes and OOM the instance. + * + * `mammoth` / SheetJS parse synchronously, so a wall-clock timeout can't interrupt + * them on the main thread. Instead we read the ZIP central directory (cheap — no + * inflation) and reject up front when the declared uncompressed size or entry count + * exceeds a sane bound. + */ + +const EOCD_SIGNATURE = 0x06054b50; // "PK\x05\x06" +const CENTRAL_FILE_HEADER_SIGNATURE = 0x02014b50; // "PK\x01\x02" +const EOCD_MIN_SIZE = 22; +const CENTRAL_FILE_HEADER_MIN_SIZE = 46; +const MAX_EOCD_COMMENT = 0xffff; +const ZIP64_SIZE_MARKER = 0xffffffff; + +/** Total declared uncompressed size across all entries (200 MB). */ +export const MAX_OOXML_UNCOMPRESSED_BYTES = 200 * 1024 * 1024; +/** Max number of entries in the archive. */ +export const MAX_OOXML_ENTRIES = 1024; + +function formatMB(bytes: number): string { + return `${(bytes / (1024 * 1024)).toFixed(1)} MB`; +} + +/** + * Reads the ZIP central directory of `buffer` and throws when the archive declares + * more than `MAX_OOXML_UNCOMPRESSED_BYTES` of uncompressed data or more than + * `MAX_OOXML_ENTRIES` entries. A buffer without a readable central directory is + * left untouched — it isn't a valid OOXML archive and the downstream parser will + * reject it (the compressed size is already bounded). + */ +export function assertOoxmlWithinBounds(buffer: Buffer, fileName: string): void { + const eocd = findEndOfCentralDirectory(buffer); + if (eocd === undefined) return; + + const entryCount = buffer.readUInt16LE(eocd + 10); + if (entryCount > MAX_OOXML_ENTRIES) { + throw new Error( + `"${fileName}" has too many archive entries (${entryCount} > ${MAX_OOXML_ENTRIES}); refusing to parse a potential decompression bomb.`, + ); + } + + let offset = buffer.readUInt32LE(eocd + 16); + let totalUncompressed = 0; + for (let i = 0; i < entryCount; i++) { + if (offset + CENTRAL_FILE_HEADER_MIN_SIZE > buffer.length) break; + if (buffer.readUInt32LE(offset) !== CENTRAL_FILE_HEADER_SIGNATURE) break; + + const uncompressed = buffer.readUInt32LE(offset + 24); + // A ZIP64 marker means the real size doesn't fit in 32 bits (>= 4 GB) — well + // past any sane bound, so reject without decoding the ZIP64 extra field. + if (uncompressed === ZIP64_SIZE_MARKER) { + throw new Error( + `"${fileName}" declares a ZIP64-sized entry; refusing to parse a potential decompression bomb.`, + ); + } + + totalUncompressed += uncompressed; + if (totalUncompressed > MAX_OOXML_UNCOMPRESSED_BYTES) { + throw new Error( + `"${fileName}" decompresses to more than ${formatMB(MAX_OOXML_UNCOMPRESSED_BYTES)}; refusing to parse a potential decompression bomb.`, + ); + } + + const nameLen = buffer.readUInt16LE(offset + 28); + const extraLen = buffer.readUInt16LE(offset + 30); + const commentLen = buffer.readUInt16LE(offset + 32); + offset += CENTRAL_FILE_HEADER_MIN_SIZE + nameLen + extraLen + commentLen; + } +} + +/** Scans backwards for the End Of Central Directory record, tolerating a trailing comment. */ +function findEndOfCentralDirectory(buffer: Buffer): number | undefined { + if (buffer.length < EOCD_MIN_SIZE) return undefined; + const scanLimit = Math.max(0, buffer.length - EOCD_MIN_SIZE - MAX_EOCD_COMMENT); + for (let i = buffer.length - EOCD_MIN_SIZE; i >= scanLimit; i--) { + if (buffer.readUInt32LE(i) === EOCD_SIGNATURE) return i; + } + return undefined; +} diff --git a/packages/@n8n/instance-ai/src/parsers/xlsx-parser.ts b/packages/@n8n/instance-ai/src/parsers/xlsx-parser.ts index cb4b4c1047b..798fcf32e79 100644 --- a/packages/@n8n/instance-ai/src/parsers/xlsx-parser.ts +++ b/packages/@n8n/instance-ai/src/parsers/xlsx-parser.ts @@ -1,3 +1,4 @@ +import { assertOoxmlWithinBounds } from './ooxml-guard'; import { MAX_DECODED_SIZE_BYTES, formatSizeLimitMessage, @@ -23,6 +24,7 @@ export async function extractXlsxAsRows( if (decoded.length > MAX_DECODED_SIZE_BYTES) { throw new Error(formatSizeLimitMessage(decoded.length)); } + assertOoxmlWithinBounds(decoded, attachment.fileName); const XLSX = await import('xlsx'); diff --git a/packages/@n8n/instance-ai/src/tools/__tests__/executions.tool.test.ts b/packages/@n8n/instance-ai/src/tools/__tests__/executions.tool.test.ts index 99242e2d8ed..e9ae08f6524 100644 --- a/packages/@n8n/instance-ai/src/tools/__tests__/executions.tool.test.ts +++ b/packages/@n8n/instance-ai/src/tools/__tests__/executions.tool.test.ts @@ -245,13 +245,14 @@ describe('executions tool', () => { expect(result).toEqual(executionResult); }); - it('should execute immediately when permission is always_allow', async () => { + it('should execute immediately when always_allow + workflow was created by the agent', async () => { const executionResult: ExecutionResult = { executionId: 'exec-456', status: 'success', }; const context = createMockContext({ permissions: { runWorkflow: 'always_allow' }, + aiCreatedWorkflowIds: new Set(['wf-1']), }); (context.executionService.run as Mock).mockResolvedValue(executionResult); @@ -273,6 +274,7 @@ describe('executions tool', () => { it('should pass undefined inputData when not provided', async () => { const context = createMockContext({ permissions: { runWorkflow: 'always_allow' }, + aiCreatedWorkflowIds: new Set(['wf-1']), }); (context.executionService.run as Mock).mockResolvedValue({ executionId: 'exec-1', @@ -606,6 +608,70 @@ describe('executions tool', () => { expect(result).toBeUndefined(); }); }); + + describe('aiCreatedWorkflowIds scope (no explicit allow-list)', () => { + it('runs without HITL when always_allow + workflow was created by the agent', async () => { + const context = createMockContext({ + permissions: { runWorkflow: 'always_allow' }, + aiCreatedWorkflowIds: new Set(['wf-built']), + }); + (context.executionService.run as Mock).mockResolvedValue({ + executionId: 'exec-1', + status: 'success', + }); + const suspendFn = vi.fn(); + + const tool = createExecutionsTool(context); + await executeTool( + tool, + { action: 'run' as const, workflowId: 'wf-built' }, + createAgentCtx({ suspend: suspendFn }) as never, + ); + + expect(suspendFn).not.toHaveBeenCalled(); + expect(context.executionService.run).toHaveBeenCalledWith('wf-built', undefined, { + timeout: undefined, + }); + }); + + it('still requires HITL for a pre-existing workflow the agent did not create', async () => { + const context = createMockContext({ + permissions: { runWorkflow: 'always_allow' }, + aiCreatedWorkflowIds: new Set(['wf-built']), + }); + (context.workflowService.get as Mock).mockResolvedValue({ name: 'Pre-existing WF' }); + const suspendFn = vi.fn(); + + const tool = createExecutionsTool(context); + const result = await executeTool( + tool, + { action: 'run' as const, workflowId: 'wf-preexisting' }, + createAgentCtx({ suspend: suspendFn }) as never, + ); + + expect(suspendFn).toHaveBeenCalled(); + expect(context.executionService.run).not.toHaveBeenCalled(); + expect(result).toBeUndefined(); + }); + + it('requires HITL when always_allow is set but the agent created nothing', async () => { + const context = createMockContext({ + permissions: { runWorkflow: 'always_allow' }, + }); + (context.workflowService.get as Mock).mockResolvedValue({ name: 'Some WF' }); + const suspendFn = vi.fn(); + + const tool = createExecutionsTool(context); + await executeTool( + tool, + { action: 'run' as const, workflowId: 'wf-1' }, + createAgentCtx({ suspend: suspendFn }) as never, + ); + + expect(suspendFn).toHaveBeenCalled(); + expect(context.executionService.run).not.toHaveBeenCalled(); + }); + }); }); // ── debug ─────────────────────────────────────────────────────────────── diff --git a/packages/@n8n/instance-ai/src/tools/executions.tool.ts b/packages/@n8n/instance-ai/src/tools/executions.tool.ts index d00a35fcb59..049c5d70974 100644 --- a/packages/@n8n/instance-ai/src/tools/executions.tool.ts +++ b/packages/@n8n/instance-ai/src/tools/executions.tool.ts @@ -212,10 +212,13 @@ async function handleRun( }; } - // `always_allow` is only honored for the workflow IDs the caller pre-authorized - // (e.g. checkpoint follow-ups scope the override to the workflows the checkpoint - // is verifying). When the allow-list is unset, `always_allow` applies broadly, - // matching the legacy behavior. + // `always_allow` is only honored for the workflow IDs the caller pre-authorized. + // Checkpoint follow-ups pass an explicit allow-list (the workflows the checkpoint is + // verifying). When the allow-list is unset (e.g. planned-build follow-ups, which grant + // `runWorkflow: 'always_allow'` without one), the bypass is scoped to the workflows the + // agent created during the active plan cycle. Running any other pre-existing workflow + // still requires HITL approval, so a prompt injection can't silently run arbitrary + // workflows under the user's authority. const allowList = context.allowedRunWorkflowIds; const workflowNameAllowList = context.allowedRunWorkflowNames; let workflowName: string | undefined; @@ -244,10 +247,14 @@ async function handleRun( allowedByName = true; } } + const allowedByList = + allowList !== undefined + ? allowList.has(workflowId) + : (context.aiCreatedWorkflowIds?.has(workflowId) ?? false); const allowedByScope = context.requireRunWorkflowApproval !== true && context.permissions?.runWorkflow === 'always_allow' && - (allowList === undefined || allowList.has(workflowId) || allowedByName); + (allowedByList || allowedByName); // A per-workflow "always allow" grant skips HITL for the rest of the session, but an // admin's `requireRunWorkflowApproval` always wins - same gate as `allowedByScope`. diff --git a/packages/@n8n/local-gateway/src/main/daemon-controller.test.ts b/packages/@n8n/local-gateway/src/main/daemon-controller.test.ts index 45b26457ca5..62f4fde3828 100644 --- a/packages/@n8n/local-gateway/src/main/daemon-controller.test.ts +++ b/packages/@n8n/local-gateway/src/main/daemon-controller.test.ts @@ -66,7 +66,7 @@ const BASE_CONFIG: GatewayConfig = { logLevel: 'info', allowedOrigins: ['https://*.app.n8n.cloud'], filesystem: { dir: '/' }, - computer: { shell: { timeout: 30_000 } }, + computer: { shell: { timeout: 30_000, dangerouslyDisableSandbox: false } }, browser: { defaultBrowser: 'chrome' }, permissions: {}, permissionConfirmation: 'instance', diff --git a/packages/@n8n/local-gateway/src/main/settings-store.ts b/packages/@n8n/local-gateway/src/main/settings-store.ts index 6b3991d425b..5a36d1b0657 100644 --- a/packages/@n8n/local-gateway/src/main/settings-store.ts +++ b/packages/@n8n/local-gateway/src/main/settings-store.ts @@ -64,7 +64,7 @@ export class SettingsStore { logLevel: s.logLevel, allowedOrigins: origins, filesystem: { dir: s.filesystemDir }, - computer: { shell: { timeout: 30_000 } }, + computer: { shell: { timeout: 30_000, dangerouslyDisableSandbox: false } }, browser: { defaultBrowser: 'chrome', }, diff --git a/packages/@n8n/nodes-langchain/nodes/llms/LMChatAnthropic/LmChatAnthropic.node.ts b/packages/@n8n/nodes-langchain/nodes/llms/LMChatAnthropic/LmChatAnthropic.node.ts index 1261247f9d9..79822845f52 100644 --- a/packages/@n8n/nodes-langchain/nodes/llms/LMChatAnthropic/LmChatAnthropic.node.ts +++ b/packages/@n8n/nodes-langchain/nodes/llms/LMChatAnthropic/LmChatAnthropic.node.ts @@ -17,6 +17,8 @@ import { type SupplyData, } from 'n8n-workflow'; +import { getCustomCredentialHeader } from '@utils/helpers'; + import { searchModels } from './methods/searchModels'; const ANTHROPIC_MODEL_BUILDER_HINT = { @@ -583,14 +585,10 @@ export class LmChatAnthropic implements INodeType { }, }; - if ( - credentials.header && - typeof credentials.headerName === 'string' && - credentials.headerName && - typeof credentials.headerValue === 'string' - ) { + const customHeader = getCustomCredentialHeader(credentials); + if (customHeader) { clientOptions.defaultHeaders = { - [credentials.headerName]: credentials.headerValue, + [customHeader.name]: customHeader.value, }; } @@ -637,7 +635,12 @@ export class LmChatAnthropic implements INodeType { model: modelName, anthropicApiUrl: baseURL, maxTokens: options.maxTokensToSample, - callbacks: [new N8nLlmTracing(this, { tokensUsageParser })], + callbacks: [ + new N8nLlmTracing(this, { + tokensUsageParser, + redactedHeaders: customHeader ? [customHeader.name] : [], + }), + ], onFailedAttempt: makeN8nLlmFailedAttemptHandler(this, failedAttemptHandler), invocationKwargs, clientOptions, diff --git a/packages/@n8n/nodes-langchain/nodes/llms/LMChatAnthropic/test/LmChatAnthropic.test.ts b/packages/@n8n/nodes-langchain/nodes/llms/LMChatAnthropic/test/LmChatAnthropic.test.ts index 092ba1f3b5f..cd4b1437f91 100644 --- a/packages/@n8n/nodes-langchain/nodes/llms/LMChatAnthropic/test/LmChatAnthropic.test.ts +++ b/packages/@n8n/nodes-langchain/nodes/llms/LMChatAnthropic/test/LmChatAnthropic.test.ts @@ -466,6 +466,31 @@ describe('LmChatAnthropic', () => { ); }); + it('should pass the declared header name to N8nLlmTracing', async () => { + const mockContext = setupMockContext(); + mockContext.getCredentials.mockResolvedValue({ + apiKey: 'test-api-key', + header: true, + headerName: 'x-custom-header', + headerValue: 'secret-value', + }); + + mockContext.getNodeParameter = vi.fn().mockImplementation((paramName: string) => { + if (paramName === 'model.value') return 'claude-sonnet-4-20250514'; + if (paramName === 'options') return {}; + return undefined; + }); + + await lmChatAnthropic.supplyData.call(mockContext, 0); + + expect(MockedN8nLlmTracing).toHaveBeenCalledWith( + mockContext, + expect.objectContaining({ + redactedHeaders: ['x-custom-header'], + }), + ); + }); + it('should create failed attempt handler for direct API', async () => { const mockContext = setupMockContext(); diff --git a/packages/@n8n/nodes-langchain/nodes/llms/LMChatOpenAi/LmChatOpenAi.node.ts b/packages/@n8n/nodes-langchain/nodes/llms/LMChatOpenAi/LmChatOpenAi.node.ts index 23cc3dc0442..3fc5514d742 100644 --- a/packages/@n8n/nodes-langchain/nodes/llms/LMChatOpenAi/LmChatOpenAi.node.ts +++ b/packages/@n8n/nodes-langchain/nodes/llms/LMChatOpenAi/LmChatOpenAi.node.ts @@ -11,7 +11,7 @@ import { type SupplyData, } from 'n8n-workflow'; -import { mergeCustomHeaders } from '@utils/helpers'; +import { getCustomCredentialHeader, mergeCustomHeaders } from '@utils/helpers'; import { openAiFailedAttemptHandler } from '../../vendors/OpenAi/helpers/error-handling'; import { @@ -778,6 +778,7 @@ export class LmChatOpenAi implements INodeType { bodyTimeout: timeout, }), }; + const customHeader = getCustomCredentialHeader(credentials); configuration.defaultHeaders = mergeCustomHeaders( credentials, (configuration.defaultHeaders ?? {}) as Record, @@ -811,7 +812,9 @@ export class LmChatOpenAi implements INodeType { timeout, maxRetries: options.maxRetries ?? 2, configuration, - callbacks: [new N8nLlmTracing(this)], + callbacks: [ + new N8nLlmTracing(this, { redactedHeaders: customHeader ? [customHeader.name] : [] }), + ], modelKwargs, onFailedAttempt: makeN8nLlmFailedAttemptHandler(this, openAiFailedAttemptHandler), // Set to false to ensure compatibility with OpenAI-compatible backends (LM Studio, vLLM, etc.) diff --git a/packages/@n8n/nodes-langchain/nodes/llms/LMOpenAi/LmOpenAi.node.ts b/packages/@n8n/nodes-langchain/nodes/llms/LMOpenAi/LmOpenAi.node.ts index 2daf834e899..b0b7c91e1fc 100644 --- a/packages/@n8n/nodes-langchain/nodes/llms/LMOpenAi/LmOpenAi.node.ts +++ b/packages/@n8n/nodes-langchain/nodes/llms/LMOpenAi/LmOpenAi.node.ts @@ -11,7 +11,7 @@ import type { import { Container } from '@n8n/di'; import { AiConfig } from '@n8n/config'; -import { mergeCustomHeaders } from '@utils/helpers'; +import { getCustomCredentialHeader, mergeCustomHeaders } from '@utils/helpers'; type LmOpenAiOptions = { baseURL?: string; @@ -250,6 +250,7 @@ export class LmOpenAi implements INodeType { }; const { openAiDefaultHeaders } = Container.get(AiConfig); + const customHeader = getCustomCredentialHeader(credentials); const defaultHeaders = mergeCustomHeaders(credentials, openAiDefaultHeaders ?? {}); const timeout = options.timeout; const configuration: ClientOptions = { @@ -273,7 +274,9 @@ export class LmOpenAi implements INodeType { configuration, timeout, maxRetries: options.maxRetries ?? 2, - callbacks: [new N8nLlmTracing(this)], + callbacks: [ + new N8nLlmTracing(this, { redactedHeaders: customHeader ? [customHeader.name] : [] }), + ], onFailedAttempt: makeN8nLlmFailedAttemptHandler(this), }); diff --git a/packages/@n8n/nodes-langchain/nodes/llms/N8nNonEstimatingTracing.ts b/packages/@n8n/nodes-langchain/nodes/llms/N8nNonEstimatingTracing.ts index 90c872bfcbb..61c541618af 100644 --- a/packages/@n8n/nodes-langchain/nodes/llms/N8nNonEstimatingTracing.ts +++ b/packages/@n8n/nodes-langchain/nodes/llms/N8nNonEstimatingTracing.ts @@ -7,12 +7,11 @@ import type { } from '@langchain/core/load/serializable'; import type { BaseMessage } from '@langchain/core/messages'; import type { LLMResult } from '@langchain/core/outputs'; +import { logAiEvent } from '@n8n/ai-utilities'; import pick from 'lodash/pick'; import type { IDataObject, ISupplyDataFunctions, JsonObject } from 'n8n-workflow'; import { NodeConnectionTypes, NodeError, NodeOperationError } from 'n8n-workflow'; -import { logAiEvent } from '@n8n/ai-utilities'; - type RunDetail = { index: number; messages: BaseMessage[] | string[] | string; diff --git a/packages/@n8n/nodes-langchain/nodes/llms/test/LmChatAnthropic.test.ts b/packages/@n8n/nodes-langchain/nodes/llms/test/LmChatAnthropic.test.ts index 07a9eedf8af..cc41e73151c 100644 --- a/packages/@n8n/nodes-langchain/nodes/llms/test/LmChatAnthropic.test.ts +++ b/packages/@n8n/nodes-langchain/nodes/llms/test/LmChatAnthropic.test.ts @@ -319,6 +319,7 @@ describe('LmChatAnthropic', () => { expect(MockedN8nLlmTracing).toHaveBeenCalledWith(mockContext, { tokensUsageParser: expect.any(Function), + redactedHeaders: [], }); }); diff --git a/packages/@n8n/nodes-langchain/nodes/llms/test/LmChatOpenAi.test.ts b/packages/@n8n/nodes-langchain/nodes/llms/test/LmChatOpenAi.test.ts index aa381ff09e4..854c68cfc72 100644 --- a/packages/@n8n/nodes-langchain/nodes/llms/test/LmChatOpenAi.test.ts +++ b/packages/@n8n/nodes-langchain/nodes/llms/test/LmChatOpenAi.test.ts @@ -356,7 +356,29 @@ describe('LmChatOpenAi', () => { await lmChatOpenAi.supplyData.call(mockContext, 0); - expect(MockedN8nLlmTracing).toHaveBeenCalledWith(mockContext); + expect(MockedN8nLlmTracing).toHaveBeenCalledWith(mockContext, { redactedHeaders: [] }); + }); + + it('should pass the declared header name to N8nLlmTracing', async () => { + const mockContext = setupMockContext(); + mockContext.getCredentials = vi.fn().mockResolvedValue({ + apiKey: 'test-api-key', + header: true, + headerName: 'x-custom-header', + headerValue: 'secret-value', + }); + + mockContext.getNodeParameter = vi.fn().mockImplementation((paramName: string) => { + if (paramName === 'model.value') return 'gpt-4o-mini'; + if (paramName === 'options') return {}; + return undefined; + }); + + await lmChatOpenAi.supplyData.call(mockContext, 0); + + expect(MockedN8nLlmTracing).toHaveBeenCalledWith(mockContext, { + redactedHeaders: ['x-custom-header'], + }); }); it('should create failed attempt handler', async () => { diff --git a/packages/@n8n/nodes-langchain/nodes/llms/test/LmOpenAi.test.ts b/packages/@n8n/nodes-langchain/nodes/llms/test/LmOpenAi.test.ts new file mode 100644 index 00000000000..936daf650b6 --- /dev/null +++ b/packages/@n8n/nodes-langchain/nodes/llms/test/LmOpenAi.test.ts @@ -0,0 +1,95 @@ +/* eslint-disable n8n-nodes-base/node-filename-against-convention */ +import { OpenAI } from '@langchain/openai'; +import { makeN8nLlmFailedAttemptHandler, N8nLlmTracing, getProxyAgent } from '@n8n/ai-utilities'; +import { createMockExecuteFunction } from 'n8n-nodes-base/test/nodes/Helpers'; +import type { INode, ISupplyDataFunctions } from 'n8n-workflow'; +import type { Mocked } from 'vitest'; + +import { LmOpenAi } from '../LMOpenAi/LmOpenAi.node'; + +vi.mock('@langchain/openai'); +vi.mock('@n8n/ai-utilities'); + +const MockedN8nLlmTracing = vi.mocked(N8nLlmTracing); +const mockedMakeN8nLlmFailedAttemptHandler = vi.mocked(makeN8nLlmFailedAttemptHandler); +const mockedGetProxyAgent = vi.mocked(getProxyAgent); + +describe('LmOpenAi', () => { + let lmOpenAi: LmOpenAi; + let mockContext: Mocked; + + const mockNode: INode = { + id: '1', + name: 'OpenAI Model', + typeVersion: 1, + type: 'n8n-nodes-langchain.lmOpenAi', + position: [0, 0], + parameters: {}, + }; + + const setupMockContext = (credentials: Record = { apiKey: 'test-api-key' }) => { + mockContext = createMockExecuteFunction( + {}, + mockNode, + ) as Mocked; + + mockContext.getCredentials = vi.fn().mockResolvedValue(credentials); + mockContext.getNode = vi.fn().mockReturnValue(mockNode); + mockContext.getNodeParameter = vi.fn().mockImplementation((paramName: string) => { + if (paramName === 'model') return 'gpt-3.5-turbo-instruct'; + if (paramName === 'options') return {}; + return undefined; + }); + + mockedMakeN8nLlmFailedAttemptHandler.mockReturnValue(vi.fn()); + mockedGetProxyAgent.mockReturnValue({} as never); + return mockContext; + }; + + beforeEach(() => { + lmOpenAi = new LmOpenAi(); + vi.clearAllMocks(); + }); + + afterEach(() => { + vi.clearAllMocks(); + }); + + describe('supplyData', () => { + it('should construct the model with an N8nLlmTracing callback', async () => { + const mockContext = setupMockContext(); + + await lmOpenAi.supplyData.call(mockContext, 0); + + expect(OpenAI).toHaveBeenCalledWith( + expect.objectContaining({ + model: 'gpt-3.5-turbo-instruct', + callbacks: expect.arrayContaining([expect.any(Object)]), + }), + ); + }); + + it('should pass empty redactedHeaders to N8nLlmTracing when no custom header is configured', async () => { + const mockContext = setupMockContext(); + + await lmOpenAi.supplyData.call(mockContext, 0); + + expect(MockedN8nLlmTracing).toHaveBeenCalledWith(mockContext, { redactedHeaders: [] }); + }); + + it('should pass the declared header name to N8nLlmTracing', async () => { + const mockContext = setupMockContext({ + apiKey: 'test-api-key', + header: true, + headerName: 'x-custom-header', + headerValue: 'secret-value', + }); + + await lmOpenAi.supplyData.call(mockContext, 0); + + expect(MockedN8nLlmTracing).toHaveBeenCalledWith(mockContext, { + redactedHeaders: ['x-custom-header'], + }); + }); + }); +}); diff --git a/packages/@n8n/nodes-langchain/nodes/mcp/McpTrigger/McpTrigger.node.ts b/packages/@n8n/nodes-langchain/nodes/mcp/McpTrigger/McpTrigger.node.ts index df7c88369c2..f63cfb6c8d7 100644 --- a/packages/@n8n/nodes-langchain/nodes/mcp/McpTrigger/McpTrigger.node.ts +++ b/packages/@n8n/nodes-langchain/nodes/mcp/McpTrigger/McpTrigger.node.ts @@ -113,6 +113,15 @@ export class McpTrigger extends Node { "Default to 'none'. n8n exposes inbound trigger URLs publicly by design. Only select an authentication method when the user explicitly asks to authenticate inbound traffic.", }, }, + { + displayName: 'Require Workflow Execute Permission', + name: 'requireExecuteAccess', + type: 'boolean', + default: true, + displayOptions: { show: { authentication: ['n8nOAuth2'] } }, // n8nOAuth2 is v2+ only + description: + 'Whether the triggering user must also have permission to execute the workflow in the project it belongs to', + }, { displayName: 'Path', name: 'path', diff --git a/packages/@n8n/nodes-langchain/nodes/mcp/McpTrigger/__tests__/n8n-oauth2-auth.test.ts b/packages/@n8n/nodes-langchain/nodes/mcp/McpTrigger/__tests__/n8n-oauth2-auth.test.ts new file mode 100644 index 00000000000..fb8f51e9095 --- /dev/null +++ b/packages/@n8n/nodes-langchain/nodes/mcp/McpTrigger/__tests__/n8n-oauth2-auth.test.ts @@ -0,0 +1,36 @@ +import type { IWebhookFunctions, N8nOAuth2ValidationResult } from 'n8n-workflow'; + +import { createMockRequest, createMockResponse } from './helpers'; +import { n8nOAuth2Auth } from '../n8n-oauth2-auth'; + +const buildContext = (validation: N8nOAuth2ValidationResult) => { + const response = createMockResponse(); + const request = createMockRequest({ headers: { authorization: 'Bearer test-token' } }); + + const context = { + getNodeWebhookUrl: () => 'https://n8n.example.com/mcp/protected-path', + getResponseObject: () => response, + getRequestObject: () => request, + validateN8nOAuth2Token: vi.fn().mockResolvedValue(validation), + } as unknown as IWebhookFunctions; + + return { context, response }; +}; + +describe('n8nOAuth2Auth', () => { + // An authenticated user lacking execute access (`insufficient_scope`) is rejected with 403; + // an unverifiable token (`invalid_token`) with 401. Both carry the reason in WWW-Authenticate. + test.each([ + ['insufficient_scope', 403], + ['invalid_token', 401], + ] as const)('maps a %s validation failure to a %i response', async (reason, code) => { + const { context, response } = buildContext({ valid: false, reason }); + + const result = await n8nOAuth2Auth(context); + + expect(result).toBe('handled'); + expect(response.writeHead).toHaveBeenCalledWith(code, { + 'WWW-Authenticate': expect.stringContaining(`error="${reason}"`), + }); + }); +}); diff --git a/packages/@n8n/nodes-langchain/nodes/mcp/McpTrigger/n8n-oauth2-auth.ts b/packages/@n8n/nodes-langchain/nodes/mcp/McpTrigger/n8n-oauth2-auth.ts index 2105f5262f2..099b1c4a6b5 100644 --- a/packages/@n8n/nodes-langchain/nodes/mcp/McpTrigger/n8n-oauth2-auth.ts +++ b/packages/@n8n/nodes-langchain/nodes/mcp/McpTrigger/n8n-oauth2-auth.ts @@ -15,13 +15,14 @@ function getBearerToken(headerValue: string | undefined): string | null { function sendUnauthorizedResponse( resp: ReturnType, + code: number, prmUrl: string, error?: string, ) { const authenticateHeader = `Bearer realm="n8n MCP Server", resource_metadata="${prmUrl}"` + (error ? `, error="${error}"` : ''); - resp.writeHead(401, { + resp.writeHead(code, { 'WWW-Authenticate': authenticateHeader, }); resp.end(); @@ -54,14 +55,16 @@ export const n8nOAuth2Auth = async ( const authHeader = req.headers['authorization']; const token = getBearerToken(authHeader); if (!token) { - sendUnauthorizedResponse(resp, prmUrl); + sendUnauthorizedResponse(resp, 401, prmUrl); return 'handled'; } const validationResult = await context.validateN8nOAuth2Token(token, resourceUrl); if (!validationResult.valid) { if (validationResult.reason === 'invalid_token') { - sendUnauthorizedResponse(resp, prmUrl, 'invalid_token'); + sendUnauthorizedResponse(resp, 401, prmUrl, 'invalid_token'); + } else if (validationResult.reason === 'insufficient_scope') { + sendUnauthorizedResponse(resp, 403, prmUrl, 'insufficient_scope'); } else { resp.status(503).send('OAuth token validation is not available'); } diff --git a/packages/@n8n/nodes-langchain/utils/helpers.ts b/packages/@n8n/nodes-langchain/utils/helpers.ts index 2d37c536ce5..a582314bf77 100644 --- a/packages/@n8n/nodes-langchain/utils/helpers.ts +++ b/packages/@n8n/nodes-langchain/utils/helpers.ts @@ -267,6 +267,24 @@ export const getConnectedTools = async ( return finalTools; }; +/** + * Reads the custom header configured on a credential, if present and valid. + * Shared by header merging and tracing redaction so the guard lives in one place. + */ +export function getCustomCredentialHeader( + credentials: ICredentialDataDecryptedObject, +): { name: string; value: string } | undefined { + if ( + credentials.header && + typeof credentials.headerName === 'string' && + credentials.headerName && + typeof credentials.headerValue === 'string' + ) { + return { name: credentials.headerName, value: credentials.headerValue }; + } + return undefined; +} + /** * Merges custom credential headers into an existing defaultHeaders object. * Used by OpenAI and other LangChain nodes that pass `configuration.defaultHeaders`. @@ -275,15 +293,11 @@ export function mergeCustomHeaders( credentials: ICredentialDataDecryptedObject, defaultHeaders: Record, ): Record { - if ( - credentials.header && - typeof credentials.headerName === 'string' && - credentials.headerName && - typeof credentials.headerValue === 'string' - ) { + const customHeader = getCustomCredentialHeader(credentials); + if (customHeader) { return { ...defaultHeaders, - [credentials.headerName]: credentials.headerValue, + [customHeader.name]: customHeader.value, }; } return defaultHeaders; diff --git a/packages/@n8n/utils/src/scrub-secrets.test.ts b/packages/@n8n/utils/src/scrub-secrets.test.ts index 6ef148fe4f5..8e551538c77 100644 --- a/packages/@n8n/utils/src/scrub-secrets.test.ts +++ b/packages/@n8n/utils/src/scrub-secrets.test.ts @@ -123,6 +123,20 @@ describe('scrubSecretsInText', () => { expect(scrubSecretsInText(ownOutput)).toBe(ownOutput); }); + it('matches a quoted value with many escapes in linear time', () => { + // A quoted secret value made of many backslashes with no closing quote. + // The value body must have a single, unambiguous parse so matching stays + // fast regardless of the input shape. + const input = '{"password": "' + '\\'.repeat(200); + const start = performance.now(); + const out = scrubSecretsInText(input); + const elapsedMs = performance.now() - start; + expect(elapsedMs).toBeLessThan(50); + // No closing quote, so the JSON-shaped pattern can't match; the value is + // left as-is and matching returns promptly. + expect(out).toContain('password'); + }); + it('leaves typed redaction markers untouched instead of nesting them', () => { expect(scrubSecretsInText('[REDACTED:secret:1]')).toBe('[REDACTED:secret:1]'); expect(scrubSecretsInText('[REDACTED:password:2]')).toBe('[REDACTED:password:2]'); diff --git a/packages/@n8n/utils/src/scrub-secrets.ts b/packages/@n8n/utils/src/scrub-secrets.ts index 5e762dd55c5..560ef551eb4 100644 --- a/packages/@n8n/utils/src/scrub-secrets.ts +++ b/packages/@n8n/utils/src/scrub-secrets.ts @@ -45,20 +45,24 @@ export const SECRET_VALUE_PATTERNS: readonly RegExp[] = [ // Run before the loose pattern so nested objects like // `{"credentials": {"apiKey": "..."}}` don't have the outer key consume // the inner key on its way to a non-quoted (object) value. The value - // body uses the standard JSON-string idiom `(?:\\.|[^"\r\n])*` so an - // escaped quote inside the value (`"abc\"def"`) doesn't end the match - // early and leak the rest of the secret. The negative lookahead skips + // body uses the unrolled JSON-string idiom `(?:[^"\\\r\n]|\\.)*`: the + // negated class excludes the backslash so a backslash can only be consumed + // by the `\\.` escape branch. Keep the two alternatives disjoint (don't + // fold `\\` back into the negated class) — that keeps every run of + // backslashes to a single, unambiguous parse, so matching stays fast on any + // input. An escaped quote inside the value (`"abc\"def"`) still doesn't end + // the match early, via the escape branch. The negative lookahead skips // values that are already a `[redacted]` / `[REDACTED]` / typed // `[REDACTED::]` placeholder so this stays idempotent when // chained behind upstream object-walking redaction (langsmith trace // payloads, mcp-browser markers). new RegExp( - `"(?:${SECRET_KEYS})"\\s*:\\s*"(?!\\[(?:redacted|REDACTED)(?::[^"\\]]*)?\\]")(?:\\\\.|[^"\\r\\n])*"`, + `"(?:${SECRET_KEYS})"\\s*:\\s*"(?!\\[(?:redacted|REDACTED)(?::[^"\\]]*)?\\]")(?:[^"\\\\\\r\\n]|\\\\.)*"`, 'gi', ), // JS-object-shaped `'key': 'value'` new RegExp( - `'(?:${SECRET_KEYS})'\\s*:\\s*'(?!\\[(?:redacted|REDACTED)(?::[^'\\]]*)?\\]')(?:\\\\.|[^'\\r\\n])*'`, + `'(?:${SECRET_KEYS})'\\s*:\\s*'(?!\\[(?:redacted|REDACTED)(?::[^'\\]]*)?\\]')(?:[^'\\\\\\r\\n]|\\\\.)*'`, 'gi', ), // Generic `password=...` / `api_key=...` / `secret=...` style assignments. diff --git a/packages/cli/src/controllers/__tests__/dynamic-node-parameters.controller.test.ts b/packages/cli/src/controllers/__tests__/dynamic-node-parameters.controller.test.ts index c30bb01cd36..6a0da55bdd4 100644 --- a/packages/cli/src/controllers/__tests__/dynamic-node-parameters.controller.test.ts +++ b/packages/cli/src/controllers/__tests__/dynamic-node-parameters.controller.test.ts @@ -64,7 +64,7 @@ describe('DynamicNodeParametersController', () => { expect(result).toEqual(expectedResult); }); - it('should call getOptionsViaLoadOptions when loadOptions is provided', async () => { + it('should call getOptionsViaLoadOptionsByPath when loadOptions is provided', async () => { const loadOptions: ILoadOptions = { routing: { operations: {}, @@ -77,12 +77,12 @@ describe('DynamicNodeParametersController', () => { const req = { user: mockUser } as AuthenticatedRequest; const expectedResult: INodePropertyOptions[] = [{ name: 'test', value: 'value' }]; - service.getOptionsViaLoadOptions.mockResolvedValue(expectedResult); + service.getOptionsViaLoadOptionsByPath.mockResolvedValue(expectedResult); const result = await controller.getOptions(req, mock(), payload); - expect(service.getOptionsViaLoadOptions).toHaveBeenCalledWith( - loadOptions, + expect(service.getOptionsViaLoadOptionsByPath).toHaveBeenCalledWith( + '/test/path', baseAdditionalData, { name: 'TestNode', version: 1 }, {}, diff --git a/packages/cli/src/controllers/dynamic-node-parameters.controller.ts b/packages/cli/src/controllers/dynamic-node-parameters.controller.ts index 1fec22eff89..caed9d2c210 100644 --- a/packages/cli/src/controllers/dynamic-node-parameters.controller.ts +++ b/packages/cli/src/controllers/dynamic-node-parameters.controller.ts @@ -52,8 +52,8 @@ export class DynamicNodeParametersController { } if (loadOptions) { - return await this.dynamicNodeParametersService.getOptionsViaLoadOptions( - loadOptions, + return await this.dynamicNodeParametersService.getOptionsViaLoadOptionsByPath( + path, additionalData, nodeTypeAndVersion, currentNodeParameters, diff --git a/packages/cli/src/modules/agents/__tests__/agent-execution-orchestrator.service.test.ts b/packages/cli/src/modules/agents/__tests__/agent-execution-orchestrator.service.test.ts index d9d613e4173..42716584666 100644 --- a/packages/cli/src/modules/agents/__tests__/agent-execution-orchestrator.service.test.ts +++ b/packages/cli/src/modules/agents/__tests__/agent-execution-orchestrator.service.test.ts @@ -1,6 +1,7 @@ import type { Agent as RuntimeAgent, StreamChunk } from '@n8n/agents'; import { N8N_CHAT_INTEGRATION_TYPE, type AgentJsonConfig } from '@n8n/api-types'; import { mockLogger } from '@n8n/backend-test-utils'; +import type { User } from '@n8n/db'; import type { JSONSchema7 } from 'json-schema'; import { OperationalError, UserError } from 'n8n-workflow'; import type { ExecuteAgentWorkflowContext, IRunExecutionData } from 'n8n-workflow'; @@ -23,6 +24,7 @@ import type { ToolRegistry } from '../tool-registry'; const agentId = 'agent-1'; const projectId = 'project-1'; const userId = 'user-1'; +const user = mock({ id: userId }); const schema: AgentJsonConfig = { name: 'Support Agent', @@ -196,7 +198,7 @@ describe('AgentExecutionOrchestratorService', () => { agentId, projectId, message: 'hello', - userId, + user, memory: { threadId: 'thread-1', resourceId: 'resource-1' }, }), ); @@ -205,6 +207,7 @@ describe('AgentExecutionOrchestratorService', () => { agentId, projectId, integrationType: N8N_CHAT_INTEGRATION_TYPE, + user, }); expect(integrationMessageContextService.setLatest).toHaveBeenCalledWith( 'thread-1', diff --git a/packages/cli/src/modules/agents/__tests__/agent-publish.service.test.ts b/packages/cli/src/modules/agents/__tests__/agent-publish.service.test.ts index ed3adebab11..086c81bf1f1 100644 --- a/packages/cli/src/modules/agents/__tests__/agent-publish.service.test.ts +++ b/packages/cli/src/modules/agents/__tests__/agent-publish.service.test.ts @@ -15,6 +15,7 @@ import { ChatIntegrationService } from '../integrations/chat-integration.service import type { AgentHistoryRepository } from '../repositories/agent-history.repository'; import type { AgentTaskSnapshotRepository } from '../repositories/agent-task-snapshot.repository'; import type { AgentRepository } from '../repositories/agent.repository'; +import type { SubAgentCleanupService } from '../sub-agents/sub-agent-cleanup.service'; const agentId = 'agent-1'; const projectId = 'project-1'; @@ -95,6 +96,7 @@ function makeService() { const runtimeCacheService = mock(); const chatIntegrationService = mock(); const taskService = mock(); + const subAgentCleanupService = mock(); const { trx, taskRepo, transaction } = makeTransaction(); Object.defineProperty(agentRepository, 'manager', { @@ -108,6 +110,7 @@ function makeService() { chatIntegrationService.disconnect.mockResolvedValue(); chatIntegrationService.disconnectChannel.mockResolvedValue(); taskService.requestReconcile.mockResolvedValue(); + subAgentCleanupService.removeSubAgentFromParents.mockResolvedValue(); Container.set(ChatIntegrationService, chatIntegrationService); Container.set(AgentTaskService, taskService); @@ -118,6 +121,7 @@ function makeService() { taskSnapshotRepository, customToolsService, runtimeCacheService, + subAgentCleanupService, ); return { @@ -129,6 +133,7 @@ function makeService() { runtimeCacheService, chatIntegrationService, taskService, + subAgentCleanupService, trx, taskRepo, }; @@ -234,8 +239,13 @@ describe('AgentPublishService', () => { }); it('avoids duplicate history inserts but creates a fresh version after unpublish', async () => { - const { service, agentRepository, agentHistoryRepository, chatIntegrationService } = - makeService(); + const { + service, + agentRepository, + agentHistoryRepository, + chatIntegrationService, + subAgentCleanupService, + } = makeService(); const agent = makeAgent({ versionId: 'v1', activeVersionId: 'v1', @@ -250,6 +260,10 @@ describe('AgentPublishService', () => { await service.unpublishAgent(agentId, projectId); expect(agent.activeVersionId).toBeNull(); expect(agent.versionId).not.toBe('v1'); + expect(subAgentCleanupService.removeSubAgentFromParents).toHaveBeenCalledWith( + agentId, + projectId, + ); expect(chatIntegrationService.disconnectChannel).toHaveBeenCalledWith( agentId, { diff --git a/packages/cli/src/modules/agents/__tests__/agent-runtime-cache.service.test.ts b/packages/cli/src/modules/agents/__tests__/agent-runtime-cache.service.test.ts index c1628f72e03..28b840962e2 100644 --- a/packages/cli/src/modules/agents/__tests__/agent-runtime-cache.service.test.ts +++ b/packages/cli/src/modules/agents/__tests__/agent-runtime-cache.service.test.ts @@ -2,6 +2,7 @@ import type { Mock } from 'vitest'; import type { Agent as RuntimeAgent } from '@n8n/agents'; import { mockLogger } from '@n8n/backend-test-utils'; import type { GlobalConfig } from '@n8n/config'; +import type { User } from '@n8n/db'; import { mock } from 'vitest-mock-extended'; import { OperationalError } from 'n8n-workflow'; @@ -89,6 +90,7 @@ describe('AgentRuntimeCacheService', () => { agent, expect.anything(), undefined, + undefined, ); }); @@ -118,6 +120,44 @@ describe('AgentRuntimeCacheService', () => { agent, expect.anything(), 'n8n_chat', + undefined, + ); + }); + + it('keys draft runtimes by user id so different users get separate runtimes, reused per user', async () => { + const { service, agentRepository, reconstructionService } = makeService(); + const agent = makeAgent(); + const userARuntime = makeRuntime(); + const userBRuntime = makeRuntime(); + const userA = mock({ id: 'user-a' }); + const userB = mock({ id: 'user-b' }); + + agentRepository.findByIdAndProjectId.mockResolvedValue(agent); + reconstructionService.reconstructFromAgentEntity + .mockResolvedValueOnce(userARuntime) + .mockResolvedValueOnce(userBRuntime); + + const forUserA = await service.getRuntime({ agentId, projectId, user: userA }); + const forUserAAgain = await service.getRuntime({ agentId, projectId, user: userA }); + const forUserB = await service.getRuntime({ agentId, projectId, user: userB }); + + expect(forUserA.agent).toBe(userARuntime.agent); + expect(forUserAAgain.agent).toBe(userARuntime.agent); + expect(forUserB.agent).toBe(userBRuntime.agent); + expect(reconstructionService.reconstructFromAgentEntity).toHaveBeenCalledTimes(2); + expect(reconstructionService.reconstructFromAgentEntity).toHaveBeenNthCalledWith( + 1, + agent, + expect.anything(), + undefined, + userA, + ); + expect(reconstructionService.reconstructFromAgentEntity).toHaveBeenNthCalledWith( + 2, + agent, + expect.anything(), + undefined, + userB, ); }); @@ -240,6 +280,7 @@ describe('AgentRuntimeCacheService', () => { }), expect.anything(), 'slack', + undefined, ); }); diff --git a/packages/cli/src/modules/agents/__tests__/agent-runtime-reconstruction-integration-tools.test.ts b/packages/cli/src/modules/agents/__tests__/agent-runtime-reconstruction-integration-tools.test.ts index b572664612d..4c33d309ac5 100644 --- a/packages/cli/src/modules/agents/__tests__/agent-runtime-reconstruction-integration-tools.test.ts +++ b/packages/cli/src/modules/agents/__tests__/agent-runtime-reconstruction-integration-tools.test.ts @@ -19,6 +19,7 @@ import { Container } from '@n8n/di'; import { mock } from 'vitest-mock-extended'; import type { ActiveExecutions } from '@/active-executions'; +import type { CredentialsFinderService } from '@/credentials/credentials-finder.service'; import { CredentialsService } from '@/credentials/credentials.service'; import type { EphemeralNodeExecutor } from '@/node-execution'; import type { OauthService } from '@/oauth/oauth.service'; @@ -26,6 +27,7 @@ import type { Publisher } from '@/scaling/pubsub/publisher.service'; import type { AiService } from '@/services/ai.service'; import type { UrlService } from '@/services/url.service'; import type { Telemetry } from '@/telemetry'; +import type { WorkflowFinderService } from '@/workflows/workflow-finder.service'; import { AgentConfigService } from '../agent-config.service'; import { AgentCustomToolsService } from '../agent-custom-tools.service'; @@ -60,6 +62,7 @@ import type { AgentTaskRepository } from '../repositories/agent-task.repository' import type { AgentRepository } from '../repositories/agent.repository'; import type { AgentSecureRuntime } from '../runtime/agent-secure-runtime'; import { SubAgentForegroundRunner } from '../sub-agents/sub-agent-foreground-runner'; +import type { SubAgentCleanupService } from '../sub-agents/sub-agent-cleanup.service'; const agentId = 'agent-1'; const projectId = 'project-1'; @@ -123,6 +126,8 @@ function makeRuntimeReconstructionService( mock(), mock({ enabled: true }), mock(), + mock(), + mock(), ); } @@ -269,6 +274,7 @@ describe('AgentRuntimeReconstructionService integration tools', () => { agentTaskSnapshotRepository, agentCustomToolsService, runtimeCacheService, + mock(), ); agentTestChatService = new AgentTestChatService(n8nMemory); agentValidationService = new AgentValidationService(agentRepository, mock()); @@ -279,6 +285,7 @@ describe('AgentRuntimeReconstructionService integration tools', () => { agentKnowledgeService, runtimeCacheService, agentTestChatService, + mock(), ); service = agentExecutionOrchestratorService; markSharedTestSetupAsUsed( diff --git a/packages/cli/src/modules/agents/__tests__/agent-runtime-reconstruction-tool-filtering.test.ts b/packages/cli/src/modules/agents/__tests__/agent-runtime-reconstruction-tool-filtering.test.ts new file mode 100644 index 00000000000..af6a44e5bd9 --- /dev/null +++ b/packages/cli/src/modules/agents/__tests__/agent-runtime-reconstruction-tool-filtering.test.ts @@ -0,0 +1,318 @@ +import type * as agents from '@n8n/agents'; +import type { CredentialProvider } from '@n8n/agents'; +import type { AgentJsonConfig, AgentJsonToolConfig } from '@n8n/api-types'; +import type { Logger } from '@n8n/backend-common'; +import type { + CustomFetch, + HttpTransport, + OutboundHttp, + SsrfProtectionService, +} from '@n8n/backend-network'; +import type { AgentsConfig, SsrfProtectionConfig } from '@n8n/config'; +import type { CredentialsEntity, User, WorkflowEntity, WorkflowRepository } from '@n8n/db'; +import { Container } from '@n8n/di'; +import { mock } from 'vitest-mock-extended'; + +import type { ActiveExecutions } from '@/active-executions'; +import type { CredentialsFinderService } from '@/credentials/credentials-finder.service'; +import type { EphemeralNodeExecutor } from '@/node-execution'; +import type { OauthService } from '@/oauth/oauth.service'; +import { userHasScopes } from '@/permissions.ee/check-access'; +import type { AiService } from '@/services/ai.service'; +import type { UrlService } from '@/services/url.service'; +import type { WorkflowFinderService } from '@/workflows/workflow-finder.service'; + +import { AgentRuntimeReconstructionService } from '../agent-runtime-reconstruction.service'; +import type { AgentKnowledgeSandboxService } from '../agent-knowledge-sandbox.service'; +import type { Agent } from '../entities/agent.entity'; +import type { N8NCheckpointStorage } from '../integrations/n8n-checkpoint-storage'; +import type { N8nMemory } from '../integrations/n8n-memory'; +import type * as FromJsonConfig from '../json-config/from-json-config'; +import type { ToolExecutor } from '../json-config/from-json-config'; +import type { AgentFileRepository } from '../repositories/agent-file.repository'; +import type { AgentRepository } from '../repositories/agent.repository'; +import type { AgentSecureRuntime } from '../runtime/agent-secure-runtime'; +import { SubAgentForegroundRunner } from '../sub-agents/sub-agent-foreground-runner'; + +vi.mock('@/permissions.ee/check-access', () => ({ + userHasScopes: vi.fn(), +})); + +const projectId = 'project-1'; +const userId = 'user-1'; +const testUser = mock({ id: userId }); + +const nodeToolWithCredential: Extract = { + type: 'node', + name: 'Send Slack message', + node: { + nodeType: 'n8n-nodes-base.slack', + nodeTypeVersion: 1, + nodeParameters: {}, + credentials: { slackApi: { id: 'cred-1', name: 'Prod Slack' } }, + }, +}; + +const nodeToolWithoutCredential: Extract = { + type: 'node', + name: 'Get date', + node: { + nodeType: 'n8n-nodes-base.dateTime', + nodeTypeVersion: 1, + nodeParameters: {}, + }, +}; + +const workflowTool: Extract = { + type: 'workflow', + workflow: 'Lookup customer', +}; + +const customTool: Extract = { + type: 'custom', + id: 'custom_tool', +}; + +// Mock buildFromJson so reconstruction doesn't try to actually build an agent; +// capture the config it receives so tests can assert on which tool refs survived filtering. +const builtAgent = mock(); +builtAgent.hasCheckpointStorage.mockReturnValue(true); +const buildFromJsonMock = vi.fn().mockImplementation(async () => builtAgent); +vi.mock('../json-config/from-json-config', async () => { + const actual = await vi.importActual('../json-config/from-json-config'); + return { + ...actual, + buildFromJson: (...args: unknown[]) => buildFromJsonMock(...args), + }; +}); + +function makeAgentEntity(tools: AgentJsonToolConfig[]): Agent { + const schema: AgentJsonConfig = { + name: 'Test Agent', + model: 'anthropic/claude-sonnet-4-5', + instructions: 'Be helpful', + tools, + }; + return { + id: 'agent-1', + projectId, + schema, + tools: { custom_tool: { descriptor: { name: 'custom_tool' }, code: '' } }, + } as unknown as Agent; +} + +function makeService(overrides: { + credentialsFinderService?: ReturnType>; + workflowFinderService?: ReturnType>; + workflowRepository?: ReturnType>; +}) { + const secureRuntime = mock(); + secureRuntime.createToolExecutor.mockReturnValue(mock()); + const transport = mock(); + transport.asCustomFetch.mockReturnValue(vi.fn() as unknown as CustomFetch); + const outboundHttp = mock(); + outboundHttp.transport.mockReturnValue(transport); + + const credentialsFinderService = + overrides.credentialsFinderService ?? mock(); + const workflowFinderService = overrides.workflowFinderService ?? mock(); + const workflowRepository = overrides.workflowRepository ?? mock(); + + const service = new AgentRuntimeReconstructionService( + mock(), + mock(), + mock(), + mock(), + workflowRepository, + mock(), + mock(), + secureRuntime, + mock(), + mock(), + mock(), + { modules: [] } as unknown as AgentsConfig, + mock(), + outboundHttp, + mock(), + mock({ enabled: true }), + mock(), + credentialsFinderService, + workflowFinderService, + ); + + return { service, credentialsFinderService, workflowFinderService, workflowRepository }; +} + +function toolNamesPassedToBuildFromJson(): string[] { + const [config] = buildFromJsonMock.mock.calls.at(-1) as [AgentJsonConfig]; + return (config.tools ?? []).map((ref) => + ref.type === 'custom' ? ref.id : ref.type === 'workflow' ? ref.workflow : ref.name, + ); +} + +describe('AgentRuntimeReconstructionService — per-user tool filtering', () => { + beforeEach(() => { + vi.clearAllMocks(); + builtAgent.hasCheckpointStorage.mockReturnValue(true); + Container.set(SubAgentForegroundRunner, mock()); + }); + + afterEach(() => { + Container.reset(); + }); + + it('does not filter tools when no user is supplied (published/integration runs)', async () => { + const { service } = makeService({}); + const entity = makeAgentEntity([nodeToolWithCredential, workflowTool, customTool]); + const credentialProvider = mock(); + + await service.reconstructFromAgentEntity(entity, credentialProvider); + + expect(userHasScopes).not.toHaveBeenCalled(); + expect(toolNamesPassedToBuildFromJson()).toEqual( + expect.arrayContaining(['Send Slack message', 'Lookup customer', 'custom_tool']), + ); + }); + + it('drops node and workflow tools for a user without workflow:execute, keeps custom tools', async () => { + vi.mocked(userHasScopes).mockResolvedValue(false); + const { service } = makeService({}); + const entity = makeAgentEntity([nodeToolWithCredential, workflowTool, customTool]); + const credentialProvider = mock(); + + await service.reconstructFromAgentEntity(entity, credentialProvider, undefined, testUser); + + expect(userHasScopes).toHaveBeenCalledWith(testUser, ['workflow:execute'], false, { + projectId, + }); + expect(toolNamesPassedToBuildFromJson()).toEqual(['custom_tool']); + }); + + it('drops a node tool when the user lacks credential:read on a baked credential', async () => { + vi.mocked(userHasScopes).mockResolvedValue(true); + const credentialsFinderService = mock(); + credentialsFinderService.findCredentialForUser.mockResolvedValue(null); + const { service } = makeService({ credentialsFinderService }); + const entity = makeAgentEntity([nodeToolWithCredential, nodeToolWithoutCredential]); + const credentialProvider = mock(); + + await service.reconstructFromAgentEntity(entity, credentialProvider, undefined, testUser); + + expect(credentialsFinderService.findCredentialForUser).toHaveBeenCalledWith( + 'cred-1', + testUser, + ['credential:read'], + ); + expect(toolNamesPassedToBuildFromJson()).toEqual(['Get date']); + }); + + it('drops a workflow tool the user cannot access, keeps one they can', async () => { + vi.mocked(userHasScopes).mockResolvedValue(true); + const workflowRepository = mock(); + workflowRepository.findOne.mockResolvedValue(mock({ id: 'wf-1' })); + const workflowFinderService = mock(); + workflowFinderService.findWorkflowForUser.mockResolvedValue(null); + const { service } = makeService({ workflowRepository, workflowFinderService }); + const entity = makeAgentEntity([workflowTool]); + const credentialProvider = mock(); + + await service.reconstructFromAgentEntity(entity, credentialProvider, undefined, testUser); + + expect(workflowFinderService.findWorkflowForUser).toHaveBeenCalledWith('wf-1', testUser, [ + 'workflow:execute', + ]); + expect(toolNamesPassedToBuildFromJson()).toEqual([]); + }); + + it('keeps node and workflow tools for a fully-privileged user', async () => { + vi.mocked(userHasScopes).mockResolvedValue(true); + const credentialsFinderService = mock(); + credentialsFinderService.findCredentialForUser.mockResolvedValue( + mock({ id: 'cred-1' }), + ); + const workflowRepository = mock(); + workflowRepository.findOne.mockResolvedValue(mock({ id: 'wf-1' })); + const workflowFinderService = mock(); + workflowFinderService.findWorkflowForUser.mockResolvedValue( + mock>>({ id: 'wf-1' }), + ); + const { service } = makeService({ + credentialsFinderService, + workflowFinderService, + workflowRepository, + }); + const entity = makeAgentEntity([nodeToolWithCredential, workflowTool, customTool]); + const credentialProvider = mock(); + + await service.reconstructFromAgentEntity(entity, credentialProvider, undefined, testUser); + + expect(toolNamesPassedToBuildFromJson()).toEqual( + expect.arrayContaining(['Send Slack message', 'Lookup customer', 'custom_tool']), + ); + }); +}); + +describe('AgentRuntimeReconstructionService.reconstructFromResolvedSource — per-user tool filtering', () => { + beforeEach(() => { + vi.clearAllMocks(); + builtAgent.hasCheckpointStorage.mockReturnValue(true); + Container.set(SubAgentForegroundRunner, mock()); + }); + + afterEach(() => { + Container.reset(); + }); + + function makeResolvedSourceConfig(tools: AgentJsonToolConfig[]): AgentJsonConfig { + return { + name: 'Child Agent', + model: 'anthropic/claude-sonnet-4-5', + instructions: 'Be helpful', + tools, + }; + } + + it('filters sub-agent tools by the delegating user access when a user is present', async () => { + vi.mocked(userHasScopes).mockResolvedValue(false); + const { service } = makeService({}); + const config = makeResolvedSourceConfig([nodeToolWithCredential, workflowTool, customTool]); + + await service.reconstructFromResolvedSource({ + config, + memoryOwnerAgentId: 'child-1', + projectId, + credentialProvider: mock(), + toolDescriptors: {}, + toolCodeByName: {}, + skills: {}, + runtimeProfile: 'sub-agent', + user: testUser, + }); + + expect(userHasScopes).toHaveBeenCalledWith(testUser, ['workflow:execute'], false, { + projectId, + }); + expect(toolNamesPassedToBuildFromJson()).toEqual(['custom_tool']); + }); + + it('does not filter sub-agent tools when no user is present (published/integration parents)', async () => { + const { service } = makeService({}); + const config = makeResolvedSourceConfig([nodeToolWithCredential, workflowTool, customTool]); + + await service.reconstructFromResolvedSource({ + config, + memoryOwnerAgentId: 'child-1', + projectId, + credentialProvider: mock(), + toolDescriptors: {}, + toolCodeByName: {}, + skills: {}, + runtimeProfile: 'sub-agent', + }); + + expect(userHasScopes).not.toHaveBeenCalled(); + expect(toolNamesPassedToBuildFromJson()).toEqual( + expect.arrayContaining(['Send Slack message', 'Lookup customer', 'custom_tool']), + ); + }); +}); diff --git a/packages/cli/src/modules/agents/__tests__/agent-task.service.test.ts b/packages/cli/src/modules/agents/__tests__/agent-task.service.test.ts index 2d2e0d1323f..6d01a3cfbb8 100644 --- a/packages/cli/src/modules/agents/__tests__/agent-task.service.test.ts +++ b/packages/cli/src/modules/agents/__tests__/agent-task.service.test.ts @@ -1,6 +1,7 @@ import type { Mock } from 'vitest'; import type { Logger } from '@n8n/backend-common'; import type { GlobalConfig } from '@n8n/config'; +import type { User } from '@n8n/db'; import { mock } from 'vitest-mock-extended'; import type { InstanceSettings, ScheduledTaskManager } from 'n8n-core'; @@ -724,18 +725,20 @@ describe('AgentTaskService', () => { }); describe('runNow', () => { + const requestingUser = mock({ id: 'user-9' }); + it('runs the task immediately as the requesting user even when unpublished', async () => { (taskRepository.findByIdAndAgentId as Mock).mockResolvedValue(makeTask()); (agentRepository.findOne as Mock).mockResolvedValue(makeAgent({ activeVersionId: null })); (agentExecutionOrchestratorService.executeForTaskNow as Mock).mockReturnValue(emptyStream()); - await service.runNow(AGENT_ID, 'task-1', 'user-9'); + await service.runNow(AGENT_ID, 'task-1', requestingUser); expect(agentExecutionOrchestratorService.executeForTaskNow).toHaveBeenCalledWith( expect.objectContaining({ agentId: AGENT_ID, projectId: 'project-1', - userId: 'user-9', + user: requestingUser, taskId: 'task-1', message: expect.stringContaining('Summarize messages'), memory: expect.objectContaining({ resourceId: 'task:task-1' }), @@ -746,7 +749,9 @@ describe('AgentTaskService', () => { it('throws NotFoundError when the task does not exist', async () => { (taskRepository.findByIdAndAgentId as Mock).mockResolvedValue(null); - await expect(service.runNow(AGENT_ID, 'missing', 'user-9')).rejects.toThrow(NotFoundError); + await expect(service.runNow(AGENT_ID, 'missing', requestingUser)).rejects.toThrow( + NotFoundError, + ); expect(agentExecutionOrchestratorService.executeForTaskNow).not.toHaveBeenCalled(); }); @@ -755,7 +760,7 @@ describe('AgentTaskService', () => { (agentRepository.findOne as Mock).mockResolvedValue(makeAgent()); (agentExecutionOrchestratorService.executeForTaskNow as Mock).mockReturnValue(emptyStream()); - await service.runNow(AGENT_ID, 'task-1', 'user-9'); + await service.runNow(AGENT_ID, 'task-1', requestingUser); await new Promise((resolve) => setImmediate(resolve)); expect(taskRepository.update).not.toHaveBeenCalled(); diff --git a/packages/cli/src/modules/agents/__tests__/agent-tasks.controller.test.ts b/packages/cli/src/modules/agents/__tests__/agent-tasks.controller.test.ts index 056b0758e6d..f0bec7c1a63 100644 --- a/packages/cli/src/modules/agents/__tests__/agent-tasks.controller.test.ts +++ b/packages/cli/src/modules/agents/__tests__/agent-tasks.controller.test.ts @@ -131,7 +131,7 @@ describe('AgentTasksController tasks', () => { const result = await controller.runTaskNow(runReq, undefined as never, 'agent-1', 'task-1'); - expect(agentTaskService.runNow).toHaveBeenCalledWith('agent-1', 'task-1', 'user-1'); + expect(agentTaskService.runNow).toHaveBeenCalledWith('agent-1', 'task-1', { id: 'user-1' }); expect(result).toEqual({ success: true }); }); diff --git a/packages/cli/src/modules/agents/__tests__/agents-service-reconstruct-gating.test.ts b/packages/cli/src/modules/agents/__tests__/agents-service-reconstruct-gating.test.ts index ba7a77c5aec..221d37da41a 100644 --- a/packages/cli/src/modules/agents/__tests__/agents-service-reconstruct-gating.test.ts +++ b/packages/cli/src/modules/agents/__tests__/agents-service-reconstruct-gating.test.ts @@ -26,10 +26,12 @@ import { Container } from '@n8n/di'; import { mock } from 'vitest-mock-extended'; import type { ActiveExecutions } from '@/active-executions'; +import type { CredentialsFinderService } from '@/credentials/credentials-finder.service'; import type { EphemeralNodeExecutor } from '@/node-execution'; import type { OauthService } from '@/oauth/oauth.service'; import type { AiService } from '@/services/ai.service'; import type { UrlService } from '@/services/url.service'; +import type { WorkflowFinderService } from '@/workflows/workflow-finder.service'; import { AgentRuntimeReconstructionService } from '../agent-runtime-reconstruction.service'; import type { AgentKnowledgeSandboxService } from '../agent-knowledge-sandbox.service'; @@ -119,6 +121,8 @@ function makeReconstructionService( mock(), mock({ enabled: true }), mock(), + mock(), + mock(), ); } @@ -343,6 +347,50 @@ describe('AgentRuntimeReconstructionService.reconstructFromAgentEntity — sub-a ]); }); + it('references a published sub-agent by id only, with no versionId pin', async () => { + const agentRepository = mock(); + agentRepository.findByIdAndProjectId.mockResolvedValue({ + id: 'agent-billing', + name: 'Billing Agent', + activeVersionId: 'version-billing', + } as Agent); + const service = makeReconstructionService([], { agentRepository }); + const config: AgentJsonConfig = { + name: 'Test', + model: 'anthropic/claude-sonnet-4-5', + instructions: 'Be helpful', + subAgents: { agents: [{ agentId: 'agent-billing' }] }, + }; + + const { sourcesById } = await service.createSubAgentDelegationConfig(config, 'project-1'); + + expect(sourcesById).toEqual({ 'agent-billing': { agentId: 'agent-billing' } }); + }); + + it('omits an unpublished sub-agent from sourcesById and availableSubAgents', async () => { + const agentRepository = mock(); + agentRepository.findByIdAndProjectId.mockResolvedValue({ + id: 'agent-billing', + name: 'Billing Agent', + activeVersionId: null, + } as Agent); + const service = makeReconstructionService([], { agentRepository }); + const config: AgentJsonConfig = { + name: 'Test', + model: 'anthropic/claude-sonnet-4-5', + instructions: 'Be helpful', + subAgents: { agents: [{ agentId: 'agent-billing' }] }, + }; + + const { sourcesById, availableSubAgents } = await service.createSubAgentDelegationConfig( + config, + 'project-1', + ); + + expect(sourcesById).toEqual({}); + expect(availableSubAgents).toEqual([]); + }); + it('resolves subAgents.modelsByDifficulty into delegate tool metadata', async () => { const credentialProvider = mock(); credentialProvider.resolve.mockImplementation(async (credentialId: string) => { diff --git a/packages/cli/src/modules/agents/__tests__/agents.service.test.ts b/packages/cli/src/modules/agents/__tests__/agents.service.test.ts index 9ad94653695..580fd51af1e 100644 --- a/packages/cli/src/modules/agents/__tests__/agents.service.test.ts +++ b/packages/cli/src/modules/agents/__tests__/agents.service.test.ts @@ -13,6 +13,7 @@ import { AgentsService } from '../agents.service'; import type { Agent } from '../entities/agent.entity'; import { ChatIntegrationService } from '../integrations/chat-integration.service'; import type { AgentRepository } from '../repositories/agent.repository'; +import type { SubAgentCleanupService } from '../sub-agents/sub-agent-cleanup.service'; const agentId = 'agent-1'; const projectId = 'project-1'; @@ -41,11 +42,13 @@ function makeService() { const testChatService = mock(); const agentTaskService = mock(); const chatIntegrationService = mock(); + const subAgentCleanupService = mock(); agentRepository.save.mockImplementation(async (agent) => agent as Agent); agentTaskService.requestReconcile.mockResolvedValue(); chatIntegrationService.disconnectChannel.mockResolvedValue(); testChatService.clearAllTestChatMessages.mockResolvedValue(); + subAgentCleanupService.removeSubAgentFromParents.mockResolvedValue(); Container.set(AgentTaskService, agentTaskService); Container.set(ChatIntegrationService, chatIntegrationService); @@ -56,6 +59,7 @@ function makeService() { agentKnowledgeService, runtimeCacheService, testChatService, + subAgentCleanupService, ); return { @@ -66,6 +70,7 @@ function makeService() { testChatService, agentTaskService, chatIntegrationService, + subAgentCleanupService, }; } @@ -110,6 +115,7 @@ describe('AgentsService', () => { testChatService, agentTaskService, chatIntegrationService, + subAgentCleanupService, } = makeService(); const agent = makeAgent({ integrations: [ @@ -136,6 +142,10 @@ describe('AgentsService', () => { credentialId: 'telegram-1', }); expect(runtimeCacheService.clearRuntimes).toHaveBeenCalledWith(agentId); + expect(subAgentCleanupService.removeSubAgentFromParents).toHaveBeenCalledWith( + agentId, + projectId, + ); expect(agentTaskService.requestReconcile).toHaveBeenCalledWith(agentId); expect(testChatService.clearAllTestChatMessages).toHaveBeenCalledWith(agentId); expect(agentKnowledgeService.destroySandbox).toHaveBeenCalledWith(projectId, agentId); diff --git a/packages/cli/src/modules/agents/agent-chat.controller.ts b/packages/cli/src/modules/agents/agent-chat.controller.ts index 7f8727f1dd3..7eebf8c4204 100644 --- a/packages/cli/src/modules/agents/agent-chat.controller.ts +++ b/packages/cli/src/modules/agents/agent-chat.controller.ts @@ -91,7 +91,7 @@ export class AgentChatController { agentId, projectId, message, - userId: req.user.id, + user: req.user, memory: { threadId, resourceId: draftChatMemoryResourceId(req.user.id), @@ -130,7 +130,7 @@ export class AgentChatController { runId, toolCallId, resumeData, - userId: req.user.id, + user: req.user, usePublishedVersion: false, integrationType: N8N_CHAT_INTEGRATION_TYPE, }), diff --git a/packages/cli/src/modules/agents/agent-execution-orchestrator.service.ts b/packages/cli/src/modules/agents/agent-execution-orchestrator.service.ts index 19bc2ac271d..850ffc3dc2d 100644 --- a/packages/cli/src/modules/agents/agent-execution-orchestrator.service.ts +++ b/packages/cli/src/modules/agents/agent-execution-orchestrator.service.ts @@ -9,6 +9,7 @@ import type { import type { AgentPersistedMessageDto } from '@n8n/api-types'; import { AGENT_WORKFLOW_TRIGGER_TYPE, N8N_CHAT_INTEGRATION_TYPE } from '@n8n/api-types'; import { Logger } from '@n8n/backend-common'; +import type { User } from '@n8n/db'; import { Service } from '@n8n/di'; import type { JSONSchema7 } from 'json-schema'; import { @@ -49,8 +50,13 @@ export interface ExecuteForChatConfig { agentId: string; projectId: string; message: string; - /** n8n user ID — used for RBAC / credential resolution. */ - userId: string; + /** + * The calling n8n user — used to gate node/workflow tools by their access, + * and for RBAC / credential resolution and telemetry attribution. Always + * present: the in-app test chat only runs behind an authenticated session + * (`AgentChatController.chat` always has `req.user`). + */ + user: User; /** Memory scope — resourceId is the chat platform user (e.g. Slack / Telegram user ID). */ memory: AgentMemoryScope; } @@ -62,6 +68,13 @@ export interface ExecuteForChatPublishedConfig { /** Memory scope — resourceId is the chat platform user (e.g. Slack / Telegram user ID). */ memory: AgentMemoryScope; integrationType?: string; + // No `user` field here: a published chat integration (Slack, Telegram, …) + // run is triggered by an inbound platform event, not an interactive n8n + // session — there is no n8n `User` to attach. This path keeps the + // project-scoped trust boundary that existed before per-user tool + // gating; the admin who published the agent is the one who approved its + // tools, and Layer A's node denylist (`EphemeralNodeExecutor`) still + // applies regardless. } export interface ResumeForChatConfig { @@ -70,8 +83,12 @@ export interface ResumeForChatConfig { runId: string; toolCallId: string; resumeData: unknown; - /** n8n user ID for in-app preview chat resumes. */ - userId?: string; + /** + * The calling n8n user for in-app preview chat resumes — used to gate + * node/workflow tools by their access. Absent for published/integration + * resumes, which keep today's project-scoped behavior. + */ + user?: User; /** Defaults to true for external integrations; preview chat passes false. */ usePublishedVersion?: boolean; /** @@ -93,13 +110,23 @@ export interface ExecuteForTaskPublishedConfig { taskId: string; /** Published agent_history version that supplied the scheduled task snapshot. */ taskVersionId: string; + // No `user` field here: this run is fired by `ScheduledTaskManager` on a + // cron tick — there is no human in the loop at all, let alone an n8n + // session, so there's nothing to gate tools against. Same project-scoped + // trust boundary as `ExecuteForChatPublishedConfig`. } export interface ExecuteForTaskNowConfig { agentId: string; projectId: string; - /** n8n user ID — used for RBAC / credential resolution and recorded on the session. */ - userId: string; + /** + * The calling n8n user — used to gate node/workflow tools by their + * access, and for RBAC / credential resolution and recorded on the + * session. Always present: manual "Run now" is triggered by an authenticated + * `AgentTasksController.runTaskNow` request, threaded down via + * `AgentTaskService.runNow(agentId, taskId, user)`. + */ + user: User; message: string; /** Memory scope — resourceId isolates per-run memory. */ memory: AgentMemoryScope; @@ -222,7 +249,7 @@ export class AgentExecutionOrchestratorService { toolCallId, resumeData, integrationType, - userId, + user, usePublishedVersion = true, } = config; @@ -247,6 +274,14 @@ export class AgentExecutionOrchestratorService { projectId, usePublishedVersion, integrationType, + // `usePublishedVersion` defaults to true and is what platform + // integrations (Slack/Telegram HITL resume) use — those have no + // interactive n8n user, so `user` is force-undefined to keep the + // existing project-scoped runtime. Only the in-app draft/test-chat + // resume passes `usePublishedVersion: false` (see + // `AgentChatController.chatResume`), and only then does the caller's + // `user` actually reach the cache/reconstruction layer. + user: usePublishedVersion ? undefined : user, }); const { agent: agentInstance, toolRegistry } = runtime; @@ -257,7 +292,7 @@ export class AgentExecutionOrchestratorService { const resultStream = await agentInstance.resume('stream', resumeData, { runId, toolCallId, - executionCounter: this.createAgentExecutionCounter({ agentId, userId }), + executionCounter: this.createAgentExecutionCounter({ agentId, userId: user?.id }), }); for await (const value of streamAgentChunks(resultStream.stream)) { @@ -301,19 +336,22 @@ export class AgentExecutionOrchestratorService { * Execute an agent for the in-app test chat and yield stream chunks. */ async *executeForChat(config: ExecuteForChatConfig): AsyncGenerator { - const { agentId, projectId, message, userId, memory } = config; + const { agentId, projectId, message, user, memory } = config; + // `user` is always set (see ExecuteForChatConfig) — this builds/reuses a + // runtime scoped to this specific user's tool access. const runtime = await this.runtimeCacheService.getRuntime({ agentId, projectId, integrationType: N8N_CHAT_INTEGRATION_TYPE, + user, }); await this.integrationMessageContextService.setLatest(memory.threadId, memory.resourceId, { integrationConnectionId: N8N_CHAT_INTEGRATION_TYPE, platform: N8N_CHAT_INTEGRATION_TYPE, - target: { type: 'dm', userId, threadId: memory.threadId }, - interactingUserId: userId, + target: { type: 'dm', userId: user.id, threadId: memory.threadId }, + interactingUserId: user.id, updatedAt: new Date().toISOString(), }); @@ -321,7 +359,7 @@ export class AgentExecutionOrchestratorService { agentInstance: runtime.agent, toolRegistry: runtime.toolRegistry, agentId, - userId, + userId: user.id, message, memory, projectId: runtime.projectId, @@ -342,6 +380,10 @@ export class AgentExecutionOrchestratorService { ): AsyncGenerator { const { agentId, projectId, message, memory, integrationType } = config; + // No `user` (see ExecuteForChatPublishedConfig): this is the shared, + // project-scoped runtime — every caller of this published agent through + // this integration reuses the same cache entry regardless of who + // triggered the platform event. const runtime = await this.runtimeCacheService.getRuntime({ agentId, projectId, @@ -373,6 +415,8 @@ export class AgentExecutionOrchestratorService { ): AsyncGenerator { const { agentId, projectId, message, memory, taskId, taskVersionId } = config; + // No `user` (see ExecuteForTaskPublishedConfig): cron-fired, no human to + // attach — same shared, project-scoped runtime for every tick. const runtime = await this.runtimeCacheService.getRuntime({ agentId, projectId, @@ -402,18 +446,22 @@ export class AgentExecutionOrchestratorService { * requesting user. */ async *executeForTaskNow(config: ExecuteForTaskNowConfig): AsyncGenerator { - const { agentId, projectId, userId, message, memory, taskId } = config; + const { agentId, projectId, user, message, memory, taskId } = config; + // `user` is always set (see ExecuteForTaskNowConfig) — manual "Run now" + // runs get a runtime scoped to the requesting user's tool access, same + // as the in-app test chat. const runtime = await this.runtimeCacheService.getRuntime({ agentId, projectId, + user, }); yield* this.streamChatResponse({ agentInstance: runtime.agent, toolRegistry: runtime.toolRegistry, agentId, - userId, + userId: user.id, message, memory, projectId: runtime.projectId, @@ -518,6 +566,13 @@ export class AgentExecutionOrchestratorService { } try { + // No `user`: this path runs an agent invoked from inside a workflow + // execution (AI Agent node, or a "Message an Agent" tool call from + // another workflow) — there is no interactive n8n user, only a bare + // telemetry id (see `executeForWorkflow`'s `telemetryUserId`), and for + // webhook/trigger-fired executions even that can be absent. This + // runtime also isn't cached (see the docstring above), so there's no + // cache-key concern here either — just no per-user tool filtering. const { agent: reconstructed } = await this.agentRuntimeReconstructionService.reconstructFromAgentEntity( agentEntity, diff --git a/packages/cli/src/modules/agents/agent-publish.service.ts b/packages/cli/src/modules/agents/agent-publish.service.ts index 422019b69b2..17980277b7f 100644 --- a/packages/cli/src/modules/agents/agent-publish.service.ts +++ b/packages/cli/src/modules/agents/agent-publish.service.ts @@ -21,6 +21,7 @@ import { ChatIntegrationService } from './integrations/chat-integration.service' import { AgentHistoryRepository } from './repositories/agent-history.repository'; import { AgentTaskSnapshotRepository } from './repositories/agent-task-snapshot.repository'; import { AgentRepository } from './repositories/agent.repository'; +import { SubAgentCleanupService } from './sub-agents/sub-agent-cleanup.service'; import { getMissingSkillIds } from '@/modules/agents/utils/agent-missing-skill-ids'; export interface PublishAgentOptions { @@ -36,6 +37,7 @@ export class AgentPublishService { private readonly agentTaskSnapshotRepository: AgentTaskSnapshotRepository, private readonly customToolsService: AgentCustomToolsService, private readonly runtimeCacheService: AgentRuntimeCacheService, + private readonly subAgentCleanupService: SubAgentCleanupService, ) {} async publishAgent( @@ -134,6 +136,8 @@ export class AgentPublishService { this.runtimeCacheService.clearRuntimes(agentId); + await this.subAgentCleanupService.removeSubAgentFromParents(agentId, projectId); + const chatIntegrationService = Container.get(ChatIntegrationService); for (const integration of agent.integrations ?? []) { await chatIntegrationService.disconnectChannel(agentId, integration, { diff --git a/packages/cli/src/modules/agents/agent-runtime-cache.service.ts b/packages/cli/src/modules/agents/agent-runtime-cache.service.ts index 2e02664398b..f5268ac4758 100644 --- a/packages/cli/src/modules/agents/agent-runtime-cache.service.ts +++ b/packages/cli/src/modules/agents/agent-runtime-cache.service.ts @@ -2,6 +2,7 @@ import type { Agent as RuntimeAgent } from '@n8n/agents'; import { Logger } from '@n8n/backend-common'; import { GlobalConfig } from '@n8n/config'; import { Time } from '@n8n/constants'; +import type { User } from '@n8n/db'; import { OnPubSubEvent } from '@n8n/decorators'; import { Service } from '@n8n/di'; @@ -26,6 +27,13 @@ export interface GetRuntimeParams { integrationType?: string; /** When true, load the published snapshot. */ usePublishedVersion?: boolean; + /** + * The calling n8n user. When present, the runtime is built with node/workflow + * tools filtered down to what this user can access, and the cache key is + * scoped to the user so different users never share a runtime. Absent for + * published/integration runs, which keep today's project-scoped runtime. + */ + user?: User; } export interface AgentRuntime { @@ -45,7 +53,7 @@ interface RuntimeInitialization { export class AgentRuntimeCacheService { /** * Cached agent runtimes. Keys follow the pattern: - * Draft: `{agentId}:draft[:{integrationType}]` + * Draft: `{agentId}:draft[:{integrationType}][:user:{userId}]` * Published: `{agentId}:published[:{integrationType}]` * * TTL = 30 minutes — entries are evicted when the agent is idle so that @@ -53,6 +61,14 @@ export class AgentRuntimeCacheService { * * Separating draft and published with explicit prefixes prevents a draft * runtime from being mistakenly returned to a published-agent execution. + * + * The `:user:{userId}` suffix only ever appears on draft keys — published + * runs never carry a `user` (see `GetRuntimeParams.user`), since they have + * no interactive n8n session to gate tools against. A draft runtime's tool + * list is filtered per-user at build time (see + * `AgentRuntimeReconstructionService.reconstructFromAgentEntity`), so two + * different users hitting the same draft agent must never resolve to the + * same cache entry — that would leak one user's tool access to the other. */ private readonly runtimes = new TtlMap(30 * Time.minutes.toMilliseconds); @@ -75,6 +91,10 @@ export class AgentRuntimeCacheService { } const parts = [params.agentId, 'draft']; if (params.integrationType) parts.push(params.integrationType); + // Per-user runtimes have node/workflow tools filtered by that user's + // access — keying by user id keeps them from colliding with each other + // or with the unscoped (no-user) runtime. + if (params.user) parts.push(`user:${params.user.id}`); return parts.join(':'); } @@ -188,7 +208,7 @@ export class AgentRuntimeCacheService { } private async reconstructRuntime(params: GetRuntimeParams): Promise { - const { agentId, projectId, integrationType, usePublishedVersion } = params; + const { agentId, projectId, integrationType, usePublishedVersion, user } = params; const agentEntity = await this.agentRepository.findByIdAndProjectId(agentId, projectId); if (!agentEntity) throw new NotFoundError(`Agent ${agentId} not found`); @@ -197,12 +217,23 @@ export class AgentRuntimeCacheService { ? getPublishedAgentSnapshot(agentEntity) : agentEntity; - const credentialProvider = createAgentCredentialProvider(this.credentialsService, projectId); + // `user` here is whatever `computeRuntimeCacheKey` above already keyed + // this build on — undefined for published/integration runs, set for + // in-app chat/resume/task-now. Forwarded to both the credential provider + // (so credential lookups are scoped to what this user can access) and + // the reconstruction service (so node/workflow tools the user can't run + // are dropped before the runtime is built). + const credentialProvider = createAgentCredentialProvider( + this.credentialsService, + projectId, + user, + ); const { agent: agentInstance, toolRegistry } = await this.agentRuntimeReconstructionService.reconstructFromAgentEntity( agentData, credentialProvider, integrationType, + user, ); return { diff --git a/packages/cli/src/modules/agents/agent-runtime-reconstruction.service.ts b/packages/cli/src/modules/agents/agent-runtime-reconstruction.service.ts index b9fbf85fb4d..07a695362ff 100644 --- a/packages/cli/src/modules/agents/agent-runtime-reconstruction.service.ts +++ b/packages/cli/src/modules/agents/agent-runtime-reconstruction.service.ts @@ -27,6 +27,7 @@ import { import { Logger } from '@n8n/backend-common'; import { OutboundHttp, SsrfProtectionService } from '@n8n/backend-network'; import { AgentsConfig, SsrfProtectionConfig } from '@n8n/config'; +import type { User } from '@n8n/db'; import { WorkflowRepository } from '@n8n/db'; import { Container, Service } from '@n8n/di'; import { UserError } from 'n8n-workflow'; @@ -34,14 +35,19 @@ import { nanoid } from 'nanoid'; import { ActiveExecutions } from '@/active-executions'; import { N8N_VERSION } from '@/constants'; +import { CredentialsFinderService } from '@/credentials/credentials-finder.service'; import { EphemeralNodeExecutor } from '@/node-execution'; import { OauthService } from '@/oauth/oauth.service'; -import { UrlService } from '@/services/url.service'; +import { userHasScopes } from '@/permissions.ee/check-access'; import { AiService } from '@/services/ai.service'; import { ProxyTokenManager } from '@/services/proxy-token-manager'; +import { UrlService } from '@/services/url.service'; import { createAiMcpFetch, createAiProxyFetch } from '@/utils/ai-proxy-fetch'; import { WorkflowRunner } from '@/workflow-runner'; +import { WorkflowFinderService } from '@/workflows/workflow-finder.service'; +import { isAgentKnowledgeBaseEnabled } from './agent-knowledge-gate'; +import { AgentKnowledgeSandboxService } from './agent-knowledge-sandbox.service'; import { Agent } from './entities/agent.entity'; import { ChatIntegrationRegistry } from './integrations/agent-chat-integration'; import { @@ -64,8 +70,6 @@ import { resolveCredentialAwareModelConfig } from './json-config/model-config'; import { AgentFileRepository } from './repositories/agent-file.repository'; import { AgentRepository } from './repositories/agent.repository'; import { AgentSecureRuntime } from './runtime/agent-secure-runtime'; -import { isAgentKnowledgeBaseEnabled } from './agent-knowledge-gate'; -import { AgentKnowledgeSandboxService } from './agent-knowledge-sandbox.service'; import { createN8nDelegateSubAgentTool } from './sub-agents/delegate-sub-agent-tool'; import { SubAgentForegroundRunner } from './sub-agents/sub-agent-foreground-runner'; import { buildToolRegistry, type ToolRegistry } from './tool-registry'; @@ -93,10 +97,16 @@ export interface ReconstructAgentRuntimeParams { integrationType?: string; /** Top-level chat/integration runtimes only. */ credentialIntegrations?: AgentIntegrationConfig[]; + /** + * The interactive n8n user of the delegating parent run, when there is one. + * When present, node/workflow tool refs are filtered by this user's access + * (same rules as reconstructFromAgentEntity). Absent for published/ + * integration parents, which keep the project-scoped trust boundary. + */ + user?: User; } async function getChatIntegrationToolServices() { - // eslint-disable-next-line import-x/no-cycle const { IntegrationMessageContextService } = await import( './integrations/integration-message-context.service' ); @@ -104,7 +114,6 @@ async function getChatIntegrationToolServices() { const { ChatIntegrationActionExecutor } = await import( './integrations/integration-action-executor' ); - // eslint-disable-next-line import-x/no-cycle const { ChatIntegrationContextQueryExecutor } = await import( './integrations/integration-context-query-executor' ); @@ -141,18 +150,33 @@ export class AgentRuntimeReconstructionService { private readonly agentKnowledgeSandboxService: AgentKnowledgeSandboxService, private readonly ssrfConfig: SsrfProtectionConfig, private readonly ssrfProtectionService: SsrfProtectionService, + private readonly credentialsFinderService: CredentialsFinderService, + private readonly workflowFinderService: WorkflowFinderService, ) {} async reconstructFromAgentEntity( agentEntity: Agent, credentialProvider: CredentialProvider, integrationType?: string, + user?: User, ): Promise<{ agent: RuntimeAgent; toolRegistry: ToolRegistry }> { - const config = agentEntity.schema; + let config = agentEntity.schema; if (!config) { throw new UserError('Agent has no JSON config.'); } + // Published/integration runs have no interactive n8n user and keep + // today's project-scoped trust boundary. When a user is present (in-app + // chat, resume, task-now), drop node/workflow tools the user can't + // execute or lacks credential/workflow access to before the runtime is + // built, so denied tools never reach the LLM or the executor. + if (user && config.tools?.length) { + config = { + ...config, + tools: await this.filterToolsForUser(config.tools, agentEntity.projectId, user), + }; + } + const toolsByName: Record = {}; const toolDescriptors: Record = {}; for (const [_toolId, toolEntry] of Object.entries(agentEntity.tools ?? {})) { @@ -178,19 +202,101 @@ export class AgentRuntimeReconstructionService { integrationType, credentialIntegrations: agentEntity.integrations ?? [], subAgentDelegation, + user, }); } + /** + * Drop node/workflow tool refs the calling user can't run: `workflow:execute` + * gates both kinds, and node tools additionally need `credential:read` on + * every credential baked into the ref. Filtering the config copy (rather + * than the resolved tools) means a denied ref never reaches + * `makeToolResolver`/`resolveToolRef`, so no inert marker tool is exposed to + * the LLM. Custom tools are untouched — they run n8n-authored code, not a + * caller-chosen node/workflow with baked credentials. + */ + private async filterToolsForUser( + tools: AgentJsonToolConfig[], + projectId: string, + user: User, + ): Promise { + const canExecute = await userHasScopes(user, ['workflow:execute'], false, { projectId }); + + const filtered: AgentJsonToolConfig[] = []; + for (const ref of tools) { + if (ref.type === 'custom') { + filtered.push(ref); + continue; + } + + if (!canExecute) continue; + + if (ref.type === 'node') { + const credentialIds = Object.values(ref.node.credentials ?? {}) + .map((credential) => credential.id) + .filter((id): id is string => Boolean(id)); + + const accessibleCredentials = await Promise.all( + credentialIds.map( + async (id) => + await this.credentialsFinderService.findCredentialForUser(id, user, [ + 'credential:read', + ]), + ), + ); + if (accessibleCredentials.some((credential) => credential === null)) continue; + + filtered.push(ref); + continue; + } + + // ref.type === 'workflow' + const workflow = await this.workflowRepository.findOne({ + where: { name: ref.workflow, shared: { projectId } }, + relations: ['shared'], + }); + if (!workflow) continue; + + const accessibleWorkflow = await this.workflowFinderService.findWorkflowForUser( + workflow.id, + user, + ['workflow:execute'], + ); + if (!accessibleWorkflow) continue; + + filtered.push(ref); + } + + return filtered; + } + + /** + * Build a sub-agent's runtime for a `delegate_subagent` call. + * + * When `params.user` is present (the delegating parent had an interactive + * n8n user), node/workflow tool refs are filtered by that user's access — + * same rules as `reconstructFromAgentEntity`. Absent for published/ + * integration parents, which keep the project-scoped trust boundary. The + * sub-agent also inherits the parent's `credentialProvider` (user-scoped + * when the parent had a user), so raw credential access stays gated there + * regardless. + */ async reconstructFromResolvedSource( params: ReconstructAgentRuntimeParams, ): Promise<{ agent: RuntimeAgent; toolRegistry: ToolRegistry }> { - const subAgentDelegation = await this.createSubAgentDelegationConfig( - params.config, - params.projectId, - ); + let config = params.config; + if (params.user && config.tools?.length) { + config = { + ...config, + tools: await this.filterToolsForUser(config.tools, params.projectId, params.user), + }; + } + + const subAgentDelegation = await this.createSubAgentDelegationConfig(config, params.projectId); return await this.reconstructRuntime({ ...params, + config, credentialIntegrations: [], subAgentDelegation, }); @@ -209,6 +315,7 @@ export class AgentRuntimeReconstructionService { integrationType?: string; credentialIntegrations: AgentIntegrationConfig[]; subAgentDelegation: SubAgentDelegationConfig; + user?: User; }): Promise<{ agent: RuntimeAgent; toolRegistry: ToolRegistry }> { const { config, @@ -223,6 +330,7 @@ export class AgentRuntimeReconstructionService { integrationType, credentialIntegrations, subAgentDelegation, + user, } = options; const toolExecutor = this.secureRuntime.createToolExecutor(toolCodeByName); @@ -273,6 +381,7 @@ export class AgentRuntimeReconstructionService { parentAgentIdForDelegation: parentAgentIdForDelegation ?? memoryOwnerAgentId, integrationType, credentialIntegrations, + user, }); return { agent: reconstructed, toolRegistry: buildToolRegistry(resolvedTools) }; @@ -293,7 +402,12 @@ export class AgentRuntimeReconstructionService { })) { if (!agent?.activeVersionId) continue; - sourcesById[agentId] = { agentId, versionId: agent.activeVersionId }; + // No versionId pin here: the delegate closure lives inside the + // cached parent runtime, so pinning would freeze the child at + // whatever was published when the parent was last built. Leaving + // it out means SubAgentSourceResolver re-resolves the child's + // current activeVersion on every delegation. + sourcesById[agentId] = { agentId }; availableSubAgents.push({ id: agentId, name: agent.name, @@ -381,6 +495,7 @@ export class AgentRuntimeReconstructionService { parentAgentIdForDelegation: string; integrationType?: string; credentialIntegrations: AgentIntegrationConfig[]; + user?: User; }): Promise { const { agent, @@ -393,6 +508,7 @@ export class AgentRuntimeReconstructionService { parentAgentIdForDelegation, integrationType, credentialIntegrations, + user, } = params; agent.tool(createGetEnvironmentTool()); @@ -479,6 +595,7 @@ export class AgentRuntimeReconstructionService { projectId, credentialProvider, delegation: subAgentDelegation, + user, }); this.attachWriteTodosTool(agent, agentId); } @@ -495,8 +612,10 @@ export class AgentRuntimeReconstructionService { projectId: string; credentialProvider: CredentialProvider; delegation: SubAgentDelegationConfig; + user?: User; }): Promise { - const { agent, config, parentAgentId, projectId, credentialProvider, delegation } = params; + const { agent, config, parentAgentId, projectId, credentialProvider, delegation, user } = + params; const inlineSubAgentModelsByDifficulty = await this.resolveInlineSubAgentModelsByDifficulty( config, credentialProvider, @@ -508,6 +627,7 @@ export class AgentRuntimeReconstructionService { projectId, parentAgentId, credentialProvider, + user, policy: this.buildSubAgentPolicy(config), ...(inlineSubAgentModelsByDifficulty !== undefined ? { inlineSubAgentModelsByDifficulty } diff --git a/packages/cli/src/modules/agents/agent-task.service.ts b/packages/cli/src/modules/agents/agent-task.service.ts index 3fe98b022e1..2a5975e7130 100644 --- a/packages/cli/src/modules/agents/agent-task.service.ts +++ b/packages/cli/src/modules/agents/agent-task.service.ts @@ -1,6 +1,7 @@ import type { AgentTaskDto, CreateAgentTaskDto, UpdateAgentTaskDto } from '@n8n/api-types'; import { Logger } from '@n8n/backend-common'; import { GlobalConfig } from '@n8n/config'; +import type { User } from '@n8n/db'; import { OnLeaderStepdown, OnLeaderTakeover, OnPubSubEvent, OnShutdown } from '@n8n/decorators'; import { Service } from '@n8n/di'; import { IsNull, Not } from '@n8n/typeorm'; @@ -518,17 +519,17 @@ export class AgentTaskService { * agent run can be long, so it is kicked off in the background and surfaces * as a session; only the lookup is awaited so a missing task still 404s. */ - async runNow(agentId: string, taskId: string, userId: string): Promise { + async runNow(agentId: string, taskId: string, user: User): Promise { const task = await this.getOrThrow(agentId, taskId); const agent = await this.agentRepository.findOne({ where: { id: agentId } }); if (!agent) { throw new NotFoundError(`Agent "${agentId}" not found`); } - void this.executeNow(task, agent.projectId, userId); + void this.executeNow(task, agent.projectId, user); } - private async executeNow(task: AgentTask, projectId: string, userId: string): Promise { + private async executeNow(task: AgentTask, projectId: string, user: User): Promise { const { message, threadId } = this.buildTaskRunMessage(task.id, task.objective); this.logger.info('[AgentTaskService] Manual task run started', { @@ -543,7 +544,7 @@ export class AgentTaskService { this.agentExecutionOrchestratorService.executeForTaskNow({ agentId: task.agentId, projectId, - userId, + user, message, memory: { threadId, resourceId: taskRunMemoryResourceId(task.id) }, taskId: task.id, diff --git a/packages/cli/src/modules/agents/agent-tasks.controller.ts b/packages/cli/src/modules/agents/agent-tasks.controller.ts index 39eaccc14aa..bb31f9f9132 100644 --- a/packages/cli/src/modules/agents/agent-tasks.controller.ts +++ b/packages/cli/src/modules/agents/agent-tasks.controller.ts @@ -89,7 +89,7 @@ export class AgentTasksController { @Param('taskId') taskId: string, ): Promise<{ success: true }> { await this.getAgentOrThrow(agentId, req.params.projectId); - await this.agentTaskService.runNow(agentId, taskId, req.user.id); + await this.agentTaskService.runNow(agentId, taskId, req.user); return { success: true }; } } diff --git a/packages/cli/src/modules/agents/agents.service.ts b/packages/cli/src/modules/agents/agents.service.ts index 958bc338842..8c61dbcf69d 100644 --- a/packages/cli/src/modules/agents/agents.service.ts +++ b/packages/cli/src/modules/agents/agents.service.ts @@ -10,6 +10,7 @@ import { AgentTestChatService } from './agent-test-chat.service'; import { Agent } from './entities/agent.entity'; import { ChatIntegrationService } from './integrations/chat-integration.service'; import { AgentRepository } from './repositories/agent.repository'; +import { SubAgentCleanupService } from './sub-agents/sub-agent-cleanup.service'; @Service() export class AgentsService { @@ -20,6 +21,7 @@ export class AgentsService { private readonly agentKnowledgeService: AgentKnowledgeService, private readonly runtimeCacheService: AgentRuntimeCacheService, private readonly testChatService: AgentTestChatService, + private readonly subAgentCleanupService: SubAgentCleanupService, ) {} async create(projectId: string, name: string): Promise { @@ -127,6 +129,8 @@ export class AgentsService { this.runtimeCacheService.clearRuntimes(agentId); + await this.subAgentCleanupService.removeSubAgentFromParents(agentId, projectId); + try { const { AgentTaskService } = await import('./agent-task.service'); await Container.get(AgentTaskService).requestReconcile(agentId); diff --git a/packages/cli/src/modules/agents/sub-agents/__tests__/sub-agent-cleanup.service.test.ts b/packages/cli/src/modules/agents/sub-agents/__tests__/sub-agent-cleanup.service.test.ts new file mode 100644 index 00000000000..2e096626582 --- /dev/null +++ b/packages/cli/src/modules/agents/sub-agents/__tests__/sub-agent-cleanup.service.test.ts @@ -0,0 +1,143 @@ +import { mockLogger } from '@n8n/backend-test-utils'; +import type { AgentJsonConfig } from '@n8n/api-types'; +import { mock } from 'vitest-mock-extended'; + +import type { AgentRuntimeCacheService } from '../../agent-runtime-cache.service'; +import type { Agent } from '../../entities/agent.entity'; +import type { AgentRepository } from '../../repositories/agent.repository'; +import { SubAgentCleanupService } from '../sub-agent-cleanup.service'; + +const projectId = 'project-1'; +const childAgentId = 'child-1'; + +function makeAgent(overrides: Partial = {}): Agent { + return { + id: 'parent-1', + projectId, + schema: { + name: 'Parent', + model: 'anthropic/claude-sonnet-4-5', + instructions: 'Delegate work', + } as AgentJsonConfig, + ...overrides, + } as unknown as Agent; +} + +function makeService() { + const agentRepository = mock(); + const runtimeCacheService = mock(); + + agentRepository.save.mockImplementation(async (agent) => agent as Agent); + + const service = new SubAgentCleanupService(mockLogger(), agentRepository, runtimeCacheService); + + return { service, agentRepository, runtimeCacheService }; +} + +describe('SubAgentCleanupService', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it('removes the sub-agent reference from a parent that configures it, preserving other refs', async () => { + const { service, agentRepository, runtimeCacheService } = makeService(); + const parent = makeAgent({ + id: 'parent-1', + versionId: 'v1', + activeVersionId: 'v1', + schema: { + name: 'Parent', + model: 'anthropic/claude-sonnet-4-5', + instructions: 'Delegate work', + subAgents: { + maxChildren: 5, + agents: [ + { agentId: childAgentId, useWhen: 'Use for X' }, + { agentId: 'other-child', useWhen: 'Use for Y' }, + ], + }, + } as AgentJsonConfig, + }); + agentRepository.find.mockResolvedValue([parent]); + + await service.removeSubAgentFromParents(childAgentId, projectId); + + expect(agentRepository.find).toHaveBeenCalledWith({ where: { projectId } }); + expect(parent.schema?.subAgents?.agents).toEqual([ + { agentId: 'other-child', useWhen: 'Use for Y' }, + ]); + expect(parent.schema?.subAgents?.maxChildren).toBe(5); + // markAgentDraftDirty bumps versionId away from the published version. + expect(parent.versionId).not.toBe('v1'); + expect(agentRepository.save).toHaveBeenCalledWith(parent); + expect(runtimeCacheService.clearRuntimes).toHaveBeenCalledWith('parent-1'); + }); + + it('leaves a parent that does not reference the sub-agent untouched', async () => { + const { service, agentRepository, runtimeCacheService } = makeService(); + const parent = makeAgent({ + id: 'parent-2', + schema: { + name: 'Parent', + model: 'anthropic/claude-sonnet-4-5', + instructions: 'Delegate work', + subAgents: { agents: [{ agentId: 'other-child' }] }, + } as AgentJsonConfig, + }); + agentRepository.find.mockResolvedValue([parent]); + + await service.removeSubAgentFromParents(childAgentId, projectId); + + expect(agentRepository.save).not.toHaveBeenCalled(); + expect(runtimeCacheService.clearRuntimes).not.toHaveBeenCalled(); + }); + + it('skips the child agent itself and agents with no subAgents config', async () => { + const { service, agentRepository, runtimeCacheService } = makeService(); + const child = makeAgent({ + id: childAgentId, + schema: { + name: 'Child', + model: 'anthropic/claude-sonnet-4-5', + instructions: 'x', + subAgents: { agents: [{ agentId: childAgentId }] }, + } as AgentJsonConfig, + }); + const noSubAgents = makeAgent({ id: 'parent-3', schema: null }); + agentRepository.find.mockResolvedValue([child, noSubAgents]); + + await service.removeSubAgentFromParents(childAgentId, projectId); + + expect(agentRepository.save).not.toHaveBeenCalled(); + expect(runtimeCacheService.clearRuntimes).not.toHaveBeenCalled(); + }); + + it('updates every matching parent when multiple agents reference the sub-agent', async () => { + const { service, agentRepository, runtimeCacheService } = makeService(); + const parentA = makeAgent({ + id: 'parent-a', + schema: { + name: 'Parent A', + model: 'anthropic/claude-sonnet-4-5', + instructions: 'x', + subAgents: { agents: [{ agentId: childAgentId }] }, + } as AgentJsonConfig, + }); + const parentB = makeAgent({ + id: 'parent-b', + schema: { + name: 'Parent B', + model: 'anthropic/claude-sonnet-4-5', + instructions: 'x', + subAgents: { agents: [{ agentId: childAgentId }] }, + } as AgentJsonConfig, + }); + agentRepository.find.mockResolvedValue([parentA, parentB]); + + await service.removeSubAgentFromParents(childAgentId, projectId); + + expect(agentRepository.save).toHaveBeenCalledTimes(2); + expect(runtimeCacheService.clearRuntimes).toHaveBeenCalledWith('parent-a'); + expect(runtimeCacheService.clearRuntimes).toHaveBeenCalledWith('parent-b'); + }); +}); diff --git a/packages/cli/src/modules/agents/sub-agents/__tests__/sub-agent-foreground-runner.test.ts b/packages/cli/src/modules/agents/sub-agents/__tests__/sub-agent-foreground-runner.test.ts index 387f94961c6..81c3308792e 100644 --- a/packages/cli/src/modules/agents/sub-agents/__tests__/sub-agent-foreground-runner.test.ts +++ b/packages/cli/src/modules/agents/sub-agents/__tests__/sub-agent-foreground-runner.test.ts @@ -11,6 +11,7 @@ import type { SubAgentSpawnRequest, } from '@n8n/api-types'; import type { Logger } from '@n8n/backend-common'; +import type { User } from '@n8n/db'; import { Container } from '@n8n/di'; import type { Mocked } from 'vitest'; import { mock } from 'vitest-mock-extended'; @@ -158,6 +159,7 @@ describe('SubAgentForegroundRunner', () => { skills: runtimeSource.skills, runtimeProfile: 'sub-agent', parentAgentIdForDelegation: undefined, + user: undefined, }); expect(childAgent.close).toHaveBeenCalledTimes(1); expect(childAgent.stream).toHaveBeenCalledWith( @@ -187,6 +189,20 @@ describe('SubAgentForegroundRunner', () => { ); }); + it('filters sub-agent tools by the delegating user access when the parent run has a user', async () => { + const user = mock({ id: 'user-1' }); + + await runner.runForeground(spawnRequest, { + projectId, + credentialProvider, + user, + }); + + expect(reconstructionService.reconstructFromResolvedSource).toHaveBeenCalledWith( + expect.objectContaining({ user }), + ); + }); + it('inherits the parent resource id as the child memory scope when provided', async () => { const result = await runner.runForeground( { ...spawnRequest, parentResourceId: 'draft-chat:user-1' }, diff --git a/packages/cli/src/modules/agents/sub-agents/__tests__/sub-agent-source-resolver.test.ts b/packages/cli/src/modules/agents/sub-agents/__tests__/sub-agent-source-resolver.test.ts index 96c8f68394a..d3466c66488 100644 --- a/packages/cli/src/modules/agents/sub-agents/__tests__/sub-agent-source-resolver.test.ts +++ b/packages/cli/src/modules/agents/sub-agents/__tests__/sub-agent-source-resolver.test.ts @@ -39,19 +39,6 @@ const customToolDescriptor: ToolDescriptor = { providerOptions: null, }; -function makeAgent(overrides: Partial = {}): Agent { - return { - id: agentId, - projectId, - versionId, - schema: runnableConfig, - integrations: [], - tools: {}, - skills: {}, - ...overrides, - } as unknown as Agent; -} - function makeAgentHistory(overrides: Partial = {}): AgentHistory { return { agentId, @@ -63,6 +50,21 @@ function makeAgentHistory(overrides: Partial = {}): AgentHistory { } as unknown as AgentHistory; } +function makeAgent(overrides: Partial = {}): Agent { + return { + id: agentId, + projectId, + versionId, + schema: runnableConfig, + integrations: [], + tools: {}, + skills: {}, + activeVersionId: versionId, + activeVersion: makeAgentHistory(), + ...overrides, + } as unknown as Agent; +} + describe('SubAgentSourceResolver', () => { let agentRepository: Mocked; let agentHistoryRepository: Mocked; @@ -75,17 +77,14 @@ describe('SubAgentSourceResolver', () => { resolver = new SubAgentSourceResolver(agentRepository, agentHistoryRepository); }); - it('resolves a saved draft n8n agent in the same project', async () => { + it('resolves the current published version of a saved n8n agent when no version is pinned', async () => { agentRepository.findByIdAndProjectId.mockResolvedValue(makeAgent()); await expect(resolver.resolveForRuntime({ agentId }, { projectId })).resolves.toMatchObject({ source: { sourceId: agentId, versionId, - config: { - ...runnableConfig, - integrations: [], - }, + config: runnableConfig, }, }); }); @@ -105,22 +104,32 @@ describe('SubAgentSourceResolver', () => { }); }); - it('resolves runtime assets for saved n8n agent drafts', async () => { + it('resolves runtime assets from the published version, not the draft', async () => { agentRepository.findByIdAndProjectId.mockResolvedValue( makeAgent({ + // Draft-only assets that must NOT be used for an unpinned delegation. tools: { - tool_1: { - code: 'return input;', - descriptor: customToolDescriptor, + draft_only_tool: { + code: 'return "draft";', + descriptor: { ...customToolDescriptor, name: 'draft_only_tool' }, }, }, - skills: { - skill_1: { - name: 'Skill 1', - description: 'Helps with tests', - instructions: 'Skill body', + skills: {}, + activeVersion: makeAgentHistory({ + tools: { + tool_1: { + code: 'return input;', + descriptor: customToolDescriptor, + }, }, - }, + skills: { + skill_1: { + name: 'Skill 1', + description: 'Helps with tests', + instructions: 'Skill body', + }, + }, + }), }), ); @@ -152,11 +161,23 @@ describe('SubAgentSourceResolver', () => { ); }); - it('rejects n8n agents with no config', async () => { - agentRepository.findByIdAndProjectId.mockResolvedValue(makeAgent({ schema: null })); + it('rejects a sub-agent with no published version when no version is pinned', async () => { + agentRepository.findByIdAndProjectId.mockResolvedValue( + makeAgent({ activeVersionId: null, activeVersion: null }), + ); await expect(resolver.resolveForRuntime({ agentId }, { projectId })).rejects.toThrow( - `Agent "${agentId}" has no config`, + `Sub-agent "${agentId}" is not published`, + ); + }); + + it('rejects a sub-agent whose published version has no schema', async () => { + agentRepository.findByIdAndProjectId.mockResolvedValue( + makeAgent({ activeVersion: makeAgentHistory({ schema: null }) }), + ); + + await expect(resolver.resolveForRuntime({ agentId }, { projectId })).rejects.toThrow( + `Sub-agent "${agentId}" is not published`, ); }); @@ -171,7 +192,9 @@ describe('SubAgentSourceResolver', () => { it('rejects a resolved config that is not runnable', async () => { const { credential: _credential, ...invalidConfig } = runnableConfig; - agentRepository.findByIdAndProjectId.mockResolvedValue(makeAgent({ schema: invalidConfig })); + agentRepository.findByIdAndProjectId.mockResolvedValue( + makeAgent({ activeVersion: makeAgentHistory({ schema: invalidConfig }) }), + ); await expect(resolver.resolveForRuntime({ agentId }, { projectId })).rejects.toThrow( 'Invalid sub-agent config', diff --git a/packages/cli/src/modules/agents/sub-agents/sub-agent-cleanup.service.ts b/packages/cli/src/modules/agents/sub-agents/sub-agent-cleanup.service.ts new file mode 100644 index 00000000000..95cfcfe3b9c --- /dev/null +++ b/packages/cli/src/modules/agents/sub-agents/sub-agent-cleanup.service.ts @@ -0,0 +1,51 @@ +import { Logger } from '@n8n/backend-common'; +import { Service } from '@n8n/di'; + +import { AgentRuntimeCacheService } from '../agent-runtime-cache.service'; +import { AgentRepository } from '../repositories/agent.repository'; +import { markAgentDraftDirty } from '../utils/agent-draft.utils'; + +@Service() +export class SubAgentCleanupService { + constructor( + private readonly logger: Logger, + private readonly agentRepository: AgentRepository, + private readonly runtimeCacheService: AgentRuntimeCacheService, + ) {} + + /** + * Remove a sub-agent reference from every parent agent's draft config. + * Called when the sub-agent is deleted or unpublished, so a parent never + * keeps delegating to (or silently reactivates delegation to) an agent + * that is no longer available as a sub-agent. Sub-agents are project- + * scoped, so only the child's own project needs scanning. + */ + async removeSubAgentFromParents(childAgentId: string, projectId: string): Promise { + const agents = await this.agentRepository.find({ where: { projectId } }); + + for (const parent of agents) { + if (parent.id === childAgentId) continue; + + const { schema } = parent; + const configuredAgents = schema?.subAgents?.agents; + if (!schema || !configuredAgents?.some((ref) => ref.agentId === childAgentId)) continue; + + parent.schema = { + ...schema, + subAgents: { + ...schema.subAgents, + agents: configuredAgents.filter((ref) => ref.agentId !== childAgentId), + }, + }; + + markAgentDraftDirty(parent); + await this.agentRepository.save(parent); + this.runtimeCacheService.clearRuntimes(parent.id); + + this.logger.debug('Removed sub-agent reference from parent agent', { + childAgentId, + parentId: parent.id, + }); + } + } +} diff --git a/packages/cli/src/modules/agents/sub-agents/sub-agent-foreground-runner.ts b/packages/cli/src/modules/agents/sub-agents/sub-agent-foreground-runner.ts index 962e4bff8c4..1b4f0f5fa5e 100644 --- a/packages/cli/src/modules/agents/sub-agents/sub-agent-foreground-runner.ts +++ b/packages/cli/src/modules/agents/sub-agents/sub-agent-foreground-runner.ts @@ -10,6 +10,7 @@ import { } from '@n8n/agents'; import type { ResolvedSubAgentSource, SubAgentSpawnRequest } from '@n8n/api-types'; import { Logger } from '@n8n/backend-common'; +import type { User } from '@n8n/db'; import { Container, Service } from '@n8n/di'; import { UserError } from 'n8n-workflow'; import { v4 as uuid } from 'uuid'; @@ -29,6 +30,12 @@ export interface SubAgentForegroundRunContext { executionCounter?: AgentExecutionCounter; /** Parent run's abort signal — cancelling the parent cancels this child. */ abortSignal?: AbortSignal; + /** + * Interactive n8n user of the delegating parent run; used to filter the + * sub-agent's node/workflow tools by their access. Absent when the parent + * is a published/integration run. + */ + user?: User; } export interface SubAgentForegroundResult { @@ -91,6 +98,7 @@ export class SubAgentForegroundRunner { skills: runtimeSource.skills, runtimeProfile: 'sub-agent', parentAgentIdForDelegation: context.parentAgentId, + user: context.user, }); // Abort the child when the parent run is cancelled. diff --git a/packages/cli/src/modules/agents/sub-agents/sub-agent-source-resolver.ts b/packages/cli/src/modules/agents/sub-agents/sub-agent-source-resolver.ts index 9c9ea43edfb..c57702fa7ec 100644 --- a/packages/cli/src/modules/agents/sub-agents/sub-agent-source-resolver.ts +++ b/packages/cli/src/modules/agents/sub-agents/sub-agent-source-resolver.ts @@ -13,7 +13,6 @@ import { NotFoundError } from '@/errors/response-errors/not-found.error'; import type { AgentHistory } from '../entities/agent-history.entity'; import type { Agent } from '../entities/agent.entity'; -import { composeJsonConfig } from '../json-config/agent-config-composition'; import { AgentHistoryRepository } from '../repositories/agent-history.repository'; import { AgentRepository } from '../repositories/agent.repository'; @@ -36,8 +35,8 @@ export class SubAgentSourceResolver { ) {} /** - * Resolve a saved n8n agent (its current draft, or a pinned published - * version) into a runnable config plus its tool/skill assets. + * Resolve a saved n8n agent (its current published version, or a pinned + * specific version) into a runnable config plus its tool/skill assets. */ async resolveForRuntime( source: SubAgentSource, @@ -77,18 +76,23 @@ export class SubAgentSourceResolver { }; } - const config = composeJsonConfig(agent); - if (!config) { - throw new UserError(`Agent "${source.agentId}" has no config`); + // No pinned version: resolve the child's current published version at + // delegation time, not its draft. Reading `agent.activeVersion` fresh + // (rather than trusting a version baked into the parent's cached + // runtime) means a re-publish of the child takes effect on the very + // next delegation, without needing to clear the parent's cache. + const publishedVersion = agent.activeVersion; + if (!agent.activeVersionId || !publishedVersion?.schema) { + throw new UserError(`Sub-agent "${source.agentId}" is not published`); } return { source: { sourceId: source.agentId, - versionId: agent.versionId ?? undefined, - config: this.toRunnableConfig(config), + versionId: agent.activeVersionId, + config: this.toRunnableConfig(publishedVersion.schema), }, - ...getAgentRuntimeAssets(agent), + ...getAgentRuntimeAssets(publishedVersion), }; } diff --git a/packages/cli/src/modules/mcp/mcp-protected-resource.ts b/packages/cli/src/modules/mcp/mcp-protected-resource.ts index b072a1817cd..0cfef583c53 100644 --- a/packages/cli/src/modules/mcp/mcp-protected-resource.ts +++ b/packages/cli/src/modules/mcp/mcp-protected-resource.ts @@ -3,6 +3,7 @@ import { Service } from '@n8n/di'; import { McpSettingsService } from './mcp.settings.service'; import type { ProtectedResource } from '@/services/protected-resource.registry'; import { UrlService } from '@/services/url.service'; +import type { User } from '@n8n/db'; export const INSTANCE_MCP_RESOURCE_ID = 'instance-mcp'; @@ -62,4 +63,11 @@ export class McpProtectedResource implements ProtectedResource { async getAllowedRedirectUris(): Promise { return await this.mcpSettingsService.getAllowedRedirectUris(); } + + async authorize(_user: User): Promise { + // The instance MCP server has no per-user authorization rule: any + // authenticated user may access it while the server is enabled, and all + // users are denied when it is disabled. + return await this.mcpSettingsService.getEnabled(); + } } diff --git a/packages/cli/src/modules/oauth-server/__tests__/oauth-consent.controller.api.test.ts b/packages/cli/src/modules/oauth-server/__tests__/oauth-consent.controller.api.test.ts index 707f4d30d70..f3d9b7d86c7 100644 --- a/packages/cli/src/modules/oauth-server/__tests__/oauth-consent.controller.api.test.ts +++ b/packages/cli/src/modules/oauth-server/__tests__/oauth-consent.controller.api.test.ts @@ -80,6 +80,7 @@ describe('GET /rest/consent/details', () => { displayName: 'My Named Workflow', getResourceUrl: () => resourceUrl, getAudiences: () => [resourceUrl], + authorize: async () => true, scopes: [], }); diff --git a/packages/cli/src/modules/oauth-server/__tests__/oauth-consent.service.test.ts b/packages/cli/src/modules/oauth-server/__tests__/oauth-consent.service.test.ts index 4bc65613c84..cd1f1c660ee 100644 --- a/packages/cli/src/modules/oauth-server/__tests__/oauth-consent.service.test.ts +++ b/packages/cli/src/modules/oauth-server/__tests__/oauth-consent.service.test.ts @@ -14,6 +14,8 @@ import { type ProtectedResource, } from '@/services/protected-resource.registry'; import { UrlService } from '@/services/url.service'; +import type { User } from '@n8n/db'; +import { ForbiddenError } from '@/errors/response-errors/forbidden.error'; const issuer = 'https://n8n.example.com'; @@ -71,7 +73,7 @@ describe('OAuthConsentService', () => { oauthSessionService.verifySession.mockReturnValue(sessionPayload); oauthClientRepository.findOne.mockResolvedValue(client); - const result = await service.getConsentDetails(sessionToken); + const result = await service.getConsentDetails(sessionToken, mock({ id: 'user-id' })); expect(result).toEqual({ ok: true, @@ -98,7 +100,7 @@ describe('OAuthConsentService', () => { oauthSessionService.verifySession.mockReturnValue(sessionPayload); oauthClientRepository.findOne.mockResolvedValue(null); - const result = await service.getConsentDetails(sessionToken); + const result = await service.getConsentDetails(sessionToken, mock({ id: 'user-id' })); expect(result).toBeNull(); }); @@ -110,7 +112,7 @@ describe('OAuthConsentService', () => { throw new Error('Invalid session'); }); - const result = await service.getConsentDetails(sessionToken); + const result = await service.getConsentDetails(sessionToken, mock({ id: 'user-id' })); expect(result).toBeNull(); expect(logger.error).toHaveBeenCalledWith('Error getting consent details', { @@ -134,7 +136,7 @@ describe('OAuthConsentService', () => { oauthSessionService.verifySession.mockReturnValue(sessionPayload); oauthClientRepository.findOne.mockResolvedValue(client); - const result = await service.getConsentDetails(sessionToken); + const result = await service.getConsentDetails(sessionToken, mock({ id: 'user-id' })); expect(result).toEqual({ ok: true, @@ -158,10 +160,10 @@ describe('OAuthConsentService', () => { oauthSessionService.verifySession.mockReturnValue(sessionPayload); oauthClientRepository.findOne.mockResolvedValue(client); protectedResourceRegistry.getByResourceUrl.mockResolvedValue( - mock({ displayName: 'My Workflow' }), + mock({ displayName: 'My Workflow', authorize: async () => true }), ); - const result = await service.getConsentDetails(sessionToken); + const result = await service.getConsentDetails(sessionToken, mock({ id: 'user-id' })); expect(result).toEqual({ ok: true, @@ -189,10 +191,14 @@ describe('OAuthConsentService', () => { oauthSessionService.verifySession.mockReturnValue(sessionPayload); oauthClientRepository.findOne.mockResolvedValue(client); protectedResourceRegistry.getByResourceUrl.mockResolvedValue( - mock({ id: 'instance-mcp', displayName: undefined }), + mock({ + id: 'instance-mcp', + displayName: undefined, + authorize: async () => true, + }), ); - const result = await service.getConsentDetails(sessionToken); + const result = await service.getConsentDetails(sessionToken, mock({ id: 'user-id' })); expect(result).toEqual({ ok: true, @@ -218,7 +224,7 @@ describe('OAuthConsentService', () => { oauthClientRepository.findOne.mockResolvedValue(client); protectedResourceRegistry.getByResourceUrl.mockResolvedValue(undefined); - const result = await service.getConsentDetails(sessionToken); + const result = await service.getConsentDetails(sessionToken, mock({ id: 'user-id' })); expect(result).toEqual({ ok: false, reason: 'resource_unavailable' }); }); @@ -237,7 +243,11 @@ describe('OAuthConsentService', () => { oauthSessionService.verifySession.mockReturnValue(sessionPayload); - const result = await service.handleConsentDecision(sessionToken, userId, false); + const result = await service.handleConsentDecision( + sessionToken, + mock({ id: userId }), + false, + ); expect(result.redirectUrl).toContain('error=access_denied'); expect(result.redirectUrl).toContain( @@ -267,8 +277,15 @@ describe('OAuthConsentService', () => { oauthSessionService.verifySession.mockReturnValue(sessionPayload); userConsentRepository.upsert.mockResolvedValue(mock()); authorizationCodeService.createAuthorizationCode.mockResolvedValue(authCode); + protectedResourceRegistry.getByResourceUrl.mockResolvedValue( + mock({ authorize: async () => true }), + ); - const result = await service.handleConsentDecision(sessionToken, userId, true); + const result = await service.handleConsentDecision( + sessionToken, + mock({ id: userId }), + true, + ); expect(result.redirectUrl).toContain('code=generated-auth-code'); expect(result.redirectUrl).toContain('state=state-xyz'); @@ -295,6 +312,29 @@ describe('OAuthConsentService', () => { }); }); + it('should throw ForbiddenError when the user is not authorized for the resource', async () => { + const sessionToken = 'valid-session-token'; + const sessionPayload = { + clientId: 'client-123', + redirectUri: 'https://example.com/callback', + codeChallenge: 'challenge-abc', + state: 'state-xyz', + resource: 'https://n8n.example.com/mcp-server/http', + }; + + oauthSessionService.verifySession.mockReturnValue(sessionPayload); + protectedResourceRegistry.getByResourceUrl.mockResolvedValue( + mock({ authorize: async () => false }), + ); + + await expect( + service.handleConsentDecision(sessionToken, mock({ id: 'user-123' }), true), + ).rejects.toThrow(ForbiddenError); + + expect(userConsentRepository.upsert).not.toHaveBeenCalled(); + expect(authorizationCodeService.createAuthorizationCode).not.toHaveBeenCalled(); + }); + it('should handle approval without state parameter', async () => { const sessionToken = 'valid-session-token'; const userId = 'user-123'; @@ -310,7 +350,11 @@ describe('OAuthConsentService', () => { userConsentRepository.upsert.mockResolvedValue(mock()); authorizationCodeService.createAuthorizationCode.mockResolvedValue(authCode); - const result = await service.handleConsentDecision(sessionToken, userId, true); + const result = await service.handleConsentDecision( + sessionToken, + mock({ id: userId }), + true, + ); expect(result.redirectUrl).toContain('code=generated-auth-code'); expect(result.redirectUrl).not.toContain('state='); @@ -340,9 +384,9 @@ describe('OAuthConsentService', () => { authorizationCodeService.createAuthorizationCode.mockResolvedValue(authCode); // First authorization - await service.handleConsentDecision(sessionToken, userId, true); + await service.handleConsentDecision(sessionToken, mock({ id: userId }), true); // Re-authorization with same userId + clientId should not throw - await service.handleConsentDecision(sessionToken, userId, true); + await service.handleConsentDecision(sessionToken, mock({ id: userId }), true); expect(userConsentRepository.upsert).toHaveBeenCalledTimes(2); expect(userConsentRepository.upsert).toHaveBeenCalledWith( @@ -363,9 +407,9 @@ describe('OAuthConsentService', () => { throw new Error('Invalid session'); }); - await expect(service.handleConsentDecision(sessionToken, userId, true)).rejects.toThrow( - 'Invalid or expired session', - ); + await expect( + service.handleConsentDecision(sessionToken, mock({ id: userId }), true), + ).rejects.toThrow('Invalid or expired session'); }); it('should handle denial without state parameter', async () => { @@ -380,7 +424,11 @@ describe('OAuthConsentService', () => { oauthSessionService.verifySession.mockReturnValue(sessionPayload); - const result = await service.handleConsentDecision(sessionToken, userId, false); + const result = await service.handleConsentDecision( + sessionToken, + mock({ id: userId }), + false, + ); expect(result.redirectUrl).toContain('error=access_denied'); expect(result.redirectUrl).not.toContain('state='); diff --git a/packages/cli/src/modules/oauth-server/__tests__/oauth-server.service.test.ts b/packages/cli/src/modules/oauth-server/__tests__/oauth-server.service.test.ts index 69b5eb327fb..0768622c029 100644 --- a/packages/cli/src/modules/oauth-server/__tests__/oauth-server.service.test.ts +++ b/packages/cli/src/modules/oauth-server/__tests__/oauth-server.service.test.ts @@ -49,6 +49,7 @@ describe('OAuthServerService', () => { scopes: SUPPORTED_SCOPES, isDefault: true, getAllowedRedirectUris, + authorize: async () => true, }); service = new OAuthServerService( @@ -1049,6 +1050,7 @@ describe('OAuthServerService', () => { getAudiences: () => [TEST_RESOURCE_URL, 'mcp-server-api'], scopes: SUPPORTED_SCOPES, isDefault: true, + authorize: async () => true, }); const secondResourceUrl = 'https://n8n.example.com/webhook/wf-1/mcp'; multiRegistry.register({ @@ -1056,6 +1058,7 @@ describe('OAuthServerService', () => { getResourceUrl: () => secondResourceUrl, getAudiences: () => [secondResourceUrl], scopes: [], + authorize: async () => true, }); const multiResourceService = new OAuthServerService( diff --git a/packages/cli/src/modules/oauth-server/__tests__/oauth-token.service.test.ts b/packages/cli/src/modules/oauth-server/__tests__/oauth-token.service.test.ts index 05770c92206..ca2c9f958fe 100644 --- a/packages/cli/src/modules/oauth-server/__tests__/oauth-token.service.test.ts +++ b/packages/cli/src/modules/oauth-server/__tests__/oauth-token.service.test.ts @@ -36,6 +36,7 @@ registry.register({ getAudiences: () => [TEST_RESOURCE_URL, LEGACY_AUDIENCE], scopes: [], isDefault: true, + authorize: async () => true, }); describe('OAuthTokenService', () => { @@ -426,6 +427,21 @@ describe('OAuthTokenService', () => { expect(result).toMatchObject({ user: null }); }); + + it('should deny when a resource-scoped audience cannot be resolved', async () => { + // Fail closed: the token carries an audience but no resource resolves for + // it (deleted, or a transient resolver failure the registry swallows), so + // the authorize gate cannot run and the token must be rejected. + const { accessToken } = service.generateTokenPair('user-123', 'client-456'); + + const result = await service.verifyOAuthAccessToken( + accessToken, + 'https://unregistered.example.com/mcp', + ); + + expect(result.user).toBeNull(); + expect(result.context?.reason).toBe('insufficient_scope'); + }); }); describe('revokeAccessToken', () => { @@ -502,12 +518,14 @@ describe('OAuthTokenService', () => { getResourceUrl: () => RESOURCE_A_URL, getAudiences: () => [RESOURCE_A_URL, LEGACY_AUDIENCE], scopes: [], + authorize: async () => true, isDefault: true, }); multiResourceRegistry.register({ id: 'workflow-trigger', getResourceUrl: () => RESOURCE_B_URL, getAudiences: () => [RESOURCE_B_URL], + authorize: async () => true, scopes: [], }); diff --git a/packages/cli/src/modules/oauth-server/__tests__/oauth.controller.test.ts b/packages/cli/src/modules/oauth-server/__tests__/oauth.controller.test.ts index 3cb31b2ebcc..82fe2b2532d 100644 --- a/packages/cli/src/modules/oauth-server/__tests__/oauth.controller.test.ts +++ b/packages/cli/src/modules/oauth-server/__tests__/oauth.controller.test.ts @@ -44,6 +44,7 @@ const resource = (scopes: string[]): ProtectedResource => ({ id: 'instance-mcp', getResourceUrl: () => 'https://n8n.test/mcp-server/http', getAudiences: () => ['https://n8n.test/mcp-server/http'], + authorize: async () => true, scopes, }); diff --git a/packages/cli/src/modules/oauth-server/__tests__/workflow-mcp-trigger-authorization.api.test.ts b/packages/cli/src/modules/oauth-server/__tests__/workflow-mcp-trigger-authorization.api.test.ts new file mode 100644 index 00000000000..fa3c9c5bec0 --- /dev/null +++ b/packages/cli/src/modules/oauth-server/__tests__/workflow-mcp-trigger-authorization.api.test.ts @@ -0,0 +1,224 @@ +import { + createWorkflowWithHistory, + setActiveVersion, + shareWorkflowWithUsers, + testDb, +} from '@n8n/backend-test-utils'; +import { GlobalConfig } from '@n8n/config'; +import type { User } from '@n8n/db'; +import { WebhookRepository } from '@n8n/db'; +import { Container } from '@n8n/di'; +import type { INode } from 'n8n-workflow'; +import { randomUUID } from 'node:crypto'; + +import { createOwner, createMember, createAdmin } from '@test-integration/db/users'; +import { setupTestServer } from '@test-integration/utils'; + +import { MCP_TRIGGER_NODE_TYPE } from '@/constants'; +import { OAuthTokenService } from '@/modules/oauth-server/oauth-token.service'; +import { CacheService } from '@/services/cache/cache.service'; +import { UrlService } from '@/services/url.service'; +import { JwtService } from '@/services/jwt.service'; + +import { OAuthClientRepository } from '../database/repositories/oauth-client.repository'; +import type { OAuthSessionPayload } from '../oauth-session.service'; + +/** + * Authorization for OAuth-protected MCP Server Triggers. A workflow whose MCP + * trigger uses `n8nOAuth2` is a protected resource; only users with + * `workflow:execute` on that workflow may mint a token for it or invoke it. + * Three actors exercise the access-control system: no access (denied), execute + * via a project/resource role, and execute via a global role. + */ +const testServer = setupTestServer({ endpointGroups: ['mcp'], modules: ['oauth-server', 'mcp'] }); + +let owner: User; +let memberNoAccess: User; +let memberWithProjectAccess: User; +let adminWithGlobalAccess: User; + +let tokenService: OAuthTokenService; +let jwtService: JwtService; +let oauthClientRepository: OAuthClientRepository; +let mcpEndpoint: string; + +const webhookBaseUrl = () => Container.get(UrlService).getWebhookBaseUrl().replace(/\/$/, ''); +const productionResourceUrl = (webhookPath: string) => + `${webhookBaseUrl()}/${mcpEndpoint}/${webhookPath}`; + +const mcpTriggerNode = (requireExecuteAccess?: boolean): INode => ({ + id: randomUUID(), + name: 'MCP Server Trigger', + type: MCP_TRIGGER_NODE_TYPE, + typeVersion: 2, + position: [0, 0], + parameters: { + path: 'unused', + authentication: 'n8nOAuth2', + // Omitted when undefined so the "absent param ⇒ secure default" path is exercised too. + ...(requireExecuteAccess === undefined ? {} : { requireExecuteAccess }), + }, +}); + +/** + * Create an active, published workflow whose MCP trigger is protected with + * n8n OAuth2, plus the production webhook row the resolver matches on. Returns + * the canonical resource URL the OAuth server binds tokens and consent to. + */ +const createProtectedWorkflow = async (workflowName: string, requireExecuteAccess?: boolean) => { + const node = mcpTriggerNode(requireExecuteAccess); + const webhookPath = randomUUID(); + const workflow = await createWorkflowWithHistory( + { name: workflowName, active: true, nodes: [node] }, + owner, + ); + await setActiveVersion(workflow.id, workflow.versionId); + await Container.get(WebhookRepository).insert({ + workflowId: workflow.id, + webhookPath, + method: 'POST', + node: node.name, + }); + return { workflow, resourceUrl: productionResourceUrl(webhookPath) }; +}; + +const registerClient = async () => + ( + await oauthClientRepository.save({ + id: `client-${randomUUID()}`, + name: 'Test OAuth Client', + redirectUris: ['https://example.com/callback'], + grantTypes: ['authorization_code'], + tokenEndpointAuthMethod: 'none', + }) + ).id; + +const mintAccessToken = async (userId: string, resourceUrl: string) => { + const clientId = await registerClient(); + const pair = tokenService.generateTokenPair(userId, clientId, resourceUrl); + await tokenService.saveTokenPair(pair.accessToken, pair.refreshToken, clientId, userId); + return pair.accessToken; +}; + +const sessionTokenFor = async (resourceUrl: string) => { + const clientId = await registerClient(); + const payload: OAuthSessionPayload = { + clientId, + redirectUri: 'https://example.com/callback', + codeChallenge: 'test-challenge-string-that-is-long-enough', + state: 'test-state', + resource: resourceUrl, + }; + return jwtService.sign(payload, { expiresIn: '10m' }); +}; + +beforeAll(async () => { + owner = await createOwner(); + memberNoAccess = await createMember(); + memberWithProjectAccess = await createMember(); + adminWithGlobalAccess = await createAdmin(); + + tokenService = Container.get(OAuthTokenService); + jwtService = Container.get(JwtService); + oauthClientRepository = Container.get(OAuthClientRepository); + mcpEndpoint = Container.get(GlobalConfig).endpoints.mcp; +}); + +afterEach(async () => { + await Container.get(CacheService).reset(); + await testDb.truncate([ + 'AccessToken', + 'RefreshToken', + 'AuthorizationCode', + 'OAuthClient', + 'UserConsent', + 'WebhookEntity', + 'SharedWorkflow', + 'WorkflowEntity', + 'WorkflowHistory', + ]); +}); + +describe('runtime gate: verifyOAuthAccessToken enforces workflow:execute', () => { + test('denies a user without execute access on the workflow', async () => { + const { resourceUrl } = await createProtectedWorkflow('Owner protected workflow'); + const token = await mintAccessToken(memberNoAccess.id, resourceUrl); + + const result = await tokenService.verifyOAuthAccessToken(token, resourceUrl); + + expect(result.user).toBeNull(); + expect(result.context?.reason).toBe('insufficient_scope'); + }); + + test('allows a user granted execute via a project role', async () => { + const { workflow, resourceUrl } = await createProtectedWorkflow('Owner protected workflow'); + await shareWorkflowWithUsers(workflow, [memberWithProjectAccess]); + const token = await mintAccessToken(memberWithProjectAccess.id, resourceUrl); + + const result = await tokenService.verifyOAuthAccessToken(token, resourceUrl); + + expect(result.user?.id).toBe(memberWithProjectAccess.id); + }); + + test('allows a user granted execute via a global role', async () => { + const { resourceUrl } = await createProtectedWorkflow('Owner protected workflow'); + const token = await mintAccessToken(adminWithGlobalAccess.id, resourceUrl); + + const result = await tokenService.verifyOAuthAccessToken(token, resourceUrl); + + expect(result.user?.id).toBe(adminWithGlobalAccess.id); + }); + + test('allows any authenticated user when require-execute is turned off', async () => { + // requireExecuteAccess:false is the opt-out — the same no-access member who is denied + // by default must now be allowed, proving the resolver reads the per-trigger setting. + const { resourceUrl } = await createProtectedWorkflow('Open MCP server', false); + const token = await mintAccessToken(memberNoAccess.id, resourceUrl); + + const result = await tokenService.verifyOAuthAccessToken(token, resourceUrl); + + expect(result.user?.id).toBe(memberNoAccess.id); + }); +}); + +describe('consent gate: workflow name and authorization code require execute access', () => { + test('does not reveal the workflow name to a user without execute access', async () => { + const workflowName = `Owner protected workflow ${randomUUID()}`; + const { resourceUrl } = await createProtectedWorkflow(workflowName); + const sessionToken = await sessionTokenFor(resourceUrl); + + const response = await testServer + .authAgentFor(memberNoAccess) + .get('/consent/details') + .set('Cookie', `n8n-oauth-session=${sessionToken}`); + + expect(JSON.stringify(response.body)).not.toContain(workflowName); + }); + + test('reveals the workflow name to the owner', async () => { + const workflowName = `Owner protected workflow ${randomUUID()}`; + const { resourceUrl } = await createProtectedWorkflow(workflowName); + const sessionToken = await sessionTokenFor(resourceUrl); + + const response = await testServer + .authAgentFor(owner) + .get('/consent/details') + .set('Cookie', `n8n-oauth-session=${sessionToken}`); + + expect(response.body.data.resourceName).toBe(workflowName); + }); + + test('does not mint an authorization code for a user without execute access', async () => { + const { resourceUrl } = await createProtectedWorkflow('Owner protected workflow'); + const sessionToken = await sessionTokenFor(resourceUrl); + + const response = await testServer + .authAgentFor(memberNoAccess) + .post('/consent/approve') + .set('Cookie', `n8n-oauth-session=${sessionToken}`) + .send({ approved: true }) + .expect(403); + + expect(String(response.body.data?.redirectUrl ?? '')).not.toContain('code='); + }); +}); diff --git a/packages/cli/src/modules/oauth-server/oauth-consent.controller.ts b/packages/cli/src/modules/oauth-server/oauth-consent.controller.ts index f1f764ce9c6..7f434bbbb8f 100644 --- a/packages/cli/src/modules/oauth-server/oauth-consent.controller.ts +++ b/packages/cli/src/modules/oauth-server/oauth-consent.controller.ts @@ -6,6 +6,7 @@ import type { Response } from 'express'; import { ApproveConsentRequestDto } from './dto/approve-consent-request.dto'; import { OAuthConsentService } from './oauth-consent.service'; import { OAuthSessionService } from './oauth-session.service'; +import { ForbiddenError } from '@/errors/response-errors/forbidden.error'; @RestController('/consent') export class OAuthConsentController { @@ -21,7 +22,7 @@ export class OAuthConsentController { const sessionToken = this.getAndValidateSessionToken(req, res); if (!sessionToken) return; - const consentDetails = await this.consentService.getConsentDetails(sessionToken); + const consentDetails = await this.consentService.getConsentDetails(sessionToken, req.user); if (!consentDetails) { this.sendInvalidSessionError(res, true); @@ -30,7 +31,15 @@ export class OAuthConsentController { if (!consentDetails.ok) { this.oauthSessionService.clearSession(res); - this.sendErrorResponse(res, 422, 'Authorization target is no longer available'); + if (consentDetails.reason === 'forbidden') { + this.sendErrorResponse( + res, + 403, + 'You do not have sufficient permissions to authorize this request', + ); + } else { + this.sendErrorResponse(res, 422, 'Authorization target is no longer available'); + } return; } @@ -61,7 +70,7 @@ export class OAuthConsentController { const result = await this.consentService.handleConsentDecision( sessionToken, - req.user.id, + req.user, payload.approved, ); @@ -76,6 +85,14 @@ export class OAuthConsentController { } catch (error) { this.logger.error('Failed to process consent', { error }); this.oauthSessionService.clearSession(res); + if (error instanceof ForbiddenError) { + this.sendErrorResponse( + res, + 403, + 'You do not have sufficient permissions to authorize this request', + ); + return; + } const message = error instanceof Error ? error.message : 'Failed to process authorization'; this.sendErrorResponse(res, 500, message); } diff --git a/packages/cli/src/modules/oauth-server/oauth-consent.service.ts b/packages/cli/src/modules/oauth-server/oauth-consent.service.ts index f9be641eadb..af83b15557b 100644 --- a/packages/cli/src/modules/oauth-server/oauth-consent.service.ts +++ b/packages/cli/src/modules/oauth-server/oauth-consent.service.ts @@ -1,4 +1,5 @@ import { Logger } from '@n8n/backend-common'; +import type { User } from '@n8n/db'; import { Service } from '@n8n/di'; import { UserError } from 'n8n-workflow'; @@ -7,12 +8,14 @@ import { UserConsentRepository } from './database/repositories/oauth-user-consen import { OAuthAuthorizationCodeService } from './oauth-authorization-code.service'; import { OAuthSessionService, type OAuthSessionPayload } from './oauth-session.service'; import { OAuthHelpers } from './oauth.helpers'; +import { ForbiddenError } from '@/errors/response-errors/forbidden.error'; import { ProtectedResourceRegistry } from '@/services/protected-resource.registry'; import { UrlService } from '@/services/url.service'; type ConsentDetailsResult = | { ok: true; clientName: string; clientId: string; resourceName?: string; redirectUri?: string } - | { ok: false; reason: 'resource_unavailable' }; + | { ok: false; reason: 'resource_unavailable' } + | { ok: false; reason: 'forbidden' }; /** * Manages the consent flow for the shared OAuth server. @@ -34,7 +37,7 @@ export class OAuthConsentService { * Get consent details from session cookie * Verifies JWT session token and returns client information */ - async getConsentDetails(sessionToken: string): Promise { + async getConsentDetails(sessionToken: string, user: User): Promise { try { const sessionPayload = this.oauthSessionService.verifySession(sessionToken); @@ -55,6 +58,12 @@ export class OAuthConsentService { return { ok: false, reason: 'resource_unavailable' }; } + if (!(await resource.authorize(user))) + return { + ok: false, + reason: 'forbidden', + }; + return { ok: true, clientName: client.name, @@ -82,7 +91,7 @@ export class OAuthConsentService { */ async handleConsentDecision( sessionToken: string, - userId: string, + user: User, approved: boolean, ): Promise<{ redirectUrl: string }> { let sessionPayload: OAuthSessionPayload; @@ -105,15 +114,34 @@ export class OAuthConsentService { this.logger.info('Consent denied', { clientId: sessionPayload.clientId, - userId, + userId: user.id, }); return { redirectUrl }; } + if (sessionPayload.resource) { + const resource = await this.protectedResourceRegistry.getByResourceUrl( + sessionPayload.resource, + ); + + if (!resource) { + throw new UserError('Resource is not available for the requested authorization'); + } + + if (!(await resource.authorize(user))) { + this.logger.warn('User is not authorized for the requested resource', { + clientId: sessionPayload.clientId, + userId: user.id, + resourceUrl: sessionPayload.resource, + }); + throw new ForbiddenError('User is not authorized for the requested resource'); + } + } + await this.userConsentRepository.upsert( { - userId, + userId: user.id, clientId: sessionPayload.clientId, grantedAt: Date.now(), }, @@ -122,7 +150,7 @@ export class OAuthConsentService { const code = await this.authorizationCodeService.createAuthorizationCode( sessionPayload.clientId, - userId, + user.id, sessionPayload.redirectUri, sessionPayload.codeChallenge, sessionPayload.state, @@ -138,7 +166,7 @@ export class OAuthConsentService { this.logger.info('Consent approved', { clientId: sessionPayload.clientId, - userId, + userId: user.id, }); return { redirectUrl: successRedirectUrl }; diff --git a/packages/cli/src/modules/oauth-server/oauth-token.service.ts b/packages/cli/src/modules/oauth-server/oauth-token.service.ts index e9cc4f22603..a18384b09ad 100644 --- a/packages/cli/src/modules/oauth-server/oauth-token.service.ts +++ b/packages/cli/src/modules/oauth-server/oauth-token.service.ts @@ -14,6 +14,8 @@ import { AccessToken } from './database/entities/oauth-access-token.entity'; import { RefreshToken } from './database/entities/oauth-refresh-token.entity'; import { AccessTokenRepository } from './database/repositories/oauth-access-token.repository'; import { RefreshTokenRepository } from './database/repositories/oauth-refresh-token.repository'; +import type { ProtectedResource } from '@/services/protected-resource.registry'; +import { ProtectedResourceRegistry } from '@/services/protected-resource.registry'; import { AccessTokenNotFoundError, JWTVerificationError } from './oauth.errors'; import { JwtService } from '@/services/jwt.service'; @@ -21,7 +23,6 @@ import type { OAuthTokenVerifier, UserWithContext, } from '@/services/oauth-token-verifier-proxy.service'; -import { ProtectedResourceRegistry } from '@/services/protected-resource.registry'; /** * Manages the OAuth 2.1 token lifecycle for the shared OAuth server. @@ -168,10 +169,19 @@ export class OAuthTokenService implements OAuthTokenVerifier { } async verifyAccessToken(token: string, expectedAudience?: string): Promise { + return await this.verifyTokenWithAudiences( + token, + await this.getAllowedAudiences(expectedAudience), + ); + } + + private async verifyTokenWithAudiences( + token: string, + allowedAudiences: string[], + ): Promise { let decoded: unknown; try { - const allowedAudiences = await this.getAllowedAudiences(expectedAudience); decoded = this.verifyJwtWithAllowedAudiences(token, allowedAudiences); } catch (error) { throw new JWTVerificationError(); @@ -203,7 +213,19 @@ export class OAuthTokenService implements OAuthTokenVerifier { async verifyOAuthAccessToken(token: string, expectedAudience?: string): Promise { try { - const authInfo = await this.verifyAccessToken(token, expectedAudience); + const resource = await this.getResourceByAudience(expectedAudience); + + // Fail closed: a token bearing a resource-scoped audience whose resource + // can't be resolved (deleted, or a transient resolver failure the registry + // swallows to `undefined`) must NOT bypass the authorize gate below. + if (expectedAudience && !resource) { + return { user: null, context: { reason: 'insufficient_scope', auth_type: 'oauth' } }; + } + + const authInfo = await this.verifyTokenWithAudiences( + token, + this.audiencesForResource(resource, expectedAudience), + ); const userId = authInfo.extra && typeof authInfo.extra === 'object' @@ -222,6 +244,14 @@ export class OAuthTokenService implements OAuthTokenVerifier { return { user: null, context: { reason: 'user_not_found', auth_type: 'oauth' } }; } + if (resource && !(await resource.authorize(user))) { + this.logger.warn('OAuth token denied: user lacks execute access', { + userId: user.id, + expectedAudience, + }); + return { user: null, context: { reason: 'insufficient_scope', auth_type: 'oauth' } }; + } + return { user, authType: 'oauth' }; } catch (error) { const errorForSure = ensureError(error); @@ -282,8 +312,22 @@ export class OAuthTokenService implements OAuthTokenVerifier { * pre-RFC-8707 `mcp-server-api`) stay scoped to their own resource this way. */ private async getAllowedAudiences(expectedAudience?: string): Promise { + const resource = expectedAudience + ? await this.resourceRegistry.getByResourceUrl(expectedAudience) + : undefined; + return this.audiencesForResource(resource, expectedAudience); + } + + /** + * Derive the `aud` values accepted for a (possibly pre-resolved) resource. + * Single source of the audience-derivation rule shared by `getAllowedAudiences` + * and the resolve-once path in `verifyOAuthAccessToken`. + */ + private audiencesForResource( + resource: ProtectedResource | undefined, + expectedAudience?: string, + ): string[] { if (expectedAudience) { - const resource = await this.resourceRegistry.getByResourceUrl(expectedAudience); return resource ? resource.getAudiences() : [expectedAudience]; } @@ -293,6 +337,15 @@ export class OAuthTokenService implements OAuthTokenVerifier { return this.resourceRegistry.getAllAudiences(); } + private async getResourceByAudience( + expectedAudience?: string, + ): Promise { + if (!expectedAudience) { + return this.resourceRegistry.getDefaultResource(); + } + return await this.resourceRegistry.getByResourceUrl(expectedAudience); + } + // TODO: drop legacy audiences and the per-audience fallback once all legacy // tokens minted before n8n v2.19 have aged out (refresh-token lifespan). private verifyJwtWithAllowedAudiences(token: string, audiences: string[]): unknown { diff --git a/packages/cli/src/modules/oauth-server/protected-resource-resolvers/workflow-mcp-test-trigger-resource.resolver.ts b/packages/cli/src/modules/oauth-server/protected-resource-resolvers/workflow-mcp-test-trigger-resource.resolver.ts index 181c0ebb00f..ff71f80c562 100644 --- a/packages/cli/src/modules/oauth-server/protected-resource-resolvers/workflow-mcp-test-trigger-resource.resolver.ts +++ b/packages/cli/src/modules/oauth-server/protected-resource-resolvers/workflow-mcp-test-trigger-resource.resolver.ts @@ -2,6 +2,7 @@ import { MCP_TRIGGER_NODE_TYPE } from '@/constants'; import type { ProtectedResourceResolver } from '@/services/protected-resource.registry'; import { UrlService } from '@/services/url.service'; import { TestWebhookRegistrationsService } from '@/webhooks/test-webhook-registrations.service'; +import { WorkflowFinderService } from '@/workflows/workflow-finder.service'; import { Logger } from '@n8n/backend-common'; import { GlobalConfig } from '@n8n/config'; import { Service } from '@n8n/di'; @@ -12,6 +13,7 @@ import { trimSlashes, trimTrailingSlash, } from './utils'; +import { User } from '@n8n/db'; @Service() export class WorkflowMcpTestTriggerResourceResolver implements ProtectedResourceResolver { @@ -20,6 +22,7 @@ export class WorkflowMcpTestTriggerResourceResolver implements ProtectedResource private readonly registrations: TestWebhookRegistrationsService, private readonly urlService: UrlService, private readonly logger: Logger, + private readonly workflowFinderService: WorkflowFinderService, ) {} readonly id = 'workflow-mcp-test-trigger'; @@ -73,13 +76,26 @@ export class WorkflowMcpTestTriggerResourceResolver implements ProtectedResource !node.disabled && node.parameters.authentication === 'n8nOAuth2' ) { - const resourceUrl = `${trimTrailingSlash(this.urlService.getTestWebhookBaseUrl())}/${this.config.endpoints.mcpTest}/${path}`; + const resourceUrl = `${trimTrailingSlash(this.urlService.getWebhookBaseUrl())}/${this.config.endpoints.mcpTest}/${path}`; + const requireExecute = node.parameters.requireExecuteAccess !== false; return { id: 'workflow-mcp-test:' + workflowEntity.id, getResourceUrl: () => resourceUrl, getAudiences: () => [resourceUrl], scopes: WORKFLOW_MCP_TRIGGER_SCOPES, displayName: workflowEntity.name, + authorize: async (user: User) => { + if (requireExecute) { + return ( + await this.workflowFinderService.findWorkflowIdsWithScopeForUser( + [workflowEntity.id], + user, + ['workflow:execute'], + ) + ).has(workflowEntity.id); + } + return true; + }, }; } diff --git a/packages/cli/src/modules/oauth-server/protected-resource-resolvers/workflow-mcp-trigger-resource.resolver.ts b/packages/cli/src/modules/oauth-server/protected-resource-resolvers/workflow-mcp-trigger-resource.resolver.ts index e9ffeeb7584..c87c79c270c 100644 --- a/packages/cli/src/modules/oauth-server/protected-resource-resolvers/workflow-mcp-trigger-resource.resolver.ts +++ b/packages/cli/src/modules/oauth-server/protected-resource-resolvers/workflow-mcp-trigger-resource.resolver.ts @@ -2,9 +2,10 @@ import { MCP_TRIGGER_NODE_TYPE } from '@/constants'; import type { ProtectedResourceResolver } from '@/services/protected-resource.registry'; import { UrlService } from '@/services/url.service'; import { WebhookService } from '@/webhooks/webhook.service'; +import { WorkflowFinderService } from '@/workflows/workflow-finder.service'; import { Logger } from '@n8n/backend-common'; import { GlobalConfig } from '@n8n/config'; -import { WorkflowRepository } from '@n8n/db'; +import { User, WorkflowRepository } from '@n8n/db'; import { Service } from '@n8n/di'; import { @@ -22,6 +23,7 @@ export class WorkflowMcpTriggerResourceResolver implements ProtectedResourceReso private readonly workflowRepository: WorkflowRepository, private readonly urlService: UrlService, private readonly logger: Logger, + private readonly workflowFinderService: WorkflowFinderService, ) {} readonly id = 'workflow-mcp-trigger'; @@ -94,12 +96,25 @@ export class WorkflowMcpTriggerResourceResolver implements ProtectedResourceReso node.parameters.authentication === 'n8nOAuth2' ) { const resourceUrl = `${trimTrailingSlash(this.urlService.getWebhookBaseUrl())}/${this.config.endpoints.mcp}/${path}`; + const requireExecute = node.parameters.requireExecuteAccess !== false; return { id: 'workflow-mcp:' + workflow.id, getResourceUrl: () => resourceUrl, getAudiences: () => [resourceUrl], scopes: WORKFLOW_MCP_TRIGGER_SCOPES, displayName: workflow.name, + authorize: async (user: User) => { + if (requireExecute) { + return ( + await this.workflowFinderService.findWorkflowIdsWithScopeForUser( + [workflow.id], + user, + ['workflow:execute'], + ) + ).has(workflow.id); + } + return true; + }, }; } diff --git a/packages/cli/src/modules/provisioning.ee/__tests__/provisioning.service.ee.test.ts b/packages/cli/src/modules/provisioning.ee/__tests__/provisioning.service.ee.test.ts index 3f8ff29cd85..efa37a9324b 100644 --- a/packages/cli/src/modules/provisioning.ee/__tests__/provisioning.service.ee.test.ts +++ b/packages/cli/src/modules/provisioning.ee/__tests__/provisioning.service.ee.test.ts @@ -260,6 +260,19 @@ describe('ProvisioningService', () => { }); it('should provision the instance role for the user', async () => { + const user = mock({ role: { slug: 'global:member' } }); + const roleSlug = 'global:admin'; + roleRepository.findOneOrFail.mockResolvedValue( + mock({ slug: 'global:admin', roleType: 'global' }), + ); + provisioningService['isInstanceRoleProvisioningEnabled'] = vi.fn().mockResolvedValue(true); + + await provisioningService.provisionInstanceRoleForUser(user, roleSlug); + + expect(userService.changeUserRole).toHaveBeenCalledWith(user, { newRoleName: roleSlug }); + }); + + it('should not promote a non-owner user to global:owner', async () => { const user = mock({ role: { slug: 'global:member' } }); const roleSlug = 'global:owner'; roleRepository.findOneOrFail.mockResolvedValue( @@ -269,7 +282,12 @@ describe('ProvisioningService', () => { await provisioningService.provisionInstanceRoleForUser(user, roleSlug); - expect(userService.changeUserRole).toHaveBeenCalledWith(user, { newRoleName: roleSlug }); + expect(userService.changeUserRole).not.toHaveBeenCalled(); + expect(logger.warn).toHaveBeenCalledTimes(1); + expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining('global:owner'), { + userId: user.id, + roleSlug: 'global:owner', + }); }); it('should do nothing if the role has not changed', async () => { @@ -306,10 +324,10 @@ describe('ProvisioningService', () => { it('sends telemetry event', async () => { const user = mock({ id: 'user-123', role: { slug: 'global:member' } }); - const roleSlug = 'global:owner'; + const roleSlug = 'global:admin'; roleRepository.findOneOrFail.mockResolvedValue( - mock({ slug: 'global:owner', roleType: 'global' }), + mock({ slug: 'global:admin', roleType: 'global' }), ); provisioningService['isInstanceRoleProvisioningEnabled'] = vi.fn().mockResolvedValue(true); @@ -974,6 +992,31 @@ describe('ProvisioningService', () => { }); }); + it('should not promote a non-owner user to global:owner via expression mapping', async () => { + roleResolverService.resolveRoles.mockResolvedValue({ + instanceRole: { + role: 'global:owner', + matchedRuleId: 'rule-1', + expression: '{{ true }}', + isFallback: false, + }, + projectRoles: new Map(), + }); + roleRepository.findOneOrFail.mockResolvedValue( + mock({ slug: 'global:owner', roleType: 'global' }), + ); + + const context = { $claims: { role: 'owner' }, $provider: 'oidc' as const }; + + await provisioningService.provisionExpressionMappedRolesForUser(user, context); + + expect(userService.changeUserRole).not.toHaveBeenCalled(); + expect(logger.warn).toHaveBeenCalledWith( + expect.stringContaining('global:owner'), + expect.objectContaining({ userId: user.id }), + ); + }); + it('should not emit when expression mapping is disabled', async () => { provisioningService['isExpressionMappingEnabled'] = vi.fn().mockResolvedValue(false); diff --git a/packages/cli/src/modules/provisioning.ee/provisioning.service.ee.ts b/packages/cli/src/modules/provisioning.ee/provisioning.service.ee.ts index 4de0d4444f0..953e2b7e16f 100644 --- a/packages/cli/src/modules/provisioning.ee/provisioning.service.ee.ts +++ b/packages/cli/src/modules/provisioning.ee/provisioning.service.ee.ts @@ -14,6 +14,7 @@ import { } from '@n8n/db'; import { OnPubSubEvent } from '@n8n/decorators'; import { Service } from '@n8n/di'; +import { GLOBAL_OWNER_ROLE_SLUG } from '@n8n/permissions'; import { Not, In } from '@n8n/typeorm'; import { InstanceSettings } from 'n8n-core'; import { jsonParse } from 'n8n-workflow'; @@ -68,7 +69,7 @@ export class ProvisioningService { return; } - const globalOwnerRoleSlug = 'global:owner'; + const globalOwnerRoleSlug = GLOBAL_OWNER_ROLE_SLUG; if (typeof roleSlug !== 'string') { this.logger.warn( @@ -101,6 +102,14 @@ export class ProvisioningService { return; } + if (dbRole.slug === globalOwnerRoleSlug && user.role.slug !== globalOwnerRoleSlug) { + this.logger.warn( + `Skipping instance role provisioning. Cannot assign owner role: ${globalOwnerRoleSlug} to user: ${user.id}`, + { userId: user.id, roleSlug }, + ); + return; + } + /* * If the user is changing from an owner to a non-owner role, * we need to check if they are the last owner to avoid an instance losing its only owner @@ -552,10 +561,17 @@ export class ProvisioningService { return; } - const globalOwnerRoleSlug = 'global:owner'; - if (user.role.slug === globalOwnerRoleSlug && dbRole.slug !== globalOwnerRoleSlug) { + if (dbRole.slug === GLOBAL_OWNER_ROLE_SLUG && user.role.slug !== GLOBAL_OWNER_ROLE_SLUG) { + this.logger.warn( + `Skipping instance role provisioning. Cannot assign owner role: ${GLOBAL_OWNER_ROLE_SLUG} to user: ${user.id}`, + { userId: user.id, roleSlug: GLOBAL_OWNER_ROLE_SLUG }, + ); + return; + } + + if (user.role.slug === GLOBAL_OWNER_ROLE_SLUG && dbRole.slug !== GLOBAL_OWNER_ROLE_SLUG) { const otherOwners = await this.userRepository.count({ - where: { role: { slug: globalOwnerRoleSlug }, id: Not(user.id) }, + where: { role: { slug: GLOBAL_OWNER_ROLE_SLUG }, id: Not(user.id) }, }); if (otherOwners === 0) { this.logger.warn( diff --git a/packages/cli/src/modules/token-exchange/services/__tests__/scoped-jwt.strategy.test.ts b/packages/cli/src/modules/token-exchange/services/__tests__/scoped-jwt.strategy.test.ts index 378f87e534d..e7e8dbc9ba4 100644 --- a/packages/cli/src/modules/token-exchange/services/__tests__/scoped-jwt.strategy.test.ts +++ b/packages/cli/src/modules/token-exchange/services/__tests__/scoped-jwt.strategy.test.ts @@ -1,5 +1,5 @@ import type { AuthenticatedRequest, User, UserRepository } from '@n8n/db'; -import { ALL_API_KEY_SCOPES, type Scope as ScopeType } from '@n8n/permissions'; +import { getApiKeyScopesForRole, type Scope as ScopeType } from '@n8n/permissions'; import type { InstanceSettings } from 'n8n-core'; import type { Mocked } from 'vitest'; import { mock } from 'vitest-mock-extended'; @@ -16,14 +16,20 @@ const otherJwtService = new JwtService( mock(), ); -/** Create a mock user with role.scopes pre-populated for scope assertions. */ -function makeUser(id: string, scopeSlugs: string[] = [], disabled = false): User { +/** Create a mock user with role.slug and role.scopes pre-populated for scope assertions. */ +function makeUser( + id: string, + scopeSlugs: string[] = [], + disabled = false, + roleSlug: User['role']['slug'] = 'global:member', +): User { return { ...mock(), id, disabled, role: { ...mock(), + slug: roleSlug, scopes: scopeSlugs.map((slug) => ({ ...mock(), slug: slug as ScopeType })), }, }; @@ -113,7 +119,21 @@ describe('ScopedJwtStrategy', () => { expect(grant.subject).toBe(subject); expect(grant.actor).toBeUndefined(); expect(grant.scopes).toEqual(['workflow:read', 'workflow:create']); - expect(grant.apiKeyScopes).toEqual(Array.from(ALL_API_KEY_SCOPES)); + // apiKeyScopes are derived from the acting role, never the full set. + expect(grant.apiKeyScopes).toEqual(getApiKeyScopesForRole(subject)); + for (const elevated of ['user:create', 'user:delete', 'user:changeRole'] as const) { + expect(grant.apiKeyScopes).not.toContain(elevated); + } + }); + + it('limits apiKeyScopes to those permitted by the acting role', async () => { + const subject = makeUser('subject-id', ['workflow:read'], false, 'global:chatUser'); + userRepository.findOne.mockResolvedValue(subject); + + const grant = await strategy.buildTokenGrant(makeTokenExchangeJwt()); + + if (!grant) throw new Error('expected grant'); + expect(grant.apiKeyScopes).toEqual([]); }); it('builds a grant with actor scopes when the act claim resolves', async () => { @@ -129,6 +149,7 @@ describe('ScopedJwtStrategy', () => { expect(grant.actor).toBe(actor); // Scopes come from the acting principal (actor) when one is resolved. expect(grant.scopes).toEqual(['credential:read', 'credential:create']); + expect(grant.apiKeyScopes).toEqual(getApiKeyScopesForRole(actor)); }); it('falls back to subject scopes when the act claim is present but actor is not found', async () => { diff --git a/packages/cli/src/modules/token-exchange/services/scoped-jwt.strategy.ts b/packages/cli/src/modules/token-exchange/services/scoped-jwt.strategy.ts index dece217fb00..75f11e4ae83 100644 --- a/packages/cli/src/modules/token-exchange/services/scoped-jwt.strategy.ts +++ b/packages/cli/src/modules/token-exchange/services/scoped-jwt.strategy.ts @@ -1,7 +1,7 @@ import type { AuthenticatedRequest, TokenGrant, User } from '@n8n/db'; import { UserRepository } from '@n8n/db'; import { Service } from '@n8n/di'; -import { ALL_API_KEY_SCOPES } from '@n8n/permissions'; +import { getApiKeyScopesForRole } from '@n8n/permissions'; import { JsonWebTokenError, TokenExpiredError } from 'jsonwebtoken'; import type { AuthStrategy, AuthStrategyOptions } from '@/services/auth-strategy.types'; @@ -65,7 +65,7 @@ export class ScopedJwtStrategy implements AuthStrategy { // 6. Scopes come from the acting user's role (role.scopes is eager: true) return { scopes: actingUser.role.scopes.map((s) => s.slug), - apiKeyScopes: Array.from(ALL_API_KEY_SCOPES), + apiKeyScopes: getApiKeyScopesForRole(actingUser), subject, ...(actor && { actor }), }; diff --git a/packages/cli/src/node-execution/__tests__/ephemeral-node-executor.test.ts b/packages/cli/src/node-execution/__tests__/ephemeral-node-executor.test.ts index bf5e1e5f7c8..924c0613c7c 100644 --- a/packages/cli/src/node-execution/__tests__/ephemeral-node-executor.test.ts +++ b/packages/cli/src/node-execution/__tests__/ephemeral-node-executor.test.ts @@ -20,6 +20,7 @@ import { NodeTypes } from '@/node-types'; import { AGENT_PROVIDER_NODE_WHITELIST, + AGENT_TOOL_NODE_DENYLIST, EphemeralNodeExecutor, isAgentProviderNode, isUsableAsAgentTool, @@ -184,6 +185,64 @@ describe('EphemeralNodeExecutor', () => { expect(result.error).not.toMatch(/not usable as a tool/); }); + it.each([...AGENT_TOOL_NODE_DENYLIST])( + 'rejects the denylisted node type "%s" even when it looks like a usable tool', + async (nodeType) => { + nodeTypes.getByNameAndVersion.mockReturnValue( + mockNodeType({ description: toolDescription }), + ); + + const result = await executor.executeInline({ + nodeType, + nodeTypeVersion: 1, + nodeParameters: {}, + inputData: [], + projectId: 'p-1', + }); + + expect(result.status).toBe('error'); + expect(result.error).toContain('not permitted for agent tool execution'); + // The denylist check must run before the node-types lookup for the + // base type, otherwise resolveToolNodeType's `*Tool` remapping in + // the caller could dodge it via a type that doesn't exist yet. + expect(nodeTypes.getByNameAndVersion).not.toHaveBeenCalled(); + }, + ); + + it.each([...AGENT_TOOL_NODE_DENYLIST])( + 'rejects the `*Tool`-suffixed variant of the denylisted node type "%s"', + async (nodeType) => { + const result = await executor.executeInline({ + nodeType: `${nodeType}Tool`, + nodeTypeVersion: 1, + nodeParameters: {}, + inputData: [], + projectId: 'p-1', + }); + + expect(result.status).toBe('error'); + expect(result.error).toContain('not permitted for agent tool execution'); + }, + ); + + it('still executes a normal tool node that is not on the denylist', async () => { + const execute = vi.fn().mockResolvedValue([[{ json: { ok: true } }]]); + nodeTypes.getByNameAndVersion.mockReturnValue({ + description: toolDescription, + execute, + } as unknown as INodeType); + + const result = await executor.executeInline({ + nodeType: 'n8n-nodes-base.slack', + nodeTypeVersion: 1, + nodeParameters: {}, + inputData: [], + projectId: 'p-1', + }); + + expect(result.status).toBe('success'); + }); + it('returns a structured error when the operation is on the blacklist (sendAndWait)', async () => { nodeTypes.getByNameAndVersion.mockReturnValue(mockNodeType({ description: toolDescription })); diff --git a/packages/cli/src/node-execution/ephemeral-node-executor.ts b/packages/cli/src/node-execution/ephemeral-node-executor.ts index 95a26d48abd..0734e73fc17 100644 --- a/packages/cli/src/node-execution/ephemeral-node-executor.ts +++ b/packages/cli/src/node-execution/ephemeral-node-executor.ts @@ -60,6 +60,30 @@ export interface NodeExecutionResult { // send and wait requires persistent workflows to handle the wait logic const OPERATION_BLACKLIST = [SEND_AND_WAIT_OPERATION, 'dispatchAndWait']; +/** + * Node types that must never run as an agent tool, regardless of RBAC — + * they grant command execution or arbitrary file-system access, which is a + * different trust tier than "call an API with a shared credential". This is + * a defense-in-depth backstop applied to every execution path (chat, + * published integrations, tasks, workflows), independent of the per-user + * access checks applied when building the tool list. + */ +export const AGENT_TOOL_NODE_DENYLIST = new Set([ + 'n8n-nodes-base.executeCommand', + 'n8n-nodes-base.ssh', + 'n8n-nodes-base.readWriteFile', +]); + +/** + * The node-types resolver may hand us the `*Tool` variant of a node + * (e.g. `executeCommand` -> `executeCommandTool`, see `resolveToolNodeType` + * in `node-tool-factory.ts`). Strip that suffix before checking the denylist + * so both the base and tool-wrapped forms are caught. + */ +function stripAgentToolSuffix(nodeType: string): string { + return nodeType.endsWith('Tool') ? nodeType.slice(0, -'Tool'.length) : nodeType; +} + /** * Vendor-API nodes the agent runtime can execute even though they aren't * marked `usableAsTool`. They expose the full provider API (image generation, @@ -212,6 +236,12 @@ export class EphemeralNodeExecutor { typeVersion: number, nodeParameters: INodeParameters, ): void { + if (AGENT_TOOL_NODE_DENYLIST.has(stripAgentToolSuffix(nodeType))) { + throw new UserError('Node type is not permitted for agent tool execution', { + extra: { nodeType }, + }); + } + const resolved = this.nodeTypes.getByNameAndVersion(nodeType, typeVersion); if (!isUsableAsAgentTool(resolved.description) && !isAgentProviderNode(nodeType)) { diff --git a/packages/cli/src/node-execution/index.ts b/packages/cli/src/node-execution/index.ts index 310e13e80b6..a3d781a477c 100644 --- a/packages/cli/src/node-execution/index.ts +++ b/packages/cli/src/node-execution/index.ts @@ -7,4 +7,5 @@ export { EphemeralNodeExecutor, isAgentProviderNode, AGENT_PROVIDER_NODE_WHITELIST, + AGENT_TOOL_NODE_DENYLIST, } from './ephemeral-node-executor'; diff --git a/packages/cli/src/services/__tests__/dynamic-node-parameters.service.test.ts b/packages/cli/src/services/__tests__/dynamic-node-parameters.service.test.ts index ed8c29b651a..f0a65636b30 100644 --- a/packages/cli/src/services/__tests__/dynamic-node-parameters.service.test.ts +++ b/packages/cli/src/services/__tests__/dynamic-node-parameters.service.test.ts @@ -2,26 +2,33 @@ import { Logger } from '@n8n/backend-common'; import { mockInstance } from '@n8n/backend-test-utils'; import { SharedWorkflowRepository } from '@n8n/db'; import type { User } from '@n8n/db'; +import { RoutingNode } from 'n8n-core'; import { - type ILoadOptions, type INodeParameters, type INodeType, type IWorkflowExecuteAdditionalData, type ResourceMapperFields, Expression, } from 'n8n-workflow'; -import type { MockInstance } from 'vitest'; +import type { Mock, MockInstance } from 'vitest'; import { mock } from 'vitest-mock-extended'; +vi.mock('n8n-core', async () => { + return { + ...(await vi.importActual('n8n-core')), + RoutingNode: vi.fn(), + }; +}); + +import { DynamicNodeParametersService } from '../dynamic-node-parameters.service'; +import { WorkflowLoaderService } from '../workflow-loader.service'; + import { CredentialsFinderService } from '@/credentials/credentials-finder.service'; import { BadRequestError } from '@/errors/response-errors/bad-request.error'; import { ForbiddenError } from '@/errors/response-errors/forbidden.error'; import { NodeTypes } from '@/node-types'; import * as checkAccess from '@/permissions.ee/check-access'; -import { DynamicNodeParametersService } from '../dynamic-node-parameters.service'; -import { WorkflowLoaderService } from '../workflow-loader.service'; - describe('DynamicNodeParametersService', () => { const logger = mockInstance(Logger); const nodeTypes = mockInstance(NodeTypes); @@ -222,7 +229,7 @@ describe('DynamicNodeParametersService', () => { await expect( service.getOptionsViaLoadOptions( - mock(), + { routing: { request: { url: '/v1/models' } } }, mock(), { name: 'TestNode', version: 1 }, mock(), @@ -231,6 +238,75 @@ describe('DynamicNodeParametersService', () => { }); }); + describe('getOptionsViaLoadOptionsByPath', () => { + it('should throw BadRequestError when no loadOptions routing exists at the parameter path', async () => { + nodeTypes.getByNameAndVersion.mockReturnValue( + mock({ + description: { + name: 'TestNode', + properties: [], + requestDefaults: { baseURL: 'https://api.example.com' }, + }, + }), + ); + + await expect( + service.getOptionsViaLoadOptionsByPath( + 'parameters.unknown', + mock(), + { name: 'TestNode', version: 1 }, + mock(), + ), + ).rejects.toThrow(BadRequestError); + }); + + it('should resolve routing from the node definition and run it', async () => { + const runNode = vi.fn().mockResolvedValue([[{ json: { name: 'opt', value: 'v' } }]]); + (RoutingNode as unknown as Mock).mockImplementation(function () { + return { runNode }; + }); + vi.spyOn(Expression.prototype, 'acquireIsolate').mockResolvedValue(true); + vi.spyOn(Expression.prototype, 'releaseIsolate').mockResolvedValue(undefined); + + const nodeRouting = { request: { url: '/v1/models', method: 'GET' as const } }; + // Plain object (not a deep mock) so Workflow's parameter resolution doesn't + // trip over auto-generated mock fields on the property. + nodeTypes.getByNameAndVersion.mockReturnValue({ + description: { + name: 'TestNode', + displayName: 'TestNode', + group: [], + version: 1, + defaults: {}, + inputs: [], + outputs: [], + properties: [ + { + displayName: 'Model', + name: 'model', + type: 'options', + default: '', + options: [], + typeOptions: { loadOptions: { routing: nodeRouting } }, + }, + ], + requestDefaults: { baseURL: 'https://api.example.com' }, + }, + } as unknown as INodeType); + + const result = await service.getOptionsViaLoadOptionsByPath( + 'parameters.model', + mock(), + { name: 'TestNode', version: 1 }, + {} as INodeParameters, + ); + + expect(RoutingNode).toHaveBeenCalled(); + expect(runNode).toHaveBeenCalled(); + expect(result).toEqual([{ name: 'opt', value: 'v' }]); + }); + }); + describe('getMethod', () => { it('should throw BadRequestError when the requested method does not exist', async () => { nodeTypes.getByNameAndVersion.mockReturnValue({ diff --git a/packages/cli/src/services/__tests__/protected-resource.registry.test.ts b/packages/cli/src/services/__tests__/protected-resource.registry.test.ts index 953ddc8699f..b59c10ccbbc 100644 --- a/packages/cli/src/services/__tests__/protected-resource.registry.test.ts +++ b/packages/cli/src/services/__tests__/protected-resource.registry.test.ts @@ -9,6 +9,7 @@ const resourceA: ProtectedResource = { getResourceUrl: () => 'https://n8n.example.com/mcp-server/http', getAudiences: () => ['https://n8n.example.com/mcp-server/http', 'mcp-server-api'], scopes: ['tool:listWorkflows', 'tool:getWorkflowDetails'], + authorize: async () => true, isDefault: true, }; @@ -16,6 +17,7 @@ const resourceB: ProtectedResource = { id: 'workflow-trigger', getResourceUrl: () => 'https://n8n.example.com/webhook/wf-1/mcp', getAudiences: () => ['https://n8n.example.com/webhook/wf-1/mcp'], + authorize: async () => true, scopes: ['tool:listWorkflows', 'workflow:execute'], }; diff --git a/packages/cli/src/services/dynamic-node-parameters.service.ts b/packages/cli/src/services/dynamic-node-parameters.service.ts index b72ac0dcd76..7170bc9dda6 100644 --- a/packages/cli/src/services/dynamic-node-parameters.service.ts +++ b/packages/cli/src/services/dynamic-node-parameters.service.ts @@ -22,7 +22,12 @@ import type { ILocalLoadOptionsFunctions, IExecuteData, } from 'n8n-workflow'; -import { Workflow, UnexpectedError, createEmptyRunExecutionData } from 'n8n-workflow'; +import { + Workflow, + UnexpectedError, + createEmptyRunExecutionData, + findDisplayedProperty, +} from 'n8n-workflow'; import { CredentialsFinderService } from '@/credentials/credentials-finder.service'; import { BadRequestError } from '@/errors/response-errors/bad-request.error'; @@ -143,6 +148,43 @@ export class DynamicNodeParametersService { }); } + /** + * Resolves the property's loadOptions routing from the node definition via the parameter path + * (not from the request body), then runs it. + */ + async getOptionsViaLoadOptionsByPath( + path: string, + additionalData: IWorkflowExecuteAdditionalData, + nodeTypeAndVersion: INodeTypeNameVersion, + currentNodeParameters: INodeParameters, + credentials?: INodeCredentials, + ): Promise { + const nodeType = this.getNodeType(nodeTypeAndVersion); + const property = findDisplayedProperty( + path, + nodeType.description.properties, + currentNodeParameters, + { typeVersion: nodeTypeAndVersion.version }, + nodeType.description, + ); + const routing = + property && 'typeOptions' in property + ? property.typeOptions?.loadOptions?.routing + : undefined; + if (!routing) { + throw new BadRequestError( + `Node type "${nodeType.description.name}" has no loadOptions routing for parameter path "${path}"`, + ); + } + return await this.getOptionsViaLoadOptions( + { routing }, + additionalData, + nodeTypeAndVersion, + currentNodeParameters, + credentials, + ); + } + /** Returns the available options via a loadOptions param */ async getOptionsViaLoadOptions( loadOptions: ILoadOptions, @@ -153,12 +195,6 @@ export class DynamicNodeParametersService { ): Promise { const nodeType = this.getNodeType(nodeTypeAndVersion); if (!nodeType.description.requestDefaults?.baseURL) { - // This is in here for now for security reasons. - // Background: As the full data for the request to make does get send, and the auth data - // will then be applied, would it be possible to retrieve that data like that. By at least - // requiring a baseURL to be defined can at least not a random server be called. - // In the future this code has to get improved that it does not use the request information from - // the request rather resolves it via the parameter-path and nodeType data. throw new BadRequestError( `Node type "${nodeType.description.name}" does not exist or does not have "requestDefaults.baseURL" defined!`, ); diff --git a/packages/cli/src/services/oauth-token-verifier-proxy.service.ts b/packages/cli/src/services/oauth-token-verifier-proxy.service.ts index ec1ccb3b0dd..7d5c8c52801 100644 --- a/packages/cli/src/services/oauth-token-verifier-proxy.service.ts +++ b/packages/cli/src/services/oauth-token-verifier-proxy.service.ts @@ -10,7 +10,8 @@ export type AuthFailureReason = | 'user_not_found' | 'user_id_not_in_auth_info' | 'unknown_error' - | 'verifier_not_registered'; + | 'verifier_not_registered' + | 'insufficient_scope'; export type Mcpauth_type = 'oauth' | 'api_key' | 'unknown'; diff --git a/packages/cli/src/services/protected-resource.registry.ts b/packages/cli/src/services/protected-resource.registry.ts index 366cb5640ad..3628b6fc393 100644 --- a/packages/cli/src/services/protected-resource.registry.ts +++ b/packages/cli/src/services/protected-resource.registry.ts @@ -1,4 +1,5 @@ import { Logger } from '@n8n/backend-common'; +import type { User } from '@n8n/db'; import { Service } from '@n8n/di'; import { ensureError } from '@n8n/utils/errors/ensure-error'; @@ -48,6 +49,15 @@ export interface ProtectedResource { * registered-URIs match per RFC 6749 §3.1.2.4. */ getAllowedRedirectUris?(): Promise; + + /** + * Determine whether the given user is authorized to access this resource. + * Called during the consent flow to gate access to the resource. + * + * @param user The user to authorize + * @returns A promise that resolves to a boolean indicating whether the user is authorized + **/ + authorize(user: User): Promise; } /** diff --git a/packages/cli/src/webhooks/webhook-helpers.ts b/packages/cli/src/webhooks/webhook-helpers.ts index 6c7f91ed37c..88688221a76 100644 --- a/packages/cli/src/webhooks/webhook-helpers.ts +++ b/packages/cli/src/webhooks/webhook-helpers.ts @@ -31,6 +31,7 @@ import type { IWorkflowDataProxyAdditionalKeys, IWorkflowExecuteAdditionalData, WebhookResponseMode, + OAuth2FailureReason, Workflow, WorkflowExecuteMode, IWorkflowExecutionDataProcess, @@ -522,6 +523,18 @@ export async function executeWebhook( }; }; + const translateAuthFailureReason = (reason?: AuthFailureReason): OAuth2FailureReason => { + switch (reason) { + case 'verifier_not_registered': + case 'unknown_error': + return 'verifier_unavailable'; + case 'insufficient_scope': + return 'insufficient_scope'; + default: + return 'invalid_token'; + } + }; + additionalData.validateN8nOAuth2Token = async (token: string, resourceUrl: string) => { const oauthTokenVerifierProxy = Container.get(OAuthTokenVerifierProxy); const result = await oauthTokenVerifierProxy.verifyOAuthAccessToken(token, resourceUrl); @@ -536,16 +549,10 @@ export async function executeWebhook( }, }; } - const VERIFIER_UNAVAILABLE_REASONS: AuthFailureReason[] = [ - 'verifier_not_registered', - 'unknown_error', - ]; + return { valid: false, - reason: - result.context?.reason && VERIFIER_UNAVAILABLE_REASONS.includes(result.context.reason) - ? 'verifier_unavailable' - : 'invalid_token', + reason: translateAuthFailureReason(result.context?.reason), }; }; diff --git a/packages/cli/test/integration/controllers/dynamic-node-parameters.controller.test.ts b/packages/cli/test/integration/controllers/dynamic-node-parameters.controller.test.ts index b9edb7c7f94..747b420b775 100644 --- a/packages/cli/test/integration/controllers/dynamic-node-parameters.controller.test.ts +++ b/packages/cli/test/integration/controllers/dynamic-node-parameters.controller.test.ts @@ -64,6 +64,7 @@ describe('DynamicNodeParametersController', () => { ownerAgent = testServer.authAgentFor(owner); const expectedResult = [{ name: 'Test Option', value: 'test' }]; + vi.spyOn(service, 'getOptionsViaLoadOptionsByPath').mockResolvedValue(expectedResult); vi.spyOn(service, 'getOptionsViaLoadOptions').mockResolvedValue(expectedResult); const response = await ownerAgent diff --git a/packages/core/src/execution-engine/node-execution-context/utils/__tests__/file-system-helper-functions.test.ts b/packages/core/src/execution-engine/node-execution-context/utils/__tests__/file-system-helper-functions.test.ts index 9fa245fa393..2c93b3f7ff2 100644 --- a/packages/core/src/execution-engine/node-execution-context/utils/__tests__/file-system-helper-functions.test.ts +++ b/packages/core/src/execution-engine/node-execution-context/utils/__tests__/file-system-helper-functions.test.ts @@ -1,13 +1,16 @@ import { SecurityConfig } from '@n8n/config'; import { Container } from '@n8n/di'; -import type { INode } from 'n8n-workflow'; +import type { INode, ResolvedFilePath } from 'n8n-workflow'; import { constants } from 'node:fs'; import { access as fsAccess, realpath as fsRealpath, stat as fsStat, open as fsOpen, + mkdir as fsMkdir, + lstat as fsLstat, } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; import { join } from 'node:path'; import type { Mock } from 'vitest'; @@ -534,4 +537,307 @@ describe('getFileSystemHelperFunctions', () => { expect(mockFileHandle.close).toHaveBeenCalled(); }); }); + + describe('symlinked ancestor directory', () => { + // Build a REAL symlinked dir via importActual (this file mocks node:fs/promises, but the + // symlink check in @n8n/backend-common runs against the real filesystem). + let realFs: { + realpath(path: string): Promise; + mkdtemp(prefix: string): Promise; + mkdir(path: string, options?: { recursive?: boolean }): Promise; + symlink(target: string, path: string): Promise; + rm(path: string, options?: { recursive?: boolean; force?: boolean }): Promise; + }; + let base: string; + let pathWithSymlinkedAncestor: string; + + beforeAll(async () => { + realFs = await vi.importActual('node:fs/promises'); + }); + + beforeEach(async () => { + base = await realFs.realpath(await realFs.mkdtemp(join(tmpdir(), 'fsh-toctou-'))); + await realFs.mkdir(join(base, 'real'), { recursive: true }); + await realFs.symlink(join(base, 'real'), join(base, 'link')); + // `link` is a symlinked ancestor of the target file. + pathWithSymlinkedAncestor = join(base, 'link', 'file.txt'); + }); + + afterEach(async () => { + await realFs.rm(base, { recursive: true, force: true }); + }); + + it('createReadStream rejects when an ancestor path component is a symlink', async () => { + (fsStat as Mock).mockResolvedValueOnce({ dev: 1, ino: 2 }); + (fsAccess as Mock).mockResolvedValueOnce(undefined); + + await expect( + helperFunctions.createReadStream( + await helperFunctions.resolvePath(pathWithSymlinkedAncestor), + ), + ).rejects.toThrow('Access to the file is not allowed'); + }); + + it('writeContentToFile rejects when an ancestor path component is a symlink', async () => { + (fsStat as Mock).mockResolvedValueOnce({ dev: 1, ino: 2, isFile: () => true }); + + await expect( + helperFunctions.writeContentToFile( + await helperFunctions.resolvePath(pathWithSymlinkedAncestor), + 'content', + ), + ).rejects.toThrow('Access to the file is not allowed'); + }); + }); + + describe('ensureParentDirectoryWithoutFollowingSymlinks', () => { + it('creates each parent component when none is a symlink', async () => { + (fsMkdir as Mock).mockResolvedValue(undefined); + (fsLstat as Mock).mockResolvedValue({ isSymbolicLink: () => false, isDirectory: () => true }); + + await expect( + helperFunctions.ensureParentDirectoryWithoutFollowingSymlinks( + await helperFunctions.resolvePath('/allowed/dir/file'), + ), + ).resolves.toBeUndefined(); + + expect(fsMkdir).toHaveBeenCalledWith('/allowed'); + expect(fsMkdir).toHaveBeenCalledWith('/allowed/dir'); + }); + + it('rejects when a parent component is a symlink', async () => { + (fsMkdir as Mock).mockResolvedValue(undefined); + (fsLstat as Mock).mockResolvedValue({ isSymbolicLink: () => true, isDirectory: () => false }); + + await expect( + helperFunctions.ensureParentDirectoryWithoutFollowingSymlinks( + await helperFunctions.resolvePath('/allowed/dir/file'), + ), + ).rejects.toThrow('Access to the file is not allowed'); + }); + }); + + describe('resolveStagingBaseForTarget', () => { + it('returns the realpath of the allowed base that contains the target', async () => { + securityConfig.restrictFileAccessTo = '/allowed/base'; + + const base = await helperFunctions.resolveStagingBaseForTarget( + await helperFunctions.resolvePath('/allowed/base/sub/repo'), + ); + + expect(base).toBe('/allowed/base'); + }); + + it('returns the containing base when several are allowed', async () => { + securityConfig.restrictFileAccessTo = '/first/base;/second/base'; + + const base = await helperFunctions.resolveStagingBaseForTarget( + await helperFunctions.resolvePath('/second/base/repo'), + ); + + expect(base).toBe('/second/base'); + }); + + it('resolves the allowed base through symlinks', async () => { + securityConfig.restrictFileAccessTo = '/allowed/link'; + (fsRealpath as Mock).mockImplementation((path: string) => + path === '/allowed/link' ? '/allowed/real' : path, + ); + + const base = await helperFunctions.resolveStagingBaseForTarget( + '/allowed/link/repo' as ResolvedFilePath, + ); + + expect(base).toBe('/allowed/real'); + }); + + it('falls back to the n8n folder when no path restriction is configured', async () => { + securityConfig.restrictFileAccessTo = ''; + + const base = await helperFunctions.resolveStagingBaseForTarget( + '/anywhere/repo' as ResolvedFilePath, + ); + + expect(base).toBe(instanceSettings.n8nFolder); + }); + }); + + describe('pinDirectory', () => { + const originalPlatform = process.platform; + const setPlatform = (platform: string) => + Object.defineProperty(process, 'platform', { value: platform, configurable: true }); + + afterEach(() => { + Object.defineProperty(process, 'platform', { + value: originalPlatform, + configurable: true, + }); + }); + + it('returns null on non-Linux platforms', async () => { + setPlatform('darwin'); + securityConfig.restrictFileAccessTo = '/allowed/base'; + + await expect( + helperFunctions.pinDirectory('/allowed/base/sub', { create: false }), + ).resolves.toBeNull(); + }); + + it('returns null when the directory is not within a trusted base', async () => { + setPlatform('linux'); + securityConfig.restrictFileAccessTo = '/allowed/base'; + + await expect( + helperFunctions.pinDirectory('/elsewhere/sub', { create: false }), + ).resolves.toBeNull(); + }); + + it('returns null when the resolved anchor no longer contains the path', async () => { + setPlatform('linux'); + securityConfig.restrictFileAccessTo = '/allowed/link'; + (fsRealpath as Mock).mockImplementation((path: string) => + path === '/allowed/link' ? '/real/base' : path, + ); + + await expect( + helperFunctions.pinDirectory('/allowed/link/sub', { create: false }), + ).resolves.toBeNull(); + }); + + it('descends from the trusted base relative to the held descriptors', async () => { + setPlatform('linux'); + securityConfig.restrictFileAccessTo = '/allowed/base'; + + const handles: Array<{ fd: number; close: Mock }> = []; + let nextFd = 10; + (fsOpen as Mock).mockImplementation(async () => { + const handle = { fd: nextFd++, close: vi.fn().mockResolvedValue(undefined) }; + handles.push(handle); + return handle; + }); + (fsMkdir as Mock).mockResolvedValue(undefined); + (fsOpen as Mock).mockClear(); + (fsMkdir as Mock).mockClear(); + + const pinned = await helperFunctions.pinDirectory('/allowed/base/sub/dir', { + create: true, + }); + + expect(fsOpen).toHaveBeenNthCalledWith(1, '/allowed/base', expect.any(Number)); + expect(fsMkdir).toHaveBeenCalledWith('/proc/self/fd/10/sub'); + expect(fsOpen).toHaveBeenNthCalledWith(2, '/proc/self/fd/10/sub', expect.any(Number)); + expect(fsMkdir).toHaveBeenCalledWith('/proc/self/fd/11/dir'); + expect(fsOpen).toHaveBeenNthCalledWith(3, '/proc/self/fd/11/dir', expect.any(Number)); + + expect(pinned).not.toBeNull(); + expect(pinned?.resolvePath('repo')).toBe('/proc/self/fd/12/repo'); + + // Intermediate descriptors are closed during the descent; the final one is held. + expect(handles[0].close).toHaveBeenCalled(); + expect(handles[1].close).toHaveBeenCalled(); + expect(handles[2].close).not.toHaveBeenCalled(); + + await pinned?.close(); + expect(handles[2].close).toHaveBeenCalled(); + }); + + it('rejects when a descended component is not a real directory', async () => { + setPlatform('linux'); + securityConfig.restrictFileAccessTo = '/allowed/base'; + + const notDirectory = Object.assign(new Error('ENOTDIR'), { code: 'ENOTDIR' }); + const closeAnchor = vi.fn().mockResolvedValue(undefined); + let call = 0; + (fsOpen as Mock).mockImplementation(async () => { + call += 1; + if (call === 1) return { fd: 20, close: closeAnchor }; + throw notDirectory; + }); + + await expect( + helperFunctions.pinDirectory('/allowed/base/link', { create: false }), + ).rejects.toThrow('Access to the file is not allowed'); + expect(closeAnchor).toHaveBeenCalled(); + }); + }); + + describe('pinned file access on Linux', () => { + const originalPlatform = process.platform; + + beforeEach(() => { + Object.defineProperty(process, 'platform', { value: 'linux', configurable: true }); + securityConfig.restrictFileAccessTo = '/allowed'; + }); + + afterEach(() => { + Object.defineProperty(process, 'platform', { + value: originalPlatform, + configurable: true, + }); + }); + + // '/allowed/sub/file' pins via two directory opens (anchor + 'sub'); the third open + // is the leaf, addressed relative to the pinned parent descriptor. + const onLeafOpen = (leaf: () => Promise) => { + let call = 0; + let fd = 30; + (fsOpen as Mock).mockImplementation(async () => { + call += 1; + if (call <= 2) { + return { fd: fd++, close: vi.fn().mockResolvedValue(undefined) }; + } + return await leaf(); + }); + }; + + it('rejects a symlinked leaf opened relative to the pinned parent', async () => { + const eloop = Object.assign(new Error('ELOOP'), { code: 'ELOOP' }); + (fsStat as Mock).mockResolvedValueOnce({ dev: 1, ino: 2 }); + (fsAccess as Mock).mockResolvedValueOnce(undefined); + onLeafOpen(async () => { + throw eloop; + }); + + await expect( + helperFunctions.createReadStream(await helperFunctions.resolvePath('/allowed/sub/file')), + ).rejects.toThrow('Symlinks are not allowed.'); + }); + + it('opens the leaf relative to the pinned parent and verifies its identity', async () => { + const stats = { dev: 7, ino: 8 }; + const stream = { pipe: vi.fn() }; + const fileHandle = { + stat: vi.fn().mockResolvedValue(stats), + createReadStream: vi.fn().mockReturnValue(stream), + close: vi.fn(), + }; + (fsStat as Mock).mockResolvedValueOnce(stats); + (fsAccess as Mock).mockResolvedValueOnce(undefined); + onLeafOpen(async () => fileHandle); + + const result = await helperFunctions.createReadStream( + await helperFunctions.resolvePath('/allowed/sub/file'), + ); + + expect(result).toBe(stream); + const leafCall = (fsOpen as Mock).mock.calls.at(-1); + expect(leafCall?.[0]).toMatch(/^\/proc\/self\/fd\/\d+\/file$/); + }); + + it('rejects when the pinned leaf identity does not match', async () => { + const fileHandle = { + stat: vi.fn().mockResolvedValue({ dev: 999, ino: 888 }), + createReadStream: vi.fn(), + close: vi.fn(), + }; + (fsStat as Mock).mockResolvedValueOnce({ dev: 7, ino: 8 }); + (fsAccess as Mock).mockResolvedValueOnce(undefined); + onLeafOpen(async () => fileHandle); + + await expect( + helperFunctions.createReadStream(await helperFunctions.resolvePath('/allowed/sub/file')), + ).rejects.toThrow('The file has changed and cannot be accessed.'); + expect(fileHandle.close).toHaveBeenCalled(); + }); + }); }); diff --git a/packages/core/src/execution-engine/node-execution-context/utils/extract-value.ts b/packages/core/src/execution-engine/node-execution-context/utils/extract-value.ts index 83b85f2d284..d42e4c020cc 100644 --- a/packages/core/src/execution-engine/node-execution-context/utils/extract-value.ts +++ b/packages/core/src/execution-engine/node-execution-context/utils/extract-value.ts @@ -23,48 +23,16 @@ function findPropertyFromParameterName( node: INode, nodeParameters: INodeParameters, ): INodePropertyOptions | INodeProperties | INodePropertyCollection { - let property: INodePropertyOptions | INodeProperties | INodePropertyCollection | undefined; - const paramParts = parameterName.split('.'); - let currentParamPath = ''; - - const findProp = ( - name: string, - options: Array, - ): INodePropertyOptions | INodeProperties | INodePropertyCollection | undefined => { - return options.find( - (i) => - i.name === name && - NodeHelpers.displayParameterPath( - nodeParameters, - i, - currentParamPath, - node, - nodeType.description, - ), - ); - }; - - for (const p of paramParts) { - const param = p.split('[')[0]; - if (!property) { - property = findProp(param, nodeType.description.properties); - } else if ('options' in property && property.options) { - property = findProp(param, property.options); - currentParamPath += `.${param}`; - } else if ('values' in property) { - property = findProp(param, property.values); - currentParamPath += `.${param}`; - } else { - throw new UserError('Could not find property', { extra: { parameterName } }); - } - if (!property) { - throw new UserError('Could not find property', { extra: { parameterName } }); - } - } + const property = NodeHelpers.findDisplayedProperty( + parameterName, + nodeType.description.properties, + nodeParameters, + node, + nodeType.description, + ); if (!property) { throw new UserError('Could not find property', { extra: { parameterName } }); } - return property; } diff --git a/packages/core/src/execution-engine/node-execution-context/utils/file-system-helper-functions.ts b/packages/core/src/execution-engine/node-execution-context/utils/file-system-helper-functions.ts index 56315b53211..b3c154ea03c 100644 --- a/packages/core/src/execution-engine/node-execution-context/utils/file-system-helper-functions.ts +++ b/packages/core/src/execution-engine/node-execution-context/utils/file-system-helper-functions.ts @@ -1,8 +1,19 @@ -import { isContainedWithin, safeJoinPath } from '@n8n/backend-common'; +import { + containsSymlinkedComponent, + isContainedWithin, + pathComponents, + pathSegmentsBetween, + safeJoinPath, +} from '@n8n/backend-common'; import { SecurityConfig } from '@n8n/config'; import { Container } from '@n8n/di'; import { NodeOperationError } from 'n8n-workflow'; -import type { FileSystemHelperFunctions, INode, ResolvedFilePath } from 'n8n-workflow'; +import type { + FileSystemHelperFunctions, + INode, + PinnedDirectory, + ResolvedFilePath, +} from 'n8n-workflow'; import type { PathLike } from 'node:fs'; import { constants } from 'node:fs'; import { @@ -10,7 +21,10 @@ import { realpath as fsRealpath, stat as fsStat, open as fsOpen, + mkdir as fsMkdir, + lstat as fsLstat, } from 'node:fs/promises'; +import type { FileHandle } from 'node:fs/promises'; import { homedir } from 'node:os'; import { posix, dirname, basename, join } from 'node:path'; @@ -73,6 +87,36 @@ function isFilePatternBlocked(resolvedFilePath: ResolvedFilePath): boolean { }); } +async function assertNoSymlinkInPath( + resolvedFilePath: ResolvedFilePath, + node: INode, +): Promise { + if (await containsSymlinkedComponent(dirname(resolvedFilePath))) { + throw new NodeOperationError(node, 'Access to the file is not allowed.', { level: 'warning' }); + } +} + +async function ensureParentDirectoryWithoutFollowingSymlinks( + resolvedFilePath: ResolvedFilePath, + node: INode, +): Promise { + for (const component of pathComponents(dirname(resolvedFilePath))) { + try { + await fsMkdir(component); + } catch (error) { + if (!(error instanceof Error && 'code' in error && error.code === 'EEXIST')) { + throw error; + } + } + const stats = await fsLstat(component); + if (stats.isSymbolicLink() || !stats.isDirectory()) { + throw new NodeOperationError(node, 'Access to the file is not allowed.', { + level: 'warning', + }); + } + } +} + function isFilePathBlocked(resolvedFilePath: ResolvedFilePath): boolean { const allowedPaths = getAllowedPaths(); const blockFileAccessToN8nFiles = process.env[BLOCK_FILE_ACCESS_TO_N8N_FILES] !== 'false'; @@ -95,6 +139,136 @@ function isFilePathBlocked(resolvedFilePath: ResolvedFilePath): boolean { return false; } +async function resolveContainingAllowedBase(targetPath: string): Promise { + const containingBase = getAllowedPaths().find((allowedPath) => + isContainedWithin(allowedPath, targetPath), + ); + return containingBase === undefined + ? null + : ((await fsRealpath(containingBase)) as ResolvedFilePath); +} + +async function resolveStagingBaseForTarget(target: ResolvedFilePath): Promise { + return ( + (await resolveContainingAllowedBase(target)) ?? + ((await fsRealpath(Container.get(InstanceSettings).n8nFolder)) as ResolvedFilePath) + ); +} + +// Linux exposes every open descriptor under `/proc/self/fd/` as a magic symlink to the +// object it holds. Resolving `/` through it addresses `` relative to that +// descriptor. +const PROC_SELF_FD = '/proc/self/fd'; + +function hasErrorCode(error: unknown, ...codes: string[]): boolean { + if (typeof error !== 'object' || error === null || !('code' in error)) { + return false; + } + const { code } = error; + return typeof code === 'string' && codes.includes(code); +} + +async function resolveTrustedAnchorForPath(targetPath: string): Promise { + const containingBase = await resolveContainingAllowedBase(targetPath); + if (containingBase !== null) { + return containingBase; + } + + const n8nFolder = (await fsRealpath( + Container.get(InstanceSettings).n8nFolder, + )) as ResolvedFilePath; + return isContainedWithin(n8nFolder, targetPath) ? n8nFolder : null; +} + +async function pinDirectory( + directoryPath: string, + node: INode, + options: { create: boolean }, +): Promise { + // /proc/self/fd exists only on Linux; the caller falls back to the path-based check. + if (process.platform !== 'linux') { + return null; + } + + const anchor = await resolveTrustedAnchorForPath(directoryPath); + if (anchor === null) { + return null; + } + const segments = pathSegmentsBetween(anchor, directoryPath); + if (segments === null) { + return null; + } + + const directoryFlags = constants.O_RDONLY | constants.O_DIRECTORY | constants.O_NOFOLLOW; + let current: FileHandle = await fsOpen(anchor, directoryFlags); + try { + for (const segment of segments) { + const relativePath = `${PROC_SELF_FD}/${current.fd}/${segment}`; + if (options.create) { + try { + await fsMkdir(relativePath); + } catch (error) { + if (!hasErrorCode(error, 'EEXIST')) throw error; + } + } + let next: FileHandle; + try { + next = await fsOpen(relativePath, directoryFlags); + } catch (error) { + if (hasErrorCode(error, 'ELOOP', 'ENOTDIR')) { + throw new NodeOperationError(node, 'Access to the file is not allowed.', { + level: 'warning', + }); + } + throw error; + } + const previous = current; + current = next; + await previous.close(); + } + } catch (error) { + await current.close(); + throw error; + } + + const pinned = current; + return { + resolvePath(name: string) { + return `${PROC_SELF_FD}/${pinned.fd}/${name}`; + }, + async close() { + await pinned.close(); + }, + }; +} + +async function openWithoutFollowingSymlinkedAncestors( + resolvedFilePath: ResolvedFilePath, + flags: number, + node: INode, +): Promise { + const pinnedParent = await pinDirectory(dirname(resolvedFilePath), node, { create: false }); + try { + if (!pinnedParent) { + await assertNoSymlinkInPath(resolvedFilePath, node); + } + const openPath = pinnedParent + ? pinnedParent.resolvePath(basename(resolvedFilePath)) + : resolvedFilePath; + return await fsOpen(openPath, flags); + } catch (error) { + if (hasErrorCode(error, 'ELOOP')) { + throw new NodeOperationError(node, error instanceof Error ? error : '', { + message: 'Symlinks are not allowed.', + level: 'warning', + }); + } + throw error; + } finally { + await pinnedParent?.close(); + } +} + export const getFileSystemHelperFunctions = (node: INode): FileSystemHelperFunctions => ({ async createReadStream(resolvedFilePath) { // Get the device and inode number of the path we're checking. @@ -121,20 +295,11 @@ export const getFileSystemHelperFunctions = (node: INode): FileSystemHelperFunct : error; } - // Open a file handle. - let fileHandle; - try { - fileHandle = await fsOpen(resolvedFilePath, constants.O_RDONLY | constants.O_NOFOLLOW); - } catch (error) { - if ('code' in error && (error as unknown as { code: string }).code === 'ELOOP') { - throw new NodeOperationError(node, error instanceof Error ? error : '', { - message: 'Symlinks are not allowed.', - level: 'warning', - }); - } else { - throw error; - } - } + const fileHandle = await openWithoutFollowingSymlinkedAncestors( + resolvedFilePath, + constants.O_RDONLY | constants.O_NOFOLLOW, + node, + ); try { // Verify that the handle we've opened is the same as the path we checked earlier. @@ -202,19 +367,11 @@ export const getFileSystemHelperFunctions = (node: INode): FileSystemHelperFunct constants.O_NOFOLLOW | (userFlags & ~constants.O_TRUNC); // Strip O_TRUNC if present - let fileHandle; - try { - fileHandle = await fsOpen(resolvedFilePath, openFlags); - } catch (error) { - if ('code' in error && (error as unknown as { code: string }).code === 'ELOOP') { - throw new NodeOperationError(node, error instanceof Error ? error : '', { - message: 'Symlinks are not allowed.', - level: 'warning', - }); - } else { - throw error; - } - } + const fileHandle = await openWithoutFollowingSymlinkedAncestors( + resolvedFilePath, + openFlags, + node, + ); try { // Verify that the handle we've opened is the same as the path we checked earlier. @@ -279,6 +436,16 @@ export const getFileSystemHelperFunctions = (node: INode): FileSystemHelperFunct }, resolvePath, isFilePathBlocked, + async assertNoSymlinkInPath(resolvedFilePath) { + await assertNoSymlinkInPath(resolvedFilePath, node); + }, + async ensureParentDirectoryWithoutFollowingSymlinks(resolvedFilePath) { + await ensureParentDirectoryWithoutFollowingSymlinks(resolvedFilePath, node); + }, + resolveStagingBaseForTarget, + async pinDirectory(directoryPath, options) { + return await pinDirectory(directoryPath, node, options); + }, }); /** diff --git a/packages/frontend/@n8n/i18n/src/locales/en.json b/packages/frontend/@n8n/i18n/src/locales/en.json index f509562bef5..877e3cc71bb 100644 --- a/packages/frontend/@n8n/i18n/src/locales/en.json +++ b/packages/frontend/@n8n/i18n/src/locales/en.json @@ -2486,6 +2486,7 @@ "oauth.consentView.error.fetchDetails": "Error fetching client details", "oauth.consentView.error.heading": "Authorization unavailable", "oauth.consentView.error.resourceUnavailable": "This authorization can no longer be completed because the target is no longer available.", + "oauth.consentView.error.insufficientScope": "You do not have sufficient permissions to authorize this request.", "oauth.consentView.success.title": "Success", "oauth.consentView.success.description": "You will soon be redirected back to the client.", "parameterInput.expressionResult": "e.g. {result}", diff --git a/packages/frontend/editor-ui/src/app/stores/consent.store.ts b/packages/frontend/editor-ui/src/app/stores/consent.store.ts index 2e7e1f04b6e..a0fe033575d 100644 --- a/packages/frontend/editor-ui/src/app/stores/consent.store.ts +++ b/packages/frontend/editor-ui/src/app/stores/consent.store.ts @@ -11,7 +11,7 @@ export const useConsentStore = defineStore(STORES.CONSENT, () => { const consentDetails = ref(null); const isLoading = ref(false); const error = ref(null); - const errorCode: Ref<'resource_unavailable' | null> = ref(null); + const errorCode: Ref<'resource_unavailable' | 'forbidden' | null> = ref(null); const rootStore = useRootStore(); @@ -26,6 +26,8 @@ export const useConsentStore = defineStore(STORES.CONSENT, () => { } catch (err) { if (err instanceof ResponseError && err.httpStatusCode === 422) { errorCode.value = 'resource_unavailable'; + } else if (err instanceof ResponseError && err.httpStatusCode === 403) { + errorCode.value = 'forbidden'; } error.value = err instanceof Error ? err.message : 'Failed to load consent details'; throw err; diff --git a/packages/frontend/editor-ui/src/app/utils/htmlUtils.test.ts b/packages/frontend/editor-ui/src/app/utils/htmlUtils.test.ts index c31dfe3456e..7501bf622bc 100644 --- a/packages/frontend/editor-ui/src/app/utils/htmlUtils.test.ts +++ b/packages/frontend/editor-ui/src/app/utils/htmlUtils.test.ts @@ -1,5 +1,5 @@ import { ALLOWED_HTML_TAGS } from '@/app/constants'; -import { sanitizeHtml } from './htmlUtils'; +import { openSafeUrl, sanitizeHtml } from './htmlUtils'; describe('sanitizeHtml', () => { test.each(ALLOWED_HTML_TAGS)('should allow allowed HTML tag %s', (tag) => { @@ -82,3 +82,34 @@ describe('sanitizeHtml', () => { expect(sanitizeHtml(dirtyURL)).toBe(expected); }); }); + +describe('openSafeUrl', () => { + let openSpy: ReturnType; + + beforeEach(() => { + openSpy = vi.spyOn(window, 'open').mockImplementation(() => null); + }); + + afterEach(() => { + openSpy.mockRestore(); + }); + + test.each(['https://example.com', 'http://example.com/path?q=1', '/relative/path'])( + 'should open allowed url %s with noopener,noreferrer', + (url) => { + openSafeUrl(url); + expect(openSpy).toHaveBeenCalledWith(url, '_blank', 'noopener,noreferrer'); + }, + ); + + test.each([ + 'javascript:alert(1)', + 'JavaScript:alert(1)', + 'data:text/html,', + 'file:///etc/passwd', + 'vbscript:msgbox(1)', + ])('should not open disallowed url %s', (url) => { + openSafeUrl(url); + expect(openSpy).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/frontend/editor-ui/src/app/utils/htmlUtils.ts b/packages/frontend/editor-ui/src/app/utils/htmlUtils.ts index 76f0bc2e862..3cbb31bf551 100644 --- a/packages/frontend/editor-ui/src/app/utils/htmlUtils.ts +++ b/packages/frontend/editor-ui/src/app/utils/htmlUtils.ts @@ -37,6 +37,27 @@ export function sanitizeHtml(dirtyHtml: string) { return sanitizedHtml; } +const SAFE_OPEN_PROTOCOLS = ['http:', 'https:']; + +/** + * Opens a URL in a new tab, but only when it resolves to an http(s) protocol. + * Persisted, untrusted values (e.g. resource locator URLs) can carry schemes + * like `javascript:` or `data:`, so anything else is dropped. Always opens with + * `noopener,noreferrer` so the new tab cannot reach back to the opener window. + */ +export function openSafeUrl(url: string): void { + let protocol: string; + try { + protocol = new URL(url, window.location.origin).protocol; + } catch { + return; + } + + if (SAFE_OPEN_PROTOCOLS.includes(protocol)) { + window.open(url, '_blank', 'noopener,noreferrer'); + } +} + /** * Checks if the input is a string and sanitizes it by removing or escaping harmful characters, * returning the original input if it's not a string. diff --git a/packages/frontend/editor-ui/src/app/views/OAuthConsentView.vue b/packages/frontend/editor-ui/src/app/views/OAuthConsentView.vue index 41dbc7fbac4..21b49763320 100644 --- a/packages/frontend/editor-ui/src/app/views/OAuthConsentView.vue +++ b/packages/frontend/editor-ui/src/app/views/OAuthConsentView.vue @@ -34,6 +34,8 @@ const resourceName = computed(() => consentStore.consentDetails?.resourceName); const errorMessage = computed(() => { if (consentStore.errorCode === 'resource_unavailable') { return i18n.baseText('oauth.consentView.error.resourceUnavailable'); + } else if (consentStore.errorCode === 'forbidden') { + return i18n.baseText('oauth.consentView.error.insufficientScope'); } return consentStore.error; }); diff --git a/packages/frontend/editor-ui/src/features/ndv/parameters/components/ResourceLocator/ResourceLocator.test.ts b/packages/frontend/editor-ui/src/features/ndv/parameters/components/ResourceLocator/ResourceLocator.test.ts index c1b3b40d773..e33041f5177 100644 --- a/packages/frontend/editor-ui/src/features/ndv/parameters/components/ResourceLocator/ResourceLocator.test.ts +++ b/packages/frontend/editor-ui/src/features/ndv/parameters/components/ResourceLocator/ResourceLocator.test.ts @@ -478,6 +478,7 @@ describe('ResourceLocator', () => { expect(windowOpenSpy).toHaveBeenCalledWith( '/projects/test-project-123/datatables/new', '_blank', + 'noopener,noreferrer', ); expect(nodeTypesStore.getNodeParameterActionResult).not.toHaveBeenCalled(); @@ -518,9 +519,54 @@ describe('ResourceLocator', () => { expect(windowOpenSpy).toHaveBeenCalledWith( '/projects/home-project-456/datatables/new', '_blank', + 'noopener,noreferrer', ); }); + it('opens a list-mode cachedResultUrl with noopener,noreferrer', async () => { + const windowOpenSpy = vi.spyOn(window, 'open'); + const { getByTestId } = renderComponent({ + props: { + modelValue: { + __rl: true, + mode: 'list', + value: 'x', + cachedResultName: 'x', + cachedResultUrl: 'https://example.com/resource', + }, + }, + }); + + const link = await waitFor(() => getByTestId('rlc-open-resource-link')); + await userEvent.click(link); + + expect(windowOpenSpy).toHaveBeenCalledWith( + 'https://example.com/resource', + '_blank', + 'noopener,noreferrer', + ); + }); + + it('does not open a list-mode cachedResultUrl with a disallowed scheme', async () => { + const windowOpenSpy = vi.spyOn(window, 'open'); + const { getByTestId } = renderComponent({ + props: { + modelValue: { + __rl: true, + mode: 'list', + value: 'x', + cachedResultName: 'x', + cachedResultUrl: 'javascript:opener.document.body.dataset.xss="1";void 0', + }, + }, + }); + + const link = await waitFor(() => getByTestId('rlc-open-resource-link')); + await userEvent.click(link); + + expect(windowOpenSpy).not.toHaveBeenCalled(); + }); + it('clears cached resources after URL redirect so fresh data is fetched on re-open', async () => { const TEST_ITEMS = [{ name: 'Old Table', value: 'old-table' }]; const TEST_ITEMS_UPDATED = [ diff --git a/packages/frontend/editor-ui/src/features/ndv/parameters/components/ResourceLocator/ResourceLocator.vue b/packages/frontend/editor-ui/src/features/ndv/parameters/components/ResourceLocator/ResourceLocator.vue index 6241c3983be..fa38d4d0244 100644 --- a/packages/frontend/editor-ui/src/features/ndv/parameters/components/ResourceLocator/ResourceLocator.vue +++ b/packages/frontend/editor-ui/src/features/ndv/parameters/components/ResourceLocator/ResourceLocator.vue @@ -54,6 +54,7 @@ import { type FromAIOverride, } from '../../utils/fromAIOverride.utils'; import { completeExpressionSyntax } from '@/app/utils/expressions'; +import { openSafeUrl } from '@/app/utils/htmlUtils'; import { DEBOUNCE_TIME, ExpressionLocalResolveContextSymbol } from '@/app/constants'; import { useProjectsStore } from '@/features/collaboration/projects/projects.store'; import { injectWorkflowDocumentStore } from '@/app/stores/workflowDocument.store'; @@ -615,7 +616,7 @@ function onKeyDown(e: KeyboardEvent) { } function openResource(url: string) { - window.open(url, '_blank'); + openSafeUrl(url); trackEvent('User clicked resource locator link'); } @@ -1206,7 +1207,11 @@ function removeOverride() { :class="$style['parameter-issues']" />
- +
diff --git a/packages/frontend/editor-ui/src/features/ndv/runData/components/RunDataHtml.test.ts b/packages/frontend/editor-ui/src/features/ndv/runData/components/RunDataHtml.test.ts new file mode 100644 index 00000000000..1d04cf53c17 --- /dev/null +++ b/packages/frontend/editor-ui/src/features/ndv/runData/components/RunDataHtml.test.ts @@ -0,0 +1,48 @@ +import { createTestingPinia } from '@pinia/testing'; +import RunDataHtml from '@/features/ndv/runData/components/RunDataHtml.vue'; +import { renderComponent } from '@/__tests__/render'; + +describe('RunDataHtml.vue', () => { + it('should render the sanitized HTML into the preview iframe', () => { + const { container } = renderComponent(RunDataHtml, { + pinia: createTestingPinia(), + props: { + inputHtml: '

Hello World

', + }, + }); + + const iframe = container.querySelector('iframe'); + expect(iframe).toBeInTheDocument(); + expect(iframe?.getAttribute('srcdoc')).toContain('Hello'); + expect(iframe?.getAttribute('srcdoc')).toContain('World'); + }); + + it('should isolate the preview iframe from the parent context', () => { + const { container } = renderComponent(RunDataHtml, { + pinia: createTestingPinia(), + props: { + inputHtml: '

content

', + }, + }); + + const iframe = container.querySelector('iframe'); + // Empty sandbox disables scripts and same-origin access; combined with + // no-referrer this keeps previewed HTML from reaching the parent origin. + expect(iframe?.getAttribute('sandbox')).toBe(''); + expect(iframe?.getAttribute('referrerpolicy')).toBe('no-referrer'); + }); + + it('should keep the preview isolated even when sanitized markup retains attributes', () => { + // Some malformed inputs survive sanitization with attributes intact, so the + // sandbox is the boundary that must hold regardless of the rendered markup. + const { container } = renderComponent(RunDataHtml, { + pinia: createTestingPinia(), + props: { + inputHtml: '', + }, + }); + + const iframe = container.querySelector('iframe'); + expect(iframe?.getAttribute('sandbox')).toBe(''); + }); +}); diff --git a/packages/frontend/editor-ui/src/features/ndv/runData/components/RunDataHtml.vue b/packages/frontend/editor-ui/src/features/ndv/runData/components/RunDataHtml.vue index d120a0ad06f..23fc4f8a11e 100644 --- a/packages/frontend/editor-ui/src/features/ndv/runData/components/RunDataHtml.vue +++ b/packages/frontend/editor-ui/src/features/ndv/runData/components/RunDataHtml.vue @@ -32,7 +32,9 @@ export default {