feat(cloud save config): mirror game lib structure - #434
Conversation
Shellywell123
changed the title
There was a problem hiding this comment.
Pull Request Overview
This pull request adds a configurable save directory structure feature, allowing users to choose between emulator/game ID-based paths or a structure that mirrors the game library organization. The implementation spans protobuf schema changes, backend Rust services, and frontend TypeScript UI components.
Key changes:
- Introduced a new
SaveDirStructureenum with two options:EMULATOR_GAME(default) andMIRROR_LIBRARY - Modified save file and save state managers to support dynamic path resolution based on configuration
- Added UI controls for users to select their preferred directory structure
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/codegen/protos/retrom/server/config.proto | Added SaveDirStructure enum and save_dir_structure field to SavesConfig message |
| packages/codegen/protos/retrom/services/server-config.proto | Added deprecation notice that file was moved |
| packages/service-common/src/config/mod.rs | Set default save_dir_structure configuration to EmulatorGame |
| packages/grpc-service/src/saves/save_file_manager.rs | Implemented mirror library path structure logic for save files with async config access |
| packages/grpc-service/src/saves/save_state_manager.rs | Implemented mirror library path structure logic for save states with async config access |
| packages/grpc-service/src/saves/mod.rs | Added .await calls for async directory path resolution methods |
| packages/client-web/src/components/modals/config/server/saves-config.tsx | Added UI dropdown for selecting save directory structure preference |
| packages/client-web/src/queries/saveFiles.tsx | Added optional enabled parameter to useGetSaveFiles query hook |
| packages/client-web/src/providers/emulator-js/ejs-session.tsx | Added fallback for undefined game.id when constructing save file query |
| packages/client-web/vite.config.ts | Added error handler to silently ignore telemetry connection refused errors |
| packages/client/gen/schemas/linux-schema.json | Auto-generated schema updates for Tauri permissions and capabilities |
| if !platform.contains("..") && !game_name.contains("..") | ||
| && !platform.is_empty() && !game_name.is_empty() { | ||
| let save_path = format!("{}/{}", platform, game_name); | ||
| tracing::info!("Final save path: {:?}", save_path); | ||
| return Ok(saves_dir.join(save_path)); |
There was a problem hiding this comment.
The path sanitization logic is insufficient to prevent path traversal attacks. The check !platform.contains("..") and !game_name.contains("..") only prevents literal ".." sequences, but doesn't catch other dangerous patterns like absolute paths, symlinks, or encoded path traversal sequences.
Consider using a more robust path validation approach:
- Use
.canonicalize()to resolve symbolic links and normalize paths - Verify the final path still starts with the expected base directory
- Check for null bytes or other special characters
Example:
let final_path = saves_dir.join(&save_path).canonicalize()?;
if !final_path.starts_with(&saves_dir.canonicalize()?) {
return Err(anyhow!("Invalid path traversal detected"));
}| tracing::info!( | ||
| "get_saves_dir - Game ID: {}, Save dir structure config: {:?}, MirrorLibrary enum value: {}, comparison: {}", | ||
| self.game.id, | ||
| save_dir_structure, | ||
| mirror_library_value, | ||
| save_dir_structure.map(|s| s == mirror_library_value).unwrap_or(false) | ||
| ); | ||
|
|
||
| if save_dir_structure.map(|s| s == mirror_library_value).unwrap_or(false) { | ||
| let game_path = PathBuf::from(&self.game.path); | ||
| tracing::info!("Mirror library mode - Game path: {:?}", game_path); | ||
|
|
||
| // Find the content directory that contains this game | ||
| let content_dirs = &config.content_directories; | ||
| tracing::info!("Content directories: {:?}", content_dirs); | ||
| for content_dir in content_dirs { | ||
| let content_path = PathBuf::from(&content_dir.path); | ||
| tracing::info!("Checking content path: {:?}", content_path); | ||
|
|
||
| // Try to get the relative path from the content directory to the game | ||
| if let Ok(relative_game_path) = game_path.strip_prefix(&content_path) { | ||
| tracing::info!("Found matching content dir! Relative game path: {:?}", relative_game_path); | ||
|
|
||
| // Extract platform and game name from the relative path | ||
| let path_components: Vec<&str> = relative_game_path.components() | ||
| .filter_map(|comp| { | ||
| if let std::path::Component::Normal(os_str) = comp { | ||
| os_str.to_str() | ||
| } else { | ||
| None | ||
| } | ||
| }) | ||
| .collect(); | ||
|
|
||
| tracing::info!("Path components: {:?}", path_components); | ||
|
|
||
| // For both single-file and multi-file games, we want platform/game_name structure | ||
| if path_components.len() >= 2 { | ||
| let platform = path_components[0]; | ||
| let game_name = path_components[1]; | ||
|
|
||
| tracing::info!("Using platform: {:?}, game_name: {:?}", platform, game_name); | ||
|
|
||
| // Sanitize each component | ||
| if !platform.contains("..") && !game_name.contains("..") | ||
| && !platform.is_empty() && !game_name.is_empty() { | ||
| let save_path = format!("{}/{}", platform, game_name); | ||
| tracing::info!("Final save path: {:?}", save_path); |
There was a problem hiding this comment.
Excessive debug logging should be removed before merging to production. Lines 246-293 contain numerous tracing::info! calls that log detailed internal state including path components, configuration values, and directory structures. This adds noise to logs and could expose sensitive information about the file system structure.
Consider either:
- Removing these debug logs entirely, or
- Changing them to
tracing::debug!level so they're only visible when explicitly enabled for debugging
| if !platform.contains("..") && !game_name.contains("..") | ||
| && !platform.is_empty() && !game_name.is_empty() { | ||
| let save_path = format!("{}/{}", platform, game_name); | ||
| return Ok(backup_dir.join(save_path)); |
There was a problem hiding this comment.
The path sanitization logic is insufficient to prevent path traversal attacks. The check !platform.contains("..") and !game_name.contains("..") only prevents literal ".." sequences, but doesn't catch other dangerous patterns like absolute paths, symlinks, or encoded path traversal sequences.
Consider using a more robust path validation approach:
- Use
.canonicalize()to resolve symbolic links and normalize paths - Verify the final path still starts with the expected base directory
- Check for null bytes or other special characters
Example:
let final_path = backup_dir.join(&save_path).canonicalize()?;
if !final_path.starts_with(&backup_dir.canonicalize()?) {
return Err(anyhow!("Invalid path traversal detected"));
}| .join(emulator_id.to_string()) | ||
| .join(self.game.id.to_string())) | ||
| // Check if we should use the game library path structure | ||
| if config.saves.as_ref().and_then(|s| s.save_dir_structure).map(|s| s == retrom_codegen::retrom::SaveDirStructure::MirrorLibrary as i32).unwrap_or(false) { |
There was a problem hiding this comment.
The complex conditional logic on line 251 is difficult to read and understand. The nested as_ref(), and_then(), map(), and unwrap_or() chain with an embedded enum comparison makes it hard to follow the logic flow.
Consider extracting this to a helper method or variable:
let should_use_mirror_library = config.saves
.as_ref()
.and_then(|s| s.save_dir_structure)
.map(|s| s == retrom_codegen::retrom::SaveDirStructure::MirrorLibrary as i32)
.unwrap_or(false);
if should_use_mirror_library {
// ...
}This improves readability and makes the condition's purpose clearer.
| if config.saves.as_ref().and_then(|s| s.save_dir_structure).map(|s| s == retrom_codegen::retrom::SaveDirStructure::MirrorLibrary as i32).unwrap_or(false) { | |
| let should_use_mirror_library = config.saves | |
| .as_ref() | |
| .and_then(|s| s.save_dir_structure) | |
| .map(|s| s == retrom_codegen::retrom::SaveDirStructure::MirrorLibrary as i32) | |
| .unwrap_or(false); | |
| if should_use_mirror_library { |
| .join(emulator_id.to_string()) | ||
| .join(self.game.id.to_string())) | ||
| // Check if we should use the game library path structure | ||
| if config.saves.as_ref().and_then(|s| s.save_dir_structure).map(|s| s == retrom_codegen::retrom::SaveDirStructure::MirrorLibrary as i32).unwrap_or(false) { |
There was a problem hiding this comment.
The complex conditional logic on line 317 is difficult to read and understand. The nested as_ref(), and_then(), map(), and unwrap_or() chain with an embedded enum comparison makes it hard to follow the logic flow.
Consider extracting this to a helper method or variable:
let should_use_mirror_library = config.saves
.as_ref()
.and_then(|s| s.save_dir_structure)
.map(|s| s == retrom_codegen::retrom::SaveDirStructure::MirrorLibrary as i32)
.unwrap_or(false);
if should_use_mirror_library {
// ...
}This improves readability and makes the condition's purpose clearer.
| if config.saves.as_ref().and_then(|s| s.save_dir_structure).map(|s| s == retrom_codegen::retrom::SaveDirStructure::MirrorLibrary as i32).unwrap_or(false) { | |
| let should_use_mirror_library = config.saves | |
| .as_ref() | |
| .and_then(|s| s.save_dir_structure) | |
| .map(|s| s == retrom_codegen::retrom::SaveDirStructure::MirrorLibrary as i32) | |
| .unwrap_or(false); | |
| if should_use_mirror_library { |
| if !platform.contains("..") && !game_name.contains("..") | ||
| && !platform.is_empty() && !game_name.is_empty() { | ||
| let save_path = format!("{}/{}", platform, game_name); | ||
| return Ok(backup_dir.join(save_path)); |
There was a problem hiding this comment.
The path sanitization logic is insufficient to prevent path traversal attacks. The check !platform.contains("..") and !game_name.contains("..") only prevents literal ".." sequences, but doesn't catch other dangerous patterns like absolute paths, symlinks, or encoded path traversal sequences.
Consider using a more robust path validation approach:
- Use
.canonicalize()to resolve symbolic links and normalize paths - Verify the final path still starts with the expected base directory
- Check for null bytes or other special characters
Example:
let final_path = backup_dir.join(&save_path).canonicalize()?;
if !final_path.starts_with(&backup_dir.canonicalize()?) {
return Err(anyhow!("Invalid path traversal detected"));
}| if let Some(game_dir) = relative_game_path.parent() { | ||
| let path_components: Vec<&str> = game_dir.components() | ||
| .filter_map(|comp| { | ||
| if let std::path::Component::Normal(os_str) = comp { | ||
| os_str.to_str() | ||
| } else { | ||
| None | ||
| } | ||
| }) | ||
| .collect(); | ||
|
|
||
| // We want platform/game_name structure (first two components) | ||
| if path_components.len() >= 2 { | ||
| let platform = path_components[0]; | ||
| let game_name = path_components[1]; | ||
|
|
||
| // Sanitize each component | ||
| if !platform.contains("..") && !game_name.contains("..") | ||
| && !platform.is_empty() && !game_name.is_empty() { | ||
| let save_path = format!("{}/{}", platform, game_name); | ||
| return Ok(states_dir.join(save_path)); | ||
| } |
There was a problem hiding this comment.
Inconsistent path extraction logic between save file and save state managers. In save_file_manager.rs, the path components are extracted directly from relative_game_path (line 270), while in save_state_manager.rs, they're extracted from relative_game_path.parent().
This means:
- Save files: Uses path structure like
platform/game_namefrom the full game file path - Save states: Uses path structure like
platform/game_namefrom the parent directory of the game file path
This could lead to different directory structures for save files vs save states for the same game, which is likely unintended. Consider standardizing the path extraction logic across both managers to ensure consistency.
| if let Some(game_dir) = relative_game_path.parent() { | |
| let path_components: Vec<&str> = game_dir.components() | |
| .filter_map(|comp| { | |
| if let std::path::Component::Normal(os_str) = comp { | |
| os_str.to_str() | |
| } else { | |
| None | |
| } | |
| }) | |
| .collect(); | |
| // We want platform/game_name structure (first two components) | |
| if path_components.len() >= 2 { | |
| let platform = path_components[0]; | |
| let game_name = path_components[1]; | |
| // Sanitize each component | |
| if !platform.contains("..") && !game_name.contains("..") | |
| && !platform.is_empty() && !game_name.is_empty() { | |
| let save_path = format!("{}/{}", platform, game_name); | |
| return Ok(states_dir.join(save_path)); | |
| } | |
| // Extract path components directly from relative_game_path (not its parent) | |
| let path_components: Vec<&str> = relative_game_path.components() | |
| .filter_map(|comp| { | |
| if let std::path::Component::Normal(os_str) = comp { | |
| os_str.to_str() | |
| } else { | |
| None | |
| } | |
| }) | |
| .collect(); | |
| // We want platform/game_name structure (first two components) | |
| if path_components.len() >= 2 { | |
| let platform = path_components[0]; | |
| let game_name = path_components[1]; | |
| // Sanitize each component | |
| if !platform.contains("..") && !game_name.contains("..") | |
| && !platform.is_empty() && !game_name.is_empty() { | |
| let save_path = format!("{}/{}", platform, game_name); | |
| return Ok(states_dir.join(save_path)); |
| const saveFilesQuery = useGetSaveFiles({ | ||
| saveFilesSelectors: [{ gameId: game.id, emulatorId: emulator?.id }], | ||
| saveFilesSelectors: [ | ||
| { | ||
| gameId: game?.id ?? 0, | ||
| emulatorId: emulator?.id, | ||
| }, | ||
| ], | ||
| }); |
There was a problem hiding this comment.
Potential issue with the query being enabled even when game ID is 0. The fallback game?.id ?? 0 on line 49 means the query will execute with gameId: 0 when game is undefined. This could result in unnecessary API calls with invalid data.
Consider either:
- Disabling the query when game is not available using the new
enabledoption:
const saveFilesQuery = useGetSaveFiles(
{
saveFilesSelectors: [{
gameId: game?.id ?? 0,
emulatorId: emulator?.id,
}],
},
{ enabled: !!game?.id }
);- Or conditionally call the hook only when game is available.
| if !platform.contains("..") && !game_name.contains("..") | ||
| && !platform.is_empty() && !game_name.is_empty() { | ||
| let save_path = format!("{}/{}", platform, game_name); | ||
| return Ok(states_dir.join(save_path)); |
There was a problem hiding this comment.
The path sanitization logic is insufficient to prevent path traversal attacks. The check !platform.contains("..") and !game_name.contains("..") only prevents literal ".." sequences, but doesn't catch other dangerous patterns like absolute paths, symlinks, or encoded path traversal sequences.
Consider using a more robust path validation approach:
- Use
.canonicalize()to resolve symbolic links and normalize paths - Verify the final path still starts with the expected base directory
- Check for null bytes or other special characters
Example:
let final_path = states_dir.join(&save_path).canonicalize()?;
if !final_path.starts_with(&states_dir.canonicalize()?) {
return Err(anyhow!("Invalid path traversal detected"));
}
|
@Shellywell123 Ignore the copilot review for now -- I'll go over its comments and let you know if any are relevant once I have time to review everything myself. Thanks! 🙂 |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Shellywell123 <44142139+Shellywell123@users.noreply.github.com>
|
@Shellywell123 There are a couple concerns I have with this approach:
Is the goal of this change solely to guard against a loss or failure of the Retrom DB? If so, I would highly prefer to dedicate time and effort into a proper backup+restore solution with the DB in mind. What are your thoughts? |
|
@JMBeresford To address your concerns + ask some clarifying questions back. :)
|
b46014c to
ee77021
Compare
1df97b4 to
6214270
Compare
eb02a1d to
e738973
Compare
The As far as Retrom is concerned, your save path from that example is
I am hesitant for two reasons:
Unfortunately, not all users opt-in to use IGDB. This is why Retrom requires manual configuration of everything. As such, there is no well-defined spec that can be used, it is all user-defined on a case-by-case basis. Additionally, there is no real way to map IGDB platform to a given emulator. tldr; I am sorry that you went to all of this effort just to be met with a 'not quite yet'. This is why I generally urge contributors start a discussion prior to any implementation work. Let's put a pin in this, at least until the many in-progress refactors are done and Retrom's architecture has matured a bit more. |
2b76cc7 to
798c1b7
Compare
No worries, I totally understand the hesitations and the answer given due to the current state of the project. I still think mimicking/duplicating the file path to a users rom, is the best way to map the two. But happy to hear other suggestions :) Alternatively we could incorporate a save file exporter tool, that retrieves all the files and dumps them back out in a sepcific format, but again this is not as clean as just storing the files in a set structure to begin with. |
A new cloud save method that will mirror the structure of the users game library.
The new
Mirror Game Library Structureoption under cloud save configurations, will mimic the structure of the users game library. This is an attempt to improve the currentEmulator/Game ID basedsystem, where the IDs are dependant on the order you setup emulators and import games. The advantage of this new method is that the mapping of saves to games persists even if the database is destroyed.Web Frontend
New drop down allowing the user to specify the file save path structure.
Server File structure
example game lib
default
Emulator/Game ID basedmethodnew
Mirror Game Library StructuremethodDisclaimer: This was built heavily with AI using sonnet 4.5 and copilot