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

Merge_feature active and attached file with special characters #409

Closed
pveith opened this issue Sep 4, 2018 · 8 comments
Closed

Merge_feature active and attached file with special characters #409

pveith opened this issue Sep 4, 2018 · 8 comments
Labels
Milestone

Comments

@pveith
Copy link
Contributor

pveith commented Sep 4, 2018

With an active merge_feature we get an "Sympa::Message::_merge_msg#1310 Unknown charset" in sympa.log and the message is rejected with a notification to the user.

Version

Sympa 6.2.34, Debian Postfix 3.1.8-0+deb9u1

Installation method

We build sympa from source on our testsystem and use debuild to build a package, which we -after testing- then install on our production system. Right now we have installed two patches:

  1. Fix for "open comment in cli" - sympa.pl: Add an option "--open_list=list@domain" to restore closed list #383
  2. Fix for "copylist" - Lost owners on list copy #384

Expected behavior

Send Message without touching the attachments.

Actual behavior

Send message refused by sympa and Sender is informed, that his message can not be "personalized". Deactivating the feature "merge_feature" does fix the problem, but will render our automatic unsubscribe notice via "append footer" defunctional.

Additional information

Steps to reproduce:

  • create a text file with special characters like "öäüß"
  • attache file to message to a list with active merge_feature.
    I am using Thunderbird 52.9.1 on Ubuntu to send the message. This issue came up with an CalDav invitation, where one User had special characters like "é" in his name.

relevant lines from sympa.log:

Sep  4 09:02:28 listserv-test sympa_msg[24319]: notice Sympa::Spindle::ProcessIncoming::_twist() Processing Sympa::Message <[email protected]
.de.1536044547.29615>; [email protected]; [email protected]; [email protected]
Sep  4 09:02:28 listserv-test sympa_msg[24319]: info Sympa::Spindle::DoMessage::_twist() Processing message Sympa::Message <[email protected]
.de.1536044547.29615> for Sympa::List <[email protected]> with priority 5, <[email protected]>
Sep  4 09:02:28 listserv-test sympa_msg[24319]: err main::#243 > Sympa::Spindle::spin#95 > Sympa::Spindle::AuthorizeMessage::_twist#177 > Sympa::Spindle::Aut
horizeMessage::_test_personalize#344 > Sympa::Message::personalize#1240 > Sympa::Message::_merge_msg#1267 > Sympa::Message::_merge_msg#1310 Unknown charset "
"
Sep  4 09:02:28 listserv-test sympa_msg[24319]: err main::#243 > Sympa::Spindle::spin#95 > Sympa::Spindle::AuthorizeMessage::_twist#177 > Sympa::Spindle::Aut
horizeMessage::_test_personalize#344 > Sympa::Message::personalize#1240 > Sympa::Message::_merge_msg#1268 Failed to personalize message part
Sep  4 09:02:28 listserv-test sympa_msg[24319]: err main::#243 > Sympa::Spindle::spin#95 > Sympa::Spindle::AuthorizeMessage::_twist#178 Failed to personalize
. Message Sympa::Message <[email protected]/shelved:dkim_sign> for list Sympa::List <[email protected]
> was rejected
Sep  4 09:02:28 listserv-test sympa_msg[24319]: notice Sympa::Spindle::ProcessTemplate::_twist() Processing Sympa::Message::Template <[email protected]
z.uni-bonn.de.1536044548.24319,3499>; envelope_sender=<>; message_id=sympa.1536044548.24319.31@listserv-test.rhrz.uni-bonn.de; [email protected];
 [email protected]; template=delivery_status_notification; action=failed; status=5.6.5
Sep  4 09:02:28 listserv-test sympa_msg[24319]: notice Sympa::Bulk::store() Message Sympa::Message::Template <[email protected]
.24319,3499> is stored into bulk spool as <5.5.1536044548.1536044548.314386.test1@listserv-test.rhrz.uni-bonn.de_s,24319,4292>
@pveith
Copy link
Contributor Author

pveith commented Sep 4, 2018

From my perspective sympa should not personalize "Content-Disposition: attachment;" message parts at all. Just my 2 cents.

@pveith
Copy link
Contributor Author

pveith commented Sep 4, 2018

If attachments should really remain untouched, this might be the solution to my problem. I just added a check if a part is of disposition "attachment" in scr/lib/Sympa/Message.pm line 1259, just after the encoding checks for each incoming message part.

my $cdisposition = $entity->head->mime_attr('Content-Disposition');
if ($cdisposition and $cdisposition == "attachment")
return $entity;
}

So now messageparts which are attachments are no longer changed in sub _merge_msg on my testserver. Does this interfere with the message workflow? I don't have enough knowledge of sympa code to recognize how this may backfire.

@racke
Copy link
Contributor

racke commented Sep 4, 2018 via email

@ikedas
Copy link
Member

ikedas commented Sep 5, 2018

If attachments should really remain untouched, this might be the solution to my problem. I just added a check if a part is of disposition "attachment" in scr/lib/Sympa/Message.pm line 1259, just after the encoding checks for each incoming message part.

my $cdisposition = $entity->head->mime_attr('Content-Disposition');
if ($cdisposition and $cdisposition == "attachment")
return $entity;
}

I feel it making sense and agree to your change. If possible, could you please submit PR?

However, looking some examples found in Bugzilla, I found attached iCalendar parts unnecessarily do not have Content-Disposition field (and unlike your example, they occasionalily have charset parameters). Do you think it problem?

@pveith
Copy link
Contributor Author

pveith commented Sep 5, 2018

New to github question: i need to fork the 6.2 branch to create a PR? Source: help page on PR
I will try to generate the PR and look further into this matter. The example which started this for me had two identifying features: The Content-Dispostion and an empty string set for mime_encoding for that part of the email. My first idea was to simply add a check for empty mime_encoding, but Content-Disposition seemed to narrow it down more. Thus i preferred that.

@ikedas
Copy link
Member

ikedas commented Sep 5, 2018

@pveith, you'd be better to fork sympa-6.2 branch instead of master (Currently sympa-6.2 is the practial master branch on this project).

I feel your idea to check two features is better.

@pveith
Copy link
Contributor Author

pveith commented Sep 5, 2018

I tested as much as i could and don't see any negative results. The original issue with invites (ICS) over a list including special characters is now resolved in my installation. (Tested with Apple Mail, Thunderbird & Lightning and Outlook2016 invites).
Adding charset checks added no further value, so i just checked for Content-Disposition.
Result of my testing can be found on #410

@ikedas ikedas added the bug label Sep 6, 2018
@ikedas
Copy link
Member

ikedas commented Sep 13, 2018

PR was merged. Thanks for improving Sympa!

@ikedas ikedas closed this as completed Sep 13, 2018
@ikedas ikedas added this to the 6.2.36 milestone Sep 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants