feat: granular session-scoped allow rules#71
Conversation
Add allowForSessionGranular to SecurityRule, enabling per-argument allow overrides scoped to a session key prefix. Lets users approve a specific file path, command, or URL for a cron job session without blanket-allowing the entire module.method. - types.ts: add allowForSessionGranular to SecurityRule - Interceptor.ts: add extractPrimaryArg(), check granular entries via glob; add reloadPolicy() - config.ts: add Session/Memory/Agent default rules; FileSystem.delete DENY -> ASK - test/granular-allow.test.mjs: full test coverage
b579e8a to
6571337
Compare
Greptile SummaryThis PR introduces
Confidence Score: 3/5Two present defects on the changed paths need resolution before merging. The Shell tilde expansion asymmetry in extractPrimaryArg causes argGlob matching to silently fail for commands containing ~, and the FileSystem.delete default change removes an unconditional hard block that existing integrations may rely on without any reconfiguration. src/core/Interceptor.ts (Shell tilde expansion in extractPrimaryArg) and src/config.ts (FileSystem.delete default policy change) need a second look. Important Files Changed
Reviews (1): Last reviewed commit: "feat: granular session-scoped allow rule..." | Re-trigger Greptile |
| if (moduleName === 'Shell') { | ||
| const v = params.command ?? params.cmd ?? null; | ||
| return v != null ? String(v) : null; | ||
| } | ||
| const raw = params.path ?? params.file_path ?? params.filePath ?? params.target ?? null; | ||
| if (raw != null) return path.resolve(expandTilde(String(raw))); | ||
| const first = Object.values(params)[0]; | ||
| return first != null ? String(first) : null; |
There was a problem hiding this comment.
Shell
~ tilde not expanded before glob match
extractPrimaryArg returns the raw command string for the Shell module (no tilde expansion), but matchesGlob only calls expandTilde on the pattern, not on the str. If an agent invokes a command like ~/scripts/backup.sh and the operator writes argGlob: "~/scripts/backup.sh", the comparison becomes:
- pattern after
expandTilde:/home/user/scripts/backup.sh→ regex anchored to that absolute path - str:
~/scripts/backup.sh— the tilde is never expanded
The regex test fails and the granular allow is silently skipped, leaving the call to fall through to the original rule. For filesystem paths this is harmless because extractPrimaryArg already calls path.resolve(expandTilde(...)), but the Shell branch needs the same treatment or matchesGlob needs to call expandTilde on the str side too.
| /** | ||
| * Extract the primary arg value for a tool call, used for granular session allow matching. | ||
| * Returns the raw string value (not resolved) so globs can match commands and URLs too. | ||
| */ |
There was a problem hiding this comment.
Misleading comment — filesystem paths ARE resolved, not "raw"
The doc-comment says "Returns the raw string value (not resolved)" but the implementation calls path.resolve(expandTilde(...)) for any non-Shell, non-Browser/Network call. In practice argGlob entries for FileSystem rules must always be absolute paths; a relative pattern like reports/*.csv will never match.
| /** | |
| * Extract the primary arg value for a tool call, used for granular session allow matching. | |
| * Returns the raw string value (not resolved) so globs can match commands and URLs too. | |
| */ | |
| /** | |
| * Extract the primary arg value for a tool call, used for granular session allow matching. | |
| * For Browser/Network modules returns the URL as-is; for Shell returns the raw command string. | |
| * For all other modules (e.g. FileSystem) the path is resolved to an absolute form via | |
| * path.resolve(expandTilde(…)), so argGlob patterns for those modules must also be absolute. | |
| */ |
| delete: { | ||
| action: 'DENY', | ||
| description: 'Deletion is strictly prohibited', | ||
| action: 'ASK', | ||
| description: 'File deletion requires approval', | ||
| }, |
There was a problem hiding this comment.
Default policy for
FileSystem.delete downgraded from hard DENY to ASK
Before this PR the default rule was an unconditional DENY — callers could not delete files regardless of session or intervention state. Changing it to ASK means deletion is now gated by a human approval prompt (TTY) or the channel approval flow. Any project that relies on the default policy and expects file deletion to be unconditionally blocked will silently lose that guarantee without any config change. The granular cron-session use case can be achieved without touching the default by leaving action: 'DENY' here and having callers supply a custom policy with the allowForSessionGranular entries.
|
|
||
| const require = createRequire(import.meta.url); | ||
| const { Interceptor } = require('../dist/core/Interceptor.js'); |
There was a problem hiding this comment.
Tests import from
dist/ — require a prior build step
Both require('../dist/core/Interceptor.js') and require('../dist/plugin/tool-interceptor.js') reference compiled output. Running npm test on a clean checkout will fail with "Cannot find module" rather than a clear test failure. Consider a pretest build script or switching to tsx/ts-node imports. The new reloadPolicy method also has no test coverage.
Add allowForSessionGranular to SecurityRule, enabling per-argument allow overrides scoped to a session key prefix. Lets users approve a specific file path, command, or URL for a cron job session without blanket-allowing the entire module.method.