diff --git a/docs/users/features/hooks.md b/docs/users/features/hooks.md index 96884859155..f514a2545de 100644 --- a/docs/users/features/hooks.md +++ b/docs/users/features/hooks.md @@ -117,7 +117,7 @@ By default, HTTP hooks cannot target private or link-local IP ranges. In platfor - This setting is **only honored from User, System, and SystemDefaults settings scopes**. A value set in Workspace (project) settings is ignored and logged as a warning, so a cloned repository can never self-grant this bypass. - The flag relaxes only the general private/CGNAT/link-local **range** checks. Cloud metadata endpoints stay blocked in every configuration: the `BLOCKED_HOSTS` list is matched literally (`metadata.google.internal`, `metadata.azure.internal`, ...), and the metadata IPs `169.254.169.254` and `100.100.100.200` are blocked in all serialized forms (including IPv4-mapped IPv6 such as `::ffff:a9fe:a9fe`) and after DNS resolution. -- The `security.allowedHttpHookUrls` whitelist still applies independently. In managed environments, pair this flag with a whitelist so only the intended internal endpoints are reachable. +- The `security.allowedHttpHookUrls` whitelist still applies independently. In managed environments, pair this flag with a whitelist so only the intended internal endpoints are reachable. Like this flag, the whitelist is **honored from User, System, and SystemDefaults settings scopes**; a value set in Workspace (project) settings can only _narrow_ the User or SystemDefaults whitelist and is logged as a warning: workspace entries that no higher-scope entry covers are dropped, when no higher scope sets a whitelist the workspace value is ignored entirely, and a System-scope whitelist always takes precedence over the workspace value (an empty whitelist means "allow all", so a repository can neither widen where hook payloads may be sent nor establish a whitelist of its own). > **Warning:** Enabling this flag lets hooks reach internal infrastructure on your network. Enable it only in trusted, managed settings — never in a repository you do not control. diff --git a/integration-tests/terminal-capture/skill-review-harness/text-capture.tsx b/integration-tests/terminal-capture/skill-review-harness/text-capture.tsx index bb4aa5a63ce..a0ff5852271 100644 --- a/integration-tests/terminal-capture/skill-review-harness/text-capture.tsx +++ b/integration-tests/terminal-capture/skill-review-harness/text-capture.tsx @@ -230,11 +230,28 @@ async function main() { const coreSrcUrl = pathToFileURL( path.join(repoRoot, 'packages', 'core', 'index.ts'), ).href; + // Core's package.json maps the ./envVarResolver subpath to dist, which a + // fresh checkout lacks (and a built one may keep stale). `after` mode + // pulls it in via SkillReviewDialog -> config/settings.js, so it needs + // the same source remap as the bare specifier (mirrors scripts/dev.js). + const envVarResolverSrcUrl = pathToFileURL( + path.join( + repoRoot, + 'packages', + 'core', + 'src', + 'utils', + 'envVarResolver.ts', + ), + ).href; const loader = ` export function resolve(specifier, context, nextResolve) { if (specifier === '@qwen-code/qwen-code-core') { return { shortCircuit: true, url: '${coreSrcUrl}', format: 'module' }; } + if (specifier === '@qwen-code/qwen-code-core/envVarResolver') { + return { shortCircuit: true, url: '${envVarResolverSrcUrl}', format: 'module' }; + } return nextResolve(specifier, context); } `; diff --git a/packages/cli/src/commands/channel/config-utils.test.ts b/packages/cli/src/commands/channel/config-utils.test.ts index 68d1a29064e..3f00bba5954 100644 --- a/packages/cli/src/commands/channel/config-utils.test.ts +++ b/packages/cli/src/commands/channel/config-utils.test.ts @@ -805,3 +805,84 @@ describe('parseChannelConfig', () => { ); }); }); + +describe('internal-secret denylist', () => { + const SECRET = 'QWEN_SERVER_TOKEN'; + + afterEach(() => { + delete process.env[SECRET]; + delete process.env[SECRET.toLowerCase()]; + }); + + it('resolveEnvVars rejects Qwen-internal secrets instead of yielding the name as a value', () => { + process.env[SECRET] = 'daemon-secret'; + expect(() => resolveEnvVars(`$${SECRET}`)).toThrow( + `${SECRET} is a Qwen-internal secret`, + ); + }); + + it('resolveEnvVars rejects case variants (Windows-style process.env)', () => { + process.env[SECRET.toLowerCase()] = 'daemon-secret'; + expect(() => resolveEnvVars(`$${SECRET.toLowerCase()}`)).toThrow( + 'is a Qwen-internal secret', + ); + }); + + it('parseChannelConfig rejects internal secrets in channel credentials', async () => { + process.env[SECRET] = 'daemon-secret'; + + await expect( + parseChannelConfig('bot', { + type: 'github', + token: `$${SECRET}`, + }), + ).rejects.toThrow(`${SECRET} is a Qwen-internal secret`); + + await expect( + parseChannelConfig( + 'bot', + { type: 'github', token: `$${SECRET}` }, + undefined, + { resolveEnvVars: 'available' }, + ), + ).rejects.toThrow(`${SECRET} is a Qwen-internal secret`); + }); + + it('webhook secret and secretEnv reject Qwen-internal secrets', async () => { + process.env[SECRET] = 'daemon-secret'; + + await expect( + parseChannelConfig('dingtalk-main', { + type: 'bare', + token: 'token', + webhooks: { + sources: { + custom: { + secret: `$${SECRET}`, + targets: { default: { chatId: 'group-1', senderId: 'webhook' } }, + }, + }, + }, + }), + ).rejects.toThrow(`${SECRET} is a Qwen-internal secret`); + + // secretEnv used to return the variable *name* as the secret — a + // public constant that makes HMAC verification bypassable. + await expect( + parseChannelConfig('dingtalk-main', { + type: 'bare', + token: 'token', + webhooks: { + sources: { + custom: { + secretEnv: SECRET, + targets: { default: { chatId: 'group-1', senderId: 'webhook' } }, + }, + }, + }, + }), + ).rejects.toThrow( + `references a Qwen-internal secret (${SECRET}); internal secrets are never resolved into channel config`, + ); + }); +}); diff --git a/packages/cli/src/commands/channel/config-utils.ts b/packages/cli/src/commands/channel/config-utils.ts index 68c55d66d4e..6451bba743d 100644 --- a/packages/cli/src/commands/channel/config-utils.ts +++ b/packages/cli/src/commands/channel/config-utils.ts @@ -4,6 +4,7 @@ import type { ChannelWebhookSourceConfig, ChannelWebhookTargetConfig, } from '@qwen-code/channel-base'; +import { isInternalSecretEnvVar } from '@qwen-code/qwen-code-core/envVarResolver'; import { resolveChannelCwd } from './channel-cwd.js'; import { getPlugin, supportedTypes } from './channel-registry.js'; @@ -29,6 +30,16 @@ export function resolveEnvVars( } if (value.startsWith('$')) { const envName = value.substring(1); + // Qwen-internal secrets never resolve into channel config: resolved + // values are sent to repo-configured endpoints. The core resolver + // preserves placeholders, but channel config throws at config time + // like every other unresolvable reference — silently keeping the + // literal would turn a secret *name* into a public, guessable value. + if (isInternalSecretEnvVar(envName)) { + throw new Error( + `Environment variable ${envName} is a Qwen-internal secret; internal secrets are never resolved into channel config (referenced as ${value})`, + ); + } const envValue = env[envName]; if (envValue === undefined) { throw new Error( @@ -76,6 +87,11 @@ function resolveConfigEnvVar(value: string, mode: EnvResolution): string { if (value.startsWith('$$')) return value.substring(1); if (mode === 'available' && value.startsWith('$')) { const envName = value.substring(1); + if (isInternalSecretEnvVar(envName)) { + throw new Error( + `Environment variable ${envName} is a Qwen-internal secret; internal secrets are never resolved into channel config (referenced as ${value})`, + ); + } const envValue = process.env[envName]; if (envValue === undefined) { throw new Error( @@ -290,6 +306,11 @@ function resolveWebhookSecretEnv( `Channel "${channelName}" field "${path}.secretEnv" must be an environment variable name or $-prefixed reference.`, ); } + if (isInternalSecretEnvVar(envName)) { + throw new Error( + `Channel "${channelName}" field "${path}.secretEnv" references a Qwen-internal secret (${envName}); internal secrets are never resolved into channel config.`, + ); + } const envValue = env[envName]; if (envValue === undefined) { throw new Error( diff --git a/packages/cli/src/config/config.test.ts b/packages/cli/src/config/config.test.ts index 2022b3156ef..0be1ef96f34 100644 --- a/packages/cli/src/config/config.test.ts +++ b/packages/cli/src/config/config.test.ts @@ -3508,6 +3508,100 @@ describe('loadCliConfig allowPrivateNetworkHooks', () => { }); }); +describe('loadCliConfig allowedHttpHookUrls', () => { + const originalArgv = process.argv; + + beforeEach(() => { + vi.resetAllMocks(); + vi.mocked(os.homedir).mockReturnValue('/mock/home/user'); + vi.stubEnv('GEMINI_API_KEY', 'test-api-key'); + }); + + afterEach(() => { + process.argv = originalArgv; + vi.unstubAllEnvs(); + vi.restoreAllMocks(); + }); + + it('should pass through security.allowedHttpHookUrls from settings', async () => { + process.argv = ['node', 'script.js']; + const settings: Settings = { + security: { + allowedHttpHookUrls: ['https://hooks.corp.com/*'], + }, + }; + const argv = await parseArguments(); + const config = await loadCliConfig(settings, argv, undefined, []); + expect(config.getAllowedHttpHookUrls()).toEqual([ + 'https://hooks.corp.com/*', + ]); + }); + + it('should read a malformed non-array allowedHttpHookUrls as an empty list instead of crashing', async () => { + process.argv = ['node', 'script.js']; + // A hand-edited settings file can hold a bare string here (settings + // are validated only as top-level JSON objects); it must be coerced + // before reaching the UrlValidator instead of aborting startup. + const settings: Settings = { + security: { + allowedHttpHookUrls: 'https://hooks.corp.com/*' as unknown as string[], + }, + }; + const argv = await parseArguments(); + const config = await loadCliConfig(settings, argv, undefined, []); + expect(config.getAllowedHttpHookUrls()).toEqual([]); + }); + + it('should warn that a malformed allowedHttpHookUrls leaves HTTP hooks unrestricted', async () => { + process.argv = ['node', 'script.js']; + // The coercion to [] reads as "allow all" in the UrlValidator; the + // user's attempted restriction vanishes, so it must be surfaced. + const settings: Settings = { + security: { + allowedHttpHookUrls: 'https://hooks.corp.com/*' as unknown as string[], + }, + }; + const argv = await parseArguments(); + const config = await loadCliConfig(settings, argv, undefined, []); + expect(config.getAllowedHttpHookUrls()).toEqual([]); + expect( + config.getWarnings().some((w) => w.includes('allowedHttpHookUrls')), + ).toBe(true); + }); + + it('should not warn about allowedHttpHookUrls when the value is a valid list', async () => { + process.argv = ['node', 'script.js']; + const settings: Settings = { + security: { + allowedHttpHookUrls: ['https://hooks.corp.com/*'], + }, + }; + const argv = await parseArguments(); + const config = await loadCliConfig(settings, argv, undefined, []); + expect( + config.getWarnings().some((w) => w.includes('allowedHttpHookUrls')), + ).toBe(false); + }); + + it('should drop non-string allowedHttpHookUrls entries', async () => { + process.argv = ['node', 'script.js']; + const settings: Settings = { + security: { + allowedHttpHookUrls: [ + 42, + 'https://hooks.corp.com/ci/*', + null, + ] as unknown as string[], + }, + }; + const argv = await parseArguments(); + const config = await loadCliConfig(settings, argv, undefined, []); + expect(config.getAllowedHttpHookUrls()).toEqual([ + 'https://hooks.corp.com/ci/*', + ]); + }); +}); + describe('loadCliConfig with includeDirectories', () => { const originalArgv = process.argv; diff --git a/packages/cli/src/config/config.ts b/packages/cli/src/config/config.ts index bf538d25955..ba00da8035d 100755 --- a/packages/cli/src/config/config.ts +++ b/packages/cli/src/config/config.ts @@ -2328,10 +2328,27 @@ export async function loadCliConfig( warnings: resolvedCliConfig.warnings, bareMode, safeMode, - allowedHttpHookUrls: - bareMode || safeMode - ? [] - : (settings.security?.allowedHttpHookUrls ?? []), + allowedHttpHookUrls: (() => { + if (bareMode || safeMode) { + return []; + } + // Settings files are validated only as top-level JSON objects, so a + // hand-edited file can put a bare string or non-string entries here; + // reduce it to a valid list before it reaches the UrlValidator, which + // maps over it and would abort startup on a non-array. + const hookUrls = settings.security?.allowedHttpHookUrls; + if (hookUrls !== undefined && !Array.isArray(hookUrls)) { + // The coercion to [] reads as "allow all" in the UrlValidator, so + // surface the lost restriction instead of silently starting + // unrestricted. + resolvedCliConfig.warnings.push( + 'Warning: security.allowedHttpHookUrls is not a list and was ignored; HTTP hooks are unrestricted apart from SSRF protection.', + ); + } + return Array.isArray(hookUrls) + ? hookUrls.filter((entry): entry is string => typeof entry === 'string') + : []; + })(), allowPrivateNetworkHooks: bareMode || safeMode ? false diff --git a/packages/cli/src/config/settings.test.ts b/packages/cli/src/config/settings.test.ts index 87db8628a0a..0235337b632 100644 --- a/packages/cli/src/config/settings.test.ts +++ b/packages/cli/src/config/settings.test.ts @@ -3265,12 +3265,13 @@ describe('Settings Loading and Merging', () => { expect(settings.merged.security?.allowPrivateNetworkHooks).toBe(true); }); - it('should strip security.allowPrivateNetworkHooks from workspace scope even when trusted', () => { + it('should strip security.allowPrivateNetworkHooks and security.allowedHttpHookUrls from workspace scope even when trusted', () => { (mockFsExistsSync as Mock).mockReturnValue(true); const workspaceSettingsContent = { security: { allowPrivateNetworkHooks: true, allowedHttpHookUrls: ['https://hooks.example.com/*'], + folderTrust: { enabled: true }, }, }; @@ -3283,13 +3284,338 @@ describe('Settings Loading and Merging', () => { ); const settings = loadSettings(MOCK_WORKSPACE_DIR); - // The flag is ignored from workspace scope... + // Both hook-security overrides are ignored from workspace scope: + // a repository must not self-grant an SSRF relaxation nor replace + // the user's hook-payload whitelist. expect( settings.merged.security?.allowPrivateNetworkHooks, ).toBeUndefined(); - // ...but other workspace security settings still merge. + expect(settings.merged.security?.allowedHttpHookUrls).toBeUndefined(); + // ...but the strip is surgical: other workspace security settings + // still merge. + expect(settings.merged.security?.folderTrust).toEqual({ + enabled: true, + }); + }); + + it('should honor security.allowedHttpHookUrls from user scope', () => { + (mockFsExistsSync as Mock).mockReturnValue(true); + (fs.readFileSync as Mock).mockImplementation( + (p: fs.PathOrFileDescriptor) => { + if (p === USER_SETTINGS_PATH) + return JSON.stringify({ + security: { allowedHttpHookUrls: ['https://hooks.corp.com/*'] }, + }); + if (p === MOCK_WORKSPACE_SETTINGS_PATH) + return JSON.stringify({ + security: { allowedHttpHookUrls: ['*'] }, + }); + return '{}'; + }, + ); + + const settings = loadSettings(MOCK_WORKSPACE_DIR); + // The workspace's self-granted "*" is stripped; the user's + // whitelist survives. expect(settings.merged.security?.allowedHttpHookUrls).toEqual([ - 'https://hooks.example.com/*', + 'https://hooks.corp.com/*', + ]); + }); + + it('should let a trusted workspace whitelist narrow the user whitelist', () => { + (mockFsExistsSync as Mock).mockReturnValue(true); + (fs.readFileSync as Mock).mockImplementation( + (p: fs.PathOrFileDescriptor) => { + if (p === USER_SETTINGS_PATH) + return JSON.stringify({ + security: { allowedHttpHookUrls: ['https://hooks.corp.com/*'] }, + }); + if (p === MOCK_WORKSPACE_SETTINGS_PATH) + return JSON.stringify({ + security: { + allowedHttpHookUrls: ['https://hooks.corp.com/ci/*'], + }, + }); + return '{}'; + }, + ); + + const settings = loadSettings(MOCK_WORKSPACE_DIR); + // The workspace entry is covered by the user's pattern, so it + // narrows the effective whitelist instead of being dropped. + expect(settings.merged.security?.allowedHttpHookUrls).toEqual([ + 'https://hooks.corp.com/ci/*', + ]); + }); + + it('should drop workspace whitelist entries not covered by the user whitelist', () => { + (mockFsExistsSync as Mock).mockReturnValue(true); + (fs.readFileSync as Mock).mockImplementation( + (p: fs.PathOrFileDescriptor) => { + if (p === USER_SETTINGS_PATH) + return JSON.stringify({ + security: { allowedHttpHookUrls: ['https://hooks.corp.com/*'] }, + }); + if (p === MOCK_WORKSPACE_SETTINGS_PATH) + return JSON.stringify({ + security: { + allowedHttpHookUrls: [ + 'https://hooks.corp.com/ci/*', + 'https://evil.example.com/*', + ], + }, + }); + return '{}'; + }, + ); + + const settings = loadSettings(MOCK_WORKSPACE_DIR); + expect(settings.merged.security?.allowedHttpHookUrls).toEqual([ + 'https://hooks.corp.com/ci/*', + ]); + }); + + it('should fall back to the user whitelist when no workspace entry is covered', () => { + (mockFsExistsSync as Mock).mockReturnValue(true); + (fs.readFileSync as Mock).mockImplementation( + (p: fs.PathOrFileDescriptor) => { + if (p === USER_SETTINGS_PATH) + return JSON.stringify({ + security: { allowedHttpHookUrls: ['https://hooks.corp.com/*'] }, + }); + if (p === MOCK_WORKSPACE_SETTINGS_PATH) + return JSON.stringify({ + security: { + allowedHttpHookUrls: ['https://evil.example.com/*'], + }, + }); + return '{}'; + }, + ); + + const settings = loadSettings(MOCK_WORKSPACE_DIR); + // An empty intersection must mean "the user's policy stands", never + // "allow all" — the user's whitelist survives unchanged. + expect(settings.merged.security?.allowedHttpHookUrls).toEqual([ + 'https://hooks.corp.com/*', + ]); + }); + + it('should ignore a workspace whitelist when the user whitelist is explicitly empty', () => { + (mockFsExistsSync as Mock).mockReturnValue(true); + (fs.readFileSync as Mock).mockImplementation( + (p: fs.PathOrFileDescriptor) => { + if (p === USER_SETTINGS_PATH) + return JSON.stringify({ + security: { allowedHttpHookUrls: [] }, + }); + if (p === MOCK_WORKSPACE_SETTINGS_PATH) + return JSON.stringify({ + security: { + allowedHttpHookUrls: ['https://hooks.example.com/*'], + }, + }); + return '{}'; + }, + ); + + const settings = loadSettings(MOCK_WORKSPACE_DIR); + // An explicit empty user whitelist means "allow all"; the workspace + // may not establish policy on top of it. + expect(settings.merged.security?.allowedHttpHookUrls).toEqual([]); + }); + + it('should let the system whitelist override workspace narrowing', () => { + (mockFsExistsSync as Mock).mockReturnValue(true); + (fs.readFileSync as Mock).mockImplementation( + (p: fs.PathOrFileDescriptor) => { + if (p === USER_SETTINGS_PATH) + return JSON.stringify({ + security: { allowedHttpHookUrls: ['https://hooks.corp.com/*'] }, + }); + if (p === MOCK_WORKSPACE_SETTINGS_PATH) + return JSON.stringify({ + security: { + allowedHttpHookUrls: ['https://hooks.corp.com/ci/*'], + }, + }); + if (p === getSystemSettingsPath()) + return JSON.stringify({ + security: { + allowedHttpHookUrls: ['https://managed.example.com/*'], + }, + }); + return '{}'; + }, + ); + + const settings = loadSettings(MOCK_WORKSPACE_DIR); + // System scope is the final override: admin policy wins over both + // the user list and any workspace narrowing. + expect(settings.merged.security?.allowedHttpHookUrls).toEqual([ + 'https://managed.example.com/*', + ]); + }); + + it('should let a trusted workspace whitelist narrow the systemDefaults whitelist', () => { + (mockFsExistsSync as Mock).mockReturnValue(true); + (fs.readFileSync as Mock).mockImplementation( + (p: fs.PathOrFileDescriptor) => { + if (p === getSystemDefaultsPath()) + return JSON.stringify({ + security: { + allowedHttpHookUrls: ['https://managed.example.com/*'], + }, + }); + if (p === MOCK_WORKSPACE_SETTINGS_PATH) + return JSON.stringify({ + security: { + allowedHttpHookUrls: [ + 'https://managed.example.com/ci/*', + 'https://uncovered.example.com/*', + ], + }, + }); + return '{}'; + }, + ); + + const settings = loadSettings(MOCK_WORKSPACE_DIR); + // SystemDefaults is a documented honored scope: covered workspace + // entries narrow it; uncovered ones are dropped. + expect(settings.merged.security?.allowedHttpHookUrls).toEqual([ + 'https://managed.example.com/ci/*', + ]); + }); + + it('should judge workspace coverage by the user whitelist when both user and systemDefaults set one', () => { + // Pins the User-before-SystemDefaults precedence of the + // narrowing `??` chain: the workspace entry is covered only by + // the systemDefaults pattern, so it must be judged against the + // USER list, dropped, and the user's whitelist must stand. + (mockFsExistsSync as Mock).mockReturnValue(true); + (fs.readFileSync as Mock).mockImplementation( + (p: fs.PathOrFileDescriptor) => { + if (p === USER_SETTINGS_PATH) + return JSON.stringify({ + security: { allowedHttpHookUrls: ['https://hooks.corp.com/*'] }, + }); + if (p === getSystemDefaultsPath()) + return JSON.stringify({ + security: { + allowedHttpHookUrls: ['https://managed.example.com/*'], + }, + }); + if (p === MOCK_WORKSPACE_SETTINGS_PATH) + return JSON.stringify({ + security: { + allowedHttpHookUrls: ['https://managed.example.com/ci/*'], + }, + }); + return '{}'; + }, + ); + + const settings = loadSettings(MOCK_WORKSPACE_DIR); + expect(settings.merged.security?.allowedHttpHookUrls).toEqual([ + 'https://hooks.corp.com/*', + ]); + }); + + it('should survive a malformed workspace allowedHttpHookUrls and keep the user whitelist', () => { + (mockFsExistsSync as Mock).mockReturnValue(true); + (fs.readFileSync as Mock).mockImplementation( + (p: fs.PathOrFileDescriptor) => { + if (p === USER_SETTINGS_PATH) + return JSON.stringify({ + security: { allowedHttpHookUrls: ['https://hooks.corp.com/*'] }, + }); + if (p === MOCK_WORKSPACE_SETTINGS_PATH) + return JSON.stringify({ + // A common hand-edit to "clear" the list. + security: { allowedHttpHookUrls: null }, + }); + return '{}'; + }, + ); + + // Must not throw out of loadSettings, and the malformed value must + // not replace the user's list (which would read as "allow all"). + const settings = loadSettings(MOCK_WORKSPACE_DIR); + expect(settings.merged.security?.allowedHttpHookUrls).toEqual([ + 'https://hooks.corp.com/*', + ]); + }); + + it('should drop non-string workspace whitelist entries instead of throwing', () => { + (mockFsExistsSync as Mock).mockReturnValue(true); + (fs.readFileSync as Mock).mockImplementation( + (p: fs.PathOrFileDescriptor) => { + if (p === USER_SETTINGS_PATH) + return JSON.stringify({ + security: { allowedHttpHookUrls: ['https://hooks.corp.com/*'] }, + }); + if (p === MOCK_WORKSPACE_SETTINGS_PATH) + return JSON.stringify({ + security: { + allowedHttpHookUrls: [42, 'https://hooks.corp.com/ci/*', null], + }, + }); + return '{}'; + }, + ); + + const settings = loadSettings(MOCK_WORKSPACE_DIR); + expect(settings.merged.security?.allowedHttpHookUrls).toEqual([ + 'https://hooks.corp.com/ci/*', + ]); + }); + + it('should drop the workspace list when the higher-scope whitelist is malformed', () => { + (mockFsExistsSync as Mock).mockReturnValue(true); + (fs.readFileSync as Mock).mockImplementation( + (p: fs.PathOrFileDescriptor) => { + if (p === USER_SETTINGS_PATH) + return JSON.stringify({ + security: { allowedHttpHookUrls: 'https://hooks.corp.com/*' }, + }); + if (p === MOCK_WORKSPACE_SETTINGS_PATH) + return JSON.stringify({ + security: { + allowedHttpHookUrls: ['https://hooks.corp.com/ci/*'], + }, + }); + return '{}'; + }, + ); + + // No throw on the non-array higher-scope value; the workspace list + // is dropped and the user's value survives the merge untouched. + const settings = loadSettings(MOCK_WORKSPACE_DIR); + expect(settings.merged.security?.allowedHttpHookUrls).toBe( + 'https://hooks.corp.com/*', + ); + }); + + it('should not let a non-object workspace security section wipe higher-scope security', () => { + (mockFsExistsSync as Mock).mockReturnValue(true); + (fs.readFileSync as Mock).mockImplementation( + (p: fs.PathOrFileDescriptor) => { + if (p === USER_SETTINGS_PATH) + return JSON.stringify({ + security: { allowedHttpHookUrls: ['https://hooks.corp.com/*'] }, + }); + if (p === MOCK_WORKSPACE_SETTINGS_PATH) + return JSON.stringify({ security: null }); + return '{}'; + }, + ); + + const settings = loadSettings(MOCK_WORKSPACE_DIR); + // The workspace's `security: null` is dropped instead of replacing + // the user's security section (an empty whitelist means "allow all"). + expect(settings.merged.security?.allowedHttpHookUrls).toEqual([ + 'https://hooks.corp.com/*', ]); }); @@ -3312,6 +3638,97 @@ describe('Settings Loading and Merging', () => { ).toBe(true); }); + it('should warn when workspace settings define security.allowedHttpHookUrls', () => { + (mockFsExistsSync as Mock).mockReturnValue(true); + (fs.readFileSync as Mock).mockImplementation( + (p: fs.PathOrFileDescriptor) => { + if (p === MOCK_WORKSPACE_SETTINGS_PATH) + return JSON.stringify({ + security: { + allowedHttpHookUrls: ['https://hooks.example.com/*'], + }, + }); + return '{}'; + }, + ); + + const settings = loadSettings(MOCK_WORKSPACE_DIR); + const warnings = getSettingsWarnings(settings); + const warning = warnings.find((w) => + w.includes('security.allowedHttpHookUrls'), + ); + expect(warning).toBeDefined(); + // The workspace list may only narrow a higher-scope whitelist; the + // warning must explain that uncovered entries are dropped and what + // happens when no higher-scope whitelist exists ("allow all" still + // applies then), so neither case reads as neutral. + expect(warning).toContain('can only narrow'); + expect(warning).toContain('unrestricted apart from SSRF protection'); + }); + + it('should not emit the narrowing warning when the workspace is not trusted', () => { + vi.mocked(isWorkspaceTrusted).mockReturnValue({ + isTrusted: false, + source: 'file', + }); + (mockFsExistsSync as Mock).mockReturnValue(true); + (fs.readFileSync as Mock).mockImplementation( + (p: fs.PathOrFileDescriptor) => { + if (p === MOCK_WORKSPACE_SETTINGS_PATH) + return JSON.stringify({ + security: { + allowedHttpHookUrls: ['https://hooks.example.com/*'], + }, + }); + return '{}'; + }, + ); + + const settings = loadSettings(MOCK_WORKSPACE_DIR); + // Untrusted workspace settings are discarded whole, so there is no + // narrowing to describe. + const warnings = getSettingsWarnings(settings); + expect( + warnings.some((w) => w.includes('security.allowedHttpHookUrls')), + ).toBe(false); + }); + + it('should not warn about stripped security fields when workspace settings do not define them', () => { + (mockFsExistsSync as Mock).mockReturnValue(true); + (fs.readFileSync as Mock).mockImplementation( + (p: fs.PathOrFileDescriptor) => { + if (p === MOCK_WORKSPACE_SETTINGS_PATH) + return JSON.stringify({ + security: { folderTrust: { enabled: true } }, + }); + return '{}'; + }, + ); + + const settings = loadSettings(MOCK_WORKSPACE_DIR); + const warnings = getSettingsWarnings(settings); + expect( + warnings.some((w) => w.includes('security.allowPrivateNetworkHooks')), + ).toBe(false); + expect( + warnings.some((w) => w.includes('security.allowedHttpHookUrls')), + ).toBe(false); + }); + + it('should not warn about stripped security fields when no workspace settings file is loaded', () => { + // beforeEach defaults (existsSync -> false): no settings file loads, + // so there is no workspace scope to strip or warn about. + const settings = loadSettings(MOCK_WORKSPACE_DIR); + expect(settings.workspace.rawJson).toBeUndefined(); + const warnings = getSettingsWarnings(settings); + expect( + warnings.some((w) => w.includes('security.allowPrivateNetworkHooks')), + ).toBe(false); + expect( + warnings.some((w) => w.includes('security.allowedHttpHookUrls')), + ).toBe(false); + }); + it('should let user scope win over a stripped workspace value', () => { (mockFsExistsSync as Mock).mockReturnValue(true); (fs.readFileSync as Mock).mockImplementation( diff --git a/packages/cli/src/config/settings.ts b/packages/cli/src/config/settings.ts index dde69e27621..77e2fc2fff7 100644 --- a/packages/cli/src/config/settings.ts +++ b/packages/cli/src/config/settings.ts @@ -14,6 +14,7 @@ import { Storage, createDebugLogger, stripRuntimeSnapshotPrefix, + hookUrlPatternCovers, } from '@qwen-code/qwen-code-core'; import type { MCPServerConfig, @@ -30,7 +31,7 @@ import { type SettingDefinition, getSettingsSchema, } from './settingsSchema.js'; -import { resolveEnvVarsInObject } from '../utils/envVarResolver.js'; +import { resolveEnvVarsInObject } from '@qwen-code/qwen-code-core/envVarResolver'; import { setNestedPropertySafe } from '../utils/settingsUtils.js'; import { customDeepMerge } from '../utils/deepMerge.js'; import { updateSettingsFilePreservingFormat } from '../utils/jsonc-editor.js'; @@ -320,6 +321,15 @@ function getModelProvidersOverrideWarnings( ]; } +// Hook security settings a Workspace scope may never widen. The boolean +// SSRF bypass is stripped outright; the URL whitelist is intersected with +// the higher-scope list so a workspace can only narrow it (see +// narrowWorkspaceHookSecurityOverrides). getSettingsWarnings reports both. +const WORKSPACE_STRIPPED_SECURITY_FIELDS = [ + 'allowPrivateNetworkHooks', + 'allowedHttpHookUrls', +] as const; + /** * Collects warnings for ignored legacy and unknown settings keys, * as well as migration warnings. @@ -358,17 +368,27 @@ export function getSettingsWarnings(loadedSettings: LoadedSettings): string[] { warningSet.add(warning); } - // security.allowPrivateNetworkHooks is stripped from Workspace scope during - // the merge; warn so the user knows their workspace setting has no effect. + // WORKSPACE_STRIPPED_SECURITY_FIELDS may not widen hook security from + // Workspace scope (see narrowWorkspaceHookSecurityOverrides); warn so the + // user knows how their workspace setting is treated. const workspaceFile = loadedSettings.forScope(SettingScope.Workspace); - if ( - workspaceFile.rawJson !== undefined && - workspaceFile.originalSettings.security?.allowPrivateNetworkHooks !== - undefined - ) { - warningSet.add( - `Warning: security.allowPrivateNetworkHooks in workspace settings (${workspaceFile.path}) is ignored. This setting is only honored from User, System, or SystemDefaults scope settings.`, - ); + if (workspaceFile.rawJson !== undefined) { + const workspaceSecurity = workspaceFile.originalSettings.security; + if (workspaceSecurity?.allowPrivateNetworkHooks !== undefined) { + warningSet.add( + `Warning: security.allowPrivateNetworkHooks in workspace settings (${workspaceFile.path}) is ignored. This setting is only honored from User, System, or SystemDefaults scope settings.`, + ); + } + // Narrowing only applies in trusted folders; untrusted workspace + // settings are discarded whole, so there is nothing to narrow. + if ( + loadedSettings.isTrusted && + workspaceSecurity?.allowedHttpHookUrls !== undefined + ) { + warningSet.add( + `Warning: security.allowedHttpHookUrls in workspace settings (${workspaceFile.path}) can only narrow the whitelist from User or SystemDefaults scope settings: entries not covered by a higher-scope whitelist are dropped, and the workspace value is ignored entirely when no higher-scope whitelist is set (HTTP hooks then remain unrestricted apart from SSRF protection). A System-scope whitelist always takes precedence over the workspace value.`, + ); + } } if ( workspaceFile.rawJson !== undefined && @@ -408,24 +428,101 @@ function tagMcpServerScope( } /** - * Network security bypasses must never be honored from Workspace scope — - * otherwise a malicious repository could self-grant access to private - * infrastructure. Strip them from workspace settings before merging. + * `security.allowPrivateNetworkHooks` relaxes SSRF protection for HTTP + * hooks, `security.allowedHttpHookUrls` controls where HTTP hooks may POST + * agent data, and `security.allowedInsecureVoiceBaseUrls` exempts voice + * providers from the cleartext/private-endpoint checks. A Workspace scope + * may widen none of them — otherwise a malicious repository could + * self-grant a bypass (point hooks or voice traffic at link-local or + * private infrastructure) or widen the whitelist to exfiltrate hook + * payloads past the user's configured boundary: + * + * - the boolean bypass and the voice base URL list are stripped outright; + * - the whitelist is intersected with the effective User/System/ + * SystemDefaults whitelist, so a trusted workspace can only NARROW the + * destinations the user already allowed. Entries no higher-scope entry + * covers are dropped; when no higher-scope whitelist exists, or nothing + * survives the intersection, the workspace list is ignored entirely — + * an empty whitelist means "allow all" to the hook URL validator, so + * discarding a narrowing list must fall back to the higher-scope policy, + * never to unrestricted. + * * Returns a shallow copy — never mutates input. */ -function stripWorkspaceSecurityBypasses(settings: Settings): Settings { +function narrowWorkspaceHookSecurityOverrides( + workspace: Settings, + system: Settings, + user: Settings, + systemDefaults: Settings, +): Settings { + const security = workspace.security; + if (security === undefined) { + return workspace; + } if ( - settings.security?.allowPrivateNetworkHooks === undefined && - settings.security?.allowedInsecureVoiceBaseUrls === undefined + security === null || + typeof security !== 'object' || + Array.isArray(security) ) { - return settings; + // A non-object `security` (e.g. `"security": null` from a hand-edited + // file) carries nothing to narrow, but customDeepMerge would still + // assign it over the higher-scope object and wipe the user's entire + // security section — which the hook URL validator reads as "allow + // all". Drop it so the higher-scope policy survives. + const rest = { ...workspace }; + delete rest.security; + return rest; + } + if ( + WORKSPACE_STRIPPED_SECURITY_FIELDS.every( + (field) => security[field] === undefined, + ) && + security.allowedInsecureVoiceBaseUrls === undefined + ) { + return workspace; + } + const restSecurity = { ...security }; + delete restSecurity.allowPrivateNetworkHooks; + // Trusted-scope-only bypass like the boolean above: a workspace must not + // self-grant cleartext/private voice endpoints. + delete restSecurity.allowedInsecureVoiceBaseUrls; + + if (security.allowedHttpHookUrls !== undefined) { + // Settings files are validated only as top-level JSON objects, so a + // hand-edited file can put null, a bare string, or non-string entries + // here; reduce both sides to valid string lists before comparing. + const workspaceUrls = Array.isArray(security.allowedHttpHookUrls) + ? security.allowedHttpHookUrls.filter( + (entry): entry is string => typeof entry === 'string', + ) + : []; + const higherUrlsRaw = + system.security?.allowedHttpHookUrls ?? + user.security?.allowedHttpHookUrls ?? + systemDefaults.security?.allowedHttpHookUrls; + const higherUrls = Array.isArray(higherUrlsRaw) + ? higherUrlsRaw.filter( + (entry): entry is string => typeof entry === 'string', + ) + : undefined; + const narrowed = + higherUrls === undefined + ? [] + : workspaceUrls.filter((entry) => + higherUrls.some((higher) => hookUrlPatternCovers(higher, entry)), + ); + // A non-empty intersection replaces the higher-scope list (it is a + // subset of what that list allows); otherwise the higher-scope policy + // stands unchanged — a malformed workspace value is dropped here + // instead of reaching the merge, where it would replace the user's + // list and read as "allow all". + if (narrowed.length > 0) { + restSecurity.allowedHttpHookUrls = narrowed; + } else { + delete restSecurity.allowedHttpHookUrls; + } } - const { - allowPrivateNetworkHooks: _privateHooks, - allowedInsecureVoiceBaseUrls: _insecureVoice, - ...restSecurity - } = settings.security; - return { ...settings, security: restSecurity }; + return { ...workspace, security: restSecurity }; } function mergeSettings( @@ -436,7 +533,15 @@ function mergeSettings( isTrusted: boolean, ): Settings { const safeWorkspace = isTrusted - ? tagMcpServerScope(stripWorkspaceSecurityBypasses(workspace), 'workspace') + ? tagMcpServerScope( + narrowWorkspaceHookSecurityOverrides( + workspace, + system, + user, + systemDefaults, + ), + 'workspace', + ) : ({} as Settings); // Settings are merged with the following precedence (last one wins for diff --git a/packages/cli/src/config/settingsSchema.ts b/packages/cli/src/config/settingsSchema.ts index 3fd86adc972..e899773ba17 100644 --- a/packages/cli/src/config/settingsSchema.ts +++ b/packages/cli/src/config/settingsSchema.ts @@ -3089,7 +3089,7 @@ const SETTINGS_SCHEMA = { requiresRestart: false, default: [] as string[], description: - 'Whitelist of URL patterns for HTTP hooks. Supports * wildcard. If empty, all URLs are allowed (subject to SSRF protection).', + 'Whitelist of URL patterns for HTTP hooks. Supports * wildcard. If empty, all URLs are allowed (subject to SSRF protection). Honored from User, System, and SystemDefaults settings scopes; a value set in Workspace settings can only narrow the User or SystemDefaults whitelist (entries not covered by it are dropped, and it is ignored entirely when no higher-scope whitelist is set), and a System-scope whitelist always takes precedence over it, so a cloned repository cannot widen where hook payloads may be sent.', showInDialog: false, items: { type: 'string', diff --git a/packages/cli/src/serve/fast-path-settings.ts b/packages/cli/src/serve/fast-path-settings.ts index d8ea34e86cc..7338d03d190 100644 --- a/packages/cli/src/serve/fast-path-settings.ts +++ b/packages/cli/src/serve/fast-path-settings.ts @@ -31,7 +31,7 @@ import { } from '../config/trust-precedence.js'; import { publishPendingCompileCache } from '../config/compile-cache.js'; import type { Settings } from '../config/settingsSchema.js'; -import { resolveEnvVarsInObject } from '../utils/envVarResolver.js'; +import { resolveEnvVarsInObject } from '@qwen-code/qwen-code-core/envVarResolver'; type ServeFastPathPolicy = Pick< NonNullable, diff --git a/packages/cli/src/services/SkillCommandLoader.test.ts b/packages/cli/src/services/SkillCommandLoader.test.ts index d9c200ccfa1..13a22335b8c 100644 --- a/packages/cli/src/services/SkillCommandLoader.test.ts +++ b/packages/cli/src/services/SkillCommandLoader.test.ts @@ -11,7 +11,7 @@ import { } from './SkillCommandLoader.js'; import { skillArgsPath } from './skill-args-file.js'; import { mkdtempSync, rmSync, readFileSync } from 'node:fs'; -import { tmpdir } from 'node:os'; +import * as os from 'node:os'; import { join } from 'node:path'; import { CommandKind, type CommandContext } from '../ui/commands/types.js'; import { @@ -57,6 +57,7 @@ describe('SkillCommandLoader', () => { getBareMode: vi.fn().mockReturnValue(false), getProjectRoot: vi.fn().mockReturnValue('/test/project'), getAutoSkillEnabled: vi.fn().mockReturnValue(true), + isTrustedFolder: vi.fn().mockReturnValue(true), getPermissionManager: vi .fn() .mockReturnValue({ addSessionAllowRule: mockAddSessionAllowRule }), @@ -290,7 +291,7 @@ describe('SkillCommandLoader', () => { it('should append raw invocation when args are provided', async () => { // The args file is written relative to the process's directory; without a // temp cwd this suite would write into the real repository. - const dir = mkdtempSync(join(tmpdir(), 'skill-cmd-args-')); + const dir = mkdtempSync(join(os.tmpdir(), 'skill-cmd-args-')); const cwd = process.cwd(); process.chdir(dir); try { @@ -497,6 +498,167 @@ describe('SkillCommandLoader', () => { expect(mockAddSessionAllowRule).not.toHaveBeenCalled(); }); + + it('does not grant allowedTools for a project skill in an untrusted folder', async () => { + (mockConfig.isTrustedFolder as ReturnType).mockReturnValue( + false, + ); + const skill = makeSkill({ + level: 'project', + allowedTools: ['Bash(git *)', 'Edit'], + }); + mockSkillManager.listSkills.mockImplementation( + ({ level }: { level: string }) => + Promise.resolve(level === 'project' ? [skill] : []), + ); + + const loader = new SkillCommandLoader(mockConfig); + const commands = await loader.loadCommands(signal); + await commands[0].action?.({} as CommandContext, ''); + + expect(mockAddSessionAllowRule).not.toHaveBeenCalled(); + }); + + it('still submits the skill body when allowedTools are gated', async () => { + (mockConfig.isTrustedFolder as ReturnType).mockReturnValue( + false, + ); + const skill = makeSkill({ + level: 'project', + allowedTools: ['Edit'], + }); + mockSkillManager.listSkills.mockImplementation( + ({ level }: { level: string }) => + Promise.resolve(level === 'project' ? [skill] : []), + ); + + const loader = new SkillCommandLoader(mockConfig); + const commands = await loader.loadCommands(signal); + const result = await commands[0].action?.({} as CommandContext, ''); + + expect(mockAddSessionAllowRule).not.toHaveBeenCalled(); + // Read-only use stays fine: only the permission grants are gated. + expect(result).toMatchObject({ + type: 'submit_prompt', + content: [{ text: makeSkillPrompt('Skill body content.') }], + }); + }); + + it('grants allowedTools for a project skill in a trusted folder', async () => { + (mockConfig.isTrustedFolder as ReturnType).mockReturnValue( + true, + ); + const skill = makeSkill({ + level: 'project', + allowedTools: ['Bash(git *)', 'Edit'], + }); + mockSkillManager.listSkills.mockImplementation( + ({ level }: { level: string }) => + Promise.resolve(level === 'project' ? [skill] : []), + ); + + const loader = new SkillCommandLoader(mockConfig); + const commands = await loader.loadCommands(signal); + await commands[0].action?.({} as CommandContext, ''); + + expect(mockAddSessionAllowRule).toHaveBeenCalledTimes(2); + }); + + it('grants allowedTools for a user-level skill regardless of folder trust', async () => { + (mockConfig.isTrustedFolder as ReturnType).mockReturnValue( + false, + ); + const skill = makeSkill({ + level: 'user', + allowedTools: ['Bash(git *)', 'Edit'], + }); + mockSkillManager.listSkills.mockImplementation( + ({ level }: { level: string }) => + Promise.resolve(level === 'user' ? [skill] : []), + ); + + const loader = new SkillCommandLoader(mockConfig); + const commands = await loader.loadCommands(signal); + await commands[0].action?.({} as CommandContext, ''); + + expect(mockAddSessionAllowRule).toHaveBeenCalledTimes(2); + }); + + it('grants allowedTools for an extension-level skill regardless of folder trust', async () => { + (mockConfig.isTrustedFolder as ReturnType).mockReturnValue( + false, + ); + const skill = makeSkill({ + level: 'extension', + allowedTools: ['Bash(git *)', 'Edit'], + }); + mockSkillManager.listSkills.mockImplementation( + ({ level }: { level: string }) => + Promise.resolve(level === 'extension' ? [skill] : []), + ); + + const loader = new SkillCommandLoader(mockConfig); + const commands = await loader.loadCommands(signal); + await commands[0].action?.({} as CommandContext, ''); + + expect(mockAddSessionAllowRule).toHaveBeenCalledTimes(2); + }); + + it('does not grant allowedTools for a home-root-shadowed user-level skill in an untrusted folder', async () => { + // SkillManager skips the 'project' level when the project root IS + // the home directory, so repository-committed skills surface at + // 'user' level there (tagged `homeRootShadow` at collection time) + // and must stay gated on folder trust. + (mockConfig.isTrustedFolder as ReturnType).mockReturnValue( + false, + ); + const skill = makeSkill({ + level: 'user', + homeRootShadow: true, + allowedTools: ['Bash(git *)', 'Edit'], + }); + mockSkillManager.listSkills.mockImplementation( + ({ level }: { level: string }) => + Promise.resolve(level === 'user' ? [skill] : []), + ); + + const loader = new SkillCommandLoader(mockConfig); + const commands = await loader.loadCommands(signal); + await commands[0].action?.({} as CommandContext, ''); + + expect(mockAddSessionAllowRule).not.toHaveBeenCalled(); + }); + + it('re-reads folder trust at invocation time, not load time', async () => { + // Workspace trust can flip mid-session without a restart in IDE + // mode (Config.isTrustedFolder() live-reads the IDE context). The + // gate must re-read trust inside the action closure — hoisting the + // read into loadCommands scope would freeze the untrusted verdict + // and silently diverge from the SkillTool path. + (mockConfig.isTrustedFolder as ReturnType).mockReturnValue( + false, + ); + const skill = makeSkill({ + level: 'project', + allowedTools: ['Bash(git *)', 'Edit'], + }); + mockSkillManager.listSkills.mockImplementation( + ({ level }: { level: string }) => + Promise.resolve(level === 'project' ? [skill] : []), + ); + + const loader = new SkillCommandLoader(mockConfig); + const commands = await loader.loadCommands(signal); + + (mockConfig.isTrustedFolder as ReturnType).mockReturnValue( + true, + ); + await commands[0].action?.({} as CommandContext, ''); + + expect(mockAddSessionAllowRule).toHaveBeenCalledTimes(2); + expect(mockAddSessionAllowRule).toHaveBeenNthCalledWith(1, 'Bash(git *)'); + expect(mockAddSessionAllowRule).toHaveBeenNthCalledWith(2, 'Edit'); + }); }); describe('skills.disabled filter', () => { diff --git a/packages/cli/src/services/SkillCommandLoader.ts b/packages/cli/src/services/SkillCommandLoader.ts index fd0b594b08b..0c6b43cb3b3 100644 --- a/packages/cli/src/services/SkillCommandLoader.ts +++ b/packages/cli/src/services/SkillCommandLoader.ts @@ -10,6 +10,7 @@ import { appendToLastTextPart, buildSkillLlmContent, applySkillAllowedTools, + isTrustedSkillLevel, recordAutoSkillUsage, } from '@qwen-code/qwen-code-core'; import { dirname } from 'node:path'; @@ -151,11 +152,29 @@ export class SkillCommandLoader implements ICommandLoader { : {}), }, action: async (context, _args): Promise => { - // Auto-approve the skill's declared allowedTools before its body is submitted. - applySkillAllowedTools( - this.config?.getPermissionManager(), - skill.allowedTools, - ); + // Auto-approve the skill's declared allowedTools before its body + // is submitted — never for repo-supplied skills in an untrusted + // folder, where frontmatter would otherwise grant session-wide + // permission auto-approvals. Same fail-closed gate as SkillTool: + // only levels that cannot originate from the repository skip it, + // and when the project root IS the home directory, listing skips + // 'project' and repository-committed skills surface at 'user' + // level — SkillManager records that on the skill as + // `homeRootShadow`, so that combination is gated too. + if ( + (isTrustedSkillLevel(skill.level) && + !(skill.level === 'user' && skill.homeRootShadow === true)) || + this.config?.isTrustedFolder() + ) { + applySkillAllowedTools( + this.config?.getPermissionManager(), + skill.allowedTools, + ); + } else if (skill.allowedTools?.length) { + debugLogger.warn( + `Skill "${skill.name}" declares allowedTools but the folder is not trusted; ignoring skill allowedTools.`, + ); + } const body = buildSkillLlmContent( dirname(skill.filePath), diff --git a/packages/cli/src/utils/envVarResolver.test.ts b/packages/cli/src/utils/envVarResolver.test.ts deleted file mode 100644 index a45b65b1b98..00000000000 --- a/packages/cli/src/utils/envVarResolver.test.ts +++ /dev/null @@ -1,297 +0,0 @@ -/** - * @license - * Copyright 2025 Google LLC - * SPDX-License-Identifier: Apache-2.0 - */ - -import { describe, it, expect, beforeEach, afterEach } from 'vitest'; -import { - resolveEnvVarsInString, - resolveEnvVarsInObject, -} from './envVarResolver.js'; - -describe('resolveEnvVarsInString', () => { - let originalEnv: NodeJS.ProcessEnv; - - beforeEach(() => { - originalEnv = { ...process.env }; - }); - - afterEach(() => { - process.env = originalEnv; - }); - - it('should resolve $VAR_NAME format', () => { - process.env['TEST_VAR'] = 'test-value'; - - const result = resolveEnvVarsInString('Value is $TEST_VAR'); - - expect(result).toBe('Value is test-value'); - }); - - it('should resolve ${VAR_NAME} format', () => { - process.env['TEST_VAR'] = 'test-value'; - - const result = resolveEnvVarsInString('Value is ${TEST_VAR}'); - - expect(result).toBe('Value is test-value'); - }); - - it('should resolve multiple variables in the same string', () => { - process.env['HOST'] = 'localhost'; - process.env['PORT'] = '3000'; - - const result = resolveEnvVarsInString('URL: http://$HOST:${PORT}/api'); - - expect(result).toBe('URL: http://localhost:3000/api'); - }); - - it('should leave undefined variables unchanged', () => { - const result = resolveEnvVarsInString('Value is $UNDEFINED_VAR'); - - expect(result).toBe('Value is $UNDEFINED_VAR'); - }); - - it('should leave undefined variables with braces unchanged', () => { - const result = resolveEnvVarsInString('Value is ${UNDEFINED_VAR}'); - - expect(result).toBe('Value is ${UNDEFINED_VAR}'); - }); - - it('should handle empty string', () => { - const result = resolveEnvVarsInString(''); - - expect(result).toBe(''); - }); - - it('should handle string without variables', () => { - const result = resolveEnvVarsInString('No variables here'); - - expect(result).toBe('No variables here'); - }); - - it('should handle mixed defined and undefined variables', () => { - process.env['DEFINED'] = 'value'; - - const result = resolveEnvVarsInString('$DEFINED and $UNDEFINED mixed'); - - expect(result).toBe('value and $UNDEFINED mixed'); - }); -}); - -describe('resolveEnvVarsInObject', () => { - let originalEnv: NodeJS.ProcessEnv; - - beforeEach(() => { - originalEnv = { ...process.env }; - }); - - afterEach(() => { - process.env = originalEnv; - }); - - it('should resolve variables in nested objects', () => { - process.env['API_KEY'] = 'secret-123'; - process.env['DB_URL'] = 'postgresql://localhost/test'; - - const config = { - server: { - auth: { - key: '$API_KEY', - }, - database: '${DB_URL}', - }, - port: 3000, - }; - - const result = resolveEnvVarsInObject(config); - - expect(result).toEqual({ - server: { - auth: { - key: 'secret-123', - }, - database: 'postgresql://localhost/test', - }, - port: 3000, - }); - }); - - it('should resolve variables in arrays', () => { - process.env['ENV'] = 'production'; - process.env['VERSION'] = '1.0.0'; - - const config = { - tags: ['$ENV', 'app', '${VERSION}'], - metadata: { - env: '$ENV', - }, - }; - - const result = resolveEnvVarsInObject(config); - - expect(result).toEqual({ - tags: ['production', 'app', '1.0.0'], - metadata: { - env: 'production', - }, - }); - }); - - it('should preserve non-string types', () => { - const config = { - enabled: true, - count: 42, - value: null, - data: undefined, - tags: ['item1', 'item2'], - }; - - const result = resolveEnvVarsInObject(config); - - expect(result).toEqual(config); - }); - - it('should handle MCP server config structure', () => { - process.env['API_TOKEN'] = 'token-123'; - process.env['SERVER_PORT'] = '8080'; - - const extensionConfig = { - name: 'test-extension', - version: '1.0.0', - mcpServers: { - 'test-server': { - command: 'node', - args: ['server.js', '--port', '${SERVER_PORT}'], - env: { - API_KEY: '$API_TOKEN', - STATIC_VALUE: 'unchanged', - }, - timeout: 5000, - }, - }, - }; - - const result = resolveEnvVarsInObject(extensionConfig); - - expect(result).toEqual({ - name: 'test-extension', - version: '1.0.0', - mcpServers: { - 'test-server': { - command: 'node', - args: ['server.js', '--port', '8080'], - env: { - API_KEY: 'token-123', - STATIC_VALUE: 'unchanged', - }, - timeout: 5000, - }, - }, - }); - }); - - it('should handle empty and null values', () => { - const config = { - empty: '', - nullValue: null, - undefinedValue: undefined, - zero: 0, - false: false, - }; - - const result = resolveEnvVarsInObject(config); - - expect(result).toEqual(config); - }); - - it('should handle circular references in objects without infinite recursion', () => { - process.env['TEST_VAR'] = 'resolved-value'; - - type ConfigWithCircularRef = { - name: string; - value: number; - self?: ConfigWithCircularRef; - }; - - const config: ConfigWithCircularRef = { - name: '$TEST_VAR', - value: 42, - }; - // Create circular reference - config.self = config; - - const result = resolveEnvVarsInObject(config); - - expect(result.name).toBe('resolved-value'); - expect(result.value).toBe(42); - expect(result.self).toBeDefined(); - expect(result.self?.name).toBe('$TEST_VAR'); // Circular reference should be shallow copied - expect(result.self?.value).toBe(42); - // Verify it doesn't create infinite recursion by checking it's not the same object - expect(result.self).not.toBe(result); - }); - - it('should handle circular references in arrays without infinite recursion', () => { - process.env['ARRAY_VAR'] = 'array-value'; - - type ArrayWithCircularRef = Array; - const arr: ArrayWithCircularRef = ['$ARRAY_VAR', 123]; - // Create circular reference - arr.push(arr); - - const result = resolveEnvVarsInObject(arr) as ArrayWithCircularRef; - - expect(result[0]).toBe('array-value'); - expect(result[1]).toBe(123); - expect(Array.isArray(result[2])).toBe(true); - const subArray = result[2] as ArrayWithCircularRef; - expect(subArray[0]).toBe('$ARRAY_VAR'); // Circular reference should be shallow copied - expect(subArray[1]).toBe(123); - // Verify it doesn't create infinite recursion - expect(result[2]).not.toBe(result); - }); - - it('should handle complex nested circular references', () => { - process.env['NESTED_VAR'] = 'nested-resolved'; - - type ObjWithRef = { - name: string; - id: number; - ref?: ObjWithRef; - }; - - const obj1: ObjWithRef = { name: '$NESTED_VAR', id: 1 }; - const obj2: ObjWithRef = { name: 'static', id: 2 }; - - // Create cross-references - obj1.ref = obj2; - obj2.ref = obj1; - - const config = { - primary: obj1, - secondary: obj2, - value: '$NESTED_VAR', - }; - - const result = resolveEnvVarsInObject(config); - - expect(result.value).toBe('nested-resolved'); - expect(result.primary.name).toBe('nested-resolved'); - expect(result.primary.id).toBe(1); - expect(result.secondary.name).toBe('static'); - expect(result.secondary.id).toBe(2); - - // Check that circular references are handled (shallow copied) - expect(result.primary.ref).toBeDefined(); - expect(result.secondary.ref).toBeDefined(); - expect(result.primary.ref?.name).toBe('static'); // Should be shallow copy - expect(result.secondary.ref?.name).toBe('nested-resolved'); // The shallow copy still gets processed - - // Most importantly: verify no infinite recursion by checking objects are different - expect(result.primary.ref).not.toBe(result.secondary); - expect(result.secondary.ref).not.toBe(result.primary); - expect(result.primary).not.toBe(obj1); // New object created - expect(result.secondary).not.toBe(obj2); // New object created - }); -}); diff --git a/packages/cli/src/utils/envVarResolver.ts b/packages/cli/src/utils/envVarResolver.ts deleted file mode 100644 index 096b5549ec0..00000000000 --- a/packages/cli/src/utils/envVarResolver.ts +++ /dev/null @@ -1,126 +0,0 @@ -/** - * @license - * Copyright 2025 Google LLC - * SPDX-License-Identifier: Apache-2.0 - */ - -/** - * Resolves environment variables in a string. - * Replaces $VAR_NAME and ${VAR_NAME} with their corresponding environment variable values. - * If the environment variable is not defined, the original placeholder is preserved. - * - * @param value - The string that may contain environment variable placeholders - * @returns The string with environment variables resolved - * - * @example - * resolveEnvVarsInString("Token: $API_KEY") // Returns "Token: secret-123" - * resolveEnvVarsInString("URL: ${BASE_URL}/api") // Returns "URL: https://api.example.com/api" - * resolveEnvVarsInString("Missing: $UNDEFINED_VAR") // Returns "Missing: $UNDEFINED_VAR" - */ -export function resolveEnvVarsInString( - value: string, - customEnv?: Record, -): string { - const envVarRegex = /\$(?:(\w+)|{([^}]+)})/g; // Find $VAR_NAME or ${VAR_NAME} - return value.replace(envVarRegex, (match, varName1, varName2) => { - const varName = varName1 || varName2; - if (customEnv && typeof customEnv[varName] === 'string') { - return customEnv[varName]; - } - if (process && process.env && typeof process.env[varName] === 'string') { - return process.env[varName]!; - } - return match; - }); -} - -/** - * Recursively resolves environment variables in an object of any type. - * Handles strings, arrays, nested objects, and preserves other primitive types. - * Protected against circular references using a WeakSet to track visited objects. - * - * @param obj - The object to process for environment variable resolution - * @returns A new object with environment variables resolved - * - * @example - * const config = { - * server: { - * host: "$HOST", - * port: "${PORT}", - * enabled: true, - * tags: ["$ENV", "api"] - * } - * }; - * const resolved = resolveEnvVarsInObject(config); - */ -export function resolveEnvVarsInObject( - obj: T, - customEnv?: Record, -): T { - return resolveEnvVarsInObjectInternal(obj, new WeakSet(), customEnv); -} - -/** - * Internal implementation of resolveEnvVarsInObject with circular reference protection. - * - * @param obj - The object to process - * @param visited - WeakSet to track visited objects and prevent circular references - * @returns A new object with environment variables resolved - */ -function resolveEnvVarsInObjectInternal( - obj: T, - visited: WeakSet, - customEnv?: Record, -): T { - if ( - obj === null || - obj === undefined || - typeof obj === 'boolean' || - typeof obj === 'number' - ) { - return obj; - } - - if (typeof obj === 'string') { - return resolveEnvVarsInString(obj, customEnv) as unknown as T; - } - - if (Array.isArray(obj)) { - // Check for circular reference - if (visited.has(obj)) { - // Return a shallow copy to break the cycle - return [...obj] as unknown as T; - } - - visited.add(obj); - const result = obj.map((item) => - resolveEnvVarsInObjectInternal(item, visited, customEnv), - ) as unknown as T; - visited.delete(obj); - return result; - } - - if (typeof obj === 'object') { - // Check for circular reference - if (visited.has(obj as object)) { - // Return a shallow copy to break the cycle - return { ...obj } as T; - } - - visited.add(obj as object); - const newObj = { ...obj } as T; - for (const key in newObj) { - if (Object.prototype.hasOwnProperty.call(newObj, key)) { - newObj[key] = resolveEnvVarsInObjectInternal( - newObj[key], - visited, - customEnv, - ); - } - } - visited.delete(obj as object); - return newObj; - } - - return obj; -} diff --git a/packages/cli/tsconfig.json b/packages/cli/tsconfig.json index 0b71d728730..fb3799ee9b4 100644 --- a/packages/cli/tsconfig.json +++ b/packages/cli/tsconfig.json @@ -11,6 +11,9 @@ "@qwen-code/qwen-code-core/transcriptRecords": [ "../core/src/utils/transcript-records.ts" ], + "@qwen-code/qwen-code-core/envVarResolver": [ + "../core/src/utils/envVarResolver.ts" + ], "@qwen-code/qwen-code-core/*": ["../core/src/*"], "@qwen-code/acp-bridge": ["../acp-bridge/src/index.ts"], "@qwen-code/acp-bridge/transcriptReplay": [ diff --git a/packages/cli/vitest.config.ts b/packages/cli/vitest.config.ts index 21ad36fe76d..8b705198e93 100644 --- a/packages/cli/vitest.config.ts +++ b/packages/cli/vitest.config.ts @@ -27,6 +27,10 @@ export default defineConfig({ __dirname, '../core/src/memory/scopes.ts', ), + '@qwen-code/qwen-code-core/envVarResolver': path.resolve( + __dirname, + '../core/src/utils/envVarResolver.ts', + ), '@qwen-code/qwen-code-core/toolWriteOrigin': path.resolve( __dirname, '../core/src/services/tool-write-origin.ts', diff --git a/packages/core/package.json b/packages/core/package.json index c8a295f95d8..69585458c30 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -33,6 +33,10 @@ "types": "./dist/src/hooks/user-prompt-submit-context.d.ts", "import": "./dist/src/hooks/user-prompt-submit-context.js" }, + "./envVarResolver": { + "types": "./dist/src/utils/envVarResolver.d.ts", + "import": "./dist/src/utils/envVarResolver.js" + }, "./package.json": "./package.json", "./dist/*": "./dist/*", "./src/*": "./src/*" diff --git a/packages/core/src/hooks/envInterpolator.test.ts b/packages/core/src/hooks/envInterpolator.test.ts index 043a4f85ed3..289248145ef 100644 --- a/packages/core/src/hooks/envInterpolator.test.ts +++ b/packages/core/src/hooks/envInterpolator.test.ts @@ -4,7 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; import { interpolateEnvVars, interpolateHeaders, @@ -14,6 +14,17 @@ import { sanitizeHeaderValue, } from './envInterpolator.js'; +const mockDebugLogger = vi.hoisted(() => ({ + isEnabled: vi.fn(() => false), + debug: vi.fn(), + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), +})); +vi.mock('../utils/debugLogger.js', () => ({ + createDebugLogger: vi.fn(() => mockDebugLogger), +})); + describe('envInterpolator', () => { const originalEnv = process.env; @@ -22,6 +33,7 @@ describe('envInterpolator', () => { process.env['MY_TOKEN'] = 'secret-token'; process.env['API_KEY'] = 'api-key-123'; process.env['EMPTY_VAR'] = ''; + mockDebugLogger.warn.mockClear(); }); afterEach(() => { @@ -62,6 +74,40 @@ describe('envInterpolator', () => { expect(result).toBe(''); }); + it('should never interpolate Qwen-internal secrets, even when whitelisted', () => { + process.env['QWEN_SERVER_TOKEN'] = 'daemon-secret'; + const result = interpolateEnvVars('token=$QWEN_SERVER_TOKEN', [ + 'QWEN_SERVER_TOKEN', + ]); + expect(result).toBe('token='); + expect(result).not.toContain('daemon-secret'); + }); + + it('should warn in the debug log, naming the blocked internal-secret variable', () => { + process.env['QWEN_SERVER_TOKEN'] = 'daemon-secret'; + interpolateEnvVars('token=$QWEN_SERVER_TOKEN', ['QWEN_SERVER_TOKEN']); + expect(mockDebugLogger.warn).toHaveBeenCalledTimes(1); + const message = String(mockDebugLogger.warn.mock.calls[0]?.[0]); + expect(message).toContain('QWEN_SERVER_TOKEN'); + // The variable name is diagnostic; its value must never be logged. + expect(message).not.toContain('daemon-secret'); + }); + + it('should not warn for variables that are merely outside the whitelist', () => { + interpolateEnvVars('token=$OTHER_VAR', ['MY_TOKEN']); + expect(mockDebugLogger.warn).not.toHaveBeenCalled(); + }); + + it('should block internal secrets regardless of casing', () => { + process.env['QWEN_SERVER_TOKEN'] = 'daemon-secret'; + process.env['qwen_server_token'] = 'daemon-secret'; + const result = interpolateEnvVars('token=$qwen_server_token', [ + 'qwen_server_token', + ]); + expect(result).toBe('token='); + expect(result).not.toContain('daemon-secret'); + }); + it('should handle empty whitelist', () => { const result = interpolateEnvVars('$MY_TOKEN', []); expect(result).toBe(''); diff --git a/packages/core/src/hooks/envInterpolator.ts b/packages/core/src/hooks/envInterpolator.ts index 5f04781e4ce..a66b8f89981 100644 --- a/packages/core/src/hooks/envInterpolator.ts +++ b/packages/core/src/hooks/envInterpolator.ts @@ -9,6 +9,11 @@ * Provides secure interpolation with whitelist-based access control. */ +import { isInternalSecretEnvVar } from '../utils/sanitize-child-env.js'; +import { createDebugLogger } from '../utils/debugLogger.js'; + +const debugLogger = createDebugLogger('ENV_INTERPOLATOR'); + /** * Strip CR, LF, and NUL bytes from a header value to prevent HTTP header * injection (CRLF injection) via env var values or hook-configured header @@ -65,6 +70,19 @@ export function interpolateEnvVars( if (!isSafeVarName(varName)) { return ''; } + // Qwen-internal secrets (daemon tokens, private capabilities) are + // never interpolated, even when a hook config names them in + // allowedEnvVars — the whitelist is repo-controlled, the values + // leave the process over the network. The warn names the variable + // (never its value): this path silently substitutes '', unlike the + // sibling resolvers that keep the placeholder or throw, so the + // debug log is the only way to diagnose the lost reference. + if (isInternalSecretEnvVar(varName)) { + debugLogger.warn( + `Hook value references Qwen-internal secret env var "${varName}"; it is never interpolated and was replaced with an empty string.`, + ); + return ''; + } if (allowedVars.includes(varName)) { return process.env[varName] || ''; } diff --git a/packages/core/src/hooks/hookAggregator.test.ts b/packages/core/src/hooks/hookAggregator.test.ts index 33b7664d3e1..511753244bf 100644 --- a/packages/core/src/hooks/hookAggregator.test.ts +++ b/packages/core/src/hooks/hookAggregator.test.ts @@ -110,6 +110,26 @@ describe('HookAggregator', () => { expect(result.finalOutput?.reason).toBe('first reason\nsecond reason'); }); + it('should concatenate systemMessages so a one-shot message survives later hooks', () => { + const outputs: HookOutput[] = [ + { continue: true, systemMessage: 'Warning: redirect to final URL' }, + { continue: true, systemMessage: 'audit ok' }, + ]; + + const results: HookExecutionResult[] = outputs.map((output) => ({ + hookConfig: { type: HookType.Command, command: 'echo test' }, + eventName: HookEventName.Stop, + success: true, + output, + duration: 100, + })); + + const result = aggregator.aggregateResults(results, HookEventName.Stop); + expect(result.finalOutput?.systemMessage).toBe( + 'Warning: redirect to final URL\naudit ok', + ); + }); + it('should block when any hook blocks', () => { const outputs: HookOutput[] = [ { reason: 'allowed', decision: 'allow' }, @@ -367,6 +387,29 @@ describe('HookAggregator', () => { expect(hookOutput.getDenyMessage()).toBe('msg1\nmsg2'); }); + it('should carry top-level systemMessage through so a one-shot warning survives', () => { + const outputs: HookOutput[] = [ + { continue: true, systemMessage: 'Warning: redirect to final URL' }, + { hookSpecificOutput: { decision: { behavior: 'allow' } } }, + ]; + + const results: HookExecutionResult[] = outputs.map((output) => ({ + hookConfig: { type: HookType.Command, command: 'echo test' }, + eventName: HookEventName.PermissionRequest, + success: true, + output, + duration: 100, + })); + + const result = aggregator.aggregateResults( + results, + HookEventName.PermissionRequest, + ); + expect(result.finalOutput?.systemMessage).toBe( + 'Warning: redirect to final URL', + ); + }); + it('should use last updatedInput', () => { const outputs: HookOutput[] = [ { @@ -548,6 +591,29 @@ describe('HookAggregator', () => { expect(result.finalOutput?.continue).toBe(false); }); + it('should concatenate systemMessages so a one-shot message survives later hooks', () => { + const outputs: HookOutput[] = [ + { continue: true, systemMessage: 'Warning: redirect to final URL' }, + { continue: true, systemMessage: 'session audited' }, + ]; + + const results: HookExecutionResult[] = outputs.map((output) => ({ + hookConfig: { type: HookType.Command, command: 'echo test' }, + eventName: HookEventName.SessionStart, + success: true, + output, + duration: 100, + })); + + const result = aggregator.aggregateResults( + results, + HookEventName.SessionStart, + ); + expect(result.finalOutput?.systemMessage).toBe( + 'Warning: redirect to final URL\nsession audited', + ); + }); + it('should concatenate additionalContext from multiple hooks', () => { const outputs: HookOutput[] = [ { diff --git a/packages/core/src/hooks/hookAggregator.ts b/packages/core/src/hooks/hookAggregator.ts index db3f7d99bb2..ee8cc3405fb 100644 --- a/packages/core/src/hooks/hookAggregator.ts +++ b/packages/core/src/hooks/hookAggregator.ts @@ -135,6 +135,7 @@ export class HookAggregator { * Rules: * - Any "block" or "deny" decision results in blocking (most restrictive wins) * - Reasons are concatenated with newlines + * - System messages are concatenated with newlines * - continue=false takes precedence over continue=true * - Additional context is concatenated * - For PostToolUse, decision and reason are required fields @@ -195,13 +196,13 @@ export class HookAggregator { } } - // Copy other fields (later values win for simple fields) + // Copy other fields (later values win for simple fields; + // systemMessage is concatenated like reason so a one-shot message + // from an earlier hook survives later hooks). if (output.suppressOutput !== undefined) { merged.suppressOutput = output.suppressOutput; } - if (output.systemMessage !== undefined) { - merged.systemMessage = output.systemMessage; - } + this.appendSystemMessage(merged, output); } // Concatenate terminal sequences from all outputs @@ -251,6 +252,7 @@ export class HookAggregator { * Rules: * - behavior: deny wins over allow (security priority) * - message: concatenated with newlines + * - systemMessage: concatenated with newlines * - updatedInput: later values win * - updatedPermissions: concatenated * - interrupt: true wins over false @@ -265,6 +267,9 @@ export class HookAggregator { const allUpdatedPermissions: Array<{ type: string; tool?: string }> = []; for (const output of outputs) { + // Top-level systemMessage (e.g. the one-shot redirect warning) must + // survive even when an output carries no permission decision. + this.appendSystemMessage(merged, output); const specific = output.hookSpecificOutput; if (!specific) continue; @@ -366,10 +371,14 @@ export class HookAggregator { for (const output of outputs) { // Collect additionalContext for concatenation this.extractAdditionalContext(output, additionalContexts); - // Exclude terminalSequence from spread — it is concatenated below - const { terminalSequence: _ts, ...rest } = output; + // Exclude terminalSequence and systemMessage from the spread — both + // are concatenated below, so a one-shot message from an earlier hook + // survives later outputs instead of being overwritten. + const { terminalSequence: _ts, systemMessage: _sm, ...rest } = output; void _ts; + void _sm; merged = { ...merged, ...rest }; + this.appendSystemMessage(merged, output); } // Merge additionalContext with concatenation @@ -431,6 +440,20 @@ export class HookAggregator { } } + /** + * Append an output's systemMessage to merged, concatenating so a + * one-shot message from an earlier hook survives later outputs. + */ + private appendSystemMessage(merged: HookOutput, output: HookOutput): void { + if (output.systemMessage !== undefined) { + merged.systemMessage = merged.systemMessage + ? [merged.systemMessage, output.systemMessage] + .filter(Boolean) + .join('\n') + : output.systemMessage; + } + } + /** * Extract additional context from hook-specific outputs */ diff --git a/packages/core/src/hooks/httpHookRunner.test.ts b/packages/core/src/hooks/httpHookRunner.test.ts index 6a8b11eaa25..db9ffafdc1d 100644 --- a/packages/core/src/hooks/httpHookRunner.test.ts +++ b/packages/core/src/hooks/httpHookRunner.test.ts @@ -9,6 +9,17 @@ import { HookEventName, HookType } from './types.js'; import type { HttpHookConfig, HookInput } from './types.js'; import { HttpHookRunner } from './httpHookRunner.js'; +const mockDebugLogger = vi.hoisted(() => ({ + isEnabled: vi.fn(() => false), + debug: vi.fn(), + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), +})); +vi.mock('../utils/debugLogger.js', () => ({ + createDebugLogger: vi.fn(() => mockDebugLogger), +})); + // Mock fetch const mockFetch = vi.fn(); vi.stubGlobal('fetch', mockFetch); @@ -165,6 +176,7 @@ describe('HttpHookRunner', () => { ok: false, status: 500, statusText: 'Internal Server Error', + headers: new Headers(), }); const config = createMockConfig(); @@ -179,6 +191,16 @@ describe('HttpHookRunner', () => { // Non-2xx is a non-blocking error, so success should be true expect(result.success).toBe(true); expect(result.output?.continue).toBe(true); + // Pins the branch condition: a plain server error must get the + // generic non-2xx message, never the redirect diagnostics. + expect(mockDebugLogger.warn).toHaveBeenCalledWith( + expect.stringContaining('returned non-2xx status 500'), + ); + expect(mockDebugLogger.warn).not.toHaveBeenCalledWith( + expect.stringContaining('returned a redirect'), + ); + // Only 3xx responses surface a user-visible warning. + expect(result.output?.systemMessage).toBeUndefined(); }); it('should handle timeout as non-blocking error', async () => { @@ -231,6 +253,252 @@ describe('HttpHookRunner', () => { expect(mockFetch).toHaveBeenCalledTimes(1); // Still 1 }); + it('should not consume a once hook slot on an undelivered 3xx redirect', async () => { + mockFetch.mockResolvedValue({ + ok: false, + status: 302, + headers: new Headers({ location: 'https://api.example.com/moved' }), + }); + + const config = createMockConfig({ once: true }); + const input = createMockInput(); + + const first = await httpRunner.execute( + config, + HookEventName.SessionStart, + input, + ); + const second = await httpRunner.execute( + config, + HookEventName.SessionStart, + input, + ); + + // A redirect delivers no payload, so it cannot consume the one + // execution: both firings must fetch instead of skipping. + expect(mockFetch).toHaveBeenCalledTimes(2); + expect(first.output?.continue).toBe(true); + expect(first.output?.systemMessage).toContain( + 'returned a redirect (302)', + ); + // The redirect-warning slot is independent of the once slot: the + // remedy is still emitted only once. + expect(second.output?.systemMessage).toBeUndefined(); + expect(second.output?.continue).toBe(true); + }); + + it('should not follow redirects — 3xx is a non-blocking error and the target is never contacted', async () => { + mockFetch.mockResolvedValueOnce({ + ok: false, + status: 302, + headers: new Headers({ + location: 'http://169.254.169.254/latest/meta-data', + }), + json: async () => ({}), + }); + + const config = createMockConfig(); + const input = createMockInput(); + + const result = await httpRunner.execute( + config, + HookEventName.PreToolUse, + input, + ); + + // Non-2xx (incl. 3xx) is a non-blocking error per spec + expect(result.success).toBe(true); + expect(result.output?.continue).toBe(true); + // Exactly one request: the redirect target is never fetched + expect(mockFetch).toHaveBeenCalledTimes(1); + // The remedy rides a systemMessage. How it surfaces is + // event-dependent: Stop/SubagentStop show it to the user; other + // events only write it to the debug-file channel — either way it + // is carried in the hook output. + expect(result.output?.systemMessage).toContain( + 'returned a redirect (302)', + ); + // The 3xx gets a dedicated, self-service warning naming the target + expect(mockDebugLogger.warn).toHaveBeenCalledWith( + expect.stringContaining('returned a redirect (302)'), + ); + expect(mockDebugLogger.warn).toHaveBeenCalledWith( + expect.stringContaining('http://169.254.169.254/latest/meta-data'), + ); + expect(mockFetch).toHaveBeenCalledWith( + 'https://api.example.com/hook', + expect.objectContaining({ redirect: 'manual' }), + ); + }); + + it.each([301, 307, 308])( + 'should treat %i redirects the same way (never follow)', + async (status) => { + mockFetch.mockResolvedValueOnce({ + ok: false, + status, + headers: new Headers({ location: 'https://api.example.com/moved' }), + }); + + const result = await httpRunner.execute( + createMockConfig(), + HookEventName.PreToolUse, + createMockInput(), + ); + + expect(result.success).toBe(true); + expect(result.output?.continue).toBe(true); + expect(mockFetch).toHaveBeenCalledTimes(1); + expect(result.output?.systemMessage).toContain( + `returned a redirect (${status})`, + ); + }, + ); + + it('should report "unknown" when a 3xx response has no Location header', async () => { + mockFetch.mockResolvedValueOnce({ + ok: false, + status: 302, + headers: new Headers(), + }); + + const result = await httpRunner.execute( + createMockConfig(), + HookEventName.PreToolUse, + createMockInput(), + ); + + expect(result.success).toBe(true); + expect(result.output?.systemMessage).toContain('"unknown"'); + }); + + it('should sanitize the attacker-controlled Location header before it reaches the systemMessage', async () => { + // A real Headers object rejects CRLF values outright, but a raw + // socket response can carry them; stub the getter to simulate one. + const evilLocation = + 'http://evil.example/\r\nFakeBoundary: 1' + 'A'.repeat(20000); + mockFetch.mockResolvedValueOnce({ + ok: false, + status: 302, + headers: { get: () => evilLocation }, + }); + + const result = await httpRunner.execute( + createMockConfig(), + HookEventName.PreToolUse, + createMockInput(), + ); + + const message = result.output?.systemMessage ?? ''; + // CR/LF stripped (the remainder concatenates), size capped like the + // other systemMessage producers. + expect(message).not.toContain('\r'); + expect(message).toContain('http://evil.example/FakeBoundary: 1'); + expect(message).toContain('[truncated'); + expect(message.length).toBeLessThan(11000); + }); + + it('should emit the redirect warning once per hook URL and event, then stay debug-only', async () => { + const redirectResponse = { + ok: false, + status: 302, + headers: new Headers({ location: 'https://api.example.com/moved' }), + }; + mockFetch + .mockResolvedValueOnce(redirectResponse) + .mockResolvedValueOnce(redirectResponse); + + const config = createMockConfig(); + const input = createMockInput(); + + const first = await httpRunner.execute( + config, + HookEventName.PreToolUse, + input, + ); + const second = await httpRunner.execute( + config, + HookEventName.PreToolUse, + input, + ); + + expect(first.output?.systemMessage).toContain( + 'returned a redirect (302)', + ); + // A PreToolUse hook behind a redirecting LB fires on every tool + // call; the remedy is emitted once, later runs stay debug-only. + expect(second.output?.systemMessage).toBeUndefined(); + expect(second.output?.continue).toBe(true); + expect( + mockDebugLogger.warn.mock.calls.filter(([msg]) => + String(msg).includes('returned a redirect (302)'), + ), + ).toHaveLength(2); + }); + + it('should keep a warn slot per event so a PreToolUse 3xx does not burn the Stop warning', async () => { + const redirectResponse = { + ok: false, + status: 302, + headers: new Headers({ location: 'https://api.example.com/moved' }), + }; + mockFetch + .mockResolvedValueOnce(redirectResponse) + .mockResolvedValueOnce(redirectResponse); + + const config = createMockConfig(); + const input = createMockInput(); + + const preToolUse = await httpRunner.execute( + config, + HookEventName.PreToolUse, + input, + ); + const stop = await httpRunner.execute(config, HookEventName.Stop, input); + + expect(preToolUse.output?.systemMessage).toContain( + 'returned a redirect (302)', + ); + // Stop/SubagentStop is where the systemMessage actually reaches + // the user; it keeps its own slot instead of inheriting the spent + // PreToolUse one. + expect(stop.output?.systemMessage).toContain('returned a redirect (302)'); + }); + + it('should warn again when the hook URL changes, even for a shared name', async () => { + const redirectResponse = { + ok: false, + status: 302, + headers: new Headers({ location: 'https://api.example.com/moved' }), + }; + mockFetch + .mockResolvedValueOnce(redirectResponse) + .mockResolvedValueOnce(redirectResponse); + + const input = createMockInput(); + + const before = await httpRunner.execute( + createMockConfig({ name: 'api-hook' }), + HookEventName.PreToolUse, + input, + ); + const after = await httpRunner.execute( + createMockConfig({ + name: 'api-hook', + url: 'https://api.example.com/hook-v2', + }), + HookEventName.PreToolUse, + input, + ); + + expect(before.output?.systemMessage).toContain( + 'returned a redirect (302)', + ); + expect(after.output?.systemMessage).toContain( + 'returned a redirect (302)', + ); + }); + it('should parse JSON response with hook output', async () => { mockFetch.mockResolvedValueOnce({ ok: true, @@ -487,5 +755,39 @@ describe('HttpHookRunner', () => { await httpRunner.execute(config, HookEventName.PreToolUse, input); expect(mockFetch).toHaveBeenCalledTimes(2); }); + + it('should re-arm the redirect warning together with the once slot', async () => { + mockFetch.mockResolvedValue({ + ok: false, + status: 302, + headers: new Headers({ location: 'https://api.example.com/moved' }), + }); + + const config = createMockConfig({ once: true }); + const input = createMockInput(); + + const first = await httpRunner.execute( + config, + HookEventName.SessionStart, + input, + ); + expect(first.output?.systemMessage).toContain( + 'returned a redirect (302)', + ); + + httpRunner.resetOnceHooks(); + + // The re-armed hook fetches again and the user-visible remedy is + // shown again — the warning slot must not survive the reset. + const afterReset = await httpRunner.execute( + config, + HookEventName.SessionStart, + input, + ); + expect(mockFetch).toHaveBeenCalledTimes(2); + expect(afterReset.output?.systemMessage).toContain( + 'returned a redirect (302)', + ); + }); }); }); diff --git a/packages/core/src/hooks/httpHookRunner.ts b/packages/core/src/hooks/httpHookRunner.ts index ced660c3d8d..c7432d94461 100644 --- a/packages/core/src/hooks/httpHookRunner.ts +++ b/packages/core/src/hooks/httpHookRunner.ts @@ -5,7 +5,11 @@ */ import { createDebugLogger } from '../utils/debugLogger.js'; -import { interpolateHeaders, interpolateUrl } from './envInterpolator.js'; +import { + interpolateHeaders, + interpolateUrl, + sanitizeHeaderValue, +} from './envInterpolator.js'; import { UrlValidator } from './urlValidator.js'; import { combineAbortSignals } from '../utils/abortController.js'; import { isBlockedAddress, isMetadataAddress } from './ssrfGuard.js'; @@ -106,6 +110,7 @@ export class HttpHookRunner { private urlValidator: UrlValidator; private readonly allowPrivateNetworkHosts: boolean; private readonly executedOnceHooks: Set = new Set(); + private readonly redirectWarnedHooks: Set = new Set(); private statusMessageCallback?: StatusMessageCallback; constructor( @@ -151,8 +156,8 @@ export class HttpHookRunner { } // Check once flag + const onceKey = `${hookConfig.url}:${eventName}`; if (hookConfig.once) { - const onceKey = `${hookConfig.url}:${eventName}`; if (this.executedOnceHooks.has(onceKey)) { debugLogger.debug( `Skipping once hook ${hookId} - already executed for ${eventName}`, @@ -243,6 +248,11 @@ export class HttpHookRunner { }, body, signal: combinedSignal, + // Never follow redirects: the whitelist and DNS-level SSRF + // checks above cover only this URL, and a 307/308 would re-send + // the hook payload to an unvalidated target. A 3xx response + // falls into the non-2xx branch below (non-blocking error). + redirect: 'manual', }); cleanup(); @@ -252,6 +262,58 @@ export class HttpHookRunner { // Per Qwen Code spec: Non-2xx status is a non-blocking error // Execution continues, but we log a warning if (!response.ok) { + if (response.status >= 300 && response.status < 400) { + // A redirect delivers no payload, so it must not consume a + // once hook's single execution: drop the slot added above so + // the hook fires again instead of silently no-op-ing forever. + if (hookConfig.once) { + this.executedOnceHooks.delete(onceKey); + } + // With redirect: 'manual' a 3xx lands here; the endpoint is + // behind a redirecting LB and silently no-ops unless the user + // knows to repoint it, so name the target and the remedy. + // debugLogger.warn alone is invisible in default runs, so also + // surface it the way command-hook non-blocking errors do. + // + // The Location header is controlled by the (possibly + // compromised) endpoint: strip CR/LF/NUL and cap it like the + // other systemMessage producers before it can reach the + // conversation. + const location = this.truncateOutput( + sanitizeHeaderValue( + response.headers.get('location') ?? 'unknown', + ), + ); + const message = + `HTTP hook ${hookId} returned a redirect (${response.status}) to ` + + `"${location}"; redirects ` + + `are never followed (SSRF protection). Point the hook at the ` + + `final URL (non-blocking).`; + debugLogger.warn(message); + // The remedy only needs saying once per hook URL and event + // (keyed like executedOnceHooks): a PreToolUse hook behind a + // redirecting LB would otherwise emit it on every tool call, + // and event-scoping keeps a debug-only PreToolUse 3xx from + // burning the slot a user-visible Stop emission needs. + const warnKey = `${hookConfig.url}:${eventName}`; + if (this.redirectWarnedHooks.has(warnKey)) { + return { + hookConfig, + eventName, + success: true, + output: { continue: true }, + duration, + }; + } + this.redirectWarnedHooks.add(warnKey); + return { + hookConfig, + eventName, + success: true, + output: { continue: true, systemMessage: `Warning: ${message}` }, + duration, + }; + } debugLogger.warn( `HTTP hook ${hookId} returned non-2xx status ${response.status} (non-blocking)`, ); @@ -442,6 +504,7 @@ export class HttpHookRunner { */ resetOnceHooks(): void { this.executedOnceHooks.clear(); + this.redirectWarnedHooks.clear(); } /** diff --git a/packages/core/src/hooks/index.ts b/packages/core/src/hooks/index.ts index 316b8d0c494..7ebd0425741 100644 --- a/packages/core/src/hooks/index.ts +++ b/packages/core/src/hooks/index.ts @@ -37,7 +37,11 @@ export { hasEnvVarReferences, extractEnvVarNames, } from './envInterpolator.js'; -export { UrlValidator, createUrlValidator } from './urlValidator.js'; +export { + UrlValidator, + createUrlValidator, + hookUrlPatternCovers, +} from './urlValidator.js'; // Export interfaces and enums export type { HookRegistryEntry } from './hookRegistry.js'; diff --git a/packages/core/src/hooks/urlValidator.test.ts b/packages/core/src/hooks/urlValidator.test.ts index 90ce6b8b37f..ee96d36a52e 100644 --- a/packages/core/src/hooks/urlValidator.test.ts +++ b/packages/core/src/hooks/urlValidator.test.ts @@ -5,7 +5,11 @@ */ import { describe, it, expect } from 'vitest'; -import { UrlValidator, createUrlValidator } from './urlValidator.js'; +import { + UrlValidator, + createUrlValidator, + hookUrlPatternCovers, +} from './urlValidator.js'; describe('UrlValidator', () => { describe('isBlocked', () => { @@ -198,3 +202,167 @@ describe('UrlValidator', () => { }); }); }); + +describe('hookUrlPatternCovers', () => { + it('treats identical patterns as covering', () => { + expect( + hookUrlPatternCovers('https://corp.com/*', 'https://corp.com/*'), + ).toBe(true); + }); + + it('lets a wildcard-suffix entry narrow its parent pattern', () => { + expect( + hookUrlPatternCovers('https://corp.com/*', 'https://corp.com/hooks/*'), + ).toBe(true); + }); + + it('lets an exact URL narrow a wildcard pattern', () => { + expect( + hookUrlPatternCovers('https://corp.com/*', 'https://corp.com/ci'), + ).toBe(true); + }); + + it('rejects entries outside the higher-scope pattern', () => { + expect( + hookUrlPatternCovers('https://corp.com/*', 'https://evil.com/*'), + ).toBe(false); + }); + + it('rejects a lookalike host that extends a literal chunk', () => { + expect( + hookUrlPatternCovers('https://corp.com/*', 'https://corp.com.evil.com/*'), + ).toBe(false); + }); + + it('rejects the catch-all "*" against a bounded pattern', () => { + expect(hookUrlPatternCovers('https://corp.com/*', '*')).toBe(false); + }); + + it('lets the catch-all "*" cover any entry', () => { + expect(hookUrlPatternCovers('*', 'https://corp.com/hooks/*')).toBe(true); + }); + + it('matches case-insensitively like the URL validator', () => { + expect( + hookUrlPatternCovers('https://CORP.com/*', 'https://corp.com/hooks/*'), + ).toBe(true); + }); + + it('fails closed on non-ASCII patterns whose case folding diverges at runtime', () => { + // toLowerCase() folds ẞ (U+1E9E) to ß (U+00DF), equating these + // hosts, but the runtime's non-Unicode /i regex never matches the two + // across — a "covers" verdict would let the inner entry survive the + // merge while its runtime regex admits a host the outer pattern + // rejects. Hook URLs are realistically ASCII, so anything else is + // unprovable and dropped. + expect( + hookUrlPatternCovers( + 'https://fuß.example.com/*', + 'https://fuẞ.example.com/*', + ), + ).toBe(false); + // Identical non-ASCII patterns fail closed too — the runtime matching + // relation is unprovable for them regardless of spelling. + expect( + hookUrlPatternCovers( + 'https://bücher.example.com/*', + 'https://bücher.example.com/*', + ), + ).toBe(false); + // Why this must fail closed: the runtime regex compiled from the + // outer pattern rejects the host that toLowerCase() calls equal. + const runtime = new UrlValidator(['https://fuß.example.com/*']); + expect(runtime.isAllowed('https://fuẞ.example.com/hook')).toBe(false); + }); + + it('treats pre-escaped and unescaped spellings as equivalent', () => { + expect( + hookUrlPatternCovers( + 'https://api\\.example\\.com/*', + 'https://api.example.com/hooks/*', + ), + ).toBe(true); + expect( + hookUrlPatternCovers( + 'https://api.example.com/*', + 'https://api\\.example\\.com/hooks/*', + ), + ).toBe(true); + }); + + it('rejects entries that add a wildcard outside the higher-scope literals', () => { + expect( + hookUrlPatternCovers('https://corp.com/*', 'https://*/corp.com/*'), + ).toBe(false); + }); + + it('does not let escapes other than \\. widen coverage (fails closed)', () => { + // `\*` is not the documented `\.` escape; normalizing it into a + // catch-all would widen the higher-scope policy. + expect(hookUrlPatternCovers('\\*', 'https://corp.com/*')).toBe(false); + }); + + it('fails closed on alternation inside a pre-escaped entry', () => { + // The literal reading covers, but the runtime compiles a pre-escaped + // pattern's non-* text as raw regex: the ungrouped alternation would + // match hosts outside the higher-scope pattern. + expect( + hookUrlPatternCovers('https://corp\\.com/*', 'https://corp\\.com/x|y/*'), + ).toBe(false); + }); + + it('fails closed when a pattern carries regex classes beyond \\.', () => { + // `\d` reads literally here, but the runtime treats it as the digit + // class, so the literal comparison cannot prove coverage. + expect( + hookUrlPatternCovers( + 'https://hooks\\.corp\\.com/status/\\d+/*', + 'https://hooks.corp.com/status/1/exfil', + ), + ).toBe(false); + expect( + hookUrlPatternCovers('https://corp.com/*', 'https://corp.com/a+b'), + ).toBe(false); + }); + + it('fails closed when a pre-escaped entry keeps a bare dot', () => { + // `compilePattern` takes the pre-escaped branch once a pattern carries + // `\.`, reading every non-* character as raw regex: a surviving bare + // dot is a wildcard there, so the literal comparison cannot prove + // coverage — the runtime regex matches lookalike hosts the outer + // pattern excludes. + expect( + hookUrlPatternCovers( + 'https://hooks.corp.com/*', + 'https://hooks\\.corp.com/*', + ), + ).toBe(false); + expect( + hookUrlPatternCovers( + 'https://hooks.example.co.uk/*', + 'https://hooks\\.example.co.uk/*', + ), + ).toBe(false); + // Why this must fail closed: the pre-escaped runtime regex treats the + // bare dots as wildcards and matches a different public host. + const runtime = new UrlValidator(['https://hooks\\.example.co.uk/*']); + expect(runtime.isAllowed('https://hooks.example.co/uk/exfil')).toBe(true); + }); + + it('stays linear on long near-miss entries instead of backtracking', () => { + const start = Date.now(); + // The input passes the startsWith/endsWith anchors, so the chunk-scan + // loop itself must walk the ~200 KB body before rejecting it: a + // quadratic rescan or regex-based containment would blow the bound. + expect( + hookUrlPatternCovers( + 'https://hooks.corp.com/*/*/*/done', + `https://hooks.corp.com/${'a'.repeat(200_000)}/done`, + ), + ).toBe(false); + // The regex this replaced measured seconds at ~1000 separators and + // scaled ~cubically; a linear scan finishes in milliseconds even on + // 200 KB entries. The generous bound only guards against regressions. + expect(Date.now() - start).toBeLessThan(2000); + }); +}); diff --git a/packages/core/src/hooks/urlValidator.ts b/packages/core/src/hooks/urlValidator.ts index 822a230a0d9..624719ca331 100644 --- a/packages/core/src/hooks/urlValidator.ts +++ b/packages/core/src/hooks/urlValidator.ts @@ -185,3 +185,84 @@ export function createUrlValidator( ): UrlValidator { return new UrlValidator(allowedUrls || [], allowPrivateNetworkHosts); } + +/** + * Returns true when every URL matched by the `inner` hook URL pattern is + * also matched by the `outer` pattern (both support `*` wildcards, like + * `security.allowedHttpHookUrls` entries). Used to intersect a + * workspace-scope whitelist with the higher-scope one: a workspace entry + * may only survive the merge when it merely narrows what a higher scope + * already allows. + * + * Both patterns are read as literal text plus `*` wildcards — the language + * `compilePattern` assigns to a pattern carrying no regex content beyond + * the `\.` escape, which is normalized first so both spellings of the same + * pattern cover each other. Any other escape or regex metacharacter makes + * coverage unprovable (the pre-escaped `compilePattern` branch would read + * it as raw regex — including a bare `.` that survives the unescaping, + * which acts as a wildcard), so such patterns fail closed (return false). + * Non-ASCII patterns fail closed too: the runtime's non-Unicode `/i` case + * folding diverges from the `toLowerCase()` used here for some of them, so + * coverage is unprovable (hook URLs are realistically ASCII — punycode and + * percent-encoded forms are unaffected). + * + * The comparison is a linear chunk scan — split `outer` on `*` and require + * the chunks in `inner` in order, anchored at both ends — never a regex + * test, because this runs on every startup merge and must stay O(n+m) on + * arbitrary-length workspace input (no catastrophic backtracking). + */ +export function hookUrlPatternCovers( + outerPattern: string, + innerPattern: string, +): boolean { + const unescape = (pattern: string) => pattern.replace(/\\\./g, '.'); + // The runtime matches with `compilePattern`'s non-Unicode `/i` RegExp, + // whose case folding is a different equivalence relation from + // toLowerCase() for some non-ASCII characters (e.g. ẞ U+1E9E lowers + // to ß U+00DF, so the two hosts fold equal here, but the runtime regex + // never matches them across). A covers verdict built on toLowerCase() + // would let such an inner entry survive the merge while its runtime + // regex admits hosts the outer pattern rejects, so fail closed on any + // non-ASCII input. + const nonAscii = /[\u0080-\uFFFF]/; + if (nonAscii.test(outerPattern) || nonAscii.test(innerPattern)) { + return false; + } + const outer = unescape(outerPattern).toLowerCase(); + const inner = unescape(innerPattern).toLowerCase(); + // `compilePattern` treats everything but `*` as raw regex once a pattern + // contains `\.`, so any remaining regex-active character could widen the + // runtime language past the literal reading used here. A bare `.` is + // regex-active in that branch too, but only after unescaping: the `\.` + // sequences it came from are literal dots, so strip them before checking. + const regexActive = /[+?^${}()|[\]\\]/; + const bareDotAfterUnescape = (pattern: string) => + pattern.includes('\\.') && pattern.replace(/\\\./g, '').includes('.'); + if ( + regexActive.test(outer) || + regexActive.test(inner) || + bareDotAfterUnescape(outerPattern) || + bareDotAfterUnescape(innerPattern) + ) { + return false; + } + const chunks = outer.split('*'); + if (chunks.length === 1) { + return inner === outer; + } + const first = chunks[0]; + const last = chunks[chunks.length - 1]; + if (!inner.startsWith(first) || !inner.endsWith(last)) { + return false; + } + let position = first.length; + const end = inner.length - last.length; + for (const chunk of chunks.slice(1, -1)) { + const found = inner.indexOf(chunk, position); + if (found === -1 || found + chunk.length > end) { + return false; + } + position = found + chunk.length; + } + return position <= end; +} diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index f40a7d6556a..61317a3ebe3 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -143,6 +143,7 @@ export * from './tools/modifiable-tool.js'; export { buildSkillLlmContent, applySkillAllowedTools, + isTrustedSkillLevel, } from './tools/skill-utils.js'; export { atomicWriteFile } from './utils/atomicFileWrite.js'; export { nextFireTime, parseCron } from './utils/cronParser.js'; @@ -660,6 +661,7 @@ export { HookRegistry, createInstructionsLoadedCallback, hookEventSupportsMatcher, + hookUrlPatternCovers, } from './hooks/index.js'; export type { HookRegistryEntry, SessionHookEntry } from './hooks/index.js'; export { diff --git a/packages/core/src/skills/skill-manager.test.ts b/packages/core/src/skills/skill-manager.test.ts index d267cafe32d..f94af1c858a 100644 --- a/packages/core/src/skills/skill-manager.test.ts +++ b/packages/core/src/skills/skill-manager.test.ts @@ -780,6 +780,27 @@ Skill 3 content`); expect(projectSkills.every((s) => s.level === 'project')).toBe(true); }); + it('tags user-level skills with homeRootShadow when the project root is the home directory', async () => { + // Listing skips the 'project' level there, so repository-committed + // skills surface at 'user' level; the side-effect trust gates + // consume this flag instead of re-reading the (rebindable) project + // root. + vi.spyOn(mockConfig, 'getProjectRoot').mockReturnValue(TEST_HOME); + + const skills = await manager.listSkills(); + + expect(skills.map((s) => s.name).sort()).toEqual(['skill1', 'skill3']); + expect(skills.every((s) => s.level === 'user')).toBe(true); + expect(skills.every((s) => s.homeRootShadow === true)).toBe(true); + }); + + it('does not tag skills with homeRootShadow in a normal project', async () => { + const skills = await manager.listSkills(); + + expect(skills).toHaveLength(3); // skill1, skill2 (project), skill3 (user) + expect(skills.every((s) => s.homeRootShadow === undefined)).toBe(true); + }); + it('should return a stable alphabetical order regardless of priority (priority only affects the /skills display layer)', async () => { vi.mocked(fs.readdir).mockReset(); mockParseYaml.mockImplementation((yamlString: string) => diff --git a/packages/core/src/skills/skill-manager.ts b/packages/core/src/skills/skill-manager.ts index f44ffc554e1..71ba00d56c3 100644 --- a/packages/core/src/skills/skill-manager.ts +++ b/packages/core/src/skills/skill-manager.ts @@ -1059,6 +1059,17 @@ export class SkillManager { skills.push(skill); } } + // Home-root shadow: when the project root IS the home directory the + // 'project' level was skipped above, so repository-committed skills + // surface at this 'user' level (~/.qwen and /.qwen are the same + // directory). Tag them so the side-effect trust gates recognise + // repo-supplied skills even where a per-agent Config override rebinds + // getProjectRoot() (worktree isolation, working_dir pins). + if (level === 'user' && isHomeDirectory) { + for (const skill of skills) { + skill.homeRootShadow = true; + } + } debugLogger.debug(`Loaded ${skills.length} ${level} level skills`); return skills; } diff --git a/packages/core/src/skills/types.ts b/packages/core/src/skills/types.ts index b8b8b614be5..a22f9b38790 100644 --- a/packages/core/src/skills/types.ts +++ b/packages/core/src/skills/types.ts @@ -69,6 +69,17 @@ export interface SkillConfig { */ level: SkillLevel; + /** + * Set by SkillManager at collection time on 'user'-level skills when the + * project root IS the home directory: listing skips the 'project' level + * there, so repository-committed skills surface at 'user' level. Trust + * gates must treat such skills as repo-supplied; the flag travels with the + * skill so the decision stays correct even where a per-agent Config + * override rebinds `getProjectRoot()` (worktree isolation, working_dir + * pins) and would flip a path-equality recomputation. + */ + homeRootShadow?: boolean; + /** * Absolute path to the skill directory containing SKILL.md */ diff --git a/packages/core/src/subagents/subagent-manager.test.ts b/packages/core/src/subagents/subagent-manager.test.ts index f79a82cd604..b6236f35ec3 100644 --- a/packages/core/src/subagents/subagent-manager.test.ts +++ b/packages/core/src/subagents/subagent-manager.test.ts @@ -1559,6 +1559,40 @@ You are a helpful assistant.`; path.normalize('/test/project/.qwen/agents/misnamed-file.md'), ); }); + + it('tags user-level agents with homeRootShadow when the project root is the home directory', async () => { + // Listing skips the 'project' level there, so repository-committed + // agent files surface at 'user' level; the spawn-time hooks gate + // consumes this flag instead of re-reading the (rebindable) + // project root. + vi.spyOn(mockConfig, 'getProjectRoot').mockReturnValue('/home/user'); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + vi.mocked(fs.readdir).mockResolvedValue(['test-agent.md'] as any); + vi.mocked(fs.readFile).mockResolvedValue(validMarkdown); + + const config = await manager.loadSubagent('test-agent'); + + expect(config).toBeDefined(); + expect(config!.level).toBe('user'); + expect(config!.filePath).toBe( + path.normalize('/home/user/.qwen/agents/test-agent.md'), + ); + expect(config!.homeRootShadow).toBe(true); + }); + + it('does not tag user-level agents with homeRootShadow in a normal project', async () => { + vi.mocked(fs.readdir) + .mockRejectedValueOnce(new Error('Project dir not found')) + // eslint-disable-next-line @typescript-eslint/no-explicit-any + .mockResolvedValueOnce(['test-agent.md'] as any); + vi.mocked(fs.readFile).mockResolvedValue(validMarkdown); + + const config = await manager.loadSubagent('test-agent'); + + expect(config).toBeDefined(); + expect(config!.level).toBe('user'); + expect(config!.homeRootShadow).toBeUndefined(); + }); }); describe('updateSubagent', () => { @@ -2572,6 +2606,311 @@ bad`); expect(unregisterSpy).toHaveBeenCalledTimes(1); }); + it('does not register hooks for a project-level subagent in an untrusted folder', async () => { + const addAgentHooksSpy = vi.fn().mockReturnValue(vi.fn()); + vi.spyOn(mockConfig, 'getHookSystem').mockReturnValue({ + getRegistry: () => ({ addAgentHooks: addAgentHooksSpy }), + } as unknown as ReturnType); + vi.spyOn(mockConfig, 'isTrustedFolder').mockReturnValue(false); + + const result = await manager.createAgentHeadless( + { + ...baseConfig, + level: 'project', + hooks: { + PreToolUse: [ + { + matcher: 'Bash', + hooks: [{ type: 'command', command: 'echo' }], + }, + ], + }, + }, + mockConfig, + ); + + expect(addAgentHooksSpy).not.toHaveBeenCalled(); + // The agent itself is still created — only the hooks are gated. + expect(result).toHaveProperty('subagent'); + await result.dispose(); + }); + + it('registers hooks for a project-level subagent in a trusted folder', async () => { + const addAgentHooksSpy = vi.fn().mockReturnValue(vi.fn()); + vi.spyOn(mockConfig, 'getHookSystem').mockReturnValue({ + getRegistry: () => ({ addAgentHooks: addAgentHooksSpy }), + } as unknown as ReturnType); + vi.spyOn(mockConfig, 'isTrustedFolder').mockReturnValue(true); + + const result = await manager.createAgentHeadless( + { + ...baseConfig, + level: 'project', + hooks: { + PreToolUse: [ + { + matcher: 'Bash', + hooks: [{ type: 'command', command: 'echo' }], + }, + ], + }, + }, + mockConfig, + ); + + expect(addAgentHooksSpy).toHaveBeenCalledTimes(1); + await result.dispose(); + }); + + it('registers hooks for a user-level subagent regardless of folder trust', async () => { + const addAgentHooksSpy = vi.fn().mockReturnValue(vi.fn()); + vi.spyOn(mockConfig, 'getHookSystem').mockReturnValue({ + getRegistry: () => ({ addAgentHooks: addAgentHooksSpy }), + } as unknown as ReturnType); + vi.spyOn(mockConfig, 'isTrustedFolder').mockReturnValue(false); + + const result = await manager.createAgentHeadless( + { + ...baseConfig, + level: 'user', + hooks: { + PreToolUse: [ + { + matcher: 'Bash', + hooks: [{ type: 'command', command: 'echo' }], + }, + ], + }, + }, + mockConfig, + ); + + expect(addAgentHooksSpy).toHaveBeenCalledTimes(1); + await result.dispose(); + }); + + it('does not register hooks for a home-root-shadowed user-level subagent in an untrusted folder', async () => { + // Listing skips the 'project' level when the project root IS the + // home directory, so repository-committed agent files surface at + // 'user' level there and are tagged `homeRootShadow`; the gate + // must require folder trust too. + const addAgentHooksSpy = vi.fn().mockReturnValue(vi.fn()); + vi.spyOn(mockConfig, 'getHookSystem').mockReturnValue({ + getRegistry: () => ({ addAgentHooks: addAgentHooksSpy }), + } as unknown as ReturnType); + vi.spyOn(mockConfig, 'isTrustedFolder').mockReturnValue(false); + + const result = await manager.createAgentHeadless( + { + ...baseConfig, + level: 'user', + homeRootShadow: true, + hooks: { + PreToolUse: [ + { + matcher: 'Bash', + hooks: [{ type: 'command', command: 'echo' }], + }, + ], + }, + }, + mockConfig, + ); + + expect(addAgentHooksSpy).not.toHaveBeenCalled(); + // The agent itself is still created — only the hooks are gated. + expect(result).toHaveProperty('subagent'); + await result.dispose(); + }); + + it('registers hooks for a home-root-shadowed user-level subagent when the folder is trusted', async () => { + const addAgentHooksSpy = vi.fn().mockReturnValue(vi.fn()); + vi.spyOn(mockConfig, 'getHookSystem').mockReturnValue({ + getRegistry: () => ({ addAgentHooks: addAgentHooksSpy }), + } as unknown as ReturnType); + vi.spyOn(mockConfig, 'isTrustedFolder').mockReturnValue(true); + + const result = await manager.createAgentHeadless( + { + ...baseConfig, + level: 'user', + homeRootShadow: true, + hooks: { + PreToolUse: [ + { + matcher: 'Bash', + hooks: [{ type: 'command', command: 'echo' }], + }, + ], + }, + }, + mockConfig, + ); + + expect(addAgentHooksSpy).toHaveBeenCalledTimes(1); + await result.dispose(); + }); + + it('keeps hooks gated when the spawn context rebinds getProjectRoot (worktree isolation)', async () => { + // Worktree-isolation / working_dir spawns pass a per-agent Config + // override whose getProjectRoot is rebound to the worktree path + // (agent.ts, InProcessBackend, workflow-orchestrator). The gate + // must consume the listing-time `homeRootShadow` flag instead of + // re-deriving the shadow from that rebound value — otherwise a + // home-rooted repo agent surfaced at 'user' level, spawned with + // isolation: 'worktree' in an untrusted folder, would register + // its hooks session-wide. + const addAgentHooksSpy = vi.fn().mockReturnValue(vi.fn()); + vi.spyOn(mockConfig, 'getHookSystem').mockReturnValue({ + getRegistry: () => ({ addAgentHooks: addAgentHooksSpy }), + } as unknown as ReturnType); + vi.spyOn(mockConfig, 'isTrustedFolder').mockReturnValue(false); + + const reboundContext = Object.create(mockConfig) as Config; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (reboundContext as any).getProjectRoot = () => + '/test/project/.qwen/worktrees/agent-x'; + + const result = await manager.createAgentHeadless( + { + ...baseConfig, + level: 'user', + homeRootShadow: true, + hooks: { + PreToolUse: [ + { + matcher: 'Bash', + hooks: [{ type: 'command', command: 'echo' }], + }, + ], + }, + }, + reboundContext, + ); + + expect(addAgentHooksSpy).not.toHaveBeenCalled(); + expect(result).toHaveProperty('subagent'); + await result.dispose(); + }); + + it('registers hooks for an extension-level subagent regardless of folder trust', async () => { + const addAgentHooksSpy = vi.fn().mockReturnValue(vi.fn()); + vi.spyOn(mockConfig, 'getHookSystem').mockReturnValue({ + getRegistry: () => ({ addAgentHooks: addAgentHooksSpy }), + } as unknown as ReturnType); + vi.spyOn(mockConfig, 'isTrustedFolder').mockReturnValue(false); + + const result = await manager.createAgentHeadless( + { + ...baseConfig, + level: 'extension', + hooks: { + PreToolUse: [ + { + matcher: 'Bash', + hooks: [{ type: 'command', command: 'echo' }], + }, + ], + }, + }, + mockConfig, + ); + + expect(addAgentHooksSpy).toHaveBeenCalledTimes(1); + await result.dispose(); + }); + + it('registers hooks for a builtin-level subagent regardless of folder trust', async () => { + // Pins the 'builtin' arm of the allowlist (spelled 'bundled' in the + // sibling skill helper — a future edit dropping the clause must fail + // a test, not silently gate builtin agents' hooks on folder trust). + const addAgentHooksSpy = vi.fn().mockReturnValue(vi.fn()); + vi.spyOn(mockConfig, 'getHookSystem').mockReturnValue({ + getRegistry: () => ({ addAgentHooks: addAgentHooksSpy }), + } as unknown as ReturnType); + vi.spyOn(mockConfig, 'isTrustedFolder').mockReturnValue(false); + + const result = await manager.createAgentHeadless( + { + ...baseConfig, + level: 'builtin', + hooks: { + PreToolUse: [ + { + matcher: 'Bash', + hooks: [{ type: 'command', command: 'echo' }], + }, + ], + }, + }, + mockConfig, + ); + + expect(addAgentHooksSpy).toHaveBeenCalledTimes(1); + await result.dispose(); + }); + + it('registers hooks for a session-level (host-supplied) subagent regardless of folder trust', async () => { + // Session agents arrive via the embedding host (SDK `agents` option + // or the control protocol), never from repository files, so the + // allowlist treats them like user-level agents. + const addAgentHooksSpy = vi.fn().mockReturnValue(vi.fn()); + vi.spyOn(mockConfig, 'getHookSystem').mockReturnValue({ + getRegistry: () => ({ addAgentHooks: addAgentHooksSpy }), + } as unknown as ReturnType); + vi.spyOn(mockConfig, 'isTrustedFolder').mockReturnValue(false); + + const result = await manager.createAgentHeadless( + { + ...baseConfig, + level: 'session', + hooks: { + PreToolUse: [ + { + matcher: 'Bash', + hooks: [{ type: 'command', command: 'echo' }], + }, + ], + }, + }, + mockConfig, + ); + + expect(addAgentHooksSpy).toHaveBeenCalledTimes(1); + expect(result).toHaveProperty('subagent'); + await result.dispose(); + }); + + it('fails closed for levels outside the allowlist (unknown or future levels)', async () => { + const addAgentHooksSpy = vi.fn().mockReturnValue(vi.fn()); + vi.spyOn(mockConfig, 'getHookSystem').mockReturnValue({ + getRegistry: () => ({ addAgentHooks: addAgentHooksSpy }), + } as unknown as ReturnType); + vi.spyOn(mockConfig, 'isTrustedFolder').mockReturnValue(false); + + const result = await manager.createAgentHeadless( + { + ...baseConfig, + // Any level not in the trusted allowlist requires folder + // trust — pin the fail-closed direction for future values. + level: 'future-level' as unknown as SubagentConfig['level'], + hooks: { + PreToolUse: [ + { + matcher: 'Bash', + hooks: [{ type: 'command', command: 'echo' }], + }, + ], + }, + }, + mockConfig, + ); + + expect(addAgentHooksSpy).not.toHaveBeenCalled(); + expect(result).toHaveProperty('subagent'); + await result.dispose(); + }); + it('dispose unregisters even when execute() never runs (early-exit leak fix)', async () => { // Caller pattern: // const { subagent, dispose } = await createAgentHeadless(...); diff --git a/packages/core/src/subagents/subagent-manager.ts b/packages/core/src/subagents/subagent-manager.ts index 810b00895da..73b7c0ed4c9 100644 --- a/packages/core/src/subagents/subagent-manager.ts +++ b/packages/core/src/subagents/subagent-manager.ts @@ -924,17 +924,39 @@ export class SubagentManager { const hookSystem = runtimeContext.getHookSystem(); const hookRegistry = hookSystem?.getRegistry(); if (config.hooks && Object.keys(config.hooks).length > 0) { - if (hookRegistry) { + // Fail closed: only levels that cannot originate from the + // repository skip the folder-trust gate. 'project' agents load + // from /.qwen/agents/ regardless of trust (read-only use is + // fine), but their hooks are repo-supplied code execution — the + // same gate Config.getProjectHooks() applies to settings-file + // hooks — and any future or unset level requires trust too. + // 'session' agents are supplied by the embedding host (SDK + // `agents` option or the control protocol), never loaded from + // repository files, so they sit on the same footing as 'user' — + // except when the project root IS the home directory: listing + // skips 'project' there, so repository-committed agent files + // surface at 'user' level and require trust too. Listing records + // that case as `homeRootShadow`; recomputing it here from + // `runtimeContext.getProjectRoot()` would read the per-agent + // override that worktree isolation / working_dir provisioning + // rebinds to the worktree path, opening the gate for exactly the + // repo-supplied agents the shadow surfaced. + const trustedAgentLevel = + (config.level === 'user' && config.homeRootShadow !== true) || + config.level === 'builtin' || + config.level === 'extension' || + config.level === 'session'; + if (!trustedAgentLevel && !runtimeContext.isTrustedFolder()) { + debugLogger.warn( + `Subagent "${config.name}" declares hooks but the folder is not trusted; ignoring per-agent hooks.`, + ); + } else if (hookRegistry) { const agentScope = `agent:${config.name}:${randomUUID()}`; unregisterAgentHooks = hookRegistry.addAgentHooks( config.hooks as { [K in HookEventName]?: HookDefinition[] }, agentScope, ); } else { - // Single outer guard; nested branch on hookRegistry. The pre-fix - // structure repeated the `config.hooks && Object.keys(...).length` - // predicate across two `if`/`else if` arms, which made it easy to - // drift one side during future edits. debugLogger.warn( `Subagent "${config.name}" declares hooks but the host has no HookSystem; ignoring per-agent hooks.`, ); @@ -1371,6 +1393,18 @@ export class SubagentManager { } } + // Home-root shadow: when the project root IS the home directory the + // 'project' level was skipped above, so repository-committed agent + // files surface at this 'user' level. Tag them so the hooks trust + // gate at spawn time stays correct even when the spawn path rebinds + // getProjectRoot() on a per-agent override (worktree isolation, + // working_dir pins). Mirrors SkillManager.listSkillsAtLevel. + if (level === 'user' && isHomeDirectory) { + for (const subagent of subagents) { + subagent.homeRootShadow = true; + } + } + return subagents; } catch (_error) { // Directory doesn't exist or can't be read diff --git a/packages/core/src/subagents/types.ts b/packages/core/src/subagents/types.ts index eeb11472a10..7fac6c5fab0 100644 --- a/packages/core/src/subagents/types.ts +++ b/packages/core/src/subagents/types.ts @@ -94,6 +94,18 @@ export interface SubagentConfig { /** Storage level - determines where the configuration file is stored */ level: SubagentLevel; + /** + * Set by SubagentManager at collection time on 'user'-level agents when + * the project root IS the home directory: listing skips the 'project' + * level there, so repository-committed agent files surface at 'user' + * level. The hooks trust gate must treat such agents as repo-supplied; + * the flag travels with the agent so the decision stays correct even + * where a per-agent Config override rebinds `getProjectRoot()` (worktree + * isolation, working_dir pins) and would flip a path-equality + * recomputation. Not serialized to frontmatter. + */ + homeRootShadow?: boolean; + /** Absolute path to the configuration file. Optional for session subagents. */ filePath?: string; diff --git a/packages/core/src/tools/skill-utils.test.ts b/packages/core/src/tools/skill-utils.test.ts index 82d29cfc726..68cb2ff4f37 100644 --- a/packages/core/src/tools/skill-utils.test.ts +++ b/packages/core/src/tools/skill-utils.test.ts @@ -7,11 +7,13 @@ import { describe, it, expect, vi, afterEach } from 'vitest'; import { applySkillAllowedTools, + isTrustedSkillLevel, collectAvailableSkillEntries, clearCollectedSkillEntriesCache, } from './skill-utils.js'; import type { PermissionManager } from '../permissions/permission-manager.js'; import type { SkillManager } from '../skills/skill-manager.js'; +import type { SkillLevel } from '../skills/types.js'; import type { Config } from '../config/config.js'; function mockPermissionManager(): { @@ -68,6 +70,22 @@ describe('applySkillAllowedTools', () => { }); }); +describe('isTrustedSkillLevel', () => { + it('accepts the levels that never originate from the repository', () => { + expect(isTrustedSkillLevel('user')).toBe(true); + expect(isTrustedSkillLevel('bundled')).toBe(true); + expect(isTrustedSkillLevel('extension')).toBe(true); + }); + + it('requires folder trust for repo-controlled and unknown levels', () => { + expect(isTrustedSkillLevel('project')).toBe(false); + // Fail closed: a missing or future level must never skip the gate. + expect(isTrustedSkillLevel(undefined)).toBe(false); + const futureLevel = 'session' as unknown as SkillLevel; + expect(isTrustedSkillLevel(futureLevel)).toBe(false); + }); +}); + describe('collectAvailableSkillEntries memoize cache', () => { function mockSkillManager(): SkillManager { return { diff --git a/packages/core/src/tools/skill-utils.ts b/packages/core/src/tools/skill-utils.ts index df3caee31d1..9e23897783f 100644 --- a/packages/core/src/tools/skill-utils.ts +++ b/packages/core/src/tools/skill-utils.ts @@ -255,6 +255,21 @@ ${escapeXml(entry.description)} .join('\n'); } +/** + * Skill levels that do not originate from the repository: 'user' lives in + * ~/.qwen, 'bundled' ships with the product, and extensions load only + * from the user-scope extensions directory. One topology breaks the + * 'user' premise: when the project root IS the home directory, listing + * skips the 'project' level and repository-committed skills surface at + * 'user' — callers re-gate that combination on folder trust. Side effects + * of skills at any other (or missing) level — allowedTools grants, hooks + * — are repo-controllable, so callers gate them on folder trust and this + * check fails closed. + */ +export function isTrustedSkillLevel(level: SkillLevel | undefined): boolean { + return level === 'user' || level === 'bundled' || level === 'extension'; +} + /** * Grants a skill's `allowedTools` as session-scoped permission allow rules. * diff --git a/packages/core/src/tools/skill.test.ts b/packages/core/src/tools/skill.test.ts index 420d9326bb5..ea51935360a 100644 --- a/packages/core/src/tools/skill.test.ts +++ b/packages/core/src/tools/skill.test.ts @@ -20,6 +20,7 @@ import { renderAvailableSkillsBlock, } from './skill-utils.js'; import { recordAutoSkillUsage } from '../skills/skill-curator.js'; +import { registerSkillHooks } from '../hooks/registerSkillHooks.js'; // Type for accessing protected methods in tests type SkillToolWithProtectedMethods = SkillTool & { @@ -38,6 +39,9 @@ type SkillToolWithProtectedMethods = SkillTool & { // Mock dependencies vi.mock('../skills/skill-manager.js'); +vi.mock('../hooks/registerSkillHooks.js', () => ({ + registerSkillHooks: vi.fn().mockReturnValue(1), +})); vi.mock('../skills/skill-curator.js', () => ({ recordAutoSkillUsage: vi.fn().mockResolvedValue(false), })); @@ -95,6 +99,8 @@ describe('SkillTool', () => { getProjectRoot: vi.fn().mockReturnValue('/test/project'), getAutoSkillEnabled: vi.fn().mockReturnValue(true), getSessionId: vi.fn().mockReturnValue('test-session-id'), + isTrustedFolder: vi.fn().mockReturnValue(true), + getHookSystem: vi.fn().mockReturnValue(undefined), getSkillManager: vi.fn(), getGeminiClient: vi.fn().mockReturnValue(undefined), getModelInvocableCommandsProvider: vi.fn().mockReturnValue(null), @@ -523,6 +529,452 @@ describe('SkillTool', () => { }); }); + describe('skill hooks trust gating', () => { + const hookedSkill: SkillConfig = { + name: 'hooked', + description: 'Skill with hooks', + level: 'project', + filePath: '/project/.qwen/skills/hooked/SKILL.md', + body: 'Body.', + hooks: { + PreToolUse: [ + { + matcher: 'Bash', + hooks: [{ type: 'command', command: './check.sh' }], + }, + ], + } as unknown as SkillConfig['hooks'], + }; + + beforeEach(() => { + vi.mocked(mockSkillManager.loadSkillForRuntime).mockResolvedValue( + hookedSkill, + ); + vi.mocked(registerSkillHooks).mockClear(); + }); + + it('does not register hooks for a project-level skill in an untrusted folder', async () => { + vi.mocked(config.isTrustedFolder).mockReturnValue(false); + const getSessionHooksManager = vi.fn(); + vi.mocked(config.getHookSystem).mockReturnValue({ + getSessionHooksManager, + } as unknown as ReturnType); + + const invocation = ( + skillTool as SkillToolWithProtectedMethods + ).createInvocation({ skill: 'hooked' }); + const result = await invocation.execute(); + + expect(registerSkillHooks).not.toHaveBeenCalled(); + // The skill itself still loads — only the hooks are gated. + expect(partToString(result.llmContent)).toContain('Body.'); + }); + + it('registers hooks for a project-level skill in a trusted folder', async () => { + vi.mocked(config.isTrustedFolder).mockReturnValue(true); + const getSessionHooksManager = vi.fn().mockReturnValue({}); + vi.mocked(config.getHookSystem).mockReturnValue({ + getSessionHooksManager, + } as unknown as ReturnType); + + const invocation = ( + skillTool as SkillToolWithProtectedMethods + ).createInvocation({ skill: 'hooked' }); + await invocation.execute(); + + expect(registerSkillHooks).toHaveBeenCalledTimes(1); + }); + + it('registers hooks for a user-level skill regardless of folder trust', async () => { + vi.mocked(config.isTrustedFolder).mockReturnValue(false); + vi.mocked(mockSkillManager.loadSkillForRuntime).mockResolvedValue({ + ...hookedSkill, + level: 'user', + }); + const getSessionHooksManager = vi.fn().mockReturnValue({}); + vi.mocked(config.getHookSystem).mockReturnValue({ + getSessionHooksManager, + } as unknown as ReturnType); + + const invocation = ( + skillTool as SkillToolWithProtectedMethods + ).createInvocation({ skill: 'hooked' }); + await invocation.execute(); + + expect(registerSkillHooks).toHaveBeenCalledTimes(1); + }); + + it('gates hooks for a home-root-shadowed user-level skill in an untrusted folder', async () => { + // SkillManager skips the 'project' level when the project root IS + // the home directory, so repository-committed skills surface at + // 'user' level there and are tagged `homeRootShadow`; the gate + // must treat them as repo-supplied. + vi.mocked(config.isTrustedFolder).mockReturnValue(false); + vi.mocked(mockSkillManager.loadSkillForRuntime).mockResolvedValue({ + ...hookedSkill, + level: 'user', + homeRootShadow: true, + }); + const getSessionHooksManager = vi.fn(); + vi.mocked(config.getHookSystem).mockReturnValue({ + getSessionHooksManager, + } as unknown as ReturnType); + + const invocation = ( + skillTool as SkillToolWithProtectedMethods + ).createInvocation({ skill: 'hooked' }); + const result = await invocation.execute(); + + expect(registerSkillHooks).not.toHaveBeenCalled(); + // The skill itself still loads — only the hooks are gated. + expect(partToString(result.llmContent)).toContain('Body.'); + }); + + it('registers hooks for a home-root-shadowed user-level skill once the folder is trusted', async () => { + vi.mocked(config.isTrustedFolder).mockReturnValue(true); + vi.mocked(mockSkillManager.loadSkillForRuntime).mockResolvedValue({ + ...hookedSkill, + level: 'user', + homeRootShadow: true, + }); + const getSessionHooksManager = vi.fn().mockReturnValue({}); + vi.mocked(config.getHookSystem).mockReturnValue({ + getSessionHooksManager, + } as unknown as ReturnType); + + const invocation = ( + skillTool as SkillToolWithProtectedMethods + ).createInvocation({ skill: 'hooked' }); + await invocation.execute(); + + expect(registerSkillHooks).toHaveBeenCalledTimes(1); + }); + + it('keeps hooks gated when the subagent config rebinds getProjectRoot (worktree isolation)', async () => { + // Inside a worktree-isolated / working_dir-pinned subagent the + // SkillTool runs on a per-agent Config override whose + // getProjectRoot is rebound to the worktree path. The gate must + // consume the listing-time `homeRootShadow` flag instead of + // re-deriving the shadow from that rebound value — otherwise a + // home-rooted repo skill surfaced at 'user' level would register + // its hooks on the parent session despite the untrusted folder. + vi.mocked(config.getProjectRoot).mockReturnValue( + '/test/project/.qwen/worktrees/agent-x', + ); + vi.mocked(config.isTrustedFolder).mockReturnValue(false); + vi.mocked(mockSkillManager.loadSkillForRuntime).mockResolvedValue({ + ...hookedSkill, + level: 'user', + homeRootShadow: true, + }); + const getSessionHooksManager = vi.fn(); + vi.mocked(config.getHookSystem).mockReturnValue({ + getSessionHooksManager, + } as unknown as ReturnType); + + const invocation = ( + skillTool as SkillToolWithProtectedMethods + ).createInvocation({ skill: 'hooked' }); + await invocation.execute(); + + expect(registerSkillHooks).not.toHaveBeenCalled(); + }); + + it('registers hooks for an extension-level skill regardless of folder trust', async () => { + // Extensions load only from the user-scope extensions directory, so + // they are in the trusted allowlist and must not regress behind the + // fail-closed gate. + vi.mocked(config.isTrustedFolder).mockReturnValue(false); + vi.mocked(mockSkillManager.loadSkillForRuntime).mockResolvedValue({ + ...hookedSkill, + level: 'extension', + }); + const getSessionHooksManager = vi.fn().mockReturnValue({}); + vi.mocked(config.getHookSystem).mockReturnValue({ + getSessionHooksManager, + } as unknown as ReturnType); + + const invocation = ( + skillTool as SkillToolWithProtectedMethods + ).createInvocation({ skill: 'hooked' }); + await invocation.execute(); + + expect(registerSkillHooks).toHaveBeenCalledTimes(1); + }); + + it('registers deferred hooks once the folder becomes trusted mid-session', async () => { + // The gate is "gated UNTIL the folder is trusted": a skill first + // invoked while untrusted must pick up its hooks on re-invocation + // after trust is granted, without waiting for /clear. + vi.mocked(config.isTrustedFolder).mockReturnValue(false); + const getSessionHooksManager = vi.fn().mockReturnValue({}); + vi.mocked(config.getHookSystem).mockReturnValue({ + getSessionHooksManager, + } as unknown as ReturnType); + + const first = ( + skillTool as SkillToolWithProtectedMethods + ).createInvocation({ skill: 'hooked' }); + await first.execute(); + expect(registerSkillHooks).not.toHaveBeenCalled(); + + vi.mocked(config.isTrustedFolder).mockReturnValue(true); + + const second = ( + skillTool as SkillToolWithProtectedMethods + ).createInvocation({ skill: 'hooked' }); + const result = await second.execute(); + + expect(registerSkillHooks).toHaveBeenCalledTimes(1); + // The body is not re-injected — only the side effects are applied. + expect(partToString(result.llmContent)).toContain('already loaded'); + + // Later invocations must not register the hooks a second time. + const third = ( + skillTool as SkillToolWithProtectedMethods + ).createInvocation({ skill: 'hooked' }); + await third.execute(); + expect(registerSkillHooks).toHaveBeenCalledTimes(1); + }); + + it('registers hooks exactly once when a trusted skill is re-invoked', async () => { + // A trust-passing first load applies side effects immediately and must + // not enter the deferred set — otherwise re-invocation would hit the + // re-apply branch and register the hooks a second time (no dedup). + vi.mocked(config.isTrustedFolder).mockReturnValue(true); + const getSessionHooksManager = vi.fn().mockReturnValue({}); + vi.mocked(config.getHookSystem).mockReturnValue({ + getSessionHooksManager, + } as unknown as ReturnType); + + const first = ( + skillTool as SkillToolWithProtectedMethods + ).createInvocation({ skill: 'hooked' }); + await first.execute(); + expect(registerSkillHooks).toHaveBeenCalledTimes(1); + + const second = ( + skillTool as SkillToolWithProtectedMethods + ).createInvocation({ skill: 'hooked' }); + const result = await second.execute(); + expect(partToString(result.llmContent)).toContain('already loaded'); + expect(registerSkillHooks).toHaveBeenCalledTimes(1); + }); + + it('keeps the deferral alive when re-invoked while still untrusted', async () => { + // A re-invocation before trust is granted must retain the deferred + // entry; dropping it would lose the hooks for the rest of the session + // even after the folder becomes trusted. + vi.mocked(config.isTrustedFolder).mockReturnValue(false); + const getSessionHooksManager = vi.fn().mockReturnValue({}); + vi.mocked(config.getHookSystem).mockReturnValue({ + getSessionHooksManager, + } as unknown as ReturnType); + + const first = ( + skillTool as SkillToolWithProtectedMethods + ).createInvocation({ skill: 'hooked' }); + await first.execute(); + expect(registerSkillHooks).not.toHaveBeenCalled(); + + const second = ( + skillTool as SkillToolWithProtectedMethods + ).createInvocation({ skill: 'hooked' }); + const result = await second.execute(); + expect(partToString(result.llmContent)).toContain('already loaded'); + expect(registerSkillHooks).not.toHaveBeenCalled(); + + vi.mocked(config.isTrustedFolder).mockReturnValue(true); + + const third = ( + skillTool as SkillToolWithProtectedMethods + ).createInvocation({ skill: 'hooked' }); + await third.execute(); + expect(registerSkillHooks).toHaveBeenCalledTimes(1); + }); + + it('clears deferred side effects on clearLoadedSkills so a fresh load registers hooks once', async () => { + // /clear resets both tracking sets. A stale deferred entry would make + // the next re-invocation re-apply side effects on top of the fresh + // load's registration, and registerSkillHooks has no dedup. + vi.mocked(config.isTrustedFolder).mockReturnValue(false); + const getSessionHooksManager = vi.fn().mockReturnValue({}); + vi.mocked(config.getHookSystem).mockReturnValue({ + getSessionHooksManager, + } as unknown as ReturnType); + + const first = ( + skillTool as SkillToolWithProtectedMethods + ).createInvocation({ skill: 'hooked' }); + await first.execute(); + expect(registerSkillHooks).not.toHaveBeenCalled(); + + vi.mocked(config.isTrustedFolder).mockReturnValue(true); + skillTool.clearLoadedSkills(); + + const second = ( + skillTool as SkillToolWithProtectedMethods + ).createInvocation({ skill: 'hooked' }); + const result2 = await second.execute(); + // After /clear the skill loads fresh (full body), not via the dedup path. + expect(partToString(result2.llmContent)).toContain('Body.'); + expect(registerSkillHooks).toHaveBeenCalledTimes(1); + + const third = ( + skillTool as SkillToolWithProtectedMethods + ).createInvocation({ skill: 'hooked' }); + const result3 = await third.execute(); + expect(partToString(result3.llmContent)).toContain('already loaded'); + expect(registerSkillHooks).toHaveBeenCalledTimes(1); + }); + }); + + describe('allowedTools trust gating', () => { + const projectSkill: SkillConfig = { + name: 'build', + description: 'Project skill with allowedTools', + level: 'project', + filePath: '/project/.qwen/skills/build/SKILL.md', + body: 'Body.', + allowedTools: ['Edit', 'Bash(git push *)'], + }; + + beforeEach(() => { + vi.mocked(mockSkillManager.loadSkillForRuntime).mockResolvedValue( + projectSkill, + ); + }); + + it('does not grant allowedTools for a project skill in an untrusted folder', async () => { + vi.mocked(config.isTrustedFolder).mockReturnValue(false); + + const invocation = ( + skillTool as SkillToolWithProtectedMethods + ).createInvocation({ skill: 'build' }); + const result = await invocation.execute(); + + expect(mockAddSessionAllowRule).not.toHaveBeenCalled(); + // The skill itself still loads — only the permission grants are gated. + expect(partToString(result.llmContent)).toContain('Body.'); + }); + + it('grants allowedTools for a project skill in a trusted folder', async () => { + vi.mocked(config.isTrustedFolder).mockReturnValue(true); + + const invocation = ( + skillTool as SkillToolWithProtectedMethods + ).createInvocation({ skill: 'build' }); + await invocation.execute(); + + expect(mockAddSessionAllowRule).toHaveBeenCalledTimes(2); + expect(mockAddSessionAllowRule).toHaveBeenNthCalledWith(1, 'Edit'); + expect(mockAddSessionAllowRule).toHaveBeenNthCalledWith( + 2, + 'Bash(git push *)', + ); + }); + + it('grants allowedTools for a user-level skill regardless of folder trust', async () => { + vi.mocked(config.isTrustedFolder).mockReturnValue(false); + vi.mocked(mockSkillManager.loadSkillForRuntime).mockResolvedValue({ + ...projectSkill, + level: 'user', + }); + + const invocation = ( + skillTool as SkillToolWithProtectedMethods + ).createInvocation({ skill: 'build' }); + await invocation.execute(); + + expect(mockAddSessionAllowRule).toHaveBeenCalledTimes(2); + }); + + it('grants allowedTools for a bundled skill regardless of folder trust', async () => { + vi.mocked(config.isTrustedFolder).mockReturnValue(false); + vi.mocked(mockSkillManager.loadSkillForRuntime).mockResolvedValue({ + ...projectSkill, + level: 'bundled', + }); + + const invocation = ( + skillTool as SkillToolWithProtectedMethods + ).createInvocation({ skill: 'build' }); + await invocation.execute(); + + expect(mockAddSessionAllowRule).toHaveBeenCalledTimes(2); + }); + + it('grants deferred allowedTools once the folder becomes trusted mid-session', async () => { + vi.mocked(config.isTrustedFolder).mockReturnValue(false); + + const first = ( + skillTool as SkillToolWithProtectedMethods + ).createInvocation({ skill: 'build' }); + await first.execute(); + expect(mockAddSessionAllowRule).not.toHaveBeenCalled(); + + vi.mocked(config.isTrustedFolder).mockReturnValue(true); + + const second = ( + skillTool as SkillToolWithProtectedMethods + ).createInvocation({ skill: 'build' }); + await second.execute(); + expect(mockAddSessionAllowRule).toHaveBeenCalledTimes(2); + + // Later invocations must not grant the rules a second time. + const third = ( + skillTool as SkillToolWithProtectedMethods + ).createInvocation({ skill: 'build' }); + await third.execute(); + expect(mockAddSessionAllowRule).toHaveBeenCalledTimes(2); + }); + + it('grants allowedTools exactly once when a trusted skill is re-invoked', async () => { + // A trust-passing first load applies the grants immediately and must + // not enter the deferred set — otherwise re-invocation would hit the + // re-apply branch and duplicate the allow rules. + vi.mocked(config.isTrustedFolder).mockReturnValue(true); + + const first = ( + skillTool as SkillToolWithProtectedMethods + ).createInvocation({ skill: 'build' }); + await first.execute(); + expect(mockAddSessionAllowRule).toHaveBeenCalledTimes(2); + + const second = ( + skillTool as SkillToolWithProtectedMethods + ).createInvocation({ skill: 'build' }); + await second.execute(); + expect(mockAddSessionAllowRule).toHaveBeenCalledTimes(2); + }); + + it('keeps the deferral alive when re-invoked while still untrusted', async () => { + // Mirrors the hooks-side retention test: the shared deferred set must + // survive still-untrusted re-invocations. + vi.mocked(config.isTrustedFolder).mockReturnValue(false); + + const first = ( + skillTool as SkillToolWithProtectedMethods + ).createInvocation({ skill: 'build' }); + await first.execute(); + expect(mockAddSessionAllowRule).not.toHaveBeenCalled(); + + const second = ( + skillTool as SkillToolWithProtectedMethods + ).createInvocation({ skill: 'build' }); + await second.execute(); + expect(mockAddSessionAllowRule).not.toHaveBeenCalled(); + + vi.mocked(config.isTrustedFolder).mockReturnValue(true); + + const third = ( + skillTool as SkillToolWithProtectedMethods + ).createInvocation({ skill: 'build' }); + await third.execute(); + expect(mockAddSessionAllowRule).toHaveBeenCalledTimes(2); + }); + }); + describe('refreshSkills', () => { it('should refresh when change listener fires', async () => { const newSkills: SkillConfig[] = [ diff --git a/packages/core/src/tools/skill.ts b/packages/core/src/tools/skill.ts index 58d426a7828..2df9ec83516 100644 --- a/packages/core/src/tools/skill.ts +++ b/packages/core/src/tools/skill.ts @@ -38,6 +38,7 @@ import { applySkillAllowedTools, collectAvailableSkillEntries, clearCollectedSkillEntriesCache, + isTrustedSkillLevel, } from './skill-utils.js'; /** @@ -98,6 +99,11 @@ export class SkillTool extends BaseDeclarativeTool { description: string; }> = []; private loadedSkillNames: Set = new Set(); + // Skills whose allowedTools/hooks were skipped by the folder-trust gate + // on first load. The gate is live (isTrustedFolder() re-reads on every + // call), so the early-return path re-applies them once the folder + // becomes trusted mid-session instead of requiring /clear. + private deferredSideEffectSkillNames: Set = new Set(); // Cleanup function returned by `addChangeListener`. Stored so per-agent // SkillTool instances (subagents share the parent's SkillManager) can // detach their listener at teardown — without this the SkillManager @@ -253,6 +259,7 @@ export class SkillTool extends BaseDeclarativeTool { (name: string) => this.loadedSkillNames.add(name), this.config.getModelInvocableCommandsExecutor(), (name: string) => this.loadedSkillNames.has(name), + this.deferredSideEffectSkillNames, ); } @@ -281,6 +288,7 @@ export class SkillTool extends BaseDeclarativeTool { */ clearLoadedSkills(): void { this.loadedSkillNames.clear(); + this.deferredSideEffectSkillNames.clear(); } /** @@ -315,6 +323,7 @@ class SkillToolInvocation extends BaseToolInvocation { ) => Promise) | null = null, private readonly isSkillLoaded: (name: string) => boolean = () => false, + private readonly deferredSideEffectSkillNames: Set = new Set(), ) { super(params); } @@ -353,6 +362,90 @@ class SkillToolInvocation extends BaseToolInvocation { } } + /** + * Applies a loaded skill's repo-supplied side effects — allowedTools + * grants and hook registration — unless the folder-trust gate defers + * them. allowedTools become session-wide permission auto-approvals, and + * hooks are code execution (the same gate Config.getProjectHooks() + * applies to settings-file hooks). Fail closed: only levels that cannot + * originate from the repository skip the gate. Returns true when the + * side effects were deferred, so the caller can re-apply them on a + * later invocation once the folder becomes trusted. + */ + private applySkillSideEffects(skill: SkillConfig): boolean { + // 'user' skills live in ~/.qwen — except when the project root IS the + // home directory: SkillManager skips the 'project' level there, so + // repository-committed skills surface at 'user' level and must be + // gated like project skills. SkillManager records that case on the + // skill at collection time (`homeRootShadow`); recomputing it from + // `this.config.getProjectRoot()` would trust the per-agent override + // this tool runs on inside subagents — worktree isolation and + // working_dir pins rebind `getProjectRoot()` to the worktree path, + // which would flip the detection and open the gate. + const sideEffectsGated = + (!isTrustedSkillLevel(skill.level) || + (skill.level === 'user' && skill.homeRootShadow === true)) && + !this.config.isTrustedFolder(); + + if (sideEffectsGated) { + if (skill.allowedTools?.length) { + debugLogger.warn( + `Skill "${this.params.skill}" declares allowedTools but the folder is not trusted; deferring skill allowedTools until the folder is trusted.`, + ); + } + } else { + // Auto-approve the skill's declared allowedTools for the rest of the session. + applySkillAllowedTools( + this.config.getPermissionManager(), + skill.allowedTools, + ); + } + + // Register skill hooks if present + debugLogger.debug('Skill hooks check:', { + hasHooks: !!skill.hooks, + hooksKeys: skill.hooks ? Object.keys(skill.hooks) : [], + skillName: skill.name, + }); + if (skill.hooks) { + if (sideEffectsGated) { + if (Object.keys(skill.hooks).length > 0) { + debugLogger.warn( + `Skill "${this.params.skill}" declares hooks but the folder is not trusted; deferring skill hooks until the folder is trusted.`, + ); + } + } else { + const hookSystem = this.config.getHookSystem(); + const sessionId = this.config.getSessionId(); + debugLogger.debug('Hook system and session:', { + hasHookSystem: !!hookSystem, + sessionId, + }); + if (hookSystem && sessionId) { + const sessionHooksManager = hookSystem.getSessionHooksManager(); + const hookCount = registerSkillHooks( + sessionHooksManager, + sessionId, + skill, + ); + if (hookCount > 0) { + debugLogger.info( + `Registered ${hookCount} hooks from skill "${this.params.skill}"`, + ); + } else { + debugLogger.warn( + `No hooks registered from skill "${this.params.skill}"`, + ); + } + } + } + } else { + debugLogger.warn(`Skill "${this.params.skill}" has no hooks to register`); + } + + return sideEffectsGated; + } + async execute( _signal?: AbortSignal, _updateOutput?: (output: ToolResultDisplay) => void, @@ -509,6 +602,20 @@ class SkillToolInvocation extends BaseToolInvocation { if (this.isSkillLoaded(this.params.skill)) { this.onSkillLoaded(this.params.skill); void this.recordAutoSkillUsageBestEffort(skill); + // Side effects skipped by the trust gate on first load are applied + // on re-invocation once the folder becomes trusted (the gate is + // live), without re-injecting the skill body into context. Drop the + // entry only once the side effects were actually applied, so + // still-untrusted re-invocations keep the deferral alive. + if ( + this.deferredSideEffectSkillNames.has(this.params.skill) && + !this.applySkillSideEffects(skill) + ) { + this.deferredSideEffectSkillNames.delete(this.params.skill); + debugLogger.info( + `Applied deferred side effects for skill "${this.params.skill}" (folder is now trusted).`, + ); + } const msg = `Skill "${this.params.skill}" is already loaded in context.`; return { llmContent: msg, @@ -518,46 +625,8 @@ class SkillToolInvocation extends BaseToolInvocation { this.onSkillLoaded(this.params.skill); - // Auto-approve the skill's declared allowedTools for the rest of the session. - applySkillAllowedTools( - this.config.getPermissionManager(), - skill.allowedTools, - ); - - // Register skill hooks if present - debugLogger.debug('Skill hooks check:', { - hasHooks: !!skill.hooks, - hooksKeys: skill.hooks ? Object.keys(skill.hooks) : [], - skillName: skill.name, - }); - if (skill.hooks) { - const hookSystem = this.config.getHookSystem(); - const sessionId = this.config.getSessionId(); - debugLogger.debug('Hook system and session:', { - hasHookSystem: !!hookSystem, - sessionId, - }); - if (hookSystem && sessionId) { - const sessionHooksManager = hookSystem.getSessionHooksManager(); - const hookCount = registerSkillHooks( - sessionHooksManager, - sessionId, - skill, - ); - if (hookCount > 0) { - debugLogger.info( - `Registered ${hookCount} hooks from skill "${this.params.skill}"`, - ); - } else { - debugLogger.warn( - `No hooks registered from skill "${this.params.skill}"`, - ); - } - } - } else { - debugLogger.warn( - `Skill "${this.params.skill}" has no hooks to register`, - ); + if (this.applySkillSideEffects(skill)) { + this.deferredSideEffectSkillNames.add(this.params.skill); } const baseDir = path.dirname(skill.filePath); diff --git a/packages/core/src/utils/envVarResolver.test.ts b/packages/core/src/utils/envVarResolver.test.ts index a45b65b1b98..9d792e2dc9a 100644 --- a/packages/core/src/utils/envVarResolver.test.ts +++ b/packages/core/src/utils/envVarResolver.test.ts @@ -52,6 +52,54 @@ describe('resolveEnvVarsInString', () => { expect(result).toBe('Value is $UNDEFINED_VAR'); }); + it('should never resolve Qwen-internal secrets from process.env', () => { + process.env['QWEN_SERVER_TOKEN'] = 'daemon-secret'; + + const result = resolveEnvVarsInString( + 'curl https://x/t=$QWEN_SERVER_TOKEN', + ); + + expect(result).toBe('curl https://x/t=$QWEN_SERVER_TOKEN'); + expect(result).not.toContain('daemon-secret'); + }); + + it('should block internal secrets regardless of casing (Windows process.env)', () => { + process.env['QWEN_SERVER_TOKEN'] = 'daemon-secret'; + process.env['qwen_server_token'] = 'daemon-secret'; + + const result = resolveEnvVarsInString( + 'curl https://x/t=$qwen_server_token', + ); + + expect(result).toBe('curl https://x/t=$qwen_server_token'); + expect(result).not.toContain('daemon-secret'); + }); + + it('should block internal secrets even when customEnv supplies them', () => { + const result = resolveEnvVarsInString('t=$QWEN_SERVER_TOKEN', { + QWEN_SERVER_TOKEN: 'custom-supplied-secret', + }); + expect(result).toBe('t=$QWEN_SERVER_TOKEN'); + expect(result).not.toContain('custom-supplied-secret'); + }); + + it('should never resolve braced ${VAR} internal secrets from process.env', () => { + process.env['QWEN_SERVER_TOKEN'] = 'daemon-secret'; + + const result = resolveEnvVarsInString('t=${QWEN_SERVER_TOKEN}'); + + expect(result).toBe('t=${QWEN_SERVER_TOKEN}'); + expect(result).not.toContain('daemon-secret'); + }); + + it('should block braced internal secrets even when customEnv supplies them', () => { + const result = resolveEnvVarsInString('t=${QWEN_SERVER_TOKEN}', { + QWEN_SERVER_TOKEN: 'custom-supplied-secret', + }); + expect(result).toBe('t=${QWEN_SERVER_TOKEN}'); + expect(result).not.toContain('custom-supplied-secret'); + }); + it('should leave undefined variables with braces unchanged', () => { const result = resolveEnvVarsInString('Value is ${UNDEFINED_VAR}'); diff --git a/packages/core/src/utils/envVarResolver.ts b/packages/core/src/utils/envVarResolver.ts index 096b5549ec0..1df91b796c3 100644 --- a/packages/core/src/utils/envVarResolver.ts +++ b/packages/core/src/utils/envVarResolver.ts @@ -4,11 +4,23 @@ * SPDX-License-Identifier: Apache-2.0 */ +import { isInternalSecretEnvVar } from './sanitize-child-env.js'; + +// Exposed so CLI-side resolvers (e.g. channel config) apply the same +// denylist through this leaf import instead of the full core bundle. +export { isInternalSecretEnvVar }; + /** * Resolves environment variables in a string. * Replaces $VAR_NAME and ${VAR_NAME} with their corresponding environment variable values. * If the environment variable is not defined, the original placeholder is preserved. * + * Qwen-internal secrets (INTERNAL_SECRET_ENV_VARS — daemon tokens, private + * capabilities) are never resolved from `process.env`: a resolved value is + * baked into hook commands, URLs, or MCP configs before child-env + * sanitization ever applies. Leaving the placeholder unresolved is safe — + * spawned children get a sanitized environment without those variables. + * * @param value - The string that may contain environment variable placeholders * @returns The string with environment variables resolved * @@ -24,6 +36,11 @@ export function resolveEnvVarsInString( const envVarRegex = /\$(?:(\w+)|{([^}]+)})/g; // Find $VAR_NAME or ${VAR_NAME} return value.replace(envVarRegex, (match, varName1, varName2) => { const varName = varName1 || varName2; + // Check the denylist before customEnv so no caller-supplied map can + // ever resolve an internal secret. + if (isInternalSecretEnvVar(varName)) { + return match; + } if (customEnv && typeof customEnv[varName] === 'string') { return customEnv[varName]; } diff --git a/packages/core/src/utils/sanitize-child-env.test.ts b/packages/core/src/utils/sanitize-child-env.test.ts index a311e5bf6a8..f670be8595c 100644 --- a/packages/core/src/utils/sanitize-child-env.test.ts +++ b/packages/core/src/utils/sanitize-child-env.test.ts @@ -23,6 +23,17 @@ describe('sanitizeChildEnv', () => { expect(result['QWEN_CODE_PRIVATE_ACP_CAPABILITY']).toBeUndefined(); }); + it('removes internal secrets regardless of key casing (Windows process.env)', () => { + const result = sanitizeChildEnv({ + qwen_server_token: 'lowercase-secret', + Qwen_Daemon_Token: 'mixed-case-secret', + PATH: '/usr/bin', + }); + expect(result['qwen_server_token']).toBeUndefined(); + expect(result['Qwen_Daemon_Token']).toBeUndefined(); + expect(result['PATH']).toBe('/usr/bin'); + }); + it('preserves benign vars and third-party credentials that shell workflows need', () => { const result = sanitizeChildEnv({ QWEN_SERVER_TOKEN: 'super-secret', diff --git a/packages/core/src/utils/sanitize-child-env.ts b/packages/core/src/utils/sanitize-child-env.ts index 6bacbd1173f..45d88f8fe21 100644 --- a/packages/core/src/utils/sanitize-child-env.ts +++ b/packages/core/src/utils/sanitize-child-env.ts @@ -32,6 +32,20 @@ export const INTERNAL_SECRET_ENV_VARS: readonly string[] = [ PRIVATE_ACP_CAPABILITY_ENV, ]; +const INTERNAL_SECRET_ENV_VARS_UPPER = new Set( + INTERNAL_SECRET_ENV_VARS.map((v) => v.toUpperCase()), +); + +/** + * Case-insensitive check for the internal-secret denylist. `process.env` is + * case-insensitive on Windows, so a mixed-case reference like + * `$qwen_server_token` must still be blocked. On POSIX systems a genuinely + * distinct lowercase variable is caught too — deliberate, since these names + * have no legitimate use. + */ +export const isInternalSecretEnvVar = (name: string): boolean => + INTERNAL_SECRET_ENV_VARS_UPPER.has(name.toUpperCase()); + /** * Return a shallow copy of `env` with Qwen-internal secrets removed, so it is * safe to pass to a child process spawned on the user's behalf. Does not @@ -43,8 +57,13 @@ export function sanitizeChildEnv( env: NodeJS.ProcessEnv = process.env, ): NodeJS.ProcessEnv { const sanitized: NodeJS.ProcessEnv = { ...env }; - for (const key of INTERNAL_SECRET_ENV_VARS) { - delete sanitized[key]; + // Case variants must go too: on Windows env keys are case-insensitive, + // so an externally set `qwen_server_token` is just the canonical secret; + // on POSIX the lowercase spelling is removed as well (deliberate). + for (const key of Object.keys(sanitized)) { + if (isInternalSecretEnvVar(key)) { + delete sanitized[key]; + } } return sanitized; } diff --git a/packages/vscode-ide-companion/schemas/settings.schema.json b/packages/vscode-ide-companion/schemas/settings.schema.json index ce0d9375fbe..aaf96b91551 100644 --- a/packages/vscode-ide-companion/schemas/settings.schema.json +++ b/packages/vscode-ide-companion/schemas/settings.schema.json @@ -1467,7 +1467,7 @@ } }, "allowedHttpHookUrls": { - "description": "Whitelist of URL patterns for HTTP hooks. Supports * wildcard. If empty, all URLs are allowed (subject to SSRF protection).", + "description": "Whitelist of URL patterns for HTTP hooks. Supports * wildcard. If empty, all URLs are allowed (subject to SSRF protection). Honored from User, System, and SystemDefaults settings scopes; a value set in Workspace settings can only narrow the User or SystemDefaults whitelist (entries not covered by it are dropped, and it is ignored entirely when no higher-scope whitelist is set), and a System-scope whitelist always takes precedence over it, so a cloned repository cannot widen where hook payloads may be sent.", "type": "array", "items": { "description": "URL pattern (supports * wildcard)", diff --git a/scripts/dev.js b/scripts/dev.js index 34e5e5d50c8..76b9d0f247f 100755 --- a/scripts/dev.js +++ b/scripts/dev.js @@ -67,11 +67,23 @@ const loaderPath = join(tmpDir, 'loader.mjs'); const coreSourcePath = join(root, 'packages', 'core', 'index.ts'); const coreSourceUrl = pathToFileURL(coreSourcePath).href; +// Core's package.json maps subpath exports (e.g. ./envVarResolver) to built +// files under packages/core/dist, which a fresh checkout does not have. The +// bare-specifier remap below does not cover subpaths, so each one cli source +// imports needs its own remap to the source file. +const envVarResolverSourceUrl = pathToFileURL( + join(root, 'packages', 'core', 'src', 'utils', 'envVarResolver.ts'), +).href; +const memoryScopesSourceUrl = pathToFileURL( + join(root, 'packages', 'core', 'src', 'memory', 'scopes.ts'), +).href; const loaderCode = ` import { pathToFileURL } from 'node:url'; const coreSourceUrl = '${coreSourceUrl}'; +const envVarResolverSourceUrl = '${envVarResolverSourceUrl}'; +const memoryScopesSourceUrl = '${memoryScopesSourceUrl}'; export function resolve(specifier, context, nextResolve) { if (specifier === '@qwen-code/qwen-code-core') { @@ -81,6 +93,20 @@ export function resolve(specifier, context, nextResolve) { format: 'module', }; } + if (specifier === '@qwen-code/qwen-code-core/envVarResolver') { + return { + shortCircuit: true, + url: envVarResolverSourceUrl, + format: 'module', + }; + } + if (specifier === '@qwen-code/qwen-code-core/memoryScopes') { + return { + shortCircuit: true, + url: memoryScopesSourceUrl, + format: 'module', + }; + } return nextResolve(specifier, context); } `;