Skip to content

Commit

Permalink
Improve resilience to unparsable outlook attachments
Browse files Browse the repository at this point in the history
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
  • Loading branch information
garethrees committed Oct 14, 2020
1 parent 1f3f630 commit edd9c9c
Show file tree
Hide file tree
Showing 3 changed files with 23 additions and 2 deletions.
14 changes: 13 additions & 1 deletion lib/mail_handler/backends/mail_backend.rb
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ def cc(val = nil)
module MailHandler
module Backends
module MailBackend
include ConfigHelper

def backend
'Mail'
Expand Down Expand Up @@ -186,7 +187,18 @@ def decode_attached_part(part, parent_mail)
part.content_type = 'text/plain'
end
elsif is_outlook?(part)
part.rfc822_attachment = mail_from_outlook(part.body.decoded)
begin
part.rfc822_attachment = mail_from_outlook(part.body.decoded)
rescue Encoding::CompatibilityError => e
if send_exception_notifications?
data = { message: 'Exception while parsing outlook attachment.',
parent_mail: parent_mail.inspect }
ExceptionNotifier.notify_exception(e, data: data)
end

part.rfc822_attachment = nil
end

if part.rfc822_attachment.nil?
# Attached mail didn't parse, so treat as binary
part.content_type = 'application/octet-stream'
Expand Down
3 changes: 3 additions & 0 deletions script/handle-mail-replies.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@
$:.push(File.join($alaveteli_dir, 'lib'))
load 'configuration.rb'

$:.push(File.join($alaveteli_dir, 'app', 'helpers'))
require 'config_helper'

$:.push(File.join($alaveteli_dir, 'lib', 'mail_handler'))
require 'mail_handler'
require 'reply_handler'
Expand Down
8 changes: 7 additions & 1 deletion spec/lib/mail_handler/backends/mail_backend_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -286,5 +286,11 @@

end


describe '#decode_attached_part' do
it 'does not error if mapi cannot parse a part' do
allow(Mapi::Msg).to receive(:open).and_raise(Encoding::CompatibilityError)
mail = get_fixture_mail('incoming-request-oft-attachments.email')
expect { decode_attached_part(mail.parts.last, mail) }.not_to raise_error
end
end
end

0 comments on commit edd9c9c

Please sign in to comment.