fix(mcp-crud-ui): address greptile 3/5 review round 2

- check READ_RE before DELETE_RE in classifyToolOp so tools like
  get_removed_entries are not silently blocked by delete-by-default
- expand undefined (allow-all) to full tool name list instead of
  collapsing to [] (allow-none) in MCPToolPermissions and mcp_tool_configuration
- log OAuth credential fetch failures instead of silently swallowing them
This commit is contained in:
Ishaan Jaffer 2026-03-11 18:55:34 -07:00
parent 409c5f795f
commit c3540b65d4
4 changed files with 17 additions and 5 deletions

View file

@ -100,7 +100,12 @@ if MCP_AVAILABLE:
if cred and cred.get("access_token"):
return {"Authorization": f"Bearer {cred['access_token']}"}
except Exception:
pass
verbose_logger.debug(
"Failed to fetch OAuth credential for user %s / server %s",
user_id,
server_id,
exc_info=True,
)
return None
def _create_tool_response_objects(tools, server_mcp_info):

View file

@ -86,7 +86,10 @@ const MCPToolPermissions: React.FC<MCPToolPermissionsProps> = ({
}, [servers]);
const handleCrudPanelChange = (serverId: string, allowed: string[] | undefined) => {
onChange({ ...toolPermissions, [serverId]: allowed ?? [] });
// `undefined` from the panel means "allow all" — expand to the full tool list
// rather than collapsing to [] ("allow none").
const resolved = allowed ?? (serverTools[serverId] || []).map((t) => t.name);
onChange({ ...toolPermissions, [serverId]: resolved });
};
const handleSelectAll = (serverId: string) => {

View file

@ -456,7 +456,7 @@ const MCPToolConfiguration: React.FC<MCPToolConfigurationProps> = ({
<McpCrudPermissionPanel
tools={tools}
value={allowedTools}
onChange={(allowed) => onAllowedToolsChange(allowed ?? [])}
onChange={(allowed) => onAllowedToolsChange(allowed ?? tools.map((t) => t.name))}
/>
)}

View file

@ -15,21 +15,25 @@ export interface MCPToolEntry {
* the name alone yields no match. This prevents incidental phrasing in
* free-form descriptions (e.g. "removes noise from…") from promoting a safe
* tool into a high-risk bucket.
*
* READ is checked before DELETE/UPDATE so that tools like `get_removed_entries`
* or `list_deleted_items` where the primary verb is a read operation are
* not silently blocked by the delete-by-default policy for new servers.
*/
export function classifyToolOp(name: string, description = ""): CrudOp {
const nameLower = name.toLowerCase();
if (READ_RE.test(nameLower)) return "read";
if (DELETE_RE.test(nameLower)) return "delete";
if (UPDATE_RE.test(nameLower)) return "update";
if (CREATE_RE.test(nameLower)) return "create";
if (READ_RE.test(nameLower)) return "read";
// Only consult description when the name is unrecognised.
if (description) {
const descLower = description.toLowerCase();
if (READ_RE.test(descLower)) return "read";
if (DELETE_RE.test(descLower)) return "delete";
if (UPDATE_RE.test(descLower)) return "update";
if (CREATE_RE.test(descLower)) return "create";
if (READ_RE.test(descLower)) return "read";
}
return "unknown";