chore(storage): restrict auth token cache deserialization - #73372
Merged
Conversation
The periodic auth-token cache verifier unpickled Redis values with a bare pickle.loads. The cache only ever holds a JSON string or a plain dict, so this switches to a restricted unpickler that constructs no classes, plus a short repo policy on pickle usage.
Contributor
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
posthog/storage/test/test_auth_token_cache_verifier.py:913
**Unsuppressed pickle blocks security CI**
When the security job scans this test, the new `pickle.dumps` call matches the blocking `avoid-pickle` rule without a `nosemgrep` suppression, causing the CI job to fail; the suppression below applies only to the separate `pickle.loads` call.
Reviews (1): Last reviewed commit: "chore(storage): restrict auth token cach..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
🤖 CI report |
rnegron
approved these changes
Jul 24, 2026
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.
Problem
The periodic auth-token cache verifier deserialized Redis values with a bare
pickle.loads. Pickle runs whatever the byte stream tells it to during unpickling, so any path that could write to those cache keys becomes a code-execution vector. This is an internal cache, so it's defense-in-depth rather than an open door, but a barepickle.loadsis more trust than this path needs: the cache only ever holds a JSON string or a plain dict, so no class ever needs to be reconstructed here.Changes
pickle.loadsin the verifier for a restricted unpickler whosefind_classalways raises. JSON-shaped values (str, dict, list, number, bool, None) deserialize through pickle opcodes that never callfind_class, so this loads exactly today's data and refuses to construct any class or callable. The three production formats it reads (pickled JSON string, pickled dict, zstd + pickle) are unchanged. It mirrors the existing_CatalogUnpicklerinposthog/hogql/database/database.py..agents/security.md: don't add new pickle, use JSON / pydantic instead, and if it's genuinely unavoidable (an existing cross-language format contract), keep it to internal data behind a restricted unpickler with a justifiednosemgrep. It also notes the implicit pickle path through Django's default cache backend.How did you test this code?
I (Claude) wrote the test first.
TestAuthTokenCacheDeserializationHardeningfeeds the deserializer a__reduce__payload wired to a marker function. Against the old code the marker fired (the payload executed) while the function still returnedNone— the exact problem. After the change the marker never runs and the payload is rejected. I also confirmed all three production cache formats still deserialize. Ran thatSimpleTestCaselocally: passed. The DB-backed verifier tests need Postgres/Redis and run in CI.Automatic notifications
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Authored by Claude (Opus 4.8), directed by the assignee. Skills invoked:
superpowers:brainstorming,superpowers:test-driven-development,writing-tests.This came out of a broader look at pickle usage in the repo. The AI conversation stream, which could actually move off pickle, is being migrated to JSON in a separate three-PR stack. This cache can't move unilaterally because its format is a cross-language contract, so hardening the read was the right call here instead. The deny-all unpickler is stronger than an allowlist for this site because it needs zero classes.