From af50a4df6b44d65ead2ae1468a722cc038bdaf0c Mon Sep 17 00:00:00 2001 From: Philipp Walter Date: Fri, 9 Oct 2026 14:10:31 +0200 Subject: [PATCH] fix: adopt core bip21 validation --- .../to/bitkit/services/Bip21DecodingTest.kt | 54 ++++++++++++ .../main/java/to/bitkit/utils/Bip21Utils.kt | 16 ---- .../java/to/bitkit/viewmodels/AppViewModel.kt | 23 ----- .../to/bitkit/utils/Bip21UrlBuilderTest.kt | 44 ---------- .../viewmodels/AppViewModelSendFlowTest.kt | 84 ++++++++++++++++++- changelog.d/next/1454.fixed.md | 1 + journeys/send/README.md | 11 +++ journeys/send/bip21-core-validation.xml | 17 ++++ 8 files changed, 166 insertions(+), 84 deletions(-) create mode 100644 app/src/androidTest/java/to/bitkit/services/Bip21DecodingTest.kt create mode 100644 changelog.d/next/1454.fixed.md create mode 100644 journeys/send/README.md create mode 100644 journeys/send/bip21-core-validation.xml diff --git a/app/src/androidTest/java/to/bitkit/services/Bip21DecodingTest.kt b/app/src/androidTest/java/to/bitkit/services/Bip21DecodingTest.kt new file mode 100644 index 0000000000..b95287a20a --- /dev/null +++ b/app/src/androidTest/java/to/bitkit/services/Bip21DecodingTest.kt @@ -0,0 +1,54 @@ +package to.bitkit.services + +import androidx.test.ext.junit.runners.AndroidJUnit4 +import com.synonym.bitkitcore.Scanner +import com.synonym.bitkitcore.decode +import org.junit.Test +import org.junit.runner.RunWith +import to.bitkit.ext.runSuspendCatching +import to.bitkit.test.BaseAndroidTest +import to.bitkit.test.annotations.CoreServiceIntegration +import to.bitkit.test.annotations.DeviceIntegration +import kotlin.test.assertEquals +import kotlin.test.assertIs +import kotlin.test.assertTrue + +@RunWith(AndroidJUnit4::class) +@DeviceIntegration +@CoreServiceIntegration +class Bip21DecodingTest : BaseAndroidTest() { + companion object { + /** Valid regtest recipient from the bitkit-core regression fixture. */ + private const val ADDRESS = "bcrt1qr289x0fhg62672e8urudfnxnsr8tcax64xk2vk" + } + + @Test + fun coreRejectsDuplicateSingletonParameters() = test { + val duplicatedUri = "bitcoin:$ADDRESS?amount=0.0000002&message=Bitkit" + + "bitcoin:$ADDRESS?amount=0.0000003&message=Bitkit" + for (uri in listOf( + duplicatedUri, + "bitcoin:$ADDRESS?amount=0.000035&AMOUNT=0.00005", + "bitcoin:$ADDRESS?label=first&LABEL=second", + "bitcoin:$ADDRESS?message=first&Message=second", + "bitcoin:$ADDRESS?pop=callback%3a&req-pop=callback%3a", + )) { + assertTrue(runSuspendCatching { decode(uri) }.isFailure, uri) + } + } + + @Test + fun corePreservesBitcoinTextInQueryMetadata() = test { + for ((key, value) in listOf( + "message" to "bitcoin:donation", + "label" to "BITCOIN:donation", + "custom" to "bitcoin:1BoatSLRHtKNngkdXEeobR76b53LETtpyT", + "message" to "Why?", + )) { + val decoded = assertIs(decode("bitcoin:$ADDRESS?amount=0.000035&$key=$value")) + assertEquals(ADDRESS, decoded.invoice.address) + assertEquals(3500uL, decoded.invoice.amountSatoshis) + assertEquals(value, decoded.invoice.params?.get(key)) + } + } +} diff --git a/app/src/main/java/to/bitkit/utils/Bip21Utils.kt b/app/src/main/java/to/bitkit/utils/Bip21Utils.kt index 9c6cf8cda9..2cd34df44f 100644 --- a/app/src/main/java/to/bitkit/utils/Bip21Utils.kt +++ b/app/src/main/java/to/bitkit/utils/Bip21Utils.kt @@ -4,22 +4,6 @@ import to.bitkit.models.SATS_IN_BTC object Bip21Utils { - private const val BIP21_PREFIX = "bitcoin:" - - /** - * Checks if a BIP21 URI is duplicated (contains multiple bitcoin: prefixes). - * Workaround for https://github.com/synonymdev/bitkit-core/issues/63 - * @return true if the input contains duplicated BIP21 URIs, false otherwise - */ - fun isDuplicatedBip21(input: String): Boolean { - val lowercased = input.lowercase() - val firstIndex = lowercased.indexOf(BIP21_PREFIX) - if (firstIndex == -1) return false - - val secondIndex = lowercased.indexOf(BIP21_PREFIX, firstIndex + BIP21_PREFIX.length) - return secondIndex != -1 - } - fun buildBip21Url( bitcoinAddress: String, amountSats: ULong? = null, diff --git a/app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt b/app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt index 9bb6e638ee..310a9d0391 100644 --- a/app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt +++ b/app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt @@ -208,7 +208,6 @@ import to.bitkit.ui.utils.localizedPubkyAuthMessage import to.bitkit.usecases.FormatMoneyValue import to.bitkit.usecases.RefreshContactPaykitLinkUseCase import to.bitkit.utils.AppError -import to.bitkit.utils.Bip21Utils import to.bitkit.utils.Logger import to.bitkit.utils.NetworkValidationHelper import to.bitkit.utils.ServiceError @@ -2441,16 +2440,6 @@ class AppViewModel @Inject constructor( } private suspend fun validateAddressWithFeedback(input: String) = withContext(bgDispatcher) { - // TODO Workaround for https://github.com/synonymdev/bitkit-core/issues/63 - if (Bip21Utils.isDuplicatedBip21(input)) { - showAddressValidationError( - titleRes = R.string.other__scan_err_decoding, - descriptionRes = R.string.other__scan__error__generic, - testTag = "DuplicatedBip21Toast", - ) - return@withContext - } - val scanResult = runCatching { coreService.decode(input.removeLightningSchemes()) } if (scanResult.isFailure) { @@ -3361,18 +3350,6 @@ class AppViewModel @Inject constructor( val fromMainScanner = isMainScanner - // TODO Workaround for https://github.com/synonymdev/bitkit-core/issues/63 - if (Bip21Utils.isDuplicatedBip21(input)) { - if (clearIncomingPaymentRequestTarget()) return@withContext - toast( - type = Toast.ToastType.ERROR, - title = context.getString(R.string.other__scan_err_decoding), - description = context.getString(R.string.other__scan__error__generic), - testTag = "DuplicatedBip21Toast", - ) - return@withContext - } - SamRockSetupRequest.parse(input)?.let { handleSamRockSetup(it) return@withContext diff --git a/app/src/test/java/to/bitkit/utils/Bip21UrlBuilderTest.kt b/app/src/test/java/to/bitkit/utils/Bip21UrlBuilderTest.kt index e61d062e3e..252305d43a 100644 --- a/app/src/test/java/to/bitkit/utils/Bip21UrlBuilderTest.kt +++ b/app/src/test/java/to/bitkit/utils/Bip21UrlBuilderTest.kt @@ -5,7 +5,6 @@ import org.junit.Test import org.junit.runner.RunWith import org.junit.runners.JUnit4 import to.bitkit.utils.Bip21Utils.buildBip21Url -import to.bitkit.utils.Bip21Utils.isDuplicatedBip21 @RunWith(JUnit4::class) class Bip21UrlBuilderTest { @@ -173,49 +172,6 @@ class Bip21UrlBuilderTest { Assert.assertEquals(expected, buildBip21Url(address, amount, lightningInvoice = invoice)) } - // Tests for isDuplicatedBip21 - Workaround for bitkit-core#63 - - @Test - fun `isDuplicatedBip21 returns false for single valid BIP21 URI`() { - val input = "bitcoin:bc1qar0srrr7xfkvy5l643lydnw9re59gtzzwf5mdq?amount=0.001&message=Bitkit" - Assert.assertFalse(isDuplicatedBip21(input)) - } - - @Test - fun `isDuplicatedBip21 returns true when BIP21 URI is duplicated`() { - val first = "bitcoin:bcrt1qr289x0fhg62672e8urudfnxnsr8tcax64xk2vk?amount=0.0000002&message=Bitkit" - val second = "bitcoin:bcrt1qr289x0fhg62672e8urudfnxnsr8tcax64xk2vk?amount=0.0000003&message=Bitkit" - val input = first + second - Assert.assertTrue(isDuplicatedBip21(input)) - } - - @Test - fun `isDuplicatedBip21 handles case-insensitive bitcoin prefix`() { - val first = "BITCOIN:bc1qar0srrr7xfkvy5l643lydnw9re59gtzzwf5mdq?amount=0.001" - val second = "bitcoin:bc1qar0srrr7xfkvy5l643lydnw9re59gtzzwf5mdq?amount=0.002" - val input = first + second - Assert.assertTrue(isDuplicatedBip21(input)) - } - - @Test - fun `isDuplicatedBip21 returns false for non-bitcoin URIs`() { - val input = "lnbc500n1p3k9v3pp5kzmj..." - Assert.assertFalse(isDuplicatedBip21(input)) - } - - @Test - fun `isDuplicatedBip21 returns false for empty string`() { - Assert.assertFalse(isDuplicatedBip21("")) - } - - @Test - fun `isDuplicatedBip21 handles mixed case duplicated URIs`() { - val first = "Bitcoin:bc1qaddr1?amount=0.001" - val second = "BITCOIN:bc1qaddr2?amount=0.002" - val input = first + second - Assert.assertTrue(isDuplicatedBip21(input)) - } - // Tests for empty/blank message and label handling - Issue #746 @Test diff --git a/app/src/test/java/to/bitkit/viewmodels/AppViewModelSendFlowTest.kt b/app/src/test/java/to/bitkit/viewmodels/AppViewModelSendFlowTest.kt index 83192292e1..e78fb672af 100644 --- a/app/src/test/java/to/bitkit/viewmodels/AppViewModelSendFlowTest.kt +++ b/app/src/test/java/to/bitkit/viewmodels/AppViewModelSendFlowTest.kt @@ -2196,11 +2196,12 @@ class AppViewModelSendFlowTest : BaseUnitTest() { } @Test - fun `duplicated bip21 request target leaves terminal feedback visible`() = test { + fun `core rejected bip21 request target leaves terminal feedback visible`() = test { sut.setIsAuthenticated(true) val request = paymentRequest() val first = "bitcoin:bcrt1qfirst?amount=0.00000001" val duplicatedBip21 = first + "bitcoin:bcrt1qsecond?amount=0.00000001" + whenever(coreService.decode(duplicatedBip21)).doSuspendableAnswer { throw AppError("Invalid payment URI") } whenever(context.getString(R.string.wallet__payment_request)).thenReturn("Payment Request") whenever(context.getString(R.string.wallet__payment_request_waiting_for_details)).thenReturn("Waiting") whenever(context.getString(R.string.wallet__payment_request_unavailable)).thenReturn( @@ -6649,6 +6650,65 @@ class AppViewModelSendFlowTest : BaseUnitTest() { assertEquals("InsufficientSavingsToast", toastCaptor.lastValue.testTag) } + @Test + fun `manual bip21 with bitcoin text delegates to core and stays valid`() = test { + val uri = "bitcoin:$REGTEST_ADDRESS?amount=0.000035&message=bitcoin:donation" + stubBip21Scan(uri) + + sut.setSendEvent(SendEvent.AddressChange(uri)) + advanceUntilIdle() + + verify(coreService).decode(uri) + assertTrue(sut.sendUiState.value.isAddressInputValid) + assertTrue(sut.canDecodeClipboard(uri)) + } + + @Test + fun `manual bip21 rejected by core stays invalid`() = test { + val uri = "bitcoin:$REGTEST_ADDRESS?amount=0.000035&amount=0.00005" + whenever(coreService.decode(uri)).doSuspendableAnswer { throw AppError("Invalid payment URI") } + clearInvocations(toastManager) + + sut.setSendEvent(SendEvent.AddressChange(uri)) + advanceUntilIdle() + + verify(coreService).decode(uri) + assertFalse(sut.sendUiState.value.isAddressInputValid) + verify(toastManager).enqueue(check { assertEquals("InvalidAddressToast", it.testTag) }) + } + + @Test + fun `scanned bip21 with bitcoin text reaches send state`() = test { + val uri = "bitcoin:$REGTEST_ADDRESS?amount=0.000035&message=bitcoin:donation" + stubBip21Scan(uri) + sut.setIsAuthenticated(true) + + sut.onScanResult(uri) + advanceUntilIdle() + + verify(coreService).decode(uri) + assertEquals(REGTEST_ADDRESS, sut.sendUiState.value.address) + assertEquals(uri, sut.sendUiState.value.addressInput) + assertEquals(3500uL, sut.sendUiState.value.amount) + } + + @Test + fun `scanned concatenated bip21 rejected by core clears previous payment`() = test { + val first = "bitcoin:$REGTEST_ADDRESS?amount=0.000035&message=Bitkit" + val uri = first + "bitcoin:$REGTEST_ADDRESS?amount=0.00005&message=Bitkit" + whenever(coreService.decode(uri)).doSuspendableAnswer { throw AppError("Invalid payment URI") } + setSendState(SendUiState(address = REGTEST_ADDRESS, amount = 3500uL)) + sut.setIsAuthenticated(true) + + sut.onScanResult(uri) + advanceUntilIdle() + + verify(coreService).decode(uri) + assertEquals("", sut.sendUiState.value.address) + assertEquals(0uL, sut.sendUiState.value.amount) + assertNull(sut.currentSheet.value) + } + @Test fun `manual input of unified own lightning invoice with no savings shows self payment toast`() = test { val bolt11 = "lnbcrt1ownunifiedmanual" @@ -10326,6 +10386,28 @@ class AppViewModelSendFlowTest : BaseUnitTest() { stubLightningScan(bolt11 = bolt11, amountSats = amountSats, payeeNodeId = payeeNodeId) } + private suspend fun stubBip21Scan(uri: String) { + balanceState.value = BalanceState(maxSendOnchainSats = 100_000uL) + whenever { coreService.decode(uri) }.thenReturn( + Scanner.OnChain( + OnChainInvoice( + address = REGTEST_ADDRESS, + amountSatoshis = 3500uL, + label = null, + message = "bitcoin:donation", + params = mapOf("message" to "bitcoin:donation"), + ) + ) + ) + whenever(coreService.validateBitcoinAddress(REGTEST_ADDRESS)).thenReturn( + ValidationResult( + address = REGTEST_ADDRESS, + network = NetworkType.REGTEST, + addressType = AddressType.P2WPKH, + ) + ) + } + private fun nonOnchainPaymentScans() = listOf( "lnbcrt1hardware" to Scanner.Lightning(lightningInvoice("lnbcrt1hardware", 1_000uL)), "lnurl1hardware" to Scanner.LnurlPay( diff --git a/changelog.d/next/1454.fixed.md b/changelog.d/next/1454.fixed.md new file mode 100644 index 0000000000..678b52dbca --- /dev/null +++ b/changelog.d/next/1454.fixed.md @@ -0,0 +1 @@ +Fixed payment URI validation to accept Bitcoin links in notes while rejecting ambiguous payment requests through the shared decoder. diff --git a/journeys/send/README.md b/journeys/send/README.md new file mode 100644 index 0000000000..1e3e72d2bd --- /dev/null +++ b/journeys/send/README.md @@ -0,0 +1,11 @@ +# Send + +`bip21-core-validation.xml` is shared with iOS, with identical actions and fixtures. It requires a released bitkit-core SDK containing [PR #151](https://github.com/synonymdev/bitkit-core/pull/151); the current 0.5.18 dependency cannot satisfy its duplicate-key checks. Keep the adoption PR in draft until that dependency is bumped and the journey passes. + +Deliver each `Open bitcoin:...` action with `adb shell "am start -a android.intent.action.VIEW -d '' to.bitkit.dev"`, replacing XML `&` with `&` in the actual URI. Inspect errors immediately. Use `android layout` for payment state and screenshots for transient error toasts, which the semantic dump does not expose. + +Use an onboarded regtest wallet with at least 100,000 sats in Savings and Quickpay disabled. The fixture address is not a destination to pay; close every payment sheet without confirming payment. + +The native-binding tests also cover labels, proof-of-payment aliases and question marks in notes. App tests cover manual entry and payment-request failure handling. Automatic clipboard prompting additionally has a manual check because arbitrary OS clipboard injection is not listed in the Capabilities table. Do not type long URI fixtures with `adb shell input text`, which can truncate them. + +`own-invoice-guard.xml` covers the existing Lightning self-payment guard and is unchanged by this adoption. diff --git a/journeys/send/bip21-core-validation.xml b/journeys/send/bip21-core-validation.xml new file mode 100644 index 0000000000..fc5a5f7706 --- /dev/null +++ b/journeys/send/bip21-core-validation.xml @@ -0,0 +1,17 @@ + + Preconditions: an onboarded English-language regtest wallet with at least 100,000 sats in Savings, Quickpay disabled, and a bitkit-core release containing PR #151. Do not pay any fixture. Open each bitcoin: link through the platform's external-URI delivery command described in this suite's README. This checks that the native decoder rejects ambiguous payments while accepting bitcoin: text as metadata. + + Open bitcoin:bcrt1qr289x0fhg62672e8urudfnxnsr8tcax64xk2vk?amount=0.000035&message=bitcoin:donation + Verify Send shows the recipient bcrt1qr289x0fhg62672e8urudfnxnsr8tcax64xk2vk, an amount of 3,500 sats, and the note bitcoin:donation without a decoding error + Close Send without paying + Open bitcoin:bcrt1qr289x0fhg62672e8urudfnxnsr8tcax64xk2vk?amount=0.0000002&message=Bitkitbitcoin:bcrt1qr289x0fhg62672e8urudfnxnsr8tcax64xk2vk?amount=0.0000003&message=Bitkit + Verify a decoding error appears and Send does not open with the previous payment + Open bitcoin:bcrt1qr289x0fhg62672e8urudfnxnsr8tcax64xk2vk?amount=0.000035&AMOUNT=0.00005 + Verify a decoding error appears and no payment sheet opens + Open bitcoin:bcrt1qr289x0fhg62672e8urudfnxnsr8tcax64xk2vk?message=first&Message=second + Verify a decoding error appears and no payment sheet opens + Open bitcoin:bcrt1qr289x0fhg62672e8urudfnxnsr8tcax64xk2vk?amount=0.000035&custom=bitcoin:1BoatSLRHtKNngkdXEeobR76b53LETtpyT + Verify Send still shows bcrt1qr289x0fhg62672e8urudfnxnsr8tcax64xk2vk as the recipient and 3,500 sats as the amount + Close Send without paying + +