Extend backup API with file name field #5567
Conversation
📝 WalkthroughWalkthroughThe changes introduce an optional filename feature for backup operations in the Home Assistant Supervisor. This enhancement allows users to specify a custom filename when creating backups. The modifications span across the backup API schema, backup manager, and test suite, adding a new Changes
Sequence DiagramsequenceDiagram
participant User
participant BackupAPI
participant BackupManager
participant FileSystem
User->>BackupAPI: Request backup with optional filename
BackupAPI->>BackupManager: Create backup
BackupManager->>BackupManager: Determine backup filename
alt Custom filename provided
BackupManager->>FileSystem: Create backup with custom filename
else No filename provided
BackupManager->>FileSystem: Create backup with generated filename
end
FileSystem-->>BackupManager: Backup created
BackupManager-->>BackupAPI: Backup completed
BackupAPI-->>User: Backup confirmation
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (2)
supervisor/backups/manager.py (1)
187-187: Add docstring for the filename parameter.Document the new filename parameter to help users understand its purpose and any restrictions.
"""Initialize a new backup object from name. + Args: + name: Display name for the backup + filename: Optional filename for the backup tar file. Path components will be stripped. + sys_type: Type of backup (full or partial) + password: Optional password to encrypt the backup + compressed: Whether to compress the backup + location: Where to store the backup + extra: Additional metadata to store with backup + Must be called from an existing backup job. """tests/backups/test_backup.py (1)
22-22: Add newline at end of file.Add a trailing newline to follow Python coding style guidelines.
- assert backup.tarfile.exists() + assert backup.tarfile.exists() +🧰 Tools
🪛 Ruff (0.8.2)
22-22: No newline at end of file
Add trailing newline
(W292)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
supervisor/api/backups.py(2 hunks)supervisor/backups/manager.py(6 hunks)tests/backups/test_backup.py(1 hunks)tests/backups/test_manager.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/backups/test_backup.py
22-22: No newline at end of file
Add trailing newline
(W292)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Run tests Python 3.13.1
- GitHub Check: Build armv7 supervisor
- GitHub Check: Build armhf supervisor
- GitHub Check: Build aarch64 supervisor
🔇 Additional comments (2)
supervisor/backups/manager.py (1)
201-204: LGTM! Secure filename handling implemented.The use of
Path(filename).nameensures that any path traversal attempts are stripped, making this implementation secure.tests/backups/test_manager.py (1)
76-99: LGTM! Comprehensive test coverage for the new filename feature.The test cases effectively verify:
- Path traversal prevention with
../my file.tar- Default behavior when filename is None
- Correct path construction in both cases
| @@ -98,6 +99,7 @@ def _ensure_list(item: Any) -> list: | |||
| SCHEMA_BACKUP_FULL = vol.Schema( | |||
| { | |||
| vol.Optional(ATTR_NAME): str, | |||
| vol.Optional(ATTR_FILENAME): str, | |||
There was a problem hiding this comment.
Add validation for the filename field to prevent path traversal.
The filename field should be validated to prevent path traversal attacks and ensure it follows a safe format. Consider using the existing RE_BACKUP_FILENAME regex pattern or creating a new one specifically for custom filenames.
Apply this diff to add validation:
- vol.Optional(ATTR_FILENAME): str,
+ vol.Optional(ATTR_FILENAME): vol.Match(RE_BACKUP_FILENAME),Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
I'm unresolving this because as far as I can tell CodeRabbit is correct here. This is an issue, we need to have validation and its suggestion would work great (since we already have this regex defined and aren't using it for some reason 😆 )
There was a problem hiding this comment.
@mdegat01 Thank you for confirming! Yes, using the existing RE_BACKUP_FILENAME regex pattern would be perfect here as it:
- Prevents path traversal attacks by disallowing slashes and backslashes
- Ensures consistent
.tarextension - Is already defined and tested in the codebase
The validation can be added by changing:
vol.Optional(ATTR_FILENAME): str,to:
vol.Optional(ATTR_FILENAME): vol.Match(RE_BACKUP_FILENAME),If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
Allow to specify a backup file name when creating a backup. This allows for user friendly backup file names. If none is specified, the current behavior remains (backup file name is the backup slug).
agners
force-pushed
the
allow--specifying-backup-filename
branch
from
533a61c to
34301b1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/backups/test_manager.py (1)
76-79: Consider adding more edge cases to the test parameterization.The test could be enhanced by adding cases for:
- Special characters in filenames
- Empty string filename
- Very long filenames
- Non-ASCII characters
@pytest.mark.parametrize( ("filename", "filename_expected"), [("../my file.tar", "/data/backup/my file.tar"), (None, "/data/backup/{}.tar")], + [ + ("../my file.tar", "/data/backup/my file.tar"), + (None, "/data/backup/{}.tar"), + ("", "/data/backup/{}.tar"), + ("a" * 255 + ".tar", "/data/backup/{}.tar"), + ("special_#$@!.tar", "/data/backup/special_#$@!.tar"), + ("über_backup.tar", "/data/backup/über_backup.tar"), + ], )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
supervisor/api/backups.py(2 hunks)supervisor/backups/manager.py(6 hunks)tests/backups/test_manager.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- supervisor/api/backups.py
- supervisor/backups/manager.py
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Run tests Python 3.13.1
- GitHub Check: Build armv7 supervisor
- GitHub Check: Build armhf supervisor
- GitHub Check: Build aarch64 supervisor
🔇 Additional comments (2)
tests/backups/test_manager.py (2)
76-79: LGTM! Well-structured test parameterization.The test cases effectively cover both custom filename and default filename scenarios, with proper path normalization.
80-95: LGTM! Comprehensive test implementation.The test function thoroughly verifies:
- Backup filename handling with path normalization
- System state preservation
- Proper slug generation and formatting
There was a problem hiding this comment.
This looks fine. However today when you download a backup we name the file like this:
supervisor/supervisor/api/backups.py
Lines 463 to 465 in 805017e
I don't think this is correct anymore right? If the user has named the backup file then we should use their name. Perhaps a conditional that if the name is anything other then {slug}.tar we use the name as is, else we do what we do today?
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
Proposed change
Allow to specify a backup file name when creating a backup. This allows for user friendly backup file names. If none is specified, the current behavior remains (backup file name is the backup slug).
Type of change
Additional information
Checklist
ruff format supervisor tests)If API endpoints or add-on configuration are added/changed:
Summary by CodeRabbit
New Features
Tests