Update Autofill to detect url bar webDomain for certain browsers #6141
Conversation
|
Great job! No new security vulnerabilities introduced in this pull request |
david-livefront
force-pushed
the
autofill-url-bar-detection
branch
3 times, most recently
from
a4902eb to
904992d
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6141 +/- ##
==========================================
+ Coverage 84.48% 84.99% +0.51%
==========================================
Files 839 723 -116
Lines 55065 52854 -2211
Branches 7697 7676 -21
==========================================
- Hits 46520 44925 -1595
+ Misses 5857 5246 -611
+ Partials 2688 2683 -5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
david-livefront
force-pushed
the
autofill-url-bar-detection
branch
4 times, most recently
from
f86d650 to
e2892af
Compare
david-livefront
force-pushed
the
autofill-url-bar-detection
branch
from
e2892af to
d5e4125
Compare
david-livefront
requested review from
a team and
brian-livefront
as code owners
|
Claude finished @david-livefront's task —— View job Code Review CompleteOverall Assessment: REQUEST CHANGES Critical Issues:
See inline comments for details. SummaryThis PR introduces URL bar detection for autofill in specific browsers (Microsoft Edge, Samsung Internet, Opera) by parsing the Architecture & Patterns: ✓ Follows MVVM architecture, proper DI with Hilt, and repository pattern FindingsFinding 1: ❌ Missing KDoc documentation on multiple public APIs violates project standards The following public properties and functions lack required KDoc documentation per
Finding 2: DetailsThe implementation parses Risk: A malicious app could potentially spoof URL bar data by creating ViewNodes with matching Mitigation Present: The warning dialog in the UI correctly communicates this is "less secure" and warns about malicious sites, which is good. The feature is also opt-in (disabled by default). Recommendation: Consider documenting the threat model implications in code comments within Example: // SECURITY NOTE: URL bar detection uses ViewNode.webDomain from browser URL fields
// to determine autofill context. This is less secure than standard autofill as:
// 1. It relies on idEntry matching (spoofable by malicious apps)
// 2. webDomain content is not cryptographically verified
// This feature is opt-in and warns users of these risks.
val urlBarWebsite = traversalDataList
.flatMap { it.urlBarWebsites }
.firstOrNull()
?.takeIf { settingsRepository.isAutofillWebDomainCompatMode }Finding 3: ❌ Insufficient test coverage (52.94% patch coverage) DetailsCodecov reports 16 lines missing coverage. Key gaps:
Required Actions:
Finding 4: 💭 Consider extracting URL_BARS map to configuration DetailsThe Consideration: As more browsers are added, this could become maintenance-heavy. Consider:
This is not blocking but would improve maintainability. Finding 5: 🎨 Improve string resource formatting consistency DetailsIn ui/src/main/res/values/strings.xml:1156-1158: The warning message is quite long (49 words). Consider breaking into shorter, more digestible sentences for better i18n and readability: <string name="compatibility_mode_warning">Compatibility mode should only be enabled if autofill doesn't work in your browser. This setting reduces security and could allow malicious sites to capture your passwords. Only enable it if you accept this risk.</string>
<string name="compatibility_mode_warning">Compatibility mode should only be enabled if autofill doesn\'t work in your browser.\n\nThis setting reduces security and could allow malicious sites to capture your passwords.\n\nOnly enable it if you accept this risk.</string>Adding line breaks improves scannability in the dialog. Finding 6: 💭 Question about browser selection criteria DetailsThe PR includes Edge, Samsung Internet, and Opera browsers. From the Jira tickets context:
This isn't blocking, but clarification would help understand the long-term strategy. Positive Observations👍 Excellent architecture compliance - proper separation of concerns across data/UI layers Required Changes
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| /** | ||
| * Whether or not the autofill web domain parsing is enabled. | ||
| */ |
There was a problem hiding this comment.
❌ Missing required KDoc documentation
Details
Per docs/STYLE_AND_BEST_PRACTICES.md line 273: "All public classes, functions, and properties should include documentation in the KDoc style."
Required:
/**
* Whether or not autofill web domain compatibility mode is enabled. When enabled,
* autofill will parse website information from browser URL bar fields for supported
* browsers. This is less secure than standard autofill.
*
* Defaults to `false` (disabled).
*/
var isAutofillWebDomainCompatMode: BooleanThis property is part of the public SettingsRepository interface and requires documentation explaining:
- What the setting controls
- Security implications
- Default value
| * | ||
| * @param autofillViews The list of views we care about for autofilling. | ||
| * @param idPackage The package id for this view, if there is one. | ||
| * @param urlBarWebsites The website associated with the URL bar view. |
There was a problem hiding this comment.
❌ Missing KDoc for property parameter
Details
Per docs/STYLE_AND_BEST_PRACTICES.md line 297-307, data classes with more than 2 constructor properties must document each individually using @property.
Required:
/**
* A convenience data structure for view node traversal.
*
* @property autofillViews The list of views we care about for autofilling.
* @property idPackage The package id for this view, if there is one.
* @property urlBarWebsites The list of website URLs detected from browser URL bar fields,
* extracted when compatibility mode is enabled for supported browsers.
* @property ignoreAutofillIds The list of [AutofillId]s that should be ignored in the fill response.
*/
data class ViewNodeTraversalData(
val autofillViews: List<AutofillView>,
val idPackage: String?,
val urlBarWebsites: List<String>,
val ignoreAutofillIds: List<AutofillId>,
)The urlBarWebsites property was added but not documented in the class-level KDoc.
| urlBarWebsites = storedUrlBarWebsites, | ||
| ignoreAutofillIds = mutableIgnoreAutofillIdList, | ||
| ) | ||
| } |
There was a problem hiding this comment.
❌ Missing KDoc documentation for private function
Details
While this is a private function, it performs an important operation and should be documented per best practices (line 273 of STYLE_AND_BEST_PRACTICES.md allows optional documentation for private members, but this function's complexity warrants it).
Suggested:
/**
* Updates the underlying [AutofillView.data] with the given [website] if the view does not already
* have a website associated with it. This is used to populate website information from browser
* URL bar fields when compatibility mode is enabled.
*
* @param website The website URL to set, or null to leave unchanged.
* @return A new [AutofillView] instance with the updated website, or the original view if
* [website] is null or a website already exists.
*/
private fun AutofillView.updateWebsiteIfNecessary(website: String?): AutofillView {This improves code comprehension and explains the behavior of returning early when website already exists or when the parameter is null.
| Timber.d("Parsing AssistStructure -- ${fillRequest?.id}") | ||
| // Parse the `assistStructure` into internal models. | ||
| val traversalDataList = assistStructure.traverse() | ||
| val urlBarWebsite = traversalDataList |
There was a problem hiding this comment.
Details
This code extracts website information from URL bar fields, which has security implications. Consider adding a comment documenting the threat model:
// SECURITY NOTE: URL bar detection uses ViewNode.webDomain from browser URL fields
// to determine autofill context. This approach has known limitations:
// 1. Relies on idEntry matching which could be spoofed by malicious apps
// 2. webDomain content is not cryptographically verified by the OS
// 3. Only enabled when user explicitly opts-in via isAutofillWebDomainCompatMode
// See PM-27708 for security analysis and user warnings in the settings UI.
val urlBarWebsite = traversalDataList
.flatMap { it.urlBarWebsites }
.firstOrNull()
?.takeIf { settingsRepository.isAutofillWebDomainCompatMode }This helps future maintainers understand why this feature is opt-in and what the security trade-offs are.
| /** | ||
| * A map of package ids and the known associated id entry for their url bar. | ||
| */ | ||
| private val URL_BARS: Map<String, String> = mapOf( |
There was a problem hiding this comment.
💭 Consider adding documentation for URL_BARS map maintenance
Details
Suggested improvement:
/**
* A map of package IDs to their known URL bar field identifiers.
*
* To add a new browser:
* 1. Enable Android's "Show view attribute inspection" in Developer Options
* 2. Use Android Studio's Layout Inspector on the browser's URL bar
* 3. Locate the ViewNode's idEntry value for the URL/address field
* 4. Add mapping: "package.name" to "id_entry_value"
*
* Supported browsers:
* - Microsoft Edge (all variants)
* - Samsung Internet (stable and beta)
* - Opera (stable and beta)
*/
private val URL_BARS: Map<String, String> = mapOf(This makes it easier for contributors to add new browsers and understand how these IDs were determined.
| any<List<ViewNodeTraversalData>>().buildPackageNameOrNull(assistStructure) | ||
| any<AutofillView>().buildUriOrNull(packageName) | ||
| } | ||
| } |
There was a problem hiding this comment.
❌ Missing test coverage for all AutofillView subtypes
Details
The existing tests only cover AutofillView.Login.Username. The updateWebsiteIfNecessary() function has branches for all AutofillView types (lines 320-340 in AutofillParserImpl.kt), but these aren't tested:
Missing test cases:
- Card types: Brand, CardholderName, ExpirationDate, ExpirationMonth, ExpirationYear, Number, SecurityCode
- Login.Password
- Unused type
Suggested test structure:
@Test
fun `parse updates Card autofill views with URL bar website when compat mode enabled`() {
// Test with Card.Number view
// Verify website is populated from URL bar
}
@Test
fun `parse does not override existing website in AutofillView when URL bar present`() {
// Setup view with existing website
// Verify URL bar website doesn't override it
}
@Test
fun `parse handles multiple URL bar nodes by using first match`() {
// Setup traversal with multiple urlBarWebsites entries
// Verify first website is used (per line 95: .firstOrNull())
}This will bring patch coverage above 80% and ensure all code paths are verified.
|
Thanks @SaintPatrck |
🎟️ Tracking
PM-27708
PM-27941
📔 Objective
This PR adds support for specific browsers to provide the website information by parsing it from the url bar field in the ViewNode graph.
📸 Screenshots
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes