Makes the application say what it is doing and when it fails

U4. Console logging plus Sentry, a same-origin tunnel so ad blockers cannot
silence browser errors, Umami on the admin SPA, and six security events that
alert rules can actually be built on.

The correlation id is the W3C trace id from the ambient Activity, enabled by
one line of ActivityTrackingOptions so every entry from every category carries
it without touching a call site. It propagates across the master/slave
boundary via traceparent, which TraceIdentifier cannot do at all, and it is
the same value ProblemDetails already returns to the browser.

The security events use source-generated LoggerMessage with constant
templates. Sentry groups log events by message, so interpolating an email
address would give every address its own issue and "more than 20 failed
logins in five minutes" could never fire — the events would arrive, be
visible, be tagged, and the alerting would silently be impossible. A test
asserts the rendered message is identical across argument values.

Scrubbing happens in-process, before transmission, and covers Set-Cookie as
well as Cookie: the login response issues the refreshToken there, so
scrubbing only the request side would protect nothing. Transactions are
scrubbed too, because they carry request data and are the channel nobody
thinks of.

The tunnel derives its destination from the DSN once at startup and reads
nothing from the request, which is what separates a tunnel from a
server-side request forgery primitive. Size is capped by a bounded read
rather than by trusting Content-Length, and the endpoint is rate limited.

Two things found along the way. Zod 4's url() hands the value to the URL
constructor, which accepts any scheme — so the existing frontend validation
would have accepted the exact "htp://" typo BR-U4-24 names, and the SPA
would have called a nonexistent origin. Now constrained to http(s). And the
new appsettings comments are verified against the real configuration
provider, because the failure mode if it rejected them is both hosts
refusing to start after a release switch.

One deviation. IAdminTokenValidator was meant to gain a reason-reporting
overload; implemented that way, a substitute returning false by default
silently inverted the access decision while both methods compiled. Two
methods whose difference is invisible at the call site is the defect, so it
is now a single Validate returning AdminTokenResult.

Touches two files from already-committed units: DatabaseMigrationExtensions
(U2) gains a flush before the rethrow, or the one Critical event in the
system dies with the process; AdminTokenValidator (U1) classifies why a
bypass was refused.

Build 0 errors; 366 backend tests pass, up from 315, and 237 frontend tests,
up from 213. tsc clean, eslint clean on every changed file.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HHoJpxYXzHACSQguHrC5fw
This commit is contained in:
2026-07-28 11:25:54 +02:00
co-authored by Claude Opus 5
parent a122548454
commit 8e79a72340
54 changed files with 2598 additions and 66 deletions
@@ -0,0 +1,137 @@
# Code Generation Summary — U4 Observability Integration
**Generated**: 2026-07-28
**Verified**:
- `dotnet build SlpModularCms.sln -c Release`**0 errors**
- `dotnet test`**366 passed, 0 failed** (315 after U3, so **+51**)
- `pnpm test`**237 passed, 0 failed** (baseline 213, so **+24**)
- `npx tsc -b` → clean
- `npx eslint` on all changed frontend files → **0 problems**; full `pnpm run lint` unchanged at the pre-existing 5 errors / 1 warning (FR-21, U5)
---
## Backend Files Created
| File | Purpose |
|---|---|
| `Core/Observability/BypassRejectionReason.cs` | Why an admin bypass was refused — never the token |
| `Core/Observability/SecurityEvents.cs` | `SecurityEventNames` + six source-generated `LoggerMessage` methods, `EventId` 50015006 |
| `Core/Hosting/Observability/ObservabilityOptions.cs` | `Observability` configuration section |
| `Core/Hosting/Observability/LoggingExtensions.cs` | `AddCmsLogging` — activity tracking, `IncludeScopes`, per-environment console |
| `Core/Hosting/Observability/SentryEventScrubber.cs` | `ISentryEventScrubber` — removes four credential headers and the body |
| `Core/Hosting/Observability/SecurityEventProcessor.cs` | Promotes the event name to a `security_event` Sentry tag |
| `Core/Hosting/Observability/SentryTunnelTarget.cs` | Envelope endpoint parsed from the DSN, once, at startup |
| `Core/Hosting/Observability/SentryExtensions.cs` | `AddCmsObservability` / `UseCmsSentry` |
| `Core/Hosting/Observability/SentryTunnelExtensions.cs` | The `/sentry-tunnel` endpoint |
| `Core/Hosting/Observability/SecurityAuthorizationResultHandler.cs` | Reports authorization denials, then defers to the framework |
## Frontend Files Created
| File | Purpose |
|---|---|
| `frontend/src/lib/sentry.ts` | `initSentry()` — skips without a DSN, tunnels same-origin |
| `frontend/src/components/SentryErrorBoundary.tsx` | Recoverable fallback with no exception text |
| `frontend/src/components/UmamiAnalytics.tsx` | Script injection, once, never in development |
## Files Modified
| File | Change |
|---|---|
| `Core/Hosting/ServiceCollectionExtensions.cs` | `OnRejected` on the rate limiter; `sentry-tunnel` limiter; registers the authorization result handler |
| `Core/Hosting/DatabaseMigrationExtensions.cs` (**U2**) | Emits `MigrationFailure`; `SentrySdk.Flush` before the rethrow |
| `Core/Hosting/Security/IAdminTokenValidator.cs` (**U1**) | Replaced by a single `Validate` returning `AdminTokenResult` — see deviations |
| `Core/Hosting/Security/AdminTokenValidator.cs` (**U1**) | Classifies the rejection cause from the exception type |
| `Core/Identity/Services/AuthService.cs` | `ILogger` dependency; emits `FailedLogin` |
| `Modules.Availability/Middleware/AvailabilityMiddleware.cs` | Emits `AdminBypassRejected` |
| `Modules.Availability/Controllers/MasterController.cs` | Emits `MasterApiKeyRejected` on all four rejection paths |
| `Modules.Master/Controllers/SlaveStatusController.cs` | Same |
| `Api/Program.cs`, `Api.Slave/Program.cs` | `AddCmsLogging``UseCmsSentry``AddCmsObservability``MapSentryTunnel` |
| `Api/appsettings.json`, `Api.Slave/appsettings.json` | `Observability` section; `Logging` raised to `Information`; EF command logging pinned; `RateLimiting:SentryTunnel` |
| `frontend/src/lib/config.ts` | Four new fields; `apiBaseUrl` accepts empty **or** an http(s) URL |
| `frontend/src/main.tsx` | `initSentry()` first; boundary inside `AuthProvider`; `UmamiAnalytics` |
| `frontend/vite.config.ts` | `__APP_VERSION__` define; dev proxy for `/sentry-tunnel` → local API |
| `frontend/src/vite-env.d.ts` | Five `VITE_` variables and `__APP_VERSION__` |
| `frontend/src/i18n/locales/{nl,en}/translation.json` | `error.unexpected.title` / `.message` |
| `frontend/package.json` | `@sentry/react` `10.68.0` |
| `Core.Tests/*.csproj` | Links both hosts' real `appsettings.json` into the test output |
## Test Files Created
| File | Tests | Covers |
|---|---|---|
| `Core.Tests/Hosting/Observability/SentryEventScrubberTests.cs` | 12 | Four headers removed, body nulled, diagnostic fields retained, transactions scrubbed |
| `Core.Tests/Hosting/Observability/SentryTunnelTargetTests.cs` | 8 | Endpoint derivation, no-DSN state, unparseable DSN fails, path outside `/api` |
| `Core.Tests/Hosting/Observability/SecurityEventsTests.cs` | 10 | **Identical rendered message across argument values**, levels, distinct IDs, tagging |
| `Core.Tests/Hosting/Security/AdminTokenRejectionReasonTests.cs` | 11 | Each rejection reason from a real token |
| `Core.Tests/Hosting/DeployedConfigurationTests.cs` | 13 | The committed `appsettings.json` of both hosts |
| `frontend/src/lib/sentry.test.ts` | 5 | Skip without a DSN, tunnel not an ingest URL, tags, PII off |
| `frontend/src/lib/config.test.ts` | 7 | Same-origin resolution, explicit URL preserved, malformed rejected |
| `frontend/src/components/UmamiAnalytics.test.tsx` | 6 | Never in dev, nothing without an ID, injected once, nothing rendered |
| `frontend/src/components/SentryErrorBoundary.test.tsx` | 5 | Fallback shown, **no exception text**, retry remounts, works without a DSN |
| `frontend/src/lib/api-client.test.ts` (extended) | +1 | Relative URL construction with an empty base |
---
## Two Findings Worth Reading
### 1. `z.string().url()` never caught the typo its own rule cites
BR-U4-24 says a malformed value must not be silently accepted, and names `htp://localhost:7221` as the case. **Zod 4's `url()` validates by handing the value to the `URL` constructor, which accepts any scheme** — verified directly:
```
z.string().url().safeParse('htp://localhost:7221') → success: true
z.string().url().safeParse('ftp://x.nl') → success: true
```
So the *pre-existing* validation, before this unit touched it, would have accepted the exact typo the rule exists to catch. The schema is now `z.url({ protocol: /^https?$/ })`, which rejects both. This was not a regression introduced here; it was found because BR-U4-24 asked for a test that the old schema could not have passed.
### 2. Comments in `appsettings.json` are now verified, not assumed
Several non-obvious values gained `//` comments. The JSON configuration provider tolerates them — but "tolerates" was worth verifying rather than assuming, because the failure mode is *both hosts refusing to start after a release switch*. `DeployedConfigurationTests` loads both real files through the real provider, and also runs `ValidateOnStart` against the committed `SecurityHeaders` section, so a policy-name typo fails here rather than in a deployment.
---
## Deviations from the NFR Design
**`IAdminTokenValidator` ended up with one method, not two.** The design specified adding an overload returning the reason alongside the existing boolean. Implementing it that way immediately broke two existing tests in a revealing way: the middleware called the new overload, the tests stubbed the old one, and an `NSubstitute` substitute returns `false` by default — so the *access decision silently inverted* while both methods still existed and compiled.
That is the shape of the defect, not just of the test failure: two methods where the difference is invisible at the call site, and a caller using the boolean form gets the right decision and silently emits no security event. Replaced with a single `Validate(string?) → AdminTokenResult` record. Five call sites and two test files updated; behaviour otherwise identical.
**`Set-Cookie` was already in the NFR design's scrub list** and is implemented; noted here because the functional design named only `Cookie`.
**`MigrationFailure` uses `SentrySdk.Flush`, not `FlushAsync`.** `MigrateCoreDatabase` is synchronous, and making it async would change a U2 signature and both hosts' startup for no benefit.
---
## Business Rule Coverage
| Rule | Where | Test |
|---|---|---|
| BR-U4-01, BR-U4-02 console always active at `Information` | `AddCmsLogging`, `appsettings.json` | `HostConfiguration_ShouldAllowInformationLevelLogging` |
| BR-U4-03 correlation ID on every entry | `ActivityTrackingOptions` + `IncludeScopes` | Carried to Build and Test |
| BR-U4-04 logging before Sentry | `Program.cs` order | Carried to Build and Test |
| BR-U4-05 no secrets in logs | EF category pinned; event templates | `HostConfiguration_ShouldPinEfCommandLoggingBelowInformation` |
| BR-U4-06, BR-U4-07 threshold split | `MinimumEventLevel` / `MinimumBreadcrumbLevel` | Carried to Build and Test |
| BR-U4-08 absent DSN supported | `UseCmsSentry` early return | `HostConfiguration_ShouldShipWithoutASentryDsn` |
| BR-U4-09 never blocks a request | SDK is fire-and-forget; tunnel returns `202` | Carried to Build and Test |
| BR-U4-10, BR-U4-11 environment and release tags | `UseCmsSentry` | `sentry.test.ts` (frontend side) |
| BR-U4-12…14 scrubbing in-process | `SentryEventScrubber` | 12 scrubber tests |
| BR-U4-15…21 tunnel | `SentryTunnelExtensions`, `SentryTunnelTarget` | 8 target tests; endpoint carried to Build and Test |
| BR-U4-22…25 frontend configuration | `config.ts` | 7 config tests + the api-client test |
| BR-U4-26 frontend Sentry skipped | `initSentry` | `sentry.test.ts` |
| BR-U4-27…29 Umami | `UmamiAnalytics` | 6 component tests |
| Six security events | `SecurityEvents` + emission sites | 10 event tests |
---
## Carried to Phase-Level Build and Test
| Behaviour | Why it needs a running host |
|---|---|
| **Trace ID propagates master → slave** | The entire justification for resolving OPEN-01 as the W3C trace ID. Needs both hosts and a real master/slave call |
| `TraceId` actually appears in rendered console output | `ActivityTrackingOptions` populates the scope; `IncludeScopes` renders it. Set one and forget the other and every line looks normal with no correlation ID and no error |
| Tunnel: `404` without a DSN, `413` oversized, `202` on upstream failure, `503` when availability-disabled | Needs the endpoint in a real pipeline |
| `security_event` tag present on a real Sentry event | The processor reads `Extra["SecurityEvent"]`; that the SDK populates it from the log state is verified against a live event rather than assumed |
| Sentry event and breadcrumb thresholds observed end to end | Needs a DSN and a real send |
| `Observability__SentryDsn` environment variable overrides the empty committed value | Confirms the D-16 secret path works |
| Both hosts start with all new sections | Partly pre-empted by `DeployedConfigurationTests` |