diff --git a/docs/TRI_API_SECURITY_RESULTS.md b/docs/TRI_API_SECURITY_RESULTS.md new file mode 100644 index 0000000000..6a12b052d5 --- /dev/null +++ b/docs/TRI_API_SECURITY_RESULTS.md @@ -0,0 +1,40 @@ +# tri-api configuration and MCP hardening - 2026-09-30 + +Base commit: afc9d38435ad01c48af6c33c311da422c38f046e. + +## Changes +Permission configuration and MCP tool metadata own their strings after parsing. +Settings use structural JSON parsing; invalid/unreadable existing settings and +rule-capacity overflow fail closed. Missing settings retain existing defaults. +MCP discovery uses structured JSON; initialization is consumed and checked before +tool listing. Tool names are JSON-escaped. Invalid write requests return before +checkpoint side effects. Lexical file path validation and shell metacharacter +checks are tightened without claiming a complete execution sandbox. + +## Checks actually run +Zig 0.15.2 with macOS shim sysroot: + +```sh +zig test --sysroot "$SHIM" -O "$MODE" src/tri-api/tool_executor.zig +zig test --sysroot "$SHIM" -O "$MODE" src/tri-api/permissions.zig +zig test --sysroot "$SHIM" -O "$MODE" src/tri-api/mcp_client.zig +``` + +Debug, ReleaseSafe, ReleaseFast, ReleaseSmall all pass. Executor suite contains +24 tests including 9 permission tests, 3 MCP tests and 5 protocol tests; counts +overlap across suites. MCP handshake uses a local Python fixture, no API keys. +Final executor logs: reports/tri-api/security-2026-09-30/. + +Full Debug executable also builds: + +```sh +zig build-exe --sysroot "$SHIM" -O Debug --dep token_rotator \ + -Mroot=src/tri-api/main.zig -Mtoken_rotator=src/tri/token_rotator.zig +``` + +## Remaining scope +Local tests, not deployed production validation or full security certification. +Symlink/TOCTOU confinement, shell capability policy, grep path/argument policy, +MCP call-result parsing and request timeouts remain separate work. Provider +failure handling, usage parsing and context compaction are unchanged. Valid writes +retain the existing stash checkpoint design. S07 specification worktree untouched. diff --git a/reports/tri-api/security-2026-09-30/tool_executor-Debug.log b/reports/tri-api/security-2026-09-30/tool_executor-Debug.log new file mode 100644 index 0000000000..487babb8d9 --- /dev/null +++ b/reports/tri-api/security-2026-09-30/tool_executor-Debug.log @@ -0,0 +1,25 @@ +1/24 tool_executor.test.ToolName.fromString...OK +2/24 tool_executor.test.isPathSafe blocks traversal...OK +3/24 tool_executor.test.isBashAllowed blocks injection...OK +4/24 tool_executor.test.redactSecrets blocks API keys...OK +5/24 tool_executor.test.redactSecrets blocks GitHub PATs...OK +6/24 tool_executor.test.redactSecrets passes clean strings...OK +7/24 tool_executor.test.invalid writes are rejected before checkpoint side effects...OK +8/24 permissions.test.ruleMatches wildcard...OK +9/24 permissions.test.ruleMatches prefix...OK +10/24 permissions.test.ruleMatches exact...OK +11/24 permissions.test.parseRuleString...OK +12/24 permissions.test.check deny wins over allow...OK +13/24 permissions.test.check defaults...OK +14/24 permissions.test.loaded rules survive JSON and file buffer release...OK +15/24 permissions.test.invalid settings fail closed, including partially parsed allow...OK +16/24 permissions.test.rules decode escaped JSON and enforce capacity without truncation...OK +17/24 mcp_client.test.MCP tool metadata owns strings after response release...OK +18/24 mcp_client.test.invalid MCP entry rolls back tool registration...OK +19/24 mcp_client.test.MCP handshake consumes initialize before listing tools...OK +20/24 tool_protocol.test.extractField basic...OK +21/24 tool_protocol.test.extractField stop_reason...OK +22/24 tool_protocol.test.extractObject basic...OK +23/24 tool_protocol.test.unescapeString...OK +24/24 tool_protocol.test.writeJsonEscaped...OK +All 24 tests passed. diff --git a/reports/tri-api/security-2026-09-30/tool_executor-ReleaseFast.log b/reports/tri-api/security-2026-09-30/tool_executor-ReleaseFast.log new file mode 100644 index 0000000000..487babb8d9 --- /dev/null +++ b/reports/tri-api/security-2026-09-30/tool_executor-ReleaseFast.log @@ -0,0 +1,25 @@ +1/24 tool_executor.test.ToolName.fromString...OK +2/24 tool_executor.test.isPathSafe blocks traversal...OK +3/24 tool_executor.test.isBashAllowed blocks injection...OK +4/24 tool_executor.test.redactSecrets blocks API keys...OK +5/24 tool_executor.test.redactSecrets blocks GitHub PATs...OK +6/24 tool_executor.test.redactSecrets passes clean strings...OK +7/24 tool_executor.test.invalid writes are rejected before checkpoint side effects...OK +8/24 permissions.test.ruleMatches wildcard...OK +9/24 permissions.test.ruleMatches prefix...OK +10/24 permissions.test.ruleMatches exact...OK +11/24 permissions.test.parseRuleString...OK +12/24 permissions.test.check deny wins over allow...OK +13/24 permissions.test.check defaults...OK +14/24 permissions.test.loaded rules survive JSON and file buffer release...OK +15/24 permissions.test.invalid settings fail closed, including partially parsed allow...OK +16/24 permissions.test.rules decode escaped JSON and enforce capacity without truncation...OK +17/24 mcp_client.test.MCP tool metadata owns strings after response release...OK +18/24 mcp_client.test.invalid MCP entry rolls back tool registration...OK +19/24 mcp_client.test.MCP handshake consumes initialize before listing tools...OK +20/24 tool_protocol.test.extractField basic...OK +21/24 tool_protocol.test.extractField stop_reason...OK +22/24 tool_protocol.test.extractObject basic...OK +23/24 tool_protocol.test.unescapeString...OK +24/24 tool_protocol.test.writeJsonEscaped...OK +All 24 tests passed. diff --git a/reports/tri-api/security-2026-09-30/tool_executor-ReleaseSafe.log b/reports/tri-api/security-2026-09-30/tool_executor-ReleaseSafe.log new file mode 100644 index 0000000000..487babb8d9 --- /dev/null +++ b/reports/tri-api/security-2026-09-30/tool_executor-ReleaseSafe.log @@ -0,0 +1,25 @@ +1/24 tool_executor.test.ToolName.fromString...OK +2/24 tool_executor.test.isPathSafe blocks traversal...OK +3/24 tool_executor.test.isBashAllowed blocks injection...OK +4/24 tool_executor.test.redactSecrets blocks API keys...OK +5/24 tool_executor.test.redactSecrets blocks GitHub PATs...OK +6/24 tool_executor.test.redactSecrets passes clean strings...OK +7/24 tool_executor.test.invalid writes are rejected before checkpoint side effects...OK +8/24 permissions.test.ruleMatches wildcard...OK +9/24 permissions.test.ruleMatches prefix...OK +10/24 permissions.test.ruleMatches exact...OK +11/24 permissions.test.parseRuleString...OK +12/24 permissions.test.check deny wins over allow...OK +13/24 permissions.test.check defaults...OK +14/24 permissions.test.loaded rules survive JSON and file buffer release...OK +15/24 permissions.test.invalid settings fail closed, including partially parsed allow...OK +16/24 permissions.test.rules decode escaped JSON and enforce capacity without truncation...OK +17/24 mcp_client.test.MCP tool metadata owns strings after response release...OK +18/24 mcp_client.test.invalid MCP entry rolls back tool registration...OK +19/24 mcp_client.test.MCP handshake consumes initialize before listing tools...OK +20/24 tool_protocol.test.extractField basic...OK +21/24 tool_protocol.test.extractField stop_reason...OK +22/24 tool_protocol.test.extractObject basic...OK +23/24 tool_protocol.test.unescapeString...OK +24/24 tool_protocol.test.writeJsonEscaped...OK +All 24 tests passed. diff --git a/reports/tri-api/security-2026-09-30/tool_executor-ReleaseSmall.log b/reports/tri-api/security-2026-09-30/tool_executor-ReleaseSmall.log new file mode 100644 index 0000000000..487babb8d9 --- /dev/null +++ b/reports/tri-api/security-2026-09-30/tool_executor-ReleaseSmall.log @@ -0,0 +1,25 @@ +1/24 tool_executor.test.ToolName.fromString...OK +2/24 tool_executor.test.isPathSafe blocks traversal...OK +3/24 tool_executor.test.isBashAllowed blocks injection...OK +4/24 tool_executor.test.redactSecrets blocks API keys...OK +5/24 tool_executor.test.redactSecrets blocks GitHub PATs...OK +6/24 tool_executor.test.redactSecrets passes clean strings...OK +7/24 tool_executor.test.invalid writes are rejected before checkpoint side effects...OK +8/24 permissions.test.ruleMatches wildcard...OK +9/24 permissions.test.ruleMatches prefix...OK +10/24 permissions.test.ruleMatches exact...OK +11/24 permissions.test.parseRuleString...OK +12/24 permissions.test.check deny wins over allow...OK +13/24 permissions.test.check defaults...OK +14/24 permissions.test.loaded rules survive JSON and file buffer release...OK +15/24 permissions.test.invalid settings fail closed, including partially parsed allow...OK +16/24 permissions.test.rules decode escaped JSON and enforce capacity without truncation...OK +17/24 mcp_client.test.MCP tool metadata owns strings after response release...OK +18/24 mcp_client.test.invalid MCP entry rolls back tool registration...OK +19/24 mcp_client.test.MCP handshake consumes initialize before listing tools...OK +20/24 tool_protocol.test.extractField basic...OK +21/24 tool_protocol.test.extractField stop_reason...OK +22/24 tool_protocol.test.extractObject basic...OK +23/24 tool_protocol.test.unescapeString...OK +24/24 tool_protocol.test.writeJsonEscaped...OK +All 24 tests passed. diff --git a/src/tri-api/mcp_client.zig b/src/tri-api/mcp_client.zig index 55f57d2b5a..dcfa35a38b 100644 --- a/src/tri-api/mcp_client.zig +++ b/src/tri-api/mcp_client.zig @@ -44,6 +44,11 @@ pub const McpManager = struct { } } self.servers.deinit(self.allocator); + for (self.tools.items) |tool| { + self.allocator.free(tool.name); + self.allocator.free(tool.description); + self.allocator.free(tool.input_schema); + } self.tools.deinit(self.allocator); } @@ -84,7 +89,16 @@ pub const McpManager = struct { return 0; } - // Send initialized notification + const initialized = self.readResponse(server_idx) orelse return 0; + defer self.allocator.free(initialized); + const init_response = std.json.parseFromSlice(std.json.Value, self.allocator, initialized, .{}) catch return 0; + defer init_response.deinit(); + if (init_response.value != .object or init_response.value.object.get("error") != null) return 0; + const init_result = init_response.value.object.get("result") orelse return 0; + const init_id = init_response.value.object.get("id") orelse return 0; + if (init_result != .object or init_id != .integer or init_id.integer != self.next_id - 1) return 0; + + // Consume initialize before requesting tools/list so responses stay aligned. self.sendNotification(server_idx, "notifications/initialized"); // List tools @@ -105,9 +119,11 @@ pub const McpManager = struct { var params: std.ArrayList(u8) = .empty; defer params.deinit(self.allocator); - params.appendSlice(self.allocator, "{\"name\":\"") catch return null; - params.appendSlice(self.allocator, actual_name) catch return null; - params.appendSlice(self.allocator, "\",\"arguments\":") catch return null; + const encoded_name = std.json.Stringify.valueAlloc(self.allocator, actual_name, .{}) catch return null; + defer self.allocator.free(encoded_name); + params.appendSlice(self.allocator, "{\"name\":") catch return null; + params.appendSlice(self.allocator, encoded_name) catch return null; + params.appendSlice(self.allocator, ",\"arguments\":") catch return null; params.appendSlice(self.allocator, args_json) catch return null; params.appendSlice(self.allocator, "}") catch return null; @@ -125,7 +141,7 @@ pub const McpManager = struct { for (self.tools.items, 0..) |tool, i| { if (i > 0) try writer.writeByte(','); try writer.writeAll("{\"name\":\""); - try writer.writeAll(tool.name); + try proto.writeJsonEscaped(writer, tool.name); try writer.writeAll("\",\"description\":\""); try proto.writeJsonEscaped(writer, tool.description); try writer.writeAll("\",\"input_schema\":"); @@ -151,38 +167,48 @@ pub const McpManager = struct { const response = self.readResponse(server_idx) orelse return 0; defer self.allocator.free(response); - // Parse tool entries from response - // Look for "name":"..." and "description":"..." and "inputSchema":{...} - var count: u32 = 0; - const tools_needle = "\"tools\":["; - const tools_start = std.mem.indexOf(u8, response, tools_needle) orelse return 0; - var pos = tools_start + tools_needle.len; - - while (pos < response.len and response[pos] != ']') { - // Find next tool object - const name = proto.extractFieldFrom(response, pos, "name") orelse break; - const desc = proto.extractFieldFrom(response, pos, "description") orelse ""; - const schema = extractSchema(response, pos) orelse "{}"; - - // Build prefixed name: "server.tool" - const full_name = std.fmt.allocPrint(self.allocator, "{s}.{s}", .{ server_name, name }) catch break; + return self.registerTools(response, server_idx, server_name) catch 0; + } - self.tools.append(self.allocator, .{ + fn registerTools(self: *McpManager, response: []const u8, server_idx: u32, server_name: []const u8) !u32 { + const parsed = try std.json.parseFromSlice(std.json.Value, self.allocator, response, .{}); + defer parsed.deinit(); + if (parsed.value != .object) return error.InvalidResponse; + const result = parsed.value.object.get("result") orelse return error.InvalidResponse; + if (result != .object) return error.InvalidResponse; + const tools = result.object.get("tools") orelse return error.InvalidResponse; + if (tools != .array) return error.InvalidResponse; + const start = self.tools.items.len; + errdefer { + for (self.tools.items[start..]) |tool| { + self.allocator.free(tool.name); + self.allocator.free(tool.description); + self.allocator.free(tool.input_schema); + } + self.tools.items.len = start; + } + for (tools.array.items) |entry| { + if (entry != .object) return error.InvalidResponse; + const name = entry.object.get("name") orelse return error.InvalidResponse; + if (name != .string or name.string.len == 0) return error.InvalidResponse; + const desc = entry.object.get("description"); + if (desc != null and desc.? != .string) return error.InvalidResponse; + const schema = entry.object.get("inputSchema") orelse return error.InvalidResponse; + if (schema != .object) return error.InvalidResponse; + const full_name = try std.fmt.allocPrint(self.allocator, "{s}.{s}", .{ server_name, name.string }); + errdefer self.allocator.free(full_name); + const description = try self.allocator.dupe(u8, if (desc) |d| d.string else ""); + errdefer self.allocator.free(description); + const input_schema = try std.json.Stringify.valueAlloc(self.allocator, schema, .{}); + errdefer self.allocator.free(input_schema); + try self.tools.append(self.allocator, .{ .name = full_name, - .description = desc, - .input_schema = schema, + .description = description, + .input_schema = input_schema, .server_idx = server_idx, - }) catch break; - - count += 1; - - // Advance past this tool object - if (std.mem.indexOfPos(u8, response, pos + 1, "\"name\":\"")) |next| { - pos = next; - } else break; + }); } - - return count; + return @intCast(self.tools.items.len - start); } fn sendRequest(self: *McpManager, server_idx: u32, method: []const u8, params: []const u8) bool { @@ -378,3 +404,53 @@ pub const ServerConfig = struct { name: []const u8, command: []const []const u8, }; + +test "MCP tool metadata owns strings after response release" { + const a = std.testing.allocator; + var manager = McpManager.init(a); + defer manager.deinit(); + const response = try a.dupe(u8, + \\{ "result" : { "tools" : [{"name":"first","description":"First tool","inputSchema":{"type":"object"}}, {"name":"second","inputSchema":{"type":"object"}}] } } + ); + try std.testing.expectEqual(@as(u32, 2), try manager.registerTools(response, 0, "test")); + a.free(response); + try std.testing.expectEqualStrings("test.first", manager.tools.items[0].name); + try std.testing.expectEqualStrings("First tool", manager.tools.items[0].description); + try std.testing.expectEqualStrings("{\"type\":\"object\"}", manager.tools.items[0].input_schema); + try std.testing.expectEqualStrings("test.second", manager.tools.items[1].name); +} + +test "invalid MCP entry rolls back tool registration" { + var manager = McpManager.init(std.testing.allocator); + defer manager.deinit(); + try std.testing.expectError(error.InvalidResponse, manager.registerTools( + \\{"result":{"tools":[{"name":"valid","inputSchema":{}},{"name":42,"inputSchema":{}}]}} + , 0, "test")); + try std.testing.expectEqual(@as(usize, 0), manager.tools.items.len); +} + +test "MCP handshake consumes initialize before listing tools" { + var manager = McpManager.init(std.testing.allocator); + defer manager.deinit(); + const server = + \\import json, sys + \\initialized = False + \\for line in sys.stdin: + \\ req = json.loads(line) + \\ method = req['method'] + \\ if method == 'initialize': + \\ result = {'protocolVersion':'2024-11-05','capabilities':{}} + \\ elif method == 'notifications/initialized': + \\ initialized = True + \\ continue + \\ elif method == 'tools/list': + \\ assert initialized + \\ result = {'tools':[{'name':'ping','description':'Owned description','inputSchema':{'type':'object'}}]} + \\ else: + \\ continue + \\ print(json.dumps({'jsonrpc':'2.0','id':req['id'],'result':result}), flush=True) + ; + try std.testing.expectEqual(@as(u32, 1), manager.connectServer("fixture", &.{ "python3", "-u", "-c", server })); + try std.testing.expectEqualStrings("fixture.ping", manager.tools.items[0].name); + try std.testing.expectEqualStrings("Owned description", manager.tools.items[0].description); +} diff --git a/src/tri-api/permissions.zig b/src/tri-api/permissions.zig index 2446950fc9..69872c2019 100644 --- a/src/tri-api/permissions.zig +++ b/src/tri-api/permissions.zig @@ -1,8 +1,7 @@ // permissions.zig — Tool permission system for tri-api -// deny > allow, project overrides user. Same model as Claude Code. +// User and project rules are combined; deny always takes priority. // Issue #65: Phase 6 permissions + checkpoints const std = @import("std"); -const proto = @import("tool_protocol.zig"); const user_settings = ".trinity/api/settings.json"; const project_settings = ".trinity/api/settings.json"; @@ -13,8 +12,11 @@ pub const Permission = enum { allow, deny }; pub const PermissionConfig = struct { allow_rules: std.ArrayList(Rule), deny_rules: std.ArrayList(Rule), + invalid: bool = false, pub fn deinit(self: *PermissionConfig, allocator: std.mem.Allocator) void { + for (self.allow_rules.items) |rule| rule.deinit(allocator); + for (self.deny_rules.items) |rule| rule.deinit(allocator); self.allow_rules.deinit(allocator); self.deny_rules.deinit(allocator); } @@ -23,6 +25,7 @@ pub const PermissionConfig = struct { /// Returns .deny if any deny rule matches, .allow if any allow rule matches, /// otherwise defaults: read_file/grep → allow, bash/write_file → deny. pub fn check(self: *const PermissionConfig, tool: []const u8, arg: []const u8) Permission { + if (self.invalid) return .deny; // 1. deny rules take priority for (self.deny_rules.items) |rule| { if (ruleMatches(rule, tool, arg)) return .deny; @@ -42,6 +45,14 @@ pub const PermissionConfig = struct { const Rule = struct { tool: []const u8, // "bash", "write_file", etc. pattern: []const u8, // glob-like: "*", "git diff *", ".env" + owned: bool = false, + + fn deinit(self: Rule, allocator: std.mem.Allocator) void { + if (self.owned) { + allocator.free(self.tool); + allocator.free(self.pattern); + } + } }; /// Check if a rule matches a tool+arg pair. @@ -62,7 +73,7 @@ fn ruleMatches(rule: Rule, tool: []const u8, arg: []const u8) bool { return std.mem.eql(u8, arg, pat); } -/// Load permission config. Project settings override user settings. +/// Load and combine user/project permission settings; deny always wins. /// Format: {"permissions":{"allow":["bash(git diff *)","read_file(*)"],"deny":["bash(rm -rf *)"]}} pub fn loadConfig(allocator: std.mem.Allocator) PermissionConfig { var config = PermissionConfig{ @@ -70,7 +81,7 @@ pub fn loadConfig(allocator: std.mem.Allocator) PermissionConfig { .deny_rules = std.ArrayList(Rule).empty, }; - // Try user settings first (~/.tri-api/settings.json) + // Try user settings first (~/.trinity/api/settings.json) const home = std.posix.getenv("HOME") orelse ""; if (home.len > 0) { var user_path_buf: [512]u8 = undefined; @@ -79,53 +90,47 @@ pub fn loadConfig(allocator: std.mem.Allocator) PermissionConfig { } else |_| {} } - // Project settings override (.tri-api/settings.json in cwd) + // Add project settings (.trinity/api/settings.json in cwd) loadFromFile(allocator, &config, project_settings); return config; } -/// Parse a settings file and add rules to config. +/// Missing settings use defaults; unreadable or invalid settings deny every tool. fn loadFromFile(allocator: std.mem.Allocator, config: *PermissionConfig, path: []const u8) void { - const content = readFile(allocator, path) catch return; + const content = readFile(allocator, path) catch |err| { + if (err != error.FileNotFound) config.invalid = true; + return; + }; defer allocator.free(content); + parseSettings(allocator, config, content) catch { + config.invalid = true; + }; +} + +fn parseSettings(allocator: std.mem.Allocator, config: *PermissionConfig, content: []const u8) !void { + const parsed = try std.json.parseFromSlice(std.json.Value, allocator, content, .{}); + defer parsed.deinit(); + if (parsed.value != .object) return error.InvalidSettings; + const perms = parsed.value.object.get("permissions") orelse return; + if (perms != .object) return error.InvalidSettings; + try parseRuleArray(allocator, perms.object.get("allow"), &config.allow_rules); + try parseRuleArray(allocator, perms.object.get("deny"), &config.deny_rules); +} - // Parse "allow":[...] array - parseRuleArray(allocator, content, "allow", &config.allow_rules); - // Parse "deny":[...] array - parseRuleArray(allocator, content, "deny", &config.deny_rules); -} - -/// Parse a JSON array of rule strings: "tool(pattern)" -fn parseRuleArray(allocator: std.mem.Allocator, data: []const u8, key: []const u8, rules: *std.ArrayList(Rule)) void { - // Find "key":[ in the JSON - var needle_buf: [64]u8 = undefined; - const needle = std.fmt.bufPrint(&needle_buf, "\"{s}\":[", .{key}) catch return; - - const idx = std.mem.indexOf(u8, data, needle) orelse return; - var pos = idx + needle.len; - - // Scan for quoted strings until ] - while (pos < data.len and data[pos] != ']') { - // Find next quoted string - if (std.mem.indexOfPos(u8, data, pos, "\"")) |q_start| { - if (q_start >= data.len) break; - const str_start = q_start + 1; - var str_end = str_start; - while (str_end < data.len and data[str_end] != '"') : (str_end += 1) {} - - if (str_end > str_start) { - const rule_str = data[str_start..str_end]; - if (parseRuleString(rule_str)) |rule| { - if (rules.items.len < max_rules) { - rules.append(allocator, rule) catch |err| { - std.log.warn("permissions: failed to append rule: {}", .{err}); - }; - } - } - } - pos = str_end + 1; - } else break; +fn parseRuleArray(allocator: std.mem.Allocator, value: ?std.json.Value, rules: *std.ArrayList(Rule)) !void { + const array = value orelse return; + if (array != .array) return error.InvalidSettings; + for (array.array.items) |item| { + if (item != .string) return error.InvalidRule; + const rule = parseRuleString(item.string) orelse return error.InvalidRule; + if (rules.items.len >= max_rules) return error.TooManyRules; + // Parsed JSON and file buffers die after loading; the config owns its strings. + const tool = try allocator.dupe(u8, rule.tool); + errdefer allocator.free(tool); + const pattern = try allocator.dupe(u8, rule.pattern); + errdefer allocator.free(pattern); + try rules.append(allocator, .{ .tool = tool, .pattern = pattern, .owned = true }); } } @@ -218,3 +223,58 @@ test "check defaults" { // write_file defaults to deny try std.testing.expectEqual(Permission.deny, config.check("write_file", "any")); } + +test "loaded rules survive JSON and file buffer release" { + const a = std.testing.allocator; + var config = PermissionConfig{ .allow_rules = .empty, .deny_rules = .empty }; + defer config.deinit(a); + var tmp = std.testing.tmpDir(.{}); + defer tmp.cleanup(); + try tmp.dir.writeFile(.{ .sub_path = "settings.json", .data = + \\{"permissions": {"allow": ["read_file(*)"], "deny": ["read_file(.env)"]}} + }); + const path = try tmp.dir.realpathAlloc(a, "settings.json"); + defer a.free(path); + loadFromFile(a, &config, path); + try std.testing.expect(!config.invalid); + try std.testing.expectEqual(Permission.deny, config.check("read_file", ".env")); + try std.testing.expectEqual(Permission.allow, config.check("read_file", "README.md")); +} + +test "invalid settings fail closed, including partially parsed allow" { + const a = std.testing.allocator; + const cases = [_][]const u8{ + "{", "[]", "{\"permissions\":null}", + "{\"permissions\":{\"allow\":[\"read_file(*)\"],\"deny\":[42]}}", "{\"permissions\":{\"deny\":[\"invalid\"]}}", "{\"permissions\":{\"deny\":\"read_file(*)\"}}", + "{\"permissions\":{\"deny\":[],\"deny\":[]}}", + }; + for (cases) |content| { + var config = PermissionConfig{ .allow_rules = .empty, .deny_rules = .empty }; + defer config.deinit(a); + var tmp = std.testing.tmpDir(.{}); + defer tmp.cleanup(); + try tmp.dir.writeFile(.{ .sub_path = "settings.json", .data = content }); + const path = try tmp.dir.realpathAlloc(a, "settings.json"); + defer a.free(path); + loadFromFile(a, &config, path); + try std.testing.expect(config.invalid); + try std.testing.expectEqual(Permission.deny, config.check("read_file", "README.md")); + try std.testing.expectEqual(Permission.deny, config.check("bash", "pwd")); + } +} + +test "rules decode escaped JSON and enforce capacity without truncation" { + const a = std.testing.allocator; + var config = PermissionConfig{ .allow_rules = .empty, .deny_rules = .empty }; + defer config.deinit(a); + try parseSettings(a, &config, + \\{ "permissions" : { "deny" : ["read_file(secret\u002efile)"] } } + ); + try std.testing.expectEqual(Permission.deny, config.check("read_file", "secret.file")); + for (0..max_rules - 1) |_| try parseSettings(a, &config, + \\{"permissions":{"deny":["read_file(*)"]}} + ); + try std.testing.expectError(error.TooManyRules, parseSettings(a, &config, + \\{"permissions":{"deny":["read_file(.env)"]}} + )); +} diff --git a/src/tri-api/tool_executor.zig b/src/tri-api/tool_executor.zig index 366bc9b1ed..26c8b6e3bf 100644 --- a/src/tri-api/tool_executor.zig +++ b/src/tri-api/tool_executor.zig @@ -210,7 +210,12 @@ pub const ToolExecutor = struct { const path = json.extractField(input_json, "path") orelse return .{ .output = "error: missing 'path' field", .is_error = true }; - // Create checkpoint before writing + if (!isPathSafe(path)) + return .{ .output = "error: path traversal blocked", .is_error = true }; + if (json.extractField(input_json, "content") == null) + return .{ .output = "error: missing 'content' field", .is_error = true }; + + // Reject invalid requests before checkpointing can mutate the working tree. self.checkpoint.createBeforeWrite(path); return self.writeFile(input_json); @@ -218,8 +223,11 @@ pub const ToolExecutor = struct { /// Reject paths containing traversal sequences fn isPathSafe(path: []const u8) bool { - // Block .. traversal - if (std.mem.indexOf(u8, path, "..") != null) return false; + if (path.len == 0 or std.fs.path.isAbsolute(path)) return false; + var components = std.mem.splitScalar(u8, path, '/'); + while (components.next()) |component| { + if (std.mem.eql(u8, component, "..")) return false; + } // Block null bytes if (std.mem.indexOfScalar(u8, path, 0) != null) return false; return true; @@ -272,14 +280,14 @@ pub const ToolExecutor = struct { /// Allowed command prefixes for bash tool const allowed_bash_cmds = [_][]const u8{ - "git ", "git\x00", "zig ", "zig\x00", "cat ", "ls ", "grep ", "find ", - "echo ", "mkdir ", "rm ", "tri ", "docker ", "gh ", "head ", "tail ", - "wc ", "pwd", "date", "env", "which ", "file ", "diff ", "sort ", - "test ", "cd ", "cp ", "mv ", "chmod ", "touch ", "sed ", "awk ", + "git ", "git\x00", "zig ", "zig\x00", "cat ", "ls ", "grep ", "find ", + "echo ", "mkdir ", "rm ", "tri ", "docker ", "gh ", "head ", "tail ", + "wc ", "which ", "file ", "diff ", "sort ", "test ", "cd ", "cp ", + "mv ", "chmod ", "touch ", "sed ", "awk ", }; /// Shell metacharacters that enable command chaining/injection - const shell_meta = [_]u8{ '|', ';', '`', '$', '(', ')', '{', '}' }; + const shell_meta = [_]u8{ '|', ';', '`', '$', '(', ')', '{', '}', '&', '<', '>', '\n', '\r', 0 }; fn isBashAllowed(command: []const u8) bool { const trimmed = std.mem.trimLeft(u8, command, &std.ascii.whitespace); @@ -295,7 +303,7 @@ pub const ToolExecutor = struct { if (std.mem.startsWith(u8, trimmed, prefix)) return true; } // Also allow bare commands without args - const bare_ok = [_][]const u8{ "pwd", "date", "env", "ls" }; + const bare_ok = [_][]const u8{ "pwd", "date", "ls" }; for (bare_ok) |cmd| { if (std.mem.eql(u8, trimmed, cmd)) return true; } @@ -387,6 +395,9 @@ test "isPathSafe blocks traversal" { try std.testing.expect(!ToolExecutor.isPathSafe("../../../etc/passwd")); try std.testing.expect(!ToolExecutor.isPathSafe("foo/../../../bar")); try std.testing.expect(!ToolExecutor.isPathSafe("foo\x00bar")); + try std.testing.expect(!ToolExecutor.isPathSafe("/etc/passwd")); + try std.testing.expect(!ToolExecutor.isPathSafe("")); + try std.testing.expect(ToolExecutor.isPathSafe("a..b.txt")); try std.testing.expect(ToolExecutor.isPathSafe("src/tri/main.zig")); try std.testing.expect(ToolExecutor.isPathSafe("specs/tri/mu_agent.tri")); } @@ -401,6 +412,12 @@ test "isBashAllowed blocks injection" { try std.testing.expect(!ToolExecutor.isBashAllowed("echo $(whoami)")); try std.testing.expect(!ToolExecutor.isBashAllowed("ls | xargs rm")); try std.testing.expect(!ToolExecutor.isBashAllowed("echo `id`")); + try std.testing.expect(!ToolExecutor.isBashAllowed("echo x & ls")); + try std.testing.expect(!ToolExecutor.isBashAllowed("echo x > file")); + try std.testing.expect(!ToolExecutor.isBashAllowed("cat < file")); + try std.testing.expect(!ToolExecutor.isBashAllowed("ls\nwhoami")); + try std.testing.expect(!ToolExecutor.isBashAllowed("pwdxxx")); + try std.testing.expect(!ToolExecutor.isBashAllowed("env sh -c whoami")); } test "redactSecrets blocks API keys" { @@ -424,3 +441,13 @@ test "redactSecrets passes clean strings" { const r1 = ToolExecutor.redactSecrets(&buf, clean); try std.testing.expectEqualSlices(u8, clean, r1); } + +test "invalid writes are rejected before checkpoint side effects" { + var executor = ToolExecutor{ .allocator = std.testing.allocator }; + const bad_path = executor.writeFileWithCheckpoint("{\"path\":\"../outside\",\"content\":\"x\"}"); + try std.testing.expect(bad_path.is_error); + try std.testing.expectEqualStrings("error: path traversal blocked", bad_path.output); + const no_content = executor.writeFileWithCheckpoint("{\"path\":\"valid.txt\"}"); + try std.testing.expect(no_content.is_error); + try std.testing.expectEqualStrings("error: missing 'content' field", no_content.output); +}