Problem/Motivation

This module has a rate-limiting / flood-control vulnerability in the PasswordReset DataProducer.

Summary

PasswordReset DataProducer (src/Plugin/GraphQL/DataProducer/User/PasswordReset.php) delegates to Drupal core's UserAuthenticationController::resetPassword() with a comment stating the client IP is preserved "for flood control." However, UserAuthenticationController::resetPassword() does not enforce flood control - only login() calls floodControl(). The HTML form equivalent (UserPasswordForm) does implement flood protection, but neither the API controller nor the
DataProducer does.

This allows unlimited password reset emails to be sent to any user account via the GraphQL endpoint.

It was decided by the Drupal security team that this can be posted as public security improvement and does not need a private security issue.

Steps to reproduce

  1. Install Drupal 11 with drupal/graphql:5.x-dev
  2. Create a custom schema extension that wires the password_reset data producer to a passwordReset mutation
  3. Run repeated requests:

    for i in $(seq 1 100); do
      curl -s -X POST https://your-site.com/graphql
        -H 'Content-Type: application/json'
        -d '{"query":"mutation { passwordReset(email: "victim@example.com") { ... on PasswordResetSuccess { message } } }"}'
    done
    
  4. All 100 requests succeed - no rate limiting is applied

Impact

  • Inbox flooding of any user account
  • Email provider quota exhaustion (SendGrid, Mailgun, etc.)
  • The misleading comment in the code ("needed for flood control") prevents discovery of the issue

Proposed fix

Inject UserFloodControlInterface and check flood limits before delegating to the controller, using Drupal's user.flood configuration.

Note

A graphql_test_auth module with GraphQL extension that uses PasswordReset DataProducer is attached

CommentFileSizeAuthor
graphql_test_auth.zip3.29 KBtibezh

Issue fork graphql-3587790

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

tibezh created an issue. See original summary.

tibezh’s picture

Assigned: tibezh » Unassigned
Status: Needs work » Needs review

Merge request has been added
Needs review

klausi’s picture

Issue summary: View changes
klausi’s picture

kingdutch made their first commit to this issue’s fork.

kingdutch’s picture

Status: Needs review » Needs work

Pushed a merge to get the tests moving.

This needs work. Klausi's feedback was marked as resolved but from what I can tell it was not addressed since we're still telling attackers why their flood was blocked:

In the Drupal core issue I was not sure if we should show a violation to attackers?

For security reasons I think it is better to always show a success to users and log the error to the logs instead.

Beyond that it looks good.

klausi’s picture

After thinking about it I think it is fine to show a specific error message to users (and attackers) as we have it now. I don't see the big security benefit of hiding that and it creates confusion in testing scenarios when it does not send emails.

I also think it is fine to re-use the user flood control settings, good enough as we documented in the comments.

Remaining steps:
* Fix phpunit tests fails
* Add new test case to trigger the flood control

tibezh’s picture

Status: Needs work » Needs review

Tests are fixed, added a testFloodControlBlocksRepeatedAttempts() test case.

klausi’s picture

Status: Needs review » Fixed

Thanks, merge request looks good!

I added it to the merge train, will be pushed soon.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

  • klausi committed 2feb53ec on 5.x authored by tibezh
    fix(PasswordReset): Added flood control for Password Reset Data Producer...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.