diff --git a/.claude/skills/android-expert/references/proguard-rules.md b/.claude/skills/android-expert/references/proguard-rules.md index c5d4aac9b2..185ef708f1 100644 --- a/.claude/skills/android-expert/references/proguard-rules.md +++ b/.claude/skills/android-expert/references/proguard-rules.md @@ -46,8 +46,7 @@ name.** In this app those are, exhaustively: |---|---|---| | JNI symbol `Java__` | `ArtiNative`, secp256k1 | covered by the default `native ` rule | | Native code calling *back* by name | `ArtiLogCallback.onLogLine`, looked up with `GetMethodID` in `tools/arti-build/src/lib.rs` | `-keep class …ArtiLogCallback { *; }` | -| JNA struct/callback mapping | lazysodium | `-keep class com.goterl.lazysodium.** { *; }` | -| Jackson **reflective** data binding | `nip47WalletConnect.rpc.**`, `experimental.clink.**` | `-keep class .** { *; }` | +| Generated code reflected over by a library | the AppFunctions bridge (`appfunctions.**`, play flavor only) | `-keep class .** { *; }` | | Enum constant persisted as a string | `UISharedPreferences` writes `enum.name`, reads `Type.valueOf(s)` | `-keepclassmembers enum * { ; … }` | | Class name in a manifest `` | `AmethystCastOptionsProvider` | explicit `-keep` — AGP generates keeps from component `android:name`, **not** from meta-data | | Class name in WorkManager's database | the three `CoroutineWorker`s | `-keep class * extends androidx.work.ListenableWorker { (...); }` | @@ -156,6 +155,20 @@ collecting assets, so a break fails the release instead of shipping. **Adding reflection means adding two things**: the keep rule, and a line in the contract. A rule with no contract line is unverified and will rot. +Scope a line to one flavor with a leading `@play` / `@fdroid` when the class is +only compiled into that variant — play-only code is absent from the F-Droid APK, +and "absent" is indistinguishable from "R8 deleted it", so an unscoped line for +it fails that release. + +Reflection that names a class R8 *cannot* rename needs no rule and no line: the +platform trust manager `quic` probes for a 3-arg `checkServerTrusted` ships in +Android, not in our DEX. The opposite case is the one to watch — a name of +*ours* used as a value. `AmethystAppFunctions` compared +`signer::class.qualifiedName` against `"…NostrSignerExternal"`; R8 renames that +class, so the branch silently stopped running the day obfuscation was turned on. +Prefer `is` over a name comparison; there is no keep rule that makes the latter +safe to write. + It reads both `mapping.txt` and `usage.txt`, because neither is enough alone — mapping.txt records only what *changed* (an intact `-keep ... { *; }` class has an empty body, and an unrenamed member has no line at all, so absence there diff --git a/amethyst/proguard-rules.pro b/amethyst/proguard-rules.pro index 3cd8e417f7..a9decf35f4 100644 --- a/amethyst/proguard-rules.pro +++ b/amethyst/proguard-rules.pro @@ -94,35 +94,13 @@ # secp256k1's JNI layer resolves these from native code. -keep class fr.acinq.secp256k1.** { *; } -# libscrypt --keep class com.lambdaworks.codec.** { *; } --keep class com.lambdaworks.crypto.** { *; } --keep class com.lambdaworks.jni.** { *; } - --keep class info.guardianproject.** { *; } - -# JNA for Libsodium: JNA maps these types onto the C ABI by reflecting over -# their fields and method signatures at runtime. --keep class com.goterl.lazysodium.** { *; } - -# JNA also requires AWT, which Android does not have. So the classes are broken down to filter AWT out --keep class com.sun.jna.ToNativeConverter { *; } --keep class com.sun.jna.NativeMapped { *; } --keep class com.sun.jna.CallbackReference { *; } --keep class com.sun.jna.ptr.IntByReference { *; } --keep class com.sun.jna.NativeLong { *; } --keep class com.sun.jna.Structure { *; } --keep class com.sun.jna.Structure$* { *; } --keep class com.sun.jna.Native$ffi_callback { *; } --keep class * implements com.sun.jna.Structure$* { *; } --keep class * implements com.sun.jna.Native$* { *; } --keep class com.sun.jna.Native { - private static com.sun.jna.NativeMapped fromNative(java.lang.Class, java.lang.Object); - private static com.sun.jna.NativeMapped fromNative(java.lang.reflect.Method, java.lang.Object); - private static java.lang.Class nativeType(java.lang.Class); - private static java.lang.Object toNative(com.sun.jna.ToNativeConverter, java.lang.Object); - private static java.lang.Object fromNative(com.sun.jna.FromNativeConverter, java.lang.Object, java.lang.reflect.Method); -} +# Nothing keeps libscrypt, NetCipher/tor-android, LazySodium or JNA any more: +# quartz replaced libsodium with a pure-Kotlin implementation (LibSodiumInstance) +# and Tor now runs through arti's own JNI layer. Their rules used to live here and +# matched zero classes in the release build — none of those artifacts appear in +# mapping.txt, usage.txt or seeds.txt, i.e. they are not on the classpath at all. +# If one ever comes back, so must its rule: JNA in particular maps types onto the +# C ABI by reflecting over their fields and method signatures at runtime. # ----------------------------------------------------------------------------- # Enum constant names diff --git a/amethyst/src/play/java/com/vitorpamplona/amethyst/appfunctions/AmethystAppFunctions.kt b/amethyst/src/play/java/com/vitorpamplona/amethyst/appfunctions/AmethystAppFunctions.kt index 08a2da1826..a0dd8d1bf9 100644 --- a/amethyst/src/play/java/com/vitorpamplona/amethyst/appfunctions/AmethystAppFunctions.kt +++ b/amethyst/src/play/java/com/vitorpamplona/amethyst/appfunctions/AmethystAppFunctions.kt @@ -60,6 +60,7 @@ import com.vitorpamplona.quartz.nip47WalletConnect.rpc.PayInvoiceErrorResponse import com.vitorpamplona.quartz.nip47WalletConnect.rpc.PayInvoiceSuccessResponse import com.vitorpamplona.quartz.nip47WalletConnect.rpc.Response import com.vitorpamplona.quartz.nip53LiveActivities.streaming.LiveActivitiesEvent +import com.vitorpamplona.quartz.nip55AndroidSigner.client.NostrSignerExternal import com.vitorpamplona.quartz.nip57Zaps.LnZapEvent import com.vitorpamplona.quartz.nip59Giftwrap.wraps.GiftWrapEvent import com.vitorpamplona.quartz.nipB1Bolt12Zaps.zap.Bolt12ZapEvent @@ -1907,13 +1908,13 @@ class AmethystAppFunctions { "Active Amethyst account is read-only (npub login). Sign in with a private key or NIP-46 bunker to publish.", ) } - // NostrSignerExternal lives in quartz/androidMain and isn't visible - // to commonMain — but we're already in android-app code, so the - // class is on the classpath. Reflective name-check keeps the - // dependency edge clean and avoids hard-coupling the bridge to - // the NIP-55 implementation class. - val klass = signer::class.qualifiedName - if (klass == "com.vitorpamplona.quartz.nip55AndroidSigner.client.NostrSignerExternal") { + // NostrSignerExternal lives in quartz/androidMain, which is on the + // classpath here because this is android-app code. This used to compare + // `signer::class.qualifiedName` against the fully-qualified name to + // avoid the import; R8 renames the class in release builds, so that + // comparison silently stopped matching and the external-signer branch + // never ran. A type check has no such failure mode. + if (signer is NostrSignerExternal) { throw AppFunctionNotSupportedException( "Amethyst is configured to use an external NIP-55 signer (Amber). " + "Write actions from Gemini aren't supported with this signer yet — " + diff --git a/tools/r8-verify/reflection-contract.txt b/tools/r8-verify/reflection-contract.txt index 9cb556da50..bdc06ba5d5 100644 --- a/tools/r8-verify/reflection-contract.txt +++ b/tools/r8-verify/reflection-contract.txt @@ -35,6 +35,20 @@ # route at the hand-written kotlinx serializers # on every target now, and the Jackson # (de)serializers for them are gone. +# Class name as a value REMOVED. requireInProcessSigner() compared +# `signer::class.qualifiedName` against the FQN +# of NostrSignerExternal. R8 renames that class, +# so the branch never ran in a release build. It +# is a plain `is` check now — nothing to keep. +# Platform-class reflection IRREDUCIBLE AND FREE. quic's +# JdkCertificateValidator probes the JDK/Android +# trust manager for the 3-arg +# checkServerTrusted(chain, authType, host). +# That class ships in the platform, not in our +# DEX, so R8 never renames it — no rule needed. +# libscrypt / NetCipher / GONE. Their keep rules matched zero classes: +# LazySodium / JNA libsodium is a pure-Kotlin implementation now +# and Tor runs through arti. Rules deleted. # Jackson on-disk stores REMOVABLE. Plain data classes; @Serializable # + kotlinx gives compile-time literal names. # App-private files, so no interop risk. @@ -49,6 +63,10 @@ # blocks enum unboxing across all 707 enums. # # Format — one per line, `#` comments to end of line: +# [@play|@fdroid] optional leading scope: check this line only on +# that flavor. Play-only code is absent from the +# F-Droid APK, and "absent" reads as "R8 deleted +# it" — an unscoped line would fail that release. # class class must keep its exact name # method ... those methods must keep their names # fields class name AND every field name preserved @@ -66,13 +84,18 @@ method com.vitorpamplona.amethyst.ui.tor.ArtiLogCallback onLogLine # in the play manifest; the Cast framework # Class.forName()s it. AGP generates keeps for component android:name, not for # meta-data values, so nothing but an explicit rule protects this one. -class com.vitorpamplona.amethyst.service.cast.chromecast.AmethystCastOptionsProvider +@play class com.vitorpamplona.amethyst.service.cast.chromecast.AmethystCastOptionsProvider # WorkManager stores the class name in its own DB and instantiates it by name on # a later process start — including after an update that reshuffled the mapping. class com.vitorpamplona.amethyst.service.scheduledposts.ScheduledPostWorker class com.vitorpamplona.amethyst.service.notifications.NotificationCatchUpWorker class com.vitorpamplona.amethyst.service.calendar.CalendarReminderWorker +# The AppFunctions bridge is kept whole by a package rule, so this asserts the +# rule still matches something. androidx.appfunctions reflects over the generated +# inventories and @AppFunctionSerializable types; play-only source set. +@play class com.vitorpamplona.amethyst.appfunctions.AmethystAppFunctions + # --- Jackson reflective data binding: field names ARE the wire format --------- # NIP-47 and CLINK used to be listed here. They are not data-bound reflectively # any more — OptimizedJsonMapper routes them at kotlinx serializers that write @@ -85,6 +108,14 @@ class com.vitorpamplona.amethyst.service.calendar.CalendarReminderWorker # the first launch after the update — silently, and only in a release build. # The enum CLASS is still renamed; only the constant names are pinned. enum com.vitorpamplona.amethyst.commons.tor.TorType INTERNAL + +# Not a preference: these constant names ARE the NIP-47 wire strings. The +# response parser does NwcErrorCode.valueOf(json["code"]) on a string another +# wallet wrote, so a rename turns every typed error into a null code and the +# UI loses the reason a payment failed. Falls back quietly (try/catch), which +# is exactly why it would never be noticed. +enum com.vitorpamplona.quartz.nip47WalletConnect.rpc.NwcErrorCode RATE_LIMITED NOT_IMPLEMENTED INSUFFICIENT_BALANCE PAYMENT_FAILED QUOTA_EXCEEDED RESTRICTED UNAUTHORIZED INTERNAL UNSUPPORTED_ENCRYPTION BAD_REQUEST NOT_FOUND EXPIRED UNSUPPORTED_PAYMENT_INSTRUCTION UNSUPPORTED_NETWORK OTHER + enum com.vitorpamplona.amethyst.model.ThemeType SYSTEM LIGHT DARK enum com.vitorpamplona.amethyst.model.BooleanType ALWAYS NEVER enum com.vitorpamplona.amethyst.model.ConnectivityType ALWAYS NEVER diff --git a/tools/r8-verify/verify_reflection_contract.py b/tools/r8-verify/verify_reflection_contract.py index cf098ef42d..80b3564b11 100755 --- a/tools/r8-verify/verify_reflection_contract.py +++ b/tools/r8-verify/verify_reflection_contract.py @@ -35,7 +35,17 @@ MEMBER_LINE = re.compile( ) +FLAVORS = ("play", "fdroid") + + def parse_contract(path): + """Read the contract. A line may open with @ to scope it. + + Play-only code (the Cast provider, the AppFunctions bridge) is not compiled + into the F-Droid APK at all, so an unscoped entry for it would read as + "R8 deleted this" on that variant and fail a release the moment CI checked + both. `@play class ...` limits the check to the variant that has the class. + """ entries = [] with open(path, encoding="utf-8") as fh: for lineno, raw in enumerate(fh, 1): @@ -43,13 +53,35 @@ def parse_contract(path): if not line: continue parts = line.split() + scope = None + if parts[0].startswith("@"): + scope = parts[0][1:] + if scope not in FLAVORS: + sys.exit(f"{path}:{lineno}: unknown flavor {scope!r}; " + f"expected one of {', '.join(FLAVORS)}") + parts = parts[1:] + if len(parts) < 2: + sys.exit(f"{path}:{lineno}: expected ' [names...]'") kind, fqn, rest = parts[0], parts[1], parts[2:] if kind not in ("class", "method", "fields", "enum"): sys.exit(f"{path}:{lineno}: unknown check kind {kind!r}") - entries.append((kind, fqn, rest, lineno)) + entries.append((kind, fqn, rest, lineno, scope)) return entries +def flavor_of(mapping_dir): + """playRelease -> play, fdroidRelease -> fdroid, anything else -> None. + + None means "check everything": an unrecognised directory must not silently + skip entries. + """ + variant = mapping_dir.rsplit("/", 1)[-1] + for flavor in FLAVORS: + if variant.startswith(flavor): + return flavor + return None + + def consistency_check(mapping_path, usage_path): """Are these two files from the SAME R8 run? @@ -169,7 +201,11 @@ def main(): return 1 entries = parse_contract(contract_path) - wanted = {fqn for _, fqn, _, _ in entries} + flavor = flavor_of(mapping_dir) + in_scope = [e for e in entries if e[4] is None or e[4] == flavor] + skipped = len(entries) - len(in_scope) + entries = in_scope + wanted = {fqn for _, fqn, _, _, _ in entries} blocks = collect(mapping_path, wanted) gone_classes, gone_members = collect_removed(usage_path, wanted) @@ -186,7 +222,7 @@ def main(): def fail(lineno, fqn, msg): failures.append(f" {contract_path}:{lineno} {fqn}\n {msg}") - for kind, fqn, rest, lineno in entries: + for kind, fqn, rest, lineno, _scope in entries: if fqn in gone_classes: fail(lineno, fqn, "deleted by R8 (listed in usage.txt) — nothing keeps it.") continue @@ -236,7 +272,8 @@ def main(): print("\nFix the keep rule in amethyst/proguard-rules.pro (or quartz/consumer-rules.pro)," "\nor update the contract if the code genuinely moved.", file=sys.stderr) return 1 - print(f"R8 reflection contract: all {checked} checks passed against {mapping_path}") + note = f" ({skipped} skipped: not in the {flavor} flavor)" if skipped else "" + print(f"R8 reflection contract: all {checked} checks passed against {mapping_path}{note}") return 0