Various performance and security fixes

[gregnazario]

Summary

Comprehensive security hardening and performance improvements based on a 3-pass security audit of the SDK. The audit report is included as SECURITY_AUDIT.md.

Security hardening

• Streaming response bounds: All HTTP response reads now use read_response_bounded() which pre-checks Content-Length and reads incrementally via chunked streaming, aborting early if the size limit is exceeded. This prevents OOM from malicious servers using chunked transfer-encoding. Error body reads are also bounded.
• Low-S ECDSA enforcement: Both secp256k1 and secp256r1 now enforce low-S signatures in signing, parsing (from_bytes/from_hex), and verification to match aptos-core's on-chain behavior and prevent signature malleability.
• Codegen injection prevention: ABI-derived strings are sanitized via sanitize_abi_string() before embedding in generated Rust code. Module names are validated against path traversal and Rust keywords. format_ident! calls are guarded with safe_format_ident() using raw identifiers for keywords.
• Zeroization: Entropy, seed, and intermediate key material in mnemonic derivation are explicitly zeroized. Pepper type uses ZeroizeOnDrop with a redacted Debug impl. Ed25519 private key buffer is zeroized after construction.
• URL scheme validation: All API client constructors (with_url) validate URL schemes to prevent SSRF via file://, gopher://, etc.
• Error sanitization: sanitized_message() redacts sensitive patterns (private keys, mnemonics, JWTs, API keys) and URL query parameters. Error body truncation prevents log flooding. UTF-8 boundary-safe truncation.
• Proc macro safety: aptos_contract_file! canonicalizes paths and verifies they remain under CARGO_MANIFEST_DIR to prevent path traversal at compile time.

Performance improvements

• AtomicU8 for chain ID: Replaced RwLock<u8> with AtomicU8 for chain ID caching, eliminating lock contention on every transaction build.
• Shared retry config: RetryConfig stored in Arc with from_shared() to avoid cloning on each request.
• Reduced allocations: const_hex::encode_prefixed for hex encoding instead of format!("0x{}", ...). Removed unnecessary payload clones in transaction building.
• Default response limit: Lowered from 100 MB to 10 MB (normal responses are under 1 MB).

Bug fixes

• Fixed sponsored transaction default expiration being applied twice (20 min → 10 min).
• Fixed generated BCS arg encoding using .unwrap() → proper error propagation.
• Fixed MoveSourceParser lacking input size limit.

Test plan

• cargo test -p aptos-sdk --all-features — 884 tests pass
• cargo test -p aptos-sdk-macros — trybuild UI tests pass
• cargo clippy -p aptos-sdk --all-features -- -D warnings — clean
• cargo clippy -p aptos-sdk-macros -- -D warnings — clean
• cargo fmt -- --check — clean

[Copilot]

Pull request overview

This PR bundles a set of hardening and efficiency improvements across the Aptos Rust SDK, focusing on safer defaults, reduced allocations/locking, and tighter handling of untrusted inputs (HTTP responses, ABIs, JWT/OIDC metadata).

Changes:

• Fixes/adjusts transaction expiration handling and reduces cloning/locking overhead (Atomic chain id, shared retry config).
• Adds multiple security hardenings: low-S ECDSA enforcement, zeroization of sensitive buffers, URL scheme validation, ABI/path traversal protections, and error-message sanitization.
• Introduces response-size limits and other allocation reductions in API clients and codegen parsing.

Reviewed changes

Copilot reviewed 23 out of 23 changed files in this pull request and generated 10 comments.

Show a summary per file

File	Description
crates/aptos-sdk/src/transaction/sponsored.rs	Fixes default expiration calculation to avoid double-applying the offset.
crates/aptos-sdk/src/transaction/batch.rs	Replaces RwLock chain-id caching with AtomicU8 for lower overhead.
crates/aptos-sdk/src/retry.rs	Stores retry config in Arc and adds from_shared to avoid clones.
crates/aptos-sdk/src/error.rs	Expands sensitive-pattern redaction and adds URL-with-query redaction in sanitized_message().
crates/aptos-sdk/src/crypto/secp256r1.rs	Enforces/validates low‑S signatures (sign/parse/verify) and adds tests.
crates/aptos-sdk/src/crypto/secp256k1.rs	Enforces/validates low‑S signatures (sign/parse/verify) and adds tests.
crates/aptos-sdk/src/crypto/ed25519.rs	Zeroizes a temporary private-key buffer after constructing the signing key.
crates/aptos-sdk/src/config.rs	Makes URL scheme validation public + reduces default max response size to 10MB.
crates/aptos-sdk/src/codegen/types.rs	Generates BCS arg encoding with error propagation instead of unwrap().
crates/aptos-sdk/src/codegen/move_parser.rs	Adds a max Move source size limit and reduces doc parsing allocations.
crates/aptos-sdk/src/codegen/generator.rs	Adds ABI string sanitization for some emitted docs/attributes.
crates/aptos-sdk/src/codegen/build_helper.rs	Validates module names to prevent path traversal and mod.rs injection.
crates/aptos-sdk/src/aptos.rs	Uses AtomicU8 for chain-id caching and removes unnecessary payload clones.
crates/aptos-sdk/src/api/indexer.rs	Adds URL scheme validation and response-size checks; uses shared retry config.
crates/aptos-sdk/src/api/fullnode.rs	Adds error-body truncation and response-size enforcement; uses shared retry config.
crates/aptos-sdk/src/api/faucet.rs	Adds URL scheme validation and response-size checks; uses shared retry config.
crates/aptos-sdk/src/account/mnemonic.rs	Switches seed derivation to return Result and zeroizes entropy/seed/key material.
crates/aptos-sdk/src/account/keyless.rs	Adds HTTPS-only JWKS handling + response-size checks; zeroizes Pepper on drop.
crates/aptos-sdk/examples/account_management.rs	Adds clearer “demo only” warning when printing a private key.
crates/aptos-sdk-macros/src/lib.rs	Canonical-path check to prevent path traversal in compile-time ABI file reads.
crates/aptos-sdk-macros/src/codegen.rs	Validates ABI-derived identifiers before format_ident! to avoid panics/injection.
SECURITY_AUDIT.md	Adds a security audit report documenting findings/remediations.
CLAUDE.md	Updates the referenced location of behavioral tests.

Comments suppressed due to low confidence (2)

crates/aptos-sdk/src/config.rs:29

• validate_url_scheme docs state that HTTP is allowed for localhost development only, but the implementation currently accepts any http:// URL (no host check). Either enforce the localhost-only restriction (e.g., allow only localhost, 127.0.0.1, ::1) or update the documentation to match the actual behavior.

/// Validates that a URL uses a safe scheme (http or https).
///
/// # Security
///
/// This prevents SSRF attacks via dangerous URL schemes like `file://`, `gopher://`, etc.
/// For production use, HTTPS is strongly recommended. HTTP is allowed for localhost
/// development only.
/// # Errors
///
/// Returns [`AptosError::Config`] if the URL scheme is not `http` or `https`.
pub fn validate_url_scheme(url: &Url) -> AptosResult<()> {
    match url.scheme() {
        "https" => Ok(()),
        "http" => {
            // HTTP is allowed but only recommended for localhost development
            // Log a warning in the future if we add logging
            Ok(())
        }

crates/aptos-sdk/src/error.rs:317

• sanitize_string truncates with &cleaned[..MAX_ERROR_MESSAGE_LENGTH], which can panic if MAX_ERROR_MESSAGE_LENGTH lands in the middle of a UTF-8 codepoint. Please truncate at a valid is_char_boundary (similar to truncate_error_body) or use chars().take(...) to guarantee this never panics while sanitizing/logging errors.

        // Truncate if too long
        if cleaned.len() > MAX_ERROR_MESSAGE_LENGTH {
            format!(
                "{}... [truncated, total length: {}]",
                &cleaned[..MAX_ERROR_MESSAGE_LENGTH],
                cleaned.len()
            )

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

sanitize_abi_string is introduced, but several ABI-derived values are still written into generated Rust string literals/comments without escaping (e.g., header //! Generated ..., MODULE_ADDRESS/MODULE_NAME constants, is_module_event prefix, and function_id construction). A crafted ABI containing quotes/backslashes/newlines could still break the generated syntax. Please apply escaping/sanitization consistently anywhere ABI strings are emitted into Rust string literals or comments (not just doc lines/serde attributes).

[Copilot]

Pepper is documented as secret material, but it derives Debug, which will print the inner bytes and can leak the pepper via logs/panic messages. Please remove Debug from the derive list or implement a custom Debug that redacts the value (similar to the private key types).

Suggested change

	#[derive(Clone, Debug, PartialEq, Eq, zeroize::Zeroize, zeroize::ZeroizeOnDrop)]
	pub struct Pepper(Vec<u8>);
	#[derive(Clone, PartialEq, Eq, zeroize::Zeroize, zeroize::ZeroizeOnDrop)]
	pub struct Pepper(Vec<u8>);
	impl fmt::Debug for Pepper {
	fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
	write!(f, "Pepper(REDACTED)")
	}
	}

[Copilot]

The ledger_info rustdoc (just above this block) still refers to panicking on a poisoned chain_id lock, but chain_id is now stored in an AtomicU8 (no lock). Please remove/update that # Panics section to keep the public API docs accurate.

[Copilot]

The max_response_size field docs still mention a 100 MB default (see the comment immediately above this field), but the default constant was changed to 10 MB below. Please update the field-level documentation to reflect the new default to avoid confusing users.

[Copilot]

from_issuer docs say “Only HTTPS issuers are accepted”, but the implementation still returns OidcProvider::Custom for non-HTTPS issuers (with an empty jwks_url) and only fails later when fetching/parsing. Please either (a) adjust the docs to clarify that non-HTTPS issuers result in a provider that will error at JWKS fetch time, or (b) change the API to return a Result (if acceptable) so rejection happens at construction time.

[Copilot]

Pull request overview

Copilot reviewed 23 out of 23 changed files in this pull request and generated 9 comments.

Comments suppressed due to low confidence (1)

crates/aptos-sdk/src/config.rs:28

• The docs claim "HTTP is allowed for localhost development only", but validate_url_scheme currently allows http for any host. Since this function is now pub and used as a security control, the documentation should either match the behavior (scheme-only validation) or the function should enforce the localhost-only constraint (host check) if that’s the intent.

/// This prevents SSRF attacks via dangerous URL schemes like `file://`, `gopher://`, etc.
/// For production use, HTTPS is strongly recommended. HTTP is allowed for localhost
/// development only.
/// # Errors
///
/// Returns [`AptosError::Config`] if the URL scheme is not `http` or `https`.
pub fn validate_url_scheme(url: &Url) -> AptosResult<()> {
    match url.scheme() {
        "https" => Ok(()),
        "http" => {
            // HTTP is allowed but only recommended for localhost development
            // Log a warning in the future if we add logging
            Ok(())

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

In the non-success path, response.text().await buffers the full error body before truncation and JSON parsing. This still leaves an OOM/memory-exhaustion vector on large error bodies. Consider reading the error body with a hard cap (streaming with a limit, or at least rejecting large Content-Length values before calling text()).

Suggested change

	// SECURITY: Truncate error body to prevent storing excessively
	// large error messages from malicious servers
	let error_text = Self::truncate_error_body(response.text().await.unwrap_or_default());
	// SECURITY: Avoid unbounded buffering of large error bodies.
	// First, use Content-Length (if present) to reject excessively
	// large error responses before calling `text()`, then truncate.
	const MAX_ERROR_BODY_SIZE: u64 = 64 * 1024; // 64 KiB cap for error bodies
	let error_text = if let Some(len) = response.content_length() {
	if len > MAX_ERROR_BODY_SIZE {
	// Do not read the body at all if it is declared too large.
	"error body too large to display".to_string()
	} else {
	let text = response.text().await.unwrap_or_default();
	Self::truncate_error_body(text)
	}
	} else {
	// Unknown length (e.g., chunked): read then truncate as before.
	let text = response.text().await.unwrap_or_default();
	Self::truncate_error_body(text)
	};

[Copilot]

The URL-query redaction logic checks for any '?' anywhere in the message whenever it contains "http://" or "https://". This can over-redact unrelated messages that happen to include a question mark. Consider extracting actual URLs and checking whether they contain a query component (e.g., parse substrings as Url and inspect query()), or at least ensure the '?' occurs within a URL token.

[Copilot]

The truncation path slices &cleaned[..MAX_ERROR_MESSAGE_LENGTH] at a fixed byte offset, which can panic if the string contains multi-byte UTF-8 characters (not a char boundary). Since sanitized_message() is intended to be safe for logging, it should not be able to panic. Consider truncating on a valid UTF-8 boundary (similar to truncate_error_body) or using char_indices() to compute the cutoff safely.

[Copilot]

The report states "All findings remediated" / "21 of 22 fixed", but some of the size-limit remediations still buffer full response bodies into memory (e.g., using response.bytes() / response.text() and checking/truncating afterward). Consider updating this status language (or the remediation details) so the report doesn’t overstate the current protection against OOM on large/chunked responses.

[Copilot]

Pull request overview

Copilot reviewed 23 out of 23 changed files in this pull request and generated 5 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Sanitization is only applied to some ABI-derived strings. Other generated-code emission sites still interpolate self.abi.name / self.abi.address (and function names) into doc comments and string literals without escaping (e.g., header/module constants/function-id construction), which can still enable code injection or broken output with crafted ABI JSON. Consider applying sanitize_abi_string() (or stricter validation) consistently anywhere ABI data is written into generated Rust source.

[Copilot]

read_response_bounded() is a new security-critical primitive (size limiting, chunked handling) but there are no unit tests covering its behavior. Since this file already has tests, consider adding tests for: (1) rejecting oversized Content-Length, and (2) rejecting responses that exceed the limit via chunked streaming.

[Copilot]

Pull request overview

Copilot reviewed 23 out of 23 changed files in this pull request and generated 5 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The module-level docs say “Signing always produces low-S signatures (the p256 crate normalizes by default)”, but the implementation immediately below states “The p256 crate does not guarantee low-S output from signing” and performs explicit normalization. Please align the documentation with the actual behavior/guarantees (either drop the “by default” claim or clarify that the SDK normalizes regardless of crate behavior).

Suggested change

	//! - **Signing** always produces low-S signatures (the `p256` crate normalizes
	//! by default).
	//! - **Signing** always produces low-S signatures (normalized by this SDK,
	//! independent of the `p256` crate's default behavior).

[Copilot]

Mnemonic::to_seed changed from returning [u8; 64] to AptosResult<[u8; 64]>, which is a breaking public API change. If this is intentional, it should be called out in release notes / changelog and any docs/examples should be updated to handle the Result (or consider adding a new fallible method and keeping the infallible one for backward compatibility).

[Copilot]

read_response_bounded uses body.len() + chunk.len() for the size check; in release builds this addition can wrap on usize overflow and potentially bypass the limit. Use checked_add/saturating_add (or compare chunk.len() > max_size - body.len()) to make the bound robust for all max_size values.

Suggested change

	if body.len() + chunk.len() > max_size {
	let new_len = match body.len().checked_add(chunk.len()) {
	Some(len) => len,
	None => {
	// Overflow in length calculation implies the response is effectively too large.
	return Err(AptosError::Api {
	status_code: response.status().as_u16(),
	message: format!(
	"response too large: exceeded limit of {max_size} bytes during streaming"
	),
	error_code: Some("RESPONSE_TOO_LARGE".into()),
	vm_error_code: None,
	});
	}
	};
	if new_len > max_size {

[Copilot]

validate_module_name is documented as enforcing a “valid Rust identifier”, but it doesn’t reject Rust keywords (e.g., mod, crate, self), which can still produce invalid mod.rs/file output or macro expansions. Consider checking against the Rust keyword set (or using syn::parse_str::<Ident>() / proc_macro2::Ident validation) and either reject or emit r#... raw identifiers.

[Copilot]

safe_format_ident can still panic on Rust keywords because validate_rust_ident only checks character classes; format_ident!("{}", "fn") (or mod, crate, etc.) will panic inside proc_macro2::Ident::new. Since this helper is explicitly meant to prevent panics on malformed ABI, add a keyword check and either return a compile error or generate a raw identifier (Ident::new_raw / r#...).

Suggested change

	/// Safely creates a `format_ident!` from a string, returning a compile error if invalid.
	fn safe_format_ident(name: &str) -> Result<proc_macro2::Ident, TokenStream> {
	if let Err(e) = validate_rust_ident(name) {
	return Err(syn::Error::new(proc_macro2::Span::call_site(), e).to_compile_error());
	}
	/// Returns true if `name` is a reserved Rust keyword.
	fn is_rust_keyword(name: &str) -> bool {
	matches!(
	name,
	// Strict keywords
	"as"
	| "break"
	| "const"
	| "continue"
	| "crate"
	| "else"
	| "enum"
	| "extern"
	| "false"
	| "fn"
	| "for"
	| "if"
	| "impl"
	| "in"
	| "let"
	| "loop"
	| "match"
	| "mod"
	| "move"
	| "mut"
	| "pub"
	| "ref"
	| "return"
	| "self"
	| "Self"
	| "static"
	| "struct"
	| "super"
	| "trait"
	| "true"
	| "type"
	| "unsafe"
	| "use"
	| "where"
	| "while"
	// 2018+ keywords and common contextual keywords that are rejected by `Ident::new`
	| "async"
	| "await"
	| "dyn"
	| "abstract"
	| "become"
	| "box"
	| "do"
	| "final"
	| "macro"
	| "override"
	| "priv"
	| "try"
	| "typeof"
	| "unsized"
	| "virtual"
	| "yield"
	)
	}
	/// Safely creates a `format_ident!` from a string, returning a compile error if invalid.
	fn safe_format_ident(name: &str) -> Result<proc_macro2::Ident, TokenStream> {
	if let Err(e) = validate_rust_ident(name) {
	return Err(syn::Error::new(proc_macro2::Span::call_site(), e).to_compile_error());
	}
	// If this is a Rust keyword, construct a raw identifier to avoid panics from `Ident::new`.
	if is_rust_keyword(name) {
	return Ok(proc_macro2::Ident::new_raw(
	name,
	proc_macro2::Span::call_site(),
	));
	}

[Copilot]

Pull request overview

Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The path traversal protection has a security gap: if either manifest_path.canonicalize() or file_path.canonicalize() fails (returns Err), the validation is silently skipped and the file read proceeds. An attacker could exploit this by referencing a path that doesn't exist yet but would be created later, or by triggering permission errors that prevent canonicalization.

Consider handling canonicalization failures explicitly:

let canonical_manifest = manifest_path.canonicalize().map_err(|e| {
    syn::Error::new(
        input.name.span(),
        format!("Failed to resolve project directory: {e}")
    )
})?;
let canonical_file = file_path.canonicalize().map_err(|e| {
    syn::Error::new(
        input.name.span(),
        format!("Failed to resolve ABI file path: {e}")
    )
})?;
if !canonical_file.starts_with(&canonical_manifest) {
    return syn::Error::new(...).to_compile_error().into();
}

This ensures the validation always runs and provides clear error messages when canonicalization fails.

[Copilot]

Pull request overview

Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The URL query parameter detection uses lower.find(scheme) to find the scheme position, but then uses indices from the lowercased string to check for '?'. This could theoretically cause issues if the lowercasing changes byte positions (though unlikely for ASCII URLs). More importantly, after finding a URL with query parameters, the function returns immediately without checking if there might be multiple URLs in the error message. While this is conservative (redacting the entire message if any URL has query params), it might be overly aggressive. Consider documenting this behavior or checking all URLs before deciding to redact.