Repository: scrapy/scrapy

Issue:
Wrong charset handling in MailSender
Looks like passing `charset='utf-8'` makes a plain text message with `Content-Transfer-Encoding: base64` which then can't be read.

At https://github.com/scrapy/scrapy/blob/5a75b14a5fbbbd37c14aa7317761655ac7706b70/scrapy/mail.py#L81 `set_charset` is called but as the payload is not set yet, the underlying class just sets the headers. When later `set_payload` is called, it doesn't do any encoding, but the `Content-Transfer-Encoding` is already set. Looks like the fix should be passing the encoding to `set_payload` too, like was proposed in #3722 (and `set_charset` may be safe to be removed, not sure about this). Note that we have tests but they don't catch this.

Note also, that all of this seems to be compat code according to the Python docs.

Discussion:
wRAR: Can you please also add tests and/or modify the existing ones so that this change is tested properly?
mmitropoulou: Hi @wRAR. I changed the positioning of set_charset method for mails with attachments (it is now similar to how it as was at the begining) and removed it for mails without attachments (it is called anyway from the get_payload method if you pass a value for charset) because i feel like this is a better approach. I didn't create new tests or modify the tests a lot, because i feel that is not needed (everything eg. charset, body etc is already tested and I didnt add anything new that needs testing). I would like to hear your thoughts on it and get feedback. Thank you!
codecov[bot]: # [Codecov](https://codecov.io/gh/scrapy/scrapy/pull/5118?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scrapy) Report
> Merging [#5118](https://codecov.io/gh/scrapy/scrapy/pull/5118?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scrapy) (bec66a1) into [master](https://codecov.io/gh/scrapy/scrapy/commit/7095df1558897f081014a3baddf9ad911308be8e?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scrapy) (7095df1) will **decrease** coverage by `4.26%`.
> The diff coverage is `95.65%`.

> :exclamation: Current head bec66a1 differs from pull request most recent head b713a38. Consider uploading reports for the commit b713a38 to get more accurate results

```diff
@@            Coverage Diff             @@
##           master    #5118      +/-   ##
==========================================
- Coverage   88.20%   83.94%   -4.27%     
==========================================
  Files         162      162              
  Lines       10401    10492      +91     
  Branches     1510     1517       +7     
==========================================
- Hits         9174     8807     -367     
- Misses        952     1423     +471     
+ Partials      275      262      -13     
```

| [Impacted Files](https://codecov.io/gh/scrapy/scrapy/pull/5118?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scrapy) | Coverage Δ | |
|---|---|---|
| [scrapy/core/downloader/handlers/http2.py](https://codecov.io/gh/scrapy/scrapy/pull/5118/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scrapy#diff-c2NyYXB5L2NvcmUvZG93bmxvYWRlci9oYW5kbGVycy9odHRwMi5weQ==) | `43.42% <0.00%> (-56.58%)` | :arrow_down: |
| [scrapy/core/engine.py](https://codecov.io/gh/scrapy/scrapy/pull/5118/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scrapy#diff-c2NyYXB5L2NvcmUvZW5naW5lLnB5) | `83.96% <84.61%> (-0.23%)` | :arrow_down: |
| [scrapy/squeues.py](https://codecov.io/gh/scrapy/scrapy/pull/5118/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scrapy#diff-c2NyYXB5L3NxdWV1ZXMucHk=) | `95.34% <89.47%> (-1.43%)` | :arrow_down: |
| [scrapy/core/downloader/handlers/http11.py](https://codecov.io/gh/scrapy/scrapy/pull/5118/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scrapy#diff-c2NyYXB5L2NvcmUvZG93bmxvYWRlci9oYW5kbGVycy9odHRwMTEucHk=) | `94.11% <90.00%> (+0.11%)` | :arrow_up: |
| [scrapy/core/scheduler.py](https://codecov.io/gh/scrapy/scrapy/pull/5118/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scrapy#diff-c2NyYXB5L2NvcmUvc2NoZWR1bGVyLnB5) | `93.79% <95.23%> (+0.59%)` | :arrow_up: |
| [scrapy/core/downloader/webclient.py](https://codecov.io/gh/scrapy/scrapy/pull/5118/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scrapy#diff-c2NyYXB5L2NvcmUvZG93bmxvYWRlci93ZWJjbGllbnQucHk=) | `94.61% <100.00%> (+0.12%)` | :arrow_up: |
| [scrapy/downloadermiddlewares/retry.py](https://codecov.io/gh/scrapy/scrapy/pull/5118/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scrapy#diff-c2NyYXB5L2Rvd25sb2FkZXJtaWRkbGV3YXJlcy9yZXRyeS5weQ==) | `100.00% <100.00%> (ø)` | |
| [scrapy/http/request/\_\_init\_\_.py](https://codecov.io/gh/scrapy/scrapy/pull/5118/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scrapy#diff-c2NyYXB5L2h0dHAvcmVxdWVzdC9fX2luaXRfXy5weQ==) | `97.77% <100.00%> (+0.51%)` | :arrow_up: |
| [scrapy/http/request/form.py](https://codecov.io/gh/scrapy/scrapy/pull/5118/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scrapy#diff-c2NyYXB5L2h0dHAvcmVxdWVzdC9mb3JtLnB5) | `95.45% <100.00%> (+0.10%)` | :arrow_up: |
| [scrapy/http/request/json\_request.py](https://codecov.io/gh/scrapy/scrapy/pull/5118/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scrapy#diff-c2NyYXB5L2h0dHAvcmVxdWVzdC9qc29uX3JlcXVlc3QucHk=) | `94.87% <100.00%> (+0.75%)` | :arrow_up: |
| ... and [28 more](https://codecov.io/gh/scrapy/scrapy/pull/5118/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scrapy) | |

Gallaecio: > I didn't create new tests or modify the tests a lot, because i feel that is not needed (everything eg. charset, body etc is already tested and I didnt add anything new that needs testing).

If you fixed https://github.com/scrapy/scrapy/issues/5096, then that’s something new that needs to be tested. You should add/modify tests as needed so that, if you reverted your fix, tests would now break.

On a separate note, I wonder if `msg.set_charset(charset)` is needed at all after your change. When there are attachments, the message body is already added with a charset parameter specified. Does removing `msg.set_charset(charset)` make any difference provided you keep passing the charset when adding the payload text without attachments?

wRAR: > If you fixed #5096, then that’s something new that needs to be tested. You should add/modify tests as needed so that, if you reverted your fix, tests would now break.

The test change covers it, because of the added `decode=True`.

> I wonder if msg.set_charset(charset) is needed at all after your change. When there are attachments, the message body is already added with a charset parameter specified. Does removing msg.set_charset(charset) make any difference provided you keep passing the charset when adding the payload text without attachments?

Removing breaks the message, as it now lacks `Content-Type` and `Content-Transfer-Encoding` top-level headers.

Patch:
diff --git a/scrapy/mail.py b/scrapy/mail.py
index c11f3898d..237327451 100644
--- a/scrapy/mail.py
+++ b/scrapy/mail.py
@@ -96,10 +96,9 @@ class MailSender:
             rcpts.extend(cc)
             msg["Cc"] = COMMASPACE.join(cc)
 
-        if charset:
-            msg.set_charset(charset)
-
         if attachs:
+            if charset:
+                msg.set_charset(charset)
             msg.attach(MIMEText(body, "plain", charset or "us-ascii"))
             for attach_name, mimetype, f in attachs:
                 part = MIMEBase(*mimetype.split("/"))
@@ -110,7 +109,7 @@ class MailSender:
                 )
                 msg.attach(part)
         else:
-            msg.set_payload(body)
+            msg.set_payload(body, charset)
 
         if _callback:
             _callback(to=to, subject=subject, body=body, cc=cc, attach=attachs, msg=msg)


Test patch:
diff --git a/tests/test_mail.py b/tests/test_mail.py
index 504c78486..2535e58db 100644
--- a/tests/test_mail.py
+++ b/tests/test_mail.py
@@ -111,7 +111,7 @@ class MailSenderTest(unittest.TestCase):
 
         msg = self.catched_msg["msg"]
         self.assertEqual(msg["subject"], subject)
-        self.assertEqual(msg.get_payload(), body)
+        self.assertEqual(msg.get_payload(decode=True).decode("utf-8"), body)
         self.assertEqual(msg.get_charset(), Charset("utf-8"))
         self.assertEqual(msg.get("Content-Type"), 'text/plain; charset="utf-8"')