CORS wildcard applies to the authenticated /metrics route, and advertises methods that do not exist #100

New Issue

clawbot · 2026-08-09T03:36:27+02:00

clawbot commented

2026-08-09 03:36:27 +02:00

The CORS middleware is registered globally and is both too broad in scope and inaccurate about what the service supports.

Current state (audited against `origin/main`, commit `9347a28`)

internal/middleware/middleware.go:173-187:

return cors.Handler(cors.Options{
    AllowedOrigins:   []string{"*"},
    AllowedMethods:   []string{"GET", "POST", "PUT", "DELETE", "OPTIONS"},
    AllowedHeaders:   []string{"Accept", "Authorization", "Content-Type", "X-CSRF-Token"},
    ExposedHeaders:   []string{"Link"},
    AllowCredentials: false,
    MaxAge:           corsMaxAge,
})

Registered globally at internal/server/routes.go:25 (s.router.Use(s.mw.CORS())).

Two distinct problems:

1. Wildcard CORS is applied to the authenticated route. REPO_POLICIES.md says wildcard Access-Control-Allow-Origin "is acceptable only for public, unauthenticated read-only APIs", and that "authenticated endpoints must restrict Access-Control-Allow-Origin to an explicit allowlist of known origins." /metrics is Basic-Auth protected (internal/server/routes.go:54-63, MetricsAuth() at internal/middleware/middleware.go:189-205) yet inherits the global wildcard, and AllowedHeaders explicitly includes Authorization. Wildcard CORS is genuinely fine for /, /api/v1/status, and the healthchecks — those are exactly the "public, unauthenticated, read-only" case the policy carves out. It is not fine for /metrics.

2. The advertised method set is fiction. Every route in internal/server/routes.go is registered with Get(). There is not a single POST, PUT, or DELETE handler anywhere in the service, and no handler reads a request body at all (grep for .Body, NewDecoder, ReadAll, MaxBytesReader across internal/handlers, internal/server, internal/middleware returns nothing). Advertising POST, PUT, DELETE — and X-CSRF-Token, when there are no forms and no state-mutating endpoints — is over-broad configuration that misrepresents the service's surface.

Definition of done

AllowedMethods is reduced to the methods the service actually serves: GET and OPTIONS.
X-CSRF-Token is removed from AllowedHeaders (no forms, no state-mutating endpoints exist). Keep only headers a cross-origin reader genuinely needs.
Wildcard CORS no longer applies to /metrics. Either scope the CORS middleware to the public route group only and leave /metrics without CORS entirely (preferred — a Prometheus scraper is not a browser and needs no CORS), or give the /metrics group its own restrictive CORS configuration. Whichever is chosen, the public routes (/, /s/..., /api/v1/status, /health, /.well-known/healthcheck) keep their wildcard, which the policy permits.
Tests in internal/middleware or internal/server assert: a cross-origin GET against a public route still gets Access-Control-Allow-Origin: *; a request against /metrics does not get a wildcard Access-Control-Allow-Origin; and the advertised Access-Control-Allow-Methods no longer contains POST, PUT, or DELETE.
make check is green, and TODO.md is updated in the same commit as the work.

The finishing commit's title must end with (closes #N) referencing this issue.

Out of scope

Security response headers (#98), http.Server timeouts (#99), and rate limiting are tracked separately — do not fold them into this PR.

The CORS middleware is registered globally and is both too broad in scope and inaccurate about what the service supports. ## Current state (audited against `origin/main`, commit `9347a28`) `internal/middleware/middleware.go:173-187`: ```go return cors.Handler(cors.Options{ AllowedOrigins: []string{"*"}, AllowedMethods: []string{"GET", "POST", "PUT", "DELETE", "OPTIONS"}, AllowedHeaders: []string{"Accept", "Authorization", "Content-Type", "X-CSRF-Token"}, ExposedHeaders: []string{"Link"}, AllowCredentials: false, MaxAge: corsMaxAge, }) ``` Registered globally at `internal/server/routes.go:25` (`s.router.Use(s.mw.CORS())`). Two distinct problems: **1. Wildcard CORS is applied to the authenticated route.** `REPO_POLICIES.md` says wildcard `Access-Control-Allow-Origin` "is acceptable only for public, unauthenticated read-only APIs", and that "authenticated endpoints must restrict `Access-Control-Allow-Origin` to an explicit allowlist of known origins." `/metrics` is Basic-Auth protected (`internal/server/routes.go:54-63`, `MetricsAuth()` at `internal/middleware/middleware.go:189-205`) yet inherits the global wildcard, and `AllowedHeaders` explicitly includes `Authorization`. Wildcard CORS is genuinely fine for `/`, `/api/v1/status`, and the healthchecks — those are exactly the "public, unauthenticated, read-only" case the policy carves out. It is not fine for `/metrics`. **2. The advertised method set is fiction.** Every route in `internal/server/routes.go` is registered with `Get()`. There is not a single `POST`, `PUT`, or `DELETE` handler anywhere in the service, and no handler reads a request body at all (grep for `.Body`, `NewDecoder`, `ReadAll`, `MaxBytesReader` across `internal/handlers`, `internal/server`, `internal/middleware` returns nothing). Advertising `POST, PUT, DELETE` — and `X-CSRF-Token`, when there are no forms and no state-mutating endpoints — is over-broad configuration that misrepresents the service's surface. ## Definition of done 1. `AllowedMethods` is reduced to the methods the service actually serves: `GET` and `OPTIONS`. 2. `X-CSRF-Token` is removed from `AllowedHeaders` (no forms, no state-mutating endpoints exist). Keep only headers a cross-origin reader genuinely needs. 3. Wildcard CORS no longer applies to `/metrics`. Either scope the CORS middleware to the public route group only and leave `/metrics` without CORS entirely (preferred — a Prometheus scraper is not a browser and needs no CORS), or give the `/metrics` group its own restrictive CORS configuration. Whichever is chosen, the public routes (`/`, `/s/...`, `/api/v1/status`, `/health`, `/.well-known/healthcheck`) keep their wildcard, which the policy permits. 4. Tests in `internal/middleware` or `internal/server` assert: a cross-origin `GET` against a public route still gets `Access-Control-Allow-Origin: *`; a request against `/metrics` does **not** get a wildcard `Access-Control-Allow-Origin`; and the advertised `Access-Control-Allow-Methods` no longer contains `POST`, `PUT`, or `DELETE`. 5. `make check` is green, and `TODO.md` is updated in the same commit as the work. The finishing commit's title must end with ` (closes #N)` referencing this issue. ## Out of scope Security response headers (#98), `http.Server` timeouts (#99), and rate limiting are tracked separately — do not fold them into this PR.

clawbot added this to the 1.0 milestone 2026-08-09 03:36:27 +02:00

clawbot referenced this issue

2026-08-09 03:36:47 +02:00

No rate limiting on the Basic-Auth-protected /metrics endpoint #101

clawbot referenced this issue

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

README accuracy sweep: architecture omissions, undocumented state field, overclaimed MAINTENANCE_MODE and /metrics #108

clawbot referenced this issue

2026-08-09 03:41:03 +02:00

internal/globals, internal/healthcheck, and internal/logger have no tests at all #110

clawbot referenced this issue

2026-08-09 03:42:01 +02:00