mirror of
https://github.com/vitorpamplona/amethyst.git
synced 2026-10-05 19:28:25 +00:00
fix: stop comparing a class name R8 renames, and scope the contract per flavor
Audit of what still reflects now that obfuscation is on. Two live defects, both
invisible in a debug build:
1. AmethystAppFunctions.requireInProcessSigner() compared
`signer::class.qualifiedName` against the fully-qualified name of
NostrSignerExternal, with a comment explaining that this avoided an import.
R8 renames that class — `NostrSignerExternal -> nbc` in the release mapping —
so the comparison never matched and the external-signer branch stopped
running the moment obfuscation was enabled. A user on Amber invoking a write
AppFunction from Gemini would fall through to the write attempt instead of
the typed "not supported here" message. It is a plain `is` check now; the
class is already imported directly by two other files in this source set.
2. The reflection contract listed AmethystCastOptionsProvider unscoped, but that
class only exists in the play flavor, and the release workflow runs the
verifier for playRelease AND fdroidRelease. Absent from a mapping reads as
"R8 deleted it", so the next release would have failed on fdroid. Measured,
not guessed: the old contract reports 2 of 21 checks failed against a real
fdroidRelease mapping. Contract lines may now open with @play / @fdroid, the
verifier derives the flavor from the mapping directory, and an unrecognised
directory still checks everything.
Also from the audit:
- Deleted 16 keep rules that matched nothing: libscrypt (com.lambdaworks),
NetCipher (info.guardianproject), LazySodium and the whole JNA block.
None of those artifacts appear in mapping.txt, usage.txt or seeds.txt —
quartz replaced libsodium with LibSodiumInstance and Tor runs through arti
now. Two of them were `-keep class * implements …` wildcards.
- Added NwcErrorCode to the contract. Its constant names are the NIP-47 wire
strings: the response parser calls NwcErrorCode.valueOf() on a string
another wallet wrote, inside a try/catch that falls back to null. The
blanket enum rule covers it today, which is exactly why it needs recording
before that rule is retired.
- Added AmethystAppFunctions (@play), so the one package keep that had no
contract line is no longer unverified.
Everything else that reflects is either a library that ships its own consumer
rules (appfunctions, kotlinx-serialization companions, WorkManager, lifecycle
ViewModel constructors, Startup, Cast, AppSearch) or names a class R8 cannot
rename: quic's JdkCertificateValidator probes the *platform* trust manager for
the 3-arg checkServerTrusted, and every setClassName() target is a
manifest-declared component that AAPT2 already generates a keep for.
Verified: 21/21 on a fresh playRelease, 19/19 + 2 skipped on fdroidRelease,
both from full assembles.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DEoxktEZyTrAS33vVZBiwm
This commit is contained in:
Claude
@@ -46,8 +46,7 @@ name.** In this app those are, exhaustively:
|
||||
|---|---|---|
|
||||
| JNI symbol `Java_<class>_<method>` | `ArtiNative`, secp256k1 | covered by the default `native <methods>` 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 <pkg>.** { *; }` |
|
||||
| Generated code reflected over by a library | the AppFunctions bridge (`appfunctions.**`, play flavor only) | `-keep class <pkg>.** { *; }` |
|
||||
| Enum constant persisted as a string | `UISharedPreferences` writes `enum.name`, reads `Type.valueOf(s)` | `-keepclassmembers enum * { <fields>; … }` |
|
||||
| Class name in a manifest `<meta-data android:value>` | `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 { <init>(...); }` |
|
||||
@@ -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
|
||||
|
||||
Vendored
+7
-29
@@ -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
|
||||
|
||||
+8
-7
@@ -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 — " +
|
||||
|
||||
@@ -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 <fqn> class must keep its exact name
|
||||
# method <fqn> <name>... those methods must keep their names
|
||||
# fields <fqn> class name AND every field name preserved
|
||||
@@ -66,13 +84,18 @@ method com.vitorpamplona.amethyst.ui.tor.ArtiLogCallback onLogLine
|
||||
# <meta-data android:value> 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
|
||||
|
||||
@@ -35,7 +35,17 @@ MEMBER_LINE = re.compile(
|
||||
)
|
||||
|
||||
|
||||
FLAVORS = ("play", "fdroid")
|
||||
|
||||
|
||||
def parse_contract(path):
|
||||
"""Read the contract. A line may open with @<flavor> 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 '<kind> <fqn> [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
|
||||
|
||||
|
||||
|
||||
Reference in New Issue
Block a user