INJIVER-1523 Release 1.7.x LSH Code Review#230
Conversation
WalkthroughAdds CWT (COSE Web Token) VC support, refactors credential status APIs from list→map, extends presentation verification with V2 and credential-status-aware results, introduces JWKS public-key resolution and CBOR utilities, updates publish metadata to io.inji and CI workflow inputs, and adds docs, tests, and fixtures. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant CwtValidator
participant CwtVerifier
participant PublicKeyResolverFactory
participant IssuerResolver
participant CoseVerifier
Client->>CwtValidator: validate(hexCredential)
CwtValidator->>CwtValidator: hexToBytes → parse CBOR/COSE → validate headers/claims/dates
CwtValidator-->>Client: ValidationStatus
Client->>CwtVerifier: verify(hexCredential)
CwtVerifier->>CwtVerifier: parse COSE_Sign1 → extract issuer & kid
CwtVerifier->>PublicKeyResolverFactory: get(issuerURI, kid)
PublicKeyResolverFactory->>IssuerResolver: resolve(issuerURI, kid) (DID or JWKS)
IssuerResolver-->>PublicKeyResolverFactory: PublicKey
PublicKeyResolverFactory-->>CwtVerifier: PublicKey
CwtVerifier->>CoseVerifier: verify(signature, publicKey)
CoseVerifier-->>CwtVerifier: boolean
CwtVerifier-->>Client: boolean
sequenceDiagram
participant Client
participant CredentialsVerifier
participant LdpStatusChecker
participant StatusListResolver
participant LocalDocumentLoader
Client->>CredentialsVerifier: getCredentialStatus(credential, format, purposes)
CredentialsVerifier->>LdpStatusChecker: getStatuses(credential, purposes)
LdpStatusChecker->>LdpStatusChecker: parse credentialStatus (List/Map) → normalize
LdpStatusChecker->>StatusListResolver: fetch statusListCredential URL
StatusListResolver->>LocalDocumentLoader: load & verify status list
LdpStatusChecker->>LdpStatusChecker: compute per-purpose CredentialStatusResult map
LdpStatusChecker-->>CredentialsVerifier: Map<String, CredentialStatusResult>
CredentialsVerifier-->>Client: Map<String, CredentialStatusResult>
sequenceDiagram
participant Client
participant PresentationVerifier
participant VCVerifier
participant CredentialStatusChecker
participant CredentialVerifier
Client->>PresentationVerifier: verifyAndGetCredentialStatusV2(presentation, purposes)
PresentationVerifier->>PresentationVerifier: parse VP & verify VP proof
PresentationVerifier->>VCVerifier: verify each VC
VCVerifier-->>PresentationVerifier: VCResultWithCredentialStatusV2
PresentationVerifier->>CredentialStatusChecker: request per-VC status checks
CredentialStatusChecker->>CredentialVerifier: getCredentialStatus(vc, format, purposes)
CredentialVerifier-->>CredentialStatusChecker: Map<String, CredentialStatusResult>
CredentialStatusChecker-->>PresentationVerifier: per-VC statuses
PresentationVerifier-->>Client: PresentationResultWithCredentialStatusV2
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~75 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
🧪 Generate unit tests (beta)
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 |
|
@coderabbitai review this PR |
|
✅ Actions performedReview triggered.
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
vc-verifier/kotlin/gradle/libs.versions.toml (1)
7-7:⚠️ Potential issue | 🟠 MajorUpgrade BouncyCastle to version 1.78 or later to address multiple known security vulnerabilities.
BouncyCastle 1.70 has five documented security advisories: CVE-2023-33201 (LDAP injection), CVE-2023-33202 (ASN.1 DoS), CVE-2024-30172 (Ed25519 verification DoS), CVE-2024-29857 (EC certificate DoS), and CVE-2025-8885 (ASN.1 OID DoS). Version 1.78 or later patches all of these issues.
vc-verifier/kotlin/vcverifier/src/test/java/io/mosip/vercred/vcverifier/credentialverifier/statusChecker/StatusListRevocationCheckerTest.kt (1)
268-295:⚠️ Potential issue | 🟡 MinorPotential resource leak: second MockWebServer not shut down.
In this test,
prepareVCis called twice, creating twoMockWebServerinstances. The second server (fromprepareVCZero) is never shut down because its reference is discarded with_.🐛 Proposed fix
- val (replacedVCZero, _) = prepareVC( + val (replacedVCZero, serverZero) = prepareVC( statusListJson = statusListJsonZero ) // ... assertions ... server.shutdown() + serverZero.shutdown()vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/keyResolver/Utils.kt (1)
133-158:⚠️ Potential issue | 🟡 MinorTypo in error message: "Missing 'x'" should be "Missing 'y'".
Line 140 throws an exception with the message
"Missing 'x'"when theycoordinate is missing.🐛 Proposed fix
val yBase64 = jwk["y"]?.toString() - ?: throw PublicKeyResolutionFailedException("Missing 'x'") + ?: throw PublicKeyResolutionFailedException("Missing 'y'")
🤖 Fix all issues with AI agents
In `@vc-verifier/kotlin/vcverifier/build.gradle.kts`:
- Around line 71-72: Review and resolve redundant CBOR dependencies: run Gradle
dependency reports (./gradlew dependencies and dependencyInsight) to see which
artifacts bring in co.nstant.in:cbor, com.authlete:cbor, se.digg.cose:cose-lib
and any upokecenter transitive jars; verify CwtVerifier.kt references
(CBORDecoder, CBORTaggedItem) rely on com.authlete:cbor and CwtValidatorTest.kt
uses com.upokecenter.cbor.CBORObject, then remove libs.cbor (co.nstant.in:cbor)
if not directly used, or replace/align versions to avoid conflicts and ensure
cose-lib is only kept if its transitive CBOR is required; update
build.gradle.kts to keep the minimal set and re-run tests to confirm no
breakage.
In
`@vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/statusChecker/LdpStatusChecker.kt`:
- Around line 145-150: The catch in LdpStatusChecker around the sendHTTPRequest
call rethrows a new StatusCheckException without preserving the original
exception, losing the stack trace; modify the catch to pass the caught Exception
e as the cause to the StatusCheckException constructor (or, if
StatusCheckException has no cause parameter, append e.toString() to the message)
so the original stack trace is preserved when throwing from
LdpStatusChecker::check (or the method containing this try/catch).
- Around line 248-262: The code treats STATUS_MESSAGE as a Map but per W3C
Bitstring Status List it must be an array of objects; update the
LdpStatusChecker logic (around isStatusMessageAvailable, STATUS_MESSAGE,
statusSize, purpose, logger) to cast entry[STATUS_MESSAGE] to a List<*> instead
of Map<*, *>, verify the list length equals expectedStatusCount
(1.shl(statusSize)), and validate each list element is a Map containing a
hex-string "status" and a string "message"; on any mismatch throw
StatusCheckException with STATUS_VERIFICATION_ERROR (same semantics as existing
errors) and keep the existing logging of the status message content.
In
`@vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt`:
- Around line 122-146: The catch block in CwtVerifier.verify currently wraps all
unknown exceptions as UnknownException, which hides UnsupportedDidUrl; update
the catch to rethrow UnsupportedDidUrl as-is (add a case alongside
PublicKeyNotFoundException and SignatureVerificationException) so
UnsupportedDidUrl propagates unchanged from extractIssuer/external resolution
logic in verify (refer to the verify function in CwtVerifier and the
UnsupportedDidUrl exception type).
In
`@vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/keyResolver/types/jwks/JwksPublicKeyResolver.kt`:
- Around line 32-35: The catch-all in JwksPublicKeyResolver.kt currently wraps
every Exception into a new generic PublicKeyNotFoundException and loses the
original messages; change the error handling to preserve specific exceptions by
first catching and rethrowing PublicKeyNotFoundException (or include its
message/cause), and only wrap unexpected exceptions with a new
PublicKeyNotFoundException that includes the original exception as the cause
and/or incorporates e.message in the log and thrown exception; update the
logger.severe call to include the exception details when rethrowing so callers
can see the original context.
In
`@vc-verifier/kotlin/vcverifier/src/test/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/LdpVerifierTest.kt`:
- Around line 26-31: The test setup currently mocks the NetworkManagerClient
class (mockkObject(NetworkManagerClient)) but the HTTP stubs target the
companion method NetworkManagerClient.Companion.sendHTTPRequest, so stubs never
apply; change the setup to mock the companion object instead
(mockkObject(NetworkManagerClient.Companion)) so that
NetworkManagerClient.Companion.sendHTTPRequest can be stubbed by
mockHttpResponse; keep the rest of setup (Util.documentLoader and
loadMockPublicKeys) unchanged.
In
`@vc-verifier/kotlin/vcverifier/src/test/resources/vp/public_key/didIdentityServiceKey.json`:
- Around line 6-9: The test fixture contains an email-like PII value in the JSON
"alsoKnownAs" array ("test@gmail.com"); replace that entry with a
non-identifying placeholder (e.g., "user@example.com") or a UUID-only alias so
the "alsoKnownAs" field contains no real email addresses—update the
"alsoKnownAs" array element in the test resource accordingly.
In
`@vc-verifier/kotlin/vcverifier/src/test/resources/vp/public_key/didMockKey.json`:
- Around line 2-4: The assertionMethod and authentication arrays currently
reference the bare DID string instead of the verificationMethod entries; update
those entries to use the exact verificationMethod id values (the DID plus
fragment) that appear in the document's verificationMethod array so each
reference resolves to a verificationMethod id (e.g., replace
"did:web:mosip.github.io:inji-config:qa-inji1" with the corresponding
verificationMethod ids present in the verificationMethod array), ensuring
assertionMethod and authentication reference existing verificationMethod ids.
🧹 Nitpick comments (20)
vc-verifier/kotlin/vcverifier/src/test/resources/ldp_vc/mockDid.json (2)
8-33: Remove@contextfrom individual verification methods.The DID Core specification requires
@contextto appear only at the document level, not within individualverificationMethodentries (lines 13, 25, 32). Having@contextfields embedded in verification methods is non-standard and may cause issues with spec-compliant DID resolvers.📋 Consolidate contexts at document level
Remove the
@contextfields from each verification method and ensure all required contexts are in the document-level@contextarray:{ "controller": "did:web:mosip.github.io:inji-config:qa-inji1:mock", "publicKeyPem": "-----BEGIN PUBLIC KEY-----\nMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEA65X0aE2ufNhM3ry40/ij\nAqM06rbaaGjO+4JQQTl9tdR5oFe6Jd50Rf/sO6RNes9k3a1tCZ6oadEgwQO6J5YU\n5kKXOTO1KQ9ifCTlIjkNlvEz06Z0uBtjKtaPplaWaVna6H0fFht0fsD2Z6ya0hCx\n2z1X7mmbC4kD9pig6N28v40n7Kjsr01Kw026zNag/hsXO/hK19YAjLgo6Q1uGY7k\ngFwcVhpGOyzVK4BKRtkhPi3RUyP5n2MdrQ5kE34p5zoyrxU0C5trPz3iKKN4rrB6\neIGgRU0EVVRWSQAEV/7ZO3WWhWbpolivNHWQddXfDUScUnZo1dgRDIn7guXaE44P\nOQIDAQAB\n-----END PUBLIC KEY-----", "id": "did:web:mosip.github.io:inji-config:qa-inji1:mock#QizhlYu5pcVa8l_fmElYzeLukKPShmkd0vIfF1Y7Oco", - "type": "RsaVerificationKey2018", - "@context": "https://w3id.org/security/v1" + "type": "RsaVerificationKey2018" }, { "controller": "did:web:mosip.github.io:inji-config:qa-inji1:mock", "publicKeyJwk": { "kty": "EC", "crv": "secp256k1", "x": "o4ODZclbHi4FErGGaCzuQwAcnXiJnCB-oKl0D2fsINU", "y": "Tel-8PGxeMn4Y8n41uhgCDOWPYZLn6RYw6kfRtE8eGQ" }, "id": "did:web:mosip.github.io:inji-config:qa-inji1:mock#ojsIq-D4X3UvTBdMQwRudb8cR-BOhoXtdM6cj74hKGM", - "type": "EcdsaSecp256k1VerificationKey2019", - "@context": "https://w3id.org/security/v1" + "type": "EcdsaSecp256k1VerificationKey2019" }, { "publicKeyMultibase": "z6Mki13hAgnc8jDw86MnBhNQ12C4DuMs5kjmpg9orskHTd45", "controller": "did:web:mosip.github.io:inji-config:qa-inji1:mock", "id": "did:web:mosip.github.io:inji-config:qa-inji1:mock#kMVyZTvx8G0h1YZnUW4OJtr2LRVZUuGl8pQuJl3ymXI", - "type": "Ed25519VerificationKey2020", - "@context": "https://w3id.org/security/suites/ed25519-2020/v1" + "type": "Ed25519VerificationKey2020" }And update the document-level
@contextto include all necessary contexts:"@context": [ - "https://www.w3.org/ns/did/v1" + "https://www.w3.org/ns/did/v1", + "https://w3id.org/security/v1", + "https://w3id.org/security/suites/ed25519-2020/v1" ],
42-42: Add newline at end of file.The file ends without a trailing newline, which may cause issues with certain tools and version control systems.
vc-verifier/kotlin/vcverifier/src/test/resources/ldp_vc/public_key/httpsMockKey.json (2)
6-6: Non-standard line endings in PEM key.The
publicKeyPemfield uses\r\n(Windows-style) line endings instead of the more standard\nfor JSON strings. While most PEM parsers handle both formats,\nis more idiomatic in JSON. Consider using\nunless\r\nis intentionally testing Windows line-ending compatibility.📝 Optional fix to use standard line endings
- "publicKeyPem": "-----BEGIN PUBLIC KEY-----\r\nMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAxo2dOCtoTV9pvHlXbhL7\r\nkNHBvTZdQM4QW6wYSGqcIaSlQovQPEz8qbSBO17rCW+ZuyRPUquxUCJp6/CCkPuJ\r\n5XVk0uC6qjkK6Xcim+2Sb/l5BT3q1obErkK1uNXymK1dMbYpN1IfWWodVtd3ZX8k\r\nUCQvOYqq1OPrCC8C1As+ZaQoP/Cbi4+ucvI07engmlnteECMCrOGSDfNCn11fw8a\r\nY+3cSwU9wcUYK9vrq7ST+M7YuPjAMZr9TyRw4VYPUZJpsJtkQPPufpSL5Lz3FEbg\r\nBZNxE5Wgk05Pr+zZVQ2kvwCtUAUZWJbQt6m8lYuPr4fJRjrPtZDeA85jGBKQXG17\r\neQIDAQAB\r\n-----END PUBLIC KEY-----\r\n" + "publicKeyPem": "-----BEGIN PUBLIC KEY-----\nMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAxo2dOCtoTV9pvHlXbhL7\nkNHBvTZdQM4QW6wYSGqcIaSlQovQPEz8qbSBO17rCW+ZuyRPUquxUCJp6/CCkPuJ\n5XVk0uC6qjkK6Xcim+2Sb/l5BT3q1obErkK1uNXymK1dMbYpN1IfWWodVtd3ZX8k\nUCQvOYqq1OPrCC8C1As+ZaQoP/Cbi4+ucvI07engmlnteECMCrOGSDfNCn11fw8a\nY+3cSwU9wcUYK9vrq7ST+M7YuPjAMZr9TyRw4VYPUZJpsJtkQPPufpSL5Lz3FEbg\nBZNxE5Wgk05Pr+zZVQ2kvwCtUAUZWJbQt6m8lYuPr4fJRjrPtZDeA85jGBKQXG17\neQIDAQAB\n-----END PUBLIC KEY-----\n"
4-5: Consider using organization-controlled or standard mock URLs for test stability.The
idandcontrollerfields reference personal GitHub pages (vharsh.github.io). While this pattern is consistent across multiple test fixtures in the suite, using organization-controlled URLs (e.g.,mosip.github.io) or standard mock domains (e.g.,example.test) would reduce dependency on personal domains and improve long-term maintainability. This is particularly relevant if the project has multiple maintainers.vc-verifier/kotlin/vcverifier/src/test/resources/ldp_vc/public_key/httpsMockKey2.json (1)
4-5: Consider using standard test domain patterns.The mock URLs use a personal GitHub domain (
vharsh.github.io). While acceptable for test resources, using standard test domain patterns likeexample.com,test.example, orlocalhostwould make it more obvious that these are test fixtures and align with common testing practices.📝 Suggested alternative using standard test domain
- "id": "https://vharsh.github.io/DID/mock-public-key2.json", - "controller": "https://vharsh.github.io/DID/mock-controller2.json", + "id": "https://example.com/DID/mock-public-key2.json", + "controller": "https://example.com/DID/mock-controller2.json",vc-verifier/kotlin/vcverifier/src/test/resources/cwt_vc/public_key/jwksinvalidECkey.json (1)
1-13: Consider clarifying what makes this key "invalid".The key structure is valid JWKS, and the x/y coordinates are identical to
jwksECkey.json. The "invalid" aspect appears to be the mismatchedkidvalue for signature verification failure tests. Consider either:
- Renaming to
jwksECkeyWrongKid.jsonfor clarity, or- Adding a comment in the test that consumes this resource explaining the test scenario
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/data/Data.kt (1)
57-60: Avoid embedding exception objects in result DTOs if they are serialized.
If these results cross module/API boundaries, carryingStatusCheckExceptioncan leak internal details or fail serialization. Consider mapping to a structured error (code/message) and keep exceptions internal.vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/utils/Util.kt (1)
149-156:hexToBytesdoes not validate hex character validity.The function validates that the input has even length but does not check that all characters are valid hexadecimal digits (0-9, a-f, A-F). If invalid characters are passed,
toInt(16)will throw aNumberFormatException, which may not be the intended error handling behavior.Consider adding validation for hex characters:
🛡️ Proposed fix
fun hexToBytes(hex: String): ByteArray { val cleanHex = hex.replace("\\s".toRegex(), "") - require(cleanHex.length % 2 == 0) { "Invalid hex length" } + require(cleanHex.length % 2 == 0 && cleanHex.matches(Regex("^[0-9a-fA-F]*$"))) { + "Invalid hex string" + } return ByteArray(cleanHex.length / 2) { i -> cleanHex.substring(i * 2, i * 2 + 2).toInt(16).toByte() } }vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/types/CwtVerifiableCredential.kt (1)
14-20: Minor: Remove unnecessary semicolons.Kotlin does not require semicolons at the end of statements. While not harmful, removing them aligns with idiomatic Kotlin style.
✨ Suggested cleanup
override fun verify(credential: String): Boolean { - return CwtVerifier().verify(credential); + return CwtVerifier().verify(credential) } override fun checkStatus(credential: String, statusPurposes: List<String>?): Map<String, CredentialStatusResult> { - return emptyMap(); + return emptyMap() }vc-verifier/kotlin/README.md (1)
696-702: Add blank line before the table.Per the static analysis hint, tables should be surrounded by blank lines for proper Markdown rendering.
📝 Proposed fix
Each `VCResultWithCredentialStatus` contains: + | Field | Type | Description | |--------------------|---------------------------------------|----------------------------------------------------------------------| | `vc` | `String` | The raw VC as it appeared in the input VP | | `status` | `VCVerificationStatus` | Verification status of the VC (`SUCCESS`, `INVALID`, `EXPIRED`) | | `credentialStatus` | `Map<String, CredentialStatusResult>` | Status check results for each purpose (e.g., revocation, suspension) |vc-verifier/kotlin/vcverifier/src/test/java/testutils/TestUtils.kt (1)
13-16: Consider using a typed reference for the JSON deserialization.The unchecked cast
as Map<String, Any>?could be avoided using aTypeReferencefor type-safe deserialization.✨ Suggested improvement
+import com.fasterxml.jackson.core.type.TypeReference + fun mockHttpResponse(url: String, responseJson: String) { every { NetworkManagerClient.Companion.sendHTTPRequest(url, any()) } answers { - mapper.readValue(responseJson, Map::class.java) as Map<String, Any>? + mapper.readValue(responseJson, object : TypeReference<Map<String, Any>>() {}) } }vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifier.kt (1)
24-84: Consider removing duplicate structural/claims validation if a validator already runs first.
If the verification flow always callsCwtValidator.validate()beforeCwtVerifier.verify(), the extra structure/claims checks here add duplicate work.Based on learnings: “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.”
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/keyResolver/types/jwks/JwksPublicKeyResolver.kt (1)
38-38: Remove empty companion object.The empty companion object serves no purpose and can be removed.
♻️ Proposed fix
- - companion object }vc-verifier/kotlin/vcverifier/src/test/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifierTest.kt (1)
47-48: Consider using more representative mock URLs.The ngrok URLs (
https://221f38cc3ffc.ngrok-free.app/...) are ephemeral tunneling addresses. While they're mocked and don't affect test execution, using more representative URLs (e.g.,https://issuer.example.com/...) would better document the expected production URL patterns and avoid confusion for future maintainers.♻️ Suggested improvement
private fun loadMockPublicKeys() { - mockHttpResponse("https://221f38cc3ffc.ngrok-free.app/v1/certify/.well-known/jwks.json", readClasspathFile("cwt_vc/public_key/jwksECkey.json")) - mockHttpResponse("https://9c65dc69fafc.ngrok-free.app/v1/certify/.well-known/jwks.json", readClasspathFile("cwt_vc/public_key/jwksinvalidECkey.json")) + mockHttpResponse("https://valid-issuer.example.com/v1/certify/.well-known/jwks.json", readClasspathFile("cwt_vc/public_key/jwksECkey.json")) + mockHttpResponse("https://invalid-issuer.example.com/v1/certify/.well-known/jwks.json", readClasspathFile("cwt_vc/public_key/jwksinvalidECkey.json")) }Note: This would also require updating the corresponding issuer references in the test CWT hex files.
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/keyResolver/PublicKeyResolverFactory.kt (1)
22-22: Minor: Add space after comma for consistency.♻️ Formatting fix
- verificationMethodStr.startsWith(HTTP_PREFIX) && verificationMethodStr.endsWith(JWKS_SUFFIX) -> JwksPublicKeyResolver().resolve(verificationMethod.toString(),keyId) + verificationMethodStr.startsWith(HTTP_PREFIX) && verificationMethodStr.endsWith(JWKS_SUFFIX) -> JwksPublicKeyResolver().resolve(verificationMethod.toString(), keyId)vc-verifier/kotlin/vcverifier/src/test/java/io/mosip/vercred/vcverifier/credentialverifier/validator/CwtValidatorTest.kt (1)
13-15: Remove empty setup method.The empty
@BeforeEachsetup method serves no purpose and was flagged by static analysis.♻️ Proposed fix
- `@BeforeEach` - fun setup() { - } - private val validator = CwtValidator()vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/statusChecker/LdpStatusChecker.kt (1)
45-45: Remove empty default constructor.The empty default constructor
class LdpStatusChecker()can be simplified toclass LdpStatusCheckeras flagged by static analysis.♻️ Proposed fix
-class LdpStatusChecker() { +class LdpStatusChecker {vc-verifier/kotlin/vcverifier/src/test/java/io/mosip/vercred/vcverifier/credentialverifier/statusChecker/StatusListRevocationCheckerTest.kt (1)
81-94: Consider reordering assertions for clearer failure diagnostics.The assertion at Line 91 (
assertNotNull(results["revocation"])) should come before Line 90's null-safe access check. If the entry is null, Line 90 will pass silently (evaluating tonull == true→ false), but the failure message won't clearly indicate the root cause.♻️ Suggested reorder
assertEquals(1, results.size) + assertNotNull(results["revocation"]) assertTrue(results["revocation"]?.isValid == true) - assertNotNull(results["revocation"])vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/keyResolver/Utils.kt (2)
141-142: Unnecessary instantiation ofBase64Decoderon each call.Lines 141-142 create new
Base64Decoder()instances instead of reusing the existingbase64Decoderdeclared at the module level (Line 44).♻️ Proposed fix
- val xBytes = Base64Decoder().decodeFromBase64Url(xBase64) - val yBytes = Base64Decoder().decodeFromBase64Url(yBase64) + val xBytes = base64Decoder.decodeFromBase64Url(xBase64) + val yBytes = base64Decoder.decodeFromBase64Url(yBase64)
236-247: EC key type dispatch injwkToPublicKeymay not handle all curves correctly.When
KeyType.ECis detected, the function always passesES256K_KEY_TYPE_2019togetPublicKeyFromJWK, which is associated with the secp256k1 curve. However,getECPublicKeyalso supports P-256 (P256) curve. If the JWK contains a P-256 key, this should still work becausegetECPublicKeyreads thecrvfield directly, but the key type constant is semantically misleading.Consider using
JWK_KEY_TYPE_ECinstead for generic EC routing.♻️ Suggested improvement
return when (jwk.keyType) { KeyType.OKP -> getPublicKeyFromJWK(jwkJson, ED25519_KEY_TYPE_2020) - KeyType.EC -> getPublicKeyFromJWK(jwkJson, ES256K_KEY_TYPE_2019) + KeyType.EC -> getPublicKeyFromJWK(jwkJson, JWK_KEY_TYPE_EC) KeyType.RSA -> getPublicKeyFromJWK(jwkJson, RSA_KEY_TYPE)
| } catch (e: Exception) { | ||
| throw StatusCheckException( | ||
| "Retrieval of the status list failed: ${e.message}", | ||
| StatusCheckErrorCode.STATUS_RETRIEVAL_ERROR | ||
| STATUS_RETRIEVAL_ERROR | ||
| ) | ||
| } |
There was a problem hiding this comment.
Original exception is swallowed, losing stack trace context.
The catch block at line 145 catches the exception from sendHTTPRequest but the re-thrown StatusCheckException doesn't include the original exception as a cause. This makes debugging network issues difficult since the original stack trace is lost.
🐛 Proposed fix to preserve exception chain
} catch (e: Exception) {
+ if (e is StatusCheckException) throw e
throw StatusCheckException(
"Retrieval of the status list failed: ${e.message}",
- STATUS_RETRIEVAL_ERROR
+ STATUS_RETRIEVAL_ERROR,
+ e
)
}Note: This assumes StatusCheckException supports a cause parameter. If not, at minimum preserve the original message.
🧰 Tools
🪛 detekt (1.23.8)
[warning] 145-145: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🤖 Prompt for AI Agents
In
`@vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/statusChecker/LdpStatusChecker.kt`
around lines 145 - 150, The catch in LdpStatusChecker around the sendHTTPRequest
call rethrows a new StatusCheckException without preserving the original
exception, losing the stack trace; modify the catch to pass the caught Exception
e as the cause to the StatusCheckException constructor (or, if
StatusCheckException has no cause parameter, append e.toString() to the message)
so the original stack trace is preserved when throwing from
LdpStatusChecker::check (or the method containing this try/catch).
| if (isStatusMessageAvailable(statusSize)) { | ||
| val statusMessage = entry[STATUS_MESSAGE] as? Map<*, *> | ||
| ?: throw StatusCheckException( | ||
| "Missing '$STATUS_MESSAGE' for $STATUS_SIZE=$statusSize", | ||
| StatusCheckErrorCode.STATUS_VERIFICATION_ERROR | ||
| STATUS_VERIFICATION_ERROR | ||
| ) | ||
| logger.info("Status message for purpose '$purpose': $statusMessage") | ||
|
|
||
| val expectedCount = 1.shl(statusSize) | ||
| if (statusMessage.size != expectedCount) { | ||
| val expectedStatusCount = 1.shl(statusSize) | ||
| if (statusMessage.size != expectedStatusCount) { | ||
| throw StatusCheckException( | ||
| "$STATUS_MESSAGE count mismatch. Expected $expectedCount entries for statusSize=$statusSize, found ${statusMessage.size}", | ||
| StatusCheckErrorCode.STATUS_VERIFICATION_ERROR | ||
| "$STATUS_MESSAGE count mismatch. Expected $expectedStatusCount entries for statusSize=$statusSize, found ${statusMessage.size}", | ||
| STATUS_VERIFICATION_ERROR | ||
| ) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Bitstring Status List specification statusMessage field format
💡 Result:
In Bitstring Status List v1.0, credentialStatus.statusMessage (used with type: "BitstringStatusListEntry" and typically statusPurpose: "message") has this format:
- Type: an array of objects. (w3.org)
- When required:
- MUST be present if
statusSize > 1 - MAY be present if
statusSize == 1(w3.org)
- MUST be present if
- Length rule: array length MUST equal the number of possible states, i.e.
2^statusSize(e.g., 2 entries for 1 bit, 4 for 2 bits, 8 for 3 bits, etc.). (w3.org) - Each array element MUST include at least: (w3.org)
status: string, the hex status value (e.g.,"0x0","0x1","0x2")message: string, the human-readable (or dictionary-key) message for that status value
Example (statusSize = 2 → 4 possible values):
"statusSize": 2,
"statusMessage": [
{"status":"0x0","message":"pending_review"},
{"status":"0x1","message":"accepted"},
{"status":"0x2","message":"rejected"}
](w3.org)
If statusMessage is omitted, the default messages are "unset" for 0 and "set" for 1. (w3.org)
Citations:
- 1: https://www.w3.org/TR/vc-bitstring-status-list/?utm_source=openai
- 2: https://www.w3.org/TR/vc-bitstring-status-list/?utm_source=openai
- 3: https://www.w3.org/TR/vc-bitstring-status-list/?utm_source=openai
- 4: https://www.w3.org/TR/vc-bitstring-status-list/
- 5: https://www.w3.org/TR/vc-bitstring-status-list/
- 6: https://www.w3.org/TR/vc-bitstring-status-list/?utm_source=openai
🏁 Script executed:
cat -n vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/statusChecker/LdpStatusChecker.kt | head -280 | tail -100Repository: inji/vc-verifier
Length of output: 4630
🏁 Script executed:
cat -n vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/statusChecker/LdpStatusChecker.kt | head -320 | tail -80Repository: inji/vc-verifier
Length of output: 3766
🏁 Script executed:
cat -n vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/statusChecker/LdpStatusChecker.kt | sed -n '280,350p'Repository: inji/vc-verifier
Length of output: 3428
🏁 Script executed:
rg "computeStatusResult" vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/statusChecker/LdpStatusChecker.kt -B 5 -A 2Repository: inji/vc-verifier
Length of output: 574
🏁 Script executed:
rg "checkStatusEntry" vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/statusChecker/LdpStatusChecker.kt -B 3 -A 3Repository: inji/vc-verifier
Length of output: 830
🏁 Script executed:
rg "credentialStatus" vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/statusChecker/LdpStatusChecker.kt -B 2 -A 2Repository: inji/vc-verifier
Length of output: 2079
🏁 Script executed:
rg "STATUS_MESSAGE" vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/statusChecker/LdpStatusChecker.kt -B 2 -A 2Repository: inji/vc-verifier
Length of output: 1116
🏁 Script executed:
fd -t f "test" vc-verifier/kotlin/vcverifier --type f -name "*StatusChecker*" | head -20Repository: inji/vc-verifier
Length of output: 290
🏁 Script executed:
find vc-verifier -type f -name "*StatusChecker*Test*.kt" | head -20Repository: inji/vc-verifier
Length of output: 42
🏁 Script executed:
find vc-verifier -type f -name "*LdpStatusChecker*" | head -20Repository: inji/vc-verifier
Length of output: 184
🏁 Script executed:
find vc-verifier -type f -path "*/test/*" -name "*.kt" | xargs grep -l "statusMessage\|STATUS_MESSAGE" 2>/dev/null | head -10Repository: inji/vc-verifier
Length of output: 199
🏁 Script executed:
cat -n vc-verifier/kotlin/vcverifier/src/test/java/io/mosip/vercred/vcverifier/credentialverifier/statusChecker/StatusListRevocationCheckerTest.kt | grep -A 10 -B 10 "statusMessage\|STATUS_MESSAGE"Repository: inji/vc-verifier
Length of output: 4022
🏁 Script executed:
cat -n vc-verifier/kotlin/vcverifier/src/test/java/io/mosip/vercred/vcverifier/credentialverifier/statusChecker/StatusListRevocationCheckerTest.kt | sed -n '315,343p'Repository: inji/vc-verifier
Length of output: 1245
🏁 Script executed:
cat vc-verifier/kotlin/vcverifier/src/test/resources/ldp_vc/vcUnrevoked-https.json 2>/dev/null | head -200Repository: inji/vc-verifier
Length of output: 3401
🏁 Script executed:
cat -n vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/statusChecker/LdpStatusChecker.kt | sed -n '1,80p'Repository: inji/vc-verifier
Length of output: 4724
Fix statusMessage type to match W3C Bitstring Status List specification.
Per the W3C Bitstring Status List specification, statusMessage must be an array of objects (each with status as a hex string and message as a string), not a Map. The current cast entry[STATUS_MESSAGE] as? Map<*, *> will reject spec-compliant entries. The implementation should be updated to parse and validate the array format correctly, where array length must equal 2^statusSize.
🤖 Prompt for AI Agents
In
`@vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/statusChecker/LdpStatusChecker.kt`
around lines 248 - 262, The code treats STATUS_MESSAGE as a Map but per W3C
Bitstring Status List it must be an array of objects; update the
LdpStatusChecker logic (around isStatusMessageAvailable, STATUS_MESSAGE,
statusSize, purpose, logger) to cast entry[STATUS_MESSAGE] to a List<*> instead
of Map<*, *>, verify the list length equals expectedStatusCount
(1.shl(statusSize)), and validate each list element is a Map containing a
hex-string "status" and a string "message"; on any mismatch throw
StatusCheckException with STATUS_VERIFICATION_ERROR (same semantics as existing
errors) and keep the existing logging of the status message content.
...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
| @BeforeAll | ||
| fun setup() { | ||
| Util.documentLoader = LocalDocumentLoader | ||
| mockkObject(NetworkManagerClient) | ||
| loadMockPublicKeys() | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and read the test file
find . -type f -name "LdpVerifierTest.kt" | head -5Repository: inji/vc-verifier
Length of output: 180
🏁 Script executed:
# Read the test file to understand the full context
cat -n ./vc-verifier/kotlin/vcverifier/src/test/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/LdpVerifierTest.ktRepository: inji/vc-verifier
Length of output: 5534
🏁 Script executed:
# Find the testutils file with mockHttpResponse
find . -type f -name "*.kt" | xargs grep -l "fun mockHttpResponse" | head -5Repository: inji/vc-verifier
Length of output: 128
🏁 Script executed:
# Read the testutils file to see how mockHttpResponse is implemented
cat -n ./vc-verifier/kotlin/vcverifier/src/test/java/testutils/TestUtils.ktRepository: inji/vc-verifier
Length of output: 815
🏁 Script executed:
# Find and read the NetworkManagerClient class
find . -type f -name "NetworkManagerClient.kt" | head -5Repository: inji/vc-verifier
Length of output: 172
🏁 Script executed:
# Read the NetworkManagerClient class definition
cat -n ./vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/networkManager/NetworkManagerClient.ktRepository: inji/vc-verifier
Length of output: 2954
🏁 Script executed:
# Check if there are other test files using mockHttpResponse
find . -type f -name "*.kt" -path "*/test/*" | xargs grep -l "mockHttpResponse" | head -10Repository: inji/vc-verifier
Length of output: 573
🏁 Script executed:
# Check the other test files to see how they set up mocks
for file in ./vc-verifier/kotlin/vcverifier/src/test/java/io/mosip/vercred/vcverifier/CredentialsVerifierTest.kt ./vc-verifier/kotlin/vcverifier/src/test/java/io/mosip/vercred/vcverifier/PresentationVerifierTest.kt ./vc-verifier/kotlin/vcverifier/src/test/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/CwtVerifierTest.kt; do
echo "=== $file ==="
grep -n "mockkObject\|mockHttpResponse" "$file" | head -15
doneRepository: inji/vc-verifier
Length of output: 2589
Mock the companion object so HTTP stubs actually apply.
mockHttpResponse stubs NetworkManagerClient.Companion.sendHTTPRequest, but the setup mocks NetworkManagerClient (the class) instead. This prevents the mock stubs from being applied, leaving HTTP calls unmocked and making tests flaky. Other test files (CredentialsVerifierTest, CwtVerifierTest) correctly mock the companion object.
🔧 Suggested fix
- mockkObject(NetworkManagerClient)
+ mockkObject(NetworkManagerClient.Companion)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @BeforeAll | |
| fun setup() { | |
| Util.documentLoader = LocalDocumentLoader | |
| mockkObject(NetworkManagerClient) | |
| loadMockPublicKeys() | |
| } | |
| `@BeforeAll` | |
| fun setup() { | |
| Util.documentLoader = LocalDocumentLoader | |
| mockkObject(NetworkManagerClient.Companion) | |
| loadMockPublicKeys() | |
| } |
🤖 Prompt for AI Agents
In
`@vc-verifier/kotlin/vcverifier/src/test/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/LdpVerifierTest.kt`
around lines 26 - 31, The test setup currently mocks the NetworkManagerClient
class (mockkObject(NetworkManagerClient)) but the HTTP stubs target the
companion method NetworkManagerClient.Companion.sendHTTPRequest, so stubs never
apply; change the setup to mock the companion object instead
(mockkObject(NetworkManagerClient.Companion)) so that
NetworkManagerClient.Companion.sendHTTPRequest can be stubbed by
mockHttpResponse; keep the rest of setup (Util.documentLoader and
loadMockPublicKeys) unchanged.
vc-verifier/kotlin/vcverifier/src/test/resources/vp/public_key/didIdentityServiceKey.json
Show resolved
Hide resolved
| "assertionMethod": [ | ||
| "did:web:mosip.github.io:inji-config:qa-inji1:mock" | ||
| ], |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat vc-verifier/kotlin/vcverifier/src/test/resources/vp/public_key/didMockKey.jsonRepository: inji/vc-verifier
Length of output: 2066
🌐 Web query:
W3C DID Core specification assertionMethod authentication verificationMethod references
💡 Result:
In W3C DID Core (DIDs v1.0), the linkage between verificationMethod, authentication, and assertionMethod works like this:
-
verificationMethod: an (optional) set of verification method objects (maps) where each entry defines a method (e.g., a public key) and MUST includeid,type,controller, plus the relevant verification material (e.g.,publicKeyJwk,publicKeyMultibase). [1] -
authenticationandassertionMethod: these are verification relationship properties. If present, each is a set of one or more verification methods, and each verification method may be either:- embedded (a full verification method object/map inline), or
- referenced (a DID URL string, typically with a fragment like
did:example:123#keys-1). [1]
-
How “references” are resolved (normative behavior):
- If the relationship value is a map, it’s embedded and used directly.
- If it’s a URL string, it’s included by reference; you dereference the URL and then find a verification method object whose
idmatches that URL, either elsewhere in the same DID document (commonly underverificationMethod) or in another DID document. [1]
-
Key rule: a verification method can only be used for a given purpose if it is present in the corresponding verification relationship (e.g., a key in
authenticationis not automatically valid forassertionMethod). [1]
Sources: DID Core v1.0 Recommendation (19 July 2022). [1] DID Specification Registries (relationship/property registry context). [2]
Use verificationMethod IDs in assertionMethod/authentication to keep the DID document valid per W3C DID Core spec.
Currently these fields point to the bare DID without a fragment, but there is no verificationMethod with that ID. According to W3C DID Core specification, verification relationships (like assertionMethod and authentication) must reference verificationMethod entries either by embedding them or by URL reference—and those URL references must resolve to an id in the document's verificationMethod array. The bare DID cannot be resolved to any of the three verification methods available (which have fragment IDs). Use the existing verificationMethod fragments to make the DID document spec-compliant.
Suggested update
- "assertionMethod": [
- "did:web:mosip.github.io:inji-config:qa-inji1:mock"
- ],
+ "assertionMethod": [
+ "did:web:mosip.github.io:inji-config:qa-inji1:mock#QizhlYu5pcVa8l_fmElYzeLukKPShmkd0vIfF1Y7Oco",
+ "did:web:mosip.github.io:inji-config:qa-inji1:mock#ojsIq-D4X3UvTBdMQwRudb8cR-BOhoXtdM6cj74hKGM",
+ "did:web:mosip.github.io:inji-config:qa-inji1:mock#kMVyZTvx8G0h1YZnUW4OJtr2LRVZUuGl8pQuJl3ymXI"
+ ],- "authentication": [
- "did:web:mosip.github.io:inji-config:qa-inji1:mock"
- ]
+ "authentication": [
+ "did:web:mosip.github.io:inji-config:qa-inji1:mock#QizhlYu5pcVa8l_fmElYzeLukKPShmkd0vIfF1Y7Oco",
+ "did:web:mosip.github.io:inji-config:qa-inji1:mock#ojsIq-D4X3UvTBdMQwRudb8cR-BOhoXtdM6cj74hKGM",
+ "did:web:mosip.github.io:inji-config:qa-inji1:mock#kMVyZTvx8G0h1YZnUW4OJtr2LRVZUuGl8pQuJl3ymXI"
+ ]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "assertionMethod": [ | |
| "did:web:mosip.github.io:inji-config:qa-inji1:mock" | |
| ], | |
| "assertionMethod": [ | |
| "did:web:mosip.github.io:inji-config:qa-inji1:mock#QizhlYu5pcVa8l_fmElYzeLukKPShmkd0vIfF1Y7Oco", | |
| "did:web:mosip.github.io:inji-config:qa-inji1:mock#ojsIq-D4X3UvTBdMQwRudb8cR-BOhoXtdM6cj74hKGM", | |
| "did:web:mosip.github.io:inji-config:qa-inji1:mock#kMVyZTvx8G0h1YZnUW4OJtr2LRVZUuGl8pQuJl3ymXI" | |
| ], |
🤖 Prompt for AI Agents
In
`@vc-verifier/kotlin/vcverifier/src/test/resources/vp/public_key/didMockKey.json`
around lines 2 - 4, The assertionMethod and authentication arrays currently
reference the bare DID string instead of the verificationMethod entries; update
those entries to use the exact verificationMethod id values (the DID plus
fragment) that appear in the document's verificationMethod array so each
reference resolves to a verificationMethod id (e.g., replace
"did:web:mosip.github.io:inji-config:qa-inji1" with the corresponding
verificationMethod ids present in the verificationMethod array), ensuring
assertionMethod and authentication reference existing verificationMethod ids.
mayuradesh
changed the title
|
Fixed all comments for CWT related code. There are few comments for previously released code. @swatigoel pls review the same. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@README.md`:
- Around line 50-51: Update the algorithm lists in the table rows for
`vc+sd-jwt` and `dc+sd-jwt` to fix spacing: insert a single space after the
comma between `RS256` and `ES256` so the sequence reads "PS256, RS256, ES256,
EdDSA (Ed25519)" in both the `vc+sd-jwt` and `dc+sd-jwt` rows.
🧹 Nitpick comments (2)
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/keyResolver/types/jwks/JwksPublicKeyResolver.kt (2)
13-13: Unused logger declaration.The logger is declared but never used in the current implementation. The logging was likely removed during the exception handling refactor.
♻️ Proposed fix to remove unused logger
class JwksPublicKeyResolver : PublicKeyResolver { - - private val logger = Logger.getLogger(JwksPublicKeyResolver::class.java.name) override fun resolve(uri: String, keyId: String?): PublicKey {Alternatively, consider adding logging back for observability (e.g., logging the uri being resolved at debug level, or logging errors before rethrowing).
38-39: Remove empty companion object.The empty companion object serves no purpose and can be removed.
♻️ Proposed fix
} } - - companion object }
|
@coderabbitai hide resolved comments |
|
✅ Actions performedComments resolved. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
vc-verifier/kotlin/vcverifier/build.gradle.kts (1)
137-140: Avoid duplicating release version literals.Line 137 and Line 140 hardcode
"1.7.0"in two places, which is easy to desync on the next release bump.Proposed refactor
+val releaseVersion = "1.7.0" + tasks.register<Jar>("jarRelease") { @@ manifest { attributes["Implementation-Title"] = project.name - attributes["Implementation-Version"] = "1.7.0" + attributes["Implementation-Version"] = releaseVersion } @@ - archiveVersion.set("1.7.0") + archiveVersion.set(releaseVersion)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vc-verifier/kotlin/vcverifier/build.gradle.kts` around lines 137 - 140, The build script duplicates the release literal "1.7.0" in attributes["Implementation-Version"] and archiveVersion.set("1.7.0"); extract a single version constant (e.g., val releaseVersion or reuse project.version) and use that symbol in both places so the version is maintained in one place; update references in the block setting attributes["Implementation-Version"] and the call to archiveVersion.set to use that version symbol.vc-verifier/kotlin/vcverifier/publish-artifact.gradle (1)
31-51: Consider extracting repeated POM metadata into shared constants/helpers.
url/scm/developer/organizationmetadata is duplicated acrossaarandjarRelease; centralizing it will reduce future drift risk during releases/rebrands.Also applies to: 126-147
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vc-verifier/kotlin/vcverifier/publish-artifact.gradle` around lines 31 - 51, The POM metadata (fields like url, scm (connection/developerConnection/url/tag), developer/organization entries, and licenses) is duplicated between the aar and jarRelease POM generation blocks; extract these repeated values into shared constants or a helper method (e.g., a map or function referenced by both POM builders) and replace the inline asNode().appendNode(...) calls in both the aar and jarRelease sections so they call the common helper (or inject the map) to populate url, scm, developers, organization and license nodes; update the references in both blocks so future changes live in one place and remove the duplicated literal strings now appearing in developers, scm, url and licenses.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@vc-verifier/kotlin/vcverifier/publish-artifact.gradle`:
- Line 41: Replace the malformed SCM developerConnection value that uses
"scm:git:ssh://github.com:inji/vc-verifier.git" with a valid SSH-style URI;
locate the appendNode call appendNode('developerConnection', ...) in
publish-artifact.gradle and change the string to a proper form such as
"scm:git:git@github.com:inji/vc-verifier.git" or
"scm:git:ssh://git@github.com/inji/vc-verifier.git" so the URI parser treats the
username not as a port.
---
Nitpick comments:
In `@vc-verifier/kotlin/vcverifier/build.gradle.kts`:
- Around line 137-140: The build script duplicates the release literal "1.7.0"
in attributes["Implementation-Version"] and archiveVersion.set("1.7.0"); extract
a single version constant (e.g., val releaseVersion or reuse project.version)
and use that symbol in both places so the version is maintained in one place;
update references in the block setting attributes["Implementation-Version"] and
the call to archiveVersion.set to use that version symbol.
In `@vc-verifier/kotlin/vcverifier/publish-artifact.gradle`:
- Around line 31-51: The POM metadata (fields like url, scm
(connection/developerConnection/url/tag), developer/organization entries, and
licenses) is duplicated between the aar and jarRelease POM generation blocks;
extract these repeated values into shared constants or a helper method (e.g., a
map or function referenced by both POM builders) and replace the inline
asNode().appendNode(...) calls in both the aar and jarRelease sections so they
call the common helper (or inject the map) to populate url, scm, developers,
organization and license nodes; update the references in both blocks so future
changes live in one place and remove the duplicated literal strings now
appearing in developers, scm, url and licenses.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2f45166f-c507-4871-b649-80abad7cfcee
📒 Files selected for processing (3)
vc-verifier/kotlin/sample-java-app/pom.xmlvc-verifier/kotlin/vcverifier/build.gradle.ktsvc-verifier/kotlin/vcverifier/publish-artifact.gradle
| appendNode('developerConnection', 'scm:git:ssh://github.com:mosip/vc-verifier.git') | ||
| appendNode('url', "https://github.com/mosip/vc-verifier") | ||
| appendNode('connection', 'scm:git:git://github.com/inji/vc-verifier.git') | ||
| appendNode('developerConnection', 'scm:git:ssh://github.com:inji/vc-verifier.git') |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="vc-verifier/kotlin/vcverifier/publish-artifact.gradle"
# Check if file exists
if [ ! -f "$FILE" ]; then
echo "File not found: $FILE"
exit 1
fi
echo "=== File content at relevant lines ==="
echo "Line 41:"
sed -n '41p' "$FILE"
echo ""
echo "Line 136:"
sed -n '136p' "$FILE"
echo ""
echo "=== All developerConnection entries ==="
rg -n "developerConnection" "$FILE"
echo ""
echo "=== URI parse validation ==="
python3 << 'PY'
from urllib.parse import urlparse
samples = [
"scm:git:ssh://github.com:inji/vc-verifier.git",
"scm:git:ssh://git@github.com/inji/vc-verifier.git",
]
print("Current vs Proposed URI validation:")
for s in samples:
uri = s.replace("scm:git:", "", 1)
p = urlparse(uri)
port_error = None
try:
_ = p.port
except ValueError as e:
port_error = str(e)
print(f"\nURI: {s}")
print(f" Scheme: {p.scheme}")
print(f" Netloc: {p.netloc}")
print(f" Hostname: {p.hostname}")
print(f" Path: {p.path}")
print(f" Port error: {port_error}")
PYRepository: inji/vc-verifier
Length of output: 1031
Fix malformed SCM developerConnection URI.
Lines 41 and 136 use scm:git:ssh://github.com:inji/vc-verifier.git, which is not a valid SSH URI format. The URI parser treats inji as a port number, causing a port parsing error that can break SCM metadata parsing.
Proposed fix
- appendNode('developerConnection', 'scm:git:ssh://github.com:inji/vc-verifier.git')
+ appendNode('developerConnection', 'scm:git:ssh://git@github.com/inji/vc-verifier.git')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| appendNode('developerConnection', 'scm:git:ssh://github.com:inji/vc-verifier.git') | |
| appendNode('developerConnection', 'scm:git:ssh://git@github.com/inji/vc-verifier.git') |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vc-verifier/kotlin/vcverifier/publish-artifact.gradle` at line 41, Replace
the malformed SCM developerConnection value that uses
"scm:git:ssh://github.com:inji/vc-verifier.git" with a valid SSH-style URI;
locate the appendNode call appendNode('developerConnection', ...) in
publish-artifact.gradle and change the string to a proper form such as
"scm:git:git@github.com:inji/vc-verifier.git" or
"scm:git:ssh://git@github.com/inji/vc-verifier.git" so the URI parser treats the
username not as a port.
3 participants
Summary by CodeRabbit
New Features
Improvements
Documentation
Chores