diff --git a/src/OpenClaw.Shared/Capabilities/CanvasCapability.cs b/src/OpenClaw.Shared/Capabilities/CanvasCapability.cs index c73f444..15bc170 100644 --- a/src/OpenClaw.Shared/Capabilities/CanvasCapability.cs +++ b/src/OpenClaw.Shared/Capabilities/CanvasCapability.cs @@ -168,6 +168,23 @@ private NodeInvokeResponse HandleA2UIPush(NodeInvokeRequest request) if (string.IsNullOrWhiteSpace(jsonl) && !string.IsNullOrWhiteSpace(jsonlPath)) { + // Validate jsonlPath to prevent arbitrary file reads. + // Resolve to absolute path and reject traversal or suspicious paths. + try + { + var fullPath = Path.GetFullPath(jsonlPath); + var tempRoot = Path.GetFullPath(Path.GetTempPath()); + if (!fullPath.StartsWith(tempRoot, StringComparison.OrdinalIgnoreCase)) + { + Logger.Warn($"canvas.a2ui.push: jsonlPath outside temp directory: {fullPath}"); + return Error("jsonlPath must be within the system temp directory"); + } + } + catch (Exception ex) + { + return Error($"Invalid jsonlPath: {ex.Message}"); + } + try { jsonl = File.ReadAllText(jsonlPath); diff --git a/src/OpenClaw.Shared/Capabilities/SystemCapability.cs b/src/OpenClaw.Shared/Capabilities/SystemCapability.cs index 62b7ed7..986f3fb 100644 --- a/src/OpenClaw.Shared/Capabilities/SystemCapability.cs +++ b/src/OpenClaw.Shared/Capabilities/SystemCapability.cs @@ -293,15 +293,23 @@ private async Task HandleRunAsync(NodeInvokeRequest request) } } - Logger.Info($"system.run: {command} (shell={shell ?? "auto"}, timeout={timeoutMs}ms)"); + // Build the full command string for policy evaluation and logging. + // When command arrives as an argv array, we must evaluate the entire + // command line — not just argv[0] — so policy rules like "rm *" correctly + // match "rm -rf /". + var fullCommand = args != null + ? FormatExecCommand(new[] { command }.Concat(args).ToArray()) + : command; + + Logger.Info($"system.run: {fullCommand} (shell={shell ?? "auto"}, timeout={timeoutMs}ms)"); // Check exec approval policy if (_approvalPolicy != null) { - var approval = _approvalPolicy.Evaluate(command, shell); + var approval = _approvalPolicy.Evaluate(fullCommand, shell); if (!approval.Allowed) { - Logger.Warn($"system.run DENIED: {command} ({approval.Reason})"); + Logger.Warn($"system.run DENIED: {fullCommand} ({approval.Reason})"); return Error($"Command denied by exec policy: {approval.Reason}"); } } diff --git a/src/OpenClaw.Shared/Models.cs b/src/OpenClaw.Shared/Models.cs index 92d6ecc..725a66b 100644 --- a/src/OpenClaw.Shared/Models.cs +++ b/src/OpenClaw.Shared/Models.cs @@ -126,7 +126,7 @@ public string DisplayText { var label = Status.ToLowerInvariant() switch { - "ok" or "connected" or "running" => "[ON]", + "ok" or "connected" or "running" or "active" => "[ON]", "linked" => "[LINKED]", "ready" => "[READY]", "connecting" or "reconnecting" => "[...]", diff --git a/src/OpenClaw.Shared/OpenClawGatewayClient.cs b/src/OpenClaw.Shared/OpenClawGatewayClient.cs index 04ed520..411ae5e 100644 --- a/src/OpenClaw.Shared/OpenClawGatewayClient.cs +++ b/src/OpenClaw.Shared/OpenClawGatewayClient.cs @@ -30,6 +30,8 @@ public class OpenClawGatewayClient : IDisposable private GatewayCostUsageInfo? _usageCost; private readonly Dictionary _pendingRequestMethods = new(); private readonly object _pendingRequestLock = new(); + private readonly object _sessionsLock = new(); + private readonly object _nodesLock = new(); private bool _usageStatusUnsupported; private bool _usageCostUnsupported; private bool _sessionPreviewUnsupported; @@ -373,12 +375,24 @@ private async Task SendConnectMessageAsync(string? nonce = null) private async Task SendRawAsync(string message) { - if (_webSocket?.State == WebSocketState.Open) + // Capture local reference to avoid TOCTOU race with reconnect/dispose + var ws = _webSocket; + if (ws?.State != WebSocketState.Open) return; + + try { var bytes = Encoding.UTF8.GetBytes(message); - await _webSocket.SendAsync(new ArraySegment(bytes), + await ws.SendAsync(new ArraySegment(bytes), WebSocketMessageType.Text, true, _cts.Token); } + catch (ObjectDisposedException) + { + // WebSocket was disposed between state check and send + } + catch (WebSocketException ex) when (ex.WebSocketErrorCode == WebSocketError.InvalidState) + { + _logger.Warn($"WebSocket send failed (state changed): {ex.Message}"); + } } private async Task SendTrackedRequestAsync(string method, object? parameters = null) @@ -990,23 +1004,37 @@ private void HandleSessionEvent(JsonElement root) private void UpdateTrackedSession(string sessionKey, bool isMain, string? currentActivity) { - if (!_sessions.ContainsKey(sessionKey)) + SessionInfo[] snapshot; + lock (_sessionsLock) { - _sessions[sessionKey] = new SessionInfo + if (!_sessions.ContainsKey(sessionKey)) { - Key = sessionKey, - IsMain = isMain, - Status = "active" - }; - } + _sessions[sessionKey] = new SessionInfo + { + Key = sessionKey, + IsMain = isMain, + Status = "active" + }; + } + + _sessions[sessionKey].CurrentActivity = currentActivity; + _sessions[sessionKey].LastSeen = DateTime.UtcNow; - _sessions[sessionKey].CurrentActivity = currentActivity; - _sessions[sessionKey].LastSeen = DateTime.UtcNow; + snapshot = GetSessionListInternal(); + } - SessionsUpdated?.Invoke(this, GetSessionList()); + SessionsUpdated?.Invoke(this, snapshot); } public SessionInfo[] GetSessionList() + { + lock (_sessionsLock) + { + return GetSessionListInternal(); + } + } + + private SessionInfo[] GetSessionListInternal() { var list = new List(_sessions.Values); list.Sort((a, b) => @@ -1096,69 +1124,75 @@ private void ParseSessions(JsonElement sessions) { try { - _sessions.Clear(); - - // Handle both Array format and Object (dictionary) format - if (sessions.ValueKind == JsonValueKind.Array) + SessionInfo[] snapshot; + lock (_sessionsLock) { - foreach (var item in sessions.EnumerateArray()) + _sessions.Clear(); + + // Handle both Array format and Object (dictionary) format + if (sessions.ValueKind == JsonValueKind.Array) { - ParseSessionItem(item); + foreach (var item in sessions.EnumerateArray()) + { + ParseSessionItem(item); + } } - } - else if (sessions.ValueKind == JsonValueKind.Object) - { - // Object format: keys are session IDs, values could be session info objects or simple strings - foreach (var prop in sessions.EnumerateObject()) + else if (sessions.ValueKind == JsonValueKind.Object) { - var sessionKey = prop.Name; + // Object format: keys are session IDs, values could be session info objects or simple strings + foreach (var prop in sessions.EnumerateObject()) + { + var sessionKey = prop.Name; - // Skip metadata fields that aren't actual sessions - if (sessionKey is "recent" or "count" or "path" or "defaults" or "ts") - continue; + // Skip metadata fields that aren't actual sessions + if (sessionKey is "recent" or "count" or "path" or "defaults" or "ts") + continue; - // Skip non-session keys (must look like a session key pattern) - if (!sessionKey.Equals("global", StringComparison.OrdinalIgnoreCase) && - !sessionKey.Contains(':') && - !sessionKey.Contains("agent") && - !sessionKey.Contains("session")) - continue; + // Skip non-session keys (must look like a session key pattern) + if (!sessionKey.Equals("global", StringComparison.OrdinalIgnoreCase) && + !sessionKey.Contains(':') && + !sessionKey.Contains("agent") && + !sessionKey.Contains("session")) + continue; - var session = new SessionInfo { Key = sessionKey }; - var item = prop.Value; + var session = new SessionInfo { Key = sessionKey }; + var item = prop.Value; - // Detect main session from key pattern - "agent:main:main" ends with ":main" - var endsWithMain = sessionKey.EndsWith(":main"); - session.IsMain = sessionKey == "main" || endsWithMain || sessionKey.Contains(":main:main"); - _logger.Debug($"Session key={sessionKey}, endsWithMain={endsWithMain}, IsMain={session.IsMain}"); + // Detect main session from key pattern - "agent:main:main" ends with ":main" + var endsWithMain = sessionKey.EndsWith(":main"); + session.IsMain = sessionKey == "main" || endsWithMain || sessionKey.Contains(":main:main"); + _logger.Debug($"Session key={sessionKey}, endsWithMain={endsWithMain}, IsMain={session.IsMain}"); - // Value might be an object with session details or just a string status - if (item.ValueKind == JsonValueKind.Object) - { - // Only override IsMain if the JSON explicitly says true - if (item.TryGetProperty("isMain", out var isMain) && isMain.GetBoolean()) - session.IsMain = true; - PopulateSessionFromObject(session, item); - } - else if (item.ValueKind == JsonValueKind.String) - { - // Simple string value - skip if it looks like a path (metadata) - var strVal = item.GetString() ?? ""; - if (strVal.StartsWith("/") || strVal.Contains("/.")) + // Value might be an object with session details or just a string status + if (item.ValueKind == JsonValueKind.Object) + { + // Only override IsMain if the JSON explicitly says true + if (item.TryGetProperty("isMain", out var isMain) && isMain.GetBoolean()) + session.IsMain = true; + PopulateSessionFromObject(session, item); + } + else if (item.ValueKind == JsonValueKind.String) + { + // Simple string value - skip if it looks like a path (metadata) + var strVal = item.GetString() ?? ""; + if (strVal.StartsWith("/") || strVal.Contains("/.")) + continue; + session.Status = strVal; + } + else if (item.ValueKind == JsonValueKind.Number) + { + // Skip numeric values (like count) continue; - session.Status = strVal; - } - else if (item.ValueKind == JsonValueKind.Number) - { - // Skip numeric values (like count) - continue; - } + } - _sessions[session.Key] = session; + _sessions[session.Key] = session; + } } + + snapshot = GetSessionListInternal(); } - SessionsUpdated?.Invoke(this, GetSessionList()); + SessionsUpdated?.Invoke(this, snapshot); } catch (Exception ex) { @@ -1308,9 +1342,12 @@ private void ParseNodeList(JsonElement nodesPayload) .ThenBy(n => n.DisplayName, StringComparer.OrdinalIgnoreCase) .ToArray(); - _nodes.Clear(); - foreach (var node in ordered) - _nodes[node.NodeId] = node; + lock (_nodesLock) + { + _nodes.Clear(); + foreach (var node in ordered) + _nodes[node.NodeId] = node; + } NodesUpdated?.Invoke(this, ordered); } @@ -1704,13 +1741,18 @@ private static string TruncateLabel(string text, int maxLen = 60) public void Dispose() { - if (!_disposed) - { - _disposed = true; - _cts.Cancel(); - ClearPendingRequests(); - _webSocket?.Dispose(); - _cts.Dispose(); - } + if (_disposed) return; + _disposed = true; + + try { _cts.Cancel(); } catch { } + + ClearPendingRequests(); + + var ws = _webSocket; + _webSocket = null; + try { ws?.Dispose(); } catch { } + + // Don't dispose _cts immediately — listen loop or reconnect may still reference it. + // It will be GC'd after all pending tasks complete. } } diff --git a/src/OpenClaw.Tray.WinUI/App.xaml.cs b/src/OpenClaw.Tray.WinUI/App.xaml.cs index df4f768..de0780f 100644 --- a/src/OpenClaw.Tray.WinUI/App.xaml.cs +++ b/src/OpenClaw.Tray.WinUI/App.xaml.cs @@ -1603,15 +1603,21 @@ private void ShowSettings() private void OnSettingsSaved(object? sender, EventArgs e) { - // Reconnect with new settings + // Reconnect with new settings — mirror the startup if/else pattern + // to avoid dual connections that cause gateway conflicts. _gatewayClient?.Dispose(); - InitializeGatewayClient(); - - // Reinitialize node service (safe dispose pattern) var oldNodeService = _nodeService; _nodeService = null; try { oldNodeService?.Dispose(); } catch (Exception ex) { Logger.Warn($"Node dispose error: {ex.Message}"); } - InitializeNodeService(); + + if (_settings?.EnableNodeMode == true) + { + InitializeNodeService(); + } + else + { + InitializeGatewayClient(); + } // Update global hotkey if (_settings!.GlobalHotkeyEnabled) diff --git a/src/OpenClaw.Tray.WinUI/Services/NodeService.cs b/src/OpenClaw.Tray.WinUI/Services/NodeService.cs index b7b5408..1bd3883 100644 --- a/src/OpenClaw.Tray.WinUI/Services/NodeService.cs +++ b/src/OpenClaw.Tray.WinUI/Services/NodeService.cs @@ -250,7 +250,7 @@ private async Task OnCanvasEval(string script) { var tcs = new TaskCompletionSource(); - _dispatcherQueue.TryEnqueue(async () => + bool enqueued = _dispatcherQueue.TryEnqueue(async () => { try { @@ -269,6 +269,8 @@ private async Task OnCanvasEval(string script) tcs.SetException(ex); } }); + if (!enqueued) + tcs.TrySetException(new InvalidOperationException("Dispatcher queue unavailable")); return await tcs.Task; } @@ -277,7 +279,7 @@ private async Task OnCanvasSnapshot(CanvasSnapshotArgs args) { var tcs = new TaskCompletionSource(); - _dispatcherQueue.TryEnqueue(async () => + bool enqueued = _dispatcherQueue.TryEnqueue(async () => { try { @@ -296,6 +298,8 @@ private async Task OnCanvasSnapshot(CanvasSnapshotArgs args) tcs.SetException(ex); } }); + if (!enqueued) + tcs.TrySetException(new InvalidOperationException("Dispatcher queue unavailable")); return await tcs.Task; } diff --git a/tests/OpenClaw.Shared.Tests/CapabilityTests.cs b/tests/OpenClaw.Shared.Tests/CapabilityTests.cs index cd16167..350cb8d 100644 --- a/tests/OpenClaw.Shared.Tests/CapabilityTests.cs +++ b/tests/OpenClaw.Shared.Tests/CapabilityTests.cs @@ -623,16 +623,50 @@ public async Task A2UIPush_WithJsonlPath_ReadsFile() public async Task A2UIPush_WithMissingJsonlPath_ReturnsError() { var cap = new CanvasCapability(NullLogger.Instance); + // Use a path within the temp directory so path validation passes + var missingFile = Path.Combine(Path.GetTempPath(), $"nonexistent-{Guid.NewGuid():N}.jsonl"); var req = new NodeInvokeRequest { Id = "c17", Command = "canvas.a2ui.push", - Args = Parse("""{"jsonlPath":"/nonexistent/path/file.jsonl"}""") + Args = Parse($"{{\"jsonlPath\":\"{missingFile.Replace("\\", "\\\\")}\"}}") }; var res = await cap.ExecuteAsync(req); Assert.False(res.Ok); Assert.Contains("Failed to read jsonlPath", res.Error); } + + [Fact] + public async Task A2UIPush_WithJsonlPathOutsideTempDir_ReturnsError() + { + var cap = new CanvasCapability(NullLogger.Instance); + var req = new NodeInvokeRequest + { + Id = "c18", + Command = "canvas.a2ui.push", + Args = Parse("""{"jsonlPath":"C:\\Windows\\System32\\config\\SAM"}""") + }; + var res = await cap.ExecuteAsync(req); + Assert.False(res.Ok); + Assert.Contains("temp directory", res.Error); + } + + [Fact] + public async Task A2UIPush_WithJsonlPathTraversal_ReturnsError() + { + var cap = new CanvasCapability(NullLogger.Instance); + // Path traversal attempt to escape temp directory + var traversalPath = Path.Combine(Path.GetTempPath(), "..", "..", "Windows", "System32", "config", "SAM"); + var req = new NodeInvokeRequest + { + Id = "c19", + Command = "canvas.a2ui.push", + Args = Parse($"{{\"jsonlPath\":\"{traversalPath.Replace("\\", "\\\\")}\"}}") + }; + var res = await cap.ExecuteAsync(req); + Assert.False(res.Ok); + Assert.Contains("temp directory", res.Error); + } } public class ScreenCapabilityTests diff --git a/tests/OpenClaw.Shared.Tests/ExecApprovalPolicyTests.cs b/tests/OpenClaw.Shared.Tests/ExecApprovalPolicyTests.cs index e8545ee..25ae50e 100644 --- a/tests/OpenClaw.Shared.Tests/ExecApprovalPolicyTests.cs +++ b/tests/OpenClaw.Shared.Tests/ExecApprovalPolicyTests.cs @@ -467,6 +467,113 @@ public async Task SystemRun_WithPolicy_AllowsApprovedCommands() } } + [Fact] + public async Task SystemRun_ArrayCommand_WithPolicy_DeniesBlockedCommands() + { + // Regression test: when command is an argv array like ["rm", "-rf", "/"], + // policy must evaluate the full "rm -rf /" string, not just "rm". + var tempDir = Path.Combine(Path.GetTempPath(), $"test-{Guid.NewGuid():N}"); + Directory.CreateDirectory(tempDir); + try + { + var policy = new ExecApprovalPolicy(tempDir, _logger); + policy.SetRules(new[] + { + new ExecApprovalRule { Pattern = "echo *", Action = ExecApprovalAction.Allow }, + }, ExecApprovalAction.Deny); + + var cap = CreateCapability(policy); + + // Array-style command: ["rm", "-rf", "/"] — must be denied + var request = new NodeInvokeRequest + { + Command = "system.run", + Args = JsonDocument.Parse("{\"command\":[\"rm\",\"-rf\",\"/\"]}").RootElement + }; + + var result = await cap.ExecuteAsync(request); + Assert.False(result.Ok); + Assert.Contains("denied", result.Error!, StringComparison.OrdinalIgnoreCase); + } + finally + { + try { Directory.Delete(tempDir, true); } catch { } + } + } + + [Fact] + public async Task SystemRun_ArrayCommand_WithPolicy_AllowsApprovedCommands() + { + // Array-style command ["echo", "hello"] should match "echo *" and be allowed + var tempDir = Path.Combine(Path.GetTempPath(), $"test-{Guid.NewGuid():N}"); + Directory.CreateDirectory(tempDir); + try + { + var policy = new ExecApprovalPolicy(tempDir, _logger); + policy.SetRules(new[] + { + new ExecApprovalRule { Pattern = "echo *", Action = ExecApprovalAction.Allow }, + }, ExecApprovalAction.Deny); + + var cap = CreateCapability(policy); + + var request = new NodeInvokeRequest + { + Command = "system.run", + Args = JsonDocument.Parse("{\"command\":[\"echo\",\"hello\"]}").RootElement + }; + + var result = await cap.ExecuteAsync(request); + Assert.True(result.Ok); + } + finally + { + try { Directory.Delete(tempDir, true); } catch { } + } + } + + [Fact] + public async Task SystemRun_ArrayCommand_PolicyEvaluatesFullCommandLine() + { + // A rule blocking "rm -rf *" should catch ["rm", "-rf", "/"] but allow ["rm", "safe.txt"] + var tempDir = Path.Combine(Path.GetTempPath(), $"test-{Guid.NewGuid():N}"); + Directory.CreateDirectory(tempDir); + try + { + var policy = new ExecApprovalPolicy(tempDir, _logger); + policy.SetRules(new[] + { + new ExecApprovalRule { Pattern = "rm -rf *", Action = ExecApprovalAction.Deny }, + new ExecApprovalRule { Pattern = "rm *", Action = ExecApprovalAction.Allow }, + }, ExecApprovalAction.Deny); + + var cap = CreateCapability(policy); + + // ["rm", "-rf", "/"] should be denied by "rm -rf *" + var dangerousRequest = new NodeInvokeRequest + { + Command = "system.run", + Args = JsonDocument.Parse("{\"command\":[\"rm\",\"-rf\",\"/\"]}").RootElement + }; + var result1 = await cap.ExecuteAsync(dangerousRequest); + Assert.False(result1.Ok); + Assert.Contains("denied", result1.Error!, StringComparison.OrdinalIgnoreCase); + + // ["rm", "safe.txt"] should be allowed by "rm *" + var safeRequest = new NodeInvokeRequest + { + Command = "system.run", + Args = JsonDocument.Parse("{\"command\":[\"rm\",\"safe.txt\"]}").RootElement + }; + var result2 = await cap.ExecuteAsync(safeRequest); + Assert.True(result2.Ok); + } + finally + { + try { Directory.Delete(tempDir, true); } catch { } + } + } + [Fact] public async Task SystemRun_WithoutPolicy_AllowsAll() {