eho/atomic-design-poc
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(brief): keep the dev hatches out of production builds (RB-11)

Browse Source 
BIO-012: roleInterceptor/subjectInterceptor are correctly registered
only under isDevMode(), but three hand-written fetch adapters
(reveal-bignummer, letter-preview, org-template's proefbrief) bypass
HttpClient and set X-Role/X-Subject themselves with no guard. The
readers underneath, role.ts and subject.ts, were ungated too: they
read ?role=/?subject= and wrote it into sessionStorage on any
navigation, in any build -- for ?subject= that value is a BSN, which
is exactly what SessionStore's G1 comment promises never happens.

Gate both layers: currentRole()/currentSubject() return their safe
default immediately outside isDevMode() (no query-param read, no
sessionStorage write), and the three adapters additionally wrap their
headers in isDevMode() so a production request carries neither header
at all, matching what an HttpClient request already does once the
interceptors aren't registered.

TE-002: reveal-bignummer's response-shape validation was a "Trust
boundary" a spec could only reach by stubbing globalThis.fetch.
Exported it as parseRevealed(body), matching the other 30 parse*
boundaries in the repo. Same treatment for letter-preview's
errorMessage and org-template's proefbrief error mapping (extracted
from an inline try/catch into a named, exported function first, since
it wasn't already separate).

BIO-006(a): reveal-bignummer sent X-Step-Up: 'true' unconditionally,
so the backend's step-up precondition constrained nothing. reveal()
now takes a stepUp flag; BriefStore.revealBigNummer() -- reachable
only after the UI's confirm() gesture -- is the one that supplies it,
so the literal no longer lives in the transport adapter.

BIO-006(b): documented in roles-and-access.md that drafter is also
the backend's fallback identity (StubIdentityProvider's catch-all
arm), not just the dev switcher's initial choice -- so the
least-privilege consequence of it also being the only role that may
reveal a BSN is visible.

Doc correction, same diff: roles-and-access.md's "wired only under
isDevMode()" claim was false for the three hand-written fetch paths;
it now says where the gate lives (interceptor registration and the
reader functions) so it doesn't go stale the same way again.
CLAUDE.md's dev-only claims needed no correction -- they already
noted these three calls bypass the interceptor.

Every fix has a test confirmed red by temporarily reverting the
source change and rerunning the suite before restoring it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
eho
2026-08-27 14:20:47 +02:00
co-authored by Claude Opus 5
parent ae7781efef
commit 772c47ea43
14 changed files with 632 additions and 37 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/project/refactor-backlog-setup/refactor-backlog/implementation/rb-11.md
+179
View File
@@ -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<void>()` 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).
docs/reference/roles-and-access.md
+19 -1
View File
@@ -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**