Opened 21 months ago

Closed 5 months ago

#21495 closed New feature (fixed)

Add a setting for CSRF Header name

Reported by: hello@… Owned by: gregorth
Component: CSRF Version: master
Severity: Normal Keywords: csrf, header, angularjs
Cc: unai@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

CSRF includes a few customizations in settings:
https://github.com/django/django/blob/master/django/conf/global_settings.py#L544
but neglects allowing the user to set the Header name used by the server.

It would be very helpful to have this setting to use with AngularJS. While AngularJS allows overriding the cookie and header name, it would be better for my workflow (and I'm sure others) to set this on the server side and then AngularJS's CSRF functionality will "just work".

Details on the AngularJS CSRF workings:
http://docs.angularjs.org/api/ng.$http § Cross Site Request Forgery (XSRF) Protection

Pull request:
https://github.com/django/django/pull/1958

Change History (17)

comment:1 Changed 21 months ago by zerok

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

Since also the cookie name is also configurable it definitely makes sense to also make the header name configurable since it might be used from the Django-context. The same is not necessarily true for the name of the form-field so keeping this one hard-coded is IMO valid.

comment:2 Changed 21 months ago by susan

I made a separate PR that addresses other people's suggestions here: https://github.com/django/django/pull/1989 Feel free to code review; I'm unsure what test(s) to add.

comment:3 Changed 21 months ago by susan

I made a separate PR that addresses other people's suggestions here: https://github.com/django/django/pull/1989 Feel free to code review; I'm unsure what test(s) to add.

comment:4 Changed 20 months ago by anonymous

YAPR (Yet Another PR) with a different interpretation for this change: https://github.com/django/django/pull/1995

comment:5 Changed 20 months ago by unaizalakain

  • Easy pickings unset
  • Needs documentation set
  • Needs tests set

I'm +1 on the last implementation (https://github.com/django/django/pull/1995) but this needs tests and docs. I would also propose to the mailing list to follow a deprecation timeline with the settings moved inside CsrfViewMiddleware. I'd be glad if an issue initially requiring Yet An Other Setting turned out in an issue removing 6 settings.

comment:6 Changed 20 months ago by unaizalakain

  • Cc unai@… added

comment:7 Changed 20 months ago by WesAlvaro

Sure. If we're good on the idea, I'll create tests and mail the list. I much prefer the idea of configuring the middleware vs adding settings!

comment:8 Changed 12 months ago by timo

As I noted on the mailing list thread: "The main problem I see with this approach is that it would no longer be straightforward for 3rd party code to access these settings. You'd need something akin to get_user_model() to retrieve the currently installed CSRF middleware so you could access its settings."

comment:9 Changed 12 months ago by anonymous

Thanks for the comment, I've replied to the thread. An interesting concern, but one that I don't think we should be accommodating.

comment:10 Changed 11 months ago by timgraham

  • Patch needs improvement set

The idea of moving settings into middleware has been thoroughly rejected on a django-developers thread discussing integrating django-secure's middleware. If it can be done in a backwards compatible way, maybe it would make sense to group the CSRF settings into a dictionary as suggested in the thread. See #22734 which discusses doing this for the SMTP & email settings and also has a patch with proof of concept code.

comment:11 Changed 8 months ago by carljm

Noting here that that mailing list thread also eventually reached a fairly negative conclusion about the benefits of grouping related settings into dictionaries. So I think an implementation of this ticket would best just add a new setting.

comment:12 Changed 5 months ago by gregorth

  • Owner changed from nobody to gregorth
  • Patch needs improvement unset
  • Status changed from new to assigned

comment:13 Changed 5 months ago by timgraham

  • Component changed from HTTP handling to CSRF
  • Version changed from 1.6 to master

Thanks, but tests and documentation are also required. See our patch review checklist for tips. Please uncheck the appropriate flags on this ticket when that's been added.

comment:14 Changed 5 months ago by gregorth

  • Needs documentation unset
  • Needs tests unset

pr fixed, tests and documentation added

comment:15 Changed 5 months ago by timgraham

  • Patch needs improvement set

I've added comments for improvement on the PR.

comment:16 Changed 5 months ago by gregorth

  • Patch needs improvement unset

improved patch, squashed changes, updated PR

comment:17 Changed 5 months ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

In 668d53cd125175eb708cc0af143f47b42cd42153:

Fixed #21495 -- Added settings.CSRF_HEADER_NAME

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