fix: refuse to save over stored settings or plugin credentials that no longer decrypt #1066

app/api/plugin-secrets/route.ts

@@ -12,6 +12,7 @@ import {
   putUserCredential,
   type VaultUser,
 } from '@/lib/plugin-secrets/vault';
+import { storedDataUnavailableResponse } from '@/lib/stored-data-errors';
 
 const PLUGIN_ID_RE = /^[a-z0-9][a-z0-9-]*[a-z0-9]$/;
 const MAX_SECRET_LENGTH = 4096;
@@ -35,7 +36,14 @@ export async function GET(request: NextRequest) {
   if (pluginId !== undefined && !PLUGIN_ID_RE.test(pluginId)) {
     return NextResponse.json({ error: 'Invalid plugin id' }, { status: 400 });
   }
-  const credentials = await listUserCredentials(user, pluginId);
+  let credentials;
+  try {
+    credentials = await listUserCredentials(user, pluginId);
+  } catch (error) {
+    const unavailable = storedDataUnavailableResponse(error, NO_STORE);
+    if (unavailable) return unavailable;
+    throw error;
+  }
   // For one plugin, also say which of its admin credentials are set up, and
   // on which servers: the host page checks the links a plugin shows against
   // them (the plugin itself only gets yes/no, see proxyAccounts).
@@ -93,6 +101,8 @@ export async function POST(request: NextRequest) {
     });
   } catch (error) {
     if (error instanceof VaultFullError) return NextResponse.json({ error: error.message }, { status: 409 });
+    const unavailable = storedDataUnavailableResponse(error, NO_STORE);
+    if (unavailable) return unavailable;
     throw error;
   }
   return NextResponse.json({ ok: true }, { headers: NO_STORE });
@@ -108,7 +118,13 @@ export async function DELETE(request: NextRequest) {
   const credentialId = request.nextUrl.searchParams.get('credential') ?? undefined;
   if (!PLUGIN_ID_RE.test(pluginId)) return bad('Invalid plugin id');
   if (credentialId !== undefined && !CREDENTIAL_ID_RE.test(credentialId)) return bad('Invalid credential id');
-  await deleteUserCredentials(user, pluginId, credentialId);
+  try {
+    await deleteUserCredentials(user, pluginId, credentialId);
+  } catch (error) {
+    const unavailable = storedDataUnavailableResponse(error, NO_STORE);
+    if (unavailable) return unavailable;
+    throw error;
+  }
   return NextResponse.json({ ok: true }, { headers: NO_STORE });
 }
 

app/api/settings/route.ts

@@ -12,6 +12,7 @@ import { hasSessionSecret } from '@/lib/auth/session-secret';
 import { MAX_ACCOUNT_SLOTS } from '@/lib/account-utils';
 import { mergeStoredTemplates } from '@/lib/template-merge';
 import { mergeStoredSubscriptions } from '@/lib/calendar-subscription-sync';
+import { storedDataUnavailableResponse } from '@/lib/stored-data-errors';
 
 /** Largest settings blob a client may store (templates and signatures included). */
 const SETTINGS_MAX_BYTES = 2 * 1024 * 1024;
@@ -141,6 +142,8 @@ export async function GET(request: NextRequest) {
     const message = error instanceof Error ? error.message : 'Unknown error';
     const code = (error as NodeJS.ErrnoException).code;
     logger.error('Settings load error', { error: message, code });
+    const unavailable = storedDataUnavailableResponse(error);
+    if (unavailable) return unavailable;
     const classified = classifyError(error);
     return NextResponse.json({ error: classified.message }, { status: classified.status });
   }
@@ -221,6 +224,10 @@ export async function POST(request: NextRequest) {
     const message = error instanceof Error ? error.message : 'Unknown error';
     const code = (error as NodeJS.ErrnoException).code;
     logger.error('Settings save error', { error: message, code });
+    // Stored settings that don't decrypt, or no store: write nothing, so
+    // the user's real settings survive and the client keeps its copy.
+    const unavailable = storedDataUnavailableResponse(error);
+    if (unavailable) return unavailable;
     const classified = classifyError(error);
     return NextResponse.json({ error: classified.message }, { status: classified.status });
   }
@@ -252,6 +259,8 @@ export async function DELETE(request: NextRequest) {
     const message = error instanceof Error ? error.message : 'Unknown error';
     const code = (error as NodeJS.ErrnoException).code;
     logger.error('Settings delete error', { error: message, code });
+    const unavailable = storedDataUnavailableResponse(error);
+    if (unavailable) return unavailable;
     const classified = classifyError(error);
     return NextResponse.json({ error: classified.message }, { status: classified.status });
   }

lib/__tests__/stored-data-unreadable.test.ts

@@ -0,0 +1,162 @@
+// @vitest-environment node
+import { mkdtempSync, readdirSync, readFileSync, rmSync, writeFileSync } from 'node:fs';
+import { tmpdir } from 'node:os';
+import path from 'node:path';
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
+import { NextRequest } from 'next/server';
+
+// A settings file or plugin vault that exists but doesn't decrypt (written
+// with another session secret, or cut short) used to read as "nothing
+// stored". The next save then merged into nothing and overwrote the user's
+// real data. Now the read throws, and the routes answer 503 and write nothing.
+
+let secret = 'a'.repeat(64);
+vi.mock('@/lib/auth/session-secret', () => ({
+  getSessionSecret: () => secret,
+  hasSessionSecret: () => true,
+}));
+vi.mock('@/lib/logger', () => ({
+  logger: { warn: () => {}, error: () => {}, info: () => {}, debug: () => {} },
+}));
+vi.mock('@/lib/admin/config-manager', () => ({
+  configManager: {
+    ensureLoaded: async () => {},
+    get: (key: string, fallback: unknown) => (key === 'settingsSyncEnabled' ? true : fallback),
+    getPolicy: () => ({ restrictions: {}, features: { pluginsEnabled: true } }),
+  },
+}));
+vi.mock('@/lib/admin/third-party-mode', () => ({ isNoThirdPartyTrafficOn: async () => false, thirdPartyRefusal: () => null }));
+
+const jar = new Map<string, string>();
+vi.mock('next/headers', () => ({
+  cookies: async () => ({
+    get: (name: string) => (jar.has(name) ? { name, value: jar.get(name)! } : undefined),
+    set: (name: string, value: string) => { jar.set(name, value); },
+    delete: (name: string) => { jar.delete(name); },
+    getAll: () => [...jar].map(([name, value]) => ({ name, value })),
+  }),
+}));
+
+const SERVER = 'https://mail.example.org';
+const USER = 'alice@example.org';
+vi.mock('@/lib/stalwart/credentials', () => ({
+  getStalwartCredentials: async () => ({ username: USER, accountName: USER, serverUrl: SERVER, authHeader: 'x' }),
+}));
+vi.mock('@/lib/plugin-secrets/known-plugins', () => ({
+  declaredUserCredential: async () => ({ scope: 'user', label: 'Nextcloud', inject: { type: 'basic' } }),
+}));
+
+let dataDir: string;
+
+beforeEach(async () => {
+  dataDir = mkdtempSync(path.join(tmpdir(), 'bw-unreadable-'));
+  vi.stubEnv('SETTINGS_DATA_DIR', path.join(dataDir, 'settings'));
+  vi.stubEnv('PLUGIN_SECRETS_DIR', path.join(dataDir, 'plugin-secrets'));
+  vi.stubEnv('SETTINGS_SYNC_ENABLED', 'true');
+  secret = 'a'.repeat(64);
+  jar.clear();
+});
+
+afterEach(() => {
+  vi.unstubAllEnvs();
+  rmSync(dataDir, { recursive: true, force: true });
+});
+
+/** Sign in after the secret change: the context cookie is sealed with the current secret. */
+async function signIn() {
+  const { setStalwartAuthContextInStore } = await import('@/lib/stalwart/auth-context');
+  setStalwartAuthContextInStore({
+    get: (n: string) => (jar.has(n) ? { name: n, value: jar.get(n)! } : undefined),
+    set: (n: string, v: string) => { jar.set(n, v); },
+    delete: (n: string) => { jar.delete(n); },
+  } as never, 0, { serverUrl: SERVER, authHeader: 'Bearer t', username: USER });
+}
+
+function onlyFile(dir: string): string {
+  const [file] = readdirSync(dir).filter((name) => name.endsWith('.enc'));
+  return path.join(dir, file);
+}
+
+describe('settings that do not decrypt', () => {
+  it('reads a missing file as nothing stored and one written with another secret as an error', async () => {
+    const { loadUserSettings, saveUserSettings } = await import('@/lib/settings-sync');
+    const { StoredDataUnreadableError } = await import('@/lib/stored-data-errors');
+    expect(await loadUserSettings(USER, SERVER)).toBeNull();
+
+    await saveUserSettings(USER, SERVER, { theme: 'dark' });
+    secret = 'b'.repeat(64);
+    await expect(loadUserSettings(USER, SERVER)).rejects.toBeInstanceOf(StoredDataUnreadableError);
+
+    writeFileSync(onlyFile(path.join(dataDir, 'settings')), Buffer.alloc(10));
+    await expect(loadUserSettings(USER, SERVER)).rejects.toBeInstanceOf(StoredDataUnreadableError);
+  });
+
+  it('answers 503 to GET and POST and leaves the stored file as it was', async () => {
+    const { saveUserSettings } = await import('@/lib/settings-sync');
+    await saveUserSettings(USER, SERVER, { theme: 'dark', templates: [{ id: 'a' }] });
+    const file = onlyFile(path.join(dataDir, 'settings'));
+    const before = readFileSync(file);
+    secret = 'b'.repeat(64);
+    await signIn();
+
+    const { GET, POST } = await import('@/app/api/settings/route');
+    const got = await GET(new NextRequest('https://webmail.example/api/settings', {
+      headers: { 'x-settings-username': USER, 'x-settings-server': SERVER },
+    }));
+    expect(got.status).toBe(503);
+
+    const saved = await POST(new NextRequest('https://webmail.example/api/settings', {
+      method: 'POST',
+      headers: { 'sec-fetch-site': 'same-origin', 'content-type': 'application/json' },
+      body: JSON.stringify({ username: USER, serverUrl: SERVER, settings: { theme: 'light' } }),
+    }));
+    expect(saved.status).toBe(503);
+    expect(readFileSync(file).equals(before)).toBe(true);
+  });
+});
+
+describe('a plugin vault that does not decrypt', () => {
+  const user = { username: USER, serverUrl: SERVER };
+  const input = { origin: 'https://cloud.example.org', inject: { type: 'basic' } as const, label: 'Nextcloud', secret: 'pw' };
+
+  it('reads a missing vault as empty and one written with another secret as an error', async () => {
+    const { listUserCredentials, putUserCredential } = await import('@/lib/plugin-secrets/vault');
+    const { StoredDataUnreadableError } = await import('@/lib/stored-data-errors');
+    expect(await listUserCredentials(user)).toEqual([]);
+
+    await putUserCredential(user, 'nextcloud', 'cloud', input);
+    secret = 'b'.repeat(64);
+    await expect(listUserCredentials(user)).rejects.toBeInstanceOf(StoredDataUnreadableError);
+    // A write merges into what is stored, so it must not land either.
+    await expect(putUserCredential(user, 'other', 'x', input)).rejects.toBeInstanceOf(StoredDataUnreadableError);
+  });
+
+  it('answers 503 on the credential routes and the proxy and leaves the vault as it was', async () => {
+    const { putUserCredential } = await import('@/lib/plugin-secrets/vault');
+    await putUserCredential(user, 'nextcloud', 'cloud', input);
+    const file = onlyFile(path.join(dataDir, 'plugin-secrets'));
+    const before = readFileSync(file);
+    secret = 'b'.repeat(64);
+
+    const { GET, POST, DELETE } = await import('@/app/api/plugin-secrets/route');
+    const { POST: proxy } = await import('@/app/api/plugin-proxy/route');
+    const same = { 'sec-fetch-site': 'same-origin' };
+
+    expect((await GET(new NextRequest('http://localhost/api/plugin-secrets'))).status).toBe(503);
+    const post = await POST(new NextRequest('http://localhost/api/plugin-secrets', {
+      method: 'POST',
+      headers: same,
+      body: JSON.stringify({ pluginId: 'nextcloud', credentialId: 'other', label: 'x', origin: 'https://cloud.example.org', inject: { type: 'basic' }, secret: 's' }),
+    }));
+    expect(post.status).toBe(503);
+    const del = await DELETE(new NextRequest('http://localhost/api/plugin-secrets?plugin=nextcloud', { method: 'DELETE', headers: same }));
+    expect(del.status).toBe(503);
+    const proxied = await proxy(new NextRequest(
+      'http://localhost/api/plugin-proxy?plugin=nextcloud&credential=cloud&url=https%3A%2F%2Fcloud.example.org%2F',
+      { method: 'POST', headers: same },
+    ));
+    expect(proxied.status).toBe(503);
+
+    expect(readFileSync(file).equals(before)).toBe(true);
+  });
+});

lib/plugin-secrets/proxy.ts

@@ -12,6 +12,7 @@ import { DisallowedUrlError, fetchPublicUrl } from '@/lib/security/url-guard';
 import { authHeaderFor, credentialOrigin, privateProxyOrigins, type CredentialInject, type CredentialSpec } from './credentials';
 import { getUserCredential, setUserCredentialSecret, userKey, type VaultUser } from './vault';
 import { declaredUserCredential } from './known-plugins';
+import { isStoredDataUnavailable } from '@/lib/stored-data-errors';
 
 /** Largest request body the proxy forwards (one upload chunk). */
 export const MAX_PROXY_REQUEST_BYTES = 100 * 1024 * 1024;
@@ -269,7 +270,15 @@ export async function proxyRequest(input: ProxyInput): Promise<Response> {
   const method = input.method.toUpperCase();
   if (!PROXY_METHODS.has(method)) throw new ProxyError(`Method ${method} is not allowed`, 405, 'bad_method');
 
-  const credential = await resolveCredential(input.user, input.pluginId, input.credentialId);
+  let credential: ResolvedCredential | null;
+  try {
+    credential = await resolveCredential(input.user, input.pluginId, input.credentialId);
+  } catch (error) {
+    if (isStoredDataUnavailable(error)) {
+      throw Object.assign(new ProxyError('The connected accounts could not be read right now', 503, 'store_unavailable'), { retryAfter: 10 });
+    }
+    throw error;
+  }
   if (!credential) throw new ProxyError('No such credential', 404, 'no_credential');
 
   let target: URL;
@@ -381,6 +390,12 @@ export async function proxyRequest(input: ProxyInput): Promise<Response> {
       audit(error.status, 0, error.code);
       throw error;
     }
+    // A captured secret that couldn't be stored: the vault doesn't read,
+    // or the shared store is down. Nothing was written.
+    if (isStoredDataUnavailable(error)) {
+      audit(503, 0, 'store unavailable');
+      throw Object.assign(new ProxyError('The connected accounts could not be read or saved right now', 503, 'store_unavailable'), { retryAfter: 10 });
+    }
     if (error instanceof DisallowedUrlError) {
       audit(403, 0, 'blocked address');
       throw new ProxyError('This server address is not allowed', 403, 'blocked_address');

lib/plugin-secrets/vault.ts

@@ -7,6 +7,7 @@ import { createHash } from 'node:crypto';
 import { readFile, writeFile, mkdir, rename, unlink } from 'node:fs/promises';
 import path from 'node:path';
 import { decryptPayload, encryptPayload } from '@/lib/auth/crypto';
+import { StoredDataUnreadableError } from '@/lib/stored-data-errors';
 import type { CredentialInject } from './credentials';
 
 export interface VaultUser {
@@ -56,18 +57,24 @@ function vaultPath(user: VaultUser): string {
   return file;
 }
 
+/**
+ * The user's credentials; {} when none are stored. A file that exists but
+ * can't be read or doesn't decrypt (another session secret) throws
+ * StoredDataUnreadableError, so no write lands on top of it.
+ */
 async function load(user: VaultUser): Promise<VaultData> {
   let raw: string;
   try {
     raw = await readFile(vaultPath(user), 'utf8');
   } catch (error) {
     if ((error as NodeJS.ErrnoException).code === 'ENOENT') return {};
-    throw error;
+    throw new StoredDataUnreadableError('plugin credentials', { cause: error });
   }
   const payload = decryptPayload(raw, 'plugin-secrets');
+  if (!payload) throw new StoredDataUnreadableError('plugin credentials');
   // Bound to this user: a file copied over from another account's name
   // decrypts, but carries the other account's key and is ignored.
-  if (!payload || payload.u !== userKey(user)) return {};
+  if (payload.u !== userKey(user)) return {};
   const data = payload.d;
   return data && typeof data === 'object' ? (data as VaultData) : {};
 }

lib/settings-sync.ts

@@ -4,6 +4,7 @@ import { existsSync } from 'node:fs';
 import path from 'node:path';
 import { logger } from '@/lib/logger';
 import { getSessionSecret } from '@/lib/auth/session-secret';
+import { StoredDataUnreadableError } from '@/lib/stored-data-errors';
 
 const ALGORITHM = 'aes-256-gcm';
 const IV_LENGTH = 12;
@@ -52,14 +53,30 @@ export async function saveUserSettings(username: string, serverUrl: string, sett
   await rename(tmpPath, targetPath);
 }
 
+/**
+ * The user's stored settings, or null when none are stored. A file that
+ * exists but can't be read or decrypted (another session secret, cut short)
+ * throws StoredDataUnreadableError: treating it as "nothing stored" would
+ * let the next save overwrite the user's real settings.
+ */
 export async function loadUserSettings(username: string, serverUrl: string): Promise<Record<string, unknown> | null> {
   const filePath = getSettingsPath(username, serverUrl);
 
+  let data: Buffer;
   try {
-    const data = await readFile(filePath);
-    if (data.length < IV_LENGTH + TAG_LENGTH) return null;
+    data = await readFile(filePath);
+  } catch (error) {
+    if ((error as NodeJS.ErrnoException).code === 'ENOENT') return null;
+    logger.warn('Failed to read user settings', { error: error instanceof Error ? error.message : 'Unknown error' });
+    throw new StoredDataUnreadableError('settings', { cause: error });
+  }
+  return decryptSettings(data);
+}
 
-    const key = getKey();
+function decryptSettings(data: Buffer): Record<string, unknown> {
+  if (data.length < IV_LENGTH + TAG_LENGTH) throw new StoredDataUnreadableError('settings');
+  const key = getKey();
+  try {
     const iv = data.subarray(0, IV_LENGTH);
     const tag = data.subarray(IV_LENGTH, IV_LENGTH + TAG_LENGTH);
     const encrypted = data.subarray(IV_LENGTH + TAG_LENGTH);
@@ -70,9 +87,8 @@ export async function loadUserSettings(username: string, serverUrl: string): Pro
     const decrypted = Buffer.concat([decipher.update(encrypted), decipher.final()]);
     return JSON.parse(decrypted.toString('utf8'));
   } catch (error) {
-    if ((error as NodeJS.ErrnoException).code === 'ENOENT') return null;
-    logger.warn('Failed to load user settings', { error: error instanceof Error ? error.message : 'Unknown error' });
-    return null;
+    logger.warn('Failed to decrypt user settings', { error: error instanceof Error ? error.message : 'Unknown error' });
+    throw new StoredDataUnreadableError('settings', { cause: error });
   }
 }
 

lib/stored-data-errors.ts

@@ -0,0 +1,44 @@
+import { NextResponse } from 'next/server';
+import { StoreUnavailableError } from '@/lib/shared-store/types';
+
+/**
+ * A user's stored document exists but can't be read: it doesn't decrypt
+ * (another session secret), it is cut short, or the read failed. It is not
+ * the same as "nothing stored": a route that treated it as empty would merge
+ * into nothing and overwrite the user's real data on the next save. Routes
+ * answer 503 and write nothing.
+ */
+export class StoredDataUnreadableError extends Error {
+  constructor(what: string, options?: { cause?: unknown }) {
+    super(`Stored ${what} could not be read`, options);
+    this.name = 'StoredDataUnreadableError';
+  }
+}
+
+/** True for the errors that mean "try again later, nothing was written". */
+export function isStoredDataUnavailable(error: unknown): boolean {
+  return error instanceof StoredDataUnreadableError || error instanceof StoreUnavailableError;
+}
+
+/**
+ * The 503 for an unreadable document or an unreachable shared store, or null
+ * for any other error. The client keeps its own copy and retries.
+ */
+export function storedDataUnavailableResponse(
+  error: unknown,
+  headers?: Record<string, string>,
+): NextResponse | null {
+  if (error instanceof StoredDataUnreadableError) {
+    return NextResponse.json(
+      { error: 'Your stored data could not be read on the server. Nothing was changed.', code: 'stored_data_unreadable' },
+      { status: 503, headers: { 'Retry-After': '10', ...headers } },
+    );
+  }
+  if (error instanceof StoreUnavailableError) {
+    return NextResponse.json(
+      { error: 'Storage is temporarily unavailable. Nothing was changed.', code: 'store_unavailable' },
+      { status: 503, headers: { 'Retry-After': '10', ...headers } },
+    );
+  }
+  return null;
+}