Skip to content

fix(redact): non-reusable sentinel for prefix secrets in file reads (#35519) - #53146

Closed
kshitijk4poor wants to merge 1 commit into
mainfrom
salvage/35519-redact-nondestructive
Closed

fix(redact): non-reusable sentinel for prefix secrets in file reads (#35519)#53146
kshitijk4poor wants to merge 1 commit into
mainfrom
salvage/35519-redact-nondestructive

Conversation

@kshitijk4poor

Copy link
Copy Markdown
Collaborator

Summary

A user's API keys are no longer corrupted (or exposed) when the agent reads a config file. Prefix-matched credentials in file content are now redacted to a non-reusable sentinel instead of a plausible-looking truncated key.

Root cause (#35519)

With security.redact_secrets on (the default), read_file / search_files / cat applied redact_sensitive_text(code_file=True) to file content. code_file=True skips the ENV/JSON patterns but still runs prefix masking, so an API key in config.yaml (ghp_…, sk-…, xai-…, …) came back as a head/tail mask like ghp_S1...Pn2T — a plausible-looking truncated key. When the agent read that value and wrote it back (re-configuring, constructing a curl, etc.), the masked string replaced the real credential and silently broke auth (401). Production evidence: a config.yaml found containing the exact 13-char masked GitHub PAT.

Why not just stop redacting config files?

The two community PRs (#35529 config_file=True, #35534 redact_prefixes=False) fix the corruption by not masking prefixes for config reads. But that returns the user's real, unmasked keys into the agent context, the model, and any logs — a security regression. This PR keeps redacting while making the output non-destructive.

Fix

  • _mask_token_nonreusable: prefix secrets → «redacted:ghp_…». The vendor prefix label is kept for debuggability (the agent can still tell which credential is present), but zero secret bytes are emitted, and the angle-bracket/ellipsis wrapper is syntactically invalid as a token — so it can't be mistaken for, or written back as, a usable key.
  • redact_sensitive_text(file_read=True) routes prefix matches through the sentinel (implies code_file=True). Default / log / display mode is unchanged_mask_token still keeps head/tail (fine for logs, which are never written back).
  • Wired the 3 file_tools.py call sites (read_file, search_files, cat) to file_read=True.

This fixes the corruption and avoids the secret-exposure of the un-redact approach.

Validation

  • tests/agent/test_redact.py88 passed (6 new).
  • New tests: sentinel shape, no-leak (raw key body / head / tail never appear), not-a-plausible-key (sentinel can't round-trip into a dead token), default mode unchanged (regression guard), file_read implies code_file, sk- prefix.
  • Mutation-verified: reverting file_read to the old _mask_token fails the sentinel + no-leak tests.
  • ruff clean. (3 pre-existing unrelated TestSensitivePathCheck failures in test_file_tools.py reproduce identically on clean origin/main — not caused by this change.)

Credit

Diagnosis and the config-read fix direction from @liuhao1024 (#35529) and @adammatski1972 (#35534) — credited as co-authors. This PR implements a safer variant (redact-but-non-destructive) that doesn't expose secrets.

Closes #35519. Supersedes #35529, #35534.

…35519)

When security.redact_secrets is on (default), read_file/search_files/cat
applied redact_sensitive_text(code_file=True) to file content, which still
ran prefix masking. An API key in config.yaml (ghp_..., sk-..., xai-..., etc.)
came back as a head/tail mask like `ghp_S1...Pn2T` — a plausible-looking
truncated key. When an agent read that and wrote it back to config, the masked
value replaced the real credential, silently breaking auth (401). Production
evidence: a config.yaml found containing the exact 13-char masked GitHub PAT.

The two community PRs (#35529, #35534) fixed the corruption by NOT redacting
prefixes for config reads — but that exposes the user's real keys to the agent
context, model, and logs (a security regression). This takes the safer route:
keep redacting, but for file content emit a NON-REUSABLE sentinel.

- New `_mask_token_nonreusable`: prefix secrets -> `«redacted:ghp_…»` (vendor
  label preserved for debuggability; zero secret bytes; angle-bracket/ellipsis
  wrapper is syntactically invalid as a token so it can't be mistaken for or
  written back as a usable key).
- New `redact_sensitive_text(file_read=True)` routes prefix matches through it
  (implies code_file=True). Default/log/display mode is UNCHANGED — `_mask_token`
  still keeps head/tail (fine for logs, never written back).
- Wired the 3 file_tools.py call sites (read_file / search_files / cat) to
  file_read=True.

Fixes both the corruption AND avoids the secret-exposure of the un-redact
approach. 6 new tests (sentinel shape, no-leak, not-a-plausible-key, default
mode unchanged, file_read implies code_file, sk- prefix); 88 redact tests pass;
mutation-verified (reverting to the old mask fails the sentinel/leak tests).

Co-authored-by: liuhao1024 <sunsky.lau@gmail.com>
Co-authored-by: adammatski1972 <289282750+adammatski1972@users.noreply.github.com>

Closes #35519. Supersedes #35529, #35534.
@github-actions

Copy link
Copy Markdown
Contributor

🔎 Lint report: salvage/35519-redact-nondestructive vs origin/main

ruff

Total: 0 on HEAD, 0 on base (➖ 0)

🆕 New issues: none

✅ Fixed issues: none

Unchanged: 0 pre-existing issues carried over.

ty (type checker)

Total: 11488 on HEAD, 11490 on base (✅ -2)

🆕 New issues (1):

Rule Count
invalid-assignment 1
First entries
tests/run_agent/test_credits_notices_toggle.py:76: [invalid-assignment] invalid-assignment: Object of type `None` is not assignable to attribute `_credits_session_start_micros` of type `int`

✅ Fixed issues (2):

Rule Count
unresolved-attribute 2
First entries
run_agent.py:3002: [unresolved-attribute] unresolved-attribute: Object of type `Self@get_credits_spent_micros` has no attribute `_credits_session_start_micros`
tests/run_agent/test_credits_notices_toggle.py:76: [unresolved-attribute] unresolved-attribute: Unresolved attribute `_credits_session_start_micros` on type `AIAgent`

Unchanged: 6034 pre-existing issues carried over.

Diagnostics are surfaced as warnings — this check never fails the build.

@alt-glitch alt-glitch added type/security Security vulnerability or hardening comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint tool/file File tools (read, write, patch, search) area/auth Authentication, OAuth, credential pools P2 Medium — degraded but workaround exists sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data labels Jun 26, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related: competes with #35529 (config_file=True) and #35534 (redact_prefixes=False), which both fix the #35519 credential-corruption by skipping prefix masking for config reads — that returns the user's real unmasked keys into agent context/logs (a security regression). This PR keeps redacting while making the output a non-reusable, write-back-safe sentinel, so it avoids both the corruption and the leak. Maintainer should pick one approach.

@teknium1

Copy link
Copy Markdown
Contributor

Merged via #54166 (rebased onto current main, your commit de928bc keeps your authorship in git log). Thanks for the non-destructive-sentinel direction — that's the variant that shipped. #54166

@teknium1 teknium1 closed this Jun 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/auth Authentication, OAuth, credential pools comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P2 Medium — degraded but workaround exists sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data tool/file File tools (read, write, patch, search) type/security Security vulnerability or hardening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: redact_sensitive_text corrupts API keys in config files when read via read_file/search_files, causing 401

3 participants