fix(frontend): scope nginx root to server + add cache-control headers

[aatchison]

Scope: this does NOT affect the ECS + CloudFront + S3 deployment

Production and staging are untouched by this PR. They do not use nginx at all — the SPA is served from S3, not from a container. Traced end to end:

• frontend/docker/etc/nginx/conf.d/appointments.conf has exactly one consumer: frontend/deploy.dockerfile:28.
• frontend/deploy.dockerfile has exactly one builder: .github/workflows/publish-images.yml:69, which publishes the multi-arch GHCR frontend image — the EKS path only.
• deploy-production.yml:41-42 deploys the frontend with aws s3 sync ./dist "s3://$FRONTEND_BUCKET" + create-invalidation. No image, no nginx.
• deploy-staging.yml:46-47 does the same: aws s3 sync frontend/dist s3://$FRONTEND_BUCKET.
• The only container build in either deploy workflow is the backend (docker build ./backend -f ./backend/Dockerfile), a different Dockerfile this PR does not touch.
• The other Dockerfile, frontend/Dockerfile, is the local-dev vite server and never reads the nginx config.

The conf is also not an input to vite build, so the bundle is byte-for-byte unaffected — this is a COPY into the nginx image and nothing else.

The other two files are equally inert for prod: frontend/docker/test-nginx-config.sh runs only in CI, and the validate.yml addition is a new job in the PR-validation workflow — it does not touch deploy-production.yml or deploy-staging.yml.

This is deliberate: the ECS/CloudFront/S3 pipeline stays untouched until the EKS cutover (thunderbird/platform-infrastructure#707). To be precise about what that means: this change is active on the EKS tb-dev deployment — that is its purpose — and inert everywhere else.

Why

root was declared inside location /, which meant it never reached sibling locations. Every new location in this change that serves from disk — /assets/, /index.html, /config.js, /site.webmanifest — would have inherited nginx's compiled-in default root (/etc/nginx/html, which does not exist in the nginx:stable image) and 404'd everything it serves, while nginx -t still reports success. root is hoisted to server scope, with a comment recording why it must stay there. Alongside it: real Cache-Control headers per content class, a JSON access log on both vhosts, X-Forwarded-For client resolution, and a CI job that probes actual requests.

To be precise about the blast radius rather than overselling it: a total root mis-scope fails closed. Reproduced in a container — with root back inside location /, /, /index.html, /config.js, /user/foo and /assets/* all 404 (open() "/etc/nginx/html/index.html" failed), because the try_files fallback re-runs location matching into location = /index.html, which has no root. So the rollout's readiness probe on / never goes green and analysis-frontend-synthetic fails its very first probe.

What neither in-cluster gate can see is breakage confined to /assets/, or a wrong or leaked Cache-Control value on a path that still returns 200. Those promote green — and that, not the total mis-scope, is the real reason the behavioural test below exists. (The upstream issue has been corrected to match; this PR now uses the same framing.)

Evidence — probe matrix against the real image

podman build -f frontend/deploy.dockerfile (exit 0), nginx 1.30.4, real dist/:

#	Probe	Status	cache-control	content-type	Bytes	Result
1	/assets/index-D5xKgVJQ.js	200	public, max-age=300	application/javascript	999401	PASS
1b	/assets/AvailabilityView-Dzsaous6.css	200	public, max-age=300	text/css	7744	PASS
2	/assets/missing-000.js	404	(none)	text/html	153	PASS
3	/assets/anything.js.map	404	(none)	text/html	153	PASS
3b	/assets/AvailabilityView-vM6NC5iH.js.map (on disk)	404	(none)	text/html	153	PASS
4	/	200	no-store	text/html	1644	PASS
5	/index.html	200	no-store	text/html	1644	PASS
6	/user/foo	200	no-store	text/html	1644	PASS
6b	deep route body is the SPA shell	—	—	—	—	PASS
7	/config.js	200	no-store	application/javascript	375	PASS
8	/favicon.svg	200	public, max-age=300	image/svg+xml	1651	PASS
9	/site.webmanifest	200	public, max-age=300	application/manifest+json	350	PASS
9b	/sitemap.txt (MIME map not clobbered)	200	public, max-age=300	text/plain	25	PASS
10	Accept-Encoding: gzip, deflate, br sent on every probe	200	public, max-age=300	application/javascript	999401	PASS
10b	no content-encoding on any probed path, and bundle keeps Content-Length	—	—	—	—	PASS
11	nginx -t	—	—	—	—	PASS
11b	nginx -T warn/emerg/duplicate lines = 0	—	—	—	—	PASS
12	shortlink vhost Host: stage.apt.mt redirect	302	—	—	145	PASS
13	/api/v1/health → error_page (unchanged)	502	(none)	—	59	PASS
14	non-JSON access log lines (of 27) = 0	—	—	—	—	PASS
15	jq-invalid access log lines = 0	—	—	—	—	PASS
16	cf_pop field gone from log	—	—	—	—	PASS
17	realip: "remote_addr":"203.0.113.9" from XFF	—	—	—	—	PASS
18	realip fires on shortlink vhost too (198.51.100.7)	—	—	—	—	PASS

24/24 pass — that is 24 data rows in the table above (an earlier revision of this body said "25/25", which was simply an arithmetic error). Separately, the CI script frontend/docker/test-nginx-config.sh asserts 22 checks and additionally hard-fails on nginx -t; the two counts are not the same thing, because the matrix above is a manual run against the real dist/ while the script runs against a synthetic tree.

Probes 14/17/18 genuinely fail on origin/main — its access_log is commented out, so lines come out in the inherited combined format (10.0.0.74 - - [...] "GET /probe14 HTTP/1.1" 200), and with no set_real_ip_from the remote_addr stays the peer's 10.0.0.74.

Probe 12 is not a regression check — it is a no-regression check. An earlier revision of this body listed it alongside 14/17/18 as failing pre-edit. That was wrong: the shortlink rewrite is untouched by this PR, and the response is byte-identical on origin/main — 302, Location: https://appointment-stage.tb.pro/user/abc, Content-Length: 145. It is in the matrix to prove the second vhost still behaves exactly as it did, which is worth asserting given a log_format and three real_ip directives were added at http scope where they reach both server blocks.

Re-nesting root inside location / (the original blocker) still gives nginx: syntax is ok but 9 probe FAILs, including /assets/* 404 and the deep route not serving the shell.

New CI job

validate-frontend-nginx runs frontend/docker/test-nginx-config.sh under the existing frontend/** path filter: the shipped conf in a stock nginx:stable over a synthetic dist/ tree. No npm build, no registry auth, no AWS creds, and no interpolation of untrusted ${{ github.event.* }}.

Two of its assertions were originally able to pass vacuously and have been hardened. Both hardenings were mutation-verified — confirmed failing against each mutation and still passing against the real conf:

Compression probe. It offered Accept-Encoding at one call site and checked a 23-byte fixture, so it missed every realistic way of turning origin gzip on. Accept-Encoding: gzip, deflate, br is now folded into the shared probe() helper so every probe asserts no Content-Encoding; the SPA-shell fixture is padded past 1000 bytes and the bundle fixture is ~64 KB (both sizes are load-bearing against a gzip_min_length); and the bundle response must still carry a Content-Length, since it is the loss of that header under nginx's chunked gzip — not the value of Content-Encoding — that lets CloudFront cache a partial object (#814).

JSON access-log check. Its bad counter only incremented while reading a grep-filtered stream, so zero matched lines read as PASS. It now counts matched lines and requires > 0, selects log lines by excluding entrypoint/[notice] output rather than include-matching ^{ (so a non-JSON format is counted as bad instead of vanishing), asserts via jq -e that a captured line has x_forwarded_for, cf_id, request_time, upstream_response_time and user_agent, and probes each vhost with X-Forwarded-For: 203.0.113.9 to assert the logged remote_addr resolves to it. The local jq-missing SKIP branch is kept but now hard-fails when CI is set, so a silently-skipped assertion cannot recur.

Deliberate choices

gzip is intentionally not enabled at the origin. Compression belongs at CloudFront: nginx's gzip rewrites the body after Content-Length is set, which breaks range/partial-object requests and makes byte-level cache validation inconsistent — the edge does Brotli better and closer to the viewer. Probe 10b asserts no Content-Encoding comes back on any path even when the client offers gzip, deflate, br, and that the bundle keeps its Content-Length.

The /assets/ TTL is deliberately max-age=300 for now. Vite bundles are content-hashed, so immutable, max-age=31536000 is correct in principle and is the follow-up — but only after curl parity is captured from the running pod, because immutable lands in browser caches, where a wrong value cannot be invalidated by any CloudFront invalidation and is unrecallable for a year.

no-store (not no-cache) on the SPA shell is deliberate: it is the only directive that also keeps the deploy pointer out of shared/CDN storage, and the 304 revalidation it forfeits is on a ~1.6 KB document.

Read $remote_addr, never the leftmost X-Forwarded-For element. The log-format comment previously said the opposite. real_ip has already resolved $remote_addr to the address the ALB observed; the leftmost XFF entry is whatever the viewer sent and is therefore the one hop an attacker fully controls. Verified from a 10/8 peer: X-Forwarded-For: 1.2.3.4, 203.0.113.9 logs remote_addr=203.0.113.9 while 1.2.3.4 is the forgery. Once CloudFront fronts this origin the trustworthy element is the rightmost-but-one — the hop CloudFront appended — or forward CloudFront-Viewer-Address.

Deferred, tracked not lost

CloudFront origin-facing ranges in set_real_ip_from, no-store on /api/v1/ and /fxa, and attaching AllViewerExceptHostHeader to the default + /assets/* behaviours all belong with the CDN work that owns the cache policies, not this file. One genuine pre-existing issue found and left alone as out of scope: the proxy blocks pass $proxy_add_x_forwarded_for, which lets a client seed the chain and bypass IP rate limiting — filing separately.

Part of thunderbird/platform-infrastructure#814

• https://github.com/thunderbird/platform-infrastructure/issues/814
• https://github.com/thunderbird/platform-infrastructure/issues/702

🤖 Generated with Claude Code

[aatchison]

Pushed a review-fix pass. Five issues, all in comments / test-script assertions / PR-and-commit text — no change to served nginx behaviour. Force-pushed as bc8e82ee (amend of 03a6e147).

What changed

1. Inverted XFF trust guidance corrected (frontend/docker/etc/nginx/conf.d/appointments.conf, comment only). The block said to read the leftmost x_forwarded_for element rather than remote_addr. That is backwards — the leftmost entry is entirely viewer-supplied and is the one hop an attacker fully controls. Verified from a 10/8 peer:

XFF: 1.2.3.4, 203.0.113.9  ->  {"remote_addr":"203.0.113.9","x_forwarded_for":"1.2.3.4, 203.0.113.9"}

remote_addr is the truthful, proxy-observed value; 1.2.3.4 is the forgery. The comment now says: read $remote_addr today; once CloudFront fronts this origin read the rightmost-but-one XFF element (the hop CloudFront appended) or forward CloudFront-Viewer-Address; everything to the left of the last proxy-appended hop must never be used for attribution, rate limiting, or authorization. The X-Cache / X-Amz-Cf-Id / X-Amz-Cf-Pop and "Amazon CloudFront" user-agent notes are unchanged — they were already correct.

2. A false rationale, repeated in four places, replaced with the true narrower one. The claim that "the synthetic analysis only probes / and /config.js, both of which stay 200 in those broken states" is false. Reproduced: with root re-nested inside location /,

/  404   /index.html  404   /config.js  404   /user/foo  404   /assets/index-abc123.js  404
nginx: configuration file /etc/nginx/nginx.conf test is successful
open() "/etc/nginx/html/index.html" failed

(/ 404s because the try_files fallback re-runs location matching into location = /index.html, which has no root.) So the readiness probe on / never passes and analysis-frontend-synthetic fails its first probe — a total root mis-scope fails closed. Corrected in the test-script header comment, the validate-frontend-nginx job comment, the PR body, and the commit message, to the true justification: what neither in-cluster gate can see is breakage confined to /assets/ or a wrong/leaked Cache-Control value on a path that still returns 200. Wording matches the upstream correction in thunderbird/platform-infrastructure#814 rather than inventing a third version.

3. Compression probe hardened — Accept-Encoding: gzip, deflate, br folded into the shared probe() helper so every probe asserts no Content-Encoding; SPA-shell fixture padded past 1000 bytes; bundle fixture grown to ~64 KB; and a new assertion that the bundle response still carries a Content-Length.

4. JSON access-log check hardened — matched lines are now counted and required to be > 0; log lines are selected by excluding entrypoint/[notice] output instead of include-matching ^{; jq -e asserts field presence; and each vhost is probed with a known X-Forwarded-For to assert the logged remote_addr resolves to it.

5. PR body arithmetic and one false regression claim. "25/25" over a 24-row table is now 24/24, with the CI script's 22 assertions stated separately. Probe 12 (Host: stage.apt.mt → 302) was listed as failing pre-edit; it does not. It is byte-identical on origin/main (302, Location: https://appointment-stage.tb.pro/user/abc, Content-Length: 145) because that rewrite is untouched — restated as a no-regression check on the unmodified vhost. Probes 14/17/18 are genuine and kept (origin/main has access_log commented out and no set_real_ip_from, so it logs 10.0.0.74 - - [...] "GET /probe14 HTTP/1.1" 200 in combined format).

Mutation verification

Both hardenings were verified by mutation, not assumed. Every mutation below was run against the previous script (all passed, exit 0 — the bug) and then against the hardened script (all fail, exit 1).

FIX 3 — compression. nginx's default gzip_types is text/html alone, so a .js asset is never compressed by a bare gzip on; at any size. That makes the shell fixture the thing that catches the common mutations and the bundle fixture the thing that catches a gzip_types list — both sizes are load-bearing, which is why the fixture comment now says so explicitly.

Mutation	Old script	Hardened script
gzip on;	PASS (exit 0)	FAIL — 6 probes, content-encoding='gzip' on /, /index.html, deep route + 3 404s
gzip on; gzip_min_length 1000;	PASS (exit 0)	FAIL — 3 probes, content-encoding='gzip' on /, /index.html, deep route
gzip on; gzip_types application/javascript text/css; gzip_min_length 1000;	(not previously probed)	FAIL — 5 probes, incl. hashed JS -> content-encoding='gzip' and bundle has NO Content-Length -- response is chunked, CloudFront may cache a partial object

Why the fixture sizes matter, measured on gzip on; gzip_min_length 1000;:

old fixtures (83 B shell, 23 B bundle):   /  Content-Length: 83     /assets/*.js  Content-Length: 23     <- nothing compressed, vacuous PASS
new fixtures (2244 B shell, 64 KB bundle): /  Transfer-Encoding: chunked  Content-Encoding: gzip        <- caught

FIX 4 — access log.

Mutation	Old script	Hardened script
access_log off; on both vhosts	PASS (exit 0)	FAIL ×4 — NO access log lines captured at all, + no line for either vhost probe
delete x_forwarded_for + cf_id from log_format	PASS (exit 0)	FAIL ×2 — log line is missing a required field on both vhosts
delete set_real_ip_from / real_ip_header / real_ip_recursive	PASS (exit 0)	FAIL ×2 — remote_addr='10.0.0.74', want '203.0.113.9' on both vhosts
swap JSON log_format for logfmt	PASS (exit 0)	FAIL ×4 — 19 of 19 access log line(s) are not JSON + no JSON line for either vhost probe

Sanity checks that did not regress: the real config still passes (22/22 assertions, exit 0), and re-nesting root inside location / still yields nginx: syntax is ok with 9 probe FAILs.

One incidental fix found by running the hardened script: yes 'console.log(1);' | head -4000 cannot be used to build the 64 KB fixture, because under this script's set -o pipefail the yes SIGPIPE (exit 141) aborts the whole run before a single probe executes. Both fixtures are generated with awk instead.

No behaviour change — proof

Status plus every response header was captured for 17 request variants (/, /index.html, /config.js, hashed .js, hashed .css, missing asset, .js.map on disk and absent, /favicon.svg, /site.webmanifest, /sitemap.txt, deep SPA route, Host: stage.apt.mt, Host: stage.apmt.local, the bundle with Accept-Encoding, /api/v1/health, /50x.html) before and after, against a fixture tree held identical across both runs so there is no fixture-size confound. Only Date/ETag/Last-Modified values were normalized; header names were kept.

diff before.txt after.txt  ->  IDENTICAL - no behaviour change   (195 lines each, empty diff)

Independently confirmed at the source level: every change to appointments.conf in this pass is a comment line — filtering comment lines out of git diff on that file leaves zero non-comment hunks.

[aatchison]

20-lens review complete: 11 findings survived refutation, 0 blockers — all now fixed

Twenty independent lenses, each finding then put through an adversarial refutation pass. Every survivor was in a comment, the PR text, or test coverage — the nginx config itself needed no change, and that is now proven rather than asserted: 17 request variants (status + every response header, identical fixture tree both runs) diff byte-identical, and filtering comments out of git diff on appointments.conf leaves zero non-comment hunks.

The correction that matters most

This PR previously claimed — following the issue — that a mis-scoped root would leave the pod Ready and the ALB health check on / passing, so a white SPA would promote with every signal green. That is false. Reproduced: with root re-nested, /, /index.html, /config.js, /user/foo and /assets/* all 404 (open() "/etc/nginx/html/index.html" failed) — / 404s because the try_files fallback re-enters location = /index.html, which has no root. The readiness probe never passes and the synthetic analysis fails its first probe: a total mis-scope fails closed.

The real justification for this script is narrower and does hold: neither in-cluster gate can see breakage confined to /assets/, or a wrong/leaked Cache-Control value on a path that still returns 200. Corrected in all four places it had propagated to, and upstream in the issue.

Two probes could false-pass — both mutation-verified before and after

Mutation	Old script	Hardened
gzip on;	PASS	FAIL x6
gzip on; gzip_min_length 1000;	PASS	FAIL x3
+ gzip_types application/javascript	not probed	FAIL x5, incl. bundle has NO Content-Length
access_log off; on both vhosts	PASS	FAIL x4
drop x_forwarded_for + cf_id	PASS	FAIL x2
drop the three real_ip lines	PASS	FAIL x2
JSON -> logfmt	PASS	FAIL x4

The access-log check passed on zero lines (bad started at 0 and was only incremented over a grep-filtered stream). It now counts matches and requires > 0, selects lines by excluding entrypoint output rather than include-matching ^{ — so a non-JSON format is counted bad instead of vanishing — asserts field presence with jq -e, and asserts a per-vhost X-Forwarded-For lands in the logged remote_addr.

A nuance worth recording: nginx's default gzip_types is text/html alone, so the JS bundle is not compressed at any size until gzip_types adds the JS MIME type. What catches the two named mutations is the shell fixture exceeding gzip_min_length — so both fixtures were enlarged. With the original 83 B / 23 B pair, gzip_min_length 1000 hid compression on every path.

Also fixed

• Inverted XFF guidance. The comment said read the leftmost X-Forwarded-For element — the viewer-supplied, forgeable one. Reproduced from a 10/8 peer: X-Forwarded-For: 1.2.3.4, 203.0.113.9 logs remote_addr=203.0.113.9 (proxy-observed, truthful) while 1.2.3.4 is the forgery. Now: read $remote_addr today; under CloudFront read the rightmost-but-one element or forward CloudFront-Viewer-Address; never attribute, rate-limit or authorize on anything left of the last proxy-appended hop.
• PR body arithmetic. "25/25" over a 24-row table -> 24/24, with the CI script's 22 assertions stated separately. Probe 12 (Host: stage.apt.mt -> 302) was credited as catching a regression but is byte-identical on origin/main (same Location, Content-Length: 145) — restated as a no-regression check on the untouched vhost. 14/17/18 are genuine.

Deliberately unchanged

/assets/ stays at max-age=300; raising it to immutable, max-age=31536000 is a follow-up per #814, because that header lands in browser caches where no invalidation, DNS change or revert can reach it. Also unchanged: no gzip at the origin (compression belongs at the edge, where Brotli wins and Content-Length survives), and no always on the /assets/ header so a =404 cannot be cached for the full TTL.

CI green on bc8e82e, all 9 checks — validate-frontend-nginx reports 22 PASS / 0 FAIL / 0 SKIP, so jq was present and the new assertions genuinely executed rather than being skipped.

[ryanjjung]

Approving, though the test config is pretty complicated and I'm not sure I followed every line of it.