Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove in query user api the unencrypted password exposed #232

Closed
glutengo opened this issue May 31, 2021 · 6 comments · Fixed by #233
Closed

Remove in query user api the unencrypted password exposed #232

glutengo opened this issue May 31, 2021 · 6 comments · Fixed by #233
Assignees
Labels
enhancement New feature or request v2.0.0 Stable version with jhipster 7.0.0
Milestone

Comments

@glutengo
Copy link
Contributor

Describe the bug
At numerous places in the application, the value of the password property of the user entity are delivered in plain text. This affects (incomplete):

  • GET /api/admin/users
  • GET /api/account

The password should not be readable to anyone, not even the admin. The password should be at least encrypted, but it would be better if it was not included in the serialized user object at all.

To Reproduce
Steps to reproduce the behavior:

  1. Generate a new app via nhipster
  2. Select the default option for every question
  3. Login as admin
  4. Go to User Management
  5. Open Dev Tools Check the returned data for the listed endpoints

Expected behavior
The password is not part of the serialized user object.

Screenshots
Screenshot 2021-05-31 at 10 58 56

Desktop (please complete the following information):

  • OS: iOS 11.3
  • NHipster Version 2.0.0-beta.1

NHipster configuration

Additional context
I discovered the problem while working on #214

@glutengo
Copy link
Contributor Author

I would suggest preventing this by not reading the password from the database as suggested here: https://stackoverflow.com/a/62957519/2202290

However I also noticed that the password is encrypted (rather than hashed). This is not considered best practise because the administrators / developers / ... could steal the password of the users by decrypting the password. Background Information: https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html#background

glutengo added a commit to glutengo/generator-jhipster-nodejs that referenced this issue May 31, 2021
@ghost
Copy link

ghost commented May 31, 2021

Hi @glutengo, thanks for the suggestion!
So, the encryption field is performed automatically by TypeORM. According to you, maybe we could delete password from the object returned by controller GET apis.

@glutengo
Copy link
Contributor Author

I think it would be good to split it into two issues:
The first (this one) handles what is returned by the API. If we do not read the password from the database at all, it will never be included in any response of the controllers. My PR handles this.

The second issue would be about password encryption / hashing. As stated above, I think it would be better if the password was hashed so that there is no way to decrypt the password.

@ghost
Copy link

ghost commented May 31, 2021

Ok, it's ok the first PR, you can open a new issue for the second point.

@ghost ghost changed the title Unencrpyted password is exposed Unencrypted password is exposed May 31, 2021
@ghost ghost linked a pull request May 31, 2021 that will close this issue
@ghost ghost assigned glutengo May 31, 2021
@ghost ghost added the enhancement New feature or request label May 31, 2021
@ghost ghost added this to To do in nodejs blueprint via automation May 31, 2021
@ghost ghost added this to the 2.0.0 milestone May 31, 2021
@ghost ghost added the v2.0.0 Stable version with jhipster 7.0.0 label May 31, 2021
@ghost ghost moved this from To do to In progress in nodejs blueprint May 31, 2021
@ghost ghost moved this from In progress to Review in progress in nodejs blueprint May 31, 2021
@ghost ghost changed the title Unencrypted password is exposed Remove in query user api the unencrypted password exposed May 31, 2021
@gmarziou
Copy link
Contributor

gmarziou commented May 31, 2021

Maybe you should consider following some practices from JHipster java:

@glutengo
Copy link
Contributor Author

glutengo commented May 31, 2021

@gmarziou I agree, the bcrypt library is the standard for storing passwords in Node.js web apps too. I have created issue #234 for that.

@ghost ghost closed this as completed in #233 Jun 2, 2021
nodejs blueprint automation moved this from Review in progress to Done Jun 2, 2021
ghost pushed a commit that referenced this issue Jun 2, 2021
Do not read user password from DB closes #232
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request v2.0.0 Stable version with jhipster 7.0.0
Projects
Development

Successfully merging a pull request may close this issue.

2 participants