I have worked in company A for less than 1 year and I was given the task to do a security code review for a small project I was involved in the late 3 months. Until now I have been mainly been reading about common security culprits like SQL injection, XSS, etc. and trying to find these in the codebase.During the review however I stumbled into something like:def user_id  return (session[:user_id] ? session[:user_id] : NOT_LOGGED_IN)endNOT_LOGGED_IN evaluates to WEBUSER. To me this seemed hazardous for various reasons so I started commenting the snippet:# INSECURE: Although the function works fine it can be susceptive to #           misuse since under the hood it sets a username to a non-user.And then I started thinking.. Is this actually something fitted in a security code review or the more common code review?I assume the main difference between the proper security issues like SQL injection and XSS is that they are very specific and thus objective. When it comes into naming of functions and variables, it involves what different people think and thus becomes subjective. Someone in this case might argue for example that user is anyone visiting the site - no matter if he is registered or not so in that case WEBUSER would be a valid guest id (but then again it is counter-intuitive that many such users would share the same id).So how should someone approach the subjective versus objective issues battle?
Should security code review involve stylistic culprits?
code reviews;clean code;code security