Skip to content

fix(redact): close header-credential leaks #50423 missed (JSON secret headers + Cookie/Set-Cookie) - #50514

Open
r266-tech wants to merge 2 commits into
NousResearch:mainfrom
r266-tech:fix/redact-header-cred-leaks-50423
Open

r266-tech wants to merge 2 commits into
NousResearch:mainfrom
r266-tech:fix/redact-header-cred-leaks-50423

Conversation

@r266-tech

@r266-tech r266-tech commented Jun 22, 2026 •

Copy link
Copy Markdown

Opaque JSON API-key headers and Cookie/Set-Cookie values remain visible in diagnostic request dumps on main. This follow-up to #50423 masks those headers while preserving parseable JSON, shell quote delimiters, the curl destination, and subsequent header lines.

JSON values are decoded, masked, and JSON-encoded so escaped quotes and backslashes cannot be split at a mask boundary. Raw and curl header handling uses bounded contexts, including continued commands and truncated diagnostic lines. JSON and Cookie source fixtures retain the existing code_file exemption. The current shared SECRET_HEADER_NAME_LIST, profile/vault handling, escaped-field handling, and Python-repr redaction are retained. Secret-bearing file reads also retain the current non-reusable mask sentinel.

Validation on macOS, Python 3.14, rebased on f86276d:

  • Canonical scripts/run_tests.sh: 242 tests passed across seven related redaction files, including two committed syntax/preservation invariants.
  • Both committed invariants fail on the same upstream base.
  • Real dump_api_request_debug with temporary storage and synthetic headers writes parseable, redacted JSON; no provider requests.
  • python scripts/check --base upstream/main: all 11 checks passed; git diff --check passed.

Overlapping proposals #50607/#83002 cover cookie subsets, #69196 covers general JSON fields, and #51049 covers dump-specific fields; none has merged at verification. This PR remains limited to the existing header-redaction topic.

AI-assisted implementation and self-review. GitHub CI has required maintainer approval; local validation is reported separately.

@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 area/auth Authentication, OAuth, credential pools P2 Medium — degraded but workaround exists labels Jun 22, 2026

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: fix(redact): header-credential leaks in JS

Security fix — closes two redaction gaps:

  1. Secret headers serialized as JSON ("x-api-key": "<value>") — bare-header regex can't match the closing quote. Opaque custom/local-backend keys leaked verbatim.
  2. Cookie headers (Cookie: / Set-Cookie:) — previously unhandled entirely. Session, auth, and CSRF tokens passed through in cleartext.

Good:

  • Escape-aware JSON string matching correctly handles embedded escaped quotes
  • name=value requirement on bare cookie regex avoids false positives on prose
  • Test coverage: bare form, JSON form, escaped quotes, mixed case, prose, combined request dump, and code_file=True passthrough

No issues — approving.

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for covering the serialized-header and cookie redaction gaps; both remain on current main (agent/redact.py:604-610), and request dumps serialize then redact their payload at agent/agent_runtime_helpers.py:1502-1505.

Problems

  • agent/redact.py:165 uses \S+=[^\r\n]+ for raw Cookie values. This consumes quote delimiters and subsequent same-line text. An inline curl -H "Cookie: sid=secret" https://… would be reduced to an unterminated command and lose its URL. Current main fixed the identical Authorization quote-eating class in bbe1bf4045427b2dca7d8e4acbe042969cd3ad6a by excluding quote characters from the credential match.

Suggested changes

  • Bound the raw Cookie payload at quote delimiters and add quoted single/double curl -H regressions that retain the quote and URL.

This is an automated hermes-sweeper review.

Comment thread agent/redact.py Outdated
# is preserved. A ``name=value`` pair is required so plain prose ("the cookie:
# …") is left untouched.
_COOKIE_HEADER_RE = re.compile(
r"((?:Set-)?Cookie:\s*)(\S+=[^\r\n]+)",

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[^\r\n]+ consumes closing shell quotes and trailing same-line command text. Bound this raw Cookie payload before '/" and add quoted curl -H regressions; current main made that exact quote-preservation fix for Authorization in bbe1bf4.

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit labels Jul 15, 2026
@egilewski

Copy link
Copy Markdown

suggesting changes

The new JSON header substitutions can turn valid serialized headers into invalid JSON. They pass the regex-captured escape stream directly to _mask_token(), so an escape pair crossing the preserved-prefix boundary is split: a Cookie value containing abcd=\"opaque-session-secret-1234567890\"; Path=/ serializes correctly but redacts to an invalid abcd=\...th=/ JSON escape. The same failure occurs for x-api-key.

This reaches the request-dump path because agent_runtime_helpers.py immediately calls json.loads() on the redacted serialization before writing it; the parse error makes the dump fail and return None. Please mask decoded header values and re-serialize them, or otherwise preserve complete JSON escape pairs, and add regressions asserting that escaped quotes at mask boundaries remain parseable after redaction.

Security evidence:

  • trust boundary: credential-bearing serialized headers cross into persisted or printed diagnostic output.
  • source/sink/invariant: JSON header captures are masked as encoded text and then reparsed; redaction must remove credentials while preserving valid JSON structure.
  • current-main reproduction: current main leaves the synthetic JSON Cookie and x-api-key secrets visible but keeps their serialization parseable.
  • PR-head or patch-replay validation: the patch replay removes both secrets but produces an invalid \... escape at the mask boundary, and json.loads() raises JSONDecodeError.
  • positive/negative cases: ordinary Cookie, Set-Cookie, mixed-case, JSON, benign-prose, and code_file=True controls behave as intended.
  • residual bypass search: boundary-position escaped quotes fail for both Cookie and x-api-key; no additional secret-leak bypass was confirmed.
  • reviewer validation: the replay is bound to the reviewed head and current main; tests/agent/test_redact.py passes 109 tests, but it does not assert that escaped JSON remains parseable.

Not checked:

  • Full test suite
  • CodeRabbit review

Signed: GPT-5.6-sol-xhigh in Codex

@r266-tech

Copy link
Copy Markdown
Author

Rebased onto current main and pushed 5530ae3da.

The reported JSON invariant is now fixed by decoding captured header values before masking and JSON-encoding them again afterward, so masking cannot split an escape pair and the request-dump json.loads() step remains valid.

The replay also adds regressions for the adjacent boundaries exercised by this redaction path:

  • canonical JSON header handling runs before the overlapping legacy apikey matcher and fails closed on truncated LF/CRLF input without consuming the next line;
  • raw and curl API-key/Cookie forms use bounded contexts and fixed *** payloads, including quoted and continued commands;
  • physical-line patterns no longer cross newlines, and repeated malformed candidates stay bounded.

Validation on the rebased head:

  • PYTHONPATH=. uv run --frozen pytest tests/agent/test_redact.py -q — 170 passed
  • python3 -m py_compile agent/redact.py tests/agent/test_redact.py
  • git diff --check upstream/main...HEAD

The remaining scope is intentional: the persisted sink emits canonical keys via Python json.dumps, and a standalone line-start Cookie: name=value is treated fail-safe as a header; code_file=True remains exempt for source fixtures.

@r266-tech
r266-tech force-pushed the fix/redact-header-cred-leaks-50423 branch from 5530ae3 to d458eb1 Compare September 10, 2026 10:23
@r266-tech

Copy link
Copy Markdown
Author

Refreshed to current main (a0749d58) at d458eb18, retaining the earlier fixes for escaped JSON values, curl quoting/continuations, truncated lines and source-code exemptions. JSON header handling now lives in the extracted _redact_assignments path; the generic apikey matcher cannot remask its escaped result.

Validation on macOS: the current redaction suite (116 tests), two new syntax/security invariants, and the preserved prior 170-test suite all pass (288 total). Both new invariants fail on upstream. I also exercised the real dump_api_request_debug path against a temporary directory: it writes valid JSON with synthetic Cookie and x-api-key values redacted, with no provider requests.

The new committed test file keeps two invariants: parseable credential-free JSON headers and bounded raw/curl headers that preserve quoting, URLs and the next line. Scope remains header redaction; general JSON-field handling in #69196 and the cookie/dump-specific proposals remain separate. AI-assisted implementation and self-review.

shapes of the same leak class still reach the on-disk request-dump sink:
- JSON-serialized secret headers ("x-api-key": "<opaque>") — opaque
  custom/local-backend keys that have no vendor prefix for _PREFIX_RE.
- Cookie:/Set-Cookie: headers (session/auth/CSRF tokens), previously
  unhandled entirely.

Both classes are closed in raw and JSON-serialized form. The JSON value
regexes are escape-aware (escaped quotes can't truncate the match) and
the JSON cookie matcher uses a single unambiguous group (no ReDoS).
Adds regression tests for each form incl. the request-dump sink.
@r266-tech
r266-tech force-pushed the fix/redact-header-cred-leaks-50423 branch from d458eb1 to af746e3 Compare October 10, 2026 16:55

This branch has not been deployed

No deployments
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:blast-broad Sweeper blast radius: broad — a core path most sessions hit sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/security Security vulnerability or hardening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants