Implicit declaration of nextPing in script.js

[ghost]

Hey cool author,

I just wanted to open a question related to your implicit declaration of nextPing variable in apply function of sessionSecurity.prototype within the script.js file.

I was just curious, what was your reason for choosing to implicitly define nextPing? I am looking at the code, and now obviously the "best practice" would suggest not doing this, however it raised the question of, "When exactly is it okay to use the implicit declaration versus explicitly creating the variable within scope?" This to me is interesting, since I enjoy being a rule breaker myself.

Since it's a "best practice", and one that makes sense in most cases, I was wondering if you could walk me through this decision, since maybe this is the exception to the rule.

[ghost]

And please don't mistake this for sarcasm, I am genuinely curious!

[yscumc]

I am not the cool author, but if I had to guess, it was probably just forgotten. Since implicit vs explicit declaration in this case doesn't have any effect on the logic of the code, it's not technically a bug and I guess no one caught nor reported it. The only exception is if there's already a global variable named, nextPing, which is actually one of the reasons that the best practice is to explicitly declare a local variable.

Ideally, I think the whole file should be enclosed in an immediately executed anonymous function with "use strict" in the beginning to catch these errors and prevent the implicit escape of local variables into the global scope. Something like this, perhaps:

(function (window, $, undefined) {
    "use strict";
    // The file should go here
})(window, jQuery);

The author is still pretty cool though :)

[ghost]

Agreed, on all counts about the author being pretty cool. Thanks for your response, I appreciate the thought and the discussion, perhaps I will create a pull request here soon with the suggested fix -- or perhaps you may want to do that, or perhaps the author can let us know what he thinks.

The other thing I was thinking is that the actual SessionSecurity object could have an attribute instead, that way nextPing could be referred/set using this.nextPing?

[yscumc]

Unfortunately, I couldn't get the tests to run properly and I currently don't have the time to figure that out and create a PR. Since it's also your discovery, I think it's best if you create it and jpic can decide whether he wants to merge it or not.

For reference, here's the doc for use strict. Also, the reason why "use strict" should be inside the anonymous function instead of at the file level is because script files are sometimes concatenated for minification.

[ghost]

Okay, I will have a look at it when I'm not on company dime and create a pull request. I am in the process of porting this to Django 1.7 / Python34 for a project I am working on.

[jpic]

We have only three cases in that function: either return, either set nextPing to this, either set nextPing to that.

In practice, there isn't any chance that we have a problem of "nextPing is not defined".

However, we should declare our variable with var.

Pull request welcome ! For Python 3.4 support too - don't we already support Django 1.7 ? It's tested on travis ..

[ghost]

I'm wrapping up my Python34 implementation this week. I'd like to clean it up and make it a little more generic since it's kind of tailored to what I am working on at work, so some stuff will need to be removed (I didn't do a PIP install, rather followed along the code and used the parts I needed). I'd like to contribute the update back since my life would have been easier if I could have dropped this module into my project.

Regards.

[ghost]

about Django 1.7, you are supporting that, my project is just django1.7/python34, the 34 being the important part ;)

[jpic]

Question in the air: should we keep Python 3.3 support or move on to Python 3.4 ?

[ghost]

IMO I'd try to maintain backwards compatibility. Perhaps we could just maintain a separate branch? There were some other issues standing in the way of me using this just as a plugin to my project that prompted me to go and rewrite some of the functionality. Most of it was decisions that had already been made before we found this OSS project, and also for security and support purposes I thought it somewhat important that I go through and understand that code at more than just surface level.

[ghost]

Honestly, I'm not even sure that it couldn't work for Python34 but I could not run the travis yaml file to confirm this in my environment and I decided to detour and explore the code instead of set it and forget it by making it a project requirement.

[yscumc]

Just wanted to expand on what @jpic said

Not declaring nextPing would cause it to refer to a globally scoped nextPing. While this would definitely cause no problem for session security since JS is single-threaded and nextPing is immediately used after being assigned, this could cause problems for other code that happens to also use a global nextPing variable.

So while technically not a problem for session_security, I think this should be fixed by implicitly declaring it to be locally scoped (using var) so that it will play nice with not-so-nice scripts.

[jpic]

@b-d-b you can run the tests going in the test_project, installing requirements et running ./manage.py test session_security.