[INJIVER-1369] - Add verification and validation for cwt formet VC with new library#216
Conversation
Signed-off-by: jaswanthkumarpolisetty <[email protected]>
Signed-off-by: jaswanthkumarpolisetty <[email protected]>
Signed-off-by: jaswanthkumarpolisetty <[email protected]>
WalkthroughAdds CWT (CBOR Web Token) support: new CwtValidator and CwtVerifier, a CwtVerifiableCredential type and factory branch, JWKS-based public key resolution with optional keyId, JWK-to-PublicKey utilities, hex/CBOR helpers, unit tests, and Gradle dependency/config updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant CredentialVerifierFactory
participant CwtVerifiableCredential
participant CwtValidator
participant Util
Client->>CredentialVerifierFactory: get(CWT_VC)
CredentialVerifierFactory->>CwtVerifiableCredential: instantiate
CredentialVerifierFactory-->>Client: CwtVerifiableCredential
Client->>CwtVerifiableCredential: validate(credential)
CwtVerifiableCredential->>CwtValidator: validate(credential)
CwtValidator->>Util: hexToBytes(credential)
Util-->>CwtValidator: bytes
CwtValidator->>CwtValidator: decode COSE_Sign1 & parse CBOR claims
CwtValidator->>Util: validateNumericDate(claims, exp/nbf/iat)
CwtValidator-->>CwtVerifiableCredential: ValidationStatus
CwtVerifiableCredential-->>Client: ValidationStatus
sequenceDiagram
participant Client
participant CwtVerifiableCredential
participant CwtVerifier
participant Util
participant PublicKeyResolverFactory
participant JwksPublicKeyResolver
Client->>CwtVerifiableCredential: verify(credential)
CwtVerifiableCredential->>CwtVerifier: verify(credential)
CwtVerifier->>Util: hexToBytes(credential)
Util-->>CwtVerifier: bytes
CwtVerifier->>CwtVerifier: decode COSE_Sign1 & extract claims, iss, kid
CwtVerifier->>PublicKeyResolverFactory: get(issuerURI, keyId)
PublicKeyResolverFactory->>JwksPublicKeyResolver: resolve(jwksUrl, keyId)
JwksPublicKeyResolver->>JwksPublicKeyResolver: fetch JWKS, select key by kid
JwksPublicKeyResolver-->>PublicKeyResolverFactory: PublicKey
PublicKeyResolverFactory-->>CwtVerifier: PublicKey
CwtVerifier->>CwtVerifier: verifySignature(coseBytes, PublicKey)
CwtVerifier-->>CwtVerifiableCredential: boolean
CwtVerifiableCredential-->>Client: boolean
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (3)📓 Common learnings📚 Learning: 2026-01-07T10:57:37.322ZApplied to files:
📚 Learning: 2026-01-09T05:57:56.476ZApplied to files:
🔇 Additional comments (1)
✏️ Tip: You can disable this entire section by setting Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
jaswanthkumartw
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI Agents
In @vc-verifier/kotlin/gradle/libs.versions.toml:
- Around line 23-24: The jackson dependency versions are inconsistent:
libs.versions.toml sets jacksonAnnotations and jacksonModuleKotlin to 2.17.2
while vcverifier/build.gradle.kts forces jackson-core:2.14.0; update the forced
dependency in build.gradle.kts (the jackson-core version pin around the
dependency declaration) to 2.17.2 to match
jacksonAnnotations/jacksonModuleKotlin, ensure jackson-module-kotlin and
jackson-core are aligned, then run and validate Jackson-dependent code (Kotlin
serialization/deserialization, polymorphic JsonTypeInfo, enum handling and any
code that may be affected by StreamRead/Write and coercion changes) to address
breaking changes introduced between 2.14.0 and 2.17.2.
In @vc-verifier/kotlin/vcverifier/build.gradle.kts:
- Around line 76-81: The global exclusions in the configurations.all block are
removing the same BouncyCastle artifact you explicitly add via
implementation(libs.bouncyCastle) and conflict with other configurations.all
blocks; replace the global exclusions with per-dependency excludes on the
dependencies that bring in the unwanted bcprov/bcpkix/bcutil transitive
artifacts (e.g., update implementation(libs.cose.lib) and any other dependency
entries that transitively pull BouncyCastle to add
exclude(group="org.bouncycastle", module="bcprov-jdk15on") /
exclude(group="org.bouncycastle", module="bcpkix-jdk15on") /
exclude(group="org.bouncycastle", module="bcutil-jdk15on") so you can keep
implementation(libs.bouncyCastle) while removing only the conflicting transitive
versions and remove the contradictory configurations.all exclusions.
- Line 68: The dependency declaration implementation(libs.cose.lib) references
se.digg.cose:cose-lib version 2.0.0 which is not on Maven Central; either
confirm the required repository (e.g., Danubetech or JitPack) is added to the
Gradle repositories block or change the version to one that is published (update
the libs.versions or the explicit version in build.gradle.kts). Locate the
implementation(libs.cose.lib) entry and ensure the repository that hosts
se.digg.cose:cose-lib is configured (or replace libs.cose.lib with a resolvable
version) so the project can resolve the dependency during build.
In
@vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt:
- Line 3: The import android.net.Uri in CwtVerifier.kt is unused and
Android-specific; remove the unused import statement from the top of the file
(the line importing android.net.Uri) so the CwtVerifier class and its functions
compile cleanly in non-Android JVM environments.
- Around line 19-26: Remove the duplicate local function hexToBytes from
CwtVerifier and replace its usages with the existing utility: call
Util.hexToBytes(...) (or add the single-function import
io.mosip.vercred.vcverifier.utils.Util.hexToBytes and call hexToBytes(...));
ensure all references inside CwtVerifier that previously called the local
hexToBytes now call the util method and delete the redundant function
definition.
- Around line 139-153: In verify(credential) fix missing error handling around
PublicKeyResolverFactory().get(issuer) by wrapping the call in a try-catch and
returning false if it throws, and also check the returned publicKey for null and
return false before calling verifySignature(coseBytes, publicKey); additionally
remove the unnecessary semicolons after statements (e.g., the semicolons after
decodeCose(...), isValidCoseStructure(...), CBORObject.DecodeFromBytes(...), and
isValidCwtStructure(...)) so the method consistently follows the defensive
return-false pattern.
- Around line 77-117: Dead-code: fetchPublicKey and extractKid implement JWKS
resolution but are never used; either remove them or wire them into the
verification flow. To fix, either delete the unused methods fetchPublicKey(...)
and extractKid(...) (clean up imports) or modify the verify() path that
currently uses PublicKeyResolverFactory to call fetchPublicKey(coseObj,
issuerMetadataJson) (or a new adapter method) and fall back to
PublicKeyResolverFactory if fetchPublicKey returns null; ensure the verify()
caller supplies issuerMetadataJson (parse issuer metadata where needed) and
handle null PublicKey results consistently with existing error handling.
In
@vc-verifier/kotlin/vcverifier/src/test/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifierTest.kt:
- Around line 22-92: The CwtVerifierTest's entire test method `should verify
valid CWT` is commented out leaving CwtVerifier untested; either re-enable and
fix the test, remove it with a TODO and issue, or document why it's disabled. To
fix: restore the test in class `CwtVerifierTest` (uncomment the `should verify
valid CWT` method), ensure necessary imports and test annotations are present,
fix any runtime issues by mocking `Util.httpGet` correctly (the current mock
targets `Util.httpGet(any())` and must return the `issuerMetadataJson` and
`jwksJson` for `"$issuer/.well-known/openid-credential-issuer"` and
`"$issuer/jwks"`), and verify the COSE creation code (references:
`Sign1COSEObject`, `COSEKey`, `ECKey.Builder`, `KeyPairGenerator`) compiles and
the asserted call `CwtVerifier().verify(coseHex)` returns true; alternatively,
if intentionally disabled remove the commented block in `CwtVerifierTest` and
add a single-line TODO with a tracking issue referencing `CwtVerifier` and
`should verify valid CWT`.
🧹 Nitpick comments (11)
vc-verifier/kotlin/example/build.gradle.kts (1)
51-51: Consider documenting the reason for this exclusion.The exclusion of
META-INF/versions/9/OSGI-INF/MANIFEST.MFsuggests a duplicate resource conflict, likely introduced by the new COSE library or updated dependencies. While this exclusion is valid, consider adding a comment explaining which dependency causes the conflict to help future maintainers.🔎 Suggested improvement
excludes += "/META-INF/{AL2.0,LGPL2.1}" + // Exclude duplicate OSGI manifest from cose-lib or transitive dependencies excludes += "META-INF/versions/9/OSGI-INF/MANIFEST.MF"vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/constants/CredentialValidatorConstants.kt (1)
55-58: Fix formatting and consider adding error codes for consistency.Formatting issues:
- Missing space after
=operator on lines 55 and 57 (inconsistent with the rest of the file)- Unnecessary blank lines at lines 56 and 58 (breaks the file's consistent spacing pattern)
Optional improvement:
Similar toERROR_MESSAGE_EMPTY_VC_JSONhaving a correspondingERROR_CODE_EMPTY_VC_JSON(lines 36, 42), consider adding error codes for the new CWT constants (e.g.,ERROR_CODE_EMPTY_VC_CWTandERROR_CODE_INVALID_HEX_VC_CWT) to maintain consistency across validation error handling.🔎 Proposed formatting fix
- const val ERROR_MESSAGE_EMPTY_VC_CWT="${VALIDATION_ERROR}Input VC CWT string is null or empty." - - const val ERROR_MESSAGE_INVALID_HEX_VC_CWT="${VALIDATION_ERROR}Invalid hexadecimal format" - + const val ERROR_MESSAGE_EMPTY_VC_CWT = "${VALIDATION_ERROR}Input VC CWT string is null or empty." + const val ERROR_MESSAGE_INVALID_HEX_VC_CWT = "${VALIDATION_ERROR}Invalid hexadecimal format"vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/types/CwtVerifiableCredential.kt (1)
14-20: Remove unnecessary semicolons and consider documenting the no-opcheckStatus.Kotlin doesn't require semicolons. Additionally, consider adding a
TODOcomment tocheckStatusif status checking for CWT credentials is planned for future implementation, or document why it's intentionally a no-op.🔎 Proposed fix
override fun verify(credential: String): Boolean { - return CwtVerifier().verify(credential); + return CwtVerifier().verify(credential) } + // TODO: Implement status checking for CWT credentials if required override fun checkStatus(credential: String, statusPurposes: List<String>?): Map<String, CredentialStatusResult> { - return emptyMap(); + return emptyMap() }vc-verifier/kotlin/vcverifier/src/test/java/io/mosip/vercred/vcverifier/credentialverifier/validator/CwtValidatorTest.kt (2)
13-15: Remove the emptysetup()method.The empty
@BeforeEachmethod adds no value and was flagged by static analysis.🔎 Proposed fix
- @BeforeEach - fun setup() { - } - private val validator = CwtValidator()
34-46: Consider consistent test naming convention.Test method names use inconsistent prefixes (
validate -vstest -). Consider standardizing to a single pattern for better readability and organization.vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/utils/Util.kt (3)
36-36: Consider makingrestTemplateprivate.Exposing
restTemplateas a public val leaks the internal HTTP implementation. If external access is needed, consider providing a factory method or making it internal/private.🔎 Proposed fix
- val restTemplate = RestTemplate() + private val restTemplate = RestTemplate()
152-159:hexToBytesthrows unchecked exception on invalid hex characters.If the input contains non-hex characters (e.g., "ZZ"),
toInt(16)throwsNumberFormatException. WhileCwtValidatorvalidates hex before calling this, other callers may not. Consider adding character validation or documenting the precondition.🔎 Proposed fix
fun hexToBytes(hex: String): ByteArray { val cleanHex = hex.replace("\\s".toRegex(), "") require(cleanHex.length % 2 == 0) { "Invalid hex length" } + require(cleanHex.matches("^[0-9a-fA-F]*$".toRegex())) { "Invalid hex characters" } return ByteArray(cleanHex.length / 2) { i -> cleanHex.substring(i * 2, i * 2 + 2).toInt(16).toByte() } }
177-183: Consider logging HTTP failures for debuggability.Silently returning
nullon any exception (network errors, timeouts, HTTP 4xx/5xx) can make debugging difficult. Consider logging at debug/trace level.vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/validator/CwtValidator.kt (2)
130-135: UseisIntegral()instead ofisNumberfor stricteralgvalidation.Per COSE specification,
algmust be an integer, not a floating-point number.isNumberreturns true for floats as well. Consider usingisIntegral()for stricter validation.🔎 Proposed fix
- if (!protectedHeader[ALG].isNumber) { + if (!protectedHeader[ALG].isIntegral) { throw ValidationException( ERROR_INVALID_FIELD + "alg must be an integer", ERROR_CODE_INVALID + "ALG" ) }
204-207: Inconsistency withUtil.hexToByteswhitespace handling.
isValidHexrejects any whitespace, butUtil.hexToBytesstrips whitespace before processing. Consider aligning behavior - either allow whitespace in validation (strip before regex check) or document that whitespace is not permitted.🔎 Proposed fix to align with Util.hexToBytes
private fun isValidHex(credential: String): Boolean { + val cleanCredential = credential.replace("\\s".toRegex(), "") val hexRegex = "^[0-9a-fA-F]+$".toRegex() - return credential.matches(hexRegex) + return cleanCredential.isNotEmpty() && cleanCredential.matches(hexRegex) }vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt (1)
14-14: Remove unused importjava.net.URL.🔎 Proposed fix
-import java.net.URL
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
vc-verifier/kotlin/example/build.gradle.ktsvc-verifier/kotlin/example/src/main/java/io/mosip/vccred/example/MainActivity.ktvc-verifier/kotlin/gradle/libs.versions.tomlvc-verifier/kotlin/vcverifier/build.gradle.ktsvc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/constants/CredentialFormat.ktvc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/constants/CredentialValidatorConstants.ktvc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/CredentialVerifierFactory.ktvc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/types/CwtVerifiableCredential.ktvc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/validator/CwtValidator.ktvc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.ktvc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/utils/Util.ktvc-verifier/kotlin/vcverifier/src/test/java/io/mosip/vercred/vcverifier/credentialverifier/validator/CwtValidatorTest.ktvc-verifier/kotlin/vcverifier/src/test/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifierTest.kt
👮 Files not reviewed due to content moderation or server errors (1)
- vc-verifier/kotlin/example/src/main/java/io/mosip/vccred/example/MainActivity.kt
🧰 Additional context used
🧬 Code graph analysis (2)
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/validator/CwtValidator.kt (1)
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/utils/Util.kt (2)
validateNumericDate(161-175)hexToBytes(152-159)
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt (1)
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/utils/Util.kt (2)
hexToBytes(152-159)httpGet(177-183)
🪛 detekt (1.23.8)
vc-verifier/kotlin/vcverifier/src/test/java/io/mosip/vercred/vcverifier/credentialverifier/validator/CwtValidatorTest.kt
[warning] 14-15: This empty block of code can be removed.
(detekt.empty-blocks.EmptyFunctionBlock)
🔇 Additional comments (4)
vc-verifier/kotlin/gradle/libs.versions.toml (1)
30-30: LGTM! New COSE library properly configured.The new
coseLibraryversion reference andcose-liblibrary alias are correctly defined and follow the existing conventions in the version catalog.Also applies to: 66-66
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/CredentialVerifierFactory.kt (1)
4-4: LGTM! Factory properly extended for CWT format.The addition of
CwtVerifiableCredentialto the factory follows the existing pattern and correctly handles the newCredentialFormat.CWT_VCformat.Also applies to: 16-16
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/constants/CredentialFormat.kt (1)
4-4: LGTM! New credential format added correctly.The
CWT_VCenum constant follows the existing naming pattern and integrates seamlessly with thefromValue()method.vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/validator/CwtValidator.kt (1)
24-59: LGTM - Clean validation flow with proper error handling.The validation structure is well-organized with clear separation between structure validation, header validation, and date validation. Exception-based control flow provides consistent error reporting.
...erifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt
Outdated
Show resolved
Hide resolved
...erifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt
Outdated
Show resolved
Hide resolved
...ier/src/test/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifierTest.kt
Show resolved
Hide resolved
Signed-off-by: jaswanthkumarpolisetty <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
@vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt:
- Around line 19-26: Remove the duplicate hexToBytes function from CwtVerifier
and replace any internal calls to hexToBytes with Util.hexToBytes; specifically,
delete the fun hexToBytes(...) definition in CwtVerifier.kt, add the appropriate
import or fully qualify calls to Util.hexToBytes wherever this local function
was used, and ensure compilation by resolving any missing imports or references
to the Util class.
- Around line 105-129: The call PublicKeyResolverFactory().get(issuer) can
return null; before calling verifySignature(coseBytes, publicKey) add a null
check and throw PublicKeyNotFoundException (or a suitable exception) with a
clear message including the issuer when publicKey is null; ensure the existing
catch block still propagates PublicKeyNotFoundException so callers see the
specific failure rather than a NullPointerException.
In
@vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/keyResolver/types/did/DidWebPublicKeyResolver.kt:
- Around line 40-42: The current use of substring matching in
verificationMethods.find ((it[ID] as? String)?.contains(verificationMethodId) ==
true) can resolve the wrong key; change the matching to exact equality: compare
the ID string exactly to verificationMethodId (or, if you must support
fragment/URI variants, perform a deterministic comparison such as URI
normalization and exact equality or use suffix/fragment-only matching like
endsWith after validating the scheme) so that verificationMethod selection via
verificationMethodId is unambiguous and not order-dependent.
🧹 Nitpick comments (2)
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt (2)
58-75: Remove unusedextractKidmethod.This method extracts the Key ID from the COSE header but is never called. The
verify()method resolves the public key using only the issuer URI viaPublicKeyResolverFactory, bypassing any KID-based resolution.Either remove this dead code or integrate KID-based key resolution into the verification flow if needed.
28-35: Log swallowed exception indecodeCose.The caught exception is silently swallowed when CBOR decoding fails. This makes debugging difficult when malformed credentials are provided.
📝 Add logging
private fun decodeCose(cwtBytes: ByteArray): CBORObject? { return try { CBORObject.DecodeFromBytes(cwtBytes) } catch (e: Exception) { - + logger.warning("Failed to decode COSE object: ${e.message}") null } }Based on static analysis hints.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
vc-verifier/kotlin/example/src/main/java/io/mosip/vccred/example/MainActivity.ktvc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.ktvc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/keyResolver/types/did/DidWebPublicKeyResolver.ktvc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/utils/Util.ktvc-verifier/kotlin/vcverifier/src/test/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifierTest.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/utils/Util.kt
👮 Files not reviewed due to content moderation or server errors (1)
- vc-verifier/kotlin/example/src/main/java/io/mosip/vccred/example/MainActivity.kt
🧰 Additional context used
🧬 Code graph analysis (1)
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt (1)
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/utils/Util.kt (1)
hexToBytes(149-156)
🪛 detekt (1.23.8)
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt
[warning] 70-70: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
[warning] 87-87: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
...erifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt
Outdated
Show resolved
Hide resolved
...erifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt
Show resolved
Hide resolved
...r/src/main/java/io/mosip/vercred/vcverifier/keyResolver/types/did/DidWebPublicKeyResolver.kt
Show resolved
Hide resolved
Signed-off-by: jaswanthkumarpolisetty <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
@vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt:
- Around line 29-36: Remove the unused helper method decodeCose: it's never
invoked (verify() calls CBORObject.DecodeFromBytes() directly), so delete the
private fun decodeCose(cwtBytes: ByteArray): CBORObject? declaration and its
body to eliminate dead code; if you prefer keeping an abstraction, refactor
verify() to call decodeCose instead, but do not leave an unused decodeCose
method in the class.
- Around line 64-81: The extractKid(CBORObject) method is unused; either remove
it or wire it into the verification path: update verify() in CwtVerifier to call
extractKid(coseObj) and, when a KID is returned, use it to select the matching
key from the resolved JWKS (prefer the KID-matching JWK when constructing the
public key), falling back to issuer-based resolution if no KID or no match;
ensure null-safe handling and remove extractKid if you choose deletion, along
with any now-unused imports.
🧹 Nitpick comments (1)
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt (1)
20-27: Remove duplicatehexToBytes- useUtil.hexToBytesinstead.This function duplicates
Util.hexToBytesfrom the utilities package. Maintaining duplicate code violates DRY and increases maintenance burden.♻️ Proposed fix
Import and use the existing utility:
+import io.mosip.vercred.vcverifier.utils.Util + class CwtVerifier { private val logger = Logger.getLogger(CwtVerifier::class.java.name) - fun hexToBytes(hex: String): ByteArray { - val cleanHex = hex.replace("\\s".toRegex(), "") - require(cleanHex.length % 2 == 0) { "Invalid hex length" } - - return ByteArray(cleanHex.length / 2) { i -> - cleanHex.substring(i * 2, i * 2 + 2).toInt(16).toByte() - } - }Then replace the call at line 115:
- val coseBytes = hexToBytes(credential) + val coseBytes = Util.hexToBytes(credential)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.ktvc-verifier/kotlin/vcverifier/src/test/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifierTest.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- vc-verifier/kotlin/vcverifier/src/test/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifierTest.kt
🧰 Additional context used
🧬 Code graph analysis (1)
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt (1)
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/utils/Util.kt (1)
hexToBytes(149-156)
🪛 detekt (1.23.8)
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt
[warning] 32-32: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
[warning] 125-125: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
...erifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt
Outdated
Show resolved
Hide resolved
Signed-off-by: jaswanthkumarpolisetty <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
@vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt:
- Around line 48-65: Delete the entire commented-out extractKid method block in
CwtVerifier (the commented function named extractKid) to remove dead code;
ensure no remaining references to extractKid exist in the class or callers, run
a build/compile to confirm nothing breaks, and commit the cleanup so Git history
retains the implementation if needed.
- Around line 106-112: The JavaDoc and exception handling around resolving the
public key are inaccurate: PublicKeyResolverFactory.get(issuer) returns a
non-null PublicKey so remove any null-checks, update the JavaDoc on the val
publicKey statement to state it resolves and returns a non-null PublicKey and
declare/cite that it may throw PublicKeyTypeNotSupportedException (not
PublicKeyNotFoundException), and change the catch/rethrow logic that currently
references PublicKeyNotFoundException to instead catch and handle or rethrow
PublicKeyTypeNotSupportedException so the actual thrown exception is handled
when calling PublicKeyResolverFactory.get and before calling
verifySignature(coseBytes, publicKey).
🧹 Nitpick comments (1)
vc-verifier/kotlin/vcverifier/build.gradle.kts (1)
13-15: Consider using per-dependency exclusions for better maintainability.Global exclusions in
configurations.allwork but can be less explicit about which dependencies cause conflicts. Consider applying exclusions directly on dependencies that transitively bring in older BouncyCastle versions (e.g., on line 71'slibs.cose.lib).♻️ Alternative approach with per-dependency exclusions
configurations.all { resolutionStrategy.force( "com.fasterxml.jackson.core:jackson-core:2.14.0") - exclude(group = "org.bouncycastle", module = "bcprov-jdk15on") - exclude(group = "org.bouncycastle", module = "bcpkix-jdk15on") - exclude(group = "org.bouncycastle", module = "bcutil-jdk15on") }Then apply exclusions where they're actually needed:
implementation(libs.cose.lib) { exclude(group = "org.bouncycastle", module = "bcprov-jdk15on") exclude(group = "org.bouncycastle", module = "bcpkix-jdk15on") exclude(group = "org.bouncycastle", module = "bcutil-jdk15on") }This makes it clear which dependencies introduce the conflicting BouncyCastle versions.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
vc-verifier/kotlin/vcverifier/build.gradle.ktsvc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt
🧰 Additional context used
🧬 Code graph analysis (1)
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt (1)
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/utils/Util.kt (1)
hexToBytes(149-156)
🔇 Additional comments (5)
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt (5)
1-21: LGTM! Clean imports and setup.The imports are well-organized, and the past issue with the Android-specific import has been properly addressed. Using
Util.hexToBytesavoids code duplication.
22-26: LGTM! Correct COSE_Sign1 structure validation.The validation correctly enforces that COSE_Sign1 must be a 4-element array (protected headers, unprotected headers, payload, signature).
28-46: LGTM! Proper issuer extraction and scheme handling.The method correctly extracts the issuer claim (CWT claim 1 per RFC 8392) and appropriately handles different URI schemes: DIDs are returned as-is, while HTTP(S) URIs are resolved to their JWKS endpoints.
67-76: LGTM! Correct CWT claims extraction.The method properly extracts the payload from the COSE_Sign1 structure (index 2) and validates that CWT claims form a CBOR map per RFC 8392.
78-93: LGTM! Signature verification logic is sound.The method correctly validates the COSE_Sign1 signature using the resolved public key. The safe cast at line 83-84 properly handles type mismatches.
...erifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt
Outdated
Show resolved
Hide resolved
...erifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt
Outdated
Show resolved
Hide resolved
Signed-off-by: jaswanthkumarpolisetty <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt (1)
76-100: Solid verification flow with correct exception handling.The main
verify()method implements a clean flow:
- Hex decode → CBOR decode → Structure validation
- Claims & issuer extraction
- Public key resolution via factory
- Signature verification
The exception handling at lines 90-98 is correct: specific exceptions (
PublicKeyNotFoundException,SignatureVerificationException) are propagated, while unexpected exceptions are wrapped inUnknownExceptionwith context. The static analysis warning about "SwallowedException" is a false positive—no exceptions are silently discarded.Minor consideration: Line 87 instantiates a new
PublicKeyResolverFactory()on every verification. If the factory is stateless, this is fine. If it has initialization overhead or caches, consider making it a class-level property.Based on learnings,
PublicKeyResolverFactory().get(issuer)throwsPublicKeyNotFoundExceptionwhen resolution fails (rather than returning null), so the current exception handling is appropriate.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
vc-verifier/kotlin/gradle/libs.versions.tomlvc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- vc-verifier/kotlin/gradle/libs.versions.toml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-07T10:57:30.150Z
Learnt from: jaswanthkumartw
Repo: inji/vc-verifier PR: 216
File: vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt:95-123
Timestamp: 2026-01-07T10:57:30.150Z
Learning: In the CwtVerifier class, the `PublicKeyResolverFactory().get(issuer)` method throws `PublicKeyNotFoundException` when a key cannot be resolved, rather than returning null.
Applied to files:
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt
🪛 detekt (1.23.8)
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt
[warning] 90-90: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🔇 Additional comments (4)
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt (4)
22-26: LGTM! Correct COSE_Sign1 structure validation.The validation correctly checks that the COSE object is an array with exactly 4 elements (protected headers, unprotected headers, payload, and signature), per the COSE_Sign1 specification.
48-57: LGTM! Correct CWT claims extraction and validation.The method correctly extracts the payload from index 2 of the COSE_Sign1 array, decodes it as CBOR, and validates it's a map structure containing the CWT claims.
59-74: LGTM! Correct COSE signature verification.The signature verification correctly:
- Decodes and validates the COSE_Sign1 object structure
- Creates a COSEKey from the resolved public key (with
nullalgorithm parameter to infer from COSE headers)- Verifies the signature using the COSE library's
validate()method
28-46: The implementation correctly follows RFC 8615 standards for well-known resource resolution. UsingURI.resolve("/.well-known/jwks.json")with an absolute path is intentional—the JWKS endpoint should be at the domain root regardless of the issuer's path structure. For example,https://api.collab.mosip.net/tenant1correctly resolves tohttps://api.collab.mosip.net/.well-known/jwks.json, which is the expected behavior.Regarding the
URISyntaxExceptionat line 40: while an explicit catch with a more descriptive error message would be a minor improvement, the exception is already handled by the outer try-catch block at line 90 and converted toUnknownException, which is acceptable error handling.Likely an incorrect or invalid review comment.
…raction method Signed-off-by: jaswanthkumarpolisetty <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/keyResolver/Utils.kt (1)
133-157: Fix error message + reuse decoder in EC JWK parsing.
Line 140 throws"Missing 'x'"whenyis missing, and you can reuse the sharedbase64Decoder.Proposed fix
@@ val yBase64 = jwk["y"]?.toString() - ?: throw PublicKeyResolutionFailedException("Missing 'x'") - val xBytes = Base64Decoder().decodeFromBase64Url(xBase64) - val yBytes = Base64Decoder().decodeFromBase64Url(yBase64) + ?: throw PublicKeyResolutionFailedException("Missing 'y'") + val xBytes = base64Decoder.decodeFromBase64Url(xBase64) + val yBytes = base64Decoder.decodeFromBase64Url(yBase64)
🤖 Fix all issues with AI agents
In
@vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt:
- Around line 48-62: The extractKid function converts KID byte arrays to String
using the platform default charset; change both occurrences where
protected[KID].GetByteString() and unprotected[KID].GetByteString() are wrapped
with String(...) to explicitly specify a charset (e.g., StandardCharsets.UTF_8)
to ensure deterministic KID values for matching against JWKS; update imports if
needed and keep the rest of the logic in extractKid, KID, coseObj, protected and
unprotected unchanged.
- Around line 95-109: The verify function calls extractClaims(coseObj) before
validateCoseStructure(coseObj), so malformed COSE objects can cause
extractClaims to throw unrelated exceptions; move the
validateCoseStructure(coseObj) call to immediately after decoding (before
extractClaims), then call extractClaims(coseObj), extractKid(coseObj),
extractIssuer(claims), resolve the public key via
PublicKeyResolverFactory().get(issuer, kid) and finally call
verifySignature(coseBytes, publicKey) so signature errors are properly
classified as SignatureVerificationException.
In
@vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/keyResolver/types/jwks/JwksPublicKeyResolver.kt:
- Around line 15-35: The resolver currently reads kty from the JWK and passes
raw values like "EC" into getPublicKeyFromJWK, which expects internal constants
(e.g., JWK_KEY_TYPE_EC); fix resolve in JwksPublicKeyResolver.kt by normalizing
the extracted kty before calling getPublicKeyFromJWK (e.g., if kty == "EC" map
to JWK_KEY_TYPE_EC, likewise map "RSA" to JWK_KEY_TYPE_RSA if applicable), then
call getPublicKeyFromJWK(jwk, normalizedKeyType) so EC keys from JWKS resolve
correctly.
- Around line 23-27: The current JwksPublicKeyResolver selects the first key
when keyId is null which is ambiguous; change the resolution logic so that in
the block that computes "val jwk = keys.filterIsInstance<Map<String,
Any>>().firstOrNull { keyId == null || it["kid"] == keyId } ?: throw
PublicKeyNotFoundException(...)" you instead detect the keyId==null case: if
keyId is null and there are multiple candidate keys in "keys" throw a
PublicKeyNotFoundException indicating ambiguous/missing kid, otherwise proceed
to select the single candidate; ensure the exception message clearly states
ambiguity when multiple keys exist and still falls back to selecting the sole
key when exactly one candidate is present.
In
@vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/keyResolver/Utils.kt:
- Around line 79-85: getPublicKeyFromJWK currently doesn't handle the raw JWK
kty value "EC", which causes EC JWKS resolution to fail; update the when branch
in getPublicKeyFromJWK to treat the string "EC" the same as
ES256K_KEY_TYPE_2019, ES256_KEY_TYPE_2019 and JWK_KEY_TYPE_EC so that
getECPublicKey(jwk) is called for both the constant values and the literal "EC";
ensure similar acceptance for other raw kty values if needed.
🧹 Nitpick comments (8)
vc-verifier/kotlin/vcverifier/src/test/java/io/mosip/vercred/vcverifier/credentialverifier/validator/CwtValidatorTest.kt (4)
13-15: Remove the empty setup method.The
setup()method contains no initialization logic and can be safely removed to reduce clutter.🧹 Proposed cleanup
- @BeforeEach - fun setup() { - } -
34-58: Add a comment explaining the manual CBOR encoding.The
cborMapWithDuplicateKeyfunction manually constructs CBOR bytes to create a malformed map with duplicate keys. A comment explaining why manual byte construction is necessary (to bypass the library's duplicate key prevention) would improve maintainability.📝 Suggested documentation
private fun cborMapWithDuplicateKey( key: Int, firstValue: CBORObject, secondValue: CBORObject ): ByteArray { - // CBOR map with 2 pairs, same key repeated + // Manually constructs a malformed CBOR map with duplicate keys. + // The CBORObject library prevents duplicate keys in maps, so we + // manually encode the bytes to bypass this validation and test + // the validator's handling of malformed CBOR. val map = CBORObject.NewArray().apply {
61-209: Consider extracting CBOR key constants for improved readability.The tests use hard-coded integers for CBOR keys (1 for
alg, 4 forexp, 5 fornbf, 6 foriat). Defining these as named constants would improve readability and maintainability.💡 Example constants
companion object { private const val CBOR_KEY_ALG = 1 private const val CBOR_KEY_EXP = 4 private const val CBOR_KEY_NBF = 5 private const val CBOR_KEY_IAT = 6 }Then use them in tests:
val claims = CBORObject.NewMap().apply { Add(CBOR_KEY_EXP, past) }
77-77: Consider using the importedbytesToHexfunction.The file imports
bytesToHexfromio.ipfs.multibase.Base16(line 4) but manually converts bytes to hex using.joinToString("") { "%02x".format(it) }in multiple places. Using the imported function would improve consistency and reduce code duplication.♻️ Example usage
- val hex = coseArray.EncodeToBytes().joinToString("") { "%02x".format(it) } + val hex = bytesToHex(coseArray.EncodeToBytes())Also applies to: 109-109, 129-129, 145-145, 203-203
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/keyResolver/types/jwks/JwksPublicKeyResolver.kt (1)
32-35: Preserve root cause + log stacktrace (current catch-all loses actionable details).
Right now you always throwPublicKeyNotFoundException("Public key string not found")and log onlytoString().Proposed fix
@@ - } catch (e: Exception) { - logger.severe("Error fetching public key string $e") - throw PublicKeyNotFoundException("Public key string not found") - } + } catch (e: Exception) { + logger.log(java.util.logging.Level.SEVERE, "Error resolving JWKS public key (uri=$uri, kid=$keyId)", e) + throw PublicKeyNotFoundException("JWKS public key resolution failed (uri=$uri, kid=$keyId)") + }vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/keyResolver/PublicKeyResolverFactory.kt (1)
18-25: Make JWKS detection robust: preferURI.scheme+URI.pathover stringstartsWith/endsWith.
This avoids missing cases like.../jwks.json?x=yand avoids accidental matches.Proposed refactor
@@ - val verificationMethodStr = verificationMethod.toString() + val verificationMethodStr = verificationMethod.toString() + val scheme = verificationMethod.scheme?.lowercase() + val path = verificationMethod.path ?: "" return when { verificationMethodStr.startsWith(DID_PREFIX) -> DidPublicKeyResolver().resolve(verificationMethod.toString()) - verificationMethodStr.startsWith(HTTP_PREFIX) && verificationMethodStr.endsWith(JWKS_SUFFIX) -> JwksPublicKeyResolver().resolve(verificationMethod.toString(),keyId) - verificationMethodStr.startsWith(HTTP_PREFIX) -> HttpsPublicKeyResolver().resolve(verificationMethod.toString()) + (scheme == "http" || scheme == "https") && path.endsWith("/$JWKS_SUFFIX") -> + JwksPublicKeyResolver().resolve(verificationMethod.toString(), keyId) + (scheme == "http" || scheme == "https") -> + HttpsPublicKeyResolver().resolve(verificationMethod.toString()) else -> throw PublicKeyTypeNotSupportedException("Public Key type is not supported") }vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/keyResolver/Utils.kt (1)
115-129: Prefer explicit checks overrequire { throw ... }(current pattern is surprising).
This is harder to read and can confuse tooling; anif (...) throw PublicKeyResolutionFailedException(...)is clearer.vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt (1)
110-118: Keep the root cause when wrapping intoUnknownException.
Right now the stacktrace is lost; debugging verification failures will be much harder.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/validator/CwtValidator.ktvc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.ktvc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/keyResolver/PublicKeyResolverFactory.ktvc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/keyResolver/Utils.ktvc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/keyResolver/types/jwks/JwksPublicKeyResolver.ktvc-verifier/kotlin/vcverifier/src/test/java/io/mosip/vercred/vcverifier/credentialverifier/validator/CwtValidatorTest.ktvc-verifier/kotlin/vcverifier/src/test/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifierTest.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/validator/CwtValidator.kt
- vc-verifier/kotlin/vcverifier/src/test/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifierTest.kt
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-07T10:57:37.322Z
Learnt from: jaswanthkumartw
Repo: inji/vc-verifier PR: 216
File: vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt:95-123
Timestamp: 2026-01-07T10:57:37.322Z
Learning: In the CwtVerifier class, the `PublicKeyResolverFactory().get(issuer)` method throws `PublicKeyNotFoundException` when a key cannot be resolved, rather than returning null.
Applied to files:
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/keyResolver/PublicKeyResolverFactory.ktvc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.ktvc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/keyResolver/types/jwks/JwksPublicKeyResolver.ktvc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/keyResolver/Utils.kt
🧬 Code graph analysis (1)
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/keyResolver/types/jwks/JwksPublicKeyResolver.kt (1)
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/keyResolver/Utils.kt (2)
getPublicKeyFromJWK(79-86)getPublicKeyFromJWK(89-100)
...erifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt
Show resolved
Hide resolved
...erifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt
Show resolved
Hide resolved
...er/src/main/java/io/mosip/vercred/vcverifier/keyResolver/types/jwks/JwksPublicKeyResolver.kt
Show resolved
Hide resolved
...er/src/main/java/io/mosip/vercred/vcverifier/keyResolver/types/jwks/JwksPublicKeyResolver.kt
Show resolved
Hide resolved
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/keyResolver/Utils.kt
Show resolved
Hide resolved
Signed-off-by: jaswanthkumarpolisetty <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
@vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt:
- Around line 22-26: The validateCoseStructure method only checks array size;
add element-level type checks mirroring CwtValidator.validateCoseStructure:
verify coseObj[0] is a ByteString (protected header), coseObj[1] is a Map
(unprotected header), coseObj[2] is a ByteString (payload) and coseObj[3] is a
ByteString (signature), and throw SignatureVerificationException with clear
messages identifying the offending index and expected type (e.g., "Invalid
COSE_Sign1: protected header must be ByteString at index 0") so subsequent calls
like GetByteString() and ContainsKey(KID) fail fast with descriptive errors.
🧹 Nitpick comments (3)
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt (3)
48-65: Consider charset handling for kid extraction.Lines 55 and 61 convert the kid byte string to a String using the default charset (UTF-8). While kid values are typically ASCII or UTF-8 in practice, the COSE specification defines kid as a byte string (bstr) without mandating a specific encoding. If a kid contains non-UTF-8 bytes, the conversion could produce incorrect results.
Consider explicitly using UTF-8 charset or adding error handling for charset conversion failures, though this is an edge case unlikely to occur with real credentials.
♻️ Optional: Explicit charset handling
- return String(protected[KID].GetByteString()) + return String(protected[KID].GetByteString(), Charsets.UTF_8)- return String(unprotected[KID].GetByteString()) + return String(unprotected[KID].GetByteString(), Charsets.UTF_8)
105-105: Usevalinstead ofvarfor immutable variable.The
issuervariable is never reassigned, so it should be declared asvalrather thanvar.♻️ Proposed fix
- var issuer = extractIssuer(claims) + val issuer = extractIssuer(claims)
109-117: Preserve the original exception as a cause when wrapping.The
elsebranch at line 114 wraps unexpected exceptions inUnknownExceptionbut only includes the message, losing the original exception's stack trace and details. This makes debugging more difficult when unexpected errors occur during verification.Pass the original exception as the cause parameter to preserve the full error context.
♻️ Proposed fix to preserve exception cause
else -> throw UnknownException( - "Error while verifying CWT credential: ${exception.message}" + "Error while verifying CWT credential: ${exception.message}", + exception )
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.ktvc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/keyResolver/types/jwks/JwksPublicKeyResolver.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/keyResolver/types/jwks/JwksPublicKeyResolver.kt
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-01-07T10:57:37.322Z
Learnt from: jaswanthkumartw
Repo: inji/vc-verifier PR: 216
File: vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt:95-123
Timestamp: 2026-01-07T10:57:37.322Z
Learning: In the CwtVerifier class, the `PublicKeyResolverFactory().get(issuer)` method throws `PublicKeyNotFoundException` when a key cannot be resolved, rather than returning null.
Applied to files:
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt
📚 Learning: 2026-01-09T05:57:50.933Z
Learnt from: jaswanthkumartw
Repo: inji/vc-verifier PR: 216
File: vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/keyResolver/Utils.kt:79-85
Timestamp: 2026-01-09T05:57:50.933Z
Learning: In the file vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/keyResolver/Utils.kt, the constant JWK_KEY_TYPE_EC is defined as "EC", so when this constant is used in pattern matching, it already covers the raw "EC" string value.
Applied to files:
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt
🧬 Code graph analysis (1)
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt (2)
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/utils/Util.kt (1)
hexToBytes(149-156)vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/validator/CwtValidator.kt (1)
validateCoseStructure(63-110)
🪛 detekt (1.23.8)
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt
[warning] 109-109: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🔇 Additional comments (3)
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt (3)
28-46: LGTM!The issuer extraction and URI resolution logic is correct. Using
URI.resolve("/.well-known/jwks.json")properly replaces the path component for http/https issuers to construct the JWKS endpoint URL, while DID issuers are returned as-is for DID resolution.
67-76: LGTM!The claims extraction properly decodes the payload from the COSE structure and validates that it's a CBOR Map, ensuring subsequent claim access operations will succeed.
78-93: LGTM!The signature verification correctly uses the se.digg.cose library to validate the COSE_Sign1 signature against the resolved public key, with appropriate error handling.
...erifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt
Show resolved
Hide resolved
Signed-off-by: jaswanthkumarpolisetty <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
@vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt:
- Around line 48-71: The extractKid function calls GetByteString() on header
values without validating their CBOR type, which can throw if the value is not a
ByteString; update extractKid to check that protected[KID].Type ==
CBORType.ByteString before calling GetByteString() and returning the UTF-8
String, and do the same for unprotected[KID], otherwise skip and continue to the
next check or return null; follow the defensive pattern used in
extractIssuer()/extractClaims() so only ByteString-typed KID values are
converted.
🧹 Nitpick comments (2)
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt (2)
111-111: Usevalinstead ofvarfor immutability.The
issuervariable is never reassigned, so it should be declared withvalto indicate immutability.♻️ Proposed fix
- var issuer = extractIssuer(claims) + val issuer = extractIssuer(claims)
115-124: Preserve the original exception cause chain.The exception handling wraps non-specific exceptions in
UnknownException, but only includes the message. This loses the original exception's stack trace and cause chain, making debugging significantly harder.♻️ Proposed fix to preserve exception cause
else -> throw UnknownException( - "Error while verifying CWT credential: ${exception.message}" + "Error while verifying CWT credential: ${exception.message}", + exception )Note: Verify that the
UnknownExceptionconstructor accepts acauseparameter. If not, consider updating the exception class to support this pattern.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: jaswanthkumartw
Repo: inji/vc-verifier PR: 216
File: vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt:22-26
Timestamp: 2026-01-09T06:10:16.923Z
Learning: In the CWT verification flow for the CwtVerifier and CwtValidator classes, validation is performed before verification. CwtValidator.validate() checks structure and claims first, then CwtVerifier.verify() performs signature verification. The verifier does not need to duplicate detailed structure validation since it's already handled by the validator.
📚 Learning: 2026-01-09T06:10:16.923Z
Learnt from: jaswanthkumartw
Repo: inji/vc-verifier PR: 216
File: vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt:22-26
Timestamp: 2026-01-09T06:10:16.923Z
Learning: In the CWT verification flow for the CwtVerifier and CwtValidator classes, validation is performed before verification. CwtValidator.validate() checks structure and claims first, then CwtVerifier.verify() performs signature verification. The verifier does not need to duplicate detailed structure validation since it's already handled by the validator.
Applied to files:
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt
📚 Learning: 2026-01-07T10:57:37.322Z
Learnt from: jaswanthkumartw
Repo: inji/vc-verifier PR: 216
File: vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt:95-123
Timestamp: 2026-01-07T10:57:37.322Z
Learning: In the CwtVerifier class, the `PublicKeyResolverFactory().get(issuer)` method throws `PublicKeyNotFoundException` when a key cannot be resolved, rather than returning null.
Applied to files:
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt
📚 Learning: 2026-01-09T05:57:50.933Z
Learnt from: jaswanthkumartw
Repo: inji/vc-verifier PR: 216
File: vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/keyResolver/Utils.kt:79-85
Timestamp: 2026-01-09T05:57:50.933Z
Learning: In the file vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/keyResolver/Utils.kt, the constant JWK_KEY_TYPE_EC is defined as "EC", so when this constant is used in pattern matching, it already covers the raw "EC" string value.
Applied to files:
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt
🧬 Code graph analysis (1)
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt (2)
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/utils/Util.kt (1)
hexToBytes(149-156)vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/validator/CwtValidator.kt (1)
validateCoseStructure(63-110)
🪛 detekt (1.23.8)
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt
[warning] 115-115: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🔇 Additional comments (2)
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt (2)
22-26: Minimal structure validation is appropriate here.Based on learnings, CwtValidator.validate() performs detailed structure checks before CwtVerifier.verify() is called, so this minimal validation (Array with 4 elements) is sufficient for the verifier's signature verification focus.
28-46: No issues found with URI resolution logic.The implementation correctly uses
URI.resolve("/.well-known/jwks.json")with an absolute path reference (leading/), which follows RFC 3986 semantics and ensures the JWKS endpoint is always resolved to the root path regardless of the issuer URL format (with/without trailing slashes or paths). Existing tests confirm this behavior works as intended for JWKS discovery.
...erifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt
Show resolved
Hide resolved
Signed-off-by: jaswanthkumarpolisetty <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt (2)
108-108: Usevalinstead ofvar.The
issuervariable is never reassigned, so it should be declared asvalfor immutability.♻️ Proposed fix
- var issuer = extractIssuer(claims) + val issuer = extractIssuer(claims)
109-109: Consider caching the PublicKeyResolverFactory instance.The factory is stateless and has negligible instantiation cost, but caching it across verification calls could improve efficiency slightly. If instantiated frequently, consider making it a class-level property.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: jaswanthkumartw
Repo: inji/vc-verifier PR: 216
File: vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt:22-26
Timestamp: 2026-01-09T06:10:16.923Z
Learning: In the CWT verification flow for the CwtVerifier and CwtValidator classes, validation is performed before verification. CwtValidator.validate() checks structure and claims first, then CwtVerifier.verify() performs signature verification. The verifier does not need to duplicate detailed structure validation since it's already handled by the validator.
📚 Learning: 2026-01-09T06:10:16.923Z
Learnt from: jaswanthkumartw
Repo: inji/vc-verifier PR: 216
File: vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt:22-26
Timestamp: 2026-01-09T06:10:16.923Z
Learning: In the CWT verification flow for the CwtVerifier and CwtValidator classes, validation is performed before verification. CwtValidator.validate() checks structure and claims first, then CwtVerifier.verify() performs signature verification. The verifier does not need to duplicate detailed structure validation since it's already handled by the validator.
Applied to files:
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt
📚 Learning: 2026-01-07T10:57:37.322Z
Learnt from: jaswanthkumartw
Repo: inji/vc-verifier PR: 216
File: vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt:95-123
Timestamp: 2026-01-07T10:57:37.322Z
Learning: In the CwtVerifier class, the `PublicKeyResolverFactory().get(issuer)` method throws `PublicKeyNotFoundException` when a key cannot be resolved, rather than returning null.
Applied to files:
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt
📚 Learning: 2026-01-09T05:57:50.933Z
Learnt from: jaswanthkumartw
Repo: inji/vc-verifier PR: 216
File: vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/keyResolver/Utils.kt:79-85
Timestamp: 2026-01-09T05:57:50.933Z
Learning: In the file vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/keyResolver/Utils.kt, the constant JWK_KEY_TYPE_EC is defined as "EC", so when this constant is used in pattern matching, it already covers the raw "EC" string value.
Applied to files:
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt
🧬 Code graph analysis (1)
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt (2)
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/utils/Util.kt (1)
hexToBytes(149-156)vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/validator/CwtValidator.kt (1)
validateCoseStructure(63-110)
🪛 detekt (1.23.8)
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt
[warning] 112-112: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🔇 Additional comments (6)
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt (6)
22-26: Lightweight structure validation as a defensive check.This minimal validation checks the basic COSE_Sign1 structure. Based on learnings, detailed structure validation is already handled by
CwtValidator.validate()before this verifier runs. This defensive check is acceptable but optional given the validation flow.
28-46: LGTM! Proper issuer extraction and resolution.The method correctly extracts the issuer claim, validates its type, and appropriately handles different URI schemes (DID vs. HTTP/HTTPS with JWKS resolution).
48-67: LGTM! Correct key ID extraction.The method properly extracts the key ID from COSE headers, checking protected headers first per COSE best practices, then falling back to unprotected headers.
70-79: LGTM! Proper claims extraction.The method correctly extracts and validates the CWT claims structure from the COSE_Sign1 payload.
81-96: LGTM! Proper signature verification.The method correctly decodes the COSE object, constructs the COSE key, and validates the signature using the library's verification method. The flow of returning true on success or throwing an exception is appropriate.
112-121: Exception handling is correct; static analysis warning is a false positive.The exception handling properly re-throws
PublicKeyNotFoundExceptionandSignatureVerificationException, and wraps other exceptions inUnknownException. The static analysis tool incorrectly flags line 112 as swallowing exceptions, but the code explicitly re-throws known exceptions and wraps unknown ones—no exceptions are actually swallowed.
|
@coderabbitai review this PR |
|
@mayuradesh: I'll review PR #216 which adds CWT (CBOR Web Token) format support for verifiable credentials. This includes new validation and verification classes, JWKS-based key resolution, and related test coverage. 🧠 Learnings used✅ Actions performedReview triggered.
|
Signed-off-by: jaswanthkumarpolisetty <[email protected]>
2 participants
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.