codyseibert / tab-tracker

A Vue.js / Express.js web application for keeping track of guitar tabs
MIT License
520 stars 197 forks source link

enhancement: some appsec issues in code #10

Open rpdeo opened 6 years ago

rpdeo commented 6 years ago

Hi,

First this was a pretty good tutorial on building a full stack app with vuejs and node/express. I enjoyed working through it. Although I implemented a few things differently (will release the repo soon on my github, after some cleanup).

I observed a few security hygiene issues with the app design, I am calling them out here just so other folks can benefit from knowing about it. Users should ensure these are fixed before deploying on any cloud services.

No code changes are expected from author as this is just a free tutorial available through freecodecamp youtube.

- Weak passpord policy validation: validation via regex on a set of
  character classes (this leads to any combination of characters being
  allowed, than say "at least" one of each character classes)

- Incorrect use of bcrypt hashing: no usage of random salt
  (`bcrypt-nodejs` provides an easy to use function.)

- Hashed password sent back in the clear in JSON API response. (o.O, use
  `delete user.password`! before `res.send()`)

- Incorrect use of password comparison function due to lack of
  understanding of JS async/await; a resolved `Promise` object will always
  evaluate to `true`, leading to any password being accepted during login.

- Bcrypt library used (=bcrypt-nodejs v0.0.3=, as of this usage) has a bug
  in `compare*` function where correct password still returns a `false`
  result. Mitigated with use of `===` JS operator. This was tested with
  separate code that makes use of salt value.

- Lack of cookies or user session implementation until passport-jwt was
  introduced in part 7 (last few minutes of this fairly long tutorial!).

- No change in session token with respect to user's login state:
  `not-logged-in -> logged-in -> logged-out`, this can lead to predictable JWT tokens.

These are just a few glaring issues! Fixing these would be recommended
before any `app deployment` section added for users.
jimelj commented 6 years ago

Hey @rpdeo can you elaborate more on the bcrypt bug, been stuck on this for days and wondering what was your work around...no matter what I do it always returns false... Thank you

rpdeo commented 6 years ago

@jimelj I am performing a simple string equals to (as noted above with === operator) between newly computed password hash and the one stored in the DB. See the bcrypt-nodejs examples on NPM.

jimelj commented 6 years ago

@rpdeo Thank you

codyseibert commented 3 years ago

@rpdeo thanks for posting this! These are useful for anyone trying to build a production ready application in the future.