Implemented cloud shell policy, which overrides local policy#73
Implemented cloud shell policy, which overrides local policy#73zmaj1231 wants to merge 1 commit into
Conversation
Greptile SummaryThis PR makes Reins Cloud (
Confidence Score: 2/5Not safe to merge as-is — the cloud policy path short-circuits destructive gating and irreversibility checks for all shell commands, and an empty policies.json disables local shell enforcement entirely. The shell interceptor now unconditionally returns early whenever a cloud policy cache is loaded, bypassing every downstream safety mechanism (destructive classification, irreversibility scoring, memory risk, rate limiting). A minimal or empty policies.json — whether the result of a sync failure, a deliberate erasure, or a bug in the cloud-side writer — turns off all of those protections silently. Combined with the unanchored MCP regex that can block unintended tools, these are present defects in the changed paths that need to be resolved before this code should run in production. src/plugin/tool-interceptor.ts and src/plugin/cloud-policy.ts require the most attention; the early-return logic and the empty-cache promotion are the root causes of the main issues. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[before_tool_call event] --> B[Load cloud policy cache\npolicies.json]
B --> C{Cloud policy\nexists?}
C -- No --> D[Use local Interceptor\nevaluate]
C -- Yes\nShell/bash/exec --> E{Cloud shell rule\nmatches?}
C -- Yes\nAny other tool --> F{Cloud MCP rule\nmatches?}
E -- BLOCK --> G[Return block=true\nLog BLOCKED]
E -- WARN --> H[Log warning\nLog ALLOWED\nReturn early]
E -- No match --> I[Log ALLOWED\nReturn early]
F -- BLOCK --> J[Return block=true\nLog BLOCKED]
F -- WARN / LOG --> K[Log ALLOWED\nReturn early]
F -- No match --> L[Continue to full\nlocal pipeline]
D --> L
L --> M[Irreversibility scoring]
M --> N[Destructive classification]
N --> O[Memory risk forecasting]
O --> P[Rate limiter check]
P --> Q[interceptor.evaluate]
Q --> R{Allowed?}
R -- Yes --> S[Return allowed]
R -- No --> T[Return block=true]
Reviews (1): Last reviewed commit: "Implemented cloud shell policy, which ov..." | Re-trigger Greptile |
| if ( | ||
| cloudPolicy && | ||
| moduleName === 'Shell' && | ||
| ['bash', 'exec'].includes(methodName) | ||
| ) { | ||
| const command = typeof params.command === 'string' ? params.command : ''; | ||
| const matchedRule = evaluateCloudShellRule(command, cloudPolicy); | ||
|
|
||
| if (matchedRule?.action === 'BLOCK') { | ||
| const reason = | ||
| `Reins Cloud policy blocked shell command. ` + | ||
| `${matchedRule.description || 'No description provided.'} ` + | ||
| `[rule: ${matchedRule.rule}; severity: ${matchedRule.severity || 'unknown'}]`; | ||
| await DecisionLog.append({ | ||
| timestamp: new Date().toISOString(), | ||
| module: moduleName, | ||
| method: methodName, | ||
| args: [params], | ||
| decision: 'BLOCKED', | ||
| decisionTime: 0, | ||
| reason, | ||
| eventType: 'tool_blocked', | ||
| tool: toolName, | ||
| }); | ||
| return { block: true, blockReason: reason }; | ||
| } | ||
|
|
||
| if (matchedRule?.action === 'WARN') { | ||
| logger.warn('Reins Cloud shell rule matched with WARN action', { | ||
| toolName, | ||
| command, | ||
| rule: matchedRule.rule, | ||
| severity: matchedRule.severity, | ||
| }); | ||
| } | ||
|
|
||
| await DecisionLog.append({ | ||
| timestamp: new Date().toISOString(), | ||
| module: moduleName, | ||
| method: methodName, | ||
| args: [params], | ||
| decision: 'ALLOWED', | ||
| decisionTime: 0, | ||
| reason: matchedRule | ||
| ? 'allowed by Reins Cloud shell policy cache' | ||
| : 'allowed by Reins Cloud shell policy cache (no matching block rule)', | ||
| eventType: 'tool_executed', | ||
| tool: toolName, | ||
| }); | ||
| return shouldReturnParams ? { params } : {}; | ||
| } |
There was a problem hiding this comment.
Shell safety pipeline bypassed when cloud policy is active
When cloudPolicy is loaded and the tool is a shell command, the hook always returns at line 179 — before irreversibility scoring, destructive gating, memory risk forecasting, and rate-limiting logic run. This means any bash or exec call that doesn't match a BLOCK rule (including all WARN/LOG rule hits and zero-rule matches) skips every downstream safety check. A policies.json with an empty shell_rules array effectively disables catastrophic-action gating for all shell commands.
| export function loadCloudPolicyCache(): CloudPolicyCache | null { | ||
| try { | ||
| const raw = readFileSync(getDataPath('policies.json'), 'utf8'); | ||
| const parsed = JSON.parse(raw) as unknown; | ||
| if (!parsed || typeof parsed !== 'object') { | ||
| return null; | ||
| } | ||
|
|
||
| const record = parsed as Record<string, unknown>; | ||
| return { | ||
| shell_rules: Array.isArray(record['shell_rules']) ? (record['shell_rules'] as ShellRule[]) : [], | ||
| mcp_rules: Array.isArray(record['mcp_rules']) ? (record['mcp_rules'] as McpRule[]) : [], | ||
| updated_at: typeof record['updated_at'] === 'string' ? record['updated_at'] : undefined, | ||
| }; | ||
| } catch { | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
Empty or minimal
policies.json silently disables local shell enforcement
loadCloudPolicyCache returns a non-null CloudPolicyCache — with empty arrays — when the file contains {} or any valid JSON object with no recognised keys. The caller in tool-interceptor.ts treats any non-null return as authoritative cloud policy, causing all shell commands to return early without going through the local interceptor. A corrupt, zero-byte (invalid JSON is caught, but {} is not), or deliberately emptied policies.json will fully bypass the local DENY/allow-path rules configured by the operator.
| export function evaluateCloudMcpRule(toolName: string, cloudPolicy: CloudPolicyCache): CloudRuleMatch | null { | ||
| for (const rule of cloudPolicy.mcp_rules) { | ||
| try { | ||
| const matches = | ||
| toolName.startsWith(rule.tool_pattern) || | ||
| new RegExp(rule.tool_pattern, 'i').test(toolName); | ||
| if (matches) { | ||
| return { | ||
| action: rule.action, | ||
| severity: rule.severity, | ||
| rule: rule.tool_pattern, | ||
| description: rule.description, | ||
| }; | ||
| } | ||
| } catch { | ||
| // Ignore invalid regex patterns from cache. | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Unanchored regex in MCP rule matching can match unintended tools
The regex path in evaluateCloudMcpRule is only reached when startsWith returns false, but the constructed RegExp is not anchored (^/$). A tool_pattern of "exec" would match reexec, context, or any tool whose name contains the substring. An operator who intends a prefix-only rule will get broader matching than expected, potentially triggering BLOCK or WARN actions on unrelated tools. Consider anchoring the regex (e.g. ^${pattern}) or documenting that patterns are treated as unanchored substrings.
|
|
||
| let params = event.params; | ||
| let shouldReturnParams = false; | ||
| const cloudPolicy = loadCloudPolicyCache(); |
There was a problem hiding this comment.
readFileSync called on every tool invocation
loadCloudPolicyCache() synchronously reads and parses policies.json on every single before_tool_call event. In a busy session this adds a synchronous disk-read to every tool call, blocking the event loop. Consider caching the result in memory (with a short TTL or file-watch invalidation) so that repeated calls are served from the in-process cache.
Reins Cloud is now the main policy source for OpenClaw runtime shell and MCP enforcement, instead of relying on the inbuilt reins policy. The policies.json file, which comes from Reins Cloud, is the source of policy unless it doesn't exist, in which case the inbuilt policy is used. protected_paths was removed from Reins Cloud interfacing because that functionality does not exist in Reins Cloud.