Skip to content

Commit cdaa72f

Browse files
os-warrenclaude
andauthored
fix(service-messaging,plugin-webhooks): classify the delivery outboxes' update-op tenant-audit surface — ack is a dispatcher sweep, redeliver threads the caller's tenant (#11010)
* fix(service-messaging,plugin-webhooks): classify the update-op tenant-audit surface — ack is a dispatcher sweep, redeliver threads the caller's tenant Three single-record (multi:false) writes on sys_http_delivery / sys_notification_delivery are audited under the `update` op, and their classifications are opposite: - SqlNotificationOutbox.ack / SqlHttpOutbox.ack are reachable only from the dispatchers' setInterval tick under a cluster lock. Declared global sweeps via the new dispatcherAckOptions() helper, whose warrant is re-derived from this tree rather than inherited from the updateMany half. - SqlHttpOutbox.redeliver is served to any authenticated user through POST /api/v1/webhooks/redeliver. It now carries the caller's tenant, to the rows it reads as well as the row it writes, and never bypassTenantAudit. IHttpOutbox.redeliver(id, options) takes a required-but-nullable tenantId so a caller cannot omit the decision, and the webhook route threads the session's active organization into it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx * test(plugin-webhooks): pin the redeliver route's tenant threading, and the ADR-0112 code+status on a cross-tenant refusal Adds webhook-redeliver-tenant-scope.test.ts, the half the service-level test cannot reach: that the tenant comes FROM THE REQUEST (the session's activeOrganizationId) and that the cross-tenant refusal surfaces as RESOURCE_NOT_FOUND with HTTP 404 rather than a 500. Also carries the changeset and the last redeliverHttp call site. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx * chore(changeset): spell the ADR-0087 runtime-interface-only refs as <path>#<Symbol> The marker named `IHttpOutbox.redeliver` / `MessagingService.redeliverHttp`, member paths the gate's parseSymbolRef refuses by design: the symbol half must be a bare identifier it can find as an exported type declaration. Names the three declarations instead and says which members moved. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx * chore(changeset): name only the ADR-0087 refs this gate can verify check-adr-0087-registration refuses `MessagingService` as unresolvable — a prose comment in packages/spec/src/api/protocol.zod.ts mentions the class name without declaring or importing it, so the gate cannot rule it unrelated. The claim moves into the marker's prose, where a reviewer reads it rather than a checker appearing to have verified it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent c49007a commit cdaa72f

15 files changed

Lines changed: 864 additions & 47 deletions
Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
---
2+
"@objectstack/service-messaging": minor
3+
"@objectstack/plugin-webhooks": minor
4+
---
5+
6+
fix(service-messaging,plugin-webhooks): the `update`-op tenant-audit surface on the delivery outboxes is classified — `ack` is a dispatcher sweep, `redeliver` threads the caller's tenant (#10740)
7+
8+
**BREAKING** signature change on `IHttpOutbox.redeliver` and
9+
`MessagingService.redeliverHttp`, shipped as `minor` under the repo's
10+
launch-window convention for breaking changes.
11+
12+
`sys_http_delivery` and `sys_notification_delivery` carry three single-record
13+
(`multi: false`) writes that the SQL driver audits under the **`update`** op —
14+
a different op, and a different throttle key, from the `updateMany` half
15+
classified previously. Their correct classifications are **opposite**, and
16+
treating them as one sweep is the dangerous reading:
17+
18+
| site | reachable from | classification |
19+
| --- | --- | --- |
20+
| `SqlNotificationOutbox.ack` | dispatcher tick only | global sweep |
21+
| `SqlHttpOutbox.ack` | dispatcher tick only | global sweep |
22+
| `SqlHttpOutbox.redeliver` | `POST /api/v1/webhooks/redeliver` | request-contextual |
23+
24+
**The two `ack` sites** are declared global sweeps through a new
25+
`dispatcherAckOptions()` helper, sibling to `dispatcherSweepOptions()` and
26+
deliberately not the same function — that one returns `& { multi: true }`, so a
27+
`multi: false` site cannot borrow it by accident. The warrant was re-derived
28+
against the current tree rather than inherited: `ack` has exactly two callers,
29+
both inside `runPartition()` on a `setInterval` tick holding a per-partition
30+
cluster lock, so no request context exists to thread; and the row being acked
31+
was claimed by a sweep that crosses organizations by construction
32+
(`hash(refId | notificationId | digestKey) mod N` is a load-spreading key, and
33+
one outbox per environment drains the whole queue). Passing the claimed row's
34+
own `organization_id` is documented at the helper as the tempting wrong answer:
35+
a predicate read off the row you are about to write matches exactly that row,
36+
adds no isolation, and silences the audit anyway — the appearance of scoping
37+
without the substance.
38+
39+
**`redeliver` is not that**, and it is the reason this shipped separately. The
40+
route in front of it is served to any authenticated user, so on a walled
41+
deployment (`OS_TENANCY_POSTURE=isolated|group`) an unscoped replay is an
42+
authenticated user writing another organization's delivery row — the case the
43+
tenant audit exists to catch. It now carries the caller's tenant, applied to
44+
the rows it reads as well as the row it writes, and it must never be given
45+
`bypassTenantAudit`: a scoped write and a bypassed write produce the same
46+
silence in the log, so the flag would convert a detectable hole into an
47+
undetectable one. The webhook route resolves the session's
48+
`activeOrganizationId` and threads it.
49+
50+
Behaviour change at the endpoint: a delivery row outside the caller's
51+
organization is now **not found** (`RESOURCE_NOT_FOUND`, HTTP 404) rather than
52+
replayed. It is deliberately invisible rather than forbidden, so the endpoint
53+
is not an existence oracle for other tenants' delivery ids. An in-tenant
54+
redelivery is unchanged.
55+
56+
Migrating a caller: `redeliver(id, guard?)` becomes
57+
`redeliver(id, { tenantId, guard? })`, and `redeliverHttp(id)` becomes
58+
`redeliverHttp(id, { tenantId })`. `tenantId` is a **required** property typed
59+
`string | undefined`, so omitting it does not compile — a caller with no tenant
60+
has to write `tenantId: undefined` and mean it. That is the point of the shape:
61+
an optional property would let the dangerous case, a request path that simply
62+
forgot, type-check in silence. Passing `undefined` leaves the write unscoped
63+
and the audit line still fires, which is the intended reporting behaviour on a
64+
deployment that cannot resolve an organization for the caller.
65+
66+
<!-- adr-0087: not-required (runtime-interface-only packages/services/service-messaging/src/http-outbox.ts#IHttpOutbox, packages/services/service-messaging/src/http-outbox.ts#RedeliverOptions) The surface that changed shape is `IHttpOutbox.redeliver`, plus the new `RedeliverOptions` argument type beside it. Both are TypeScript declarations in a service package with no `packages/spec` schema behind them: no metadata author writes a `redeliver` key, there is no authorable spelling and no `retiredKey()` tombstone, so `os migrate meta` has no stack source to rewrite. The change is a required second argument on an in-process method — a compile error at every call site, which is the notification channel, not a silent runtime gap. `MessagingService.redeliverHttp` moves with it and is deliberately NOT in the list above: this gate refuses that symbol as unresolvable, because `packages/spec/src/api/protocol.zod.ts` mentions the class name in a prose comment about `MessagingService.listInbox` while neither declaring nor importing it. The claim would be true and the gate cannot check it, so it is stated here for a reviewer instead of asserted where it would read as verified. It is a thin delegate to `IHttpOutbox.redeliver` in the same package and carries no schema of its own either. -->

packages/plugins/plugin-webhooks/src/webhook-drop-durable-record.test.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -200,7 +200,11 @@ describe('dropped webhook subscription leaves a durable, unsendable record (#806
200200
await new HttpDispatcher({ nodeId: 'n1', outbox, fetchImpl: impl, partitionCount: 1 }).tick();
201201
expect(calls).toHaveLength(0);
202202

203-
await expect(messaging.redeliverHttp(row.id)).rejects.toMatchObject({
203+
// [#10740] `redeliverHttp` now requires the requesting caller's tenant.
204+
// This fixture has no organization and no tenancy posture, so
205+
// `undefined` is the honest value — required rather than optional
206+
// precisely so that answer is written down instead of defaulted into.
207+
await expect(messaging.redeliverHttp(row.id, { tenantId: undefined })).rejects.toMatchObject({
204208
code: 'DELIVERY_NEVER_SENT',
205209
});
206210
// The refusal did not mutate the row on its way out.

packages/plugins/plugin-webhooks/src/webhook-outbox-plugin.ts

Lines changed: 56 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,17 @@ import {
2828
interface MessagingHttpSurface {
2929
isHttpDeliveryReady(): boolean;
3030
enqueueHttp(input: EnqueueHttpInput): Promise<string>;
31-
redeliverHttp(id: string): Promise<{ id: string; status: string }>;
31+
/**
32+
* [#10740] Takes the REQUESTING caller's organization. Declared with the
33+
* required-but-nullable `tenantId` the service declares, so this plugin
34+
* cannot call the endpoint's backing method without deciding what tenant
35+
* the request carries — the omission this structural view would otherwise
36+
* type-check happily.
37+
*/
38+
redeliverHttp(
39+
id: string,
40+
options: { tenantId: string | undefined },
41+
): Promise<{ id: string; status: string }>;
3242
/**
3343
* [#8069] Where this plugin's veto over redelivering `source: 'webhook'`
3444
* rows is installed. Declared REQUIRED on this structural view even though
@@ -347,8 +357,21 @@ export class WebhookOutboxPlugin implements Plugin {
347357

348358
/**
349359
* Mount POST /api/v1/webhooks/redeliver on the host Hono app, if one is
350-
* available. Delegates to `messaging.redeliverHttp(deliveryId)`. Auth is the
351-
* better-auth session cookie — every authenticated user counts.
360+
* available. Delegates to `messaging.redeliverHttp(deliveryId, …)`. Auth is
361+
* the better-auth session cookie — every authenticated user counts.
362+
*
363+
* [#10740] Which is precisely why the caller's ACTIVE ORGANIZATION is
364+
* resolved here and threaded into the call. `sys_http_delivery` is
365+
* tenant-scoped, and this is the one door on it a request can reach: an
366+
* unscoped replay from here is an authenticated user reaching another
367+
* organization's delivery row on a walled deployment. With the tenant
368+
* threaded, a row outside the caller's organization is simply not found.
369+
*
370+
* ⚠️ A session with no active organization threads `undefined`, and the
371+
* driver's tenant-audit line then fires for that write. That is deliberate:
372+
* the deployment could not tell us who is asking, and reporting the gap is
373+
* the correct outcome. ⛔ It is never repaired with `bypassTenantAudit`,
374+
* which would silence the report without closing anything.
352375
*/
353376
private registerAdminRoutes(ctx: PluginContext): void {
354377
const http = this.tryGetService<any>(ctx, ['http-server']);
@@ -361,8 +384,9 @@ export class WebhookOutboxPlugin implements Plugin {
361384
if (!rawApp || !messaging) return;
362385

363386
rawApp.post('/api/v1/webhooks/redeliver', async (c: any) => {
364-
const userId = await this.resolveSessionUserId(ctx, c);
365-
if (!userId) {
387+
const session = await this.resolveSession(ctx, c);
388+
const userId = session?.user?.id;
389+
if (typeof userId !== 'string' || userId.length === 0) {
366390
return c.json(
367391
{ success: false, error: { code: 'UNAUTHENTICATED', message: 'Sign in to redeliver webhook deliveries.' } },
368392
401,
@@ -382,8 +406,20 @@ export class WebhookOutboxPlugin implements Plugin {
382406
);
383407
}
384408
try {
385-
const row = await messaging.redeliverHttp(deliveryId);
386-
ctx.logger.info?.('[webhook-outbox] redelivered', { deliveryId, requestedBy: userId });
409+
// [#10740] `session.session.activeOrganizationId` is the
410+
// canonical spelling of the caller's active organization
411+
// (better-auth's organization plugin; see
412+
// `plugin-auth/auth-schema-config.ts`). Read from the one
413+
// place it lives — a `??` chain over alternative spellings
414+
// would make a MISSING organization indistinguishable from a
415+
// differently-shaped one, and the missing case is the one that
416+
// must stay visible.
417+
const activeOrg = session?.session?.activeOrganizationId;
418+
const tenantId = typeof activeOrg === 'string' && activeOrg.length > 0
419+
? activeOrg
420+
: undefined;
421+
const row = await messaging.redeliverHttp(deliveryId, { tenantId });
422+
ctx.logger.info?.('[webhook-outbox] redelivered', { deliveryId, requestedBy: userId, tenantId });
387423
return c.json({ success: true, data: { id: row.id, status: row.status } });
388424
} catch (err: any) {
389425
const code = err?.code;
@@ -412,7 +448,18 @@ export class WebhookOutboxPlugin implements Plugin {
412448
ctx.logger.info?.('[webhook-outbox] redeliver endpoint mounted at POST /api/v1/webhooks/redeliver');
413449
}
414450

415-
private async resolveSessionUserId(ctx: PluginContext, c: any): Promise<string | undefined> {
451+
/**
452+
* [#10740] The better-auth session envelope (`{ user, session }`) for this
453+
* request, or `undefined`.
454+
*
455+
* Widened from the previous `resolveSessionUserId` because the route now
456+
* needs two facts from ONE lookup: who is asking (`user.id`, the
457+
* authentication gate) and which organization they are asking as
458+
* (`session.activeOrganizationId`, the tenant threaded into the write).
459+
* Resolving them separately would mean two `getSession` calls that can
460+
* disagree.
461+
*/
462+
private async resolveSession(ctx: PluginContext, c: any): Promise<any | undefined> {
416463
try {
417464
const authService: any = this.tryGetService<any>(ctx, ['auth']);
418465
if (!authService) return undefined;
@@ -421,9 +468,7 @@ export class WebhookOutboxPlugin implements Plugin {
421468
api = await authService.getApi();
422469
}
423470
if (!api?.getSession) return undefined;
424-
const session = await api.getSession({ headers: c.req.raw.headers });
425-
const uid = session?.user?.id;
426-
return typeof uid === 'string' && uid.length > 0 ? uid : undefined;
471+
return await api.getSession({ headers: c.req.raw.headers });
427472
} catch {
428473
return undefined;
429474
}
Lines changed: 167 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,167 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* #10740 — `POST /api/v1/webhooks/redeliver` carries the CALLER'S tenant.
5+
*
6+
* This route is the reason `SqlHttpOutbox.redeliver` is classified
7+
* request-contextual rather than as a dispatcher sweep: its auth gate is "any
8+
* authenticated user", and `sys_http_delivery` is a tenant-scoped object. On a
9+
* walled deployment (`OS_TENANCY_POSTURE=isolated`) an unscoped replay from
10+
* here is an authenticated user writing another organization's delivery row —
11+
* exactly what the driver's tenant audit exists to catch, and the one site
12+
* where silencing that audit would convert a detectable hole into an
13+
* undetectable one.
14+
*
15+
* ## What this file pins that the service-level test cannot
16+
* The tenant has to come from the REQUEST. `service-messaging`'s
17+
* `delivery-update-tenant-audit.integration.test.ts` proves the outbox applies
18+
* whatever tenant it is handed, right down to the options that reach the
19+
* driver; nothing there can prove the route hands it the right one, or hands
20+
* it anything at all. Here the session is the only source of the value, so a
21+
* route that dropped it would go red.
22+
*
23+
* It also pins the HTTP half of the ADR-0112 envelope: the service layer
24+
* carries the `code`, and the `status` exists only at this boundary. Both are
25+
* asserted for the cross-tenant refusal — a `code` assertion alone would not
26+
* notice the refusal surfacing as a 500.
27+
*/
28+
29+
import { describe, it, expect } from 'vitest';
30+
import { WebhookOutboxPlugin } from './webhook-outbox-plugin.js';
31+
32+
/** Captures the handler `registerAdminRoutes` mounts, and lets us call it. */
33+
function mountRoute(opts: {
34+
session: any;
35+
redeliverHttp: (id: string, options: { tenantId: string | undefined }) => Promise<any>;
36+
}): { post: (body: any) => Promise<{ status: number; json: any }> } {
37+
let handler: ((c: any) => Promise<any>) | undefined;
38+
const rawApp = {
39+
post(path: string, h: (c: any) => Promise<any>) {
40+
if (path === '/api/v1/webhooks/redeliver') handler = h;
41+
},
42+
};
43+
const services: Record<string, any> = {
44+
'http-server': { getRawApp: () => rawApp },
45+
messaging: {
46+
// `getMessaging` gates on this being a function.
47+
enqueueHttp: async () => 'unused',
48+
isHttpDeliveryReady: () => true,
49+
registerRedeliverGuard: () => {},
50+
redeliverHttp: opts.redeliverHttp,
51+
},
52+
auth: { api: { getSession: async () => opts.session } },
53+
};
54+
const ctx: any = {
55+
getService: (n: string) => services[n],
56+
logger: { debug: () => {}, info: () => {}, warn: () => {}, error: () => {} },
57+
};
58+
(new WebhookOutboxPlugin() as any).registerAdminRoutes(ctx);
59+
if (!handler) throw new Error('route was not mounted');
60+
61+
return {
62+
async post(body: any) {
63+
let status = 200;
64+
let json: any;
65+
const c = {
66+
req: { raw: { headers: new Headers() }, json: async () => body },
67+
json(payload: any, s?: number) {
68+
json = payload;
69+
if (s !== undefined) status = s;
70+
return { status, json };
71+
},
72+
};
73+
await handler!(c);
74+
return { status, json };
75+
},
76+
};
77+
}
78+
79+
/** A better-auth session envelope: `{ user, session }`. */
80+
const sessionFor = (userId: string, activeOrganizationId?: string) => ({
81+
user: { id: userId },
82+
session: { userId, ...(activeOrganizationId ? { activeOrganizationId } : {}) },
83+
});
84+
85+
describe('POST /api/v1/webhooks/redeliver — the caller\'s tenant reaches the outbox (#10740)', () => {
86+
it('threads the session\'s active organization into redeliverHttp', async () => {
87+
const seen: Array<{ id: string; tenantId: string | undefined }> = [];
88+
const route = mountRoute({
89+
session: sessionFor('user_1', 'org_a'),
90+
async redeliverHttp(id, options) {
91+
seen.push({ id, tenantId: options.tenantId });
92+
return { id, status: 'pending' };
93+
},
94+
});
95+
96+
const res = await route.post({ deliveryId: 'del_1' });
97+
98+
expect(res.status).toBe(200);
99+
expect(res.json).toEqual({ success: true, data: { id: 'del_1', status: 'pending' } });
100+
// The whole point: not `undefined`, and not some other org.
101+
expect(seen).toEqual([{ id: 'del_1', tenantId: 'org_a' }]);
102+
});
103+
104+
it('refuses a cross-tenant delivery id with RESOURCE_NOT_FOUND and 404', async () => {
105+
// The outbox scopes its reads by the tenant it is handed, so a row in
106+
// another organization is INVISIBLE rather than forbidden — which is
107+
// also what stops this endpoint being an existence oracle for other
108+
// tenants' delivery ids.
109+
const route = mountRoute({
110+
session: sessionFor('user_1', 'org_a'),
111+
async redeliverHttp() {
112+
const err: any = new Error("Delivery row 'del_other' not found");
113+
err.name = 'HttpRedeliverError';
114+
err.code = 'RESOURCE_NOT_FOUND';
115+
throw err;
116+
},
117+
});
118+
119+
const res = await route.post({ deliveryId: 'del_other' });
120+
121+
// ADR-0112: `code` AND `status`. Either alone passes on a refusal that
122+
// surfaced as a 500, or on a 404 carrying the wrong code.
123+
expect(res.json?.error?.code).toBe('RESOURCE_NOT_FOUND');
124+
expect(res.status).toBe(404);
125+
});
126+
127+
it('threads `undefined` for a session with no active organization — reported, never silenced', async () => {
128+
// The honest half. The deployment could not tell the route which
129+
// organization is asking, so the write goes out unscoped and the
130+
// driver's tenant-audit line fires for it. ⛔ The repair for that is
131+
// never `bypassTenantAudit`, which would hide the report and close
132+
// nothing — so what this pins is that the route invents no tenant.
133+
const seen: Array<string | undefined> = [];
134+
const route = mountRoute({
135+
session: sessionFor('user_1'),
136+
async redeliverHttp(id, options) {
137+
seen.push(options.tenantId);
138+
return { id, status: 'pending' };
139+
},
140+
});
141+
142+
const res = await route.post({ deliveryId: 'del_1' });
143+
144+
expect(res.status).toBe(200);
145+
expect(seen).toEqual([undefined]);
146+
});
147+
148+
it('never reaches the outbox at all for an unauthenticated caller', async () => {
149+
// The pre-existing gate, re-pinned because the session lookup was
150+
// widened from "user id" to the whole envelope: a widening that lost
151+
// the auth check would be invisible to every assertion above.
152+
let called = 0;
153+
const route = mountRoute({
154+
session: null,
155+
async redeliverHttp(id) {
156+
called += 1;
157+
return { id, status: 'pending' };
158+
},
159+
});
160+
161+
const res = await route.post({ deliveryId: 'del_1' });
162+
163+
expect(res.status).toBe(401);
164+
expect(res.json?.error?.code).toBe('UNAUTHENTICATED');
165+
expect(called).toBe(0);
166+
});
167+
});

0 commit comments

Comments
 (0)