From a8ec8d8b42291fd8a07442ea4d24d63c3d579057 Mon Sep 17 00:00:00 2001 From: DevCop95 Date: Sat, 17 Oct 2026 13:26:38 -0500 Subject: [PATCH] fix(site): resolve asset URLs before trusting them, and stop mislabelling a failed render MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I shipped this page claiming "every href is checked against this repo's own asset prefix". The check was a startsWith on the raw string, and that is not the same property. This URL starts with the prefix and still resolves outside the download path: https://github.com/DevCop95/pullgoscript/releases/download/v9/../../../evil -> https://github.com/DevCop95/evil Found by running the shipped file, not by reading it: site.js was exercised in node against a stub DOM with the real API payload, the real checksums.txt, and a hostile payload. Before this commit that hostile payload yielded two rows, one of them pointing at a path the comment claimed was impossible. Not exploitable as shipped, and worth saying plainly: browser_download_url comes from api.github.com over TLS, so forging it needs either the ability to write releases in this repo (in which case you would simply point the link wherever you wanted) or a broken TLS chain. Nothing a reader can be tricked into. It is a claim/code mismatch, which is the same defect class as the validHost comment fixed in #7, and the fix is two lines: resolve the URL and check the origin and the path it actually lands on. Anything that escapes after resolution is dropped, which fails closed — no row, and the caller falls back to the pinned list. Two user-visible strings were also wrong or imprecise: - "Could not read the release list automatically" was printed when the list had been read perfectly well and simply had no usable asset. It now says "Could not build the live table automatically", which is what happened. - The render guard said "the latest release has no assets" when the filter, not the release, was the reason. It now names the URL check. Re-verified after the change, same harness: the real payload still renders 7 rows with the amd64 SHA-256 matching the published checksums.txt, the hostile payload drops from 2 survivors to 1 (javascript: URL, foreign host and the traversal are all gone, and a raw HTML asset name is still written as text), and the 403 and empty-payload paths still show the pinned list. No href assigned in any scenario fails the prefix assertion. --- docs/site.js | 35 +++++++++++++++++++++++++++-------- 1 file changed, 27 insertions(+), 8 deletions(-) diff --git a/docs/site.js b/docs/site.js index 9a13ea4..d5cf33c 100644 --- a/docs/site.js +++ b/docs/site.js @@ -2,16 +2,17 @@ // goes stale between releases. // // Every string that comes from the API is written with textContent, never -// innerHTML, and every href is checked against this repo's own asset prefix -// before it is assigned. The console's v1 had a DOM XSS from rendering -// server data through innerHTML; a docs page fed by an API deserves the same -// discipline. +// innerHTML, and every href is resolved and checked against this repo's own +// asset path before it is assigned. The console's v1 had a DOM XSS from +// rendering server data through innerHTML; a docs page fed by an API deserves +// the same discipline. (function () { "use strict"; var REPO = "DevCop95/pullgoscript"; var API = "https://api.github.com/repos/" + REPO + "/releases/latest"; - var PREFIX = "https://github.com/" + REPO + "/releases/download/"; + var ORIGIN = "https://github.com"; + var ASSET_PATH = "/" + REPO + "/releases/download/"; var mount = document.getElementById("dl-live"); var fallback = document.getElementById("dl-fallback"); @@ -25,8 +26,26 @@ return n; } + // A startsWith check on the raw string is not enough. This URL starts with + // the asset prefix and still resolves outside the download path: + // + // https://github.com/DevCop95/pullgoscript/releases/download/v9/../../../evil + // -> https://github.com/DevCop95/evil + // + // So resolve the URL and check the origin and the path it actually ends up + // at. Anything that survives the string check but escapes after resolution + // is dropped, which fails closed: no row, and the caller falls back. function safeURL(u) { - return typeof u === "string" && u.indexOf(PREFIX) === 0 ? u : null; + if (typeof u !== "string" || u.indexOf(ORIGIN + ASSET_PATH) !== 0) return null; + var parsed; + try { + parsed = new URL(u); + } catch (err) { + return null; + } + if (parsed.origin !== ORIGIN) return null; + if (parsed.pathname.indexOf(ASSET_PATH) !== 0) return null; + return parsed.href; } function human(bytes) { @@ -49,7 +68,7 @@ var assets = (release.assets || []).filter(function (a) { return safeURL(a.browser_download_url); }); - if (!assets.length) throw new Error("the latest release has no assets"); + if (!assets.length) throw new Error("no release asset survived the URL check"); var table = el("table"); table.appendChild(el("caption", "Latest release: " + release.tag_name + @@ -101,7 +120,7 @@ .catch(function () { render(release, {}); }); }) .catch(function (err) { - status.textContent = "Could not read the release list automatically (" + + status.textContent = "Could not build the live table automatically (" + err.message + "). The pinned list below is the v2.0.2 snapshot; the " + "Releases page is always current."; if (fallback) fallback.hidden = false;