From 73d59a29bfa8d6befe32a02d7ceb80d3c55de98e Mon Sep 17 00:00:00 2001 From: Vitor Pamplona Date: Sun, 19 Jul 2026 21:28:05 -0400 Subject: [PATCH] fix(nip46): gate identity reads on pairing; make decrypt consent informed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two problems in the remote signer, both about a client getting something without the user meaningfully agreeing to it. **`get_public_key` and `get_relays` answered anyone.** Every other method runs through `ifAuthorized`; these did not, and nothing required a prior successful `connect`. The service decrypts and dispatches any well-formed kind-24133 envelope, so anyone holding the `bunker://` URI — pasted into a malicious app, posted for support, leaked in a screenshot — could ask it which account it belongs to, without the secret and without connecting. `get_relays` additionally handed over the inbox relay set. That defeated the transport/identity split, which otherwise works: the relay-visible traffic really is anonymous, since the p-tag and author are a transport key and the payload is NIP-44. Both now require the client to be paired. The authorizer interface gains `isPaired` with NO default, so a future authorizer has to state its own rule rather than silently inheriting "everyone is paired". `ping` is deliberately left open. It reveals nothing the caller does not already have — a signer is alive at a pubkey they hold — and first-party behaviour could be confirmed but third-party clients that ping before connecting could not be ruled out. Breaking a legitimate handshake to close a minor oracle is a bad trade. The choice is pinned by a test that also asserts the pairing check is never consulted, so it stays deliberate rather than drifting back by accident. **Decrypt consent showed nothing at all.** The bridge populated the content preview and raw data only for signing requests, so a decrypt request produced an empty preview block — no ciphertext, no counterparty, not even the "Show event" toggle — leaving "AppName wants to read your private messages" with *Allow always* as the primary button. Meanwhile the coordinator documented the opposite: "Amethyst decrypts first, then asks permission to expose." That was never implemented. Now: - The counterparty is resolved and shown, so the prompt reads "…read your private messages **with Alice**". It never degrades to nothing — cached name, else a shortened npub. Knowing *whose* messages is a categorically different decision. - The message is decrypted BEFORE prompting and the plaintext is the preview, as documented. It is a local operation and nothing is exposed until approval. Failure, blank and hang all collapse to an explanatory string under a timeout, so the dialog is never empty and cannot stall. - A narrower grant is offered ALONGSIDE the broad one, not instead of it: `DecryptFrom(counterparty)` keyed `decrypt:` next to `Decrypt`. The dialog's primary button becomes "Always allow for Alice" with the broad option demoted. Because the ledger stores an opaque op key, no persisted decision migrates and the storage format is untouched. Scoping decrypt per counterparty *instead* would have been worse than the bug: a DM client would prompt once per conversation, training users to approve everything. A narrow option beside the broad one gives granularity without the prompt explosion. Also fixes a latent bug found on the way: `AllowForSession` recorded the *requested* op rather than the *granted* one, which would have widened a narrow session grant back to broad. Verified by three sabotage passes; the tests that stayed green under them are the intended negative guards. One existing test asserted the buggy behaviour outright ("public reads are never gated") and was rewritten. Not done: the batched consent sheet still records the broad op for "remember" — offering the narrow choice per row there is a UX design question, not a mechanical change. Needs a device check before release: the decrypt preview runs the account signer before consent. That is free for a local key, but an account backed by an external NIP-55 signer (Amber) may show Amber's own prompt ahead of Amethyst's. Co-Authored-By: Claude Opus 4.8 --- .../consent/SignerConsentActivity.kt | 41 +++- .../consent/SignerConsentCoordinator.kt | 17 ++ .../model/nip46Signer/Nip46ConsentBridge.kt | 80 +++++--- .../nip46Signer/Nip46ConsentInfoBuilder.kt | 176 +++++++++++++++++ .../model/nip46Signer/Nip46SignerState.kt | 7 +- .../amethyst/napplet/NostrSignerOpLabels.kt | 19 ++ .../napplets/ConnectedAppDetailScreen.kt | 2 + amethyst/src/main/res/values/strings.xml | 8 + .../Nip46ConsentInfoBuilderTest.kt | 177 ++++++++++++++++++ .../amethyst/cli/commands/BunkerCommand.kt | 15 ++ .../nip46/Nip46PermissionAuthorizer.kt | 56 +++++- .../connectedApps/signers/NostrSignerOp.kt | 18 +- .../signers/NostrSignerPermissionLedger.kt | 4 +- .../nip46/Nip46PermissionAuthorizerTest.kt | 150 +++++++++++++++ .../NostrSignerPermissionLedgerTest.kt | 28 +++ .../server/BunkerRequestProcessor.kt | 58 +++++- .../server/Nip46RequestAuthorizer.kt | 20 +- .../BunkerRequestProcessorConcurrencyTest.kt | 2 + .../server/BunkerRequestProcessorTest.kt | 72 ++++++- .../server/NostrConnectSignerServiceTest.kt | 2 + 20 files changed, 893 insertions(+), 59 deletions(-) create mode 100644 amethyst/src/main/java/com/vitorpamplona/amethyst/model/nip46Signer/Nip46ConsentInfoBuilder.kt create mode 100644 amethyst/src/test/java/com/vitorpamplona/amethyst/model/nip46Signer/Nip46ConsentInfoBuilderTest.kt diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/connectedApps/consent/SignerConsentActivity.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/connectedApps/consent/SignerConsentActivity.kt index e9528737f2..0e9f7edc40 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/connectedApps/consent/SignerConsentActivity.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/connectedApps/consent/SignerConsentActivity.kt @@ -193,6 +193,16 @@ private fun SignerConsentDialog( if (info.accountName != null) { ConnectedAccountRow(info.accountName, info.accountPicture, info.accountPubKey) } + // For a decrypt request, WHOSE conversation is being read is the decision. Show + // that person as an avatar + name, never as nothing. + if (info.counterpartyName != null) { + Text( + stringResource(R.string.nip46_signer_messages_with), + style = MaterialTheme.typography.labelSmall, + color = MaterialTheme.colorScheme.onSurfaceVariant, + ) + ConnectedAccountRow(info.counterpartyName, info.counterpartyPicture, info.counterpartyPubKey) + } } Spacer(Modifier.height(12.dp)) @@ -204,12 +214,31 @@ private fun SignerConsentDialog( HorizontalDivider() Spacer(Modifier.height(8.dp)) - // Primary: always allow this op - Button( - onClick = { onGrant(SignerOpGrant.AllowForOp(info.op)) }, - modifier = Modifier.fillMaxWidth().padding(horizontal = 24.dp), - ) { - Text(stringResource(R.string.napplet_consent_allow_always)) + // Primary: the NARROWEST "remember" available. For decrypt that is "always allow for + // Alice" — one broad decrypt grant would otherwise hand over every conversation + // forever, and scoping the op itself would mean a prompt per conversation. + val narrowOp = info.narrowOp + if (narrowOp != null && info.narrowOpLabel != null) { + Button( + onClick = { onGrant(SignerOpGrant.AllowForOp(narrowOp)) }, + modifier = Modifier.fillMaxWidth().padding(horizontal = 24.dp), + ) { + Text(info.narrowOpLabel) + } + // The broad grant stays available, but demoted below the scoped one. + OutlinedButton( + onClick = { onGrant(SignerOpGrant.AllowForOp(info.op)) }, + modifier = Modifier.fillMaxWidth().padding(horizontal = 24.dp), + ) { + Text(stringResource(R.string.napplet_consent_allow_always)) + } + } else { + Button( + onClick = { onGrant(SignerOpGrant.AllowForOp(info.op)) }, + modifier = Modifier.fillMaxWidth().padding(horizontal = 24.dp), + ) { + Text(stringResource(R.string.napplet_consent_allow_always)) + } } // Secondary: allow just once diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/connectedApps/consent/SignerConsentCoordinator.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/connectedApps/consent/SignerConsentCoordinator.kt index 92730ea5b7..ac3f5247fe 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/connectedApps/consent/SignerConsentCoordinator.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/connectedApps/consent/SignerConsentCoordinator.kt @@ -65,6 +65,23 @@ data class SignerConsentInfo( * non-event ops. */ val previewTemplate: EventTemplate? = null, + /** + * The OTHER party of a decrypt request — whose conversation the app is asking to read — shown as + * an avatar + name. "X wants to read your messages with Alice" is a categorically different + * decision from "X wants to read your private messages", so this must reach the dialog. + * Null for every op that has no counterparty (signing, and the napplet/browser paths). + */ + val counterpartyName: String? = null, + val counterpartyPicture: String? = null, + val counterpartyPubKey: String? = null, + /** + * A NARROWER op the dialog may offer to remember instead of [op] — today only + * [com.vitorpamplona.amethyst.commons.connectedApps.signers.NostrSignerOp.DecryptFrom], i.e. + * "always allow, but only for this counterparty". Offered ALONGSIDE the broad "Always allow" so + * the user gets granularity without a prompt per conversation. [narrowOpLabel] is its button text. + */ + val narrowOp: NostrSignerOp? = null, + val narrowOpLabel: String? = null, ) /** One pending per-operation consent request, as the batched sheet renders it. */ diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/model/nip46Signer/Nip46ConsentBridge.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/nip46Signer/Nip46ConsentBridge.kt index caf6de75ec..104166e77a 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/model/nip46Signer/Nip46ConsentBridge.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/nip46Signer/Nip46ConsentBridge.kt @@ -29,14 +29,14 @@ import com.vitorpamplona.amethyst.commons.connectedApps.signers.SignerOpGrant import com.vitorpamplona.amethyst.connectedApps.consent.SignerConnectCoordinator import com.vitorpamplona.amethyst.connectedApps.consent.SignerConnectInfo import com.vitorpamplona.amethyst.connectedApps.consent.SignerConsentCoordinator -import com.vitorpamplona.amethyst.connectedApps.consent.SignerConsentInfo import com.vitorpamplona.amethyst.model.LocalCache import com.vitorpamplona.amethyst.napplet.label import com.vitorpamplona.quartz.nip01Core.core.HexKey -import com.vitorpamplona.quartz.nip01Core.jackson.JacksonMapper +import com.vitorpamplona.quartz.nip01Core.signers.NostrSigner import com.vitorpamplona.quartz.nip46RemoteSigner.BunkerRequest import com.vitorpamplona.quartz.nip46RemoteSigner.BunkerRequestConnect -import com.vitorpamplona.quartz.nip46RemoteSigner.BunkerRequestSign +import com.vitorpamplona.quartz.nip46RemoteSigner.BunkerRequestNip04Decrypt +import com.vitorpamplona.quartz.nip46RemoteSigner.BunkerRequestNip44Decrypt import kotlinx.coroutines.withTimeoutOrNull /** @@ -119,39 +119,43 @@ object Nip46ConsentBridge { } ?: AppConnectResult.Cancelled } - /** Per-operation consent: describe the request (op + event preview) and await the user's grant. */ + /** + * Per-operation consent: describe the request and await the user's grant. + * + * For a decrypt request this DECRYPTS FIRST and shows the resulting plaintext, together with the + * counterparty the conversation is with. That is what makes the decision reviewable: without it + * the dialog said only "wants to read your private messages" with no way to tell one request from + * another. Decryption is local — [signer] runs on this device and nothing leaves it unless the + * user approves — and it is bounded by [Nip46ConsentInfoBuilder.DECRYPT_PREVIEW_TIMEOUT_MS] so a slow or failing signer + * degrades to an explanatory message instead of hanging or blanking the prompt. + */ suspend fun requestOp( coordinate: String, clientPubKey: HexKey, op: NostrSignerOp, request: BunkerRequest, + signer: NostrSigner, ): SignerOpGrant { val context = Amethyst.instance.appContext val info = runCatching { Amethyst.instance.nip46ClientStore.load(coordinate) }.getOrNull() val title = info?.name?.ifBlank { null } ?: context.getString(R.string.nip46_signer_remote_app) - val preview = - if (request is BunkerRequestSign) { - request.event.content - .take(160) - .trim() - } else { - "" - } - val rawData = if (request is BunkerRequestSign) JacksonMapper.toJsonPretty(request.event) else "" - val face = accountFace(coordinate) + val consentInfo = - SignerConsentInfo( - appletTitle = title, + Nip46ConsentInfoBuilder.build( coordinate = coordinate, - op = op, - operationSummary = op.label(context), - contentPreview = preview, - rawData = rawData, + title = title, iconUrl = info?.image, - accountName = face.name, - accountPicture = face.picture, - accountPubKey = face.pubKey, - previewTemplate = (request as? BunkerRequestSign)?.event, + op = op, + request = request, + account = accountFace(coordinate), + faceOf = ::userFace, + strings = + Nip46ConsentStrings( + opLabel = { it.label(context) }, + allowAlwaysFor = { context.getString(R.string.nip46_signer_allow_always_for, it) }, + decryptFailed = context.getString(R.string.nip46_signer_decrypt_failed), + ), + decrypt = { decryptWithAccountSigner(signer, it) }, ) // Fail closed if the prompt is never answered so a stuck dialog can't hold the signer hostage. return withTimeoutOrNull(CONSENT_TIMEOUT_MS) { @@ -159,16 +163,30 @@ object Nip46ConsentBridge { } ?: SignerOpGrant.DenyOnce } + /** + * Performs the local decryption behind the decrypt preview with the account's own signer. Errors + * and timeouts are handled by [Nip46ConsentInfoBuilder]; this only maps the request to a call. + */ + private suspend fun decryptWithAccountSigner( + signer: NostrSigner, + request: BunkerRequest, + ): String? = + when (request) { + is BunkerRequestNip04Decrypt -> signer.nip04Decrypt(request.ciphertext, request.pubKey) + is BunkerRequestNip44Decrypt -> signer.nip44Decrypt(request.ciphertext, request.pubKey) + else -> null + } + /** The account being signed for (avatar + name), resolved from the coordinate's signer pubkey. */ - private fun accountFace(coordinate: String): AccountFace { + private fun accountFace(coordinate: String): SignerFace { val pubKey = Nip46PermissionAuthorizer.signerPubKeyOf(coordinate) val user = pubKey?.let { LocalCache.getUserIfExists(it) } - return AccountFace(name = user?.toBestDisplayName(), picture = user?.profilePicture(), pubKey = pubKey) + return SignerFace(name = user?.toBestDisplayName(), picture = user?.profilePicture(), pubKey = pubKey) } - private data class AccountFace( - val name: String?, - val picture: String?, - val pubKey: String?, - ) + /** Cached profile for a counterparty; the builder supplies the shortened-npub fallback. */ + private fun userFace(pubKey: HexKey): SignerFace { + val user = LocalCache.getUserIfExists(pubKey) + return SignerFace(name = user?.toBestDisplayName(), picture = user?.profilePicture(), pubKey = pubKey) + } } diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/model/nip46Signer/Nip46ConsentInfoBuilder.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/nip46Signer/Nip46ConsentInfoBuilder.kt new file mode 100644 index 0000000000..c31b73dc4f --- /dev/null +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/nip46Signer/Nip46ConsentInfoBuilder.kt @@ -0,0 +1,176 @@ +/* + * Copyright (c) 2025 Vitor Pamplona + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * this software and associated documentation files (the "Software"), to deal in + * the Software without restriction, including without limitation the rights to use, + * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the + * Software, and to permit persons to whom the Software is furnished to do so, + * subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS + * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR + * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN + * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION + * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ +package com.vitorpamplona.amethyst.model.nip46Signer + +import com.vitorpamplona.amethyst.commons.connectedApps.nip46.Nip46PermissionAuthorizer.Companion.decryptCounterparty +import com.vitorpamplona.amethyst.commons.connectedApps.nip46.Nip46PermissionAuthorizer.Companion.toNarrowSignerOp +import com.vitorpamplona.amethyst.commons.connectedApps.signers.NostrSignerOp +import com.vitorpamplona.amethyst.connectedApps.consent.SignerConsentInfo +import com.vitorpamplona.quartz.nip01Core.core.HexKey +import com.vitorpamplona.quartz.nip01Core.jackson.JacksonMapper +import com.vitorpamplona.quartz.nip19Bech32.entities.NPub +import com.vitorpamplona.quartz.nip46RemoteSigner.BunkerRequest +import com.vitorpamplona.quartz.nip46RemoteSigner.BunkerRequestSign +import com.vitorpamplona.quartz.utils.Log +import kotlinx.coroutines.CancellationException +import kotlinx.coroutines.withTimeoutOrNull + +/** Avatar + display name for one pubkey, as the consent dialogs render it. */ +data class SignerFace( + val name: String?, + val picture: String?, + val pubKey: String?, +) + +/** + * The user-visible strings the builder needs, injected rather than read from `R.string` so the + * builder itself carries no Android dependency and can be unit-tested. + */ +class Nip46ConsentStrings( + /** Human-readable label for an op, e.g. "read your private messages with Alice". */ + val opLabel: (NostrSignerOp) -> String, + /** Button text for the counterparty-scoped grant; the argument is the counterparty's name. */ + val allowAlwaysFor: (String) -> String, + /** Shown as the preview when Amethyst itself could not decrypt the message. */ + val decryptFailed: String, +) + +/** + * Builds the [SignerConsentInfo] for one NIP-46 per-operation prompt. + * + * Split out of [Nip46ConsentBridge] (which owns the Android `Context`/`LocalCache` lookups) so the + * decisions that matter for safety are testable without an emulator: + * - a decrypt request is DECRYPTED FIRST and the plaintext becomes the preview, honouring the + * contract the dialog documented but never implemented; + * - a decrypt that cannot be decrypted still produces a populated dialog, never a blank one; + * - the counterparty label is never empty — it degrades to a shortened npub, never to nothing. + */ +object Nip46ConsentInfoBuilder { + /** Characters of plaintext/content shown inline before the "show more" toggle takes over. */ + const val PREVIEW_MAX_CHARS = 160 + + /** + * Upper bound on the pre-consent decryption. Short on purpose: the preview is a nicety, the + * prompt is not, so a signer that stalls (e.g. an external NIP-55 app that is not responding) + * must not delay the dialog. + */ + const val DECRYPT_PREVIEW_TIMEOUT_MS = 8_000L + + suspend fun build( + coordinate: String, + title: String, + iconUrl: String?, + op: NostrSignerOp, + request: BunkerRequest, + account: SignerFace, + /** Resolves a pubkey to a cached profile; the builder supplies its own npub fallback. */ + faceOf: (HexKey) -> SignerFace, + strings: Nip46ConsentStrings, + /** Performs the local decryption. May fail, return null, or hang — all are handled. */ + decrypt: suspend (BunkerRequest) -> String?, + ): SignerConsentInfo { + val counterparty = request.decryptCounterparty() + val plaintext = if (counterparty != null) decryptPreview(request, decrypt, strings.decryptFailed) else null + + val preview = + when { + request is BunkerRequestSign -> + request.event.content + .take(PREVIEW_MAX_CHARS) + .trim() + plaintext != null -> plaintext.take(PREVIEW_MAX_CHARS).trim() + else -> "" + } + val rawData = + when { + request is BunkerRequestSign -> JacksonMapper.toJsonPretty(request.event) + // Only worth a "show more" toggle when the preview actually truncated it. + plaintext != null && plaintext.length > PREVIEW_MAX_CHARS -> plaintext + else -> "" + } + + // A decrypt grant can be scoped to one conversation: offer "always allow for Alice" next to + // the broad "always allow", instead of only the all-conversations-forever choice. + val narrowOp = request.toNarrowSignerOp() + val counterpartyFace = counterparty?.let { face(it, faceOf) } + + return SignerConsentInfo( + appletTitle = title, + coordinate = coordinate, + op = op, + // For decrypt this names the counterparty ("read your private messages with Alice"). + operationSummary = strings.opLabel(narrowOp ?: op), + contentPreview = preview, + rawData = rawData, + iconUrl = iconUrl, + accountName = account.name, + accountPicture = account.picture, + accountPubKey = account.pubKey, + previewTemplate = (request as? BunkerRequestSign)?.event, + counterpartyName = counterpartyFace?.name, + counterpartyPicture = counterpartyFace?.picture, + counterpartyPubKey = counterparty, + narrowOp = narrowOp, + narrowOpLabel = counterpartyFace?.name?.let { strings.allowAlwaysFor(it) }, + ) + } + + /** + * Decrypts the message the app asked to read. Never throws and never hangs: a signer that fails, + * refuses, returns nothing, or takes too long yields [failureText], because a request whose + * ciphertext we cannot even read is itself worth showing — a blank dialog is not. + */ + private suspend fun decryptPreview( + request: BunkerRequest, + decrypt: suspend (BunkerRequest) -> String?, + failureText: String, + ): String = + withTimeoutOrNull(DECRYPT_PREVIEW_TIMEOUT_MS) { + try { + decrypt(request)?.ifBlank { null } + } catch (e: CancellationException) { + // Includes this block's own timeout — must propagate so withTimeoutOrNull sees it. + throw e + } catch (e: Exception) { + Log.w("NIP46Signer") { "decrypt preview failed: ${e.message}" } + null + } + } ?: failureText + + /** [faceOf], but with a guaranteed non-blank name (shortened npub when the user isn't cached). */ + private fun face( + pubKey: HexKey, + faceOf: (HexKey) -> SignerFace, + ): SignerFace { + val resolved = runCatching { faceOf(pubKey) }.getOrNull() + return SignerFace( + name = resolved?.name?.ifBlank { null } ?: shortIdentifier(pubKey), + picture = resolved?.picture, + pubKey = pubKey, + ) + } + + /** A shortened npub for an uncached pubkey; falls back to the hex prefix if it isn't valid hex. */ + fun shortIdentifier(pubKey: HexKey): String { + val npub = runCatching { NPub.create(pubKey) }.getOrNull() + return if (!npub.isNullOrBlank()) npub.take(12) + "…" else pubKey.take(12) + "…" + } +} diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/model/nip46Signer/Nip46SignerState.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/nip46Signer/Nip46SignerState.kt index 5466080f30..4fd726c943 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/model/nip46Signer/Nip46SignerState.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/nip46Signer/Nip46SignerState.kt @@ -170,7 +170,12 @@ class Nip46SignerState( // connect, and an allow/deny prompt whenever the ledger says ASK (dangerous kinds, // decryption, DMs, or a PARANOID app). Same surface + ledger as napplet/browser signing. connectConsent = Nip46ConsentBridge::requestConnect, - opConsent = Nip46ConsentBridge::requestOp, + // The account's own signer goes to the bridge so a decrypt request can be decrypted + // BEFORE the prompt — the dialog shows the actual plaintext instead of an opaque + // "wants to read your private messages". Local only; nothing is disclosed until approval. + opConsent = { coordinate, clientPubKey, op, request -> + Nip46ConsentBridge.requestOp(coordinate, clientPubKey, op, request, signer) + }, ) init { diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/napplet/NostrSignerOpLabels.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/napplet/NostrSignerOpLabels.kt index b7fab80348..3472835691 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/napplet/NostrSignerOpLabels.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/napplet/NostrSignerOpLabels.kt @@ -30,10 +30,13 @@ import com.vitorpamplona.amethyst.commons.napplet.protocol.NappletRequest import com.vitorpamplona.amethyst.connectedApps.consent.SignerConnectInfo import com.vitorpamplona.amethyst.connectedApps.consent.SignerConsentInfo import com.vitorpamplona.amethyst.favorites.BrowserIconRegistry +import com.vitorpamplona.amethyst.model.LocalCache import com.vitorpamplona.amethyst.ui.screen.loggedIn.relays.kindNameFor import com.vitorpamplona.quartz.nip01Core.core.Event +import com.vitorpamplona.quartz.nip01Core.core.HexKey import com.vitorpamplona.quartz.nip01Core.jackson.JacksonMapper import com.vitorpamplona.quartz.nip01Core.signers.EventTemplate +import com.vitorpamplona.quartz.nip19Bech32.entities.NPub import com.vitorpamplona.quartz.utils.TimeUtils /** Human-readable label for a [NostrSignerOp]. */ @@ -42,8 +45,24 @@ fun NostrSignerOp.label(context: Context): String = is NostrSignerOp.SignKind -> context.getString(R.string.napplet_op_sign_kind_named, kindNameFor(context, kind), kind) NostrSignerOp.Encrypt -> context.getString(R.string.napplet_op_encrypt) NostrSignerOp.Decrypt -> context.getString(R.string.napplet_op_decrypt) + is NostrSignerOp.DecryptFrom -> context.getString(R.string.napplet_op_decrypt_from, counterpartyLabel(counterparty)) } +/** + * A person's display name for a consent prompt: their profile name when we have it cached, otherwise + * a shortened npub. Never empty — "read your private messages with " would be worse than the + * broad wording it replaces. + */ +fun counterpartyLabel(pubKeyHex: HexKey): String { + LocalCache + .getUserIfExists(pubKeyHex) + ?.toBestDisplayName() + ?.ifBlank { null } + ?.let { return it } + val npub = runCatching { NPub.create(pubKeyHex) }.getOrNull() + return if (npub != null) npub.take(12) + "…" else pubKeyHex.take(12) + "…" +} + /** Builds the [SignerConsentInfo] needed by the per-op consent dialog. */ fun buildSignerConsentInfo( context: Context, diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/napplets/ConnectedAppDetailScreen.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/napplets/ConnectedAppDetailScreen.kt index d7abf620c2..ea09d12af3 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/napplets/ConnectedAppDetailScreen.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/napplets/ConnectedAppDetailScreen.kt @@ -82,6 +82,7 @@ import com.vitorpamplona.amethyst.commons.napplet.permissions.NappletPermissionL import com.vitorpamplona.amethyst.favorites.BrowserIconRegistry import com.vitorpamplona.amethyst.favorites.rememberManifestIconModel import com.vitorpamplona.amethyst.favorites.rememberWebAppIconModel +import com.vitorpamplona.amethyst.napplet.counterpartyLabel import com.vitorpamplona.amethyst.napplet.descriptionRes import com.vitorpamplona.amethyst.napplet.labelRes import com.vitorpamplona.amethyst.napplet.resolveNappletMeta @@ -703,6 +704,7 @@ private fun NostrSignerOp.opLabel(): String = is NostrSignerOp.SignKind -> stringResource(R.string.napplet_op_sign_kind, kind) NostrSignerOp.Encrypt -> stringResource(R.string.napplet_op_encrypt) NostrSignerOp.Decrypt -> stringResource(R.string.napplet_op_decrypt) + is NostrSignerOp.DecryptFrom -> stringResource(R.string.napplet_op_decrypt_from, counterpartyLabel(counterparty)) } @Composable diff --git a/amethyst/src/main/res/values/strings.xml b/amethyst/src/main/res/values/strings.xml index e74fab5127..ae9eb3ad26 100644 --- a/amethyst/src/main/res/values/strings.xml +++ b/amethyst/src/main/res/values/strings.xml @@ -947,6 +947,14 @@ encrypt a message read your private messages + + read your private messages with %1$s + + Always allow for %1$s + + Amethyst could not decrypt this message. It may not be addressed to this account. + + Messages with Connected Apps diff --git a/amethyst/src/test/java/com/vitorpamplona/amethyst/model/nip46Signer/Nip46ConsentInfoBuilderTest.kt b/amethyst/src/test/java/com/vitorpamplona/amethyst/model/nip46Signer/Nip46ConsentInfoBuilderTest.kt new file mode 100644 index 0000000000..bb5344e8c6 --- /dev/null +++ b/amethyst/src/test/java/com/vitorpamplona/amethyst/model/nip46Signer/Nip46ConsentInfoBuilderTest.kt @@ -0,0 +1,177 @@ +/* + * Copyright (c) 2025 Vitor Pamplona + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * this software and associated documentation files (the "Software"), to deal in + * the Software without restriction, including without limitation the rights to use, + * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the + * Software, and to permit persons to whom the Software is furnished to do so, + * subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS + * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR + * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN + * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION + * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ +package com.vitorpamplona.amethyst.model.nip46Signer + +import com.vitorpamplona.amethyst.commons.connectedApps.signers.NostrSignerOp +import com.vitorpamplona.amethyst.connectedApps.consent.SignerConsentInfo +import com.vitorpamplona.quartz.nip01Core.core.Event +import com.vitorpamplona.quartz.nip01Core.signers.EventTemplate +import com.vitorpamplona.quartz.nip46RemoteSigner.BunkerRequest +import com.vitorpamplona.quartz.nip46RemoteSigner.BunkerRequestNip04Decrypt +import com.vitorpamplona.quartz.nip46RemoteSigner.BunkerRequestNip44Decrypt +import com.vitorpamplona.quartz.nip46RemoteSigner.BunkerRequestSign +import kotlinx.coroutines.awaitCancellation +import kotlinx.coroutines.test.runTest +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNotNull +import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue +import org.junit.Test + +/** + * The decrypt-consent contract: a user asked to expose a private message must be able to see WHOSE + * conversation it is and WHAT it says. Before this, both were empty for every decrypt request, so the + * dialog skipped its whole preview block and offered "Allow always" over a blank body. + */ +class Nip46ConsentInfoBuilderTest { + private val alice = "1".repeat(64) + private val coordinate = "nip46:${"a".repeat(64)}:${"c".repeat(64)}" + private val account = SignerFace(name = "Me", picture = null, pubKey = "a".repeat(64)) + + private val decryptFailedText = "Amethyst could not decrypt this message." + + private fun strings() = + Nip46ConsentStrings( + opLabel = { op -> + when (op) { + is NostrSignerOp.DecryptFrom -> "read your private messages with ${op.counterparty.take(6)}" + NostrSignerOp.Decrypt -> "read your private messages" + else -> "do something" + } + }, + allowAlwaysFor = { "Always allow for $it" }, + decryptFailed = decryptFailedText, + ) + + private suspend fun build( + request: BunkerRequest, + op: NostrSignerOp = NostrSignerOp.Decrypt, + faceOf: (String) -> SignerFace = { SignerFace(name = null, picture = null, pubKey = it) }, + decrypt: suspend (BunkerRequest) -> String? = { "the plaintext" }, + ): SignerConsentInfo = + Nip46ConsentInfoBuilder.build( + coordinate = coordinate, + title = "Some App", + iconUrl = null, + op = op, + request = request, + account = account, + faceOf = faceOf, + strings = strings(), + decrypt = decrypt, + ) + + @Test + fun decryptConsentCarriesTheCounterpartyAndThePlaintext() = + runTest { + val info = build(BunkerRequestNip44Decrypt("1", alice, "ciphertext")) + + assertEquals(alice, info.counterpartyPubKey) + assertTrue("counterparty label must not be empty", !info.counterpartyName.isNullOrBlank()) + assertEquals("the plaintext", info.contentPreview) + // The dialog renders its preview block only when one of these is non-blank. + assertTrue("the dialog must have content to show", info.contentPreview.isNotBlank() || info.rawData.isNotBlank()) + } + + @Test + fun anUncachedCounterpartyFallsBackToAShortenedNpubNeverToNothing() = + runTest { + val info = build(BunkerRequestNip44Decrypt("1", alice, "ct"), faceOf = { SignerFace(null, null, it) }) + + val name = info.counterpartyName + assertNotNull(name) + assertTrue("expected an npub fallback, got '$name'", name!!.startsWith("npub")) + } + + @Test + fun aBlankCachedNameStillFallsBackRatherThanShowingAnEmptyLabel() = + runTest { + val info = build(BunkerRequestNip44Decrypt("1", alice, "ct"), faceOf = { SignerFace(" ", null, it) }) + + assertTrue(!info.counterpartyName.isNullOrBlank()) + } + + @Test + fun anUndecryptableMessageStillProducesAPopulatedDialog() = + runTest { + val info = build(BunkerRequestNip04Decrypt("1", alice, "garbage"), decrypt = { error("bad ciphertext") }) + + assertEquals(decryptFailedText, info.contentPreview) + assertTrue("the counterparty is still shown", !info.counterpartyName.isNullOrBlank()) + assertTrue("the dialog must not be blank", info.contentPreview.isNotBlank()) + } + + @Test + fun aSignerThatReturnsNothingIsTreatedAsAFailureNotAsAnEmptyDialog() = + runTest { + val blank = build(BunkerRequestNip44Decrypt("1", alice, "ct"), decrypt = { " " }) + assertEquals(decryptFailedText, blank.contentPreview) + + val none = build(BunkerRequestNip44Decrypt("1", alice, "ct"), decrypt = { null }) + assertEquals(decryptFailedText, none.contentPreview) + } + + /** A signer that never answers must not wedge the prompt — runTest fast-forwards the timeout. */ + @Test + fun aHangingSignerTimesOutIntoTheFailureTextInsteadOfBlockingThePrompt() = + runTest { + val info = build(BunkerRequestNip44Decrypt("1", alice, "ct"), decrypt = { awaitCancellation() }) + + assertEquals(decryptFailedText, info.contentPreview) + } + + @Test + fun aLongPlaintextIsTruncatedInlineAndOfferedInFullBehindTheToggle() = + runTest { + val long = "x".repeat(500) + val info = build(BunkerRequestNip44Decrypt("1", alice, "ct"), decrypt = { long }) + + assertEquals(Nip46ConsentInfoBuilder.PREVIEW_MAX_CHARS, info.contentPreview.length) + assertEquals(long, info.rawData) + } + + @Test + fun decryptOffersANarrowerPerCounterpartyGrantAlongsideTheBroadOne() = + runTest { + val info = build(BunkerRequestNip44Decrypt("1", alice, "ct")) + + assertEquals(NostrSignerOp.DecryptFrom(alice), info.narrowOp) + assertTrue("the narrow button needs a label", !info.narrowOpLabel.isNullOrBlank()) + // The broad op is still what the dialog's "Always allow" grants. + assertEquals(NostrSignerOp.Decrypt, info.op) + // The headline names the counterparty rather than saying "your private messages". + assertTrue(info.operationSummary.contains("with")) + } + + @Test + fun signRequestsAreUnchangedAndCarryNoCounterparty() = + runTest { + val request = BunkerRequestSign("1", EventTemplate(createdAt = 1L, kind = 1, tags = emptyArray(), content = "hello")) + val info = build(request, op = NostrSignerOp.SignKind(1)) + + assertEquals("hello", info.contentPreview) + assertTrue("sign still shows its JSON", info.rawData.contains("\"kind\"")) + assertNull(info.counterpartyName) + assertNull(info.counterpartyPubKey) + assertNull(info.narrowOp) + assertNotNull(info.previewTemplate) + } +} diff --git a/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/BunkerCommand.kt b/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/BunkerCommand.kt index 130414c7c0..db81032a02 100644 --- a/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/BunkerCommand.kt +++ b/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/BunkerCommand.kt @@ -47,6 +47,7 @@ import kotlinx.coroutines.sync.Mutex import kotlinx.coroutines.sync.withLock import kotlinx.coroutines.withContext import kotlinx.coroutines.withTimeoutOrNull +import java.util.concurrent.ConcurrentHashMap /** * `amy bunker [--relay URL[,URL…]] [--secret S] [--perms P] [--interactive] [--timeout SECS]` @@ -94,11 +95,25 @@ object BunkerCommand { ) : Nip46RequestAuthorizer { private val promptLock = Mutex() + /** + * Clients that presented the right secret in this process's lifetime. The CLI bunker keeps no + * state on disk, so pairing is in-memory: a client must `connect` once per `amy bunker` run + * before it can read the hosted identity. + */ + private val paired = ConcurrentHashMap.newKeySet() + + /** + * Ungated (the headless default hosting the operator's own key) everything is open, matching + * [authorize]. Gated, the identity reads require a `connect` first. + */ + override suspend fun isPaired(clientPubKey: HexKey): Boolean = !gated || clientPubKey in paired + override suspend fun onConnect( clientPubKey: HexKey, request: BunkerRequestConnect, ): Nip46ConnectDecision = if (request.secret == secret) { + paired.add(clientPubKey) Nip46ConnectDecision.Accept(BunkerRequestProcessor.ACK) } else { Nip46ConnectDecision.Reject("invalid secret") diff --git a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/connectedApps/nip46/Nip46PermissionAuthorizer.kt b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/connectedApps/nip46/Nip46PermissionAuthorizer.kt index 1eb84daa24..ff7d41797e 100644 --- a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/connectedApps/nip46/Nip46PermissionAuthorizer.kt +++ b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/connectedApps/nip46/Nip46PermissionAuthorizer.kt @@ -136,6 +136,14 @@ class Nip46PermissionAuthorizer( if (shouldWrite) ledger.updateLastUsed(coordinate, now) } + /** + * A client is paired exactly when the ledger holds a trust level for its coordinate — which is + * what a successful `connect` writes (and what "Forget" removes). Because it is persisted, a + * client stays paired across restarts, so a returning app that does not re-`connect` still reads + * the identity it was already granted. + */ + override suspend fun isPaired(clientPubKey: HexKey): Boolean = ledger.hasPolicy(coordinateFor(clientPubKey)) + override suspend fun onConnect( clientPubKey: HexKey, request: BunkerRequestConnect, @@ -187,16 +195,23 @@ class Nip46PermissionAuthorizer( // sign/encrypt/decrypt, so this branch is a safety net). val op = request.toSignerOp() ?: return true val coordinate = coordinateFor(clientPubKey) + // A decrypt request also carries a narrower op ("decrypt messages from THIS counterparty"). + // A standing narrow grant satisfies the request without widening the broad one. + val narrowOp = request.toNarrowSignerOp() val allowed = when (ledger.decide(coordinate, op)) { NostrOpDecision.ALLOW -> true + // An explicit DENY on the broad op is final — a narrow grant never overrides it. NostrOpDecision.DENY -> false - // ASK: honor a live session grant first, otherwise prompt the user (if wired). No - // prompt → deny, so a headless signer only ever performs pre-granted operations. + // ASK: honor a live session grant first, then any narrower standing/session grant, + // otherwise prompt the user (if wired). No prompt → deny, so a headless signer only + // ever performs pre-granted operations. NostrOpDecision.ASK -> { if (isSessionAllowed(coordinate, op)) { true + } else if (narrowOp != null && isNarrowAllowed(coordinate, narrowOp)) { + true } else { askOpConsent(coordinate, clientPubKey, op, request) } @@ -215,11 +230,23 @@ class Nip46PermissionAuthorizer( val grant = opConsent?.invoke(coordinate, clientPubKey, op, request) ?: return false ledger.record(coordinate, grant) if (grant is SignerOpGrant.AllowForSession) { - throttleLock.withLock { sessionAllows.add(sessionKey(coordinate, op)) } + // Use the GRANT's op, not the requested one: the dialog may have returned a narrower op + // ("only from this counterparty"), and a session grant must be no wider than what was given. + throttleLock.withLock { sessionAllows.add(sessionKey(coordinate, grant.op)) } } return grant.isAllowed } + /** True when a standing or session grant exists for the narrower [narrowOp] (e.g. decrypt-from-X). */ + private suspend fun isNarrowAllowed( + coordinate: String, + narrowOp: NostrSignerOp, + ): Boolean = + isSessionAllowed(coordinate, narrowOp) || + // Only an explicit per-op override counts. decide() would otherwise fall through to the + // app's policy, and FULL_TRUST/REASONABLE would answer for an op nobody ever granted. + ledger.store.loadOpDecision(coordinate, narrowOp)?.let { ledger.decide(coordinate, narrowOp) == NostrOpDecision.ALLOW } ?: false + private suspend fun isSessionAllowed( coordinate: String, op: NostrSignerOp, @@ -344,5 +371,28 @@ class Nip46PermissionAuthorizer( is BunkerRequestNip44Decrypt -> NostrSignerOp.Decrypt else -> null } + + /** + * The NARROWER op a request could be granted, or `null` when it has no narrower form. + * + * Only decryption has one today: `decrypt` reveals private conversations, and one broad + * "always allow" hands over every conversation forever — so the consent dialog can also offer + * "always allow for THIS counterparty" ([NostrSignerOp.DecryptFrom]). Encryption and signing + * have no equivalent: their counterparty/kind is already the thing being granted. + */ + fun BunkerRequest.toNarrowSignerOp(): NostrSignerOp? = + when (this) { + is BunkerRequestNip04Decrypt -> NostrSignerOp.DecryptFrom(pubKey) + is BunkerRequestNip44Decrypt -> NostrSignerOp.DecryptFrom(pubKey) + else -> null + } + + /** The counterparty a decrypt request names, or `null` for any other request. */ + fun BunkerRequest.decryptCounterparty(): HexKey? = + when (this) { + is BunkerRequestNip04Decrypt -> pubKey + is BunkerRequestNip44Decrypt -> pubKey + else -> null + } } } diff --git a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/connectedApps/signers/NostrSignerOp.kt b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/connectedApps/signers/NostrSignerOp.kt index 5987dc46ef..2187b69f07 100644 --- a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/connectedApps/signers/NostrSignerOp.kt +++ b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/connectedApps/signers/NostrSignerOp.kt @@ -33,9 +33,22 @@ sealed interface NostrSignerOp { /** Encrypt a message (NIP-04 or NIP-44). */ data object Encrypt : NostrSignerOp - /** Decrypt a message (NIP-04 or NIP-44). */ + /** Decrypt a message (NIP-04 or NIP-44) from ANYONE — the broad grant. */ data object Decrypt : NostrSignerOp + /** + * Decrypt messages from one [counterparty] only — the narrow alternative to [Decrypt]. + * + * A single "always allow decrypt" grant lets an app read every private conversation the user has, + * forever. Replacing [Decrypt] with this would be worse, though: a DM client would prompt once per + * conversation, training users to approve everything. So this exists *alongside* [Decrypt]: the + * consent dialog offers both, and the authorizer honours a narrow grant when the broad one is + * still ASK. Never auto-granted — only a deliberate "always allow for X" writes one. + */ + data class DecryptFrom( + val counterparty: String, + ) : NostrSignerOp + /** Stable storage key for this operation, used as a DataStore key fragment. */ val key: String get() = @@ -43,6 +56,7 @@ sealed interface NostrSignerOp { is SignKind -> "sign:$kind" Encrypt -> "encrypt" Decrypt -> "decrypt" + is DecryptFrom -> "decrypt:$counterparty" } companion object { @@ -50,6 +64,8 @@ sealed interface NostrSignerOp { when { key == "encrypt" -> Encrypt key == "decrypt" -> Decrypt + // Additive: the broad grant keeps its bare "decrypt" key, so no stored key migrates. + key.startsWith("decrypt:") -> key.removePrefix("decrypt:").ifBlank { null }?.let { DecryptFrom(it) } key.startsWith("sign:") -> key.removePrefix("sign:").toIntOrNull()?.let { SignKind(it) } else -> null } diff --git a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/connectedApps/signers/NostrSignerPermissionLedger.kt b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/connectedApps/signers/NostrSignerPermissionLedger.kt index 9688631c8b..1c7807a640 100644 --- a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/connectedApps/signers/NostrSignerPermissionLedger.kt +++ b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/connectedApps/signers/NostrSignerPermissionLedger.kt @@ -168,7 +168,9 @@ class NostrSignerPermissionLedger( is NostrSignerOp.SignKind -> if (op.kind in REASONABLE_SIGN_KINDS) NostrOpDecision.ALLOW else NostrOpDecision.ASK NostrSignerOp.Encrypt -> NostrOpDecision.ALLOW - NostrSignerOp.Decrypt -> NostrOpDecision.ASK + // Decryption always asks under REASONABLE — both the broad grant and the per-counterparty + // one, which is only ever created by an explicit "always allow for X" in the dialog. + NostrSignerOp.Decrypt, is NostrSignerOp.DecryptFrom -> NostrOpDecision.ASK } companion object { diff --git a/commons/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/connectedApps/nip46/Nip46PermissionAuthorizerTest.kt b/commons/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/connectedApps/nip46/Nip46PermissionAuthorizerTest.kt index 96a51803c7..3ddbafa2ba 100644 --- a/commons/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/connectedApps/nip46/Nip46PermissionAuthorizerTest.kt +++ b/commons/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/connectedApps/nip46/Nip46PermissionAuthorizerTest.kt @@ -97,6 +97,156 @@ class Nip46PermissionAuthorizerTest { assertFalse(authorizer.authorize(client, BunkerRequestNip44Decrypt("2", client, "ct"))) } + // --------------------------------------------------------------------- + // Pairing gate (identity reads) + // --------------------------------------------------------------------- + + @Test + fun isPairedIsFalseBeforeConnectAndTrueAfter() = + runTest { + val ledger = ledger() + val authorizer = Nip46PermissionAuthorizer(ledger, signerPubKey = signer, validateSecret = { _, s -> s == "good" }) + + assertFalse(authorizer.isPaired(client), "a client that never connected must not read the identity") + + authorizer.onConnect(client, BunkerRequestConnect(id = "1", remoteKey = client, secret = "good")) + + assertTrue(authorizer.isPaired(client), "a successful connect pairs the client") + } + + @Test + fun isPairedStaysFalseAfterARejectedConnect() = + runTest { + val ledger = ledger() + val authorizer = Nip46PermissionAuthorizer(ledger, signerPubKey = signer, validateSecret = { _, s -> s == "good" }) + + authorizer.onConnect(client, BunkerRequestConnect(id = "1", remoteKey = client, secret = "wrong")) + + assertFalse(authorizer.isPaired(client)) + } + + @Test + fun forgetUnpairsTheClient() = + runTest { + val ledger = ledger() + val authorizer = Nip46PermissionAuthorizer(ledger, signerPubKey = signer, validateSecret = { _, _ -> true }) + authorizer.onConnect(client, BunkerRequestConnect(id = "1", remoteKey = client, secret = "x")) + + authorizer.forget(client) + + assertFalse(authorizer.isPaired(client)) + } + + // --------------------------------------------------------------------- + // Per-counterparty decrypt grants + // --------------------------------------------------------------------- + + private val alice = "1".repeat(64) + private val bob = "2".repeat(64) + + @Test + fun aPerCounterpartyDecryptGrantAllowsThatConversationWithoutPrompting() = + runTest { + val ledger = ledger() + ledger.setPolicy(coordinate, AppSignerPolicy.REASONABLE) + var prompts = 0 + val authorizer = + Nip46PermissionAuthorizer( + ledger, + signerPubKey = signer, + validateSecret = { _, _ -> true }, + opConsent = { _, _, _, _ -> + prompts++ + SignerOpGrant.DenyOnce + }, + ) + + ledger.setOpDecision(coordinate, NostrSignerOp.DecryptFrom(alice), NostrOpDecision.ALLOW) + + assertTrue(authorizer.authorize(client, BunkerRequestNip44Decrypt("1", alice, "ct"))) + assertEquals(0, prompts, "a standing narrow grant must not re-prompt") + } + + @Test + fun aPerCounterpartyDecryptGrantDoesNotLeakToOtherCounterparties() = + runTest { + val ledger = ledger() + ledger.setPolicy(coordinate, AppSignerPolicy.REASONABLE) + val authorizer = Nip46PermissionAuthorizer(ledger, signerPubKey = signer, validateSecret = { _, _ -> true }) + + ledger.setOpDecision(coordinate, NostrSignerOp.DecryptFrom(alice), NostrOpDecision.ALLOW) + + assertFalse(authorizer.authorize(client, BunkerRequestNip44Decrypt("1", bob, "ct")), "Bob's messages were never granted") + } + + @Test + fun aBroadDecryptDenyIsNotOverriddenByANarrowGrant() = + runTest { + val ledger = ledger() + ledger.setPolicy(coordinate, AppSignerPolicy.REASONABLE) + val authorizer = Nip46PermissionAuthorizer(ledger, signerPubKey = signer, validateSecret = { _, _ -> true }) + + ledger.setOpDecision(coordinate, NostrSignerOp.Decrypt, NostrOpDecision.DENY) + ledger.setOpDecision(coordinate, NostrSignerOp.DecryptFrom(alice), NostrOpDecision.ALLOW) + + assertFalse(authorizer.authorize(client, BunkerRequestNip44Decrypt("1", alice, "ct")), "an explicit broad DENY is final") + } + + @Test + fun aNarrowRememberFromTheDialogIsPersistedAndReusedForThatCounterpartyOnly() = + runTest { + val ledger = ledger() + ledger.setPolicy(coordinate, AppSignerPolicy.REASONABLE) + var prompts = 0 + val authorizer = + Nip46PermissionAuthorizer( + ledger, + signerPubKey = signer, + validateSecret = { _, _ -> true }, + // The dialog's "Always allow for Alice" button returns the NARROW op. + opConsent = { _, _, _, _ -> + prompts++ + SignerOpGrant.AllowForOp(NostrSignerOp.DecryptFrom(alice)) + }, + ) + + assertTrue(authorizer.authorize(client, BunkerRequestNip44Decrypt("1", alice, "ct"))) + assertEquals(1, prompts) + + // Second request from Alice reuses the stored narrow grant. + assertTrue(authorizer.authorize(client, BunkerRequestNip44Decrypt("2", alice, "ct2"))) + assertEquals(1, prompts, "the narrow grant must be remembered") + + // Bob still prompts (and this prompt would grant Alice again, so it is denied). + authorizer.authorize(client, BunkerRequestNip44Decrypt("3", bob, "ct3")) + assertEquals(2, prompts, "a different counterparty is a different decision") + } + + @Test + fun aNarrowSessionGrantIsScopedToItsOwnCounterparty() = + runTest { + val ledger = ledger() + ledger.setPolicy(coordinate, AppSignerPolicy.REASONABLE) + var prompts = 0 + val authorizer = + Nip46PermissionAuthorizer( + ledger, + signerPubKey = signer, + validateSecret = { _, _ -> true }, + opConsent = { _, _, _, _ -> + prompts++ + SignerOpGrant.AllowForSession(NostrSignerOp.DecryptFrom(alice)) + }, + ) + + assertTrue(authorizer.authorize(client, BunkerRequestNip44Decrypt("1", alice, "ct"))) + assertTrue(authorizer.authorize(client, BunkerRequestNip44Decrypt("2", alice, "ct2"))) + assertEquals(1, prompts, "the session grant covers Alice for the rest of the session") + + authorizer.authorize(client, BunkerRequestNip44Decrypt("3", bob, "ct3")) + assertEquals(2, prompts, "and must NOT cover Bob") + } + @Test fun paranoidAppRefusesEverythingUntilPerOpGrant() = runTest { diff --git a/commons/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/connectedApps/signers/NostrSignerPermissionLedgerTest.kt b/commons/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/connectedApps/signers/NostrSignerPermissionLedgerTest.kt index c6a0ed1fc0..87b08de6e1 100644 --- a/commons/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/connectedApps/signers/NostrSignerPermissionLedgerTest.kt +++ b/commons/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/connectedApps/signers/NostrSignerPermissionLedgerTest.kt @@ -106,4 +106,32 @@ class NostrSignerPermissionLedgerTest { val ledger = NostrSignerPermissionLedger(InMemoryNostrSignerPermissionStore()) assertEquals(NostrOpDecision.ASK, ledger.decide(coordinate, NostrSignerOp.SignKind(1))) } + + // --------------------------------------------------------------------- + // Per-counterparty decrypt op + // --------------------------------------------------------------------- + + @Test + fun decryptFromRoundTripsThroughItsStorageKeyWithoutColliding() { + val alice = "1".repeat(64) + val op = NostrSignerOp.DecryptFrom(alice) + + assertEquals("decrypt:$alice", op.key) + // The broad grant keeps its historical bare key, so no stored decision migrates. + assertEquals("decrypt", NostrSignerOp.Decrypt.key) + assertEquals(op, NostrSignerOp.fromKey(op.key)) + assertEquals(NostrSignerOp.Decrypt, NostrSignerOp.fromKey("decrypt")) + assertEquals(null, NostrSignerOp.fromKey("decrypt:")) + } + + @Test + fun decryptFromAlwaysAsksUnderReasonableSoItIsOnlyEverGrantedExplicitly() = + runTest { + val store = InMemoryNostrSignerPermissionStore() + val ledger = NostrSignerPermissionLedger(store) + val coordinate = "nip46:a:b" + ledger.setPolicy(coordinate, AppSignerPolicy.REASONABLE) + + assertEquals(NostrOpDecision.ASK, ledger.decide(coordinate, NostrSignerOp.DecryptFrom("1".repeat(64)))) + } } diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip46RemoteSigner/server/BunkerRequestProcessor.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip46RemoteSigner/server/BunkerRequestProcessor.kt index 973f288b45..2328aa3d5c 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip46RemoteSigner/server/BunkerRequestProcessor.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip46RemoteSigner/server/BunkerRequestProcessor.kt @@ -59,12 +59,23 @@ import kotlinx.coroutines.sync.withLock * [NostrSigner] surface (`sign`, `nip04/44Encrypt/Decrypt`). Whichever signer * the user logged in with is the one that ultimately performs the work. * - * Authorization is delegated to [authorizer]: signing/encryption/decryption are - * gated, while public/harmless reads (`get_public_key`, `ping`, `get_relays`) - * always succeed. All failures — decryption, authorization, an unsupported - * method, or an exception from the signer — are turned into a - * [BunkerResponseError] carrying the request id, so the client always gets a - * reply it can correlate. + * Authorization is delegated to [authorizer] in two layers: + * - **pairing** ([Nip46RequestAuthorizer.isPaired]) gates the identity reads + * `get_public_key` and `get_relays`. Anyone who obtains the `bunker://` URI + * holds the transport pubkey and the NIP-44 conversation key, so without this + * gate they could ask an unpaired signer *which Nostr account it signs for* + * (and its inbox relay set) without ever knowing the pairing secret — which + * would defeat the transport/identity split the rest of the design maintains. + * `connect` is deliberately NOT gated (it is how pairing happens), and neither + * is `ping`: it only confirms that a signer is alive at a pubkey the caller + * already has, leaks no identity, and clients use it as a pre-flight liveness + * check, so gating it would risk breaking legitimate handshakes for no gain. + * - **per-operation consent** ([Nip46RequestAuthorizer.authorize]) gates + * signing/encryption/decryption. + * + * All failures — decryption, authorization, an unsupported method, or an + * exception from the signer — are turned into a [BunkerResponseError] carrying + * the request id, so the client always gets a reply it can correlate. * * Pairs with [NostrConnectSignerService], which subscribes to the relays, * decrypts each kind-24133 request, calls [process], and publishes the reply. @@ -100,12 +111,20 @@ class BunkerRequestProcessor( is Nip46ConnectDecision.Reject -> BunkerResponseError(request.id, decision.reason) } - is BunkerRequestGetPublicKey -> BunkerResponsePublicKey(request.id, signer.pubKey) + // Identity reads: only for a client that has already paired. See the class doc — an + // unpaired holder of the bunker URI must not be able to learn WHICH account this is. + is BunkerRequestGetPublicKey -> + ifPaired(clientPubKey, request) { + BunkerResponsePublicKey(request.id, signer.pubKey) + } + // Liveness only; answers with no identity at all, so it stays open (see class doc). is BunkerRequestPing -> BunkerResponsePong(request.id) is BunkerRequestGetRelays -> - BunkerResponseGetRelays(request.id, relays().associate { it.url to ReadWrite(read = true, write = true) }) + ifPaired(clientPubKey, request) { + BunkerResponseGetRelays(request.id, relays().associate { it.url to ReadWrite(read = true, write = true) }) + } is BunkerRequestSign -> ifAuthorized(clientPubKey, request) { @@ -148,6 +167,22 @@ class BunkerRequestProcessor( BunkerResponseError(request.id, "${e::class.simpleName}: ${e.message}") } + /** + * Runs [block] only when [clientPubKey] has already paired with this signer (a successful + * `connect`). Used for the identity reads, which need no per-op consent but must not answer a + * stranger who merely holds the bunker URI. + */ + private suspend inline fun ifPaired( + clientPubKey: HexKey, + request: BunkerRequest, + block: () -> BunkerResponse, + ): BunkerResponse = + if (authorizer.isPaired(clientPubKey)) { + block() + } else { + BunkerResponseError(request.id, ERROR_NOT_CONNECTED) + } + private suspend inline fun ifAuthorized( clientPubKey: HexKey, request: BunkerRequest, @@ -172,6 +207,13 @@ class BunkerRequestProcessor( /** Error result returned when [Nip46RequestAuthorizer.authorize] denies a request. */ const val ERROR_UNAUTHORIZED: String = "unauthorized" + /** + * Error result returned to a client that has not paired ([Nip46RequestAuthorizer.isPaired]) + * when it asks for the signer's identity (`get_public_key`, `get_relays`). It must not reveal + * whether the account exists, so it says only that this client is not connected. + */ + const val ERROR_NOT_CONNECTED: String = "not connected" + /** Error result returned when the account can no longer sign (logged out / read-only / no signer). */ const val ERROR_ACCOUNT_UNAVAILABLE: String = "account unavailable" diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip46RemoteSigner/server/Nip46RequestAuthorizer.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip46RemoteSigner/server/Nip46RequestAuthorizer.kt index 670208581b..1ee44400b9 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip46RemoteSigner/server/Nip46RequestAuthorizer.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip46RemoteSigner/server/Nip46RequestAuthorizer.kt @@ -35,10 +35,26 @@ import com.vitorpamplona.quartz.nip46RemoteSigner.BunkerRequestConnect * client do this?". Everything here is `suspend` so an implementation may block * on disk, a live user prompt, or IPC without changing the processor. * - * Public, harmless requests (`get_public_key`, `ping`, `get_relays`) are NOT - * routed through [authorize]; only signing, encryption and decryption are. + * Two gates, not one: + * - [isPaired] answers "has this client completed a `connect`?" and guards the + * identity reads (`get_public_key`, `get_relays`) — they need no per-op + * consent, but they must not answer a stranger who merely obtained the + * `bunker://` URI, since the answer names the user's account. `ping` is not + * guarded (pure liveness), and `connect` obviously cannot be. + * - [authorize] gates signing, encryption and decryption. */ interface Nip46RequestAuthorizer { + /** + * `true` when [clientPubKey] has already paired with this signer — i.e. a `connect` succeeded + * and the client holds a standing grant. Guards the identity reads (`get_public_key`, + * `get_relays`), which would otherwise tell any holder of the bunker URI *which Nostr account* + * this signer belongs to without ever presenting the pairing secret. + * + * Deliberately has no default: every implementation must state its own pairing rule, so a new + * authorizer cannot silently inherit "everyone is paired". + */ + suspend fun isPaired(clientPubKey: HexKey): Boolean + /** * Called when a client sends a `connect` request. The implementation * validates the offered secret (the `bunker://…?secret=…` pairing token), diff --git a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip46RemoteSigner/server/BunkerRequestProcessorConcurrencyTest.kt b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip46RemoteSigner/server/BunkerRequestProcessorConcurrencyTest.kt index f64a4fc7a6..7ee9fdc303 100644 --- a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip46RemoteSigner/server/BunkerRequestProcessorConcurrencyTest.kt +++ b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip46RemoteSigner/server/BunkerRequestProcessorConcurrencyTest.kt @@ -114,6 +114,8 @@ class BunkerRequestProcessorConcurrencyTest { private class GatedAuthorizer( val gateFor: (BunkerRequest) -> CompletableDeferred?, ) : Nip46RequestAuthorizer { + override suspend fun isPaired(clientPubKey: HexKey) = true + override suspend fun onConnect( clientPubKey: HexKey, request: BunkerRequestConnect, diff --git a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip46RemoteSigner/server/BunkerRequestProcessorTest.kt b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip46RemoteSigner/server/BunkerRequestProcessorTest.kt index fcfb8a711b..3533bb2959 100644 --- a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip46RemoteSigner/server/BunkerRequestProcessorTest.kt +++ b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip46RemoteSigner/server/BunkerRequestProcessorTest.kt @@ -45,6 +45,7 @@ import com.vitorpamplona.quartz.nip57Zaps.LnZapRequestEvent import kotlinx.coroutines.test.runTest import kotlin.test.Test import kotlin.test.assertEquals +import kotlin.test.assertNull import kotlin.test.assertTrue /** @@ -128,9 +129,16 @@ class BunkerRequestProcessorTest { private class FakeAuthorizer( val connectDecision: Nip46ConnectDecision, val allow: Boolean, + val paired: Boolean = true, ) : Nip46RequestAuthorizer { var connectCalls = 0 var authorizeCalls = 0 + var isPairedCalls = 0 + + override suspend fun isPaired(clientPubKey: HexKey): Boolean { + isPairedCalls++ + return paired + } override suspend fun onConnect( clientPubKey: HexKey, @@ -157,34 +165,86 @@ class BunkerRequestProcessorTest { ) = BunkerRequestProcessor(signer, { setOf(relay) }, authorizer) @Test - fun getPublicKeyReturnsUserPubKeyWithoutAuthorization() = + fun getPublicKeyReturnsUserPubKeyForAPairedClientWithoutPerOpAuthorization() = runTest { - val authorizer = FakeAuthorizer(Nip46ConnectDecision.Accept("ack"), allow = false) + // Paired but every op denied: the identity read still answers — it needs pairing, not consent. + val authorizer = FakeAuthorizer(Nip46ConnectDecision.Accept("ack"), allow = false, paired = true) val res = processor(authorizer = authorizer).process(clientPubKey, BunkerRequestGetPublicKey("1")) assertTrue(res is BunkerResponsePublicKey) assertEquals(userPubKey, res.pubkey) assertEquals("1", res.id) - // public reads are never gated + // gated on pairing only, never on per-op consent assertEquals(0, authorizer.authorizeCalls) + assertEquals(1, authorizer.isPairedCalls) } + /** + * The identity leak: anyone who obtains the `bunker://` URI can encrypt a well-formed request + * without knowing the secret. `get_public_key` must not tell them WHICH account this signer is. + */ @Test - fun pingReturnsPong() = + fun getPublicKeyIsRefusedForAnUnpairedClient() = runTest { - val res = processor().process(clientPubKey, BunkerRequestPing("2")) + val authorizer = FakeAuthorizer(Nip46ConnectDecision.Accept("ack"), allow = true, paired = false) + val res = processor(authorizer = authorizer).process(clientPubKey, BunkerRequestGetPublicKey("1")) + + assertTrue(res is BunkerResponseError, "unpaired get_public_key must error, got $res") + assertEquals(BunkerRequestProcessor.ERROR_NOT_CONNECTED, res.error) + assertEquals("1", res.id) + assertNull(res.result, "the error reply must carry no identity result") + } + + /** + * `ping` stays open on purpose: it only confirms a signer is alive at a pubkey the caller already + * has (it is in the URI they hold) and leaks no identity, so gating it would risk breaking a + * legitimate pre-connect liveness check for no privacy gain. Pinned so the choice is deliberate. + */ + @Test + fun pingReturnsPongEvenForAnUnpairedClient() = + runTest { + val authorizer = FakeAuthorizer(Nip46ConnectDecision.Accept("ack"), allow = false, paired = false) + val res = processor(authorizer = authorizer).process(clientPubKey, BunkerRequestPing("2")) assertTrue(res is BunkerResponsePong) assertEquals("2", res.id) + assertEquals(0, authorizer.isPairedCalls, "ping is deliberately not pairing-gated") } @Test - fun getRelaysReturnsConfiguredRelays() = + fun getRelaysReturnsConfiguredRelaysForAPairedClient() = runTest { val res = processor().process(clientPubKey, BunkerRequestGetRelays("3")) assertTrue(res is BunkerResponseGetRelays) assertTrue(res.relays.containsKey(relay.url)) } + /** `get_relays` additionally yields the user's inbox relay set — same pairing gate. */ + @Test + fun getRelaysIsRefusedForAnUnpairedClient() = + runTest { + val authorizer = FakeAuthorizer(Nip46ConnectDecision.Accept("ack"), allow = true, paired = false) + val res = processor(authorizer = authorizer).process(clientPubKey, BunkerRequestGetRelays("3")) + + assertTrue(res is BunkerResponseError, "unpaired get_relays must error, got $res") + assertEquals(BunkerRequestProcessor.ERROR_NOT_CONNECTED, res.error) + assertNull(res.result, "the error reply must carry no relay set") + } + + /** + * The regression that would break ALL pairing: `connect` is how a client becomes paired, so it + * must stay reachable to a client that is not paired yet. + */ + @Test + fun connectStillWorksForAnUnpairedClient() = + runTest { + val authorizer = FakeAuthorizer(Nip46ConnectDecision.Accept("s3cr3t"), allow = false, paired = false) + val res = processor(authorizer = authorizer).process(clientPubKey, BunkerRequestConnect(id = "9", remoteKey = userPubKey, secret = "s3cr3t")) + + assertEquals(1, authorizer.connectCalls) + assertTrue(res !is BunkerResponseError, "connect must not be pairing-gated, got $res") + assertEquals("s3cr3t", res.result) + } + @Test fun connectAcceptEchoesSecret() = runTest { diff --git a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip46RemoteSigner/server/NostrConnectSignerServiceTest.kt b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip46RemoteSigner/server/NostrConnectSignerServiceTest.kt index 98a7dcff66..9f3ac113f1 100644 --- a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip46RemoteSigner/server/NostrConnectSignerServiceTest.kt +++ b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip46RemoteSigner/server/NostrConnectSignerServiceTest.kt @@ -147,6 +147,8 @@ class NostrConnectSignerServiceTest { private class AllowAuthorizer : Nip46RequestAuthorizer { var logoutCalls = 0 + override suspend fun isPaired(clientPubKey: HexKey) = true + override suspend fun onConnect( clientPubKey: HexKey, request: BunkerRequestConnect,