Adding secondary /api/login route for JSON user login processing

[lirantal]

Implementation of idea suggested in #90

The secondary JSON login route at /api/login can now be exploited and demonstrated using classic MongoDB noSQL injection by posting the following:

{
	"userName": {"$gt": ""},
	"password": {"$gt": ""}
}

[binarymist]

Have you run the grunt precommit and security regression test over this?

[lirantal]

No, thanks for bringing it up.
Running that updates other files as well in the repo, should I commit with just the files in this PR or everything it fixed?

[binarymist]

I know there is a line or two that upsets the linter in the profile-test.js, which I introduced ages ago before I realised the linting was a thing for NodeGoat, I'll fix that when I revisit the sec reg tests, but I think any other files can be commited. Which files is the linter complaining about?

[lirantal]

quite a few:

        modified:   app/data/memos-dao.js
        modified:   app/data/user-dao.js
        modified:   app/routes/memos.js
        modified:   app/routes/profile.js
        modified:   app/routes/session.js
        modified:   app/views/allocations.html
        modified:   app/views/login.html
        modified:   app/views/memos.html
        modified:   app/views/tutorial/a1.html
        modified:   app/views/tutorial/a6.html
        modified:   app/views/tutorial/a9.html
        modified:   app/views/tutorial/redos.html
        modified:   test/security/profile-test.js

[binarymist]

I think they can be commited, readme says "Please resolve all jsHint errors before committing the code." So profile-test.js aside, I think so. Personally I disagree with some of the linting decisions, but that's not really my call, and I respect @ckarande decision on this.

[lirantal]

Updated

[binarymist]

Why so much duplication (DRY principle) in session.js? handleApiLoginRequest appears to be a duplicate of handleLoginRequest. Was the generic errorMessage missed? Is there any reason we're not using const instead of var?

[lirantal]

@binarymist I did that as a quick demo to explain what I meant in my question on #90.
It's obviously far from then, there's no documentation added and nothing else, it's plainly to give a working example of what I want to push in.

If you guys vote up for this added so we can also demo classic nosql injections with mongodb's operators then I'll work on this PR further.

[ckarande]

@lirantal Thanks for the PR demonstrating the concept. As I commented on #90 I think this example would be a great addition to NodeGoat. I would prefer to incorporate it in existing login page instead of another page or API.

@binarymist while creating the jshint config, I picked up the rules just to have code consistency, and I do not any hard preferences about these rule. If we have to change it now, we will have to fix all the code, but I am open for any suggestions or changes if you think any alternate rules are more popular or make coding easier. So feel free to share your thoughts on it.

[binarymist]

In full agreement @ckarande . There are probably other rules that are closer to main stream js development, standardisation is the most important as you say. Things like 4 spaces for a tab seems more than I'm used to, there are arguments against, and probably some for. There are a few other things I'd change, if I was the only contributor, but I'm not, and it doesn't (nor should it) upset me enough to kick up a stink.

[binarymist]

Any movement on this one @lirantal ?

[lirantal]

@binarymist I'm still thinking about the best solution to take.
I see some value in the current login implementation with regards to the bcrypt() method, which if we replace with my proposal of the classic User.find() nosql injection then we will miss it.

That's why it lead me to discuss about another 'dummy' login by simulating an API end-point.

Let me know if you have other ideas how to keep both functionalities, but right now I'm still trying to figure out how we can win both worlds.

[binarymist]

@ckarande Any ideas on how you would like to "incorporate it in existing login page instead of another page or API."?

[ckarande]

@lirantal @binarymist Sorry for the delay on getting back on it. I was thinking of proposing a totally new feature (e.g portfolio performance) with a new screen to incorporate the MongoDB injection. However, I wanted to hold off on these changes as new OWASP Top 10 version seems right around the corner. We will need major screen changes to accommodate these new changes. So let's get back to it once we plan for these as part of #99

[lirantal]

Sure. Let's keep this one open @ckarande so we can re-visit this missing proof of concept.

[UlisesGascon]

@lirantal @ckarande maybe we can solve the conflicts and extend it with one test for this scenario using cypress? What do you think? :-)

[lirantal]

I always use this example when I demo NodeGoat but it was pointed out that maybe we should find a different example of this case of SQL Injection. Do you have a different idea or do you think it's ok to merge this if we resolve conflicts?

[UlisesGascon]

@lirantal I will re-integrate this change once #187 has been merged

[lirantal]

Sounds wonderful, thanks @UlisesGascon 🤗