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

Fix User fields #78

Merged
merged 3 commits into from
Oct 13, 2021
Merged

Fix User fields #78

merged 3 commits into from
Oct 13, 2021

Conversation

qgerome
Copy link
Contributor

@qgerome qgerome commented Sep 29, 2021

No description provided.

@qgerome qgerome changed the base branch from main to develop September 29, 2021 15:11
@lgtm-com
Copy link

lgtm-com bot commented Sep 29, 2021

This pull request introduces 5 alerts and fixes 6 when merging c172b35 into d02ac16 - view on LGTM.com

new alerts:

  • 3 for Clear-text logging of sensitive information
  • 2 for Use of a broken or weak cryptographic hashing algorithm on sensitive data

fixed alerts:

  • 3 for Unused local variable
  • 3 for Unused import

Copy link
Member

@edarchis edarchis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be merged into the current release but I'll do some more testing.

LGTM complains about logging of sensitive data: it is the userid/username. It only occurs when there is a failed attempt at changing the username. So I'll stand by the need to log this information.
It also complains about the password hashing function that is not complex enough. It's a SHA256, which is already good but libs like bcrypt will apply it a number of times to make the computation a lot harder. Given that we are still in hybrid mode, we have to maintain the compatibility with the legacy .NET and therefore maintain this hashing. The records are all salted with very long salts, so it's too much of a problem.

@edarchis edarchis merged commit a58a7d8 into develop Oct 13, 2021
@edarchis edarchis deleted the user branch October 13, 2021 19:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants