From 7002497bed056b7a5ea4d01ec572030f253df180 Mon Sep 17 00:00:00 2001 From: dborup Date: Tue, 13 Oct 2026 12:02:05 +0200 Subject: [PATCH] fix(ui): escape and validate miniMarkdown link destinations miniMarkdown() in public/roles.js built its anchors from a '$2' replacement template, so the captured Markdown link destination went into a double-quoted href with neither attribute escaping nor any restriction on the URL scheme. The rest of the text was already & < > escaped, but the two quote characters were not, so a destination could close the attribute and add its own (including an event handler), and nothing stopped javascript: / data: / vbscript:. public/home.js renders Home step descriptions and FAQ answers through miniMarkdown() with innerHTML, and that text comes from site customization (SITE_CONFIG.home), which is importable and therefore untrusted. - escape the destination for attribute context with the same escapeAttr the Home sinks use, now defined once in roles.js and exported; home.js's local copy delegates to it instead of redefining it - reject a destination whose scheme is not http/https, after lowercasing and removing ASCII controls and whitespace, so case- and control-obfuscated schemes are rejected too. Schemeless destinations (relative paths, #fragments, ?query) are untouched - render a rejected link as its already-escaped label text - build the anchor from a replacement function, so a '$&' or "$'" in the destination is no longer re-expanded by String.replace Tests: test-issue-368-minimarkdown-links.js (registered in test-all.sh) and test-xss-368-home-links-e2e.js (registered in deploy.yml), which drives the real Home page against a local server with the hostile destinations injected into /api/config/theme. Relates to #368 --- .github/workflows/deploy.yml | 1 + public/home.js | 6 +- public/roles.js | 44 ++++- scripts/collect-frontend-coverage.js | 3 + test-all.sh | 1 + test-issue-368-minimarkdown-links.js | 257 ++++++++++++++++++++++++++ test-xss-368-home-links-e2e.js | 266 +++++++++++++++++++++++++++ 7 files changed, 576 insertions(+), 2 deletions(-) create mode 100644 test-issue-368-minimarkdown-links.js create mode 100644 test-xss-368-home-links-e2e.js diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml index 4b69907b5..9dbe3800a 100644 --- a/.github/workflows/deploy.yml +++ b/.github/workflows/deploy.yml @@ -431,6 +431,7 @@ jobs: BASE_URL=http://localhost:13581 node test-observer-iata-1188-e2e.js 2>&1 | tee -a e2e-output.txt CHROMIUM_REQUIRE=1 BASE_URL=http://localhost:13581 node test-xss-333-pages-e2e.js 2>&1 | tee -a e2e-output.txt CHROMIUM_REQUIRE=1 BASE_URL=http://localhost:13581 node test-xss-392-perf-e2e.js 2>&1 | tee -a e2e-output.txt + CHROMIUM_REQUIRE=1 BASE_URL=http://localhost:13581 node test-xss-368-home-links-e2e.js 2>&1 | tee -a e2e-output.txt BASE_URL=http://localhost:13581 node test-issue-1639-observers-sort-e2e.js 2>&1 | tee -a e2e-output.txt CHROMIUM_REQUIRE=1 BASE_URL=http://localhost:13581 node test-issue-1758-ng-filter-rerenders-e2e.js 2>&1 | tee -a e2e-output.txt CHROMIUM_REQUIRE=1 BASE_URL=http://localhost:13581 node test-nav-fluid-1055-e2e.js 2>&1 | tee -a e2e-output.txt diff --git a/public/home.js b/public/home.js index 008bd7a69..7e35be568 100644 --- a/public/home.js +++ b/public/home.js @@ -653,7 +653,11 @@ // ==================== HELPERS ==================== function escapeHtml(s) { return String(s).replace(/&/g,'&').replace(//g,'>'); } - function escapeAttr(s) { return String(s).replace(/"/g,'"').replace(/'/g,'''); } + /* #368: one shared implementation, defined and exported by public/roles.js + (which index.html loads before this file) so the Markdown link + destinations in miniMarkdown() and the attribute sinks here cannot drift + apart. Behaviour is unchanged from the local copy this replaces. */ + function escapeAttr(s) { return window.escapeAttr(s); } function timeSinceMs(d) { return Date.now() - d.getTime(); } function checklist(homeCfg) { diff --git a/public/roles.js b/public/roles.js index 1fa21214f..f01a5f37f 100644 --- a/public/roles.js +++ b/public/roles.js @@ -1025,6 +1025,41 @@ } }; + /* #368 — attribute escape for the Markdown link destination below. + Same 5-char-minus-&<> set as the escapeAttr helpers in public/home.js and + public/path-inspector.js: by the time miniMarkdown() captures a + destination it has already turned & < > into entities, so the only + characters that can still leave a double-quoted href are the two quote + characters. Exported so the Home render path has one shared copy. */ + function escapeAttr(s) { + return String(s == null ? '' : s).replace(/"/g, '"').replace(/'/g, '''); + } + window.escapeAttr = escapeAttr; + + /* #368 — scheme allowlist for Markdown link destinations. + Home checklist/step text is site-customization content, which is + importable and therefore untrusted, so a destination may not be able to + pick its own URL scheme. Only these are anchored; everything without a + scheme (relative paths, #fragments, ?query) is left alone. mailto: is not + in the list because nothing in the product emits one today — adding it is + a product decision, not a security one. */ + var SAFE_LINK_SCHEMES = ['http', 'https']; + /* RFC 3986 scheme grammar, lowercased before matching. */ + var LINK_SCHEME_RE = /^([a-z][a-z0-9+.\-]*):/; + + /* Returns the escaped href for `dest`, or '' when the destination must not + become a link. Normalisation mirrors what a browser does before it + resolves a scheme: ASCII controls and ASCII whitespace are ignored, and + the scheme is case-insensitive. So `JaVa\tScRiPt:alert(1)` is rejected for + the same reason `javascript:alert(1)` is. */ + function safeLinkHref(dest) { + var raw = String(dest == null ? '' : dest); + var probe = raw.replace(/[\u0000-\u0020\u007f]/g, '').toLowerCase(); + if (!probe) return ''; + var m = LINK_SCHEME_RE.exec(probe); + if (m && SAFE_LINK_SCHEMES.indexOf(m[1]) === -1) return ''; + return escapeAttr(raw); + } // Simple markdown → HTML (bold, italic, links, code, lists, line breaks) window.miniMarkdown = function(text) { if (!text) return ''; @@ -1033,7 +1068,14 @@ .replace(/\*\*(.+?)\*\*/g, '$1') .replace(/\*(.+?)\*/g, '$1') .replace(/`(.+?)`/g, '$1') - .replace(/\[([^\]]+)\]\(([^)]+)\)/g, '$1') + /* #368: a replacement function, not a '$2' template — a '$&'/"$'" in the + destination would otherwise be re-expanded by String.replace. A + rejected destination degrades to its already-escaped label text. */ + .replace(/\[([^\]]+)\]\(([^)]+)\)/g, function(_m, label, dest) { + var href = safeLinkHref(dest); + if (!href) return label; + return '' + label + ''; + }) .replace(/^- (.+)/gm, '
  • $1
  • ') .replace(/\n/g, '
    '); // Wrap consecutive
  • in