From 76bdfb4891c33ce9b900a815fa0b52eba7beb40f Mon Sep 17 00:00:00 2001 From: Georges-Antoine Assi Date: Sat, 7 Mar 2026 09:36:45 -0500 Subject: [PATCH] changes from self review --- backend/endpoints/collections.py | 80 +++++++------- backend/endpoints/rom.py | 100 ++++++++---------- .../handler/filesystem/resources_handler.py | 30 +----- frontend/src/composables/useFavoriteToggle.ts | 4 +- frontend/src/console/views/Game.vue | 4 +- frontend/src/console/views/Play.vue | 4 +- frontend/src/services/api/setup.ts | 4 +- frontend/src/services/api/user.ts | 9 +- frontend/src/stores/galleryFilter.ts | 4 +- 9 files changed, 95 insertions(+), 144 deletions(-) diff --git a/backend/endpoints/collections.py b/backend/endpoints/collections.py index 5a33eaa2f..022fbcf64 100644 --- a/backend/endpoints/collections.py +++ b/backend/endpoints/collections.py @@ -31,7 +31,7 @@ from models.collection import ( VirtualCollection, ) from utils.router import APIRouter -from utils.validation import ValidationError, validate_url_for_http_request +from utils.validation import ValidationError router = APIRouter( prefix="/collections", @@ -70,14 +70,6 @@ async def add_collection( "user_id": request.user.id, } - # Validate URL if provided - if url_cover: - try: - validate_url_for_http_request(url_cover, "Cover URL") - except ValidationError as e: - log.error(f"Invalid cover URL in add_collection: {str(e)}") - raise HTTPException(status_code=400, detail=str(e)) from e - db_collection = db_collection_handler.get_collection_by_name( cleaned_data["name"], request.user.id ) @@ -87,21 +79,25 @@ async def add_collection( _added_collection = db_collection_handler.add_collection(Collection(**cleaned_data)) - if artwork is not None and artwork.filename is not None: - file_ext = artwork.filename.split(".")[-1] - artwork_content = BytesIO(await artwork.read()) - ( - path_cover_l, - path_cover_s, - ) = await fs_resource_handler.store_artwork( - _added_collection, artwork_content, file_ext - ) - else: - path_cover_s, path_cover_l = await fs_resource_handler.get_cover( - entity=_added_collection, - overwrite=True, - url_cover=_added_collection.url_cover, - ) + try: + if artwork is not None and artwork.filename is not None: + file_ext = artwork.filename.split(".")[-1] + artwork_content = BytesIO(await artwork.read()) + ( + path_cover_l, + path_cover_s, + ) = await fs_resource_handler.store_artwork( + _added_collection, artwork_content, file_ext + ) + else: + path_cover_s, path_cover_l = await fs_resource_handler.get_cover( + entity=_added_collection, + overwrite=True, + url_cover=_added_collection.url_cover, + ) + except ValidationError as e: + log.error(f"Invalid cover URL in add_collection: {str(e)}") + raise HTTPException(status_code=400, detail=str(e)) from e _added_collection.path_cover_s = path_cover_s _added_collection.path_cover_l = path_cover_l @@ -448,26 +444,22 @@ async def update_collection( current_url_cover != collection.url_cover or not fs_resource_handler.cover_exists(collection, CoverSize.BIG) ): - # Validate URL if provided and changed - if current_url_cover: - try: - validate_url_for_http_request(current_url_cover, "Cover URL") - except ValidationError as e: - log.error(f"Invalid cover URL in update_collection: {str(e)}") - raise HTTPException(status_code=400, detail=str(e)) from e - - path_cover_s, path_cover_l = await fs_resource_handler.get_cover( - entity=collection, - overwrite=True, - url_cover=current_url_cover, - ) - cleaned_data.update( - { - "url_cover": current_url_cover, - "path_cover_s": path_cover_s, - "path_cover_l": path_cover_l, - } - ) + try: + path_cover_s, path_cover_l = await fs_resource_handler.get_cover( + entity=collection, + overwrite=True, + url_cover=current_url_cover, + ) + cleaned_data.update( + { + "url_cover": current_url_cover, + "path_cover_s": path_cover_s, + "path_cover_l": path_cover_l, + } + ) + except ValidationError as e: + log.error(f"Invalid cover URL in update_collection: {str(e)}") + raise HTTPException(status_code=400, detail=str(e)) from e updated_collection = db_collection_handler.update_collection( id, cleaned_data, parsed_rom_ids diff --git a/backend/endpoints/rom.py b/backend/endpoints/rom.py index 3ecf8009c..c4c360326 100644 --- a/backend/endpoints/rom.py +++ b/backend/endpoints/rom.py @@ -73,7 +73,7 @@ from utils.filesystem import sanitize_filename from utils.hashing import crc32_to_hex from utils.nginx import FileRedirectResponse, ZipContentLine, ZipResponse from utils.router import APIRouter -from utils.validation import ValidationError, validate_url_for_http_request +from utils.validation import ValidationError router = APIRouter( prefix="/roms", @@ -1291,24 +1291,20 @@ async def update_rom( cleaned_data.update({"igdb_id": None, "igdb_metadata": {}}) url_screenshots = cleaned_data.get("url_screenshots", []) - if url_screenshots: - for screenshot_url in url_screenshots: - try: - validate_url_for_http_request(str(screenshot_url), "Screenshot URL") - except ValidationError as e: - log.error(f"Invalid screenshot URL in update_rom: {str(e)}") - raise HTTPException(status_code=400, detail=str(e)) from e - screenshots_changed = pydash.xor(url_screenshots, rom.url_screenshots or []) if url_screenshots: - path_screenshots = await fs_resource_handler.get_rom_screenshots( - rom=rom, - overwrite=bool(screenshots_changed), - url_screenshots=cleaned_data.get("url_screenshots", []), - ) - cleaned_data.update( - {"path_screenshots": path_screenshots, "url_screenshots": []} - ) + try: + path_screenshots = await fs_resource_handler.get_rom_screenshots( + rom=rom, + overwrite=bool(screenshots_changed), + url_screenshots=cleaned_data.get("url_screenshots", []), + ) + cleaned_data.update( + {"path_screenshots": path_screenshots, "url_screenshots": []} + ) + except ValidationError as e: + log.error(f"Invalid screenshot URL in update_rom: {str(e)}") + raise HTTPException(status_code=400, detail=str(e)) from e cleaned_data.update( { @@ -1354,49 +1350,41 @@ async def update_rom( url_cover = ( form_data.url_cover if "url_cover" in provided_fields else rom.url_cover ) - # Validate URL if provided and changed - if url_cover and url_cover != rom.url_cover: - try: - validate_url_for_http_request(url_cover, "Cover URL") - except ValidationError as e: - log.error(f"Invalid cover URL in update_rom: {str(e)}") - raise HTTPException(status_code=400, detail=str(e)) from e - - path_cover_s, path_cover_l = await fs_resource_handler.get_cover( - entity=rom, - overwrite=url_cover != rom.url_cover, - url_cover=str(url_cover), - ) - cleaned_data.update( - { - "url_cover": url_cover, - "path_cover_s": path_cover_s, - "path_cover_l": path_cover_l, - } - ) + try: + path_cover_s, path_cover_l = await fs_resource_handler.get_cover( + entity=rom, + overwrite=url_cover != rom.url_cover, + url_cover=str(url_cover), + ) + cleaned_data.update( + { + "url_cover": url_cover, + "path_cover_s": path_cover_s, + "path_cover_l": path_cover_l, + } + ) + except ValidationError as e: + log.error(f"Invalid cover URL in update_rom: {str(e)}") + raise HTTPException(status_code=400, detail=str(e)) from e url_manual = ( form_data.url_manual if "url_manual" in provided_fields else rom.url_manual ) - # Validate URL if provided and changed - if url_manual and url_manual != rom.url_manual: - try: - validate_url_for_http_request(url_manual, "Manual URL") - except ValidationError as e: - log.error(f"Invalid manual URL in update_rom: {str(e)}") - raise HTTPException(status_code=400, detail=str(e)) from e - - path_manual = await fs_resource_handler.get_manual( - rom=rom, - overwrite=url_manual != rom.url_manual, - url_manual=str(url_manual) if url_manual else None, - ) - cleaned_data.update( - { - "url_manual": url_manual, - "path_manual": path_manual, - } - ) + try: + path_manual = await fs_resource_handler.get_manual( + rom=rom, + overwrite=url_manual != rom.url_manual, + url_manual=str(url_manual) if url_manual else None, + ) + cleaned_data.update( + { + "url_manual": url_manual, + "path_manual": path_manual, + } + ) + except ValidationError as e: + log.error(f"Invalid manual URL in update_rom: {str(e)}") + raise HTTPException(status_code=400, detail=str(e)) from e # Handle RetroAchievements badges when the ID has changed if cleaned_data["ra_id"] and int(cleaned_data["ra_id"]) != rom.ra_id: diff --git a/backend/handler/filesystem/resources_handler.py b/backend/handler/filesystem/resources_handler.py index 7879af7c9..ee5771a76 100644 --- a/backend/handler/filesystem/resources_handler.py +++ b/backend/handler/filesystem/resources_handler.py @@ -95,11 +95,7 @@ class FSResourcesHandler(FSHandler): else: # Handle HTTP URLs # Validate URL to prevent SSRF attacks - try: - validate_url_for_http_request(url_cover, "Cover URL") - except ValidationError as e: - log.error(f"URL validation failed for cover: {str(e)}") - return None + validate_url_for_http_request(url_cover, "Cover URL") httpx_client = ctx_httpx_client.get() try: @@ -268,11 +264,7 @@ class FSResourcesHandler(FSHandler): else: # Handle HTTP URLs # Validate URL to prevent SSRF attacks - try: - validate_url_for_http_request(url_screenhot, "Screenshot URL") - except ValidationError as e: - log.error(f"URL validation failed for screenshot: {str(e)}") - return None + validate_url_for_http_request(url_screenhot, "Screenshot URL") httpx_client = ctx_httpx_client.get() try: @@ -388,11 +380,7 @@ class FSResourcesHandler(FSHandler): else: # Handle HTTP URL # Validate URL to prevent SSRF attacks - try: - validate_url_for_http_request(url_manual, "Manual URL") - except ValidationError as e: - log.error(f"URL validation failed for manual: {str(e)}") - return None + validate_url_for_http_request(url_manual, "Manual URL") httpx_client = ctx_httpx_client.get() try: @@ -453,11 +441,7 @@ class FSResourcesHandler(FSHandler): # Retroachievements async def store_ra_badge(self, url: str, path: str) -> None: # Validate URL to prevent SSRF attacks - try: - validate_url_for_http_request(url, "Badge URL") - except ValidationError as e: - log.error(f"URL validation failed for badge: {str(e)}") - return + validate_url_for_http_request(url, "Badge URL") httpx_client = ctx_httpx_client.get() directory, filename = os.path.split(path) @@ -523,11 +507,7 @@ class FSResourcesHandler(FSHandler): else: # Handle HTTP URLs # Validate URL to prevent SSRF attacks - try: - validate_url_for_http_request(url, "Media URL") - except ValidationError as e: - log.error(f"URL validation failed for media file: {str(e)}") - return None + validate_url_for_http_request(url, "Media URL") httpx_client = ctx_httpx_client.get() try: diff --git a/frontend/src/composables/useFavoriteToggle.ts b/frontend/src/composables/useFavoriteToggle.ts index 38b36c6b1..a975b5a62 100644 --- a/frontend/src/composables/useFavoriteToggle.ts +++ b/frontend/src/composables/useFavoriteToggle.ts @@ -47,7 +47,7 @@ export function useFavoriteToggle(emitter?: Emitter) { const currentlyFav = fav.rom_ids.includes(rom.id); if (currentlyFav) { - fav.rom_ids = fav.rom_ids.filter((id: number) => id !== rom.id); + fav.rom_ids = fav.rom_ids.filter((id) => id !== rom.id); if (romsStore.currentCollection?.id === fav.id) { romsStore.remove([rom]); } @@ -72,7 +72,7 @@ export function useFavoriteToggle(emitter?: Emitter) { if (currentlyFav) { fav.rom_ids.push(rom.id); } else { - fav.rom_ids = fav.rom_ids.filter((id: number) => id !== rom.id); + fav.rom_ids = fav.rom_ids.filter((id) => id !== rom.id); } const detail = (error as { response?: { data?: { detail?: string } } }) ?.response?.data?.detail; diff --git a/frontend/src/console/views/Game.vue b/frontend/src/console/views/Game.vue index 96796859a..4c20761c4 100644 --- a/frontend/src/console/views/Game.vue +++ b/frontend/src/console/views/Game.vue @@ -11,7 +11,6 @@ import { } from "vue"; import { useI18n } from "vue-i18n"; import { useRoute, useRouter } from "vue-router"; -import type { DetailedRomSchema } from "@/__generated__/models/DetailedRomSchema"; import BackButton from "@/console/components/BackButton.vue"; import NavigationHint from "@/console/components/NavigationHint.vue"; import NavigationText from "@/console/components/NavigationText.vue"; @@ -22,6 +21,7 @@ import { ROUTES } from "@/plugins/router"; import romApi from "@/services/api/rom"; import stateApi from "@/services/api/state"; import storeHeartbeat from "@/stores/heartbeat"; +import type { DetailedRom } from "@/stores/roms"; import storeRoms from "@/stores/roms"; import { getSupportedEJSCores, toBrowserLocale } from "@/utils"; import { @@ -46,7 +46,7 @@ const route = useRoute(); const router = useRouter(); const { t, locale } = useI18n(); -const rom = ref(null); +const rom = ref(null); const playerState = ref("loading"); const errorMessage = ref(null); diff --git a/frontend/src/console/views/Play.vue b/frontend/src/console/views/Play.vue index eb13dbf31..e9e6e1b03 100644 --- a/frontend/src/console/views/Play.vue +++ b/frontend/src/console/views/Play.vue @@ -15,7 +15,6 @@ import type { Body_add_state_api_states_post as AddStateInput, FirmwareSchema, } from "@/__generated__"; -import type { DetailedRomSchema } from "@/__generated__/models/DetailedRomSchema"; import NavigationText from "@/console/components/NavigationText.vue"; import { useInputScope } from "@/console/composables/useInputScope"; import { useThemeAssets } from "@/console/composables/useThemeAssets"; @@ -25,6 +24,7 @@ import firmwareApi from "@/services/api/firmware"; import romApi from "@/services/api/rom"; import storeConfig from "@/stores/config"; import storeLanguage from "@/stores/language"; +import type { DetailedRom } from "@/stores/roms"; import { getSupportedEJSCores, getControlSchemeForPlatform, @@ -60,7 +60,7 @@ const { selectedLanguage } = storeToRefs(languageStore); const romId = Number(route.params.rom); const initialSaveId = route.query.save ? Number(route.query.save) : null; const initialStateId = route.query.state ? Number(route.query.state) : null; -const romRef = ref(null); +const romRef = ref(null); const showHint = ref(true); const bezelSrc = ref(""); const showExitPrompt = ref(false); diff --git a/frontend/src/services/api/setup.ts b/frontend/src/services/api/setup.ts index c3c95bf6f..77c23ebd1 100644 --- a/frontend/src/services/api/setup.ts +++ b/frontend/src/services/api/setup.ts @@ -1,5 +1,5 @@ -import type { PlatformSchema } from "@/__generated__"; import api from "@/services/api"; +import type { Platform } from "@/stores/platforms"; export type LibraryStructure = "struct_a" | "struct_b" | null; @@ -11,7 +11,7 @@ export interface ExistingPlatform { export interface SetupLibraryInfo { detected_structure: LibraryStructure; existing_platforms: ExistingPlatform[]; - supported_platforms: PlatformSchema[]; + supported_platforms: Platform[]; } export interface CreatePlatformsResponse { diff --git a/frontend/src/services/api/user.ts b/frontend/src/services/api/user.ts index f1bb3487d..3dfb420ae 100644 --- a/frontend/src/services/api/user.ts +++ b/frontend/src/services/api/user.ts @@ -62,13 +62,6 @@ async function updateUser({ avatar?: File; password?: string; }) { - const uiSettings = - typeof attrs.ui_settings === "string" - ? attrs.ui_settings - : attrs.ui_settings - ? JSON.stringify(attrs.ui_settings) - : attrs.ui_settings; - return api.put( `/users/${id}`, { @@ -79,7 +72,7 @@ async function updateUser({ enabled: attrs.enabled, role: attrs.role, ra_username: attrs.ra_username, - ui_settings: uiSettings, + ui_settings: attrs.ui_settings, }, { headers: { diff --git a/frontend/src/stores/galleryFilter.ts b/frontend/src/stores/galleryFilter.ts index d198c82a3..0018f1258 100644 --- a/frontend/src/stores/galleryFilter.ts +++ b/frontend/src/stores/galleryFilter.ts @@ -1,8 +1,6 @@ import { defineStore } from "pinia"; -import type { PlatformSchema } from "@/__generated__"; import { romStatusMap } from "@/utils"; - -export type Platform = PlatformSchema; +import type { Platform } from "./platforms"; export type FilterType = | "genres"