Skip to content

Fix request body/POST access #5590

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 3 commits into from
Nov 15, 2017
Merged

Conversation

rpkilby
Copy link
Member

@rpkilby rpkilby commented Nov 13, 2017

Supersedes/closes #5583 and fixes #5582.

@rpkilby
Copy link
Member Author

rpkilby commented Nov 13, 2017

cc @D3X, @tomchristie.

Hi Tom, see my comment here for context. It seems like setting request._post in addition to request._files might have been the correct thing to do. Thoughts?

@D3X
Copy link
Contributor

D3X commented Nov 13, 2017

@rpkilby One thing I'm concerned about is that this changes the contract on request._post.

Normally, Django would set it to an empty QueryDict if the Content-Type of the request was not "application/x-www-form-urlencoded" or "multipart/form-data".

With this PR, it gets set to parsed data even for other types, like "application/json". This could result in not-easy-to-track bugs if some middlewares depended on Django's behaviour in this case.

@xordoquy
Copy link
Collaborator

This shouldn't impact middlewares as they are called with Django's request. DRF's views does the wrapping so it's really used "inside" DRF whether core or the application code.

@D3X
Copy link
Contributor

D3X commented Nov 14, 2017

@xordoquy I wrote a short test to show how the interface on request.POST changes when using DRF and posting JSON: D3X@aa6a4c4

I don't know any app that would break because of it, but it is possible some would.

@rpkilby rpkilby force-pushed the django-request-body branch from 7583b3a to feff4c2 Compare November 14, 2017 16:25
@rpkilby rpkilby force-pushed the django-request-body branch from feff4c2 to 278cb68 Compare November 15, 2017 01:13
@rpkilby
Copy link
Member Author

rpkilby commented Nov 15, 2017

@D3X - thanks, good catch.

@carltongibson
Copy link
Collaborator

@rpkilby This looks fine to me — I'm inclined to just pull it in, but I'm not quite seeing how the commits match the conversation. (Did you rebase? What changed?)

I wrote a short test to show how the interface on request.POST changes...

OK, added, but what was this line in the first version?

     self._request._post = self.POST

Thanks!

@carltongibson carltongibson added this to the v3.7.4 milestone Nov 15, 2017
@rpkilby
Copy link
Member Author

rpkilby commented Nov 15, 2017

@carltongibson - yeah, I amended the commits from earlier in the conversation. Previously was

     self._request._post = self._data
     self._request._files = self._files

@carltongibson
Copy link
Collaborator

@rpkilby OK, thanks. That makes perfect sense.

Copy link
Collaborator

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

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

OK, let’s have this. Thanks @D3X @rpkilby @xordoquy!

@carltongibson carltongibson merged commit 9f66e8b into encode:master Nov 15, 2017
@rpkilby rpkilby deleted the django-request-body branch November 15, 2017 19:59
@xordoquy
Copy link
Collaborator

I have no idea how this PR works.
_load_data_and_files call POST which may call _load_data_and_files.

@rpkilby
Copy link
Member Author

rpkilby commented Nov 15, 2017

@xordoquy - it's definitely weird, but it won't reenter. eg, if you call request.POST, it will trigger the call to _load_data_and_files(). At this point, _data/_files/_full_data are set, so the inner call to request.POST won't call _load_data_and_files() again.

pchiquet pushed a commit to pchiquet/django-rest-framework that referenced this pull request Nov 17, 2020
* Modernize middleware tests

* Added a failing test for encode#5582

* Set data ref on underlying django request
Techcable added a commit to Techcable/vmprof-server that referenced this pull request Apr 11, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DRF 3.6.4/3.7.0 breaks reading request.POST if request.body was read
5 participants