sneak/dnswatcher
1
0
Fork 0
Code Issues 37 Pull Requests 8 Actions Packages Projects Releases Wiki Activity

feat: add security response headers middleware (closes #98) #112

Open
clawbot wants to merge 1 commits from fix/98-security-headers into next
pull from: fix/98-security-headers
merge into: sneak:next
Conversation 4 Commits 1 Files Changed 5 +471 -1
clawbot commented 2026-08-09 03:48:12 +02:00
Collaborator
Copy Link

Closes #98.

Adds SecurityHeaders() to internal/middleware/middleware.go and registers it in the global middleware stack in internal/server/routes.go, immediately after chimw.RequestID and before Logging/CORS, so it applies to every route: /, /s/..., /.well-known/healthcheck, /health, /api/v1/status, and the /metrics group.

Headers set on every response

Header Value
Strict-Transport-Security max-age=31536000; includeSubDomains
Content-Security-Policy see below
X-Frame-Options DENY
X-Content-Type-Options nosniff
Referrer-Policy no-referrer
Permissions-Policy accelerometer, autoplay, camera, display-capture, encrypted-media, fullscreen, geolocation, gyroscope, magnetometer, microphone, midi, payment, picture-in-picture, publickey-credentials-get, screen-wake-lock, usb, xr-spatial-tracking — all ()

CSP, exactly as emitted:

default-src 'self'; script-src 'none'; style-src 'self'; img-src 'self'; font-src 'none'; connect-src 'none'; object-src 'none'; base-uri 'none'; form-action 'none'; frame-ancestors 'none'

Why this CSP

Verified against internal/handlers/templates/dashboard.html and static/: the template has no <script> tags (the 30-second refresh is a <meta http-equiv="refresh">), no inline style=, no inline event handlers, and no <img>. Its only subresource is <link rel="stylesheet" href="/s/css/tailwind.min.css">, served same-origin from the embedded FS, which style-src 'self' permits. static/css/tailwind.min.css contains no url() and no @font-face, so font-src 'none' is safe. With no JavaScript, script-src 'none' and connect-src 'none' cost nothing. img-src 'self' is kept rather than 'none' so a future same-origin /favicon.ico is not blocked. Neither unsafe-inline nor unsafe-eval appears anywhere. frame-ancestors 'none' is the primary anti-framing control, with X-Frame-Options: DENY retained as the legacy fallback per policy.

Two deliberate choices worth review attention:

  • HSTS is unconditional, never gated on r.TLS != nil. The service speaks plain HTTP behind a TLS-terminating proxy, and REPO_POLICIES.md requires the application itself to emit HSTS so the browser enforces HTTPS end to end.
  • Referrer-Policy: no-referrer rather than the strict-origin-when-cross-origin baseline. The policy and the issue both allow "or stricter"; the dashboard has no cross-origin navigation needs and its URL can name internal hosts, so leaking nothing is the better default.

The headers are written before the request reaches the next handler, so they are present on error responses too, including panics recovered by chimw.Recoverer and 504s produced by chimw.Timeout.

Tests

New external-package tests in internal/middleware/middleware_test.go:

  • TestSecurityHeaders — table-driven over all six headers, asserting exact values (expected values written out literally in the test rather than imported from the implementation, so a change to the middleware must be made deliberately in both places).
  • TestSecurityHeadersCSPDirectives — asserts the CSP contains neither unsafe-inline nor unsafe-eval, and does contain default-src 'self', script-src 'none', style-src 'self', frame-ancestors 'none'.
  • TestSecurityHeadersOnErrorResponse — headers present on a 500.
  • TestDashboardRendersWithSecurityHeaders — the real handlers.HandleDashboard() wired through a chi router with the middleware: asserts HTTP 200, that the rendered body still references /s/css/tailwind.min.css, and that the emitted CSP is the expected one and permits that stylesheet.

No DNS is involved anywhere in this change or its tests.

Verification

  • make check (test + lint + fmt-check) green: 0 issues., full cold run 10.98s wall, well under the 20-second policy ceiling; internal/middleware tests run in 0.006s.
  • make fmt run; result included in the commit.
  • .golangci.yml untouched — sha256 still 021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb. The golangci-lint commit pin is untouched.
  • README.md gains a "Security Headers" subsection under the HTTP API area documenting the six headers, the CSP, and the two rationale notes above; the architecture listing now mentions security headers.
  • TODO.md updated in the same commit as the work.

Out of scope

http.Server timeouts, request body limits, rate limiting, and CORS scoping are tracked separately and are not touched here.

Closes #98. Adds `SecurityHeaders()` to `internal/middleware/middleware.go` and registers it in the global middleware stack in `internal/server/routes.go`, immediately after `chimw.RequestID` and before `Logging`/`CORS`, so it applies to every route: `/`, `/s/...`, `/.well-known/healthcheck`, `/health`, `/api/v1/status`, and the `/metrics` group. ## Headers set on every response | Header | Value | |--------|-------| | `Strict-Transport-Security` | `max-age=31536000; includeSubDomains` | | `Content-Security-Policy` | see below | | `X-Frame-Options` | `DENY` | | `X-Content-Type-Options` | `nosniff` | | `Referrer-Policy` | `no-referrer` | | `Permissions-Policy` | accelerometer, autoplay, camera, display-capture, encrypted-media, fullscreen, geolocation, gyroscope, magnetometer, microphone, midi, payment, picture-in-picture, publickey-credentials-get, screen-wake-lock, usb, xr-spatial-tracking — all `()` | CSP, exactly as emitted: ``` default-src 'self'; script-src 'none'; style-src 'self'; img-src 'self'; font-src 'none'; connect-src 'none'; object-src 'none'; base-uri 'none'; form-action 'none'; frame-ancestors 'none' ``` ## Why this CSP Verified against `internal/handlers/templates/dashboard.html` and `static/`: the template has no `<script>` tags (the 30-second refresh is a `<meta http-equiv="refresh">`), no inline `style=`, no inline event handlers, and no `<img>`. Its only subresource is `<link rel="stylesheet" href="/s/css/tailwind.min.css">`, served same-origin from the embedded FS, which `style-src 'self'` permits. `static/css/tailwind.min.css` contains no `url()` and no `@font-face`, so `font-src 'none'` is safe. With no JavaScript, `script-src 'none'` and `connect-src 'none'` cost nothing. `img-src 'self'` is kept rather than `'none'` so a future same-origin `/favicon.ico` is not blocked. Neither `unsafe-inline` nor `unsafe-eval` appears anywhere. `frame-ancestors 'none'` is the primary anti-framing control, with `X-Frame-Options: DENY` retained as the legacy fallback per policy. Two deliberate choices worth review attention: - **HSTS is unconditional**, never gated on `r.TLS != nil`. The service speaks plain HTTP behind a TLS-terminating proxy, and `REPO_POLICIES.md` requires the application itself to emit HSTS so the browser enforces HTTPS end to end. - **`Referrer-Policy: no-referrer`** rather than the `strict-origin-when-cross-origin` baseline. The policy and the issue both allow "or stricter"; the dashboard has no cross-origin navigation needs and its URL can name internal hosts, so leaking nothing is the better default. The headers are written before the request reaches the next handler, so they are present on error responses too, including panics recovered by `chimw.Recoverer` and 504s produced by `chimw.Timeout`. ## Tests New external-package tests in `internal/middleware/middleware_test.go`: - `TestSecurityHeaders` — table-driven over all six headers, asserting exact values (expected values written out literally in the test rather than imported from the implementation, so a change to the middleware must be made deliberately in both places). - `TestSecurityHeadersCSPDirectives` — asserts the CSP contains neither `unsafe-inline` nor `unsafe-eval`, and does contain `default-src 'self'`, `script-src 'none'`, `style-src 'self'`, `frame-ancestors 'none'`. - `TestSecurityHeadersOnErrorResponse` — headers present on a 500. - `TestDashboardRendersWithSecurityHeaders` — the real `handlers.HandleDashboard()` wired through a chi router with the middleware: asserts HTTP 200, that the rendered body still references `/s/css/tailwind.min.css`, and that the emitted CSP is the expected one and permits that stylesheet. No DNS is involved anywhere in this change or its tests. ## Verification - `make check` (test + lint + fmt-check) green: `0 issues.`, full cold run 10.98s wall, well under the 20-second policy ceiling; `internal/middleware` tests run in 0.006s. - `make fmt` run; result included in the commit. - `.golangci.yml` untouched — sha256 still `021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb`. The golangci-lint commit pin is untouched. - `README.md` gains a "Security Headers" subsection under the HTTP API area documenting the six headers, the CSP, and the two rationale notes above; the architecture listing now mentions security headers. - `TODO.md` updated in the same commit as the work. ## Out of scope `http.Server` timeouts, request body limits, rate limiting, and CORS scoping are tracked separately and are not touched here.
clawbot added the needs-review label 2026-08-09 03:48:12 +02:00
clawbot self-assigned this 2026-08-09 03:48:19 +02:00
clawbot commented 2026-08-09 03:48:37 +02:00
Author
Collaborator
Copy Link

Definition of done from #98, item by item:

  1. Six headers on every response — done. SecurityHeaders() on *Middleware in internal/middleware/middleware.go sets Strict-Transport-Security: max-age=31536000; includeSubDomains, the CSP below, X-Frame-Options: DENY, X-Content-Type-Options: nosniff, Referrer-Policy: no-referrer, and a Permissions-Policy denying seventeen features including camera, microphone, and geolocation. Values live as package constants; the tests assert them literally rather than importing the constants, so the implementation and the expectation cannot drift silently.
  2. Registered globally — done. internal/server/routes.go calls s.router.Use(s.mw.SecurityHeaders()) in the global stack, after chimw.RequestID and before Logging/CORS, so it covers /, /s/... static assets, both healthchecks, /api/v1/status, and the /metrics group (the group inherits global middleware; only MetricsAuth is group-scoped).
  3. HSTS unconditional — done. No r.TLS check anywhere in the middleware; the header is set on every request regardless of scheme.
  4. Tests — done. internal/middleware/middleware_test.go (external package middleware_test): a table-driven test over all six headers asserting exact values, a CSP directive test, a test that the headers survive a 500, and TestDashboardRendersWithSecurityHeaders, which serves the real handlers.HandleDashboard() through a chi router with the middleware and asserts HTTP 200.
  5. CSP does not break the dashboard — done and asserted: the dashboard test checks the rendered body still contains /s/css/tailwind.min.css and that the emitted CSP carries style-src 'self', which permits that same-origin stylesheet. The template loads no other subresource — no scripts, no inline styles, no images, no fonts.
  6. README — done. New "Security Headers" subsection under the HTTP API area with the header table, the full CSP, and the reasoning for unconditional HSTS and for no-referrer. The architecture listing now names security headers among the middleware.
  7. make check green, TODO.md in the same commit — done. Single commit e97a4e5, which contains the middleware, the route registration, the tests, the README, and the TODO.md entry together.

The exact CSP:

default-src 'self'; script-src 'none'; style-src 'self'; img-src 'self'; font-src 'none'; connect-src 'none'; object-src 'none'; base-uri 'none'; form-action 'none'; frame-ancestors 'none'

Two calls a reviewer may want to second-guess, both argued in the PR body: Referrer-Policy: no-referrer instead of the strict-origin-when-cross-origin baseline (the issue allows "or stricter"), and img-src 'self' rather than 'none' so a future same-origin favicon is not blocked.

Verification run: make check — 0 issues., all packages pass, 10.98s wall on a cold run (previous baseline ~7.8s; the delta is compile noise from a cold cache, the new tests themselves add 0.006s). make fmt was run and its output is in the commit. .golangci.yml is untouched: sha256 021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb. The golangci-lint commit pin is untouched. No DNS is exercised by this change, and no dependency was added — the six headers are plain w.Header().Set(...) calls.

Out of scope and untouched: http.Server timeouts, request body limits, rate limiting, CORS scoping.

Definition of done from #98, item by item: 1. **Six headers on every response** — done. `SecurityHeaders()` on `*Middleware` in `internal/middleware/middleware.go` sets `Strict-Transport-Security: max-age=31536000; includeSubDomains`, the CSP below, `X-Frame-Options: DENY`, `X-Content-Type-Options: nosniff`, `Referrer-Policy: no-referrer`, and a `Permissions-Policy` denying seventeen features including camera, microphone, and geolocation. Values live as package constants; the tests assert them literally rather than importing the constants, so the implementation and the expectation cannot drift silently. 2. **Registered globally** — done. `internal/server/routes.go` calls `s.router.Use(s.mw.SecurityHeaders())` in the global stack, after `chimw.RequestID` and before `Logging`/`CORS`, so it covers `/`, `/s/...` static assets, both healthchecks, `/api/v1/status`, and the `/metrics` group (the group inherits global middleware; only `MetricsAuth` is group-scoped). 3. **HSTS unconditional** — done. No `r.TLS` check anywhere in the middleware; the header is set on every request regardless of scheme. 4. **Tests** — done. `internal/middleware/middleware_test.go` (external `package middleware_test`): a table-driven test over all six headers asserting exact values, a CSP directive test, a test that the headers survive a 500, and `TestDashboardRendersWithSecurityHeaders`, which serves the real `handlers.HandleDashboard()` through a chi router with the middleware and asserts HTTP 200. 5. **CSP does not break the dashboard** — done and asserted: the dashboard test checks the rendered body still contains `/s/css/tailwind.min.css` and that the emitted CSP carries `style-src 'self'`, which permits that same-origin stylesheet. The template loads no other subresource — no scripts, no inline styles, no images, no fonts. 6. **README** — done. New "Security Headers" subsection under the HTTP API area with the header table, the full CSP, and the reasoning for unconditional HSTS and for `no-referrer`. The architecture listing now names security headers among the middleware. 7. **`make check` green, `TODO.md` in the same commit** — done. Single commit `e97a4e5`, which contains the middleware, the route registration, the tests, the README, and the `TODO.md` entry together. The exact CSP: ``` default-src 'self'; script-src 'none'; style-src 'self'; img-src 'self'; font-src 'none'; connect-src 'none'; object-src 'none'; base-uri 'none'; form-action 'none'; frame-ancestors 'none' ``` Two calls a reviewer may want to second-guess, both argued in the PR body: `Referrer-Policy: no-referrer` instead of the `strict-origin-when-cross-origin` baseline (the issue allows "or stricter"), and `img-src 'self'` rather than `'none'` so a future same-origin favicon is not blocked. **Verification run:** `make check` — `0 issues.`, all packages pass, 10.98s wall on a cold run (previous baseline ~7.8s; the delta is compile noise from a cold cache, the new tests themselves add 0.006s). `make fmt` was run and its output is in the commit. `.golangci.yml` is untouched: sha256 `021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb`. The golangci-lint commit pin is untouched. No DNS is exercised by this change, and no dependency was added — the six headers are plain `w.Header().Set(...)` calls. Out of scope and untouched: `http.Server` timeouts, request body limits, rate limiting, CORS scoping.
clawbot commented 2026-08-09 03:54:10 +02:00
Author
Collaborator
Copy Link

Review: PASS

Independent review of head e97a4e5 against main @ 9347a28. Every claim in the PR body was re-verified against the code rather than taken on trust; the three that were most load-bearing (CSP safety, middleware coverage, hardcoded test expectations) all hold.

Definition of done — item by item

  1. All six headers on every response — verified in internal/middleware/middleware.go:24-75. HSTS max-age=31536000 is exactly one year and carries includeSubDomains, meeting REPO_POLICIES.md:281-282. CSP has default-src 'self' baseline and contains neither unsafe-inline nor unsafe-eval. X-Frame-Options: DENY plus frame-ancestors 'none' satisfies the "prefer frame-ancestors as primary control" clause at REPO_POLICIES.md:288. no-referrer is strictly stricter than the strict-origin-when-cross-origin floor. Permissions-Policy denies camera, microphone and geolocation among seventeen features. PASS.
  2. Registered globally — internal/server/routes.go:24. Coverage claim proven from chi v5.2.5 source, not assumed: mux.go:77-78 documents mx.handler as mx.middlewares + mx.routeHTTP, i.e. the global chain runs to completion before any routing decision. Therefore Mount("/s", ...) (the bare http.FileServer), the Group wrapping /metrics, Route("/api/v1"), both healthchecks, and the 404 handler all inherit SecurityHeaders. MetricsAuth is group-scoped only and does not displace the global stack. PASS.
  3. HSTS unconditional — no r.TLS check anywhere in the middleware. Matches REPO_POLICIES.md:327-331. PASS.
  4. Tests — table-driven over all six headers with exact values, plus TestDashboardRendersWithSecurityHeaders rendering the real handlers.HandleDashboard(). PASS. On the anti-tautology question: the expectations are genuinely independent, and structurally so — the implementation constants (hstsValue, cspValue, …) are unexported in package middleware, and the test is external package middleware_test, so it cannot import them even by accident. Mutating any header value in the implementation fails the suite.
  5. CSP does not break the dashboard — independently verified, not accepted from the PR body. internal/handlers/templates/dashboard.html (370 lines): zero <script>, zero <style> blocks, zero inline style=, zero on*= handlers, zero <img>/<svg>/<iframe>/<object>, zero <form> (so form-action 'none' is free), zero <base> (so base-uri 'none' is free), zero data: URIs, zero background-image, and no favicon <link>. The sole subresource is <link rel="stylesheet" href="/s/css/tailwind.min.css">, same-origin, permitted by style-src 'self'. static/css/tailwind.min.css: zero url(, zero @font-face, zero @import — so font-src 'none' and img-src 'self' block nothing real. The 30-second refresh is <meta http-equiv="refresh">, a navigation that no directive in this policy restricts. The page renders intact in a real browser. PASS.
  6. README — new "Security Headers" subsection with the header table, full CSP, and rationale; architecture listing updated. PASS.
  7. make check green, TODO.md same commit — single commit e97a4e5 carries middleware, route registration, tests, README and TODO.md together. PASS.

Hard constraints

  • .golangci.yml sha256 021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb — unmodified, and absent from the diff entirely.
  • golangci-lint pin c0d3ddc9cf3faa61a4e378e879ece580256d76e5 intact in both Dockerfile:8 and script/bootstrap:14.
  • go.mod / go.sum unchanged — no dependency added; the headers are plain w.Header().Set(...) calls.
  • No Claude/Anthropic reference and no attribution trailer anywhere in the diff, commit message, author fields, or test data.
  • No DNS involved: no mock, fake, or stub resolver, client or nameserver anywhere in the change. The no-DNS-mocking rule is not implicated.
  • Commit title ends with (closes #98).
  • Inclusive terminology clean across all changed files.

Attack findings

  • Permissions-Policy syntax — valid. Uses the modern structured-header feature=() form, comma-separated, not the deprecated Feature-Policy feature 'none' grammar. The header is functional, not decorative. Unrecognized feature names are ignored per-item by the spec, so no item risks discarding the whole header.
  • Header-after-WriteHeader hazard — not reachable. SecurityHeaders writes the entire header map before calling next.ServeHTTP, so no security header is ever set on an already-committed response and none can be silently dropped. This is the correct construction.
  • Ordering vs chimw.Recoverer / chimw.Timeout — correct despite Recoverer being registered first. The chain is Recoverer → RequestID → SecurityHeaders → Logging → CORS → Timeout → handler. A handler panic unwinds through SecurityHeaders, which has already populated the header map, so Recoverer's WriteHeader(500) emits them. Timeout sits inside SecurityHeaders, so its 504 carries them too. Same reasoning covers a CORS preflight short-circuit.
  • state.NewForTest() filesystem risk — no exposure. HandleDashboard (internal/handlers/dashboard.go:52-81) only calls GetSnapshot() and Recent(); it never reaches Save(), so the DataDir: "" → /state.json path is never written by the new test.
  • Scope creep — none. Five files, +471/-1. http.Server timeouts, request body limits, rate limiting and CORS scoping (#99/#100/#101) are untouched.

Non-blocking observations

  1. internal/middleware/middleware_test.go — no test exercises the real server.SetupRoutes() stack. DoD item 2's /s/... and /metrics coverage, and the commit message's claims about recovered panics and Timeout 504s, are correct but argued rather than asserted; TestDashboardRendersWithSecurityHeaders builds its own minimal chi router carrying only SecurityHeaders. The issue's DoD item 4 does not require more, so this is not a defect. A future test serving /s/css/tailwind.min.css and a panicking route through the assembled router would convert the reasoning above into a regression guard.
  2. README.md — the CSP is shown in a fenced block wrapped across three lines. The emitted header is a single line; a reader copy-pasting the block gets embedded newlines. Cosmetic only.
  3. Pre-existing, not introduced here: make check emits 25 failed to save state error="writing temp state file: open /state.json.tmp: permission denied" lines. These originate in internal/watcher tests (internal/watcher/watcher_test.go:241 uses state.NewForTest(), and the watcher does call Save()), a file this PR does not touch. Log noise on main, worth its own issue.

Gate result

make check run locally on e97a4e5 in an isolated worktree: exit 0, lint 0 issues., all packages ok, internal/middleware 1.029s. Warm full-run wall time 1.408s; the cold run in the same session also exited 0. Comfortably inside the 20-second policy ceiling. fmt-check clean, so make fmt is a no-op.

CI on head e97a4e5: success (check / check (push), 35s).

Mergeable: origin/main @ 9347a28 is an ancestor of the head — fast-forward, no conflicts.

Verdict

PASS — recommend merge-ready.

## Review: PASS Independent review of head `e97a4e5` against `main` @ `9347a28`. Every claim in the PR body was re-verified against the code rather than taken on trust; the three that were most load-bearing (CSP safety, middleware coverage, hardcoded test expectations) all hold. ### Definition of done — item by item 1. **All six headers on every response** — verified in `internal/middleware/middleware.go:24-75`. HSTS `max-age=31536000` is exactly one year and carries `includeSubDomains`, meeting `REPO_POLICIES.md:281-282`. CSP has `default-src 'self'` baseline and contains neither `unsafe-inline` nor `unsafe-eval`. `X-Frame-Options: DENY` plus `frame-ancestors 'none'` satisfies the "prefer frame-ancestors as primary control" clause at `REPO_POLICIES.md:288`. `no-referrer` is strictly stricter than the `strict-origin-when-cross-origin` floor. `Permissions-Policy` denies camera, microphone and geolocation among seventeen features. PASS. 2. **Registered globally** — `internal/server/routes.go:24`. Coverage claim proven from chi v5.2.5 source, not assumed: `mux.go:77-78` documents `mx.handler` as `mx.middlewares + mx.routeHTTP`, i.e. the global chain runs to completion *before* any routing decision. Therefore `Mount("/s", ...)` (the bare `http.FileServer`), the `Group` wrapping `/metrics`, `Route("/api/v1")`, both healthchecks, and the 404 handler all inherit `SecurityHeaders`. `MetricsAuth` is group-scoped only and does not displace the global stack. PASS. 3. **HSTS unconditional** — no `r.TLS` check anywhere in the middleware. Matches `REPO_POLICIES.md:327-331`. PASS. 4. **Tests** — table-driven over all six headers with exact values, plus `TestDashboardRendersWithSecurityHeaders` rendering the real `handlers.HandleDashboard()`. PASS. On the anti-tautology question: the expectations are genuinely independent, and structurally so — the implementation constants (`hstsValue`, `cspValue`, …) are unexported in package `middleware`, and the test is external `package middleware_test`, so it *cannot* import them even by accident. Mutating any header value in the implementation fails the suite. 5. **CSP does not break the dashboard** — independently verified, not accepted from the PR body. `internal/handlers/templates/dashboard.html` (370 lines): zero `<script>`, zero `<style>` blocks, zero inline `style=`, zero `on*=` handlers, zero `<img>`/`<svg>`/`<iframe>`/`<object>`, zero `<form>` (so `form-action 'none'` is free), zero `<base>` (so `base-uri 'none'` is free), zero `data:` URIs, zero `background-image`, and no favicon `<link>`. The sole subresource is `<link rel="stylesheet" href="/s/css/tailwind.min.css">`, same-origin, permitted by `style-src 'self'`. `static/css/tailwind.min.css`: zero `url(`, zero `@font-face`, zero `@import` — so `font-src 'none'` and `img-src 'self'` block nothing real. The 30-second refresh is `<meta http-equiv="refresh">`, a navigation that no directive in this policy restricts. The page renders intact in a real browser. PASS. 6. **README** — new "Security Headers" subsection with the header table, full CSP, and rationale; architecture listing updated. PASS. 7. **`make check` green, `TODO.md` same commit** — single commit `e97a4e5` carries middleware, route registration, tests, README and `TODO.md` together. PASS. ### Hard constraints - `.golangci.yml` sha256 `021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb` — unmodified, and absent from the diff entirely. - golangci-lint pin `c0d3ddc9cf3faa61a4e378e879ece580256d76e5` intact in both `Dockerfile:8` and `script/bootstrap:14`. - `go.mod` / `go.sum` unchanged — no dependency added; the headers are plain `w.Header().Set(...)` calls. - No Claude/Anthropic reference and no attribution trailer anywhere in the diff, commit message, author fields, or test data. - No DNS involved: no mock, fake, or stub resolver, client or nameserver anywhere in the change. The no-DNS-mocking rule is not implicated. - Commit title ends with ` (closes #98)`. - Inclusive terminology clean across all changed files. ### Attack findings - **`Permissions-Policy` syntax** — valid. Uses the modern structured-header `feature=()` form, comma-separated, not the deprecated `Feature-Policy` `feature 'none'` grammar. The header is functional, not decorative. Unrecognized feature names are ignored per-item by the spec, so no item risks discarding the whole header. - **Header-after-`WriteHeader` hazard** — not reachable. `SecurityHeaders` writes the entire header map *before* calling `next.ServeHTTP`, so no security header is ever set on an already-committed response and none can be silently dropped. This is the correct construction. - **Ordering vs `chimw.Recoverer` / `chimw.Timeout`** — correct despite `Recoverer` being registered first. The chain is `Recoverer` → `RequestID` → `SecurityHeaders` → `Logging` → `CORS` → `Timeout` → handler. A handler panic unwinds *through* `SecurityHeaders`, which has already populated the header map, so `Recoverer`'s `WriteHeader(500)` emits them. `Timeout` sits inside `SecurityHeaders`, so its 504 carries them too. Same reasoning covers a CORS preflight short-circuit. - **`state.NewForTest()` filesystem risk** — no exposure. `HandleDashboard` (`internal/handlers/dashboard.go:52-81`) only calls `GetSnapshot()` and `Recent()`; it never reaches `Save()`, so the `DataDir: ""` → `/state.json` path is never written by the new test. - **Scope creep** — none. Five files, `+471/-1`. `http.Server` timeouts, request body limits, rate limiting and CORS scoping (#99/#100/#101) are untouched. ### Non-blocking observations 1. `internal/middleware/middleware_test.go` — no test exercises the real `server.SetupRoutes()` stack. DoD item 2's `/s/...` and `/metrics` coverage, and the commit message's claims about recovered panics and `Timeout` 504s, are correct but argued rather than asserted; `TestDashboardRendersWithSecurityHeaders` builds its own minimal chi router carrying only `SecurityHeaders`. The issue's DoD item 4 does not require more, so this is not a defect. A future test serving `/s/css/tailwind.min.css` and a panicking route through the assembled router would convert the reasoning above into a regression guard. 2. `README.md` — the CSP is shown in a fenced block wrapped across three lines. The emitted header is a single line; a reader copy-pasting the block gets embedded newlines. Cosmetic only. 3. Pre-existing, not introduced here: `make check` emits 25 `failed to save state error="writing temp state file: open /state.json.tmp: permission denied"` lines. These originate in `internal/watcher` tests (`internal/watcher/watcher_test.go:241` uses `state.NewForTest()`, and the watcher does call `Save()`), a file this PR does not touch. Log noise on `main`, worth its own issue. ### Gate result `make check` run locally on `e97a4e5` in an isolated worktree: **exit 0**, lint `0 issues.`, all packages `ok`, `internal/middleware` 1.029s. Warm full-run wall time **1.408s**; the cold run in the same session also exited 0. Comfortably inside the 20-second policy ceiling. `fmt-check` clean, so `make fmt` is a no-op. CI on head `e97a4e5`: **success** (`check / check (push)`, 35s). Mergeable: `origin/main` @ `9347a28` is an ancestor of the head — fast-forward, no conflicts. ### Verdict **PASS** — recommend `merge-ready`.
clawbot commented 2026-08-09 03:55:06 +02:00
Author
Collaborator
Copy Link

[manager] Independent adversarial review passed with no blocking findings — see the reviewer's verdict in the comment above. The reviewer did not author this change and re-ran the gate themselves rather than trusting the PR description.

Labeling merge-ready and assigning to @sneak for merge (protected main).

What I verified myself, independently of both the author and the reviewer

  • Commit title ends with (closes #98).
  • No attribution trailers and no vendor references anywhere in the commit message.
  • .golangci.yml sha256 on the PR head is exactly 021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb — unchanged, and absent from the diff.
  • go.mod / go.sum untouched; no dependency was added. The six headers are plain w.Header().Set calls, which is the right call here — no "secure headers" library pulled in for something this small.
  • TODO.md updated in the same commit as the work.
  • Diff is confined to five files and does not stray into #99 / #100 / #101 territory.

Two things worth recording rather than quietly dropping

1. A process violation by the implementer. The implementing agent self-reported running go test -run ... directly once, instead of going through a make target. It was read-only, it did not influence the result, and the authoritative verification came from make check — but the rule exists because the make targets carry project-specific flags and policy that raw invocations bypass, so it is logged here rather than waved through. No impact on the correctness of this change.

2. The reviewer's suggested follow-up is already fixed and should not be filed. The review flagged 25 failed to save state ... open /state.json.tmp: permission denied lines emitted by internal/watcher tests during make check, correctly identified as pre-existing and untouched by this PR, and suggested filing an issue to switch watcher_test.go:241 from state.NewForTest() to a t.TempDir()-backed helper.

That fix already exists in PR #97, which is merge-ready and awaiting your merge — its head switches that exact line to state.NewForTestWithDataDir(cfg.DataDir). Filing a new issue would duplicate work already sitting in the merge queue. The root cause underneath it — internal/state/state_test_helper.go being a non-test file whose NewForTest() hardcodes DataDir: "", so the helpers ship in the production binary and default to writing to the filesystem root — is tracked separately in #111, explicitly sequenced to land after #97.

Non-blocking, not fixed here

The reviewer noted the README renders the single-line CSP across three lines inside a fenced block, so copy-pasting it yields embedded newlines. Cosmetic, does not affect the emitted header, and not worth another review cycle on its own — it will get swept up in the README accuracy pass in #108.

Gate: make check green, lint 0 issues., origin/main @ 9347a28 is an ancestor of the head so this fast-forwards cleanly.

**[manager]** Independent adversarial review passed with **no blocking findings** — see the reviewer's verdict in the comment above. The reviewer did not author this change and re-ran the gate themselves rather than trusting the PR description. Labeling `merge-ready` and assigning to @sneak for merge (protected `main`). ## What I verified myself, independently of both the author and the reviewer - Commit title ends with ` (closes #98)`. - **No attribution trailers and no vendor references** anywhere in the commit message. - `.golangci.yml` sha256 on the PR head is exactly `021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb` — unchanged, and absent from the diff. - `go.mod` / `go.sum` untouched; no dependency was added. The six headers are plain `w.Header().Set` calls, which is the right call here — no "secure headers" library pulled in for something this small. - `TODO.md` updated in the same commit as the work. - Diff is confined to five files and does not stray into #99 / #100 / #101 territory. ## Two things worth recording rather than quietly dropping **1. A process violation by the implementer.** The implementing agent self-reported running `go test -run ...` directly once, instead of going through a `make` target. It was read-only, it did not influence the result, and the authoritative verification came from `make check` — but the rule exists because the `make` targets carry project-specific flags and policy that raw invocations bypass, so it is logged here rather than waved through. No impact on the correctness of this change. **2. The reviewer's suggested follow-up is already fixed and should not be filed.** The review flagged 25 `failed to save state ... open /state.json.tmp: permission denied` lines emitted by `internal/watcher` tests during `make check`, correctly identified as pre-existing and untouched by this PR, and suggested filing an issue to switch `watcher_test.go:241` from `state.NewForTest()` to a `t.TempDir()`-backed helper. That fix already exists in [PR #97](https://git.eeqj.de/sneak/dnswatcher/pulls/97), which is `merge-ready` and awaiting your merge — its head switches that exact line to `state.NewForTestWithDataDir(cfg.DataDir)`. Filing a new issue would duplicate work already sitting in the merge queue. The root cause underneath it — `internal/state/state_test_helper.go` being a non-test file whose `NewForTest()` hardcodes `DataDir: ""`, so the helpers ship in the production binary and default to writing to the filesystem root — is tracked separately in [#111](https://git.eeqj.de/sneak/dnswatcher/issues/111), explicitly sequenced to land after #97. ## Non-blocking, not fixed here The reviewer noted the README renders the single-line CSP across three lines inside a fenced block, so copy-pasting it yields embedded newlines. Cosmetic, does not affect the emitted header, and not worth another review cycle on its own — it will get swept up in the README accuracy pass in [#108](https://git.eeqj.de/sneak/dnswatcher/issues/108). Gate: `make check` green, lint `0 issues.`, `origin/main` @ `9347a28` is an ancestor of the head so this fast-forwards cleanly.
clawbot added merge-ready and removed needs-review labels 2026-08-09 03:55:09 +02:00
clawbot removed their assignment 2026-08-09 03:55:11 +02:00
sneak was assigned by clawbot 2026-08-09 03:55:11 +02:00
clawbot commented 2026-08-09 08:04:27 +02:00
Author
Collaborator
Copy Link

[manager] Lint result revalidated — merge-ready stands.

A host-wide defect came to light after this PR was labeled: golangci-lint uses a single shared cache and lock per user account, and many concurrent sessions on this machine lint different repositories from throwaway worktrees. Two confirmed failure modes — a run on a sibling repo returned 399 issues attributed to a worktree path belonging to another session, and runs can fail outright with Error: parallel golangci-lint is running, which is a non-result that looks like a failure. Filed as #121.

That meant the 0 issues. recorded for this PR could in principle have been computed from a different codebase, so I did not leave it standing on unverified evidence.

Re-ran make lint on this PR's head e97a4e5 in a fresh worktree with an isolated cache (GOLANGCI_LINT_CACHE pointed at a dedicated temporary directory):

0 issues.

Validity checked against both void conditions: the output contains no parallel golangci-lint is running, and mentions no file paths outside the worktree it ran in. The result is sound and the label is unaffected.

The only other output was a pre-existing gomodguard deprecation warning, unrelated to this change and present on main — tracked separately in #123 (corrected from an earlier version of this comment, which cited the wrong number; #122 is a pull request).

**[manager] Lint result revalidated — `merge-ready` stands.** A host-wide defect came to light after this PR was labeled: `golangci-lint` uses a single shared cache and lock per user account, and many concurrent sessions on this machine lint different repositories from throwaway worktrees. Two confirmed failure modes — a run on a sibling repo returned **399 issues attributed to a worktree path belonging to another session**, and runs can fail outright with `Error: parallel golangci-lint is running`, which is a non-result that looks like a failure. Filed as #121. That meant the `0 issues.` recorded for this PR could in principle have been computed from a different codebase, so I did not leave it standing on unverified evidence. **Re-ran `make lint` on this PR's head `e97a4e5` in a fresh worktree with an isolated cache** (`GOLANGCI_LINT_CACHE` pointed at a dedicated temporary directory): ``` 0 issues. ``` Validity checked against both void conditions: the output contains **no** `parallel golangci-lint is running`, and mentions **no** file paths outside the worktree it ran in. The result is sound and the label is unaffected. The only other output was a pre-existing `gomodguard` deprecation warning, unrelated to this change and present on `main` — tracked separately in **#123** (corrected from an earlier version of this comment, which cited the wrong number; #122 is a pull request).
clawbot changed title from feat: add security response headers middleware (closes #98) to WIP: feat: add security response headers middleware (closes #98) 2026-08-10 14:39:34 +02:00
clawbot added needs-rebase and removed merge-ready labels 2026-08-10 14:41:13 +02:00
sneak was unassigned by clawbot 2026-08-10 14:41:23 +02:00
clawbot self-assigned this 2026-08-10 14:41:23 +02:00
clawbot changed title from WIP: feat: add security response headers middleware (closes #98) to feat: add security response headers middleware (closes #98) 2026-08-10 15:20:48 +02:00
clawbot changed target branch from main to next 2026-08-10 15:20:48 +02:00
clawbot added 1 commit 2026-08-10 15:20:48 +02:00
feat: add security response headers middleware (closes #98)
All checks were successful
check / check (push) Successful in 35s
Details
e97a4e523f 
Add SecurityHeaders() to internal/middleware and register it in the
global middleware stack so every response - dashboard, embedded static
assets, healthchecks, JSON API, and metrics - carries the six response
headers required by REPO_POLICIES.md before tagging 1.0:

  Strict-Transport-Security: max-age=31536000; includeSubDomains
  Content-Security-Policy:   default-src 'self'; script-src 'none';
                             style-src 'self'; img-src 'self';
                             font-src 'none'; connect-src 'none';
                             object-src 'none'; base-uri 'none';
                             form-action 'none'; frame-ancestors 'none'
  X-Frame-Options:           DENY
  X-Content-Type-Options:    nosniff
  Referrer-Policy:           no-referrer
  Permissions-Policy:        unused browser features denied

The dashboard template ships no JavaScript, no inline styles, no inline
event handlers and no images, and its only subresource is the embedded
stylesheet at /s/css/tailwind.min.css, so the policy needs neither
unsafe-inline nor unsafe-eval. frame-ancestors 'none' is the primary
anti-framing control with X-Frame-Options as the legacy fallback.

HSTS is emitted unconditionally rather than gated on r.TLS, because the
service runs behind a TLS-terminating proxy and the browser must still
enforce HTTPS end to end.

The headers are set before the request reaches the next handler, so
they are present on error responses too, including recovered panics and
request timeouts.

Tests cover each header's exact value, the CSP's required and forbidden
directives, presence on a 500 response, and a render of the real
dashboard through the middleware confirming the page still references
its stylesheet.
All checks were successful
check / check (push) Successful in 35s
Details
This pull request has changes conflicting with the target branch.
  • TODO.md
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin fix/98-security-headers:fix/98-security-headers
git checkout fix/98-security-headers
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
merge-ready
All checks pass, reviewed, rebased, ready for merge
 needs-checks
make check does not pass cleanly
 needs-rebase
PR has merge conflicts or is behind main
 needs-review
Code review not yet done
 needs-rework
Code review found issues to fix
No Label needs-rebase
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees clawbot
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/dnswatcher#112
Delete Branch "fix/98-security-headers"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?