test: run integration tests against the real nginx image - #61
Merged
Conversation
Nothing in CI has ever executed nginx.conf. The two existing suites run against tests/fragment-server.mjs, a hand-written Express mirror of the nginx rewrites — and a mirror is only as faithful as the last person to update both sides. It had already drifted twice: it sent no response headers at all, so the two X-Frame-Options tests were passing against a server that could not have failed them (#45), and it answered /knowledge-base with a 308 where nginx.conf deliberately does an internal rewrite to avoid leaking the container address over HTTPS (#60). Adds a fourth suite that drives the actual production image and asserts the routing contract, the header set, and the container's runtime posture against the shipped config. It needs Docker, so it is deliberately not part of `npm test`, which stays hermetic; CI runs it inside the image job that already builds the image. Testcontainers was considered and not used: there is one container, no dependency graph and no fixtures to wire, the image already exposes /healthz for the wait, and Ryuk would add a sidecar to a repo whose build is otherwise dependency-light. Playwright's own webServer covers it. Probing real nginx also turned up a wider version of #60 than the issue described: try_files serves a directory path as its index with a 200 whether or not it ends in a slash, and never redirects, while express.static answers 301 by default. Both halves of the mirror are corrected and both suites now assert the no-redirect contract. Closes #60 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 #61.** The CSP is verified by the container suite that PR adds. The diff narrows once #61 merges. ## What #42's core vulnerability, its defence-in-depth half, and the related hardening in #55. ## 1. The actual fix — publish-time sanitisation `markdown-it` ran with `html: true` and nothing sanitised the result. That output is re-hosted with `set:html` on the knowledge base's **own origin**, next to every other team's docs. So a `<script>` or an `<img onerror>` in any onboarding repo's markdown executed against every other doc's page. `markdown-it`'s `validateLink` blocks `javascript:` in *markdown* links; raw HTML bypasses it entirely. `actions/publish-single-page-docs/src/sanitize.js` puts rendered HTML through an allowlist before it is ever packed into an artifact. Raw HTML stays supported — only the subset the doc stylesheet actually renders survives. Dropped: `<script>`, `on*` handlers, `<style>` and `style=`, `<iframe>`/`<object>`/`<embed>`, forms and form controls, `javascript:`/`data:` URLs. Inner text is kept where there is any, because losing a paragraph to an unsupported wrapper is a worse failure than losing the wrapper. `contract/SINGLE_PAGE.md` documents exactly what is kept and dropped, since this is a behaviour change for every onboarding repo. ## 2. The CSP — and the two things that had to happen first The policy's point is `script-src 'self'` with **no `'unsafe-inline'`**. Getting there was the interesting part. **The action's mermaid bootstrap was an inline `<script>`.** Now `assets/mermaid-init.js`. Hashing it instead would have hard-coupled nginx's config to the action's exact bytes, breaking bundles published by any other version. **Already-published bundles still contain the inline version.** Confirmed against the sibling example repo's real `dist.tar.gz`: `example-service/index.html` ships one inline script. This repo does not control when those repos re-publish, so enforcing `script-src 'self'` would have **silently killed every already-published doc's diagrams** — no error, no failed request, just diagrams that stopped rendering. So `scripts/hoist-inline-scripts.js` moves any inline script found in a sub-app artifact into a file at build time, before anything else reads `apps/`. On this build it hoisted 12 scripts across 4 apps — the vendored docs-example fixture alone had 5, so this was never only about mermaid. Content-addressed filenames, original attributes preserved, and a classic `<script src>` blocks the parser exactly like an inline one, so execution order is unchanged. **Verified in a real browser, against the real 2.5 MB mermaid bundle, on a doc published by the old action:** diagrams render as SVG, zero CSP violations. ### What the policy does not do `style-src` keeps `'unsafe-inline'`. `Base.astro`'s `@layer` ordering fix, the shadow-DOM compat styles and sub-app `style=` attributes are all inline, and CSS is a much weaker primitive than script. Tightening it means hoisting styles the same way — worth doing, not worth blocking this on. `frame-src` allows `https:` because `type: "iframe"` entries embed arbitrary external sites. ## 3. #55 — step outputs out of the `run:` body `${{ }}` is substituted into the shell text before bash sees it. The values are validated kebab-case slugs today so this was not exploitable — but that safety rested on a regex in a different file, for a reusable action other repositories run with `contents: write`. Now passed through `env:`, matching what the upload step already did. ## Verification ``` action self-test → 20 checks (was 11; +9) npm run test:container → 28 passed (was 21; +7) npx playwright test --config=playwright.config.ci.js → 22 passed npm test → 61 passed (was 57; +4) ``` The self-test asserts the allowlist rather than describing it — each case is markdown a doc repo could commit today, and the "keeps what the contract promises" case guards against over-sanitising. The container suite asserts the CSP **in a browser**, not just as a header: a policy that blocks something needed fails silently, and only a browser reports it. `build-integrity.spec.js` fails the build if any inline script reaches `dist/`, so the CSP can never quietly become wrong. Caught during this work: my allowlist initially dropped `<label>`, which would have cost task-list checkboxes their accessible name and click target. The self-test now pins it. ## Follow-ups worth filing - Hoist inline `<style>` the same way, then drop `'unsafe-inline'` from `style-src`. - `#54` (self-hosting Inter) would let `style-src`/`font-src` drop the Google Fonts origins. Closes #42 Closes #55
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
Nothing in CI has ever executed
nginx.conf. Both existing suites run againsttests/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:X-Frame-Optionstests instandalone.spec.jswere passing against a server that could not have failed them. That is part of why Security: nginx location blocks silently drop every inherited CORS and security header #45 lived as long as it did./knowledge-basewith a 308, wherenginx.confdeliberately does an internal rewrite — the config comment says a redirect would expose the container's internal HTTP address and break mixed-content under HTTPS (The Express nginx mirror contradicts production on the no-trailing-slash rule #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:/healthz, landing, sub-app page,/__wf/…/style.cssrewrite, 404s,OPTIONS→ 204maxRedirects: 0— status 200 and noLocationheaderuid=101(nginx), no root process,nginx -tvalidnpm run test:container, wired into the existingimageCI job viaKB_SKIP_BUILDso the image is built once and reused.tests/fragment-server.mjs— mirror corrected (#60).playwright.config.js—testIgnorenow excludescontainer.spec.jstoo; 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:
/healthzand has aHEALTHCHECK, so the wait strategy is a URL poll, which Playwright'swebServerdoes natively.ajv).fragment-server.mjs,host/server.mjs); a ~60-lineserve.mjsmatches 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 -fin 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.htmlserves a directory path as its index with a 200 whether or not it ends in a slash, and never redirects:/knowledge-base/knowledge-base/user-guideexpress.staticanswers 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 withredirect: falseplus an explicit$uri/index.htmlfallback, 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
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