Closed Bug 910691 Opened 9 years ago Closed 6 years ago

API response shouldn't create anoncsrf cookie

Categories

(Input Graveyard :: Submission, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: willkg, Unassigned)

Details

(Whiteboard: u=dev c=api p= s=)

When using the Input API, it shouldn't create an anoncsrf cookie when you post feedback data.
Whiteboard: u=dev c=api p= s=input.2013q3
I'm pretty sure this isn't hard to do--it just needs looking at.

Bumping it to 2013q4.
Priority: -- → P3
Whiteboard: u=dev c=api p= s=input.2013q3 → u=dev c=api p= s=input.2013q4
This is annoying, but I'm going to push it off to the next quarter.

Also, this is highly related to bug #947767. They're both csrf-token-with-api related.
Whiteboard: u=dev c=api p= s=input.2013q4 → u=dev c=api p= s=input.2014q1
Moving this to 2014q2.
Whiteboard: u=dev c=api p= s=input.2014q1 → u=dev c=api p= s=input.2014q2
Pushing it off again.
Whiteboard: u=dev c=api p= s=input.2014q2 → u=dev c=api p= s=input.2014q3
There are two steps here:

1. Figure out what code is creating the anoncsrf cookie.

2. Depending on what the answer to 1 is, fix it so that it doesn't create cookies for API things.
Whiteboard: u=dev c=api p= s=input.2014q3 → u=dev c=api p= s=input.2014q4
I'm pushing this to the backlog since this isn't crucial and this quarter is feeling pretty full.
Whiteboard: u=dev c=api p= s=input.2014q4 → u=dev c=api p= s=
The cookie is set by django-session-csrf middleware due to the ANON_ALWAYS=True setting that we have configured in fjord/settings/base.py. Here is what the README file (https://github.com/mozilla/django-session-csrf/blob/master/README.rst) of the package says 

<snip>
If you want all views to have CSRF protection for anonymous users, use the following setting:

    ANON_ALWAYS

        always provide CSRF protection for anonymous users

        Default: False
</snip>

I tried overriding it by using the "anonymous_csrf_exempt" decorator, but that doesn't work as the setting seems to override it.

One way to solve this would be to remove ANON_ALWAYS setting (defaults to False) and manually add it to all the views that require the anonymous csrf protection. But that would be very cumbersome to say the least.

Another way would be to fork django-session-csrf (since it doesn't appear to be actively maintained) and add code changes to not apply the anonymous CSRF protection to views decorated with anonymous_csrf_exempt decorator even when ANON_ALWAYS is set to True. This looks to be a very simple code change which I can make.

OTOH, if it is possible to discuss this with the maintainer of the project and see if we can merge our changes, then nothing like it.
django-session-csrf is a Mozilla project, so I guess we can discuss this with the maintainer and see if we can merge our proposed change.
Looking at that repository, I doubt that there is a maintainer. There certainly isn't any activity.

I don't know much about django-session-csrf and I don't think I understand why we use it rather than the Django csrf stuff. I'll need to spend some time reading through the code in question. If we don't need it--if Django has upped its csrf game or django-session-csrf just isn't important for Input or some third reason--then we can nix it. If we do need it, then your idea about fixing the decorator sounds like a good one.
I have made code changes by forking willkg's fork of django-session-csrf. Here is the pull request - https://github.com/willkg/django-session-csrf/pull/1/

What I have done is, I have created a new decorator anonymous_csrf_exempt_always (instead of changing the behaviour of the existing anonymous_csrf_exempt decorator) and made the changes to not set the anoncsrf cookie when the view is decorated with it. This will not break any of the existing code in django-session-csrf or fjord as it introduces a new decorator and makes changes to the behaviour only when it is used.
(In reply to lgp171188 from comment #10)
> I have made code changes by forking willkg's fork of django-session-csrf.
> Here is the pull request -
> https://github.com/willkg/django-session-csrf/pull/1/
> 
> What I have done is, I have created a new decorator
> anonymous_csrf_exempt_always (instead of changing the behaviour of the
> existing anonymous_csrf_exempt decorator) and made the changes to not set
> the anoncsrf cookie when the view is decorated with it. This will not break
> any of the existing code in django-session-csrf or fjord as it introduces a
> new decorator and makes changes to the behaviour only when it is used.

I think you can just use django's built-in csrf_exempt decorator:
https://docs.djangoproject.com/en/dev/ref/csrf/#django.views.decorators.csrf.csrf_exempt
(In reply to Ricky Rosario [:rrosario, :r1cky] from comment #11)
> (In reply to lgp171188 from comment #10)
> > I have made code changes by forking willkg's fork of django-session-csrf.
> > Here is the pull request -
> > https://github.com/willkg/django-session-csrf/pull/1/
> > 
> > What I have done is, I have created a new decorator
> > anonymous_csrf_exempt_always (instead of changing the behaviour of the
> > existing anonymous_csrf_exempt decorator) and made the changes to not set
> > the anoncsrf cookie when the view is decorated with it. This will not break
> > any of the existing code in django-session-csrf or fjord as it introduces a
> > new decorator and makes changes to the behaviour only when it is used.
> 
> I think you can just use django's built-in csrf_exempt decorator:
> https://docs.djangoproject.com/en/dev/ref/csrf/#django.views.decorators.csrf.
> csrf_exempt

The csrf_exempt decorator provided by Django is indeed being used in the view that has this bug. See https://github.com/mozilla/fjord/blob/master/fjord/feedback/api_views.py#L137

The problem is, with ANON_ALWAYS=True configured in settings module, the csrf_exempt decorator is not honored by django-session-csrf. It may have been intentional or an accidental side effect and I am not sure about that.

I didn't want to change the existing behaviour of django-session-csrf in any way as I didn't want to risk breaking anything that uses it. That is why I have added a new decorator and customized the behavior only when it is used.

Of course, modifying django-session-csrf to honor csrf_exempt would be the cleanest and ideal solution. But for the reasons stated above, I haven't done it.
The Input service has been decommissioned (see bug 1315316) and has been replaced by a redirect to an external vendor (SurveyGizmo). I'm bulk WONTFIXing Input bugs that do not appear to be relevant anymore.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
Product: Input → Input Graveyard
You need to log in before you can comment on or make changes to this bug.