fix(webhooks): stop storing the subscriber signing secret in cleartext (#7799)

.changeset/webhook-subscriber-signing-secret-encrypted.md

@@ -0,0 +1,52 @@
+---
+"@objectstack/plugin-webhooks": patch
+"@objectstack/objectql": patch
+---
+
+fix(webhooks): the subscriber's HMAC signing secret is no longer readable from `sys_webhook` over the data API (#7799)
+
+`bootstrapDeclaredWebhooks` persisted the whole validated `Webhook` envelope —
+**`secret` included** — as `definition_json: JSON.stringify(wh)`, and
+`AutoEnqueuer.parseRow` read `defn.secret` straight back out to sign deliveries.
+`definition_json` is an ordinary textarea on an admin-authorable object with no
+restrictive `enable.apiMethods`, so a plain `GET /api/v1/data/sys_webhook`
+returned the key to **every persona that can read the object**. That key is the
+receiver's only proof that a delivery came from us.
+
+This is the remaining half of #7722, which removed the same secret's per-attempt
+copies from `sys_http_delivery`. Unlike the delivery table, no retention window
+ever aged these out.
+
+**What changed.** The authored key now lands in `sys_webhook.signing_secret`, a
+new `type: 'secret'` column: the engine encrypts it into `sys_secret` on write,
+keeps only an opaque `secret:<id>` ref on the row, and returns a mask on every
+read path. `definition_json` carries the same envelope **minus** `secret`. The
+auto-enqueuer recovers the plaintext server-side when it refreshes its
+subscription cache.
+
+**Nothing about authoring changes.** `packages/spec/src/automation/webhook.zod.ts`
+is untouched — `defineWebhook({ secret })` is written exactly as before, and the
+delivered `X-Objectstack-Signature` is byte-identical, so no receiver has to
+change anything.
+
+**Existing rows are migrated.** A boot sweep moves any cleartext
+`definition_json.secret` into the encrypted column — including the rows the
+seeder deliberately never rewrites (`managed_by: 'admin'`, and package rows an
+admin froze with `customized: true`), which are the ones most likely to hold a
+real production key. The sweep is idempotent and stores the encrypted copy in
+the same update that strips the blob, so a failure can never leave a webhook
+stripped *and* unsigned. Until a row is swept, signing keeps working from the
+legacy blob and the enqueuer warns that the value is still exposed.
+
+**Fail-closed.** With no `ICryptoProvider` wired the engine refuses the write
+rather than storing cleartext, so a secret-bearing webhook is skipped — and a
+legacy row is left intact — with an actionable log line carrying an ADR-0112
+`code`/`status` pair. It is never seeded with an exposed key in a new column.
+
+Also adds `ObjectQL.resolveSecretField(object, recordId, field)` — the privileged,
+driver-level dereference of one row's `secret`-typed field. `resolveSecret()` was
+already documented for "privileged consumers … against the stored ref", but the
+read mask meant no consumer could obtain that ref; this is why the webhook key
+can live in the encrypted channel at all. It refuses any field not declared
+`type: 'secret'`, so it cannot become a mask bypass over a `password` field
+(plaintext at rest by design — ADR-0100).

packages/objectql/src/engine.ts

@@ -4837,6 +4837,58 @@ export class ObjectQL implements IObjectQLEngine {
  });
  }
 
+ /**
+ * Privileged: recover the plaintext of ONE row's `secret`-typed field.
+ *
+ * {@link resolveSecret} is documented for "privileged consumers … against the
+ * stored ref", but until #7799 there was no supported way for such a consumer
+ * to OBTAIN that ref: {@link maskSecretFields} replaces it with
+ * {@link SECRET_MASK} on every `find`/`findOne`, unconditionally and after
+ * hooks. A server-side reader that genuinely needs the value therefore had
+ * two options, both bad — keep the credential in cleartext somewhere the read
+ * path does not mask (which is the defect #7799 reports on
+ * `sys_webhook.definition_json`), or reach around the engine into the driver
+ * from a plugin. This method is the supported third option, and it is why the
+ * webhook signing secret can now live in the encrypted channel at all.
+ *
+ * The row is read at DRIVER level on purpose — that is the only layer where
+ * the ref still exists — which means it bypasses read hooks, field-level
+ * security and sharing. That is the same trust `resolveSecret` already places
+ * in its caller, and the reason both are spelled as explicit, separately-named
+ * privileged verbs rather than an option on `find`: there is no query string
+ * that reaches this, so it cannot be turned on from outside the process.
+ *
+ * Refuses any field that is not declared `type: 'secret'`. Without that guard
+ * this would be a generic mask-bypass — in particular over a `password` field,
+ * which is stored as PLAINTEXT at rest and is masked for exactly that reason
+ * (ADR-0100). Only the encrypted channel is dereferenceable.
+ *
+ * Fail-closed like {@link resolveSecret}: throws when no CryptoProvider is
+ * registered or the `sys_secret` row has gone missing. Returns `null` when the
+ * row does not exist or the field holds no secret (never set, or cleared).
+ */
+ async resolveSecretField(
+ object: string,
+ recordId: string,
+ field: string,
+ opts?: { tenantId?: string },
+ ): Promise<string | null> {
+ const schema = this._registry.getObject(object);
+ if (!collectSecretFields(schema).includes(field)) {
+ throw new Error(
+ `Cannot resolve secret field "${object}.${field}": it is not declared as type 'secret'. `
+ + 'Only the encrypted secret channel is dereferenceable — a `password` field is stored as '
+ + 'plaintext at rest and is masked deliberately (ADR-0100), so dereferencing one here '
+ + 'would be a mask bypass, not a decrypt.',
+ );
+ }
+ const driver = this.getDriver(object);
+ const found = await driver.find(object, { where: { id: recordId } });
+ const row: any = Array.isArray(found) ? found[0] : found;
+ if (!row) return null;
+ return this.resolveSecret(row[field], opts);
+ }
+
  /**
  * Helper to get object definition
  */

packages/objectql/src/secret-fields.test.ts

@@ -33,31 +33,37 @@ function makeStubDriver() {
  }
  return true;
  };
+ // Rows leave the driver as COPIES, like a real driver's do. Handing out the
+ // live stored object makes the double lie: `maskSecretFields` mutates the
+ // rows it is given, so one `find` would stamp SECRET_MASK over the stored ref
+ // and the next `resolveSecret*` would read the mask back as "no secret" — an
+ // artefact of the double that reads exactly like an engine bug (#7799).
+ const copy = <T,>(r: T): T => (r == null ? r : ({ ...r } as T));
  const driver: any = {
  name: 'memory', version: '0.0.0', supports: {},
  async connect() {}, async disconnect() {}, async checkHealth() { return true; },
  async execute() { return null; },
  async find(object: string, ast: any) {
- return Array.from(storeFor(object).values()).filter((r) => matches(r, ast?.where));
+ return Array.from(storeFor(object).values()).filter((r) => matches(r, ast?.where)).map(copy);
  },
  async findOne(object: string, ast: any) {
- for (const r of storeFor(object).values()) if (matches(r, ast?.where)) return r;
+ for (const r of storeFor(object).values()) if (matches(r, ast?.where)) return copy(r);
  return null;
  },
  async create(object: string, data: Record<string, unknown>) {
  nextId += 1;
  const id = (data.id as string) ?? `r_${nextId}`;
  const row = { ...data, id };
  storeFor(object).set(id, row);
- return row;
+ return copy(row);
  },
  async update(object: string, id: string, data: Record<string, unknown>) {
  const s = storeFor(object);
  const cur = s.get(id);
  if (!cur) throw new Error(`not found: ${object}/${id}`);
  const updated = { ...cur, ...data, id };
  s.set(id, updated);
- return updated;
+ return copy(updated);
  },
  async upsert(object: string, data: Record<string, unknown>) {
  const id = data.id as string | undefined;
@@ -204,6 +210,44 @@ describe('objectql secret-field channel', () => {
  expect(secretReads[0].ast.where).toEqual({ id: expect.any(String) });
  });
 
+ // [#7799] `resolveSecret` is documented for privileged consumers "against the
+ // stored ref" — but the read mask means no consumer can OBTAIN that ref, so
+ // the only ways to reach a stored credential were to keep a cleartext copy
+ // somewhere unmasked (the defect #7799 reports on `sys_webhook`) or to reach
+ // past the engine into the driver. This is the supported third way.
+ it("resolveSecretField recovers one row's plaintext without the caller ever seeing the ref", async () => {
+ const created = await ctx.engine.insert('ext_datasource', { name: 'pg', db_password: 's3cr3t' });
+
+ // The caller has ONLY what the generic read path gives it — the mask.
+ const viaFind = (await ctx.engine.find('ext_datasource', { where: { id: created.id } }))[0] as any;
+ expect(viaFind.db_password).toBe(SECRET_MASK);
+
+ expect(await ctx.engine.resolveSecretField('ext_datasource', created.id, 'db_password')).toBe('s3cr3t');
+ });
+
+ it('resolveSecretField returns null for an unset secret and for a row that is gone', async () => {
+ const created = await ctx.engine.insert('ext_datasource', { name: 'pg' });
+ expect(await ctx.engine.resolveSecretField('ext_datasource', created.id, 'db_password')).toBeNull();
+ expect(await ctx.engine.resolveSecretField('ext_datasource', 'nope', 'db_password')).toBeNull();
+ });
+
+ it('resolveSecretField refuses a non-secret field — it is a decrypt, not a mask bypass', async () => {
+ const created = await ctx.engine.insert('ext_datasource', { name: 'pg', db_password: 's3cr3t' });
+ // A plain column is not dereferenceable…
+ await expect(
+ ctx.engine.resolveSecretField('ext_datasource', created.id, 'name'),
+ ).rejects.toThrow(/not declared as type 'secret'/i);
+
+ // …and neither is a `password` field, which is PLAINTEXT at rest and masked
+ // for exactly that reason (ADR-0100). Allowing it here would hand back the
+ // very value the mask exists to withhold.
+ const pw = await buildPasswordEngine();
+ const device = await pw.engine.insert('device', { name: 'router', admin_password: 'hunter2' });
+ await expect(
+ pw.engine.resolveSecretField('device', device.id, 'admin_password'),
+ ).rejects.toThrow(/not declared as type 'secret'/i);
+ });
+
  it('fail-closed: writing a secret field with no CryptoProvider throws', async () => {
  const bare = await buildEngine(false);
  await expect(

packages/plugins/plugin-webhooks/package.json

@@ -30,6 +30,7 @@
  },
  "devDependencies": {
  "@objectstack/metadata-core": "workspace:*",
+ "@objectstack/objectql": "workspace:*",
  "@types/node": "^26.1.2",
  "typescript": "^6.0.3",
  "vitest": "^4.1.10"

packages/plugins/plugin-webhooks/src/auto-enqueuer.ts

@@ -3,6 +3,7 @@
 import type { IDataEngine, IRealtimeService, RealtimeEventPayload } from '@objectstack/spec/contracts';
 import type { WebhookTriggerType } from '@objectstack/spec/automation';
 import type { EnqueueHttpInput } from '@objectstack/service-messaging';
+import { WEBHOOK_SECRET_FIELD, readLegacySecret, resolveWebhookSecret } from './webhook-secret.js';
 
 /**
  * The authored trigger vocabulary, taken from the spec rather than restated
@@ -201,6 +202,14 @@ export class AutoEnqueuer {
  for (const row of rows) {
  const sub = this.parseRow(row);
  if (!sub) continue;
+ // [#7799] The signing key is no longer in the row we just read — it
+ // lives encrypted in `sys_secret`, and this read path returns only a
+ // mask. Dereference it here, on the 60s refresh, rather than per
+ // event: the cache already holds the plaintext in memory (it always
+ // did), so this changes where the value comes FROM, not how long it
+ // is held. A row whose key cannot be recovered is DROPPED — see
+ // `attachSecret`.
+ if (!(await this.attachSecret(sub, row))) continue;
  // Empty objectName == "any object" → indexed under '*'.
  const key = sub.objectName ?? '*';
  const arr = next.get(key) ?? [];
@@ -217,6 +226,64 @@ export class AutoEnqueuer {
  });
  }
 
+ /**
+ * [#7799] Resolve `sub.secret` for one cached subscription. Returns `false`
+ * when the subscription must be dropped from the cache.
+ *
+ * Three sources, in order:
+ * 1. `sys_webhook.signing_secret` — the encrypted column. The read path
+ * returns a mask, so presence is decidable here but the value is not;
+ * `resolveWebhookSecret` dereferences it server-side.
+ * 2. `definition_json.secret` — a row not yet swept by
+ * `migrateLegacyWebhookSecrets` (or hand-edited back in). Still honoured
+ * so an un-migrated deployment keeps signing, and warned about once per
+ * refresh so the exposure is visible rather than silently permanent.
+ * 3. Neither — an unsigned webhook, which is a legitimate authored choice
+ * (`secret` is optional on the envelope).
+ *
+ * A stored-but-unresolvable key DROPS the subscription instead of
+ * delivering unsigned. The signature is the receiver's only proof of
+ * origin (#7722, #7799): a webhook that stops arriving is visible and gets
+ * investigated, while one that keeps arriving unsigned is invisible and
+ * teaches the receiver to accept unauthenticated traffic.
+ *
+ * Cost: one point read + one decrypt per secret-bearing row per refresh
+ * (default 60s), off the write path entirely. Deliberately NOT memoised
+ * across refreshes — the only cheap cache key would be `updated_at`, which
+ * nothing guarantees is stamped when a secret is rotated, and a stale key
+ * signs every delivery with a signature the receiver rejects.
+ */
+ private async attachSecret(sub: CachedSubscription, row: any): Promise<boolean> {
+ try {
+ const stored = await resolveWebhookSecret(this.engine, row, this.subscriptionsObject);
+ if (stored) {
+ sub.secret = stored;
+ return true;
+ }
+ } catch (err) {
+ this.logger.warn?.(
+ `[webhook-auto-enqueuer] webhook '${sub.name}' holds an encrypted signing secret that ` +
+ `could not be decrypted — the subscription is DROPPED rather than delivered unsigned ` +
+ `(#7799). Deliveries resume once the sys_secret row and CryptoProvider are reachable.`,
+ { id: sub.id, field: WEBHOOK_SECRET_FIELD, err: (err as Error)?.message ?? err },
+ );
+ return false;
+ }
+
+ const legacy = readLegacySecret(row?.definition_json);
+ if (legacy) {
+ this.logger.warn?.(
+ `[webhook-auto-enqueuer] webhook '${sub.name}' still carries its signing secret as ` +
+ `CLEARTEXT in definition_json, readable over the data API (#7799). Signing continues ` +
+ `from it; run the boot sweep (migrateLegacyWebhookSecrets) with a CryptoProvider wired ` +
+ `to move it into sys_secret.`,
+ { id: sub.id },
+ );
+ sub.secret = legacy;
+ }
+ return true;
+ }
+
  private parseRow(row: any): CachedSubscription | null {
  if (!row?.id || !row?.url) return null;
  // `triggers` is now authored as a multi-select (stored as an array), but
@@ -283,8 +350,9 @@ export class AutoEnqueuer {
  }
 
  // The "definition_json" field carries advanced config (headers,
- // secret, timeout); attempt a best-effort parse. Fall back to
- // top-level fields where present.
+ // timeout); attempt a best-effort parse. Fall back to top-level fields
+ // where present. It no longer carries the signing secret (#7799) —
+ // `attachSecret` sources that from the encrypted column.
  let defn: Record<string, any> = {};
  if (typeof row.definition_json === 'string' && row.definition_json.length > 0) {
  try {
@@ -306,7 +374,8 @@ export class AutoEnqueuer {
  // the select change (legacy rows stored 'POST').
  method: String(row.method ?? defn.method ?? 'POST').toUpperCase(),
  headers: defn.headers,
- secret: defn.secret,
+ // `secret` is filled by attachSecret() from the encrypted column,
+ // NOT read off the row — see #7799.
  timeoutMs: defn.timeoutMs,
  };
  }

packages/plugins/plugin-webhooks/src/bootstrap-declared-webhooks.test.ts

@@ -275,8 +275,12 @@ describe('bootstrapDeclaredWebhooks', () => {
 
  expect(calls).toHaveLength(1);
  expect(calls[0].url).toBe('https://hooks.example/task');
- // headers + secret came from the definition_json envelope the bridge wrote.
+ // Headers come from the definition_json envelope the bridge wrote. The
+ // signing secret does NOT (#7799) — it goes to the `signing_secret` column,
+ // which this fake engine (no encrypted-field channel) stores verbatim, so
+ // the enqueuer reads it back from there.
  expect(calls[0].signingSecret).toBe('shh');
+ expect(engine.rows['sys_webhook'][0].definition_json).not.toContain('shh');
  expect(calls[0].headers).toEqual({ 'X-Env': 'prod' });
  await ae.stop();
  });