Conversation
Explores 7 architecture options for policy-based filtering of tool calls and MCP calls: 1. Simple YAML Rules - allowlist/denylist patterns 2. Kyverno-Style Policies - rich declarative validation/mutation 3. OPA/Rego - industry standard policy engine 4. K8s-Native RBAC - ServiceAccount-based enforcement 5. ABAC - attribute-based access control 6. CEL - Common Expression Language (K8s 1.26+ style) 7. Hybrid Approach - layered combination (recommended) Includes trade-offs, concrete namespace restriction examples, comparison matrix, and implementation roadmap. https://claude.ai/code/session_01ELTfzzxV3mBS2RzTAZiXeX Signed-off-by: Claude <noreply@anthropic.com>
Add a policy enforcement system that filters tool calls based on
configurable rules using Python expressions evaluated with simpleeval.
Features:
- Namespace allow/deny lists with glob pattern support
- Tool allow/deny lists with glob pattern support
- Custom rules with Python expressions
- Context-aware policies (team, role, etc.)
- Helper functions: match(), regex(), startswith(), endswith(), contains()
Configuration example:
```yaml
policy:
namespaces:
allow: ["team-a-*"]
deny: ["kube-system"]
rules:
- name: no-secrets-in-prod
match: ["kubectl_*"]
expression: 'not (params.get("namespace", "").startswith("prod") and params.get("kind") == "secret")'
```
Integration:
- PolicyEnforcer class in holmes/core/policy.py
- Hooked into tool_calling_llm.py:_directly_invoke_tool_call()
- PolicyConfig added to Config class
https://claude.ai/code/session_01ELTfzzxV3mBS2RzTAZiXeX
Signed-off-by: Claude <noreply@anthropic.com>
Replace confusing expression-must-be-true-to-allow with explicit:
- effect: allow/deny - what happens when condition matches
- when: expression - condition to evaluate
- default: allow/deny - fallback when no rule matches
Rules evaluated in order, first matching rule wins.
Example:
```yaml
policy:
default: deny
rules:
- name: deny-system-ns
effect: deny
when: 'params.get("namespace") in ["kube-system"]'
- name: allow-team-a
effect: allow
when: 'params.get("namespace", "").startswith("team-a-")'
```
https://claude.ai/code/session_01ELTfzzxV3mBS2RzTAZiXeX
Signed-off-by: Claude <noreply@anthropic.com>
Default is ALLOW everything. Users opt-in to restrictions by adding
deny rules. Much simpler mental model - no effect field needed.
Config format:
```yaml
policy:
deny:
- name: block-system-namespaces
match: ["kubectl_*"]
when: 'params.get("namespace") in ["kube-system"]'
message: "System namespaces are restricted"
- name: block-bash
match: ["bash/*"]
# no 'when' = always deny when tool matches
```
Semantics:
- If ANY deny rule matches (tool pattern + when condition) → DENY
- Otherwise → ALLOW
- Future: allow rules can be added with different semantics
https://claude.ai/code/session_01ELTfzzxV3mBS2RzTAZiXeX
Signed-off-by: Claude <noreply@anthropic.com>
Change from deny-only model to scoped validation semantics: - Tools matching NO rules → ALLOW (default open) - Tools matching one or more rules → ALL matching rules' 'when' must pass - If 'when' is omitted → treated as False (blocks matched tools) This provides intuitive allowlist-style patterns where rules define constraints that must be satisfied, rather than blocklist patterns. Rename DenyRule to PolicyRule and config.deny to config.rules. https://claude.ai/code/session_01ELTfzzxV3mBS2RzTAZiXeX Signed-off-by: Claude <noreply@anthropic.com>
The exploration document is no longer needed now that the implementation is complete. The code in holmes/core/policy.py is self-documenting. https://claude.ai/code/session_01ELTfzzxV3mBS2RzTAZiXeX Signed-off-by: Claude <noreply@anthropic.com>
Add `default` field to PolicyConfig: - `default: allow` (default) - unmatched tools allowed (blacklist mode) - `default: deny` - unmatched tools denied (whitelist mode) The `default` setting ONLY affects tools matching NO rules. When rules DO match, ALL matching rules' `when` conditions must still be True (AND semantics, unchanged). Omitting `when` still blocks matched tools regardless of default. https://claude.ai/code/session_01ELTfzzxV3mBS2RzTAZiXeX Signed-off-by: Claude <noreply@anthropic.com>
Breaking change: `when` is now required in policy rules. Use `when: "True"` to always allow matched tools, `when: "False"` to always block. This makes rule intent explicit and removes ambiguity. Add comprehensive policy filtering documentation at docs/reference/policy-filtering.md covering: - Configuration and semantics - Expression language and built-in functions - Examples: blacklist, whitelist, multi-tenant, layered - Helm configuration - Debugging tips https://claude.ai/code/session_01ELTfzzxV3mBS2RzTAZiXeX Signed-off-by: Claude <noreply@anthropic.com>
✅ Deploy Preview for holmes-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
WalkthroughAdds a new, configurable policy engine for rule-based filtering of tool calls, integrates lazy PolicyEnforcer initialization into Config, wires enforcement into ToolCallingLLM to short-circuit disallowed tool invocations, adds tests and documentation, and introduces a runtime dependency for sandboxed Python expression evaluation. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Config
participant PolicyEnforcer
participant ToolCallingLLM
participant Tool
User->>Config: Initialize with policy config
Config->>PolicyEnforcer: Create via lazy property (policy_enforcer)
ToolCallingLLM->>ToolCallingLLM: Receive policy_enforcer (global or injected)
User->>ToolCallingLLM: Request tool invocation
ToolCallingLLM->>PolicyEnforcer: check(tool_name, params, context)
rect rgba(200, 100, 100, 0.5)
PolicyEnforcer->>PolicyEnforcer: Match rules against tool_name
PolicyEnforcer->>PolicyEnforcer: Evaluate conditions (Python/Bash) in sandbox
PolicyEnforcer->>PolicyEnforcer: Check rate limits and helpers (env/http)
end
alt Policy allows
PolicyEnforcer-->>ToolCallingLLM: PolicyResult(allowed=true)
ToolCallingLLM->>Tool: Invoke tool with params
Tool-->>ToolCallingLLM: Return result
ToolCallingLLM-->>User: Return tool result
else Policy denies
PolicyEnforcer-->>ToolCallingLLM: PolicyResult(allowed=false, message)
ToolCallingLLM-->>User: Return denial StructuredToolResult
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@docs/reference/policy-filtering.md`:
- Around line 208-211: Update the fenced code block that contains the two
"Policy denied tool ..." log lines to include a language identifier (e.g.,
change the opening "```" to "```text" or "```log") so markdownlint MD040 is
satisfied; locate the block with the lines "Policy denied tool 'kubectl_get'..."
and "Policy denied tool 'bash/run_command'..." and replace the fence
accordingly.
In `@holmes/config.py`:
- Around line 125-129: The policy enforcer property (Config.policy_enforcer) is
never passed into ToolCallingLLM, so the global default enforcer stays
uninitialized and enforcement never runs; update each factory that constructs
ToolCallingLLM—create_console_toolcalling_llm, create_agui_toolcalling_llm,
create_toolcalling_llm, create_issue_investigator,
create_console_issue_investigator—to pass policy_enforcer=self.policy_enforcer
(or config.policy_enforcer) into the ToolCallingLLM(...) call so
ToolCallingLLM.__init__ receives the enforcer instead of relying on
get_policy_enforcer() returning None.
In `@holmes/core/policy.py`:
- Around line 224-226: The current logger.info call prints the full params dict
(logger.info(f"Policy denied tool '{tool_name}' with params {params}:
{message}")), which can expose sensitive values; change it to log only
non-sensitive information by either logging the param keys (e.g.,
list(params.keys())) or creating a redacted copy via a helper (e.g.,
redact_sensitive_params(params)) before inserting into the message, keeping the
same logger.info, tool_name and message variables but never interpolating raw
params values at INFO level.
- Around line 124-145: The SAFE_FUNCTIONS dict in holmes/core/policy.py exposes
getattr (and optionally hasattr) which can bypass simpleeval's AST protections;
remove getattr from SAFE_FUNCTIONS (and remove hasattr if you agree with the
reviewer) so attribute-access cannot be invoked from expressions, and update any
tests or callers that relied on SAFE_FUNCTIONS containing getattr/hasattr to use
safe, explicit helpers instead.
- Around line 208-215: The code builds the evaluation context by merging
rule.vars into names after setting "tool", "params", and "context", allowing
rule.vars to silently shadow those built-ins (seen in matching_rules loop and
the names dict using tool_name, params, context, **rule.vars); change this to
validate and prevent shadowing by detecting if any key in rule.vars conflicts
with the reserved keys ("tool", "params", "context") and either raise/log a
clear error or remove those keys from rule.vars before merging, ensuring
matching_rules processing does not allow rule.vars to override the actual tool
call data.
- Around line 147-163: The shared self._evaluator created in Policy.__init__ is
mutated during Policy.check() and causes a race when ToolCallingLLM runs
concurrent calls; to fix, stop using the shared evaluator and instead create a
fresh EvalWithCompoundTypes inside Policy.check() (copy the SAFE_FUNCTIONS and
helper functions onto that local evaluator and set its names before calling
eval), or alternatively protect the existing self._evaluator with a
threading.Lock around the code that mutates names and calls eval; update
Policy.check() (the method that sets evaluator.names and calls eval) to use the
per-call local evaluator or acquire/release the lock so concurrent
_directly_invoke_tool_call executions in ToolCallingLLM are safe.
In `@holmes/core/tool_calling_llm.py`:
- Around line 535-546: The policy_enforcer.check call in
_directly_invoke_tool_call is missing the context argument, so update the code
to pass a context dict into PolicyEnforcer.check (e.g.,
policy_enforcer.check(tool_name, tool_params, context)) and propagate that
context into _directly_invoke_tool_call's signature (and all callers) or
otherwise obtain a proper context object (not an empty dict) before calling;
ensure the unique symbols involved are _directly_invoke_tool_call and
policy_enforcer.check (and update callers of _directly_invoke_tool_call
accordingly) so context-based rules like context.get("role") work correctly.
🧹 Nitpick comments (2)
holmes/core/policy.py (1)
240-248: Uselogging.exceptionto preserve the traceback.Per Ruff TRY400,
logging.exceptionis preferred overlogging.errorwhen inside anexceptblock, as it automatically includes the traceback for debugging.Proposed fix
except Exception as e: - logger.error( + logger.exception( f"Policy rule '{rule.name}' evaluation failed: {e}. Denying by default." )tests/core/test_policy.py (1)
257-275: Consider adding a test forvarsshadowing built-in names (tool,params,context).If a rule defines
vars: {"params": "overridden"}, it would silently shadow the actual params dict. A test documenting the expected behavior (whether it's an error, warning, or intentional override) would clarify the contract.
Comment on lines
+208
to
+211
| ``` | ||
| Policy denied tool 'kubectl_get' with params {'namespace': 'kube-system'}: Only team-a namespaces are allowed | ||
| Policy denied tool 'bash/run_command': no matching rules (default: deny) | ||
| ``` |
Contributor
There was a problem hiding this comment.
Add a language identifier to the fenced code block.
The log output example is missing a language specifier, which triggers a markdownlint warning (MD040). Use log or text for non-code output.
Proposed fix
-```
+```text
Policy denied tool 'kubectl_get' with params {'namespace': 'kube-system'}: Only team-a namespaces are allowed
Policy denied tool 'bash/run_command': no matching rules (default: deny)
```📝 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.
Suggested change
| ``` | |
| Policy denied tool 'kubectl_get' with params {'namespace': 'kube-system'}: Only team-a namespaces are allowed | |
| Policy denied tool 'bash/run_command': no matching rules (default: deny) | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.20.0)
[warning] 208-208: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In `@docs/reference/policy-filtering.md` around lines 208 - 211, Update the fenced
code block that contains the two "Policy denied tool ..." log lines to include a
language identifier (e.g., change the opening "```" to "```text" or "```log") so
markdownlint MD040 is satisfied; locate the block with the lines "Policy denied
tool 'kubectl_get'..." and "Policy denied tool 'bash/run_command'..." and
replace the fence accordingly.
Comment on lines
+125
to
+129
| @property | ||
| def policy_enforcer(self) -> Optional[PolicyEnforcer]: | ||
| if self._policy_enforcer is None and self.policy is not None: | ||
| self._policy_enforcer = init_policy_from_config(self.policy) | ||
| return self._policy_enforcer |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check ToolCallingLLM.__init__ signature and policy_enforcer handling
rg -A 15 'class ToolCallingLLM' holmes/core/tool_calling_llm.pyRepository: HolmesGPT/holmesgpt
Length of output: 467
🏁 Script executed:
# Check all factory methods that create ToolCallingLLM
rg -B 3 -A 8 'return ToolCallingLLM\(' holmes/config.pyRepository: HolmesGPT/holmesgpt
Length of output: 1245
🏁 Script executed:
# Check if policy_enforcer property is accessed anywhere
rg -n '\.policy_enforcer' holmes/Repository: HolmesGPT/holmesgpt
Length of output: 354
🏁 Script executed:
# Check IssueInvestigator.__init__ signature
rg -A 10 'class IssueInvestigator' holmes/Repository: HolmesGPT/holmesgpt
Length of output: 695
🏁 Script executed:
# Search for any explicit calls to init_policy_from_config or config.policy_enforcer
rg -n 'init_policy_from_config|\.policy_enforcer' holmes/ --type pyRepository: HolmesGPT/holmesgpt
Length of output: 668
🏁 Script executed:
# Check what get_policy_enforcer() does and where the global is managed
rg -B 3 -A 8 'def get_policy_enforcer\|def set_policy_enforcer' holmes/Repository: HolmesGPT/holmesgpt
Length of output: 45
🏁 Script executed:
# Search for get_policy_enforcer definition and global policy management
rg -n 'def get_policy_enforcer|_policy_enforcer_global|GLOBAL.*POLICY' holmes/ --type pyRepository: HolmesGPT/holmesgpt
Length of output: 143
🏁 Script executed:
# Check holmes/core/policy.py for global management and init_policy_from_config
rg -A 20 'def init_policy_from_config' holmes/core/policy.pyRepository: HolmesGPT/holmesgpt
Length of output: 397
🏁 Script executed:
# Look for any module-level policy enforcer or global state
rg -B 3 -A 3 'get_policy_enforcer' holmes/core/tool_calling_llm.pyRepository: HolmesGPT/holmesgpt
Length of output: 634
🏁 Script executed:
# Confirm: search for any place where config.policy_enforcer is accessed
rg -n 'config\.policy_enforcer\|self\.policy_enforcer' holmes/ --type pyRepository: HolmesGPT/holmesgpt
Length of output: 45
🏁 Script executed:
# Check set_policy_enforcer to understand global state management
rg -B 2 -A 6 'def set_policy_enforcer' holmes/core/policy.pyRepository: HolmesGPT/holmesgpt
Length of output: 337
Policy enforcer is never wired into ToolCallingLLM — policy enforcement will not activate.
None of the factory methods (create_console_toolcalling_llm, create_agui_toolcalling_llm, create_toolcalling_llm, create_issue_investigator, create_console_issue_investigator) pass policy_enforcer to ToolCallingLLM. When ToolCallingLLM.__init__ receives no explicit enforcer, it falls back to get_policy_enforcer() (line 184), which returns None because the global _default_enforcer is never initialized—the Config.policy_enforcer lazy property that would set it is never accessed anywhere in the codebase.
Pass policy_enforcer=self.policy_enforcer in each factory method call to ToolCallingLLM(), or ensure the lazy property is triggered during config initialization.
Proposed fix: pass policy_enforcer in factory methods
Example for create_console_toolcalling_llm (apply to all factory methods):
return ToolCallingLLM(
tool_executor,
self.max_steps,
self._get_llm(tracer=tracer, model_key=model_name),
+ policy_enforcer=self.policy_enforcer,
)🤖 Prompt for AI Agents
In `@holmes/config.py` around lines 125 - 129, The policy enforcer property
(Config.policy_enforcer) is never passed into ToolCallingLLM, so the global
default enforcer stays uninitialized and enforcement never runs; update each
factory that constructs ToolCallingLLM—create_console_toolcalling_llm,
create_agui_toolcalling_llm, create_toolcalling_llm, create_issue_investigator,
create_console_issue_investigator—to pass policy_enforcer=self.policy_enforcer
(or config.policy_enforcer) into the ToolCallingLLM(...) call so
ToolCallingLLM.__init__ receives the enforcer instead of relying on
get_policy_enforcer() returning None.
Comment on lines
+147
to
+163
| def __init__(self, config: Optional[PolicyConfig] = None): | ||
| self.config = config or PolicyConfig() | ||
| self._evaluator = EvalWithCompoundTypes() | ||
|
|
||
| # Add safe functions | ||
| self._evaluator.functions.update(self.SAFE_FUNCTIONS) | ||
|
|
||
| # Add helper functions | ||
| self._evaluator.functions["match"] = lambda pattern, string: fnmatch.fnmatch( | ||
| string or "", pattern | ||
| ) | ||
| self._evaluator.functions["regex"] = lambda pattern, string: bool( | ||
| re.search(pattern, string or "") | ||
| ) | ||
| self._evaluator.functions["startswith"] = lambda s, prefix: (s or "").startswith(prefix) | ||
| self._evaluator.functions["endswith"] = lambda s, suffix: (s or "").endswith(suffix) | ||
| self._evaluator.functions["contains"] = lambda s, sub: sub in (s or "") |
Contributor
There was a problem hiding this comment.
Race condition: shared _evaluator instance is not thread-safe.
ToolCallingLLM executes tool calls concurrently via ThreadPoolExecutor (see tool_calling_llm.py line 451). Each concurrent call to _directly_invoke_tool_call may invoke self.policy_enforcer.check(), which mutates self._evaluator.names on line 219 before calling self._evaluator.eval() on line 220. A concurrent call can overwrite names between these two lines, causing one thread to evaluate its expression against the wrong tool/params.
Fix: create a new EvalWithCompoundTypes per check() invocation, or use a lock.
🔒 Proposed fix: create evaluator per-call
def __init__(self, config: Optional[PolicyConfig] = None):
self.config = config or PolicyConfig()
- self._evaluator = EvalWithCompoundTypes()
-
- # Add safe functions
- self._evaluator.functions.update(self.SAFE_FUNCTIONS)
-
- # Add helper functions
- self._evaluator.functions["match"] = lambda pattern, string: fnmatch.fnmatch(
- string or "", pattern
- )
- self._evaluator.functions["regex"] = lambda pattern, string: bool(
- re.search(pattern, string or "")
- )
- self._evaluator.functions["startswith"] = lambda s, prefix: (s or "").startswith(prefix)
- self._evaluator.functions["endswith"] = lambda s, suffix: (s or "").endswith(suffix)
- self._evaluator.functions["contains"] = lambda s, sub: sub in (s or "")
+ self._functions: dict = {}
+ self._functions.update(self.SAFE_FUNCTIONS)
+ self._functions["match"] = lambda pattern, string: fnmatch.fnmatch(
+ string or "", pattern
+ )
+ self._functions["regex"] = lambda pattern, string: bool(
+ re.search(pattern, string or "")
+ )
+ self._functions["startswith"] = lambda s, prefix: (s or "").startswith(prefix)
+ self._functions["endswith"] = lambda s, suffix: (s or "").endswith(suffix)
+ self._functions["contains"] = lambda s, sub: sub in (s or "")
+
+ def _create_evaluator(self, names: dict) -> EvalWithCompoundTypes:
+ """Create a new evaluator instance with the given names (thread-safe)."""
+ evaluator = EvalWithCompoundTypes()
+ evaluator.functions.update(self._functions)
+ evaluator.names = names
+ return evaluatorThen in check(), replace:
- self._evaluator.names = names
- condition_passed = self._evaluator.eval(rule.when)
+ evaluator = self._create_evaluator(names)
+ condition_passed = evaluator.eval(rule.when)🤖 Prompt for AI Agents
In `@holmes/core/policy.py` around lines 147 - 163, The shared self._evaluator
created in Policy.__init__ is mutated during Policy.check() and causes a race
when ToolCallingLLM runs concurrent calls; to fix, stop using the shared
evaluator and instead create a fresh EvalWithCompoundTypes inside Policy.check()
(copy the SAFE_FUNCTIONS and helper functions onto that local evaluator and set
its names before calling eval), or alternatively protect the existing
self._evaluator with a threading.Lock around the code that mutates names and
calls eval; update Policy.check() (the method that sets evaluator.names and
calls eval) to use the per-call local evaluator or acquire/release the lock so
concurrent _directly_invoke_tool_call executions in ToolCallingLLM are safe.
Comment on lines
+208
to
+215
| for rule in matching_rules: | ||
| # Build evaluation context | ||
| names = { | ||
| "tool": tool_name, | ||
| "params": params, | ||
| "context": context, | ||
| **rule.vars, | ||
| } |
Contributor
There was a problem hiding this comment.
Rule vars can silently shadow built-in expression variables.
The spread **rule.vars on line 214 is applied after tool, params, and context are set. If a rule defines vars: {"params": ...} or vars: {"tool": ...}, it would silently override the actual tool call data, leading to incorrect policy evaluation.
🛡️ Proposed fix: validate or warn on shadowing
+ _RESERVED_NAMES = {"tool", "params", "context"}
+
names = {
"tool": tool_name,
"params": params,
"context": context,
- **rule.vars,
}
+ for k, v in rule.vars.items():
+ if k in _RESERVED_NAMES:
+ logger.warning(
+ f"Policy rule '{rule.name}' var '{k}' shadows built-in name; ignoring"
+ )
+ else:
+ names[k] = v🤖 Prompt for AI Agents
In `@holmes/core/policy.py` around lines 208 - 215, The code builds the evaluation
context by merging rule.vars into names after setting "tool", "params", and
"context", allowing rule.vars to silently shadow those built-ins (seen in
matching_rules loop and the names dict using tool_name, params, context,
**rule.vars); change this to validate and prevent shadowing by detecting if any
key in rule.vars conflicts with the reserved keys ("tool", "params", "context")
and either raise/log a clear error or remove those keys from rule.vars before
merging, ensuring matching_rules processing does not allow rule.vars to override
the actual tool call data.
Comment on lines
+224
to
+226
| logger.info( | ||
| f"Policy denied tool '{tool_name}' with params {params}: {message}" | ||
| ) |
Contributor
There was a problem hiding this comment.
Logging full params dict may expose sensitive data.
The params dict could contain sensitive values (e.g., credentials, tokens, or PII passed as tool parameters). Logging the full dict at INFO level in production could be a compliance concern. Consider logging only param keys or redacting sensitive values.
Proposed fix
logger.info(
- f"Policy denied tool '{tool_name}' with params {params}: {message}"
+ f"Policy denied tool '{tool_name}' (params keys: {list(params.keys())}): {message}"
)📝 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.
Suggested change
| logger.info( | |
| f"Policy denied tool '{tool_name}' with params {params}: {message}" | |
| ) | |
| logger.info( | |
| f"Policy denied tool '{tool_name}' (params keys: {list(params.keys())}): {message}" | |
| ) |
🤖 Prompt for AI Agents
In `@holmes/core/policy.py` around lines 224 - 226, The current logger.info call
prints the full params dict (logger.info(f"Policy denied tool '{tool_name}' with
params {params}: {message}")), which can expose sensitive values; change it to
log only non-sensitive information by either logging the param keys (e.g.,
list(params.keys())) or creating a redacted copy via a helper (e.g.,
redact_sensitive_params(params)) before inserting into the message, keeping the
same logger.info, tool_name and message variables but never interpolating raw
params values at INFO level.
Comment on lines
+535
to
+546
| # Policy enforcement: check if tool call is allowed | ||
| if self.policy_enforcer: | ||
| policy_result = self.policy_enforcer.check(tool_name, tool_params) | ||
| if not policy_result.allowed: | ||
| logging.warning( | ||
| f"Policy denied tool '{tool_name}' with params {tool_params}: {policy_result.message}" | ||
| ) | ||
| return StructuredToolResult( | ||
| status=StructuredToolResultStatus.ERROR, | ||
| error=f"Policy denied: {policy_result.message}", | ||
| params=tool_params, | ||
| ) |
Contributor
There was a problem hiding this comment.
context is never passed to policy_enforcer.check(), breaking context-based rules.
The check() call on line 537 omits the context parameter. This means any policy rules that reference context.get("role"), context.get("team"), etc. — as documented and tested — will always see an empty dict, and the rules will not behave as expected.
You need to either:
- Plumb a
contextdict through to_directly_invoke_tool_call, or - Set a default context on the
PolicyEnforcerat initialization time.
🤖 Prompt for AI Agents
In `@holmes/core/tool_calling_llm.py` around lines 535 - 546, The
policy_enforcer.check call in _directly_invoke_tool_call is missing the context
argument, so update the code to pass a context dict into PolicyEnforcer.check
(e.g., policy_enforcer.check(tool_name, tool_params, context)) and propagate
that context into _directly_invoke_tool_call's signature (and all callers) or
otherwise obtain a proper context object (not an empty dict) before calling;
ensure the unique symbols involved are _directly_invoke_tool_call and
policy_enforcer.check (and update callers of _directly_invoke_tool_call
accordingly) so context-based rules like context.get("role") work correctly.
- Change policy rule syntax from `when:` to `allow_if:` with explicit
`python:` or `bash:` condition type
- Add bash command conditions with Jinja2-style templating for external
checks like Kubernetes RBAC verification via kubectl auth can-i
- Support template variables: {{ params.X }}, {{ context.X }}, {{ tool }}
- Support template filters: | default:"value", | quote
- Fix mypy type errors with proper annotations
- Update documentation with comprehensive examples
https://claude.ai/code/session_01ELTfzzxV3mBS2RzTAZiXeX
Signed-off-by: Claude <noreply@anthropic.com>
Auto-formatting changes from ruff, isort, and end-of-file fixer. https://claude.ai/code/session_01ELTfzzxV3mBS2RzTAZiXeX Signed-off-by: Claude <noreply@anthropic.com>
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
2 similar comments
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@docs/reference/policy-filtering.md`:
- Around line 80-100: The docs overstate available builtins; update the
policy-filtering.md Python Expression Language section to accurately reflect
that only the explicitly whitelisted functions in SAFE_FUNCTIONS (referenced in
policy.py) are available — replace "Standard Python functions are also
available... etc." with a clear statement listing or linking to SAFE_FUNCTIONS
and examples of common excluded builtins (e.g., print, open, type, range, map,
filter, zip, enumerate, round, hex) so users are not misled; ensure the doc
references the SAFE_FUNCTIONS symbol in policy.py for the authoritative list.
In `@holmes/core/policy.py`:
- Line 396: Remove the local import "import re as re_module" from inside the
_render_template function and use the module-level "re" already imported at the
top of the file (replace all uses of re_module with re); update the
_render_template function to reference re directly and delete the in-function
import to comply with the project's import guidelines.
In `@tests/core/test_policy.py`:
- Around line 39-49: Move the inline imports of pytest to the module top: add a
single "import pytest" at the top of the file, then remove the in-function
imports inside the test_neither_raises and test_both_raises functions (which
call AllowCondition); ensure no other inline pytest imports remain in those
tests.
🧹 Nitpick comments (3)
docs/reference/policy-filtering.md (1)
288-294: Consider removing the Security Considerations section.As per coding guidelines, security best practices sections should be skipped in MkDocs documentation — users are assumed to understand basics like least privilege and defense-in-depth. The content here (RBAC, sandboxing caveats,
default: deny) could be folded into the relevant configuration/examples sections as brief inline notes instead.holmes/core/policy.py (2)
377-466: Consider using the real Jinja2 library instead of this hand-rolled template engine.Jinja2 is already a project dependency (v3.1.2). The custom template parser here (~90 lines) supports a subset of Jinja2 syntax with bespoke filter handling. Using
jinja2.Environmentwith a sandboxed environment would be more robust, better-tested, and support the full filter/expression syntax users might expect from "Jinja2-style templating."
476-499: Global enforcer state lacks thread-safety documentation.
_default_enforcer,set_policy_enforcer, andget_policy_enforceruse unsynchronized module-level global state. This is likely fine since the enforcer is set once during startup, but a brief docstring note would prevent future misuse (e.g., dynamic policy reloading in a multi-threaded context).
Comment on lines
+80
to
+100
| ## Python Expression Language | ||
|
|
||
| ### Available Variables | ||
|
|
||
| | Variable | Description | | ||
| |----------|-------------| | ||
| | `tool` | Name of the tool being called | | ||
| | `params` | Dictionary of parameters passed to the tool | | ||
| | `context` | Additional context (user, team, etc.) | | ||
|
|
||
| ### Built-in Functions | ||
|
|
||
| | Function | Description | | ||
| |----------|-------------| | ||
| | `match(pattern, string)` | Glob pattern matching (fnmatch) | | ||
| | `regex(pattern, string)` | Regular expression matching | | ||
| | `startswith(s, prefix)` | String prefix check | | ||
| | `endswith(s, suffix)` | String suffix check | | ||
| | `contains(s, sub)` | Substring check | | ||
|
|
||
| Standard Python functions are also available: `len`, `str`, `int`, `bool`, `list`, `dict`, `any`, `all`, `min`, `max`, etc. |
Contributor
There was a problem hiding this comment.
Documentation states standard Python functions are available but some are actually restricted.
Line 100 says "Standard Python functions are also available: len, str, int, bool, list, dict, any, all, min, max, etc." — however, looking at policy.py, the available functions are explicitly limited to SAFE_FUNCTIONS. Functions like print, open, type, range, map, filter, zip, enumerate, round, hex, etc. are not available. The wording "Standard Python functions" with trailing "etc." overpromises and will confuse users who try other builtins.
Proposed fix
-Standard Python functions are also available: `len`, `str`, `int`, `bool`, `list`, `dict`, `any`, `all`, `min`, `max`, etc.
+Additional safe functions are available: `len`, `str`, `int`, `float`, `bool`, `list`, `dict`, `set`, `tuple`, `abs`, `min`, `max`, `sum`, `sorted`, `any`, `all`, `isinstance`, `hasattr`, `getattr`.📝 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.
Suggested change
| ## Python Expression Language | |
| ### Available Variables | |
| | Variable | Description | | |
| |----------|-------------| | |
| | `tool` | Name of the tool being called | | |
| | `params` | Dictionary of parameters passed to the tool | | |
| | `context` | Additional context (user, team, etc.) | | |
| ### Built-in Functions | |
| | Function | Description | | |
| |----------|-------------| | |
| | `match(pattern, string)` | Glob pattern matching (fnmatch) | | |
| | `regex(pattern, string)` | Regular expression matching | | |
| | `startswith(s, prefix)` | String prefix check | | |
| | `endswith(s, suffix)` | String suffix check | | |
| | `contains(s, sub)` | Substring check | | |
| Standard Python functions are also available: `len`, `str`, `int`, `bool`, `list`, `dict`, `any`, `all`, `min`, `max`, etc. | |
| ## Python Expression Language | |
| ### Available Variables | |
| | Variable | Description | | |
| |----------|-------------| | |
| | `tool` | Name of the tool being called | | |
| | `params` | Dictionary of parameters passed to the tool | | |
| | `context` | Additional context (user, team, etc.) | | |
| ### Built-in Functions | |
| | Function | Description | | |
| |----------|-------------| | |
| | `match(pattern, string)` | Glob pattern matching (fnmatch) | | |
| | `regex(pattern, string)` | Regular expression matching | | |
| | `startswith(s, prefix)` | String prefix check | | |
| | `endswith(s, suffix)` | String suffix check | | |
| | `contains(s, sub)` | Substring check | | |
| Additional safe functions are available: `len`, `str`, `int`, `float`, `bool`, `list`, `dict`, `set`, `tuple`, `abs`, `min`, `max`, `sum`, `sorted`, `any`, `all`, `isinstance`, `hasattr`, `getattr`. |
🤖 Prompt for AI Agents
In `@docs/reference/policy-filtering.md` around lines 80 - 100, The docs overstate
available builtins; update the policy-filtering.md Python Expression Language
section to accurately reflect that only the explicitly whitelisted functions in
SAFE_FUNCTIONS (referenced in policy.py) are available — replace "Standard
Python functions are also available... etc." with a clear statement listing or
linking to SAFE_FUNCTIONS and examples of common excluded builtins (e.g., print,
open, type, range, map, filter, zip, enumerate, round, hex) so users are not
misled; ensure the doc references the SAFE_FUNCTIONS symbol in policy.py for the
authoritative list.
Comment on lines
+340
to
+348
|
|
||
| try: | ||
| result = subprocess.run( | ||
| command, | ||
| shell=True, | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=10, # 10 second timeout | ||
| ) |
Contributor
There was a problem hiding this comment.
Command injection risk: bash template values are not quoted by default.
subprocess.run(..., shell=True) executes the rendered template as a shell command. Template substitution (e.g., {{ params.kind }}) injects values directly into the command string. The | quote filter exists but is opt-in. If a user writes:
bash: 'kubectl auth can-i get {{ params.kind }}'and params.kind is "pods; curl attacker.com", the injected command runs unescaped. Consider auto-quoting all template values by default and requiring an explicit | raw or | noquote filter to opt out, rather than the current opt-in | quote.
🧰 Tools
🪛 Ruff (0.14.14)
[error] 342-342: subprocess call with shell=True identified, security issue
(S602)
| - {{ params.key | default:"value" }} - with default | ||
| - {{ params.key | quote }} - shell-escaped | ||
| """ | ||
| import re as re_module |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Move import re to the top of the file — re is already imported at line 60.
Line 396 re-imports re as re_module inside _render_template. Since re is already imported at the module level (line 60) with no name collision, use the existing import directly.
Proposed fix
- import re as re_module
-
- def get_value(path: str, default: Optional[str] = None) -> Any:
+ def get_value(path: str, default: Optional[str] = None) -> Any:Then replace all re_module references with re:
- def replace_match(m: re_module.Match) -> str:
+ def replace_match(m: re.Match) -> str:- return re_module.sub(pattern, replace_match, template)
+ return re.sub(pattern, replace_match, template)As per coding guidelines, **/*.py: "ALWAYS place Python imports at the top of the file, not inside functions or methods".
🤖 Prompt for AI Agents
In `@holmes/core/policy.py` at line 396, Remove the local import "import re as
re_module" from inside the _render_template function and use the module-level
"re" already imported at the top of the file (replace all uses of re_module with
re); update the _render_template function to reference re directly and delete
the in-function import to comply with the project's import guidelines.
Comment on lines
+39
to
+49
| def test_neither_raises(self): | ||
| import pytest | ||
|
|
||
| with pytest.raises(ValueError, match="exactly one"): | ||
| AllowCondition() | ||
|
|
||
| def test_both_raises(self): | ||
| import pytest | ||
|
|
||
| with pytest.raises(ValueError, match="exactly one"): | ||
| AllowCondition(python="True", bash="echo ok") |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Move import pytest to the top of the file.
pytest is imported inside test_neither_raises (line 40) and test_both_raises (line 47). Per coding guidelines, Python imports must always be placed at the top of the file, not inside functions or methods.
Proposed fix
Add at top of file (after line 1):
import pytestThen remove the inline imports:
def test_neither_raises(self):
- import pytest
-
with pytest.raises(ValueError, match="exactly one"):
AllowCondition()
def test_both_raises(self):
- import pytest
-
with pytest.raises(ValueError, match="exactly one"):
AllowCondition(python="True", bash="echo ok")As per coding guidelines, **/*.py: "ALWAYS place Python imports at the top of the file, not inside functions or methods".
🤖 Prompt for AI Agents
In `@tests/core/test_policy.py` around lines 39 - 49, Move the inline imports of
pytest to the module top: add a single "import pytest" at the top of the file,
then remove the in-function imports inside the test_neither_raises and
test_both_raises functions (which call AllowCondition); ensure no other inline
pytest imports remain in those tests.
- Add http_get() and http_post() functions for making HTTP requests in Python policy expressions - Add env() function for accessing environment variables - Support basic auth (tuple) and bearer token (string) authentication - Add comprehensive tests for HTTP helpers - Update documentation with Confluence access control examples showing both Python (with http_get/http_post) and Bash (with curl) alternatives https://claude.ai/code/session_01ELTfzzxV3mBS2RzTAZiXeX Signed-off-by: Claude <noreply@anthropic.com>
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
3 similar comments
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/reference/policy-filtering.md`:
- Around line 222-234: The Python example under the allow_if python block uses
the walrus operator (:=) which simpleeval doesn't support; replace the inline
walrus usage by assigning account separately before the permission check: call
http_get(...) and store its .get(...)[0].get("accountId") into an account
variable, then call http_post(...) using that account to evaluate
.get("hasPermission", False); update the allow_if python block to perform these
two sequential operations (assign account, then call http_post) and remove the
walrus expression so simpleeval can evaluate it.
🧹 Nitpick comments (3)
holmes/core/policy.py (2)
520-622: HTTP helpers silently return{}on failure — policy expressions may misinterpret this.Both
_http_getand_http_postreturn{}on any error (network failure, 4xx/5xx, non-JSON response). A policy expression likehttp_get(...).get("hasPermission", False)will evaluate toFalseon network error, which denies the tool call. This is fail-safe (good), but the user gets no indication that the denial was due to a network error vs. an actual permission denial. Consider having the error path set a distinguishable marker or at least log at a higher level when these are used within policy evaluation.
625-648: Global mutable singleton_default_enforceris not thread-safe.
set_policy_enforcerandget_policy_enforcerread/write a module-level global without synchronization. In practice this is likely only set at startup, but ifinit_policy_from_configis ever called concurrently (e.g., config reload), it could race. Consider using athreading.Lockor documenting that it must only be called during single-threaded initialization.docs/reference/policy-filtering.md (1)
371-377: Consider removing the Security Considerations section.As per coding guidelines for
docs/**/*.md: "skip Security Best Practices sections - assume users understand basics like rotating credentials, using least privilege, and deleting local secrets."That said, some of these points (e.g., sandboxing scope,
quotefilter for bash) are specific to this feature and may be worth keeping in a more concise form — use your judgment.
- Add RateLimitConfig with sliding window, max_total, and max_per group - RateLimitTracker provides thread-safe in-memory call tracking - Rate limits compose with allow_if: denied calls don't count against limits - Support duration strings: "30s", "5m", "1h", "1d", "1h30m" - Group by any dotted path: params.namespace, context.cluster, etc. - Each rule tracks counters independently - Add 26 new tests covering tracker, config validation, and integration - Update documentation with rate limiting examples https://claude.ai/code/session_01ELTfzzxV3mBS2RzTAZiXeX Signed-off-by: Claude <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
holmes/core/policy.py (2)
504-512: Uselogger.exceptioninstead oflogger.errorto preserve tracebacks.Per Ruff TRY400,
logger.exceptionautomatically includes the traceback. Applies to lines 505, 535, 564, and 571.- logger.error( + logger.exception( f"Policy rule '{rule.name}' evaluation failed: {e}. Denying by default." )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/core/policy.py` around lines 504 - 512, Replace the logger.error calls inside the exception handlers in the policy evaluation code with logger.exception so tracebacks are preserved; specifically update the except blocks that log "Policy rule '{rule.name}' evaluation failed..." (and the other similar handlers referenced around rule.name and PolicyResult creation) to call logger.exception with the same message before returning the PolicyResult, keeping the existing message text and parameters unchanged so the traceback is included for debugging.
345-365: AnnotateSAFE_FUNCTIONSwithClassVarto satisfy Ruff RUF012.+ from typing import ClassVar ... - SAFE_FUNCTIONS = { + SAFE_FUNCTIONS: ClassVar[dict[str, Any]] = {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/core/policy.py` around lines 345 - 365, Annotate the SAFE_FUNCTIONS class variable with ClassVar to satisfy RUF012: import ClassVar (and typing helpers like Dict, Callable, Any if not present) and change the declaration of SAFE_FUNCTIONS to include a ClassVar type hint (e.g., SAFE_FUNCTIONS: ClassVar[Dict[str, Callable[..., Any]]]) inside the Policy class so the linter recognizes it as a class-level constant.docs/reference/policy-filtering.md (1)
465-471: Consider removing the "Security Considerations" section.As per coding guidelines: "Skip 'Security Best Practices' sections in documentation. Assume users understand basics like rotating credentials, using least privilege, and deleting local secrets. These sections add little value." Most of the points here (RBAC defense-in-depth, default deny, input validation) are already covered contextually in the examples above.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/policy-filtering.md` around lines 465 - 471, Remove the entire "## Security Considerations" section (the header and its bullet list) from the policy-filtering documentation; delete the block containing the header text "Security Considerations" and the four bullets about enforcement level, RBAC on the Holmes ServiceAccount, sandboxed Python expressions, Bash input validation via the `quote` filter, and the "default: deny" recommendation so the doc omits the redundant security-best-practices section already covered elsewhere.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@holmes/core/policy.py`:
- Around line 729-741: _policy.py currently exposes all environment variables
via the static helper _get_env (used by env() in policy expressions), allowing
policies to read secrets; restrict this by adding an allowlist mechanism:
introduce configurable allowed_env_names and allowed_env_prefixes checked inside
_get_env (or route env() calls through a new sanitizer) so only matching names
are returned, and update _get_env to log/raise when access is denied; also add a
clear comment/docstring near _get_env and the env() entrypoint documenting the
trust boundary and how to configure the allowlist._get_env and any env() wrapper
are the symbols to modify._
- Around line 744-791: _http_get (and the sibling http_post) currently allow
requests to arbitrary URLs which creates SSRF risk; add hostname/IP validation
that blocks requests to well-known metadata hosts (e.g., 169.254.169.254,
metadata.google.internal), RFC1918/private ranges, link-local addresses and
localhost before making the requests. Implement this by resolving the target
host to one or more IPs using socket.getaddrinfo (or ipaddress.ip_address) and
checking each IP against private/link-local/loopback networks, and deny the
request (log and return {} or raise a controlled exception) if any resolved IP
is in a blocked range; apply the same validation for direct IP URLs and for
http_post to ensure consistent protection. Ensure the checks run early in
_http_get and http_post and that Authorization handling and error logging remain
unchanged.
---
Duplicate comments:
In `@docs/reference/policy-filtering.md`:
- Around line 453-456: The fenced code block containing the log output starting
with "Policy denied tool 'kubectl_get'..." should include a language identifier
to satisfy markdownlint MD040; change the opening fence from ``` to ```text so
the block becomes a plain-text code block and renders as log output.
- Line 113: The docs overclaim available builtins; change the sentence that
lists "Standard Python functions..." to either enumerate the exact functions
from SAFE_FUNCTIONS or replace it with a direct link/reference to where
SAFE_FUNCTIONS is defined (so users know the exact allowed set). Locate the
mention in the policy-filtering documentation and update the wording to remove
"etc." and point readers to the SAFE_FUNCTIONS constant or its source file (or
paste the exact list) so only the explicitly allowed functions are shown.
- Around line 256-267: The Python example uses the walrus operator (account :=
http_get(...)) which simpleeval cannot parse (raises FeatureNotAvailable);
update the snippet to avoid the walrus by performing the http_get and assigning
its result to account in a separate expression (call
http_get(...).get("results", [{}])[0].get("accountId") into account before the
and-check) or else replace the example with the Bash/Option B alternative; refer
to the python block, the http_get call and the account variable when making the
change and ensure no use of := remains so simpleeval can evaluate it.
In `@holmes/core/policy.py`:
- Line 650: In _render_template remove the inline "import re as re_module" and
replace all uses of re_module with the top-level re import (the file already
imports re); update any references inside the _render_template function to call
re directly and delete the redundant local import to comply with the
module-level import guideline.
- Around line 542-549: The subprocess.run call uses shell=True with a composed
command string in the variable command, which allows command injection when
template variables are user-controlled; fix by either building an args list and
calling subprocess.run with shell=False (pass the command parts as a list) or
ensure every substituted template value is safely quoted using shlex.quote at
the point where the command string is assembled (or apply an automatic quote
filter to template rendering), and keep using the same identifiers (the command
variable and the subprocess.run call) so the fix replaces the unsafe string
invocation with a safely quoted or list-based invocation.
- Around line 490-492: The current logger.info call logs the full params dict
(variable params) which may leak secrets; update the logging in the Policy
decision path (the logger.info that mentions tool_name and message) to log only
parameter keys or a redacted summary (e.g., list(params.keys()) or masked
placeholders) instead of values, preserving tool_name and message but never
serializing param values; ensure you update the specific logger.info invocation
that reports "Policy denied tool" so only safe metadata is emitted.
- Around line 484-486: The shared self._evaluator is mutated concurrently in
_evaluate_python (setting self._evaluator.names then calling
self._evaluator.eval), causing a race when ToolCallingLLM uses
ThreadPoolExecutor; fix by not mutating the shared evaluator: either create a
fresh EvalWithCompoundTypes instance per call (instantiate a new
EvalWithCompoundTypes with the same config, set its names and call its eval)
inside _evaluate_python, or protect all accesses to self._evaluator with a
dedicated threading.Lock around setting names and calling eval; update
_evaluate_python to use one of these approaches and reference the existing
self._evaluator, _evaluate_python, and EvalWithCompoundTypes symbols when making
the change.
- Around line 362-364: SAFE_FUNCTIONS still exposes introspection helpers
(getattr, hasattr, isinstance), which are sandbox escape vectors; remove these
from the SAFE_FUNCTIONS mapping so they cannot be used in untrusted policy
evaluation. Locate the SAFE_FUNCTIONS definition in policy.py and delete or
comment out the entries for "getattr", "hasattr", and "isinstance" (or replace
them with safe, limited wrappers if specific safe behavior is required),
ensuring any code paths that previously relied on
SAFE_FUNCTIONS["getattr"/"hasattr"/"isinstance"] are updated to avoid using
them.
- Around line 477-482: The current construction of names allows rule.vars to
silently overwrite the reserved keys ("tool", "params", "context"); detect any
conflicts between rule.vars.keys() and the reserved keys and fail fast (raise a
clear exception including the conflicting keys and the rule identifier) or
alternatively enforce precedence by merging as {**rule.vars, "tool": tool_name,
"params": params, "context": context} so the explicit values (tool_name, params,
context) cannot be overridden; update the code around the names dict (the names
assignment that uses rule.vars) to implement one of these fixes and include a
helpful error message referencing the rule if you choose the validation
approach.
In `@tests/core/test_policy.py`:
- Around line 45-55: Add a single top-level import for pytest (near the other
module imports) and remove the inline "import pytest" statements from the test
functions; specifically delete the inline imports from test_neither_raises and
test_both_raises (and the other test functions that currently import pytest
inline) so all tests use the module-level pytest import.
---
Nitpick comments:
In `@docs/reference/policy-filtering.md`:
- Around line 465-471: Remove the entire "## Security Considerations" section
(the header and its bullet list) from the policy-filtering documentation; delete
the block containing the header text "Security Considerations" and the four
bullets about enforcement level, RBAC on the Holmes ServiceAccount, sandboxed
Python expressions, Bash input validation via the `quote` filter, and the
"default: deny" recommendation so the doc omits the redundant
security-best-practices section already covered elsewhere.
In `@holmes/core/policy.py`:
- Around line 504-512: Replace the logger.error calls inside the exception
handlers in the policy evaluation code with logger.exception so tracebacks are
preserved; specifically update the except blocks that log "Policy rule
'{rule.name}' evaluation failed..." (and the other similar handlers referenced
around rule.name and PolicyResult creation) to call logger.exception with the
same message before returning the PolicyResult, keeping the existing message
text and parameters unchanged so the traceback is included for debugging.
- Around line 345-365: Annotate the SAFE_FUNCTIONS class variable with ClassVar
to satisfy RUF012: import ClassVar (and typing helpers like Dict, Callable, Any
if not present) and change the declaration of SAFE_FUNCTIONS to include a
ClassVar type hint (e.g., SAFE_FUNCTIONS: ClassVar[Dict[str, Callable[...,
Any]]]) inside the Policy class so the linter recognizes it as a class-level
constant.
Comment on lines
+729
to
+741
| @staticmethod | ||
| def _get_env(name: str, default: str = "") -> str: | ||
| """ | ||
| Get environment variable value. | ||
|
|
||
| Args: | ||
| name: Environment variable name | ||
| default: Default value if not set | ||
|
|
||
| Returns: | ||
| Environment variable value or default | ||
| """ | ||
| return os.environ.get(name, default) |
Contributor
There was a problem hiding this comment.
env() exposes all environment variables to policy expressions.
Policy expressions can access any environment variable via env(), including secrets injected as env vars (e.g., database credentials, API tokens). This is acceptable if policy authors are fully trusted, but worth documenting the trust boundary explicitly. Consider supporting an allowlist of accessible env var names/prefixes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@holmes/core/policy.py` around lines 729 - 741, _policy.py currently exposes
all environment variables via the static helper _get_env (used by env() in
policy expressions), allowing policies to read secrets; restrict this by adding
an allowlist mechanism: introduce configurable allowed_env_names and
allowed_env_prefixes checked inside _get_env (or route env() calls through a new
sanitizer) so only matching names are returned, and update _get_env to log/raise
when access is denied; also add a clear comment/docstring near _get_env and the
env() entrypoint documenting the trust boundary and how to configure the
allowlist._get_env and any env() wrapper are the symbols to modify._
Comment on lines
+744
to
+791
| def _http_get( | ||
| url: str, | ||
| params: Optional[Dict[str, Any]] = None, | ||
| headers: Optional[Dict[str, str]] = None, | ||
| auth: Optional[Union[Tuple[str, str], str]] = None, | ||
| timeout: int = 10, | ||
| ) -> Dict[str, Any]: | ||
| """ | ||
| Make an HTTP GET request and return JSON response. | ||
|
|
||
| Args: | ||
| url: URL to request | ||
| params: Query parameters | ||
| headers: Request headers | ||
| auth: Authentication tuple (username, password) or bearer token string | ||
| timeout: Request timeout in seconds | ||
|
|
||
| Returns: | ||
| Parsed JSON response as dict, or empty dict on error | ||
|
|
||
| Example: | ||
| http_get("https://api.example.com/user", params={"email": "user@example.com"}) | ||
| """ | ||
| try: | ||
| request_headers = headers or {} | ||
| request_auth = None | ||
|
|
||
| # Handle auth - tuple for basic auth, string for bearer token | ||
| if isinstance(auth, tuple): | ||
| request_auth = auth | ||
| elif isinstance(auth, str): | ||
| request_headers["Authorization"] = f"Bearer {auth}" | ||
|
|
||
| response = requests.get( | ||
| url, | ||
| params=params, | ||
| headers=request_headers, | ||
| auth=request_auth, | ||
| timeout=timeout, | ||
| ) | ||
| response.raise_for_status() | ||
| return response.json() | ||
| except requests.exceptions.RequestException as e: | ||
| logger.warning(f"HTTP GET failed for {url}: {e}") | ||
| return {} | ||
| except json.JSONDecodeError as e: | ||
| logger.warning(f"HTTP GET response not JSON for {url}: {e}") | ||
| return {} |
Contributor
There was a problem hiding this comment.
SSRF risk: http_get/http_post allow requests to arbitrary URLs including internal endpoints.
Policy expressions can call http_get("http://169.254.169.254/latest/meta-data/...") to access cloud metadata services, internal APIs, or localhost services. Consider:
- Adding a URL allowlist or blocklist (e.g., deny RFC 1918 ranges, link-local addresses)
- Documenting the trust boundary — policy authors have equivalent access to the process's network
- At minimum, blocking well-known metadata endpoints (
169.254.169.254,metadata.google.internal, etc.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@holmes/core/policy.py` around lines 744 - 791, _http_get (and the sibling
http_post) currently allow requests to arbitrary URLs which creates SSRF risk;
add hostname/IP validation that blocks requests to well-known metadata hosts
(e.g., 169.254.169.254, metadata.google.internal), RFC1918/private ranges,
link-local addresses and localhost before making the requests. Implement this by
resolving the target host to one or more IPs using socket.getaddrinfo (or
ipaddress.ip_address) and checking each IP against private/link-local/loopback
networks, and deny the request (log and return {} or raise a controlled
exception) if any resolved IP is in a blocked range; apply the same validation
for direct IP URLs and for http_post to ensure consistent protection. Ensure the
checks run early in _http_get and http_post and that Authorization handling and
error logging remain unchanged.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR introduces a comprehensive policy-based filtering system to HolmesGPT that enables security-conscious deployments to control which tools can be called and with what parameters. The system uses Python expressions evaluated in a sandboxed environment to define flexible access control rules.
Key Changes
New
holmes/core/policy.pymodule: Implements the core policy engine with:PolicyConfig: Configuration model for policy rulesPolicyRule: Individual rule definition with pattern matching and conditional expressionsPolicyEnforcer: Evaluates policy rules against tool calls usingsimpleevalPolicyResult: Result object indicating whether a tool call is alloweddefault: allow) and whitelist mode (default: deny)Policy integration into
holmes/config.py:policyfield toConfigclasspolicy_enforcerproperty that lazily initializes from configTool call enforcement in
holmes/core/tool_calling_llm.py:policy_enforcerparameter toToolCallingLLM.__init___directly_invoke_tool_callComprehensive documentation (
docs/reference/policy-filtering.md):Extensive test coverage (
tests/core/test_policy.py):Dependencies: Added
simpleeval ^1.0.0topyproject.tomlfor safe expression evaluationImplementation Details
Policy Semantics
defaultsettingwhenconditions must evaluate toTrue(AND semantics)tool,params,context, rulevars, and safe built-in functionsmatch()(fnmatch),regex(),startswith(),endswith(),contains()Security Features
simpleevalConfiguration Examples
Blacklist mode (default allow, block specific tools):
Whitelist mode (default deny, allow specific tools):
https://claude.ai/code/session_01ELTfzzxV3mBS2RzTAZiXeX
Summary by CodeRabbit
New Features
Documentation
Tests
Chores