[INJICERT] Remove sensitive data from debug logs#619
[INJICERT] Remove sensitive data from debug logs#619swatigoel merged 1 commit intoinji:release-0.14.xfrom
Conversation
Signed-off-by: jaswanthkumarpolisetty <jaswanthkumar.p@thoughtworks.com>
WalkthroughThe PR modifies logging statements across the credential signing and issuance service, converting debug-level logs to info-level logs and refining messages for consistency. No functional changes or control flow alterations are introduced. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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/services/CertifyIssuanceServiceImpl.java (1)
307-315:⚠️ Potential issue | 🟠 MajorCertifyException catch block re-wraps the exception with a new error code, hiding the original.
The
CertifyExceptionthrown at line 314 replaces the original error code (e.getErrorCode()) with a hardcoded"ERROR_SIGNING_QR_DATA"string. If the original exception already carries a meaningful domain error code (e.g., fromsignQrEntries), this catch block obscures it.Also,
signQrEntriesitself already throwsCertifyExceptionwith specific error codes (lines 356, 384, 388), so catchingCertifyExceptionhere and re-wrapping it loses that specificity. Consider lettingCertifyExceptionpropagate directly.🐛 Suggested fix: let CertifyException propagate
- } catch (CertifyException e) { - log.error("Error during signing qr data: {}", e.getMessage()); - throw new CertifyException("ERROR_SIGNING_QR_DATA", e.getMessage()); - } + } catch (CertifyException e) { + log.error("Error during credential generation: {}", e.getErrorCode()); + throw e; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@certify-service/src/main/java/io/mosip/certify/services/CertifyIssuanceServiceImpl.java` around lines 307 - 315, The CertifyException catch block in CertifyIssuanceServiceImpl is re-wrapping and replacing the original domain error code with a hardcoded "ERROR_SIGNING_QR_DATA", which hides the original error information (e.g., from signQrEntries); change this catch to either remove the CertifyException handler entirely or rethrow the caught exception as-is (throw e) so the original getErrorCode()/message propagates unchanged, ensuring callers receive the original CertifyException from signQrEntries and other methods.
🧹 Nitpick comments (1)
certify-service/src/main/java/io/mosip/certify/services/CertifyIssuanceServiceImpl.java (1)
325-341: Excessive info-level logging inside the loop.Lines 325, 327, 330, and 341 each emit an
infolog on every iteration (or once before the loop). For credentials with multiple QR entries this will be noisy in production. Consider keeping per-iteration progress logs atdebuglevel and retaining only the summary logs (e.g., line 391) atinfo.Since none of these per-entry messages appear to contain sensitive data (they only log the index
i),debugwould still be safe here and aligned with the PR's goal of cleaning up logs.♻️ Suggested: demote per-entry logs back to debug
- log.info("Processing QR entry index {}", i); + log.debug("Processing QR entry index {}", i);- log.info("Claim 169 mapped data for QR entry index {}", i); + log.debug("Claim 169 mapped data for QR entry index {}", i);Apply similarly to lines 372 and 377.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@certify-service/src/main/java/io/mosip/certify/services/CertifyIssuanceServiceImpl.java` around lines 325 - 341, The per-entry info logs in CertifyIssuanceServiceImpl that run inside the qrDataJson loop are too verbose for production; change the per-iteration log.info calls (the "Processing QR entry index {}", "Claim 169 mapped data for QR entry index {}", and similar per-entry messages related to claim169KeyMapper/claim169ValueMapper usage around the loop that call pixelPass.getMappedData) to log.debug so only summary/info-level messages remain for overall progress, and apply the same demotion to the two other per-entry logs referenced (the ones called later in the method around where lines 372 and 377 are logged). Ensure you keep the existing info logs that summarize initialization/completion but demote per-entry indexing/debug traces to debug.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@certify-service/src/main/java/io/mosip/certify/services/CertifyIssuanceServiceImpl.java`:
- Around line 307-315: The CertifyException catch block in
CertifyIssuanceServiceImpl is re-wrapping and replacing the original domain
error code with a hardcoded "ERROR_SIGNING_QR_DATA", which hides the original
error information (e.g., from signQrEntries); change this catch to either remove
the CertifyException handler entirely or rethrow the caught exception as-is
(throw e) so the original getErrorCode()/message propagates unchanged, ensuring
callers receive the original CertifyException from signQrEntries and other
methods.
---
Nitpick comments:
In
`@certify-service/src/main/java/io/mosip/certify/services/CertifyIssuanceServiceImpl.java`:
- Around line 325-341: The per-entry info logs in CertifyIssuanceServiceImpl
that run inside the qrDataJson loop are too verbose for production; change the
per-iteration log.info calls (the "Processing QR entry index {}", "Claim 169
mapped data for QR entry index {}", and similar per-entry messages related to
claim169KeyMapper/claim169ValueMapper usage around the loop that call
pixelPass.getMappedData) to log.debug so only summary/info-level messages remain
for overall progress, and apply the same demotion to the two other per-entry
logs referenced (the ones called later in the method around where lines 372 and
377 are logged). Ensure you keep the existing info logs that summarize
initialization/completion but demote per-entry indexing/debug traces to debug.
2 participants
Summary by CodeRabbit