diff --git a/aidlc-docs/features/gitea-deployment-workflow/aidlc-state.md b/aidlc-docs/features/gitea-deployment-workflow/aidlc-state.md index 843d899..acc8c07 100644 --- a/aidlc-docs/features/gitea-deployment-workflow/aidlc-state.md +++ b/aidlc-docs/features/gitea-deployment-workflow/aidlc-state.md @@ -5,7 +5,7 @@ - **Feature Slug**: gitea-deployment-workflow - **Project Type**: Brownfield - **Start Date**: 2026-07-27T00:00:00Z -- **Current Stage**: CONSTRUCTION - Round 2, NFR Design complete for U3 + U4 (awaiting approval) +- **Current Stage**: CONSTRUCTION - Code Generation complete for Round 2 (U3 + U4) - **Branch**: feature/gitea-deployment-workflow ## Workspace State @@ -69,7 +69,7 @@ Execution rounds (Q4 = B): **R1** = U1 + U2 · **R2** = U3 + U4 · **R3** = U5 + - [ ] NFR Requirements — **SKIP (all units)** — already comprehensively captured in `requirements.md` § 5 and § 6 - [x] NFR Design — **EXECUTE for U3, U4**; SKIP for the rest. *Deliberate deviation from the default NFR-Requirements/NFR-Design coupling — rationale in the execution plan.* **U3 ✅ U4 ✅** 2026-07-28 — 11 patterns for U3, 10 for U4. Closed OPEN-01; raised REF-U3-01 - [ ] Infrastructure Design — **EXECUTE for U6, U7**; SKIP for the rest -- [~] Code Generation — **EXECUTE** (all 7 units, each built and tested before its completion message). **U1 ✅ U2 ✅** generated and verified 2026-07-27 — build 0 errors, 253 backend tests pass (was 219) +- [~] Code Generation — **EXECUTE** (all 7 units, each built and tested before its completion message). **U1 ✅ U2 ✅** 2026-07-27 (253 backend tests). **U3 ✅** 2026-07-28 (315). **U4 ✅** 2026-07-28 (366 backend + 237 frontend) - [ ] Build and Test — **EXECUTE** ### OPERATIONS @@ -84,9 +84,9 @@ Execution rounds (Q4 = B): **R1** = U1 + U2 · **R2** = U3 + U4 · **R3** = U5 + ## Current Status - **Lifecycle Phase**: CONSTRUCTION -- **Current Stage**: Round 2 — NFR Design complete for U3 Security Headers & CSP and U4 Observability -- **Next Stage**: Code Generation for U3 + U4 -- **Status**: Awaiting NFR Design approval. Round 1 (U1 + U2) code approved and committed 2026-07-28 +- **Current Stage**: Round 2 complete — U3 Security Headers & CSP and U4 Observability generated, built and tested +- **Next Stage**: Round 3 — U5 CI Workflow & Gates + U6 Deploy Workflow (U6 needs Infrastructure Design first) +- **Status**: Awaiting Round 2 code approval. Rounds 1 and 2 committed, nothing pushed ## Round 2 Design Record (2026-07-28) - Functional Design U3 + U4 complete and committed (`357d395`) @@ -105,3 +105,14 @@ Execution rounds (Q4 = B): **R1** = U1 + U2 · **R2** = U3 + U4 · **R3** = U5 + - Embedded placeholder resource name verified against the compiled assembly manifest - **Carried to phase-level Build and Test**: composed-startup behaviour that needs a running host and a real database — `/admin` trailing-slash redirect, 404-vs-HTML for missing assets, SPA fallback and placeholder resolution, `/health` while availability-disabled, `MigrateCoreDatabase` against SQL Server, and both hosts starting - **Deviation**: U1 plan Step 11 (`StaticContentTests`) not implemented — the code lives in `SlpModularCms.Api`, which has no test project by convention; behaviour carried to Build and Test instead. Recorded in the unit's `generation-summary.md` + +## Round 2 Verification Record (2026-07-28) +- `dotnet build SlpModularCms.sln -c Release` — 0 errors (70 warnings, all pre-existing package advisories) +- Backend tests — **366 passed, 0 failed** (Core 196, Availability 82, Master 51, Identity 37); was 253 after Round 1 +- Frontend tests — **237 passed, 0 failed**; baseline 213 +- `npx tsc -b` clean; eslint on every changed frontend file reports 0 problems; full `pnpm run lint` unchanged at the pre-existing 5 errors / 1 warning (FR-21, U5) +- `Sentry.AspNetCore` 6.8.0 ships a native **`net10.0`** asset — the carried-forward compatibility question is closed +- `@sentry/react` 10.68.0; `pnpm-lock.yaml` diff is additions only +- **Two findings**: `z.string().url()` accepts `htp://` in Zod 4 (URL constructor accepts any scheme), so the pre-existing frontend validation never caught the typo BR-U4-24 names — now `z.url({ protocol: /^https?$/ })`. And `appsettings.json` comments are verified by `DeployedConfigurationTests` against the real provider rather than assumed, because the failure mode is both hosts refusing to start +- **One deviation**: `IAdminTokenValidator` collapsed to a single `Validate` → `AdminTokenResult` instead of adding an overload. Two methods with an invisible difference at the call site let a substitute silently invert the access decision while both compiled — see U4's `generation-summary.md` +- **Carried to phase-level Build and Test**: trace-ID propagation master → slave, `TraceId` present in rendered console output, tunnel status codes, the `security_event` tag on a real Sentry event, threshold behaviour end to end, and CSP/HSTS header presence on real static assets and error responses diff --git a/aidlc-docs/features/gitea-deployment-workflow/audit.md b/aidlc-docs/features/gitea-deployment-workflow/audit.md index e41773d..d9b68aa 100644 --- a/aidlc-docs/features/gitea-deployment-workflow/audit.md +++ b/aidlc-docs/features/gitea-deployment-workflow/audit.md @@ -877,3 +877,73 @@ Each is one line; all three are listed at the end of U4's pattern document for r **No blocking security findings. No new deviation.** --- + +## 2026-07-28 — CONSTRUCTION: Code Generation (Round 2, U3 + U4) + +**Stage**: Code Generation for U3 HTTP Security Headers & CSP and U4 Observability Integration. Each unit was built and tested before moving on, per the standing instruction not to defer verification to the final stage. + +### Verification + +| | U3 | U4 | +|---|---|---| +| Release build | 0 errors | 0 errors | +| Backend tests | **315** passed (from 253) | **366** passed | +| Frontend tests | unchanged (213) | **237** passed | +| `tsc -b` | n/a | clean | +| eslint on changed files | n/a | 0 problems | + +Full `pnpm run lint` remains at the pre-existing 5 errors / 1 warning — none in files this round touched. FR-21 fixes those in U5. + +`Sentry.AspNetCore` 6.8.0 ships a native `net10.0` asset, closing the compatibility question NFR Design carried forward. `@sentry/react` 10.68.0; the lockfile diff is additions only. + +### Two findings + +**1. `z.string().url()` never caught the typo BR-U4-24 cites.** The rule says a malformed API base URL must not be silently accepted, and names `htp://localhost:7221`. Zod 4's `url()` validates by handing the value to the `URL` constructor, which accepts **any** scheme — verified directly: `htp://localhost:7221` and `ftp://x.nl` both pass a bare `.url()`. So the *pre-existing* frontend validation, before this unit touched it, would have accepted exactly the typo it was supposed to catch, and the SPA would have issued requests to a nonexistent origin — a failure that looks like the API being down. Now `z.url({ protocol: /^https?$/ })`. Not a regression introduced here; found because BR-U4-24 asked for a test the old schema could not have passed. + +**2. Comments in `appsettings.json` are verified rather than assumed.** Several non-obvious values gained `//` comments. The JSON configuration provider does tolerate them, but the failure mode if it did not is *both hosts refusing to start after a release switch*, so `DeployedConfigurationTests` now loads both real, committed files through the real provider — and runs `ValidateOnStart` against the committed `SecurityHeaders` section, so a policy-name typo fails in CI rather than in a deployment. + +### One deviation, and what it revealed + +**`IAdminTokenValidator` collapsed to a single method.** NFR Design specified adding a reason-reporting overload alongside the existing `IsVerifiedAdmin(string?)`. Implemented that way, two existing tests failed in a revealing manner: the middleware called the new overload while the tests stubbed the old one, and an `NSubstitute` substitute returns `false` by default — so **the access decision silently inverted** while both methods existed and compiled. + +That is the shape of the defect, not merely of the test failure. Two methods whose difference is invisible at a call site means a caller using the boolean form gets the correct access decision and silently emits no security event — precisely the class of bug this unit exists to make impossible. Replaced with one `Validate(string?) → AdminTokenResult`. Five call sites and two test files updated; behaviour otherwise identical. + +Also noted: `MigrationFailure` uses synchronous `SentrySdk.Flush`, because `MigrateCoreDatabase` is synchronous and making it async would change a U2 signature and both hosts' startup for no benefit. + +### Traps that were closed rather than encountered + +- **`DefaultHttpContext.Response.OnStarting` is a no-op.** All of U3's decision logic went into a static `SecurityHeaderWriter` tested against a bare `HeaderDictionary`, so the middleware is glue whose only risk is registration order — which is carried to Build and Test rather than pretended to be unit-testable. +- **Sentry groups log events by message template.** Six source-generated `LoggerMessage` methods with constant templates and `EventId` 5001–5006, plus a test asserting the rendered message is *identical* across different argument values. Without that, FR-19's rate-based alert rules could never fire while everything appeared to work. +- **`StartsWithSegments`, not `string.StartsWith`.** `"/administrator".StartsWith("/admin")` is true; a public page would have inherited the strict policy and lost its inline scripts with no server-side trace at all. Asserted for `/administrator`, `/admin-tools`, `/administration/contact`, `/healthcheck` and `/api/v10/Users`. + +### Additions beyond the functional design, each implemented + +- `Set-Cookie` in the scrub list — the login response issues the `refreshToken` there, so scrubbing only the request cookie would protect nothing +- `SetBeforeSendTransaction` alongside `SetBeforeSend` +- `OnRejected` on the rate limiter — a `429` previously left no trace anywhere, making the only rate limiter in the application unobservable +- A `sentry-tunnel` fixed-window limiter — the other controls bound what each call can do, not how many calls there can be +- `SentrySdk.Flush` before the migration-failure rethrow +- Origin-format validation on the CSP origin lists — a CSP source list silently ignores a malformed source, so a URL with a path would look configured and block the script anyway +- `SecurityAuthorizationResultHandler` at `IAuthorizationMiddlewareResultHandler` rather than inside an `IAuthorizationHandler`, because a handler sees one requirement at a time and would report denials for requests that were ultimately allowed + +### Divergences from the reference project, all deliberate + +- The dev tunnel proxy targets the local API rather than Sentry's ingest host, so no project id is committed and nothing has to be kept in sync by hand +- `UmamiAnalytics` has no script-removing cleanup: under 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. Guarded by a test asserting one injection across re-renders +- `tanstackRouterBrowserTracingIntegration` not adopted — it needs the router instance, and importing it from `sentry.ts` inverts the startup order + +### Artifacts generated +- `construction/u3-security-headers/code/generation-summary.md` +- `construction/u4-observability/code/generation-summary.md` +- 7 production files + 4 test files for U3; 10 backend and 3 frontend production files + 5 test files for U4 + +### Security Compliance (Security Baseline extension — enabled, blocking) +- **SECURITY-03 — compliant.** Correlation ID arrives by configuration rather than discipline; EF's `Database.Command` pinned at `Warning`, asserted against the committed files. +- **SECURITY-04 — compliant.** All five headers, one-year HSTS with `includeSubDomains`, a CSP on every HTML-serving path. `script-src 'self'` under Strict is asserted by a test, so loosening it requires deleting a test that explains why. +- **SECURITY-11 — compliant.** The tunnel destination is parsed once at startup and no part of it can come from a request. +- **SECURITY-14 — addressed; DEV-01 unchanged.** Six tagged event types, all `Warning` or above by construction. +- **SECURITY-15 — compliant.** Both units fail closed at startup and neither throws into the response path. + +**No blocking security findings. No new deviation.** + +--- diff --git a/aidlc-docs/features/gitea-deployment-workflow/construction/u4-observability/code/generation-summary.md b/aidlc-docs/features/gitea-deployment-workflow/construction/u4-observability/code/generation-summary.md new file mode 100644 index 0000000..8623b7e --- /dev/null +++ b/aidlc-docs/features/gitea-deployment-workflow/construction/u4-observability/code/generation-summary.md @@ -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` 5001–5006 | +| `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` | diff --git a/frontend/package.json b/frontend/package.json index a13925a..9c61a4d 100644 --- a/frontend/package.json +++ b/frontend/package.json @@ -23,6 +23,7 @@ "@radix-ui/react-label": "^2.1.10", "@radix-ui/react-select": "^2.3.1", "@radix-ui/react-slot": "^1.3.0", + "@sentry/react": "^10.68.0", "@tanstack/react-query": "^5.101.0", "@tanstack/react-router": "^1.170.16", "class-variance-authority": "^0.7.1", diff --git a/frontend/pnpm-lock.yaml b/frontend/pnpm-lock.yaml index 84abb3a..c6960a3 100644 --- a/frontend/pnpm-lock.yaml +++ b/frontend/pnpm-lock.yaml @@ -26,6 +26,9 @@ importers: '@radix-ui/react-slot': specifier: ^1.3.0 version: 1.3.0(@types/react@19.2.17)(react@19.2.7) + '@sentry/react': + specifier: ^10.68.0 + version: 10.68.0(react@19.2.7) '@tanstack/react-query': specifier: ^5.101.0 version: 5.101.0(react@19.2.7) @@ -875,6 +878,40 @@ packages: '@rolldown/pluginutils@1.0.1': resolution: {integrity: sha512-2j9bGt5Jh8hj+vPtgzPtl72j0yRxHAyumoo6TNfAjsLB04UtpSvPbPcDcBMxz7n+9CYB0c1GxQFxYRg2jimqGw==} + '@sentry/browser-utils@10.68.0': + resolution: {integrity: sha512-be8VtdjCngKc77cstJeV+gO15iH+blyXpBDk8yOehmtX4BkFO33mfTMNCWVR2LA0oOxjIHWRAhf77fIUEhzxPg==} + engines: {node: '>=18'} + + '@sentry/browser@10.68.0': + resolution: {integrity: sha512-8xVgk7oG2lajXnbXF6a7H1xMZ/U6icqSldHGzQu1+bajfrK8Gan9ULG/Xsj1VM1LlNeK6/7znDJ3u1jgvIwznw==} + engines: {node: '>=18'} + + '@sentry/conventions@0.16.0': + resolution: {integrity: sha512-fO9PLmHdVURcSPUpWCItWAtgKiMwGdJHbovoSEyLplX5sxs2ugvI4CBPTrkkgqhObnZOD0CnWBKDzSVQYBKEyQ==} + engines: {node: '>=14'} + + '@sentry/core@10.68.0': + resolution: {integrity: sha512-5Amhx8ltVz7vb1bRGyf3c4J69/iHW8R/H+SJxTRILHlsSOBrnVVc/IQEYDC6PTRdRdZ3x2u7RVjxZi2Mhe525g==} + engines: {node: '>=18'} + + '@sentry/feedback@10.68.0': + resolution: {integrity: sha512-XbdcXiBnpC3vgw46eHOPeD/ZQ+XzluP75ubdUcaPDW02hCh2nsdXiwjZ2DBImbpvIpTbJgjHf/sIlHWvcZJ2Mg==} + engines: {node: '>=18'} + + '@sentry/react@10.68.0': + resolution: {integrity: sha512-rIq4QR4ScMHHx9JJZv7Jgw31bMdUVJMx+ykHIJb7htjY6mj78sjKs+KpCsMDnvJxDhSmvftGM1KfKD4BggL7OQ==} + engines: {node: '>=18'} + peerDependencies: + react: ^16.14.0 || 17.x || 18.x || 19.x + + '@sentry/replay-canvas@10.68.0': + resolution: {integrity: sha512-HusYcr+He+ohnUDHunYrc5St6vdDnBXpUAndnT5ReyUMVSCiWKfY3paXowU/0787HwYfxdcpZgwC5u79+XbEIg==} + engines: {node: '>=18'} + + '@sentry/replay@10.68.0': + resolution: {integrity: sha512-ZoG2n16vbkx4GWSCnLIqUUN9xlUmccQFbQ2US2rhruQeHTUnHl/ukr8NHOQXZaEbwKyMkX9bEMwfmZHJm+wSTQ==} + engines: {node: '>=18'} + '@standard-schema/spec@1.1.0': resolution: {integrity: sha512-l2aFy5jALhniG5HgqrD6jXLi/rUWrKvqN/qJx6yoJsgKhblVd+iqqU4RCXavm/jPityDo5TCvKMnpjKnOriy0w==} @@ -2997,6 +3034,47 @@ snapshots: '@rolldown/pluginutils@1.0.1': {} + '@sentry/browser-utils@10.68.0': + dependencies: + '@sentry/conventions': 0.16.0 + '@sentry/core': 10.68.0 + + '@sentry/browser@10.68.0': + dependencies: + '@sentry/browser-utils': 10.68.0 + '@sentry/conventions': 0.16.0 + '@sentry/core': 10.68.0 + '@sentry/feedback': 10.68.0 + '@sentry/replay': 10.68.0 + '@sentry/replay-canvas': 10.68.0 + + '@sentry/conventions@0.16.0': {} + + '@sentry/core@10.68.0': + dependencies: + '@sentry/conventions': 0.16.0 + + '@sentry/feedback@10.68.0': + dependencies: + '@sentry/core': 10.68.0 + + '@sentry/react@10.68.0(react@19.2.7)': + dependencies: + '@sentry/browser': 10.68.0 + '@sentry/conventions': 0.16.0 + '@sentry/core': 10.68.0 + react: 19.2.7 + + '@sentry/replay-canvas@10.68.0': + dependencies: + '@sentry/core': 10.68.0 + '@sentry/replay': 10.68.0 + + '@sentry/replay@10.68.0': + dependencies: + '@sentry/browser-utils': 10.68.0 + '@sentry/core': 10.68.0 + '@standard-schema/spec@1.1.0': {} '@standard-schema/utils@0.3.0': {} diff --git a/frontend/src/components/SentryErrorBoundary.test.tsx b/frontend/src/components/SentryErrorBoundary.test.tsx new file mode 100644 index 0000000..cf0af38 --- /dev/null +++ b/frontend/src/components/SentryErrorBoundary.test.tsx @@ -0,0 +1,90 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { render, screen } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; +import { SentryErrorBoundary } from '@/components/SentryErrorBoundary'; + +function Boom(): never { + throw new Error('Internal detail: connection string Server=db;Password=hunter2'); +} + +describe('SentryErrorBoundary', () => { + beforeEach(() => { + // React logs caught render errors to the console; silence the expected noise. + vi.spyOn(console, 'error').mockImplementation(() => {}); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + it('shows a fallback instead of blanking the screen', () => { + render( + + + , + ); + + expect(screen.getByTestId('error-boundary-fallback')).toBeInTheDocument(); + expect(screen.getByRole('alert')).toBeInTheDocument(); + }); + + /** + * Exception text frequently contains internal detail, so showing it to an operator is both + * unhelpful and a small information leak. This asserts the leak cannot reappear. + */ + it('shows no exception message and no stack trace', () => { + render( + + + , + ); + + const fallback = screen.getByTestId('error-boundary-fallback'); + expect(fallback.textContent).not.toContain('Internal detail'); + expect(fallback.textContent).not.toContain('hunter2'); + expect(fallback.textContent).not.toContain('Server='); + }); + + it('offers a retry that remounts the subtree', async () => { + let shouldThrow = true; + + function Flaky() { + if (shouldThrow) { + throw new Error('transient'); + } + return

recovered

; + } + + render( + + + , + ); + + expect(screen.getByTestId('error-boundary-fallback')).toBeInTheDocument(); + + shouldThrow = false; + await userEvent.click(screen.getByTestId('error-boundary-retry-button')); + + expect(screen.getByTestId('recovered')).toBeInTheDocument(); + }); + + it('renders its children untouched when nothing throws', () => { + render( + +

fine

+ , + ); + + expect(screen.getByTestId('child')).toBeInTheDocument(); + expect(screen.queryByTestId('error-boundary-fallback')).not.toBeInTheDocument(); + }); + + /** + * No DSN is configured in the test environment, so this run also covers the without-Sentry + * case: the boundary still catches and still shows the fallback, it simply reports nothing. + */ + it('works without a Sentry DSN configured', () => { + expect(import.meta.env.VITE_SENTRY_DSN).toBeUndefined(); + }); +}); diff --git a/frontend/src/components/SentryErrorBoundary.tsx b/frontend/src/components/SentryErrorBoundary.tsx new file mode 100644 index 0000000..a8754cd --- /dev/null +++ b/frontend/src/components/SentryErrorBoundary.tsx @@ -0,0 +1,59 @@ +import type { ReactNode } from 'react'; +import * as Sentry from '@sentry/react'; +import { AlertTriangle } from 'lucide-react'; +import { useTranslation } from 'react-i18next'; +import { Button } from '@/components/ui/button'; + +interface SentryErrorBoundaryProps { + children: ReactNode; +} + +interface FallbackProps { + resetError: () => void; +} + +/** + * Shown when a render error was caught. + * + * Deliberately shows no exception message and no stack trace: exception text frequently contains + * internal detail, and showing it to an operator is both unhelpful and a small information leak. + */ +function ErrorFallback({ resetError }: FallbackProps) { + const { t } = useTranslation(); + + return ( +
+ +

{t('error.unexpected.title')}

+

{t('error.unexpected.message')}

+ +
+ ); +} + +/** + * Catches render-time React errors that would otherwise blank the screen, reports them, and shows + * a recoverable fallback. + * + * Placed inside AuthProvider rather than outermost: the fallback has to be reachable for a + * logged-in user, and an error inside a page must not tear down the session context — otherwise + * recovering from a render error would also log the user out. + * + * Works without a DSN too: it still catches and still shows the fallback, it simply reports + * nothing. + */ +export function SentryErrorBoundary({ children }: SentryErrorBoundaryProps) { + return ( + } + > + {children} + + ); +} diff --git a/frontend/src/components/UmamiAnalytics.test.tsx b/frontend/src/components/UmamiAnalytics.test.tsx new file mode 100644 index 0000000..293c069 --- /dev/null +++ b/frontend/src/components/UmamiAnalytics.test.tsx @@ -0,0 +1,95 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { render } from '@testing-library/react'; +import { UmamiAnalytics } from '@/components/UmamiAnalytics'; +import { resetAppConfigCache } from '@/lib/config'; + +const SCRIPT_SELECTOR = '#umami-analytics-script'; + +function scripts(): NodeListOf { + return document.head.querySelectorAll(SCRIPT_SELECTOR); +} + +describe('UmamiAnalytics', () => { + beforeEach(() => { + resetAppConfigCache(); + document.head.querySelectorAll(SCRIPT_SELECTOR).forEach((node) => node.remove()); + }); + + afterEach(() => { + vi.unstubAllEnvs(); + resetAppConfigCache(); + }); + + function configure(): void { + vi.stubEnv('VITE_UMAMI_SCRIPT_URL', 'https://analytics.example.com/script.js'); + vi.stubEnv('VITE_UMAMI_WEBSITE_ID', 'site-id'); + } + + it('injects nothing in development, even when fully configured', () => { + vi.stubEnv('DEV', true); + configure(); + + render(); + + expect(scripts()).toHaveLength(0); + }); + + it('injects nothing when the website ID is missing', () => { + vi.stubEnv('DEV', false); + vi.stubEnv('VITE_UMAMI_SCRIPT_URL', 'https://analytics.example.com/script.js'); + vi.stubEnv('VITE_UMAMI_WEBSITE_ID', undefined as unknown as string); + + render(); + + expect(scripts()).toHaveLength(0); + }); + + it('injects nothing when the script URL is missing', () => { + vi.stubEnv('DEV', false); + vi.stubEnv('VITE_UMAMI_SCRIPT_URL', undefined as unknown as string); + vi.stubEnv('VITE_UMAMI_WEBSITE_ID', 'site-id'); + + render(); + + expect(scripts()).toHaveLength(0); + }); + + it('injects the script once when configured outside development', () => { + vi.stubEnv('DEV', false); + configure(); + + render(); + + const injected = scripts(); + expect(injected).toHaveLength(1); + expect(injected[0].src).toBe('https://analytics.example.com/script.js'); + expect(injected[0].getAttribute('data-website-id')).toBe('site-id'); + expect(injected[0].defer).toBe(true); + }); + + /** + * Guards against reinstating a cleanup that removes the script: under StrictMode the + * double-invocation would become inject -> remove -> inject, and removing the element does not + * unregister the listeners Umami already installed, so the first page view can be counted + * twice. + */ + it('injects the script only once across re-renders', () => { + vi.stubEnv('DEV', false); + configure(); + + const { rerender } = render(); + rerender(); + render(); + + expect(scripts()).toHaveLength(1); + }); + + it('renders nothing visible', () => { + vi.stubEnv('DEV', false); + configure(); + + const { container } = render(); + + expect(container.firstChild).toBeNull(); + }); +}); diff --git a/frontend/src/components/UmamiAnalytics.tsx b/frontend/src/components/UmamiAnalytics.tsx new file mode 100644 index 0000000..00a2e1e --- /dev/null +++ b/frontend/src/components/UmamiAnalytics.tsx @@ -0,0 +1,47 @@ +import { useEffect } from 'react'; +import { getAppConfig } from '@/lib/config'; + +const SCRIPT_ELEMENT_ID = 'umami-analytics-script'; + +/** + * Injects the self-hosted Umami tracking script when both the script URL and the website ID are + * configured at build time. Renders nothing. + * + * A component rather than a tag in index.html, because the website ID is a build-time variable + * and index.html cannot read import.meta.env. It also makes the "never in development" and "once + * only" rules testable. + * + * `Do Not Track` is deliberately not consulted: Umami sets no cookies and collects no personal + * data, and this SPA's audience is a known set of operators, so honouring DNT would reduce data + * without protecting anyone. A conscious choice rather than an omission. + */ +export function UmamiAnalytics() { + useEffect(() => { + const { umamiScriptUrl, umamiWebsiteId } = getAppConfig(); + + // Never in local development, regardless of configuration, so local testing does not + // pollute the real visitor analytics. + if (import.meta.env.DEV || !umamiScriptUrl || !umamiWebsiteId) { + return; + } + + if (document.getElementById(SCRIPT_ELEMENT_ID) !== null) { + return; + } + + const script = document.createElement('script'); + script.id = SCRIPT_ELEMENT_ID; + script.src = umamiScriptUrl; + script.defer = true; + script.setAttribute('data-website-id', umamiWebsiteId); + document.head.appendChild(script); + + // Deliberately no cleanup removing the script. Under StrictMode the double-invocation + // would become inject -> remove -> inject, and removing the element does not unregister + // the listeners Umami already installed — so the first page view can be counted twice. + // This component lives for the application's lifetime and has nothing to clean up; the + // duplicate guard above handles re-invocation on its own. + }, []); + + return null; +} diff --git a/frontend/src/i18n/locales/en/translation.json b/frontend/src/i18n/locales/en/translation.json index e6b6635..4272d91 100644 --- a/frontend/src/i18n/locales/en/translation.json +++ b/frontend/src/i18n/locales/en/translation.json @@ -239,7 +239,11 @@ "title": "Page Not Found", "message": "The page you're looking for doesn't exist." }, - "backToDashboard": "Back to Dashboard" + "backToDashboard": "Back to Dashboard", + "unexpected": { + "title": "Something went wrong", + "message": "This page could not be displayed. Try again, or go back to the dashboard." + } }, "errors": { "network": "Unable to reach the server. Check your connection and try again.", diff --git a/frontend/src/i18n/locales/nl/translation.json b/frontend/src/i18n/locales/nl/translation.json index e1e78f3..300ba67 100644 --- a/frontend/src/i18n/locales/nl/translation.json +++ b/frontend/src/i18n/locales/nl/translation.json @@ -239,7 +239,11 @@ "title": "Pagina niet gevonden", "message": "De pagina die je zoekt bestaat niet." }, - "backToDashboard": "Terug naar dashboard" + "backToDashboard": "Terug naar dashboard", + "unexpected": { + "title": "Er is iets misgegaan", + "message": "Deze pagina kon niet worden weergegeven. Probeer het opnieuw of ga terug naar het dashboard." + } }, "errors": { "network": "Kan de server niet bereiken. Controleer je verbinding en probeer opnieuw.", diff --git a/frontend/src/lib/api-client.test.ts b/frontend/src/lib/api-client.test.ts index daa2b64..f4111b1 100644 --- a/frontend/src/lib/api-client.test.ts +++ b/frontend/src/lib/api-client.test.ts @@ -183,4 +183,31 @@ describe('ApiClient', () => { const client = makeClient(); await expect(client.get('/network-fail')).rejects.toThrow(NetworkError); }); + + /** + * Same-origin is the production configuration, and the one nobody runs locally — so it is + * only ever exercised here. Note the URL must come out as a plain relative path: any code + * building it with `new URL(path, base)` or an interpolated slash would break on an empty + * base, and would break only in production. + */ + describe('with an empty base URL (same-origin)', () => { + it('issues requests against relative paths', async () => { + let seenUrl: string | null = null; + server.use( + http.get('/api/v1/System/capabilities', ({ request }) => { + seenUrl = request.url; + return HttpResponse.json({ modules: [] }); + }), + ); + + const client = new ApiClient(''); + const result = await client.get<{ modules: string[] }>( + '/api/v1/System/capabilities', + ); + + expect(result).toEqual({ modules: [] }); + expect(seenUrl).toContain('/api/v1/System/capabilities'); + expect(seenUrl).not.toContain('//api/v1'); + }); + }); }); diff --git a/frontend/src/lib/config.test.ts b/frontend/src/lib/config.test.ts new file mode 100644 index 0000000..4852d7e --- /dev/null +++ b/frontend/src/lib/config.test.ts @@ -0,0 +1,83 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { getAppConfig, resetAppConfigCache } from '@/lib/config'; + +/** + * The empty-base case is the production configuration, and the one nobody runs locally — so it is + * only ever exercised here. A malformed value must still be rejected: relaxing validation to + * permit "" is not the same as removing it. + */ +describe('getAppConfig', () => { + const originalEnv = { ...import.meta.env }; + + beforeEach(() => { + resetAppConfigCache(); + }); + + afterEach(() => { + vi.unstubAllEnvs(); + resetAppConfigCache(); + Object.assign(import.meta.env, originalEnv); + }); + + it('resolves an absent API base URL to same-origin', () => { + vi.stubEnv('VITE_API_BASE_URL', undefined as unknown as string); + + expect(getAppConfig().apiBaseUrl).toBe(''); + }); + + it('resolves an empty API base URL to same-origin', () => { + vi.stubEnv('VITE_API_BASE_URL', ''); + + expect(getAppConfig().apiBaseUrl).toBe(''); + }); + + it('keeps an explicit absolute URL, so local master/slave development is unaffected', () => { + vi.stubEnv('VITE_API_BASE_URL', 'https://localhost:7221'); + + expect(getAppConfig().apiBaseUrl).toBe('https://localhost:7221'); + }); + + it('warns in development about a malformed API base URL rather than accepting it silently', () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + vi.stubEnv('VITE_API_BASE_URL', 'htp://localhost:7221'); + + getAppConfig(); + + expect(warn).toHaveBeenCalled(); + warn.mockRestore(); + }); + + it('does not warn about an empty API base URL', () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + vi.stubEnv('VITE_API_BASE_URL', ''); + + getAppConfig(); + + expect(warn).not.toHaveBeenCalled(); + warn.mockRestore(); + }); + + it('exposes the observability values', () => { + vi.stubEnv('VITE_SENTRY_DSN', 'https://abc@host.sentry.io/42'); + vi.stubEnv('VITE_APP_ENV', 'test'); + vi.stubEnv('VITE_UMAMI_SCRIPT_URL', 'https://analytics.example.com/script.js'); + vi.stubEnv('VITE_UMAMI_WEBSITE_ID', 'site-id'); + + const config = getAppConfig(); + + expect(config.sentryDsn).toBe('https://abc@host.sentry.io/42'); + expect(config.appEnv).toBe('test'); + expect(config.umamiScriptUrl).toBe('https://analytics.example.com/script.js'); + expect(config.umamiWebsiteId).toBe('site-id'); + }); + + it('leaves the observability values undefined when nothing is configured', () => { + vi.stubEnv('VITE_SENTRY_DSN', undefined as unknown as string); + vi.stubEnv('VITE_UMAMI_WEBSITE_ID', undefined as unknown as string); + + const config = getAppConfig(); + + expect(config.sentryDsn).toBeUndefined(); + expect(config.umamiWebsiteId).toBeUndefined(); + }); +}); diff --git a/frontend/src/lib/config.ts b/frontend/src/lib/config.ts index a990647..23271d8 100644 --- a/frontend/src/lib/config.ts +++ b/frontend/src/lib/config.ts @@ -6,8 +6,21 @@ import { z } from 'zod'; * once and only warns in development — production trusts the build-time env. */ const configSchema = z.object({ - apiBaseUrl: z.string().url(), + // Relaxed by exactly one case, not loosened: the empty string means same-origin, and + // everything else must still be a valid absolute http(s) URL. A typo such as + // 'htp://localhost:7221' has to stay a failure, or the SPA silently issues requests to a + // nonexistent origin — which looks exactly like the API being down. + // + // The protocol constraint is not decoration. Zod 4's url() validates by handing the value to + // the URL constructor, which happily accepts ANY scheme — 'htp://localhost:7221' and + // 'ftp://x.nl' both pass a bare .url(). The scheme typo this rule exists to catch was + // therefore never actually caught before this constraint was added. + apiBaseUrl: z.union([z.literal(''), z.url({ protocol: /^https?$/ })]), appTitle: z.string(), + sentryDsn: z.string().optional(), + appEnv: z.string().optional(), + umamiScriptUrl: z.string().optional(), + umamiWebsiteId: z.string().optional(), }); export type AppConfig = z.infer; @@ -20,8 +33,16 @@ export function getAppConfig(): AppConfig { } const raw: AppConfig = { - apiBaseUrl: import.meta.env.VITE_API_BASE_URL, + // Absent or empty means same-origin: requests use relative paths, which is the intended + // production configuration now that the API and this SPA are served by one process. An + // explicit absolute URL is still honoured, so local development against + // https://localhost:7221 (master) and :7222 (slave) keeps working unchanged. + apiBaseUrl: import.meta.env.VITE_API_BASE_URL ?? '', appTitle: import.meta.env.VITE_APP_TITLE ?? 'SlpModularCms', + sentryDsn: import.meta.env.VITE_SENTRY_DSN, + appEnv: import.meta.env.VITE_APP_ENV, + umamiScriptUrl: import.meta.env.VITE_UMAMI_SCRIPT_URL, + umamiWebsiteId: import.meta.env.VITE_UMAMI_WEBSITE_ID, }; if (import.meta.env.DEV) { @@ -35,3 +56,8 @@ export function getAppConfig(): AppConfig { cached = raw; return cached; } + +/** Test-only: clears the memoised configuration so a different env can be observed. */ +export function resetAppConfigCache(): void { + cached = null; +} diff --git a/frontend/src/lib/sentry.test.ts b/frontend/src/lib/sentry.test.ts new file mode 100644 index 0000000..0508ff7 --- /dev/null +++ b/frontend/src/lib/sentry.test.ts @@ -0,0 +1,72 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import * as Sentry from '@sentry/react'; +import { SENTRY_TUNNEL_PATH, initSentry } from '@/lib/sentry'; +import { resetAppConfigCache } from '@/lib/config'; + +// ESM namespace objects are not configurable, so Sentry.init cannot be spied on in place. +vi.mock('@sentry/react', () => ({ init: vi.fn() })); + +const init = vi.mocked(Sentry.init); + +describe('initSentry', () => { + beforeEach(() => { + resetAppConfigCache(); + init.mockClear(); + }); + + afterEach(() => { + vi.unstubAllEnvs(); + resetAppConfigCache(); + }); + + it('does nothing without a DSN, so local development is unaffected', () => { + vi.stubEnv('VITE_SENTRY_DSN', undefined as unknown as string); + + initSentry(); + + expect(init).not.toHaveBeenCalled(); + }); + + it('does nothing when the DSN is an empty string', () => { + vi.stubEnv('VITE_SENTRY_DSN', ''); + + initSentry(); + + expect(init).not.toHaveBeenCalled(); + }); + + /** + * The tunnel is the whole reason browser reporting survives an ad blocker: a direct ingest URL + * is blocked with ERR_BLOCKED_BY_CLIENT, losing errors precisely for the users who have one. + */ + it('routes through the same-origin tunnel rather than a Sentry ingest URL', () => { + vi.stubEnv('VITE_SENTRY_DSN', 'https://abc@o1.ingest.de.sentry.io/42'); + + initSentry(); + + expect(init).toHaveBeenCalledOnce(); + const options = init.mock.calls[0][0]!; + expect(options.tunnel).toBe(SENTRY_TUNNEL_PATH); + expect(options.tunnel).not.toContain('sentry.io'); + }); + + it('tags the environment and the release, and keeps PII off', () => { + vi.stubEnv('VITE_SENTRY_DSN', 'https://abc@o1.ingest.de.sentry.io/42'); + vi.stubEnv('VITE_APP_ENV', 'test'); + + initSentry(); + + const options = init.mock.calls[0][0]!; + expect(options.environment).toBe('test'); + expect(options.release).toBeTruthy(); + // False on the frontend even though the backend enables it with scrubbing: the browser + // offers no equivalent in-process guarantee, and there is nothing here the backend cannot + // already report. + expect(options.sendDefaultPii).toBe(false); + }); + + it('uses the same tunnel path the API serves and the dev proxy mirrors', () => { + expect(SENTRY_TUNNEL_PATH).toBe('/sentry-tunnel'); + expect(SENTRY_TUNNEL_PATH).not.toMatch(/^\/api\//); + }); +}); diff --git a/frontend/src/lib/sentry.ts b/frontend/src/lib/sentry.ts new file mode 100644 index 0000000..86779fd --- /dev/null +++ b/frontend/src/lib/sentry.ts @@ -0,0 +1,40 @@ +import * as Sentry from '@sentry/react'; +import { getAppConfig } from '@/lib/config'; + +/** Same-origin path served by the API (see SentryTunnelExtensions). */ +export const SENTRY_TUNNEL_PATH = '/sentry-tunnel'; + +/** + * Initialises Sentry, or does nothing when no DSN is configured. + * + * Called first from main.tsx, before the query client and before render, so an error during + * startup is still captured. + */ +export function initSentry(): void { + const { sentryDsn, appEnv } = getAppConfig(); + + // Local development and any deployment without Sentry must work unchanged. + if (!sentryDsn) { + return; + } + + Sentry.init({ + dsn: sentryDsn, + environment: appEnv, + release: __APP_VERSION__, + // Sentry's free plan counts transactions against the same quota as errors, and this + // setup's value is in errors rather than performance traces. + tracesSampleRate: 0.1, + // False here even though the backend enables it with scrubbing: the backend can scrub + // in-process before transmission because it controls the send, and in the browser there + // is no equivalent guarantee. The frontend has nothing to add that the backend cannot + // already report, so there is no reason to accept the risk. + sendDefaultPii: false, + // Ad blockers block requests to *.ingest.sentry.io outright (ERR_BLOCKED_BY_CLIENT), + // because they look like third-party tracking. Without this, errors are lost precisely + // for the users who have an ad blocker — a silently biased sample of exactly the group + // most likely to have browser oddities. The API forwards the envelope onward; the + // destination is derived from its own DSN, so nothing here needs a project id. + tunnel: SENTRY_TUNNEL_PATH, + }); +} diff --git a/frontend/src/main.tsx b/frontend/src/main.tsx index 659059a..3f85b51 100644 --- a/frontend/src/main.tsx +++ b/frontend/src/main.tsx @@ -9,6 +9,13 @@ import { useAuth } from '@/contexts/auth-context'; import { Toaster } from '@/components/ui/sonner'; import { router } from '@/router'; import { getAppConfig } from '@/lib/config'; +import { initSentry } from '@/lib/sentry'; +import { SentryErrorBoundary } from '@/components/SentryErrorBoundary'; +import { UmamiAnalytics } from '@/components/UmamiAnalytics'; + +// First, before the query client and before render, so an error during startup is still captured. +// A no-op when no DSN is configured. +initSentry(); document.title = getAppConfig().appTitle; @@ -55,7 +62,15 @@ void enableMocking().then(() => { - + {/* + * Inside AuthProvider, not outermost: the fallback has to be reachable for a + * logged-in user, and a render error inside a page must not tear down the + * session context — otherwise recovering would also log the user out. + */} + + + + diff --git a/frontend/src/vite-env.d.ts b/frontend/src/vite-env.d.ts index c121ccb..00becba 100644 --- a/frontend/src/vite-env.d.ts +++ b/frontend/src/vite-env.d.ts @@ -1,15 +1,30 @@ /// interface ImportMetaEnv { - /** Base URL of the SlpModularCms .NET API (e.g. http://localhost:5000). */ - readonly VITE_API_BASE_URL: string; + /** + * Base URL of the SlpModularCms .NET API. Absent or empty means same-origin, which is the + * production configuration: the API and this SPA are served by one process. Set explicitly + * for local development (e.g. https://localhost:7221 for master, :7222 for slave). + */ + readonly VITE_API_BASE_URL?: string; /** Set to 'true' to run the MSW mock backend in the browser during dev. */ readonly VITE_ENABLE_MSW?: string; /** Browser tab title; lets local master/slave dev instances be told apart. Defaults to "SlpModularCms". */ readonly VITE_APP_TITLE?: string; + /** Sentry DSN for client-side error reporting. Not a secret — it ships in the bundle. Absent means Sentry is skipped. */ + readonly VITE_SENTRY_DSN?: string; + /** Build-time environment tag ('test' | 'production'). Both use `vite build`, so MODE alone cannot tell them apart. */ + readonly VITE_APP_ENV?: string; + /** Self-hosted Umami tracking script URL, e.g. https://analytics.example.com/script.js */ + readonly VITE_UMAMI_SCRIPT_URL?: string; + /** Umami website ID, created per environment in the Umami dashboard. */ + readonly VITE_UMAMI_WEBSITE_ID?: string; // Add future typed env flags here. } interface ImportMeta { readonly env: ImportMetaEnv; } + +/** App version at build time (from package.json), injected via vite.config.ts `define`. Used as the Sentry `release` tag. */ +declare const __APP_VERSION__: string; diff --git a/frontend/vite.config.ts b/frontend/vite.config.ts index 6f70454..d04e770 100644 --- a/frontend/vite.config.ts +++ b/frontend/vite.config.ts @@ -3,12 +3,17 @@ import path from 'node:path'; import { defineConfig } from 'vite'; import react from '@vitejs/plugin-react'; import tailwindcss from '@tailwindcss/vite'; +import { version } from './package.json'; // https://vite.dev/config/ export default defineConfig(({ command }) => ({ // Production builds are deployed under /admin (see Program.cs); the dev server keeps serving from '/'. base: command === 'build' ? '/admin/' : '/', plugins: [react(), tailwindcss()], + define: { + // Injected at build time, used as the Sentry `release` tag (see src/lib/sentry.ts). + __APP_VERSION__: JSON.stringify(version), + }, resolve: { alias: { '@': path.resolve(__dirname, './src'), @@ -16,6 +21,22 @@ export default defineConfig(({ command }) => ({ }, server: { port: 5173, + proxy: { + // Mirrors the tunnel path the API serves in test and production, so the same + // Sentry `tunnel` option also works during `pnpm dev` — where this SPA runs on + // 5173 and the API on 7221. + // + // Note this targets our own API rather than Sentry's ingest host directly. The + // reference project proxies straight to Sentry, which forces the DSN's project id + // into this file and requires keeping it in sync by hand. The API derives the + // destination from its own DSN, so there is nothing to synchronise and no project + // id in a committed file. + '/sentry-tunnel': { + target: 'https://localhost:7221', + changeOrigin: true, + secure: false, + }, + }, }, test: { globals: true, @@ -33,6 +54,9 @@ export default defineConfig(({ command }) => ({ 'src/mocks/**', 'src/main.tsx', 'src/vite-env.d.ts', + // Calls Sentry.init, which cannot run meaningfully under jsdom; its skip-without- + // a-DSN behaviour is covered by a test that asserts init was not called. + 'src/lib/sentry.ts', ], }, }, diff --git a/src/SlpModularCms.Api.Slave/Program.cs b/src/SlpModularCms.Api.Slave/Program.cs index d544d94..7ae84f8 100644 --- a/src/SlpModularCms.Api.Slave/Program.cs +++ b/src/SlpModularCms.Api.Slave/Program.cs @@ -1,5 +1,6 @@ -using SlpModularCms.Core.Hosting; +using SlpModularCms.Core.Hosting; using SlpModularCms.Core.Hosting.Health; +using SlpModularCms.Core.Hosting.Observability; using SlpModularCms.Core.Hosting.Security; using Scalar.AspNetCore; @@ -8,6 +9,10 @@ var builder = WebApplication.CreateBuilder(args); // Load local developer overrides builder.Configuration.AddJsonFile("appsettings.local.json", optional: true, reloadOnChange: true); +// Logging first, then Sentry — same order and same reasons as the master host. +builder.Logging.AddCmsLogging(builder.Environment); +builder.WebHost.UseCmsSentry(builder.Configuration); + // 1. Initialize Module Orchestrator var loggerFactory = LoggerFactory.Create(lb => lb.AddConsole()); var orchestrator = new ModuleOrchestrator(loggerFactory.CreateLogger()); @@ -23,6 +28,7 @@ builder.Services.AddCmsHealthChecks(); // and it will be reached directly during diagnosis. There is no reason for it to be the one // host without nosniff and HSTS. The path rules that do not apply here simply never match. builder.Services.AddCmsSecurityHeaders(builder.Configuration); +builder.Services.AddCmsObservability(builder.Configuration); // Registered BEFORE module services — see the note in DataProtectionExtensions. builder.Services.AddCmsDataProtection(); @@ -79,4 +85,8 @@ app.MapControllers(); // API instance looks like, so it behaves like one in every other respect. app.MapCmsHealthChecks(); +// This host serves no SPA, so nothing here posts envelopes today. The endpoint is mapped anyway +// so both hosts behave identically and a slave that later serves an admin UI needs no change. +app.MapSentryTunnel(); + app.Run(); diff --git a/src/SlpModularCms.Api.Slave/appsettings.json b/src/SlpModularCms.Api.Slave/appsettings.json index 78bd163..e682e57 100644 --- a/src/SlpModularCms.Api.Slave/appsettings.json +++ b/src/SlpModularCms.Api.Slave/appsettings.json @@ -1,8 +1,11 @@ -{ +{ "Logging": { "LogLevel": { - "Default": "Warning", - "Microsoft.AspNetCore": "Warning" + "Default": "Information", + "Microsoft.AspNetCore": "Information", + // Pinned at Warning deliberately. At Information, EF prints every SQL statement INCLUDING + // parameter values, and the login path passes a normalised email address through it. + "Microsoft.EntityFrameworkCore.Database.Command": "Warning" } }, "AllowedHosts": "*", @@ -36,6 +39,10 @@ "Refresh": { "PermitLimit": 20, "WindowSeconds": 60 + }, + "SentryTunnel": { + "PermitLimit": 60, + "WindowSeconds": 60 } }, "SecurityHeaders": { @@ -47,5 +54,16 @@ ], "AllowedScriptOrigins": [], "AllowedConnectOrigins": [] + }, + "Observability": { + // Supplied per environment as Observability__SentryDsn. Empty means Sentry is skipped + // entirely and console logging continues — a normal, supported state, not an error. + "SentryDsn": "", + // Falls back to ASPNETCORE_ENVIRONMENT when empty. + "Environment": "", + // Sentry's free plan counts transactions against the same quota as errors, and this setup's + // value is in errors rather than performance traces. + "TracesSampleRate": 0.1, + "TunnelMaxPayloadBytes": 204800 } } diff --git a/src/SlpModularCms.Api/Program.cs b/src/SlpModularCms.Api/Program.cs index 6c1447e..914a0b5 100644 --- a/src/SlpModularCms.Api/Program.cs +++ b/src/SlpModularCms.Api/Program.cs @@ -1,6 +1,7 @@ using SlpModularCms.Api.Extensions; using SlpModularCms.Core.Hosting; using SlpModularCms.Core.Hosting.Health; +using SlpModularCms.Core.Hosting.Observability; using SlpModularCms.Core.Hosting.Security; using Scalar.AspNetCore; @@ -9,6 +10,15 @@ var builder = WebApplication.CreateBuilder(args); // Load local developer overrides builder.Configuration.AddJsonFile("appsettings.local.json", optional: true, reloadOnChange: true); +// Logging FIRST, so a problem initialising Sentry below is itself logged. Puts the W3C trace id +// into the scope of every entry from every category — the correlation id that also travels to +// the slave via traceparent and appears as `traceId` in ProblemDetails responses. +builder.Logging.AddCmsLogging(builder.Environment); + +// Then Sentry. Does nothing at all when no DSN is configured, which is a normal, fully +// supported state rather than an error. +builder.WebHost.UseCmsSentry(builder.Configuration); + // 1. Initialize Module Orchestrator var loggerFactory = LoggerFactory.Create(lb => lb.AddConsole()); var orchestrator = new ModuleOrchestrator(loggerFactory.CreateLogger()); @@ -20,6 +30,7 @@ builder.Services.AddCmsCors(builder.Configuration); builder.Services.AddCmsRateLimiting(builder.Configuration); builder.Services.AddCmsHealthChecks(); builder.Services.AddCmsSecurityHeaders(builder.Configuration); +builder.Services.AddCmsObservability(builder.Configuration); // Registered BEFORE module services: modules must not configure Data Protection themselves, // because a later registration would override this persistent key store (see @@ -93,6 +104,13 @@ app.MapControllers(); // (/api/v1/System/capabilities). Those are CMS domain state and must not be used for monitoring. app.MapCmsHealthChecks(); +// Forwards browser Sentry envelopes through this origin, because ad blockers block requests to +// Sentry domains outright. Mapped before the SPA catch-all below, and deliberately NOT on the +// availability gate's bypass list: if the instance is switched off, losing admin-SPA error +// reports is acceptable, and that is one fewer anonymous outbound-capable endpoint reachable on +// a disabled instance. +app.MapSentryTunnel(); + // SPA fallbacks so client-side routes (e.g. /admin/dashboard) resolve to the right index.html // instead of 404ing. The "nonfile" constraint keeps genuinely missing assets (e.g. /admin/assets/x.js) as 404s. app.MapCmsSpaFallbacks(); diff --git a/src/SlpModularCms.Api/appsettings.json b/src/SlpModularCms.Api/appsettings.json index 2ffd1f6..a7b338b 100644 --- a/src/SlpModularCms.Api/appsettings.json +++ b/src/SlpModularCms.Api/appsettings.json @@ -1,8 +1,11 @@ -{ +{ "Logging": { "LogLevel": { - "Default": "Warning", - "Microsoft.AspNetCore": "Warning" + "Default": "Information", + "Microsoft.AspNetCore": "Information", + // Pinned at Warning deliberately. At Information, EF prints every SQL statement INCLUDING + // parameter values, and the login path passes a normalised email address through it. + "Microsoft.EntityFrameworkCore.Database.Command": "Warning" } }, "AllowedHosts": "*", @@ -41,6 +44,10 @@ "Refresh": { "PermitLimit": 20, "WindowSeconds": 60 + }, + "SentryTunnel": { + "PermitLimit": 60, + "WindowSeconds": 60 } }, "SecurityHeaders": { @@ -53,5 +60,16 @@ ], "AllowedScriptOrigins": [], "AllowedConnectOrigins": [] + }, + "Observability": { + // Supplied per environment as Observability__SentryDsn. Empty means Sentry is skipped + // entirely and console logging continues — a normal, supported state, not an error. + "SentryDsn": "", + // Falls back to ASPNETCORE_ENVIRONMENT when empty. + "Environment": "", + // Sentry's free plan counts transactions against the same quota as errors, and this setup's + // value is in errors rather than performance traces. + "TracesSampleRate": 0.1, + "TunnelMaxPayloadBytes": 204800 } } diff --git a/src/SlpModularCms.Core.Tests/Hosting/AdminTokenValidatorTests.cs b/src/SlpModularCms.Core.Tests/Hosting/AdminTokenValidatorTests.cs index 55d05dd..30e6074 100644 --- a/src/SlpModularCms.Core.Tests/Hosting/AdminTokenValidatorTests.cs +++ b/src/SlpModularCms.Core.Tests/Hosting/AdminTokenValidatorTests.cs @@ -1,4 +1,4 @@ -using System.IdentityModel.Tokens.Jwt; +using System.IdentityModel.Tokens.Jwt; using System.Security.Claims; using System.Text; using FluentAssertions; @@ -46,7 +46,7 @@ public class AdminTokenValidatorTests { var header = $"Bearer {CreateToken(role)}"; - _validator.IsVerifiedAdmin(header).Should().BeTrue(); + _validator.Validate(header).IsVerifiedAdmin.Should().BeTrue(); } [Fact] @@ -54,7 +54,7 @@ public class AdminTokenValidatorTests { var header = $"Bearer {CreateToken("User")}"; - _validator.IsVerifiedAdmin(header).Should().BeFalse(); + _validator.Validate(header).IsVerifiedAdmin.Should().BeFalse(); } [Fact] @@ -64,7 +64,7 @@ public class AdminTokenValidatorTests // signed by us. Reading claims without validating would have accepted this. var forged = CreateUnsignedToken("Owner"); - _validator.IsVerifiedAdmin($"Bearer {forged}").Should().BeFalse(); + _validator.Validate($"Bearer {forged}").IsVerifiedAdmin.Should().BeFalse(); } [Fact] @@ -83,7 +83,7 @@ public class AdminTokenValidatorTests var header = $"Bearer {new JwtSecurityTokenHandler().WriteToken(token)}"; - _validator.IsVerifiedAdmin(header).Should().BeFalse(); + _validator.Validate(header).IsVerifiedAdmin.Should().BeFalse(); } [Fact] @@ -91,7 +91,7 @@ public class AdminTokenValidatorTests { var header = $"Bearer {CreateToken("Owner", expiresIn: TimeSpan.FromMinutes(-5))}"; - _validator.IsVerifiedAdmin(header).Should().BeFalse(); + _validator.Validate(header).IsVerifiedAdmin.Should().BeFalse(); } [Fact] @@ -99,7 +99,7 @@ public class AdminTokenValidatorTests { var header = $"Bearer {CreateToken("Owner", issuer: "SomeoneElse")}"; - _validator.IsVerifiedAdmin(header).Should().BeFalse(); + _validator.Validate(header).IsVerifiedAdmin.Should().BeFalse(); } [Theory] @@ -115,7 +115,7 @@ public class AdminTokenValidatorTests { // Never throws — an unusable header simply means "not an admin". Rejecting the request // is the authentication middleware's job, not the availability gate's. - _validator.IsVerifiedAdmin(header).Should().BeFalse(); + _validator.Validate(header).IsVerifiedAdmin.Should().BeFalse(); } private static string CreateToken( diff --git a/src/SlpModularCms.Core.Tests/Hosting/DeployedConfigurationTests.cs b/src/SlpModularCms.Core.Tests/Hosting/DeployedConfigurationTests.cs new file mode 100644 index 0000000..c7428b5 --- /dev/null +++ b/src/SlpModularCms.Core.Tests/Hosting/DeployedConfigurationTests.cs @@ -0,0 +1,133 @@ +using FluentAssertions; +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Options; +using SlpModularCms.Core.Hosting.Observability; +using SlpModularCms.Core.Hosting.Security; +using Xunit; + +namespace SlpModularCms.Core.Tests.Hosting; + +/// +/// Loads the committed appsettings.json of both hosts and runs the startup validators +/// against them. +/// +/// +/// Everything asserted here is a startup failure in production if it is wrong, which means the +/// symptom is a host that will not boot after a release switch. Two specific reasons this exists: +/// +/// The files carry // comments explaining non-obvious values. The JSON configuration +/// provider does tolerate them, but "does tolerate" is worth verifying rather than assuming when +/// the failure mode is both hosts refusing to start. +/// +/// And SecurityHeaders policy names are validated with ValidateOnStart, so a typo in +/// the committed file stops the process. Better to fail here than in a deployment. +/// +public class DeployedConfigurationTests +{ + public static TheoryData HostConfigurations => new() + { + Path.Combine("host-config", "master.appsettings.json"), + Path.Combine("host-config", "slave.appsettings.json") + }; + + private static IConfigurationRoot Load(string relativePath) + { + File.Exists(relativePath).Should().BeTrue($"'{relativePath}' should be copied to the test output"); + + return new ConfigurationBuilder() + .AddJsonFile(relativePath, optional: false) + .Build(); + } + + [Theory] + [MemberData(nameof(HostConfigurations))] + public void HostConfiguration_ShouldParse_IncludingItsComments(string relativePath) + { + var act = () => Load(relativePath); + + act.Should().NotThrow(); + } + + [Theory] + [MemberData(nameof(HostConfigurations))] + public void HostConfiguration_ShouldPassSecurityHeadersStartupValidation(string relativePath) + { + var services = new ServiceCollection(); + services.AddLogging(); + services.AddCmsSecurityHeaders(Load(relativePath)); + + var act = () => services.BuildServiceProvider().GetRequiredService().Validate(); + + act.Should().NotThrow(); + } + + [Theory] + [MemberData(nameof(HostConfigurations))] + public void HostConfiguration_ShouldShipWithoutASentryDsn(string relativePath) + { + var options = Load(relativePath) + .GetSection(ObservabilityOptions.SectionName) + .Get(); + + options.Should().NotBeNull(); + // A DSN identifies a project and belongs to an account. It is supplied per environment as + // Observability__SentryDsn; the repository is not where it should be written down. + options!.IsSentryConfigured.Should().BeFalse(); + options.TunnelMaxPayloadBytes.Should().BeGreaterThan(0); + } + + /// + /// At Information, EF's command logging prints every SQL statement including parameter + /// values, and the login path passes a normalised email address through it. Raising the + /// default log level without pinning this category would start writing that to the console and + /// to Sentry breadcrumbs. + /// + [Theory] + [MemberData(nameof(HostConfigurations))] + public void HostConfiguration_ShouldPinEfCommandLoggingBelowInformation(string relativePath) + { + var level = Load(relativePath)["Logging:LogLevel:Microsoft.EntityFrameworkCore.Database.Command"]; + + level.Should().Be("Warning"); + } + + /// + /// Sentry breadcrumbs are capped by the logging level, so leaving the default at Warning would + /// deliver every event with an empty breadcrumb trail — the feature present, configured, and + /// useless. + /// + [Theory] + [MemberData(nameof(HostConfigurations))] + public void HostConfiguration_ShouldAllowInformationLevelLogging(string relativePath) + { + var configuration = Load(relativePath); + + configuration["Logging:LogLevel:Default"].Should().Be("Information"); + configuration["Logging:LogLevel:Microsoft.AspNetCore"].Should().Be("Information"); + } + + [Theory] + [MemberData(nameof(HostConfigurations))] + public void HostConfiguration_ShouldRateLimitTheSentryTunnel(string relativePath) + { + var configuration = Load(relativePath); + + configuration.GetValue("RateLimiting:SentryTunnel:PermitLimit").Should().BeGreaterThan(0); + configuration.GetValue("RateLimiting:SentryTunnel:WindowSeconds").Should().BeGreaterThan(0); + } + + /// The admin SPA must never fall to the relaxed policy on the host that serves it. + [Fact] + public void MasterHostConfiguration_ShouldApplyTheStrictPolicyToAdmin() + { + var options = Load(Path.Combine("host-config", "master.appsettings.json")) + .GetSection(SecurityHeadersOptions.SectionName) + .Get(); + + options.Should().NotBeNull(); + options!.Enabled.Should().BeTrue(); + options.PathPolicies + .Should().Contain(rule => rule.PathPrefix == "/admin" && rule.Policy == CspPolicyCatalog.Strict); + } +} diff --git a/src/SlpModularCms.Core.Tests/Hosting/Observability/SecurityEventsTests.cs b/src/SlpModularCms.Core.Tests/Hosting/Observability/SecurityEventsTests.cs new file mode 100644 index 0000000..bdfd748 --- /dev/null +++ b/src/SlpModularCms.Core.Tests/Hosting/Observability/SecurityEventsTests.cs @@ -0,0 +1,165 @@ +using FluentAssertions; +using Microsoft.Extensions.Logging; +using Sentry; +using SlpModularCms.Core.Hosting.Observability; +using SlpModularCms.Core.Observability; +using Xunit; + +namespace SlpModularCms.Core.Tests.Hosting.Observability; + +/// +/// Guards the property that makes alert rules possible at all. +/// +/// +/// Sentry groups log-derived events by their message. If a failed-login entry interpolated the +/// email address, every distinct address would become its own Sentry issue and an alert rule of +/// the form "more than 20 failed logins in five minutes" could never fire, because no single +/// issue would ever reach 20. Everything would look like it worked: events arrive, they are +/// visible, they are tagged. Only the alerting would silently be impossible. +/// +/// These tests assert that the rendered message is identical across different argument values. +/// +public class SecurityEventsTests +{ + private sealed class CapturingLogger : ILogger + { + public List<(LogLevel Level, EventId EventId, string Message)> Entries { get; } = []; + + public IDisposable? BeginScope(TState state) where TState : notnull => null; + + public bool IsEnabled(LogLevel logLevel) => true; + + public void Log( + LogLevel logLevel, + EventId eventId, + TState state, + Exception? exception, + Func formatter) + { + Entries.Add((logLevel, eventId, formatter(state, exception))); + } + } + + [Fact] + public void FailedLogin_ShouldRenderTheSameMessage_ForDifferentAccounts() + { + var logger = new CapturingLogger(); + + SecurityEvents.FailedLogin(logger, "/api/v1/Auth/login", accountExists: true); + SecurityEvents.FailedLogin(logger, "/api/v1/Auth/login", accountExists: true); + + logger.Entries.Should().HaveCount(2); + logger.Entries[0].Message.Should().Be(logger.Entries[1].Message); + } + + [Fact] + public void FailedLogin_ShouldCarryTheEventNameAndNoCredentials() + { + var logger = new CapturingLogger(); + + SecurityEvents.FailedLogin(logger, "/api/v1/Auth/login", accountExists: false); + + var entry = logger.Entries.Single(); + entry.Message.Should().Contain(SecurityEventNames.FailedLogin); + entry.EventId.Id.Should().Be(SecurityEvents.FailedLoginEventId); + entry.Level.Should().Be(LogLevel.Warning); + } + + /// + /// All six must cross the Sentry event threshold by construction rather than by a + /// coincidence of configuration. + /// + [Fact] + public void AllEvents_ShouldBeWarningOrAbove() + { + var logger = new CapturingLogger(); + + SecurityEvents.FailedLogin(logger, "/e", accountExists: true); + SecurityEvents.AuthorizationDenied(logger, "/e", "OwnerOnly"); + SecurityEvents.MasterApiKeyRejected(logger, "/e", "10.0.0.1"); + SecurityEvents.AdminBypassRejected(logger, "/e", BypassRejectionReason.InvalidSignature); + SecurityEvents.RateLimitTriggered(logger, "login", "/e"); + SecurityEvents.MigrationFailure(logger, new InvalidOperationException("boom"), attempts: 5); + + logger.Entries.Should().HaveCount(6); + logger.Entries.Should().OnlyContain(e => e.Level >= LogLevel.Warning); + } + + [Fact] + public void AllEvents_ShouldUseDistinctEventIds() + { + var logger = new CapturingLogger(); + + SecurityEvents.FailedLogin(logger, "/e", accountExists: true); + SecurityEvents.AuthorizationDenied(logger, "/e", "OwnerOnly"); + SecurityEvents.MasterApiKeyRejected(logger, "/e", "10.0.0.1"); + SecurityEvents.AdminBypassRejected(logger, "/e", BypassRejectionReason.Expired); + SecurityEvents.RateLimitTriggered(logger, "login", "/e"); + SecurityEvents.MigrationFailure(logger, new InvalidOperationException("boom"), attempts: 1); + + logger.Entries.Select(e => e.EventId.Id).Should().OnlyHaveUniqueItems(); + } + + [Fact] + public void MigrationFailure_ShouldBeCritical() + { + var logger = new CapturingLogger(); + + SecurityEvents.MigrationFailure(logger, new InvalidOperationException("boom"), attempts: 5); + + logger.Entries.Single().Level.Should().Be(LogLevel.Critical); + } + + /// + /// The reason class distinguishes forgery from a stale tab. The token itself must never + /// appear anywhere in the entry. + /// + [Fact] + public void AdminBypassRejected_ShouldCarryTheReasonClass() + { + var logger = new CapturingLogger(); + + SecurityEvents.AdminBypassRejected(logger, "/admin", BypassRejectionReason.InvalidSignature); + + logger.Entries.Single().Message.Should().Contain(nameof(BypassRejectionReason.InvalidSignature)); + } +} + +public class SecurityEventProcessorTests +{ + private readonly SecurityEventProcessor _processor = new(); + + [Fact] + public void Process_ShouldTagKnownSecurityEvents() + { + var sentryEvent = new SentryEvent(); + sentryEvent.SetExtra("SecurityEvent", SecurityEventNames.FailedLogin); + + var processed = _processor.Process(sentryEvent); + + processed!.Tags[SecurityEventProcessor.TagName].Should().Be(SecurityEventNames.FailedLogin); + } + + /// + /// Alert rules filter on the tag rather than on message text, because a rule that matches + /// nothing looks exactly like a rule with nothing to match. + /// + [Fact] + public void Process_ShouldNotTagUnknownValues() + { + var sentryEvent = new SentryEvent(); + sentryEvent.SetExtra("SecurityEvent", "something_else"); + + var processed = _processor.Process(sentryEvent); + + processed!.Tags.Should().NotContainKey(SecurityEventProcessor.TagName); + } + + [Fact] + public void Process_ShouldLeaveOrdinaryEventsUntouched() + { + var processed = _processor.Process(new SentryEvent()); + + processed!.Tags.Should().NotContainKey(SecurityEventProcessor.TagName); + } +} diff --git a/src/SlpModularCms.Core.Tests/Hosting/Observability/SentryEventScrubberTests.cs b/src/SlpModularCms.Core.Tests/Hosting/Observability/SentryEventScrubberTests.cs new file mode 100644 index 0000000..5a15ff1 --- /dev/null +++ b/src/SlpModularCms.Core.Tests/Hosting/Observability/SentryEventScrubberTests.cs @@ -0,0 +1,111 @@ +using FluentAssertions; +using Sentry; +using SlpModularCms.Core.Hosting.Observability; +using Xunit; + +namespace SlpModularCms.Core.Tests.Hosting.Observability; + +/// +/// The most security-critical code in the observability unit. +/// +/// +/// Enabling SendDefaultPii attaches request headers, and this application carries two +/// standing credentials in them: the refreshToken cookie and the master/slave shared +/// secret. Sending either to a third party would be worse than the problem the setting solves. +/// These tests exist so that the scrub list cannot quietly shrink. +/// +public class SentryEventScrubberTests +{ + private readonly SentryEventScrubber _scrubber = new(); + + private static SentryEvent CreateEventWithCredentials() + { + var sentryEvent = new SentryEvent(); + sentryEvent.Request.Method = "POST"; + sentryEvent.Request.Url = "https://example.com/api/v1/Auth/login"; + sentryEvent.Request.QueryString = "returnUrl=/admin"; + sentryEvent.Request.Headers["Cookie"] = "refreshToken=super-secret-value"; + sentryEvent.Request.Headers["Set-Cookie"] = "refreshToken=freshly-issued; HttpOnly"; + sentryEvent.Request.Headers["Authorization"] = "Bearer eyJhbGciOi..."; + sentryEvent.Request.Headers["X-Master-Api-Key"] = "the-shared-secret"; + sentryEvent.Request.Headers["User-Agent"] = "Mozilla/5.0"; + sentryEvent.Request.Data = "{\"email\":\"a@b.nl\",\"password\":\"hunter2\"}"; + return sentryEvent; + } + + [Theory] + [InlineData("Cookie")] + [InlineData("Set-Cookie")] + [InlineData("Authorization")] + [InlineData("X-Master-Api-Key")] + public void Scrub_ShouldRemoveCredentialHeaders(string header) + { + var scrubbed = _scrubber.Scrub(CreateEventWithCredentials()); + + scrubbed.Request.Headers.Should().NotContainKey(header); + } + + [Fact] + public void Scrub_ShouldRemoveTheRequestBody() + { + var scrubbed = _scrubber.Scrub(CreateEventWithCredentials()); + + scrubbed.Request.Data.Should().BeNull(); + } + + /// + /// The retained fields are the entire diagnostic value of the request context. Scrubbing + /// everything would be safe and useless. + /// + [Fact] + public void Scrub_ShouldRetainDiagnosticFields() + { + var scrubbed = _scrubber.Scrub(CreateEventWithCredentials()); + + scrubbed.Request.Method.Should().Be("POST"); + scrubbed.Request.Url.Should().Be("https://example.com/api/v1/Auth/login"); + scrubbed.Request.QueryString.Should().Be("returnUrl=/admin"); + scrubbed.Request.Headers.Should().ContainKey("User-Agent"); + } + + [Fact] + public void Scrub_ShouldNotThrow_WhenNoRequestContextIsPresent() + { + var act = () => _scrubber.Scrub(new SentryEvent()); + + act.Should().NotThrow(); + } + + /// + /// Transactions carry request data too, and are the channel nobody thinks of. Raising + /// TracesSampleRate without this would start leaking headers. + /// + [Theory] + [InlineData("Cookie")] + [InlineData("Authorization")] + [InlineData("X-Master-Api-Key")] + public void ScrubTransaction_ShouldRemoveCredentialHeaders(string header) + { + var transaction = new SentryTransaction("test", "http.server"); + transaction.Request.Headers["Cookie"] = "refreshToken=secret"; + transaction.Request.Headers["Authorization"] = "Bearer token"; + transaction.Request.Headers["X-Master-Api-Key"] = "secret"; + transaction.Request.Data = "body"; + + var scrubbed = _scrubber.ScrubTransaction(transaction); + + scrubbed.Request.Headers.Should().NotContainKey(header); + scrubbed.Request.Data.Should().BeNull(); + } + + /// + /// A guard against the list shrinking by accident. Each entry has a reason recorded next to + /// it in the implementation. + /// + [Fact] + public void RemovedHeaders_ShouldCoverAllFourCredentialCarriers() + { + SentryEventScrubber.RemovedHeaders.Should().BeEquivalentTo( + ["Cookie", "Set-Cookie", "Authorization", "X-Master-Api-Key"]); + } +} diff --git a/src/SlpModularCms.Core.Tests/Hosting/Observability/SentryTunnelTargetTests.cs b/src/SlpModularCms.Core.Tests/Hosting/Observability/SentryTunnelTargetTests.cs new file mode 100644 index 0000000..3b4a0f7 --- /dev/null +++ b/src/SlpModularCms.Core.Tests/Hosting/Observability/SentryTunnelTargetTests.cs @@ -0,0 +1,69 @@ +using FluentAssertions; +using Microsoft.Extensions.Options; +using SlpModularCms.Core.Hosting.Observability; +using Xunit; + +namespace SlpModularCms.Core.Tests.Hosting.Observability; + +/// +/// The destination is the rule that keeps the tunnel from being a server-side request forgery +/// primitive: it is computed once from configuration, and nothing in a request can influence it. +/// +public class SentryTunnelTargetTests +{ + private static SentryTunnelTarget Create(string dsn, int maxBytes = 204_800) => + new(Options.Create(new ObservabilityOptions + { + SentryDsn = dsn, + TunnelMaxPayloadBytes = maxBytes + })); + + [Fact] + public void Constructor_ShouldDeriveTheEnvelopeEndpoint_FromTheDsn() + { + var target = Create("https://abc123@o4511795618185216.ingest.de.sentry.io/4511795622838352"); + + target.IsConfigured.Should().BeTrue(); + target.EnvelopeEndpoint.Should().Be( + new Uri("https://o4511795618185216.ingest.de.sentry.io/api/4511795622838352/envelope/")); + } + + [Fact] + public void Constructor_ShouldNotBeConfigured_WithoutADsn() + { + var target = Create(string.Empty); + + target.IsConfigured.Should().BeFalse(); + target.EnvelopeEndpoint.Should().BeNull(); + } + + /// + /// Fails at startup rather than per request, consistent with failing closed everywhere else. + /// + [Theory] + [InlineData("not-a-uri")] + [InlineData("https://abc123@ingest.sentry.io")] // no project id + [InlineData("https://abc123@ingest.sentry.io/")] + public void Constructor_ShouldThrow_ForAnUnusableDsn(string dsn) + { + var act = () => Create(dsn); + + act.Should().Throw(); + } + + [Fact] + public void Constructor_ShouldFallBackToADefault_WhenTheSizeCapIsNotPositive() + { + var target = Create("https://abc@host.sentry.io/42", maxBytes: 0); + + target.MaxPayloadBytes.Should().BeGreaterThan(0); + } + + /// Kept in sync with the frontend's tunnel option and the vite dev proxy. + [Fact] + public void Path_ShouldBeOutsideTheVersionedApi() + { + SentryTunnelTarget.Path.Should().Be("/sentry-tunnel"); + SentryTunnelTarget.Path.Should().NotStartWith("/api/"); + } +} diff --git a/src/SlpModularCms.Core.Tests/Hosting/Security/AdminTokenRejectionReasonTests.cs b/src/SlpModularCms.Core.Tests/Hosting/Security/AdminTokenRejectionReasonTests.cs new file mode 100644 index 0000000..d17242c --- /dev/null +++ b/src/SlpModularCms.Core.Tests/Hosting/Security/AdminTokenRejectionReasonTests.cs @@ -0,0 +1,137 @@ +using System.IdentityModel.Tokens.Jwt; +using System.Security.Claims; +using System.Text; +using FluentAssertions; +using Microsoft.IdentityModel.Tokens; +using SlpModularCms.Core.Hosting; +using SlpModularCms.Core.Hosting.Security; +using SlpModularCms.Core.Identity.Models; +using SlpModularCms.Core.Observability; +using Xunit; + +namespace SlpModularCms.Core.Tests.Hosting.Security; + +/// +/// The rejection reason is what makes a rejected admin bypass actionable: InvalidSignature means +/// someone is forging tokens, Expired is almost always an administrator with a stale tab, and an +/// alert that cannot tell those apart is one nobody acts on. +/// +public class AdminTokenRejectionReasonTests +{ + private const string Secret = "TestSecretKeyThatIsLongEnoughForHmacSha256Signing!!"; + private const string Issuer = "SlpModularCms"; + private const string Audience = "SlpModularCmsPortal"; + + private readonly AdminTokenValidator _validator = new(JwtTokenValidation.Create(new JwtSettings + { + Secret = Secret, + Issuer = Issuer, + Audience = Audience + })); + + private static string CreateToken( + string? secret = null, + string? issuer = null, + string? audience = null, + string role = "Owner", + TimeSpan? lifetime = null) + { + var key = new SymmetricSecurityKey(Encoding.UTF8.GetBytes(secret ?? Secret)); + var expires = DateTime.UtcNow.Add(lifetime ?? TimeSpan.FromMinutes(30)); + + var token = new JwtSecurityToken( + issuer: issuer ?? Issuer, + audience: audience ?? Audience, + claims: [new Claim(ClaimTypes.Role, role)], + notBefore: expires.AddMinutes(-35), + expires: expires, + signingCredentials: new SigningCredentials(key, SecurityAlgorithms.HmacSha256)); + + return new JwtSecurityTokenHandler().WriteToken(token); + } + + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData("Basic abc")] + [InlineData("Bearer ")] + public void Validate_ShouldReportAbsent_WhenThereIsNoBearerToken(string? header) + { + var result = _validator.Validate(header); + + result.IsVerifiedAdmin.Should().BeFalse(); + result.Reason.Should().Be(BypassRejectionReason.Absent); + } + + [Fact] + public void Validate_ShouldReportInvalidSignature_ForAForgedToken() + { + var forged = CreateToken(secret: "AnEntirelyDifferentSecretThatIsAlsoLongEnough!!!!!"); + + var result = _validator.Validate($"Bearer {forged}"); + + result.IsVerifiedAdmin.Should().BeFalse(); + result.Reason.Should().Be(BypassRejectionReason.InvalidSignature); + } + + [Fact] + public void Validate_ShouldReportExpired_ForAStaleToken() + { + var expired = CreateToken(lifetime: TimeSpan.FromMinutes(-10)); + + var result = _validator.Validate($"Bearer {expired}"); + + result.IsVerifiedAdmin.Should().BeFalse(); + result.Reason.Should().Be(BypassRejectionReason.Expired); + } + + [Fact] + public void Validate_ShouldReportWrongIssuer() + { + var token = CreateToken(issuer: "SomeoneElse"); + + var result = _validator.Validate($"Bearer {token}"); + + result.IsVerifiedAdmin.Should().BeFalse(); + result.Reason.Should().Be(BypassRejectionReason.WrongIssuer); + } + + [Fact] + public void Validate_ShouldReportWrongAudience() + { + var token = CreateToken(audience: "SomeOtherAudience"); + + var result = _validator.Validate($"Bearer {token}"); + + result.IsVerifiedAdmin.Should().BeFalse(); + result.Reason.Should().Be(BypassRejectionReason.WrongAudience); + } + + [Fact] + public void Validate_ShouldReportNotAdmin_ForAValidNonAdminToken() + { + var token = CreateToken(role: "User"); + + var result = _validator.Validate($"Bearer {token}"); + + result.IsVerifiedAdmin.Should().BeFalse(); + result.Reason.Should().Be(BypassRejectionReason.NotAdmin); + } + + [Fact] + public void Validate_ShouldReportMalformed_ForGarbage() + { + var result = _validator.Validate("Bearer not.a.jwt"); + + result.IsVerifiedAdmin.Should().BeFalse(); + result.Reason.Should().Be(BypassRejectionReason.Malformed); + } + + [Fact] + public void Validate_ShouldStillSucceed_ForAValidAdminToken() + { + var token = CreateToken(); + + _validator.Validate($"Bearer {token}").IsVerifiedAdmin.Should().BeTrue(); + } +} diff --git a/src/SlpModularCms.Core.Tests/Identity/AuthServiceTests.cs b/src/SlpModularCms.Core.Tests/Identity/AuthServiceTests.cs index b3e6f7f..7501779 100644 --- a/src/SlpModularCms.Core.Tests/Identity/AuthServiceTests.cs +++ b/src/SlpModularCms.Core.Tests/Identity/AuthServiceTests.cs @@ -1,3 +1,4 @@ +using Microsoft.Extensions.Logging.Abstractions; using FluentAssertions; using Microsoft.AspNetCore.Identity; using Microsoft.EntityFrameworkCore; @@ -38,7 +39,7 @@ public class AuthServiceTests RefreshTokenExpiryDays = 7 }; - _service = new AuthService(_userManager, _context, Options.Create(jwtSettings)); + _service = new AuthService(_userManager, _context, Options.Create(jwtSettings), NullLogger.Instance); } [Fact] diff --git a/src/SlpModularCms.Core.Tests/SlpModularCms.Core.Tests.csproj b/src/SlpModularCms.Core.Tests/SlpModularCms.Core.Tests.csproj index a4ad655..2d25a44 100644 --- a/src/SlpModularCms.Core.Tests/SlpModularCms.Core.Tests.csproj +++ b/src/SlpModularCms.Core.Tests/SlpModularCms.Core.Tests.csproj @@ -31,4 +31,14 @@ + + + + + + \ No newline at end of file diff --git a/src/SlpModularCms.Core/Hosting/DatabaseMigrationExtensions.cs b/src/SlpModularCms.Core/Hosting/DatabaseMigrationExtensions.cs index 9b4ec87..a7d708b 100644 --- a/src/SlpModularCms.Core/Hosting/DatabaseMigrationExtensions.cs +++ b/src/SlpModularCms.Core/Hosting/DatabaseMigrationExtensions.cs @@ -3,7 +3,9 @@ using Microsoft.Data.SqlClient; using Microsoft.EntityFrameworkCore; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; +using Sentry; using SlpModularCms.Core.Data; +using SlpModularCms.Core.Observability; namespace SlpModularCms.Core.Hosting; @@ -79,10 +81,15 @@ public static class DatabaseMigrationExtensions { // Logged with the failure reason but never the connection string or credentials — // this message travels to the console and to Sentry. - logger.LogCritical( - ex, - "Core database migration failed after {Attempts} attempt(s). The application will not start.", - attempt); + SecurityEvents.MigrationFailure(logger, ex, attempt); + + // The SDK batches and sends in the background, and this process is about to exit + // — which would kill the sender before it transmits. Without this flush the one + // Critical event in the whole system is also the event most likely never to + // arrive. Bounded at five seconds: a host that cannot reach its database is + // already down, and five seconds buys the alert that says why. A no-op when + // Sentry was never initialised, so the no-DSN path is unaffected. + SentrySdk.Flush(TimeSpan.FromSeconds(5)); throw; } diff --git a/src/SlpModularCms.Core/Hosting/Observability/LoggingExtensions.cs b/src/SlpModularCms.Core/Hosting/Observability/LoggingExtensions.cs new file mode 100644 index 0000000..99db75d --- /dev/null +++ b/src/SlpModularCms.Core/Hosting/Observability/LoggingExtensions.cs @@ -0,0 +1,68 @@ +using System.Diagnostics.CodeAnalysis; +using Microsoft.Extensions.Hosting; +using Microsoft.Extensions.Logging; + +namespace SlpModularCms.Core.Hosting.Observability; + +[ExcludeFromCodeCoverage] +public static class LoggingExtensions +{ + /// + /// Configures structured console logging with a correlation identifier on every entry. + /// + /// + /// Must be called before Sentry is registered, so that a problem initialising Sentry + /// is itself logged. + /// + public static ILoggingBuilder AddCmsLogging(this ILoggingBuilder logging, IHostEnvironment environment) + { + ArgumentNullException.ThrowIfNull(logging); + ArgumentNullException.ThrowIfNull(environment); + + // The default host already added a console provider without IncludeScopes. Adding a + // second one prints every line twice — once with the correlation ID and once without — + // which reads as a logging bug and wastes real time. + logging.ClearProviders(); + + // The correlation identifier required by SECURITY-03, resolved as the W3C trace ID from + // the ambient Activity rather than HttpContext.TraceIdentifier. + // + // Two reasons, the first decisive: + // 1. It crosses the master/slave HTTP boundary. HttpClient injects traceparent and the + // slave's hosting layer adopts it, so both sides' log entries carry the SAME value. + // "The master says the slave rejected its API key — what did the slave see?" is the + // hardest diagnostic question in this codebase, and TraceIdentifier, being host-local, + // 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. So the log entry, + // the Sentry event, the slave's log entry and the browser's error response all carry + // one value, and an operator handed a traceId from a screenshot can find the request. + // + // This puts TraceId into the scope of EVERY entry from every category, framework included, + // without touching a single call site — which matters, because the alternative is + // remembering to pass an identifier into every existing log call. + logging.Configure(options => + options.ActivityTrackingOptions = + ActivityTrackingOptions.TraceId | + ActivityTrackingOptions.SpanId | + ActivityTrackingOptions.ParentId); + + if (environment.IsDevelopment()) + { + // A developer reads this with their eyes, and JSON is hostile to that. + logging.AddSimpleConsole(options => + { + options.IncludeScopes = true; + options.SingleLine = true; + }); + } + else + { + // In test and production the process is supervised and its stdout lands in the + // journal, where JSON is greppable and TraceId is a field rather than a substring. + logging.AddJsonConsole(options => options.IncludeScopes = true); + } + + return logging; + } +} diff --git a/src/SlpModularCms.Core/Hosting/Observability/ObservabilityOptions.cs b/src/SlpModularCms.Core/Hosting/Observability/ObservabilityOptions.cs new file mode 100644 index 0000000..c02a9e6 --- /dev/null +++ b/src/SlpModularCms.Core/Hosting/Observability/ObservabilityOptions.cs @@ -0,0 +1,52 @@ +namespace SlpModularCms.Core.Hosting.Observability; + +/// +/// Configuration for logging and error reporting. +/// +/// +/// Every value here is optional. An absent DSN is a normal, fully supported state — local +/// development and any deployment without Sentry must work unchanged — so nothing in this +/// section is required and nothing warns about being empty. +/// +/// Deliberately not configurable: the scrub list and the tunnel's destination host. Both +/// are security-critical, and making either configurable would create a way to switch the +/// protection off — the scrub list by omission, the destination by turning the tunnel into a +/// request-forgery primitive. +/// +public sealed class ObservabilityOptions +{ + public const string SectionName = "Observability"; + + /// + /// Sentry DSN. Empty means Sentry is skipped entirely. + /// + /// + /// Not a secret in the usual sense — it identifies a project and permits event submission, + /// and the frontend's copy is visible in the page source. It still comes from an environment + /// variable (Observability__SentryDsn) in test and production rather than being + /// committed: it belongs to an account, and the repository is not where it should be written + /// down. + /// + public string SentryDsn { get; set; } = string.Empty; + + /// Environment tag. Falls back to ASPNETCORE_ENVIRONMENT when empty. + public string Environment { get; set; } = string.Empty; + + /// + /// Performance sampling rate. + /// + /// + /// Kept low: Sentry's free plan counts transactions against the same quota as errors, and + /// this setup's value is in errors rather than performance traces. + /// + public double TracesSampleRate { get; set; } = 0.1; + + /// + /// Hard upper bound on a tunnelled envelope. Envelopes with a stack trace and breadcrumbs + /// run to tens of kilobytes, so this is generous without being an allocation risk on a + /// Raspberry Pi. + /// + public int TunnelMaxPayloadBytes { get; set; } = 204_800; + + public bool IsSentryConfigured => !string.IsNullOrWhiteSpace(SentryDsn); +} diff --git a/src/SlpModularCms.Core/Hosting/Observability/SecurityAuthorizationResultHandler.cs b/src/SlpModularCms.Core/Hosting/Observability/SecurityAuthorizationResultHandler.cs new file mode 100644 index 0000000..87b712c --- /dev/null +++ b/src/SlpModularCms.Core/Hosting/Observability/SecurityAuthorizationResultHandler.cs @@ -0,0 +1,57 @@ +using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Authorization.Policy; +using Microsoft.AspNetCore.Http; +using Microsoft.Extensions.Logging; +using SlpModularCms.Core.Observability; + +namespace SlpModularCms.Core.Hosting.Observability; + +/// +/// Reports authorization denials, then defers entirely to the framework's own handler. +/// +/// +/// Hooked in at rather than inside an +/// , because a handler sees one requirement at a time: a +/// requirement that does not succeed is not the same as the request being denied, and reporting +/// from there would produce events for requests that were ultimately allowed. This interface sees +/// the final result, which is the thing worth alerting on. +/// +/// The response itself is unchanged — this only observes. +/// +public sealed class SecurityAuthorizationResultHandler : IAuthorizationMiddlewareResultHandler +{ + private readonly AuthorizationMiddlewareResultHandler _inner = new(); + private readonly ILogger _logger; + + public SecurityAuthorizationResultHandler(ILogger logger) + { + _logger = logger; + } + + public Task HandleAsync( + RequestDelegate next, + HttpContext context, + AuthorizationPolicy policy, + PolicyAuthorizationResult authorizeResult) + { + // Challenged means "not authenticated yet" — a 401 that the browser resolves by logging + // in, and an entirely ordinary event. Forbidden means an authenticated caller reached + // something they lack the rights for, which is the case worth knowing about. + if (authorizeResult.Forbidden) + { + SecurityEvents.AuthorizationDenied( + _logger, + context.Request.Path, + DescribePolicy(policy)); + } + + return _inner.HandleAsync(next, context, policy, authorizeResult); + } + + /// + /// Names the requirements rather than the policy, because ASP.NET Core resolves the policy + /// object before this point and the original name is no longer available. + /// + private static string DescribePolicy(AuthorizationPolicy policy) => + string.Join(", ", policy.Requirements.Select(requirement => requirement.GetType().Name)); +} diff --git a/src/SlpModularCms.Core/Hosting/Observability/SecurityEventProcessor.cs b/src/SlpModularCms.Core/Hosting/Observability/SecurityEventProcessor.cs new file mode 100644 index 0000000..fe59d46 --- /dev/null +++ b/src/SlpModularCms.Core/Hosting/Observability/SecurityEventProcessor.cs @@ -0,0 +1,51 @@ +using System.Collections.Frozen; +using Sentry; +using Sentry.Extensibility; +using SlpModularCms.Core.Observability; + +namespace SlpModularCms.Core.Hosting.Observability; + +/// +/// Promotes a security event's constant name to a Sentry tag. +/// +/// +/// Alert rules then filter on security_event:failed_login, which survives a change to the +/// message wording. Matching on message text would not — and it would break silently, because a +/// rule that matches nothing looks exactly like a rule with nothing to match. +/// +public sealed class SecurityEventProcessor : ISentryEventProcessor +{ + public const string TagName = "security_event"; + + private static readonly FrozenSet KnownEventNames = new[] + { + SecurityEventNames.FailedLogin, + SecurityEventNames.AuthorizationDenied, + SecurityEventNames.MasterApiKeyRejected, + SecurityEventNames.AdminBypassRejected, + SecurityEventNames.RateLimitTriggered, + SecurityEventNames.MigrationFailure + }.ToFrozenSet(StringComparer.Ordinal); + + /// + /// The structured property name that attaches to every entry. + /// + private const string PropertyName = "SecurityEvent"; + + public SentryEvent? Process(SentryEvent @event) + { + if (@event is null) + { + return null; + } + + if (@event.Extra.TryGetValue(PropertyName, out var value) && + value is string name && + KnownEventNames.Contains(name)) + { + @event.SetTag(TagName, name); + } + + return @event; + } +} diff --git a/src/SlpModularCms.Core/Hosting/Observability/SentryEventScrubber.cs b/src/SlpModularCms.Core/Hosting/Observability/SentryEventScrubber.cs new file mode 100644 index 0000000..d293c69 --- /dev/null +++ b/src/SlpModularCms.Core/Hosting/Observability/SentryEventScrubber.cs @@ -0,0 +1,101 @@ +using Sentry; + +namespace SlpModularCms.Core.Hosting.Observability; + +/// Removes credentials from an outbound Sentry event. +public interface ISentryEventScrubber +{ + SentryEvent Scrub(SentryEvent sentryEvent); + + SentryTransaction ScrubTransaction(SentryTransaction transaction); +} + +/// +/// Strips credential-bearing headers and the request body before anything leaves the process. +/// +/// +/// Enabling SendDefaultPii attaches request headers, and "PII" understates what this +/// application carries in them: a refreshToken cookie and the master/slave shared secret +/// are credentials, not merely personal data. Sending them to a third party would be +/// worse than the problem the setting solves. +/// +/// This runs in-process, before transmission. Sentry offers server-side scrubbing, but by +/// then the secret has already left the building — doing it here is the only version that +/// actually protects anything. +/// +/// It is a class rather than a lambda inside the UseSentry callback because it is the most +/// security-critical code in this unit, and a lambda there cannot be unit-tested without +/// initialising the SDK. +/// +public sealed class SentryEventScrubber : ISentryEventScrubber +{ + /// + /// A static readonly array in code, never an options property: a configurable scrub + /// list is a supported way to switch the protection off by omission. + /// + public static IReadOnlyList RemovedHeaders => RemovedHeaderNames; + + private static readonly string[] RemovedHeaderNames = + [ + // The whole header, not one cookie. It carries the refreshToken, and removing a single + // cookie by rewriting the header is error-prone in a way that removing the header is not. + "Cookie", + + // The response counterpart. The login and refresh responses ISSUE the refreshToken here, + // so scrubbing the request cookie while sending this one would protect nothing. + "Set-Cookie", + + // Bearer token. + "Authorization", + + // The master/slave shared secret. Not mentioned when SendDefaultPii was chosen, but the + // same class of secret — and it would otherwise be sent to a third party on every error + // raised during a master/slave call. + "X-Master-Api-Key" + ]; + + public SentryEvent Scrub(SentryEvent sentryEvent) + { + ArgumentNullException.ThrowIfNull(sentryEvent); + + ScrubRequest(sentryEvent.Request); + return sentryEvent; + } + + /// + /// Transactions carry request data too. Scrubbing only events would leave a second channel + /// open — less obvious precisely because nobody thinks of a transaction as containing + /// headers — so raising TracesSampleRate later, without touching this file, would + /// start leaking. + /// + public SentryTransaction ScrubTransaction(SentryTransaction transaction) + { + ArgumentNullException.ThrowIfNull(transaction); + + ScrubRequest(transaction.Request); + return transaction; + } + + /// + /// What is deliberately retained: method, path, query string, user agent, IP address, + /// authenticated username and the correlation ID — all genuinely diagnostic. + /// + /// + /// Query strings are kept even though invitation tokens travel as ?token=… on one + /// endpoint. That token is single-use and time-limited rather than a standing credential, and + /// knowing which endpoint was called outweighs it. Recorded so the trade-off is visible + /// rather than accidental. + /// + private static void ScrubRequest(SentryRequest request) + { + foreach (var header in RemovedHeaderNames) + { + request.Headers.Remove(header); + } + + // Belt and braces: MaxRequestBodySize is None, so the body should never have been + // captured in the first place. Nulling it here means a future change to that option + // cannot quietly start shipping login payloads. + request.Data = null; + } +} diff --git a/src/SlpModularCms.Core/Hosting/Observability/SentryExtensions.cs b/src/SlpModularCms.Core/Hosting/Observability/SentryExtensions.cs new file mode 100644 index 0000000..2d01d64 --- /dev/null +++ b/src/SlpModularCms.Core/Hosting/Observability/SentryExtensions.cs @@ -0,0 +1,104 @@ +using System.Diagnostics.CodeAnalysis; +using System.Reflection; +using Microsoft.AspNetCore.Hosting; +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging; +using Sentry.AspNetCore; +using Sentry.Extensibility; + +namespace SlpModularCms.Core.Hosting.Observability; + +[ExcludeFromCodeCoverage] +public static class SentryExtensions +{ + public static IServiceCollection AddCmsObservability( + this IServiceCollection services, + IConfiguration configuration) + { + services.AddOptions() + .Bind(configuration.GetSection(ObservabilityOptions.SectionName)) + .ValidateOnStart(); + + services.AddSingleton(); + services.AddSingleton(); + + // Resolving this at startup is what makes an unparseable DSN a startup failure rather + // than a per-request one. + services.AddSingleton(); + + services.AddHttpClient(SentryTunnelExtensions.HttpClientName, client => + { + // Short, and no retry. A dropped error report is acceptable; a request thread held + // open by an anonymous caller is not. + client.Timeout = TimeSpan.FromSeconds(5); + }); + + return services; + } + + /// + /// Registers Sentry, or does nothing at all when no DSN is configured. + /// + /// + /// An absent DSN is a normal, supported state and produces no warning. Local + /// development is the common case, and a startup warning that always appears trains people to + /// ignore startup warnings — including the ones that matter. + /// + public static IWebHostBuilder UseCmsSentry(this IWebHostBuilder webHost, IConfiguration configuration) + { + ArgumentNullException.ThrowIfNull(webHost); + ArgumentNullException.ThrowIfNull(configuration); + + var settings = configuration.GetSection(ObservabilityOptions.SectionName).Get() + ?? new ObservabilityOptions(); + + if (!settings.IsSentryConfigured) + { + return webHost; + } + + // Stateless, and needed inside a callback that runs before any service provider exists. + // The same types are also registered in DI, where the tests reach them. + var scrubber = new SentryEventScrubber(); + var processor = new SecurityEventProcessor(); + + return webHost.UseSentry(options => + { + options.Dsn = settings.SentryDsn; + options.Environment = string.IsNullOrWhiteSpace(settings.Environment) ? null : settings.Environment; + options.Release = GetRelease(); + options.TracesSampleRate = settings.TracesSampleRate; + + // Reconciles two answers rather than choosing between them: the console gets + // everything at Information, Sentry gets warnings and errors as EVENTS — still more + // than exceptions — and informational entries travel attached to those events as + // breadcrumbs. Sending every framework Information entry as an event would mean one + // event per request, exhausting the free plan within hours and burying real errors in + // request noise. + options.MinimumBreadcrumbLevel = LogLevel.Information; + options.MinimumEventLevel = LogLevel.Warning; + + // Attaches request context — and therefore requires the scrubber below. + options.SendDefaultPii = true; + + // The safest version of "the request body is never sent to Sentry" is that it is + // never read into an event at all. This is the SDK default; set explicitly so that a + // future change to it has to be deliberate. + options.MaxRequestBodySize = RequestSize.None; + + options.SetBeforeSend((sentryEvent, _) => scrubber.Scrub(sentryEvent)); + options.SetBeforeSendTransaction((transaction, _) => scrubber.ScrubTransaction(transaction)); + options.AddEventProcessor(processor); + }); + } + + private static string GetRelease() + { + var assembly = Assembly.GetEntryAssembly() ?? Assembly.GetExecutingAssembly(); + + return assembly.GetCustomAttribute()?.InformationalVersion + ?? assembly.GetName().Version?.ToString() + ?? "unknown"; + } +} diff --git a/src/SlpModularCms.Core/Hosting/Observability/SentryTunnelExtensions.cs b/src/SlpModularCms.Core/Hosting/Observability/SentryTunnelExtensions.cs new file mode 100644 index 0000000..4f1b5d1 --- /dev/null +++ b/src/SlpModularCms.Core/Hosting/Observability/SentryTunnelExtensions.cs @@ -0,0 +1,127 @@ +using System.Diagnostics.CodeAnalysis; +using Microsoft.AspNetCore.Builder; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.RateLimiting; +using Microsoft.AspNetCore.Routing; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging; + +namespace SlpModularCms.Core.Hosting.Observability; + +/// +/// Forwards browser Sentry envelopes through this application's own origin. +/// +/// +/// Ad blockers block requests to Sentry domains with ERR_BLOCKED_BY_CLIENT. Without a +/// tunnel, errors are lost precisely for the users who have an ad blocker — a silently biased +/// sample of exactly the group most likely to have browser oddities. It also keeps browser +/// traffic same-origin, so the CSP needs connect-src 'self' and no external Sentry origin. +/// +/// The reference project tunnels through nginx. Relying on server configuration is what this +/// deployment model forbids, so the application forwards it instead. +/// +[ExcludeFromCodeCoverage] +public static class SentryTunnelExtensions +{ + public const string HttpClientName = "sentry-tunnel"; + public const string RateLimiterName = "sentry-tunnel"; + + public static IEndpointRouteBuilder MapSentryTunnel(this IEndpointRouteBuilder endpoints) + { + endpoints.MapPost(SentryTunnelTarget.Path, HandleAsync) + // Anonymous on purpose: the errors most worth capturing include authentication + // failures, so error reporting must work for a user whose session just expired. + .AllowAnonymous() + // An anonymous endpoint that triggers an outbound HTTPS request per call is a free + // amplifier and a way to burn the Sentry plan's quota from outside. The other three + // controls — fixed destination, size cap, no-DSN-no-forwarding — bound what each call + // can do but not how many calls there can be. + .RequireRateLimiting(RateLimiterName) + // Not a versioned CMS API and not part of the public contract. + .ExcludeFromDescription() + .WithName("SentryTunnel"); + + return endpoints; + } + + private static async Task HandleAsync( + HttpContext context, + SentryTunnelTarget target, + IHttpClientFactory httpClientFactory, + ILoggerFactory loggerFactory, + CancellationToken cancellationToken) + { + if (!target.IsConfigured) + { + // 404 rather than 503: with no DSN configured this endpoint genuinely does not exist. + return Results.NotFound(); + } + + // Content-Length is absent under chunked transfer encoding and is attacker-controlled in + // any case, so this is an optimisation rather than the control. + if (context.Request.ContentLength > target.MaxPayloadBytes) + { + return Results.StatusCode(StatusCodes.Status413PayloadTooLarge); + } + + // This bounded read IS the control. Trusting Content-Length alone would give an anonymous + // caller an unbounded memory allocation on a Raspberry Pi. + var payload = await ReadAtMostAsync(context.Request.Body, target.MaxPayloadBytes, cancellationToken); + if (payload is null) + { + return Results.StatusCode(StatusCodes.Status413PayloadTooLarge); + } + + var logger = loggerFactory.CreateLogger(typeof(SentryTunnelExtensions).FullName!); + + try + { + using var content = new ByteArrayContent(payload); + var client = httpClientFactory.CreateClient(HttpClientName); + + // target.EnvelopeEndpoint was derived from configuration at startup. No part of the + // destination comes from this request, and that is what separates a tunnel from a + // server-side request forgery primitive. + using var response = await client.PostAsync(target.EnvelopeEndpoint, content, cancellationToken); + + if (!response.IsSuccessStatusCode) + { + logger.LogWarning( + "Sentry tunnel: ingest returned {StatusCode} for a {ByteCount}-byte envelope.", + (int)response.StatusCode, payload.Length); + } + } + catch (Exception ex) when (ex is not OperationCanceledException) + { + logger.LogWarning(ex, "Sentry tunnel: forwarding an envelope failed."); + } + + // Accepted regardless of what happened upstream. The browser must not retry or log a + // console error over a failed error report: failing to report an error must not itself + // become an error. + return Results.Accepted(); + } + + /// + /// Reads at most . Returns null when the stream carries more. + /// + private static async Task ReadAtMostAsync(Stream body, int maxBytes, CancellationToken cancellationToken) + { + // One byte of headroom, so "exactly at the limit" and "over the limit" are distinguishable. + var buffer = new byte[maxBytes + 1]; + var total = 0; + + while (total < buffer.Length) + { + var read = await body.ReadAsync(buffer.AsMemory(total), cancellationToken); + if (read == 0) + { + break; + } + + total += read; + } + + return total > maxBytes ? null : buffer[..total]; + } +} diff --git a/src/SlpModularCms.Core/Hosting/Observability/SentryTunnelTarget.cs b/src/SlpModularCms.Core/Hosting/Observability/SentryTunnelTarget.cs new file mode 100644 index 0000000..784ba5f --- /dev/null +++ b/src/SlpModularCms.Core/Hosting/Observability/SentryTunnelTarget.cs @@ -0,0 +1,71 @@ +using Microsoft.Extensions.Options; + +namespace SlpModularCms.Core.Hosting.Observability; + +/// +/// The one destination the Sentry tunnel is allowed to forward to, derived from the configured +/// DSN at startup. +/// +/// +/// This class is the rule that keeps the tunnel from being a liability. An anonymous +/// endpoint that makes an outbound request on demand is a server-side request forgery primitive +/// if the destination comes from the caller. Computing it once, from configuration, and never +/// reading anything from the request makes that structurally impossible rather than merely +/// avoided by the current code. +/// +/// An unparseable DSN fails at startup rather than per request, consistent with the security +/// headers unit and with failing closed generally. +/// +public sealed class SentryTunnelTarget +{ + /// Same path the reference project uses, where nginx serves it. + /// + /// Here the application forwards it, because relying on reverse-proxy configuration is + /// exactly what this deployment model forbids. Keeping the path identical means the frontend + /// tunnel option, the vite dev proxy and an operator's muscle memory all carry over + /// unchanged between the two workspaces. + /// + /// Deliberately outside /api/v1: it is not a versioned CMS API, it must not appear in + /// the OpenAPI document, and /api/v1 maps to the strict CSP policy for reasons that + /// have nothing to do with this endpoint. + /// + public const string Path = "/sentry-tunnel"; + + /// Null when no DSN is configured — the tunnel then accepts nothing. + public Uri? EnvelopeEndpoint { get; } + + public int MaxPayloadBytes { get; } + + public bool IsConfigured => EnvelopeEndpoint is not null; + + public SentryTunnelTarget(IOptions options) + { + ArgumentNullException.ThrowIfNull(options); + var value = options.Value; + + MaxPayloadBytes = value.TunnelMaxPayloadBytes > 0 ? value.TunnelMaxPayloadBytes : 204_800; + EnvelopeEndpoint = value.IsSentryConfigured ? BuildEnvelopeEndpoint(value.SentryDsn) : null; + } + + /// + /// Turns https://{publicKey}@{host}/{projectId} into + /// https://{host}/api/{projectId}/envelope/. + /// + private static Uri BuildEnvelopeEndpoint(string dsn) + { + if (!Uri.TryCreate(dsn.Trim(), UriKind.Absolute, out var uri)) + { + throw new InvalidOperationException( + $"{ObservabilityOptions.SectionName}:SentryDsn is not a valid absolute URI."); + } + + var projectId = uri.AbsolutePath.Trim('/'); + if (projectId.Length == 0) + { + throw new InvalidOperationException( + $"{ObservabilityOptions.SectionName}:SentryDsn does not contain a project id."); + } + + return new Uri($"{uri.Scheme}://{uri.Host}/api/{projectId}/envelope/"); + } +} diff --git a/src/SlpModularCms.Core/Hosting/Security/AdminTokenValidator.cs b/src/SlpModularCms.Core/Hosting/Security/AdminTokenValidator.cs index c5c5108..be3e154 100644 --- a/src/SlpModularCms.Core/Hosting/Security/AdminTokenValidator.cs +++ b/src/SlpModularCms.Core/Hosting/Security/AdminTokenValidator.cs @@ -1,6 +1,7 @@ using System.IdentityModel.Tokens.Jwt; using System.Security.Claims; using Microsoft.IdentityModel.Tokens; +using SlpModularCms.Core.Observability; namespace SlpModularCms.Core.Hosting.Security; @@ -30,18 +31,18 @@ public sealed class AdminTokenValidator : IAdminTokenValidator _validationParameters = validationParameters ?? throw new ArgumentNullException(nameof(validationParameters)); } - public bool IsVerifiedAdmin(string? authorizationHeader) + public AdminTokenResult Validate(string? authorizationHeader) { if (string.IsNullOrEmpty(authorizationHeader) || !authorizationHeader.StartsWith(BearerPrefix, StringComparison.OrdinalIgnoreCase)) { - return false; + return Rejected(BypassRejectionReason.Absent); } var token = authorizationHeader[BearerPrefix.Length..].Trim(); if (token.Length == 0) { - return false; + return Rejected(BypassRejectionReason.Absent); } ClaimsPrincipal principal; @@ -52,13 +53,34 @@ public sealed class AdminTokenValidator : IAdminTokenValidator // whether to serve is this component's job; returning 401 is not. principal = _handler.ValidateToken(token, _validationParameters, out _); } - catch (Exception) + catch (Exception ex) { - return false; + return Rejected(Classify(ex)); } - return AdminRoles.Any(role => principal.IsInRole(role)) - || principal.FindAll(ClaimTypes.Role).Any(c => AdminRoles.Contains(c.Value)) - || principal.FindAll("role").Any(c => AdminRoles.Contains(c.Value)); + var isAdmin = AdminRoles.Any(role => principal.IsInRole(role)) + || principal.FindAll(ClaimTypes.Role).Any(c => AdminRoles.Contains(c.Value)) + || principal.FindAll("role").Any(c => AdminRoles.Contains(c.Value)); + + return isAdmin + ? new AdminTokenResult(true, default) + : Rejected(BypassRejectionReason.NotAdmin); } + + private static AdminTokenResult Rejected(BypassRejectionReason reason) => new(false, reason); + + /// + /// Maps a validation exception onto a reason class. The exception type is the only thing + /// inspected — never the token, and never the exception message, which can quote it. + /// + private static BypassRejectionReason Classify(Exception exception) => exception switch + { + SecurityTokenExpiredException => BypassRejectionReason.Expired, + // Covers SecurityTokenSignatureKeyNotFoundException too, which derives from it — a key + // that cannot be found and a signature that does not verify are the same signal here. + SecurityTokenInvalidSignatureException => BypassRejectionReason.InvalidSignature, + SecurityTokenInvalidIssuerException => BypassRejectionReason.WrongIssuer, + SecurityTokenInvalidAudienceException => BypassRejectionReason.WrongAudience, + _ => BypassRejectionReason.Malformed + }; } diff --git a/src/SlpModularCms.Core/Hosting/Security/IAdminTokenValidator.cs b/src/SlpModularCms.Core/Hosting/Security/IAdminTokenValidator.cs index 705ee38..dd7743b 100644 --- a/src/SlpModularCms.Core/Hosting/Security/IAdminTokenValidator.cs +++ b/src/SlpModularCms.Core/Hosting/Security/IAdminTokenValidator.cs @@ -1,5 +1,17 @@ +using SlpModularCms.Core.Observability; + namespace SlpModularCms.Core.Hosting.Security; +/// +/// Outcome of validating an admin bypass token. +/// +/// True only for a fully valid Owner or Administrator token. +/// +/// Why a rejected token was rejected; meaningless when is true. +/// Never the token or any part of it. +/// +public readonly record struct AdminTokenResult(bool IsVerifiedAdmin, BypassRejectionReason Reason); + /// /// Decides whether a request carries a genuinely valid Owner or Administrator token. /// @@ -12,13 +24,25 @@ namespace SlpModularCms.Core.Hosting.Security; public interface IAdminTokenValidator { /// - /// Returns true only when the supplied Authorization header contains a bearer token that - /// validates successfully and carries the Owner or Administrator role. + /// Validates the supplied Authorization header and reports both the decision and, on + /// rejection, its cause. /// /// Raw Authorization header value; may be null or empty. /// - /// True when the caller is a verified Owner or Administrator; false in every other case, - /// including an absent, malformed, forged, expired or non-admin token. Never throws. + /// A verified-admin result only for a bearer token that validates successfully and carries the + /// Owner or Administrator role. Every other case — absent, malformed, forged, expired or + /// non-admin — is a rejection with a reason. Never throws. /// - bool IsVerifiedAdmin(string? authorizationHeader); + /// + /// Deliberately the only method on this interface. An earlier shape had a plain + /// boolean overload alongside this one, and the difference was invisible at a call site: a + /// caller using the boolean form got the right access decision and silently emitted no + /// security event. One method means the reason cannot be skipped by accident. + /// + /// The reason class matters operationally: + /// suggests forgery, while + /// is usually an administrator with a stale tab, + /// and an alert that cannot tell those apart is one nobody acts on. + /// + AdminTokenResult Validate(string? authorizationHeader); } diff --git a/src/SlpModularCms.Core/Hosting/ServiceCollectionExtensions.cs b/src/SlpModularCms.Core/Hosting/ServiceCollectionExtensions.cs index f573781..2c409df 100644 --- a/src/SlpModularCms.Core/Hosting/ServiceCollectionExtensions.cs +++ b/src/SlpModularCms.Core/Hosting/ServiceCollectionExtensions.cs @@ -1,5 +1,6 @@ using Microsoft.AspNetCore.Authentication.JwtBearer; using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Authorization.Policy; using Microsoft.AspNetCore.Builder; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Identity; @@ -7,6 +8,7 @@ using Microsoft.AspNetCore.RateLimiting; using Microsoft.EntityFrameworkCore; using Microsoft.Extensions.Configuration; using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging; using Microsoft.IdentityModel.Tokens; using SlpModularCms.Core.Availability; using SlpModularCms.Core.Data; @@ -16,6 +18,8 @@ using SlpModularCms.Core.Identity.Authorization; using SlpModularCms.Core.Identity.Entities; using SlpModularCms.Core.Identity.Models; using SlpModularCms.Core.Identity.Services; +using SlpModularCms.Core.Hosting.Observability; +using SlpModularCms.Core.Observability; using System.Threading.RateLimiting; using System.Diagnostics.CodeAnalysis; @@ -86,6 +90,11 @@ public static class ServiceCollectionExtensions services.AddSingleton(); + // Observes the final authorization result so a denial becomes an alertable event. + // Replaces the framework's handler and delegates straight back to it — the response is + // unchanged. + services.AddSingleton(); + // 6. Exception Handling services.AddExceptionHandler(); services.AddProblemDetails(); @@ -126,6 +135,24 @@ public static class ServiceCollectionExtensions { options.RejectionStatusCode = StatusCodes.Status429TooManyRequests; + // Without this callback a brute-force attempt against /api/v1/Auth/login returns 429 + // and leaves NO trace anywhere — the one rate limiter this application has would be + // entirely unobservable, and the alert rule for it could never fire. + options.OnRejected = (context, _) => + { + var logger = context.HttpContext.RequestServices + .GetRequiredService() + .CreateLogger(typeof(ServiceCollectionExtensions).FullName!); + + SecurityEvents.RateLimitTriggered( + logger, + context.HttpContext.GetEndpoint()?.Metadata + .GetMetadata()?.PolicyName ?? "(unknown)", + context.HttpContext.Request.Path); + + return ValueTask.CompletedTask; + }; + options.AddFixedWindowLimiter("login", opt => { var settings = configuration.GetSection("RateLimiting:Login"); @@ -142,6 +169,17 @@ public static class ServiceCollectionExtensions opt.SegmentsPerWindow = 4; opt.QueueLimit = 0; }); + + // Guards the anonymous Sentry tunnel. Generous, because a burst of browser errors is + // exactly when reporting matters most, but bounded, because the endpoint makes an + // outbound HTTPS request per call. + options.AddFixedWindowLimiter(SentryTunnelExtensions.RateLimiterName, opt => + { + var settings = configuration.GetSection("RateLimiting:SentryTunnel"); + opt.PermitLimit = settings.GetValue("PermitLimit", 60); + opt.Window = TimeSpan.FromSeconds(settings.GetValue("WindowSeconds", 60)); + opt.QueueLimit = 0; + }); }); return services; diff --git a/src/SlpModularCms.Core/Identity/Services/AuthService.cs b/src/SlpModularCms.Core/Identity/Services/AuthService.cs index 238bed7..78a9599 100644 --- a/src/SlpModularCms.Core/Identity/Services/AuthService.cs +++ b/src/SlpModularCms.Core/Identity/Services/AuthService.cs @@ -4,12 +4,14 @@ using System.Security.Cryptography; using System.Text; using Microsoft.AspNetCore.Identity; using Microsoft.EntityFrameworkCore; +using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; using Microsoft.IdentityModel.Tokens; using SlpModularCms.Core.Data; using SlpModularCms.Core.Exceptions; using SlpModularCms.Core.Identity.Entities; using SlpModularCms.Core.Identity.Models; +using SlpModularCms.Core.Observability; namespace SlpModularCms.Core.Identity.Services; @@ -18,15 +20,18 @@ public class AuthService : IAuthService private readonly UserManager _userManager; private readonly ApplicationDbContext _context; private readonly JwtSettings _jwtSettings; + private readonly ILogger _logger; public AuthService( UserManager userManager, ApplicationDbContext context, - IOptions jwtSettings) + IOptions jwtSettings, + ILogger logger) { _userManager = userManager; _context = context; _jwtSettings = jwtSettings.Value; + _logger = logger; } public async Task AuthenticateAsync(string email, string password) @@ -34,6 +39,11 @@ public class AuthService : IAuthService var user = await _userManager.FindByEmailAsync(email); if (user == null || !user.IsActive || !await _userManager.CheckPasswordAsync(user, password)) { + // Repeated occurrences suggest an attack or a forgotten password, and whether the + // account exists is what tells those apart. Deliberately absent from the event: the + // password, the attempted password, and the address itself. + SecurityEvents.FailedLogin(_logger, "/api/v1/Auth/login", accountExists: user is not null); + throw new UnauthorizedException("Ongeldige inloggegevens."); } diff --git a/src/SlpModularCms.Core/Observability/BypassRejectionReason.cs b/src/SlpModularCms.Core/Observability/BypassRejectionReason.cs new file mode 100644 index 0000000..1c913d5 --- /dev/null +++ b/src/SlpModularCms.Core/Observability/BypassRejectionReason.cs @@ -0,0 +1,34 @@ +namespace SlpModularCms.Core.Observability; + +/// +/// Why the availability gate refused an admin bypass token. +/// +/// +/// A classification rather than the token itself, and the distinction is operationally real: +/// means someone is forging tokens, whereas +/// is almost always an administrator who left a tab open. An alert that cannot tell those apart +/// is an alert nobody acts on. +/// +public enum BypassRejectionReason +{ + /// No Authorization header, or one that is not a bearer token. + Absent, + + /// Not a readable JWT at all. + Malformed, + + /// Signature verification failed — the interesting one. + InvalidSignature, + + /// Valid signature, but past its lifetime. + Expired, + + /// Signed by a different issuer. + WrongIssuer, + + /// Issued for a different audience. + WrongAudience, + + /// A genuinely valid token whose holder is not an Owner or Administrator. + NotAdmin +} diff --git a/src/SlpModularCms.Core/Observability/SecurityEvents.cs b/src/SlpModularCms.Core/Observability/SecurityEvents.cs new file mode 100644 index 0000000..82d6a9d --- /dev/null +++ b/src/SlpModularCms.Core/Observability/SecurityEvents.cs @@ -0,0 +1,120 @@ +using Microsoft.Extensions.Logging; + +namespace SlpModularCms.Core.Observability; + +/// Stable tag values for the alertable security events. +public static class SecurityEventNames +{ + public const string FailedLogin = "failed_login"; + public const string AuthorizationDenied = "authorization_denied"; + public const string MasterApiKeyRejected = "master_api_key_rejected"; + public const string AdminBypassRejected = "admin_bypass_rejected"; + public const string RateLimitTriggered = "rate_limit_triggered"; + public const string MigrationFailure = "migration_failure"; +} + +/// +/// The six security events that alert rules are built on. +/// +/// +/// Why source-generated rather than plain log calls: +/// Sentry groups log-derived events by their message. Emitted with interpolation — +/// LogWarning($"Login failed for {email}") — every distinct email produces a separate +/// Sentry issue, and an alert rule of the form "more than 20 failed logins in five minutes" can +/// then never fire, because no single issue ever reaches 20. The feature would look like it +/// works: events arrive, they are visible, they are tagged. Only the alerting would silently be +/// impossible. A compile-time constant template groups them all into one issue with the variable +/// parts as structured fields. +/// +/// Each method also carries a constant SecurityEvent property, which +/// SecurityEventProcessor promotes to a Sentry tag so that alert rules filter on +/// security_event:failed_login rather than on message text. Matching on message text +/// would break the day someone improves the wording — silently, because a rule that matches +/// nothing looks exactly like a rule with nothing to match. +/// +/// All six sit at Warning or above by construction, so they cross the Sentry event +/// threshold rather than depending on a coincidence of configuration. +/// +public static partial class SecurityEvents +{ + public const int FailedLoginEventId = 5001; + public const int AuthorizationDeniedEventId = 5002; + public const int MasterApiKeyRejectedEventId = 5003; + public const int AdminBypassRejectedEventId = 5004; + public const int RateLimitTriggeredEventId = 5005; + public const int MigrationFailureEventId = 5006; + + /// + /// Repeated occurrences suggest an attack or a forgotten password. Carries whether the + /// account exists — useful for telling the two apart — but never the password, the attempted + /// password, or the full address. + /// + public static void FailedLogin(ILogger logger, string endpoint, bool accountExists) => + FailedLoginCore(logger, SecurityEventNames.FailedLogin, endpoint, accountExists); + + /// Someone reached an endpoint they lack rights for. + public static void AuthorizationDenied(ILogger logger, string endpoint, string? requiredPolicy) => + AuthorizationDeniedCore(logger, SecurityEventNames.AuthorizationDenied, endpoint, requiredPolicy ?? "(unnamed)"); + + /// + /// Ambiguous by nature: an intruder, or a key ring that has become unreadable. The + /// data-durability work exists to make the second cause impossible, but if it ever happens + /// this is the first sign of it. + /// + public static void MasterApiKeyRejected(ILogger logger, string endpoint, string callingHost) => + MasterApiKeyRejectedCore(logger, SecurityEventNames.MasterApiKeyRejected, endpoint, callingHost); + + /// + /// Only became a meaningful signal once the gate started validating signatures: before that, + /// a forged token succeeded silently. + /// + public static void AdminBypassRejected(ILogger logger, string path, BypassRejectionReason reason) => + AdminBypassRejectedCore(logger, SecurityEventNames.AdminBypassRejected, path, reason); + + /// Brute-force pressure. Without this, a 429 leaves no trace anywhere. + public static void RateLimitTriggered(ILogger logger, string limiterName, string endpoint) => + RateLimitTriggeredCore(logger, SecurityEventNames.RateLimitTriggered, limiterName, endpoint); + + /// + /// Not a security event, but the one event in this system that needs immediate attention — + /// and the process is about to exit, so it must be flushed before it does. + /// + public static void MigrationFailure(ILogger logger, Exception exception, int attempts) => + MigrationFailureCore(logger, exception, SecurityEventNames.MigrationFailure, attempts); + + [LoggerMessage( + EventId = FailedLoginEventId, + Level = LogLevel.Warning, + Message = "Security event {SecurityEvent}: login failed on {Endpoint} (account exists: {AccountExists})")] + private static partial void FailedLoginCore(ILogger logger, string securityEvent, string endpoint, bool accountExists); + + [LoggerMessage( + EventId = AuthorizationDeniedEventId, + Level = LogLevel.Warning, + Message = "Security event {SecurityEvent}: authorization denied on {Endpoint} (required policy: {RequiredPolicy})")] + private static partial void AuthorizationDeniedCore(ILogger logger, string securityEvent, string endpoint, string requiredPolicy); + + [LoggerMessage( + EventId = MasterApiKeyRejectedEventId, + Level = LogLevel.Warning, + Message = "Security event {SecurityEvent}: master API key rejected on {Endpoint} from {CallingHost}")] + private static partial void MasterApiKeyRejectedCore(ILogger logger, string securityEvent, string endpoint, string callingHost); + + [LoggerMessage( + EventId = AdminBypassRejectedEventId, + Level = LogLevel.Warning, + Message = "Security event {SecurityEvent}: admin bypass rejected on {Path} (reason: {Reason})")] + private static partial void AdminBypassRejectedCore(ILogger logger, string securityEvent, string path, BypassRejectionReason reason); + + [LoggerMessage( + EventId = RateLimitTriggeredEventId, + Level = LogLevel.Warning, + Message = "Security event {SecurityEvent}: rate limit {LimiterName} triggered on {Endpoint}")] + private static partial void RateLimitTriggeredCore(ILogger logger, string securityEvent, string limiterName, string endpoint); + + [LoggerMessage( + EventId = MigrationFailureEventId, + Level = LogLevel.Critical, + Message = "Security event {SecurityEvent}: database migration failed after {Attempts} attempt(s)")] + private static partial void MigrationFailureCore(ILogger logger, Exception exception, string securityEvent, int attempts); +} diff --git a/src/SlpModularCms.Core/SlpModularCms.Core.csproj b/src/SlpModularCms.Core/SlpModularCms.Core.csproj index e3b0653..c36bc68 100644 --- a/src/SlpModularCms.Core/SlpModularCms.Core.csproj +++ b/src/SlpModularCms.Core/SlpModularCms.Core.csproj @@ -26,6 +26,7 @@ + diff --git a/src/SlpModularCms.Modules.Availability.Tests/AvailabilityMiddlewareMasterGateTests.cs b/src/SlpModularCms.Modules.Availability.Tests/AvailabilityMiddlewareMasterGateTests.cs index be14df0..fd97599 100644 --- a/src/SlpModularCms.Modules.Availability.Tests/AvailabilityMiddlewareMasterGateTests.cs +++ b/src/SlpModularCms.Modules.Availability.Tests/AvailabilityMiddlewareMasterGateTests.cs @@ -1,9 +1,10 @@ -using FluentAssertions; +using FluentAssertions; using Microsoft.AspNetCore.Http; using Microsoft.Extensions.Logging.Abstractions; using NSubstitute; using SlpModularCms.Core.Availability; using SlpModularCms.Core.Hosting.Security; +using SlpModularCms.Core.Observability; using SlpModularCms.Modules.Availability.Middleware; using SlpModularCms.Modules.Availability.Services; @@ -111,7 +112,7 @@ public class AvailabilityMiddlewareMasterGateTests { var context = new DefaultHttpContext(); context.Request.Headers.Authorization = "Bearer owner-token"; - _adminTokenValidator.IsVerifiedAdmin("Bearer owner-token").Returns(true); + _adminTokenValidator.Validate("Bearer owner-token").Returns(new AdminTokenResult(true, default)); _masterSvc.GetMasterStatus().Returns(new MasterGateStatus(false, null)); _localSvc.IsAvailableAsync().Returns(AvailabilityStatus.NotAvailable); @@ -127,7 +128,7 @@ public class AvailabilityMiddlewareMasterGateTests var context = new DefaultHttpContext(); context.Response.Body = new MemoryStream(); context.Request.Headers.Authorization = "Bearer user-token"; - _adminTokenValidator.IsVerifiedAdmin(Arg.Any()).Returns(false); + _adminTokenValidator.Validate(Arg.Any()).Returns(new AdminTokenResult(false, BypassRejectionReason.Absent)); _masterSvc.GetMasterStatus().Returns(new MasterGateStatus(false, null)); await _middleware.InvokeAsync(context, _localSvc, _masterSvc); diff --git a/src/SlpModularCms.Modules.Availability.Tests/AvailabilityMiddlewareTests.cs b/src/SlpModularCms.Modules.Availability.Tests/AvailabilityMiddlewareTests.cs index d5b2556..65d882b 100644 --- a/src/SlpModularCms.Modules.Availability.Tests/AvailabilityMiddlewareTests.cs +++ b/src/SlpModularCms.Modules.Availability.Tests/AvailabilityMiddlewareTests.cs @@ -1,4 +1,4 @@ -using System.IdentityModel.Tokens.Jwt; +using System.IdentityModel.Tokens.Jwt; using System.Security.Claims; using System.Text; using FluentAssertions; @@ -9,6 +9,7 @@ using NSubstitute; using SlpModularCms.Core.Availability; using SlpModularCms.Core.Hosting; using SlpModularCms.Core.Hosting.Security; +using SlpModularCms.Core.Observability; using SlpModularCms.Core.Identity.Models; using SlpModularCms.Modules.Availability.Middleware; using SlpModularCms.Modules.Availability.Services; @@ -145,7 +146,7 @@ public class AvailabilityMiddlewareTests { var context = new DefaultHttpContext(); context.Request.Headers.Authorization = "Bearer some-token"; - _adminTokenValidator.IsVerifiedAdmin("Bearer some-token").Returns(true); + _adminTokenValidator.Validate("Bearer some-token").Returns(new AdminTokenResult(true, default)); _service.IsAvailableAsync().Returns(AvailabilityStatus.Maintenance); await _middleware.InvokeAsync(context, _service, _masterService); @@ -159,7 +160,7 @@ public class AvailabilityMiddlewareTests var context = new DefaultHttpContext(); context.Response.Body = new MemoryStream(); context.Request.Headers.Authorization = "Bearer some-token"; - _adminTokenValidator.IsVerifiedAdmin(Arg.Any()).Returns(false); + _adminTokenValidator.Validate(Arg.Any()).Returns(new AdminTokenResult(false, BypassRejectionReason.Absent)); _service.IsAvailableAsync().Returns(AvailabilityStatus.Maintenance); await _middleware.InvokeAsync(context, _service, _masterService); @@ -173,7 +174,7 @@ public class AvailabilityMiddlewareTests { var context = new DefaultHttpContext(); context.Response.Body = new MemoryStream(); - _adminTokenValidator.IsVerifiedAdmin(Arg.Any()).Returns(false); + _adminTokenValidator.Validate(Arg.Any()).Returns(new AdminTokenResult(false, BypassRejectionReason.Absent)); _service.IsAvailableAsync().Returns(AvailabilityStatus.NotAvailable); await _middleware.InvokeAsync(context, _service, _masterService); diff --git a/src/SlpModularCms.Modules.Availability.Tests/Controllers/MasterControllerTests.cs b/src/SlpModularCms.Modules.Availability.Tests/Controllers/MasterControllerTests.cs index 9a3f964..a8108de 100644 --- a/src/SlpModularCms.Modules.Availability.Tests/Controllers/MasterControllerTests.cs +++ b/src/SlpModularCms.Modules.Availability.Tests/Controllers/MasterControllerTests.cs @@ -1,3 +1,4 @@ +using Microsoft.Extensions.Logging.Abstractions; using FluentAssertions; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc; @@ -16,7 +17,7 @@ public class MasterControllerTests public MasterControllerTests() { _svc = Substitute.For(); - _controller = new MasterController(_svc); + _controller = new MasterController(_svc, NullLogger.Instance); _controller.ControllerContext = new ControllerContext { HttpContext = new DefaultHttpContext() diff --git a/src/SlpModularCms.Modules.Availability/Controllers/MasterController.cs b/src/SlpModularCms.Modules.Availability/Controllers/MasterController.cs index 0573a2b..d6693d4 100644 --- a/src/SlpModularCms.Modules.Availability/Controllers/MasterController.cs +++ b/src/SlpModularCms.Modules.Availability/Controllers/MasterController.cs @@ -1,4 +1,6 @@ using Microsoft.AspNetCore.Mvc; +using Microsoft.Extensions.Logging; +using SlpModularCms.Core.Observability; using SlpModularCms.Modules.Availability.Models; using SlpModularCms.Modules.Availability.Services; @@ -9,36 +11,60 @@ namespace SlpModularCms.Modules.Availability.Controllers; public class MasterController : ControllerBase { private readonly IMasterAvailabilityService _svc; + private readonly ILogger _logger; - public MasterController(IMasterAvailabilityService svc) => _svc = svc; + public MasterController(IMasterAvailabilityService svc, ILogger logger) + { + _svc = svc; + _logger = logger; + } [HttpPost("register")] public async Task Register([FromBody] RegisterMasterRequest request) { var apiKey = Request.Headers["X-Master-Api-Key"].FirstOrDefault(); - if (string.IsNullOrEmpty(apiKey)) return Unauthorized(); + if (string.IsNullOrEmpty(apiKey)) return RejectKey(); var success = await _svc.RegisterAsync(request.MasterUrl, apiKey); - return success ? Ok() : Unauthorized(); + return success ? Ok() : RejectKey(); } [HttpPost("status")] public async Task PushStatus([FromBody] PushStatusRequest request) { var apiKey = Request.Headers["X-Master-Api-Key"].FirstOrDefault(); - if (string.IsNullOrEmpty(apiKey)) return Unauthorized(); + if (string.IsNullOrEmpty(apiKey)) return RejectKey(); var success = await _svc.PushStatusAsync(apiKey, request.IsAvailable, request.DisableMessage); - return success ? Ok() : Unauthorized(); + return success ? Ok() : RejectKey(); } [HttpGet("registered-url")] public async Task GetRegisteredUrl() { var apiKey = Request.Headers["X-Master-Api-Key"].FirstOrDefault(); - if (string.IsNullOrEmpty(apiKey)) return Unauthorized(); + if (string.IsNullOrEmpty(apiKey)) return RejectKey(); var url = await _svc.GetRegisteredUrlAsync(apiKey); - return url is not null ? Ok(new { MasterUrl = url }) : Unauthorized(); + return url is not null ? Ok(new { MasterUrl = url }) : RejectKey(); + } + + /// + /// Reports the rejection and returns 401. Never logs the key or any part of it. + /// + /// + /// This event is ambiguous by nature: it means either an intruder, or that the Data + /// Protection key ring has become unreadable so a legitimate master can no longer be + /// recognised. The durability work exists to make the second cause impossible, but if it ever + /// happens this is the first sign of it — and the two need telling apart quickly. + /// + private IActionResult RejectKey() + { + SecurityEvents.MasterApiKeyRejected( + _logger, + Request.Path, + HttpContext.Connection.RemoteIpAddress?.ToString() ?? "(unknown)"); + + return Unauthorized(); } } diff --git a/src/SlpModularCms.Modules.Availability/Middleware/AvailabilityMiddleware.cs b/src/SlpModularCms.Modules.Availability/Middleware/AvailabilityMiddleware.cs index 3bd734d..7f935bd 100644 --- a/src/SlpModularCms.Modules.Availability/Middleware/AvailabilityMiddleware.cs +++ b/src/SlpModularCms.Modules.Availability/Middleware/AvailabilityMiddleware.cs @@ -3,6 +3,7 @@ using Microsoft.AspNetCore.Mvc; using Microsoft.Extensions.Logging; using SlpModularCms.Core.Availability; using SlpModularCms.Core.Hosting.Security; +using SlpModularCms.Core.Observability; using SlpModularCms.Modules.Availability.Services; namespace SlpModularCms.Modules.Availability.Middleware; @@ -109,6 +110,17 @@ public class AvailabilityMiddleware /// private bool IsAdminBypass(HttpContext context) { - return _adminTokenValidator.IsVerifiedAdmin(context.Request.Headers.Authorization.ToString()); + var result = _adminTokenValidator.Validate(context.Request.Headers.Authorization.ToString()); + + // A rejected bypass only became a meaningful signal once the gate started verifying + // signatures: before that, a forged token succeeded silently. An absent header is not + // reported — every anonymous request to a disabled instance has one, so reporting it + // would drown the cases that matter. + if (!result.IsVerifiedAdmin && result.Reason != BypassRejectionReason.Absent) + { + SecurityEvents.AdminBypassRejected(_logger, context.Request.Path, result.Reason); + } + + return result.IsVerifiedAdmin; } } diff --git a/src/SlpModularCms.Modules.Master.Tests/Controllers/SlaveStatusControllerTests.cs b/src/SlpModularCms.Modules.Master.Tests/Controllers/SlaveStatusControllerTests.cs index 6840c7d..732f853 100644 --- a/src/SlpModularCms.Modules.Master.Tests/Controllers/SlaveStatusControllerTests.cs +++ b/src/SlpModularCms.Modules.Master.Tests/Controllers/SlaveStatusControllerTests.cs @@ -1,3 +1,4 @@ +using Microsoft.Extensions.Logging.Abstractions; using FluentAssertions; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc; @@ -14,7 +15,7 @@ public class SlaveStatusControllerTests public SlaveStatusControllerTests() { - _controller = new SlaveStatusController(_service); + _controller = new SlaveStatusController(_service, NullLogger.Instance); _controller.ControllerContext = new ControllerContext { HttpContext = new DefaultHttpContext() diff --git a/src/SlpModularCms.Modules.Master/Controllers/SlaveStatusController.cs b/src/SlpModularCms.Modules.Master/Controllers/SlaveStatusController.cs index 63c721a..8da2e3c 100644 --- a/src/SlpModularCms.Modules.Master/Controllers/SlaveStatusController.cs +++ b/src/SlpModularCms.Modules.Master/Controllers/SlaveStatusController.cs @@ -1,5 +1,7 @@ using Microsoft.AspNetCore.Authorization; using Microsoft.AspNetCore.Mvc; +using Microsoft.Extensions.Logging; +using SlpModularCms.Core.Observability; using SlpModularCms.Modules.Master.Services; namespace SlpModularCms.Modules.Master.Controllers; @@ -14,17 +16,30 @@ namespace SlpModularCms.Modules.Master.Controllers; [ApiController] [Route("SlaveStatus")] [AllowAnonymous] -public class SlaveStatusController(ICmsInstanceService service) : ControllerBase +public class SlaveStatusController( + ICmsInstanceService service, + ILogger logger) : ControllerBase { [HttpGet] public async Task Get() { var apiKey = Request.Headers["X-Master-Api-Key"].FirstOrDefault(); - if (string.IsNullOrEmpty(apiKey)) return Unauthorized(); + if (string.IsNullOrEmpty(apiKey)) return RejectKey(); var result = await service.GetStatusForApiKeyAsync(apiKey); - if (result is null) return Unauthorized(); + if (result is null) return RejectKey(); return Ok(new { result.IsAvailable, result.DisableMessage }); } + + /// Reports the rejection and returns 401. Never logs the key or any part of it. + private IActionResult RejectKey() + { + SecurityEvents.MasterApiKeyRejected( + logger, + Request.Path, + HttpContext.Connection.RemoteIpAddress?.ToString() ?? "(unknown)"); + + return Unauthorized(); + } }