From 859c2b4496618c8142c70610ebba395e9aa4d5dc Mon Sep 17 00:00:00 2001
From: cyBerta <cyberta@riseup.net>
Date: Fri, 3 Nov 2017 15:17:45 +0100
Subject: #8778 fixes invalid provider error handling

---
 .../test/TestConfigurationWizard.java              | 36 ++++++++++++++++------
 .../java/se/leap/bitmaskclient/ProviderAPI.java    | 12 ++++++--
 .../java/se/leap/bitmaskclient/ConfigHelper.java   |  2 +-
 .../se/leap/bitmaskclient/ProviderApiBase.java     | 11 +++++++
 .../java/se/leap/bitmaskclient/ProviderAPI.java    | 12 ++++++--
 5 files changed, 57 insertions(+), 16 deletions(-)

diff --git a/app/src/androidTest/java/se/leap/bitmaskclient/test/TestConfigurationWizard.java b/app/src/androidTest/java/se/leap/bitmaskclient/test/TestConfigurationWizard.java
index 90b8b839..12e88af8 100644
--- a/app/src/androidTest/java/se/leap/bitmaskclient/test/TestConfigurationWizard.java
+++ b/app/src/androidTest/java/se/leap/bitmaskclient/test/TestConfigurationWizard.java
@@ -14,7 +14,6 @@ import se.leap.bitmaskclient.R;
 public class TestConfigurationWizard extends ActivityInstrumentationTestCase2<ConfigurationWizard> {
 
     private Solo solo;
-    private static int added_providers;
 
     public TestConfigurationWizard() {
         super(ConfigurationWizard.class);
@@ -34,11 +33,14 @@ public class TestConfigurationWizard extends ActivityInstrumentationTestCase2<Co
         super.tearDown();
     }
 
-    public void testListProviders() {
+    /**
+     * Tests should run independently from each other. We need a better approach to test the amount of providers added
+      */
+    /*public void testListProviders() {
         assertEquals(solo.getCurrentViews(ListView.class).size(), 1);
 
         assertEquals("Number of available providers differ", predefinedProviders() + added_providers, shownProviders());
-    }
+    }*/
 
     private int shownProviders() {
         return solo.getCurrentViews(ListView.class).get(0).getCount();
@@ -76,24 +78,40 @@ public class TestConfigurationWizard extends ActivityInstrumentationTestCase2<Co
 
     public void testAddNewProvider() {
         //addProvider("calyx.net");
-        addProvider("riseup.net");
+        addProvider("riseup.net", true);
+    }
+
+    public void testAddFalseProviderReturning404() {
+        //addProvider("calyx.net");
+        addProvider("startpage.com", false);
+    }
+
+    public void testAddFalseProviderReturning200() {
+        //addProvider("calyx.net");
+        addProvider("test.com", false);
     }
 
-    private void addProvider(String url) {
-        boolean is_new_provider = !solo.searchText(url);
+    private void addProvider(String url, boolean expectSuccess) {
 
-        if (is_new_provider)
-            added_providers = added_providers + 1;
         solo.clickOnActionBarItem(R.id.new_provider);
         solo.enterText(0, url);
         if ( BuildConfig.FLAVOR.equals("insecure")) {
             solo.clickOnCheckBox(0);
         }
         solo.clickOnText(solo.getString(R.string.save));
-        waitForProviderDetails();
+        if (expectSuccess) {
+            waitForProviderDetails();
+        } else {
+            waitForNoValidProviderError();
+        }
         solo.goBack();
     }
 
+    private void waitForNoValidProviderError() {
+        String text = solo.getString(R.string.malformed_url);
+        assertTrue("Provider details dialog did not appear", solo.waitForText(text, 1, 60*1000));
+    }
+
     public void testShowAbout() {
         showAbout();
     }
diff --git a/app/src/insecure/java/se/leap/bitmaskclient/ProviderAPI.java b/app/src/insecure/java/se/leap/bitmaskclient/ProviderAPI.java
index 588ff7e2..87fff283 100644
--- a/app/src/insecure/java/se/leap/bitmaskclient/ProviderAPI.java
+++ b/app/src/insecure/java/se/leap/bitmaskclient/ProviderAPI.java
@@ -125,6 +125,12 @@ public class ProviderAPI extends ProviderApiBase {
             else
                 provider_dot_json_string = downloadWithCommercialCA(provider_main_url + "/provider.json", danger_on, provider_ca_cert_fingerprint);
 
+            if (!isValidJson(provider_dot_json_string)) {
+                result.putString(ERRORS, getString(malformed_url));
+                result.putBoolean(RESULT_KEY, false);
+                return result;
+            }
+
             try {
                 JSONObject provider_json = new JSONObject(provider_dot_json_string);
                 provider_api_url = provider_json.getString(Provider.API_URL) + "/" + provider_json.getString(Provider.API_VERSION);
@@ -165,7 +171,7 @@ public class ProviderAPI extends ProviderApiBase {
                 preferences.edit().putString(Constants.KEY, eip_service_json.toString()).commit();
 
                 result.putBoolean(RESULT_KEY, true);
-            } catch (JSONException e) {
+            } catch (NullPointerException | JSONException e) {
                 String reason_to_fail = pickErrorMessage(eip_service_json_string);
                 result.putString(ERRORS, reason_to_fail);
                 result.putBoolean(RESULT_KEY, false);
@@ -189,7 +195,7 @@ public class ProviderAPI extends ProviderApiBase {
 
             String cert_string = downloadWithProviderCA(new_cert_string_url.toString(), last_danger_on);
 
-            if (cert_string.isEmpty() || ConfigHelper.checkErroneousDownload(cert_string))
+            if (cert_string == null || cert_string.isEmpty() || ConfigHelper.checkErroneousDownload(cert_string))
                 return false;
             else
                 return loadCertificate(cert_string);
@@ -274,7 +280,7 @@ public class ProviderAPI extends ProviderApiBase {
 
         responseString = sendGetStringToServer(string_url, headerArgs, okHttpClient);
 
-        if (responseString.contains(ERRORS)) {
+        if (responseString != null && responseString.contains(ERRORS)) {
             try {
                 // try to download with provider CA on certificate error
                 JSONObject responseErrorJson = new JSONObject(responseString);
diff --git a/app/src/main/java/se/leap/bitmaskclient/ConfigHelper.java b/app/src/main/java/se/leap/bitmaskclient/ConfigHelper.java
index 4929f040..33b2e225 100644
--- a/app/src/main/java/se/leap/bitmaskclient/ConfigHelper.java
+++ b/app/src/main/java/se/leap/bitmaskclient/ConfigHelper.java
@@ -47,7 +47,7 @@ public class ConfigHelper {
             } else {
                 return false;
             }
-        } catch (JSONException e) {
+        } catch (NullPointerException | JSONException e) {
             return false;
         }
     }
diff --git a/app/src/main/java/se/leap/bitmaskclient/ProviderApiBase.java b/app/src/main/java/se/leap/bitmaskclient/ProviderApiBase.java
index caa48231..bf1c3941 100644
--- a/app/src/main/java/se/leap/bitmaskclient/ProviderApiBase.java
+++ b/app/src/main/java/se/leap/bitmaskclient/ProviderApiBase.java
@@ -687,6 +687,17 @@ public abstract class ProviderApiBase extends IntentService {
         return CA_CERT_DOWNLOADED;
     }
 
+    protected boolean isValidJson(String jsonString) {
+        try {
+            new JSONObject(jsonString);
+            return true;
+        } catch(JSONException e) {
+            return false;
+        } catch(NullPointerException e) {
+            return false;
+        }
+    }
+
     protected boolean validCertificate(String cert_string) {
         boolean result = false;
         if (!ConfigHelper.checkErroneousDownload(cert_string)) {
diff --git a/app/src/production/java/se/leap/bitmaskclient/ProviderAPI.java b/app/src/production/java/se/leap/bitmaskclient/ProviderAPI.java
index 982080de..70a2c27c 100644
--- a/app/src/production/java/se/leap/bitmaskclient/ProviderAPI.java
+++ b/app/src/production/java/se/leap/bitmaskclient/ProviderAPI.java
@@ -107,6 +107,12 @@ public class ProviderAPI extends ProviderApiBase {
             else
                 provider_dot_json_string = downloadWithCommercialCA(provider_main_url + "/provider.json", provider_ca_cert_fingerprint);
 
+                if (!isValidJson(provider_dot_json_string)) {
+                    result.putString(ERRORS, getString(malformed_url));
+                    result.putBoolean(RESULT_KEY, false);
+                    return result;
+                }
+
             try {
                 JSONObject provider_json = new JSONObject(provider_dot_json_string);
                 provider_api_url = provider_json.getString(Provider.API_URL) + "/" + provider_json.getString(Provider.API_VERSION);
@@ -147,7 +153,7 @@ public class ProviderAPI extends ProviderApiBase {
                 preferences.edit().putString(Constants.KEY, eip_service_json.toString()).commit();
 
                 result.putBoolean(RESULT_KEY, true);
-            } catch (JSONException e) {
+            } catch (NullPointerException | JSONException e) {
                 String reason_to_fail = pickErrorMessage(eip_service_json_string);
                 result.putString(ERRORS, reason_to_fail);
                 result.putBoolean(RESULT_KEY, false);
@@ -171,7 +177,7 @@ public class ProviderAPI extends ProviderApiBase {
 
             String cert_string = downloadWithProviderCA(new_cert_string_url.toString());
 
-            if (cert_string.isEmpty() || ConfigHelper.checkErroneousDownload(cert_string))
+            if (cert_string == null || cert_string.isEmpty() || ConfigHelper.checkErroneousDownload(cert_string))
                 return false;
             else
                 return loadCertificate(cert_string);
@@ -253,7 +259,7 @@ public class ProviderAPI extends ProviderApiBase {
 
         responseString = sendGetStringToServer(string_url, headerArgs, okHttpClient);
 
-        if (responseString.contains(ERRORS)) {
+        if (responseString != null && responseString.contains(ERRORS)) {
             try {
                 // try to download with provider CA on certificate error
                 JSONObject responseErrorJson = new JSONObject(responseString);
-- 
cgit v1.2.3