fix(relay): record the NIP-OA owner for direct members on closed relays

[rmichelena]

Fixes #4223. Also fixes the cluster reported in #4937 (rate class, owner context, backfill).

The bug

On a closed relay (require_relay_membership = true), an agent that is a direct relay member and presents a valid NIP-OA auth tag never gets users.agent_owner_pubkey recorded. The attestation is accepted for transport and then dropped.

check_relay_membership short-circuits on direct membership (api/mod.rs:77-79) and only consults the tag as a membership fallback for non-members, so enforce_relay_membership returns Ok(None) for a member. Both materialization sites then re-derived the owner behind the same conditional:

// api/bridge.rs (HTTP submit) and handlers/auth.rs (NIP-42 AUTH)
owner.or_else(|| {
    if !state.config.require_relay_membership {
        extract_nip_oa_owner(&pubkey_bytes, auth_tag)
    } else {
        None            // closed relay + direct member ⇒ owner never recorded
    }
})

The posture is inverted: the stricter deployment is the only one that never records ownership, and enrolling an agent as a member — the natural provisioning order — is what breaks it.

Why this is a correctness fix, not a policy change

Three places in the tree already say the tag should be believed unconditionally:

• config.rs:213-218, on the flag itself: "extraction for agent→owner backfill happens unconditionally (the signature is cryptographically self-proving). This flag only controls whether NIP-OA can grant membership access on closed relays." That describes the behavior this PR implements, as though it were already true.
• extract_nip_oa_owner's own doc comment: "cryptographically self-proving, so no feature flag is needed."
• The ban cascade in the very same handler (handlers/auth.rs:137) and its git counterpart (api/git/transport.rs:257) extract the owner with no relay-openness condition — because a ban on a human must reach their agents. The same tag, in the same function, was trusted for denying access and discarded for recording ownership.

allow_nip_oa_auth is untouched: it still governs whether NIP-OA can grant membership, which is the only thing its doc comment claims.

Change

One shared, pure helper — relay_members::resolve_nip_oa_owner(gate_owner, pubkey, auth_tag) — keeps the delegated owner when membership came through one, and otherwise verifies the presented tag. Both materialization sites call it.

Doing it in one place rather than fixing each or_else is deliberate: the two call sites were identical copies, which is how they drifted from the intent in the first place, and a pure function is unit-testable without Postgres or Redis (the existing tests in this module cover only extract_nip_oa_owner).

I also corrected enforce_relay_membership's doc comment, since Ok(None) reading as "no owner" rather than "admitted on its own" is what the two call sites got wrong.

No behavior change on open relays (the or_else there already extracted), and none for callers that don't record ownership — media.rs, audio/handler.rs and the git transport keep using the membership gate exactly as before.

What this restores on closed relays

Consequence of a NULL owner	Where
channel_add_policy: owner_only unenforceable — no owner to match	policy check
Observer frames (kind 24200) refused: restricted: observer frame is not authorized for this agent owner	handlers/event.rs
Agent rate-limited at human_messages_per_min instead of agent_standard_messages_per_min	connection.rs:632, :659-661

The last two are the reason this needs the auth.rs half: both read agent_owner_pubkey from the session auth context, with no DB fallback, so a DB-level repair doesn't help a live connection and reconnecting re-runs the same gate.

Considered consequence

An agent admitted as a direct member can now enter the agent rate class by presenting a self-minted attestation (any keypair can attest any other). That authority is not new — the open-relay path and the ViaOwner path already accept exactly the same self-proving tag — and on a closed relay the actor must already be an admitted member. Flagging it explicitly rather than leaving it implicit: if maintainers want the agent rate class to require something stronger than a valid NIP-OA tag, that's a separate discussion about the rate class, not about which membership branch was taken.

Existing rows stay NULL until the agent next authenticates or submits; this is a fix-forward, not a migration.

Relation to #4260

#4260 (@iroiro147, Aug 2) fixes the same conditional in bridge.rs — the HTTP path — with the same reasoning. It's a correct diagnosis and it predates this PR. What it doesn't cover is handlers/auth.rs, which is where the WebSocket session context is built, so the rate class and observer-frame consequences survive it.

Happy to go either way: reduce this to the auth.rs half plus tests on top of #4260, or land this and close that one — whichever maintainers prefer. I'd rather not have two open PRs on one conditional.

Tests

Four new unit tests next to the existing extract_nip_oa_owner ones (cargo test -p buzz-relay --lib, no infrastructure needed):

• delegated owner from the gate wins over the presented tag;
• the regression: a caller the gate admitted on its own still has its verified owner resolved;
• a member with no tag stays ownerless — membership alone never invents an owner;
• a tag minted for a different agent is rejected, so an intercepted attestation can't be replayed onto another pubkey.

Also run: cargo fmt --check, cargo clippy -p buzz-relay --all-targets -- -D warnings.

Local run on this branch: cargo fmt --check clean, cargo clippy -p buzz-relay --all-targets -- -D warnings clean, cargo test -p buzz-relay --lib → 863 passed / 8 failed, where all 8 are the api::admin / api::media tests that need Postgres (Sqlx(PoolTimedOut)) and none touch this path — no Docker on this machine, so CI is the real check there.

[ravarora2]

🤖 Review verdict: request changes

The core fix is useful and should remain: a directly enrolled agent must still have its valid owner relationship recorded. Both the HTTP and WebSocket paths need this repair. Before merge, please address the following.

Blockers

1. Enforce the signed credential time bounds before trusting or persisting the owner.

   buzz_sdk::nip_oa::verify_auth_tag currently validates the tag structure and signature, but does not evaluate created_at<... or created_at>.... The relay must evaluate those clauses against the timestamp of the signed NIP-42 or NIP-98 authentication event before it:

   • writes users.agent_owner_pubkey;
   • sets AuthContext.agent_owner_pubkey;
   • grants owner-derived authorization;
   • selects the higher agent message quota.

   Please test a valid window, an expired credential, a not-yet-valid credential, and exact equality at both strict boundaries.

2. Preserve the closed-relay owner trust boundary.

   A direct member can currently present a tag signed by a fresh second key that is not a relay member. The PR then creates that key as the owner and places it in the trusted WebSocket context, even with BUZZ_ALLOW_NIP_OA_AUTH=false. This differs from the existing ViaOwner path, which requires an active member owner, and it can change the default quota from the human 60/min tier to the agent 120/min tier.

   On a closed relay, require the claimed owner to satisfy the relay trust policy before using it for owner-only actions, live-session authorization, or rate classification. If unconditional cryptographic relationship backfill is intentional, separate relationship metadata from trusted authorization so an untrusted/non-member owner does not gain those effects.

3. Add regression tests through both production paths.

   The four new tests call only resolve_nip_oa_owner. Restoring both old buggy call-site conditionals leaves every added test green, so the tests do not protect the actual regression.

   Please add tests that execute:

   • closed relay + direct member + valid tag through HTTP /events -> expected owner stored;
   • closed relay + direct member + valid tag through NIP-42 -> expected owner stored and live auth context set;
   • missing or invalid tag -> owner remains unset;
   • expired or not-yet-valid tag -> no materialization and no agent session classification;
   • non-member-owner and configuration combinations -> behavior matches the documented closed-relay policy.

   Acceptance criterion: reverting either HTTP or WebSocket production fix must fail its corresponding test.

Non-blockers

• Update crates/buzz-auth/src/lib.rs:75-79, which still says direct members have no owner context.
• Update crates/buzz-relay/src/api/mod.rs:149-155, which still describes owner extraction as open-relay-only.
• Document that existing NULL owner rows are repaired only when the agent next authenticates or submits an event; this is fix-forward, not a migration.

Verification evidence

• The intended HTTP behavior was reproduced locally: a legitimate direct-member tag was accepted and the expected owner was stored.
• A live closed-relay NIP-42 probe accepted created_at<1 from a non-member claimed owner with BUZZ_ALLOW_NIP_OA_AUTH=false, then persisted that owner. This confirms blockers 1 and 2 through the real WebSocket path.
• Full cargo test -p buzz-relay: 869 passed, 2 failed, 40 ignored. Independent parent/base runs reproduced the failures in unchanged tests, so no package-test regression was attributable to this PR.
• cargo fmt --check and cargo clippy -p buzz-relay --all-targets -- -D warnings passed in independent review.

Review scope: commit 9bb913f25cf37b4aede9525ac90d3af31b0c8a4e. This is the unique ID of the exact PR code snapshot that was reviewed and tested. If new commits are pushed, the PR gets a new ID and the changed code must be re-reviewed.

[rmichelena]

Accepting all three blockers. I verified each against 9bb913f2 rather than reasoning about them, and two of the checks turned up something that changes what the fix has to look like.

Blocker 3 — confirmed empirically, and it's worse than stated

I reverted both production call sites (bridge.rs and auth.rs), keeping the helper and the four tests:

cargo test -p buzz-relay relay_members
7 passed; 0 failed

Green with the bug restored. Your acceptance criterion is the right one.

But the tests wouldn't have caught it even if they had been written against the call sites, because no CI job executes api::relay_members::tests. I checked every path:

• just test-unit enumerates packages explicitly and buzz-relay is not among them.
• scripts/run-tests.sh integration runs cargo test --test '*', which only matches integration-test targets; crates/buzz-relay has no tests/ directory.
• ci.yml selects buzz-relay tests only through two explicit nextest filters — test(/api::invites::tests/) and test(/handlers::relay_admin::tests/).

api::relay_members::tests matches none of them, so those four tests and the four that predate this PR have never run in CI. The justfile already warns about this class of gap in a comment: "nothing in CI runs cargo test --workspace — workspace membership alone buys clippy/check, not a single executed test."

So the fix is to follow the handlers::relay_admin::tests precedent — #[ignore]d Postgres-backed tests exercising the real HTTP and NIP-42 paths, plus a CI step selecting them explicitly. Adding tests to the existing module without the CI wiring would satisfy the letter of the criterion and still never run.

Blocker 1 — correct, and the evaluator already exists in-tree

validate_conditions is purely syntactic: validate_clause checks that created_at<N is a canonical decimal in range and never compares N to anything. Probed directly:

created_at<1             (expired since 1970)     -> accepted, owner materialized
created_at>4294967294    (not valid until 2106)   -> accepted, owner materialized

Two things worth putting on the record.

First, this is not new in this PR. check_relay_membership calls the same verify_auth_tag for the ViaOwner branch, which I don't touch — so on main today an expired credential can already grant relay membership on a closed relay, which is a strictly stronger effect than recording an owner. This PR widens the reach of an existing gap rather than creating one. That's not a defence of shipping it as-is; it's an argument that the fix belongs where both paths pick it up.

Second, the semantics you asked me to pin are already pinned in-tree. enforce_request_auth_time_bounds in handlers/identity_archive.rs:328 evaluates exactly these clauses, and its tests fix both boundaries as strict:

assert!(enforce_request_auth_time_bounds(&auth, 150).is_ok());
assert!(enforce_request_auth_time_bounds(&auth, 100).is_err());  // created_at>100
assert!(enforce_request_auth_time_bounds(&auth, 200).is_err());  // created_at<200

So the archive handler already enforces time bounds on a NIP-OA tag while the ownership path does not. I'd rather hoist that function into shared code and apply it in both places than write a second evaluator that could drift from it — tell me if you'd prefer it scoped differently.

Blocker 2 — correct, and it's this PR's doing

I traced the consequences and they're a little wider than described:

• materialize_nip_oa_owner calls ensure_user for the owner, so an arbitrary non-member key gets a user row created on a closed relay.
• connection.rs:632 derives is_agent from agent_owner_pubkey.is_some() alone, and :658 switches the limit from human_messages_per_min (60) to agent_standard_messages_per_min (120). Any direct member can double its own quota with a throwaway keypair.
• set_agent_owner is first-write-wins. A single authentication with a wrong or stale tag permanently pins that mapping — the legitimate owner is refused afterwards and materialized comes back false. That's durable corruption of the ownership record, not just a transient privilege bump.

The framing in my commit message cited the agent rate class as a benefit of the fix. It's also the abuse vector, and I didn't see that.

I'll take your first option — require the claimed owner to satisfy the relay trust policy on closed relays, mirroring what ViaOwner already demands — rather than the metadata/authorization split, which needs a schema change to represent an untrusted owner. Worth noting for anyone tracking #4223: the stricter version still fixes the original bug, because the deployments that hit it enrol the agent as a member while the owner is a member too.

Note allow_nip_oa_auth stays out of this path deliberately, since its own doc comment scopes it to granting membership; the trust boundary I'm adding is owner-membership. Say the word if you want the flag consulted as well.

Non-blockers all confirmed, including buzz-auth/src/lib.rs:75, whose "None for direct relay members" is precisely what this PR stops being true.

Re-rolling with these. Thanks for the depth here — reproducing the failures on the parent before attributing them to the PR is more care than a review usually gets.

[rmichelena]

Pushed 3a3806ea with blockers 1 and 2 addressed. Blocker 3 is half done and I want to be precise about which half.

Blocker 2 — owner trust boundary

The claimed owner must now be a relay member on closed relays, mirroring what ViaOwner already demands. Open relays are unchanged, since there is no membership boundary there to check against.

allow_nip_oa_auth deliberately stays out of the path — its own doc comment scopes it to whether NIP-OA may grant membership, which this never does. Requiring it would leave #4223 unfixed on deployments that run with it off while enrolling agents directly. Say the word if you want it consulted anyway.

Worth recording that the blast radius was slightly wider than either of us wrote: set_agent_owner is first-write-wins, so a single authentication with a bogus tag pinned that mapping permanently and the legitimate owner was refused afterwards. Not just a transient quota bump — durable corruption of the ownership record.

Blocker 1 — time bounds

evaluate_time_bounds + verify_auth_tag_at in buzz-sdk, evaluated against the created_at of the signed authentication event, per your framing. enforce_request_auth_time_bounds now delegates to the shared evaluator instead of keeping its own copy, so the NIP-IA handler and the ownership path cannot drift. Its existing test still passes unchanged, which is what pins the strict-at-both-edges semantics.

Three deliberate choices:

Bounds are enforced even when the gate already resolved an owner. check_relay_membership does not evaluate them, so an expired tag can still produce a ViaOwner decision. I left that pre-existing membership grant alone — it is a wider change across eight call sites and deserves its own review — but it can no longer be materialized into an ownership record. If you'd rather I close the membership half here too, I'll do it; it's the same evaluator, just plumbed through enforce_relay_membership.

Time bounds gate granting, never denying. extract_nip_oa_owner keeps its signature-only behaviour for the ban cascades in handlers::auth and api::git::transport. An expired attestation must not become an escape hatch from an owner ban — widening who gets denied is safe, widening who gets trusted is not. The two entry points are named and documented for that split.

No NIP-98 event means no ownership. verify_bridge_auth now reports the request event's created_at; it's None under X-Pubkey dev auth, where nothing was signed. That case records no owner rather than treating the tag as unbounded.

Blocker 3 — the pure tests are gone, the real ones are owed

The four tests you flagged are replaced with coverage of what is actually pure: time bounds at both strict edges, the signature binding, and the grant/deny asymmetry above. I'm not claiming those close the gap — they don't, and I'd rather say so than dress them up.

The Postgres-backed tests through HTTP /events and NIP-42 are still outstanding, along with the CI filter that selects them. As noted above, api::relay_members::tests is run by no job today, so the tests and the wiring have to land together or the acceptance criterion is satisfied only on paper. I don't have Postgres available where I'm working and I'm not going to write DB tests I can't execute; I'm sorting that out and they'll come as a follow-up commit on this branch.

Verification

cargo clippy -p buzz-relay -p buzz-sdk -p buzz-auth --all-targets -- -D warnings   clean
cargo fmt --all --check                                                            clean
cargo test -p buzz-sdk --lib      261 passed; 0 failed
cargo test -p buzz-auth --lib      45 passed; 0 failed
cargo test -p buzz-relay --lib    863 passed; 8 failed

Those 8 are infra-dependent (api::media, api::admin) and fail identically on 9bb913f2 with the changes stashed — same eight names, same count — so they are my missing Postgres, not a regression. Your run saw 869/2 with a database present. One further caution from doing that comparison: api::invites::tests::claim_limiter_expires_entries failed in one run out of three and passed in the others, so it looks timing-sensitive and flaky rather than related to anything here.

Non-blockers are all corrected, including AuthContext::agent_owner_pubkey, whose doc now also warns that the field selects the agent rate class — that coupling is what made blocker 2 reachable, and it deserved to be written down next to the field rather than a hundred lines away in connection.rs.

[rmichelena]

Blocker 3 is closed. 0bf52ba2 adds seven Postgres-backed tests through both production paths, plus the CI step that selects them.

Acceptance criterion, verified by doing it

I reverted each production fix in turn and confirmed the corresponding test fails:

reverted	fails
bridge.rs (HTTP)	nip_oa_owner_http_records_owner_for_direct_member, ..._refuses_an_expired_attestation
handlers/auth.rs (WebSocket)	nip_oa_owner_ws_records_owner_and_sets_auth_context

That doubles as proof the tests actually execute rather than skipping on a missing database — a skipped test cannot fail.

Worth stating plainly: only the positive-recording cases discriminate. With the old code nothing is ever materialized on a closed relay, so the refusal cases hold vacuously against it. They guard the new trust boundary, not the original regression, and I'd rather say which test earns which claim than present seven green checks as if they were interchangeable.

What they cover

Entry is at the production call sites — submit_event_authed (the authenticated core of POST /events; everything outside it is NIP-98 verification and the attribution log) and handle_auth itself.

• direct member + valid tag → owner recorded
• non-member owner → refused
• expired attestation → refused, and the same tag accepted one second inside its window, so the refusal is attributable to the time bound rather than to some unrelated rejection
• no tag → nothing recorded
• WebSocket: owner reaches the live AuthContext, which is what observer-frame authorization and the agent rate class read

One incidental finding: NIP-42 rejects a stale AUTH event, so the WebSocket tests stamp at the real clock and express bounds relative to it. That is also how a live deployment presents an expiring credential, so it is the more faithful shape anyway.

The CI step is not optional here

Without it these would satisfy the criterion on paper and never run. As noted earlier: just test-unit does not list buzz-relay, run-tests.sh integration only picks up tests/ targets and this crate has none, and ci.yml selected exactly two buzz-relay modules by name. The new step selects by test name (test(/nip_oa_owner_/)) rather than by module, because the tests span api::bridge and handlers::auth, and it sets REDIS_URL alongside DATABASE_URL since the submit path takes the NIP-98 replay guard.

If you'd prefer this crate's whole unit set to run in CI rather than a third named filter, that's a bigger change than this PR should carry, but it is the actual fix and I'm happy to open it separately.

Verification

cargo nextest/test -p buzz-relay --lib nip_oa_owner -- --ignored   8 passed; 0 failed
cargo test -p buzz-relay --lib                                   866 passed; 5 failed; 47 ignored
cargo test -p buzz-sdk --lib                                     261 passed; 0 failed
cargo test -p buzz-auth --lib                                     45 passed; 0 failed
cargo clippy -p buzz-relay -p buzz-sdk -p buzz-auth --all-targets -- -D warnings   clean
cargo fmt --all --check                                                            clean

The 5 are api::mesh_demo and tunnel::*. I ran the identical command at 9bb913f2 against the same database and got the same five names and the same count, so they are pre-existing and unrelated. Notably they fail only with a database present and pass without one, which given they do Redis-keyed fence/ownership acquisition looks like shared-Redis interference between tests rather than anything about this change — flagging it in case it is news to you, but I have not investigated further since it is outside this PR.

Also still true from my earlier comment: api::invites::tests::claim_limiter_expires_entries is timing-flaky, failing roughly one run in three independently of these changes.