Security hardening: action budgets, dry-run mode, intent capsules - #38
Conversation
… capsules, RAG defense Implements security remediations from the Feb 2026 security audit against OWASP LLM Top 10 (2025) and OWASP Agentic Top 10 (ASI-2026): Phase 0: Centralized policy loader - Extract duplicated TOML→ZoraPolicy parsing into src/config/policy-loader.ts - Refactor src/cli/index.ts and src/cli/daemon.ts to use shared loadPolicy() Phase 1 (LLM06/LLM10 — Excessive Agency / Unbounded Consumption): - Add BudgetPolicy type with max_actions_per_session, per-type caps, token_budget - Implement budget tracking and enforcement in PolicyEngine.createCanUseTool() - Support 'block' and 'flag' modes when budget is exceeded - Add budget defaults to all policy presets (locked/safe/balanced/power) Phase 2 (ASI02 — Tool Misuse): - Add DryRunPolicy type for previewing write operations without executing - Implement dry-run interception in PolicyEngine for Write/Edit/Bash tools - Skip read-only commands (ls, git status, etc.) in dry-run mode - Optional audit logging of dry-run interceptions Phase 3 (ASI01 — Agent Goal Hijack): - Create IntentCapsuleManager with HMAC-SHA256 signed mandate bundles - Implement keyword-based and category-based drift detection - Integrate with PolicyEngine.createCanUseTool() for per-action drift checks - Wire into Orchestrator.boot() and submitTask() lifecycle Phase 4 (LLM01 — Prompt Injection): - Add 10 RAG/tool-output injection patterns to PromptDefense - Create sanitizeToolOutput() with distinct <untrusted_tool_output> tags - Enhanced sanitizeInput() to include RAG injection patterns All new features are backward-compatible: old policy.toml files without budget/dry_run sections continue to work identically. Tests: 502 passing (50 new tests across 3 new test files + existing tests updated) https://claude.ai/code/session_017MTe9JTshvSB6eWtVyaQ24
Archive pre-hardening versions to docs/archive/2026-02/ and update all user-facing documentation to reflect OWASP LLM/Agentic security features: action budgets, dry-run preview mode, intent capsules, and RAG defense. - SECURITY.md: Full rewrite with OWASP compliance matrix, security architecture table, new feature sections with config examples - README.md: Security table in How Security Works, new status rows - CHANGELOG.md: Detailed Security Hardening section for v0.6.0 - SETUP_GUIDE.md: [budget] and [dry_run] in example policy.toml - POLICY_PRESETS.md: All 4 presets with budget/dry-run, summary table - POLICY_REFERENCE.md: Full field reference for new sections - PRODUCTION_READINESS.md: New security components, P2 progress - BEGINNERS_GUIDE.md: Updated presets, key concepts table https://claude.ai/code/session_017MTe9JTshvSB6eWtVyaQ24
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
📝 WalkthroughWalkthroughA comprehensive security hardening update (v0.6) introducing action budgets, dry-run preview mode, intent capsules with drift detection, expanded RAG injection defenses, and a centralized policy-loader module. Includes new security components, policy configuration sections, orchestrator integration, extensive documentation updates, and test coverage. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI
participant Orchestrator
participant PolicyEngine
participant IntentCapsuleManager
participant AuditLogger
User->>CLI: Submit Task
CLI->>Orchestrator: Initialize Session
Orchestrator->>IntentCapsuleManager: Create Signed Capsule
IntentCapsuleManager-->>Orchestrator: Capsule with Mandate Hash
Orchestrator->>PolicyEngine: Register Capsule & Start Session
Orchestrator->>User: Ready to Execute
User->>Orchestrator: Execute Action
Orchestrator->>PolicyEngine: Check Intent Drift
PolicyEngine->>IntentCapsuleManager: Verify Against Mandate
IntentCapsuleManager-->>PolicyEngine: Drift Status
alt Goal Drift Detected
PolicyEngine->>AuditLogger: Log goal_drift Event
PolicyEngine-->>User: Flag for Approval
end
PolicyEngine->>PolicyEngine: Check Action Budget
alt Budget Exceeded
PolicyEngine->>AuditLogger: Log budget_exceeded Event
PolicyEngine-->>User: Flag or Block
end
PolicyEngine->>PolicyEngine: Check Dry-Run Mode
alt Dry-Run Enabled
PolicyEngine->>AuditLogger: Log dry_run Event
PolicyEngine-->>User: Preview (No Execution)
else Allowed
PolicyEngine->>User: Execute Action
PolicyEngine->>AuditLogger: Log audit_success
end
sequenceDiagram
participant Daemon
participant PolicyLoader
participant TOML Parser
participant PolicyEngine
Daemon->>PolicyLoader: loadPolicy(policyPath)
alt Policy File Exists
PolicyLoader->>TOML Parser: Parse policy.toml
TOML Parser-->>PolicyLoader: Raw Config Object
PolicyLoader->>PolicyLoader: parsePolicy(raw)
Note over PolicyLoader: Extract [filesystem], [shell],<br/>[actions], [network],<br/>[budget], [dry_run]<br/>with defaults
PolicyLoader-->>Daemon: ZoraPolicy Object
else Policy Missing
PolicyLoader-->>Daemon: Error with Location Info
end
Daemon->>PolicyEngine: Initialize with Policy
PolicyEngine->>PolicyEngine: Expand Policy (include budget/dry_run)
PolicyEngine-->>Daemon: Ready
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
codeant-ai
Bot
added
the
size:XXL
Summary of ChangesHello @ryaker, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the security posture of the Zora agent by integrating several critical mitigations aligned with OWASP LLM Top 10 (2025) and OWASP Agentic Top 10 (ASI-2026). The changes introduce robust controls for agent autonomy, including action budgets, a dry-run preview mode for sensitive operations, and cryptographically signed intent capsules to prevent goal hijacking. Additionally, prompt injection defenses have been expanded to cover Retrieval-Augmented Generation (RAG) and tool outputs, and the policy loading mechanism has been centralized for improved reliability and configuration management. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Nitpicks 🔍
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant security hardening measures for Zora, aligning with OWASP LLM Top 10 and Agentic Top 10 guidelines. Key changes include implementing action budgets (per-session and per-type limits, token budgets) to prevent unbounded autonomous loops, a dry-run preview mode for write operations, and intent capsules with mandate signing for goal drift detection. Additionally, RAG/tool-output injection defenses have been enhanced with new patterns and a sanitizeToolOutput() function. The policy loading mechanism has been centralized into src/config/policy-loader.ts to reduce code duplication and ensure backward compatibility for existing policy files. Documentation across CHANGELOG.md, PRODUCTION_READINESS.md, README.md, SECURITY.md, SETUP_GUIDE.md, and docs/BEGINNERS_GUIDE.md has been extensively updated to reflect these new features, including a new 'Locked' security preset and detailed explanations of the new security architecture and OWASP compliance. Review comments suggest refactoring the conditional property additions in policy-loader.ts for better readability and clarifying the hash chain calculation description in SECURITY.md for improved user understanding.
| return { | ||
| filesystem: { | ||
| allowed_paths: (fsPol?.['allowed_paths'] as string[]) ?? [], | ||
| denied_paths: (fsPol?.['denied_paths'] as string[]) ?? [], | ||
| resolve_symlinks: (fsPol?.['resolve_symlinks'] as boolean) ?? true, | ||
| follow_symlinks: (fsPol?.['follow_symlinks'] as boolean) ?? false, | ||
| }, | ||
| shell: { | ||
| mode: (shPol?.['mode'] as 'allowlist' | 'denylist' | 'deny_all') ?? 'allowlist', | ||
| allowed_commands: (shPol?.['allowed_commands'] as string[]) ?? ['ls', 'npm', 'git'], | ||
| denied_commands: (shPol?.['denied_commands'] as string[]) ?? [], | ||
| split_chained_commands: (shPol?.['split_chained_commands'] as boolean) ?? true, | ||
| max_execution_time: (shPol?.['max_execution_time'] as string) ?? '1m', | ||
| }, | ||
| actions: { | ||
| reversible: (actPol?.['reversible'] as string[]) ?? [], | ||
| irreversible: (actPol?.['irreversible'] as string[]) ?? [], | ||
| always_flag: (actPol?.['always_flag'] as string[]) ?? [], | ||
| }, | ||
| network: { | ||
| allowed_domains: (netPol?.['allowed_domains'] as string[]) ?? [], | ||
| denied_domains: (netPol?.['denied_domains'] as string[]) ?? [], | ||
| max_request_size: (netPol?.['max_request_size'] as string) ?? '10mb', | ||
| }, | ||
| ...(budPol ? { | ||
| budget: { | ||
| max_actions_per_session: (budPol['max_actions_per_session'] as number) ?? 0, | ||
| max_actions_per_type: (budPol['max_actions_per_type'] as Record<string, number>) ?? {}, | ||
| token_budget: (budPol['token_budget'] as number) ?? 0, | ||
| on_exceed: (budPol['on_exceed'] as 'block' | 'flag') ?? 'block', | ||
| }, | ||
| } : {}), | ||
| ...(dryPol ? { | ||
| dry_run: { | ||
| enabled: (dryPol['enabled'] as boolean) ?? false, | ||
| tools: (dryPol['tools'] as string[]) ?? [], | ||
| audit_dry_runs: (dryPol['audit_dry_runs'] as boolean) ?? true, | ||
| }, | ||
| } : {}), | ||
| }; |
There was a problem hiding this comment.
The use of spread syntax with a ternary operator (...(condition ? { ... } : {})) to conditionally add properties can be a bit difficult to read and is not a very common pattern.
A more straightforward and readable approach would be to construct the base object first and then use standard if statements to add the optional budget and dry_run properties. This would make the logic clearer to future maintainers.
const policy: ZoraPolicy = {
filesystem: {
allowed_paths: (fsPol?.['allowed_paths'] as string[]) ?? [],
denied_paths: (fsPol?.['denied_paths'] as string[]) ?? [],
resolve_symlinks: (fsPol?.['resolve_symlinks'] as boolean) ?? true,
follow_symlinks: (fsPol?.['follow_symlinks'] as boolean) ?? false,
},
shell: {
mode: (shPol?.['mode'] as 'allowlist' | 'denylist' | 'deny_all') ?? 'allowlist',
allowed_commands: (shPol?.['allowed_commands'] as string[]) ?? ['ls', 'npm', 'git'],
denied_commands: (shPol?.['denied_commands'] as string[]) ?? [],
split_chained_commands: (shPol?.['split_chained_commands'] as boolean) ?? true,
max_execution_time: (shPol?.['max_execution_time'] as string) ?? '1m',
},
actions: {
reversible: (actPol?.['reversible'] as string[]) ?? [],
irreversible: (actPol?.['irreversible'] as string[]) ?? [],
always_flag: (actPol?.['always_flag'] as string[]) ?? [],
},
network: {
allowed_domains: (netPol?.['allowed_domains'] as string[]) ?? [],
denied_domains: (netPol?.['denied_domains'] as string[]) ?? [],
max_request_size: (netPol?.['max_request_size'] as string) ?? '10mb',
},
};
if (budPol) {
policy.budget = {
max_actions_per_session: (budPol['max_actions_per_session'] as number) ?? 0,
max_actions_per_type: (budPol['max_actions_per_type'] as Record<string, number>) ?? {},
token_budget: (budPol['token_budget'] as number) ?? 0,
on_exceed: (budPol['on_exceed'] as 'block' | 'flag') ?? 'block',
};
}
if (dryPol) {
policy.dry_run = {
enabled: (dryPol['enabled'] as boolean) ?? false,
tools: (dryPol['tools'] as string[]) ?? [],
audit_dry_runs: (dryPol['audit_dry_runs'] as boolean) ?? true,
};
}
return policy;| 2. Entry 2: `hash_chain = hash(entry1_hash + entry2)` | ||
| 3. Entry 3: `hash_chain = hash(entry2_hash + entry3)` |
There was a problem hiding this comment.
The description of how the hash chain is calculated is a bit ambiguous. For example, hash(entry1_hash + entry2) could be interpreted in a few ways. To improve clarity for users trying to understand or verify the audit log, I suggest describing the process more explicitly.
For example, you could rephrase to something like:
- Entry 1:
hash_chain = hash(genesis_block_data + data_of_entry_1) - Entry 2:
hash_chain = hash(hash_from_entry_1 + data_of_entry_2) - Entry 3:
hash_chain = hash(hash_from_entry_2 + data_of_entry_3)
This makes it clearer that each new hash is a function of the previous hash and the current data.
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| } catch { | ||
| console.error('Policy not found at ~/.zora/policy.toml. Run `zora init` first.'); |
There was a problem hiding this comment.
Suggestion: The catch block treats all loadPolicy failures as "policy not found", so if the policy file exists but is unreadable or contains invalid TOML, the real error is swallowed and the user is incorrectly told to rerun zora init, making debugging harder and potentially causing them to overwrite a broken policy instead of fixing it. [logic error]
Severity Level: Major ⚠️
- ⚠️ `zora start` misreports invalid policy as missing file.
- ⚠️ Users nudged to rerun `zora init` unnecessarily.| } catch { | |
| console.error('Policy not found at ~/.zora/policy.toml. Run `zora init` first.'); | |
| } catch (err) { | |
| const message = err instanceof Error ? err.message : String(err); | |
| if (message.includes('Policy file not found')) { | |
| console.error('Policy not found at ~/.zora/policy.toml. Run `zora init` first.'); | |
| } else { | |
| console.error('Failed to load policy from ~/.zora/policy.toml:', message); | |
| } |
Steps of Reproduction ✅
1. Ensure Zora is initialized so that `~/.zora/config.toml` exists and is valid (checked
in `src/cli/daemon.ts:35-45` before loading the policy).
2. Create or edit `~/.zora/policy.toml` so the file exists but contains invalid TOML
(e.g., an unmatched bracket), causing TOML parsing to fail when read (this file path is
passed as `policyPath` in `src/cli/daemon.ts:38`).
3. Run `zora start`, which executes the CLI entrypoint in `src/cli/index.ts:188-232`; the
`start` command forks the daemon process by running `daemon.js` (see
`src/cli/index.ts:215-221`), which corresponds to `src/cli/daemon.ts` at runtime.
4. In the daemon process, `main()` in `src/cli/daemon.ts:35-55` executes:
`loadPolicy(policyPath)` at line 51 calls `loadPolicy` in
`src/config/policy-loader.ts:15-22`, which reads and parses `policy.toml`. The invalid
TOML causes `parseTOML` at `policy-loader.ts:20-21` to throw an error. This error is
caught by the bare `catch` in `daemon.ts:50-55`, which logs `Policy not found at
~/.zora/policy.toml. Run \`zora init\` first.` and exits with code 1, even though the file
exists and the real issue is a parse/IO error, making debugging the broken policy
difficult.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/cli/daemon.ts
**Line:** 52:53
**Comment:**
*Logic Error: The catch block treats all `loadPolicy` failures as "policy not found", so if the policy file exists but is unreadable or contains invalid TOML, the real error is swallowed and the user is incorrectly told to rerun `zora init`, making debugging harder and potentially causing them to overwrite a broken policy instead of fixing it.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| } else { | ||
| try { | ||
| policy = await loadPolicy(policyPath); | ||
| } catch { |
There was a problem hiding this comment.
Suggestion: In the shared setupContext path, any error from loadPolicy (including invalid or unreadable policy files) is caught and reported as "policy not found", which both hides the real failure mode and may prompt users to rerun zora init and overwrite their existing policy instead of fixing the underlying issue. [logic error]
Severity Level: Major ⚠️
- ⚠️ All CLI commands using setupContext misreport policy parse failures.
- ⚠️ Users may overwrite existing policies by rerunning `zora init`.
- ⚠️ Troubleshooting broken policy.toml becomes significantly harder.| } catch { | |
| } catch (err) { | |
| const message = err instanceof Error ? err.message : String(err); | |
| if (message.includes('Policy file not found')) { | |
| console.error('Policy not found at ~/.zora/policy.toml. Run `zora init` first.'); | |
| } else { | |
| console.error('Failed to load policy from ~/.zora/policy.toml:', message); | |
| } |
Steps of Reproduction ✅
1. Start from a system where Zora has already been initialized so that
`~/.zora/config.toml` and `~/.zora/policy.toml` both exist (the config and policy paths
are constructed in `src/cli/index.ts:76-78` inside `setupContext()`).
2. Manually edit `~/.zora/policy.toml` and introduce a syntax error (for example, remove a
closing bracket), so the file still exists on disk but contains invalid TOML; this file is
later read by `loadPolicy()` in `src/config/policy-loader.ts:15-22` using
`fs.readFileSync` and `smol-toml`'s `parseTOML`.
3. Run any CLI command that invokes `setupContext()`, such as `zora ask "test"` defined at
`src/cli/index.ts:109-146`, which calls `setupContext()` at line 117, or the `status`
command at lines 149-186 which also calls `setupContext()` at line 162.
4. During `setupContext()` (`src/cli/index.ts:75-106`), the code imports `loadPolicy`
(line 89) and calls `policy = await loadPolicy(policyPath);` (line 92); `loadPolicy()`
reads and parses the existing but invalid `~/.zora/policy.toml`, causing `parseTOML` at
`policy-loader.ts:20-22` to throw a parse error. This error is caught by the bare `catch {
... }` block at `index.ts:91-95`, which logs `Policy not found at ~/.zora/policy.toml. Run
\`zora init\` first.` even though the file exists and the real problem is a parse/load
error, misleading the user toward re-running `zora init` (and potentially overwriting
their policy) instead of fixing the TOML.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/cli/index.ts
**Line:** 93:93
**Comment:**
*Logic Error: In the shared `setupContext` path, any error from `loadPolicy` (including invalid or unreadable policy files) is caught and reported as "policy not found", which both hides the real failure mode and may prompt users to rerun `zora init` and overwrite their existing policy instead of fixing the underlying issue.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| const base = this._extractBaseCommand(command); | ||
| if (PolicyEngine.READ_ONLY_COMMANDS.has(base)) return true; | ||
| // git status, git log, git diff are read-only | ||
| if (base === 'git') { | ||
| const parts = command.trim().split(/\s+/); | ||
| const subCommand = parts[1] ?? ''; | ||
| if (['status', 'log', 'diff', 'show', 'branch', 'remote', 'tag'].includes(subCommand)) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Suggestion: The dry-run detector _isReadOnlyCommand treats any Bash command as read-only based only on its first binary (and some git subcommands), so chained commands like ls && rm -rf /tmp or pipelines starting with a "safe" command can bypass dry-run interception and execute destructive operations without preview. [security]
Severity Level: Critical 🚨
- ❌ Dry-run mode skips preview for destructive chained bash commands.
- ❌ ASI02 dry-run mitigation bypassed when chains start with ls.
- ⚠️ Users may suffer unintended filesystem changes despite dry-run enabled.| const base = this._extractBaseCommand(command); | |
| if (PolicyEngine.READ_ONLY_COMMANDS.has(base)) return true; | |
| // git status, git log, git diff are read-only | |
| if (base === 'git') { | |
| const parts = command.trim().split(/\s+/); | |
| const subCommand = parts[1] ?? ''; | |
| if (['status', 'log', 'diff', 'show', 'branch', 'remote', 'tag'].includes(subCommand)) { | |
| return true; | |
| } | |
| const trimmed = command.trim(); | |
| // If the command string contains chaining operators, treat it as potentially state-changing. | |
| if (/[;&|]/.test(trimmed)) { | |
| return false; | |
| } | |
| const base = this._extractBaseCommand(trimmed); | |
| if (PolicyEngine.READ_ONLY_COMMANDS.has(base)) return true; | |
| // Only obviously read-only git subcommands are treated as safe | |
| if (base === 'git') { | |
| const parts = trimmed.split(/\s+/); | |
| const subCommand = parts[1] ?? ''; | |
| if (['status', 'log', 'diff', 'show'].includes(subCommand)) { | |
| return true; | |
| } | |
| } | |
Steps of Reproduction ✅
1. Enable dry-run mode in the policy file used by the CLI:
- Edit `~/.zora/policy.toml` to add a `[dry_run]` section with `enabled = true` and
`tools = []` so dry-run applies to all write tools (as parsed in
`src/config/policy-loader.ts:29-35,61-75`, which populates `ZoraPolicy.dry_run` and is
used by `PolicyEngine` in `src/security/policy-engine.ts:244-246`).
2. Configure the shell allowlist so destructive commands are permitted while relying on
dry-run as the safety net:
- In the same policy file, ensure `[shell]` includes `mode = "allowlist"`,
`split_chained_commands = true` (the default in `policy-loader.ts:44-49`), and add `rm`
to `allowed_commands` alongside `ls`.
- This configuration is loaded via `loadPolicy()` in
`src/config/policy-loader.ts:15-22` and passed into `new PolicyEngine(policy)` in
`src/cli/index.ts:14,98`, establishing the policy used by
`PolicyEngine.createCanUseTool()` in `src/security/policy-engine.ts:444-451`.
3. Run the real CLI flow that wires the Agent SDK through `PolicyEngine`:
- Invoke the documented `ask` command described in `PRODUCTION_READINESS.md:117` (the
only end-to-end path) via `zora ask "..."`, which uses `PolicyEngine` (imported and
instantiated in `src/cli/index.ts:14,98`) to construct the SDK-compatible `canUseTool`
callback (`createCanUseTool()` at `src/security/policy-engine.ts:444-451`).
- During the session, prompt the agent to execute a Bash tool call like: `ls && rm -rf
./tmp/dry-run-bug`.
- The SDK calls `createCanUseTool()`'s inner function (`policy-engine.ts:452-598`) with
`toolName = 'Bash'` and `input.command = 'ls && rm -rf ./tmp/dry-run-bug'`.
4. Observe how the destructive part of the chain bypasses dry-run interception:
- `createCanUseTool()` validates the command with `validateCommand()` at
`src/security/policy-engine.ts:385-431`, which respects `shell.split_chained_commands =
true` and uses `_splitChainedCommands()` (`policy-engine.ts:867-897`) so both `ls` and
`rm -rf ./tmp/dry-run-bug` are allowed under the configured allowlist.
- For dry-run, it then calls `_checkDryRun()` at `policy-engine.ts:241-258`, which for
`toolName === 'Bash'` invokes `_isReadOnlyCommand(command)` (line `255-257`).
- `_isReadOnlyCommand()` at `policy-engine.ts:286-300` only looks at the first binary:
it calls `_extractBaseCommand()` (`policy-engine.ts:903-925`) on the full string `ls &&
rm -rf ./tmp/dry-run-bug`, yielding base command `ls`. Since `ls` is in
`READ_ONLY_COMMANDS` (defined at `policy-engine.ts:65-69`), `_isReadOnlyCommand()`
returns `true`, causing `_checkDryRun()` to exit early (`return null;` at
`policy-engine.ts:257`) and skip dry-run interception.
- As a result, `createCanUseTool()` returns `{ behavior: 'allow', updatedInput: input
}` (`policy-engine.ts:596-597`), the Bash tool executes the *full* chain including `rm
-rf ./tmp/dry-run-bug`, no entry is added to the dry-run log (`getDryRunLog()` at
`policy-engine.ts:230-231`), and the user sees only `getPolicySummary()`'s message `Dry
Run: ENABLED (write operations will be previewed only)` (`policy-engine.ts:671-701`)
despite a destructive write having executed without any preview.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/security/policy-engine.ts
**Line:** 290:298
**Comment:**
*Security: The dry-run detector `_isReadOnlyCommand` treats any `Bash` command as read-only based only on its first binary (and some git subcommands), so chained commands like `ls && rm -rf /tmp` or pipelines starting with a "safe" command can bypass dry-run interception and execute destructive operations without preview.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
|
CodeAnt AI finished reviewing your PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/security/policy-engine.ts (1)
529-594:⚠️ Potential issue | 🟠 MajorDry-run intercepted actions still consume budget quota.
Budget enforcement (line 530-543) runs before dry-run interception (line 588-594). Actions that are intercepted by dry-run (never actually executed) still increment the budget counters via
recordActionat line 532. In dry-run mode, this causes premature budget exhaustion — the budget tracks hypothetical actions rather than actually executed ones.Consider moving the dry-run check before budget enforcement, or not recording budget for dry-run-intercepted actions.
Proposed fix: move dry-run check before budget enforcement
+ // ─── Dry-run interception (ASI02) ────────────────────────────── + const dryRunResult = this._checkDryRun(toolName, input); + if (dryRunResult) { + return { + behavior: 'deny' as const, + message: `[DRY RUN] ${dryRunResult.wouldExecute}`, + }; + } + // ─── Budget enforcement (LLM06/LLM10) ───────────────────────── if (this._policy.budget) { const actionType = this._classifyAction(toolName, input) ?? 'unknown'; ... } ... - // ─── Dry-run interception (ASI02) ────────────────────────────── - const dryRunResult = this._checkDryRun(toolName, input); - if (dryRunResult) { - return { - behavior: 'deny' as const, - message: `[DRY RUN] ${dryRunResult.wouldExecute}`, - }; - }SECURITY.md (1)
431-438:⚠️ Potential issue | 🟡 MinorUpdate vulnerability reporting URL to match current repository.
Line 435: The advisories link points to
https://github.com/ryaker/AgentDev/security/advisories, but this repository isryaker/zora. Update the URL tohttps://github.com/ryaker/zora/security/advisories.
🤖 Fix all issues with AI agents
In `@docs/archive/2026-02/README-pre-hardening.md`:
- Around line 62-77: The fenced ASCII diagram block (the triple-backtick block
containing the ORCHESTRATOR CORE / LLM PROVIDER REGISTRY diagram) lacks a
language specifier causing linter warnings; fix it by changing the opening fence
from ``` to ```text (or ```ascii) so the block becomes a labeled code fence and
keep the closing ``` unchanged, ensuring the ASCII box content (lines with ┌┐│└┘
and headings like ORCHESTRATOR CORE, LLM PROVIDER REGISTRY) remains exactly
as-is.
In `@docs/archive/2026-02/SECURITY-pre-hardening.md`:
- Around line 254-258: Update the incorrect vulnerability reporting URLs that
currently point to "https://github.com/ryaker/AgentDev/security/advisories":
find and replace those occurrences in
docs/archive/2026-02/SECURITY-pre-hardening.md (the URL on/around line 256) and
in SECURITY.md (the URL on/around line 435) so they instead point to
"https://github.com/ryaker/zora/security/advisories"; ensure both files contain
the exact corrected URL string and no other references to the old repository
path remain.
In `@README.md`:
- Around line 56-67: Update the OWASP reference in the "v0.6 Security Hardening"
header line: replace the incorrect phrase "OWASP Agentic Top 10 (ASI-2026)" with
the official name "OWASP Top 10 for Agentic AI Applications (2026 edition)";
ensure the updated string appears in the same sentence that currently reads
"Audited against OWASP LLM Top 10 (2025) and OWASP Agentic Top 10 (ASI-2026)" so
the line becomes "Audited against OWASP LLM Top 10 (2025) and OWASP Top 10 for
Agentic AI Applications (2026 edition)".
In `@SECURITY.md`:
- Around line 449-464: Update the SECURITY.md entry for "always_flag interactive
approval" to reflect that enforcement is partially implemented: mention that
PolicyEngine.createCanUseTool() already enforces `_shouldFlag` and
`_flagCallback` when a flag callback is configured (so interactive approvals
work in that configuration), and clarify that full enforcement across all
runtime scenarios is still in progress; reference the
PolicyEngine.createCanUseTool(), `_shouldFlag`, and `_flagCallback` symbols in
the note so readers can find the implementation.
In `@specs/v5/docs/POLICY_REFERENCE.md`:
- Around line 104-106: The table row for the `split_chained_commands` policy
contains an unescaped pipe character in the inline code snippet (`` `&&`, `||`,
`;`, `|` ``) which breaks the Markdown table; update the content in
POLICY_REFERENCE.md for `split_chained_commands` to escape the pipe (e.g., `\|`)
or replace it with an HTML entity so the inline code becomes something like ``
`&&`, `||`, `;`, \| `` to keep the cell a single column and satisfy markdownlint
MD056.
In `@src/cli/daemon.ts`:
- Around line 50-55: The current bare catch around the loadPolicy call masks all
errors; change the catch to capture the exception (e.g., catch (err)) and
differentiate missing-file vs parse errors: if the error indicates a missing
file/ENOENT for policyPath, print the existing "Policy not found..." message and
exit, otherwise print the actual error (err.message or err.stack) so parse
errors are visible and then exit; update the catch block that wraps await
loadPolicy(policyPath) accordingly.
In `@src/cli/index.ts`:
- Around line 91-96: The catch block around loadPolicy(policyPath) incorrectly
treats all errors as "not found"; change it to inspect the thrown error (from
loadPolicy) and handle file-not-found vs parse/other errors: if the error code
or errno indicates ENOENT (file missing) keep the current "Policy not found..."
message, otherwise print a clear parse/validation error including error.message
(e.g., "Error loading policy: <error.message>") and exit; reference loadPolicy
and policyPath when updating the try/catch so parse errors are surfaced instead
of being masked.
In `@src/cli/presets.ts`:
- Around line 37-47: The locked preset's budget fields use 0 (which per
BudgetPolicy JSDoc means "unlimited") and thus contradict the "Zero access"
intent; update the locked preset's budget object in presets.ts (the budget
property on the locked preset) to explicit tight limits (e.g., set
max_actions_per_session to 10 and token_budget to 50000, and keep
max_actions_per_type as an empty map and on_exceed as 'block') so partial
relaxation of the preset doesn't remove budget guardrails; reference the
BudgetPolicy JSDoc in src/types.ts when selecting final numeric limits.
- Around line 74-78: The safe preset incorrectly sets shell_exec_destructive: 0
which is treated as "unlimited" because PolicyEngine checks per-type limits with
if (typeLimit > 0); change PolicyEngine's per-type check (the code that enforces
per-type limits in PolicyEngine / BudgetPolicy handling) to treat a numeric 0 as
"blocked" by checking for presence (e.g., typeLimit !== undefined) and then
explicitly handle typeLimit === 0 as a hard block, otherwise enforce numeric
limits, update the BudgetPolicy interface docs/comments to document that
per-type 0 means "blocked" (global 0 remains unlimited), and add a unit test
that uses the safe preset to assert shell_exec_destructive operations are
blocked.
In `@src/security/intent-capsule.ts`:
- Around line 90-93: The current call to crypto.timingSafeEqual may throw if the
buffers differ in length; update the verification (the code that reads
capsule.signature and compares to expectedSignature using
crypto.timingSafeEqual) to first safely construct both buffers inside a
try/catch (or validate hex), check that Buffer.byteLength(buf1) ===
Buffer.byteLength(buf2), and only then call crypto.timingSafeEqual; if buffer
construction fails or lengths differ, return false instead of letting the
RangeError propagate, ensuring the comparison routine (that references
capsule.signature, expectedSignature, and crypto.timingSafeEqual) fails
gracefully on malformed/tampered input.
🧹 Nitpick comments (13)
docs/archive/2026-02/README-pre-hardening.md (1)
1-1: Consider documenting the path context for archived content.The relative paths for images (lines 1, 9) and documentation links (lines 110-116) reference locations relative to the repository root. Since this archived file is located at
docs/archive/2026-02/, these paths won't resolve correctly when viewed from the archive location.While this may be acceptable for historical snapshots, consider adding a brief note at the top indicating that paths reference the original structure, or updating paths to work from the archive location (e.g.,
../../../specs/v5/assets/...).Also applies to: 9-9, 110-116
src/security/prompt-defense.ts (1)
98-123:sanitizeToolOutputduplicatessanitizeInput— extract shared logic.Both functions build the identical
allPatternsarray and apply the same global-flag + replace loop. The only difference is the wrapper tag. This violates DRY and means any future pattern or logic change must be applied in two places.Also, the docstring claims this is "more aggressive than sanitizeInput()" but both functions apply the exact same pattern set — the only difference is the tag name. Either make it genuinely more aggressive (e.g., additional tool-specific patterns) or correct the docstring.
♻️ Proposed refactor to extract shared logic
+function wrapInjectionPatterns(content: string, tag: string): string { + let result = content; + const allPatterns = [ + ...INJECTION_PATTERNS, + ...ENCODED_INJECTION_PATTERNS, + ...RAG_INJECTION_PATTERNS, + ]; + for (const pattern of allPatterns) { + const globalPattern = pattern.global + ? pattern + : new RegExp(pattern.source, pattern.flags + 'g'); + result = result.replace(globalPattern, (match) => `<${tag}>${match}</${tag}>`); + } + return result; +} + export function sanitizeInput(content: string): string { - let result = content; - - const allPatterns = [...INJECTION_PATTERNS, ...ENCODED_INJECTION_PATTERNS, ...RAG_INJECTION_PATTERNS]; - - for (const pattern of allPatterns) { - // Ensure global flag is set so all occurrences are replaced, not just the first - const globalPattern = pattern.global - ? pattern - : new RegExp(pattern.source, pattern.flags + 'g'); - result = result.replace(globalPattern, (match) => `<untrusted_content>${match}</untrusted_content>`); - } - - return result; + return wrapInjectionPatterns(content, 'untrusted_content'); } export function sanitizeToolOutput(content: string): string { - let result = content; - - const allPatterns = [ - ...INJECTION_PATTERNS, - ...ENCODED_INJECTION_PATTERNS, - ...RAG_INJECTION_PATTERNS, - ]; - - for (const pattern of allPatterns) { - const globalPattern = pattern.global - ? pattern - : new RegExp(pattern.source, pattern.flags + 'g'); - result = result.replace( - globalPattern, - (match) => `<untrusted_tool_output>${match}</untrusted_tool_output>`, - ); - } - - return result; + return wrapInjectionPatterns(content, 'untrusted_tool_output'); }specs/v5/docs/POLICY_PRESETS.md (1)
35-46: Locked preset: dry-run is redundant given zero budget.With
max_actions_per_session = 0andon_exceed = "block", the budget will block all actions before dry-run interception ever fires. Havingdry_run.enabled = trueis harmless (defense-in-depth), but could confuse users who think dry-run is doing the blocking. Consider adding a brief comment in the doc noting that the budget is the primary gate here.src/orchestrator/orchestrator.ts (1)
99-105: Session ID usesDate.now()— not unique under concurrent boots.
session_${Date.now()}could collide if two sessions start within the same millisecond (unlikely but possible in automated/test scenarios). Consider appending a random suffix similar to thejobIdpattern on line 235.Proposed fix
- this._policyEngine.startSession(`session_${Date.now()}`); + this._policyEngine.startSession(`session_${Date.now()}_${crypto.randomBytes(4).toString('hex')}`);src/cli/daemon.ts (1)
47-48: Bothindex.tsanddaemon.tsduplicate thecreateProvidersfunction and policy-loading boilerplate.
createProviders(lines 19-33 here, lines 44-63 inindex.ts) and the policy-loading try/catch block are nearly identical. Consider extracting both into a shared setup utility to reduce duplication, especially since both files now depend on the same centralized loader.src/types.ts (1)
1-6: Stale version in file header.The comment references "v0.5" but this PR targets v0.6.
📝 Suggested fix
-/** - * Zora Core Types — v0.5 +/** + * Zora Core Types — v0.6tests/unit/security/intent-capsule.test.ts (1)
82-87: Minor grammar nit in test name.
'uses different signing keys produce different signatures'reads awkwardly.📝 Suggested fix
- it('uses different signing keys produce different signatures', () => { + it('rejects capsule verified with a different signing key', () => {src/config/policy-loader.ts (2)
29-77: No runtime validation of TOML value types.All fields use bare
ascasts (e.g.,as string[],as boolean,as number). If a user writesallowed_paths = 42in their TOML, the cast silently passes anumberwhere astring[]is expected, producing confusing downstream errors rather than a clear parse-time message.Since TOML is typed, this is unlikely in well-formed files, but a "fail-fast with a helpful message" approach would be more robust for a security-critical policy loader.
15-23:loadPolicydoesn't handle TOML parse errors gracefully.If
parseTOMLthrows on malformed TOML, the error propagates as an opaquesmol-tomlinternal error. Consider wrapping it with a user-friendly message indicating the file path.Proposed improvement
export async function loadPolicy(policyPath: string): Promise<ZoraPolicy> { if (!fs.existsSync(policyPath)) { throw new Error(`Policy file not found at ${policyPath}. Run \`zora init\` first.`); } const { parse: parseTOML } = await import('smol-toml'); - const raw = parseTOML(fs.readFileSync(policyPath, 'utf-8')) as Record<string, unknown>; - return parsePolicy(raw); + let raw: Record<string, unknown>; + try { + raw = parseTOML(fs.readFileSync(policyPath, 'utf-8')) as Record<string, unknown>; + } catch (err) { + throw new Error(`Failed to parse policy file at ${policyPath}: ${err instanceof Error ? err.message : String(err)}`); + } + return parsePolicy(raw); }src/security/intent-capsule.ts (2)
158-160:getActiveCapsulereturns a mutable reference, unlikegetDriftHistory.
getDriftHistory()(line 166) defensively copies with[...this._driftHistory], butgetActiveCapsule()returns the internal capsule object directly. A caller mutating the returned object would corrupt the manager's state and break subsequentcheckDriftcalls and signature verification.Return a shallow copy for consistency
getActiveCapsule(): IntentCapsule | null { - return this._activeCapsule; + return this._activeCapsule ? { ...this._activeCapsule } : null; }
131-153: Keyword overlap drift detection has a very low threshold (10%) and uses linear search.The 10% keyword overlap threshold at line 138 means a single common keyword in a 10-word action detail is enough to pass. This is intentionally permissive (to avoid false positives in legitimate workflows), but it also makes it easy for an injected action to include one mandate keyword to evade drift detection. Just worth being aware of in threat modeling.
src/security/policy-engine.ts (2)
529-585:_classifyActionis called three times with identical arguments.
_classifyAction(toolName, input)is invoked at line 531 (budget), line 546 (always_flag), and line 566 (drift check) within the samecanUseToolinvocation. Compute it once and reuse the result.Proposed consolidation
+ const action = this._classifyAction(toolName, input); + // ─── Budget enforcement (LLM06/LLM10) ───────────────────────── if (this._policy.budget) { - const actionType = this._classifyAction(toolName, input) ?? 'unknown'; + const actionType = action ?? 'unknown'; const budgetResult = this.recordAction(actionType); ... } // Check always_flag for actions that require approval - const action = this._classifyAction(toolName, input); if (action && this._shouldFlag(action)) { ... } // ─── Intent capsule drift check (ASI01) ──────────────────────── if (this._intentCapsuleManager) { - const driftAction = this._classifyAction(toolName, input) ?? 'unknown'; + const driftAction = action ?? 'unknown'; ... }
122-150: Budget counter increments even when the action is denied.
recordActionincrements_totalActionsand_actionCounts(lines 126-127) before checking limits. This means denied actions inflate the counters. If the intent is to track attempted actions, this is fine. If the intent is to track allowed actions, the increment should happen after the limit check passes.Given the
on_exceed = 'flag'flow where a callback can approve over-budget actions, the current approach means every subsequent action also requires approval once the limit is hit (since the counter keeps climbing). This seems intentional but is worth a doc comment to avoid confusion.
| ``` | ||
| ┌─────────────────────────────────────────────────┐ | ||
| │ ORCHESTRATOR CORE │ | ||
| │ Router → Execution Loop → Failover Controller │ | ||
| │ Retry Queue │ Session Manager │ | ||
| ├─────────────────────────────────────────────────┤ | ||
| │ LLM PROVIDER REGISTRY │ | ||
| │ Claude (Primary) │ Gemini (Secondary) │ | ||
| │ Agent SDK (Native) │ CLI (Subprocess) │ | ||
| ├─────────────────────────────────────────────────┤ | ||
| │ Tools │ Memory │ Security │ | ||
| │ Shell │ MEMORY.md │ Policy Engine │ | ||
| │ Filesystem │ Daily Notes │ Audit Log │ | ||
| │ Web │ Context Loader │ Restrictive FS │ | ||
| └─────────────────────────────────────────────────┘ | ||
| ``` |
There was a problem hiding this comment.
Add language specifier to the fenced code block.
The architecture diagram code block is missing a language identifier, which is flagged by the markdown linter. Adding text or ascii improves rendering consistency across different markdown parsers.
📝 Proposed fix
-```
+```text
┌─────────────────────────────────────────────────┐
│ ORCHESTRATOR CORE │📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ``` | |
| ┌─────────────────────────────────────────────────┐ | |
| │ ORCHESTRATOR CORE │ | |
| │ Router → Execution Loop → Failover Controller │ | |
| │ Retry Queue │ Session Manager │ | |
| ├─────────────────────────────────────────────────┤ | |
| │ LLM PROVIDER REGISTRY │ | |
| │ Claude (Primary) │ Gemini (Secondary) │ | |
| │ Agent SDK (Native) │ CLI (Subprocess) │ | |
| ├─────────────────────────────────────────────────┤ | |
| │ Tools │ Memory │ Security │ | |
| │ Shell │ MEMORY.md │ Policy Engine │ | |
| │ Filesystem │ Daily Notes │ Audit Log │ | |
| │ Web │ Context Loader │ Restrictive FS │ | |
| └─────────────────────────────────────────────────┘ | |
| ``` |
🤖 Prompt for AI Agents
In `@docs/archive/2026-02/README-pre-hardening.md` around lines 62 - 77, The
fenced ASCII diagram block (the triple-backtick block containing the
ORCHESTRATOR CORE / LLM PROVIDER REGISTRY diagram) lacks a language specifier
causing linter warnings; fix it by changing the opening fence from ``` to
```text (or ```ascii) so the block becomes a labeled code fence and keep the
closing ``` unchanged, ensuring the ASCII box content (lines with ┌┐│└┘ and
headings like ORCHESTRATOR CORE, LLM PROVIDER REGISTRY) remains exactly as-is.
| Please use GitHub Security Advisories for private disclosure: | ||
|
|
||
| **https://github.com/ryaker/AgentDev/security/advisories** | ||
|
|
||
| If GitHub advisories are not available to you, open a GitHub issue with the minimum necessary detail and note that you can provide a private report if contacted. |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check the actual URL at line 256 in the target file
sed -n '254,258p' docs/archive/2026-02/SECURITY-pre-hardening.md
# Also check what repository this code is in
cat .git/config 2>/dev/null | grep -A 1 'url =' || echo "Git config not available"
# Check all SECURITY*.md files for security/advisories references
rg -n 'security/advisories' --glob '*.md' -A 1 -B 1Repository: ryaker/zora
Length of output: 727
Vulnerability reporting URLs reference the wrong repository.
Lines 256 in docs/archive/2026-02/SECURITY-pre-hardening.md and line 435 in SECURITY.md both point to github.com/ryaker/AgentDev/security/advisories, but the repository is github.com/ryaker/zora. Update both URLs to https://github.com/ryaker/zora/security/advisories.
🤖 Prompt for AI Agents
In `@docs/archive/2026-02/SECURITY-pre-hardening.md` around lines 254 - 258,
Update the incorrect vulnerability reporting URLs that currently point to
"https://github.com/ryaker/AgentDev/security/advisories": find and replace those
occurrences in docs/archive/2026-02/SECURITY-pre-hardening.md (the URL on/around
line 256) and in SECURITY.md (the URL on/around line 435) so they instead point
to "https://github.com/ryaker/zora/security/advisories"; ensure both files
contain the exact corrected URL string and no other references to the old
repository path remain.
| **v0.6 Security Hardening** — Audited against OWASP LLM Top 10 (2025) and OWASP Agentic Top 10 (ASI-2026): | ||
|
|
||
| | Defense | What It Does | | ||
| |---------|-------------| | ||
| | **Action Budgets** | Per-session limits on tool invocations and token spend prevent unbounded loops | | ||
| | **Dry-Run Mode** | Preview write operations without executing — test policies safely | | ||
| | **Intent Capsules** | HMAC-SHA256 signed mandates detect goal hijacking from injected instructions | | ||
| | **RAG Injection Defense** | 20+ patterns detect prompt injection in tool outputs and RAG documents | | ||
| | **Hash-Chain Audit** | SHA-256 chained append-only log with tamper detection | | ||
| | **AES-256-GCM Secrets** | Encrypted credential storage with PBKDF2 key derivation | | ||
|
|
||
| See **[SECURITY.md](SECURITY.md)** for the full security guide and OWASP compliance matrix. |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
OWASP Agentic AI Security Top 10 ASI 2026 official name
💡 Result:
The official OWASP ASI “Agentic AI Security Top 10” (2026 edition) is named “OWASP Top 10 for Agentic Applications” (also referred to in OWASP materials as the “OWASP Top 10 for Agentic AI Applications”). [1], [2]
Update OWASP standard naming on line 56.
The reference "OWASP Agentic Top 10 (ASI-2026)" is incorrect. The official OWASP standard is named "OWASP Top 10 for Agentic AI Applications" (2026 edition). Update line 56 to use the correct official naming to maintain credibility in security documentation.
🤖 Prompt for AI Agents
In `@README.md` around lines 56 - 67, Update the OWASP reference in the "v0.6
Security Hardening" header line: replace the incorrect phrase "OWASP Agentic Top
10 (ASI-2026)" with the official name "OWASP Top 10 for Agentic AI Applications
(2026 edition)"; ensure the updated string appears in the same sentence that
currently reads "Audited against OWASP LLM Top 10 (2025) and OWASP Agentic Top
10 (ASI-2026)" so the line becomes "Audited against OWASP LLM Top 10 (2025) and
OWASP Top 10 for Agentic AI Applications (2026 edition)".
| | Path allow/deny enforcement | Enforced via PolicyEngine | | ||
| | Shell command allow/deny enforcement | Enforced via PolicyEngine | | ||
| | Symlink boundary checks | Enforced | | ||
| | Agent sees its own policy boundaries | Policy injected into system prompt | | ||
| | `check_permissions` tool (agent self-checks) | Available to agent | | ||
| | Hash-chain audit trail | Working | | ||
| | Action budgets (per-session + per-type) | Enforced via PolicyEngine | | ||
| | Token budget enforcement | Enforced via PolicyEngine | | ||
| | Dry-run preview mode | Enforced via PolicyEngine | | ||
| | Intent capsules (mandate signing) | Active in orchestrator | | ||
| | Goal drift detection | Active with flag callback | | ||
| | RAG injection pattern detection | Active in PromptDefense | | ||
| | Tool output sanitization | Active via sanitizeToolOutput() | | ||
| | `always_flag` interactive approval | Config parsed, enforcement in progress | | ||
| | Runtime permission expansion (mid-task grants) | Planned | | ||
|
|
There was a problem hiding this comment.
always_flag status may be understated.
Line 462 says "enforcement in progress," but PolicyEngine.createCanUseTool() (lines 546-562 in policy-engine.ts) already implements _shouldFlag + _flagCallback enforcement. Consider updating the status to reflect partial enforcement (works when a flag callback is configured).
🤖 Prompt for AI Agents
In `@SECURITY.md` around lines 449 - 464, Update the SECURITY.md entry for
"always_flag interactive approval" to reflect that enforcement is partially
implemented: mention that PolicyEngine.createCanUseTool() already enforces
`_shouldFlag` and `_flagCallback` when a flag callback is configured (so
interactive approvals work in that configuration), and clarify that full
enforcement across all runtime scenarios is still in progress; reference the
PolicyEngine.createCanUseTool(), `_shouldFlag`, and `_flagCallback` symbols in
the note so readers can find the implementation.
| | `denied_commands` | string[] | `[]` | Commands blocked. In `"denylist"` mode, these are the only ones blocked. | | ||
| | `split_chained_commands` | bool | `true` | Parse chained commands (`&&`, `||`, `;`, `|`) and validate each individually. | | ||
| | `max_execution_time` | string | `"1m"` | Maximum wall-clock time for a single command. Format: `"30s"`, `"5m"`, `"1h"`. | |
There was a problem hiding this comment.
Pipe character | in table cell breaks Markdown rendering.
The | characters inside the description of split_chained_commands (within `&&`, `||`, `;`, `|`) are interpreted as table column delimiters, causing the row to have too many columns. This is confirmed by the markdownlint MD056 warning. Escape pipes inside table cells with \|.
📝 Proposed fix
-| `split_chained_commands` | bool | `true` | Parse chained commands (`&&`, `||`, `;`, `|`) and validate each individually. |
+| `split_chained_commands` | bool | `true` | Parse chained commands (`&&`, `\|\|`, `;`, `\|`) and validate each individually. |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| | `denied_commands` | string[] | `[]` | Commands blocked. In `"denylist"` mode, these are the only ones blocked. | | |
| | `split_chained_commands` | bool | `true` | Parse chained commands (`&&`, `||`, `;`, `|`) and validate each individually. | | |
| | `max_execution_time` | string | `"1m"` | Maximum wall-clock time for a single command. Format: `"30s"`, `"5m"`, `"1h"`. | | |
| | `denied_commands` | string[] | `[]` | Commands blocked. In `"denylist"` mode, these are the only ones blocked. | | |
| | `split_chained_commands` | bool | `true` | Parse chained commands (`&&`, `\|\|`, `;`, `\|`) and validate each individually. | | |
| | `max_execution_time` | string | `"1m"` | Maximum wall-clock time for a single command. Format: `"30s"`, `"5m"`, `"1h"`. | |
🧰 Tools
🪛 markdownlint-cli2 (0.20.0)
[warning] 105-105: Spaces inside code span elements
(MD038, no-space-in-code)
[warning] 105-105: Spaces inside code span elements
(MD038, no-space-in-code)
[warning] 105-105: Table column count
Expected: 4; Actual: 7; Too many cells, extra data will be missing
(MD056, table-column-count)
🤖 Prompt for AI Agents
In `@specs/v5/docs/POLICY_REFERENCE.md` around lines 104 - 106, The table row for
the `split_chained_commands` policy contains an unescaped pipe character in the
inline code snippet (`` `&&`, `||`, `;`, `|` ``) which breaks the Markdown
table; update the content in POLICY_REFERENCE.md for `split_chained_commands` to
escape the pipe (e.g., `\|`) or replace it with an HTML entity so the inline
code becomes something like `` `&&`, `||`, `;`, \| `` to keep the cell a single
column and satisfy markdownlint MD056.
| try { | ||
| policy = await loadPolicy(policyPath); | ||
| } catch { | ||
| console.error('Policy not found at ~/.zora/policy.toml. Run `zora init` first.'); | ||
| process.exit(1); | ||
| } |
There was a problem hiding this comment.
Same error-masking issue as in src/cli/index.ts.
The bare catch swallows parse errors from loadPolicy, reporting them all as "Policy not found." Apply the same fix as suggested for index.ts.
Proposed fix
- try {
- policy = await loadPolicy(policyPath);
- } catch {
- console.error('Policy not found at ~/.zora/policy.toml. Run `zora init` first.');
+ try {
+ policy = await loadPolicy(policyPath);
+ } catch (err) {
+ console.error(err instanceof Error ? err.message : 'Failed to load policy at ~/.zora/policy.toml. Run `zora init` first.');
process.exit(1);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| policy = await loadPolicy(policyPath); | |
| } catch { | |
| console.error('Policy not found at ~/.zora/policy.toml. Run `zora init` first.'); | |
| process.exit(1); | |
| } | |
| try { | |
| policy = await loadPolicy(policyPath); | |
| } catch (err) { | |
| console.error(err instanceof Error ? err.message : 'Failed to load policy at ~/.zora/policy.toml. Run `zora init` first.'); | |
| process.exit(1); | |
| } |
🤖 Prompt for AI Agents
In `@src/cli/daemon.ts` around lines 50 - 55, The current bare catch around the
loadPolicy call masks all errors; change the catch to capture the exception
(e.g., catch (err)) and differentiate missing-file vs parse errors: if the error
indicates a missing file/ENOENT for policyPath, print the existing "Policy not
found..." message and exit, otherwise print the actual error (err.message or
err.stack) so parse errors are visible and then exit; update the catch block
that wraps await loadPolicy(policyPath) accordingly.
| try { | ||
| policy = await loadPolicy(policyPath); | ||
| } catch { | ||
| console.error('Policy not found at ~/.zora/policy.toml. Run `zora init` first.'); | ||
| process.exit(1); | ||
| } |
There was a problem hiding this comment.
Catch block masks TOML parse errors as "not found".
If policy.toml exists but contains invalid TOML, loadPolicy will throw a parse error, but the catch block always prints "Policy not found." This will confuse users who have a policy file but with a syntax error.
Proposed fix
- try {
- policy = await loadPolicy(policyPath);
- } catch {
- console.error('Policy not found at ~/.zora/policy.toml. Run `zora init` first.');
+ try {
+ policy = await loadPolicy(policyPath);
+ } catch (err) {
+ console.error(err instanceof Error ? err.message : 'Failed to load policy at ~/.zora/policy.toml. Run `zora init` first.');
process.exit(1);
}🤖 Prompt for AI Agents
In `@src/cli/index.ts` around lines 91 - 96, The catch block around
loadPolicy(policyPath) incorrectly treats all errors as "not found"; change it
to inspect the thrown error (from loadPolicy) and handle file-not-found vs
parse/other errors: if the error code or errno indicates ENOENT (file missing)
keep the current "Policy not found..." message, otherwise print a clear
parse/validation error including error.message (e.g., "Error loading policy:
<error.message>") and exit; reference loadPolicy and policyPath when updating
the try/catch so parse errors are surfaced instead of being masked.
| budget: { | ||
| max_actions_per_session: 0, | ||
| max_actions_per_type: {}, | ||
| token_budget: 0, | ||
| on_exceed: 'block', | ||
| }, | ||
| dry_run: { | ||
| enabled: true, | ||
| tools: [], | ||
| audit_dry_runs: true, | ||
| }, |
There was a problem hiding this comment.
locked preset budget values of 0 mean "unlimited" per BudgetPolicy docs — contradicts intent.
According to the BudgetPolicy JSDoc in src/types.ts (Line 346–350), max_actions_per_session: 0 and token_budget: 0 both mean unlimited. For a preset described as "Zero access," this is semantically contradictory and reduces defense-in-depth.
While the locked preset's other controls (empty allowed_paths, deny_all shell mode) prevent execution in practice, an operator who partially relaxes the locked preset (e.g., adding one allowed path) would inadvertently have no budget guardrails.
Consider setting tight explicit limits (e.g., max_actions_per_session: 10, token_budget: 50_000) rather than relying on 0 = unlimited.
🛡️ Proposed fix
budget: {
- max_actions_per_session: 0,
+ max_actions_per_session: 10,
max_actions_per_type: {},
- token_budget: 0,
+ token_budget: 50_000,
on_exceed: 'block',
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| budget: { | |
| max_actions_per_session: 0, | |
| max_actions_per_type: {}, | |
| token_budget: 0, | |
| on_exceed: 'block', | |
| }, | |
| dry_run: { | |
| enabled: true, | |
| tools: [], | |
| audit_dry_runs: true, | |
| }, | |
| budget: { | |
| max_actions_per_session: 10, | |
| max_actions_per_type: {}, | |
| token_budget: 50_000, | |
| on_exceed: 'block', | |
| }, | |
| dry_run: { | |
| enabled: true, | |
| tools: [], | |
| audit_dry_runs: true, | |
| }, |
🤖 Prompt for AI Agents
In `@src/cli/presets.ts` around lines 37 - 47, The locked preset's budget fields
use 0 (which per BudgetPolicy JSDoc means "unlimited") and thus contradict the
"Zero access" intent; update the locked preset's budget object in presets.ts
(the budget property on the locked preset) to explicit tight limits (e.g., set
max_actions_per_session to 10 and token_budget to 50000, and keep
max_actions_per_type as an empty map and on_exceed as 'block') so partial
relaxation of the preset doesn't remove budget guardrails; reference the
BudgetPolicy JSDoc in src/types.ts when selecting final numeric limits.
| budget: { | ||
| max_actions_per_session: 100, | ||
| max_actions_per_type: { shell_exec: 20, shell_exec_destructive: 0 }, | ||
| token_budget: 200_000, | ||
| on_exceed: 'block', |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "policy-engine.ts" -o -name "*budget*.ts" | head -20Repository: ryaker/zora
Length of output: 130
🏁 Script executed:
# Search for PolicyEngine and budget-related files
fd -e ts | grep -E "(policy|budget)" | head -30Repository: ryaker/zora
Length of output: 196
🏁 Script executed:
# Check the BudgetPolicy interface and PolicyEngine implementation
rg -n 'max_actions_per_type' --type=ts -A 10 -B 5 | head -100Repository: ryaker/zora
Length of output: 5824
🏁 Script executed:
# Look for per-type budget handling logic
rg -n 'per.type|per_type' --type=ts -A 5 -B 2 | head -150Repository: ryaker/zora
Length of output: 9277
Per-type limit of 0 is treated as "unlimited" — not "blocked" — a bug in the safe preset.
The PolicyEngine checks per-type limits with the condition if (typeLimit > 0) at lines 138–139. When shell_exec_destructive: 0 in the safe preset, this check is skipped entirely, allowing unlimited destructive commands. This contradicts the intent to block them. The BudgetPolicy interface also lacks documentation of per-type 0 semantics, unlike the global limits which explicitly document 0 = unlimited. No test coverage exists for this scenario.
🤖 Prompt for AI Agents
In `@src/cli/presets.ts` around lines 74 - 78, The safe preset incorrectly sets
shell_exec_destructive: 0 which is treated as "unlimited" because PolicyEngine
checks per-type limits with if (typeLimit > 0); change PolicyEngine's per-type
check (the code that enforces per-type limits in PolicyEngine / BudgetPolicy
handling) to treat a numeric 0 as "blocked" by checking for presence (e.g.,
typeLimit !== undefined) and then explicitly handle typeLimit === 0 as a hard
block, otherwise enforce numeric limits, update the BudgetPolicy interface
docs/comments to document that per-type 0 means "blocked" (global 0 remains
unlimited), and add a unit test that uses the safe preset to assert
shell_exec_destructive operations are blocked.
| return crypto.timingSafeEqual( | ||
| Buffer.from(capsule.signature, 'hex'), | ||
| Buffer.from(expectedSignature, 'hex'), | ||
| ); |
There was a problem hiding this comment.
timingSafeEqual throws on length mismatch — tampered capsules crash instead of returning false.
If capsule.signature is malformed (not valid hex, truncated, or padded), Buffer.from(capsule.signature, 'hex') may produce a buffer of a different length than expectedSignature. crypto.timingSafeEqual throws a RangeError when buffer lengths differ, causing an unhandled exception instead of gracefully returning false.
Proposed fix: guard against length mismatch
verifyCapsule(capsule: IntentCapsule): boolean {
const payload = JSON.stringify({
capsuleId: capsule.capsuleId,
mandate: capsule.mandate,
mandateHash: capsule.mandateHash,
mandateKeywords: capsule.mandateKeywords,
allowedActionCategories: capsule.allowedActionCategories,
createdAt: capsule.createdAt,
expiresAt: capsule.expiresAt,
});
const expectedSignature = crypto
.createHmac('sha256', this._signingKey)
.update(payload)
.digest('hex');
+ const sigBuf = Buffer.from(capsule.signature, 'hex');
+ const expBuf = Buffer.from(expectedSignature, 'hex');
+
+ if (sigBuf.length !== expBuf.length) return false;
+
return crypto.timingSafeEqual(
- Buffer.from(capsule.signature, 'hex'),
- Buffer.from(expectedSignature, 'hex'),
+ sigBuf,
+ expBuf,
);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return crypto.timingSafeEqual( | |
| Buffer.from(capsule.signature, 'hex'), | |
| Buffer.from(expectedSignature, 'hex'), | |
| ); | |
| const sigBuf = Buffer.from(capsule.signature, 'hex'); | |
| const expBuf = Buffer.from(expectedSignature, 'hex'); | |
| if (sigBuf.length !== expBuf.length) return false; | |
| return crypto.timingSafeEqual( | |
| sigBuf, | |
| expBuf, | |
| ); |
🤖 Prompt for AI Agents
In `@src/security/intent-capsule.ts` around lines 90 - 93, The current call to
crypto.timingSafeEqual may throw if the buffers differ in length; update the
verification (the code that reads capsule.signature and compares to
expectedSignature using crypto.timingSafeEqual) to first safely construct both
buffers inside a try/catch (or validate hex), check that Buffer.byteLength(buf1)
=== Buffer.byteLength(buf2), and only then call crypto.timingSafeEqual; if
buffer construction fails or lengths differ, return false instead of letting the
RangeError propagate, ensuring the comparison routine (that references
capsule.signature, expectedSignature, and crypto.timingSafeEqual) fails
gracefully on malformed/tampered input.
2 participants
User description
Summary
Implements three critical security mitigations from OWASP LLM Top 10 (2025) and OWASP Agentic Top 10 (ASI-2026):
Changes
Core Security Features
src/security/intent-capsule.ts(new) —IntentCapsuleManagercreates HMAC-SHA256 signed intent capsules at task start, extracts mandate keywords, and detects goal hijacking via keyword overlap and action category matchingsrc/security/policy-engine.ts— Added budget tracking (recordAction,recordTokens,getBudgetStatus), dry-run mode integration, and intent capsule verification hookssrc/security/prompt-defense.ts— Added RAG/tool-output injection patterns (e.g.,[IMPORTANT INSTRUCTION],NOTE TO AI) andsanitizeToolOutput()for defense against prompt injection via retrieved documentssrc/security/security-types.ts— New types:BudgetStatus,DryRunResult,IntentCapsule,DriftCheckResultConfiguration & Policy
src/types.ts— AddedBudgetPolicyandDryRunPolicyinterfaces toZoraPolicysrc/config/policy-loader.ts(new) — Centralized TOML →ZoraPolicyparsing with backward compatibility for missing sectionssrc/cli/presets.ts— Updated all presets (Locked, Safe, Balanced, Power) with budget and dry-run sectionsspecs/v5/docs/POLICY_PRESETS.md— Added "Locked" preset (fresh install default, zero access) and budget/dry-run config for all presetsspecs/v5/docs/POLICY_REFERENCE.md— Complete reference for[budget]and[dry_run]policy sectionsCLI & Daemon
src/cli/index.ts— Refactored to use centralizedloadPolicy()from policy-loadersrc/cli/daemon.ts— Refactored to use centralizedloadPolicy()from policy-loadersrc/orchestrator/orchestrator.ts— Added intent capsule creation at task start and budget/drift tracking integrationDocumentation
SECURITY.md— Updated with v0.6 security hardening overview, four trust levels (added "Locked"), budget limits per preset, and dry-run mode explanationCHANGELOG.md— Added v0.6.0 security hardening section detailing all three mitigationsSETUP_GUIDE.md— Added budget configuration section with OWASP referencesREADME.md— Updated security callout to mention action budgetsdocs/BEGINNERS_GUIDE.md— Updated security section to reference four presetsPRODUCTION_READINESS.md— Updated component count (27 → 29) and added IntentCapsuleManager and policy-loader to production-grade listTests
tests/unit/security/action-budget.test.ts(new) — 8 tests covering per-session limits, per-type limits, and budget status queriestests/unit/security/dry-run.test.ts(new) — 9 tests covering dry-run mode enable/disable, write operation preview, and audit loggingtests/unit/security/intent-capsule.test.ts(new) — 8https://claude.ai/code/session_017MTe9JTshvSB6eWtVyaQ24
CodeAnt-AI Description
Enforce action budgets, dry-run previews, and signed intent capsules; sanitize tool outputs for injection patterns
What Changed
Impact
✅ Fewer runaway tool invocations✅ Clearer previews for destructive operations✅ Fewer prompt-injection surprises💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Summary by CodeRabbit
Release Notes
New Features
Documentation