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

Disallow confirmation token reuse #98

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

mdchaney
Copy link

This fairly complex and kind of ugly change fixes the token reuse issue for email confirmation.

Asserts cookie is http_only, secure, and same-site is "strict".
Closes stevepolitodesign#87.
Closes stevepolitodesign#86.

The issue is that signed tokens have a simple payload by default that
doesn't verify anything other than the record id and the token's
purpose.  This can lead to a security challenge as the token can be used
to confirm anything that has the same "purpose" for that record.

An example would be someone changing their email address to an email
address that they don't control.  Using one account, they change the
email to an email address that they control and get the confirmation
token.  They then change the email to one that they can't access and
use the token from the first request to "confirm" the second request.
The tokens can be used any number of times as long as they're used
before expiration.

With this change, the email address is included as plain text in the
request, as well as being used as part of the "purpose" in the token.
The second request fails because the plain text email address is used to
constrain the signed lookup.  If they change the plain text email
address in the link then the message will fail to be validated as the
"purpose" won't match.  Either way, the token is usable only for
confirming the original email from the token's creation.
@stevepolitodesign
Copy link
Owner

I wonder if we could use generates_token_for for this?

@mdchaney
Copy link
Author

I didn't notice generates_token_for before. That's essentially the same thing, although oddly it seems to be taking a different code path. I'm not using the confirmation functionality right now but might revisit this when I have time.

It looks like the failing test is due to me allowing another change into this PR. Need to work on this.

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.

None yet

2 participants