From c4923bbc1346d3cdba56bc95272ad90d82121b3d Mon Sep 17 00:00:00 2001 From: Banx17 Date: Tue, 29 Sep 2026 12:16:10 +0100 Subject: [PATCH 1/3] fix(contracts): use saturating_add in RemittanceNFT get_score_history get_score_history computed its page boundary as (offset + limit).min(len). contracts/Cargo.toml enables overflow-checks for the release profile, so the addition is checked in the deployed wasm: offset = 1 with limit = u32::MAX traps the whole call with "attempt to add with overflow" instead of returning the requested page (reproduced on the unmodified code before fixing). Use offset.saturating_add(limit).min(len) instead. The saturating_add must come first and the .min(len) second: that ordering is what makes the saturated u32::MAX resolve to the number of entries that actually exist, rather than being used as an oversized range bound (which would run the loop past len and panic in history.get(idx).unwrap()). Non-overflowing offset/limit pairs are arithmetically unchanged, so valid pagination returns exactly what it did before. Same approach as the reviewed PR #1764; no new error variant, no signature change, MAX_SCORE_HISTORY_ENTRIES untouched. Tests: adds test_get_score_history_limit_u32_max_does_not_trap (limit = u32::MAX at offset 0, 1, the last valid offset, and offset >= len) and test_get_score_history_saturating_add_preserves_valid_pagination (10 entries, offset = 3 with limit = u32::MAX - 1 must yield exactly 7 entries, plus the unchanged limit = 1000 / limit = 0 / offset = 0 cases). Both assert exact entry ledgers so an oversized or misaligned page fails them too. Necessary side-fix: main's test.rs declares test_transfer_rejects_burned_destination twice (E0428), which stops the test target from compiling, so the second declaration is renamed to test_transfer_rejects_auto_burned_destination. This is identical to the rename already carried by the open PRs #1950/#1951 and is called out in the PR description because it is not part of issue #1144. Closes #1144 --- contracts/remittance_nft/src/lib.rs | 8 +- contracts/remittance_nft/src/test.rs | 111 ++++++++++++++++++++++++++- 2 files changed, 117 insertions(+), 2 deletions(-) diff --git a/contracts/remittance_nft/src/lib.rs b/contracts/remittance_nft/src/lib.rs index 9290637f..db265eac 100644 --- a/contracts/remittance_nft/src/lib.rs +++ b/contracts/remittance_nft/src/lib.rs @@ -1239,7 +1239,13 @@ impl RemittanceNFT { return Vec::new(&env); } let mut page = Vec::new(&env); - let end = (offset + limit).min(len); + // `offset + limit` is not safe here: contract Wasm is built with + // `overflow-checks = true` (contracts/Cargo.toml), so any offset above + // zero combined with a large limit (e.g. offset = 1, limit = u32::MAX) + // traps the whole call. Saturate first, THEN clamp to `len`, so a + // saturated u32::MAX still resolves to the real available tail instead + // of being used as an oversized range bound. + let end = offset.saturating_add(limit).min(len); for idx in offset..end { page.push_back(history.get(idx).unwrap()); } diff --git a/contracts/remittance_nft/src/test.rs b/contracts/remittance_nft/src/test.rs index e1700aa0..f15d7038 100644 --- a/contracts/remittance_nft/src/test.rs +++ b/contracts/remittance_nft/src/test.rs @@ -1227,7 +1227,7 @@ fn test_transfer_rejects_destination_with_existing_state() { } #[test] -fn test_transfer_rejects_burned_destination() { +fn test_transfer_rejects_auto_burned_destination() { // Regression test: transfer only checked has_any_remittance_state(to), // which looks at Metadata/Score only. burn_internal() removes those two // keys but leaves Burned(to) set, so a burned destination previously @@ -2880,3 +2880,112 @@ fn test_burn_removes_all_per_user_keys() { Err(Ok(NftError::CommitmentMissing)) ); } + +#[test] +fn test_get_score_history_limit_u32_max_does_not_trap() { + // Regression test for #1144: `get_score_history` computed + // `let end = (offset + limit).min(len);`. With `overflow-checks = true` in + // the release profile (contracts/Cargo.toml) a plain `+` traps the whole + // call whenever `offset + limit` exceeds u32::MAX, e.g. limit = u32::MAX + // with offset = 1. The saturating value must still be clamped down to the + // real available length, so valid offsets keep returning the tail. + let env = Env::default(); + env.mock_all_auths(); + + let admin = Address::generate(&env); + let user = Address::generate(&env); + + let contract_id = env.register(RemittanceNFT, ()); + let client = RemittanceNFTClient::new(&env, &contract_id); + + client.initialize(&admin); + client.mint( + &user, + &500, + &create_test_hash(&env, 31), + &create_test_uri(&env), + &create_test_commitment(&env, 1), + &None, + ); + + // 5 history entries, ledgers 1..=5. + for sequence in 1..=5u32 { + env.ledger().set_sequence_number(sequence); + client.update_score(&user, &1_000_000_000, &None); + } + + // offset = 0: `0 + u32::MAX` does not overflow by itself, but the saturated + // end must still be clamped to `len` rather than used as a range bound. + let all = client.get_score_history(&user, &0, &u32::MAX); + assert_eq!(all.len(), 5); + assert_eq!(all.get(0).unwrap().ledger, 1); + assert_eq!(all.get(4).unwrap().ledger, 5); + + // offset = 1: `1 + u32::MAX` overflows u32 and trapped before this fix. + // It must return the remaining 4 entries, in order. + let tail = client.get_score_history(&user, &1, &u32::MAX); + assert_eq!(tail.len(), 4); + assert_eq!(tail.get(0).unwrap().ledger, 2); + assert_eq!(tail.get(3).unwrap().ledger, 5); + + // The last valid offset still yields exactly one entry. + let last = client.get_score_history(&user, &4, &u32::MAX); + assert_eq!(last.len(), 1); + assert_eq!(last.get(0).unwrap().ledger, 5); + + // offset >= len stays an empty page, even with a saturated limit. + assert_eq!(client.get_score_history(&user, &5, &u32::MAX).len(), 0); +} + +#[test] +fn test_get_score_history_saturating_add_preserves_valid_pagination() { + // #1144 ordering check: `.min(len)` must be applied *after* + // `saturating_add`. Clamping first (or using the saturated value directly + // as the range bound) would run the loop past `len`, and + // `history.get(idx).unwrap()` would panic instead of returning a tail. + let env = Env::default(); + env.mock_all_auths(); + + let admin = Address::generate(&env); + let user = Address::generate(&env); + + let contract_id = env.register(RemittanceNFT, ()); + let client = RemittanceNFTClient::new(&env, &contract_id); + + client.initialize(&admin); + client.mint( + &user, + &500, + &create_test_hash(&env, 32), + &create_test_uri(&env), + &create_test_commitment(&env, 1), + &None, + ); + + // 10 history entries, ledgers 1..=10. + for sequence in 1..=10u32 { + env.ledger().set_sequence_number(sequence); + client.update_score(&user, &1_000_000_000, &None); + } + + // A plain, non-saturating page is unchanged by the fix. + let page = client.get_score_history(&user, &3, &1_000); + assert_eq!(page.len(), 7); + assert_eq!(page.get(0).unwrap().ledger, 4); + assert_eq!(page.get(6).unwrap().ledger, 10); + + // limit = u32::MAX - 1 only overflows once the offset is added; the + // saturated result still clamps to the available tail. + let saturated = client.get_score_history(&user, &3, &(u32::MAX - 1)); + assert_eq!(saturated.len(), 7); + assert_eq!(saturated.get(0).unwrap().ledger, 4); + assert_eq!(saturated.get(6).unwrap().ledger, 10); + + // limit = 0 still returns an empty page (empty range, no underflow). + assert_eq!(client.get_score_history(&user, &3, &0).len(), 0); + + // offset = 0 with a fully saturated limit returns everything available. + let everything = client.get_score_history(&user, &0, &u32::MAX); + assert_eq!(everything.len(), 10); + assert_eq!(everything.get(9).unwrap().ledger, 10); +} From 4cb63908c3daee48c19e54f4bf9407e4c64902b4 Mon Sep 17 00:00:00 2001 From: Banx17 Date: Tue, 29 Sep 2026 10:10:06 +0100 Subject: [PATCH 2/3] style(backend): apply the prettier formatting the lint job requires The `backend` CI job fails at its Lint step with 8 prettier/prettier errors, so its Build, Type check and Run tests steps never execute and the whole pipeline stays red. The errors are pre-existing (they come from files unrelated to the contracts change in this branch) but they block this PR from going green, so they are fixed here. `prettier --write` on exactly the four files eslint flagged: - src/__tests__/remittanceFilters.test.ts (1 error, line 42) - src/services/__tests__/auditLogService.pagination.test.ts (2 errors, lines 103/125) - src/services/auditLogService.ts (1 error, line 27) - src/tests/idempotency.namespace.test.ts (4 errors, lines 2/36/78/176) Formatting only: line-wrap decisions, nothing else. No logic, no import added or removed, no assertion touched. Verified with `prettier --check` on the same four files, which now reports "All matched files use Prettier code style!". Refs #1146 --- .../src/__tests__/remittanceFilters.test.ts | 7 ++++--- .../auditLogService.pagination.test.ts | 8 ++----- backend/src/services/auditLogService.ts | 3 +-- .../src/tests/idempotency.namespace.test.ts | 21 +++++++------------ 4 files changed, 15 insertions(+), 24 deletions(-) diff --git a/backend/src/__tests__/remittanceFilters.test.ts b/backend/src/__tests__/remittanceFilters.test.ts index 080c3b12..aadc6172 100644 --- a/backend/src/__tests__/remittanceFilters.test.ts +++ b/backend/src/__tests__/remittanceFilters.test.ts @@ -37,9 +37,10 @@ jest.unstable_mockModule('../services/notificationService.js', () => ({ })); jest.unstable_mockModule('../utils/stellarEnvelope.js', () => ({ - parseAndValidateSignedEnvelope: jest - .fn() - .mockReturnValue({ source: 'GCWEPACYJLN7S3ZUXSVMXZBFKYXSHRGZ6O326HDDPDKBKZPXD45XNHC3', signatureCount: 1 }), + parseAndValidateSignedEnvelope: jest.fn().mockReturnValue({ + source: 'GCWEPACYJLN7S3ZUXSVMXZBFKYXSHRGZ6O326HDDPDKBKZPXD45XNHC3', + signatureCount: 1, + }), })); const mockQuery = jest.fn(); diff --git a/backend/src/services/__tests__/auditLogService.pagination.test.ts b/backend/src/services/__tests__/auditLogService.pagination.test.ts index d89cc90a..60645ecb 100644 --- a/backend/src/services/__tests__/auditLogService.pagination.test.ts +++ b/backend/src/services/__tests__/auditLogService.pagination.test.ts @@ -100,9 +100,7 @@ describe('getAuditLogs keyset pagination and totals (#1808)', () => { it('omits the count query unless withTotal is set', async () => { await getAuditLogs({ limit: 2 }); - const countCalls = mockQuery.mock.calls.filter(([text]) => - String(text).includes('COUNT(*)'), - ); + const countCalls = mockQuery.mock.calls.filter(([text]) => String(text).includes('COUNT(*)')); expect(countCalls).toHaveLength(0); }); @@ -122,9 +120,7 @@ describe('getAuditLogs keyset pagination and totals (#1808)', () => { limit: 2, }); - const countCall = mockQuery.mock.calls.find(([text]) => - String(text).includes('COUNT(*)'), - ); + const countCall = mockQuery.mock.calls.find(([text]) => String(text).includes('COUNT(*)')); const countSql = String(countCall?.[0]); const countValues = (countCall?.[1] as unknown[]) ?? []; diff --git a/backend/src/services/auditLogService.ts b/backend/src/services/auditLogService.ts index 1fcfc8d7..a7dfd33b 100644 --- a/backend/src/services/auditLogService.ts +++ b/backend/src/services/auditLogService.ts @@ -24,8 +24,7 @@ export function encodeCursor(row: Record | undefined): string | const id = row.id; const createdAt = row.created_at; if (id === undefined || id === null || !createdAt) return null; - const createdAtIso = - createdAt instanceof Date ? createdAt.toISOString() : String(createdAt); + const createdAtIso = createdAt instanceof Date ? createdAt.toISOString() : String(createdAt); return `${createdAtIso}${CURSOR_SEPARATOR}${String(id)}`; } diff --git a/backend/src/tests/idempotency.namespace.test.ts b/backend/src/tests/idempotency.namespace.test.ts index aedb3af7..3b58d4ed 100644 --- a/backend/src/tests/idempotency.namespace.test.ts +++ b/backend/src/tests/idempotency.namespace.test.ts @@ -1,5 +1,9 @@ import { Request, Response, NextFunction } from 'express'; -import { idempotencyMiddleware, computeFingerprint, namespacedKey } from '../middleware/idempotency.js'; +import { + idempotencyMiddleware, + computeFingerprint, + namespacedKey, +} from '../middleware/idempotency.js'; import { cacheService } from '../services/cacheService.js'; import { jest } from '@jest/globals'; @@ -33,8 +37,7 @@ describe('idempotencyMiddleware key namespacing (#1809)', () => { return request; }; - const cacheKeysRead = () => - asMock(cacheService.get).mock.calls.map(([key]) => String(key)); + const cacheKeysRead = () => asMock(cacheService.get).mock.calls.map(([key]) => String(key)); beforeEach(() => { req = buildRequest(ALICE); @@ -75,11 +78,7 @@ describe('idempotencyMiddleware key namespacing (#1809)', () => { jest.clearAllMocks(); asMock(cacheService.setNotExists).mockResolvedValue(true); - await idempotencyMiddleware( - buildRequest(BOB) as Request, - res as Response, - next, - ); + await idempotencyMiddleware(buildRequest(BOB) as Request, res as Response, next); const bobKey = cacheKeysRead()[0]; expect(aliceKey).not.toBe(bobKey); @@ -173,11 +172,7 @@ describe('idempotencyMiddleware key namespacing (#1809)', () => { jest.clearAllMocks(); asMock(cacheService.setNotExists).mockResolvedValue(true); - await idempotencyMiddleware( - buildRequest(undefined) as Request, - res as Response, - next, - ); + await idempotencyMiddleware(buildRequest(undefined) as Request, res as Response, next); expect(cacheKeysRead()[0]).toBe(first); expect(first).toContain('anon'); From 404a317ebf194bb7d093496abd09641653d63566 Mon Sep 17 00:00:00 2001 From: Banx17 Date: Tue, 29 Sep 2026 10:23:59 +0100 Subject: [PATCH 3/3] test(backend): repair the 9 stale tests that keep the backend job red The backend job has never reached its Run tests step: Lint failed first, so 9 broken assertions sat undetected on main. With lint fixed they fail, and every one of them is a test-side bug - each implementation is internally consistent with its documented behaviour: auditLogService.pagination.test.ts (#1808) - 'pages with a (created_at, id) ...' called getAuditLogs with no cursor and then asserted the keyset predicate that only a cursor produces. - 'emits a composite nextCursor' split the cursor on the first ':' although the documented format is `${iso}:${id}` and an ISO timestamp contains ':'; it now splits on the last one, exactly as decodeCursor reads it back. - 'resumes correctly from a cursor it previously issued' inspected the first SELECT of the test instead of the resumed one; the pageQuery() helper now returns the most recent page query. - 'counts an unfiltered table as a single plain query' compared against a string without the trailing space the builder interpolates; it now asserts there is no WHERE clause and compares the trimmed statement. - 'keeps the documented filter surface' expected 8 filters, but both AuditLogFilters and the swagger docs expose 7; it now pins those 7 names. idempotency.test.ts (pre-#1809 expectations) - expected `idemp:${key}` and `idemp:${key}:lock`; #1809 namespaces both keys per wallet (this fixture is unauthenticated, hence the `anon` namespace). idempotency.namespace.test.ts (#1809's own tests) - the cache and lock mocks answered identically for every key, which cannot model a keyed store: 'does not replay another wallet's cached response' handed Bob's entry to Alice, and 'does not reject a user with 409' hit Bob's lock. Both mocks are now key-aware and the assertions pin the namespaced key plus the fact that Alice's handler still runs. Verified locally: the three files pass under jest (28 tests) and `eslint .` reports 0 errors / 30 pre-existing warnings. No production code changes. Refs #1808, #1809 --- .../auditLogService.pagination.test.ts | 37 ++++++++++++++----- .../src/tests/idempotency.namespace.test.ts | 36 ++++++++++++------ backend/src/tests/idempotency.test.ts | 9 +++-- 3 files changed, 58 insertions(+), 24 deletions(-) diff --git a/backend/src/services/__tests__/auditLogService.pagination.test.ts b/backend/src/services/__tests__/auditLogService.pagination.test.ts index 60645ecb..89cd8357 100644 --- a/backend/src/services/__tests__/auditLogService.pagination.test.ts +++ b/backend/src/services/__tests__/auditLogService.pagination.test.ts @@ -17,11 +17,12 @@ const PAGE_ROWS = [ { id: '298', created_at: '2026-03-01T00:00:00.000Z' }, ]; -/** Last call to query() — always the SELECT page statement. */ +/** Most recent SELECT page statement issued by getAuditLogs. */ const pageQuery = () => { - const call = mockQuery.mock.calls.find( + const pageCalls = mockQuery.mock.calls.filter( ([text]) => typeof text === 'string' && text.includes('SELECT * FROM audit_logs'), ); + const call = pageCalls[pageCalls.length - 1]; return { text: String(call?.[0]), values: (call?.[1] as unknown[]) ?? [] }; }; @@ -48,7 +49,8 @@ describe('getAuditLogs keyset pagination and totals (#1808)', () => { }); it('pages with a (created_at, id) row comparison, not id alone', async () => { - await getAuditLogs({ limit: 2 }); + // Resume from the composite cursor the previous page ended on. + await getAuditLogs({ limit: 2, cursor: '2026-03-02T00:00:00.000Z:298' }); const { text, values } = pageQuery(); expect(text).toMatch(/\(created_at, id\)\s*<\s*\(\$\d+, \$\d+\)/); @@ -67,11 +69,13 @@ describe('getAuditLogs keyset pagination and totals (#1808)', () => { const result = await getAuditLogs({ limit: 2 }); expect(result.nextCursor).not.toBeNull(); - // The cursor carries the timestamp *and* the id it is paging from. - expect(result.nextCursor).toContain(':'); - const [createdAt, id] = String(result.nextCursor).split(':'); - expect(createdAt).toBe('2026-03-02T00:00:00.000Z'); - expect(id).toBe('299'); + // The cursor carries the timestamp *and* the id it is paging from. An ISO + // timestamp contains ':' itself, so read it back from the *last* + // separator — exactly how decodeCursor parses it. + const cursor = String(result.nextCursor); + const separatorAt = cursor.lastIndexOf(':'); + expect(cursor.slice(0, separatorAt)).toBe('2026-03-02T00:00:00.000Z'); + expect(cursor.slice(separatorAt + 1)).toBe('299'); }); it('returns a null cursor on the last page', async () => { @@ -167,7 +171,10 @@ describe('getAuditLogs keyset pagination and totals (#1808)', () => { const countSql = String( mockQuery.mock.calls.find(([text]) => String(text).includes('COUNT(*)'))?.[0], ); - expect(countSql).toBe('SELECT COUNT(*) as count FROM audit_logs'); + // No filters → no WHERE clause. The builder interpolates an empty clause, + // so compare the trimmed statement instead of its trailing space. + expect(countSql).not.toContain('WHERE'); + expect(countSql.trim()).toBe('SELECT COUNT(*) as count FROM audit_logs'); }); }); @@ -199,6 +206,16 @@ describe('AuditLogFilters shape (#1808)', () => { limit: 1, withTotal: true, }; - expect(Object.keys(filters)).toHaveLength(8); + // Exactly the 7 query parameters documented for GET /admin/audit-logs + // (src/swagger/adminSwagger.ts). + expect(Object.keys(filters).sort()).toEqual([ + 'action', + 'actor', + 'cursor', + 'from', + 'limit', + 'to', + 'withTotal', + ]); }); }); diff --git a/backend/src/tests/idempotency.namespace.test.ts b/backend/src/tests/idempotency.namespace.test.ts index 3b58d4ed..b068689a 100644 --- a/backend/src/tests/idempotency.namespace.test.ts +++ b/backend/src/tests/idempotency.namespace.test.ts @@ -87,30 +87,44 @@ describe('idempotencyMiddleware key namespacing (#1809)', () => { }); it('does not replay another wallet’s cached response', async () => { - // Bob's response is already cached under his namespace… - asMock(cacheService.get).mockResolvedValue({ - status: 201, - body: { id: 'bob-loan' }, - fingerprint: computeFingerprint(buildRequest(BOB) as Request).fingerprint, - }); + // Bob's response is already cached under *his* namespace. Redis is keyed, + // so only Bob's namespace holds it — the mock models that by answering + // per key instead of returning Bob's entry for any key. + asMock(cacheService.get).mockImplementation((key: unknown) => + String(key).includes(BOB) + ? { + status: 201, + body: { id: 'bob-loan' }, + fingerprint: computeFingerprint(buildRequest(BOB) as Request).fingerprint, + } + : null, + ); // …so Alice sending the identical key, path and body gets a cache miss - // and runs the handler instead of receiving Bob's response. + // and runs the handler instead of receiving Bob's response. `res.json` is + // captured up-front because the middleware wraps it on a cache miss. + const jsonSpy = asMock(res.json); await idempotencyMiddleware(req as Request, res as Response, next); - expect(cacheKeysRead()[0]).not.toContain('bob-loan'); - expect(res.json).not.toHaveBeenCalledWith({ id: 'bob-loan' }); + expect(cacheKeysRead()[0]).toContain(ALICE); + expect(cacheKeysRead()[0]).not.toContain(BOB); + expect(jsonSpy).not.toHaveBeenCalledWith({ id: 'bob-loan' }); + expect(next).toHaveBeenCalled(); }); it('does not reject a user with 409 because another user holds the key', async () => { - // Bob's in-flight lock is held under his namespace. + // Bob's in-flight lock is held under his namespace, so the reservation is + // only refused for Bob's namespaced lock key. asMock(cacheService.get).mockResolvedValue(null); - asMock(cacheService.setNotExists).mockResolvedValue(false); + asMock(cacheService.setNotExists).mockImplementation((key: unknown) => + Promise.resolve(!String(key).includes(BOB)), + ); await idempotencyMiddleware(req as Request, res as Response, next); // Alice is unaffected by Bob's lock: the handler still runs. expect(asMock(res.status)).not.toHaveBeenCalledWith(409); + expect(next).toHaveBeenCalled(); }); it('namespaces the lock key as well as the cache key', async () => { diff --git a/backend/src/tests/idempotency.test.ts b/backend/src/tests/idempotency.test.ts index 918ca12a..4f2a2d14 100644 --- a/backend/src/tests/idempotency.test.ts +++ b/backend/src/tests/idempotency.test.ts @@ -62,7 +62,9 @@ describe('Idempotency Middleware', () => { await idempotencyMiddleware(req as Request, res as Response, next); - expect(cacheService.get).toHaveBeenCalledWith(`idemp:${key}`); + // #1809: the cache key is namespaced by the caller's wallet; this fixture is + // unauthenticated, so it reads from the shared `anon` namespace. + expect(cacheService.get).toHaveBeenCalledWith(`idemp:anon:${key}`); expect(res.status).toHaveBeenCalledWith(201); expect(res.set).toHaveBeenCalledWith('X-Idempotency-Cache', 'HIT'); expect(res.json).toHaveBeenCalledWith(cachedResponse.body); @@ -169,9 +171,10 @@ describe('Idempotency Middleware', () => { (res.json as unknown as (b: unknown) => void)({ success: true }); await finishHandler(); - expect(cacheService.delete).toHaveBeenCalledWith(`idemp:${key}:lock`); + // Both keys are namespaced by caller (#1809); this fixture is unauthenticated. + expect(cacheService.delete).toHaveBeenCalledWith(`idemp:anon:${key}:lock`); const setCall = (cacheService.set as jest.Mock).mock.calls[0]; - expect(setCall[0]).toBe(`idemp:${key}`); + expect(setCall[0]).toBe(`idemp:anon:${key}`); const stored = setCall[1] as { fingerprint: string; body: unknown }; expect(stored.body).toEqual({ success: true }); expect(stored.fingerprint).toBe(computeFingerprint(req as Request).fingerprint);