docs: record gap-review backlog issues 11-16

README.md

@@ -10,7 +10,7 @@ Waymark is a schema-driven journey orchestration engine for onboarding and compl
 - [`getting-started.md`](./getting-started.md) — prerequisites, local setup, and contribution workflow
 - [`user-guide.md`](./user-guide.md) — persona model, session lifecycle, and webhook integration
 - [`docs/journey-json.md`](./docs/journey-json.md) — journey JSON notation, structure, frontend linkage, and visualization
-- [`feature-gaps.md`](./feature-gaps.md) — known gaps tracked as GitHub issues
+- [`docs/issues/`](./docs/issues/) — known gaps tracked as GitHub issues
 
 ## Repository structure
 

docs/issues/11-browser-api-key-grants-operator.md

@@ -0,0 +1,68 @@
+---
+title: "Frontend API key grants full Operator access to every browser visitor"
+labels: ["security", "authentication", "frontend"]
+---
+
+## Summary
+
+The SPA authenticates to the backend with a **shared API key baked into the browser bundle** (`VITE_API_KEY`), and `ApiKeyAuthenticationHandler` maps any valid API key to a **full `Operator` principal**. Vite inlines `VITE_*` variables at build time, so the key ships as a literal string in the published JavaScript and is readable by anyone who loads the public onboarding page.
+
+Any visitor can extract the key and call every Operator-scoped endpoint.
+
+**Affected files:**
+- `src/backend/OpenOnboarding.Api/Authentication/ApiKeyAuthenticationHandler.cs` (lines 34–39)
+- `src/frontend/.env.example`
+- `src/frontend/src/onboarding/api/workflow-api-client.ts` (line 19)
+- `src/frontend/src/onboarding/hooks/useOnboarding.ts` (line 14), `useFlow.ts` (line 4)
+- `src/frontend/src/App.tsx` (line 107), `builder/AdminJourneyBuilderPage.tsx` (line 6), `builder/FlowAuthoringPanel.tsx` (line 13)
+- `src/frontend/src/sessions/SessionList.tsx`, `sessions/SessionDetail.tsx`, `webhooks/WebhookDeliveries.tsx`, `analytics/FlowAnalytics.tsx`, `flows/FlowVersionHistory.tsx`
+
+### The handler grants Operator unconditionally
+
+```csharp
+var claims = new[]
+{
+    new Claim(ClaimTypes.NameIdentifier, "api-key-user"),
+    new Claim(ClaimTypes.Name, "api-key-user"),
+    new Claim(ClaimTypes.Role, AppRoles.Operator),
+};
+```
+
+### What the leaked key unlocks
+
+With the key alone (no SSO, no cookie) a caller can reach, among others:
+
+| Endpoint | Policy | Exposure |
+|---|---|---|
+| `GET /api/workflow/sessions` | `OperatorOnly` | Every applicant session across every flow |
+| `GET /api/workflow/sessions/{id}/submissions` | `OperatorOnly` | All submitted form data (PII) |
+| `GET /api/workflow/flows/{id}/stats` | `OperatorOnly` | Business metrics |
+| `POST`/`PUT`/`DELETE /api/flows` | Operator | Create, rewrite or delete journey definitions |
+| `POST /api/webhooks` | Operator | Register an attacker-controlled webhook and exfiltrate future events |
+
+The resource-based `SessionOwnershipHandler` short-circuits on `IsInRole(Operator)`, so per-session ownership checks provide no defence either.
+
+The `Applicant` role and the `customerProfileId` ownership path already exist in the backend, but the shipped frontend has no way to obtain an applicant credential — it uses the Operator API key for applicant traffic.
+
+---
+
+## Requirements
+
+1. Stop shipping a privileged credential to the browser. The public onboarding app must authenticate as an **applicant**, not an operator.
+2. Introduce a per-session applicant credential issued by the backend at `POST /api/workflow/sessions/start` (e.g. a short-lived JWT carrying `role=Applicant` and `customerProfileId`), and have the SPA send that instead of `X-Api-Key`.
+3. Treat `X-Api-Key` as a **server-to-server** credential only: document it as such, and consider binding it to a named integration principal rather than a blanket Operator role.
+4. Operator-scoped UI must use the existing SAML `AdminSession` cookie, not an API key.
+5. Remove `VITE_API_KEY` from `.env.example` and from all frontend source, or restrict it to a local-development-only escape hatch that is absent from production builds.
+
+---
+
+## Acceptance Criteria
+
+- [ ] A production frontend build contains no API key string: `grep -r "$VITE_API_KEY" dist/` returns no matches.
+- [ ] Starting a session and submitting a step succeed without any `X-Api-Key` header being sent from the browser.
+- [ ] An applicant credential issued for session A returns `403 Forbidden` when used against session B's `GET /api/workflow/sessions/{id}` and `.../submit`.
+- [ ] An applicant credential returns `403 Forbidden` from `GET /api/workflow/sessions`, `GET /api/workflow/sessions/{id}/submissions` and `GET /api/workflow/flows/{id}/stats`.
+- [ ] Operator-scoped frontend views authenticate with the `AdminSession` cookie; removing the cookie makes them return `401`/`403` rather than succeeding via an API key.
+- [ ] `src/frontend/.env.example` no longer instructs developers to place an Operator-equivalent API key in the browser bundle.
+- [ ] Integration tests cover: applicant credential accepted for its own session; rejected for another session; rejected for every `OperatorOnly` endpoint.
+- [ ] `docs/runbook.md` documents the applicant credential lifetime and the server-to-server-only status of `Authentication:ApiKey`.

docs/issues/12-operator-ui-on-public-route.md

@@ -0,0 +1,52 @@
+---
+title: "Operator-only UI is rendered on the unauthenticated applicant route"
+labels: ["security", "frontend"]
+---
+
+## Summary
+
+`App.tsx` guards **only** `/admin/journey-builder` behind the SSO session check. The root route `/` — the page an applicant loads to complete onboarding — renders the full operator toolset below the step renderer with no authentication gate at all.
+
+**Affected file:** `src/frontend/src/App.tsx`
+
+### The guard covers one route
+
+```tsx
+if (isAdminJourneyBuilderRoute && adminAuthState !== 'authorized') { ... }   // line 223
+if (isAdminJourneyBuilderRoute && adminAuthState === 'authorized') { ... }   // line 231
+```
+
+Everything after that falls through to the applicant view, which renders:
+
+| Component | Line | What it exposes |
+|---|---|---|
+| `FlowAuthoringPanel` | 320 | Create / edit / publish journey definitions |
+| `FlowVersionHistory` | 328 | Version history and rollback controls |
+| `FlowAnalytics` | 372 | Aggregate conversion and drop-off metrics |
+| `SessionList` / `SessionDetail` | 384 | Every applicant's session, including submitted field data |
+| `WebhookDeliveries` | 390 | Webhook endpoints and delivery payloads |
+
+This is a separate defect from the credential problem in the API-key issue: even after the backend stops trusting a browser-held Operator key, these views would still render and issue operator API calls on the applicant's page. Their requests would then fail with `401`/`403`, leaving broken UI in front of applicants.
+
+The applicant page also renders a "Journey" and "Persona" picker plus the Visual Journey Builder canvas, which let an applicant switch themselves onto any seeded flow.
+
+---
+
+## Requirements
+
+1. Move every operator-facing section off the applicant route onto an authenticated route (e.g. extend `/admin` to host flow authoring, version history, analytics, sessions and webhook deliveries).
+2. Apply the existing `adminAuthState === 'authorized'` check — which already requires an `Operator` role from `GET /api/auth/me` — to those routes.
+3. Reduce the applicant route to the onboarding experience: the step renderer and its progress indication.
+4. Replace the ad-hoc `window.location.pathname` routing with a single place that maps route → required role, so a new admin surface cannot be added without a guard.
+
+---
+
+## Acceptance Criteria
+
+- [ ] Loading `/` with no `AdminSession` cookie renders only the onboarding step renderer — none of `FlowAuthoringPanel`, `FlowVersionHistory`, `FlowAnalytics`, `SessionList`, `SessionDetail` or `WebhookDeliveries` appear in the DOM.
+- [ ] Loading `/` issues no request to any `OperatorOnly` endpoint (`/api/workflow/sessions`, `/api/workflow/flows/{id}/stats`, `/api/webhooks`).
+- [ ] The journey and persona selectors are not present on the unauthenticated applicant route.
+- [ ] Operator sections are reachable on an authenticated admin route and render there for a user whose `/api/auth/me` response includes the `Operator` role.
+- [ ] A user without the `Operator` role who navigates directly to an admin route is redirected to `/login` and never sees operator data.
+- [ ] Route → required-role mapping lives in one module covered by unit tests.
+- [ ] Vitest tests assert the unauthenticated root render excludes each operator component; a Playwright spec covers the redirect for an unauthenticated admin route.

docs/issues/13-frontend-unit-tests-not-in-ci.md

@@ -0,0 +1,64 @@
+---
+title: "npm test fails on a clean checkout and frontend unit tests never run in CI"
+labels: ["bug", "testing", "frontend"]
+---
+
+## Summary
+
+Two defects compound each other:
+
+1. `npm test` (vitest) **fails** on a clean checkout — it collects the Playwright specs in `e2e/` and every one of them errors.
+2. The `frontend-ci` job never runs `npm test`, so the failure is invisible and the 108 frontend unit tests added for the frontend-unit-tests issue are **not enforced on any pull request**.
+
+**Affected files:**
+- `src/frontend/vitest.config.ts`
+- `.github/workflows/ci.yml` (`frontend-ci` job, lines 93–120)
+- `src/frontend/playwright/onboarding-journeys.spec.ts` (orphaned duplicate)
+
+### Reproduction
+
+```
+$ cd src/frontend && npm test
+ Test Files  6 failed | 14 passed (20)
+      Tests  108 passed (108)
+```
+
+Each failure is the same:
+
+```
+Error: Playwright Test did not expect test.describe() to be called here.
+ ❯ e2e/step-renderer.spec.ts:41:6
+```
+
+### Root cause
+
+`vitest.config.ts` excludes the wrong directory:
+
+```ts
+exclude: [...configDefaults.exclude, 'playwright/**'],
+```
+
+The Playwright specs live in `e2e/` — `playwright.config.ts` sets `testDir: './e2e'`. The `playwright/` directory still contains `onboarding-journeys.spec.ts`, byte-identical to `e2e/onboarding-journeys.spec.ts`, and is outside `testDir`, so that copy is never executed by Playwright either. Vitest excludes the dead directory and collects the live one.
+
+### Why CI did not catch it
+
+`frontend-ci` runs `npm ci` → `npm run lint` → `npm run build`. There is no test step. The other jobs run `npm run test:pact` and `npm run test:e2e`, so Pact and Playwright are covered, but the vitest suite is not.
+
+---
+
+## Requirements
+
+1. Fix the vitest exclude so it matches the real Playwright directory (`e2e/**`), or scope vitest's `include` to `src/**`.
+2. Delete the orphaned `src/frontend/playwright/` directory — it is a stale duplicate that no runner executes.
+3. Add a test step to `frontend-ci` so a failing unit test blocks the pull request.
+4. Consider uploading vitest coverage as a build artifact, matching the backend job's behaviour.
+
+---
+
+## Acceptance Criteria
+
+- [ ] `npm test` exits `0` on a clean checkout of `main`.
+- [ ] `npm test` reports `20 passed (20)` test files — no Playwright spec is collected by vitest.
+- [ ] `src/frontend/playwright/` no longer exists, and `npm run test:e2e` still runs the full `e2e/` suite.
+- [ ] `frontend-ci` runs `npm test` and the job fails when a unit test fails (demonstrate with a deliberately broken test on a scratch branch).
+- [ ] Introducing a failing vitest assertion causes a red CI check on a pull request.

docs/issues/14-sse-single-instance.md

@@ -0,0 +1,50 @@
+---
+title: "SSE session progress events break when the API runs more than one replica"
+labels: ["bug", "reliability", "infrastructure"]
+---
+
+## Summary
+
+`ISessionEventEmitter` has exactly one implementation, `InMemorySessionEventEmitter`, registered unconditionally as a singleton. It holds a `ConcurrentDictionary<Guid, Channel<SessionEvent>>` in **process memory**, so an event written on one API instance can only be read by a `GET /api/workflow/sessions/{id}/events` stream attached to that same instance.
+
+Both CD workflows deploy to platforms that routinely run more than one replica — Azure Container Apps (`cd-azure.yml`) and Amazon ECS (`cd-aws.yml`). At two or more replicas, an applicant whose SSE stream is held by instance B silently receives nothing for a step submitted through instance A.
+
+**Affected files:**
+- `src/backend/OpenOnboarding.Infrastructure/DependencyInjection/ServiceCollectionExtensions.cs` (line 67)
+- `src/backend/OpenOnboarding.Infrastructure/Services/InMemorySessionEventEmitter.cs`
+- `src/backend/OpenOnboarding.Api/Controllers/WorkflowController.cs` (`GET sessions/{sessionId:guid}/events`, line 269)
+
+### The registration has no distributed alternative
+
+```csharp
+services.AddSingleton<ISessionEventEmitter, InMemorySessionEventEmitter>();
+```
+
+Unlike `IEventBus` — which already selects `RabbitMqEventBus` when `RabbitMq:Host` is configured and falls back to `InMemoryEventBus` otherwise — there is no configuration-driven swap for the event emitter.
+
+### Failure mode
+
+The failure is silent. The stream stays open, the client sees no error, and progress simply never arrives; the applicant appears stuck until they reload and re-fetch state over HTTP. Sticky sessions would mask it only while the assigned instance survives — a scale-in or rolling deploy re-breaks it mid-journey.
+
+RabbitMQ is already a dependency in `docker-compose.yml` and the infrastructure layer, so a fan-out adapter has a natural home.
+
+---
+
+## Requirements
+
+1. Add a distributed `ISessionEventEmitter` adapter that fans an event out to every API instance — a RabbitMQ fanout exchange (reusing the existing connection management) or Redis pub/sub.
+2. Select it by configuration the same way `IEventBus` is selected, keeping `InMemorySessionEventEmitter` as the single-instance and test default.
+3. Preserve the existing semantics: per-session channels, the 100-event capacity bound, the completion/abandonment terminal events, and the stale-channel cleanup.
+4. Document the deployment requirement — running more than one replica without a distributed emitter configured must be called out in the runbook.
+5. Consider failing startup (or logging a prominent warning) when the in-memory emitter is active in a non-Development environment.
+
+---
+
+## Acceptance Criteria
+
+- [ ] A distributed `ISessionEventEmitter` implementation exists and is registered when its transport is configured; `InMemorySessionEventEmitter` remains the default otherwise.
+- [ ] An integration test starts two API hosts sharing one transport, opens the SSE stream on host B, emits an event through host A, and asserts host B's stream receives it.
+- [ ] Terminal events (`session-completed`, `session-abandoned`) close the stream on every instance, not just the emitting one.
+- [ ] Channel capacity limits and stale-channel cleanup behave identically for both implementations, covered by tests.
+- [ ] A non-Development environment running the in-memory emitter logs a warning naming the scale-out limitation.
+- [ ] `docs/runbook.md` documents the configuration keys and states that multi-replica deployments require the distributed emitter.

docs/issues/15-saml-assertion-encryption.md

@@ -0,0 +1,65 @@
+---
+title: "SAML metadata advertises assertion encryption the ACS cannot decrypt"
+labels: ["bug", "security", "authentication"]
+---
+
+## Summary
+
+`GET /auth/saml/metadata` publishes a `<KeyDescriptor use="encryption">` containing the SP certificate, telling every identity provider that Waymark accepts encrypted assertions. The callback never configures a decryption certificate, so an IdP that honours the advertised key will produce `EncryptedAssertion` responses that the ACS cannot read — every login fails with a generic `saml_invalid_assertion` redirect.
+
+Azure AD, Okta and Auth0 all offer to encrypt assertions when the SP metadata advertises an encryption key, and some tenants enforce it by policy.
+
+**Affected file:** `src/backend/OpenOnboarding.Api/Controllers/SamlAuthController.cs`
+
+### Metadata advertises the capability (line 45)
+
+```xml
+<KeyDescriptor use="encryption">
+  <ds:KeyInfo xmlns:ds="http://www.w3.org/2000/09/xmldsig#">
+    <ds:X509Data>
+      <ds:X509Certificate>{certBase64}</ds:X509Certificate>
+    </ds:X509Data>
+  </ds:KeyInfo>
+</KeyDescriptor>
+```
+
+### `BuildSamlConfiguration` never sets a decryption certificate
+
+```csharp
+var config = new Saml2Configuration
+{
+    Issuer = issuer,
+    SingleSignOnDestination = new Uri(idpSsoUrl),
+    SigningCertificate = spCert,
+    CertificateValidationMode = X509CertificateValidationMode.None,
+    RevocationMode = X509RevocationMode.NoCheck
+};
+```
+
+`Saml2Configuration.DecryptionCertificate` is required for ITfoxtec to unwrap an `EncryptedAssertion`. The SP certificate is already loaded with its private key (`X509Certificate2.CreateFromPem(certPem, keyPem)`), so the material needed is in hand.
+
+### Related loose end
+
+`src/frontend/src/App.tsx` defines login error messages for `saml_assertion_not_encrypted` and `saml_certificate_expired`, but the controller emits only `saml_invalid_assertion`, `saml_access_denied` and `saml_csrf_failed`. Those two messages are unreachable, and an encrypted-assertion failure surfaces to the operator as the generic "Invalid SAML assertion" with nothing in the logs distinguishing it.
+
+---
+
+## Requirements
+
+1. Set `DecryptionCertificate` on the `Saml2Configuration` from the configured SP certificate and private key so `EncryptedAssertion` responses decrypt.
+2. Add an optional `Authentication:Saml:RequireEncryptedAssertion` setting. When `true`, reject an unencrypted assertion and redirect with `saml_assertion_not_encrypted`.
+3. If encryption will not be supported, remove the `use="encryption"` `KeyDescriptor` from metadata instead — the current state advertises a capability that does not exist and must not stay as-is.
+4. Distinguish decryption and signature failures in the logs so an operator can tell them apart from the generic redirect.
+5. Reconcile the frontend error-code table with the codes the controller actually emits (including `saml_certificate_expired`, which is currently unreachable).
+
+---
+
+## Acceptance Criteria
+
+- [ ] A `POST /auth/saml/callback` carrying an `EncryptedAssertion` encrypted to the SP certificate decrypts successfully and issues an `AdminSession` cookie.
+- [ ] Both a signed unencrypted assertion and a signed encrypted assertion are accepted when `RequireEncryptedAssertion` is unset or `false`.
+- [ ] With `RequireEncryptedAssertion=true`, an unencrypted assertion redirects to `/login?error=saml_assertion_not_encrypted`.
+- [ ] An assertion encrypted to the wrong key redirects to `/login?error=saml_invalid_assertion` and logs a decryption-specific message.
+- [ ] Tests cover the encrypted happy path, the wrong-key failure, and the `RequireEncryptedAssertion` rejection, using self-signed test certificates in the style of the existing `SamlAuthControllerTests`.
+- [ ] Every error code in the frontend `LOGIN_ERROR_MESSAGES` table is reachable from the controller, or is removed.
+- [ ] `docs/runbook.md` documents the encryption configuration and the IdP-side setup it implies.