Skip to content

fix: add rate limiting in the forward auth endpoint#555

Merged
steveiliop56 merged 1 commit into
mainfrom
fix/rate-limiting
Dec 31, 2025
Merged

fix: add rate limiting in the forward auth endpoint#555
steveiliop56 merged 1 commit into
mainfrom
fix/rate-limiting

Conversation

@steveiliop56
Copy link
Copy Markdown
Member

@steveiliop56 steveiliop56 commented Dec 31, 2025

Discovered by @offw0rld

The rate limiting mechanism can be bypassed by sending login attempts through the forwardauth endpoint instead of the regular login endpoint. This allows an attacker to perform unlimited password guessing attempts without being blocked.

This pull request fixes the vulnerability by using the rate limit function in the context controller too.

Summary by CodeRabbit

Release Notes

  • Security Improvements

    • Account locking now operates on a per-username basis instead of IP address.
    • Added HTTP response headers indicating account lock status and reset time for better transparency during authentication lockouts.
  • Authentication Updates

    • Login attempts are now logged and tracked by username for improved security monitoring.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Dec 31, 2025

📝 Walkthrough

Walkthrough

The changes implement username-based account locking across the authentication flow, replacing IP-based rate limiting. New HTTP headers expose lock status and reset times. Login attempts are now recorded by username in both middleware and controller layers, with account-locked checks performed early in the request flow.

Changes

Cohort / File(s) Summary
Account Locking & Lock Status Headers
internal/controller/user_controller.go, internal/middleware/context_middleware.go
Replaces IP-based rate limiting with username-based IsAccountLocked checks; adds x-tinyauth-lock-locked and x-tinyauth-lock-reset HTTP headers computed from remaining lock duration; middleware performs account-locked check before user lookup; controller returns lock headers in 429 responses.
Login Attempt Recording
internal/controller/user_controller.go, internal/middleware/context_middleware.go
Records login attempts keyed by username (not IP) at multiple points: middleware logs attempts on auth failure; controller logs attempts on user-not-found, invalid-password, and successful login in login and TOTP flows.
Username-Centric Logging & Context
internal/controller/user_controller.go
Simplifies log messages to reference username only instead of username+IP; updates TOTP and login handlers to use req.Username and context.Username for locking and logging; adds time package import.

Sequence Diagram

sequenceDiagram
    participant Client
    participant Middleware as Auth Middleware
    participant AccountLock as Account Lock Service
    participant Controller as Login Controller
    participant Storage as User Storage

    Client->>Middleware: HTTP request with Basic Auth
    Middleware->>AccountLock: IsAccountLocked(username)
    alt Account is locked
        AccountLock-->>Middleware: true + remaining duration
        Middleware->>Middleware: Set x-tinyauth-lock-locked: true<br/>Set x-tinyauth-lock-reset: reset_time
        Middleware-->>Client: Short-circuit with headers
    else Account is not locked
        AccountLock-->>Middleware: false
        Middleware->>Storage: Look up user by username
        alt User not found
            Storage-->>Middleware: unknown/error
            Middleware->>AccountLock: RecordLoginAttempt(username, false)
            Middleware-->>Client: 401 Unauthorized
        else User found
            Storage-->>Middleware: user
            Middleware->>Middleware: Verify password
            alt Password invalid
                Middleware->>AccountLock: RecordLoginAttempt(username, false)
                Middleware-->>Client: 401 Unauthorized
            else Password valid
                Middleware->>AccountLock: RecordLoginAttempt(username, true)
                Middleware->>Controller: Continue to controller
                Controller-->>Client: Login success
            end
        end
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 Hop, hop! The locks now know your name,
No more IP-based game.
Username keys unlock the way,
With reset headers bright today!
Rate limiting, refined and true,
For every hop you try to do. 🔐

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: adding rate limiting in the forward auth endpoint to close a security vulnerability.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f564032 and ad62bb8.

📒 Files selected for processing (2)
  • internal/controller/user_controller.go
  • internal/middleware/context_middleware.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Analyze (go)
🔇 Additional comments (7)
internal/middleware/context_middleware.go (4)

120-128: Security fix looks correct — rate limiting now applies to forward auth.

This addresses the reported bypass where unlimited login attempts could be made through the forward auth endpoint. The implementation correctly:

  • Checks account lock status before authentication proceeds
  • Sets informative headers for downstream consumers
  • Short-circuits without recording additional attempts (preventing lock extension)

One note: unlike the controller which returns 429, this middleware sets headers but continues with c.Next(). This is consistent with the existing pattern where auth failures (unknown user, wrong password) also just continue without setting context, leaving the downstream handler to enforce access.


132-137: LGTM — failed attempts recorded for unknown users.

Recording login attempts for non-existent users is good security practice. It prevents username enumeration attacks and ensures rate limiting applies regardless of whether the account exists.


139-144: LGTM — failed password attempts tracked.

Correctly records failed password verification attempts for rate limiting purposes.


146-147: LGTM — successful attempts recorded.

Recording successful login attempts allows the rate limiter to reset the failure counter for this user.

internal/controller/user_controller.go (3)

64-77: LGTM — username-based rate limiting with informative headers.

The switch from IP-based to username-based locking is more effective against distributed attacks. The x-tinyauth-lock-* headers provide useful feedback to clients about lock status and reset time.


81-103: LGTM — login attempt tracking is correct.

The recording logic correctly handles all cases:

  • Unknown user → failed attempt
  • Invalid password → failed attempt
  • Valid password → successful attempt (even if TOTP is pending, as the password phase succeeded)

This separation allows password and TOTP phases to be rate-limited independently.


207-238: LGTM — TOTP rate limiting mirrors login handler pattern.

The TOTP handler correctly applies the same username-based rate limiting:

  • Lock check before validation
  • Failed attempts recorded for invalid codes
  • Successful attempts recorded on valid TOTP

This prevents brute-force attacks on TOTP codes.


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

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

@codecov
Copy link
Copy Markdown

codecov Bot commented Dec 31, 2025

Codecov Report

❌ Patch coverage is 62.50000% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 19.44%. Comparing base (f564032) to head (ad62bb8).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
internal/middleware/context_middleware.go 0.00% 10 Missing ⚠️
internal/controller/user_controller.go 90.90% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #555      +/-   ##
==========================================
- Coverage   19.49%   19.44%   -0.06%     
==========================================
  Files          39       39              
  Lines        2267     2273       +6     
==========================================
  Hits          442      442              
- Misses       1795     1803       +8     
+ Partials       30       28       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@steveiliop56 steveiliop56 merged commit f1e2b55 into main Dec 31, 2025
8 checks passed
@Rycochet Rycochet deleted the fix/rate-limiting branch April 1, 2026 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant