diff --git a/contracts/remittance_nft/src/lib.rs b/contracts/remittance_nft/src/lib.rs index 1c696665..b379c338 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] @@ -161,6 +161,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") @@ -296,19 +300,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 b6e26044..3a771dd8 100644 --- a/contracts/remittance_nft/src/test.rs +++ b/contracts/remittance_nft/src/test.rs @@ -3257,3 +3257,238 @@ fn test_set_min_repayment_amount_requires_admin_auth() { env.mock_auths(&[]); client.set_min_repayment_amount(&1_000_000); } + +// --------------------------------------------------------------------------- +// 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)); +}