Skip to content

Commit b90c0a6

Browse files
kamalclaude
authored andcommitted
Harden env deep-merge and prevent mutation leaks
- Skip deep-merge when env is an array (falls through to Zod validation) - Return detached copy of env from getAll() to prevent accidental mutation - Add tests for both cases Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent bb73bdb commit b90c0a6

4 files changed

Lines changed: 51 additions & 4 deletions

File tree

src/utils/__tests__/session-aware-tool-factory.test.ts

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -290,4 +290,33 @@ describe('createSessionAwareTool', () => {
290290
const parsed = JSON.parse(envContent.text);
291291
expect(parsed).toEqual({ API_KEY: 'abc123', DEBUG: 'true', VERBOSE: '0' });
292292
});
293+
294+
it('rejects array passed as env instead of deep-merging it', async () => {
295+
const envSchema = z.object({
296+
scheme: z.string(),
297+
projectPath: z.string().optional(),
298+
env: z.record(z.string(), z.string()).optional(),
299+
});
300+
301+
const envHandler = createSessionAwareTool<z.infer<typeof envSchema>>({
302+
internalSchema: envSchema,
303+
logicFunction: async (params) => ({
304+
content: [{ type: 'text', text: JSON.stringify(params.env) }],
305+
isError: false,
306+
}),
307+
getExecutor: () => createMockExecutor({ success: true }),
308+
requirements: [{ allOf: ['scheme'] }],
309+
});
310+
311+
sessionStore.setDefaults({
312+
scheme: 'App',
313+
projectPath: '/a.xcodeproj',
314+
env: { API_KEY: 'abc123' },
315+
});
316+
317+
// Array should not be deep-merged; Zod validation should reject it
318+
const result = await envHandler({ env: ['not', 'a', 'record'] as unknown });
319+
expect(result.isError).toBe(true);
320+
expect(result.content[0].text).toContain('Parameter validation failed');
321+
});
293322
});

src/utils/__tests__/session-store.test.ts

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,4 +94,15 @@ describe('SessionStore', () => {
9494
expect(sessionStore.getActiveProfile()).toBeNull();
9595
expect(sessionStore.listProfiles()).toEqual([]);
9696
});
97+
98+
it('getAll returns a detached copy of env so mutations do not affect stored defaults', () => {
99+
sessionStore.setDefaults({ env: { API_KEY: 'secret' } });
100+
101+
const copy = sessionStore.getAll();
102+
copy.env!.API_KEY = 'tampered';
103+
copy.env!.EXTRA = 'injected';
104+
105+
const stored = sessionStore.getAll();
106+
expect(stored.env).toEqual({ API_KEY: 'secret' });
107+
});
97108
});

src/utils/session-store.ts

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -114,10 +114,12 @@ class SessionStore {
114114
}
115115

116116
getAllForProfile(profile: string | null): SessionDefaults {
117-
if (profile === null) {
118-
return { ...this.globalDefaults };
117+
const defaults = profile === null ? this.globalDefaults : (this.profiles[profile] ?? {});
118+
const copy = { ...defaults };
119+
if (copy.env) {
120+
copy.env = { ...copy.env };
119121
}
120-
return { ...(this.profiles[profile] ?? {}) };
122+
return copy;
121123
}
122124

123125
listProfiles(): string[] {

src/utils/typed-tool-factory.ts

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -168,7 +168,12 @@ function createSessionAwareHandler<TParams, TContext>(opts: {
168168

169169
// Deep-merge env: combine session-default env vars with user-provided ones
170170
// (user-provided keys take precedence on conflict)
171-
if (sessionDefaults.env && typeof sanitizedArgs.env === 'object' && sanitizedArgs.env) {
171+
if (
172+
sessionDefaults.env &&
173+
typeof sanitizedArgs.env === 'object' &&
174+
sanitizedArgs.env &&
175+
!Array.isArray(sanitizedArgs.env)
176+
) {
172177
merged.env = { ...sessionDefaults.env, ...(sanitizedArgs.env as Record<string, string>) };
173178
}
174179

0 commit comments

Comments
 (0)