Skip to content

fix(redact): mask Cookie / Set-Cookie session tokens - #50607

Open
ly-wang19 wants to merge 1 commit into
NousResearch:mainfrom
ly-wang19:fix/redact-cookie-headers
Open

ly-wang19 wants to merge 1 commit into
NousResearch:mainfrom
ly-wang19:fix/redact-cookie-headers

Conversation

@ly-wang19

Copy link
Copy Markdown

Summary

Follow-up to 6f0ecf37d ("mask all Authorization schemes and x-api-key style headers").

redact_sensitive_text masks Authorization / Proxy-Authorization and the x-api-key / api-key header family — the threat model that commit names is a logged request or echoed curl leaking live credentials. But Cookie / Set-Cookie headers were left untouched, so a session token — an account-takeover credential — leaks verbatim into logs, transcripts, and tool output.

Reproduced against current main:

>>> redact_sensitive_text("Cookie: sessionid=abcdef123456SECRET7890; csrftoken=xyz", force=True)
'Cookie: sessionid=abcdef123456SECRET7890; csrftoken=xyz'   # ← leaked
>>> redact_sensitive_text("Set-Cookie: session=topsecretvalue12345; HttpOnly; Path=/", force=True)
'Set-Cookie: session=topsecretvalue12345; HttpOnly; Path=/' # ← leaked
>>> redact_sensitive_text("authorization: Bearer sk-x", force=True)
'authorization: Bearer ***'                                  # (already masked)

Cookie carries arbitrarily many secret pairs (sessionid, csrftoken, auth tokens); Set-Cookie carries the cookie plus non-secret attributes (Path, HttpOnly, …). Neither is covered by the Authorization or x-api-key rules, and cookie is not in the JSON/query secret-key set.

Fix

Add a Cookie/Set-Cookie rule alongside the existing header rules. It masks each cookie value via _mask_token (the same masker used for Authorization/x-api-key, so the partial-reveal policy is consistent) while preserving cookie names and the non-secret Set-Cookie attributes (Path/Domain/Expires/Max-Age/SameSite + the HttpOnly/Secure flags) for debuggability. The value run stops at end-of-line or a closing quote so an inline curl -H 'Cookie: …' https://host doesn't swallow the trailing URL.

After the fix:

Cookie: sessionid=abcdef...7890; csrftoken=***
Set-Cookie: session=topsec...2345; HttpOnly; Path=/
curl -H 'Cookie: auth=***' https://api.example.com/v1/x

Tests

tests/agent/test_redact.py::TestCookieHeaders — Cookie multi-pair masking (names preserved), Set-Cookie value masked with attributes preserved, and a curl-command case (URL preserved). All three fail without the rule; the full suite (85 tests) passes with it.

redact_sensitive_text masks Authorization and x-api-key / api-key style headers
(the 6f0ecf3 threat model: a logged request or echoed curl leaking live
credentials), but Cookie / Set-Cookie headers were left untouched — so a
session token (an account-takeover credential) leaks verbatim into logs,
transcripts, and tool output.

Add a Cookie/Set-Cookie rule alongside the existing header rules: mask each
cookie value via _mask_token (the same masker used for Authorization/x-api-key)
while preserving cookie names and the non-secret Set-Cookie attributes
(Path/Domain/Expires/Max-Age/SameSite + HttpOnly/Secure flags). The value run
stops at end-of-line or a closing quote so an inline `curl -H 'Cookie: …' URL`
doesn't swallow the trailing URL.

Adds 3 tests; all fail without the rule.
@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 duplicate This issue or pull request already exists labels Jun 22, 2026
@alt-glitch

Copy link
Copy Markdown

Duplicate of #50514 — same Cookie/Set-Cookie session-token masking in agent/redact.py (same _COOKIE_HEADER_RE mechanism via _mask_token, same follow-up to merged #50423). #50514 is the earlier, broader PR (it additionally covers JSON-serialized secret headers at the same on-disk request/response dump sink).

@ly-wang19

Copy link
Copy Markdown
Author

Quick repro against current main (the gap this closes):

>>> redact_sensitive_text("Cookie: sessionid=abcdef123456SECRET7890; csrftoken=xyz", force=True)
'Cookie: sessionid=abcdef123456SECRET7890; csrftoken=xyz'   # ← session token leaks verbatim
>>> redact_sensitive_text("authorization: Bearer sk-x", force=True)
'authorization: Bearer ***'                                  # already masked by 6f0ecf37d

This is a direct follow-up to 6f0ecf3 — same threat model (a logged request / echoed curl leaking a live credential), just the Cookie / Set-Cookie family that the Authorization + x-api-key rules don't cover. Session cookies are account-takeover credentials, so the leak is higher-impact than an API key in some flows.

It's one self-contained rule mirroring the existing header logic (mask the value via _mask_token, keep cookie names + non-secret Set-Cookie attributes), with 3 regression tests that fail without it; the full test_redact.py suite (85) passes. Still merges cleanly on main.

CI hasn't been able to run yet (first-time contributor) — whenever a maintainer has a moment, could the workflows be approved here? Happy to adjust scope or rebase. Thanks!

@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 addressing a real raw-header redaction gap: current main bb5fc723b67c still applies only Authorization and API-key header rules at agent/redact.py:595-610.

Problems

  • agent/redact.py:158 excludes quote characters from the header payload. For Cookie: sid="opaque", the match stops after sid=; _COOKIE_PAIR_RE at line 161 then has no value to mask, leaving the opaque token unredacted. Please cover quoted cookie values while preserving the outer curl quotation behavior.
  • This PR does not cover JSON-serialized "Cookie": "..." headers. Current main's JSON key matcher is at agent/redact.py:177-181; Cookie is absent there. The duplicate discussion correctly points to #50514, whose diff includes that sibling request-dump representation.

Suggested changes

  • Add quoted-cookie regression cases and adjust the matcher accordingly.
  • Consolidate the JSON-header path with the broader #50514 approach so both representations are protected.

Automated hermes-sweeper review.

Comment thread agent/redact.py
# Header value runs to end-of-line or a closing quote (so an inline
# ``curl -H 'Cookie: …' https://host`` doesn't swallow the trailing URL).
_COOKIE_HEADER_RE = re.compile(
r"((?:Set-)?Cookie:\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.

This excludes a quote at the start of a cookie value. Cookie: sid="opaque" therefore matches only sid=, so _COOKIE_PAIR_RE receives no value to mask and opaque remains outside the replacement. Please distinguish quoted cookie values from the outer curl-command quote and add a regression.

@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 area/sessions Session lifecycle, resume, persistence, history labels Jul 15, 2026

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 area/sessions Session lifecycle, resume, persistence, history comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint duplicate This issue or pull request already exists 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.

3 participants