django-cookie-consent icon indicating copy to clipboard operation
django-cookie-consent copied to clipboard

beforeDeclined Not Working/Google Analytics Issue

Open EnochLucas opened this issue 3 years ago • 4 comments

BACKGROUND I have a project that uses google analytics to track scrolling. To disable google analytics cookies you have to use this window['ga-disable-YOUR-GA-TAG'] = true; to prevent scroll tracking. (Otherwise when you scroll it will just make the cookie come back.)

Attempt At Using beforeDeclined I saw in the test project for this package a beforeDeclined option in the "showCookieBar" JS function. To test this, I did the following:

var cookie_groups = [];
{% for cookie_group in cookie_groups %}
  cookie_groups.push("{{ cookie_group.varname }}");
{% endfor %}

function ready(fn) {
    if (document.readyState != 'loading') {
      fn();
    } else if (document.addEventListener) {
      document.addEventListener('DOMContentLoaded', fn);
    } else {
      document.attachEvent('onreadystatechange', function() {
        if (document.readyState != 'loading')
          fn();
      });
    }
}

ready(function() {
    showCookieBar({
    content: "... My Banner",
    cookie_groups: cookie_groups,
    cookie_decline: "{% get_decline_cookie_groups_cookie_string request cookie_groups %}",
    beforeDeclined: function() {
      console.log("Sanity Check") // Does not show up in the console
    }
  });
});

The console log "Sanity Check" does not show up in the console, making me believe the beforeDeclined option does not work. If so, I cannot use the window['ga-disable-YOUR-GA-TAG'] = true; when a user declines cookies using the beforeDeclined option.

My Workaround I simply copied the JS file in the cookie_consent/cookiebar.js file and added in the window['ga-disable-YOUR-GA-TAG'] = true; in the declined event listener, and used that JS instead.

Conclusion This brings up several questions,

  1. Are we meant to use the cookie_consent/cookiebar.js file? Or was that just meant as a demonstration, and we should be using our own method of handling this?
  2. Does anyone else have the same problem?
  3. Am I using the beforeDeclined option correctly?

EnochLucas avatar Sep 03 '21 20:09 EnochLucas

I am using this code in my base.html:

https://github.com/bmihelac/django-cookie-consent/blob/master/tests/core/templates/test_page.html#L70-L108

The cookiebar.js file is used for the cookie bar like the name suggests and should be located your static root django directory/folder after running python manage.py collectstatic.

I also do not get anything printed from console when using console.log("Sanity Check"); like you have above if I did exactly the same thing as test_page.html.

In cookiebar.js, I am getting a problem where document.cookie is not the same as opts.cookie_decline; If we put console.log(opts); after e.preventDefault(); the browser inspect console beforeDeclined details looks like it keeps infinite looping if you keep expanding the details. console.log(typeof opts.declined); is undefined when clicking decline on the cookie bar. There are some wacky things going on if you console.log() every line in this file.

If you view the source code, I see the following for document.cookie = "{% get_decline_cookie_groups_cookie_string request cookie_groups %}";

document.cookie = "cookie_consent=GROUP1=-1|GROUP2=-1|GROUP3=-1; expires=Thu, 02 Mar 2023 13:37:15 GMT; path=/";

Where GROUP1, GROUP2, GROUP3 are the names of my cookie groups.

I am not sure what the objective of beforeDeclined is. It might not be used at all. But it seems like it is there to set document.cookie to something before the cookie bar is declined.

innerhtml is a vulnerability (specifically XSS) because it will also run the code given to it.

https://gomakethings.com/preventing-cross-site-scripting-attacks-when-using-innerhtml-in-vanilla-javascript/

These things all seem to be undesirable:

eval(), innerHTML, var, type="text/javascript" / DOMContentLoaded

some1ataplace avatar Mar 02 '22 04:03 some1ataplace

I've looked the JS over and it does seem to have a bug there. I commented on the specific piece of code. It's the bit where you click the cookie bar.

document
  .querySelector(".cc-cookie-decline", content)
  .addEventListener('click', (e) => {
    e.preventDefault();
    if (typeof opts.declined === "function") {
      // ? where is this declined attribute coming from? Shouldn't is be cookie_declined?
      // (and, if so, what would be the point of calling it here if beforeDecline was already meant to be the callback?
      opts.declined();  
    }
    fetch(e.target.getAttribute("href"), {method: "POST"})
    .then(() => {
      content.style.display = "none";
      body.classList.remove('with-cookie-bar');
      if (opts.cookie_decline) {
        // ! There should be a call to beforeDecline (_if it exists_) here, right? Like in the comment below:
        // beforeDecline();
        document.cookie = opts.cookie_decline;
      }
    })  

Tell me what you all feel about it.

MrCordeiro avatar Apr 02 '22 10:04 MrCordeiro

@MrCordeiro Thanks and great work for looking this over. Your suggestions allowed me to think of this:

You can call opts.beforeDeclined(); anywhere after const opts = Object.assign(defaults, options); is defined within cookiebar.js in either of your comments to execute that beforeDeclined function in test_page.html so we may be able to resolve this issue by doing that.

Maybe instead of opts.declined which is undefined we need to use opts.cookie_decline or opts.beforeDeclined() instead like you thought of which seems to work.

I am thinking what we have here are typos where it should be like this instead:

#Since opts.beforeDeclined() is a function and opts.cookie_decline is not
if (typeof opts.beforeDeclined() === "function") {
    opts.beforeDeclined();
}

#I think we can leave this alone? document.cookie will exclude csrftoken
if (opts.cookie_decline) {
    document.cookie = opts.cookie_decline;
}

I am not 100% sure if this should be the way. Maybe it is actually the opposite order. It cannot be determined what the original intent was since both if statements have 'decline' in them.

#Normally printing document.cookie and not setting it to opts.cookie_decline;
console.log(document.cookie);
csrftoken=fzozRpjLZwxI2kZuuIyCyK6W8sVOZrojVa2gLaBsp9XI1g0MqcKGZbo7KyMQed7q; cookie_consent=social=-1|optional=-1

console.log(typeof opts.cookie_declined);
undefined

console.log(typeof opts.beforeDeclined);
function

console.log(opts.cookie_decline);
cookie_consent=social=-1|optional=-1; expires=Sun, 02 Apr 2023 18:48:52 GMT; path=/

console.log(opts.beforeDeclined);
ƒ () {
           document.cookie = "cookie_consent=social=-1|optional=-1; expires=Sun, 02 Apr 2023 18:52:24 GMT; path=/";
}

As an aside, I also thought that the evalXCookieConsent in cookiebar.js was strange when accepting the cookie bar. Accepting the cookie bar looks for the script type like this <script type="x/cookie_consent"> which is the google+ API script in test_page.html.

The function currently works like this after console.log for each line after clicking accept all cookies:

var src is null if src is not null then create a <script> tag and append this script tag to the <head> tag else eval(script.innerHTML) which is undefined remove the entire script

function evalXCookieConsent(script) {
  //null when accepting all cookies from the cookie bar using the current test_page.html (since there is no src attribute)
  var src = script.getAttribute("src");
  console.log(src);
  //if not null then create <script> in the <head> tag - there is a src in the <script> tag on test_page.html
  if (src) {
      var newScript = document.createElement('script');
      newScript.src = src;
      document.getElementsByTagName("head")[0].appendChild(newScript);
      console.log(document.head.innerHTML);
  } else {
      //there is no src defined, so execute the script
      eval(script.innerHTML);
  }
  /*
console.log(script); = 
<script type="x/cookie_consent" data-varname="social">
      (function() {
        var po = document.createElement('script'); po.type = 'text/javascript'; po.async = true;
        po.src = 'https://apis.google.com/js/plusone.js';
        var s = document.getElementsByTagName('script')[0]; s.parentNode.insertBefore(po, s);
      })();
    </script>
  */

  script.remove();
}

src is usually an attribute / link to the javascript file in the

https://django-cookie-consent.readthedocs.io/en/latest/getting_started.html#checking-for-3rd-party-cookies-dynamically

However, according to the docs link description, evalXCookieConsent seems to be working properly so I may be completely wrong about it being buggy.

We still have to rewrite the javascript and make it more secure but at least we are getting closer!

We can use the function constructor instead of eval. But some told me eval may not be needed at all. Not sure what the original intention of eval was.

var fxn = new Function("code here"); 
fxn();

eval() is just a case of a huge security risk due to entirely the wrong approach. You should never need to treat a string as JS . eval() and innerHTML() can be dangerous as they allow malicious users from injecting bad scripts into your program.

innerHTML - XSS vulnerabilities and there are plenty of alternative methods var - explained by the tag and linked article type="text/javascript" - redundant, even if you actively want to write a classic script (you shouldn't) DOMContentLoaded - redundant with type=module's (which comes with many other benefits, as per the tag) deferring behaviour

Never use .innerHTML before you really know how to use it safely, in addition to the window.trustedTypes API. https://developer.mozilla.org/en-US/docs/Web/API/Trusted_Types_API Or, instead, just use one of the following methods:

  • element.append(...contents)
  • element.prepend(...contents)
  • node.replaceWith(...contents)
  • node.after(...contents)
  • node.before(...contents)
  • node.textContent = textContent

Please never use var. Default to const, switch to let only if you need to reassign (a lot less often than you probably think). See https://javascript.info/var for the reasons why. If you need to support legacy browsers, use a transpiler such as Babel.js. var is not recommended, better use ES6 let or const

Unless you're supporting ancient legacy systems, always add type="module" to all your script tags:

and place the tag inside .

some1ataplace avatar Apr 03 '22 04:04 some1ataplace

I am still wondering if beforeDeclined() is truly correct though because right now my PR executes it when the user clicks decline on the cookie bar and it isn't truly before declined. It is more like after declined or when clicking decline.

If we don't put the beforeDeclined function inside of the decline button click this seems like it is truly beforeDeclined:

test_page.html:

          beforeDeclined: function() {
            document.cookie = "username=John Doe";
            console.log(document.cookie);
          }

cookiebar.js:

    if (typeof opts.beforeDeclined() === "function") {
      opts.beforeDeclined();
    }

  document
  .querySelector(".cc-cookie-decline", content)
  .addEventListener('click', (e) => {
    e.preventDefault();
    fetch(e.target.getAttribute("href"), {method: "POST"})
    .then(() => {
      content.style.display = "none";
      body.classList.remove('with-cookie-bar');
      if (opts.cookie_decline) {
        document.cookie = opts.cookie_decline;
      }
    })  
  });
}

Update: I changed the beforeDeclined function to now truly be before declined. I don't think this should interfere with COOKIE_CONSENT_OPT_OUT setting but maybe I am wrong.

The other problem is that "username=John Doe"; will still be set after declining the cookies. Actually now I think that we indeed need to keep it as it was originally or put a warning in the docs so that the document.cookie does not get set in the browser before accepting/declining. So my recommendation would be to put a warning in the docs and remove document.cookie from the beforeDeclined function altogether. I also don't know if beforeDeclined differs much from using regular javascript separate from django-cookie-consent to do the same thing?

some1ataplace avatar Apr 06 '22 20:04 some1ataplace

The beforeDeclined call was patched in #90 and it sounds like that should resolve this issue. django-cookie-consent 0.4.0 is now on PyPI, so you can try installing it. I'm closing this issue, but feel free to re-open it if the original problem is not solved with this patch yet!

sergei-maertens avatar Jun 11 '23 15:06 sergei-maertens