Skim code review points

[queicherius]

Hey there,

I just skimmed through most of the code because I wanted to see if there was any inspiration for a project of mine.

I found the following points which might be interesting to you. Keep in mind that it was a very rough skim, so it's very possible i might have missed things.

• You are using JWTs for sessions (which I would not recommend) and using a Redis-backed blacklist for invalidated tokens. The Redis instance is (at least as far as I can tell) not started with correct persistence enabled. This means if Redis crashes all keys that got invalidated in the last 60 seconds are now valid again.
• Your types for Redis are set(key: string, value: any) when Redis can only handle value: string correctly
• Your rate limiting is setup to use in-memory storage, but you are running your processes clustered, so they don't share the same rate limit values. You should move that limit information into Redis, for example.
• You are hashing the password with sha256 which is vulnerable to a rainbow table attack. Please read this article on how to implement password storage safely.
• Your password length is limited to a max of 20 characters, when you should not limit it at all (or to something like 4k).
• You are saving password reset tokens as plaintext in the DB when they should be hashed

[onur-ozkan]

First of all, thank you very much for looking at my project and taking the time. I will try to give you answer in the same order:

• The Redis algorithm of JWT (I can proof JWT is much secure than any other current auth techs, take a look at this article) in Feednext is just extra security layer, if I enable redis persistance, it will effect the redis performance too much and I dont really need to get every tokens back into redis if there would be any crash. Tokens are 45min life time anyway. As I said, its just some small extra security thing.
• There was some types are missing in the project before, so that "any" type was quick solution at that time, the values that given to redis functions are already string type. (WILL BE FIXED)
• Each IP is going to single cluster via Nginx, so there wouldnt be any issue about memory-storage rate limit. You cant hit 2 or more clusters from single IP, every rate will be stored correctly.
• Yes, hashing algorithm mush be refactored with using bcrypt package. But first I need to figure out how to keep the current users with their passwords. At least I need to mail them with feednext mail service. (WILL BE FIXED)
• I didnt get this, I dont want to give too much space for passwords in db. Whats the point making it limitless ?
• Password tokens are gets deleted once they are used, but true, it should be hashed anyway. (WILL BE FIXED)

[onur-ozkan]

The improvements are listed at #82

[queicherius]

I didnt get this, I dont want to give too much space for passwords in db. Whats the point making it limitless ?

Since you are hashing the passwords they are always the same size. Following the article of bcrypt(base64(sha256(password))), the only limit is processing time, since this will always hash the password into 60 characters you are saving into the database. Even right now with your hash it will always result in 64 characters.

[onur-ozkan]

I didnt get this, I dont want to give too much space for passwords in db. Whats the point making it limitless ?

Since you are hashing the passwords they are always the same size. Following the article of bcrypt(base64(sha256(password))), the only limit is processing time, since this will always hash the password into 60 characters you are saving into the database. Even right now with your hash it will always result in 64 characters.

Oh thats right, thanks for the explanations. How the hell I didnt look from this view :) I will make the length 100-200 or so. Not because of the size in the database but payload size of the request.