diff --git a/CHANGELOG.md b/CHANGELOG.md index c39bf5d..70b7a85 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -86,6 +86,23 @@ order releases happen, newest first. ### Fixed +- **`NextAction` and `Delivery` no longer print what they hold.** Both derived + `Debug`, so one `tracing::debug!("{charge:?}")` put a live Stripe + `client_secret` into a log file, and one `tracing::debug!("{delivery:?}")` + put a provider's signature and the whole body it signed into the same place. + Stripe's own documentation says a client secret "should not be stored, + logged, or exposed to anyone other than the customer" — anybody holding one + can confirm or cancel that payment from a browser. + + Both are written rather than derived now, the way `Raw` already was: the two + handles show as a length, a delivery names which headers arrived and counts + its body, and the redirect address is still printed whole because a caller + who cannot log it cannot log the one thing that variant is for. + + **`Debug` output changed**, so a test asserting on the old text will fail. + Nothing else about either type moved. `Charge` derives `Debug` and holds both + a `NextAction` and a `Raw`, so `{charge:?}` is now safe throughout. + - **A stored-card charge at Stripe carries the caller's `return_url`.** `Stripe::charge_saved_card` sends `confirm: true`, which is the one create Stripe documents the field as usable on — and `saved::Payment` had no such diff --git a/crates/kasapay-core/src/charge.rs b/crates/kasapay-core/src/charge.rs index 682d6c4..7d74c42 100644 --- a/crates/kasapay-core/src/charge.rs +++ b/crates/kasapay-core/src/charge.rs @@ -196,7 +196,21 @@ impl Status { } /// What the payer has to do before the payment can go on. -#[derive(Debug, Clone, PartialEq, Eq)] +/// +/// # It does not print its handles +/// +/// [`Debug`] is written rather than derived, for the reason [`Raw`]'s is: one +/// `tracing::debug!("{charge:?}")` puts whatever it prints into a file that +/// outlives the request. Both handles here are values a provider says to keep +/// to itself — Stripe's `client_secret` confirms or cancels the payment from a +/// browser, and iyzico's In-Store `paymentSessionToken` decrypts the callback +/// — so both are shown as a length. +/// +/// The address is printed whole. It is where the payer is sent, so a caller +/// who cannot log it cannot log the one thing this variant is for. A provider +/// that puts a token *inside* that address — iyzico's hosted form does — has +/// made that a decision for the caller rather than for this type. +#[derive(Clone, PartialEq, Eq)] #[non_exhaustive] pub enum NextAction { /// Send the payer to this address — a hosted page, or an app deep link. @@ -227,6 +241,36 @@ pub enum NextAction { }, } +impl fmt::Debug for NextAction { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::Redirect { url, continuation } => f + .debug_struct("Redirect") + // Display, not Debug: `Url`'s own Debug takes the address apart + // into scheme, host and path, and the address whole is the point. + .field("url", &format_args!("{url}")) + .field("continuation", &Held(continuation.as_deref())) + .finish(), + Self::ConfirmOnClient { client_secret } => f + .debug_struct("ConfirmOnClient") + .field("client_secret", &Held(Some(client_secret))) + .finish(), + } + } +} + +/// A value whose length is worth seeing and whose contents are not. +struct Held<'a>(Option<&'a str>); + +impl fmt::Debug for Held<'_> { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self.0 { + Some(held) => write!(f, "<{} chars>", held.chars().count()), + None => f.write_str("None"), + } + } +} + /// A charge, as the provider currently sees it. /// /// Every field is public and the struct is open: a provider adapter living diff --git a/crates/kasapay-core/src/webhook.rs b/crates/kasapay-core/src/webhook.rs index b4a1990..700bb07 100644 --- a/crates/kasapay-core/src/webhook.rs +++ b/crates/kasapay-core/src/webhook.rs @@ -30,12 +30,34 @@ pub struct RepeatedHeader { /// /// Header names are matched without regard to case, because HTTP/2 lowercases /// them and HTTP/1.1 does not. -#[derive(Debug, Clone, Copy)] +#[derive(Clone, Copy)] pub struct Delivery<'a> { headers: &'a [(&'a str, &'a str)], body: &'a [u8], } +/// Names the headers and counts the body, and prints neither. +/// +/// A delivery holds the provider's signature and the whole payload. Deriving +/// this put both wherever a handler logged the delivery it was handed — which +/// is the first thing anybody does while a webhook is not working. Which +/// headers arrived is the useful half and is safe; what they say is not. +impl fmt::Debug for Delivery<'_> { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("Delivery") + .field( + "headers", + &self + .headers + .iter() + .map(|(name, _)| *name) + .collect::>(), + ) + .field("body", &format_args!("{} bytes", self.body.len())) + .finish() + } +} + impl<'a> Delivery<'a> { /// Holds a delivery's headers and its body. #[must_use] diff --git a/crates/kasapay-core/tests/secrets.rs b/crates/kasapay-core/tests/secrets.rs new file mode 100644 index 0000000..4211d18 --- /dev/null +++ b/crates/kasapay-core/tests/secrets.rs @@ -0,0 +1,86 @@ +//! Nothing that holds a handle may print one. +//! +//! Each adapter has a file with this name, because a secret in a log line is an +//! incident and the usual way one gets there is a `{:?}` on a type somebody +//! added a field to. `kasapay-core` had no such file, and it is where the two +//! worst offenders lived: `NextAction`, which carries Stripe's `client_secret` +//! and iyzico's `paymentSessionToken`, and `Delivery`, which carries a +//! provider's signature and the whole body it signed. +//! +//! Both are reached by `tracing::debug!("{charge:?}")` and +//! `tracing::debug!("{delivery:?}")` — the first thing anybody writes while a +//! payment or a webhook is not working. + +use kasapay_core::{Delivery, NextAction, Raw, Secret}; + +/// Distinctive enough that a substring search cannot miss it. +const HELD: &str = "kasapayMUSTNOTAPPEARinlogs"; + +#[test] +fn a_client_secret_is_not_printed() { + let action = NextAction::ConfirmOnClient { + client_secret: format!("pi_1_secret_{HELD}").into(), + }; + let shown = format!("{action:?}"); + + assert!( + !shown.contains(HELD), + "the client secret reached a Debug: {shown}" + ); + // Stripe's own documentation: it "should not be stored, logged, or exposed + // to anyone other than the customer" — anybody holding one can confirm or + // cancel that payment from a browser. + assert!( + shown.contains("chars"), + "and the length is still useful: {shown}" + ); +} + +#[test] +fn a_continuation_token_is_not_printed_and_the_address_is() { + let action = NextAction::Redirect { + url: "https://provider.test/form/abc".parse().expect("valid url"), + continuation: Some(HELD.into()), + }; + let shown = format!("{action:?}"); + + assert!( + !shown.contains(HELD), + "the continuation token reached a Debug: {shown}" + ); + // The address is where the payer is sent. A caller who cannot log it + // cannot log the one thing this variant is for. + assert!( + shown.contains("https://provider.test/form/abc"), + "the address is not printed, which makes this unloggable: {shown}" + ); +} + +#[test] +fn a_delivery_prints_which_headers_arrived_and_not_what_they_say() { + let headers = [ + ("Stripe-Signature", HELD), + ("Content-Type", "application/json"), + ]; + let body = format!(r#"{{"secret":"{HELD}"}}"#); + let delivery = Delivery::new(&headers, body.as_bytes()); + let shown = format!("{delivery:?}"); + + assert!( + !shown.contains(HELD), + "the signature or the body reached a Debug: {shown}" + ); + // Which headers arrived is the useful half, and it is safe. + assert!( + shown.contains("Stripe-Signature"), + "the header names are worth keeping: {shown}" + ); + assert!(shown.contains("bytes"), "and the body's size: {shown}"); +} + +/// The two that were already right, kept here so the pair reads as a rule. +#[test] +fn a_secret_and_a_raw_body_were_already_silent() { + assert!(!format!("{:?}", Secret::new(HELD)).contains(HELD)); + assert!(!format!("{:?}", Raw::from_text(HELD)).contains(HELD)); +}