diff --git a/Endpoints/EndpointHelpers.cs b/Endpoints/EndpointHelpers.cs index 60ef54d..d25586e 100644 --- a/Endpoints/EndpointHelpers.cs +++ b/Endpoints/EndpointHelpers.cs @@ -164,6 +164,13 @@ internal static class EndpointHelpers return data[8..].StartsWith(brandBytes); } + public static bool IsValidHttpUrl(string? url) + { + if (string.IsNullOrWhiteSpace(url)) return true; // empty is allowed + if (!Uri.TryCreate(url, UriKind.Absolute, out var uri)) return false; + return uri.Scheme is "http" or "https"; + } + public static async Task IsAdmin(HttpContext ctx, AppDbContext db, IConfiguration config) { var player = await GetAuthenticatedPlayer(ctx, db); diff --git a/Endpoints/SuggestEndpoints.cs b/Endpoints/SuggestEndpoints.cs index bb23a86..93faa8c 100644 --- a/Endpoints/SuggestEndpoints.cs +++ b/Endpoints/SuggestEndpoints.cs @@ -57,6 +57,10 @@ public static class SuggestEndpoints { return Results.BadRequest(new { error = "Screenshot URL could not be validated as an image." }); } + if (!EndpointHelpers.IsValidHttpUrl(request.GameUrl)) + return Results.BadRequest(new { error = "Game URL must be http or https." }); + if (!EndpointHelpers.IsValidHttpUrl(request.YoutubeUrl)) + return Results.BadRequest(new { error = "YouTube URL must be http or https." }); if (!ValidatePlayers(request.MinPlayers, request.MaxPlayers, out var playersError)) return Results.BadRequest(new { error = playersError }); @@ -160,6 +164,10 @@ public static class SuggestEndpoints { return Results.BadRequest(new { error = "Screenshot URL could not be validated as an image." }); } + if (!EndpointHelpers.IsValidHttpUrl(request.GameUrl)) + return Results.BadRequest(new { error = "Game URL must be http or https." }); + if (!EndpointHelpers.IsValidHttpUrl(request.YoutubeUrl)) + return Results.BadRequest(new { error = "YouTube URL must be http or https." }); if (!ValidatePlayers(request.MinPlayers, request.MaxPlayers, out var playersError)) return Results.BadRequest(new { error = playersError }); diff --git a/TASKS.md b/TASKS.md index 00e436e..b234338 100644 --- a/TASKS.md +++ b/TASKS.md @@ -1,7 +1,7 @@ # TASKS - Review findings - [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. +- [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. - [ ] **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. diff --git a/wwwroot/js/ui.js b/wwwroot/js/ui.js index fdee132..74b64d6 100644 --- a/wwwroot/js/ui.js +++ b/wwwroot/js/ui.js @@ -13,6 +13,25 @@ const truncate = (text, max) => { if (!text) return ""; return text.length > max ? `${text.slice(0, max - 1)}…` : text; }; +const escapeHtml = (value) => + (value ?? "") + .toString() + .replace(/&/g, "&") + .replace(//g, ">") + .replace(/"/g, """) + .replace(/'/g, "'"); +const safeUrl = (url) => { + if (!url) return null; + try { + const u = new URL(url); + if (u.protocol === "http:" || u.protocol === "https:") return u.href; + } catch { + return null; + } + return null; +}; +const cssEscapeUrl = (url) => url.replace(/['")\\]/g, "\\$&"); export function setAuthUI(isAuthed) { const main = document.querySelector("main"); @@ -264,25 +283,30 @@ export function renderResults() { const row = document.createElement("tr"); const podiumClass = rank === 1 ? "podium podium-1" : rank === 2 ? "podium podium-2" : rank === 3 ? "podium podium-3" : ""; row.className = podiumClass; + const safeName = escapeHtml(r.name); + const safeAuthor = escapeHtml(r.author ?? "—"); + const safeShot = safeUrl(r.screenshotUrl); + const safeGameUrl = safeUrl(r.gameUrl); + const safeYoutubeUrl = safeUrl(r.youtubeUrl); row.innerHTML = ` ${medal} - ${r.screenshotUrl ? `${r.name}` : ''} + ${safeShot ? `${safeName}` : ''}
- ${r.name} + ${safeName} ${renderLinkBadge(r)}
${buildResultMeta(r)}
- ${r.author ?? "—"} + ${safeAuthor || "—"} ${r.average?.toFixed ? r.average.toFixed(1) : r.average} ${formatVotes(r.votes)} ${formatMyVote(r.myVote)} - ${r.gameUrl ? `${t("results.link.site")}
` : ''} - ${r.youtubeUrl ? `${t("results.link.youtube")}` : ''} + ${safeGameUrl ? `${t("results.link.site")}
` : ''} + ${safeYoutubeUrl ? `${t("results.link.youtube")}` : ''} `; tbody.appendChild(row); @@ -301,7 +325,7 @@ function buildResultMeta(r) { const players = hasPlayers ? t("card.players", { min: r.minPlayers ?? "?", max: r.maxPlayers ?? "?" }) : null; - const bits = [r.genre, players].filter(Boolean); + const bits = [r.genre ? escapeHtml(r.genre) : null, players].filter(Boolean); if (bits.length === 0) return ""; return `
${bits.join(" • ")}
`; } @@ -324,6 +348,13 @@ export function buildCard( const card = document.createElement("article"); card.className = "game-card"; const hasImage = !!s.screenshotUrl; + const safeShot = safeUrl(s.screenshotUrl); + const nameText = escapeHtml(s.name); + const genreText = escapeHtml(s.genre); + const descText = escapeHtml(s.description); + const authorText = escapeHtml(s.author); + const safeGameUrl = safeUrl(s.gameUrl); + const safeYoutubeUrl = safeUrl(s.youtubeUrl); const linkedTitles = linkedPeerTitles(s); const linked = isLinked(s); const linkTooltip = linked @@ -331,11 +362,12 @@ export function buildCard( ? t("card.linkedWith", { names: linkedTitles.join(", ") }) : t("card.linked") : ""; + const linkTooltipSafe = escapeHtml(linkTooltip); const linkChip = linked - ? `` + ? `` : ""; - const visual = hasImage - ? `` + const visual = hasImage && safeShot + ? `` : `
`; const hasPlayers = s.minPlayers || s.maxPlayers; const players = hasPlayers @@ -346,36 +378,36 @@ export function buildCard( : ""; const genreAndPlayers = s.genre ? hasPlayers - ? `${s.genre} • ${players}` - : s.genre + ? `${genreText} • ${players}` + : genreText : hasPlayers ? players : undefined; - const hasExtraInfo = genreAndPlayers || s.gameUrl || s.youtubeUrl; + const hasExtraInfo = genreAndPlayers || safeGameUrl || safeYoutubeUrl; card.innerHTML = ` ${visual}
-

${s.name}

+

${nameText}

${linkChip} - ${showAuthor && s.author ? `${s.author}` : ""} + ${showAuthor && s.author ? `${authorText}` : ""} ${allowEdit ? `` : ""} ${allowDelete ? `` : ""}
${hasExtraInfo ? `

` : ""} ${genreAndPlayers ? genreAndPlayers : ""} - ${s.gameUrl ? `${t("card.site")}` : ""} - ${s.youtubeUrl ? `${t("card.youtube")}` : ""} + ${safeGameUrl ? `${t("card.site")}` : ""} + ${safeYoutubeUrl ? `${t("card.youtube")}` : ""} ${hasExtraInfo ? `

` : ""} - ${s.description ? `

${s.description}

` : ""} + ${s.description ? `

${descText}

` : ""}
`; if (hasImage) { const btn = card.querySelector(".card-visual"); - setupCardVisualHover(btn, s.screenshotUrl); - btn.addEventListener("click", () => openLightbox(s.screenshotUrl, s.name)); + setupCardVisualHover(btn, safeShot); + btn.addEventListener("click", () => openLightbox(safeShot, s.name)); } if (linked && state.me?.isAdmin) { const unlinkBtn = card.querySelector("[data-unlink]"); @@ -587,11 +619,12 @@ export function openNewSuggestionModal() { export function openLightbox(url, title) { const overlay = document.createElement("div"); overlay.className = "lightbox"; + const safeTitle = escapeHtml(title || ""); overlay.innerHTML = ` `; overlay.addEventListener("click", (e) => { @@ -728,10 +761,12 @@ function renderAdminVoteStatus() { state.adminVoteStatus.voters.forEach((v) => { const tr = document.createElement("tr"); const statusText = displayPlayerStatus(v); - const gamesTooltip = (v.suggestionTitles || []).join(", "); + const gamesTooltip = escapeHtml((v.suggestionTitles || []).join(", ")); + const nameText = escapeHtml(truncate(v.name, 28)); + const userText = escapeHtml(truncate(v.username, 24)); tr.innerHTML = ` - ${truncate(v.name, 28)} - ${truncate(v.username, 24)} + ${nameText} + ${userText} ${statusText} ${v.suggestionCount ?? 0} @@ -930,7 +965,7 @@ function linkTooltip(s) { function renderLinkBadge(s) { if (!isLinked(s)) return ""; - return `🔗`; + return `🔗`; } function buildLinkOptionLabel(s) {