Opened 3 years ago

Closed 19 months ago

#21495 closed New feature (fixed)

Add a setting for CSRF Header name

Reported by: hello@… Owned by: Grzegorz Ślusarek
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 3 years ago by Horst Gutmann

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Triage Stage: UnreviewedAccepted

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 3 years ago by Susan Tan

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 3 years ago by Susan Tan

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 3 years ago by anonymous

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

comment:5 Changed 3 years ago by Unai Zalakain

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 3 years ago by Unai Zalakain

Cc: unai@… added

comment:7 Changed 3 years 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 2 years ago by Tim Graham

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 2 years 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 2 years ago by Tim Graham

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 22 months ago by Carl Meyer

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 20 months ago by Grzegorz Ślusarek

Owner: changed from nobody to Grzegorz Ślusarek
Patch needs improvement: unset
Status: newassigned

comment:13 Changed 20 months ago by Tim Graham

Component: HTTP handlingCSRF
Version: 1.6master

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 19 months ago by Grzegorz Ślusarek

Needs documentation: unset
Needs tests: unset

pr fixed, tests and documentation added

comment:15 Changed 19 months ago by Tim Graham

Patch needs improvement: set

I've added comments for improvement on the PR.

comment:16 Changed 19 months ago by Grzegorz Ślusarek

Patch needs improvement: unset

improved patch, squashed changes, updated PR

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

Resolution: fixed
Status: assignedclosed

In 668d53cd125175eb708cc0af143f47b42cd42153:

Fixed #21495 -- Added settings.CSRF_HEADER_NAME

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