Skip to content

Support rate limit allow lists#18864

Merged
dschom merged 1 commit intomainfrom
support-rate-limit-allow-lists
May 21, 2025
Merged

Support rate limit allow lists#18864
dschom merged 1 commit intomainfrom
support-rate-limit-allow-lists

Conversation

@dschom
Copy link
Copy Markdown
Contributor

@dschom dschom commented May 14, 2025

Because

  • We want the ability to skip customs checks for certain emails, ips, or uids.
  • This was supported in the previous version of customs, and useful for development or smoke testing.

This pull request

  • Adds ability to configure and ignore certain ips, email, uids
  • Wires this up in auth-server

Issue that this pull request solves

Closes: FXA-11663

Checklist

Put an x in the boxes that apply

  • My commit is GPG signed.
  • If applicable, I have modified or added tests which pass locally.
  • I have added necessary documentation (if appropriate).
  • I have verified that my changes render correctly in RTL (if appropriate).

Screenshots (Optional)

Please attach the screenshots of the changes made in case of change in user interface.

Other information (Optional)

Any other information that is important to this pull request.

@dschom dschom force-pushed the support-rate-limit-allow-lists branch 2 times, most recently from 20453e2 to e3fd6d9 Compare May 20, 2025 18:18
@dschom dschom marked this pull request as ready for review May 20, 2025 18:20
@dschom dschom requested a review from a team as a code owner May 20, 2025 18:20
opts.ip ? 'ip':'',
opts.email ? 'email':'',
opts.uid ? 'uid':'',
]);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This can cause a cardinality issue, depending on the configuration and environment where this is used. E.g. if the email pattern is 'restmail'. Do you think we'd want to know the specific ip/email/uid skipped?

Copy link
Copy Markdown
Contributor Author

@dschom dschom May 21, 2025

Choose a reason for hiding this comment

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

We are not passing in the IP, email or uid values themselves. Let's say all three opts were provided. The values passed in would be, ['email','ip','uid']. I don't think this would cause a cardinality problem.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh I totally misread that. 👍

rateLimit = new RateLimit(
{
rules: {},
ignoreEmails: ['@mozilla.com$', 'foo@firefox.com'],
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I always feel a bit nervous using regex in these types of filters. Mostly because of the chance to mess them up and allow unintended things. Should we do this based on specific domains or explict emails?

Copy link
Copy Markdown
Contributor Author

@dschom dschom May 21, 2025

Choose a reason for hiding this comment

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

I think this is how it worked previously. I do understand this concern though... There are some cases where it makes sense to filter by domain, and not explicitly by email address. Let me file a polish PR where we can do this by ignoreEmails and ignoreEmailDomains. For now, or rules would be very regexes would be similar to the examples above, so I'm not too worried about it.

Also, just a side note, but the more I think about how these filters work, I think using the ip filters is the way to go.

@dschom dschom merged commit 1e1c39b into main May 21, 2025
19 checks passed
@dschom dschom deleted the support-rate-limit-allow-lists branch May 21, 2025 15:41
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