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

To: addresses of form "lastname, firstname <name@addr.com>" cause MissingTo error #690

Closed
mdharm opened this issue Oct 28, 2021 · 7 comments · Fixed by #760
Closed

To: addresses of form "lastname, firstname <name@addr.com>" cause MissingTo error #690

mdharm opened this issue Oct 28, 2021 · 7 comments · Fixed by #760

Comments

@mdharm
Copy link

mdharm commented Oct 28, 2021

When creating a message using Message::builder() and adding multiple To: headers of the format lastname, firstname <name@addr.com>, an Error::MissingTo error result is returned.

NOTE: This is different from #672 in that the issue is in how the address is parsed; in #672 the address is provided as

(Some(r#""Doe, Jóhn""#.parse().unwrap()),
        "john.doe@example.com".parse().unwrap())

but here I am using str::parse(), which is failing to detect the comma and MIME-encode the name portion. I suspect that a change in Mailbox::from_str() would be required.

This code snippit will demonstrate the issue:

use lettre::Message;

fn main() {
    let builder = Message::builder()
        .from("test@foo.com".parse().unwrap())
        .to("ln1, fn1 <addr1@foo.com>".parse().unwrap())
        .to("ln2, fn2 <addr1@foo.com>".parse().unwrap())
        .to("ln3, fn3 <addr1@foo.com>".parse().unwrap());

    println!("builder is: {:#?}", builder);
    println!(
        "result after calling body(): {:#?}",
        builder.body(String::from("Be happy!"))
    );
}

Running this produces:

builder is: MessageBuilder {
    headers: Headers {
        headers: [
            (
                HeaderName(
                    "From",
                ),
                "test@foo.com",
            ),
            (
                HeaderName(
                    "To",
                ),
                "ln3, fn3 <addr1@foo.com>",
            ),
        ],
    },
    envelope: None,
}
result after calling body(): Err(
    MissingTo,
)

If I remove the commas between the last/first names, I get the (much more reasonable):

builder is: MessageBuilder {
    headers: Headers {
        headers: [
            (
                HeaderName(
                    "From",
                ),
                "test@foo.com",
            ),
            (
                HeaderName(
                    "To",
                ),
                "ln1 fn1 <addr1@foo.com>, ln2 fn2 <addr1@foo.com>, ln3 fn3 <addr1@foo.com>",
            ),
        ],
    },
    envelope: None,
}

I'm not 100% certain what the "correct" behavior would be, but I would expect it to be something like:

            (
                HeaderName(
                    "To",
                ),
                ""ln1, fn1" <addr1@foo.com>, "ln2, fn2" <addr1@foo.com>, "ln3, fn3" <addr1@foo.com>",
            ),

This happens with Lettre version 0.10.0-rc.3

@kevincox
Copy link
Contributor

I looked into this too and it appears to be that in the builder lettre mushes headers into a comma separated list. Then when it parses them back it does so simply by splitting on comma. It seems that there are a lot of assumptions baked in here and a quick look at the code gave no obvious fix.

@kevincox
Copy link
Contributor

kevincox commented Feb 8, 2022

#698 was not enough to fix this. As a hack we could update the code to encode the , using base64 as if it was a disallowed character. IDK if this is a good idea but it would be a quick hack.

@paolobarbolini
Copy link
Member

I'm thinking of merging #670 (did I miss something for not having merged it earlier?) and then write an hacky solution dependent on https://github.com/lettre/email-encoding while we figure out a clean solution for #693

@kevincox
Copy link
Contributor

kevincox commented Feb 8, 2022

I don't think #670 is related but I would be happy to merge it. I'll leave a comment clarifying the state there.

I think a hacky fix would be nice. I am working around this my my code by running name.replace(',', "") but that is only really great for my use case. We shouldn't be "corrupting" user's requests silently.

@paolobarbolini
Copy link
Member

I don't think #670 is related but I would be happy to merge it. I'll leave a comment clarifying the state there.

It's mainly to avoid the headache around merge conflicts and internal API changes

@paolobarbolini
Copy link
Member

Should be fixed by #760

@kevincox
Copy link
Contributor

Thanks, I can confirm that this fix is working!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

3 participants