fix: restore dropped nginx headers and harden the runtime image - #59
Merged
Conversation
nginx's add_header does not merge across configuration levels: a block declaring any add_header discards every add_header inherited from its parent. Two location blocks each set a Cache-Control, which silently stripped CORS and all security headers from every static asset and from the whole /knowledge-base/ prefix — in practice from all traffic, leaving the server-level set applying only to / and /healthz. The shared set moves to nginx.headers.conf, included by the server block and by every location that declares a header of its own. Verified against a real nginx container: before, /knowledge-base/ carried none of the four headers; after, all five probed paths carry them. X-XSS-Protection is dropped rather than restored — the legacy auditor is gone from current browsers and the header has introduced vulnerabilities of its own. Content-Security-Policy is its replacement and is tracked separately in #42, where the inline styles and the mermaid bootstrap script can be accounted for properly. /healthz now sets its content type with default_type, since a header added after `return` never applies. The image runs as UID 101 via nginx-unprivileged instead of a root master process, and is pinned by digest the way the GitHub Actions already are. dist/ is still built outside the image, so the build now fails loudly when it lacks index.html or style.css rather than shipping an image that 404s. Closes #45 Closes #53 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QqFK6yffibtCBTF8xZ4hXW
The new image scan job did its job on its first run: the pinned 1.27-alpine digest sits on alpine 3.21.3 with openssl 3.3.3-r0, which Trivy flags for CVE-2026-31789 (heap buffer overflow parsing large X.509 certificates on 32-bit systems), fixed in 3.3.7-r0. 1.29-alpine is alpine 3.23.4 with openssl 3.5.6-r0. Rebuilt and re-verified: still UID 101, config still valid, all five probed paths still carry the full header set. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QqFK6yffibtCBTF8xZ4hXW
oto-macenauer-absa
added a commit
that referenced
this pull request
Aug 14, 2026
> **Stacked on #59.** Branched off `fix/deployment-headers-and-image` because it needs `nginx.headers.conf` and the unprivileged Dockerfile. The diff below narrows to just this work once #59 merges. ## What Nothing in CI has ever executed `nginx.conf`. Both existing suites run against `tests/fragment-server.mjs` — a hand-written Express mirror of the nginx rewrites. A mirror is only as faithful as the last person to remember to update both sides, and this one had already drifted twice: - It sent **no response headers at all**, so the two `X-Frame-Options` tests in `standalone.spec.js` were passing against a server that could not have failed them. That is part of why #45 lived as long as it did. - It answered `/knowledge-base` with a **308**, where `nginx.conf` deliberately does an internal rewrite — the config comment says a redirect would expose the container's internal HTTP address and break mixed-content under HTTPS (#60). So the suites were partly asserting against a second implementation of the contract rather than the contract. ## Changes **`playwright.config.docker.js` + `tests/container/serve.mjs` + `tests/container.spec.js`** — a fourth suite that builds and runs the production image and drives it over HTTP. 21 tests covering: - routing: `/healthz`, landing, sub-app page, `/__wf/…/style.css` rewrite, 404s, `OPTIONS` → 204 - the internal-rewrite rule, asserted with `maxRedirects: 0` — status 200 and *no* `Location` header - the full CORS + security set on six paths, which is the #45 guard against the shipped config rather than against config text - container posture: `uid=101(nginx)`, no root process, `nginx -t` valid **`npm run test:container`**, wired into the existing `image` CI job via `KB_SKIP_BUILD` so the image is built once and reused. **`tests/fragment-server.mjs`** — mirror corrected (#60). **`playwright.config.js`** — `testIgnore` now excludes `container.spec.js` too; it was being picked up by the embedded suite and run against the wrong server. ## Why not testcontainers Considered, and it does not earn its place here: - One container. No dependency graph, no database fixtures, no dynamic wiring — that is the case testcontainers exists for. - The image already exposes `/healthz` and has a `HEALTHCHECK`, so the wait strategy is a URL poll, which Playwright's `webServer` does natively. - It pulls a Ryuk sidecar — an extra moving part in a repo whose stated virtue is a dependency-light, hermetic build (one runtime dependency, `ajv`). - The repo already hand-rolls its test servers (`fragment-server.mjs`, `host/server.mjs`); a ~60-line `serve.mjs` matches that. Its genuine wins are random ports and guaranteed cleanup on crash. Ports are fixed here the way :3000 and :4201 already are, and cleanup is a `docker rm -f` in three signal handlers. ## Found while building it Probing real nginx turned up a **wider version of #60 than the issue described**. `try_files $uri $uri/index.html` serves a directory path as its index with a 200 whether or not it ends in a slash, and never redirects: | path | nginx | mirror (before) | |---|---|---| | `/knowledge-base` | 200 | 301 | | `/knowledge-base/user-guide` | 200 | 301 | `express.static` answers 301 for a slash-less directory by default. So the mirror was redirecting on *every* sub-app path, not just the one the issue named. Fixed with `redirect: false` plus an explicit `$uri/index.html` fallback, and both suites now assert the no-redirect contract on both paths. This is exactly the class of thing the suite exists to find, on its first run. ## Verification ``` npm run test:container → 21 passed (new; cold build from scratch) npx playwright test --config=playwright.config.ci.js → 22 passed (was 20; +2) npm test → 57 passed ``` ## Scope limit This verifies nginx faithfully. It does **not** cover the web-fragments gateway in front of it in production — that remains the embedded harness's job. It closes the mirror-drift gap, not the whole deployment path. Closes #60
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Two deployment-layer issues, grouped because they are the same three files and the header fix needs a new file that the Dockerfile has to ship.
The header bug — #45
add_headerin nginx does not merge across configuration levels. A block that declares anyadd_headerdiscards everyadd_headerinherited from its parent. Two location blocks each set aCache-Control, and in doing so silently dropped the CORS and security set declared atserverlevel:location ~* \.(css|js|woff2?|…)$→ every static assetlocation ^~ /knowledge-base/→ every pageBetween them that is all real traffic; the server-level headers only ever applied to
/and/healthz.Verified against a real nginx container rather than argued from the docs — the pre-fix config, same probe:
/knowledge-base//knowledge-base/style.css/__wf/knowledge-base/style.css/Fix
The shared set moves to
nginx.headers.conf, included by the server block and by every location that declares a header of its own. One place to add a header; the include is what makes it reach everything.It lives outside
conf.d/deliberately — nginx loadsconf.d/*.confas top-level server configuration, and this is a fragment.Also in this file
X-XSS-Protectiondropped, not restored. The legacy XSS auditor is gone from every current browser and the header has a history of introducing vulnerabilities rather than preventing them.<style>inBase.astro, the shadow-DOM compat styles and the mermaid bootstrap script can be accounted for — and where it can be tested against the reframed iframe. A CSP guessed at in this PR would break the fragment silently, which is the failure mode the existingX-Frame-Optionstest exists to prevent./healthzset its content type with anadd_headerplaced afterreturn 200, which never applies. Nowdefault_type.The image — #53
nginxinc/nginx-unprivilegedinstead of a root master process. The container serves static files on 8080 and never needed the privilege. Confirmed in the running container:uid=101(nginx), no root process inps..github/workflows/is already pinned. Dependabot bumps the tag; the digest stops the deployment moving underneath it meanwhile.RUN rm-ed — a non-root image cannot delete files under/etc/nginx.dist/staleness guard.dist/is built outside the image and is not reproducible from the Dockerfile alone. Rather than convert to a multi-stage build — which would break the deploy pipeline's ability to build the image from a CI-produceddist/artifact — the build now fails loudly whendist/lacksindex.htmlorstyle.css, instead of shipping an image that 404s.New CI job
image— builds the runtime image from thebuildjob'sdist/artifact and scans it with Trivy.npm auditcovers JS dependencies only; nothing in CI looked at the base image, which is what the digest pin exists to control. CRITICAL-only andignore-unfixed, so a routine base-image CVE does not block unrelated PRs; the fix when it does fire is to bump the pinned digest. The job doubles as proof that the Dockerfile'sdist/guards pass on a real build.Verification
New
tests/nginx-config.spec.jsassertsnginx.confas text, because no nginx runs anywhere in CI — the E2E suites hittests/fragment-server.mjs, an Express mirror of the rewrites. It checks that everylocationdeclaring anadd_headeralso includes the shared snippet, that the shared set is defined in exactly one place, thatX-Frame-Optionsis notDENY, and thatX-XSS-Protectionis absent. Confirmed the guard reports exactly the two originally-broken blocks when the includes are removed.tests/fragment-server.mjsnow mirrors the header set too, so the five new assertions instandalone.spec.jsare not vacuous — the two existingX-Frame-Optionstests had been passing against a server that sent no headers at all.Noticed, not fixed
location ^~ /knowledge-base/takes precedence over the regex asset block, so theexpires 1y; immutablepolicy never applies to knowledge-base assets — they getno-transforminstead. Pre-existing, unrelated to the header bug, and changing it is a caching behaviour change rather than a fix. Worth its own issue; interacts with thestyle.csscaching point already raised in #50.Closes #45
Closes #53