Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Mail parts should have no "Date" or "Mime-Version" headers. #389

Closed
wants to merge 3 commits into from

3 participants

@djmaze

Includes specs. Tested on ree-1.8.7-2011.03 and ruby-1.9.2-p180.

@djmaze

Sorry, seems like it doesn't really fix the problem we are seeing with Outlook 10.

The current behaviour isn't standards compliant nonetheless.

@mikel
Owner

Hey there, happy to merge this if it is not RFC compliant. Do you know what part of the RFCs say this should not be included? If so, I'll merge straight away.

@jeremy jeremy commented on the diff
lib/mail/part.rb
@@ -9,7 +9,7 @@ class Part < Message
#
# It will preserve the content ID you specify if you do.
def add_content_id(content_id_val = '')
- header['content-id'] = content_id_val
+ header['content-id'] = content_id_val if inline?
@jeremy Collaborator
jeremy added a note

Content-ID is optional. No need to limit it to inline attachments.

@jeremy Collaborator
jeremy added a note

Old Outlook and Windows Phone treat parts with Content-ID as inline, even if they're attachments. So this is worth pursuing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jeremy jeremy commented on the diff
lib/mail/part.rb
@@ -34,9 +34,9 @@ def url
end
def inline?
- header[:content_disposition].disposition_type == 'inline' if header[:content_disposition]
+ header[:content_disposition].nil? || header[:content_disposition].disposition_type == 'inline'
@jeremy Collaborator
jeremy added a note

This is a little clearer, but it's unrelated to the purpose of the pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jeremy
Collaborator

http://tools.ietf.org/html/rfc2045#section-4

   Note that the MIME-Version header field is required at the top level
   of a message.  It is not required for each body part of a multipart
   entity.  It is required for the embedded headers of a body of type
   "message/rfc822" or "message/partial" if and only if the embedded
   message is itself claimed to be MIME-conformant.

MIME-Version is required for some parts and optional for others. It's not safe to always omit it.

Date is a message header, not a MIME header. So it's safe to omit.

@jeremy jeremy commented on the diff
lib/mail/message.rb
@@ -1942,8 +1942,10 @@ def add_required_fields
add_multipart_mixed_header unless !body.multipart?
body = nil if body.nil?
add_message_id unless (has_message_id? || self.class == Mail::Part)
- add_date unless has_date?
- add_mime_version unless has_mime_version?
+ unless is_a?(Part)
+ add_date unless has_date?
+ add_mime_version unless has_mime_version?
+ end
@jeremy Collaborator
jeremy added a note

Consider extracting to a separate Mail::Message#add_required_message_fields and override in Mail::Part#add_required_message_fields to do nothing. Or override Mail::Part#add_required_fields entirely—clearer than special casing with class-conditionals here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jeremy jeremy closed this pull request from a commit
@jeremy jeremy Fix that Outlook and Windows Phone treat all our MIME parts as inline…
… attachments since they include unused, unreferenced Content-IDs.

Closes #389
aa394d0
@jeremy jeremy closed this in aa394d0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 30, 2012
  1. @djmaze

    Mail parts should have no "Date" or "Mime-Version" headers.

    djmaze authored
    Fixes attachments not shown in Outlook 10.
Commits on May 4, 2012
  1. @djmaze
  2. @djmaze

    Attachments which are not inline do not need a content-id.

    djmaze authored
    (Possible fix for attachments not shown in Outlook 10.)
This page is out of date. Refresh to see the latest.
View
6 lib/mail/message.rb
@@ -1942,8 +1942,10 @@ def add_required_fields
add_multipart_mixed_header unless !body.multipart?
body = nil if body.nil?
add_message_id unless (has_message_id? || self.class == Mail::Part)
- add_date unless has_date?
- add_mime_version unless has_mime_version?
+ unless is_a?(Part)
+ add_date unless has_date?
+ add_mime_version unless has_mime_version?
+ end
@jeremy Collaborator
jeremy added a note

Consider extracting to a separate Mail::Message#add_required_message_fields and override in Mail::Part#add_required_message_fields to do nothing. Or override Mail::Part#add_required_fields entirely—clearer than special casing with class-conditionals here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
add_content_type unless has_content_type?
add_charset unless has_charset?
add_content_transfer_encoding unless has_content_transfer_encoding?
View
6 lib/mail/part.rb
@@ -9,7 +9,7 @@ class Part < Message
#
# It will preserve the content ID you specify if you do.
def add_content_id(content_id_val = '')
- header['content-id'] = content_id_val
+ header['content-id'] = content_id_val if inline?
@jeremy Collaborator
jeremy added a note

Content-ID is optional. No need to limit it to inline attachments.

@jeremy Collaborator
jeremy added a note

Old Outlook and Windows Phone treat parts with Content-ID as inline, even if they're attachments. So this is worth pursuing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
end
# Returns true if the part has a content ID field, the field may or may
@@ -34,9 +34,9 @@ def url
end
def inline?
- header[:content_disposition].disposition_type == 'inline' if header[:content_disposition]
+ header[:content_disposition].nil? || header[:content_disposition].disposition_type == 'inline'
@jeremy Collaborator
jeremy added a note

This is a little clearer, but it's unrelated to the purpose of the pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
end
-
+
def add_required_fields
add_content_id unless has_content_id?
super
View
2  spec/mail/attachments_list_spec.rb
@@ -162,7 +162,7 @@ def check_decoded(actual, expected)
describe "getting the content ID from an inline attachment" do
before(:each) do
@mail = Mail.new
- @mail.attachments['test.gif'] = File.read(fixture('attachments', 'test.gif'))
+ @mail.attachments.inline['test.gif'] = File.read(fixture('attachments', 'test.gif'))
@cid = @mail.attachments['test.gif'].content_id
end
View
15 spec/mail/part_spec.rb
@@ -9,6 +9,12 @@
part.to_s
part.content_id.should_not be_nil
end
+
+ it "should not put content-ids into parts which are not inline" do
+ part = Mail::Part.new(:content_disposition => 'attachment')
+ part.to_s
+ part.content_id.should be_nil
+ end
it "should preserve any content id that you put into it" do
part = Mail::Part.new do
@@ -140,6 +146,15 @@
@delivery_report.should_not be_retryable
end
+ it "should not have a date header" do
+ @delivery_report.add_required_fields
+ @delivery_report.header['Date'].should be_nil
+ end
+
+ it "should not have a mime-version header" do
+ @delivery_report.add_required_fields
+ @delivery_report.header['Mime-Version'].should be_nil
+ end
end
it "should correctly parse plain text raw source and not truncate after newlines - issue 208" do
Something went wrong with that request. Please try again.