fix(remittance_nft): enforce the ipfs:// / https:// metadata URI prefix (#1145)

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(())
     }
 

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));
+}