From c4a1775d7dfffbeaca8ec9c6be3b26913d08d842 Mon Sep 17 00:00:00 2001 From: Kawa Date: Thu, 11 Jun 2026 14:30:19 +0200 Subject: [PATCH] Harden auth, headers, and container per OWASP review - Add per-account lockout + IP rate limiter on local sign-in (A07) - Emit CSP and security headers on every response (A05) - Run container as non-root `app`, /data 0700 (A05/A02) - Stop reflecting raw token-endpoint body into redirect URL (A09) - Handle missing refresh_token in connect callback without a 500 Co-Authored-By: Claude Opus 4.8 (1M context) --- Core/Models/AppUser.cs | 13 +++++++ Dockerfile | 10 +++++ Infrastructure/OAuth/OAuthEndpoints.cs | 28 +++++++++++--- Program.cs | 53 +++++++++++++++++++++++++- SECURITY-TODO.md | 42 ++++++++++++++++++++ Services/Auth/UserService.cs | 39 ++++++++++++++++++- 6 files changed, 176 insertions(+), 9 deletions(-) diff --git a/Core/Models/AppUser.cs b/Core/Models/AppUser.cs index ab5eaea..6c7d65d 100644 --- a/Core/Models/AppUser.cs +++ b/Core/Models/AppUser.cs @@ -15,4 +15,17 @@ public class AppUser public DateTimeOffset CreatedAt { get; set; } = DateTimeOffset.UtcNow; public DateTimeOffset? LastLogin { get; set; } + + // ── Local-account brute-force lockout ─────────────────────────────────────── + // Consecutive failed password attempts and, once the threshold is hit, the UTC + // instant the account unlocks again. Only meaningful for AuthProvider.Local. + // A per-account counter (not just an IP rate limiter) is the control that holds + // up here: forwarded headers are trusted from any source, so an attacker who can + // rotate X-Forwarded-For would evade IP-based throttling but not this. + + /// Consecutive failed local-login attempts since the last success. + public int FailedLoginCount { get; set; } + + /// UTC instant the account unlocks; null when not locked. + public DateTimeOffset? LockoutEndUtc { get; set; } } diff --git a/Dockerfile b/Dockerfile index 301aa0f..00ef9ad 100644 --- a/Dockerfile +++ b/Dockerfile @@ -25,6 +25,16 @@ FROM base AS final WORKDIR /app COPY --from=build /app/publish . +# Run as the non-root `app` user shipped in the aspnet image (UID 1654) instead of root. +# /data holds the crown jewels (Data Protection keys, app-only certs, the user store), so +# create it owned by `app` with 0700 before declaring the volume — Docker seeds a fresh +# named volume from the image path's ownership/mode, so the running user can write it and +# other host users can't read the keys/certs at rest. +RUN mkdir -p /data \ + && chown -R app:app /app /data \ + && chmod 700 /data +USER app + # Volume for persistent data (profiles, settings, templates, logs, exports) VOLUME ["/data"] diff --git a/Infrastructure/OAuth/OAuthEndpoints.cs b/Infrastructure/OAuth/OAuthEndpoints.cs index 1174e57..dc0adb9 100644 --- a/Infrastructure/OAuth/OAuthEndpoints.cs +++ b/Infrastructure/OAuth/OAuthEndpoints.cs @@ -72,12 +72,18 @@ public static class OAuthEndpoints string? error_description, IOAuthFlowCache flowCache, IOptions opts, - IHttpClientFactory httpClientFactory) => + IHttpClientFactory httpClientFactory, + ILoggerFactory loggerFactory) => { + var log = loggerFactory.CreateLogger("SharepointToolbox.Web.OAuth.Connect"); + if (!string.IsNullOrEmpty(error)) { - var errMsg = Uri.EscapeDataString(error_description ?? error); - return Results.Redirect($"/?connect_error={errMsg}"); + // The provider's verbose error_description can carry correlation/trace ids and + // lands in the URL bar + proxy access logs. Log it server-side; surface only the + // short, safe OAuth error code (e.g. "access_denied") to the browser. + log.LogWarning("Connect callback returned error {Error}: {Description}", error, error_description); + return Results.Redirect($"/?connect_error={Uri.EscapeDataString(error)}"); } if (string.IsNullOrEmpty(code) || string.IsNullOrEmpty(state)) @@ -107,14 +113,24 @@ public static class OAuthEndpoints if (!resp.IsSuccessStatusCode) { - var msg = Uri.EscapeDataString($"Token exchange failed: {json}"); - return Results.Redirect($"/?connect_error={msg}"); + // The raw token-endpoint body can contain trace ids / claim hints — keep it out of + // the URL and the proxy logs. Record it server-side, redirect with a generic notice. + log.LogWarning("Token exchange failed ({Status}): {Body}", resp.StatusCode, json); + return Results.Redirect($"/?connect_error={Uri.EscapeDataString("Token exchange failed. Please try connecting again.")}"); } using var doc = JsonDocument.Parse(json); var root = doc.RootElement; var upn = ExtractUpnFromIdToken(root); - var refreshToken = root.GetProperty("refresh_token").GetString()!; + + // offline_access should yield a refresh_token; if the tenant/app withheld it the + // session can't be persisted. Fail cleanly instead of throwing a 500 + stack trace. + if (!root.TryGetProperty("refresh_token", out var refreshTokenEl) || + refreshTokenEl.GetString() is not { Length: > 0 } refreshToken) + { + log.LogWarning("Token response had no refresh_token for tenant {Tenant}.", flowState.TenantId); + return Results.Redirect($"/?connect_error={Uri.EscapeDataString("Sign-in did not return a refresh token. Please try again.")}"); + } var tokens = new SessionTokens { diff --git a/Program.cs b/Program.cs index d36e7bf..1cd32c4 100644 --- a/Program.cs +++ b/Program.cs @@ -6,6 +6,8 @@ using Microsoft.AspNetCore.Antiforgery; using Microsoft.AspNetCore.DataProtection; using Microsoft.AspNetCore.HttpOverrides; using Microsoft.AspNetCore.Identity; +using Microsoft.AspNetCore.RateLimiting; +using System.Threading.RateLimiting; using Microsoft.Extensions.Options; using Microsoft.IdentityModel.Protocols.OpenIdConnect; using Serilog; @@ -210,6 +212,25 @@ builder.Services.AddAuthorization(options => options.AddPolicy("Admin", p => p.RequireClaim("app_role", nameof(UserRole.Admin))); }); +// ── Rate limiting ─────────────────────────────────────────────────────────────── +// Volumetric defence on the sign-in endpoints, partitioned by client IP (RemoteIpAddress +// reflects X-Forwarded-For — UseForwardedHeaders runs first). This is the coarse layer; +// the per-account lockout in UserService is what holds up when XFF is spoofed/rotated, +// since forwarded headers are trusted from any source behind the proxy. +builder.Services.AddRateLimiter(options => +{ + options.RejectionStatusCode = StatusCodes.Status429TooManyRequests; + options.AddPolicy("login", httpContext => + RateLimitPartition.GetFixedWindowLimiter( + partitionKey: httpContext.Connection.RemoteIpAddress?.ToString() ?? "unknown", + factory: _ => new FixedWindowRateLimiterOptions + { + PermitLimit = 10, + Window = TimeSpan.FromMinutes(1), + QueueLimit = 0, + })); +}); + // ── Memory cache (used by OAuth flow cache) ─────────────────────────────────── builder.Services.AddMemoryCache(); builder.Services.AddHttpClient("oauth"); @@ -362,6 +383,33 @@ if (publicBaseUri is not null) } } +// ── Security response headers ─────────────────────────────────────────────────── +// Defence-in-depth on every response. CSP is tuned to this app: all scripts are external +// (_framework/blazor.web.js, js/app.js) so script-src can stay 'self' with no unsafe-*; +// the login page and the Blazor components use inline