improvement(mothership): add file patch tool#3712
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
@cursor review |
PR SummaryHigh Risk Overview Introduces a dedicated PPTX preview endpoint and UI rendering. A new Expands Copilot file tooling and UI metadata. Written by Cursor Bugbot for commit cc214cd. Configure here. |
Greptile SummaryThis PR adds PPTX authoring capability to the Mothership copilot: the AI can now write/patch PptxGenJS source files ( Key changes:
Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant AI as Copilot AI
participant Router as Server Tool Router
participant WF as workspace_file tool
participant DL as download_to_workspace_file tool
participant VM as pptx-vm.ts (main process)
participant W as pptx-worker.cjs (subprocess)
participant Serve as /api/files/serve
Note over AI,Serve: Write / patch a .pptx source file
AI->>Router: workspace_file {op: write|patch, .pptx}
Router->>WF: execute()
WF->>WF: store raw PptxGenJS code as text/x-pptxgenjs
WF-->>AI: {success, fileId}
Note over AI,Serve: Download external file into workspace
AI->>Router: download_to_workspace_file {url}
Router->>DL: execute()
DL->>DL: isPrivateUrl(url) check
DL->>DL: fetch(url, redirect:'follow')
DL->>DL: uploadWorkspaceFile()
DL-->>AI: {success, fileId}
Note over AI,Serve: Serve/compile .pptx on demand
AI->>Serve: GET /api/files/serve/workspace/{id}/file.pptx
Serve->>Serve: compilePptxIfNeeded()
Serve->>VM: generatePptxFromCode(code, workspaceId)
VM->>W: spawn subprocess + send {type:generate, code}
W->>W: vm.createContext (null-prototype sandbox)
W-->>VM: {type:getFile, fileId} (IPC)
VM->>VM: getWorkspaceFile + downloadWorkspaceFile
VM-->>W: {type:fileResult, data:base64}
W-->>VM: {type:result, data:base64}
VM-->>Serve: Buffer (compiled .pptx)
Serve-->>AI: binary PPTX response
Reviews (2): Last reviewed commit: "Fix Buffer not assignable to BodyInit in..." | Re-trigger Greptile |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Serve route passes undefined workspaceId breaking file references
- Both handleLocalFile and handleCloudProxy now extract workspaceId from the file key using parseWorkspaceFileKey and pass it to compilePptxIfNeeded.
- ✅ Fixed: Server-side code execution via unsandboxed new Function
- Replaced new Function() with vm.createContext() which creates an isolated context exposing only pptx and getFileBase64, blocking access to process and other Node.js globals.
Or push these changes by commenting:
@cursor push cccb0dcbe3
Preview (cccb0dcbe3)
diff --git a/apps/sim/app/api/files/serve/[...path]/route.ts b/apps/sim/app/api/files/serve/[...path]/route.ts
--- a/apps/sim/app/api/files/serve/[...path]/route.ts
+++ b/apps/sim/app/api/files/serve/[...path]/route.ts
@@ -6,6 +6,7 @@
import { generatePptxFromCode } from '@/lib/copilot/tools/server/files/workspace-file'
import { CopilotFiles, isUsingCloudStorage } from '@/lib/uploads'
import type { StorageContext } from '@/lib/uploads/config'
+import { parseWorkspaceFileKey } from '@/lib/uploads/contexts/workspace/workspace-file-manager'
import { downloadFile } from '@/lib/uploads/core/storage-service'
import { inferContextFromKey } from '@/lib/uploads/utils/file-utils'
import { verifyFileAccess } from '@/app/api/files/authorization'
@@ -138,10 +139,11 @@
const rawBuffer = await readFile(filePath)
const segment = filename.split('/').pop() || filename
const displayName = stripStorageKeyPrefix(segment)
+ const workspaceId = parseWorkspaceFileKey(filename)
const { buffer: fileBuffer, contentType } = await compilePptxIfNeeded(
rawBuffer,
displayName,
- undefined,
+ workspaceId || undefined,
raw
)
@@ -202,10 +204,11 @@
const segment = cloudKey.split('/').pop() || 'download'
const displayName = stripStorageKeyPrefix(segment)
+ const workspaceId = parseWorkspaceFileKey(cloudKey)
const { buffer: fileBuffer, contentType } = await compilePptxIfNeeded(
rawBuffer,
displayName,
- undefined,
+ workspaceId || undefined,
raw
)
diff --git a/apps/sim/lib/copilot/tools/server/files/workspace-file.ts b/apps/sim/lib/copilot/tools/server/files/workspace-file.ts
--- a/apps/sim/lib/copilot/tools/server/files/workspace-file.ts
+++ b/apps/sim/lib/copilot/tools/server/files/workspace-file.ts
@@ -1,3 +1,4 @@
+import vm from 'node:vm'
import { createLogger } from '@sim/logger'
import PptxGenJS from 'pptxgenjs'
import type { BaseServerTool, ServerToolContext } from '@/lib/copilot/tools/server/base-tool'
@@ -36,8 +37,19 @@
return `data:${mime};base64,${buffer.toString('base64')}`
}
- const fn = new Function('pptx', 'getFileBase64', `return (async () => { ${code} })()`)
- await fn(pptx, getFileBase64)
+ const sandbox = {
+ pptx,
+ getFileBase64,
+ __result: null as unknown,
+ }
+
+ vm.createContext(sandbox)
+
+ const wrappedCode = `(async () => { ${code} })()`
+ const script = new vm.Script(`__result = ${wrappedCode}`, { filename: 'pptx-code.js' })
+ script.runInContext(sandbox)
+ await sandbox.__result
+
const output = await pptx.write({ outputType: 'nodebuffer' })
return output as Buffer
}This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.
AI-generated PptxGenJS code was executed via new Function() in both the server (full Node.js access) and browser (XSS risk). Replace with a dedicated Node.js subprocess (pptx-worker.cjs) that runs user code inside vm.createContext with a null-prototype sandbox — no access to process, require, Buffer, or any Node.js globals. Process-level isolation ensures a vm escape cannot reach the main process or DB. File access is brokered via IPC so the subprocess never touches the database directly, mirroring the isolated-vm worker pattern. Compilation happens lazily at serve time (compilePptxIfNeeded) rather than on write, matching industry practice for source-stored PPTX pipelines. - Add pptx-worker.cjs: sandboxed subprocess worker - Add pptx-vm.ts: orchestration, IPC bridge, file brokering - Add /api/workspaces/[id]/pptx/preview: REST-correct preview endpoint - Update serve route: compile pptxgenjs source to binary on demand - Update workspace-file.ts: remove unsafe new Function(), store source only - Update next.config.ts: include pptxgenjs in outputFileTracingIncludes - Update trigger.config.ts: add pptx-worker.cjs and pptxgenjs to build
- Add 'patch' to workspace_file WRITE_ACTIONS — patch operation was missing, letting read-only users modify file content - Add download_to_workspace_file to WRITE_ACTIONS with '*' wildcard — tool was completely ungated, letting read-only users write workspace files - Update isActionAllowed to handle '*' (always-write tools) and undefined action (tools with no operation/action field) - Block private/internal URLs in download_to_workspace_file to prevent SSRF against RFC 1918 ranges, loopback, and cloud metadata endpoints - Fix file-reader.ts image size limit comment and error message (was 20MB, actual constant is 5MB)
Wrap Buffer in Uint8Array for NextResponse body — Buffer is not directly assignable to BodyInit in strict TypeScript mode.
|
@greptile |
|
@cursor review |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| const response = await fetch(params.url, { | ||
| redirect: 'follow', | ||
| signal: context.abortSignal, | ||
| }) |
There was a problem hiding this comment.
SSRF bypass via HTTP redirect to internal URLs
High Severity
The isPrivateUrl check only validates params.url (the initial URL), but fetch is called with redirect: 'follow', allowing the server to redirect to private/internal addresses. An attacker can supply a public URL that 302-redirects to http://169.254.169.254/latest/meta-data/ (cloud metadata endpoint) or any other internal service. The redirect target is never validated via isPrivateUrl, so the response from the internal endpoint gets saved as a workspace file accessible to the user.
|
|
||
| try { | ||
| assertServerToolNotAborted(context) | ||
|
|
||
| if (isPrivateUrl(params.url)) { | ||
| return { |
There was a problem hiding this comment.
isPrivateUrl validates the caller-supplied URL before the fetch, but the fetch uses redirect: 'follow'. A publicly-reachable server can return a 301 to http://169.254.169.254/latest/meta-data/ (AWS/GCP/Azure instance metadata) or any other RFC-1918/cloud-metadata address. The check never runs against the redirected-to URL, so the fetch silently follows the redirect and the response body is written to the workspace as a file.
The simplest fix is to use redirect: 'manual', detect a 3xx status, resolve the Location header with isPrivateUrl, and only then re-fetch (or fail). Alternatively, switch to redirect: 'error' and accept that some legitimate CDN redirects will be blocked — the AI can retry with the final URL.
// Option A: fail on any redirect (strict, simple)
const response = await fetch(params.url, {
redirect: 'error',
signal: context.abortSignal,
})
// Option B: manually validate the redirect target
const response = await fetch(params.url, {
redirect: 'manual',
signal: context.abortSignal,
})
if (response.status >= 300 && response.status < 400) {
const location = response.headers.get('location')
if (!location || isPrivateUrl(location)) {
return { success: false, message: 'Redirect to private or internal URL is not allowed' }
}
// continue with the resolved URL…
}| const ipv4 = hostname.match(/^(\d+)\.(\d+)\.(\d+)\.(\d+)$/) | ||
| if (ipv4) { | ||
| const [a, b] = [Number(ipv4[1]), Number(ipv4[2])] |
There was a problem hiding this comment.
isPrivateUrl only checks hostname === '::1' for IPv6. The following valid private IPv6 addresses are not filtered:
::ffff:10.0.0.1/::ffff:192.168.1.1(IPv4-mapped IPv6)fe80::1(link-local,fe80::/10)fd00::1/fc00::1(unique-local,fc00::/7)
Any of these could be used as an SSRF vector if the host has IPv6 network access.
| const ipv4 = hostname.match(/^(\d+)\.(\d+)\.(\d+)\.(\d+)$/) | |
| if (ipv4) { | |
| const [a, b] = [Number(ipv4[1]), Number(ipv4[2])] | |
| if (hostname === 'localhost' || hostname === '::1') return true | |
| // IPv6 private ranges: loopback, link-local (fe80::/10), unique-local (fc00::/7), | |
| // and IPv4-mapped addresses (::ffff:…) | |
| if (hostname.startsWith('fe80:') || hostname.startsWith('fc') || hostname.startsWith('fd')) return true | |
| if (hostname.startsWith('::ffff:')) { | |
| // Extract the embedded IPv4 and re-check | |
| const embedded = hostname.slice(7) | |
| if (isPrivateUrl(`http://${embedded}/`)) return true | |
| } |
| ) | ||
| const fileName = inferOutputFileName( | ||
| params.fileName, | ||
| response, |
There was a problem hiding this comment.
response.arrayBuffer() buffers the entire response body before checking its size. A malicious (or accidentally huge) URL can cause the server to OOM before the emptiness check on line 185 fires.
Check Content-Length first and/or stream with a byte counter:
const MAX_DOWNLOAD_BYTES = 100 * 1024 * 1024 // 100 MB — tune as needed
const contentLength = response.headers.get('content-length')
if (contentLength && Number(contentLength) > MAX_DOWNLOAD_BYTES) {
return { success: false, message: `File too large: ${contentLength} bytes (limit ${MAX_DOWNLOAD_BYTES})` }
}
const reader = response.body?.getReader()
const chunks: Uint8Array[] = []
let totalBytes = 0
while (reader) {
const { done, value } = await reader.read()
if (done) break
totalBytes += value.byteLength
if (totalBytes > MAX_DOWNLOAD_BYTES) {
reader.cancel()
return { success: false, message: `Download aborted: file exceeds size limit of ${MAX_DOWNLOAD_BYTES} bytes` }
}
chunks.push(value)
}
const fileBuffer = Buffer.concat(chunks.map(Buffer.from))|
|
||
| const error = fetchError | ||
| ? fetchError instanceof Error | ||
| ? fetchError.message |
There was a problem hiding this comment.
workspaceId missing from useEffect dependency array
workspaceId is captured in the closure for the preview-API fetch but is not listed as an effect dependency. React's exhaustive-deps rule would flag this. Although workspaceId is stable for a given mounted PptxPreview, it's a good idea to include it so the linter doesn't suppress warnings globally.
| ? fetchError.message | |
| }, [fileData, dataUpdatedAt, streamingContent, cacheKey, workspaceId]) |
| * Shares the same query key as useWorkspaceFileContent so cache | ||
| * invalidation from file updates triggers a refetch automatically. | ||
| */ | ||
| export function useWorkspaceFileBinary(workspaceId: string, fileId: string, key: string) { | ||
| return useQuery({ | ||
| queryKey: workspaceFilesKeys.content(workspaceId, fileId, 'binary'), | ||
| queryFn: ({ signal }) => fetchWorkspaceFileBinary(key, signal), |
There was a problem hiding this comment.
refetchOnWindowFocus for binary hook
useWorkspaceFileContent sets refetchOnWindowFocus: 'always' so the editor reloads after a file is updated in another tab. useWorkspaceFileBinary omits this option, so the PPTX preview can show a stale rendered deck when the user switches away and back after the AI modifies the file.
Consider adding refetchOnWindowFocus: 'always' to match the behavior of the text content hook.
Summary
Add file patch tool, add pptx capability
Type of Change
Testing
Manual
Checklist