diff --git a/app/src/main/java/org/fdroid/fdroid/IndexV1Updater.java b/app/src/main/java/org/fdroid/fdroid/IndexV1Updater.java index f5d586e2c..5118dbf90 100644 --- a/app/src/main/java/org/fdroid/fdroid/IndexV1Updater.java +++ b/app/src/main/java/org/fdroid/fdroid/IndexV1Updater.java @@ -9,6 +9,8 @@ import android.support.v4.content.LocalBroadcastManager; import android.text.TextUtils; import android.util.Log; +import com.fasterxml.jackson.annotation.JsonAutoDetect; +import com.fasterxml.jackson.annotation.PropertyAccessor; import com.fasterxml.jackson.core.JsonFactory; import com.fasterxml.jackson.core.JsonParser; import com.fasterxml.jackson.core.type.TypeReference; @@ -46,6 +48,12 @@ import java.util.jar.JarFile; * by Jackson. This is possible but not wise to do with {@link Repo} since that * class has many fields that are related to security components of the * implementation internal to this app. + *

+ * All non-{@code public} fields and fields tagged with {@code @JsonIgnore} are + * ignored. All methods are ignored unless they are tagged with {@code @JsonProperty}. + * This setup prevents the situation where future developers add variables to the + * App/Apk classes, resulting in malicious servers being able to populate those + * variables. */ public class IndexV1Updater extends RepoUpdater { public static final String TAG = "IndexV1Updater"; @@ -121,6 +129,22 @@ public class IndexV1Updater extends RepoUpdater { return true; } + /** + * Get the standard {@link ObjectMapper} instance used for parsing {@code index-v1.json}. + * This ignores unknown properties so that old releases won't crash when new things are + * added to {@code index-v1.json}. This is required for both forward compatibility, + * but also because ignoring such properties when coming from a malicious server seems + * reasonable anyway. + */ + public static ObjectMapper getObjectMapperInstance(long repoId) { + ObjectMapper mapper = new ObjectMapper(); + mapper.disable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES); + mapper.setInjectableValues(new InjectableValues.Std().addValue("repoId", repoId)); + mapper.setVisibility(PropertyAccessor.ALL, JsonAutoDetect.Visibility.NONE); + mapper.setVisibility(PropertyAccessor.FIELD, JsonAutoDetect.Visibility.PUBLIC_ONLY); + return mapper; + } + /** * Parses the index and feeds it to the database via {@link Repo}, {@link App}, * and {@link Apk} instances. @@ -132,9 +156,7 @@ public class IndexV1Updater extends RepoUpdater { */ public void processIndexV1(InputStream indexInputStream, JarEntry indexEntry, String cacheTag) throws IOException, UpdateException { - ObjectMapper mapper = new ObjectMapper(); - mapper.disable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES); - mapper.setInjectableValues(new InjectableValues.Std().addValue("repoId", repo.getId())); + ObjectMapper mapper = getObjectMapperInstance(repo.getId()); JsonFactory f = mapper.getFactory(); JsonParser parser = f.createParser(indexInputStream); HashMap repoMap = null; diff --git a/app/src/test/java/org/fdroid/fdroid/updater/IndexV1UpdaterTest.java b/app/src/test/java/org/fdroid/fdroid/updater/IndexV1UpdaterTest.java index fef637a85..9abb974ca 100644 --- a/app/src/test/java/org/fdroid/fdroid/updater/IndexV1UpdaterTest.java +++ b/app/src/test/java/org/fdroid/fdroid/updater/IndexV1UpdaterTest.java @@ -7,7 +7,6 @@ import com.fasterxml.jackson.core.JsonParser; import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.core.type.TypeReference; import com.fasterxml.jackson.databind.DeserializationFeature; -import com.fasterxml.jackson.databind.InjectableValues; import com.fasterxml.jackson.databind.ObjectMapper; import com.fasterxml.jackson.databind.ObjectReader; import org.apache.commons.io.IOUtils; @@ -129,10 +128,9 @@ public class IndexV1UpdaterTest extends FDroidProviderTest { @Test public void testJacksonParsing() throws IOException { - ObjectMapper mapper = new ObjectMapper(); + ObjectMapper mapper = IndexV1Updater.getObjectMapperInstance(FAKE_REPO_ID); // the app ignores all unknown fields when complete, do not ignore during dev to catch mistakes mapper.enable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES); - mapper.setInjectableValues(new InjectableValues.Std().addValue("repoId", FAKE_REPO_ID)); JsonFactory f = mapper.getFactory(); JsonParser parser = f.createParser(TestUtils.copyResourceToTempFile("guardianproject_index-v1.json")); @@ -199,7 +197,10 @@ public class IndexV1UpdaterTest extends FDroidProviderTest { * index parsing, Jackson is always reading JSON into {@link App} and {@link Apk} * instances. {@code @JsonIgnoreProperties} applies to both directions. *

- * The test sets come from the top of {@link App} and {@link Apk}. + * If this test fails for you, then it probably means you are adding a new instance + * variable to {@link App}. In that case, you need to add it to the appropriate + * list here. If it is allowed, make sure this new instance variable is in sync with + * {@code index-v1.json} and {@code fdroidserver}. */ @Test public void testJsonIgnoreApp() throws JsonProcessingException { @@ -256,6 +257,19 @@ public class IndexV1UpdaterTest extends FDroidProviderTest { runJsonIgnoreTest(new App(), allowedInApp, ignoredInApp); } + + /** + * Test that all the fields are properly marked as whether Jackson should ignore them + * or not. Technically this test goes the opposite direction of how its being used: + * it writes out {@link App} and {@link Apk} instances to JSON using Jackson. With the + * index parsing, Jackson is always reading JSON into {@link App} and {@link Apk} + * instances. {@code @JsonIgnoreProperties} applies to both directions. + *

+ * If this test fails for you, then it probably means you are adding a new instance + * variable to {@link Apk}. In that case, you need to add it to the appropriate + * list here. If it is allowed, make sure this new instance variable is in sync with + * {@code index-v1.json} and {@code fdroidserver}. + */ @Test public void testJsonIgnoreApk() throws JsonProcessingException { String[] allowedInApk = new String[]{ @@ -299,7 +313,7 @@ public class IndexV1UpdaterTest extends FDroidProviderTest { private void runJsonIgnoreTest(Object instance, String[] allowed, String[] ignored) throws JsonProcessingException { - ObjectMapper mapper = new ObjectMapper(); + ObjectMapper mapper = IndexV1Updater.getObjectMapperInstance(FAKE_REPO_ID); String objectAsString = mapper.writeValueAsString(instance); Set fields = getFields(instance); for (String field : ignored) { @@ -321,10 +335,7 @@ public class IndexV1UpdaterTest extends FDroidProviderTest { @Test public void testInstanceVariablesAreProperlyMarked() throws IOException { - ObjectMapper mapper = new ObjectMapper(); - // testing with unknown metadata in it - mapper.disable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES); - mapper.setInjectableValues(new InjectableValues.Std().addValue("repoId", FAKE_REPO_ID)); + ObjectMapper mapper = IndexV1Updater.getObjectMapperInstance(FAKE_REPO_ID); JsonFactory f = mapper.getFactory(); JsonParser parser = f.createParser(TestUtils.copyResourceToTempFile("all_fields_index-v1.json"));