[PM-28157] Add string extension to prefix URIs with www #6183
Conversation
Implements prefixWwwIfNecessaryOrNull() and prefixWwwIfNecessary() string extension functions to ensure relying party IDs are properly formatted with www. prefix for Digital Asset Link validation. - Add prefixWww extension functions with KDoc - Update OriginManager to apply www prefix before https - Add comprehensive unit tests for www prefixing logic 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
SaintPatrck
requested review from
a team,
brian-livefront and
david-livefront
as code owners
|
Claude finished @SaintPatrck's task —— View job Review CompleteOverall Assessment: APPROVE The PR successfully implements www-prefixing for passkey Digital Asset Link validation with solid test coverage and documentation. The latest commit addresses the previously identified test coverage gap. Status of Previous FindingsFinding 1 (
These tests properly validate the end-to-end www-prefixing behavior through the actual Finding 2 (💭 - Security implications): No response required Code Quality✅ Architecture compliance: Follows established dependency injection patterns Minor ObservationsThe implementation correctly:
No blocking issues identified. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| sourceWebSite = relyingPartyId | ||
| .prefixWwwIfNecessary() | ||
| .prefixHttpsIfNecessary(), |
There was a problem hiding this comment.
The existing OriginManagerTest only tests with DEFAULT_RELYING_PARTY_ID = "www.bitwarden.com" (line 251 in the test file), which already has the www prefix. This means the tests don't actually verify the new www-prefixing behavior works correctly.
Recommended test cases to add to OriginManagerTest
@Test
fun `validateOrigin should prefix www to rpId without www before checking asset links`() = runTest {
coEvery {
mockDigitalAssetLinkService.checkDigitalAssetLinksRelations(
sourceWebSite = "https://www.example.com", // Expected: www + https
targetPackageName = DEFAULT_PACKAGE_NAME,
targetCertificateFingerprint = DEFAULT_CERT_FINGERPRINT,
relations = listOf("delegate_permission/common.handle_all_urls"),
)
} returns DEFAULT_ASSET_LINKS_CHECK_RESPONSE.asSuccess()
val result = originManager.validateOrigin(
relyingPartyId = "example.com", // Input: no www, no https
callingAppInfo = mockNonPrivilegedAppInfo,
)
assertEquals(ValidateOriginResult.Success(null), result)
}
@Test
fun `validateOrigin should preserve existing www prefix when present`() = runTest {
coEvery {
mockDigitalAssetLinkService.checkDigitalAssetLinksRelations(
sourceWebSite = "https://www.example.com",
targetPackageName = DEFAULT_PACKAGE_NAME,
targetCertificateFingerprint = DEFAULT_CERT_FINGERPRINT,
relations = listOf("delegate_permission/common.handle_all_urls"),
)
} returns DEFAULT_ASSET_LINKS_CHECK_RESPONSE.asSuccess()
val result = originManager.validateOrigin(
relyingPartyId = "www.example.com", // Input: already has www
callingAppInfo = mockNonPrivilegedAppInfo,
)
assertEquals(ValidateOriginResult.Success(null), result)
}
@Test
fun `validateOrigin should handle rpId with https scheme correctly`() = runTest {
coEvery {
mockDigitalAssetLinkService.checkDigitalAssetLinksRelations(
sourceWebSite = "https://www.example.com",
targetPackageName = DEFAULT_PACKAGE_NAME,
targetCertificateFingerprint = DEFAULT_CERT_FINGERPRINT,
relations = listOf("delegate_permission/common.handle_all_urls"),
)
} returns DEFAULT_ASSET_LINKS_CHECK_RESPONSE.asSuccess()
val result = originManager.validateOrigin(
relyingPartyId = "https://example.com", // Input: has https but no www
callingAppInfo = mockNonPrivilegedAppInfo,
)
assertEquals(ValidateOriginResult.Success(null), result)
}These tests would verify that:
example.com→https://www.example.comwww.example.com→https://www.example.comhttps://example.com→https://www.example.com
| .checkDigitalAssetLinksRelations( | ||
| sourceWebSite = relyingPartyId.prefixHttpsIfNecessary(), | ||
| sourceWebSite = relyingPartyId | ||
| .prefixWwwIfNecessary() |
There was a problem hiding this comment.
💭 Consider the security implications of automatic www prefixing
While the www-prefixing improves compatibility, it's worth considering the security implications:
Current behavior: If a site hosts their Digital Asset Links file at https://www.example.com/.well-known/assetlinks.json but NOT at https://example.com/.well-known/assetlinks.json, this code will allow passkey authentication by automatically trying the www variant.
Question: Is this the intended behavior? Should we:
- Keep current approach (automatic fallback to www) - More permissive, better UX
- Try both variants (with and without www) - Most robust
- Only use the exact rpId provided - Most restrictive, follows spec strictly
From a security perspective, option 1 (current) seems reasonable since:
- DAL files are cryptographically verifiable via certificate fingerprints
- The www subdomain is typically controlled by the same entity as the apex domain
- This aligns with how browsers handle www/non-www redirects
However, this should be documented as an intentional security decision, not just a compatibility fix.
| } | ||
|
|
||
| else -> "www.$this" | ||
| } |
There was a problem hiding this comment.
👍 Excellent implementation
The logic correctly handles all cases:
- Blank/invalid URIs →
null - Already has
www.prefix → unchanged - Has scheme (
http://orhttps://) → insertswww.after scheme - No scheme → prepends
www.
The check for "://www." prevents double-prefixing when www is already present after a scheme, which is good.
Edge case verification
Verified the logic handles these correctly:
"example.com"→"www.example.com"✓"www.example.com"→"www.example.com"✓"http://example.com"→"http://www.example.com"✓"https://www.example.com"→"https://www.example.com"✓""→null✓"invalid uri"→null✓
| val invalidUri = "invalid uri" | ||
| assertNull(invalidUri.prefixWwwIfNecessaryOrNull()) | ||
| } | ||
| } |
There was a problem hiding this comment.
👍 Comprehensive test coverage for string extensions
The test coverage for the new prefixWwwIfNecessaryOrNull() function is excellent:
- ✅ No scheme, no www
- ✅ Already has www prefix
- ✅ HTTP scheme without www
- ✅ HTTPS scheme without www
- ✅ HTTP scheme with www
- ✅ HTTPS scheme with www
- ✅ Empty string
- ✅ Invalid URI
All major edge cases are covered. Good work!
|
Great job! No new security vulnerabilities introduced in this pull request |
- Documented why we are injecting `www` when checking digital asset links relations. - Added test cases for `OriginManagerImpl`.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6183 +/- ##
=======================================
Coverage 85.22% 85.22%
=======================================
Files 724 724
Lines 53005 53014 +9
Branches 7705 7710 +5
=======================================
+ Hits 45173 45181 +8
Misses 5143 5143
- Partials 2689 2690 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Thanks @david-livefront |
This reverts commit 7b491d3.
🎟️ Tracking
PM-28157
Relates to Issue #6164
📔 Objective
This PR adds support for prefixing relying party IDs (rpIds) with "www." during passkey Digital Asset Link validation to improve compatibility with web origins that use the www subdomain.
When validating passkey requests, the Android Credential Manager may receive rpIds in various formats (e.g.,
example.com,www.example.com,https://example.com). However, Digital Asset Link files are often hosted athttps://www.example.com/.well-known/assetlinks.json. Without proper www prefixing, the validation can fail even when valid DAL relationships exist.Changes:
prefixWwwIfNecessaryOrNull()andprefixWwwIfNecessary()string extension functions that intelligently add "www." prefix to URIs while preserving existing schemesOriginManagerImpl.validateCallingApplicationAssetLinks()to apply www prefixing before https prefixing during DAL validationThis ensures rpIds are properly normalized to
https://www.{domain}format before querying Digital Asset Links, improving passkey authentication success rates for websites using www subdomains.⏰ 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