PM-26577: Add support for authentication app links #6277
Conversation
david-livefront
requested review from
a team and
brian-livefront
as code owners
|
Claude finished @david-livefront's task in 2m 54s —— View job I'll analyze this and get back to you. |
|
Great job! No new security vulnerabilities introduced in this pull request |
david-livefront
force-pushed
the
PM-26577-app-links-suppoort
branch
from
35b8f13 to
fb60c24
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| android:launchMode="singleTop" | ||
| android:noHistory="true" | ||
| android:theme="@android:style/Theme.NoDisplay"> | ||
| <intent-filter android:autoVerify="true"> |
There was a problem hiding this comment.
❓ App Link Verification Status
The PR adds android:autoVerify="true" for app links on bitwarden.com, bitwarden.eu, and bitwarden.pw domains. For secure operation, Digital Asset Links (assetlinks.json) files must be deployed and verified.
Questions:
- Have assetlinks.json files been deployed to all three domains?
- Has app link verification been tested on Android 6+ devices?
- What's the fallback behavior if verification fails?
Security Impact: Without proper verification, malicious apps could potentially intercept authentication callbacks by claiming the same intent filters.
| sendAction( | ||
| EnterpriseSignOnAction.Internal.OnGenerateUriForSsoResult( | ||
| uri = uri.toUri(), | ||
| scheme = requireNotNull(prevalidateSsoResult.redirectUri.toUri().scheme), |
There was a problem hiding this comment.
requireNotNull(prevalidateSsoResult.redirectUri.toUri().scheme) will crash if the scheme is null, but doesn't validate the scheme value itself.
Problem: An attacker-controlled server could send a redirectUri with an unexpected scheme (e.g., "http", "javascript", "file") causing crashes or security issues.
Suggested fix:
val scheme = prevalidateSsoResult.redirectUri.toUri().scheme
require(scheme == "bitwarden" || scheme == "https") { "Invalid redirect scheme" }
| return if (action == Intent.ACTION_VIEW && localData?.host == SSO_HOST) { | ||
| return if (action == Intent.ACTION_VIEW && | ||
| // We check the host for Deeplinks, and the path for App Links. | ||
| (localData?.host == SSO_HOST || localData?.path == SSO_HOST) |
There was a problem hiding this comment.
❓ Path-Based Intent Matching Verification
The condition localData?.host == SSO_HOST || localData?.path == SSO_HOST checks both host and path.
Questions:
- For app links (https://bitwarden.com/sso-callback), does
Uri.pathreturn "/sso-callback" or "sso-callback"? - Has this been tested with actual app link URIs to confirm the path matching works?
Risk: If path doesn't match expected format, app links will silently fail.
| ?.let { token -> | ||
| PrevalidateSsoResult.Success( | ||
| token = token, | ||
| redirectUri = response |
There was a problem hiding this comment.
The server-provided redirectUri is accepted without validation, only checked if blank.
Problem: Server compromise could inject malicious redirect URIs (file://, javascript://, arbitrary https URLs).
Suggested fix:
private fun isValidRedirectUri(uri: String): Boolean {
val parsed = uri.toUri()
return when (parsed.scheme) {
"bitwarden" -> parsed.host in listOf("sso-callback", "duo-callback", "webauthn-callback")
"https" -> parsed.host in listOf("bitwarden.com", "bitwarden.eu", "bitwarden.pw") &&
parsed.path in listOf("/sso-callback", "/duo-callback", "/webauthn-callback")
else -> false
}
}
david-livefront
force-pushed
the
PM-26577-app-links-suppoort
branch
from
fb60c24 to
79f8019
Compare
Code Review Summary: App Links Authentication SupportOverall Assessment: REQUEST CHANGES Critical Issues: Important Issues: See detailed findings below. |
Finding 1: Missing Redirect URI ValidationLocation: Severity: ❌ CRITICAL Issue: The code accepts redirect URIs from server responses without validation in Attack Scenario:
Required Fix:
These must match the intent filters declared in AndroidManifest.xml. Reject any other redirect URIs and fall back to default deep link. |
Finding 2: App Link Verification Configuration Missing DocumentationLocation: Severity: ❌ CRITICAL Issue: The PR adds
Security Impact:
Required Fix:
|
Finding 3: Duo Redirect URI Validation MissingLocation: Severity: ❌ CRITICAL Issue: The Code: val GetTokenResponseJson.TwoFactorRequired?.twoFactorDuoRedirectUri: String
get() = this
?.authMethodsData
?.duo
?.get("RedirectUri")
?.jsonPrimitive
?.contentOrNull
?: "bitwarden://duo-callback" // Only used if nullAttack Vector: Required Fix: val GetTokenResponseJson.TwoFactorRequired?.twoFactorDuoRedirectUri: String
get() {
val serverUri = this?.authMethodsData?.duo
?.get("RedirectUri")?.jsonPrimitive?.contentOrNull
return if (serverUri?.isValidCallbackUri() == true) {
serverUri
} else {
"bitwarden://duo-callback"
}
} |
Finding 4: WebAuthn Redirect URI Validation MissingLocation: Severity: ❌ CRITICAL Issue: Similar to Duo, the Code: fun JsonObject.getRedirectUri(): String = this["RedirectUri"]
?.jsonPrimitive
?.contentOrNull
?: CALLBACK_URI // "bitwarden://webauthn-callback"Attack Vector: Required Fix: |
Finding 5: Redirect URI Reconstruction May Not Match OriginalLocation: Severity: Issue: The SSO callback result reconstruction removes path, query, and fragment from the original URI. This assumes all app links use base domain only, but the AndroidManifest allows path patterns. Code: redirectUri = this
.buildUpon()
.path(null) // Removes /sso-callback path
.query(null)
.fragment(null)
.build()
.toString(),Problem: Required Fix: redirectUri = this
.buildUpon()
.apply {
// Keep path for app links, remove for deep links
if (this@getSsoCallbackResult?.scheme != "bitwarden") {
// For app links, keep path but remove query/fragment
query(null)
fragment(null)
} else {
// For deep links, remove all
path(null)
query(null)
fragment(null)
}
}
.build()
.toString(), |
|
Finding 6: requireNotNull on Scheme Could Crash Application Location: app/src/main/kotlin/com/x8bit/bitwarden/ui/auth/feature/enterprisesignon/EnterpriseSignOnViewModel.kt:411 Severity: IMPORTANT Issue: Using requireNotNull on redirect URI scheme will crash the app if server provides malformed URI without scheme. If redirectUri lacks scheme, .toUri().scheme returns null causing crash. Required Fix: Handle null scheme gracefully with fallback to default scheme. Apply to all three locations: EnterpriseSignOnViewModel.kt:411, TwoFactorLoginViewModel.kt:195, TwoFactorLoginViewModel.kt:234 |
|
Finding 7: Missing Test Coverage for App Link Path Matching Location: app/src/test/kotlin/com/x8bit/bitwarden/data/auth/repository/util/SsoUtilsTest.kt Severity: IMPORTANT Issue: Tests only verify deep link host matching (bitwarden://sso-callback) but not new app link path matching (https://bitwarden.com/sso-callback). The implementation checks both host and path but tests do not cover app link scenarios. Code in SsoUtils.kt line 70: Missing test cases:
Required Fix: Add test coverage for app link path pattern matching in SsoUtilsTest, DuoUtilsTest, and create WebAuthUtilsTest if it does not exist. |
|
Finding 8: Intent Filter Path Pattern Too Broad Location: app/src/main/AndroidManifest.xml:85 Severity: SUGGESTED Issue: The pathPattern="/redirect-connector.*" uses wildcard matching which could unintentionally match paths like /redirect-connector-admin or /redirect-connector/evil/path. While not currently exploitable, overly broad patterns increase attack surface. Current: Recommendation: Use exact path match if possible, or limit wildcards: Or if query parameters needed: Verify with product team whether wildcard is required for this endpoint. |
|
Finding 9: Inconsistent Redirect URI Handling Between Auth Methods Location: Multiple files Severity: TECHNICAL DEBT Issue: SSO uses dynamic redirect URI from server, while Duo and WebAuthn have hardcoded defaults with server override capability. This inconsistency makes security auditing harder and could lead to different vulnerability profiles. SSO: Always uses server-provided or falls back to DEFAULT_SSO_REDIRECT_URI Recommendation: Standardize redirect URI handling across all auth methods. Create single validation function used by all three flows. Document the security model in code comments. Example: This would prevent security fixes from being applied inconsistently. |
|
Finding 10: Missing Security Documentation in PR Description Location: PR description Severity: SUGGESTED Issue: The PR description states this is on-hold waiting for final requirements and API spec verification, but does not document:
Given this adds new authentication redirect flows with security implications, the PR should document security review findings and deployment approach. Recommendation: Update PR description with:
|
|
QUESTION: Redirect URI Scheme Determination Strategy Location: app/src/main/kotlin/com/x8bit/bitwarden/ui/auth/feature/enterprisesignon/EnterpriseSignOnViewModel.kt:411 The code extracts the redirect scheme from server-provided redirect URI to pass to IntentManager.startAuthTab. This determines whether to use deep links (bitwarden://) or app links (https://). Question: What is the expected behavior when:
This affects whether validation should allow mixed schemes or enforce consistency per environment/session. |
Final Review AssessmentOverall Assessment: REQUEST CHANGES Critical Security Issues Identified: 4 CRITICAL ISSUES (must fix before merge):
IMPORTANT ISSUES (should fix before merge): TECHNICAL DEBT: SUGGESTIONS: POSITIVE ASPECTS:
The implementation is well-structured but requires security hardening before release. The redirect URI validation is the highest priority fix. |
david-livefront
force-pushed
the
PM-26577-app-links-suppoort
branch
from
79f8019 to
0379b9b
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6277 +/- ##
==========================================
- Coverage 85.53% 85.50% -0.04%
==========================================
Files 761 762 +1
Lines 54522 54590 +68
Branches 7833 7850 +17
==========================================
+ Hits 46636 46677 +41
- Misses 5169 5185 +16
- Partials 2717 2728 +11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🎟️ Tracking
PM-26577
📔 Objective
This PR adds support to app links. It is currently on-hold while we wait for final requirements and verification on the API spec.
⏰ 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