Security: tenant webhook event_categories boundary + match-ALL update guard (v0.1.25.50)

[amavashev]

Security fix (v0.1.25.50): tenant-plane webhook event_categories were never validated, letting a tenant key subscribe to admin-only event classes.

The gap

WebhookTenantController.validateTenantEventTypes enforces the tenant-accessible boundary (budget.*/reservation.*/tenant.*) on event_types for both create and update — but event_categories was never validated on that plane, and WebhookRepository.matchesEventType treats categories as an additive union with types:

POST /v1/webhooks  (tenant API key)
{"url": "...", "event_types": ["budget.created"], "event_categories": ["api_key"]}

→ 201, and the subscription receives every api_key.* event for the tenant (likewise policy, webhook, system). The same smuggle worked via PATCH /v1/webhooks/{id}.

Spec citation: governance spec revision v0.1.25.38 (pending, prepared in parallel) adds the normative rule — tenant-plane create AND update MUST validate event_categories against the same tenant-accessible boundary, 400 INVALID_REQUEST.

The fix

• validateTenantEventCategories alongside the existing event_types check, on both create and update; same 400 + message style. The boundary is derived from a new EventCategory.isTenantAccessible() (BUDGET/RESERVATION/TENANT), and EventType.isTenantAccessible() now delegates to it — one source of truth, the two checks cannot drift.
• Admin-on-behalf-of decision: the check runs unconditionally on the tenant-plane endpoints, admin callers included — exactly how validateTenantEventTypes already behaves on the dual-auth PATCH path (create is tenant-key-only by design, spec v0.1.25.14). The boundary belongs to the endpoint, not the caller: admin categories are minted/managed on /v1/admin/webhooks/*; letting admin PATCH them onto a tenant-plane subscription would recreate the smuggle (the tenant retains the endpoint URL + signing secret).
• Empty-both edge — verified reachable, guarded: create requires @NotEmpty event_types, but WebhookService.update applied "event_types": [] verbatim; with no categories the result hit matchesEventType's empty-both branch = match-ALL (every event class, admin-only included). Update now returns 400 on both planes when a PATCH would leave both event fields empty — a match-all subscription is not creatable, so it must not be reachable by update. Category-only subscriptions remain legal.

Operator note — audit existing subscriptions

On 0.1.25.49 and earlier, tenant-created subscriptions may already carry admin-only categories. Audit tenant-plane subscriptions and strip/delete offenders, e.g.:

redis-cli --scan --pattern 'webhook:*' | xargs -L1 redis-cli GET | jq -r \
  'select(.event_categories != null)
   | select(.event_categories - ["budget","reservation","tenant"] | length > 0)
   | .subscription_id + " " + .tenant_id + " " + (.event_categories|tostring)'

Admin-provisioned subscriptions legitimately carry admin categories — check provenance via createTenantWebhook audit-log entries (tenant-plane creations) vs createWebhookSubscription (admin plane).

Tests

• Controller: smuggle on create → 400 (service never invoked); smuggle on update → 400; admin-on-behalf-of PATCH with an admin category → 400 (endpoint-bound boundary pinned); tenant-accessible categories pass on create (all three) and update.
• Service: clearing event_types with no categories → 400; clearing both in one PATCH → 400; category-only survivor → allowed.
• Pure admin-plane /v1/admin/webhooks endpoints unaffected (no new validation there; existing suite green).
• Full mvn -B verify: 1,561 tests (190 model + 561 data + 810 api), 0 failures, coverage gates met.

Version 0.1.25.49 → 0.1.25.50 (pom <revision>, per the v0.1.25.48 feature-PR precedent); CHANGELOG (Security + Fixed sections with the operator recipe) and AUDIT.md updated.

[amavashev]

Codex security round 2 — applied in bdfcfdd

Core fix confirmed sound (no bypass routes, boundary derivation correct, unknown/mixed-case enum values 400 at Jackson binding, delivery-time enforcement correctly rejected). Two REVISE-MINOR items applied + one recorded decision.

1. Operator audit recipe — corrected and completed

The original recipe was unreliable (webhook:* also matched encrypted webhook:secret:* values → jq choked on non-JSON) and incomplete (audited admin-only categories but not the legacy empty-both match-ALL rows). Verified the key layout in WebhookRepository.save — subscription = webhook:<whsub_…>, secret = webhook:secret:<id>, indexes = webhooks:* (plural) — so webhook:whsub_* cleanly selects only subscription keys. Re-verified the predicate against the stored JSON shape (event_categories is NON_NULL: absent when null, [] when empty; event_types always serialized). Corrected recipe (verbatim, copy-pasteable, flags both classes):

redis-cli --scan --pattern 'webhook:whsub_*' \
  | while read -r k; do redis-cli GET "$k"; done \
  | jq -rR 'fromjson? // empty
      | (.event_categories // []) as $cats
      | (.event_types // []) as $types
      | ($cats - ["budget","reservation","tenant"]) as $admin
      | if ($admin | length) > 0 then
          "ADMIN_CATEGORIES \(.subscription_id) \(.tenant_id) \($admin)"
        elif ($types | length) == 0 and ($cats | length) == 0 then
          "MATCH_ALL \(.subscription_id) \(.tenant_id)"
        else empty end'

ADMIN_CATEGORIES = admin-only categories; MATCH_ALL = empty-both wildcard. Logic validated against all four JSON shapes + a non-JSON line: only the two offender classes are flagged, the valid row and the garbage line are silently dropped.

2. Tests added (all green)

• Admin-plane boundary scope (WebhookAdminControllerTest): create WITH admin-only event_categories → 201; update → 200. Proves the boundary is tenant-plane-only, not a global tightening.
• Exhaustive enum coverage (EventModelTest): EventCategory.isTenantAccessible() over every enum value; every EventType delegates to its category's accessibility. Guards future drift when a type/category is added.
• Repair path (WebhookServiceTest): clearing event_categories to [] with event_types omitted → event_types survives, update proceeds (the realistic fix for a smuggled ADMIN_CATEGORIES row).

Build: mvn -B verify green — 1,566 tests (192 model + 561 data + 813 api), coverage gates met.

3. Recorded decision (no code change)

Codex's open question: a tenant-plane PATCH on a legacy/admin-created row that already holds admin-only categories validates only the provided arrays, so a tenant could retain pre-existing admin categories by PATCHing an unrelated field. This is intended and matches the spec (validate the request, not the stored state). A tenant taking over an admin-created tenant-scoped subscription is a separate provenance/ownership threat model, out of scope for this patch — I did not expand scope. If you consider the takeover risk worth tracking, I'd suggest a separate issue (e.g. "tenant-plane mutations on admin-provisioned subscriptions") rather than folding it in here; happy to file if you want it.

[amavashev]

Filed the provenance/ownership follow-up as #209 (tenant takeover of admin-created tenant-scoped subscriptions) — the residual threat model codex flagged, deliberately out of scope for this fix. This PR closes the injection path (create/update category validation) and the empty-both match-ALL door, and ships the operator audit recipe for pre-existing offenders.

[amavashev]

External review (codex) security pass: SHIP after round 2. Round 1 confirmed the core fix clean — no bypass routes (bulk endpoints only status/delete, replay/test don't mutate filters), boundary derivation exact (EventCategory.isTenantAccessible = budget/reservation/tenant, every EventType delegates), unknown/mixed-case enum values 400 at Jackson binding, delivery-time enforcement correctly rejected (would break legit admin-created rows). Round 2 fixed the operator audit recipe (webhook:whsub_* only, jq fromjson? tolerance, flags both ADMIN_CATEGORIES and MATCH_ALL rows) and added the 5 boundary/drift/repair tests. Residual provenance risk tracked as #209. 1,566 tests green, coverage gates met.