Repository: lettre/lettre (language: rust)

Issue:
Attachment filename with utf-8 chars is broken
**Describe the bug**
When updating from lettre 0.10.0-alpha.4 to 0.10.0-rc.2, the Content-Disposition header handling was replaced with an own implementation in #601.

When setting an attachment filename with the new attachment API containing an umlaut like "Töööst.pdf", it will also encode the ``filename=`` part of the header. Example output:
```
Content-Disposition: attachment; =?utf-8?b?ZmlsZW5hbWU9IlTDtnN0?=.pdf
```
My email client no longer shows any filename for the attachment :)

This is how I handled an utf-8 based filename for attachments in lettre 0.10.0-alpha.4:
```
parts = parts.singlepart(
    SinglePart::base64()
        .header(header::ContentType(
            "application/pdf".parse().unwrap(),
        ))
        .header(lettre::message::header::ContentDisposition {
            disposition: DispositionType::Attachment,
            parameters: vec![DispositionParam::Filename(
                Charset::Ext("utf-8".into()),
                None,
                pdf.filename.as_bytes().to_vec(),
            )],
        })
        .body(raw_pdf),
);
```
This resulted in this header:
```
Content-Disposition: attachment; filename="Töst.pdf"
```
Also not ideal since it does not specify the character encoding, the mail client assumed utf-8 though.

I'm unsure how the attachment API for non utf-8 filenames is best exposed in lettre.

Related read: https://blog.nodemailer.com/2017/01/27/the-mess-that-is-attachment-filenames/


Discussion / hints:
Thanks for the report and for sharing that post. I was able to reproduce the issue with all of my clients. I'll have to look more into this and into the RFC. As a quick test I tried changing how we do the utf-8 encoding (I added ` || c == '='` [here](https://github.com/lettre/lettre/blob/1d8249165cdda87611ea243023f7f4eb405e4112/src/message/header/mod.rs#L532)) and I produced the following email. Can you confirm if this fixes it for your client too? You should see two files attached.


[test.eml.zip](https://github.com/lettre/lettre/files/6517192/test.eml.zip)

> Can you confirm if this fixes it for your client too? You should see two files attached.
> 
> [test.eml.zip](https://github.com/lettre/lettre/files/6517192/test.eml.zip)

I've tried with KDE's kmail 5.15.3 (Fedora 33) and it didn't show any attachments.
After replacing the filename with ``filename="test.txt"``, the attachments showed up of course.

Here's how kmail generated the header for a filename ``töst.txt``:
```
Content-Disposition: attachment; filename*=ISO-8859-1''t%F6st%2Etxt
```

This is what Thunderbird generated for the same file:
```
Content-Disposition: attachment;
 filename*=UTF-8''%74%C3%B6%73%74%2E%74%78%74
```

I think what is needed is "encoded words" parameter handling as defined in RFC 2231:
https://datatracker.ietf.org/doc/html/rfc2231

This encoding can be quite "fun" to parse, here's a more advanced example from a C++ unit test I wrote ages ago:
```
    String text = "attachment; ";
    text += "filename*2=\"andard.We_are_going_to_find_this_out.txt\"; ";
    text += "filename*0=\"this_is_a_very_long_filename_which_will_be_splitted_accordin\"; ";
    text += "filename*1=\"g_to_rfc_2231_if_the_mail_client_is_compliant_to_the_MIME_st\"";
```
Interesting side note: Microsoft Outlook 2019 doesn't seem to support "RFC2231: 3. Parameter Value Continuations":
https://techcommunity.microsoft.com/t5/office-insider/the-file-name-of-the-attachment-received-in-outlook-2019-is/m-p/1762750

The post is from October 2020, so it's probably still valid. Writing it down here in case someone reports this in the future as "lettre bug".
I may have found a solution which would only require a slight modification to our [RFC 1522 encoder](https://github.com/lettre/lettre/blob/1d8249165cdda87611ea243023f7f4eb405e4112/src/message/header/mod.rs#L297-L544).

You should see 1 attachment: [test2.eml.zip](https://github.com/lettre/lettre/files/6517539/test2.eml.zip)
> You should see 1 attachment: test2.eml.zip

it worked in kmail, but both Thunderbird and neomutt showed something like this:
``Attachment #2: =?utf-8?b?VMO2w7bDts O2w7bDtsO2w7ZzdC50eHQ=?=``


Ten years ago I wrote a full RFC2231 encoder/decoder for a closed source C++ library at work,
I could look into porting this to Rust -> lettre. Code relicensing is not an issue, that's something under my control there ;) The code has quite a few exhaustive unit tests for nasty corner cases, it was tested against about a million of messages and also spam emails.

Since lettre does just email construction and not parsing, it shouldn't bee too much of an undertaking.
Ideally I would have enough time to include this in 0.10.0...

Does lettre use any crate for character encoding conversion?
Or would it be ok to write out everything as utf-8 only?
> it worked in kmail, but both Thunderbird and neomutt showed something like this:
> `Attachment #2: =?utf-8?b?VMO2w7bDts O2w7bDtsO2w7ZzdC50eHQ=?=`

Ops. [This one should work better](https://github.com/lettre/lettre/files/6523334/test4.eml.zip). I handcrafted the last one, and didn't take into account that you have to close the encoding before going to a new line.

> Does lettre use any crate for character encoding conversion?

No

> Ten years ago I wrote a full RFC2231 encoder/decoder for a closed source C++ library at work,
> I could look into porting this to Rust -> lettre. Code relicensing is not an issue, that's something under my control there ;) The code has quite a few exhaustive unit tests for nasty corner cases, it was tested against about a million of messages and also spam emails.

> Or would it be ok to write out everything as utf-8 only?

#607 made it so that [RFC 1522 header value encoding](https://github.com/lettre/lettre/blob/1d8249165cdda87611ea243023f7f4eb405e4112/src/message/header/mod.rs#L297-L544) is handled globally. The fix here will be to have to have only the part inside double quotes get encoded.

Non utf-8 filenames aren't currently supported, but it would be a nice to have if people are interested in it. I'm not even sure how good support for it was in alpha 4, since the library we were using for headers was really meant to be used for for HTTP. If we were to support RFC 2231 encoding we would also need a way of bypassing the RFC 1522 encoder for that header, since certain characters needed for RFC 2231 currently get escaped in order to prevent certain security issues.

> > it worked in kmail, but both Thunderbird and neomutt showed something like this:
> > `Attachment #2: =?utf-8?b?VMO2w7bDts O2w7bDtsO2w7ZzdC50eHQ=?=`
> 
> Ops. [This one should work better](https://github.com/lettre/lettre/files/6523334/test4.eml.zip). I handcrafted the last one, and didn't take into account that you have to close the encoding before going to a new line.

that one worked in Thunderbird and kmail. Neomutt showed
``Attachment #2:  =?utf-8?b?VMO2w7bDtnN0?=.txt``

> > Ten years ago I wrote a full RFC2231 encoder/decoder for a closed source C++ library at work,
> > I could look into porting this to Rust -> lettre. Code relicensing is not an issue, that's something under my control there ;) The code has quite a few exhaustive unit tests for nasty corner cases, it was tested against about a million of messages and also spam emails.
> 
> > Or would it be ok to write out everything as utf-8 only?
> 
> #607 made it so that [RFC 1522 header value encoding](https://github.com/lettre/lettre/blob/1d8249165cdda87611ea243023f7f4eb405e4112/src/message/header/mod.rs#L297-L544) is handled globally. The fix here will be to have to have only the part inside double quotes get encoded.
> 
> Non utf-8 filenames aren't currently supported, but it would be a nice to have if people are interested in it. I'm not even sure how good support for it was in alpha 4, since the library we were using for headers was really meant to be used for for HTTP. If we were to support RFC 2231 encoding we would also need a way of bypassing the RFC 1522 encoder for that header, since certain characters needed for RFC 2231 currently get escaped in order to prevent certain security issues.

yes, good point. It looks like RFC2231 is the only way forward with certain email clients. I had a quick look around crates.io and found this *decoder*: https://github.com/jothan/rustyknife/blob/master/src/rfc2231.rs
GPL v3 license is not compatible with lettre and being a decoder doesn't help either.

Let's go with RFC 2231 then. We'll just need the encoder
To avoid double work: Should I give it a go or do you want to look into it?

I could work on it soon and would first migrate my existing code to Rust and then integrate it into lettre once all unit tests are running.

Given that this encoder has the potential to need an API change to deal with the RFC 1522 header encoder, would you consider this issue for inclusion in 0.10.0 or is it too late?

If you'd like to take a stab at this I'd be more than happy to review your PR. I'll have less time to dedicate to lettre in the following weeks, so your help would definitely be appreciated and make it possible to have this in 0.10.0.

Regarding the API change we could probably go for a `HeaderValue` struct which can either contain a normal header value, which goes through the normal RFC 1522 encoder, or a raw header value that just gets printed as is.
> f you'd like to take a stab at this I'd be more than happy to review your PR

sounds like a good plan, I'll start working on it on the side. Thanks for your feedback.
Any news on this one?
I started looking into this and I'm working on a solution that would solve both this issue and #672 
I didn't start on the task as I'm currently on a reduced work schedule (accumulated way too much overtime). So I'm happy if someone else wants to look into it.
I already implemented RFC 2047 and RFC 2231 encoding. We decided to put the implementation in a separate repository, in order to avoid bloating the main lettre crate with too much stuff, which will be published as a separate crate that lettre will use internally.

The repository is currently private but we will publish it as soon as possible, the main remaining question is probably crate naming. Do we want to make a crate which only implements encoding? For example an [RFC 2047 decoder crate already exists](https://crates.io/crates/rfc2047-decoder).

Hey @paolobarbolini, first **thanks for the efforts in lettre :)**

Regarding the approach, if you plan to keep functionality required by lettre around but not in the main repo, maybe a lettre-utils package with all the external functionality required by the main repo could work and not have conflicts with other mail related packages. If not, a lettre-* repo per feature its also an option.


Best,
J


> Regarding the approach, if you plan to keep functionality required by lettre around but not in the main repo, maybe a lettre-utils package with all the external functionality required by the main repo could work and not have conflicts with other mail related packages. If not, a lettre-* repo per feature its also an option.

I've been thinking about this and what I don't think going the actix-web naming route (having many crates starting with `actix-` in their case) fits what we want to do here. For example Alexis already owns the `smtp` crate on crates.io, so for #543 we're going to reuse that crate for the implementation. For this I'd want to name it in a way that gives the idea that, although being a low level implementation, it could be discovered and also be useful to other projects (basically how [tokio-rs](https://github.com/tokio-rs) or [hyperium](https://github.com/hyperium) for example name their crates).


@paolobarbolini Super thanks for the work you are putting in!
I understand those things take time, and I'm not at all trying to beat the drum here, but if there is an existing solution to this issue, then maybe there is a small tiny chance that I could have a peek at some of the code? that way I could try some ugly duct tape hack to get it handled for my own little project. You know, at least maybe a temporary fix until it's done and proper. =) I'd be forever grateful.
> @paolobarbolini Super thanks for the work you are putting in! I understand those things take time, and I'm not at all trying to beat the drum here, but if there is an existing solution to this issue, then maybe there is a small tiny chance that I could have a peek at some of the code? that way I could try some ugly duct tape hack to get it handled for my own little project. You know, at least maybe a temporary fix until it's done and proper. =) I'd be forever grateful.

Actually there might be an hacky solution to this 😃. The correct `filename` encoder has already been implemented, what's slowing everything down are questions about crate naming (since it has been implemented in a separate unpublished crate) and changes to the header API. I may however have a temporary hacky way to implement it just for the `Content-Disposition` header. I'll try putting something up.
> You know, at least maybe a temporary fix until it's done and proper. =) I'd be forever grateful.

I worked around the issue in my code with a small function that replaces any non-whitelisted character with ``_``. Also fixes up some typical umlauts:

```
fn sanitize_filename_for_email(filename: &str) -> String {
    let mut sanitized = String::new();

    for c in filename.chars() {
        match c {
            'ä' => sanitized.push_str("ae"),
            'ö' => sanitized.push_str("oe"),
            'ü' => sanitized.push_str("ue"),
            'Ä' => sanitized.push_str("Ae"),
            'Ö' => sanitized.push_str("Oe"),
            'Ü' => sanitized.push_str("Ue"),
            'ß' => sanitized.push_str("ss"),
            'a'..='z' | 'A'..='Z' | '0'..='9' => sanitized.push(c),
            '&' | '.' | '_' | '-' | '+' | ' ' => sanitized.push(c),
            _ => sanitized.push('_'), // replace any other character
        }
    }

    sanitized
}
```

Hope that helps.
I've opened #685 with the fix to this issue

Fix patch:
diff --git a/Cargo.toml b/Cargo.toml
index b7e721b2f..5b7e95bc7 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -29,6 +29,7 @@ fastrand = { version = "1.4", optional = true }
 quoted_printable = { version = "0.4", optional = true }
 base64 = { version = "0.13", optional = true }
 regex = { version = "1", default-features = false, features = ["std", "unicode-case"] }
+email-encoding = { git = "https://github.com/lettre/email-encoding.git", rev = "9b55bae0ed02ca52f6fb86a82b4814baa712e45a", optional = true }
 
 # file transport
 uuid = { version = "0.8", features = ["v4"], optional = true }
@@ -81,7 +82,7 @@ name = "transport_smtp"
 
 [features]
 default = ["smtp-transport", "pool", "native-tls", "hostname", "builder"]
-builder = ["httpdate", "mime", "base64", "fastrand", "quoted_printable"]
+builder = ["httpdate", "mime", "base64", "fastrand", "quoted_printable", "email-encoding"]
 mime03 = ["mime"]
 
 # transports
diff --git a/src/message/header/content_disposition.rs b/src/message/header/content_disposition.rs
index f217bb6f7..d66fafd60 100644
--- a/src/message/header/content_disposition.rs
+++ b/src/message/header/content_disposition.rs
@@ -1,3 +1,7 @@
+use std::fmt::Write;
+
+use email_encoding::headers::EmailWriter;
+
 use super::{Header, HeaderName, HeaderValue};
 use crate::BoxError;
 
@@ -5,25 +9,47 @@ use crate::BoxError;
 ///
 /// Defined in [RFC2183](https://tools.ietf.org/html/rfc2183)
 #[derive(Debug, Clone, PartialEq)]
-pub struct ContentDisposition(String);
+pub struct ContentDisposition(HeaderValue);
 
 impl ContentDisposition {
     /// An attachment which should be displayed inline into the message
     pub fn inline() -> Self {
-        Self("inline".into())
+        Self(HeaderValue::dangerous_new_pre_encoded(
+            Self::name(),
+            "inline".to_string(),
+            "inline".to_string(),
+        ))
     }
 
     /// An attachment which should be displayed inline into the message, but that also
     /// species the filename in case it were to be downloaded
     pub fn inline_with_name(file_name: &str) -> Self {
-        debug_assert!(!file_name.contains('"'), "file_name shouldn't contain '\"'");
-        Self(format!("inline; filename=\"{}\"", file_name))
+        Self::with_name("inline", file_name)
     }
 
     /// An attachment which is separate from the body of the message, and can be downloaded separately
     pub fn attachment(file_name: &str) -> Self {
-        debug_assert!(!file_name.contains('"'), "file_name shouldn't contain '\"'");
-        Self(format!("attachment; filename=\"{}\"", file_name))
+        Self::with_name("attachment", file_name)
+    }
+
+    fn with_name(kind: &str, file_name: &str) -> Self {
+        let raw_value = format!("{}; filename=\"{}\"", kind, file_name);
+
+        let mut encoded_value = String::new();
+        let line_len = "Content-Disposition: ".len();
+        let mut w = EmailWriter::new(&mut encoded_value, line_len, false);
+        w.write_str(kind).expect("writing `kind` returned an error");
+        w.write_char(';').expect("writing `;` returned an error");
+        w.space();
+
+        email_encoding::headers::rfc2231::encode("filename", file_name, &mut w)
+            .expect("some Write implementation returned an error");
+
+        Self(HeaderValue::dangerous_new_pre_encoded(
+            Self::name(),
+            raw_value,
+            encoded_value,
+        ))
     }
 }
 
@@ -33,11 +59,19 @@ impl Header for ContentDisposition {
     }
 
     fn parse(s: &str) -> Result<Self, BoxError> {
-        Ok(Self(s.into()))
+        match (s.split_once(';'), s) {
+            (_, "inline") => Ok(Self::inline()),
+            (Some((kind @ ("inline" | "attachment"), file_name)), _) => file_name
+                .split_once(" filename=\"")
+                .and_then(|(_, file_name)| file_name.strip_suffix('"'))
+                .map(|file_name| Self::with_name(kind, file_name))
+                .ok_or_else(|| "Unsupported ContentDisposition value".into()),
+            _ => Err("Unsupported ContentDisposition value".into()),
+        }
     }
 
     fn display(&self) -> HeaderValue {
-        HeaderValue::new(Self::name(), self.0.clone())
+        self.0.clone()
     }
 }
 
diff --git a/src/message/header/mod.rs b/src/message/header/mod.rs
index 1b6242acc..b8236435a 100644
--- a/src/message/header/mod.rs
+++ b/src/message/header/mod.rs
@@ -275,7 +275,7 @@ impl PartialEq<HeaderName> for &str {
     }
 }
 
-#[derive(Debug, Clone)]
+#[derive(Debug, Clone, PartialEq)]
 pub struct HeaderValue {
     name: HeaderName,
     raw_value: String,
@@ -293,6 +293,18 @@ impl HeaderValue {
             encoded_value,
         }
     }
+
+    pub fn dangerous_new_pre_encoded(
+        name: HeaderName,
+        raw_value: String,
+        encoded_value: String,
+    ) -> Self {
+        Self {
+            name,
+            raw_value,
+            encoded_value,
+        }
+    }
 }
 
 const ENCODING_START_PREFIX: &str = "=?utf-8?b?";


Test patch: