fix(secrets): span-aware redaction to prevent double-skip and chain-replace bugs#726
Merged
Merged
Conversation
…eplace bugs
The early-return check `content.contains("[REDACTED_SECRET:")` caused fresh
secrets in the same text to be silently skipped. Replace it with overlap-based
span protection that shields existing markers while still detecting new secrets.
Rewrite restore_secrets to walk the string left-to-right using the marker regex,
eliminating HashMap-iteration-based replacement that could chain-replace when a
secret value looked like another key.
Wire SecretManager::redact_and_store_secrets into async and interactive agent
modes so user prompts and feedback are redacted before reaching the LLM.
Add 18-test regression suite covering stale markers, session-growth redaction,
shell-special chars in restore, adjacent markers, and determinism.
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
2 participants
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.
Description
Fix secret redaction regressions that caused fresh secrets to be silently skipped and restoration to chain-replace incorrectly.
Bugs fixed
Double-redaction early-return skip:
redact_secretsreturned immediately whencontent.contains("[REDACTED_SECRET:"), so any new secret in the same text was never detected. Overhaul to use span-based overlap checks — existing markers are protected from re-detection while fresh secrets are still redacted.restore_secretschain-replacement: Iterating aHashMapand callingString::replaceper entry could rewrite previously-restored markers when a secret value resembled another key. Rewrite to walk left-to-right with the marker regex, replacing each marker exactly once in order.redact_passwordearly-return skip: Samecontains("[REDACTED_SECRET:")guard prevented password redaction when any stale marker was present. Fixed with the same span-based overlap filter.CLI secret leak in prompts: User input and feedback in
mode_async.rsandmode_interactive.rswere not routed throughSecretManager::redact_and_store_secrets, so secrets reached the LLM in plaintext. wired redaction into both paths.Related Issues
Fixes secret redaction regressions reported by Abdalla.
Changes Made
libs/shared/src/secrets/mod.rs: Span-aware redaction withfind_protected_spans/overlaps_protected_span; left-to-rightrestore_secretsusingREDACTED_SECRET_MARKER_REregex;redact_passworduses same span filtercli/src/commands/agent/run/mode_async.rs: Route user input + feedback throughSecretManager::redact_and_store_secretscli/src/commands/agent/run/mode_interactive.rs: Route user input throughSecretManager::redact_and_store_secretslibs/shared/tests/secret_redaction_regressions.rs: 18-test regression suiteTesting
cargo test -p stakpak-shared --test secret_redaction_regressions— 18/18)cargo clippy --all-targets -- -D warnings— zero warningscargo fmt --check— cleanBreaking Changes
None — public API signatures unchanged; internal logic fixes only.