WEB-112: Sanitize GSIM fields before calculateLoanSchedule to avoid NPE#3569
WEB-112: Sanitize GSIM fields before calculateLoanSchedule to avoid NPE#3569deepaksoni47 wants to merge 1 commit into
Conversation
|
Note
|
| Layer / File(s) | Summary |
|---|---|
Sanitization Helper src/app/loans/loans.service.ts |
New private sanitizeLoanPayload function recursively deep-copies the payload and removes forbidden GSIM keys (linkAccountId, linkAccountOwnerId, linkAccountOwnerName, linkedSavingsAccount) from objects and arrays at any nesting depth. |
Service Integration src/app/loans/loans.service.ts |
calculateLoanSchedule method now sanitizes the input payload via the helper before posting to /loans?command=calculateLoanSchedule. |
Test Suite and Cases src/app/loans/loans.service.spec.ts |
Test suite sets up HttpClientTestingModule and mocked dependencies. Tests verify forbidden GSIM fields are removed from top-level objects, nested objects, and arrays, while allowed fields and primitive values are preserved in the request body. |
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title 'WEB-112: Sanitize GSIM fields before calculateLoanSchedule to avoid NPE' directly matches the main change: sanitizing GSIM-related fields in the loan schedule calculation to prevent null pointer exceptions. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
Tip
💬 Introducing Slack Agent: The best way for teams to turn conversations into code.
Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
- Generate code and open pull requests
- Plan features and break down work
- Investigate incidents and troubleshoot customer tickets together
- Automate recurring tasks and respond to alerts with triggers
- Summarize progress and report instantly
Built for teams:
- Shared memory across your entire org—no repeating context
- Per-thread sandboxes to safely plan and execute work
- Governance built-in—scoped access, auditability, and budget controls
One agent for your entire SDLC. Right inside Slack.
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 help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/app/loans/loans.service.ts (1)
717-767: ⚡ Quick winSimplify
sanitizeLoanPayloadwith lodash and a single recursion.The current implementation has a parallel src→dst traversal with two object-construction sites (object branch and array branch), two duplicated checks for
null/undefined/typeof !== 'object', and an inlineforbiddenKeysSet rebuilt on every call. Per the repo guideline to "re-use the lodash and moment libraries efficiently", and to reduce cognitive load, a single recursive walker (or_.cloneDeepWith+_.omit) expresses the intent more clearly. PullingforbiddenKeysto a class/file-levelreadonlyconstant also avoids re-allocation per call.Note: like the current code, any non-plain-object instance (e.g.
Date, class instances) would be coerced to a plain{}. In practice the schedule payload is built with formatted-date strings, so this is acceptable — but worth keeping in mind if the calling code ever passes rawDatevalues.♻️ Proposed simplification
+ private static readonly FORBIDDEN_LOAN_PAYLOAD_KEYS: ReadonlySet<string> = new Set([ + 'linkAccountId', + 'linkAccountOwnerId', + 'linkAccountOwnerName', + 'linkedSavingsAccount' + ]); + /** * Remove potentially unsafe GSIM linkage fields from payload recursively. * This is a defensive workaround until backend validation is fixed in Fineract. */ private sanitizeLoanPayload(obj: any): any { - if (obj === null || obj === undefined) return obj; - // Primitive types - if (typeof obj !== 'object') return obj; - - // Work on a deep copy to avoid mutating caller's data - const copy = Array.isArray(obj) ? [] : {}; - - const forbiddenKeys = new Set([ - 'linkAccountId', - 'linkAccountOwnerId', - 'linkAccountOwnerName', - 'linkedSavingsAccount' - ]); - - const recurse = (src: any, dst: any) => { - if (src === null || src === undefined) return src; - if (typeof src !== 'object') return src; - if (Array.isArray(src)) { - for (let i = 0; i < src.length; i++) { - const val = src[i]; - if (typeof val === 'object' && val !== null) { - const child = Array.isArray(val) ? [] : {}; - dst.push(recurse(val, child)); - } else { - dst.push(val); - } - } - return dst; - } - - for (const key of Object.keys(src)) { - if (forbiddenKeys.has(key)) { - // skip this key - continue; - } - const val = src[key]; - if (val === null || val === undefined) { - dst[key] = val; - } else if (typeof val === 'object') { - dst[key] = Array.isArray(val) ? [] : {}; - recurse(val, dst[key]); - } else { - dst[key] = val; - } - } - return dst; - }; - - return recurse(obj, copy); + if (obj === null || typeof obj !== 'object') return obj; + if (Array.isArray(obj)) return obj.map((item) => this.sanitizeLoanPayload(item)); + const out: Record<string, any> = {}; + for (const key of Object.keys(obj)) { + if (LoansService.FORBIDDEN_LOAN_PAYLOAD_KEYS.has(key)) continue; + out[key] = this.sanitizeLoanPayload(obj[key]); + } + return out; }Equivalent lodash variant (if preferred):
import { cloneDeepWith, isPlainObject, omit } from 'lodash-es'; private sanitizeLoanPayload(obj: any): any { return cloneDeepWith(obj, (value) => isPlainObject(value) ? cloneDeepWith(omit(value as object, [...LoansService.FORBIDDEN_LOAN_PAYLOAD_KEYS]), (v) => (isPlainObject(v) ? this.sanitizeLoanPayload(v) : undefined)) : undefined ); }As per coding guidelines: "Re-use the lodash and moment libraries efficiently."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/loans/loans.service.ts` around lines 717 - 767, The sanitizeLoanPayload function duplicates traversal logic, rebuilds forbiddenKeys on every call, and repeats null/type checks; replace it with a single recursive walker (or use lodash cloneDeepWith + omit) and move the forbiddenKeys Set to a class/file-level readonly constant (e.g. LoansService.FORBIDDEN_LOAN_PAYLOAD_KEYS) so it isn’t reallocated per call; update sanitizeLoanPayload to perform one traversal that omits any key in that constant for plain objects and correctly handles arrays by recursing into elements, removing the duplicated object/array construction and redundant typeof/null checks.src/app/loans/loans.service.spec.ts (2)
38-70: ⚡ Quick winStrengthen sanitization coverage: assert all four forbidden keys and payload immutability.
The test only exercises
linkAccountIdandlinkAccountOwnerId. The service explicitly forbids two more keys (linkAccountOwnerName,linkedSavingsAccount) which are currently uncovered, so a future change that drops one of them from theSetwould go undetected. The helper also documents that it must not mutate the caller's payload — that invariant is also untested.🧪 Suggested additions
it('should sanitize GSIM fields before posting calculateLoanSchedule', () => { const payload = { principal: 1000, linkAccountId: 42, + linkAccountOwnerName: 'Alice', + linkedSavingsAccount: { id: 5 }, nested: { linkAccountOwnerId: 7, + linkAccountOwnerName: 'Bob', ok: 'keep' }, members: [ { id: 1, linkAccountId: 99 }, - { id: 2, name: 'foo' } + { id: 2, name: 'foo', linkedSavingsAccount: { id: 9 } } ] } as any; + const original = JSON.parse(JSON.stringify(payload)); service.calculateLoanSchedule(payload).subscribe(() => {}); const req = httpMock.expectOne('/loans?command=calculateLoanSchedule'); expect(req.request.method).toBe('POST'); const body = req.request.body; - // Forbidden keys should be removed at top level and nested locations + // All forbidden keys should be removed at every depth expect(body.linkAccountId).toBeUndefined(); + expect(body.linkAccountOwnerName).toBeUndefined(); + expect(body.linkedSavingsAccount).toBeUndefined(); expect(body.nested.linkAccountOwnerId).toBeUndefined(); + expect(body.nested.linkAccountOwnerName).toBeUndefined(); expect(body.members[0].linkAccountId).toBeUndefined(); + expect(body.members[1].linkedSavingsAccount).toBeUndefined(); // Allowed keys should be preserved expect(body.principal).toBe(1000); expect(body.nested.ok).toBe('keep'); expect(body.members[1].name).toBe('foo'); + // Original caller payload must remain unmutated + expect(payload).toEqual(original); + req.flush({}); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/loans/loans.service.spec.ts` around lines 38 - 70, The test for service.calculateLoanSchedule currently only checks removal of linkAccountId and linkAccountOwnerId; update the test to also include and assert removal of the other two forbidden keys linkAccountOwnerName and linkedSavingsAccount (in both top-level and nested/array locations as appropriate), and assert that the original payload object is not mutated (compare deep-equality or check original fields still exist on the input after calling calculateLoanSchedule) so the helper's immutability guarantee is covered.
2-2: 💤 Low valueUse
provideHttpClient()andprovideHttpClientTesting()instead of the deprecatedHttpClientTestingModule.
HttpClientTestingModulehas been deprecated since Angular 18 in favor of function-based providers. Angular 20.3.17 used in this project recommends usingprovideHttpClient()andprovideHttpClientTesting()in the providers array instead of importing the module. This is non-blocking but aligns the test file with current Angular testing best practices.♻️ Proposed change
-import { HttpClientTestingModule, HttpTestingController } from '@angular/common/http/testing'; +import { HttpTestingController, provideHttpClientTesting } from '@angular/common/http/testing'; +import { provideHttpClient } from '@angular/common/http'; @@ TestBed.configureTestingModule({ - imports: [HttpClientTestingModule], providers: [ LoansService, + provideHttpClient(), + provideHttpClientTesting(), { provide: SettingsService, useValue: mockSettings }, { provide: Dates, useValue: mockDates }] });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/loans/loans.service.spec.ts` at line 2, Replace the deprecated HttpClientTestingModule import and usage with the function-based providers: remove HttpClientTestingModule from the TestBed.configureTestingModule imports and instead add provideHttpClient() and provideHttpClientTesting() to the providers array; update top-level imports to use provideHttpClient (from `@angular/common/http`) and provideHttpClientTesting (from `@angular/common/http/testing`) and keep HttpTestingController usage unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/app/loans/loans.service.spec.ts`:
- Around line 38-70: The test for service.calculateLoanSchedule currently only
checks removal of linkAccountId and linkAccountOwnerId; update the test to also
include and assert removal of the other two forbidden keys linkAccountOwnerName
and linkedSavingsAccount (in both top-level and nested/array locations as
appropriate), and assert that the original payload object is not mutated
(compare deep-equality or check original fields still exist on the input after
calling calculateLoanSchedule) so the helper's immutability guarantee is
covered.
- Line 2: Replace the deprecated HttpClientTestingModule import and usage with
the function-based providers: remove HttpClientTestingModule from the
TestBed.configureTestingModule imports and instead add provideHttpClient() and
provideHttpClientTesting() to the providers array; update top-level imports to
use provideHttpClient (from `@angular/common/http`) and provideHttpClientTesting
(from `@angular/common/http/testing`) and keep HttpTestingController usage
unchanged.
In `@src/app/loans/loans.service.ts`:
- Around line 717-767: The sanitizeLoanPayload function duplicates traversal
logic, rebuilds forbiddenKeys on every call, and repeats null/type checks;
replace it with a single recursive walker (or use lodash cloneDeepWith + omit)
and move the forbiddenKeys Set to a class/file-level readonly constant (e.g.
LoansService.FORBIDDEN_LOAN_PAYLOAD_KEYS) so it isn’t reallocated per call;
update sanitizeLoanPayload to perform one traversal that omits any key in that
constant for plain objects and correctly handles arrays by recursing into
elements, removing the duplicated object/array construction and redundant
typeof/null checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3b842986-7780-4a9c-8025-ef0744ad67af
📒 Files selected for processing (2)
src/app/loans/loans.service.spec.tssrc/app/loans/loans.service.ts
- Add sanitizeLoanPayload method to LoansService to remove unsafe GSIM linkage fields (linkAccountId, linkAccountOwnerId, linkAccountOwnerName, linkedSavingsAccount) before posting to backend - Apply sanitization in calculateLoanSchedule endpoint to prevent payload from triggering NullPointerException in Fineract LoanScheduleCalculationPlatformServiceImpl - Add unit tests for payload sanitization to verify forbidden fields are removed and allowed fields are preserved - Fixes 500 Internal Server Error when generating repayment schedule for GLIM loans linked to GSIM accounts
deepaksoni47
force-pushed
the
WEB-112-fix-gsim-schedule-sanitization
branch
from
7a2b853 to
ca7060f
Compare
|
Addressed the CodeRabbit review comments and updated the implementation accordingly. Also re-ran tests and checks after the changes. |
| providedIn: 'root' | ||
| }) | ||
| export class LoansService { | ||
| private static readonly FORBIDDEN_LOAN_PAYLOAD_KEYS: ReadonlySet<string> = new Set([ |
There was a problem hiding this comment.
what if a person requires to disburse a loan to a linked savings account?
There was a problem hiding this comment.
@IOhacker Thanks for pointing this out.
The intention here is only to sanitize payloads for the calculateLoanSchedule() flow, which is used for repayment schedule preview/calculation and does not perform actual loan disbursement operations.
Actual disbursement to linked savings accounts goes through separate actions/endpoints such as disburse and disbursetosavings, so those flows remain unaffected by this change.
The issue I observed was specific to the schedule calculation endpoint, where the backend was throwing an NPE when GSIM-linked fields were included in the calculation payload. Since these fields are not required for repayment schedule computation itself, this change removes them only for that calculation request as a temporary defensive workaround until the backend-side validation is addressed.
Please let me know if there's a specific disbursement scenario that uses calculateLoanSchedule() that I may have missed.
2 participants
Description
This PR adds a frontend safeguard to prevent GSIM-linked fields from being sent to the backend loan schedule calculation endpoint, which was causing a 500 Internal Server Error.
Changes Made:
sanitizeLoanPayload()private method toLoansServicethat recursively removes unsafe GSIM linkage fields (linkAccountId,linkAccountOwnerId,linkAccountOwnerName,linkedSavingsAccount) from loan payloadscalculateLoanSchedule()endpoint before posting to/loans?command=calculateLoanScheduleWhy These Changes:
The root cause of the 500 error is a
NullPointerExceptionin the Fineract backend (LoanScheduleCalculationPlatformServiceImpl) during GLIM + GSIM validation. While a permanent fix should be made in Apache Fineract, this frontend safeguard prevents invalid payloads from being sent, providing immediate relief for users.Dependencies:
No new dependencies required. Uses existing Angular testing utilities (
provideHttpClient,provideHttpClientTestingfor Angular 20 compatibility).Related issues and discussion
WEB-112
Screenshots, if any
N/A - Backend API safeguard (no UI changes)
Checklist
web-app/.github/CONTRIBUTING.md.npm run lint,npm run headers:add)npm run test- 2/2 tests passing)Summary by CodeRabbit
Release Notes
Bug Fixes
Tests