xTr1m/GameList
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Escape rendered suggestion content and validate URLs

Browse Source
This commit is contained in:
Frank Tovar
2026-02-05 16:51:05 +01:00
parent d88469724a
commit 1d28ea6568
4 changed files with 76 additions and 26 deletions
Download Patch File Download Diff File Expand all files Collapse all files

7
Endpoints/EndpointHelpers.cs
View File

@@ -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<bool> IsAdmin(HttpContext ctx, AppDbContext db, IConfiguration config)
{
var player = await GetAuthenticatedPlayer(ctx, db);

8
Endpoints/SuggestEndpoints.cs
View File

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

2
TASKS.md
View File

@@ -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.

85
wwwroot/js/ui.js
View File

@@ -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, "&amp;")
.replace(/</g, "&lt;")
.replace(/>/g, "&gt;")
.replace(/"/g, "&quot;")
.replace(/'/g, "&#39;");
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 = `
<td class="rank-cell"><span class="medal">${medal}</span></td>
<td class="game-cell">
${r.screenshotUrl ? `<img class="thumb clickable-thumb" src="${r.screenshotUrl}" alt="${r.name}">` : ''}
${safeShot ? `<img class="thumb clickable-thumb" src="${safeShot}" alt="${safeName}">` : ''}
<div class="game-meta">
<div class="title-line">
<span class="title-text">${r.name}</span>
<span class="title-text">${safeName}</span>
${renderLinkBadge(r)}
</div>
${buildResultMeta(r)}
</div>
</td>
<td class="author-cell">${r.author ?? "—"}</td>
<td class="author-cell">${safeAuthor || "—"}</td>
<td>${r.average?.toFixed ? r.average.toFixed(1) : r.average}</td>
<td>${formatVotes(r.votes)}</td>
<td>${formatMyVote(r.myVote)}</td>
<td>
${r.gameUrl ? `<a class="link compact" href="${r.gameUrl}" target="_blank" rel="noopener">${t("results.link.site")}</a><br>` : ''}
${r.youtubeUrl ? `<a class="link compact" href="${r.youtubeUrl}" target="_blank" rel="noopener">${t("results.link.youtube")}</a>` : ''}
${safeGameUrl ? `<a class="link compact" href="${safeGameUrl}" target="_blank" rel="noopener">${t("results.link.site")}</a><br>` : ''}
${safeYoutubeUrl ? `<a class="link compact" href="${safeYoutubeUrl}" target="_blank" rel="noopener">${t("results.link.youtube")}</a>` : ''}
</td>
`;
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 `<div class="muted small">${bits.join(" • ")}</div>`;
}
@@ -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
? `<button class="chip icon link-chip${state.me?.isAdmin ? " link-chip-action" : ""}" data-unlink="${s.id}" type="button" title="${linkTooltip}">🔗</button>`
? `<button class="chip icon link-chip${state.me?.isAdmin ? " link-chip-action" : ""}" data-unlink="${s.id}" type="button" title="${linkTooltipSafe}">🔗</button>`
: "";
const visual = hasImage
? `<button class="card-visual" data-img="${s.screenshotUrl}" aria-label="${t("card.openScreenshot")}" style="background-image:url('${s.screenshotUrl}')"></button>`
const visual = hasImage && safeShot
? `<button class="card-visual" data-img="${safeShot}" aria-label="${t("card.openScreenshot")}" style="background-image:url('${cssEscapeUrl(safeShot)}')"></button>`
: `<div class="card-visual"></div>`;
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}
<div class="card-body">
<div class="card-title-row">
<h3 class="card-title" title="${s.name}">${s.name}</h3>
<h3 class="card-title" title="${nameText}">${nameText}</h3>
<div class="title-meta">
${linkChip}
${showAuthor && s.author ? `<span class="chip">${s.author}</span>` : ""}
${showAuthor && s.author ? `<span class="chip">${authorText}</span>` : ""}
${allowEdit ? `<button class="chip icon" data-edit="${s.id}" type="button" title="${t("card.edit")}">✏️</button>` : ""}
${allowDelete ? `<button class="chip icon danger-chip" data-delete="${s.id}" type="button" title="${t("card.delete")}">✕</button>` : ""}
</div>
</div>
${hasExtraInfo ? `<p class="muted">` : ""}
${genreAndPlayers ? genreAndPlayers : ""}
${s.gameUrl ? `<a class="link compact" href="${s.gameUrl}" target="_blank" rel="noopener">${t("card.site")}</a>` : ""}
${s.youtubeUrl ? `<a class="link compact" href="${s.youtubeUrl}" target="_blank" rel="noopener">${t("card.youtube")}</a>` : ""}
${safeGameUrl ? `<a class="link compact" href="${safeGameUrl}" target="_blank" rel="noopener">${t("card.site")}</a>` : ""}
${safeYoutubeUrl ? `<a class="link compact" href="${safeYoutubeUrl}" target="_blank" rel="noopener">${t("card.youtube")}</a>` : ""}
${hasExtraInfo ? `</p>` : ""}
${s.description ? `<p>${s.description}</p>` : ""}
${s.description ? `<p>${descText}</p>` : ""}
</div>
`;
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 = `
<div class="lightbox-content">
<button class="lightbox-close" aria-label="${t("lightbox.close")}">✕</button>
<img src="${url}" alt="${title}" />
<p>${title || ""}</p>
<img src="${url}" alt="${safeTitle}" />
<p>${safeTitle}</p>
</div>
`;
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 = `
<td title="${v.name}">${truncate(v.name, 28)}</td>
<td class="muted small" title="${v.username}">${truncate(v.username, 24)}</td>
<td title="${escapeHtml(v.name)}">${nameText}</td>
<td class="muted small" title="${escapeHtml(v.username)}">${userText}</td>
<td>${statusText}</td>
<td title="${gamesTooltip}">${v.suggestionCount ?? 0}</td>
<td><button class="chip" data-grant-joker="${v.playerId}" type="button">${v.hasJoker ? "🎟" : t("admin.grantJokerChip")}</button></td>
@@ -930,7 +965,7 @@ function linkTooltip(s) {
function renderLinkBadge(s) {
if (!isLinked(s)) return "";
return `<span class="chip icon link-chip" title="${linkTooltip(s)}">🔗</span>`;
return `<span class="chip icon link-chip" title="${escapeHtml(linkTooltip(s))}">🔗</span>`;
}
function buildLinkOptionLabel(s) {