sneak/neoirc
1
0
Fork 0
Code Issues 5 Pull Requests 3 Actions Packages Projects Releases Wiki Activity

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

Merged
sneak merged 1 commits from feature/login-rate-limit into main 2026-03-22 00:39:38 +01:00
Conversation 11 Commits 1 Files Changed 9 +431 -5
clawbot commented 2026-03-17 10:27:22 +01:00
Collaborator
Copy Link

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, 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
All checks were successful
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
Author
Collaborator
Copy Link

Review: PR #78 — Login Rate Limiting

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
Owner
Copy Link

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 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
Author
Collaborator
Copy Link

Rework Complete

What was done

  1. 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.

  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.

## 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
Author
Collaborator
Copy Link

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

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
Owner
Copy Link

@clawbot fix conflict pls

@clawbot fix conflict pls
clawbot added needs-rework and removed merge-ready labels 2026-03-20 06:51:20 +01:00
sneak was unassigned by clawbot 2026-03-20 06:51:21 +01:00
clawbot self-assigned this 2026-03-20 06:51:21 +01:00
clawbot force-pushed feature/login-rate-limit from d0e925bf70 to 1446d6873b 2026-03-20 06:56:45 +01:00 Compare
clawbot commented 2026-03-20 06:56:53 +01:00
Author
Collaborator
Copy Link

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.
clawbot added needs-review and removed needs-rework labels 2026-03-20 06:59:22 +01:00
clawbot reviewed 2026-03-20 07:06:12 +01:00
clawbot left a comment
Author
Collaborator
Copy Link

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 — 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)

# 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 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).
clawbot added needs-rework and removed needs-review labels 2026-03-20 07:07:03 +01:00
clawbot force-pushed feature/login-rate-limit from 1446d6873b to 0bcc711a92 2026-03-20 07:15:03 +01:00 Compare
clawbot commented 2026-03-20 07:15:13 +01:00
Author
Collaborator
Copy Link

Rebased feature/login-rate-limit onto current main (db3d23c).

Conflicts resolved:

  • README.md — kept both OPER config vars (from PR #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.

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.
clawbot added needs-review and removed needs-rework labels 2026-03-20 07:15:51 +01:00
clawbot reviewed 2026-03-20 07:19:03 +01:00
clawbot left a comment
Author
Collaborator
Copy Link

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)

# Requirement Status
1 Rate limiting on POST /api/v1/login only (per sneak's instruction) ✅ 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) 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.
clawbot added merge-ready and removed needs-review labels 2026-03-20 07:19:27 +01:00
clawbot removed their assignment 2026-03-20 07:19:28 +01:00
sneak was assigned by clawbot 2026-03-20 07:19:28 +01:00
sneak removed their assignment 2026-03-20 23:54:49 +01:00
clawbot was assigned by sneak 2026-03-20 23:54:49 +01:00
clawbot added needs-rework and removed merge-ready labels 2026-03-20 23:56:02 +01:00
clawbot force-pushed feature/login-rate-limit from 0bcc711a92 to 1eee5f5508 2026-03-21 00:01:03 +01:00 Compare
clawbot commented 2026-03-21 00:01:19 +01:00
Author
Collaborator
Copy Link

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.
clawbot added needs-review and removed needs-rework labels 2026-03-21 00:02:02 +01:00
clawbot reviewed 2026-03-21 00:09:07 +01:00
clawbot left a comment
Author
Collaborator
Copy Link

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.

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.

## 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.
clawbot added merge-ready and removed needs-review labels 2026-03-21 00:09:52 +01:00
clawbot removed their assignment 2026-03-21 00:09:52 +01:00
sneak was assigned by clawbot 2026-03-21 00:09:52 +01:00
sneak merged commit 08f57bc105 into main 2026-03-22 00:39:38 +01:00
sneak deleted branch feature/login-rate-limit 2026-03-22 00:39:38 +01:00
Sign in to join this conversation.
Reviewers
No Reviewers
clawbot
Labels
Clear labels
merge-ready
All checks pass, reviewed, rebased, ready for merge
 needs-checks
make check does not pass cleanly
 needs-rebase
PR has merge conflicts or is behind main
 needs-review
Code review not yet done
 needs-review
needs-rework
Code review found issues to fix
No Label merge-ready
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees sneak
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/neoirc#78
Delete Branch "feature/login-rate-limit"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?