feat: org-level tool permissions with admin UI - #375
Merged
Conversation
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughAdds an org-scoped tool permissions system: centralized registry, org_tool_permissions DB table with RLS, backend APIs (list/toggle/seed), client service + Next.js forwarders, SecuritySettings UI for toggles, runtime threading of permitted tools into State, and gate/confirmation identifier refinements. ChangesOrganization Tool Permissions System
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
damianloch
marked this pull request as draft
May 8, 2026 22:09
Actions running in background contexts were blocked by gate_action for all
destructive tools (MCP writes, Terraform apply/destroy, Bitbucket merges).
This adds an org-level tool permissions system that allows admins to
pre-approve specific tools so actions can execute them without HITL.
- Refactor tool_names for granularity (mcp_{server}_{tool}, iac_tool:{action}, bitbucket:{action})
- Add TOOL_REGISTRY with 28 gated tools across 5 connectors
- Add org_tool_permissions DB table with RLS
- Add _is_org_tool_permitted() bypass at top of gate_action (background only)
- Propagate permitted_tools through executor -> trigger_metadata -> State
- Add CRUD API routes (GET list, PUT toggle, POST seed)
- Add frontend proxy routes and service layer
Per-connector collapsible sections with toggle switches, risk badges, and auto-seeding on first load. Optimistic UI updates on toggle.
- Split POST /seed into its own route file (was incorrectly on the main route) - Remove incorrect POST from main route (only GET needed) - Add empty state message when tool permissions fail to load
…lish - Mount docker.sock in celery_worker and chatbot for GitHub MCP - Fix Spinnaker trigger_pipeline to respect permitted_tools in background - Add wildcard matching (mcp_github_*) to _is_org_tool_permitted - Add all missing GitHub MCP tools to registry - Add connector logos to tool permissions UI - Filter tool permissions to only show connected connectors - Remove risk badges from UI
damianloch
force-pushed
the
feat/tool-permissions
branch
from
May 9, 2026 13:26
2145bbd to
ef26b35
Compare
damianloch
marked this pull request as ready for review
May 9, 2026 13:27
- Extract nested ternary into IIFE with if/return in SecuritySettings - Define _ERR_NO_ORG constant for duplicated literal in tool_permissions
![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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
server/services/actions/executor.py (1)
86-90:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep
permitted_toolsout of generictrigger_metadata.This metadata object is reused downstream as chat/session metadata, and
_execute_background_chat()now also trusts it to populateState.permitted_tools. That makes a generic, persisted blob the source of truth for a confirmation-bypass decision and leaks the org allowlist into session state. Pass the allowlist in a worker-only field/arg instead, or at minimum only honor it forsource == "action".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/services/actions/executor.py` around lines 86 - 90, The trigger_meta dictionary must not include the org allowlist; remove the "permitted_tools": _fetch_org_permitted_tools(org_id, user_id) entry from trigger_meta and instead pass that allowlist as a separate worker-only argument to the background executor (or worker invocation) so it is not persisted into generic metadata; alternatively, if you cannot change the call signature, ensure _execute_background_chat and any code that sets State.permitted_tools only reads the allowlist when trigger_meta.get("source") == "action" (use _fetch_org_permitted_tools in the caller and wire it into the worker-specific param rather than into trigger_meta).server/chat/backend/agent/tools/iac/iac_commands_tool.py (1)
103-109:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSerialize parsed var values with JSON/HCL syntax, not Python repr.
After
json.loads(vars), this buildskey=valuewith Python formatting. That turns booleans intoTrue/Falseand objects/lists into single-quoted Python reprs, which breaks valid inputs like{"enabled": true}or nested maps. Serialize each parsed value withjson.dumps()before quoting the fullkey=valuetoken.Suggested fix
if vars: try: vars_dict = json.loads(vars) if isinstance(vars, str) else vars for key, value in vars_dict.items(): - plan_command += f" -var={shlex.quote(f'{key}={value}')}" + serialized_value = json.dumps(value) + plan_command += f" -var={shlex.quote(f'{key}={serialized_value}')}" except (json.JSONDecodeError, TypeError): plan_command += f" -var={shlex.quote(str(vars))}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/chat/backend/agent/tools/iac/iac_commands_tool.py` around lines 103 - 109, The loop building plan_command uses Python repr for values after json.loads(vars), which breaks Terraform var syntax; update the code in the vars handling block (the json.loads branch and its iteration over vars_dict.items()) to serialize each parsed value with json.dumps(...) when constructing the key=value token before passing to shlex.quote (e.g., build f'{key}={json.dumps(value)}' with a compact JSON style and then shlex.quote that string). Leave the existing except fallback to handle unparseable inputs but avoid using Python repr there as well (serialize the fallback value with json.dumps or str appropriately before quoting).server/chat/backend/agent/tools/mcp_tools.py (1)
1238-1249:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep MCP error notifications keyed to the new server-scoped tool name.
Line 1238 switched to
mcp_{server_type}_{original_tool_name}, but error paths still emit non-scoped keys. That desynchronizes start/completion/error correlation for failed calls.Suggested fix
- send_tool_error(original_tool_name, error_msg) + send_tool_error(tool_name, error_msg) ... - send_tool_error(f"mcp_{original_tool_name}", str(e)) + send_tool_error(tool_name, str(e))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/chat/backend/agent/tools/mcp_tools.py` around lines 1238 - 1249, The MCP code now prefixes tools with the server-scoped name (tool_name = f"mcp_{server_type}_{original_tool_name}") but the error/notification paths still emit the old non-scoped keys, breaking correlation; update all notification/error calls to use the scoped tool_name and the generated tool_call_id (e.g., in send_tool_start, the except block handling start_notify_err, any send_tool_error/send_tool_completion calls, and related exception handlers) so they emit the same mcp_{server_type}_{...} key and tool_call_id used when creating the signature_hash and tool_call_id.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@client/src/components/SecuritySettings.tsx`:
- Around line 327-342: The effect currently calls fetchToolPerms() for all users
causing admin-only GET/POSTs; update the useEffect that invokes fetchPolicies(),
fetchTemplates(), fetchToolPerms() to include the admin flag in its dependency
array and only call fetchToolPerms() when admin is true (i.e., if (admin)
fetchToolPerms()). This ensures fetchToolPerms (and its internal seed call) is
skipped for non-admin sessions; ensure fetchToolPerms, setToolPermsLoading and
related state are unchanged otherwise.
- Around line 435-450: handleToggleTool is doing optimistic updates for every
click which allows concurrent toggles to be sent out-of-order; serialize
per-tool writes or disable the switch while the request is in-flight. Add a
per-tool inFlight map keyed by toolKey (or a boolean flag on the tool objects)
and check it at the top of handleToggleTool to ignore/disable further toggles
until the pending toggle completes; set the flag before calling
toolPermissionService.toggleTool and clear it in both success and catch
branches, and on error call fetchToolPerms (or revert the optimistic update) as
currently done. Ensure the UI switch reads the in-flight flag to render disabled
state and keep references to handleToggleTool, toolPermissionService.toggleTool,
setToolPerms, and fetchToolPerms when implementing.
In `@docker-compose.prod-local.yml`:
- Around line 265-267: Remove the host Docker socket mount from the
celery_worker and chatbot service definitions in docker-compose.prod-local.yml:
locate the volume entry that mounts /var/run/docker.sock into those services
(referenced as the celery_worker and chatbot service blocks) and delete that
specific volume mapping (the "- /var/run/docker.sock:/var/run/docker.sock"
entry); if those services need to interact with Docker, replace the mount with a
safer alternative (e.g., use a dedicated Docker-in-Docker sidecar, remote Docker
API with proper auth, or an explicit CI/CD job) rather than mounting the host
socket.
In `@docker-compose.yaml`:
- Around line 259-263: Remove the host Docker socket mount from the affected
services: edit the docker-compose service blocks for celery_worker and chatbot
and delete the volume entry that mounts /var/run/docker.sock (the line "-
/var/run/docker.sock:/var/run/docker.sock"); ensure no other service volumes
reference the socket and, if those services require Docker access, replace the
socket mount with a safer alternative (e.g., use a dedicated Docker-in-Docker
service, remote Docker API with proper credentials, or refactor to avoid direct
Docker control) so the compose file no longer exposes the host Docker daemon.
In `@server/chat/backend/agent/tools/spinnaker_rca_tool.py`:
- Around line 215-218: The background-mode precheck in trigger_pipeline uses a
hard check against permitted_tools and "spinnaker_rca", which rejects wildcard
permissions; replace this local membership test with the shared permission logic
by invoking the existing gate_action() (or the shared org-permission matcher
used elsewhere) instead of checking permitted_tools directly so permission
resolution (including wildcards) is consistent with the rest of the
background-bypass flow; update the block that references state and
permitted_tools to call gate_action("trigger_pipeline", state) (or the shared
matcher) and return the same error/json only if that call denies the action.
In `@server/chat/background/task.py`:
- Around line 1170-1172: The code can raise TypeError when trigger_metadata
contains "permitted_tools": None; change the permitted_tools expression to
normalize None to an empty iterable before casting to set (e.g., replace
set(trigger_metadata.get("permitted_tools", [])) with
set(trigger_metadata.get("permitted_tools") or []) so that None becomes []);
keep the surrounding conditional (the existing "if trigger_metadata else None")
intact and update the argument passed where permitted_tools is constructed.
In `@server/routes/tool_permissions.py`:
- Around line 22-24: The list_permissions route is currently only guarded by
`@require_auth_only`; replace that with the RBAC decorator
`@require_permission`('admin') so the endpoint is admin-only like the other
permission-management routes — update the decorator on the list_permissions
function to use `@require_permission`('admin') (removing or not using
`@require_auth_only`) to enforce admin RBAC per routing guidelines.
- Around line 61-63: The route currently coerces enabled with enabled =
bool(body.get("enabled", False)), which treats strings like "false" as True;
instead read the JSON into body with request.get_json(...) and validate that
body.get("enabled") is strictly a bool (e.g., isinstance(value, bool)) before
assigning to enabled, and if it's missing or not a boolean return a
400/validation error response; update the code surrounding body and enabled in
server/routes/tool_permissions.py (the request.get_json call and enabled
assignment) to enforce this strict boolean check and respond accordingly.
In `@server/utils/auth/tool_registry.py`:
- Around line 22-24: The registry entries in tool_registry.py use incorrect keys
for the GitHub rerun tools; update the keys "mcp_github_rerun_workflow" and
"mcp_github_rerun_workflow_failed_jobs" to match the actual tool names defined
in mcp_tools.py ("rerun_workflow_run" and "rerun_failed_jobs" respectively)
while keeping their connector, label, risk, and default attributes unchanged so
org-level permission checks (in the registry) align with the allowed tool names
used at execution time.
---
Outside diff comments:
In `@server/chat/backend/agent/tools/iac/iac_commands_tool.py`:
- Around line 103-109: The loop building plan_command uses Python repr for
values after json.loads(vars), which breaks Terraform var syntax; update the
code in the vars handling block (the json.loads branch and its iteration over
vars_dict.items()) to serialize each parsed value with json.dumps(...) when
constructing the key=value token before passing to shlex.quote (e.g., build
f'{key}={json.dumps(value)}' with a compact JSON style and then shlex.quote that
string). Leave the existing except fallback to handle unparseable inputs but
avoid using Python repr there as well (serialize the fallback value with
json.dumps or str appropriately before quoting).
In `@server/chat/backend/agent/tools/mcp_tools.py`:
- Around line 1238-1249: The MCP code now prefixes tools with the server-scoped
name (tool_name = f"mcp_{server_type}_{original_tool_name}") but the
error/notification paths still emit the old non-scoped keys, breaking
correlation; update all notification/error calls to use the scoped tool_name and
the generated tool_call_id (e.g., in send_tool_start, the except block handling
start_notify_err, any send_tool_error/send_tool_completion calls, and related
exception handlers) so they emit the same mcp_{server_type}_{...} key and
tool_call_id used when creating the signature_hash and tool_call_id.
In `@server/services/actions/executor.py`:
- Around line 86-90: The trigger_meta dictionary must not include the org
allowlist; remove the "permitted_tools": _fetch_org_permitted_tools(org_id,
user_id) entry from trigger_meta and instead pass that allowlist as a separate
worker-only argument to the background executor (or worker invocation) so it is
not persisted into generic metadata; alternatively, if you cannot change the
call signature, ensure _execute_background_chat and any code that sets
State.permitted_tools only reads the allowlist when trigger_meta.get("source")
== "action" (use _fetch_org_permitted_tools in the caller and wire it into the
worker-specific param rather than into trigger_meta).
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8a261f09-7126-4518-a6e3-0a264ec79ffd
📒 Files selected for processing (22)
client/src/app/api/org/tool-permissions/[toolKey]/route.tsclient/src/app/api/org/tool-permissions/route.tsclient/src/app/api/org/tool-permissions/seed/route.tsclient/src/components/SecuritySettings.tsxclient/src/lib/services/tool-permissions.tsdocker-compose.prod-local.ymldocker-compose.yamlserver/chat/backend/agent/tools/bitbucket/branches_tool.pyserver/chat/backend/agent/tools/bitbucket/pipelines_tool.pyserver/chat/backend/agent/tools/bitbucket/prs_tool.pyserver/chat/backend/agent/tools/bitbucket/repos_tool.pyserver/chat/backend/agent/tools/iac/iac_commands_tool.pyserver/chat/backend/agent/tools/mcp_tools.pyserver/chat/backend/agent/tools/spinnaker_rca_tool.pyserver/chat/backend/agent/utils/state.pyserver/chat/background/task.pyserver/main_compute.pyserver/routes/tool_permissions.pyserver/services/actions/executor.pyserver/utils/auth/command_gate.pyserver/utils/auth/tool_registry.pyserver/utils/db/db_utils.py
Move permitted_tools resolution into task.py so RCA investigations and webhook-triggered background chats also get org tool permissions, not just actions dispatched via the executor.
Accidentally removed during earlier docker.sock edits.
These are local dev changes, not part of the tool permissions feature.
- Fix registry key mismatch: rerun_workflow → rerun_workflow_run, rerun_workflow_failed_jobs → rerun_failed_jobs (aligns with MCP tool names) - Fix same mismatch in _DESTRUCTIVE_MCP_TOOLS set - Remove permitted_tools from trigger_metadata (fetch fresh from DB instead) - Use shared _is_org_tool_permitted matcher in spinnaker_rca_tool - Guard fetchToolPerms behind admin check to prevent 403s for non-admins - Add per-tool in-flight guard to prevent toggle race conditions - Fix Terraform var serialization with json.dumps for non-string values
- Remove unused trigger_metadata param from _resolve_permitted_tools - Extract renderConnectorGroup to reduce function nesting depth below 4
- Upgrade GET /tool-permissions to require admin RBAC (was require_auth_only) - Validate enabled field as strict boolean instead of coercing
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
- Remove is_background guard from _is_org_tool_permitted - Fetch permitted_tools in foreground chatbot State creation - Log warning on permission fetch failure instead of silent pass - Collapse all connector groups by default in security settings
damianloch
changed the title
beng360
reviewed
May 9, 2026
Contributor
|
Suggestion: Split permissions into tiered groups per connector Right now every tool is a flat toggle. I'd group them into tiers so admins can bulk-enable sensible defaults without reading 30+ individual toggles. Each tier maps to one toggle in the UI, but individual tools can still be overridden if you expand the group. GitHub — 3 tiersTier 1: "Read & Comment" (default: ON) Low-risk, additive-only, doesn't touch code:
Tier 2: "Branch & PR" (default: ON) Creates artifacts but nothing irreversible:
Tier 3: "Destructive" (default: OFF) Hard to reverse, affects main/prod:
Kill the Terraform — 3 tiersTier 1: "Plan" (default: ON)
Tier 2: "Apply" (default: OFF)
Tier 3: "Destroy" (default: OFF, scary warning)
Notion — 3 tiersTier 1: "Read & Export" (default: ON)
Tier 2: "Create & Update" (default: OFF)
Tier 3: "Destructive" (default: OFF)
UI idea: Each connector shows its tiers as collapsible sub-groups with a "enable tier" toggle at the group header. Expand to override individual tools within a tier. The tier toggle is just sugar over bulk-setting the individual tools. |
- Distinguish None vs empty set in _is_org_tool_permitted (None = not configured, empty set = nothing permitted) - Support :* wildcard delimiter for Bitbucket/IaC tool patterns - Add logger.warning to bare except blocks in command_gate and task.py - Add Redis cache invalidation: admin toggle sets dirty flag, running chats refresh permissions on next tool call
Contributor
Author
I disagree I think admins want that granularity even if there are long lists to choose from. Github does the same thing when creating the app as well there are hundreds of permissions to choose from but the priority is security so its the solution enterprises like |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
server/chat/backend/agent/tools/mcp_tools.py (2)
44-54:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBroaden destructive detection to cover the GitHub write-style tools.
gate_action()only runs whenis_destructive_mcp_tool()returnsTrue, but this matcher still misses mutating tools you expose later inallowed_github_mcp_tools, likeissue_write,label_write,sub_issue_write,pull_request_review_write,run_workflow, andmark_all_notifications_read. Those paths will bypass both confirmation and org-level toggles entirely.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/chat/backend/agent/tools/mcp_tools.py` around lines 44 - 54, The destructive-tool matcher currently only checks _DESTRUCTIVE_MCP_TOOLS, so tools listed in allowed_github_mcp_tools (e.g., issue_write, label_write, sub_issue_write, pull_request_review_write, run_workflow, mark_all_notifications_read) bypass gate_action(); update is_destructive_mcp_tool() to also treat write-style and other mutating tool names as destructive (for example by checking for suffixes like "_write" and explicitly including "run_workflow" and "mark_all_notifications_read"), or expand _DESTRUCTIVE_MCP_TOOLS to include those specific names, so gate_action() will run for all mutating GitHub MCP tools.
1238-1413:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep MCP error notifications on the new server-scoped tool key.
After switching the wrapper key to
mcp_{server_type}_{original_tool_name}, the error paths still emitoriginal_tool_name/mcp_{original_tool_name}. A failed MCP call now starts as one tool and errors as another, which breaks correlation for the in-flight tool entry and any downstream status tracking.🐛 Minimal fix
- send_tool_error(original_tool_name, error_msg) + send_tool_error(tool_name, error_msg) ... - send_tool_error(f"mcp_{original_tool_name}", str(e)) + send_tool_error(tool_name, str(e))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/chat/backend/agent/tools/mcp_tools.py` around lines 1238 - 1413, The issue is that error/notification calls still use original_tool_name (or f"mcp_{original_tool_name}") after you switched to the server-scoped key tool_name (f"mcp_{server_type}_{original_tool_name}"), breaking correlation; update all notification/error usages to use the unified tool_name and include tool_call_id where available. Specifically, replace send_tool_error(original_tool_name, ...) and send_tool_error(f"mcp_{original_tool_name}", ...) with send_tool_error(tool_name, ...), and ensure the server-restart notification (where error_msg is created) and the enhanced GitHub error notification both call send_tool_error(tool_name, <message>) (and include tool_call_id if the send_* signature supports it); also keep send_tool_start and send_tool_completion using tool_name so all notifications consistently use the same identifier.
♻️ Duplicate comments (2)
server/utils/auth/tool_registry.py (1)
33-33:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAvoid a wildcard that can auto-permit future GitHub write tools.
Once an admin enables
mcp_github_*, any newly discovered GitHub MCP operation can bypass confirmation without another review, including tooling the registry does not know how to classify yet. This should stay explicit, or the matcher needs a hard stop for futurehigh/criticalactions plus a much stronger warning surface.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/utils/auth/tool_registry.py` at line 33, The registry entry using the wildcard key "mcp_github_*" is too permissive; replace it with explicit keys for each known GitHub MCP operation (i.e., enumerate specific identifiers instead of the wildcard) in the tool registry mapping (where "mcp_github_*" is defined) or, if you must support pattern matching, implement a strict matcher that explicitly rejects or requires admin confirmation for any future actions classified as "high" or "critical" and surfaces a strong warning for unknown classifications; update the mapping logic in the registry to stop auto-permitting unknown GitHub write tools and ensure any pattern-match path has that hard stop and warning behavior.client/src/components/SecuritySettings.tsx (1)
751-753:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't hide connectors that still have active permissions.
This filter removes disconnected providers from the page, but their
enabled=truerows still live in the backend and are still honored when the provider is reconnected. Keep a connector visible when any of its tools are enabled, or surface an explicit warning/cleanup path instead.Minimal mitigation
- {Object.entries(toolPerms) - .filter(([connector]) => ALWAYS_SHOW_CONNECTORS.has(connector) || connectedProviders.has(connector)) + {Object.entries(toolPerms) + .filter(([connector, tools]) => + ALWAYS_SHOW_CONNECTORS.has(connector) || + connectedProviders.has(connector) || + tools.some((tool) => tool.enabled) + ) .map(([connector, tools]) => renderConnectorGroup(connector, tools))}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/src/components/SecuritySettings.tsx` around lines 751 - 753, The current filter hides connectors unless they are in ALWAYS_SHOW_CONNECTORS or present in connectedProviders, which removes connectors that still have active backend permissions; update the filter on toolPerms (the Object.entries(...).filter(...) before .map(renderConnectorGroup)) to also keep a connector when any of its tools are enabled (e.g., tools.some(tool => tool.enabled)), so that connectors with enabled permissions remain visible; use the existing symbols toolPerms, ALWAYS_SHOW_CONNECTORS, connectedProviders, and renderConnectorGroup to locate and change the predicate accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@client/src/components/SecuritySettings.tsx`:
- Around line 729-730: The help text under the Action Tool Permissions header
(in SecuritySettings.tsx, the <h2> "Action Tool Permissions" and its adjacent
<p>) is incomplete; update the copy in that <p> to explicitly state that
enabling a tool will skip confirmation both in interactive chats and in
background actions (e.g., "Tools enabled here can run without confirmation in
interactive chats and background actions") so admins understand the full bypass
scope.
In `@server/routes/tool_permissions.py`:
- Around line 22-30: The _invalidate_permissions_cache function currently
swallows all exceptions; update it to log failures instead of silent pass so
Redis cache-invalidation errors are diagnosable: catch exceptions around
get_redis_client()/rc.set and log the error (including org_id and exception
details) using the module logger (e.g., logging.getLogger(__name__) and
logger.exception or logger.error with exc_info=True) while preserving the
best-effort behavior (do not re-raise).
In `@server/utils/auth/command_gate.py`:
- Around line 182-196: The one-shot dirty flag (dirty_key) must be replaced with
a versioned or timestamped invalidation so other State instances can detect
updates; stop deleting the shared key. Change the logic around
dirty_key/rc.get/rc.delete to instead use a shared version key (e.g.,
tool_perms_version:{org_id}) and a per-State stored version on state (e.g.,
state.permitted_tools_version): read the current version from Redis, if it
differs from state.permitted_tools_version then refresh permissions (use
db_pool.get_connection, set_rls_context, execute the SELECT and update
state.permitted_tools) and set state.permitted_tools_version to the current
version; do not rc.delete the shared key — ensure permission mutation paths
increment or update the shared version (atomic INCR or set timestamp) so
subsequent gates will see the new version.
- Around line 197-198: The except Exception: pass swallows refresh failures and
leaves stale state.permitted_tools in memory; change the except block in the
refresh logic to log the exception (use logger.exception or similar) and clear
the cached allowlist (e.g. state.permitted_tools = set() or
state.permitted_tools.clear()) so that subsequent calls to
_is_org_tool_permitted() will fail closed; optionally also set any dirty flag
appropriately (or leave dirty=True) so callers know the permit list is invalid.
---
Outside diff comments:
In `@server/chat/backend/agent/tools/mcp_tools.py`:
- Around line 44-54: The destructive-tool matcher currently only checks
_DESTRUCTIVE_MCP_TOOLS, so tools listed in allowed_github_mcp_tools (e.g.,
issue_write, label_write, sub_issue_write, pull_request_review_write,
run_workflow, mark_all_notifications_read) bypass gate_action(); update
is_destructive_mcp_tool() to also treat write-style and other mutating tool
names as destructive (for example by checking for suffixes like "_write" and
explicitly including "run_workflow" and "mark_all_notifications_read"), or
expand _DESTRUCTIVE_MCP_TOOLS to include those specific names, so gate_action()
will run for all mutating GitHub MCP tools.
- Around line 1238-1413: The issue is that error/notification calls still use
original_tool_name (or f"mcp_{original_tool_name}") after you switched to the
server-scoped key tool_name (f"mcp_{server_type}_{original_tool_name}"),
breaking correlation; update all notification/error usages to use the unified
tool_name and include tool_call_id where available. Specifically, replace
send_tool_error(original_tool_name, ...) and
send_tool_error(f"mcp_{original_tool_name}", ...) with
send_tool_error(tool_name, ...), and ensure the server-restart notification
(where error_msg is created) and the enhanced GitHub error notification both
call send_tool_error(tool_name, <message>) (and include tool_call_id if the
send_* signature supports it); also keep send_tool_start and
send_tool_completion using tool_name so all notifications consistently use the
same identifier.
---
Duplicate comments:
In `@client/src/components/SecuritySettings.tsx`:
- Around line 751-753: The current filter hides connectors unless they are in
ALWAYS_SHOW_CONNECTORS or present in connectedProviders, which removes
connectors that still have active backend permissions; update the filter on
toolPerms (the Object.entries(...).filter(...) before
.map(renderConnectorGroup)) to also keep a connector when any of its tools are
enabled (e.g., tools.some(tool => tool.enabled)), so that connectors with
enabled permissions remain visible; use the existing symbols toolPerms,
ALWAYS_SHOW_CONNECTORS, connectedProviders, and renderConnectorGroup to locate
and change the predicate accordingly.
In `@server/utils/auth/tool_registry.py`:
- Line 33: The registry entry using the wildcard key "mcp_github_*" is too
permissive; replace it with explicit keys for each known GitHub MCP operation
(i.e., enumerate specific identifiers instead of the wildcard) in the tool
registry mapping (where "mcp_github_*" is defined) or, if you must support
pattern matching, implement a strict matcher that explicitly rejects or requires
admin confirmation for any future actions classified as "high" or "critical" and
surfaces a strong warning for unknown classifications; update the mapping logic
in the registry to stop auto-permitting unknown GitHub write tools and ensure
any pattern-match path has that hard stop and warning behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 04ee2372-d677-42e9-945c-c2e77693269b
📒 Files selected for processing (9)
client/src/components/SecuritySettings.tsxserver/chat/backend/agent/tools/iac/iac_commands_tool.pyserver/chat/backend/agent/tools/mcp_tools.pyserver/chat/backend/agent/tools/spinnaker_rca_tool.pyserver/chat/background/task.pyserver/main_chatbot.pyserver/routes/tool_permissions.pyserver/utils/auth/command_gate.pyserver/utils/auth/tool_registry.py
- Replace one-shot dirty flag with version counter (all sessions detect changes) - Fail closed on refresh error (clear permitted_tools → gate applies) - Update UI copy to clarify bypass applies to chats and background actions
Tools within each connector are now organized into risk groups (low, medium, high, critical) with section headers for easier scanning.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@client/src/components/SecuritySettings.tsx`:
- Around line 473-476: The current grouping logic (riskOrder and grouped) drops
any tool whose t.risk isn't one of ["low","medium","high","critical"]; update
the grouping in SecuritySettings.tsx so tools with unknown/misspelled risks are
not filtered out — e.g., compute a map of risks from tools, build groups in the
order of riskOrder first using tools.filter(t => t.risk === risk), then collect
any remaining risks (tools whose t.risk is not in riskOrder) and append groups
for those risk strings (or a single "unknown" group) so every tool in tools
appears in grouped and is rendered for review/toggle.
- Around line 507-513: The permission Switch controls lack an accessible name;
update the Switch component (the one using checked={tool.enabled} and
onCheckedChange calling handleToggleTool(tool.tool_key, v)) to provide a
programmatic label for assistive tech by using aria-label or aria-labelledby
that references the visible tool label (tool.label) or a generated unique id
derived from tool.tool_key, ensuring the control remains disabled when !admin or
togglingTools.has(tool.tool_key); this ties the switch to the visible span so
screen readers announce the tool name.
In `@client/src/lib/services/tool-permissions.ts`:
- Line 1: The import in this module uses a relative path for
apiGet/apiPut/apiPost; update the import to use the repo path alias instead
(e.g., change import { apiGet, apiPut, apiPost } from "./api-client" to import
from "@/lib/api-client") so it follows the frontend convention of using `@/`* →
./src/*; adjust the import statement referencing apiGet, apiPut, and apiPost
accordingly.
In `@server/routes/tool_permissions.py`:
- Around line 57-60: The response's seeded flag currently uses len(db_state) > 0
which flips true after the first saved row; instead compute seeded by checking
that all current registry tool keys are present in db_state. When building the
response (the block that returns tools_by_connector and seeded), derive the set
of registry keys from the same source used to build tools_by_connector (e.g.,
the connector/tool registry or the keys in tools_by_connector) and set seeded =
True only if set(registry_keys) ⊆ set(db_state.keys()) (or missing_keys =
registry_keys - db_state.keys() and seeded = len(missing_keys) == 0), then
return that seeded value with tools_by_connector.
In `@server/utils/auth/command_gate.py`:
- Around line 152-156: The code currently treats state.permitted_tools = None as
a permanent "disabled" sentinel and immediately returns False before calling
_maybe_refresh_permitted_tools, which makes transient refresh failures one-shot;
change the sentinel behavior so refresh is always attempted: remove the early
return when permitted is None (or replace None with an empty set() as the
fail-closed sentinel) and ensure _maybe_refresh_permitted_tools(state) is called
whenever permitted is None or stale, leaving state._perms_version unchanged on
transient errors (or set permitted_tools to set() rather than None) so future
gate checks will retry; apply the same change to the other occurrence that sets
state.permitted_tools = None (the block around state.permitted_tools assignment
in the refresh/failure path).
- Around line 192-199: The RLS context set by set_rls_context must be checked
for success before running the SELECT; if set_rls_context(...) returns False,
bail out the refresh path instead of executing the query and overwriting
state.permitted_tools/state._perms_version. Update the block that calls
set_rls_context(cur, conn, user_id, log_prefix="[Gate:refresh_perms]") so it
tests the return value and returns/raises (matching the existing bail-on-False
pattern used elsewhere in this file) when False, only running the SELECT and
assigning state.permitted_tools when set_rls_context succeeded.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: bb6e08cb-3772-4977-8efa-91d4fa47ae68
📒 Files selected for processing (4)
client/src/components/SecuritySettings.tsxclient/src/lib/services/tool-permissions.tsserver/routes/tool_permissions.pyserver/utils/auth/command_gate.py
- Replace mcp_github_* wildcard with 24 explicit tools in 3 tiers - Tier 1 "Read & Comment" (9 tools, default ON) - Tier 2 "Branch & PR" (12 tools, default ON) - Tier 3 "Destructive" (3 tools, default OFF) - Rename risk field to tier across registry/frontend - Add aria-label to permission switches for a11y - Use @/ import alias in tool-permissions service - Fix seeded check to backfill new registry tools - Move refresh before None check to self-heal transient errors - Check set_rls_context return before querying permissions
damianloch
force-pushed
the
feat/tool-permissions
branch
from
May 9, 2026 19:17
c480f4a to
f19aeae
Compare
|
beng360
approved these changes
May 9, 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
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
Adds a centralized tool permissions system that lets org admins control which destructive tools (GitHub MCP, Bitbucket, Terraform, Spinnaker, Notion) can execute without confirmation prompts. When a tool is enabled in permissions, the
gate_actionconfirmation step is bypassed in both interactive chats and background actions.org_tool_permissionstable (RLS-protected), REST API, andgate_actionbypass logicmcp_github_create_branch,bitbucket:delete_branch,iac_tool:apply)mcp_github_*) for dynamically discovered MCP toolsHow it works
API
/api/org/tool-permissions/api/org/tool-permissions/<tool_key>/api/org/tool-permissions/seedKey files
server/utils/auth/tool_registry.py— central registry of all gated toolsserver/utils/auth/command_gate.py—_is_org_tool_permitted()bypassserver/routes/tool_permissions.py— API endpointsclient/src/components/SecuritySettings.tsx— admin UIserver/main_chatbot.py— foreground chat permission loadingserver/chat/background/task.py— background chat permission loadingTest plan
Summary by CodeRabbit
New Features
Behavior Changes
Bug Fixes