security: fix pre-auth bypasses and restore Immich 3.x compatibility (#378)
Seven security issues from an audit, plus album pages and the map coming up empty against Immich 3.x. Verified against a real Immich 3.0.3 instance in a production build. Security: - Admin session forgery via a hardcoded 'dev-fallback-secret' signing key that bypassed the production AUTH_SECRET guard - /api/map ignored album passwords and returned working asset tokens; rewriting ?size=thumbnail to ?size=original downloaded full-resolution originals from protected albums - Admin login had no rate limit, and the client IP came from a spoofable header, so rotating it bypassed every limit in the app - Raw Immich asset UUIDs were serialized into the RSC payload - Album tokens never expired server-side; a captured token replayed forever - Path traversal into the Immich API via an unvalidated album id - /admin ran with no enforced CSP (only Report-Only reached it) TRUSTED_PROXIES is replaced by TRUSTED_PROXY_HOPS. The old variable matched proxy IPs against the socket peer address, which self-hosted Next.js never exposes, so the check could never pass: unset, the limiter read a spoofable header; set, every request shared one bucket and a single client could rate-limit the whole site. Migrates automatically. Immich 3.x: GET /albums/:id no longer embeds assets. Assets now come from POST /search/metadata with withExif: true — without that flag EXIF is omitted and grid aspect ratios, the lightbox panel and map markers stay empty. Drops support for Immich below 3.0, which now logs a warning. Upgrade: requires Immich 3.0+; set TRUSTED_PROXY_HOPS (nginx = 1); visitors of protected areas and admin sessions must log in once more. 91 unit tests (25 new); the new admin-auth and rate-limit cases were confirmed to fail against the pre-fix code. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
137
lib/__tests__/admin-auth.test.ts
Normal file
137
lib/__tests__/admin-auth.test.ts
Normal file
@@ -0,0 +1,137 @@
|
||||
import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
|
||||
import crypto from 'crypto';
|
||||
|
||||
// next/headers is only used by isAdminAuthenticated(), which we do not exercise here.
|
||||
vi.mock('next/headers', () => ({ cookies: async () => ({ get: () => undefined }) }));
|
||||
|
||||
const ADMIN_PASSWORD = 'correct-horse-battery-staple';
|
||||
const AUTH_SECRET = 'test-auth-secret-32-chars-long-min';
|
||||
|
||||
vi.mock('../env', () => ({
|
||||
env: {
|
||||
get AUTH_SECRET() {
|
||||
return process.env.__TEST_AUTH_SECRET;
|
||||
},
|
||||
get ADMIN_PASSWORD() {
|
||||
return process.env.__TEST_ADMIN_PASSWORD;
|
||||
},
|
||||
},
|
||||
}));
|
||||
|
||||
/** Import fresh so module-level state cannot leak between cases. */
|
||||
async function loadAuth() {
|
||||
vi.resetModules();
|
||||
return import('../admin/auth');
|
||||
}
|
||||
|
||||
function forgeToken(signingKey: Buffer, payload: object): string {
|
||||
const data = Buffer.from(JSON.stringify(payload)).toString('base64url');
|
||||
const sig = crypto.createHmac('sha256', signingKey).update(data).digest('base64url');
|
||||
return `${data}.${sig}`;
|
||||
}
|
||||
|
||||
beforeEach(() => {
|
||||
process.env.__TEST_AUTH_SECRET = AUTH_SECRET;
|
||||
process.env.__TEST_ADMIN_PASSWORD = ADMIN_PASSWORD;
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
delete process.env.__TEST_AUTH_SECRET;
|
||||
delete process.env.__TEST_ADMIN_PASSWORD;
|
||||
vi.unstubAllEnvs();
|
||||
});
|
||||
|
||||
describe('admin session tokens', () => {
|
||||
it('accepts a token it just issued', async () => {
|
||||
const { createAdminToken, verifyAdminToken } = await loadAuth();
|
||||
expect(verifyAdminToken(createAdminToken())).toBe(true);
|
||||
});
|
||||
|
||||
it('rejects a token forged with the old hardcoded dev fallback secret', async () => {
|
||||
const { verifyAdminToken } = await loadAuth();
|
||||
// The exact key an attacker could derive from the public source before the fix.
|
||||
const legacyKey = crypto.createHash('sha256').update('admin:dev-fallback-secret').digest();
|
||||
const forged = forgeToken(legacyKey, {
|
||||
role: 'admin',
|
||||
iat: Date.now(),
|
||||
exp: Date.now() + 60_000,
|
||||
});
|
||||
expect(verifyAdminToken(forged)).toBe(false);
|
||||
});
|
||||
|
||||
it('does not fall back to a guessable secret when AUTH_SECRET is unset in dev', async () => {
|
||||
delete process.env.__TEST_AUTH_SECRET;
|
||||
const { verifyAdminToken } = await loadAuth();
|
||||
const legacyKey = crypto.createHash('sha256').update('admin:dev-fallback-secret').digest();
|
||||
const forged = forgeToken(legacyKey, {
|
||||
role: 'admin',
|
||||
iat: Date.now(),
|
||||
exp: Date.now() + 60_000,
|
||||
});
|
||||
expect(verifyAdminToken(forged)).toBe(false);
|
||||
});
|
||||
|
||||
it('throws rather than signing with a guessable secret when AUTH_SECRET is unset in production', async () => {
|
||||
delete process.env.__TEST_AUTH_SECRET;
|
||||
vi.stubEnv('NODE_ENV', 'production');
|
||||
const { createAdminToken } = await loadAuth();
|
||||
expect(() => createAdminToken()).toThrow(/AUTH_SECRET/);
|
||||
});
|
||||
|
||||
it('returns false (not throw) for a malformed cookie with a short signature', async () => {
|
||||
const { verifyAdminToken } = await loadAuth();
|
||||
// Pre-fix this hit crypto.timingSafeEqual outside the try block -> RangeError -> HTTP 500.
|
||||
expect(() => verifyAdminToken('a.b')).not.toThrow();
|
||||
expect(verifyAdminToken('a.b')).toBe(false);
|
||||
expect(verifyAdminToken('not-a-token')).toBe(false);
|
||||
expect(verifyAdminToken('')).toBe(false);
|
||||
});
|
||||
|
||||
it('rejects an expired token', async () => {
|
||||
const { verifyAdminToken } = await loadAuth();
|
||||
const key = crypto
|
||||
.createHash('sha256')
|
||||
.update(`admin:${AUTH_SECRET}:${ADMIN_PASSWORD}`)
|
||||
.digest();
|
||||
const expired = forgeToken(key, { role: 'admin', iat: 0, exp: Date.now() - 1000 });
|
||||
expect(verifyAdminToken(expired)).toBe(false);
|
||||
});
|
||||
|
||||
it('rejects a validly signed token whose role is not admin', async () => {
|
||||
const { verifyAdminToken } = await loadAuth();
|
||||
const key = crypto
|
||||
.createHash('sha256')
|
||||
.update(`admin:${AUTH_SECRET}:${ADMIN_PASSWORD}`)
|
||||
.digest();
|
||||
const wrongRole = forgeToken(key, { role: 'viewer', iat: 0, exp: Date.now() + 60_000 });
|
||||
expect(verifyAdminToken(wrongRole)).toBe(false);
|
||||
});
|
||||
|
||||
it('invalidates existing sessions when ADMIN_PASSWORD is rotated', async () => {
|
||||
const first = await loadAuth();
|
||||
const token = first.createAdminToken();
|
||||
expect(first.verifyAdminToken(token)).toBe(true);
|
||||
|
||||
process.env.__TEST_ADMIN_PASSWORD = 'a-brand-new-password';
|
||||
const second = await loadAuth();
|
||||
expect(second.verifyAdminToken(token)).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
describe('verifyAdminPassword', () => {
|
||||
it('accepts the configured password and rejects others', async () => {
|
||||
const { verifyAdminPassword } = await loadAuth();
|
||||
expect(verifyAdminPassword(ADMIN_PASSWORD)).toBe(true);
|
||||
expect(verifyAdminPassword('wrong')).toBe(false);
|
||||
expect(verifyAdminPassword(`${ADMIN_PASSWORD}x`)).toBe(false);
|
||||
expect(verifyAdminPassword('')).toBe(false);
|
||||
});
|
||||
|
||||
it('rejects everything when no ADMIN_PASSWORD is configured', async () => {
|
||||
delete process.env.__TEST_ADMIN_PASSWORD;
|
||||
const { verifyAdminPassword, isAdminEnabled } = await loadAuth();
|
||||
expect(isAdminEnabled()).toBe(false);
|
||||
expect(verifyAdminPassword('')).toBe(false);
|
||||
expect(verifyAdminPassword('anything')).toBe(false);
|
||||
});
|
||||
});
|
||||
@@ -1,4 +1,4 @@
|
||||
import { describe, it, expect, vi } from 'vitest';
|
||||
import { describe, it, expect, vi, afterEach } from 'vitest';
|
||||
|
||||
// Mock config to provide a predictable API key and subpage config
|
||||
vi.mock('@/lib/config', () => ({
|
||||
@@ -93,3 +93,60 @@ describe('isAuthenticated', () => {
|
||||
expect(isAuthenticated('public', getCookie)).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
describe('token expiry', () => {
|
||||
function tokenFor(slug: string, password: string): string {
|
||||
return authenticate(slug, password)!.split('=')[1].split(';')[0];
|
||||
}
|
||||
|
||||
afterEach(() => {
|
||||
vi.useRealTimers();
|
||||
});
|
||||
|
||||
it('rejects a token once its embedded expiry has passed', () => {
|
||||
const token = tokenFor('private', 'secret123');
|
||||
const getCookie = (name: string) => (name === 'lb_auth_private' ? token : undefined);
|
||||
expect(isAuthenticated('private', getCookie)).toBe(true);
|
||||
|
||||
// Previously the 24h lifetime lived only in the cookie's Max-Age, so a
|
||||
// captured token replayed via curl worked forever.
|
||||
vi.useFakeTimers();
|
||||
vi.setSystemTime(Date.now() + 25 * 60 * 60 * 1000);
|
||||
expect(isAuthenticated('private', getCookie)).toBe(false);
|
||||
});
|
||||
|
||||
it('still accepts a token just before it expires', () => {
|
||||
const token = tokenFor('private', 'secret123');
|
||||
const getCookie = (name: string) => (name === 'lb_auth_private' ? token : undefined);
|
||||
|
||||
vi.useFakeTimers();
|
||||
vi.setSystemTime(Date.now() + 23 * 60 * 60 * 1000);
|
||||
expect(isAuthenticated('private', getCookie)).toBe(true);
|
||||
});
|
||||
|
||||
it('rejects a token whose expiry was tampered with', () => {
|
||||
const token = tokenFor('private', 'secret123');
|
||||
const [, sig] = token.split('.');
|
||||
const farFuture = Date.now() + 365 * 24 * 60 * 60 * 1000;
|
||||
|
||||
// The expiry is covered by the HMAC, so extending it invalidates the token.
|
||||
const getCookie = (name: string) =>
|
||||
name === 'lb_auth_private' ? `${farFuture}.${sig}` : undefined;
|
||||
expect(isAuthenticated('private', getCookie)).toBe(false);
|
||||
});
|
||||
|
||||
it('rejects malformed and legacy tokens without throwing', () => {
|
||||
for (const value of ['', '.', 'abc', 'abc.def', `${Date.now() + 1000}.`, 'deadbeef']) {
|
||||
const getCookie = (name: string) => (name === 'lb_auth_private' ? value : undefined);
|
||||
expect(() => isAuthenticated('private', getCookie)).not.toThrow();
|
||||
expect(isAuthenticated('private', getCookie)).toBe(false);
|
||||
}
|
||||
});
|
||||
|
||||
it('sets a cookie Max-Age consistent with the embedded expiry', () => {
|
||||
const cookie = authenticate('private', 'secret123')!;
|
||||
const maxAge = Number(cookie.match(/Max-Age=(\d+)/)![1]);
|
||||
const exp = Number(cookie.split('=')[1].split(';')[0].split('.')[0]);
|
||||
expect(Math.abs(exp - (Date.now() + maxAge * 1000))).toBeLessThan(2000);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
import { describe, it, expect, vi, beforeEach } from 'vitest';
|
||||
import { immich } from '../immich';
|
||||
import * as config from '../config';
|
||||
import { cache } from '../cache';
|
||||
|
||||
// Mock the config by wrapping it in a factory
|
||||
vi.mock('../config', async () => {
|
||||
@@ -28,6 +29,9 @@ describe('ImmichClient', () => {
|
||||
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks();
|
||||
// getAlbum() memoises into the module-level LRU; without this, later tests
|
||||
// read a previous test's album and never reach the mocked fetch.
|
||||
cache.clear();
|
||||
});
|
||||
|
||||
describe('request()', () => {
|
||||
@@ -80,23 +84,105 @@ describe('ImmichClient', () => {
|
||||
expect(album).toBeNull();
|
||||
});
|
||||
|
||||
it('returns album data and filters trashed assets', async () => {
|
||||
mockFetch.mockResolvedValueOnce({
|
||||
/**
|
||||
* Immich 3.x returns an empty `assets` array from GET /albums/:id, so
|
||||
* assets come from a separate POST /search/metadata call. Route the mock by
|
||||
* URL rather than by call order — the two run concurrently.
|
||||
*/
|
||||
function mockAlbumApi(options: {
|
||||
order?: 'asc' | 'desc';
|
||||
pages?: Array<{ items: unknown[]; nextPage: string | null }>;
|
||||
}) {
|
||||
const pages = options.pages ?? [];
|
||||
const json = (body: unknown) => ({
|
||||
ok: true,
|
||||
headers: { get: () => 'application/json' },
|
||||
json: async () => ({
|
||||
json: async () => body,
|
||||
});
|
||||
|
||||
mockFetch.mockImplementation(async (url: string, init?: { body?: string }) => {
|
||||
if (url.includes('/search/metadata')) {
|
||||
const page = JSON.parse(init?.body ?? '{}').page ?? 1;
|
||||
const p = pages[page - 1] ?? { items: [], nextPage: null };
|
||||
return json({ assets: { items: p.items, nextPage: p.nextPage, total: p.items.length } });
|
||||
}
|
||||
// Mirrors the real 3.x response: assetCount set, assets empty.
|
||||
return json({
|
||||
id: 'album-1',
|
||||
albumName: 'Name',
|
||||
assets: [
|
||||
{ id: 'asset-1', isTrashed: false },
|
||||
{ id: 'asset-2', isTrashed: true },
|
||||
],
|
||||
}),
|
||||
assetCount: 2,
|
||||
assets: [],
|
||||
order: options.order ?? 'desc',
|
||||
});
|
||||
});
|
||||
}
|
||||
|
||||
const asset = (id: string, date: string, isTrashed = false) => ({
|
||||
id,
|
||||
isTrashed,
|
||||
fileCreatedAt: date,
|
||||
});
|
||||
|
||||
it('loads assets via metadata search and filters trashed ones', async () => {
|
||||
mockAlbumApi({
|
||||
pages: [
|
||||
{
|
||||
items: [
|
||||
asset('asset-1', '2024-01-02T00:00:00Z'),
|
||||
asset('asset-2', '2024-01-03T00:00:00Z', true),
|
||||
],
|
||||
nextPage: null,
|
||||
},
|
||||
],
|
||||
});
|
||||
|
||||
const album = await immich.getAlbum('album-1');
|
||||
expect(album?.assets).toHaveLength(1);
|
||||
expect(album?.assets[0].id).toBe('asset-1');
|
||||
});
|
||||
|
||||
it('requests EXIF data — without it the grid, lightbox and map go blank', async () => {
|
||||
mockAlbumApi({ pages: [{ items: [], nextPage: null }] });
|
||||
await immich.getAlbum('album-1');
|
||||
|
||||
const search = mockFetch.mock.calls.find((c: unknown[]) =>
|
||||
String(c[0]).includes('/search/metadata'),
|
||||
);
|
||||
expect(search).toBeDefined();
|
||||
const body = JSON.parse((search[1] as { body: string }).body);
|
||||
expect(body.withExif).toBe(true);
|
||||
expect(body.albumIds).toEqual(['album-1']);
|
||||
expect(search[1].method).toBe('POST');
|
||||
});
|
||||
|
||||
it('follows pagination until nextPage is null', async () => {
|
||||
mockAlbumApi({
|
||||
pages: [
|
||||
{ items: [asset('a', '2024-01-01T00:00:00Z')], nextPage: '2' },
|
||||
{ items: [asset('b', '2024-01-02T00:00:00Z')], nextPage: '3' },
|
||||
{ items: [asset('c', '2024-01-03T00:00:00Z')], nextPage: null },
|
||||
],
|
||||
});
|
||||
|
||||
const album = await immich.getAlbum('album-1');
|
||||
expect(album?.assets.map((a) => a.id)).toEqual(['c', 'b', 'a']); // desc by fileCreatedAt
|
||||
});
|
||||
|
||||
it('honours the album sort order', async () => {
|
||||
const items = [
|
||||
asset('old', '2024-01-01T00:00:00Z'),
|
||||
asset('new', '2024-06-01T00:00:00Z'),
|
||||
asset('mid', '2024-03-01T00:00:00Z'),
|
||||
];
|
||||
|
||||
mockAlbumApi({ order: 'asc', pages: [{ items, nextPage: null }] });
|
||||
let album = await immich.getAlbum('album-1');
|
||||
expect(album?.assets.map((a) => a.id)).toEqual(['old', 'mid', 'new']);
|
||||
|
||||
cache.clear(); // same album id — otherwise the asc result is reused
|
||||
mockAlbumApi({ order: 'desc', pages: [{ items, nextPage: null }] });
|
||||
album = await immich.getAlbum('album-1');
|
||||
expect(album?.assets.map((a) => a.id)).toEqual(['new', 'mid', 'old']);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -1,48 +1,87 @@
|
||||
import { describe, it, expect, beforeEach } from 'vitest';
|
||||
import { describe, it, expect, beforeEach, vi } from 'vitest';
|
||||
import { NextRequest } from 'next/server';
|
||||
|
||||
let mockHops = 0;
|
||||
vi.mock('@/lib/config', () => ({
|
||||
getConfig: () => ({ trustedProxyHops: mockHops }),
|
||||
}));
|
||||
|
||||
import { checkRateLimit, getClientIp } from '@/lib/rate-limit';
|
||||
|
||||
describe('getClientIp', () => {
|
||||
it('prioritizes request.ip when present', () => {
|
||||
const req = {
|
||||
ip: '1.2.3.4',
|
||||
headers: new Headers({
|
||||
'x-real-ip': '5.6.7.8',
|
||||
'x-forwarded-for': '9.10.11.12',
|
||||
}),
|
||||
} as unknown as NextRequest;
|
||||
function req(headers: Record<string, string>, ip?: string): NextRequest {
|
||||
return { ...(ip ? { ip } : {}), headers: new Headers(headers) } as unknown as NextRequest;
|
||||
}
|
||||
|
||||
expect(getClientIp(req)).toBe('1.2.3.4');
|
||||
describe('getClientIp with no proxy configured (TRUSTED_PROXY_HOPS=0)', () => {
|
||||
beforeEach(() => {
|
||||
mockHops = 0;
|
||||
});
|
||||
|
||||
it('falls back to x-real-ip if request.ip is missing', () => {
|
||||
const req = {
|
||||
headers: new Headers({
|
||||
'x-real-ip': '5.6.7.8',
|
||||
'x-forwarded-for': '9.10.11.12',
|
||||
}),
|
||||
} as unknown as NextRequest;
|
||||
|
||||
expect(getClientIp(req)).toBe('5.6.7.8');
|
||||
it('prioritizes request.ip when the platform provides it', () => {
|
||||
expect(req({ 'x-real-ip': '5.6.7.8' }, '1.2.3.4')).toBeTruthy();
|
||||
expect(getClientIp(req({ 'x-real-ip': '5.6.7.8' }, '1.2.3.4'))).toBe('1.2.3.4');
|
||||
});
|
||||
|
||||
it('falls back to x-forwarded-for if request.ip and x-real-ip are missing', () => {
|
||||
const req = {
|
||||
headers: new Headers({
|
||||
'x-forwarded-for': '9.10.11.12, 13.14.15.16',
|
||||
}),
|
||||
} as unknown as NextRequest;
|
||||
|
||||
// Should take the first IP from x-forwarded-for list
|
||||
expect(getClientIp(req)).toBe('9.10.11.12');
|
||||
it('falls back to headers on a best-effort basis', () => {
|
||||
expect(getClientIp(req({ 'x-real-ip': '5.6.7.8' }))).toBe('5.6.7.8');
|
||||
expect(getClientIp(req({ 'x-forwarded-for': '9.10.11.12, 13.14.15.16' }))).toBe('9.10.11.12');
|
||||
});
|
||||
|
||||
it('returns unknown if no IP headers are present', () => {
|
||||
const req = {
|
||||
headers: new Headers(),
|
||||
} as unknown as NextRequest;
|
||||
it('returns unknown if no IP information is available at all', () => {
|
||||
expect(getClientIp(req({}))).toBe('unknown');
|
||||
});
|
||||
});
|
||||
|
||||
expect(getClientIp(req)).toBe('unknown');
|
||||
describe('getClientIp behind one trusted proxy (TRUSTED_PROXY_HOPS=1, e.g. nginx)', () => {
|
||||
beforeEach(() => {
|
||||
mockHops = 1;
|
||||
});
|
||||
|
||||
it('takes the rightmost X-Forwarded-For entry, which the proxy appended itself', () => {
|
||||
// nginx `$proxy_add_x_forwarded_for` appends the real peer address, so the
|
||||
// last entry is authoritative no matter what the client sent.
|
||||
expect(getClientIp(req({ 'x-forwarded-for': '203.0.113.9' }))).toBe('203.0.113.9');
|
||||
});
|
||||
|
||||
it('ignores client-supplied entries to the left of the appended one', () => {
|
||||
// The attacker sends `X-Forwarded-For: 1.1.1.1`; nginx appends their real IP.
|
||||
const spoofed = req({ 'x-forwarded-for': '1.1.1.1, 203.0.113.9' });
|
||||
expect(getClientIp(spoofed)).toBe('203.0.113.9');
|
||||
});
|
||||
|
||||
it('cannot be pinned to an attacker-chosen bucket by a long forged chain', () => {
|
||||
const forged = req({ 'x-forwarded-for': '1.1.1.1, 2.2.2.2, 3.3.3.3, 203.0.113.9' });
|
||||
expect(getClientIp(forged)).toBe('203.0.113.9');
|
||||
});
|
||||
|
||||
it('does not trust a spoofed X-Real-IP over the appended X-Forwarded-For entry', () => {
|
||||
const spoofed = req({ 'x-real-ip': '1.1.1.1', 'x-forwarded-for': '9.9.9.9, 203.0.113.9' });
|
||||
expect(getClientIp(spoofed)).toBe('203.0.113.9');
|
||||
});
|
||||
|
||||
it('accepts X-Real-IP when the proxy sets only that header (nginx overwrites it)', () => {
|
||||
expect(getClientIp(req({ 'x-real-ip': '203.0.113.9' }))).toBe('203.0.113.9');
|
||||
});
|
||||
|
||||
it('refuses to identify a request that did not traverse the proxy', () => {
|
||||
// No proxy headers at all, yet hops are configured: the request reached the
|
||||
// app directly. Fall back to unknown rather than to a spoofable value.
|
||||
expect(getClientIp(req({}))).toBe('unknown');
|
||||
});
|
||||
});
|
||||
|
||||
describe('getClientIp behind two trusted proxies (TRUSTED_PROXY_HOPS=2)', () => {
|
||||
beforeEach(() => {
|
||||
mockHops = 2;
|
||||
});
|
||||
|
||||
it('skips both proxy hops from the right', () => {
|
||||
const r = req({ 'x-forwarded-for': '203.0.113.9, 10.0.0.5' });
|
||||
expect(getClientIp(r)).toBe('203.0.113.9');
|
||||
});
|
||||
|
||||
it('returns unknown when the chain is shorter than the configured hop count', () => {
|
||||
expect(getClientIp(req({ 'x-forwarded-for': '10.0.0.5' }))).toBe('unknown');
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
Reference in New Issue
Block a user