Opened 8 years ago

Closed 16 months ago

#7060 closed Cleanup/optimization (fixed)

Fix race condition in tutorial vote() view

Reported by: donald.ball@… Owned by: Raphael Michel
Component: Documentation Version: master
Severity: Normal Keywords:
Cc: donald.ball@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

In the portion of the tutorial where you introduce the template system, you don't make any mention of how to escape output to avoid XSS and similar attacks. Later, when you show code to increment the vote count, you have a classic race condition.

Both of these potential bugs could be considered advanced topics, not suitable for an introductory tutorial, but I am of the view that you should show folks the correct way to do things from the get-to.

Change History (14)

comment:1 Changed 8 years ago by anonymous

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

1: All output is HTML escaped by default, so XSS is not a worry unless people explicitly mark data as safe.

2: Way more details on the race condition please.

comment:2 in reply to:  1 Changed 8 years ago by anonymous

Replying to anonymous:

1: All output is HTML escaped by default, so XSS is not a worry unless people explicitly mark data as safe.

2: Way more details on the race condition please.

Forgive me, the auto-escaping is introduced in the SVN version.

comment:3 Changed 8 years ago by Donald Ball <donald.ball@…>

On the race condition:

def vote(request, poll_id):
    p = get_object_or_404(Poll, pk=poll_id)
    try:
        selected_choice = p.choice_set.get(pk=request.POST['choice'])
    except (KeyError, Choice.DoesNotExist):
        # Redisplay the poll voting form.
        return render_to_response('polls/detail.html', {
            'poll': p,
            'error_message': "You didn't select a choice.",
        })
    else:
        selected_choice.votes += 1
        selected_choice.save()
        # Always return an HttpResponseRedirect after successfully dealing
        # with POST data. This prevents data from being posted twice if a
        # user hits the Back button.
        return HttpResponseRedirect('/polls/%s/results/' % p.id)

Two requests hit this section of code at the same time. When the race condition occurs, one request gets the number of votes as X, then the second (again, as X), then the first sets the number of votes to X+1, then the second does the same, so one vote is lost. Presumably, one would protect the critical section using a transaction.

Good to know output escaping is the default in the latest version.

comment:4 Changed 8 years ago by Ian Kelly

Race conditions like that are best solved at the database level by raising the transaction isolation level. Ticket #6064 would provide a relatively straightforward way to do this, but it hasn't yet been committed. I don't know if that issue should be brought up in the tutorial until it has.

comment:5 Changed 8 years ago by Simon Greenhill

Summary: Tutorial skips over two bugs: output escaping and a race conditionTutorial skips over a race condition
Triage Stage: UnreviewedAccepted
Version: 0.96SVN

Changing the ticket to focus on the race condition, as the autoescape seems to be fixed.

comment:6 Changed 8 years ago by James Bennett

Resolution: invalid
Status: newclosed

Since the focus of the tutorial is to introduce the basics of Django's components and how they work, rather than to exhaustively document how to write web applications, this feels out of scope.

comment:7 Changed 17 months ago by Jim Garrison

Easy pickings: unset
Resolution: invalid
Severity: Normal
Status: closednew
Type: Uncategorized
UI/UX: unset

I am re-opening this, as I think the decision here ought to be revisited. It reflects poorly on Django for the tutorial to teach poor coding practices, particularly when there is a race condition in a vote-tallying app. More and more, such race conditions are being treated as true security vulnerabilities.

Following discussion on reddit, I suggest putting a note in the tutorial mentioning that the code indeed has a race condition, and that more advanced users should consider Django's F-expressions instead.

comment:8 Changed 17 months ago by Tim Graham

Easy pickings: set
Summary: Tutorial skips over a race conditionFix race condition in tutorial vote() view
Type: UncategorizedCleanup/optimization

I think changing the vote() view to use F expressions is a good idea. We could link to docs/ref/models/expressions/#avoiding-race-conditions-using-f for a more complete explanation.

comment:9 Changed 17 months ago by Baptiste Mispelon

Personally I think adding F expressions at this point in the tutorial is premature and too complicated.

The vote view has no user authenticated and no rate-limiting so adding a protection against race conditions seems pointless to me.

I'm not sure the argument about better coding practices applies here either because if that's what we were going for, we'd be using a Form instead of the current ad-hoc handling.

However I would be OK with a paragraph mentionning the limitations of the code and linking to things like forms, F objects, ...

comment:10 Changed 17 months ago by Jim Garrison

The vote view has no user authenticated and no rate-limiting so adding a protection against race conditions seems pointless to me.

I think it is reasonable to expect people going through the tutorial to recognize that the voting app has no rate-limiting or user authentication. On the other hand, I do not think it is reasonable to expect a developer new to Django to recognize that there is a race condition in the vote tallying code. Since it involves the ORM layer (which is largely a black box at this point in the tutorial), new developers might even get the impression that the lines selected_choice.votes += 1 and selected_choice.save() lead to an atomic operation being performed on the database, and are therefore the preferred way of doing such operations. With a more complex ORM, this might be a reasonable assumption, but it is not the way Django works.

In any event, it seems that we each agree that a note should be added to the tutorial (at a minimum), so there is probably no reason to belabor these points further.

comment:11 Changed 16 months ago by Raphael Michel

Owner: changed from nobody to Raphael Michel
Status: newassigned

comment:12 Changed 16 months ago by Raphael Michel

https://github.com/django/django/pull/4756 is a proposal for a note in the tutorial. Feel free to suggest any changes.

comment:13 Changed 16 months ago by Raphael Michel

Has patch: set

comment:14 Changed 16 months ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In 3e9b5bfd:

Fixed #7060 -- Added a note about race conditions to the tutorial

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