From 576274ad79613320ffb4301b8e001fa103fb5be5 Mon Sep 17 00:00:00 2001 From: rangarajan19 Date: Fri, 2 Oct 2026 12:05:55 +0530 Subject: [PATCH 1/3] fix(connectors): fetch-and-attach real multipart file parts (#645) form-data bodies had no way to carry an actual file: appendFormParam coerced every value to a string, so a tool meant to upload an image (e.g. Etsy's uploadListingImage) sent the literal URL text as a form field instead of a real file part. Add a __file bodyMapping marker (bodyMapping: { image: { __file: "$image" } }), following the existing __raw/__spread convention. When the form-data branch sees it, it fetches the URL server-side through the existing SSRF guard, buffers it (capped by MAX_FILE_UPLOAD_BYTES, default 10MB, aborted early once crossed), and attaches it as a real multipart part with filename/content-type. Content-Disposition's filename wins when present; both it and the URL-derived fallback are sanitized the same way. Non-2xx responses from the file URL fail the tool call instead of attaching an error page as the file. The download is a bare request with none of the connector's own auth/headers/proxy config attached. form-urlencoded can't carry a file, so the marker is a config error there instead of being silently stringified. A form-data body backed by a file Buffer can only be read once; the OAuth2/LOGIN_TOKEN 401 auto-retry now rebuilds it from a small factory instead of resending the drained body. Frontend: a 'file' param type in the custom tool builder, selectable only when target is body and encoding is multipart/form-data. It emits the __file marker and is exposed to the model as { type: 'string', format: 'uri' }, since JSON Schema has no file type. --- docs/tool-definition.md | 22 ++ .../connectors/engines/rest.engine.spec.ts | 194 ++++++++++++++ .../src/connectors/engines/rest.engine.ts | 244 +++++++++++++++++- .../src/components/tool-editor/index.tsx | 64 ++++- 4 files changed, 509 insertions(+), 15 deletions(-) diff --git a/docs/tool-definition.md b/docs/tool-definition.md index 709675ff..8b0acd17 100644 --- a/docs/tool-definition.md +++ b/docs/tool-definition.md @@ -235,6 +235,28 @@ the identifier is itself a URL (a Search Console property such as `https://www.e URL), set `"encodePathParams": true` on the mapping and every substituted value is percent-encoded (`https%3A%2F%2Fwww.example.com%2F`). The model then passes the value raw, as the API returned it. +### File uploads (multipart/form-data) + +A `bodyMapping` entry can carry a `__file` marker instead of a plain `"$param"` string. +Only valid when `bodyEncoding` is `"form-data"`: + +```json +{ + "bodyEncoding": "form-data", + "bodyMapping": { + "image": { "__file": "$image_url" } + } +} +``` + +The tool input parameter (`image_url` here) is a **URL**, not file content — there is no way +for an MCP tool call to carry raw bytes. The connector fetches that URL server-side (through the +same SSRF guard as every other outbound request, capped by `MAX_FILE_UPLOAD_BYTES`, 10 MB by +default) and attaches the response as a real multipart part, with a filename and content-type +instead of a text field containing the URL. Declare the parameter with `"type": "string", +"format": "uri"` so the model knows to pass a link. `__file` in a `form-urlencoded` body is a +configuration error — that encoding cannot carry a file at all. + ### Response headers and pagination (`exposeHeaders`) By default a tool receives the response **body** and nothing else. Some APIs put diff --git a/packages/backend/src/connectors/engines/rest.engine.spec.ts b/packages/backend/src/connectors/engines/rest.engine.spec.ts index bdcdef30..23d01a5a 100644 --- a/packages/backend/src/connectors/engines/rest.engine.spec.ts +++ b/packages/backend/src/connectors/engines/rest.engine.spec.ts @@ -2,6 +2,8 @@ import { RestEngine, serializeRepeatedParams } from './rest.engine'; import { OAuth2TokenService } from './oauth2-token.service'; import { LoginTokenService } from './login-token.service'; import axios, { AxiosError } from 'axios'; +import FormData from 'form-data'; +import { Readable } from 'stream'; // Mock the callable default export but keep the real AxiosError class so the // engine's `instanceof AxiosError` checks (used by the retry logic) work. @@ -1404,6 +1406,198 @@ describe('RestEngine', () => { expect(retried.headers.Authorization).toBeUndefined(); }); }); + + describe('form-data file upload (__file)', () => { + function fileResponse(opts: { + status?: number; + chunks: Buffer[]; + headers?: Record; + }) { + return { + status: opts.status ?? 200, + headers: opts.headers ?? { 'content-type': 'image/jpeg' }, + data: Readable.from(opts.chunks), + }; + } + + it('fetches the __file URL and attaches it as a real multipart part, with no connector credentials on the download', async () => { + const bytes = Buffer.from('fake-image-bytes'); + mockedAxios + .mockResolvedValueOnce(fileResponse({ chunks: [bytes] })) + .mockResolvedValueOnce({ data: { ok: true } }); + const appendSpy = jest.spyOn(FormData.prototype, 'append'); + + const result = await engine.execute( + { + baseUrl: 'https://openapi.etsy.com', + authType: 'BEARER_TOKEN', + authConfig: { token: 'etsy-token' }, + }, + { + method: 'POST', + path: '/v3/application/shops/1/listings/2/images', + bodyEncoding: 'form-data', + bodyMapping: { image: { __file: '$image' } }, + }, + { image: 'https://cdn.example.com/photos/mug-123.jpg' }, + ); + + expect(result).toEqual({ ok: true }); + + // Call 0: the bare file download — no connector auth or headers attached. + const downloadCall = mockedAxios.mock.calls[0][0] as any; + expect(downloadCall.url).toBe( + 'https://cdn.example.com/photos/mug-123.jpg', + ); + expect(downloadCall.headers?.Authorization).toBeUndefined(); + + // Call 1: the real Etsy request, carrying its own auth and the file part. + const mainCall = mockedAxios.mock.calls[1][0] as any; + expect(mainCall.headers.Authorization).toBe('Bearer etsy-token'); + expect(appendSpy).toHaveBeenCalledWith( + 'image', + expect.any(Buffer), + expect.objectContaining({ + filename: 'mug-123.jpg', + contentType: 'image/jpeg', + }), + ); + + appendSpy.mockRestore(); + }); + + it('aborts the download once it crosses the size cap, without sending the real request', async () => { + const saved = process.env.MAX_FILE_UPLOAD_BYTES; + process.env.MAX_FILE_UPLOAD_BYTES = '10'; + try { + mockedAxios.mockResolvedValueOnce( + fileResponse({ chunks: [Buffer.alloc(20, 'a')] }), + ); + + await expect( + engine.execute( + { baseUrl: 'https://openapi.etsy.com', authType: 'NONE' }, + { + method: 'POST', + path: '/images', + bodyEncoding: 'form-data', + bodyMapping: { image: { __file: '$image' } }, + }, + { image: 'https://cdn.example.com/big.jpg' }, + ), + ).rejects.toThrow(/exceeds the \d+ MB upload limit/); + + // The real request never went out — only the aborted download did. + expect(mockedAxios).toHaveBeenCalledTimes(1); + } finally { + if (saved === undefined) delete process.env.MAX_FILE_UPLOAD_BYTES; + else process.env.MAX_FILE_UPLOAD_BYTES = saved; + } + }); + + it('fails the tool call when the file URL answers with a non-2xx status', async () => { + mockedAxios.mockResolvedValueOnce( + fileResponse({ status: 404, chunks: [Buffer.from('not found')] }), + ); + + await expect( + engine.execute( + { baseUrl: 'https://openapi.etsy.com', authType: 'NONE' }, + { + method: 'POST', + path: '/images', + bodyEncoding: 'form-data', + bodyMapping: { image: { __file: '$image' } }, + }, + { image: 'https://cdn.example.com/missing.jpg' }, + ), + ).rejects.toThrow(/HTTP 404/); + + expect(mockedAxios).toHaveBeenCalledTimes(1); + }); + + it('rejects a __file marker when encoding is form-urlencoded (it cannot carry a file)', async () => { + await expect( + engine.execute( + { baseUrl: 'https://api.example.com', authType: 'NONE' }, + { + method: 'POST', + path: '/x', + bodyEncoding: 'form-urlencoded', + bodyMapping: { image: { __file: '$image' } }, + }, + { image: 'https://cdn.example.com/photo.jpg' }, + ), + ).rejects.toThrow(/form-urlencoded cannot carry a file/); + + expect(mockedAxios).not.toHaveBeenCalled(); + }); + + describe('blocked host', () => { + const saved = process.env.SSRF_GUARD; + beforeEach(() => { + process.env.SSRF_GUARD = 'enabled'; + }); + afterEach(() => { + if (saved === undefined) delete process.env.SSRF_GUARD; + else process.env.SSRF_GUARD = saved; + }); + + it('refuses to download a __file URL that resolves to a blocked address', async () => { + await expect( + engine.execute( + { baseUrl: 'https://openapi.etsy.com', authType: 'NONE' }, + { + method: 'POST', + path: '/images', + bodyEncoding: 'form-data', + bodyMapping: { image: { __file: '$image' } }, + }, + { image: 'http://127.0.0.1/secret.jpg' }, + ), + ).rejects.toThrow(/SSRF guard/); + + expect(mockedAxios).not.toHaveBeenCalled(); + }); + }); + + it('rebuilds the multipart body with the same file on a 401 retry, fetching the file only once', async () => { + const bytes = Buffer.from('fake-image-bytes'); + const err = new AxiosError('Unauthorized'); + (err as any).response = { status: 401, data: {} }; + + mockedAxios + .mockResolvedValueOnce(fileResponse({ chunks: [bytes] })) // download + .mockRejectedValueOnce(err) // first attempt on Etsy -> 401 + .mockResolvedValueOnce({ data: { ok: true } }); // retry succeeds + + const result = await engine.execute( + { + baseUrl: 'https://openapi.etsy.com', + authType: 'OAUTH2', + authConfig: { + refreshToken: 'rt', + tokenUrl: 'https://openapi.etsy.com/token', + }, + }, + { + method: 'POST', + path: '/images', + bodyEncoding: 'form-data', + bodyMapping: { image: { __file: '$image' } }, + }, + { image: 'https://cdn.example.com/photos/mug-123.jpg' }, + ); + + expect(result).toEqual({ ok: true }); + // download(0) + first attempt(1) + retry(2) — the file itself was only fetched once. + expect(mockedAxios).toHaveBeenCalledTimes(3); + + const retryCall = mockedAxios.mock.calls[2][0] as any; + expect(retryCall.data).toBeInstanceOf(FormData); + expect(retryCall.headers.Authorization).toBe('Bearer new-access-token'); + }); + }); }); describe('RestEngine — bodyTemplate that will not parse', () => { diff --git a/packages/backend/src/connectors/engines/rest.engine.ts b/packages/backend/src/connectors/engines/rest.engine.ts index 57412fea..3283105d 100644 --- a/packages/backend/src/connectors/engines/rest.engine.ts +++ b/packages/backend/src/connectors/engines/rest.engine.ts @@ -250,6 +250,13 @@ export class RestEngine { if (encoding === 'form-urlencoded') { const urlParams = new URLSearchParams(); for (const [k, v] of spreadFormEntries(mapped)) { + if (isFileMarker(v)) { + throw new Error( + `bodyMapping.${k} uses the __file marker, but this tool's ` + + `encoding is 'form-urlencoded'. A file part needs ` + + `'multipart/form-data' — form-urlencoded cannot carry a file.`, + ); + } appendFormParam((key, val) => urlParams.append(key, val), k, v); } axiosConfig.data = urlParams.toString(); @@ -266,15 +273,47 @@ export class RestEngine { }; } } else if (encoding === 'form-data') { - const form = new FormData(); + const plainEntries: Array<[string, unknown]> = []; + const fileEntries: Array<{ key: string; file: FetchedFile }> = []; for (const [k, v] of spreadFormEntries(mapped)) { - appendFormParam((key, val) => form.append(key, val), k, v); + if (isFileMarker(v)) { + fileEntries.push({ + key: k, + file: await fetchFileForUpload(String(v.__file)), + }); + } else { + plainEntries.push([k, v]); + } } + // A FormData's underlying stream can only be read once. Rebuilding + // it from these already-resolved entries — instead of reusing the + // same instance — is what lets a 401 retry (OAuth2 refresh, + // LOGIN_TOKEN relogin) resend a complete body instead of an empty + // one. See rebuildRetriableBody. + const buildForm = (): FormData => { + const form = new FormData(); + for (const [k, v] of plainEntries) { + appendFormParam((key, val) => form.append(key, val), k, v); + } + for (const { key, file } of fileEntries) { + form.append(key, file.buffer, { + filename: file.filename, + contentType: file.contentType, + knownLength: file.buffer.length, + }); + } + return form; + }; + const form = buildForm(); axiosConfig.data = form; axiosConfig.headers = { ...axiosConfig.headers, ...form.getHeaders(), }; + if (fileEntries.length > 0) { + (axiosConfig as AxiosConfigWithFormRebuild).__rebuildFormData = + buildForm; + } } else { axiosConfig.data = mapped; } @@ -330,6 +369,7 @@ export class RestEngine { ...axiosConfig.headers, ...buildOauth2TokenHeader(config.authConfig, newToken), }; + this.rebuildRetriableBody(axiosConfig); const retryResponse = await axios(axiosConfig); return withMeta(retryResponse); } @@ -348,6 +388,7 @@ export class RestEngine { config.connectorId, ); injectLoginTokenHeaders(axiosConfig, authConfig, bundle.token, bundle.aud); + this.rebuildRetriableBody(axiosConfig); const retryResponse = await axios(axiosConfig); return withMeta(retryResponse); } @@ -463,6 +504,23 @@ export class RestEngine { } } + /** + * A form-data body built with a file part carries a Buffer-backed stream + * that is drained the first time it is sent. Without rebuilding it, the + * OAuth2/LOGIN_TOKEN 401 auto-retry would resend an empty body instead of + * the file. Only set when the body actually contains a `__file` part (see + * the form-data branch in executeWithMeta) — every other body keeps + * reusing axiosConfig.data exactly as before. + */ + private rebuildRetriableBody(axiosConfig: AxiosRequestConfig): void { + const rebuild = (axiosConfig as AxiosConfigWithFormRebuild) + .__rebuildFormData; + if (!rebuild) return; + const form = rebuild(); + axiosConfig.data = form; + axiosConfig.headers = { ...axiosConfig.headers, ...form.getHeaders() }; + } + private async injectAuth( axiosConfig: AxiosRequestConfig, config: { @@ -944,6 +1002,188 @@ function appendFormParam( sink(key, String(value)); } +/** + * A `{ __file: "" }` wrapper in a form-data bodyMapping entry — the + * signal that this field must be fetched and attached as a real file part + * instead of stringified, mirroring the existing __raw/__spread convention. + */ +function isFileMarker(value: unknown): value is { __file: unknown } { + return ( + value !== null && + typeof value === 'object' && + !Array.isArray(value) && + '__file' in (value as Record) + ); +} + +interface FetchedFile { + buffer: Buffer; + filename: string; + contentType: string; +} + +/** Carries the retry-rebuild hook introduced for form-data file parts. */ +type AxiosConfigWithFormRebuild = AxiosRequestConfig & { + __rebuildFormData?: () => FormData; +}; + +const DEFAULT_MAX_FILE_UPLOAD_BYTES = 10 * 1024 * 1024; + +function getMaxFileUploadBytes(env: NodeJS.ProcessEnv = process.env): number { + const parsed = Number(env.MAX_FILE_UPLOAD_BYTES); + return Number.isFinite(parsed) && parsed > 0 + ? parsed + : DEFAULT_MAX_FILE_UPLOAD_BYTES; +} + +const fileFetchLogger = new Logger('RestEngine:file-upload'); + +/** + * Fetch a `__file` URL and return it as a ready-to-attach multipart part. + * + * This is a plain, bare request to a third party the connector has no + * relationship with — none of the connector's auth, headers or proxy config + * are attached, and the URL's query string (which a presigned link uses to + * carry its credentials) is never logged. + */ +async function fetchFileForUpload(url: string): Promise { + await assertSafeOutboundUrl(url); + + let parsed: URL; + try { + parsed = new URL(url); + } catch { + throw new Error(`__file value is not a valid URL: '${url}'`); + } + fileFetchLogger.debug( + `Fetching file for upload: ${parsed.origin}${parsed.pathname}`, + ); + + const response = await axios({ + method: 'GET', + url, + responseType: 'stream', + timeout: 30000, + validateStatus: () => true, + }); + + if (response.status < 200 || response.status >= 300) { + throw new Error( + `Could not download the file for upload: the URL answered with HTTP ${response.status}.`, + ); + } + + const maxBytes = getMaxFileUploadBytes(); + const buffer = await readStreamWithLimit( + response.data as NodeJS.ReadableStream, + maxBytes, + ); + const headers = (response.headers ?? {}) as Record; + const contentType = String( + headers['content-type'] ?? 'application/octet-stream', + ); + const filename = resolveUploadFilename(url, headers); + + return { buffer, filename, contentType }; +} + +/** Buffers a stream, aborting as soon as `maxBytes` is crossed. */ +function readStreamWithLimit( + stream: NodeJS.ReadableStream, + maxBytes: number, +): Promise { + return new Promise((resolve, reject) => { + const chunks: Buffer[] = []; + let total = 0; + let settled = false; + + const cleanup = () => { + stream.removeListener('data', onData); + stream.removeListener('end', onEnd); + stream.removeListener('error', onError); + }; + const finish = (err: Error | null, buffer?: Buffer) => { + if (settled) return; + settled = true; + cleanup(); + if (err) reject(err); + else resolve(buffer as Buffer); + }; + const onData = (chunk: Buffer) => { + total += chunk.length; + if (total > maxBytes) { + const mb = Math.floor(maxBytes / (1024 * 1024)); + (stream as unknown as { destroy?: () => void }).destroy?.(); + finish( + new Error( + `The file exceeds the ${mb} MB upload limit (set MAX_FILE_UPLOAD_BYTES to raise it).`, + ), + ); + return; + } + chunks.push(chunk); + }; + const onEnd = () => finish(null, Buffer.concat(chunks)); + const onError = (err: Error) => finish(err); + + stream.on('data', onData); + stream.on('end', onEnd); + stream.on('error', onError); + }); +} + +/** + * Filename for the attached part: the response's own Content-Disposition + * wins when present, otherwise the URL's last path segment. Both go through + * the same sanitizing so neither can inject a path separator or an unsafe + * character into the multipart part. + */ +function resolveUploadFilename( + url: string, + headers: Record, +): string { + const fromDisposition = filenameFromContentDisposition( + String(headers['content-disposition'] ?? ''), + ); + return fromDisposition ?? filenameFromUrl(url); +} + +function sanitizeFilename(name: string): string { + const cleaned = name.trim().replace(/[^A-Za-z0-9._-]/g, '_'); + return cleaned || 'file'; +} + +function filenameFromUrl(url: string): string { + let pathname: string; + try { + pathname = new URL(url).pathname; + } catch { + return 'file'; + } + const last = pathname.split('/').filter(Boolean).pop() ?? ''; + let decoded = last; + try { + decoded = decodeURIComponent(last); + } catch { + /* not validly percent-encoded — sanitize the raw segment instead */ + } + return sanitizeFilename(decoded); +} + +function filenameFromContentDisposition(value: string): string | undefined { + const match = /filename\*?=(?:"([^"]+)"|([^;]+))/i.exec(value); + const raw = match?.[1] ?? match?.[2]; + if (!raw) return undefined; + const stripped = raw.replace(/^UTF-8''/i, ''); + let decoded = stripped; + try { + decoded = decodeURIComponent(stripped); + } catch { + /* not validly percent-encoded — sanitize the raw value instead */ + } + return sanitizeFilename(decoded); +} + /** * Param names that, if interpolated into a JSON body, can poison the * Object.prototype chain after JSON.parse and let an attacker forge diff --git a/packages/frontend/src/components/tool-editor/index.tsx b/packages/frontend/src/components/tool-editor/index.tsx index bd53c712..262d378f 100644 --- a/packages/frontend/src/components/tool-editor/index.tsx +++ b/packages/frontend/src/components/tool-editor/index.tsx @@ -19,7 +19,13 @@ import type { ResponseTransform } from '@/lib/api'; export interface ToolParam { name: string; - type: 'string' | 'number' | 'boolean' | 'integer' | 'array' | 'object'; + /** + * 'file': a public URL the connector fetches server-side and attaches as + * a real multipart part — only valid when target is 'body' and the body + * encoding is 'form-data'. There is no JSON Schema "file" type, so this + * is exposed to the model as `{ type: 'string', format: 'uri' }`. + */ + type: 'string' | 'number' | 'boolean' | 'integer' | 'array' | 'object' | 'file'; description: string; required: boolean; /** Where this param is injected into the API request */ @@ -227,18 +233,33 @@ function parseExistingTool( } // Parse bodyMapping. The field editor can only express a flat - // "param -> $param" mapping; anything else (nested objects, arrays, literal - // values, {{variables}}) is kept as raw JSON so saving from the GUI cannot - // silently discard it. + // "param -> $param" mapping — plus the `{ __file: "$param" }` wrapper a + // 'file' param produces — so anything else (nested objects, arrays, + // literal values, {{variables}}) is kept as raw JSON instead, so saving + // from the GUI cannot silently discard it. let detectedBodyMappingJson: string | undefined; + const fileMapped = new Set(); if (em.bodyMapping) { const entries = Object.entries(em.bodyMapping as Record); + const isFileEntry = (value: unknown): value is { __file: string } => + !!value && + typeof value === 'object' && + typeof (value as { __file?: unknown }).__file === 'string' && + (value as { __file: string }).__file.startsWith('$'); const isSimple = entries.every( - ([, value]) => typeof value === 'string' && value.startsWith('$'), + ([, value]) => + (typeof value === 'string' && value.startsWith('$')) || + isFileEntry(value), ); if (isSimple) { for (const [, value] of entries) { - bodyMapped.add((value as string).substring(1)); + if (isFileEntry(value)) { + const paramName = value.__file.substring(1); + bodyMapped.add(paramName); + fileMapped.add(paramName); + } else { + bodyMapped.add((value as string).substring(1)); + } } } else if (entries.length > 0) { detectedBodyMappingJson = JSON.stringify(em.bodyMapping, null, 2); @@ -293,7 +314,7 @@ function parseExistingTool( params.push({ name, - type: p.type || 'string', + type: fileMapped.has(name) ? 'file' : p.type || 'string', description: p.description || '', required: required.includes(name), target, @@ -354,10 +375,21 @@ function buildToolPayload(data: ToolEditorData, connectorType: string) { const required: string[] = []; for (const p of data.params) { - properties[p.name] = { - type: p.type, - ...(p.description ? { description: p.description } : {}), - }; + // JSON Schema has no "file" type — a 'file' param is exposed to the + // model as a URL string it must link to, not inline content. + properties[p.name] = + p.type === 'file' + ? { + type: 'string', + format: 'uri', + description: + p.description || + "A public URL to the file to upload (a link, not the file's raw content).", + } + : { + type: p.type, + ...(p.description ? { description: p.description } : {}), + }; if (p.required) { required.push(p.name); } @@ -371,7 +403,7 @@ function buildToolPayload(data: ToolEditorData, connectorType: string) { // Build endpointMapping based on connector type const queryParams: Record = {}; - const bodyMapping: Record = {}; + const bodyMapping: Record = {}; const headers: Record = {}; for (const p of data.params) { @@ -383,7 +415,8 @@ function buildToolPayload(data: ToolEditorData, connectorType: string) { case 'body': case 'soap': if (!(data.useBodyTemplate && data.bodyTemplate)) { - bodyMapping[p.name] = `$${p.name}`; + bodyMapping[p.name] = + p.type === 'file' ? { __file: `$${p.name}` } : `$${p.name}`; } break; case 'header': @@ -1036,6 +1069,11 @@ export function ToolEditor({ { value: 'boolean', label: 'boolean' }, { value: 'array', label: 'array' }, { value: 'object', label: 'object' }, + // A file part only makes sense in a multipart body — + // form-urlencoded and JSON can't carry one. + ...(param.target === 'body' && bodyEncoding === 'form-data' + ? [{ value: 'file', label: 'file (from URL)' }] + : []), ]} /> From db0469329aa64df027192a5f73c6b59ded32e50c Mon Sep 17 00:00:00 2001 From: rangarajan19 Date: Sat, 3 Oct 2026 12:51:21 +0530 Subject: [PATCH 2/3] fix(connectors): use the shared fetchOutbound helper for file uploads Replaces the plain axios call + manual stream-cap in fetchFileForUpload with fetchOutbound() (#821), which already covers the SSRF check on every redirect hop, the size cap, the timeout and non-2xx handling. Filename resolution now uses the post-redirect finalUrl rather than the original URL, so a short link resolves to the real file's name. Tests mock fetchOutbound directly rather than the raw axios call. --- .../connectors/engines/rest.engine.spec.ts | 209 +++++++++++------- .../src/connectors/engines/rest.engine.ts | 97 ++------ 2 files changed, 143 insertions(+), 163 deletions(-) diff --git a/packages/backend/src/connectors/engines/rest.engine.spec.ts b/packages/backend/src/connectors/engines/rest.engine.spec.ts index 23d01a5a..3b06230f 100644 --- a/packages/backend/src/connectors/engines/rest.engine.spec.ts +++ b/packages/backend/src/connectors/engines/rest.engine.spec.ts @@ -3,7 +3,20 @@ import { OAuth2TokenService } from './oauth2-token.service'; import { LoginTokenService } from './login-token.service'; import axios, { AxiosError } from 'axios'; import FormData from 'form-data'; -import { Readable } from 'stream'; +import { fetchOutbound, OutboundFetchError } from '../../common/outbound-fetch.util'; +import { SsrfBlockedError } from '../../common/ssrf.util'; + +// The file-upload path delegates its download entirely to fetchOutbound +// (SSRF checks, size cap, timeout, non-2xx handling all live there — see +// outbound-fetch.util.spec.ts); these tests only need to control what it +// resolves or rejects with. +jest.mock('../../common/outbound-fetch.util', () => ({ + ...jest.requireActual('../../common/outbound-fetch.util'), + fetchOutbound: jest.fn(), +})); +const mockedFetchOutbound = fetchOutbound as jest.MockedFunction< + typeof fetchOutbound +>; // Mock the callable default export but keep the real AxiosError class so the // engine's `instanceof AxiosError` checks (used by the retry logic) work. @@ -1408,23 +1421,28 @@ describe('RestEngine', () => { }); describe('form-data file upload (__file)', () => { - function fileResponse(opts: { - status?: number; - chunks: Buffer[]; + function fetchResult(opts: { + body: Buffer; headers?: Record; + finalUrl: string; }) { return { - status: opts.status ?? 200, + status: 200, headers: opts.headers ?? { 'content-type': 'image/jpeg' }, - data: Readable.from(opts.chunks), + body: opts.body, + finalUrl: opts.finalUrl, }; } - it('fetches the __file URL and attaches it as a real multipart part, with no connector credentials on the download', async () => { + it('fetches the __file URL via fetchOutbound and attaches it as a real multipart part, with no connector credentials on the download', async () => { const bytes = Buffer.from('fake-image-bytes'); - mockedAxios - .mockResolvedValueOnce(fileResponse({ chunks: [bytes] })) - .mockResolvedValueOnce({ data: { ok: true } }); + mockedFetchOutbound.mockResolvedValueOnce( + fetchResult({ + body: bytes, + finalUrl: 'https://cdn.example.com/photos/mug-123.jpg', + }), + ); + mockedAxios.mockResolvedValueOnce({ data: { ok: true } }); const appendSpy = jest.spyOn(FormData.prototype, 'append'); const result = await engine.execute( @@ -1444,15 +1462,15 @@ describe('RestEngine', () => { expect(result).toEqual({ ok: true }); - // Call 0: the bare file download — no connector auth or headers attached. - const downloadCall = mockedAxios.mock.calls[0][0] as any; - expect(downloadCall.url).toBe( + // Called with only maxBytes/timeoutMs — no connector headers, auth or + // proxy config reach the download at all. + expect(mockedFetchOutbound).toHaveBeenCalledWith( 'https://cdn.example.com/photos/mug-123.jpg', + { maxBytes: 10 * 1024 * 1024, timeoutMs: 30000 }, ); - expect(downloadCall.headers?.Authorization).toBeUndefined(); - // Call 1: the real Etsy request, carrying its own auth and the file part. - const mainCall = mockedAxios.mock.calls[1][0] as any; + // The real Etsy request still carries its own auth and the file part. + const mainCall = mockedAxios.mock.calls[0][0] as any; expect(mainCall.headers.Authorization).toBe('Bearer etsy-token'); expect(appendSpy).toHaveBeenCalledWith( 'image', @@ -1466,38 +1484,68 @@ describe('RestEngine', () => { appendSpy.mockRestore(); }); - it('aborts the download once it crosses the size cap, without sending the real request', async () => { - const saved = process.env.MAX_FILE_UPLOAD_BYTES; - process.env.MAX_FILE_UPLOAD_BYTES = '10'; - try { - mockedAxios.mockResolvedValueOnce( - fileResponse({ chunks: [Buffer.alloc(20, 'a')] }), - ); - - await expect( - engine.execute( - { baseUrl: 'https://openapi.etsy.com', authType: 'NONE' }, - { - method: 'POST', - path: '/images', - bodyEncoding: 'form-data', - bodyMapping: { image: { __file: '$image' } }, - }, - { image: 'https://cdn.example.com/big.jpg' }, - ), - ).rejects.toThrow(/exceeds the \d+ MB upload limit/); - - // The real request never went out — only the aborted download did. - expect(mockedAxios).toHaveBeenCalledTimes(1); - } finally { - if (saved === undefined) delete process.env.MAX_FILE_UPLOAD_BYTES; - else process.env.MAX_FILE_UPLOAD_BYTES = saved; - } + it('names the part after the redirected finalUrl, not the original short link', async () => { + mockedFetchOutbound.mockResolvedValueOnce( + fetchResult({ + body: Buffer.from('pdf-bytes'), + headers: { 'content-type': 'application/pdf' }, + finalUrl: 'https://cdn.example.com/files/report.pdf', + }), + ); + mockedAxios.mockResolvedValueOnce({ data: { ok: true } }); + const appendSpy = jest.spyOn(FormData.prototype, 'append'); + + await engine.execute( + { baseUrl: 'https://api.example.com', authType: 'NONE' }, + { + method: 'POST', + path: '/upload', + bodyEncoding: 'form-data', + bodyMapping: { doc: { __file: '$doc' } }, + }, + { doc: 'https://short.ly/abc123' }, + ); + + expect(appendSpy).toHaveBeenCalledWith( + 'doc', + expect.any(Buffer), + expect.objectContaining({ filename: 'report.pdf' }), + ); + + appendSpy.mockRestore(); + }); + + it('propagates a too-large failure from fetchOutbound without sending the real request', async () => { + mockedFetchOutbound.mockRejectedValueOnce( + new OutboundFetchError( + 'https://cdn.example.com/big.jpg is larger than 10 MB.', + 'too_large', + ), + ); + + await expect( + engine.execute( + { baseUrl: 'https://openapi.etsy.com', authType: 'NONE' }, + { + method: 'POST', + path: '/images', + bodyEncoding: 'form-data', + bodyMapping: { image: { __file: '$image' } }, + }, + { image: 'https://cdn.example.com/big.jpg' }, + ), + ).rejects.toThrow(/is larger than 10 MB/); + + expect(mockedAxios).not.toHaveBeenCalled(); }); - it('fails the tool call when the file URL answers with a non-2xx status', async () => { - mockedAxios.mockResolvedValueOnce( - fileResponse({ status: 404, chunks: [Buffer.from('not found')] }), + it('propagates a non-2xx failure from fetchOutbound without sending the real request', async () => { + mockedFetchOutbound.mockRejectedValueOnce( + new OutboundFetchError( + 'https://cdn.example.com/missing.jpg answered HTTP 404.', + 'status', + 404, + ), ); await expect( @@ -1513,7 +1561,30 @@ describe('RestEngine', () => { ), ).rejects.toThrow(/HTTP 404/); - expect(mockedAxios).toHaveBeenCalledTimes(1); + expect(mockedAxios).not.toHaveBeenCalled(); + }); + + it('propagates a blocked-host failure from fetchOutbound without sending the real request', async () => { + mockedFetchOutbound.mockRejectedValueOnce( + new SsrfBlockedError( + "SSRF guard: address '127.0.0.1' is not a public IP", + ), + ); + + await expect( + engine.execute( + { baseUrl: 'https://openapi.etsy.com', authType: 'NONE' }, + { + method: 'POST', + path: '/images', + bodyEncoding: 'form-data', + bodyMapping: { image: { __file: '$image' } }, + }, + { image: 'http://127.0.0.1/secret.jpg' }, + ), + ).rejects.toThrow(/SSRF guard/); + + expect(mockedAxios).not.toHaveBeenCalled(); }); it('rejects a __file marker when encoding is form-urlencoded (it cannot carry a file)', async () => { @@ -1530,44 +1601,22 @@ describe('RestEngine', () => { ), ).rejects.toThrow(/form-urlencoded cannot carry a file/); + expect(mockedFetchOutbound).not.toHaveBeenCalled(); expect(mockedAxios).not.toHaveBeenCalled(); }); - describe('blocked host', () => { - const saved = process.env.SSRF_GUARD; - beforeEach(() => { - process.env.SSRF_GUARD = 'enabled'; - }); - afterEach(() => { - if (saved === undefined) delete process.env.SSRF_GUARD; - else process.env.SSRF_GUARD = saved; - }); - - it('refuses to download a __file URL that resolves to a blocked address', async () => { - await expect( - engine.execute( - { baseUrl: 'https://openapi.etsy.com', authType: 'NONE' }, - { - method: 'POST', - path: '/images', - bodyEncoding: 'form-data', - bodyMapping: { image: { __file: '$image' } }, - }, - { image: 'http://127.0.0.1/secret.jpg' }, - ), - ).rejects.toThrow(/SSRF guard/); - - expect(mockedAxios).not.toHaveBeenCalled(); - }); - }); - it('rebuilds the multipart body with the same file on a 401 retry, fetching the file only once', async () => { const bytes = Buffer.from('fake-image-bytes'); const err = new AxiosError('Unauthorized'); (err as any).response = { status: 401, data: {} }; + mockedFetchOutbound.mockResolvedValueOnce( + fetchResult({ + body: bytes, + finalUrl: 'https://cdn.example.com/photos/mug-123.jpg', + }), + ); mockedAxios - .mockResolvedValueOnce(fileResponse({ chunks: [bytes] })) // download .mockRejectedValueOnce(err) // first attempt on Etsy -> 401 .mockResolvedValueOnce({ data: { ok: true } }); // retry succeeds @@ -1590,10 +1639,12 @@ describe('RestEngine', () => { ); expect(result).toEqual({ ok: true }); - // download(0) + first attempt(1) + retry(2) — the file itself was only fetched once. - expect(mockedAxios).toHaveBeenCalledTimes(3); + // The file itself was only fetched once, even though the main request + // (first attempt + retry) went out twice. + expect(mockedFetchOutbound).toHaveBeenCalledTimes(1); + expect(mockedAxios).toHaveBeenCalledTimes(2); - const retryCall = mockedAxios.mock.calls[2][0] as any; + const retryCall = mockedAxios.mock.calls[1][0] as any; expect(retryCall.data).toBeInstanceOf(FormData); expect(retryCall.headers.Authorization).toBe('Bearer new-access-token'); }); diff --git a/packages/backend/src/connectors/engines/rest.engine.ts b/packages/backend/src/connectors/engines/rest.engine.ts index 3283105d..7075b60f 100644 --- a/packages/backend/src/connectors/engines/rest.engine.ts +++ b/packages/backend/src/connectors/engines/rest.engine.ts @@ -15,6 +15,7 @@ import { LoginTokenAuthConfig, } from './login-token.service'; import { assertSafeOutboundUrl } from '../../common/ssrf.util'; +import { fetchOutbound } from '../../common/outbound-fetch.util'; import { assertNoUnresolvedPlaceholders } from '../../common/unresolved-placeholders.util'; import { XMLParser } from 'fast-xml-parser'; import { pickExposedHeaders } from './response-headers.util'; @@ -1036,100 +1037,28 @@ function getMaxFileUploadBytes(env: NodeJS.ProcessEnv = process.env): number { : DEFAULT_MAX_FILE_UPLOAD_BYTES; } -const fileFetchLogger = new Logger('RestEngine:file-upload'); - /** * Fetch a `__file` URL and return it as a ready-to-attach multipart part. * - * This is a plain, bare request to a third party the connector has no - * relationship with — none of the connector's auth, headers or proxy config - * are attached, and the URL's query string (which a presigned link uses to - * carry its credentials) is never logged. + * The download itself — SSRF checks on the URL and every redirect, no + * connector credentials, the size cap, the timeout, non-2xx as an error — + * is entirely fetchOutbound's job; this only resolves the filename and + * content-type once the bytes are back. */ async function fetchFileForUpload(url: string): Promise { - await assertSafeOutboundUrl(url); - - let parsed: URL; - try { - parsed = new URL(url); - } catch { - throw new Error(`__file value is not a valid URL: '${url}'`); - } - fileFetchLogger.debug( - `Fetching file for upload: ${parsed.origin}${parsed.pathname}`, - ); - - const response = await axios({ - method: 'GET', - url, - responseType: 'stream', - timeout: 30000, - validateStatus: () => true, + const result = await fetchOutbound(url, { + maxBytes: getMaxFileUploadBytes(), + timeoutMs: 30000, }); - if (response.status < 200 || response.status >= 300) { - throw new Error( - `Could not download the file for upload: the URL answered with HTTP ${response.status}.`, - ); - } - - const maxBytes = getMaxFileUploadBytes(); - const buffer = await readStreamWithLimit( - response.data as NodeJS.ReadableStream, - maxBytes, - ); - const headers = (response.headers ?? {}) as Record; const contentType = String( - headers['content-type'] ?? 'application/octet-stream', + result.headers['content-type'] ?? 'application/octet-stream', ); - const filename = resolveUploadFilename(url, headers); + // finalUrl, not the original url: a short link redirecting to + // /files/report.pdf should name the part after the real file. + const filename = resolveUploadFilename(result.finalUrl, result.headers); - return { buffer, filename, contentType }; -} - -/** Buffers a stream, aborting as soon as `maxBytes` is crossed. */ -function readStreamWithLimit( - stream: NodeJS.ReadableStream, - maxBytes: number, -): Promise { - return new Promise((resolve, reject) => { - const chunks: Buffer[] = []; - let total = 0; - let settled = false; - - const cleanup = () => { - stream.removeListener('data', onData); - stream.removeListener('end', onEnd); - stream.removeListener('error', onError); - }; - const finish = (err: Error | null, buffer?: Buffer) => { - if (settled) return; - settled = true; - cleanup(); - if (err) reject(err); - else resolve(buffer as Buffer); - }; - const onData = (chunk: Buffer) => { - total += chunk.length; - if (total > maxBytes) { - const mb = Math.floor(maxBytes / (1024 * 1024)); - (stream as unknown as { destroy?: () => void }).destroy?.(); - finish( - new Error( - `The file exceeds the ${mb} MB upload limit (set MAX_FILE_UPLOAD_BYTES to raise it).`, - ), - ); - return; - } - chunks.push(chunk); - }; - const onEnd = () => finish(null, Buffer.concat(chunks)); - const onError = (err: Error) => finish(err); - - stream.on('data', onData); - stream.on('end', onEnd); - stream.on('error', onError); - }); + return { buffer: result.body, filename, contentType }; } /** From a996cfd984f2751e89ce14b6d62ab06e5d9fda35 Mon Sep 17 00:00:00 2001 From: keysersoft Date: Sat, 3 Oct 2026 18:13:53 +0200 Subject: [PATCH 3/3] Only fields the tool declares may fetch a file; skip an optional file left out A $param resolves to the caller's value as-is, objects included, and __spread lets the caller choose the keys. So a model could make any form-data tool download a URL of its choosing by sending { "__file": "https://..." }. The marker now counts only where the tool's own bodyMapping writes it; one arriving in the arguments fails the call. A declared file the caller did not pass is left out of the form. --- .../connectors/engines/rest.engine.spec.ts | 54 +++++++++++++++++++ .../src/connectors/engines/rest.engine.ts | 23 ++++++++ 2 files changed, 77 insertions(+) diff --git a/packages/backend/src/connectors/engines/rest.engine.spec.ts b/packages/backend/src/connectors/engines/rest.engine.spec.ts index 909b4eab..89b94fcc 100644 --- a/packages/backend/src/connectors/engines/rest.engine.spec.ts +++ b/packages/backend/src/connectors/engines/rest.engine.spec.ts @@ -1630,6 +1630,60 @@ describe('RestEngine', () => { expect(mockedAxios).not.toHaveBeenCalled(); }); + it('refuses a __file marker that arrives in the call arguments instead of the tool config', async () => { + // `meta: "$meta"` passes the caller's object through as-is; a model must + // not be able to turn an ordinary form field into a server-side download. + await expect( + engine.execute( + { baseUrl: 'https://api.example.com', authType: 'NONE' }, + { + method: 'POST', + path: '/x', + bodyEncoding: 'form-data', + bodyMapping: { meta: '$meta' }, + }, + { meta: { __file: 'https://attacker.example/payload.bin' } }, + ), + ).rejects.toThrow(/Files can only be fetched for fields the tool itself declares/); + + // Same through __spread, where the caller chooses the keys. + await expect( + engine.execute( + { baseUrl: 'https://api.example.com', authType: 'NONE' }, + { + method: 'POST', + path: '/x', + bodyEncoding: 'form-data', + bodyMapping: { __spread: '$params' }, + }, + { params: { image: { __file: 'https://attacker.example/payload.bin' } } }, + ), + ).rejects.toThrow(/Files can only be fetched/); + + expect(mockedFetchOutbound).not.toHaveBeenCalled(); + expect(mockedAxios).not.toHaveBeenCalled(); + }); + + it('leaves out an optional file part the caller did not pass', async () => { + mockedAxios.mockResolvedValueOnce({ data: { ok: true } }); + const appendSpy = jest.spyOn(FormData.prototype, 'append'); + + await engine.execute( + { baseUrl: 'https://api.example.com', authType: 'NONE' }, + { + method: 'POST', + path: '/x', + bodyEncoding: 'form-data', + bodyMapping: { title: '$title', image: { __file: '$image' } }, + }, + { title: 'Mug' }, + ); + + expect(mockedFetchOutbound).not.toHaveBeenCalled(); + expect(appendSpy.mock.calls.map((c) => c[0])).toEqual(['title']); + appendSpy.mockRestore(); + }); + it('rejects a __file marker when encoding is form-urlencoded (it cannot carry a file)', async () => { await expect( engine.execute( diff --git a/packages/backend/src/connectors/engines/rest.engine.ts b/packages/backend/src/connectors/engines/rest.engine.ts index 2762a943..cd8d7ea0 100644 --- a/packages/backend/src/connectors/engines/rest.engine.ts +++ b/packages/backend/src/connectors/engines/rest.engine.ts @@ -268,6 +268,11 @@ export class RestEngine { } else { const mapped = this.mapParams(endpointMapping.bodyMapping, params); const encoding = endpointMapping.bodyEncoding || 'json'; + // Only fields whose marker is written in the tool's own bodyMapping + // may fetch a file. A `$param` resolves to whatever the caller sent, + // objects included, so without this a model could make any + // form-data tool download a URL of its choosing. + const fileKeys = configuredFileKeys(endpointMapping.bodyMapping); if (encoding === 'form-urlencoded') { const urlParams = new URLSearchParams(); @@ -299,6 +304,15 @@ export class RestEngine { const fileEntries: Array<{ key: string; file: FetchedFile }> = []; for (const [k, v] of spreadFormEntries(mapped)) { if (isFileMarker(v)) { + if (!fileKeys.has(k)) { + throw new Error( + `bodyMapping.${k} received a __file marker from the call's ` + + `arguments. Files can only be fetched for fields the tool ` + + `itself declares with { "__file": "$param" }.`, + ); + } + // An optional file the caller did not pass: leave the part out. + if (v.__file === undefined || v.__file === null || v.__file === '') continue; fileEntries.push({ key: k, file: await fetchFileForUpload(String(v.__file)), @@ -1067,6 +1081,15 @@ function isFileMarker(value: unknown): value is { __file: unknown } { ); } +/** Top-level bodyMapping keys whose template is a `{ __file: ... }` marker. */ +function configuredFileKeys(bodyMapping: Record): Set { + return new Set( + Object.entries(bodyMapping) + .filter(([, template]) => isFileMarker(template)) + .map(([key]) => key), + ); +} + interface FetchedFile { buffer: Buffer; filename: string;