feature/migrate-to-bcrypt

[jantaylor]

Context

• This is part of release-1.5 Roadmap for 2019-2020 #148
• References Issue: Migration to bcrypt #158

Tasks

• Remove dependency bcrypt-nodejs in package.json
• Add dependency bcrypt in package.json
• Migrate file app/data/user-dao.js to bcrypt
  • Validated that the password worked with the insecure password plain text and bcrypt hash
• Validate the installation with the local test
  • Runs and finishes immediately with "no files to check".
• Add and submit the changes in package-lock.json
• Add the primary dependency list to the readme.md
  • There is not a dependency list in readme.md
• Check that the npm tasks are working as expected
  • test:ci fails the first test due to the before() which runs cy.dbReset(). If I comment it out, it passes, and then the next test suite fails it's before() which does the cy.dbReset().
• Update the readme.md with the extra relevant info (if needed)
  • No extra info needed however I added a section for using mongo as a docker container

Additional changes:

• Updated the repository in package.json
• Added a section for using mongo as a docker container
• Had to also change app/routes/session.js to async/await for bcrypt or plain text check

I ran npm run precommit and it returned a long list of formatting fixes and changed 36 files. I did not commit those changes as it would disrupt this PR and feature.

[PeterWunderlich]

LGTM.

[rcowsill]

I think this might cause regression of the Cypress test flakiness that got fixed in #214. seedDatabase isn't waiting for the async work queued by dropDatabase to finish. If seedDatabase manages to connect to the database faster than dropDatabase, then the seed data would end up deleted or incomplete.

[jantaylor]

Agreed, good catch! The previous setup was ignoring the rejection of collections that didn't and still resolving them.
I'll call dropDatabase() within seedDatabse() or wrap them asynchronously.

[jantaylor]

@rcowsill It didn't go as smoothly as I wanted so I decided to revert the commit. It's probably best to keep the db-reset changes out of this PR.

[rcowsill]

The previous setup was ignoring the rejection of collections that didn't and still resolving them.

Yes that was to avoid errors when running db-reset.js twice. The script doesn't seed the collections "contributions" or "errors", so they'd fail to delete on a second run. That isn't an issue if it's changed to drop the database, but I'm wondering if the script will fail when run on an empty MongoDB instance.

@rcowsill It didn't go as smoothly as I wanted so I decided to revert the commit. It's probably best to keep the db-reset changes out of this PR.

Yeah, that's a good idea 👍

[jantaylor]

That isn't an issue if it's changed to drop the database, but I'm wondering if the script will fail when run on an empty MongoDB instance.

Right, that's what I noticed. It was always failing to run for the first time with the CI tests, though it ran fine manually in all cases (as the errors of dropping unknown collections weren't rejecting).

Anyways, we can always create another issue and work on it. 😄

[binarymist]

@jantaylor Are there no changes to be applied to docker-compose? You could also add a check list item that purpleteam doesn't find any more or fewer bugs? NodeGoat is one of the main SUTs we use to hone purpleteam

[jantaylor]

Are there no changes to be applied to docker-compose?

@binarymist Not to the docker-compose. Now that you mentioned that, I think there's a change required for the Dockerfile as alpine images of node usually don't have the required software installed for bcrypt, usually python. https://github.com/kelektiv/node.bcrypt.js#dependencies

I'll try building it and see if it works otherwise, it'd need to have python installed or move to node:12.

You could also add a check list item that purpleteam doesn't find any more or fewer bugs? NodeGoat is one of the main SUTs we use to hone purpleteam

I'm not sure I'm understanding the request. Did you want me to add a known issues list?
It looks like purpleteam is a security application that can test your apps. If my understanding is correct then are you are using NodeGoat for training & validations?

[binarymist]

I'm not sure I'm understanding the request. Did you want me to add a known issues list?
It looks like purpleteam is a security application that can test your apps. If my understanding is correct then are you are using NodeGoat for training & validations?

Purpleteam was born out of this very file which is of course now redundant. We've been using NodeGoat since as a SUT. I was thinking you could just add a task to the existing task list in this PR to just run purpleteam over NodeGoat once this change is ready? For me, this just means updating my NodeGoat fork and redeploying it using purpleteam-iac-sut which was also created to deploy NodeGoat on AWS for free using Terraform, then running purpleteam over it.

When I see your ready with this PR I can do the honors.

[lirantal]

@jantaylor happy to land the changes but want to run a check by you incase you had anything else you wanted to follow-up on.

[jantaylor]

@lirantal It looks like most of the changes are with the package-lock.json. For the the other two files, we need to make sure that app/routes/session.js calls await on userDAO.validateLogin. Feel free to land the changes.

[lirantal]

Seems that it does: https://github.com/OWASP/NodeGoat/pull/237/files#diff-c08aab9e521b9749dbc6a23ffb3163a41871c1d89162017d8c9bb702dfc29d2eR56

If you think it's ready as-is, I'll go ahead and land it.

[jantaylor]

@lirantal it looks like the conflicts need to be resoled. Want me to resolve them so we can get this PR merged?

[lirantal]

@jantaylor yes, thank you 🙏🏽