From 1e2e7d2e7c7028a469149a5678b6a7ad64defe04 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 16 May 2026 23:54:31 -0400 Subject: [PATCH] =?UTF-8?q?fix(central-ui):=20resolve=20CentralUI-005=20?= =?UTF-8?q?=E2=80=94=20sliding=20cookie=20session=20expiry=20(Security=20A?= =?UTF-8?q?ddCookie=20+=20AuthEndpoints=20+=20SessionExpiry)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- code-reviews/CentralUI/findings.md | 57 ++++++++++++---- src/ScadaLink.CentralUI/Auth/AuthEndpoints.cs | 34 +++++++--- .../Components/Shared/SessionExpiry.razor | 65 +++++++++++++------ .../ServiceCollectionExtensions.cs | 23 +++++++ .../Auth/SessionExpiryPolicyTests.cs | 46 +++++++++++++ .../ScadaLink.Security.Tests/SecurityTests.cs | 57 ++++++++++++++++ 6 files changed, 240 insertions(+), 42 deletions(-) create mode 100644 tests/ScadaLink.CentralUI.Tests/Auth/SessionExpiryPolicyTests.cs diff --git a/code-reviews/CentralUI/findings.md b/code-reviews/CentralUI/findings.md index e00e925..fc1740e 100644 --- a/code-reviews/CentralUI/findings.md +++ b/code-reviews/CentralUI/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-16 | | Reviewer | claude-agent | | Commit reviewed | `9c60592` | -| Open findings | 2 | +| Open findings | 1 | ## Summary @@ -248,7 +248,7 @@ Fixed by the commit whose message references `CentralUI-004`. |--|--| | Severity | Medium | | Category | Design-document adherence | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.CentralUI/Auth/AuthEndpoints.cs:47-81`; `src/ScadaLink.CentralUI/Components/Shared/SessionExpiry.razor:18-30` | **Description** @@ -269,17 +269,48 @@ fixed 30-minute model. The code and the documented decision must agree. **Resolution** -_Unresolved — requires a cross-module change plus a design decision, both out of -scope for a CentralUI-only fix._ Verified 2026-05-16: the discrepancy is real. -The sliding-expiration mechanism, however, is owned by the cookie -authentication middleware configured in **`ScadaLink.Security`** -(`ServiceCollectionExtensions.AddCookie` — currently sets neither -`ExpireTimeSpan` nor `SlidingExpiration`); `AuthEndpoints` (CentralUI) only sets -the absolute `ExpiresUtc`/`expires_at`. Implementing "15-minute sliding token" -means editing `ScadaLink.Security`, which this module's review cannot touch, and -the alternative — amending the documented decision to a fixed 30-minute model — -is a design decision, not a code fix. Left Open and surfaced for a follow-up -that spans CentralUI + Security, or a design-doc amendment. +Resolved 2026-05-16 (commit ``) — cross-module fix (CentralUI + +Security), explicitly authorized. Root cause confirmed against the source: +`AddCookie` (`ScadaLink.Security/ServiceCollectionExtensions.cs`) set neither +`ExpireTimeSpan` nor `SlidingExpiration`; `AuthEndpoints` stamped a fixed +`expires_at = UtcNow + 30 min` claim and a 30-minute absolute cookie +`ExpiresUtc`; `SessionExpiry.razor` scheduled one hard redirect at that fixed +instant — a hard 30-minute cap, no sliding renewal, no 15-minute component. + +**What was implemented — sliding session window.** ASP.NET Core cookie +authentication exposes a single `ExpireTimeSpan` plus a `SlidingExpiration` +flag; it cannot natively model *both* a 15-minute sliding token *and* a separate +30-minute absolute idle cap. The faithful interpretation implemented: the cookie +session window **is** the idle timeout. `AddSecurity` now post-configures the +cookie options with `ExpireTimeSpan = TimeSpan.FromMinutes(SecurityOptions.IdleTimeoutMinutes)` +(default 30, bound from `appsettings` via the existing options pattern, not +hard-coded) and `SlidingExpiration = true`. The middleware therefore re-issues +the cookie on activity once past the halfway mark of the window: an active user +is continually renewed, an idle user is signed out after the 30-minute idle +timeout — exactly the documented "sliding refresh, 30-minute idle timeout". The +separate 15-minute `JwtExpiryMinutes` governs the lifetime of the *embedded JWT* +itself (`JwtTokenService`) — a distinct layer from the cookie session window; +it is not, and per the ASP.NET cookie model cannot be, a second independent +sliding window inside the same cookie. `AuthEndpoints` no longer imposes a +contradictory absolute cap: the `expires_at` claim and the manual cookie +`ExpiresUtc` were removed, and a new `BuildSignInProperties()` helper sets only +`IsPersistent = true` (no `ExpiresUtc`, `AllowRefresh` left unset) so the +middleware owns expiry. `SessionExpiry.razor` no longer reads a fixed +login-time deadline (the `expires_at` claim is gone) and no longer hard-redirects +at a fixed instant: it now polls the authentication state on a recurring +interval and redirects to `/login` only once the sliding cookie has actually +lapsed server-side — so an active user is never logged out mid-session. + +Regression tests fail against the pre-fix code and pass after. Security: +`AddSecurity_AuthCookie_UsesSlidingExpiration`, +`AddSecurity_AuthCookie_ExpireTimeSpanMatchesIdleTimeout` (pre-fix +`ExpireTimeSpan` was the 14-day default — confirmed failing), and +`AddSecurity_AuthCookie_ExpireTimeSpanIsConfigurable` (pins the options-pattern +binding). CentralUI: `SessionExpiryPolicyTests.BuildSignInProperties_DoesNotSetFixedAbsoluteExpiry`, +`..._IsPersistent`, `..._AllowsSlidingRefresh` pin that the login sign-in no +longer imposes a fixed absolute cap. `dotnet build ScadaLink.slnx` clean; +`tests/ScadaLink.Security.Tests` 57 passed, `tests/ScadaLink.CentralUI.Tests` +254 passed. ### CentralUI-006 — Deployment status page polls every 10s despite the documented SignalR-push design diff --git a/src/ScadaLink.CentralUI/Auth/AuthEndpoints.cs b/src/ScadaLink.CentralUI/Auth/AuthEndpoints.cs index 2cd48d1..0bb09f0 100644 --- a/src/ScadaLink.CentralUI/Auth/AuthEndpoints.cs +++ b/src/ScadaLink.CentralUI/Auth/AuthEndpoints.cs @@ -44,15 +44,17 @@ public static class AuthEndpoints // Map LDAP groups to roles var roleMappingResult = await roleMapper.MapGroupsToRolesAsync(authResult.Groups ?? []); - var expiresAt = DateTimeOffset.UtcNow.AddMinutes(30); - - // Build claims from LDAP auth + role mapping + // Build claims from LDAP auth + role mapping. + // CentralUI-005: no fixed "expires_at" absolute-cap claim is stamped + // — session expiry is owned by the cookie middleware's sliding window + // (ScadaLink.Security AddCookie: ExpireTimeSpan = idle timeout, + // SlidingExpiration = true). A frozen absolute claim would contradict + // the documented sliding-refresh policy. var claims = new List { new(ClaimTypes.Name, authResult.Username ?? username), new(JwtTokenService.DisplayNameClaimType, authResult.DisplayName ?? username), new(JwtTokenService.UsernameClaimType, authResult.Username ?? username), - new("expires_at", expiresAt.ToUnixTimeSeconds().ToString()), }; foreach (var role in roleMappingResult.Roles) @@ -74,11 +76,7 @@ public static class AuthEndpoints await context.SignInAsync( CookieAuthenticationDefaults.AuthenticationScheme, principal, - new AuthenticationProperties - { - IsPersistent = true, - ExpiresUtc = expiresAt - }); + BuildSignInProperties()); context.Response.Redirect("/"); }).DisableAntiforgery(); @@ -138,4 +136,22 @@ public static class AuthEndpoints return endpoints; } + + /// + /// Builds the for the login sign-in. + /// CentralUI-005: deliberately does not set . + /// Session expiry is owned by the cookie authentication middleware's sliding + /// window (configured in ScadaLink.Security's AddCookie: + /// ExpireTimeSpan = the idle timeout, SlidingExpiration = true). + /// Setting a fixed ExpiresUtc here would re-impose a hard absolute cap + /// that overrides the sliding window and contradicts the documented + /// "sliding refresh, 30-minute idle timeout" policy. + /// is true so the cookie survives a browser restart within the idle window; + /// is left unset (null) + /// so the middleware is free to slide the expiry on activity. + /// + public static AuthenticationProperties BuildSignInProperties() => new() + { + IsPersistent = true + }; } diff --git a/src/ScadaLink.CentralUI/Components/Shared/SessionExpiry.razor b/src/ScadaLink.CentralUI/Components/Shared/SessionExpiry.razor index 6dc4dbb..09f1e48 100644 --- a/src/ScadaLink.CentralUI/Components/Shared/SessionExpiry.razor +++ b/src/ScadaLink.CentralUI/Components/Shared/SessionExpiry.razor @@ -3,37 +3,62 @@ @inject NavigationManager Navigation @code { + // CentralUI-005: session expiry is a sliding window owned by the cookie + // authentication middleware (ScadaLink.Security AddCookie: + // ExpireTimeSpan = idle timeout, SlidingExpiration = true). An active user's + // cookie is continually renewed; an idle user's cookie lapses after the idle + // timeout. There is therefore no fixed login-time deadline to redirect at — + // the old code read an "expires_at" claim and scheduled a single hard + // redirect, which both contradicted the sliding policy and logged active + // users out mid-session. + // + // Instead this component polls the authentication state on a recurring + // interval. While the session is still valid it does nothing; once the + // sliding cookie has expired (the server-side idle cutoff has been reached) + // the next poll observes an unauthenticated principal and redirects to the + // login page. Re-checking the state is itself circuit activity, so this poll + // alone never keeps a truly idle session alive — only genuine user activity + // refreshes the cookie before it lapses. + + /// How often the session validity is re-checked. + internal static readonly TimeSpan PollInterval = TimeSpan.FromMinutes(1); + private CancellationTokenSource? _cts; - protected override async Task OnInitializedAsync() + protected override void OnInitialized() { // The login page uses the same layout, so this component renders there - // too. Redirecting /login → /login would loop ("too many redirects"). + // too. Polling/redirecting on /login → /login would loop. var path = Navigation.ToBaseRelativePath(Navigation.Uri); if (path.StartsWith("login", StringComparison.OrdinalIgnoreCase)) return; - var auth = await AuthStateProvider.GetAuthenticationStateAsync(); - if (auth.User.Identity?.IsAuthenticated != true) return; - - var claim = auth.User.FindFirst("expires_at")?.Value; - if (!long.TryParse(claim, out var unix)) return; - - var remaining = DateTimeOffset.FromUnixTimeSeconds(unix) - DateTimeOffset.UtcNow; - if (remaining <= TimeSpan.Zero) - { - Navigation.NavigateTo("/login", forceLoad: true); - return; - } - _cts = new CancellationTokenSource(); - _ = ScheduleRedirectAsync(remaining + TimeSpan.FromSeconds(2), _cts.Token); + _ = PollSessionAsync(_cts.Token); } - private async Task ScheduleRedirectAsync(TimeSpan delay, CancellationToken token) + private async Task PollSessionAsync(CancellationToken token) { - try { await Task.Delay(delay, token); } - catch (TaskCanceledException) { return; } - await InvokeAsync(() => Navigation.NavigateTo("/login", forceLoad: true)); + while (!token.IsCancellationRequested) + { + try { await Task.Delay(PollInterval, token); } + catch (TaskCanceledException) { return; } + + AuthenticationState auth; + try + { + auth = await AuthStateProvider.GetAuthenticationStateAsync(); + } + catch (ObjectDisposedException) + { + return; + } + + if (auth.User.Identity?.IsAuthenticated != true) + { + await InvokeAsync(() => Navigation.NavigateTo("/login", forceLoad: true)); + return; + } + } } public void Dispose() diff --git a/src/ScadaLink.Security/ServiceCollectionExtensions.cs b/src/ScadaLink.Security/ServiceCollectionExtensions.cs index 4e0d25b..954feba 100644 --- a/src/ScadaLink.Security/ServiceCollectionExtensions.cs +++ b/src/ScadaLink.Security/ServiceCollectionExtensions.cs @@ -1,5 +1,6 @@ using Microsoft.AspNetCore.Authentication.Cookies; using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Options; namespace ScadaLink.Security; @@ -25,6 +26,28 @@ public static class ServiceCollectionExtensions options.Cookie.SecurePolicy = Microsoft.AspNetCore.Http.CookieSecurePolicy.Always; }); + // CentralUI-005: configure the cookie session as a sliding window so the + // code matches the documented policy ("sliding refresh, 30-minute idle + // timeout"). ASP.NET cookie auth exposes a single ExpireTimeSpan plus a + // SlidingExpiration flag — it cannot natively model a 15-minute sliding + // token AND a separate 30-minute absolute idle cap. The faithful + // interpretation: the cookie window IS the idle timeout + // (SecurityOptions.IdleTimeoutMinutes, default 30) and SlidingExpiration + // renews it on activity (the middleware re-issues the cookie once past + // the halfway mark of the window). An active user is therefore kept + // signed in; an idle user is signed out after the idle timeout. The + // 15-minute JwtExpiryMinutes governs the lifetime of the embedded JWT + // itself (see JwtTokenService) — a separate layer from the cookie + // session window. Bound here via PostConfigure so SecurityOptions + // (configured by the Host after AddSecurity) is honoured. + services.AddOptions(CookieAuthenticationDefaults.AuthenticationScheme) + .Configure>((cookieOptions, securityOptions) => + { + var idleMinutes = securityOptions.Value.IdleTimeoutMinutes; + cookieOptions.ExpireTimeSpan = TimeSpan.FromMinutes(idleMinutes); + cookieOptions.SlidingExpiration = true; + }); + services.AddScadaLinkAuthorization(); return services; diff --git a/tests/ScadaLink.CentralUI.Tests/Auth/SessionExpiryPolicyTests.cs b/tests/ScadaLink.CentralUI.Tests/Auth/SessionExpiryPolicyTests.cs new file mode 100644 index 0000000..d8f1789 --- /dev/null +++ b/tests/ScadaLink.CentralUI.Tests/Auth/SessionExpiryPolicyTests.cs @@ -0,0 +1,46 @@ +using Microsoft.AspNetCore.Authentication; +using ScadaLink.CentralUI.Auth; + +namespace ScadaLink.CentralUI.Tests.Auth; + +/// +/// Regression tests for CentralUI-005. AuthEndpoints previously stamped a +/// fixed expires_at = UtcNow + 30 min claim and a 30-minute absolute cookie +/// ExpiresUtc with no sliding refresh, contradicting the documented +/// "sliding refresh, 30-minute idle timeout" policy. The login handler must now +/// build that let the cookie middleware +/// own expiry (sliding window) rather than imposing a contradictory fixed +/// absolute cap. +/// +public class SessionExpiryPolicyTests +{ + [Fact] + public void BuildSignInProperties_DoesNotSetFixedAbsoluteExpiry() + { + var props = AuthEndpoints.BuildSignInProperties(); + + // A fixed ExpiresUtc would re-introduce the hard 30-minute cap that + // overrides the middleware's sliding window. Expiry must be owned by + // the cookie middleware (ExpireTimeSpan + SlidingExpiration). + Assert.Null(props.ExpiresUtc); + } + + [Fact] + public void BuildSignInProperties_IsPersistent() + { + var props = AuthEndpoints.BuildSignInProperties(); + + Assert.True(props.IsPersistent); + } + + [Fact] + public void BuildSignInProperties_AllowsSlidingRefresh() + { + var props = AuthEndpoints.BuildSignInProperties(); + + // AllowRefresh left null/true lets the cookie middleware slide the + // expiry on activity. A false value would freeze the session to an + // absolute cap — the bug this finding pins. + Assert.NotEqual(false, props.AllowRefresh); + } +} diff --git a/tests/ScadaLink.Security.Tests/SecurityTests.cs b/tests/ScadaLink.Security.Tests/SecurityTests.cs index a044913..9632719 100644 --- a/tests/ScadaLink.Security.Tests/SecurityTests.cs +++ b/tests/ScadaLink.Security.Tests/SecurityTests.cs @@ -423,6 +423,63 @@ public class SecurityReviewRegressionTests Assert.True(cookieOptions.Cookie.HttpOnly); } + // --- CentralUI-005: cookie auth must use a sliding session window --- + // Documented policy (CLAUDE.md Security & Auth): sliding refresh with a + // 30-minute idle timeout. The cookie middleware must enable SlidingExpiration + // so an active session is renewed on activity and an idle session expires. + + [Fact] + public void AddSecurity_AuthCookie_UsesSlidingExpiration() + { + var services = new ServiceCollection(); + services.AddLogging(); + services.AddDataProtection(); + services.AddSecurity(); + + using var provider = services.BuildServiceProvider(); + var cookieOptions = provider + .GetRequiredService>() + .Get(Microsoft.AspNetCore.Authentication.Cookies.CookieAuthenticationDefaults.AuthenticationScheme); + + Assert.True(cookieOptions.SlidingExpiration); + } + + [Fact] + public void AddSecurity_AuthCookie_ExpireTimeSpanMatchesIdleTimeout() + { + var services = new ServiceCollection(); + services.AddLogging(); + services.AddDataProtection(); + services.AddSecurity(); + // The idle timeout drives the cookie's expiry window. + services.Configure(o => o.IdleTimeoutMinutes = 30); + + using var provider = services.BuildServiceProvider(); + var cookieOptions = provider + .GetRequiredService>() + .Get(Microsoft.AspNetCore.Authentication.Cookies.CookieAuthenticationDefaults.AuthenticationScheme); + + Assert.Equal(TimeSpan.FromMinutes(30), cookieOptions.ExpireTimeSpan); + } + + [Fact] + public void AddSecurity_AuthCookie_ExpireTimeSpanIsConfigurable() + { + var services = new ServiceCollection(); + services.AddLogging(); + services.AddDataProtection(); + services.AddSecurity(); + services.Configure(o => o.IdleTimeoutMinutes = 45); + + using var provider = services.BuildServiceProvider(); + var cookieOptions = provider + .GetRequiredService>() + .Get(Microsoft.AspNetCore.Authentication.Cookies.CookieAuthenticationDefaults.AuthenticationScheme); + + Assert.Equal(TimeSpan.FromMinutes(45), cookieOptions.ExpireTimeSpan); + Assert.True(cookieOptions.SlidingExpiration); + } + // --- Security-001: StartTLS transport must be reachable --- [Fact]