ReCaptchaSettings.DetectionThreshold is not counted correctly for log in

[rjpowers10]

Describe the bug

My value of ReCaptchaSettings.DetectionThreshold is 5. I expect this to mean I can fail logging in 5 times and then on the 6th attempt I see the ReCaptcha.

• In OC 1.8.2 I actually have to fail 6 times before I see the ReCaptcha.
• In OC 2.1.2 I have to fail 7 times before I see the ReCaptcha.

In OC 2.1.2 it seems like the number of failed attempts went up by 1, but it seems like it was off in 1.8.2 as well. If the 1.8.2 behavior is correct then that's fine with me. But it does seem like it unintentionally changed in 2.1.2, which is my bigger concern.

Orchard Core version

I noticed this in the process of updating from OC 1.8.2 to 2.1.2.

To Reproduce

1. Create a new site with the blog recipe.
2. Enable the OrchardCore.ReCaptcha.Users feature.
3. Set the site key and secret key for ReCaptcha under Security > Settings > reCaptcha.
4. Log out.
5. Go to /login.
6. Repeatedly log in with dummy credentials.

Expected behavior

The default setting of ReCaptchaSettings.DetectionThreshold is 5 and I'm using the default value.

In OC 1.8.2 I fail the log in process 5 times in a row. I would expect to see the ReCaptcha here, but I actually need to fail a 6th time for it to appear.

In this gif I click the "log in" button 5 times and fail, but it isn't until I click "log in" a 6th time that the ReCaptcha is displayed.

In OC 2.1.2 it is similar, except I have to fail 7 times even though my threshold is still set to 5.

[Piedone]

I don't actually know why this worked differently before, since the logic is quite simple and implemented here, what didn't change:

OrchardCore/src/OrchardCore/OrchardCore.ReCaptcha.Core/ActionFilters/Detection/IPAddressRobotDetector.cs

Line 38 in 1c890da

public RobotDetectionResult DetectRobot()

So, the algorithm is that DetectRobot() (which checks faultyRequestCount > _settings.DetectionThreshold) is called when the login POST is handled, and FlagAsRobot(), which increments faultyRequestCount, after that, when the login fails. This is why you get 6 instead of 5.

You get 7 instead of 6 because faultyRequestCount > _settings.DetectionThreshold will still be false when the counter reaches 5.

I only elaborated this in 10 times the characters the actual code is long, to show that for me it's really not clear how else this should be implemented, while agreeing that the configuration value of DetectionThreshold should work how you expect: after 5 failing logins, show captcha (i.e. at the 6th login).

Changing that > to >= is a trivial fix, but since this is the logic since 2018, I'm hesitant to just change it.

[rjpowers10]

I agree that changing > to >= would fix the original problem of getting 6 instead of 5. I can also understand why you would hesitate to make that change since it's been that way for a long time.

But I'm still not sure why in 2.1.2 I'm getting 7 instead of 6.

[rjpowers10]

I think I know why it increased from 6 to 7 in 2.1.2. It has to do with the refactoring to make the login form driver-based.

On the POST the form shape is built on line 108. This will validate the ReCaptcha and increment the counter if it fails.

OrchardCore/src/OrchardCore.Modules/OrchardCore.Users/Controllers/AccountController.cs

Line 108 in 4ab7c34

var formShape = await _loginFormDisplayManager.UpdateEditorAsync(model, _updateModelAccessor.ModelUpdater, false, string.Empty, string.Empty);

But if something about the POST fails (e.g. ReCaptcha validation fails) then the form shape is simply redisplayed as is with View(formShape). The form needs to be rebuilt before it is redisplayed so we can check the counter again and potentially inject the ReCaptcha into the shape.

So to summarize I think there are two separate but related bugs.

1. The evaluation in IPAddressRobotDetector should be >= instead of >. I think this has been a bug since the beginning.
2. The form needs to be rebuilt before being redisplayed in AccountController.LoginPOST. This has been a bug since 2.0 when the login form was refactored to be driver-based.

[Piedone]

@MikeAlhayek opinion on #2?

[rjpowers10]

The model state is probably going to be lost if the form is rebuilt, so the entered username / email address will be lost. That's probably why it is the way it is now.

[MikeAlhayek]

Self Notes

First observation, the IPAddressRobotDetector should not be using IMemoryCache as won't work in a distributed environment since there is no guarantee that the post request will be handed by the same machine where the memory is cached. We should use DistributedCache or the new HybridCache.

Second, it would be better for IClientIPAddressAccessor to return ValueTask instead of task with the IPAddress. The IDetectRobots should all be asyncronous calls and return ValueTask.

Recommendation

On issue 1, I agree that the condition should be >= not > since we here start at 0 not 1.

On issue 2, I think the ReCaptchaLoginFormDisplayDriver should be replaced by a ShapeTableProvider so that the reCaptcha is not added using the display driver but using the shape table provider. I'll submit a PR for this fix.

[sebastienros]

First observation, the IPAddressRobotDetector should not be using IMemoryCache

I don't quite agree, a local cache is still valid. It's just that every node has the cache. Not all caches need to be reused/shared. In this case it might be better to not issue a network call (when actually running distributed). There is no general truth in this case, it depends on the cost of the cached item.

[MikeAlhayek]

@sebastienros the problem here has nothing to so with distribution. This issue happens one a single node.

In regard to the IPAddressRobotDetector, we keep a count of how many attempts the user made to the server before showing the ReCaptcha challenge. The default settings indicate that the we show the challenge when the user submits the form 5 invalid attempts in a row, So if there are multiple nodes, and the user makes 4 attempts per server they will never see the challenge. So we should distribute the attempt count to ensure that the total requests to any node is 5.

[sebastienros]

@rjpowers10 we are now considering removing this threshold detection logic completely. The idea being that login already has a feature to limit the number of attempts, and rate limiting is an existing middleware that could also be used here (and anywhere else). The amount of changes and the fact we also need to maintain a shared cached to count (not thinking about eviction time) is too important for what we think is the actual usage: you are the first one to find it doesn't work even though this feature is not new.

[github-actions]

We triaged this issue and set the milestone according to the priority we think is appropriate (see the docs on how we triage and prioritize issues).

This indicates when the core team may start working on it. However, if you'd like to contribute, we'd warmly welcome you to do that anytime. See our guide on contributions here.

[rjpowers10]

The idea being that login already has a feature to limit the number of attempts, and rate limiting is an existing middleware that could also be used here (and anywhere else).

This is true but I'm unsure what this means for the ReCaptcha feature. I know .NET has a built in feature to lock an account after a number of failed attempts, but that feels like a separate topic from ReCaptcha.

Or are you thinking that ReCaptcha just becomes an on/off feature? No counter, the ReCatpcha is just shown every time (if enabled).

[MikeAlhayek]

Or are you thinking that ReCaptcha just becomes an on/off feature? No counter, the ReCatpcha is just shown every time (if enabled).

exactly the idea.

[rjpowers10]

So you're only talking about removing the IP-based counter. Going forward, you can use ReCaptcha, or you can use the .NET lockout, or you can use both. ReCaptcha is either on or off with no detection features.

I'm starting to see why I'm confused. The IP-counter was only intended for logging in.

• In 1.8.2 the registration form shows the ReCaptcha on the first attempt, no counter check involved.
• In 2.1.2 registration is now checking the counter, but it shouldn't be. That's another bug.

Which makes sense now that I think about it more. Counting registration failures is kind of a weird concept to begin with.

Okay, removing the counter sounds good to me. I see where you're going with this.

[MikeAlhayek]

@Piedone are you also okay with removing the threshold and the counter from reCaptcha altogether?

[rjpowers10]

Are you thinking this would be a 2.1.x release? It's a breaking change...but there are also issues with it already.

[MikeAlhayek]

no. That would be a breaking change. 3.0

[rjpowers10]

Maybe this is worth a separate issue then but I think there is an issue with registration.

• 1.8.2 would show the ReCaptcha on the first attempt
• 2.1.2 is never showing the ReCaptcha because it is checking the counter but the counter also never gets incremented

Maybe something can be done there without the larger, breaking change?

EDIT: My mistake

[MikeAlhayek]

doesn't the reCaptcha always show now?

[rjpowers10]

Oh I see, the ReCaptcha tag helper has an "always show" mode that registration is using. Yes, you are correct. 2.1.2 registration is working. My mistake.

[Piedone]

@Piedone are you also okay with removing the threshold and the counter from reCaptcha altogether?

Yep, good points.