nodes/Comfy-Org-ComfyUI-Manager
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Escape untrusted Manager UI content

Browse Source
This commit is contained in:
Dr.Lt.Data
2026-09-19 08:07:44 +09:00
parent 21ab2b78c2
commit 2f7d16f382
30 changed files with 3921 additions and 114 deletions
Download Patch File Download Diff File Expand all files Collapse all files
.gitignore
+3 -1
View File
@@ -16,5 +16,7 @@ comfyworkflows_sharekey
github-stats-cache.json
pip_overrides.json
*.json
# Track JSON test fixtures.
!tests/cases/*.json
check2.sh
/venv/
/venv/
glob/manager_server.py
+30 -23
View File
@@ -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"<a href='{match.group(2)}' target='blank'>{match.group(1)}</a>"
# Entities written in the source URL (e.g. &amp;) 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"<a href='\x00H{len(hrefs) - 1}\x00' target='_blank' rel='noopener noreferrer'>{text}</a>"
def replace_w(match):
return f"<p class='cm-warn-note'>{match.group(1)}</p>"
@@ -990,20 +997,35 @@ def convert_markdown_to_html(input_text):
def replace_white(match):
return f"<font color='white'>{match.group(1)}</font>"
input_text = input_text.replace('\\[', '&#91;').replace('\\]', '&#93;').replace('<', '&lt;').replace('>', '&gt;')
# NUL dropped first so the input cannot forge the href placeholders.
input_text = input_text.replace('\x00', '')
input_text = input_text.replace('\\[', '&#91;').replace('\\]', '&#93;')
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", "<BR>")
return result_text.replace("\n", "<BR>")
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'(<a\s+href="[^"]*"\s*[^>]*)(>)'
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"<HR>ComfyUI: {version_tag} [Desktop]"
@@ -1876,8 +1885,6 @@ async def get_notice(request):
# markdown_content += f"<BR>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp;()"
markdown_content += f"<BR>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):
glob/manager_util.py
+80
View File
@@ -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('<', '&lt;').replace('>', '&gt;')
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 &notebook 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:
js/comfyui-manager.js
+7 -7
View File
@@ -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 += `<li><a href='${url}' target='_blank'>${title}</a></li>`;
msg += `<li><a href='${safeHref(url)}' target='_blank' rel='noopener noreferrer'>${title}</a></li>`;
}
else {
msg += `<li>${k}</li>`;
msg += `<li>${sanitizeHTML(String(k))}</li>`;
}
}
msg += "</ul>";
@@ -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 += `<li><a href='${url}' target='_blank'>${title}</a></li>`;
msg += `<li><a href='${safeHref(url)}' target='_blank' rel='noopener noreferrer'>${title}</a></li>`;
}
else {
msg += `<li>${k}</li>`;
msg += `<li>${sanitizeHTML(String(k))}</li>`;
}
}
js/comfyui-share-common.js
+2 -2
View File
@@ -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: <a href='" + response_json.comfyworkflows.url + "' target='_blank'>" + response_json.comfyworkflows.url + "</a>";
this.final_message.innerHTML = "Your art has been shared: <a href='" + safeHref(response_json.comfyworkflows.url) + "' target='_blank' rel='noopener noreferrer'>" + sanitizeHTML(response_json.comfyworkflows.url) + "</a>";
if (response_json.matrix.success) {
this.final_message.innerHTML += "<br>Your art has been shared in the ComfyUI Matrix server's #share channel!";
}
js/comfyui-share-copus.js
+2 -2
View File
@@ -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. <a href="${url}" target="_blank">Click here to view it.</a>`;
this.message.innerHTML = `Workflow has been shared successfully. <a href="${safeHref(url)}" target="_blank" rel="noopener noreferrer">Click here to view it.</a>`;
this.previewImage.src = "";
this.previewImage.style.display = "none";
this.uploadedImages = [];
js/comfyui-share-openart.js
+2 -2
View File
@@ -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. <a href="${url}" target="_blank">Click here to view it.</a>`;
this.message.innerHTML = `Workflow has been shared successfully. <a href="${safeHref(url)}" target="_blank" rel="noopener noreferrer">Click here to view it.</a>`;
this.previewImage.src = "";
this.previewImage.style.display = "none";
this.uploadedImages = [];
js/comfyui-share-youml.js
+2 -2
View File
@@ -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, ` +
`<a href="${recipePageUrl}" target="_blank">visit it on YouML</a>`;
`<a href="${safeHref(recipePageUrl)}" target="_blank" rel="noopener noreferrer">visit it on YouML</a>`;
this.uploadedImages = [];
this.nameInput.value = "";
js/common.js
+42 -6
View File
@@ -427,6 +427,40 @@ export function sanitizeHTML(str) {
.replace(/'/g, "&#039;");
}
// 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) {
js/custom-nodes-manager.js
+42 -28
View File
@@ -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 `<button class="cn-btn-${id} p-button p-component" group="${action}" mode="${bt.mode}">${bt.label}</button>`;
return `<button class="cn-btn-${id} p-button p-component" group="${sanitizeHTML(String(action))}" mode="${bt.mode}">${bt.label}</button>`;
}).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 = `<b>${title}</b>`;
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 `<div>${version}</div><div>[${rowItem.cnr_latest}]</div>`;
return `<div>${safeVersion}</div><div>[${safeLatest}]</div>`;
}
return `<div>${version}</div><div>[↑${rowItem.cnr_latest}]</div>`;
return `<div>${safeVersion}</div><div>[↑${safeLatest}]</div>`;
}
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 `<span title="This author has been active for more than six months in GitHub">✅ ${author}</span>`;
return `<span title="This author has been active for more than six months in GitHub">✅ ${safeAuthor}</span>`;
}
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 `<span title="${ago}">${short}</span>`;
}
}];
@@ -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 = `<div class="cn-nodes-pack" hash="${rowItem.hash}">${rowItem.title}</div>`;
if (isNotInstalled) {
titleHtml += '<div class="cn-pack-badge">Not Installed</div>'
@@ -1161,7 +1170,8 @@ export class CustomNodesManager {
list.push(`<div class="${rowClass}">`);
list.push(`<div class="cn-nodes-sn">${i+1}</div>`);
list.push(`<div class="cn-nodes-name">${it.name}</div>`);
// extName via /customnode/getmappings, no server sanitize: escaped at the sink.
list.push(`<div class="cn-nodes-name">${sanitizeHTML(String(it.name))}</div>`);
if (it.conflicts) {
list.push(`<div class="cn-conflicts-list"><div class="cn-nodes-conflict cn-icon">${icons.conflicts}</div><b>Conflict with</b>${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(`<div class="cn-selected-buttons">
<span>Selected <b>${selectedMap[v].length}</b> ${filterItem ? filterItem.label : v}</span>
<span>Selected <b>${selectedMap[v].length}</b> ${sanitizeHTML(String(filterItem ? filterItem.label : v))}</span>
${this.grid.hasMask ? "" : this.getActionButtons(v, null, true)}
</div>`);
});
@@ -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 `<div>${tag.trim()}</div>`;
return `<div>${sanitizeHTML(tag.trim())}</div>`;
}).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;
}
js/manager-grid.js
+67
View File
@@ -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 <mark>.
wrapper.append(text.slice(offset));
node.replaceWith(wrapper);
}
}
}
}
js/model-manager.js
+36 -23
View File
@@ -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 `<option value="${sanitizeHTML(String(item.value))}"${selected}>${sanitizeHTML(String(item.label))}</option>`;
}).join("");
const pageHtml = `
<div class="cmm-manager cmm-manager-dark">
<div class="cmm-manager-grid"></div>
@@ -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 `<option value="${item.value}"${selected}>${item.label}</option>`
}).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 `<option value="${item.value}"${selected}>${item.label}</option>`
}).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 `<option value="${item.value}"${selected}>${item.label}</option>`
}).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 `<a href=${rowItem.reference} target="_blank"><b>${name}</b></a>`;
// Names are server-escaped; raw references need URL and attribute handling.
return `<a href="${safeHref(rowItem.reference)}" target="_blank" rel="noopener noreferrer"><b>${name}</b></a>`;
}
}, {
id: 'installed',
@@ -351,7 +356,7 @@ export class ModelManager {
sortable: false,
align: 'center',
formatter: (url, rowItem, columnItem) => {
return `<a class="cmm-btn-download" title="Download file" href="${url}" target="_blank">${icons.download}</a>`;
return `<a class="cmm-btn-download" title="Download file" href="${safeHref(url)}" target="_blank" rel="noopener noreferrer">${icons.download}</a>`;
}
}, {
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;
}
js/node-usage-analyzer.js
+17 -9
View File
@@ -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 `<a href=${rowItem.reference} target="_blank"><b>${name}</b></a>`;
// Names are escaped during loadData; references need URL and attribute handling.
return `<a href="${safeHref(rowItem.reference)}" target="_blank" rel="noopener noreferrer"><b>${name}</b></a>`;
}
}, {
id: 'used_in_count',
@@ -283,7 +285,8 @@ export class NodeUsageAnalyzer {
workflowList.forEach((workflow, i) => {
list.push(`<div class="cn-nodes-row">`);
list.push(`<div class="cn-nodes-sn">${i + 1}</div>`);
list.push(`<div class="cn-nodes-name">${workflow.filename}</div>`);
// Workflow filenames are raw server data.
list.push(`<div class="cn-nodes-name">${sanitizeHTML(String(workflow.filename))}</div>`);
list.push(`<div class="cn-nodes-details">${workflow.nodeCount} node${workflow.nodeCount > 1 ? 's' : ''}</div>`);
list.push(`</div>`);
});
@@ -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;
}
js/snapshot.js
+3 -3
View File
@@ -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(`<BR><font color="green">Restore snapshot '${target.name}'</font>`);
self.updateMessage(`<BR><font color="green">Restore snapshot '${sanitizeHTML(target.name)}'</font>`);
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 = `&nbsp;${data}`;
data2.innerHTML = `&nbsp;${sanitizeHTML(data)}`;
var data_button = document.createElement('td');
data_button.style.textAlign = "center";
data_button.className = "data-btns";
prestartup_script.py
+21 -5
View File
@@ -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.")
pyproject.toml
+1 -1
View File
@@ -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"
requirements.txt
+2
View File
@@ -9,3 +9,5 @@ typing-extensions
toml
uv
chardet
packaging
nh3>=0.3.7
tests/cases/url_sanitize_cases.json
+153
View File
@@ -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&#58;a",
"javascript&colon;a",
"&#x6a;avascript:a",
"&#106;avascript: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<U+FEFF>' -> python 'http://h/x<U+FEFF>', 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."
}
}
tests/js_lift.py
+159
View File
@@ -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 <base>: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")]
tests/manager_test_utils.py
+32
View File
@@ -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=[]), "<markdown-extract>", "exec"), namespace)
return namespace
tests/test_grid_highlight_escaping.py
+189
View File
@@ -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='<div id="grid" style="width:800px;height:300px"></div>',
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 <img src="/bad.png" onerror="window.__probe=1"> & 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 = '<b>Alpha</b> beta Alpha beta <svg><text>Alpha</text></svg><textarea>beta</textarea>';
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: '<img src="/bad.png" onerror="window.__probe=1"><b>Alpha</b> &amp; 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: '<b>Model</b> &lt;Flux&gt;'}};
const matched = grid.highlightKeywordsFilter(row, ['name'], 'model <flux>');
return {matched, text: row.text_name, highlight: row.mark_name};
}""")
assert result == {"matched": True, "text": "Model <Flux>", "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 &lt;Flux&gt; &lt;img src=x onerror=window.__probe=1&gt;'},
{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 <Flux> <img src=x onerror=window.__probe=1>' 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 &lt;Flux&gt;", "<flux>", "&lt;flux&gt;"),
("custom-nodes-manager.js", "description", "Notes &lt;Flux&gt;", "<flux>", "&lt;flux&gt;"),
("custom-nodes-manager.js", "author", "Writer &lt;Alias&gt;", "&lt;alias&gt;", "<alias>"),
("custom-nodes-manager.js", "author", "Writer <Alias>", "<alias>", "&lt;alias&gt;"),
("model-manager.js", "name", "Model &lt;Flux&gt;", "<flux>", "&lt;flux&gt;"),
("model-manager.js", "description", "<b>Notes</b> &lt;Flux&gt;", "<flux>", "&lt;flux&gt;"),
("model-manager.js", "filename", "model &lt;Flux&gt;", "&lt;flux&gt;", "<flux>"),
("model-manager.js", "type", "Type <Flux>", "<flux>", "&lt;flux&gt;"),
])
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 = '<div id="tested-grid" class="cn-manager-grid cmm-manager-grid" style="width:800px;height:300px"></div>';
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])")
tests/test_manager_markdown_escaping.py
+331
View File
@@ -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 <a> 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("<a href='https://example.com/guide?a=1&amp;b=2'", html)
self.assertIn(">the docs</a>", html)
self.assertIn("target='_blank'", html)
def test_link_text_is_escaped_once(self):
for label in ["a <b> tag", "a &lt;b&gt; tag"]:
with self.subTest(label=label):
html = CONVERT(f"[a/{label}](https://example.com)")
self.assertIn("&lt;b&gt;", html)
self.assertNotIn("&amp;lt;", html)
def test_prose_and_link_labels_are_safe_inside_formatted_notes(self):
item = {"description": '<img src=x> [w/Read [a/**<Flux>**](https://example.com/?q=<Flux>)]\n<img src=y>'}
load_markdown_functions(MANAGER_UTIL)["populate_markdown"](item)
html = item["description"]
self.assertEqual(_parse_element_names(html), ["p", "a", "b", "br"])
self.assertIn("<B>&lt;Flux&gt;</B>", html)
self.assertIn("&lt;img src=x&gt;", html)
self.assertIn("&lt;img src=y&gt;", html)
self.assertEqual(_parse_anchors(html)[0]["href"], "https://example.com/?q=<Flux>")
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=<Flux>", "https://example.com/?q=<Flux>"),
("https://example.com/?a=1&amp;b=2", "https://example.com/?a=1&b=2"),
("https://example.com/?q=&lt;Flux&gt;", "https://example.com/?q=<Flux>"),
("https://example.com/?q=&#60;Flux&#x3e;", "https://example.com/?q=<Flux>"),
("https://example.com/?a=1&notebook=2&copy=3", "https://example.com/?a=1&notebook=2&copy=3"),
("https://example.com/?q=&notit;", "https://example.com/?q=&notit;"),
("https://example.com/?q=&amp;lt;", "https://example.com/?q=&lt;"),
("https://example.com/a**b**c?x=1&amp;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&#58;alert`1`", "&#106;avascript:alert`1`", "java&Tab;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&#39; onmouseover=&#39;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
<font>/<B>/<p> 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("<font", html)
def test_bold_markers_in_a_url_leave_the_href_intact(self):
html = CONVERT("[a/t](https://e.com/a**b**c)")
self.assertIn("href='https://e.com/a**b**c'", html)
self.assertNotIn("<B>", 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("<B>bold</B>", CONVERT("[a/**bold** text](https://e.com)"))
self.assertIn("<font color='white'>hi</font>", 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("<font color='white'>white</font>", html)
self.assertIn("<B>bold</B>", 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 '<BR>' be injected into the
# attribute the escaper had secured. Not exploitable — '<BR>' 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("<BR>", 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("<BR>", 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&#58;alert`1`", "&#x6a;avascript:alert`1`",
"&#106;avascript:alert`1`"]:
with self.subTest(hidden=hidden):
href = MANAGER_UTIL.escape_html_attribute(MANAGER_UTIL.sanitize_url(hidden))
self.assertIn("&amp;", href)
self.assertNotIn("&#58;", href)
self.assertNotIn("&#x6a;", href)
self.assertNotIn("&#106;", href)
def test_entity_hidden_colon_in_markdown_produces_no_live_scheme(self):
html = CONVERT("[a/click](javascript&#58;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("""&<>"'"""),
"&amp;&lt;&gt;&quot;&#x27;",
)
def test_ampersand_is_escaped_first_and_only_once(self):
self.assertEqual(MANAGER_UTIL.escape_html_attribute("a&b"), "a&amp;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 <Flux> workflows"}
populate_markdown(pack)
self.assertEqual(pack["title"], "Nodes for &lt;Flux&gt; workflows")
def test_sanitize_tag_escapes_angle_brackets(self):
self.assertEqual(MANAGER_UTIL.sanitize_tag("<img>"), "&lt;img&gt;")
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()
tests/test_message_sink_provenance.py
+487
View File
@@ -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"(?<![\w$.])([\w$]+(?:\??\.[\w$]+)*)"
rf"\??\.({SINK_METHODS})\("
)
SINK_CALL_RE = re.compile(
rf"(?:\.(?:{SINK_METHODS})|\[\s*['\"](?:{SINK_METHODS})['\"]\s*\])"
r"\s*(?:\?\.\s*)?\("
)
PINNED_RECEIVERS = frozenset({"this", "self", "this.ui"})
# Destructuring would hide calls from the receiver-based inventory.
DESTRUCTURE_RE = re.compile(
r"(?:const|let|var)\s*\{[^}]*\b"
rf"(?:{SINK_METHODS})"
r"\b[^}]*\}"
)
SERVER_ESCAPED = "server-escaped" # already display-ready; caller must NOT escape
RAW = "raw" # not display-ready; caller MUST escape
NO_DATA = "no-data" # no channel/registry value interpolated
FORWARDED = "forwarded" # showError's own delegation; see HtmlByDesignSinkTest
def _argument(source: str, start: int) -> 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 '&lt;Flux&gt;' 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 "
"'&lt;Flux&gt;' instead of '<Flux>'. 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": [
'"");',
'"");',
'`<span>Selected <b>${selectedList.length}</b> models <button '
'class="cmm-btn-install p-button p-component" mode="install">Install</button>`);',
],
"node-usage-analyzer.js": [
'"");',
'"");',
'`<span>Selected <b>${selectedList.length}</b> packages (none can be uninstalled)</span>`);',
'` <div class="nu-selected-buttons"> <span>Selected <b>${installedSelected.length}</b> '
'installed packages</span> <button class="nu-btn-uninstall" mode="uninstall">'
'Uninstall Selected</button> </div> `);',
],
}
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"(?<![\w$.])errorMsg\s*(?:[+*/%&|^?-]*=(?!=)|\+\+|--)[^;]*;")
for name, statements in expected.items():
source = (JS_DIR / name).read_text(encoding="utf-8")
actual = {re.sub(r"\s+", " ", match) for match in mutations.findall(source)}
with self.subTest(file=name):
self.assertEqual(actual, statements)
def test_error_guard_rejects_unclassified_arguments(self):
for argument in (
"err);", "errText);", "errorMsg$raw);", "err + extra);", "err.detail);",
'"prefix " + errText);', "'prefix ' + errText);",
"sanitizeHTML(err) + extra);",
):
with self.subTest(argument=argument):
guard = HtmlByDesignSinkTest("test_show_error_arguments_are_escaped_or_literal")
guard.sites = [("custom-nodes-manager.js", 1, "this", "showError", argument)]
result = unittest.TestResult()
guard.run(result)
self.assertEqual(len(result.failures), 1)
self.assertEqual(result.errors, [])
def test_html_guards_reject_unreviewed_source(self):
cases = [
("custom-nodes-manager.js", 'this.showSelection(list.join(""));',
'this.showSelection(selectedList[0].title);',
"test_show_selection_receives_no_channel_data"),
("node-usage-analyzer.js", "${installedSelected.length}</b>",
"${installedSelected[0].title}</b>",
"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()
tests/test_model_manager_escaping.py
+412
View File
@@ -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"><img src=x onerror=alert(1)>'
TEXT_PAYLOAD = "<img src=x onerror=alert(1)>"
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&#58;alert(1)'})",
"data": "nameFormatter('Model', {reference: 'data:text/html,<script>alert(1)</script>'})",
"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 `&amp;`, so `&#58;` reaches the browser as text.
self.assertIn("&amp;#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 ``&lt;Flux&gt;`` 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("<", "&lt;").replace(">", "&gt;")
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</option>", 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 <option> markup inline" % dropdown,
)
# --- description: server-transformed HTML, must NOT be double-escaped ---
def test_description_column_is_not_re_escaped(self):
block = _slice_column("'description'")
self.assertNotIn(
"formatter", block,
"description is server-transformed HTML (populate_markdown); re-escaping "
"it renders visible mojibake such as 'Nodes for &lt;Flux&gt; workflows'",
)
def test_description_server_value_still_reads_cleanly(self):
# What populate_markdown -> sanitize_tag actually emits for "<Flux>".
server_value = "Nodes for &lt;Flux&gt; workflows"
self.assertEqual(html.unescape(server_value), "Nodes for <Flux> workflows")
# Run through the client escaper as well, the user would see the entity
# text instead of the angle brackets — that is what this guard forbids.
double = _run_node(
"%s\nconsole.log(JSON.stringify({v: sanitizeHTML(%s)}));\n"
% (_lift_sanitize_html(), json.dumps(server_value))
)["v"]
self.assertEqual(html.unescape(double), server_value)
@unittest.skipIf(NODE is None, "node is required to execute the lifted production JS")
class ModelManagerMessageSinkTest(unittest.TestCase):
"""model-manager's OWN message sinks — it does not use createUIStateManager.
This class replaces an earlier ``test_turbogrid_bundle_untouched`` guard,
which asserted that ``js/turbogrid.esm.js`` was unchanged relative to a
fixed base commit. That was an INTEGRATION ARTIFACT, not a property of this
file: it encoded "only model-manager was touched by the work item that
added these tests". In the integrated tree the vendored grid IS changed on
purpose, so the assertion reported a cross-file fact that says nothing
about whether model-manager's own sinks are safe. What this file can
actually guarantee is asserted instead — and it is strictly more coverage,
because the sinks below were not covered by anything before.
"""
@classmethod
def setUpClass(cls):
cls.sanitize = _lift_sanitize_html()
def _call_sink(self, method: str, msg, color=None) -> str:
"""Run one lifted sink against a stub element; return the real innerHTML."""
lifted = "function " + slice_braced(MODEL_MANAGER_SRC, "\t%s(msg, color)" % method)
args = json.dumps(msg) + ("" if color is None else ", " + json.dumps(color))
return _run_node(
"%s\n"
"const sink = %s;\n"
"const el = { innerHTML: '' };\n"
"const ctx = { element: { querySelector: () => el } };\n"
"sink.call(ctx, %s);\n"
"console.log(JSON.stringify({v: el.innerHTML}));\n"
% (self.sanitize, lifted, args)
)["v"]
# --- the sinks take DISPLAY-READY text and do NOT escape ---------------
def test_the_sinks_deliberately_do_not_escape(self):
# Structural, and load-bearing: a well-meaning sanitizeHTML added here
# would double-escape the server-escaped values these sinks actually
# receive. tests/test_message_sink_provenance.py is what keeps that safe
# by classifying every caller.
for method in ("showStatus(msg, color)", "showMessage(msg, color)"):
body = slice_braced(MODEL_MANAGER_SRC, "\t" + method)
self.assertNotIn(
"sanitizeHTML", body,
"%s escapes at the sink; its callers pass ALREADY-escaped server "
"values, so this renders '&lt;Flux&gt;' to the user" % method,
)
self.assertIn("innerHTML", body)
def test_a_server_escaped_value_renders_inert_through_the_sink(self):
server_value = "Install %s ..." % TEXT_PAYLOAD.replace(
"<", "&lt;").replace(">", "&gt;")
rendered = self._call_sink("showStatus", server_value)
self.assertNotIn("img", _parse(rendered).tags)
def test_a_benign_angle_bracket_name_is_not_double_escaped(self):
# THE regression this arrangement exists to prevent. The server sends
# 'Model for &lt;Flux&gt; workflows'; the user must read the angle
# brackets, not the entities.
server_value = "Install Model for &lt;Flux&gt; workflows ..."
rendered = self._call_sink("showStatus", server_value)
self.assertEqual(
html.unescape(rendered), "Install Model for <Flux> workflows ..."
)
self.assertNotIn("&amp;lt;", rendered)
def test_the_font_wrapper_still_renders(self):
rendered = self._call_sink("showMessage", "plain text", "red")
collector = _parse(rendered)
self.assertIn("font", collector.tags)
self.assertIn(("color", "red"), collector.attrs)
# --- showError: HTML BY CONTRACT, escaped by its callers ---------------
def test_show_error_forwards_to_the_non_escaping_show_message(self):
# showError is HTML by contract: its errorMsg is built escaped because
# the SAME string also goes to ComfyUI-core show_message(). Forwarding
# to a NON-escaping showMessage keeps that at exactly one escape.
body = slice_braced(MODEL_MANAGER_SRC, "\tshowError(err)")
self.assertIn("this.showMessage(err, \"red\")", body)
# --- the caller side ---------------------------------------------------
def test_the_install_status_caller_passes_the_server_value_untouched(self):
self.assertIn("this.showStatus(`Install ${item.name} ...`)", MODEL_MANAGER_SRC)
def test_the_install_error_message_escapes_only_the_raw_response(self):
# The name is already escaped by the server; the error response is raw.
self.assertIn(
"errorMsg = `'${item.name}': `", MODEL_MANAGER_SRC
)
self.assertIn("errorMsg += sanitizeHTML(await res.text())", MODEL_MANAGER_SRC)
def test_install_failure_preserves_server_escaped_names_in_both_sinks(self):
populate = load_markdown_functions(MANAGER_UTIL)["populate_markdown"]
methods = ",\n".join(slice_braced(MODEL_MANAGER_SRC, marker) for marker in [
"async installModels(list, btn)", "showError(err)", "showMessage(msg, color)", "showStatus(msg, color)"
])
response_text = '<img src=x onerror="window.__probe=1"> download failed'
for name in ["Model <Flux>", '<img src=x onerror="window.__probe=1">']:
for status in [403, 500]:
with self.subTest(name=name, status=status):
item = {"name": name, "originalData": {}, "hash": "model"}
populate(item)
rendered = _run_node("""
%s
const messages = {};
const elements = {};
const api = {fetchApi: async (url) => url.endsWith('/status')
? {json: async () => ({is_processing: false})}
: {status: %d, json: async () => ({}), text: async () => %s}};
const show_message = (message) => messages.dialog = message;
const ctx = {
%s,
element: {querySelector: (selector) => elements[selector] ||= {innerHTML: ''}},
grid: {scrollRowIntoView() {}, updateCell() {}},
focusInstall() {return true;}
};
await ctx.installModels([%s], {classList: {add() {}}});
messages.panel = elements['.cmm-manager-message'].innerHTML;
console.log(JSON.stringify(messages));
""" % (self.sanitize, status, json.dumps(response_text), methods, json.dumps(item)))
self.assertEqual(set(rendered), {"panel", "dialog"})
for message in rendered.values():
self.assertIn("'%s': " % name, html.unescape(message))
self.assertNotIn("img", _parse(message).tags)
if status == 500:
self.assertIn(response_text, html.unescape(message))
def test_the_queue_result_error_text_is_escaped_by_the_caller(self):
self.assertIn("errorMsg += sanitizeHTML(String(v))", MODEL_MANAGER_SRC)
if __name__ == "__main__":
unittest.main()
tests/test_node_usage_escaping.py
+286
View File
@@ -0,0 +1,286 @@
"""RED->GREEN guard for the js/node-usage-analyzer.js innerHTML sinks.
The analyzer writes pack data and workflow data into innerHTML through turbogrid
cells, a flyover, and the status/message elements. Before the fix: the title
column built an UNQUOTED href (a bare space breaks out of the attribute); the
install-progress showStatus interpolated a raw pack key; the usage-details
flyover interpolated a raw workflow filename; and the install error path
interpolated both a raw pack key and raw server text.
One value needed fixing at its SOURCE rather than at the sink. The title column
is built as `pack.title || packKey`: /customnode/getlist runs populate_markdown
over each pack, so `pack.title` is server-escaped, but `packKey` is a dict key
that no server transform touches. Escaping at the formatter would have
double-escaped the common path into visible entity text ("Nodes for &lt;Flux&gt;
workflows"); leaving it bare would have shipped the raw fallback. The fix
normalises the fallback so the column carries ONE provenance, which is what
makes the bare formatter correct by construction.
This grid uses cellResizeObserver only — no rowFilter, no
highlightKeywordsFilter — so the keyword-highlight re-render does not reach it.
The two searchable managers use js/manager-grid.js to secure that path without
modifying the vendored TurboGrid bundle.
The status/message sinks take DISPLAY-READY text and do NOT escape, because
their callers carry mixed provenance; every caller is classified in
tests/test_message_sink_provenance.py. This analyzer is the only file that
reaches BOTH sink implementations — the shared one from common.js via `this.ui`
and its own class methods.
REPRODUCE THE RED HALF against the pre-fix revision — one command:
mkdir -p /some/dir
git show <base>:js/node-usage-analyzer.js > /some/dir/node-usage-analyzer.js
git show <base>:js/common.js > /some/dir/common.js
MANAGER_JS_DIR=/some/dir pytest tests/test_node_usage_escaping.py
"""
import html
import json
import os
import unittest
from pathlib import Path
from js_lift import (
NODE,
JsSource,
event_handlers,
parse,
run_node,
slice_braced,
)
REPO_ROOT = Path(__file__).resolve().parent.parent
JS = JsSource(os.environ.get("MANAGER_JS_DIR") or (REPO_ROOT / "js"))
ANALYZER = "node-usage-analyzer.js"
COMMON = "common.js"
TITLE_FORMATTER_ANCHOR = "formatter: function (name, rowItem, columnItem, cellNode)"
BREAKOUT_HREF = "https://evil.example/a onmouseover=alert(1) x"
QUOTE_BREAKOUT = 'https://evil.example/a"><img src=x onerror=alert(1)>'
# An ordinary pack reference, for the legs that ask what a CORRECT link
# looks like rather than what a hostile one cannot do.
BENIGN_REFERENCE = "https://github.com/owner/repo"
TEXT_PAYLOAD = "<img src=x onerror=alert(1)>"
def _url_helper_home() -> str:
"""Which file currently owns sanitizeUrl/safeHref, or '' on a pre-fix tree.
They were local to the analyzer when this guard was written and now live in
common.js, imported by every client file. Resolving the home rather than
hard-coding it keeps the MANAGER_JS_DIR reproduce working against both
revisions, and makes a THIRD home fail loudly instead of being lifted twice.
"""
for filename in (COMMON, ANALYZER):
if "SAFE_URL_SCHEMES" in JS.text(filename):
return filename
return ""
def _preamble() -> str:
parts = [JS.lift_declaration(COMMON, "export function sanitizeHTML(")]
home = _url_helper_home()
if home:
span = JS.lift_span(
home, "SAFE_URL_SCHEMES", "safeHref = (url) => sanitizeHTML(sanitizeUrl(url));")
parts.append("const " + span.replace("export ", ""))
return "\n".join(parts)
@unittest.skipIf(NODE is None, "node is required to execute the lifted production JS")
class NodeUsageSinkEscapingTest(unittest.TestCase):
"""Every sink is exercised through the REAL lifted code, never a copy."""
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,),
)
self.assertEqual(
event_handlers(markup), [],
"event-handler attribute injected into: %r" % (markup,),
)
# --- C2: title column, previously UNQUOTED href -------------------------
def _render_title(self, reference: str, name: str = "My Pack") -> str:
return run_node(
"%s\nconst titleFormatter = %s;\n"
"console.log(JSON.stringify({markup: titleFormatter(%s, {reference: %s})}));\n"
% (_preamble(),
JS.lift_formatter(ANALYZER, TITLE_FORMATTER_ANCHOR),
json.dumps(name), json.dumps(reference))
)["markup"]
def test_c2_href_survives_breakout_attempts(self):
for reference in (BREAKOUT_HREF, QUOTE_BREAKOUT):
markup = self._render_title(reference)
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_c2_title_anchor_severs_the_opener(self):
"""The pack chooses this host, so the page it opens must not keep us.
`reference` is channel data, so the pack author picks the page that
opens; without rel that page holds a live window.opener back into the
Manager UI. Membership on the SPLIT attribute, not a literal match:
'noreferrer noopener' is the same thing, and pinning token order would
report a correct fix as a defect.
"""
markup = self._render_title(BENIGN_REFERENCE)
attrs = dict(parse(markup).attrs)
self.assertEqual(
attrs.get("target"), "_blank",
"this guard is about _blank anchors; the target changed, so "
"re-derive it rather than deleting the assertion: %r" % (markup,))
self.assertIn(
"rel", attrs,
"the title anchor opens a pack-chosen page in a new tab without "
"rel, so that page keeps a reference to this window: %r" % (markup,))
tokens = attrs["rel"].lower().split()
self.assertIn("noopener", tokens, markup)
self.assertIn("noreferrer", tokens, markup)
def test_c2_href_rejects_dangerous_schemes(self):
for reference in ("javascript:alert(1)", "java\tscript:alert(1)",
"data:text/html,<script>alert(1)</script>"):
href = dict(parse(self._render_title(reference)).attrs)["href"]
self.assertEqual(href, "#", "%r survived as %r" % (reference, href))
def test_c2_benign_reference_and_title_are_untouched(self):
markup = self._render_title(BENIGN_REFERENCE, name="My Pack")
self.assertEqual(dict(parse(markup).attrs)["href"], BENIGN_REFERENCE)
self.assertIn("<b>My Pack</b>", markup)
def test_c2_title_is_not_double_escaped_at_the_formatter(self):
# The column value is already escaped HTML text by the time it arrives,
# so re-escaping here would put entity text on screen.
block = JS.lift_formatter(ANALYZER, TITLE_FORMATTER_ANCHOR)
self.assertNotIn(
"sanitizeHTML(String(name))", block,
"title is normalised at its source; escaping it again renders '&lt;' to the user",
)
server_value = "Nodes for &lt;Flux&gt; workflows"
markup = self._render_title("https://example.com/x", name=server_value)
self.assertIn(server_value, markup)
self.assertEqual(html.unescape(server_value), "Nodes for <Flux> workflows")
def _run_panel(self, action, packs=None, status=500):
"""Run the production data, operation and rendering methods with stubbed I/O."""
methods = ",\n".join(slice_braced(JS.text(ANALYZER), "\n\t" + marker) for marker in [
"async loadData()", "async installModels(list, btn)", "async uninstallModels(list, btn)",
"showUsageDetails(rowItem)", "showError(err)", "showMessage(msg, color)", "showStatus(msg, color)",
])
return run_node("""
%s
%s
%s
const elements = {};
const messages = {};
const posts = [];
const manager_instance = {datasrc_combo: {value: 'default'}};
const fetchData = async () => ({data: {channel: 'default', node_packs: %s}});
const analyzeWorkflowUsage = async () => ({success: true});
const api = {fetchApi: async (url, options) => {
if (options?.body) posts.push(JSON.parse(options.body));
return {status: %d, json: async () => ({is_processing: false}), text: async () => %s};
}};
const customConfirm = async () => true;
const md5 = () => 'hash';
const show_message = message => messages.dialog = message;
const btn = {classList: {add() {}, remove() {}}};
const ctx = {
%s,
element: {querySelector: selector => elements[selector] ||= {innerHTML: '', style: {}}},
grid: {scrollRowIntoView() {}, updateCell() {}, updateRow() {}},
flyover: {show: (title, body) => Object.assign(messages, {title, body})},
showLoading() {}, hideLoading() {}, renderGrid() {}, getModelList(rows) {return rows;}
};
ctx.ui = createUIStateManager(ctx.element, {
status: '.nu-manager-status', message: '.nu-manager-message'
});
%s
console.log(JSON.stringify({elements, messages, posts, rows: ctx.modelList}));
""" % (_preamble(),
JS.lift_declaration(COMMON, "export function createUIStateManager(element, selectors)"),
JS.lift_declaration(COMMON, "export async function uninstallNodes(nodeList, options = {})"),
json.dumps(packs or {}), status, json.dumps(TEXT_PAYLOAD), methods, action))
def test_title_fallback_is_escaped_at_its_source(self):
out = self._run_panel("await ctx.loadData();", {TEXT_PAYLOAD: {"state": "enabled"}})
row = out["rows"][0]
self._assert_inert(row["title"], allowed_tags=set())
self.assertEqual(html.unescape(row["title"]), TEXT_PAYLOAD)
self.assertEqual(row["name"], TEXT_PAYLOAD)
def test_install_progress_and_errors_escape_raw_values(self):
for status in (403, 500):
with self.subTest(status=status):
out = self._run_panel("await ctx.loadData(); await ctx.installModels(ctx.modelList, btn);",
{TEXT_PAYLOAD: {"state": "enabled"}}, status)
progress = out["elements"][".nu-manager-status"]["innerHTML"]
self._assert_inert(progress, allowed_tags=set())
self.assertEqual(html.unescape(progress), f"Install {TEXT_PAYLOAD} ...")
errors = [out["elements"][".nu-manager-message"]["innerHTML"], out["messages"]["dialog"]]
for error in errors:
self._assert_inert(error, allowed_tags={"font"})
self.assertIn(f"'{TEXT_PAYLOAD}': ", html.unescape(error))
if status == 500:
self.assertIn(": " + TEXT_PAYLOAD, html.unescape(error))
def test_flyover_escapes_workflow_filename(self):
for filename in (TEXT_PAYLOAD, "workflow <Flux> & friends.json"):
with self.subTest(filename=filename):
out = self._run_panel("ctx.showUsageDetails(%s);" % json.dumps({
"title": "My Pack", "workflowDetails": [{"filename": filename, "nodeCount": 2}],
}))
body = out["messages"]["body"]
self._assert_inert(body, allowed_tags={"div"})
self.assertIn(filename, html.unescape(body))
self.assertIn("2 nodes", body)
def test_uninstall_failure_keeps_normalized_titles_readable(self):
for name in ("Pack <Flux> & friends", TEXT_PAYLOAD):
for has_title in (False, True):
for status in (403, 500):
with self.subTest(name=name, has_title=has_title, status=status):
pack = {"state": "enabled"}
if has_title:
pack["title"] = html.escape(name)
out = self._run_panel("await ctx.loadData(); await ctx.uninstallModels(ctx.modelList, btn);",
{name: pack}, status)
progress = out["elements"][".nu-manager-status"]["innerHTML"]
self.assertEqual(html.unescape(progress), f"Uninstall {name} ...")
errors = [out["elements"][".nu-manager-message"]["innerHTML"], out["messages"]["dialog"]]
for error in errors:
self._assert_inert(error, allowed_tags={"font"})
self.assertIn(f"'{name}': ", html.unescape(error))
if status == 500:
self.assertIn(": " + TEXT_PAYLOAD, html.unescape(error))
def test_uninstall_helper_escapes_a_raw_name_fallback(self):
out = self._run_panel("""
await uninstallNodes([{name: %s}], {
onProgress: msg => messages.progress = msg,
onError: msg => messages.error = msg
});
""" % json.dumps(TEXT_PAYLOAD))
for message in out["messages"].values():
self._assert_inert(message, allowed_tags=set())
self.assertIn(TEXT_PAYLOAD, html.unescape(message))
self.assertEqual(out["posts"][0]["name"], TEXT_PAYLOAD)
def test_grid_has_no_keyword_highlight_path(self):
# The premise the row states: this grid cannot hit the highlight defect.
source = JS.text(ANALYZER)
self.assertNotIn("highlightKeywordsFilter", source)
self.assertNotIn("rowFilter:", source)
if __name__ == "__main__":
unittest.main()
tests/test_notice_dependencies.py
+83
View File
@@ -0,0 +1,83 @@
"""Exercise notice dependency installation without starting ComfyUI or pip."""
import ast
import builtins
import importlib.metadata
import itertools
import os
import re
import subprocess
import sys
import types
import unittest
from pathlib import Path
from unittest.mock import Mock, mock_open, patch
import toml
from packaging.requirements import Requirement
from manager_test_utils import load_manager_util
ROOT = Path(__file__).resolve().parent.parent
class NoticeDependencyTest(unittest.TestCase):
def test_util_can_load_before_nh3_is_installed(self):
with patch.dict(sys.modules, {'nh3': None}):
util = load_manager_util()
self.assertEqual(util.escape_html_attribute('<text>'), '&lt;text&gt;')
with self.assertRaises(ModuleNotFoundError):
util.sanitize_html_fragment('<p>notice</p>')
def test_bootstrap_installs_missing_or_outdated_nh3_before_importing_it(self):
manifest = (ROOT / 'requirements.txt').read_text()
commented_manifest = '\n# Dependencies\n--index-url https://pypi.org/simple\n' + manifest.replace(
'nh3>=0.3.7', ' nh3>=0.3.7 # notice sanitizer',
)
for installed_version, requirements in itertools.product(
(None, '0.3.0', '0.3.7rc1', '0.3.7', '0.4.0'), (manifest, commented_manifest),
):
with self.subTest(installed_version=installed_version, commented=requirements == commented_manifest):
source = (ROOT / 'prestartup_script.py').read_text()
node = next(n for n in ast.parse(source).body
if isinstance(n, ast.FunctionDef) and n.name == 'ensure_dependencies')
installer = Mock()
namespace = {
'__file__': str(ROOT / 'prestartup_script.py'),
'os': os,
're': re,
'subprocess': types.SimpleNamespace(
check_output=installer, CalledProcessError=subprocess.CalledProcessError,
),
'manager_util': types.SimpleNamespace(make_pip_cmd=lambda args: args),
}
exec(compile(ast.Module(body=[node], type_ignores=[]), '<bootstrap>', 'exec'), namespace)
needs_install = installed_version in (None, '0.3.0', '0.3.7rc1')
modules = {name: types.ModuleType(name) for name in ('git', 'toml', 'rich', 'chardet')}
# An old native module must not be imported before pip upgrades it.
modules['nh3'] = None if needs_install else types.ModuleType('nh3')
failure = importlib.metadata.PackageNotFoundError('nh3') if installed_version is None else None
with patch.dict(sys.modules, modules), patch(
'importlib.metadata.version', return_value=installed_version, side_effect=failure,
), patch('builtins.open', mock_open(read_data=requirements)), patch('builtins.print'), patch(
'builtins.__import__', wraps=builtins.__import__,
) as imports:
namespace['ensure_dependencies']()
nh3_imports = [call for call in imports.call_args_list if call.args[0] == 'nh3']
self.assertEqual(len(nh3_imports), 0 if needs_install else 1)
if needs_install:
installer.assert_called_once_with(['install', '-r', str(ROOT / 'requirements.txt')])
else:
installer.assert_not_called()
def test_dependency_manifests_agree_on_notice_requirements(self):
lines = (re.split(r'\s+#', line.strip(), maxsplit=1)[0]
for line in (ROOT / 'requirements.txt').read_text().splitlines())
pip_requirements = {
req.name: req for req in map(Requirement, (line for line in lines if line and not line.startswith(('#', '-'))))
}
project_requirements = {
req.name: req for req in map(Requirement, toml.load(ROOT / 'pyproject.toml')['project']['dependencies'])
}
for name in ('nh3', 'packaging'):
with self.subTest(name=name):
self.assertEqual(str(pip_requirements[name]), str(project_requirements[name]))
tests/test_pack_column_escaping.py
+372
View File
@@ -0,0 +1,372 @@
"""RED->GREEN guard for the pack-grid column sinks in js/custom-nodes-manager.js.
WHY THESE THREE VALUES
----------------------
The Custom Nodes grid renders every cell through turbogrid, which hands a column
formatter the RAW row value and assigns the formatter's return through
``innerHTML``. Whatever a formatter returns unescaped is therefore live markup on
FIRST RENDER, before any click, for every row the list contains.
Three values reached that sink unescaped, and all three carry the same provenance
as ``author`` (which the neighbouring formatter in the same column array already
escapes):
``stars`` and ``last_update`` are written by ``populate_github_stats`` in
glob/manager_core.py straight off github-stats.json, with no transform.
``populate_markdown`` -- the only server-side escape on this payload -- covers
``description``, ``name`` and ``title`` and NOTHING else, and
``populate_github_stats`` swallows a broken row with a bare ``except:``, so a
channel-supplied ``stars`` / ``last_update`` survives untouched when
``v['reference']`` raises. That double fact is pinned in
``PopulateProvenanceTest`` below, because it is what makes escaping HERE
correct rather than a double-escape.
``channel`` is the resolved channel alias from /customnode/getlist -- a key out
of the channel-list config file, passed through untransformed into the
"Channel: ..." banner's ``innerHTML``.
WHY THE OLD GUARDS DID NOT STOP THEM
------------------------------------
Neither ``stars`` guard was a guard. ``stars < 0`` is a NaN comparison for a
string ('<img ...>' < 0 is false), and ``typeof stars === 'number'`` only chooses
the toLocaleString branch -- so a string payload fell through to the bare
``return stars;``. ``last_update`` looked defended by ``.split(' ')[0]``, but a
space is not the only attribute separator a browser accepts: ``/`` works, and so
do tab and newline, so ``<img/src=x/onerror=alert(1)>`` passes through the split
whole. Escaping is what closes both; a smarter split would not.
REPRODUCE THE RED HALF against the pre-fix revision -- one command:
mkdir -p /some/dir
git show <base>:js/custom-nodes-manager.js > /some/dir/custom-nodes-manager.js
git show <base>:js/common.js > /some/dir/common.js
MANAGER_JS_DIR=/some/dir pytest tests/test_pack_column_escaping.py
"""
import html
import json
import os
import unittest
from pathlib import Path
from js_lift import (
NODE,
JsSource,
event_handlers,
line_containing,
parse,
slice_object_entry,
slice_braced,
)
from js_lift import run_node
from manager_test_utils import load_manager_util, load_markdown_functions
REPO_ROOT = Path(__file__).resolve().parent.parent
JS = JsSource(os.environ.get("MANAGER_JS_DIR") or (REPO_ROOT / "js"))
GLOB_DIR = Path(os.environ.get("MANAGER_GLOB_DIR") or (REPO_ROOT / "glob"))
MANAGER = "custom-nodes-manager.js"
COMMON = "common.js"
STARS_ANCHOR = "id: 'stars',"
LAST_UPDATE_ANCHOR = "id: 'last_update',"
CHANNEL_ANCHOR = '.cn-manager-channel").innerHTML'
# A space-separated payload for the columns that never split, and a
# SLASH-separated one for last_update, whose `.split(' ')[0]` used to read as a
# defence. Neither needs `<` to be dangerous in an attribute, but both are
# element injections here because the sink is element text.
TEXT_PAYLOAD = "<img src=x onerror=alert(1)>"
NO_SPACE_PAYLOAD = "<img/src=x/onerror=alert(1)>"
TAB_PAYLOAD = "<img\tsrc=x\tonerror=alert(1)>"
def _lift_get_time_ago() -> str:
"""getTimeAgo, lifted by SPAN rather than by brace matching.
js_lift's brace scanner reads a lone ``/`` as a regex literal -- correct for
every range it was written for -- and getTimeAgo DIVIDES
(``Math.abs(diff) / divisor``), so ``slice_braced`` raises on it by design
rather than handing back a wrong slice. The span is bounded by the
function's own last statement instead, so a change to that statement fails
loudly here rather than lifting a truncated body.
"""
span = JS.lift_span(COMMON, "export function getTimeAgo(", "'in a year', diff < 0);")
return span.replace("export ", "", 1) + "\n}"
def _preamble() -> str:
"""sanitizeHTML and getTimeAgo, lifted from the shipped common.js."""
return "\n".join([
JS.lift_declaration(COMMON, "export function sanitizeHTML("),
_lift_get_time_ago(),
])
def _render(formatter_source: str, value) -> str:
return run_node(
"%s\nconst formatter = %s;\n"
"console.log(JSON.stringify({markup: String(formatter(%s))}));\n"
% (_preamble(), formatter_source, json.dumps(value))
)["markup"]
class _InertAssertions(unittest.TestCase):
def assert_inert(self, markup: str, allowed_tags):
parsed = parse(markup)
self.assertEqual(
[t for t in parsed.tags if t not in allowed_tags], [],
"live element injected into: %r" % (markup,),
)
self.assertEqual(
event_handlers(markup), [],
"event-handler attribute injected into: %r" % (markup,),
)
@unittest.skipIf(NODE is None, "node is required to execute the lifted production JS")
class StarsColumnTest(_InertAssertions):
"""The ★ column formatter, lifted and executed as shipped."""
@classmethod
def setUpClass(cls):
cls.formatter = JS.lift_formatter(MANAGER, STARS_ANCHOR)
def test_a_string_payload_renders_inert(self):
for payload in (TEXT_PAYLOAD, NO_SPACE_PAYLOAD, TAB_PAYLOAD):
with self.subTest(payload=payload):
markup = self._markup(payload)
self.assert_inert(markup, allowed_tags=set())
self.assertEqual(
html.unescape(markup), payload,
"the payload must survive as READABLE TEXT, not be dropped",
)
def _markup(self, value):
return _render(self.formatter, value)
def test_a_number_still_renders_through_toLocaleString(self):
"""The behaviour the fix must not disturb: a real star count is grouped.
Compared against node's OWN toLocaleString rather than a hard-coded
"1,234", so the assertion does not encode the runtime's default locale.
"""
expected = run_node(
"console.log(JSON.stringify({v: (1234567).toLocaleString()}));"
)["v"]
markup = self._markup(1234567)
self.assertEqual(markup, expected)
self.assertNotIn("&", markup, "a number must not come back entity-escaped")
def test_a_negative_count_is_still_na(self):
# -1 is populate_github_stats' "no stats for this pack" sentinel.
self.assertEqual(self._markup(-1), "N/A")
def test_zero_is_not_treated_as_missing(self):
self.assertEqual(self._markup(0), "0")
def test_a_missing_value_still_renders_an_empty_cell(self):
"""null / undefined pass through, as at every other escaped column.
turbogrid's renderNodeContent does `void 0===e&&(e="")` and then assigns
`innerHTML`, which is [LegacyNullToEmptyString] -- so a formatter that
passes null/undefined THROUGH yields an empty cell, while one that
stringifies them first puts the literal words "undefined" / "null" on
screen. That is the shape spelled out at the id formatter in this same
file and at model-manager's escapeCell, both of which say so in a comment.
It is reachable on exactly the malformed-row path this file is about:
populate_github_stats swallows a broken entry with a bare `except:`, so
`stars` is simply ABSENT for a pack whose `reference` lookup raises.
Asserted through renderNodeContent's own semantics rather than on the
formatter's return alone, because "returns undefined" and "renders an
empty cell" are different claims and it is the second one the user sees.
"""
out = run_node(
"%s\nconst formatter = %s;\n"
"const cell = (v) => { let e = formatter(v); if (void 0 === e) e = '';\n"
" return e === null ? '' : String(e); };\n"
"console.log(JSON.stringify({\n"
" undefined_passthrough: formatter(undefined) === undefined,\n"
" null_passthrough: formatter(null) === null,\n"
" undefined_cell: cell(undefined), null_cell: cell(null)}));\n"
% (_preamble(), self.formatter)
)
self.assertTrue(
out["undefined_passthrough"],
"a missing star count is stringified instead of passed through, so "
"the cell reads 'undefined' where it used to be empty",
)
self.assertTrue(out["null_passthrough"], "null is stringified, not passed through")
self.assertEqual(out["undefined_cell"], "")
self.assertEqual(out["null_cell"], "")
@unittest.skipIf(NODE is None, "node is required to execute the lifted production JS")
class LastUpdateColumnTest(_InertAssertions):
"""The Last Update column formatter, lifted and executed as shipped."""
@classmethod
def setUpClass(cls):
cls.formatter = JS.lift_formatter(MANAGER, LAST_UPDATE_ANCHOR)
def _markup(self, value):
return _render(self.formatter, value)
def test_a_slash_separated_payload_renders_inert(self):
"""The case `.split(' ')[0]` cannot touch: no space anywhere in it."""
markup = self._markup(NO_SPACE_PAYLOAD)
self.assert_inert(markup, allowed_tags={"span"})
def test_separator_variants_render_inert(self):
for payload in (TEXT_PAYLOAD, NO_SPACE_PAYLOAD, TAB_PAYLOAD):
with self.subTest(payload=payload):
self.assert_inert(self._markup(payload), allowed_tags={"span"})
def test_the_title_attribute_cannot_be_broken_out_of(self):
# `ago` is not escaped, deliberately: getTimeAgo returns "" or a phrase
# off a fixed table and never echoes its input. This pins that premise
# behaviourally rather than trusting it.
markup = self._markup('x" onmouseover=alert(1) y')
self.assertEqual(event_handlers(markup), [], markup)
self.assertEqual(
sorted(name for name, _ in parse(markup).attrs), ["title"], markup,
)
def test_a_normal_date_still_renders_its_short_form(self):
markup = self._markup("2024-01-02 03:04:05")
self.assertIn(">2024-01-02<", markup)
self.assertEqual([t for t in parse(markup).tags], ["span"])
def test_a_negative_timestamp_is_still_na(self):
self.assertEqual(self._markup(-1), "N/A")
@unittest.skipIf(NODE is None, "node is required to execute the lifted production JS")
class ChannelBannerTest(_InertAssertions):
"""The "Channel: ..." banner, rendered through the shipped source line."""
@classmethod
def setUpClass(cls):
cls.line = line_containing(JS.text(MANAGER), CHANNEL_ANCHOR)
def _markup(self, channel):
expression = self.line.split("innerHTML =", 1)[1].strip().rstrip(";")
return run_node(
"%s\nconst self = {channel: %s};\n"
"console.log(JSON.stringify({markup: %s}));\n"
% (_preamble(), json.dumps(channel),
expression.replace("this.channel", "self.channel"))
)["markup"]
def test_a_payload_in_the_channel_alias_renders_inert(self):
markup = self._markup(TEXT_PAYLOAD)
self.assert_inert(markup, allowed_tags=set())
self.assertIn("Channel: ", markup)
def test_a_benign_alias_is_still_readable(self):
# No server transform touches this value, so escaping it once is
# correct and cannot show entity text for an ordinary alias.
self.assertIn("Channel: dev (Incomplete list)", self._markup("dev"))
class ColumnShapeTest(unittest.TestCase):
"""The formatters must keep EXISTING, or the grid's default takes over.
turbogrid's default formatter is a pass-through, so deleting either
formatter silently restores the raw-value-into-innerHTML path these tests
exist to close -- with every behavioural test above still passing, because
they lift the formatter and would simply stop being able to find one.
"""
def test_both_columns_declare_a_formatter(self):
source = JS.text(MANAGER)
for anchor in (STARS_ANCHOR, LAST_UPDATE_ANCHOR):
with self.subTest(column=anchor):
entry = slice_object_entry(source, anchor)
self.assertIn(
"formatter:", entry,
"%s lost its formatter; the grid default renders the raw "
"value into innerHTML" % (anchor,),
)
self.assertIn("sanitizeHTML(", entry)
class PopulateProvenanceTest(unittest.TestCase):
"""WHY escaping here is single-escaping, not double-escaping.
If the server ever started escaping these two fields, the formatters would
render entity text to the user and this file would be the thing to change.
The premise is therefore asserted rather than assumed.
"""
def test_populate_markdown_does_not_cover_stars_or_last_update(self):
source = (GLOB_DIR / "manager_server.py").read_text(encoding="utf-8")
body = source[source.index("def populate_markdown("):]
body = body[:body.index("\ndef ", 1)] if "\ndef " in body[1:] else body
for field in ("'description'", "'name'", "'title'"):
self.assertIn("if %s in x:" % field, body)
for field in ("stars", "last_update"):
self.assertNotIn(
"'%s'" % field, body,
"populate_markdown now touches %s -- if it escapes it, the "
"formatter must STOP escaping or the user reads entity text"
% (field,),
)
def test_the_stats_fields_are_written_untransformed(self):
source = (GLOB_DIR / "manager_core.py").read_text(encoding="utf-8")
body = source[source.index("def populate_github_stats("):]
body = body[:body.index("\ndef ", 1)]
self.assertIn("v['stars'] = json_obj_github[url]['stars']", body)
self.assertIn("v['last_update'] = json_obj_github[url]['last_update']", body)
self.assertIn(
"except:", body,
"the bare except is what leaves a channel-supplied stars/last_update "
"in place when reference lookup raises; if it is gone, re-derive the "
"provenance argument in this file's docstring",
)
@unittest.skipIf(NODE is None, "node is required to execute the lifted production JS")
class PackOperationErrorTest(_InertAssertions):
def test_failed_operations_keep_server_escaped_titles_readable(self):
populate = load_markdown_functions(load_manager_util())["populate_markdown"]
methods = ",\n".join(slice_braced(JS.text(MANAGER), "\n\t" + marker) for marker in [
"async installNodes(list, btn, title, selected_version)",
"showError(err)", "showMessage(msg, color)", "showStatus(msg, color)",
])
for title in ("Pack <Flux> & friends", TEXT_PAYLOAD):
item = {"title": title, "originalData": {}, "hash": "pack"}
populate(item)
for mode in ("install", "uninstall"):
for status in (403, 500):
with self.subTest(title=title, mode=mode, status=status):
out = run_node("""
%s
const item = %s;
const elements = {};
const messages = {};
const api = {fetchApi: async () => ({status: %d,
json: async () => ({is_processing: false}), text: async () => %s})};
const show_message = message => messages.dialog = message;
const customConfirm = async () => true;
const ctx = {
%s,
element: {querySelector: selector => elements[selector] ||= {innerHTML: ''}},
grid: {getRowItemBy: () => item, scrollRowIntoView() {}, updateCell() {}},
focusInstall() {return true;}
};
await ctx.installNodes(['pack'], {target: {classList: {add() {}}},
label: 'Operation', mode: %s}, '', null);
messages.panel = elements['.cn-manager-message'].innerHTML;
console.log(JSON.stringify(messages));
""" % (_preamble(), json.dumps(item), status, json.dumps(TEXT_PAYLOAD), methods, json.dumps(mode)))
for message in out.values():
self.assert_inert(message, allowed_tags={"font"})
self.assertIn(f"'{title}': ", html.unescape(message))
if status == 500:
self.assertIn(": " + TEXT_PAYLOAD, html.unescape(message))
if __name__ == "__main__":
unittest.main()
tests/test_residual_sink_escaping.py
+590
View File
@@ -0,0 +1,590 @@
"""Check remote notice HTML, sharing URLs, snapshot names and node selection UI.
Execute shipped Python and JavaScript with isolated external boundaries. Rendering
checks cover hostile inputs and readable controls; link checks preserve opener isolation.
"""
import ast
import os
import re
import unittest
from html import unescape
from html.parser import HTMLParser
from pathlib import Path
from js_lift import NODE, JsSource, run_node
from manager_test_utils import GLOB_DIR, load_manager_util, load_markdown_functions
REPO_ROOT = Path(__file__).resolve().parent.parent
JS = JsSource(os.environ.get("MANAGER_JS_DIR") or (REPO_ROOT / "js"))
COMMON = "common.js"
SHARE_COMMON = "comfyui-share-common.js"
SHARE_OPENART = "comfyui-share-openart.js"
SHARE_COPUS = "comfyui-share-copus.js"
SHARE_YOUML = "comfyui-share-youml.js"
SNAPSHOT = "snapshot.js"
MANAGER = "custom-nodes-manager.js"
# A scheme that executes on click, and a scheme that carries its own document.
# Both are strings a third party can put in a URL field; neither is a host.
SCHEME_PAYLOAD = "javascript:window.__probe=1"
DATA_PAYLOAD = "data:text/html,<b>x</b>"
BENIGN_URL = "https://example.com/w/123?a=1&b=2"
# An http URL that ALSO closes the attribute it lands in. A service-chosen URL
# is not "either a bad scheme or a quote" -- it is one string that can be both,
# so a scheme allow-list alone leaves the quote. One variant per quoting style,
# because the call sites differ.
BREAKOUT_SINGLE_QUOTED = "https://example.com/x' onmouseover='window.__probe=1"
BREAKOUT_DOUBLE_QUOTED = 'https://example.com/x" onmouseover="window.__probe=1'
# Element text, for the sinks that render a name rather than a link.
TEXT_PAYLOAD = '<img src=x onerror="window.__probe=1">'
def _function_source(name: str, filename: str = "manager_server.py") -> str:
"""The source text of one top-level function, WITHOUT importing its module.
Used for the wiring legs, where the question is "does this call site pass
its value through the normaliser" -- a question about the call site, which a
behavioural test on the normaliser alone cannot answer.
"""
source = (GLOB_DIR / filename).read_text(encoding="utf-8")
tree = ast.parse(source)
wanted = [
node for node in tree.body
if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)) and node.name == name
]
assert len(wanted) == 1, "expected exactly one %s, got %d" % (name, len(wanted))
return ast.get_source_segment(source, wanted[0]) or ""
class _Collector(HTMLParser):
"""What a browser would actually build from a fragment."""
def __init__(self):
super().__init__()
self.elements = []
def handle_starttag(self, tag, attrs):
self.elements.append((tag, dict(attrs)))
handle_startendtag = handle_starttag
def parse(markup: str) -> _Collector:
collector = _Collector()
collector.feed(markup)
collector.close()
return collector
def tags(markup: str):
return [tag for tag, _ in parse(markup).elements]
def handler_attrs(markup: str):
return [
"%s@%s" % (tag, name)
for tag, attrs in parse(markup).elements
for name in attrs
if name.lower().startswith("on")
]
def hrefs(markup: str):
return [
attrs["href"] for tag, attrs in parse(markup).elements
if tag == "a" and "href" in attrs
]
# ---------------------------------------------------------------------------
# [D1] the relayed wiki fragment
# ---------------------------------------------------------------------------
class NoticeFragmentSanitizerTest(unittest.TestCase):
"""Notice sanitization must remove executable content while preserving formatting."""
@classmethod
def setUpClass(cls):
cls.manager_util = load_manager_util()
def _sanitize(self, fragment: str) -> str:
fn = getattr(self.manager_util, "sanitize_html_fragment", None)
self.assertIsNotNone(
fn,
"glob/manager_util.py exposes no sanitize_html_fragment(fragment). "
"The notice fragment is remote HTML the server relays verbatim and "
"the client assigns to innerHTML by design, so the server is the "
"only boundary that can constrain it.",
)
return fn(fragment)
def test_a_script_element_does_not_survive(self):
out = self._sanitize('<p>before</p><script>window.__probe=1</script><p>after</p>')
self.assertNotIn("script", tags(out), out)
self.assertIn("p", tags(out), "ordinary markup was destroyed along with it: %r" % (out,))
def test_a_handler_attribute_does_not_survive(self):
out = self._sanitize('<img src="https://example.com/i.png" onerror="window.__probe=1">')
self.assertEqual(handler_attrs(out), [], out)
def test_a_handler_attribute_does_not_survive_on_any_element(self):
# The allow-list is over ATTRIBUTES, not over one element that happened
# to be tested: a deny-list keyed to <img> leaves <div onmouseover=...>.
for markup in (
'<div onmouseover="window.__probe=1">hover</div>',
'<a href="https://example.com/" onclick="window.__probe=1">click</a>',
'<b ONERROR="window.__probe=1">bold</b>',
):
with self.subTest(markup=markup):
self.assertEqual(handler_attrs(self._sanitize(markup)), [], markup)
def test_an_executing_scheme_does_not_survive_in_an_href(self):
for payload in (SCHEME_PAYLOAD, DATA_PAYLOAD):
with self.subTest(payload=payload):
out = self._sanitize('<a href="%s">click</a>' % (payload,))
self.assertEqual(
[h for h in hrefs(out) if h.startswith(("javascript:", "data:"))], [],
"the scheme survived into an href: %r" % (out,),
)
def test_ordinary_formatting_still_displays(self):
"""The half a strip-everything sanitizer would fail.
The notice is formatted prose. If the fix returns text, the feature is
gone and no other test in this file would say so.
"""
fragment = (
'<p><b>bold</b> and <i>italic</i></p>'
'<ul><li>one</li><li>two</li></ul>'
'<a href="https://example.com/x">link</a>'
'<img src="https://example.com/i.png">'
)
out = self._sanitize(fragment)
for tag in ("p", "b", "i", "ul", "li", "a", "img"):
with self.subTest(tag=tag):
self.assertIn(tag, tags(out), "%r was dropped: %r" % (tag, out))
self.assertIn("https://example.com/x", hrefs(out), out)
self.assertIn("one", out)
self.assertIn("two", out)
def test_an_unclosed_element_does_not_blank_the_rest_of_the_notice(self):
"""Malformed elements must not discard unrelated notice content."""
for wrapper in ("div", "blockquote", "section", "article", "figure",
"main", "header", "nav", "aside"):
with self.subTest(wrapper=wrapper):
out = self._sanitize(
"<%s><svg></%s><p>after</p>" % (wrapper, wrapper))
self.assertIn(
"after", out,
"an unclosed element inside <%s> swallowed the rest of the "
"notice: %r" % (wrapper, out))
self.assertNotIn("svg", tags(out), out)
def test_an_unterminated_script_still_fails_closed(self):
"""HTML script/style contents must not be emitted as notice markup."""
for fragment in ("<section><script></section><p>after</p>",
"<div><style></div><p>after</p>"):
with self.subTest(fragment=fragment):
out = self._sanitize(fragment)
# NOT `out == ""`: a wrapper's OPEN tag is emitted before the
# skip window starts, so `<div><style>...` legitimately keeps
# its `<div>`. The property is that nothing AFTER the
# unterminated element comes back -- as markup or as text.
self.assertNotIn(
"after", out,
"text after an unterminated script/style came back: %r"
% (out,))
self.assertEqual(
[t for t in tags(out) if t != "div"], [],
"markup after an unterminated script/style came back: %r"
% (out,))
def test_foreign_content_does_not_hide_safe_following_prose(self):
# HTML5 parsing closes the SVG context before this paragraph. The old
# HTMLParser-based sanitizer treated the rest as script text and lost it.
out = self._sanitize('<svg><script></svg><p>after</p>')
self.assertEqual(tags(out), ['p'])
self.assertIn('after', out)
def test_notice_formatting_attributes_survive(self):
out = self._sanitize(
'<p class="notice" title="Release" align="center">'
'<font color="red">Update</font></p>'
'<table width="400"><tr><td colspan="2" rowspan="3">Data</td></tr></table>'
'<img src="https://example.com/i.png" alt="Preview" width="100" height="80">'
)
elements = dict(parse(out).elements)
self.assertEqual(elements['p'], {'class': 'notice', 'title': 'Release', 'align': 'center'})
self.assertEqual(elements['font'], {'color': 'red'})
self.assertEqual(elements['table']['width'], '400')
self.assertEqual(elements['td'], {'colspan': '2', 'rowspan': '3'})
self.assertEqual(elements['img'], {
'src': 'https://example.com/i.png', 'alt': 'Preview', 'width': '100', 'height': '80',
})
def test_notice_links_open_safely_regardless_of_input_attribute_order(self):
for attrs in (
'href="https://example.com/?a=1&amp;b=2" target="named" rel="opener"',
'title="Docs" rel="opener" target="named" href="https://example.com/?a=1&amp;b=2"',
):
with self.subTest(attrs=attrs):
out = self._sanitize(f'<a {attrs}>Docs</a>')
anchor = dict(parse(out).elements)['a']
self.assertEqual(anchor['href'], 'https://example.com/?a=1&b=2')
self.assertEqual(anchor['target'], '_blank')
self.assertEqual(set(anchor['rel'].split()), {'noopener', 'noreferrer'})
def test_notice_url_schemes_remain_limited_to_http_https_and_relative(self):
for url in ('ftp://example.com', 'mailto:a@example.com', 'java&#9;script:alert`1`'):
with self.subTest(url=url):
out = self._sanitize(f'<a href="{url}">Link</a><img src="{url}">')
for _, attrs in parse(out).elements:
self.assertNotIn('href', attrs)
self.assertNotIn('src', attrs)
for url in ('http://example.com', 'https://example.com', '/relative', '#section'):
with self.subTest(url=url):
self.assertEqual(hrefs(self._sanitize(f'<a href="{url}">Link</a>')), [url])
def test_hidden_element_contents_are_not_exposed_as_notice_text(self):
for tag in ('script', 'style', 'iframe', 'object', 'svg', 'math',
'template', 'noscript', 'textarea', 'title'):
with self.subTest(tag=tag):
out = self._sanitize(f'<{tag}>hidden</{tag}><p>after</p>')
self.assertNotIn('hidden', out)
self.assertIn('after', out)
def test_a_mixed_fragment_keeps_the_good_and_drops_the_bad(self):
"""One fragment carrying both, since a real wiki page would."""
out = self._sanitize(
'<p>Release <b>1.2</b></p>'
'<script>window.__probe=1</script>'
'<a href="https://example.com/notes">notes</a>'
'<a href="%s">click</a>'
'<img src="https://example.com/i.png" onerror="window.__probe=1">'
% (SCHEME_PAYLOAD,)
)
self.assertNotIn("script", tags(out), out)
self.assertEqual(handler_attrs(out), [], out)
self.assertEqual(
[h for h in hrefs(out) if h.startswith(("javascript:", "data:"))], [], out)
self.assertIn("https://example.com/notes", hrefs(out), out)
self.assertIn("b", tags(out), out)
self.assertIn("img", tags(out), "the benign image was dropped: %r" % (out,))
class NoticeWiringTest(unittest.TestCase):
"""Check the notice route applies the sanitizer to its captured remote HTML."""
def test_get_notice_passes_the_captured_fragment_through_the_sanitizer(self):
"""Verify the captured HTML is sanitized, not merely that the function is mentioned."""
body = _function_source("get_notice")
self.assertIn(
"match.group(1)", body,
"get_notice no longer captures the fragment the way this guard "
"expects; re-derive it against the new shape rather than deleting "
"the assertion.",
)
self.assertRegex(
body, r"sanitize_html_fragment\s*\(\s*match\.group\(1\)\s*\)",
"get_notice relays the captured wiki fragment without passing it "
"through an allow-list normaliser. The client assigns the response "
"body to innerHTML, so whatever the wiki page contains is what the "
"browser builds. The call has to take the capture as its argument; "
"naming the function elsewhere in the body is not wiring.",
)
self.assertNotRegex(
body, re.compile(r"^\s*markdown_content\s*=\s*match\.group\(1\)\s*$",
re.MULTILINE),
"the raw capture is still assigned directly; the sanitizer must sit "
"on this assignment, not beside it.",
)
# ---------------------------------------------------------------------------
# [D2] share-service URLs
# ---------------------------------------------------------------------------
@unittest.skipIf(NODE is None, "node is required to execute the lifted production JS")
class ShareDialogUrlTest(unittest.TestCase):
"""Share-service URLs must use safe schemes and quoted attributes."""
def _preamble(self) -> str:
span = JS.lift_span(
COMMON, "export const SAFE_URL_SCHEMES",
"export const safeHref = (url) => sanitizeHTML(sanitizeUrl(url));")
return "\n".join([
JS.lift_declaration(COMMON, "export function sanitizeHTML("),
span.replace("export ", ""),
])
def _render(self, expression: str, bindings: str) -> str:
return run_node(
"%s\n%s\nconsole.log(JSON.stringify({markup: %s}));\n"
% (self._preamble(), bindings, expression)
)["markup"]
def _assert_scheme_rejected(self, markup: str, label: str):
bad = [h for h in hrefs(markup) if h.startswith(("javascript:", "data:"))]
self.assertEqual(
bad, [],
"%s emits a service-chosen scheme as a clickable href: %r" % (label, markup),
)
self.assertEqual(handler_attrs(markup), [], markup)
# --- comfyui-share-common.js, the fully service-controlled URL ----------
def _common_expression(self) -> str:
from js_lift import line_containing
line = line_containing(JS.text(SHARE_COMMON),
'this.final_message.innerHTML = "Your art has been shared:')
return line.split("innerHTML =", 1)[1].strip().rstrip(";")
def test_share_common_rejects_an_executing_scheme(self):
# The href here is SINGLE-quoted, so the breakout variant uses `'`.
for payload in (SCHEME_PAYLOAD, DATA_PAYLOAD, BREAKOUT_SINGLE_QUOTED):
with self.subTest(payload=payload):
markup = self._render(
self._common_expression(),
"const response_json = {comfyworkflows: {url: %s}};" % (_json(payload),))
self._assert_scheme_rejected(markup, "comfyui-share-common.js")
def test_share_common_still_links_an_ordinary_url(self):
"""The leg a refuse-everything fix would fail."""
markup = self._render(
self._common_expression(),
"const response_json = {comfyworkflows: {url: %s}};" % (_json(BENIGN_URL),))
self.assertIn(
BENIGN_URL, hrefs(markup),
"an ordinary https URL must still become a working link: %r" % (markup,))
def test_every_share_anchor_severs_the_opener(self):
"""Check opener isolation without imposing an order on rel tokens."""
cases = [
("comfyui-share-common.js", self._common_expression(),
"const response_json = {comfyworkflows: {url: %s}};" % (_json(BENIGN_URL),)),
("comfyui-share-youml.js", self._youml_expression(),
"const messagePrefix = 'shared.';\nconst recipePageUrl = %s;" % (_json(BENIGN_URL),)),
("comfyui-share-openart.js", self._openart_expression(),
"const url = %s;" % (_json(BENIGN_URL),)),
("comfyui-share-copus.js", self._copus_expression(),
"const url = %s;" % (_json(BENIGN_URL),)),
]
for name, expression, bindings in cases:
with self.subTest(file=name):
markup = self._render(expression, bindings)
anchors = [attrs for tag, attrs in parse(markup).elements if tag == "a"]
self.assertTrue(anchors, "no anchor produced by %s: %r" % (name, markup))
for attrs in anchors:
self.assertEqual(
attrs.get("target"), "_blank",
"%s: this guard is about _blank anchors; the target "
"changed, so re-derive it: %r" % (name, markup))
self.assertIn(
"rel", attrs,
"%s opens a service-chosen page in a new tab without "
"rel, so that page keeps a reference to this window: %r"
% (name, markup))
rel = attrs["rel"].lower().split()
self.assertIn("noopener", rel, markup)
self.assertIn("noreferrer", rel, markup)
# --- comfyui-share-youml.js, also fully service-controlled --------------
def _youml_expression(self) -> str:
span = JS.lift_span(SHARE_YOUML, "this.message.innerHTML = `${messagePrefix}",
"visit it on YouML</a>`;")
return span.split("innerHTML =", 1)[1].strip().rstrip(";")
def test_share_youml_rejects_an_executing_scheme(self):
# The href here is DOUBLE-quoted, so the breakout variant uses `"`.
for payload in (SCHEME_PAYLOAD, DATA_PAYLOAD, BREAKOUT_DOUBLE_QUOTED):
with self.subTest(payload=payload):
markup = self._render(
self._youml_expression(),
"const messagePrefix = 'Workflow has been shared.';\n"
"const recipePageUrl = %s;" % (_json(payload),))
self._assert_scheme_rejected(markup, "comfyui-share-youml.js")
# --- openart / copus: an id is interpolated into a URL PATH -------------
# The scheme is hardcoded, so these cannot carry javascript:. What they CAN
# carry is a quote: the id is a service-returned string dropped inside a
# double-quoted attribute, so `"` closes it and the rest is markup. That is a
# different failure from the two above and is asserted as such.
def _openart_expression(self) -> str:
from js_lift import line_containing
line = line_containing(JS.text(SHARE_OPENART),
"this.message.innerHTML = `Workflow has been shared successfully.")
return line.split("innerHTML =", 1)[1].strip().rstrip(";")
def _copus_expression(self) -> str:
from js_lift import line_containing
line = line_containing(JS.text(SHARE_COPUS),
"this.message.innerHTML = `Workflow has been shared successfully.")
return line.split("innerHTML =", 1)[1].strip().rstrip(";")
def test_openart_id_cannot_break_out_of_the_href_attribute(self):
markup = self._render(
self._openart_expression(),
'const url = "https://openart.ai/workflows/-/-/" + '
'%s;' % (_json('x" onmouseover="window.__probe=1'),))
self.assertEqual(handler_attrs(markup), [], markup)
def test_copus_id_cannot_break_out_of_the_href_attribute(self):
markup = self._render(
self._copus_expression(),
'const url = "https://example.invalid/work/" + '
'%s;' % (_json('x" onmouseover="window.__probe=1'),))
self.assertEqual(handler_attrs(markup), [], markup)
# ---------------------------------------------------------------------------
# [D3] the snapshot name
# ---------------------------------------------------------------------------
@unittest.skipIf(NODE is None, "node is required to execute the lifted production JS")
class SnapshotNameTest(unittest.TestCase):
"""Snapshot filenames must remain inert text in status messages."""
def _render(self, name: str) -> str:
line = _unique_line(JS.text(SNAPSHOT), "self.updateMessage(`<BR><font color=")
expression = _call_argument(line, "self.updateMessage(")
return run_node(
"%s\nconst target = {name: %s};\n"
"console.log(JSON.stringify({markup: %s}));\n"
% (JS.lift_declaration(COMMON, "export function sanitizeHTML("),
_json(name), expression)
)["markup"]
def test_a_crafted_snapshot_name_renders_as_text(self):
markup = self._render(TEXT_PAYLOAD)
self.assertEqual(handler_attrs(markup), [], markup)
self.assertNotIn("img", tags(markup), markup)
def test_an_ordinary_snapshot_name_still_reads_normally(self):
markup = self._render("2026-09-07_backup")
self.assertIn("2026-09-07_backup", markup)
# ---------------------------------------------------------------------------
# [D4] rel=noopener, and the defensive escapes
# ---------------------------------------------------------------------------
class ServerAnchorRelTest(unittest.TestCase):
"""Converted [a/text](url) links must isolate the opener and remain usable."""
@classmethod
def setUpClass(cls):
cls.manager_util = load_manager_util()
# staticmethod, or attribute access binds it and passes `self`
# as the markdown argument.
cls.convert = staticmethod(load_markdown_functions(cls.manager_util)["convert_markdown_to_html"])
def test_a_markdown_link_carries_rel_noopener_noreferrer(self):
out = self.convert("see [a/the docs](https://example.com/docs)")
anchors = [attrs for tag, attrs in parse(out).elements if tag == "a"]
self.assertTrue(anchors, "no anchor was produced at all: %r" % (out,))
for attrs in anchors:
with self.subTest(attrs=attrs):
self.assertIn(
"rel", attrs,
"the anchor opens a new tab without rel: %r. The opened page "
"keeps a reference to this window." % (out,),
)
rel = attrs["rel"].lower().split()
self.assertIn("noopener", rel, out)
self.assertIn("noreferrer", rel, out)
def test_the_link_still_works(self):
out = self.convert("see [a/the docs](https://example.com/docs)")
self.assertIn("https://example.com/docs", hrefs(out), out)
class ClientAnchorAndStateTest(unittest.TestCase):
"""External-link isolation and rendering of unrecognised pack states."""
@classmethod
def setUpClass(cls):
cls.source = JS.text(MANAGER)
def test_the_title_link_severs_the_opener(self):
marker = "link.target = '_blank';"
index = _unique_index(self.source, marker)
# max(0, ...) because a negative start would wrap to the END of the
# string and silently hand back an unrelated window.
window = self.source[max(0, index - 400):index + 400]
self.assertRegex(
window, r"link\.rel\s*=",
"the title link opens a new tab without setting rel, so the pack's "
"own repository page keeps a reference to this window.",
)
rel = re.search(r"link\.rel\s*=\s*['\"]([^'\"]*)['\"]", window)
self.assertIsNotNone(rel, "link.rel is set from a non-literal: %r" % (window,))
tokens = rel.group(1).lower().split()
# Membership, not token order: 'noreferrer noopener' is the same thing,
# and demanding one spelling would false-RED a correct fix. Matches how
# ServerAnchorRelTest checks the server-side anchor.
self.assertIn("noopener", tokens, window)
self.assertIn("noreferrer", tokens, window)
@unittest.skipIf(NODE is None, "node is required")
def test_selection_survives_unrecognised_and_prototype_names(self):
sanitize = JS.lift_declaration(COMMON, "export function sanitizeHTML(")
buttons = JS.lift_declaration(MANAGER, "\n\tgetActionButtons(")
render = JS.lift_declaration(MANAGER, "\n\trenderSelected()")
for state in ['not-a-known-install-group', 'constructor', '__proto__', TEXT_PAYLOAD, 'enabled']:
with self.subTest(state=state):
result = run_node(sanitize + "\nconst manager = {" + buttons + "," + render + """,
grid: {hasMask: false, getSelectedRows: () => [{action: STATE, hash: 'fixture'}]},
getFilterItem: () => null,
showSelection(html) { this.html = html; }
};
manager.renderSelected();
console.log(JSON.stringify({html: manager.html,
buttons: manager.getActionButtons(STATE, {version: 'unknown'}, true)}));
""".replace('STATE', _json(state)))
self.assertIn('Selected <b>1</b>', result['html'])
self.assertIn(state, unescape(result['html']))
self.assertFalse(handler_attrs(result['html']))
self.assertNotIn('img', tags(result['html']))
if state == 'enabled':
self.assertIn('<button', result['buttons'])
else:
self.assertEqual(result['buttons'], '')
def _json(value) -> str:
import json
return json.dumps(value)
def _unique_index(source: str, marker: str) -> int:
"""Index of `marker`, refusing to guess when it is not unique.
js_lift.line_containing takes the FIRST occurrence silently. Every marker
here is unique today; this makes a future second occurrence fail loudly
instead of pinning whichever copy happens to come first.
"""
count = source.count(marker)
assert count == 1, "marker %r occurs %d times; disambiguate it" % (marker, count)
return source.index(marker)
def _unique_line(source: str, marker: str) -> str:
"""The single source line holding `marker`, with the same uniqueness rule."""
index = _unique_index(source, marker)
start = source.rfind("\n", 0, index) + 1
return source[start:source.index("\n", index)]
def _call_argument(line: str, call_marker: str) -> str:
"""The argument text of `call_marker(...)`, dropping exactly one `)` and `;`.
NOT `.rstrip(");")`: rstrip takes a CHARACTER SET, so it eats every trailing
`)`, `;` and `"`. A fix spelled `updateMessage(escape(`...`));` would lose a
closing paren it needs and become a node syntax error -- which pytest would
show as this guard failing, i.e. the fix would look like the defect.
"""
text = line.split(call_marker, 1)[1].rstrip()
if text.endswith(";"):
text = text[:-1].rstrip()
assert text.endswith(")"), "unexpected call shape: %r" % (line.strip(),)
return text[:-1]
if __name__ == "__main__":
unittest.main()
tests/test_update_result_rendering.py
+58
View File
@@ -0,0 +1,58 @@
"""Render actual update-completion handlers in an isolated browser."""
from pathlib import Path
import pytest
from js_lift import JsSource
JS = JsSource(Path(__file__).resolve().parent.parent / "js")
@pytest.mark.parametrize("status", ["success", "failed"])
def test_update_results_preserve_links_titles_and_missing_url_labels(status):
playwright = pytest.importorskip("playwright.sync_api")
helpers = JS.lift_declaration("common.js", "export function sanitizeHTML(")
helpers += JS.lift_span(
"common.js", "export const SAFE_URL_SCHEMES",
"export const safeHref = (url) => sanitizeHTML(sanitizeUrl(url));",
).replace("export ", "")
helpers += JS.lift_declaration("common.js", "export function show_message(")
handler = JS.lift_declaration("comfyui-manager.js", "async function onQueueStatus(")
url = "https://example.test/guide?q=O'Reilly&lang=en"
title = "Guide <draft> & notes"
fallback_id = "Pack <draft>"
with playwright.sync_playwright() as driver:
if not Path(driver.chromium.executable_path).exists():
pytest.skip("install Chromium with: playwright install chromium")
browser = driver.chromium.launch()
page = browser.new_page()
page.route("**/*", lambda route: route.abort())
page.set_content('<div id="dialog"></div>')
page.add_script_tag(content="""
let is_updating = true;
function reset_action_buttons() {}
function setNeedRestart() {}
function infoToast() {}
const app = {ui: {dialog: {
element: document.getElementById('dialog'),
show(message) { this.element.innerHTML = message; }
}}};
""" + helpers + handler)
page.evaluate("event => onQueueStatus(event)", {"detail": {
"status": "done", "nodepack_result": {
"comfyui": "skip",
"pack-link": {"url": url, "title": title, "msg": status},
fallback_id: {"msg": status},
},
}})
items = page.locator("#dialog li")
assert items.all_text_contents() == [title, fallback_id]
assert items.nth(0).locator("a").get_attribute("href") == url
assert set(items.nth(0).locator("a").evaluate("node => node.getAttributeNames()")) == {
"href", "target", "rel",
}
assert items.nth(0).locator("a").get_attribute("rel") == "noopener noreferrer"
assert items.nth(1).locator("a").count() == 0
assert page.locator("#dialog draft").count() == 0
browser.close()
tests/test_url_sanitize_parity.py
+410
View File
@@ -0,0 +1,410 @@
"""Drift guard: the JS sanitizeUrl must agree with glob/manager_util.py::sanitize_url.
There are two implementations of one URL allow-list — ``sanitizeUrl`` in
js/common.js and ``sanitize_url`` in glob/manager_util.py. They agree today.
Nothing structural keeps them agreeing: a change to either side has no failing
test on the other, and the JS side is what stands between channel data and an
href. This module is that missing coupling. It runs BOTH real implementations
over a shared case list and fails on any divergence, naming the input and both
answers.
WHAT IT COMPARES: the real shipped code on both sides. The Python function is
loaded by file location (``glob/`` has no ``__init__.py``, and putting it on
sys.path would shadow the stdlib ``glob``); the JS function is lifted out of
js/common.js by tests/js_lift and executed in node. Neither is a copy, so a
perturbation of either implementation turns this guard RED.
CASES: the shared matrix at ``cases/url_sanitize_cases.json`` (57 inputs handed
over by gm3-review from the WI-112 and WI-122 reviews, since extended) PLUS a
generated grid that composes scheme x in-scheme-noise x delimiter. The generated
half matters because a fixed list can only catch divergence it happens to
contain, and the implementations are what is under test, not the list.
TWO AXES ARE COMPARED, not one. Behaviour over sampled inputs cannot see a
difference no sample happens to hold, so the two pieces of CONFIGURATION the
implementations branch on are compared directly as well: the scheme allow-list
(``test_the_scheme_allow_lists_are_identical``) and the whitespace set each side
trims off an accepted URL (``test_the_trim_sets_are_identical``). The trim axis
was added after the matrix was found to contain no BOM or unicode-whitespace
input at all — ``String.prototype.trim()`` and ``str.strip()`` disagree on six
codepoints (U+FEFF one way; U+001C-001F and U+0085 the other), and every case in
the matrix had ASCII edges, so the divergence sat unguarded. js/common.js now
spells Python's set out rather than delegating to ``trim()``; see the
``unicode_whitespace_divergence`` block in the case file.
⛔ STRING INPUTS ONLY, deliberately. Python ``str()`` and JS ``String()`` are
not equivalent for non-strings — 1.0 -> '1.0' vs '1', True -> 'True' vs 'true',
[1,2] -> '[1, 2]' vs '1,2', {} -> '{}' vs '[object Object]' — so feeding
non-strings would report divergence for something that is not a defect. In
practice ``url`` arrives as a string off the JSON payload. The two aligned
non-string cases that DO matter are pinned separately in
``test_null_and_undefined_agree`` rather than left as an undocumented gap; the
right response to a coercion mismatch is to narrow the domain, never to loosen
the comparison, because a loosened comparison stops catching real drift.
"""
import inspect
import json
import os
import re
import unittest
from pathlib import Path
from js_lift import NODE, JsSource, run_node
from manager_test_utils import load_manager_util
REPO_ROOT = Path(__file__).resolve().parent.parent
JS = JsSource(os.environ.get("MANAGER_JS_DIR") or (REPO_ROOT / "js"))
CASES_PATH = Path(__file__).resolve().parent / "cases" / "url_sanitize_cases.json"
# Every codepoint ``str.strip()`` removes, measured by RUNNING it rather than by
# restating a table. sanitize_url returns ``raw.strip()``, so this is the set the
# JS side has to match; test_the_trim_sets_are_identical asserts that linkage
# still holds before comparing.
PYTHON_TRIM_SET = frozenset(c for c in range(0x110000) if chr(c).strip() == "")
def _generated_cases(allow_listed):
"""Compose scheme x in-scheme noise x delimiter into extra inputs.
A fixed list only catches the divergence it already contains. This grid is
derived from the axes the two implementations actually branch on — the
scheme allow-list, the control-char strip, and the head-delimiter cut — so
it explores combinations nobody wrote down.
``allow_listed`` is the UNION of the two live allow-lists, not a literal, so
a scheme added to either side is exercised behaviourally without anyone
remembering to add a case for it. That is a second layer under
test_the_scheme_allow_lists_are_identical, which is what catches an
allow-list divergence definitively; this layer only makes the behavioural
sampling track the configuration instead of drifting away from it.
"""
schemes = sorted(set(allow_listed) | {
"HTTP", "javascript", "data", "vbscript", "file", "a-b", "a.b",
})
noises = ["", "\t", "\n", "\x00", "\x01", " ", "\x7f"]
delimiters = ["", "/", "?", "#", "//host/p", "?q=1", "#frag"]
for scheme in schemes:
for noise in noises:
head = scheme[:2] + noise + scheme[2:]
for delimiter in delimiters:
yield "%s:%s" % (head, delimiter)
yield " %s:%s" % (head, delimiter)
def _all_cases(allow_listed):
matrix = json.loads(CASES_PATH.read_text(encoding="utf-8"))
cases = list(matrix["shared_matrix"])
cases.extend(_generated_cases(allow_listed))
# De-duplicate while preserving order so a failure names a stable index.
seen = set()
ordered = []
for case in cases:
if case not in seen:
seen.add(case)
ordered.append(case)
return ordered
def _lift_sanitize_url() -> str:
"""The REAL sanitizeUrl and the two constants it closes over, from common.js.
Lifted as one contiguous span rather than three declarations: SAFE_URL_SCHEMES
is an array literal, so a brace-matching lift would run past it into the
function body and declare the constants twice.
"""
span = JS.lift_span("common.js", "export const SAFE_URL_SCHEMES",
"export const safeHref = (url) => sanitizeHTML(sanitizeUrl(url));")
# safeHref needs sanitizeHTML, which this guard does not exercise; drop the
# trailing line rather than pulling an unrelated dependency into the script.
span = span[:span.index("export const safeHref")]
return span.replace("export ", "")
_JS_ALLOW_LIST_LITERAL = re.compile(r"(SAFE_URL_SCHEMES\s*=\s*)\[[^\]]*\]")
def _js_allow_list(lifted_js=None):
"""The JS SAFE_URL_SCHEMES as the running code actually sees it.
Evaluated in node rather than parsed out of the source, so a change to how
the list is spelled cannot make this silently read the old value.
"""
return run_node(
"%s\nconsole.log(JSON.stringify({v: SAFE_URL_SCHEMES}));\n"
% (lifted_js if lifted_js is not None else _lift_sanitize_url())
)["v"]
def _js_trim_set(lifted_js=None):
"""Every codepoint the JS side strips off the edges of an accepted URL.
Evaluated in node, for the same reason ``_js_allow_list`` is: a set parsed
out of the source text stops describing the running code the moment the
spelling changes.
Shimmed on a PRE-FIX tree, where sanitizeUrl called ``raw.trim()`` inline and
no ``trimUrl`` exists. Without the shim this guard would raise on the old
revision instead of reporting the divergence, and the RED half of the fix
could not be demonstrated with the guard that is supposed to catch it.
"""
lifted = _lift_sanitize_url() if lifted_js is None else lifted_js
shim = "" if "trimUrl" in lifted else "const trimUrl = (value) => value.trim();\n"
return set(run_node(
"%s\n%sconst out = [];\n"
"for (let c = 0; c < 0x110000; c += 1) {\n"
" if (trimUrl(String.fromCodePoint(c)) === '') out.push(c);\n"
"}\n"
"console.log(JSON.stringify({v: out}));\n" % (lifted, shim)
)["v"])
def _perturb_js_allow_list(lifted_js, extra_scheme):
"""Add a scheme to the lifted JS allow-list, whatever it currently holds.
Rewriting the array by PATTERN rather than by a hard-coded literal matters:
a literal-match perturbation stops applying the moment someone edits the
allow-list, and then the harness's own did-it-apply self-test fires instead
of the parity comparison — a red that looks like coverage and is not.
"""
perturbed, count = _JS_ALLOW_LIST_LITERAL.subn(
lambda m: "%s%s" % (m.group(1), json.dumps(sorted(
set(_js_allow_list(lifted_js)) | {extra_scheme}))),
lifted_js, count=1,
)
assert count == 1, "could not locate the JS SAFE_URL_SCHEMES array to perturb"
return perturbed
def _js_results(cases):
"""Run the REAL js/common.js sanitizeUrl over every case, in node."""
lifted = _lift_sanitize_url()
return run_node(
"%s\nconst cases = %s;\n"
"console.log(JSON.stringify({results: cases.map((c) => sanitizeUrl(c))}));\n"
% (lifted, json.dumps(cases))
)["results"]
@unittest.skipIf(NODE is None, "node is required to execute the lifted production JS")
class UrlSanitizeParityTest(unittest.TestCase):
"""Both real implementations, one case list, zero tolerated divergence."""
@classmethod
def setUpClass(cls):
cls.manager_util = load_manager_util()
cls.allow_lists = {
"python": set(cls.manager_util.SAFE_URL_SCHEMES),
"js": set(_js_allow_list()),
}
cls.cases = _all_cases(cls.allow_lists["python"] | cls.allow_lists["js"])
cls.js = _js_results(cls.cases)
def test_case_matrix_is_actually_loaded(self):
# A guard that silently ran zero cases would pass forever.
matrix = json.loads(CASES_PATH.read_text(encoding="utf-8"))
self.assertGreaterEqual(len(matrix["shared_matrix"]), 57)
self.assertGreater(len(self.cases), 400)
self.assertEqual(len(self.js), len(self.cases))
def test_the_scheme_allow_lists_are_identical(self):
"""Compare the CONFIGURATION, not only the behaviour it drives.
Comparing behaviour over sampled inputs cannot see a scheme added to one
allow-list unless that scheme happens to be in the sample — adding 'ftp'
or 'mailto' or 'ws' to either side changes what is accepted and produces
no divergence at all. That is the most plausible future drift for this
pair (someone adds a scheme for a real reason and forgets the other
side), so it is closed definitively here rather than by sampling.
"""
self.assertEqual(
self.allow_lists["js"], self.allow_lists["python"],
"the URL scheme allow-lists have diverged — js/common.js accepts %r "
"and glob/manager_util.py accepts %r. A scheme added to one side and "
"not the other silently changes what reaches an href. Fix the lists, "
"do NOT relax this comparison."
% (sorted(self.allow_lists["js"]), sorted(self.allow_lists["python"])),
)
def test_the_trim_sets_are_identical(self):
"""Compare the trim CONFIGURATION, the way the allow-lists are compared.
Both implementations end an accepted URL with a trim, and the two trims
are NOT the same function: ``String.prototype.trim()`` strips U+FEFF,
which ``str.strip()`` keeps, and leaves U+001C-001F and U+0085, which
``str.strip()`` strips. Sampling cannot be relied on to find that — the
handed-over matrix had ASCII edges on all 62 inputs and reported no
divergence for years of cases. The cases added alongside this test do
catch it, but the set comparison is what closes the axis DEFINITIVELY,
exactly as test_the_scheme_allow_lists_are_identical does for schemes.
The fix direction is fixed, not free: glob/manager_util.py is the
declared source of truth for this pair, so the JS side matches Python.
"""
source = inspect.getsource(self.manager_util.sanitize_url)
self.assertIn(
"raw.strip()", source,
"sanitize_url no longer returns raw.strip(), so PYTHON_TRIM_SET is "
"measuring the wrong function — re-derive it from whatever the "
"Python side trims with now.",
)
js = _js_trim_set()
self.assertEqual(
js, set(PYTHON_TRIM_SET),
"the URL trim sets have diverged. js/common.js strips %s that "
"glob/manager_util.py keeps, and keeps %s that it strips. Both sides "
"must trim the SAME set or the two implementations answer differently "
"for the same accepted URL; fix js/common.js's URL_TRIM, do NOT relax "
"this comparison."
% (sorted("U+%04X" % c for c in js - PYTHON_TRIM_SET) or "nothing",
sorted("U+%04X" % c for c in PYTHON_TRIM_SET - js) or "nothing"),
)
def test_js_and_python_agree_on_every_case(self):
divergences = []
for case, js_answer in zip(self.cases, self.js):
py_answer = self.manager_util.sanitize_url(case)
if py_answer != js_answer:
divergences.append(
" input=%r\n python=%r\n js =%r" % (case, py_answer, js_answer)
)
self.assertEqual(
divergences, [],
"js/common.js sanitizeUrl has drifted from "
"glob/manager_util.py::sanitize_url over %d case(s):\n%s\n"
"Fix the implementations so they agree again — do NOT loosen this "
"comparison, which is the only thing coupling them."
% (len(divergences), "\n".join(divergences)),
)
def test_null_and_undefined_agree(self):
"""The two non-string inputs both sides handle explicitly.
Everything else non-string is out of domain — see the module docstring.
"""
out = run_node(
"%s\nconsole.log(JSON.stringify({"
"nul: sanitizeUrl(null), undef: sanitizeUrl(undefined)}));\n"
% _lift_sanitize_url()
)
self.assertEqual(out["nul"], self.manager_util.sanitize_url(None))
self.assertEqual(out["undef"], "#")
def _divergences_against(self, lifted_js):
"""Run the SAME comparison test_js_and_python_agree_on_every_case runs."""
js = run_node(
"%s\nconst cases = %s;\n"
"console.log(JSON.stringify({results: cases.map((c) => sanitizeUrl(c))}));\n"
% (lifted_js, json.dumps(self.cases))
)["results"]
return [
case for case, answer in zip(self.cases, js)
if self.manager_util.sanitize_url(case) != answer
]
def test_the_guard_goes_red_on_each_perturbation_of_the_js_side(self):
"""Prove the comparison has teeth rather than asserting that it does.
Each perturbation is a shape a real regression takes — a scheme added to
the allow-list, the control-char strip losing its /g flag so only the
FIRST noise byte is removed, the scheme-less branch changing its answer,
and the empty-input fallback changing. Every one must be caught by the
case list as it stands, so a guard whose lift silently returned a stub,
or whose matrix had rotted into irrelevance, cannot look healthy.
"""
base = _lift_sanitize_url()
perturbations = {
# By PATTERN, not by literal: a literal-match perturbation stops
# applying the moment someone edits the allow-list, and the
# did-it-apply assertion below then fires INSTEAD of the parity
# comparison — a red that reads like coverage and is not.
"allow-list gains a scheme": _perturb_js_allow_list(base, "javascript"),
"noise strip loses its /g flag":
base.replace("/[\\x00-\\x20\\x7f]/g", "/[\\x00-\\x20\\x7f]/", 1),
"scheme-less branch stops trimming":
base.replace("return trimUrl(raw);\n}", "return raw;\n}", 1),
# The regression this fix closed: delegating back to String.trim(),
# which strips U+FEFF that str.strip() keeps and keeps U+001C-001F
# and U+0085 that str.strip() strips. Caught only by the unicode
# cases added with the fix — before them the matrix had ASCII edges
# throughout and this perturbation produced zero divergence.
"trim delegates back to String.trim()":
base.replace("trimUrl(raw)", "raw.trim()"),
"empty-input fallback changes": base.replace("return '#';", "return '';", 1),
}
for label, lifted in perturbations.items():
with self.subTest(perturbation=label):
self.assertNotEqual(
lifted, base,
"perturbation %r did not apply — this is the HARNESS "
"self-test, NOT the parity comparison. Do not read it as "
"coverage: fix the perturbation so the comparison is "
"actually exercised." % (label,),
)
caught = self._divergences_against(lifted)
self.assertNotEqual(
caught, [],
"perturbing the JS side (%s) produced NO divergence — the case "
"list cannot detect this drift, so the guard is weaker than it "
"looks. Add a case that distinguishes it." % (label,),
)
def test_the_guard_goes_red_when_either_allow_list_gains_a_scheme(self):
"""The perturbation that was NOT caught before this test existed.
Both directions are exercised, because the drift can start on either
side and the guard is only worth having if it is symmetric. 'ftp' is used
deliberately: it is absent from the shared matrix, so nothing but the
allow-list comparison itself can catch it.
"""
with self.subTest(side="python (the declared SoT)"):
real = self.manager_util.SAFE_URL_SCHEMES
try:
setattr(self.manager_util, "SAFE_URL_SCHEMES", frozenset(set(real) | {"ftp"}))
self.assertNotEqual(
set(self.manager_util.SAFE_URL_SCHEMES), self.allow_lists["js"],
"adding a scheme to the PYTHON allow-list produced no "
"divergence — the configuration axis is unguarded again",
)
finally:
setattr(self.manager_util, "SAFE_URL_SCHEMES", real)
self.assertEqual(set(self.manager_util.SAFE_URL_SCHEMES), self.allow_lists["python"])
with self.subTest(side="js"):
perturbed = _js_allow_list(_perturb_js_allow_list(_lift_sanitize_url(), "ftp"))
self.assertIn("ftp", perturbed, "the JS perturbation did not apply")
self.assertNotEqual(
set(perturbed), self.allow_lists["python"],
"adding a scheme to the JS allow-list produced no divergence",
)
def test_the_guard_goes_red_on_a_perturbed_python_side(self):
"""The coupling has to run both ways, not just JS-drifts-from-Python."""
real = self.manager_util.sanitize_url
try:
setattr(self.manager_util, "sanitize_url", lambda url: "#")
caught = self._divergences_against(_lift_sanitize_url())
finally:
setattr(self.manager_util, "sanitize_url", real)
self.assertNotEqual(
caught, [],
"perturbing the PYTHON side produced no divergence — the comparison "
"is not reading the Python implementation it claims to",
)
def test_sanitize_url_has_exactly_one_client_side_home(self):
"""The consolidation this guard exists to protect."""
js_dir = JS.js_dir
homes = sorted(
path.name for path in js_dir.glob("*.js")
if "function sanitizeUrl" in path.read_text(encoding="utf-8")
)
self.assertEqual(
homes, ["common.js"],
"sanitizeUrl must live only in common.js; every other client file "
"imports it. A second copy is the drift this guard cannot see, "
"because it only compares the copy in common.js.",
)
if __name__ == "__main__":
unittest.main()