fix(modpack-checker): Code review fixes — license, safety, and polish
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) <noreply@anthropic.com>
This commit is contained in:
parent
c6d40dcf39
commit
3457b87aef
@@ -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,
|
||||
];
|
||||
|
||||
@@ -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}");
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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",
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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()
|
||||
|
||||
|
||||
@@ -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]:
|
||||
|
||||
@@ -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,
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user