Skip to content

feat(server): inject caller's Nostr pubkey into request extensions#99

Merged
ContextVM-org merged 4 commits into
mainfrom
feat/clientPubkey
Jul 10, 2026
Merged

feat(server): inject caller's Nostr pubkey into request extensions#99
ContextVM-org merged 4 commits into
mainfrom
feat/clientPubkey

Conversation

@ContextVM-org

Copy link
Copy Markdown
Collaborator

Add ClientPubkey type and inject the caller's Nostr public key (hex) into every inbound request's extensions typemap. This allows tool/resource/prompt handlers to identify the caller via ctx.extensions.get::<ClientPubkey>(). The feature is always on, unlike the TS adapter's opt-in _meta field, and uses rmcp's typed extensions (local-only). The inbound event id remains accessible as ctx.id.

Add `ClientPubkey` type and inject the caller's Nostr public key (hex) into
every inbound request's `extensions` typemap. This allows tool/resource/prompt
handlers to identify the caller via `ctx.extensions.get::<ClientPubkey>()`.
The feature is always on, unlike the TS adapter's opt-in `_meta` field, and
uses rmcp's typed extensions (local-only). The inbound event id remains
accessible as `ctx.id`.
@ContextVM-org ContextVM-org requested a review from harsh04044 July 8, 2026 15:51
Expose the complete verified Nostr event via `InboundEvent` in
`extensions` so handlers can access `sig`, `id`, `pubkey`, `tags`, etc.
This eliminates the need for handlers to fabricate synthetic events
when binding tool calls to the original publishing event (e.g., an MLS
key-package coordinator returning the publication event). Also add an
`event` field to `IncomingRequest` for the channel seam.

FFI types (`FfiIncomingRequest`, the UniFFI `IncomingRequest`)
intentionally do not mirror the new field yet because no FFI consumer
requires the raw Nostr event, and exposing `nostr_sdk::Event` across
the C ABI + UniFFI is non-trivial. Omission is documented in place.
…EP-41)

Implement sender-side keepalive for OpenStreamWriter to prevent resource
leaks when a client silently disconnects (crash, sleep, network drop) without
sending an abort frame. Previously only reader sessions ran keepalive timers,
so pure producer streams (e.g. subscription tools) were never probed, leaving
writers and downstream producers alive indefinitely.

- Add idle_timeout / probe_timeout to OpenStreamWriterOptions to enable
  keepalive (disabled by default, e.g. for tests).
- Arm an idle window when the writer starts streaming; the server keepalive
  sweep now probes writers via the new `tick` method.
- Route inbound pong frames for the stream to the writer to clear probes.
- On probe timeout, abort with "Probe timeout", flush a terminal response
  via the existing on_abort hook, and evict the dead client’s session
  (mirroring the TS SDK 0.13.8 fix, per CEP-41’s “release local state”).
- Reuse the reader idle/probe timeout knobs (one idle/probe pair per stream).
- Per CEP-41, only inbound frames reset the idle window; relay writes do not.

Closes the silent-disconnect leak and aligns with CEP-41 requirements.

@harsh04044 harsh04044 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Went through this against ts-sdk 0.13.8 piece by piece. Solid change, docs and tests are thorough. One thing I'd sort before merge, then a few smaller notes.

The parity story holds up: TS's injectClientPubkey is opt-in (default off, nostr-server-transport.ts:229) and writes to the on-wire _meta (utils.ts:47), so doing it always-on through local rmcp extensions instead is a reasonable call.

The plaintext path doesn't verify the event signature, but ts-sdk does. This is half of ts-sdk's identity-forgery fix (ContextVM/sdk#64, PR ContextVM/sdk#69). That fix verified both inbound paths: the decrypted inner event, and the unencrypted one (a separate commit, fix: verify unencrypted event signatures). We port the first at server/mod.rs:1698 (inner.verify()), but the plaintext else arm (server/mod.rs:1741-1759) trusts event.pubkey without ever calling verify(). ts-sdk verifies it in handleUnencryptedEvent (event-pipeline.ts:162), and the reasoning is on the encrypted path just above (event-pipeline.ts:113-116): an attacker can put any pubkey in the plaintext and bypass allowlists. We gate the auth allowlist on exactly that pubkey (server/mod.rs:1789), so it lands here too.

To be clear it isn't exploitable on the default transport: nostr-sdk's RelayPool already verifies inbound signatures (nostr-relay-pool-0.44.1, relay/inner.rs:1233), which is the whole reason ts-sdk has to do it in-app, since nostr-tools doesn't. It only bites with a custom RelayPoolTrait that skips verification, like our own MockRelayPool. RelayPoolTrait is public, so as written, caller identity quietly depends on which pool someone plugs in. The gap predates this PR, but this is the change that makes it matter, since it promotes event.pubkey into first-class handler identity. I'd just mirror the gift-wrap arm:

// verify() also checks event.id matches its content, which we rely on for
// correlation and the request id below. Redundant against the default pool,
// but it keeps identity independent of the pool impl.
if let Err(e) = event.verify() {
    tracing::warn!(target: LOG_TARGET, "Plaintext event signature verification failed: {e}");
    continue;
}

Not a hard blocker given the default pool, but it's cheap insurance, and I'd rather caller identity not hinge on an undocumented assumption about the transport.

A few smaller things:

Scope / description. Three separate changes are riding together: ClientPubkey (the parity piece), InboundEvent, and the CEP-41 open-stream fix. The description only really covers the first, so it might be worth mentioning all three, since the CEP-41 fix isn't described at all and a reviewer could miss it.

InboundEvent. No issue with it going beyond the reference. TS only surfaces the event lazily via getNostrRequestEvent (nostr-server-transport.ts:627), and injecting it directly is nicer. One real gap though: it's silently None for oversized (CEP-22) requests. The reassembly path sends a real sender_pubkey with event: None (server/mod.rs:2211-2214), so a handler can't tell "no event" from "the request was too big to fit in one." That's exactly the MLS key-package case the description pitches, since a key package is the kind of payload that goes oversized. At least worth making the docs honest about it.

ClientPubkey on synthetic requests. It's injected unconditionally, so the internal announcement/init requests get a ClientPubkey that's really the ANNOUNCEMENT_REQUEST_ID sentinel (InboundEvent is correctly None there). ts-sdk only ever injects a real event.pubkey. Might be worth skipping it for those, but gate on the sentinel value, not on event.is_none(), since the oversized path above carries a real pubkey with no event.

Couple of things I checked that are right as-is: evicting the whole session on a writer probe timeout matches ts-sdk's handleProbeTimeout (open-stream-factory.ts:307), and ClientPubkey(String) as raw hex is correct, since a typed key would drift from _meta.clientPubkey: string.

Really nice work overall. The encrypted-path verification, per-request extension isolation, the whoami/whoami_event coverage across all three encryption modes, the keepalive tests plus the silent-client regression, and documenting the FFI omission instead of leaving it implicit all stood out.

…nit drives

The transport's own announcement and initialization drives use the
`ANNOUNCEMENT_REQUEST_ID` sentinel as their pubkey. Previously, this
sentinel was unconditionally injected into request extensions, handing
handlers a bogus caller identity. Now guard the injection so that only
real client pubkeys (including those from oversized CEP-22 requests)
are exposed via `ClientPubkey`.
@ContextVM-org ContextVM-org requested a review from harsh04044 July 10, 2026 10:51

@harsh04044 harsh04044 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, all items addressed! Just worth updating the PR description to cover InboundEvent and the CEP-41 fix, since right now it only mentions the pubkey injection.

@ContextVM-org ContextVM-org merged commit 808fe40 into main Jul 10, 2026
9 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants