Migration to bcrypt - Githubissues

OWASP / NodeGoat

The OWASP NodeGoat project provides an environment to learn how OWASP Top 10 security risks apply to web applications developed using Node.js and how to effectively address them.

https://www.owasp.org/index.php/Projects/OWASP_Node_js_Goat_Project

Apache License 2.0

1.89k stars 1.66k forks source link

Migration to bcrypt #158

Open UlisesGascon opened 5 years ago

UlisesGascon commented 5 years ago

Context

This is part of release-1.5 #148
MEDIUM priority task

Tasks

[ ] Remove dependency bcrypt-nodejs in package.json
[ ] Add dependency bcrypt in package.json
[ ] Migrate file app/data/user-dao.js to bcrypt
[ ] Validate the instalation with the local test
[ ] Add and submit the changes in package-lock.json
[ ] Add the primary dependency list to the readme.md
[ ] Check that the npm tasks are working as expected
[ ] Update the readme.md with the extra relevant info (if needed)

Assignation

This tasks is open for assignation, just claim it (as reply to this) and submit your PR ;-)

Important

Check the current roadmap #148
Don't forget to check the Contributing Guidelines and follow the Code of Conduct

PeterWunderlich commented 5 years ago

Hi all! As i want to get more into FOSS I stumbled upon this project again and saw this issue. I would claim it for now and would be glad to provide the PR by the end of this week, if it's okay? :)

If so, i'll do the migration from bcrypt-nodejs to bcrypt.

ckarande commented 5 years ago

@PeterWunderlich thank you for your interest in contributing to the project. Yes, you are very welcome to make the PR. I have assigned it to you 👍

UlisesGascon commented 5 years ago

Thank you, @PeterWunderlich! :-)

PeterWunderlich commented 5 years ago

Hey @ckarande & @UlisesGascon, the migration is finished from my part but i have 2 final questions regarding the tasks:

Add the primary dependency list to the readme.md: Shall i add a list of the dependencies (devDependencies exlucded) with a short explanation to the README.md? I would add the list before the "How to Setup Your Copy of NodeGoat" chapter.
I've run into a few problems on my Windows machine executing the npm scripts, as "NODE_ENV" is not a known command for my OS. Usually i use cross-env for convenience. Do you want me to add this, do you have any other suggestions or did I use something wrong? Maybe this could become a new issue? :)

As I'm finished i would create a PR heading to the "OWASP:release-1.5" branch or do you want me to merge elsewhere?

Thanks for your help. :)

UlisesGascon commented 4 years ago

Hi @PeterWunderlich!

Don't worry about the README.md dependencies. I will include an script that will autodocument that part for us.
Feel free to add cross-env to add support for windows users.
PR to OWASP:release-1.5 is perfect 👍

Thanks a lot for your support!

jantaylor commented 3 years ago

It looks like this task is already assigned and has not been implemented. Would you like me to take it on instead?

PeterWunderlich commented 3 years ago

@jantaylor sure, feel free to take over.

jantaylor commented 3 years ago

@PeterWunderlich I've submitted the PR if you want to look it over and give your review. @UlisesGascon, the PR is ready for your review as well.

snowkluster commented 1 year ago

Hi, I would like to work on this issue if the above PR has not been approved yet.

All that need to be done is to migrate from the deprecated bcrypt-nodejs to bcrypt and to validate the installation with the local tests.

lirantal commented 1 year ago

I've pinged Jan in the original PR. Let's see if he is able to follow-up first.

snowkluster commented 1 year ago

I've pinged Jan in the original PR. Let's see if he is able to follow-up first.

snowkluster commented 1 year ago

Is it alright if I open a PR as I think, I have fixed this issue as well as issue #271 and ran "npm run precommit" and fixed all those errors.

snowkluster commented 1 year ago

Are there any updates about the PR

jantaylor commented 1 year ago

@snow-kluster I haven't gotten any indication that the maintainers wanted to merge my PR #237.