mirror of
https://github.com/simstudioai/sim.git
synced 2026-09-24 15:45:35 +08:00
fix(executor): copy the env map at the workflow-tool boundary (#6618)
Follow-up hardening to #6611, which began forwarding the invoking run's environment variables into a workflow run as an agent tool. A runtime audit of that change confirmed nothing today writes through `ctx.environmentVariables`, so this is not a live defect. But `tools/index.ts` was the only consumer handing the map across an execution boundary by reference, and it hands it to the longest-lived consumer there is: the child holds it for its entire run. `agent-handler`, `function-handler`, `condition-handler` and `providers/utils` all copy via `normalizeStringRecord` before handing the map anywhere. A future write through the child's reference would corrupt the parent's env and every later sibling tool call in the same agent turn — a cross-run bug with no local symptom. A shallow spread is exact here: the value is typed `Record<string, string>`, and the sub-Executor already re-copies it through `normalizeStringRecord` (`executor.ts:73`), so the child receives a byte-identical map either way. The spread also subsumes the previous `?? {}`, since spreading `undefined` yields `{}`. The test mutates the forwarded map and asserts the parent context is unchanged; it fails without the spread.
This commit is contained in:
@@ -1550,6 +1550,29 @@ describe('executeTool Function', () => {
|
||||
)
|
||||
})
|
||||
|
||||
it('copies the env map so a child run cannot corrupt the parent context', async () => {
|
||||
mockRunWorkflowTool.mockResolvedValueOnce({ success: true, output: { ok: true } })
|
||||
const executionContext = createToolExecutionContext({
|
||||
environmentVariables: { MY_API_KEY: 'parent-secret' },
|
||||
})
|
||||
|
||||
await executeTool(
|
||||
'workflow_executor_child-workflow',
|
||||
{ workflowId: 'child-workflow' },
|
||||
{ executionContext }
|
||||
)
|
||||
|
||||
const forwarded = (mockRunWorkflowTool.mock.calls[0]?.[1] as Record<string, unknown>)
|
||||
.environmentVariables as Record<string, string>
|
||||
expect(forwarded).toEqual({ MY_API_KEY: 'parent-secret' })
|
||||
expect(forwarded).not.toBe(executionContext.environmentVariables)
|
||||
|
||||
forwarded.MY_API_KEY = 'mutated-by-child'
|
||||
forwarded.INJECTED = 'added-by-child'
|
||||
|
||||
expect(executionContext.environmentVariables).toEqual({ MY_API_KEY: 'parent-secret' })
|
||||
})
|
||||
|
||||
it('leaves the custom-block runner without the consumer redaction policy', async () => {
|
||||
mockRunCustomBlockTool.mockResolvedValueOnce({ success: true, output: { ok: true } })
|
||||
|
||||
|
||||
@@ -1792,7 +1792,11 @@ async function executeToolImplementation(
|
||||
// Trusted `executionContext`, never `_context` — that bag spreads
|
||||
// model-reachable `contextParams._context` first, so a model could otherwise
|
||||
// inject its own env map or disable redaction.
|
||||
environmentVariables: executionContext?.environmentVariables ?? {},
|
||||
// Copied, not aliased: the child holds this map for its whole run, and a
|
||||
// write through it would corrupt the parent's env and every later sibling
|
||||
// tool call. Every other consumer of `ctx.environmentVariables` already
|
||||
// copies (`normalizeStringRecord`); this boundary is the longest-lived one.
|
||||
environmentVariables: { ...executionContext?.environmentVariables },
|
||||
piiBlockOutputRedaction: executionContext?.piiBlockOutputRedaction,
|
||||
}
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user