diff --git a/CLAUDE.md b/CLAUDE.md index 4124753..a712617 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -341,16 +341,24 @@ Dockerfile declares `ARG PUBLIC_ORIGIN` to receive it. Changing it needs a it prompts, and then leaves `@angular/cli` and `typescript` in place anyway. A larger image beats a step that silently half-works. -**`NG_ALLOWED_HOSTS` is load-bearing, and it fails silently.** Angular 21 checks -the `Host` header against an allowlist (SSRF protection). Off the list it does not -error — it falls back to **client-side rendering**, quietly discarding the SSR and -structured-data work in PRD §8.5. The env var *replaces* the build-time list in -`angular.json` (`getAllowedHostsFromEnv() ?? options.allowedHosts`), so a deploy -must list every hostname it answers on. The port is stripped before matching, and -`*.example.com` wildcards match by suffix (`isHostAllowed` turns `*.x` into -`hostname.endsWith('.x')`), so one wildcard covers a multi-label host. Verify -with: the HTML for `/` contains `ng-server-context`. `angular.json` carries -`localhost` so `node server.mjs` renders locally. +**Two different things switch SSR off, and they look nothing alike.** + +**`NG_ALLOWED_HOSTS` — a hard 400.** Angular checks `Host` and +`X-Forwarded-Host` against an allowlist (SSRF). With one configured a miss is +**400 `text/plain`**; only an *empty* list falls back to CSR. The env var is a +comma list and is **unioned with** `angular.json`'s `security.allowedHosts`, not +a replacement. `*.example.com` matches by suffix. Never set it to `*`. + +**Untrusted proxy headers — this is the silent one.** Angular deopts to CSR on +any `x-forwarded-*` header it was not told to trust, returning a normal 200 that +was never server-rendered. Its default covers only host and proto; Render also +sends `x-forwarded-for`, so the deploy rendered client-side for every visitor +while every test passed. `frontend/src/server.ts` trusts the full set, reading +`NG_TRUST_PROXY_HEADERS` first so a deploy can narrow it. + +Verify with `pnpm run verify:deploy `, which tells the two apart. Reproduce +the second locally: +`curl -s -H 'X-Forwarded-For: 203.0.113.9' localhost:8080/ | grep ng-server-context` **`PUBLIC_ORIGIN` is a BUILD-time variable, not a runtime one** — a `Dockerfile` `ARG`. On Render it is declared as a normal env var only because Render turns @@ -361,22 +369,17 @@ the build finishes. `index.html`, `sitemap.xml` and `robots.txt` carry a file, and `scripts/stamp-seo.mjs` replaces it across **the whole `dist/frontend` tree** as the last step of `pnpm run build`. -**It must not narrow to `browser/` again.** It did, and the deployed site served -a literal `__PUBLIC_ORIGIN__` in its `canonical` and `og:image`: Angular keeps -its own copies of the page HTML under `server/` — `index.server.html` and the -`assets-chunks/*.mjs` templates, `index_csr_html.mjs` among them — and those are -what the SSR handler serves. `sitemap.xml` and `robots.txt` looked right the -whole time because they come from `browser/`, which is what made it hard to see. -`.mjs` is in the stampable extension set for exactly this reason. +**It must not narrow to `browser/` again.** It did, and the deploy served a +literal `__PUBLIC_ORIGIN__`: Angular keeps its own page HTML under `server/`, and +that is what the SSR handler serves. `sitemap.xml` looked right throughout, which +is what hid it. `.mjs` is stampable for that reason. Unset, `PUBLIC_ORIGIN` substitutes `''` and everything stays root-relative and valid. It must carry the scheme: the value is substituted verbatim, so a bare hostname yields a `` that is not a URL. -**`pnpm run verify:deploy `** checks the deployed result of all of this — -`ng-server-context` present, no sentinel surviving, and the converter configured -on another origin. Local green does not mean deployed correct: SSR fell back to -CSR in production while every test passed and the page looked fine. +**`pnpm run verify:deploy `** checks the deployed result. Local green does +not mean deployed correct. **The service worker must never cache `/api`.** `ngsw-config.json` has no `dataGroups` at all, deliberately: a cached response would show stale money and diff --git a/Progress.md b/Progress.md index f373d4d..f0833bb 100644 --- a/Progress.md +++ b/Progress.md @@ -125,7 +125,7 @@ alive. Always verify after — `pkill`'s exit status is not proof. | Per-category budgets | 1 | ✅ | `budgets.service.ts:status` unions budgeted and spent categories | | Per-team budgets | 2 | ⬜ | No team entity exists | | Threshold alerts (80%) | 1 | ⬜ | Utilization is computed and shown; no threshold, no alert, no notification | -| Rollover vs reset | 1 | 🟡 | `rollover` column is persisted but **read by nothing** — `status()` always computes a fresh calendar month, so the flag has no effect | +| Rollover vs reset | 1 | ⬜ | Column and DTO field exist, read by nothing — `status()` always computes a fresh calendar month. The Budgets form **no longer offers the checkbox**; `budgets.spec.ts` guards against it returning without the behaviour | | Budget creation UI | 1 | ✅ | A form on the Budgets page for owner/admin. `POST /budgets` inserts and a unique index makes a repeat a 409, so only categories without a budget are offered — **changing** an existing budget is still unsupported (no PATCH route) | **Verify:** with a category over budget, confirm the bar turns danger-toned and @@ -190,7 +190,7 @@ not a naive sum, and that it says how many rows it left out. | Month-over-month deltas | 1 | 🟡 | Computed for the pace benchmark; no delta tile | | Team vs individual | 2 | ⬜ | — | | CSV export | 0 | ✅ | Chunked, cancellable, complete across pages | -| **PDF export** | 2 | 🟡 | `format: 'pdf'` is **accepted and silently returns CSV**. Either implement it or reject the value | +| **PDF export** | 2 | ⬜ | `pdf` is now **rejected** rather than silently answered with CSV. `shared/src/report-format-contract.spec.ts` pins the tool schema and the backend DTO to the same list | | `/api/analytics/*` | 1 | ⬜ | No controller; the dashboard derives everything client-side | ## §6.7 Notifications — ⬜ @@ -267,7 +267,7 @@ and the offline banner appearing and clearing on the network events. | Structured data | ✅ | Real `application/ld+json` `SoftwareApplication` | | llms.txt | ✅ | Accurate tool inventory and permission model | | OG / Twitter | ✅ | 1200×630 `og.png` generated from the brand tokens, plus `og:url`, `og:image:alt`, `twitter:image` and a canonical link | -| SSR on public pages | 🟡 ⚠️ | `app.routes.server.ts` prerenders `**` — including authenticated routes, which land on the app shell and hydrate client-side (correct for a gated view, accidental rather than chosen). **Broken on the deployed site as of 2026-09-03:** `/` carries no `ng-server-context`, so Angular is falling back to CSR there and discarding the SSR entirely. It works locally, which is exactly how it went unnoticed — the same silent failure as 2026-08-29. `NG_ALLOWED_HOSTS` must be set on the *service*, not only in `render.yaml` and the Dockerfile. `pnpm run verify:deploy ` now checks it | +| SSR on public pages | 🟡 | `app.routes.server.ts` prerenders `**`, including authenticated routes, which hydrate client-side (correct for a gated view, accidental rather than chosen). **Was broken on the deploy until 2026-09-03, and not for the documented reason:** Angular downgrades to CSR on any untrusted `x-forwarded-*` header, and Render sends `x-forwarded-for`; a host-allowlist miss is a 400 instead. `frontend/src/server.ts` now trusts the full proxy set | | noindex on gated views | ✅ | `data.robots` per route, applied by `SeoService` on every navigation; a route that declares nothing defaults to `noindex`. Verified live: the tag flips going from `/` to `/expenses` | ## §9 Non-functional diff --git a/README.md b/README.md index 811e6a7..41fc1fe 100644 --- a/README.md +++ b/README.md @@ -260,17 +260,26 @@ grep -o '[^<]*' frontend/dist/frontend/browser/sitemap.xml ### Allowed hosts — the one that fails silently -Angular 21 refuses to server-render a request whose `Host` header is not on an -allowlist (SSRF protection). Off the list it does **not** error: it quietly falls -back to client-side rendering, which throws away the SSR and structured-data work -on the public pages. `NG_ALLOWED_HOSTS` in `render.yaml` is that list, and it -*replaces* the build-time list in `angular.json` rather than adding to it. +Two different things switch SSR off behind a deploy, and they look nothing alike. + +`NG_ALLOWED_HOSTS` is the host allowlist (SSRF protection). With it configured, a +hostname that is not on the list gets a **400**, not a silent downgrade — and the +value is *unioned with* the build-time list in `angular.json`, not a replacement. + +The one that fails silently is **proxy headers**. Angular downgrades to +client-side rendering, with a perfectly normal 200, whenever it receives an +`x-forwarded-*` header it was not told to trust — its default covers only +`x-forwarded-host` and `x-forwarded-proto`, while most platform proxies also send +`x-forwarded-for`. `frontend/src/server.ts` trusts the full set, overridable with +`NG_TRUST_PROXY_HEADERS`. After any deploy, confirm the HTML for `/` contains `ng-server-context` — which -is what `pnpm run verify:deploy` does. If it does not, `NG_ALLOWED_HOSTS` is not -reaching the running container. Declaring it in `render.yaml` is not sufficient -on its own: a service created by hand rather than from the Blueprint never had -those `envVars` applied, so check the service's own environment first. +is what `pnpm run verify:deploy` does, and it names which of the two causes it +is. You can reproduce the proxy-header case locally without deploying: + +```bash +curl -s -H 'X-Forwarded-For: 203.0.113.9' localhost:8080/ | grep ng-server-context +``` ### The cross-origin demo on a deployed URL diff --git a/backend/src/reports/dto/report.dto.ts b/backend/src/reports/dto/report.dto.ts index 82f3e84..109760f 100644 --- a/backend/src/reports/dto/report.dto.ts +++ b/backend/src/reports/dto/report.dto.ts @@ -7,7 +7,8 @@ export class GenerateReportDto { @IsISO8601({ strict: true }, { message: 'to must be a date in YYYY-MM-DD form' }) to!: string; + /** CSV only — `pdf` was accepted here and then ignored by the service. */ @IsOptional() - @IsIn(['csv', 'pdf']) - format?: 'csv' | 'pdf'; + @IsIn(['csv'], { message: 'format must be csv — PDF export is not implemented' }) + format?: 'csv'; } diff --git a/backend/src/reports/reports.service.ts b/backend/src/reports/reports.service.ts index 3a40336..1deec19 100644 --- a/backend/src/reports/reports.service.ts +++ b/backend/src/reports/reports.service.ts @@ -11,7 +11,7 @@ export interface ReportJob { id: string; orgId: string; status: ReportStatus; - format: 'csv' | 'pdf'; + format: 'csv'; rows?: number; url?: string; content?: string; diff --git a/frontend/src/app/pages/budgets/budgets.spec.ts b/frontend/src/app/pages/budgets/budgets.spec.ts index 4ef040f..3fa7f25 100644 --- a/frontend/src/app/pages/budgets/budgets.spec.ts +++ b/frontend/src/app/pages/budgets/budgets.spec.ts @@ -385,13 +385,23 @@ describe('Budgets — setting one', () => { categoryId: null, amount: 10000, period: 'monthly', - rollover: false, }); // The bars are server-computed, so they have to come back from it. const fetchesAfter = api.get.mock.calls.filter((c) => c[0] === '/budgets/status').length; expect(fetchesAfter).toBeGreaterThan(fetchesBefore); }); + /** A control that changes no figure is a promise. Restore it with the behaviour. */ + it('offers no rollover control while nothing honours the flag', async () => { + await create(); + + const checkboxes = Array.from( + host().querySelectorAll('input[type="checkbox"]'), + ); + expect(checkboxes).toHaveLength(0); + expect(host().textContent).not.toContain('Roll unspent budget'); + }); + it('sends the chosen category rather than the empty org-wide value', async () => { await create(); const select = host().querySelector('select') as HTMLSelectElement; diff --git a/frontend/src/app/pages/budgets/budgets.ts b/frontend/src/app/pages/budgets/budgets.ts index 7597962..30d92cf 100644 --- a/frontend/src/app/pages/budgets/budgets.ts +++ b/frontend/src/app/pages/budgets/budgets.ts @@ -191,16 +191,6 @@ import { - - @if (formMessage(); as message) {

(null); protected readonly formFailed = signal(false); @@ -297,7 +286,8 @@ export class Budgets { categoryId: this.newCategoryId() || null, amount: Math.round(amount * 100) / 100, period: 'monthly', - rollover: this.newRollover(), + // No `rollover`: nothing reads the flag — `status()` always computes a + // fresh calendar month — so the form stopped offering it (PRD §6.3). }); this.formFailed.set(false); this.formMessage.set('Budget saved.'); diff --git a/frontend/src/app/tools/expense-tools.ts b/frontend/src/app/tools/expense-tools.ts index 291d61f..a08c826 100644 --- a/frontend/src/app/tools/expense-tools.ts +++ b/frontend/src/app/tools/expense-tools.ts @@ -103,7 +103,7 @@ export class ExpenseTools { * abort mid-flight actually stops the work rather than merely detaching from * it — the UI has to reflect a real stop, not a cosmetic one. */ - generateReport(): ActuoTool<{ from: string; to: string; format?: 'csv' | 'pdf' }> { + generateReport(): ActuoTool<{ from: string; to: string; format?: 'csv' }> { return { contract: GENERATE_REPORT, execute: async (args, { signal }) => { diff --git a/frontend/src/server.ts b/frontend/src/server.ts index d131f96..65ba53f 100644 --- a/frontend/src/server.ts +++ b/frontend/src/server.ts @@ -9,8 +9,37 @@ import { join } from 'node:path'; const browserDistFolder = join(import.meta.dirname, '../browser'); +/** + * Trusted `x-forwarded-*` headers, or SSR silently downgrades. + * + * Angular deopts to client-side rendering on any untrusted `x-forwarded-*` + * header — a normal-looking 200 that was never server-rendered. Its default + * covers only host and proto; Render also sends `x-forwarded-for`. (A host + * *allowlist* miss is a 400 instead — different failure.) + * + * Safe to trust: only proto and host build the request URL, and host is still + * checked against `allowedHosts`. Keep that list restrictive. + */ +const PROXY_HEADERS = [ + 'x-forwarded-host', + 'x-forwarded-proto', + 'x-forwarded-for', + 'x-forwarded-port', + 'x-forwarded-prefix', +]; + +/** + * Read here, not left to Angular: `AngularNodeAppEngine` resolves + * `options?.trustProxyHeaders ?? getTrustProxyHeadersFromEnv()`, so passing the + * option at all would disable the env var. + */ +const trustProxyHeaders = + process.env['NG_TRUST_PROXY_HEADERS']?.split(',') + .map((header) => header.trim().toLowerCase()) + .filter((header) => header.length > 0) ?? PROXY_HEADERS; + const app = express(); -const angularApp = new AngularNodeAppEngine(); +const angularApp = new AngularNodeAppEngine({ trustProxyHeaders }); /** * **Do not add `/api` routes here.** diff --git a/scripts/stamp-seo.mjs b/scripts/stamp-seo.mjs index 309bd84..6bcb9c1 100644 --- a/scripts/stamp-seo.mjs +++ b/scripts/stamp-seo.mjs @@ -12,15 +12,9 @@ * survives prerendering into every generated HTML file, so one pass covers all * of them rather than each page needing its own handling. * - * **It has to be the whole `dist/frontend` tree, not just `browser/`.** This - * walked only `browser/` once, and the deployed site shipped a literal - * `__PUBLIC_ORIGIN__` in its `canonical` and `og:image`: Angular keeps its own - * copies of the page HTML under `server/` — `index.server.html` and the - * `assets-chunks/*.mjs` templates, `index_csr_html.mjs` among them — and those - * are what the SSR handler serves. `sitemap.xml` and `robots.txt` looked right - * throughout, because they are served from `browser/`, which is exactly what - * made it hard to see. A crawler reading a malformed URL is worse than one - * reading a relative URL, so this must not narrow again. + * **The whole `dist/frontend` tree, not just `browser/`.** Angular keeps its own + * copies of the page HTML under `server/`, and those are what the SSR handler + * serves — walking only `browser/` shipped a literal sentinel in production. * * With PUBLIC_ORIGIN unset it substitutes the empty string, leaving every URL * root-relative and still valid — a local build is never broken by an @@ -35,11 +29,7 @@ import { extname, join, resolve } from 'node:path'; import { fileURLToPath } from 'node:url'; const SENTINEL = '__PUBLIC_ORIGIN__'; -/** - * `.mjs` is here for `server/assets-chunks/*.mjs` — Angular inlines each page's - * HTML into a JS module, sentinel and all. Stamping a string constant inside a - * generated module is safe: the sentinel appears nowhere else in the output. - */ +/** `.mjs` covers `server/assets-chunks/*.mjs`, where Angular inlines page HTML. */ const STAMPABLE = new Set(['.html', '.xml', '.txt', '.webmanifest', '.json', '.mjs']); const ROOT = resolve(fileURLToPath(new URL('..', import.meta.url))); diff --git a/scripts/verify-deploy.mjs b/scripts/verify-deploy.mjs index cf35d61..e73b057 100644 --- a/scripts/verify-deploy.mjs +++ b/scripts/verify-deploy.mjs @@ -1,25 +1,13 @@ /** * Smoke-checks a running deploy. * - * Every check here exists because the thing it checks **failed silently in - * production** and nobody noticed until someone opened the site by hand: + * Every check here failed silently in production at least once: SSR quietly + * downgraded to client-side rendering, and `__PUBLIC_ORIGIN__` shipped + * unstamped. Neither is visible to a unit test or to a glance at the page. * - * - SSR fell back to client-side rendering. Angular does not error when the - * `Host` header is off its allowlist — it quietly renders on the client and - * throws away the SSR and structured-data work. The page still looks fine. - * - `__PUBLIC_ORIGIN__` shipped literally in `canonical` and `og:image`, - * because the SEO stamp only walked `browser/` while the SSR handler serves - * HTML from `server/`. A crawler read a malformed URL, which is worse than a - * relative one. `sitemap.xml` looked correct throughout, which is what made - * it hard to see. - * - * Neither is visible to a unit test, and both are invisible to a casual look at - * the page. So they are checked here, against the real origin, over the network. - * - * node scripts/verify-deploy.mjs https://actuo.example * pnpm run verify:deploy https://actuo.example * - * Exits non-zero on the first failure, naming what to change. + * Exits non-zero on failure, naming what to change. */ const SENTINEL = '__PUBLIC_ORIGIN__'; @@ -31,7 +19,6 @@ if (!base) { process.exit(2); } -/** A deploy that is merely asleep should read as slow, not as broken. */ async function get(path) { const url = `${base}${path}`; const response = await fetch(url, { signal: AbortSignal.timeout(TIMEOUT_MS) }); @@ -62,15 +49,25 @@ let home = ''; try { const { status, body } = await get('/'); home = body; - if (status !== 200) { + /* + * 400 -> host allowlist -> NG_ALLOWED_HOSTS + * 200 without it -> proxy headers -> trustProxyHeaders + */ + if (status === 400) { + fail( + '/ server-rendered', + `status 400 — the host allowlist rejected "${new URL(base).hostname}"`, + 'Add this hostname to NG_ALLOWED_HOSTS on the service (a comma-separated list; "*.example.com" matches by suffix). It is checked at runtime, so a restart is enough.', + ); + } else if (status !== 200) { fail('/', `status ${status}`, 'The Angular handler is not answering.'); } else if (body.includes('ng-server-context')) { pass('/', 'server-rendered (ng-server-context present)'); } else { fail( '/ server-rendered', - 'no ng-server-context — Angular fell back to client-side rendering', - 'Set NG_ALLOWED_HOSTS on the SERVICE (runtime, not just build) to cover this hostname, e.g. "*.onrender.com". Angular does not error when the Host header is off the list; it silently renders on the client.', + 'no ng-server-context — a 200 that was rendered on the client', + 'The proxy is sending an x-forwarded-* header Angular does not trust, so it downgraded to CSR. frontend/src/server.ts passes the full set to AngularNodeAppEngine, so if this fails the deploy predates that fix — redeploy — or NG_TRUST_PROXY_HEADERS is set on the service and is too narrow. The service logs name the exact header: "Received \"x-...\" header but trustProxyHeaders was not set up to allow it".', ); } } catch (error) { diff --git a/shared/src/report-format-contract.spec.ts b/shared/src/report-format-contract.spec.ts new file mode 100644 index 0000000..f34a840 --- /dev/null +++ b/shared/src/report-format-contract.spec.ts @@ -0,0 +1,38 @@ +import { readFileSync } from 'node:fs'; +import { describe, expect, it } from 'vitest'; +import { GENERATE_REPORT } from './tools.js'; + +const ROOT = new URL('../../', import.meta.url).pathname; + +/** + * The report format must mean the same thing at every layer that accepts it. + * + * `pdf` was advertised in the tool schema and validated by the DTO, then + * dropped — the service always builds CSV. Worst in the tool schema: an agent + * offered `pdf` picks it. These pin the two ends together. + */ +describe('report format contract', () => { + const formatSchema = ( + GENERATE_REPORT.inputSchema as { + properties: { format?: { enum?: string[]; default?: string } }; + } + ).properties.format; + + it('offers a model only formats the server can actually produce', () => { + expect(formatSchema?.enum).toEqual(['csv']); + }); + + it('defaults to a value it also allows', () => { + expect(formatSchema?.enum).toContain(formatSchema?.default); + }); + + /** Read from source: `shared` must not depend on `backend`. */ + it('matches the formats the backend DTO will accept', () => { + const dto = readFileSync(`${ROOT}backend/src/reports/dto/report.dto.ts`, 'utf8'); + const match = dto.match(/@IsIn\(\[([^\]]*)\]/); + expect(match, 'no @IsIn on ReportQueryDto.format').not.toBeNull(); + + const accepted = [...match![1].matchAll(/'([^']+)'/g)].map((m) => m[1]); + expect(accepted).toEqual(formatSchema?.enum); + }); +}); diff --git a/shared/src/tools.ts b/shared/src/tools.ts index cb05f32..c6008df 100644 --- a/shared/src/tools.ts +++ b/shared/src/tools.ts @@ -132,7 +132,9 @@ export const GENERATE_REPORT: ActuoToolContract = { properties: { from: { type: 'string', format: 'date' }, to: { type: 'string', format: 'date' }, - format: { type: 'string', enum: ['csv', 'pdf'], default: 'csv' }, + // CSV only. `pdf` was listed here and silently answered with CSV — an + // agent offered it will pick it. Add it back with a renderer, not before. + format: { type: 'string', enum: ['csv'], default: 'csv' }, }, required: ['from', 'to'], additionalProperties: false,