Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a new Android sample application demonstrating the integration of the Android Credential Manager API to request and parse digital credentials (such as Mobile Driver's Licenses and Verified Emails). The review feedback highlights several critical issues that need to be addressed: using Modifier.weight inside a scrollable Column will throw an IllegalStateException at runtime; referencing Theme.AppCompat in the manifest without declaring the appcompat dependency will cause build or runtime failures; casting context directly to Activity is unsafe and should be replaced with a recursive unwrapper; and an off-by-one error in the SD-JWT parser may skip the last disclosure. Additionally, a hardcoded string should be moved to resources for localization.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new Android sample application, DigitalCredentialsApp, demonstrating integration with the Android Credential Manager API to request and parse Mobile Driver's Licenses (mDL) and Verified Emails. It includes a custom CBOR decoder, utility classes for OpenID4VP request generation, and a Jetpack Compose UI to display extracted claims. The review feedback highlights a critical security vulnerability in the CBOR decoder regarding resource exhaustion (OOM and stack overflow) from untrusted payloads, and suggests enhancing the legacy parsing fallback to support flat strings and JSON arrays for broader wallet compatibility.
| private fun handleLegacyParsing(vpToken: Any, claims: MutableList<CredentialClaim>) { | ||
| try { | ||
| if (vpToken is JSONObject) { | ||
| val keys = vpToken.keys() | ||
| while (keys.hasNext()) { | ||
| val key = keys.next() | ||
| val tokenArray = vpToken.optJSONArray(key) | ||
| if (tokenArray != null && tokenArray.length() > 0) { | ||
| val rawToken = tokenArray.getString(0) | ||
| if (rawToken.contains("~")) { | ||
| claims.addAll(parseSdJwtClaims(rawToken)) | ||
| } else { | ||
| claims.addAll(parseMdocClaims(rawToken)) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } catch (e: Exception) { | ||
| Log.e("CredentialManagerUtil", "Fallback parsing failed", e) | ||
| } | ||
| } |
There was a problem hiding this comment.
The handleLegacyParsing function currently only handles cases where vpToken is a JSONObject. However, in many standard or simpler OpenID4VP responses (especially when presentation_submission is missing), vp_token can be returned directly as a flat String (the raw token) or a JSONArray of strings.
Updating handleLegacyParsing to support String and JSONArray types will significantly improve compatibility with various digital wallet implementations.
private fun handleLegacyParsing(vpToken: Any, claims: MutableList<CredentialClaim>) {
try {
when (vpToken) {
is String -> {
if (vpToken.contains("~")) {
claims.addAll(parseSdJwtClaims(vpToken))
} else {
claims.addAll(parseMdocClaims(vpToken))
}
}
is JSONArray -> {
for (i in 0 until vpToken.length()) {
val token = vpToken.optString(i) ?: continue
if (token.contains("~")) {
claims.addAll(parseSdJwtClaims(token))
} else {
claims.addAll(parseMdocClaims(token))
}
}
}
is JSONObject -> {
val keys = vpToken.keys()
while (keys.hasNext()) {
val key = keys.next()
val tokenArray = vpToken.optJSONArray(key)
if (tokenArray != null && tokenArray.length() > 0) {
val rawToken = tokenArray.getString(0)
if (rawToken.contains("~")) {
claims.addAll(parseSdJwtClaims(rawToken))
} else {
claims.addAll(parseMdocClaims(rawToken))
}
} else {
val rawToken = vpToken.optString(key)
if (rawToken.isNotEmpty()) {
if (rawToken.contains("~")) {
claims.addAll(parseSdJwtClaims(rawToken))
} else {
claims.addAll(parseMdocClaims(rawToken))
}
}
}
}
}
}
} catch (e: Exception) {
Log.e("CredentialManagerUtil", "Fallback parsing failed", e)
}
}There was a problem hiding this comment.
Why are we creating strings manually rather than calling a JSON library?
There was a problem hiding this comment.
I don't believe a standard JSON library for constructing DC API requests exists on Android yet; this is also what we showcase in our DAC pages. I do agree it's a bit error-prone though - would be curious what @QZHelen thinks here.
There was a problem hiding this comment.
I'm not asking for a JSON library that is aware of DC API, but just a generic JSON library to serialize a JSON object rather than manual string interpolation. String interpolation is brittle and may introduce subtle bugs.
There was a problem hiding this comment.
Got it. This makes sense as a best practice, so I've updated it.
This works with CMWallet and GPM (for email verification).