From 987df06643a55c490bf1676eb6c662ba81780d14 Mon Sep 17 00:00:00 2001 From: Banx17 Date: Tue, 29 Sep 2026 11:08:36 +0100 Subject: [PATCH 1/3] fix(remittance_nft): enforce the ipfs:// / https:// metadata URI prefix validate_metadata_uri documented an ipfs://https:// prefix check that did not exist: `_ipfs_prefix`/`_https_prefix` were dead bindings and the only real rule was `uri.len() < 8`, so any 8+ byte string ("aaaaaaaa", the example in the issue) was accepted and minted. - Replace the dead bindings with has_accepted_metadata_uri_prefix, which reads the URI's leading bytes via EnvBase::string_copy_to_slice (the host call String::copy_into_slice delegates to; soroban_sdk::String cannot be sliced) and requires an exact b"ipfs://" or b"https://" prefix. read_len is bounded by uri.len(), so the copy can never read out of range or trap. - Keep the length floor as a pre-copy guard, expressed through the new LONGEST_URI_PREFIX_LEN (8) constant: its only remaining unique effect is rejecting the bare scheme "ipfs://", which was rejected before, so dropping it would have loosened validation. - Rejection keeps this function's convention (Err(NftError::InvalidMetadataUri)) and applies uniformly to all three callers: mint, admin_remint, update_metadata_uri. - Rewrite the doc comment to say what is actually enforced; full RFC 3986 parsing and content addressing remain out of scope, as the issue states. Tests: 6 new tests - "aaaaaaaa" is now rejected through mint where it used to succeed, ipfs:// and https:// URIs are accepted, prefix lookalikes (uppercase scheme, missing slash, http://, ftp://, leading space) are rejected, the length-floor boundary is pinned, and both admin_remint and update_metadata_uri enforce the same rule (their rejection is side-effect free). Also rename the duplicated test_transfer_rejects_burned_destination, which made the whole test target fail to compile on main with E0428. Both bodies are kept: the auto-burn path is now test_transfer_rejects_auto_burned_destination. Closes #1145 --- contracts/remittance_nft/src/lib.rs | 52 +++++- contracts/remittance_nft/src/test.rs | 237 ++++++++++++++++++++++++++- 2 files changed, 279 insertions(+), 10 deletions(-) diff --git a/contracts/remittance_nft/src/lib.rs b/contracts/remittance_nft/src/lib.rs index 9290637f..132e9eaa 100644 --- a/contracts/remittance_nft/src/lib.rs +++ b/contracts/remittance_nft/src/lib.rs @@ -1,7 +1,7 @@ #![cfg_attr(not(test), no_std)] use soroban_sdk::{ contract, contracterror, contractimpl, contracttype, symbol_short, Address, BytesN, Env, - String, Symbol, Vec, + EnvBase, String, Symbol, Val, Vec, }; #[contracterror] @@ -115,6 +115,10 @@ impl RemittanceNFT { /// events, enabling spam attacks. This floor rejects such calls early with /// InvalidRepaymentAmount (error 7). pub const MIN_SCORE_UPDATE_REPAYMENT: i128 = Self::POINTS_DENOMINATOR; + /// Length of the longest accepted metadata URI prefix (`"https://"`). + /// Doubles as the minimum accepted URI length and as the size of the stack + /// buffer `validate_metadata_uri` inspects the start of a URI in. + const LONGEST_URI_PREFIX_LEN: usize = 8; fn admin_key() -> soroban_sdk::Symbol { symbol_short!("ADMIN") @@ -250,19 +254,49 @@ impl RemittanceNFT { .publish((symbol_short!("MntAuth"), minter.clone()), ()); } + /// True when `uri` begins with `"ipfs://"` or `"https://"`. + /// + /// `String` cannot be sliced, and `String::copy_into_slice` insists on a + /// buffer matching the *whole* string, so the leading bytes are read with the + /// very host call `copy_into_slice` delegates to - + /// `EnvBase::string_copy_to_slice`, which fills the destination slice from a + /// given offset. `read_len` never exceeds `uri.len()`, so the copy cannot trap. + fn has_accepted_metadata_uri_prefix(env: &Env, uri: &String) -> bool { + const IPFS_PREFIX: &[u8] = b"ipfs://"; + const HTTPS_PREFIX: &[u8] = b"https://"; + + let uri_len = uri.len() as usize; + if uri_len < IPFS_PREFIX.len() { + return false; + } + + let read_len = core::cmp::min(uri_len, Self::LONGEST_URI_PREFIX_LEN); + let mut buf = [0u8; Self::LONGEST_URI_PREFIX_LEN]; + let _ = env.string_copy_to_slice(uri.to_object(), Val::U32_ZERO, &mut buf[..read_len]); + + let head = &buf[..read_len]; + head.starts_with(IPFS_PREFIX) || head.starts_with(HTTPS_PREFIX) + } + + /// Validate the metadata URI of a user's NFT. + /// + /// The URI must start with `"ipfs://"` or `"https://"`; anything else is + /// rejected with `InvalidMetadataUri`, however long it is. Full RFC 3986 + /// parsing, content addressing and CID validation are deliberately out of + /// scope: the scheme prefix *is* the acceptance rule, applied uniformly to + /// every caller (`mint`, `admin_remint`, `update_metadata_uri`). fn validate_metadata_uri(env: &Env, uri: &String) -> Result<(), NftError> { - // Check if URI starts with "ipfs://" or "https://" - let _ipfs_prefix = String::from_str(env, "ipfs://"); - let _https_prefix = String::from_str(env, "https://"); + // A bare scheme such as "ipfs://" names no content, and anything shorter + // cannot carry a scheme at all; both were rejected before the prefix check + // existed, so keep rejecting them. + if uri.len() < Self::LONGEST_URI_PREFIX_LEN as u32 { + return Err(NftError::InvalidMetadataUri); + } - // Simple validation: check if the URI has a reasonable length and starts with valid prefix - // We can't do complex string operations in no_std, so we do basic checks - if uri.len() < 8 { + if !Self::has_accepted_metadata_uri_prefix(env, uri) { return Err(NftError::InvalidMetadataUri); } - // For now, we accept any non-empty URI with reasonable length - // More sophisticated validation would require string comparison which is limited in no_std Ok(()) } diff --git a/contracts/remittance_nft/src/test.rs b/contracts/remittance_nft/src/test.rs index e1700aa0..9ef4f0df 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,238 @@ fn test_burn_removes_all_per_user_keys() { Err(Ok(NftError::CommitmentMissing)) ); } + +// --------------------------------------------------------------------------- +// Issue #1145: validate_metadata_uri must enforce the documented ipfs:// / +// https:// prefix instead of only checking a length floor. "aaaaaaaa" is the +// example from the issue: exactly 8 bytes, so it satisfied the old +// `uri.len() < 8` check and used to mint successfully. +// --------------------------------------------------------------------------- + +#[test] +fn test_mint_rejects_metadata_uri_without_accepted_prefix() { + 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); + + let result = client.try_mint( + &user, + &500, + &create_test_hash(&env, 1), + &String::from_str(&env, "aaaaaaaa"), + &create_test_commitment(&env, 1), + &None, + ); + assert_eq!(result, Err(Ok(NftError::InvalidMetadataUri))); + + // The rejected mint wrote nothing for the user. + assert!(client.get_metadata(&user).is_none()); + assert_eq!(client.get_score(&user), 0); +} + +#[test] +fn test_mint_accepts_ipfs_and_https_metadata_uris() { + let env = Env::default(); + env.mock_all_auths(); + + let admin = Address::generate(&env); + let contract_id = env.register(RemittanceNFT, ()); + let client = RemittanceNFTClient::new(&env, &contract_id); + + client.initialize(&admin); + + let ipfs_uri = String::from_str(&env, "ipfs://QmTest123"); + let ipfs_user = Address::generate(&env); + client.mint( + &ipfs_user, + &500, + &create_test_hash(&env, 2), + &ipfs_uri, + &create_test_commitment(&env, 2), + &None, + ); + assert_eq!(client.get_metadata_uri(&ipfs_user), Some(ipfs_uri)); + + let https_uri = String::from_str(&env, "https://example.com/metadata/1.json"); + let https_user = Address::generate(&env); + client.mint( + &https_user, + &500, + &create_test_hash(&env, 3), + &https_uri, + &create_test_commitment(&env, 3), + &None, + ); + assert_eq!(client.get_metadata_uri(&https_user), Some(https_uri)); +} + +#[test] +fn test_mint_rejects_metadata_uri_prefix_lookalikes() { + let env = Env::default(); + env.mock_all_auths(); + + let admin = Address::generate(&env); + let contract_id = env.register(RemittanceNFT, ()); + let client = RemittanceNFTClient::new(&env, &contract_id); + + client.initialize(&admin); + + // Every one of these is 8+ bytes, so all of them were accepted before the + // prefix check existed. + let rejected = [ + "IPFS://QmTest123", // scheme comparison is case-sensitive + "HTTPS://example.com", // uppercase scheme + "ipfs:/QmTest123", // one slash short + "ipfs/QmTest123", // no scheme separator + "https:/example.com", // one slash short + "http://example.com", // wrong scheme + "ftp://example.com", // wrong scheme + " ipfs://QmTest123", // leading whitespace before the scheme + "metadata.json", // no scheme at all + ]; + + for candidate in rejected { + let user = Address::generate(&env); + let result = client.try_mint( + &user, + &500, + &create_test_hash(&env, 4), + &String::from_str(&env, candidate), + &create_test_commitment(&env, 4), + &None, + ); + assert_eq!( + result, + Err(Ok(NftError::InvalidMetadataUri)), + "expected {candidate} to be rejected" + ); + } +} + +#[test] +fn test_mint_enforces_metadata_uri_minimum_length() { + let env = Env::default(); + env.mock_all_auths(); + + let admin = Address::generate(&env); + let contract_id = env.register(RemittanceNFT, ()); + let client = RemittanceNFTClient::new(&env, &contract_id); + + client.initialize(&admin); + + // A bare "ipfs://" is 7 bytes: the only URI the kept length floor rejects + // that the prefix check on its own would accept, so pin the behaviour. + let bare_ipfs_user = Address::generate(&env); + let result = client.try_mint( + &bare_ipfs_user, + &500, + &create_test_hash(&env, 5), + &String::from_str(&env, "ipfs://"), + &create_test_commitment(&env, 5), + &None, + ); + assert_eq!(result, Err(Ok(NftError::InvalidMetadataUri))); + + // "https://" is the longest accepted prefix (8 bytes) and therefore the + // shortest URI that clears both checks. + let bare_https_user = Address::generate(&env); + let result = client.try_mint( + &bare_https_user, + &500, + &create_test_hash(&env, 6), + &String::from_str(&env, "https://"), + &create_test_commitment(&env, 6), + &None, + ); + assert!(result.is_ok()); +} + +#[test] +fn test_admin_remint_enforces_metadata_uri_prefix() { + 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); + + // Only a burned account with an outstanding approval can be reminted. + client.mint( + &user, + &500, + &create_test_hash(&env, 7), + &create_test_uri(&env), + &create_test_commitment(&env, 7), + &None, + ); + client.burn(&user, &None); + client.approve_remint(&user); + + let result = client.try_admin_remint( + &user, + &500, + &create_test_hash(&env, 8), + &String::from_str(&env, "aaaaaaaa"), + &create_test_commitment(&env, 8), + ); + assert_eq!(result, Err(Ok(NftError::InvalidMetadataUri))); + + // The rejection is side-effect free: the one-time approval is not consumed, + // so the admin can retry with an accepted URI. + assert!(client.is_remint_approved(&user)); + assert!(client.get_metadata(&user).is_none()); + + client.admin_remint( + &user, + &500, + &create_test_hash(&env, 8), + &create_test_uri(&env), + &create_test_commitment(&env, 8), + ); + assert_eq!(client.get_metadata_uri(&user), Some(create_test_uri(&env))); +} + +#[test] +fn test_update_metadata_uri_enforces_metadata_uri_prefix() { + 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, 9), + &create_test_uri(&env), + &create_test_commitment(&env, 9), + &None, + ); + + let original = client.get_metadata_uri(&user).unwrap(); + + let result = client.try_update_metadata_uri(&user, &String::from_str(&env, "aaaaaaaa"), &None); + assert_eq!(result, Err(Ok(NftError::InvalidMetadataUri))); + + // A rejected update must leave the stored URI exactly as it was. + assert_eq!(client.get_metadata_uri(&user), Some(original)); + + let replacement = String::from_str(&env, "https://example.com/metadata/2.json"); + client.update_metadata_uri(&user, &replacement, &None); + assert_eq!(client.get_metadata_uri(&user), Some(replacement)); +} From a92d22be2947428697bbd366779855f7e175ff16 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 22718e6460cbf5b9cd48a02608a9e1a165104acb 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);