Skip to content

Conversation

@Piyush7034
Copy link
Contributor

@Piyush7034 Piyush7034 commented Jan 21, 2026

Summary by CodeRabbit

  • Improvements
    • Enhanced mobile document payload encoding for stronger standards compliance and interoperability.
    • Optimized certificate inclusion during digital signing for more reliable signature packaging.
    • Improved signing error handling and logging for clearer failure reporting.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Jan 21, 2026

Walkthrough

The PR updates signMSO in MDocProcessor to CBOR-encode the MSO payload (stored in msoCbor) and use those bytes as the COSE_Sign1 payload; it removes the protected header x5c and sets an unprotected header with certificate inclusion enabled.

Changes

Cohort / File(s) Summary
MSO CBOR Encoding
certify-service/src/main/java/io/mosip/certify/utils/MDocProcessor.java
Replace JSON+Base64URL MSO payload with CBOR-encoded bytes (msoCbor) for COSE_Sign1 payload; remove protected header x5c and add unprotected header with includeCertificate = true; adjust signing/error handling accordingly.

Sequence Diagram(s)

(Skipped — changes are localized serialization/signing adjustments without multi-component flow requiring visualization.)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • swatigoel

Poem

🐰 Bytes hop in place of strings tonight,
CBOR tucked in snug and tight,
MSO signed with certificates true,
A tiny change, a clearer view,
Hooray — the rabbit says, "Good byte!" 🥕✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly and specifically describes the main change: adding CBOR encoding for MSO objects in the signing process.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
certify-service/src/main/java/io/mosip/certify/utils/MDocProcessor.java (1)

458-484: Error handling gap: encodeToCBOR exceptions bypass the catch block.

The encodeToCBOR method (line 224-235) throws a generic Exception, but the catch block on line 481 only catches CertifyException. If CBOR encoding fails, the exception propagates unwrapped, bypassing the error logging and consistent error wrapping.

Proposed fix to catch all exceptions
         } catch (CertifyException e) {
             log.error("Error during COSE signing: {}", e.getMessage(), e);
             throw new CertifyException(ErrorConstants.VC_SIGNING_ERROR, "COSE signing failed: " + e.getMessage());
+        } catch (Exception e) {
+            log.error("Error during COSE signing: {}", e.getMessage(), e);
+            throw new CertifyException(ErrorConstants.VC_SIGNING_ERROR, "COSE signing failed: " + e.getMessage());
         }

@@ -470,6 +472,9 @@ public byte[] signMSO(Map<String, Object> mso, String appID, String refID, Strin
protectedHeader.put("x5c", true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we remove setting up of protected header?

Since only alg is requried to be in the protected header which is already added by the Cose_Sign1 creating function

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed

@sonarqubecloud
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
E Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants