From 2e1443578ad68596b05c4eb4c964e78543588e0f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?David=20B=C3=BCrgin?= <dbuergin@gluet.ch>
Date: Mon, 29 Jun 2020 09:37:25 +0200
Subject: [PATCH] Warn when max message size misconfigured

---
 CHANGELOG.md          |  3 +++
 spamassassin-milter.8 |  4 ++--
 src/callbacks.rs      |  3 ++-
 src/client.rs         | 51 ++++++++++++++++++++++++++++++++++++++++++-
 4 files changed, 57 insertions(+), 4 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 0da61f1..3379de8 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -4,6 +4,9 @@
 
 *   Add `--reply-code`, `--reply-status-code`, and `--reply-text` options to
     allow customising the SMTP reply when rejecting spam.
+*   Log a warning and do not truncate message body when `--max-message-size` is
+    misconfigured (must be ≥ `spamc` max size as documented).
+*   Update dependencies in `Cargo.lock`.
 
 ## 0.1.2 (2020-06-07)
 
diff --git a/spamassassin-milter.8 b/spamassassin-milter.8
index d0451d1..950c1e6 100644
--- a/spamassassin-milter.8
+++ b/spamassassin-milter.8
@@ -85,8 +85,8 @@ Maximum message size in bytes to pass to
 .I BYTES
 must be equal to or greater than the max size configured for
 .BR spamc ,
-else it is possible that the body of spam messages is truncated to the size
-configured for
+in order to ensure that SpamAssassin does not process messages truncated to the
+size configured for
 .BR spamc .
 Defaults to the
 .B spamc
diff --git a/src/callbacks.rs b/src/callbacks.rs
index 77c1aef..bfe04fe 100644
--- a/src/callbacks.rs
+++ b/src/callbacks.rs
@@ -185,7 +185,8 @@ fn handle_body(mut ctx: Context, bytes: &[u8]) -> milter::Result<Status> {
     client.send_body_chunk(bytes)?;
 
     let max = config::get().max_message_size();
-    Ok(if client.bytes_written() >= max {
+    Ok(if client.bytes_written() > max {
+        client.skip_body();
         let id = queue_id(&ctx.api)?;
         verbose!("{}: skipping rest of message larger than {} bytes", id, max);
         Status::Skip
diff --git a/src/client.rs b/src/client.rs
index 6a982f4..04725f9 100644
--- a/src/client.rs
+++ b/src/client.rs
@@ -96,6 +96,7 @@ pub struct Client {
     recipients: Vec<String>,
     headers: HeaderMap,
     bytes: usize,
+    skipped: bool,
 }
 
 impl Client {
@@ -106,6 +107,7 @@ impl Client {
             recipients: vec![],
             headers: HeaderMap::new(),
             bytes: 0,
+            skipped: false,
         }
     }
 
@@ -117,6 +119,10 @@ impl Client {
         self.bytes
     }
 
+    pub fn skip_body(&mut self) {
+        self.skipped = true;
+    }
+
     pub fn connect(&mut self) -> Result<()> {
         self.process.connect()
     }
@@ -252,7 +258,7 @@ impl Client {
             }
             if !config.preserve_body() {
                 rewriter.rewrite_report_headers(id, actions)?;
-                email::replace_body(id, email.body, actions, config)?;
+                replace_body(id, email.body, self.skipped, actions, config)?;
             }
         }
 
@@ -280,6 +286,29 @@ fn reject_spam(id: &str, actions: &impl SetErrorReply, config: &Config) -> milte
     }
 }
 
+fn replace_body(
+    id: &str,
+    body: &[u8],
+    skipped: bool,
+    actions: &impl ActionContext,
+    config: &Config,
+) -> milter::Result<()> {
+    // Do not replace the message body when part of it was skipped. This only
+    // occurs when the milter’s max message size is less than that of `spamc`.
+    // This condition ensures message integrity in such a misconfigured setup.
+    if skipped {
+        eprintln!(
+            "{}: not replacing possibly truncated message body; \
+            please review max message size setting ({})",
+            id,
+            config.max_message_size()
+        );
+        Ok(())
+    } else {
+        email::replace_body(id, body, actions, config)
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -514,4 +543,24 @@ mod tests {
             assert!(expected.contains(&a));
         }
     }
+
+    #[test]
+    fn client_process_skipped_body_not_replaced() {
+        let spamc = MockSpamc::with_output(
+            b"X-Spam-Flag: YES\r\nX-Spam-Level: *****\r\n\r\nReport".to_vec(),
+        );
+        let actions = MockActionContext::new();
+        let config = Default::default();
+
+        let mut client = Client::new(spamc, String::from("sender"));
+        client.skip_body();
+
+        let status = client.process("id", &actions, &config).unwrap();
+
+        assert_eq!(status, Status::Accept);
+
+        let called = actions.called.borrow();
+        assert!(called.contains(&Action::AddHeader("X-Spam-Level".into(), " *****".into())));
+        assert!(!called.contains(&Action::AppendBodyChunk(b"Report".to_vec())));
+    }
 }