Switch to signed cookie auth and stop leaking player IDs

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();
         });
     }

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<Player?> 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()
+        var provided = ctx.Request.Headers["X-Admin-Key"].FirstOrDefault();
-                       ?? ctx.Request.Query["key"].FirstOrDefault();
         var expected = config["ADMIN_PASSWORD"];
         return !string.IsNullOrWhiteSpace(expected) && provided == expected;
     }

Endpoints/ResultsEndpoints.cs

@@ -8,8 +8,10 @@ public static class ResultsEndpoints
 {
     public static void MapResultsEndpoints(this IEndpointRouteBuilder app)
     {
-        app.MapGet(
+        var group = app.MapGroup("/api/results").RequireAuthorization();
-            "/api/results",
+
+        group.MapGet(
+            "/",
             async (HttpContext ctx, AppDbContext db) =>
             {
                 var player = await EndpointHelpers.GetAuthenticatedPlayer(ctx, db);

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))

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))

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();

IIS.md

@@ -14,6 +14,7 @@
 - `ADMIN_PASSWORD=<your-secret>`
 - `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).

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<Claim>
         {
-            var pathBase = ctx.Request.PathBase.HasValue ? ctx.Request.PathBase.Value : "/";
+            new Claim(ClaimTypes.NameIdentifier, player.Id.ToString()),
-            var isHttps = string.Equals(ctx.Request.Scheme, "https", StringComparison.OrdinalIgnoreCase);
+            new Claim(ClaimTypes.Name, player.Username),
-
+            new Claim(AdminClaim, player.IsAdmin ? "true" : "false")
-            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
         };
+
+        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 =>

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();

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.

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,
         ]),
     );
 }

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,