⚓ T381033 Rate limiting on 'badoath' usi

⚓ T381033 Rate limiting on 'badoath' using $wgRateLimits doesn't work
Page Menu
Phabricator
Create Task
Maniphest
T381033
Rate limiting on 'badoath' using $wgRateLimits doesn't work
Closed, Resolved
Public
Security
Actions
Edit Task
Edit Related Tasks...
Create Subtask
Edit Parent Tasks
Edit Subtasks
Merge Duplicates In
Close As Duplicate
Edit Related Objects...
Edit Commits
Edit Mocks
Mute Notifications
Protect as security issue
Assigned To
mmartorana
Authored By
Mprep
Nov 27 2024, 7:57 PM
2024-11-27 19:57:33 (UTC+0)
Tags
MediaWiki-extensions-OATHAuth
(Backlog)
Security
Security-Team
(Incoming)
MediaWiki-User-management
(Backlog)
ConfirmEdit (CAPTCHA extension)
(Bugs)
MediaWiki-Engineering
(MW Platform Team)
MediaWiki-Platform-Team (Radar)
MW-1.39-release
(Blocker)
MW-1.42-release
(Blocker)
MW-1.43-release
(Blocker)
Patch-For-Review
MW-1.39-notes
MW-1.42-notes
MW-1.43-notes
MW-1.44-notes (1.44.0-wmf.19; 2025-03-04)
Referenced Files
F58358558: T381033.patch
Feb 5 2025, 9:59 AM
2025-02-05 09:59:19 (UTC+0)
F58317257: Screenshot 2025-01-30 at 16.00.31.png
Jan 30 2025, 4:02 PM
2025-01-30 16:02:00 (UTC+0)
Subscribers
acooper
Aklapper
daniel
gerritbot
JTweed-WMF
larissagaulia
mmartorana
View All 12 Subscribers
Description
Steps to replicate the issue
Set up a MediaWiki installation with OATHAuth installed.
Add the following lines of code to
LocalSettings.php
$wgRateLimits
'badoath'
][
'&can-bypass'
false
$wgRateLimits
'badoath'
][
'user'
60
];
$wgRateLimits
'badoath'
][
'user-global'
60
];
or
$wgRateLimits
'badoath'
][
'&can-bypass'
false
$wgRateLimits
'badoath'
][
'ip-all'
60
];
Just in case, restart the web server and whatever is running the PHP for the web server (e.g. PHP FPM).
Create a new wiki user via the web interface or via the
createAndPromote
maintenance command.
Set up 2FA for the new user.
Log out.
Log back in.
When prompted, enter and submit an incorrect 2FA code multiple times.
Enter and submit a correct 2FA code.
What happens?
You're fully logged into your account.
What should have happened instead?
You're rate limited at step 8 for 60 seconds.
Software version
(on
Special:Version
page; skip for WMF-hosted wikis like Wikipedia):
Product
Version
MediaWiki
1.42.3
PHP
8.3.6 (fpm-fcgi)
ICU
74.2
MariaDB
10.11.8-MariaDB-0ubuntu0.24.04.1
Pygments
2.17.2
OATHAuth extension's version is 0.5.0. The web server is nginx version 1.24.0.
Other information
(browser name/version, screenshots, etc.):
Issue was observed both on a pre-existing wiki running behind Cloudflare as well as on a fresh (completely separate VPS; MediaWiki downloaded, installed and configured as instructed by
MediaWiki.org itself
) newly spun up wiki set up via the web interface (aforementioned software version info identical on both wikis) with nothing (including no other extensions than OATHAuth) additional installed or modified (excluding the issue reproduction steps) accessed directly via the server's IP. Both wikis use PHP's object cache as the MediaWiki cache and have PHP APCu installed.
Details
Related Changes in Gerrit:
Subject
Repo
Branch
Lines +/-
RateLimiter: Fix peek mode
mediawiki/core
REL1_43
+40
-2
RateLimiter: Fix peek mode
mediawiki/core
REL1_42
+42
-2
RateLimiter: Fix peek mode
mediawiki/core
REL1_39
+42
-2
RateLimiter: Fix peek mode
mediawiki/core
master
+40
-2
Customize query in gerrit
Related Objects
Mentions
Mentioned Here
rECOE7559502822f5: Add RateLimit check for false CAPTCHAs
rECOE31c59374a4b1: Add AuthManager support to SimpleCaptcha, QuestyCaptcha, FancyCaptcha…
rMWa8ee61d9d682: Implement rate limiting in Authority.
T48292: ConfirmEdit should block IPs after a set number of failed CAPTCHA attempts
T110302: Update ConfirmEdit to use AuthManager
T261963: Spike to explore Authority concept, implementation, and migration
T310476: Add rate limiting to Authority
rEOATa6b60d246501: Apply rate limits to all token verifications
Event Timeline
Mprep
created this task.
Nov 27 2024, 7:57 PM
2024-11-27 19:57:33 (UTC+0)
Restricted Application
added a subscriber:
Aklapper
View Herald Transcript
Nov 27 2024, 7:57 PM
2024-11-27 19:57:36 (UTC+0)
Mprep
set Security to Software security bug.
Nov 27 2024, 8:23 PM
2024-11-27 20:23:40 (UTC+0)
Mprep
added projects:
Security
Security-Team
Mprep
changed the visibility from "Public (No Login Required)" to "
Custom Policy
".
Mprep
changed the subtype of this task from "Bug Report" to "Security Issue".
Comment Actions
Not sure if this counts as a security issue but escalating it just in case
sbassett
added subscribers:
mmartorana
sbassett
Dec 2 2024, 5:43 PM
2024-12-02 17:43:46 (UTC+0)
Comment Actions
In
T381033#10363653
@Mprep
wrote:
Not sure if this counts as a security issue but escalating it just in case
There's never a problem erring on the side of caution. We can always make the task public if it's low/no risk.
As for the issue, it looks like
rate-limiting was implemented in ext:OATHAuth
via MW's ping-limiting functionality, but I noticed some comments saying they aren't increasing the limiter under certain code paths. Not sure if that could have unintended consequences. We also don't appear to have the
badoath
rate-limiting enabled in any way in Wikimedia production, so it not working as expected may have gone unnoticed. Anyhow,
@mmartorana
will investigate a bit.
mmartorana
changed the task status from
Open
to
In Progress
Dec 9 2024, 3:47 PM
2024-12-09 15:47:06 (UTC+0)
mmartorana
triaged this task as
Low
priority.
acooper
added a subscriber:
Reedy
Dec 16 2024, 5:12 PM
2024-12-16 17:12:29 (UTC+0)
acooper
subscribed.
Reedy
added a comment.
Jan 29 2025, 3:50 PM
2025-01-29 15:50:22 (UTC+0)
Comment Actions
For WMF production:
reedy@deploy2002:~$ mwscript eval.php enwiki
DEPRECATION WARNING: Maintenance scripts are moving to Kubernetes. See
Maintenance hosts will be going away; please submit feedback promptly if
maintenance scripts on Kubernetes don't work for you. (T341553)
> var_dump( $wgRateLimits['badoath'] );
array(3) {
["&can-bypass"]=>
bool(false)
["user"]=>
array(2) {
[0]=>
int(10)
[1]=>
int(60)
["user-global"]=>
array(2) {
[0]=>
int(10)
[1]=>
int(60)
Reedy
added a comment.
Edited
Jan 29 2025, 3:56 PM
2025-01-29 15:56:05 (UTC+0)
Comment Actions
Rate limiting was implemented in
rEOATa6b60d246501: Apply rate limits to all token verifications
Obviously the code has been re-factored quite a lot since then...
Reedy
added a comment.
Jan 29 2025, 4:04 PM
2025-01-29 16:04:11 (UTC+0)
Comment Actions
@Mprep
Does any rate limiting actually work on your wiki(s)?
What is
$wgMainCacheType
set to?
Mprep
added a comment.
Edited
Jan 30 2025, 8:17 AM
2025-01-30 08:17:07 (UTC+0)
Comment Actions
In
T381033#10505109
@Reedy
wrote:
@Mprep
Does any rate limiting actually work on your wiki(s)?
Partially, it seems. I've added the following line to
LocalSettings.php
to test it:
$wgRateLimits['edit']['user'] = [ 1, 86400 ];
and it did ratelimit me after a single edit.
As an anti-abuse measure, you are limited from performing this action too many times in a short space of time, and you have exceeded this limit. Please try again in a few minutes.
However, weirdly enough, when I tried ratelimiting based on ConfirmEdit's bad captcha completions (specifically hCaptcha), it doesn't seem to work:
$wgRateLimits['badcaptcha']['ip-all'] = [ 1, 86400 ];
Neither editing nor creating accounts (both of which are restricted by captcha) seem to be ratelimited (failing the captcha doesn't prevent you from submitting an edit or creating a new account if you complete the captcha correctly next time). Adding
$wgRateLimits['badcaptcha']['&can-bypass'] = false;
doesn't help either. Could it be that the ratelimiting issues are something related to extensions using the feature?
I've only done these ratelimit tests on the pre-existing wiki running behind Cloudflare, but if it would help, I could yet again spin up a fresh new wiki on a separate VPS to test if the same thing occurs there.
In
T381033#10505109
@Reedy
wrote:
What is
$wgMainCacheType
set to?
$wgMainCacheType = CACHE_ACCEL;
Mprep
added a comment.
Jan 30 2025, 8:20 AM
2025-01-30 08:20:46 (UTC+0)
Comment Actions
In
T381033#10505071
@Reedy
wrote:
For WMF production:
reedy@deploy2002:~$ mwscript eval.php enwiki
DEPRECATION WARNING: Maintenance scripts are moving to Kubernetes. See
Maintenance hosts will be going away; please submit feedback promptly if
maintenance scripts on Kubernetes don't work for you. (T341553)
> var_dump( $wgRateLimits['badoath'] );
array(3) {
["&can-bypass"]=>
bool(false)
["user"]=>
array(2) {
[0]=>
int(10)
[1]=>
int(60)
["user-global"]=>
array(2) {
[0]=>
int(10)
[1]=>
int(60)
Results on my end when running it via
maintenance/run.php eval
> var_dump($wgRateLimits['badoath'])
array(3) {
["&can-bypass"]=>
bool(false)
["user"]=>
array(2) {
[0]=>
int(10)
[1]=>
int(60)
["user-global"]=>
array(2) {
[0]=>
int(10)
[1]=>
int(60)
Reedy
added a comment.
Jan 30 2025, 1:51 PM
2025-01-30 13:51:00 (UTC+0)
Comment Actions
To quote a colleague
[02:12:12]

> echo '$u = User::newFromName("KrinkleSock"); return $u->pingLimiter("badoath");' | mwscript eval.php enwiki -d 1
[02:12:25]

Repeating that 11 times from mwmaint, results in it tripping
If I set the config you suggest...
wfLoadExtension
'OATHAuth'
);
//$wgOATHExclusiveRights[] = 'edit';
//$wgOATHAuthMultipleDevicesMigrationStage = SCHEMA_COMPAT_READ_OLD | SCHEMA_COMPAT_WRITE_BOTH;
$wgRateLimits
'badoath'
][
'&can-bypass'
false
$wgRateLimits
'badoath'
][
'user'
60
];
$wgRateLimits
'badoath'
][
'user-global'
60
];
It shows up fine in my running dev wiki (with rate limit changes turned off from DevelopmentSettings in my case)
> var_dump($wgRateLimits['badoath']);
/var/www/wiki/mediawiki/core/maintenance/eval.php(132) : eval()'d code:1:
array(3) {
'&can-bypass' =>
bool(false)
'user' =>
array(2) {
[0] =>
int(1)
[1] =>
int(60)
'user-global' =>
array(2) {
[0] =>
int(1)
[1] =>
int(60)
Mprep
added a comment.
Jan 30 2025, 3:43 PM
2025-01-30 15:43:43 (UTC+0)
Comment Actions
In
T381033#10508293
@Reedy
wrote:
To quote a colleague
[02:12:12]

> echo '$u = User::newFromName("KrinkleSock"); return $u->pingLimiter("badoath");' | mwscript eval.php enwiki -d 1
[02:12:25]

Repeating that 11 times from mwmaint, results in it tripping
Can confirm that programmatically triggering the ratelimit does indeed work (trips ratelimit on the (N+1)th attempt, where N is the limit). Despite that, I can still reproduce the issue (being able to complete 2FA and login despite exceeding the ratelimit; running the same code right after still returns that the ratelimit was tripped).
In
T381033#10508293
@Reedy
wrote:
If I set the config you suggest...
wfLoadExtension
'OATHAuth'
);
//$wgOATHExclusiveRights[] = 'edit';
//$wgOATHAuthMultipleDevicesMigrationStage = SCHEMA_COMPAT_READ_OLD | SCHEMA_COMPAT_WRITE_BOTH;
$wgRateLimits
'badoath'
][
'&can-bypass'
false
$wgRateLimits
'badoath'
][
'user'
60
];
$wgRateLimits
'badoath'
][
'user-global'
60
];
It shows up fine in my running dev wiki (with rate limit changes turned off from DevelopmentSettings in my case)
> var_dump($wgRateLimits['badoath']);
/var/www/wiki/mediawiki/core/maintenance/eval.php(132) : eval()'d code:1:
array(3) {
'&can-bypass' =>
bool(false)
'user' =>
array(2) {
[0] =>
int(1)
[1] =>
int(60)
'user-global' =>
array(2) {
[0] =>
int(1)
[1] =>
int(60)
The config shows up fine for me as well, that wasn't the issue. By default the wiki 2FA ratelimit is set to 10. I've only set it to 1 temporarily (as well as in the freshly spun up wiki on a separate VPS) to ease testing. Repeating the initial steps to replicate the issue with 11+ failed attempts to complete 2FA results in the same outcome.
Reedy
added a comment.
Jan 30 2025, 4:02 PM
2025-01-30 16:02:00 (UTC+0)
Comment Actions
Ok, indeed, replicated.
2025-01-30 15:59:48 ubuntu64-web-esxi wikidb-mw_: MediaWiki\Permissions\RateLimiter::limit: limiting badoath rate for Reedy
2025-01-30 15:59:48 ubuntu64-web-esxi wikidb-mw_: MediaWiki\Permissions\RateLimiter::limit: limiting badoath rate for Reedy
2025-01-30 15:59:48 ubuntu64-web-esxi wikidb-mw_: User::pingLimiter: User tripped rate limit
2025-01-30 15:59:48 ubuntu64-web-esxi wikidb-mw_: User::pingLimiter: User tripped rate limit
2025-01-30 16:00:05 ubuntu64-web-esxi wikidb-mw_: MediaWiki\Permissions\RateLimiter::limit: limiting badoath rate for Reedy
2025-01-30 16:00:05 ubuntu64-web-esxi wikidb-mw_: MediaWiki\Permissions\RateLimiter::limit: limiting badoath rate for Reedy
2025-01-30 16:00:05 ubuntu64-web-esxi wikidb-mw_: User::pingLimiter: User tripped rate limit
2025-01-30 16:00:05 ubuntu64-web-esxi wikidb-mw_: User::pingLimiter: User tripped rate limit
It seems it's the
in
if ( $user->pingLimiter( 'badoath', 0 ) ) {
... If I swap that to
, I get it returning true to get an error:
Even though the log is saying it's tripped, it's not kicking out...
So that passing through of 0 is doing something... fun with the ratelimits
Reedy
raised the priority of this task from
Low
to
Needs Triage
Jan 30 2025, 4:02 PM
2025-01-30 16:02:50 (UTC+0)
Reedy
added a project:
MediaWiki-User-management
Reedy
added a comment.
Edited
Jan 30 2025, 4:06 PM
2025-01-30 16:06:14 (UTC+0)
Comment Actions
Confirmable on CLI too (I have it set to 1 in 600 seconds)
reedy@ubuntu64-web-esxi:/var/www/wiki/mediawiki/core$
echo
'$u = User::newFromName("Reedy"); return $u->pingLimiter("badoath",0);'
php maintenance/eval.php

*******************************************************************************
NOTE: Do not run maintenance scripts directly, use maintenance/run.php instead!
Running scripts directly has been deprecated in MediaWiki
.40.
It may not work
for
some
or any
scripts in the future.
*******************************************************************************

/var/www/wiki/mediawiki/core/maintenance/eval.php:151:
bool
false
reedy@ubuntu64-web-esxi:/var/www/wiki/mediawiki/core$
echo
'$u = User::newFromName("Reedy"); return $u->pingLimiter("badoath",0);'
php maintenance/eval.php

/var/www/wiki/mediawiki/core/maintenance/eval.php:151:
bool
true
Reedy
added a comment.
Jan 30 2025, 4:09 PM
2025-01-30 16:09:27 (UTC+0)
Comment Actions
If I test on a web browser on enwiki... I also don't get anything other than
Verification failed.
Reedy
added a project:
ConfirmEdit (CAPTCHA extension)
Jan 30 2025, 4:47 PM
2025-01-30 16:47:38 (UTC+0)
Comment Actions
It seems/feels like
pingLimiter( 'badoath', 0 )
doesn't actually do what the comment says it does/should -
// Don't increase pingLimiter, just check for limit exceeded.
Considering
ConfirmEdit (CAPTCHA extension)
does the same thing... That might explain why it's seemingly not working there either...
// don't increase pingLimiter here, just check, if CAPTCHA limit exceeded
if
$user
->
pingLimiter
'badcaptcha'
The MW core actions don't pass
to check...
Reedy
triaged this task as
High
priority.
Jan 30 2025, 4:56 PM
2025-01-30 16:56:43 (UTC+0)
Reedy
added a project:
MediaWiki-Engineering
acooper
added a comment.
Jan 31 2025, 2:54 PM
2025-01-31 14:54:16 (UTC+0)
Comment Actions
Just circling back on Scott's comment that the rate limit isn't configured in production, I chatted with
@Reedy
and its enabled by default without the config set.
mmartorana
added a comment.
Jan 31 2025, 5:13 PM
2025-01-31 17:13:18 (UTC+0)
Comment Actions
In
T381033#10509337
@Reedy
wrote:
It seems/feels like
pingLimiter( 'badoath', 0 )
doesn't actually do what the comment says it does/should -
// Don't increase pingLimiter, just check for limit exceeded.
Considering
ConfirmEdit (CAPTCHA extension)
does the same thing... That might explain why it's seemingly not working there either...
// don't increase pingLimiter here, just check, if CAPTCHA limit exceeded
if
$user
->
pingLimiter
'badcaptcha'
The MW core actions don't pass
to check...
Yes, exactly. I tested it locally by changing it to 1 in
TOTPSecondaryAuthenticationProvider.php
while using the same settings as
@Mprep
, and it worked. However, I’m unsure about potential side effects (i.e. double counting).
sbassett
added a comment.
Jan 31 2025, 5:20 PM
2025-01-31 17:20:26 (UTC+0)
Comment Actions
In
T381033#10512369
@acooper
wrote:
Just circling back on Scott's comment that the rate limit isn't configured in production, I chatted with
@Reedy
and its enabled by default without the config set.
Sure, though parts of its functionality appear to have been broken or working unexpectedly in Wikimedia production for some time.
acooper
added a comment.
Jan 31 2025, 5:30 PM
2025-01-31 17:30:24 (UTC+0)
Comment Actions
We're going to take some more time to understand the potential side effects of this issue and will resume next week.
mmartorana
added a comment.
Edited
Feb 5 2025, 9:59 AM
2025-02-05 09:59:19 (UTC+0)
Comment Actions
In
T381033#10509337
@Reedy
wrote:
It seems/feels like
pingLimiter( 'badoath', 0 )
doesn't actually do what the comment says it does/should -
// Don't increase pingLimiter, just check for limit exceeded.
Considering
ConfirmEdit (CAPTCHA extension)
does the same thing... That might explain why it's seemingly not working there either...
// don't increase pingLimiter here, just check, if CAPTCHA limit exceeded
if
$user
->
pingLimiter
'badcaptcha'
The MW core actions don't pass
to check...
The current issue is that
pingLimiter
isn’t being consistently tracked across new requests, likely due to caching, which prevents enforcement (it resets each time).
One potential solution is to introduce a persistent layer, such as storing the limit in a db to ensure it persists across requests or within the session.
Other authentication flows haven’t been tested yet, but this fix improves the core logic. If similar rate limiting inconsistencies exist in other areas (e.g., the API), they may also need adjustments.
For now, I’m proposing this patch as a starting point: I moved the counter increment before the rate limit check to ensure enforcement works. While the rate limiting is now being applied, there are still some issues/inconsistencies: specifically, we should likely reset the counter after a successful login, and also the first attempt is considered wrong even if it is not.
@Reedy
, what’s your take on this? Is there a standard approach for handling rate limits in MediaWiki that we should follow?
T381033.patch
1 KB
Mprep
added a comment.
Feb 5 2025, 9:30 PM
2025-02-05 21:30:33 (UTC+0)
Comment Actions
In
T381033#10524289
@mmartorana
wrote:
While the rate limiting is now being applied, there are still some issues/inconsistencies: specifically, we should likely reset the counter after a successful login, and also the first attempt is considered wrong even if it is not.
In regards to resetting the counter after a successful login, wouldn't that make ratelimiting 2FA by anything other than user (e.g. IP or subnet) completely pointless? An attacker could simply figure out the ratelimit's max number of failed 2FA attempts by failing it several times via a designated IP address and then simply bruteforce 2FA for whichever account they want by simply successfully completing 2FA for a dummy account they set up every

- 1 attempts in order to reset the ratelimit counter.
mmartorana
added a comment.
Feb 6 2025, 3:09 PM
2025-02-06 15:09:40 (UTC+0)
Comment Actions
In
T381033#10527167
@Mprep
wrote:
In
T381033#10524289
@mmartorana
wrote:
While the rate limiting is now being applied, there are still some issues/inconsistencies: specifically, we should likely reset the counter after a successful login, and also the first attempt is considered wrong even if it is not.
In regards to resetting the counter after a successful login, wouldn't that make ratelimiting 2FA by anything other than user (e.g. IP or subnet) completely pointless? An attacker could simply figure out the ratelimit's max number of failed 2FA attempts by failing it several times via a designated IP address and then simply bruteforce 2FA for whichever account they want by simply successfully completing 2FA for a dummy account they set up every

- 1 attempts in order to reset the ratelimit counter.
You’re right about IPs and subnets, but my focus right now is specifically on 2FA for users, as that’s the main topic of this proposed patch.
BPirkle
added a subscriber:
MSantos
Feb 12 2025, 2:40 PM
2025-02-12 14:40:39 (UTC+0)
MSantos
added a project:
MediaWiki-Platform-Team
Feb 12 2025, 2:42 PM
2025-02-12 14:42:52 (UTC+0)
MSantos
moved this task from
Inbox, needs triage
to
MW Platform Team
on the
MediaWiki-Engineering
board.
MSantos
added subscribers:
JTweed-WMF
larissagaulia
larissagaulia
moved this task from
Inbox, needs triage
to
Radar
on the
MediaWiki-Platform-Team
board.
Feb 17 2025, 2:27 PM
2025-02-17 14:27:09 (UTC+0)
larissagaulia
edited projects, added
MediaWiki-Platform-Team (Radar)
; removed
MediaWiki-Platform-Team
mmartorana
added a comment.
Feb 26 2025, 4:35 PM
2025-02-26 16:35:51 (UTC+0)
Comment Actions
Does anyone else have feedback on this? Should we consider a different approach that prevents the pinglimiter from resetting with each request?
Also, would it be possible to put this on Gerrit to gather more input?
Reedy
added a comment.
Feb 26 2025, 11:13 PM
2025-02-26 23:13:32 (UTC+0)
Comment Actions
In
T381033#10583753
@mmartorana
wrote:
Should we consider a different approach that prevents the pinglimiter from resetting with each request?
The problem isn't the ping limiter being reset on each request. That data is stored in shared caching across hosts (maybe not across DC; but not an issue we really care about as is).
As seen by the testing, if we change the to
(rather than
), we can trigger the rate limit, proving the rate limiting does actually work.
With how the code is written, and passing
instead, it doesn't make it behave as intended (ie following the rate limits as implemented), but that's a different issue to it not working at all when we're "checking", and as such, not applying any rate limiting at all.
We don't need to introduce any other layer or otherwise. Using a database would almost certainly be slower, and is duplicating functionality we already have.
The thing that doesn't work, is passing
to "check" if the limit has already been exceeded.
It's a bug buried somewhere in the rate limiting code.
It's a little hard to tell, but I'm guessing it did work when it was implemented (at least in ConfirmEdit, if not also in OATHAuth).
In the ConfirmEdit extension it was touched in
rECOE31c59374a4b1: Add AuthManager support to SimpleCaptcha, QuestyCaptcha, FancyCaptcha…
(May 2016,
T110302: Update ConfirmEdit to use AuthManager
), but that was mostly removing the
$wgUser
call. It was originally added in
rECOE7559502822f5: Add RateLimit check for false CAPTCHAs
(March 2015,
T48292: ConfirmEdit should block IPs after a set number of failed CAPTCHA attempts
).
In the OATHAuth extension it was added in
rEOATa6b60d246501: Apply rate limits to all token verifications
(October 2016 is also a long time ago).
In those 8-10 years since, there's been a lot of code changes around the Authority classes (done primarily by
MediaWiki-Engineering
, see
T261963: Spike to explore Authority concept, implementation, and migration
etc), and I don't think any specific test for that (at least in MediaWiki core) to test whether
still works. And to that extent, no test to prevent any regressions, like this that we're seeing. There are some pingLimiter tests, for example
testPingLimiter
in
tests/phpunit/includes/user/UserTest.php
and the entire of
tests/phpunit/integration/includes/Permissions/RateLimiterTest.php
, and to a lesser extent
tests/phpunit/unit/includes/Permissions/UserAuthorityTest.php
There are some tests in
UserAuthorityTest
using
$this->assertFalse( $authority->limit( 'edit', 0, null ) );
, but none seemingly using
assertTrue
(which would show up the bug)
Those tests added in
rMWa8ee61d9d682: Implement rate limiting in Authority.
T310476: Add rate limiting to Authority
. For my own notes, this suggests possibly broken since 1.41.0 (TODO: Test on REL1_39! Still supported, but the change isn't in it. I have a REL1_39 dev wiki, but not sure I have the caching setup; easily fixed)
UserAuthority tracks which limits have already been incremented during the current request, to avoid duplicate increments caused by code that still calls pingLimiter directly.
^ This may be another hint.
It's not a pattern used in MW core, nor in any other extensions bar the two named. So is very much an edge case in that sense.
Regarding the patch, it doesn't help fix the issue, as we're still doing the
pingLimiter( 'badoath', 0 )
call, and evaluating the return value, which still doesn't work.
On the moved call,
pingLimiter( 'badoath' )
we're not checking the return value from the function, so we're not doing any action on it, as that is the callers job. The function tells you if you've hit the limit, what you do with that information.
The comment that moved too is incorrect (but that is a minor point), as at that point in the code we haven't even checked if it is a failed or a succeeded attempt, that's what
$this->module->verify()
does.
We're checking the limiter, because if they should be (already) rate limited, we're not going to let them pass just because they eventually got it right.
Reedy
added projects:
MW-1.39-release
MW-1.42-release
MW-1.43-release
Feb 26 2025, 11:54 PM
2025-02-26 23:54:31 (UTC+0)
Comment Actions
Hmm. Seems it's still similarly broken on REL1_39...
Same behaviour, if I change the
pingLimiter
call passing to
, I get the rate limiting I would expect.
Seen on CLI too that
doesn't seem to change anything.
reedy@ubuntu64-web-esxi:/var/www/wiki-1.39/core$ echo '$u = User::newFromName("Reedy"); return $u->pingLimiter("badoath",1);' | php8.2 maintenance/eval.php
bool(true)

reedy@ubuntu64-web-esxi:/var/www/wiki-1.39/core$ echo '$u = User::newFromName("Reedy"); return $u->pingLimiter("badoath",1);' | php8.2 maintenance/eval.php
bool(true)

reedy@ubuntu64-web-esxi:/var/www/wiki-1.39/core$ echo '$u = User::newFromName("Reedy"); return $u->pingLimiter("badoath",0);' | php8.2 maintenance/eval.php
bool(false)
daniel
subscribed.
Feb 28 2025, 6:09 PM
2025-02-28 18:09:06 (UTC+0)
Comment Actions
Looking...
daniel
added a comment.
Feb 28 2025, 6:19 PM
2025-02-28 18:19:03 (UTC+0)
Comment Actions
Patch is up. I guess this was broken by
. The LimitBatch calss introduced in that patch has a peek() method, but it didn't get used in PingLimiter. My patch fixes that. The fix isn't very elegant, improvments are welcome :)
daniel
added a subscriber:
tstarling
Feb 28 2025, 6:19 PM
2025-02-28 18:19:37 (UTC+0)
Comment Actions
Pinging
@tstarling
for awareness.
daniel
added a comment.
Feb 28 2025, 6:23 PM
2025-02-28 18:23:47 (UTC+0)
Comment Actions
@Reedy
Thank you for the analysis, it allowed me to find the issue very quickly :)
daniel
added a comment.
Feb 28 2025, 6:26 PM
2025-02-28 18:26:48 (UTC+0)
Comment Actions
Re security implications:
The bug Reedy identified shouldn't be security relevant. It doesn't affect code that actually applies rate limiting. It only affects code that "peeks" the rate limit beforehand, in order to provide information to the use before actually trying to perform an operation.
However, the observation that led to this ticket being filed indicates that the limit isn't enforced. If that's the case, there is probably *another* bug lurking somewhere.
daniel
added a comment.
Feb 28 2025, 6:39 PM
2025-02-28 18:39:17 (UTC+0)
Comment Actions
High level observation: It seems to me that rate limits on authentication attempts should use MediaWiki\Auth\Throttler, not PingLimiter.
daniel
added a comment.
Feb 28 2025, 6:48 PM
2025-02-28 18:48:35 (UTC+0)
Comment Actions
I suspect the issue is that the extension is trying to enforce a per-user limit before the use is actually identified. PingLimiter doesn't work that way. It doesn't do limits per user name, but per use id. And there is no user ID if auth fails. Even user-global limits don't apply if the user isn't logged in.
It should still be possible to enforce a limit on login attempts per IP. PingLimiter doesn't support limits per user name. I guess that could be changed... but currently, that's how it is: if there is no user IP, anon limits are enforced, and user limits are not enforced.
Using MediaWiki\Auth\Throttler would fix that.
sbassett
added a subscriber:
gerritbot
Feb 28 2025, 6:52 PM
2025-02-28 18:52:24 (UTC+0)
Comment Actions
In
T381033#10592066
@daniel
wrote:
Re security implications:
The bug Reedy identified shouldn't be security relevant. It doesn't affect code that actually applies rate limiting. It only affects code that "peeks" the rate limit beforehand, in order to provide information to the use before actually trying to perform an operation.
My understanding is that it
does
have security implications for OATHAuth and ConfirmEdit, since those extensions were explicitly relying upon the "peek" check to potentially block nefarious behavior. And I believe we have assumed this functionality also works for private, ad-hoc security mitigations deployed within Wikimedia production, which definitely attempt to block nefarious behavior. I think we'll want to get
merged sooner than later.
Reedy
added a comment.
Edited
Feb 28 2025, 6:54 PM
2025-02-28 18:54:01 (UTC+0)
Comment Actions
In
T381033#10592044
@daniel
wrote:
Patch is up. I guess this was broken by
. The LimitBatch calss introduced in that patch has a peek() method, but it didn't get used in PingLimiter. My patch fixes that. The fix isn't very elegant, improvments are welcome :)
Thanks for that!
I guess that gives us a good sign that it has existed as far back as REL1_39 (which confirms my testing), but maybe not further. But as REL1_40/REL1_41 aren't supported, so aren't REL1_38 or before.
For those following along, patch is
I'll have a proper look later today, and test it.
In
T381033#10592066
@daniel
wrote:
Re security implications:
The bug Reedy identified shouldn't be security relevant. It doesn't affect code that actually applies rate limiting. It only affects code that "peeks" the rate limit beforehand, in order to provide information to the use before actually trying to perform an operation.
However, the observation that led to this ticket being filed indicates that the limit isn't enforced. If that's the case, there is probably *another* bug lurking somewhere.
I think the problem is that the code is using the peek to apply the limitations (similar code in ConfirmEdit and OATHAuth), for example:
// Don't increase pingLimiter, just check for limit exceeded.
if
$user
->
pingLimiter
'badoath'
return
AuthenticationResponse
::
newUI
new
TOTPAuthenticationRequest
()
],
new
Message
'oathauth-throttled'
// Arbitrary duration given here
Message
::
durationParam
60
),
'error'
);
In
T381033#10592080
@daniel
wrote:
High level observation: It seems to me that rate limits on authentication attempts should use MediaWiki\Auth\Throttler, not PingLimiter.
Certainly possible, but it's old code, hasn't been touched pretty much since implementation, that hasn't been explicitly deprecated, so no specific reason before now (and this bug).
Whether it actually completely worked as designed when first used years ago...
Something to potentially address in a followup.
In
T381033#10592115
@daniel
wrote:
I suspect the issue is that the extension is trying to enforce a per-user limit before the use is actually identified. PingLimiter doesn't work that way. It doesn't do limits per user name, but per use id. And there is no user ID if auth fails. Even user-global limits don't apply if the user isn't logged in.
It should still be possible to enforce a limit on login attempts per IP. PingLimiter doesn't support limits per user name. I guess that could be changed... but currently, that's how it is: if there is no user IP, anon limits are enforced, and user limits are not enforced.
Using MediaWiki\Auth\Throttler would fix that.
I'm not quite sure offhand which should be applying, and when... Arguably we want to prevent multiple IPs against a single user account, and similarly, prevent a single IP against multiple accounts etc...
Reedy
added a comment.
Feb 28 2025, 11:22 PM
2025-02-28 23:22:27 (UTC+0)
Comment Actions
Patch tested on master, confirmed to fix the bug as previously tested for.
+1'd, not +2-ing it yet due to Tim's CR/tweak proposal
daniel
added a comment.
Edited
Mar 1 2025, 8:03 AM
2025-03-01 08:03:49 (UTC+0)
Comment Actions
In
T381033#10592130
@sbassett
wrote:
My understanding is that it
does
have security implications for OATHAuth and ConfirmEdit, since those extensions were explicitly relying upon the "peek" check to potentially block nefarious behavior. And I believe we have assumed this functionality also works for private, ad-hoc security mitigations deployed within Wikimedia production, which definitely attempt to block nefarious behavior. I think we'll want to get
merged sooner than later.
You are right - the actual increment works, but it's ineffective, since the code doesn't use the second call to rateLimiter to gate critical functionality. It only uses the peek for gating.
I think it's important to investigate whether the per-user limits are working at all, though. I suspect that they are not.
gerritbot
added a comment.
Mar 1 2025, 12:39 PM
2025-03-01 12:39:39 (UTC+0)
Comment Actions
Change #1123698
merged
by jenkins-bot:
[mediawiki/core@master] RateLimiter: Fix peek mode
gerritbot
added a comment.
Mar 1 2025, 3:49 PM
2025-03-01 15:49:00 (UTC+0)
Comment Actions
Change #1123749 had a related patch set uploaded (by Reedy; author: Daniel Kinzler):
[mediawiki/core@REL1_43] RateLimiter: Fix peek mode
gerritbot
added a project:
Patch-For-Review
Mar 1 2025, 3:49 PM
2025-03-01 15:49:03 (UTC+0)
gerritbot
added a comment.
Mar 1 2025, 3:52 PM
2025-03-01 15:52:03 (UTC+0)
Comment Actions
Change #1123752 had a related patch set uploaded (by Reedy; author: Daniel Kinzler):
[mediawiki/core@REL1_42] RateLimiter: Fix peek mode
gerritbot
added a comment.
Mar 1 2025, 3:54 PM
2025-03-01 15:54:29 (UTC+0)
Comment Actions
Change #1123755 had a related patch set uploaded (by Reedy; author: Daniel Kinzler):
[mediawiki/core@REL1_39] RateLimiter: Fix peek mode
gerritbot
added a comment.
Mar 1 2025, 4:14 PM
2025-03-01 16:14:41 (UTC+0)
Comment Actions
Change #1123755
merged
by jenkins-bot:
[mediawiki/core@REL1_39] RateLimiter: Fix peek mode
gerritbot
added a comment.
Mar 1 2025, 4:18 PM
2025-03-01 16:18:19 (UTC+0)
Comment Actions
Change #1123752
merged
by jenkins-bot:
[mediawiki/core@REL1_42] RateLimiter: Fix peek mode
gerritbot
added a comment.
Mar 1 2025, 4:21 PM
2025-03-01 16:21:42 (UTC+0)
Comment Actions
Change #1123749
merged
by jenkins-bot:
[mediawiki/core@REL1_43] RateLimiter: Fix peek mode
Jdforrester-WMF
added projects:
MW-1.39-notes
MW-1.42-notes
MW-1.43-notes
MW-1.44-notes (1.44.0-wmf.19; 2025-03-04)
Mar 3 2025, 2:57 AM
2025-03-03 02:57:57 (UTC+0)
Reedy
moved this task from
Backlog
to
Bugs
on the
ConfirmEdit (CAPTCHA extension)
board.
Mar 10 2025, 11:43 AM
2025-03-10 11:43:04 (UTC+0)
mmartorana
added a comment.
Mar 10 2025, 12:36 PM
2025-03-10 12:36:38 (UTC+0)
Comment Actions
In
T381033#10592764
@Reedy
wrote:
Patch tested on master, confirmed to fix the bug as previously tested for.
+1'd, not +2-ing it yet due to Tim's CR/tweak proposal
Are you referring to this task issue or the one related to the peak limit?
sbassett
added a comment.
Mar 10 2025, 3:04 PM
2025-03-10 15:04:29 (UTC+0)
Comment Actions
In
T381033#10618344
@mmartorana
wrote:
In
T381033#10592764
@Reedy
wrote:
Patch tested on master, confirmed to fix the bug as previously tested for.
+1'd, not +2-ing it yet due to Tim's CR/tweak proposal
Are you referring to this task issue or the one related to the peak limit?
Pretty sure this was in regards to
(the work above) which was merged and backported, and should fix the issue described in this task.
mmartorana
added a comment.
Mar 11 2025, 4:00 PM
2025-03-11 16:00:48 (UTC+0)
Comment Actions
I have tested this thoroughly and confirm that the issue is resolved both locally and in production.
mmartorana
closed this task as
Resolved
Mar 19 2025, 3:38 PM
2025-03-19 15:38:27 (UTC+0)
mmartorana
claimed this task.
mmartorana
changed the visibility from "
Custom Policy
" to "Public (No Login Required)".
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct.
Wikimedia Foundation
Code of Conduct
Disclaimer
CC-BY-SA
GPL
Credits