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

Adds an option to strictly enforce single recipients for emails #5680

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nickmalcolm
Copy link

Devise sends email containing sensitive values such as confirmation URLs, password reset URLs, and unlock URLs. In most (all?) cases, these should only be sent to a single person so that they alone can click the link. If the email is sent to multiple addresses, another person could click the link.

Set Devise.strict_single_recipient_emails to an array of actions to raise an error when the email would be sent to more than one email address.

By default Devise is secure:

  • Devise.email_regexp will reject email addresses containing separators (,;)
  • Devise gets a single email address from record.email

However, when using opts, and particularly if providing untrusted user input to opts, multiple values could be present in to:, cc:, or bcc:.

Example:

# POST https://your-app.com/customised-reset-password?email[]="[email protected]"&email[]="[email protected]"

# Returns the victim's user
user = User.find_by(email: params[:email])

# unsafe, will send the link to two addresses: 
Devise.mailer.reset_password_instructions(user, 'fake-token', {to: params[:email]})

# safe, devise will use the user's email address
Devise.reset_password_instructions(user, 'fake-token')

# safe, will raise error:
Devise.strict_single_recipient_emails = [
  :confirmation_instructions,
  :reset_password_instructions,
  :unlock_instructions
]
Devise.mailer.reset_password_instructions(user, 'fake-token', {to: params[:email]})

This work is similar to what I introduced at GitLab, but disabled by default and more configurable:

a) to avoid breaking changes,
b) to make it easier to enable for a subset of actions

GitLab MR: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/145753

This is my first contribution to Devise - very happy to receive feedback and change things up as needed ❤️ Also fine if you'd rather not include this change 👍

Devise sends email containing sensitive values such as confirmation
URLs, password reset URLs, and unlock URLs. In most (all?) cases, these
should only be sent to a single person so that they alone can click the
link. If the email is sent to multiple addresses, another person could
click the link.

Set `Devise.strict_single_recipient_emails` to an array of actions to
raise an error when the email would be sent to more than one email
address.

By default Devise is secure:

- `Devise.email_regexp` will reject email addresses containing
separators (`,;`)
- Devise gets a single email address from `record.email`

However, when using `opts`, and particularly if providing untrusted
user input to `opts`, multiple values could be present in `to:`, `cc:`,
or `bcc:`.

Example:

```ruby
# POST https://your-app.com/customised-reset-password?email[]="[email protected]"&email[]="[email protected]"

# Returns the victim's user
user = User.find_by(email: params[:email])

# unsafe, will send the link to two addresses:
Devise.mailer.reset_password_instructions(user, 'fake-token', {to: params[:email]})

# safe, devise will use the user's email address
Devise.reset_password_instructions(user, 'fake-token')

# safe, will raise error:
Devise.strict_single_recipient_emails = [
  :confirmation_instructions,
  :reset_password_instructions,
  :unlock_instructions
]
Devise.mailer.reset_password_instructions(user, 'fake-token', {to: params[:email]})
```
@nickmalcolm
Copy link
Author

This is ready for review @carlosantoniodasilva 🙇

@nickmalcolm
Copy link
Author

👋 @carlosantoniodasilva do you or another contributor have capacity to review this? If it's not a contribution that's a good fit, I can close it 👍

Thanks for all the time & effort you put in to devise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

1 participant