Fix open-redirect token leak and related auth hardening
Security review fixes: - Constrain OAuth connect returnUrl to a site-relative path so the redeemable token_key can't be redirected off-domain (was a refresh- token leak / connection hijack) - Route all login redirects (entra/dev/local) through ToLocalReturnUrl, also closing a protocol-relative // open redirect in local-login - Neutralize CSV formula prefixes in both audit-log exporters via CsvSanitizer - Force Secure flag on the prod auth cookie (Always, not SameAsRequest) - Gate admin pages with an app_role-claim "Admin" policy instead of a render-time check Findings and rationale recorded in SECURITY-TODO.md. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
+22
-11
@@ -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");
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user