Skip to content

[HttpFoundation] Prevent accepted rate limits with no remaining token to be preferred over denied ones #47283

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.

Sign up for GitHub

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

Jump to bottom
Merged
fabpot merged 1 commit into from
Aug 19, 2022
Merged

[HttpFoundation] Prevent accepted rate limits with no remaining token to be preferred over denied ones #47283

fabpot merged 1 commit into from
Aug 19, 2022
+117 −4

Conversation

@MatTheCat
Copy link
Contributor

@MatTheCat MatTheCat commented Aug 15, 2022 •
edited
Loading

Q A
Branch? 5.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #47282
License MIT
Doc PR N/A

When a rate limit is first accepted with no remaining token, following rejected rate limits will be ignored because they also will have no remaining token. The request would then be processed.

@carsonbot carsonbot added this to the 5.4 milestone Aug 15, 2022
wouterj
wouterj requested changes Aug 15, 2022
View reviewed changes
Copy link
Member

@wouterj wouterj left a comment

Choose a reason for hiding this comment

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

Thanks for the blazing fast bug fix, @MatTheCat!

Can you please add a test case for this fix, so we can be sure not to break it in the future?

@wouterj wouterj linked an issue Aug 15, 2022 that may be closed by this pull request
[Security] login_throttling un-blocking for *one* attempt at "5 * max_attempts" #47282
Closed
@MatTheCat
Copy link
Contributor Author

MatTheCat commented Aug 15, 2022 •
edited
Loading

@wouterj thanks! What should I do to make my test pass? Seems my changes are missing from them 🤔

@MatTheCat MatTheCat requested a review from chalasr as a code owner August 16, 2022 08:27
@MatTheCat MatTheCat force-pushed the ticket_47282 branch 2 times, most recently from 751caf4 to d07c9ea Compare August 16, 2022 12:03
@wouterj
Copy link
Member

wouterj commented Aug 16, 2022

See https://symfony.com/doc/current/contributing/code/pull_requests.html#automated-tests for more information about our test suite.

The failing checks are checking each individual package, rather than the monorepo. So it'll download rate limiter in the current available version, rather than the version of your PR.

Ideally, we would have a test for this abstract class' behavior in the HttpFoundation component itself. If you have some time, it would be great to create a MockRequestRateLimiter and add this specific test case there (based on the test that you've added).

@MatTheCat MatTheCat force-pushed the ticket_47282 branch 3 times, most recently from 9ef5695 to 311c53b Compare August 16, 2022 13:46
@MatTheCat
Copy link
Contributor Author

MatTheCat commented Aug 16, 2022

Okay, remaining failures seem unrelated 👌

wouterj
wouterj approved these changes Aug 18, 2022
View reviewed changes
Copy link
Member

@wouterj wouterj left a comment

Choose a reason for hiding this comment

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

Looking good!

@fabpot
Copy link
Member

fabpot commented Aug 19, 2022

Thank you @MatTheCat.

@fabpot fabpot merged commit 5294748 into symfony:5.4 Aug 19, 2022
@MatTheCat MatTheCat deleted the ticket_47282 branch August 19, 2022 08:06
@fabpot fabpot mentioned this pull request Aug 26, 2022
Merged
This was referenced Aug 26, 2022
Merged
Merged
@fabpot fabpot mentioned this pull request Sep 30, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wouterj wouterj wouterj approved these changes

@chalasr chalasr Awaiting requested review from chalasr

Assignees

No one assigned

Labels

Bug HttpFoundation Status: Reviewed

Projects

None yet

Milestone

5.4

Development

Successfully merging this pull request may close these issues.

[Security] login_throttling un-blocking for *one* attempt at "5 * max_attempts"

4 participants

@MatTheCat @wouterj @fabpot @carsonbot