Conversation
…errer-Policy on proxied API responses - /assets/ location now repeats the security header trio explicitly because its own Cache-Control add_header breaks server-level inheritance - /v1/ location suppresses inherited security headers with empty-value add_header entries so only the API's own headers reach the client - Header ownership rule documented in the config template Refs bytefolk#135
Review — the change is correct; holding on a scope collision with #143I verified this at runtime rather than by reading the config, because #135 was filed as Method: built nginx Both defects reproduce as described, and the fix resolves them
Honest caveats so this is reproducible: my minimal local build excludes the HTTP rewrite module, so One narrowing this introduces — worth a decision, not a redesignCutting inheritance in Exploitability is low — static nginx error pages,
Two nits
On severitynginx appends after upstream headers, so on The actual blocker: #143 fixes the same defect with the opposite design#143 (from So before either one moves, the decision #135 asked for has to actually be made — who owns security headers on proxied responses, the API or the edge? — and one of the two PRs closed. My own read favors this PR's rule (the API stays correct without nginx, per REQ-003, and the edge stops inventing policy), with #143's Related process items, all currently observable:
No objection to the diff itself, and the header dumps above can be attached to #135 to move it to |
|
Closing under the fork-workflow decision recorded on 2026-09-03: repository #135 and #136, re-landed as #161. The diagnosis here was correct and is the one #135 recorded: #161 takes the same fix through the ownership split that was decided for #136 on The commits are not lost. A closed fork PR keeps its head ref: git fetch https://github.com/bytefolk/mem.git refs/pull/144/head:pr-144Every file in this branch was therefore available to the re-doing work, whether |
… headers (#161) ## What this changes Implements the security-header split decided for this repository on 2026-09-03: **nginx is the single authority for `X-Content-Type-Options`, `X-Frame-Options` and `Referrer-Policy`, uniformly `no-referrer`, with the proxied path de-duplicated by `proxy_hide_header`; the API keeps `Content-Security-Policy`, `X-XSS-Protection` and `Content-Disposition`.** Closes #135 (the two live proxy defects) and Closes #136 (the ownership question that defect raised). This is the in-repo redo of #144, which came from a fork and is being closed under the fork-policy decision. It is not a cherry-pick of that commit — no commit from it is imported here. ### The two defects, as measured Both were real on `main@1332bf4` and neither is visible by reading the config, which is why the test starts an nginx rather than grepping one. 1. **`/assets/` lost all three headers.** An `add_header` inside a location replaces the inherited set instead of adding to it, and that block has always had `add_header Cache-Control`. Every cached bundle shipped with no `nosniff`, no `X-Frame-Options` and no `Referrer-Policy` at all. 2. **`/v1/` sent two conflicting `Referrer-Policy` values on one response.** The proxy said `same-origin`, the API says `no-referrer`, and nginx's `add_header` appends rather than replaces, so a client received both and the effective policy depended on which one the browser kept. `X-Content-Type-Options` and `X-Frame-Options` were also duplicated (same value twice). ### Why the API keeps sending the three it no longer owns `proxy_hide_header` makes the wire value nginx's, which is what "nginx 独占" requires, without deleting `nosniff`/`DENY`/`no-referrer` from `securityHeadersMiddleware`. A `memd` reached directly — the Helm path exposes the Service, and `docs/DEPLOYMENT.md` only makes the nginx guarantee for the container — keeps its defense in depth. If the intended reading was instead that the Go middleware should stop setting them, that is a one-line change here and it should be said before merge, because the alternative silently weakens the no-proxy deployment. ## Test evidence `scripts/test_nginx_security_headers.sh` renders the shipped template with the same `envsubst` filter and variables the container entrypoint uses, runs a fake upstream that answers exactly like `securityHeadersMiddleware` does, starts nginx against the config, and reads headers off the wire across five surfaces (`/`, `/assets/`, a 404 under `/assets/`, `/v1/`, `/healthz`). | run | result | | --- | --- | | template as shipped on `main` | **11 of 28 assertions fail** — 6 missing across the two `/assets/` surfaces, `same-origin` on `/` and `/healthz`, 3 duplicated on `/v1/` | | this branch | **28 of 28 pass** | | drop the `/assets/` restatement | 3 fail (`/assets/` 200) | | drop `always` from the `/assets/` restatement | 3 fail (`/assets/` 404 only) | | drop `proxy_hide_header` | 3 fail (`/v1/` duplicates) | Each of the three fix sites is therefore individually load-bearing, and the 404 surface earns its place: it is the only one that tests `always`. The harness also refuses to report success on a partial run (the assertion count is pinned), hard-fails when a caller names an `NGINX_BIN` that is not executable, and only reports `SKIP` when no nginx was found at all — so the CI leg cannot go green by measuring nothing. Executed locally against nginx **1.27.4**, the same minor the `web/Dockerfile` pins (`nginxinc/nginx-unprivileged:1.27.4-alpine3.21`). **Measured in CI on the built image, and it did go red there first.** The first run of the new step failed exactly where this paragraph expected the risk to be: `web/Dockerfile:17` ends on `nginxinc/nginx-unprivileged`, whose own build stops at `USER 101`, so `apk add` could not write the package database -- `ERROR: Unable to lock database: Permission denied`, `exit code: 99`. Commit `1c917f96` installs the four tools in a throwaway child image that returns to uid 101 afterwards, so the harness still runs unprivileged, as the shipped container does. On that head `Deployment profiles` is green and its job log carries `all 28 security-header assertions passed`, so the leg executed the contract against the nginx that ships rather than printing `SKIP`. The harness content measured above is unchanged by that commit; it touches only the workflow step. ## Not in scope here, and worth a decision Nothing in this repository sets a `Content-Security-Policy` for the SPA itself — `default-src 'none'` is an API-only value and would break the web app if applied to it. The decided split covers the three shared headers and leaves HTML/SPA CSP unowned. This PR deliberately does not invent one. ## Links and state - `Refs` the decided split; `Closes #135` / `Closes #136` on merge. - No Go code changed, so the existing `security_headers_test.go` and `content_*` tests are unaffected. - `Web` is red on this head and identically red on `main` itself (`Audit dependencies`, `browserslist <=4.28.6`, GHSA-73wf-gq98-2v4g). That is a pre-existing repository condition rather than something this branch introduced, and the queued fix is #159; the readback is on #138. - Opening this PR is not a claim that it should be merged. Independent review is the reviewers'; I have not requested or recorded one. --------- Co-authored-by: waterbro-8 <318569545+waterbro-8@users.noreply.github.com>
3 participants
Tracking
Refs #135
Problem
Two defects in
web/nginx/default.conf.template:/assets/loses all security headers. nginxadd_headerinheritance is suppressed when a nested block defines anyadd_headerof its own.location /assets/declaresCache-Control, so the three server-level security headers (X-Content-Type-Options,Referrer-Policy,X-Frame-Options) are not inherited.Proxied API responses carry duplicate
Referrer-Policywith conflicting values.location /v1/inherits the server-level trio and proxies to the API, which sets its ownReferrer-Policy: no-referrer. The response carries bothno-referrer(from API) andsame-origin(from nginx). RFC 9191 says the receiver uses the first value, so the effective policy depends on ordering rather than intent.Fix
/assets/: Repeat the security header trio explicitly in this block./v1/: Suppress inherited security headers using nginx's documented empty-value rule (add_headerwith empty string value is skipped), so only the API's own headers reach the client.Requirements met
/v1/suppresses inherited headers via empty-valueadd_header/assets/now repeats the trio explicitlydefault.conf.templateNon-goals