From 985b4c2ea35a1c87daa2fc16b42165d52790a4f0 Mon Sep 17 00:00:00 2001 From: davotoula Date: Sun, 30 Aug 2026 18:17:35 +0200 Subject: [PATCH] fix(nwc): only claim a binding for a zap request the provider accepted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Kotlin review found the feature's own soundness property could be false on the wire. lnAddressInvoice drops the zap request for a provider that does not advertise `allowsNostr` — `nostrRequest = if (allowsNostr) nostrRequest else null` — but assembleInvoice set Payable.zapRequest unconditionally from the request it had built. So paying a lightning address whose provider ignores `nostr=` still attached metadata.nostr to the payment, for an invoice whose description_hash commits to nothing about it. Every claim the feature makes — the KDoc, the byte-identity test, the wallet-side binding check we asked BrollyZapper to keep strict — rests on those bytes being what the callback hashed. Here they were not. A conformant wallet refuses such a row, so no false attribution was displayed; what was wrong is that we asserted a binding we could not support, and spent the 4096-char budget doing it. lnAddressInvoice now reports the request it actually sent, and only that is carried forward. The size estimate also counted raw string length for `comment`, which is free text a user typed. JSON escaping expands it — a quote or backslash to two characters, a control character to six — so an escaping-heavy comment could breach the ceiling unnoticed, and NWC-06 makes the wallet drop the WHOLE object then, taking recipient_data with it. escapedLength() counts what actually reaches the wire; KEY_OVERHEAD drops to the fixed punctuation cost now that escaping is no longer hiding inside it. Both paths were untested and now have regression tests. Not changed: dropMetadataIfUnsupported still mutates the caller's Request. The review confirmed every current call site builds a fresh request inline, and the contract is documented on both public send functions. Verified: quartz + amethyst suites, commons/desktopApp/cli/geode compile, spotless clean. --- .../amethyst/service/ZapPaymentHandler.kt | 7 +++- .../service/lnurl/LightningAddressResolver.kt | 17 +++++++++- .../rpc/NwcTransactionMetadata.kt | 34 +++++++++++++++---- .../NwcOutgoingMetadataTest.kt | 34 +++++++++++++++++++ 4 files changed, 83 insertions(+), 9 deletions(-) diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/ZapPaymentHandler.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/ZapPaymentHandler.kt index 738411e97b..ace32c51b3 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/ZapPaymentHandler.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/ZapPaymentHandler.kt @@ -567,6 +567,10 @@ class ZapPaymentHandler( ): Payable { var progressThisPayment = 0.00f + // Only the request the provider actually accepted may be claimed as bound to + // this invoice; see lnAddressInvoice's onZapRequestSent. + var sentZapRequest: LnZapRequestEvent? = null + val invoice = LightningAddressResolver().lnAddressInvoice( lnAddress = lud16, @@ -580,6 +584,7 @@ class ZapPaymentHandler( onProgressStep(step) }, context = context, + onZapRequestSent = { sentZapRequest = it }, ) onProgressStep(1 - progressThisPayment) @@ -588,7 +593,7 @@ class ZapPaymentHandler( info = splitSetup, amountMilliSats = zapValue, invoice = invoice, - zapRequest = nostrZapRequest, + zapRequest = sentZapRequest, message = message, ) } diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/lnurl/LightningAddressResolver.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/lnurl/LightningAddressResolver.kt index b0ff223cac..2ed7dce908 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/lnurl/LightningAddressResolver.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/lnurl/LightningAddressResolver.kt @@ -201,6 +201,13 @@ class LightningAddressResolver { ?: response.code.toString() } + /** + * @param onZapRequestSent receives the zap request that was ACTUALLY sent to the + * callback, or null when it was not. A provider that does not advertise + * `allowsNostr` never sees [nostrRequest], and its invoice therefore commits to + * nothing about it — so a caller must not go on to claim the two are bound. See + * the drop below. + */ suspend fun lnAddressInvoice( lnAddress: String, milliSats: Long, @@ -209,6 +216,7 @@ class LightningAddressResolver { okHttpClient: (String) -> OkHttpClient, onProgress: (percent: Float) -> Unit, context: Context, + onZapRequestSent: (LnZapRequestEvent?) -> Unit = {}, ): String { val mapper = jacksonObjectMapper() @@ -264,12 +272,19 @@ class LightningAddressResolver { ) } + // NIP-57 binds a zap request to its invoice through `description_hash`, and a + // provider that ignores `nostr=` mints an invoice that commits to nothing about + // it. Report what actually went, so a caller cannot attach the event to a + // payment it was never bound to. + val sentZapRequest = nostrRequest?.takeIf { allowsNostr } + onZapRequestSent(sentZapRequest) + val invoice = fetchLightningInvoice( lnCallback = callbackUrl, milliSats = milliSats, message = message, - nostrRequest = if (allowsNostr) nostrRequest else null, + nostrRequest = sentZapRequest, okHttpClient = okHttpClient, context = context, ) diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip47WalletConnect/rpc/NwcTransactionMetadata.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip47WalletConnect/rpc/NwcTransactionMetadata.kt index 16a55c90a6..c95e636604 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip47WalletConnect/rpc/NwcTransactionMetadata.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip47WalletConnect/rpc/NwcTransactionMetadata.kt @@ -127,12 +127,32 @@ class NwcTransactionMetadata( */ const val MAX_METADATA_CHARS = 4096 - // The keys and punctuation around the values — - // `{"recipient_data":{"identifier":""},"comment":"","nostr":}` is 58 chars — - // plus room for JSON escaping to expand `identifier` and `comment` on the way - // out, which is the only thing that can make this estimate low. `nostr` needs - // no such allowance: the length added below is the serialized string itself. - private const val KEY_OVERHEAD = 96 + // The keys and punctuation around the values: + // `{"recipient_data":{"identifier":""},"comment":"","nostr":}` is 58 chars. + // Escaping is counted separately by [escapedLength], so this is a fixed cost. + private const val KEY_OVERHEAD = 64 + + /** + * The length a string occupies once JSON-escaped. + * + * `comment` is free text a user typed, so its raw length is not what reaches + * the wire: a quote or backslash becomes two characters and a control + * character becomes six. Counting the raw length instead would let an + * escaping-heavy comment breach [MAX_METADATA_CHARS] unnoticed — and NWC-06 + * makes the wallet drop the WHOLE object then, losing `recipient_data` too, + * which is the degradation this budget exists to protect. + * + * Over-counts the short forms (`\n` is two characters, not six), which is + * the safe direction. + */ + private fun escapedLength(value: String): Int = + value.sumOf { char -> + when { + char == '"' || char == '\\' -> 2 + char < ' ' -> 6 + else -> 1 + } + } /** * Assembles NWC-06 `metadata` for an outgoing payment, or null when there is @@ -175,7 +195,7 @@ class NwcTransactionMetadata( // string that gets embedded, not a reconstruction of it. val raw = zapRequest.toJson() val chars = - recipientIdentifier.orEmpty().length + comment.orEmpty().length + + escapedLength(recipientIdentifier.orEmpty()) + escapedLength(comment.orEmpty()) + raw.length + KEY_OVERHEAD if (chars <= MAX_METADATA_CHARS) { lean["nostr"] = RawJson(raw) diff --git a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip47WalletConnect/NwcOutgoingMetadataTest.kt b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip47WalletConnect/NwcOutgoingMetadataTest.kt index 830287b80b..1a20c1987c 100644 --- a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip47WalletConnect/NwcOutgoingMetadataTest.kt +++ b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip47WalletConnect/NwcOutgoingMetadataTest.kt @@ -121,6 +121,40 @@ class NwcOutgoingMetadataTest { assertEquals("hi", meta["comment"]) } + /** + * A provider that does not advertise `allowsNostr` never receives the zap request, + * so its invoice commits to nothing about it. Passing null here is how the caller + * says so, and the metadata must then make no claim it cannot support — while the + * payee's address still labels the row. + */ + @Test + fun withoutAZapRequestTheRowIsStillNamedButClaimsNoBinding() { + val meta = assertNotNull(NwcTransactionMetadata.build(null, "user@domain.com", "great post")) + + assertFalse(meta.containsKey("nostr")) + assertEquals(mapOf("identifier" to "user@domain.com"), meta["recipient_data"]) + assertEquals("great post", meta["comment"]) + } + + /** + * `comment` is free text, and JSON escaping expands it on the way out. Counting the + * raw length would let an escaping-heavy comment breach the 4096 ceiling unnoticed + * — and NWC-06 makes the wallet drop the WHOLE object then, taking `recipient_data` + * with it. + */ + @Test + fun theCeilingCountsEscapedLengthNotRawLength() { + // Every character escapes to six, so 900 raw chars occupy ~5400 on the wire. + val controlHeavy = "\u0001".repeat(900) + val meta = assertNotNull(NwcTransactionMetadata.build(zapRequest(), "user@domain.com", controlHeavy)) + + assertFalse(meta.containsKey("nostr"), "the escaped comment alone exceeds the ceiling") + + // The same length in plain characters leaves room for the zap request. + val plain = "a".repeat(900) + assertTrue(assertNotNull(NwcTransactionMetadata.build(zapRequest(), "user@domain.com", plain)).containsKey("nostr")) + } + @Test fun whatWeSendStaysUnderTheSpecCeiling() { val meta = NwcTransactionMetadata.build(zapRequest(), "user@domain.com", "great post")