diff --git a/apps/ssp/src/app/brief/application/brief.store.ts b/apps/ssp/src/app/brief/application/brief.store.ts index 46e9210..88c2a1c 100644 --- a/apps/ssp/src/app/brief/application/brief.store.ts +++ b/apps/ssp/src/app/brief/application/brief.store.ts @@ -233,9 +233,12 @@ export class BriefStore implements PendingSave { /** Reveal the masked case BIG-nummer (PRD-0002 §5c). Server re-checks the capability + step-up and audits the attempt; on success we swap the masked value in the already-loaded caseContext (a field update, not a reload). The step-up gesture - itself is the UI's concern — this command just runs the audited server call. */ + itself is the UI's concern (`behandel-scherm.component.ts`'s `onReveal()` confirm) + — this command is only reachable once that gesture has happened, so it is the one + that tells the adapter to send `X-Step-Up` (BIO-006a: the adapter itself no longer + hardcodes the header). */ async revealBigNummer() { - const r = await this.revealAdapter.reveal(); + const r = await this.revealAdapter.reveal(true); if (!r.ok) { this.actionState.set({ tag: 'Failed', error: r.error }); return; diff --git a/apps/ssp/src/app/brief/infrastructure/letter-preview.adapter.spec.ts b/apps/ssp/src/app/brief/infrastructure/letter-preview.adapter.spec.ts new file mode 100644 index 0000000..7c52ca7 --- /dev/null +++ b/apps/ssp/src/app/brief/infrastructure/letter-preview.adapter.spec.ts @@ -0,0 +1,69 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { errorMessage, PREVIEW_FAILED, LetterPreviewAdapter } from './letter-preview.adapter'; + +// Minimal Response stand-in — errorMessage only calls `.json()`. Avoids stubbing +// globalThis.fetch to reach this trust boundary (TE-002). +const fakeResponse = (body: unknown): Response => + ({ json: () => Promise.resolve(body) }) as unknown as Response; + +describe('errorMessage (TE-002 trust boundary)', () => { + it('surfaces the ProblemDetails detail when present', async () => { + expect(await errorMessage(fakeResponse({ detail: 'Geen toegang.', status: 403 }))).toBe( + 'Geen toegang.', + ); + }); + + it('falls back to PREVIEW_FAILED when the body has no detail', async () => { + expect(await errorMessage(fakeResponse({ status: 500 }))).toBe(PREVIEW_FAILED); + }); + + it('falls back to PREVIEW_FAILED when the body is not JSON', async () => { + const res = { json: () => Promise.reject(new Error('not json')) } as unknown as Response; + expect(await errorMessage(res)).toBe(PREVIEW_FAILED); + }); +}); + +// isDevMode() reads the `ngDevMode` global the Angular CLI defines away in a +// production build. There is no ambient type for it in app code, so this is +// accessed through an untyped bag rather than a `declare const`. +const globals = globalThis as Record; +const originalNgDevMode = globals['ngDevMode']; +const setDevMode = (on: boolean) => { + globals['ngDevMode'] = on; +}; + +describe('LetterPreviewAdapter.preview (BIO-012)', () => { + const okResponse = () => + ({ ok: true, blob: () => Promise.resolve(new Blob()) }) as unknown as Response; + + afterEach(() => { + globals['ngDevMode'] = originalNgDevMode; + vi.unstubAllGlobals(); + history.pushState({}, '', '/'); + sessionStorage.clear(); + }); + + it('sends no X-Role/X-Subject headers outside isDevMode()', async () => { + setDevMode(false); + history.pushState({}, '', '/?subject=111222333'); + const fetchSpy = vi.fn().mockResolvedValue(okResponse()); + vi.stubGlobal('fetch', fetchSpy); + + await new LetterPreviewAdapter().preview(); + + expect(fetchSpy.mock.calls[0][1].headers).toEqual({}); + }); + + it('sends X-Role (and X-Subject when known) under isDevMode()', async () => { + setDevMode(true); + history.pushState({}, '', '/?subject=111222333'); + const fetchSpy = vi.fn().mockResolvedValue(okResponse()); + vi.stubGlobal('fetch', fetchSpy); + + await new LetterPreviewAdapter().preview(); + + const headers = fetchSpy.mock.calls[0][1].headers as Record; + expect(headers['X-Role']).toBeDefined(); + expect(headers['X-Subject']).toBe('111222333'); + }); +}); diff --git a/apps/ssp/src/app/brief/infrastructure/letter-preview.adapter.ts b/apps/ssp/src/app/brief/infrastructure/letter-preview.adapter.ts index 7fc1f31..d777d7d 100644 --- a/apps/ssp/src/app/brief/infrastructure/letter-preview.adapter.ts +++ b/apps/ssp/src/app/brief/infrastructure/letter-preview.adapter.ts @@ -1,4 +1,4 @@ -import { Injectable } from '@angular/core'; +import { Injectable, isDevMode } from '@angular/core'; import { Result, ok, err } from '@shared/kernel/fp'; import { currentRole } from '@shared/infrastructure/role'; import { currentSubject } from '@shared/infrastructure/subject'; @@ -15,7 +15,10 @@ export const PREVIEW_FAILED = $localize`:@@brief.preview.failed:De voorvertoning * hand-written fetch, not the `ApiClient`. That also means it bypasses `HttpClient`'s * `roleInterceptor` AND `subjectInterceptor`, so both `X-Role` and `X-Subject` are set * here explicitly (WP-74 — without `X-Subject` this always previewed - * `DocumentStore.DemoOwner`'s letter regardless of who was actually logged in). + * `DocumentStore.DemoOwner`'s letter regardless of who was actually logged in). Both are + * dev-only identity stand-ins (`role.ts`/`subject.ts`) and are sent only under + * `isDevMode()`, mirroring how the interceptors themselves are only registered in dev + * (`app.config.ts`) — a production build sends neither header from this call (BIO-012). * * `cache: 'no-store'` (WP-74): the endpoint has no `Cache-Control`, only a CORS-driven * `Vary: Origin`, and its content changes at the SAME URL as the letter moves @@ -43,7 +46,9 @@ export class LetterPreviewAdapter { const subject = currentSubject(); res = await fetch(`${environment.apiBaseUrl}/api/v1/brief/preview`, { cache: 'no-store', - headers: { 'X-Role': currentRole(), ...(subject ? { 'X-Subject': subject } : {}) }, + headers: isDevMode() + ? { 'X-Role': currentRole(), ...(subject ? { 'X-Subject': subject } : {}) } + : {}, }); } catch { return err(PREVIEW_FAILED); @@ -53,7 +58,9 @@ export class LetterPreviewAdapter { } } -async function errorMessage(res: Response): Promise { +/** Trust boundary (TE-002): maps a non-OK response to a message. Exported so a spec + can call it directly instead of stubbing `globalThis.fetch`. */ +export async function errorMessage(res: Response): Promise { try { return problemDetail(await res.json(), PREVIEW_FAILED); } catch { diff --git a/apps/ssp/src/app/brief/infrastructure/org-template.adapter.spec.ts b/apps/ssp/src/app/brief/infrastructure/org-template.adapter.spec.ts index ac7d221..b102a90 100644 --- a/apps/ssp/src/app/brief/infrastructure/org-template.adapter.spec.ts +++ b/apps/ssp/src/app/brief/infrastructure/org-template.adapter.spec.ts @@ -1,6 +1,10 @@ import { describe, it, expect } from 'vitest'; import { OrgTemplateAdminViewDto, OrgTemplateDto } from '@shared/infrastructure/api-client'; -import { parseOrgTemplateAdminView } from './org-template.adapter'; +import { + parseOrgTemplateAdminView, + proefbriefErrorMessage, + PROEFBRIEF_FAILED, +} from './org-template.adapter'; const draft: OrgTemplateDto = { subOrgId: 'cibg-registers', @@ -54,3 +58,25 @@ describe('parseOrgTemplateAdminView', () => { expect(r.ok).toBe(false); }); }); + +// Minimal Response stand-in — proefbriefErrorMessage only calls `.json()`. Avoids +// stubbing globalThis.fetch to reach this trust boundary (TE-002). +const fakeResponse = (body: unknown): Response => + ({ json: () => Promise.resolve(body) }) as unknown as Response; + +describe('proefbriefErrorMessage (TE-002 trust boundary)', () => { + it('surfaces the ProblemDetails detail when present', async () => { + expect( + await proefbriefErrorMessage(fakeResponse({ detail: 'Niet gevonden.', status: 404 })), + ).toBe('Niet gevonden.'); + }); + + it('falls back to PROEFBRIEF_FAILED when the body has no detail', async () => { + expect(await proefbriefErrorMessage(fakeResponse({ status: 500 }))).toBe(PROEFBRIEF_FAILED); + }); + + it('falls back to PROEFBRIEF_FAILED when the body is not JSON', async () => { + const res = { json: () => Promise.reject(new Error('not json')) } as unknown as Response; + expect(await proefbriefErrorMessage(res)).toBe(PROEFBRIEF_FAILED); + }); +}); diff --git a/apps/ssp/src/app/brief/infrastructure/org-template.adapter.ts b/apps/ssp/src/app/brief/infrastructure/org-template.adapter.ts index 9477030..fef9891 100644 --- a/apps/ssp/src/app/brief/infrastructure/org-template.adapter.ts +++ b/apps/ssp/src/app/brief/infrastructure/org-template.adapter.ts @@ -1,4 +1,4 @@ -import { Injectable, inject } from '@angular/core'; +import { Injectable, inject, isDevMode } from '@angular/core'; import { Result, ok, err } from '@shared/kernel/fp'; import { runSubmit } from '@shared/application/submit'; import { currentRole } from '@shared/infrastructure/role'; @@ -26,10 +26,13 @@ import { parseOrgTemplate } from '@brief/infrastructure/brief.adapter'; * rollback go through the generated client (X-Role added by `roleInterceptor`); * `parse*` narrows the untrusted wire shape. The proefbrief is `text/html` and * `ExcludeFromDescription`'d — a hand-written fetch, same seam as `letter-preview.adapter`. + * `X-Role` there is a dev-only identity stand-in (`role.ts`) and is sent only under + * `isDevMode()`, mirroring `roleInterceptor`'s own dev-only registration — a production + * build never sends it from this hand-written call either (BIO-012). */ const FAILED = $localize`:@@orgTemplate.action.failed:De actie is niet gelukt. Probeer het later opnieuw.`; -const PROEFBRIEF_FAILED = $localize`:@@orgTemplate.proefbrief.failed:De proefbrief kon niet worden geopend.`; +export const PROEFBRIEF_FAILED = $localize`:@@orgTemplate.proefbrief.failed:De proefbrief kon niet worden geopend.`; @Injectable({ providedIn: 'root' }) export class OrgTemplateAdapter { @@ -76,22 +79,26 @@ export class OrgTemplateAdapter { try { res = await fetch( `${environment.apiBaseUrl}/api/v1/admin/org-template/${encodeURIComponent(subOrgId)}/preview`, - { headers: { 'X-Role': currentRole() } }, + { headers: isDevMode() ? { 'X-Role': currentRole() } : {} }, ); } catch { return err(PROEFBRIEF_FAILED); } - if (!res.ok) { - try { - return err(problemDetail(await res.json(), PROEFBRIEF_FAILED)); - } catch { - return err(PROEFBRIEF_FAILED); - } - } + if (!res.ok) return err(await proefbriefErrorMessage(res)); return ok(await res.blob()); } } +/** Trust boundary (TE-002): maps a non-OK proefbrief response to a message. Exported + so a spec can call it directly instead of stubbing `globalThis.fetch`. */ +export async function proefbriefErrorMessage(res: Response): Promise { + try { + return problemDetail(await res.json(), PROEFBRIEF_FAILED); + } catch { + return PROEFBRIEF_FAILED; + } +} + // --- parse: wire → domain, validating at the boundary --- function parseSubOrg(dto: SubOrgSummaryDto): Result { diff --git a/apps/ssp/src/app/brief/infrastructure/reveal-bignummer.adapter.spec.ts b/apps/ssp/src/app/brief/infrastructure/reveal-bignummer.adapter.spec.ts new file mode 100644 index 0000000..c5ba514 --- /dev/null +++ b/apps/ssp/src/app/brief/infrastructure/reveal-bignummer.adapter.spec.ts @@ -0,0 +1,77 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { parseRevealed, REVEAL_FAILED, RevealBigNummerAdapter } from './reveal-bignummer.adapter'; + +describe('parseRevealed (TE-002 trust boundary)', () => { + it('accepts a well-formed body', () => { + const r = parseRevealed({ bigNummer: '12345678' }); + expect(r.ok).toBe(true); + if (r.ok) expect(r.value).toBe('12345678'); + }); + + // The finding's own named case: a numeric bigNummer must be rejected, not + // coerced — this is a PII reveal, not a display formatter. + it('rejects a bigNummer sent as a number', () => { + const r = parseRevealed({ bigNummer: 42 }); + expect(r).toEqual({ ok: false, error: REVEAL_FAILED }); + }); + + it('rejects a missing bigNummer field', () => { + expect(parseRevealed({}).ok).toBe(false); + }); + + it('rejects null and non-object bodies', () => { + expect(parseRevealed(null).ok).toBe(false); + expect(parseRevealed(undefined).ok).toBe(false); + expect(parseRevealed('12345678').ok).toBe(false); + expect(parseRevealed(42).ok).toBe(false); + }); +}); + +// isDevMode() reads the `ngDevMode` global the Angular CLI defines away in a +// production build. There is no ambient type for it in app code, so this is +// accessed through an untyped bag rather than a `declare const`. +const globals = globalThis as Record; +const originalNgDevMode = globals['ngDevMode']; +const setDevMode = (on: boolean) => { + globals['ngDevMode'] = on; +}; + +describe('RevealBigNummerAdapter.reveal (BIO-006a + BIO-012)', () => { + const okResponse = () => + ({ ok: true, json: () => Promise.resolve({ bigNummer: '12345678' }) }) as unknown as Response; + + beforeEach(() => setDevMode(true)); + afterEach(() => { + globals['ngDevMode'] = originalNgDevMode; + vi.unstubAllGlobals(); + }); + + it('sends X-Step-Up only when the caller passes stepUp: true', async () => { + const fetchSpy = vi.fn().mockResolvedValue(okResponse()); + vi.stubGlobal('fetch', fetchSpy); + + await new RevealBigNummerAdapter().reveal(false); + const headersWithoutStepUp = fetchSpy.mock.calls[0][1].headers as Record; + expect(headersWithoutStepUp['X-Step-Up']).toBeUndefined(); + + await new RevealBigNummerAdapter().reveal(true); + const headersWithStepUp = fetchSpy.mock.calls[1][1].headers as Record; + expect(headersWithStepUp['X-Step-Up']).toBe('true'); + }); + + it('sends X-Role only under isDevMode()', async () => { + const fetchSpy = vi.fn().mockResolvedValue(okResponse()); + vi.stubGlobal('fetch', fetchSpy); + + setDevMode(false); + await new RevealBigNummerAdapter().reveal(true); + const prodHeaders = fetchSpy.mock.calls[0][1].headers as Record; + expect(prodHeaders['X-Role']).toBeUndefined(); + expect(prodHeaders['X-Step-Up']).toBe('true'); // step-up is not a dev-only hatch + + setDevMode(true); + await new RevealBigNummerAdapter().reveal(true); + const devHeaders = fetchSpy.mock.calls[1][1].headers as Record; + expect(devHeaders['X-Role']).toBeDefined(); + }); +}); diff --git a/apps/ssp/src/app/brief/infrastructure/reveal-bignummer.adapter.ts b/apps/ssp/src/app/brief/infrastructure/reveal-bignummer.adapter.ts index 1b722e9..60d721b 100644 --- a/apps/ssp/src/app/brief/infrastructure/reveal-bignummer.adapter.ts +++ b/apps/ssp/src/app/brief/infrastructure/reveal-bignummer.adapter.ts @@ -1,47 +1,62 @@ -import { Injectable } from '@angular/core'; +import { Injectable, isDevMode } from '@angular/core'; import { Result, ok, err } from '@shared/kernel/fp'; import { currentRole } from '@shared/infrastructure/role'; import { problemDetail } from '@shared/infrastructure/api-error'; import { environment } from '@shared/environments/environment'; -const REVEAL_FAILED = $localize`:@@brief.reveal.failed:Het BIG-nummer kon niet worden getoond.`; +/** Exported so specs can assert against the same message id instead of retyping the + Dutch sentence (matches `letter-preview.adapter.ts`'s `PREVIEW_FAILED`). */ +export const REVEAL_FAILED = $localize`:@@brief.reveal.failed:Het BIG-nummer kon niet worden getoond.`; /** * Field-level PII reveal (PRD-0002 §5c). The case screen ships the BIG-nummer masked; * this unmasks it, gated server-side by the reveal capability AND a step-up. The - * step-up is stubbed as the `X-Step-Up` header — the caller sends it only after the - * user's confirm gesture, so a plain call (or a role without the capability) 403s. + * step-up is stubbed as the `X-Step-Up` header, sent only when the caller passes + * `stepUp: true` — `BriefStore.revealBigNummer()` is the only caller and it is only + * ever reachable after `behandel-scherm.component.ts`'s `onReveal()` confirm gesture, + * so the header now reflects that gesture instead of being a constant baked into this + * adapter (BIO-006a — a call that skips confirmation sends no step-up at all). * * Hand-written fetch (not the `ApiClient`) because the call needs a per-request header; * `.ExcludeFromDescription()` on the endpoint keeps the generated client JSON-only, the - * same seam as `/brief/preview` and uploads — which also means `X-Role` is set here. + * same seam as `/brief/preview` and uploads. `X-Role` is a dev-only identity stand-in + * (see `role.ts`) and is therefore only sent under `isDevMode()`, mirroring the + * `roleInterceptor` registration in `app.config.ts` — a production build never sends it + * from this hand-written call either (BIO-012). */ @Injectable({ providedIn: 'root' }) export class RevealBigNummerAdapter { - async reveal(): Promise> { + async reveal(stepUp: boolean): Promise> { let res: Response; try { res = await fetch(`${environment.apiBaseUrl}/api/v1/brief/reveal-bignummer`, { method: 'POST', - headers: { 'X-Role': currentRole(), 'X-Step-Up': 'true' }, + headers: { + ...(isDevMode() ? { 'X-Role': currentRole() } : {}), + ...(stepUp ? { 'X-Step-Up': 'true' } : {}), + }, }); } catch { return err(REVEAL_FAILED); } if (!res.ok) return err(await errorMessage(res)); - const body: unknown = await res.json().catch(() => null); - // Trust boundary: validate the shape before handing back a plain string. - if ( - typeof body === 'object' && - body !== null && - typeof (body as { bigNummer?: unknown }).bigNummer === 'string' - ) { - return ok((body as { bigNummer: string }).bigNummer); - } - return err(REVEAL_FAILED); + return parseRevealed(await res.json().catch(() => null)); } } +/** Trust boundary: validate the untrusted response shape before handing back a plain + string (TE-002) — exported so a spec can call it without stubbing `globalThis.fetch`. */ +export function parseRevealed(body: unknown): Result { + if ( + typeof body === 'object' && + body !== null && + typeof (body as { bigNummer?: unknown }).bigNummer === 'string' + ) { + return ok((body as { bigNummer: string }).bigNummer); + } + return err(REVEAL_FAILED); +} + async function errorMessage(res: Response): Promise { try { return problemDetail(await res.json(), REVEAL_FAILED); diff --git a/docs/project/refactor-backlog-setup/refactor-backlog/implementation/rb-11.md b/docs/project/refactor-backlog-setup/refactor-backlog/implementation/rb-11.md new file mode 100644 index 0000000..23c46c5 --- /dev/null +++ b/docs/project/refactor-backlog-setup/refactor-backlog/implementation/rb-11.md @@ -0,0 +1,179 @@ +# RB-11 — dev hatches out of prod, trust boundaries exported, doc corrected + +Status: **implemented** · 2026-08-27 · Source findings: `07-bio2-compliance.md` BIO-012, +TE-002, BIO-006(a)+(b) · `99-backlog.md` RB-11 + +## What was wrong + +**BIO-012 — the dev hatches were not actually dev-only.** The `roleInterceptor` / +`subjectInterceptor` chain is correctly registered only under `isDevMode()` +(`app.config.ts`), but three adapters bypass `HttpClient` entirely and set headers +themselves with no guard at all: + +- `reveal-bignummer.adapter.ts:26` — `'X-Role': currentRole(), 'X-Step-Up': 'true'` +- `letter-preview.adapter.ts:46` — `'X-Role': currentRole()`, plus `'X-Subject'` when present +- `org-template.adapter.ts:79` — `'X-Role': currentRole()` + +The readers underneath were ungated too: `role.ts:24` and `subject.ts:24` both read the +`?role=`/`?subject=` query param and **wrote it into `sessionStorage`** on any +navigation, in any build. For `?subject=` that value is a BSN — `subject.ts`'s own doc +comment argued at length that the BSN must never leave `SessionStore` and then routed it +through `sessionStorage` anyway. `docs/reference/roles-and-access.md:23` claimed "Both +are wired only under `isDevMode()` — they do not exist in a production build", which was +false for exactly these three call sites. + +**TE-002 — the reveal's trust boundary was not callable.** The response-shape validation +in `reveal-bignummer.adapter.ts` (the code's own comment called it a "Trust boundary") +lived inline inside `async reveal()`, after `await fetch(...)` on the global `fetch`. A +spec could not reach it without stubbing `globalThis.fetch`. The same shape recurred, +un-exported, in `letter-preview.adapter.ts`'s `errorMessage` and — contrary to the +finding's text, see "Judgement calls" below — as an inline `try/catch` (not yet a +function) in `org-template.adapter.ts`'s `proefbrief()`. + +**BIO-006(a) — the step-up stub was a constant.** `reveal-bignummer.adapter.ts` sent +`'X-Step-Up': 'true'` unconditionally, as a literal, so the backend's +`canReveal && X-Step-Up == "true"` precondition was satisfied by every call that reached +the endpoint and constrained nothing. + +**BIO-006(b) — the default role holds the PII-reveal capability, undocumented.** +`StubIdentityProvider`'s `_ =>` role-switch arm resolves any request with no (or an +unrecognised) `X-Role` header to `drafter` — the one role `Authz.CanRevealBigNummer` +grants. `roles-and-access.md` documented `drafter` as "the only role that may reveal a +BSN" without noting that it is also the fallback identity, so the least-privilege +consequence was invisible. + +## What changed + +| File | Change | +| --------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `libs/shared/src/infrastructure/role.ts` | `currentRole()` returns `'drafter'` immediately when `!isDevMode()` — no query-param read, no `sessionStorage` write | +| `libs/shared/src/infrastructure/subject.ts` | `currentSubject()` returns `undefined` immediately when `!isDevMode()` — same treatment for the BSN | +| `reveal-bignummer.adapter.ts` | `reveal(stepUp: boolean)`; `X-Role` sent only under `isDevMode()`, `X-Step-Up` sent only when `stepUp`; inline shape check moved to exported `parseRevealed(body)`; `REVEAL_FAILED` exported | +| `letter-preview.adapter.ts` | headers wrapped in `isDevMode() ? {...} : {}`; `errorMessage` exported | +| `org-template.adapter.ts` | `X-Role` sent only under `isDevMode()`; the inline `proefbrief()` error `try/catch` extracted to an exported `proefbriefErrorMessage`; `PROEFBRIEF_FAILED` exported | +| `apps/ssp/src/app/brief/application/brief.store.ts` | `revealBigNummer()` calls `this.revealAdapter.reveal(true)` — the literal now lives at the one call site reachable only after the UI's confirm gesture, not inside the adapter | +| `docs/reference/roles-and-access.md` | records which two places the `isDevMode()` gate now lives (interceptor registration **and** the reader functions) and why; adds the BIO-006(b) note that `drafter` is also the backend's fallback identity | +| 6 new/extended `*.spec.ts` | see "Verification" below | + +**The fix is two layers, not one, because the finding named both.** Gating only +`currentRole()`/`currentSubject()` would already stop the query param and the +`sessionStorage` write from working outside `isDevMode()` — the three adapters would +then send the safe default (`'X-Role': 'drafter'`, no `X-Subject`) even in production. +The adapters are _also_ wrapped in `isDevMode()` so a production request from any of the +three hand-written `fetch` calls carries no `X-Role`/`X-Subject` header at all, exactly +matching what a `HttpClient` request already does once `roleInterceptor` is not +registered — the two paths now agree on production behaviour instead of merely agreeing +on the resulting header value. + +**`X-Step-Up` is deliberately not folded into the same `isDevMode()` gate.** It is not a +`?role=`/`?subject=`-style dev override; it is the stub for a control BIO-006 says must +survive into production (in stubbed form) until a real step-up exists. Nesting it inside +`isDevMode()` would make the reveal endpoint permanently unreachable in a production +build. Instead it is gated on the `stepUp` parameter alone, which is `true` only when +`BriefStore.revealBigNummer()` — reachable only via `behandel-scherm.component.ts`'s +`onReveal()` confirm — calls it. + +## Judgement calls + +- **`org-template.adapter.ts`'s proefbrief error mapping was not "already a separate + function".** The finding's remediation text says "the proefbrief error mapping in + `org-template.adapter.ts` — both are already separate functions and only need + `export` and a spec", matching `letter-preview.adapter.ts`'s `errorMessage`. Reading + the file: the other two adapters do have a standalone `errorMessage`/similar function, + but `org-template.adapter.ts`'s proefbrief error handling was inlined directly in the + `try { … } catch { … }` block, not a named function. This is a minor factual + imprecision in the finding, not a blocker — I extracted the same inline logic into a + named `proefbriefErrorMessage`, exported it, and added the same spec shape as its two + siblings. The result matches the finding's intent (a callable, spec'd trust boundary) + even though the starting shape needed one extra step the finding didn't mention. +- **The BIO-006(a) literal moved to `BriefStore.revealBigNummer()`, not to the UI.** + `behandel-scherm.component.ts`'s `onReveal()` already gates the _only_ path that can + reach `store.revealBigNummer()` behind a `confirm()` dialog, and the store's own + docstring says the step-up gesture "is the UI's concern". Threading a boolean through + the component's `output()` and the page's template binding would touch three more + files for no behavioural change, since the call graph already guarantees confirmation + happened first. I moved the literal one layer up instead — out of the adapter (the + transport) and into the store (the command that is exclusively reachable via the + confirmed gesture) — which is the smallest change consistent with "not from the + adapter's literal" and with this repo's ui → application → infrastructure layering (ui + cannot call infrastructure directly to pass the flag down any other way). +- **Redundant-looking `isDevMode()` guards, kept anyway.** After gating + `currentRole()`/`currentSubject()`, the three adapters' own `isDevMode()` wrap around + the headers object is not strictly load-bearing for `X-Subject` (already `undefined` + outside dev) and only changes the _value sent_ for `X-Role` (a hardcoded `'drafter'` + vs. no header) rather than any security outcome (the backend treats both identically). + I kept the adapter-level gate anyway so the security posture is visible by inspection + at the fetch call site — matching `app.config.ts`'s `isDevMode() ? [...] : []` pattern + — rather than requiring a reviewer to trace into `role.ts`/`subject.ts` to confirm it. +- **No `proefbrief()`-level header spec.** `OrgTemplateAdapter` injects `ApiClient` via + `inject()`, so exercising `proefbrief()` itself needs a `TestBed` + a mock `ApiClient` + purely to reach a method that doesn't use either. I judged that disproportionate to the + marginal coverage gained, since the identical `isDevMode()` pattern is already + exercised end-to-end (via `fetch` stubbing) on the other two adapters + (`reveal-bignummer.adapter.spec.ts`, `letter-preview.adapter.spec.ts`), and the + underlying reader-level fix is covered directly in `role.spec.ts`. Noted here as a + residual rather than silently skipped. +- **`setRole()` (the dev-switcher writer) was left ungated.** BIO-012's evidence names + the two _readers_ (`currentRole`/`currentSubject`); `setRole()` is only ever invoked + from `debug-state.component.ts`, which is itself rendered only under + `shell.component.ts`'s `@if (isDev && debugPanel)`. Gating it too would be harmless but + wasn't asked for and has no reachable production call site to protect — left alone to + keep the diff to what the finding actually named. + +## Consequences worth knowing + +- **Doc correction, same diff.** `roles-and-access.md`'s "Both are wired only under + `isDevMode()`" line is accurate as of this commit — the gate now lives in the + interceptor registration **and** inside `currentRole()`/`currentSubject()` themselves. + Before this commit, the sentence was false for the three hand-written `fetch` paths; the + doc has been extended, not merely left as-is, to say _where_ the gate lives so a future + reader doesn't have to rediscover why the interceptor site alone wasn't sufficient. +- **CLAUDE.md needed no correction.** Its "Scenario toggle (dev-only, not wired in prod + builds)" and "Dev role stand-in (dev-only)" lines don't claim anything about the three + hand-written `fetch` adapters specifically (the accompanying sentence already says they + "bypass the interceptor", which stays true — they still don't go through + `HttpClient`). Those claims were already compatible with a fix landing here; they made + no false statement that needed walking back. +- **`?subject=` is still undocumented by name in `roles-and-access.md`.** The BIO-012 + evidence and this ticket's brief both discuss it, but the doc file never named + `?subject=`/`X-Subject` before this change and still doesn't get a dedicated section — + only the new paragraph under "How to switch role" mentions it in passing. A full + `?subject=` write-up (its own e2e-only purpose, `X-Medewerker`/`X-Rollen` parallel) is + arguably worth a follow-up doc pass, but out of scope for a security-focused ticket + about production leakage. +- **Behaviour spec regenerated.** `libs/shared/docs/behaviour-spec.mdx` is generated from + the suite (`npm run gen:behaviour-spec`) and is included in this diff — the CI gate's + drift check would otherwise fail on the 6 new `describe` blocks this ticket adds. + +## Verification + +Every fix below was confirmed **red without it** by temporarily reverting the source +change (tests unchanged) and re-running the affected suite, then restoring the fix: + +- `libs/shared/src/infrastructure/role.spec.ts` — removing the `if (!isDevMode())` guard + from `currentRole()` turned 3 "outside isDevMode()" tests red (`?role=` still honoured, + still written to `sessionStorage`). +- `libs/shared/src/infrastructure/subject.spec.ts` — same removal on `currentSubject()` + turned its 3 "outside isDevMode()" tests red (a BSN still read from the URL and written + to `sessionStorage`). +- `apps/ssp/src/app/brief/infrastructure/reveal-bignummer.adapter.spec.ts` — reverting + `reveal()` to the original unconditional `{ 'X-Role': currentRole(), 'X-Step-Up': 'true' }` + turned both `RevealBigNummerAdapter.reveal` tests red (`X-Step-Up` sent regardless of the + `stepUp` argument; `X-Role` sent regardless of `isDevMode()`). + +New specs, all pure/exported-boundary tests per house convention (no `TestBed`, no +`globalThis.fetch` stub needed for the pure halves): + +| File | Covers | +| ----------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------- | +| `reveal-bignummer.adapter.spec.ts` | `parseRevealed` (incl. the finding's own `{ bigNummer: 42 }` rejection case); `reveal()`'s `X-Step-Up`/`X-Role` gating via a stubbed `fetch` | +| `letter-preview.adapter.spec.ts` | `errorMessage`; `preview()`'s header gating via a stubbed `fetch` | +| `org-template.adapter.spec.ts` (extended) | `proefbriefErrorMessage` | +| `role.spec.ts` (new) | `currentRole()` dev behaviour + the `isDevMode()`-gated production behaviour | +| `subject.spec.ts` (new) | `currentSubject()` dev behaviour + the `isDevMode()`-gated production behaviour | + +`npm run ci` (lint, typecheck, `dep:check`, `format:check`, `check:tokens`, `check:seam`, +full test suite with coverage, `ng build --localize` for both apps, `npm audit`, backend +`dotnet format` + `dotnet test`, showcase-snippets/behaviour-spec/api-client drift +checks): **green**, including all 4 vitest projects (ssp/behandelportal/shared/beheer) and +`dotnet test` (241 passed). diff --git a/docs/reference/roles-and-access.md b/docs/reference/roles-and-access.md index 5348023..6613c2f 100644 --- a/docs/reference/roles-and-access.md +++ b/docs/reference/roles-and-access.md @@ -20,7 +20,15 @@ acting role to exercise the drafter/approver/admin flows. ## How to switch role (dev only) -Both are wired only under `isDevMode()` — they do not exist in a production build. +Both are wired only under `isDevMode()` — they do not exist in a production build. That +gate lives in two places: the `roleInterceptor` registration (`app.config.ts`) for every +`HttpClient` request, **and** inside `role.ts`'s `currentRole()` itself, because three +hand-written `fetch` calls (`reveal-bignummer.adapter.ts`, `letter-preview.adapter.ts`, +`org-template.adapter.ts`'s proefbrief) read the role directly and bypass the +interceptor entirely (RB-11/BIO-012). Before RB-11, `currentRole()` had no such gate, so +`?role=` kept working through those three calls in a production build even though this +page said otherwise; the same defect applied to `?subject=` and `subject.ts`, which is +how a BSN reached `sessionStorage` in any build. - **Dev switcher (easiest):** open the `⚙ state` panel (bottom-right in a dev build) and pick a role from the **role** dropdown. The page reloads with the new role. @@ -66,6 +74,16 @@ The admin pages appear in the header nav and in the dashboard **"Beheer"** secti matching capability is present — otherwise they are reachable only by URL (and the route guard redirects a user who lacks the capability back to `/dashboard`). +**`drafter` is also the backend's fallback identity (BIO-006).** It is not only the dev +switcher's initial selection — `StubIdentityProvider`'s role switch resolves **any** +request with no `X-Role` header at all (or an unrecognised one) to `drafter` too. Because +`drafter` is also the _only_ role that may reveal a BIG-nummer, the least-privilege +consequence is real: an unauthenticated or misconfigured caller inherits the PII-reveal +capability by default, rather than the weakest one. This is acceptable only because the +POC has no real identity or step-up yet (see the pre-production compliance checklist — +binding the reveal to an app-overlay attribute instead of the coarse role is a named, +not-yet-built item); it must not survive real identity and step-up. + ## The one principle Identity (AD/OIDC, faked here) supplies **coarse roles**; the app owns a **fine-grained capability** diff --git a/libs/shared/docs/behaviour-spec.mdx b/libs/shared/docs/behaviour-spec.mdx index 6a55c75..f2c75ea 100644 --- a/libs/shared/docs/behaviour-spec.mdx +++ b/libs/shared/docs/behaviour-spec.mdx @@ -20,7 +20,7 @@ tested where._ Every bullet below is a real test name from the suite — an `it()` title (frontend) or a test method name (backend), read as a sentence. Nothing here is hand-written prose: this page -**is** the suite, reshaped for a business reader. 415 frontend behaviours across +**is** the suite, reshaped for a business reader. 440 frontend behaviours across 9 contexts; 228 backend behaviours across 38 test classes. @@ -181,6 +181,16 @@ classes. - swaps the masked value for the revealed one on success - keeps the value masked and surfaces the error on failure +#### LetterPreviewAdapter.preview (BIO-012) + +- sends no X-Role/X-Subject headers outside isDevMode() +- sends X-Role (and X-Subject when known) under isDevMode() + +#### RevealBigNummerAdapter.reveal (BIO-006a + BIO-012) + +- sends X-Step-Up only when the caller passes stepUp: true +- sends X-Role only under isDevMode() + #### besluitGuidance - positief: counts inserted passages, no reden needed (positief has no redenen) @@ -232,6 +242,12 @@ classes. - marks added, removed, changed and unchanged by blockId - changedBlocks drops unchanged and keeps added/removed/changed +#### errorMessage (TE-002 trust boundary) + +- surfaces the ProblemDetails detail when present +- falls back to PREVIEW_FAILED when the body has no detail +- falls back to PREVIEW_FAILED when the body is not JSON + #### inferSelection - round-trips a positief selection @@ -270,6 +286,13 @@ classes. - rejects a missing count field - rejects a malformed history entry +#### parseRevealed (TE-002 trust boundary) + +- accepts a well-formed body +- rejects a bigNummer sent as a number +- rejects a missing bigNummer field +- rejects null and non-object bodies + #### passagesForBesluit - positief = shared intro + the positief passage, no negatief/reason passages @@ -278,6 +301,12 @@ classes. - preserves library order (= reading order) - never offers non-kern passages +#### proefbriefErrorMessage (TE-002 trust boundary) + +- surfaces the ProblemDetails detail when present +- falls back to PROEFBRIEF_FAILED when the body has no detail +- falls back to PROEFBRIEF_FAILED when the body is not JSON + #### redenenFor - derives reason checkboxes (code + label) from the negatief reason passages @@ -642,6 +671,29 @@ classes. - applies the pure update on dispatch - dispatch from inside an effect does not self-loop +#### currentRole (dev mechanism) + +- lists the three roles +- reads a valid ?role= from the URL and persists it for the tab +- falls back to drafter when nothing is set or the value is invalid + +#### currentRole (dev mechanism) › outside isDevMode() (production build) + +- ignores a ?role= in the URL and returns the default +- never touches sessionStorage +- ignores a role already sitting in sessionStorage from a prior dev session + +#### currentSubject (dev mechanism) + +- reads a ?subject= from the URL and persists it for the tab +- returns undefined when nothing has ever been set + +#### currentSubject (dev mechanism) › outside isDevMode() (production build) + +- ignores a ?subject= (a BSN) in the URL and returns undefined +- never writes the BSN into sessionStorage +- ignores a subject already sitting in sessionStorage from a prior dev session + #### delete flow (optimistic, revertible) - UploadDeleteRequested keeps the documentId for revert diff --git a/libs/shared/src/infrastructure/role.spec.ts b/libs/shared/src/infrastructure/role.spec.ts new file mode 100644 index 0000000..7b5cb9d --- /dev/null +++ b/libs/shared/src/infrastructure/role.spec.ts @@ -0,0 +1,65 @@ +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { currentRole, ROLES } from './role'; + +const setUrl = (search: string) => history.pushState({}, '', search || '/'); + +// isDevMode() reads the `ngDevMode` global the Angular CLI defines away in a +// production build. There is no ambient type for it in app code, so this is +// accessed through an untyped bag rather than a `declare const`. +const globals = globalThis as Record; +const originalNgDevMode = globals['ngDevMode']; +const setDevMode = (on: boolean) => { + globals['ngDevMode'] = on; +}; + +describe('currentRole (dev mechanism)', () => { + beforeEach(() => { + sessionStorage.clear(); + setUrl('/'); + setDevMode(true); + }); + afterEach(() => { + globals['ngDevMode'] = originalNgDevMode; + }); + + it('lists the three roles', () => { + expect(ROLES).toEqual(['drafter', 'approver', 'admin']); + }); + + it('reads a valid ?role= from the URL and persists it for the tab', () => { + setUrl('?role=admin'); + expect(currentRole()).toBe('admin'); + setUrl('/'); // navigation drops the query param — value stays sticky + expect(currentRole()).toBe('admin'); + }); + + it('falls back to drafter when nothing is set or the value is invalid', () => { + expect(currentRole()).toBe('drafter'); + setUrl('?role=nonsense'); + expect(currentRole()).toBe('drafter'); + }); + + // BIO-012: the three hand-written `fetch` adapters call this function directly, + // bypassing `roleInterceptor`'s own isDevMode()-gated registration — so the gate + // has to hold here, not just at the interceptor, or `?role=` keeps working in a + // production build through that side door. + describe('outside isDevMode() (production build)', () => { + beforeEach(() => setDevMode(false)); + + it('ignores a ?role= in the URL and returns the default', () => { + setUrl('?role=admin'); + expect(currentRole()).toBe('drafter'); + }); + + it('never touches sessionStorage', () => { + setUrl('?role=admin'); + currentRole(); + expect(sessionStorage.getItem('dev-role')).toBeNull(); + }); + + it('ignores a role already sitting in sessionStorage from a prior dev session', () => { + sessionStorage.setItem('dev-role', 'admin'); + expect(currentRole()).toBe('drafter'); + }); + }); +}); diff --git a/libs/shared/src/infrastructure/role.ts b/libs/shared/src/infrastructure/role.ts index 7931cae..6dd421c 100644 --- a/libs/shared/src/infrastructure/role.ts +++ b/libs/shared/src/infrastructure/role.ts @@ -1,3 +1,4 @@ +import { isDevMode } from '@angular/core'; import { Role } from '@shared/domain/role'; /** @@ -14,13 +15,22 @@ import { Role } from '@shared/domain/role'; * don't carry it), which would silently revert an admin to drafter mid-session and * 403 the admin endpoints. So a `?role=` seen in the URL is remembered for the tab; * later requests use the remembered value. Set `?role=drafter` (or a fresh tab) to - * reset. Dev-only — the interceptor itself is only wired under `isDevMode()`. + * reset. + * + * **Gated here, not only at the interceptor (BIO-012):** the `roleInterceptor` that + * consumes this for `HttpClient` traffic is only registered under `isDevMode()` + * (`app.config.ts`), but `brief`'s three hand-written `fetch` adapters call this + * function directly, bypassing that interceptor entirely. Reading `?role=` and + * writing it to `sessionStorage` is therefore gated in the function itself — outside + * `isDevMode()` the query param is never read, `sessionStorage` is never touched, and + * the fixed default (`drafter`, the least-privileged role) is returned every time. */ const STORAGE_KEY = 'dev-role'; export const ROLES: readonly Role[] = ['drafter', 'approver', 'admin']; const isRole = (v: string | null): v is Role => !!v && ROLES.includes(v as Role); export function currentRole(): Role { + if (!isDevMode()) return 'drafter'; const fromUrl = new URLSearchParams(window.location.search).get('role'); if (isRole(fromUrl)) { sessionStorage.setItem(STORAGE_KEY, fromUrl); diff --git a/libs/shared/src/infrastructure/subject.spec.ts b/libs/shared/src/infrastructure/subject.spec.ts new file mode 100644 index 0000000..254ab21 --- /dev/null +++ b/libs/shared/src/infrastructure/subject.spec.ts @@ -0,0 +1,57 @@ +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { currentSubject } from './subject'; + +const setUrl = (search: string) => history.pushState({}, '', search || '/'); + +// isDevMode() reads the `ngDevMode` global the Angular CLI defines away in a +// production build. There is no ambient type for it in app code, so this is +// accessed through an untyped bag rather than a `declare const`. +const globals = globalThis as Record; +const originalNgDevMode = globals['ngDevMode']; +const setDevMode = (on: boolean) => { + globals['ngDevMode'] = on; +}; + +describe('currentSubject (dev mechanism)', () => { + beforeEach(() => { + sessionStorage.clear(); + setUrl('/'); + setDevMode(true); + }); + afterEach(() => { + globals['ngDevMode'] = originalNgDevMode; + }); + + it('reads a ?subject= from the URL and persists it for the tab', () => { + setUrl('?subject=111222333'); + expect(currentSubject()).toBe('111222333'); + setUrl('/'); // navigation drops the query param — value stays sticky + expect(currentSubject()).toBe('111222333'); + }); + + it('returns undefined when nothing has ever been set', () => { + expect(currentSubject()).toBeUndefined(); + }); + + // BIO-012: a BSN is art. 9 GDPR special-category data. Prior to this fix this + // function wrote it into sessionStorage on any navigation, in any build. + describe('outside isDevMode() (production build)', () => { + beforeEach(() => setDevMode(false)); + + it('ignores a ?subject= (a BSN) in the URL and returns undefined', () => { + setUrl('?subject=111222333'); + expect(currentSubject()).toBeUndefined(); + }); + + it('never writes the BSN into sessionStorage', () => { + setUrl('?subject=111222333'); + currentSubject(); + expect(sessionStorage.getItem('dev-subject')).toBeNull(); + }); + + it('ignores a subject already sitting in sessionStorage from a prior dev session', () => { + sessionStorage.setItem('dev-subject', '111222333'); + expect(currentSubject()).toBeUndefined(); + }); + }); +}); diff --git a/libs/shared/src/infrastructure/subject.ts b/libs/shared/src/infrastructure/subject.ts index 5556d87..ec64d7f 100644 --- a/libs/shared/src/infrastructure/subject.ts +++ b/libs/shared/src/infrastructure/subject.ts @@ -1,3 +1,5 @@ +import { isDevMode } from '@angular/core'; + /** * Dev-only role stand-in's sibling (the reading MECHANISM for `X-Subject`; see * `role.ts`'s own doc comment for the twin `X-Role` mechanism this mirrors). This @@ -13,6 +15,13 @@ * hand-written `fetch`, which bypasses every `HttpInterceptorFn` — the same reason * that adapter already sets `X-Role` explicitly via `currentRole()`). * + * **Gated here, not only at the interceptor (BIO-012):** `subjectInterceptor` is only + * registered under `isDevMode()`, but `letter-preview.adapter.ts` calls this function + * directly and bypasses that interceptor. The value read here is a **BSN** — a GDPR + * special-category identifier — so outside `isDevMode()` the query param is never + * read and `sessionStorage` is never written; `undefined` is returned unconditionally, + * exactly as if no `?subject=` had ever been seen. + * * `undefined` (not a default BSN) when nothing has ever set `?subject=`: unlike * `currentRole()` (a closed enum with a sensible default), there is no "default * subject" to fall back to here — omitting the header entirely lets the backend's @@ -21,6 +30,7 @@ const STORAGE_KEY = 'dev-subject'; export function currentSubject(): string | undefined { + if (!isDevMode()) return undefined; const fromUrl = new URLSearchParams(window.location.search).get('subject'); if (fromUrl) { sessionStorage.setItem(STORAGE_KEY, fromUrl);