Opened 4 years ago

Closed 6 months ago

Last modified 6 months ago

#29208 closed Bug (wontfix)

Mistake in the documentation, request.POST['username'] is not working, but request.POST.get('username') is working!

Reported by: Marat Mkhitaryan Owned by: nobody
Component: Documentation Version: 4.0
Severity: Normal Keywords:
Cc: Marat, Mkhitaryan, Mogoh Viol Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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

Change History (9)

comment:1 Changed 4 years ago by Marat Mkhitaryan

Summary: Mistake in the documentation, request.POST['username'] is not working, but request.POST.get('username') is woring!Mistake in the documentation, request.POST['username'] is not working, but request.POST.get('username') is working!

comment:2 Changed 4 years ago by Claude Paroz

Resolution: wontfix
Status: newclosed

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.

comment:3 Changed 6 months ago 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, is because it depends on the POST request that 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.

Last edited 6 months ago by Mogoh Viol (previous) (diff)

comment:4 Changed 6 months ago by Mogoh Viol

Resolution: wontfix
Status: closednew
Version: 2.04.0

comment:5 Changed 6 months ago by Mogoh Viol

Cc: Mogoh Viol added

comment:6 Changed 6 months ago 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 LoginView.

comment:7 in reply to:  6 Changed 6 months ago by Mogoh Viol

Replying to 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.

comment:8 Changed 6 months ago by Tim Graham

Resolution: wontfix
Status: newclosed

There are other examples that use indexing of request.POST. Like Claude, 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.

Last edited 6 months ago by Tim Graham (previous) (diff)

comment:9 Changed 6 months ago 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.

Note: See TracTickets for help on using tickets.
Back to Top