Should security code review involve stylistic culprits?

Question

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)
end

NOT_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?

Answer 1

Function misuse can create security issues in future, even if it works now. In this case, that function actually might create a security issue at some point if misused.

Also, I see a security code review as a more focused code review. If you notice other things that can improve the code, I see no reason why they should not be mentioned, as long as the focus of the whole review is security.

Depending on the nature of the issues, a note at the end would be not distracting and definitely helpful, e.g. "Also, I noticed that the usage of ... is not consistent in some places (e.g. X, Y, Z)".