feat: allow HTTP and HTTPS POST as Backup location#4061
Conversation
Add support for pushing export backups to HTTP/HTTPS endpoints and avoid treating remote URLs as local files. Changes include: - BackupController: detect HTTP/HTTPS backup paths via a new #isHttpUrl helper. - Create backup: if rule.backupPath is an HTTP(S) URL, reject raw DB backups and call new #pushExportBackup to POST export data to the URL instead of writing a local file. - #pushExportBackup: generate and stringify export data, determine content type, POST to the target URL, and return a PreviousBackupInfo entry for history trimming. - Cleanup and deletion logic: when trimming old backups, skip unlinking entries whose filePath is an HTTP(S) URL and only remove the history entry; similarly avoid unlinking remote paths when deleting individual backups. - UI: update BackupRuleEditor text to clarify that raw DB backups cannot be pushed to HTTP(S) URLs and that export backups may be posted to http:// or https:// endpoints. These changes enable remote push-style backups for export formats while preserving local file behavior and preventing accidental attempts to write raw DB files to remote URLs.
📝 WalkthroughWalkthroughThis PR extends backup functionality to support HTTP/HTTPS destinations. The backup controller now detects URL-like paths, routes export backups to HTTP endpoints via POST requests, and adjusts cleanup/deletion logic to skip filesystem operations for remote URLs. UI text clarifies the new capability and its limitations. Changes
Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 1
🧹 Nitpick comments (1)
companion/lib/ImportExport/Backups.ts (1)
451-456: Optional: Consider authentication header support for future enhancement.Many backup endpoints require authentication (API keys, bearer tokens, etc.). This might be a nice future enhancement to add a configurable headers field, but it's understandable if that's out of scope for this PR!
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 39ae2dfe-ae8d-4e77-b96b-c4d99e962e62
📒 Files selected for processing (2)
companion/lib/ImportExport/Backups.tswebui/src/UserConfig/BackupRuleEditor.tsx
There was a problem hiding this comment.
Pull request overview
This PR adds support for “push” backups by allowing export-style backups to be POSTed to HTTP/HTTPS endpoints, while preventing raw database backups from being sent to remote URLs and updating the UI copy to clarify the behavior.
Changes:
- Add HTTP/HTTPS URL detection and a new push-backup code path that POSTs export backups to a remote server.
- Prevent
db(raw) backups from using HTTP/HTTPS destinations and adjust cleanup/deletion logic to avoid unlinking remote backups. - Update the backup rule editor help text/warning to explain URL support and restrictions.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
companion/lib/ImportExport/Backups.ts |
Implements HTTP(S) push backups, URL detection, and adjusts cleanup/delete behavior for remote destinations. |
webui/src/UserConfig/BackupRuleEditor.tsx |
Updates UI messaging to clarify that export backups can be POSTed to URLs, but raw DB backups cannot. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Delete old local backup files. For HTTP push backups, only trim history. | ||
| const deletionPromises = backupsToDelete.map(async (backup) => { | ||
| if (this.#isHttpUrl(backup.filePath)) { | ||
| this.#logger.info(`Removed old push backup entry: ${backup.filePath}`) | ||
| return backup | ||
| } |
There was a problem hiding this comment.
For HTTP push backups this returns the same backup.filePath (the URL) for every backup. Downstream cleanup removes history entries by matching on filePath, so if multiple backups share the same URL it can end up removing all entries (including those that should be kept). Consider storing a unique identifier per push (eg include createdAt and/or backupName in the stored path/id) and/or change cleanup comparison to include createdAt as well as filePath.
| // Delete old local backup files. For HTTP push backups, only trim history. | ||
| const deletionPromises = backupsToDelete.map(async (backup) => { | ||
| if (this.#isHttpUrl(backup.filePath)) { | ||
| this.#logger.info(`Removed old push backup entry: ${backup.filePath}`) |
There was a problem hiding this comment.
Logging the full push-backup URL can leak credentials or tokens if the URL contains userinfo/query parameters. Please redact sensitive URL components (eg strip username/password and querystring) before logging.
| this.#logger.info(`Removed old push backup entry: ${backup.filePath}`) | |
| let redactedUrl = backup.filePath | |
| try { | |
| const parsedUrl = new URL(backup.filePath) | |
| parsedUrl.username = '' | |
| parsedUrl.password = '' | |
| parsedUrl.search = '' | |
| redactedUrl = parsedUrl.toString() | |
| } catch { | |
| // If parsing fails, fall back to logging the original value | |
| } | |
| this.#logger.info(`Removed old push backup entry: ${redactedUrl}`) |
| const backupPath = rule.backupPath ? rule.backupPath : this.#defaultBackupDir | ||
| if (this.#isHttpUrl(backupPath)) { | ||
| if (rule.backupType === 'db') { | ||
| throw new Error('Raw database backups must be saved to a local file path, not HTTP/HTTPS URLs') | ||
| } |
There was a problem hiding this comment.
backupPath comes from user input and isn't trimmed before URL detection. A value like ' https://host/path' will fail new URL() parsing and then be treated as a local directory, potentially creating unexpected folders like " https:". Trim whitespace (and ideally validate) before #isHttpUrl and before using it in fs.mkdir/path operations.
| logger.info(`Pushing backup to ${backupUrl}`) | ||
|
|
There was a problem hiding this comment.
This log line includes the full backupUrl, which may contain credentials or tokens. Consider logging a redacted form (eg protocol+host+pathname only) to avoid secrets ending up in logs.
| logger.info(`Pushing backup to ${backupUrl}`) | |
| let redactedUrl = backupUrl | |
| try { | |
| const parsed = new URL(backupUrl) | |
| redactedUrl = `${parsed.protocol}//${parsed.host}${parsed.pathname}` | |
| } catch { | |
| // If parsing fails, fall back to the original string | |
| } | |
| logger.info(`Pushing backup to ${redactedUrl}`) |
| return { | ||
| filePath: backupUrl, | ||
| fileSize: typeof exportData.data === 'string' ? Buffer.byteLength(exportData.data) : exportData.data.length, | ||
| createdAt: Date.now(), |
There was a problem hiding this comment.
Returning filePath: backupUrl for push backups makes multiple history entries indistinguishable (same filePath each run). That breaks keep-count trimming and makes UI deletion ambiguous because deletion is keyed by filePath. Consider storing a unique per-backup identifier (eg ${backupUrl}#${createdAt} or include the generated filename) and/or extending the delete API to target by both filePath and createdAt.
| return { | |
| filePath: backupUrl, | |
| fileSize: typeof exportData.data === 'string' ? Buffer.byteLength(exportData.data) : exportData.data.length, | |
| createdAt: Date.now(), | |
| const createdAt = Date.now() | |
| return { | |
| filePath: `${backupUrl}#${createdAt}`, | |
| fileSize: typeof exportData.data === 'string' ? Buffer.byteLength(exportData.data) : exportData.data.length, | |
| createdAt, |
| if (!this.#isHttpUrl(filePath)) { | ||
| // Delete the file if it exists | ||
| try { | ||
| await fs.unlink(filePath) | ||
| this.#logger.info(`Deleted backup file: ${filePath}`) | ||
| } catch (err) { | ||
| this.#logger.warn(`Failed to delete backup file ${filePath}, but will remove from rule: ${err}`) | ||
| } | ||
| } |
There was a problem hiding this comment.
Skipping filesystem deletion for HTTP paths is fine, but the backend removes a history entry by looking up previousBackups via filePath. If multiple push backups share the same URL, the UI can't delete a specific entry (it will remove the first match). Consider updating the delete input to include createdAt (or an explicit id) so push-backup history entries are uniquely addressable.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
|
SNRSE
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (1)
companion/lib/ImportExport/Backups.ts (1)
538-546: Looks good—just a tiny logging suggestionThe conditional skip for HTTP URLs is exactly right. One small thought: for local files, you log either success or a warning. For HTTP URL entries, there's no log indicating the entry was removed. If you'd like consistency in your logs (especially for debugging/auditing backup history), you could add a brief info log here. Totally optional though!
💡 Optional: Add info log for HTTP entry removal
if (!this.#isHttpUrl(filePath)) { // Delete the file if it exists try { await fs.unlink(filePath) this.#logger.info(`Deleted backup file: ${filePath}`) } catch (err) { this.#logger.warn(`Failed to delete backup file ${filePath}, but will remove from rule: ${err}`) } + } else { + this.#logger.info(`Removing HTTP backup entry from history: ${filePath}`) }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: eb670721-81d6-49fa-83a1-5ebeb7519d9b
📒 Files selected for processing (1)
companion/lib/ImportExport/Backups.ts
|
Thanks for the PR! I think it would be good to in the ui have a 'storage type' dropdown to pick between 'local' and 'http/https', to make it more explicit. It would be good to have an alert (like the one that shows when raw database is selected) when set to http mode and raw database is selected, to make it very visual that something is setup wrong |
Thanks for your Feedback |
This pull request enhances the backup system by adding support for HTTP/HTTPS push backups, allowing export-type backups to be POSTed directly to a remote server. It also enforces restrictions to prevent raw database backups from being sent to HTTP URLs, and updates the UI to clarify these behaviors.
Backup system improvements:
export-json,export-gz,export-yaml) to HTTP/HTTPS endpoints by POSTing the backup data, including a new#pushExportBackupmethod and a utility to detect HTTP URLs (#isHttpUrl). [1] [2]dbtype) must be saved to a local file path, not an HTTP/HTTPS URL, with error handling for invalid configurations.User interface updates:
Codebase cleanup:
Summary by CodeRabbit
New Features
Changes
Documentation