search

pazz / alot

Terminal-based Mail User Agent
GNU General Public License v3.0
700 stars 163 forks source link

envelope: set policy when constructing email #1485

Closed stregouet closed 4 years ago

stregouet commented 4 years ago

default policy ensure headers are correctly encoded (i.e. will not leak non-ascii char in headers)

(related to #1482)

pacien commented 4 years ago

This possibly solves #1434 and #1484 in addition to #1482.

stregouet commented 4 years ago

@pazz do you know why travis is not reporting status/running build ?

pazz commented 4 years ago

@pazz do you know why travis is not reporting status/running build ?

Travis integration in github is a troublemaker at the moment. I believe its due to github restructuring "apps". I don't really have the time to look into it and am thinking of removing it alltogether.

pazz commented 4 years ago

I've set up travis from scratch and it seems to run now. There are failing tests though..

pazz commented 4 years ago

Ah, they fail only on python <3.8 which makes sense. In fact, perhaps we should find a way to make this work on python 3.6 and 3.7.. https://travis-ci.com/github/pazz/alot/builds/158944184

stregouet commented 4 years ago

cool! thanks for your time pazz. I will try to find a solution for python 3.6 and 3.7

stregouet commented 4 years ago

hi, sorry for delayed response… I cannot find travis-ci log anymore, probably because I'm too late… Anyway I run test in with maxking/mailman-ci-runner docker image (used in mailman project). But I only get failure with python3.6 (not python3.7 as suggested in previous comment)

I finally find where is difference with python3.8: it's in function email._header_value_parser._refold_parse_tree at the beginning maxlen variable default to sys.maxsize in python3.7 and above but in python3.6 it is float("+inf") which break arithmetic and string slicing.

I can see 2 options to fix this:

  1. monkeypatching _refold_parse_tree in python3.6 but this function is quite big…
  2. use optionnal parameter maxheaderlen in as_string method and force it to sys.maxsize

what do you think? do you see another option? (not to forget initial goal of this PR: fixing encoding of headers when sending mail)

stregouet commented 4 years ago

@pazz did you have time to think about my proposal? (not really sure if I need to ping you, I'm not used to github…)

pazz commented 4 years ago

Sorry, not yet. I will look into this at the end of the week for sure though.

pazz commented 4 years ago

Could you rebase this to master? It seems that travis is choking on some sphinx-versioning stuff that I remember having fixed some time ago.

Also, the following test failure is reported by travis (now, after triggering it again) for python 3.7:

======================================================================
1318ERROR: test_construct_encoding (tests.db.test_envelope.TestEnvelope)
1319----------------------------------------------------------------------
1320Traceback (most recent call last):
1321  File "/opt/python/3.7.1/lib/python3.7/unittest/mock.py", line 1191, in patched
1322    return func(*args, **keywargs)
1323  File "/home/travis/build/pazz/alot/tests/db/test_envelope.py", line 127, in test_construct_encoding
1324    raw = mail.as_string(policy=email.policy.SMTP)
1325  File "/opt/python/3.7.1/lib/python3.7/email/message.py", line 158, in as_string
1326    g.flatten(self, unixfrom=unixfrom)
1327  File "/opt/python/3.7.1/lib/python3.7/email/generator.py", line 116, in flatten
1328    self._write(msg)
1329  File "/opt/python/3.7.1/lib/python3.7/email/generator.py", line 195, in _write
1330    self._write_headers(msg)
1331  File "/opt/python/3.7.1/lib/python3.7/email/generator.py", line 222, in _write_headers
1332    self.write(self.policy.fold(h, v))
1333  File "/opt/python/3.7.1/lib/python3.7/email/policy.py", line 183, in fold
1334    return self._fold(name, value, refold_binary=True)
1335  File "/opt/python/3.7.1/lib/python3.7/email/policy.py", line 205, in _fold
1336    return value.fold(policy=self)
1337  File "/opt/python/3.7.1/lib/python3.7/email/headerregistry.py", line 258, in fold
1338    return header.fold(policy=policy)
1339  File "/opt/python/3.7.1/lib/python3.7/email/_header_value_parser.py", line 144, in fold
1340    return _refold_parse_tree(self, policy=policy)
1341  File "/opt/python/3.7.1/lib/python3.7/email/_header_value_parser.py", line 2650, in _refold_parse_tree
1342    part.ew_combine_allowed, charset)
1343  File "/opt/python/3.7.1/lib/python3.7/email/_header_value_parser.py", line 2727, in _fold_as_ew
1344    first_part = to_encode[:text_space]
1345TypeError: slice indices must be integers or None or have an __index__ method
stregouet commented 4 years ago

just rebased on master

I think I got why travis is failing on 3.7 but test on my laptop is not! from traceback above I guess, travis is running 3.7.1, but my docker is using 3.7.7. Do you know why travis is not using last 3.7 release?

(_header_value_parser.py has changed between version 3.7.1 and version 3.7.7)

pazz commented 4 years ago

Quoting stregouet (2020-05-06 14:20:33)

just rebased on master

I think I got why travis is failing on 3.7 but test on my laptop is not! from traceback above I guess, travis is running 3.7.1, but my docker is using 3.7.7. Do you know why travis is not using last 3.7 release?

(_header_value_parser.py has changed between version 3.7.1 and version 3.7.7)

OK, good do know. No, I'm not sure why travis uses the versions it does. But it is not really an issue for us because we should really support most recent versions. Given that for most distributions it is not too long ago that they switched away from python 2.7 as a default, I believe dropping support for 3.6 may be a bit premature. (or is there a good reason for it except that it makes our life a little harder?)

pazz commented 4 years ago

I wonder if it is totally unreasonable to drop support for old python versions: By old I mean those that are not in use as default in current Fedora (stable is 32 and has python 3.8.2-2) and Debian (stable is buster, has python 3.7.3-1) and by extension everything based on it.

stregouet commented 4 years ago

it seems reasonable! unfortunately there is still this "bug" with float("+inf") in python 3.7.3 for _header_value_parser.py… We need python3.7.4 _header_value_parser.py

So my test is still failing with python3.7.3

pazz commented 4 years ago

Sorry, just to clarify: Are you saying that you are working on this or that you want to sit it out and re-submit this PR once we drop support for python 3.7.3 and lower?

Quoting stregouet (2020-05-09 13:52:22)

it seems reasonable! unfortunately there is still this "bug" with float("+inf") in python 3.7.3 for [1]_header_value_parser.py…

So my test is still failing with python3.7.3

— You are receiving this because you were mentioned. Reply to this email directly, [2]view it on GitHub, or [3]unsubscribe.

References

Visible links

  1. https://github.com/python/cpython/blob/v3.7.3/Lib/email/_header_value_parser.py#L2594
  2. https://github.com/pazz/alot/pull/1485#issuecomment-626171632
  3. https://github.com/notifications/unsubscribe-auth/AAEZNZYP6JDLGCNHVDCEA73RQVGYNANCNFSM4L226DFQ
stregouet commented 4 years ago

I just want to help fixing issue with header encoding ;)

Suggested solution in this PR is facing issue with python <= 3.7.3 so we need to either drop support for older version or find a workaround

As you wrote python 3.7.3 is default version for debian stable so it seems not reasonnable to drop support for it. I've made a suggestion for a workaround, just waiting for your opinion on this ;)

pazz commented 4 years ago

Right, thanks. So if I understand your two suggestions correctly then number two should be the way to go: if indeed email.as_string accepts some parameter that we can set and thereby make it work for all python versions then this sounds like the cheapest, and still correct, option.

stregouet commented 4 years ago

ok, thanks for your reply

just pushed another commit, tell me if it seems enough, especially if explanation in commit message is clear

pazz commented 4 years ago

This possibly solves #1434 and #1484 in addition to #1482.

@pacien can you comment on whether this indeed fixed your earlier problem #1434 ?

pacien commented 4 years ago

I'm not sure of what changed but I'm afraid I could not reproduce my issue (#1434) in 0.9 with or without this patch.

pazz commented 4 years ago

OK thanks @pacien. this at least meanst that things did not get worse :)

stregouet commented 4 years ago

cool! again thanks for your time :) @pazz do you have any idea when this change will be released?

pazz commented 4 years ago

Quoting stregouet (2020-05-11 12:46:26)

cool! again thanks for your time :)

Thanks to you for pushing this issue (and project)!

[1]@pazz do you have any idea when this change will be released?

It is already part of the master branch. I plan to make a new release soonish as some people were pushing for it (#1500)..

Ornanovitch commented 4 years ago

Hi,

I just installed alot few days ago and am configuring it. Manjaro should provides 0.9.1 in a few days.

When I reply to messages, my from is received like this by the recipient : =?utf-8?b?SsOpcsO0bWUgVmlhbCA8cHJvZEBkb3plY29tcGFnbmllLmZyPg==?=

Does this fix has a chance to resolve this or should I open a new issue ?

Ornanovitch commented 4 years ago

Hi,

I just installed alot few days ago and am configuring it. Manjaro should provides 0.9.1 in a few days.

When I reply to messages, my from is received like this by the recipient : =?utf-8?b?SsOpcsO0bWUgVmlhbCA8cHJvZEBkb3plY29tcGFnbmllLmZyPg==?=

Does this fix has a chance to resolve this or should I open a new issue ?

Upgraded this morning, I confirm it fixes everything !