Feat | Harden login captcha gate by replacing request body counter with server-side session counter

[matiasperrone-exo]

Task

Ref.: https://app.clickup.com/t/86b9v1acw

Dependency

Ref: https://app.clickup.com/t/86b8xw6ue (Migrate from Google reCAPTCHA to Cloudflare Turnstile)
PR #121 - Feat | Migrate from Google reCAPTCHA to Cloudflare Turnstile

Changes

The change fixes a security gap where the captcha trigger counter was controlled by the client (sent in the request body) instead of the server. Three files changed:

app/Http/Controllers/UserController.php

• $login_attempts is now initialized from the server-side session (Session::get('captcha_failed_attempts', 0)) instead of from the request body.
• On each failed auth attempt, the counter is incremented via $user->updateLoginFailedAttempt() and persisted to the session.
• On success, the session key is cleared (Session::forget). The user lookup was moved before the auth attempt so it's available in both success and failure paths.
• postLogin now reads login_attempts from the session instead of the request body, preventing clients from resetting or falsifying the counter.
• Session key is properly incremented on failed attempts and cleared on success. For both password and OTP flows.

resources/js/login/login.js

• Removed the client-side code that was sending login_attempts in the request body (no longer needed/meaningful).

tests/UserLoginTurnstileTest.php

• Tests rewritten to drive captcha gating via session state rather than request body parameters.
• All 11 tests pass with the new server-side behavior.

---

Requested GOAL

Current state

After rolling back the username-enumeration regression from PR #121, UserController::postLogin() reads $login_attempts from Request::input('login_attempts') (the original implementation). The value travels through a hidden form field that errorLogin() flashes into the session and login.blade.php renders into the next form. This source is server-set under the normal browser flow, but it is supplied in the request body and is therefore fully attacker-controllable: a curl POST that omits the field, or sets login_attempts=0, skips the captcha rule unconditionally regardless of how many times the attacker has failed. The captcha gate is effectively a suggestion, not a control.

Target state

$login_attempts is sourced from a Laravel session value that the server alone writes. The validator gates the captcha rule on a counter the attacker cannot tamper with from the request body,
while still being independent of the submitted username (so the gate does not fork on user existence and the enumeration oracle that PR #121 introduced cannot reappear). The persisted per-user
counter (User.loginFailedAttempt) remains in place for audit and lockout, but does not drive the captcha decision.

TASKS

1. Replace counter source in UserController::postLogin
   1.1 Change $login_attempts = intval(Request::input('login_attempts')) to $login_attempts = (int) Session::get('captcha_failed_attempts', 0).
   1.2 Confirm the validator rule block (cf-turnstile-response = ['required', new Turnstile()] when $login_attempts >= threshold) is unchanged.
2. Increment session counter on every authentication failure
   2.1 In the AuthenticationException catch in postLogin (around L479-L501) increment Session::put('captcha_failed_attempts', current+1) before calling errorLogin().
   2.2 Pass the updated session counter as login_attempts in the errorLogin payload so the redirected login page renders the right config.
3. Reset session counter on successful login
   3.1 Locate the successful authentication path (login_strategy->postLogin() or equivalent) and call Session::forget('captcha_failed_attempts') after the authenticated session is established.
4. Drop the now-redundant hidden form input
   4.1 Remove or stop rendering the hidden login_attempts input in login.blade.php (or wherever it is emitted). Server-driven gating no longer needs it.
   4.2 Confirm the React login.js no longer reads it, or treats absence as 0.
5. Update functional test suite
   5.1 Replace any test setup that relied on Request::input('login_attempts') (overrides like ['login_attempts' => N]) with Session::put('captcha_failed_attempts', N) via withSession() or
   equivalent.
   5.2 Add testRequestSuppliedLoginAttemptsIsIgnored: post username + password + login_attempts=0 with the session counter at threshold, assert cf-turnstile-response is still required (proves the
   request-body field is no longer trusted).
   5.3 Add testCaptchaRequiredForUnknownUsernameWhenSessionAtThreshold: with the session counter at threshold, post a non-existent username and no captcha; assert cf-turnstile-response is required
   (locks in enumeration-safety alongside the rollback).
   5.4 Add testSuccessfulLoginClearsSessionCounter: drive counter to threshold, post valid credentials with a faked Turnstile pass, assert Session::has('captcha_failed_attempts') is false
   afterwards.
   5.5 Update testLoginScreenIncludesTurnstileConfigWhenAboveThreshold to drive the precondition via Session::put('captcha_failed_attempts', threshold - 1) instead of any request-body or DB-mutation helper.

ACCEPTANCE CRITERIA

1. UserController::postLogin reads $login_attempts only from Session, not from Request::input nor from a User record, before building the validator rule set.
2. POST /auth/login with username=victim@example.com (real account) and POST /auth/login with username=nobody@doesnotexist.example (no record), holding all other fields equal and the session
   counter at threshold, return validator error sets that both contain cf-turnstile-response (no enumeration oracle).
3. After a successful login, Session::has('captcha_failed_attempts') returns false on the next request.
4. uv run vendor/bin/phpunit --filter UserLoginTurnstileTest passes with 0 failures, and includes the three new tests listed in tasks 5.2-5.4.
5. Manual exercise: fail 3 logins with a real account in the browser, confirm Turnstile renders; clear cookies, fail 3 logins with a non-existent email, confirm Turnstile renders at the same
   threshold; using curl, attempt to bypass with login_attempts=0 in the body and confirm Cloudflare validation still fails.

Summary by CodeRabbit

• Security Improvements
  • Enhanced login attempt tracking to be managed server-side, preventing potential tampering with CAPTCHA challenge triggers. Failed login attempts are now accurately counted, ensuring CAPTCHA verification is applied reliably based on actual failed attempts rather than client-submitted values.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 10e5dd4a-0f49-4e63-9234-7a392164eed5

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/harden-login-captcha-gate-by-replacing-request-body-counter-with-server-side-session-counter

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[github-actions]

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-132/

This page is automatically updated on each push to this PR.

[github-actions]

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-132/

This page is automatically updated on each push to this PR.

[github-actions]

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-132/

This page is automatically updated on each push to this PR.

[github-actions]

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-132/

This page is automatically updated on each push to this PR.

[github-actions]

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-132/

This page is automatically updated on each push to this PR.

[github-actions]

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-132/

This page is automatically updated on each push to this PR.

[github-actions]

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-132/

This page is automatically updated on each push to this PR.

[github-actions]

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-132/

This page is automatically updated on each push to this PR.

[github-actions]

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-132/

This page is automatically updated on each push to this PR.

[github-actions]

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-132/

This page is automatically updated on each push to this PR.

[github-actions]

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-132/

This page is automatically updated on each push to this PR.

[github-actions]

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-132/

This page is automatically updated on each push to this PR.

[github-actions]

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-132/

This page is automatically updated on each push to this PR.

[github-actions]

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-132/

This page is automatically updated on each push to this PR.

[github-actions]

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-132/

This page is automatically updated on each push to this PR.

[github-actions]

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-132/

This page is automatically updated on each push to this PR.

[caseylocker]

Change request — race condition leaves Task 2 / AC #2 unmet under concurrency

The PR satisfies Task 2's "+1 per failure" invariant and AC #2's threshold guarantee in sequential request flows, but both break under concurrent POSTs against the same session.

Root cause

• Deployed session driver is redis (SESSION_DRIVER=redis in .env.example:27)
• Illuminate\Session\CacheBasedSessionHandler::write() overwrites the entire serialized session blob non-atomically (no read-modify-write lock)
• Illuminate\Session\SessionManager::shouldBlock() defaults to false and the login POST route is not blocked (routes/web.php:52)

Two concurrent failed POSTs sharing the same session cookie both read counter=0, both compute 0+1=1, both write counter=1 — the second increment is lost. An attacker firing N concurrent POSTs can make N failed attempts while the captcha gate sees counter=1, holding the gate open through the entire burst. That contradicts:

• Task 2 ("Increment session counter on every authentication failure")
• AC #2 (counter at threshold must reliably trigger the cf-turnstile-response rule)
• Goal ("the validator gates the captcha rule on a counter the attacker cannot tamper with")

Suggested patch

--- a/routes/web.php
+++ b/routes/web.php
@@ -49,7 +49,7 @@
             Route::group(array('prefix' => 'verification'), function () {
                 Route::post('resend', ['middleware' => ['csrf'], 'uses' => 'UserController@resendVerificationEmail']);
             });
-            Route::post('', ['middleware' => 'csrf', 'uses' => 'UserController@postLogin']);
+            Route::post('', ['middleware' => 'csrf', 'uses' => 'UserController@postLogin'])->block();

Route::block() serializes concurrent same-session requests via an atomic cache lock. Defaults (10s lock TTL, 10s wait) are appropriate for an auth endpoint — tune if needed.

Test coverage note

The +1 per sequential failure invariant is already pinned by the existing testRepeatedKnownUserFailuresIncrementSessionCounterByOnePerAttempt at tests/UserLoginTurnstileTest.php:340, which asserts counter === CAPTCHA_THRESHOLD after sequential failures. BrowserKit is single-threaded so it can't reproduce the race itself — no new test is required to land this change. The route block alone closes the gap.

With the route block in place, the ticket's tasks, acceptance criteria, and goal are all satisfied.

[matiasperrone-exo]

Thanks @caseylocker for your comment, I added the requested change. Please review.

Change request — race condition leaves Task 2 / AC # 2 unmet under concurrency

The PR satisfies Task 2's "+1 per failure" invariant and AC # 2's threshold guarantee in sequential request flows, but both break under concurrent POSTs against the same session.

Root cause

• Deployed session driver is redis (SESSION_DRIVER=redis in .env.example:27)
• Illuminate\Session\CacheBasedSessionHandler::write() overwrites the entire serialized session blob non-atomically (no read-modify-write lock)
• Illuminate\Session\SessionManager::shouldBlock() defaults to false and the login POST route is not blocked (routes/web.php:52)

Two concurrent failed POSTs sharing the same session cookie both read counter=0, both compute 0+1=1, both write counter=1 — the second increment is lost. An attacker firing N concurrent POSTs can make N failed attempts while the captcha gate sees counter=1, holding the gate open through the entire burst. That contradicts:

• Task 2 ("Increment session counter on every authentication failure")
• AC # 2 (counter at threshold must reliably trigger the cf-turnstile-response rule)
• Goal ("the validator gates the captcha rule on a counter the attacker cannot tamper with")

Suggested patch

--- a/routes/web.php
+++ b/routes/web.php
@@ -49,7 +49,7 @@
             Route::group(array('prefix' => 'verification'), function () {
                 Route::post('resend', ['middleware' => ['csrf'], 'uses' => 'UserController@resendVerificationEmail']);
             });
-            Route::post('', ['middleware' => 'csrf', 'uses' => 'UserController@postLogin']);
+            Route::post('', ['middleware' => 'csrf', 'uses' => 'UserController@postLogin'])->block();

Route::block() serializes concurrent same-session requests via an atomic cache lock. Defaults (10s lock TTL, 10s wait) are appropriate for an auth endpoint — tune if needed.

Test coverage note

The +1 per sequential failure invariant is already pinned by the existing testRepeatedKnownUserFailuresIncrementSessionCounterByOnePerAttempt at tests/UserLoginTurnstileTest.php:340, which asserts counter === CAPTCHA_THRESHOLD after sequential failures. BrowserKit is single-threaded so it can't reproduce the race itself — no new test is required to land this change. The route block alone closes the gap.

With the route block in place, the ticket's tasks, acceptance criteria, and goal are all satisfied.