From 7a88c4bae6f0d2b0215e87c8a22155250a152e0d Mon Sep 17 00:00:00 2001 From: aj-rosado <109146700+aj-rosado@users.noreply.github.com> Date: Thu, 9 Jul 2026 18:12:02 +0100 Subject: [PATCH] [PM-39612] fix: resolve autofill container-hint redirect to autofillable child (#7146) --- .../autofill/parser/AutofillParserImpl.kt | 29 ++++- .../data/autofill/util/ViewNodeExtensions.kt | 40 ++++++- .../autofill/parser/AutofillParserTests.kt | 67 ++++++++++++ .../autofill/util/ViewNodeExtensionsTest.kt | 100 ++++++++++++++++++ 4 files changed, 229 insertions(+), 7 deletions(-) diff --git a/app/src/main/kotlin/com/x8bit/bitwarden/data/autofill/parser/AutofillParserImpl.kt b/app/src/main/kotlin/com/x8bit/bitwarden/data/autofill/parser/AutofillParserImpl.kt index 61f54b71e6..7e15ea5323 100644 --- a/app/src/main/kotlin/com/x8bit/bitwarden/data/autofill/parser/AutofillParserImpl.kt +++ b/app/src/main/kotlin/com/x8bit/bitwarden/data/autofill/parser/AutofillParserImpl.kt @@ -280,6 +280,9 @@ private fun AssistStructure.ViewNode.traverse( // Set up mutable lists for collecting valid AutofillViews and ignorable view ids. val mutableAutofillViewList: MutableList = mutableListOf() val mutableIgnoreAutofillIdList: MutableList = mutableListOf() + // Tracks autofill IDs already claimed by a non-Unused view so that child nodes whose IDs + // were redirected to by a container ancestor are not added a second time. + val claimedAutofillIds: MutableSet = mutableSetOf() // OS sometimes defaults node.idPackage to "android", which is not a valid // package name so it is ignored to prevent auto-filling unknown applications. var storedIdPackage: String? = this.idPackage?.takeUnless { it.isBlank() || it == "android" } @@ -293,7 +296,12 @@ private fun AssistStructure.ViewNode.traverse( // Try converting this `ViewNode` into an `AutofillView`. If a valid instance is returned, add // it to the list. Otherwise, ignore the `AutofillId` associated with this `ViewNode`. toAutofillView(parentWebsite = parentWebsite) - ?.run(mutableAutofillViewList::add) + ?.also { view -> + if (view !is AutofillView.Unused) { + claimedAutofillIds.add(view.data.autofillId) + } + mutableAutofillViewList.add(view) + } ?: autofillId?.run(mutableIgnoreAutofillIdList::add) // Recursively traverse all of this view node's children. @@ -302,8 +310,23 @@ private fun AssistStructure.ViewNode.traverse( getChildAt(i) .traverse(parentWebsite = website) .let { viewNodeTraversalData -> - viewNodeTraversalData.autofillViews.forEach(mutableAutofillViewList::add) - viewNodeTraversalData.ignoreAutofillIds.forEach(mutableIgnoreAutofillIdList::add) + viewNodeTraversalData.autofillViews + // filter out existing AutofillIds to avoid duplicates + .filter { view -> + val id = view.data.autofillId + if (id in claimedAutofillIds) { + false + } else if (view !is AutofillView.Unused) { + claimedAutofillIds.add(id) + true + } else { + true + } + } + .forEach(mutableAutofillViewList::add) + viewNodeTraversalData.ignoreAutofillIds + .filter { it !in claimedAutofillIds } + .forEach(mutableIgnoreAutofillIdList::add) // Get the first non-null idPackage. if (storedIdPackage == null) { diff --git a/app/src/main/kotlin/com/x8bit/bitwarden/data/autofill/util/ViewNodeExtensions.kt b/app/src/main/kotlin/com/x8bit/bitwarden/data/autofill/util/ViewNodeExtensions.kt index 5d80e86fc8..112a1607b3 100644 --- a/app/src/main/kotlin/com/x8bit/bitwarden/data/autofill/util/ViewNodeExtensions.kt +++ b/app/src/main/kotlin/com/x8bit/bitwarden/data/autofill/util/ViewNodeExtensions.kt @@ -2,6 +2,7 @@ package com.x8bit.bitwarden.data.autofill.util import android.app.assist.AssistStructure import android.view.View +import android.view.autofill.AutofillId import android.widget.EditText import androidx.annotation.VisibleForTesting import com.bitwarden.ui.platform.base.util.orNullIfBlank @@ -53,15 +54,32 @@ fun AssistStructure.ViewNode.toAutofillView( parentWebsite: String?, ): AutofillView? { val nonNullAutofillId = this.autofillId ?: return null - if (this.supportedAutofillHint == null && !this.isInputField) return null + val hint = this.supportedAutofillHint + val isInput = this.isInputField + if (hint == null && !isInput) return null + + // When a container (autofillType=NONE) is classified with a semantic hint, the container + // itself cannot receive an autofill value. Redirect to the first autofillable child so the + // fill reaches the inner EditText instead of silently failing on the wrapper. + // The child's autofillType must also be used — the container's type=0 (NONE) would cause + // buildFilledItemOrNull to return null and drop the field from the fill dataset entirely. + val autofillableChild = + if (hint != null && autofillType == View.AUTOFILL_TYPE_NONE) { + findFirstAutofillableChild() + } else { + null + } + val effectiveAutofillId = autofillableChild?.autofillId ?: nonNullAutofillId + val effectiveAutofillType = autofillableChild?.autofillType ?: autofillType + val autofillOptions = this .autofillOptions .orEmpty() .map { it.toString() } val autofillViewData = AutofillView.Data( - autofillId = nonNullAutofillId, + autofillId = effectiveAutofillId, autofillOptions = autofillOptions, - autofillType = this.autofillType, + autofillType = effectiveAutofillType, isFocused = this.isFocused, textValue = this.autofillValue?.extractTextValue(), hasPasswordTerms = this.hasPasswordTerms(), @@ -70,10 +88,24 @@ fun AssistStructure.ViewNode.toAutofillView( return buildAutofillView( autofillOptions = autofillOptions, autofillViewData = autofillViewData, - autofillHint = this.supportedAutofillHint, + autofillHint = hint, ) } +private data class AutofillableChild(val autofillId: AutofillId, val autofillType: Int) + +private fun AssistStructure.ViewNode.findFirstAutofillableChild(): AutofillableChild? { + for (i in 0 until childCount) { + val child = getChildAt(i) + if (child.autofillType == View.AUTOFILL_TYPE_TEXT) { + val id = child.autofillId ?: continue + return AutofillableChild(autofillId = id, autofillType = child.autofillType) + } + child.findFirstAutofillableChild()?.let { return it } + } + return null +} + /** * The first supported autofill hint for this view node, or null if none are found. */ diff --git a/app/src/test/kotlin/com/x8bit/bitwarden/data/autofill/parser/AutofillParserTests.kt b/app/src/test/kotlin/com/x8bit/bitwarden/data/autofill/parser/AutofillParserTests.kt index e23a4faa0b..f3ee111b1f 100644 --- a/app/src/test/kotlin/com/x8bit/bitwarden/data/autofill/parser/AutofillParserTests.kt +++ b/app/src/test/kotlin/com/x8bit/bitwarden/data/autofill/parser/AutofillParserTests.kt @@ -947,6 +947,73 @@ class AutofillParserTests { } } + @Suppress("MaxLineLength") + @Test + fun `parse should deduplicate when a child node returns an AutofillView with the same autofillId already claimed by a non-Unused parent view`() { + // Setup + val sharedAutofillId: AutofillId = mockk() + val childViewNode: AssistStructure.ViewNode = mockk { + every { this@mockk.childCount } returns 0 + every { this@mockk.idPackage } returns null + every { this@mockk.website } returns null + } + val parentViewNode: AssistStructure.ViewNode = mockk { + every { this@mockk.childCount } returns 1 + every { this@mockk.getChildAt(0) } returns childViewNode + every { this@mockk.idPackage } returns ID_PACKAGE + every { this@mockk.website } returns WEBSITE + } + val windowNode: AssistStructure.WindowNode = mockk { + every { this@mockk.rootViewNode } returns parentViewNode + } + every { assistStructure.windowNodeCount } returns 1 + every { assistStructure.getWindowNodeAt(0) } returns windowNode + + val sharedAutofillView = AutofillView.Login.Password( + data = AutofillView.Data( + autofillId = sharedAutofillId, + autofillOptions = emptyList(), + autofillType = AUTOFILL_TYPE, + isFocused = true, + textValue = null, + hasPasswordTerms = false, + website = URI, + ), + ) + every { parentViewNode.toAutofillView(parentWebsite = any()) } returns sharedAutofillView + every { childViewNode.toAutofillView(parentWebsite = any()) } returns sharedAutofillView + + val expected = AutofillRequest.Fillable( + ignoreAutofillIds = emptyList(), + inlinePresentationSpecs = inlinePresentationSpecs, + maxInlineSuggestionsCount = MAX_INLINE_SUGGESTION_COUNT, + packageName = PACKAGE_NAME, + partition = AutofillPartition.Login(views = listOf(sharedAutofillView)), + uri = URI, + ) + + // Test + val actual = parser.parse( + autofillAppInfo = autofillAppInfo, + fillRequest = fillRequest, + ) + + // Verify + assertEquals(expected, actual) + verify(exactly = 1) { + fillRequest.getInlinePresentationSpecs( + autofillAppInfo = autofillAppInfo, + isInlineAutofillEnabled = true, + ) + fillRequest.getMaxInlineSuggestionsCount( + autofillAppInfo = autofillAppInfo, + isInlineAutofillEnabled = true, + ) + any>().buildPackageNameOrNull(assistStructure) + any().buildUriOrNull(PACKAGE_NAME) + } + } + /** * Setup [assistStructure] to return window nodes with each [AutofillView] type (card and login) * so we can test how different window node configurations produce different partitions. diff --git a/app/src/test/kotlin/com/x8bit/bitwarden/data/autofill/util/ViewNodeExtensionsTest.kt b/app/src/test/kotlin/com/x8bit/bitwarden/data/autofill/util/ViewNodeExtensionsTest.kt index 2fec4139eb..b4b325d626 100644 --- a/app/src/test/kotlin/com/x8bit/bitwarden/data/autofill/util/ViewNodeExtensionsTest.kt +++ b/app/src/test/kotlin/com/x8bit/bitwarden/data/autofill/util/ViewNodeExtensionsTest.kt @@ -34,10 +34,12 @@ class ViewNodeExtensionsTest { private val testAutofillValue: AutofillValue = mockk() private val mockHtmlInfo: HtmlInfo = mockk { every { attributes } returns emptyList() + every { tag } returns "" } private val viewNode: AssistStructure.ViewNode = mockk { every { autofillId } returns expectedAutofillId + every { className } returns null every { idEntry } returns null every { hint } returns null every { autofillOptions } returns AUTOFILL_OPTIONS_ARRAY @@ -1236,6 +1238,104 @@ class ViewNodeExtensionsTest { assertNull(actual) } + @Suppress("MaxLineLength") + @Test + fun `toAutofillView should redirect autofillId and autofillType to first autofillable child when container has AUTOFILL_TYPE_NONE and a supported hint`() { + // Setup + val childAutofillId: AutofillId = mockk() + val childAutofillType = View.AUTOFILL_TYPE_TEXT + val childNode: AssistStructure.ViewNode = mockk { + every { autofillId } returns childAutofillId + every { autofillType } returns childAutofillType + every { childCount } returns 0 + every { className } returns null + every { htmlInfo } returns mockHtmlInfo + } + every { viewNode.autofillHints } returns arrayOf(View.AUTOFILL_HINT_PASSWORD) + every { viewNode.autofillType } returns View.AUTOFILL_TYPE_NONE + every { viewNode.childCount } returns 1 + every { viewNode.getChildAt(0) } returns childNode + val expected = AutofillView.Login.Password( + data = autofillViewData.copy( + autofillId = childAutofillId, + autofillType = childAutofillType, + ), + ) + + // Test + val actual = viewNode.toAutofillView(parentWebsite = null) + + // Verify + assertEquals(expected, actual) + } + + @Suppress("MaxLineLength") + @Test + fun `toAutofillView should redirect to nested grandchild when direct child has AUTOFILL_TYPE_NONE`() { + // Setup + val grandchildAutofillId: AutofillId = mockk() + val grandchildAutofillType = View.AUTOFILL_TYPE_TEXT + val grandchildNode: AssistStructure.ViewNode = mockk { + every { autofillId } returns grandchildAutofillId + every { autofillType } returns grandchildAutofillType + every { childCount } returns 0 + every { className } returns null + every { htmlInfo } returns mockHtmlInfo + } + val intermediateChildNode: AssistStructure.ViewNode = mockk { + every { autofillId } returns mockk() + every { autofillType } returns View.AUTOFILL_TYPE_NONE + every { childCount } returns 1 + every { getChildAt(0) } returns grandchildNode + every { className } returns null + every { htmlInfo } returns mockHtmlInfo + } + every { viewNode.autofillHints } returns arrayOf(View.AUTOFILL_HINT_PASSWORD) + every { viewNode.autofillType } returns View.AUTOFILL_TYPE_NONE + every { viewNode.childCount } returns 1 + every { viewNode.getChildAt(0) } returns intermediateChildNode + val expected = AutofillView.Login.Password( + data = autofillViewData.copy( + autofillId = grandchildAutofillId, + autofillType = grandchildAutofillType, + ), + ) + + // Test + val actual = viewNode.toAutofillView(parentWebsite = null) + + // Verify + assertEquals(expected, actual) + } + + @Suppress("MaxLineLength") + @Test + fun `toAutofillView should fall back to container own autofillId when AUTOFILL_TYPE_NONE and hint present but no autofillable child exists`() { + // Setup + val nonAutofillableChildNode: AssistStructure.ViewNode = mockk { + every { autofillId } returns mockk() + every { autofillType } returns View.AUTOFILL_TYPE_NONE + every { childCount } returns 0 + every { className } returns null + every { htmlInfo } returns mockHtmlInfo + } + every { viewNode.autofillHints } returns arrayOf(View.AUTOFILL_HINT_PASSWORD) + every { viewNode.autofillType } returns View.AUTOFILL_TYPE_NONE + every { viewNode.childCount } returns 1 + every { viewNode.getChildAt(0) } returns nonAutofillableChildNode + val expected = AutofillView.Login.Password( + data = autofillViewData.copy( + autofillType = View.AUTOFILL_TYPE_NONE, + ), + ) + + // Test + val actual = viewNode.toAutofillView(parentWebsite = null) + + // Verify + assertEquals(expected, actual) + } + /** * Set up [viewNode] to be an input field but not supported. */