diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 916a98a..61c0748 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -8,7 +8,7 @@ # typecheck — astro check (TypeScript / Astro type errors) # build — headless build from the vendored fixture; uploads dist/ # e2e — Playwright: embedded web-fragment harness + standalone layer -# image — docker build of the runtime image + Trivy scan +# image — docker build + container integration tests against real nginx + Trivy scan # audit — npm dependency vulnerability gate name: CI @@ -169,11 +169,16 @@ jobs: # exists to control. CRITICAL-only so a routine base-image CVE does not block # unrelated PRs; the fix is to bump the pinned digest. image: - name: Image build + scan + name: Image build + integration + scan runs-on: ubuntu-latest needs: build steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + - uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0 + with: + node-version: '24' + cache: npm + - run: npm ci - name: Download dist artifact uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 with: @@ -182,6 +187,15 @@ jobs: # Also proves the Dockerfile's dist/ sanity checks pass on a real build. - name: Build image run: docker build -t knowledge-base:ci . + # The only place nginx.conf itself is executed. The other suites run + # against tests/fragment-server.mjs, an Express mirror of the rewrites — + # see playwright.config.docker.js for why that is not sufficient. + # KB_SKIP_BUILD reuses the image built above instead of building twice. + - name: Container integration tests + run: npm run test:container + env: + KB_IMAGE: knowledge-base:ci + KB_SKIP_BUILD: 'true' - name: Scan image uses: aquasecurity/trivy-action@ed142fd0673e97e23eac54620cfb913e5ce36c25 # v0.36.0 with: @@ -190,6 +204,13 @@ jobs: exit-code: '1' ignore-unfixed: true severity: CRITICAL + - name: Upload Playwright report + if: failure() + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + with: + name: playwright-report-container + path: playwright-report/ + retention-days: 7 # ── 6. Dependency audit ──────────────────────────────────────────────────── audit: diff --git a/AGENTS.md b/AGENTS.md index d348e63..1b8d9ec 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -89,6 +89,20 @@ The build and both suites are hermetic: they use the committed no sibling repository is required. If a change makes any of them need network, that is the bug — fix the change, not the test. +A fourth suite needs Docker and is therefore **not** part of `npm test`: + +```bash +npm run test:container # integration tests against the real nginx image +``` + +Run it when touching `nginx.conf`, `nginx.headers.conf` or the `Dockerfile`. It +is the only place the shipped config is executed — the other suites run against +`tests/fragment-server.mjs`, an Express mirror of the nginx rewrites. That mirror +is a second implementation of the same contract and it has drifted twice +(#45, #60), so **a change to nginx behaviour means changing both, and proving it +with this suite.** CI runs it inside the `image` job, which already builds the +image. + `npm audit --omit=dev --audit-level=high` must also stay clean; it gates CI. ## Repository conventions diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 1b23618..e357a6d 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -28,6 +28,7 @@ opening your first PR, and required reading for automated contributors. |---|---| | `npm test` | Embedded web-fragment harness (`playwright.config.js`) — host gateway proxies/embeds the fragment on `:4201`. | | `npx playwright test --config=playwright.config.ci.js` | Standalone fragment-server layer (`:3000`) — headers, headless contract, asset routing, #297. | +| `npm run test:container` | Integration tests against the real nginx image. **Needs Docker**, so it is not part of `npm test`. Run it when changing `nginx.conf`, `nginx.headers.conf` or the `Dockerfile`. | Both run in CI (`.github/workflows/ci.yml`). Please make sure both pass before opening a PR. diff --git a/README.md b/README.md index 941d347..f7af070 100644 --- a/README.md +++ b/README.md @@ -371,7 +371,9 @@ knowledge-base/ │ ├── standalone.spec.js ← Standalone fragment-server suite │ ├── build-integrity.spec.js │ ├── artifact-safety.spec.js ← Tarball extraction guards -│ ├── nginx-config.spec.js ← nginx header-inheritance guard +│ ├── nginx-config.spec.js ← nginx header-inheritance guard (static) +│ ├── container.spec.js ← Integration suite vs. the real nginx image +│ ├── container/serve.mjs ← Builds + runs the image for that suite │ ├── host/server.mjs ← Reference web-fragments host (gateway) │ ├── fragment-server.mjs ← nginx-mirroring static server │ ├── support/fragment.js ← Shadow-DOM test helpers diff --git a/package.json b/package.json index 96c749f..8acd925 100644 --- a/package.json +++ b/package.json @@ -24,7 +24,8 @@ "test": "npx playwright test", "test:ui": "npx playwright test --ui", "test:headed": "npx playwright test --headed", - "test:debug": "npx playwright test --debug" + "test:debug": "npx playwright test --debug", + "test:container": "npx playwright test --config=playwright.config.docker.js" }, "dependencies": { "ajv": "^8.17.1" diff --git a/playwright.config.docker.js b/playwright.config.docker.js new file mode 100644 index 0000000..73b2567 --- /dev/null +++ b/playwright.config.docker.js @@ -0,0 +1,54 @@ +// playwright.config.docker.js +// +// Container integration layer — drives the real production image (nginx serving +// dist/) instead of the Express mirror the other two suites use. +// +// Run with: +// npm run build:headless +// npx playwright test --config=playwright.config.docker.js +// +// The other suites run against tests/fragment-server.mjs, a hand-written mirror +// of the nginx rewrites. A mirror is only as faithful as the last person to +// update both sides: #45 (locations silently dropping inherited headers) and #60 +// (a 308 where nginx does an internal rewrite) both survived because nothing +// ever exercised nginx.conf itself. This suite closes that gap. +// +// NOT hermetic — it needs Docker and `docker build` pulls the base image, so it +// is deliberately excluded from `npm test`. In CI it runs inside the job that +// already builds the image, reusing it via KB_SKIP_BUILD. +// +// Scope note: this verifies nginx faithfully. It does not cover the +// web-fragments gateway that sits in front of it in production — that is what +// the embedded harness in playwright.config.js is for. + +import { defineConfig } from '@playwright/test'; + +const PORT = process.env.KB_CONTAINER_PORT || '8099'; + +export default defineConfig({ + testDir: './tests', + testMatch: '**/container.spec.js', + fullyParallel: true, + retries: process.env.CI ? 1 : 0, + reporter: [['list'], ['html', { open: 'never' }]], + timeout: 20_000, + + use: { + baseURL: `http://localhost:${PORT}`, + }, + + webServer: { + command: 'node tests/container/serve.mjs', + // /healthz is the container's own health endpoint — the same one the + // Dockerfile HEALTHCHECK uses. + url: `http://localhost:${PORT}/healthz`, + reuseExistingServer: !process.env.CI, + // A cold `docker build` pulls the base image; give it room. + timeout: 300_000, + stdout: 'pipe', + stderr: 'pipe', + }, + + // No browser needed — every assertion is an HTTP request against nginx. + projects: [{ name: 'nginx' }], +}); diff --git a/playwright.config.js b/playwright.config.js index f02e7dc..a0c7037 100644 --- a/playwright.config.js +++ b/playwright.config.js @@ -16,10 +16,11 @@ import { defineConfig, devices } from '@playwright/test'; */ export default defineConfig({ testDir: './tests', - // standalone.spec.js targets the fragment server directly on :3000 — it has its - // own config (playwright.config.ci.js). This embedded config drives the host - // gateway on :4201, so exclude it here. - testIgnore: '**/standalone.spec.js', + // Each of these targets a different server and has its own config: + // standalone.spec.js → the fragment server on :3000 (playwright.config.ci.js) + // container.spec.js → the real nginx image (playwright.config.docker.js) + // This embedded config drives the host gateway on :4201, so exclude both. + testIgnore: ['**/standalone.spec.js', '**/container.spec.js'], fullyParallel: false, // fragments share DOM/history — keep navigation sequential retries: process.env.CI ? 1 : 0, workers: 1, diff --git a/tests/container.spec.js b/tests/container.spec.js new file mode 100644 index 0000000..b108d41 --- /dev/null +++ b/tests/container.spec.js @@ -0,0 +1,176 @@ +/** + * tests/container.spec.js + * + * Integration tests against the real production image — nginx serving dist/, + * driven through playwright.config.docker.js. + * + * Everything here is a claim that only the actual nginx.conf can settle: + * location precedence, internal rewrites, `add_header` inheritance, and the + * container's own runtime posture. The other two suites run against + * tests/fragment-server.mjs, an Express mirror — useful and fast, but it is a + * second implementation of the same contract and it has drifted twice (#45, + * #60). This file is the one that reads the shipped config. + */ + +import { test, expect } from '@playwright/test'; +import { execFileSync } from 'node:child_process'; + +const CONTAINER = 'kb-container-test'; + +/** The set nginx.headers.conf defines and every location block must include. */ +const SHARED_HEADERS = { + 'access-control-allow-origin': '*', + 'x-content-type-options': 'nosniff', + 'x-frame-options': 'SAMEORIGIN', + 'referrer-policy': 'strict-origin', +}; + +// ───────────────────────────────────────────────────────────────────────────── +// Routing contract +// ───────────────────────────────────────────────────────────────────────────── + +test.describe('routing', () => { + test('healthz responds 200 as plain text', async ({ request }) => { + const res = await request.get('/healthz'); + expect(res.status()).toBe(200); + expect(res.headers()['content-type']).toContain('text/plain'); + expect((await res.text()).trim()).toBe('ok'); + }); + + test('the landing catalog is served', async ({ request }) => { + const res = await request.get('/knowledge-base/'); + expect(res.status()).toBe(200); + expect(res.headers()['content-type']).toContain('text/html'); + }); + + test('a packaged sub-app page is served', async ({ request }) => { + const res = await request.get('/knowledge-base/user-guide/'); + expect(res.status()).toBe(200); + expect(await res.text()).toContain('data-mp-headless'); + }); + + // The production-only rule. nginx.conf uses an INTERNAL rewrite here, not a + // redirect, because a 301 would expose the container's internal HTTP address + // to the browser and break mixed-content under HTTPS. The Express mirror sent + // a 308 instead (#60) — this is the assertion that tells the two apart. + test('/knowledge-base without a trailing slash is rewritten internally, not redirected', async ({ request }) => { + const res = await request.get('/knowledge-base', { maxRedirects: 0 }); + expect(res.status(), 'a 3xx here leaks the internal container address').toBe(200); + expect(res.headers()['location'], 'no Location header — this must not be a redirect').toBeUndefined(); + expect(res.headers()['content-type']).toContain('text/html'); + }); + + // try_files $uri $uri/index.html — nginx serves a directory path as its index + // with a 200 whether or not it has a trailing slash, and never redirects. + // express.static does the opposite by default (301), which is what made the + // mirror diverge; see tests/fragment-server.mjs. + test('a sub-app page without a trailing slash is served, not redirected', async ({ request }) => { + const res = await request.get('/knowledge-base/user-guide', { maxRedirects: 0 }); + expect(res.status()).toBe(200); + expect(res.headers()['location']).toBeUndefined(); + }); + + // Sub-app pages hardcode this path; the gateway/nginx rewrite is what makes it + // resolve to dist/style.css. If it 404s, every sub-app page loses its styling. + test('the fragment-prefixed marketplace stylesheet resolves', async ({ request }) => { + const res = await request.get('/__wf/knowledge-base/style.css'); + expect(res.status()).toBe(200); + expect(res.headers()['content-type']).toContain('text/css'); + }); + + test('the marketplace stylesheet also resolves under the normal prefix', async ({ request }) => { + const res = await request.get('/knowledge-base/style.css'); + expect(res.status()).toBe(200); + expect(res.headers()['content-type']).toContain('text/css'); + }); + + test('an unknown sub-app path is a 404, not the landing page', async ({ request }) => { + const res = await request.get('/knowledge-base/no-such-app/'); + expect(res.status()).toBe(404); + }); + + test('an unknown top-level path is a 404', async ({ request }) => { + const res = await request.get('/no-such-thing'); + expect(res.status()).toBe(404); + }); + + test('OPTIONS preflight is answered without hitting try_files', async ({ request }) => { + const res = await request.fetch('/knowledge-base/', { method: 'OPTIONS' }); + expect(res.status()).toBe(204); + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// Header inheritance +// +// This is the #45 regression guard, asserted against the shipped config rather +// than against nginx.conf as text (tests/nginx-config.spec.js) or against a +// mirror that sends whatever it was told to (tests/standalone.spec.js). +// ───────────────────────────────────────────────────────────────────────────── + +test.describe('response headers', () => { + const paths = [ + ['the landing page', '/knowledge-base/'], + ['a sub-app page', '/knowledge-base/user-guide/'], + ['a static asset', '/knowledge-base/style.css'], + ['a fragment-prefixed asset', '/__wf/knowledge-base/style.css'], + ['the health endpoint', '/healthz'], + ['the no-trailing-slash path', '/knowledge-base'], + ]; + + for (const [label, path] of paths) { + test(`CORS and security headers reach ${label}`, async ({ request }) => { + const res = await request.get(path, { maxRedirects: 0 }); + const headers = res.headers(); + for (const [name, value] of Object.entries(SHARED_HEADERS)) { + expect( + (headers[name] ?? '').toUpperCase(), + `${name} missing on ${path} — a location block declaring add_header ` + + 'discards every inherited add_header unless it includes kb-headers.conf', + ).toBe(value.toUpperCase()); + } + }); + } + + test('X-Frame-Options is never DENY — it would block the web-fragments iframe', async ({ request }) => { + for (const [, path] of paths) { + const res = await request.get(path, { maxRedirects: 0 }); + expect((res.headers()['x-frame-options'] ?? '').toUpperCase()).not.toBe('DENY'); + } + }); + + test('the deprecated X-XSS-Protection header is not sent', async ({ request }) => { + const res = await request.get('/knowledge-base/'); + expect(res.headers()['x-xss-protection']).toBeUndefined(); + }); + + test('knowledge-base responses carry Cache-Control: no-transform', async ({ request }) => { + // Stops an intermediate proxy (the FragmentGateway) re-encoding the body and + // leaking a Content-Encoding header the browser then fails to decode. + const res = await request.get('/knowledge-base/'); + expect(res.headers()['cache-control']).toContain('no-transform'); + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// Container posture +// ───────────────────────────────────────────────────────────────────────────── + +test.describe('container', () => { + test('nginx does not run as root', () => { + const id = execFileSync('docker', ['exec', CONTAINER, 'id'], { encoding: 'utf8' }); + expect(id, 'the image must run unprivileged — see the Dockerfile').toContain('uid=101(nginx)'); + + const processes = execFileSync('docker', ['exec', CONTAINER, 'ps', '-o', 'user,comm'], { encoding: 'utf8' }); + const owners = processes.trim().split('\n').slice(1).map((l) => l.trim().split(/\s+/)[0]); + expect(owners.filter((u) => u === 'root'), 'no nginx process may run as root').toEqual([]); + }); + + test('the shipped nginx config is valid', () => { + const out = execFileSync('docker', ['exec', CONTAINER, 'nginx', '-t'], { + encoding: 'utf8', + stdio: ['pipe', 'pipe', 'pipe'], + }); + expect(out + '').toBeDefined(); + }); +}); diff --git a/tests/container/serve.mjs b/tests/container/serve.mjs new file mode 100644 index 0000000..f9ec5ee --- /dev/null +++ b/tests/container/serve.mjs @@ -0,0 +1,79 @@ +/** + * tests/container/serve.mjs + * + * Builds and runs the production runtime image, for Playwright's `webServer` to + * drive. Used by playwright.config.docker.js. + * + * WHY A REAL CONTAINER + * + * tests/fragment-server.mjs is a hand-written Express mirror of the nginx + * rewrites. It is what the other two suites run against, and it is only as + * faithful as the last person to remember to update both — which is how #45 + * (every location block silently dropping the inherited headers) and #60 (the + * mirror sending a 308 where nginx does an internal rewrite) both survived. This + * suite asserts the routing and header contract against the actual nginx.conf, + * in the actual image, so drift is caught rather than assumed absent. + * + * Deliberately NOT part of `npm test`: that suite is hermetic and Docker-free, + * and `docker build` pulls a base image. This runs in the CI job that already + * builds the image, so it costs no extra build. + * + * Env: + * KB_IMAGE image tag to run (default knowledge-base:container-test) + * KB_SKIP_BUILD skip `docker build` (set in CI, where the image exists) + * KB_CONTAINER_PORT host port (default 8099) + */ + +import { spawn, spawnSync } from 'node:child_process'; +import { dirname, join } from 'node:path'; +import { fileURLToPath } from 'node:url'; + +const ROOT = join(dirname(fileURLToPath(import.meta.url)), '..', '..'); + +const IMAGE = process.env.KB_IMAGE || 'knowledge-base:container-test'; +const PORT = process.env.KB_CONTAINER_PORT || '8099'; +const NAME = 'kb-container-test'; + +function run(cmd, args, opts = {}) { + const res = spawnSync(cmd, args, { stdio: 'inherit', ...opts }); + if (res.error) throw res.error; + return res.status; +} + +/** Best-effort removal — a stale container from a killed run must not block this one. */ +function removeContainer() { + spawnSync('docker', ['rm', '-f', NAME], { stdio: 'ignore' }); +} + +if (run('docker', ['version', '--format', '{{.Server.Version}}'], { stdio: 'ignore' }) !== 0) { + console.error('✗ Docker is not available — this suite needs it. Run `npm test` for the Docker-free suites.'); + process.exit(1); +} + +if (!process.env.KB_SKIP_BUILD) { + console.log(`▶ building ${IMAGE}…`); + // Fails fast and loudly when dist/ is missing: the Dockerfile checks for it. + if (run('docker', ['build', '-t', IMAGE, '.'], { cwd: ROOT }) !== 0) { + console.error('✗ docker build failed — did you run `npm run build:headless` first?'); + process.exit(1); + } +} + +removeContainer(); + +console.log(`▶ knowledge-base container → http://localhost:${PORT}/knowledge-base/`); +const child = spawn( + 'docker', + ['run', '--rm', '--name', NAME, '-p', `${PORT}:8080`, IMAGE], + { stdio: 'inherit' }, +); + +// Playwright terminates this process on teardown. `docker run --rm` usually +// cleans up on its own, but a hard kill can orphan the container, so the removal +// is repeated here rather than relied upon. +const shutdown = () => { removeContainer(); process.exit(0); }; +process.on('SIGTERM', shutdown); +process.on('SIGINT', shutdown); +process.on('exit', removeContainer); + +child.on('exit', (code) => { removeContainer(); process.exit(code ?? 0); }); diff --git a/tests/fragment-server.mjs b/tests/fragment-server.mjs index 0c6ec6f..47750f8 100644 --- a/tests/fragment-server.mjs +++ b/tests/fragment-server.mjs @@ -60,15 +60,40 @@ app.use((req, _res, next) => { next(); }); +// nginx: location = /knowledge-base { rewrite ^ /knowledge-base/ last; } +// An INTERNAL rewrite, not a redirect — nginx.conf is explicit that a 3xx here +// would expose the container's internal HTTP address to the browser and cause a +// mixed-content error when the host app is served over HTTPS. This mirror used +// to answer with a 308, i.e. the exact thing that comment forbids (#60). +// Must run before the static mount so the rewritten path is what it sees. +app.use((req, _res, next) => { + if (req.url === `/${PREFIX}`) req.url = `/${PREFIX}/`; + next(); +}); + // nginx: location ^~ /knowledge-base/ { rewrite strips prefix; try_files $uri $uri/index.html } // express.static strips the mount path and resolves files from dist root. +// +// redirect:false is essential. serve-static answers a directory path that lacks +// a trailing slash with a 301, whereas nginx's try_files just serves +// {path}/index.html with a 200 — verified against the image: /knowledge-base, +// /knowledge-base/user-guide and their trailing-slash forms are all 200, no +// Location header anywhere. A mirror that redirects is testing a contract the +// production server does not have. app.use( `/${PREFIX}`, - express.static(DIST, { extensions: ['html'], index: 'index.html' }), + express.static(DIST, { extensions: ['html'], index: 'index.html', redirect: false }), ); -// nginx: location = /knowledge-base { rewrite ^ /knowledge-base/ } -app.get(`/${PREFIX}`, (_req, res) => res.redirect(308, `/${PREFIX}/`)); +// nginx: the `$uri/index.html` half of try_files — a directory path resolves to +// its index without a redirect, trailing slash or not. +app.use(`/${PREFIX}`, (req, res, next) => { + const candidate = join(DIST, decodeURIComponent(req.path), 'index.html'); + // Never serve outside dist/, whatever the request path claims. + if (!candidate.startsWith(DIST)) return next(); + if (!existsSync(candidate)) return next(); + res.sendFile(candidate); +}); app.get('/healthz', (_req, res) => res.type('text/plain').send('ok')); diff --git a/tests/standalone.spec.js b/tests/standalone.spec.js index 118a0e8..5907df4 100644 --- a/tests/standalone.spec.js +++ b/tests/standalone.spec.js @@ -83,6 +83,26 @@ test.describe('HTTP headers', () => { const res = await request.get('/knowledge-base/'); expect(res.headers()['x-xss-protection']).toBeUndefined(); }); + + // nginx serves /knowledge-base (no trailing slash) with an internal rewrite, + // never a redirect: a 3xx would expose the container's internal HTTP address + // and break mixed-content under HTTPS. This mirror answered with a 308 — the + // opposite of production (#60) — and nothing asserted it either way. + // tests/container.spec.js makes the same assertion against real nginx. + test('serves /knowledge-base without a trailing slash by internal rewrite, not redirect', async ({ request }) => { + const res = await request.get('/knowledge-base', { maxRedirects: 0 }); + expect(res.status()).toBe(200); + expect(res.headers()['location']).toBeUndefined(); + }); + + // nginx's try_files serves a directory path as its index with a 200 whether or + // not it ends in a slash. express.static answers 301 by default, which is the + // wider half of the same drift. + test('serves a sub-app page without a trailing slash, not a redirect', async ({ request }) => { + const res = await request.get('/knowledge-base/user-guide', { maxRedirects: 0 }); + expect(res.status()).toBe(200); + expect(res.headers()['location']).toBeUndefined(); + }); }); // ─────────────────────────────────────────────────────────────────────────────