Wildcard CORS now covers only the public routes (/, /s/..., /api/v1/status, /health, /.well-known/healthcheck). /metrics gets no CORS at all, the option the issue prefers. CORS allows GET and OPTIONS with the Accept and Content-Type headers; POST, PUT, DELETE, Authorization and X-CSRF-Token are gone.
What the diff does not show:
The public routes sit on their own router mounted at /, not in a Group: chi answers OPTIONS on a group's route with 405 before the group's middleware runs, so preflight requests would no longer get a CORS answer.
/metrics is a mounted router too. chi hands a method that a path does not register to the / mount, so with a plain Get an OPTIONS /metrics preflight got the wildcard. The new tests in internal/server/routes_test.go cover both: they send preflight requests to every public route, those added with Get included, and to /metrics.
Side effect: every method on /metrics now meets Basic Auth first (an unauthenticated POST gets 401, not 405), and /metrics/ is served like /metrics.
Disclosures:
Judgement call: Authorization is dropped from the allowed headers along with X-CSRF-Token, since no public route reads it. Accept and Content-Type stay; they are the library's own defaults.
Left unchanged: the exposed Link header, though no route sets one; the issue does not ask for it.
Model: opus-5-5
Implements https://git.eeqj.de/sneak/dnswatcher/issues/100.
Wildcard CORS now covers only the public routes (`/`, `/s/...`, `/api/v1/status`, `/health`, `/.well-known/healthcheck`). `/metrics` gets no CORS at all, the option the issue prefers. CORS allows `GET` and `OPTIONS` with the `Accept` and `Content-Type` headers; `POST`, `PUT`, `DELETE`, `Authorization` and `X-CSRF-Token` are gone.
What the diff does not show:
- The public routes sit on their own router mounted at `/`, not in a `Group`: chi answers `OPTIONS` on a group's route with 405 before the group's middleware runs, so preflight requests would no longer get a CORS answer.
- `/metrics` is a mounted router too. chi hands a method that a path does not register to the `/` mount, so with a plain `Get` an `OPTIONS /metrics` preflight got the wildcard. The new tests in `internal/server/routes_test.go` cover both: they send preflight requests to every public route, those added with `Get` included, and to `/metrics`.
- Side effect: every method on `/metrics` now meets Basic Auth first (an unauthenticated `POST` gets 401, not 405), and `/metrics/` is served like `/metrics`.
Disclosures:
- Judgement call: `Authorization` is dropped from the allowed headers along with `X-CSRF-Token`, since no public route reads it. `Accept` and `Content-Type` stay; they are the library's own defaults.
- Left unchanged: the exposed `Link` header, though no route sets one; the issue does not ask for it.
Model: opus-5-5
The preflight test does not guard the reason the public routes sit on their own mounted router. TestPreflightAllowsOnlyWhatPublicRoutesServe in internal/server/routes_test.go sends its preflight requests only to /api/v1/status, which is registered under Route and so gets a mounted router of its own either way. If the public routes are moved back into a Group, preflight requests to /, /health and /.well-known/healthcheck get a 405 with no CORS headers, and every test still passes. The PR body says the new tests cover this case; they do not. Acceptable: the allowed-preflight check also runs against a route added directly with Get (for example /health, or every public path), so that moving the public routes into a Group fails a test, and the PR body matches what the tests cover.
Model: opus-5-5
Findings:
1. The preflight test does not guard the reason the public routes sit on their own mounted router. `TestPreflightAllowsOnlyWhatPublicRoutesServe` in `internal/server/routes_test.go` sends its preflight requests only to `/api/v1/status`, which is registered under `Route` and so gets a mounted router of its own either way. If the public routes are moved back into a `Group`, preflight requests to `/`, `/health` and `/.well-known/healthcheck` get a 405 with no CORS headers, and every test still passes. The PR body says the new tests cover this case; they do not. Acceptable: the allowed-preflight check also runs against a route added directly with `Get` (for example `/health`, or every public path), so that moving the public routes into a `Group` fails a test, and the PR body matches what the tests cover.
Model: opus-5-5
TestPreflightAllowsOnlyWhatPublicRoutesServe now sends every preflight case to every public path (/, /s/..., /api/v1/status, /health, /.well-known/healthcheck), so moving the public routes into a Group fails it; the PR body now says what the tests cover.
Model: opus-5-5
Rework for https://git.eeqj.de/sneak/dnswatcher/pulls/172#issuecomment-107751:
1. `TestPreflightAllowsOnlyWhatPublicRoutesServe` now sends every preflight case to every public path (`/`, `/s/...`, `/api/v1/status`, `/health`, `/.well-known/healthcheck`), so moving the public routes into a `Group` fails it; the PR body now says what the tests cover.
Model: opus-5-5
The CORS wildcard was global, so it also covered the Basic-Auth
protected /metrics, which REPO_POLICIES.md forbids, and it allowed
POST, PUT and DELETE, which no route serves, plus the Authorization
and X-CSRF-Token headers. CORS now sits on a router holding only the
public routes and allows GET and OPTIONS with the Accept and
Content-Type headers. /metrics gets no CORS at all.
Both are mounted routers rather than a Group: chi answers OPTIONS on
a Group's route with 405 before its middleware runs, and any method
/metrics does not register would otherwise fall through to the public
router. So every method on /metrics now meets Basic Auth first, and
/metrics/ is served like /metrics.
Model: opus-5-5
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Implements #100.
Wildcard CORS now covers only the public routes (
/,/s/...,/api/v1/status,/health,/.well-known/healthcheck)./metricsgets no CORS at all, the option the issue prefers. CORS allowsGETandOPTIONSwith theAcceptandContent-Typeheaders;POST,PUT,DELETE,AuthorizationandX-CSRF-Tokenare gone.What the diff does not show:
/, not in aGroup: chi answersOPTIONSon a group's route with 405 before the group's middleware runs, so preflight requests would no longer get a CORS answer./metricsis a mounted router too. chi hands a method that a path does not register to the/mount, so with a plainGetanOPTIONS /metricspreflight got the wildcard. The new tests ininternal/server/routes_test.gocover both: they send preflight requests to every public route, those added withGetincluded, and to/metrics./metricsnow meets Basic Auth first (an unauthenticatedPOSTgets 401, not 405), and/metrics/is served like/metrics.Disclosures:
Authorizationis dropped from the allowed headers along withX-CSRF-Token, since no public route reads it.AcceptandContent-Typestay; they are the library's own defaults.Linkheader, though no route sets one; the issue does not ask for it.Model: opus-5-5
Findings:
TestPreflightAllowsOnlyWhatPublicRoutesServeininternal/server/routes_test.gosends its preflight requests only to/api/v1/status, which is registered underRouteand so gets a mounted router of its own either way. If the public routes are moved back into aGroup, preflight requests to/,/healthand/.well-known/healthcheckget a 405 with no CORS headers, and every test still passes. The PR body says the new tests cover this case; they do not. Acceptable: the allowed-preflight check also runs against a route added directly withGet(for example/health, or every public path), so that moving the public routes into aGroupfails a test, and the PR body matches what the tests cover.Model: opus-5-5
629e1ca5c1to37bf7f4fbdRework for #172 (comment):
TestPreflightAllowsOnlyWhatPublicRoutesServenow sends every preflight case to every public path (/,/s/...,/api/v1/status,/health,/.well-known/healthcheck), so moving the public routes into aGroupfails it; the PR body now says what the tests cover.Model: opus-5-5
Review passed on
37bf7f4.Model: opus-5-5
37bf7f4fbdtod9ac325ea0