diff --git a/Components/Pages/Admin/AuditLogs.razor b/Components/Pages/Admin/AuditLogs.razor index c9a28ff..764b2dd 100644 --- a/Components/Pages/Admin/AuditLogs.razor +++ b/Components/Pages/Admin/AuditLogs.razor @@ -1,5 +1,5 @@ @page "/admin/audit" -@attribute [Microsoft.AspNetCore.Authorization.Authorize] +@attribute [Microsoft.AspNetCore.Authorization.Authorize(Policy = "Admin")] @inject IAuditService AuditService @inject IUserContextAccessor UserContext @inject NavigationManager Nav diff --git a/Components/Pages/Admin/UserManagement.razor b/Components/Pages/Admin/UserManagement.razor index da66efa..360094c 100644 --- a/Components/Pages/Admin/UserManagement.razor +++ b/Components/Pages/Admin/UserManagement.razor @@ -1,5 +1,5 @@ @page "/admin/users" -@attribute [Microsoft.AspNetCore.Authorization.Authorize] +@attribute [Microsoft.AspNetCore.Authorization.Authorize(Policy = "Admin")] @inject IUserService UserService @inject IUserContextAccessor UserContext @inject IAuditService Audit diff --git a/Core/Helpers/StringExtensions.cs b/Core/Helpers/StringExtensions.cs index d703415..46cff8d 100644 --- a/Core/Helpers/StringExtensions.cs +++ b/Core/Helpers/StringExtensions.cs @@ -4,4 +4,18 @@ public static class StringExtensions { public static string? TrimOrNull(this string? s) => string.IsNullOrWhiteSpace(s) ? null : s.Trim(); + + /// + /// Returns only when it is a safe site-relative path, + /// otherwise "/". Rejects absolute URLs and protocol-relative paths ("//evil.com", + /// "/\evil.com") so a post-auth / post-connect redirect can never leave the app. + /// Used by every login and OAuth-connect redirect to prevent open redirects. + /// + public static string ToLocalReturnUrl(this string? returnUrl) + { + if (string.IsNullOrEmpty(returnUrl)) return "/"; + if (returnUrl[0] != '/') return "/"; // not site-relative + if (returnUrl.Length > 1 && (returnUrl[1] == '/' || returnUrl[1] == '\\')) return "/"; // protocol-relative + return returnUrl; + } } diff --git a/Infrastructure/OAuth/OAuthEndpoints.cs b/Infrastructure/OAuth/OAuthEndpoints.cs index 1ef0729..1174e57 100644 --- a/Infrastructure/OAuth/OAuthEndpoints.cs +++ b/Infrastructure/OAuth/OAuthEndpoints.cs @@ -4,6 +4,7 @@ using System.Text.Json; using Microsoft.AspNetCore.WebUtilities; using Microsoft.Extensions.Options; using SharepointToolbox.Web.Core.Config; +using SharepointToolbox.Web.Core.Helpers; using SharepointToolbox.Web.Core.Models; using SharepointToolbox.Web.Infrastructure.Persistence; using SharepointToolbox.Web.Services.Auth; @@ -51,7 +52,10 @@ public static class OAuthEndpoints TenantId = profile.TenantId, ClientId = profile.ClientId, SpHost = ExtractHost(profile.TenantUrl), - ReturnUrl = string.IsNullOrEmpty(returnUrl) ? "/" : returnUrl, + // Constrain to a site-relative path: the callback appends the redeemable + // token_key to this URL, so an external returnUrl would leak the client's + // SharePoint refresh token off-domain. + ReturnUrl = returnUrl.ToLocalReturnUrl(), IsRegistration = false, }); diff --git a/Program.cs b/Program.cs index fe10c37..d36e7bf 100644 --- a/Program.cs +++ b/Program.cs @@ -10,6 +10,7 @@ using Microsoft.Extensions.Options; using Microsoft.IdentityModel.Protocols.OpenIdConnect; using Serilog; using SharepointToolbox.Web.Core.Config; +using SharepointToolbox.Web.Core.Helpers; using SharepointToolbox.Web.Core.Models; using SharepointToolbox.Web.Infrastructure.Auth; using SharepointToolbox.Web.Infrastructure.OAuth; @@ -109,7 +110,11 @@ else // Auth state lives entirely in the browser cookie (Data Protection encrypted) options.SessionStore = null; options.Cookie.SameSite = SameSiteMode.Lax; - options.Cookie.SecurePolicy = CookieSecurePolicy.SameAsRequest; + // Always mark the auth cookie Secure in prod. The app sits behind a TLS-terminating + // proxy and forwarded headers are trusted from any source (proxy IP is unknown inside + // the container network), so SameAsRequest would let a spoofed X-Forwarded-Proto: http + // — or any direct plaintext hit — emit a non-Secure cookie. Always wins regardless. + options.Cookie.SecurePolicy = CookieSecurePolicy.Always; options.ExpireTimeSpan = TimeSpan.FromHours(8); options.SlidingExpiration = true; }) @@ -197,7 +202,13 @@ else }); } -builder.Services.AddAuthorization(); +// "Admin" policy checks the app_role claim value directly, rather than [Authorize(Roles=…)] +// — the local/dev sign-in identities don't set a ClaimTypes.Role claim, so a Roles check would +// silently deny local admins. Every identity (OIDC, local, dev) carries app_role. +builder.Services.AddAuthorization(options => +{ + options.AddPolicy("Admin", p => p.RequireClaim("app_role", nameof(UserRole.Admin))); +}); // ── Memory cache (used by OAuth flow cache) ─────────────────────────────────── builder.Services.AddMemoryCache(); @@ -401,7 +412,7 @@ if (isDev) CookieAuthenticationDefaults.AuthenticationScheme)); await ctx.SignInAsync(CookieAuthenticationDefaults.AuthenticationScheme, principal); - ctx.Response.Redirect(string.IsNullOrEmpty(returnUrl) ? "/" : returnUrl); + ctx.Response.Redirect(returnUrl.ToLocalReturnUrl()); }); } else @@ -411,7 +422,7 @@ else { var props = new AuthenticationProperties { - RedirectUri = string.IsNullOrEmpty(returnUrl) ? "/" : returnUrl + RedirectUri = returnUrl.ToLocalReturnUrl() }; await ctx.ChallengeAsync(OpenIdConnectDefaults.AuthenticationScheme, props); }); @@ -427,7 +438,7 @@ app.MapPost("/account/local-login", async (HttpContext ctx, IAntiforgery antifor var email = form["email"].ToString(); var password = form["password"].ToString(); var returnUrl = form["returnUrl"].ToString(); - var safeReturn = string.IsNullOrEmpty(returnUrl) || !returnUrl.StartsWith('/') ? "/" : returnUrl; + var safeReturn = returnUrl.ToLocalReturnUrl(); var user = await userService.ValidateLocalCredentialsAsync(email, password); if (user is null) @@ -500,13 +511,13 @@ app.MapGet("/audit/export", async (AuditRepository auditRepo, HttpContext ctx) = sb.AppendLine("Timestamp,UserEmail,UserDisplay,UserRole,Action,Client,Sites,Details"); foreach (var e in entries.OrderByDescending(x => x.Timestamp)) { - string Esc(string v) => v.Contains(',') || v.Contains('"') || v.Contains('\n') - ? $"\"{v.Replace("\"", "\"\"")}\"" : v; + // CsvSanitizer neutralizes spreadsheet formula prefixes (= + - @) on top of RFC 4180 + // quoting — audited fields (display name, client/site names, details) are user-controlled. sb.AppendLine(string.Join(",", - Esc(e.Timestamp.ToString("yyyy-MM-dd HH:mm:ss")), - Esc(e.UserEmail), Esc(e.UserDisplay), Esc(e.UserRole.ToString()), - Esc(e.Action), Esc(e.ClientName), - Esc(string.Join("; ", e.Sites)), Esc(e.Details))); + CsvSanitizer.Escape(e.Timestamp.ToString("yyyy-MM-dd HH:mm:ss")), + CsvSanitizer.Escape(e.UserEmail), CsvSanitizer.Escape(e.UserDisplay), CsvSanitizer.Escape(e.UserRole.ToString()), + CsvSanitizer.Escape(e.Action), CsvSanitizer.Escape(e.ClientName), + CsvSanitizer.Escape(string.Join("; ", e.Sites)), CsvSanitizer.Escape(e.Details))); } return Results.File(System.Text.Encoding.UTF8.GetBytes(sb.ToString()), "text/csv", "audit-log.csv"); }); diff --git a/SECURITY-TODO.md b/SECURITY-TODO.md new file mode 100644 index 0000000..177331f --- /dev/null +++ b/SECURITY-TODO.md @@ -0,0 +1,51 @@ +# Security findings + +Review date: 2026-06-11. Items 1–4 and 6 fixed the same day; #5 reviewed and accepted by design. + +--- + +## 1. [HIGH] ✅ FIXED — Open redirect in `/connect/initiate` leaked SharePoint session tokens + +**Was:** `returnUrl` from the query string was stored verbatim in the OAuth flow state and used as the final redirect target after the code exchange — with `token_key` (a 2-minute, redeemable handle to the connected client's refresh token) appended. `…?returnUrl=https://evil.com` leaked that handle off-domain, enabling connection hijack. + +**Fix:** `returnUrl` is now constrained to a site-relative path via the new `string.ToLocalReturnUrl()` helper before it is stored ([Infrastructure/OAuth/OAuthEndpoints.cs](Infrastructure/OAuth/OAuthEndpoints.cs)). The helper rejects absolute *and* protocol-relative (`//host`, `/\host`) URLs. + +--- + +## 2. [MEDIUM] ✅ FIXED — CSV formula injection in audit-log exports + +**Was:** Both audit CSV exporters quoted per RFC 4180 but didn't neutralize a leading `=` `+` `-` `@`, so user-controlled fields (display/client/site names, details) could execute as formulas in Excel. + +**Fix:** Both paths now use `CsvSanitizer.Escape` (which neutralizes formula prefixes): the `/audit/export` endpoint ([Program.cs](Program.cs)) and `AuditService.ExportCsvAsync` ([Services/Audit/AuditService.cs](Services/Audit/AuditService.cs)). The local `CsvEscape`/`Esc` duplicates were removed. + +--- + +## 3. [LOW-MEDIUM] ✅ FIXED — Spoofable `X-Forwarded-Proto` could yield a non-Secure auth cookie + +**Was:** Forwarded headers are trusted from any source (proxy IP unknown inside the container network), and the prod auth cookie used `SecurePolicy = SameAsRequest` — so a spoofed `X-Forwarded-Proto: http` or a direct plaintext hit could emit a non-Secure cookie. + +**Fix:** Prod auth cookie now uses `CookieSecurePolicy.Always` ([Program.cs](Program.cs)), so the Secure flag is set regardless of the (untrusted) forwarded scheme. The forwarded-headers trust config is unchanged (still required behind the proxy); the residual XFF concern is limited — client IP is not used for auth and is not recorded in the audit log. + +--- + +## 4. [LOW] ✅ FIXED — Open redirect in `/account/login/entra` and `/account/login/dev` + +**Was:** `returnUrl` was used directly as the post-auth redirect on the entra and dev sign-in paths; only local-login validated it (and even that check allowed protocol-relative `//evil.com`). + +**Fix:** All three sign-in redirects now route `returnUrl` through `ToLocalReturnUrl()` ([Program.cs](Program.cs)), which also closes the protocol-relative gap in the old local-login check. + +--- + +## 5. [LOW] ⚪ ACCEPTED (by design) — Authenticated users can download any export / report + +**Where:** `/export/download/{fileName}` and `/reports/download/{id}` ([Program.cs](Program.cs)). + +**Decision:** Not changed. The app has no per-user profile authorization anywhere — any tech may select and use any shared profile (see the "standard techs use profiles without sign-in" design), and `GeneratedReport` carries only a `ProfileId`, no owner. Reports are listed and shared *per client/profile*, not per user, so per-user download scoping would contradict the product model rather than fix a boundary. Both endpoints already require authentication and strip path traversal (`Path.GetFileName`). Revisit only if a per-tech profile ACL is introduced. + +--- + +## 6. [LOW] ✅ FIXED — Admin pages gated only by a render-time role check + +**Was:** `/admin/users` and `/admin/audit` used `@attribute [Authorize]` (any authenticated user) plus an in-markup `if (Role != Admin) return;`. + +**Fix:** Added an `Admin` authorization policy (`RequireClaim("app_role", "Admin")`) in [Program.cs](Program.cs) and both pages now use `[Authorize(Policy = "Admin")]` ([UserManagement.razor](Components/Pages/Admin/UserManagement.razor), [AuditLogs.razor](Components/Pages/Admin/AuditLogs.razor)). A claim-value policy (not `[Authorize(Roles=…)]`) was used deliberately: the local/dev sign-in identities don't set a `ClaimTypes.Role` claim, so a Roles check would have silently denied local admins. The in-component checks were kept as defense-in-depth. diff --git a/Services/Audit/AuditService.cs b/Services/Audit/AuditService.cs index 274ef9c..6686c9e 100644 --- a/Services/Audit/AuditService.cs +++ b/Services/Audit/AuditService.cs @@ -2,6 +2,7 @@ using System.Text; using Microsoft.AspNetCore.Authentication; using SharepointToolbox.Web.Core.Models; using SharepointToolbox.Web.Infrastructure.Persistence; +using SharepointToolbox.Web.Services.Export; using SharepointToolbox.Web.Services.Session; namespace SharepointToolbox.Web.Services.Audit; @@ -41,23 +42,18 @@ public class AuditService : IAuditService sb.AppendLine("Timestamp,UserEmail,UserDisplay,UserRole,Action,Client,Sites,Details"); foreach (var e in entries.OrderByDescending(x => x.Timestamp)) { + // CsvSanitizer adds spreadsheet formula-injection guards (= + - @) on top of + // RFC 4180 quoting; the user/display/client/site fields are user-controlled. sb.AppendLine(string.Join(",", - CsvEscape(e.Timestamp.ToLocalTime().ToString("yyyy-MM-dd HH:mm:ss")), - CsvEscape(e.UserEmail), - CsvEscape(e.UserDisplay), - CsvEscape(e.UserRole.ToString()), - CsvEscape(e.Action), - CsvEscape(e.ClientName), - CsvEscape(string.Join("; ", e.Sites)), - CsvEscape(e.Details))); + CsvSanitizer.Escape(e.Timestamp.ToLocalTime().ToString("yyyy-MM-dd HH:mm:ss")), + CsvSanitizer.Escape(e.UserEmail), + CsvSanitizer.Escape(e.UserDisplay), + CsvSanitizer.Escape(e.UserRole.ToString()), + CsvSanitizer.Escape(e.Action), + CsvSanitizer.Escape(e.ClientName), + CsvSanitizer.Escape(string.Join("; ", e.Sites)), + CsvSanitizer.Escape(e.Details))); } return sb.ToString(); } - - private static string CsvEscape(string value) - { - if (value.Contains(',') || value.Contains('"') || value.Contains('\n')) - return $"\"{value.Replace("\"", "\"\"")}\""; - return value; - } }