Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#15469 closed Bug (fixed)

CSRF/Ajax/JQuery - Token is set to be inserted on both GET and POST

Reported by: goran@… Owned by: nobody
Component: CSRF Version: master
Severity: Normal Keywords: csrf, ajax, jquery
Cc: aymeric.augustin@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX:


My understanding of the documentation is that a csrf token is only required for POST requests.

The JQuery code currently attaches (or attempts to insert the token) to XHR requests for both GET and POST actions to urls on the same domain.

I suggest that we need to insert an if statement to test if the request is a POST before an unnecessary loop of the browser cookies is called.

$('html').ajaxSend(function(event, xhr, settings) {
    xhr.setRequestHeader("x-testing1", 'testme1');
    function getCookie(name) {
        var cookieValue = null;
        if (document.cookie && document.cookie != '') {
            var cookies = document.cookie.split(';');
            for (var i = 0; i < cookies.length; i++) {
                var cookie = jQuery.trim(cookies[i]);
                // Does this cookie string begin with the name we want?
                if (cookie.substring(0, name.length + 1) == (name + '=')) {
                    cookieValue = decodeURIComponent(cookie.substring(name.length + 1));
        return cookieValue;
    if (settings.type == 'POST') {
    	if (!(/^http:.*/.test(settings.url) || /^https:.*/.test(settings.url))) {
        	// Only send the token to relative URLs i.e. locally.
        	xhr.setRequestHeader("X-CSRFToken", getCookie('csrftoken'));

Note the insertion of the (settings.type == 'POST') test (I understand this could be incorporated in the below if statement but thought it was more readable to present it this way.

Change History (9)

comment:1 Changed 6 years ago by Luke Plant

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

We decided to leave this check out for simplicity, but adding it would probably be better, due to the overhead of cookie parsing.

However, we would want to do settings.type != 'GET' instead of the above, for future compatibility with PUT and DELETE which will need the token. (Our CSRF protection doesn't handle them yet, but it should and hopefully will soon). I'd also like to find out whether case insensitive matching on the method name 'GET' might be required.

comment:2 Changed 6 years ago by Gabriel Hurley

Component: Contrib appscontrib.csrf

comment:3 Changed 5 years ago by Aymeric Augustin

Cc: aymeric.augustin@… added

settings.type is uppercased at line 6674 of jQuery.js: s.type = s.type.toUpperCase();

The ajaxSend event is fired at line 6756: globalEventContext.trigger( "ajaxSend", [ jqXHR, s ] );

So we can safely assume that settings.type is uppercase.

I also noticed jQuery computes settings.hasContent like this: s.hasContent = !rnoContent.test( s.type ); where rnoContent = /^(?:GET|HEAD)$/.

This parameter is internal to jQuery; it is used, for instance, to determine whether to send the X-Requested-With header.

So the test for safe requests could be: if (!settings.hasContent) { /* send token on local requests */ }

Finally, since jQuery 1.5, settings.crossDomain tells whether the request is cross-domain — see around line 6647. Hence my suggestion:

$('html').ajaxSend(function(event, xhr, settings) {

    // ... define getCookie here ...

    // only send the CSRF token for local unsafe requests — safe requests are GET and HEAD
    if (!s.crossDomain && s.hasContent ) {
         xhr.setRequestHeader("X-CSRFToken", getCookie('csrftoken'));

One last thought: we could mention the possibility to use and this code:

$('html').ajaxSend(function(event, xhr, settings) {
    // only send the CSRF token for local unsafe requests (safe requests are GET and HEAD)
    if (!s.crossDomain && s.hasContent ) {
         xhr.setRequestHeader("X-CSRFToken", $.cookie('csrftoken'));

Any opinions on this before I propose a patch?

comment:4 Changed 5 years ago by Łukasz Rekucki

Severity: Normal
Type: Bug

comment:5 Changed 5 years ago by Luke Plant

Easy pickings: unset

I'm dubious about using settings.hasContent because it is only accidental that it does the right thing.

We also can't make the default implementation use things only available in jQuery 1.5

But we can mention some of these things in the docs.

comment:6 Changed 5 years ago by Luke Plant

In [16190]:

Mentioned simplification of AJAX example code in CSRF docs.

Refs #15469. Thanks to aaugustin for the suggestion

comment:7 Changed 5 years ago by Luke Plant

Resolution: fixed
Status: newclosed

In [16191]:

Fixed #15469 - CSRF token is inserted on GET requests

Thanks to goran for report.

comment:8 Changed 5 years ago by Luke Plant

In [16193]:

[1.3.X] Fixed #15469 - CSRF token is inserted on GET requests

Thanks to goran for report.

Backport of [16191] from trunk.

comment:9 Changed 5 years ago by Luke Plant

In [16194]:

[1.2.X] Fixed #15469 - CSRF token is inserted on GET requests

Thanks to goran for report.

Backport of [16191] from trunk.

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