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

internal/middleware: CORS, logging, and the metrics auth gate are untested #79

New Issue
Closed
opened 2026-08-09 03:48:37 +02:00 by clawbot · 2 comments
clawbot commented 2026-08-09 03:48:37 +02:00
Collaborator
Copy Link
Copy Source

Verified against main at 61f42e6.

internal/middleware has two tests, both on security headers: TestSecurityHeaders and TestSecurityHeaders_PreservesExistingHeaders.

Untested:

  • MetricsAuth (middleware.go:135-144) — the basic-auth gate on /metrics. This is an access control check with no test. Metrics can leak upstream hostnames, traffic volumes, and cache behavior; if this gate regresses, nothing catches it.
  • CORS (middleware.go:114) — currently hardcoded to []string{"*"}. Becomes configurable via the access_control_allow_origin work; that change should land on top of a test that pins current behavior first.
  • Logging (middleware.go:82-109) — including that it does not log secrets. Worth an explicit assertion that the signing key never appears in a log line, given it can arrive as a form value on POST /.
  • Metrics (middleware.go:124-132).

Definition of done

  1. MetricsAuth tests: no credentials returns 401 with a WWW-Authenticate challenge; wrong credentials returns 401; correct credentials passes through. Also assert the comparison is constant-time if it is not already (and make it so if not — a timing-attackable metrics password is a small but free win).
  2. CORS tests pinning the current * behavior, written so they extend naturally when the config key lands.
  3. A logging test asserting that a POST / body containing the signing key does not put that value into the log output.
  4. A test that the metrics recorder middleware is installed when expected — see the separate issue about metrics only being collected when credentials are configured.
  5. New tests only. make check green.
Verified against `main` at `61f42e6`. `internal/middleware` has two tests, both on security headers: `TestSecurityHeaders` and `TestSecurityHeaders_PreservesExistingHeaders`. Untested: - **`MetricsAuth` (`middleware.go:135-144`)** — the basic-auth gate on `/metrics`. This is an access control check with no test. Metrics can leak upstream hostnames, traffic volumes, and cache behavior; if this gate regresses, nothing catches it. - **`CORS` (`middleware.go:114`)** — currently hardcoded to `[]string{"*"}`. Becomes configurable via the `access_control_allow_origin` work; that change should land on top of a test that pins current behavior first. - **`Logging` (`middleware.go:82-109`)** — including that it does not log secrets. Worth an explicit assertion that the signing key never appears in a log line, given it can arrive as a form value on `POST /`. - **`Metrics` (`middleware.go:124-132`)**. ## Definition of done 1. `MetricsAuth` tests: no credentials returns 401 with a `WWW-Authenticate` challenge; wrong credentials returns 401; correct credentials passes through. Also assert the comparison is constant-time if it is not already (and make it so if not — a timing-attackable metrics password is a small but free win). 2. CORS tests pinning the current `*` behavior, written so they extend naturally when the config key lands. 3. A logging test asserting that a `POST /` body containing the signing key does not put that value into the log output. 4. A test that the metrics recorder middleware is installed when expected — see the separate issue about metrics only being collected when credentials are configured. 5. New tests only. `make check` green.
clawbot added this to the 1.0.0 milestone 2026-08-09 03:48:37 +02:00
clawbot commented 2026-10-04 09:25:51 +02:00
Author
Collaborator
Copy Link
Copy Source

Plan, checked against next at 1616e91. Since this issue was filed, CORS became configurable with a test (TestCORSAnswersWithConfiguredOrigin) and the client address and its logging got tests. Still untested, and what this unit adds (new tests in internal/middleware, or in internal/server where the behaviour lives in the router setup):

  1. MetricsAuth: no credentials answers 401 with a WWW-Authenticate challenge; a wrong username or password answers 401; the right ones pass through. Read github.com/99designs/basicauth-go (as pinned in go.mod) and say in the PR whether it compares in constant time. If it does not, the fix is a separate small commit in this PR that compares with crypto/subtle, as the issue's definition of done allows.
  2. CORS: a request from another origin than the configured one gets no Access-Control-Allow-Origin, and * answers any origin (the existing test covers the configured origin only).
  3. Logging: a POST / whose form carries the signing key leaves no trace of the key in the log output.
  4. Metrics: with a metrics username set, /metrics reports a request the router served; with none set, nothing is recorded. This pins today's behaviour, which #85 may change after the owner rules on #168.

New tests only, apart from the constant-time fix if needed. t.Parallel() where the test allows it.

Model: opus-5-5

Plan, checked against `next` at `1616e91`. Since this issue was filed, CORS became configurable with a test (`TestCORSAnswersWithConfiguredOrigin`) and the client address and its logging got tests. Still untested, and what this unit adds (new tests in `internal/middleware`, or in `internal/server` where the behaviour lives in the router setup): 1. `MetricsAuth`: no credentials answers 401 with a `WWW-Authenticate` challenge; a wrong username or password answers 401; the right ones pass through. Read `github.com/99designs/basicauth-go` (as pinned in `go.mod`) and say in the PR whether it compares in constant time. If it does not, the fix is a separate small commit in this PR that compares with `crypto/subtle`, as the issue's definition of done allows. 2. CORS: a request from another origin than the configured one gets no `Access-Control-Allow-Origin`, and `*` answers any origin (the existing test covers the configured origin only). 3. `Logging`: a `POST /` whose form carries the signing key leaves no trace of the key in the log output. 4. Metrics: with a metrics username set, `/metrics` reports a request the router served; with none set, nothing is recorded. This pins today's behaviour, which https://git.eeqj.de/sneak/pixa/issues/85 may change after the owner rules on https://git.eeqj.de/sneak/pixa/issues/168. New tests only, apart from the constant-time fix if needed. `t.Parallel()` where the test allows it. Model: opus-5-5
clawbot commented 2026-10-04 10:28:24 +02:00
Author
Collaborator
Copy Link
Copy Source

Tests are in #178. No code changed: the pinned basic auth library already compares the password in constant time. Recording with a metrics username set is tested on the metrics middleware alone, not through the router; the PR says why.

Model: opus-5-5

Tests are in https://git.eeqj.de/sneak/pixa/pulls/178. No code changed: the pinned basic auth library already compares the password in constant time. Recording with a metrics username set is tested on the metrics middleware alone, not through the router; the PR says why. Model: opus-5-5
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
critical
Extremely important: data loss, a security hole, production down, or basic use impossible
 merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No labels
Milestone
No items
No Milestone 1.0.0
Projects
Clear projects
No projects
Assignees
Clear assignees
clawbot sneak (Jeffrey Paul)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/pixa#79