Ruby on Rails | Screencasts | Download | Documentation | Weblog | Community | Source

Ticket #10317 (closed defect: fixed)

Opened 7 months ago

Last modified 7 months ago

TMail scanner.rb fails to handle @ character properly

Reported by: mikel Assigned to: core
Priority: normal Milestone: 2.0
Component: ActionMailer Version: edge
Severity: normal Keywords: [PATCH] TMail, ActionMailer
Cc: bitsweat, nzkoz, david

Description

The scanner.rb module inside of TMail fails to handle the '@' symbol in address and header checks.

The C version of scanner.rb handles these correctly.

This error only comes up when you are using the pure ruby version (which Rails does). If you are using the TMail gem, then it is not usually a problem as that usually uses the C version compiled in.

Handling is removing the @ symbol as possible ATOM character - this is per RFC 2822 Point 3.2.4

Patch is attached. You can only really see this work inside the TMail test suite as it fixes an internal TMail problem - I don't want to make a test case for TMail inside of ActionMailer.

You can see the error for yourself by checking out TMail revision 150 running "script/clobber/distclean" then "script/test" from the TMail trunk directory, then checking out HEAD and running the same.

Mikel Lindsaar TMail maintainer.

Attachments

tmail.diff (0.6 kB) - added by mikel on 11/30/07 08:18:21.
TMail fix to scanner.rb
update_tmail_testing_unquoted_at_char_and_base64.diff (6.9 kB) - added by mikel on 12/02/07 16:56:39.
This is off revision 8255

Change History

11/30/07 08:18:21 changed by mikel

  • attachment tmail.diff added.

TMail fix to scanner.rb

11/30/07 08:19:09 changed by mikel

There are other changes to TMail to bring it up to 1.2.0, but this can wait till Rails 2.1 I think. Anyone can update to the Gem if they want (we should probably mention this somwhere as well).

12/01/07 08:06:21 changed by nzkoz

  • status changed from new to closed.
  • resolution set to fixed.

(In [8246]) Remove @ as an ATOM character, syncing with upstream tmail. [mikel] Closes #10317

12/02/07 00:23:00 changed by nzkoz

  • status changed from closed to reopened.
  • resolution deleted.

Turns out we added these for a reason:

#1206 and [1476] have the details.

What's the proper resolution here?

12/02/07 08:44:05 changed by mikel

OK, I'll check this out. Having it in there definitely in alignment with RFC 2822.

I'll add the test cases and see how else this could be solved. Putting the @ char in there actually breaks about 20 other header related tests inside TMail's test suite. So I'll find a better solution.

Mikel

12/02/07 10:16:23 changed by mikel

Alright, done a bit of investigation and Mail.app no longer has this bug. But I have sent an email off to the RFC 822 mailing list to check on the current way to handle this, or if it is ignored because no one else sees it. Will wait for that response and either do a patch for it or delete the test case within 24 hours.

Mikel

12/02/07 16:53:55 changed by mikel

OK. Research done.

Here is the final data:

1) an unquoted @ outside of the email address is totally a bug and not part of the RFC.

2) Apple's Mail.app (which is why this was patched originally) had this bug, it is now fixed in the later releases.

So we don't need this patch anymore to patch Apple's unpatched now patched software. Make sense? :)

So I removed the test case and the fixture that the test case used.

I also updated Base64.rb in the tmail directory, to remove out the C extensions (TMail 1.2.0 doesn't use them any more as they are about the same speed as the ruby ones) additionally I fixed up the test cases on Base64 so that they would run if you had the TMail gem installed OR the TMail bundled version inside rails installed.

I didn't update the rest of TMail (there are about 5 other patches) as we are at RC 2.0 and this should wait for 2.1. The Base64 one is harmless as Rails does not use the C libraries anyway.

All tests now pass.

The diff is called update_tmail_testing_unquoted_at_char_and_base64.diff

Mikel

12/02/07 16:56:39 changed by mikel

  • attachment update_tmail_testing_unquoted_at_char_and_base64.diff added.

This is off revision 8255

12/02/07 20:31:42 changed by nzkoz

  • status changed from reopened to closed.
  • resolution set to fixed.

(In [8257]) Remove old tests which relied on @ being an ATOM to work around old Mail.app bugs. Closes #10317 [mikel]