Do not log PII in Debug implementations

Related to #1707 but does not resolve it. Still need a solution to redact headers as needed.

sdk/core/azure_core/src/credentials.rs

@@ -57,7 +57,7 @@ impl From<&'static str> for Secret {
 
 impl Debug for Secret {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        f.debug_tuple("Secret").field(&"<REDACTED>").finish()
+        f.write_str("Secret")
     }
 }
 

sdk/typespec/src/error/mod.rs

@@ -54,19 +54,16 @@ impl Display for ErrorKind {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         match self {
             #[cfg(feature = "http")]
-            ErrorKind::HttpResponse { status, error_code } => {
-                write!(
-                    f,
-                    "HttpResponse({},{})",
-                    status,
-                    error_code.as_deref().unwrap_or("unknown")
-                )
-            }
-            ErrorKind::Io => write!(f, "Io"),
-            ErrorKind::DataConversion => write!(f, "DataConversion"),
-            ErrorKind::Credential => write!(f, "Credential"),
-            ErrorKind::MockFramework => write!(f, "MockFramework"),
-            ErrorKind::Other => write!(f, "Other"),
+            ErrorKind::HttpResponse { status, error_code } => f
+                .debug_tuple("HttpResponse")
+                .field(status)
+                .field(&error_code.as_deref().unwrap_or("(unknown error code)"))
+                .finish(),
+            ErrorKind::Io => f.write_str("Io"),
+            ErrorKind::DataConversion => f.write_str("DataConversion"),
+            ErrorKind::Credential => f.write_str("Credential"),
+            ErrorKind::MockFramework => f.write_str("MockFramework"),
+            ErrorKind::Other => f.write_str("Other"),
         }
     }
 }
@@ -194,10 +191,7 @@ impl Error {
             return Err(self);
         }
         // Unwrapping is ok here since we already check above that the downcast will work
-        Ok(*self
-            .into_inner()?
-            .downcast()
-            .expect("failed to unwrap downcast"))
+        Ok(*self.into_inner()?.downcast().expect("downcast is Some(T)"))
     }
 
     /// Returns a reference to the inner error wrapped by this error, if any.

sdk/typespec/typespec_client_core/src/error/http_error.rs

@@ -13,11 +13,13 @@ use serde::Deserialize;
 use std::{collections::HashMap, fmt};
 
 /// An HTTP error response.
-#[derive(Debug)]
 pub struct HttpError {
     status: StatusCode,
     details: ErrorDetails,
     headers: HashMap<String, String>,
+
+    // Attempt to read body for debugging purposes only.
+    #[allow(dead_code)]
     body: Bytes,
 }
 
@@ -36,7 +38,7 @@ impl HttpError {
             .into_body()
             .collect()
             .await
-            .unwrap_or_else(|_| Bytes::from_static(b"<ERROR COLLECTING BODY>"));
+            .unwrap_or_else(|_| Bytes::from_static(b"(error reading body)"));
         let details = ErrorDetails::new(&headers, &body);
         HttpError {
             status,
@@ -76,10 +78,21 @@ impl HttpError {
     }
 }
 
+impl fmt::Debug for HttpError {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        // Elide potential PII since it's too easily to accidentally leak through Debug or Display.
+        f.debug_struct("HttpError")
+            .field("status", &self.status)
+            .field("details", &self.details)
+            .finish_non_exhaustive()
+    }
+}
+
 impl fmt::Display for HttpError {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        // Elide potential PII since it's too easily to accidentally leak through Debug or Display.
         let newline = if f.alternate() { "\n" } else { " " };
-        let tab = if f.alternate() { "\t" } else { " " };
+        let tab = if f.alternate() { "\t" } else { "" };
         write!(f, "HttpError {{{newline}")?;
         write!(f, "{tab}Status: {},{newline}", self.status)?;
         write!(
@@ -88,16 +101,21 @@ impl fmt::Display for HttpError {
             self.details
                 .code
                 .as_deref()
-                .unwrap_or("<unknown error code>")
+                .unwrap_or("(unknown error code)")
         )?;
-        // TODO: sanitize body
-        write!(f, "{tab}Body: \"{:?}\",{newline}", self.body)?;
+        write!(f, "{tab}Body: \"(sanitized)\",{newline}")?;
+
+        let mut keys: Vec<_> = self.headers.keys().collect();
+        keys.sort();
+
         write!(f, "{tab}Headers: [{newline}")?;
-        // TODO: sanitize headers
-        for (k, v) in &self.headers {
-            write!(f, "{tab}{tab}{k}:{v}{newline}")?;
+        for key in keys {
+            write!(f, "{tab}{tab}{key}: (sanitized),{newline}")?;
+        }
+        write!(f, "{tab}],{newline}}}")?;
+        if f.alternate() {
+            write!(f, "{newline}")?;
         }
-        write!(f, "{tab}],{newline}}}{newline}")?;
 
         Ok(())
     }
@@ -190,4 +208,46 @@ mod tests {
             if error_code.as_deref() == Some("teapot")
         ));
     }
+
+    #[test]
+    fn debug_is_sanitized() {
+        let err = HttpError {
+            status: StatusCode::NotFound,
+            details: ErrorDetails {
+                code: Some("Not Found".to_string()),
+                message: Some("Resource not found".to_string()),
+            },
+            body: Bytes::from_static(b"resource not found"),
+            headers: HashMap::from([
+                ("authorization".to_string(), "bearer *****".to_string()),
+                ("x-ms-request-id".to_string(), "abcd1234".to_string()),
+            ]),
+        };
+        let actual = format!("{err:?}");
+        assert_eq!(
+            actual,
+            r#"HttpError { status: NotFound, details: ErrorDetails { code: Some("Not Found"), message: Some("Resource not found") }, .. }"#
+        );
+    }
+
+    #[test]
+    fn display_is_sanitized() {
+        let err = HttpError {
+            status: StatusCode::NotFound,
+            details: ErrorDetails {
+                code: None,
+                message: None,
+            },
+            body: Bytes::from_static(b"resource not found"),
+            headers: HashMap::from([
+                ("authorization".to_string(), "bearer *****".to_string()),
+                ("x-ms-request-id".to_string(), "abcd1234".to_string()),
+            ]),
+        };
+        let actual = format!("{err}");
+        assert_eq!(
+            actual,
+            r#"HttpError { Status: 404, Error Code: (unknown error code), Body: "(sanitized)", Headers: [ authorization: (sanitized), x-ms-request-id: (sanitized), ], }"#
+        );
+    }
 }

sdk/typespec/typespec_client_core/src/fmt.rs

@@ -0,0 +1,55 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License.
+
+//! Formatting helpers.
+
+use std::borrow::Cow;
+
+/// Converts ASCII characters in `value` to lowercase if required; otherwise, returns the original slice.
+///
+/// # Examples
+///
+/// Returns the original slice:
+///
+/// ```
+/// # use std::borrow::Cow;
+/// # use typespec_client_core::fmt::to_ascii_lowercase;
+/// let actual = to_ascii_lowercase("hello, world!");
+/// assert!(matches!(actual, Cow::Borrowed("hello, world!")));
+/// ```
+///
+/// Returns a clone converted to lowercase ASCII character.
+///
+/// ```
+/// # use std::borrow::Cow;
+/// # use typespec_client_core::fmt::to_ascii_lowercase;
+/// let actual = to_ascii_lowercase("hello, World!");
+/// assert!(matches!(
+///     actual,
+///     Cow::Owned(expected) if expected == "hello, world!"
+/// ));
+/// ```
+pub fn to_ascii_lowercase<'a>(value: &'a str) -> Cow<'a, str> {
+    for (i, c) in value.chars().enumerate() {
+        if c.is_ascii_uppercase() {
+            let mut s = value.to_owned();
+            s[i..].make_ascii_lowercase();
+
+            return Cow::Owned(s);
+        }
+    }
+
+    Cow::Borrowed(value)
+}
+
+#[test]
+fn test_to_ascii_lowercase() {
+    let actual = to_ascii_lowercase("hello, 🌎!");
+    assert!(matches!(actual, Cow::Borrowed("hello, 🌎!")));
+
+    let actual = to_ascii_lowercase("Hello, 🌎!");
+    assert!(matches!(
+        actual,
+        Cow::Owned(expected) if expected == "hello, 🌎!"
+    ));
+}

sdk/typespec/typespec_client_core/src/http/context.rs

@@ -7,6 +7,9 @@ use std::collections::HashMap;
 use std::sync::Arc;
 
 /// Pipeline execution context.
+///
+/// Do not store Personally-Identifiable Information (PII) in a `Context`.
+/// It could easily leak in logs or traces.
 #[derive(Clone, Debug)]
 pub struct Context<'a> {
     type_map: Cow<'a, HashMap<TypeId, Arc<dyn Any + Send + Sync>>>,

sdk/typespec/typespec_client_core/src/http/headers/mod.rs

@@ -9,7 +9,7 @@ mod microsoft;
 pub use common::*;
 pub use microsoft::*;
 
-use std::{borrow::Cow, convert::Infallible, fmt::Debug, str::FromStr};
+use std::{borrow::Cow, convert::Infallible, fmt, str::FromStr};
 use typespec::error::{Error, ErrorKind, ResultExt};
 
 /// A trait for converting a type into request headers.
@@ -84,7 +84,7 @@ pub trait Header {
 }
 
 /// A collection of headers.
-#[derive(Clone, Debug, PartialEq, Eq, Default)]
+#[derive(Clone, PartialEq, Eq, Default)]
 pub struct Headers(std::collections::HashMap<HeaderName, HeaderValue>);
 
 impl Headers {
@@ -220,6 +220,13 @@ impl Headers {
     }
 }
 
+impl fmt::Debug for Headers {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        // TODO: Sanitize headers.
+        write!(f, "Headers(len: {})", self.0.len())
+    }
+}
+
 impl IntoIterator for Headers {
     type Item = (HeaderName, HeaderValue);
 
@@ -292,7 +299,7 @@ impl From<String> for HeaderName {
 }
 
 /// A header value.
-#[derive(Clone, Debug, PartialEq, Eq)]
+#[derive(Clone, PartialEq, Eq)]
 pub struct HeaderValue(Cow<'static, str>);
 
 impl HeaderValue {
@@ -315,6 +322,12 @@ impl HeaderValue {
     }
 }
 
+impl fmt::Debug for HeaderValue {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        f.write_str("HeaderValue")
+    }
+}
+
 impl From<&'static str> for HeaderValue {
     fn from(s: &'static str) -> Self {
         Self::from_cow(s)

sdk/typespec/typespec_client_core/src/http/pager.rs

@@ -8,14 +8,15 @@ use typespec::Error;
 
 use crate::http::{headers::HeaderName, Response};
 
+/// The result of fetching a single page from a [`Pager`], whether the `Pager` should continue or is complete.
 pub enum PagerResult<T, C> {
+    /// The [`Pager`] may fetch additional pages with the included `continuation` token.
     Continue {
         response: Response<T>,
         continuation: C,
     },
-    Complete {
-        response: Response<T>,
-    },
+    /// The [`Pager`] is complete and there are no additional pages to fetch.
+    Complete { response: Response<T> },
 }
 
 impl<T> PagerResult<T, String> {

sdk/typespec/typespec_client_core/src/http/request/mod.rs

@@ -17,7 +17,7 @@ use serde::Serialize;
 use std::{fmt::Debug, marker::PhantomData, str::FromStr};
 
 /// An HTTP Body.
-#[derive(Debug, Clone)]
+#[derive(Clone)]
 pub enum Body {
     /// A body of a known size.
     Bytes(bytes::Bytes),
@@ -54,6 +54,15 @@ impl Body {
     }
 }
 
+impl Debug for Body {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        match self {
+            Self::Bytes(v) => write!(f, "Bytes(len: {})", v.len()),
+            Self::SeekableStream(v) => write!(f, "SeekableStream(len: {})", v.len()),
+        }
+    }
+}
+
 impl<B> From<B> for Body
 where
     B: Into<Bytes>,

sdk/typespec/typespec_client_core/src/http/response.rs

@@ -214,9 +214,8 @@ impl<T> fmt::Debug for Response<T> {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         f.debug_struct("Response")
             .field("status", &self.status)
-            .field("headers", &self.headers)
-            .field("body", &"(body)")
-            .finish()
+            // TODO: Sanitize headers and emit body as "(body)".
+            .finish_non_exhaustive()
     }
 }
 

sdk/typespec/typespec_client_core/src/lib.rs

@@ -8,6 +8,7 @@ mod macros;
 pub mod base64;
 pub mod date;
 pub mod error;
+pub mod fmt;
 #[cfg(feature = "http")]
 pub mod http;
 #[cfg(feature = "json")]

sdk/typespec/typespec_client_core/src/stream/bytes_stream.rs

@@ -4,6 +4,7 @@
 use super::{Bytes, SeekableStream};
 use futures::io::AsyncRead;
 use futures::stream::Stream;
+use std::fmt;
 use std::pin::Pin;
 use std::task::Poll;
 
@@ -12,7 +13,7 @@ use std::task::Poll;
 /// This struct implements both `Stream` and `SeekableStream` for an
 /// immutable bytes buffer. It's cheap to clone but remember to `reset`
 /// the stream position if you clone it.
-#[derive(Debug, Clone)]
+#[derive(Clone)]
 pub struct BytesStream {
     bytes: Bytes,
     bytes_read: usize,
@@ -32,6 +33,15 @@ impl BytesStream {
     }
 }
 
+impl fmt::Debug for BytesStream {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        f.debug_struct("BytesStream")
+            .field("bytes", &format!("(len: {})", self.bytes.len()))
+            .field("bytes_read", &self.bytes_read)
+            .finish()
+    }
+}
+
 impl From<Bytes> for BytesStream {
     fn from(bytes: Bytes) -> Self {
         Self::new(bytes)

sdk/typespec/typespec_client_core/src/xml.rs

@@ -32,7 +32,7 @@ where
 {
     from_reader(slice_bom(body)).with_context(ErrorKind::DataConversion, || {
         let t = core::any::type_name::<T>();
-        let xml = std::str::from_utf8(body).unwrap_or("<XML IS NOT UTF-8>");
+        let xml = std::str::from_utf8(body).unwrap_or("(XML is not UTF8-encoded)");
         format!("failed to deserialize the following xml into a {t}\n{xml}")
     })
 }