Adjust behaviour on malformed Address to ignore and continue rather t…

…han fall back (unnecessary limiting of addresses)
Multibit-Legacy · Jul 11, 2015 · aabe6c4 · aabe6c4
1 parent c815935
commit aabe6c4
Show file tree

Hide file tree

Showing 2 changed files with 25 additions and 11 deletions.
diff --git a/brit-core/src/main/java/org/multibit/hd/brit/core/dto/MatcherResponse.java b/brit-core/src/main/java/org/multibit/hd/brit/core/dto/MatcherResponse.java
@@ -127,8 +127,11 @@ public static MatcherResponse parse(byte[] serialisedMatcherResponse) throws Mat
             try {
               bitcoinAddresses.add(new Address(MainNetParams.get(), rows[i]));
             } catch (AddressFormatException e) {
-              // BRIT version 2 is more strict in its handling of malformed content
-              throw new MatcherResponseException("Malformed address in response: " + rows[i]);
+              // We ignore the malformed address but continue processing for the following reasons:
+              // 1. With a valid HMAC it is overwhelmingly likely to be a glitch at the server so safe to ignore
+              // 2. Coercing this code to send Version 1 means it is already compromised
+              // 3. Version 1 has this behaviour so this code works with legacy PayerRequests assisting testing
+              log.warn("Malformed address in response: '{}'", rows[i]);
             }
           }
         }

diff --git a/brit-core/src/test/java/org/multibit/hd/brit/matcher/BasicMatcherTest.java b/brit-core/src/test/java/org/multibit/hd/brit/matcher/BasicMatcherTest.java
@@ -28,16 +28,15 @@
 import org.junit.Test;
 import org.multibit.commons.crypto.AESUtils;
 import org.multibit.commons.crypto.PGPUtils;
+import org.multibit.hd.brit.BritTestUtils;
 import org.multibit.hd.brit.core.dto.*;
-import org.multibit.hd.brit.core.exceptions.MatcherResponseException;
 import org.multibit.hd.brit.core.matcher.*;
 import org.multibit.hd.brit.core.payer.BasicPayer;
 import org.multibit.hd.brit.core.payer.Payer;
 import org.multibit.hd.brit.core.payer.PayerConfig;
 import org.multibit.hd.brit.core.payer.Payers;
 import org.multibit.hd.brit.core.seed_phrase.Bip39SeedPhraseGenerator;
 import org.multibit.hd.brit.core.seed_phrase.SeedPhraseGenerator;
-import org.multibit.hd.brit.BritTestUtils;
 import org.multibit.hd.brit.dto.BRITWalletIdTest;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -51,7 +50,6 @@
 import java.util.Set;
 
 import static org.fest.assertions.api.Assertions.assertThat;
-import static org.fest.assertions.api.Assertions.fail;
 
 
 public class BasicMatcherTest {
@@ -67,7 +65,7 @@ public static void setUpOnce() throws Exception {
 
     String[] rawTestAddresses = new String[]{
 
-      // Good addresses
+      // Good addresses (6)
       "1AhN6rPdrMuKBGFDKR1k9A8SCLYaNgXhty",
       "14Ru32Lb4kdLGfAMz1VAtxh3UFku62HaNH",
       "1KesQEF2yC2FzkJYLLozZJdbBF7zRhrdSC",
@@ -153,11 +151,13 @@ public void testPayerRequestAndMatcherResponse_Version_1_All_Good() throws Excep
     // The Payer's Matcher response contains the list of addresses the Payer will use
     Set<Address> addressList = payersMatcherResponse.getBitcoinAddresses();
     assertThat(addressList).isNotNull();
+    assertThat(addressList.size()).isEqualTo(6);
     assertThat(addressList.contains(testAddresses.get(0))).isTrue();
     assertThat(addressList.contains(testAddresses.get(1))).isTrue();
     assertThat(addressList.contains(testAddresses.get(2))).isTrue();
     assertThat(addressList.contains(testAddresses.get(3))).isTrue();
     assertThat(addressList.contains(testAddresses.get(4))).isTrue();
+    assertThat(addressList.contains(testAddresses.get(5))).isTrue();
 
     // The Payer's Matcher response contains a stored replay date for the wallet
     Date replayDate = payersMatcherResponse.getReplayDate().get();
@@ -230,6 +230,7 @@ public void testPayerRequestAndMatcherResponse_Version_2_All_Good() throws Excep
     // The Payer's Matcher response contains the list of addresses the Payer will use
     Set<Address> addressList = payersMatcherResponse.getBitcoinAddresses();
     assertThat(addressList).isNotNull();
+    assertThat(addressList.size()).isEqualTo(6);
     assertThat(addressList.contains(testAddresses.get(0))).isTrue();
     assertThat(addressList.contains(testAddresses.get(1))).isTrue();
     assertThat(addressList.contains(testAddresses.get(2))).isTrue();
@@ -246,7 +247,7 @@ public void testPayerRequestAndMatcherResponse_Version_2_All_Good() throws Excep
    * Verifies the Version 2 interaction when one address is bad (server damaged)
    * @throws Exception
    */
-  @Test(expected = MatcherResponseException.class)
+  @Test
   public void testPayerRequestAndMatcherResponse_Version_2_One_Bad() throws Exception {
 
     // Create a standard Payer
@@ -307,12 +308,12 @@ public byte[] serialise() {
         }
         if (getBitcoinAddresses() != null) {
           for (Address address : getBitcoinAddresses()) {
+            // Interleave a bad address to avoid test misses
+            builder.append("1XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX").append(PayerRequest.SEPARATOR);
             builder.append(address).append(PayerRequest.SEPARATOR);
           }
         }
 
-        // Append a bad address
-        builder.append("1XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX").append(PayerRequest.SEPARATOR);
 
         return builder.toString().getBytes(Charsets.UTF_8);
 
@@ -328,9 +329,18 @@ public byte[] serialise() {
     // The payer can decrypt the encryptedMatcherResponse
     // as it knows the BRITWalletId and session id
     // We expect an exception due to the malformed address within
-    payer.decryptMatcherResponse(encryptedMatcherResponse, payerRequest);
+    MatcherResponse payersMatcherResponse = payer.decryptMatcherResponse(encryptedMatcherResponse, payerRequest);
 
-    fail("Expected exception");
+    // The Payer's Matcher response contains the list of addresses the Payer will use
+    Set<Address> addressList = payersMatcherResponse.getBitcoinAddresses();
+    assertThat(addressList).isNotNull();
+    assertThat(addressList.size()).isEqualTo(6);
+    assertThat(addressList.contains(testAddresses.get(0))).isTrue();
+    assertThat(addressList.contains(testAddresses.get(1))).isTrue();
+    assertThat(addressList.contains(testAddresses.get(2))).isTrue();
+    assertThat(addressList.contains(testAddresses.get(3))).isTrue();
+    assertThat(addressList.contains(testAddresses.get(4))).isTrue();
+    assertThat(addressList.contains(testAddresses.get(5))).isTrue();
 
   }
 
@@ -383,6 +393,7 @@ private Matcher createTestMatcher_One_Bad() throws Exception {
     bitcoinAddresses.add(testAddresses.get(2));
     bitcoinAddresses.add(testAddresses.get(3));
     bitcoinAddresses.add(testAddresses.get(4));
+    bitcoinAddresses.add(testAddresses.get(5));
 
     matcherStore.storeBitcoinAddressesForDate(bitcoinAddresses, new Date());