Harden WebDAV chat sync - #3769
Conversation
Constrain WebDAV transport to HTTPS-only fixed sync endpoints, validate decrypted snapshots, and keep sync scope limited to chat sessions while preserving local conflict safety. Constraint: Desktop IPC must not become a generic HTTP proxy and mobile/desktop sync behavior must stay consistent. Rejected: Allowing renderer-provided WebDAV targets through platform transports | preserves SSRF and redirect bypass risk. Confidence: high Scope-risk: moderate Directive: Keep future sync expansion out of settings, credentials, license, OAuth, and provider-token storage unless a new reviewed snapshot contract is added. Tested: git diff --check; vitest run src/shared/sync-settings.test.ts src/renderer/packages/sync/*.test.ts; biome lint src/shared/sync-settings.test.ts src/shared/sync-webdav.ts src/renderer/packages/sync; tsc --noEmit Not-tested: Full app packaging and live third-party WebDAV interoperability matrix
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
|
Solid implementation! Using Electron's net.fetch for WebDAV requests bypasses CORS issues. The crypto test for encrypt/decrypt envelope is a good addition. Hardening chat sync is important for reliability. |
themez
left a comment
themez
left a comment
There was a problem hiding this comment.
Thanks for the hardening work here. The transport boundary is much safer than a generic renderer-driven fetch, but I found two data-integrity issues that should be fixed before merge, plus one credential compatibility issue.
themez
left a comment
themez
left a comment
There was a problem hiding this comment.
One process item before this can move forward: the repository PR template includes a Contributor Agreement section, but this PR body does not include the checkbox confirmation.
Please update the PR description to include and check the template confirmation:
[x] I have read and agree with the above statement.
This confirms the submitted code is authorized for both the GPLv3 community edition and the proprietary official edition under the terms in .github/PULL_REQUEST_TEMPLATE.md.
themez
left a comment
themez
left a comment
There was a problem hiding this comment.
Re-reviewed the latest head (700ef0a). The earlier upload-merge, local-blob-reference, UTF-8 Basic Auth, and Contributor Agreement issues have been addressed. I found two data-loss risks and one conflict-idempotency issue below, so I am keeping this at changes requested.
… conflict IDs - Restore overwritten sessions on failure instead of deleting everything (P1) - Route existing-session updates through chatStore's UpdateQueue and React Query cache; new sessions use atomic createSessionWithId (P2) - Use deterministic UUIDv5 for conflict copies to prevent duplicate imports across repeated downloads (P3) - Track session changes as create/update-metadata kinds for precise rollback in downloadAndMergeWebDAVSnapshot
…ents duplicate explosion - SyncSnapshot gains updatedAt (epoch ms); older snapshots fall back to exportedAt - isRemoteSnapshotStale() compares remote.updatedAt <= lastSyncedAt; when true, both upload and download skip merge to prevent re-creating (Synced copy) duplicates - upload writes precise updatedAt on every PUT; download returns remoteStale flag - WebDAVSyncDeps adds optional getLastSyncedAt; local.ts wires it from settings fix(export): unify AdvancedSettingTab export to redact provider secrets instead of delete - Remove redundant delete providers branch; sanitizeSettingsForExport already strips apiKey/accessKey/secretKey/sessionToken per-provider when !includeSecrets - Without this fix, unchecking 'API KEY & License' would drop all provider configs (model, baseURL, etc.) from export
themez
left a comment
themez
left a comment
There was a problem hiding this comment.
Re-reviewed the latest head (7830ed7). The previous rollback and serialized-session-write issues are addressed, and stable conflict IDs deduplicate an unchanged replay. However, the new timestamp-based stale gate, mutable synced-copy handling, and non-secret export behavior introduce three blocking regressions. There is also unrelated tooling/file-mode noise that should be removed.
- Remove .workbuddy/ entry from .gitignore (local preference, not upstream material) - Restore scripts/ralph/ralph.sh executable bit (0755) in git index
…API keys from settings export - sanitizeProviderForExport now also deletes the entire oauth object (accessToken/refreshToken/extra) alongside apiKey/accessKey/secretKey - Top-level sanitize deletes memorizedManualLicenseKey and lastSelectedLicenseByUser (email-to-license-key map) - Deep-clone extension.webSearch then delete tavilyApiKey, bochaApiKey, queritApiKey - Tests expanded from 2 to 5: nested OAuth, remembered license key, web search keys — both !includeSecrets and includeSecrets directions
…dup; preserve edited sync copies P1-1 (ETag dedup instead of clock comparison): - Remove SyncSnapshot.updatedAt, snapshotUpdatedAt(), and the entire isRemoteSnapshotStale function that compared device clocks - New isRemoteSnapshotAlreadySeen: exact byte-identity check scoped to the WebDAV endpoint (url+username) + ETag. No ETag / no record / endpoint mismatch → always merge (safe default) - New rememberRemoteSnapshot: records PUT response ETag after upload, GET ETag after download; stored as sync.lastSeenEndpoint+lastSeenETag - Result flag renamed remoteStale → remoteUnchanged; UI message reworded - Tests rewritten for ETag semantics, plus clock-skew regression test and endpoint-scoping regression test P1-3 (don't throw on edited synced copy): - existingCopy with different content → continue instead of throw. The stable conflict ID uniquely identifies the import event; content mismatch can only mean the user edited the copy - Rewrote the throw assertion test, added reviewer-reproduction regression test (import copy → append message → replay → succeed, copy unchanged)
…nc rollback AggregateError - sanitizeSettingsForExport now defensively handles settings.sync / settings.extension being absent (export reads raw storage via cast; old settings persisted before these fields existed may lack them until the next settings save re-normalizes) - Added regression test: legacy settings without sync/extension do not crash; non-secret redaction still applies to provider fields - Added AggregateError test for download rollback: when the second session create fails and the rollback delete also fails, the download returns AggregateError containing both errors (chatboxai#3 checklist gap)
themez
left a comment
themez
left a comment
There was a problem hiding this comment.
Re-reviewed the latest head (159bf842). The previous clock-ordering, mutable synced-copy, requested credential-redaction, and unrelated-file findings are addressed. I found two ETag-state paths that can still drop remote-only conversations, plus remaining credential fields in the non-secret settings export, so this remains changes requested.
…crets - Unconditionally strip lastSyncedAt/lastSeenEndpoint/lastSeenETag from sync exports so a restored device does not skip its first download merge. - Strip mineru.apiToken, MCP transport env (stdio) and headers (http) when includeSecrets is false. - Deep-copy documentParser/mineru and mcp.servers/transport before redaction to avoid polluting the original settings object.
…napshot A merged upload writes remote-only sessions into the uploaded snapshot but never persists them locally. Recording the snapshot ETag as "last seen" would make every subsequent download report remoteUnchanged, and those remote-only sessions would never land on this device. Guard rememberRemoteSnapshot with `if (!mergeResult)` so the next download re-merges idempotently and persists sessions that were missing locally.
…te envelope params - service.ts: reject weak (W/) and malformed ETags in requireStrongWebDAVSnapshotETag before attempting conditional PUT. - snapshot.ts: replace localeCompare with code-unit sort so stable conflict UUIDs are deterministic across devices with different locales. - crypto.ts: validate envelope fields (iterations, salt, IV, ciphertext length, canonical Base64) before any key derivation, preventing KDF parameter injection attacks. - Add regression tests for all three fixes.
themez
left a comment
themez
left a comment
There was a problem hiding this comment.
Reviewed the current head (ade18596) specifically for maintainability and UI behavior. The latest export/ETag/crypto commits are included in this pass and the previously resolved data-integrity findings remain fixed. However, a real isolated Electron smoke test found that opening General Settings now crashes in WebDAVSyncSection; the remaining comments cover misleading validation/manual-sync semantics and state ownership that should be clarified before shipping.
|
|
||
| const WebDAVSyncSection = () => { | ||
| const { t } = useTranslation() | ||
| const { setSettings, sync } = useSettingsStore((state) => ({ |
There was a problem hiding this comment.
[P1] This selector returns a fresh object on every useStore snapshot and no shallow equality function is supplied. In an isolated Electron run, navigating to #/settings/general consistently throws Maximum update depth exceeded with WebDAVSyncSection at the top of the stack, so the whole Settings page falls into the error boundary. Splitting this into scalar selectors (state.setSettings and state.sync) makes the page render normally. Please fix the selector and add at least a component smoke test for rendering General Settings so service-only tests cannot miss this regression.
| disabled={Boolean(runningAction)} | ||
| onClick={() => | ||
| runSyncAction('test', async () => { | ||
| await testWebDAVConnection(currentSettings(), { platform }) |
There was a problem hiding this comment.
[P2] The form presents the sync encryption password as part of the connection configuration, but this action only performs MKCOL/PROPFIND and never downloads or decrypts an existing snapshot. A correct WebDAV password plus a wrong encryption password therefore produces Connection successful, and the user discovers the invalid configuration only on Upload/Download. When a snapshot exists, validate it with GET + decrypt; otherwise rename this to Test WebDAV credentials and clearly state that the encryption password has not been verified.
| </Stack> | ||
|
|
||
| <Switch | ||
| label={t('Enable WebDAV sync')} |
There was a problem hiding this comment.
[P2] sync.enabled has no sync lifecycle behind it: outside this section it is never read, and here it only disables the two manual action buttons. The label Enable WebDAV sync implies background/automatic synchronization, which the implementation does not provide. Either wire the flag into an actual sync trigger/lifecycle, or remove the switch and present this explicitly as manual WebDAV backup/merge actions.
| loading={runningAction === 'upload'} | ||
| disabled={Boolean(runningAction) || !sync.enabled} | ||
| onClick={() => | ||
| runSyncAction('upload', async () => { |
There was a problem hiding this comment.
[P2] Upload Now can remove conversations from the remote snapshot: once an ETag is already seen, the upload intentionally writes the current local set without re-adding locally deleted sessions. The UI presents this with the same light styling as connection testing and gives no preview or warning, even though the single remote file has no version/undo in this feature. Please confirm the replace/update effect before uploading (ideally show local/remote counts), or at minimum explain that local deletions can be propagated to WebDAV.
| password: '', | ||
| syncPassword: '', | ||
| }), | ||
| lastSyncedAt: z.string().optional().catch(undefined), |
There was a problem hiding this comment.
[P2] These fields are device-local runtime state, not user configuration, but placing them in Settings couples the sync engine, settings UI, persistence, and backup sanitizer. That coupling already caused restored exports to inherit another device's ETag and required special-case redaction. Please separate transferable WebDAVSyncConfig (endpoint/credentials) from a dedicated local WebDAVSyncState (lastSeen/lastSyncedAt), preferably keyed by endpoint/account; this removes the export footgun and makes endpoint switching and future sync providers easier to maintain.
…AV transport - snapshot.ts: attach syncConflictSourceId to conflict copies so they can be recognized across devices; skip re-importing a copy whose source session already exists locally. - snapshot.ts: add sessionHasActiveGeneration() check to prevent uploading sessions with in-progress streaming responses. - snapshot.ts: strip runtime-only message state (cancel, generating, status, isStreamingMode) from synced snapshots. - service.ts: reject upload while any session is still generating, with a clear error message. - session.ts: add optional syncConflictSourceId field to SessionSchema. - mobile_platform.ts: use new @chatbox/capacitor-webdav-http Capacitor plugin for Android WebDAV transport. - Workspace: add packages/* for monorepo; add new capacitor plugin package. - Regression tests for all changes (provenance, generation detection, legacy copy recognition).
…l .gitignore dist rule)
themez
left a comment
themez
left a comment
There was a problem hiding this comment.
Re-reviewed the latest WebDAV sync commits. Two additional issues remain in conflict-copy handling.
| return remoteSession.syncConflictSourceId | ||
| } | ||
| const fingerprint = sessionContentFingerprint(remoteSession) | ||
| return [...localSessionById.keys()].find( |
There was a problem hiding this comment.
[P1] Avoid the quadratic legacy-copy scan for ordinary sessions. Every missing remote session without syncConflictSourceId reaches this full scan, including normal UUIDv4 sessions, and mergeRemoteSnapshot runs synchronously on the renderer thread. With disjoint one-message histories, I measured about 1.0s for 500+500 sessions, 4.0s for 1000+1000, and 16.3s for 2000+2000 on Node 22/macOS; mobile will be worse. Please gate legacy inference to IDs that can actually be old conflict copies (these generated IDs are UUIDv5), or use another bounded migration path instead of computing the local/remote Cartesian product.
| ...remoteSessionWithMeta, | ||
| id: conflictId, | ||
| name: copiedName, | ||
| syncConflictSourceId: remoteSession.syncConflictSourceId ?? remoteSession.id, |
There was a problem hiding this comment.
[P2] Keep provenance tied to the immediate source copy. conflictId above is derived from remoteSession.id, but this assignment inherits the root syncConflictSourceId. If the same synced copy is later edited differently on two devices, the nested conflict points to the original root; on the next round the receiving device compares it with that root instead of its identical immediate copy and imports another duplicate. I reproduced this as an unexpected create change. Set the new copy's source to remoteSession.id and add a nested-conflict round-trip regression test.
…updates, credential sanitization - settings-export: strip customProviders credentials; drop all custom MCP servers on secret-free export (transport args/URL/path may embed secrets) - sync/local: add restoreSyncSessionMetadata with conditional field-by-field rollback; updateSyncSessionMetadata returns scoped undo records; track sortOrder/createdAt ordering separately - sync/service: add previewWebDAVUpload to warn before replacing remote sessions; testWebDAVConnection now verifies encryption password - sync/snapshot: track comparableLocalSessionById to avoid false conflicts when outgoing copy differs from local (e.g. differing sortOrder) - general.tsx: remove sync enabled toggle (manual-only); add upload preview modal (AdaptiveModal) with conflict count; flush StoreStorage before sync actions so main/renderer processes share the same WebDAV URL - SQLiteSessionMetaStorage.update: selective column SET instead of full-row replace — only touches fields explicitly provided in the patch - SessionMetaStorage (IndexedDB).update: read-then-write in a single transaction instead of blind put - StoreStorage: add flushItem for synchronous persistence - defaults / settings schema: remove sync.enabled (manual sync only)
fb392d7 to
e9f73c4
Compare
…lict detection, partial-import retention Make WebDAV sync import resilient to crashes and race conditions: - Remove unused `saveSyncMetas`/`deleteSyncSession` exports and `saveMetas`/`deleteSession` dependency slots - `createSessionWithId` becomes idempotent: retry after a crash repairs orphaned data instead of rejecting; uses `normalizePersistedSession`/ `normalizeImportedMeta` to tolerate JSON/SQLite serialization differences when detecting true conflicts - Concurrent fixed-ID imports serialize: the second caller waits for the first, then reconciles against the now-present state - Failed imports keep created sessions instead of deleting them; retry reuses them directly Co-authored-by: Reasonix <reasonix@mocia.ai>
3 participants
Summary
If-None-Match: *, overwrite withIf-MatchETags, retry after 412 conflicts, and refuse unsafe overwrites when an existing remote snapshot has no ETag.Test Plan
Notes
Contributor Agreement
I agree to contribute all code submitted in this PR to the open-source community edition licensed under GPLv3 and the proprietary official edition without compensation.
I grant the official edition development team the rights to freely use, modify, and distribute this code, including for commercial purposes.
I confirm that this code is my original work, or I have obtained the appropriate authorization from the copyright holder to submit this code under these terms.
I understand that the submitted code will be publicly released under the GPLv3 license, and may also be used in the proprietary official edition.