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

v3.2.13 changes the behavior of g_mime_message_get_addresses when more than one Cc header exists. #129

Closed
dkg opened this issue Oct 4, 2022 · 4 comments · Fixed by #130

Comments

@dkg
Copy link
Contributor

dkg commented Oct 4, 2022

In the notmuch test suite, we're seeing a failure to reply correctly to a message that has multiple cc headers.
From a git bisect, i found that 4a80ae5 introduces this failure:

 FAIL   Reply to a message with multiple Cc headers
	--- T220-reply.17.expected	2022-10-04 02:36:37.774311603 +0000
	+++ T220-reply.17.output	2022-10-04 02:36:37.774311603 +0000
	@@ -1,7 +1,7 @@
	 From: Notmuch Test Suite <[email protected]>
	 Subject: Re: wowsers!
	 To: Alice <[email protected]>, Daniel <[email protected]>
	-Cc: Bob <[email protected]>, Charles <[email protected]>
	+Cc: Charles <[email protected]>
	 In-Reply-To: <[email protected]>
	 References: <[email protected]>

The message that is being replied to is this simple, subtly-malformed message:

From: Alice <[email protected]>
To: Daniel <[email protected]>
Cc: Bob <[email protected]>
Subject: wowsers!
cc: Charles <[email protected]>
Message-Id: <[email protected]>
Date: Thu, 16 Jun 2016 22:14:41 -0400

Note the Cc: and cc: headers.

I note that RFC 5322 §3.6 indicates that there must be exactly 0 or 1 Cc header, but in practice, notmuch has been working off the assumption that earlier versions of gmime had, which is that multiple Cc headers could or would be collapsed and treated as a single header.

@dkg
Copy link
Contributor Author

dkg commented Oct 4, 2022

So i see two ways that we can deal with this:

  • accept that the notmuch test suite was wrong -- it should not have expected the two Cc: header fields to be collapsed into one, in deference to RFC 5322.
  • treat the upper bound on Cc: header fields indicated in the RFC as dubious and revert the behavior of gmime to its old behavior.

I'm not sure what the correct approach is. I don't know how many messages in existence carry this particular quirky violation of the standard. And i don't know how else to interpret such a message -- i mean, which Cc: should the implementation pick if it's faced with a message with multiple copies?

@dkg
Copy link
Contributor Author

dkg commented Oct 4, 2022

Attached are two simple test files:

if you build demonstrate-129 and run it as:

./demonstrate-129 < broken-cc.eml

Then you'll see that with versions before 3.2.13 the result is to show both Bob and Charles, but as of 3.2.13 (or really, as of 4a80ae5) the result is to show only Charles.

@dkg dkg changed the title v3.2.13 breaks the notmuch test suite by changing the list of Cc headers accumulated when more than one Cc header exists. v3.2.13 changes the behavior of g_mime_message_get_addresses when more than one Cc header exists. Oct 4, 2022
@dkg
Copy link
Contributor Author

dkg commented Oct 4, 2022

Hm, @ojwb points out on the #notmuch IRC channel that older versions of the specification were more flexible than RFC 5322.

RFC 5322 §4 ("Obsolete Syntax") acknowledges as much:

Earlier versions of this specification allowed for different (usually
more liberal) syntax than is allowed in this version. Also, there
have been syntactic elements used in messages on the Internet whose
interpretations have never been documented. Though these syntactic
forms MUST NOT be generated according to the grammar in section 3,
they MUST be accepted and parsed by a conformant receiver.

And in §4.5.3 ("Obsolete Destination Address Fields"), we see:

When multiple occurrences of destination address fields occur in a
message, they SHOULD be treated as if the address list in the first
occurrence of the field is combined with the address lists of the
subsequent occurrences by adding a comma and concatenating.

So as a parser, i think gmime 3.2.13 is doing the wrong thing here, and it should be fixed.

dkg added a commit to dkg/gmime that referenced this issue Oct 4, 2022
I think this resolves a copy/paste error that was introduced in
4a80ae5.

The logic for `message_add_addresses` is that it is supposed to just
be appending to a list of addresses, rather than trying to re-merge a
series of addresses.

It looks like the function's framing was copy/pasted from
`message_update_addresses`, which does the more complete
re-build/re-merge.

However, `message_update_addresses` starts with an initial
`internet_address_list_clear`, and then proceeds to cycle through the
entire list of headers, trying to collect the ones that match the
field name, so clearing the address list initially makes sense.

For `message_add_addresses` though, the goal is just to append to the
current list that already exists, so the list should not be initially
cleared.

Fixes: jstedfast#129
@jstedfast
Copy link
Owner

Oops, yes, your patch looks correct. My intention was always to have GMime combine all of the Cc: addresses into 1 InternetAddressList for convenient usage.

jstedfast pushed a commit that referenced this issue Oct 4, 2022
I think this resolves a copy/paste error that was introduced in
4a80ae5.

The logic for `message_add_addresses` is that it is supposed to just
be appending to a list of addresses, rather than trying to re-merge a
series of addresses.

It looks like the function's framing was copy/pasted from
`message_update_addresses`, which does the more complete
re-build/re-merge.

However, `message_update_addresses` starts with an initial
`internet_address_list_clear`, and then proceeds to cycle through the
entire list of headers, trying to collect the ones that match the
field name, so clearing the address list initially makes sense.

For `message_add_addresses` though, the goal is just to append to the
current list that already exists, so the list should not be initially
cleared.

Fixes: #129
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants