Skip to content

Commit 0799e97

Browse files
committed
refactor(hooks): align HTTP hook security with Claude Code behavior
- Add CRLF/NUL sanitization for env var interpolation (header injection) - Implement combined abort signal (external signal + timeout) - Upgrade SSRF protection to DNS-level with ssrfGuard - Allow loopback (127.0.0.0/8, ::1) for local dev hooks - Block CGNAT (100.64.0.0/10) and IPv6 private ranges - Increase default HTTP hook timeout to 10 minutes - Fix VS Code hooks schema to support http type - Add url, headers, allowedEnvVars, async, once, statusMessage, shell fields - Note: "function" type is SDK-only (callback cannot be serialized to JSON)
1 parent e1a451f commit 0799e97

File tree

10 files changed

+812
-37
lines changed

10 files changed

+812
-37
lines changed

packages/cli/src/config/settingsSchema.ts

Lines changed: 36 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -131,18 +131,31 @@ const HOOK_DEFINITION_ITEMS: SettingItemDefinition = {
131131
items: {
132132
type: 'object',
133133
description:
134-
'A hook configuration entry that defines a command to execute.',
134+
'A hook configuration entry that defines a hook to execute.',
135135
properties: {
136136
type: {
137137
type: 'string',
138-
description: 'The type of hook.',
139-
enum: ['command'],
138+
description: 'The type of hook. Note: "function" type is only available via SDK registration, not settings.json.',
139+
enum: ['command', 'http'],
140140
required: true,
141141
},
142142
command: {
143143
type: 'string',
144-
description: 'The command to execute when the hook is triggered.',
145-
required: true,
144+
description: 'The command to execute when the hook is triggered. Required for "command" type.',
145+
},
146+
url: {
147+
type: 'string',
148+
description: 'The URL to send the POST request to. Required for "http" type.',
149+
},
150+
headers: {
151+
type: 'object',
152+
description: 'HTTP headers to include in the request. Supports env var interpolation ($VAR, ${VAR}).',
153+
additionalProperties: { type: 'string' },
154+
},
155+
allowedEnvVars: {
156+
type: 'array',
157+
description: 'List of environment variables allowed for interpolation in headers and URL.',
158+
items: { type: 'string' },
146159
},
147160
name: {
148161
type: 'string',
@@ -154,14 +167,31 @@ const HOOK_DEFINITION_ITEMS: SettingItemDefinition = {
154167
},
155168
timeout: {
156169
type: 'number',
157-
description: 'Timeout in milliseconds for the hook execution.',
170+
description: 'Timeout in seconds for the hook execution.',
158171
},
159172
env: {
160173
type: 'object',
161174
description:
162175
'Environment variables to set when executing the hook command.',
163176
additionalProperties: { type: 'string' },
164177
},
178+
async: {
179+
type: 'boolean',
180+
description: 'Whether to execute the hook asynchronously (non-blocking, for "command" type only).',
181+
},
182+
once: {
183+
type: 'boolean',
184+
description: 'Whether to execute the hook only once per session (for "http" type).',
185+
},
186+
statusMessage: {
187+
type: 'string',
188+
description: 'A message to display while the hook is executing.',
189+
},
190+
shell: {
191+
type: 'string',
192+
description: 'The shell to use for command execution.',
193+
enum: ['bash', 'powershell'],
194+
},
165195
},
166196
},
167197
},
Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,91 @@
1+
/**
2+
* @license
3+
* Copyright 2026 Qwen Team
4+
* SPDX-License-Identifier: Apache-2.0
5+
*/
6+
7+
import { describe, it, expect } from 'vitest';
8+
import { createCombinedAbortSignal } from './combinedAbortSignal.js';
9+
10+
describe('createCombinedAbortSignal', () => {
11+
it('should return a non-aborted signal by default', () => {
12+
const { signal, cleanup } = createCombinedAbortSignal();
13+
expect(signal.aborted).toBe(false);
14+
cleanup();
15+
});
16+
17+
it('should abort after timeout', async () => {
18+
const { signal, cleanup } = createCombinedAbortSignal(undefined, {
19+
timeoutMs: 50,
20+
});
21+
expect(signal.aborted).toBe(false);
22+
23+
await new Promise((resolve) => setTimeout(resolve, 100));
24+
expect(signal.aborted).toBe(true);
25+
cleanup();
26+
});
27+
28+
it('should abort when external signal is aborted', () => {
29+
const externalController = new AbortController();
30+
const { signal, cleanup } = createCombinedAbortSignal(
31+
externalController.signal,
32+
);
33+
expect(signal.aborted).toBe(false);
34+
35+
externalController.abort();
36+
expect(signal.aborted).toBe(true);
37+
cleanup();
38+
});
39+
40+
it('should abort immediately if external signal is already aborted', () => {
41+
const externalController = new AbortController();
42+
externalController.abort();
43+
44+
const { signal, cleanup } = createCombinedAbortSignal(
45+
externalController.signal,
46+
);
47+
expect(signal.aborted).toBe(true);
48+
cleanup();
49+
});
50+
51+
it('should cleanup timeout timer', async () => {
52+
const { signal, cleanup } = createCombinedAbortSignal(undefined, {
53+
timeoutMs: 50,
54+
});
55+
56+
cleanup();
57+
58+
// Wait longer than timeout - should not abort because timer was cleared
59+
await new Promise((resolve) => setTimeout(resolve, 100));
60+
expect(signal.aborted).toBe(false);
61+
});
62+
63+
it('should work with both external signal and timeout', async () => {
64+
const externalController = new AbortController();
65+
const { signal, cleanup } = createCombinedAbortSignal(
66+
externalController.signal,
67+
{ timeoutMs: 200 },
68+
);
69+
70+
// Abort external signal before timeout
71+
externalController.abort();
72+
expect(signal.aborted).toBe(true);
73+
cleanup();
74+
});
75+
76+
it('should timeout before external signal', async () => {
77+
const externalController = new AbortController();
78+
const { signal, cleanup } = createCombinedAbortSignal(
79+
externalController.signal,
80+
{ timeoutMs: 50 },
81+
);
82+
83+
// Wait for timeout
84+
await new Promise((resolve) => setTimeout(resolve, 100));
85+
expect(signal.aborted).toBe(true);
86+
87+
// External signal is still not aborted
88+
expect(externalController.signal.aborted).toBe(false);
89+
cleanup();
90+
});
91+
});
Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
/**
2+
* @license
3+
* Copyright 2026 Qwen Team
4+
* SPDX-License-Identifier: Apache-2.0
5+
*/
6+
7+
/**
8+
* Create a combined AbortSignal that aborts when either:
9+
* - The provided external signal is aborted, OR
10+
* - The timeout is reached
11+
*
12+
* @param externalSignal - Optional external AbortSignal to combine
13+
* @param timeoutMs - Timeout in milliseconds
14+
* @returns Object containing the combined signal and a cleanup function
15+
*/
16+
export function createCombinedAbortSignal(
17+
externalSignal?: AbortSignal,
18+
options?: { timeoutMs?: number },
19+
): { signal: AbortSignal; cleanup: () => void } {
20+
const controller = new AbortController();
21+
22+
const timeoutMs = options?.timeoutMs;
23+
24+
// Set up timeout
25+
let timeoutId: ReturnType<typeof setTimeout> | undefined;
26+
if (timeoutMs !== undefined && timeoutMs > 0) {
27+
timeoutId = setTimeout(() => {
28+
controller.abort();
29+
}, timeoutMs);
30+
}
31+
32+
// Listen to external signal
33+
if (externalSignal) {
34+
if (externalSignal.aborted) {
35+
controller.abort();
36+
} else {
37+
const abortHandler = () => {
38+
controller.abort();
39+
};
40+
externalSignal.addEventListener('abort', abortHandler, { once: true });
41+
}
42+
}
43+
44+
const cleanup = () => {
45+
if (timeoutId !== undefined) {
46+
clearTimeout(timeoutId);
47+
timeoutId = undefined;
48+
}
49+
};
50+
51+
return { signal: controller.signal, cleanup };
52+
}

packages/core/src/hooks/envInterpolator.test.ts

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import {
1111
interpolateUrl,
1212
hasEnvVarReferences,
1313
extractEnvVarNames,
14+
sanitizeHeaderValue,
1415
} from './envInterpolator.js';
1516

1617
describe('envInterpolator', () => {
@@ -70,6 +71,30 @@ describe('envInterpolator', () => {
7071
const result = interpolateEnvVars('MY_TOKEN', ['MY_TOKEN']);
7172
expect(result).toBe('MY_TOKEN');
7273
});
74+
75+
it('should sanitize CR characters to prevent header injection', () => {
76+
process.env['EVIL_TOKEN'] = 'good\r\nX-Evil: injected';
77+
const result = interpolateEnvVars('$EVIL_TOKEN', ['EVIL_TOKEN']);
78+
expect(result).toBe('goodX-Evil: injected');
79+
});
80+
81+
it('should sanitize LF characters to prevent header injection', () => {
82+
process.env['EVIL_TOKEN'] = 'good\nX-Evil: injected';
83+
const result = interpolateEnvVars('$EVIL_TOKEN', ['EVIL_TOKEN']);
84+
expect(result).toBe('goodX-Evil: injected');
85+
});
86+
87+
it('should sanitize NUL characters', () => {
88+
process.env['EVIL_TOKEN'] = 'good\x00bad';
89+
const result = interpolateEnvVars('$EVIL_TOKEN', ['EVIL_TOKEN']);
90+
expect(result).toBe('goodbad');
91+
});
92+
93+
it('should sanitize CRLF and NUL combined', () => {
94+
process.env['EVIL_TOKEN'] = 'token\r\nX-Injected: 1\x00more';
95+
const result = interpolateEnvVars('Bearer $EVIL_TOKEN', ['EVIL_TOKEN']);
96+
expect(result).toBe('Bearer tokenX-Injected: 1more');
97+
});
7398
});
7499

75100
describe('interpolateHeaders', () => {
@@ -143,4 +168,30 @@ describe('envInterpolator', () => {
143168
expect(extractEnvVarNames('plain text')).toEqual([]);
144169
});
145170
});
171+
172+
describe('sanitizeHeaderValue', () => {
173+
it('should strip CR characters', () => {
174+
expect(sanitizeHeaderValue('token\r\nX-Evil: 1')).toBe('tokenX-Evil: 1');
175+
});
176+
177+
it('should strip LF characters', () => {
178+
expect(sanitizeHeaderValue('token\nX-Evil: 1')).toBe('tokenX-Evil: 1');
179+
});
180+
181+
it('should strip NUL characters', () => {
182+
expect(sanitizeHeaderValue('good\x00bad')).toBe('goodbad');
183+
});
184+
185+
it('should strip all three dangerous characters', () => {
186+
expect(sanitizeHeaderValue('a\r\nb\x00c')).toBe('abc');
187+
});
188+
189+
it('should not affect safe values', () => {
190+
expect(sanitizeHeaderValue('Bearer abc123')).toBe('Bearer abc123');
191+
});
192+
193+
it('should handle empty string', () => {
194+
expect(sanitizeHeaderValue('')).toBe('');
195+
});
196+
});
146197
});

packages/core/src/hooks/envInterpolator.ts

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,19 @@
99
* Provides secure interpolation with whitelist-based access control.
1010
*/
1111

12+
/**
13+
* Strip CR, LF, and NUL bytes from a header value to prevent HTTP header
14+
* injection (CRLF injection) via env var values or hook-configured header
15+
* templates. A malicious env var like "token\r\nX-Evil: 1" would otherwise
16+
* inject a second header into the request.
17+
*
18+
* Aligned with Claude Code's sanitizeHeaderValue behavior.
19+
*/
20+
export function sanitizeHeaderValue(value: string): string {
21+
// eslint-disable-next-line no-control-regex
22+
return value.replace(/[\r\n\x00]/g, '');
23+
}
24+
1225
/**
1326
* Interpolate environment variables in a string value.
1427
* Only variables in the allowedVars list will be replaced.
@@ -18,7 +31,7 @@
1831
*
1932
* @param value - The string containing environment variable references
2033
* @param allowedVars - List of allowed environment variable names
21-
* @returns The interpolated string
34+
* @returns The interpolated string (sanitized to prevent header injection)
2235
*/
2336
/**
2437
* Dangerous variable names that could be used for prototype pollution attacks
@@ -45,7 +58,7 @@ export function interpolateEnvVars(
4558
allowedVars: string[],
4659
): string {
4760
// Match $VAR_NAME or ${VAR_NAME}
48-
return value.replace(
61+
const interpolated = value.replace(
4962
/\$\{?([A-Za-z_][A-Za-z0-9_]*)\}?/g,
5063
(match, varName: string) => {
5164
// Block dangerous variable names to prevent prototype pollution
@@ -59,6 +72,8 @@ export function interpolateEnvVars(
5972
return '';
6073
},
6174
);
75+
// Sanitize to prevent CRLF/NUL header injection
76+
return sanitizeHeaderValue(interpolated);
6277
}
6378

6479
/**

packages/core/src/hooks/httpHookRunner.test.ts

Lines changed: 26 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -101,10 +101,10 @@ describe('HttpHookRunner', () => {
101101
expect(mockFetch).not.toHaveBeenCalled();
102102
});
103103

104-
it('should fail for blocked URL (SSRF)', async () => {
104+
it('should fail for blocked URL (SSRF - link-local metadata)', async () => {
105105
const runner = new HttpHookRunner([]); // Allow all patterns
106106
const config = createMockConfig({
107-
url: 'http://localhost:8080/hook',
107+
url: 'http://169.254.169.254/latest/meta-data',
108108
});
109109
const input = createMockInput();
110110

@@ -115,10 +115,33 @@ describe('HttpHookRunner', () => {
115115
);
116116

117117
expect(result.success).toBe(false);
118-
expect(result.error?.message).toContain('SSRF');
118+
expect(result.error?.message).toContain('blocked');
119119
expect(mockFetch).not.toHaveBeenCalled();
120120
});
121121

122+
it('should ALLOW localhost for local dev hooks', async () => {
123+
mockFetch.mockResolvedValueOnce({
124+
ok: true,
125+
headers: new Headers({ 'content-type': 'application/json' }),
126+
json: async () => ({ continue: true }),
127+
});
128+
129+
const runner = new HttpHookRunner([]); // Allow all patterns
130+
const config = createMockConfig({
131+
url: 'http://localhost:8080/hook',
132+
});
133+
const input = createMockInput();
134+
135+
const result = await runner.execute(
136+
config,
137+
HookEventName.PreToolUse,
138+
input,
139+
);
140+
141+
expect(result.success).toBe(true);
142+
expect(mockFetch).toHaveBeenCalled();
143+
});
144+
122145
it('should interpolate environment variables in headers', async () => {
123146
process.env['MY_TOKEN'] = 'secret-token';
124147

0 commit comments

Comments
 (0)