Skip to content

Commit 5c804a3

Browse files
gregnazariogregnazario
committed
add security fixes
1 parent 44c9cb0 commit 5c804a3

File tree

14 files changed

+316
-69
lines changed

14 files changed

+316
-69
lines changed

‎crates/aptos-rust-sdk-v2/src/account/multi_ed25519.rs‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -377,6 +377,8 @@ impl Account for MultiEd25519Account {
377377

378378
impl fmt::Debug for MultiEd25519Account {
379379
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
380+
// SECURITY: Intentionally omit private_keys to prevent secret leakage
381+
// in logs, panic messages, or debug output
380382
f.debug_struct("MultiEd25519Account")
381383
.field("address", &self.address)
382384
.field(
@@ -389,7 +391,7 @@ impl fmt::Debug for MultiEd25519Account {
389391
),
390392
)
391393
.field("public_key", &self.public_key)
392-
.field("private_keys", &self.private_keys)
394+
.field("private_keys", &"[REDACTED]")
393395
.finish()
394396
}
395397
}

‎crates/aptos-rust-sdk-v2/src/account/multi_key.rs‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -448,6 +448,8 @@ impl Account for MultiKeyAccount {
448448

449449
impl fmt::Debug for MultiKeyAccount {
450450
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
451+
// SECURITY: Intentionally omit private_keys to prevent secret leakage
452+
// in logs, panic messages, or debug output
451453
f.debug_struct("MultiKeyAccount")
452454
.field("address", &self.address)
453455
.field(
@@ -461,7 +463,7 @@ impl fmt::Debug for MultiKeyAccount {
461463
)
462464
.field("types", &self.key_types())
463465
.field("public_key", &self.public_key)
464-
.field("private_keys", &self.private_keys)
466+
.field("private_keys", &"[REDACTED]")
465467
.finish()
466468
}
467469
}

‎crates/aptos-rust-sdk-v2/src/api/fullnode.rs‎

Lines changed: 52 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -270,6 +270,7 @@ impl FullnodeClient {
270270
let bcs_bytes = signed_txn.to_bcs()?;
271271
let client = self.client.clone();
272272
let retry_config = self.retry_config.clone();
273+
let max_response_size = self.config.pool_config().max_response_size;
273274

274275
let executor = RetryExecutor::new((*retry_config).clone());
275276
executor
@@ -286,7 +287,7 @@ impl FullnodeClient {
286287
.send()
287288
.await?;
288289

289-
Self::handle_response_static(response).await
290+
Self::handle_response_static(response, max_response_size).await
290291
}
291292
})
292293
.await
@@ -400,6 +401,7 @@ impl FullnodeClient {
400401
let bcs_bytes = signed_txn.to_bcs()?;
401402
let client = self.client.clone();
402403
let retry_config = self.retry_config.clone();
404+
let max_response_size = self.config.pool_config().max_response_size;
403405

404406
let executor = RetryExecutor::new((*retry_config).clone());
405407
executor
@@ -416,7 +418,7 @@ impl FullnodeClient {
416418
.send()
417419
.await?;
418420

419-
Self::handle_response_static(response).await
421+
Self::handle_response_static(response, max_response_size).await
420422
}
421423
})
422424
.await
@@ -459,6 +461,7 @@ impl FullnodeClient {
459461

460462
let client = self.client.clone();
461463
let retry_config = self.retry_config.clone();
464+
let max_response_size = self.config.pool_config().max_response_size;
462465

463466
let executor = RetryExecutor::new((*retry_config).clone());
464467
executor
@@ -475,7 +478,7 @@ impl FullnodeClient {
475478
.send()
476479
.await?;
477480

478-
Self::handle_response_static(response).await
481+
Self::handle_response_static(response, max_response_size).await
479482
}
480483
})
481484
.await
@@ -558,12 +561,17 @@ impl FullnodeClient {
558561
fn build_url(&self, path: &str) -> Url {
559562
let mut url = self.config.fullnode_url().clone();
560563
if !path.is_empty() {
561-
// Ensure base path ends with /
562-
if !url.path().ends_with('/') {
563-
url.set_path(&format!("{}/", url.path()));
564+
// Avoid format! allocations by building the path string manually
565+
let base_path = url.path();
566+
let needs_slash = !base_path.ends_with('/');
567+
let new_len = base_path.len() + path.len() + usize::from(needs_slash);
568+
let mut new_path = String::with_capacity(new_len);
569+
new_path.push_str(base_path);
570+
if needs_slash {
571+
new_path.push('/');
564572
}
565-
// Append the path segment
566-
url.set_path(&format!("{}{}", url.path(), path));
573+
new_path.push_str(path);
574+
url.set_path(&new_path);
567575
}
568576
url
569577
}
@@ -575,6 +583,7 @@ impl FullnodeClient {
575583
let client = self.client.clone();
576584
let url_clone = url.clone();
577585
let retry_config = self.retry_config.clone();
586+
let max_response_size = self.config.pool_config().max_response_size;
578587

579588
let executor = RetryExecutor::new((*retry_config).clone());
580589
executor
@@ -588,18 +597,39 @@ impl FullnodeClient {
588597
.send()
589598
.await?;
590599

591-
Self::handle_response_static(response).await
600+
Self::handle_response_static(response, max_response_size).await
592601
}
593602
})
594603
.await
595604
}
596605

597606
/// Handles an HTTP response without retry (for internal use).
607+
///
608+
/// # Security
609+
///
610+
/// This method checks the Content-Length header against `max_response_size`
611+
/// to prevent memory exhaustion from extremely large responses.
598612
async fn handle_response_static<T: for<'de> serde::Deserialize<'de>>(
599613
response: reqwest::Response,
614+
max_response_size: usize,
600615
) -> AptosResult<AptosResponse<T>> {
601616
let status = response.status();
602617

618+
// SECURITY: Check Content-Length to prevent memory exhaustion
619+
// This protects against malicious servers sending extremely large responses
620+
if let Some(content_length) = response.content_length()
621+
&& content_length > max_response_size as u64
622+
{
623+
return Err(AptosError::Api {
624+
status_code: status.as_u16(),
625+
message: format!(
626+
"response body too large: {content_length} bytes exceeds limit of {max_response_size} bytes"
627+
),
628+
error_code: Some("RESPONSE_TOO_LARGE".to_string()),
629+
vm_error_code: None,
630+
});
631+
}
632+
603633
// Extract headers before consuming response body
604634
let ledger_version = response
605635
.headers()
@@ -632,6 +662,13 @@ impl FullnodeClient {
632662
.and_then(|v| v.to_str().ok())
633663
.map(ToString::to_string);
634664

665+
// Extract Retry-After header for rate limiting (before consuming body)
666+
let retry_after_secs = response
667+
.headers()
668+
.get("retry-after")
669+
.and_then(|v| v.to_str().ok())
670+
.and_then(|v| v.parse().ok());
671+
635672
if status.is_success() {
636673
let data: T = response.json().await?;
637674
Ok(AptosResponse {
@@ -643,6 +680,10 @@ impl FullnodeClient {
643680
oldest_ledger_version,
644681
cursor,
645682
})
683+
} else if status.as_u16() == 429 {
684+
// SECURITY: Return specific RateLimited error with Retry-After info
685+
// This allows callers to respect the server's rate limiting
686+
Err(AptosError::RateLimited { retry_after_secs })
646687
} else {
647688
let body: serde_json::Value = response.json().await.unwrap_or_default();
648689
let message = body
@@ -673,7 +714,8 @@ impl FullnodeClient {
673714
&self,
674715
response: reqwest::Response,
675716
) -> AptosResult<AptosResponse<T>> {
676-
Self::handle_response_static(response).await
717+
let max_response_size = self.config.pool_config().max_response_size;
718+
Self::handle_response_static(response, max_response_size).await
677719
}
678720
}
679721

‎crates/aptos-rust-sdk-v2/src/aptos.rs‎

Lines changed: 29 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -213,8 +213,13 @@ impl Aptos {
213213
sender: &A,
214214
payload: TransactionPayload,
215215
) -> AptosResult<RawTransaction> {
216-
let sequence_number = self.get_sequence_number(sender.address()).await?;
217-
let gas_estimation = self.fullnode.estimate_gas_price().await?;
216+
// Fetch sequence number and gas price in parallel - they're independent
217+
let (sequence_number, gas_estimation) = tokio::join!(
218+
self.get_sequence_number(sender.address()),
219+
self.fullnode.estimate_gas_price()
220+
);
221+
let sequence_number = sequence_number?;
222+
let gas_estimation = gas_estimation?;
218223

219224
TransactionBuilder::new()
220225
.sender(sender.address())
@@ -542,16 +547,29 @@ impl Aptos {
542547
.ok_or_else(|| AptosError::FeatureNotEnabled("faucet".into()))?;
543548
let txn_hashes = faucet.fund(address, amount).await?;
544549

545-
// Wait for all faucet transactions to be confirmed
546-
for hash_str in &txn_hashes {
547-
// Hash might have 0x prefix or not
548-
let hash_str_clean = hash_str.strip_prefix("0x").unwrap_or(hash_str);
549-
if let Ok(hash) = HashValue::from_hex(hash_str_clean) {
550-
// Wait for the transaction to be confirmed
550+
// Parse hashes first to own them
551+
let hashes: Vec<HashValue> = txn_hashes
552+
.iter()
553+
.filter_map(|hash_str| {
554+
// Hash might have 0x prefix or not
555+
let hash_str_clean = hash_str.strip_prefix("0x").unwrap_or(hash_str);
556+
HashValue::from_hex(hash_str_clean).ok()
557+
})
558+
.collect();
559+
560+
// Wait for all faucet transactions to be confirmed in parallel
561+
let wait_futures: Vec<_> = hashes
562+
.iter()
563+
.map(|hash| {
551564
self.fullnode
552-
.wait_for_transaction(&hash, Some(Duration::from_secs(60)))
553-
.await?;
554-
}
565+
.wait_for_transaction(hash, Some(Duration::from_secs(60)))
566+
})
567+
.collect();
568+
569+
// Wait for all transactions in parallel
570+
let results = futures::future::join_all(wait_futures).await;
571+
for result in results {
572+
result?;
555573
}
556574

557575
Ok(txn_hashes)

‎crates/aptos-rust-sdk-v2/src/codegen/types.rs‎

Lines changed: 11 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -155,16 +155,17 @@ impl MoveTypeMapper {
155155

156156
// Handle generic struct types (e.g., 0x1::coin::Coin<0x1::aptos_coin::AptosCoin>)
157157
if move_type.contains("::") {
158-
// Extract the struct name for the Rust type
159-
let parts: Vec<&str> = move_type.split("::").collect();
160-
if parts.len() >= 3 {
161-
// Get the base struct name (without generics)
162-
let struct_name = parts.last().unwrap();
163-
let base_name = struct_name.split('<').next().unwrap_or(struct_name);
164-
165-
// Create a pascal case name
166-
let rust_name = to_pascal_case(base_name);
167-
return RustType::new(rust_name).with_doc(format!("Move type: {move_type}"));
158+
// Use rsplit to avoid collecting into Vec - we only need the last part
159+
let part_count = move_type.matches("::").count() + 1;
160+
if part_count >= 3 {
161+
// Get the base struct name (without generics) using rsplit
162+
if let Some(struct_name) = move_type.rsplit("::").next() {
163+
let base_name = struct_name.split('<').next().unwrap_or(struct_name);
164+
165+
// Create a pascal case name
166+
let rust_name = to_pascal_case(base_name);
167+
return RustType::new(rust_name).with_doc(format!("Move type: {move_type}"));
168+
}
168169
}
169170
}
170171

0 commit comments

Comments
 (0)