From a04bdbd594428c9a22a0ec34d6c2108d623912f5 Mon Sep 17 00:00:00 2001 From: JingMatrix Date: Fri, 7 Aug 2026 13:01:28 +0200 Subject: [PATCH] Fix the scope requests that cannot be granted and the answers that are dropped "system" is the framework rather than a package: it names system_server, which belongs to no package and resolves for nobody. The receiver looked the requested package up before acting on the button, so every framework prompt died there -- the approval the user had just given was answered "Package not found", the request was closed, its notification cancelled and no row written. The approve branch normalises "system" to user 0, and that line could never once have run. Accept the framework name without asking the package manager, and move the lookup under "approve", which is the only answer that has to name something real: deny and the one-hour timeout were reported as "Package not found" too when a package had been uninstalled while the prompt was up, which told the module the opposite of what had happened. One request is now one prompt. The interface takes a list and a single IXposedScopeCallback for it, and the client library's listener is documented to run "when the request is completed", but the daemon put one prompt per package on screen and answered each in its own right: a module asking for three packages made the user answer three questions and then fired that one listener three times, and a module that took the first answer as the answer acted on a third of it. The whole list goes up as one prompt whose Approve answers for all of it, deduplicated and sorted so that the same set asked twice replaces its own prompt rather than stacking a second copy of the same question, and the per-module ceiling now bounds calls rather than packages. What the user gives away in one press is what the prompt lists, so it lists all of them; the notification reuses the string it always did, with the packages joined into it, so no translation changes. An approval was thrown away when the module's process had died. A prompt sits for an hour and the app a module runs inside can be killed at any point in it, and the receiver returned on a dead callback binder before claiming the answer or cancelling the notification: the user pressed Approve, nothing was written, and the prompt stayed on screen with live buttons that did nothing for the rest of the hour. An approval is a decision about the module's scope and is recorded whether or not the module is still there to be told, deny and the timeout still take the prompt down, and the only call that can fail against a dead module is caught where it is made. None of this said anything anywhere. The refusal path had no log line at all, so a scope request that could never be granted left no trace in the daemon log, in logcat or on screen, and the framework case above went unnoticed for as long as it did for that reason. Name the packages that did not resolve, and the ones that were approved. --- .../org/matrix/vector/daemon/VectorService.kt | 133 ++++++++++++------ .../vector/daemon/ipc/ModuleAppService.kt | 25 +++- .../daemon/system/NotificationManager.kt | 99 +++++++------ 3 files changed, 168 insertions(+), 89 deletions(-) diff --git a/daemon/src/main/kotlin/org/matrix/vector/daemon/VectorService.kt b/daemon/src/main/kotlin/org/matrix/vector/daemon/VectorService.kt index 23f83e06f..e0e2280b4 100644 --- a/daemon/src/main/kotlin/org/matrix/vector/daemon/VectorService.kt +++ b/daemon/src/main/kotlin/org/matrix/vector/daemon/VectorService.kt @@ -385,29 +385,39 @@ object VectorService : IVectorDaemon.Stub() { val packageName = parts[0] val userId = parts[1].toIntOrNull() ?: return - val scopePackageName = data.path?.substring(1) ?: return // remove leading '/' + // Everything the one prompt asked about, in the order it listed them. ',' cannot occur in a + // package name, so this is the list the module named and not a guess at it. + val scopePackageNames = + data.path?.substring(1)?.split(",")?.filter { it.isNotEmpty() } ?: return // strip '/' + if (scopePackageNames.isEmpty()) return val action = data.getQueryParameter("action") ?: return - // A prompt outlives the process that asked for it: it sits for an hour, and the app the module - // is running inside can be killed at any point in that hour. For approve, deny and the timeout - // there is then nobody left to tell, so those are dropped where they always were. "Never ask - // again" is not like them — it is the user's decision about the module, not an answer owed to a - // caller who is still listening — so it is honoured whether or not anyone is there to hear it, - // and the claim below still takes the prompt down. - if (!callbackBinder.isBinderAlive && action != "block") return - // One prompt reaches this receiver from four places — its three buttons and its delete intent // — and a swipe or the one-hour timeout fires the delete intent whether or not a button was // pressed first. Answering the module twice, an approval followed by a spurious "Request // timeout", would be worse than the dismissal never reaching it, so whichever of the four // arrives first is the one that answers and the rest are dropped. The request is identified by - // the module, its user and the package it asked for; the action is deliberately not part of - // that, since the whole point is that a second *different* action must not answer again. - if (!NotificationManager.claimScopeAnswer(packageName, userId, scopePackageName)) { - Log.d(TAG, "Ignoring $action of $scopePackageName for $packageName: already answered") + // the module, its user and the set of packages it asked for; the action is deliberately not + // part of that, since the whole point is that a second *different* action must not answer + // again. + if (!NotificationManager.claimScopeAnswer(packageName, userId, scopePackageNames)) { + Log.d( + TAG, + "Ignoring $action of ${scopePackageNames.joinToString()} for $packageName:" + + " already answered") return } + // A prompt outlives the process that asked for it: it sits for an hour, and the app the module + // is running inside can be killed at any point in that hour. Nothing is dropped for that any + // more. There used to be a `!callbackBinder.isBinderAlive` return above the claim, exempting + // only "never ask again", and it did more than lose an answer nobody was listening for: an + // approval the user had already given was thrown away, and because the return sat above the + // claim and the cancel, the prompt stayed on screen with live buttons that did nothing for the + // rest of the hour. An approval is a decision about the module's scope and is written down + // whether or not the module is there to hear it; deny and the timeout have nothing to record + // but still have a prompt to take down. What actually fails against a dead module is the + // callback below, and that is caught where it is made. val iCallback = IXposedScopeCallback.Stub.asInterface(callbackBinder) runCatching { // Answered before the requested package is looked up at all, because "never ask again" is @@ -419,12 +429,10 @@ object VectorService : IVectorDaemon.Stub() { // down, and the module was free to ask again a second later. if (action == "block") { blockScopeRequests(packageName) - // The preference only stops the *next* request. A module that asked for three packages - // has a prompt up for each, so without this the user says "never ask again" and is left + // The preference only stops the *next* request. A module that asked three times has a + // prompt up for each, so without this the user says "never ask again" and is left // looking at two more questions, both still approvable. Each withdrawn request is - // answered in its own right, because each of them was asked in its own right; what that - // costs a module whose listener expects one call is written out on - // withdrawScopeRequests. + // answered in its own right, because each of them was asked in its own right. NotificationManager.withdrawScopeRequests(packageName).forEach { pending -> runCatching { pending.onScopeRequestFailed("Request blocked by configuration") } } @@ -437,31 +445,72 @@ object VectorService : IVectorDaemon.Stub() { return@runCatching } - val appInfo = packageManager?.getPackageInfoCompat(scopePackageName, 0, userId) - if (appInfo == null) { - // Leaving the whole function here skipped the cancel below, which used to be merely - // untidy and is now a prompt nobody can use: the request has been answered, so every - // later press of its buttons is dropped. The request is over either way, so the - // notification goes with it. - iCallback.onScopeRequestFailed("Package not found") - return@runCatching - } when (action) { "approve" -> { + // "system" is the framework and not a package: it names system_server, which belongs + // to no package and resolves for nobody. The lookup below therefore came back null + // for every framework prompt, and the approval the user had just given was answered + // "Package not found" — the request was closed, its notification cancelled, and no + // row written. The normalisation to user 0 further down could never once have run. + // + // Package by package rather than all-or-nothing: the prompt may have been up for an + // hour and one of the packages it named can have been uninstalled in the meantime, + // which is no reason to throw away the user's answer about the rest. + // + // Only under "approve", because only an approval has to name something real. Deny + // and the timeout used to be refused here too, so dismissing a prompt for a package + // that had since been uninstalled told the module "Package not found" when what had + // actually happened was that the user turned it down. + val granted = + scopePackageNames.filter { + it == "system" || packageManager?.getPackageInfoCompat(it, 0, userId) != null + } + if (granted.isEmpty()) { + // Logged, because until now this said nothing anywhere: the module was told + // "Package not found", the user was told nothing at all, and the daemon kept no + // record that the press had even arrived. The framework-scope failure above was + // invisible for exactly that reason. + Log.w( + TAG, + "None of ${scopePackageNames.joinToString()} resolve for user $userId;" + + " refusing the scope request of $packageName") + // Leaving the whole function here skipped the cancel below, which used to be + // merely untidy and is now a prompt nobody can use: the request has been answered, + // so every later press of its buttons is dropped. The request is over either way, + // so the notification goes with it. + iCallback.onScopeRequestFailed("Package not found") + return@runCatching + } val scopes = ModuleDatabase.getModuleScope(packageName) ?: mutableListOf() - // Compared against where the row will land, not against the user who asked: the - // framework is stored under user 0 whoever requested it, so for "system" this test - // never matched and every approval appended a duplicate and rewrote the whole table. - val storedUserId = if (scopePackageName == "system") 0 else userId - if (scopes.none { it.packageName == scopePackageName && it.userId == storedUserId }) { - scopes.add( - ScopeEntry().apply { - this.packageName = scopePackageName - this.userId = storedUserId - }) - ModuleDatabase.setModuleScope(packageName, scopes) + var added = false + granted.forEach { scopePackageName -> + // Compared against where the row will land, not against the user who asked: the + // framework is stored under user 0 whoever requested it, so for "system" this test + // never matched and every approval appended a duplicate and rewrote the whole + // table. + val storedUserId = if (scopePackageName == "system") 0 else userId + val present = + scopes.any { it.packageName == scopePackageName && it.userId == storedUserId } + if (!present) { + scopes.add( + ScopeEntry().apply { + this.packageName = scopePackageName + this.userId = storedUserId + }) + added = true + } } - iCallback.onScopeRequestApproved(listOf(scopePackageName)) + // One write for the whole prompt, and none at all when the user approved what the + // module already had. `setModuleScope` replaces the module's rows wholesale and + // enables the module on the way through, so writing per package would rewrite the + // table once per package — leaving a window after each in which the scope is only + // partly what was agreed to — and writing unconditionally would let a module enable + // itself by asking again for what it has. + if (added) ModuleDatabase.setModuleScope(packageName, scopes) + Log.i(TAG, "Approved ${granted.joinToString()} for $packageName on user $userId") + // The packages that were granted, which is what the list in this callback is for. A + // module comparing it against what it asked for can see what it did not get. + iCallback.onScopeRequestApproved(granted) } "deny" -> iCallback.onScopeRequestFailed("Request denied by user") "delete" -> iCallback.onScopeRequestFailed("Request timeout") @@ -470,9 +519,9 @@ object VectorService : IVectorDaemon.Stub() { // onScopeRequestFailed declares @NonNull, and Throwable.message is frequently null. .onFailure { runCatching { iCallback.onScopeRequestFailed(it.message ?: it.toString()) } } - // Only this one request goes; a module that asked for several packages has a prompt still open - // for each of the others, and they are answered on their own. - NotificationManager.cancelScopeRequest(packageName, userId, scopePackageName) + // Only this one request goes; a module that asked more than once has a prompt still open for + // each of its other requests, and they are answered on their own. + NotificationManager.cancelScopeRequest(packageName, userId, scopePackageNames) } /** diff --git a/daemon/src/main/kotlin/org/matrix/vector/daemon/ipc/ModuleAppService.kt b/daemon/src/main/kotlin/org/matrix/vector/daemon/ipc/ModuleAppService.kt index 8a3bbcd52..bdcbb8339 100644 --- a/daemon/src/main/kotlin/org/matrix/vector/daemon/ipc/ModuleAppService.kt +++ b/daemon/src/main/kotlin/org/matrix/vector/daemon/ipc/ModuleAppService.kt @@ -194,9 +194,26 @@ class ModuleAppService(private val loadedModule: LoadedModule) : IXposedService. ?.distinct() ?: emptyList() } + /** + * One request, one question, one answer. + * + * The AIDL hands over a list and takes a single [IXposedScopeCallback] for it, and the javadoc on + * the client's `OnScopeEventListener` says its listener runs "when the request is completed" — + * singular — with `onScopeRequestApproved` taking the *packages* that were approved. This used to + * put one prompt per package on screen, each answered in its own right, so a module asking for + * three packages made the user answer three questions and then fired that one listener three + * times. A module that took the first answer as the answer acted on a third of it. + * + * So the whole list goes up as one prompt and Approve answers for all of it. What the user gives + * away in one press is what the prompt lists, which is why it lists all of them rather than a + * count, and why the packages are sorted and deduplicated first: it is a set that is being agreed + * to, the same set asked for twice is the same question, and `NotificationManager` identifies a + * prompt by the set it names. + */ override fun requestScope(packages: List, callback: IXposedScopeCallback) { val userId = ensureModule() - if (packages.isEmpty()) { + val requested = packages.distinct().sorted() + if (requested.isEmpty()) { // Nothing was asked for, so the request is trivially satisfied. Returning without touching // the callback would leave the module waiting forever. callback.onScopeRequestApproved(emptyList()) @@ -205,7 +222,7 @@ class ModuleAppService(private val loadedModule: LoadedModule) : IXposedService. // A module that fixed its own scope in module.prop does not get to ask for more of it at // runtime. Prompting the user here would make "fixed" mean nothing. ConfigCache.staticScopeOf(loadedModule.packageName)?.let { claimed -> - val beyond = packages.filterNot { claimed.contains(it) } + val beyond = requested.filterNot { claimed.contains(it) } if (beyond.isNotEmpty()) { callback.onScopeRequestFailed( "This module declares a static scope, so ${beyond.joinToString()} cannot be added") @@ -213,9 +230,7 @@ class ModuleAppService(private val loadedModule: LoadedModule) : IXposedService. } } if (!PreferenceStore.isScopeRequestBlocked(loadedModule.packageName)) { - packages.forEach { pkg -> - NotificationManager.requestModuleScope(loadedModule.packageName, userId, pkg, callback) - } + NotificationManager.requestModuleScope(loadedModule.packageName, userId, requested, callback) } else { callback.onScopeRequestFailed("Scope request blocked by user configuration") } diff --git a/daemon/src/main/kotlin/org/matrix/vector/daemon/system/NotificationManager.kt b/daemon/src/main/kotlin/org/matrix/vector/daemon/system/NotificationManager.kt index 602ab3883..8eaa9574d 100644 --- a/daemon/src/main/kotlin/org/matrix/vector/daemon/system/NotificationManager.kt +++ b/daemon/src/main/kotlin/org/matrix/vector/daemon/system/NotificationManager.kt @@ -41,20 +41,18 @@ private const val SCOPE_REQUEST_TIMEOUT_MS = 60L * 60 * 1000 /** * How many prompts one module may have waiting for an answer at once. * - * `IXposedService.requestScope` takes an unbounded list and there is now one prompt per package in - * it, none of them deduplicated beyond exact string equality and none checked for existence before - * it goes up. Nothing else bounds them: these are enqueued as "android", which - * NotificationManagerService exempts from its per-package limit, and they are IMPORTANCE_HIGH, so a - * module asking for a thousand packages gets a thousand heads-up prompts that each sit for - * [SCOPE_REQUEST_TIMEOUT_MS]. That was hidden before this branch only because every prompt of a - * module shared one tag and so replaced the one before it — which is the bug this branch fixed. + * Nothing else bounds them: these are enqueued as "android", which NotificationManagerService + * exempts from its per-package limit, and they are IMPORTANCE_HIGH, so a module calling + * `IXposedService.requestScope` in a loop would get a heads-up prompt per call, each sitting for + * [SCOPE_REQUEST_TIMEOUT_MS]. It is a *call* that costs a place, not a package: one request is one + * prompt however many packages it names, which is what makes a single Approve able to answer for + * all of them. * * Sixteen because it is far above anything an honest module asks for in one go, and low enough that * the worst a module can do to the shade is a screenful. It bounds what is *unanswered*, not what * may be asked over time: answering a prompt frees its place at once, so a module that asks a few - * questions and waits for them never meets it. A module that does meet it is told so per package - * rather than left waiting, because a callback that never fires is the failure this whole path - * exists to avoid. + * questions and waits for them never meets it. A module that does meet it is told so rather than + * left waiting, because a callback that never fires is the failure this whole path exists to avoid. */ private const val MAX_OPEN_SCOPE_REQUESTS_PER_MODULE = 16 @@ -161,9 +159,14 @@ object NotificationManager { * that each replaced the one before it: only the last request was ever answerable, the earlier * ones were never granted and their callbacks were never called at all. One module running under * two users collided in exactly the same way. + * + * The requested packages are named as a set rather than one at a time, because one call to + * `requestScope` is one prompt. Canonical order is the caller's job — see `ModuleAppService`, + * which sorts — so that the same request asked twice lands on the same tag and replaces its own + * prompt instead of stacking a second copy of the same question. */ - private fun scopeTag(modulePkg: String, moduleUserId: Int, scopePkg: String) = - "$modulePkg:$moduleUserId:$scopePkg" + private fun scopeTag(modulePkg: String, moduleUserId: Int, scopePkgs: List) = + "$modulePkg:$moduleUserId:${scopePkgs.joinToString(",")}" /** Cancels what we posted under [tag]; the id is derived from it exactly as it is on enqueue. */ private fun cancelByTag(tag: String) { @@ -179,13 +182,13 @@ object NotificationManager { } /** - * Takes down the prompt for one (module, user, requested package) once it has been answered. + * Takes down the prompt for one (module, user, requested set) once it has been answered. * - * It has to name the requested package, because a module asking for several has one prompt per - * package and answering one of them must not clear the rest. + * It has to name the requested set, because a module that asked twice for different sets has a + * prompt for each and answering one of them must not clear the other. */ - fun cancelScopeRequest(modulePkg: String, moduleUserId: Int, scopePkg: String) = - cancelByTag(scopeTag(modulePkg, moduleUserId, scopePkg)) + fun cancelScopeRequest(modulePkg: String, moduleUserId: Int, scopePkgs: List) = + cancelByTag(scopeTag(modulePkg, moduleUserId, scopePkgs)) /** * The "not activated yet" half of [notifyModuleUpdated], which is the half that can go stale. @@ -310,20 +313,20 @@ object NotificationManager { } /** - * Claims the right to answer the prompt for one (module, user, requested package). + * Claims the right to answer the prompt for one (module, user, requested set). * * @return true for the first caller, false for every later one — the module's * [IXposedScopeCallback] must be called exactly once per request. */ - fun claimScopeAnswer(modulePkg: String, moduleUserId: Int, scopePkg: String) = - OutstandingScopeRequests.claim(scopeTag(modulePkg, moduleUserId, scopePkg)) != null + fun claimScopeAnswer(modulePkg: String, moduleUserId: Int, scopePkgs: List) = + OutstandingScopeRequests.claim(scopeTag(modulePkg, moduleUserId, scopePkgs)) != null /** * Withdraws every prompt [modulePkg] still has on screen and hands back their callbacks, so the * caller can tell each of those requests it will not be granted. * - * What makes "never ask again" mean what it says. A module that asked for three packages now has - * a prompt for each of them; answering the user's "stop asking" by leaving two more questions on + * What makes "never ask again" mean what it says. A module that asked three times has a prompt + * for each of those requests; answering the user's "stop asking" by leaving two more questions on * screen — both still approvable — would be answering it with the opposite. * * They are claimed before they are cancelled, and that order is load-bearing, though not for the @@ -338,16 +341,14 @@ object NotificationManager { * moment, would otherwise answer a request we are in the middle of withdrawing. Claiming first * makes every one of those arrive at a closed door. * - * Each withdrawn request is handed back on its own, so the caller reports one failure per - * package. That is the honest reading — the module named each package separately and each was - * asked in its own right — but it is worth knowing what it costs: `requestScope` supplies one - * callback for the whole list, so those failures all land on the same binder, and the shipped - * client library does not collapse them. Its `OnScopeEventListener` wrapper calls the listener - * every time and merely drops its map entry afterwards, so a module written to the singular - * javadoc ("invoked when the request is completed") runs its handler once per package rather than - * once per call. Reporting the withdrawal once would be the other defensible choice, but it would - * have to pick one of the packages to name and would leave the rest with no answer at all, which - * is exactly the failure this map exists to prevent. + * Each withdrawn request is handed back on its own, and that is now one failure per + * `requestScope` call rather than one per package. It used to be per package, which put a module + * in an awkward spot: `requestScope` supplies one callback for the whole list, those failures all + * landed on the same binder, and the shipped client library does not collapse them — its + * `OnScopeEventListener` wrapper calls the listener every time and merely drops its map entry + * afterwards, so a module written to the singular javadoc ("invoked when the request is + * completed") ran its handler once per package. Batching the prompt is what fixed that, here and + * everywhere else on this path: the answer is now shaped like the question the module asked. */ fun withdrawScopeRequests(modulePkg: String): List { val withdrawn = OutstandingScopeRequests.claimAllOf(modulePkg) @@ -373,10 +374,10 @@ object NotificationManager { fun requestModuleScope( modulePkg: String, moduleUserId: Int, - scopePkg: String, + scopePkgs: List, callback: IXposedScopeCallback ) { - val tag = scopeTag(modulePkg, moduleUserId, scopePkg) + val tag = scopeTag(modulePkg, moduleUserId, scopePkgs) // Registered before the notification is built, let alone posted: the buttons are live from the // moment the platform accepts it, and a prompt the receiver does not know about is one whose // answer it drops. Registering is also what enforces the ceiling, so there is no point @@ -386,14 +387,13 @@ object NotificationManager { Log.w( TAG, "$modulePkg is already waiting on $MAX_OPEN_SCOPE_REQUESTS_PER_MODULE scope prompts;" + - " not asking about $scopePkg") - // Refused, not ignored. The module is told about this package rather than left holding a - // callback that can never fire, and the message names the package so a module developer can - // see which of their list did not make it. + " not asking about ${scopePkgs.joinToString()}") + // Refused, not ignored. The module is told rather than left holding a callback that can + // never fire, and the message names what did not make it so a module developer can see it. runCatching { callback.onScopeRequestFailed( "Too many scope requests are already waiting for an answer from the user," + - " so $scopePkg was not asked about") + " so ${scopePkgs.joinToString()} was not asked about") } .onFailure { Log.w(TAG, "Could not tell $modulePkg its request was refused", it) } return @@ -415,7 +415,12 @@ object NotificationManager { Uri.Builder() .scheme("module") .encodedAuthority("$modulePkg:$moduleUserId") - .encodedPath(scopePkg) + // The whole list, because one request is one prompt and one answer. ',' is a + // legal path character and cannot occur in a package name, so the receiver can + // split it back apart; it is also what keeps two requests naming different sets + // on separate PendingIntents, which are identified by their intent and not by + // the extras that carry the callback. + .encodedPath(scopePkgs.joinToString(",")) .appendQueryParameter("action", actionParams) .build() putExtras(Bundle().apply { putBinder("callback", callback.asBinder()) }) @@ -432,7 +437,10 @@ object NotificationManager { .setContentTitle(context.getString(R.string.xposed_module_request_scope_title)) .setContentText( context.getString( - R.string.xposed_module_request_scope_content, modulePkg, userName, scopePkg)) + R.string.xposed_module_request_scope_content, + modulePkg, + userName, + scopePkgs.joinToString())) .setSmallIcon(getNotificationIcon()) .addAction( Notification.Action.Builder( @@ -466,7 +474,14 @@ object NotificationManager { R.string.xposed_module_request_scope_content, modulePkg, userName, - scopePkg))) + // The whole list, wrapped over as many lines as it takes. Approve + // answers for all of it at once, so all of it is what the user is + // agreeing to; the collapsed line above is one line whatever we put in + // it, and this is where a request naming more packages than fit there + // becomes readable. Comma-separated rather than one per line because + // the string this fills is a sentence and the list sits mid-way + // through it. + scopePkgs.joinToString()))) .build() .apply { extras.putString("android.substName", BuildConfig.FRAMEWORK_NAME) }