diff --git a/app/api/plugin-approval-status/route.ts b/app/api/plugin-approval-status/route.ts index d33f659c6..8d8374651 100644 --- a/app/api/plugin-approval-status/route.ts +++ b/app/api/plugin-approval-status/route.ts @@ -107,6 +107,7 @@ export async function POST(request: NextRequest) { permissions: manifest.permissions, httpOrigins: manifest.httpOrigins, apiPostPaths: manifest.apiPostPaths, + ...(manifest.credentials ? { credentials: manifest.credentials } : {}), }; let entry: ApprovalEntry; diff --git a/app/api/plugin-secrets/__tests__/route.test.ts b/app/api/plugin-secrets/__tests__/route.test.ts index c99c310c9..81ecbcc6b 100644 --- a/app/api/plugin-secrets/__tests__/route.test.ts +++ b/app/api/plugin-secrets/__tests__/route.test.ts @@ -26,6 +26,14 @@ vi.mock('@/lib/plugin-secrets/vault', () => ({ listUserCredentials: vi.fn(async () => []), deleteUserCredentials: vi.fn(async () => {}), })); +// What the server knows the plugins declare (lib/plugin-secrets/known-plugins). +const declared: Record = { + 'nextcloud/cloud': { scope: 'user', label: 'Nextcloud', inject: { type: 'basic' } }, + 'paperless/docs': { scope: 'user', label: 'Paperless', inject: { type: 'bearer' }, origin: 'https://paperless.example.com' }, +}; +vi.mock('@/lib/plugin-secrets/known-plugins', () => ({ + declaredUserCredential: async (pluginId: string, credentialId: string) => declared[`${pluginId}/${credentialId}`] ?? null, +})); const proxied: unknown[] = []; vi.mock('@/lib/plugin-secrets/proxy', async (orig) => { const real = await orig(); @@ -90,6 +98,20 @@ describe('POST /api/plugin-secrets', () => { expect(store.size).toBe(0); }); + it('refuses a credential no known plugin declares, so the proxy is no relay for made-up plugin ids', async () => { + expect((await save({ ...VALID, pluginId: 'made-up' })).status).toBe(404); + expect((await save({ ...VALID, credentialId: 'other' })).status).toBe(404); + expect(store.size).toBe(0); + }); + + it('keeps a credential on the server its plugin fixes and stores the inject the manifest declares', async () => { + const paperless = { ...VALID, pluginId: 'paperless', credentialId: 'docs', inject: { type: 'basic' } }; + expect((await save({ ...paperless, origin: 'https://elsewhere.example.com' })).status).toBe(400); + expect(store.size).toBe(0); + expect((await save({ ...paperless, origin: 'https://paperless.example.com/api/' })).status).toBe(200); + expect(store.get('paperless/docs')).toMatchObject({ origin: 'https://paperless.example.com', inject: { type: 'bearer' } }); + }); + it('refuses without a session, with plugins off, or from another site', async () => { signedIn = false; expect((await save(VALID)).status).toBe(401); diff --git a/app/api/plugin-secrets/route.ts b/app/api/plugin-secrets/route.ts index 0e67b59ff..e56909a4e 100644 --- a/app/api/plugin-secrets/route.ts +++ b/app/api/plugin-secrets/route.ts @@ -4,6 +4,7 @@ import { rejectCrossOriginRequest } from '@/lib/security/same-origin'; import { getStalwartCredentials } from '@/lib/stalwart/credentials'; import { CREDENTIAL_ID_RE, checkInject, credentialOrigin } from '@/lib/plugin-secrets/credentials'; import { adminCredentialOrigins, adminCredentialStatus } from '@/lib/plugin-secrets/proxy'; +import { declaredUserCredential } from '@/lib/plugin-secrets/known-plugins'; import { VaultFullError, deleteUserCredentials, @@ -76,9 +77,17 @@ export async function POST(request: NextRequest) { if (username !== undefined && (typeof username !== 'string' || username.length > 256 || /[\r\n]/.test(username))) return bad('Invalid username'); if (secret !== undefined && (typeof secret !== 'string' || !secret || secret.length > MAX_SECRET_LENGTH || /[\r\n]/.test(secret))) return bad('Invalid secret'); + // A credential belongs to a plugin this server knows declares it: without + // that, any signed-in user could park a secret under a made-up plugin id + // and use the proxy as a relay to any server. The manifest's own inject + // and, when fixed, its origin are what count, not what the browser sent. + const spec = await declaredUserCredential(pluginId, credentialId); + if (!spec) return NextResponse.json({ error: 'No such plugin credential' }, { status: 404 }); + if (spec.origin && credentialOrigin(spec.origin) !== origin) return bad('This credential is only for the server its plugin names'); + try { await putUserCredential(user, pluginId, credentialId, { - origin, inject, label: label.trim(), + origin, inject: spec.inject, label: label.trim(), ...(typeof username === 'string' && username ? { username } : {}), ...(typeof secret === 'string' ? { secret } : {}), }); diff --git a/lib/__tests__/plugin-known-credentials.test.ts b/lib/__tests__/plugin-known-credentials.test.ts new file mode 100644 index 000000000..f9559beca --- /dev/null +++ b/lib/__tests__/plugin-known-credentials.test.ts @@ -0,0 +1,80 @@ +/** + * @vitest-environment node + */ +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +// The proxy injected whatever credential the vault held for the (plugin, +// credential) pair in the query string, and the connect route stored one for +// any plugin id of the right shape. Neither asked whether a plugin the server +// knows declares that credential, so a signed-in user could relay requests +// through the server to any origin, and a plugin could reach for another's +// secret by id. Now a user credential exists only for a plugin the server +// can name: an installed one, or an admin-approved upload. + +const registry = new Map }>(); +const approvals: { pluginId: string; status: string; manifest: { credentials?: Record } }[] = []; +vi.mock('@/lib/admin/plugin-registry', () => ({ getPlugin: async (id: string) => registry.get(id) ?? null })); +vi.mock('@/lib/admin/plugin-approvals', () => ({ listApprovals: async () => approvals })); +vi.mock('@/lib/admin/plugin-config', () => ({ getPluginConfig: async () => ({}) })); +vi.mock('@/lib/logger', () => ({ logger: { info: () => {}, warn: () => {}, error: () => {}, debug: () => {} } })); + +const vault = new Map(); +vi.mock('@/lib/plugin-secrets/vault', () => ({ + getUserCredential: async (_u: unknown, p: string, c: string) => vault.get(`${p}/${c}`) ?? null, + setUserCredentialSecret: async () => {}, + userKey: () => 'user', +})); + +import { declaredUserCredential } from '@/lib/plugin-secrets/known-plugins'; +import { resolveCredential } from '@/lib/plugin-secrets/proxy'; + +const USER = { username: 'alice@example.org', serverUrl: 'https://mail.example.org' }; +const CLOUD = { scope: 'user', label: 'Nextcloud', inject: { type: 'basic' } }; + +beforeEach(() => { + registry.clear(); + approvals.length = 0; + vault.clear(); +}); + +describe('declaredUserCredential', () => { + it('finds the credential of an installed, enabled plugin', async () => { + registry.set('nextcloud', { enabled: true, credentials: { cloud: CLOUD } }); + expect(await declaredUserCredential('nextcloud', 'cloud')).toEqual(CLOUD); + expect(await declaredUserCredential('nextcloud', 'other')).toBeNull(); + registry.set('nextcloud', { enabled: false, credentials: { cloud: CLOUD } }); + expect(await declaredUserCredential('nextcloud', 'cloud')).toBeNull(); + }); + + it('finds the credential of an approved upload, not of a pending or denied one', async () => { + approvals.push({ pluginId: 'upload', status: 'pending', manifest: { credentials: { cloud: CLOUD } } }); + expect(await declaredUserCredential('upload', 'cloud')).toBeNull(); + approvals.push({ pluginId: 'upload', status: 'approved', manifest: { credentials: { cloud: CLOUD } } }); + expect(await declaredUserCredential('upload', 'cloud')).toEqual(CLOUD); + }); + + it('knows nothing about a made-up plugin or an admin-scope credential', async () => { + expect(await declaredUserCredential('x-y', 'c')).toBeNull(); + registry.set('ai', { enabled: true, credentials: { key: { scope: 'admin', label: 'Key', inject: { type: 'bearer' }, secretKey: 'apiKey' } } }); + expect(await declaredUserCredential('ai', 'key')).toBeNull(); + }); +}); + +describe('resolveCredential for a stored user credential', () => { + it('refuses one stored under a plugin the server does not know', async () => { + vault.set('x-y/c', { origin: 'https://any.example', inject: { type: 'none' } }); + expect(await resolveCredential(USER, 'x-y', 'c')).toBeNull(); + }); + + it('serves one a known plugin declares, on the origin the manifest fixes', async () => { + registry.set('nextcloud', { enabled: true, credentials: { cloud: CLOUD } }); + vault.set('nextcloud/cloud', { origin: 'https://cloud.example.org', inject: { type: 'basic' }, secret: 's' }); + expect(await resolveCredential(USER, 'nextcloud', 'cloud')).toMatchObject({ origin: 'https://cloud.example.org', scope: 'user' }); + + registry.set('paperless', { enabled: true, credentials: { docs: { ...CLOUD, origin: 'https://paperless.example.com' } } }); + vault.set('paperless/docs', { origin: 'https://elsewhere.example.com', inject: { type: 'basic' }, secret: 's' }); + expect(await resolveCredential(USER, 'paperless', 'docs')).toBeNull(); + vault.set('paperless/docs', { origin: 'https://paperless.example.com', inject: { type: 'basic' }, secret: 's' }); + expect(await resolveCredential(USER, 'paperless', 'docs')).toMatchObject({ origin: 'https://paperless.example.com' }); + }); +}); diff --git a/lib/__tests__/plugin-secret-proxy.test.ts b/lib/__tests__/plugin-secret-proxy.test.ts index 053a9eaf8..9eb8b1076 100644 --- a/lib/__tests__/plugin-secret-proxy.test.ts +++ b/lib/__tests__/plugin-secret-proxy.test.ts @@ -8,6 +8,15 @@ import path from 'node:path'; vi.mock('@/lib/admin/plugin-registry', () => ({ getPlugin: vi.fn(async () => null) })); vi.mock('@/lib/admin/plugin-config', () => ({ getPluginConfig: vi.fn(async () => ({})) })); +// A user credential exists only for a plugin the server knows declares it +// (lib/plugin-secrets/known-plugins.ts): the Nextcloud plugin of these tests +// is an admin-approved upload. +vi.mock('@/lib/admin/plugin-approvals', () => ({ + listApprovals: async () => [{ + pluginId: 'nextcloud', status: 'approved', + manifest: { credentials: { cloud: { scope: 'user', label: 'Nextcloud', inject: { type: 'basic' } } } }, + }], +})); const auditLines: Record[] = []; vi.mock('@/lib/logger', () => ({ logger: { diff --git a/lib/admin/plugin-approvals.ts b/lib/admin/plugin-approvals.ts index ef0ac516b..66f1a7705 100644 --- a/lib/admin/plugin-approvals.ts +++ b/lib/admin/plugin-approvals.ts @@ -15,6 +15,7 @@ import nodePath from 'node:path'; import { existsSync } from 'node:fs'; import { logger } from '@/lib/logger'; import { ensureConfigDir, getConfigPath, assertWritable } from './paths'; +import type { CredentialSpec } from '@/lib/plugin-secrets/credentials'; export type ApprovalStatus = 'pending' | 'approved' | 'denied'; @@ -31,6 +32,8 @@ export interface ApprovalEntry { permissions?: string[]; httpOrigins?: string[]; apiPostPaths?: string[]; + /** The credentials it may use through the plugin proxy (lib/plugin-secrets). */ + credentials?: Record; }; requestedBy: string; // JMAP username who triggered the request requestedAt: string; // ISO 8601 diff --git a/lib/plugin-secrets/known-plugins.ts b/lib/plugin-secrets/known-plugins.ts new file mode 100644 index 000000000..9385ff082 --- /dev/null +++ b/lib/plugin-secrets/known-plugins.ts @@ -0,0 +1,28 @@ +import { getPlugin } from '@/lib/admin/plugin-registry'; +import { listApprovals } from '@/lib/admin/plugin-approvals'; +import type { CredentialSpec, UserCredentialSpec } from './credentials'; + +/** + * The user-scope credential `credentialId` as the manifest of `pluginId` + * declares it, when this server knows that manifest: an installed (registry) + * plugin, or a user-uploaded package an admin approved. + * + * The connect dialog and the proxy used to take the plugin id at its word, + * so any signed-in user could store a credential under a made-up plugin id + * and relay requests through the server to any origin with it, and a + * plugin could reach for a credential declared by none. A credential is + * only ever for a plugin the server can name, and only on the server the + * manifest fixes, when it fixes one. + */ +export async function declaredUserCredential(pluginId: string, credentialId: string): Promise { + const plugin = await getPlugin(pluginId); + const fromRegistry = (plugin as { credentials?: Record } | null)?.credentials?.[credentialId]; + if (plugin && plugin.enabled && fromRegistry?.scope === 'user') return fromRegistry; + if (plugin) return null; + for (const entry of await listApprovals()) { + if (entry.pluginId !== pluginId || entry.status !== 'approved') continue; + const spec = entry.manifest.credentials?.[credentialId]; + if (spec?.scope === 'user') return spec; + } + return null; +} diff --git a/lib/plugin-secrets/proxy.ts b/lib/plugin-secrets/proxy.ts index 87cd7afc5..d703a35bf 100644 --- a/lib/plugin-secrets/proxy.ts +++ b/lib/plugin-secrets/proxy.ts @@ -11,6 +11,7 @@ import { logger } from '@/lib/logger'; 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'; /** Largest request body the proxy forwards (one upload chunk); see proxyClientMaxBodySize in next.config.ts. */ export const MAX_PROXY_REQUEST_BYTES = 100 * 1024 * 1024; @@ -69,6 +70,11 @@ export interface ResolvedCredential { export async function resolveCredential(user: VaultUser, pluginId: string, credentialId: string): Promise { const stored = await getUserCredential(user, pluginId, credentialId); if (stored) { + // Only for a plugin this server knows declares it, on the server the + // manifest fixes (see declaredUserCredential). + const spec = await declaredUserCredential(pluginId, credentialId); + if (!spec) return null; + if (spec.origin && credentialOrigin(spec.origin) !== stored.origin) return null; return { origin: stored.origin, inject: stored.inject, secret: stored.secret, username: stored.username, trusted: privateProxyOrigins().includes(stored.origin), scope: 'user',