Blocks 1.0: no CSRF protection on the cookie-authenticated POST / and POST /generate forms #93

New Issue

clawbot · 2026-08-09T07:02:57+02:00

clawbot commented

2026-08-09 07:02:57 +02:00

From the audit against the canonical REPO_POLICIES.md. Independently verified against main at 61f42e6 — a case-insensitive grep for csrf across internal/ and cmd/ returns zero matches, including in the templates.

The policy requires:

> CSRF tokens on all state-mutating HTML forms. API endpoints authenticated via Authorization header (Bearer token, API key) are exempt because the browser does not attach these automatically.

pixa has two state-mutating HTML form posts, and neither qualifies for the exemption:

POST / (internal/server/routes.go:49 → handleLoginPost, internal/handlers/auth.go:37-66) calls r.ParseForm() and on success s.sessMgr.CreateSession(w) — it creates session state.
POST /generate (routes.go:51 → HandleGenerateURL, auth.go:77) gates on s.sessMgr.IsAuthenticated(r) — i.e. it is cookie-authenticated, not Authorization-header authenticated. The exemption explicitly does not apply.

The concrete exposure on POST / is login CSRF: an attacker forces a victim's browser into a session under the attacker's signing key, so anything the victim then generates is attributable to, and visible in, a session the attacker controls. On POST /generate, a cross-origin post can mint signed image URLs using the victim's authenticated session.

Note that SameSite=Strict on the session cookie (added in #47) blunts the second case in modern browsers, but it is a defense-in-depth mitigation, not the control the policy requires — and it does nothing for login CSRF, where no session cookie exists yet.

Definition of done

A CSRF token is issued alongside the session and required on both POST / and POST /generate, rendered as a hidden field in internal/templates/login.html and generator.html.
Login CSRF is covered specifically — the token on POST / must be bound to a pre-session value (e.g. a cookie set when the form is rendered), not to a session that does not yet exist.
Token comparison is constant-time; token generation uses crypto/rand.
Prefer a well-maintained library over hand-rolling (gorilla/csrf or the chi ecosystem's equivalent). Check ~/.claude/GO_PACKAGE_DEFAULTS.md first and record the decision there.
Failing tests first: a POST without a token is rejected; with a stale or foreign token is rejected; with a valid token succeeds. This lands in internal/handlers, which currently has no auth-flow tests at all — coordinate with that coverage issue.
make check green.

Priority

Blocks 1.0.0. The policy states this requirement unconditionally, and both affected routes are cookie-authenticated.

From the audit against the canonical `REPO_POLICIES.md`. Independently verified against `main` at `61f42e6` — a case-insensitive grep for `csrf` across `internal/` and `cmd/` returns **zero matches**, including in the templates. The policy requires: > CSRF tokens on all state-mutating HTML forms. API endpoints authenticated via `Authorization` header (Bearer token, API key) are exempt because the browser does not attach these automatically. pixa has two state-mutating HTML form posts, and neither qualifies for the exemption: - **`POST /`** (`internal/server/routes.go:49` → `handleLoginPost`, `internal/handlers/auth.go:37-66`) calls `r.ParseForm()` and on success `s.sessMgr.CreateSession(w)` — it creates session state. - **`POST /generate`** (`routes.go:51` → `HandleGenerateURL`, `auth.go:77`) gates on `s.sessMgr.IsAuthenticated(r)` — i.e. it is **cookie**-authenticated, not `Authorization`-header authenticated. The exemption explicitly does not apply. The concrete exposure on `POST /` is login CSRF: an attacker forces a victim's browser into a session under the attacker's signing key, so anything the victim then generates is attributable to, and visible in, a session the attacker controls. On `POST /generate`, a cross-origin post can mint signed image URLs using the victim's authenticated session. Note that `SameSite=Strict` on the session cookie (added in #47) blunts the second case in modern browsers, but it is a defense-in-depth mitigation, not the control the policy requires — and it does nothing for login CSRF, where no session cookie exists yet. ## Definition of done 1. A CSRF token is issued alongside the session and required on both `POST /` and `POST /generate`, rendered as a hidden field in `internal/templates/login.html` and `generator.html`. 2. Login CSRF is covered specifically — the token on `POST /` must be bound to a pre-session value (e.g. a cookie set when the form is rendered), not to a session that does not yet exist. 3. Token comparison is constant-time; token generation uses `crypto/rand`. 4. Prefer a well-maintained library over hand-rolling (`gorilla/csrf` or the chi ecosystem's equivalent). Check `~/.claude/GO_PACKAGE_DEFAULTS.md` first and record the decision there. 5. Failing tests first: a POST without a token is rejected; with a stale or foreign token is rejected; with a valid token succeeds. This lands in `internal/handlers`, which currently has no auth-flow tests at all — coordinate with that coverage issue. 6. `make check` green. ## Priority **Blocks 1.0.0.** The policy states this requirement unconditionally, and both affected routes are cookie-authenticated.

clawbot added this to the 1.0.0 milestone 2026-08-09 07:02:57 +02:00

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.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/pixa#93