Skip to content

Commit b4f471c

Browse files
committed
fix(sandbox): preserve boundary compatibility
1 parent 58d4e9a commit b4f471c

8 files changed

Lines changed: 171 additions & 83 deletions

File tree

‎apps/sim/lib/execution/remote-sandbox/session-files.test.ts‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,8 @@ vi.mock('@/lib/execution/remote-sandbox/session-lock', () => ({
3535
}))
3636

3737
import { observeSandboxExecution } from '@/lib/execution/remote-sandbox/execution-observer'
38+
import { SandboxOutputLimitError } from '@/lib/execution/remote-sandbox/output-limits'
39+
import { readSessionSecretProvenance } from '@/lib/execution/remote-sandbox/session-file-provenance'
3840
import {
3941
readSessionSandboxFile,
4042
writeSessionSandboxFile,
@@ -63,6 +65,24 @@ describe('workbench file cancellation', () => {
6365
run.mockResolvedValue({ stdout: '', stderr: '', exitCode: 0 })
6466
})
6567

68+
it('distinguishes a file-size failure without returning provider diagnostics', async () => {
69+
read.mockRejectedValueOnce(new SandboxOutputLimitError(4 * 1024 * 1024 + 1, 4 * 1024 * 1024))
70+
expect(await readSessionSandboxFile('chat', 'large.txt')).toEqual({
71+
outcome: 'error',
72+
detail: 'Workbench file exceeds the maximum read size of 4194304 bytes',
73+
})
74+
})
75+
76+
it('distinguishes a provenance outage from a missing file without returning storage diagnostics', async () => {
77+
vi.mocked(readSessionSecretProvenance).mockRejectedValueOnce(
78+
new Error('SYNTHETIC_PRIVATE_DIAGNOSTIC')
79+
)
80+
expect(await readSessionSandboxFile('chat', 'input.txt')).toEqual({
81+
outcome: 'error',
82+
detail: 'Workbench file secret provenance is unavailable',
83+
})
84+
})
85+
6686
it('does not write if Stop arrives during the sandbox lookup', async () => {
6787
const controller = new AbortController()
6888
find.mockImplementation(async () => {

‎apps/sim/lib/execution/remote-sandbox/session-files.ts‎

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,11 @@
11
import { posix } from 'node:path'
22
import { createLogger } from '@sim/logger'
33
import { getErrorMessage } from '@sim/utils/errors'
4-
import { PayloadSizeLimitError } from '@/lib/core/utils/stream-limits'
4+
import { isPayloadSizeLimitError, PayloadSizeLimitError } from '@/lib/core/utils/stream-limits'
55
import type { DurableSecretProvenance } from '@/lib/execution/durable-secret-provenance'
66
import { prepareSandboxSessionAccess } from '@/lib/execution/remote-sandbox/execution-observer'
77
import { withSandboxFilePublication } from '@/lib/execution/remote-sandbox/file-publication'
8+
import { isSandboxOutputLimitError } from '@/lib/execution/remote-sandbox/output-limits'
89
import { resolveProvider } from '@/lib/execution/remote-sandbox/provider'
910
import {
1011
ensureSessionSandbox,
@@ -66,19 +67,32 @@ export async function readSessionSandboxFile(
6667
if (!sandbox) return { outcome: 'no-session' }
6768
await sandbox.extendLifetime?.(SESSION_SANDBOX_IDLE_MS)
6869
signal.throwIfAborted()
70+
let file: { content: string }
6971
try {
70-
const file = await sandbox.readFileWithLimit(resolved, {
72+
file = await sandbox.readFileWithLimit(resolved, {
7173
maxBytes: READ_LIMIT_BYTES,
7274
encoding,
7375
signal,
7476
})
77+
} catch (error) {
78+
signal.throwIfAborted()
79+
if (isSandboxOutputLimitError(error) || isPayloadSizeLimitError(error)) {
80+
return {
81+
outcome: 'error',
82+
detail: `Workbench file exceeds the maximum read size of ${READ_LIMIT_BYTES} bytes`,
83+
}
84+
}
85+
return { outcome: 'no-file', detail: 'Workbench file is missing or unreadable' }
86+
}
87+
try {
7588
const secretProvenance = await readSessionSecretProvenance(sessionKey, {
7689
providerId: provider.id,
7790
sandboxId: sandbox.sandboxId,
7891
})
7992
return { outcome: 'read', content: file.content, secretProvenance }
80-
} catch (error) {
81-
return { outcome: 'no-file', detail: 'Workbench file could not be read safely' }
93+
} catch {
94+
signal.throwIfAborted()
95+
return { outcome: 'error', detail: 'Workbench file secret provenance is unavailable' }
8296
}
8397
})
8498
} catch (error) {

‎apps/sim/lib/mothership/agent-cli/run-cli-files.test.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,7 @@ describe('the CLI owns workbench file semantics', () => {
124124
return {
125125
outcome: 'read',
126126
content: Buffer.from(JSON.stringify({ session })).toString('base64'),
127+
secretProvenance: { status: 'exact', entries: [] },
127128
}
128129
})
129130
const transport = async (input: string | URL | Request, init?: RequestInit) => {
@@ -153,6 +154,7 @@ describe('the CLI owns workbench file semantics', () => {
153154
read.mockResolvedValue({
154155
outcome: 'read',
155156
content: Buffer.from('wf-one\nwf-two\n').toString('base64'),
157+
secretProvenance: { status: 'exact', entries: [] },
156158
})
157159
const transport = async (input: string | URL | Request) => {
158160
requests.push(new URL(input instanceof Request ? input.url : input))

‎apps/sim/lib/mothership/agent-cli/run-cli.test.ts‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,11 @@ describe('embedded CLI binary workbench bridge', () => {
2323
it('preserves arbitrary bytes in both directions and binds both to the same chat', async () => {
2424
const bytes = Uint8Array.from([0, 255, 137, 80, 78, 71, 13, 10, 128, 195, 0])
2525
const stream = new Blob([bytes]).stream()
26-
readFile.mockResolvedValue({ outcome: 'read', content: Buffer.from(bytes).toString('base64') })
26+
readFile.mockResolvedValue({
27+
outcome: 'read',
28+
content: Buffer.from(bytes).toString('base64'),
29+
secretProvenance: { status: 'exact', entries: [] },
30+
})
2731
writeFile.mockResolvedValue({ outcome: 'written', path: '/home/user/result.png' })
2832
embedded.mockImplementation(async (_args, _identity, options) => {
2933
expect(await options.readFile('image.png')).toEqual(Buffer.from(bytes))
@@ -51,7 +55,11 @@ describe('embedded CLI binary workbench bridge', () => {
5155
})
5256

5357
it('resolves equals-form file flags identically without reading escaped literals', async () => {
54-
readFile.mockResolvedValue({ outcome: 'read', content: Buffer.from('{}').toString('base64') })
58+
readFile.mockResolvedValue({
59+
outcome: 'read',
60+
content: Buffer.from('{}').toString('base64'),
61+
secretProvenance: { status: 'exact', entries: [] },
62+
})
5563
embedded.mockImplementation(async (_args, _identity, options) => {
5664
expect(await options.readFile('input.json')).toEqual(Buffer.from('{}'))
5765
return { exitCode: 0, stdout: '', stderr: '' }

‎apps/sim/lib/mothership/tools/handlers/function-execute.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -597,7 +597,7 @@ export async function executeFunctionExecute(
597597

598598
/** Receipt records every value placed in the runtime, including silent resolutions. */
599599
for (const [name, plaintext] of Object.entries(mounted.envVars)) {
600-
if (!mountedRegistry.recordResolved(name, plaintext)) {
600+
if (plaintext.length > 0 && !mountedRegistry.recordResolved(name, plaintext)) {
601601
throw new CopilotCodeSecretAccessError('Mounted secret provenance is unavailable')
602602
}
603603
}

‎apps/sim/lib/mothership/tools/handlers/workbench-confidentiality.live.test.ts‎

Lines changed: 54 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -6,55 +6,30 @@ import { createDelegatedPrincipal } from '@sim/testing/factories/principal.facto
66
import { createDeferred } from '@sim/testing/helpers/deferred'
77
import { setEnv } from '@sim/testing/mocks/env.mock'
88
import { envFlagsMock } from '@sim/testing/mocks/env-flags.mock'
9+
import { redisConfigMockFns } from '@sim/testing/mocks/redis-config.mock'
10+
import {
11+
remoteSandboxProviderMock,
12+
remoteSandboxProviderMockFns,
13+
} from '@sim/testing/mocks/remote-sandbox-provider.mock'
14+
import { toolsMock, toolsMockFns } from '@sim/testing/mocks/tools.mock'
915
import { generateShortId } from '@sim/utils/id'
1016
import Redis from 'ioredis'
1117
import { afterAll, beforeEach, describe, expect, it, vi } from 'vitest'
1218

13-
const io = vi.hoisted(() => ({ execute: vi.fn(), mount: vi.fn(), find: vi.fn(), write: vi.fn() }))
14-
vi.mock('@/tools', () => ({ executeTool: io.execute }))
19+
const io = vi.hoisted(() => ({ mount: vi.fn(), find: vi.fn(), write: vi.fn() }))
20+
vi.mock('@/tools', () => toolsMock)
1521
vi.mock('@/lib/mothership/tools/secret-mount-materializer.server', () => ({
1622
materializeCopilotCodeSecrets: io.mount,
1723
CopilotCodeSecretAccessError: class extends Error {},
1824
}))
1925
vi.mock('@/lib/secrets/usage/record', () => ({ recordSecretUsage: vi.fn() }))
20-
vi.mock('@/lib/execution/remote-sandbox/provider', () => ({
21-
resolveProvider: () => ({
22-
id: 'e2b',
23-
dependencyStrategy: 'prebuilt',
24-
resolveLifetimeMs: (ms: number) => ms,
25-
findSessionSandbox: io.find,
26-
create: async () => {
27-
throw new Error('Only the existing disposable worker may be used')
28-
},
29-
}),
30-
}))
26+
vi.mock('@/lib/execution/remote-sandbox/provider', () => remoteSandboxProviderMock)
3127
vi.mock('@/lib/execution/remote-sandbox/resolve', () => ({
3228
resolveWorkspaceSandbox: async () => null,
3329
provisionRuntimeDependencies: async () => {},
3430
repairMissingSandboxImage: async () => null,
3531
RUNTIME_INSTALL_TIMEOUT_MS: 60_000,
3632
}))
37-
vi.mock('@/lib/core/config/redis', () => ({
38-
getRedisClient: () => redis,
39-
getConfiguredRedisUrl: () => undefined,
40-
acquireLock: async (key: string, owner: string, ttl: number) =>
41-
(await redis.set(key, owner, 'EX', ttl, 'NX')) === 'OK',
42-
extendLock: async (key: string, owner: string, ttl: number) =>
43-
(await redis.eval(
44-
"if redis.call('GET',KEYS[1]) == ARGV[1] then return redis.call('EXPIRE',KEYS[1],ARGV[2]) else return 0 end",
45-
1,
46-
key,
47-
owner,
48-
ttl
49-
)) === 1,
50-
releaseLock: async (key: string, owner: string) =>
51-
redis.eval(
52-
"if redis.call('GET',KEYS[1]) == ARGV[1] then return redis.call('DEL',KEYS[1]) else return 0 end",
53-
1,
54-
key,
55-
owner
56-
),
57-
}))
5833
vi.mock('@/lib/mothership/tools/sandbox-session', () => ({
5934
buildMothershipSandboxSession: async (args: { sessionKey: string }) => ({ key: args.sessionKey }),
6035
}))
@@ -168,6 +143,38 @@ function localWorker(): SandboxHandle {
168143
}
169144

170145
beforeEach(async () => {
146+
redisConfigMockFns.mockGetRedisClient.mockReturnValue(redis)
147+
redisConfigMockFns.mockAcquireLock.mockImplementation(
148+
async (key: string, owner: string, ttl: number) =>
149+
(await redis.set(key, owner, 'EX', ttl, 'NX')) === 'OK'
150+
)
151+
redisConfigMockFns.mockExtendLock.mockImplementation(
152+
async (key: string, owner: string, ttl: number) =>
153+
(await redis.eval(
154+
"if redis.call('GET',KEYS[1]) == ARGV[1] then return redis.call('EXPIRE',KEYS[1],ARGV[2]) else return 0 end",
155+
1,
156+
key,
157+
owner,
158+
ttl
159+
)) === 1
160+
)
161+
redisConfigMockFns.mockReleaseLock.mockImplementation(async (key: string, owner: string) => {
162+
await redis.eval(
163+
"if redis.call('GET',KEYS[1]) == ARGV[1] then return redis.call('DEL',KEYS[1]) else return 0 end",
164+
1,
165+
key,
166+
owner
167+
)
168+
})
169+
remoteSandboxProviderMockFns.mockResolveProvider.mockReturnValue({
170+
id: 'e2b',
171+
dependencyStrategy: 'prebuilt',
172+
resolveLifetimeMs: (ms: number) => ms,
173+
findSessionSandbox: io.find,
174+
create: async () => {
175+
throw new Error('Only the existing disposable worker may be used')
176+
},
177+
})
171178
setEnv({ ENCRYPTION_KEY: 'a'.repeat(64) })
172179
envFlagsMock.isMothershipSandboxEnabled = true
173180
envFlagsMock.isRemoteSandboxEnabled = true
@@ -189,7 +196,7 @@ beforeEach(async () => {
189196
file: { id: 'review-file', name: 'review.txt', size: canary.length, type: 'text/plain' },
190197
vfsPath: 'files/review.txt',
191198
}))
192-
io.execute.mockImplementation(
199+
toolsMockFns.mockExecuteTool.mockImplementation(
193200
async (
194201
_id,
195202
params: CodeExecutionInput,
@@ -256,6 +263,18 @@ async function run(code: string, secrets: string[] = []) {
256263
}
257264

258265
describe('persistent workbench output confidentiality', () => {
266+
it('allows a mounted empty value without requiring a redaction receipt', async () => {
267+
const emptyCatalog = [
268+
{ name: 'TOKEN', plaintext: '', encryptedValue: (await encryptSecret('')).encrypted },
269+
]
270+
io.mount.mockResolvedValue({ envVars: { TOKEN: '' }, catalogEntries: emptyCatalog })
271+
const result = await run('printenv TOKEN >/dev/null && test -z "$TOKEN" && printf allowed', [
272+
'TOKEN',
273+
])
274+
expect(result.raw.success).toBe(true)
275+
expect(result.projected.safe).toBe(true)
276+
expect(JSON.stringify(result.projected.result)).toContain('allowed')
277+
})
259278
it('control: same-call secret output is redacted', async () => {
260279
const result = await run('printf "%s" "$TOKEN"', ['TOKEN'])
261280
expect(result.raw.success).toBe(true)

‎apps/sim/lib/webhooks/provider-subscriptions.test.ts‎

Lines changed: 44 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -33,23 +33,29 @@ describe('createExternalWebhookSubscription', () => {
3333
mockGetEffectiveDecryptedEnv.mockResolvedValue({ ASHBY_API_KEY: 'real-secret-key' })
3434
})
3535

36-
it('projects resolved values out of provider failures while preserving status', async () => {
37-
const failure = Object.assign(new Error('Provider refused real-secret-key'), { status: 429 })
38-
mockGetProviderHandler.mockReturnValue({
39-
createSubscription: async () => {
40-
throw failure
41-
},
42-
})
43-
await expect(
44-
createExternalWebhookSubscription(
45-
{} as NextRequest,
46-
{ provider: 'ashby', providerConfig: { apiKey: '{{ASHBY_API_KEY}}' } },
47-
{ workspaceId: 'ws-1' },
48-
'user-1',
49-
'req-1'
50-
)
51-
).rejects.toMatchObject({ message: 'Provider refused {{ASHBY_API_KEY}}', status: 429 })
52-
})
36+
it.each([
37+
['{{ASHBY_API_KEY}}', '{{ASHBY_API_KEY}}'],
38+
['real-secret-key', '[REDACTED_SECRET]'],
39+
])(
40+
'projects configured credential %s out of provider failures while preserving status',
41+
async (apiKey, replacement) => {
42+
const failure = Object.assign(new Error('Provider refused real-secret-key'), { status: 429 })
43+
mockGetProviderHandler.mockReturnValue({
44+
createSubscription: async () => {
45+
throw failure
46+
},
47+
})
48+
await expect(
49+
createExternalWebhookSubscription(
50+
{} as NextRequest,
51+
{ provider: 'ashby', providerConfig: { apiKey } },
52+
{ workspaceId: 'ws-1' },
53+
'user-1',
54+
'req-1'
55+
)
56+
).rejects.toMatchObject({ message: `Provider refused ${replacement}`, status: 429 })
57+
}
58+
)
5359

5460
it('resolves {{ENV_VAR}} references in providerConfig before calling the provider', async () => {
5561
const createSubscription = vi.fn().mockResolvedValue({
@@ -118,21 +124,27 @@ describe('cleanupExternalWebhook', () => {
118124
* non-admin owner without a credential grant leave `{{VAR}}` unresolved, and
119125
* the provider was handed the literal reference as its credential.
120126
*/
121-
it('keeps resolved cleanup values out of retryable deployment failures', async () => {
122-
mockGetProviderHandler.mockReturnValue({
123-
deleteSubscription: async () => {
124-
throw new Error('Provider refused real-secret-key')
125-
},
126-
})
127-
await expect(
128-
cleanupExternalWebhook(
129-
{ provider: 'calendly', providerConfig: { apiKey: '{{CALENDLY_API_KEY}}' } },
130-
{ userId: 'user-1', workspaceId: 'workspace-1' },
131-
'req-1',
132-
{ throwOnError: true }
133-
)
134-
).rejects.toThrow('Provider refused {{CALENDLY_API_KEY}}')
135-
})
127+
it.each([
128+
['{{CALENDLY_API_KEY}}', '{{CALENDLY_API_KEY}}'],
129+
['real-secret-key', '[REDACTED_SECRET]'],
130+
])(
131+
'keeps configured cleanup credential %s out of retryable deployment failures',
132+
async (apiKey, replacement) => {
133+
mockGetProviderHandler.mockReturnValue({
134+
deleteSubscription: async () => {
135+
throw new Error('Provider refused real-secret-key')
136+
},
137+
})
138+
await expect(
139+
cleanupExternalWebhook(
140+
{ provider: 'calendly', providerConfig: { apiKey } },
141+
{ userId: 'user-1', workspaceId: 'workspace-1' },
142+
'req-1',
143+
{ throwOnError: true }
144+
)
145+
).rejects.toThrow(`Provider refused ${replacement}`)
146+
}
147+
)
136148

137149
it('resolves {{ENV_VAR}} references before deleting the provider subscription', async () => {
138150
const deleteSubscription = vi.fn().mockResolvedValue(undefined)

0 commit comments

Comments
 (0)