They often publish pieces of disgusting code and memes about programming. But one day I saw something absolutely amazing there.
This piece of code has earned the honorary title of "best work" of the week.
I decided to disassemble this code, but there are so many wrong things that I find it difficult even to choose the first problem for analysis.
If you are a beginner programmer, then my material will help you understand what terrible mistakes were made by those who wrote this code.
28 lines of code errors
To make it more convenient, let's type this code in the editor:
<script>
function authenticateUser(username, password) {
var accounts = apiService.sql(
"SELECT * FROM users"
);
for (var i = 0; i < accounts.length; i++) {
var account = accounts [i];
if (account.username === username &&
account.password === password)
{
return true;
}
}
if ("true" === "true") {
return false;
}
}
$('#login').click(function() {
var username = $("#username").val();
var password = $("#password").val();
var authenticated = authenticateUser(username, password);
if (authenticated === true) {
$.cookie('loggedin', 'yes', { expires: 1 });
} else if (authenticated === false) {
$("error_message").show(LogInFailed);
}
});
</script>
And ... Well, really - I don't know where to start analyzing it. But you still need to start. I decided to break down the shortcomings of this code into three categories:
- Security issues.
- Problems with basic programming knowledge.
- Code formatting problems.
Security issues
This code is probably running on the client. This is indicated by the fact that it is wrapped in a tag
<script>(and, in addition, jQuery is used here).
But don't get me wrong. This code would look just as awful if it was for the server. But executing it on the client means that everyone who is able to read this code will have access to the database that is used in it.
Let's take a look at the function
authenticateUser:
function authenticateUser(username, password) {
var accounts = apiService.sql(
"SELECT * FROM users"
);
for (var i = 0; i < accounts.length; i++) {
var account = accounts [i];
if (account.username === username &&
account.password === password)
{
return true;
}
}
if ("true" === "true") {
return false;
}
}
From the analysis of its code, we can conclude that somewhere there is an object
apiServicethat gives us a method .sqlwith which we can execute SQL queries to the database. This means that if you open the developer console while viewing the page that contains this code, you can execute any queries against the database.
For example, you can run a query like this:
apiService.sql("show tables;");
In response, a complete list of database tables will be returned. And all this is done using the project's own API. But, what is already there, let's imagine that this is not a real problem. Let's take a better look at this:
if (account.username === username &&
account.password === password)
This code tells me that passwords are stored in the project without hashing. Great move! This means that I can take the debugger and see the passwords of the project users. I also believe that a large percentage of users use the same username / password pair on social media, email services, banking apps, and other similar places.
I also see the problem in how cookies are set in this code
loggedin:
$.cookie('loggedin', 'yes', { expires: 1 });
It turns out that it uses jQuery to set a cookie that tells the web application if the user is authenticated.
Remember: never set these cookies using JavaScript.
If you need to store this kind of information on the client, then cookies are most often used for this. I cannot say anything bad about such an idea. But setting a cookie using JavaScript means the attribute cannot be configured
httpOnly. This, in turn, means that any malicious script can access these cookies.
And yes, I know that only something like a key: value pair is stored here,
'loggedin': 'yes', so even if someone else's script reads something like this, there won't be much harm from it. But this is very bad practice anyway.
Also, when I open the Chrome console, I can always enter a command like
$.cookie('loggedin', 'yes', { expires: 1000000000000 });. As a result, it turns out that I have entered the site forever, even without having an account on it.
Problems with basic programming knowledge
We can talk and talk about such problems that can be found in this piece of code, and we don't have much time.
So, obviously a function
authenticateUseris an example of very poorly written code. It demonstrates that the person who wrote it lacks some basic knowledge of programming.
function authenticateUser(username, password) {
var accounts = apiService.sql(
"SELECT * FROM users"
);
for (var i = 0; i < accounts.length; i++) {
var account = accounts [i];
if (account.username === username &&
account.password === password)
{
return true;
}
}
if ("true" === "true") {
return false;
}
}
Maybe instead of getting a complete list of users from the database, it is enough to select a user with a given name and password? What happens if millions of users are stored in this database?
I already talked about this, but I will repeat myself, asking the following question: "Why don't they hash passwords in their database?"
Let's now take a look at what the function returns
authenticateUser. From what I can see, I can infer that it takes two type arguments stringand returns a single type value boolean.
Therefore, the following piece of code, although it is written horribly, cannot be called meaningless:
for (var i = 0; i < accounts.length; i++) {
var account = accounts [i];
if (account.username === username &&
account.password === password)
{
return true;
}
}
If you translate it into ordinary language, you get something like this: “Is there a user with the name X and with the password Y? If yes, return true. "
Now let's take a look at this piece of code:
if ("true" === "true") {
return false;
}
Nonsense. Why don't functions just return
false? Why does she need a condition that is always true?
Now let's analyze the following code:
$('#login').click(function() {
var username = $("#username").val();
var password = $("#password").val();
var authenticated = authenticateUser(username, password);
if (authenticated === true) {
$.cookie('loggedin', 'yes', { expires: 1 });
} else if (authenticated === false) {
$("error_message").show(LogInFailed);
}
});
The jQuery portion of this code looks fine. But the problem here is the organization of work with cookies
loggedin.
We have already said that, even without an account, you can simply open the Chrome console and run the command
$.cookie('loggedin', 'yes', { expires: 1 });and be authenticated for one day.
How does this page know who the authenticated user is? Maybe it just displays something special, intended only for those who successfully pass the username and password verification and do not display any personal data? We will not know this.
Code formatting issues
Styling is probably the smallest and most insignificant issue in this code. But it clearly indicates that the person who created this code copied and pasted its individual fragments from somewhere.
Here's an example of using double quotes when formatting strings:
var username = $("#username").val();
var password = $("#password").val();
And elsewhere, single quotes are used:
$.cookie('loggedin', 'yes', { expires: 1 });
This may sound unimportant, but it actually tells us that the developer may have copied some code from, say, StackOverflow, without even rewriting it, following the style guide used in the project. Of course, this is a small detail, but it indicates that the developer does not really care about understanding how the code works. This developer just needs the code to function somehow.
I would like to clarify here my point of view on this problem. I search Google for code-related stuff every day, but I think it's much more important to understand how to set a cookie, for example, rather than making a piece of code thoughtlessly copied from somewhere work. What if for some reason the program stops working? How can a programmer who does not understand what is going on in his code find an error?
Outcome
I am absolutely sure that this piece of code is a fake. Here I first saw an example of synchronous execution of an SQL query:
var accounts = apiService.sql(
"SELECT * FROM users"
);
Usually such tasks are solved as follows:
var accounts = apiService.sql("SELECT * FROM users", (err, res) => {
console.log(err); //
console.log(res); //
});
They are also solved like this:
var accounts = await apiService.sql(
"SELECT * FROM users"
);
Even if the method
apiService.sqlreturns the result in synchronous mode (which I doubt), it needs to open a connection to the database, execute a request, send the result to the call point. And these operations (as you might guess) cannot be performed synchronously.
But even if this is completely real code, I am sure that it was written by a novice programmer, junior. I'm sure that when I worked as a programmer for the first weeks, I also wrote terrible code (sorry: D).
But this is not the fault of the junior programmer.
Let's say this is real code that is running somewhere. Some junior would go all out for this code to work. This developer still has to learn how to properly handle SQL queries, cookies, and everything else. And in this light, it is completely normal.
But the chief of this programmer must control the work, point out mistakes, and get the beginner to understand his mistakes. This will lead to the fact that such terrible code does not make it to production.
I know for sure that there are companies that do not care about the quality of the code that goes into production. Does the code solve the problem? If so, it is being deployed without any questions. Is the code written by a junior and not even tested by a more experienced programmer? In production it!
In general, there are griefs in life.
Update as of August 8, 2020
When I was writing this article, I believed that the code I was analyzing was a fake. But after discussing the material on Reddit, I was given a link to this thread. As it turned out, this code is used in some internal system supporting 1,500 users. So I was obviously wrong. This is completely real code.
Have you come across frankly bad code fragments in production, full of all sorts of problems?
