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

Fix quote-handling regression from Ragel upgrade #1023

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
6 participants
@garethrees
Contributor

garethrees commented Aug 25, 2016

Came across a regression in how quotes are handled in display names
after upgrading mysociety/alaveteli from Mail 2.5.4 to 2.6.x, similar to
issues #929 and #1017.

This commit fixes #929 and #1017.

I've included a spec similar to that demonstrated in #1017 and one that
mirrors the spec that highlighted the regression in mysociety/alaveteli.

Also included a spec to cover a quoted display name containing
backslash-escaped double quotes and parenthesis, with comments inside and
outside the address section.

I haven't included a spec for the failure example in #929, because I think that
Mail::FromField should expect the Mail::Address to be correct. The
spec for #1017 also covers the example in #929.

It would be great to see this released in the 2.6.x series. The commit cherry-picks cleanly to the 2-6-stable branch.


Finding the regression

I used git bisect to figure out which commit introduced the bug. I'd just upgraded from 2.5.4 to 2.6.4, so I knew it was somewhere between there.

git bisect start
#2.6.4
git bisect good 0227973b83f488ca9ab87461d83d7eb7cf665c88
#2.5.4
git bisect bad 86101c6843a47c1c5444ffdc533ac8677e1f7045

bundle --quiet
git bisect run bundle exec ~/mail_bisect

Here's the test I was running…

#!/bin/bash
bundle > /dev/null 2>&1

# A bit hacky, but does the job
/home/vagrant/.rbenv/shims/ruby <<-EOF
require '/code/mail/lib/mail'

str = '"Mikel \" Lindsaar" <test@lindsaar.net>'
address = Mail::Address.new('"Mikel \" Lindsaar" <test@lindsaar.net>')

if address.format == str
  exit 0
else
  exit 1
end
EOF

…and here's the bisect output, pointing at 2da7c79 as being the offending commit:

running bundle exec /home/vagrant/mail_bisect
Bisecting: 75 revisions left to test after this (roughly 6 steps)
[133ac26fefbd215f77496628af4896f886f424c8] Merge pull request #636 from jzinn/patch-1
running bundle exec /home/vagrant/mail_bisect
Bisecting: 36 revisions left to test after this (roughly 5 steps)
[bdb11d20bd4cba062b8cca4914ffea355da1d3cd] Merge pull request #576 from srawlins/add-rdoc-exclude-to-gemspec
running bundle exec /home/vagrant/mail_bisect
Bisecting: 18 revisions left to test after this (roughly 4 steps)
[dd89a6fa8b8af1f51d012434e54ba06cf2ae3255] Updating CHANGELOG
running bundle exec /home/vagrant/mail_bisect
Bisecting: 9 revisions left to test after this (roughly 3 steps)
[aaaac219deab053e64fdfe6c03f70a4ef016fc2f] Merge branch 'pure_ruby_ragel_parser' of git://github.com/bpot/mail into bpot-pure_ruby_ragel_parser
running bundle exec /home/vagrant/mail_bisect
Bisecting: 4 revisions left to test after this (roughly 2 steps)
[ab2af8c99c39055e7a26bfd7a72cbb42e34ee38c] common.rl: use clearer .* in dot_atom_text, instead of (.+)?
running bundle exec /home/vagrant/mail_bisect
Bisecting: 1 revision left to test after this (roughly 1 step)
[c6b656c230692ce048b27dd0af84f883a545d375] AddressListsParser: Don't add an extra address to the address list when it ends with a space.
running bundle exec /home/vagrant/mail_bisect
Bisecting: 0 revisions left to test after this (roughly 0 steps)
[2da7c7985c221272f6451b27ab8b41e84e0a6804] Replace Treetop parser with a Ragel based parser
running bundle exec /home/vagrant/mail_bisect
2da7c7985c221272f6451b27ab8b41e84e0a6804 is the first bad commit
commit 2da7c7985c221272f6451b27ab8b41e84e0a6804
Author: Bob Potter <bobby.potter@gmail.com>
Date:   Mon Jan 7 18:40:55 2013 -0600

    Replace Treetop parser with a Ragel based parser

:100644 100644 5c66e374a85607536878ed053b449b8881215cf1 87515b677a58c7568be0488739d7ead84abdf489 M      Gemfile
:040000 040000 adcb1908e068b4757f7dde88dd76d4af2ec2f60b 4959b72cdb14f94f77dea737aa05a8a4fa0bb7db M      lib
:100644 100644 07c8ff9d332960312f810ebc2fc13adc39793dac a0c6568783567129e3984560382c68d96c8bfbbe M      mail.gemspec
:040000 040000 4ab53c797d99999375c0e8d5f7cd0e04b6805596 5a2f224407a2ebe41ceba37ab73f0258293e5d91 M      reference
:040000 040000 ea53d75476823259ad0581e3eb307d289ffc1c69 5c8e71c8f98c37425b273f8355afe41eaa0bcf8a M      spec
bisect run success

I've attached two patches that apply passing specs for the examples provided in #929 and #1017.

mail-929.patch.txt
mail-1017-spec.patch.txt

Fix quote-handling regression from Ragel upgrade
Came across a regression in how quotes are handled in display names
after upgrading mysociety/alaveteli from Mail 2.5.4 to 2.6.x, similar to
issues #929 and #1017.

This commit fixes #929 and #1017.

I've included a spec similar to that demonstrated in #1017 and one that
mirrors the spec that highlighted the regression in mysociety/alaveteli.

Also included a spec to cover a quoted display name containing
backslash-escaped double quotes and parenthesis, with comments inside and
outside the address section.

I haven't included a spec for the failure example in #929, because I think that
`Mail::FromField` should expect the `Mail::Address` to be correct. The
spec for #1017 also covers the example in #929.
@stanhu

This comment has been minimized.

Show comment
Hide comment

stanhu commented Aug 27, 2016

👍

# string = '"This is \"a string\""'
# unescape(string) #=> '"This is "a string""'
def unescape( str )
str.gsub(/\\(.)/, '\1')

This comment has been minimized.

@muteor

muteor Aug 28, 2016

Looking at the RFC https://tools.ietf.org/html/rfc2822#section-3.2.2 seems that it would be safe to do /\\"/ and only replace quoted pairs, as \ is a valid char within the text. This might not catch double escaping like \\" but that is sort of expected.

@muteor

muteor Aug 28, 2016

Looking at the RFC https://tools.ietf.org/html/rfc2822#section-3.2.2 seems that it would be safe to do /\\"/ and only replace quoted pairs, as \ is a valid char within the text. This might not catch double escaping like \\" but that is sort of expected.

This comment has been minimized.

@garethrees

garethrees Sep 1, 2016

Contributor

That doesn't seem quite right to me:

str = '"Hello \" World"'

puts str.gsub(/\\(.)/, '\1')
# Expected
# => "Hello " World"

puts str.gsub(/\\"/, '\1')
# Missing dquote in the middle
# => "Hello  World"

Edit: this was the original failure case we found when upgrading to 2.6.x

@garethrees

garethrees Sep 1, 2016

Contributor

That doesn't seem quite right to me:

str = '"Hello \" World"'

puts str.gsub(/\\(.)/, '\1')
# Expected
# => "Hello " World"

puts str.gsub(/\\"/, '\1')
# Missing dquote in the middle
# => "Hello  World"

Edit: this was the original failure case we found when upgrading to 2.6.x

This comment has been minimized.

@muteor

muteor Sep 3, 2016

I was thinking something like this:

http://refiddle.com/refiddles/57ca78c875622d7947c32a00

The spec says that \" is the only escaping pair so all other \ chars should be maintained.

@muteor

muteor Sep 3, 2016

I was thinking something like this:

http://refiddle.com/refiddles/57ca78c875622d7947c32a00

The spec says that \" is the only escaping pair so all other \ chars should be maintained.

@muteor

This comment has been minimized.

Show comment
Hide comment
@muteor

muteor Aug 28, 2016

Cool, nice find, would be good to get this merged :)

muteor commented Aug 28, 2016

Cool, nice find, would be good to get this merged :)

@itsNikolay

This comment has been minimized.

Show comment
Hide comment
@stanhu

This comment has been minimized.

Show comment
Hide comment
@stanhu

stanhu Sep 12, 2016

@jeremy Would you mind taking a look at this? This bug is affecting many of our users. Thanks!

stanhu commented Sep 12, 2016

@jeremy Would you mind taking a look at this? This bug is affecting many of our users. Thanks!

@jeremy

This comment has been minimized.

Show comment
Hide comment
@jeremy

jeremy Sep 12, 2016

Collaborator

Nice find and fix! Checking it out.

Collaborator

jeremy commented Sep 12, 2016

Nice find and fix! Checking it out.

@stanhu

This comment has been minimized.

Show comment
Hide comment
@stanhu

stanhu Sep 15, 2016

@jeremy Just a friendly reminder here to take a look! Thank you for all your work.

stanhu commented Sep 15, 2016

@jeremy Just a friendly reminder here to take a look! Thank you for all your work.

@wyefei

This comment has been minimized.

Show comment
Hide comment
@wyefei

wyefei Oct 27, 2016

@jeremy Can we please move forward with this? Thank you so much!

wyefei commented Oct 27, 2016

@jeremy Can we please move forward with this? Thank you so much!

@jeremy

This comment has been minimized.

Show comment
Hide comment
@jeremy

jeremy Feb 2, 2017

Collaborator

Merged @ 254fee8 and backported to 2-6-stable @ e63023f.

Collaborator

jeremy commented Feb 2, 2017

Merged @ 254fee8 and backported to 2-6-stable @ e63023f.

@jeremy jeremy closed this Feb 2, 2017

@stanhu

This comment has been minimized.

Show comment
Hide comment
@stanhu

stanhu Feb 19, 2017

@jeremy Thanks! Would you mind releasing 2.6.5?

stanhu commented Feb 19, 2017

@jeremy Thanks! Would you mind releasing 2.6.5?

mysociety-pusher pushed a commit to mysociety/alaveteli that referenced this pull request Apr 27, 2017

Update mail
Includes bugfix for quote-handling regression:
mikel/mail#1023

@garethrees garethrees referenced this pull request Apr 27, 2017

Merged

Update mail #3933

mysociety-pusher pushed a commit to mysociety/alaveteli that referenced this pull request Apr 27, 2017

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