Skip to content

Handle edge case where attachment part is missing filename #1332

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

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.rdoc
Original file line number Diff line number Diff line change
Expand Up @@ -17,5 +17,7 @@ Compatibility:
Features:
* Message#inspect_structure and PartsList#inspect_structure pretty-print the hierarchy of message parts. (TylerRick)

Bugs:
* Handle case when attachment parts do not contain a filename or Content-Location header. (lao9)

Please check [2-7-stable](https://github.com/mikel/mail/blob/2-7-stable/CHANGELOG.rdoc) for previous changes.
16 changes: 16 additions & 0 deletions lib/mail/fields/content_type_field.rb
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,18 @@ def filename
@filename ||= parameters['filename'] || parameters['name']
end

def attachment_name
case
when filename
@attachment_name = filename
when string && !non_attachment_types.include?(string)
@attachment_name = "untitled"
else
@attachment_name = nil
end
@attachment_name
end

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels like the message itself should be responsible for determining whether it's an attachment, rather than its Content-Type field.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking a look and providing feedback @jeremy . I can re-work this a bit and get back to you. Agree that the longer round-trip isn't ideal.

def encoded
p = ";\r\n\s#{parameters.encoded}" if parameters && parameters.length > 0
"#{name}: #{content_type}#{p}\r\n"
Expand Down Expand Up @@ -168,5 +180,9 @@ def get_mime_type(val)
'text/plain'
end
end

def non_attachment_types
['text/plain', 'text/html', 'text/enriched', 'application/x-yaml', 'multipart/alternative', 'multipart/mixed']
end
end
end
3 changes: 3 additions & 0 deletions lib/mail/message.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2119,13 +2119,16 @@ def find_attachment
content_type_name = header[:content_type].filename rescue nil
content_disp_name = header[:content_disposition].filename rescue nil
content_loc_name = header[:content_location].location rescue nil
content_type_attachment_name = header[:content_type].attachment_name rescue nil
case
when content_disposition && content_disp_name
filename = content_disp_name
when content_type && content_type_name
filename = content_type_name
when content_location && content_loc_name
filename = content_loc_name
when content_type && content_type_attachment_name
filename = content_type_attachment_name
else
filename = nil
end
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
Mime-Version: 1.0 (Apple Message framework v730)
Content-Type: multipart/mixed; boundary=Apple-Mail-13-196941151
Message-Id: <[email protected]>
From: [email protected]
Subject: testing
Date: Mon, 6 Jun 2005 22:21:22 +0200
To: [email protected]


--Apple-Mail-13-196941151
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain;
charset=ISO-8859-1;
delsp=yes;
format=flowed

This is the first part.

--Apple-Mail-13-196941151
Content-Type: image/jpeg
Content-Transfer-Encoding: base64
Content-ID: <qbFGyPQAS8>
Content-Disposition: inline

jamisSqGSIb3DQEHAqCAMIjamisxCzAJBgUrDgMCGgUAMIAGCSqGSjamisEHAQAAoIIFSjCCBUYw
ggQujamisQICBD++ukQwDQYJKojamisNAQEFBQAwMTELMAkGA1UEBhMCRjamisAKBgNVBAoTA1RE
QzEUMBIGjamisxMLVERDIE9DRVMgQ0jamisNMDQwMjI5MTE1OTAxWhcNMDYwMjamisIyOTAxWjCB
gDELMAkGA1UEjamisEsxKTAnBgNVBAoTIEjamisuIG9yZ2FuaXNhdG9yaXNrIHRpbjamisRuaW5=

--Apple-Mail-13-196941151--
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
Mime-Version: 1.0 (Apple Message framework v730)
Content-Type: multipart/mixed; boundary=Apple-Mail-13-196941151
Message-Id: <[email protected]>
From: [email protected]
Subject: testing
Date: Mon, 6 Jun 2005 22:21:22 +0200
To: [email protected]


--Apple-Mail-13-196941151
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain;
charset=ISO-8859-1;
delsp=yes;
format=flowed

This is the first part.

--Apple-Mail-13-196941151
Content-Type: image/jpeg; filename="Photo25.jpg"
Content-Transfer-Encoding: base64
Content-ID: <qbFGyPQAS8>
Content-Disposition: inline

jamisSqGSIb3DQEHAqCAMIjamisxCzAJBgUrDgMCGgUAMIAGCSqGSjamisEHAQAAoIIFSjCCBUYw
ggQujamisQICBD++ukQwDQYJKojamisNAQEFBQAwMTELMAkGA1UEBhMCRjamisAKBgNVBAoTA1RE
QzEUMBIGjamisxMLVERDIE9DRVMgQ0jamisNMDQwMjI5MTE1OTAxWhcNMDYwMjamisIyOTAxWjCB
gDELMAkGA1UEjamisEsxKTAnBgNVBAoTIEjamisuIG9yZ2FuaXNhdG9yaXNrIHRpbjamisRuaW5=

--Apple-Mail-13-196941151--
25 changes: 25 additions & 0 deletions spec/mail/fields/content_type_field_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -674,6 +674,31 @@

end

describe "finding an attachment_name" do

it "should return the filename if there is a filename" do
string = %q{application/octet-stream; filename=mikel.jpg}
c = Mail::ContentTypeField.new(string)
expect(c.attachment_name).to eq 'mikel.jpg'
end

it "should return nil if it is missing a filename and not an attachment type" do
c = Mail::ContentTypeField.new(['text', 'html', {'charset' => 'UTF-8'}])
expect(c.attachment_name).to eq nil
end

it "should return nil if it is missing a filename and has no attachment type" do
c = Mail::ContentTypeField.new
expect(c.attachment_name).to eq nil
end

it "should return 'untitled' if it missing a filename and an attachment type" do
c = Mail::ContentTypeField.new('image/jpg')
expect(c.attachment_name).to eq 'untitled'
end

end

describe "handling badly formated content-type fields" do

it "should handle missing sub-type on a text content type" do
Expand Down
43 changes: 42 additions & 1 deletion spec/mail/message_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1432,7 +1432,7 @@ def message_headers_should_match(message, other)
part.body = 'a' * 999
end
mail.encoded

expect(mail.parts.count).to eq(1)
expect(mail.parts.last.content_transfer_encoding).to match(/7bit|8bit|binary/)
end
Expand All @@ -1455,6 +1455,47 @@ def message_headers_should_match(message, other)
end
end

describe "find_attachment" do
context "with content location" do
subject do
read_fixture('emails', 'attachment_emails', 'attachment_content_location.eml')
end

it "returns content location filename" do
expect(subject.parts[0].send(:find_attachment)).to eq(nil)
expect(subject.parts[0].attachment?).to eq(false)
expect(subject.parts[1].send(:find_attachment)).to eq("Photo25.jpg")
expect(subject.parts[1].attachment?).to eq(true)
end
end

context "with content type filename" do
subject do
read_fixture('emails', 'attachment_emails', 'attachment_with_filename.eml')
end

it "returns content type filename" do
expect(subject.parts[0].send(:find_attachment)).to eq(nil)
expect(subject.parts[0].attachment?).to eq(false)
expect(subject.parts[1].send(:find_attachment)).to eq("Photo25.jpg")
expect(subject.parts[1].attachment?).to eq(true)
end
end

context "without content location or filename" do
subject do
read_fixture('emails', 'attachment_emails', 'attachment_missing_filename.eml')
end

it "returns 'untitled' for missing attachment name" do
expect(subject.parts[0].send(:find_attachment)).to eq(nil)
expect(subject.parts[0].attachment?).to eq(false)
expect(subject.parts[1].send(:find_attachment)).to eq("untitled")
expect(subject.parts[1].attachment?).to eq(true)
end
end
end

describe "content-transfer-encoding" do

it "should use 7bit for only US-ASCII chars" do
Expand Down