diff --git a/src/servers/sidecar/notify/index.ts b/src/servers/sidecar/notify/index.ts index e16e0774..1086e064 100644 --- a/src/servers/sidecar/notify/index.ts +++ b/src/servers/sidecar/notify/index.ts @@ -2,6 +2,7 @@ import type { SidecarCommand, SidecarEvent } from '../protocol'; import { createSidecarConnector } from '../connect'; import { dispatch, configuredChannels } from './dispatch'; import { handleDeviceRoute } from './devices'; +import { resolveNotifyUser } from './resolve-user'; import { closeApnsSessions } from './apns'; import type { Notification, NotifyType } from './types'; @@ -61,17 +62,10 @@ const server = Bun.serve({ if (!body.type || !VALID_TYPES.includes(body.type)) { return Response.json({ error: `type must be one of ${VALID_TYPES.join(', ')}` }, { status: 400 }); } - // Producers inside the tailnet POST directly over loopback and say who to notify — the queue and - // the email sidecar have no session to speak from, so the body is their only way to name a user. - // - // The header WINS where it is present, and that ordering is the whole access control here. A - // request carrying X-Officer-User arrived through createSidecarProxy, meaning a signed-in browser - // sent it; letting its body override the id the platform authenticated would let any account with - // the `notify` capability push to any other account's devices. A direct producer sets no header, - // so its body is still honoured. - const headerUser = Number(req.headers.get('X-Officer-User')); - const userId = Number.isFinite(headerUser) && headerUser > 0 ? headerUser : body.userId; - if (typeof userId !== 'number' || !Number.isFinite(userId) || userId <= 0) { + // The header wins over the body wherever it is present — see ./resolve-user.ts for why that + // ordering is the whole access control on this route. + const userId = resolveNotifyUser({ header: req.headers.get('X-Officer-User'), bodyUserId: body.userId }); + if (userId === null) { return Response.json({ error: 'userId is required (body or X-Officer-User)' }, { status: 400 }); } @@ -93,7 +87,9 @@ const server = Bun.serve({ }, }); -console.log(`[notify] listening on http://127.0.0.1:${server.port} — channels: ${configuredChannels().join(', ') || 'none configured'}`); +console.log( + `[notify] listening on http://127.0.0.1:${server.port} — channels: ${configuredChannels().join(', ') || 'none configured'}`, +); // ── Connect to the API server ── diff --git a/src/servers/sidecar/notify/resolve-user.test.ts b/src/servers/sidecar/notify/resolve-user.test.ts new file mode 100644 index 00000000..a11f74ca --- /dev/null +++ b/src/servers/sidecar/notify/resolve-user.test.ts @@ -0,0 +1,40 @@ +import { describe, expect, test } from 'bun:test'; +import { resolveNotifyUser } from './resolve-user'; + +describe('resolveNotifyUser', () => { + test('a loopback producer with no header is trusted with the body', () => { + expect(resolveNotifyUser({ header: null, bodyUserId: 13 })).toBe(13); + }); + + test('a proxied request uses the header the platform injected', () => { + expect(resolveNotifyUser({ header: '1', bodyUserId: undefined })).toBe(1); + }); + + // The regression this module exists for: `body.userId ?? header` let any account holding the + // `notify` capability push to any other account's devices by naming them in a JSON body. + test('the header beats a conflicting body userId', () => { + expect(resolveNotifyUser({ header: '13', bodyUserId: 1 })).toBe(13); + expect(resolveNotifyUser({ header: '1', bodyUserId: 13 })).toBe(1); + }); + + test('neither claimant supplying an id is a 400, not a default', () => { + expect(resolveNotifyUser({ header: null, bodyUserId: undefined })).toBeNull(); + expect(resolveNotifyUser({ header: '', bodyUserId: null })).toBeNull(); + }); + + test('a malformed header does not silently fall through to the body', () => { + // Otherwise a browser could send a junk header alongside a chosen body id and win. + expect(resolveNotifyUser({ header: 'abc', bodyUserId: 13 })).toBeNull(); + expect(resolveNotifyUser({ header: '0', bodyUserId: 13 })).toBeNull(); + expect(resolveNotifyUser({ header: '-1', bodyUserId: 13 })).toBeNull(); + expect(resolveNotifyUser({ header: '1.5', bodyUserId: 13 })).toBeNull(); + }); + + test('a body userId that is not a positive integer is refused', () => { + expect(resolveNotifyUser({ header: null, bodyUserId: '13' })).toBeNull(); + expect(resolveNotifyUser({ header: null, bodyUserId: 0 })).toBeNull(); + expect(resolveNotifyUser({ header: null, bodyUserId: -3 })).toBeNull(); + expect(resolveNotifyUser({ header: null, bodyUserId: 2.5 })).toBeNull(); + expect(resolveNotifyUser({ header: null, bodyUserId: { id: 1 } })).toBeNull(); + }); +}); diff --git a/src/servers/sidecar/notify/resolve-user.ts b/src/servers/sidecar/notify/resolve-user.ts new file mode 100644 index 00000000..9b8220ce --- /dev/null +++ b/src/servers/sidecar/notify/resolve-user.ts @@ -0,0 +1,35 @@ +// Who a notification is for, and which of the two claimants wins. +// +// Two kinds of caller reach POST /_officer/notify and they are told apart by nothing but this header: +// +// - A producer inside the tailnet (the queue, the email sidecar, the agent) POSTs over loopback. It +// has no session to speak from, so naming a user in the body is its ONLY option. +// - A signed-in browser arrives through createSidecarProxy, which injects X-Officer-User from the +// token the platform just authenticated. The browser cannot know its own numeric id, and must not +// be trusted with it if it did. +// +// So the header wins wherever it is present. This lives in its own module, away from the Bun.serve +// entrypoint, purely so it can be tested — it is an access-control decision, and the version that read +// `body.userId ?? header` let any account holding the `notify` capability push to any other account's +// devices by naming them in a JSON body. + +type ResolveUserParams = { + /** Raw X-Officer-User header value, or null when absent. */ + header: string | null; + /** `userId` as it appeared in the request body — unvalidated, may be anything. */ + bodyUserId: unknown; +}; + +const asUserId = (v: unknown): number | null => (typeof v === 'number' && Number.isInteger(v) && v > 0 ? v : null); + +/** The user to notify, or null when neither claimant supplied a usable id. */ +export function resolveNotifyUser({ header, bodyUserId }: ResolveUserParams): number | null { + // PRESENCE of the header is the signal, not its validity. A header that is present but unparseable + // means a proxied request went wrong, and falling through to the body there would hand the decision + // straight back to the caller we just declined to trust — send junk, name yourself in the body, win. + if (header !== null) return asUserId(Number(header)); + + // No header at all: a loopback producer. Its body is honoured, but still has to be a real id — a + // string "13" is a producer bug worth surfacing as a 400 rather than coercing into a silent success. + return asUserId(bodyUserId); +} diff --git a/src/servers/sidecar/notify/types.ts b/src/servers/sidecar/notify/types.ts index 87c147e4..e59b5517 100644 --- a/src/servers/sidecar/notify/types.ts +++ b/src/servers/sidecar/notify/types.ts @@ -10,7 +10,7 @@ export type NotifyType = 'job' | 'mail' | 'agent' | 'download' | 'test'; export type Notification = { type: NotifyType; - /** Who to notify. Single-user today, but the registry is keyed by user, so this stays explicit. */ + /** Who to notify. Resolved by ./resolve-user.ts — a proxied header beats anything a body claims. */ userId: number; /** * The thing this is about, if there is one — a job id, a session id. The device fetches by it.