fix: stop trusting pg_notify payloads — notifications are hints, not transport

[nicolasburtey]

Summary

PostgreSQL performs no authorization on LISTEN/NOTIFY channels — any role able to connect to the database can signal (and listen to) any channel. obix treated notification bodies as trusted transport, which this PR fixes. Notifications are now strictly hints; event data is always read from the tables with the consumer's own credentials.

⚠️ Breaking change — the setup migration 20251204130225_obix_setup.sql is modified in place (the ephemeral trigger now sends only {event_type, recorded_at} instead of row_to_json(NEW)). Existing deployments must recreate the schema; there is no in-place upgrade migration. Marked fix! per conventional commits.

🔴 Ephemeral event forgery & eavesdropping

• Before: the trigger sent the full payload over pg_notify, and listeners deserialized and broadcast it without touching the table. Any DB role (or SQL injection in any application sharing the database) could inject forged ephemeral events into all consumers, and could LISTEN to harvest every ephemeral payload with no table grant.
• After: the trigger sends only {event_type, recorded_at}. Listeners always fetch the payload from the table; recorded_at lets them skip the fetch when the in-process cache is already current. A burst of N notifications for the same type collapses into a single fetch (deduped in the drain loop).

🟠 Forged persistent notifications — phantom head + unbounded range fetch

• Before: a forged {min_sequence, max_sequence} with a huge max_sequence was applied directly to highest_known_sequence (pinning the gap-fill loop against a phantom head — a fill query every second, forever) and drove an unbounded fetch_notified_range scanning the entire table tail on every forgery.
• After: both the head advance and the fetch up_to are clamped to the sequence's authoritative last_value before being applied. last_value advances at nextval (pre-commit), so a forged claim inside (committed_head, last_value] can still trigger a bounded grace-period gap-fill that self-heals via ON CONFLICT — it never stalls.

🟡 Minor hardening

• tbl_prefix validation — restricted to 1–25 chars of [A-Za-z0-9_] (no leading digit) at derive time; it is interpolated into generated SQL identifiers and the pg_notify channel literal.
• No panics on DB-derived data — unparseable inbox status → InboxError::InvalidStatus; undeserializable ephemeral event_type → row dropped with an error span.

Test plan

• forged_ephemeral_notification_is_not_delivered — fails on pre-fix code (forged event was broadcast verbatim), passes with the fix
• ephemeral_event_written_externally_is_fetched_from_db — cross-instance delivery via hint + DB fetch
• forged_persistent_notification_does_not_stall_listener — now runs against a populated table (5 events), so the range-fetch amplification is exercised; forged i64::MAX head neither synthesizes phantom events beyond the real head nor stalls delivery
• Full suite green: cargo test --all-features (63 tests), cargo clippy --all-targets clean, cargo fmt --check clean

[bodymindarts]

Review: stop trusting pg_notify payloads — ✅ Approve

Strong, correctly-scoped trust-boundary fix. The core thesis — NOTIFY is a wake-up hint, never transport; always re-read from the table with the consumer's own credentials — is applied consistently on both the ephemeral (payload) and persistent (head) paths, the regression tests are meaningful (the ephemeral one genuinely fails pre-fix), and CI is green. Shippable as-is. The items below are defense-in-depth / operational notes, not blockers — one is worth a follow-up because it's the same class of bug this PR fixes, left unclamped on a sibling path.

---

🔒 Security — what this gets right

• Ephemeral forgery + eavesdropping closed. migrations/20260803120000_obix_ephemeral_notify_hint.sql slims the trigger to {event_type, recorded_at}, and handle_ephemeral_notification (src/out/ephemeral/cache.rs:142-172) never reads the body — it always fetch_event_by_type from the table, using recorded_at only to skip a redundant fetch. Injection and payload-harvest are both gone.
• Persistent phantom-head neutralized. src/out/persistent/cache.rs:505-538 clamps the claimed head to the sequence's authoritative last_value before advancing highest_known_sequence. A forged i64::MAX can no longer pin the gap-fill loop against a phantom head.
• Derive-attribute identifier injection closed. obix-macros/src/tables.rs:33-55 — tbl_prefix restricted to [A-Za-z0-9_], 1–25 chars, no leading digit. The ≤25 bound is exactly right: longest derived name {prefix}_persistent_outbox_events_sequence_seq = prefix + 38 ≤ 63 (PG's identifier limit).
• Panic removal on DB-derived data. InboxError::InvalidStatus (verified FromStr::Err = String, so no info lost) and record_ephemeral_event_type_undecodable replace .expect()/.unwrap() on foreign/drifted rows.

---

🟠 Concern (non-blocking) — the forged-range fetch is still unclamped

src/out/persistent/cache.rs:480-503 + handle_notification (:319-348) + fetch_notified_range (:303-314)

The claimed head is clamped, but the range fetch the same forged {min,max} drives is not. A forged {min: 1, max: i64::MAX} still:

1. forces handle_notification to compute missing by filtering min..=max against the cache — O(cache_size) synchronous hashmap lookups on the single-threaded cache loop (bounded by the cache high-water mark, but per-forgery and on the critical path); and
2. spawns fetch_notified_range(after, i64::MAX) → load_events_in_range = WHERE sequence > after AND sequence <= i64::MAX → an index range scan to the end of the table, streaming every tail row through cache_fill (Arc allocs + broadcast) on every forged NOTIFY.

No data leak (the consumer already has table-read creds), but it's a cheap-for-attacker / expensive-for-victim amplification — and it's the same "don't let a forged claim drive unbounded work" bug this PR fixes on the head path, left in place on the fetch path. The new forged_persistent_notification_does_not_stall_listener test runs against an empty table, so this cost is unexercised.

Suggestion: do the authoritative highest_known_persistent_sequence read before spawning the fetch, and clamp the fetch up_to (and the missing-range span) to that confirmed head. Free for legit notifications (up_to == real head), protective for forged. The head read already happens for the advance path — this just reorders it.

❓ Nuance worth stating — clamp target is last_value (allocated), not committed head

src/out/persistent/cache.rs:523-534 — highest_known_persistent_sequence reads the sequence's last_value, which advances at nextval (pre-commit) and includes sequences held by in-flight/rolled-back txns. So a forged claim inside the window (committed_head, last_value] passes the min(claimed, last_value) clamp and advances the head prematurely, triggering the grace-period gap-fill against in-flight sequences. It's bounded by last_value and self-heals via the ON CONFLICT speculative-insert block (so it never stalls — the test holds), but it's worth being explicit in the comment that the clamp bounds forged-fill work rather than eliminating it.

🟡 Operational — mixed-version rollout window (ephemeral only)

Once the migration replaces the trigger with the hint-only variant, any still-running old-code instance parses {event_type, recorded_at} with payload_omitted defaulting to false, falls into the else branch, tries to deserialize a full EphemeralOutboxEvent (which now has no payload) → None → and never falls back to a DB fetch. So not-yet-upgraded instances silently stop delivering ephemeral events until they roll. (New-code-against-old-trigger is fine: the old row_to_json still contains event_type + recorded_at; extra fields are ignored.) Because sqlx migrates on boot, the first upgraded instance flips the trigger for everyone. Impact is limited to ephemeral (transient, last-write-wins, heals on upgrade/resync) and the persistent path is unaffected — but the upgrade note should call out this transient ephemeral-delivery gap during a rolling deploy.

🔵 Nits

• README wording (README.md:15): "a forged or snooped pg_notify can neither inject nor leak events" slightly overstates it — payloads are protected, but the hint still exposes event metadata to any connectable role: ephemeral event_type names + recorded_at timing, and persistent {min,max} sequence numbers (i.e. event volume/rate). Fine for the threat model; consider "neither inject events nor leak payloads."
• Duplicate ephemeral broadcasts under burst (src/out/ephemeral/cache.rs:101-118): the LWW guard uses strict >, and a burst of N notifications for the same new type is drained against one cache snapshot (fetches are async, cache unchanged during the drain), so all N spawn identical fetches → N broadcasts of the same latest event (equal recorded_at not deduped). LWW consumers should be idempotent, so low impact and not a regression — could dedupe by not re-broadcasting an equal recorded_at.

---

Test coverage

Good: forged ephemeral (not delivered + listener stays live), external-write hint→fetch path, forged persistent (no phantom head, no stall). The ephemeral forgery test is a real regression (fails pre-fix).

Gaps worth a follow-up: (1) no burst/coalescing test (many NOTIFYs → bounded fetches / single head read); (2) the forged-persistent test uses an empty table, so the range-fetch amplification above is untested on a populated table; (3) no test that the last_value-window forged claim self-heals rather than stalls.

Nice work — the reasoning in the code comments is unusually clear and made this easy to audit.

[nicolasburtey]

Thanks for the thorough review — all three substantive points addressed in the latest push (652c601, rebased onto main incl. #106).

🟠 Unclamped range fetch — done. The authoritative highest_known_persistent_sequence read now happens before the fetch spawn, and the fetch up_to is clamped to that confirmed head (same min(claimed, last_value) as the head advance). A forged {min:1, max:i64::MAX} now resolves to fetch_notified_range(0, real_head) instead of scanning the table tail. On a transient head-read failure the fetch is skipped (not fired unclamped); a subsequent notification/resync retries. The forged_persistent_notification_does_not_stall_listener test now runs against a populated table (5 events) so the amplification path is actually exercised.

❓ last_value nuance — called out explicitly in the clamp comment: last_value advances at nextval (pre-commit), so a forged claim inside (committed_head, last_value] passes the clamp and can trigger a bounded grace-period gap-fill that self-heals via ON CONFLICT. The comment now states the clamp bounds forged-fill work rather than eliminating it.

🔵 Burst dedup nit — done. handle_ephemeral_notification now returns Option<EphemeralEventType> instead of spawning inline; the drain loop collects unique types into a HashSet before spawning, so N notifications for the same type collapse into one fetch.

🔵 README nit — reworded to "neither inject events nor leak payloads."

Migration — folded into the existing 20251204130225_obix_setup.sql (no separate migration file); marked fix! since the checksum change is breaking for existing deployments.

The mixed-version ephemeral-delivery gap during a rolling deploy is a real operational note — left as-is since it's inherent to changing the trigger's payload format and limited to the transient ephemeral stream (heals on upgrade/resync).