fix: prevent secret placeholder writeback on file read/write tools (#1333)#1343
Merged
Aaronontheweb merged 2 commits intoJun 6, 2026
Conversation
Aaronontheweb
added
bug
…older writeback (netclaw-dev#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
- 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')
Aaronontheweb
force-pushed
the
worktree-fix-1333-secret-redact-writeback
branch
from
69911aa to
02f6737
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
SecretOutputRedactorwas replacing real secret values with***REDACTED***infile_readoutput before it reached the model. When the model wrote that content back viafile_write/file_edit, the placeholders got persisted to disk, destroying real secrets in config files likeappsettings.json(#1333).The fix: redaction belongs on the observability path (spill files, session logs), not on the model-facing tool result.
FileReadToolnow opts out of model-facing redaction via a newINetclawTool.SuppressOutputRedactionflag while the dispatcher still writes redacted content to spill files on disk.Separately, this consolidates
file_writeintoFileEditToolas aContentparameter mode.FileWriteToolstays registered under thefile_writetool name but delegates toFileEditTool.WriteFileAsync.Changes
INetclawTool.SuppressOutputRedaction(defaultfalse) tellsDispatchingToolExecutorto skipSecretOutputRedactoron the result sent back to the model.FileReadToolsets it totrue. The dispatcher computes both raw and redacted versions: raw goes to the model, redacted goes toToolOutputSpillfor the on-disk spill file. StreamingToolActivityUpdatechunks always redact regardless of the flag since they're progressive display, not content the model writes back.FileEditTool.Paramspicks up an optionalContentparameter for full-file writes, mutually exclusive withOldString/NewString/ReplaceAll. The write logic lives inFileEditTool.WriteFileAsync(internal), andFileWriteTooldelegates there so existing sessions, approval grants, and audience profiles keep working under thefile_writename.SessionLogActornow redactsToolResultOutput.Resultbefore writing tosession.log, closing a leak whereSuppressOutputRedactionwould have put raw secrets into the observability log. UpdatedToolOutputSpilldoc comments to match the two-param overload.Test plan
File_read_preserves_secret_values_for_model- reads file with secrets, verifies model result is unredactedShell_output_still_redacts_secrets- verifies shell_execute output is still redactedFile_read_spill_file_is_redacted_even_when_model_result_is_not- forces spill, verifies model sees raw content but spill file on disk is redactedContent_creates_new_file,Content_overwrites_existing_file,Content_creates_parent_directories- FileEditTool Content modeContent_and_OldString_are_mutually_exclusive,Content_and_NewString_are_mutually_exclusive,Content_and_ReplaceAll_are_mutually_exclusive- validation