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

Possible issue with sequalize logout function - security issue? #76

Closed
craigharman opened this issue Mar 21, 2021 · 2 comments · Fixed by #81
Closed

Possible issue with sequalize logout function - security issue? #76

craigharman opened this issue Mar 21, 2021 · 2 comments · Fixed by #81
Assignees
Labels
🐞 Bug Something isn't working

Comments

@craigharman
Copy link

Describe the Bug(버그 설명)

On line 44 of the uth.service.ts file in the sequalize folder I have noticed that you search for a user only by their password. I am not sure what the user being returned is used for (as they are being logged out) but it seems to me that you could return an incorrect user if 2 users were to use the same password. Suggest changing this to search for user by a unique field or email and password combination?

Version to Reproduce(현재 사용한 버전)

Any environment.

Steps to Reproduce(재현 순서)

  1. Create a user with password "password"
  2. Create a second user with password "password"
  3. Login as the second user
  4. Log out
  5. You will be returned user from step 1 above when you were logged in as user from step 2!

Expected Behavior(예상 동작)

Should return the user you are logged in as.

Actual Behavior(실제 동작)

A different user (with the same password) is returned.

Additional Context(추가 사항)

This same technique seems to be used in TypeORM and mongoose as well. If it is an issue then it probably needs changing in all these.

Capture screen(캡쳐 화면)

@craigharman craigharman added the 🐞 Bug Something isn't working label Mar 21, 2021
@ljlm0402
Copy link
Owner

ljlm0402 commented Apr 7, 2021

@craigharman

Sorry for the late reply. We will respond promptly after confirmation.

@ljlm0402 ljlm0402 linked a pull request Apr 18, 2021 that will close this issue
3 tasks
@ljlm0402
Copy link
Owner

@craigharman

Sorry for the late response.
In order to secure the case of using the same password mentioned first, the e-mail has also been changed through the and clause.

However, I don't think that it is very dangerous for security because the account information stored in the existing cookie or header is verified through auth.middleware during the logout process.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐞 Bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants