Not sure if this counts as a security issue but escalating it just in case

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.

For WMF production:

reedy@deploy2002:~$ mwscript eval.php enwiki
DEPRECATION WARNING: Maintenance scripts are moving to Kubernetes. See
https://wikitech.wikimedia.org/wiki/Maintenance_scripts for the new process.
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)
  }
}

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...

@Mprep Does any rate limiting actually work on your wiki(s)?

https://www.mediawiki.org/wiki/Manual:$wgRateLimits#Details
https://www.mediawiki.org/wiki/Special:MyLanguage/Manual:$wgMainCacheType

What is $wgMainCacheType set to?

In T381033#10505109, @Reedy wrote:

@Mprep Does any rate limiting actually work on your wiki(s)?

https://www.mediawiki.org/wiki/Manual:$wgRateLimits#Details
https://www.mediawiki.org/wiki/Special:MyLanguage/Manual:$wgMainCacheType

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;

In T381033#10505071, @Reedy wrote:

For WMF production:

reedy@deploy2002:~$ mwscript eval.php enwiki
DEPRECATION WARNING: Maintenance scripts are moving to Kubernetes. See
https://wikitech.wikimedia.org/wiki/Maintenance_scripts for the new process.
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)
  }
}

To quote a colleague

[02:12:12] <Krinkle> > echo '$u = User::newFromName("KrinkleSock"); return $u->pingLimiter("badoath");' | mwscript eval.php enwiki -d 1
[02:12:25] <Krinkle> 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'] = [ 1, 60 ];
$wgRateLimits['badoath']['user-global'] = [ 1, 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)
  }
}

In T381033#10508293, @Reedy wrote:

To quote a colleague

[02:12:12] <Krinkle> > echo '$u = User::newFromName("KrinkleSock"); return $u->pingLimiter("badoath");' | mwscript eval.php enwiki -d 1
[02:12:25] <Krinkle> 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'] = [ 1, 60 ];
$wgRateLimits['badoath']['user-global'] = [ 1, 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.

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 0 in if ( $user->pingLimiter( 'badoath', 0 ) ) {... If I swap that to 1, 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

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 1.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

*******************************************************************************
NOTE: Do not run maintenance scripts directly, use maintenance/run.php instead!
      Running scripts directly has been deprecated in MediaWiki 1.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

*******************************************************************************
NOTE: Do not run maintenance scripts directly, use maintenance/run.php instead!
      Running scripts directly has been deprecated in MediaWiki 1.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",1);' | php maintenance/eval.php

*******************************************************************************
NOTE: Do not run maintenance scripts directly, use maintenance/run.php instead!
      Running scripts directly has been deprecated in MediaWiki 1.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",1);' | php maintenance/eval.php

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

/var/www/wiki/mediawiki/core/maintenance/eval.php:151:
bool(true)

If I test on a web browser on enwiki... I also don't get anything other than Verification failed.

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', 0 ) ) {

The MW core actions don't pass 0 to check...

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.

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', 0 ) ) {

The MW core actions don't pass 0 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).

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.

We're going to take some more time to understand the potential side effects of this issue and will resume next week.

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', 0 ) ) {

The MW core actions don't pass 0 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?

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 <max number of failed 2FA attempts> - 1 attempts in order to reset the ratelimit counter.

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 <max number of failed 2FA attempts> - 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.

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?

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 1 (rather than 0), we can trigger the rate limit, proving the rate limiting does actually work.

With how the code is written, and passing 1 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 0 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 0 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.

Hmm. Seems it's still similarly broken on REL1_39...

Same behaviour, if I change the pingLimiter call passing to 1, I get the rate limiting I would expect.

Seen on CLI too that 0 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",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",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)

Looking...

Patch is up. I guess this was broken by https://gerrit.wikimedia.org/r/c/mediawiki/core/+/808201. 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 :)

Pinging @tstarling for awareness.

@Reedy Thank you for the analysis, it allowed me to find the issue very quickly :)

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.

High level observation: It seems to me that rate limits on authentication attempts should use MediaWiki\Auth\Throttler, not PingLimiter.

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.

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 https://gerrit.wikimedia.org/r/1123698 merged sooner than later.

In T381033#10592044, @daniel wrote:

Patch is up. I guess this was broken by https://gerrit.wikimedia.org/r/c/mediawiki/core/+/808201. 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 https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1123698

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', 0 ) ) {
			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...

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

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 https://gerrit.wikimedia.org/r/1123698 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.

Change #1123698 merged by jenkins-bot:

[mediawiki/core@master] RateLimiter: Fix peek mode

https://gerrit.wikimedia.org/r/1123698

Change #1123749 had a related patch set uploaded (by Reedy; author: Daniel Kinzler):

[mediawiki/core@REL1_43] RateLimiter: Fix peek mode

https://gerrit.wikimedia.org/r/1123749

Change #1123752 had a related patch set uploaded (by Reedy; author: Daniel Kinzler):

[mediawiki/core@REL1_42] RateLimiter: Fix peek mode

https://gerrit.wikimedia.org/r/1123752

Change #1123755 had a related patch set uploaded (by Reedy; author: Daniel Kinzler):

[mediawiki/core@REL1_39] RateLimiter: Fix peek mode

https://gerrit.wikimedia.org/r/1123755

Change #1123755 merged by jenkins-bot:

[mediawiki/core@REL1_39] RateLimiter: Fix peek mode

https://gerrit.wikimedia.org/r/1123755

Change #1123752 merged by jenkins-bot:

[mediawiki/core@REL1_42] RateLimiter: Fix peek mode

https://gerrit.wikimedia.org/r/1123752

Change #1123749 merged by jenkins-bot:

[mediawiki/core@REL1_43] RateLimiter: Fix peek mode

https://gerrit.wikimedia.org/r/1123749

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?

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 https://gerrit.wikimedia.org/r/1123698 (the work above) which was merged and backported, and should fix the issue described in this task.

I have tested this thoroughly and confirm that the issue is resolved both locally and in production.