Backend: ehnc - Einvoice auth and generation logs#1228
Open
kavya-shree-s wants to merge 1 commit into
Open
Conversation
WalkthroughTwo e-invoice interface functions refactored to emit logging before and after operations. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/mobility-core/src/Kernel/External/GSTEInvoice/Interface.hs`:
- Around line 24-28: The current authenticateEInvoice function logs the entire
serviceConfig and resp (via show) which may leak credentials/tokens; change the
logging to avoid printing full structures by replacing show serviceConfig and
show resp with safe metadata (e.g., log only the config type/identifier and
non-sensitive flags) or call a redaction helper (e.g., renderRedactedConfig ::
CharteredInfoEInvoiceConfig -> Text and renderRedactedResp :: RespType -> Text)
before logging; keep the calls to CharteredInfo.authenticate(cfg) intact but
ensure any error/success logs only include redacted or high-level info (status,
service name, request id) not raw credentials or tokens.
- Around line 42-46: The generateEInvoice function currently logs the full
payload and resp (variables payload and resp) which may contain sensitive
invoice/customer tax and IRN/QR data; update the two logInfo calls in
generateEInvoice (and ensure any downstream CharteredInfo.generateInvoice return
shape is used) to redact or extract only non-sensitive identifiers/status (e.g.,
invoiceId or invoiceNumber, customerId masked, operation status, and a masked
IRN/QR showing only last 4 characters) and log those minimal fields instead of
show payload/show resp; do not remove logging entirely—emit safe, contextual
messages that avoid full payload/response serialization.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1bed3a29-4b43-4ff6-b5a8-c3e445191662
📒 Files selected for processing (1)
lib/mobility-core/src/Kernel/External/GSTEInvoice/Interface.hs
Comment on lines
+24
to
+28
| authenticateEInvoice serviceConfig = do | ||
| logInfo $ "GSTEInvoice.authenticateEInvoice: calling GSP with config=" <> show serviceConfig | ||
| resp <- case serviceConfig of | ||
| CharteredInfoEInvoiceConfig cfg -> CharteredInfo.authenticate cfg | ||
| logInfo $ "GSTEInvoice.authenticateEInvoice: received response=" <> show resp |
There was a problem hiding this comment.
Avoid logging full auth config and auth response.
show serviceConfig and show resp can persist credentials, GST identifiers, or auth tokens/session data in application logs. Prefer lifecycle logs with safe metadata only, or add explicit redacted renderers.
🔒 Proposed safe logging shape
authenticateEInvoice serviceConfig = do
- logInfo $ "GSTEInvoice.authenticateEInvoice: calling GSP with config=" <> show serviceConfig
+ logInfo "GSTEInvoice.authenticateEInvoice: calling GSP"
resp <- case serviceConfig of
CharteredInfoEInvoiceConfig cfg -> CharteredInfo.authenticate cfg
- logInfo $ "GSTEInvoice.authenticateEInvoice: received response=" <> show resp
+ logInfo "GSTEInvoice.authenticateEInvoice: received response"
pure resp📝 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.
Suggested change
| authenticateEInvoice serviceConfig = do | |
| logInfo $ "GSTEInvoice.authenticateEInvoice: calling GSP with config=" <> show serviceConfig | |
| resp <- case serviceConfig of | |
| CharteredInfoEInvoiceConfig cfg -> CharteredInfo.authenticate cfg | |
| logInfo $ "GSTEInvoice.authenticateEInvoice: received response=" <> show resp | |
| authenticateEInvoice serviceConfig = do | |
| logInfo "GSTEInvoice.authenticateEInvoice: calling GSP" | |
| resp <- case serviceConfig of | |
| CharteredInfoEInvoiceConfig cfg -> CharteredInfo.authenticate cfg | |
| logInfo "GSTEInvoice.authenticateEInvoice: received response" | |
| pure resp |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/mobility-core/src/Kernel/External/GSTEInvoice/Interface.hs` around lines
24 - 28, The current authenticateEInvoice function logs the entire serviceConfig
and resp (via show) which may leak credentials/tokens; change the logging to
avoid printing full structures by replacing show serviceConfig and show resp
with safe metadata (e.g., log only the config type/identifier and non-sensitive
flags) or call a redaction helper (e.g., renderRedactedConfig ::
CharteredInfoEInvoiceConfig -> Text and renderRedactedResp :: RespType -> Text)
before logging; keep the calls to CharteredInfo.authenticate(cfg) intact but
ensure any error/success logs only include redacted or high-level info (status,
service name, request id) not raw credentials or tokens.
Comment on lines
+42
to
+46
| generateEInvoice serviceConfig authToken payload = do | ||
| logInfo $ "GSTEInvoice.generateEInvoice: calling GSP with payload=" <> show payload | ||
| resp <- case serviceConfig of | ||
| CharteredInfoEInvoiceConfig cfg -> CharteredInfo.generateInvoice cfg authToken payload | ||
| logInfo $ "GSTEInvoice.generateEInvoice: received response=" <> show resp |
There was a problem hiding this comment.
Do not log full e-invoice payloads or generation responses.
payload and resp likely contain invoice/customer tax data and generated IRN/QR details. Logging them verbatim creates a privacy/compliance risk; log only non-sensitive identifiers/status, with redaction where needed.
🔒 Proposed safe logging shape
generateEInvoice serviceConfig authToken payload = do
- logInfo $ "GSTEInvoice.generateEInvoice: calling GSP with payload=" <> show payload
+ logInfo "GSTEInvoice.generateEInvoice: calling GSP"
resp <- case serviceConfig of
CharteredInfoEInvoiceConfig cfg -> CharteredInfo.generateInvoice cfg authToken payload
- logInfo $ "GSTEInvoice.generateEInvoice: received response=" <> show resp
+ logInfo "GSTEInvoice.generateEInvoice: received response"
pure resp📝 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.
Suggested change
| generateEInvoice serviceConfig authToken payload = do | |
| logInfo $ "GSTEInvoice.generateEInvoice: calling GSP with payload=" <> show payload | |
| resp <- case serviceConfig of | |
| CharteredInfoEInvoiceConfig cfg -> CharteredInfo.generateInvoice cfg authToken payload | |
| logInfo $ "GSTEInvoice.generateEInvoice: received response=" <> show resp | |
| generateEInvoice serviceConfig authToken payload = do | |
| logInfo "GSTEInvoice.generateEInvoice: calling GSP" | |
| resp <- case serviceConfig of | |
| CharteredInfoEInvoiceConfig cfg -> CharteredInfo.generateInvoice cfg authToken payload | |
| logInfo "GSTEInvoice.generateEInvoice: received response" | |
| pure resp |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/mobility-core/src/Kernel/External/GSTEInvoice/Interface.hs` around lines
42 - 46, The generateEInvoice function currently logs the full payload and resp
(variables payload and resp) which may contain sensitive invoice/customer tax
and IRN/QR data; update the two logInfo calls in generateEInvoice (and ensure
any downstream CharteredInfo.generateInvoice return shape is used) to redact or
extract only non-sensitive identifiers/status (e.g., invoiceId or invoiceNumber,
customerId masked, operation status, and a masked IRN/QR showing only last 4
characters) and log those minimal fields instead of show payload/show resp; do
not remove logging entirely—emit safe, contextual messages that avoid full
payload/response serialization.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Type of Change
Description
Additional Changes
Motivation and Context
How did you test it?
Checklist
./dev/format-all-files.shSummary by CodeRabbit
Release Notes