fix: prevent secret placeholder writeback on file read/write tools (#1333) (#1343)

* fix: suppress output redaction on file tools to prevent secret placeholder writeback (#1333)

SecretOutputRedactor was replacing secret values with ***REDACTED***
placeholders in file_read output before it reached the model. When the
model subsequently wrote that content back via file_write/file_edit, the
placeholders were persisted to disk, destroying real secret values in
config files.

The fix moves redaction out of the model-facing path for file tools while
keeping it on the observability path (spill files, logs, transcripts):

- Add SuppressOutputRedaction flag to INetclawTool; FileReadTool opts in
- DispatchingToolExecutor splits model-facing (raw) vs spill (redacted)
  result when the flag is set
- ToolOutputSpill gains a two-param overload so spill files always use
  redacted content even when the inline result is unredacted
- Consolidate file_write logic into FileEditTool as a Content parameter
  mode; FileWriteTool becomes a thin backward-compatible delegate

* fix: address code review findings from #1333 redaction PR

- Redact tool results in SessionLogActor before writing to session.log
  so file_read's SuppressOutputRedaction doesn't leak secrets to the
  observability log on disk
- Tighten FileEditTool Content mutual-exclusivity guard to also reject
  NewString and ReplaceAll (not just OldString) when Content is supplied
- Always redact streaming ToolActivityUpdate chunks regardless of
  SuppressOutputRedaction — activity chunks are progressive display, not
  content the model writes back
- Fix stale doc comment in ToolOutputSpill that claimed inputs are
  always pre-redacted
- Normalize error message casing in FileWriteTool ('path' → 'Path')

Author: Aaronontheweb

src/Netclaw.Actors.Tests/Tools/DispatchingToolExecutorTests.cs

@@ -143,6 +143,97 @@ public async Task Small_output_is_redacted_without_spilling()
         Assert.DoesNotContain("saved to", result); // small → no spill
     }
 
+    [Fact]
+    public async Task File_read_preserves_secret_values_for_model()
+    {
+        var sessionDir = Path.Combine(Path.GetTempPath(), "nc-disp-" + Guid.NewGuid().ToString("N"));
+        Directory.CreateDirectory(sessionDir);
+        try
+        {
+            // file_read suppresses output redaction so the model sees real
+            // content and can write it back without corrupting secrets (#1333).
+            var file = Path.Combine(sessionDir, "appsettings.json");
+            await File.WriteAllTextAsync(file,
+                """{"secretKey": "real-secret-value", "name": "myapp"}""",
+                CancellationToken.None);
+            var toolCall = new FunctionCallContent("call-secret", "file_read",
+                ToolInput.Create("Path", file));
+            var context = new Netclaw.Tools.ToolExecutionContext("slack/thread-1", sessionDir)
+            {
+                Audience = TrustAudience.Personal,
+            };
+
+            var result = await _executor.ExecuteAsync(toolCall, context, CancellationToken.None);
+
+            Assert.Contains("real-secret-value", result);
+            Assert.DoesNotContain("***REDACTED***", result);
+        }
+        finally
+        {
+            Directory.Delete(sessionDir, recursive: true);
+        }
+    }
+
+    [Fact]
+    public async Task Shell_output_still_redacts_secrets()
+    {
+        // Shell output continues to be redacted — only file tools suppress it.
+        var toolCall = new FunctionCallContent("call-shell-secret", "shell_execute",
+            ToolInput.Create("Command", "echo API_KEY=secret123"));
+        var context = new Netclaw.Tools.ToolExecutionContext("signalr/thread-1", null)
+        {
+            Audience = TrustAudience.Personal,
+        };
+
+        var result = await _executor.ExecuteAsync(toolCall, context, CancellationToken.None);
+
+        Assert.Contains("***REDACTED***", result);
+        Assert.DoesNotContain("secret123", result);
+    }
+
+    [Fact]
+    public async Task File_read_spill_file_is_redacted_even_when_model_result_is_not()
+    {
+        var sessionDir = Path.Combine(Path.GetTempPath(), "nc-disp-" + Guid.NewGuid().ToString("N"));
+        Directory.CreateDirectory(sessionDir);
+        try
+        {
+            // Create a file with a secret that exceeds the shell's verbose
+            // budget so it spills. file_read uses the content budget (12000)
+            // so we need a bigger file. Use a custom executor with a small
+            // content budget to force a spill.
+            var file = Path.Combine(sessionDir, "big-config.json");
+            var bigContent = $$"""{"secretKey": "real-secret-value", "data": "{{new string('x', 15000)}}"}""";
+            await File.WriteAllTextAsync(file, bigContent, CancellationToken.None);
+
+            var toolCall = new FunctionCallContent("call-spill-secret", "file_read",
+                ToolInput.Create("Path", file));
+            var context = new Netclaw.Tools.ToolExecutionContext("slack/thread-1", sessionDir)
+            {
+                Audience = TrustAudience.Personal,
+                MaxInlineToolResultChars = 500,
+            };
+
+            var result = await _executor.ExecuteAsync(toolCall, context, CancellationToken.None);
+
+            // The inline result (model-facing) should NOT contain the redacted sentinel
+            Assert.DoesNotContain("***REDACTED***", result);
+            // But it should be truncated (spilled)
+            Assert.Contains("output saved to", result);
+
+            // The spill file on disk SHOULD be redacted
+            var spillPath = Path.Combine(sessionDir, "tool-calls", "call-spill-secret.log");
+            Assert.True(File.Exists(spillPath));
+            var spillContent = await File.ReadAllTextAsync(spillPath, CancellationToken.None);
+            Assert.Contains("***REDACTED***", spillContent);
+            Assert.DoesNotContain("real-secret-value", spillContent);
+        }
+        finally
+        {
+            Directory.Delete(sessionDir, recursive: true);
+        }
+    }
+
     [Fact]
     public async Task Content_tool_under_default_budget_not_spilled()
     {

src/Netclaw.Actors.Tests/Tools/FileEditToolTests.cs

@@ -180,6 +180,78 @@ public async Task Concurrent_edits_to_same_file_all_apply()
             Assert.Contains($"slot{i} = EDITED;", content);
     }
 
+    // ── Full-write mode via Content parameter ──
+
+    [Fact]
+    public async Task Content_creates_new_file()
+    {
+        var filePath = Path.Combine(_dir.Path, "new-via-content.txt");
+
+        var result = await _tool.ExecuteAsync(ToolInput.Create("Path", filePath, "Content", "full write"), CreatePersonalContext(), CancellationToken.None);
+
+        Assert.Contains("Successfully wrote", result);
+        Assert.Equal("full write", await File.ReadAllTextAsync(filePath, TestContext.Current.CancellationToken));
+    }
+
+    [Fact]
+    public async Task Content_overwrites_existing_file()
+    {
+        var filePath = Path.Combine(_dir.Path, "existing.txt");
+        await File.WriteAllTextAsync(filePath, "old", TestContext.Current.CancellationToken);
+
+        var result = await _tool.ExecuteAsync(ToolInput.Create("Path", filePath, "Content", "new"), CreatePersonalContext(), CancellationToken.None);
+
+        Assert.Contains("Successfully wrote", result);
+        Assert.Equal("new", await File.ReadAllTextAsync(filePath, TestContext.Current.CancellationToken));
+    }
+
+    [Fact]
+    public async Task Content_creates_parent_directories()
+    {
+        var filePath = Path.Combine(_dir.Path, "sub", "dir", "deep.txt");
+
+        var result = await _tool.ExecuteAsync(ToolInput.Create("Path", filePath, "Content", "deep"), CreatePersonalContext(), CancellationToken.None);
+
+        Assert.Contains("Successfully wrote", result);
+        Assert.Equal("deep", await File.ReadAllTextAsync(filePath, TestContext.Current.CancellationToken));
+    }
+
+    [Fact]
+    public async Task Content_and_OldString_are_mutually_exclusive()
+    {
+        var filePath = Path.Combine(_dir.Path, "conflict.txt");
+
+        var result = await _tool.ExecuteAsync(ToolInput.Create(
+            "Path", filePath, "Content", "full", "OldString", "old", "NewString", "new"), CreatePersonalContext(), CancellationToken.None);
+
+        Assert.Contains("mutually exclusive", result);
+        Assert.False(File.Exists(filePath));
+    }
+
+    [Fact]
+    public async Task Content_and_NewString_are_mutually_exclusive()
+    {
+        var filePath = Path.Combine(_dir.Path, "conflict2.txt");
+
+        var result = await _tool.ExecuteAsync(ToolInput.Create(
+            "Path", filePath, "Content", "full", "NewString", "new"), CreatePersonalContext(), CancellationToken.None);
+
+        Assert.Contains("mutually exclusive", result);
+        Assert.False(File.Exists(filePath));
+    }
+
+    [Fact]
+    public async Task Content_and_ReplaceAll_are_mutually_exclusive()
+    {
+        var filePath = Path.Combine(_dir.Path, "conflict3.txt");
+
+        var result = await _tool.ExecuteAsync(ToolInput.Create(
+            "Path", filePath, "Content", "full", "ReplaceAll", true), CreatePersonalContext(), CancellationToken.None);
+
+        Assert.Contains("mutually exclusive", result);
+        Assert.False(File.Exists(filePath));
+    }
+
     private ToolExecutionContext CreatePersonalContext()
         => new("signalr/thread-1", _sessionDir)
         {

src/Netclaw.Actors/Sessions/SessionLogActor.cs

@@ -7,6 +7,7 @@
 using Akka.Event;
 using Netclaw.Actors.Protocol;
 using Netclaw.Actors.SubAgents;
+using Netclaw.Security;
 
 namespace Netclaw.Actors.Sessions;
 
@@ -107,7 +108,7 @@ private void OnOutput(SessionOutput output)
             {
                 TextOutput text => $"Assistant: {TextTruncation.EllipsisAppend(text.Text, 1000)}",
                 ToolCallOutput toolCall => FormatToolCall(toolCall),
-                ToolResultOutput toolResult => $"Tool result: {toolResult.ToolName} (call={toolResult.CallId}) → {TextTruncation.EllipsisAppend(toolResult.Result, 1000)}",
+                ToolResultOutput toolResult => $"Tool result: {toolResult.ToolName} (call={toolResult.CallId}) → {TextTruncation.EllipsisAppend(SecretOutputRedactor.Redact(toolResult.Result), 1000)}",
                 ThinkingOutput thinking => $"Thinking: {TextTruncation.EllipsisAppend(thinking.Text, 1000)}",
                 ThinkingDeltaOutput thinkingDelta => $"Thinking delta: {TextTruncation.EllipsisAppend(thinkingDelta.Delta, 1000)}",
                 UsageOutput usage => $"Usage: in={usage.InputTokens} out={usage.OutputTokens} cached={usage.CachedInputTokens} reasoning={usage.ReasoningTokens} context={usage.UsagePercent:P0}",

src/Netclaw.Actors/Tools/DispatchingToolExecutor.cs

@@ -67,14 +67,21 @@ public async Task<string> ExecuteAsync(FunctionCallContent toolCall, ToolExecuti
                 ? await tool.ExecuteAsync(toolCall.Arguments, context, ct)
                 : await tool.ExecuteAsync(toolCall.Arguments, ct);
 
-            result = SecretOutputRedactor.Redact(result);
+            var redacted = SecretOutputRedactor.Redact(result);
+
+            // Tools that suppress output redaction (e.g. file_read) return the
+            // raw result to the model so read-modify-write cycles don't corrupt
+            // secret values with ***REDACTED*** placeholders. The spill file
+            // (persisted to disk) always uses the redacted version.
+            var modelFacing = tool.SuppressOutputRedaction ? result : redacted;
+
             // Single, uniform bounding+spill point for every tool (main session and
             // sub-agents both funnel through here): cap the inline result to the
             // tool's budget and, when it overflows, spill the full redacted result
             // to a session file and steer the model to read a slice. Tools only
             // bound their own capture for memory safety; they do not window or spill.
             result = await ToolOutputSpill.BoundAndSpillAsync(
-                result, toolCall.CallId, ResolveInlineBudget(tool, context), context, ct);
+                modelFacing, redacted, toolCall.CallId, ResolveInlineBudget(tool, context), context, ct);
 
             sw.Stop();
             _logger.LogInformation(
@@ -133,12 +140,13 @@ public async IAsyncEnumerable<ToolCallUpdate> ExecuteStreamAsync(
                 case ToolCompletedUpdate completed:
                     sw.Stop();
                     var redacted = SecretOutputRedactor.Redact(completed.Result);
-                    redacted = await ToolOutputSpill.BoundAndSpillAsync(
-                        redacted, toolCall.CallId, ResolveInlineBudget(tool, context), context, ct);
+                    var modelResult = tool.SuppressOutputRedaction ? completed.Result : redacted;
+                    modelResult = await ToolOutputSpill.BoundAndSpillAsync(
+                        modelResult, redacted, toolCall.CallId, ResolveInlineBudget(tool, context), context, ct);
                     _logger.LogInformation(
                         "Tool executed: {ToolName} ({Duration}ms, {ResultLength} chars)",
-                        toolCall.Name, sw.ElapsedMilliseconds, redacted.Length);
-                    yield return new ToolCompletedUpdate(redacted);
+                        toolCall.Name, sw.ElapsedMilliseconds, modelResult.Length);
+                    yield return new ToolCompletedUpdate(modelResult);
                     break;
                 case ToolActivityUpdate { OutputChunk: not null } activity:
                     yield return activity with { OutputChunk = SecretOutputRedactor.Redact(activity.OutputChunk) };

src/Netclaw.Actors/Tools/FileEditTool.cs

@@ -12,11 +12,14 @@
 namespace Netclaw.Actors.Tools;
 
 /// <summary>
-/// Makes targeted text replacements in an existing file without rewriting the entire file.
-/// Matches literal text (not regex). Fails if OldString is not found or is ambiguous.
+/// Edits files: targeted text replacement (OldString → NewString) or full content
+/// write (Content alone). When Content is supplied without OldString, the file is
+/// created or overwritten — this subsumes the former <c>file_write</c> tool.
 /// </summary>
 [NetclawTool(ToolName,
-    "Make targeted text replacements in an existing file without rewriting the entire file",
+    "Edit a file with targeted text replacement (OldString/NewString), or write entire content (Content). " +
+    "For targeted edits, matches literal text (not regex) and fails if OldString is not found or is ambiguous. " +
+    "For full writes, creates the file and parent directories if needed.",
     Grant = "file")]
 public sealed partial class FileEditTool : NetclawTool<FileEditTool.Params>
 {
@@ -26,10 +29,11 @@ public sealed partial class FileEditTool : NetclawTool<FileEditTool.Params>
     private readonly ScopedFileAccessPolicy _fileAccessPolicy;
 
     public record Params(
-        [property: Description("Absolute path to the file to edit")] string Path,
-        [property: Description("The exact text to find in the file")] string OldString,
-        [property: Description("The text to replace it with (must differ from OldString; use empty string to delete)")] string NewString,
-        [property: Description("Replace all occurrences instead of just the first (default: false)")] bool? ReplaceAll = null);
+        [property: Description("Absolute path to the file")] string Path,
+        [property: Description("The exact text to find in the file (omit when using Content for a full write)")] string? OldString = null,
+        [property: Description("The text to replace OldString with (must differ from OldString; use empty string to delete)")] string? NewString = null,
+        [property: Description("Replace all occurrences instead of just the first (default: false)")] bool? ReplaceAll = null,
+        [property: Description("Full content to write to the file, creating parent directories if needed. Mutually exclusive with OldString/NewString.")] string? Content = null);
 
     public FileEditTool(ToolPathPolicy? pathPolicy = null)
     {
@@ -51,13 +55,64 @@ protected override async Task<string> ExecuteAsync(Params args, ToolExecutionCon
         if (string.IsNullOrWhiteSpace(args.Path))
             return "Error: 'Path' parameter is required.";
 
+        // Full-write mode: Content is supplied without OldString
+        if (args.Content is not null)
+        {
+            if (args.OldString is not null || args.NewString is not null || args.ReplaceAll is not null)
+                return "Error: 'Content' and 'OldString'/'NewString'/'ReplaceAll' are mutually exclusive. " +
+                       "Use Content for a full file write, or OldString/NewString for targeted replacement.";
+
+            return await WriteFileAsync(args.Path, args.Content, context, ct);
+        }
+
+        // Targeted edit mode: OldString → NewString
         if (string.IsNullOrEmpty(args.OldString))
-            return "Error: 'OldString' parameter is required and must not be empty.";
+            return "Error: either 'OldString' (for targeted edit) or 'Content' (for full write) is required.";
+
+        if (args.NewString is null)
+            return "Error: 'NewString' is required when using OldString for targeted replacement.";
 
         if (args.NewString == args.OldString)
             return "Error: 'NewString' must be different from 'OldString'.";
 
-        if (!_fileAccessPolicy.TryResolveWritePath(args.Path, context, out var authorizedPath, out var accessError))
+        return await EditFileAsync(args.Path, args.OldString, args.NewString, args.ReplaceAll == true, context, ct);
+    }
+
+    internal async Task<string> WriteFileAsync(string path, string content, ToolExecutionContext context, CancellationToken ct)
+    {
+        if (!_fileAccessPolicy.TryResolveWritePath(path, context, out var authorizedPath, out var accessError))
+            return accessError;
+
+        if (_pathPolicy?.IsDenied(authorizedPath) == true)
+            return FileToolErrors.ControlPlaneWriteDenied(authorizedPath);
+
+        try
+        {
+            var directory = System.IO.Path.GetDirectoryName(authorizedPath);
+            if (!string.IsNullOrEmpty(directory))
+                Directory.CreateDirectory(directory);
+
+            var bytes = Encoding.UTF8.GetBytes(content);
+
+            return await FileMutationGate.RunExclusiveAsync(authorizedPath, async () =>
+            {
+                await File.WriteAllBytesAsync(authorizedPath, bytes, ct);
+                return $"Successfully wrote {bytes.Length} bytes to {authorizedPath}";
+            }, ct);
+        }
+        catch (UnauthorizedAccessException)
+        {
+            return $"Error: Permission denied: {authorizedPath}";
+        }
+        catch (IOException ex)
+        {
+            return $"Error writing file: {ex.Message}";
+        }
+    }
+
+    private async Task<string> EditFileAsync(string path, string oldString, string newString, bool replaceAll, ToolExecutionContext context, CancellationToken ct)
+    {
+        if (!_fileAccessPolicy.TryResolveWritePath(path, context, out var authorizedPath, out var accessError))
             return accessError;
 
         if (_pathPolicy?.IsDenied(authorizedPath) == true)
@@ -74,8 +129,7 @@ protected override async Task<string> ExecuteAsync(Params args, ToolExecutionCon
             {
                 var content = await File.ReadAllTextAsync(authorizedPath, Encoding.UTF8, ct);
 
-                var replaceAll = args.ReplaceAll == true;
-                var occurrences = CountOccurrences(content, args.OldString);
+                var occurrences = CountOccurrences(content, oldString);
 
                 if (occurrences == 0)
                     return $"Error: The specified text was not found in {authorizedPath}";
@@ -89,17 +143,16 @@ protected override async Task<string> ExecuteAsync(Params args, ToolExecutionCon
 
                 if (replaceAll)
                 {
-                    newContent = content.Replace(args.OldString, args.NewString, StringComparison.Ordinal);
+                    newContent = content.Replace(oldString, newString, StringComparison.Ordinal);
                     replacementCount = occurrences;
                 }
                 else
                 {
-                    // Single replacement at first occurrence
-                    var index = content.IndexOf(args.OldString, StringComparison.Ordinal);
+                    var index = content.IndexOf(oldString, StringComparison.Ordinal);
                     newContent = string.Concat(
                         content.AsSpan(0, index),
-                        args.NewString,
-                        content.AsSpan(index + args.OldString.Length));
+                        newString,
+                        content.AsSpan(index + oldString.Length));
                     replacementCount = 1;
                 }
 

src/Netclaw.Actors/Tools/FileReadTool.cs

@@ -33,6 +33,8 @@ public sealed partial class FileReadTool : NetclawTool<FileReadTool.Params>
     private static readonly Encoding StrictUtf32Be = new UTF32Encoding(bigEndian: true, byteOrderMark: true, throwOnInvalidCharacters: true);
     private static readonly Encoding Windows1252 = new Windows1252Encoding();
 
+    public override bool SuppressOutputRedaction => true;
+
     private readonly ToolConfig _config;
     private readonly ToolPathPolicy? _pathPolicy;
     private readonly ScopedFileAccessPolicy _fileAccessPolicy;