SLP_Software/slp-modular-cms
1
0
Fork 0
Code Issues Pull Requests Actions Projects Releases Wiki Activity

Settles how the headers and observability units get built

Browse Source 
NFR Design for U3 and U4. Two decisions the earlier stages had deliberately
left open, plus four risks the functional design did not name.

OPEN-01 closed: the correlation ID is the W3C trace ID from the ambient
Activity, with TraceIdentifier as the fallback. 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 client.

REF-U3-01 raised: BR-U3-22's Umami-origin startup warning cannot work. The
backend never sees VITE_UMAMI_WEBSITE_ID, so the check would either always
warn or never warn. Withdrawn from U3 and replaced by a blocking U5 CI gate
that compares the frontend build variable against that environment's CSP
origins, where both values are visible.

Four additions beyond the functional design:
- Set-Cookie added to the scrub list; the login response issues the
  refreshToken there, so scrubbing only the request cookie protects nothing
- SetBeforeSendTransaction alongside SetBeforeSend; transactions carry
  request data too
- OnRejected on the rate limiter; today a 429 leaves no trace anywhere
- FlushAsync before the migration-failure rethrow, or the one Critical
  event in the system dies with the process

Two traps recorded with tests attached: Sentry groups log events by message
template, so interpolated messages make FR-19's rate-based alert rules
unimplementable while appearing to work; and DefaultHttpContext.Response
.OnStarting is a no-op, so the obvious middleware test asserts nothing.

Three values chosen rather than escalated, each one line to change and all
three listed for review at the end of U4's pattern document: JSON console
outside Development, TracesSampleRate 0.1, tunnel cap 200 KB.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HHoJpxYXzHACSQguHrC5fw
This commit is contained in:
Sluijsens
2026-07-28 10:25:11 +02:00
co-authored by Claude Opus 5
parent 589146a167
commit 5102f8668b
7 changed files with 1210 additions and 9 deletions
Download Patch File Download Diff File Expand all files Collapse all files
aidlc-docs/features/gitea-deployment-workflow/audit.md
+77
View File
@@ -800,3 +800,80 @@ Note this is a **blocking** consideration under the enabled Security Baseline ex
**No blocking security findings. No new deviation** — FU2 = C removed the need for the DEV-06 that Q2 = B would have required.
---
## 2026-07-28 — CONSTRUCTION: NFR Design (Round 2, U3 + U4)
**Stage**: NFR Design — executed for U3 and U4 only, per the execution plan. NFR Requirements skipped for all units (already captured in `requirements.md` § 5 and § 6).
No question round. The two decisions the earlier stages had left open were technical rather than preferential, and three remaining values had defensible defaults; all five are recorded below with the reasoning, and each is one line to change.
### OPEN-01 CLOSED — the correlation ID is the W3C trace ID
Decided: the trace ID from the ambient `Activity`, with `HttpContext.TraceIdentifier` as the fallback when no `Activity` exists. Enabled by `ActivityTrackingOptions` plus `IncludeScopes`, so every entry from every category — framework included — carries it with **no change to any call site**.
Two reasons, the first decisive:
1. **It crosses the master/slave HTTP boundary.** `HttpClient` injects `traceparent`; the slave's hosting layer adopts it. The hardest diagnostic question in this codebase — "the master says the slave rejected its API key; what did the slave see?" — is answerable with one identifier. `TraceIdentifier` is host-local and cannot answer it at all.
2. **It is already the value the client is shown.** ASP.NET Core's `ProblemDetails` writes `traceId` as `Activity.Current?.Id ?? HttpContext.TraceIdentifier`. Choosing the W3C trace ID makes the log entry, the Sentry event, the slave's log entry and the browser's error response carry one value. Choosing `TraceIdentifier` would create two competing identifiers for the same request.
Trap recorded: `ActivityTrackingOptions` populates the scope, `IncludeScopes` renders it. Set the first and forget the second and every log line looks entirely normal with no correlation ID and no error anywhere. A test asserts the field is present in rendered output.
### REF-U3-01 — BR-U3-22 is not implementable and has been replaced
BR-U3-22 asked the backend to warn at startup when a Umami website ID is configured but its script origin is missing from the CSP. **The backend cannot see that ID**: it is a `VITE_` variable baked into the frontend bundle at build time. The check would either always warn or never warn.
Withdrawn from U3 and moved to the **U5 CI workflow**, where both values are visible: if `VITE_UMAMI_SCRIPT_URL` is set for an environment's frontend build, its origin must appear in that environment's `SecurityHeaders:AllowedScriptOrigins`, or the **pipeline fails**. Stronger than the rule it replaces — drift now blocks the deployment instead of writing a warning into a log on a Raspberry Pi.
Carried to U5 as a required gate.
### Four risks closed that the functional design did not name
1. **`Set-Cookie` added to the scrub list.** The functional design named the request `Cookie` header. Response headers can be attached to an event, and `Set-Cookie` on the login and refresh responses carries the `refreshToken` being issued. Scrubbing the request cookie while sending the response cookie would protect nothing.
2. **`SetBeforeSendTransaction` as well as `SetBeforeSend`.** Performance transactions carry request data too. Scrubbing only events leaves a second channel open — less obvious precisely because nobody thinks of a transaction as containing headers. Enabling tracing later without touching that file would start leaking.
3. **`OnRejected` on the rate limiter.** `AddCmsRateLimiting` sets only `RejectionStatusCode`. Today a brute-force attempt against `/api/v1/Auth/login` returns `429` and leaves **no trace anywhere** — the one rate limiter this application has is unobservable. The `RateLimitTriggered` security event needs this callback to exist.
4. **`SentrySdk.FlushAsync` before the migration-failure rethrow.** The SDK sends in the background; a process that throws during startup and exits kills the sender first. The migration failure is the one `Critical` event in the system, and it was the event most likely never to arrive. Bounded at five seconds: a host that cannot reach its database is already down.
### Two "looks correct, does nothing" traps recorded with tests attached
- **Sentry groups log-derived events by message template.** Emitted with interpolation, every distinct email produces a separate Sentry issue, and an alert rule of the form "more than 20 failed logins in 5 minutes" can never fire — no single issue ever reaches 20. FR-19 would be unimplementable while appearing to work. Resolved with source-generated `LoggerMessage` (fixed templates, `EventId` 5001–5006) plus an `ISentryEventProcessor` that maps the ID to a `security_event` tag, so alert rules filter on a tag rather than message text.
- **`DefaultHttpContext.Response.OnStarting` is a no-op.** A unit test that drives the security-headers middleware through a `DefaultHttpContext` finds no headers — and a test asserting *nothing was set* passes for the wrong reason and keeps passing after the middleware is deleted. Resolved by putting all decision logic in a static, dependency-free `SecurityHeaderWriter` tested against a bare `HeaderDictionary`, leaving the middleware as glue whose only risk is ordering, which is verified at Build and Test.
### Three defaults chosen rather than escalated
| Decision | Chosen | Reasoning |
|---|---|---|
| Console format | JSON outside Development, human-readable locally | Supervised process on the Pi; `TraceId` becomes a queryable field. Both set `IncludeScopes`, so the information is identical either way |
| `TracesSampleRate` | `0.1` on both sides | Sentry's free plan counts transactions against the same quota as errors, and this design's value is in errors. The reference project's `1.0` is for a low-traffic marketing site with no API |
| Tunnel max payload | 200 KB | Envelopes with a stack trace and breadcrumbs run tens of KB |
Each is one line; all three are listed at the end of U4's pattern document for review.
### Packages added
| Project | Package | Version |
|---|---|---|
| `SlpModularCms.Core` | `Sentry.AspNetCore` | `6.8.0` — latest on nuget.org; `net10.0` asset to be confirmed at Code Generation |
| `frontend` | `@sentry/react` | `^10.68.0` — same major as the reference project |
`Sentry.AspNetCore` goes into `Core` rather than the hosts, because the scrubber, the processor, `UseCmsSentry` and the tunnel all live there and both hosts consume them. **FR-22 interaction**: this adds a new dependency subtree while U5 is due to pin two packages against advisories — the new tree must be checked in the same pass, not assumed clean because it is new.
### Three divergences from the reference project, all deliberate
- **The dev tunnel proxy targets the local API, not Sentry's ingest host.** The reference proxies straight to Sentry, which hard-codes the project ID in `vite.config.ts` and carries a `LET OP` comment about keeping it in sync by hand. Targeting the local API removes both the synchronisation and the committed project ID.
- **`UmamiAnalytics` drops the script-removing cleanup.** Under React 18 `StrictMode` the double-invocation becomes inject → remove → inject, and removing the element does not unregister Umami's listeners, so the first page view can be counted twice. The component lives for the application's lifetime and has nothing to clean up; the existing duplicate guard handles re-invocation.
- **`tanstackRouterBrowserTracingIntegration` not adopted.** It needs the router instance, and importing `@/router` from `sentry.ts` — called before anything else in `main.tsx` — inverts the initialisation order and pulls the whole route tree into startup. Can be added later inside `main.tsx`, where the router is already imported.
### Artifacts generated
- `construction/u3-security-headers/nfr-design/`: `nfr-design-patterns.md` (11 patterns), `logical-components.md`
- `construction/u4-observability/nfr-design/`: `nfr-design-patterns.md` (10 patterns), `logical-components.md`
- `inception/requirements/requirements.md`: OPEN-01 struck through and closed; open-item count 3 → 2
### Security Compliance (Security Baseline extension — enabled, blocking)
- **SECURITY-03 — compliant, and now mechanised.** The correlation ID arrives by configuration rather than by discipline. EF's `Database.Command` category pinned at `Warning`, because at `Information` it prints parameter values and the login path passes a normalised email through it.
- **SECURITY-11 — compliant.** The tunnel destination is parsed once at startup from the DSN and no part of it can come from the request, which is the single rule separating a tunnel from a server-side request forgery primitive. A rate limiter was added because the other controls bound what each call can do but not how many calls there can be.
- **SECURITY-14 — addressed; DEV-01 unchanged.** Six tagged event types; alert rules configured in Operations.
- **SECURITY-15 — compliant.** Both units fail closed at startup (`ValidateOnStart`, unparseable DSN) and neither throws into the response path.
**No blocking security findings. No new deviation.**
---