Skip to content

Commit 27f1063

Browse files
committed
fix(copilot): move file output write-permission gate after no-op skip branches
Address Cursor review: in maybeWriteOutputToFile the gate ran before the sandbox-export skip branch (which returns the result unchanged without writing), so a read-only caller with a sandbox files payload was denied even though no workspace write would occur. Move the check to immediately before writeWorkspaceFileByPath so it only fires when a write is actually performed.
1 parent 12d3b05 commit 27f1063

2 files changed

Lines changed: 15 additions & 3 deletions

File tree

apps/sim/lib/copilot/request/tools/files.test.ts

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -141,6 +141,18 @@ describe('maybeWriteOutputToFile', () => {
141141
expect(mockWriteWorkspaceFileByPath).not.toHaveBeenCalled()
142142
})
143143

144+
it('does not deny a read-only principal when no workspace write occurs (sandbox export active)', async () => {
145+
const result = await maybeWriteOutputToFile(
146+
FunctionExecute.id,
147+
{ outputs: { files: [{ path: 'files/report.csv', mode: 'overwrite' }] } },
148+
{ success: true, output: { result: { files: [{ path: 'report.csv' }] }, stdout: '' } },
149+
buildContext({ userPermission: 'read' })
150+
)
151+
152+
expect(result.success).toBe(true)
153+
expect(mockWriteWorkspaceFileByPath).not.toHaveBeenCalled()
154+
})
155+
144156
it('writes the output file for a write principal', async () => {
145157
const result = await maybeWriteOutputToFile(
146158
FunctionExecute.id,

apps/sim/lib/copilot/request/tools/files.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -211,9 +211,6 @@ export async function maybeWriteOutputToFile(
211211
const outputFiles = getOutputFileDeclarations(params).filter((file) => !file.sandboxPath)
212212
if (outputFiles.length === 0) return result
213213

214-
const denied = denyOutputWriteWithoutWritePermission(context)
215-
if (denied) return denied
216-
217214
const outputObject =
218215
result.output && typeof result.output === 'object' && !Array.isArray(result.output)
219216
? (result.output as Record<string, unknown>)
@@ -232,6 +229,9 @@ export async function maybeWriteOutputToFile(
232229
return result
233230
}
234231

232+
const denied = denyOutputWriteWithoutWritePermission(context)
233+
if (denied) return denied
234+
235235
// Only span the actual write path (where we upload to storage). Fast
236236
// no-op returns above don't need a span — they'd just pad the trace
237237
// with empty work.

0 commit comments

Comments
 (0)