P1 security: no rate limiting or lockout on the signing-key login form #66

New Issue

clawbot · 2026-08-09T03:44:20+02:00

clawbot commented

2026-08-09 03:44:20 +02:00

Verified against main at 61f42e6. Covers the "per-IP rate limiting" P2 item in TODO.md, scoped to the place it matters most first.

handleLoginPost (internal/handlers/auth.go:37-67) compares the submitted key against config.SigningKey in constant time, logs a warning on mismatch, and re-renders the form. There is no rate limit, no lockout, no backoff, and no per-IP accounting anywhere in the codebase.

The credential being guessed is not a scoped password — it is the signing key, the master secret. It also derives the encrypted-URL key and the session key (internal/encurl, internal/seal, internal/session). Compromise means the attacker can mint arbitrary signed and encrypted URLs, i.e. use pixa as an open proxy against any host, permanently.

Config validation enforces a 32-character minimum (internal/config/config.go), so brute force is not the realistic threat against a well-chosen key. The realistic threats are a weak-but-long operator-chosen key, and the fact that an unthrottled endpoint gives an attacker unlimited free attempts with zero cost or visibility beyond log lines nobody is alerting on.

Definition of done

Per-IP rate limiting on POST / with a sane default (a few attempts per minute), returning 429 with Retry-After once tripped.
The limiter is shared infrastructure, not login-specific, so the remaining TODO.md rate-limiting items (per-IP on image routes, per-origin) can reuse it. Prefer a well-maintained library over hand-rolling; check ~/.claude/GO_PACKAGE_DEFAULTS.md first and record the decision there.
Client IP is derived correctly behind a reverse proxy — pixa is explicitly meant to sit behind nginx/caddy, so a naive RemoteAddr limiter would key every request to the proxy's IP and lock out all users at once. Trusted-proxy handling must be configurable, and must not trust X-Forwarded-For blindly from untrusted sources.
Limiter state is bounded — no unbounded per-IP map (see also the unbounded hostSems map issue).
Failing tests first: N+1 failed logins from one IP returns 429; a different IP is unaffected; a successful login is not blocked by another IP's failures.
make check green.

Follow-on (not in this issue)

Once the limiter exists, per-IP limiting on the image routes and per-origin limiting are separate commit-sized units.

Verified against `main` at `61f42e6`. Covers the "per-IP rate limiting" P2 item in `TODO.md`, scoped to the place it matters most first. `handleLoginPost` (`internal/handlers/auth.go:37-67`) compares the submitted key against `config.SigningKey` in constant time, logs a warning on mismatch, and re-renders the form. There is no rate limit, no lockout, no backoff, and no per-IP accounting anywhere in the codebase. The credential being guessed is not a scoped password — it is the **signing key**, the master secret. It also derives the encrypted-URL key and the session key (`internal/encurl`, `internal/seal`, `internal/session`). Compromise means the attacker can mint arbitrary signed and encrypted URLs, i.e. use pixa as an open proxy against any host, permanently. Config validation enforces a 32-character minimum (`internal/config/config.go`), so brute force is not the realistic threat against a well-chosen key. The realistic threats are a weak-but-long operator-chosen key, and the fact that an unthrottled endpoint gives an attacker unlimited free attempts with zero cost or visibility beyond log lines nobody is alerting on. ## Definition of done 1. Per-IP rate limiting on `POST /` with a sane default (a few attempts per minute), returning 429 with `Retry-After` once tripped. 2. The limiter is shared infrastructure, not login-specific, so the remaining `TODO.md` rate-limiting items (per-IP on image routes, per-origin) can reuse it. Prefer a well-maintained library over hand-rolling; check `~/.claude/GO_PACKAGE_DEFAULTS.md` first and record the decision there. 3. Client IP is derived correctly behind a reverse proxy — pixa is explicitly meant to sit behind nginx/caddy, so a naive `RemoteAddr` limiter would key every request to the proxy's IP and lock out all users at once. Trusted-proxy handling must be configurable, and must not trust `X-Forwarded-For` blindly from untrusted sources. 4. Limiter state is bounded — no unbounded per-IP map (see also the unbounded `hostSems` map issue). 5. Failing tests first: N+1 failed logins from one IP returns 429; a different IP is unaffected; a successful login is not blocked by another IP's failures. 6. `make check` green. ## Follow-on (not in this issue) Once the limiter exists, per-IP limiting on the image routes and per-origin limiting are separate commit-sized units.

clawbot added this to the 1.0.0 milestone 2026-08-09 03:44:20 +02:00

clawbot referenced this issue

2026-08-09 07:03:11 +02:00

Blocks 1.0: no trusted-proxy handling — every log line and login record shows the proxy's IP #94

clawbot referenced this issue

2026-08-09 16:10:39 +02:00

Tracking: 1.0.0 sequencing and critical path #103

Sign in to join this conversation.