make the notify identity rule testable, and fix what the test found

Extracting resolveNotifyUser into its own module immediately caught a hole
in the fix from the previous commit: a header that was present but
unparseable fell through to the body, so a browser could send junk in the
header, name any user in the body and win.

PRESENCE of X-Officer-User is the signal, not its validity — a malformed
header means a proxied request went wrong, and falling through hands the
decision back to the caller we just declined to trust.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

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 ──
 

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();
+  });
+});

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);
+}

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.