From 2f7d16f3820ca9ec73c94fadd483520a7e8aed6a Mon Sep 17 00:00:00 2001 From: "Dr.Lt.Data" Date: Sat, 19 Sep 2026 02:57:39 +0900 Subject: [PATCH] Escape untrusted Manager UI content --- .gitignore | 4 +- glob/manager_server.py | 53 ++- glob/manager_util.py | 80 ++++ js/comfyui-manager.js | 14 +- js/comfyui-share-common.js | 4 +- js/comfyui-share-copus.js | 4 +- js/comfyui-share-openart.js | 4 +- js/comfyui-share-youml.js | 4 +- js/common.js | 48 +- js/custom-nodes-manager.js | 70 +-- js/manager-grid.js | 67 +++ js/model-manager.js | 59 ++- js/node-usage-analyzer.js | 26 +- js/snapshot.js | 6 +- prestartup_script.py | 26 +- pyproject.toml | 2 +- requirements.txt | 2 + tests/cases/url_sanitize_cases.json | 153 ++++++ tests/js_lift.py | 159 +++++++ tests/manager_test_utils.py | 32 ++ tests/test_grid_highlight_escaping.py | 189 ++++++++ tests/test_manager_markdown_escaping.py | 331 +++++++++++++ tests/test_message_sink_provenance.py | 487 +++++++++++++++++++ tests/test_model_manager_escaping.py | 412 +++++++++++++++++ tests/test_node_usage_escaping.py | 286 ++++++++++++ tests/test_notice_dependencies.py | 83 ++++ tests/test_pack_column_escaping.py | 372 +++++++++++++++ tests/test_residual_sink_escaping.py | 590 ++++++++++++++++++++++++ tests/test_update_result_rendering.py | 58 +++ tests/test_url_sanitize_parity.py | 410 ++++++++++++++++ 30 files changed, 3921 insertions(+), 114 deletions(-) create mode 100644 js/manager-grid.js create mode 100644 tests/cases/url_sanitize_cases.json create mode 100644 tests/js_lift.py create mode 100644 tests/manager_test_utils.py create mode 100644 tests/test_grid_highlight_escaping.py create mode 100644 tests/test_manager_markdown_escaping.py create mode 100644 tests/test_message_sink_provenance.py create mode 100644 tests/test_model_manager_escaping.py create mode 100644 tests/test_node_usage_escaping.py create mode 100644 tests/test_notice_dependencies.py create mode 100644 tests/test_pack_column_escaping.py create mode 100644 tests/test_residual_sink_escaping.py create mode 100644 tests/test_update_result_rendering.py create mode 100644 tests/test_url_sanitize_parity.py diff --git a/.gitignore b/.gitignore index 33ee743b7..20e94e0ff 100644 --- a/.gitignore +++ b/.gitignore @@ -16,5 +16,7 @@ comfyworkflows_sharekey github-stats-cache.json pip_overrides.json *.json +# Track JSON test fixtures. +!tests/cases/*.json check2.sh -/venv/ \ No newline at end of file +/venv/ diff --git a/glob/manager_server.py b/glob/manager_server.py index 4b8826260..abb1e8af0 100644 --- a/glob/manager_server.py +++ b/glob/manager_server.py @@ -304,7 +304,6 @@ print_comfyui_version() core.check_invalid_nodes() - def setup_environment(): git_exe = core.get_config()['git_exe'] @@ -969,14 +968,22 @@ async def update_all(request): def convert_markdown_to_html(input_text): + """Parse source markdown, escaping text and URLs at their HTML boundaries.""" pattern_a = re.compile(r'\[a/([^]]+)]\(([^)]+)\)') pattern_w = re.compile(r'\[w/([^]]+)]') pattern_i = re.compile(r'\[i/([^]]+)]') pattern_bold = re.compile(r'\*\*([^*]+)\*\*') pattern_white = re.compile(r'%%([^*]+)%%') + # Format link labels separately from URLs; escape URLs when inserted. + hrefs = [] + def replace_a(match): - return f"{match.group(1)}" + # Entities written in the source URL (e.g. &) retain their meaning. + url = manager_util.unescape_html_entities(match.group(2)) + hrefs.append(manager_util.sanitize_url(url)) + text = manager_util.escape_html_text(match.group(1)) + return f"{text}" def replace_w(match): return f"

{match.group(1)}

" @@ -990,20 +997,35 @@ def convert_markdown_to_html(input_text): def replace_white(match): return f"{match.group(1)}" - input_text = input_text.replace('\\[', '[').replace('\\]', ']').replace('<', '<').replace('>', '>') + # NUL dropped first so the input cannot forge the href placeholders. + input_text = input_text.replace('\x00', '') + input_text = input_text.replace('\\[', '[').replace('\\]', ']') - result_text = re.sub(pattern_a, replace_a, input_text) + # Parse links before escaping prose, so generated text entities never enter URLs. + parts = [] + start = 0 + for match in pattern_a.finditer(input_text): + parts.append(manager_util.sanitize_tag(input_text[start:match.start()])) + parts.append(replace_a(match)) + start = match.end() + parts.append(manager_util.sanitize_tag(input_text[start:])) + result_text = ''.join(parts) result_text = re.sub(pattern_w, replace_w, result_text) result_text = re.sub(pattern_i, replace_i, result_text) result_text = re.sub(pattern_bold, replace_bold, result_text) result_text = re.sub(pattern_white, replace_white, result_text) + result_text = result_text.replace("\n", "
") - return result_text.replace("\n", "
") + return re.sub( + r'\x00H(\d+)\x00', + lambda m: manager_util.escape_html_attribute(hrefs[int(m.group(1))]), + result_text, + ) def populate_markdown(x): if 'description' in x: - x['description'] = convert_markdown_to_html(manager_util.sanitize_tag(x['description'])) + x['description'] = convert_markdown_to_html(x['description']) if 'name' in x: x['name'] = manager_util.sanitize_tag(x['name']) @@ -1780,7 +1802,6 @@ async def set_db_mode_handler(request): return web.Response(status=200) - @routes.get("/manager/policy/component") async def get_component_policy(request): return web.Response(text=core.get_config()['component_policy'], status=200) @@ -1834,19 +1855,6 @@ async def set_channel_url_list(request): return web.Response(status=200) -def add_target_blank(html_text): - pattern = r'(]*)(>)' - - def add_target(match): - if 'target=' not in match.group(1): - return match.group(1) + ' target="_blank"' + match.group(2) - return match.group(0) - - modified_html = re.sub(pattern, add_target, html_text) - - return modified_html - - @routes.get("/manager/notice") async def get_notice(request): url = "github.com" @@ -1862,7 +1870,8 @@ async def get_notice(request): match = pattern.search(html_content) if match: - markdown_content = match.group(1) + # Sanitize remote HTML before appending Manager's version text. + markdown_content = manager_util.sanitize_html_fragment(match.group(1)) version_tag = os.environ.get('__COMFYUI_DESKTOP_VERSION__') if version_tag is not None: markdown_content += f"
ComfyUI: {version_tag} [Desktop]" @@ -1876,8 +1885,6 @@ async def get_notice(request): # markdown_content += f"
         ()" markdown_content += f"
Manager: {core.version_str}" - markdown_content = add_target_blank(markdown_content) - try: if '__COMFYUI_DESKTOP_VERSION__' not in os.environ: if core.comfy_ui_commit_datetime == datetime(1900, 1, 1, 0, 0, 0): diff --git a/glob/manager_util.py b/glob/manager_util.py index fd110b7fc..c54bcb22d 100644 --- a/glob/manager_util.py +++ b/glob/manager_util.py @@ -16,6 +16,8 @@ import logging import platform import shlex from functools import lru_cache +from html import escape, unescape +from html.entities import html5 cache_lock = threading.Lock() @@ -268,6 +270,84 @@ def sanitize_tag(x): return x.replace('<', '<').replace('>', '>') +SAFE_URL_SCHEMES = frozenset({'http', 'https'}) +_URL_SCHEME_NOISE = re.compile(r'[\x00-\x20\x7f]') +_URL_HEAD_DELIMITERS = ('/', '?', '#') + + +def escape_html_attribute(value): + """Escape a value for a quoted HTML attribute.""" + return escape(str(value), quote=True) + + +_HTML_ENTITY = re.compile(r'&(#[0-9]+|#[xX][0-9a-fA-F]+|[A-Za-z][A-Za-z0-9]*);') +_HTML_TEXT_TOKEN = re.compile(_HTML_ENTITY.pattern + "|[&<>\"']") + + +def unescape_html_entities(text): + """Decode complete entities once, leaving query keys such as ¬ebook intact.""" + def replace(match): + entity = match.group(1) + if entity.startswith('#'): + return unescape(match.group(0)) + return html5.get(entity + ';', match.group(0)) + + return _HTML_ENTITY.sub(replace, text) + + +def escape_html_text(value): + """Escape HTML text without double-escaping existing entities.""" + return _HTML_TEXT_TOKEN.sub( + lambda m: m.group(0) if m.group(1) else escape_html_attribute(m.group(0)), + str(value).replace('\x00', ''), + ) + + +def sanitize_url(url): + """Allow HTTP(S), relative URLs and anchors. + + HTML callers must also escape the result as an attribute.""" + raw = '' if url is None else str(url) + probe = _URL_SCHEME_NOISE.sub('', raw) + if not probe: + return '#' + + cut = len(probe) + for delimiter in _URL_HEAD_DELIMITERS: + found = probe.find(delimiter) + if found != -1: + cut = min(cut, found) + head = probe[:cut] + + if ':' in head: + return raw.strip() if head.split(':', 1)[0].lower() in SAFE_URL_SCHEMES else '#' + + return raw.strip() + + +def sanitize_html_fragment(fragment): + """Sanitize notice HTML, preserving its formatting and opening links safely.""" + # prestartup_script imports this module before installing dependencies. + import nh3 + + return nh3.clean( + '' if fragment is None else str(fragment), + tags=nh3.ALLOWED_TAGS | {'font'}, + attributes={ + **nh3.ALLOWED_ATTRIBUTES, + '*': {'class', 'title', 'align', 'width', 'height'}, + 'font': {'color'}, + }, + # These elements previously hid their contents from the notice. + clean_content_tags={ + 'script', 'style', 'iframe', 'object', 'embed', 'svg', 'math', + 'template', 'noscript', 'textarea', 'title', + }, + url_schemes=SAFE_URL_SCHEMES, + set_tag_attribute_values={'a': {'target': '_blank'}}, + ) + + def extract_package_as_zip(file_path, extract_path): import zipfile try: diff --git a/js/comfyui-manager.js b/js/comfyui-manager.js index 9ddb49403..4af212a77 100644 --- a/js/comfyui-manager.js +++ b/js/comfyui-manager.js @@ -14,7 +14,7 @@ import { OpenArtShareDialog } from "./comfyui-share-openart.js"; import { free_models, install_pip, install_via_git_url, manager_instance, rebootAPI, setManagerInstance, show_message, customAlert, customPrompt, - infoToast, showTerminal, setNeedRestart, handle403Response + infoToast, showTerminal, setNeedRestart, handle403Response, sanitizeHTML, safeHref } from "./common.js"; import { ComponentBuilderDialog, getPureName, load_components, set_component_policy } from "./components-manager.js"; import { CustomNodesManager } from "./custom-nodes-manager.js"; @@ -713,12 +713,12 @@ async function onQueueStatus(event) { for(let x in success_list) { let k = success_list[x]; let url = event.detail.nodepack_result[k].url; - let title = event.detail.nodepack_result[k].title; + let title = sanitizeHTML(String(event.detail.nodepack_result[k].title ?? '')); if(url) { - msg += `
  • ${title}
    • `; + msg += `
  • ${title}
    • `; } else { - msg += `
  • ${k}
    • `; + msg += `
  • ${sanitizeHTML(String(k))}
    • `; } } msg += ""; @@ -732,12 +732,12 @@ async function onQueueStatus(event) { for(let x in failed_list) { let k = failed_list[x]; let url = event.detail.nodepack_result[k].url; - let title = event.detail.nodepack_result[k].title; + let title = sanitizeHTML(String(event.detail.nodepack_result[k].title ?? '')); if(url) { - msg += `
  • ${title}
    • `; + msg += `
  • ${title}
    • `; } else { - msg += `
  • ${k}
    • `; + msg += `
  • ${sanitizeHTML(String(k))}
    • `; } } diff --git a/js/comfyui-share-common.js b/js/comfyui-share-common.js index e6f3e1039..b621e8eea 100644 --- a/js/comfyui-share-common.js +++ b/js/comfyui-share-common.js @@ -4,7 +4,7 @@ import { $el, ComfyDialog } from "../../scripts/ui.js"; import { CopusShareDialog } from "./comfyui-share-copus.js"; import { OpenArtShareDialog } from "./comfyui-share-openart.js"; import { YouMLShareDialog } from "./comfyui-share-youml.js"; -import { customAlert } from "./common.js"; +import { customAlert, safeHref, sanitizeHTML } from "./common.js"; export const SUPPORTED_OUTPUT_NODE_TYPES = [ "PreviewImage", @@ -937,7 +937,7 @@ export class ShareDialog extends ComfyDialog { const response_json = await response.json(); if (response_json.comfyworkflows.url) { - this.final_message.innerHTML = "Your art has been shared: " + response_json.comfyworkflows.url + ""; + this.final_message.innerHTML = "Your art has been shared: " + sanitizeHTML(response_json.comfyworkflows.url) + ""; if (response_json.matrix.success) { this.final_message.innerHTML += "
    Your art has been shared in the ComfyUI Matrix server's #share channel!"; } diff --git a/js/comfyui-share-copus.js b/js/comfyui-share-copus.js index 46288e596..91aab7026 100644 --- a/js/comfyui-share-copus.js +++ b/js/comfyui-share-copus.js @@ -1,6 +1,6 @@ import { app } from "../../scripts/app.js"; import { $el, ComfyDialog } from "../../scripts/ui.js"; -import { customAlert } from "./common.js"; +import { customAlert, safeHref } from "./common.js"; const env = "prod"; @@ -883,7 +883,7 @@ export class CopusShareDialog extends ComfyDialog { const { data } = res.data; if (data) { const url = `${DEFAULT_HOMEPAGE_URL}/work/${data}`; - this.message.innerHTML = `Workflow has been shared successfully. Click here to view it.`; + this.message.innerHTML = `Workflow has been shared successfully. Click here to view it.`; this.previewImage.src = ""; this.previewImage.style.display = "none"; this.uploadedImages = []; diff --git a/js/comfyui-share-openart.js b/js/comfyui-share-openart.js index 1c96a8c73..2b853fe28 100644 --- a/js/comfyui-share-openart.js +++ b/js/comfyui-share-openart.js @@ -1,7 +1,7 @@ import {app} from "../../scripts/app.js"; import {api} from "../../scripts/api.js"; import {ComfyDialog, $el} from "../../scripts/ui.js"; -import { customAlert } from "./common.js"; +import { customAlert, safeHref } from "./common.js"; const LOCAL_STORAGE_KEY = "openart_comfy_workflow_key"; const DEFAULT_HOMEPAGE_URL = "https://openart.ai/workflows/dev?developer=true"; @@ -511,7 +511,7 @@ export class OpenArtShareDialog extends ComfyDialog { const {workflow_id} = response.data; if (workflow_id) { const url = `https://openart.ai/workflows/-/-/${workflow_id}`; - this.message.innerHTML = `Workflow has been shared successfully. Click here to view it.`; + this.message.innerHTML = `Workflow has been shared successfully. Click here to view it.`; this.previewImage.src = ""; this.previewImage.style.display = "none"; this.uploadedImages = []; diff --git a/js/comfyui-share-youml.js b/js/comfyui-share-youml.js index efd8916f6..18639f376 100644 --- a/js/comfyui-share-youml.js +++ b/js/comfyui-share-youml.js @@ -1,7 +1,7 @@ import {app} from "../../scripts/app.js"; import {api} from "../../scripts/api.js"; import {ComfyDialog, $el} from "../../scripts/ui.js"; -import { customAlert } from "./common.js"; +import { customAlert, safeHref } from "./common.js"; const BASE_URL = "https://youml.com"; //const BASE_URL = "http://localhost:3000"; @@ -420,7 +420,7 @@ export class YouMLShareDialog extends ComfyDialog { } } this.message.innerHTML = `${messagePrefix} To turn your workflow into an interactive app, ` + - `visit it on YouML`; + `visit it on YouML`; this.uploadedImages = []; this.nameInput.value = ""; diff --git a/js/common.js b/js/common.js index 5c82259e3..2b1179e68 100644 --- a/js/common.js +++ b/js/common.js @@ -427,6 +427,40 @@ export function sanitizeHTML(str) { .replace(/'/g, "'"); } +// Keep URL handling aligned with glob/manager_util.py; covered by shared cases. +export const SAFE_URL_SCHEMES = ['http', 'https']; +const URL_SCHEME_NOISE = /[\x00-\x20\x7f]/g; +const URL_HEAD_DELIMITERS = ['/', '?', '#']; +// Match Python str.strip(); JavaScript trim() uses different Unicode whitespace. +const URL_TRIM = /^[\t-\r\x1c-\x20\x85\xa0\u1680\u2000-\u200a\u2028\u2029\u202f\u205f\u3000]+|[\t-\r\x1c-\x20\x85\xa0\u1680\u2000-\u200a\u2028\u2029\u202f\u205f\u3000]+$/g; +const trimUrl = (value) => value.replace(URL_TRIM, ''); + +export function sanitizeUrl(url) { + const raw = (url === null || url === undefined) ? '' : String(url); + const probe = raw.replace(URL_SCHEME_NOISE, ''); + if (!probe) { + return '#'; + } + + let cut = probe.length; + for (const delimiter of URL_HEAD_DELIMITERS) { + const found = probe.indexOf(delimiter); + if (found !== -1) { + cut = Math.min(cut, found); + } + } + const head = probe.slice(0, cut); + + if (head.includes(':')) { + return SAFE_URL_SCHEMES.includes(head.split(':', 1)[0].toLowerCase()) ? trimUrl(raw) : '#'; + } + + return trimUrl(raw); +} + +// Validate the URL, then escape it for a quoted href attribute. +export const safeHref = (url) => sanitizeHTML(sanitizeUrl(url)); + export function showTerminal() { try { const panel = app.extensionManager.bottomPanel; @@ -646,7 +680,9 @@ export async function uninstallNodes(nodeList, options = {}) { for (const nodeItem of nodeList) { target_items.push(nodeItem); - onProgress(`Uninstall ${nodeItem.title || nodeItem.name} ...`); + // Titles are escaped by the caller; the name fallback is a raw pack key. + const displayTitle = nodeItem.title || sanitizeHTML(String(nodeItem.name)); + onProgress(`Uninstall ${displayTitle} ...`); const data = nodeItem.originalData || nodeItem; data.channel = channel; @@ -659,7 +695,7 @@ export async function uninstallNodes(nodeList, options = {}) { }); if (res.status != 200) { - errorMsg = `'${sanitizeHTML(String(nodeItem.title || nodeItem.name))}': `; + errorMsg = `'${displayTitle}': `; if (res.status == 403) { errorMsg += `This action is not allowed with this security level configuration.\n`; @@ -988,14 +1024,14 @@ export function createFlyover(container, options = {}) { return flyover; } -// Shared UI State Methods - consolidated from multiple managers +// Message methods accept HTML; callers escape raw data and preserve formatted content. export function createUIStateManager(element, selectors) { return { showSelection: (msg) => { const el = element.querySelector(selectors.selection); if (el) el.innerHTML = msg; }, - + showError: (err) => { const el = element.querySelector(selectors.message); if (el) { @@ -1003,7 +1039,7 @@ export function createUIStateManager(element, selectors) { el.innerHTML = msg; } }, - + showMessage: (msg, color) => { const el = element.querySelector(selectors.message); if (el) { @@ -1013,7 +1049,7 @@ export function createUIStateManager(element, selectors) { el.innerHTML = msg; } }, - + showStatus: (msg, color) => { const el = element.querySelector(selectors.status); if (el) { diff --git a/js/custom-nodes-manager.js b/js/custom-nodes-manager.js index 1963ae609..dff9a048f 100644 --- a/js/custom-nodes-manager.js +++ b/js/custom-nodes-manager.js @@ -6,13 +6,13 @@ import { buildGuiFrameCustomHeader, createSettingsCombo } from "./comfyui-gui-b import { manager_instance, rebootAPI, install_via_git_url, fetchData, md5, icons, show_message, customConfirm, customAlert, customPrompt, - sanitizeHTML, infoToast, showTerminal, setNeedRestart, + sanitizeHTML, sanitizeUrl, infoToast, showTerminal, setNeedRestart, storeColumnWidth, restoreColumnWidth, getTimeAgo, copyText, loadCss, showPopover, hidePopover, getWorkflowNodeTypes, findPackageByCnrId, analyzeWorkflowUsage, createFlyover } from "./common.js"; // https://cenfun.github.io/turbogrid/api.html -import TG from "./turbogrid.esm.js"; +import ManagerGrid from "./manager-grid.js"; loadCss("./custom-nodes-manager.css"); @@ -374,18 +374,17 @@ export class CustomNodesManager { } let list = installGroups[action]; + if (!Array.isArray(list)) { + return ""; + } if(is_selected_button || rowItem?.version === "unknown") { list = list.filter(it => it !== "switch"); } - if (!list) { - return ""; - } - return list.map(id => { const bt = buttons[id]; - return ``; + return ``; }).join(""); } @@ -528,7 +527,7 @@ export class CustomNodesManager { initGrid() { const container = this.element.querySelector(".cn-manager-grid"); - const grid = new TG.Grid(container); + const grid = new ManagerGrid(container); this.grid = grid; this.flyover = createFlyover(container, { @@ -593,6 +592,9 @@ export class CustomNodesManager { grid.setOption({ + highlightKeywords: { + textGenerator: (row, column) => column === 'author' ? sanitizeHTML(String(row[column] ?? '')) : row[column] + }, theme: 'dark', selectVisible: true, selectMultiple: true, @@ -698,7 +700,8 @@ export class CustomNodesManager { id: 'id', name: 'ID', width: 50, - align: 'center' + align: 'center', + formatter: (value) => (value === null || value === undefined) ? value : sanitizeHTML(String(value)) }, { id: 'title', name: 'Title', @@ -724,11 +727,13 @@ export class CustomNodesManager { } const link = document.createElement('a'); + // DOM href takes a validated raw URL, without HTML attribute escaping. if(rowItem.originalData.repository) - link.href = rowItem.originalData.repository; + link.href = sanitizeUrl(rowItem.originalData.repository); else - link.href = rowItem.reference; + link.href = sanitizeUrl(rowItem.reference); link.target = '_blank'; + link.rel = 'noopener noreferrer'; link.innerHTML = `${title}`; link.title = rowItem.originalData.id; container.appendChild(link); @@ -746,13 +751,15 @@ export class CustomNodesManager { if(!version) { return; } + const safeVersion = sanitizeHTML(String(version)); if(rowItem.cnr_latest && version != rowItem.cnr_latest) { + const safeLatest = sanitizeHTML(String(rowItem.cnr_latest)); if(version == 'nightly') { - return `
    ${version}
    [${rowItem.cnr_latest}]
    `; + return `
    ${safeVersion}
    [${safeLatest}]
    `; } - return `
    ${version}
    [↑${rowItem.cnr_latest}]
    `; + return `
    ${safeVersion}
    [↑${safeLatest}]
    `; } - return version; + return safeVersion; } }, { id: 'action', @@ -804,10 +811,11 @@ export class CustomNodesManager { width: 120, classMap: "cn-pack-author", formatter: (author, rowItem, columnItem) => { + const safeAuthor = sanitizeHTML(String(author ?? '')); if (rowItem.trust) { - return `✅ ${author}`; + return `✅ ${safeAuthor}`; } - return author; + return safeAuthor; } }, { id: 'stars', @@ -821,7 +829,7 @@ export class CustomNodesManager { if (typeof stars === 'number') { return stars.toLocaleString(); } - return stars; + return (stars === null || stars === undefined) ? stars : sanitizeHTML(String(stars)); } }, { id: 'last_update', @@ -835,7 +843,7 @@ export class CustomNodesManager { return 'N/A'; } const ago = getTimeAgo(last_update); - const short = `${last_update}`.split(' ')[0]; + const short = sanitizeHTML(String(last_update).split(' ')[0]); return `${short}`; } }]; @@ -1145,6 +1153,7 @@ export class CustomNodesManager { const rowItem = d.rowItem; const isNotInstalled = rowItem.action == "not-installed"; + // Pack titles are already escaped by the server. let titleHtml = `
    ${rowItem.title}
    `; if (isNotInstalled) { titleHtml += '
    Not Installed
    ' @@ -1161,7 +1170,8 @@ export class CustomNodesManager { list.push(`
    `); list.push(`
    ${i+1}
    `); - list.push(`
    ${it.name}
    `); + // extName via /customnode/getmappings, no server sanitize: escaped at the sink. + list.push(`
    ${sanitizeHTML(String(it.name))}
    `); if (it.conflicts) { list.push(`
    ${icons.conflicts}
    Conflict with${it.conflicts.map(c => { @@ -1283,7 +1293,7 @@ export class CustomNodesManager { return; } - const selectedMap = {}; + const selectedMap = Object.create(null); selectedList.forEach(item => { let type = item.action; if (item.restart) { @@ -1302,7 +1312,7 @@ export class CustomNodesManager { Object.keys(selectedMap).forEach(v => { const filterItem = this.getFilterItem(v); list.push(`
    - Selected ${selectedMap[v].length} ${filterItem ? filterItem.label : v} + Selected ${selectedMap[v].length} ${sanitizeHTML(String(filterItem ? filterItem.label : v))} ${this.grid.hasMask ? "" : this.getActionButtons(v, null, true)}
    `); }); @@ -1467,7 +1477,8 @@ export class CustomNodesManager { }); if (res.status != 200) { - errorMsg = `'${sanitizeHTML(String(item.title))}': `; + // The title is already escaped by populate_markdown. + errorMsg = `'${item.title}': `; if(res.status == 403) { try { @@ -1561,7 +1572,8 @@ export class CustomNodesManager { for (let hash in result) { let v = result[hash]; if (v != 'success' && v != 'skip') { - errorMsg += v + '\n'; + // Escape raw task errors for both HTML message destinations. + errorMsg += sanitizeHTML(String(v)) + '\n'; } } @@ -1753,7 +1765,7 @@ export class CustomNodesManager { this.showStatus(`Loading missing nodes (${mode}) ...`); const res = await fetchData(`/customnode/getmappings?mode=${mode}`); if (res.error) { - this.showError(`Failed to get custom node mappings: ${res.error}`); + this.showError(`Failed to get custom node mappings: ${sanitizeHTML(String(res.error))}`); return; } @@ -1871,7 +1883,7 @@ export class CustomNodesManager { this.showStatus(`Loading alternatives (${mode}) ...`); const res = await fetchData(`/customnode/alternatives?mode=${mode}`); if (res.error) { - this.showError(`Failed to get alternatives: ${res.error}`); + this.showError(`Failed to get alternatives: ${sanitizeHTML(String(res.error))}`); return []; } @@ -1887,8 +1899,9 @@ export class CustomNodesManager { continue; } + // tags (alter-list.json) is not server-transformed, so escape before innerHTML. const tags = `${item.tags}`.split(",").map(tag => { - return `
    ${tag.trim()}
    `; + return `
    ${sanitizeHTML(tag.trim())}
    `; }).join(""); hashMap[custom_node.hash] = { @@ -1906,7 +1919,7 @@ export class CustomNodesManager { const result = await analyzeWorkflowUsage(this.custom_nodes); if (!result.success) { - this.showError(`Failed to get workflow data: ${result.error}`); + this.showError(`Failed to get workflow data: ${sanitizeHTML(String(result.error))}`); return {}; } @@ -1977,7 +1990,7 @@ export class CustomNodesManager { this.custom_nodes = node_packs; if(this.channel !== 'default') { - this.element.querySelector(".cn-manager-channel").innerHTML = `Channel: ${this.channel} (Incomplete list)`; + this.element.querySelector(".cn-manager-channel").innerHTML = `Channel: ${sanitizeHTML(String(this.channel))} (Incomplete list)`; } for (const k in node_packs) { @@ -2109,6 +2122,7 @@ export class CustomNodesManager { // =========================================================================================== + // Messages accept HTML; see createUIStateManager in common.js. showSelection(msg) { this.element.querySelector(".cn-manager-selection").innerHTML = msg; } diff --git a/js/manager-grid.js b/js/manager-grid.js new file mode 100644 index 000000000..4ea8ff2bd --- /dev/null +++ b/js/manager-grid.js @@ -0,0 +1,67 @@ +import { Grid } from "./turbogrid.esm.js"; + +// Keep Manager's search hardening outside the vendored TurboGrid bundle. +export default class ManagerGrid extends Grid { + highlightKeywordsFilter(rowItem, columns, value) { + const { textKey, textGenerator, highlightKey } = this.options.highlightKeywords; + for (const column of columns) { + rowItem[`${highlightKey}${column}`] = null; + } + const keywords = value ? String(value).trim().toLowerCase().split(/\s+/).filter(Boolean) : []; + if (!keywords.length) return true; + + let matched = false; + for (const column of columns) { + const value = typeof textGenerator === "function" ? textGenerator(rowItem, column) : rowItem[column]; + if (value === null || value === undefined) continue; + let text = String(value).trim(); + if (!text) continue; + const key = `${textKey}${column}`; + if (rowItem[key] === null || rowItem[key] === undefined) { + // A detached element's innerHTML can still run image handlers. + rowItem[key] = new DOMParser().parseFromString(text, "text/html").body.textContent; + } + text = rowItem[key].toLowerCase(); + let offset = 0; + const found = keywords.every(keyword => { + const index = text.indexOf(keyword, offset); + if (index === -1) return false; + offset = index + keyword.length; + return true; + }); + if (found) { + rowItem[`${highlightKey}${column}`] = true; + this.highlightKeywords = keywords; + matched = true; + } + } + return matched; + } + + highlightTextNodes(nodes, keywords) { + if (!keywords.length) return; + let keywordIndex = 0; + for (const node of nodes) { + const text = node.textContent; + const lower = text.toLowerCase(); + const wrapper = document.createElement("span"); + let offset = 0; + while (offset < text.length) { + const keyword = keywords[keywordIndex]; + const index = lower.indexOf(keyword, offset); + if (index === -1) break; + wrapper.append(text.slice(offset, index)); + const mark = document.createElement("mark"); + mark.textContent = text.slice(index, index + keyword.length); + wrapper.append(mark); + offset = index + keyword.length; + keywordIndex = (keywordIndex + 1) % keywords.length; + } + if (wrapper.childNodes.length) { + // Decoded cell text stays in text nodes, including inside . + wrapper.append(text.slice(offset)); + node.replaceWith(wrapper); + } + } + } +} diff --git a/js/model-manager.js b/js/model-manager.js index 5ba0e9e9f..b8c3f2d85 100644 --- a/js/model-manager.js +++ b/js/model-manager.js @@ -3,18 +3,28 @@ import { $el } from "../../scripts/ui.js"; import { manager_instance, rebootAPI, fetchData, md5, icons, show_message, customAlert, infoToast, showTerminal, - storeColumnWidth, restoreColumnWidth, loadCss, formatSize, sizeToBytes + storeColumnWidth, restoreColumnWidth, loadCss, formatSize, sizeToBytes, + sanitizeHTML, safeHref } from "./common.js"; import { api } from "../../scripts/api.js"; // https://cenfun.github.io/turbogrid/api.html -import TG from "./turbogrid.esm.js"; +import ManagerGrid from "./manager-grid.js"; import { buildGuiFrameCustomHeader, createSettingsCombo } from "./comfyui-gui-builder.js"; loadCss("./model-manager.css"); const gridId = "model"; +// Escape raw cells while preserving empty values. +const escapeCell = (value) => (value === null || value === undefined) ? value : sanitizeHTML(String(value)); + +// Escape option markup while comparing the original values. +const renderOptions = (list, current) => list.map(item => { + const selected = item.value === current ? " selected" : ""; + return ``; +}).join(""); + const pageHtml = `
    @@ -101,22 +111,13 @@ export class ModelManager { updateFilter() { const $filter = this.element.querySelector(".cmm-manager-filter"); - $filter.innerHTML = this.filterList.map(item => { - const selected = item.value === this.filter ? " selected" : ""; - return `` - }).join(""); + $filter.innerHTML = renderOptions(this.filterList, this.filter); const $type = this.element.querySelector(".cmm-manager-type"); - $type.innerHTML = this.typeList.map(item => { - const selected = item.value === this.type ? " selected" : ""; - return `` - }).join(""); + $type.innerHTML = renderOptions(this.typeList, this.type); const $base = this.element.querySelector(".cmm-manager-base"); - $base.innerHTML = this.baseList.map(item => { - const selected = item.value === this.base ? " selected" : ""; - return `` - }).join(""); + $base.innerHTML = renderOptions(this.baseList, this.base); } @@ -199,7 +200,7 @@ export class ModelManager { initGrid() { const container = this.element.querySelector(".cmm-manager-grid"); - const grid = new TG.Grid(container); + const grid = new ManagerGrid(container); this.grid = grid; grid.bind('onUpdated', (e, d) => { @@ -227,6 +228,9 @@ export class ModelManager { }); grid.setOption({ + highlightKeywords: { + textGenerator: (row, column) => ['name', 'description'].includes(column) ? row[column] : escapeCell(row[column]) + }, theme: 'dark', selectVisible: true, @@ -325,7 +329,8 @@ export class ModelManager { maxWidth: 500, classMap: 'cmm-node-name', formatter: function(name, rowItem, columnItem, cellNode) { - return `${name}`; + // Names are server-escaped; raw references need URL and attribute handling. + return `${name}`; } }, { id: 'installed', @@ -351,7 +356,7 @@ export class ModelManager { sortable: false, align: 'center', formatter: (url, rowItem, columnItem) => { - return `${icons.download}`; + return `${icons.download}`; } }, { id: 'size', @@ -366,11 +371,14 @@ export class ModelManager { }, { id: 'type', name: 'Type', - width: 100 + width: 100, + formatter: escapeCell }, { id: 'base', - name: 'Base' + name: 'Base', + formatter: escapeCell }, { + // Descriptions are formatted HTML from the server. id: 'description', name: 'Description', width: 400, @@ -379,11 +387,13 @@ export class ModelManager { }, { id: "save_path", name: 'Save Path', - width: 200 + width: 200, + formatter: escapeCell }, { id: 'filename', name: 'Filename', - width: 200 + width: 200, + formatter: escapeCell }]; restoreColumnWidth(gridId, columns); @@ -458,6 +468,7 @@ export class ModelManager { }); } + // Model names are already escaped by the server. this.showStatus(`Install ${item.name} ...`); const data = item.originalData; @@ -483,7 +494,7 @@ export class ModelManager { errorMsg += `This action is not allowed with this security level configuration.\n`; } } else { - errorMsg += await res.text() + '\n'; + errorMsg += sanitizeHTML(await res.text()) + '\n'; } break; @@ -552,8 +563,9 @@ export class ModelManager { for(let hash in result){ let v = result[hash]; + // Escape raw task errors for both HTML message destinations. if(v != 'success') - errorMsg += v + '\n'; + errorMsg += sanitizeHTML(String(v)) + '\n'; } for(let k in self.install_context.targets) { @@ -670,6 +682,7 @@ export class ModelManager { // =========================================================================================== + // Messages accept HTML; see createUIStateManager in common.js. showSelection(msg) { this.element.querySelector(".cmm-manager-selection").innerHTML = msg; } diff --git a/js/node-usage-analyzer.js b/js/node-usage-analyzer.js index aaf6e7414..3cd3f85a7 100644 --- a/js/node-usage-analyzer.js +++ b/js/node-usage-analyzer.js @@ -4,7 +4,8 @@ import { manager_instance, fetchData, md5, show_message, customAlert, infoToast, showTerminal, storeColumnWidth, restoreColumnWidth, loadCss, uninstallNodes, - analyzeWorkflowUsage, sizeToBytes, createFlyover, createUIStateManager + analyzeWorkflowUsage, sizeToBytes, createFlyover, createUIStateManager, + sanitizeHTML, safeHref } from "./common.js"; import { api } from "../../scripts/api.js"; @@ -220,7 +221,8 @@ export class NodeUsageAnalyzer { maxWidth: 500, classMap: 'nu-pack-name', formatter: function (name, rowItem, columnItem, cellNode) { - return `${name}`; + // Names are escaped during loadData; references need URL and attribute handling. + return `${name}`; } }, { id: 'used_in_count', @@ -283,7 +285,8 @@ export class NodeUsageAnalyzer { workflowList.forEach((workflow, i) => { list.push(`
    `); list.push(`
    ${i + 1}
    `); - list.push(`
    ${workflow.filename}
    `); + // Workflow filenames are raw server data. + list.push(`
    ${sanitizeHTML(String(workflow.filename))}
    `); list.push(`
    ${workflow.nodeCount} node${workflow.nodeCount > 1 ? 's' : ''}
    `); list.push(`
    `); }); @@ -346,7 +349,8 @@ export class NodeUsageAnalyzer { target_items.push(item); - this.ui.showStatus(`Install ${item.name} ...`); + // Here item.name is a raw pack key, not a server-escaped title. + this.ui.showStatus(`Install ${sanitizeHTML(String(item.name))} ...`); const data = item.originalData; data.ui_id = item.hash; @@ -357,12 +361,13 @@ export class NodeUsageAnalyzer { }); if (res.status != 200) { - errorMsg = `'${item.name}': `; + // Escape raw task errors for both HTML message destinations. + errorMsg = `'${sanitizeHTML(String(item.name))}': `; if (res.status == 403) { errorMsg += `This action is not allowed with this security level configuration.\n`; } else { - errorMsg += await res.text() + '\n'; + errorMsg += sanitizeHTML(await res.text()) + '\n'; } break; @@ -466,8 +471,9 @@ export class NodeUsageAnalyzer { for (let hash in result) { let v = result[hash]; + // Escape raw task errors for both HTML message destinations. if (v != 'success' && v != 'skip') - errorMsg += v + '\n'; + errorMsg += sanitizeHTML(String(v)) + '\n'; } for (let k in self.install_context.targets) { @@ -591,7 +597,7 @@ export class NodeUsageAnalyzer { if (result.error.toString().includes('204')) { this.showMessage("No workflows were found for analysis."); } else { - this.showError(result.error); + this.showError(sanitizeHTML(String(result.error))); this.hideLoading(); return; } @@ -612,7 +618,8 @@ export class NodeUsageAnalyzer { const workflowDetails = result.workflowDetailsMap?.get(packKey) || []; models.push({ - title: pack.title || packKey, + // Preserve server-escaped titles; escape only the raw pack-key fallback. + title: pack.title || sanitizeHTML(String(packKey)), reference: pack.reference || pack.files?.[0] || '#', used_in_count: usedCount, workflowDetails: workflowDetails, @@ -639,6 +646,7 @@ export class NodeUsageAnalyzer { // =========================================================================================== + // Messages accept HTML; see createUIStateManager in common.js. showSelection(msg) { this.element.querySelector(".nu-manager-selection").innerHTML = msg; } diff --git a/js/snapshot.js b/js/snapshot.js index ea6bad90a..89a893bd6 100644 --- a/js/snapshot.js +++ b/js/snapshot.js @@ -1,7 +1,7 @@ import { app } from "../../scripts/app.js"; import { api } from "../../scripts/api.js" import { ComfyDialog, $el } from "../../scripts/ui.js"; -import { manager_instance, rebootAPI, show_message, handle403Response, loadCss } from "./common.js"; +import { manager_instance, rebootAPI, show_message, handle403Response, loadCss, sanitizeHTML } from "./common.js"; import { buildGuiFrame } from "./comfyui-gui-builder.js"; loadCss("./snapshot.css"); @@ -133,7 +133,7 @@ export class SnapshotManager extends ComfyDialog { startRestore(target) { const self = SnapshotManager.instance; - self.updateMessage(`
    Restore snapshot '${target.name}'`); + self.updateMessage(`
    Restore snapshot '${sanitizeHTML(target.name)}'`); for(let i in self.restore_buttons) { self.restore_buttons[i].disabled = true; @@ -214,7 +214,7 @@ export class SnapshotManager extends ComfyDialog { data1.style.textAlign = "center"; data1.innerHTML = i+1; var data2 = document.createElement('td'); - data2.innerHTML = ` ${data}`; + data2.innerHTML = ` ${sanitizeHTML(data)}`; var data_button = document.createElement('td'); data_button.style.textAlign = "center"; data_button.className = "data-btns"; diff --git a/prestartup_script.py b/prestartup_script.py index dabd39dae..9250586c9 100644 --- a/prestartup_script.py +++ b/prestartup_script.py @@ -455,16 +455,32 @@ except Exception as e: def ensure_dependencies(): + requirements_path = os.path.join(os.path.dirname(__file__), "requirements.txt") try: import git # noqa: F401 import toml # noqa: F401 import rich # noqa: F401 import chardet # noqa: F401 - except ModuleNotFoundError: - my_path = os.path.dirname(__file__) - requirements_path = os.path.join(my_path, "requirements.txt") + from importlib.metadata import version + from packaging.requirements import Requirement - print("## ComfyUI-Manager: installing dependencies. (GitPython)") + # Check the declared version before loading the native extension, so an + # older nh3 can be upgraded without loading its binary into this process. + with open(requirements_path) as requirements: + for line in requirements: + line = re.split(r'\s+#', line.strip(), maxsplit=1)[0] + if not line or line.startswith(('#', '-')): + continue + nh3_requirement = Requirement(line) + if nh3_requirement.name.lower() == 'nh3': + break + else: + raise ValueError('requirements.txt does not declare nh3') + if version('nh3') not in nh3_requirement.specifier: + raise ModuleNotFoundError(str(nh3_requirement)) + import nh3 # noqa: F401 + except ModuleNotFoundError: + print("## ComfyUI-Manager: installing dependencies.") try: subprocess.check_output(manager_util.make_pip_cmd(['install', '-r', requirements_path])) except subprocess.CalledProcessError: @@ -472,7 +488,7 @@ def ensure_dependencies(): try: subprocess.check_output(manager_util.make_pip_cmd(['install', '--user', '-r', requirements_path])) except subprocess.CalledProcessError: - print("## [ERROR] ComfyUI-Manager: Failed to install the GitPython package in the correct Python environment. Please install it manually in the appropriate environment. (You can seek help at https://app.element.io/#/room/%23comfyui_space%3Amatrix.org)") + print("## [ERROR] ComfyUI-Manager: Failed to install required packages in the correct Python environment. Please install requirements.txt manually in the appropriate environment. (You can seek help at https://app.element.io/#/room/%23comfyui_space%3Amatrix.org)") try: print("## ComfyUI-Manager: installing dependencies done.") diff --git a/pyproject.toml b/pyproject.toml index 8bc2ff147..0b86cae20 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ description = "ComfyUI-Manager provides features to install and manage custom no version = "3.41" license = { file = "LICENSE.txt" } requires-python = ">=3.9" -dependencies = ["GitPython", "PyGithub", "matrix-nio", "transformers", "huggingface-hub>0.20", "typer", "rich", "typing-extensions", "toml", "uv", "chardet"] +dependencies = ["GitPython", "PyGithub", "matrix-nio", "transformers", "huggingface-hub>0.20", "typer", "rich", "typing-extensions", "toml", "uv", "chardet", "packaging", "nh3>=0.3.7"] [project.urls] Repository = "https://github.com/ltdrdata/ComfyUI-Manager" diff --git a/requirements.txt b/requirements.txt index b179f07f1..3371acf7b 100644 --- a/requirements.txt +++ b/requirements.txt @@ -9,3 +9,5 @@ typing-extensions toml uv chardet +packaging +nh3>=0.3.7 diff --git a/tests/cases/url_sanitize_cases.json b/tests/cases/url_sanitize_cases.json new file mode 100644 index 000000000..5138f7d69 --- /dev/null +++ b/tests/cases/url_sanitize_cases.json @@ -0,0 +1,153 @@ +{ + "note": "JS<->Python sanitize_url differential case matrix. STRING inputs only: Python str() and JS String() disagree for non-strings, so feeding them would report divergence for something that is not a defect. See the docstring of tests/test_url_sanitize_parity.py.", + "provenance": "Handed over by gm3-review: 35 cases built to clear the WI-112 model-manager port, plus 22 built for the WI-120 review (scheme grammar, protocol-relative with userinfo, colons after a delimiter, unicode spaces and RTL override, degenerate schemes, a 500-char path). All 57 agreed on both implementations when handed over -- they are coverage, not known failures.", + "type_coercion_warning": { + "why": "Python str() and JS String() are NOT equivalent for non-strings. Feeding non-strings into the differential matrix produces FALSE divergence - a guard RED for a non-bug.", + "measured": { + "1.0": { + "py": "1.0", + "js": "1" + }, + "True": { + "py": "True", + "js": "true" + }, + "False": { + "py": "False", + "js": "false" + }, + "[1,2]": { + "py": "[1, 2]", + "js": "1,2" + }, + "{}": { + "py": "{}", + "js": "[object Object]" + } + }, + "aligned": { + "0": "both '0'", + "None/null": "both '#' - explicit null handling in both impls" + }, + "recommendation": "Scope the matrix to STRING inputs and say so in the guard docstring; url is always a string off the JSON payload in practice." + }, + "shared_matrix": [ + "javascript:a", + "JaVaScRiPt:a", + "java\tscript:a", + "java\nscript:a", + "\u0001javascript:a", + " javascript:a", + "data:text/html,x", + "vbscript:a", + "file:///etc/passwd", + "about:blank", + "blob:http://x", + "javascript:a", + "javascript:a", + "javascript:a", + "javascript:a", + "http://e.com", + "https://e.com/a?b=1&c=2", + "HTTPS://E.COM", + "/rel/path", + "rel", + "#anchor", + "//proto/x", + "a&b/c", + "x&y?z=1", + "e.com:8080/p", + "foo:bar", + ":alert(1)", + "", + " ", + "https://e.com/x\" onmouseover=\"y", + "https://evil.example/a onmouseover=alert(1) x", + "\\\\evil.com/x", + "/\\evil.com", + "\u0000javascript:a", + "j\u0000avascript:a", + "coap+tcp://h/x", + "x-scheme://h", + "a.b://h", + "a-b://h", + "HTTP+S://h", + "//user:pass@evil.example/x", + "#a:b", + "?a=b:c", + "/p:q", + "http://h/#f:g", + "http　://h", + "javascript :a", + "ja​vascript:a", + "ｈｔｔｐ://h", + "http://h/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + "http://h/‮evil", + "http:", + "https:", + "http:/h", + ":", + "::", + "a::b", + " /rel/path ", + " rel ", + "\t#anchor\n", + " http://h/x ", + "\thttps://h/x\t", + "http://h/x", + "/rel/path", + "https://h/x", + "", + " /rel ", + "http://h/x…", + "…http://h/x", + "http://h/x\u001c", + "http://h/x\u001d", + "http://h/x\u001e", + "http://h/x\u001f", + "/rel/path…", + "/rel/path\u001c", + "https://h/x…", + "http://h/x ", + " http://h/x", + "http://h/x ", + "http://h/x ", + "http://h/x ", + "http://h/x
", + "http://h/x
", + "http://h/x ", + "http://h/x ", + "http://h/x　", + "　/rel", + "  /rel  " + ], + "gap_cases_found_by_enforcement": { + "why": "Found by tests/test_url_sanitize_parity.py's own perturbation test, not by review: perturbing the JS scheme-less branch from `return raw.trim()` to `return raw` produced ZERO divergences against the handed-over 57, because every scheme-less case in that set is already trim-clean. The two trim call sites in sanitize_url were therefore unpinned on both sides.", + "cases": [ + " /rel/path ", + " rel ", + "\t#anchor\n", + " http://h/x ", + "\thttps://h/x\t" + ] + }, + "unicode_whitespace_divergence": { + "why": "The 62-case matrix had no BOM or unicode-whitespace input, so the two trim call sites in sanitize_url were compared only over inputs whose edges are ASCII. They are NOT equivalent: String.prototype.trim() and str.strip() disagree on six codepoints.", + "measured": { + "js_strips_python_keeps": [ + "U+FEFF" + ], + "python_strips_js_keeps": [ + "U+001C", + "U+001D", + "U+001E", + "U+001F", + "U+0085" + ], + "method": "enumerated every codepoint below U+110000 on both sides: chr(c).isspace() in Python vs /\\s/-equivalent String.trim() in node; 24 shared, 6 divergent" + }, + "example": "'http://h/x' -> python 'http://h/x', js 'http://h/x'", + "resolution": "ALIGNED, not accepted. glob/manager_util.py is the declared source of truth for this pair (see test_the_guard_goes_red_when_either_allow_list_gains_a_scheme, which names it), so js/common.js stopped delegating to trim() and spells Python's str.strip() set out as URL_TRIM. The divergence is therefore a REGRESSION SHAPE this matrix now catches, not a gap_case.", + "scope": "The trim only shapes the RETURNED string for an already-accepted URL. Scheme acceptance is decided on `probe`, which both sides build identically, so no case here changes what is allowed." + } +} diff --git a/tests/js_lift.py b/tests/js_lift.py new file mode 100644 index 000000000..ff84965ad --- /dev/null +++ b/tests/js_lift.py @@ -0,0 +1,159 @@ +"""Lift real production JS out of the shipped files and execute it in node. + +WHY LIFT RATHER THAN IMPORT: the manager's front-end modules pull in +``../../scripts/app.js`` and touch ``document`` at module scope, so node cannot +import them. WHY LIFT RATHER THAN RETYPE: a retyped copy of a formatter stops +tracking the formatter the moment someone edits it, and then the guard passes +while the shipped code regresses. Lifting keeps the executed bytes the shipped +bytes — if the production function changes, so does the text under test. + +This mirrors tests/test_manager_markdown_escaping.py, which AST-extracts +``convert_markdown_to_html`` rather than importing manager_server. + +Consumers point ``JsSource`` at a directory of .js files; an override directory +(e.g. one populated with ``git show :js/foo.js``) makes the RED half of a +fix reproducible in one command. +""" +import json +import shutil +import subprocess +from html.parser import HTMLParser +from pathlib import Path + +NODE = shutil.which("node") + + +def _skip_literal(source: str, pos: int) -> int: + """If a literal or comment starts at pos, return the index just past it. + + Returns pos unchanged when nothing starts there. Template literals are + consumed whole, which is safe for brace counting because a `${...}` + substitution is itself balanced. A lone `/` is read as a regex literal — + sanitizeHTML's own `.replace(/'/g, ...)` hides an apostrophe inside one, and + no scanned range uses `/` as division. Were that to change, the assertion + below fails loudly rather than returning a wrong slice. + """ + ch = source[pos] + nxt = source[pos + 1] if pos + 1 < len(source) else "" + if ch == "/" and nxt == "/": + return source.index("\n", pos) + if ch == "/" and nxt == "*": + return source.index("*/", pos) + 2 + if ch not in "\"'`/": + return pos + cursor = pos + 1 + while cursor < len(source): + if source[cursor] == "\\": + cursor += 2 + continue + if source[cursor] == ch: + return cursor + 1 + cursor += 1 + raise AssertionError("unterminated literal at offset %d" % (pos,)) + + +def slice_braced(source: str, start_marker: str) -> str: + """Return start_marker plus the balanced {...} block that follows it.""" + start = source.index(start_marker) + pos = source.index("{", start + len(start_marker)) + depth = 0 + while pos < len(source): + skipped = _skip_literal(source, pos) + if skipped != pos: + pos = skipped + continue + if source[pos] == "{": + depth += 1 + elif source[pos] == "}": + depth -= 1 + if depth == 0: + return source[start:pos + 1] + pos += 1 + raise AssertionError("unbalanced braces after %r" % (start_marker,)) + + +def slice_object_entry(source: str, marker: str, indent: str = "\t\t") -> str: + """Return one object literal out of a `}, {`-chained array, by id marker. + + These objects are literals inside an array, so the first `{` after the + marker belongs to the NEXT entry — brace matching cannot be used. The entry + instead ends at the first `}` sitting at the array's own indent. + """ + start = source.index(marker) + end = source.index("\n%s}" % indent, start) + return source[start:end] + + +def line_containing(source: str, marker: str) -> str: + """Return the single source line that holds `marker` (fails if absent).""" + start = source.index(marker) + line_start = source.rfind("\n", 0, start) + 1 + return source[line_start:source.index("\n", start)] + + +class JsSource: + """The shipped .js files under test, read from `js_dir`.""" + + def __init__(self, js_dir): + self.js_dir = Path(js_dir) + self._cache = {} + + def text(self, filename: str) -> str: + if filename not in self._cache: + self._cache[filename] = (self.js_dir / filename).read_text(encoding="utf-8") + return self._cache[filename] + + def lift_declaration(self, filename: str, marker: str) -> str: + """Lift an `export function`/`export const` declaration, minus `export`.""" + return slice_braced(self.text(filename), marker).replace("export ", "", 1) + + def lift_formatter(self, filename: str, anchor: str) -> str: + """Lift the `formatter: ...` at `anchor` as a bare function expression.""" + block = slice_braced(self.text(filename), anchor) + marker = "formatter:" + return block[block.index(marker) + len(marker):].strip() + + def lift_span(self, filename: str, start_marker: str, end_marker: str) -> str: + """Lift the source between two markers, end_marker included.""" + source = self.text(filename) + start = source.index(start_marker) + end = source.index(end_marker, start) + len(end_marker) + return source[start:end] + + +def run_node(script: str) -> dict: + """Execute an ES-module script and parse the single JSON object it prints.""" + assert NODE is not None, "node is required to execute the lifted production JS" + result = subprocess.run( + [NODE, "--input-type=module", "-e", script], + capture_output=True, text=True, timeout=60, + ) + assert result.returncode == 0, "node failed:\n%s" % (result.stderr,) + return json.loads(result.stdout) + + +class Collector(HTMLParser): + """Collects the tags and attributes a browser would actually build.""" + + def __init__(self): + super().__init__() + self.tags = [] + self.attrs = [] + + def handle_starttag(self, tag, attrs): + self.tags.append(tag) + self.attrs.extend(attrs) + + handle_startendtag = handle_starttag + + +def parse(markup: str) -> Collector: + collector = Collector() + collector.feed(markup) + collector.close() + return collector + + +def event_handlers(markup: str): + """Every on* attribute a browser would attach for this markup.""" + return [name for name, _ in parse(markup).attrs if name.lower().startswith("on")] diff --git a/tests/manager_test_utils.py b/tests/manager_test_utils.py new file mode 100644 index 000000000..94d01084e --- /dev/null +++ b/tests/manager_test_utils.py @@ -0,0 +1,32 @@ +"""Load Python escaping code without importing the ComfyUI server. + +MANAGER_GLOB_DIR can point to another revision for regression checks. Loading +manager_util by path avoids shadowing the standard library's glob module. +""" +import ast +import importlib.util +import os +import re +from pathlib import Path + +GLOB_DIR = Path(os.environ.get("MANAGER_GLOB_DIR") or (Path(__file__).resolve().parent.parent / "glob")) + + +def load_manager_util(): + path = GLOB_DIR / "manager_util.py" + spec = importlib.util.spec_from_file_location("_manager_util_under_test", path) + assert spec is not None and spec.loader is not None, f"cannot load {path}" + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + +def load_markdown_functions(manager_util): + source = (GLOB_DIR / "manager_server.py").read_text(encoding="utf-8") + names = {"convert_markdown_to_html", "populate_markdown"} + functions = [node for node in ast.parse(source).body + if isinstance(node, ast.FunctionDef) and node.name in names] + assert len(functions) == len(names) and {node.name for node in functions} == names + namespace = {"re": re, "manager_util": manager_util} + exec(compile(ast.Module(body=functions, type_ignores=[]), "", "exec"), namespace) + return namespace diff --git a/tests/test_grid_highlight_escaping.py b/tests/test_grid_highlight_escaping.py new file mode 100644 index 000000000..51238e22f --- /dev/null +++ b/tests/test_grid_highlight_escaping.py @@ -0,0 +1,189 @@ +"""Exercise Manager's search overrides with the shipped TurboGrid in Chromium. + +Browser cases require Playwright and its Chromium install. All requests are +intercepted locally; no ComfyUI server or external services are used. +""" +import os +from pathlib import Path + +import pytest + +JS = Path(os.environ.get("MANAGER_JS_DIR") or (Path(__file__).resolve().parent.parent / "js")) + + +@pytest.mark.parametrize("filename", ["custom-nodes-manager.js", "model-manager.js"]) +def test_searching_managers_use_the_safe_grid(filename): + source = (JS / filename).read_text() + assert 'import ManagerGrid from "./manager-grid.js"' in source + assert "new ManagerGrid(container)" in source + assert 'from "./turbogrid.esm.js"' not in source + + +@pytest.fixture(scope="module") +def browser(): + playwright = pytest.importorskip("playwright.sync_api") + with playwright.sync_playwright() as driver: + if not Path(driver.chromium.executable_path).exists(): + pytest.skip("install Chromium with: playwright install chromium") + instance = driver.chromium.launch() + yield instance + instance.close() + + +@pytest.fixture +def page(browser): + page = browser.new_page() + + def route(request): + name = request.request.url.rsplit("/", 1)[-1] + if name in {"manager-grid.js", "turbogrid.esm.js"}: + request.fulfill(path=str(JS / name), content_type="text/javascript") + elif request.request.url == "http://manager.test/": + request.fulfill( + body='
    ', + content_type="text/html", + ) + else: + request.abort() + + page.route("**/*", route) + page.goto("http://manager.test/") + page.evaluate("""async () => { + const {default: ManagerGrid} = await import('/manager-grid.js'); + window.grid = new ManagerGrid(document.getElementById('grid')); + grid.setData({columns: [{id: 'name', name: 'Name'}], rows: []}); + grid.render(); + }""") + page.wait_for_function("window.grid.options !== undefined") + yield page + page.close() + + +@pytest.mark.parametrize("keyword", ["before", "img", "after"]) +def test_highlighting_keeps_all_text_inert(page, keyword): + text = 'before & after' + result = page.evaluate("""({text, keyword}) => { + const cell = document.createElement('div'); + cell.id = 'cell'; + cell.textContent = text; + document.body.append(cell); + grid.highlightKeywordsSync([cell], [keyword]); + return {text: cell.textContent, marks: [...cell.querySelectorAll('mark')].map(x => x.textContent), + images: cell.querySelectorAll('img').length}; + }""", {"text": text, "keyword": keyword}) + assert result == {"text": text, "marks": [keyword], "images": 0} + assert page.evaluate("window.__probe === undefined") + + +def test_keyword_order_continues_across_text_nodes(page): + result = page.evaluate("""() => { + const cell = document.createElement('div'); + cell.innerHTML = 'Alpha beta Alpha beta Alpha'; + grid.highlightKeywordsSync([cell], ['alpha', 'beta']); + return [...cell.querySelectorAll('mark')].map(x => x.textContent); + }""") + assert result == ["Alpha", "beta", "Alpha", "beta"] + + +def test_search_cache_does_not_execute_html(page): + result = page.evaluate("""() => { + const row = {description: 'Alpha & Beta'}; + const matched = grid.highlightKeywordsFilter(row, ['description'], 'alpha & beta'); + return {matched, text: row.tg_text_description, highlight: row.tg_highlight_description}; + }""") + assert result == {"matched": True, "text": "Alpha & Beta", "highlight": True} + page.wait_for_timeout(50) + assert page.evaluate("window.__probe === undefined") + + +def test_search_preserves_order_columns_and_empty_query(page): + result = page.evaluate("""() => { + const row = {name: 'Alpha Beta', description: 'Gamma', missing: null, count: 0}; + const columns = ['name', 'description', 'missing', 'count']; + const ordered = grid.highlightKeywordsFilter(row, columns, ' ALPHA beta '); + const reversed = grid.highlightKeywordsFilter(row, columns, 'beta alpha'); + const acrossColumns = grid.highlightKeywordsFilter(row, columns, 'alpha gamma'); + const zero = grid.highlightKeywordsFilter(row, columns, '0'); + const empty = grid.highlightKeywordsFilter(row, columns, ''); + return {ordered, reversed, acrossColumns, zero, empty, + flags: columns.map(column => row['tg_highlight_' + column])}; + }""") + assert result == {"ordered": True, "reversed": False, "acrossColumns": False, + "zero": True, "empty": True, "flags": [None] * 4} + + +def test_custom_text_generator_and_cache_keys(page): + result = page.evaluate("""() => { + Object.assign(grid.options.highlightKeywords, { + textKey: 'text_', highlightKey: 'mark_', + textGenerator: (row, column) => row[column].label + }); + const row = {name: {label: 'Model <Flux>'}}; + const matched = grid.highlightKeywordsFilter(row, ['name'], 'model '); + return {matched, text: row.text_name, highlight: row.mark_name}; + }""") + assert result == {"matched": True, "text": "Model ", "highlight": True} + + +def test_grid_render_search_and_clear_keep_names_readable(page): + page.evaluate("""() => { + window.query = 'flux'; + grid.setOption({rowFilter: row => grid.highlightKeywordsFilter(row, ['name'], window.query)}); + grid.setData({columns: [{id: 'name', name: 'Name'}], rows: [ + {name: 'Model <Flux> <img src=x onerror=window.__probe=1>'}, + {name: 'Other Model'} + ]}); + grid.render(); + }""") + page.wait_for_function("document.querySelectorAll('#grid mark').length === 1") + assert page.locator("#grid mark").all_text_contents() == ["Flux"] + assert page.evaluate("grid.viewRows.length") == 1 + assert page.locator("#grid img").count() == 0 + assert 'Model ' in page.locator("#grid").inner_text() + + page.evaluate("window.query = 'other'; grid.update()") + page.wait_for_function("document.querySelector('#grid mark')?.textContent === 'Other'") + assert page.evaluate("grid.viewRows.length") == 1 + + page.evaluate("window.query = ''; grid.update()") + page.wait_for_function("grid.viewRows.length === 2 && !document.querySelector('#grid mark')") + assert page.locator("#grid img").count() == 0 + assert page.evaluate("window.__probe === undefined") + + +@pytest.mark.parametrize("filename,column,value,query,nonmatching", [ + ("custom-nodes-manager.js", "title", "Pack <Flux>", "", "<flux>"), + ("custom-nodes-manager.js", "description", "Notes <Flux>", "", "<flux>"), + ("custom-nodes-manager.js", "author", "Writer <Alias>", "<alias>", ""), + ("custom-nodes-manager.js", "author", "Writer ", "", "<alias>"), + ("model-manager.js", "name", "Model <Flux>", "", "<flux>"), + ("model-manager.js", "description", "Notes <Flux>", "", "<flux>"), + ("model-manager.js", "filename", "model <Flux>", "<flux>", ""), + ("model-manager.js", "type", "Type ", "", "<flux>"), +]) +def test_manager_search_matches_visible_text(page, filename, column, value, query, nonmatching): + source = (JS / filename).read_text() + init_body = source.split("\tinitGrid() {", 1)[1].split("\n\t}\n", 1)[0] + common = (JS / "common.js").read_text() + sanitizer = common.split("export function sanitizeHTML(str) {", 1)[1].split("\n}", 1)[0] + escape_cell = "undefined" + if filename == "model-manager.js": + escape_cell = source.split("const escapeCell = ", 1)[1].split(";", 1)[0] + page.evaluate("""({init_body, sanitizer, escape_cell, column, value, query}) => { + const sanitizeHTML = new Function('str', sanitizer); + const escapeCell = new Function('sanitizeHTML', `return (${escape_cell})`)(sanitizeHTML); + const initGrid = new Function('ManagerGrid', 'createFlyover', 'gridId', 'sanitizeHTML', 'escapeCell', + `return function initGrid() {${init_body}}`)(grid.constructor, () => ({}), 'review', sanitizeHTML, escapeCell); + const element = document.createElement('div'); + element.innerHTML = '
    '; + document.body.append(element); + window.manager = {element, keywords: query, showStatus() {}, handleFlyoverHover() {}, createFlyover: () => ({}), hasAlternatives: () => false}; + initGrid.call(manager); + manager.grid.setData({columns: [{id: column, name: column}], rows: [{[column]: value}]}); + manager.grid.render(); + }""", {"init_body": init_body, "sanitizer": sanitizer, "escape_cell": escape_cell, + "column": column, "value": value, "query": query}) + page.wait_for_function("manager.grid.viewRows !== undefined") + assert page.evaluate("manager.grid.viewRows.length") == 1 + page.evaluate("query => {manager.keywords=query;}", nonmatching) + assert not page.evaluate("manager.grid.options.rowFilter(manager.grid.data.rows[0])") diff --git a/tests/test_manager_markdown_escaping.py b/tests/test_manager_markdown_escaping.py new file mode 100644 index 000000000..78bc9cfaa --- /dev/null +++ b/tests/test_manager_markdown_escaping.py @@ -0,0 +1,331 @@ +"""RED->GREEN guard for the unescaped-innerHTML regression in the legacy Manager UI. + +A node pack `description` is markdown-transformed server-side and then written to +innerHTML by the client. `convert_markdown_to_html`'s `replace_a` interpolated the +captured URL RAW into a single-quoted href, and the URL pattern is `([^)]+)` — so a +URL carrying a single quote closed the attribute and injected an event handler. +No `<` or `>` is needed, which is why the upstream `<`/`>` escaping did not stop it. + +IMPORT DISCIPLINE (load-bearing, see tests/conftest.py): + - `glob/` has NO `__init__.py`, so `import glob.manager_util` is not a package + import, and putting `glob/` on sys.path would SHADOW the stdlib `glob` module + and break pytest itself. manager_util is therefore loaded by FILE LOCATION + under a private module name — it is the real module, really executed. + - `glob/manager_server.py` starts a network thread at import and needs the + ComfyUI stack, so it is NEVER imported. `convert_markdown_to_html` is + AST-extracted and executed in an isolated namespace instead — the same + technique tests/test_csrf_content_type_helper.py already uses in this repo. +""" +import unittest + +from manager_test_utils import load_manager_util, load_markdown_functions + + +def _parse_anchors(html): + """Return one dict of attributes per tag, using a real HTML parser.""" + from html.parser import HTMLParser + + class _Collector(HTMLParser): + def __init__(self): + super().__init__() + self.anchors = [] + + def handle_starttag(self, tag, attrs): + if tag == "a": + self.anchors.append(dict(attrs)) + + collector = _Collector() + collector.feed(html) + collector.close() + return collector.anchors + + +def _parse_element_names(html): + """Return the tag name of every element the HTML actually produces.""" + from html.parser import HTMLParser + + class _Collector(HTMLParser): + def __init__(self): + super().__init__() + self.names = [] + + def handle_starttag(self, tag, attrs): + self.names.append(tag) + + collector = _Collector() + collector.feed(html) + collector.close() + return collector.names + + +MANAGER_UTIL = load_manager_util() +CONVERT = load_markdown_functions(MANAGER_UTIL)["convert_markdown_to_html"] + +# Synthetic stand-in for the crafted payload. It keeps a LEGITIMATE https scheme +# on purpose: a scheme allow-list alone does not stop this one, so the test pins +# the attribute ESCAPING rather than the allow-list. +BREAKOUT_URL = "https://example.com/x' onmouseover='alert`1`" +BREAKOUT_MD = f"[a/click me]({BREAKOUT_URL})" + + +class TestAttributeBreakout(unittest.TestCase): + def test_quote_in_url_cannot_close_the_href_attribute(self): + html = CONVERT(BREAKOUT_MD) + self.assertNotIn( + "' onmouseover='", html, + f"URL quote escaped the href attribute and injected a handler: {html!r}", + ) + + def test_transformed_anchor_exposes_no_event_handler_attribute(self): + # Parsed, not regex-matched: after the fix the payload text still LIVES + # inside the href value (harmlessly, as an odd URL), so a naive + # /\son[a-z]+=/ scan would flag the fixed output too. What actually + # matters is which ATTRIBUTES the anchor ends up carrying. + html = CONVERT(BREAKOUT_MD) + anchors = _parse_anchors(html) + self.assertEqual(len(anchors), 1, f"expected one anchor, got {html!r}") + names = set(anchors[0]) + self.assertEqual( + names, {"href", "target", "rel"}, + f"unexpected attribute(s) on the anchor: {sorted(names)} in {html!r}", + ) + + def test_legit_link_still_renders_a_usable_anchor(self): + html = CONVERT("see [a/the docs](https://example.com/guide?a=1&b=2) please") + self.assertIn("the docs", html) + self.assertIn("target='_blank'", html) + + def test_link_text_is_escaped_once(self): + for label in ["a tag", "a <b> tag"]: + with self.subTest(label=label): + html = CONVERT(f"[a/{label}](https://example.com)") + self.assertIn("<b>", html) + self.assertNotIn("&lt;", html) + + def test_prose_and_link_labels_are_safe_inside_formatted_notes(self): + item = {"description": ' [w/Read [a/****](https://example.com/?q=)]\n'} + load_markdown_functions(MANAGER_UTIL)["populate_markdown"](item) + html = item["description"] + self.assertEqual(_parse_element_names(html), ["p", "a", "b", "br"]) + self.assertIn("<Flux>", html) + self.assertIn("<img src=x>", html) + self.assertIn("<img src=y>", html) + self.assertEqual(_parse_anchors(html)[0]["href"], "https://example.com/?q=") + + +class TestDescriptionUrlRoundTrip(unittest.TestCase): + def test_description_urls_are_escaped_once(self): + populate = load_markdown_functions(MANAGER_UTIL)["populate_markdown"] + cases = [ + ("https://example.com/?q=", "https://example.com/?q="), + ("https://example.com/?a=1&b=2", "https://example.com/?a=1&b=2"), + ("https://example.com/?q=<Flux>", "https://example.com/?q="), + ("https://example.com/?q=<Flux>", "https://example.com/?q="), + ("https://example.com/?a=1¬ebook=2©=3", "https://example.com/?a=1¬ebook=2©=3"), + ("https://example.com/?q=¬it;", "https://example.com/?q=¬it;"), + ("https://example.com/?q=&lt;", "https://example.com/?q=<"), + ("https://example.com/a**b**c?x=1&y=2", "https://example.com/a**b**c?x=1&y=2"), + ] + for url, expected in cases: + with self.subTest(url=url): + item = {"description": f"[a/link]({url})"} + populate(item) + anchors = _parse_anchors(item["description"]) + self.assertEqual(anchors[0]["href"], expected) + self.assertEqual(_parse_element_names(item["description"]), ["a"]) + + def test_decoded_entities_cannot_bypass_url_or_attribute_checks(self): + populate = load_markdown_functions(MANAGER_UTIL)["populate_markdown"] + for url in ["javascript:alert`1`", "javascript:alert`1`", "java script:alert`1`"]: + with self.subTest(url=url): + item = {"description": f"[a/link]({url})"} + populate(item) + self.assertEqual(_parse_anchors(item["description"])[0]["href"], "#") + + url = "https://example.com/x' onmouseover='alert`1`" + item = {"description": f"[a/link]({url})"} + populate(item) + anchor = _parse_anchors(item["description"])[0] + self.assertEqual(anchor["href"], "https://example.com/x' onmouseover='alert`1`") + self.assertEqual(set(anchor), {"href", "target", "rel"}) + + +class TestEscapedHrefSurvivesTheLaterPasses(unittest.TestCase): + """The escaper's output must still be intact by the time it is returned. + + replace_a runs before the %%..%% / **..** / [w/..] / [i/..] passes, and those + passes used to rewrite the INSIDE of the href it had just secured — putting + //

    markup and fresh single quotes back into the attribute. That + both corrupted legitimate URLs and undid the escaping. + """ + + def test_percent_markers_in_a_url_leave_the_href_intact(self): + html = CONVERT("[a/t](https://e.com/%%A%%)") + self.assertIn("href='https://e.com/%%A%%'", html) + self.assertNotIn("", html) + + def test_note_markers_in_a_url_leave_the_href_intact(self): + html = CONVERT("[a/t](https://e.com/[w/A])") + self.assertIn("href='https://e.com/[w/A]'", html) + self.assertNotIn("cm-warn-note", html) + + def test_no_element_leaks_out_of_a_url(self): + # Parsed rather than string-matched: the anchor must be the ONLY element. + html = CONVERT("[a/t](https://e.com/%%A%%and**B**)") + self.assertEqual(_parse_element_names(html), ["a"]) + + def test_nested_markdown_in_the_LINK_TEXT_still_renders(self): + # Protecting the href must not cost the label. Moving the anchor + # substitution last would have escaped these into visible mojibake. + self.assertIn("bold", CONVERT("[a/**bold** text](https://e.com)")) + self.assertIn("hi", CONVERT("[a/%%hi%% there](https://e.com)")) + + def test_markup_outside_a_link_is_unaffected(self): + html = CONVERT("%%white%% and **bold** and [w/warn]") + self.assertIn("white", html) + self.assertIn("bold", html) + self.assertIn("cm-warn-note", html) + + def test_the_newline_pass_cannot_reach_inside_the_href(self): + # The newline substitution is the LAST thing convert_markdown_to_html + # does, so restoring the href before it let '
    ' be injected into the + # attribute the escaper had secured. Not exploitable — '
    ' carries no + # quote — but it is the same class of breach this function exists to fix, + # so the invariant is pinned all the way to the returned string. + html = CONVERT("[a/t](https://e.com/a\nb)") + anchors = _parse_anchors(html) + self.assertEqual(len(anchors), 1) + self.assertNotIn("
    ", anchors[0]["href"]) + self.assertEqual(_parse_element_names(html), ["a"]) + + def test_newlines_outside_a_link_still_become_line_breaks(self): + html = CONVERT("first\nsecond") + self.assertIn("
    ", html) + + def test_a_forged_placeholder_in_the_input_cannot_hijack_restoration(self): + html = CONVERT("\x00H0\x00 [a/t](https://e.com/real)") + self.assertIn("href='https://e.com/real'", html) + self.assertEqual(html.count("https://e.com/real"), 1) + + +class TestUrlSchemeAllowlist(unittest.TestCase): + def test_helpers_are_public_on_manager_util(self): + self.assertTrue(callable(getattr(MANAGER_UTIL, "escape_html_attribute", None))) + self.assertTrue(callable(getattr(MANAGER_UTIL, "sanitize_url", None))) + + def test_dangerous_schemes_are_rejected(self): + for bad in [ + "javascript:alert`1`", + "JaVaScRiPt:alert`1`", + " javascript:alert`1`", + "java\tscript:alert`1`", + "java\nscript:alert`1`", + "\x01javascript:alert`1`", + "data:text/html;base64,PHNjcmlwdD4=", + "vbscript:msgbox", + ]: + with self.subTest(bad=bad): + self.assertEqual(MANAGER_UTIL.sanitize_url(bad), "#") + + def test_safe_urls_pass_through(self): + for good in [ + "https://example.com/a?b=1", + "http://example.com", + "HTTPS://Example.COM/Path", + "/relative/path", + "relative/path", + "#in-page-anchor", + "//protocol-relative/path", + # A relative link may legitimately contain '&'. An earlier version + # rejected this shape while guarding against an entity-hidden colon; + # the guarantee comes from the escape layer instead (below), so the + # rejection was redundant and only broke real links. + "a&b/c", + "docs/page?x=1&y=2", + ]: + with self.subTest(good=good): + self.assertEqual(MANAGER_UTIL.sanitize_url(good), good) + + def test_entity_hidden_colon_is_inert_after_escaping(self): + # Asserted on the POST-ESCAPE href, because that is where the safety + # actually comes from: sanitize_url returns these unchanged (they have no + # real colon, so they read as relative), and escape_html_attribute then + # escapes the '&'. Entity decoding in an attribute is single-pass, so + # what the browser ends up with is a literal string, never a scheme. + for hidden in ["javascript:alert`1`", "javascript:alert`1`", + "javascript:alert`1`"]: + with self.subTest(hidden=hidden): + href = MANAGER_UTIL.escape_html_attribute(MANAGER_UTIL.sanitize_url(hidden)) + self.assertIn("&", href) + self.assertNotIn(":", href) + self.assertNotIn("j", href) + self.assertNotIn("j", href) + + def test_entity_hidden_colon_in_markdown_produces_no_live_scheme(self): + html = CONVERT("[a/click](javascript:alert`1`)") + anchors = _parse_anchors(html) + self.assertEqual(len(anchors), 1) + # The parser decodes entities exactly as a browser would; after that the + # href must NOT be a javascript: URL. + self.assertFalse(anchors[0]["href"].lower().startswith("javascript:")) + + def test_dangerous_scheme_in_markdown_does_not_reach_the_href(self): + html = CONVERT("[a/click](javascript:alert`1`)") + self.assertNotIn("javascript:", html.lower()) + self.assertIn("href='#'", html) + + +class TestAttributeEscaper(unittest.TestCase): + def test_escapes_all_five_characters(self): + self.assertEqual( + MANAGER_UTIL.escape_html_attribute("""&<>"'"""), + "&<>"'", + ) + + def test_ampersand_is_escaped_first_and_only_once(self): + self.assertEqual(MANAGER_UTIL.escape_html_attribute("a&b"), "a&b") + + def test_non_string_input_is_coerced(self): + self.assertEqual(MANAGER_UTIL.escape_html_attribute(None), "None") + + +class TestServerTitleTransformIsTheOnlyUpstreamControl(unittest.TestCase): + """Pin the server-side facts that the bare flyover title sinks rest on. + + js/custom-nodes-manager.js says, at both flyover pack-title sinks, that the + title is already escaped by populate_markdown and so must NOT be escaped + again at the client (a second escape would double-escape a legitimate + <>-title into mojibake). That comment is a load-bearing claim about THIS + file's subject: it is the whole reason those two sinks are bare. These + cases make the claim machine-checked, so if the server side ever stops + covering `title` the suite says so, instead of leaving a comment asserting + a protection that is gone. + """ + + def test_populate_markdown_escapes_the_title_before_it_reaches_the_client(self): + # The specific link the sink comments name. This is the case that fails + # if someone drops `title` from populate_markdown, which is the drift + # the bare sinks are actually exposed to. + populate_markdown = load_markdown_functions(MANAGER_UTIL)["populate_markdown"] + pack = {"title": "Nodes for workflows"} + populate_markdown(pack) + self.assertEqual(pack["title"], "Nodes for <Flux> workflows") + + def test_sanitize_tag_escapes_angle_brackets(self): + self.assertEqual(MANAGER_UTIL.sanitize_tag(""), "<img>") + + def test_sanitize_tag_does_not_escape_quotes_or_ampersand(self): + # Documented limit, not a defect: both title interpolations are in + # element-text position, where quotes are inert. It IS the reason a + # title moving into attribute position would need escaping. + self.assertEqual(MANAGER_UTIL.sanitize_tag("\"a\" & 'b'"), "\"a\" & 'b'") + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_message_sink_provenance.py b/tests/test_message_sink_provenance.py new file mode 100644 index 000000000..8a662341a --- /dev/null +++ b/tests/test_message_sink_provenance.py @@ -0,0 +1,487 @@ +"""Inventory the current direct message-sink calls and reviewed error builders. + +These sinks accept HTML because some callers render buttons and server-escaped +metadata. Unknown arguments or assignments require a provenance review. Runtime +rendering tests separately verify that raw text stays inert and escaped titles +remain readable; this source inventory is not a general JavaScript dataflow check. +""" +import re +import tempfile +import unittest +from collections import Counter +from pathlib import Path +from unittest.mock import patch + +from js_lift import _skip_literal + +REPO_ROOT = Path(__file__).resolve().parent.parent +JS_DIR = REPO_ROOT / "js" + +SINK_FILES = [ + "common.js", + "custom-nodes-manager.js", + "model-manager.js", + "node-usage-analyzer.js", +] + +SINK_METHODS = r"showStatus|showMessage|showError|showSelection" + +# Capture receivers so a new alias is reviewed instead of silently omitted. +CALL_RE = re.compile( + r"(? str: + """Read a complete argument, including multiline templates.""" + depth = 1 + end = start + while depth: + skipped = _skip_literal(source, end) + if skipped != end: + end = skipped + continue + if source[end] == "(": + depth += 1 + elif source[end] == ")": + depth -= 1 + end += 1 + if source[end:end + 1] == ";": + end += 1 + return re.sub(r"\s+", " ", source[start:end].strip()) + + +def _call_sites(): + """Return (file, line, receiver, method, argument) for direct calls.""" + sites = [] + for name in SINK_FILES: + source = (JS_DIR / name).read_text(encoding="utf-8") + matches = list(CALL_RE.finditer(source)) + parsed = {match.end() for match in matches} + for candidate in SINK_CALL_RE.finditer(source): + if candidate.end() not in parsed: + lineno = source.count("\n", 0, candidate.start()) + 1 + raise AssertionError(f"Unsupported message-sink call at {name}:{lineno}") + for match in matches: + lineno = source.count("\n", 0, match.start()) + 1 + sites.append((name, lineno, match.group(1), match.group(2), + _argument(source, match.end()))) + return sites + + +# --------------------------------------------------------------------------- +# THE REGISTRY. Keyed by (file, method, argument-source). Every showStatus and +# showMessage call site must appear here with a provenance verdict and a reason. +# --------------------------------------------------------------------------- +REGISTRY = { + # --- custom-nodes-manager.js ------------------------------------------- + ("custom-nodes-manager.js", "showStatus", + "`${prevViewRowsLength.toLocaleString()} custom nodes`);"): + (NO_DATA, "a formatted row count"), + ("custom-nodes-manager.js", "showStatus", + "`Loading node mappings (${mode}) ...`);"): + (NO_DATA, "`mode` is a datasrc-combo value chosen in the UI, not channel data"), + ("custom-nodes-manager.js", "showStatus", + "`${label} ${item.title} ...`);"): + (SERVER_ESCAPED, + "item.title comes from /customnode/getlist, where populate_markdown -> " + "sanitize_tag has already escaped it; escaping again shows entities"), + ("custom-nodes-manager.js", "showStatus", + "`${label} ${Object.keys(result).length} custom node(s) successfully`);"): + (NO_DATA, "a count of queue results"), + ("custom-nodes-manager.js", "showStatus", + "`Loading missing nodes (${mode}) ...`);"): + (NO_DATA, "UI-chosen mode"), + ("custom-nodes-manager.js", "showStatus", + "`Loading alternatives (${mode}) ...`);"): + (NO_DATA, "UI-chosen mode"), + ("custom-nodes-manager.js", "showStatus", + "`Loading workflow usage analysis ...`);"): + (NO_DATA, "hardcoded"), + ("custom-nodes-manager.js", "showStatus", + "`Loading custom nodes (${mode}) ...`);"): + (NO_DATA, "UI-chosen mode"), + ("custom-nodes-manager.js", "showMessage", + "`To apply the installed/updated/disabled/enabled custom node, please restart " + "ComfyUI. And refresh browser.`, \"red\");"): + (NO_DATA, "hardcoded"), + ("custom-nodes-manager.js", "showMessage", "\"\");"): + (NO_DATA, "clears the banner"), + + # --- model-manager.js --------------------------------------------------- + ("model-manager.js", "showStatus", + "`${grid.viewRows.length.toLocaleString()} external models`);"): + (NO_DATA, "a formatted row count"), + ("model-manager.js", "showStatus", "`Install ${item.name} ...`);"): + (SERVER_ESCAPED, + "item.name comes from /externalmodel/getlist, where populate_markdown -> " + "sanitize_tag has already escaped it. WI-112 added a caller escape here " + "and it rendered '<Flux>' to the user; removing it is the fix"), + ("model-manager.js", "showStatus", "`Install ${result.length} models successfully`);"): + (NO_DATA, "a count of queue results"), + ("model-manager.js", "showStatus", "`Loading external model list ...`);"): + (NO_DATA, "hardcoded"), + ("model-manager.js", "showMessage", + "`To apply the installed model, please click the 'Refresh' button.`, \"red\")"): + (NO_DATA, "hardcoded"), + ("model-manager.js", "showMessage", "\"\");"): + (NO_DATA, "clears the banner"), + + # --- node-usage-analyzer.js -------------------------------------------- + ("node-usage-analyzer.js", "showStatus", + "`${grid.viewRows.length.toLocaleString()} installed packages`);"): + (NO_DATA, "a formatted row count"), + ("node-usage-analyzer.js", "showStatus", + "`Install ${sanitizeHTML(String(item.name))} ...`);"): + (RAW, + "item.name is packKey (loadData sets `name: packKey`) — a raw dict key no " + "server transform touches, so this caller escapes it. The ONLY raw-provenance " + "showStatus caller in the four files"), + ("node-usage-analyzer.js", "showStatus", "msg);"): + (SERVER_ESCAPED, + "uninstallNodes preserves the escaped title or escapes the raw name fallback; " + "test_node_usage_escaping executes that path through the panel sink"), + ("node-usage-analyzer.js", "showStatus", + "`Uninstalled ${targets.length} custom node(s) successfully`);"): + (NO_DATA, "a count of targets"), + ("node-usage-analyzer.js", "showStatus", + "`Install ${Object.keys(result).length} models successfully`);"): + (NO_DATA, "a count of queue results"), + ("node-usage-analyzer.js", "showStatus", + "`Uninstall ${Object.keys(result).length} custom node(s) successfully`);"): + (NO_DATA, "a count of queue results"), + ("node-usage-analyzer.js", "showStatus", "`Analyzing node usage ...`);"): + (NO_DATA, "hardcoded"), + ("node-usage-analyzer.js", "showMessage", + "`To apply the uninstalled custom nodes, please restart ComfyUI and refresh " + "browser.`, \"red\");"): + (NO_DATA, "hardcoded"), + ("node-usage-analyzer.js", "showMessage", + "`To apply the installed model, please click the 'Refresh' button.`, \"red\");"): + (NO_DATA, "hardcoded"), + ("node-usage-analyzer.js", "showMessage", "\"No workflows were found for analysis.\");"): + (NO_DATA, "hardcoded"), + ("node-usage-analyzer.js", "showMessage", "\"\");"): + (NO_DATA, "clears the banner"), + + # --- showError's own delegation, in each of the three classes ----------- + # Not an application caller: this IS `showError(err) { this.showMessage(err, + # "red"); }`. Whether `err` is display-ready is the showError CALLER's + # responsibility, which HtmlByDesignSinkTest below enforces separately. It is + # registered rather than pattern-excluded so that if showError ever stops + # forwarding — or starts forwarding something else — this entry goes stale + # and the guard says so. + ("custom-nodes-manager.js", "showMessage", "err, \"red\");"): + (FORWARDED, "showError delegation; err is escaped by the showError caller"), + ("model-manager.js", "showMessage", "err, \"red\");"): + (FORWARDED, "showError delegation; err is escaped by the showError caller"), + ("node-usage-analyzer.js", "showMessage", "err, \"red\");"): + (FORWARDED, "showError delegation; err is escaped by the showError caller"), +} + +DISPLAY_READY_METHODS = ("showStatus", "showMessage") + +# A caller escape is spelled this way throughout the UI files. +ESCAPE_MARKER = "sanitizeHTML" + + +class MessageSinkRegistryTest(unittest.TestCase): + """The registry must account for every display-ready-sink call site.""" + + @classmethod + def setUpClass(cls): + cls.sites = _call_sites() + + def test_the_four_sink_implementations_all_exist(self): + # If a file stops defining its own sink (e.g. it starts delegating to + # createUIStateManager) the registry's routing assumptions change and + # this guard must be re-derived rather than silently kept. + own = { + "custom-nodes-manager.js": ".cn-manager-status", + "model-manager.js": ".cmm-manager-status", + "node-usage-analyzer.js": ".nu-manager-status", + } + for name, selector in own.items(): + source = (JS_DIR / name).read_text(encoding="utf-8") + self.assertIn("showStatus(msg, color)", source, "%s lost its own sink" % name) + self.assertIn(selector, source) + common = (JS_DIR / "common.js").read_text(encoding="utf-8") + self.assertIn("export function createUIStateManager(", common) + self.assertIn("showStatus: (msg, color)", common) + + def test_every_caller_is_classified(self): + """FAIL-CLOSED: an unregistered call site breaks the build.""" + observed = Counter((name, method, arg) for name, _, _, method, arg in self.sites + if method in DISPLAY_READY_METHODS) + duplicates = { + ('custom-nodes-manager.js', 'showStatus', '`Loading workflow usage analysis ...`);'), + ('node-usage-analyzer.js', 'showMessage', + '`To apply the uninstalled custom nodes, please restart ComfyUI and refresh browser.`, "red");'), + } + expected = Counter({key: 1 + (key in duplicates) for key in REGISTRY}) + self.assertEqual(observed, expected, 'Changed message calls require a provenance review') + + def test_the_observed_receivers_are_the_pinned_set(self): + observed = {receiver for _name, _lineno, receiver, _m, _a in self.sites} + self.assertEqual( + observed, set(PINNED_RECEIVERS), + "the set of receivers calling the message sinks changed: %s. These " + "sinks assign innerHTML and DO NOT escape, so every route to them " + "must be accounted for. Classify the new receiver's call sites in " + "REGISTRY and add it to PINNED_RECEIVERS — do not narrow CALL_RE to " + "make this pass." % (sorted(observed ^ set(PINNED_RECEIVERS)),), + ) + + def test_no_sink_method_is_destructured_out_of_its_receiver(self): + for name in SINK_FILES: + source = (JS_DIR / name).read_text(encoding="utf-8") + with self.subTest(file=name): + self.assertEqual( + DESTRUCTURE_RE.findall(source), [], + "%s destructures a message-sink method out of its receiver. " + "The resulting bare calls are invisible to this guard; call " + "them through their receiver instead." % (name,), + ) + + def test_unsupported_sink_calls_require_review(self): + source = (JS_DIR / "common.js").read_text(encoding="utf-8") + for call in ('this["showMessage"](raw);', + 'createUIStateManager(element).showError(raw);'): + with self.subTest(call=call), patch(__name__ + ".SINK_FILES", ["common.js"]): + with patch.object(Path, "read_text", return_value=source + "\n" + call): + with self.assertRaisesRegex(AssertionError, "Unsupported message-sink call"): + _call_sites() + + def test_the_registry_has_no_stale_entries(self): + """A registry entry whose call site is gone or whose argument changed.""" + live = { + (name, method, arg) + for name, _lineno, _receiver, method, arg in self.sites + if method in DISPLAY_READY_METHODS + } + stale = sorted(key for key in REGISTRY if key not in live) + self.assertEqual( + stale, [], + "REGISTRY entries no longer match any call site — the caller was removed " + "or its argument changed, so its provenance verdict is unverified:\n %s" + % "\n ".join("%s %s(%s" % key for key in stale), + ) + + def test_server_escaped_callers_do_not_escape_again(self): + for (name, method, arg), (provenance, reason) in REGISTRY.items(): + if provenance != SERVER_ESCAPED: + continue + with self.subTest(file=name, method=method, arg=arg): + self.assertNotIn( + ESCAPE_MARKER, arg, + "%s %s escapes an ALREADY-escaped value — the user reads " + "'<Flux>' instead of ''. Reason on file: %s" + % (name, method, reason), + ) + + def test_raw_callers_escape(self): + for (name, method, arg), (provenance, reason) in REGISTRY.items(): + if provenance != RAW: + continue + with self.subTest(file=name, method=method, arg=arg): + self.assertIn( + ESCAPE_MARKER, arg, + "%s %s passes a RAW value into a sink that does not escape. " + "Reason on file: %s" % (name, method, reason), + ) + + def test_no_data_callers_interpolate_no_channel_value(self): + # Interpolations are allowed, but only of things that cannot carry + # channel/registry text: counts, lengths, and the UI-chosen mode. + allowed = re.compile( + r"^\$\{(?:[A-Za-z_.]*\.length|Object\.keys\([A-Za-z_.]+\)\.length" + r"|[A-Za-z_.]+\.toLocaleString\(\)|mode|label" + r"|[A-Za-z_.]*\.length\.toLocaleString\(\))\}$" + ) + for (name, method, arg), (provenance, reason) in REGISTRY.items(): + if provenance != NO_DATA: + continue + for expr in re.findall(r"\$\{[^}]*\}", arg): + with self.subTest(file=name, method=method, expr=expr): + self.assertRegex( + expr, allowed, + "%s %s is classified %s but interpolates %s, which is not a " + "count or a UI-chosen value. Re-classify it. Reason on file: %s" + % (name, method, NO_DATA, expr, reason), + ) + + +class HtmlByDesignSinkTest(unittest.TestCase): + """showSelection / showError render real markup and are NOT display-ready sinks.""" + + @classmethod + def setUpClass(cls): + cls.sites = _call_sites() + + def test_show_selection_receives_no_channel_data(self): + expected = { + "custom-nodes-manager.js": [ + '"");', + '"");', + # Unknown labels in this builder have behavioral coverage. + 'list.join(""));', + ], + "model-manager.js": [ + '"");', + '"");', + '`Selected ${selectedList.length} models `);', + ], + "node-usage-analyzer.js": [ + '"");', + '"");', + '`Selected ${selectedList.length} packages (none can be uninstalled)`);', + '`

    Selected ${installedSelected.length} ' + 'installed packages
    `);', + ], + } + actual = Counter((name, arg) for name, _, _, method, arg in self.sites if method == "showSelection") + self.assertEqual(actual, Counter((name, arg) for name, args in expected.items() for arg in args)) + + def test_show_error_arguments_are_escaped_or_literal(self): + # showError forwards to showMessage, which does not escape, AND its + # errorMsg is separately handed to ComfyUI-core show_message(). So every + # non-literal argument must already be escaped at the call site. + escaped = { + "custom-nodes-manager.js": { + '`Failed to get custom node mappings: ${sanitizeHTML(String(res.error))}`);', + '`Failed to get alternatives: ${sanitizeHTML(String(res.error))}`);', + '`Failed to get workflow data: ${sanitizeHTML(String(result.error))}`);', + }, + "node-usage-analyzer.js": {'sanitizeHTML(String(result.error)));'}, + } + for name, lineno, _receiver, method, arg in self.sites: + if method != "showError": + continue + is_literal = re.fullmatch( + r'''(?:"(?:\\.|[^"\\])*"|'(?:\\.|[^'\\])*')\s*\);''', arg, + ) is not None + # These direct arguments are checked by the per-file guards below. + is_error_var = re.fullmatch(r"errorMsg\s*\);", arg) is not None + with self.subTest(file=name, line=lineno, arg=arg): + if is_literal or is_error_var: + continue + self.assertIn( + arg, escaped.get(name, set()), + "%s:%d passes an unescaped non-literal into showError, which does " + "not escape and whose text also reaches core show_message()" + % (name, lineno), + ) + + def test_every_error_message_builder_escapes_its_server_text(self): + common = { + 'errorMsg = "";', + "errorMsg += `This action is not allowed with this security level configuration.\\n`;", + "errorMsg += sanitizeHTML(await res.text()) + '\\n';", + } + queue_error = "errorMsg += sanitizeHTML(String(v)) + '\\n';" + outdated = "errorMsg += `ComfyUI version is outdated. Please update ComfyUI to use Manager normally.\\n`;" + expected = { + "custom-nodes-manager.js": common | { + "errorMsg = `'${item.title}': `;", + "errorMsg = `Not found custom node: ${hash}`;", + queue_error, outdated, + }, + "model-manager.js": common | { + "errorMsg = `'${item.name}': `;", queue_error, outdated, + }, + "node-usage-analyzer.js": common | { + "errorMsg = `'${sanitizeHTML(String(item.name))}': `;", queue_error, + }, + "common.js": common | {"errorMsg = `'${displayTitle}': `;"}, + } + mutations = re.compile(r"(?", + "${installedSelected[0].title}    ", + "test_show_selection_receives_no_channel_data"), + ("custom-nodes-manager.js", 'let errorMsg = "";', + 'let errorMsg = "";\nerrorMsg += response.message;', + "test_every_error_message_builder_escapes_its_server_text"), + ("model-manager.js", "errorMsg += sanitizeHTML(String(v)) + '\\n';", + "errorMsg += sanitizeHTML(String(v)) + response.message + '\\n';", + "test_every_error_message_builder_escapes_its_server_text"), + ] + for name, before, after, test in cases: + with self.subTest(file=name, replacement=after), tempfile.TemporaryDirectory() as tmp: + directory = Path(tmp) + for filename in SINK_FILES: + source = (JS_DIR / filename).read_text(encoding="utf-8") + if filename == name: + self.assertIn(before, source) + source = source.replace(before, after, 1) + (directory / filename).write_text(source, encoding="utf-8") + with patch(__name__ + ".JS_DIR", directory): + guard = HtmlByDesignSinkTest(test) + guard.sites = _call_sites() + result = unittest.TestResult() + guard.run(result) + self.assertTrue(result.failures) + self.assertEqual(result.errors, []) + + def test_duplicate_calls_require_review(self): + for cls, test, argument in ( + (MessageSinkRegistryTest, 'test_every_caller_is_classified', 'msg);'), + (HtmlByDesignSinkTest, 'test_show_selection_receives_no_channel_data', 'list.join(""));'), + ): + with self.subTest(test=test): + guard = cls(test) + guard.sites = _call_sites() + guard.sites.append(next(site for site in guard.sites if site[-1] == argument)) + result = unittest.TestResult() + guard.run(result) + self.assertTrue(result.failures) + self.assertEqual(result.errors, []) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_model_manager_escaping.py b/tests/test_model_manager_escaping.py new file mode 100644 index 000000000..bca49f408 --- /dev/null +++ b/tests/test_model_manager_escaping.py @@ -0,0 +1,412 @@ +import html +import json +import os +import unittest +from pathlib import Path + +from js_lift import NODE, JsSource, parse as _parse, run_node as _run_node, slice_braced +from manager_test_utils import load_manager_util, load_markdown_functions + +REPO_ROOT = Path(__file__).resolve().parent.parent +JS_DIR = Path(os.environ.get("MANAGER_JS_DIR") or (REPO_ROOT / "js")) +JS = JsSource(JS_DIR) +MANAGER_UTIL = load_manager_util() + + +MODEL_MANAGER_SRC = (JS_DIR / "model-manager.js").read_text(encoding="utf-8") +COMMON_SRC = (JS_DIR / "common.js").read_text(encoding="utf-8") + + +def _lift_sanitize_html() -> str: + return JS.lift_declaration("common.js", "export function sanitizeHTML(") + + +def _lift_icons() -> str: + """The real `icons` constant — the url formatter embeds icons.download.""" + return JS.lift_declaration("common.js", "export const icons = ") + + +def _lift_url_helpers() -> str: + """sanitizeUrl + safeHref, which live in common.js since the consolidation. + + Returns '' on a pre-consolidation tree so the MANAGER_JS_DIR reproduce still + works against an older revision: there the helpers were local to + model-manager.js and _lift_model_manager_helpers picks them up instead. + """ + if "export function sanitizeUrl" not in COMMON_SRC: + return "" + span = JS.lift_span("common.js", "export const SAFE_URL_SCHEMES", + "export const safeHref = (url) => sanitizeHTML(sanitizeUrl(url));") + return span.replace("export ", "") + + +def _lift_model_manager_helpers() -> str: + """The escaping helpers that are still local to model-manager.js. + + escapeCell and renderOptions stay in the client file — they are specific to + its grid and its filter dropdowns. On a pre-consolidation tree this block + also still contains sanitizeUrl/safeHref, which is why the slice starts at + whichever marker that revision actually has. + """ + if "const escapeCell" not in MODEL_MANAGER_SRC: + return "" + marker = ("const SAFE_URL_SCHEMES" if "const SAFE_URL_SCHEMES" in MODEL_MANAGER_SRC + else "const escapeCell") + start = MODEL_MANAGER_SRC.index(marker) + end = MODEL_MANAGER_SRC.index("const renderOptions") + end = MODEL_MANAGER_SRC.index('}).join("");', end) + len('}).join("");') + return MODEL_MANAGER_SRC[start:end] + + +def _slice_column(column_id: str) -> str: + """Return one column-definition object out of the `}, {`-chained columns array. + + These objects are literals inside an array, so the first `{` after the id + marker belongs to the NEXT column — brace matching cannot be used. The + object instead ends at the first `}` sitting at the array's own indent. + """ + start = MODEL_MANAGER_SRC.index("id: %s," % column_id) + end = MODEL_MANAGER_SRC.index("\n\t\t}", start) + return MODEL_MANAGER_SRC[start:end] + + +def _lift_formatter(anchor: str) -> str: + """Lift the ``formatter: ...`` at ``anchor`` as a bare function expression.""" + block = slice_braced(MODEL_MANAGER_SRC, anchor) + marker = "formatter:" + return block[block.index(marker) + len(marker):].strip() + + +NAME_FORMATTER_ANCHOR = "formatter: function(name, rowItem, columnItem, cellNode)" +URL_FORMATTER_ANCHOR = "formatter: (url, rowItem, columnItem)" + +BREAKOUT_HREF = "https://evil.example/a onmouseover=alert(1) x" +QUOTE_BREAKOUT = 'https://evil.example/a">' +TEXT_PAYLOAD = "" +ATTR_BREAKOUT = '" onfocus=alert(1) x="' + + +@unittest.skipIf(NODE is None, "node is required to execute the lifted production JS") +class ModelManagerSinkEscapingTest(unittest.TestCase): + """Every sink is exercised through the REAL lifted code, never a copy.""" + + @classmethod + def setUpClass(cls): + cls.preamble = "\n".join([ + _lift_sanitize_html(), + _lift_icons(), + _lift_url_helpers(), + _lift_model_manager_helpers(), + ]) + + def _eval(self, expressions: dict) -> dict: + body = ",".join("%s: %s" % (json.dumps(k), v) for k, v in expressions.items()) + return _run_node( + "%s\n" + "const nameFormatter = %s;\n" + "const urlFormatter = %s;\n" + "console.log(JSON.stringify({%s}));\n" + % (self.preamble, + _lift_formatter(NAME_FORMATTER_ANCHOR), + _lift_formatter(URL_FORMATTER_ANCHOR), + body) + ) + + def _assert_inert(self, markup: str, allowed_tags): + """No element outside the allow-list, and no event-handler attribute.""" + parsed = _parse(markup) + self.assertEqual( + [t for t in parsed.tags if t not in allowed_tags], [], + "live element injected into: %r" % (markup,), + ) + handlers = [name for name, _ in parsed.attrs if name.lower().startswith("on")] + self.assertEqual(handlers, [], "event-handler attribute injected into: %r" % (markup,)) + + # --- D-IV: name column, previously UNQUOTED href ------------------------ + def test_d4_name_href_survives_breakout_attempts(self): + out = self._eval({ + "space": "nameFormatter('Model', {reference: %s})" % json.dumps(BREAKOUT_HREF), + "quote": "nameFormatter('Model', {reference: %s})" % json.dumps(QUOTE_BREAKOUT), + }) + for markup in out.values(): + self._assert_inert(markup, allowed_tags={"a", "b"}) + self.assertEqual( + sorted(name for name, _ in _parse(markup).attrs), + ["href", "rel", "target"], + "unexpected attributes on the anchor: %r" % (markup,), + ) + + def test_d4_name_href_rejects_dangerous_schemes(self): + out = self._eval({ + "js": "nameFormatter('Model', {reference: 'javascript:alert(1)'})", + "obfuscated": r"nameFormatter('Model', {reference: 'java\tscript:alert(1)'})", + "entity": "nameFormatter('Model', {reference: 'javascript:alert(1)'})", + "data": "nameFormatter('Model', {reference: 'data:text/html,'})", + "ok": "nameFormatter('Model', {reference: 'https://huggingface.co/repo'})", + }) + for key in ("js", "obfuscated", "data"): + href = dict(_parse(out[key]).attrs)["href"] + self.assertEqual(href, "#", "%s: dangerous scheme survived as %r" % (key, href)) + # The entity form is defeated by the attribute escape, not the scheme + # check: `&` becomes `&`, so `:` reaches the browser as text. + self.assertIn("&#58;", out["entity"]) + self.assertEqual(dict(_parse(out["ok"]).attrs)["href"], "https://huggingface.co/repo") + + # --- D-V: url column, quoted but previously unescaped ------------------- + def test_d5_url_href_escaped_inside_its_quotes(self): + out = self._eval({ + "quote": "urlFormatter(%s)" % json.dumps(QUOTE_BREAKOUT), + "attr": "urlFormatter(%s)" % json.dumps(ATTR_BREAKOUT), + "js": "urlFormatter('javascript:alert(1)')", + }) + self._assert_inert(out["quote"], allowed_tags={"a", "svg", "path"}) + self._assert_inert(out["attr"], allowed_tags={"a", "svg", "path"}) + self.assertEqual(dict(_parse(out["js"]).attrs)["href"], "#") + + # --- D-III: the four previously formatter-less columns ------------------ + def test_d3_columns_bind_the_escaping_formatter(self): + for column in ("'type'", "'base'", '"save_path"', "'filename'"): + block = _slice_column(column) + self.assertIn( + "formatter: escapeCell", block, + "column %s still renders RAW model-list.json into innerHTML" % column, + ) + + def test_d3_escape_cell_neutralises_payload_and_keeps_empty_cells(self): + out = _run_node( + "%s\nconsole.log(JSON.stringify({" + "payload: escapeCell(%s)," + "nul: escapeCell(null), undef: escapeCell(undefined)," + "num: escapeCell(42), plain: escapeCell('checkpoints')" + "}));\n" % (self.preamble, json.dumps(TEXT_PAYLOAD)) + ) + self._assert_inert(out["payload"], allowed_tags=set()) + self.assertEqual(html.unescape(out["payload"]), TEXT_PAYLOAD, + "the payload must survive as visible text") + # null/undefined pass through so turbogrid keeps rendering an empty cell. + self.assertIsNone(out["nul"]) + self.assertNotIn("undef", out) + self.assertEqual(out["num"], "42") + self.assertEqual(out["plain"], "checkpoints") + + # --- D-VI: the showStatus message sink ---------------------------------- + def test_d6_show_status_renders_item_name_inert(self): + """item.name must reach .cmm-manager-status innerHTML inert. + + This used to assert that the CALL SITE carried a sanitizeHTML. That was + wrong in a way the assertion could not see: ``item.name`` arrives from + ``/externalmodel/getlist`` ALREADY escaped by ``populate_markdown`` -> + ``sanitize_tag``, so escaping it again rendered ``<Flux>`` to the + user. The call-site escape was removed for that reason, and the sink + deliberately does not escape either -- see + tests/test_message_sink_provenance.py for why a blanket sink escape + cannot be correct across these callers. + + The GUARANTEE is unchanged, and is now asserted end-to-end on the value + the caller ACTUALLY receives: take the server's own output for a hostile + name, push it through the REAL lifted sink, and read innerHTML. That is + strictly stronger than the old form, which passed whether or not the + rendered result was inert. + """ + anchor = "this.showStatus(`Install " + line = MODEL_MANAGER_SRC[MODEL_MANAGER_SRC.index(anchor):] + line = line[:line.index("\n")] + self.assertNotIn( + "sanitizeHTML", line, + "item.name is already server-escaped; escaping here is the mojibake " + "bug this call site used to have: %r" % (line,), + ) + # What populate_markdown -> sanitize_tag puts on the wire for the payload. + server_value = TEXT_PAYLOAD.replace("<", "<").replace(">", ">") + lifted = "function " + slice_braced(MODEL_MANAGER_SRC, "\tshowStatus(msg, color)") + rendered = _run_node( + "%s\nconst sink = %s;\n" + "const item = {name: %s};\n" + "const el = { innerHTML: '' };\n" + "const ctx = { element: { querySelector: () => el } };\n" + "sink.call(ctx, `Install ${item.name} ...`);\n" + "console.log(JSON.stringify({msg: el.innerHTML}));\n" + % (_lift_sanitize_html(), lifted, json.dumps(server_value)) + ) + self._assert_inert(rendered["msg"], allowed_tags=set()) + # ...and the user reads the payload as text, not as entity soup. + self.assertEqual( + html.unescape(rendered["msg"]), "Install %s ..." % TEXT_PAYLOAD + ) + + # --- filter dropdowns: same raw type/base values, different route ------- + def test_filter_dropdown_options_escape_label_and_value(self): + out = _run_node( + "%s\nconsole.log(JSON.stringify({" + "label: renderOptions([{label: %s, value: 'x'}], '')," + "value: renderOptions([{label: 'x', value: %s}], '')," + "plain: renderOptions([{label: 'checkpoints', value: 'checkpoints'}], 'checkpoints')" + "}));\n" % (self.preamble, json.dumps(TEXT_PAYLOAD), json.dumps(ATTR_BREAKOUT)) + ) + self._assert_inert(out["label"], allowed_tags={"option"}) + self._assert_inert(out["value"], allowed_tags={"option"}) + # Selection still matches on the RAW value, and benign data is untouched. + self.assertIn(" selected", out["plain"]) + self.assertIn(">checkpoints", out["plain"]) + + def test_update_filter_routes_all_three_dropdowns_through_render_options(self): + block = slice_braced(MODEL_MANAGER_SRC, "updateFilter()") + for dropdown in ("$filter", "$type", "$base"): + self.assertIn( + "%s.innerHTML = renderOptions(" % dropdown, block, + "%s still builds