diff --git a/GameList.Tests/StateTests.cs b/GameList.Tests/StateTests.cs index 8c60511..5e70c77 100644 --- a/GameList.Tests/StateTests.cs +++ b/GameList.Tests/StateTests.cs @@ -213,6 +213,10 @@ public class StateTests var anon = factory.CreateClient(); var unauthorized = await anon.GetAsync("/api/state"); Assert.Equal(HttpStatusCode.Unauthorized, unauthorized.StatusCode); + var unauthorizedJson = await unauthorized.Content.ReadFromJsonAsync(); + Assert.Equal("Unauthorized", unauthorizedJson.GetProperty("title").GetString()); + Assert.Equal("Unauthorized", unauthorizedJson.GetProperty("detail").GetString()); + Assert.Equal("Unauthorized", unauthorizedJson.GetProperty("error").GetString()); var client = factory.CreateClientWithCookies(); await client.RegisterAsync("counting"); diff --git a/Program.cs b/Program.cs index 61d38b6..ffc5532 100644 --- a/Program.cs +++ b/Program.cs @@ -2,6 +2,7 @@ using GameList.Data; using GameList.Endpoints; using GameList.Infrastructure; using Microsoft.AspNetCore.Authentication.Cookies; +using Microsoft.AspNetCore.Mvc; using Microsoft.AspNetCore.DataProtection; using Microsoft.AspNetCore.HttpOverrides; using Microsoft.Data.Sqlite; @@ -54,16 +55,8 @@ builder.Services.AddAuthentication(CookieAuthenticationDefaults.AuthenticationSc options.ExpireTimeSpan = TimeSpan.FromDays(30); options.Events = new CookieAuthenticationEvents { - OnRedirectToLogin = ctx => - { - ctx.Response.StatusCode = StatusCodes.Status401Unauthorized; - return Task.CompletedTask; - }, - OnRedirectToAccessDenied = ctx => - { - ctx.Response.StatusCode = StatusCodes.Status401Unauthorized; - return Task.CompletedTask; - } + OnRedirectToLogin = ctx => WriteUnauthorizedChallengeAsync(ctx.HttpContext), + OnRedirectToAccessDenied = ctx => WriteUnauthorizedChallengeAsync(ctx.HttpContext) }; }); @@ -139,6 +132,26 @@ static ForwardedHeadersOptions BuildForwardedHeadersOptions(IConfiguration confi return options; } +static Task WriteUnauthorizedChallengeAsync(HttpContext context) +{ + context.Response.StatusCode = StatusCodes.Status401Unauthorized; + if (!context.Request.Path.StartsWithSegments("/api")) + return Task.CompletedTask; + + if (context.Response.HasStarted) + return Task.CompletedTask; + + context.Response.ContentType = "application/problem+json"; + var problem = new ProblemDetails + { + Status = StatusCodes.Status401Unauthorized, + Title = "Unauthorized", + Detail = "Unauthorized" + }; + problem.Extensions["error"] = "Unauthorized"; + return context.Response.WriteAsJsonAsync(problem); +} + static void UpdateIndexMetaBase(IWebHostEnvironment env, string basePath) { try diff --git a/REVIEW.md b/REVIEW.md index a65778b..c04dc6c 100644 --- a/REVIEW.md +++ b/REVIEW.md @@ -11,11 +11,7 @@ Active maintainability risks (priority order): - Cross-feature coupling still exists via shared mutable state usage across UI/data modules (`wwwroot/js/ui.js:180`, `wwwroot/js/ui.js:401`, `wwwroot/js/ui.js:622`, `wwwroot/js/data.js:82`). - Impact: high regression surface and expensive refactors even after removing global `window` bridges. -2. Unauthenticated 401 response shape is still framework-driven (Medium) -- Endpoint and filter unauthorized responses are standardized when app logic executes (`Infrastructure/AdminOnlyFilter.cs:15`, `Infrastructure/PhaseRequirementFilter.cs:15`, `Endpoints/SuggestEndpoints.cs:18`), but anonymous challenge responses remain middleware-controlled (`GameList.Tests/StateTests.cs:214`). -- Impact: clients must tolerate both app-produced problem payloads and framework challenge responses. - -3. Static analysis and frontend lint guardrails are still missing (Medium) +2. Static analysis and frontend lint guardrails are still missing (Medium) - CI currently gates restore/build/test only (`.github/workflows/ci.yml:23`-`.github/workflows/ci.yml:29`). - Impact: style drift and low-signal warnings can enter the codebase undetected. @@ -29,14 +25,6 @@ Active maintainability risks (priority order): - Effort / Risk: `L / Med`. - Dependencies (if any): none. -[P1] Unify client handling of unauthenticated 401 challenge responses -- Problem: Severity `Medium`, Category `API/Contracts`. Fully unauthenticated requests can still bypass application-level problem shaping. -- Evidence: challenge behavior is exercised in `GameList.Tests/StateTests.cs:214` while app-level unauthorized shaping is exercised in `GameList.Tests/AuthTests.cs:164`. -- Recommendation: either customize cookie-auth challenge response to RFC7807 or explicitly codify dual-shape handling contract in API docs and frontend error layer. -- Acceptance criteria (testable): single documented 401 contract path, with tests validating response shape and frontend behavior. -- Effort / Risk: `M / Med`. -- Dependencies (if any): none. - [P2] Add static analysis and JS lint/format guardrails - Problem: Severity `Medium`, Category `Tooling`. CI does not enforce analyzers or JS lint/format checks. - Evidence: `.github/workflows/ci.yml:23`-`.github/workflows/ci.yml:29`. @@ -56,9 +44,8 @@ Active maintainability risks (priority order): ## C) Suggested execution order 1. Decompose `ui.js` by feature and keep orchestration thin. -2. Normalize/declare unauthenticated 401 contract behavior. -3. Add analyzers + JS lint gates in CI. -4. Externalize i18n/FAQ assets. +2. Add analyzers + JS lint gates in CI. +3. Externalize i18n/FAQ assets. ## D) Guardrails