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 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
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.
| const webdav = getWebDAVSettings(settings) | ||
| await ensureWebDAVCollections(settings, deps.platform) | ||
|
|
||
| const [sessions, metas, deviceName] = await Promise.all([ |
There was a problem hiding this comment.
This upload path overwrites the remote snapshot with only the current device's local sessions/metas. If device A has already uploaded conversation A, then device B with only conversation B clicks Upload Now, the remote snapshot becomes B-only and A disappears from the sync source. Upload should first download and merge the existing remote snapshot, or the UI should make this an explicit destructive overwrite with confirmation.
| deviceName: string | ||
| exportedAt?: string | ||
| }): SyncSnapshot { | ||
| const sessions = input.sessions.filter(isChatSession) |
There was a problem hiding this comment.
The snapshot includes whole Session objects, but this PR only syncs sessions/metas and not the local blobs referenced by storageKey fields in messages, pictures, files, links, avatars, etc. Importing this snapshot on another device will create sessions with dangling local references, so image/file/link-backed history appears synced but cannot render or be reused. Either include the referenced blobs in the WebDAV sync contract or strip/mark these references during export/import.
| webdavRequest?: (request: WebDAVRequest, baseUrl: string) => Promise<WebDAVResponse> | ||
| } | ||
|
|
||
| export function buildBasicAuthHeader(username: string, password: string): string { |
There was a problem hiding this comment.
btoa() only accepts Latin-1 input. WebDAV usernames or app passwords containing non-ASCII characters will throw before the request is sent. Please encode username:password as UTF-8 bytes before base64 encoding.
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.
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.