feat: add per-IP rate limiting to login endpoint #78

clawbot · 2026-03-17T10:27:22+01:00

clawbot commented

2026-03-17 10:27:22 +01:00

Summary

Adds per-IP rate limiting to POST /api/v1/login to prevent brute-force password attacks.

closes #35

What Changed

New package: `internal/ratelimit/`

A generic per-key token-bucket rate limiter built on golang.org/x/time/rate:

New(ratePerSec, burst) creates a limiter with automatic background cleanup of stale entries
Allow(key) checks if a request from the given key should be permitted
Stop() terminates the background sweep goroutine
Stale entries (unused for 15 minutes) are pruned every 10 minutes

The login handler (internal/handlers/auth.go) now:

Extracts the client IP from X-Forwarded-For, X-Real-IP, or RemoteAddr
Checks the per-IP rate limiter before processing the login
Returns 429 Too Many Requests with a Retry-After: 1 header when the limit is exceeded

Configuration

Two new environment variables (via Viper):

Variable	Default	Description
`LOGIN_RATE_LIMIT`	`1`	Allowed login attempts per second per IP
`LOGIN_RATE_BURST`	`5`	Maximum burst of login attempts per IP

Scope

Per sneak's instruction, only the login endpoint is rate-limited. Session creation and registration use hashcash proof-of-work instead.

Tests

6 unit tests for the ratelimit package (constructor, burst, burst exceeded, key isolation, key tracking, stop)
2 integration tests in api_test.go:
- TestLoginRateLimitExceeded: exhausts burst with rapid requests, verifies 429 response and Retry-After header
- TestLoginRateLimitAllowsNormalUse: verifies normal login still works

README

Added "Login Rate Limiting" subsection under "Rate Limiting & Abuse Prevention"
Added LOGIN_RATE_LIMIT and LOGIN_RATE_BURST to the Configuration table

## Summary Adds per-IP rate limiting to `POST /api/v1/login` to prevent brute-force password attacks. closes #35 ## What Changed ### New package: `internal/ratelimit/` A generic per-key token-bucket rate limiter built on `golang.org/x/time/rate`: - `New(ratePerSec, burst)` creates a limiter with automatic background cleanup of stale entries - `Allow(key)` checks if a request from the given key should be permitted - `Stop()` terminates the background sweep goroutine - Stale entries (unused for 15 minutes) are pruned every 10 minutes ### Login handler integration The login handler (`internal/handlers/auth.go`) now: 1. Extracts the client IP from `X-Forwarded-For`, `X-Real-IP`, or `RemoteAddr` 2. Checks the per-IP rate limiter before processing the login 3. Returns **429 Too Many Requests** with a `Retry-After: 1` header when the limit is exceeded ### Configuration Two new environment variables (via Viper): | Variable | Default | Description | |---|---|---| | `LOGIN_RATE_LIMIT` | `1` | Allowed login attempts per second per IP | | `LOGIN_RATE_BURST` | `5` | Maximum burst of login attempts per IP | ### Scope Per [sneak's instruction](https://git.eeqj.de/sneak/chat/issues/35), only the login endpoint is rate-limited. Session creation and registration use hashcash proof-of-work instead. ## Tests - 6 unit tests for the `ratelimit` package (constructor, burst, burst exceeded, key isolation, key tracking, stop) - 2 integration tests in `api_test.go`: - `TestLoginRateLimitExceeded`: exhausts burst with rapid requests, verifies 429 response and `Retry-After` header - `TestLoginRateLimitAllowsNormalUse`: verifies normal login still works ## README - Added "Login Rate Limiting" subsection under "Rate Limiting & Abuse Prevention" - Added `LOGIN_RATE_LIMIT` and `LOGIN_RATE_BURST` to the Configuration table

clawbot added 1 commit 2026-03-17 10:27:23 +01:00

feat: add per-IP rate limiting to login endpoint

check / check (push) Successful in 1m15s

Details

ba943d95ed

Add a token-bucket rate limiter (golang.org/x/time/rate) that limits
login attempts per client IP on POST /api/v1/login. Returns 429 Too
Many Requests with a Retry-After header when the limit is exceeded.

Configurable via LOGIN_RATE_LIMIT (requests/sec, default 1) and
LOGIN_RATE_BURST (burst size, default 5). Stale per-IP entries are
automatically cleaned up every 10 minutes.

Only the login endpoint is rate-limited per sneak's instruction —
session creation and registration use hashcash proof-of-work instead.

clawbot added the needs-review label 2026-03-17 10:27:52 +01:00

clawbot reviewed 2026-03-17 10:42:40 +01:00

clawbot left a comment

Requirements Checklist (#35)

#	Requirement	Status
1	Rate limiting on `POST /api/v1/login` only (per sneak's instruction)	✅
2	Session creation and registration NOT rate-limited	✅
3	Per-IP rate limiting	✅
4	Token bucket algorithm via `golang.org/x/time/rate`	✅
5	Configurable rate + burst via env vars	✅
6	Returns 429 + `Retry-After` header on limit exceeded	✅
7	Background cleanup of stale per-IP entries	✅
8	Unit tests for ratelimit package	✅ (6 tests)
9	Integration tests for login endpoint	✅ (2 tests)
10	README documentation	✅

Policy Compliance

Check	Status
External dep pinned in go.sum	✅ `golang.org/x/time v0.6.0` hash present
All exported types/functions have doc comments	✅
All exported types/functions have tests	✅
No code in `cmd/`	✅
New code in `internal/`	✅ `internal/ratelimit/`
Tests use `testing.T`, no assertion libraries	✅
Tests are parallel	✅
Blank line before returns	✅
No inline error handling	✅ (inline `if val := ...; val != ""` is value-checking, not error handling)
`nolint` directives justified	✅ `mnd` for `SplitN(,2)`, `exhaustruct` for existing pattern

Security Analysis: IP Extraction

clientIP() checks X-Forwarded-For → X-Real-IP → RemoteAddr. Without a trusted reverse proxy, these headers are client-controlled — an attacker could rotate X-Forwarded-For values to bypass rate limiting, or set a victim's IP to deny them service.

Assessment: This is the standard trade-off for IP-based rate limiting. Using only RemoteAddr would break rate limiting behind any reverse proxy (all clients share the proxy IP). The codebase has no existing proxy trust configuration. This is a reasonable first implementation. If sneak wants hardened proxy trust (trusted CIDR allowlist for header stripping), that's a follow-up issue, not a blocker for this PR.

Build Result

✅ docker build . — PASSED

All existing tests pass
6 new ratelimit unit tests pass
2 new integration tests pass (TestLoginRateLimitExceeded, TestLoginRateLimitAllowsNormalUse)
Lint, format, build all clean

Code Quality Notes

Clean separation: generic ratelimit.Limiter in internal/ratelimit/, login-specific wiring in handlers.go
Proper lifecycle management: Stop() called in stopCleanup() via fx OnStop hook
Defensive defaults: loginRate <= 0 falls back to DefaultRate
Rate limiter check happens before JSON parsing — correct early-reject pattern
entry type is unexported (correctly private)
Sweep goroutine uses select on both ticker and stop channel — clean shutdown

Verdict: PASS ✅

Clean implementation, correct scoping per sneak's instructions, comprehensive tests, full policy compliance, and the build is green.

## Review: PR #78 — Login Rate Limiting ### Requirements Checklist ([#35](https://git.eeqj.de/sneak/chat/issues/35)) | # | Requirement | Status | |---|---|---| | 1 | Rate limiting on `POST /api/v1/login` only (per [sneak's instruction](https://git.eeqj.de/sneak/chat/issues/35#issuecomment-11610)) | ✅ | | 2 | Session creation and registration NOT rate-limited | ✅ | | 3 | Per-IP rate limiting | ✅ | | 4 | Token bucket algorithm via `golang.org/x/time/rate` | ✅ | | 5 | Configurable rate + burst via env vars | ✅ | | 6 | Returns 429 + `Retry-After` header on limit exceeded | ✅ | | 7 | Background cleanup of stale per-IP entries | ✅ | | 8 | Unit tests for ratelimit package | ✅ (6 tests) | | 9 | Integration tests for login endpoint | ✅ (2 tests) | | 10 | README documentation | ✅ | ### Policy Compliance | Check | Status | |---|---| | External dep pinned in go.sum | ✅ `golang.org/x/time v0.6.0` hash present | | All exported types/functions have doc comments | ✅ | | All exported types/functions have tests | ✅ | | No code in `cmd/` | ✅ | | New code in `internal/` | ✅ `internal/ratelimit/` | | Tests use `testing.T`, no assertion libraries | ✅ | | Tests are parallel | ✅ | | Blank line before returns | ✅ | | No inline error handling | ✅ (inline `if val := ...; val != ""` is value-checking, not error handling) | | `nolint` directives justified | ✅ `mnd` for `SplitN(,2)`, `exhaustruct` for existing pattern | ### Security Analysis: IP Extraction `clientIP()` checks `X-Forwarded-For` → `X-Real-IP` → `RemoteAddr`. Without a trusted reverse proxy, these headers are client-controlled — an attacker could rotate `X-Forwarded-For` values to bypass rate limiting, or set a victim's IP to deny them service. **Assessment:** This is the standard trade-off for IP-based rate limiting. Using only `RemoteAddr` would break rate limiting behind any reverse proxy (all clients share the proxy IP). The codebase has no existing proxy trust configuration. This is a reasonable first implementation. If sneak wants hardened proxy trust (trusted CIDR allowlist for header stripping), that's a follow-up issue, not a blocker for this PR. ### Build Result ✅ `docker build .` — **PASSED** - All existing tests pass - 6 new `ratelimit` unit tests pass - 2 new integration tests pass (`TestLoginRateLimitExceeded`, `TestLoginRateLimitAllowsNormalUse`) - Lint, format, build all clean ### Code Quality Notes - Clean separation: generic `ratelimit.Limiter` in `internal/ratelimit/`, login-specific wiring in `handlers.go` - Proper lifecycle management: `Stop()` called in `stopCleanup()` via fx OnStop hook - Defensive defaults: `loginRate <= 0` falls back to `DefaultRate` - Rate limiter check happens before JSON parsing — correct early-reject pattern - `entry` type is unexported (correctly private) - Sweep goroutine uses `select` on both ticker and stop channel — clean shutdown ### Verdict: **PASS** ✅ Clean implementation, correct scoping per sneak's instructions, comprehensive tests, full policy compliance, and the build is green.

clawbot added merge-ready and removed needs-review labels 2026-03-17 10:43:16 +01:00

sneak was assigned by clawbot

2026-03-17 10:43:16 +01:00

sneak commented

2026-03-17 12:46:06 +01:00

fix the conflict, and update the README in this PR to document the security (client-controlled req headers) issue when using this without a properly configured reverse proxy.

sneak removed their assignment 2026-03-17 12:46:14 +01:00

clawbot was assigned by sneak

2026-03-17 12:46:14 +01:00

clawbot added needs-rework and removed merge-ready labels 2026-03-17 12:47:01 +01:00

clawbot force-pushed feature/login-rate-limit from ba943d95ed to d0e925bf70

2026-03-17 12:51:16 +01:00

Compare

clawbot commented

2026-03-17 12:51:54 +01:00

Rework Complete

What was done

Rebased onto main — resolved merge conflict in internal/handlers/handlers.go where PR #80 (runtime stats) and this PR both modified imports, struct fields, and the constructor. Both stats and ratelimit are now present.
Added reverse proxy security note to the "Login Rate Limiting" section of README.md, documenting:
- clientIP() trusts X-Forwarded-For and X-Real-IP headers
- These headers are client-controlled
- Without a reverse proxy that strips/overwrites these headers, an attacker can bypass rate limiting by rotating header values, or DoS a victim by spoofing their IP
- Recommendation to deploy behind a reverse proxy (nginx, Caddy, Traefik) that sets these headers correctly

Build verification

✅ docker build . passes — all tests green (including ratelimit unit tests and login rate limit integration tests), lint/format clean, binary builds.

## Rework Complete ### What was done 1. **Rebased onto `main`** — resolved merge conflict in `internal/handlers/handlers.go` where PR [#80](https://git.eeqj.de/sneak/chat/pulls/80) (runtime stats) and this PR both modified imports, struct fields, and the constructor. Both `stats` and `ratelimit` are now present. 2. **Added reverse proxy security note** to the "Login Rate Limiting" section of README.md, documenting: - `clientIP()` trusts `X-Forwarded-For` and `X-Real-IP` headers - These headers are client-controlled - Without a reverse proxy that strips/overwrites these headers, an attacker can bypass rate limiting by rotating header values, or DoS a victim by spoofing their IP - Recommendation to deploy behind a reverse proxy (nginx, Caddy, Traefik) that sets these headers correctly ### Build verification ✅ `docker build .` passes — all tests green (including ratelimit unit tests and login rate limit integration tests), lint/format clean, binary builds.

clawbot added needs-review and removed needs-rework labels 2026-03-17 12:52:43 +01:00

clawbot reviewed 2026-03-17 12:56:07 +01:00

clawbot left a comment

Rework Verification

Rework Item	Status
Rebase onto `main` (resolve conflict with PR #80)	✅ Clean — both `ratelimit` and `stats` imports, struct fields, and constructor present
Security documentation for client-controlled headers	✅ Accurate and thorough

Rebase Integrity

The merge conflict in internal/handlers/handlers.go was resolved correctly:

Both ratelimit and stats imports present in the import block
Handlers struct has both loginLimiter *ratelimit.Limiter and stats *stats.Tracker fields
Params struct includes Stats *stats.Tracker (from PR #80)
Constructor initializes both fields: loginLimiter: ratelimit.New(...) and stats: params.Stats
stopCleanup() calls loginLimiter.Stop() — lifecycle management intact
No broken imports, no missing struct fields

Security Documentation Verification

The README security warning accurately matches the clientIP() implementation in internal/handlers/auth.go:

README Claim	Code Reality
Checks `X-Forwarded-For` first	✅ `request.Header.Get("X-Forwarded-For")` checked first
Then `X-Real-IP`	✅ Checked second
Falls back to TCP `RemoteAddr`	✅ `net.SplitHostPort(request.RemoteAddr)` as final fallback
Headers are client-controlled	✅ Correct — no trusted proxy CIDR filtering
Bypass by rotating `X-Forwarded-For` values	✅ Accurate — each value creates a separate rate limit bucket
DoS by spoofing victim's IP	✅ Accurate — exhausts victim's bucket
Recommends reverse proxy (nginx, Caddy, Traefik)	✅ Practical and correct

The documentation is clear, technically accurate, and appropriately scoped. The blockquote callout format with ⚠️ makes the warning visually prominent.

Policy Compliance

Check	Status
README prose ≤80 columns	✅
External dep pinned (`golang.org/x/time v0.6.0` hash in go.sum)	✅
Doc comments on all exported types/functions	✅
Tests use `testing.T`, no assertion libraries	✅
Tests are parallel	✅
`nolint` directives justified	✅ `mnd` for `SplitN(,2)`, `exhaustruct` for existing pattern
No code in `cmd/`	✅
New code in `internal/`	✅

Build Result

✅ docker build . — PASSED

All 6 ratelimit unit tests pass
TestLoginRateLimitExceeded — confirms 429 with Retry-After header
TestLoginRateLimitAllowsNormalUse — confirms normal login works
All pre-existing tests pass (including stats tests from PR #80)
Lint, format, build all clean

Verdict: PASS ✅

Rebase is clean, security documentation accurately reflects the code, all tests green, build passes. The rework addresses both of sneak's requests (conflict resolution + security documentation).

## Post-Rework Review: PR #78 — Login Rate Limiting ### Rework Verification | Rework Item | Status | |---|---| | Rebase onto `main` (resolve conflict with [PR #80](https://git.eeqj.de/sneak/chat/pulls/80)) | ✅ Clean — both `ratelimit` and `stats` imports, struct fields, and constructor present | | Security documentation for client-controlled headers | ✅ Accurate and thorough | ### Rebase Integrity The merge conflict in `internal/handlers/handlers.go` was resolved correctly: - Both `ratelimit` and `stats` imports present in the import block - `Handlers` struct has both `loginLimiter *ratelimit.Limiter` and `stats *stats.Tracker` fields - `Params` struct includes `Stats *stats.Tracker` (from [PR #80](https://git.eeqj.de/sneak/chat/pulls/80)) - Constructor initializes both fields: `loginLimiter: ratelimit.New(...)` and `stats: params.Stats` - `stopCleanup()` calls `loginLimiter.Stop()` — lifecycle management intact - No broken imports, no missing struct fields ### Security Documentation Verification The README security warning accurately matches the `clientIP()` implementation in `internal/handlers/auth.go`: | README Claim | Code Reality | |---|---| | Checks `X-Forwarded-For` first | ✅ `request.Header.Get("X-Forwarded-For")` checked first | | Then `X-Real-IP` | ✅ Checked second | | Falls back to TCP `RemoteAddr` | ✅ `net.SplitHostPort(request.RemoteAddr)` as final fallback | | Headers are client-controlled | ✅ Correct — no trusted proxy CIDR filtering | | Bypass by rotating `X-Forwarded-For` values | ✅ Accurate — each value creates a separate rate limit bucket | | DoS by spoofing victim's IP | ✅ Accurate — exhausts victim's bucket | | Recommends reverse proxy (nginx, Caddy, Traefik) | ✅ Practical and correct | The documentation is clear, technically accurate, and appropriately scoped. The blockquote callout format with ⚠️ makes the warning visually prominent. ### Policy Compliance | Check | Status | |---|---| | README prose ≤80 columns | ✅ | | External dep pinned (`golang.org/x/time v0.6.0` hash in go.sum) | ✅ | | Doc comments on all exported types/functions | ✅ | | Tests use `testing.T`, no assertion libraries | ✅ | | Tests are parallel | ✅ | | `nolint` directives justified | ✅ `mnd` for `SplitN(,2)`, `exhaustruct` for existing pattern | | No code in `cmd/` | ✅ | | New code in `internal/` | ✅ | ### Build Result ✅ `docker build .` — **PASSED** - All 6 ratelimit unit tests pass - `TestLoginRateLimitExceeded` — confirms 429 with `Retry-After` header - `TestLoginRateLimitAllowsNormalUse` — confirms normal login works - All pre-existing tests pass (including stats tests from [PR #80](https://git.eeqj.de/sneak/chat/pulls/80)) - Lint, format, build all clean ### Verdict: **PASS** ✅ Rebase is clean, security documentation accurately reflects the code, all tests green, build passes. The rework addresses both of sneak's requests (conflict resolution + security documentation).

clawbot added merge-ready and removed needs-review labels 2026-03-17 12:57:15 +01:00

clawbot removed their assignment 2026-03-17 12:57:15 +01:00

sneak was assigned by clawbot

2026-03-17 12:57:15 +01:00

sneak commented

2026-03-18 03:42:36 +01:00

@clawbot fix conflict pls

check / check (push) Successful in 1m8s

Required

Details

This pull request has changes conflicting with the target branch.

internal/handlers/handlers.go

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin feature/login-rate-limit:feature/login-rate-limit

git checkout feature/login-rate-limit

Sign in to join this conversation.

2 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/chat#78

feat: add per-IP rate limiting to login endpoint #78

Summary

What Changed

New package: internal/ratelimit/

Login handler integration

Configuration

Scope

Tests

README

Review: PR #78 — Login Rate Limiting

Requirements Checklist (#35)

Policy Compliance

Security Analysis: IP Extraction

Build Result

Code Quality Notes

Verdict: PASS ✅

Rework Complete

What was done

Build verification

Post-Rework Review: PR #78 — Login Rate Limiting

Rework Verification

Rebase Integrity

Security Documentation Verification

Policy Compliance

Build Result

Verdict: PASS ✅

Checkout

New package: `internal/ratelimit/`