Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 56 additions & 0 deletions packages/core/src/core/permissionFlow.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,62 @@ describe('evaluatePermissionFlow', () => {
expect(result.denyMessage).toContain('Matching deny rule');
});

it('frames a specifier-scoped deny as invocation-scoped, not tool-scoped', async () => {
const mockPm = {
hasRelevantRules: vi.fn().mockReturnValue(true),
evaluate: vi.fn().mockResolvedValue('deny'),
findMatchingDenyRule: vi.fn().mockReturnValue('Bash(npm view *)'),
hasMatchingAskRule: vi.fn().mockReturnValue(false),
};

const invocation = mockInvocation({
getDefaultPermission: vi.fn().mockResolvedValue('ask'),
});

const result = await evaluatePermissionFlow(
mockConfig({ getPermissionManager: vi.fn().mockReturnValue(mockPm) }),
invocation,
'shell',
{ command: 'npm view foo' },
);

expect(result.finalPermission).toBe('deny');
// The message must read as "this call was blocked", not "the tool is gone"
// (issue #11405), and must reassure the model the tool is still usable.
expect(result.denyMessage).toContain('invocation was denied');
expect(result.denyMessage).toContain('Bash(npm view *)');
expect(result.denyMessage).toContain(
'Other uses of this tool are still permitted',
);
});

it('does not reassure for tool-wide catch-all deny rules (#11405)', async () => {
for (const raw of ['Bash(*)', 'Read(//**)', 'WebFetch(*)']) {
const mockPm = {
hasRelevantRules: vi.fn().mockReturnValue(true),
evaluate: vi.fn().mockResolvedValue('deny'),
findMatchingDenyRule: vi.fn().mockReturnValue(raw),
hasMatchingAskRule: vi.fn().mockReturnValue(false),
};

const result = await evaluatePermissionFlow(
mockConfig({ getPermissionManager: vi.fn().mockReturnValue(mockPm) }),
mockInvocation({
getDefaultPermission: vi.fn().mockResolvedValue('ask'),
}),
'shell',
{ command: 'echo hello' },
);

// The rule is still cited …
expect(result.denyMessage).toContain(`Matching deny rule: "${raw}"`);
// … but a fully-blocked tool must not be told it can try other uses.
expect(result.denyMessage).not.toContain(
'Other uses of this tool are still permitted',
);
}
});

it('should return ask permission when PM has no relevant rules', async () => {
const mockPm = {
hasRelevantRules: vi.fn().mockReturnValue(false),
Expand Down
36 changes: 35 additions & 1 deletion packages/core/src/core/permissionFlow.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import {
buildPermissionCheckContext,
evaluatePermissionRules,
} from './permission-helpers.js';
import { parseRule } from '../permissions/rule-parser.js';
import type { PermissionDecision } from '../permissions/types.js';
import type { ToolCallConfirmationDetails } from '../tools/tools.js';

Expand Down Expand Up @@ -107,13 +108,46 @@ export async function evaluatePermissionFlow(
const ruleInfo = matchingRule
? ` Matching deny rule: "${matchingRule}".`
: '';
result.denyMessage = `Tool "${toolName}" is denied by permission rules.${ruleInfo}`;
// A specifier-scoped deny (e.g. `Bash(npm view *)`) blocks only this
// invocation, not the whole tool. Say so explicitly so the model does
// not abandon the tool entirely (issue #11405). Tool-wide catch-alls
// (`Bash(*)`, `Read(//**)`, `WebFetch(*)`) block the tool entirely and
// must NOT get this reassurance.
const stillPermitted =
matchingRule && !isToolWideDenyRule(matchingRule)
? ' Other uses of this tool are still permitted.'
: '';
result.denyMessage = `This "${toolName}" invocation was denied by permission rules.${ruleInfo}${stillPermitted}`;
}
}

return result;
}

/**
* Whether a raw deny-rule string blocks the tool as a whole (tool-wide)
* rather than a single scoped invocation.
*
* Tool-wide forms are the bare tool name (`Bash`), an empty specifier
* (`Bash()`), the command/domain catch-all (`Bash(*)`, `WebFetch(*)`), and
* the filesystem-root path catch-all (`Read(//**)`). A rule like
* `Bash(npm view *)` is scoped and returns false.
*/
function isToolWideDenyRule(raw: string): boolean {
const rule = parseRule(raw);
if (!rule.specifier) {
// No specifier blocks the whole tool; a param-matcher-only rule
// (e.g. `Agent(model:opus)`) is still scoped by its matchers.
return !rule.toolParamMatchers?.length;
}
if (rule.specifierKind === 'path') {
// `//**` resolves to the filesystem root (`/**`) and matches every path.
return rule.specifier === '//**';
}
// `*` is the documented catch-all for command and domain specifiers.
return rule.specifier === '*';
}

/**
* Check if the tool needs user confirmation based on the permission flow
* result and the current ApprovalMode.
Expand Down
32 changes: 32 additions & 0 deletions packages/core/src/permissions/permission-manager.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3847,6 +3847,38 @@ describe('PermissionManager.findMatchingDenyRule', () => {
fs.rmSync(root, { recursive: true, force: true });
}
});

it('cites the deny rule for a compound command segment', () => {
const pm = new PermissionManager(
makeConfig({ permissionsDeny: ['Bash(npm view *)'] }),
);
pm.initialize();

// evaluate() splits the compound command and denies on the `npm view`
// segment, so findMatchingDenyRule must cite that same rule (issue #11405).
expect(
pm.findMatchingDenyRule({
toolName: 'run_shell_command',
command: 'cd /tmp && npm view foo',
}),
).toBe('Bash(npm view *)');
});

it('cites the deny rule when a shell command is denied via a virtual file op', () => {
const pm = new PermissionManager(
makeConfig({ permissionsDeny: ['Read(//**/node_modules/**)'] }),
);
pm.initialize();

// A `cat` of a node_modules file is denied through the shell virtual-op
// pass (Read rule), not a Bash rule. findMatchingDenyRule must cite it.
expect(
pm.findMatchingDenyRule({
toolName: 'run_shell_command',
command: 'cat /app/node_modules/lodash/index.js',
}),
).toBe('Read(//**/node_modules/**)');
});
});

// ─── AUTO mode dangerous-rule stash ────────────────────────────────────
Expand Down
51 changes: 47 additions & 4 deletions packages/core/src/permissions/permission-manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -919,6 +919,52 @@ export class PermissionManager {
}
: undefined;

const denyRules = [...this.sessionRules.deny, ...this.persistentRules.deny];

// ── Cross-command virtual-op pass (shell tools only) ─────────────────
// Mirrors evaluate(): a shell command can be denied by a Read/Edit/Write/
// WebFetch rule matching an operation extracted from the command, even
// though the deny rule's toolName (e.g. `read_file`) never matches the
// shell tool name. Without this pass the citation silently drops.
if (SHELL_TOOL_NAMES.has(toolName) && command !== undefined) {
const cwdForOps = pathCtx?.cwd ?? process.cwd();
const ops = extractShellOperationsAcrossCommand(command, cwdForOps);
for (const op of ops) {
const opMatchArgs = [
op.virtualTool,
undefined,
op.filePath,
op.domain,
pathCtx,
undefined,
] as const;
for (const rule of denyRules) {
if (
matchesRule(rule, ...opMatchArgs, undefined, undefined, 'canonical')
) {
return rule.raw;
}
}
}
}

// ── Compound-command pass ────────────────────────────────────────────
// Mirrors evaluate(): each segment is evaluated independently, so a deny
// rule matching any segment is the deciding rule. Recurse per segment so
// nested compounds and per-segment virtual ops are covered.
if (SHELL_TOOL_NAMES.has(toolName) && command !== undefined) {
const subCommands = splitCompoundCommand(command);
if (subCommands.length > 1) {
for (const subCmd of subCommands) {
const rule = this.findMatchingDenyRule({ ...ctx, command: subCmd });
if (rule) {
return rule;
}
}
}
}

// ── Single-context match ─────────────────────────────────────────────
const matchArgs = [
toolName,
command,
Expand All @@ -930,10 +976,7 @@ export class PermissionManager {
toolAliases,
] as const;

for (const rule of [
...this.sessionRules.deny,
...this.persistentRules.deny,
]) {
for (const rule of denyRules) {
if (matchesRule(rule, ...matchArgs, 'canonical')) {
return rule.raw;
}
Expand Down
Loading