Backend: Ench - IMAP implementation for email fetch#1171
Merged
kavya-shree-s merged 1 commit intomainfrom Mar 26, 2026
Merged
Conversation
kavya-shree-s
force-pushed
the
Backend-feat-parsing-report
branch
from
d3c2819 to
7db59c9
Compare
kavya-shree-s
changed the title
kavya-shree-s
force-pushed
the
Backend-feat-parsing-report
branch
from
7db59c9 to
dd237c6
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds encrypted-credential types and an encryption shim; implements Email (IMAP via curl) and SFTP (sshpass/scp) settlement fetchers with MIME CSV extraction and base64 decoding; composes fetch+parse flows in the Interface; wires an email-fetch CLI, tests, fixtures, and an accidental absolute-path file. Changes
Sequence DiagramsequenceDiagram
actor User
participant CLI as "Test CLI\n(test-settlement-parser/src/Main.hs)"
participant Enc as "Encryption Shim\n(Kernel.External.Encryption)"
participant Fetch as "Email Fetch\n(Sources/Email.hs)"
participant IMAP as "IMAP Server"
participant Parser as "Parser\n(Interface.hs)"
participant FS as "Local FS"
User->>CLI: run email <pg> <user> <app-pass> [subj]
CLI->>Enc: decrypt(encryptedPassword)
Enc-->>CLI: plainPassword
CLI->>Fetch: fetchSettlementFileDebugRaw(config, plainPassword)
Fetch->>IMAP: SEARCH (UNSEEN/ALL, subject)
IMAP-->>Fetch: UIDs
Fetch->>IMAP: FETCH UID BODY
IMAP-->>Fetch: raw MIME
Fetch->>FS: write debug-raw-mime.txt
CLI->>Fetch: fetchSettlementFileWithPlainPassword(config, plainPassword)
Fetch->>IMAP: SEARCH / FETCH (attachment)
IMAP-->>Fetch: raw MIME with attachment
Fetch->>Fetch: extractCsvFromMime -> decodeContent
Fetch-->>CLI: CSV bytes
CLI->>FS: write samples/<pg>/<attachment>
CLI->>Parser: fetchAndParsePaymentSettlement(sourceConfig, parseConfig)
Parser-->>CLI: parsed result
CLI->>FS: write output/<pg>/<attachment>.parsed.txt
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
kavya-shree-s
force-pushed
the
Backend-feat-parsing-report
branch
from
dd237c6 to
1a210b8
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (7)
test-settlement-parser/src/Main.hs (2)
76-84: Clarify the comment about the dummy password field.The comment on line 81 says
-- dummy, not usedbut thepasswordfield is required byEmailConfig. The actual plaintext password is passed separately tofetchSettlementFileWithPlainPassword. Consider making the comment clearer:📝 Clearer comment
password = Encrypted (T.pack pass), -- dummy, not used + -- Note: This Encrypted value is not used; we pass plainPassword directly + -- to fetchSettlementFileWithPlainPassword to bypass encryption for local testing🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test-settlement-parser/src/Main.hs` around lines 76 - 84, The comment next to the EmailConfig.password field is ambiguous; update it to clearly state that the Encrypted (T.pack pass) value is a placeholder and that the real plaintext password is supplied separately to fetchSettlementFileWithPlainPassword, e.g. replace "-- dummy, not used" with a clarifying note referencing EmailConfig.password, Encrypted and fetchSettlementFileWithPlainPassword so future readers understand the placeholder usage and where the actual password is provided.
68-119: Email fetch only supports payment report parsing.The
testEmailFetchfunction hardcodesparsePaymentSettlementCsv(line 118). If the email contains a payout settlement report, it will be parsed incorrectly or fail.Consider adding a report type parameter to the email command, similar to the file-based parsing:
cabal run test-settlement-parser -- email <pg> <payment|payout> <user> <pass> [subject]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test-settlement-parser/src/Main.hs` around lines 68 - 119, testEmailFetch always calls parsePaymentSettlementCsv which fails for payout reports; add a report-type parameter to the email command and thread it into testEmailFetch (similar to file-based parsing), then choose the parser based on that value (e.g. call parsePaymentSettlementCsv for "payment" and parsePayoutSettlementCsv for "payout"). Update the command-line handling to accept the extra argument (payment|payout) and pass it into testEmailFetch, and ensure SettlementParseConfig/filePattern usage remains the same; locate and modify testEmailFetch, the CLI dispatch that invokes it, and the parse call site where parsePaymentSettlementCsv is hardcoded.lib/mobility-core/src/Kernel/External/Settlement/Sources/Email.hs (2)
194-200: Base64 decode fallback silently returns raw content on failure.When
B64.decodefails, the code silently falls back to treating the content as plain text. This could mask encoding issues where the email claimsContent-Transfer-Encoding: base64but contains invalid base64.Consider logging or noting when the fallback is used, or checking the
Content-Transfer-Encodingheader to decide the decoding strategy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/mobility-core/src/Kernel/External/Settlement/Sources/Email.hs` around lines 194 - 200, The function decodeContent currently swallows B64.decode failures and returns the raw text, which masks malformed base64; update decodeContent to propagate the decode error instead of silently falling back: when calling B64.decode in decodeContent, capture the Left error and return Left with a clear error Text (including the B64.decode error message and noting base64 decoding failed), leaving the Right branch to return the decoded LBS; this makes failures visible to callers (and allows higher-level code to consult Content-Transfer-Encoding if needed).
111-166: Significant code duplication withfetchSettlementFileWithPlainPassword.
fetchSettlementFileDebugRawduplicates the IMAP URL construction and curl invocation logic. Consider extracting shared helpers:♻️ Refactor suggestion
-- Shared IMAP search helper searchEmails :: MonadIO m => EmailConfig -> Text -> Text -> m (Either Text [Text]) searchEmails config plainPassword searchQuery = do let imapUrl = buildImapUrl config (exitCode, stdout, stderr) <- liftIO $ readProcessWithExitCode "curl" [...] "" case exitCode of ExitFailure code -> pure $ Left $ "IMAP SEARCH failed..." ExitSuccess -> pure $ Right $ extractMessageIds (T.pack stdout) -- Shared fetch helper fetchEmailMime :: MonadIO m => EmailConfig -> Text -> Text -> m (Either Text Text) fetchEmailMime config password msgId = ...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/mobility-core/src/Kernel/External/Settlement/Sources/Email.hs` around lines 111 - 166, fetchSettlementFileDebugRaw duplicates IMAP URL construction and curl logic already present in fetchSettlementFileWithPlainPassword; refactor by extracting a shared buildImapUrl helper and two helpers like searchEmails (performs the SEARCH curl call and returns either error or [msgId] using extractMessageIds) and fetchEmailMime (performs the fetch curl call and returns the raw MIME Text for a given msgId), then replace the duplicated URL and readProcessWithExitCode calls inside fetchSettlementFileDebugRaw (and reuse in fetchSettlementFileWithPlainPassword) to call these new helpers and propagate their results.lib/mobility-core/src/Kernel/External/Settlement/Types.hs (1)
71-74: Consider using a record forSFTPSourceConfigto clarify the second field's purpose.The
Textparameter inSFTPSourceConfig SFTPConfig Textis presumably the filename, but this isn't clear from the type definition. A record or named wrapper would improve readability.📝 Possible improvement
data SettlementSourceConfig - = SFTPSourceConfig SFTPConfig Text + = SFTPSourceConfig { sftpConfig :: SFTPConfig, sftpFileName :: Text } | EmailSourceConfig EmailConfig deriving (Show, Eq, Generic, ToJSON, FromJSON)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/mobility-core/src/Kernel/External/Settlement/Types.hs` around lines 71 - 74, The SFTPSourceConfig variant of SettlementSourceConfig uses an anonymous Text field which obscures its meaning; change the algebraic data type to use a record for SFTPSourceConfig (e.g., SFTPSourceConfig { sftpConfig :: SFTPConfig, sftpFilename :: Text }) so the second field is named and self-documenting, update any pattern matches or constructors referencing SFTPSourceConfig and related serialization (ToJSON/FromJSON/derive Generic) to use the new record names.lib/mobility-core/src/Kernel/External/Settlement/Sources/SFTP.hs (1)
35-54: Consider validating and sanitizing path inputs.The
fileNameandremotePathvalues from configuration flow directly into file path construction. WhilereadProcessWithExitCodepasses arguments safely (not via shell), the paths could still cause issues:
- Path traversal via
../sequences infileName- Special characters that affect SCP path interpretation
Consider validating that
fileNamecontains no path separators or traversal sequences.🛡️ Example validation
fetchSettlementFile config fileName = do -- Validate fileName has no path components when (T.any (== '/') fileName || ".." `T.isInfixOf` fileName) $ pure $ Left "Invalid filename: contains path separators or traversal" decryptedPassword <- decrypt config.password -- ... rest of implementation🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/mobility-core/src/Kernel/External/Settlement/Sources/SFTP.hs` around lines 35 - 54, In fetchSettlementFile validate and sanitize fileName (and optionally config.remotePath) before building scpTarget: ensure fileName contains no path separators or traversal sequences (no '/' or '\' and no ".." substrings), reject or return Left on invalid input, normalize/trim trailing slashes from config.remotePath to avoid double separators, and only then construct scpTarget and call readProcessWithExitCode; reference fileName, config.remotePath, scpTarget, and readProcessWithExitCode so the checks are added at the top of fetchSettlementFile.test-settlement-parser/README.md (1)
178-184: Add language identifier to fenced code block.The code block showing the file paths is missing a language identifier. This was flagged by markdownlint.
📝 Proposed fix
Example for `billdesk`: -``` +```text samples/billdesk/billdesk_settlement_report_sample.csv # downloaded CSV output/billdesk/billdesk_settlement_report_sample.csv.parsed.txt # parsed output output/billdesk/debug-raw-mime.txt # raw MIME</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@test-settlement-parser/README.mdaround lines 178 - 184, The fenced code
block under "Example forbilldesk" in README.md is missing a language
identifier; update the opening fence fromtotext so it becomestext and thereby mark the block as plain text (affecting the three lines that start with "samples/billdesk/..." and the closing).</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In@lib/mobility-core/src/Kernel/External/Settlement/Sources/Email.hs:
- Around line 66-68: The command-line passes credentials via "--user" (see the
code building the curl args in fetchSettlementFile / the Email.hs block using
T.unpack config.username and plainPassword), exposing passwords to ps/proc;
instead create a temporary netrc file containing the machine/username/password
for the target host, set restrictive permissions, pass "--netrc-file" with that
temp path (or use curl's stdin/netrc support) and remove the temp file after the
request; locate the argument construction that currently emits "--user" with
T.unpack config.username <> ":" <> T.unpack plainPassword and replace it with
logic that writes the netrc content and adds "--netrc-file", tempPath (ensuring
cleanup and secure permissions).- Around line 54-70: The SEARCH query builds searchQuery by directly
interpolating config.subjectFilter into the IMAP command (used when building the
curl args passed to readProcessWithExitCode), which allows IMAP command
injection; sanitize or escape config.subjectFilter (escape double quotes and
backslashes per IMAP quoted-string rules or validate/whitelist allowed
characters) before constructing searchQuery so the resulting string is always
safe (e.g., produce a safeQuoted variable from config.subjectFilter and use
"SUBJECT "" <> safeQuoted <> "" UNSEEN"); ensure this sanitized value is used
wherever searchQuery is built and avoid direct interpolation of raw
config.subjectFilter into the curl "-X" argument.In
@lib/mobility-core/src/Kernel/External/Settlement/Sources/SFTP.hs:
- Around line 44-47: The SCP invocation built in the readProcessWithExitCode
call currently forces "-o StrictHostKeyChecking=no" which disables SSH host
verification; change this by removing the hardcoded "StrictHostKeyChecking=no"
and instead accept a configurable host-checking mode (e.g., "accept-new" or a
value from configuration) or ensure the target server key is provisioned into
known_hosts before running; update the code that composes the scp arguments (the
call referencing scpTarget, decryptedPassword, portArg, localPath in the
readProcessWithExitCode invocation) to read the host-checking policy from config
or to skip adding the option entirely and document that known_hosts must be
pre-populated.In
@lib/mobility-core/src/Kernel/External/Settlement/Sources/Untitled:
- Line 1: Delete the accidental file named "Untitled" that contains the
developer machine absolute path (the local-path artifact in the Sources folder);
remove it from git tracking with git rm (or git rm --cached if you need to
preserve local copy), commit the deletion, and push; if this file was already
pushed to a shared repo consider rewriting history or running git
filter-repo/BFG to expunge the sensitive path; finally scan the repo for other
absolute paths and add appropriate .gitignore entries or CI checks to prevent
similar accidental commits.In
@test-settlement-parser/output/billdesk/debug-raw-mime.txt:
- Around line 1-104: The test fixture contains real PII (email addresses
kavya@nammayatri.in and imap8182@gmail.com), real IPs (209.85.220.41) and full
authentication headers (DKIM/ARC); replace those with clearly fake values (e.g.,
test-sender@example.com, test-receiver@example.com), swap IPs to RFC5737
placeholders (e.g., 192.0.2.1, 198.51.100.2), and remove or redact long
DKIM/ARC/Authentication-Results blocks so the MIME parsing inputs retain
structure but no real identifiers; update occurrences in this fixture (search
for the strings kavya@nammayatri.in, imap8182@gmail.com, 209.85.220.41, and
header names DKIM-Signature / ARC-*) accordingly.In
@test-settlement-parser/output/debug-raw-mime.txt:
- Around line 1-104: This fixture exposes real PII (e.g., the "From: Kavyashree
S kavya@nammayatri.in", IPs and server names) and appears duplicate of the
other fixture containing "Subject: Settlement Report" and similar Message-ID;
redact or replace all PII in this fixture (replace email addresses, IPs,
hostnames, and full Message-ID with synthetic placeholders) and then remove or
consolidate duplicates by keeping only one redacted fixture (the one containing
"Subject: Settlement Report" / the Message-ID shown) to avoid maintenance
overhead.In
@test-settlement-parser/src/Main.hs:
- Around line 121-134: extractAttachmentName currently scans all lines for any
"filename=" which can return a non-CSV attachment name; change it to locate the
same MIME part that extractCsvFromMime targets (the "text/csv" or
"application/csv" Content-Type) and extract the filename only from that part.
Specifically: in extractAttachmentName, when given Just rawMime, split into
lines (T.lines), find the index of a line containing "text/csv" or
"application/csv" (use T.toLower for case-insensitive match), collect a small
window of nearby header lines (e.g., idx-2 to idx+2) and search that window for
"filename=", falling back to "email-attachment.csv" if none found; this keeps
extractAttachmentName aligned with extractCsvFromMime's selection logic.
Nitpick comments:
In@lib/mobility-core/src/Kernel/External/Settlement/Sources/Email.hs:
- Around line 194-200: The function decodeContent currently swallows B64.decode
failures and returns the raw text, which masks malformed base64; update
decodeContent to propagate the decode error instead of silently falling back:
when calling B64.decode in decodeContent, capture the Left error and return Left
with a clear error Text (including the B64.decode error message and noting
base64 decoding failed), leaving the Right branch to return the decoded LBS;
this makes failures visible to callers (and allows higher-level code to consult
Content-Transfer-Encoding if needed).- Around line 111-166: fetchSettlementFileDebugRaw duplicates IMAP URL
construction and curl logic already present in
fetchSettlementFileWithPlainPassword; refactor by extracting a shared
buildImapUrl helper and two helpers like searchEmails (performs the SEARCH curl
call and returns either error or [msgId] using extractMessageIds) and
fetchEmailMime (performs the fetch curl call and returns the raw MIME Text for a
given msgId), then replace the duplicated URL and readProcessWithExitCode calls
inside fetchSettlementFileDebugRaw (and reuse in
fetchSettlementFileWithPlainPassword) to call these new helpers and propagate
their results.In
@lib/mobility-core/src/Kernel/External/Settlement/Sources/SFTP.hs:
- Around line 35-54: In fetchSettlementFile validate and sanitize fileName (and
optionally config.remotePath) before building scpTarget: ensure fileName
contains no path separators or traversal sequences (no '/' or '' and no ".."
substrings), reject or return Left on invalid input, normalize/trim trailing
slashes from config.remotePath to avoid double separators, and only then
construct scpTarget and call readProcessWithExitCode; reference fileName,
config.remotePath, scpTarget, and readProcessWithExitCode so the checks are
added at the top of fetchSettlementFile.In
@lib/mobility-core/src/Kernel/External/Settlement/Types.hs:
- Around line 71-74: The SFTPSourceConfig variant of SettlementSourceConfig uses
an anonymous Text field which obscures its meaning; change the algebraic data
type to use a record for SFTPSourceConfig (e.g., SFTPSourceConfig { sftpConfig
:: SFTPConfig, sftpFilename :: Text }) so the second field is named and
self-documenting, update any pattern matches or constructors referencing
SFTPSourceConfig and related serialization (ToJSON/FromJSON/derive Generic) to
use the new record names.In
@test-settlement-parser/README.md:
- Around line 178-184: The fenced code block under "Example for
billdesk" in
README.md is missing a language identifier; update the opening fence fromtotext so it becomestext and thereby mark the block as plain text (affecting the three lines that start with "samples/billdesk/..." and the closing).In
@test-settlement-parser/src/Main.hs:
- Around line 76-84: The comment next to the EmailConfig.password field is
ambiguous; update it to clearly state that the Encrypted (T.pack pass) value is
a placeholder and that the real plaintext password is supplied separately to
fetchSettlementFileWithPlainPassword, e.g. replace "-- dummy, not used" with a
clarifying note referencing EmailConfig.password, Encrypted and
fetchSettlementFileWithPlainPassword so future readers understand the
placeholder usage and where the actual password is provided.- Around line 68-119: testEmailFetch always calls parsePaymentSettlementCsv
which fails for payout reports; add a report-type parameter to the email command
and thread it into testEmailFetch (similar to file-based parsing), then choose
the parser based on that value (e.g. call parsePaymentSettlementCsv for
"payment" and parsePayoutSettlementCsv for "payout"). Update the command-line
handling to accept the extra argument (payment|payout) and pass it into
testEmailFetch, and ensure SettlementParseConfig/filePattern usage remains the
same; locate and modify testEmailFetch, the CLI dispatch that invokes it, and
the parse call site where parsePaymentSettlementCsv is hardcoded.</details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Organization UI **Review profile**: CHILL **Plan**: Pro **Run ID**: `3ade9023-c469-4a2a-82b4-bcc7a10ff41c` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between a5e9c4c1bf945c4414755179009e279cada74c84 and 1a210b894315481a1ee176fb2bec82fe5abee54b. </details> <details> <summary>⛔ Files ignored due to path filters (3)</summary> * `test-settlement-parser/output/email-attachment.csv` is excluded by `!**/*.csv` * `test-settlement-parser/samples/billdesk/billdesk_settlement_report_sample.csv` is excluded by `!**/*.csv` * `test-settlement-parser/samples/billdesk/email-attachment.csv` is excluded by `!**/*.csv` </details> <details> <summary>📒 Files selected for processing (16)</summary> * `lib/mobility-core/mobility-core.cabal` * `lib/mobility-core/src/Kernel/External/Settlement/Interface.hs` * `lib/mobility-core/src/Kernel/External/Settlement/Sources/Email.hs` * `lib/mobility-core/src/Kernel/External/Settlement/Sources/SFTP.hs` * `lib/mobility-core/src/Kernel/External/Settlement/Sources/Untitled` * `lib/mobility-core/src/Kernel/External/Settlement/Types.hs` * `lib/mobility-core/test/app/Main.hs` * `lib/mobility-core/test/src/SettlementEmail.hs` * `test-settlement-parser/README.md` * `test-settlement-parser/output/billdesk/billdesk_settlement_report_sample.csv.parsed.txt` * `test-settlement-parser/output/billdesk/debug-raw-mime.txt` * `test-settlement-parser/output/billdesk/email-attachment.csv.parsed.txt` * `test-settlement-parser/output/debug-raw-mime.txt` * `test-settlement-parser/shims/Kernel/External/Encryption.hs` * `test-settlement-parser/src/Main.hs` * `test-settlement-parser/test-settlement-parser.cabal` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Comment on lines
+54
to
+70
| -- NOTE: For testing, use "ALL" to include already-read emails. | ||
| -- For production, switch back to "UNSEEN". | ||
| searchQuery = maybe "UNSEEN" (\s -> "SUBJECT \"" <> T.unpack s <> "\" UNSEEN") config.subjectFilter | ||
| -- searchQuery = maybe "ALL" (\s -> "SUBJECT \"" <> T.unpack s <> "\"") config.subjectFilter | ||
|
|
||
| (exitCode, stdout, stderr) <- | ||
| liftIO $ | ||
| readProcessWithExitCode | ||
| "curl" | ||
| [ "--silent", | ||
| "--url", | ||
| imapUrl, | ||
| "--user", | ||
| T.unpack config.username <> ":" <> T.unpack plainPassword, | ||
| "-X", | ||
| "SEARCH " <> searchQuery | ||
| ] |
There was a problem hiding this comment.
Critical: IMAP command injection via unescaped subjectFilter.
The subjectFilter value is interpolated directly into the IMAP SEARCH command without escaping quotes or special characters:
searchQuery = maybe "UNSEEN" (\s -> "SUBJECT \"" <> T.unpack s <> "\" UNSEEN") config.subjectFilterA malicious or malformed subjectFilter containing " could break out of the SUBJECT string and inject arbitrary IMAP commands. For example, a filter like foo" OR ALL " would search all emails.
Escape or validate the subject filter to prevent injection.
🔒 Proposed fix - escape double quotes
+escapeImapString :: Text -> Text
+escapeImapString = T.replace "\"" "\\\""
+
fetchSettlementFileWithPlainPassword config plainPassword = do
let imapUrl = ...
- searchQuery = maybe "UNSEEN" (\s -> "SUBJECT \"" <> T.unpack s <> "\" UNSEEN") config.subjectFilter
+ searchQuery = maybe "UNSEEN" (\s -> "SUBJECT \"" <> T.unpack (escapeImapString s) <> "\" UNSEEN") config.subjectFilter🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/mobility-core/src/Kernel/External/Settlement/Sources/Email.hs` around
lines 54 - 70, The SEARCH query builds searchQuery by directly interpolating
config.subjectFilter into the IMAP command (used when building the curl args
passed to readProcessWithExitCode), which allows IMAP command injection;
sanitize or escape config.subjectFilter (escape double quotes and backslashes
per IMAP quoted-string rules or validate/whitelist allowed characters) before
constructing searchQuery so the resulting string is always safe (e.g., produce a
safeQuoted variable from config.subjectFilter and use "SUBJECT \"" <> safeQuoted
<> "\" UNSEEN"); ensure this sanitized value is used wherever searchQuery is
built and avoid direct interpolation of raw config.subjectFilter into the curl
"-X" argument.
Comment on lines
+66
to
+68
| "--user", | ||
| T.unpack config.username <> ":" <> T.unpack plainPassword, | ||
| "-X", |
There was a problem hiding this comment.
Credentials visible in process list.
The password is passed as a command-line argument (--user user:password), which is visible to other users via ps aux or /proc on Linux systems.
For production, consider using curl's --netrc-file option or stdin password input (-n --netrc-file <(echo ...)) to avoid exposing credentials in the process list. For a local testing tool this may be acceptable, but the production fetchSettlementFile path uses the same mechanism.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/mobility-core/src/Kernel/External/Settlement/Sources/Email.hs` around
lines 66 - 68, The command-line passes credentials via "--user" (see the code
building the curl args in fetchSettlementFile / the Email.hs block using
T.unpack config.username and plainPassword), exposing passwords to ps/proc;
instead create a temporary netrc file containing the machine/username/password
for the target host, set restrictive permissions, pass "--netrc-file" with that
temp path (or use curl's stdin/netrc support) and remove the temp file after the
request; locate the argument construction that currently emits "--user" with
T.unpack config.username <> ":" <> T.unpack plainPassword and replace it with
logic that writes the netrc content and adds "--netrc-file", tempPath (ensuring
cleanup and secure permissions).
Comment on lines
+44
to
+47
| readProcessWithExitCode | ||
| "sshpass" | ||
| ["-p", T.unpack decryptedPassword, "scp", "-P", portArg, "-o", "StrictHostKeyChecking=no", scpTarget, localPath] | ||
| "" |
There was a problem hiding this comment.
Security: StrictHostKeyChecking=no disables SSH host verification.
Disabling host key checking makes the connection vulnerable to man-in-the-middle attacks. An attacker could intercept the connection, capture the password, and serve malicious settlement files.
For production use, consider:
- Pre-populating known hosts with the expected server key
- Using
StrictHostKeyChecking=accept-newif you want to trust on first use - Distributing server fingerprints via configuration
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/mobility-core/src/Kernel/External/Settlement/Sources/SFTP.hs` around
lines 44 - 47, The SCP invocation built in the readProcessWithExitCode call
currently forces "-o StrictHostKeyChecking=no" which disables SSH host
verification; change this by removing the hardcoded "StrictHostKeyChecking=no"
and instead accept a configurable host-checking mode (e.g., "accept-new" or a
value from configuration) or ensure the target server key is provisioned into
known_hosts before running; update the code that composes the scp arguments (the
call referencing scpTarget, decryptedPassword, portArg, localPath in the
readProcessWithExitCode invocation) to read the host-checking policy from config
or to skip adding the option entirely and document that known_hosts must be
pre-populated.
| @@ -0,0 +1 @@ | |||
| /Users/kavyashrees/Documents/nammayatri-backend/nammayatri No newline at end of file | |||
There was a problem hiding this comment.
Remove accidental local-path file (PII exposure).
Line 1 contains a developer machine absolute path in a source folder. This leaks local user/workstation details and introduces a non-functional artifact that should not ship.
🧹 Proposed fix (delete the file)
--- a/lib/mobility-core/src/Kernel/External/Settlement/Sources/Untitled
+++ /dev/null
@@ -1 +0,0 @@
-/Users/kavyashrees/Documents/nammayatri-backend/nammayatri📝 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
| /Users/kavyashrees/Documents/nammayatri-backend/nammayatri |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/mobility-core/src/Kernel/External/Settlement/Sources/Untitled` at line 1,
Delete the accidental file named "Untitled" that contains the developer machine
absolute path (the local-path artifact in the Sources folder); remove it from
git tracking with git rm (or git rm --cached if you need to preserve local
copy), commit the deletion, and push; if this file was already pushed to a
shared repo consider rewriting history or running git filter-repo/BFG to expunge
the sensitive path; finally scan the repo for other absolute paths and add
appropriate .gitignore entries or CI checks to prevent similar accidental
commits.
Comment on lines
+1
to
+104
| Delivered-To: imap8182@gmail.com | ||
| Received: by 2002:a54:2e05:0:b0:2c9:cf1c:d449 with SMTP id q5csp805986ecp; | ||
| Mon, 23 Mar 2026 04:23:27 -0700 (PDT) | ||
| X-Received: by 2002:a05:6402:3786:b0:662:e479:e883 with SMTP id 4fb4d7f45d1cf-668c91152damr7502185a12.5.1774265007607; | ||
| Mon, 23 Mar 2026 04:23:27 -0700 (PDT) | ||
| ARC-Seal: i=2; a=rsa-sha256; t=1774265007; cv=pass; | ||
| d=google.com; s=arc-20240605; | ||
| b=FNhMoEu9qeIbhy8O93bowQnUsXCvJfCgBmOEZIEOK5RssUS9w+iZlNOpl1NGwtknUH | ||
| sA7CuaceZH7XxnC45TpcTNGwh1q+jsTaFkIlOd1S3j0n3b1cfXifS+h6aV2N/p4RfV3U | ||
| railYAc0jLTHuA0gGS1HlLbWAYWg3Mpo0kaePlnb/YpZ5YgZdXMQco4xFWkHgFhkY0Lx | ||
| eWfS2VR9cODLwwhlcsyGPQY2cbHC67AC9RcTgO4OSP2FW4oO2Zpxkl/lbgAp0zCZvxjn | ||
| Qf/bSxRfSZLa+YoBzgGt8Nh3wpzvIx6HgH2mhSWZMXClWn+EErcWlnN7OTy83K1STyYD | ||
| I9uQ== | ||
| ARC-Message-Signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20240605; | ||
| h=to:subject:message-id:date:from:mime-version:dkim-signature; | ||
| bh=cF1NqSJRlFv4Sw/wW0O45uVfPVH2z2ougaR7t7KmHow=; | ||
| fh=Y4RBdMh5b9UVrCPYtOvVi84uJ9kxdyNx270nS+Kf04s=; | ||
| b=QoS9z7lerfrJbTH5kqNhtRERJZ+6CX0myjsTTOlGarYLk2GXw6Q88etEQQbJQRUvrM | ||
| AbaF14Sd6rCUjlVSczmOosvaD+Tff7E/3xHiQPWJYHGEaM/cPPlAIQj2FMRIzla3y/au | ||
| 0HPNofwKDqw96xIe20Wl0rRL6WEcQLYfrcaE429Wveo3M4IHLph0Yl3ukHW6QfWd3o18 | ||
| YCBXTXupvd8fMRyc5nWR9oaCMdrutMOrAHxRKleI8mprQ/wtN6gO+k7kds1tEunwbZWM | ||
| X0Nv+ptwOW5bL7kdKdYKKbzkyqGvfIfy+KAXEnydV+E7FnslMb4caFVEbgdDyD9cCr6S | ||
| CiZQ==; | ||
| dara=google.com | ||
| ARC-Authentication-Results: i=2; mx.google.com; | ||
| dkim=pass header.i=@nammayatri.in header.s=google header.b=au606YRd; | ||
| arc=pass (i=1); | ||
| spf=pass (google.com: domain of kavya@nammayatri.in designates 209.85.220.41 as permitted sender) smtp.mailfrom=kavya@nammayatri.in; | ||
| dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=nammayatri.in; | ||
| dara=pass header.i=@gmail.com | ||
| Return-Path: <kavya@nammayatri.in> | ||
| Received: from mail-sor-f41.google.com (mail-sor-f41.google.com. [209.85.220.41]) | ||
| by mx.google.com with SMTPS id 4fb4d7f45d1cf-668d17eedf7sor865295a12.3.2026.03.23.04.23.27 | ||
| for <imap8182@gmail.com> | ||
| (Google Transport Security); | ||
| Mon, 23 Mar 2026 04:23:27 -0700 (PDT) | ||
| Received-SPF: pass (google.com: domain of kavya@nammayatri.in designates 209.85.220.41 as permitted sender) client-ip=209.85.220.41; | ||
| Authentication-Results: mx.google.com; | ||
| dkim=pass header.i=@nammayatri.in header.s=google header.b=au606YRd; | ||
| arc=pass (i=1); | ||
| spf=pass (google.com: domain of kavya@nammayatri.in designates 209.85.220.41 as permitted sender) smtp.mailfrom=kavya@nammayatri.in; | ||
| dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=nammayatri.in; | ||
| dara=pass header.i=@gmail.com | ||
| ARC-Seal: i=1; a=rsa-sha256; t=1774265007; cv=none; | ||
| d=google.com; s=arc-20240605; | ||
| b=fS+6rhsZjHO54EpgS37b40Me565Wi1AoWQH5nyOI6p0CPT84EkfBfdcVGKfb/5Tjih | ||
| k9J4/744atpCShKS4qcrxSIS3qbeSgQH6wxtb9Jp53WwCYH5Sc6FixtdYTq5mMvgHjHI | ||
| V/U2Fd/dRv+XrxVW4QwacuTENt4kFs4JwBynS5bmdBKgrKMdYbKfYX4BL9BfBp8Ttxcl | ||
| 04Ig4qihUcvRvRV9TcRv3PmPfmHsMG7wi8s0dEblR8plzR0RnJCptoACJs3aUkyZ4NhJ | ||
| mGUAeI0/Vwk3kJWaft1jRd6AXC8fOfrEwMe2QDogayWYnAeEXYnyPNNsjcbOfeVNtL+f | ||
| 1BPw== | ||
| ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20240605; | ||
| h=to:subject:message-id:date:from:mime-version:dkim-signature; | ||
| bh=cF1NqSJRlFv4Sw/wW0O45uVfPVH2z2ougaR7t7KmHow=; | ||
| fh=Y4RBdMh5b9UVrCPYtOvVi84uJ9kxdyNx270nS+Kf04s=; | ||
| b=ksw016nfzrv8zC4L8ZJpuZvH8nVOjOqJgck2E6vFP8xdY/GQKNdQQyiPOwZs4oBSgl | ||
| 8Ku/58Jnn6TV5NCMe9gWBa7TW1zxcpAUBNyoQgxj3A1tsa93bvYDWIAYbYRaPmuvlGzp | ||
| LT0mHVRAGL4wPIBRfzgjXgEEhXVSJPwBt5IjH0YJnCMEOYu5T8SlhRoBrrcBGzt15AT4 | ||
| qqBW8Awtc8J0Jusxu89Zmqqakj/o43fDU4ASz/8CwIKKdkdUDTXKjik/MZ8TS17WUcjR | ||
| AeP5v7ff9kjm6lOWN3cHmKO1+OuyHmfd/3fXk6OzE0aFuF3OA8YrpiXuCGVUCKIxeLxI | ||
| 0w3A==; | ||
| dara=google.com | ||
| ARC-Authentication-Results: i=1; mx.google.com; arc=none | ||
| DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; | ||
| d=nammayatri.in; s=google; t=1774265007; x=1774869807; dara=google.com; | ||
| h=to:subject:message-id:date:from:mime-version:from:to:cc:subject | ||
| :date:message-id:reply-to; | ||
| bh=cF1NqSJRlFv4Sw/wW0O45uVfPVH2z2ougaR7t7KmHow=; | ||
| b=au606YRdEEFZRyWSGQd9eT3ZytA9pns7ODqnN0cLc/nekDhohjUPHLNhZvsdl6Hdwp | ||
| f7ixwygf2gdtABDwOmUHeJwYwBsMvD5lphzvnvHirwglPKPwAmrGH2SU/Ru0jBP25x8M | ||
| 5itJ1azpY5rxQ0zdsDImGrxqIYY0rZrl08ae2AJ0CJFBW9KZkhA9L12g3qml0JXvgCyD | ||
| nCrguu5DEwXqon51GU2hQiw/R3XneMT/8cz2VzcT6eHydRcUCKsLOjLoSUlVb8Tn5TYt | ||
| k96oPx4HsHKUksyC0zCm243giJYcAtH+IPO1MffTC5xFBCSZtd5uPKOzS2AGhW+EBaD8 | ||
| ucvA== | ||
| X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; | ||
| d=1e100.net; s=20251104; t=1774265007; x=1774869807; | ||
| h=to:subject:message-id:date:from:mime-version:x-gm-gg | ||
| :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; | ||
| bh=cF1NqSJRlFv4Sw/wW0O45uVfPVH2z2ougaR7t7KmHow=; | ||
| b=oFj8Q56hN+kh7udTgnwnzezK306qrYE59dmkxiFlTzc/JxIkNqtv5zD4kjUxn4KvAL | ||
| MS+b14sxbJjsETWfNiVllkYoOshmDPq7UdCqAFHBgy8fQ2EOqo9cDMB/u5IBKVKffQGE | ||
| MV3HIL5d7dWnLE+EsV0OR8e9CC0WMqGcyJ0qhGgLs7prpZ0FfHPC+UWCJIW82MHuTS6B | ||
| SuVFjCpbX+NvpGOIWJ3vJj/lDB30D0jbQepnFqepe6OiwHPzBfXwL+yvYXvNCOz0kBAQ | ||
| 5dMCZfUJ8gyw2lhPF/qln6MotEF2wYyjXDNqeNZDfYujp+o4bIEnWHFl+clBMHGu1yQu | ||
| 1Grg== | ||
| X-Gm-Message-State: AOJu0YxoBChNa8kUjs205zFJF8DsazX7eDWfRTkG54Cqw2jO3zR1qzg6 | ||
| vKj3boYPWf8CSjeQPJuV+kC0Ad7ggrscY6ZH5NHAPwzTgGT6V8yQ81V7NGyoBqkSf6nogRS2+AC | ||
| TkWW77DzjtaVrnTwi/ZvazAkbSjK2dZDQptK5VcZQ0UW58RgfO2lEy5s= | ||
| X-Gm-Gg: ATEYQzxsEiP/mIZNoKN7Cqmc5qUmSpeSOldA/0Qq4DNZlmjpCFRYoagpW409BSc3/PD | ||
| kzsACskk+2t1eT7u/CC3KiG+KjfwB9/HLPP4Uj0IusEeUCALYea4Bw6DQw8aL8rnoaloMz7Egk3 | ||
| pR16gN+UDEUqjw2tQdUX8z0slxvAI7IByFOxCUWopEbeCKz/1L4D7gj3ZHCy8tUaTGNxPuaPNQg | ||
| 3HmJsbJfjJrzOy7vHcGXcMzhf/ZNIbey+VJXZCj1p4lj0isOFdJ7/CUe6NeJc14ABskIPQVJFdN | ||
| Zz0FiVjXzWIqkmAq2AIEHCPFHn6kr4ypIMAOXCXOFYiXnJNlibrm6BDizKMgqggEMovjYe4ZZAI | ||
| GGQanSw== | ||
| X-Received: by 2002:a05:6402:518b:b0:668:50f5:2291 with SMTP id | ||
| 4fb4d7f45d1cf-668c9328721mr4228511a12.2.1774265006673; Mon, 23 Mar 2026 | ||
| 04:23:26 -0700 (PDT) | ||
| MIME-Version: 1.0 | ||
| From: Kavyashree S <kavya@nammayatri.in> | ||
| Date: Mon, 23 Mar 2026 16:53:14 +0530 | ||
| X-Gm-Features: AQROBzBP1fN1Nc1hBuTcHFsgsM0qkMHuBl6G6632HLvtS2seJTvgVz48C-BL83Q | ||
| Message-ID: <CAGbM3Fq5LH0cwyyo-eaP6oUcbdgescojkj7W=PXOc3ixshwDUA@mail.gmail.com> | ||
| Subject: Settlement Report | ||
| To: imap8182@gmail.com |
There was a problem hiding this comment.
Potential PII exposure in test fixture.
This fixture contains what appear to be real email addresses (kavya@nammayatri.in, imap8182@gmail.com), real IP addresses, and server names. If these are actual personal/work identifiers, consider anonymizing them for the test fixture to avoid committing PII to the repository.
Recommended approach:
- Replace with clearly fake addresses like
test-sender@example.com,test-receiver@example.com - Use placeholder IP addresses from RFC 5737 documentation ranges (e.g.,
192.0.2.x,198.51.100.x) - Remove or redact DKIM signatures and authentication headers that aren't essential for MIME parsing tests
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test-settlement-parser/output/billdesk/debug-raw-mime.txt` around lines 1 -
104, The test fixture contains real PII (email addresses kavya@nammayatri.in and
imap8182@gmail.com), real IPs (209.85.220.41) and full authentication headers
(DKIM/ARC); replace those with clearly fake values (e.g.,
test-sender@example.com, test-receiver@example.com), swap IPs to RFC5737
placeholders (e.g., 192.0.2.1, 198.51.100.2), and remove or redact long
DKIM/ARC/Authentication-Results blocks so the MIME parsing inputs retain
structure but no real identifiers; update occurrences in this fixture (search
for the strings kavya@nammayatri.in, imap8182@gmail.com, 209.85.220.41, and
header names DKIM-Signature / ARC-*) accordingly.
Comment on lines
+1
to
+104
| Delivered-To: imap8182@gmail.com | ||
| Received: by 2002:a54:2e05:0:b0:2c9:cf1c:d449 with SMTP id q5csp781708ecp; | ||
| Mon, 23 Mar 2026 03:33:49 -0700 (PDT) | ||
| X-Received: by 2002:a17:907:c8a3:b0:b98:9da:4d94 with SMTP id a640c23a62f3a-b982f38bd3cmr731336666b.48.1774262028962; | ||
| Mon, 23 Mar 2026 03:33:48 -0700 (PDT) | ||
| ARC-Seal: i=2; a=rsa-sha256; t=1774262028; cv=pass; | ||
| d=google.com; s=arc-20240605; | ||
| b=ax1tGCKOhRlQGjN0TusIxlVKvyvNIAidbef8LX5f6bp/URytQrzw4bGzD21ACHnoCN | ||
| LZR6y0V7vWAOyUalwvdL/AseZPrTFx1Q/e/vdfon9EFQk9HA6857ejr9AD31cW/AbfCe | ||
| T54MlyjPlQOSQWWCSuAqRqzPmuYv4CJW/z1UKNa+5glLazqQwdtHroeOHHXZq7emVpnh | ||
| bUTYxIEG1jncQnCi0uNS5j8g6ZiB3d0+aJ1rES/qq9tWQ3XqhWzYF3eWNAP8dXxW99jr | ||
| 05Zz8AJpcMsK8KyzR8UI9izDT+8h1yzZfctEURAcbMmrkL32xFRtuaHRy+udePwxGroW | ||
| ucOQ== | ||
| ARC-Message-Signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20240605; | ||
| h=to:subject:message-id:date:from:mime-version:dkim-signature; | ||
| bh=pPXq/Duvy1sn+wseb+RkBCXNVAcq2TdS1+vHWqffvQI=; | ||
| fh=Y4RBdMh5b9UVrCPYtOvVi84uJ9kxdyNx270nS+Kf04s=; | ||
| b=feUNPabooRrrEpr4EbBlPDgzW4tTbG6hVE7JuNDOKeAzefGHjrbA4ZYDMlefJtGBvF | ||
| FWuDo/XZKIsagfgDcyOb4J8XZl8Mavrl/Rz+ExmHOk6QVdz+GjEAfgd/sqhTd+H2qB1V | ||
| kYJ+QsmdCDL+3LpZhUCCW3cFUsREQ3hsLmLQncgx2Ws53lo12Cug37/1FavKtFyWUJDM | ||
| d0BOSe3HlBOHNwjqibee4GZBH+NDfXJGbtD0yI8MH4idxnuVPAbCrt2RTcwvO9pvsyGg | ||
| GP/Q5NLnOM8a43zSWfEHbn1DxjfWcXRsFxjKKb8WybTCuNxL2pNmcMZpkQeUXVNcQ8k2 | ||
| qALg==; | ||
| dara=google.com | ||
| ARC-Authentication-Results: i=2; mx.google.com; | ||
| dkim=pass header.i=@nammayatri.in header.s=google header.b=QMZDAwFk; | ||
| arc=pass (i=1); | ||
| spf=pass (google.com: domain of kavya@nammayatri.in designates 209.85.220.41 as permitted sender) smtp.mailfrom=kavya@nammayatri.in; | ||
| dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=nammayatri.in; | ||
| dara=pass header.i=@gmail.com | ||
| Return-Path: <kavya@nammayatri.in> | ||
| Received: from mail-sor-f41.google.com (mail-sor-f41.google.com. [209.85.220.41]) | ||
| by mx.google.com with SMTPS id a640c23a62f3a-b98334f0389sor105595166b.3.2026.03.23.03.33.48 | ||
| for <imap8182@gmail.com> | ||
| (Google Transport Security); | ||
| Mon, 23 Mar 2026 03:33:48 -0700 (PDT) | ||
| Received-SPF: pass (google.com: domain of kavya@nammayatri.in designates 209.85.220.41 as permitted sender) client-ip=209.85.220.41; | ||
| Authentication-Results: mx.google.com; | ||
| dkim=pass header.i=@nammayatri.in header.s=google header.b=QMZDAwFk; | ||
| arc=pass (i=1); | ||
| spf=pass (google.com: domain of kavya@nammayatri.in designates 209.85.220.41 as permitted sender) smtp.mailfrom=kavya@nammayatri.in; | ||
| dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=nammayatri.in; | ||
| dara=pass header.i=@gmail.com | ||
| ARC-Seal: i=1; a=rsa-sha256; t=1774262028; cv=none; | ||
| d=google.com; s=arc-20240605; | ||
| b=NbPGbEHqWHlmc1x7loSRbNbzthPwLnLvxSO0OHvZ8zERJlimGGcxEmy8flou324QkC | ||
| s2ElInKKDIK0IBfupwpxCvEfvPcfOxZyyBUzoBe+MoUUpVLvgbMIu4l1nAZJ/HFvvmkj | ||
| xN25VqOPKWyrVws/tzGkXX2qfaJVpMlW7GCTywDM5iKBJWq/Vu+uAjIp6+43nWzQXVZc | ||
| MJzMWcSnW0lPM/Rp5T+Kk6DlkSOpBCiE9mHGKUa7ZlUU62UBEwJPLMtrziMhuhhw90nx | ||
| PnsH3J2wZtT2QfxCi4ByIcgifrjFEgPSIb0ruN2cqAxuaU1Jh1srwwQbYbsu/b3TzA5o | ||
| 2XkQ== | ||
| ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20240605; | ||
| h=to:subject:message-id:date:from:mime-version:dkim-signature; | ||
| bh=pPXq/Duvy1sn+wseb+RkBCXNVAcq2TdS1+vHWqffvQI=; | ||
| fh=Y4RBdMh5b9UVrCPYtOvVi84uJ9kxdyNx270nS+Kf04s=; | ||
| b=B0Vi4LqKqJJEIYNqw6D9SyIbJ+u7Ije9FdzwaG7sdXNX2lhyVzmW3y/o64V5RHBjxL | ||
| irve8ius/zL7v0MpDiR0bhzmt8xOfW8V7Ij8awDWouDqgQIAyZNnx/D39iYhCSaUqjI4 | ||
| B86Mjl9oRWhpX1poAtjx9AEUlaXebC7+cdQIbElvDZe5k6Ea6AciW/7DyierGHOr3njR | ||
| NKdBYDo0KtD/jOEbRFiFrAqj/Vj20X0hUoLjQhgIfe+31BgEmvO3g3Dqd6bs8F3xFCAQ | ||
| ouOpSkn5kR1kCXQqr3U4iffHcUUPfI9mH5cvMEsPe/IgvkdXlCgYaCA1FzALVqDFJKiN | ||
| F7ig==; | ||
| dara=google.com | ||
| ARC-Authentication-Results: i=1; mx.google.com; arc=none | ||
| DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; | ||
| d=nammayatri.in; s=google; t=1774262028; x=1774866828; dara=google.com; | ||
| h=to:subject:message-id:date:from:mime-version:from:to:cc:subject | ||
| :date:message-id:reply-to; | ||
| bh=pPXq/Duvy1sn+wseb+RkBCXNVAcq2TdS1+vHWqffvQI=; | ||
| b=QMZDAwFkzfGe0MNy+D7jmPh/hiVLezP8e7t7qJUh354Rt7qyg1FPq1mJ9PX4VAPNA7 | ||
| PagxxZHqVPmGOAgnQJUtJCJvEPzaW/hSigi3Iy8njBGmYGoY3AXZ3pqS5lbm4nzV+4zD | ||
| W0HXEuZ1QuSpmEQDVtlXNNdCwr4gEJKhXJc+ROpl1LuaaTvBQOqzBMx4bA5EW0efcXZ4 | ||
| C13zwvbIKHZBsJkZzZGPN795ckljfxUiwGHaMLYlup7VCaCq8Xq/VbLxSRvhXpoYFCCk | ||
| 6Cb9HkRvCPfw74Pau+kAg1RKejFkr2C6gAzsGCUZugGw36D9karXE0IZFCQti+DSX4zJ | ||
| Koqw== | ||
| X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; | ||
| d=1e100.net; s=20251104; t=1774262028; x=1774866828; | ||
| h=to:subject:message-id:date:from:mime-version:x-gm-gg | ||
| :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; | ||
| bh=pPXq/Duvy1sn+wseb+RkBCXNVAcq2TdS1+vHWqffvQI=; | ||
| b=DBQHQN1pfulXXMAcDSNq4/GZ9DCsz7Kb4TR34dkedXFypu2Hn1iOKX6FCxxJ+pl2PV | ||
| nQUgq+bmvn5wU5zy1qhtZ4eXpYFNQCHIKFt70y+6344xPVKsZ56sto9S0HGh5YbOIPbG | ||
| +w23RNyA9P+NpZu0bhmcjiISQ5gWvLBK7BiWetheun+ulJCdvVEUJhrOwjDCbIh+VLTL | ||
| u7yQkdJo/TG/fnOU1ZSgONQXEJhIiN6a3aAzyURkbVw9lrTDd8XPuSypA2Na2NIfzUn1 | ||
| 0LK9CPT0stDWuFOgjOHDm79Mot6PcNDdS1/P7dovuqPytTnkpK/XW/vwq8clQbUz3JK2 | ||
| BRRA== | ||
| X-Gm-Message-State: AOJu0YxwuE/YvWEMYOrxQ1OYNmQQjplCE4+3bJXugR4YazM87E/weOnh | ||
| d2y/1ei2V1frd41f3M/9joChXdjg+iFeWefbCYEiLtD2InLpbYQZt8uBPIe+oor3ZOiprHHOh+X | ||
| itNww2NidakgiMUYH6Lo0lVGSGDc/flW68BWzeFMGZCx5YY2G8W3idHc= | ||
| X-Gm-Gg: ATEYQzwWWQQG/2RvVj12HJiNMRIbzT09EwH7gjhVpVddxvqgGCxXOZdWwe10pqOMvJB | ||
| PYj+uXmFGZpeowQGL1QushrY2t/tI34Fp3wl43q5Pw9Z5BVbg70VlIkn0RtMaIH9BnAnDVVhVHA | ||
| s3QWVOTtWaxd2T/lUNkC4xPCKmTEtUqBiesAqqhJs0wXDMDogdhhBjb6YqBxEqeScExbxfEOlhU | ||
| 2wXOEc/LB4sbMC0lBaQKHD87jAlFiBvTLqSR5bJLyNceDZyyhaAhpy6OoVRkyajzGiFMA7s5/C1 | ||
| k6kU6teu962pN5KHy8Xd6m2crkE+WpD5IgcdJePj7pA24+bgj7ET6CCuXUVRMTJ+ZZYmsww= | ||
| X-Received: by 2002:a05:6402:4304:b0:66a:11c9:f6ac with SMTP id | ||
| 4fb4d7f45d1cf-66a11c9fea6mr266917a12.8.1774262027951; Mon, 23 Mar 2026 | ||
| 03:33:47 -0700 (PDT) | ||
| MIME-Version: 1.0 | ||
| From: Kavyashree S <kavya@nammayatri.in> | ||
| Date: Mon, 23 Mar 2026 16:03:36 +0530 | ||
| X-Gm-Features: AQROBzAnPkNdaI2yZQ1qX-voLBd-t3QhcwI69W86Zf4MR9uvW0BYnrygpFwb8zM | ||
| Message-ID: <CAGbM3Fo5kGtc=fMw7uPDAdYEWJsC-fzy=ahnqxCwrWqxGk+Lyg@mail.gmail.com> | ||
| Subject: Settlement Report | ||
| To: imap8182@gmail.com | ||
| Content-Type: multipart/mixed; boundary="000000000000eae33a064dae91b7" |
There was a problem hiding this comment.
Same PII concern and potential duplication.
This fixture has the same PII exposure issue as the other debug-raw-mime.txt file (real email addresses, IPs, server names). Additionally, this appears to be nearly identical to test-settlement-parser/output/billdesk/debug-raw-mime.txt — consider whether both fixtures are necessary or if one could be removed to reduce maintenance burden.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test-settlement-parser/output/debug-raw-mime.txt` around lines 1 - 104, This
fixture exposes real PII (e.g., the "From: Kavyashree S <kavya@nammayatri.in>",
IPs and server names) and appears duplicate of the other fixture containing
"Subject: Settlement Report" and similar Message-ID; redact or replace all PII
in this fixture (replace email addresses, IPs, hostnames, and full Message-ID
with synthetic placeholders) and then remove or consolidate duplicates by
keeping only one redacted fixture (the one containing "Subject: Settlement
Report" / the Message-ID shown) to avoid maintenance overhead.
Comment on lines
+121
to
+134
| -- | Extract the attachment filename from raw MIME, fallback to "email-attachment.csv" | ||
| extractAttachmentName :: Maybe Text -> FilePath | ||
| extractAttachmentName Nothing = "email-attachment.csv" | ||
| extractAttachmentName (Just rawMime) = | ||
| let lns = T.lines rawMime | ||
| filenameLine = find (\l -> "filename=" `T.isInfixOf` T.toLower l) lns | ||
| in case filenameLine of | ||
| Just l -> | ||
| let stripped = T.strip l | ||
| -- Extract value after filename=, remove quotes | ||
| afterEq = T.dropWhile (/= '=') stripped | ||
| name = T.filter (/= '"') (T.drop 1 afterEq) | ||
| in if T.null name then "email-attachment.csv" else T.unpack (T.strip name) | ||
| Nothing -> "email-attachment.csv" |
There was a problem hiding this comment.
extractAttachmentName may extract wrong filename from multi-part emails.
This function scans all lines for any filename= header, but extractCsvFromMime (in Email.hs) specifically looks for text/csv or application/csv content types. In a multi-part email with HTML + CSV + PDF attachments, extractAttachmentName could match the PDF's filename while extractCsvFromMime correctly extracts the CSV content.
Consider aligning the logic to find the filename from the same MIME section that contains text/csv.
🔧 Suggested approach
-- Option 1: Export a helper from Email.hs that returns both filename and content
-- Option 2: Search specifically for filename in text/csv section
extractAttachmentName :: Maybe Text -> FilePath
extractAttachmentName Nothing = "email-attachment.csv"
extractAttachmentName (Just rawMime) =
let lns = T.lines rawMime
-- Find the line with text/csv content-type, then look for filename nearby
csvIdx = findIndex (\l -> "text/csv" `T.isInfixOf` T.toLower l) lns
in case csvIdx of
Just idx ->
let nearby = take 5 (drop (max 0 (idx - 2)) lns)
in extractFilenameFrom nearby
Nothing -> "email-attachment.csv"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test-settlement-parser/src/Main.hs` around lines 121 - 134,
extractAttachmentName currently scans all lines for any "filename=" which can
return a non-CSV attachment name; change it to locate the same MIME part that
extractCsvFromMime targets (the "text/csv" or "application/csv" Content-Type)
and extract the filename only from that part. Specifically: in
extractAttachmentName, when given Just rawMime, split into lines (T.lines), find
the index of a line containing "text/csv" or "application/csv" (use T.toLower
for case-insensitive match), collect a small window of nearby header lines
(e.g., idx-2 to idx+2) and search that window for "filename=", falling back to
"email-attachment.csv" if none found; this keeps extractAttachmentName aligned
with extractCsvFromMime's selection logic.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
lib/mobility-core/src/Kernel/External/Settlement/Sources/SFTP.hs (1)
39-42: Predictable temp file path could enable symlink attacks.Using
tmpDir </> T.unpack fileNamecreates a predictable path. A malicious local user could pre-create a symlink at that location to overwrite arbitrary files or read the downloaded content.Consider using
openTempFileorcreateTempDirectoryto generate a unique, unpredictable path.♻️ Proposed fix using unique temp file
+import System.IO (openTempFile, hClose) + tmpDir <- liftIO getTemporaryDirectory - let localPath = tmpDir </> T.unpack fileName + (localPath, handle) <- liftIO $ openTempFile tmpDir "settlement_" + liftIO $ hClose handle -- Close handle; scp will write to the path🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/mobility-core/src/Kernel/External/Settlement/Sources/SFTP.hs` around lines 39 - 42, The current use of localPath = tmpDir </> T.unpack fileName creates a predictable temp path and risks symlink attacks; change the download handling in this module to create a unique, secure temporary file or directory instead (use openTempFile or createTempDirectory) and write the incoming data into that unpredictable path rather than localPath; update any code that references localPath (and related values derived from it such as remoteFile, scpTarget, portArg where applicable) to use the new temp file path, ensure you close and atomically move or set restrictive permissions on the temp file and perform proper cleanup on success or error.
🤖 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/Settlement/Sources/SFTP.hs`:
- Around line 50-53: The ExitSuccess branch currently reads the temp file with
LBS.readFile and then calls removeFile, but if LBS.readFile throws the temp file
is never removed; update the ExitSuccess block to ensure removeFile runs even on
exceptions by wrapping the read in a finally/bracket-style cleanup (e.g., use
liftIO $ bracket (pure localPath) removeFile LBS.readFile or use finally to call
removeFile after attempting LBS.readFile) so that localPath is always removed
and the function still returns Right contents on success or propagates the error
on failure.
- Around line 43-48: The call that constructs the scp command using
readProcessWithExitCode exposes decryptedPassword in process args (see
readProcessWithExitCode, decryptedPassword, scpTarget, localPath); change to
invoke sshpass with the -e flag and pass the password via the environment
instead of the CLI, e.g., build a CreateProcess/Proc style invocation (use proc
and readCreateProcessWithExitCode) and set the environment entry "SSHPASS" to
T.unpack decryptedPassword so the password is not present in argv, or
alternatively switch to key-based auth; ensure the new CreateProcess uses the
same portArg/scpTarget/localPath values and that liftIO still executes the
process.
---
Nitpick comments:
In `@lib/mobility-core/src/Kernel/External/Settlement/Sources/SFTP.hs`:
- Around line 39-42: The current use of localPath = tmpDir </> T.unpack fileName
creates a predictable temp path and risks symlink attacks; change the download
handling in this module to create a unique, secure temporary file or directory
instead (use openTempFile or createTempDirectory) and write the incoming data
into that unpredictable path rather than localPath; update any code that
references localPath (and related values derived from it such as remoteFile,
scpTarget, portArg where applicable) to use the new temp file path, ensure you
close and atomically move or set restrictive permissions on the temp file and
perform proper cleanup on success or error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b386e780-ac8b-43cc-98c8-357cb52d022f
📒 Files selected for processing (4)
lib/mobility-core/src/Kernel/External/Settlement/Interface.hslib/mobility-core/src/Kernel/External/Settlement/Sources/SFTP.hslib/mobility-core/src/Kernel/External/Settlement/Types.hstest-settlement-parser/test-settlement-parser.cabal
🚧 Files skipped from review as they are similar to previous changes (2)
- test-settlement-parser/test-settlement-parser.cabal
- lib/mobility-core/src/Kernel/External/Settlement/Types.hs
Comment on lines
+43
to
+48
| (exitCode, _stdout, stderr) <- | ||
| liftIO $ | ||
| readProcessWithExitCode | ||
| "sshpass" | ||
| ["-p", T.unpack decryptedPassword, "scp", "-P", portArg, "-o", "StrictHostKeyChecking=no", scpTarget, localPath] | ||
| "" |
There was a problem hiding this comment.
Security: Password exposed in process arguments.
Passing the decrypted password via -p command-line argument makes it visible to any user on the system via ps aux or /proc/<pid>/cmdline. This is a significant credential exposure risk, especially on shared hosts.
Consider using SSHPASS environment variable instead (sshpass supports -e flag to read from environment), or use SSH key-based authentication.
🔒 Proposed fix using environment variable
- (exitCode, _stdout, stderr) <-
- liftIO $
- readProcessWithExitCode
- "sshpass"
- ["-p", T.unpack decryptedPassword, "scp", "-P", portArg, "-o", "StrictHostKeyChecking=no", scpTarget, localPath]
- ""
+ (exitCode, _stdout, stderr) <- liftIO $ do
+ let env = [("SSHPASS", T.unpack decryptedPassword)]
+ args = ["-e", "scp", "-P", portArg, "-o", "StrictHostKeyChecking=no", scpTarget, localPath]
+ readCreateProcessWithExitCode ((proc "sshpass" args) { env = Just env }) ""Note: This requires importing System.Process (proc, readCreateProcessWithExitCode).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/mobility-core/src/Kernel/External/Settlement/Sources/SFTP.hs` around
lines 43 - 48, The call that constructs the scp command using
readProcessWithExitCode exposes decryptedPassword in process args (see
readProcessWithExitCode, decryptedPassword, scpTarget, localPath); change to
invoke sshpass with the -e flag and pass the password via the environment
instead of the CLI, e.g., build a CreateProcess/Proc style invocation (use proc
and readCreateProcessWithExitCode) and set the environment entry "SSHPASS" to
T.unpack decryptedPassword so the password is not present in argv, or
alternatively switch to key-based auth; ensure the new CreateProcess uses the
same portArg/scpTarget/localPath values and that liftIO still executes the
process.
Comment on lines
+50
to
+53
| ExitSuccess -> do | ||
| contents <- liftIO $ LBS.readFile localPath | ||
| liftIO $ removeFile localPath | ||
| pure $ Right contents |
There was a problem hiding this comment.
Resource leak: Temp file not cleaned up on read failure.
If LBS.readFile throws an exception (e.g., permission denied, disk error), removeFile is never called and the temp file containing potentially sensitive settlement data remains on disk.
🛡️ Proposed fix using bracket or finally
+import Control.Exception (bracket, finally)
+
ExitSuccess -> do
- contents <- liftIO $ LBS.readFile localPath
- liftIO $ removeFile localPath
- pure $ Right contents
+ contents <- liftIO $ LBS.readFile localPath `finally` removeFile localPath
+ pure $ Right contentsOr use bracket for more explicit resource management:
liftIO $ bracket
(pure localPath)
removeFile
LBS.readFile🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/mobility-core/src/Kernel/External/Settlement/Sources/SFTP.hs` around
lines 50 - 53, The ExitSuccess branch currently reads the temp file with
LBS.readFile and then calls removeFile, but if LBS.readFile throws the temp file
is never removed; update the ExitSuccess block to ensure removeFile runs even on
exceptions by wrapping the read in a finally/bracket-style cleanup (e.g., use
liftIO $ bracket (pure localPath) removeFile LBS.readFile or use finally to call
removeFile after attempting LBS.readFile) so that localPath is always removed
and the function still returns Right contents on success or propagates the error
on failure.
3fa0bd4 to
72adc5c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test-settlement-parser/src/Main.hs (1)
62-78: Consider making IMAP server configurable for broader testing.The IMAP host (
imap.gmail.com) and port (993) are hardcoded. For a test tool, this works for Gmail accounts, but consider making these configurable via additional CLI arguments or environment variables if testing with other email providers is needed in the future.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test-settlement-parser/src/Main.hs` around lines 62 - 78, The IMAP host and port are hardcoded in testEmailFetch (imapHost and imapPort in the EmailConfig), make them configurable by reading from environment variables or additional CLI arguments and falling back to the current defaults ("imap.gmail.com" and 993); update the testEmailFetch signature or the caller to accept imapHost/imapPort (or fetch them via getEnv) and wire those values into EmailConfig so different providers can be used without changing the source.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test-settlement-parser/src/Main.hs`:
- Around line 91-93: The error message in the Left err branch inside
fetchSettlementFileWithPlainPassword is misleading (it says "Parse error" while
this function performs the IMAP fetch); change the log to reflect a fetch/IMAP
error (e.g., "Failed to fetch settlement file" or "IMAP fetch error") and
include the actual err details in the message, leaving the existing liftIO
exitFailure behavior intact so failures still terminate as before.
- Line 7: The import line "qualified Data.Text.Encoding as TE" is unused; remove
that import from Main.hs's import list (delete the "import qualified
Data.Text.Encoding as TE" line) and ensure there are no remaining references to
the TE symbol in functions like main or any helper functions before committing
the change.
---
Nitpick comments:
In `@test-settlement-parser/src/Main.hs`:
- Around line 62-78: The IMAP host and port are hardcoded in testEmailFetch
(imapHost and imapPort in the EmailConfig), make them configurable by reading
from environment variables or additional CLI arguments and falling back to the
current defaults ("imap.gmail.com" and 993); update the testEmailFetch signature
or the caller to accept imapHost/imapPort (or fetch them via getEnv) and wire
those values into EmailConfig so different providers can be used without
changing the source.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1b3bcc6a-0aa6-4b61-bf20-aa4ae3ebb54e
📒 Files selected for processing (5)
lib/mobility-core/src/Kernel/External/Settlement/Interface.hslib/mobility-core/src/Kernel/External/Settlement/Sources/SFTP.hslib/mobility-core/src/Kernel/External/Settlement/Types.hstest-settlement-parser/src/Main.hstest-settlement-parser/test-settlement-parser.cabal
🚧 Files skipped from review as they are similar to previous changes (2)
- test-settlement-parser/test-settlement-parser.cabal
- lib/mobility-core/src/Kernel/External/Settlement/Interface.hs
| import Data.List (isSuffixOf) | ||
| import Data.List (find, isSuffixOf) | ||
| import qualified Data.Text as T | ||
| import qualified Data.Text.Encoding as TE |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if Data.Text.Encoding (TE) is used anywhere in this file
rg -n "TE\." test-settlement-parser/src/Main.hsRepository: nammayatri/shared-kernel
Length of output: 50
🏁 Script executed:
# View the file and check for the import, and search for any Data.Text.Encoding usage
head -20 test-settlement-parser/src/Main.hs && echo "---" && rg "encodeUtf8|decodeUtf8|Data\.Text\.Encoding" test-settlement-parser/src/Main.hsRepository: nammayatri/shared-kernel
Length of output: 948
Remove unused import Data.Text.Encoding.
The import qualified Data.Text.Encoding as TE at line 7 is not used anywhere in the file. Remove it to keep imports clean and maintain compliance with the guideline of avoiding unnecessary dependencies.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test-settlement-parser/src/Main.hs` at line 7, The import line "qualified
Data.Text.Encoding as TE" is unused; remove that import from Main.hs's import
list (delete the "import qualified Data.Text.Encoding as TE" line) and ensure
there are no remaining references to the TE symbol in functions like main or any
helper functions before committing the change.
Comment on lines
+91
to
+93
| Left err -> do | ||
| liftIO $ TIO.putStrLn $ "Parse error: " <> err | ||
| liftIO exitFailure |
There was a problem hiding this comment.
Misleading error message.
The error message says "Parse error" but fetchSettlementFileWithPlainPassword performs IMAP fetching, not parsing. The actual parsing happens later at line 106. This could confuse users debugging issues.
📝 Suggested fix
Left err -> do
- liftIO $ TIO.putStrLn $ "Parse error: " <> err
+ liftIO $ TIO.putStrLn $ "Fetch error: " <> err
liftIO exitFailure📝 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
| Left err -> do | |
| liftIO $ TIO.putStrLn $ "Parse error: " <> err | |
| liftIO exitFailure | |
| Left err -> do | |
| liftIO $ TIO.putStrLn $ "Fetch error: " <> err | |
| liftIO exitFailure |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test-settlement-parser/src/Main.hs` around lines 91 - 93, The error message
in the Left err branch inside fetchSettlementFileWithPlainPassword is misleading
(it says "Parse error" while this function performs the IMAP fetch); change the
log to reflect a fetch/IMAP error (e.g., "Failed to fetch settlement file" or
"IMAP fetch error") and include the actual err details in the message, leaving
the existing liftIO exitFailure behavior intact so failures still terminate as
before.
72adc5c to
779d100
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
test-settlement-parser/README.md (1)
178-184: Add language specifier to fenced code block.The code block at line 180 is missing a language identifier, which improves syntax highlighting and accessibility.
📝 Proposed fix
Example for `billdesk`: -``` +```text samples/billdesk/billdesk_settlement_report_sample.csv # downloaded CSV output/billdesk/billdesk_settlement_report_sample.csv.parsed.txt # parsed output output/billdesk/debug-raw-mime.txt # raw MIME</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@test-settlement-parser/README.mdaround lines 178 - 184, Update the fenced
code block under the "Example forbilldesk" section so it includes a language
specifier (use "text") — locate the triple-backtick block containing the three
sample paths (samples/billdesk/... and output/billdesk/...) and change the
opening fence to ```text to enable proper syntax highlighting and accessibility.</details> </blockquote></details> <details> <summary>lib/mobility-core/src/Kernel/External/Settlement/Types.hs (2)</summary><blockquote> `76-79`: **Document the `Text` parameter in `SFTPSourceConfig`.** The second parameter to `SFTPSourceConfig` (line 77) represents the file pattern but this isn't obvious from the type definition. Consider using a newtype or adding a comment. ```haskell data SettlementSourceConfig = SFTPSourceConfig SFTPConfig Text -- ^ SFTP config and file pattern/name to fetch | EmailSourceConfig EmailConfig ``` <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@lib/mobility-core/src/Kernel/External/Settlement/Types.hs` around lines 76 - 79, The variant SFTPSourceConfig in the SettlementSourceConfig sum type has an undocumented Text parameter (second field) which is the file pattern/name to fetch; update the declaration for SettlementSourceConfig by either introducing a newtype (e.g. newtype FilePattern = FilePattern Text and use SFTPSourceConfig SFTPConfig FilePattern) or at minimum add an inline Haddock comment on the Text field to document it (referencing SettlementSourceConfig, SFTPSourceConfig, SFTPConfig and Text) so callers immediately know the meaning of that parameter. ``` </details> --- `44-51`: **`merchantId` and `merchantOperatingCityId` fields are unused in the settlement parsing logic.** Both `parsePaymentSettlementCsv` and `parsePayoutSettlementCsv` only access `config.service`. The `merchantId` and `merchantOperatingCityId` fields are never read, and test code only populates them with dummy values. If these are reserved for future use, consider documenting the intent. Otherwise, removing them would simplify the API. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@lib/mobility-core/src/Kernel/External/Settlement/Types.hs` around lines 44 - 51, The SettlementParseConfig type declares unused fields merchantId and merchantOperatingCityId which are never read by parsePaymentSettlementCsv or parsePayoutSettlementCsv; remove these two fields from the SettlementParseConfig declaration and update all construction sites (tests and callers that instantiate SettlementParseConfig) to stop passing dummy values, or if they are intended for future use add a one-line comment above SettlementParseConfig documenting their reserved purpose and keep them as Maybe Text to indicate optionality; ensure references to SettlementParseConfig in parsePaymentSettlementCsv and parsePayoutSettlementCsv remain unchanged other than adjusted record construction. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In@lib/mobility-core/src/Kernel/External/Settlement/Types.hs:
- Around line 76-79: The variant SFTPSourceConfig in the SettlementSourceConfig
sum type has an undocumented Text parameter (second field) which is the file
pattern/name to fetch; update the declaration for SettlementSourceConfig by
either introducing a newtype (e.g. newtype FilePattern = FilePattern Text and
use SFTPSourceConfig SFTPConfig FilePattern) or at minimum add an inline Haddock
comment on the Text field to document it (referencing SettlementSourceConfig,
SFTPSourceConfig, SFTPConfig and Text) so callers immediately know the meaning
of that parameter.- Around line 44-51: The SettlementParseConfig type declares unused fields
merchantId and merchantOperatingCityId which are never read by
parsePaymentSettlementCsv or parsePayoutSettlementCsv; remove these two fields
from the SettlementParseConfig declaration and update all construction sites
(tests and callers that instantiate SettlementParseConfig) to stop passing dummy
values, or if they are intended for future use add a one-line comment above
SettlementParseConfig documenting their reserved purpose and keep them as Maybe
Text to indicate optionality; ensure references to SettlementParseConfig in
parsePaymentSettlementCsv and parsePayoutSettlementCsv remain unchanged other
than adjusted record construction.In
@test-settlement-parser/README.md:
- Around line 178-184: Update the fenced code block under the "Example for
billdesk" section so it includes a language specifier (use "text") — locate
the triple-backtick block containing the three sample paths
(samples/billdesk/... and output/billdesk/...) and change the opening fence to
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID:
acae8423-1bad-4de9-985b-ebc5c5ff978e⛔ Files ignored due to path filters (3)
test-settlement-parser/output/email-attachment.csvis excluded by!**/*.csvtest-settlement-parser/samples/billdesk/billdesk_settlement_report_sample.csvis excluded by!**/*.csvtest-settlement-parser/samples/billdesk/email-attachment.csvis excluded by!**/*.csv📒 Files selected for processing (16)
lib/mobility-core/mobility-core.caballib/mobility-core/src/Kernel/External/Settlement/Interface.hslib/mobility-core/src/Kernel/External/Settlement/Sources/Email.hslib/mobility-core/src/Kernel/External/Settlement/Sources/SFTP.hslib/mobility-core/src/Kernel/External/Settlement/Sources/Untitledlib/mobility-core/src/Kernel/External/Settlement/Types.hslib/mobility-core/test/app/Main.hslib/mobility-core/test/src/SettlementEmail.hstest-settlement-parser/README.mdtest-settlement-parser/output/billdesk/billdesk_settlement_report_sample.csv.parsed.txttest-settlement-parser/output/billdesk/debug-raw-mime.txttest-settlement-parser/output/billdesk/email-attachment.csv.parsed.txttest-settlement-parser/output/debug-raw-mime.txttest-settlement-parser/shims/Kernel/External/Encryption.hstest-settlement-parser/src/Main.hstest-settlement-parser/test-settlement-parser.cabal✅ Files skipped from review due to trivial changes (8)
- lib/mobility-core/test/app/Main.hs
- lib/mobility-core/src/Kernel/External/Settlement/Sources/Untitled
- test-settlement-parser/output/debug-raw-mime.txt
- test-settlement-parser/output/billdesk/billdesk_settlement_report_sample.csv.parsed.txt
- test-settlement-parser/output/billdesk/debug-raw-mime.txt
- test-settlement-parser/output/billdesk/email-attachment.csv.parsed.txt
- test-settlement-parser/shims/Kernel/External/Encryption.hs
- test-settlement-parser/test-settlement-parser.cabal
🚧 Files skipped from review as they are similar to previous changes (3)
- lib/mobility-core/mobility-core.cabal
- lib/mobility-core/src/Kernel/External/Settlement/Interface.hs
- lib/mobility-core/src/Kernel/External/Settlement/Sources/Email.hs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/mobility-core/test/src/SettlementEmail.hs (1)
51-83: Tighten assertions to exact outputs to reduce false positives.
isInfixOfand genericLeft _checks can pass even when parsing regresses. Prefer exact equality for decoded CSV and for the expected error text.Suggested assertion tightening
case extractCsvFromMime mime of Left err -> assertFailure $ "Expected CSV content but got error: " <> toString err Right bs -> do let csv = TE.decodeUtf8 (LBS.toStrict bs) - assertBool "should contain txn_id header" ("txn_id" `isInfixOf` csv) - assertBool "should contain T001 row" ("T001" `isInfixOf` csv), + csv @?= "txn_id,amount,status\nT001,100.00,settled\nT002,200.50,settled", @@ case extractCsvFromMime mime of Left err -> assertFailure $ "Expected CSV content but got error: " <> toString err Right bs -> do let csv = TE.decodeUtf8 (LBS.toStrict bs) - assertBool "should contain hello,world" ("hello,world" `isInfixOf` csv), + csv @?= "hello,world", @@ - case extractCsvFromMime mime of - Left _ -> pure () - Right _ -> assertFailure "Expected error for non-CSV email" + extractCsvFromMime mime @?= Left "No CSV attachment found in email"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/mobility-core/test/src/SettlementEmail.hs` around lines 51 - 83, The tests use fuzzy assertions (isInfixOf and Left _) which hide regressions; update the three testCase blocks that call extractCsvFromMime to assert exact outputs: for the first CSV test replace the isInfixOf checks with a single assertEqual comparing the decoded csv string to the full expected CSV text (including headers and rows); for the base64 CSV test replace the isInfixOf with assertEqual comparing the decoded csv to "hello,world\n" (or the exact expected bytes decoded) and for the non-CSV test match Left err and assertEqual (toString err) to the exact expected error message returned by extractCsvFromMime instead of using Left _ so the test fails if the parser returns a different error or succeeds. Ensure you reference extractCsvFromMime and the testCase blocks around the joinMimeLines inputs when making the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/mobility-core/test/src/SettlementEmail.hs`:
- Around line 51-83: The tests use fuzzy assertions (isInfixOf and Left _) which
hide regressions; update the three testCase blocks that call extractCsvFromMime
to assert exact outputs: for the first CSV test replace the isInfixOf checks
with a single assertEqual comparing the decoded csv string to the full expected
CSV text (including headers and rows); for the base64 CSV test replace the
isInfixOf with assertEqual comparing the decoded csv to "hello,world\n" (or the
exact expected bytes decoded) and for the non-CSV test match Left err and
assertEqual (toString err) to the exact expected error message returned by
extractCsvFromMime instead of using Left _ so the test fails if the parser
returns a different error or succeeds. Ensure you reference extractCsvFromMime
and the testCase blocks around the joinMimeLines inputs when making the changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 32945ae3-5e90-4843-a32e-c04be497ae29
📒 Files selected for processing (1)
lib/mobility-core/test/src/SettlementEmail.hs
kavya-shree-s
force-pushed
the
Backend-feat-parsing-report
branch
2 times, most recently
from
f1748de to
efe84f3
Compare
kavya-shree-s
changed the title
kavya-shree-s
force-pushed
the
Backend-feat-parsing-report
branch
4 times, most recently
from
131c4c7 to
9633104
Compare
kavya-shree-s
force-pushed
the
Backend-feat-parsing-report
branch
from
9633104 to
894d856
Compare
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
New Features
Documentation
Tests