From 6b5f8a66c976539d7fcdb71245c6255edf49d805 Mon Sep 17 00:00:00 2001 From: Frank Tovar Date: Thu, 5 Feb 2026 17:07:37 +0100 Subject: [PATCH] Remove admin key support; admin must be authenticated --- Endpoints/AdminEndpoints.cs | 32 ++++++++++++++++---------------- Endpoints/EndpointHelpers.cs | 8 ++------ Endpoints/SuggestEndpoints.cs | 8 ++++---- TASKS.md | 2 +- 4 files changed, 23 insertions(+), 27 deletions(-) diff --git a/Endpoints/AdminEndpoints.cs b/Endpoints/AdminEndpoints.cs index 90e3067..2c24a83 100644 --- a/Endpoints/AdminEndpoints.cs +++ b/Endpoints/AdminEndpoints.cs @@ -13,9 +13,9 @@ public static class AdminEndpoints { var admin = app.MapGroup("/api/admin"); - admin.MapPost("/results", async ([FromBody] Contracts.ResultsOpenRequest request, HttpContext ctx, AppDbContext db, IConfiguration config) => + admin.MapPost("/results", async ([FromBody] Contracts.ResultsOpenRequest request, HttpContext ctx, AppDbContext db) => { - if (!await EndpointHelpers.IsAdmin(ctx, db, config)) return Results.Unauthorized(); + if (!await EndpointHelpers.IsAdmin(ctx, db)) return Results.Unauthorized(); var state = await db.AppState.FirstAsync(); state.ResultsOpen = request.ResultsOpen; @@ -36,9 +36,9 @@ public static class AdminEndpoints return Results.Ok(new { currentState.ResultsOpen, currentState.UpdatedAt }); }); - admin.MapGet("/vote-status", async (HttpContext ctx, AppDbContext db, IConfiguration config) => + admin.MapGet("/vote-status", async (HttpContext ctx, AppDbContext db) => { - if (!await EndpointHelpers.IsAdmin(ctx, db, config)) return Results.Unauthorized(); + if (!await EndpointHelpers.IsAdmin(ctx, db)) return Results.Unauthorized(); var voters = await db.Players .AsNoTracking() @@ -59,9 +59,9 @@ public static class AdminEndpoints return Results.Ok(new { voters, ready, waiting }); }); - admin.MapPost("/joker", async ([FromBody] GrantJokerRequest request, HttpContext ctx, AppDbContext db, IConfiguration config) => + admin.MapPost("/joker", async ([FromBody] GrantJokerRequest request, HttpContext ctx, AppDbContext db) => { - if (!await EndpointHelpers.IsAdmin(ctx, db, config)) return Results.Unauthorized(); + if (!await EndpointHelpers.IsAdmin(ctx, db)) return Results.Unauthorized(); var player = await db.Players.FirstOrDefaultAsync(p => p.Id == request.PlayerId); if (player is null) return Results.NotFound(new { error = "Player not found." }); @@ -76,9 +76,9 @@ public static class AdminEndpoints return Results.Ok(new { player.Id, player.HasJoker }); }); - admin.MapDelete("/players/{playerId:guid}", async (Guid playerId, HttpContext ctx, AppDbContext db, IConfiguration config) => + admin.MapDelete("/players/{playerId:guid}", async (Guid playerId, HttpContext ctx, AppDbContext db) => { - if (!await EndpointHelpers.IsAdmin(ctx, db, config)) return Results.Unauthorized(); + if (!await EndpointHelpers.IsAdmin(ctx, db)) return Results.Unauthorized(); var player = await db.Players .Include(p => p.Suggestions) @@ -111,10 +111,10 @@ public static class AdminEndpoints return Results.Ok(new { DeletedPlayerId = playerId }); }); - admin.MapPost("/link-suggestions", async ([FromBody] LinkSuggestionsRequest request, HttpContext ctx, AppDbContext db, IConfiguration config) => + admin.MapPost("/link-suggestions", async ([FromBody] LinkSuggestionsRequest request, HttpContext ctx, AppDbContext db) => { var player = await EndpointHelpers.GetAuthenticatedPlayer(ctx, db); - if (player is null || !await EndpointHelpers.IsAdmin(ctx, db, config)) return Results.Unauthorized(); + if (player is null || !await EndpointHelpers.IsAdmin(ctx, db)) return Results.Unauthorized(); var phase = await EndpointHelpers.GetPhase(db, player.Id); if (phase != Phase.Vote) @@ -183,10 +183,10 @@ public static class AdminEndpoints }); }); - admin.MapPost("/unlink-suggestions", async ([FromBody] UnlinkSuggestionsRequest request, HttpContext ctx, AppDbContext db, IConfiguration config) => + admin.MapPost("/unlink-suggestions", async ([FromBody] UnlinkSuggestionsRequest request, HttpContext ctx, AppDbContext db) => { var player = await EndpointHelpers.GetAuthenticatedPlayer(ctx, db); - if (player is null || !await EndpointHelpers.IsAdmin(ctx, db, config)) return Results.Unauthorized(); + if (player is null || !await EndpointHelpers.IsAdmin(ctx, db)) return Results.Unauthorized(); var phase = await EndpointHelpers.GetPhase(db, player.Id); if (phase != Phase.Vote) @@ -238,9 +238,9 @@ public static class AdminEndpoints }); }); - admin.MapPost("/reset", async (HttpContext ctx, AppDbContext db, IConfiguration config) => + admin.MapPost("/reset", async (HttpContext ctx, AppDbContext db) => { - if (!await EndpointHelpers.IsAdmin(ctx, db, config)) return Results.Unauthorized(); + if (!await EndpointHelpers.IsAdmin(ctx, db)) return Results.Unauthorized(); await db.Votes.ExecuteDeleteAsync(); await db.Suggestions.ExecuteDeleteAsync(); @@ -256,9 +256,9 @@ public static class AdminEndpoints return Results.Ok(new { Phase = Phase.Suggest, state.ResultsOpen, state.UpdatedAt }); }); - admin.MapPost("/factory-reset", async (HttpContext ctx, AppDbContext db, IConfiguration config) => + admin.MapPost("/factory-reset", async (HttpContext ctx, AppDbContext db) => { - if (!await EndpointHelpers.IsAdmin(ctx, db, config)) return Results.Unauthorized(); + if (!await EndpointHelpers.IsAdmin(ctx, db)) return Results.Unauthorized(); await using var tx = await db.Database.BeginTransactionAsync(); diff --git a/Endpoints/EndpointHelpers.cs b/Endpoints/EndpointHelpers.cs index 1b85e94..598344c 100644 --- a/Endpoints/EndpointHelpers.cs +++ b/Endpoints/EndpointHelpers.cs @@ -235,14 +235,10 @@ internal static class EndpointHelpers return uri.Scheme is "http" or "https"; } - public static async Task IsAdmin(HttpContext ctx, AppDbContext db, IConfiguration config) + public static async Task IsAdmin(HttpContext ctx, AppDbContext db) { var player = await GetAuthenticatedPlayer(ctx, db); - if (player?.IsAdmin == true) return true; - - var provided = ctx.Request.Headers["X-Admin-Key"].FirstOrDefault(); - var expected = config["ADMIN_PASSWORD"]; - return !string.IsNullOrWhiteSpace(expected) && provided == expected; + return player?.IsAdmin == true; } public static AppState NewAppState() => new() diff --git a/Endpoints/SuggestEndpoints.cs b/Endpoints/SuggestEndpoints.cs index ba47792..def8b21 100644 --- a/Endpoints/SuggestEndpoints.cs +++ b/Endpoints/SuggestEndpoints.cs @@ -109,11 +109,11 @@ public static class SuggestEndpoints return Results.Created($"/api/suggestions/{suggestion.Id}", new { suggestion.Id }); }); - group.MapDelete("/{id:int}", async (int id, HttpContext ctx, AppDbContext db, IConfiguration config) => + group.MapDelete("/{id:int}", async (int id, HttpContext ctx, AppDbContext db) => { var player = await EndpointHelpers.GetAuthenticatedPlayer(ctx, db); if (player is null) return Results.Unauthorized(); - var isAdmin = await EndpointHelpers.IsAdmin(ctx, db, config); + var isAdmin = await EndpointHelpers.IsAdmin(ctx, db); var phase = await EndpointHelpers.GetPhase(db, player.Id); if (!isAdmin && phase != Phase.Suggest) @@ -138,10 +138,10 @@ public static class SuggestEndpoints return Results.NoContent(); }); - group.MapPut("/{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, IHttpClientFactory http) => { var player = await EndpointHelpers.GetAuthenticatedPlayer(ctx, db); - var isAdmin = await EndpointHelpers.IsAdmin(ctx, db, config); + var isAdmin = await EndpointHelpers.IsAdmin(ctx, db); if (!isAdmin) { diff --git a/TASKS.md b/TASKS.md index 6c9fce7..3cebef1 100644 --- a/TASKS.md +++ b/TASKS.md @@ -2,7 +2,7 @@ - [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. - [x] **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. +- [x] **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. - [x] **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. - [ ] **Medium - Admin key accepted via query string** (`Endpoints/EndpointHelpers.IsAdmin`): Accepting `key` in the query leaks secrets via logs and referrers. Require the `X-Admin-Key` header (or authenticated admin account) only, and rate-limit/monitor attempts. - [x] **Medium - Brittle base path detection on the client** (`wwwroot/js/api.js`): The heuristic that picks the first path segment can double-prefix or break when the app is nested (e.g., `/picknplay/`). Prefer explicit `` or `` and drop the autodetect fallback.