feat: add database backup settings#158
Conversation
🤖 Augment PR SummarySummary: This PR introduces configurable SQLite database backups (path + retention) and wires the settings through the backend and UI. Changes:
Technical Notes: Backup is disabled when the backup path is empty; server-side path normalization/validation is applied before persisting settings. 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| if (StringUtils.isNotBlank(globalSetting.getBackupPath())) { | ||
| java.nio.file.Path normalized = java.nio.file.Paths.get(globalSetting.getBackupPath()).normalize(); | ||
| if (!normalized.isAbsolute()) { | ||
| throw new IllegalArgumentException("backup path must be an absolute path"); |
There was a problem hiding this comment.
GlobalSettingService.saveGlobalSetting: throwing IllegalArgumentException here will be handled by CustomExceptionHandler's generic Exception handler and returned as HTTP 500, so invalid user input becomes a server error. Consider mapping this validation failure (and potential InvalidPathException from Paths.get(...)) to a request/parameter exception so the client gets a 4xx and an actionable message.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| if (!normalized.isAbsolute()) { | ||
| throw new IllegalArgumentException("backup path must be an absolute path"); | ||
| } | ||
| if (normalized.toString().contains("..")) { |
There was a problem hiding this comment.
normalized.toString().contains("..") is a brittle traversal check (especially after normalize()) and can also reject legitimate absolute paths that contain .. in a directory name (e.g., foo..bar). This can block valid backup directories while not reliably enforcing the intended constraint.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| String backupPath = setting.getBackupPath(); | ||
| File backupDir = new File(backupPath); | ||
| if (!backupDir.exists()) { | ||
| if (!backupDir.mkdirs()) { |
There was a problem hiding this comment.
DatabaseBackupTask.autoBackupDatabase: if backupPath exists but is a regular file (not a directory), this code will proceed and the backup will likely fail later with a confusing error. Consider explicitly checking backupDir.isDirectory() and handling that case.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| File backupFile = new File(backupDir, "db_backup_" + timestamp + ".sqlite"); | ||
| try (Connection conn = dataSource.getConnection(); | ||
| PreparedStatement stmt = conn.prepareStatement("VACUUM INTO ?")) { | ||
| stmt.setString(1, backupFile.getAbsolutePath()); |
There was a problem hiding this comment.
Please double-check that the SQLite JDBC driver supports binding a placeholder in VACUUM INTO ?; some SQLite statements require a string literal and will fail at runtime with parameters. If placeholders aren't supported, backups would fail in production even though the unit test passes (it mocks the statement).
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| autoSaveSiteBlacklists: yup.string().nullable() | ||
| autoSaveSiteBlacklists: yup.string().nullable(), | ||
| backupPath: yup.string().nullable(), | ||
| backupKeepDays: yup.number().nullable().min(1, t('backupKeepDaysMin')) |
There was a problem hiding this comment.
In the Yup schema, yup.number().nullable() doesn't treat an empty string (common from <input type="number">) as null, so clearing backupKeepDays can become NaN and cause unexpected validation/submission behavior. This may conflict with the UI hint that leaving retention empty defaults to 30 days.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
1 participant
Summary
Tests