diff --git a/TODO.md b/TODO.md index b234e612..0924e46b 100644 --- a/TODO.md +++ b/TODO.md @@ -698,19 +698,12 @@ outside the example's new README: unreachable from the LAN while localhost and Tailscale work. Fix: `sudo ufw allow from 192.168.47.0/24 to any port 9010 proto tcp` -- [ ] **Move the APNs `.p8` into the secret store — and teach the store to hold a foreign secret.** - Agreed 2026-08-19 when the key went into `.env` as an interim. The store's own argument applies - almost verbatim: `.env` is auto-loaded by bun into every PM2 process, so a credential there is - readable from `/proc//environ` of twenty processes — `officer-music` and `officer-pty` - would both be able to read a key that can push to your apps forever and does not expire. - - **The blocker is not a purpose string.** `getKey(purpose)` GENERATES a key and returns it; there - is no setter, because everything in the store so far is a key the platform minted itself. The - `.p8`, the APNs Key ID, the Team ID and the FCM service-account JSON are all supplied from - outside, so this needs a `putSecret(purpose, value)` and a decision about who may call it. - - Same applies to `DISCORD_WEBHOOK_URL` and every plugin credential still read from env. - See `docs/secret-store.md` and `docs/push-notifications.md`. +- [ ] **Move the remaining env credentials into the secret store.** `putSecret`/`getSecret` and + `bun run secret:set` landed 2026-08-19, and notify now reads `apns-p8`, `apns-key-id`, + `apns-team-id`, `fcm-service-account` and `discord-webhook` from the store with env as a + fallback. Every other plugin credential is still env-only — same argument applies: bun + auto-loads `.env` into every PM2 process, so each one is readable from `/proc//environ` + of twenty processes that have no business with it. ## Backburner diff --git a/docs/push-notifications.md b/docs/push-notifications.md index 76d1de49..3b1df8b5 100644 --- a/docs/push-notifications.md +++ b/docs/push-notifications.md @@ -205,16 +205,23 @@ Env only, on the sidecar, never in the platform process and never in the databas The `.p8` does not expire and can push to your apps forever. Treat it as spending-grade. -**Env is the interim, not the destination.** These belong in the secret store -(`src/databases/officer_db/src/secret-store.ts`, built 2026-08-13), and for the reason that store exists: -bun auto-loads `.env` into every PM2 process, so a key there is readable from `/proc//environ` of -twenty processes — `officer-music` has no business being able to read a credential that can push to your -apps forever. +**The secret store is where these live now** — env is only the fallback. -It cannot move there yet, and the obstacle is not a missing purpose string. `getKey(purpose)` **generates** -a key; there is no setter, because every secret in the store so far is one the platform minted itself. The -`.p8` and the FCM service account come from outside, so holding them needs a `putSecret` first. Tracked in -`TODO.md`. +```bash +cat AuthKey_ABCD123456.p8 | bun run secret:set apns-p8 +echo -n ABCD123456 | bun run secret:set apns-key-id +echo -n RGHC4NJC77 | bun run secret:set apns-team-id +pm2 restart officer-notify # _health should then report channels: ["apns"] +``` + +Purposes: `apns-p8`, `apns-key-id`, `apns-team-id`, `fcm-service-account`, `discord-webhook`. Read with +`getSecret` and NOT `getKey` — `getKey` mints a random value for an absent purpose, which for a credential +supplied from outside would mean silently authenticating to Apple with a fake. + +Env is still read when the store has nothing, so an install that predates this keeps working. But it is a +fallback: bun auto-loads `.env` into every PM2 process, so a key there is readable from `/proc//environ` +of twenty of them, and `officer-music` has no business being able to read a credential that can push to your +apps forever. `APNS_ENV` appeared in the first design's table and is deliberately **not** implemented: the host is chosen per device from the `environment` column, not per process, so one server pushes to a TestFlight build and a diff --git a/package.json b/package.json index 73767958..75a71b8d 100644 --- a/package.json +++ b/package.json @@ -11,6 +11,8 @@ "preinstall": "node -e \"var v = +process.versions.node.split('.')[0]; if (v < 22) { console.error('Node 22 or newer required (got ' + process.versions.node + '). Run: nvm use 22'); process.exit(1); }\"", "gen:index": "bun run ./scripts/gen-index.ts", "fix:env": "bun run ./scripts/fix-env.ts", + "secret:set": "bun run --env-file=.env ./scripts/secret-set.ts", + "notify:test": "bun run --env-file=.env ./scripts/notify-test.ts", "gen:ecosystem": "bun run --env-file=.env ./scripts/gen-ecosystem.ts", "predev": "bun run ./scripts/gen-index.ts", "dev": "bun --env-file=.env --watch src/server.tsx", diff --git a/scripts/notify-test.ts b/scripts/notify-test.ts new file mode 100644 index 00000000..d57309b3 --- /dev/null +++ b/scripts/notify-test.ts @@ -0,0 +1,108 @@ +#!/usr/bin/env bun +/** + * Fire one notification at a registered device, to prove the pipe. + * + * bun run notify:test # officer/core:test to user 1 + * bun run notify:test --user 2 # someone else + * bun run notify:test --code officer/core:job --id 412 + * + * Talks to the sidecar over loopback, which is how every other producer reaches it — so this exercises + * the real path rather than a special one. It reports what the channels actually did, because the thing + * that wastes a day here is that a send with nothing configured returns success and delivers nothing. + */ +import { readFileSync } from 'node:fs'; +import { join } from 'node:path'; +import { DATA_PATH } from '../src/servers/data-path'; + +const arg = (name: string): string | undefined => { + const i = process.argv.indexOf(`--${name}`); + return i === -1 ? undefined : process.argv[i + 1]; +}; + +const code = arg('code') ?? 'officer/core:test'; +const userId = Number(arg('user') ?? 1); +const id = arg('id'); +const n = arg('n'); + +const addressFile = join(DATA_PATH, 'notify-server.url'); + +let base: string; +try { + base = readFileSync(addressFile, 'utf8').trim(); +} catch { + console.error(`[notify:test] no address at ${addressFile}`); + console.error(' officer-notify is not running — `pm2 start ecosystem.config.cjs --only officer-notify`'); + process.exit(1); +} + +// The file is only a hint: a crash leaves it behind, so prove the listener is there before believing it. +let health: { ok: boolean; channels: string[] }; +try { + const res = await fetch(`${base}/_health`, { signal: AbortSignal.timeout(5000) }); + health = (await res.json()) as typeof health; +} catch { + console.error(`[notify:test] ${base} is not answering — stale address file, or the sidecar is down`); + process.exit(1); +} + +console.log(`[notify:test] sidecar ${base}`); +console.log(`[notify:test] channels configured: ${health.channels.length ? health.channels.join(', ') : 'NONE'}`); + +if (health.channels.length === 0) { + // Said before sending rather than after, because the send below WILL return ok and deliver nothing. + console.error(''); + console.error('[notify:test] nothing is configured, so this cannot reach a device.'); + console.error(' APNs needs apns-p8, apns-key-id and apns-team-id:'); + console.error(' cat AuthKey_XXXXXXXXXX.p8 | bun run secret:set apns-p8'); + console.error(' echo -n XXXXXXXXXX | bun run secret:set apns-key-id'); + console.error(' echo -n RGHC4NJC77 | bun run secret:set apns-team-id'); + console.error(' then: pm2 restart officer-notify'); + process.exit(1); +} + +const body = { + code, + userId, + ...(id === undefined ? {} : { id }), + ...(n === undefined ? {} : { n: Number(n) }), +}; + +console.log(`[notify:test] sending ${JSON.stringify(body)}`); + +const res = await fetch(`${base}/_officer/notify`, { + method: 'POST', + headers: { 'content-type': 'application/json', 'X-Officer-User': String(userId) }, + body: JSON.stringify(body), + signal: AbortSignal.timeout(20_000), +}); + +const out = (await res.json()) as { ok?: boolean; error?: string; results?: Array> }; + +if (!res.ok || out.error) { + console.error(`[notify:test] refused (${res.status}): ${out.error ?? 'unknown'}`); + process.exit(1); +} + +const results = out.results ?? []; +if (results.length === 0) { + console.error('[notify:test] no channel took it — nothing configured, or no device registered for this user'); + process.exit(1); +} + +let delivered = 0; +for (const r of results) { + const sent = Number(r.sent ?? 0); + delivered += sent; + const dead = Array.isArray(r.dead) && r.dead.length ? `, ${r.dead.length} dead token(s) deleted` : ''; + console.log(`[notify:test] ${r.channel}: sent ${sent}, failed ${r.failed}${dead}`); +} + +// `sent: 0, failed: 0` is the quiet one: the channel is configured but the user has no device of that +// platform registered. Worth naming, because it looks like success. +if (delivered === 0) { + console.error('[notify:test] nothing was delivered — is a device registered for this user?'); + console.error(' check: GET /api/notify/_officer/devices'); + process.exit(1); +} + +console.log(`[notify:test] delivered to ${delivered} device(s) — look at your phone`); diff --git a/scripts/secret-set.ts b/scripts/secret-set.ts new file mode 100644 index 00000000..8d249013 --- /dev/null +++ b/scripts/secret-set.ts @@ -0,0 +1,50 @@ +#!/usr/bin/env bun +/** + * Put a secret the platform did not generate into the secret store. + * + * bun run secret:set apns-p8 < AuthKey_ABCD123456.p8 + * pbpaste | bun run secret:set fcm-service-account + * + * ── Why stdin, and no value argument ── + * + * A secret passed as `argv` is in the shell history, in `ps` output for the life of the process, and in + * any audit log that records command lines. Stdin is none of those. There is deliberately no `--value` + * flag: offering one would mean the convenient path is the leaky one. + * + * Multi-line input is kept verbatim, which is what a PEM needs — no escaping, unlike `.env`. + */ +import { putSecret, getSecret, secretStorePath } from '../src/databases/officer_db/src/secret-store'; + +const purpose = process.argv[2]; + +if (!purpose || purpose.startsWith('-')) { + console.error('usage: bun run secret:set < file'); + console.error(' cat AuthKey_ABCD123456.p8 | bun run secret:set apns-p8'); + process.exit(2); +} + +const value = (await Bun.stdin.text()).trim(); + +if (!value) { + console.error(`[secret:set] nothing on stdin — refusing to store an empty secret for '${purpose}'`); + console.error(' (did you mean `bun run secret:set < file`?)'); + process.exit(1); +} + +const previous = putSecret(purpose, value); + +// The value itself is never printed, here or anywhere. Enough to confirm the right thing landed: +// the length and last four characters identify a paste without reproducing it. +const fingerprint = `${value.length} chars, ending …${value.slice(-4)}`; + +if (previous === null) { + console.log(`[secret:set] '${purpose}' stored — ${fingerprint}`); +} else if (previous === value) { + console.log(`[secret:set] '${purpose}' unchanged — ${fingerprint}`); +} else { + console.log(`[secret:set] '${purpose}' REPLACED — ${fingerprint}`); + console.log(` the previous value is retired, not deleted (retiredKeys('${purpose}') has it)`); +} + +console.log(`[secret:set] store: ${secretStorePath()}`); +console.log(`[secret:set] read back: ${getSecret(purpose) === value ? 'ok' : 'MISMATCH — do not restart anything'}`); diff --git a/src/databases/officer_db/src/secret-store.ts b/src/databases/officer_db/src/secret-store.ts index 923ee9ba..763cb632 100644 --- a/src/databases/officer_db/src/secret-store.ts +++ b/src/databases/officer_db/src/secret-store.ts @@ -144,6 +144,49 @@ export function getKey(purpose: string): string { } } +/** + * Read a secret WITHOUT creating one. Null when the purpose has never been set. + * + * `getKey` is the wrong door for anything supplied from outside: it mints a random 32-byte value when the + * purpose is absent, which for a foreign credential means silently substituting a key that authenticates + * to nothing. A caller that has an env fallback needs to be told "not here", not handed a fake. + */ +export function getSecret(purpose: string): string | null { + return readActive(purpose); +} + +/** + * Store a secret the platform did NOT generate — an APNs `.p8`, an FCM service account, a webhook URL. + * + * Everything else in this store is minted here, which is why `getKey` creates on demand. These arrive + * from outside and can never be regenerated, so they are written rather than derived, and replacing one + * RETIRES the old row instead of deleting it: a mistyped paste that overwrites a working key should be + * recoverable, and `retiredKeys` already exists for exactly this shape. + * + * Returns the previous value, or null if there was none — so a caller can tell "set" from "replaced" + * without reading first and racing itself. + */ +export function putSecret(purpose: string, value: string): string | null { + if (!value) throw new Error('secret-store: refusing to store an empty secret'); + + const handle = open(); + const now = Math.floor(Date.now() / 1000); + + // One transaction, because the partial unique index means the retire MUST land before the insert — + // two active rows for one purpose is the state it exists to prevent. + const previous = handle.transaction((): string | null => { + const existing = readActive(purpose); + if (existing === value) return existing; // idempotent: re-running a setup script is not a rotation + if (existing !== null) { + handle.query('UPDATE keys SET retired_at = ? WHERE purpose = ? AND retired_at IS NULL').run(now, purpose); + } + handle.query('INSERT INTO keys (purpose, key, created_at) VALUES (?, ?, ?)').run(purpose, value, now); + return existing; + })(); + + return previous; +} + /** Whether a purpose has an active key, without creating one. */ export function hasKey(purpose: string): boolean { return readActive(purpose) !== null; diff --git a/src/servers/sidecar/notify/apns.ts b/src/servers/sidecar/notify/apns.ts index d44faa54..c6c8018b 100644 --- a/src/servers/sidecar/notify/apns.ts +++ b/src/servers/sidecar/notify/apns.ts @@ -1,4 +1,5 @@ import http2 from 'node:http2'; +import { getSecret } from 'officerdb/secret-store'; import { getPushDevices, deletePushDevice, recordPushFailure, markPushDeviceSeen } from 'officerdb'; import type { Channel, DeliveryResult, Notification, SendContext } from './types'; import { ApnsAuthToken } from './apns-jwt'; @@ -22,10 +23,32 @@ const HOSTS = { /** Apple's reasons for "this token is dead" — the row goes immediately rather than after three strikes. */ const DEAD_TOKEN_REASONS = new Set(['BadDeviceToken', 'Unregistered', 'DeviceTokenNotForTopic']); -// `OFFICERDEV_X ?? X` — the house convention since the env prefixing, so an install that predates it -// keeps working. See src/servers/env-prefix.ts. +// Secret store first, env second. +// +// The store is where these belong: bun auto-loads `.env` into every PM2 process, so a key there is +// readable from `/proc//environ` of twenty of them, and `officer-music` has no business being able +// to read a credential that can push to the owner's apps forever and never expires. +// +// `getSecret`, NOT `getKey` — `getKey` mints a random value when the purpose is absent, which for a +// credential supplied from outside would mean silently authenticating to Apple with a fake. Null is the +// answer that lets the env fallback work. +// +// Env stays as the fallback so an install that predates the store, or one mid-migration, keeps working. +// `OFFICERDEV_X ?? X` is the house convention — see src/servers/env-prefix.ts. const env = (name: string) => process.env[`OFFICERDEV_${name}`] ?? process.env[name]; -const [APNS_KEY_P8, APNS_KEY_ID, APNS_TEAM_ID] = [env('APNS_KEY_P8'), env('APNS_KEY_ID'), env('APNS_TEAM_ID')]; +const stored = (purpose: string): string | undefined => { + try { + return getSecret(purpose) ?? undefined; + } catch { + // A store that cannot be opened must not take the sidecar down: the env fallback may well hold the + // key, and a notifier that refuses to boot is worse than one with a channel switched off. + return undefined; + } +}; + +const APNS_KEY_P8 = stored('apns-p8') ?? env('APNS_KEY_P8'); +const APNS_KEY_ID = stored('apns-key-id') ?? env('APNS_KEY_ID'); +const APNS_TEAM_ID = stored('apns-team-id') ?? env('APNS_TEAM_ID'); const auth = APNS_KEY_P8 && APNS_KEY_ID && APNS_TEAM_ID diff --git a/src/servers/sidecar/notify/discord.ts b/src/servers/sidecar/notify/discord.ts index 5e0bb496..b04988fc 100644 --- a/src/servers/sidecar/notify/discord.ts +++ b/src/servers/sidecar/notify/discord.ts @@ -1,3 +1,4 @@ +import { getSecret } from 'officerdb/secret-store'; import type { Channel, Notification, SendContext } from './types'; import { renderNotification } from './render'; @@ -15,7 +16,16 @@ import { renderNotification } from './render'; // and its history is searchable and permanent in a way a push banner is not — arguably it deserves the // rule more, not less. -const WEBHOOK_URL = process.env.OFFICERDEV_DISCORD_WEBHOOK_URL ?? process.env.DISCORD_WEBHOOK_URL; +// Store first, env second — same reasoning as apns.ts. +const WEBHOOK_URL = (() => { + try { + const stored = getSecret('discord-webhook'); + if (stored) return stored; + } catch { + /* unreadable store — fall through to env */ + } + return process.env.OFFICERDEV_DISCORD_WEBHOOK_URL ?? process.env.DISCORD_WEBHOOK_URL; +})(); export const discordChannel: Channel = { name: 'discord', diff --git a/src/servers/sidecar/notify/fcm.ts b/src/servers/sidecar/notify/fcm.ts index 998533fb..e7833ccf 100644 --- a/src/servers/sidecar/notify/fcm.ts +++ b/src/servers/sidecar/notify/fcm.ts @@ -1,3 +1,4 @@ +import { getSecret } from 'officerdb/secret-store'; import { getPushDevices, deletePushDevice, recordPushFailure, markPushDeviceSeen } from 'officerdb'; import type { Channel, DeliveryResult, Notification, SendContext } from './types'; import { FcmAccessToken, parseServiceAccount } from './fcm-auth'; @@ -17,7 +18,16 @@ const ENDPOINT = (projectId: string) => `https://fcm.googleapis.com/v1/projects/ /** Google's ways of saying "this token is dead" — the row goes immediately. */ const DEAD_TOKEN_CODES = new Set(['UNREGISTERED', 'INVALID_ARGUMENT', 'NOT_FOUND']); -const raw = process.env.OFFICERDEV_FCM_SERVICE_ACCOUNT ?? process.env.FCM_SERVICE_ACCOUNT; +// Store first, env second — same reasoning as apns.ts. +const raw = (() => { + try { + const stored = getSecret('fcm-service-account'); + if (stored) return stored; + } catch { + /* unreadable store — fall through to env */ + } + return process.env.OFFICERDEV_FCM_SERVICE_ACCOUNT ?? process.env.FCM_SERVICE_ACCOUNT; +})(); const auth = raw ? new FcmAccessToken(parseServiceAccount(raw)) : null; type FcmMessage = { diff --git a/src/servers/sidecar/notify/index.ts b/src/servers/sidecar/notify/index.ts index e0acf095..5c13c66f 100644 --- a/src/servers/sidecar/notify/index.ts +++ b/src/servers/sidecar/notify/index.ts @@ -7,6 +7,9 @@ import { resolveNotifyUser } from './resolve-user'; import { closeApnsSessions } from './apns'; import type { Notification } from './types'; import { API_URL } from '../../officer-url.mjs'; +import { DATA_PATH } from '../../data-path'; +import { writeFileSync, rmSync } from 'node:fs'; +import { join } from 'node:path'; // The officer-notify sidecar. The one place anything leaves this machine to tell the owner something. // @@ -51,6 +54,13 @@ function getFreePort(): number { const port = getFreePort(); +// Where this sidecar can be reached, written down for processes that are not the platform. +// +// The port is ephemeral by design, and the platform learns it through sidecar registration — but a CLI +// (`bun run notify:test`) has no session and no registry, and scraping `pm2 logs` for it is the kind of +// thing that works until it doesn't. One file, rewritten on every boot, deleted on clean shutdown. +const ADDRESS_FILE = join(DATA_PATH, 'notify-server.url'); + const server = Bun.serve({ port, hostname: '127.0.0.1', @@ -112,6 +122,8 @@ const server = Bun.serve({ }, }); +writeFileSync(ADDRESS_FILE, `http://127.0.0.1:${server.port}\n`); + console.log( `[notify] listening on http://127.0.0.1:${server.port} — channels: ${configuredChannels().join(', ') || 'none configured'}`, ); @@ -140,6 +152,9 @@ const connection = createSidecarConnector({ function shutdown(signal: string) { console.log(`[notify] ${signal} received, shutting down...`); + // So a stale address cannot outlive the listener. A crash still leaves one behind, which is why the + // CLI treats an unreachable address as "not running" rather than trusting the file. + rmSync(ADDRESS_FILE, { force: true }); closeApnsSessions(); connection.destroy(); process.exit(0);