fix: harden downloads, settings, and UI edge cases
- Integrate DownloadRegistry into concurrent download limit and clean up timers on removal/clear - Wrap MetadataSyncService batch deletes in single transaction - Always re-throw checksum errors in RuffleService to prevent verification bypass - Fix system-settings early return skipping second job update when first cron expression is invalid - Increase DatabaseService hot-reload grace period to 30s - Add permission guard to metadata sync status endpoint - Use encodeURIComponent for game launch content URLs - Clamp limit params to minimum of 1 in jobs routes - Use non-null assertion for authenticated user permissions - Fix ConfirmDialog not calling onCancel on overlay dismiss - Guard FormattedDate against invalid date strings - Add breadcrumbContext to GameListItem memo comparison - Fix MetadataUpdateCard stale closure in interval callback - Move Dockerfile healthcheck to docker-compose for flexibility - Fix build:analyze script to set VITE_ANALYZE env var
This commit is contained in:
Darkraise
@@ -118,9 +118,10 @@ export class GameZipServer {
|
||||
return { success: true, downloading: true, statusCode: 202 };
|
||||
}
|
||||
|
||||
if (downloadsInProgress.size >= MAX_CONCURRENT_DOWNLOADS) {
|
||||
const totalActive = downloadsInProgress.size + DownloadRegistry.getActiveCount();
|
||||
if (totalActive >= MAX_CONCURRENT_DOWNLOADS) {
|
||||
logger.warn(
|
||||
`[GameZipServer] Too many concurrent downloads (${downloadsInProgress.size}), rejecting ${gameId}`
|
||||
`[GameZipServer] Too many concurrent downloads (${totalActive}), rejecting ${gameId}`
|
||||
);
|
||||
return { success: false, statusCode: 503 };
|
||||
}
|
||||
@@ -406,6 +407,7 @@ export class GameZipServer {
|
||||
for (const [gameId, progress] of downloadsInProgress.entries()) {
|
||||
if (now - progress.startTime > DOWNLOAD_STALE_MS) {
|
||||
logger.warn(`[GameZipServer] Removing stale download for ${gameId}`);
|
||||
DownloadRegistry.fail(gameId);
|
||||
downloadsInProgress.delete(gameId);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -230,7 +230,7 @@ router.get(
|
||||
}
|
||||
}
|
||||
|
||||
contentUrl = `${proxyUrl}/${encodeURI(fullUrl)}`;
|
||||
contentUrl = `${proxyUrl}/${encodeURIComponent(fullUrl)}`;
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -34,7 +34,7 @@ router.get(
|
||||
asyncHandler(async (req: Request, res: Response) => {
|
||||
try {
|
||||
const rawLimit = parseInt(req.query.limit as string, 10);
|
||||
const limit = Math.min(isNaN(rawLimit) ? 100 : rawLimit, 200);
|
||||
const limit = Math.min(Math.max(1, isNaN(rawLimit) ? 100 : rawLimit), 200);
|
||||
const rawOffset = parseInt(req.query.offset as string, 10);
|
||||
const offset = Math.max(0, isNaN(rawOffset) ? 0 : rawOffset);
|
||||
|
||||
@@ -212,7 +212,7 @@ router.get(
|
||||
}
|
||||
|
||||
const rawLimit = parseInt(req.query.limit as string, 10);
|
||||
const limit = Math.min(isNaN(rawLimit) ? 50 : rawLimit, 100);
|
||||
const limit = Math.min(Math.max(1, isNaN(rawLimit) ? 50 : rawLimit), 100);
|
||||
const rawOffset = parseInt(req.query.offset as string, 10);
|
||||
const offset = Math.max(0, isNaN(rawOffset) ? 0 : rawOffset);
|
||||
|
||||
|
||||
@@ -80,7 +80,7 @@ router.post(
|
||||
if (data.permissionIds && data.permissionIds.length > 0) {
|
||||
await roleService.validatePermissionEscalation(
|
||||
data.permissionIds,
|
||||
req.user?.permissions || []
|
||||
req.user!.permissions ?? []
|
||||
);
|
||||
}
|
||||
|
||||
@@ -127,7 +127,7 @@ router.put(
|
||||
|
||||
const data = updatePermissionsSchema.parse(req.body);
|
||||
|
||||
await roleService.validatePermissionEscalation(data.permissionIds, req.user?.permissions || []);
|
||||
await roleService.validatePermissionEscalation(data.permissionIds, req.user!.permissions ?? []);
|
||||
|
||||
await roleService.updateRolePermissions(id, data.permissionIds);
|
||||
const role = await roleService.getRoleById(id);
|
||||
|
||||
@@ -29,14 +29,13 @@ function updateJobScheduler(category: string, settings: CategorySettings): void
|
||||
'0 * * * *') as string;
|
||||
|
||||
if (cronSchedule && !cron.validate(cronSchedule)) {
|
||||
logger.warn(`[Settings] Invalid cron expression: ${cronSchedule}`);
|
||||
return;
|
||||
logger.warn(`[Settings] Invalid metadata-sync cron expression: ${cronSchedule}`);
|
||||
} else {
|
||||
JobScheduler.updateJob('metadata-sync', {
|
||||
enabled,
|
||||
cronSchedule,
|
||||
});
|
||||
}
|
||||
|
||||
JobScheduler.updateJob('metadata-sync', {
|
||||
enabled,
|
||||
cronSchedule,
|
||||
});
|
||||
}
|
||||
|
||||
if ('ruffleUpdateEnabled' in settings || 'ruffleUpdateSchedule' in settings) {
|
||||
@@ -49,14 +48,13 @@ function updateJobScheduler(category: string, settings: CategorySettings): void
|
||||
'0 0 * * *') as string;
|
||||
|
||||
if (cronSchedule && !cron.validate(cronSchedule)) {
|
||||
logger.warn(`[Settings] Invalid cron expression: ${cronSchedule}`);
|
||||
return;
|
||||
logger.warn(`[Settings] Invalid ruffle-update cron expression: ${cronSchedule}`);
|
||||
} else {
|
||||
JobScheduler.updateJob('ruffle-update', {
|
||||
enabled,
|
||||
cronSchedule,
|
||||
});
|
||||
}
|
||||
|
||||
JobScheduler.updateJob('ruffle-update', {
|
||||
enabled,
|
||||
cronSchedule,
|
||||
});
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -98,6 +98,7 @@ router.post(
|
||||
router.get(
|
||||
'/metadata/sync/status',
|
||||
authenticate,
|
||||
requirePermission('settings.read'),
|
||||
asyncHandler(async (req, res) => {
|
||||
const syncStatus = SyncStatusService.getInstance();
|
||||
const status = syncStatus.getStatus();
|
||||
|
||||
@@ -328,8 +328,9 @@ export class DatabaseService {
|
||||
const oldDb = this.db;
|
||||
this.db = newDb;
|
||||
|
||||
// Close old connection after grace period (5s)
|
||||
// Allows in-flight queries that captured the old reference to complete
|
||||
// Close old connection after grace period (30s)
|
||||
// Allows in-flight queries that captured the old reference to complete.
|
||||
// 30s accommodates slow queries; the server request timeout is 120s.
|
||||
if (oldDb) {
|
||||
setTimeout(() => {
|
||||
try {
|
||||
@@ -338,7 +339,7 @@ export class DatabaseService {
|
||||
} catch (closeError) {
|
||||
logger.warn('Failed to close old database handle:', closeError);
|
||||
}
|
||||
}, 5000).unref(); // .unref() so it doesn't prevent shutdown
|
||||
}, 30000).unref(); // .unref() so it doesn't prevent shutdown
|
||||
}
|
||||
|
||||
// Update modification time
|
||||
@@ -397,8 +398,9 @@ export class DatabaseService {
|
||||
const oldDb = this.db;
|
||||
this.db = newDb;
|
||||
|
||||
// Close old connection after grace period (5s)
|
||||
// Allows in-flight queries that captured the old reference to complete
|
||||
// Close old connection after grace period (30s)
|
||||
// Allows in-flight queries that captured the old reference to complete.
|
||||
// 30s accommodates slow queries; the server request timeout is 120s.
|
||||
if (oldDb) {
|
||||
setTimeout(() => {
|
||||
try {
|
||||
@@ -407,7 +409,7 @@ export class DatabaseService {
|
||||
} catch (closeError) {
|
||||
logger.warn('Failed to close old database handle:', closeError);
|
||||
}
|
||||
}, 5000).unref(); // .unref() so it doesn't prevent shutdown
|
||||
}, 30000).unref(); // .unref() so it doesn't prevent shutdown
|
||||
}
|
||||
|
||||
// Update modification time
|
||||
|
||||
@@ -18,6 +18,7 @@ export interface DownloadEntry {
|
||||
*/
|
||||
export class DownloadRegistry {
|
||||
private static activeDownloads = new Map<string, DownloadEntry>();
|
||||
private static pendingTimers = new Map<string, ReturnType<typeof setTimeout>>();
|
||||
|
||||
/**
|
||||
* Register a download. Returns false if already downloading (duplicate prevention).
|
||||
@@ -31,6 +32,8 @@ export class DownloadRegistry {
|
||||
logger.debug(`[DownloadRegistry] Download already in progress for ${gameId}`);
|
||||
return false;
|
||||
}
|
||||
// Cancel any pending cleanup timer from a previous download of the same game
|
||||
this.cancelTimer(gameId);
|
||||
this.activeDownloads.set(gameId, {
|
||||
...entry,
|
||||
startedAt: Date.now(),
|
||||
@@ -53,9 +56,7 @@ export class DownloadRegistry {
|
||||
entry.status = 'completed';
|
||||
logger.debug(`[DownloadRegistry] Download completed: ${gameId}`);
|
||||
// Remove after short delay to allow SSE clients to see completion status
|
||||
setTimeout(() => {
|
||||
this.activeDownloads.delete(gameId);
|
||||
}, 5000);
|
||||
this.scheduleRemoval(gameId, entry.startedAt);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -70,9 +71,36 @@ export class DownloadRegistry {
|
||||
entry.status = 'failed';
|
||||
logger.debug(`[DownloadRegistry] Download failed: ${gameId}`);
|
||||
// Remove after short delay to allow SSE clients to see failure status
|
||||
setTimeout(() => {
|
||||
this.scheduleRemoval(gameId, entry.startedAt);
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Schedule removal of an entry after a delay, verifying entry identity
|
||||
* to prevent deleting a newer entry for the same gameId.
|
||||
*/
|
||||
private static scheduleRemoval(gameId: string, startedAt: number): void {
|
||||
this.cancelTimer(gameId);
|
||||
const timer = setTimeout(() => {
|
||||
this.pendingTimers.delete(gameId);
|
||||
const current = this.activeDownloads.get(gameId);
|
||||
// Only delete if the entry is the same one we marked (not a newer download)
|
||||
if (current && current.startedAt === startedAt) {
|
||||
this.activeDownloads.delete(gameId);
|
||||
}, 5000);
|
||||
}
|
||||
}, 5000);
|
||||
timer.unref();
|
||||
this.pendingTimers.set(gameId, timer);
|
||||
}
|
||||
|
||||
/**
|
||||
* Cancel a pending cleanup timer for a gameId.
|
||||
*/
|
||||
private static cancelTimer(gameId: string): void {
|
||||
const existing = this.pendingTimers.get(gameId);
|
||||
if (existing) {
|
||||
clearTimeout(existing);
|
||||
this.pendingTimers.delete(gameId);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -138,9 +166,13 @@ export class DownloadRegistry {
|
||||
}
|
||||
|
||||
/**
|
||||
* Clear all entries (for testing/cleanup).
|
||||
* Clear all entries and cancel pending timers (for testing/cleanup).
|
||||
*/
|
||||
static clear(): void {
|
||||
for (const timer of this.pendingTimers.values()) {
|
||||
clearTimeout(timer);
|
||||
}
|
||||
this.pendingTimers.clear();
|
||||
this.activeDownloads.clear();
|
||||
logger.debug('[DownloadRegistry] Cleared all entries');
|
||||
}
|
||||
|
||||
@@ -647,21 +647,22 @@ export class MetadataSyncService {
|
||||
if (!Array.isArray(gameIds) || gameIds.length === 0) return;
|
||||
const db = DatabaseService.getDatabase();
|
||||
|
||||
// Process in batches to avoid SQL variable limits
|
||||
// Wrap all batches in a single transaction to maintain consistency.
|
||||
// If the process crashes mid-way, all changes are rolled back.
|
||||
const BATCH_SIZE = 500;
|
||||
for (let i = 0; i < gameIds.length; i += BATCH_SIZE) {
|
||||
const batch = gameIds.slice(i, i + BATCH_SIZE);
|
||||
const placeholders = batch.map(() => '?').join(', ');
|
||||
db.transaction(() => {
|
||||
for (let i = 0; i < gameIds.length; i += BATCH_SIZE) {
|
||||
const batch = gameIds.slice(i, i + BATCH_SIZE);
|
||||
const placeholders = batch.map(() => '?').join(', ');
|
||||
|
||||
db.transaction(() => {
|
||||
// Orphan children first (prevents FK violation)
|
||||
db.prepare(
|
||||
`UPDATE game SET parentGameId = NULL WHERE parentGameId IN (${placeholders})`
|
||||
).run(batch);
|
||||
// Then delete
|
||||
db.prepare(`DELETE FROM game WHERE id IN (${placeholders})`).run(batch);
|
||||
})();
|
||||
}
|
||||
}
|
||||
})();
|
||||
}
|
||||
|
||||
private async updatePreferencesTimestamps(
|
||||
|
||||
@@ -125,7 +125,9 @@ export class RuffleService {
|
||||
const normalizedCurrent = currentVersion ? this.normalizeVersion(currentVersion) : null;
|
||||
const normalizedLatest = this.normalizeVersion(latest.version);
|
||||
|
||||
const updateAvailable = normalizedCurrent === null || normalizedCurrent !== normalizedLatest;
|
||||
// Only flag as "update available" when Ruffle is already installed but outdated.
|
||||
// When not installed (null), the server auto-installs at startup — no need to show update UI.
|
||||
const updateAvailable = normalizedCurrent !== null && normalizedCurrent !== normalizedLatest;
|
||||
|
||||
return {
|
||||
currentVersion,
|
||||
@@ -210,14 +212,13 @@ export class RuffleService {
|
||||
|
||||
logger.info('[RuffleService] SHA-256 checksum verification passed');
|
||||
} catch (error) {
|
||||
if (error instanceof Error && error.message.includes('checksum mismatch')) {
|
||||
// Re-throw checksum failures
|
||||
throw error;
|
||||
}
|
||||
|
||||
// Log but don't fail on other errors (e.g., network issues downloading checksum)
|
||||
logger.error('[RuffleService] Error during checksum verification:', error);
|
||||
logger.warn('[RuffleService] Continuing without checksum verification');
|
||||
// Always re-throw when a checksum URL was explicitly provided —
|
||||
// skipping verification only when no checksum file exists in the release.
|
||||
// This prevents an attacker who can block the checksum URL from
|
||||
// forcing verification bypass while the tampered ZIP goes through.
|
||||
logger.error('[RuffleService] Checksum verification failed:', error);
|
||||
const message = error instanceof Error ? error.message : 'Unknown checksum error';
|
||||
throw new Error(`Checksum verification failed: ${message}`);
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
+2
-3
@@ -62,9 +62,8 @@ RUN chown -R nginx:nginx /usr/share/nginx/html && \
|
||||
# Expose port (8080 for non-root user)
|
||||
EXPOSE 8080
|
||||
|
||||
# Health check (using wget which is built into Alpine)
|
||||
HEALTHCHECK --interval=30s --timeout=3s --start-period=10s --retries=3 \
|
||||
CMD wget --quiet --spider http://localhost:8080/ || exit 1
|
||||
# Health check is configured in docker-compose.yml for flexibility.
|
||||
# When running standalone (docker run), add --health-cmd manually.
|
||||
|
||||
# Run as non-root user (nginx user from base image)
|
||||
USER nginx
|
||||
|
||||
@@ -6,7 +6,7 @@
|
||||
"scripts": {
|
||||
"dev": "vite",
|
||||
"build": "tsc && vite build",
|
||||
"build:analyze": "tsc && vite build",
|
||||
"build:analyze": "VITE_ANALYZE=true tsc && vite build",
|
||||
"preview": "vite preview",
|
||||
"test": "vitest run",
|
||||
"test:watch": "vitest",
|
||||
@@ -16,7 +16,7 @@
|
||||
"typecheck": "tsc --noEmit",
|
||||
"format": "prettier --write \"src/**/*.{ts,tsx,js,jsx,css,json}\"",
|
||||
"format:check": "prettier --check \"src/**/*.{ts,tsx,js,jsx,css,json}\"",
|
||||
"check-bundle": "vite build --mode analyze"
|
||||
"check-bundle": "VITE_ANALYZE=true vite build"
|
||||
},
|
||||
"dependencies": {
|
||||
"@hookform/resolvers": "^5.2.2",
|
||||
|
||||
@@ -43,6 +43,9 @@ export function ConfirmDialog({
|
||||
const variantNormalized = variant === 'danger' ? 'destructive' : variant;
|
||||
|
||||
const handleOpenChange = (newOpen: boolean) => {
|
||||
if (!newOpen && onCancel) {
|
||||
onCancel();
|
||||
}
|
||||
if (onOpenChange) {
|
||||
onOpenChange(newOpen);
|
||||
}
|
||||
|
||||
@@ -12,14 +12,18 @@ export function FormattedDate({ date, type = 'datetime', className }: FormattedD
|
||||
const getFormattedValue = () => {
|
||||
if (!date) return 'Unknown';
|
||||
|
||||
switch (type) {
|
||||
case 'date':
|
||||
return formatDate(date);
|
||||
case 'time':
|
||||
return formatTime(date);
|
||||
case 'datetime':
|
||||
default:
|
||||
return formatDateTime(date);
|
||||
try {
|
||||
switch (type) {
|
||||
case 'date':
|
||||
return formatDate(date);
|
||||
case 'time':
|
||||
return formatTime(date);
|
||||
case 'datetime':
|
||||
default:
|
||||
return formatDateTime(date);
|
||||
}
|
||||
} catch {
|
||||
return String(date);
|
||||
}
|
||||
};
|
||||
|
||||
|
||||
@@ -194,6 +194,8 @@ export const GameListItem = memo(
|
||||
prevProps.showAddToPlaylistButton === nextProps.showAddToPlaylistButton &&
|
||||
prevProps.isFavoritePage === nextProps.isFavoritePage &&
|
||||
prevProps.shareToken === nextProps.shareToken &&
|
||||
prevProps.breadcrumbContext?.label === nextProps.breadcrumbContext?.label &&
|
||||
prevProps.breadcrumbContext?.href === nextProps.breadcrumbContext?.href &&
|
||||
(prevProps.favoriteGameIds?.has(prevProps.game.id) ?? false) ===
|
||||
(nextProps.favoriteGameIds?.has(nextProps.game.id) ?? false)
|
||||
);
|
||||
|
||||
@@ -17,6 +17,8 @@ export function MetadataUpdateCard() {
|
||||
const [syncMessage, setSyncMessage] = useState('');
|
||||
const [metadataInfo, setMetadataInfo] = useState<MetadataUpdateInfo | null>(null);
|
||||
const [error, setError] = useState<string | null>(null);
|
||||
// Use ref for the fetch guard to avoid stale closure issues in interval callbacks
|
||||
const isFetchingRef = useRef(false);
|
||||
const [isFetchingMetadata, setIsFetchingMetadata] = useState(false);
|
||||
|
||||
// Use ref to track poll interval (avoids stale closure issues)
|
||||
@@ -24,10 +26,11 @@ export function MetadataUpdateCard() {
|
||||
const timeoutRef = useRef<NodeJS.Timeout | null>(null);
|
||||
|
||||
const fetchMetadataInfo = async () => {
|
||||
if (isFetchingMetadata) {
|
||||
if (isFetchingRef.current) {
|
||||
return;
|
||||
}
|
||||
|
||||
isFetchingRef.current = true;
|
||||
setIsFetchingMetadata(true);
|
||||
|
||||
try {
|
||||
@@ -42,6 +45,7 @@ export function MetadataUpdateCard() {
|
||||
});
|
||||
setError('Failed to check metadata updates. Please try again later.');
|
||||
} finally {
|
||||
isFetchingRef.current = false;
|
||||
setIsFetchingMetadata(false);
|
||||
}
|
||||
};
|
||||
@@ -51,7 +55,7 @@ export function MetadataUpdateCard() {
|
||||
});
|
||||
|
||||
const checkMetadataUpdates = async () => {
|
||||
setIsFetchingMetadata(false);
|
||||
isFetchingRef.current = false;
|
||||
await fetchMetadataInfo();
|
||||
};
|
||||
|
||||
|
||||
@@ -6,9 +6,9 @@ import { visualizer } from 'rollup-plugin-visualizer';
|
||||
export default defineConfig({
|
||||
plugins: [
|
||||
react(),
|
||||
// Bundle analyzer - run with: npm run build && open stats.html
|
||||
// Bundle analyzer - run with: npm run build:analyze && open stats.html
|
||||
visualizer({
|
||||
open: false,
|
||||
open: !!process.env.VITE_ANALYZE,
|
||||
filename: 'stats.html',
|
||||
gzipSize: true,
|
||||
brotliSize: true,
|
||||
|
||||
Reference in New Issue
Block a user