From 2b42c943eaa7e6671f3b1bc82bced02c15be6727 Mon Sep 17 00:00:00 2001 From: Mario Ruci Date: Tue, 11 Aug 2026 10:05:04 +0200 Subject: [PATCH] fix(station): AllowListed ignores addresses listed by the requester AllowListed approves a transfer outright when its destination is present in the address book. Address book writes default to the operator rule while an account's transfer policy is typically admin-tier, so a single operator could list their own address, self-approve that entry at operator quorum, and then spend to it with no further approval. Record the user that last created or edited an entry, and stop AllowListed treating an entry as evidence of trust when the same user submitted the transfer. Listing and spending now require two different people. Entries stored before this field existed decode with no author and are still honoured, so existing allow-lists keep working across the upgrade. Co-Authored-By: Claude Opus 5 --- .../requests/add_address_book_entry.rs | 5 +- .../requests/edit_address_book_entry.rs | 5 +- core/station/impl/src/mappers/address_book.rs | 2 + core/station/impl/src/migration_tests/mod.rs | 2 + .../snapshots/address_book_repository_v3.bin | Bin 65536 -> 65536 bytes core/station/impl/src/models/address_book.rs | 47 +++++++ .../impl/src/models/request_policy_rule.rs | 115 +++++++++++++++++- .../station/impl/src/services/address_book.rs | 24 ++-- 8 files changed, 188 insertions(+), 12 deletions(-) diff --git a/core/station/impl/src/factories/requests/add_address_book_entry.rs b/core/station/impl/src/factories/requests/add_address_book_entry.rs index a0d7d7cc8..63dd55f9c 100644 --- a/core/station/impl/src/factories/requests/add_address_book_entry.rs +++ b/core/station/impl/src/factories/requests/add_address_book_entry.rs @@ -48,7 +48,10 @@ impl<'p, 'o> AddAddressBookEntryRequestExecute<'p, 'o> { impl Execute for AddAddressBookEntryRequestExecute<'_, '_> { async fn execute(&self) -> Result { let address_book_entry = ADDRESS_BOOK_SERVICE - .create_entry(self.operation.input.to_owned()) + .create_entry( + self.operation.input.to_owned(), + Some(self.request.requested_by), + ) .await .map_err(|e| RequestExecuteError::Failed { reason: format!("Failed to create address book entry: {e}"), diff --git a/core/station/impl/src/factories/requests/edit_address_book_entry.rs b/core/station/impl/src/factories/requests/edit_address_book_entry.rs index c5f711f2c..bea07c70a 100644 --- a/core/station/impl/src/factories/requests/edit_address_book_entry.rs +++ b/core/station/impl/src/factories/requests/edit_address_book_entry.rs @@ -61,7 +61,10 @@ impl<'p, 'o> EditAddressBookEntryRequestExecute<'p, 'o> { impl Execute for EditAddressBookEntryRequestExecute<'_, '_> { async fn execute(&self) -> Result { ADDRESS_BOOK_SERVICE - .edit_entry(self.operation.input.to_owned()) + .edit_entry( + self.operation.input.to_owned(), + Some(self.request.requested_by), + ) .await .map_err(|e| RequestExecuteError::Failed { reason: format!("Failed to update address book entry: {e}"), diff --git a/core/station/impl/src/mappers/address_book.rs b/core/station/impl/src/mappers/address_book.rs index 2cf0ea57b..d428b42b7 100644 --- a/core/station/impl/src/mappers/address_book.rs +++ b/core/station/impl/src/mappers/address_book.rs @@ -39,6 +39,7 @@ impl AddressBookMapper { pub fn from_create_input( input: AddAddressBookEntryOperationInput, entry_id: UUID, + created_by: Option, ) -> Result { let new_entry = AddressBookEntry { id: entry_id, @@ -49,6 +50,7 @@ impl AddressBookMapper { labels: input.labels, metadata: input.metadata.into(), last_modification_timestamp: next_time(), + last_modified_by: created_by, }; Ok(new_entry) diff --git a/core/station/impl/src/migration_tests/mod.rs b/core/station/impl/src/migration_tests/mod.rs index 5d8132302..7f97c74ef 100644 --- a/core/station/impl/src/migration_tests/mod.rs +++ b/core/station/impl/src/migration_tests/mod.rs @@ -53,6 +53,7 @@ mod test { blockchain: crate::models::Blockchain::InternetComputer, labels: vec!["Alice".to_string(), "Bob".to_string()], last_modification_timestamp: 0, + last_modified_by: None, metadata: Metadata::default(), }, AddressBookEntry { @@ -63,6 +64,7 @@ mod test { blockchain: crate::models::Blockchain::InternetComputer, labels: vec!["Alice".to_string(), "Bob".to_string()], last_modification_timestamp: 0, + last_modified_by: None, metadata: Metadata::new( [ ("key1".to_string(), "value1".to_string()), diff --git a/core/station/impl/src/migration_tests/snapshots/address_book_repository_v3.bin b/core/station/impl/src/migration_tests/snapshots/address_book_repository_v3.bin index 9d9b778e9cc9f8c2ceb4f049d73b783ddd773187..9f16d3e73f710fc8b3e2537b6eed5d2a607bd9c8 100644 GIT binary patch delta 77 zcmZo@U}!+3gg6AM4n<`R~k-vBpi53c|K diff --git a/core/station/impl/src/models/address_book.rs b/core/station/impl/src/models/address_book.rs index e72ce5a8d..05700079d 100644 --- a/core/station/impl/src/models/address_book.rs +++ b/core/station/impl/src/models/address_book.rs @@ -34,6 +34,13 @@ pub struct AddressBookEntry { pub labels: Vec, /// The last time the record was updated or created. pub last_modification_timestamp: Timestamp, + /// The user that last created or edited this entry. + /// + /// `AllowListed` treats presence in the address book as approval for a transfer, so this is + /// used to stop the same user both listing an address and spending to it. `None` on entries + /// written before this field existed. + #[serde(default)] + pub last_modified_by: Option, } #[storable] @@ -159,6 +166,45 @@ mod tests { use super::address_book_entry_test_utils::mock_address_book_entry; use super::*; + /// `last_modified_by` was added after entries were already in stable memory. Entries are + /// stored as CBOR, so a record written without the field must still decode, with the field + /// defaulting to `None` rather than trapping the upgrade. + #[test] + fn decodes_entries_stored_before_last_modified_by_existed() { + use ic_stable_structures::Storable; + + #[derive(serde::Serialize)] + struct LegacyAddressBookEntry { + id: AddressBookEntryId, + address_owner: String, + address: String, + blockchain: Blockchain, + address_format: AddressFormat, + metadata: Metadata, + labels: Vec, + last_modification_timestamp: Timestamp, + } + + let legacy = LegacyAddressBookEntry { + id: [7; 16], + address_owner: "Alice".to_string(), + address: "0x1234".to_string(), + blockchain: Blockchain::InternetComputer, + address_format: AddressFormat::ICPAccountIdentifier, + metadata: Metadata::default(), + labels: vec!["counterparty".to_string()], + last_modification_timestamp: 42, + }; + + let bytes = serde_cbor::to_vec(&legacy).expect("Failed to encode legacy entry"); + let decoded = AddressBookEntry::from_bytes(std::borrow::Cow::Owned(bytes)); + + assert_eq!(decoded.last_modified_by, None); + assert_eq!(decoded.address, "0x1234"); + assert_eq!(decoded.labels, vec!["counterparty".to_string()]); + assert_eq!(decoded.last_modification_timestamp, 42); + } + #[test] fn test_address_book_entry_validation() { let address_book_entry = mock_address_book_entry(); @@ -274,6 +320,7 @@ pub mod address_book_entry_test_utils { blockchain: Blockchain::InternetComputer, metadata: Metadata::mock(), last_modification_timestamp: 0, + last_modified_by: None, } } diff --git a/core/station/impl/src/models/request_policy_rule.rs b/core/station/impl/src/models/request_policy_rule.rs index ddbc925f2..09d376816 100644 --- a/core/station/impl/src/models/request_policy_rule.rs +++ b/core/station/impl/src/models/request_policy_rule.rs @@ -564,10 +564,19 @@ impl continue; }; - let is_in_address_book = ADDRESS_BOOK_REPOSITORY - .exists(asset.blockchain, transfer.input.to.clone()); + // An entry the requester listed themselves is not independent + // evidence that the destination is trusted: address book writes + // sit at a lower approval tier than transfers by default, so + // honouring them would let one user both allow-list an address + // and spend to it. Entries written before this was tracked have + // no author and are still honoured. + let listed_independently = ADDRESS_BOOK_REPOSITORY + .find_by_address(asset.blockchain, transfer.input.to.clone()) + .is_some_and(|entry| { + entry.last_modified_by != Some(request.requested_by) + }); - if is_in_address_book { + if listed_independently { return Ok(RequestPolicyRuleResult { status: EvaluationStatus::Approved, evaluated_rule: EvaluatedRequestPolicyRule::AllowListed, @@ -973,3 +982,103 @@ mod test { ); } } + +#[cfg(test)] +mod allow_listed_tests { + use super::*; + use crate::core::test_utils::init_canister_system; + use crate::models::{ + account_test_utils::mock_account, + address_book_entry_test_utils::mock_address_book_entry, + asset_test_utils::mock_asset, + request_specifier::{AddressBookMetadataMatcher, UserMatcher}, + request_test_utils::mock_request, + AccountAsset, Metadata, RequestOperation, TokenStandard, TransferOperation, + TransferOperationInput, + }; + use crate::repositories::{ACCOUNT_REPOSITORY, ADDRESS_BOOK_REPOSITORY, ASSET_REPOSITORY}; + use orbit_essentials::repository::Repository; + use orbit_essentials::types::UUID; + + const DESTINATION: &str = "0xdeadbeef"; + + fn evaluator() -> RequestPolicyRuleEvaluator { + RequestPolicyRuleEvaluator { + user_matcher: Arc::new(UserMatcher), + address_book_metadata_matcher: Arc::new(AddressBookMetadataMatcher), + } + } + + /// Lists `DESTINATION` in the address book as `listed_by`, then returns a transfer request to + /// that destination submitted by `requested_by`. + fn transfer_to_listed_address(listed_by: Option, requested_by: UUID) -> Request { + init_canister_system(); + + let asset = mock_asset(); + ASSET_REPOSITORY.insert(asset.id, asset.clone()); + + let mut account = mock_account(); + account.assets = vec![AccountAsset { + asset_id: asset.id, + balance: None, + }]; + ACCOUNT_REPOSITORY.insert(account.to_key(), account.clone()); + + let mut entry = mock_address_book_entry(); + entry.blockchain = asset.blockchain.clone(); + entry.address = DESTINATION.to_string(); + entry.last_modified_by = listed_by; + ADDRESS_BOOK_REPOSITORY.insert(entry.to_key(), entry); + + let mut request = mock_request(); + request.requested_by = requested_by; + request.operation = RequestOperation::Transfer(TransferOperation { + fee: None, + transfer_id: None, + asset: asset.clone(), + input: TransferOperationInput { + from_account_id: account.id, + from_asset_id: asset.id, + with_standard: TokenStandard::InternetComputerNative, + to: DESTINATION.to_string(), + amount: 100u64.into(), + metadata: Metadata::default(), + network: "mainnet".to_string(), + fee: None, + }, + }); + + request + } + + fn evaluate(request: Request) -> EvaluationStatus { + evaluator() + .evaluate((Arc::new(request), Arc::new(RequestPolicyRule::AllowListed))) + .expect("Failed to evaluate AllowListed") + .status + } + + #[test] + fn approves_an_address_listed_by_someone_else() { + let request = transfer_to_listed_address(Some([1; 16]), [2; 16]); + + assert_eq!(evaluate(request), EvaluationStatus::Approved); + } + + /// Address book writes sit at a lower approval tier than transfers by default, so a user must + /// not be able to both list a destination and spend to it. + #[test] + fn rejects_an_address_the_requester_listed_themselves() { + let requester = [2; 16]; + let request = transfer_to_listed_address(Some(requester), requester); + + assert_eq!(evaluate(request), EvaluationStatus::Rejected); + } + + #[test] + fn approves_entries_that_predate_authorship_tracking() { + let request = transfer_to_listed_address(None, [2; 16]); + + assert_eq!(evaluate(request), EvaluationStatus::Approved); + } +} diff --git a/core/station/impl/src/services/address_book.rs b/core/station/impl/src/services/address_book.rs index bbbda5590..e6e66bc7d 100644 --- a/core/station/impl/src/services/address_book.rs +++ b/core/station/impl/src/services/address_book.rs @@ -16,7 +16,9 @@ use crate::{ repositories::{AddressBookRepository, AddressBookWhereClause, ADDRESS_BOOK_REPOSITORY}, }; use lazy_static::lazy_static; -use orbit_essentials::{api::ServiceResult, model::ModelValidator, repository::Repository}; +use orbit_essentials::{ + api::ServiceResult, model::ModelValidator, repository::Repository, types::UUID, +}; use station_api::PaginationInput; use std::sync::Arc; use uuid::Uuid; @@ -111,11 +113,13 @@ impl AddressBookService { pub async fn create_entry( &self, input: AddAddressBookEntryOperationInput, + created_by: Option, ) -> ServiceResult { let uuid = generate_uuid_v4().await; let key = AddressBookEntry::key(*uuid.as_bytes()); - let new_entry = AddressBookMapper::from_create_input(input.to_owned(), *uuid.as_bytes())?; + let new_entry = + AddressBookMapper::from_create_input(input.to_owned(), *uuid.as_bytes(), created_by)?; new_entry.validate()?; if let Some(v) = self @@ -139,6 +143,7 @@ impl AddressBookService { pub async fn edit_entry( &self, input: EditAddressBookEntryOperationInput, + edited_by: Option, ) -> ServiceResult { let mut entry = self.get_entry_by_id(&input.address_book_entry_id)?; @@ -150,6 +155,8 @@ impl AddressBookService { entry.metadata.change(change_metadata); } + entry.last_modified_by = edited_by; + entry.validate()?; self.address_book_repository @@ -215,7 +222,10 @@ mod tests { }, }; - let result = ctx.service.create_entry(operation.input.clone()).await; + let result = ctx + .service + .create_entry(operation.input.clone(), None) + .await; let new_entry = result.unwrap(); @@ -226,7 +236,7 @@ mod tests { // adding a new entry for the same address should fail - let result = ctx.service.create_entry(operation.input).await; + let result = ctx.service.create_entry(operation.input, None).await; result.unwrap_err(); } @@ -258,7 +268,7 @@ mod tests { )), labels: None, }; - let result = ctx.service.edit_entry(operation).await; + let result = ctx.service.edit_entry(operation, None).await; assert!(result.is_ok()); let updated_entry = result.unwrap(); address_book_entry.address_owner = "test_edit".to_string(); @@ -298,7 +308,7 @@ mod tests { )), labels: None, }; - let result = ctx.service.edit_entry(operation).await; + let result = ctx.service.edit_entry(operation, None).await; assert!(result.is_ok()); let updated_entry = result.unwrap(); address_book_entry.metadata = new_metadata_dto; @@ -315,7 +325,7 @@ mod tests { change_metadata: Some(ChangeMetadata::RemoveKeys(remove_keys)), labels: None, }; - let result = ctx.service.edit_entry(operation).await; + let result = ctx.service.edit_entry(operation, None).await; assert!(result.is_ok()); let updated_entry = result.unwrap(); address_book_entry.metadata = new_metadata_dto.into();