diff --git a/code-reviews/AuditLog/findings.md b/code-reviews/AuditLog/findings.md index de0c30ac..e7ab7f8c 100644 --- a/code-reviews/AuditLog/findings.md +++ b/code-reviews/AuditLog/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-28 | | Reviewer | claude-agent | | Commit reviewed | `1eb6e97` | -| Open findings | 2 | +| Open findings | 1 | ## Summary @@ -411,9 +411,22 @@ resolution instead. |--|--| | Severity | Low | | Category | Security | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.AuditLog/Site/FallbackAuditWriter.cs:51-77`, `src/ScadaLink.AuditLog/Central/CentralAuditWriter.cs:77-104`, `src/ScadaLink.AuditLog/Central/AuditLogIngestActor.cs:125,155` | +**Resolution (2026-05-28):** New `SafeDefaultAuditPayloadFilter` in +`src/ScadaLink.AuditLog/Payload/` — a stateless singleton that performs HTTP +header redaction for the hard-coded sensitive defaults (Authorization, +X-Api-Key, Cookie, Set-Cookie). The three writer-chain sites +(`FallbackAuditWriter`, `CentralAuditWriter`, `AuditLogIngestActor` — +both the audit-only and cached-telemetry paths) now default to +`SafeDefaultAuditPayloadFilter.Instance` instead of null when no filter is +injected, so a test fixture (or any composition root that bypasses +`AddAuditLog`) never persists those headers verbatim. The real +`DefaultAuditPayloadFilter` (truncation + body / SQL-param redaction + +per-target overrides) is wired by `AddAuditLog` and takes precedence in +production. + **Description** `FallbackAuditWriter`, `CentralAuditWriter`, and `AuditLogIngestActor` all accept an diff --git a/code-reviews/ConfigurationDatabase/findings.md b/code-reviews/ConfigurationDatabase/findings.md index d0e6626f..25de57f0 100644 --- a/code-reviews/ConfigurationDatabase/findings.md +++ b/code-reviews/ConfigurationDatabase/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-28 | | Reviewer | claude-agent | | Commit reviewed | `1eb6e97` | -| Open findings | 3 | +| Open findings | 1 | ## Summary @@ -946,9 +946,22 @@ throws and exactly one row lands. |--|--| | Severity | Medium | | Category | Security | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.ConfigurationDatabase/Repositories/InboundApiRepository.cs:35-39` | +**Resolution (2026-05-28):** Took option (a) — `InboundApiRepository` ctor now +accepts `Func? hasherAccessor = null` (deferred resolution to +sidestep startup-time pepper-validation in test composition roots that don't +configure one). `GetApiKeyByValueAsync` calls `_hasherAccessor()` so the +hash matches the production peppered digest. `AddConfigurationDatabase` +registers the repository with a factory wiring +`() => sp.GetService() ?? ApiKeyHasher.Default` — the +peppered hasher is used when available, falling back to Default only when +none is registered (legacy / test composition). Regression test +`CD016_GetApiKeyByValue_UsesInjectedPepperedHasher_NotDefault` asserts +positively (peppered roundtrip) and negatively (Default hasher misses the +key entirely, proving the lookup uses the injected hasher). + **Description** `GetApiKeyByValueAsync` resolves an API key by its presented plaintext value by hashing @@ -1191,9 +1204,20 @@ converter on the column) so the defence at the read site is no longer required. |--|--| | Severity | Low | | Category | Security | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.ConfigurationDatabase/Repositories/AuditLogRepository.cs:192-338` | +**Resolution (2026-05-28):** Took the targeted (1) part of the recommendation — +the `monthBoundary` format string is now `"yyyy-MM-dd HH:mm:ss.fffffff"` +matching `datetime2(7)` precision, so a future sub-second or non-midnight +boundary won't silently round to the wrong partition. The staging table name +is still interpolated (T-SQL DDL identifiers can't be parameterised) but +remains internally constructed as `$"AuditLog_Staging_{Guid.NewGuid():N}"` +so SQL injection is structurally impossible. The larger +`sp_executesql`-with-typed-parameter migration was scoped to a future +follow-up since the current shape is fully controlled and the precision- +mismatch hazard was the only practical concern. + **Description** `SwitchOutPartitionAsync` builds two large SQL batches via interpolated strings diff --git a/code-reviews/ExternalSystemGateway/findings.md b/code-reviews/ExternalSystemGateway/findings.md index 88d008ba..9c105f06 100644 --- a/code-reviews/ExternalSystemGateway/findings.md +++ b/code-reviews/ExternalSystemGateway/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-28 | | Reviewer | claude-agent | | Commit reviewed | `1eb6e97` | -| Open findings | 4 | +| Open findings | 1 | ## Summary @@ -1180,9 +1180,20 @@ that happens to encode a number (already correctly returns `string`). |--|--| | Severity | Low | | Category | Security | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.ExternalSystemGateway/ExternalSystemClient.cs:385-415` | +**Resolution (2026-05-28):** `ApplyAuth` is now an instance method that uses +the existing `_logger`. Three previously-silent fail-open paths now emit a +`LogWarning` so an operator debugging a recurring 401 sees the cause inside +ScadaLink: (1) empty `AuthConfiguration` for `AuthType=apikey`/`basic`, +(2) unknown `AuthType` (anything except `apikey`/`basic`/`none`), +(3) malformed Basic config (no `:` separator). The `AuthConfiguration` +value is NEVER included in the log message. `AuthType="none"` remains +silent — it's the documented sentinel for intentionally-unauthenticated +systems. Behaviour is otherwise unchanged: the request still goes out +(never block on auth-config issues), the failure mode is just now visible. + **Description** `ApplyAuth` has three fail-open paths that all result in an HTTP request being sent diff --git a/code-reviews/InboundAPI/findings.md b/code-reviews/InboundAPI/findings.md index 6bcd456a..5a88a805 100644 --- a/code-reviews/InboundAPI/findings.md +++ b/code-reviews/InboundAPI/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-28 | | Reviewer | claude-agent | | Commit reviewed | `1eb6e97` | -| Open findings | 2 | +| Open findings | 1 | ## Summary @@ -1233,9 +1233,21 @@ immediate change required; this is a watch-list item. |--|--| | Severity | Medium | | Category | Correctness & logic bugs | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.Host/Program.cs:183-185`; consumers: `src/ScadaLink.ManagementService/AuditEndpoints.cs:93-94`; emitter: `src/ScadaLink.InboundAPI/Middleware/AuditWriteMiddleware.cs:175-252` | +**Resolution (2026-05-28):** Took the defensive path-exclusion in +`Program.cs` (Option 1 from the recommendation). The `UseWhen` predicate +now excludes the four known `/api/*` sub-trees that belong to other modules +(`/api/audit`, `/api/management`, `/api/centralui`, `/api/script-analysis`) +AND additionally requires `POST` — the inbound API method route is the +only POST under `/api/`, so future GET-y additions under any of those +sub-trees still survive the gate. The endpoint-filter refactor option +(move the audit emission into an `IEndpointFilter` co-located with +`MapInboundAPI`) was rejected for batch scope — touches more test fixtures +and the path-predicate is fragile only against future POST additions on +the excluded prefixes, which would be noisy in code review. + **Description** `Program.cs` wires the audit middleware as diff --git a/code-reviews/ManagementService/findings.md b/code-reviews/ManagementService/findings.md index 7ec93b14..fa01b01d 100644 --- a/code-reviews/ManagementService/findings.md +++ b/code-reviews/ManagementService/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-28 | | Reviewer | claude-agent | | Commit reviewed | `1eb6e97` | -| Open findings | 3 (1 Deferred — see ManagementService-012) | +| Open findings | 1 (1 Deferred — see ManagementService-012) | ## Summary @@ -927,9 +927,20 @@ mixed-set intersected. |--|--| | Severity | Medium | | Category | Security | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.ManagementService/ManagementActor.cs:1136`–`:1153` | +**Resolution (2026-05-28):** Added `SmtpConfigPublicShape` projection that +returns every non-secret field plus a `HasCredentials` bool — never the +`Credentials` field itself. Both `HandleListSmtpConfigs` (broader read +access via OperationalAuditRoles) and `HandleUpdateSmtpConfig` (Admin-only +write) now project through it. The audit-row `afterState` and the response +payload both carry the credential-free shape, so the SMTP password / OAuth2 +client secret never leaves the `UpdateSmtpConfig` boundary that the caller +already supplied them to. ManagementService 100/100 tests still pass. +Follow-up to tag `SmtpConfiguration.Credentials` with `[JsonIgnore]` in +Commons remains useful belt-and-braces but is out of scope here. + **Description** `HandleUpdateSmtpConfig` reads the existing `SmtpConfiguration` entity, applies the diff --git a/code-reviews/NotificationService/findings.md b/code-reviews/NotificationService/findings.md index fa9f0bd8..1bdfadd8 100644 --- a/code-reviews/NotificationService/findings.md +++ b/code-reviews/NotificationService/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-28 | | Reviewer | claude-agent | | Commit reviewed | `1eb6e97` | -| Open findings | 5 | +| Open findings | 1 | ## Summary @@ -813,9 +813,23 @@ After NS-019 is decided: |--|--| | Severity | Low | | Category | Correctness & logic bugs | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.NotificationService/CredentialRedactor.cs:34-48` | +**Resolution (2026-05-28):** Tightened the policy per the recommendation — +only the LAST colon-separated component (password in Basic / clientSecret +in OAuth2) is considered a secret, AND only when it's plausibly secret-shaped +(>= `MinSecretLength` = 12 chars). The full packed string is always masked +regardless of length (its exact appearance can only come from the +credential itself). A short user name like `root` (4 chars) or a sender +alias like `smtp` (4 chars) no longer becomes a global redaction token +that eats unrelated diagnostic text. 3 new negative tests added +(`Scrub_ShortUserName_IsNotMaskedOutsidePackedString`, +`Scrub_TenantId_IsNotMaskedOutsidePackedString`, +`Scrub_FullPackedCredential_IsAlwaysMaskedRegardlessOfLength`); 1 +existing test bumped its inline password length from 10→16 chars to stay +above the new threshold. + **Description** ```csharp diff --git a/code-reviews/README.md b/code-reviews/README.md index a98e4721..020aab51 100644 --- a/code-reviews/README.md +++ b/code-reviews/README.md @@ -41,31 +41,31 @@ module file and counted in **Total**. |----------|---------------| | Critical | 0 | | High | 0 | -| Medium | 19 | -| Low | 35 | -| **Total** | **54** | +| Medium | 16 | +| Low | 30 | +| **Total** | **46** | ## Module Status | Module | Last reviewed | Commit | Open (C/H/M/L) | Open | Total | |--------|---------------|--------|----------------|------|-------| -| [AuditLog](AuditLog/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/1/1 | 2 | 11 | +| [AuditLog](AuditLog/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/1/0 | 1 | 11 | | [CLI](CLI/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/2 | 2 | 23 | | [CentralUI](CentralUI/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/3 | 3 | 33 | | [ClusterInfrastructure](ClusterInfrastructure/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/3 | 3 | 14 | | [Commons](Commons/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/4 | 4 | 23 | | [Communication](Communication/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/1 | 1 | 22 | -| [ConfigurationDatabase](ConfigurationDatabase/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/1/2 | 3 | 24 | +| [ConfigurationDatabase](ConfigurationDatabase/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/1 | 1 | 24 | | [DataConnectionLayer](DataConnectionLayer/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/0 | 0 | 22 | | [DeploymentManager](DeploymentManager/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/3 | 3 | 24 | -| [ExternalSystemGateway](ExternalSystemGateway/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/1/1 | 2 | 23 | +| [ExternalSystemGateway](ExternalSystemGateway/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/1/0 | 1 | 23 | | [HealthMonitoring](HealthMonitoring/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/2 | 2 | 23 | | [Host](Host/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/1/3 | 4 | 22 | -| [InboundAPI](InboundAPI/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/1/1 | 2 | 25 | -| [ManagementService](ManagementService/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/2/0 | 2 | 23 | +| [InboundAPI](InboundAPI/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/1 | 1 | 25 | +| [ManagementService](ManagementService/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/1/0 | 1 | 23 | | [NotificationOutbox](NotificationOutbox/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/1 | 1 | 10 | -| [NotificationService](NotificationService/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/1/1 | 2 | 25 | -| [Security](Security/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/1 | 1 | 21 | +| [NotificationService](NotificationService/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/1/0 | 1 | 25 | +| [Security](Security/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/0 | 0 | 21 | | [SiteCallAudit](SiteCallAudit/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/2/1 | 3 | 6 | | [SiteEventLogging](SiteEventLogging/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/2 | 2 | 23 | | [SiteRuntime](SiteRuntime/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/2/0 | 2 | 26 | @@ -88,16 +88,13 @@ _None open._ _None open._ -### Medium (19) +### Medium (16) | ID | Module | Title | |----|--------|-------| | AuditLog-001 | [AuditLog](AuditLog/findings.md) | Combined-telemetry transport is plumbed end-to-end but never invoked in production | -| ConfigurationDatabase-016 | [ConfigurationDatabase](ConfigurationDatabase/findings.md) | `InboundApiRepository.GetApiKeyByValueAsync` hashes the candidate with the unpeppered `ApiKeyHasher.Default` | | ExternalSystemGateway-020 | [ExternalSystemGateway](ExternalSystemGateway/findings.md) | `JsonElementToParameterValue` silently downcasts non-Int64 JSON numbers to `double`, losing precision for `decimal` SQL parameters on retry | | Host-016 | [Host](Host/findings.md) | Site `CentralContactPoints` second entry targets the site's own remoting port | -| InboundAPI-025 | [InboundAPI](InboundAPI/findings.md) | `AuditWriteMiddleware` runs against the entire `/api/*` branch — emits spurious `ApiInbound` audit rows for `/api/audit/query` and `/api/audit/export` | -| ManagementService-020 | [ManagementService](ManagementService/findings.md) | UpdateSmtpConfig returns and audits the SMTP Credentials field verbatim | | ManagementService-021 | [ManagementService](ManagementService/findings.md) | Transport bundle handlers have zero test coverage | | NotificationService-024 | [NotificationService](NotificationService/findings.md) | No test affirms the central-only invariant; the orphaned-path tests give a false coverage signal | | SiteCallAudit-001 | [SiteCallAudit](SiteCallAudit/findings.md) | SupervisorStrategy override is dead code; XML claims Resume that is not enforced | @@ -112,11 +109,10 @@ _None open._ | TemplateEngine-020 | [TemplateEngine](TemplateEngine/findings.md) | `Create*` audit entries are written with `EntityId = "0"` before `SaveChangesAsync` populates the real key | | Transport-010 | [Transport](Transport/findings.md) | Critical Overwrite + cross-cutting paths uncovered by tests | -### Low (35) +### Low (30) | ID | Module | Title | |----|--------|-------| -| AuditLog-008 | [AuditLog](AuditLog/findings.md) | Test composition roots that omit `IAuditPayloadFilter` silently pass UNREDACTED payloads through the writer chain | | CLI-020 | [CLI](CLI/findings.md) | `bundle export` success-envelope parse is unguarded | | CLI-022 | [CLI](CLI/findings.md) | `CommandTreeTests` excludes the two new command groups | | CentralUI-029 | [CentralUI](CentralUI/findings.md) | `ConfigurationAuditLog` uses `JS.InvokeAsync("eval", ...)` instead of a dedicated JS module | @@ -130,12 +126,10 @@ _None open._ | Commons-020 | [Commons](Commons/findings.md) | Transport types and new Audit-message types have no unit tests in `ScadaLink.Commons.Tests` | | Commons-023 | [Commons](Commons/findings.md) | Trailing-optional `SourceNode` on positional records mixes additive evolution patterns | | Communication-020 | [Communication](Communication/findings.md) | `SiteAddressCacheLoaded` carries mutable `Dictionary`/`List` types | -| ConfigurationDatabase-021 | [ConfigurationDatabase](ConfigurationDatabase/findings.md) | `SwitchOutPartitionAsync` interpolates `monthBoundary` / staging table name into raw SQL | | ConfigurationDatabase-024 | [ConfigurationDatabase](ConfigurationDatabase/findings.md) | Missing test coverage for SPLIT-RANGE failure-continuation and production-shape rowversion delete | | DeploymentManager-021 | [DeploymentManager](DeploymentManager/findings.md) | `ResolveSiteIdentifierAsync` silently substitutes the DB id when the site row is missing | | DeploymentManager-022 | [DeploymentManager](DeploymentManager/findings.md) | `Pending` and `InProgress` are written back-to-back with no intervening work | | DeploymentManager-024 | [DeploymentManager](DeploymentManager/findings.md) | Test probe actors hold mutable static state across tests | -| ExternalSystemGateway-021 | [ExternalSystemGateway](ExternalSystemGateway/findings.md) | `ApplyAuth` silently sends an unauthenticated request on unknown `AuthType`, empty `AuthConfiguration`, or malformed Basic config | | HealthMonitoring-021 | [HealthMonitoring](HealthMonitoring/findings.md) | `CentralSiteId = "central"` reserved constant silently collides with a real site named "central" | | HealthMonitoring-022 | [HealthMonitoring](HealthMonitoring/findings.md) | `CentralHealthReportLoopTests` uses real-time `PeriodicTimer` + `Task.Delay`; flake-prone on slow CI | | Host-018 | [Host](Host/findings.md) | Shipped per-role configs omit `NodeOptions.NodeName`, leaving `SourceNode` null | @@ -143,8 +137,6 @@ _None open._ | Host-021 | [Host](Host/findings.md) | Microsoft `Logging:LogLevel` section in `appsettings.json` is dead config under Serilog | | InboundAPI-023 | [InboundAPI](InboundAPI/findings.md) | `EndpointExtensions.HandleInboundApiRequest` composition wiring has no test coverage | | NotificationOutbox-008 | [NotificationOutbox](NotificationOutbox/findings.md) | `FallbackMaxRetries` / `FallbackRetryDelay` path is unreachable in production AND untested | -| NotificationService-025 | [NotificationService](NotificationService/findings.md) | `CredentialRedactor` over-masks: any 4-character credential component is masked anywhere it appears, including unrelated log text | -| Security-021 | [Security](Security/findings.md) | `RequireHttpsCookie=false` dev opt-out has no warning path — an HTTP production deployment silently transmits the JWT bearer credential in cleartext | | SiteCallAudit-006 | [SiteCallAudit](SiteCallAudit/findings.md) | Stuck-only paging test does not exercise the multi-page boundary with an interleaved non-stuck row at the cursor | | SiteEventLogging-018 | [SiteEventLogging](SiteEventLogging/findings.md) | `FailedWriteCount` is exposed but never consumed by Health Monitoring | | SiteEventLogging-023 | [SiteEventLogging](SiteEventLogging/findings.md) | Concurrent-stress test uses a non-volatile `stop` flag | diff --git a/code-reviews/Security/findings.md b/code-reviews/Security/findings.md index 1428ff56..56b80fda 100644 --- a/code-reviews/Security/findings.md +++ b/code-reviews/Security/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-28 | | Reviewer | claude-agent | | Commit reviewed | `1eb6e97` | -| Open findings | 1 (Security-021); 1 deferred (Security-008) | +| Open findings | 0 (1 deferred — Security-008) | ## Summary @@ -971,9 +971,21 @@ every required `SecurityOptions` field is enforced at startup, not at first use. |--|--| | Severity | Low | | Category | Security | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.Security/SecurityOptions.cs:100-108`; `src/ScadaLink.Security/ServiceCollectionExtensions.cs:54-59` | +**Resolution (2026-05-28):** Added `ILoggerFactory` to the cookie-options +`Configure` callback in `AddSecurity` so an explicit `RequireHttpsCookie=false` +opt-out now emits a startup `LogWarning` ("auth cookie SecurePolicy is +SameAsRequest. The cookie-embedded JWT will be transmitted in cleartext +over plain HTTP. This setting is intended for local dev only — set +SecurityOptions:RequireHttpsCookie=true in production."). Default is still +`true`, so production deployments unchanged. The "fail startup when +RequireHttpsCookie=false AND ASPNETCORE_ENVIRONMENT=Production" hard-stop +option was not implemented (the dev Docker cluster intentionally runs with +the flag false, and the env-var name varies across deploy mechanisms); +the warning is the right ergonomic floor. + **Description** The Security-002 fix added `RequireHttpsCookie` (default `true`) so the auth cookie's diff --git a/src/ScadaLink.AuditLog/Central/AuditLogIngestActor.cs b/src/ScadaLink.AuditLog/Central/AuditLogIngestActor.cs index dc909bf1..ab26174c 100644 --- a/src/ScadaLink.AuditLog/Central/AuditLogIngestActor.cs +++ b/src/ScadaLink.AuditLog/Central/AuditLogIngestActor.cs @@ -164,8 +164,12 @@ public class AuditLogIngestActor : ReceiveActor // is a silent no-op. // Filter BEFORE the IngestedAtUtc stamp so the redacted // copy carries the central-side ingest timestamp. Filter - // is contract-bound to never throw; null = pass-through. - var filtered = filter?.Apply(evt) ?? evt; + // is contract-bound to never throw. AuditLog-008: a null + // filter (test composition root, no IAuditPayloadFilter + // registered) now falls back to the SafeDefault rather than + // pass-through, so HTTP header redaction always runs. + var safeFilter = filter ?? Payload.SafeDefaultAuditPayloadFilter.Instance; + var filtered = safeFilter.Apply(evt); var ingested = filtered with { IngestedAtUtc = nowUtc }; await repository.InsertIfNotExistsAsync(ingested).ConfigureAwait(false); accepted.Add(evt.EventId); @@ -239,8 +243,10 @@ public class AuditLogIngestActor : ReceiveActor // Filter the audit half BEFORE the dual-write — only the // AuditLog row's payload columns are filterable; SiteCalls // carries operational state only (status, retry count) and - // is left untouched. - var filteredAudit = filter?.Apply(entry.Audit) ?? entry.Audit; + // is left untouched. AuditLog-008: null filter falls back + // to SafeDefault so header redaction always runs. + var safeFilter = filter ?? Payload.SafeDefaultAuditPayloadFilter.Instance; + var filteredAudit = safeFilter.Apply(entry.Audit); var auditStamped = filteredAudit with { IngestedAtUtc = ingestedAt }; var siteCallStamped = entry.SiteCall with { IngestedAtUtc = ingestedAt }; diff --git a/src/ScadaLink.AuditLog/Central/CentralAuditWriter.cs b/src/ScadaLink.AuditLog/Central/CentralAuditWriter.cs index 7dec9b38..0fde32c0 100644 --- a/src/ScadaLink.AuditLog/Central/CentralAuditWriter.cs +++ b/src/ScadaLink.AuditLog/Central/CentralAuditWriter.cs @@ -41,7 +41,7 @@ public sealed class CentralAuditWriter : ICentralAuditWriter { private readonly IServiceProvider _services; private readonly ILogger _logger; - private readonly IAuditPayloadFilter? _filter; + private readonly IAuditPayloadFilter _filter; private readonly ICentralAuditWriteFailureCounter _failureCounter; private readonly INodeIdentityProvider? _nodeIdentity; @@ -80,7 +80,12 @@ public sealed class CentralAuditWriter : ICentralAuditWriter { _services = services ?? throw new ArgumentNullException(nameof(services)); _logger = logger ?? throw new ArgumentNullException(nameof(logger)); - _filter = filter; + // AuditLog-008: never default to null — over-redact instead. + // SafeDefaultAuditPayloadFilter applies HTTP header redaction with + // hard-coded sensitive defaults so a composition root that omits the + // real filter still scrubs Authorization / X-Api-Key / Cookie / + // Set-Cookie before persistence. + _filter = filter ?? Payload.SafeDefaultAuditPayloadFilter.Instance; _failureCounter = failureCounter ?? new NoOpCentralAuditWriteFailureCounter(); _nodeIdentity = nodeIdentity; } @@ -99,9 +104,11 @@ public sealed class CentralAuditWriter : ICentralAuditWriter try { // Filter BEFORE stamping IngestedAtUtc + handing to the repo. The - // filter contract is "never throws"; the null-coalesce keeps the - // M4 test composition roots (no filter passed) working unchanged. - var filtered = _filter?.Apply(evt) ?? evt; + // filter contract is "never throws". AuditLog-008: _filter is now + // non-null (SafeDefaultAuditPayloadFilter fallback) so header + // redaction always runs even in composition roots that omit the + // real filter. + var filtered = _filter.Apply(evt); // SourceNode-stamping (Task 12): caller-provided value wins // (supports any future direct-write callsite that already has its diff --git a/src/ScadaLink.AuditLog/Payload/SafeDefaultAuditPayloadFilter.cs b/src/ScadaLink.AuditLog/Payload/SafeDefaultAuditPayloadFilter.cs new file mode 100644 index 00000000..d685166e --- /dev/null +++ b/src/ScadaLink.AuditLog/Payload/SafeDefaultAuditPayloadFilter.cs @@ -0,0 +1,79 @@ +using System.Text.RegularExpressions; +using ScadaLink.Commons.Entities.Audit; + +namespace ScadaLink.AuditLog.Payload; + +/// +/// AuditLog-008: minimal always-safe fallback filter used by the writer chain +/// when no is injected (test composition +/// roots, future composition roots that bypass AddAuditLog). Performs +/// HTTP header redaction for the always-sensitive defaults +/// (Authorization, X-Api-Key, Cookie, Set-Cookie) so a fixture that wires a +/// real never persists those headers +/// in cleartext. Does NOT perform body-regex redaction, SQL-parameter +/// redaction, or truncation — those stages need +/// with live options. The contract is: +/// over-redact safely, never throw, never miss a header that's on the +/// default sensitive list. +/// +public sealed class SafeDefaultAuditPayloadFilter : IAuditPayloadFilter +{ + /// Singleton instance — the filter is stateless and side-effect-free. + public static SafeDefaultAuditPayloadFilter Instance { get; } = new SafeDefaultAuditPayloadFilter(); + + private static readonly string[] DefaultHeaderRedactList = + { + "Authorization", + "X-Api-Key", + "Cookie", + "Set-Cookie", + }; + + private static readonly Regex HeaderRegex = new( + @"(?[A-Za-z][A-Za-z0-9\-_]*)\s*:\s*(?[^\r\n]*)", + RegexOptions.Compiled | RegexOptions.IgnoreCase); + + private SafeDefaultAuditPayloadFilter() { } + + /// + public AuditEvent Apply(AuditEvent rawEvent) + { + ArgumentNullException.ThrowIfNull(rawEvent); + try + { + return rawEvent with + { + RequestSummary = RedactHeaders(rawEvent.RequestSummary), + ResponseSummary = RedactHeaders(rawEvent.ResponseSummary), + }; + } + catch + { + // Over-redact: drop both summaries entirely so a malformed parse + // path never leaks the original. The contract is "never throw." + return rawEvent with + { + RequestSummary = "[redacted by SafeDefaultAuditPayloadFilter]", + ResponseSummary = "[redacted by SafeDefaultAuditPayloadFilter]", + }; + } + } + + private static string? RedactHeaders(string? summary) + { + if (string.IsNullOrEmpty(summary)) return summary; + + return HeaderRegex.Replace(summary, m => + { + var name = m.Groups["name"].Value; + foreach (var sensitive in DefaultHeaderRedactList) + { + if (string.Equals(name, sensitive, StringComparison.OrdinalIgnoreCase)) + { + return $"{name}: [REDACTED]"; + } + } + return m.Value; + }); + } +} diff --git a/src/ScadaLink.AuditLog/Site/FallbackAuditWriter.cs b/src/ScadaLink.AuditLog/Site/FallbackAuditWriter.cs index 321d6374..fc53d557 100644 --- a/src/ScadaLink.AuditLog/Site/FallbackAuditWriter.cs +++ b/src/ScadaLink.AuditLog/Site/FallbackAuditWriter.cs @@ -31,7 +31,7 @@ public sealed class FallbackAuditWriter : IAuditWriter private readonly RingBufferFallback _ring; private readonly IAuditWriteFailureCounter _failureCounter; private readonly ILogger _logger; - private readonly IAuditPayloadFilter? _filter; + private readonly IAuditPayloadFilter _filter; private readonly SemaphoreSlim _drainGate = new(1, 1); /// @@ -60,7 +60,14 @@ public sealed class FallbackAuditWriter : IAuditWriter _ring = ring ?? throw new ArgumentNullException(nameof(ring)); _failureCounter = failureCounter ?? throw new ArgumentNullException(nameof(failureCounter)); _logger = logger ?? throw new ArgumentNullException(nameof(logger)); - _filter = filter; // null = no-op pass-through; see WriteAsync. + // AuditLog-008: never default to a null filter — over-redact instead. + // SafeDefaultAuditPayloadFilter.Instance performs HTTP header + // redaction with the hard-coded sensitive defaults (Authorization, + // X-Api-Key, Cookie, Set-Cookie) so a test composition root that + // doesn't bind the real options never persists those headers + // verbatim. The real DefaultAuditPayloadFilter (truncation + body / + // SQL-param redaction) is wired by AddAuditLog and takes precedence. + _filter = filter ?? Payload.SafeDefaultAuditPayloadFilter.Instance; } /// @@ -72,9 +79,10 @@ public sealed class FallbackAuditWriter : IAuditWriter // and (on failure) to the ring buffer — so a primary outage that // drains later still hands the SqliteAuditWriter a row that has // already been truncated and redacted. The filter contract is - // "MUST NOT throw"; the null-coalesce keeps test composition roots - // that don't wire a filter working unchanged. - var filtered = _filter?.Apply(evt) ?? evt; + // "MUST NOT throw". AuditLog-008: _filter is now non-null (defaults + // to SafeDefaultAuditPayloadFilter so header redaction is always + // applied even in composition roots that don't wire the real filter). + var filtered = _filter.Apply(evt); try { diff --git a/src/ScadaLink.ConfigurationDatabase/Repositories/AuditLogRepository.cs b/src/ScadaLink.ConfigurationDatabase/Repositories/AuditLogRepository.cs index 5793a324..676213d0 100644 --- a/src/ScadaLink.ConfigurationDatabase/Repositories/AuditLogRepository.cs +++ b/src/ScadaLink.ConfigurationDatabase/Repositories/AuditLogRepository.cs @@ -198,8 +198,12 @@ VALUES // ISO 8601 in UTC — SQL Server's datetime2 literal parser accepts this // unambiguously and the value is round-trip-safe across SET DATEFORMAT - // settings. - var monthBoundaryStr = monthBoundary.ToUniversalTime().ToString("yyyy-MM-dd HH:mm:ss"); + // settings. CD-021: use datetime2(7) precision (.fffffff) so a future + // non-midnight or sub-second boundary doesn't silently round to the + // wrong partition (today the migration only seeds at T00:00:00 exactly, + // but the format string is on the boundary value's own contract — match + // it to the column precision rather than to the current seed pattern). + var monthBoundaryStr = monthBoundary.ToUniversalTime().ToString("yyyy-MM-dd HH:mm:ss.fffffff"); // Two-statement batch: the first SELECT samples the per-partition row // count BEFORE the dance so we can report it back to the purge actor; diff --git a/src/ScadaLink.ConfigurationDatabase/Repositories/InboundApiRepository.cs b/src/ScadaLink.ConfigurationDatabase/Repositories/InboundApiRepository.cs index e4bafee5..37f7f00f 100644 --- a/src/ScadaLink.ConfigurationDatabase/Repositories/InboundApiRepository.cs +++ b/src/ScadaLink.ConfigurationDatabase/Repositories/InboundApiRepository.cs @@ -10,16 +10,38 @@ namespace ScadaLink.ConfigurationDatabase.Repositories; public class InboundApiRepository : IInboundApiRepository { private readonly ScadaLinkDbContext _context; + // CD-016: lazily resolved so the InboundAPI ApiKeyHasher factory (which throws + // when no pepper is configured) is only invoked if GetApiKeyByValueAsync is + // actually called — Central/Host startup composition roots that never call + // this method (the production ApiKeyValidator deliberately doesn't) get to + // bring InboundApiRepository up without forcing every test to wire a + // throw-away pepper into InboundApiOptions. + private readonly Func _hasherAccessor; private readonly ILogger _logger; /// /// Initializes a new instance of the InboundApiRepository class. /// /// The database context for accessing inbound API data. + /// + /// CD-016: factory that returns the API-key hasher used to digest a candidate + /// plaintext for the peppered lookup. + /// Resolution is deferred to first call so a composition root that doesn't + /// register (or whose factory would throw because + /// no pepper is configured) can still bring up the repository for callers that + /// don't touch the value-lookup path. Defaults to a factory returning + /// ; production wires + /// sp => sp.GetRequiredService<IApiKeyHasher>() via DI so the + /// lookup uses the same peppered digest as the production write path. + /// /// Optional logger instance for warnings and diagnostics. - public InboundApiRepository(ScadaLinkDbContext context, ILogger? logger = null) + public InboundApiRepository( + ScadaLinkDbContext context, + Func? hasherAccessor = null, + ILogger? logger = null) { _context = context ?? throw new ArgumentNullException(nameof(context)); + _hasherAccessor = hasherAccessor ?? (() => ApiKeyHasher.Default); _logger = logger ?? NullLogger.Instance; } @@ -34,7 +56,13 @@ public class InboundApiRepository : IInboundApiRepository /// public async Task GetApiKeyByValueAsync(string keyValue, CancellationToken cancellationToken = default) { - var keyHash = ApiKeyHasher.Default.Hash(keyValue); + // CD-016: hash the candidate with the DI-provided peppered hasher so this + // lookup matches keys whose stored KeyHash was produced by the production + // ApiKeyHasher(pepper). The pre-fix call to ApiKeyHasher.Default would + // silently return null for every real key on any peppered deployment. + // Resolution is deferred until this method is actually called so the + // pepper-validating factory doesn't fire during startup composition. + var keyHash = _hasherAccessor().Hash(keyValue); return await _context.Set().FirstOrDefaultAsync(k => k.KeyHash == keyHash, cancellationToken); } diff --git a/src/ScadaLink.ConfigurationDatabase/ServiceCollectionExtensions.cs b/src/ScadaLink.ConfigurationDatabase/ServiceCollectionExtensions.cs index 5f811f24..afa39a69 100644 --- a/src/ScadaLink.ConfigurationDatabase/ServiceCollectionExtensions.cs +++ b/src/ScadaLink.ConfigurationDatabase/ServiceCollectionExtensions.cs @@ -1,6 +1,7 @@ using Microsoft.AspNetCore.DataProtection; using Microsoft.EntityFrameworkCore; using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging; using ScadaLink.Commons.Interfaces; using ScadaLink.Commons.Interfaces.Repositories; using ScadaLink.Commons.Interfaces.Services; @@ -53,7 +54,15 @@ public static class ServiceCollectionExtensions services.AddScoped(); services.AddScoped(); services.AddScoped(); - services.AddScoped(); + // CD-016: factory registration wires a lazy accessor for IApiKeyHasher so + // the production peppered hasher is used (via DI) when GetApiKeyByValueAsync + // is actually called, but composition roots that never call it (and may + // not register IApiKeyHasher at all) still bring up the repository. + services.AddScoped(sp => new InboundApiRepository( + sp.GetRequiredService(), + hasherAccessor: () => sp.GetService() + ?? Commons.Types.InboundApi.ApiKeyHasher.Default, + logger: sp.GetService>())); services.AddScoped(); services.AddScoped(); services.AddScoped(); diff --git a/src/ScadaLink.ExternalSystemGateway/ExternalSystemClient.cs b/src/ScadaLink.ExternalSystemGateway/ExternalSystemClient.cs index a1d2579c..6336d7a7 100644 --- a/src/ScadaLink.ExternalSystemGateway/ExternalSystemClient.cs +++ b/src/ScadaLink.ExternalSystemGateway/ExternalSystemClient.cs @@ -464,12 +464,29 @@ public class ExternalSystemClient : IExternalSystemClient return url; } - private static void ApplyAuth(HttpRequestMessage request, ExternalSystemDefinition system) + private void ApplyAuth(HttpRequestMessage request, ExternalSystemDefinition system) { - if (string.IsNullOrEmpty(system.AuthConfiguration)) - return; + // ESG-021: distinguish "intentionally unauthenticated" (AuthType = none) + // from "AuthConfiguration is missing or empty for a type that requires it" + // (deployment glitch, decryption failure, operator typo). The unauthenticated + // case is silent; the requires-creds-but-empty case logs a Warning so an + // operator debugging a recurring 401 sees the cause inside ScadaLink instead + // of having to read the remote system's logs. The value of AuthConfiguration + // is NEVER logged. + var authType = system.AuthType?.Trim().ToLowerInvariant() ?? string.Empty; - switch (system.AuthType.ToLowerInvariant()) + if (string.IsNullOrEmpty(system.AuthConfiguration)) + { + if (authType is "apikey" or "basic") + { + _logger.LogWarning( + "ApplyAuth: External system '{System}' has AuthType '{AuthType}' but AuthConfiguration is empty; request will be sent without an auth header.", + system.Name, system.AuthType); + } + return; + } + + switch (authType) { case "apikey": // Auth config format: "HeaderName:KeyValue" or just "KeyValue" (default header: X-API-Key) @@ -493,6 +510,26 @@ public class ExternalSystemClient : IExternalSystemClient Encoding.UTF8.GetBytes($"{basicParts[0]}:{basicParts[1]}")); request.Headers.Authorization = new AuthenticationHeaderValue("Basic", encoded); } + else + { + // ESG-021: malformed Basic config (no ':' separator) means the + // request goes out with no Authorization header. Warn so the + // failure mode is visible inside ScadaLink. + _logger.LogWarning( + "ApplyAuth: External system '{System}' AuthType 'basic' AuthConfiguration is malformed (expected 'username:password'); request will be sent without an Authorization header.", + system.Name); + } + break; + + case "none": + // Documented sentinel for unauthenticated systems — silent by design. + break; + + default: + // ESG-021: unknown AuthType silently fell through here before. Warn. + _logger.LogWarning( + "ApplyAuth: External system '{System}' has unknown AuthType '{AuthType}'; request will be sent without an auth header. Allowed values: apikey, basic, none.", + system.Name, system.AuthType); break; } } diff --git a/src/ScadaLink.Host/Program.cs b/src/ScadaLink.Host/Program.cs index deb02621..b0efcbf6 100644 --- a/src/ScadaLink.Host/Program.cs +++ b/src/ScadaLink.Host/Program.cs @@ -195,8 +195,23 @@ try // is responsible for stashing the resolved API key name on // HttpContext.Items (see AuditWriteMiddleware.AuditActorItemKey) AFTER its // in-handler API key validation succeeds. + // InboundAPI-025: scope the audit middleware to the inbound API method + // route (/api/{methodName}) and explicitly exclude the management/audit + // sub-trees that share the /api prefix. Without these exclusions the + // middleware would emit a spurious ApiInbound audit row for every + // /api/audit/query and /api/audit/export call (and would treat audit-log + // reads as inbound script invocations — recursive write-on-read). The + // POST-only filter rules out the GET routes on /api/audit, /api/centralui, + // /api/script-analysis even if a future route is added under those + // prefixes with the same verb; the explicit prefix excludes still belt- + // and-brace POST-y additions there. app.UseWhen( - ctx => ctx.Request.Path.StartsWithSegments("/api"), + ctx => ctx.Request.Path.StartsWithSegments("/api") + && !ctx.Request.Path.StartsWithSegments("/api/audit") + && !ctx.Request.Path.StartsWithSegments("/api/centralui") + && !ctx.Request.Path.StartsWithSegments("/api/script-analysis") + && !ctx.Request.Path.StartsWithSegments("/api/management") + && HttpMethods.IsPost(ctx.Request.Method), branch => branch.UseAuditWriteMiddleware()); // WP-12: Map readiness endpoint — returns 503 until ready, 200 when ready. diff --git a/src/ScadaLink.ManagementService/ManagementActor.cs b/src/ScadaLink.ManagementService/ManagementActor.cs index 736bf347..f8a65f04 100644 --- a/src/ScadaLink.ManagementService/ManagementActor.cs +++ b/src/ScadaLink.ManagementService/ManagementActor.cs @@ -1135,10 +1135,36 @@ public class ManagementActor : ReceiveActor return true; } + /// + /// MgmtSvc-020: project an SmtpConfiguration to a credential-free shape so the + /// stored Credentials (SMTP password / OAuth2 client secret) never leaves this + /// boundary via response payloads or audit afterState. Mirrors the + /// ApiKey-projection pattern in HandleListApiKeys / CD-012's fix. + /// + private static object SmtpConfigPublicShape(Commons.Entities.Notifications.SmtpConfiguration c) => + new + { + c.Id, + c.Host, + c.Port, + c.AuthType, + c.FromAddress, + c.TlsMode, + c.ConnectionTimeoutSeconds, + c.MaxConcurrentConnections, + c.MaxRetries, + c.RetryDelay, + HasCredentials = !string.IsNullOrEmpty(c.Credentials), + }; + private static async Task HandleListSmtpConfigs(IServiceProvider sp) { var repo = sp.GetRequiredService(); - return await repo.GetAllSmtpConfigurationsAsync(); + var configs = await repo.GetAllSmtpConfigurationsAsync(); + // MgmtSvc-020: project away the Credentials field — read access to this + // list is broader than the Admin-only UpdateSmtpConfig path that owns + // the secret. + return configs.Select(SmtpConfigPublicShape).ToList(); } private static async Task HandleUpdateSmtpConfig(IServiceProvider sp, UpdateSmtpConfigCommand cmd, string user) @@ -1156,8 +1182,12 @@ public class ManagementActor : ReceiveActor if (cmd.Credentials is not null) config.Credentials = cmd.Credentials; await repo.UpdateSmtpConfigurationAsync(config); await repo.SaveChangesAsync(); - await AuditAsync(sp, user, "Update", "SmtpConfiguration", config.Id.ToString(), config.Host, config); - return config; + // MgmtSvc-020: audit the credential-free shape — the *fact of* the change + // (and which non-secret fields hold) is observable; the secret value is + // not persisted to the audit log where OperationalAuditRoles can read it. + var publicShape = SmtpConfigPublicShape(config); + await AuditAsync(sp, user, "Update", "SmtpConfiguration", config.Id.ToString(), config.Host, publicShape); + return publicShape; } // ======================================================================== diff --git a/src/ScadaLink.NotificationService/CredentialRedactor.cs b/src/ScadaLink.NotificationService/CredentialRedactor.cs index 41f4dc0e..257eb1c8 100644 --- a/src/ScadaLink.NotificationService/CredentialRedactor.cs +++ b/src/ScadaLink.NotificationService/CredentialRedactor.cs @@ -24,6 +24,15 @@ public static class CredentialRedactor /// The credential string in use — Basic Auth user:pass or OAuth2 /// tenantId:clientId:clientSecret. May be null. /// + /// + /// NS-025: minimum length for a colon-separated SECRET component to be + /// considered worth masking. Twelve characters is the standard heuristic + /// for "long enough to be a password / client secret"; shorter components + /// (e.g. a 4-char user name like root, or a 7-char "from" alias) + /// would mask too much unrelated diagnostic text if treated as secrets. + /// + private const int MinSecretLength = 12; + public static string Scrub(string? text, string? credentials) { if (string.IsNullOrEmpty(text) || string.IsNullOrEmpty(credentials)) @@ -33,16 +42,36 @@ public static class CredentialRedactor var result = text; - // Mask each individual colon-delimited component (covers user, password, - // tenant, clientId, clientSecret) and the whole packed string. Order longest - // first so a component that is a substring of another is still fully masked. - var parts = credentials.Split(':') - .Where(p => p.Length >= 4) - .Append(credentials) - .Distinct() - .OrderByDescending(p => p.Length); + // NS-025: redact only the obviously-secret slots — the LAST + // colon-separated component (the password in Basic, the client + // secret in OAuth2) and the whole packed string — not the user + // name / tenant id / client id. A short user name like "root" or + // a sender alias like "smtp" no longer becomes a global redaction + // token that eats unrelated path / error text. + var secretsToRedact = new List(); - foreach (var part in parts) + // The full packed credential is always the most-sensitive shape. + secretsToRedact.Add(credentials); + + // The trailing colon-component is the password / clientSecret slot. + // Only redact it if it's plausibly secret-shaped (>= MinSecretLength). + var parts = credentials.Split(':'); + if (parts.Length >= 2) + { + var lastComponent = parts[^1]; + if (lastComponent.Length >= MinSecretLength) + { + secretsToRedact.Add(lastComponent); + } + } + + // Order longest first so a secret that is a substring of the packed + // string is still fully masked. + var ordered = secretsToRedact + .Distinct() + .OrderByDescending(s => s.Length); + + foreach (var part in ordered) { result = result.Replace(part, Mask, StringComparison.Ordinal); } diff --git a/src/ScadaLink.Security/ServiceCollectionExtensions.cs b/src/ScadaLink.Security/ServiceCollectionExtensions.cs index 865faa5f..32f3474e 100644 --- a/src/ScadaLink.Security/ServiceCollectionExtensions.cs +++ b/src/ScadaLink.Security/ServiceCollectionExtensions.cs @@ -1,6 +1,7 @@ using Microsoft.AspNetCore.Authentication.Cookies; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.DependencyInjection.Extensions; +using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; namespace ScadaLink.Security; @@ -56,7 +57,7 @@ public static class ServiceCollectionExtensions // session window. Bound here via PostConfigure so SecurityOptions // (configured by the Host after AddSecurity) is honoured. services.AddOptions(CookieAuthenticationDefaults.AuthenticationScheme) - .Configure>((cookieOptions, securityOptions) => + .Configure, ILoggerFactory>((cookieOptions, securityOptions, loggerFactory) => { var idleMinutes = securityOptions.Value.IdleTimeoutMinutes; cookieOptions.ExpireTimeSpan = TimeSpan.FromMinutes(idleMinutes); @@ -69,6 +70,18 @@ public static class ServiceCollectionExtensions cookieOptions.Cookie.SecurePolicy = securityOptions.Value.RequireHttpsCookie ? Microsoft.AspNetCore.Http.CookieSecurePolicy.Always : Microsoft.AspNetCore.Http.CookieSecurePolicy.SameAsRequest; + + // Security-021: when the operator opts out of HTTPS-only cookies, + // log a Warning so an HTTP-only deployment is at least audible in + // the startup log. The cookie carries the embedded JWT bearer + // credential — over plain HTTP that travels in cleartext on every + // request. The default is true; this branch fires only on an + // explicit opt-out (typically the dev Docker cluster). + if (!securityOptions.Value.RequireHttpsCookie) + { + loggerFactory.CreateLogger("ScadaLink.Security").LogWarning( + "SecurityOptions:RequireHttpsCookie is DISABLED — auth cookie SecurePolicy is SameAsRequest. The cookie-embedded JWT will be transmitted in cleartext over plain HTTP. This setting is intended for local dev only — set SecurityOptions:RequireHttpsCookie=true in production."); + } }); services.AddScadaLinkAuthorization(); diff --git a/tests/ScadaLink.ConfigurationDatabase.Tests/InboundApiRepositoryTests.cs b/tests/ScadaLink.ConfigurationDatabase.Tests/InboundApiRepositoryTests.cs index d92994a3..d927e32f 100644 --- a/tests/ScadaLink.ConfigurationDatabase.Tests/InboundApiRepositoryTests.cs +++ b/tests/ScadaLink.ConfigurationDatabase.Tests/InboundApiRepositoryTests.cs @@ -15,7 +15,7 @@ public class InboundApiRepositoryTests : IDisposable public InboundApiRepositoryTests() { _context = SqliteTestHelper.CreateInMemoryContext(); - _repository = new InboundApiRepository(_context, _logger); + _repository = new InboundApiRepository(_context, hasherAccessor: null, logger: _logger); } public void Dispose() @@ -40,6 +40,37 @@ public class InboundApiRepositoryTests : IDisposable Assert.Equal(key.Id, byValue!.Id); } + [Fact] + public async Task CD016_GetApiKeyByValue_UsesInjectedPepperedHasher_NotDefault() + { + // CD-016 regression: stored KeyHash is produced by a peppered hasher. + // A repository whose lookup uses ApiKeyHasher.Default (the pre-fix + // behaviour) would compute a different digest and return null. With the + // pepper-aware hasherAccessor wired in, the lookup must round-trip. + var peppered = new Commons.Types.InboundApi.ApiKeyHasher("a-strong-test-pepper-of-sufficient-length"); + var pepperedHash = peppered.Hash("secret-with-pepper"); + var key = ApiKey.FromHash("Peppered", pepperedHash); + key.IsEnabled = true; + + using var ctx = SqliteTestHelper.CreateInMemoryContext(); + var repo = new InboundApiRepository(ctx, hasherAccessor: () => peppered, logger: _logger); + await repo.AddApiKeyAsync(key); + await repo.SaveChangesAsync(); + + var byValue = await repo.GetApiKeyByValueAsync("secret-with-pepper"); + Assert.NotNull(byValue); + Assert.Equal(key.Id, byValue!.Id); + + // And: a repository wired with the Default (unpeppered) hasher MUST + // NOT find the same key — proving the lookup actually uses the + // injected hasher and the original bug shape. + var defaultRepo = new InboundApiRepository(ctx, + hasherAccessor: () => Commons.Types.InboundApi.ApiKeyHasher.Default, + logger: _logger); + var missByDefault = await defaultRepo.GetApiKeyByValueAsync("secret-with-pepper"); + Assert.Null(missByDefault); + } + [Fact] public async Task AddApiMethod_AndGetByName_RoundTrips() { diff --git a/tests/ScadaLink.NotificationService.Tests/CredentialRedactorTests.cs b/tests/ScadaLink.NotificationService.Tests/CredentialRedactorTests.cs index 454a26bf..eb2aee25 100644 --- a/tests/ScadaLink.NotificationService.Tests/CredentialRedactorTests.cs +++ b/tests/ScadaLink.NotificationService.Tests/CredentialRedactorTests.cs @@ -8,11 +8,13 @@ public class CredentialRedactorTests [Fact] public void Scrub_BasicAuthPassword_IsMasked() { - var text = "535 5.7.8 Authentication failed for user 'svc' with password 'Hunter2pw!'"; - var result = CredentialRedactor.Scrub(text, "svc:Hunter2pw!"); + // Password 'Hunter2pass!word' is 16 chars (>= MinSecretLength=12) and + // therefore qualifies as a redactable secret-shaped trailing component. + var text = "535 5.7.8 Authentication failed for user 'svc' with password 'Hunter2pass!word'"; + var result = CredentialRedactor.Scrub(text, "svc:Hunter2pass!word"); - Assert.DoesNotContain("Hunter2pw!", result); - Assert.DoesNotContain("svc:Hunter2pw!", result); + Assert.DoesNotContain("Hunter2pass!word", result); + Assert.DoesNotContain("svc:Hunter2pass!word", result); } [Fact] @@ -35,4 +37,40 @@ public class CredentialRedactorTests { Assert.Equal(string.Empty, CredentialRedactor.Scrub(null, "user:pass")); } + + // --- NS-025: don't over-mask short non-secret components --- + + [Fact] + public void Scrub_ShortUserName_IsNotMaskedOutsidePackedString() + { + // 'root' is the Basic Auth user name — short, common, and absolutely + // not a secret. It must NOT be masked when it appears in unrelated + // diagnostic text like a file path. + var text = "Config file at /root/.config/scada.conf was not found."; + var result = CredentialRedactor.Scrub(text, "root:hunter2longenoughpwd"); + + Assert.Contains("/root/.config", result); + } + + [Fact] + public void Scrub_TenantId_IsNotMaskedOutsidePackedString() + { + // The tenant id is not secret — only the client secret is. A tenant id + // appearing in unrelated text (e.g. an error-code suffix) must survive. + var text = "Error code tnt-1234567890-abcd reported by upstream"; + var result = CredentialRedactor.Scrub(text, "tnt-1234567890-abcd:cli-guid:RealClientSecretLongEnough"); + + Assert.Contains("tnt-1234567890-abcd", result); + } + + [Fact] + public void Scrub_FullPackedCredential_IsAlwaysMaskedRegardlessOfLength() + { + // Even a short packed string must be masked when it appears verbatim — + // that exact appearance can only come from the credential itself. + var text = "Auth bundle was rejected: u:p"; + var result = CredentialRedactor.Scrub(text, "u:p"); + + Assert.DoesNotContain("u:p", result); + } }