fix: mask secrets in long strings and dict keys in code variables - #988
Merged
Merged
Conversation
Search strings up to the 2,048-character pattern-match cap for known credential formats, instead of stopping at 200 characters, so a key inside a longer string such as an inlined SQL query is redacted. Mask dict keys, not only their values. A non-string key is rendered as a repr, which can embed field values, so a pattern match now replaces the key with a numbered placeholder. Keys that look like secrets or are too long to scan get the same placeholder, and URL credentials are removed from string keys. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GaMtfy4QuhjzpquxM9p4fN
Contributor
posthog-python Compliance ReportDate: 2026-09-29T16:03:29.285013+00:00 ✅ All Tests Passed!121/121 tests passed Capture_V1 Tests✅ 95/95 tests passed View Details
Capture_Ai Tests✅ 5/5 tests passed View Details
Feature_Flags Tests✅ 17/17 tests passed View Details
Feature_Flags_Local_Evaluation Tests✅ 4/4 tests passed View Details
|
Contributor
|
[Medium risk] Expands secret masking to longer strings and dictionary keys. The PR appears safe to merge, though key probing can reduce the usefulness of captured exception variables. Reviews (2) · Last reviewed commit: "fix: bound key probes and fail closed on..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
- Replace a string key that matches a mask pattern unless it is shaped like a field name, so `password=...` or SQL text used as a key cannot leak. - Replace a non-string key when masking its parts redacts anything; its repr defeats the entropy check. - Give a key a placeholder when its str() raises, instead of aborting the whole variable into the repr fallback. - Keep both entries when two keys end up with the same text, e.g. URLs that differ only in credentials, or a literal key equal to a placeholder. - Stop the OpenAI/Anthropic `sk-` patterns matching inside words like `disk-`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GaMtfy4QuhjzpquxM9p4fN
Contributor
🦔 PostHog Review reviewed this pull requestFound 2 must fix, 2 should fix, 0 consider. Published 4 findings (view the review). |
Contributor
|
PostHog Review alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
- Share the traversal's `seen` set with key probes, so probes count against the node budget and cycle guard. A probe that reaches an already visited object replaces the key, because that part of its repr went unchecked. - Replace the key when its probe raises, instead of falling back to the repr of the whole variable. - Redact the value of a key whose str() raises, because nothing shows whether the key names a secret. - Remove the `sk-` word-boundary lookbehind: a key often follows a letter or digit, e.g. `%3Dsk-...` in a URL, and the lookbehind let it through. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NHmek81fkmrddpN8MqWmCz
ablaszkiewicz
marked this pull request as ready for review
September 29, 2026 15:45
A key probe recorded its visits in the traversal's `seen` set, so an object that a key shares with a value rendered as `<circular ref>` in the value. Probes now record visits under a separate tag. They still stop at every visited object and count against the same node budget. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NHmek81fkmrddpN8MqWmCz
dustinbyrne
approved these changes
Sep 29, 2026
dustinbyrne
left a comment
dustinbyrne
left a comment
Contributor
There was a problem hiding this comment.
Reviewed the expanded masking window, dictionary-key collisions, unreadable keys and shared traversal budget, including the MCP detector caller. The earlier review findings are addressed. No issues found.
Validation: static source review and existing CI; no tests or device runs executed for this review.
Human-driven, agent-assisted review.
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.
💡 Motivation and Context
ConnectionKey(..., proxy_auth=BasicAuth(..., password=...))leaks proxy credentials.Changes
$$_posthog_redacted_key_<n>_$$when its text could hold a secret: a mask match that is not a plain field name, a secret-looking key, a non-string key with parts that need masking, an unreadable key, or a key too long to scan.passwordkeep their key, and only the value is redacted, as before.mainConnectionKey(..., password=...)("admin", "<random token>"){"password": "hunter2"}{"password": <REDACTED>}Warning
Behavior change:
{("db", "password"): "x"}now masks to{"$$_posthog_redacted_key_0_$$": <REDACTED>}.Note
The
sk-patterns stay unanchored, so words such asdisk-usage-…are over-redacted in longer strings too. A word boundary would miss%3Dsk-…in URLs.Performance: median ms per capture, local benchmark (not committed), default configuration.
main💚 How did you test it?
mainand pass here, one or more for each leak above.sk-key after%3Dstays redacted.make prep_localinside the PostHog app.📝 Checklist
If releasing new changes
sampo addto generate a changeset file🤖 Agent context
Autonomy: Human-driven (agent-assisted)
claude-opus-5-5). Skills: qa-swarm, writing-tests, writing-code-comments, writing-pr-descriptions.sk-(495e712), because it missed keys in percent-encoded URLs.sampo addformat, because the CLI is not installed locally.🤖 Generated with Claude Code
https://claude.ai/code/session_01NHmek81fkmrddpN8MqWmCz