Skip to content

Fix decoding strings that contain '=?', but are not actually encoded#509

Closed
javan wants to merge 2 commits intomikel:masterfrom
javan:master
Closed

Fix decoding strings that contain '=?', but are not actually encoded#509
javan wants to merge 2 commits intomikel:masterfrom
javan:master

Conversation

@javan
Copy link
Contributor

@javan javan commented Feb 8, 2013

I ran into an exception today when sending an email that contained "1+1=?" in the subject.

Here's a test (not included in the commit) to simulate that failure:

it "should allow setting the value of a field to a string that contains '?='"  do
  mail = Mail.new
  mail.charset = 'utf-8'
  mail.subject = "1+1=?"
  mail[:subject].encoded.should eq "1+1=?"
end
Failures:

  1) mail encoding using default encoding should allow setting the value of a field to a string that contains '?='
     Failure/Error: mail[:subject].encoded.should eq "1+1=?"
     NoMethodError:
       undefined method `+' for nil:NilClass
     # ./lib/mail/encodings.rb:295:in `block in collapse_adjacent_encodings'
     # ./lib/mail/encodings.rb:291:in `each'
     # ./lib/mail/encodings.rb:291:in `collapse_adjacent_encodings'
     # ./lib/mail/encodings.rb:120:in `value_decode'
     # ./lib/mail/encodings.rb:101:in `decode_encode'
     # ./lib/mail/fields/unstructured_field.rb:73:in `do_decode'
     # ./lib/mail/fields/unstructured_field.rb:55:in `decoded'
     # ./lib/mail/fields/unstructured_field.rb:122:in `fold'
     # ./lib/mail/fields/unstructured_field.rb:103:in `wrapped_value'
     # ./lib/mail/fields/unstructured_field.rb:69:in `do_encode'
     # ./lib/mail/fields/unstructured_field.rb:51:in `encoded'
     # ./lib/mail/field.rb:167:in `method_missing'
     # ./spec/mail/encoding_spec.rb:25:in `block (3 levels) in <top (required)>'

This fixes the problem by checking more closely (with a regex) to see if the string actually is encoded before proceeding.

@bpot
Copy link
Contributor

bpot commented Feb 8, 2013

Seeing this problem too. 👍

@jeremy
Copy link
Collaborator

jeremy commented Feb 8, 2013

Glad to see this fixed! However, the test should pass with those first two lines removed, considering that's meant as an optimization to short circuit the transfer-encoding checks.

@jeremy jeremy mentioned this pull request Feb 8, 2013
Merged
@javan
Copy link
Contributor Author

javan commented Feb 8, 2013

They don't currently pass with those two lines removed.

...F............................F.................................FF..........................................................F...*........*...........FFFF...F......F.F...FFFF..FF....FF.FF.FF..FFFFF*..F.FFF..F..........................FF.........*.F................................................FFF.....................FF*..F....F..............................FF..FFFFF.***.F.F.F.F.............................F...................FF........FF.............FF.....F..............................FFF......................................................................................................................................................................FF...........................................................................FF.......FF.......FF.............FF...........FF......FF.......FF....*FF............F....FF.............F.FFF..FFFFFFFFFFFF......................................F....................................F.................F.....................F...........................F.....F........FFFFFFF......FFFF..FFFF.FFFF.................F.....................FFFFFFF........F....FFFFFFFFFFFFFFF....F..................................F...........................................FFF..FFFF.FFFF..FFFF..FFFF.FF..FFFFFFFFFF...FF.................................................F..F..F.................FFFF.F.FFF..F.F.....................................FFF........................................................FF.FFFF.F...F.F...F.F

@jeremy
Copy link
Collaborator

jeremy commented Feb 9, 2013

Unpossible 😮

@javan
Copy link
Contributor Author

javan commented Feb 9, 2013

I pushed a possible alternative, 1130801

I stand by the first approach though. The comment with that optimization states "If there's no encoded-words in the string, just return it", but it doesn't work. It thinks '1+1=?' is an encoded string, and passes that string along to Encodings.collapse_adjacent_encodings which really expects an encoded string.

Let me know what you think. I can work up a new PR.

@jeremy
Copy link
Collaborator

jeremy commented Feb 10, 2013

Cool—pushed your first fix! 700ba21

@jeremy jeremy closed this Feb 10, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@javan @bpot @jeremy