From c30ad1db0782ca54de0022521b1ea33591e1b4fe Mon Sep 17 00:00:00 2001 From: Tudor Date: Sun, 23 Aug 2026 10:54:26 +0100 Subject: [PATCH] feat(flags): serve /api/flags, and keep the public proxy off it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The endpoint and its exposure control ship together on purpose. The moment /api/flags exists, app/api/[...path] forwards it — and the response names every unreleased feature the codebase knows about, along with whether it is on. Publishing that is the opposite of shipping dark. Denied on an exact first-segment match, not a prefix, so /api/flagship does not go down with /api/flags. Next reads the endpoint server-side over the Docker network, which never transits the public proxy. jest.setup.js now guards its browser globals. It runs for every suite, including the one that declares @jest-environment node to exercise the route handler — NextRequest needs Fetch API globals jsdom lacks, and there is no window there to define matchMedia on. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015mWQnpye9F299NVRCCSRvj --- backend/app.py | 14 +++++++++ backend/tests/test_flags.py | 21 +++++++++++++ .../__tests__/api/proxyDenylist.test.ts | 31 +++++++++++++++++++ nextjs-app/app/api/[...path]/route.ts | 19 ++++++++++++ nextjs-app/jest.setup.js | 8 +++++ 5 files changed, 93 insertions(+) create mode 100644 nextjs-app/__tests__/api/proxyDenylist.test.ts diff --git a/backend/app.py b/backend/app.py index 2284ac8..4828a8b 100644 --- a/backend/app.py +++ b/backend/app.py @@ -35,6 +35,7 @@ from .data_loader import ( search_schools_typesense, ) from .data_loader import get_data_info as get_db_info +from . import flags from .places import build_place_registry from .schemas import METRIC_DEFINITIONS, RANKING_COLUMNS, SCHOOL_COLUMNS from .utils import clean_for_json, convert_to_native @@ -459,6 +460,7 @@ def validate_postcode(postcode: Optional[str]) -> Optional[str]: async def lifespan(app: FastAPI): """Application lifespan - startup and shutdown events.""" global _sitemaps + flags.init() print("Loading school data from marts...") df = load_school_data() if df.empty: @@ -1263,6 +1265,18 @@ async def get_place(request: Request, kind: str, slug: str, } +@app.get("/api/flags") +@limiter.limit(f"{settings.rate_limit_per_minute}/minute") +async def get_feature_flags(request: Request): + """Every declared flag and its current value. + + Internal only. The Next proxy denies this path, because the response names + every unreleased feature the codebase knows about — which is exactly what + shipping dark is meant to keep quiet. + """ + return flags.all_flags() + + @app.get("/api/data-info") @limiter.limit(f"{settings.rate_limit_per_minute}/minute") async def get_data_info(request: Request): diff --git a/backend/tests/test_flags.py b/backend/tests/test_flags.py index 5aa0147..114a219 100644 --- a/backend/tests/test_flags.py +++ b/backend/tests/test_flags.py @@ -72,3 +72,24 @@ def test_a_flag_older_than_the_limit_fails_this_test(): "Remove the flag and the code branches it guards, or move its `added` " "date deliberately." ) + + +def _client(): + from fastapi.testclient import TestClient + from backend import app as app_module + return TestClient(app_module.app, raise_server_exceptions=False) + + +def test_the_flags_endpoint_lists_every_declared_flag(monkeypatch): + monkeypatch.setattr(flags, "_client", None) + body = _client().get("/api/flags").json() + assert set(body) == set(flags.REGISTRY) + + +def test_the_flags_endpoint_answers_false_when_unleash_is_unreachable(monkeypatch): + # The endpoint must still answer. A frontend that cannot read flags renders + # everything dark, which is right; one that gets a 500 renders nothing. + monkeypatch.setattr(flags, "_client", None) + res = _client().get("/api/flags") + assert res.status_code == 200 + assert all(v is False for v in res.json().values()) diff --git a/nextjs-app/__tests__/api/proxyDenylist.test.ts b/nextjs-app/__tests__/api/proxyDenylist.test.ts new file mode 100644 index 0000000..10731f2 --- /dev/null +++ b/nextjs-app/__tests__/api/proxyDenylist.test.ts @@ -0,0 +1,31 @@ +/** + * The /api/* proxy is public. Anything it forwards is on the internet. + * + * @jest-environment node + */ +// The docblock above is load-bearing. jest.config.js sets jsdom globally, and +// NextRequest/NextResponse need the Web Fetch API globals that only the node +// environment provides — under jsdom this suite fails on import, not on an +// assertion. +import { NextRequest } from 'next/server'; +import { GET } from '@/app/api/[...path]/route'; + +function request(path: string) { + return new NextRequest(`http://localhost:3000/api/${path}`); +} + +describe('public API proxy', () => { + it('refuses to forward internal-only paths', async () => { + // /api/flags names every unreleased feature and its state. Forwarding it + // publishes the thing shipping dark exists to keep quiet. + const res = await GET(request('flags'), { params: Promise.resolve({ path: ['flags'] }) }); + expect(res.status).toBe(404); + }); + + it('does not deny a path that merely starts with the same letters', async () => { + // A prefix match would take /api/flagship down with /api/flags. + const res = await GET( + request('flagship'), { params: Promise.resolve({ path: ['flagship'] }) }); + expect(res.status).not.toBe(404); + }); +}); diff --git a/nextjs-app/app/api/[...path]/route.ts b/nextjs-app/app/api/[...path]/route.ts index ba7cc85..5f2d368 100644 --- a/nextjs-app/app/api/[...path]/route.ts +++ b/nextjs-app/app/api/[...path]/route.ts @@ -26,8 +26,27 @@ function backendBase(): string { const STRIPPED_RESPONSE_HEADERS = ['content-encoding', 'content-length', 'transfer-encoding', 'connection']; const METHODS_WITH_BODY = new Set(['POST', 'PUT', 'PATCH', 'DELETE']); +/* + * API paths this public proxy must not forward. + * + * Matched on the first segment, exactly — a prefix match would take + * /api/flagship down with /api/flags. + * + * `flags` is here because GET /api/flags names every unreleased feature the + * codebase knows about, along with whether it is on. Publishing that defeats + * the point of shipping dark. Next reads it server-side via FASTAPI_URL, on + * the Docker network, which never transits this route. + * + * Anything else internal-only belongs here too. + */ +const INTERNAL_ONLY_SEGMENTS = new Set(['flags']); + async function handler(req: NextRequest, ctx: { params: Promise<{ path: string[] }> }) { const { path } = await ctx.params; + if (INTERNAL_ONLY_SEGMENTS.has(path[0])) { + return NextResponse.json({ detail: 'Not Found' }, { status: 404 }); + } + const target = `${backendBase()}/${path.join('/')}${req.nextUrl.search}`; const headers = new Headers(req.headers); diff --git a/nextjs-app/jest.setup.js b/nextjs-app/jest.setup.js index 256811b..804aa70 100644 --- a/nextjs-app/jest.setup.js +++ b/nextjs-app/jest.setup.js @@ -12,6 +12,12 @@ jest.mock('next/navigation', () => ({ useSearchParams: () => new URLSearchParams(), })); +// Everything below this line is browser furniture, and this file runs for +// every suite — including the ones that declare `@jest-environment node` to +// test route handlers, where NextRequest needs Fetch API globals jsdom does +// not provide. There is no `window` there, so guard rather than assume one. +if (typeof window !== 'undefined') { + // Mock window.matchMedia Object.defineProperty(window, 'matchMedia', { writable: true, @@ -52,3 +58,5 @@ const localStorageMock = { clear: jest.fn(), }; global.localStorage = localStorageMock; + +} // end: browser-only globals