Fix FR-2: preserve Personal data during Pro app saves#63
Fix FR-2: preserve Personal data during Pro app saves#63patrickkidd-hurin wants to merge 1 commit intomasterfrom
Conversation
When the Pro app saves a full DiagramData blob via update_with_version_check, it omits Personal-owned fields (pdp, clusters, clusterCacheKey). Previously this replaced the entire data column, silently destroying those fields. The fix adds _merge_personal_fields() which deserializes both blobs and copies Personal-owned fields from existing data when absent in incoming data. When the Personal app writes (including all fields), no merge is needed and the incoming blob passes through unchanged. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical data loss issue where Personal app-specific data fields were being inadvertently deleted when the Pro app saved diagram data. The solution introduces a robust merging mechanism that ensures these sensitive fields are preserved during updates, thereby maintaining data integrity across different application contexts without altering the external interface or data structure. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request aims to resolve an issue where personal data was being lost during saves from the Pro app by introducing a _merge_personal_fields function to intelligently merge incoming and existing data. However, a critical security vulnerability has been identified: the use of pickle.loads() on untrusted user input within the _merge_personal_fields function. This allows for Remote Code Execution (RCE) via malicious pickle payloads in diagram data, including potential persistent RCE from database-stored payloads. It is strongly recommended to replace pickle with a safer serialization format like JSON for all user-supplied and database-stored diagram data. While the core logic for fixing data loss is sound, this security vulnerability must be addressed before merging.
|
|
||
| import PyQt5.sip # Required for unpickling QtCore objects | ||
|
|
||
| incoming = pickle.loads(incoming_blob) |
There was a problem hiding this comment.
The _merge_personal_fields function uses pickle.loads() on incoming_blob without validation, which is a critical security vulnerability. This incoming_blob originates from user-supplied data via the update route in btcopilot/personal/routes/diagrams.py, allowing an attacker to provide a malicious pickle payload for Remote Code Execution. The pickle module is not secure for untrusted data. It is highly recommended to switch to a safer serialization format like JSON, potentially implementing custom serialization/deserialization logic for complex objects.
| # Incoming data has all Personal-owned fields — nothing to merge. | ||
| return incoming_blob | ||
|
|
||
| existing = pickle.loads(existing_blob) |
There was a problem hiding this comment.
The function _merge_personal_fields also calls pickle.loads() on existing_blob, which is the current diagram data stored in the database (self.data). Since self.data can be updated by users via the update route with arbitrary data (as seen in the call to _merge_personal_fields on line 194), an attacker can store a malicious pickle payload in the database. Subsequent calls to this function will then execute the payload, leading to persistent Remote Code Execution (RCE).
|
This needs to happen but is not a priority right now. |
|
Ready for Patrick's review — CI passing, no conflicts. Tracked in patrickkidd/theapp#101. |
|
\ud83d\udce9 Task spawned \u2014 addressing your comment. |
|
Acknowledged — no action needed. PR is ready for your review. CI green, no conflicts. |
|
\ud83d\udce9 Follow-up queued \u2014 resuming agent session to address 3 comment(s). |
2 participants
Summary
Diagram.update_with_version_check()replaced the entire pickle blob whennew_datawas provided. Pro app saves omit Personal-owned fields (pdp,clusters,clusterCacheKey), so those were silently destroyed._merge_personal_fields()indiagram.pythat deserializes both existing and incoming blobs, and preserves Personal-owned fields from existing data when they're absent in the incoming data. When all fields are present (e.g. Personal app writes), the incoming blob passes through unchanged — zero overhead.Test plan
_merge_personal_fields()covering: PDP preservation, cluster preservation, incoming data respected when present, empty existing data, and all-fields-present pass-throughCloses patrickkidd/theapp#47
🤖 Generated with Claude Code
Closes #47