Code refactor after manual security review

After performing security analysis using semgrep, I will perform a quick analysis of the code so far in order to think of possible vulnerabilities. Though it is still a rudimentary stage of the app, security if taken into consideration at the early stages might result in a more secure app.

I will use the Open Web Application Security Project (OWASP) and the vulnerabilities listed on their website to comb through the code. OWASP has a huge list of vulnerabilities listed on their website:

https://owasp.org/www-community/vulnerabilities/

They also have a shorter top 10 vulnerabilities list:

https://owasp.org/www-project-top-ten/

To begin with, one major shortcoming so far is the lack of any logs generated. Ideally, I would need to log every action with separate tags – info, debug, warning, error etc. These tags would help to filter the logs later and might also be used for generating automatic notifications in cases of serious breaches. Therefore, if there is an ongoing attack, rather than wait for the issue to be reported, if logs can be generated that send an automatic notification in terms of an email or a text message, this could help to minimize the damage or preventing it from completely bringing down the app. I will soon investigate logging with Django.

Now to analyze the code so far and examine ways in which it could be made a bit more secure. I will go through the outer view classes first. The first view class is the RegisterUserView. To begin with, since it is a POST request, this might need to handle CSRF. Since, there is no frontend at the moment, and requests are made with the browser using the forms generated by DRF or through a client such as Postman. Unless there is another browser accessing the API endpoints from another domain, CSRF does not really come into play. So this issue can be handled later for all endpoints.

In terms of the behaviour of this endpoint, for a valid registration that contains a username that is a genuine email and a password (not validated at the moment), the view returns an object with the username and the inactive status. This might be ok as the view is merely returning back what was supplied to it in the POST data. Now to look at when the API fails, this would occur when either username and/or password are missing, username is not an email, the passwords are not matching or if a user with that email already exists. All the errors are being caught using tests with appropriate error messages:

['Username must be a valid email']

The Password Field Is Required

The Username Field Is Required

The Username Field Is Required

A User With That Username Already Exists.

The Confirm Password Field Is Required

Passwords Are Not Matching.

The only potential issue is the error message that a username already exists in the database. It might be possible that a hacker could run a script off a collection of email addresses to check if any of them are registered in the app. In case they are, they might be potential entry points to the app. However, there are a number of ways in which someone could find email addresses and this probably would be the most painstaking. Also, if a user forgets the email with which they are registered and tries to register again, there should be a sensible error message that states that the email is already in the database. If the user cannot login with that email, they should reset the password. But for this action to be possible, the error message needs to be clear that the username exists. A vague error “Could not register user” may not give this information though it will also not give any indication that a user email exists in the database.

Next coming to the VerifyUserView which is after a user clicks on the verification link received by email. Here, the following conditions are possible – the verification is successful, the token has expired, the token is not genuine or if the user no longer exists. The last is if a user registers but does not click on the registration link for a long time, nor requests a new verification link. A cleanup script could delete all these inactivate accounts that have never been verified. The error responses are:

Token is invalid or expired

Token is invalid or expired

['User could not be activated']

The last error is in reality “User has been deleted”. However, there is no need to give out that much information. This might be the case where it is best not to disclose internal procedure to a user. The frontend need only display an error that activation failed and the user should try to ask for a new verification email or register again.

The next view is the ResendVerificationEmailView. This is when the user has not clicked the verification link in time. This is a get request with the user ID as a parameter. The valid case is when the user exists in the database. However, this is when it is clear that there is an issue with the send email utility method which is also used while registering a new user. The email method does not check if the user is already active. Though the verification link is something that will be used only in the beginning upon registration, it does have the capacity of becoming a nuisance if a number of random requests are made and all of them result in an attempt to send an email.

This only needs the send_verification_email method to be called inside the try block as it can throw an error that needs to be caught. And in the send_verification_method, only an additional check needs to be made if the user is activated and a ValidationError raised if that is the case.

In addition to this, there are a couple of issues with this view class. In the case of a success, it returns the email address corresponding to the user ID. This is a huge vulnerability as a get request with a random ID could return actual email addresses. In the request is a success, only a blank 200 response needs to be returned.

The second issue is that in case the user has been deleted or if the ID is simply random and does not correspond to a user, the error returned is “User matching query does not exist”. This is a core Django SQL error and should not be returned by the API as it is. In this case, a separate except block needs to be inserted that catches the error ObjectDoesNotExist from django.core.exceptions.

The next view class is the LoginUserView. This takes the username and password and either provides the JWT in the response or returns a 401 unauthorized.

The next view class is the ResetPasswordView. This also takes a user ID as parameter and sends a password reset link to the user email. A successful response is only a 200, while errors are if a user is not active which results in “No user to send email to” or if a user does not exist. The error for the user not existing is the same SQL error as before which needs that code refactor in the view class in the form of an extra except block.