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

Add functionality to mark tokens as claimed to prevent re-use #51

Merged
merged 3 commits into from
Apr 30, 2019

Conversation

JoeSouthan
Copy link
Contributor

Description

To help improve the security and one time nature of the links generated by passwordless, I’ve put together a configuration option to expire tokens after a login attempt.

Let me know what you think.

Copy link
Owner

@mikker mikker left a comment

Choose a reason for hiding this comment

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

Very nice! Thanks for doing this. I think this is something we could provide.

Just curious: Why are you worried about token reuse? How much more secure does this make anything? I don't know. Tokens are already expiring pretty quickly?

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
app/controllers/passwordless/sessions_controller.rb Outdated Show resolved Hide resolved
lib/passwordless/version.rb Outdated Show resolved Hide resolved
test/dummy/db/schema.rb Outdated Show resolved Hide resolved
@JoeSouthan
Copy link
Contributor Author

Hi @mikker! Thanks for having a look over the PR.

There is the possibility of a bad actor intercepting the token from the email and using it to initiate their own session, the token should immediately be claimed to prevent this. Expiry goes some way to prevent this, but if it's still within the expiry time you can log back in.

@JoeSouthan JoeSouthan force-pushed the joesouthan/expire-token-callback branch from 7ba51ba to 41107d7 Compare April 26, 2019 16:10
Since claimed_at no longer has constraints, there is no need for a HA migration
@JoeSouthan
Copy link
Contributor Author

@mikker I've removed the section on HA migrations as there are no column constraints/indices being created.

@mikker mikker merged commit 80d3e00 into mikker:master Apr 30, 2019
@mikker
Copy link
Owner

mikker commented Apr 30, 2019

💙💚💛💜❤️

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.

3 participants