diff --git a/apps/api/src/contacts/timeline.ts b/apps/api/src/contacts/timeline.ts index 649e063..b5257df 100644 --- a/apps/api/src/contacts/timeline.ts +++ b/apps/api/src/contacts/timeline.ts @@ -18,6 +18,7 @@ import { import { todayIn } from '../generator/load.ts'; import { getSetting } from '../services/settings.ts'; import { ContrackError, contrackBook, contrackInteraction, contrackSettings } from './contrack.ts'; +import { ON_THE_CALL } from '../services/cdr.ts'; /** A recorded call waits this long for its summary before it's logged without one. */ const SUMMARY_WAIT_MS = 30 * 60_000; @@ -141,11 +142,13 @@ export async function logCallsToContrack(d: { .limit(500); // One entry per call and person (a person can be in a call as caller, rang and answered). - const todo = new Map(); + // `heard`: made or answered it; a phone that only rang gets no recording or summary (08 §7 #3). + const todo = new Map(); for (const r of rows) { const k = `${r.call.id}|${r.userId}`; const out = r.role === 'caller' || todo.get(k)?.out === true; - todo.set(k, { ...r, out }); + const heard = ON_THE_CALL.includes(r.role) || todo.get(k)?.heard === true; + todo.set(k, { ...r, out, heard }); } const skipUser = new Set(); // Contrack down or token refused: try again next run let logged = 0; @@ -169,7 +172,8 @@ export async function logCallsToContrack(d: { await record(null); continue; } - const wantSummary = r.withSummary && summariesOn && r.call.recordingId !== null; + const recorded = r.heard && r.call.recordingId !== null; + const wantSummary = r.withSummary && summariesOn && recorded; if (wantSummary && !r.aiDoneAt && now.getTime() - r.call.endedAt.getTime() < SUMMARY_WAIT_MS) continue; // the transcript job hasn't summarized it yet const e = timelineEntry( @@ -180,7 +184,7 @@ export async function logCallsToContrack(d: { toNumber: r.call.toNumber, talkSeconds: r.call.talkSeconds, answeredBy: r.answeredById && r.answeredById !== r.extId ? r.answeredBy : null, - recorded: r.call.recordingId !== null, + recorded, }, wantSummary ? r.summary : null, ); diff --git a/apps/api/src/routes/calls.ts b/apps/api/src/routes/calls.ts index 89b23ab..2c69e85 100644 --- a/apps/api/src/routes/calls.ts +++ b/apps/api/src/routes/calls.ts @@ -29,6 +29,7 @@ import { } from '../db/schema.ts'; import { contactNames } from '../contacts/book.ts'; import { badRequest, conflict, notFound } from '../errors.ts'; +import { ON_THE_CALL } from '../services/cdr.ts'; import { getSetting } from '../services/settings.ts'; const answerer = alias(extensions, 'answerer'); @@ -228,6 +229,8 @@ export async function callRoutes(app: FastifyInstance) { const out = mine.includes('caller'); return { ...c, + // Only rang (a ring group): someone else's conversation (08 §7 #3). + recordingId: mine.some((r) => ON_THE_CALL.includes(r)) ? c.recordingId : null, myRole: out ? 'out' : 'in', missedByMe: !out && (MISSED as readonly string[]).includes(c.status), }; diff --git a/apps/api/src/routes/contacts.ts b/apps/api/src/routes/contacts.ts index 32cead4..e68c746 100644 --- a/apps/api/src/routes/contacts.ts +++ b/apps/api/src/routes/contacts.ts @@ -270,8 +270,10 @@ export async function contactRoutes(app: FastifyInstance) { .object({ url: z.string().trim().min(1), companyToken: z.string().trim().optional() }) .parse(req.body); const saved = await getSetting(db, 'contacts.contrack'); + // The saved token only for the saved address: another one gets none (08 §7 #7). + const same = saved?.url === b.url.replace(/\/+$/, ''); const token = - b.companyToken || (saved?.companyTokenEnc ? decrypt(saved.companyTokenEnc) : null); + b.companyToken || (same && saved.companyTokenEnc ? decrypt(saved.companyTokenEnc) : null); try { return { ok: true, detail: await contrackCheck(b.url, token) }; } catch (err) { diff --git a/apps/api/src/routes/oidc.ts b/apps/api/src/routes/oidc.ts index 5c894c4..bfbfc3a 100644 --- a/apps/api/src/routes/oidc.ts +++ b/apps/api/src/routes/oidc.ts @@ -1,6 +1,6 @@ // SPDX-License-Identifier: AGPL-3.0-or-later // Sign in with OpenID Connect, connect and disconnect an OpenID account (12 §3.2, §3.3). -import { and, eq, gt, isNull, notExists } from 'drizzle-orm'; +import { and, eq, gt, isNull, ne, notExists } from 'drizzle-orm'; import type { FastifyInstance, FastifyReply, FastifyRequest } from 'fastify'; import { z } from 'zod'; import { decrypt, encrypt } from '../crypto/secrets.ts'; @@ -15,7 +15,7 @@ import { } from '../errors.ts'; import { requireAdmin, requireAuth } from '../auth/guards.ts'; import { isLoginBlocked, recordLoginFailure } from '../auth/login-limiter.ts'; -import { createSession, setSessionCookie } from '../auth/sessions.ts'; +import { COOKIE, createSession, getSessionUser, setSessionCookie } from '../auth/sessions.ts'; import { audit } from '../services/audit.ts'; import { getSetting } from '../services/settings.ts'; import { decide, verifiedEmail } from '../sso/claims.ts'; @@ -146,7 +146,13 @@ export async function oidcRoutes(app: FastifyInstance) { .where(and(eq(userIdentities.kind, 'oidc'), eq(userIdentities.subject, r.subject))); if (connecting) { - // Connect (12 §3.3): to the user of the session that asked, if it's still signed in. + // Connect (12 §3.3): to the user of the session that asked, if it's still signed in, and + // only in that browser: a planted sp_oidc can't connect someone else's account (08 §7 #8). + const here = req.cookies[COOKIE] + ? await getSessionUser(db, req.cookies[COOKIE], false) + : null; + if (here?.sessionId !== connecting) + return fail('expired', 'connect finished in another browser than the one that asked'); const [sess] = await db .select({ userId: sessions.userId, email: users.email }) .from(sessions) @@ -186,7 +192,8 @@ export async function oidcRoutes(app: FastifyInstance) { let userId = owner?.userId; if (!userId) { - // A user without a password or an OpenID account yet, by verified email (12 §3.3 step 2). + // A user without a password or any connected account yet, by verified email (12 §3.3 + // step 2). Never an admin: a provider account with their email isn't them (08 §7 #2). const v = verifiedEmail(r.claims, s.trustEmail); const [u] = v ? await db @@ -196,13 +203,12 @@ export async function oidcRoutes(app: FastifyInstance) { and( eq(users.email, v), isNull(users.passwordHash), + ne(users.role, 'admin'), notExists( db .select({ id: userIdentities.id }) .from(userIdentities) - .where( - and(eq(userIdentities.userId, users.id), eq(userIdentities.kind, 'oidc')), - ), + .where(eq(userIdentities.userId, users.id)), ), ), ) @@ -322,8 +328,15 @@ export async function oidcRoutes(app: FastifyInstance) { recordLoginFailure(req.ip, login); throw badRequest('The directory refused that username or password'); } - if ((await connectDirectory(db, req.log, req.user!.id, entry, req.ip)) === 'taken') + const r = await connectDirectory(db, req.log, req.user!.id, entry, req.ip); + if (r === 'taken') throw new HttpError(409, 'taken', 'That directory account belongs to another user'); + if (r === 'connected_other') + throw new HttpError( + 409, + 'connected_other', + 'Another directory account is connected. Ask an admin to disconnect it first', + ); return { ok: true }; }); app.delete('/api/users/:id/identities/:kind', { preHandler: requireAdmin }, (req, reply) => { diff --git a/apps/api/src/routes/recordings.ts b/apps/api/src/routes/recordings.ts index 88558e8..314a3bf 100644 --- a/apps/api/src/routes/recordings.ts +++ b/apps/api/src/routes/recordings.ts @@ -13,6 +13,7 @@ import { callExtensions, calls, extensions, recordings } from '../db/schema.ts'; import { badRequest, forbidden, notFound } from '../errors.ts'; import { audit } from '../services/audit.ts'; import { sideFiles } from '../ai/calls.ts'; +import { ON_THE_CALL } from '../services/cdr.ts'; /** The recording and, when call transcripts left them, its two side files. */ const removeFiles = async (dir: string, filePath: string) => { @@ -109,7 +110,10 @@ export async function recordingRoutes(app: FastifyInstance) { }, ); - /** Admins, or a participant: their extension is on the call this recording belongs to. */ + /** + * Admins, or who was on the call: their extension made or answered it. Not a phone that only + * rang (a ring group, a queue): that's a coworker's conversation (08 §7 #3). + */ async function mayHear(user: { id: string; role: string }, recordingId: string) { if (user.role === 'admin') return; const [mine] = await db @@ -117,7 +121,13 @@ export async function recordingRoutes(app: FastifyInstance) { .from(calls) .innerJoin(callExtensions, eq(callExtensions.callId, calls.id)) .innerJoin(extensions, eq(extensions.id, callExtensions.extensionId)) - .where(and(eq(calls.recordingId, recordingId), eq(extensions.userId, user.id))) + .where( + and( + eq(calls.recordingId, recordingId), + eq(extensions.userId, user.id), + inArray(callExtensions.role, ON_THE_CALL), + ), + ) .limit(1); if (!mine) throw forbidden('Not your call'); } diff --git a/apps/api/src/routes/settings.ts b/apps/api/src/routes/settings.ts index 9b743d3..5e8adca 100644 --- a/apps/api/src/routes/settings.ts +++ b/apps/api/src/routes/settings.ts @@ -1,6 +1,7 @@ // SPDX-License-Identifier: AGPL-3.0-or-later // /api/settings (04 §7.3) and the dashboard warnings. Admin only. -import { eq, inArray } from 'drizzle-orm'; +import { randomBytes } from 'node:crypto'; +import { eq, inArray, isNotNull } from 'drizzle-orm'; import type { FastifyInstance } from 'fastify'; import { z } from 'zod'; import { @@ -26,7 +27,7 @@ import { type Warning, } from '@selfphoneng/shared'; import type { Db } from '../db/client.ts'; -import { dids, extensions, trunks } from '../db/schema.ts'; +import { dids, extensions, trunks, users } from '../db/schema.ts'; import { HttpError, badRequest } from '../errors.ts'; import { SLOW, requireAdmin } from '../auth/guards.ts'; import { decrypt, encrypt } from '../crypto/secrets.ts'; @@ -63,6 +64,7 @@ const READ_ONLY = new Set([ 'spam.state', 'sms.webhookToken', 'setup.completed', + 'network.probeKeyEnc', ]); async function toDto(db: Db): Promise { @@ -72,10 +74,12 @@ async function toDto(db: Db): Promise { 'auth.oidc': oidc, 'auth.ldap': ldap, spam, + 'network.probeKeyEnc': probeKey, ...rest } = await getAllSettings(db); return { ...rest, + 'network.hasProbeKey': probeKey !== null, spam: { ...spam, ipqs: spam.ipqs && { hasKey: true }, @@ -257,6 +261,7 @@ export async function settingsRoutes(app: FastifyInstance) { app.patch('/api/settings', async (req) => { const body = z.record(z.string(), z.unknown()).parse(req.body); const values: Record = {}; + let contrackMoved = false; for (const [key, raw] of Object.entries(body)) { if (!(key in SettingsSchemas) || READ_ONLY.has(key)) throw badRequest(`${key} can't be changed here`); @@ -287,8 +292,13 @@ export async function settingsRoutes(app: FastifyInstance) { } catch (err) { throw badRequest(`Contrack address: ${(err as Error).message}`); } + // The saved tokens only go back to the address they were typed for (08 §7 #7). + const url = input?.url.replace(/\/+$/, ''); + const moved = !!old && !!input && old.url !== url; + if (moved && old.companyTokenEnc && !input.companyToken && !input.clearCompanyToken) + throw badRequest('The Contrack address changed: enter the company token again'); values[key] = input && { - url: input.url.replace(/\/+$/, ''), + url: url!, publicUrl: input.publicUrl, companyTokenEnc: input.companyToken ? encrypt(input.companyToken) @@ -296,6 +306,7 @@ export async function settingsRoutes(app: FastifyInstance) { ? '' : (old?.companyTokenEnc ?? ''), }; + if (moved) contrackMoved = true; forgetBook(null); } else if (key === 'auth.ldap') { values[key] = await ldapFromInput(raw); @@ -349,8 +360,17 @@ export async function settingsRoutes(app: FastifyInstance) { const cert = await readTlsCert(app.config.TLS_DIR); if (!cert.ok) throw badRequest(`Can't turn on TLS. ${cert.error}`); } + // The probe's key was made for its address: another probe doesn't get it (08 §7 #9). + if ('network.probeUrl' in values && values['network.probeUrl'] !== before['network.probeUrl']) + values['network.probeKeyEnc'] = null; for (const [key, value] of Object.entries(values)) await setSetting(db, key as SettingKey, value as never); + // People's own Contrack tokens were for the old address: they connect again (08 §7 #7). + if (contrackMoved) + await db + .update(users) + .set({ contrackTokenEnc: null }) + .where(isNotNull(users.contrackTokenEnc)); // A new IPQualityScore key may have credits: don't wait for spamQuotaCheck (14 §3). if ((body.spam as SpamInput | undefined)?.ipqs?.key) await resumeIpqs(db, 'a new key'); const changed = Object.keys(values).filter( @@ -474,6 +494,25 @@ export async function settingsRoutes(app: FastifyInstance) { }, ); + /** + * A new key for the network test probe (08 §7 #9): shown this once, for its PROBE_KEY. Made for + * the probe address set now; the old key stops working on the next test. + */ + app.post('/api/settings/network/probe-key', async (req) => { + if (!(await getSetting(db, 'network.probeUrl'))) + throw badRequest('Set the probe address first'); + const key = randomBytes(32).toString('base64url'); + await setSetting(db, 'network.probeKeyEnc', encrypt(key)); + await audit(db, { + actorUserId: req.user!.id, + actorIp: req.ip, + action: 'update', + entityType: 'settings', + summary: 'Made a new network test probe key', + }); + return { key }; + }); + /** The server's public IPv4, as an outside service sees it (only sent when asked). */ app.post('/api/settings/network/detect-public-ip', { config: SLOW }, async () => { const ip = await detectPublicIp(); diff --git a/apps/api/src/services/callreport.ts b/apps/api/src/services/callreport.ts index 5d8ef85..909c112 100644 --- a/apps/api/src/services/callreport.ts +++ b/apps/api/src/services/callreport.ts @@ -138,6 +138,9 @@ export async function sendCallReports(d: { aiDoneAt: recordings.aiDoneAt, out: sql`exists (select 1 from ${callExtensions} where ${callExtensions.callId} = ${calls.id} and ${callExtensions.extensionId} = ${e.id} and ${callExtensions.role} = 'caller')`, + // Made or answered it: only rang means a coworker's conversation (08 §7 #3). + heard: sql`exists (select 1 from ${callExtensions} where ${callExtensions.callId} = ${calls.id} + and ${callExtensions.extensionId} = ${e.id} and ${callExtensions.role} in ('caller', 'answered'))`, }) .from(calls) .leftJoin(answerer, eq(answerer.id, calls.answeredByExtensionId)) @@ -177,9 +180,9 @@ export async function sendCallReports(d: { toNumber: r.c.toNumber, talkSeconds: r.c.talkSeconds, answeredBy: r.answeredById && r.answeredById !== e.id ? r.answeredBy : null, - recorded: r.c.recordingId !== null, - summary: r.summary, - summaryPending: r.c.recordingId !== null && r.aiDoneAt === null, + recorded: r.heard && r.c.recordingId !== null, + summary: r.heard ? r.summary : null, + summaryPending: r.heard && r.c.recordingId !== null && r.aiDoneAt === null, })); const withSummaries = e.callReportSummaries && diff --git a/apps/api/src/services/cdr.ts b/apps/api/src/services/cdr.ts index 50c1726..ee44f7f 100644 --- a/apps/api/src/services/cdr.ts +++ b/apps/api/src/services/cdr.ts @@ -14,6 +14,12 @@ import { callExtensions, calls, cdr, dids, extensions, recordings } from '../db/ import { durationMs } from '../media/audio.ts'; type CdrRow = typeof cdr.$inferSelect; + +/** + * The call_extensions roles of someone who was on the call, not just rung: they may hear its + * recording and read its transcript and summary (08 §7 #3). + */ +export const ON_THE_CALL = ['caller', 'answered']; type Tx = Parameters[0]>[0]; /** cdr_manager.conf has usegmtime = yes: "2026-10-18 19:28:37" is UTC. */ diff --git a/apps/api/src/services/diagnostics.ts b/apps/api/src/services/diagnostics.ts index b63c98f..1480946 100644 --- a/apps/api/src/services/diagnostics.ts +++ b/apps/api/src/services/diagnostics.ts @@ -9,6 +9,7 @@ import { BlockList, isIPv4 } from 'node:net'; import type { DiagnosticCheck, NetworkTestDto } from '@selfphoneng/shared'; import type { AmiClient } from '../ami/client.ts'; import type { Config } from '../config.ts'; +import { decrypt } from '../crypto/secrets.ts'; import type { Db } from '../db/client.ts'; import { detectPublicIp } from '../jobs.ts'; import { getAllSettings } from './settings.ts'; @@ -221,15 +222,23 @@ export async function insideChecks(d: DiagDeps): Promise { ); out.push( - s['network.probeUrl'] - ? check('probe', 'Network test probe', 'ok', s['network.probeUrl']) - : check( + !s['network.probeUrl'] + ? check( 'probe', 'Network test probe', 'skip', 'None set, so nothing is tested from outside your network.', 'Run apps/probe on a machine outside this network (deploy/probe), then put its URL in Settings → Network.', - ), + ) + : s['network.probeKeyEnc'] + ? check('probe', 'Network test probe', 'ok', `${s['network.probeUrl']}, with its key`) + : check( + 'probe', + 'Network test probe', + 'warn', + `${s['network.probeUrl']} has no key: anyone who finds it can use it.`, + 'Settings → Network → Generate key, then set it as PROBE_KEY on the probe and restart it (strongly recommended).', + ), ); return out; } @@ -289,14 +298,20 @@ export async function networkTest( let ip: string | null = null; let results: ProbeResult[] = []; let probeError: string | null = null; + let keyRefused = false; + const key = s['network.probeKeyEnc']; try { const r = await (d.fetchProbe ?? fetch)(new URL('/v1/check', probe), { method: 'POST', - headers: { 'content-type': 'application/json' }, + headers: { + 'content-type': 'application/json', + ...(key && { authorization: `Bearer ${decrypt(key)}` }), + }, body: JSON.stringify({ host, tests }), signal: AbortSignal.timeout(30_000), }); const body = (await r.json()) as { ip?: string; results?: ProbeResult[]; error?: string }; + keyRefused = r.status === 401; if (!r.ok) probeError = body.error ?? `HTTP ${r.status}`; else ({ ip = null, results = [] } = body as { ip: string; results: ProbeResult[] }); // Datagrams sent last arrive just after the answer. @@ -316,7 +331,9 @@ export async function networkTest( 'Network test probe', 'fail', `${probe} didn't answer: ${probeError}`, - 'Check the probe runs and is reachable from this server, then try again.', + keyRefused + ? "The probe's PROBE_KEY isn't this server's key. Settings → Network → Generate key, set it as PROBE_KEY on the probe and restart it." + : 'Check the probe runs and is reachable from this server, then try again.', ), ], }; diff --git a/apps/api/src/services/voicemail.ts b/apps/api/src/services/voicemail.ts index eeb3327..21a0937 100644 --- a/apps/api/src/services/voicemail.ts +++ b/apps/api/src/services/voicemail.ts @@ -121,11 +121,12 @@ async function transcribeMessage(d: VoicemailDeps, row: Row): Promise d.log.error({ err }, 'webhook enqueue failed')); + // Every mailbox, with or without a user (the live message is only for the user's own). Not + // the transcript: webhooks go off this server, and transcripts never do (10 §1, 08 §7 #6). + const { transcript: _words, ...hook } = toVoicemailDto(row); + void enqueueWebhook(d.db, 'voicemail.new', { ...hook, extension: box.number }).catch( + (err: unknown) => d.log.error({ err }, 'webhook enqueue failed'), + ); if (box.userId) d.live.emit('message', { type: 'voicemail.new', diff --git a/apps/api/src/sso/directory.ts b/apps/api/src/sso/directory.ts index efbcbe5..3f0adeb 100644 --- a/apps/api/src/sso/directory.ts +++ b/apps/api/src/sso/directory.ts @@ -1,7 +1,7 @@ // SPDX-License-Identifier: AGPL-3.0-or-later // A directory entry → its SelfPhoneNG user (12 §4.3): connected before, or by the directory's // email. Never a new user (owner, 2026-10-25). -import { and, eq } from 'drizzle-orm'; +import { and, eq, notExists } from 'drizzle-orm'; import { deleteUserSessions } from '../auth/sessions.ts'; import type { Db } from '../db/client.ts'; import { userIdentities, users } from '../db/schema.ts'; @@ -22,7 +22,21 @@ export async function directoryUserFor(db: Db, e: DirectoryEntry): Promise u.role !== 'admin' && (u.passwordHash === null || u.mustChangePassword); -/** Connect a directory account the person proved (My settings, 12 §4.3). */ +/** + * Connect a directory account the person proved (My settings, 12 §4.3). Not another one in place + * of the one they have: the leaver check follows it, so swapping is an admin's job (08 §7 #5). + */ export async function connectDirectory( db: Db, log: { info(o: object, m: string): void }, userId: string, e: DirectoryEntry, ip: string, -): Promise<'connected' | 'taken'> { +): Promise<'connected' | 'taken' | 'connected_other'> { const [owner] = await db .select({ userId: userIdentities.userId }) .from(userIdentities) .where(and(eq(userIdentities.kind, 'ldap'), eq(userIdentities.subject, e.id))); if (owner && owner.userId !== userId) return 'taken'; + const [mine] = await db + .select({ subject: userIdentities.subject }) + .from(userIdentities) + .where(and(eq(userIdentities.userId, userId), eq(userIdentities.kind, 'ldap'))); + if (mine && mine.subject !== e.id) return 'connected_other'; await db.transaction(async (tx) => { await tx .delete(userIdentities) @@ -101,11 +123,8 @@ export async function directorySignIn( entityId: u.id, summary: `${u.email} connected directory account ${e.dn}`, }); - } else if (mine.subject !== e.id) { - // Their email matches, but another directory account is already theirs: not this one. - log.info({ userId: u.id }, `directory entry ${e.dn} matches by email but isn't theirs`); - return null; } else + // Found by this account's id (directoryUserFor never matches by email someone connected). await db .update(userIdentities) .set({ email: e.email, lastUsedAt: new Date() }) diff --git a/apps/api/test/call-reports.test.ts b/apps/api/test/call-reports.test.ts index 8c34aa3..32f9aae 100644 --- a/apps/api/test/call-reports.test.ts +++ b/apps/api/test/call-reports.test.ts @@ -181,4 +181,43 @@ describe.skipIf(!hasTestDb)('call report email', () => { await run('2026-10-23T13:06:00Z'); expect(t.mails[1]!.text).not.toContain('quote'); }); + + it('no recording or summary of a call that only rang my phone (08 §7 #3)', async () => { + await dan('PATCH', '/api/me', { callReportSummaries: true }); + const [rec] = await t.db + .insert(recordings) + .values({ + linkedid: 'r4', + filePath: '2026/10/24/r4.wav', + startedAt: new Date('2026-10-24T15:00:00Z'), + summary: 'Secret pricing for Acme', + aiDoneAt: new Date(), + }) + .returning(); + const [c] = await t.db + .insert(calls) + .values({ + linkedid: 'c4', // the ring group rang Dan; a coworker answered and talked + direction: 'inbound', + status: 'answered', + startedAt: new Date('2026-10-24T15:00:00Z'), + endedAt: new Date('2026-10-24T15:05:00Z'), + fromNumber: '13125557777', + talkSeconds: 300, + recordingId: rec!.id, + }) + .returning(); + await t.db.insert(callExtensions).values({ callId: c!.id, extensionId: extId, role: 'rang' }); + await t.db + .update(extensions) + .set({ callReportSentAt: new Date('2026-10-24T13:00:00Z') }) + .where(eq(extensions.id, extId)); + t.mails.length = 0; + expect(await run('2026-10-25T13:06:00Z')).toBe(1); + const text = t.mails[0]!.text; + expect(text).toContain('Incoming from (312) 555-7777'); + expect(text).not.toContain('Secret pricing'); + expect(text).not.toContain('recorded'); + expect(text).not.toContain('Summary not ready'); + }); }); diff --git a/apps/api/test/contrack.test.ts b/apps/api/test/contrack.test.ts index 4936911..6bb2f3c 100644 --- a/apps/api/test/contrack.test.ts +++ b/apps/api/test/contrack.test.ts @@ -460,6 +460,19 @@ describe.skipIf(!hasTestDb)('contacts in Contrack', () => { fake.state.readOnly.clear(); expect(await run()).toBe(0); expect(jane).toHaveLength(8); + + // Only rang Dan (a coworker took it): on his timeline, but not its recording or summary. + const [k12] = await t.db + .insert(calls) + .values(call('k12', 1.5, { recordingId: done!.id })) + .returning(); + await t.db + .insert(callExtensions) + .values({ callId: k12!.id, extensionId: ext!.id, role: 'rang' }); + forgetBook(danId); + expect(await run()).toBe(1); + expect(jane.at(-1)!.content).not.toMatch(/recorded|Acme/); + expect(jane.at(-1)).not.toHaveProperty('actionItem'); }); it('contacts from before Contrack are copied once, the builtin ones kept (C6)', async () => { @@ -521,4 +534,35 @@ describe.skipIf(!hasTestDb)('contacts in Contrack', () => { ]); expect((await dan('GET', '/api/me')).json().contrack).toBeNull(); }); + + it('the saved tokens only go to the address they were saved for (08 §7 #7)', async () => { + // Another machine on the network that answers like Contrack, and notes what it's sent. + const heard: string[] = []; + const other = createServer((req, res) => { + heard.push(String(req.headers.authorization ?? '')); + res.setHeader('content-type', 'application/json'); + res.end('[]'); + }); + await new Promise((ok) => other.listen(0, '127.0.0.1', ok)); + const otherUrl = `http://127.0.0.1:${(other.address() as AddressInfo).port}`; + try { + await admin('POST', '/api/contacts/contrack/test', { url: otherUrl }); + expect(heard.join(' ')).not.toContain('ctk_company'); + // A new address needs its token typed again… + const kept = await admin('PATCH', '/api/settings', { + 'contacts.contrack': { url: otherUrl }, + }); + expect(kept.statusCode).toBe(400); + expect(kept.json().error.message).toMatch(/token again/); + // …and people's own tokens are for the old one: they connect again. + const moved = await admin('PATCH', '/api/settings', { + 'contacts.contrack': { url: otherUrl, companyToken: 'ctk_new_place' }, + }); + expect(moved.statusCode, moved.body).toBe(200); + const [d] = await t.db.select().from(users).where(eq(users.id, danId)); + expect(d!.contrackTokenEnc).toBeNull(); + } finally { + other.close(); + } + }); }); diff --git a/apps/api/test/diagnostics.test.ts b/apps/api/test/diagnostics.test.ts index b98a84e..631d365 100644 --- a/apps/api/test/diagnostics.test.ts +++ b/apps/api/test/diagnostics.test.ts @@ -154,4 +154,55 @@ describe.skipIf(!hasTestDb)('diagnostics', () => { 401, ); }); + + it('the probe key: made here, shown once, sent to the probe, never in settings (08 §7 #9)', async () => { + const cookie = await t.login(ADMIN.email, ADMIN.password); + const as = (method: string, url: string, payload?: object) => + t.app.inject({ method: method as 'GET', url, payload, headers: { ...H, cookie } }); + await setSetting(t.db, 'network.probeUrl', 'https://probe.test'); + // No key yet: the inside checks say to make one. + expect(byId(await insideChecks(deps)).probe).toMatchObject({ status: 'warn' }); + + const g = await as('POST', '/api/settings/network/probe-key'); + expect(g.statusCode, g.body).toBe(200); + const { key } = g.json() as { key: string }; + expect(key).toMatch(/^[A-Za-z0-9_-]{43}$/); + const s = await as('GET', '/api/settings'); + expect(s.json()['network.hasProbeKey']).toBe(true); + expect(s.body).not.toContain(key); + expect(s.json()).not.toHaveProperty('network.probeKeyEnc'); + expect((await as('PATCH', '/api/settings', { 'network.probeKeyEnc': 'x' })).statusCode).toBe( + 400, + ); + expect(byId(await insideChecks(deps)).probe).toMatchObject({ status: 'ok' }); + + let auth: string | null = null; + await networkTest( + { + ...deps, + fetchProbe: (async (_u: URL, init: RequestInit) => { + auth = new Headers(init.headers).get('authorization'); + return new Response(JSON.stringify({ ip: '203.0.113.10', results: [] })); + }) as unknown as typeof fetch, + }, + 'quick', + ); + expect(auth).toBe(`Bearer ${key}`); + // The probe refusing it says what to do. + const refused = await networkTest( + { + ...deps, + fetchProbe: (async () => + new Response(JSON.stringify({ error: 'a key is needed' }), { + status: 401, + })) as unknown as typeof fetch, + }, + 'quick', + ); + expect(byId(refused.checks).probe!.fix).toContain('PROBE_KEY'); + + // Another address: the key was for the old probe, so it's gone. + await as('PATCH', '/api/settings', { 'network.probeUrl': 'https://other-probe.test' }); + expect((await as('GET', '/api/settings')).json()['network.hasProbeKey']).toBe(false); + }); }); diff --git a/apps/api/test/directory-signin.test.ts b/apps/api/test/directory-signin.test.ts index 273f73d..7c89647 100644 --- a/apps/api/test/directory-signin.test.ts +++ b/apps/api/test/directory-signin.test.ts @@ -1,12 +1,13 @@ // SPDX-License-Identifier: AGPL-3.0-or-later // 12 §4.3: who a directory entry may connect to by email, who may disconnect it (§4.5), and a -// directory that's down counting as a failed sign-in (§4.2). +// directory that's down counting as a failed sign-in (§4.2). 08 §7: no OpenID user by email, no +// swapping the connected account. import { eq } from 'drizzle-orm'; import { afterAll, beforeAll, describe, expect, it } from 'vitest'; import { resetLoginLimiter } from '../src/auth/login-limiter.ts'; import { userIdentities, users } from '../src/db/schema.ts'; import { setSetting } from '../src/services/settings.ts'; -import { directorySignIn } from '../src/sso/directory.ts'; +import { connectDirectory, directorySignIn } from '../src/sso/directory.ts'; import type { DirectoryEntry } from '../src/sso/ldap.ts'; import { hasTestDb } from './db.ts'; import { ADMIN, H, createTestApp } from './app.ts'; @@ -52,6 +53,38 @@ describe.skipIf(!hasTestDb)('directory accounts by email (12 §4.3)', () => { } }); + it('never connects by email someone who signs in with OpenID (08 §7 #1)', async () => { + // A directory that lets people edit their own mail: Mallory's entry, the victim's email. + const victim = await t.addUser('oidc-only@example.com', 'unused-pw-2'); + await t.db.update(users).set({ passwordHash: null }).where(eq(users.id, victim.id)); + await t.db + .insert(userIdentities) + .values({ userId: victim.id, kind: 'oidc', subject: 'https://idp victim-sub' }); + expect(await directorySignIn(t.db, log, entry(victim.email), '127.0.0.1')).toBeNull(); + expect(await connected(victim.id)).toBe(1); // the OpenID account only + }); + + it('a connected directory account is only swapped by an admin (08 §7 #5)', async () => { + const u = await t.addUser('swapper@example.com', 'swapper-password-1'); + expect(await connectDirectory(t.db, log, u.id, entry(u.email), '127.0.0.1')).toBe('connected'); + const [first] = await t.db.select().from(userIdentities).where(eq(userIdentities.userId, u.id)); + // The same account again is fine; another one (a shared mailbox) is not. + expect( + await connectDirectory( + t.db, + log, + u.id, + { ...entry(u.email), id: first!.subject }, + '127.0.0.1', + ), + ).toBe('connected'); + expect(await connectDirectory(t.db, log, u.id, entry('shared@example.com'), '127.0.0.1')).toBe( + 'connected_other', + ); + const now = await t.db.select().from(userIdentities).where(eq(userIdentities.userId, u.id)); + expect(now.map((i) => i.subject)).toEqual([first!.subject]); + }); + it('only an admin can disconnect a directory account (a leaver must stay checked)', async () => { const u = await t.addUser('leaver@example.com', 'leaver-password-1'); await t.db.insert(userIdentities).values({ userId: u.id, kind: 'ldap', subject: 'leaver-id' }); diff --git a/apps/api/test/oidc.test.ts b/apps/api/test/oidc.test.ts index 92fa741..40eff5e 100644 --- a/apps/api/test/oidc.test.ts +++ b/apps/api/test/oidc.test.ts @@ -207,6 +207,56 @@ for (const alg of ['RS256', 'ES256'] as const) expect(ids).toMatchObject([{ kind: 'oidc', subject: `${idp.issuer} u-cara` }]); }); + it('never matches by email an admin, or someone with a directory account (08 §7 #2)', async () => { + const [boss, dir] = await t.db + .insert(users) + .values([ + { email: 'diradmin@example.com', displayName: 'Boss', passwordHash: null, role: 'admin' }, + { email: 'diruser@example.com', displayName: 'Dir', passwordHash: null }, + ]) + .returning(); + await t.db + .insert(userIdentities) + .values({ userId: dir!.id, kind: 'ldap', subject: 'dir-user-id' }); + for (const u of [boss!, dir!]) { + idp.st.user = { sub: `u-${u.email}`, email: u.email, email_verified: true }; + const { callback } = await flow(); + expect(errorOf(callback!)).toBe('not_connected'); + expect(session(callback!)).toBeNull(); + } + }); + + it('connect finishes only in the browser whose session asked (08 §7 #8)', async () => { + // Mallory starts Connect, then plants her sp_oidc cookie in Vic's browser and sends him on. + await t.addUser('mallory@example.com', 'mallory-password-1'); + const [m] = await t.db.select().from(users).where(eq(users.email, 'mallory@example.com')); + await t.addUser('vic@example.com', 'vic-password-123'); + const mallory = await t.login('mallory@example.com', 'mallory-password-1'); + const vic = await t.login('vic@example.com', 'vic-password-123'); + idp.st.user = { sub: 'u-vic', email: 'vic@example.com', email_verified: true }; + for (const theirs of [vic, '']) { + const start = await t.app.inject({ + method: 'POST', + url: '/api/auth/oidc/connect', + headers: { ...H, cookie: mallory }, + payload: {}, + }); + const pending = start.cookies.find((c) => c.name === 'sp_oidc')!; + const atIdp = await fetch((start.json() as { url: string }).url, { redirect: 'manual' }); + const back = new URL(atIdp.headers.get('location')!); + const callback = await t.app.inject({ + url: back.pathname + back.search, + headers: { cookie: [`sp_oidc=${pending.value}`, theirs].filter(Boolean).join('; ') }, + }); + expect(errorOf(callback)).toBe('expired'); + const ids = await t.db + .select() + .from(userIdentities) + .where(eq(userIdentities.userId, m!.id)); + expect(ids).toEqual([]); + } + }); + it('trustEmail matches an unverified email', async () => { await t.db .insert(users) diff --git a/apps/api/test/recordings.test.ts b/apps/api/test/recordings.test.ts index f62ef48..50fd163 100644 --- a/apps/api/test/recordings.test.ts +++ b/apps/api/test/recordings.test.ts @@ -1,13 +1,13 @@ // SPDX-License-Identifier: AGPL-3.0-or-later // M7-T3: a recorded call (sp_recording) is linked to its calls row and playable by admins and the -// people on the call; delete and retention remove file and row. +// people on the call (not a phone that only rang, 08 §7 #3); delete and retention remove file and row. import { existsSync, mkdirSync, readFileSync, writeFileSync } from 'node:fs'; import { join } from 'node:path'; import { eq } from 'drizzle-orm'; import type { LightMyRequestResponse } from 'fastify'; import { afterAll, beforeAll, describe, expect, it } from 'vitest'; import type { AmiMessage } from '../src/ami/events.ts'; -import { calls, cdr, extensions, recordings } from '../src/db/schema.ts'; +import { callExtensions, calls, cdr, extensions, recordings, users } from '../src/db/schema.ts'; import { encrypt } from '../src/crypto/secrets.ts'; import { cleanupRecordings } from '../src/routes/recordings.ts'; import { aggregateCall, toCdrRow } from '../src/services/cdr.ts'; @@ -56,6 +56,8 @@ describe.skipIf(!hasTestDb)('call recordings', () => { }; }); afterAll(() => t?.close()); + const bobId = async () => + (await t.db.select().from(users).where(eq(users.email, 'bob@example.com')))[0]!.id; it('the recording is linked to its call, with size and length', async () => { const [c] = await t.db.select().from(calls); @@ -91,6 +93,26 @@ describe.skipIf(!hasTestDb)('call recordings', () => { expect((await as.alice('GET', '/api/recordings')).statusCode).toBe(403); }); + it('a phone that only rang (ring group, queue) gets neither audio nor transcript (08 §7 #3)', async () => { + const [bob] = await t.db + .insert(extensions) + .values({ + number: '102', + name: 'Bob', + userId: await bobId(), + sipPasswordEnc: encrypt('x'), + voicemailPinEnc: encrypt('4826'), + }) + .returning(); + const [c] = await t.db.select().from(calls); + await t.db.insert(callExtensions).values({ callId: c!.id, extensionId: bob!.id, role: 'rang' }); + expect((await as.bob('GET', `/api/recordings/${recId}/audio`)).statusCode).toBe(403); + expect((await as.bob('GET', `/api/recordings/${recId}/transcript`)).statusCode).toBe(403); + const mine = (await as.bob('GET', '/api/me/calls')).json(); + expect(mine.items).toEqual([expect.objectContaining({ id: c!.id, recordingId: null })]); + expect((await as.alice('GET', `/api/recordings/${recId}/transcript`)).statusCode).toBe(200); + }); + it('retention deletes recordings past their days; delete removes file and link', async () => { expect(await cleanupRecordings(t.db, dir(), 3650)).toBe(0); expect((await as.admin('DELETE', `/api/recordings/${recId}`)).statusCode).toBe(200); diff --git a/apps/api/test/voicemail.test.ts b/apps/api/test/voicemail.test.ts index 0e046a1..8c6b662 100644 --- a/apps/api/test/voicemail.test.ts +++ b/apps/api/test/voicemail.test.ts @@ -7,7 +7,7 @@ import { join } from 'node:path'; import { eq } from 'drizzle-orm'; import { afterAll, beforeAll, describe, expect, it } from 'vitest'; import type { LiveMessage } from '@selfphoneng/shared'; -import { extensions, voicemailMessages } from '../src/db/schema.ts'; +import { extensions, voicemailMessages, webhookDeliveries, webhooks } from '../src/db/schema.ts'; import { parseCallerId, type watchVoicemail } from '../src/services/voicemail.ts'; import { setSetting } from '../src/services/settings.ts'; import { hasTestDb } from './db.ts'; @@ -175,11 +175,21 @@ describe.skipIf(!hasTestDb)('voicemail watcher', () => { const url = `http://127.0.0.1:${(stt.address() as AddressInfo).port}/v1`; await setSetting(t.db, 'ai.stt', { url, model: 'whisper' }); await setSetting(t.db, 'ai.voicemailTranscripts', true); + await t.db + .insert(webhooks) + .values({ url: 'https://hooks.example.com/x', secretEnc: 'x', events: ['voicemail.new'] }); try { leave('INBOX', 3, '1792300700-00000004', '<104>'); await w.scan('101'); const r = (await rows()).find((x) => x.msgId === '1792300700-00000004')!; expect(r.transcript).toBe('Please call me.'); + // A webhook goes off this server: the words stay here (10 §1, 08 §7 #6). + const [hook] = await t.db + .select() + .from(webhookDeliveries) + .where(eq(webhookDeliveries.event, 'voicemail.new')); + expect((hook!.payload as { data: object }).data).toMatchObject({ id: r.id }); + expect((hook!.payload as { data: object }).data).not.toHaveProperty('transcript'); expect(t.mails.at(-1)!.text).toContain( 'What they said (automatic, may have mistakes):\nPlease call me.', ); diff --git a/apps/probe/src/index.ts b/apps/probe/src/index.ts index 47654d9..e3575ff 100644 --- a/apps/probe/src/index.ts +++ b/apps/probe/src/index.ts @@ -1,10 +1,21 @@ // SPDX-License-Identifier: AGPL-3.0-or-later // The network test probe (08 §2.6): run it outside the network SelfPhoneNG is in, and put its URL // in Settings → Network. `node src/index.ts` (Node 24 runs TypeScript as is). -// PORT (default 8080), TRUST_PROXY=true behind a reverse proxy that sets X-Forwarded-For. +// PORT (default 8080), TRUST_PROXY=true behind a reverse proxy on this machine that sets +// X-Forwarded-For. HOST: where to listen; with TRUST_PROXY=true 127.0.0.1 (only the proxy may +// reach it), else every address. PROBE_KEY: the key from SelfPhoneNG's Settings → Network; +// without one anyone who finds the probe can use it. import { createProbe } from './server.ts'; const port = Number(process.env.PORT ?? 8080); -const probe = createProbe({ trustProxy: process.env.TRUST_PROXY === 'true' }); -probe.listen(port, () => console.log(`selfphoneng-probe listening on ${port}`)); +const trustProxy = process.env.TRUST_PROXY === 'true'; +const host = process.env.HOST ?? (trustProxy ? '127.0.0.1' : '::'); +const key = process.env.PROBE_KEY || undefined; +const probe = createProbe({ trustProxy, key }); +probe.listen(port, host, () => console.log(`selfphoneng-probe listening on ${host} ${port}`)); +if (!key) + console.warn( + 'No PROBE_KEY: anyone who finds this probe can use it. Make one in SelfPhoneNG ' + + '(Settings → Network → Generate key) and set PROBE_KEY.', + ); for (const sig of ['SIGINT', 'SIGTERM']) process.on(sig, () => probe.close(() => process.exit(0))); diff --git a/apps/probe/src/probe.test.ts b/apps/probe/src/probe.test.ts index 6bd2252..4df8750 100644 --- a/apps/probe/src/probe.test.ts +++ b/apps/probe/src/probe.test.ts @@ -1,12 +1,18 @@ // SPDX-License-Identifier: AGPL-3.0-or-later // The probe against stand-ins on 127.0.0.1: TCP, HTTP and a WebSocket upgrade, SIP over UDP and -// TCP, raw UDP; and the server: it only tests the address that asked, validates, rate-limits. +// TCP, raw UDP; and the server: it only tests the address that asked, validates, rate-limits, +// believes X-Forwarded-For only from a proxy on this machine (08 §7 #4), and with PROBE_KEY only +// answers who sends it (#9). import { createSocket, type Socket as UdpSocket } from 'node:dgram'; -import { createServer as httpServer, type Server as HttpServer } from 'node:http'; +import { + createServer as httpServer, + type IncomingMessage, + type Server as HttpServer, +} from 'node:http'; import { createServer as tcpServer, type AddressInfo, type Server } from 'node:net'; import { afterAll, beforeAll, describe, expect, it } from 'vitest'; import { runTest } from './checks.ts'; -import { createProbe, parseCheck } from './server.ts'; +import { clientIp, createProbe, parseCheck } from './server.ts'; const port = (s: Server | HttpServer) => (s.address() as AddressInfo).port; const listen = (s: T) => @@ -165,3 +171,38 @@ describe('probe server', () => { target.close(); }); }); + +describe('the probe key', () => { + it('with a key, only a request that sends it gets tested (08 §7 #9)', async () => { + const key = 'k3y'.repeat(15); + const probe = await listen(createProbe({ key })); + const post = (authorization?: string) => + fetch(`http://127.0.0.1:${port(probe)}/v1/check`, { + method: 'POST', + headers: { 'content-type': 'application/json', ...(authorization && { authorization }) }, + body: JSON.stringify({ host: 'pbx.test', tests: [{ id: 'a', kind: 'tcp', port: 1 }] }), + }); + expect((await post()).status).toBe(401); + expect((await post('Bearer wrong')).status).toBe(401); + expect((await post(`Bearer ${key}x`)).status).toBe(401); + expect((await post(`Bearer ${key}`)).status).toBe(200); + expect((await fetch(`http://127.0.0.1:${port(probe)}/healthz`)).status).toBe(200); + probe.close(); + }); +}); + +describe('the address that asked', () => { + const req = (peer: string, xff?: string) => + ({ + headers: xff ? { 'x-forwarded-for': xff } : {}, + socket: { remoteAddress: peer }, + }) as unknown as IncomingMessage; + + it('takes X-Forwarded-For only from the local proxy (08 §7 #4)', () => { + // Reached straight from the internet: the header is whatever the caller wrote. + expect(clientIp(req('203.0.113.5', '198.51.100.9'), true)).toBe('203.0.113.5'); + for (const proxy of ['127.0.0.1', '::ffff:127.0.0.1', '::1']) + expect(clientIp(req(proxy, '10.0.0.1, 198.51.100.9'), true)).toBe('198.51.100.9'); + expect(clientIp(req('::ffff:203.0.113.5', '198.51.100.9'), false)).toBe('203.0.113.5'); + }); +}); diff --git a/apps/probe/src/server.ts b/apps/probe/src/server.ts index 39b7cf6..691f1ed 100644 --- a/apps/probe/src/server.ts +++ b/apps/probe/src/server.ts @@ -1,7 +1,9 @@ // SPDX-License-Identifier: AGPL-3.0-or-later // POST /v1/check {host, tests}: runs the tests against the address the request came from and // answers {ip, results}. GET /healthz. No other target can be given, which is what keeps the probe -// from being aimed at someone else; each address gets a few checks per 10 minutes. +// from being aimed at someone else; each address gets a few checks per 10 minutes. With a key +// (PROBE_KEY, made in SelfPhoneNG's Settings → Network), only who sends it is tested (08 §7 #9). +import { createHash, timingSafeEqual } from 'node:crypto'; import { createServer, type IncomingMessage, type Server } from 'node:http'; import { isIP } from 'node:net'; import { runTest, type Result, type Test } from './checks.ts'; @@ -9,6 +11,8 @@ import { runTest, type Result, type Test } from './checks.ts'; export type ProbeOptions = { /** Behind a reverse proxy: the address is the last X-Forwarded-For entry (the proxy adds it). */ trustProxy?: boolean; + /** Only requests with `Authorization: Bearer ` are tested. None: anyone may ask. */ + key?: string; /** Checks per address per window. */ maxChecks?: number; windowMs?: number; @@ -48,15 +52,27 @@ export function parseCheck(body: unknown): { host: string; tests: Test[] } | str return { host: b.host, tests }; } -const clientIp = (req: IncomingMessage, trustProxy: boolean) => { +/** + * The address to test. X-Forwarded-For only counts when the request came from a proxy on this + * machine: reached directly, the header is whatever the caller wrote (08 §7 #4). + */ +export const clientIp = (req: IncomingMessage, trustProxy: boolean) => { + const peer = req.socket.remoteAddress!.replace(/^::ffff:/, ''); const xff = req.headers['x-forwarded-for']; - const raw = - trustProxy && typeof xff === 'string' - ? xff.split(',').at(-1)!.trim() - : req.socket.remoteAddress!; - return raw.replace(/^::ffff:/, ''); + const local = peer === '127.0.0.1' || peer === '::1'; + return trustProxy && local && typeof xff === 'string' + ? xff + .split(',') + .at(-1)! + .trim() + .replace(/^::ffff:/, '') + : peer; }; +/** Equal, in the same time whatever was sent (hashes, so lengths don't show either). */ +const sameKey = (a: string, b: string) => + timingSafeEqual(createHash('sha256').update(a).digest(), createHash('sha256').update(b).digest()); + function readJson(req: IncomingMessage): Promise { return new Promise((resolve, reject) => { let size = 0; @@ -78,7 +94,7 @@ function readJson(req: IncomingMessage): Promise { } export function createProbe(opts: ProbeOptions = {}): Server { - const { trustProxy = false, maxChecks = 10, windowMs = 10 * 60_000 } = opts; + const { trustProxy = false, key, maxChecks = 10, windowMs = 10 * 60_000 } = opts; // ponytail: in-process memory; addresses past the window are swept on each check. const seen = new Map(); const allow = (ip: string, now = Date.now()) => { @@ -95,6 +111,8 @@ export function createProbe(opts: ProbeOptions = {}): Server { if (req.method === 'GET' && req.url === '/healthz') return send(res, 200, { ok: true }); if (req.method !== 'POST' || req.url !== '/v1/check') return send(res, 404, { error: 'not found' }); + if (key && !sameKey(req.headers.authorization ?? '', `Bearer ${key}`)) + return send(res, 401, { error: 'this probe needs its key' }); const ip = clientIp(req, trustProxy); void readJson(req) .then(async (body) => { diff --git a/apps/web/src/api/client.ts b/apps/web/src/api/client.ts index 6f00373..a854fa0 100644 --- a/apps/web/src/api/client.ts +++ b/apps/web/src/api/client.ts @@ -295,6 +295,9 @@ export const systemApi = { api<{ ok: true }>('POST', `/api/system/asterisk/restart?now=${now}`), detectPublicIp: () => api<{ ip: string }>('POST', '/api/settings/network/detect-public-ip').then((r) => r.ip), + /** A new key for the network test probe, shown this once (08 §7 #9). */ + probeKey: () => + api<{ key: string }>('POST', '/api/settings/network/probe-key').then((r) => r.key), tls: () => api('GET', '/api/system/tls'), diagnostics: () => api('GET', '/api/system/diagnostics'), networkTest: (mode: 'quick' | 'complete') => diff --git a/apps/web/src/pages/admin/SettingsPage.tsx b/apps/web/src/pages/admin/SettingsPage.tsx index f566f8a..1022c26 100644 --- a/apps/web/src/pages/admin/SettingsPage.tsx +++ b/apps/web/src/pages/admin/SettingsPage.tsx @@ -505,6 +505,11 @@ function NetworkTab({ s }: { s: SettingsDto }) { const [watch, setWatch] = useState(s['network.publicIpWatch']); const [probe, setProbe] = useState(s['network.probeUrl'] ?? ''); const detect = useMutation({ mutationFn: systemApi.detectPublicIp, onSuccess: setIp }); + const qc = useQueryClient(); + const makeKey = useMutation({ + mutationFn: systemApi.probeKey, + onSuccess: () => qc.invalidateQueries({ queryKey: ['settings'] }), + }); return (
)} + {s['network.probeUrl'] && ( +
+

+ Probe key +

+ {s['network.hasProbeKey'] ? ( +

Set: the probe only answers this server.

+ ) : ( +

+ Not set: anyone who finds your probe can use it. Strongly recommended: generate a key + and set it on the probe. +

+ )} + {makeKey.data ? ( + <> +

+ Set this on the probe (its compose file or service), then restart it. It’s + shown only now. +

+
+ + PROBE_KEY={makeKey.data} + + +
+ + ) : ( +
+ +
+ )} +

+ A new probe address drops the key: generate one for the new probe. +

+
+ )}
{save.isSuccess && Saved.}
- {(save.error ?? detect.error) && {(save.error ?? detect.error)!.message}} + {(save.error ?? detect.error ?? makeKey.error) && ( + {(save.error ?? detect.error ?? makeKey.error)!.message} + )}
); } diff --git a/deploy/probe/compose.yml b/deploy/probe/compose.yml index 52b797b..508b5a7 100644 --- a/deploy/probe/compose.yml +++ b/deploy/probe/compose.yml @@ -6,8 +6,9 @@ # docker compose -f deploy/probe/compose.yml up -d → http://:8080 # # Host networking, so it sees the real address that asks (Docker's published ports can hide it). -# Behind your own HTTPS reverse proxy instead: set PORT to a local port, TRUST_PROXY=true, and have -# the proxy set X-Forwarded-For. The SIP checks connect out from here, so no other ports are needed. +# Behind your own HTTPS reverse proxy on this machine instead: set PORT to a local port, +# TRUST_PROXY=true (it then listens on 127.0.0.1 only; HOST changes that), and have the proxy set +# X-Forwarded-For. The SIP checks connect out from here, so no other ports are needed. services: probe: # Built by each release, like the api and asterisk images (pick your registry and version). @@ -17,6 +18,9 @@ services: environment: PORT: '8080' TRUST_PROXY: 'false' + # Strongly recommended: SelfPhoneNG → Settings → Network → Generate key, paste it here, and + # the probe only answers your server. Empty: anyone who finds the probe can use it. + PROBE_KEY: '' logging: driver: json-file options: { max-size: 10m, max-file: '3' } diff --git a/deploy/probe/selfphoneng-probe.service b/deploy/probe/selfphoneng-probe.service index 8393f76..4468db8 100644 --- a/deploy/probe/selfphoneng-probe.service +++ b/deploy/probe/selfphoneng-probe.service @@ -12,7 +12,11 @@ Wants=network-online.target [Service] ExecStart=/usr/local/bin/selfphoneng-probe Environment=PORT=8080 -# Behind your own HTTPS reverse proxy that sets X-Forwarded-For: PORT to a local one, and +# Strongly recommended: SelfPhoneNG → Settings → Network → Generate key, then uncomment and paste +# it; the probe then only answers your server. +#Environment=PROBE_KEY= +# Behind your own HTTPS reverse proxy on this machine that sets X-Forwarded-For: PORT to a local +# one, and TRUST_PROXY=true (it then listens on 127.0.0.1 only; HOST changes that). #Environment=TRUST_PROXY=true Restart=always RestartSec=5 diff --git a/docs/admin-guide/ai.md b/docs/admin-guide/ai.md index ae1066a..62d66d0 100644 --- a/docs/admin-guide/ai.md +++ b/docs/admin-guide/ai.md @@ -72,8 +72,10 @@ on, each recorded call is also recorded one side at a time, and a minute or two - the two one-side files are deleted. The normal recording stays. On the **Recordings** page each call shows its summary, **Show transcript**, and the search box -finds calls by what was said. People who were on a call can read its transcript too, like they can -play its recording, and get its summary in their call report email (My settings → Call report). +finds calls by what was said. People who made or answered a call can read its transcript too, like +they can play its recording, and get its summary in their call report email (My settings → Call +report). Someone whose phone only rang (a ring group or queue a coworker answered) can't: it's +someone else's conversation. - Only calls that are recorded are written out (user record modes, phone numbers, queues and conference rooms set to record), and only those recorded while it's on. diff --git a/docs/admin-guide/network.md b/docs/admin-guide/network.md index 70dbd33..cf581b8 100644 --- a/docs/admin-guide/network.md +++ b/docs/admin-guide/network.md @@ -103,7 +103,8 @@ sudo ufw allow 8080/tcp (On GitHub, the downloads are at `https://github.com/iamdoubz/selfphoneng/releases/download/$v/…`.) To try it first without the service, run `./selfphoneng-probe-linux-x64` and open `http://:8080/healthz`. `PORT` changes the port; behind your own HTTPS reverse proxy -that sets X-Forwarded-For, also set `TRUST_PROXY=true`. +on the same machine that sets X-Forwarded-For, also set `TRUST_PROXY=true`. The probe then only +listens on 127.0.0.1 (`HOST` changes it) and only believes X-Forwarded-For from there. **With Docker:** @@ -117,6 +118,14 @@ against someone else, and each address gets a few tests per 10 minutes. Its SIP OPTIONS request; if you run fail2ban or CrowdSec, add the probe's address to their allow list so a test never counts toward a ban. +**Give the probe a key (strongly recommended).** Without one, anyone who finds your probe can use +it to test their own address, and spend its few tests per 10 minutes. After saving the address, +click **Generate key** under it, copy the `PROBE_KEY=…` line, and add it to the probe: under +`environment:` in `deploy/probe/compose.yml` (`PROBE_KEY: '…'`), or as an `Environment=PROBE_KEY=…` +line in the service file (`sudo systemctl edit selfphoneng-probe`). Restart the probe. From then on +it only answers this server, and System → Diagnostics stops warning about it. The key is shown only +once; a new probe address drops it, so generate one for the new probe. + ## Firewall Allow only the ports in the [port table](README.md#ports). All SelfPhoneNG containers use the diff --git a/docs/spec/04-backend-api.md b/docs/spec/04-backend-api.md index a256593..35036f7 100644 --- a/docs/spec/04-backend-api.md +++ b/docs/spec/04-backend-api.md @@ -286,7 +286,9 @@ As built (M7-T1): outside); otherwise `src`. An outbound call has no `answered_by` (no extension answered). - **recording** (M7-T3): `sp-sub-record` sets `CDR(SP_REC)`; the file (relative, under `RECORDINGS_DIR`) becomes a `recordings` row (size, ffprobe length) linked to the call. - `/api/recordings/:id/audio` serves it to admins and to users whose extension is on the call. + `/api/recordings/:id/audio` serves it to admins and to users whose extension made or answered + the call (`call_extensions` role `caller` or `answered`), not to a phone that only rang (08 §7 + #3); `/api/me/calls` shows such a call without its `recordingId`. Retention runs once a day (and at start) in the same day-change check as the nightly regenerate. - Aggregation runs 5 s after a linkedid's last Cdr event; on every AMI connect, linkedids older than a minute without a `calls` row are aggregated (instead of the nightly job). @@ -443,6 +445,7 @@ As built (M11-T6): `routes/reports.ts`. `from`/`to` are company-time-zone days, | GET | `/api/system/warnings` | `{items: [{code, message, link, dismissable?}]}` for the dashboard banner: `restart_required`, `e911_cid_missing` (a trunk exists, no default E911 caller ID), `e911_address_unconfirmed` (that DID has no `e911_address`), `smtp_missing`, `tls_cert` (TLS on and the cert missing or expiring within 14 days, M11-T2), `backup_old` (backups on and today's not made yet though the job had its chance, M11-T3), `directory_check` (12 §4.2, §4.5: the directory unreachable at a sign-in or by `directoryCheck`, or that job refusing to disable most people), `spam_quota` (14 §3, dismissable). "Trunk down" comes from live state in the browser | | POST | `/api/system/warnings/spam_quota/dismiss` | hides `spam_quota` until IPQualityScore's credits run out again (14 §3) | | POST | `/api/settings/network/detect-public-ip` | | +| POST | `/api/settings/network/probe-key` | a new key for the network test probe, `{ key }` shown this once, stored encrypted in `network.probeKeyEnc` (never in GET: `network.hasProbeKey`); 400 without a probe address. Diagnostics sends it as `Authorization: Bearer`; a new `network.probeUrl` drops it (08 §7 #9) | | GET | `/api/system/status` | `{ db: ok, ami: connected, asteriskVersion, uptime, trunks: [...], disk: {free, total}, restartRequired, lastGeneratorRun }` | | GET | `/api/system/tls` | the SIP TLS certificate in `TLS_DIR`: `{ok: true, subject, names, notAfter}` or `{ok: false, error}` (M11-T2) | | GET | `/api/system/generated-files` | names + contents of generated files, **passwords masked** (debug view) | @@ -533,7 +536,8 @@ Events: `call.started`, `call.answered`, `call.ended`, `voicemail.new`, `sms.rec `sms.sent`, `emergency.call`, `trunk.registration_changed`, `extension.registration_changed`. - Payload: `{ "id": "", "event": "call.ended", "createdAt": "...", "data": {...} }`. - `data` uses the same shapes as the REST API. + `data` uses the same shapes as the REST API, except `voicemail.new` has no `transcript`: a + webhook leaves the server and transcripts don't (10 §1, 08 §7 #6). - Headers: `Content-Type: application/json`, `X-SelfPhoneNG-Event`, `X-SelfPhoneNG-Timestamp` (unix seconds), `X-SelfPhoneNG-Signature: sha256=`. diff --git a/docs/spec/08-security-ops.md b/docs/spec/08-security-ops.md index ff21fd3..853f08e 100644 --- a/docs/spec/08-security-ops.md +++ b/docs/spec/08-security-ops.md @@ -117,8 +117,12 @@ how to fix it. works on Gitea's runner, where the job's checkout can't be bind-mounted). `deploy/probe/selfphoneng-probe.service` runs it under systemd with `DynamicUser` and a read-only system. About 45 MB of memory. + - With `PROBE_KEY` set the probe only answers `Authorization: Bearer ` (401 otherwise, + compared in constant time); without it, it answers anyone and logs a warning at start. The key + is made in Settings → Network and sent by the api only, never the browser (08 §7 #9). - `POST /v1/check {host, tests}`: the probe tests **only the address the request came from** (the - last X-Forwarded-For entry with `TRUST_PROXY=true`), so it can't be aimed at anyone else; 20 + last X-Forwarded-For entry with `TRUST_PROXY=true`, believed only from a proxy on 127.0.0.1 + or ::1, where the probe then listens by default; 08 §7 #4), so it can't be aimed at anyone else; 20 tests at most, 10 checks per address per 10 minutes, 4 s per test. - Quick: HTTP 80, HTTPS 443 `/healthz` (status and certificate), SIP OPTIONS over UDP (and TCP when on) to `sip.udpPort`. Complete adds SIP over TLS (when on), a WebSocket upgrade on `/ws` @@ -237,3 +241,36 @@ Fixed after the review (owner, 2026-10-21): exist; with credentials it counts as a failure (`devices.test.ts`). - The login limiter forgets keys past every window, swept at most once a minute (`login-limiter.test.ts`). + +## 7. Security review 2 (2026-10-28) + +Everything since §6 (`8d5475c..`): contacts and Contrack, single sign-on, call reports, local AI, +spam check, the mobile app's pairing, my reports and spam reports. Two passes again: one against +the code by the author, and an independent review agent (read-only, told to prove each finding +end to end). Each fix came with a test that failed before it and passes now. Nothing Critical or +High. + +| # | Severity | Finding | Fix | Proof | +|---|---|---|---|---| +| 1 | Medium | A directory sign-in matched by email a user who signs in with OpenID only. Where people can edit their own `mail`, Mallory set hers to the victim's and her own directory password opened the victim's account | the email match also needs no connected account of any kind (12 §4.3 step 2) | `directory-signin.test.ts` | +| 2 | Medium | An OpenID sign-in matched by verified email an admin, or a user who signs in through the directory (no password, no OpenID account yet): a provider account with that email got their session, admin included | not an admin, and no connected account of any kind (12 §3.3 step 2) | `oidc.test.ts`, both key types | +| 3 | Medium, privacy | Everyone whose phone **rang** counted as on the call: a ring group or queue member who didn't answer could play a coworker's recording, read its transcript, and got its summary in their call report email and on their Contrack timeline. The audio part predates §6 | `ON_THE_CALL` (`services/cdr.ts`): only the `caller` and `answered` roles, in `mayHear`, `/api/me/calls` (no `recordingId`), the call report and the timeline | `recordings.test.ts`, `call-reports.test.ts`, `contrack.test.ts` | +| 4 | Low | The probe with `TRUST_PROXY=true` believed any X-Forwarded-For and listened on every address: reached directly, anyone could have it test any address and ports | X-Forwarded-For only from 127.0.0.1 or ::1; `HOST` defaults to 127.0.0.1 with `TRUST_PROXY=true` (§2.6) | `probe.test.ts` | +| 5 | Low | A user could swap their directory account for another one they know the password of (a shared mailbox): the leaver check then followed the wrong account | `POST /api/me/identities/ldap` → 409 `connected_other` when a different one is connected; an admin disconnects first (12 §4.3) | `directory-signin.test.ts` | +| 6 | Low, privacy | The `voicemail.new` webhook carried the transcript, though webhooks leave the server and 10 §1 says transcripts don't | the webhook has no `transcript` (04 §9) | `voicemail.test.ts` | +| 7 | Low | Contrack's saved company token went to any address: Test with another URL, or saving a new URL, sent it there (SMTP, LDAP and OpenID already refused) | the saved token only for the saved address; a new address needs it typed again and clears everyone's own token (11 §4.3) | `contrack.test.ts` | +| 8 | Low | The OpenID connect callback wasn't tied to the browser that asked: a planted `sp_oidc` (a sibling subdomain can set it) connected the victim's provider account to the attacker's user | the callback's own session must be the one in `sp_oidc`, else `expired` (12 §3.3) | `oidc.test.ts`, both key types | +| 9 | Low | Anyone who found a probe could use it (owner's idea after #4): to test their own address, and spend its per-address limits | an optional key: Settings → Network → Generate key, set as the probe's `PROBE_KEY`; the api sends it as a Bearer token, encrypted at rest, dropped when the probe address changes. Without a key the probe stays open (owner, 2026-10-28) and Diagnostics warns, strongly recommending one (§2.6) | `probe.test.ts`, `diagnostics.test.ts` | + +The independent agent found 1, 2, 4, 5 and 8, the author 3; both found 6 and 7; 9 was the +owner's idea. The agent had +checked the recording rule and called it sound: it's the same rule as the audio, which §6 also +passed. + +Checked and sound (both passes): every new route's guard and ownership check (contacts and +sharing, spam reports, my reports, caller check, mobile devices); a paired phone is never an admin +and can't make pairing codes, and codes are single-use; OpenID PKCE, state, nonce, `safeNext` and +encrypted provider tokens; LDAP filter escaping and no empty binds; the caller-name lookup only +for Asterisk on this machine with its token; phonebook XML and timeline HTML escaped, report +emails plain text; AI and Contrack addresses literal local IPs with redirects refused; the spam +provider key kept out of errors; every new raw SQL parameterized; no secret in any GET. diff --git a/docs/spec/10-ai.md b/docs/spec/10-ai.md index 983f113..1fa47cb 100644 --- a/docs/spec/10-ai.md +++ b/docs/spec/10-ai.md @@ -47,7 +47,7 @@ is logged and the message goes out without it. or timing out: the run stops and the call is retried next minute. - API: `GET /api/recordings` has `summary`, `hasTranscript`, `aiError`, and `?q=` (ILIKE on the transcript and summary, `%`/`_` taken literally). `GET /api/recordings/:id/transcript`: admins - and the people on the call, like the audio. Deleting a recording, and retention, delete its side + and the people on the call (made or answered it, not only rang), like the audio. Deleting a recording, and retention, delete its side files too. ## 4. Next (in order) diff --git a/docs/spec/11-contacts.md b/docs/spec/11-contacts.md index e1cc7f7..2977486 100644 --- a/docs/spec/11-contacts.md +++ b/docs/spec/11-contacts.md @@ -190,6 +190,9 @@ against the pinned version's `openapi.json`. checked by reading contacts). - Admin, same page: the **company Contrack account's token** (read-only is enough; read-write lets "Add to company contacts" work), write-only like the SMTP password (`hasCompanyToken` in GET). + Like it, the saved token only goes to the address it was saved for: Test with another address + sends none, and saving a new address needs the token typed again (400). A new address also + clears everyone's own token: they connect again (08 §7 #7). The admin makes that account in Contrack (e.g. `company@…`) and fills it like any other. - Contrack tokens expire in 90 days unless made with **Never**: the guide and the forms say to pick Never (or a year). diff --git a/docs/spec/12-sso.md b/docs/spec/12-sso.md index 7634a83..0ea1b85 100644 --- a/docs/spec/12-sso.md +++ b/docs/spec/12-sso.md @@ -199,11 +199,12 @@ Failed callbacks count in the IP login limiter (04 §2.2). Signing in, in this order: 1. A `user_identities` row (`oidc`, `" "`) → its user. -2. Else, a user with that email (case-insensitive) who has **no password and no OpenID account - yet** (made by an admin in `oidc` mode, or whose password an admin removed), **and the email is - verified** (`email_verified === true`, or `trustEmail`) → connect it now (step 3's row) and sign - in. Users who have a password are never matched by email: they connect from My account, so a - provider where people can set their own email can't hand over an account. +2. Else, a user with that email (case-insensitive) who is **not an admin** and has **no password + and no connected account of any kind yet** (made by an admin in `oidc` mode, or whose password + an admin removed), **and the email is verified** (`email_verified === true`, or `trustEmail`) + → connect it now (step 3's row) and sign in. Users who have a password, an admin, or someone + who signs in through the directory are never matched by email: they connect from My account, + so a provider where people can set their own email can't hand over an account (08 §7 #2). 3. Else `not_connected`. Connecting (from My account, already signed in, any mode but `password`): @@ -211,6 +212,9 @@ Connecting (from My account, already signed in, any mode but `password`): - After the callback, the session's user gets the `user_identities` row (email and groups as sent). Refused (`taken`) when that `iss sub` belongs to another user; a user who already has one replaces it (confirm in the UI). Other sessions stay. Audit `sso_connected`. +- The callback must arrive in the browser whose session asked: its `sp_session` is the session in + `sp_oidc`, else `expired`. A planted `sp_oidc` (from a sibling subdomain) can't connect the + victim's provider account to someone else's user (08 §7 #8). - **Disconnect** (`DELETE /api/me/identities/oidc`; admins also on Users, `DELETE /api/users/:id/identities/oidc`): ends that user's `oidc` sessions. Refused (409 `last_way_in`) when it would leave them no way to sign in in the current mode (no password, or @@ -376,11 +380,13 @@ A user can have both a SelfPhoneNG password and a directory account: either pass 1. A `user_identities` row (`ldap`, id) → its user. Their email here follows the directory's when it changes (unless another user has it: kept, logged). -2. Else, a user with the directory email who is **not an admin** and has **no password or only the - temporary one** from Users (`must_change_password`) → connect it (add the row) and sign in, +2. Else, a user with the directory email who is **not an admin**, has **no password or only the + temporary one** from Users (`must_change_password`) and **no connected account of any kind** + (an OpenID user's account is theirs, 08 §7 #1) → connect it (add the row) and sign in, audit `ldap_connected`. Anyone else connects on My settings → Sign-in → **Connect directory account** (`POST /api/me/identities/ldap`, `{ login, password }`, checked by the directory and - counted by the login limiter; 409 `taken` when another user has it). Security audit, + counted by the login limiter; 409 `taken` when another user has it, 409 `connected_other` when + they already have a different one: an admin disconnects it first, 08 §7 #5). Security audit, 2026-10-26: a directory where people can edit their own `mail` (common OpenLDAP access rules) must not hand out the admin's account. 3. Else → 401 "Wrong email or password" (no account is made, §1), with `login_failed` audited as diff --git a/docs/v1-checklist.md b/docs/v1-checklist.md index af144c1..beba6d3 100644 --- a/docs/v1-checklist.md +++ b/docs/v1-checklist.md @@ -8,6 +8,8 @@ Tick a box only with proof: a date and what was seen. - [x] Milestones M0–M11 built and tested (simulated phones and a simulated voip.ms). - [x] Security review (M11-T8), with an independent review (08 §6). +- [x] Second security review of everything after it, with an independent review; all eight + findings fixed with tests (08 §7, 2026-10-28). ## Real voip.ms account (M3) diff --git a/packages/shared/src/settings.ts b/packages/shared/src/settings.ts index e0bb12d..0d6a2dd 100644 --- a/packages/shared/src/settings.ts +++ b/packages/shared/src/settings.ts @@ -42,6 +42,11 @@ export const SettingsSchemas = { .url({ protocol: /^https?$/ }) .nullable() .default(null), + /** + * The probe's key (its PROBE_KEY), encrypted: made by POST /api/settings/network/probe-key, + * sent as a Bearer token, dropped when the address changes (08 §7 #9). Never in GET. + */ + 'network.probeKeyEnc': z.string().nullable().default(null), 'network.stunServer': z .string() .regex(/^[A-Za-z0-9.-]+(:\d{1,5})?$/) @@ -327,10 +332,17 @@ export type SettingsDto = { [ K in Exclude< SettingKey, - 'smtp' | 'sms.webhookToken' | 'contacts.contrack' | 'auth.oidc' | 'auth.ldap' | 'spam' + | 'smtp' + | 'sms.webhookToken' + | 'contacts.contrack' + | 'auth.oidc' + | 'auth.ldap' + | 'spam' + | 'network.probeKeyEnc' > ]: SettingValue; } & { + 'network.hasProbeKey': boolean; smtp: (Omit & { hasPassword: boolean }) | null; 'contacts.contrack': { url: string; publicUrl: string | null; hasCompanyToken: boolean } | null; spam: SpamSettingsDto;