fix(lint): restrict data-plan detector to known event builder types

tooling/custom-lint-rules/src/main/java/com/mparticle/lints/Extensions.kt

@@ -9,6 +9,7 @@ import com.intellij.psi.PsiVariable
 import com.intellij.psi.impl.compiled.ClsMethodImpl
 import com.intellij.psi.impl.source.PsiClassReferenceType
 import com.intellij.psi.impl.source.PsiImmediateClassType
+import com.mparticle.lints.dtos.AllowedTypes
 import com.mparticle.lints.dtos.Constructor
 import com.mparticle.lints.dtos.Expression
 import com.mparticle.lints.dtos.MethodCall
@@ -238,7 +239,7 @@ internal fun UExpression.resolveChainedCalls(returnValue: Boolean, instance: Exp
         }
 }
 
-internal fun Pair<*, *>.resolveToEnum(): Enum<*> {
+internal fun Pair<*, *>.resolveToEnum(): Enum<*>? {
     val className =
         when (first) {
             is ClassId -> "${(first as ClassId).packageFqName}.${
@@ -247,6 +248,9 @@ internal fun Pair<*, *>.resolveToEnum(): Enum<*> {
             is String -> first as String
             else -> null
         }
+    if (!AllowedTypes.isAllowed(className)) {
+        return null
+    }
     return className?.let { className ->
         val enumName = second.toString()
         val constructor =
@@ -255,7 +259,7 @@ internal fun Pair<*, *>.resolveToEnum(): Enum<*> {
                 .methods
                 .first { it.name == "valueOf" }
         constructor.invoke(null, enumName)
-    } as Enum<*>
+    } as? Enum<*>
 }
 
 internal fun List<Value>.resolve(): List<Any?> = map { it.resolve() }

tooling/custom-lint-rules/src/main/java/com/mparticle/lints/dtos/AllowedTypes.kt

@@ -0,0 +1,31 @@
+package com.mparticle.lints.dtos
+
+/**
+ * The Data Plan detector needs to compute the message body of an event builder chain found in
+ * analyzed source, which requires resolving constructor/static calls via reflection. Reflection
+ * must never run against a class named by the analyzed source itself unless it's one of the
+ * mParticle DTO/builder types (or a small set of collection helpers commonly used inline to build
+ * attribute maps) - anything else is attacker-controlled input, not code we intend to execute.
+ */
+internal object AllowedTypes {
+    private val ALLOWED_CLASS_NAMES =
+        setOf(
+            "com.mparticle.MPEvent",
+            "com.mparticle.MPEvent\$Builder",
+            "com.mparticle.commerce.CommerceEvent",
+            "com.mparticle.commerce.CommerceEvent\$Builder",
+            "com.mparticle.commerce.Product",
+            "com.mparticle.commerce.Product\$Builder",
+            "com.mparticle.commerce.Promotion",
+            "com.mparticle.commerce.Impression",
+            "com.mparticle.commerce.TransactionAttributes",
+            "com.mparticle.MParticle\$EventType",
+            "java.util.HashMap",
+            "java.util.LinkedHashMap",
+            "java.util.ArrayList",
+            "kotlin.collections.MapsKt",
+            "kotlin.collections.CollectionsKt",
+        )
+
+    fun isAllowed(qualifiedClassName: String?): Boolean = qualifiedClassName != null && ALLOWED_CLASS_NAMES.contains(qualifiedClassName)
+}

tooling/custom-lint-rules/src/main/java/com/mparticle/lints/dtos/Constructor.kt

@@ -7,9 +7,12 @@ import org.jetbrains.uast.UCallExpression
 data class Constructor(override val parent: Expression, val methodName: String?, override val node: UCallExpression) : ParameterizedExpression {
     override var arguments: List<Value> = listOf()
 
-    override fun resolve(): Any? {
+    override fun resolve(): Any? = ResolutionGuard.guarded {
         val qualifiedClassName =
             node.receiverClassName()?.replace(".Builder", "\$Builder")
+        if (!AllowedTypes.isAllowed(qualifiedClassName)) {
+            return@guarded null
+        }
         val clazz = Class.forName(qualifiedClassName)
         val params: List<Any?> = arguments.resolve()
         val argumentClasses =
@@ -29,15 +32,15 @@ data class Constructor(override val parent: Expression, val methodName: String?,
         try {
             if (constructor != null) {
                 if (params.size > 0) {
-                    return constructor.newInstance(*params.toTypedArray())
+                    return@guarded constructor.newInstance(*params.toTypedArray())
                 } else {
-                    return constructor.newInstance()
+                    return@guarded constructor.newInstance()
                 }
             }
         } catch (ex: Exception) {
             "no new Instance for $clazz.name, tried constructor: ${constructor?.name}"
         }
-        return clazz
+        return@guarded clazz
     }
 
     override fun forEachExpression(predicate: (Expression) -> Unit) {

tooling/custom-lint-rules/src/main/java/com/mparticle/lints/dtos/MethodCall.kt

@@ -10,10 +10,10 @@ data class MethodCall(override val parent: Expression, val methodName: String?,
         arguments.forEach { it.parent = this }
     }
 
-    override fun resolve(): Any? {
+    override fun resolve(): Any? = ResolutionGuard.guarded {
         val instance = parent.resolve()
         if (instance == null) {
-            return null
+            return@guarded null
         }
         var matchingMethods =
             instance::class.java.methods
@@ -33,12 +33,12 @@ data class MethodCall(override val parent: Expression, val methodName: String?,
             val arguments = arguments.resolve()
             val value = method.invoke(instance, *arguments.toTypedArray())
             if (returnValue) {
-                return value
+                return@guarded value
             } else {
-                return instance
+                return@guarded instance
             }
         }
-        return null
+        return@guarded null
     }
 
     override fun equals(other: Any?): Boolean = node.equals((other as? MethodCall)?.node)

tooling/custom-lint-rules/src/main/java/com/mparticle/lints/dtos/ResolutionGuard.kt

@@ -0,0 +1,25 @@
+package com.mparticle.lints.dtos
+
+/**
+ * Bounds the depth of expression resolution. Some qualified call chains cause a resolved
+ * expression's parent to keep resolving into itself; without a bound that recurses until the
+ * stack overflows, which callers' try/catch(Exception) blocks don't catch. Throwing a regular
+ * exception once a generous depth is exceeded keeps that failure catchable.
+ */
+internal object ResolutionGuard {
+    private const val MAX_DEPTH = 50
+    private val depth = ThreadLocal.withInitial { 0 }
+
+    fun <T> guarded(block: () -> T): T {
+        val current = depth.get()
+        if (current >= MAX_DEPTH) {
+            throw IllegalStateException("Expression resolution exceeded max depth of $MAX_DEPTH")
+        }
+        depth.set(current + 1)
+        try {
+            return block()
+        } finally {
+            depth.set(current)
+        }
+    }
+}

tooling/custom-lint-rules/src/main/java/com/mparticle/lints/dtos/StaticFactory.kt

@@ -9,8 +9,11 @@ class StaticFactory(val methodName: String?, override val node: UCallExpression)
     override val parent = RootParent(node)
     override var arguments: List<Value> = listOf()
 
-    override fun resolve(): Any? {
+    override fun resolve(): Any? = ResolutionGuard.guarded {
         val qualifiedClassName = (node.resolve()?.parent as? ClsClassImpl)?.stub?.qualifiedName
+        if (!AllowedTypes.isAllowed(qualifiedClassName)) {
+            return@guarded null
+        }
         val methods = HashSet<Method>()
         val clazz = Class.forName(qualifiedClassName)
         methods.addAll(clazz.declaredMethods)
@@ -31,9 +34,9 @@ class StaticFactory(val methodName: String?, override val node: UCallExpression)
                 }
             val arguments = arguments.resolve()
             method.isAccessible = true
-            return method.invoke(null, *arguments.toTypedArray())
+            return@guarded method.invoke(null, *arguments.toTypedArray())
         }
-        return null
+        return@guarded null
     }
 
     override fun forEachExpression(predicate: (Expression) -> Unit) {

tooling/custom-lint-rules/src/test/java/com/mparticle/lints/Constants.kt

@@ -22,12 +22,23 @@ object Constants {
 
     @Language("JAVA")
     const val APPLICATION_STUB = """
-    package android.app; 
+    package android.app;
     public class Application {
        public void onCreate() {}
        public void onResume() {}
     }"""
 
+    @Language("JAVA")
+    const val MPEVENT_STUB = """package com.mparticle;
+    public class MPEvent {
+       public static class Builder {
+           public Builder(String eventName) {}
+           public Builder customAttributes(java.util.Map<String, ?> customAttributes) { return this; }
+           public MPEvent build() { return new MPEvent(); }
+       }
+    }"""
+
     val mParticleStubClass = java(MPARTICLE_STUB)
     val mApplicationStubClass = java(APPLICATION_STUB)
+    val mpEventStubClass = java(MPEVENT_STUB)
 }

tooling/custom-lint-rules/src/test/java/com/mparticle/lints/DataplanDetectorTest.kt

@@ -2,9 +2,14 @@ package com.mparticle.lints
 
 import com.android.tools.lint.checks.infrastructure.LintDetectorTest
 import com.android.tools.lint.checks.infrastructure.TestMode
+import com.mparticle.lints.Constants.mApplicationStubClass
 import com.mparticle.lints.Constants.mParticleStubClass
+import com.mparticle.lints.Constants.mpEventStubClass
 import com.mparticle.lints.detectors.DataplanDetector
+import com.mparticle.tooling.Config
+import com.mparticle.tooling.Utils
 import org.intellij.lang.annotations.Language
+import org.junit.Assert.assertTrue
 import org.junit.Test
 import java.io.File
 
@@ -35,6 +40,97 @@ class DataplanDetectorTest : LintDetectorTest() {
             .expectErrorCount(0)
     }
 
+    // Regression test: a local variable feeding an MPEvent.Builder argument used to be resolved
+    // by reflectively replaying every call made on it in the enclosing method, with no
+    // restriction on what classes/methods those calls could touch. This reproduces that shape
+    // (an unrelated File operation feeding the event name) and asserts lint never performs it.
+    @Test
+    fun testEnclosingMethodSideEffectsAreNeverExecuted() {
+        val sdkHome =
+            System.getenv("ANDROID_HOME")
+                ?: "${System.getProperty("user.home")}/Library/Android/sdk"
+
+        val marker = File.createTempFile("mparticle-lint-regression", ".txt")
+        marker.writeText("must survive lint analysis")
+        val markerPath = marker.absolutePath.replace("\\", "\\\\")
+
+        try {
+            withConfigFile(Config()) {
+                @Language("JAVA")
+                val source = """
+                    package com.mparticle.lints;
+                    import android.app.Application;
+                    import com.mparticle.MPEvent;
+                    import java.io.File;
+                    public class HasUnrelatedFileCall extends Application {
+                        @Override
+                        public void onCreate() {
+                            super.onCreate();
+                            File f = new File("$markerPath");
+                            f.delete();
+                            String name = f.getName();
+                            new MPEvent.Builder(name);
+                        }
+                    }
+                    """
+                lint()
+                    .sdkHome(File(sdkHome))
+                    .files(java(source), mParticleStubClass, mApplicationStubClass, mpEventStubClass)
+                    .skipTestModes(TestMode.PARENTHESIZED)
+                    .run()
+            }
+            assertTrue(
+                "Data plan lint must not perform filesystem operations found in analyzed source",
+                marker.exists(),
+            )
+        } finally {
+            marker.delete()
+        }
+    }
+
+    // A legitimate builder chain (the only thing this detector needs to resolve) must keep
+    // resolving successfully - the allowlist should reject non-DTO reflection, not the feature.
+    @Test
+    fun testAllowlistedBuilderChainStillResolves() {
+        val sdkHome =
+            System.getenv("ANDROID_HOME")
+                ?: "${System.getProperty("user.home")}/Library/Android/sdk"
+
+        withConfigFile(Config()) {
+            @Language("JAVA")
+            val source = """
+                package com.mparticle.lints;
+                import android.app.Application;
+                import com.mparticle.MPEvent;
+                public class HasAllowlistedCall extends Application {
+                    @Override
+                    public void onCreate() {
+                        super.onCreate();
+                        new MPEvent.Builder("test").build();
+                    }
+                }
+                """
+            val result =
+                lint()
+                    .sdkHome(File(sdkHome))
+                    .files(java(source), mParticleStubClass, mApplicationStubClass, mpEventStubClass)
+                    .skipTestModes(TestMode.PARENTHESIZED)
+                    .run()
+            // With no local data plan configured, a successfully-resolved event is reported as
+            // NO_DATA_PLAN; a failure to resolve at all would instead produce no report.
+            result.expectContains(DataplanDetector.NO_DATA_PLAN.id)
+        }
+    }
+
+    private fun withConfigFile(config: Config, block: () -> Unit) {
+        Utils.setConfigFile(config)
+        try {
+            block()
+        } finally {
+            Utils.removeConfigFile()
+        }
+    }
+
     override fun requireCompileSdk() = false
 
     override fun getDetector() = DataplanDetector()