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

Fixes #15 and Fixes #9 #16

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

acolchagoff
Copy link

@acolchagoff acolchagoff commented Jun 14, 2019

Here's my take on the UTF-8 fix.

@acolchagoff
Copy link
Author

After spending sever hours on this I've concluded that Ruby just wants to use UTF-8, in order to get this all to play nice with ASCII-8BIT we would need to force ascii encoding all over the place. it would be ridiculous. Unless someone has a way to specifically change encoding to ascii all string operations in an entire class, I believe this is the only real solution to this problem.

@pshires
Copy link

pshires commented Aug 13, 2019

This helped me with some issues I was running into, thanks!

@acolchagoff
Copy link
Author

@aquasync can I get your feedback here?

@aquasync
Copy link
Owner

Hey @denodster, sorry haven't really had time to look at this of late. While your fix sort of works, I've discussed briefly on another issue here, what I think is the right way forward - essentially that mime objects should be treated as binary rather than encoded text. It shouldn't be too much work to fix, though whether it makes sense to do that or just move it to using an external library for the rfc822/2045 stuff I'm not sure (I started down that track with tmail a long time ago).

@acolchagoff
Copy link
Author

@aquasync I fear you're letting 'great' be the enemy of 'good' here. This gem crashes with quite a few of the emails I throw at it and the fix I've implemented has worked fine for me so far. Your idea is great, but I really wouldn't know where to start and it appears that in 5+ years no one else has taken a wack at it either. Perhaps make a TODO to do it your way when the opportunity comes along or another bug presents itself? The current version of the gem is pretty broken because of this issue. I was using the release in production and we had quite a few people encounter this crash. If you disagree, I understand, and I'll stick with our fork until either I have time to do this the better way or you get around to it.

mysociety-pusher pushed a commit to mysociety/alaveteli that referenced this pull request Oct 13, 2020
Sometimes we receive messages with outlook attachments that can't be
parsed due to an issue in mapi [1].

There's a potential fix [2] but it conflicts [3] with an existing patch
we apply [4].

This at least allows users to download the raw attachment, rather than
us preventing the entire request from loading because we raise an
exception.

Part of #5783.

[1] aquasync/ruby-msg#15
[2] aquasync/ruby-msg#16
[3] #5783 (comment)
[4] mysociety/ruby-msg#3
mysociety-pusher pushed a commit to mysociety/alaveteli that referenced this pull request Oct 14, 2020
Sometimes we receive messages with outlook attachments that can't be
parsed due to an issue in mapi [1].

There's a potential fix [2] but it conflicts [3] with an existing patch
we apply [4].

This at least allows users to download the raw attachment, rather than
us preventing the entire request from loading because we raise an
exception.

It's not easy to include an attachment in the specs to replicate a real
error case due to the complexity of removing PII, so I've just stubbed
the call to `.open` as we don't care about the specifics in this spec.

`script/handle-mail-replies` needs an explicit require as we minimise
the load path for that script.

Part of #5783.

[1] aquasync/ruby-msg#15
[2] aquasync/ruby-msg#16
[3] #5783 (comment)
[4] mysociety/ruby-msg#3
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 this pull request may close these issues.

3 participants