Security: Replace pickle serialization with JSON (CWE-502)#1646
Open
jamestford wants to merge 1 commit into
Open
Security: Replace pickle serialization with JSON (CWE-502)#1646jamestford wants to merge 1 commit into
jamestford wants to merge 1 commit into
Conversation
- Replace pickle with JSON for session file storage (CWE-502) - Robinhood: session files now stored as .json instead of .pickle - TDA: encrypted tokens stored as base64-encoded strings in JSON - TDA: datetime objects stored as ISO format strings - Bumped version to 3.5.0 BREAKING CHANGE: Existing .pickle session files will not be loaded. Users will need to re-authenticate once after updating.
DhruvaBansal00
added a commit
to DhruvaBansal00/robin_stocks
that referenced
this pull request
May 23, 2026
Upstream PR jmfernandes#1646 (jmfernandes/robin_stocks). Replaces pickle with JSON for the Robinhood and TDA session caches. Eliminates CWE-502 (arbitrary code execution via crafted pickle on disk) and makes the cache files human-readable. For TDA, the encrypted credential bytes are base64-encoded before being written, and datetime stamps are stored as ISO-8601 strings. Also threads the format change through the MCP layer: - robin_stocks_mcp/auth.py now reads ~/.tokens/robinhood.json via json.load() and reports session-reuse failures with the new wording. - The existing auth-bootstrap tests now write JSON instead of pickle, plus a new test confirms a malformed session file does not abort the bootstrap. - Help text and printed messages no longer reference "pickle". Breaking change: existing ~/.tokens/robinhood.pickle and ~/.tokens/tda.pickle files will not be loaded — users get prompted to log in once. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
DhruvaBansal00
added a commit
to DhruvaBansal00/robin_stocks
that referenced
this pull request
May 23, 2026
DhruvaBansal00
added a commit
to DhruvaBansal00/robin_stocks
that referenced
this pull request
May 23, 2026
Adds a credential-free, fully-mocked test suite covering the changes landed across the 12 upstream-PR merges plus the surrounding SDK. New tests/sdk/ tree (mock-based, no network/credentials): - test_urls: every URL builder we touched (options, futures, recurring, tax-lots, marketdata-index) - test_helper / test_helper_requests: index routing, rate limiter + thread safety, request_get/post/delete/document branches, filter_data - test_authentication_robinhood / test_auth_verification: login JSON persistence (jmfernandes#1646), store_session=False fix (jmfernandes#1643), the device verification polling flow - test_authentication_tda: JSON + base64 + Fernet roundtrip, refresh branches - test_orders / test_order_wrappers / test_option_crypto_orders: market_hours coupling (jmfernandes#454), tax-lot payloads, every order wrapper - test_options_index_routing / test_options_remaining: index chain symbols (jmfernandes#541), find_options_by_* variants, historicals validation - test_stocks / test_stocks_index_routing: quote routing, historicals - test_crypto: open-positions nonzero filter (jmfernandes#333) - test_futures: full module coverage incl. pagination + P&L helpers - test_recurring_investments: create/update/cancel payloads + branches - test_account / test_markets / test_misc_modules: legacy sweeps New tests/mcp tests: - test_all_tools_dispatch: introspection-based generic dispatch test that verifies all 206 registered MCP tools forward to their SDK function (synthesizes args from each tool's signature) - test_remaining_tools: download + TDA-auth tools with bespoke returns Results: 801 passing, 1 skipped. Coverage: 100% on every MCP tool module and on the modules we authored (futures, recurring_investments, markets, tda/authentication); 92% overall across robinhood + MCP. Remaining gaps are legacy SDK paths untouched by these PRs. Also adds pytest-cov to the dev extra and ignores coverage artifacts. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Thanks for this contribution! It's included with test coverage in the new repo, alongside 11 other open PRs from here. If you'd like a maintained build with this change, that's where to find it. Shipped as-is; note it's a breaking change for existing pickle session files (one-time re-login). |
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.
Summary
This PR replaces all uses of Python
picklefor session file storage withjson, eliminating a class of arbitrary code execution vulnerabilities (CWE-502: Deserialization of Untrusted Data).Why This Matters
Python's
picklemodule can execute arbitrary code during deserialization. Whenpickle.load()is called on a file, it can instantiate any Python object and run any code — including malware, reverse shells, or file deletion. This is not a theoretical risk; it is one of the most well-documented attack vectors in Python.Current risk scenario: An attacker with write access to
~/.tokens/robinhood.pickle(or equivalent TDA file) can replace it with a crafted pickle payload. The next time the user's script callslogin(), the malicious code executes with full user privileges — no password needed, no warning given.This has been flagged by three independent security scanners in our audit:
avoid-pickle)What This PR Changes
Robinhood (
robin_stocks/robinhood/authentication.py)import pickle→import json.pickle(binary) →.json(plaintext)pickle.load(f)→json.load(f)pickle.dump(data, f)→json.dump(data, f, indent=2)rb/wb→r/wTDA (
robin_stocks/tda/authentication.py)import pickle→import json+import base64datetimeobjects → ISO 8601 strings (datetime.isoformat()/datetime.fromisoformat())pickle.dump/pickle.loadcalls replaced with JSON equivalentsTDA Globals (
robin_stocks/tda/globals.py)PICKLE_NAME = "tda.pickle"→SESSION_NAME = "tda.json"with backward-compatible aliasVersion
3.5.0(feature-level change due to file format change)Do We Lose Any Functionality?
No. The session data consists entirely of:
All of these are natively representable in JSON. Pickle's ability to serialize arbitrary Python objects is the vulnerability, not a feature being used by this codebase.
Breaking Change
Existing
.picklesession files will not be loaded by the updated code. Users will need to log in once after updating, which creates a new.jsonsession file. This is the same experience as when a pickle file becomes corrupted (issue #451, #524).The parameter names
pickle_pathandpickle_nameare preserved for backward API compatibility.Related Issues
This PR addresses the root cause behind several existing issues:
Testing
The session data structure is unchanged — only the serialization format differs. All existing tests that don't directly test pickle file internals should pass without modification. The
login()andlogout()function signatures are unchanged.