Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't implement Display for @sensitive string shapes and don't #[derive(Debug)] #3604

Open
2 tasks
david-perez opened this issue Apr 26, 2024 · 1 comment
Open
2 tasks
Labels
breaking-change This will require a breaking change server Rust server SDK

Comments

@david-perez
Copy link
Contributor

david-perez commented Apr 26, 2024

We currently newtype constrained string shapes. When they are marked as @sensitive:

@sensitive
@length(min: 1, max: 69)
string Password

We also implement Display:

impl ::std::fmt::Display for SchemaJson {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        "*** Sensitive Data Redacted ***".fmt(f)
    }
}

and derive Debug:

#[derive(
    ::std::clone::Clone, ::std::cmp::Eq, ::std::cmp::PartialEq, ::std::fmt::Debug, ::std::hash::Hash,
)]
pub struct Password(pub(crate) ::std::string::String);
  • Don't implement Display. Implementing Display is footgunny. If you have a fn save_password_to_database(password: String), calling save_password_to_database(password.to_string()) seems like would make sense, but would cause permanent data loss. Users should instead use the generated password.into_inner(), so it's better to point them in that direction by avoiding implementing Display.

  • Don't #[derive(Debug)]. While Debug should format in a programmer-facing debugging context, debugging with the redaction should suffice.

    • Note that this is consistent with how Debug is implemented throughout smithy-rs e.g. the Debug implementation for a structure shape that has member shapes that target @sensitive string shapes will print the struct with redactions for the @sensitive strings.
    • We can feature-gate a "truly unredacted" debugging experience, like we already do with sensitive.rs; see RFC: Logging in the Presence of Sensitive Data.
@david-perez david-perez added the server Rust server SDK label Apr 26, 2024
@david-perez
Copy link
Contributor Author

This issue is related (but separate) to #1883, which is concerned about whether a @sensitive un constrained string shape should be newtyped or not.

@david-perez david-perez added the breaking-change This will require a breaking change label Apr 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change This will require a breaking change server Rust server SDK
Projects
None yet
Development

No branches or pull requests

1 participant