Code

Opened 3 years ago

Closed 3 years ago

Last modified 3 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: contrib.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:

Description

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));
                    break;
                }
            }
        }
        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.

Attachments (0)

Change History (9)

comment:1 Changed 3 years ago by lukeplant

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

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

  • Component changed from Contrib apps to contrib.csrf

comment:3 Changed 3 years ago by aaugustin

  • 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 http://plugins.jquery.com/project/Cookie 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 3 years ago by lrekucki

  • Severity set to Normal
  • Type set to Bug

comment:5 Changed 3 years ago by lukeplant

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

In [16190]:

Mentioned simplification of AJAX example code in CSRF docs.

Refs #15469. Thanks to aaugustin for the suggestion

comment:7 Changed 3 years ago by lukeplant

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

In [16191]:

Fixed #15469 - CSRF token is inserted on GET requests

Thanks to goran for report.

comment:8 Changed 3 years ago by lukeplant

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

In [16194]:

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

Thanks to goran for report.

Backport of [16191] from trunk.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.