fix: address remaining PR review comments

- Pepper: implement custom Debug that redacts secret bytes instead of
  deriving Debug which would leak pepper material in logs/panics
- Error body OOM: use read_response_bounded() for error paths in
  fullnode.rs (view_bcs and handle_response_static) instead of
  unbounded response.text().await
- Codegen sanitization: apply sanitize_abi_string() to all remaining
  ABI-derived values (header, MODULE_ADDRESS/MODULE_NAME constants,
  function_id format strings, event type constants, is_module_event)
- URL redaction: fix over-redaction by only checking for '?' within
  the URL token itself, not anywhere in the message
- Tests: add 5 unit tests for read_response_bounded() covering normal,
  oversized Content-Length, oversized body, exact limit, and empty
- SECURITY_AUDIT.md: update report to reflect streaming body reads

Co-authored-by: Cursor <cursoragent@cursor.com>

Author: gregnazario

SECURITY_AUDIT.md

@@ -3,7 +3,7 @@
 **Date:** 2026-02-09
 **Scope:** Full SDK (`aptos-sdk` v0.3.0, `aptos-sdk-macros` v0.1.0)
 **Method:** 3-pass audit (automated surface scan, deep manual review, design-level assessment)
-**Status:** All findings remediated (21 of 22 fixed; F-21 deferred as large effort)
+**Status:** All findings remediated (21 of 22 fixed; F-21 deferred as large effort). Response body reads now use incremental streaming with size limits (`read_response_bounded`) to prevent OOM from chunked transfer-encoding.
 
 ---
 
@@ -353,7 +353,7 @@ The SDK operates with the following trust boundaries:
 
 ### 3b. Missing Hardening
 
-1. **Response body streaming** -- All response reads load full bodies into memory. No streaming with incremental size checks. (Addresses F-02, F-03, F-17)
+1. **Response body streaming** -- All response reads now use `read_response_bounded()` which pre-checks `Content-Length` and reads incrementally via `response.chunk()`, aborting early if the size limit is exceeded. Error body reads are also bounded. (Addresses F-02, F-03, F-17)
 2. **Constant-time operations** -- Signature verification delegates to underlying crates (ed25519-dalek, k256, p256) which use constant-time comparison. The SDK itself does not perform any custom constant-time operations, which is correct.
 3. **Fuzz testing** -- Infrastructure exists but is unused (F-21).
 4. **Side-channel resistance** -- Signing operations use library implementations with side-channel resistance. Non-security-critical operations (address parsing, ABI processing) are not constant-time, which is acceptable.

crates/aptos-sdk/src/account/keyless.rs

@@ -176,9 +176,15 @@ impl OidcProvider {
 /// The pepper is secret material used to derive keyless account addresses.
 /// It is automatically zeroized when dropped to prevent key material from
 /// lingering in memory.
-#[derive(Clone, Debug, PartialEq, Eq, zeroize::Zeroize, zeroize::ZeroizeOnDrop)]
+#[derive(Clone, PartialEq, Eq, zeroize::Zeroize, zeroize::ZeroizeOnDrop)]
 pub struct Pepper(Vec<u8>);
 
+impl std::fmt::Debug for Pepper {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        write!(f, "Pepper(REDACTED)")
+    }
+}
+
 impl Pepper {
     /// Creates a new pepper from raw bytes.
     pub fn new(bytes: Vec<u8>) -> Self {

crates/aptos-sdk/src/api/fullnode.rs

@@ -556,13 +556,18 @@ impl FullnodeClient {
                     // Check for errors before reading body
                     let status = response.status();
                     if !status.is_success() {
-                        // 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: Bound error body reads to prevent OOM from
+                        // malicious servers sending huge error responses.
+                        let error_bytes =
+                            crate::config::read_response_bounded(response, MAX_ERROR_BODY_SIZE)
+                                .await
+                                .ok();
+                        let error_text = error_bytes
+                            .and_then(|b| String::from_utf8(b).ok())
+                            .unwrap_or_default();
                         return Err(AptosError::Api {
                             status_code: status.as_u16(),
-                            message: error_text,
+                            message: Self::truncate_error_body(error_text),
                             error_code: None,
                             vm_error_code: None,
                         });
@@ -790,9 +795,15 @@ impl FullnodeClient {
             // This allows callers to respect the server's rate limiting
             Err(AptosError::RateLimited { retry_after_secs })
         } else {
-            // 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: Bound error body reads to prevent OOM from malicious
+            // servers sending huge error responses (including chunked encoding).
+            let error_bytes = crate::config::read_response_bounded(response, MAX_ERROR_BODY_SIZE)
+                .await
+                .ok();
+            let error_text = error_bytes
+                .and_then(|b| String::from_utf8(b).ok())
+                .unwrap_or_default();
+            let error_text = Self::truncate_error_body(error_text);
             let body: serde_json::Value = serde_json::from_str(&error_text).unwrap_or_default();
             let message = body
                 .get("message")

crates/aptos-sdk/src/codegen/generator.rs

@@ -248,7 +248,10 @@ impl<'a> ModuleGenerator<'a> {
             writeln!(
                 output,
                 "    pub const {}: &str = \"{}::{}::{}\";",
-                const_name, self.abi.address, self.abi.name, struct_def.name
+                const_name,
+                sanitize_abi_string(&self.abi.address),
+                sanitize_abi_string(&self.abi.name),
+                sanitize_abi_string(&struct_def.name)
             )?;
         }
         writeln!(output, "}}")?;
@@ -299,7 +302,8 @@ impl<'a> ModuleGenerator<'a> {
         writeln!(
             output,
             "    event_type.starts_with(\"{}::{}::\")",
-            self.abi.address, self.abi.name
+            sanitize_abi_string(&self.abi.address),
+            sanitize_abi_string(&self.abi.name)
         )?;
         writeln!(output, "}}")?;
         writeln!(output)
@@ -310,7 +314,8 @@ impl<'a> ModuleGenerator<'a> {
         writeln!(
             output,
             "//! Generated Rust bindings for `{}::{}`.",
-            self.abi.address, self.abi.name
+            sanitize_abi_string(&self.abi.address),
+            sanitize_abi_string(&self.abi.name)
         )?;
         writeln!(output, "//!")?;
         writeln!(
@@ -347,14 +352,14 @@ impl<'a> ModuleGenerator<'a> {
         writeln!(
             output,
             "pub const MODULE_ADDRESS: &str = \"{}\";",
-            self.abi.address
+            sanitize_abi_string(&self.abi.address)
         )?;
         writeln!(output)?;
         writeln!(output, "/// The module name.")?;
         writeln!(
             output,
             "pub const MODULE_NAME: &str = \"{}\";",
-            self.abi.name
+            sanitize_abi_string(&self.abi.name)
         )?;
         writeln!(output)
     }
@@ -569,11 +574,14 @@ impl<'a> ModuleGenerator<'a> {
         writeln!(output, ") -> AptosResult<TransactionPayload> {{")?;
 
         // Function body
+        // SECURITY: Sanitize ABI-derived values embedded in string literals
         writeln!(output, "    let function_id = format!(")?;
         writeln!(
             output,
             "        \"{}::{}::{}\",",
-            self.abi.address, self.abi.name, func.name
+            sanitize_abi_string(&self.abi.address),
+            sanitize_abi_string(&self.abi.name),
+            sanitize_abi_string(&func.name)
         )?;
         writeln!(output, "    );")?;
         writeln!(output)?;
@@ -774,11 +782,14 @@ impl<'a> ModuleGenerator<'a> {
         writeln!(output, ") -> AptosResult<Vec<serde_json::Value>> {{")?;
 
         // Function body
+        // SECURITY: Sanitize ABI-derived values embedded in string literals
         writeln!(output, "    let function_id = format!(")?;
         writeln!(
             output,
             "        \"{}::{}::{}\",",
-            self.abi.address, self.abi.name, func.name
+            sanitize_abi_string(&self.abi.address),
+            sanitize_abi_string(&self.abi.name),
+            sanitize_abi_string(&func.name)
         )?;
         writeln!(output, "    );")?;
         writeln!(output)?;

crates/aptos-sdk/src/config.rs

@@ -876,4 +876,83 @@ mod tests {
         assert!(config.pool_config().max_idle_total > 0);
         assert_eq!(config.chain_id().id(), 2);
     }
+
+    #[tokio::test]
+    async fn test_read_response_bounded_normal() {
+        use wiremock::{Mock, MockServer, ResponseTemplate, matchers::method};
+        let server = MockServer::start().await;
+        Mock::given(method("GET"))
+            .respond_with(ResponseTemplate::new(200).set_body_string("hello world"))
+            .mount(&server)
+            .await;
+
+        let response = reqwest::get(server.uri()).await.unwrap();
+        let body = read_response_bounded(response, 1024).await.unwrap();
+        assert_eq!(body, b"hello world");
+    }
+
+    #[tokio::test]
+    async fn test_read_response_bounded_rejects_oversized_content_length() {
+        use wiremock::{Mock, MockServer, ResponseTemplate, matchers::method};
+        let server = MockServer::start().await;
+        // Send a body whose accurate Content-Length exceeds the limit.
+        // The function should reject based on Content-Length pre-check
+        // before streaming the full body.
+        let body = "x".repeat(200);
+        Mock::given(method("GET"))
+            .respond_with(ResponseTemplate::new(200).set_body_string(body))
+            .mount(&server)
+            .await;
+
+        let response = reqwest::get(server.uri()).await.unwrap();
+        // Limit is 100 but body is 200 -- should be rejected via Content-Length pre-check
+        let result = read_response_bounded(response, 100).await;
+        assert!(result.is_err());
+        let err = result.unwrap_err().to_string();
+        assert!(err.contains("response too large"));
+    }
+
+    #[tokio::test]
+    async fn test_read_response_bounded_rejects_oversized_body() {
+        use wiremock::{Mock, MockServer, ResponseTemplate, matchers::method};
+        let server = MockServer::start().await;
+        let large_body = "x".repeat(500);
+        Mock::given(method("GET"))
+            .respond_with(ResponseTemplate::new(200).set_body_string(large_body))
+            .mount(&server)
+            .await;
+
+        let response = reqwest::get(server.uri()).await.unwrap();
+        let result = read_response_bounded(response, 100).await;
+        assert!(result.is_err());
+    }
+
+    #[tokio::test]
+    async fn test_read_response_bounded_exact_limit() {
+        use wiremock::{Mock, MockServer, ResponseTemplate, matchers::method};
+        let server = MockServer::start().await;
+        let body = "x".repeat(100);
+        Mock::given(method("GET"))
+            .respond_with(ResponseTemplate::new(200).set_body_string(body.clone()))
+            .mount(&server)
+            .await;
+
+        let response = reqwest::get(server.uri()).await.unwrap();
+        let result = read_response_bounded(response, 100).await.unwrap();
+        assert_eq!(result.len(), 100);
+    }
+
+    #[tokio::test]
+    async fn test_read_response_bounded_empty() {
+        use wiremock::{Mock, MockServer, ResponseTemplate, matchers::method};
+        let server = MockServer::start().await;
+        Mock::given(method("GET"))
+            .respond_with(ResponseTemplate::new(200))
+            .mount(&server)
+            .await;
+
+        let response = reqwest::get(server.uri()).await.unwrap();
+        let result = read_response_bounded(response, 1024).await.unwrap();
+        assert!(result.is_empty());
+    }
 }

crates/aptos-sdk/src/error.rs

@@ -301,10 +301,21 @@ impl AptosError {
         // SECURITY: Redact URLs with query parameters, which may contain API keys
         // or other credentials not caught by keyword patterns above.
         // e.g., reqwest errors include the request URL.
-        if lower.contains("http://") || lower.contains("https://") {
-            // Check if the URL has a query string (contains '?' after the scheme)
-            if lower.contains('?') {
-                return "[REDACTED: message contained URL with query parameters]".into();
+        // Only redact when '?' appears within a URL token (after the scheme),
+        // not just anywhere in the message.
+        for scheme in ["http://", "https://"] {
+            if let Some(scheme_pos) = lower.find(scheme) {
+                // Look for '?' after the scheme, within the URL token
+                // (URLs end at whitespace or common delimiters)
+                let url_start = scheme_pos;
+                let url_rest = &lower[url_start..];
+                let url_end = url_rest
+                    .find(|c: char| c.is_whitespace() || c == '>' || c == '"' || c == '\'')
+                    .unwrap_or(url_rest.len());
+                let url_token = &url_rest[..url_end];
+                if url_token.contains('?') {
+                    return "[REDACTED: message contained URL with query parameters]".into();
+                }
             }
         }