fix(security): resolve Security-004..007 — configurable user-id attribute, DN escaping, JWT issuer/audience validation, idle-timeout preservation

code-reviews/Security/findings.md

@@ -8,7 +8,7 @@
 | Last reviewed | 2026-05-16 |
 | Reviewer | claude-agent |
 | Commit reviewed | `9c60592` |
-| Open findings | 8 |
+| Open findings | 4 |
 
 ## Summary
 
@@ -156,7 +156,7 @@ corrected to state the requirement. Regression tests
 |--|--|
 | Severity | Medium |
 | Category | Correctness & logic bugs |
-| Status | Open |
+| Status | Resolved |
 | Location | `src/ScadaLink.Security/LdapAuthService.cs:66`, `:138`, `:157-159` |
 
 **Description**
@@ -176,7 +176,15 @@ consistently in both the search filter and the fallback DN. Update the XML doc t
 
 **Resolution**
 
-_Unresolved._
+Resolved 2026-05-16 (commit `pending`). Confirmed: the search filter was hard-coded
+`(uid={username})` (both in `AuthenticateAsync` and `ResolveUserDnAsync`) while the
+fallback DN used `cn={username}` — the two auth modes were not interchangeable. Added
+a configurable `SecurityOptions.LdapUserIdAttribute` (default `uid`) used for both the
+search filter and the fallback DN via the new `BuildFallbackUserDn` helper, and
+corrected the `LdapServiceAccountDn` XML doc to reference `{LdapUserIdAttribute}`.
+Regression tests `BuildFallbackUserDn_UsesConfiguredUserIdAttribute`,
+`BuildFallbackUserDn_HonoursNonDefaultUserIdAttribute`,
+`SecurityOptions_LdapUserIdAttribute_DefaultsToUid`.
 
 ### Security-005 — DN injection in the no-service-account bind fallback
 
@@ -184,7 +192,7 @@ _Unresolved._
 |--|--|
 | Severity | Medium |
 | Category | Security |
-| Status | Open |
+| Status | Resolved |
 | Location | `src/ScadaLink.Security/LdapAuthService.cs:157-159` |
 
 **Description**
@@ -207,7 +215,15 @@ raw DN from untrusted input is risky; restrict it or remove it.
 
 **Resolution**
 
-_Unresolved._
+Resolved 2026-05-16 (commit `pending`). Confirmed: the fallback path interpolated
+the raw `username` straight into `cn={username},{LdapSearchBase}` with no DN escaping,
+and the `username.Contains('=')` shortcut let a caller supply an arbitrary bind DN.
+Added an RFC 4514 `EscapeLdapDn` helper (escapes `, + " \ < > ;`, leading/trailing
+space, leading `#`, NUL) applied in `BuildFallbackUserDn`, so a username such as
+`victim,ou=admins` can no longer alter the DN structure. The `Contains('=')` raw-DN
+shortcut was removed entirely — untrusted input no longer controls the bind identity.
+Regression tests `BuildFallbackUserDn_EscapesDnMetacharacters`,
+`EscapeLdapDn_EscapesAllRfc4514Specials`, `EscapeLdapDn_EscapesLeadingAndTrailingSpaces`.
 
 ### Security-006 — JWT validation disables issuer and audience checks
 
@@ -215,7 +231,7 @@ _Unresolved._
 |--|--|
 | Severity | Medium |
 | Category | Security |
-| Status | Open |
+| Status | Resolved |
 | Location | `src/ScadaLink.Security/JwtTokenService.cs:67-75`, `:56-59` |
 
 **Description**
@@ -236,7 +252,14 @@ validation.
 
 **Resolution**
 
-_Unresolved._
+Resolved 2026-05-16 (commit `pending`). Confirmed: `GenerateToken` set neither `iss`
+nor `aud` and `ValidateToken` had `ValidateIssuer = false`/`ValidateAudience = false`.
+`GenerateToken` now binds `JwtTokenService.TokenIssuer`/`TokenAudience`
+(both `"scadalink-central"`) into every token, and `ValidateToken` enables
+`ValidateIssuer`/`ValidateAudience` against those fixed values — a token signed with
+the shared key but a foreign issuer is now rejected. Regression tests
+`GenerateToken_SetsIssuerAndAudience`, `ValidateToken_RejectsTokenWithWrongIssuer`,
+`ValidateToken_AcceptsOwnIssuerAndAudience`.
 
 ### Security-007 — Idle-timeout claim is reset on every token refresh
 
@@ -244,7 +267,7 @@ _Unresolved._
 |--|--|
 | Severity | Medium |
 | Category | Correctness & logic bugs |
-| Status | Open |
+| Status | Resolved |
 | Location | `src/ScadaLink.Security/JwtTokenService.cs:40`, `:111-123` |
 
 **Description**
@@ -269,7 +292,22 @@ path agree on the semantics.
 
 **Resolution**
 
-_Unresolved._
+Resolved 2026-05-16 (commit `pending`). Confirmed: `RefreshToken` → `GenerateToken`
+unconditionally wrote `LastActivity = UtcNow`, so the idle clock reset on every
+refresh and the documented 30-minute idle timeout could never fire for a client that
+polls in the background. Implemented option (a) — the Security-side half of the
+documented "15-min sliding + 30-min idle" policy (the cross-module partner of
+CentralUI-005): `GenerateToken` now takes an optional `lastActivity` anchor;
+`RefreshToken` carries the **existing** `LastActivity` claim forward unchanged, and a
+new explicit `RecordActivity` method advances the anchor to now — to be called by the
+CentralUI request pipeline on genuine user interaction (not on a background refresh).
+`IsIdleTimedOut` is unchanged and now agrees with the refresh path. The remaining
+CentralUI-side wiring (calling `RecordActivity` from the middleware, setting
+`SlidingExpiration`/`ExpireTimeSpan`) stays tracked under CentralUI-005; this finding's
+Security-side defect — the reset-on-refresh bug — is fully fixed here. Regression tests
+`RefreshToken_PreservesOriginalLastActivityClaim`,
+`RefreshToken_DoesNotResetIdleTimeoutWhenUserIsActuallyIdle`,
+`RecordActivity_UpdatesLastActivityToNow`.
 
 ### Security-008 — N+1 query loading site-scope rules in `RoleMapper`