fix: address review findings from the two-day changes

Critical: DbQueryPool stranded the caller of any query whose worker died —
pending promises were only rejected when the last worker went, so a single
crash left an HTTP request hanging until the timeout. Track the in-flight
query per slot, reject exactly that caller, and settle queued work when the
pool empties or shuts down.

Security: same-origin acceptance compared host but ignored scheme, so a
plaintext page could claim same-origin against an HTTPS deployment; the
metadata source allowlist accepted http://, where a MITM could rewrite the
game database; and asset paths were built from remote game IDs without
validation, allowing traversal outside the images directory.

Also: the reconciler now derives filenames via GameDataDownloader.getFilename
instead of re-parsing dateAdded (formats that helper normalizes would never
have matched), and warns once instead of failing when the database is
read-only — the state of the shipped compose mount. getYearRange runs through
the worker pool like its siblings rather than blocking the main thread.

backend/src/routes/game-zip.ts

@@ -28,6 +28,7 @@ const adminCors = (req: Request, res: Response, next: NextFunction) => {
     isOriginAllowed({
       origin,
       host: req.headers.host,
+      protocol: req.protocol,
       configuredOrigins: config.allowedOrigins,
       domainOrigins: new Set<string>(),
     });

backend/src/server.ts

@@ -169,6 +169,7 @@ async function startServer() {
       const allowed = isOriginAllowed({
         origin,
         host: req.headers.host,
+        protocol: req.protocol,
         configuredOrigins: config.allowedOrigins,
         domainOrigins,
       });

backend/src/services/AssetDownloadService.ts

@@ -16,6 +16,7 @@ export interface AssetDownloadProgress {
   cancelled: boolean;
 }
 
+const UUID_PATTERN = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i;
 const CONCURRENCY = 4;
 const REQUEST_TIMEOUT_MS = 15000;
 const MAX_IMAGE_BYTES = 10 * 1024 * 1024;
@@ -59,8 +60,19 @@ export class AssetDownloadService {
     }
   }
 
-  /** Images live at Data/Images/{Logos,Screenshots}/ab/cd/{id}.png. */
+  /**
+   * Images live at Data/Images/{Logos,Screenshots}/ab/cd/{id}.png.
+   *
+   * Game IDs arrive from a remote API and are sliced straight into filesystem
+   * paths, so anything that is not a UUID is rejected rather than joined — an id
+   * containing path separators would otherwise escape the images directory.
+   */
   static relativePathsFor(gameId: string): string[] {
+    if (!UUID_PATTERN.test(gameId)) {
+      logger.warn(`[AssetDownload] Ignoring malformed game id: ${gameId}`);
+      return [];
+    }
+
     const shard = `${gameId.substring(0, 2)}/${gameId.substring(2, 4)}`;
     return [`Logos/${shard}/${gameId}.png`, `Screenshots/${shard}/${gameId}.png`];
   }

backend/src/services/DbQueryPool.test.ts

@@ -54,6 +54,34 @@ describe('DbQueryPool', () => {
     expect(results.every((rows) => rows[0].count === 3)).toBe(true);
   });
 
+  it('rejects the in-flight query when its worker dies, instead of hanging', async () => {
+    // Kill one worker while the pool still has others: the caller whose query it
+    // was running must be rejected, not left waiting for a result that can never
+    // arrive. A hung promise here means a hung HTTP request in production.
+    const pool = DbQueryPool as unknown as {
+      slots: Array<{
+        worker: { terminate: () => Promise<number> };
+        busy: boolean;
+        queryId: number | null;
+      }>;
+      pending: Map<number, { reject: (e: Error) => void }>;
+      drain: () => void;
+    };
+
+    const victim = pool.slots[0];
+    victim.busy = true;
+    victim.queryId = 999_999;
+
+    const stranded = new Promise<void>((resolve, reject) => {
+      pool.pending.set(999_999, { reject: (error: Error) => reject(error) } as never);
+      setTimeout(resolve, 3000);
+    });
+
+    await victim.worker.terminate();
+
+    await expect(stranded).rejects.toThrow(/stopped while running/);
+  });
+
   it('rejects a bad statement without killing the worker', async () => {
     await expect(DbQueryPool.all('SELECT * FROM does_not_exist')).rejects.toThrow();
 

backend/src/services/DbQueryPool.ts

@@ -12,6 +12,8 @@ interface PendingQuery {
 interface WorkerSlot {
   worker: Worker;
   busy: boolean;
+  /** Query this worker is executing, so a crash can reject exactly that caller. */
+  queryId: number | null;
 }
 
 interface QueuedQuery {
@@ -59,7 +61,7 @@ export class DbQueryPool {
 
     for (let i = 0; i < count; i += 1) {
       try {
-        this.slots.push({ worker: this.spawn(workerPath, dbPath), busy: false });
+        this.slots.push({ worker: this.spawn(workerPath, dbPath), busy: false, queryId: null });
       } catch (error: unknown) {
         logger.warn('[DbQueryPool] Failed to start worker; continuing with fewer:', error);
       }
@@ -100,6 +102,7 @@ export class DbQueryPool {
         const slot = this.slots.find((s) => s.worker === worker);
         if (slot) {
           slot.busy = false;
+          slot.queryId = null;
         }
 
         const waiting = this.pending.get(message.id);
@@ -133,21 +136,47 @@ export class DbQueryPool {
     return worker;
   }
 
-  /** A dead worker must not strand the queries it was holding. */
+  /**
+   * A dead worker must not strand the query it was holding.
+   *
+   * Rejecting only when the pool empties leaves the crashed worker's caller
+   * waiting forever — the request hangs until the timeout middleware kills it and
+   * the pending entry leaks. Reject that query specifically, and when no worker
+   * is left, reject everything still queued as well.
+   */
   private static dropWorker(worker: Worker): void {
     const index = this.slots.findIndex((s) => s.worker === worker);
     if (index === -1) {
       return;
     }
 
-    this.slots.splice(index, 1);
+    const [dropped] = this.slots.splice(index, 1);
 
-    if (this.slots.length === 0 && this.pending.size > 0) {
-      const stranded = new Error('Database query workers are unavailable');
-      for (const waiting of this.pending.values()) {
-        waiting.reject(stranded);
-      }
-      this.pending.clear();
+    if (dropped.queryId !== null) {
+      const waiting = this.pending.get(dropped.queryId);
+      this.pending.delete(dropped.queryId);
+      waiting?.reject(new Error('Database query worker stopped while running the query'));
+    }
+
+    if (this.slots.length === 0) {
+      logger.error('[DbQueryPool] All query workers are gone; falling back to in-process queries');
+      this.rejectOutstanding(new Error('Database query workers are unavailable'));
+    } else {
+      // A freed slot may exist now; keep the queue moving.
+      this.drain();
+    }
+  }
+
+  /** Settle everything in flight or waiting, so no caller is left hanging. */
+  private static rejectOutstanding(error: Error): void {
+    for (const waiting of this.pending.values()) {
+      waiting.reject(error);
+    }
+    this.pending.clear();
+
+    const queued = this.queue.splice(0, this.queue.length);
+    for (const item of queued) {
+      item.reject(error);
     }
   }
 
@@ -179,6 +208,7 @@ export class DbQueryPool {
       this.nextQueryId += 1;
 
       slot.busy = true;
+      slot.queryId = id;
       this.pending.set(id, { resolve: next.resolve, reject: next.reject });
       slot.worker.postMessage({ type: 'query', id, sql: next.sql, params: [...next.params] });
     }
@@ -196,6 +226,7 @@ export class DbQueryPool {
     const slots = [...this.slots];
     this.slots = [];
     this.started = false;
+    this.rejectOutstanding(new Error('Database query workers are shutting down'));
     await Promise.all(slots.map((slot) => slot.worker.terminate()));
   }
 }

backend/src/services/DownloadedReconciler.ts

@@ -2,12 +2,24 @@ import fs from 'fs/promises';
 import { config } from '../config';
 import { logger } from '../utils/logger';
 import { DatabaseService } from './DatabaseService';
+import { GameDataDownloader } from '../game/services/GameDataDownloader';
 import { GameSearchCache } from './GameSearchCache';
 import { GameService } from './GameService';
 
 const ZIP_NAME_PATTERN =
   /^([0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12})-(\d+)\.zip$/i;
 
+/** SQLite's error code when the database or its filesystem is read-only. */
+function isReadOnlyError(error: unknown): boolean {
+  return (
+    error instanceof Error &&
+    'code' in error &&
+    typeof (error as { code?: unknown }).code === 'string' &&
+    ((error as { code: string }).code.startsWith('SQLITE_READONLY') ||
+      (error as { code: string }).code === 'SQLITE_CANTOPEN')
+  );
+}
+
 interface PendingRow {
   id: number;
   gameId: string;
@@ -32,15 +44,16 @@ export class DownloadedReconciler {
       return { scanned: 0, marked: 0 };
     }
 
-    const onDisk = new Map<string, Set<number>>();
+    // Index by exact filename and compare against what GameDataDownloader would
+    // generate for each row. Re-deriving the timestamp here instead would miss
+    // rows whose dateAdded uses one of the formats that helper normalizes.
+    const onDisk = new Set<string>();
+    const onDiskGameIds = new Set<string>();
     for (const file of files) {
       const match = ZIP_NAME_PATTERN.exec(file);
       if (!match) continue;
-      const gameId = match[1].toLowerCase();
-      const timestamp = Number(match[2]);
-      const timestamps = onDisk.get(gameId) ?? new Set<number>();
-      timestamps.add(timestamp);
-      onDisk.set(gameId, timestamps);
+      onDisk.add(file);
+      onDiskGameIds.add(match[1].toLowerCase());
     }
 
     if (onDisk.size === 0) {
@@ -55,10 +68,16 @@ export class DownloadedReconciler {
       .all() as PendingRow[];
 
     const toMark = pending.filter((row) => {
-      const timestamps = onDisk.get(row.gameId.toLowerCase());
-      if (!timestamps) return false;
-      const parsed = new Date(row.dateAdded).getTime();
-      return !isNaN(parsed) && timestamps.has(parsed);
+      if (!onDiskGameIds.has(row.gameId.toLowerCase())) {
+        return false;
+      }
+
+      try {
+        return onDisk.has(GameDataDownloader.getFilename(row.gameId, row.dateAdded));
+      } catch {
+        // A row whose dateAdded cannot produce a filename simply has no match.
+        return false;
+      }
     });
 
     if (toMark.length === 0) {
@@ -70,12 +89,25 @@ export class DownloadedReconciler {
       'UPDATE game SET activeDataOnDisk = 1 WHERE id = ? AND activeDataId = ?'
     );
 
-    db.transaction(() => {
-      for (const row of toMark) {
-        markData.run(row.id);
-        markGame.run(row.gameId, row.id);
+    try {
+      db.transaction(() => {
+        for (const row of toMark) {
+          markData.run(row.id);
+          markGame.run(row.gameId, row.id);
+        }
+      })();
+    } catch (error: unknown) {
+      if (isReadOnlyError(error)) {
+        // The shipped compose mounts Flashpoint read-only. Say so once, clearly,
+        // instead of failing every startup with a stack trace.
+        logger.warn(
+          '[DownloadedReconciler] Database is read-only, cannot record downloaded state. ' +
+            'Mount Flashpoint read-write or set ENABLE_LOCAL_DB_COPY=true.'
+        );
+        return { scanned: files.length, marked: 0 };
       }
-    })();
+      throw error;
+    }
 
     DatabaseService.noteSelfWrite();
     GameSearchCache.clearCache();

backend/src/services/GameSearchCache.ts

@@ -57,7 +57,7 @@ export class GameSearchCache {
       playModes: query.playModes ? [...query.playModes].sort().join(',') : '',
       languages: query.languages ? [...query.languages].sort().join(',') : '',
       library: query.library || '',
-      downloaded: query.downloaded ?? '',
+      downloaded: query.downloaded === true,
       tags: query.tags ? [...query.tags].sort().join(',') : '',
       yearFrom: query.yearFrom ?? '',
       yearTo: query.yearTo ?? '',

backend/src/services/GameService.downloaded.test.ts

@@ -113,6 +113,7 @@ describe('downloaded filter options use the durable cache', () => {
   function queryCount(): number {
     return (
       vi.mocked(DatabaseService.all).mock.calls.length +
+      vi.mocked(DatabaseService.allAsync).mock.calls.length +
       vi.mocked(DatabaseService.get).mock.calls.length
     );
   }

backend/src/services/GameService.ts

@@ -1366,7 +1366,11 @@ export class GameService {
         WHERE ${conditions.join(' AND ')}
       `;
 
-      const result = DatabaseService.get(sql, queryParams) as { min: number; max: number } | null;
+      const rows = (await DatabaseService.allAsync(sql, queryParams)) as Array<{
+        min: number;
+        max: number;
+      }>;
+      const result = rows[0] ?? null;
       return result ?? { min: 1970, max: new Date().getFullYear() };
     } catch (error) {
       logger.error('Error getting year range:', error);

backend/src/utils/metadataSources.ts

@@ -14,7 +14,10 @@ export const DEFAULT_METADATA_SOURCE_URL = 'https://fpfss.flashpointarchive.org'
 
 export function isAllowedMetadataSourceUrl(baseUrl: string): boolean {
   try {
-    return ALLOWED_METADATA_HOSTS.includes(new URL(baseUrl).hostname);
+    const url = new URL(baseUrl);
+    // HTTPS only: a sync writes rows straight into flashpoint.sqlite, so a
+    // plaintext source would let anyone on the path rewrite the game database.
+    return url.protocol === 'https:' && ALLOWED_METADATA_HOSTS.includes(url.hostname);
   } catch {
     return false;
   }

backend/src/utils/origins.test.ts

@@ -66,6 +66,7 @@ describe('isOriginAllowed', () => {
       isOriginAllowed({
         origin: 'http://192.168.0.118',
         host: '192.168.0.118',
+        protocol: 'http',
         configuredOrigins: [],
         domainOrigins: noDomains,
       })
@@ -77,6 +78,7 @@ describe('isOriginAllowed', () => {
       isOriginAllowed({
         origin: 'http://192.168.0.118:8080',
         host: '192.168.0.118:8080',
+        protocol: 'http',
         configuredOrigins: [],
         domainOrigins: noDomains,
       })
@@ -153,6 +155,20 @@ describe('isOriginAllowed', () => {
     ).toBe(false);
   });
 
+  it('rejects a plaintext origin claiming to be same-origin on an HTTPS request', () => {
+    // http://example.com and https://example.com are different origins. Matching
+    // on host alone would let an attacker's plaintext page pass as same-origin.
+    expect(
+      isOriginAllowed({
+        origin: 'http://example.com',
+        host: 'example.com',
+        protocol: 'https',
+        configuredOrigins: [],
+        domainOrigins: noDomains,
+      })
+    ).toBe(false);
+  });
+
   it('rejects an unparseable Origin header', () => {
     expect(
       isOriginAllowed({

backend/src/utils/origins.ts

@@ -44,6 +44,8 @@ interface OriginCheck {
   readonly origin: string | undefined;
   /** Host header from the request, including a non-default port. */
   readonly host: string | undefined;
+  /** Request scheme ('http' or 'https'), honouring X-Forwarded-Proto behind a trusted proxy. */
+  readonly protocol?: string;
   /** Origins from the DOMAIN setting, already normalized. */
   readonly configuredOrigins: readonly string[];
   /** Origins derived from the domains table, already normalized. */
@@ -58,6 +60,7 @@ interface OriginCheck {
 export function isOriginAllowed({
   origin,
   host,
+  protocol,
   configuredOrigins,
   domainOrigins,
 }: OriginCheck): boolean {
@@ -70,13 +73,13 @@ export function isOriginAllowed({
     return false;
   }
 
-  if (host !== undefined && host !== '') {
-    try {
-      if (new URL(normalized).host === host) {
-        return true;
-      }
-    } catch {
-      // Unparseable normalized origin cannot be same-origin; fall through.
+  if (host !== undefined && host !== '' && protocol !== undefined && protocol !== '') {
+    // Compare the whole origin, not just the host: http://example.com and
+    // https://example.com are different origins, and treating them as one would
+    // let a plaintext page claim same-origin against an HTTPS deployment.
+    const requestOrigin = normalizeOrigin(`${protocol}://${host}`);
+    if (requestOrigin !== null && requestOrigin === normalized) {
+      return true;
     }
   }