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 security issue #15

Open
felipekk opened this issue Oct 3, 2014 · 4 comments
Open

Possible security issue #15

felipekk opened this issue Oct 3, 2014 · 4 comments

Comments

@felipekk
Copy link

felipekk commented Oct 3, 2014

Hello,

I'm adding your gem to my app, but when going through the code, I've noticed that you use this to locate an existing user record:
existing_user = current_user || User.where('email = ?', auth_params['info']['email']).first

This assumes that the email address that the provider sent you can be trusted. Couldn't a user use this to hijack another account?

Say I have a regular user Alice. She registered through my website and is not using OAuth. Bob is a hacker and he wants to be able to use Alice's account on my website. He knows her email address. He knows she doesn't have Facebook, and that you can login to the website using Facebook. So he creates a fake Facebook account using Alice's email address, and uses that Facebook account to login to Alice's account on my website. Isn't this possible?

Of course, it may seem an edge case (Facebook probably requires email verification), but since this gem supposedly supports any OAuth provider...

One possible solution to this is to require that the user be logged in before allowing him to login to an existing account with a "new" provider.

@arjanfrans
Copy link

This is was I did:

authentication = provider.user_authentications.where(uid: auth_params.uid).first
existing_user = current_user || (authentication.nil? ? nil : User.where(id: authentication.user_id).first)

This basically checks if there is a UserAuthentication record between the user and the provider account.

@xuanwu
Copy link

xuanwu commented Oct 29, 2014

Should the second line say User.where(id: authentication.user_id).first instead of User.where(authentication.id).first ?

@arjanfrans
Copy link

Yes you are right, I changed it.

@xuanwu
Copy link

xuanwu commented Nov 2, 2014

Cool thanks!

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

No branches or pull requests

3 participants