Opened 12 years ago

Last modified 12 years ago

#335 closed defect (FIXED)

Exception: KeyError: CSRF_TOKEN

Reported by: joar Owned by: Elrond
Priority: major Milestone: 0.2.0
Component: programming Keywords:
Cc: Parent Tickets:

Description

Trying to view a video breaks stuff.

BROKEN (videos)

`http://mg.wandborg.se/u/joar/m/fluid-box-colourful/ <http://mg.wandborg.se/u/joar/m/fluid-box-colourful/>`_
`http://mg.wandborg.se/u/joar/m/new-deploy-test/ <http://mg.wandborg.se/u/joar/m/new-deploy-test/>`_
`http://mg.wandborg.se/u/joar/m/sintel/ <http://mg.wandborg.se/u/joar/m/sintel/>`_

WORKING (images)

`http://mg.wandborg.se/u/joar/m/photo-maj-09-8-06-07-em/ <http://mg.wandborg.se/u/joar/m/photo-maj-09-8-06-07-em/>`_
`http://mg.wandborg.se/u/joar/m/vasaparken/ <http://mg.wandborg.se/u/joar/m/vasaparken/>`_
`http://mg.wandborg.se/u/joar/m/4ec9534946102a5b6f00002c-tove/ <http://mg.wandborg.se/u/joar/m/4ec9534946102a5b6f00002c-tove/>`_

BACKTRACE

::

    File '/srv/mg.wandborg.se/mediagoblin/lib/python2.7/site-packages/Paste-1.7.5.1-py2.7.egg/paste/exceptions/errormiddleware.py', line 144 in __call__
      app_iter = self.application(environ, sr_checker)
    File '/srv/mg.wandborg.se/mediagoblin/lib/python2.7/site-packages/Paste-1.7.5.1-py2.7.egg/paste/urlmap.py', line 203 in __call__
      return app(environ, start_response)
    File '/srv/mg.wandborg.se/mediagoblin/lib/python2.7/site-packages/Beaker-1.6.1-py2.7.egg/beaker/middleware.py', line 155 in __call__
      return self.wrap_app(environ, session_start_response)
    File '/srv/mg.wandborg.se/mediagoblin/mediagoblin/app.py', line 158 in __call__
      return render_404(request)(environ, start_response)
    File '/srv/mg.wandborg.se/mediagoblin/mediagoblin/tools/response.py', line 33 in render_404
      request, 'mediagoblin/404.html', {}, status=400)
    File '/srv/mg.wandborg.se/mediagoblin/mediagoblin/tools/response.py', line 24 in render_to_response
      render_template(request, template, context),
    File '/srv/mg.wandborg.se/mediagoblin/mediagoblin/tools/template.py', line 82 in render_template
      context['csrf_token'] = render_csrf_form_token(request)
    File '/srv/mg.wandborg.se/mediagoblin/mediagoblin/meddleware/csrf.py', line 53 in render_csrf_form_token
      form = CsrfForm(csrf_token=request.environ['CSRF_TOKEN'])
    KeyError: 'CSRF_TOKEN'

BRANCH master

WORKING TREE STATE (latest commit)

::

    commit 72567762e36c849ffe8172b6cea4ca1be682e511
    Merge: a3663b4 ca9ebfe
    Author: Elrond <well@this-is-an-email-address.example.org>
    Date:   Mon Nov 28 18:40:45 2011 +0100
    
        Merge remote branch 'remotes/nyergler/issue-680-csrf-optout'
    
        * remotes/nyergler/issue-680-csrf-optout:
          Issue 680 Allow decorating views to prevent CSRF protection.
          Issue 680: Dispatch meddleware request processing post-routing



Change History (8)

comment:1 by Elrond, 12 years ago

The most interesting part is the 404. So somehow video pages
request an invalid URL, I think. I hope Joar can investigate that
part.

Reproducing **this** issue is thus very simple: Give GMG an invalid
URL, like /frob/ or so.

Adding Nathan Yergler, as he made the latest changes to the
meddlware.

The problem is, that the process\_request hook is called **after**
the render\_404 in app.py:MediaGoblinApp.**call**. And the
render\_404 wants to setup a complete context for rendering a 404
page. That context wants to contain a CSRF token.

I think, we have three options


1. Move the meddlware a bit up, just before the 404 stuff. This
   might not be very straight forward, as the route might not exist
   and so no controller might exist, and so all those controller/route
   based things in the meddlware must be skipped.
2. Make the render\_404 more autonomous. That is: Make it not
   depend on the normal tools and work "in any case".
3. Split meddlware stuff in "very early" hook for putting the csrf
   token in the context, and "after routing" for actual checking and
   controller based disabling.

I know, I don't like option 1, because it makes the meddlware
messy.
But I don't know, if I like option 2 or 3 better, **or** if I would
even prefer both. Opinions?



comment:2 by Elrond, 12 years ago

Status: NewIn Progress

comment:2 by nyergler, 12 years ago

I'm not sure this is actually a meddleware positioning issue. I
think the problem is that we're trying to **always** make the csrf
token available to templates, and because the process\_request half
of the CSRF meddleware never fired, the token never got generated.

I'd probably just handle this in render\_template and
render\_csrf\_form\_token (only provide the CSRF token if it
exists).



comment:3 by nyergler, 12 years ago

We should probably also have a unit test along the lines of

::

    assert_equal(test_app.get("/does-not-exist/").retval_as_int, 404)

This **should** fail before we apply a fix for this bug.



comment:4 by Elrond, 12 years ago

Okay, I have a failing unit test in my local tree on the line of
this assert\_equal thing.
I'll probably push it tomorrow.



comment:5 by Elrond, 12 years ago

Okay, pushed failing unit test.



comment:6 by Elrond, 12 years ago

Status: In ProgressClosed
Thanks Nathan Yergler for the suggestions!

I just implemented that and pushed it. Works fine!



comment:7 by Will Kahn-Greene, 12 years ago

The original url for this bug was http://bugs.foocorp.net/issues/685 .

Note: See TracTickets for help on using tickets.