Skip to content

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

Merged
nickolas-dimitrakas merged 4 commits into
mainfrom
fix/dataplan-lint-reflection-allowlist
Sep 21, 2026
Merged

nickolas-dimitrakas merged 4 commits into
mainfrom
fix/dataplan-lint-reflection-allowlist

Conversation

@nickolas-dimitrakas

@nickolas-dimitrakas nickolas-dimitrakas commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

Background

The Data Plan lint detector evaluates MPEvent.Builder/CommerceEvent.Builder calls found in analyzed source to check them against a data plan. This change tightens what it's allowed to touch while doing that evaluation.

What Has Changed

  • Restrict the detector's expression evaluation to an explicit allowlist of mParticle event/commerce builder types.
  • Add a bounded recursion depth to expression resolution.

Screenshots/Video

N/A — lint tooling change, no UI.

Checklist

  • Self-review completed
  • Tests added
  • Tested locally

Additional Notes

  • ./gradlew :tooling:custom-lint-rules:test and :tooling:custom-lint-rules:ktlintCheck pass.
  • New regression tests in DataplanDetectorTest confirm the allowlist is enforced and that a normal builder chain still resolves as before.

🤖 Generated with Claude Code

…ents

DataplanDetector resolved MPEvent.Builder/CommerceEvent.Builder chains
via unrestricted reflection (Class.forName/newInstance/invoke) driven
by class and method names found in the analyzed project's own source,
including anything reachable through a local variable's initializer
chain. Restrict resolution to an explicit allowlist of the mParticle
DTO/builder types this detector actually needs, and bound expression
resolution depth so an unrelated failure mode can't escape the
existing exception handling as a stack overflow.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@nickolas-dimitrakas
nickolas-dimitrakas requested a review from a team as a code owner September 17, 2026 14:35
@cursor

cursor Bot commented Sep 17, 2026

Copy link
Copy Markdown

PR Summary

Low Risk
Changes are isolated to custom build-time lint rules and expression resolution hardening, with no impact on the runtime SDK.

Overview
Restricts reflective evaluation in the Data Plan lint detector by introducing an explicit allowlist of permitted classes and factory methods (AllowedTypes). This prevents arbitrary code execution and filesystem side effects from analyzed source code during lint runs.

Key changes include:

  • Added AllowedTypes to permit only specific mParticle builder types, collection helpers (e.g., mapOf, listOf), and safe instance types during resolution in Constructor, MethodCall, StaticFactory, and enum helpers.
  • Added ResolutionGuard to enforce a maximum recursion depth of 50 during expression resolution, preventing unhandled StackOverflowError crashes from cyclic call chains.
  • Added regression tests verifying that unsafe side-effects and reflection escapes are blocked while valid event builder chains continue to resolve properly.

Reviewed by Cursor Bugbot for commit 7a82061. Bugbot is set up for automated code reviews on this repo. Configure here.

nickolas-dimitrakas and others added 2 commits September 17, 2026 10:35
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Nickolas Dimitrakas <nickolas.dimitrakas@rokt.com>
@github-actions

github-actions Bot commented Sep 17, 2026

Copy link
Copy Markdown

📦 SDK Size Impact Report

What the SDK adds to a minified release APK.

Measured against an empty baseline app. Unlike the Rokt kit, android-core ships no Compose and no resources, so there is nothing here that a host app would already provide.

mParticle Core SDK

Metric Target branch This PR Change
APK size 119.76 KB 119.76 KB +7 bytes
Download size 117.58 KB 117.58 KB +7 bytes
Dex bytes 213.75 KB 213.75 KB 0 bytes

➡️ SDK size impact change is minimal.

Raw measurements

Target branch:

{"baseline_dex_bytes": 0, "baseline_download_bytes": 2513, "baseline_install_bytes": 7529, "core_dex_bytes": 218884, "core_download_bytes": 122911, "core_install_bytes": 130161}

This PR:

{"baseline_dex_bytes": 0, "baseline_download_bytes": 2511, "baseline_install_bytes": 7527, "core_dex_bytes": 218884, "core_download_bytes": 122916, "core_install_bytes": 130166}

Measured 7a82061 merged into 38bf857

@nickolas-dimitrakas nickolas-dimitrakas self-assigned this Sep 17, 2026
thomson-t
thomson-t previously approved these changes Sep 18, 2026
…class allowlist

Three follow-ups from review:

- MethodCall.resolve() invoked any method matching name+arity on
  whatever its parent resolved to, with no check of its own. An
  allowed instance's inherited getClass() returns a java.lang.Class,
  which is not itself allowed, so chaining a further call onto that
  (e.g. Class.forName, reachable via ordinary instance-call syntax)
  went through unchecked. Gate the resolved instance before searching
  for a matching method, not just at construction time.

- Constructor.resolve() still returned the raw Class object when no
  matching constructor was found or newInstance() threw, instead of
  null - the same java.lang.Class leak as above, reachable without
  even needing getClass().

- StaticFactory checked the allowlist using the class name a compiled
  reference resolves to, but Kotlin multifile-class functions like
  mapOf/listOf resolve to a part class (e.g.
  kotlin.collections.MapsKt__MapsKt), not the bare facade
  (kotlin.collections.MapsKt) - which also declares no methods of its
  own, so the check could never pass. Normalize the "__Part" suffix
  for the allowlist lookup (Class.forName still uses the original
  name), and restrict to the specific factory methods actually needed
  instead of every declared method on the class.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@nickolas-dimitrakas
nickolas-dimitrakas merged commit 06d9832 into main Sep 21, 2026
44 checks passed
@nickolas-dimitrakas
nickolas-dimitrakas deleted the fix/dataplan-lint-reflection-allowlist branch September 21, 2026 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants