mirror of
https://github.com/mauriceboe/TREK.git
synced 2026-10-18 21:16:47 +00:00
fix(maps): resolve the Places key per instance, not per whoever is asking
Searching worked for the admin and 403'd for every other member. getMapsKey read the calling user's own row first and then fell through to "any admin" via LIMIT 1 with no ORDER BY, so the admin who pasted the key hit their own row while everyone else got a non-deterministic stranger's. That fallback is what CLAUDE.md forbids. The Places and Unsplash keys bill the install, not the account that pasted them in, so they become instance configuration: an encrypted app_settings row, resolved as env -> instance -> the caller's own row, the same shape #1772 chose for the LLM endpoint. The per-user column stays as the last step because /me/api-keys has always accepted a personal key. The trailing migration carries an existing install's key into the instance row so nothing degrades to OpenStreetMap on upgrade, but only where that key already was the whole install's: as soon as a second row holds one, it is left alone, because the instance row outranks everybody's own column and an upgrade must not move a member onto someone else's key and bill. Two things from the same report: The key test button probed with field mask places.displayName while the search asks for places.rating and more, which is a separate SKU and can be restricted on its own, so the button could pass while every search failed. It now probes the resolved key with the search's own mask. Saving API keys wrote no audit line. It now writes one settings.api_keys_update row naming which fields changed, never their values, comparing cleartext because the random IV makes every ciphertext look different. Closes #1939
This commit is contained in:
Maurice
@@ -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) {
|
||||
|
||||
@@ -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');
|
||||
}
|
||||
|
||||
|
||||
@@ -11,6 +11,7 @@ const ACTION_LABELS: Record<string, string> = {
|
||||
'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<string, unknown>): 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}` : '';
|
||||
}
|
||||
|
||||
@@ -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 };
|
||||
}
|
||||
|
||||
|
||||
@@ -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 = !!(
|
||||
|
||||
@@ -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<Pick<User, 'role' | 'maps_api_key' | 'openweather_api_key' | 'unsplash_api_key'>>(
|
||||
'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<User, 'maps_api_key' | 'openweather_api_key' | 'unsplash_api_key'> | 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<string, unknown>,
|
||||
current: Pick<User, 'maps_api_key' | 'openweather_api_key' | 'unsplash_api_key'> | 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<string, unknown>, 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<Pick<User, 'maps_api_key' | 'openweather_api_key' | 'unsplash_api_key'>>('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<Pick<User, 'id' | 'username' | 'email' | 'role' | 'maps_api_key' | 'openweather_api_key' | 'unsplash_api_key' | 'avatar' | 'mfa_enabled'>>(
|
||||
'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<string, unknown> } {
|
||||
): { error?: string; status?: number; success?: boolean; user?: Record<string, unknown>; 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<Pick<User, 'id' | 'username' | 'email' | 'role' | 'maps_api_key' | 'openweather_api_key' | 'unsplash_api_key' | 'avatar' | 'mfa_enabled'>>(
|
||||
@@ -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<Pick<User, 'role' | 'maps_api_key' | 'openweather_api_key'>>('SELECT role, maps_api_key, openweather_api_key FROM users WHERE id = ?', userId);
|
||||
const user = this.db.get<Pick<User, 'role' | 'openweather_api_key'>>('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' }),
|
||||
}
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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<Response> {
|
||||
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<string, unknown>[]; 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;
|
||||
|
||||
@@ -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<InstanceApiKeyName, string> = {
|
||||
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<Record<string, string | null>>(USER_ROW_SQL[name], userId);
|
||||
const own = decrypt_api_key(row?.[name]) || null;
|
||||
return own ? { key: own, source: 'user-row' } : { key: null, source: null };
|
||||
}
|
||||
@@ -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<string, string> = {
|
||||
* 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) {
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -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();
|
||||
});
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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();
|
||||
});
|
||||
});
|
||||
@@ -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' });
|
||||
|
||||
@@ -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' } });
|
||||
});
|
||||
|
||||
@@ -38,6 +38,8 @@ type SsrfCheckStub = Pick<SsrfResult, 'allowed' | 'error'>;
|
||||
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<SsrfCheckStub> => ({
|
||||
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(
|
||||
|
||||
@@ -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 });
|
||||
});
|
||||
});
|
||||
@@ -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<unknown> }).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<unknown> }).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', () => {
|
||||
|
||||
@@ -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<string, string>;
|
||||
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<string, string>;
|
||||
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;
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user