Security: Unsafe deserialization in kb-store.ts via JSON.parse without validation

[tomaioo]

Summary

Security: Unsafe deserialization in kb-store.ts via JSON.parse without validation

Problem

Severity: Medium | File: desktop/src/main/kb-store.ts:L22

The KbStore._load() method reads a JSON file and parses it with JSON.parse() without any schema validation or sanitization. While this is a local file, if an attacker can modify the state.json file (e.g., via another compromised process or symlink attack), they could inject malicious data. More critically, the spread operator { ...DEFAULTS, ...JSON.parse(...) } only does shallow merging, which could lead to prototype pollution if the JSON contains __proto__ or constructor keys, potentially affecting downstream code that iterates over objects.

Solution

Add validation for parsed data using a schema validator like zod, or at minimum check that the parsed result is a plain object and sanitize dangerous keys. Consider using Object.create(null) for internal maps, or explicitly delete __proto__ and constructor from parsed objects before merging.

Changes

• desktop/src/main/kb-store.ts (modified)

Summary by CodeRabbit

• Bug Fixes
  • Improved loading of saved app state so invalid or unsafe data is handled more reliably.
  • Added checks to ignore malformed state and prevent problematic saved entries from affecting defaults.
  • Made startup more resilient when reading user preferences or local state files.

[coderabbitai]

📝 Walkthrough

Walkthrough

The _load() method in KbStore was updated to safely parse state.json, validate that the parsed value is a non-array object, and filter out potentially dangerous keys (__proto__, constructor) before merging the sanitized data with defaults.

Changes

State Loading Safety

Layer / File(s)	Summary
Guarded state parsing and key filtering desktop/src/main/kb-store.ts	_load() now validates parsed JSON shape and filters top-level keys to exclude __proto__ and constructor before merging with DEFAULTS, replacing the prior direct merge of unvalidated data.

Estimated code review effort: 1 (Trivial) | ~5 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the security fix in kb-store.ts by calling out unsafe JSON parsing without validation.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (2)

desktop/src/main/kb-store.ts (2)

38-48: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Dedupe the DEFAULTS deep-clone.

JSON.parse(JSON.stringify(DEFAULTS)) is duplicated between the invalid-shape fallback and the catch block. Extract a small cloneDefaults() helper (or use structuredClone(DEFAULTS) if targeting Node ≥ 17) to avoid the repetition.

♻️ Proposed dedupe

+function cloneDefaults(): StoreData {
+  return JSON.parse(JSON.stringify(DEFAULTS));
+}
+
 class KbStore {
   ...
   private _load(): StoreData {
     try {
       const raw = JSON.parse(fs.readFileSync(this.file, "utf8"));
       if (raw == null || typeof raw !== "object" || Array.isArray(raw)) {
-        return JSON.parse(JSON.stringify(DEFAULTS));
+        return cloneDefaults();
       }
       ...
       return { ...DEFAULTS, ...safe };
     } catch {
-      return JSON.parse(JSON.stringify(DEFAULTS));
+      return cloneDefaults();
     }
   }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@desktop/src/main/kb-store.ts` around lines 38 - 48, The DEFAULTS deep-clone
logic is duplicated in the invalid-shape fallback and the catch block inside the
kb-store loading flow. Extract that repeated cloning into a small helper such as
cloneDefaults() in kb-store.ts, or switch both call sites to
structuredClone(DEFAULTS) if supported, and use that helper consistently
wherever DEFAULTS needs to be returned.

---

41-45: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low value

Optional: also filter "prototype" for defense-in-depth.

Static analysis flags the missing "prototype" check alongside __proto__/constructor. In this specific shallow, single-level spread into a plain object literal, copying a prototype key isn't itself exploitable, but adding it costs nothing and matches the common CWE-1321 mitigation triad.

🛡️ Proposed tweak

-        if (key !== "__proto__" && key !== "constructor") {
+        if (key !== "__proto__" && key !== "constructor" && key !== "prototype") {
           safe[key] = raw[key];
         }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@desktop/src/main/kb-store.ts` around lines 41 - 45, The safe-copy loop in
kb-store should also exclude the "prototype" key alongside "__proto__" and
"constructor" for defense-in-depth. Update the filtering logic in the
raw-to-safe object copy so the Object.keys(raw) iteration skips "prototype" too,
keeping the same shallow copy behavior while matching the common CWE-1321
mitigation pattern.

Source: Linters/SAST tools

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@desktop/src/main/kb-store.ts`:
- Around line 38-48: The DEFAULTS deep-clone logic is duplicated in the
invalid-shape fallback and the catch block inside the kb-store loading flow.
Extract that repeated cloning into a small helper such as cloneDefaults() in
kb-store.ts, or switch both call sites to structuredClone(DEFAULTS) if
supported, and use that helper consistently wherever DEFAULTS needs to be
returned.
- Around line 41-45: The safe-copy loop in kb-store should also exclude the
"prototype" key alongside "__proto__" and "constructor" for defense-in-depth.
Update the filtering logic in the raw-to-safe object copy so the
Object.keys(raw) iteration skips "prototype" too, keeping the same shallow copy
behavior while matching the common CWE-1321 mitigation pattern.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: a36d42bc-a82d-4fed-92ef-fbbb9afb056f

📥 Commits

Reviewing files that changed from the base of the PR and between 8ae2806 and d2ded02.

📒 Files selected for processing (1)

• desktop/src/main/kb-store.ts