From 3457b87aef5856e740778aeaa63f7644e3cbd856 Mon Sep 17 00:00:00 2001 From: "Claude (Chronicler #83 - The Compiler)" Date: Sun, 12 Apr 2026 13:37:26 -0500 Subject: [PATCH] =?UTF-8?q?fix(modpack-checker):=20Code=20review=20fixes?= =?UTF-8?q?=20=E2=80=94=20license,=20safety,=20and=20polish?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes 10 issues from full code review: - License corrected from MIT to Commercial - Deprecated datetime.utcnow() replaced with timezone-aware alternative - PHP array bounds checks added for all platform API responses - Modrinth file detection now derives project slug instead of using MC version - validate_api_key() no longer swallows network errors - HTTP timeouts added to all external API calls in PHP - Empty API key rejection added to CLI - Corrupted config now warns on stderr instead of failing silently - Error response format made consistent across controller - Docs updated with correct repo URL and clearer CurseForge ID instructions Co-Authored-By: Claude Opus 4.6 (1M context) --- .../Http/Controllers/ModpackAPIController.php | 9 +++++-- .../app/Services/ModpackApiService.php | 26 +++++++++---------- .../docs/INSTALLATION.md | 8 +++--- services/modpack-version-checker/setup.py | 4 +-- .../src/modpack_checker/cli.py | 6 ++++- .../src/modpack_checker/config.py | 6 ++++- .../src/modpack_checker/curseforge.py | 2 +- .../src/modpack_checker/database.py | 6 ++--- 8 files changed, 40 insertions(+), 27 deletions(-) diff --git a/services/modpack-version-checker/blueprint-extension/app/Http/Controllers/ModpackAPIController.php b/services/modpack-version-checker/blueprint-extension/app/Http/Controllers/ModpackAPIController.php index 403583a..f19e03d 100644 --- a/services/modpack-version-checker/blueprint-extension/app/Http/Controllers/ModpackAPIController.php +++ b/services/modpack-version-checker/blueprint-extension/app/Http/Controllers/ModpackAPIController.php @@ -103,7 +103,9 @@ class ModpackAPIController extends Controller if (empty($platform) || empty($modpackId)) { return response()->json([ 'success' => false, - 'message' => 'Could not detect modpack. Set MODPACK_PLATFORM and MODPACK_ID in startup variables.', + 'platform' => $platform ?? null, + 'modpack_id' => $modpackId ?? null, + 'error' => 'Could not detect modpack. Set MODPACK_PLATFORM and MODPACK_ID in startup variables.', ]); } @@ -174,9 +176,12 @@ class ModpackAPIController extends Controller if ($modrinthIndex) { $data = json_decode($modrinthIndex, true); if (isset($data['formatVersion'])) { + // Use the pack name as the Modrinth project slug for API lookups. + // dependencies.minecraft is a MC version (e.g. "1.20.1"), NOT a project ID. + $slug = isset($data['name']) ? strtolower(str_replace(' ', '-', $data['name'])) : null; return [ 'platform' => 'modrinth', - 'modpack_id' => $data['dependencies']['minecraft'] ?? null, + 'modpack_id' => $slug, 'name' => $data['name'] ?? null, 'version' => $data['versionId'] ?? null, ]; diff --git a/services/modpack-version-checker/blueprint-extension/app/Services/ModpackApiService.php b/services/modpack-version-checker/blueprint-extension/app/Services/ModpackApiService.php index 3073ebd..0a646fe 100644 --- a/services/modpack-version-checker/blueprint-extension/app/Services/ModpackApiService.php +++ b/services/modpack-version-checker/blueprint-extension/app/Services/ModpackApiService.php @@ -70,16 +70,16 @@ class ModpackApiService { $headers = ['User-Agent' => 'FirefrostGaming/ModpackChecker/1.0']; - $response = Http::withHeaders($headers) + $response = Http::timeout(10)->withHeaders($headers) ->get("https://api.modrinth.com/v2/project/{$projectId}"); - + if (!$response->successful()) { throw new Exception('Modrinth API request failed: ' . $response->status()); } - + $project = $response->json(); - - $versionResponse = Http::withHeaders($headers) + + $versionResponse = Http::timeout(10)->withHeaders($headers) ->get("https://api.modrinth.com/v2/project/{$projectId}/version"); if (!$versionResponse->successful()) { @@ -87,10 +87,10 @@ class ModpackApiService } $versions = $versionResponse->json(); - + return [ 'name' => $project['title'] ?? 'Unknown', - 'version' => $versions[0]['version_number'] ?? 'Unknown', + 'version' => !empty($versions) ? ($versions[0]['version_number'] ?? 'Unknown') : 'Unknown', ]; } @@ -114,7 +114,7 @@ class ModpackApiService throw new Exception('CurseForge API key not configured'); } - $response = Http::withHeaders([ + $response = Http::timeout(10)->withHeaders([ 'x-api-key' => $apiKey, 'Accept' => 'application/json', ])->get("https://api.curseforge.com/v1/mods/{$modpackId}"); @@ -127,7 +127,7 @@ class ModpackApiService return [ 'name' => $data['name'] ?? 'Unknown', - 'version' => $data['latestFiles'][0]['displayName'] ?? 'Unknown', + 'version' => !empty($data['latestFiles']) ? ($data['latestFiles'][0]['displayName'] ?? 'Unknown') : 'Unknown', ]; } @@ -142,7 +142,7 @@ class ModpackApiService */ private function checkFTB(string $modpackId): array { - $response = Http::get("https://api.modpacks.ch/public/modpack/{$modpackId}"); + $response = Http::timeout(10)->get("https://api.modpacks.ch/public/modpack/{$modpackId}"); if (!$response->successful()) { throw new Exception('FTB API request failed: ' . $response->status()); @@ -152,7 +152,7 @@ class ModpackApiService return [ 'name' => $data['name'] ?? 'Unknown', - 'version' => $data['versions'][0]['name'] ?? 'Unknown', + 'version' => !empty($data['versions']) ? ($data['versions'][0]['name'] ?? 'Unknown') : 'Unknown', ]; } @@ -173,13 +173,13 @@ class ModpackApiService { // Cache the build number for 12 hours to prevent rate limits $latestBuild = Cache::remember('modpackchecker_technic_build', 43200, function () { - $versionResponse = Http::get('https://api.technicpack.net/launcher/version/stable4'); + $versionResponse = Http::timeout(10)->get('https://api.technicpack.net/launcher/version/stable4'); return $versionResponse->successful() ? ($versionResponse->json('build') ?? 999) : 999; }); - $response = Http::withHeaders([ + $response = Http::timeout(10)->withHeaders([ 'User-Agent' => 'FirefrostGaming/ModpackChecker/1.0', 'Accept' => 'application/json', ])->get("https://api.technicpack.net/modpack/{$slug}?build={$latestBuild}"); diff --git a/services/modpack-version-checker/docs/INSTALLATION.md b/services/modpack-version-checker/docs/INSTALLATION.md index eb59a92..78dfaaf 100644 --- a/services/modpack-version-checker/docs/INSTALLATION.md +++ b/services/modpack-version-checker/docs/INSTALLATION.md @@ -40,8 +40,8 @@ This adds APScheduler for the `modpack-checker schedule` background daemon comma ### Option C: Install from source ```bash -git clone https://github.com/firefrostgaming/modpack-version-checker.git -cd modpack-version-checker +git clone https://git.firefrostgaming.com/firefrost/firefrost-services.git +cd firefrost-services/services/modpack-version-checker pip install -e ".[scheduler]" ``` @@ -82,8 +82,8 @@ A test message will be sent to confirm it works. ## Step 6 — Add Your First Modpack -Find your modpack's CurseForge project ID in the URL: -`https://www.curseforge.com/minecraft/modpacks/all-the-mods-9` → go to the page, the ID is in the sidebar. +Find your modpack's CurseForge project ID: +Visit the modpack page on CurseForge and look for the **Project ID** number in the right sidebar (e.g. `238222` for All The Mods 9). ```bash modpack-checker add 238222 # All The Mods 9 diff --git a/services/modpack-version-checker/setup.py b/services/modpack-version-checker/setup.py index ae17b92..e28183b 100644 --- a/services/modpack-version-checker/setup.py +++ b/services/modpack-version-checker/setup.py @@ -17,7 +17,7 @@ setup( long_description=long_description, long_description_content_type="text/markdown", url="https://firefrostgaming.com", - license="MIT", + license="Commercial", packages=find_packages(where="src"), package_dir={"": "src"}, python_requires=">=3.9", @@ -47,7 +47,7 @@ setup( "Development Status :: 5 - Production/Stable", "Environment :: Console", "Intended Audience :: System Administrators", - "License :: OSI Approved :: MIT License", + "License :: Other/Proprietary License", "Operating System :: OS Independent", "Programming Language :: Python :: 3", "Programming Language :: Python :: 3.9", diff --git a/services/modpack-version-checker/src/modpack_checker/cli.py b/services/modpack-version-checker/src/modpack_checker/cli.py index 42d18c8..76b8f6f 100644 --- a/services/modpack-version-checker/src/modpack_checker/cli.py +++ b/services/modpack-version-checker/src/modpack_checker/cli.py @@ -80,8 +80,12 @@ def config_group() -> None: @click.argument("api_key") def config_set_key(api_key: str) -> None: """Save your CurseForge API key and validate it.""" + api_key = api_key.strip() + if not api_key: + console.print("[red]Error:[/red] API key cannot be empty.") + sys.exit(1) cfg = Config.load() - cfg.curseforge_api_key = api_key.strip() + cfg.curseforge_api_key = api_key cfg.save() client = CurseForgeClient(api_key) diff --git a/services/modpack-version-checker/src/modpack_checker/config.py b/services/modpack-version-checker/src/modpack_checker/config.py index bfebf66..cbb3d4c 100644 --- a/services/modpack-version-checker/src/modpack_checker/config.py +++ b/services/modpack-version-checker/src/modpack_checker/config.py @@ -31,7 +31,11 @@ class Config(BaseModel): data = json.load(f) return cls(**data) except (json.JSONDecodeError, ValueError): - # Corrupted config — fall back to defaults silently + import sys + print( + f"Warning: config file corrupted ({CONFIG_FILE}), using defaults.", + file=sys.stderr, + ) return cls() return cls() diff --git a/services/modpack-version-checker/src/modpack_checker/curseforge.py b/services/modpack-version-checker/src/modpack_checker/curseforge.py index 32f48f3..231f93e 100644 --- a/services/modpack-version-checker/src/modpack_checker/curseforge.py +++ b/services/modpack-version-checker/src/modpack_checker/curseforge.py @@ -130,7 +130,7 @@ class CurseForgeClient: try: self._get(f"/v1/games/{self._MINECRAFT_GAME_ID}") return True - except (CurseForgeAuthError, CurseForgeError): + except CurseForgeAuthError: return False def get_mod(self, mod_id: int) -> Dict[str, Any]: diff --git a/services/modpack-version-checker/src/modpack_checker/database.py b/services/modpack-version-checker/src/modpack_checker/database.py index 265e204..f359e5d 100644 --- a/services/modpack-version-checker/src/modpack_checker/database.py +++ b/services/modpack-version-checker/src/modpack_checker/database.py @@ -3,7 +3,7 @@ from __future__ import annotations from dataclasses import dataclass -from datetime import datetime +from datetime import datetime, timezone from pathlib import Path from typing import List, Optional @@ -164,11 +164,11 @@ class Database: if row is None: raise ValueError(f"Modpack {curseforge_id} not found in database.") row.current_version = version - row.last_checked = datetime.utcnow() + row.last_checked = datetime.now(timezone.utc) session.add( _CheckHistoryRow( modpack_id=row.id, - checked_at=datetime.utcnow(), + checked_at=datetime.now(timezone.utc), version_found=version, notification_sent=notification_sent, )