From de3818e3d4578c16aa17dea2b86d3624353e0825 Mon Sep 17 00:00:00 2001 From: Thomas Date: Fri, 12 Jun 2026 17:27:50 +0200 Subject: [PATCH] Reject unrecognized device ABIs and broaden APK match error handling GetAppDownloadInfo and GetAppUpdateInfo pass a fully client-controlled device spec to bundletool's ApkMatcher. protovalidate only checks that supported_abis is non-empty, not that its entries name real ABIs, so an unrecognized ABI reaches the matcher. bundletool then reports it inconsistently: an IncompatibleDeviceException for split APKs (or silently ignores it when an ABI the app ships a split for precedes it in the list) and an InvalidCommandException for multi-ABI ones. Mapping by exception type would give the same malformed spec different status codes depending on the app. Validate supported_abis against bundletool's known ABI names before matching and reject unrecognized ones with INVALID_ARGUMENT, so a malformed spec is treated as a bad request rather than an incompatible device. Separately, some matchers reject the spec without raising a BundleToolException: DeviceTierApkMatcher and CountrySetApkMatcher call Guava's checkArgument when the spec's device_tier / country_set doesn't match the bundle's declared values, and OpenGlFeatureMatcher throws a NumberFormatException decoding the spec's reqGlEsVersion device feature. The tier and country set values are bundle-relative, so protovalidate can't cover them. Widen getMatchingApkPaths's catch from IncompatibleDeviceException to BundleToolException and IllegalArgumentException so such specs yield an empty match (FAILED_PRECONDITION) instead of escaping uncaught. --- .../server/directory/ApkMatchingUtils.kt | 22 +++++- .../server/directory/AppServiceImpl.kt | 17 +++++ .../server/directory/AppServiceImplTest.kt | 69 +++++++++++++++++++ 3 files changed, 105 insertions(+), 3 deletions(-) diff --git a/directory/src/main/kotlin/app/accrescent/server/directory/ApkMatchingUtils.kt b/directory/src/main/kotlin/app/accrescent/server/directory/ApkMatchingUtils.kt index 650905d5..45e9c6e0 100644 --- a/directory/src/main/kotlin/app/accrescent/server/directory/ApkMatchingUtils.kt +++ b/directory/src/main/kotlin/app/accrescent/server/directory/ApkMatchingUtils.kt @@ -8,16 +8,28 @@ import app.accrescent.appstore.v1.DeviceAttributes import app.accrescent.bundletool.android.bundle.Commands import app.accrescent.bundletool.android.bundle.Devices import com.android.tools.build.bundletool.device.ApkMatcher -import com.android.tools.build.bundletool.model.exceptions.IncompatibleDeviceException +import com.android.tools.build.bundletool.model.AbiName +import com.android.tools.build.bundletool.model.exceptions.BundleToolException import java.util.Optional +/** + * Returns the first ABI in [deviceAttributes]'s device spec that bundletool does not recognize, or + * `null` if every supported ABI is known. + * + * Lets callers reject unrecognized ABIs before matching, since bundletool otherwise surfaces them + * inconsistently (an `IncompatibleDeviceException` for split APKs, an `InvalidCommandException` for + * multi-ABI ones, or no error at all). + */ +fun firstUnrecognizedAbi(deviceAttributes: DeviceAttributes): String? = + deviceAttributes.spec.supportedAbisList.firstOrNull { !AbiName.fromPlatformName(it).isPresent } + /** * Gets an app's matching APK paths for the given device * * @param appMetadata the `BuildApksResult` of the app * @param deviceAttributes the device attributes of the device * @return a list of APK paths in the APK set associated with [appMetadata] and matching the given - * device, or an empty list if none match + * device, or an empty list if none match or bundletool rejects [deviceAttributes] */ fun getMatchingApkPaths( appMetadata: Commands.BuildApksResult, @@ -31,7 +43,11 @@ fun getMatchingApkPaths( false, true, ).getMatchingApks(appMetadata) - } catch (_: IncompatibleDeviceException) { + } catch (_: BundleToolException) { + emptyList() + } catch (_: IllegalArgumentException) { + // Some matchers (device_tier, country_set, reqGlEsVersion) reject the spec with a plain + // IllegalArgumentException rather than a BundleToolException. emptyList() }.map { it.path.toString() } diff --git a/directory/src/main/kotlin/app/accrescent/server/directory/AppServiceImpl.kt b/directory/src/main/kotlin/app/accrescent/server/directory/AppServiceImpl.kt index 5c9b4062..695eec2d 100644 --- a/directory/src/main/kotlin/app/accrescent/server/directory/AppServiceImpl.kt +++ b/directory/src/main/kotlin/app/accrescent/server/directory/AppServiceImpl.kt @@ -5,6 +5,7 @@ package app.accrescent.server.directory import app.accrescent.appstore.v1.AppService +import app.accrescent.appstore.v1.DeviceAttributes import app.accrescent.appstore.v1.GetAppDownloadInfoRequest import app.accrescent.appstore.v1.GetAppDownloadInfoResponse import app.accrescent.appstore.v1.GetAppListingRequest @@ -223,6 +224,7 @@ class AppServiceImpl @Inject constructor( request: GetAppDownloadInfoRequest, ): Uni { validateRequestOrThrow(request) + validateDeviceAbisOrThrow(request.deviceAttributes) val response = ReleaseChannel.findByAppIdAndName( request.appId, @@ -275,6 +277,7 @@ class AppServiceImpl @Inject constructor( @WithSession override fun getAppUpdateInfo(request: GetAppUpdateInfoRequest): Uni { validateRequestOrThrow(request) + validateDeviceAbisOrThrow(request.deviceAttributes) val response = ReleaseChannel.findByAppIdAndName( request.appId, @@ -328,6 +331,20 @@ class AppServiceImpl @Inject constructor( return response } + /** + * Rejects a request whose device spec contains an ABI bundletool does not recognize. + * + * @throws StatusRuntimeException if the device spec contains an unrecognized ABI + */ + private fun validateDeviceAbisOrThrow(deviceAttributes: DeviceAttributes) { + firstUnrecognizedAbi(deviceAttributes)?.let { abi -> + throw Status + .fromCode(Status.Code.INVALID_ARGUMENT) + .withDescription("unrecognized ABI in device spec: $abi") + .asRuntimeException() + } + } + /** * Verifies the request is valid or else throws an exception. * diff --git a/directory/src/test/kotlin/app/accrescent/server/directory/AppServiceImplTest.kt b/directory/src/test/kotlin/app/accrescent/server/directory/AppServiceImplTest.kt index 187a8cd6..5e56db62 100644 --- a/directory/src/test/kotlin/app/accrescent/server/directory/AppServiceImplTest.kt +++ b/directory/src/test/kotlin/app/accrescent/server/directory/AppServiceImplTest.kt @@ -271,6 +271,60 @@ class AppServiceImplImplTest { ) } + @Test + fun getAppDownloadInfoWithUnrecognizedDeviceAbiReturnsInvalidArgument() { + KafkaHelper.publishApps(kafka, TestDataHelper.validAppPublicationRequested) + + val request = getAppDownloadInfoRequest { + appId = "app.accrescent.client" + deviceAttributes = deviceAttributesWithUnrecognizedAbi + } + + val status = CompletableFuture() + appService.getAppDownloadInfo(request) + .subscribe() + .with( + { status.complete(Status.Code.OK) }, + { + require(it is StatusRuntimeException) + status.complete(it.status.code) + }, + ) + + assertEquals( + Status.Code.INVALID_ARGUMENT, + status.get(REQUEST_TIMEOUT_SECS, TimeUnit.SECONDS), + ) + } + + @Test + fun getAppDownloadInfoWithUnparseableOpenGlVersionReturnsFailedPrecondition() { + KafkaHelper.publishApps(kafka, TestDataHelper.validAppPublicationRequested) + + // A non-numeric reqGlEsVersion makes the matcher throw a NumberFormatException + // rather than a BundleToolException, exercising the wider backstop catch. + val request = getAppDownloadInfoRequest { + appId = "app.accrescent.client" + deviceAttributes = deviceAttributesWithUnparseableOpenGlVersion + } + + val status = CompletableFuture() + appService.getAppDownloadInfo(request) + .subscribe() + .with( + { status.complete(Status.Code.OK) }, + { + require(it is StatusRuntimeException) + status.complete(it.status.code) + }, + ) + + assertEquals( + Status.Code.FAILED_PRECONDITION, + status.get(REQUEST_TIMEOUT_SECS, TimeUnit.SECONDS), + ) + } + private fun getExpectedAppDownloadInfoResponse() = getAppDownloadInfoResponse { appDownloadInfo = appDownloadInfo { splitDownloadInfo.addAll( @@ -307,6 +361,21 @@ class AppServiceImplImplTest { } .build() + private val deviceAttributesWithUnrecognizedAbi: DeviceAttributes = validDeviceAttributes + .toBuilder() + .setSpec(validDeviceAttributes.spec.toBuilder().addSupportedAbis("definitely-not-an-abi")) + .build() + + private val deviceAttributesWithUnparseableOpenGlVersion: DeviceAttributes = + validDeviceAttributes + .toBuilder() + .setSpec( + validDeviceAttributes.spec.toBuilder() + .clearDeviceFeatures() + .addDeviceFeatures("reqGlEsVersion=banana") + ) + .build() + private val validGetAppListingRequest = getAppListingRequest { appId = "app.accrescent.client" preferredLanguages.add("en-US")