From a6265e8656a9b6d0ef735708e8c487c273834c6e Mon Sep 17 00:00:00 2001 From: Frank Tovar Date: Thu, 5 Feb 2026 16:28:22 +0100 Subject: [PATCH] Switch to signed cookie auth and stop leaking player IDs --- Endpoints/AuthEndpoints.cs | 9 ++- Endpoints/EndpointHelpers.cs | 24 ++++++-- Endpoints/ResultsEndpoints.cs | 6 +- Endpoints/StateEndpoints.cs | 12 ++-- Endpoints/SuggestEndpoints.cs | 18 +++--- Endpoints/VoteEndpoints.cs | 8 ++- IIS.md | 1 + Infrastructure/PlayerIdentityExtensions.cs | 72 ++++++---------------- Program.cs | 28 ++++++++- TASKS.md | 2 +- wwwroot/js/data.js | 2 +- wwwroot/js/ui.js | 2 +- 12 files changed, 100 insertions(+), 84 deletions(-) diff --git a/Endpoints/AuthEndpoints.cs b/Endpoints/AuthEndpoints.cs index a5bfdaa..4f94097 100644 --- a/Endpoints/AuthEndpoints.cs +++ b/Endpoints/AuthEndpoints.cs @@ -3,7 +3,6 @@ using GameList.Data; using GameList.Domain; using GameList.Infrastructure; using Microsoft.AspNetCore.Mvc; -using Microsoft.AspNetCore.Http; using Microsoft.EntityFrameworkCore; namespace GameList.Endpoints; @@ -59,7 +58,7 @@ public static class AuthEndpoints db.Players.Add(player); await db.SaveChangesAsync(); - PlayerIdentityExtensions.IssuePlayerCookie(ctx, player.Id, player.Username); + await PlayerIdentityExtensions.SignInPlayerAsync(ctx, player); return Results.Ok(new { player.Id, player.Username, player.DisplayName, player.IsAdmin }); }); @@ -84,14 +83,14 @@ public static class AuthEndpoints player.LastLoginAt = DateTimeOffset.UtcNow; await db.SaveChangesAsync(); - PlayerIdentityExtensions.IssuePlayerCookie(ctx, player.Id, player.Username); + await PlayerIdentityExtensions.SignInPlayerAsync(ctx, player); return Results.Ok(new { player.Id, player.Username, player.DisplayName, player.IsAdmin }); }); - group.MapPost("/logout", (HttpContext ctx) => + group.MapPost("/logout", async (HttpContext ctx) => { - PlayerIdentityExtensions.ClearPlayerCookie(ctx); + await PlayerIdentityExtensions.SignOutPlayerAsync(ctx); return Results.NoContent(); }); } diff --git a/Endpoints/EndpointHelpers.cs b/Endpoints/EndpointHelpers.cs index ff0dc2e..60ef54d 100644 --- a/Endpoints/EndpointHelpers.cs +++ b/Endpoints/EndpointHelpers.cs @@ -3,6 +3,7 @@ using GameList.Data; using GameList.Domain; using Microsoft.AspNetCore.Mvc; using Microsoft.EntityFrameworkCore; +using System.Security.Claims; namespace GameList.Endpoints; @@ -10,12 +11,24 @@ internal static class EndpointHelpers { public static async Task GetAuthenticatedPlayer(HttpContext ctx, AppDbContext db) { - if (!ctx.Items.TryGetValue(Infrastructure.PlayerIdentityExtensions.PlayerCookieName, out var value) || value is not Guid playerId) + if (ctx?.User?.Identity?.IsAuthenticated != true) { return null; } + var idValue = ctx.User.FindFirstValue(ClaimTypes.NameIdentifier); + if (string.IsNullOrWhiteSpace(idValue) || !Guid.TryParse(idValue, out var playerId)) + { + // Auth cookie is present but malformed; clear and reject. + await Infrastructure.PlayerIdentityExtensions.SignOutPlayerAsync(ctx); + return null; + } + var existing = await db.Players.FindAsync(playerId); + if (existing is null) + { + await Infrastructure.PlayerIdentityExtensions.SignOutPlayerAsync(ctx); + } return existing; } @@ -43,7 +56,11 @@ internal static class EndpointHelpers player.VotesFinal = false; } - await db.SaveChangesAsync(); + var changed = db.ChangeTracker.HasChanges(); + if (changed) + { + await db.SaveChangesAsync(); + } return player.CurrentPhase; } @@ -152,8 +169,7 @@ internal static class EndpointHelpers var player = await GetAuthenticatedPlayer(ctx, db); if (player?.IsAdmin == true) return true; - var provided = ctx.Request.Headers["X-Admin-Key"].FirstOrDefault() - ?? ctx.Request.Query["key"].FirstOrDefault(); + var provided = ctx.Request.Headers["X-Admin-Key"].FirstOrDefault(); var expected = config["ADMIN_PASSWORD"]; return !string.IsNullOrWhiteSpace(expected) && provided == expected; } diff --git a/Endpoints/ResultsEndpoints.cs b/Endpoints/ResultsEndpoints.cs index a43d2a2..c418ede 100644 --- a/Endpoints/ResultsEndpoints.cs +++ b/Endpoints/ResultsEndpoints.cs @@ -8,8 +8,10 @@ public static class ResultsEndpoints { public static void MapResultsEndpoints(this IEndpointRouteBuilder app) { - app.MapGet( - "/api/results", + var group = app.MapGroup("/api/results").RequireAuthorization(); + + group.MapGet( + "/", async (HttpContext ctx, AppDbContext db) => { var player = await EndpointHelpers.GetAuthenticatedPlayer(ctx, db); diff --git a/Endpoints/StateEndpoints.cs b/Endpoints/StateEndpoints.cs index 9ee5ee3..f2acea5 100644 --- a/Endpoints/StateEndpoints.cs +++ b/Endpoints/StateEndpoints.cs @@ -10,7 +10,9 @@ public static class StateEndpoints { public static void MapStateEndpoints(this IEndpointRouteBuilder app) { - app.MapGet("/api/state", async (HttpContext ctx, AppDbContext db) => + var group = app.MapGroup("/api").RequireAuthorization(); + + group.MapGet("/state", async (HttpContext ctx, AppDbContext db) => { var player = await EndpointHelpers.GetAuthenticatedPlayer(ctx, db); if (player is null) return Results.Unauthorized(); @@ -31,7 +33,7 @@ public static class StateEndpoints return Results.Ok(summary); }); - app.MapGet("/api/me", async (HttpContext ctx, AppDbContext db) => + group.MapGet("/me", async (HttpContext ctx, AppDbContext db) => { var player = await EndpointHelpers.GetAuthenticatedPlayer(ctx, db); if (player is null) return Results.Unauthorized(); @@ -39,7 +41,7 @@ public static class StateEndpoints return Results.Ok(new { player.Id, player.DisplayName, player.Username, player.IsAdmin, CurrentPhase = phase, player.VotesFinal, player.HasJoker }); }); - app.MapPost("/api/me/phase/next", async (HttpContext ctx, AppDbContext db, IConfiguration config) => + group.MapPost("/me/phase/next", async (HttpContext ctx, AppDbContext db, IConfiguration config) => { var player = await EndpointHelpers.GetAuthenticatedPlayer(ctx, db); if (player is null) return Results.Unauthorized(); @@ -59,7 +61,7 @@ public static class StateEndpoints return Results.Ok(new { player.CurrentPhase, appState.ResultsOpen }); }); - app.MapPost("/api/me/phase/prev", async (HttpContext ctx, AppDbContext db, IConfiguration config) => + group.MapPost("/me/phase/prev", async (HttpContext ctx, AppDbContext db, IConfiguration config) => { var player = await EndpointHelpers.GetAuthenticatedPlayer(ctx, db); if (player is null) return Results.Unauthorized(); @@ -76,7 +78,7 @@ public static class StateEndpoints return Results.Ok(new { player.CurrentPhase, appState.ResultsOpen }); }); - app.MapPost("/api/me/name", async ([FromBody] SetNameRequest request, HttpContext ctx, AppDbContext db) => + group.MapPost("/me/name", async ([FromBody] SetNameRequest request, HttpContext ctx, AppDbContext db) => { var name = EndpointHelpers.TrimTo(request.Name, 16); if (string.IsNullOrWhiteSpace(name)) diff --git a/Endpoints/SuggestEndpoints.cs b/Endpoints/SuggestEndpoints.cs index f8a86a4..bb23a86 100644 --- a/Endpoints/SuggestEndpoints.cs +++ b/Endpoints/SuggestEndpoints.cs @@ -10,7 +10,9 @@ public static class SuggestEndpoints { public static void MapSuggestEndpoints(this IEndpointRouteBuilder app) { - app.MapGet("/api/suggestions/mine", async (HttpContext ctx, AppDbContext db) => + var group = app.MapGroup("/api/suggestions").RequireAuthorization(); + + group.MapGet("/mine", async (HttpContext ctx, AppDbContext db) => { var player = await EndpointHelpers.GetAuthenticatedPlayer(ctx, db); if (player is null) return Results.Unauthorized(); @@ -40,7 +42,7 @@ public static class SuggestEndpoints return Results.Ok(ordered); }); - app.MapPost("/api/suggestions", async ([FromBody] SuggestionRequest request, HttpContext ctx, AppDbContext db, IHttpClientFactory http) => + group.MapPost("/", async ([FromBody] SuggestionRequest request, HttpContext ctx, AppDbContext db, IHttpClientFactory http) => { if (string.IsNullOrWhiteSpace(request.Name) || request.Name.Length > 100) { @@ -103,7 +105,7 @@ public static class SuggestEndpoints return Results.Created($"/api/suggestions/{suggestion.Id}", new { suggestion.Id }); }); - app.MapDelete("/api/suggestions/{id:int}", async (int id, HttpContext ctx, AppDbContext db, IConfiguration config) => + group.MapDelete("/{id:int}", async (int id, HttpContext ctx, AppDbContext db, IConfiguration config) => { var player = await EndpointHelpers.GetAuthenticatedPlayer(ctx, db); if (player is null) return Results.Unauthorized(); @@ -132,7 +134,7 @@ public static class SuggestEndpoints return Results.NoContent(); }); - app.MapPut("/api/suggestions/{id:int}", async (int id, [FromBody] SuggestionRequest request, HttpContext ctx, AppDbContext db, IConfiguration config, IHttpClientFactory http) => + group.MapPut("/{id:int}", async (int id, [FromBody] SuggestionRequest request, HttpContext ctx, AppDbContext db, IConfiguration config, IHttpClientFactory http) => { var player = await EndpointHelpers.GetAuthenticatedPlayer(ctx, db); var isAdmin = await EndpointHelpers.IsAdmin(ctx, db, config); @@ -200,7 +202,7 @@ public static class SuggestEndpoints }); }); - app.MapGet("/api/suggestions/all", async (HttpContext ctx, AppDbContext db) => + group.MapGet("/all", async (HttpContext ctx, AppDbContext db) => { var player = await EndpointHelpers.GetAuthenticatedPlayer(ctx, db); if (player is null) return Results.Unauthorized(); @@ -213,7 +215,6 @@ public static class SuggestEndpoints .Select(s => new { s.Id, - s.PlayerId, s.Name, s.Genre, s.Description, @@ -224,7 +225,8 @@ public static class SuggestEndpoints s.MaxPlayers, Author = s.Player!.DisplayName, s.CreatedAt, - s.ParentSuggestionId + s.ParentSuggestionId, + IsOwner = s.PlayerId == player.Id }) .ToListAsync(); @@ -242,7 +244,6 @@ public static class SuggestEndpoints return new { s.Id, - s.PlayerId, s.Name, s.Genre, s.Description, @@ -253,6 +254,7 @@ public static class SuggestEndpoints s.MaxPlayers, s.Author, s.ParentSuggestionId, + s.IsOwner, LinkedIds = linkedIds, LinkedTitles = linkedIds .Where(id => nameLookup.ContainsKey(id)) diff --git a/Endpoints/VoteEndpoints.cs b/Endpoints/VoteEndpoints.cs index b93bbc1..d1b7162 100644 --- a/Endpoints/VoteEndpoints.cs +++ b/Endpoints/VoteEndpoints.cs @@ -10,7 +10,9 @@ public static class VoteEndpoints { public static void MapVoteEndpoints(this IEndpointRouteBuilder app) { - app.MapGet("/api/votes/mine", async (HttpContext ctx, AppDbContext db) => + var group = app.MapGroup("/api/votes").RequireAuthorization(); + + group.MapGet("/mine", async (HttpContext ctx, AppDbContext db) => { var player = await EndpointHelpers.GetAuthenticatedPlayer(ctx, db); if (player is null) return Results.Unauthorized(); @@ -25,7 +27,7 @@ public static class VoteEndpoints return Results.Ok(votes); }); - app.MapPost("/api/votes", async ([FromBody] VoteRequest request, HttpContext ctx, AppDbContext db) => + group.MapPost("/", async ([FromBody] VoteRequest request, HttpContext ctx, AppDbContext db) => { if (request.Score is < 0 or > 10) return Results.BadRequest(new { error = "Score must be between 0 and 10." }); @@ -77,7 +79,7 @@ public static class VoteEndpoints return Results.Ok(new { SuggestionIds = linkedIds, request.Score }); }); - app.MapPost("/api/votes/finalize", async ([FromBody] VoteFinalizeRequest request, HttpContext ctx, AppDbContext db) => + group.MapPost("/finalize", async ([FromBody] VoteFinalizeRequest request, HttpContext ctx, AppDbContext db) => { var player = await EndpointHelpers.GetAuthenticatedPlayer(ctx, db); if (player is null) return Results.Unauthorized(); diff --git a/IIS.md b/IIS.md index 14d1af7..4734dd9 100644 --- a/IIS.md +++ b/IIS.md @@ -14,6 +14,7 @@ - `ADMIN_PASSWORD=` - `BasePath=/vote` (only if the site is under a subfolder; omit for root) - Optional: enable stdout logging in `web.config` during troubleshooting only; disable afterward. +- Data protection keys are persisted to `App_Data/keys`; ensure this folder is deployed and writable so auth cookies stay valid across app pool recycles. ## Permissions - Grant modify rights to the app pool identity on `App_Data` (DB file + wal). diff --git a/Infrastructure/PlayerIdentityExtensions.cs b/Infrastructure/PlayerIdentityExtensions.cs index f819b93..51e3192 100644 --- a/Infrastructure/PlayerIdentityExtensions.cs +++ b/Infrastructure/PlayerIdentityExtensions.cs @@ -1,3 +1,7 @@ +using System.Security.Claims; +using GameList.Domain; +using Microsoft.AspNetCore.Authentication; +using Microsoft.AspNetCore.Authentication.Cookies; using Microsoft.AspNetCore.Diagnostics; namespace GameList.Infrastructure; @@ -5,65 +9,27 @@ namespace GameList.Infrastructure; public static class PlayerIdentityExtensions { public const string PlayerCookieName = "player"; - public const string PlayerUsernameCookieName = "player_username"; + public const string AdminClaim = "is_admin"; + public const string AdminPolicy = "AdminOnly"; - public static IApplicationBuilder UsePlayerIdentity(this IApplicationBuilder app) + public static async Task SignInPlayerAsync(HttpContext ctx, Player player) { - app.Use(async (ctx, next) => + var claims = new List { - var pathBase = ctx.Request.PathBase.HasValue ? ctx.Request.PathBase.Value : "/"; - var isHttps = string.Equals(ctx.Request.Scheme, "https", StringComparison.OrdinalIgnoreCase); - - Guid playerId; - if (!ctx.Request.Cookies.TryGetValue(PlayerCookieName, out var value) || !Guid.TryParse(value, out playerId)) - { - await next(); - return; - } - - IssuePlayerCookie(ctx, playerId); - - await next(); - }); - - return app; - } - - public static void IssuePlayerCookie(HttpContext ctx, Guid playerId, string? username = null) - { - var options = BuildCookieOptions(ctx); - ctx.Response.Cookies.Append(PlayerCookieName, playerId.ToString(), options); - if (!string.IsNullOrWhiteSpace(username)) - { - ctx.Response.Cookies.Append(PlayerUsernameCookieName, username, options); - } - ctx.Items[PlayerCookieName] = playerId; - } - - public static void ClearPlayerCookie(HttpContext ctx) - { - var options = BuildCookieOptions(ctx); - options.Expires = DateTimeOffset.UtcNow.AddDays(-1); - ctx.Response.Cookies.Append(PlayerCookieName, string.Empty, options); - ctx.Response.Cookies.Append(PlayerUsernameCookieName, string.Empty, options); - ctx.Items.Remove(PlayerCookieName); - } - - private static CookieOptions BuildCookieOptions(HttpContext ctx) - { - var pathBase = ctx.Request.PathBase.HasValue ? ctx.Request.PathBase.Value : "/"; - var isHttps = string.Equals(ctx.Request.Scheme, "https", StringComparison.OrdinalIgnoreCase); - return new CookieOptions - { - HttpOnly = true, - SameSite = SameSiteMode.Strict, - Secure = isHttps, - IsEssential = true, - Expires = DateTimeOffset.UtcNow.AddYears(1), - Path = pathBase + new Claim(ClaimTypes.NameIdentifier, player.Id.ToString()), + new Claim(ClaimTypes.Name, player.Username), + new Claim(AdminClaim, player.IsAdmin ? "true" : "false") }; + + var identity = new ClaimsIdentity(claims, CookieAuthenticationDefaults.AuthenticationScheme); + var principal = new ClaimsPrincipal(identity); + + await ctx.SignInAsync(CookieAuthenticationDefaults.AuthenticationScheme, principal); } + public static Task SignOutPlayerAsync(HttpContext ctx) + => ctx.SignOutAsync(CookieAuthenticationDefaults.AuthenticationScheme); + public static IApplicationBuilder UseGlobalExceptionLogging(this IApplicationBuilder app) { app.UseExceptionHandler(handler => diff --git a/Program.cs b/Program.cs index 9aa12ac..f54eaa6 100644 --- a/Program.cs +++ b/Program.cs @@ -1,6 +1,8 @@ using GameList.Data; using GameList.Endpoints; using GameList.Infrastructure; +using Microsoft.AspNetCore.Authentication.Cookies; +using Microsoft.AspNetCore.DataProtection; using Microsoft.AspNetCore.HttpOverrides; using Microsoft.Data.Sqlite; using Microsoft.EntityFrameworkCore; @@ -10,6 +12,8 @@ var builder = WebApplication.CreateBuilder(args); var dataDirectory = Path.Combine(builder.Environment.ContentRootPath, "App_Data"); Directory.CreateDirectory(dataDirectory); +var dataProtectionDirectory = Path.Combine(dataDirectory, "keys"); +Directory.CreateDirectory(dataProtectionDirectory); var configuredConnection = builder.Configuration.GetConnectionString("Default"); var dbPath = Path.Combine(dataDirectory, "gamelist.db"); @@ -38,6 +42,27 @@ builder.Services.ConfigureHttpJsonOptions(options => }); builder.Services.AddHttpClient(); +builder.Services.AddDataProtection() + .PersistKeysToFileSystem(new DirectoryInfo(dataProtectionDirectory)); + +builder.Services.AddAuthentication(CookieAuthenticationDefaults.AuthenticationScheme) + .AddCookie(options => + { + options.Cookie.Name = PlayerIdentityExtensions.PlayerCookieName; + options.Cookie.HttpOnly = true; + options.Cookie.SameSite = SameSiteMode.Strict; + options.Cookie.SecurePolicy = builder.Environment.IsDevelopment() + ? CookieSecurePolicy.SameAsRequest + : CookieSecurePolicy.Always; + options.SlidingExpiration = true; + options.ExpireTimeSpan = TimeSpan.FromDays(30); + }); + +builder.Services.AddAuthorization(options => +{ + options.AddPolicy(PlayerIdentityExtensions.AdminPolicy, policy => + policy.RequireClaim(PlayerIdentityExtensions.AdminClaim, "true")); +}); var app = builder.Build(); @@ -53,6 +78,8 @@ if (!string.IsNullOrWhiteSpace(basePath)) } app.UseGlobalExceptionLogging(); +app.UseAuthentication(); +app.UseAuthorization(); // Ensure database and migrations are applied on startup using (var scope = app.Services.CreateScope()) @@ -63,7 +90,6 @@ using (var scope = app.Services.CreateScope()) app.UseDefaultFiles(); app.UseStaticFiles(); -app.UsePlayerIdentity(); app.MapHealthChecks(); app.MapAuthEndpoints(); diff --git a/TASKS.md b/TASKS.md index 55dbc8b..d2a3fbc 100644 --- a/TASKS.md +++ b/TASKS.md @@ -1,6 +1,6 @@ # TASKS - Review findings -- [ ] **Critical - Unsigned auth cookie allows impersonation** (`Infrastructure/PlayerIdentityExtensions.cs`, `Endpoints/EndpointHelpers.cs`, `wwwroot/js/api.js`): The `player` cookie stores a raw GUID and is trusted on every request; `/api/suggestions/all` exposes other players' IDs, so a user can copy an ID, set the cookie, and become that account (including admins). Move to ASP.NET Core cookie auth with signed/encrypted tickets or a server-side session table, and avoid exposing player IDs where not necessary. +- [x] **Critical - Unsigned auth cookie allows impersonation** (`Infrastructure/PlayerIdentityExtensions.cs`, `Endpoints/EndpointHelpers.cs`, `wwwroot/js/api.js`): The `player` cookie stores a raw GUID and is trusted on every request; `/api/suggestions/all` exposes other players' IDs, so a user can copy an ID, set the cookie, and become that account (including admins). Move to ASP.NET Core cookie auth with signed/encrypted tickets or a server-side session table, and avoid exposing player IDs where not necessary. - [ ] **Critical - Stored XSS via suggestion content/links** (`wwwroot/js/ui.js`, `wwwroot/index.html`, `Endpoints/SuggestEndpoints.cs`): Names, descriptions, and URLs are injected with `innerHTML` and unescaped links; `gameUrl`/`youtubeUrl` accept any scheme. A malicious suggestion can run script when viewed. Sanitize/escape on render (use `textContent`), validate URLs to http/https on the server, and consider HTML sanitization for descriptions. - [ ] **High - SSRF surface in screenshot validation** (`Endpoints/EndpointHelpers.IsReachableImageAsync`, `SuggestEndpoints.cs`): The server performs HEAD/GET requests to arbitrary user-provided URLs with minimal limits, enabling internal port probing and latency spikes. Restrict to allowlisted hosts or disable remote fetch; at minimum block private IPs/localhost, disallow redirects, and enforce tight size/time limits. - [ ] **High - Phase recalculation writes on every poll** (`Endpoints/EndpointHelpers.GetPhase`, `wwwroot/app.js`): `GetPhase` unconditionally calls `SaveChangesAsync`; the client polls `/api/state` every 4s, causing constant writes and SQLite WAL churn/locks even when nothing changes. Track a `changed` flag and persist only when state mutations occur; consider reducing poll frequency or using long polling/server push later. diff --git a/wwwroot/js/data.js b/wwwroot/js/data.js index b40743f..623f4a0 100644 --- a/wwwroot/js/data.js +++ b/wwwroot/js/data.js @@ -85,7 +85,6 @@ export function signatureSuggestions(list) { return JSON.stringify( list.map((s) => [ s.id, - s.playerId, s.name, s.genre, s.description, @@ -95,6 +94,7 @@ export function signatureSuggestions(list) { s.minPlayers, s.maxPlayers, s.parentSuggestionId, + s.isOwner, ]), ); } diff --git a/wwwroot/js/ui.js b/wwwroot/js/ui.js index b1f5630..fdee132 100644 --- a/wwwroot/js/ui.js +++ b/wwwroot/js/ui.js @@ -147,7 +147,7 @@ export function renderVotes() { state.myVotes.map((v) => [v.suggestionId, v.score]), ); sortByName(state.allSuggestions).forEach((s) => { - const canEdit = !!state.me?.isAdmin || s.playerId === state.me?.id; + const canEdit = !!state.me?.isAdmin || s.isOwner; const lockTitle = state.phase !== "Suggest" && !state.me?.isAdmin; const li = buildCard(s, { showAuthor: true,