Skip to content

Fix web UI when using session-based CSRF #6207

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Feb 19, 2019
Merged

Fix web UI when using session-based CSRF #6207

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
09f8ac2
include raw token in template, pass token value without getCookie() call
seawolf42 Sep 23, 2018
b98d9ef
fix error when rendering with no context
seawolf42 Sep 23, 2018
cee6823
update docs regarding coreapi with session-CSRF
seawolf42 Sep 23, 2018
9c23899
template includes csrfToken property in javascript
seawolf42 Oct 19, 2018
e7c6cc4
fix broken tests
seawolf42 Oct 23, 2018
cc2be97
test template render with request context
seawolf42 Oct 23, 2018
c71e472
remove notes from API docs as per @carltongibson request
seawolf42 Oct 24, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion rest_framework/static/rest_framework/js/csrf.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ function sameOrigin(url) {
!(/^(\/\/|http:|https:).*/.test(url));
}

var csrftoken = getCookie(window.drf.csrfCookieName);
var csrftoken = window.drf.csrfToken;

$.ajaxSetup({
beforeSend: function(xhr, settings) {
Expand Down
2 changes: 1 addition & 1 deletion rest_framework/templates/rest_framework/admin.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,7 @@ <h4 class="modal-title" id="myModalLabel">{{ error_title }}</h4>
<script>
window.drf = {
csrfHeaderName: "{{ csrf_header_name|default:'X-CSRFToken' }}",
csrfCookieName: "{{ csrf_cookie_name|default:'csrftoken' }}"
csrfToken: "{{ csrf_token }}"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again {% csrf_token %}? And do we need the if request check?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do need that check, because there is currently a unit test for rendering with no request and in that case the csrf_token call throws an exception (see https://code.djangoproject.com/ticket/29785, which you closed as "by design").

Copy link
Contributor Author

@seawolf42 seawolf42 Oct 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, read the comments out of order. Here we don't need the {% if request %} check because (presumably) there will always be a request when you load an admin page. In the base.html case, however, the check is only there because of the exception thrown if there is no request, this is to satisfy test_templates.test_base_template_with_no_context().

};
</script>
<script src="{% static "rest_framework/js/jquery-3.3.1.min.js" %}"></script>
Expand Down
2 changes: 1 addition & 1 deletion rest_framework/templates/rest_framework/base.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,7 +286,7 @@ <h1>{{ name }}</h1>
<script>
window.drf = {
csrfHeaderName: "{{ csrf_header_name|default:'X-CSRFToken' }}",
csrfCookieName: "{{ csrf_cookie_name|default:'csrftoken' }}"
csrfToken: "{% if request %}{{ csrf_token }}{% endif %}"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean {% csrf_token %}? c.f. #3703 &co.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How very odd... I am sure I used it this way when I was first figuring things out, clearly I was mistaken. OK, I'll have to re-work one test too, will take care of that in the next day or two.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, no, I do want {{ csrf_token }}. I don't want the entire HTML tag, just the value of the token.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good, and TemplateHTMLRenderer will create a RequestContext, so csrf_token will be in the context. Fine. 👍

};
</script>
<script src="{% static "rest_framework/js/jquery-3.3.1.min.js" %}"></script>
Expand Down
12 changes: 11 additions & 1 deletion tests/test_templates.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,17 @@
import re

from django.shortcuts import render


def test_base_template_with_context():
context = {'request': True, 'csrf_token': 'TOKEN'}
result = render({}, 'rest_framework/base.html', context=context)
assert re.search(r'\bcsrfToken: "TOKEN"', result.content.decode('utf-8'))


def test_base_template_with_no_context():
# base.html should be renderable with no context,
# so it can be easily extended.
render({}, 'rest_framework/base.html')
result = render({}, 'rest_framework/base.html')
# note that this response will not include a valid CSRF token
assert re.search(r'\bcsrfToken: ""', result.content.decode('utf-8'))