[Django] #29208: Mistake in the documentation, request.POST['username'] is not working, but request.POST.get('username') is woring!
Django
request.POST.get('username') is woring!
--------------------------------------------+------------------------
Reporter: Marat Mkhitaryan | Owner: nobody
Type: Bug | Status: new
Component: Documentation | Version: 2.0
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
--------------------------------------------+------------------------
https://docs.djangoproject.com/en/2.0/topics/auth/default/#django.contrib.auth.login
{{{
username = request.POST['username']
password = request.POST['password']
# MultiValueDictKeyError
username = request.POST.get('username')
password = request.POST.get('password')
# No errors
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29208>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
----------------------------------+--------------------------------------
Reporter: Marat Mkhitaryan | Owner: nobody
Type: Bug | Status: new
Component: Documentation | Version: 2.0
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--
Ticket URL: <https://code.djangoproject.com/ticket/29208#comment:1>
Django
----------------------------------+--------------------------------------
Reporter: Marat Mkhitaryan | Owner: nobody
Component: Documentation | Version: 2.0
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Claude Paroz):
* status: new => closed
* resolution: => wontfix
Comment:
I wouldn't say this is a bug. The example assumes that request.POST
contains `username` and `password`. If you use that in a context where it
might not be the case, then yes, you should defensively use `.get()`.
That's pure standard Python behaviour.
--
Ticket URL: <https://code.djangoproject.com/ticket/29208#comment:2>
Django
----------------------------------+--------------------------------------
Reporter: Marat Mkhitaryan | Owner: nobody
Component: Documentation | Version: 2.0
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mogoh Viol):
I stumbled into this just this week and I say this is still a problem and
bad style.
The reason, why this is a problem, because it depends on the POST request
the user sends.
''Normally'', what the user sends is depending on the form provide by the
server.
But it is trivially to craft a POST request that does not contain a
username or a password or whatever.
A malicious attacker could just provoke a application crash and an
internal server error (which happened to me).
This in itself is of course not a security breach but an attacker should
never be able to provoke a crash like this.
In my case this curl commands provoked the crash:
This one is for obtaining csfr cookies and tokens:
{{{
curl -sS --location --cookie-jar cookies.txt
http://localhost:8080/en/intern/login/ | grep 'csrfmiddlewaretoken'
}}}
Here we send the POST request with omiting the password:
{{{
curl -X POST --data "csrfmiddlewaretoken=<----------TOKEN
HERE----------->&username=x" --cookie cookies.txt -sS --location --dump-
header - http://localhost:8080/en/intern/login/ -o /dev/null
}}}
And the fix as Marat Mkhitaryan suggestes is as trivial as the attack.
So please let's change this.
--
Ticket URL: <https://code.djangoproject.com/ticket/29208#comment:3>
Django
----------------------------------+--------------------------------------
Reporter: Marat Mkhitaryan | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mogoh Viol):
* status: closed => new
* version: 2.0 => 4.0
* resolution: wontfix =>
--
Ticket URL: <https://code.djangoproject.com/ticket/29208#comment:4>
Django
----------------------------------+--------------------------------------
Reporter: Marat Mkhitaryan | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mogoh Viol):
* cc: Mogoh Viol (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/29208#comment:5>
Django
----------------------------------+--------------------------------------
Reporter: Marat Mkhitaryan | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham):
Maybe it's unclear that this code is for example purposes only and isn't
meant to be copied into your project. This is an example of how to use the
low-level `login()` function. It's not meant as a robust
[https://docs.djangoproject.com/en/stable/topics/auth/default/#django.contrib.auth.views.LoginView
LoginView].
--
Ticket URL: <https://code.djangoproject.com/ticket/29208#comment:6>
Django
----------------------------------+--------------------------------------
Reporter: Marat Mkhitaryan | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mogoh Viol):
Replying to [comment:6 Tim Graham]:
> Maybe it's unclear that this code is for example purposes only and isn't
meant to be copied into your project.
That is the least that should be changed.
But why don't just change it to a production ready working example?
I see no downsides in changing it.
And even if we decide to not make the example production ready, I think
obtaining fields from a PUSH request should always check for invalid
fields.
The documentation should teach safe code.
--
Ticket URL: <https://code.djangoproject.com/ticket/29208#comment:7>
Django
----------------------------------+--------------------------------------
Reporter: Marat Mkhitaryan | Owner: nobody
Component: Documentation | Version: 4.0
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* status: new => closed
* resolution: => wontfix
Comment:
There are other examples that use indexing of `request.POST`. Like
Carlton, I don't see much advantage to making this change throughout all
the documentation. You can write to the DevelopersMailingList if you wish
to seek other opinions.
--
Ticket URL: <https://code.djangoproject.com/ticket/29208#comment:8>
Django
----------------------------------+--------------------------------------
Reporter: Marat Mkhitaryan | Owner: nobody
Component: Documentation | Version: 4.0
Severity: Normal | Resolution: wontfix
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
I don't recall commenting here, but I do agree. The example is
pedagogical… — it shows using `login()` -- making it production worthy
would obscure the point trying to be made.
Claude's comment:2 seems right:
> If you use that in a context where it might not be the case, then yes,
you should defensively use .get(). That's pure standard Python behaviour.
--
Ticket URL: <https://code.djangoproject.com/ticket/29208#comment:9>