diff --git a/server/src/db/migrations.ts b/server/src/db/migrations.ts index 6fc504466..0e55ccdfc 100644 --- a/server/src/db/migrations.ts +++ b/server/src/db/migrations.ts @@ -3909,6 +3909,52 @@ function runMigrations(db: Database.Database): void { ) `); }, + // #1939 — the Google Places and Unsplash keys are instance configuration and + // now resolve out of app_settings (nest/settings/instance-api-keys.ts). This + // carries the value the old resolver would have handed out — the lowest-id + // admin who has one — into that row, so an install that upgrades keeps the + // key it was searching with instead of falling back to OpenStreetMap. + // Only where that key already was the whole install's, though. The resolver + // reads the instance row before the caller's own column, so promoting a + // personal key while a second row still holds one would move that member + // onto a stranger's key and a stranger's bill without anyone saying so. As + // soon as another row has a value for the same column nothing is written: + // everybody keeps resolving what they resolved before, and a member with no + // key of their own searches via OpenStreetMap until an admin saves the + // instance key once in the panel. Handing a key to the whole instance is the + // admin's call, not a migration's. + // The users columns are left alone: they are still the per-user fallback, + // and nothing here can lose a value. Stored blobs are copied verbatim; both + // sides use the same apiKeyCrypto format, legacy plaintext included. + // Appended LAST — the array is index-addressed against schema_version. + () => { + const upsert = db.prepare( + `INSERT INTO app_settings (key, value) VALUES (?, ?) + ON CONFLICT(key) DO UPDATE SET value = excluded.value` + ); + for (const column of ['maps_api_key', 'unsplash_api_key'] as const) { + const existing = db.prepare('SELECT value FROM app_settings WHERE key = ?').get(column) as + | { value: string | null } + | undefined; + if (existing?.value) continue; + const row = db + .prepare( + `SELECT id, ${column} AS value FROM users + WHERE role = 'admin' AND ${column} IS NOT NULL AND ${column} != '' + ORDER BY id ASC LIMIT 1` + ) + .get() as { id: number; value: string } | undefined; + if (!row?.value) continue; + const otherHolder = db + .prepare( + `SELECT 1 FROM users + WHERE id != ? AND ${column} IS NOT NULL AND ${column} != '' LIMIT 1` + ) + .get(row.id); + if (otherHolder) continue; + upsert.run(column, row.value); + } + }, ]; if (currentVersion < migrations.length) { diff --git a/server/src/demo/demo-reset.ts b/server/src/demo/demo-reset.ts index 45b2812e4..4eec2c538 100644 --- a/server/src/demo/demo-reset.ts +++ b/server/src/demo/demo-reset.ts @@ -27,6 +27,19 @@ function resetDemoUser(): void { console.error('[Demo Reset] Failed to read admin data:', e instanceof Error ? e.message : e); } + // The Places/Unsplash keys the searches actually use live in app_settings + // since #1939, so they have to survive the restore alongside the columns — + // otherwise the demo instance loses map search on every reset. + interface InstanceKeyRow { key: string; value: string | null } + let instanceKeys: InstanceKeyRow[] = []; + try { + instanceKeys = db.prepare( + "SELECT key, value FROM app_settings WHERE key IN ('maps_api_key', 'unsplash_api_key')" + ).all() as InstanceKeyRow[]; + } catch (e: unknown) { + console.error('[Demo Reset] Failed to read instance API keys:', e instanceof Error ? e.message : e); + } + // Flush WAL to main DB file try { db.exec('PRAGMA wal_checkpoint(TRUNCATE)'); } catch (e) {} @@ -67,6 +80,19 @@ function resetDemoUser(): void { } } + if (instanceKeys.length) { + try { + const { db: freshDb } = require('../db/database'); + const upsert = freshDb.prepare( + `INSERT INTO app_settings (key, value) VALUES (?, ?) + ON CONFLICT(key) DO UPDATE SET value = excluded.value` + ); + for (const row of instanceKeys) upsert.run(row.key, row.value); + } catch (e: unknown) { + console.error('[Demo Reset] Failed to restore instance API keys:', e instanceof Error ? e.message : e); + } + } + console.log('[Demo Reset] Database restored from baseline'); } diff --git a/server/src/nest/audit/audit.service.ts b/server/src/nest/audit/audit.service.ts index e6de049be..8e6fe9746 100644 --- a/server/src/nest/audit/audit.service.ts +++ b/server/src/nest/audit/audit.service.ts @@ -11,6 +11,7 @@ const ACTION_LABELS: Record = { 'user.mfa_enable': 'enabled MFA', 'user.mfa_disable': 'disabled MFA', 'settings.app_update': 'updated settings', + 'settings.api_keys_update': 'updated API keys', 'trip.create': 'created trip', 'trip.delete': 'deleted trip', 'admin.user_role_change': 'changed user role', @@ -38,6 +39,12 @@ function buildInfoSummary(action: string, details?: Record): st if (details.require_mfa !== undefined) parts.push(`mfa=${details.require_mfa}`); return parts.length ? ` (${parts.join(', ')})` : ''; } + if (action === 'settings.api_keys_update') { + // The names, read straight out of details — the writer puts nothing else in + // there, and a key value must never reach a log line. + const changed = Array.isArray(details.changed) ? details.changed : []; + return changed.length ? ` (${changed.join(', ')})` : ''; + } if (action === 'immich.private_ip_configured') { return details.resolved_ip ? ` url=${details.immich_url} ip=${details.resolved_ip}` : ''; } diff --git a/server/src/nest/auth/auth.controller.ts b/server/src/nest/auth/auth.controller.ts index b692eb235..c1074fb1b 100644 --- a/server/src/nest/auth/auth.controller.ts +++ b/server/src/nest/auth/auth.controller.ts @@ -123,22 +123,51 @@ export class AuthController { return { success: true }; } + /** + * One row per save that actually changed a key, on all three routes that can + * write these columns (#1939 asked for it: the API keys were the only admin + * setting whose change left no trace, while `settings.app_update` next door + * has always been logged). + * + * Names only. `details` is rendered as raw JSON in the admin audit panel and + * mirrored into the debug log, so a value here would leak the key twice over. + * Nothing is written when nothing changed — the panel saves before every test + * click, and each click would otherwise cost a log line. + */ + private auditApiKeys(userId: number, changed: string[], req: Request): void { + if (!changed.length) return; + this.audit.writeAudit({ + userId, + action: 'settings.api_keys_update', + resource: 'api_keys', + ip: getClientIp(req), + details: { changed }, + }); + } + @Put('me/maps-key') - mapsKey(@CurrentUser() user: User, @Body() body: MapsKeyUpdateDto) { - return this.profile.updateMapsKey(user.id, body.maps_api_key); + mapsKey(@CurrentUser() user: User, @Body() body: MapsKeyUpdateDto, @Req() req: Request) { + // changedKeys is for the audit line, not for the client: destructured off so + // the response body stays what it always was. + const { changedKeys = [], ...result } = this.profile.updateMapsKey(user.id, body.maps_api_key); + this.auditApiKeys(user.id, changedKeys, req); + return result; } @Put('me/api-keys') - apiKeys(@CurrentUser() user: User, @Body() body: ApiKeysUpdateDto) { - return this.profile.updateApiKeys(user.id, body); + apiKeys(@CurrentUser() user: User, @Body() body: ApiKeysUpdateDto, @Req() req: Request) { + const { changedKeys = [], ...result } = this.profile.updateApiKeys(user.id, body); + this.auditApiKeys(user.id, changedKeys, req); + return result; } @Put('me/settings') - updateSettings(@CurrentUser() user: User, @Body() body: SettingsUpdateDto) { + updateSettings(@CurrentUser() user: User, @Body() body: SettingsUpdateDto, @Req() req: Request) { const result = this.profile.updateSettings(user.id, body); if (result.error) { throw new HttpException({ error: result.error }, result.status!); } + this.auditApiKeys(user.id, result.changedKeys ?? [], req); return { success: result.success, user: result.user }; } diff --git a/server/src/nest/auth/auth.service.ts b/server/src/nest/auth/auth.service.ts index 3cb0c91c6..31ce4528a 100644 --- a/server/src/nest/auth/auth.service.ts +++ b/server/src/nest/auth/auth.service.ts @@ -14,6 +14,7 @@ import { PermissionsService } from '../permissions/permissions.service'; import { validatePassword } from '../common/passwordPolicy'; import { encryptMfaSecret, decryptMfaSecret } from '../common/crypto/mfaCrypto'; import { decrypt_api_key, maybe_encrypt_api_key, encrypt_api_key } from '../common/crypto/apiKeyCrypto'; +import { resolveApiKey } from '../settings/instance-api-keys'; import { EphemeralTokenService } from './ephemeral-token.service'; // Import from sessionManager directly, NOT the ../../mcp barrel: the barrel pulls // the whole tools fan-out (and via the domain bridges, the Nest services) into @@ -235,13 +236,12 @@ export class AuthService { // One directory deeper than the legacy src/services location — the extra // '../' keeps resolving to the workspace package.json. const version: string = readEnv().app.appVersion ?? require('../../../package.json').version; - // The operator's key counts too. getMapsKey prefers it over anything in the - // users table, so asking the table alone would report "no key" on an install - // where every lookup works, and the client would hide the Google features it - // is actually able to use. - const hasGoogleKey = - !!readEnv().maps.placesApiKey || - !!this.db.get("SELECT maps_api_key FROM users WHERE role = 'admin' AND maps_api_key IS NOT NULL AND maps_api_key != '' LIMIT 1"); + // Asked through the same resolver the search itself uses, so the client can + // never show Google features that rest on a key this caller does not get — + // nor hide them from a member who does have one (#1939). Unauthenticated the + // question is only about the instance, which is the first two steps of the + // chain; id 0 matches no row. + const hasGoogleKey = !!resolveApiKey(this.db, 'maps_api_key', authenticatedUser?.id ?? 0, readEnv().maps.placesApiKey).key; const oidcDisplayName = readEnv().oidc.displayName || this.db.get<{ value: string }>("SELECT value FROM app_settings WHERE key = 'oidc_display_name'")?.value || null; const oidcConfigured = !!( diff --git a/server/src/nest/auth/user-profile.service.ts b/server/src/nest/auth/user-profile.service.ts index 0c374340a..b3b44b897 100644 --- a/server/src/nest/auth/user-profile.service.ts +++ b/server/src/nest/auth/user-profile.service.ts @@ -7,6 +7,14 @@ import { decrypt_api_key, maybe_encrypt_api_key } from '../common/crypto/apiKeyC import { avatarUrl } from '../common/avatarUrl'; import { EMAIL_REGEX, avatarDir, mask_stored_api_key } from './auth.helpers'; import { splitManagedKeys, MANAGED_LOCKED_PROFILE_KEYS } from '../common/managed'; +import { + INSTANCE_API_KEY_NAMES, + readInstanceApiKey, + resolveApiKey, + writeInstanceApiKey, + type InstanceApiKeyName, +} from '../settings/instance-api-keys'; +import { SEARCH_TEXT_FIELD_MASK } from '../maps/maps.helpers'; import { User } from '../../types'; /** @@ -41,33 +49,109 @@ export class UserProfileService { return readEnv().managed.enabled; } + /** The three key columns plus the role that decides where a save lands. */ + private currentKeys(userId: number) { + return this.db.get>( + 'SELECT role, maps_api_key, openweather_api_key, unsplash_api_key FROM users WHERE id = ?', + userId + ); + } + + /** + * The value a save is measured against, in cleartext. + * + * For an admin and one of the two instance-wide names that is the instance + * value, because that is what the panel shows them and what the search uses; + * their own column only speaks when no instance value has been set yet. + */ + private storedKeyPlaintext( + name: 'maps_api_key' | 'openweather_api_key' | 'unsplash_api_key', + current: Pick | undefined, + isAdmin: boolean, + ): string { + if (isAdmin && (INSTANCE_API_KEY_NAMES as readonly string[]).includes(name)) { + const instance = readInstanceApiKey(this.db, name as InstanceApiKeyName); + if (instance !== null) return instance; + } + return decrypt_api_key(current?.[name]) ?? ''; + } + + /** + * Which key names this body actually changes. + * + * Compared on cleartext, never on what is stored: the IV is random, so the + * same key encrypted twice differs every time and a ciphertext comparison + * would report a change on every save. The panel saves before each test click, + * so that difference is the one between an audit trail and a full log. + * + * `skipped` are the names a managed install refuses to write — auditing them + * would claim a change that never happened. + */ + private changedKeyNames( + body: Record, + current: Pick | undefined, + isAdmin: boolean, + skipped: string[] = [], + ): string[] { + const norm = (v: unknown) => String(v ?? '').trim(); + return (['maps_api_key', 'openweather_api_key', 'unsplash_api_key'] as const).filter( + (name) => body[name] !== undefined && !skipped.includes(name) && norm(body[name]) !== norm(this.storedKeyPlaintext(name, current, isAdmin)) + ); + } + + /** + * Mirror the two instance-wide names into app_settings when an admin saves. + * + * Only an admin: `PUT /me/api-keys` is not admin-gated (the class carries + * JwtAuthGuard alone), so letting any caller write here would hand every + * member the instance credential. A non-admin keeps writing their own column, + * which is still the last step of the resolver. + */ + private mirrorInstanceKeys(body: Record, isAdmin: boolean): void { + if (!isAdmin) return; + for (const name of INSTANCE_API_KEY_NAMES) { + if (body[name] !== undefined) writeInstanceApiKey(this.db, name, body[name]); + } + } + updateMapsKey(userId: number, key: unknown) { const maps_api_key = key as string | null | undefined; if (this.managed) { - return { success: true, maps_api_key: null, managed_keys: ['maps_api_key'] }; + return { success: true, maps_api_key: null, managed_keys: ['maps_api_key'], changedKeys: [] }; } - this.db.run( - 'UPDATE users SET maps_api_key = ?, updated_at = CURRENT_TIMESTAMP WHERE id = ?', - maybe_encrypt_api_key(maps_api_key), userId - ); - return { success: true, maps_api_key: mask_stored_api_key(maps_api_key) }; + const current = this.currentKeys(userId); + const isAdmin = current?.role === 'admin'; + const changedKeys = this.changedKeyNames({ maps_api_key }, current, isAdmin); + this.db.transaction(() => { + this.db.run( + 'UPDATE users SET maps_api_key = ?, updated_at = CURRENT_TIMESTAMP WHERE id = ?', + maybe_encrypt_api_key(maps_api_key), userId + ); + this.mirrorInstanceKeys({ maps_api_key }, isAdmin); + }); + return { success: true, maps_api_key: mask_stored_api_key(maps_api_key), changedKeys }; } updateApiKeys(userId: number, rawBody: unknown) { const body = rawBody as { maps_api_key?: string; openweather_api_key?: string; unsplash_api_key?: string }; const { blocked } = splitManagedKeys(body, this.managed); for (const key of blocked) delete body[key as keyof typeof body]; - const current = this.db.get>('SELECT maps_api_key, openweather_api_key, unsplash_api_key FROM users WHERE id = ?', userId); + const current = this.currentKeys(userId); + const isAdmin = current?.role === 'admin'; + const changedKeys = this.changedKeyNames(body, current, isAdmin, blocked); - // `?? null` instead of the former non-null assertions: a user row deleted - // mid-request must degrade to a 0-row UPDATE, not a TypeError/500. - this.db.run( - 'UPDATE users SET maps_api_key = ?, openweather_api_key = ?, unsplash_api_key = ?, updated_at = CURRENT_TIMESTAMP WHERE id = ?', - body.maps_api_key !== undefined ? maybe_encrypt_api_key(body.maps_api_key) : current?.maps_api_key ?? null, - body.openweather_api_key !== undefined ? maybe_encrypt_api_key(body.openweather_api_key) : current?.openweather_api_key ?? null, - body.unsplash_api_key !== undefined ? maybe_encrypt_api_key(body.unsplash_api_key) : current?.unsplash_api_key ?? null, - userId - ); + this.db.transaction(() => { + // `?? null` instead of the former non-null assertions: a user row deleted + // mid-request must degrade to a 0-row UPDATE, not a TypeError/500. + this.db.run( + 'UPDATE users SET maps_api_key = ?, openweather_api_key = ?, unsplash_api_key = ?, updated_at = CURRENT_TIMESTAMP WHERE id = ?', + body.maps_api_key !== undefined ? maybe_encrypt_api_key(body.maps_api_key) : current?.maps_api_key ?? null, + body.openweather_api_key !== undefined ? maybe_encrypt_api_key(body.openweather_api_key) : current?.openweather_api_key ?? null, + body.unsplash_api_key !== undefined ? maybe_encrypt_api_key(body.unsplash_api_key) : current?.unsplash_api_key ?? null, + userId + ); + this.mirrorInstanceKeys(body, isAdmin); + }); const updated = this.db.get>( 'SELECT id, username, email, role, maps_api_key, openweather_api_key, unsplash_api_key, avatar, mfa_enabled FROM users WHERE id = ?', @@ -79,13 +163,14 @@ export class UserProfileService { success: true, ...(blocked.length ? { managed_keys: blocked } : {}), user: { ...u, maps_api_key: mask_stored_api_key(u?.maps_api_key), openweather_api_key: mask_stored_api_key(u?.openweather_api_key), unsplash_api_key: mask_stored_api_key(u?.unsplash_api_key), avatar_url: avatarUrl(updated || {}) }, + changedKeys, }; } updateSettings( userId: number, rawBody: unknown - ): { error?: string; status?: number; success?: boolean; user?: Record } { + ): { error?: string; status?: number; success?: boolean; user?: Record; changedKeys?: string[] } { const body = rawBody as { maps_api_key?: string; openweather_api_key?: string; unsplash_api_key?: string; username?: string; email?: string }; const { maps_api_key, openweather_api_key, unsplash_api_key, username, email } = body; @@ -124,10 +209,19 @@ export class UserProfileService { if (username !== undefined) { updates.push('username = ?'); params.push(username.trim()); } if (email !== undefined) { updates.push('email = ?'); params.push(email.trim()); } + // Read before the write, so the comparison sees the old value; the role in + // the same row decides whether the two instance-wide names travel with it. + const current = this.currentKeys(userId); + const isAdmin = current?.role === 'admin'; + const changedKeys = keyLocked ? [] : this.changedKeyNames(body, current, isAdmin, blocked); + if (updates.length > 0) { updates.push('updated_at = CURRENT_TIMESTAMP'); params.push(userId); - this.db.run(`UPDATE users SET ${updates.join(', ')} WHERE id = ?`, ...params); + this.db.transaction(() => { + this.db.run(`UPDATE users SET ${updates.join(', ')} WHERE id = ?`, ...params); + if (!keyLocked) this.mirrorInstanceKeys(body, isAdmin); + }); } const updated = this.db.get>( @@ -140,6 +234,7 @@ export class UserProfileService { success: true, ...(blocked.length ? { managed_keys: blocked } : {}), user: { ...u, maps_api_key: mask_stored_api_key(u?.maps_api_key), openweather_api_key: mask_stored_api_key(u?.openweather_api_key), unsplash_api_key: mask_stored_api_key(u?.unsplash_api_key), avatar_url: avatarUrl(updated || {}) }, + changedKeys, }; } @@ -166,11 +261,15 @@ export class UserProfileService { }; } + // Maps and Unsplash are read where the search reads them: instance-wide + // first. Showing the admin their own column while every request used another + // value is the confusion #1939 reported. Their column still answers while no + // instance value exists — on that install it is what the resolver picks too. return { settings: { - maps_api_key: decrypt_api_key(user.maps_api_key), + maps_api_key: readInstanceApiKey(this.db, 'maps_api_key') ?? decrypt_api_key(user.maps_api_key), openweather_api_key: decrypt_api_key(user.openweather_api_key), - unsplash_api_key: decrypt_api_key(user.unsplash_api_key), + unsplash_api_key: readInstanceApiKey(this.db, 'unsplash_api_key') ?? decrypt_api_key(user.unsplash_api_key), }, }; } @@ -226,7 +325,7 @@ export class UserProfileService { // ------------------------------------------------------------------------- async validateKeys(userId: number): Promise<{ error?: string; status?: number; maps: boolean; weather: boolean; maps_details: null | { ok: boolean; status: number | null; status_text: string | null; error_message: string | null; error_status: string | null; error_raw: string | null } }> { - const user = this.db.get>('SELECT role, maps_api_key, openweather_api_key FROM users WHERE id = ?', userId); + const user = this.db.get>('SELECT role, openweather_api_key FROM users WHERE id = ?', userId); if (user?.role !== 'admin') return { error: 'Admin access required', status: 403, maps: false, weather: false, maps_details: null }; const result: { @@ -242,7 +341,10 @@ export class UserProfileService { }; } = { maps: false, weather: false, maps_details: null }; - const maps_api_key = decrypt_api_key(user.maps_api_key); + // The key a search would actually use, not the one in this admin's column: + // testing a value nothing resolves to is how "the panel says the key is + // fine" and "every search 403s" coexisted (#1939). + const { key: maps_api_key } = resolveApiKey(this.db, 'maps_api_key', userId, readEnv().maps.placesApiKey); if (maps_api_key) { try { // Same Referer as maps.service googleFetch — without it, keys with an @@ -256,7 +358,9 @@ export class UserProfileService { ...(referer ? { Referer: referer } : {}), 'Content-Type': 'application/json', 'X-Goog-Api-Key': maps_api_key, - 'X-Goog-FieldMask': 'places.displayName', + // The mask the real search sends. A narrower probe passes on keys + // that are restricted to fewer Places SKUs than TREK asks for. + 'X-Goog-FieldMask': SEARCH_TEXT_FIELD_MASK, }, body: JSON.stringify({ textQuery: 'test' }), } diff --git a/server/src/nest/maps/maps.helpers.ts b/server/src/nest/maps/maps.helpers.ts index 69d4b25fe..d4b6f7bae 100644 --- a/server/src/nest/maps/maps.helpers.ts +++ b/server/src/nest/maps/maps.helpers.ts @@ -22,6 +22,17 @@ export function buildUserAgent(instanceUrl: string | undefined): string { // Computed once at load — getAppUrl() reads only env vars, which don't change at runtime. export const UA = buildUserAgent(getAppUrl()); +/** + * The fields places:searchText is asked for. + * + * Here rather than beside the call because the admin panel's key test has to + * send the same mask: a key restricted to a narrower set of Places SKUs answers + * a one-field probe with 200 and the real search with 403, which is the second + * way to reach the #1939 report ("test button green, searching fails"). + */ +export const SEARCH_TEXT_FIELD_MASK = + 'places.id,places.displayName,places.formattedAddress,places.location,places.rating,places.websiteUri,places.nationalPhoneNumber,places.types,places.googleMapsUri,places.businessStatus'; + // TREK's internal language codes mostly coincide with valid BCP-47 codes, but a // couple don't: 'br' is Brazilian Portuguese here (BCP-47 'pt-BR'; bare 'br' is // Breton) and 'gr' is Greek (BCP-47 'el'). Outbound geo APIs (Google Places, diff --git a/server/src/nest/maps/maps.service.ts b/server/src/nest/maps/maps.service.ts index 764071cf0..4657174f0 100644 --- a/server/src/nest/maps/maps.service.ts +++ b/server/src/nest/maps/maps.service.ts @@ -9,13 +9,14 @@ import type { } from '@trek/shared'; import { readEnv, getAppUrl } from '../../app-config'; import { safeFetchFollow, SsrfBlockedError } from '../../utils/ssrfGuard'; -import { decrypt_api_key } from '../common/crypto/apiKeyCrypto'; +import { resolveApiKey, type ApiKeySource } from '../settings/instance-api-keys'; // ── Photo cache (disk-backed) ──────────────────────────────────────────────── import { PlacePhotoCacheService } from '../place-photos/place-photo-cache.service'; import { DatabaseService } from '../database/database.service'; import { nominatimFetch, type GeoLane } from '../geo/nominatim.client'; import { UA, + SEARCH_TEXT_FIELD_MASK, toApiLang, googleFtidFromMapsUrl, buildOsmDetails, @@ -59,6 +60,18 @@ function placesEndpoint(endpoint: string): string { return base.replace(/\/+$/, '') + endpoint.slice(PLACES_UPSTREAM.length); } +/** + * Says which of the three credentials Google rejected, never which value. + * + * The response body Google sends ("The caller does not have permission") is + * identical whichever key was used, so without this line a report of "works for + * the admin, fails for everyone else" cannot be told apart from a genuinely + * broken key. + */ +function logKeyFailure(label: string, status: number, userId: number, source: ApiKeySource | null): void { + console.error(`[Maps] ${label} failed with ${status} userId=${userId} keySource=${source}`); +} + function googleFetch(rawEndpoint: string, label: string, init?: RequestInit): Promise { const endpoint = placesEndpoint(rawEndpoint); googleApiCallCount++; @@ -548,25 +561,25 @@ export class MapsService { // ── API key retrieval ────────────────────────────────────────────────────── - getMapsKey(userId: number): string | null { - // When the operator supplies the credential, the database is not asked at - // all. Two reasons, and the second is the load-bearing one: a per-user key - // would route around whatever the operator's endpoint counts, and the key - // columns are readable by any admin of this instance, so a shared key stored - // there would be a key handed out. Unset, this branch never runs. - const operatorKey = readEnv().maps.placesApiKey; - if (operatorKey) return operatorKey; + /** + * The Places credential for this request, and where it came from. + * + * Operator env first: a per-user key would route around whatever the + * operator's endpoint counts, and unset, that branch never runs. Then the + * instance-wide value the admin panel writes, then the caller's own row. + * + * What is deliberately gone is the old third step, "any admin's key" (#1939): + * it read a stranger's credential, which server/CLAUDE.md forbids, and made + * the answer depend on who was asking — the saving admin got their own key, + * everybody else got the lowest-id admin's and a 403 from Google. The source + * is returned so a provider error can say which of the three was used. + */ + resolveMapsKey(userId: number): { key: string | null; source: ApiKeySource | null } { + return resolveApiKey(this.database, 'maps_api_key', userId, readEnv().maps.placesApiKey); + } - const user = this.database.get<{ maps_api_key: string | null }>( - 'SELECT maps_api_key FROM users WHERE id = ?', - userId, - ); - const user_key = decrypt_api_key(user?.maps_api_key); - if (user_key) return user_key; - const admin = this.database.get<{ maps_api_key: string }>( - "SELECT maps_api_key FROM users WHERE role = 'admin' AND maps_api_key IS NOT NULL AND maps_api_key != '' LIMIT 1", - ); - return decrypt_api_key(admin?.maps_api_key) || null; + getMapsKey(userId: number): string | null { + return this.resolveMapsKey(userId).key; } // ── Nominatim search ─────────────────────────────────────────────────────── @@ -1417,7 +1430,7 @@ export class MapsService { lang?: string, locationBias?: { lat: number; lng: number; radius?: number }, ): Promise<{ places: Record[]; source: string }> { - const apiKey = this.getMapsKey(userId); + const { key: apiKey, source: keySource } = this.resolveMapsKey(userId); if (!apiKey) { const places = await this.searchNominatim(query, lang); @@ -1441,8 +1454,7 @@ export class MapsService { headers: { 'Content-Type': 'application/json', 'X-Goog-Api-Key': apiKey, - 'X-Goog-FieldMask': - 'places.id,places.displayName,places.formattedAddress,places.location,places.rating,places.websiteUri,places.nationalPhoneNumber,places.types,places.googleMapsUri,places.businessStatus', + 'X-Goog-FieldMask': SEARCH_TEXT_FIELD_MASK, }, body: JSON.stringify(searchBody), }); @@ -1450,6 +1462,7 @@ export class MapsService { const data = (await response.json()) as { places?: GooglePlaceResult[]; error?: { message?: string } }; if (!response.ok) { + logKeyFailure('searchText', response.status, userId, keySource); const err = new Error(data.error?.message || 'Google Places API error') as Error & { status: number }; err.status = response.status; throw err; @@ -1488,7 +1501,7 @@ export class MapsService { locationBias?: { low: { lat: number; lng: number }; high: { lat: number; lng: number } }, sessionToken?: string, ): Promise<{ suggestions: { placeId: string; mainText: string; secondaryText: string }[]; source: string }> { - const apiKey = this.getMapsKey(userId); + const { key: apiKey, source: keySource } = this.resolveMapsKey(userId); if (!apiKey) { return this.autocompleteNominatim(input, lang); @@ -1526,6 +1539,7 @@ export class MapsService { }; if (!response.ok) { + logKeyFailure('autocomplete', response.status, userId, keySource); const err = new Error(data.error?.message || 'Google Places Autocomplete error') as Error & { status: number }; err.status = response.status; throw err; diff --git a/server/src/nest/settings/instance-api-keys.ts b/server/src/nest/settings/instance-api-keys.ts new file mode 100644 index 000000000..a5c4d4bd9 --- /dev/null +++ b/server/src/nest/settings/instance-api-keys.ts @@ -0,0 +1,89 @@ +import { DatabaseService } from '../database/database.service'; +import { decrypt_api_key, maybe_encrypt_api_key } from '../common/crypto/apiKeyCrypto'; + +/** + * The third-party keys that belong to the instance rather than to a person. + * + * Google Places and Unsplash bill the install, not the account that pasted the + * key in, so the value is instance configuration — the same argument #1772 made + * for the LLM endpoint (see llm-parse/llm-config.ts: instance-wide, admin-set, + * wins; per-user as the fallback). Both keys used to be resolved out of the + * `users` columns alone, which meant "the key of the admin with the lowest id" + * for everyone else and produced #1939: the admin searched with the key they + * had just saved, every other member searched with a stranger's and got a 403 + * from Google. + * + * Stored as an encrypted `app_settings` row under the name below, reusing + * apiKeyCrypto, so the format matches what the users columns already hold and + * a legacy plaintext value still reads back. + */ +export type InstanceApiKeyName = 'maps_api_key' | 'unsplash_api_key'; + +/** Instance names whose per-user column is still honoured as a last resort. */ +export const INSTANCE_API_KEY_NAMES: readonly InstanceApiKeyName[] = ['maps_api_key', 'unsplash_api_key']; + +/** + * Where a resolved key came from. Logged beside a provider error so "works for + * the admin, 403 for everyone else" is one line in the log rather than a + * guessing game; the key itself is never logged. + */ +export type ApiKeySource = 'operator-env' | 'instance' | 'user-row'; + +// Full statements rather than an interpolated column: the name doubles as the +// users column AND the app_settings key, and identifiers only ever come from a +// literal allow-list. +const USER_ROW_SQL: Record = { + maps_api_key: 'SELECT maps_api_key FROM users WHERE id = ?', + unsplash_api_key: 'SELECT unsplash_api_key FROM users WHERE id = ?', +}; + +/** The instance-wide value in cleartext, or null when unset/cleared. */ +export function readInstanceApiKey(db: DatabaseService, name: InstanceApiKeyName): string | null { + const row = db.get<{ value: string | null }>('SELECT value FROM app_settings WHERE key = ?', name); + if (!row?.value) return null; + return decrypt_api_key(row.value) || null; +} + +/** + * Store the instance-wide value, encrypted. A blank value stores '' rather than + * deleting the row: an admin who clears the field means "this instance has no + * key", and a missing row would let the resolver fall through to whatever old + * value still sits in their own users column. + */ +export function writeInstanceApiKey(db: DatabaseService, name: InstanceApiKeyName, value: unknown): void { + db.run( + `INSERT INTO app_settings (key, value) VALUES (?, ?) + ON CONFLICT(key) DO UPDATE SET value = excluded.value`, + name, maybe_encrypt_api_key(value) ?? '' + ); +} + +/** + * Resolve a key for one request: operator env → instance-wide → the caller's + * own row. + * + * The caller's own row is last, not second, and no other user's row is ever + * read — falling back to a stranger's key is what server/CLAUDE.md forbids and + * what #1939 reported. It stays as a fallback because `PUT /me/api-keys` has + * always accepted a personal key and dropping it would break the installs where + * each member pays for their own. + * + * `userId` 0 means "nobody is asking" (the unauthenticated app-config read): + * there is no personal key to find, so the chain ends at the instance. + */ +export function resolveApiKey( + db: DatabaseService, + name: InstanceApiKeyName, + userId: number, + operatorKey: string | undefined, +): { key: string | null; source: ApiKeySource | null } { + if (operatorKey) return { key: operatorKey, source: 'operator-env' }; + + const instance = readInstanceApiKey(db, name); + if (instance) return { key: instance, source: 'instance' }; + if (!userId) return { key: null, source: null }; + + const row = db.get>(USER_ROW_SQL[name], userId); + const own = decrypt_api_key(row?.[name]) || null; + return own ? { key: own, source: 'user-row' } : { key: null, source: null }; +} diff --git a/server/src/nest/unsplash/unsplash.service.ts b/server/src/nest/unsplash/unsplash.service.ts index b002b29e7..73df46489 100644 --- a/server/src/nest/unsplash/unsplash.service.ts +++ b/server/src/nest/unsplash/unsplash.service.ts @@ -3,7 +3,7 @@ import fs from 'fs'; import { v4 as uuidv4 } from 'uuid'; import { Injectable } from '@nestjs/common'; import { safeFetch } from '../../utils/ssrfGuard'; -import { decrypt_api_key } from '../common/crypto/apiKeyCrypto'; +import { resolveApiKey } from '../settings/instance-api-keys'; import { DatabaseService } from '../database/database.service'; import { RuntimeEnvService } from '../app-config/runtime-env.service'; @@ -42,7 +42,7 @@ const COVER_EXT_BY_TYPE: Record = { * Unsplash search and cover download. * * Its own module rather than a method on trips or places: both call it, and the - * key resolution reads the env AND the users table, so it needs the injected + * key resolution reads the env AND the database, so it needs the injected * connection either way. */ @Injectable() @@ -55,18 +55,14 @@ export class UnsplashService { /** * Resolve an Unsplash access key for a request, in precedence order: * the `UNSPLASH_ACCESS_KEY` env var (instance-wide override, matching the - * SMTP/OIDC env-over-DB convention) → the caller's own stored key → any - * admin's stored key. Mirrors `getMapsKey`. Returns null when none is set, - * in which case the search falls back to the unauthenticated endpoint. + * SMTP/OIDC env-over-DB convention) → the instance-wide value the admin panel + * saves → the caller's own stored key. Mirrors `resolveMapsKey`, including what + * it dropped: "any admin's stored key" read a stranger's credential and made + * the answer depend on who asked (#1939). Returns null when none is set, in + * which case the search falls back to the unauthenticated endpoint. */ getUnsplashKey(userId: number): string | null { - const env_key = this.env.env().integrations.unsplashAccessKey; - if (env_key) return env_key; - const user = this.db.get<{ unsplash_api_key: string | null }>('SELECT unsplash_api_key FROM users WHERE id = ?', userId); - const user_key = decrypt_api_key(user?.unsplash_api_key); - if (user_key) return user_key; - const admin = this.db.get<{ unsplash_api_key: string }>("SELECT unsplash_api_key FROM users WHERE role = 'admin' AND unsplash_api_key IS NOT NULL AND unsplash_api_key != '' LIMIT 1"); - return decrypt_api_key(admin?.unsplash_api_key) || null; + return resolveApiKey(this.db, 'unsplash_api_key', userId, this.env.env().integrations.unsplashAccessKey).key; } async searchUnsplashPhotos(query: string, perPage = 9, accessKey?: string | null) { diff --git a/server/tests/e2e/auth.e2e.test.ts b/server/tests/e2e/auth.e2e.test.ts index c1bd09cdf..7ef0f1b49 100644 --- a/server/tests/e2e/auth.e2e.test.ts +++ b/server/tests/e2e/auth.e2e.test.ts @@ -45,6 +45,7 @@ import { MailerService } from '../../src/nest/notifications/mailer/mailer.servic import { createTables } from '../../src/db/schema'; import { runMigrations } from '../../src/db/migrations'; import { createUser } from '../helpers/factories'; +import { encrypt_api_key } from '../../src/nest/common/crypto/apiKeyCrypto'; import { resetRateLimits } from '../helpers/test-db'; import { AuthModule } from '../../src/nest/auth/auth.module'; import { AuthService } from '../../src/nest/auth/auth.service'; @@ -251,6 +252,80 @@ describe('Auth e2e (real auth guard + real service + real cookie service + temp expect(res.body.error).toMatch(/password/i); }); + it('PUT /me/api-keys stores the key instance-wide and audits it once (#1939)', async () => { + const admin = createUser(db as never, { username: 'keys-admin', email: 'keys-admin@example.test', role: 'admin' }); + const before = auditRows('settings.api_keys_update'); + + const res = await request(server) + .put('/api/auth/me/api-keys') + .set('Cookie', sessionCookie(admin.user.id)) + .send({ maps_api_key: 'e2e-google-key' }); + expect(res.status).toBe(200); + expect(res.body.success).toBe(true); + // Masked in the response and never accompanied by the audit bookkeeping. + expect(res.body.user.maps_api_key).not.toBe('e2e-google-key'); + expect(res.body).not.toHaveProperty('changedKeys'); + + // What every member's search will now resolve to, encrypted at rest. + const stored = db.prepare("SELECT value FROM app_settings WHERE key = 'maps_api_key'").get() as { value: string }; + expect(stored.value).toMatch(/^enc:v1:/); + // And the admin panel reads back what the search uses. + const settings = await request(server).get('/api/auth/me/settings').set('Cookie', sessionCookie(admin.user.id)); + expect(settings.body.settings.maps_api_key).toBe('e2e-google-key'); + + expect(auditRows('settings.api_keys_update')).toBe(before + 1); + const row = db + .prepare("SELECT resource, details FROM audit_log WHERE action = 'settings.api_keys_update' ORDER BY id DESC LIMIT 1") + .get() as { resource: string; details: string }; + expect(row.resource).toBe('api_keys'); + expect(row.details).toContain('maps_api_key'); + expect(row.details).not.toContain('e2e-google-key'); + + // Saving the same value again is not a change, so the log does not grow. + await request(server) + .put('/api/auth/me/api-keys') + .set('Cookie', sessionCookie(admin.user.id)) + .send({ maps_api_key: 'e2e-google-key' }); + expect(auditRows('settings.api_keys_update')).toBe(before + 1); + }, 10000); + + it('GET /app-config answers has_maps_key from the instance row, never from an admin column (#1939)', async () => { + const member = createUser(db as never, { username: 'keys-member', email: 'keys-member@example.test' }); + const admin = createUser(db as never, { username: 'keys-cfg-admin', email: 'keys-cfg-admin@example.test', role: 'admin' }); + // Seeded here instead of riding on the save above, so running this case on + // its own asserts the same thing. + const setInstanceKey = (value: string | null) => { + if (value === null) { + db.prepare("DELETE FROM app_settings WHERE key = 'maps_api_key'").run(); + return; + } + db.prepare( + `INSERT INTO app_settings (key, value) VALUES ('maps_api_key', ?) + ON CONFLICT(key) DO UPDATE SET value = excluded.value` + ).run(encrypt_api_key(value)); + }; + const hasMapsKey = async (userId: number) => { + const res = await request(server).get('/api/auth/app-config').set('Cookie', sessionCookie(userId)); + expect(res.status).toBe(200); + return res.body.has_maps_key; + }; + + setInstanceKey('instance-configured-key'); + db.prepare('UPDATE users SET maps_api_key = NULL WHERE id = ?').run(admin.user.id); + // Instance key, no column anywhere: the member searches with it, so the + // client is told the feature is there. + expect(await hasMapsKey(member.user.id)).toBe(true); + + // The reported half of #1939: the key exists only in one admin's column. + // It is not the member's to spend, so they are told there is none instead + // of being offered a Google search that answers 403. + setInstanceKey(null); + db.prepare('UPDATE users SET maps_api_key = ? WHERE id = ?').run(encrypt_api_key('admins-own-key'), admin.user.id); + expect(await hasMapsKey(member.user.id)).toBe(false); + // That same column still counts for the admin themselves. + expect(await hasMapsKey(admin.user.id)).toBe(true); + }); + it('POST /logout clears the session cookie', async () => { const res = await request(server).post('/api/auth/logout'); expect(res.status).toBe(200); diff --git a/server/tests/integration/leg-mode-incoming.test.ts b/server/tests/integration/leg-mode-incoming.test.ts index eabe889dd..a1e7a328d 100644 --- a/server/tests/integration/leg-mode-incoming.test.ts +++ b/server/tests/integration/leg-mode-incoming.test.ts @@ -43,8 +43,9 @@ describe('incoming_leg_transport_mode migration', () => { // // >>> Appending a migration? Re-point the "undo" below at whatever yours // >>> does. That is the whole maintenance cost of this guard. The current - // >>> trailing slot deletes reservation_day_positions rows whose reservation - // >>> and day sit on different trips, so undoing it means putting one back. + // >>> trailing slot copies the lowest-id admin's Google and Unsplash keys + // >>> into app_settings, so undoing it means clearing those rows and + // >>> leaving an admin holding a key for it to find again. const upgraded = new Database(':memory:'); upgraded.exec('PRAGMA foreign_keys = ON'); createTables(upgraded); @@ -52,23 +53,30 @@ describe('incoming_leg_transport_mode migration', () => { const { version } = upgraded.prepare('SELECT version FROM schema_version').get() as { version: number }; - upgraded.prepare("INSERT INTO users (id, username, email, password_hash) VALUES (1, 'a', 'a@test.local', 'x')").run(); - upgraded.prepare("INSERT INTO users (id, username, email, password_hash) VALUES (2, 'b', 'b@test.local', 'x')").run(); - const tripA = upgraded.prepare("INSERT INTO trips (user_id, title) VALUES (1, 'A')").run().lastInsertRowid; - const tripB = upgraded.prepare("INSERT INTO trips (user_id, title) VALUES (2, 'B')").run().lastInsertRowid; - const reservationOnA = upgraded.prepare("INSERT INTO reservations (trip_id, title, type) VALUES (?, 'r', 'other')").run(tripA).lastInsertRowid; - const dayOnA = upgraded.prepare("INSERT INTO days (trip_id, day_number, date) VALUES (?, 1, '2026-01-01')").run(tripA).lastInsertRowid; - const dayOnB = upgraded.prepare("INSERT INTO days (trip_id, day_number, date) VALUES (?, 1, '2026-01-01')").run(tripB).lastInsertRowid; - const insertPosition = upgraded.prepare('INSERT INTO reservation_day_positions (reservation_id, day_id, position) VALUES (?, ?, ?)'); - insertPosition.run(reservationOnA, dayOnB, 1); // the mismatched pair the migration clears - insertPosition.run(reservationOnA, dayOnA, 2); // a legitimate one it must leave alone + upgraded.prepare("DELETE FROM app_settings WHERE key IN ('maps_api_key', 'unsplash_api_key')").run(); + upgraded + .prepare( + `INSERT INTO users (id, username, email, password_hash, role, maps_api_key) + VALUES (2, 'b', 'b@test.local', 'x', 'admin', NULL)` + ) + .run(); + // The only key on the install, which is the one condition under which the + // migration promotes it at all; the rest of that decision is pinned in + // tests/unit/db/instance-api-key-backfill.test.ts. + upgraded + .prepare( + `INSERT INTO users (id, username, email, password_hash, role, maps_api_key) + VALUES (1, 'a', 'a@test.local', 'x', 'admin', 'key-of-the-first-admin')` + ) + .run(); upgraded.prepare('UPDATE schema_version SET version = ?').run(version - 1); runMigrations(upgraded); - const rows = upgraded.prepare('SELECT day_id FROM reservation_day_positions ORDER BY day_id').all() as { day_id: number }[]; - expect(rows).toEqual([{ day_id: Number(dayOnA) }]); + expect(upgraded.prepare("SELECT value FROM app_settings WHERE key = 'maps_api_key'").get()).toEqual({ + value: 'key-of-the-first-admin', + }); expect(upgraded.prepare('SELECT version FROM schema_version').get()).toEqual({ version }); upgraded.close(); }); diff --git a/server/tests/integration/security.test.ts b/server/tests/integration/security.test.ts index 7745e3c16..6241c53fb 100644 --- a/server/tests/integration/security.test.ts +++ b/server/tests/integration/security.test.ts @@ -136,6 +136,35 @@ describe('API key encryption', () => { expect(row.openweather_api_key).toMatch(/^enc:v1:/); }); + it('SEC-008 — saving keys audits the names and never the values (#1939)', async () => { + const { user } = createUser(testDb); + + const save = () => + request(app) + .put('/api/auth/me/api-keys') + .set('Cookie', authCookie(user.id)) + .send({ openweather_api_key: 'test-api-key-12345' }); + + const first = await save(); + expect(first.status).toBe(200); + // changedKeys is internal: the client body is what it always was. + expect(first.body).not.toHaveProperty('changedKeys'); + + const rows = () => + testDb + .prepare("SELECT details FROM audit_log WHERE action = 'settings.api_keys_update'") + .all() as { details: string | null }[]; + expect(rows()).toHaveLength(1); + expect(rows()[0].details).toContain('openweather_api_key'); + expect(rows()[0].details).not.toContain('test-api-key-12345'); + + // The same value again writes no second row. This is the real test of the + // cleartext comparison: encryption uses a random IV, so the stored blob + // differs on every save even when the key does not. + await save(); + expect(rows()).toHaveLength(1); + }); + it('SEC-008 — GET /api/auth/me does not return plaintext API key', async () => { const { user } = createUser(testDb); await request(app) diff --git a/server/tests/unit/db/instance-api-key-backfill.test.ts b/server/tests/unit/db/instance-api-key-backfill.test.ts new file mode 100644 index 000000000..a8be1280a --- /dev/null +++ b/server/tests/unit/db/instance-api-key-backfill.test.ts @@ -0,0 +1,141 @@ +/** + * Unit test for the instance API-key backfill migration (#1939). + * + * The Places/Unsplash credential moved from "whichever admin row the resolver + * happened to find" to an instance-wide app_settings row. An install that + * upgrades must keep searching with the key it already had, so the migration + * copies the value the old resolver would have handed out — the lowest-id admin + * who has one — and leaves the users columns alone. It only does so where that + * value was the whole install's key: as soon as a second row holds one, the + * promotion is skipped, because the instance row outranks everybody's own + * column and nobody may be moved onto a stranger's key by an upgrade. + * + * Seeded before runMigrations rather than by rewinding schema_version + * afterwards: createTables already brings users.role and both key columns, so + * the backfill fires on its normal pass and finds the rows. That keeps this + * file independent of where the entry sits in the append-only array, so + * appending the next migration does not drag it along. The one guard that does + * have to track the tail is tests/integration/leg-mode-incoming.test.ts, which + * says so in place. + */ +import { describe, it, expect } from 'vitest'; +import Database from 'better-sqlite3'; +import { createTables } from '../../../src/db/schema'; +import { runMigrations } from '../../../src/db/migrations'; + +function freshDb() { + const db = new Database(':memory:'); + db.exec('PRAGMA journal_mode = WAL'); + db.exec('PRAGMA foreign_keys = ON'); + createTables(db); + return db; +} + +function seedUser( + db: Database.Database, + id: number, + role: 'admin' | 'user', + keys: { maps?: string; unsplash?: string } = {}, +) { + db.prepare( + `INSERT INTO users (id, username, email, password_hash, role, maps_api_key, unsplash_api_key) + VALUES (?, ?, ?, 'x', ?, ?, ?)` + ).run(id, `u${id}`, `u${id}@test.local`, role, keys.maps ?? null, keys.unsplash ?? null); +} + +const setting = (db: Database.Database, key: string) => + (db.prepare('SELECT value FROM app_settings WHERE key = ?').get(key) as { value: string } | undefined)?.value; + +describe('instance API-key backfill migration', () => { + it('KEYFILL-001: copies the one key the install was searching with and keeps the column', () => { + const db = freshDb(); + seedUser(db, 1, 'admin', { maps: 'the-admin-google', unsplash: 'the-admin-unsplash' }); + seedUser(db, 2, 'user'); + + runMigrations(db); + + expect(setting(db, 'maps_api_key')).toBe('the-admin-google'); + expect(setting(db, 'unsplash_api_key')).toBe('the-admin-unsplash'); + // Nothing is taken away: the columns are still the per-user fallback. + const row = db.prepare('SELECT maps_api_key FROM users WHERE id = 1').get() as { maps_api_key: string }; + expect(row.maps_api_key).toBe('the-admin-google'); + db.close(); + }); + + it('KEYFILL-002: skips an admin whose column is empty and takes the next one', () => { + const db = freshDb(); + seedUser(db, 1, 'admin', { maps: '' }); + seedUser(db, 2, 'admin', { maps: 'the-only-real-key' }); + + runMigrations(db); + + expect(setting(db, 'maps_api_key')).toBe('the-only-real-key'); + db.close(); + }); + + it('KEYFILL-003: never overwrites a value the admin has already saved instance-wide', () => { + const db = freshDb(); + seedUser(db, 1, 'admin', { maps: 'old-column-key' }); + db.prepare("INSERT INTO app_settings (key, value) VALUES ('maps_api_key', 'already-instance-wide')").run(); + + runMigrations(db); + + expect(setting(db, 'maps_api_key')).toBe('already-instance-wide'); + db.close(); + }); + + it('KEYFILL-004: ignores a non-admin key and writes no row at all', () => { + const db = freshDb(); + seedUser(db, 1, 'user', { maps: 'members-own-key' }); + + runMigrations(db); + + // A member's key stays theirs — the resolver still finds it for them, and + // promoting it would hand their billing to the whole instance. + expect(setting(db, 'maps_api_key')).toBeUndefined(); + db.close(); + }); + + it('KEYFILL-005: writes nothing on an install that never had a key', () => { + const db = freshDb(); + seedUser(db, 1, 'admin'); + + runMigrations(db); + + expect(setting(db, 'maps_api_key')).toBeUndefined(); + expect(setting(db, 'unsplash_api_key')).toBeUndefined(); + db.close(); + }); + + it('KEYFILL-006: leaves a column alone once a member holds a key of their own', () => { + const db = freshDb(); + seedUser(db, 1, 'admin', { maps: 'admins-own-google', unsplash: 'admins-own-unsplash' }); + seedUser(db, 7, 'user', { maps: 'members-own-google' }); + + runMigrations(db); + + // Member 7 pays for their own Google key. Promoting the admin's would put + // every one of their searches on his key and his bill, because the instance + // row is resolved before their own column. + expect(setting(db, 'maps_api_key')).toBeUndefined(); + // Decided per column: nobody else has an Unsplash key, so that one really + // was the whole install's and stays it. + expect(setting(db, 'unsplash_api_key')).toBe('admins-own-unsplash'); + const row = db.prepare('SELECT maps_api_key FROM users WHERE id = 7').get() as { maps_api_key: string }; + expect(row.maps_api_key).toBe('members-own-google'); + db.close(); + }); + + it('KEYFILL-007: two admins with their own keys both keep them', () => { + const db = freshDb(); + seedUser(db, 1, 'admin', { maps: 'first-admin-google' }); + seedUser(db, 2, 'admin', { maps: 'second-admin-google' }); + + runMigrations(db); + + expect(setting(db, 'maps_api_key')).toBeUndefined(); + const rows = db.prepare('SELECT maps_api_key FROM users ORDER BY id').all() as { maps_api_key: string }[]; + expect(rows.map((r) => r.maps_api_key)).toEqual(['first-admin-google', 'second-admin-google']); + db.close(); + }); +}); diff --git a/server/tests/unit/nest/audit.service.test.ts b/server/tests/unit/nest/audit.service.test.ts index f64c320ad..3c9499218 100644 --- a/server/tests/unit/nest/audit.service.test.ts +++ b/server/tests/unit/nest/audit.service.test.ts @@ -170,6 +170,24 @@ describe('writeAudit', () => { testDb.prepare('ALTER TABLE audit_log_gone RENAME TO audit_log').run(); }); + it('AUDIT-SVC-018: settings.api_keys_update names the changed keys and nothing else (#1939)', () => { + seedUser(1, 'admin@b.c'); + svc.writeAudit({ + userId: 1, + action: 'settings.api_keys_update', + resource: 'api_keys', + details: { changed: ['maps_api_key', 'unsplash_api_key'] }, + ip: '1.2.3.4', + }); + expect(logInfo).toHaveBeenLastCalledWith('admin@b.c updated API keys (maps_api_key, unsplash_api_key) ip=1.2.3.4'); + // A details blob without the array (or with an empty one) leaves the brief off + // rather than stringifying whatever else is in there. + svc.writeAudit({ userId: 1, action: 'settings.api_keys_update', details: { changed: [] }, ip: '1.2.3.4' }); + expect(logInfo).toHaveBeenLastCalledWith('admin@b.c updated API keys ip=1.2.3.4'); + svc.writeAudit({ userId: 1, action: 'settings.api_keys_update', details: { other: 'x' }, ip: '1.2.3.4' }); + expect(logInfo).toHaveBeenLastCalledWith('admin@b.c updated API keys ip=1.2.3.4'); + }); + it('AUDIT-SVC-014: buildInfoSummary variants (settings parts, login empty brief)', () => { seedUser(1, 'a@b.c'); svc.writeAudit({ userId: 1, action: 'settings.app_update', details: { notification_channel: 'smtp', require_mfa: false }, ip: '1.1.1.1' }); diff --git a/server/tests/unit/nest/auth.controller.test.ts b/server/tests/unit/nest/auth.controller.test.ts index 23a26b742..e3eb5a23c 100644 --- a/server/tests/unit/nest/auth.controller.test.ts +++ b/server/tests/unit/nest/auth.controller.test.ts @@ -289,15 +289,52 @@ describe('AuthController (authenticated)', () => { it('maps-key + api-keys pass straight through to the service', () => { const updateMapsKey = vi.fn().mockReturnValue({ success: true }); - expect(ac(asvc({}), rl(), {}, { updateMapsKey }).mapsKey(user, { maps_api_key: 'k' })).toEqual({ success: true }); + expect(ac(asvc({}), rl(), {}, { updateMapsKey }).mapsKey(user, { maps_api_key: 'k' }, req)).toEqual({ success: true }); expect(updateMapsKey).toHaveBeenCalledWith(1, 'k'); const updateApiKeys = vi.fn().mockReturnValue({ ok: 1 }); - expect(ac(asvc({}), rl(), {}, { updateApiKeys }).apiKeys(user, anyBody({ a: 1 } as never))).toEqual({ ok: 1 }); + expect(ac(asvc({}), rl(), {}, { updateApiKeys }).apiKeys(user, anyBody({ a: 1 } as never), req)).toEqual({ ok: 1 }); + // No changedKeys from the service (and none in the body): nothing to audit. + expect(writeAudit).not.toHaveBeenCalled(); + }); + + it('api-keys audits one settings.api_keys_update with the changed names only (#1939)', () => { + const updateApiKeys = vi.fn().mockReturnValue({ success: true, user: { id: 1 }, changedKeys: ['maps_api_key'] }); + const c = ac(asvc({}), rl(), {}, { updateApiKeys }); + // changedKeys is stripped off the response — the client body is unchanged. + expect(c.apiKeys(user, anyBody({ maps_api_key: 'AIza-super-secret' } as never), req)).toEqual({ success: true, user: { id: 1 } }); + expect(writeAudit).toHaveBeenCalledTimes(1); + expect(writeAudit).toHaveBeenCalledWith({ + userId: 1, + action: 'settings.api_keys_update', + resource: 'api_keys', + ip: '1.2.3.4', + details: { changed: ['maps_api_key'] }, + }); + // Never the value, not even truncated: details is rendered raw in the panel. + expect(JSON.stringify(writeAudit.mock.calls)).not.toContain('AIza-super-secret'); + }); + + it('maps-key and settings audit the same action; an unchanged save writes nothing', () => { + const mapsC = ac(asvc({}), rl(), {}, { updateMapsKey: vi.fn().mockReturnValue({ success: true, changedKeys: ['maps_api_key'] }) }); + mapsC.mapsKey(user, { maps_api_key: 'k' }, req); + expect(writeAudit).toHaveBeenCalledWith(expect.objectContaining({ action: 'settings.api_keys_update', details: { changed: ['maps_api_key'] } })); + + writeAudit.mockClear(); + const setC = ac(asvc({}), rl(), {}, { updateSettings: vi.fn().mockReturnValue({ success: true, user: { id: 1 }, changedKeys: ['unsplash_api_key'] }) }); + expect(setC.updateSettings(user, {}, req)).toEqual({ success: true, user: { id: 1 } }); + expect(writeAudit).toHaveBeenCalledWith(expect.objectContaining({ action: 'settings.api_keys_update', details: { changed: ['unsplash_api_key'] } })); + + // The test button in the panel saves before every click — an identical value + // must not cost a log line. + writeAudit.mockClear(); + ac(asvc({}), rl(), {}, { updateApiKeys: vi.fn().mockReturnValue({ success: true, changedKeys: [] }) }) + .apiKeys(user, anyBody({ maps_api_key: 'same' } as never), req); + expect(writeAudit).not.toHaveBeenCalled(); }); it('update-settings + get-settings map errors, else return their payloads', () => { - expect(thrown(() => ac(asvc({}), rl(), {}, { updateSettings: vi.fn().mockReturnValue({ error: 'Bad', status: 400 }) }).updateSettings(user, {}))).toEqual({ status: 400, body: { error: 'Bad' } }); - expect(ac(asvc({}), rl(), {}, { updateSettings: vi.fn().mockReturnValue({ success: true, user: { id: 1 } }) }).updateSettings(user, {})).toEqual({ success: true, user: { id: 1 } }); + expect(thrown(() => ac(asvc({}), rl(), {}, { updateSettings: vi.fn().mockReturnValue({ error: 'Bad', status: 400 }) }).updateSettings(user, {}, req))).toEqual({ status: 400, body: { error: 'Bad' } }); + expect(ac(asvc({}), rl(), {}, { updateSettings: vi.fn().mockReturnValue({ success: true, user: { id: 1 } }) }).updateSettings(user, {}, req)).toEqual({ success: true, user: { id: 1 } }); expect(thrown(() => ac(asvc({}), rl(), {}, { getSettings: vi.fn().mockReturnValue({ error: 'Nope', status: 404 }) }).getSettings(user))).toEqual({ status: 404, body: { error: 'Nope' } }); expect(ac(asvc({}), rl(), {}, { getSettings: vi.fn().mockReturnValue({ settings: { theme: 'dark' } }) }).getSettings(user)).toEqual({ settings: { theme: 'dark' } }); }); diff --git a/server/tests/unit/nest/maps.service.test.ts b/server/tests/unit/nest/maps.service.test.ts index 6920b189f..2161148b9 100644 --- a/server/tests/unit/nest/maps.service.test.ts +++ b/server/tests/unit/nest/maps.service.test.ts @@ -38,6 +38,8 @@ type SsrfCheckStub = Pick; const { mockDbGet, mockDbRun, + mockInstanceGet, + preparedSql, mockCheckSsrf, mockCacheGet, mockCacheGetErrored, @@ -47,8 +49,14 @@ const { mockCacheSetInFlight, mockServeFilePath, } = vi.hoisted(() => ({ - mockDbGet: vi.fn(() => undefined as any), + mockDbGet: vi.fn((..._args: unknown[]) => undefined as any), mockDbRun: vi.fn(), + // The instance-wide key row (#1939) is read before the caller's own row, so it + // gets its own seam: every case below that stubs mockDbGet means "this user's + // row holds a key", and would otherwise have its stub eaten by the + // app_settings lookup. + mockInstanceGet: vi.fn((..._args: unknown[]) => undefined as any), + preparedSql: [] as string[], mockCheckSsrf: vi.fn(async (_url: string, _bypassInternalIpAllowed?: boolean): Promise => ({ allowed: true, })), @@ -69,7 +77,14 @@ const { vi.mock('../../../src/db/database', () => ({ db: { - prepare: () => ({ get: mockDbGet, all: vi.fn(() => []), run: mockDbRun }), + prepare: (sql: string) => { + preparedSql.push(sql); + return { + get: (...args: unknown[]) => (sql.includes('app_settings') ? mockInstanceGet(...args) : mockDbGet(...args)), + all: vi.fn(() => []), + run: mockDbRun, + }; + }, }, })); @@ -96,6 +111,8 @@ vi.mock('../../../src/utils/ssrfGuard', () => { vi.mock('../../../src/nest/common/crypto/apiKeyCrypto', () => ({ decrypt_api_key: (v: string | null) => v, + // Unused by the read paths here, but instance-api-keys imports it. + maybe_encrypt_api_key: (v: string | null) => v, })); vi.mock('../../../src/config', () => ({ @@ -131,6 +148,9 @@ afterEach(() => { vi.unstubAllGlobals(); mockDbGet.mockReset(); mockDbGet.mockReturnValue(undefined); + mockInstanceGet.mockReset(); + mockInstanceGet.mockReturnValue(undefined); + preparedSql.length = 0; mockDbRun.mockReset(); mockCheckSsrf.mockReset(); mockCheckSsrf.mockResolvedValue({ allowed: true }); @@ -392,24 +412,48 @@ describe('buildOsmDetails', () => { }); }); -// ── getMapsKey ──────────────────────────────────────────────────────────────── +// ── resolveMapsKey / getMapsKey ─────────────────────────────────────────────── -describe('getMapsKey', () => { - it('MAPS-015: returns user key when user has one', () => { +describe('resolveMapsKey', () => { + const ORIGINAL_PLACES_KEY = process.env.PLACES_API_KEY; + afterEach(() => { + if (ORIGINAL_PLACES_KEY === undefined) delete process.env.PLACES_API_KEY; + else process.env.PLACES_API_KEY = ORIGINAL_PLACES_KEY; + }); + + it('MAPS-015: returns the caller own row key when nothing above it is set', () => { + mockDbGet.mockReturnValue({ maps_api_key: 'user-api-key' }); + expect(svc.resolveMapsKey(1)).toEqual({ key: 'user-api-key', source: 'user-row' }); + expect(svc.getMapsKey(1)).toBe('user-api-key'); // the wrapper reads the same chain + }); + + it('MAPS-016: the instance-wide key wins over the caller own row (#1939)', () => { + mockInstanceGet.mockReturnValueOnce({ value: 'instance-api-key' }); mockDbGet.mockReturnValueOnce({ maps_api_key: 'user-api-key' }); - expect(svc.getMapsKey(1)).toBe('user-api-key'); + expect(svc.resolveMapsKey(1)).toEqual({ key: 'instance-api-key', source: 'instance' }); }); - it('MAPS-016: falls back to admin key when user has none', () => { - mockDbGet.mockReturnValueOnce({ maps_api_key: null }); - mockDbGet.mockReturnValueOnce({ maps_api_key: 'admin-api-key' }); - expect(svc.getMapsKey(1)).toBe('admin-api-key'); - }); - - it('MAPS-017: returns null when neither user nor admin has a key', () => { - mockDbGet.mockReturnValue(undefined); + it('MAPS-017: returns null with no source when nothing is set anywhere', () => { + expect(svc.resolveMapsKey(1)).toEqual({ key: null, source: null }); expect(svc.getMapsKey(1)).toBeNull(); }); + + it('MAPS-017b: the operator env key wins and the database is never asked', () => { + process.env.PLACES_API_KEY = 'operator-key'; + expect(svc.resolveMapsKey(1)).toEqual({ key: 'operator-key', source: 'operator-env' }); + expect(mockInstanceGet).not.toHaveBeenCalled(); + expect(mockDbGet).not.toHaveBeenCalled(); + }); + + it("MAPS-017c: never reads another user's row — the admin fallback is gone (#1939)", () => { + svc.resolveMapsKey(1); + // Two statements, both scoped: the instance row and this caller's own row. + // The old chain ended in "WHERE role = 'admin' ... LIMIT 1", which handed a + // stranger's credential to every non-admin. + expect(preparedSql).toHaveLength(2); + expect(preparedSql.join(' ')).not.toContain("role = 'admin'"); + expect(preparedSql.some((sql) => sql.includes('WHERE id = ?'))).toBe(true); + }); }); // ── reverseGeocode ──────────────────────────────────────────────────────────── @@ -1153,6 +1197,25 @@ describe('searchPlaces (fetch stubbed)', () => { }); }); + it('MAPS-039h: a Google rejection logs which credential was used, never the credential (#1939)', async () => { + mockInstanceGet.mockReturnValueOnce({ value: 'instance-secret-key' }); + vi.stubGlobal( + 'fetch', + vi.fn().mockResolvedValue({ + ok: false, + status: 403, + json: async () => ({ error: { message: 'The caller does not have permission' } }), + }), + ); + const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + await expect(svc.searchPlaces(7, 'anything')).rejects.toMatchObject({ status: 403 }); + const logged = errorSpy.mock.calls.map((c) => String(c[0])).join('\n'); + expect(logged).toContain('keySource=instance'); + expect(logged).toContain('userId=7'); + expect(logged).not.toContain('instance-secret-key'); + errorSpy.mockRestore(); + }); + it('MAPS-039c: throws with generic message when Google error has no message', async () => { mockDbGet.mockReturnValueOnce({ maps_api_key: 'some-key' }); vi.stubGlobal( @@ -1319,6 +1382,20 @@ describe('autocompletePlaces (fetch stubbed)', () => { }); }); + it('MAPS-083b: the autocomplete rejection logs the key source too (#1939)', async () => { + mockDbGet.mockReturnValueOnce({ maps_api_key: 'own-row-secret' }); + vi.stubGlobal( + 'fetch', + vi.fn().mockResolvedValue({ ok: false, status: 403, json: async () => ({ error: { message: 'nope' } }) }), + ); + const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + await expect(svc.autocompletePlaces(4, 'anything')).rejects.toMatchObject({ status: 403 }); + const logged = errorSpy.mock.calls.map((c) => String(c[0])).join('\n'); + expect(logged).toContain('keySource=user-row'); + expect(logged).not.toContain('own-row-secret'); + errorSpy.mockRestore(); + }); + it('MAPS-084: throws generic message when Google error has no message', async () => { mockDbGet.mockReturnValueOnce({ maps_api_key: 'some-key' }); vi.stubGlobal( diff --git a/server/tests/unit/nest/settings/instance-api-keys.test.ts b/server/tests/unit/nest/settings/instance-api-keys.test.ts new file mode 100644 index 000000000..54905ff84 --- /dev/null +++ b/server/tests/unit/nest/settings/instance-api-keys.test.ts @@ -0,0 +1,139 @@ +/** + * instance-api-keys.test.ts — INSTKEY-001 through INSTKEY-008. + * + * The shared resolver behind #1939: the Places/Unsplash credential is instance + * configuration, so it comes from the env or an encrypted app_settings row, and + * only then from the caller's own users row. Run against a real in-memory DB + * with real apiKeyCrypto (a fixed ENCRYPTION_KEY), because the round trip + * through the random IV is half of what these functions promise. + */ + +const { testDb, dbMock } = vi.hoisted(() => { + const Database = require('better-sqlite3'); + const db = new Database(':memory:'); + db.exec('PRAGMA journal_mode = WAL'); + db.exec('PRAGMA foreign_keys = ON'); + return { testDb: db, dbMock: { db, closeDb: () => {}, reinitialize: () => {} } }; +}); + +vi.mock('../../../../src/db/database', () => dbMock); +vi.mock('../../../../src/config', () => ({ + JWT_SECRET: 'test-secret', + ENCRYPTION_KEY: 'a1b2c3d4e5f6a7b8c9d0e1f2a3b4c5d6a7b8c9d0e1f2a3b4c5d6a7b8c9d0e1f2', +})); + +import { describe, it, expect, beforeAll, beforeEach, afterAll, vi } from 'vitest'; +import { createTables } from '../../../../src/db/schema'; +import { runMigrations } from '../../../../src/db/migrations'; +import { createUser, createAdmin } from '../../../helpers/factories'; +import { resetTestDb } from '../../../helpers/test-db'; +import { DatabaseService } from '../../../../src/nest/database/database.service'; +import { + readInstanceApiKey, + writeInstanceApiKey, + resolveApiKey, +} from '../../../../src/nest/settings/instance-api-keys'; + +const db = new DatabaseService(testDb); + +beforeAll(() => { + createTables(testDb); + runMigrations(testDb); +}); + +beforeEach(() => { + resetTestDb(testDb); + delete process.env.PLACES_API_KEY; +}); + +afterAll(() => { + testDb.close(); +}); + +const storedValue = (key: string) => + (testDb.prepare('SELECT value FROM app_settings WHERE key = ?').get(key) as { value: string } | undefined)?.value; + +describe('instance API keys', () => { + it('INSTKEY-001: round-trips through encryption at rest', () => { + writeInstanceApiKey(db, 'maps_api_key', 'AIza-instance-key'); + expect(storedValue('maps_api_key')).toMatch(/^enc:v1:/); + expect(storedValue('maps_api_key')).not.toContain('AIza-instance-key'); + expect(readInstanceApiKey(db, 'maps_api_key')).toBe('AIza-instance-key'); + }); + + it('INSTKEY-002: a second write of the same value replaces the row (no second one)', () => { + writeInstanceApiKey(db, 'maps_api_key', 'same-key'); + const first = storedValue('maps_api_key'); + writeInstanceApiKey(db, 'maps_api_key', 'same-key'); + // Same plaintext, different blob — the IV is random. That is exactly why + // "did this change?" is never asked of the stored value. + expect(storedValue('maps_api_key')).not.toBe(first); + expect(readInstanceApiKey(db, 'maps_api_key')).toBe('same-key'); + const rows = testDb.prepare("SELECT COUNT(*) AS n FROM app_settings WHERE key = 'maps_api_key'").get() as { n: number }; + expect(rows.n).toBe(1); + }); + + it('INSTKEY-003: a blank value reads back as unset but keeps the row', () => { + writeInstanceApiKey(db, 'unsplash_api_key', ' '); + expect(storedValue('unsplash_api_key')).toBe(''); + expect(readInstanceApiKey(db, 'unsplash_api_key')).toBeNull(); + }); + + it('INSTKEY-004: a legacy plaintext row still reads', () => { + testDb.prepare("INSERT INTO app_settings (key, value) VALUES ('maps_api_key', 'plain-old-key')").run(); + expect(readInstanceApiKey(db, 'maps_api_key')).toBe('plain-old-key'); + }); + + it('INSTKEY-005: the operator env key wins and the database is never read', () => { + process.env.PLACES_API_KEY = 'operator-key'; + writeInstanceApiKey(db, 'maps_api_key', 'instance-key'); + expect(resolveApiKey(db, 'maps_api_key', 1, process.env.PLACES_API_KEY)).toEqual({ + key: 'operator-key', + source: 'operator-env', + }); + }); + + it('INSTKEY-006: the instance value wins over the caller own row', () => { + const { user } = createAdmin(testDb); + testDb.prepare('UPDATE users SET maps_api_key = ? WHERE id = ?').run('personal-key', user.id); + writeInstanceApiKey(db, 'maps_api_key', 'instance-key'); + expect(resolveApiKey(db, 'maps_api_key', user.id, undefined)).toEqual({ key: 'instance-key', source: 'instance' }); + }); + + it("INSTKEY-007: without an instance value the caller's own row answers — and nobody else's (#1939)", () => { + const { user: admin } = createAdmin(testDb); + testDb.prepare('UPDATE users SET maps_api_key = ? WHERE id = ?').run('admins-own-key', admin.id); + const { user: member } = createUser(testDb); + + // The admin gets theirs... + expect(resolveApiKey(db, 'maps_api_key', admin.id, undefined)).toEqual({ + key: 'admins-own-key', + source: 'user-row', + }); + // ...and the member gets nothing rather than the admin's, which is the whole + // point: they used to get it, and Google answered them with a 403. + expect(resolveApiKey(db, 'maps_api_key', member.id, undefined)).toEqual({ key: null, source: null }); + }); + + it('INSTKEY-009: userId 0 asks about the instance only', () => { + const { user } = createAdmin(testDb); + testDb.prepare('UPDATE users SET maps_api_key = ? WHERE id = ?').run('personal-key', user.id); + // app-config is optional-auth: with nobody asking there is no own row, and + // the answer must not be some other row that happens to be first. + expect(resolveApiKey(db, 'maps_api_key', 0, undefined)).toEqual({ key: null, source: null }); + writeInstanceApiKey(db, 'maps_api_key', 'instance-key'); + expect(resolveApiKey(db, 'maps_api_key', 0, undefined)).toEqual({ key: 'instance-key', source: 'instance' }); + }); + + it('INSTKEY-008: an empty instance value does not fall through to the own row', () => { + const { user } = createAdmin(testDb); + testDb.prepare('UPDATE users SET unsplash_api_key = ? WHERE id = ?').run('stale-personal', user.id); + writeInstanceApiKey(db, 'unsplash_api_key', 'to-be-cleared'); + writeInstanceApiKey(db, 'unsplash_api_key', ''); + // The admin who cleared the field cleared their column in the same save, so + // the fallback finding the old value would only happen on a row nobody + // touched — here it must not resurrect a cleared instance key for them. + testDb.prepare('UPDATE users SET unsplash_api_key = NULL WHERE id = ?').run(user.id); + expect(resolveApiKey(db, 'unsplash_api_key', user.id, undefined)).toEqual({ key: null, source: null }); + }); +}); diff --git a/server/tests/unit/nest/unsplash/unsplash.service.test.ts b/server/tests/unit/nest/unsplash/unsplash.service.test.ts index 9e88c0c86..74d6d4ca5 100644 --- a/server/tests/unit/nest/unsplash/unsplash.service.test.ts +++ b/server/tests/unit/nest/unsplash/unsplash.service.test.ts @@ -6,12 +6,28 @@ import path from 'path'; // safeFetch is mocked so saveUnsplashCover never hits the network. // db is mocked so getUnsplashKey resolves from a controllable stub, and // decrypt_api_key is a passthrough so stored values compare as plaintext. -const { safeFetch, mockDbGet } = vi.hoisted(() => ({ safeFetch: vi.fn(), mockDbGet: vi.fn(() => undefined as unknown) })); +// The instance-wide row (#1939) is read before the user's own, so it gets its +// own seam rather than eating the mockDbGet stub of every case below. +const { safeFetch, mockDbGet, mockInstanceGet } = vi.hoisted(() => ({ + safeFetch: vi.fn(), + mockDbGet: vi.fn((..._args: unknown[]) => undefined as unknown), + mockInstanceGet: vi.fn((..._args: unknown[]) => undefined as unknown), +})); vi.mock('../../../../src/utils/ssrfGuard', () => ({ safeFetch })); vi.mock('../../../../src/db/database', () => ({ - db: { prepare: () => ({ get: mockDbGet, all: vi.fn(() => []), run: vi.fn() }) }, + db: { + prepare: (sql: string) => ({ + get: (...args: unknown[]) => (sql.includes('app_settings') ? mockInstanceGet(...args) : mockDbGet(...args)), + all: vi.fn(() => []), + run: vi.fn(), + }), + }, +})); +vi.mock('../../../../src/nest/common/crypto/apiKeyCrypto', () => ({ + decrypt_api_key: (v: string | null) => v, + // Unused by the read path here, but instance-api-keys imports it. + maybe_encrypt_api_key: (v: string | null) => v, })); -vi.mock('../../../../src/nest/common/crypto/apiKeyCrypto', () => ({ decrypt_api_key: (v: string | null) => v })); import { UnsplashService } from '../../../../src/nest/unsplash/unsplash.service'; import { DatabaseService } from '../../../../src/nest/database/database.service'; @@ -31,6 +47,7 @@ afterEach(() => { vi.clearAllMocks(); vi.unstubAllGlobals(); mockDbGet.mockReturnValue(undefined); + mockInstanceGet.mockReturnValue(undefined); if (ORIGINAL_UNSPLASH_ENV === undefined) delete process.env.UNSPLASH_ACCESS_KEY; else process.env.UNSPLASH_ACCESS_KEY = ORIGINAL_UNSPLASH_ENV; }); @@ -65,6 +82,37 @@ describe('unsplashService.searchUnsplashPhotos', () => { expect(await searchUnsplashPhotos('paris')).toEqual({ error: 'Rate limited', status: 429 }); }); + it('UNSPLASH-016: a 200 carrying unparseable JSON becomes a 502, not a crash', async () => { + // The web endpoint answers 200 with an HTML challenge page when it decides the + // caller is a datacenter (#1449), so a parse failure on an ok response is an + // upstream problem, not the caller's. + const bad = fakeRes({ ok: true, type: 'text/html' }); + (bad as { json: () => Promise }).json = async () => { + throw new SyntaxError('Unexpected token < in JSON'); + }; + vi.stubGlobal('fetch', vi.fn().mockResolvedValue(bad)); + expect(await searchUnsplashPhotos('paris')).toEqual({ error: 'Unsplash search unavailable', status: 502 }); + }); + + it('UNSPLASH-017: a failing response with unparseable JSON keeps its own status', async () => { + const bad = fakeRes({ ok: false, status: 503, type: 'text/html' }); + (bad as { json: () => Promise }).json = async () => { + throw new SyntaxError('Unexpected token < in JSON'); + }; + vi.stubGlobal('fetch', vi.fn().mockResolvedValue(bad)); + expect(await searchUnsplashPhotos('paris')).toEqual({ error: 'Unsplash search unavailable', status: 503 }); + }); + + it('UNSPLASH-018: falls back to the generic message when a non-ok body names no error', async () => { + vi.stubGlobal('fetch', vi.fn().mockResolvedValue(fakeRes({ ok: false, status: 500, type: 'application/json', json: {} }))); + expect(await searchUnsplashPhotos('paris')).toEqual({ error: 'Unsplash search unavailable', status: 500 }); + }); + + it('UNSPLASH-019: uses the single error field when the errors array is absent', async () => { + vi.stubGlobal('fetch', vi.fn().mockResolvedValue(fakeRes({ ok: false, status: 401, type: 'application/json', json: { error: 'Bad credentials' } }))); + expect(await searchUnsplashPhotos('paris')).toEqual({ error: 'Bad credentials', status: 401 }); + }); + it('UNSPLASH-004: returns normalised photos on success and drops entries missing a url/thumb', async () => { vi.stubGlobal('fetch', vi.fn().mockResolvedValue(fakeRes({ ok: true, @@ -115,18 +163,28 @@ describe('unsplashService.getUnsplashKey', () => { expect(getUnsplashKey(1)).toBe('user-key'); }); - it('UNSPLASH-014: falls back to the admin key when the user has none', () => { + it('UNSPLASH-014: the instance-wide key wins over the user own key (#1939)', () => { delete process.env.UNSPLASH_ACCESS_KEY; - mockDbGet.mockReturnValueOnce({ unsplash_api_key: null }); - mockDbGet.mockReturnValueOnce({ unsplash_api_key: 'admin-key' }); - expect(getUnsplashKey(1)).toBe('admin-key'); + mockInstanceGet.mockReturnValue({ value: 'instance-key' }); + mockDbGet.mockReturnValue({ unsplash_api_key: 'user-key' }); + expect(getUnsplashKey(1)).toBe('instance-key'); + expect(mockDbGet).not.toHaveBeenCalled(); // the own row is not even read }); - it('UNSPLASH-015: returns null when neither env, user, nor admin has a key', () => { + it('UNSPLASH-015: returns null when neither env, instance, nor the user has a key', () => { delete process.env.UNSPLASH_ACCESS_KEY; mockDbGet.mockReturnValue(undefined); expect(getUnsplashKey(1)).toBeNull(); }); + + it("UNSPLASH-015b: never reads another user's key — the admin fallback is gone (#1939)", () => { + delete process.env.UNSPLASH_ACCESS_KEY; + mockDbGet.mockReturnValue(undefined); + expect(getUnsplashKey(1)).toBeNull(); + // Both reads are scoped: the instance row and this caller's own row. + expect(mockDbGet).toHaveBeenCalledTimes(1); + expect(mockDbGet).toHaveBeenCalledWith(1); + }); }); describe('unsplashService.saveUnsplashCover', () => { diff --git a/server/tests/unit/nest/user-profile.service.test.ts b/server/tests/unit/nest/user-profile.service.test.ts index 01d3d58fa..7025fca8d 100644 --- a/server/tests/unit/nest/user-profile.service.test.ts +++ b/server/tests/unit/nest/user-profile.service.test.ts @@ -40,6 +40,7 @@ import { resetTestDb } from '../../helpers/test-db'; import { createUser, createAdmin } from '../../helpers/factories'; import { UserProfileService } from '../../../src/nest/auth/user-profile.service'; import { DatabaseService } from '../../../src/nest/database/database.service'; +import { SEARCH_TEXT_FIELD_MASK } from '../../../src/nest/maps/maps.helpers'; const profile = new UserProfileService(new DatabaseService(testDb)); @@ -253,6 +254,55 @@ describe('validateKeys', () => { if (prevAppUrl === undefined) delete process.env.APP_URL; else process.env.APP_URL = prevAppUrl; }); + + it('AUTH-DB-111: probes the operator key on an install where the admin has no column of their own', async () => { + const prevPlacesKey = process.env.PLACES_API_KEY; + process.env.PLACES_API_KEY = 'operator-key'; + const fetchSpy = vi.spyOn(global, 'fetch').mockResolvedValueOnce({ + status: 200, + statusText: 'OK', + text: async () => '', + } as Response); + + try { + // The admin column stays empty on purpose: on an operator-run install the + // key belongs to whoever runs the box, and the panel used to answer + // "no maps key" while every search on that same install worked. + const { user } = createAdmin(testDb); + const result = await profile.validateKeys(user.id); + const headers = (fetchSpy.mock.calls[0]?.[1]?.headers ?? {}) as Record; + expect(headers['X-Goog-Api-Key']).toBe('operator-key'); + expect(result.maps).toBe(true); + } finally { + fetchSpy.mockRestore(); + if (prevPlacesKey === undefined) delete process.env.PLACES_API_KEY; + else process.env.PLACES_API_KEY = prevPlacesKey; + } + }); + + it('AUTH-DB-112: probes the instance key, not a stale value in the admin own column', async () => { + const { user } = createAdmin(testDb); + // What a second admin sees after the instance key was saved by the first + // one: their column still holds whatever they pasted long ago, while every + // search on the install runs on the app_settings row. + testDb.prepare('UPDATE users SET maps_api_key = ? WHERE id = ?').run('stale-personal-key', user.id); + testDb.prepare("INSERT INTO app_settings (key, value) VALUES ('maps_api_key', 'live-instance-key')").run(); + + const fetchSpy = vi.spyOn(global, 'fetch').mockResolvedValueOnce({ + status: 200, + statusText: 'OK', + text: async () => '', + } as Response); + + await profile.validateKeys(user.id); + const headers = (fetchSpy.mock.calls[0]?.[1]?.headers ?? {}) as Record; + expect(headers['X-Goog-Api-Key']).toBe('live-instance-key'); + // And it asks for the fields the real search asks for, so a key restricted + // to fewer Places SKUs fails the test button instead of passing it. + expect(headers['X-Goog-FieldMask']).toBe(SEARCH_TEXT_FIELD_MASK); + + fetchSpy.mockRestore(); + }); }); describe('updateMapsKey / avatar', () => { @@ -285,3 +335,110 @@ describe('profile quirk fixes', () => { expect(result.success).toBe(true); }); }); + +// --------------------------------------------------------------------------- +// Instance-wide keys + changedKeys (#1939) +// +// apiKeyCrypto is mocked to identity in this file, so nothing here asserts +// anything about encryption — the random-IV half of changedKeys is pinned in +// tests/integration/security.test.ts, which runs with a real ENCRYPTION_KEY. +// --------------------------------------------------------------------------- + +const instanceRow = (key: string) => + (testDb.prepare('SELECT value FROM app_settings WHERE key = ?').get(key) as { value: string } | undefined)?.value; + +describe('instance-wide API keys', () => { + it('AUTH-DB-102: an admin save lands in app_settings AND in their own column', () => { + const { user } = createAdmin(testDb); + profile.updateApiKeys(user.id, { maps_api_key: 'instance-google-key', unsplash_api_key: 'instance-unsplash-key' }); + expect(instanceRow('maps_api_key')).toBe('instance-google-key'); + expect(instanceRow('unsplash_api_key')).toBe('instance-unsplash-key'); + // The column stays in step so clearing the field clears both. + const row = testDb.prepare('SELECT maps_api_key FROM users WHERE id = ?').get(user.id) as { maps_api_key: string }; + expect(row.maps_api_key).toBe('instance-google-key'); + }); + + it('AUTH-DB-103: a non-admin save never touches the instance value', () => { + const { user: admin } = createAdmin(testDb); + profile.updateApiKeys(admin.id, { maps_api_key: 'admin-set' }); + const { user } = createUser(testDb); + profile.updateApiKeys(user.id, { maps_api_key: 'members-own' }); + expect(instanceRow('maps_api_key')).toBe('admin-set'); + const row = testDb.prepare('SELECT maps_api_key FROM users WHERE id = ?').get(user.id) as { maps_api_key: string }; + expect(row.maps_api_key).toBe('members-own'); + }); + + it('AUTH-DB-104: clearing the field stores an empty instance value rather than dropping the row', () => { + const { user } = createAdmin(testDb); + profile.updateApiKeys(user.id, { maps_api_key: 'to-be-removed' }); + profile.updateApiKeys(user.id, { maps_api_key: '' }); + // '' and not a missing row: a missing row would fall through to whatever + // still sat in the admin's own column. + expect(instanceRow('maps_api_key')).toBe(''); + }); + + it('AUTH-DB-105: getSettings reads the instance value, not the admin own column', () => { + const { user } = createAdmin(testDb); + testDb.prepare('UPDATE users SET maps_api_key = ? WHERE id = ?').run('stale-personal-key', user.id); + testDb.prepare("INSERT INTO app_settings (key, value) VALUES ('maps_api_key', 'live-instance-key')").run(); + expect(profile.getSettings(user.id).settings?.maps_api_key).toBe('live-instance-key'); + // openweather is per-user and unaffected. + expect(profile.getSettings(user.id).settings?.openweather_api_key).toBeNull(); + }); + + it('AUTH-DB-106: updateMapsKey mirrors for an admin and stays personal for a member', () => { + const { user: admin } = createAdmin(testDb); + expect(profile.updateMapsKey(admin.id, 'via-maps-key-route').changedKeys).toEqual(['maps_api_key']); + expect(instanceRow('maps_api_key')).toBe('via-maps-key-route'); + const { user } = createUser(testDb); + profile.updateMapsKey(user.id, 'members-own'); + expect(instanceRow('maps_api_key')).toBe('via-maps-key-route'); + }); + + it('AUTH-DB-107: updateSettings mirrors the key half without touching name/email handling', () => { + const { user } = createAdmin(testDb); + const result = profile.updateSettings(user.id, { maps_api_key: 'from-settings-route', username: 'renamed' }); + expect(result.success).toBe(true); + expect(result.user?.username).toBe('renamed'); + expect(instanceRow('maps_api_key')).toBe('from-settings-route'); + expect(result.changedKeys).toEqual(['maps_api_key']); + }); +}); + +describe('changedKeys', () => { + it('AUTH-DB-108: only the names in the body count, and clearing counts as a change', () => { + const { user } = createAdmin(testDb); + profile.updateApiKeys(user.id, { maps_api_key: 'k1', openweather_api_key: 'w1' }); + // unsplash was never sent, so it can never be reported. + expect(profile.updateApiKeys(user.id, { openweather_api_key: 'w2' }).changedKeys).toEqual(['openweather_api_key']); + expect(profile.updateApiKeys(user.id, { maps_api_key: '' }).changedKeys).toEqual(['maps_api_key']); + }); + + it('AUTH-DB-109: an unchanged value reports nothing, whitespace included', () => { + const { user } = createAdmin(testDb); + profile.updateApiKeys(user.id, { maps_api_key: 'unchanged-key' }); + expect(profile.updateApiKeys(user.id, { maps_api_key: 'unchanged-key' }).changedKeys).toEqual([]); + // maybe_encrypt_api_key trims on the way in, so the comparison has to too. + expect(profile.updateApiKeys(user.id, { maps_api_key: ' unchanged-key ' }).changedKeys).toEqual([]); + // A member is measured against their own column, not the instance value. + const { user: member } = createUser(testDb); + expect(profile.updateApiKeys(member.id, { maps_api_key: 'unchanged-key' }).changedKeys).toEqual(['maps_api_key']); + }); + + it('AUTH-DB-110: a managed install reports no change for the names it refuses to write', () => { + const prev = process.env.TREK_MANAGED; + process.env.TREK_MANAGED = 'true'; + try { + const { user } = createAdmin(testDb); + const result = profile.updateApiKeys(user.id, { maps_api_key: 'operator-owns-this' }); + expect(result.managed_keys).toEqual(['maps_api_key']); + expect(result.changedKeys).toEqual([]); + expect(instanceRow('maps_api_key')).toBeUndefined(); + expect(profile.updateMapsKey(user.id, 'operator-owns-this').changedKeys).toEqual([]); + expect(profile.updateSettings(user.id, { maps_api_key: 'operator-owns-this' }).changedKeys).toEqual([]); + } finally { + if (prev === undefined) delete process.env.TREK_MANAGED; + else process.env.TREK_MANAGED = prev; + } + }); +});