std::fmt::Display implementation on error shapes does not honor the sensitive trait

[david-perez]

This model:

@error("client")
structure ErrorWithMessage {
    message: SensitiveString
}

@sensitive
string SensitiveString

generates:

#[allow(missing_docs)] // documentation missing in model
#[derive(std::clone::Clone, std::cmp::PartialEq)]
pub struct ErrorWithMessage {
    #[allow(missing_docs)] // documentation missing in model
    #[doc(hidden)]
    pub message: std::option::Option<std::string::String>,
}
impl std::fmt::Debug for ErrorWithMessage {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        let mut formatter = f.debug_struct("ErrorWithMessage");
        formatter.field("message", &"*** Sensitive Data Redacted ***");
        formatter.finish()
    }
}
impl ErrorWithMessage {
    /// Returns the error message.
    pub fn message(&self) -> std::option::Option<&str> {
        self.message.as_deref()
    }
    #[doc(hidden)]
    /// Returns the error name.
    pub fn name(&self) -> &'static str {
        "ErrorWithMessage"
    }
}
impl std::fmt::Display for ErrorWithMessage {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        write!(f, "ErrorWithMessage")?;
        if let Some(inner_1) = &self.message {
            write!(f, ": {}", inner_1)?;
        }
        Ok(())
    }
}
impl std::error::Error for ErrorWithMessage {}
...

Note how the Debug implementation honors the sensitive trait, but the Display implementation does not.

The code that generates the Debug implementation is:

https://github.com/awslabs/smithy-rs/blob/ffafbec885c1e83a460fed38fc98119bb63d0439/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/generators/StructureGenerator.kt#L108-L127

The code that generates the Display implementation is:

https://github.com/awslabs/smithy-rs/blob/ffafbec885c1e83a460fed38fc98119bb63d0439/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/generators/error/TopLevelErrorGenerator.kt#L82-L93

[hlbarber]

During implementation of Logging in the Presence of Sensitive Data, I added https://github.com/awslabs/smithy-rs/blob/dcfb85578ef10e8d4fcb037c2fcf7d2a5e879f70/rust-runtime/aws-smithy-http-server/src/instrumentation/sensitivity/sensitive.rs.

Reusing this machinery might be a good idea because:

• Anything using Sensitive would be enjoy toggling via the feature flag unredacted-logging.
• The redaction word is no longer a "magic constant" by virtue of https://github.com/awslabs/smithy-rs/blob/dcfb85578ef10e8d4fcb037c2fcf7d2a5e879f70/rust-runtime/aws-smithy-http-server/src/instrumentation/sensitivity/mod.rs#L26.

This changes the proposed implementation very slightly:

impl std::fmt::Debug for ErrorWithMessage {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        let mut formatter = f.debug_struct("ErrorWithMessage");
        formatter.field("message", Sensitive(self.message));
        formatter.finish()
    }
}

A similar argument can be made for going back and editing this implementation too #229.

[jjant]

I think the current implementation would only make sense if the @sensitive trait could only be used in member shapes:

structure ErrorWithMessage {
    @sensitive
    message: String
}

That way you could only worry about the field being sensitive in the context of the encompassing ErrorWithMessage struct (like we do in the Debug/Display impls for it).

But if we have a plain type like:

@sensitive
string SensitiveString

That should be sensitive regardless of where it's used, but currently you could do:

println!("{}", error_with_message.message/* or .unwrap() */)

And have it logged.

---

From what I can see, the only way to fix it would be to generate a Sensitive<String> everywhere SensitiveString is used in the Smithy model.

[jjant]

Done in #1802.

[david-perez]

@jjant We should create a new issue with your findings in #1743 (comment) about whether we should wrap Strings to make their Display implementation honor sensitive; we still need to (eventually) make a decision on that.