self package targeting for side by side install revanced coregms#308
self package targeting for side by side install revanced coregms#308zappybiby wants to merge 7 commits intoReVanced:mainfrom
Conversation
zappybiby
changed the title
zappybiby
changed the title
This can be optimized with a compile time constant, sparing resolving the package name dynamically. Check if this reduces the amount of code changes needed. (BuildConfig.APPLICATION_ID like you already used works well) |
| private fun sendUserConsentBroadcast(request: SmsRetrieverRequest, messageBody: String) { | ||
| val userConsentIntent = Intent(context, UserConsentPromptActivity::class.java) | ||
| userConsentIntent.setPackage(Constants.GMS_PACKAGE_NAME) | ||
| userConsentIntent.setPackage(context.packageName) |
There was a problem hiding this comment.
I am wondering why upstream decided to use the constant here instead of context.packageName?
There was a problem hiding this comment.
I assume upstream used the constant simply because, at the time, canonical com.google.android.gms was assumed to be the app’s own package, so there was no practical difference from context.packageName.
I tested revanced coregms + stock gms with Constants.GMS_PACKAGE_NAME:
- actual serving APK: app.revanced.android.gms
- activity component in intent: app.revanced.android.gms/...UserConsentPromptActivity
- package field in intent: com.google.android.gms
- result: still launches ReVanced activity because the intent was already explicit before setPackage(...) was called. but package field points at the wrong app.
So effectively,
- Intent(context, UserConsentPromptActivity::class.java) set the component to
app.revanced.android.gms/...UserConsentPromptActivity - setPackage(Constants.GMS_PACKAGE_NAME) then added package=com.google.android.gms
So the change to context.packageName does not change what app receives the SMS Retriever result. It only fixes which GmsCore package this internal consent activity launch is addressed to. In the ReVanced GmsCore + stock GMS setup, that internal activity belongs to ReVanced GmsCore, not to com.google.android.gms, so context.packageName is the correct self-target here.
There was a problem hiding this comment.
context.packageName is the correct self-target here.
Would it be possible to use BuildConfig.APPLICATION_ID? Using a constant would match the original code more because upstream also used a constant.
There was a problem hiding this comment.
It's a library module so we can't directly use application id here (you could pipe it in, but that would be messy and not ideal for one use)
The most direct replacement currently is the user_microg_package_name constant. But I assume we want to do something with that, whether we deprecate it or rename it.
play-services-base/core/src/main/java/org/microg/gms/common/PackageUtils.java
Outdated
Show resolved
Hide resolved
play-services-core/src/main/java/com/google/android/gms/chimera/DynamiteContextFactory.java
Outdated
Show resolved
Hide resolved
play-services-core/src/main/java/org/microg/gms/auth/login/LoginActivity.java
Outdated
Show resolved
Hide resolved
...rvices-core/src/main/kotlin/org/microg/gms/auth/credentials/identity/AuthorizationService.kt
Outdated
Show resolved
Hide resolved
play-services-maps/src/main/java/org/microg/gms/maps/MapsRemoteContextHolder.java
Outdated
Show resolved
Hide resolved
Stop using canonical package identity at self-targeting call sites that actually mean the installed GmsCore APK, while keeping the Maps change narrowly scoped to the Mapbox backend path. For Maps, keep the loader and other backends unchanged and only stop the Mapbox backend from reconstructing a canonical com.google.android.gms context. Resolve the package context from the serving runtime application id instead, and keep MultiArchLoader version/APK lookup aligned with that Mapbox context. Also drop the transient self-package helpers and use direct package resolution at each call site: BuildConfig.APPLICATION_ID in play-services-core, and inline self-package context resolution in library modules that do not have the app BuildConfig.
The package-rename diagnostics showed that the Vision/MLKit face detector creators were still using the canonical package context even though the actual detector helper and model asset are owned by the serving self APK. Apply that ownership decision to all four remaining Vision/MLKit face detector creators by resolving the installed self package directly from the incoming context instead of going through a shared helper.
Runtime checks showed these LoginActivity package targets are self-owned flows. The signup handoff already resolved to LoginActivity's internal MainActivity component; the remaining issue was that the package field still pointed at canonical com.google.android.gms instead of the serving GmsCore app. The post-login ACTION_GCM_REGISTER_ACCOUNT broadcast also needs to target the receiver in the serving GmsCore app, not a separate canonical GMS package. Because LoginActivity is in the app module, BuildConfig.APPLICATION_ID is the simplest self-package constant for both call sites.
zappybiby
force-pushed
the
gmscore-pkg-rename
branch
from
5934c9d to
3fb21f2
Compare
|
Was this supposed to be closed? |
play-services-core/src/main/java/org/microg/gms/games/GamesStubService.java
Outdated
Show resolved
Hide resolved
play-services-core/src/main/kotlin/org/microg/gms/accountsettings/ui/MainActivity.kt
Outdated
Show resolved
Hide resolved
| import java.io.File | ||
|
|
||
| class MapContext(private val context: Context) : ContextWrapper(context.createPackageContext(Constants.GMS_PACKAGE_NAME, Context.CONTEXT_INCLUDE_CODE or Context.CONTEXT_IGNORE_SECURITY)) { | ||
| class MapContext(private val context: Context) : ContextWrapper( |
There was a problem hiding this comment.
context.packageName can't be used (that can be the client app’s package in this path) and app BuildConfig isn't directly accessible
Do you just want to use Constants.USER_MICROG_PACKAGE_NAME? a new constant? Relates to MultiArchLoader
There was a problem hiding this comment.
Do you just want to use Constants.USER_MICROG_PACKAGE_NAME? a new constant? Relates to MultiArchLoader
I see. Well, here's the deal with our fork of GmsCore. GmsCore upstream has a new flavor for user installs of GMS. ReVanced GmsCore, technically matches this flavor. In other words, we should build and serve the user flavor of GmsCore to everyone. However, using Constants.USER_MICROG_PACKAGE_NAME would be wrong because if we compile the regular flavor, the package name would be incorrect. We require a constant, that matches the current package name. The hack below works. However, what is the reason BuildConfig isn't directly accessible? Maybe it just requries toggling a flag in the Gradle build file? If so, then we can simply use BuildConfig, like it is possible everywhere else.
There was a problem hiding this comment.
Because BuildConfig.APPLICATION_ID inside MapContext would refer to the mapbox library BuildConfig, not the app’s.
this is the BuildConfig you get: org.microg.gms.maps.mapbox.BuildConfig
But APPLICATION_ID exists on the app generated class: com.google.android.gms.BuildConfig
So:
- the library can access its own
BuildConfig - its own
BuildConfigdoes not containAPPLICATION_ID
The library cannot import the app’s BuildConfig because the app is built on top of the library, not the other way around.
So BuildConfig.APPLICATION_ID does not work directly because it means “this module’s BuildConfig,” and in that module, that field is not there.
I wired it into the gradle build.
| val cacheVersion = kotlin.runCatching { cacheFileStamp.readText() }.getOrNull() | ||
| // TODO: Use better version indicator | ||
| val mapVersion = PackageUtils.versionName(mapContext, Constants.GMS_PACKAGE_NAME) | ||
| val mapVersion = PackageUtils.versionName(mapContext, mapContext.applicationInfo.packageName) |
There was a problem hiding this comment.
The alternative to this would be using Constants.USER_MICROG_PACKAGE_NAME - just trying to figure out what we want to handle USER_MICROG_PACKAGE_NAME going forward.
There was a problem hiding this comment.
|
Should the issues related to this PR be added to it so that they get automatically closed and so that people can more easily find it? |
|
In the opening message of this PR you can add "Closes #number" as many times as you want |
|
Does this PR also solve notifications, apparently people don't receive them currently |
I know but I did not create the PR so I can't edit the opening message. |
|
I don't want to pivot too far off the main focus of the PR, but I did see the patched YTM app did send me this notification, which I believe is remote? You can see in my screenshot that both the stock app and the patched app sent it (YT Music Revanced is the 9:34 notification) I'm not sure what exactly notification-wise wasn't working for users, I don't normally have notifications on.
Remote notification diagnostic excerptIs there an existing issue for notifications to discuss that part more? And @oSumAtrIX do you have thoughts on the microg package constant I mentioned in my comments? The PR functionally is essentially complete for the scope I wanted to cover, I verified the call sites resolve to correct package. |
| val context = context.unwrap<Context>() ?: throw RuntimeException("Context is null") | ||
| val remoteContext = GooglePlayServicesUtil.getRemoteContext(context) ?: throw RuntimeException("remoteContext is null") | ||
| Log.d(TAG, "newFaceDetector: context: ${context.packageName} remoteContext: ${remoteContext.packageName}") | ||
| val detectorContext = context.createPackageContext(context.applicationContext?.packageName ?: context.packageName, 0) |
There was a problem hiding this comment.
What is the reason for this change here? Why was remoteContext changed to detectorContext?
Whats the reason GooglePlayServicesUtil.getRemoteContext cant be used here? Is it internally using the original GMS package name?
| val context = context.unwrap<Context>() ?: throw RuntimeException("Context is null") | ||
| val remoteContext = GooglePlayServicesUtil.getRemoteContext(context) ?: throw RuntimeException("remoteContext is null") | ||
| Log.d(TAG, "newFaceDetector: context: ${context.packageName} remoteContext: ${remoteContext.packageName}") | ||
| val detectorContext = context.createPackageContext(context.applicationContext?.packageName ?: context.packageName, 0) |
There was a problem hiding this comment.
| val context = context.unwrap<Context>() ?: throw RuntimeException("Context is null") | ||
| val remoteContext = GooglePlayServicesUtil.getRemoteContext(context) ?: throw RuntimeException("remoteContext is null") | ||
| Log.d(TAG, "newFaceDetector: context: ${context.packageName} remoteContext: ${remoteContext.packageName}") | ||
| val detectorContext = context.createPackageContext(context.applicationContext?.packageName ?: context.packageName, 0) |
There was a problem hiding this comment.
| val context = context.unwrap<Context>() ?: throw RuntimeException("Context is null") | ||
| val remoteContext = GooglePlayServicesUtil.getRemoteContext(context) ?: throw RuntimeException("remoteContext is null") | ||
| Log.d(TAG, "ThickFaceDetectorCreator newFaceDetector: context: ${context.packageName} remoteContext: ${remoteContext.packageName}") | ||
| val detectorContext = context.createPackageContext(context.applicationContext?.packageName ?: context.packageName, 0) |
There was a problem hiding this comment.
oSumAtrIX
left a comment
oSumAtrIX
left a comment
There was a problem hiding this comment.
Apart from the review, rest lgtm. I would like to understand a last question apart from those:
How did you determine the completeness of the changes in this PR? If you just did it by skimming the code, there is possibilty that we missed changing something. Is there maybe a systematic approach to verify, that the fix is applied correctly everywhere where applicable?
Users dont receive notifications, i am not sure if this PR solves it, but since you receive notifications (with this PR) I assume it'll fix it too. |
Using androguard, Common package rename sinks reviewedStrings reviewedI looked at strings containing: Code paths reviewed for possible package renamingI looked at code that could possibly relate to package renaming There were only two exercised runtime buckets that did not immediately collapse to a simple “app targeted ReVanced GmsCore and Android resolved ReVanced GmsCore” result. After investigating them, they do not currently look like missed rename call sites where ReVanced functionality already exists, and I don't see further changes that need to be made. Cronet HTTP-flags probeThe first is the Cronet HTTP-flags probe, which is a stock-only external probe: {"package":"app.revanced.android.youtube","label":"rename-sink-pm-resolve-service","payload":{"intent":{"action":"android.net.http.FLAGS_FILE_PROVIDER"},"match":{"package":"com.google.android.gms","name":"com.google.android.gms.chimera.GmsApiService"},"callSite":"bgrj#run:78"}}Media-route discoveryThe second is media-route discovery. It was worth mentioning because the discovery query returns both stock and ReVanced matches, which is exactly the kind of output that could hide a missed rename if the app later chose the wrong target. In the exercised path here, that is not what happened: discovery is dual at query time, but the later bind is still ReVanced GmsCore: {"package":"app.revanced.android.youtube","label":"rename-sink-pm-query-intent-services","payload":{"intent":{"action":"android.media.MediaRouteProviderService"},"matches":[{"package":"com.google.android.gms","name":"com.google.android.gms.cast.media.CastMediaRoute2ProviderService_Persistent"},{"package":"app.revanced.android.gms","name":"com.google.android.gms.cast.media.CastMediaRouteProviderService"}],"callSite":"dfy#b:93"}}
{"package":"app.revanced.android.youtube","label":"rename-sink-intent-set-component","payload":{"requestedComponent":"app.revanced.android.gms/com.google.android.gms.cast.media.CastMediaRouteProviderService","emittedIntent":{"action":"android.media.MediaRouteProviderService","component":"app.revanced.android.gms/com.google.android.gms.cast.media.CastMediaRouteProviderService"},"callSite":"dfw#f:14"}}Missing-endpoint casesSeparately from those two live runtime exceptions, there are also a few missing-endpoint cases ReVanced GmsCore does not currently expose the corresponding endpoint/module.
NotificationsLastly, for notifications:
YouTube: YT Music: The only currently stock pointing notification related call site is ACTION_SCHEDULE. ACTION_SCHEDULE is for GcmNetworkManager, which is an old background job scheduler API.
My Analysis found no broadcast receiver in app.revanced.android.gms that can handle I do not see any references to ACTION_SCHEDULE in YouTube or YouTube Music apk, so I do not think this finding has any relevance. Summary: With the few stock-only call sites known, I don't see further missed renamed sites that aren't part of the few niche call sites that don't have a matching Revanced component we could switch them to. |
zappybiby
force-pushed
the
gmscore-pkg-rename
branch
from
e0eef99 to
12227d7
Compare
There was a problem hiding this comment.
To make BuildConfig.APPLICATION_ID resolve to the correct package for default vs user, we need to add the target flavors to play-services-vision/core itself. That changes the module from a flavorless published library into a flavored published library. Because play-services-vision/core also uses this shared publish script, it necessitates a small adjustment. Without that adjustment, it still assumes a single plain release variant and fails when building.
There was a problem hiding this comment.
I am not sure why this this has been changed. Is this not an issue with upstream gmscore?
There was a problem hiding this comment.
It only had to be changed because you specified you wanted compile time resolution instead of at runtime.
To do this, we need play-services-vision/core/build.gradle to generate a compile-time BuildConfig.APPLICATION_ID.
A library module only gets its own BuildConfig. If that library stays flavorless, it can only generate one package constant. In this repo, the app still has two app package values app.revanced.android.gms and com.google.android.gms
Once we switch that module to compile-time BuildConfig.APPLICATION_ID, the module itself has to know which target package it was built for. That means adding the target flavors to play-services-vision/core. And because play services-vision/core is also published through the shared Android publish script, that new flavored module shape is what forces the small publish-android.gradle adjustment.
This change is required if you want compile-time BuildConfig.APPLICATION_ID. It is not required if you are okay with runtime package resolution, something like what was in the previous commit.
This isn't an issue with upstream coregms, it doesn't need play-services-vision/core to be flavored because It just uses one runtime helper that always resolves the canonical package.
There was a problem hiding this comment.
This isn't an issue with upstream coregms, it doesn't need play-services-vision/core to be flavored because It just uses one runtime helper that always resolves the canonical package.
What does "canonical package" mean here? Does it refer to the actual package name of the app? So if we change the package name, is it reflected in the canonical package too? If upstream used a runtime helper here, then we have no requirement to change that. You said:
You wanted compile time resolution
But this was relating to lines from upstream that were already compile time. If a line used Constants.GMS_PACKAGE_NAME then we also want to use a compile time constant, such as BuildConfig.APPLICATION_ID. But play-services-vision/core used
val remoteContext = GooglePlayServicesUtil.getRemoteContext(context) ?: throw RuntimeException("remoteContext is null")
If we check the implementation (here)[https://github.com/ReVanced/GmsCore/blob/main/play-services-base/src/main/java/com/google/android/gms/common/GooglePlayServicesUtil.java#L138] you will see it uses Constants.GMS_PACKAGE_NAME again. So why are we not simply replacing Constants.GMS_PACKAGE_NAME with BuildConfig.APPLICATION_ID?
There was a problem hiding this comment.
Every usage of getRemoteContext in this PR effectively is replaced with
context.createPackageContext(BuildConfig.APPLICATION_ID, Context.CONTEXT_INCLUDE_CODE or Context.CONTEXT_IGNORE_SECURITY)
So why not change the implementation of getRemoteContext so that instead of Constants.GMS_PACKAGE_NAME it uses BuildConfig.APPLICATION_ID.
Just to understand, you believe this is complete, but you can't verify it is? It's fine though, i think this second review you did should give enough confidence for completeness. |
What I did was the static analysis with androguard as I described. I also attached runtime probes to GmsCore and a diagnostic patch for Revanced YT/YTMusic to ensure call sites are being correctly resolved. I feel quite confident that all potential call sites have been addressed. Im not worried about the small things I saw that only have a stock gms functionality as I don't think they have any significant impact. I already verified that one notification finding is essentially an older API that isnt used in either YT or YTMusic. I am not familiar with tools like Frida so that kind of dynamic analysis is not something I could do. |
4 participants
Several call sites were still using GMS_PACKAGE_NAME even when they were really trying to target the currently installed GmsCore APK. This change switches those sites to runtime self-package resolution, while keeping the remaining explicit package cases unchanged.
Goals of the patch:
If a call site means "this installed GmsCore APK", use the runtime self package.
If a call site means the canonical Google package identity for protocol, network, spoofing, or compatibility, keep Constants.GMS_PACKAGE_NAME.
Do not use USER_MICROG_PACKAGE_NAME as a generic stand-in for "self".
Do not derive remote Maps / Dynamite resource contexts from a caller or embedding-app Context.
As part of the changes, the identified issue affecting patched YT Music app and Android Auto (DynamiteContextFactory) is fixed. DynamiteContextFactory no longer resolves the GmsCore package context through GMS_PACKAGE_NAME, and now uses BuildConfig.APPLICATION_ID instead. That keeps the YT Music on Android Auto Dynamite path working for renamed/repackaged installs.
Closes ReVanced/revanced-patches#6602