Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#18826 closed Cleanup/optimization (worksforme)

A bit changed JavaScript for CSRF with async JS

Reported by: panco Owned by: nobody
Component: Documentation Version: 1.4
Severity: Normal Keywords: ajax, csrf
Cc: lrekucki@… Triage Stage: Design decision needed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by lrekucki)

Recently I've found use for the code found at https://docs.djangoproject.com/en/dev/ref/contrib/csrf/#ajax
and as I can't sleep well if my JS doesn't pass some of the basic JSlint standards I've changed the snippet a bit:

jQuery(document).ajaxSend(function (event, xhr, settings) {
        function getCookie(name) {
                var cookieValue = null, cookies = [], i = 0, j = 0, cookie = {};
                if (document.cookie && document.cookie !== '') {
                        cookies = document.cookie.split(';');
                        for (j = cookies.length; i < j; i += 1) {
                                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;
        }

        function sameOrigin(url) {
                // url could be relative or scheme relative or absolute
                var host = document.location.host, // host + port
                        protocol = document.location.protocol,
                        sr_origin = '//' + host,
                        origin = protocol + sr_origin;
                // Allow absolute or scheme relative URLs to same origin
                return (url === origin || url.slice(0, origin.length + 1) === origin + '/') || (url === sr_origin || url.slice(0, sr_origin.length + 1) === sr_origin + '/') || // or any other URL that isn't scheme relative or absolute i.e relative.
                        !(/^(\/\/|http:|https:).*/.test(url));
        }

        function safeMethod(method) {
                return (/^(GET|HEAD|OPTIONS|TRACE)$/.test(method));
        }

        if (!safeMethod(settings.type) && sameOrigin(settings.url)) {
                xhr.setRequestHeader("X-CSRFToken", getCookie('csrftoken'));
        }
});
  • "===" instead of "=="
  • all vars at the beginning of the function (and all the changes that brings forth)

I realize these changes are very small, but I think since this is a simple copy/paste snippet it should be of the highest quality possible (there's room for improvement still).
I'm using it and it performs as intended.

Change History (5)

comment:1 Changed 3 years ago by timo

  • Has patch unset
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

My feeling is that the current code is functional and works fine, but I'll defer if another core dev wants to accept this.

comment:2 Changed 3 years ago by lrekucki

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

The "Coding style" section in the docs is certainly missing a JavaScript section (this might be a good django-dev material). But until we get a one, I don't think making changes that hurt readability to satisfy a this particular linter is a good idea.

Version 0, edited 3 years ago by lrekucki (next)

comment:3 Changed 3 years ago by aaugustin

  • Triage Stage changed from Unreviewed to Accepted

The changes described in the report (=== and proper variable declaration) usually don't make the code less readable and they're widely accepted good practices.

But there are other changes in the snippet I'm less comfortable with:

  • using ajaxSend instead of ajaxSetup — no idea, I'm not a frontend dev;
  • re-compiling getCookie, sameOrigin and `safeMethod' every time — looks wasteful.

If there was a proper patch against the documentation, I would be willing to reconsider this ticket.

comment:4 Changed 3 years ago by aaugustin

  • Triage Stage changed from Accepted to Design decision needed

comment:5 Changed 3 years ago by lrekucki

  • Cc lrekucki@… added
  • Description modified (diff)

I mainly meant things like:

  • 8 spaces of indentation;
  • very long line ending with a comment;
  • initializing cookie to an object while it's later assigned a string;
  • not initializing value of i in the loop while initializing a j with list length for a premature optimization.

Changing to === is ok, while hoisting all the variables to the top, IMHO, actually makes the code less readable (but it's probably a matter of taste).

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