Fix dispatching of webhooks with non-ASCII content by sending only binary strings
Review Request #8560 — Created Dec. 4, 2016 and submitted
When dispatching a webhook that contained non-ASCII content, the urlopen call raised an exception
that reported an encoding error, complaining that some characters are not encodable with theascii
encoding.This issue occurs because the
urlopenimplementation concatenates the given URL, headers, and body
using string concatenation, and, if the resulting string is a unicode string, encodes it implicitly
as ASCII before sending it to the server.This change encodes all data passed to
urlopenas binary strings, so that the result of string
concatenation is also a binary string, thus necessitating no implicit encoding. While writing tests
for this issue, an issue with silently swallowed assertions in the test_urlopenwas fixed.
Added 4 unit tests for non-ASCII custom, JSON, XML and Form-Data content.
Checked locally that payloads are sent without issue.
Added check that all data sent to_test_dispatchis made of binary strings.
| Description | From | Last Updated |
|---|---|---|
|
Col: 17 E131 continuation line unaligned for hanging indent |
 reviewbot
|
|
|
Col: 80 E501 line too long (90 > 79 characters) |
reviewbot
|
|
|
Col: 80 E501 line too long (82 > 79 characters) |
reviewbot
|
|
|
Can we switch these lines (to keep it in alphabetical order)? |
 david
|
|
|
I know the other methods in here have "Test ..." but our general convention for unit test docstrings is "Testing … |
david
|
|
|
Same here. |
david
|
|
|
Same here. |
david
|
|
|
Same here. |
david
|
|
|
Please add a blank line between these two. |
david
|
|
|
Please add a blank line between these two. |
david
|
|
|
Should have two blank lines between top-level classes. |
david
|
|
|
We should get six from django.utils, rather than the standalone package. |
david
|
|
|
Please add a blank line between these two. |
david
|
|
|
This kind of comment (narrating the code) doesn't really help too much with understanding the code. The call to .encode() … |
david
|
|
|
Again, this comment is kind of narrative instead of explanatory. Perhaps an even better solution than doing this test would … |
david
|
|
|
Instead of having this comment, can we have a unit test that calls this while spying on urlopen/Request and verifies … |
david
|
|
|
Again, this comment doesn't really add to the understanding of the code. |
david
|
|
|
'six' imported but unused |
reviewbot
|
- Change Summary:
-
Fix line length issues from ReviewBot
- Commit:
-
5ac0239ab4e7020dfc724c61734c194860c7b67e8f0590fb2b2765636bf46149673624ff46a4fd0c
-
Tool: Pyflakes Processed Files: reviewboard/notifications/webhooks.py reviewboard/notifications/tests.py Tool: PEP8 Style Checker Processed Files: reviewboard/notifications/webhooks.py reviewboard/notifications/tests.py
- Change Summary:
-
Fix import and formatting issues from David Trowbridge's review
- Description:
-
Commits:
- Avoid silent assertion failures in
WebHookDispatchTests._test_dispatch
- Send only binary strings in
urlopenwhen dispatching webhooks- Avoid calling
.encodewhen content is already encoded to a binary string - Call
.encodeafter custom payload rendering - Encode all headers and URL to binary strings
- Avoid calling
+ - Fix import and formatting issues from David Trowbridge's review
- Avoid silent assertion failures in
- Commit:
-
8f0590fb2b2765636bf46149673624ff46a4fd0c6e1ed431158ae2eb594a3dcd7502306c41a4a988
-
Tool: Pyflakes Processed Files: reviewboard/notifications/webhooks.py reviewboard/notifications/tests.py Tool: PEP8 Style Checker Processed Files: reviewboard/notifications/webhooks.py reviewboard/notifications/tests.py
-
-
This kind of comment (narrating the code) doesn't really help too much with understanding the code. The call to
.encode()is pretty self-explanatory. -
Again, this comment is kind of narrative instead of explanatory. Perhaps an even better solution than doing this test would be to put
bytes = bytes.encode('utf-8')into each of the cases above where it's needed. -
Instead of having this comment, can we have a unit test that calls this while spying on
urlopen/Requestand verifies that all headers are bytes? That way we won't accidentally regress this. -
- Change Summary:
-
Remove added comments and move
encode('utf-8')calls inwebhooks.py - Description:
-
Commits:
- Avoid silent assertion failures in
WebHookDispatchTests._test_dispatch
- Send only binary strings in
urlopenwhen dispatching webhooks- Avoid calling
.encodewhen content is already encoded to a binary string - Call
.encodeafter custom payload rendering - Encode all headers and URL to binary strings
- Avoid calling
- Fix import and formatting issues from David Trowbridge's review
+ - Remove comments from
webhooks.py
+ - Move
encode('utf-8')calls to only necessary places.
- Avoid silent assertion failures in
- Commit:
-
6e1ed431158ae2eb594a3dcd7502306c41a4a98844a6ce4fbc2cb8e618cdc35b3600719e1ef520f2
- Change Summary:
-
Remove unnecessary import of
six. - Description:
-
Commits:
- Avoid silent assertion failures in
WebHookDispatchTests._test_dispatch
- Send only binary strings in
urlopenwhen dispatching webhooks- Avoid calling
.encodewhen content is already encoded to a binary string - Call
.encodeafter custom payload rendering - Encode all headers and URL to binary strings
- Avoid calling
- Fix import and formatting issues from David Trowbridge's review
- Remove comments from
webhooks.py
- Move
encode('utf-8')calls to only necessary places.
+ - Remove unnecessary import of
six.
- Avoid silent assertion failures in
- Commit:
-
44a6ce4fbc2cb8e618cdc35b3600719e1ef520f2f0266950237c179fa3118112d1bdd117f07e5005
-
Tool: Pyflakes Processed Files: reviewboard/notifications/webhooks.py reviewboard/notifications/tests.py Tool: PEP8 Style Checker Processed Files: reviewboard/notifications/webhooks.py reviewboard/notifications/tests.py
-
We have a specific format we like for the review request descriptions, which are used for the commit message. We like to clearly communicate what the problem was before and how it was fixed. Would you mind updating the review request for that? We have details and examples up at https://www.notion.so/reviewboard/Writing-Good-Change-Descriptions-10529e7c207743fa8ca90153d4b21fea
- Summary:
-
Send only binary strings in urlopen when dispatching webhooksFix dispatching of webhooks with non-ASCII content by sending only binary strings
- Description:
-
~ Commits:
~ When dispatching a webhook that contained non-ASCII content, the urlopen call raised an exception
+ that reported an encoding error, complaining that some characters are not encodable with the ascii+ encoding. ~ - Avoid silent assertion failures in
WebHookDispatchTests._test_dispatch
~ - Send only binary strings in
urlopenwhen dispatching webhooks- Avoid calling
.encodewhen content is already encoded to a binary string - Call
.encodeafter custom payload rendering - Encode all headers and URL to binary strings
- Avoid calling
~ - Fix import and formatting issues from David Trowbridge's review
~ - Remove comments from
webhooks.py
~ - Move
encode('utf-8')calls to only necessary places.
~ - Remove unnecessary import of
six.
~ This issue occurs because the
urlopenimplementation concatenates the given URL, headers, and body~ using string concatenation, and, if the resulting string is a unicode string, encodes it implicitly ~ as ASCII before sending it to the server. ~ ~ This change encodes all data passed to
urlopenas binary strings, so that the result of string~ concatenation is also a binary string, thus necessitating no implicit encoding. While writing tests + for this issue, an issue with silently swallowed assertions in the test _urlopenwas fixed. - Avoid silent assertion failures in
Tool: Pyflakes Processed Files: reviewboard/notifications/webhooks.py reviewboard/notifications/tests.py Tool: PEP8 Style Checker Processed Files: reviewboard/notifications/webhooks.py reviewboard/notifications/tests.py