Conversation
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 6 minutes and 9 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds organization-scoped command policy management: client UI and API proxies for CRUD/test/templates, a server-side regex-based policy engine with caching and DB schema, enforcement hooks in execution tools to block commands, and inclusion of active policy text in LLM system prompts. Changes
Sequence Diagram(s)sequenceDiagram
participant Admin
participant Client as Client UI (SecuritySettings)
participant Proxy as Next.js Proxy
participant Server as Flask Backend
participant DB as Database
Admin->>Client: Open Security settings / issue CRUD
Client->>Proxy: GET/POST/PUT/DELETE /api/org/command-policies...
Proxy->>Server: Forward request with auth headers
Server->>DB: Query/Modify org_command_policies + prefs
DB-->>Server: Rules / state / ack
Server-->>Proxy: JSON response (status)
Proxy-->>Client: Return response -> UI refresh
sequenceDiagram
participant User
participant Tool as Exec Tool
participant Policy as Policy Engine
participant DB as Database
participant Exec as Execution Backend
User->>Tool: Request command execution
Tool->>Policy: evaluate_compound_command(org_id, command)
Policy->>Policy: Check cache (30s TTL)
alt Cache miss
Policy->>DB: Load org rules & toggles
DB-->>Policy: Rules
Policy->>Policy: Compile regexes & split command
end
Policy-->>Tool: CommandVerdict(allowed?, rule_description?)
alt Denied
Tool->>User: Return POLICY_DENIED with rule_description
else Allowed
Tool->>Exec: Proceed to execute command
Exec-->>Tool: Result
Tool->>User: Return result
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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. ✨ Finishing Touches🧪 Generate unit tests (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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Dual allowlist/denylist security settings UI with toggles, test command panel, and BFF proxy routes. Includes design document and mockup screenshots. Made-with: Cursor
…ting tools Single shared allowlist/denylist evaluated at every command execution point. Fail-closed: DB errors deny all commands. 30s in-memory cache. Enforcement points: - terminal_exec (shell commands, checked before cloud/IaC routing) - cloud_exec (cloud CLI commands) - tailscale_ssh (remote SSH commands) - kubectl_onprem (on-prem cluster commands) - iac_tool (terraform local-exec provisioner scan before apply/destroy) Also includes: DB schema, API routes, prompt injection, seed templates. Made-with: Cursor
Made-with: Cursor
damianloch
changed the title
Made-with: Cursor
damianloch
marked this pull request as ready for review
April 18, 2026 17:08
![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: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/chat/backend/agent/tools/cloud_exec_tool.py (1)
1619-2026:⚠️ Potential issue | 🔴 CriticalPolicy gate is placed too late — Tailscale and AWS multi-account paths bypass enforcement.
The policy check at lines 2011-2026 runs only after several early-return execution paths, so any deny rule an admin configures will be silently bypassed for:
- Tailscale (lines 1804-1848):
execute_tailscale_command(command, isolated_env)runs and returns before the policy gate is ever reached. Every Tailscale REST command (includingdevice delete,auth-key create,aclmutations) skips the org policy entirely.- AWS multi-account fan-out (lines 1621-1633):
_cloud_exec_aws_multi_account(...)is invoked whenever a user has >1 AWS connection and no explicitaccount_idis passed. That helper never callsevaluate_compound_command, so a single request can fan out a denied command (e.g.,ec2 terminate-instances) across every connected account.gcloud config get-value projectintercept (line 1776): also returns before the gate. Low impact (read-only), but inconsistent with the "shared firewall across all tools" invariant the PR advertises.Since denylist/allowlist are org-scoped and meant to be enforced uniformly, the gate should live on the common path, before any provider-specific early return. Suggested fix: run the policy check immediately after
commandis finalized (i.e., right after thegcloud/aws/azprefix + flag injection around line ~1999), and also add an equivalent check at the top of_cloud_exec_aws_multi_account(evaluating against each fanned-out command, or at minimum the incomingcommand).🔒 Proposed fix — hoist the gate and cover multi-account
@@ def cloud_exec(...): - logger.info(f"Executing command: {command}") - - current_mode = get_mode_from_context() - allowed, read_only_message = ModeAccessController.ensure_cloud_command_allowed( + logger.info(f"Executing command: {command}") + + # Org command policy check — must run on the common path so Tailscale, + # multi-account AWS, and any future early-return providers are covered. + from utils.auth.command_policy import evaluate_compound_command + from utils.auth.stateless_auth import get_org_id_for_user + org_id = get_org_id_for_user(user_id) if user_id else None + verdict = evaluate_compound_command(org_id, command) + if not verdict.allowed: + logger.warning("Policy denied cloud command for user %s (%s)", + user_id, verdict.rule_description) + return json.dumps({ + "success": False, + "error": f"Command blocked by organization policy: {verdict.rule_description}", + "code": "POLICY_DENIED", + "final_command": command, + "provider": provider.lower(), + }) + + current_mode = get_mode_from_context() + allowed, read_only_message = ModeAccessController.ensure_cloud_command_allowed( current_mode, is_read_only_command(command), command, ) @@ - # Org command policy check (shared allow/deny firewall across all tools) - from utils.auth.command_policy import evaluate_compound_command - from utils.auth.stateless_auth import get_org_id_for_user - org_id = get_org_id_for_user(user_id) if user_id else None - verdict = evaluate_compound_command(org_id, command) - if not verdict.allowed: - logger.warning("Policy denied cloud command for user %s (%s)", - user_id, verdict.rule_description) - return json.dumps({ - "success": False, - "error": f"Command blocked by organization policy: {verdict.rule_description}", - "code": "POLICY_DENIED", - "final_command": command, - "provider": provider.lower(), - }) -Additionally, add an equivalent gate at the top of
_cloud_exec_aws_multi_account(before_run_on_accountfans out) so a policy-denied command never reaches any account.Note: hoisting the check above
ensure_cloud_command_allowedalso means the Tailscale branch at line 1804 and thegcloud config get-value projectintercept at line 1776 need to be moved below the gate (or the gate needs to be duplicated there). Moving the gate up to just after provider normalization / CLI prefixing is the simpler refactor.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/chat/backend/agent/tools/cloud_exec_tool.py` around lines 1619 - 2026, The policy gate is currently executed too late so paths like Tailscale (execute_tailscale_command), AWS multi-account fan-out (_cloud_exec_aws_multi_account) and the gcloud-config intercept return before evaluate_compound_command/ModeAccessController.ensure_cloud_command_allowed run; move/hoist the org policy check (use evaluate_compound_command and ModeAccessController.ensure_cloud_command_allowed) to the common path immediately after the command is finalized (i.e., right after the CLI prefixing and flag injection logic that builds `command` and before any provider-specific early returns), and additionally add an equivalent policy check at the top of _cloud_exec_aws_multi_account to block fanned-out commands before any per-account _run_on_account invocation; ensure the Tailscale branch and the gcloud config get-value intercept are executed only after this centralized check (or perform a duplicate check there if refactoring is impractical).
🧹 Nitpick comments (11)
client/src/app/globals.css (1)
143-158: Make.scrollbar-thinconsistent across axes and input types.On Line 144 only
heightis set for WebKit, so vertical scrollbars remain default width there, while Firefox applies thin sizing both ways (Line 157). Also, keeping the thumb fully transparent until hover can make dragging hard on non-hover devices.Suggested CSS tweak
.scrollbar-thin::-webkit-scrollbar { - height: 1px; + width: 6px; + height: 6px; } .scrollbar-thin::-webkit-scrollbar-track { background: transparent; } .scrollbar-thin::-webkit-scrollbar-thumb { - background: transparent; + background: hsl(var(--muted-foreground) / 0.15); border-radius: 2px; } .scrollbar-thin:hover::-webkit-scrollbar-thumb { background: hsl(var(--muted-foreground) / 0.25); } .scrollbar-thin { scrollbar-width: thin; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/app/globals.css` around lines 143 - 158, The .scrollbar-thin rules are inconsistent: update the WebKit rules for .scrollbar-thin (::-webkit-scrollbar) to set both width and height so thin sizing applies to vertical and horizontal axes, and adjust the ::-webkit-scrollbar-thumb so it is slightly visible by default (use a low-opacity background) instead of fully transparent while keeping a stronger color on .scrollbar-thin:hover::-webkit-scrollbar-thumb; keep ::-webkit-scrollbar-track transparent and retain scrollbar-width: thin for non-WebKit browsers to match behavior across engines.server/chat/backend/agent/tools/kubectl_onprem_tool.py (1)
44-62: Gate is correctly placed; minor duplication offull_command.Evaluating the reconstructed
kubectl <command>string ensures allow/deny regex rules that targetkubectlverbs (e.g.,kubectl\s+delete) match regardless of whether the caller passed the prefix. Behavior looks right.Two small nits, both optional:
full_commandis computed here and then recomputed at line 124; you can hoist it once above the policy check and reuse.user_idis already validated at line 30, soif user_id else Noneon line 49 is unreachable-false and can be dropped.♻️ Optional simplification
- # Org command policy check against full kubectl command - from utils.auth.command_policy import evaluate_compound_command - from utils.auth.stateless_auth import get_org_id_for_user - full_command = f"kubectl {command}" - org_id = get_org_id_for_user(user_id) if user_id else None - verdict = evaluate_compound_command(org_id, full_command) + full_command = f"kubectl {command}" + + # Org command policy check against full kubectl command + from utils.auth.command_policy import evaluate_compound_command + from utils.auth.stateless_auth import get_org_id_for_user + org_id = get_org_id_for_user(user_id) + verdict = evaluate_compound_command(org_id, full_command) @@ - # Format response - match cloud_exec/terminal_exec pattern - full_command = f"kubectl {command}" - response_data = { + # Format response - match cloud_exec/terminal_exec pattern + response_data = {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/chat/backend/agent/tools/kubectl_onprem_tool.py` around lines 44 - 62, Hoist construction of full_command (currently set before the policy check and again later) so it’s built once and reused; remove the redundant conditional "if user_id else None" when calling get_org_id_for_user in the policy check (user_id is already validated), i.e., call get_org_id_for_user(user_id) directly and pass org_id into evaluate_compound_command(full_command) to avoid duplicated computation and unreachable branching (referencing full_command, get_org_id_for_user, evaluate_compound_command, and user_id).server/chat/backend/agent/tools/terminal_exec_tool.py (1)
265-278: Policy gate placement looks correct for this tool.The gate runs after the SSH
-J→ProxyCommandrewrite and thesh:/shell:prefix strip, so the evaluated string matches what will actually be executed (or re-routed). It also precedes the cloud/IaC routing and the dangerous-pattern/elevation checks, so denies short-circuit cleanly. Note that commands routed tocloud_execwill be re-evaluated downstream, which is redundant but harmless.One small note: since
user_idis already validated as non-empty at line 231 (early return), theif user_id else Noneguard on line 269 is dead code — you can simplify toorg_id = get_org_id_for_user(user_id). Non-blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/chat/backend/agent/tools/terminal_exec_tool.py` around lines 265 - 278, The policy gate placement is fine; simplify the dead conditional by calling get_org_id_for_user directly since user_id is guaranteed non-empty: replace the guarded assignment org_id = get_org_id_for_user(user_id) if user_id else None with a direct call org_id = get_org_id_for_user(user_id) so the code around evaluate_compound_command and get_org_id_for_user (used before verdict = evaluate_compound_command(org_id, command)) is cleaner and removes the unnecessary branch.client/src/components/SecuritySettings.tsx (1)
317-326: Avoid ternary-for-side-effects; ESLintno-unused-expressionswill flag this.- mode === "allow" ? setShowAddAllow(false) : setShowAddDeny(false); + if (mode === "allow") setShowAddAllow(false); + else setShowAddDeny(false);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/components/SecuritySettings.tsx` around lines 317 - 326, The ternary expression in handleAddRule is being used for side effects and triggers eslint no-unused-expressions; replace the ternary (mode === "allow" ? setShowAddAllow(false) : setShowAddDeny(false)) with an explicit conditional block: check mode using if (mode === "allow") { setShowAddAllow(false) } else { setShowAddDeny(false) } so the side-effect calls in handleAddRule are clear and not treated as unused expressions.server/chat/backend/agent/tools/iac/iac_commands_tool.py (1)
72-74: Glob relies on unquoted/*.tf; avoidshell=Trueby listing files in Python.
shlex.quote(terraform_dir)quotes the directory, so the command becomescat '…'/*.tf— the glob still expands because*.tfis outside the quotes, which is what you want, but the Ruff S604 finding is real in the sense that you don't need a shell at all. ListingPath(terraform_dir).glob("*.tf")and reading each file in Python is simpler, avoids spawning a subprocess per IaC call, and removes theshell=True/timeout dependency. It also sidesteps any future surprise ifterraform_direver contained a path whose quoting semantics differ from whatterminal_runexpects.Proposed refactor
- from utils.auth.command_policy import evaluate_compound_command - from utils.terminal.terminal_run import terminal_run - - safe_dir = shlex.quote(terraform_dir) - list_cmd = f"cat {safe_dir}/*.tf 2>/dev/null || true" - result = terminal_run(list_cmd, shell=True, capture_output=True, text=True, timeout=15) - if result.returncode != 0 or not result.stdout.strip(): - return [] - - content = result.stdout + from pathlib import Path + from utils.auth.command_policy import evaluate_compound_command + + try: + tf_paths = list(Path(terraform_dir).glob("*.tf")) + except OSError: + return [] + if not tf_paths: + return [] + chunks: List[str] = [] + for p in tf_paths: + try: + chunks.append(p.read_text(errors="replace")) + except OSError: + continue + content = "\n".join(chunks) + if not content.strip(): + return []🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/chat/backend/agent/tools/iac/iac_commands_tool.py` around lines 72 - 74, Replace the shell-based glob and cat call with pure-Python file listing and reading: stop constructing safe_dir/list_cmd and calling terminal_run; instead use pathlib.Path(terraform_dir).glob("*.tf") to iterate matching .tf files, open and read each file's contents in Python (concatenate or process as needed) and produce the same result previously held in result; remove use of shlex.quote, shell=True, and timeout related to terminal_run to avoid subprocesses and shell globbing. Ensure you update any downstream uses of result to match the new in-memory string/list produced from reading the files.client/src/app/api/org/command-policies/route.ts (1)
7-41:reqparameter inproxyRequestis unused; the dummyNextRequestinGET()is a code smell.
proxyRequestdoesn't reference itsreqargument anywhere, yetGET()manufacturesnew NextRequest("http://localhost")just to satisfy the signature. Drop the parameter to make both call sites honest:-async function proxyRequest(req: NextRequest, method: string, path: string, body?: unknown) { +async function proxyRequest(method: string, path: string, body?: unknown) { @@ -export async function GET() { - return proxyRequest(new NextRequest("http://localhost"), "GET", "/api/org/command-policies"); -} +export async function GET() { + return proxyRequest("GET", "/api/org/command-policies"); +} @@ -export async function POST(req: NextRequest) { - const body = await req.json(); - return proxyRequest(req, "POST", "/api/org/command-policies", body); -} +export async function POST(req: NextRequest) { + const body = await req.json(); + return proxyRequest("POST", "/api/org/command-policies", body); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/app/api/org/command-policies/route.ts` around lines 7 - 41, The proxyRequest function currently accepts an unused req parameter and GET() constructs a dummy NextRequest — remove the unused parameter to clean the API: change proxyRequest signature from proxyRequest(req: NextRequest, method: string, path: string, body?: unknown) to proxyRequest(method: string, path: string, body?: unknown), remove any NextRequest type usage inside it, and update call sites GET() to call proxyRequest("GET", "/api/org/command-policies") (remove new NextRequest) and POST(req: NextRequest) to call proxyRequest("POST", "/api/org/command-policies", body); ensure imports/usages referencing NextRequest in this file are adjusted accordingly.server/routes/command_policies.py (2)
155-168: Ruff S608 on the dynamic UPDATE is a false positive — just leave a note.The field list at line 139 is an allowlist of hard-coded
(field, col)tuples, and onlycolis interpolated into the SQL; all values go through parameters. Ruff's S608 here is a false positive, but a brief# noqa: S608 — column names are whitelisted(or a short comment) would prevent future maintainers from "fixing" this by concatenating user input.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/command_policies.py` around lines 155 - 168, Add a brief inline suppression/comment explaining that the dynamic column interpolation is safe and whitelisted to silence Ruff S608: next to the code that builds updates and calls cur.execute (the updates list and params array used in the f"UPDATE org_command_policies SET {', '.join(updates)} ..." call), add a single-line comment like "# noqa: S608 — column names are whitelisted" or "# column names validated/whitelisted above" so maintainers and linters know this is intentional rather than changing SQL construction or parameterization in updates, params, db_pool.get_admin_connection, cur.execute.
105-107: Drop-on-the-floor regex error prevents admins from fixing their pattern.
validate_patternreturns the underlyingre.errorstring (e.g."unbalanced parenthesis at position 5"), but the endpoint discards it and replies with a generic"Invalid regex pattern". Admins using the UI then see "Failed to add rule" with no hint about what is wrong. Same thing inupdate_policyat line 148. Surfacingerris low risk here — this is an admin-only endpoint and the string comes from Python's own regex compiler, not user-controlled backend state.- err = validate_pattern(pattern) - if err: - return jsonify({"error": "Invalid regex pattern"}), 400 + err = validate_pattern(pattern) + if err: + return jsonify({"error": f"Invalid regex pattern: {err}"}), 400🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/command_policies.py` around lines 105 - 107, The error response for invalid regex discards the underlying re.error string; modify the handlers that call validate_pattern (the create/add rule flow where err = validate_pattern(pattern) and the update_policy handler referenced at update_policy) to return the actual err message in the JSON response (e.g. jsonify({"error": f"Invalid regex pattern: {err}"}) or similar) and keep the 400 status code; ensure this change is applied in both the add/create rule endpoint and the update_policy path so admins see the concrete regex compiler error.server/utils/auth/command_policy.py (2)
113-122: Module-level_cachedict is not concurrency-safe and unbounded.Under a multi-threaded Flask/gunicorn worker (threaded workers, gthread, or async with sync tool calls),
_cache.get/_cache[...] = ...race withinvalidate_cache, and a cache miss storm can issue N concurrent DB fetches for the same org. The dict also grows once per org and never evicts, so long-running workers slowly accumulate entries for every org that has ever been seen.Minimal improvement:
-import logging +import logging +import threading @@ -_cache: Dict[str, _CacheEntry] = {} +_cache: Dict[str, _CacheEntry] = {} +_cache_lock = threading.Lock() @@ def _get_cached(...): - entry = _cache.get(org_id) - if entry is not None: - allow, deny, states, ts = entry - if time.monotonic() - ts < _CACHE_TTL: - return allow, deny, states - - allow, deny, states = _fetch(org_id) - _cache[org_id] = (allow, deny, states, time.monotonic()) - return allow, deny, states + with _cache_lock: + entry = _cache.get(org_id) + if entry is not None: + allow, deny, states, ts = entry + if time.monotonic() - ts < _CACHE_TTL: + return allow, deny, states + + allow, deny, states = _fetch(org_id) + with _cache_lock: + _cache[org_id] = (allow, deny, states, time.monotonic()) + return allow, deny, statesAn LRU bound (or
functools.lru_cachekeyed on a(org_id, time_bucket)tuple) would also be worth considering given that every command evaluation goes through here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/utils/auth/command_policy.py` around lines 113 - 122, The module-level _cache dict used in _get_cached is not concurrency-safe and unbounded; replace it with a thread-safe bounded cache (e.g., use collections.OrderedDict with a lock to implement an LRU or use functools.lru_cache keyed by (org_id, time_bucket) semantics) to prevent race conditions with invalidate_cache and unbounded growth; specifically, wrap accesses to _cache (reads in _get_cached, writes when setting the entry, and invalidate_cache) with a shared threading.Lock (or use a concurrent LRU implementation) and add a max size/eviction policy tied to an LRU or time-bucket TTL based on _CACHE_TTL so concurrent cache-miss storms won’t trigger N _fetch(org_id) calls and old org entries are evicted.
237-260:evaluate_compound_commandignores short-circuit semantics and short-circuits on denied rule_description only.Two smaller points to consider:
- Commands joined by
||/&&are decomposed and all must pass. That's conservatively correct for a security gate, but it meanscmd_that_might_fail || safe_fallbackis denied whenever either side is denied, even when the denied side would never actually execute. This is a safety-first design — worth documenting explicitly so template authors understand why rules should cover fallbacks.last_verdictaccumulation means the allowed verdict you return carries therule_descriptionof whichever sub-command matched last. For logging/telemetry purposes, reporting "which rule allowed this command" is ambiguous for compound commands. If callers ever surfacerule_descriptionon the success path, consider aggregating descriptions or returning a plainallowed=Truewithout one.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/utils/auth/command_policy.py` around lines 237 - 260, evaluate_compound_command currently treats every sub-command as required to pass and returns the rule_description from the last sub-command, which can be misleading; update the function (referencing evaluate_compound_command, _split_compound_command and evaluate_command) to keep the conservative "all sub-commands must pass" behavior but explicitly document this short-circuit/security-first semantics, and on success return a CommandVerdict with allowed=True and no rule_description (or aggregate descriptions if desired) instead of carrying the last sub-command's rule_description so callers don't receive ambiguous attribution for compound commands.client/src/app/api/org/command-policy-toggle/route.ts (1)
6-18: Consider matching the timeout pattern used in sibling policy routes.The other command-policy proxy routes (
command-policies/route.ts,command-policies/[id]/route.ts) wrapfetchin anAbortControllerwith a 20s timeout and map aborts to 504. This handler does not, so a hung backend will keep the Node request pending until Node's default limits. Adopting the same pattern would keep the UX consistent across the toggle flow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/app/api/org/command-policy-toggle/route.ts` around lines 6 - 18, The PUT handler for command-policy-toggle is missing the AbortController timeout used by sibling routes; update the PUT function to wrap the fetch call in an AbortController with a 20s timeout (create controller, pass signal to fetch, clearTimeout after response) and catch abort errors to return NextResponse.json({}, { status: 504 }) while preserving existing header/body logic and using API_BASE_URL and getAuthenticatedUser as currently implemented; ensure the timeout is cleaned up to avoid leaks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/src/components/SecuritySettings.tsx`:
- Around line 375-384: The optimistic update clears local state before the API
call in handleRemoveTemplate; change it to await
commandPolicyService.clearActiveTemplate() before calling
setActiveTemplateId(null) and fetchPolicies(), and on failure keep the existing
activeTemplateId (or restore it) so the UI remains consistent; additionally, add
a confirmation step matching the Apply flow (use the same confirmId pattern) to
confirm the destructive clearActiveTemplate action and only proceed with the API
call after user confirmation, while keeping toast notifications for
success/failure.
In `@client/src/lib/services/command-policies.ts`:
- Around line 51-63: The client response types for updatePolicy, deletePolicy,
and toggleList in command-policies.ts do not match the backend shapes:
updatePolicy/deletePolicy are typed as { ok: boolean } but server returns {
status: "updated" } / { status: "deleted" }, and toggleList should include {
status: "updated", allowlist_enabled, denylist_enabled, active_template_id } not
an { ok } field. Fix by updating the return types used by
apiPut/apiDelete/apiPut in the functions updatePolicy, deletePolicy, and
toggleList to match the backend (e.g., use { status: string } or a narrow union
for status for updatePolicy/deletePolicy, and for toggleList use { status:
string; allowlist_enabled: boolean; denylist_enabled: boolean;
active_template_id?: string | number } ), referencing the functions
updatePolicy, deletePolicy, and toggleList so callers get correct typings.
In `@server/chat/backend/agent/prompt/composer.py`:
- Around line 115-123: The code currently injects raw text from
get_policy_prompt_text into the system prompt via security_policy (in
composer.py), which lets configurable rule descriptions act as instructions;
instead, treat that output as untrusted data by escaping or encoding it and
embedding it inside a clearly delimited non-executable block (e.g., JSON-encode,
base64, or prefix with a sentinel like "POLICY_DATA_START/END" and an explicit
"Do not follow as instructions" header) before concatenating into the prompt;
update all places that use get_policy_prompt_text (security_policy and the other
occurrences where prompt text is prepended) to wrap/escape the policy text the
same way so regexes and descriptions cannot be interpreted as instructions by
the model.
In `@server/chat/backend/agent/tools/iac/iac_commands_tool.py`:
- Around line 47-97: The regexes _LOCAL_EXEC_RE and _EXTERNAL_RE in
_scan_tf_for_exec_provisioners fail on nested braces and quoted/escaped commas
(so provisioners with nested blocks, maps, heredocs or program arrays with
commas inside quotes are skipped/misparsed); replace the brittle regex approach
with a simple stateful scanner or use an HCL-aware tokenizer to locate
provisioner "local-exec" blocks and data "external" program arrays, implementing
brace-depth tracking that respects quotes and heredocs for extracting the
command string, and parse the program array with quote-aware splitting (or use
the tokenizer to build the command list) before passing the assembled command to
evaluate_compound_command.
In `@server/chat/backend/agent/tools/tailscale_ssh_tool.py`:
- Around line 219-232: The policy check currently only evaluates the `command`
via `evaluate_compound_command` (with `get_org_id_for_user`) but does not
validate the `device_hostname` that is later interpolated into the SSH shell
invocation; to fix, validate and/or sanitize `device_hostname` before calling
`evaluate_compound_command` (or include it in the evaluated payload) by
rejecting or escaping any input that contains shell metacharacters (e.g., ; | &
$ ` > < \n, quotes, backticks) and only allow a safe character class (letters,
digits, dots, dashes) or explicit hostname pattern, and if invalid return the
same policy-style JSON error (e.g., code "INVALID_TARGET") instead of proceeding
to build the `/bin/bash -c` command; update references around `device_hostname`,
`command`, `evaluate_compound_command`, and the tailscale SSH tool invocation to
enforce this check.
In `@server/routes/command_policies.py`:
- Around line 348-370: The route function clear_active_template currently
deletes all org_command_policies rows and flips both allow/deny preferences (via
the DELETE SQL on org_command_policies and subsequent store_user_preference
calls), which is destructive and can diverge if the DB delete fails; change the
implementation so it only disassociates the active template pointer (do not
delete custom policy rows) or rename the endpoint to a clear/reset intent (e.g.,
reset_policies) and document this in the function docstring, wrap the DB
operation in try/except around db_pool.get_admin_connection()/cur.execute so
preferences (store_user_preference calls) and invalidate_cache(org_id) are only
updated/returned on successful commit, and update the UI flow
(SecuritySettings.tsx handleRemoveTemplate) to show a confirmation explaining
the full consequences if you keep the destructive behavior.
- Line 98: The current unchecked coercion priority = int(data.get("priority",
0)) can raise ValueError for non-numeric input; update the request handler that
parses 'data' to explicitly validate the "priority" field (e.g., check it's
numeric or attempt int() inside a try/except), and on invalid input return a 400
response with a clear error message instead of letting the exception propagate;
make sure to reference and protect the priority variable and any function/method
that processes the incoming data payload when adding this validation.
- Around line 45-370: Wrap the body of every Flask handler in this file (e.g.,
list_policies, create_policy, update_policy, delete_policy, test_command,
toggle_list, list_templates, apply_template, clear_active_template) in a
try/except Exception block that calls logger.exception(...) with a clear context
message and returns jsonify({"error": "internal error"}), 500; ensure you import
or reference the module logger used across the project. Additionally, add
specific ValueError handling around numeric coercions: in create_policy where
priority = int(data.get("priority", 0)) and in update_policy when handling the
"priority" field, validate/convert to int inside a try/except ValueError to
return a 400 with a clear error message (e.g., "priority must be an integer")
rather than raising. Ensure all DB operations, preference stores and template
application code inside those handlers are covered by the try/except so
exceptions are logged consistently.
- Line 174: The store_user_preference calls in this file are double-encoding
string values because store_user_preference already json.dumps() internally;
update all calls that currently pass json.dumps("on") or json.dumps("off") to
pass the raw Python strings ("on" / "off") and leave None as-is (e.g., change
occurrences where command_policy_active_template or related preference keys are
set using json.dumps(...) to use "on"/"off" directly), ensuring you modify the
calls to store_user_preference so they pass unencoded values.
In `@server/utils/auth/command_policy.py`:
- Around line 67-110: The function _fetch currently swallows DB errors and
returns empty allow/deny lists + ListStates(false,false), which leads
evaluate_command to allow everything (fail-open); to implement true fail-closed,
change _fetch to propagate failure (e.g., raise the exception or return a
sentinel like None) instead of returning empty rules, update evaluate_command to
treat a None/sentinel return from _fetch as a hard deny (return
CommandVerdict(allowed=False,...)), and ensure the logger.exception call in
_fetch remains but its message matches the new behavior ("Failed to fetch
command policies for org %s, fail-closed"); reference symbols: _fetch,
ListStates, evaluate_command, CommandVerdict, and logger.exception.
In `@server/utils/db/db_utils.py`:
- Around line 381-396: The org-scoped table org_command_policies is missing from
the RLS registration list, so add "org_command_policies" to the rls_tables
collection/tuple used by the RLS enable/force/policy loop (the variable
referenced at the RLS loop around line 1899) so it gets the same
enable/force/policy treatment as other org-scoped tables; locate the rls_tables
definition in db_utils.py and append "org_command_policies" alongside the other
org-scoped table names to ensure RLS is applied.
---
Outside diff comments:
In `@server/chat/backend/agent/tools/cloud_exec_tool.py`:
- Around line 1619-2026: The policy gate is currently executed too late so paths
like Tailscale (execute_tailscale_command), AWS multi-account fan-out
(_cloud_exec_aws_multi_account) and the gcloud-config intercept return before
evaluate_compound_command/ModeAccessController.ensure_cloud_command_allowed run;
move/hoist the org policy check (use evaluate_compound_command and
ModeAccessController.ensure_cloud_command_allowed) to the common path
immediately after the command is finalized (i.e., right after the CLI prefixing
and flag injection logic that builds `command` and before any provider-specific
early returns), and additionally add an equivalent policy check at the top of
_cloud_exec_aws_multi_account to block fanned-out commands before any
per-account _run_on_account invocation; ensure the Tailscale branch and the
gcloud config get-value intercept are executed only after this centralized check
(or perform a duplicate check there if refactoring is impractical).
---
Nitpick comments:
In `@client/src/app/api/org/command-policies/route.ts`:
- Around line 7-41: The proxyRequest function currently accepts an unused req
parameter and GET() constructs a dummy NextRequest — remove the unused parameter
to clean the API: change proxyRequest signature from proxyRequest(req:
NextRequest, method: string, path: string, body?: unknown) to
proxyRequest(method: string, path: string, body?: unknown), remove any
NextRequest type usage inside it, and update call sites GET() to call
proxyRequest("GET", "/api/org/command-policies") (remove new NextRequest) and
POST(req: NextRequest) to call proxyRequest("POST", "/api/org/command-policies",
body); ensure imports/usages referencing NextRequest in this file are adjusted
accordingly.
In `@client/src/app/api/org/command-policy-toggle/route.ts`:
- Around line 6-18: The PUT handler for command-policy-toggle is missing the
AbortController timeout used by sibling routes; update the PUT function to wrap
the fetch call in an AbortController with a 20s timeout (create controller, pass
signal to fetch, clearTimeout after response) and catch abort errors to return
NextResponse.json({}, { status: 504 }) while preserving existing header/body
logic and using API_BASE_URL and getAuthenticatedUser as currently implemented;
ensure the timeout is cleaned up to avoid leaks.
In `@client/src/app/globals.css`:
- Around line 143-158: The .scrollbar-thin rules are inconsistent: update the
WebKit rules for .scrollbar-thin (::-webkit-scrollbar) to set both width and
height so thin sizing applies to vertical and horizontal axes, and adjust the
::-webkit-scrollbar-thumb so it is slightly visible by default (use a
low-opacity background) instead of fully transparent while keeping a stronger
color on .scrollbar-thin:hover::-webkit-scrollbar-thumb; keep
::-webkit-scrollbar-track transparent and retain scrollbar-width: thin for
non-WebKit browsers to match behavior across engines.
In `@client/src/components/SecuritySettings.tsx`:
- Around line 317-326: The ternary expression in handleAddRule is being used for
side effects and triggers eslint no-unused-expressions; replace the ternary
(mode === "allow" ? setShowAddAllow(false) : setShowAddDeny(false)) with an
explicit conditional block: check mode using if (mode === "allow") {
setShowAddAllow(false) } else { setShowAddDeny(false) } so the side-effect calls
in handleAddRule are clear and not treated as unused expressions.
In `@server/chat/backend/agent/tools/iac/iac_commands_tool.py`:
- Around line 72-74: Replace the shell-based glob and cat call with pure-Python
file listing and reading: stop constructing safe_dir/list_cmd and calling
terminal_run; instead use pathlib.Path(terraform_dir).glob("*.tf") to iterate
matching .tf files, open and read each file's contents in Python (concatenate or
process as needed) and produce the same result previously held in result; remove
use of shlex.quote, shell=True, and timeout related to terminal_run to avoid
subprocesses and shell globbing. Ensure you update any downstream uses of result
to match the new in-memory string/list produced from reading the files.
In `@server/chat/backend/agent/tools/kubectl_onprem_tool.py`:
- Around line 44-62: Hoist construction of full_command (currently set before
the policy check and again later) so it’s built once and reused; remove the
redundant conditional "if user_id else None" when calling get_org_id_for_user in
the policy check (user_id is already validated), i.e., call
get_org_id_for_user(user_id) directly and pass org_id into
evaluate_compound_command(full_command) to avoid duplicated computation and
unreachable branching (referencing full_command, get_org_id_for_user,
evaluate_compound_command, and user_id).
In `@server/chat/backend/agent/tools/terminal_exec_tool.py`:
- Around line 265-278: The policy gate placement is fine; simplify the dead
conditional by calling get_org_id_for_user directly since user_id is guaranteed
non-empty: replace the guarded assignment org_id = get_org_id_for_user(user_id)
if user_id else None with a direct call org_id = get_org_id_for_user(user_id) so
the code around evaluate_compound_command and get_org_id_for_user (used before
verdict = evaluate_compound_command(org_id, command)) is cleaner and removes the
unnecessary branch.
In `@server/routes/command_policies.py`:
- Around line 155-168: Add a brief inline suppression/comment explaining that
the dynamic column interpolation is safe and whitelisted to silence Ruff S608:
next to the code that builds updates and calls cur.execute (the updates list and
params array used in the f"UPDATE org_command_policies SET {', '.join(updates)}
..." call), add a single-line comment like "# noqa: S608 — column names are
whitelisted" or "# column names validated/whitelisted above" so maintainers and
linters know this is intentional rather than changing SQL construction or
parameterization in updates, params, db_pool.get_admin_connection, cur.execute.
- Around line 105-107: The error response for invalid regex discards the
underlying re.error string; modify the handlers that call validate_pattern (the
create/add rule flow where err = validate_pattern(pattern) and the update_policy
handler referenced at update_policy) to return the actual err message in the
JSON response (e.g. jsonify({"error": f"Invalid regex pattern: {err}"}) or
similar) and keep the 400 status code; ensure this change is applied in both the
add/create rule endpoint and the update_policy path so admins see the concrete
regex compiler error.
In `@server/utils/auth/command_policy.py`:
- Around line 113-122: The module-level _cache dict used in _get_cached is not
concurrency-safe and unbounded; replace it with a thread-safe bounded cache
(e.g., use collections.OrderedDict with a lock to implement an LRU or use
functools.lru_cache keyed by (org_id, time_bucket) semantics) to prevent race
conditions with invalidate_cache and unbounded growth; specifically, wrap
accesses to _cache (reads in _get_cached, writes when setting the entry, and
invalidate_cache) with a shared threading.Lock (or use a concurrent LRU
implementation) and add a max size/eviction policy tied to an LRU or time-bucket
TTL based on _CACHE_TTL so concurrent cache-miss storms won’t trigger N
_fetch(org_id) calls and old org entries are evicted.
- Around line 237-260: evaluate_compound_command currently treats every
sub-command as required to pass and returns the rule_description from the last
sub-command, which can be misleading; update the function (referencing
evaluate_compound_command, _split_compound_command and evaluate_command) to keep
the conservative "all sub-commands must pass" behavior but explicitly document
this short-circuit/security-first semantics, and on success return a
CommandVerdict with allowed=True and no rule_description (or aggregate
descriptions if desired) instead of carrying the last sub-command's
rule_description so callers don't receive ambiguous attribution for compound
commands.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ab67d19c-61d9-4b87-b2b0-7d21d20f5714
📒 Files selected for processing (23)
client/src/app/api/org/command-policies/[id]/route.tsclient/src/app/api/org/command-policies/route.tsclient/src/app/api/org/command-policies/test/route.tsclient/src/app/api/org/command-policy-templates/active/route.tsclient/src/app/api/org/command-policy-templates/apply/route.tsclient/src/app/api/org/command-policy-templates/route.tsclient/src/app/api/org/command-policy-toggle/route.tsclient/src/app/globals.cssclient/src/components/SecuritySettings.tsxclient/src/components/SettingsModal.tsxclient/src/lib/services/command-policies.tsserver/chat/backend/agent/prompt/composer.pyserver/chat/backend/agent/prompt/provider_rules.pyserver/chat/backend/agent/prompt/schema.pyserver/chat/backend/agent/tools/cloud_exec_tool.pyserver/chat/backend/agent/tools/iac/iac_commands_tool.pyserver/chat/backend/agent/tools/kubectl_onprem_tool.pyserver/chat/backend/agent/tools/tailscale_ssh_tool.pyserver/chat/backend/agent/tools/terminal_exec_tool.pyserver/main_compute.pyserver/routes/command_policies.pyserver/utils/auth/command_policy.pyserver/utils/db/db_utils.py
Zarlanx
reviewed
Apr 18, 2026
Zarlanx
reviewed
Apr 18, 2026
Zarlanx
reviewed
Apr 18, 2026
Zarlanx
reviewed
Apr 18, 2026
damianloch
commented
Apr 18, 2026
Remove IaC provisioner regex scan (fragile HCL parsing, tool-level gates are primary enforcement). Fix double json.dumps in preference stores, add priority input validation, hostname validation on tailscale SSH, truncate rule_description in error responses, correct fail-open log message, add RLS for org_command_policies, fix frontend response types and optimistic state update. Made-with: Cursor
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
server/chat/backend/agent/tools/tailscale_ssh_tool.py (1)
214-218:⚠️ Potential issue | 🔴 CriticalUse
fullmatchafter normalizing the hostname.
re.match(...$)can accept a trailing newline, and the raw hostname is later interpolated into a shell command. Normalize once and validate withfullmatchbefore policy approval/execution.🔒 Proposed hardening
- if not re.match(r'^[A-Za-z0-9._:\-]+$', device_hostname): + device_hostname = device_hostname.strip() + if not re.fullmatch(r'[A-Za-z0-9._:\-]+', device_hostname): return json.dumps({ "success": False, "error": "Invalid device hostname" })Run this to verify the regex behavior:
#!/bin/bash python - <<'PY' import re pattern = r'^[A-Za-z0-9._:\-]+$' safer = r'[A-Za-z0-9._:\-]+' for value in ["host", "host\n", "host;id"]: print(repr(value), "match=", bool(re.match(pattern, value)), "fullmatch=", bool(re.fullmatch(safer, value))) PY🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/chat/backend/agent/tools/tailscale_ssh_tool.py` around lines 214 - 218, Normalize device_hostname (e.g., strip whitespace/newlines) and validate it using re.fullmatch instead of re.match before any policy approval or shell interpolation; update the validation in the device hostname check (the block that returns JSON with "Invalid device hostname") to perform a single normalization step on device_hostname and then call re.fullmatch(r'[A-Za-z0-9._:\-]+', device_hostname) to reject inputs with trailing newlines or other unsafe characters.
🧹 Nitpick comments (1)
server/routes/command_policies.py (1)
157-160: Replace deprecateddatetime.utcnow()with timezone-awaredatetime.now(timezone.utc).
datetime.utcnow()is deprecated as of Python 3.12 and returns a naive datetime. The codebase already standardizes ondatetime.now(timezone.utc)in other modules. Update the import and replace the call:Proposed fix
-from datetime import datetime +from datetime import datetime, timezone ... updates.append("updated_at = %s") - params.append(datetime.utcnow()) + params.append(datetime.now(timezone.utc))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/command_policies.py` around lines 157 - 160, Replace the naive datetime call used when setting the updated_at param: change the call from datetime.utcnow() to a timezone-aware datetime.now(timezone.utc) and update the imports accordingly (ensure datetime and timezone are imported from datetime). Specifically, in the block that appends "updated_at = %s" to updates and appends the timestamp to params (the updates and params variables and the updated_at/updated_by assignments), replace datetime.utcnow() with datetime.now(timezone.utc) and adjust the module imports so timezone is available.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/src/lib/services/command-policies.ts`:
- Around line 72-73: The response type for clearActiveTemplate is missing the
allowlist_enabled and denylist_enabled boolean fields returned by the backend;
update the generic type passed to apiDelete in clearActiveTemplate to include
allowlist_enabled: boolean and denylist_enabled: boolean (keeping existing
status and active_template_id) so consumers of clearActiveTemplate receive the
full shape returned by server.routes.command_policies.clear_active_template.
In `@server/chat/backend/agent/prompt/composer.py`:
- Around line 179-180: The reminder message appended when
segments.security_policy is present is worded as "outside the organization
policy" which implies allowlist behavior; update the string passed to
parts.append in composer.py (the branch guarded by segments.security_policy) to
use violation-based wording such as "REMINDER: Commands that violate the
organization policy will be rejected. Do not attempt workarounds." so it
correctly reflects both denylist and allowlist policy modes.
In `@server/chat/backend/agent/tools/cloud_exec_tool.py`:
- Around line 2011-2026: Policy enforcement is currently applied too late and
misses early return paths like _cloud_exec_aws_multi_account and the Tailscale
REST path; extract the logic around evaluate_compound_command and
get_org_id_for_user (and the logger/json error response structure using
provider, command, verdict) into a reusable helper (e.g.,
enforce_org_command_policy(org_id, command, provider, user_id)) and invoke this
helper at the start of every execution branch in cloud_exec (including before
calling _cloud_exec_aws_multi_account and before the Tailscale REST execution
path) so the final command string for each path is validated and returns the
same POLICY_DENIED JSON when denied.
- Around line 2019-2022: The code slices a potentially None value at
verdict.rule_description[:200]; update the return to guard against None by using
a safe default or conditional, e.g. replace verdict.rule_description[:200] with
(verdict.rule_description or "")[:200] or use verdict.rule_description if
present else "No description" so the policy denial returns "POLICY_DENIED"
without raising a TypeError; apply this change where the verdict variable and
CommandVerdict.rule_description are used in the json.dumps error construction.
In `@server/chat/backend/agent/tools/tailscale_ssh_tool.py`:
- Around line 234-237: The return path builds an error string using
verdict.rule_description[:200] which will raise when rule_description is None;
update the code that constructs the error (the dict returned where "code":
"POLICY_DENIED") to safely handle a nullable description—e.g. use a safe
expression like (verdict.rule_description or "")[:200] or conditionally format
the message to fall back to a generic text when rule_description is None so
slicing never occurs on None.
In `@server/utils/db/db_utils.py`:
- Around line 381-396: The migration is not idempotent because CREATE TABLE IF
NOT EXISTS won't add the new priority column to an existing org_command_policies
table; add an idempotent ALTER-based migration that: uses ALTER TABLE
org_command_policies ADD COLUMN IF NOT EXISTS priority INT DEFAULT 0; then ALTER
TABLE ... ALTER COLUMN priority SET DEFAULT 0 (if needed), run UPDATE
org_command_policies SET priority = 0 WHERE priority IS NULL to backfill
existing rows, and ensure any related index/constraints are created only if
missing; reference the org_command_policies table, the priority column, and
idx_ocp_org in the migration logic so repeated runs are safe.
---
Duplicate comments:
In `@server/chat/backend/agent/tools/tailscale_ssh_tool.py`:
- Around line 214-218: Normalize device_hostname (e.g., strip
whitespace/newlines) and validate it using re.fullmatch instead of re.match
before any policy approval or shell interpolation; update the validation in the
device hostname check (the block that returns JSON with "Invalid device
hostname") to perform a single normalization step on device_hostname and then
call re.fullmatch(r'[A-Za-z0-9._:\-]+', device_hostname) to reject inputs with
trailing newlines or other unsafe characters.
---
Nitpick comments:
In `@server/routes/command_policies.py`:
- Around line 157-160: Replace the naive datetime call used when setting the
updated_at param: change the call from datetime.utcnow() to a timezone-aware
datetime.now(timezone.utc) and update the imports accordingly (ensure datetime
and timezone are imported from datetime). Specifically, in the block that
appends "updated_at = %s" to updates and appends the timestamp to params (the
updates and params variables and the updated_at/updated_by assignments), replace
datetime.utcnow() with datetime.now(timezone.utc) and adjust the module imports
so timezone is available.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c17c17d3-d921-472c-a525-4c03c7883a81
📒 Files selected for processing (10)
client/src/components/SecuritySettings.tsxclient/src/lib/services/command-policies.tsserver/chat/backend/agent/prompt/composer.pyserver/chat/backend/agent/tools/cloud_exec_tool.pyserver/chat/backend/agent/tools/kubectl_onprem_tool.pyserver/chat/backend/agent/tools/tailscale_ssh_tool.pyserver/chat/backend/agent/tools/terminal_exec_tool.pyserver/routes/command_policies.pyserver/utils/auth/command_policy.pyserver/utils/db/db_utils.py
🚧 Files skipped from review as they are similar to previous changes (3)
- server/chat/backend/agent/tools/terminal_exec_tool.py
- server/chat/backend/agent/tools/kubectl_onprem_tool.py
- server/utils/auth/command_policy.py
Made-with: Cursor
…eturns Made-with: Cursor
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
server/utils/auth/command_policy.py (1)
107-122: Avoid caching the failure result to limit the fail-open blast radius.
_fetchcurrently swallows any exception and returns empty rule lists plusListStates(False, False), and_get_cachedthen unconditionally writes that result into_cachewith a 30s TTL. A single transient DB error therefore pins the org to "no enforcement" for up to 30s on every worker that hits the path, even after DB recovery — and a partial failure (rules load, preferences read raises) silently drops enforcement for 30s despite having valid rules.Signal the failure out of
_fetchand skip the cache write in the failure path so the next call retries. Tag ``.♻️ Proposed refactor
-def _fetch(org_id: str) -> Tuple[List[PolicyRule], List[PolicyRule], ListStates]: +def _fetch(org_id: str) -> Optional[Tuple[List[PolicyRule], List[PolicyRule], ListStates]]: """Load policy rules and list states for *org_id*.""" allow_rules: List[PolicyRule] = [] deny_rules: List[PolicyRule] = [] states = ListStates(allowlist_enabled=False, denylist_enabled=False) try: # ... fetch rules + preferences ... + return allow_rules, deny_rules, states except Exception: logger.exception("Failed to fetch command policies for org %s, fail-open", org_id) - - return allow_rules, deny_rules, states + return None @@ def _get_cached(org_id: str) -> Tuple[List[PolicyRule], List[PolicyRule], ListStates]: entry = _cache.get(org_id) if entry is not None: allow, deny, states, ts = entry if time.monotonic() - ts < _CACHE_TTL: return allow, deny, states - allow, deny, states = _fetch(org_id) - _cache[org_id] = (allow, deny, states, time.monotonic()) - return allow, deny, states + result = _fetch(org_id) + if result is None: + # Don't poison the cache with a failure snapshot; retry on next call. + return [], [], ListStates(False, False) + _cache[org_id] = (*result, time.monotonic()) + return result🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/utils/auth/command_policy.py` around lines 107 - 122, The current flow swallows exceptions in _fetch and unconditionally writes the empty "fail-open" result into _cache in _get_cached; change _fetch to surface failures (raise the exception) instead of returning empty rules, then update _get_cached to call _fetch inside a try/except and only write to _cache when _fetch succeeds; on _fetch exception, do not overwrite the cache—either return the existing cached entry if one was present (allow, deny, states from entry) or re-raise/log the error so callers can handle the transient failure. Update references to _cache, _CACHE_TTL, _fetch, and _get_cached accordingly.server/routes/command_policies.py (1)
124-127: Prefer typed exception for unique-constraint detection.Substring-matching
str(e)for"unique"/"duplicate"is fragile: driver upgrades, locale changes, or a different constraint whose error message happens to contain "duplicate" will route incorrectly. The table definesUNIQUE(org_id, mode, pattern)on line 393 ofdb_utils.py, sopsycopg2.errors.UniqueViolationwill reliably fire. The project already uses this pattern inadmin_routes.pyline 167.♻️ Proposed refactor
+import psycopg2 @@ try: with db_pool.get_admin_connection() as conn: with conn.cursor() as cur: cur.execute( "INSERT INTO org_command_policies " "(org_id, mode, pattern, description, priority, updated_by) " "VALUES (%s, %s, %s, %s, %s, %s) RETURNING id", (org_id, mode, pattern, description, priority, user_id), ) new_id = cur.fetchone()[0] conn.commit() - except Exception as e: - if "unique" in str(e).lower() or "duplicate" in str(e).lower(): - return jsonify({"error": "A rule with this mode and pattern already exists"}), 409 - raise + except psycopg2.errors.UniqueViolation: + return jsonify({"error": "A rule with this mode and pattern already exists"}), 409🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/command_policies.py` around lines 124 - 127, The current except block uses string-matching on Exception which is fragile; replace it by catching the specific psycopg2.errors.UniqueViolation exception and handling that with the 409 response, re-raising other errors. Import psycopg2.errors.UniqueViolation at top of the module, change the broad "except Exception as e:" to "except UniqueViolation as e:" (or add an additional except UniqueViolation before the general except) in the rule-creation/insert handler where the UNIQUE(org_id, mode, pattern) constraint is enforced, and keep the generic exception re-raise for all other errors (mirroring the pattern used in admin_routes.py).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@server/routes/command_policies.py`:
- Around line 124-127: The current except block uses string-matching on
Exception which is fragile; replace it by catching the specific
psycopg2.errors.UniqueViolation exception and handling that with the 409
response, re-raising other errors. Import psycopg2.errors.UniqueViolation at top
of the module, change the broad "except Exception as e:" to "except
UniqueViolation as e:" (or add an additional except UniqueViolation before the
general except) in the rule-creation/insert handler where the UNIQUE(org_id,
mode, pattern) constraint is enforced, and keep the generic exception re-raise
for all other errors (mirroring the pattern used in admin_routes.py).
In `@server/utils/auth/command_policy.py`:
- Around line 107-122: The current flow swallows exceptions in _fetch and
unconditionally writes the empty "fail-open" result into _cache in _get_cached;
change _fetch to surface failures (raise the exception) instead of returning
empty rules, then update _get_cached to call _fetch inside a try/except and only
write to _cache when _fetch succeeds; on _fetch exception, do not overwrite the
cache—either return the existing cached entry if one was present (allow, deny,
states from entry) or re-raise/log the error so callers can handle the transient
failure. Update references to _cache, _CACHE_TTL, _fetch, and _get_cached
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0494e0e5-118a-4e03-85fc-d64428b54169
📒 Files selected for processing (9)
client/src/lib/services/command-policies.tsserver/chat/backend/agent/prompt/composer.pyserver/chat/backend/agent/tools/cloud_exec_tool.pyserver/chat/backend/agent/tools/kubectl_onprem_tool.pyserver/chat/backend/agent/tools/tailscale_ssh_tool.pyserver/chat/backend/agent/tools/terminal_exec_tool.pyserver/routes/command_policies.pyserver/utils/auth/command_policy.pyserver/utils/db/db_utils.py
✅ Files skipped from review due to trivial changes (2)
- server/utils/db/db_utils.py
- client/src/lib/services/command-policies.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- server/chat/backend/agent/tools/kubectl_onprem_tool.py
- server/chat/backend/agent/tools/cloud_exec_tool.py
- server/chat/backend/agent/tools/tailscale_ssh_tool.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
server/routes/command_policies.py (2)
148-159:⚠️ Potential issue | 🟡 MinorValidate
priorityon updates too.
create_policynow returns 400 for non-integer priority, butupdate_policystill passes arbitrary request data into the integer column and can 500 on values like"high".Proposed fix
if field in data: if field == "mode" and data[field] not in ("allow", "deny"): return jsonify({"error": "mode must be 'allow' or 'deny'"}), 400 if field == "pattern": err = validate_pattern(data[field]) if err: return jsonify({"error": "Invalid regex pattern"}), 400 + if field == "priority": + try: + data[field] = int(data[field]) + except (TypeError, ValueError): + return jsonify({"error": "priority must be an integer"}), 400 updates.append(f"{col} = %s") params.append(data[field])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/command_policies.py` around lines 148 - 159, update_policy's field loop currently appends whatever is in data["priority"] into params without checking type, which can cause a 500 for non-integer values; add the same integer validation used in create_policy: when field == "priority" ensure data[field] is an int (or can be parsed to int) and return a 400 with an error if not, before appending to updates and params. Locate the loop that iterates over ("mode","pattern","description","priority","enabled") in update_policy and add a branch that validates priority, similar to the validation for mode and pattern.
252-279:⚠️ Potential issue | 🟠 MajorAvoid enabling a list before first-enable seeds are committed.
Line 254 flips enforcement on before the seed insert transaction. If seeding fails, the org can be left with allowlist/denylist enabled but no seed rules, and cache invalidation is skipped.
Safer ordering
- store_user_preference(org_key, pref_key, "on" if enabled else "off") - # Auto-seed rules on first enable if list is empty if enabled: mode = "allow" if list_name == "allowlist" else "deny" from utils.db.connection_pool import db_pool @@ seed["description"], seed["priority"], user_id), ) conn.commit() + + store_user_preference(org_key, pref_key, "on" if enabled else "off") invalidate_cache(org_id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/command_policies.py` around lines 252 - 279, The current flow flips the preference via store_user_preference (pref_key) before seeding DB rules, so if seeding fails the list is enabled without rules and cache invalidation may be skipped; change the ordering so seeding happens inside the admin DB transaction first (use db_pool.get_admin_connection(), cursor cur, SELECT COUNT(*) on org_command_policies and INSERT seed rows from get_seed_rules() for the computed mode) and only after a successful commit call store_user_preference(pref_key, "on"/"off") (and then trigger any cache invalidation) so the preference is only enabled when seeding has committed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/src/components/SecuritySettings.tsx`:
- Around line 39-58: The Switch and icon-only Button lack accessible names;
update the Switch used in the rule row (the Switch component that calls
onToggle(rule)) to include an aria-label or aria-labelledby that references the
rule (e.g., include rule.pattern or rule.description) so screen readers know
which policy is being toggled, and update the delete Button (the Button that
calls onDelete(rule.id)) to include an aria-label like "Delete policy
{rule.pattern}" or "Delete {rule.description}"; apply the same pattern to any
list-level toggles referenced elsewhere (the other Switch instances and trash
icon Buttons around lines 480-526) so every toggle and icon-only control exposes
a clear accessible name tied to the rule.
In `@server/routes/command_policies.py`:
- Around line 330-356: The ON CONFLICT must match the partial unique index
predicate; replace the single pref_upsert SQL with two upsert statements that
include the correct WHERE clause for the partial indexes and call the
appropriate one based on org_id being NULL or NOT NULL. Concretely, create
pref_upsert_with_org using "ON CONFLICT (user_id, org_id, preference_key) WHERE
org_id IS NOT NULL DO UPDATE ..." and pref_upsert_null_org using "ON CONFLICT
(user_id, preference_key) WHERE org_id IS NULL DO UPDATE ...", then in the block
that currently references pref_upsert (the cur.execute calls that set
"command_policy_allowlist/denylist/active_template") use the matching SQL
(pref_upsert_with_org if org_id is not null, otherwise pref_upsert_null_org);
update the same pattern at the later occurrence noted in the comment.
---
Duplicate comments:
In `@server/routes/command_policies.py`:
- Around line 148-159: update_policy's field loop currently appends whatever is
in data["priority"] into params without checking type, which can cause a 500 for
non-integer values; add the same integer validation used in create_policy: when
field == "priority" ensure data[field] is an int (or can be parsed to int) and
return a 400 with an error if not, before appending to updates and params.
Locate the loop that iterates over
("mode","pattern","description","priority","enabled") in update_policy and add a
branch that validates priority, similar to the validation for mode and pattern.
- Around line 252-279: The current flow flips the preference via
store_user_preference (pref_key) before seeding DB rules, so if seeding fails
the list is enabled without rules and cache invalidation may be skipped; change
the ordering so seeding happens inside the admin DB transaction first (use
db_pool.get_admin_connection(), cursor cur, SELECT COUNT(*) on
org_command_policies and INSERT seed rows from get_seed_rules() for the computed
mode) and only after a successful commit call store_user_preference(pref_key,
"on"/"off") (and then trigger any cache invalidation) so the preference is only
enabled when seeding has committed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0bf78f9f-67bd-4183-9f4e-5b7ef54b941b
📒 Files selected for processing (4)
client/src/components/SecuritySettings.tsxclient/src/lib/services/command-policies.tsserver/routes/command_policies.pyserver/utils/db/db_utils.py
✅ Files skipped from review due to trivial changes (1)
- client/src/lib/services/command-policies.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- server/utils/db/db_utils.py
Zarlanx
approved these changes
Apr 19, 2026
This was referenced Apr 21, 2026
This was referenced May 5, 2026
Merged
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
3 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.
Adds an organization-scoped command policy system that evaluates every agent-executed command against configurable allow and deny regex rules before execution.
What's included
command_policy.py) — regex-based allow/deny evaluation with caching, compound command splitting, and fail-closed semanticscommand_policies.py) — CRUD for rules, list toggling, template apply/remove, and a test-command endpointorg_command_policiestable with org-scoped indexBoth lists default to disabled. Policies are org-scoped (shared across all users in an organization).
Summary by CodeRabbit