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
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.
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
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.
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.
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 sneak2026-03-17 12:46:14 +01:00
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.
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).
Rebased feature/login-rate-limit onto main, resolving merge conflicts in internal/handlers/handlers.go.
Conflict resolution: The Handlers struct and New() constructor now include both channelHashcash (from #79) and loginLimiter (from this PR). README.md and api_test.go auto-merged cleanly.
Build status:docker build . fails due to TestLoginValid timing out at 30s — but this is a pre-existing issue on main (confirmed: main HEAD bf4d63b also fails the same way). This PR's changes did not introduce the failure.
Rebased `feature/login-rate-limit` onto `main`, resolving merge conflicts in `internal/handlers/handlers.go`.
**Conflict resolution:** The `Handlers` struct and `New()` constructor now include both `channelHashcash` (from [#79](https://git.eeqj.de/sneak/chat/pulls/79)) and `loginLimiter` (from this PR). `README.md` and `api_test.go` auto-merged cleanly.
**Build status:** `docker build .` fails due to `TestLoginValid` timing out at 30s — but this is a **pre-existing issue on `main`** (confirmed: `main` HEAD `bf4d63b` also fails the same way). This PR's changes did not introduce the failure.
1. Branch not rebased onto current main — merge conflicts present
The PR branch (feature/login-rate-limit) is based on bf4d63b, but main is now at db3d23c (which includes PR #82 — username/hostname support with bcrypt MinCost for tests). Attempting to merge produces conflicts in:
README.md
internal/config/config.go
internal/handlers/api_test.go
Gitea confirms: Mergeable: false. The PR cannot be merged as-is.
sneak's last comment (2026-03-18) was "@clawbot fix conflict pls" — this remains unaddressed against current main. The reworker's rebase resolved against stale main (bf4d63b) instead of current main (db3d23c).
Rate limiting on POST /api/v1/login only (per sneak's instruction)
✅ Met
2
Session creation and registration NOT rate-limited
✅ Met
3
Per-IP rate limiting
✅ Met
4
Token bucket algorithm via golang.org/x/time/rate
✅ Met
5
Configurable rate + burst via env vars
✅ Met
6
Returns 429 + Retry-After header on limit exceeded
✅ Met
7
Background cleanup of stale per-IP entries
✅ Met
8
Unit tests for ratelimit package
✅ Met (6 tests)
9
Integration tests for login endpoint
✅ Met (2 tests)
10
README documentation
✅ Met
11
Security docs for reverse proxy (sneak's rework request)
✅ Met
12
Rebase onto current main (sneak's second request)
❌Unmet — conflicts with current main
Build Result
docker build . on the PR branch alone: ✅ PASSES (all tests green, lint clean, 22.5s handlers).
However, the branch cannot be merged into current main due to conflicts. A successful isolated build is not sufficient — the branch must be mergeable.
Code Quality Assessment (for reference — not sufficient to override FAIL)
The implementation itself is solid:
Clean separation: generic ratelimit.Limiter in internal/ratelimit/, login-specific wiring in handlers.go
Proper lifecycle management: Stop() called in stopCleanup()
Defensive defaults: loginRate <= 0 falls back to DefaultRate
Rate limiter check happens before JSON parsing — correct early-reject
External dep golang.org/x/time v0.6.0 pinned in go.sum✅
All exported types/functions have doc comments ✅
Tests use testing.T, no assertion libs, parallel ✅
No modifications to .golangci.yml, Makefile, or CI config ✅
nolint directives justified (mnd for SplitN(,2), exhaustruct for existing pattern) ✅
README security warning is thorough and technically accurate ✅
Verdict: FAIL❌
The branch has merge conflicts with current main (db3d23c) and cannot be merged. sneak's instruction to fix conflicts is not addressed against the current main HEAD. Needs a rebase onto main that incorporates PR #82 changes (username/hostname support, bcrypt MinCost for tests, conditional -v in Makefile).
## Review: PR #78 — Login Rate Limiting (post-rebase attempt)
### Policy Divergences
**1. Branch not rebased onto current `main` — merge conflicts present**
The PR branch (`feature/login-rate-limit`) is based on `bf4d63b`, but `main` is now at `db3d23c` (which includes [PR #82](https://git.eeqj.de/sneak/chat/pulls/82) — username/hostname support with bcrypt MinCost for tests). Attempting to merge produces conflicts in:
- `README.md`
- `internal/config/config.go`
- `internal/handlers/api_test.go`
Gitea confirms: `Mergeable: false`. The PR cannot be merged as-is.
sneak's last comment (2026-03-18) was "@clawbot fix conflict pls" — this remains **unaddressed** against current `main`. The reworker's rebase resolved against stale `main` (`bf4d63b`) instead of current `main` (`db3d23c`).
### Requirements Checklist ([issue #35](https://git.eeqj.de/sneak/chat/issues/35))
| # | Requirement | Status |
|---|---|---|
| 1 | Rate limiting on `POST /api/v1/login` only (per sneak's instruction) | ✅ Met |
| 2 | Session creation and registration NOT rate-limited | ✅ Met |
| 3 | Per-IP rate limiting | ✅ Met |
| 4 | Token bucket algorithm via `golang.org/x/time/rate` | ✅ Met |
| 5 | Configurable rate + burst via env vars | ✅ Met |
| 6 | Returns 429 + `Retry-After` header on limit exceeded | ✅ Met |
| 7 | Background cleanup of stale per-IP entries | ✅ Met |
| 8 | Unit tests for ratelimit package | ✅ Met (6 tests) |
| 9 | Integration tests for login endpoint | ✅ Met (2 tests) |
| 10 | README documentation | ✅ Met |
| 11 | Security docs for reverse proxy (sneak's rework request) | ✅ Met |
| 12 | Rebase onto current main (sneak's second request) | ❌ **Unmet** — conflicts with current `main` |
### Build Result
`docker build .` on the **PR branch alone**: ✅ PASSES (all tests green, lint clean, 22.5s handlers).
However, the branch **cannot be merged** into current `main` due to conflicts. A successful isolated build is not sufficient — the branch must be mergeable.
### Code Quality Assessment (for reference — not sufficient to override FAIL)
The implementation itself is solid:
- Clean separation: generic `ratelimit.Limiter` in `internal/ratelimit/`, login-specific wiring in `handlers.go`
- Proper lifecycle management: `Stop()` called in `stopCleanup()`
- Defensive defaults: `loginRate <= 0` falls back to `DefaultRate`
- Rate limiter check happens before JSON parsing — correct early-reject
- External dep `golang.org/x/time v0.6.0` pinned in `go.sum` ✅
- All exported types/functions have doc comments ✅
- Tests use `testing.T`, no assertion libs, parallel ✅
- No modifications to `.golangci.yml`, `Makefile`, or CI config ✅
- `nolint` directives justified (`mnd` for `SplitN(,2)`, `exhaustruct` for existing pattern) ✅
- README security warning is thorough and technically accurate ✅
### Verdict: **FAIL** ❌
The branch has merge conflicts with current `main` (`db3d23c`) and cannot be merged. sneak's instruction to fix conflicts is not addressed against the current `main` HEAD. Needs a rebase onto `main` that incorporates [PR #82](https://git.eeqj.de/sneak/chat/pulls/82) changes (username/hostname support, bcrypt MinCost for tests, conditional `-v` in Makefile).
Rebased `feature/login-rate-limit` onto current main (`db3d23c`).
**Conflicts resolved:**
- `README.md` — kept both OPER config vars (from [PR #82](https://git.eeqj.de/sneak/chat/pulls/82)) and rate-limit config vars
- `internal/config/config.go` — merged OPER and rate-limit fields in struct, defaults, and config initialization (3 conflict regions)
- `internal/handlers/api_test.go` — kept all username/hostname test helpers and tests from main alongside rate-limit tests
**Additional fixes:**
- Removed duplicate `clientIP()` function from `auth.go` (identical copy already exists in `api.go` from PR #82)
- Extracted `executeLogin()` helper from `handleLogin()` to satisfy `funlen` lint rule (was 86 lines, limit 80)
`docker build .` passes — lint clean (0 issues), all tests green.
Rebase onto db3d23c is clean — no lost code, no conflict markers, all main branch features (username/hostname/OPER from PR #82) preserved alongside the rate limiting implementation. Build is green. Implementation is correct and scoped per sneak's instructions.
## Review: PR #78 — Login Rate Limiting (post-rebase onto db3d23c)
### Rebase Integrity
| Check | Status |
|---|---|
| Main (`db3d23c`) is ancestor of PR branch | ✅ Clean — `git merge-base --is-ancestor` confirms |
| No conflict markers in any file | ✅ Verified via grep |
| All tests from main preserved on PR branch | ✅ Every `func Test*` from main present, plus 2 new |
| Config struct has both OPER + rate-limit fields | ✅ `OperName`, `OperPassword`, `LoginRateLimit`, `LoginRateBurst` all present |
| Handlers struct has all fields (stats, hashcash, ratelimit) | ✅ All present |
| `clientIP()` exists only in `api.go` (no duplicate in `auth.go`) | ✅ Duplicate correctly removed |
| Username/hostname tests preserved (`TestWhoisShowsHostInfo`, `TestWhoShowsHostInfo`, `TestSessionUsernameDefault`, `TestNamesShowsHostmask`, OPER tests) | ✅ All present |
| README has both OPER and rate-limit config vars | ✅ |
### Requirements Checklist ([issue #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)) | ✅ Only `handleLogin` references `loginLimiter` |
| 2 | Session creation and registration NOT rate-limited | ✅ No references in `api.go` or `session.go` |
| 3 | Per-IP rate limiting via token bucket (`golang.org/x/time/rate`) | ✅ |
| 4 | Configurable rate + burst via `LOGIN_RATE_LIMIT` / `LOGIN_RATE_BURST` env vars | ✅ |
| 5 | Returns 429 + `Retry-After: 1` header when limit exceeded | ✅ |
| 6 | Background cleanup of stale per-IP entries (15 min TTL, 10 min sweep) | ✅ |
| 7 | Unit tests for ratelimit package | ✅ 6 tests, all `t.Parallel()` |
| 8 | Integration tests for login endpoint | ✅ 2 tests (`TestLoginRateLimitExceeded`, `TestLoginRateLimitAllowsNormalUse`) |
| 9 | README documentation (config vars + security warning) | ✅ |
| 10 | Security docs for reverse proxy (sneak's rework request) | ✅ Accurate warning about client-controlled headers |
### Policy Compliance
| Check | Status |
|---|---|
| `.golangci.yml` unmodified | ✅ |
| `Makefile` unmodified | ✅ |
| CI config unmodified | ✅ |
| External dep `golang.org/x/time v0.6.0` pinned by hash in `go.sum` | ✅ |
| All exported types/functions have doc comments | ✅ |
| Tests use `testing.T`, no assertion libraries | ✅ |
| Unit tests parallel | ✅ All 6 ratelimit tests |
| Integration tests follow existing non-parallel convention | ✅ Consistent with codebase |
| New code in `internal/` (not `cmd/`) | ✅ `internal/ratelimit/` |
| `nolint` directives: only pre-existing (`exhaustruct`, `mnd`, `contextcheck`) | ✅ No new nolint added |
| `handleLogin` 55 lines, `executeLogin` 44 lines — both under funlen 80 | ✅ |
### Code Quality Notes
- `executeLogin` extraction is clean — all original logic preserved, `payload.Nick`/`payload.Password` correctly replaced with `nick`/`password` params
- Rate limit check happens before JSON parsing — correct early-reject pattern
- `Limiter.Allow()` holds mutex only for map lookup, calls `rate.Limiter.Allow()` outside lock — correct since `rate.Limiter` is goroutine-safe
- `Stop()` properly called in `stopCleanup()` with nil guard
- Defensive defaults: `loginRate <= 0` falls back to `DefaultRate`
### Build Result
✅ `docker build .` — **PASSED** (lint clean, all tests green, binary builds)
### Verdict: **PASS** ✅
Rebase onto `db3d23c` is clean — no lost code, no conflict markers, all main branch features (username/hostname/OPER from [PR #82](https://git.eeqj.de/sneak/chat/pulls/82)) preserved alongside the rate limiting implementation. Build is green. Implementation is correct and scoped per sneak's instructions.
Rebased onto current main after PR #84 merge (auth refactor: cookies replace Bearer tokens, /register removed, PASS command added).
Conflicts resolved:
internal/handlers/auth.go — executeLogin() response: removed token field from JSON body (now set via setAuthCookie), used nick parameter instead of payload.Nick (correct scope in extracted function)
Auto-merged files updated for compatibility:
internal/handlers/api_test.go — TestLoginRateLimitAllowsNormalUse: replaced removed /api/v1/register call with createSession() + PASS command (matching current test patterns)
Verified:docker build . passes — lint, fmt-check, all tests green including rate limit tests.
Rebased onto current `main` after [PR #84](https://git.eeqj.de/sneak/chat/pulls/84) merge (auth refactor: cookies replace Bearer tokens, `/register` removed, PASS command added).
**Conflicts resolved:**
- `internal/handlers/auth.go` — `executeLogin()` response: removed `token` field from JSON body (now set via `setAuthCookie`), used `nick` parameter instead of `payload.Nick` (correct scope in extracted function)
**Auto-merged files updated for compatibility:**
- `internal/handlers/api_test.go` — `TestLoginRateLimitAllowsNormalUse`: replaced removed `/api/v1/register` call with `createSession()` + PASS command (matching current test patterns)
**Verified:** `docker build .` passes — lint, fmt-check, all tests green including rate limit tests.
Review: PR #78 — per-IP rate limiting on login endpoint
Verdict: PASS✅
Rebase Integrity
Clean single commit (1eee5f5) on top of current main. No conflict markers found. No stale references to Bearer tokens or /register endpoint. All PR #84 auth refactor code (cookie-based auth, setAuthCookie, doRequestAuth with cookies) is intact.
Code Review
internal/ratelimit/ratelimit.go — Clean token-bucket implementation wrapping golang.org/x/time/rate. Per-key map with mutex, background sweep goroutine with configurable TTL, proper Stop() for cleanup. The Allow() method correctly releases the mutex before calling ent.limiter.Allow() (safe because rate.Limiter is goroutine-safe). No issues.
internal/handlers/auth.go — Rate limit check is the first thing in handleLogin, before JSON parsing or bcrypt. Returns 429 with Retry-After: 1 header. The executeLogin extraction is clean — handleLogin handles parsing + validation + rate limiting, then delegates to executeLogin(nick, password). All references use nick parameter, not payload.Nick. Uses setAuthCookie (cookie auth, not Bearer). Correct.
internal/handlers/handlers.go — loginLimiter field properly initialized in New() with config values (with fallback to defaults if ≤0). Stop() called in stopCleanup(). Clean integration.
internal/config/config.go — LOGIN_RATE_LIMIT (float64) and LOGIN_RATE_BURST (int) with sensible defaults (1 req/s, burst 5).
Scope — Rate limiting is applied ONLY to handleLogin. Session creation endpoint is not rate-limited (protected by hashcash instead). Matches issue #35 scope.
Tests
internal/ratelimit/ratelimit_test.go — 6 unit tests covering creation, burst allowance, burst exhaustion, key isolation, key counting, and stop safety. All use t.Parallel(). Tests with rate=0 for deterministic burst-only behavior — good approach.
internal/handlers/api_test.go — Two integration tests:
TestLoginRateLimitExceeded: Exhausts burst of 5 with bad logins, verifies 6th gets 429 + Retry-After header.
TestLoginRateLimitAllowsNormalUse: Creates session via createSession() + PASS command (cookie-based, no /register), then verifies single login succeeds with 200.
README properly documents the env vars, the token-bucket algorithm, the 429 response, and includes a thorough security warning about X-Forwarded-For spoofing and reverse proxy requirements. The rationale for rate limiting login (bcrypt cost) vs. not rate limiting session creation (hashcash) is clearly explained.
## Review: PR #78 — per-IP rate limiting on login endpoint
**Verdict: PASS** ✅
### Rebase Integrity
Clean single commit (`1eee5f5`) on top of current `main`. No conflict markers found. No stale references to `Bearer` tokens or `/register` endpoint. All PR #84 auth refactor code (cookie-based auth, `setAuthCookie`, `doRequestAuth` with cookies) is intact.
### Code Review
**`internal/ratelimit/ratelimit.go`** — Clean token-bucket implementation wrapping `golang.org/x/time/rate`. Per-key map with mutex, background sweep goroutine with configurable TTL, proper `Stop()` for cleanup. The `Allow()` method correctly releases the mutex before calling `ent.limiter.Allow()` (safe because `rate.Limiter` is goroutine-safe). No issues.
**`internal/handlers/auth.go`** — Rate limit check is the first thing in `handleLogin`, before JSON parsing or bcrypt. Returns 429 with `Retry-After: 1` header. The `executeLogin` extraction is clean — `handleLogin` handles parsing + validation + rate limiting, then delegates to `executeLogin(nick, password)`. All references use `nick` parameter, not `payload.Nick`. Uses `setAuthCookie` (cookie auth, not Bearer). Correct.
**`internal/handlers/handlers.go`** — `loginLimiter` field properly initialized in `New()` with config values (with fallback to defaults if ≤0). `Stop()` called in `stopCleanup()`. Clean integration.
**`internal/config/config.go`** — `LOGIN_RATE_LIMIT` (float64) and `LOGIN_RATE_BURST` (int) with sensible defaults (1 req/s, burst 5).
**Scope** — Rate limiting is applied ONLY to `handleLogin`. Session creation endpoint is not rate-limited (protected by hashcash instead). Matches [issue #35](https://git.eeqj.de/sneak/chat/issues/35) scope.
### Tests
**`internal/ratelimit/ratelimit_test.go`** — 6 unit tests covering creation, burst allowance, burst exhaustion, key isolation, key counting, and stop safety. All use `t.Parallel()`. Tests with rate=0 for deterministic burst-only behavior — good approach.
**`internal/handlers/api_test.go`** — Two integration tests:
- `TestLoginRateLimitExceeded`: Exhausts burst of 5 with bad logins, verifies 6th gets 429 + `Retry-After` header.
- `TestLoginRateLimitAllowsNormalUse`: Creates session via `createSession()` + PASS command (cookie-based, no `/register`), then verifies single login succeeds with 200.
### Build
`docker build .` passes — lint, fmt-check, tests, compilation all green.
### Documentation
README properly documents the env vars, the token-bucket algorithm, the 429 response, and includes a thorough security warning about X-Forwarded-For spoofing and reverse proxy requirements. The rationale for rate limiting login (bcrypt cost) vs. not rate limiting session creation (hashcash) is clearly explained.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
clawbot
Summary
Adds per-IP rate limiting to
POST /api/v1/loginto 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 entriesAllow(key)checks if a request from the given key should be permittedStop()terminates the background sweep goroutineLogin handler integration
The login handler (
internal/handlers/auth.go) now:X-Forwarded-For,X-Real-IP, orRemoteAddrRetry-After: 1header when the limit is exceededConfiguration
Two new environment variables (via Viper):
LOGIN_RATE_LIMIT1LOGIN_RATE_BURST5Scope
Per sneak's instruction, only the login endpoint is rate-limited. Session creation and registration use hashcash proof-of-work instead.
Tests
ratelimitpackage (constructor, burst, burst exceeded, key isolation, key tracking, stop)api_test.go:TestLoginRateLimitExceeded: exhausts burst with rapid requests, verifies 429 response andRetry-AfterheaderTestLoginRateLimitAllowsNormalUse: verifies normal login still worksREADME
LOGIN_RATE_LIMITandLOGIN_RATE_BURSTto the Configuration tableReview: PR #78 — Login Rate Limiting
Requirements Checklist (#35)
POST /api/v1/loginonly (per sneak's instruction)golang.org/x/time/rateRetry-Afterheader on limit exceededPolicy Compliance
golang.org/x/time v0.6.0hash presentcmd/internal/internal/ratelimit/testing.T, no assertion librariesif val := ...; val != ""is value-checking, not error handling)nolintdirectives justifiedmndforSplitN(,2),exhaustructfor existing patternSecurity Analysis: IP Extraction
clientIP()checksX-Forwarded-For→X-Real-IP→RemoteAddr. Without a trusted reverse proxy, these headers are client-controlled — an attacker could rotateX-Forwarded-Forvalues 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
RemoteAddrwould 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 .— PASSEDratelimitunit tests passTestLoginRateLimitExceeded,TestLoginRateLimitAllowsNormalUse)Code Quality Notes
ratelimit.Limiterininternal/ratelimit/, login-specific wiring inhandlers.goStop()called instopCleanup()via fx OnStop hookloginRate <= 0falls back toDefaultRateentrytype is unexported (correctly private)selecton both ticker and stop channel — clean shutdownVerdict: PASS ✅
Clean implementation, correct scoping per sneak's instructions, comprehensive tests, full policy compliance, and the build is green.
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.
ba943d95edtod0e925bf70Rework Complete
What was done
Rebased onto
main— resolved merge conflict ininternal/handlers/handlers.gowhere PR #80 (runtime stats) and this PR both modified imports, struct fields, and the constructor. Bothstatsandratelimitare now present.Added reverse proxy security note to the "Login Rate Limiting" section of README.md, documenting:
clientIP()trustsX-Forwarded-ForandX-Real-IPheadersBuild verification
✅
docker build .passes — all tests green (including ratelimit unit tests and login rate limit integration tests), lint/format clean, binary builds.Post-Rework Review: PR #78 — Login Rate Limiting
Rework Verification
main(resolve conflict with PR #80)ratelimitandstatsimports, struct fields, and constructor presentRebase Integrity
The merge conflict in
internal/handlers/handlers.gowas resolved correctly:ratelimitandstatsimports present in the import blockHandlersstruct has bothloginLimiter *ratelimit.Limiterandstats *stats.TrackerfieldsParamsstruct includesStats *stats.Tracker(from PR #80)loginLimiter: ratelimit.New(...)andstats: params.StatsstopCleanup()callsloginLimiter.Stop()— lifecycle management intactSecurity Documentation Verification
The README security warning accurately matches the
clientIP()implementation ininternal/handlers/auth.go:X-Forwarded-Forfirstrequest.Header.Get("X-Forwarded-For")checked firstX-Real-IPRemoteAddrnet.SplitHostPort(request.RemoteAddr)as final fallbackX-Forwarded-ForvaluesThe documentation is clear, technically accurate, and appropriately scoped. The blockquote callout format with ⚠️ makes the warning visually prominent.
Policy Compliance
golang.org/x/time v0.6.0hash in go.sum)testing.T, no assertion librariesnolintdirectives justifiedmndforSplitN(,2),exhaustructfor existing patterncmd/internal/Build Result
✅
docker build .— PASSEDTestLoginRateLimitExceeded— confirms 429 withRetry-AfterheaderTestLoginRateLimitAllowsNormalUse— confirms normal login worksVerdict: 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 fix conflict pls
d0e925bf70to1446d6873bRebased
feature/login-rate-limitontomain, resolving merge conflicts ininternal/handlers/handlers.go.Conflict resolution: The
Handlersstruct andNew()constructor now include bothchannelHashcash(from #79) andloginLimiter(from this PR).README.mdandapi_test.goauto-merged cleanly.Build status:
docker build .fails due toTestLoginValidtiming out at 30s — but this is a pre-existing issue onmain(confirmed:mainHEADbf4d63balso fails the same way). This PR's changes did not introduce the failure.Review: PR #78 — Login Rate Limiting (post-rebase attempt)
Policy Divergences
1. Branch not rebased onto current
main— merge conflicts presentThe PR branch (
feature/login-rate-limit) is based onbf4d63b, butmainis now atdb3d23c(which includes PR #82 — username/hostname support with bcrypt MinCost for tests). Attempting to merge produces conflicts in:README.mdinternal/config/config.gointernal/handlers/api_test.goGitea confirms:
Mergeable: false. The PR cannot be merged as-is.sneak's last comment (2026-03-18) was "@clawbot fix conflict pls" — this remains unaddressed against current
main. The reworker's rebase resolved against stalemain(bf4d63b) instead of currentmain(db3d23c).Requirements Checklist (issue #35)
POST /api/v1/loginonly (per sneak's instruction)golang.org/x/time/rateRetry-Afterheader on limit exceededmainBuild Result
docker build .on the PR branch alone: ✅ PASSES (all tests green, lint clean, 22.5s handlers).However, the branch cannot be merged into current
maindue to conflicts. A successful isolated build is not sufficient — the branch must be mergeable.Code Quality Assessment (for reference — not sufficient to override FAIL)
The implementation itself is solid:
ratelimit.Limiterininternal/ratelimit/, login-specific wiring inhandlers.goStop()called instopCleanup()loginRate <= 0falls back toDefaultRategolang.org/x/time v0.6.0pinned ingo.sum✅testing.T, no assertion libs, parallel ✅.golangci.yml,Makefile, or CI config ✅nolintdirectives justified (mndforSplitN(,2),exhaustructfor existing pattern) ✅Verdict: FAIL ❌
The branch has merge conflicts with current
main(db3d23c) and cannot be merged. sneak's instruction to fix conflicts is not addressed against the currentmainHEAD. Needs a rebase ontomainthat incorporates PR #82 changes (username/hostname support, bcrypt MinCost for tests, conditional-vin Makefile).1446d6873bto0bcc711a92Rebased
feature/login-rate-limitonto current main (db3d23c).Conflicts resolved:
README.md— kept both OPER config vars (from PR #82) and rate-limit config varsinternal/config/config.go— merged OPER and rate-limit fields in struct, defaults, and config initialization (3 conflict regions)internal/handlers/api_test.go— kept all username/hostname test helpers and tests from main alongside rate-limit testsAdditional fixes:
clientIP()function fromauth.go(identical copy already exists inapi.gofrom PR #82)executeLogin()helper fromhandleLogin()to satisfyfunlenlint rule (was 86 lines, limit 80)docker build .passes — lint clean (0 issues), all tests green.Review: PR #78 — Login Rate Limiting (post-rebase onto
db3d23c)Rebase Integrity
db3d23c) is ancestor of PR branchgit merge-base --is-ancestorconfirmsfunc Test*from main present, plus 2 newOperName,OperPassword,LoginRateLimit,LoginRateBurstall presentclientIP()exists only inapi.go(no duplicate inauth.go)TestWhoisShowsHostInfo,TestWhoShowsHostInfo,TestSessionUsernameDefault,TestNamesShowsHostmask, OPER tests)Requirements Checklist (issue #35)
POST /api/v1/loginonly (per sneak's instruction)handleLoginreferencesloginLimiterapi.goorsession.gogolang.org/x/time/rate)LOGIN_RATE_LIMIT/LOGIN_RATE_BURSTenv varsRetry-After: 1header when limit exceededt.Parallel()TestLoginRateLimitExceeded,TestLoginRateLimitAllowsNormalUse)Policy Compliance
.golangci.ymlunmodifiedMakefileunmodifiedgolang.org/x/time v0.6.0pinned by hash ingo.sumtesting.T, no assertion librariesinternal/(notcmd/)internal/ratelimit/nolintdirectives: only pre-existing (exhaustruct,mnd,contextcheck)handleLogin55 lines,executeLogin44 lines — both under funlen 80Code Quality Notes
executeLoginextraction is clean — all original logic preserved,payload.Nick/payload.Passwordcorrectly replaced withnick/passwordparamsLimiter.Allow()holds mutex only for map lookup, callsrate.Limiter.Allow()outside lock — correct sincerate.Limiteris goroutine-safeStop()properly called instopCleanup()with nil guardloginRate <= 0falls back toDefaultRateBuild Result
✅
docker build .— PASSED (lint clean, all tests green, binary builds)Verdict: PASS ✅
Rebase onto
db3d23cis clean — no lost code, no conflict markers, all main branch features (username/hostname/OPER from PR #82) preserved alongside the rate limiting implementation. Build is green. Implementation is correct and scoped per sneak's instructions.0bcc711a92to1eee5f5508Rebased onto current
mainafter PR #84 merge (auth refactor: cookies replace Bearer tokens,/registerremoved, PASS command added).Conflicts resolved:
internal/handlers/auth.go—executeLogin()response: removedtokenfield from JSON body (now set viasetAuthCookie), usednickparameter instead ofpayload.Nick(correct scope in extracted function)Auto-merged files updated for compatibility:
internal/handlers/api_test.go—TestLoginRateLimitAllowsNormalUse: replaced removed/api/v1/registercall withcreateSession()+ PASS command (matching current test patterns)Verified:
docker build .passes — lint, fmt-check, all tests green including rate limit tests.Review: PR #78 — per-IP rate limiting on login endpoint
Verdict: PASS ✅
Rebase Integrity
Clean single commit (
1eee5f5) on top of currentmain. No conflict markers found. No stale references toBearertokens or/registerendpoint. All PR #84 auth refactor code (cookie-based auth,setAuthCookie,doRequestAuthwith cookies) is intact.Code Review
internal/ratelimit/ratelimit.go— Clean token-bucket implementation wrappinggolang.org/x/time/rate. Per-key map with mutex, background sweep goroutine with configurable TTL, properStop()for cleanup. TheAllow()method correctly releases the mutex before callingent.limiter.Allow()(safe becauserate.Limiteris goroutine-safe). No issues.internal/handlers/auth.go— Rate limit check is the first thing inhandleLogin, before JSON parsing or bcrypt. Returns 429 withRetry-After: 1header. TheexecuteLoginextraction is clean —handleLoginhandles parsing + validation + rate limiting, then delegates toexecuteLogin(nick, password). All references usenickparameter, notpayload.Nick. UsessetAuthCookie(cookie auth, not Bearer). Correct.internal/handlers/handlers.go—loginLimiterfield properly initialized inNew()with config values (with fallback to defaults if ≤0).Stop()called instopCleanup(). Clean integration.internal/config/config.go—LOGIN_RATE_LIMIT(float64) andLOGIN_RATE_BURST(int) with sensible defaults (1 req/s, burst 5).Scope — Rate limiting is applied ONLY to
handleLogin. Session creation endpoint is not rate-limited (protected by hashcash instead). Matches issue #35 scope.Tests
internal/ratelimit/ratelimit_test.go— 6 unit tests covering creation, burst allowance, burst exhaustion, key isolation, key counting, and stop safety. All uset.Parallel(). Tests with rate=0 for deterministic burst-only behavior — good approach.internal/handlers/api_test.go— Two integration tests:TestLoginRateLimitExceeded: Exhausts burst of 5 with bad logins, verifies 6th gets 429 +Retry-Afterheader.TestLoginRateLimitAllowsNormalUse: Creates session viacreateSession()+ PASS command (cookie-based, no/register), then verifies single login succeeds with 200.Build
docker build .passes — lint, fmt-check, tests, compilation all green.Documentation
README properly documents the env vars, the token-bucket algorithm, the 429 response, and includes a thorough security warning about X-Forwarded-For spoofing and reverse proxy requirements. The rationale for rate limiting login (bcrypt cost) vs. not rate limiting session creation (hashcash) is clearly explained.