chore: full implementation of code review recommendations (security, quality, tests)

lib/__tests__/auth.test.ts

@@ -4,6 +4,7 @@ import { describe, it, expect, vi } from 'vitest';
 vi.mock('@/lib/config', () => ({
   getConfig: () => ({
     immich: { apiKey: 'test-api-key-for-auth-tests' },
+    authSecret: 'test-auth-secret-32-chars-long-min',
     subpages: [
       {
         name: 'Private',

lib/__tests__/immich.test.ts

@@ -0,0 +1,103 @@
+import { describe, it, expect, vi, beforeEach } from 'vitest';
+import { immich } from '../immich';
+import * as config from '../config';
+
+// Mock the config by wrapping it in a factory
+vi.mock('../config', async () => {
+  const actual = await vi.importActual<typeof config>('../config');
+  return {
+    ...actual,
+    getConfig: () => ({
+      immich: { apiUrl: 'http://immich.test/api', apiKey: 'test-key' },
+      authSecret: 'test-auth-secret-32-chars-long-min',
+      albums: ['album-1', 'album-2'],
+      standaloneAlbums: ['album-1'],
+      subpages: [],
+      albumOverrides: { 'album-1': 'Override Name' },
+      cacheTtl: 0,
+    }),
+  };
+});
+
+// Mock fetch globally
+global.fetch = vi.fn();
+
+describe('ImmichClient', () => {
+  const mockFetch = global.fetch as any;
+
+  beforeEach(() => {
+    vi.clearAllMocks();
+  });
+
+  describe('request()', () => {
+    it('returns data on successful JSON response', async () => {
+      mockFetch.mockResolvedValueOnce({
+        ok: true,
+        headers: { get: () => 'application/json' },
+        json: async () => ({ foo: 'bar' }),
+      });
+
+      // Access private method via any casting for testing
+      const result = await (immich as any).request('/test');
+      expect(result.data).toEqual({ foo: 'bar' });
+      expect(result.error).toBeNull();
+    });
+
+    it('returns error on non-ok response', async () => {
+      mockFetch.mockResolvedValueOnce({
+        ok: false,
+        status: 404,
+        statusText: 'Not Found',
+      });
+
+      const result = await (immich as any).request('/test');
+      expect(result.data).toBeNull();
+      expect(result.error).toContain('404 Not Found');
+    });
+  });
+
+  describe('getAlbums()', () => {
+    it('filters and overrides albums based on config', async () => {
+      mockFetch.mockResolvedValueOnce({
+        ok: true,
+        headers: { get: () => 'application/json' },
+        json: async () => [
+          { id: 'album-1', albumName: 'Original Name', assetCount: 10 },
+          { id: 'album-3', albumName: 'Not Allowed', assetCount: 5 },
+        ],
+      });
+
+      const albums = await immich.getAlbums();
+      expect(albums).toHaveLength(1);
+      expect(albums[0].id).toBe('album-1');
+      expect(albums[0].albumName).toBe('Override Name');
+      expect(albums[0].slug).toBe('override-name');
+    });
+  });
+
+  describe('getAlbum()', () => {
+    it('returns null for unallowed album IDs', async () => {
+      const album = await immich.getAlbum('album-3');
+      expect(album).toBeNull();
+    });
+
+    it('returns album data and filters trashed assets', async () => {
+      mockFetch.mockResolvedValueOnce({
+        ok: true,
+        headers: { get: () => 'application/json' },
+        json: async () => ({
+          id: 'album-1',
+          albumName: 'Name',
+          assets: [
+            { id: 'asset-1', isTrashed: false },
+            { id: 'asset-2', isTrashed: true },
+          ],
+        }),
+      });
+
+      const album = await immich.getAlbum('album-1');
+      expect(album?.assets).toHaveLength(1);
+      expect(album?.assets[0].id).toBe('asset-1');
+    });
+  });
+});

lib/__tests__/tokens.test.ts

@@ -4,6 +4,7 @@ import { describe, it, expect, vi, beforeEach } from 'vitest';
 vi.mock('@/lib/config', () => ({
   getConfig: () => ({
     immich: { apiKey: 'test-api-key-for-vitest-unit-tests' },
+    authSecret: 'test-auth-secret-32-chars-long-min',
   }),
 }));
 

lib/auth.ts

@@ -2,21 +2,30 @@
  * Album authentication helpers.
  * Uses HMAC tokens stored in HttpOnly cookies — no database needed.
  *
- * Token = HMAC-SHA256(slug + password, apiKey)
+ * Token = HMAC-SHA256(slug + passwordSecret, authSecret)
  * Cookie = lb_auth_<slug> = <token>
  */
 
 import crypto from 'crypto';
+import bcrypt from 'bcryptjs';
 import { getConfig, SubpageConfig } from './config';
 
 const TOKEN_EXPIRY_HOURS = 24;
 
+/**
+ * Check if a string looks like a bcrypt hash.
+ */
+function isHash(str: string): boolean {
+  return str.startsWith('$2a$') || str.startsWith('$2b$') || str.startsWith('$2y$');
+}
+
 function hmac(data: string): string {
   return crypto.createHmac('sha256', getConfig().authSecret).update(data).digest('hex');
 }
 
-function authToken(slug: string, password: string): string {
-  return hmac(`${slug}:${password}`);
+function authToken(slug: string, passwordSecret: string): string {
+  // Use the hash/password as part of the HMAC for a secure, unique session token
+  return hmac(`${slug}:${passwordSecret}`);
 }
 
 function cookieName(slug: string): string {
@@ -46,9 +55,23 @@ export function authenticate(slug: string, password: string): string | null {
   const sp = findSubpageBySlug(slug);
   if (!sp?.password) return null;
 
-  if (password !== sp.password) return null;
+  let isValid = false;
+  if (isHash(sp.password)) {
+    isValid = bcrypt.compareSync(password, sp.password);
+  } else {
+    // Plaintext fallback (deprecated)
+    isValid = password === sp.password;
+    if (isValid) {
+      console.warn(
+        `\n⚠️  SECURITY WARNING: Subpage "${slug}" is using a plaintext password in gallery.yaml.\n` +
+          `   Please hash it for better security. You can use: npx bcryptjs ${sp.password}\n`,
+      );
+    }
+  }
 
-  const token = authToken(slug, password);
+  if (!isValid) return null;
+
+  const token = authToken(slug, sp.password);
   const maxAge = TOKEN_EXPIRY_HOURS * 60 * 60;
   const secure = process.env.NODE_ENV === 'production' ? '; Secure' : '';
 

lib/env.ts

@@ -29,10 +29,25 @@ const envSchema = z.object({
 
   /**
    * Secret used for signing auth cookies and encrypting asset tokens.
-   * If not provided, IMMICH_API_KEY will be used as a fallback (not recommended for production).
    */
   AUTH_SECRET: z.string().min(16, 'AUTH_SECRET should be at least 16 characters').optional(),
-});
+}).refine(
+  (data) => {
+    // Only require AUTH_SECRET in production at runtime, not during build
+    if (
+      process.env.NODE_ENV === 'production' &&
+      !process.env.NEXT_PHASE?.includes('build') &&
+      !data.AUTH_SECRET
+    ) {
+      return false;
+    }
+    return true;
+  },
+  {
+    message: 'AUTH_SECRET is required in production',
+    path: ['AUTH_SECRET'],
+  },
+);
 
 export type Env = z.infer<typeof envSchema>;
 

lib/immich.ts

@@ -77,6 +77,8 @@ export interface SubpageSummary {
   coverAssetId: string | null;
 }
 
+export type ImmichResult<T> = { data: T | null; error: string | null };
+
 // ── API Client ─────────────────────────────────────────────────
 
 class ImmichClient {
@@ -89,7 +91,7 @@ class ImmichClient {
   /**
    * Make an authenticated request to the Immich API.
    */
-  private async request<T>(endpoint: string): Promise<T | null> {
+  private async request<T>(endpoint: string): Promise<ImmichResult<T>> {
     const url = `${this.config.immich.apiUrl}${endpoint}`;
     try {
       const res = await fetch(url, {
@@ -100,18 +102,20 @@ class ImmichClient {
       });
 
       if (!res.ok) {
-        console.error(`[Immich] ${res.status} ${res.statusText} for ${endpoint}`);
-        return null;
+        const error = `[Immich] ${res.status} ${res.statusText} for ${endpoint}`;
+        console.error(error);
+        return { data: null, error };
       }
 
       const contentType = res.headers.get('Content-Type') || '';
       if (contentType.includes('application/json')) {
-        return (await res.json()) as T;
+        return { data: (await res.json()) as T, error: null };
       }
-      return null;
-    } catch (error) {
-      console.error(`[Immich] Failed to reach ${url}:`, error);
-      return null;
+      return { data: null, error: 'Invalid response format (expected JSON)' };
+    } catch (err) {
+      const error = `[Immich] Failed to reach ${url}: ${err instanceof Error ? err.message : 'Unknown error'}`;
+      console.error(error);
+      return { data: null, error };
     }
   }
 
@@ -160,8 +164,8 @@ class ImmichClient {
     const cached = cache.get<ImmichAlbum[]>(cacheKey);
     if (cached) return cached;
 
-    const all = await this.request<ImmichAlbum[]>('/albums?shared=true');
-    if (!all) return [];
+    const { data: all, error } = await this.request<ImmichAlbum[]>('/albums?shared=true');
+    if (error || !all) return [];
 
     const allowedIds = new Set(this.config.albums);
     const filtered = all
@@ -276,8 +280,8 @@ class ImmichClient {
     const cached = cache.get<ImmichAlbum>(cacheKey);
     if (cached) return cached;
 
-    const album = await this.request<ImmichAlbum>(`/albums/${encodeURIComponent(albumId)}`);
-    if (!album) return null;
+    const { data: album, error } = await this.request<ImmichAlbum>(`/albums/${encodeURIComponent(albumId)}`);
+    if (error || !album) return null;
 
     // Filter out trashed assets
     album.assets = (album.assets || []).filter((a) => !a.isTrashed);
@@ -325,8 +329,8 @@ class ImmichClient {
     const cached = cache.get<ImmichAsset>(cacheKey);
     if (cached) return cached;
 
-    const asset = await this.request<ImmichAsset>(`/assets/${encodeURIComponent(assetId)}`);
-    if (!asset) return null;
+    const { data: asset, error } = await this.request<ImmichAsset>(`/assets/${encodeURIComponent(assetId)}`);
+    if (error || !asset) return null;
 
     cache.set(cacheKey, asset, this.config.cacheTtl);
     return asset;
@@ -406,8 +410,8 @@ class ImmichClient {
    * Check if the Immich server is reachable.
    */
   async ping(): Promise<boolean> {
-    const res = await this.request<{ res: string }>('/server/ping');
-    return !!res;
+    const { data } = await this.request<{ res: string }>('/server/ping');
+    return !!data;
   }
 }
 

lib/rate-limit.ts

@@ -2,6 +2,11 @@
  * In-memory sliding-window rate limiter.
  * Tracks request counts per IP per minute bucket.
  * Auto-evicts expired entries to prevent memory leaks.
+ *
+ * ⚠️ NOTE: This is an in-memory store. In a multi-node or serverless
+ * environment (Vercel, AWS Lambda, Docker Swarm), each instance will
+ * have its own independent rate limit. For a global limit across
+ * multiple nodes, a persistent store like Redis would be required.
  */
 
 interface RateLimitEntry {