Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Enforced data size in channels 2.1.7 prevent large file uploads #1242

Closed
wants to merge 3 commits into from
Closed

Enforced data size in channels 2.1.7 prevent large file uploads #1242

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
60d93ed
Test case for #1240
EliotBerriot Feb 7, 2019
3e448c2
Fix #1240: ignore files when checking request size for multipart requ…
EliotBerriot Feb 7, 2019
43aa241
Factorized boundary / multipart asgi request
EliotBerriot Feb 8, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion channels/http.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,8 +120,11 @@ def __init__(self, scope, body):
# TODO: chunked bodies

# Limit the maximum request data size that will be handled in-memory.
# but only for non-multipart requests, because files are handled
# differently by django, see #1240
if (
settings.DATA_UPLOAD_MAX_MEMORY_SIZE is not None
self.content_type != "multipart/form-data"
and settings.DATA_UPLOAD_MAX_MEMORY_SIZE is not None
agateblue marked this conversation as resolved.
Show resolved Hide resolved
and self._content_length > settings.DATA_UPLOAD_MAX_MEMORY_SIZE
):
raise RequestDataTooBig(
Expand Down
66 changes: 51 additions & 15 deletions tests/test_http.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,24 @@
from channels.sessions import SessionMiddlewareStack
from channels.testing import HttpCommunicator

BOUNDARY_WORD = b"BOUNDARY"
BOUNDARY = b"--" + BOUNDARY_WORD + b"\r\n"
END_BOUNDARY = b"--" + BOUNDARY_WORD + b"--"


def MultipartAsgiRequest(body, **kwargs):
data = {
"http_version": "1.1",
"method": "POST",
"path": "/test/",
"headers": {
"content-type": b"multipart/form-data; boundary=" + BOUNDARY_WORD,
"content-length": str(len(body)).encode("ascii"),
},
}
data.update(kwargs)
return AsgiRequest(data, body)


class RequestTests(unittest.TestCase):
"""
Expand Down Expand Up @@ -112,26 +130,15 @@ def test_post_files(self):
Tests POSTing files using multipart form data.
"""
body = (
b"--BOUNDARY\r\n"
BOUNDARY
+ b'Content-Disposition: form-data; name="title"\r\n\r\n'
+ b"My First Book\r\n"
+ b"--BOUNDARY\r\n"
+ BOUNDARY
+ b'Content-Disposition: form-data; name="pdf"; filename="book.pdf"\r\n\r\n'
+ b"FAKEPDFBYTESGOHERE"
+ b"--BOUNDARY--"
)
request = AsgiRequest(
{
"http_version": "1.1",
"method": "POST",
"path": "/test/",
"headers": {
"content-type": b"multipart/form-data; boundary=BOUNDARY",
"content-length": str(len(body)).encode("ascii"),
},
},
body,
+ END_BOUNDARY
)
request = MultipartAsgiRequest(body)
self.assertEqual(request.method, "POST")
self.assertEqual(len(request.body), len(body))
self.assertTrue(request.META["CONTENT_TYPE"].startswith("multipart/form-data"))
Expand Down Expand Up @@ -182,6 +189,35 @@ def test_size_exceeded(self):
b"",
)

def test_size_check_ignore_files(self):
body = (
BOUNDARY
+ b'Content-Disposition: form-data; name="title"\r\n\r\n'
+ b"My First Book\r\n"
+ BOUNDARY
+ b'Content-Disposition: form-data; name="pdf"; filename="book.pdf"\r\n\r\n'
+ b"FAKEPDFBYTESGOHERETHISISREALLYLONGBUTNOTUSEDTOCOMPUTETHESIZEOFTHEREQUEST"
+ END_BOUNDARY
)
request = MultipartAsgiRequest(body)
with override_settings(DATA_UPLOAD_MAX_MEMORY_SIZE=60):
request.POST

def test_size_check_ignore_files_but_honor_other_post_data(self):
body = (
BOUNDARY
+ b'Content-Disposition: form-data; name="title"\r\n\r\n'
+ b"My First Book\r\n"
+ BOUNDARY
+ b'Content-Disposition: form-data; name="pdf"; filename="book.pdf"\r\n\r\n'
+ b"FAKEPDFBYTESGOHERETHISISREALLYLONGBUTNOTUSEDTOCOMPUTETHESIZEOFTHEREQUEST"
+ END_BOUNDARY
)
agateblue marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if body is the same as above, would it work to define a module or class level variable ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jpic I can, but my own experience is that we usually don't need the same level of factorization in tests and in runtime code, and I tend to avoid shared state between tests.

Tests should be readable, and independant. If we try to factorize everything, it makes it really harder to understand and maintain tests. Quick example: it took me literally 30 seconds to write those tests, because I could read above tests and copy-paste the code, and edit it as needed.

It would have take me way more time if I had to jump between 5 constants, understand if each matched my needs. Then figure if I should create a new one for my test (which may be slightly different), or edit the one in place (which could break other tests). What if someone edit that contstant in the future? They would need to understand their tests, and mine, to avoid breaking anything.

I like that you can screenshot/copy-paste a test and understand what's going on. It's far more important than factorization IMHO, which quickly become a nightmare with huge codebases and 1000s of tests.

That being said, I'm not maintaining this project, and I'll happily move the whole body in a constant if you think it's the best thing :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I can live with anything, it was just my advice because I like to refactor tests after writing them, let's see what others think about it ;)

Grats for your contribution and have a great day !

request = MultipartAsgiRequest(body)
with override_settings(DATA_UPLOAD_MAX_MEMORY_SIZE=1):
with pytest.raises(RequestDataTooBig):
request.POST
agateblue marked this conversation as resolved.
Show resolved Hide resolved


### Handler tests

Expand Down