Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import { TabsContent } from "@retrom/ui/components/tabs";
import {
SavesConfigSchema,
ServerConfig,
SaveDirStructure,
} from "@retrom/codegen/retrom/server/config_pb";
import { useUpdateServerConfig } from "@/mutations/useUpdateServerConfig";
import { zodResolver } from "@hookform/resolvers/zod";
Expand All @@ -31,6 +32,7 @@ type SavesConfigShape = Record<
const savesSchema = z.object({
maxSaveFilesBackups: z.coerce.number().default(5),
maxSaveStatesBackups: z.coerce.number().default(5),
saveDirStructure: z.nativeEnum(SaveDirStructure).default(SaveDirStructure.EMULATOR_GAME),
}) satisfies z.ZodObject<SavesConfigShape>;

export function SavesConfig(props: {
Expand All @@ -41,9 +43,10 @@ export function SavesConfig(props: {

const form = useForm<z.infer<typeof savesSchema>>({
resolver: zodResolver(savesSchema),
defaultValues: props.currentConfig.saves ?? {
maxSaveFilesBackups: 5,
maxSaveStatesBackups: 5,
defaultValues: {
maxSaveFilesBackups: props.currentConfig.saves?.maxSaveFilesBackups ?? 5,
maxSaveStatesBackups: props.currentConfig.saves?.maxSaveStatesBackups ?? 5,
saveDirStructure: props.currentConfig.saves?.saveDirStructure ?? SaveDirStructure.EMULATOR_GAME,
},
});

Expand Down Expand Up @@ -110,6 +113,27 @@ export function SavesConfig(props: {
)}
/>

<FormField
control={form.control}
name="saveDirStructure"
render={({ field }) => (
<FormItem>
<FormLabel>Save File Directory Structure</FormLabel>
<FormControl>
<select
className="w-full px-3 py-2 bg-background border rounded-md"
value={field.value}
onChange={(e) => field.onChange(parseInt(e.target.value))}
>
<option value={SaveDirStructure.EMULATOR_GAME}>Emulator/Game ID Based</option>
<option value={SaveDirStructure.MIRROR_LIBRARY}>Mirror Game Library Structure</option>
</select>
</FormControl>
<FormMessage />
</FormItem>
)}
/>

<DialogFooter className="gap-2">
<Button
onClick={() =>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,12 @@ export function EJSSessionStateProvider(props: PropsWithChildren) {
// const { uploadFiles, downloadFiles } = useRemoteFiles();
const { mutateAsync: updateSave } = useUpdateSaveFiles();
const saveFilesQuery = useGetSaveFiles({
saveFilesSelectors: [{ gameId: game.id, emulatorId: emulator?.id }],
saveFilesSelectors: [
{
gameId: game?.id ?? 0,
emulatorId: emulator?.id,
},
],
});
Comment on lines 46 to 53
Copy link

Copilot AI Nov 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Disabling the query when game is not available using the new enabled option:
const saveFilesQuery = useGetSaveFiles(
  {
    saveFilesSelectors: [{
      gameId: game?.id ?? 0,
      emulatorId: emulator?.id,
    }],
  },
  { enabled: !!game?.id }
);
  1. Or conditionally call the hook only when game is available.

Copilot uses AI. Check for mistakes.

const startTime = useMemo(() => Date.now(), []);
Expand Down
2 changes: 2 additions & 0 deletions packages/client-web/src/queries/saveFiles.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,12 @@ export function useStatSaveFiles(

export function useGetSaveFiles(
request: MessageInitShape<typeof GetSaveFilesRequestSchema>,
options?: { enabled?: boolean },
) {
const retromClient = useRetromClient();

return useQuery({
enabled: options?.enabled !== false,
queryFn: async () => {
const { update } = toast({
id: "download-remote-saves",
Expand Down
9 changes: 9 additions & 0 deletions packages/client-web/vite.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,15 @@ export default defineConfig(({ mode }) => {
"/v1/traces": {
target: localTracesEndpoint,
changeOrigin: true,
configure: (proxy, _options) => {
proxy.on('error', (err, _req, _res) => {
if (err.code === 'ECONNREFUSED') {
// Silently ignore telemetry connection errors
return;
}
console.log('proxy error', err);
Copy link

Copilot AI Nov 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent error handling: the code silently ignores ECONNREFUSED errors but logs other errors to console. However, the log message uses console.log instead of console.error, which is inconsistent for an error handler.

Consider using console.error for proper error logging:

console.error('proxy error', err);
Suggested change
console.log('proxy error', err);
console.error('proxy error', err);

Copilot uses AI. Check for mistakes.
});
},
},
},
},
Expand Down
8 changes: 8 additions & 0 deletions packages/codegen/protos/retrom/server/config.proto
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,17 @@ message SteamConfig {
string user_id = 2;
}

enum SaveDirStructure {
// Default save directory structure: <data_dir>/saves/<emulator_id>/<game_id>/
EMULATOR_GAME = 0;
// Mirror game library structure: <data_dir>/saves/<game_library_path>/
MIRROR_LIBRARY = 1;
}

message SavesConfig {
int32 max_save_files_backups = 1;
int32 max_save_states_backups = 2;
optional SaveDirStructure save_dir_structure = 3;
}

message MetadataConfig {
Expand Down
4 changes: 4 additions & 0 deletions packages/codegen/protos/retrom/services/server-config.proto
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
syntax = "proto3";
package retrom;

// This file was moved to server/config.proto
2 changes: 2 additions & 0 deletions packages/grpc-service/src/saves/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,7 @@ impl SavesService for SavesServiceHandlers {

let save_dir = save_file_manager
.get_saves_dir(emulator_id)
.await
.map_err(|e| tonic::Status::internal(e.to_string()))?;

for file_stat in save_file_stat.file_stats.into_iter() {
Expand Down Expand Up @@ -256,6 +257,7 @@ impl SavesService for SavesServiceHandlers {

let states_dir = save_state_manager
.get_states_dir(emulator_id)
.await
.map_err(|e| tonic::Status::internal(e.to_string()))?;

for save_states_stat in save_states {
Expand Down
144 changes: 129 additions & 15 deletions packages/grpc-service/src/saves/save_file_manager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,8 @@ impl GameSaveFileManager {
pub trait SaveFileManager {
async fn resolve_save_files(&self, include_backups: bool) -> Result<Vec<SaveFilesStat>>;
async fn reindex_backups(&self, emulator_id: Option<i32>) -> Result<()>;
fn get_saves_dir(&self, emulator_id: Option<i32>) -> Result<PathBuf>;
fn get_saves_backup_dir(&self, emulator_id: Option<i32>) -> Result<PathBuf>;
async fn get_saves_dir(&self, emulator_id: Option<i32>) -> Result<PathBuf>;
async fn get_saves_backup_dir(&self, emulator_id: Option<i32>) -> Result<PathBuf>;

async fn backup_save_files(
&self,
Expand Down Expand Up @@ -96,8 +96,8 @@ impl SaveFileManager for GameSaveFileManager {
let mut files = vec![];

for emulator_id in emulators.iter().map(|e| e.id) {
let save_dir = self.get_saves_dir(Some(emulator_id))?;
let backup_dir = self.get_saves_backup_dir(Some(emulator_id))?;
let save_dir = self.get_saves_dir(Some(emulator_id)).await?;
let backup_dir = self.get_saves_backup_dir(Some(emulator_id)).await?;

let file_stats: Vec<FileStat> = if save_dir.exists() {
WalkDir::new(&save_dir)
Expand Down Expand Up @@ -227,7 +227,8 @@ impl SaveFileManager for GameSaveFileManager {
}

#[instrument(skip(self), fields(game_id = self.game.id))]
fn get_saves_dir(&self, emulator_id: Option<i32>) -> Result<PathBuf> {
async fn get_saves_dir(&self, emulator_id: Option<i32>) -> Result<PathBuf> {
let config = self.config.get_config().await;
let saves_dir = RetromDirs::new().data_dir().join("saves");

let emulator_id = match emulator_id {
Expand All @@ -239,13 +240,79 @@ impl SaveFileManager for GameSaveFileManager {
}
};

Ok(saves_dir
.join(emulator_id.to_string())
.join(self.game.id.to_string()))
// Check if we should use the game library path structure
let save_dir_structure = config.saves.as_ref().and_then(|s| s.save_dir_structure);
let mirror_library_value = retrom_codegen::retrom::SaveDirStructure::MirrorLibrary as i32;
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);
Comment on lines +246 to +293
Copy link

Copilot AI Nov 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copilot uses AI. Check for mistakes.
return Ok(saves_dir.join(save_path));
Comment on lines +290 to +294
Copy link

Copilot AI Nov 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"));
}

Copilot uses AI. Check for mistakes.
}
}
break; // Found the matching content directory, no need to continue
}
}

// Fallback to default structure if no content directory matched or path is invalid
Ok(saves_dir
.join(emulator_id.to_string())
.join(self.game.id.to_string()))
} else {
// Default directory structure
Ok(saves_dir
.join(emulator_id.to_string())
.join(self.game.id.to_string()))
}
}

#[instrument(skip(self), fields(game_id = self.game.id))]
fn get_saves_backup_dir(&self, emulator_id: Option<i32>) -> Result<PathBuf> {
async fn get_saves_backup_dir(&self, emulator_id: Option<i32>) -> Result<PathBuf> {
let config = self.config.get_config().await;
let backup_dir = RetromDirs::new().data_dir().join("saves_backups");

let emulator_id = match emulator_id {
Expand All @@ -257,9 +324,56 @@ impl SaveFileManager for GameSaveFileManager {
}
};

Ok(backup_dir
.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) {
Copy link

Copilot AI Nov 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The complex conditional logic on line 328 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.

Copilot uses AI. Check for mistakes.
let game_path = PathBuf::from(&self.game.path);

// Find the content directory that contains this game
let content_dirs = &config.content_directories;
for content_dir in content_dirs {
let content_path = PathBuf::from(&content_dir.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) {
// Get the parent directory of the game file (game directory)
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(backup_dir.join(save_path));
Comment on lines +356 to +359
Copy link

Copilot AI Nov 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"));
}

Copilot uses AI. Check for mistakes.
}
}
}
break; // Found the matching content directory, no need to continue
}
}

// Fallback to default structure if no content directory matched or path is invalid
Ok(backup_dir
.join(emulator_id.to_string())
.join(self.game.id.to_string()))
} else {
// Default directory structure
Ok(backup_dir
.join(emulator_id.to_string())
.join(self.game.id.to_string()))
}
}

#[instrument(skip(self), fields(game_id = self.game.id))]
Expand All @@ -276,7 +390,7 @@ impl SaveFileManager for GameSaveFileManager {

for save_files in all_save_files.iter_mut() {
let save_path = PathBuf::from(&save_files.save_path);
let backups_dir = self.get_saves_backup_dir(save_files.emulator_id)?;
let backups_dir = self.get_saves_backup_dir(save_files.emulator_id).await?;
let backup_dir =
backups_dir.join(chrono::Local::now().to_utc().format("%s.%f").to_string());

Expand Down Expand Up @@ -362,7 +476,7 @@ impl SaveFileManager for GameSaveFileManager {

#[instrument(skip_all, fields(game_id = self.game.id))]
async fn update_save_files(&self, save_files: SaveFiles, dry_run: bool) -> Result<()> {
let save_dir = self.get_saves_dir(save_files.emulator_id)?;
let save_dir = self.get_saves_dir(save_files.emulator_id).await?;
self.backup_save_files(save_files.emulator_id, dry_run)
.await?;

Expand Down Expand Up @@ -461,7 +575,7 @@ impl SaveFileManager for GameSaveFileManager {
emulator_id: Option<i32>,
dry_run: bool,
) -> Result<()> {
let saves_dir = self.get_saves_dir(emulator_id)?;
let saves_dir = self.get_saves_dir(emulator_id).await?;
let backup_path = PathBuf::from(backup.backup_path);

if !backup_path.exists() {
Expand Down
Loading