fix(security): connection authz hardening — owner-scoped keys, default-deny permissions, signer binding, session revocation#18
Merged
Conversation
… permissions
C1 (critical IDOR): createConnection now verifies the nsecKeyId belongs to the
authenticated user (findFirst scoped by { id, userId }) before binding a
connection or starting a relay listener, closing a cross-tenant private-key
takeover where a user could sign/decrypt with another user's key.
H2 (fail-open permissions): the NIP-46 RPC handler is now default-deny for every
capability method (sign_event, nip04/nip44 encrypt/decrypt). An empty permission
set no longer grants everything. New connections are seeded with a conservative
default set (sign_event kinds 0/1/3/4/7, no decrypt oracle); explicit perms from
the connect request or dashboard still override the defaults.
M5: adds bunker-rpc.handler.spec.ts (previously untested) covering unknown/revoked
clients, default-deny, per-kind permission enforcement, unguarded ping/get_public_key,
the connect handshake, and audit logging; extends connections.service.spec.ts with
the C1 ownership check and default-permission seeding.
Note: existing perm-less connections will be denied under default-deny until perms
are granted (dashboard or connect); no data migration is included.
The RPC handler resolved the connection by clientPubkey alone, ignoring the signer pubkey the request was addressed to (the relay #p tag / listener key). A client with connections to several of the user's keys could have a request meant for key A served by a connection bound to key B - signing/decrypting with the wrong key under a possibly broader permission set (key confusion). Replace findByClientPubkey with findByClientAndSigner(clientPubkey, signerPubkey), additionally filtering on nsecKey.publicKey === signerPubkey, and pass the signer through in both the main dispatch and the auto-connect path. Adds a handler test.
updatePassword wrote a new hash but never deleted Session rows, so a leaked/stale refresh token kept working for the full refresh lifetime (~7d) after the canonical post-compromise remediation. It now deletes all of the user's sessions, keeping the caller's current session (sessionId from the JWT) so they stay signed in here. Wires the previously-unused revoke-all-other-sessions intent into the flow. Adds tests for keep-current, revoke-all, and no-revoke-on-wrong-password.
Companion to the default-deny permission model: existing ACTIVE/PENDING connections with no permission rows were operating under the old fail-open (allow-all) behaviour and would be denied outright by the new handler. This data migration makes their prior implicit access explicit (method-level sign_event + nip04/nip44 encrypt/decrypt, kind NULL) so live signers keep working; operators can then tighten in the dashboard. Only touches ACTIVE/PENDING connections that currently have zero permissions; idempotent. Validated against a throwaway PostgreSQL 17 (full migrate deploy + logic + idempotency).
Wrap the password-hash update and session deleteMany in a single $transaction so a crash between them cannot persist the new password while leaving old refresh sessions valid. Surfaced by the panel review.
…eny UI
Per NIP-46 a client's requested permissions are a REQUEST, not a grant. Replace the
auto-grant-on-connect behaviour with an interactive approval flow:
- The connect handler records client-requested perms as PENDING (ConnectionPermission.allowed
= false) instead of granting them; the handshake still completes (ack/secret) and the
connection works with its already-granted (seeded/operator) perms. The capability check
enforces only granted (allowed = true) perms, so a client can never self-escalate.
- New service methods requestPermissions / approveRequests / denyRequests, and ownership-scoped
POST /connections/:id/permissions/{approve,deny} endpoints. setPermissions now manages the
GRANTED whitelist and preserves outstanding pending requests. Repurposes the previously-dead
'allowed' column (granted vs pending) — no schema migration needed.
- Frontend: ConnectionDetailView gets a 'requests awaiting approval' section (approve/deny,
per-item and all) and a corrected default-deny permission editor (the old empty=allow-all
framing was wrong after the default-deny change); ConnectionsView shows granted count + a
pending badge.
Also folds in two panel-review defense-in-depth fixes: ensureListeningForConnection and
sendConnectResponse now take a userId and scope the nsec-key lookup to its owner (same class
as the C1 fix), so they can never start a listener / sign a response with another user's key.
…-kind grant UI A gated request denied at runtime (e.g. sign_event:30078) is now recorded as a PENDING permission request so the operator can approve it in the dashboard — extending the interactive approval flow from connect-declared perms to the capabilities a client actually exercises. Recorded with skipDuplicates so a client retrying the same method/kind can't spam the queue, and it never downgrades a grant. Frontend: the capability toggle now reflects ONLY a method-level (all-kinds) grant instead of any kind-scoped row, so it no longer overstates what is allowed; adds a kind input to grant a specific sign_event kind (e.g. 30078) and a clear 'allow all kinds' control.
…E_SECURE The refresh cookie was marked Secure whenever NODE_ENV=production. A Secure cookie is silently dropped by the browser when the app is served over plain HTTP on a non-localhost host (e.g. http://mini7:8080), so the in-memory access token worked while navigating but every full page reload — which can only restore the session by exchanging the refresh cookie at /api/auth/refresh — found no cookie and bounced the user to /login. Add a COOKIE_SECURE override (secure-by-default: unset falls back to NODE_ENV==='production'). Operators serving over plain HTTP on a trusted network set COOKIE_SECURE=false; HTTPS deployments are unaffected. Documented in .env.example and wired through docker-compose.yml.
…generating a bunker:// URI In the bunker:// flow the operator generates the URI, so they are the authority over what the connection may do. Let them pick the granted permission seed up front in the 'Generate bunker:// URI' dialog instead of always falling back to the conservative default set. The chosen perms ride along the pending secret and become the auto-created connection's GRANTED permissions on connect (default-deny, operator-authoritative). Per NIP-46 the bunker:// URI carries no perms (unlike nostrconnect://), so this is the natural place for the operator to declare intent. An absent/empty/invalid selection still falls back to DEFAULT_CONNECTION_PERMISSIONS so a connection is never fail-open. - createConnection() takes an optional operator seed; bunker generate-uri validates a PermissionDescriptor[] and threads it through PendingSecretInfo -> handleNewConnect. - Frontend: permission picker (all-kinds toggles + sign_event kind input), prefilled with the defaults, in the generate dialog. - Tests: seed override + default fallback + pending-secret perms round-trip (server 173).
…er:// URI picker A single toggle grants every gated method (all kinds) at once, or clears the whole selection, alongside the per-method toggles and the sign_event kind input.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the highest-severity findings from a security/quality review of the connection-authorization and NIP-46 signing path — the core of what a bunker must enforce. Four commits, each independently testable.
nsecKeyId8c5172d8c5172d0322cd6ba7b6ba8c5172d0e191d7Details
C1 — scope nsec keys to their owner
createConnectionnow verifies thensecKeyIdbelongs to the authenticated user (nsecKey.findFirst({ where: { id, userId } })→NotFoundException) before creating the connection or starting a relay listener. Previously a user could create a connection referencing another user'snsecKeyIdand have the bunker sign/decrypt with the victim's key. The sister methodgenerate-bunker-urialready did this check;createConnectiondid not.H2 — default-deny permissions
The NIP-46 handler is now default-deny for every capability method (
sign_event,nip04/nip44_encrypt/decrypt): thepermissions.length > 0 &&short-circuit is removed, so an empty permission set no longer grants everything.ping/get_public_key/connect/switch_relaysstay unguarded. New connections are seeded with a conservative default set (sign_eventkinds 0/1/3/4/7, no decrypt oracle); explicit perms from the connect request or dashboard still override the defaults.M1 — bind resolution to the signer key
The handler resolved the connection by
clientPubkeyalone, ignoring the signer pubkey the request was addressed to. A client with connections to several of the user's keys could have a request for key A served by a connection bound to key B.findByClientPubkeyis replaced byfindByClientAndSigner(clientPubkey, signerPubkey)which also filters onnsecKey.publicKey === signerPubkey.H1 — revoke sessions on password change
updatePasswordnow deletes the user's sessions (keeping the current one via the JWTsessionId), so a leaked/stale refresh token cannot survive the canonical post-compromise remediation. Wires in the previously-unused revoke-all-other-sessions intent.Grandfather migration
20260624130000_grandfather_connection_permissionsmakes existing perm-less ACTIVE/PENDING connections' prior implicit allow-all access explicit, so default-deny doesn't break live signers. Idempotent; only touches connections with zero permissions. Validated against a throwaway PostgreSQL 17 (fullmigrate deploy+ grandfather logic + idempotency).Under default-deny, any existing perm-less connection that the migration does not cover (e.g. a
REVOKED/EXPIREDone later reactivated, or rows created out-of-band) will be denied until permissions are granted via the dashboard or a fresh connect. The migration grandfathers the live (ACTIVE/PENDING) ones to avoid downtime.Tests & verification
bunker-rpc.handler.spec.ts(the previously-untested signing/permission dispatch): unknown/revoked clients, default-deny, per-kind enforcement, signer binding, unguarded ping/get_public_key, connect handshake, audit logging.connections.service.spec.ts(C1 ownership + default-permission seeding) andusers.service.spec.ts(H1 keep-current / revoke-all / no-revoke-on-wrong-password).prisma migrate deployclean on PG17,nest build0 TS issues, ESLint +lint:securityclean, prettierformat:checkclean.Not included (remaining review items)
M2 (TOTP replay window), M3 (SSRF via IPv4-mapped IPv6), M4 (SSE connection cap), and the low-severity items (username-enumeration timing, refresh-expiry unit bug, CSP header, etc.). Happy to follow up in separate PRs.