feat(backup): add WebDAV backup and restore functionality - #108
Merged
Conversation
Add WebDAV-based remote backup support with automatic scheduling: - Backend: WebDAV client integration, backup/restore handlers, scheduled task - Frontend: Configuration UI with connection test, manual backup trigger, and restore from remote - Settings: WebDAV URL, credentials, path, interval, retention count, stats inclusion - i18n: English, Simplified Chinese, Traditional Chinese translations
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds WebDAV backup settings, client and backup/restore logic, scheduler and HTTP endpoints, plus frontend settings UI and locale text for managing and restoring backups. ChangesWebDAV Backup Feature
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
go.mod (1)
68-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
gowebdavshould be a direct dependency, not// indirect.It is imported directly in
internal/webdav/client.goandinternal/webdav/backup.go. Runninggo mod tidywill drop the// indirectmarker; leaving it can fail CI tidiness checks.Proposed fix
- github.com/studio-b12/gowebdav v0.12.0 // indirect + github.com/studio-b12/gowebdav v0.12.0(Ideally move it into the direct
requireblock.)🤖 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 `@go.mod` at line 68, The go.mod dependency entry for github.com/studio-b12/gowebdav is incorrectly marked as indirect even though it is used directly by internal/webdav/client.go and internal/webdav/backup.go. Move this module into the direct require block or run go mod tidy so the // indirect marker is removed, keeping the dependency declaration aligned with its direct imports.internal/webdav/client.go (1)
68-81: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueProxy failure silently falls back to a non-proxied client.
When a proxy URL is configured but
GetHTTPClientSystemProxy(true)returns an error, the code falls through and returns a non-proxied client. WebDAV traffic would then bypass the proxy without any signal, which can be surprising in proxy-mandated environments. Consider logging the failure (or returning the error) instead of silently degrading.🤖 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 `@internal/webdav/client.go` around lines 68 - 81, The getHTTPClient function currently hides proxy setup failures by falling back to a non-proxied client when a proxy URL is configured and client.GetHTTPClientSystemProxy(true) fails. Update getHTTPClient so the proxy path does not silently degrade: either log the error and keep the failure visible, or return the error to the caller instead of proceeding to the non-proxy branch. Use the getHTTPClient and client.GetHTTPClientSystemProxy symbols to keep the proxy behavior explicit.
🤖 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 `@internal/server/handlers/setting.go`:
- Around line 93-103: The WebDAV backup interval handler in setting.Update
currently only calls task.Update, which won’t activate webdav_backup if it was
never registered because init starts with a zero interval. Update the
SettingKeyWebDAVBackupInterval branch to ensure the task is registered when the
value changes from 0 to a positive duration, using the existing task/init and
task.Update flow around setting.Key so enabling the setting takes effect
immediately without a restart.
In `@internal/server/handlers/webdav_backup.go`:
- Around line 69-84: The restoreWebDAVBackup handler currently trusts
req.Filename after JSON binding, so a crafted name can bypass isBackupFile() and
escape the backup directory through path.Join normalization. Update
restoreWebDAVBackup and the backup validation path in
webdav.RestoreFromBackup/isBackupFile to reject any filename containing path
separators or traversal segments, and only allow a plain basename or a value
that matches an entry returned by ListBackups.
In `@internal/webdav/backup.go`:
- Around line 152-154: The restore filename validation in isBackupFile is too
weak because it only checks the backup prefix and suffix, allowing path
traversal inputs to reach RestoreFromBackup. Strengthen the filename check to
reject any name containing path separators or parent-directory segments before
path.Join is used, so RestoreFromBackup can only resolve files under
cfg.BackupPath. Use the existing isBackupFile helper and the restore flow that
passes filename into RestoreFromBackup to locate the fix.
In `@web/src/api/endpoints/setting.ts`:
- Around line 286-293: The useWebDAVBackupList query currently has no polling
interval, so backup lists stay stale until a manual refetch. Update the useQuery
call in useWebDAVBackupList to include a 30s refetchInterval, matching the
behavior of the other server-side list query hooks. Keep the change localized to
the useWebDAVBackupList helper and preserve the existing queryKey, queryFn, and
enabled flag.
In `@web/src/components/modules/setting/WebDAVBackup.tsx`:
- Around line 70-80: The successful restore path in handleRestore currently only
shows a toast, so the controlled WebDAV settings inputs stay on stale
pre-restore values and can overwrite the restored server state on the next edit.
After restoreBackup.mutateAsync(filename) succeeds, re-fetch or reset the
settings state used by WebDAVBackup (and any related form state) before clearing
restoringFile so the UI rehydrates from the restored server data.
- Line 50: The WebDAV backup list currently treats pending/error and empty
results the same by defaulting `backupList.data` to an empty array in
`WebDAVBackup`, which causes a false “no backups” state on first render and
after failures. Update the rendering logic around `backupList` and the
`noBackups` fallback so loading and error states are handled explicitly before
showing empty-state UI, and only use the empty state once the request has
settled and the list is truly empty; apply the same fix wherever the same
pattern appears.
---
Nitpick comments:
In `@go.mod`:
- Line 68: The go.mod dependency entry for github.com/studio-b12/gowebdav is
incorrectly marked as indirect even though it is used directly by
internal/webdav/client.go and internal/webdav/backup.go. Move this module into
the direct require block or run go mod tidy so the // indirect marker is
removed, keeping the dependency declaration aligned with its direct imports.
In `@internal/webdav/client.go`:
- Around line 68-81: The getHTTPClient function currently hides proxy setup
failures by falling back to a non-proxied client when a proxy URL is configured
and client.GetHTTPClientSystemProxy(true) fails. Update getHTTPClient so the
proxy path does not silently degrade: either log the error and keep the failure
visible, or return the error to the caller instead of proceeding to the
non-proxy branch. Use the getHTTPClient and client.GetHTTPClientSystemProxy
symbols to keep the proxy behavior explicit.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e9b81200-00ec-445f-9bb8-97fe04b33824
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (14)
go.modinternal/model/setting.gointernal/server/handlers/setting.gointernal/server/handlers/webdav_backup.gointernal/task/init.gointernal/task/webdav_backup.gointernal/webdav/backup.gointernal/webdav/client.goweb/public/locale/en.jsonweb/public/locale/zh_hans.jsonweb/public/locale/zh_hant.jsonweb/src/api/endpoints/setting.tsweb/src/components/modules/setting/WebDAVBackup.tsxweb/src/components/modules/setting/index.tsx
Security fixes: - Strengthen filename validation to prevent path traversal attacks - Register WebDAV backup task when enabling from zero interval UX improvements: - Add 30s auto-refresh for backup list - Refresh settings after restore completes - Fix empty state handling (distinguish loading/error/empty) Code quality: - Log proxy setup failures instead of silent fallback - Fix gowebdav indirect marker in go.mod Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Owner
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
Owner
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
internal/webdav/client.go (1)
35-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFallback defaults duplicate
model.DefaultSettings().
/octopus-backups, retention10, and the include-stats default are hard-coded here and again ininternal/model/setting.goDefaultSettings(). If one changes, they drift. Consider referencing shared constants for these defaults.🤖 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 `@internal/webdav/client.go` around lines 35 - 46, The WebDAV fallback values are duplicated from model.DefaultSettings(), so update the WebDAV client logic in client.go to reuse the shared default constants instead of hard-coding "/octopus-backups", 10, and the include-stats default. Use the existing default definitions from the settings/model package (for example the symbols used by DefaultSettings() and the SettingKeyWebDAV* lookups) so changes stay consistent in one place.
🤖 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 `@internal/webdav/backup.go`:
- Around line 43-66: The WebDAV client created by NewClient currently has no
explicit timeout, so ReadDir/Write/Read/Remove calls can hang past the worker
deadline. Update internal/webdav/client.go so the client is built with a fixed
timeout (or call SetTimeout on the gowebdav client) and ensure all requests use
that timeout behavior; then the backup flows in backup.go will stop relying on
the outer task deadline alone.
In `@internal/webdav/client.go`:
- Around line 69-84: Proxy client creation failures are being swallowed in
getHTTPClient, which lets NewClient fall back to a direct connection when
proxy_url is configured. Update getHTTPClient to return an error instead of nil
on GetHTTPClientSystemProxy failures, and adjust callers in NewClient,
TestConnection, and RunBackup to fail loudly rather than skipping SetTransport
or continuing without the proxy. Keep the proxy branch in
internal/webdav/client.go using the existing client.GetHTTPClientSystemProxy
path, but ensure any error is propagated and handled as a hard failure.
- Around line 86-95: TestConnection is treating every ReadDir failure as a
missing directory and then falling back to MkdirAll, which hides the real cause.
Update TestConnection in the WebDAV client to inspect the ReadDir error and only
call c.MkdirAll(cfg.BackupPath, 0755) when the error indicates the path does not
exist (for example, a not-found/404 condition). For auth, permission, or network
errors, return the original ReadDir error directly so the caller sees the true
failure.
In `@web/src/components/modules/setting/WebDAVBackup.tsx`:
- Around line 210-213: The WebDAVBackup loading/error labels are using missing
translation keys, so the fallback still shows the missing-key string; update
WebDAVBackup to use existing locale keys or add setting.webdavBackup.loading and
setting.webdavBackup.loadError to all locale files. Locate the affected UI in
WebDAVBackup and ensure the messages are defined consistently in en.json,
zh_hans.json, and zh_hant.json, or switch the t() calls to next-intl
default-message usage so the plain English fallback is rendered.
---
Nitpick comments:
In `@internal/webdav/client.go`:
- Around line 35-46: The WebDAV fallback values are duplicated from
model.DefaultSettings(), so update the WebDAV client logic in client.go to reuse
the shared default constants instead of hard-coding "/octopus-backups", 10, and
the include-stats default. Use the existing default definitions from the
settings/model package (for example the symbols used by DefaultSettings() and
the SettingKeyWebDAV* lookups) so changes stay consistent in one place.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1de251aa-3af9-407c-8ed4-f9c79f32e74f
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (14)
go.modinternal/model/setting.gointernal/server/handlers/setting.gointernal/server/handlers/webdav_backup.gointernal/task/init.gointernal/task/webdav_backup.gointernal/webdav/backup.gointernal/webdav/client.goweb/public/locale/en.jsonweb/public/locale/zh_hans.jsonweb/public/locale/zh_hant.jsonweb/src/api/endpoints/setting.tsweb/src/components/modules/setting/WebDAVBackup.tsxweb/src/components/modules/setting/index.tsx
…error handling - Add 30s timeout to WebDAV client to prevent hanging requests - Propagate proxy client creation failures instead of silently falling back - Distinguish 404 from auth/network errors in TestConnection - Add missing loading/loadError i18n keys to all locale files - Reload page after restore so settings rehydrate from server state Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add WebDAV-based remote backup support with automatic scheduling:
Close #87
Summary by CodeRabbit