Restrict snaphsot recovery to snapshots directory, hide file contents#8341
Conversation
There was a problem hiding this comment.
Pull request overview
This PR restricts snapshot recovery from file:// URLs by validating that the referenced file path is within the configured snapshots directory. This prevents potential path traversal attacks where a privileged user could trick the system into reading arbitrary files on the filesystem via the snapshot recovery API.
Changes:
- Added
validate_snapshot_path()helper indownload.rsthat canonicalizes both the target path and the allowed directory and uses component-wisestarts_withto enforce containment. - Updated the
download_snapshot()signature to accept asnapshots_pathparameter used forfile://URL validation. - Updated both callers (
recover.rsandsnapshots.rs) to pass the appropriate snapshots directory path.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
lib/storage/src/content_manager/snapshots/download.rs |
Core change: adds validate_snapshot_path and updates download_snapshot signature to accept snapshots_path for file:// URL validation |
lib/storage/src/content_manager/snapshots/recover.rs |
Passes toc.snapshots_path() (root snapshot dir) as the snapshots_path argument to download_snapshot |
src/common/snapshots.rs |
Passes collection.snapshots_path() (collection-scoped snapshot dir) as the snapshots_path argument to download_snapshot |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn validate_snapshot_path(path: &Path, allowed_dir: &Path) -> Result<PathBuf, StorageError> { | ||
| let canonical_path = path.canonicalize().map_err(|err| { | ||
| StorageError::bad_request(format!("Failed to resolve snapshot path {path:?}: {err}")) | ||
| })?; | ||
|
|
||
| let canonical_allowed = allowed_dir.canonicalize().map_err(|err| { | ||
| StorageError::service_error(format!( | ||
| "Failed to resolve snapshots directory {allowed_dir:?}: {err}" | ||
| )) | ||
| })?; | ||
|
|
||
| if !canonical_path.starts_with(&canonical_allowed) { | ||
| return Err(StorageError::bad_request(format!( | ||
| "Snapshot file path must be inside the snapshots directory {canonical_allowed:?}", | ||
| ))); | ||
| } | ||
|
|
||
| Ok(canonical_path) | ||
| } |
There was a problem hiding this comment.
The new validate_snapshot_path function implements a security-critical path traversal prevention mechanism. The adjacent download_tar.rs file has unit tests for its public function, but download.rs has none. Given that this function is the core of the security feature being introduced, it should have unit tests covering at minimum: (1) a path inside the allowed directory (should succeed), (2) a path outside the allowed directory (should fail with bad_request), (3) a path that uses .. components to escape the allowed directory (e.g., /snapshots/../etc/passwd, should fail), and (4) a non-existent allowed directory (should fail with service_error).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| // Must hide error in release builds to not leak contents of potentially sensitive files | ||
| #[cfg(not(debug_assertions))] | ||
| format!("Malformed tar archive, unable to read entries"), | ||
| #[cfg(debug_assertions)] | ||
| format!("Malformed tar archive, unable to read entries: {err}"), |
There was a problem hiding this comment.
A core problem was that we expose the tar error message, which could leak file contents.
This now hides the tar message in release builds. It is kept in debug builds as it's still useful for development.
Alternatively we can hide the message for both build types. Any opinions?
cc @generall
timvisee
changed the title
…lection_snapshot The recover from snapshot API now requires file:// paths to be inside the configured snapshots directory. Write the test snapshot into the peer's snapshots directory instead of a temp file so the request is allowed. Made-with: Cursor
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…#8341) * restrict recovery from snapshot folder * fmt * clippy * Extend test, path must be inside snapshots directory * In release builds, hide detailed tar error message to not leak contents * Change from bad request to forbidden * fix(auth_tests): place snapshot in peer snapshots dir for recover_collection_snapshot The recover from snapshot API now requires file:// paths to be inside the configured snapshots directory. Write the test snapshot into the peer's snapshots directory instead of a temp file so the request is allowed. Made-with: Cursor --------- Co-authored-by: timvisee <[email protected]> Co-authored-by: Tim Visée <[email protected]> Co-authored-by: Cursor Agent <[email protected]>
…#8341) * restrict recovery from snapshot folder * fmt * clippy * Extend test, path must be inside snapshots directory * In release builds, hide detailed tar error message to not leak contents * Change from bad request to forbidden * fix(auth_tests): place snapshot in peer snapshots dir for recover_collection_snapshot The recover from snapshot API now requires file:// paths to be inside the configured snapshots directory. Write the test snapshot into the peer's snapshots directory instead of a temp file so the request is allowed. Made-with: Cursor --------- Co-authored-by: timvisee <[email protected]> Co-authored-by: Tim Visée <[email protected]> Co-authored-by: Cursor Agent <[email protected]>
Only allow
file://...snapshots to be recoverred from snapshot directly, for security reasons.