fix(core): NextAction and Delivery do not print what they hold

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

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,34 @@ 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")
+                .field("url", 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

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::<Vec<_>>(),
+            )
+            .field("body", &format_args!("{} bytes", self.body.len()))
+            .finish()
+    }
+}
+
 impl<'a> Delivery<'a> {
     /// Holds a delivery's headers and its body.
     #[must_use]

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));
+}