fix(desktop): add CSRF protection to mutating requests in library, framework-api, memory-api, x-monitor - #2165
fix(desktop): add CSRF protection to mutating requests in library, framework-api, memory-api, x-monitor#2165hognek wants to merge 3 commits into
Conversation
…amework-api, memory-api, x-monitor Four lib modules were using raw fetch() on mutating HTTP methods (POST/PUT/DELETE) without the withCsrf wrapper, bypassing the double-submit CSRF check. Affected functions: - library.ts: deleteLibraryItem, reprocessLibraryItem, ingestLibraryUrl - framework-api.ts: startFrameworkUpdate, setPermittedModels - memory-api.ts: setMemoryModel - x-monitor.ts: postJson/putJson helpers, deleteWatch Fixes jaylfc#2126
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughDesktop entrypoints now install an authentication guard, and mutating requests in framework, library, memory, and X monitor clients now use ChangesDesktop authentication and request protection
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant EntryModule
participant installAuthGuard
participant window.fetch
participant LoginGate
EntryModule->>installAuthGuard: install during startup
installAuthGuard->>window.fetch: wrap API requests
window.fetch-->>installAuthGuard: 401 response from /api/*
installAuthGuard->>LoginGate: emit session-expired signal
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
|
PR Summary by Qodofix(desktop): add CSRF wrapper to mutating lib API calls
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Previous Review Summary (commit ca4d9d2)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit ca4d9d2)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
Files Reviewed (4 files)
|
|
Reviewed. The change is correct and I am not blocking it, but I found something while checking it that makes this PR more important than it looks, and it is two lines from being complete. Please fold that in here rather than opening a follow-up. First, what I checked and got wrong so nobody repeats it. My initial suspicion was that Second, the thing that actually matters. My next thought was that these call sites are redundant, because One entry point. There are three:
Requested change, in this PR: add Please also add a test that would have caught this, since the current suite passes with the guard installed in only one of three entry points. A test asserting each entry module installs the guard is enough. Without it the same gap reappears the next time an entry point is added, and there is now a fourth on the way. Worth stating plainly: the per-call-site Doc sweep and CHANGELOG in the same PR per the commit gate. |
chat-main.tsx and app-standalone-main.tsx were missing installAuthGuard(), leaving the chat PWA and standalone app PWA without CSRF protection on window.fetch calls. Only main.tsx (desktop shell PWA) had the guard. Add the import + call to both entry modules, matching the pattern in main.tsx. installAuthGuard is idempotent (module-level flag), so double install is safe if an entry point is loaded more than once in the same realm. Add entry-guards.test.ts asserting all three entry modules install the auth guard on import.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@desktop/src/lib/x-monitor.ts`:
- Around line 67-79: Update fetchJson to preserve Headers instances supplied by
withCsrf instead of spreading them into a plain object, retaining X-CSRF-Token
and Content-Type. Ensure the request headers remain valid for the postJson and
putJson mutation wrappers while keeping existing header-merging behavior for
other inputs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 342732d7-19fb-45d7-8ca1-6872a667b491
⛔ Files ignored due to path filters (1)
desktop/package-lock.jsonis excluded by!**/package-lock.json,!**/package-lock.json
📒 Files selected for processing (7)
desktop/src/__tests__/entry-guards.test.tsdesktop/src/app-standalone-main.tsxdesktop/src/chat-main.tsxdesktop/src/lib/framework-api.tsdesktop/src/lib/library.tsdesktop/src/lib/memory-api.tsdesktop/src/lib/x-monitor.ts
|
This went CONFLICT because #2174 merged and touched Worth knowing what landed, because it changes what this PR is for. #2174 installed That is not a reason to drop it. Keep the explicit wrapping: it is harmless ( #2174 also added a coverage test that enumerates entry points from |
|
Correcting myself: the Headers-spread bug IS real, and it is in this PR's blast radius. Earlier I said my suspicion was wrong. I had checked
const res = await fetch(url, { ...init, headers: { Accept: "application/json", ...init?.headers } });
So the moment an x-monitor call is routed through Fix: normalise instead of spreading, so it works for both shapes. const headers = new Headers({ Accept: "application/json" });
new Headers(init?.headers).forEach((v, k) => headers.set(k, v));
const res = await fetch(url, { ...init, headers });
Please add a test that would have caught it: call This PR is also DIRTY now (#2174 merged and touched auth-guard), so you need a rebase regardless. Fold this into the same pass. Credit where due: Qodo flagged this and I dismissed it after checking the wrong file. Its finding stands and mine did not. |
…regression test withCsrf() returns a Headers instance at csrf.ts:33. Spreading that instance into an object literal (the old fetchJson line 56 pattern) silently drops every entry because Headers entries are not own- enumerable properties. The CSRF token the PR was written to add was therefore never reaching the wire. Fix by building a fresh Headers with the Accept default and copying caller headers via forEach — the iterator visits all entries correctly. Add a regression test that exercises createWatch → postJson → fetchJson → withCsrf with a stubbed csrf_token cookie, asserting the token survives into the final fetch call.
|
The Headers fix is correct and I verified it rather than eyeballing it. Ran the old and new logic side by side against exactly what That last line matters as much as the first: Only the rebase is left. One thing to watch on this rebase specifically. #2174 installed And run the check that saved #2177: Rebase and I will merge it. Thanks for turning that one around quickly, and credit to Qodo for the original catch that I initially dismissed. |
2 participants
Summary
Adds CSRF protection via a globally-installed auth guard to all three desktop entry points.
Previous PR state: installAuthGuard() was only in main.tsx (desktop shell PWA).
chat-main.tsx (chat PWA) and app-standalone-main.tsx (standalone app PWA) had no guard,
so mutating API calls made from those PWAs would fail CSRF validation.
jaylfc review follow-up (2026-07-27)
Testing
Closes #2126
Summary by CodeRabbit
New Features
Bug Fixes
Tests