Your art has been shared in the ComfyUI Matrix server's #share channel!"; } diff --git a/comfyui_manager/js/comfyui-share-copus.js b/comfyui_manager/js/comfyui-share-copus.js index 46288e596..af14f32b0 100644 --- a/comfyui_manager/js/comfyui-share-copus.js +++ b/comfyui_manager/js/comfyui-share-copus.js @@ -1,6 +1,6 @@ import { app } from "../../scripts/app.js"; import { $el, ComfyDialog } from "../../scripts/ui.js"; -import { customAlert } from "./common.js"; +import { customAlert, sanitizeHTML, sanitizeUrl } from "./common.js"; const env = "prod"; @@ -883,7 +883,7 @@ export class CopusShareDialog extends ComfyDialog { const { data } = res.data; if (data) { const url = `${DEFAULT_HOMEPAGE_URL}/work/${data}`; - this.message.innerHTML = `Workflow has been shared successfully. Click here to view it.`; + this.message.innerHTML = `Workflow has been shared successfully. Click here to view it.`; this.previewImage.src = ""; this.previewImage.style.display = "none"; this.uploadedImages = []; diff --git a/comfyui_manager/js/comfyui-share-openart.js b/comfyui_manager/js/comfyui-share-openart.js index d867d4d79..cb294bab4 100644 --- a/comfyui_manager/js/comfyui-share-openart.js +++ b/comfyui_manager/js/comfyui-share-openart.js @@ -1,7 +1,7 @@ import {app} from "../../scripts/app.js"; import {api} from "../../scripts/api.js"; import {ComfyDialog, $el} from "../../scripts/ui.js"; -import { customAlert } from "./common.js"; +import { customAlert, sanitizeHTML, sanitizeUrl } from "./common.js"; const LOCAL_STORAGE_KEY = "openart_comfy_workflow_key"; const DEFAULT_HOMEPAGE_URL = "https://openart.ai/workflows/dev?developer=true"; @@ -511,7 +511,7 @@ export class OpenArtShareDialog extends ComfyDialog { const {workflow_id} = response.data; if (workflow_id) { const url = `https://openart.ai/workflows/-/-/${workflow_id}`; - this.message.innerHTML = `Workflow has been shared successfully. Click here to view it.`; + this.message.innerHTML = `Workflow has been shared successfully. Click here to view it.`; this.previewImage.src = ""; this.previewImage.style.display = "none"; this.uploadedImages = []; diff --git a/comfyui_manager/js/comfyui-share-youml.js b/comfyui_manager/js/comfyui-share-youml.js index 20dc8a2e4..d3452ea1c 100644 --- a/comfyui_manager/js/comfyui-share-youml.js +++ b/comfyui_manager/js/comfyui-share-youml.js @@ -1,7 +1,7 @@ import {app} from "../../scripts/app.js"; import {api} from "../../scripts/api.js"; import {ComfyDialog, $el} from "../../scripts/ui.js"; -import { customAlert } from "./common.js"; +import { customAlert, sanitizeHTML, sanitizeUrl } from "./common.js"; const BASE_URL = "https://youml.com"; //const BASE_URL = "http://localhost:3000"; @@ -420,7 +420,7 @@ export class YouMLShareDialog extends ComfyDialog { } } this.message.innerHTML = `${messagePrefix} To turn your workflow into an interactive app, ` + - `visit it on YouML`; + `visit it on YouML`; this.uploadedImages = []; this.nameInput.value = ""; diff --git a/comfyui_manager/js/common.js b/comfyui_manager/js/common.js index fd06998de..f79522827 100644 --- a/comfyui_manager/js/common.js +++ b/comfyui_manager/js/common.js @@ -216,7 +216,7 @@ export async function install_pip(packages) { }); if(res.status == 403) { - show_message("To use this feature, set
allow_pip_install = true in the [default] section of config.ini. This setting is independent of security_level.Note: if the ComfyUI listener is not local,
network_mode = personal_cloud is also required.");
+ show_message(await install_denial_message(res, 'allow_pip_install'));
return;
}
@@ -251,7 +251,7 @@ export async function install_via_git_url(url, manager_dialog) {
});
if(res.status == 403) {
- show_message("To use this feature, set allow_git_url_install = true in the [default] section of config.ini. This setting is independent of security_level.Note: if the ComfyUI listener is not local,
network_mode = personal_cloud is also required.");
+ show_message(await install_denial_message(res, 'allow_git_url_install'));
return;
}
@@ -659,4 +659,54 @@ function initTooltip () {
document.body.addEventListener('mouseleave', mouseleaveHandler, true);
}
-initTooltip();
\ No newline at end of file
+initTooltip();
+// Installation flags and network mode take effect after a restart.
+const INSTALL_DENIAL_RESTART_NOTE = "Both values are read once at ComfyUI startup, so changing either one needs a restart, done with the server down: STOP ComfyUI, change the setting, then start it again."; + +export const INSTALL_DENIAL_MESSAGES = { + allow_git_url_install: "To use this feature, set
allow_git_url_install = true in the [default] section of config.ini. This setting is independent of security_level.Note: if the ComfyUI listener is not local,
network_mode = personal_cloud is also required." + INSTALL_DENIAL_RESTART_NOTE,
+ allow_pip_install: "To use this feature, set allow_pip_install = true in the [default] section of config.ini. This setting is independent of security_level.Note: if the ComfyUI listener is not local,
network_mode = personal_cloud is also required." + INSTALL_DENIAL_RESTART_NOTE
+};
+
+// Last resort when no entry above applies. Names no flag on purpose.
+const INSTALL_DENIAL_UNKNOWN_REASON = "This action was refused by the server, and the client could not determine which setting caused it. The server terminal log states the exact condition. Please contact the administrator.";
+
+// Use the endpoint's flag when the 403 response body is missing or malformed.
+export async function install_denial_message(res, fallbackReason) {
+ let reason = fallbackReason;
+ try {
+ const data = await res.json();
+ if (typeof data === 'object' && data !== null && typeof data.reason === 'string'
+ && Object.prototype.hasOwnProperty.call(INSTALL_DENIAL_MESSAGES, data.reason)) {
+ reason = data.reason;
+ }
+ }
+ catch {
+ // absent / non-JSON body: keep the fallback
+ }
+ return INSTALL_DENIAL_MESSAGES[reason] ?? INSTALL_DENIAL_UNKNOWN_REASON;
+}
+
+/**
+ * Scheme allow-list for registry-supplied URLs that land in an href.
+ * Returns the URL if it is http(s) or relative, otherwise "". Never throws.
+ */
+export function sanitizeUrl(url) {
+ const raw = String(url ?? '').trim();
+ if (!raw) {
+ return '';
+ }
+ let parsed;
+ try {
+ parsed = new URL(raw);
+ }
+ catch {
+ // not absolute: keep a scheme-less relative path, drop anything else
+ return /^[a-z][a-z0-9+.\-]*:/i.test(raw) ? '' : raw;
+ }
+ // URL parsing already normalised the scheme (`java\tscript:` collapses).
+ if (parsed.protocol === 'http:' || parsed.protocol === 'https:') {
+ return raw;
+ }
+ return '';
+}
diff --git a/comfyui_manager/js/custom-nodes-manager.js b/comfyui_manager/js/custom-nodes-manager.js
index 7d587a00f..2ef8ff510 100644
--- a/comfyui_manager/js/custom-nodes-manager.js
+++ b/comfyui_manager/js/custom-nodes-manager.js
@@ -8,11 +8,13 @@ import {
fetchData, md5, icons, show_message, customConfirm, customAlert, customPrompt,
sanitizeHTML, infoToast, showTerminal, setNeedRestart,
storeColumnWidth, restoreColumnWidth, getTimeAgo, copyText, loadCss,
- showPopover, hidePopover, generateUUID
+ showPopover, hidePopover, generateUUID, sanitizeUrl
} from "./common.js";
+// Registry titles, names and descriptions are server-escaped; other fields are raw.
+
// https://cenfun.github.io/turbogrid/api.html
-import TG from "./turbogrid.esm.js";
+import ManagerGrid from "./manager-grid.js";
loadCss("./custom-nodes-manager.css");
@@ -363,19 +365,20 @@ export class CustomNodesManager {
installGroups.enabled = installGroups.enabled.filter(it => it !== "disable" && it !== "uninstall" && it !== "switch");
}
- let list = installGroups[action];
+ const list = installGroups[action];
- if(is_selected_button || rowItem?.version === "unknown") {
- list = list.filter(it => it !== "switch");
- }
-
- if (!list) {
+ if (!Array.isArray(list)) {
return "";
}
- return list.map(id => {
+ const shown = (is_selected_button || rowItem?.version === "unknown")
+ ? list.filter(it => it !== "switch")
+ : list;
+
+ return shown.map(id => {
const bt = buttons[id];
- return ``;
+ // `action` is registry-derived and lands in an attribute.
+ return ``;
}).join("");
}
@@ -518,7 +521,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 = this.createFlyover(container);
@@ -579,6 +582,9 @@ export class CustomNodesManager {
grid.setOption({
+ highlightKeywords: {
+ textGenerator: (row, column) => column === 'author' ? sanitizeHTML(String(row[column] ?? '')) : row[column]
+ },
theme: 'dark',
selectVisible: true,
selectMultiple: true,
@@ -646,7 +652,8 @@ export class CustomNodesManager {
let res = await response.json();
- let title = `Error message occurred while importing the '${rowItem.title}' module.` + // `title` is server-escaped; `res['msg']` below is raw and is escaped. + let title = `Error message occurred while importing the '${String(rowItem.title ?? '')}' module.
` if(res.code == 400) { @@ -711,11 +718,12 @@ export class CustomNodesManager { const link = document.createElement('a'); 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.innerHTML = `${title}`; + link.rel = 'noopener noreferrer'; + link.innerHTML = `${String(title ?? '')}`; link.title = rowItem.originalData.id; container.appendChild(link); @@ -734,11 +742,11 @@ export class CustomNodesManager { } if(rowItem.cnr_latest && version != rowItem.cnr_latest) { if(version == 'nightly') { - return `
${version}
[${rowItem.cnr_latest}]
`;
+ return `${sanitizeHTML(String(version))}
[${sanitizeHTML(String(rowItem.cnr_latest))}]
`;
}
- return `${version}
[↑${rowItem.cnr_latest}]
`;
+ return `${sanitizeHTML(String(version))}
[↑${sanitizeHTML(String(rowItem.cnr_latest))}]
`;
}
- return version;
+ return sanitizeHTML(String(version));
}
}, {
id: 'action',
@@ -777,13 +785,13 @@ export class CustomNodesManager {
width: 400,
maxWidth: 5000,
invisible: !this.hasAlternatives(),
- classMap: 'cn-pack-desc'
+ classMap: 'cn-pack-desc' // composed HTML; tags are escaped in getAlternatives
}, {
id: 'description',
name: 'Description',
width: 400,
maxWidth: 5000,
- classMap: 'cn-pack-desc'
+ classMap: 'cn-pack-desc' // composed HTML from the server (convert_markdown_to_html)
}, {
id: 'author',
name: 'Author',
@@ -791,9 +799,9 @@ export class CustomNodesManager {
classMap: "cn-pack-author",
formatter: (author, rowItem, columnItem) => {
if (rowItem.trust) {
- return `✅ ${author}`;
+ return `✅ ${sanitizeHTML(String(author ?? ''))}`;
}
- return author;
+ return sanitizeHTML(String(author ?? ''));
}
}, {
id: 'stars',
@@ -807,7 +815,7 @@ export class CustomNodesManager {
if (typeof stars === 'number') {
return stars.toLocaleString();
}
- return stars;
+ return sanitizeHTML(String(stars ?? ''));
}
}, {
id: 'last_update',
@@ -822,7 +830,7 @@ export class CustomNodesManager {
}
const ago = getTimeAgo(last_update);
const short = `${last_update}`.split(' ')[0];
- return `${short}`;
+ return `${sanitizeHTML(String(short))}`;
}
}];
@@ -1211,7 +1219,7 @@ export class CustomNodesManager {
const rowItem = d.rowItem;
const isNotInstalled = rowItem.action == "not-installed";
- let titleHtml = `${rowItem.title}
`;
+ let titleHtml = `${String(rowItem.title ?? '')}
`;
if (isNotInstalled) {
titleHtml += 'Not Installed
'
}
@@ -1227,11 +1235,12 @@ export class CustomNodesManager {
list.push(``);
list.push(`
`);
@@ -1349,7 +1358,7 @@ export class CustomNodesManager {
return;
}
- const selectedMap = {};
+ const selectedMap = Object.create(null);
selectedList.forEach(item => {
let type = item.action;
if (item.restart) {
@@ -1367,8 +1376,10 @@ export class CustomNodesManager {
const list = [];
Object.keys(selectedMap).forEach(v => {
const filterItem = this.getFilterItem(v);
+ // `v` is a registry-supplied `state`; escape it when no filter label matches.
+ const typeLabel = filterItem ? filterItem.label : sanitizeHTML(String(v ?? ''));
list.push(``);
});
@@ -1466,7 +1477,6 @@ export class CustomNodesManager {
target.classList.add("cn-btn-loading");
this.showError("");
- let needRestart = false;
let errorMsg = "";
let target_items = [];
@@ -1475,13 +1485,13 @@ export class CustomNodesManager {
for (const hash of list) {
const item = this.grid.getRowItemBy("hash", hash);
- target_items.push(item);
if (!item) {
- errorMsg = `Not found custom node: ${hash}`;
+ errorMsg = `Not found custom node: ${sanitizeHTML(String(hash))}`;
break;
}
+ target_items.push(item);
this.grid.scrollRowIntoView(item);
if (!this.focusInstall(item, mode)) {
@@ -1538,23 +1548,48 @@ export class CustomNodesManager {
this.batch_id = generateUUID();
batch['batch_id'] = this.batch_id;
- const res = await api.fetchApi(`/v2/manager/queue/batch`, {
- method: 'POST',
- body: JSON.stringify(batch)
- });
+ let failed;
+ try {
+ const { data, error } = await fetchData(`/v2/manager/queue/batch`, {
+ method: 'POST',
+ body: JSON.stringify(batch)
+ });
+ if (error) throw error;
+ if (!Array.isArray(data?.failed)) throw new Error('Invalid batch response.');
+ failed = data.failed;
+ } catch (error) {
+ errorMsg = `Failed to submit installation request: ${sanitizeHTML(String(error))}`;
+ this.showError(errorMsg);
+ show_message("[Installation Errors]\n" + errorMsg);
+ target.classList.remove("cn-btn-loading");
+ this.element.querySelectorAll(".cn-btn-loading").forEach(button => {
+ button.classList.remove("cn-btn-loading");
+ });
+ this.hideLoading();
+ this.hideStop();
+ this.install_context = undefined;
+ return;
+ }
+ for (const id of failed) {
+ const item = target_items.find(item => item.originalData.id === id);
+ errorMsg += `[FAIL] ${item?.title ?? sanitizeHTML(String(id))}\n`;
+ }
+ if (errorMsg) {
+ this.showError(errorMsg);
+ show_message("[Installation Errors]\n" + errorMsg);
+ }
- let failed = await res.json();
+ showTerminal();
- if(failed.length > 0) {
- for(let k in failed) {
- let hash = failed[k];
- const item = this.grid.getRowItemBy("hash", hash);
- errorMsg = `[FAIL] ${item.title}`;
- }
+ // No queued work means the server will not send batch-done.
+ if (target_items.every(item => failed.includes(item.originalData.id))) {
+ this.hideLoading();
+ this.hideStop();
+ this.install_context = undefined;
+ return;
}
this.showStop();
- showTerminal();
}
}
@@ -1601,7 +1636,7 @@ export class CustomNodesManager {
let v = result[hash];
if(v != 'success' && v != 'skip')
- errorMsg += v+'\n';
+ errorMsg += sanitizeHTML(String(v))+'\n';
}
for(let k in self.install_context.targets) {
@@ -1719,7 +1754,7 @@ export class CustomNodesManager {
if(unresolved_cnr_list.length > 0) {
let error_msg = "Failed to find the following ComfyRegistry list.\nThe cache may be outdated, or the nodes may have been removed from ComfyRegistry.${i+1}
`);
- list.push(`${it.name}
`);
+ // `it.name` is a raw node name from getmappings (not the pack name)
+ list.push(`${sanitizeHTML(String(it.name ?? ''))}
`);
if (it.conflicts) {
list.push(`Conflict with${it.conflicts.map(c => {
- return `
`);
}
list.push(`${c.title}
`;
+ return `${String(c.title ?? '')}
`;
}).join(",")}"; for(let i in unresolved_cnr_list) { - error_msg += '
${tag.trim()}
`;
+ return `${sanitizeHTML(tag.trim())}
`;
}).join("");
hashMap[custom_node.hash] = {
@@ -1948,7 +1983,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) {
@@ -2077,6 +2112,7 @@ export class CustomNodesManager {
// ===========================================================================================
showSelection(msg) {
+ // Takes composed HTML (buttons included); ingredients are escaped in renderSelected.
this.element.querySelector(".cn-manager-selection").innerHTML = msg;
}
@@ -2084,18 +2120,17 @@ export class CustomNodesManager {
this.showMessage(err, "red");
}
+ // Messages contain HTML: preserve server-escaped names and escape raw values at the caller.
showMessage(msg, color) {
- if (color) {
- msg = `${msg}`;
- }
- this.element.querySelector(".cn-manager-message").innerHTML = msg;
+ const element = this.element.querySelector(".cn-manager-message");
+ element.style.color = color || "";
+ element.innerHTML = msg ?? "";
}
showStatus(msg, color) {
- if (color) {
- msg = `${msg}`;
- }
- this.element.querySelector(".cn-manager-status").innerHTML = msg;
+ const element = this.element.querySelector(".cn-manager-status");
+ element.style.color = color || "";
+ element.innerHTML = msg ?? "";
}
showLoading() {
diff --git a/comfyui_manager/js/manager-grid.js b/comfyui_manager/js/manager-grid.js
new file mode 100644
index 000000000..4ea8ff2bd
--- /dev/null
+++ b/comfyui_manager/js/manager-grid.js
@@ -0,0 +1,67 @@
+import { Grid } from "./turbogrid.esm.js";
+
+// Keep Manager's search hardening outside the vendored TurboGrid bundle.
+export default class ManagerGrid extends Grid {
+ highlightKeywordsFilter(rowItem, columns, value) {
+ const { textKey, textGenerator, highlightKey } = this.options.highlightKeywords;
+ for (const column of columns) {
+ rowItem[`${highlightKey}${column}`] = null;
+ }
+ const keywords = value ? String(value).trim().toLowerCase().split(/\s+/).filter(Boolean) : [];
+ if (!keywords.length) return true;
+
+ let matched = false;
+ for (const column of columns) {
+ const value = typeof textGenerator === "function" ? textGenerator(rowItem, column) : rowItem[column];
+ if (value === null || value === undefined) continue;
+ let text = String(value).trim();
+ if (!text) continue;
+ const key = `${textKey}${column}`;
+ if (rowItem[key] === null || rowItem[key] === undefined) {
+ // A detached element's innerHTML can still run image handlers.
+ rowItem[key] = new DOMParser().parseFromString(text, "text/html").body.textContent;
+ }
+ text = rowItem[key].toLowerCase();
+ let offset = 0;
+ const found = keywords.every(keyword => {
+ const index = text.indexOf(keyword, offset);
+ if (index === -1) return false;
+ offset = index + keyword.length;
+ return true;
+ });
+ if (found) {
+ rowItem[`${highlightKey}${column}`] = true;
+ this.highlightKeywords = keywords;
+ matched = true;
+ }
+ }
+ return matched;
+ }
+
+ highlightTextNodes(nodes, keywords) {
+ if (!keywords.length) return;
+ let keywordIndex = 0;
+ for (const node of nodes) {
+ const text = node.textContent;
+ const lower = text.toLowerCase();
+ const wrapper = document.createElement("span");
+ let offset = 0;
+ while (offset < text.length) {
+ const keyword = keywords[keywordIndex];
+ const index = lower.indexOf(keyword, offset);
+ if (index === -1) break;
+ wrapper.append(text.slice(offset, index));
+ const mark = document.createElement("mark");
+ mark.textContent = text.slice(index, index + keyword.length);
+ wrapper.append(mark);
+ offset = index + keyword.length;
+ keywordIndex = (keywordIndex + 1) % keywords.length;
+ }
+ if (wrapper.childNodes.length) {
+ // Decoded cell text stays in text nodes, including inside .
+ wrapper.append(text.slice(offset));
+ node.replaceWith(wrapper);
+ }
+ }
+ }
+}
diff --git a/comfyui_manager/js/model-manager.js b/comfyui_manager/js/model-manager.js
index bb085ba4d..16288eb48 100644
--- a/comfyui_manager/js/model-manager.js
+++ b/comfyui_manager/js/model-manager.js
@@ -3,17 +3,22 @@ import { $el } from "../../scripts/ui.js";
import {
manager_instance, rebootAPI,
fetchData, md5, icons, show_message, customAlert, infoToast, showTerminal,
- storeColumnWidth, restoreColumnWidth, loadCss, generateUUID
+ storeColumnWidth, restoreColumnWidth, loadCss, generateUUID,
+ sanitizeHTML, sanitizeUrl
} from "./common.js";
+
+// Model names and descriptions are server-escaped; other fields are raw.
+
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";
+const escapeCell = (value) => sanitizeHTML(String(value ?? ""));
const pageHtml = `
@@ -101,22 +106,17 @@ export class ModelManager {
updateFilter() {
const $filter = this.element.querySelector(".cmm-manager-filter");
- $filter.innerHTML = this.filterList.map(item => {
- const selected = item.value === this.filter ? " selected" : "";
- return ``
- }).join("");
+ const option = (item, current) => {
+ const selected = item.value === current ? " selected" : "";
+ return ``;
+ };
+ $filter.innerHTML = this.filterList.map(item => option(item, this.filter)).join("");
const $type = this.element.querySelector(".cmm-manager-type");
- $type.innerHTML = this.typeList.map(item => {
- const selected = item.value === this.type ? " selected" : "";
- return ``
- }).join("");
+ $type.innerHTML = this.typeList.map(item => option(item, this.type)).join("");
const $base = this.element.querySelector(".cmm-manager-base");
- $base.innerHTML = this.baseList.map(item => {
- const selected = item.value === this.base ? " selected" : "";
- return ``
- }).join("");
+ $base.innerHTML = this.baseList.map(item => option(item, this.base)).join("");
}
@@ -199,7 +199,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 +227,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 +328,12 @@ export class ModelManager {
maxWidth: 500,
classMap: 'cmm-node-name',
formatter: function(name, rowItem, columnItem, cellNode) {
- return `${name}`;
+ // Names are server-escaped; raw references need URL and attribute handling.
+ const href = sanitizeUrl(rowItem.reference);
+ if (!href) {
+ return `${String(name ?? '')}`;
+ }
+ return `${String(name ?? '')}`;
}
}, {
id: 'installed',
@@ -351,7 +359,11 @@ export class ModelManager {
sortable: false,
align: 'center',
formatter: (url, rowItem, columnItem) => {
- return `${icons.download}`;
+ const href = sanitizeUrl(url);
+ if (!href) {
+ return '';
+ }
+ return `${icons.download}`;
}
}, {
id: 'size',
@@ -361,29 +373,33 @@ export class ModelManager {
if (typeof size === "number") {
return this.formatSize(size);
}
- return size;
+ return sanitizeHTML(String(size ?? ''));
}
}, {
id: 'type',
name: 'Type',
- width: 100
+ width: 100,
+ formatter: escapeCell
}, {
id: 'base',
- name: 'Base'
+ name: 'Base',
+ formatter: escapeCell
}, {
id: 'description',
name: 'Description',
width: 400,
maxWidth: 5000,
- classMap: 'cmm-node-desc'
+ classMap: 'cmm-node-desc' // composed HTML from the server (convert_markdown_to_html)
}, {
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);
@@ -433,7 +449,6 @@ export class ModelManager {
btn.classList.add("cmm-btn-loading");
this.showError("");
- let needRefresh = false;
let errorMsg = "";
let target_items = [];
@@ -454,6 +469,8 @@ export class ModelManager {
const data = item.originalData;
data.ui_id = item.hash;
+ // Batch rejections refer to id; models need a stable request ID.
+ data.id ??= item.hash;
if(batch['install_model']) {
@@ -466,38 +483,54 @@ export class ModelManager {
this.install_context = {btn: btn, targets: target_items};
- if(errorMsg) {
- this.showError(errorMsg);
- show_message("[Installation Errors]\n"+errorMsg);
+ this.batch_id = generateUUID();
+ batch['batch_id'] = this.batch_id;
- // reset
- for(let k in target_items) {
- const item = target_items[k];
- this.grid.updateCell(item, "installed");
- }
- }
- else {
- this.batch_id = generateUUID();
- batch['batch_id'] = this.batch_id;
-
- const res = await api.fetchApi(`/v2/manager/queue/batch`, {
+ let failed;
+ try {
+ const { data, error } = await fetchData(`/v2/manager/queue/batch`, {
method: 'POST',
body: JSON.stringify(batch)
});
-
- let failed = await res.json();
-
- if(failed.length > 0) {
- for(let k in failed) {
- let hash = failed[k];
- const item = self.grid.getRowItemBy("hash", hash);
- errorMsg = `[FAIL] ${item.title}`;
- }
- }
-
- this.showStop();
- showTerminal();
+ if (error) throw error;
+ if (!Array.isArray(data?.failed)) throw new Error('Invalid batch response.');
+ failed = data.failed;
+ } catch (error) {
+ errorMsg = `Failed to submit installation request: ${sanitizeHTML(String(error))}`;
+ this.showError(errorMsg);
+ show_message("[Installation Errors]\n" + errorMsg);
+ btn.classList.remove("cmm-btn-loading");
+ this.element.querySelectorAll(".cmm-btn-loading").forEach(button => {
+ button.classList.remove("cmm-btn-loading");
+ });
+ this.hideLoading();
+ this.hideStop();
+ this.install_context = undefined;
+ return;
}
+ for (const id of failed) {
+ const item = target_items.find(item => item.originalData.id === id);
+ errorMsg += `[FAIL] ${item?.name ?? sanitizeHTML(String(id))}\n`;
+ }
+ if (errorMsg) {
+ this.showError(errorMsg);
+ show_message("[Installation Errors]\n" + errorMsg);
+ }
+
+ showTerminal();
+
+ // No queued work means the server will not send batch-done.
+ if (target_items.every(item => failed.includes(item.originalData.id))) {
+ this.element.querySelectorAll(".cmm-btn-loading").forEach(button => {
+ button.classList.remove("cmm-btn-loading");
+ });
+ this.hideLoading();
+ this.hideStop();
+ this.install_context = undefined;
+ return;
+ }
+
+ this.showStop();
}
async onQueueStatus(event) {
@@ -544,7 +577,7 @@ export class ModelManager {
let v = result[hash];
if(v != 'success')
- errorMsg += v + '\n';
+ errorMsg += sanitizeHTML(String(v)) + '\n';
}
for(let k in self.install_context.targets) {
@@ -720,22 +753,20 @@ export class ModelManager {
this.showMessage(err, "red");
}
+ // Messages contain HTML: preserve server-escaped names and escape raw values at the caller.
showMessage(msg, color) {
- if (color) {
- msg = `${msg}`;
- }
- this.element.querySelector(".cmm-manager-message").innerHTML = msg;
+ const element = this.element.querySelector(".cmm-manager-message");
+ element.style.color = color || "";
+ element.innerHTML = msg ?? "";
}
showStatus(msg, color) {
- if (color) {
- msg = `${msg}`;
- }
- this.element.querySelector(".cmm-manager-status").innerHTML = msg;
+ const element = this.element.querySelector(".cmm-manager-status");
+ element.style.color = color || "";
+ element.innerHTML = msg ?? "";
}
showLoading() {
-// this.setDisabled(true);
if (this.grid) {
this.grid.showLoading();
this.grid.showMask({
@@ -745,45 +776,12 @@ export class ModelManager {
}
hideLoading() {
-// this.setDisabled(false);
if (this.grid) {
this.grid.hideLoading();
this.grid.hideMask();
}
}
- setDisabled(disabled) {
- const $close = this.element.querySelector(".cmm-manager-close");
- const $refresh = this.element.querySelector(".cmm-manager-refresh");
- const $stop = this.element.querySelector(".cmm-manager-stop");
-
- const list = [
- ".cmm-manager-header input",
- ".cmm-manager-header select",
- ".cmm-manager-footer button",
- ".cmm-manager-selection button"
- ].map(s => {
- return Array.from(this.element.querySelectorAll(s));
- })
- .flat()
- .filter(it => {
- return it !== $close && it !== $refresh && it !== $stop;
- });
-
- list.forEach($elem => {
- if (disabled) {
- $elem.setAttribute("disabled", "disabled");
- } else {
- $elem.removeAttribute("disabled");
- }
- });
-
- Array.from(this.element.querySelectorAll(".cmm-btn-loading")).forEach($elem => {
- $elem.classList.remove("cmm-btn-loading");
- });
-
- }
-
showRefresh() {
this.element.querySelector(".cmm-manager-refresh").style.display = "block";
}
diff --git a/comfyui_manager/js/snapshot.js b/comfyui_manager/js/snapshot.js
index 6c50b3453..f537fd4a2 100644
--- a/comfyui_manager/js/snapshot.js
+++ b/comfyui_manager/js/snapshot.js
@@ -1,7 +1,7 @@
import { app } from "../../scripts/app.js";
import { api } from "../../scripts/api.js"
import { ComfyDialog, $el } from "../../scripts/ui.js";
-import { manager_instance, rebootAPI, show_message, loadCss } from "./common.js";
+import { manager_instance, rebootAPI, show_message, loadCss, sanitizeHTML } from "./common.js";
import { buildGuiFrame } from "./comfyui-gui-builder.js";
loadCss("./snapshot.css");
@@ -133,7 +133,7 @@ export class SnapshotManager extends ComfyDialog {
startRestore(target) {
const self = SnapshotManager.instance;
- self.updateMessage(`
Restore snapshot '${target.name}'`); + self.updateMessage(`
Restore snapshot '${sanitizeHTML(String(target.name ?? ''))}'`); for(let i in self.restore_buttons) { self.restore_buttons[i].disabled = true; @@ -214,7 +214,7 @@ export class SnapshotManager extends ComfyDialog { data1.style.textAlign = "center"; data1.innerHTML = i+1; var data2 = document.createElement('td'); - data2.innerHTML = ` ${data}`; + data2.textContent = `\u00a0${data}`; var data_button = document.createElement('td'); data_button.style.textAlign = "center"; data_button.className = "data-btns"; diff --git a/comfyui_manager/legacy/html_utils.py b/comfyui_manager/legacy/html_utils.py new file mode 100644 index 000000000..3bb10944b --- /dev/null +++ b/comfyui_manager/legacy/html_utils.py @@ -0,0 +1,82 @@ +"""HTML rendering helpers used only by the legacy Manager UI.""" +import re +from html import escape, unescape +from html.entities import html5 + + +SAFE_URL_SCHEMES = frozenset({'http', 'https'}) +_URL_SCHEME_NOISE = re.compile(r'[\x00-\x20\x7f]') +_URL_HEAD_DELIMITERS = ('/', '?', '#') + + +def escape_html_attribute(value): + """Escape a value for a quoted HTML attribute.""" + return escape(str(value), quote=True) + + +_HTML_ENTITY = re.compile(r'&(#[0-9]+|#[xX][0-9a-fA-F]+|[A-Za-z][A-Za-z0-9]*);') +_HTML_TEXT_TOKEN = re.compile(_HTML_ENTITY.pattern + "|[&<>\"']") + + +def unescape_html_entities(text): + """Decode complete entities once, leaving query keys such as ¬ebook intact.""" + def replace(match): + entity = match.group(1) + if entity.startswith('#'): + return unescape(match.group(0)) + return html5.get(entity + ';', match.group(0)) + + return _HTML_ENTITY.sub(replace, text) + + +def escape_html_text(value): + """Escape HTML text without double-escaping existing entities.""" + return _HTML_TEXT_TOKEN.sub( + lambda m: m.group(0) if m.group(1) else escape_html_attribute(m.group(0)), + str(value), + ) + + +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.""" + # Keep nh3 loading within the legacy notice path. + 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'}}, + ) diff --git a/comfyui_manager/legacy/manager_core.py b/comfyui_manager/legacy/manager_core.py index 6543713e0..17938bf45 100644 --- a/comfyui_manager/legacy/manager_core.py +++ b/comfyui_manager/legacy/manager_core.py @@ -33,6 +33,7 @@ from packaging import version import uuid from ..common import cm_global +from ..common.config_writer import DirtyTrackingConfig, write_config_merged from ..common import cnr_utils from ..common import manager_util from ..common import git_utils @@ -1672,39 +1673,30 @@ class ManagerFuncs: manager_funcs = ManagerFuncs() +# Settings owned by this server; other keys and sections are preserved. +WRITTEN_CONFIG_KEYS = ( + 'git_exe', + 'use_uv', + 'channel_url', + 'share_option', + 'bypass_ssl', + 'file_logging', + 'update_policy', + 'windows_selector_event_loop_policy', + 'model_download_by_agent', + 'downgrade_blacklist', + 'security_level', + 'always_lazy_install', + 'network_mode', + 'db_mode', + 'allow_git_url_install', + 'allow_pip_install', +) + + def write_config(): - config = configparser.ConfigParser(strict=False) - - config['default'] = { - 'git_exe': get_config()['git_exe'], - 'use_uv': get_config()['use_uv'], - 'channel_url': get_config()['channel_url'], - 'share_option': get_config()['share_option'], - 'bypass_ssl': get_config()['bypass_ssl'], - "file_logging": get_config()['file_logging'], - 'update_policy': get_config()['update_policy'], - 'windows_selector_event_loop_policy': get_config()['windows_selector_event_loop_policy'], - 'model_download_by_agent': get_config()['model_download_by_agent'], - 'downgrade_blacklist': get_config()['downgrade_blacklist'], - 'security_level': get_config()['security_level'], - 'always_lazy_install': get_config()['always_lazy_install'], - 'network_mode': get_config()['network_mode'], - 'db_mode': get_config()['db_mode'], - 'allow_git_url_install': get_config()['allow_git_url_install'], - 'allow_pip_install': get_config()['allow_pip_install'], - } - - # Sanitize all string values to prevent CRLF injection attacks - for key, value in config['default'].items(): - if isinstance(value, str): - config['default'][key] = value.replace('\r', '').replace('\n', '').replace('\x00', '') - - directory = os.path.dirname(context.manager_config_path) - if not os.path.exists(directory): - os.makedirs(directory) - - with open(context.manager_config_path, 'w') as configfile: - config.write(configfile) + """Persist changed settings through the shared writer.""" + write_config_merged(context.manager_config_path, get_config(), WRITTEN_CONFIG_KEYS) def read_config(): @@ -1770,7 +1762,8 @@ def get_config(): global cached_config if cached_config is None: - cached_config = read_config() + # Start tracking changes after the startup configuration is loaded. + cached_config = DirtyTrackingConfig(read_config()) if cached_config['http_channel_enabled']: print("[ComfyUI-Manager] Warning: http channel enabled, make sure server in secure env") diff --git a/comfyui_manager/legacy/manager_server.py b/comfyui_manager/legacy/manager_server.py index d5771de4e..ee3a27b3f 100644 --- a/comfyui_manager/legacy/manager_server.py +++ b/comfyui_manager/legacy/manager_server.py @@ -19,6 +19,7 @@ import asyncio from collections import deque from . import manager_core as core +from . import html_utils from ..common import manager_util from ..common import cm_global from ..common import manager_downloader @@ -37,14 +38,15 @@ logging.info("[ComfyUI-Manager] network_mode: " + network_mode_description) comfy_ui_hash = "-" comfyui_tag = None -SECURITY_MESSAGE_MIDDLE = "ERROR: To use this action, a security_level of `normal or below` is required. Please contact the administrator.\nReference: https://github.com/Comfy-Org/ComfyUI-Manager#security-policy" -SECURITY_MESSAGE_MIDDLE_P = "ERROR: To use this action, security_level must be `normal or below`, and network_mode must be set to `personal_cloud`. Please contact the administrator.\nReference: https://github.com/ltdrdata/ComfyUI-Manager#security-policy" -SECURITY_MESSAGE_HIGH_P = "ERROR: To use this action, '--listen' must be set to a local IP and security_level must be 'normal-' or lower. Please contact the administrator.\nReference: https://github.com/Comfy-Org/ComfyUI-Manager#security-policy" -SECURITY_MESSAGE_NORMAL_MINUS = "ERROR: To use this feature, you must either set '--listen' to a local IP and set the security level to 'normal-' or lower, or set the security level to 'middle' or 'weak'. Please contact the administrator.\nReference: https://github.com/Comfy-Org/ComfyUI-Manager#security-policy" -SECURITY_MESSAGE_GENERAL = "ERROR: This installation is not allowed in this security_level. Please contact the administrator.\nReference: https://github.com/Comfy-Org/ComfyUI-Manager#security-policy" -SECURITY_MESSAGE_NORMAL_MINUS_MODEL = "ERROR: Downloading models that are not in '.safetensors' format is only allowed for models registered in the 'default' channel at this security level. If you want to download this model, set the security level to 'normal-' or lower." -SECURITY_MESSAGE_FLAG_GIT_URL = "ERROR: This action requires 'allow_git_url_install = true' in config.ini ([default] section). This setting is independent of security_level. Please contact the administrator.\nReference: https://github.com/Comfy-Org/ComfyUI-Manager#security-policy" -SECURITY_MESSAGE_FLAG_PIP = "ERROR: This action requires 'allow_pip_install = true' in config.ini ([default] section). This setting is independent of security_level. Please contact the administrator.\nReference: https://github.com/Comfy-Org/ComfyUI-Manager#security-policy" +from ..common.security_messages import ( + SECURITY_MESSAGE_MIDDLE, + SECURITY_MESSAGE_MIDDLE_P, + SECURITY_MESSAGE_HIGH_P, + SECURITY_MESSAGE_GENERAL, + SECURITY_MESSAGE_NORMAL_MINUS_MODEL, +) +SECURITY_MESSAGE_FLAG_GIT_URL = "ERROR: This action requires BOTH: (1) 'allow_git_url_install = true' in config.ini ([default] section), AND (2) a network position the Manager treats as private - either ComfyUI launched with a loopback '--listen' (any 127.x.x.x address, or ::1), OR 'network_mode = personal_cloud' in config.ini. Currently listening on {listen} with network_mode = {network_mode}. Both values are read once at ComfyUI startup, so changing either one needs a restart, done with the server down: STOP ComfyUI, change the setting, then start it again. Both are independent of security_level. Please contact the administrator.\nReference: https://github.com/Comfy-Org/ComfyUI-Manager#security-policy" +SECURITY_MESSAGE_FLAG_PIP = "ERROR: This action requires BOTH: (1) 'allow_pip_install = true' in config.ini ([default] section), AND (2) a network position the Manager treats as private - either ComfyUI launched with a loopback '--listen' (any 127.x.x.x address, or ::1), OR 'network_mode = personal_cloud' in config.ini. Currently listening on {listen} with network_mode = {network_mode}. Both values are read once at ComfyUI startup, so changing either one needs a restart, done with the server down: STOP ComfyUI, change the setting, then start it again. Both are independent of security_level. Please contact the administrator.\nReference: https://github.com/Comfy-Org/ComfyUI-Manager#security-policy" routes = PromptServer.instance.routes @@ -259,7 +261,6 @@ print_comfyui_version() core.check_invalid_nodes() - def setup_environment(): git_exe = core.get_config()['git_exe'] @@ -974,14 +975,22 @@ async def _update_all(json_data): def convert_markdown_to_html(input_text): + """Parse source markdown, escaping text and URLs at their HTML boundaries.""" pattern_a = re.compile(r'\[a/([^]]+)]\(([^)]+)\)') pattern_w = re.compile(r'\[w/([^]]+)]') pattern_i = re.compile(r'\[i/([^]]+)]') pattern_bold = re.compile(r'\*\*([^*]+)\*\*') pattern_white = re.compile(r'%%([^*]+)%%') + # Format link labels separately from URLs; escape URLs when inserted. + hrefs = [] + def replace_a(match): - return f"{match.group(1)}" + # Entities written in the source URL (e.g. &) retain their meaning. + url = html_utils.unescape_html_entities(match.group(2)) + hrefs.append(html_utils.sanitize_url(url)) + text = html_utils.escape_html_text(match.group(1)) + return f"{text}" def replace_w(match): return f"
") - return result_text.replace("\n", "
") + return re.sub( + r'\x00H(\d+)\x00', + lambda m: html_utils.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']) @@ -1396,7 +1420,6 @@ async def import_fail_info_bulk(request): return web.Response(status=500, text="Internal server error") - @routes.post("/v2/manager/queue/reset") async def reset_queue(request): rejection = manager_security.reject_simple_form_post(request) @@ -1412,7 +1435,6 @@ async def reset_queue(request): return web.Response(status=200) - @routes.get("/v2/manager/queue/status") async def queue_count(request): global task_queue @@ -1493,7 +1515,8 @@ async def _install_custom_node(json_data): # Flag-deny PRESERVES today's 404 response shape at this position (R1). if risky_level == 'high+': if not _dedicated_install_allowed('allow_git_url_install'): - logging.error(SECURITY_MESSAGE_FLAG_GIT_URL) + logging.error(SECURITY_MESSAGE_FLAG_GIT_URL.format( + listen=args.listen, network_mode=core.get_config()['network_mode'])) return web.Response(status=404, text="A security error has occurred. Please check the terminal logs") elif not is_allowed_security_level(risky_level): logging.error(SECURITY_MESSAGE_GENERAL) @@ -1551,8 +1574,10 @@ async def _fix_custom_node(json_data): async def install_custom_node_git_url(request): # goal265 S-A: dedicated-flag gate, decoupled from security_level (spec §1.2). if not _dedicated_install_allowed('allow_git_url_install'): - logging.error(SECURITY_MESSAGE_FLAG_GIT_URL) - return web.Response(status=403) + logging.error(SECURITY_MESSAGE_FLAG_GIT_URL.format( + listen=args.listen, network_mode=core.get_config()['network_mode'])) + # Only the flag name goes on the wire; the full message is logged above. + return web.json_response({'reason': 'allow_git_url_install'}, status=403) url = await request.text() res = await core.gitclone_install(url) @@ -1572,8 +1597,10 @@ async def install_custom_node_git_url(request): async def install_custom_node_pip(request): # goal265 S-B: dedicated-flag gate, decoupled from security_level (spec §1.2). if not _dedicated_install_allowed('allow_pip_install'): - logging.error(SECURITY_MESSAGE_FLAG_PIP) - return web.Response(status=403) + logging.error(SECURITY_MESSAGE_FLAG_PIP.format( + listen=args.listen, network_mode=core.get_config()['network_mode'])) + # Only the flag name goes on the wire; the full message is logged above. + return web.json_response({'reason': 'allow_pip_install'}, status=403) packages = await request.text() core.pip_install(packages.split(' ')) @@ -1815,19 +1842,6 @@ async def set_channel_url(request): return web.Response(status=400, text='Invalid request') -def add_target_blank(html_text): - pattern = r'(]*)(>)'
-
- def add_target(match):
- if 'target=' not in match.group(1):
- return match.group(1) + ' target="_blank"' + match.group(2)
- return match.group(0)
-
- modified_html = re.sub(pattern, add_target, html_text)
-
- return modified_html
-
-
@routes.get("/v2/manager/notice")
async def get_notice(request):
url = "github.com"
@@ -1843,7 +1857,9 @@ async def get_notice(request):
match = pattern.search(html_content)
if match:
- markdown_content = match.group(1)
+ # Remote page content lands in innerHTML on the client:
+ # keep formatting markup only.
+ markdown_content = html_utils.sanitize_html_fragment(match.group(1))
version_tag = os.environ.get('__COMFYUI_DESKTOP_VERSION__')
if version_tag is not None:
markdown_content += f"
ComfyUI: {version_tag} [Desktop]" @@ -1857,8 +1873,6 @@ async def get_notice(request): # markdown_content += f"
()" markdown_content += f"
Manager: {core.version_str}" - markdown_content = add_target_blank(markdown_content) - try: if '__COMFYUI_DESKTOP_VERSION__' not in os.environ: if core.comfy_ui_commit_datetime == datetime(1900, 1, 1, 0, 0, 0): diff --git a/pyproject.toml b/pyproject.toml index ed8e5f72f..bcfb9e103 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -35,7 +35,9 @@ dependencies = [ "typing-extensions", "toml", "uv", - "chardet" + "chardet", + "packaging", + "nh3>=0.3.7" ] [project.optional-dependencies] diff --git a/requirements.txt b/requirements.txt index b505f2adb..433c4529d 100644 --- a/requirements.txt +++ b/requirements.txt @@ -10,3 +10,5 @@ typing-extensions toml uv chardet +packaging +nh3>=0.3.7 diff --git a/tests/e2e/test_e2e_secgate_legacy_flags.py b/tests/e2e/test_e2e_secgate_legacy_flags.py index 890137b41..4e496a1d0 100644 --- a/tests/e2e/test_e2e_secgate_legacy_flags.py +++ b/tests/e2e/test_e2e_secgate_legacy_flags.py @@ -59,6 +59,7 @@ otherwise (harness precedent: tests/e2e/test_e2e_secgate_default.py). from __future__ import annotations import configparser +import json import os import shutil import subprocess @@ -506,6 +507,94 @@ class TestFlagsOffWeakLevel: ) +# =========================================================================== +# The 403 denial reason code +# +# A denied install returns 403 with a JSON body naming the flag that denied it +# — `{"reason": "allow_git_url_install"}` or `{"reason": "allow_pip_install"}` +# — so the browser can tell the user WHICH setting to change instead of +# guessing from the endpoint it happened to call. The full +# SECURITY_MESSAGE_FLAG_* text still goes to the terminal via `logging.error`; +# only the flag name goes on the wire. +# +# Every other assertion on these endpoints tests `status_code` alone, so +# nothing else in this file can see the body at all — these rows are the only +# coverage the wire contract has. +# +# Reuses CFG-D (git=F, pip=F, sl=weak) — the config that already produces a 403 +# on both arms, so these rows add no new server boot. +# =========================================================================== + +class TestDenialReasonCode: + """The 403 body must name WHICH flag denied — and nothing more.""" + + def _denial_body(self, resp): + """Parse the 403 body as the wire contract, with the failure message + carrying what was actually received.""" + assert resp.status_code == 403, ( + f"precondition: expected 403, got {resp.status_code}" + ) + try: + return json.loads(resp.text) + except json.JSONDecodeError as exc: + pytest.fail( + "the 403 body is not JSON. Expected a JSON object carrying " + "the denial reason code; " + f"observed {resp.text!r} (len={len(resp.text)}). " + f"JSONDecodeError: {exc}. A 403 emitted with no `text=` leaves " + "the body EMPTY, and the client then has nothing to derive its " + "message from." + ) + + def test_t9_pip_denial_body_names_the_flag(self, comfyui_flags_d): + """The pip 403 body carries the reason code. + + Expected: a JSON OBJECT whose `reason` is the string + `allow_pip_install`. + """ + resp = _post_pip("text-unidecode") + body = self._denial_body(resp) + + assert isinstance(body, dict), ( + "the 403 body must be a JSON OBJECT (the client's guard is " + "`typeof data === 'object' && data !== null && typeof data.reason " + f"=== 'string'`); observed {type(body).__name__} {body!r}" + ) + assert body.get("reason") == "allow_pip_install", ( + "expected the pip 403 body to carry " + "{'reason': 'allow_pip_install'} so the client can name the " + f"condition the SERVER actually failed on; observed {body!r}" + ) + + def test_t9_git_url_denial_body_names_the_flag(self, comfyui_flags_d): + """The git_url 403 body carries its own code. + + A separate node from the pip arm on purpose: the two emissions are + separate `return` statements, so one can be changed while the other is + missed. + """ + resp = _post_git_url(UNKNOWN_GIT_URL) + body = self._denial_body(resp) + + assert isinstance(body, dict), ( + "the 403 body must be a JSON OBJECT; observed " + f"{type(body).__name__} {body!r}" + ) + assert body.get("reason") == "allow_git_url_install", ( + "expected the git_url 403 body to carry " + f"{{'reason': 'allow_git_url_install'}}; observed {body!r}" + ) + + def test_t9b_denial_body_discloses_nothing_further(self, comfyui_flags_d): + """Denied requests expose only the failed flag name.""" + responses = { + "allow_pip_install": _post_pip("text-unidecode"), + "allow_git_url_install": _post_git_url(UNKNOWN_GIT_URL), + } + for reason, response in responses.items(): + assert self._denial_body(response) == {"reason": reason} + + # =========================================================================== # CFG-E: git=T, pip=T, sl=weak, nm=public — Q1 guard # =========================================================================== diff --git a/tests/legacy_html_testutil.py b/tests/legacy_html_testutil.py new file mode 100644 index 000000000..c582e078b --- /dev/null +++ b/tests/legacy_html_testutil.py @@ -0,0 +1,36 @@ +"""Execute the legacy renderer without starting ComfyUI or its network threads.""" +import ast +import importlib.util +import json +import re +import sys +import types +from pathlib import Path + +ROOT = Path(__file__).resolve().parent.parent +PACKAGE = ROOT / 'comfyui_manager' + + +def load_functions(path, names, namespace=None): + namespace = {} if namespace is None else namespace + nodes = [node for node in ast.parse(path.read_text(encoding='utf-8')).body + if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)) and node.name in names] + assert len(nodes) == len(names) + exec(compile(ast.Module(nodes, []), str(path), 'exec'), namespace) + return namespace + + +def load_renderer(): + path = PACKAGE / 'legacy/html_utils.py' + spec = importlib.util.spec_from_file_location('_legacy_html_utils', path) + helpers = importlib.util.module_from_spec(spec) + spec.loader.exec_module(helpers) + util = load_functions(PACKAGE / 'common/manager_util.py', {'sanitize_tag'}) + return load_functions(PACKAGE / 'legacy/manager_server.py', {'convert_markdown_to_html', 'populate_markdown'}, + {'re': re, 'html_utils': helpers, 'manager_util': types.SimpleNamespace(**util)}) + + +if __name__ == '__main__': + item = json.load(sys.stdin) + load_renderer()['populate_markdown'](item) + print(json.dumps(item)) diff --git a/tests/playwright/README.md b/tests/playwright/README.md index af57c61cc..df13f740b 100644 --- a/tests/playwright/README.md +++ b/tests/playwright/README.md @@ -5,20 +5,25 @@ Browser-based E2E tests for the ComfyUI-Manager legacy UI. ## Prerequisites 1. **E2E environment** built via `python tests/e2e/scripts/setup_e2e_env.py` -2. **Playwright installed**: `npx playwright install chromium` +2. **Playwright runner and Chromium** (from the repository root): + `npm install --no-save --package-lock=false @playwright/test@1.59.1`, then + `npx playwright install chromium`. 3. **ComfyUI running** with legacy UI enabled: ```bash E2E_ROOT=/tmp/e2e_full_test PORT=8199 -$E2E_ROOT/venv/bin/python $E2E_ROOT/comfyui/main.py \ +PYTHONPATH="$PWD" $E2E_ROOT/venv/bin/python $E2E_ROOT/comfyui/main.py \ --listen 127.0.0.1 --port $PORT \ - --enable-manager-legacy-ui \ + --enable-manager --enable-manager-legacy-ui \ --cpu ``` ## Running Tests +Run from the repository root. XSS fixtures execute the checkout's Python renderer +using `python3`; set `PYTHON=/path/to/python` to select another interpreter. + ```bash # With server already running: PORT=8199 npx playwright test @@ -42,3 +47,6 @@ PORT=8199 npx playwright test --debug | `legacy-ui-model-manager.spec.ts` | Model list grid, filter, search | | `legacy-ui-snapshot.spec.ts` | Snapshot list, save, remove | | `legacy-ui-navigation.spec.ts` | Dialog open/close, nested navigation, no duplicates | +| `custom-nodes-xss.spec.ts` | Node metadata, Markdown, links, errors, search highlighting | +| `model-manager-xss.spec.ts` | Model metadata, links, errors, search highlighting | +| `legacy-message-errors.spec.ts` | Batch rejection IDs, name readability, DOM colors, lookup errors | diff --git a/tests/playwright/custom-nodes-xss.spec.ts b/tests/playwright/custom-nodes-xss.spec.ts new file mode 100644 index 000000000..fbc59fb99 --- /dev/null +++ b/tests/playwright/custom-nodes-xss.spec.ts @@ -0,0 +1,796 @@ +/** + * Browser-level XSS regression suite for the Custom Nodes manager UI. + * + * Every row feeds hostile node-pack metadata through a stubbed registry + * response and asserts the value renders as INERT TEXT: the payload is visible + * as literal characters, no element is constructed from it, and its `onerror` + * never runs. The three checks are made together because they fail + * differently — see `expectInert` in ./helpers.ts. + * + * NOT EVERY ROW CLOSES A LIVE HOLE, and the distinction matters when reading a + * failure. `version`/`cnr_latest`, `author`, `stars` and `last_update` are + * served unsanitized and sit in always-visible columns, so opening the manager + * is the whole exploit; so are node names from `/v2/customnode/getmappings`, + * a route that sanitizes nothing. Those fields are escaped ON THE CLIENT and + * their rows target that escape. + * + * `title` / `name` / `description` are different: `populate_markdown` -> + * `sanitize_tag` escapes them SERVER-SIDE, and the client renders them as-is + * on purpose. Escaping them again is not free safety — `sanitizeHTML` also maps + * `&`, so a second pass turns the server's `<` into `<` and the pack + * `ComfyUI-` reaches the user as literal `ComfyUI-<Impact-Pack>`.
+ * The rows targeting those fields assert the SERVER->CLIENT CHAIN, not an
+ * independent client guarantee.
+ *
+ * CAVEAT: `page.route()` REPLACES the HTTP response, so every fixture here
+ * bypasses `populate_markdown`. For the raw fields that is exactly what a live
+ * server delivers. For the server-escaped fields it is NOT, so those rows pass
+ * their payload through `serverSanitizeTag` first — the real transform, read
+ * out of `manager_util.py` — and assert on the wire form the server would
+ * actually have emitted.
+ *
+ * Requires ComfyUI running with --enable-manager-legacy-ui on PORT (see
+ * tests/playwright/README.md).
+ */
+
+import { test, expect } from '@playwright/test';
+import {
+ waitForComfyUI,
+ openManagerMenu,
+ openCustomNodesManager,
+ routeNodeList,
+ routeMappings,
+ routeAlternatives,
+ makePack,
+ expectInert,
+ serverSanitizeTag,
+ serverPopulateMarkdown,
+ serverInstallDenialRestartClause,
+ XSS_PAYLOAD,
+ XSS_PAYLOAD_NOSPACE,
+} from './helpers';
+
+const GRID = '.cn-manager-grid';
+const FLYOVER = '.cn-flyover';
+
+/** Collect page errors so the rows that assert "did not throw" can see them. */
+function trackPageErrors(page: import('@playwright/test').Page) {
+ const errors: string[] = [];
+ page.on('pageerror', (e) => errors.push(String(e)));
+ return errors;
+}
+
+test.describe('grid render sinks', () => {
+ test.beforeEach(async ({ page }) => {
+ await page.goto('/');
+ await waitForComfyUI(page);
+ });
+
+ test('hostile title renders inert through the server escape', async ({ page }) => {
+ // NOT defence-in-depth, and the fixture is what says so. `title` is in the
+ // populate_markdown set, so the SERVER escape is the one defence and the
+ // client deliberately renders the value as-is — escaping it a second time
+ // printed `<` to every user (see the double-escape row below).
+ //
+ // The fixture is therefore the server's own sanitize_tag OUTPUT, lifted
+ // from manager_util.py. A raw payload here would assert the opposite of the
+ // design and fail by construction, on a wire form the live server cannot
+ // emit; what this row asserts is that the form it DOES emit stays inert
+ // once the client renders it.
+ await routeNodeList(page, {
+ hostile: makePack('hostile', { title: serverSanitizeTag(XSS_PAYLOAD) }),
+ });
+ await openManagerMenu(page);
+ await openCustomNodesManager(page);
+
+ await expectInert(page, GRID, XSS_PAYLOAD, 'img', 'title');
+ });
+
+ // This row needs a WIDER VIEWPORT THAN THE REST, so it gets its own describe.
+ //
+ // turbogrid virtualises columns horizontally: at the config's default
+ // viewport the `.cn-pack-stars` and `.cn-pack-last-update` BODY CELLS are
+ // not in the DOM at all — a dump returned `stars: []`, `lastUpdate: []`
+ // with `rowCount: 4`, while the HEADER showed both columns. The row's
+ // `last_update` leg then failed leg 1 (presence) with legs 2 and 3 passing,
+ // which is expectInert's own signature for "the payload never rendered",
+ // i.e. the row was reporting the viewport, not the sink. Measured: at
+ // 2600x1000 both cells render and both are inert. Pin the viewport so this
+ // row asserts on the sink it names.
+ test.describe('wide viewport — stars/last_update are column-virtualised', () => {
+ test.use({ viewport: { width: 2600, height: 1000 } });
+
+ test('hostile version/author/stars/last_update render inert (LIVE no-user-action)', async ({ page }) => {
+ // The four fields that carry the no-user-action property: served
+ // unsanitized AND in always-visible columns, so opening the manager is
+ // the whole exploit.
+ await routeNodeList(page, {
+ p1: makePack('p1', { version: XSS_PAYLOAD, cnr_latest: XSS_PAYLOAD }),
+ p2: makePack('p2', { author: XSS_PAYLOAD }),
+ // `stars` must be a NON-number to reach the raw fall-through
+ // (`if (typeof stars === 'number') return stars.toLocaleString()`).
+ p3: makePack('p3', { stars: XSS_PAYLOAD }),
+ // `last_update` is cut at the first space, so the payload must be
+ // space-free or the row would pass for the wrong reason.
+ p4: makePack('p4', { last_update: XSS_PAYLOAD_NOSPACE }),
+ });
+ await openManagerMenu(page);
+ await openCustomNodesManager(page);
+
+ // PRECONDITION — the two virtualised columns must actually have body
+ // cells before their legs mean anything. Without this the row can go
+ // pass again the moment a layout change hides them, on a fixture that
+ // proved nothing (the same vacuous-pass shape the tags row guards against).
+ await expect(
+ page.locator(`${GRID} .cn-pack-stars`).first(),
+ 'precondition: no .cn-pack-stars body cell rendered — the stars column is virtualised away at this viewport, so its leg would pass vacuously.',
+ ).toBeVisible({ timeout: 15_000 });
+ await expect(
+ page.locator(`${GRID} .cn-pack-last-update`).first(),
+ 'precondition: no .cn-pack-last-update body cell rendered — the last_update column is virtualised away at this viewport, so its leg would pass vacuously.',
+ ).toBeVisible({ timeout: 15_000 });
+
+ await expectInert(page, GRID, XSS_PAYLOAD, 'img', 'version/cnr_latest');
+ await expectInert(page, GRID, XSS_PAYLOAD, 'img', 'author');
+ await expectInert(page, GRID, XSS_PAYLOAD_NOSPACE, 'img', 'last_update');
+ });
+ });
+
+ test('hostile tags render inert once the alternatives column is shown (LIVE, gated)', async ({ page }) => {
+ // The alternatives column is `invisible: !this.hasAlternatives()`, so this
+ // row MUST establish the gate. Without the precondition assertion below it
+ // passes vacuously against a grid that never rendered the column at all.
+ await routeNodeList(page, { alt1: makePack('alt1') });
+ await routeAlternatives(page, {
+ alt1: {
+ id: 'alt1',
+ title: 'alt1',
+ description: 'alternative description',
+ tags: XSS_PAYLOAD,
+ },
+ });
+ await openManagerMenu(page);
+ await openCustomNodesManager(page);
+
+ // ESTABLISH THE GATE. `hasAlternatives()` is `this.filter ===
+ // ShowMode.ALTERNATIVES`, so the column exists ONLY while the filter
+ // dropdown is on "Alternatives of A1111" — selecting it is also what
+ // triggers getAlternatives() and therefore the `:1903` tag loop. Without
+ // this step the fixture is never fetched, nothing renders, and the row
+ // would report a failure that has nothing to do with the sink.
+ const filterSelect = page.locator('select.cn-manager-filter').first();
+ await expect(filterSelect).toBeVisible({ timeout: 10_000 });
+ await filterSelect.selectOption({ label: 'Alternatives of A1111' });
+
+ // PRECONDITION — assert on `.cn-tag-list` SPECIFICALLY, the div the tag
+ // loop emits. An earlier draft accepted `.cn-pack-desc` too, which the
+ // description column also carries: it matched, the row failed on a
+ // different leg, and it looked like a reproduction. That is the vacuous
+ // vacuous pass this row has to guard against, arriving as a vacuous
+ // FAILURE instead.
+ await expect(
+ page.locator(`${GRID} .cn-tag-list`).first(),
+ 'precondition: no .cn-tag-list rendered, so the alternatives tag sink was never exercised — the row proves nothing about the sink.',
+ ).toBeVisible({ timeout: 15_000 });
+
+ await expectInert(page, GRID, XSS_PAYLOAD, 'img', 'tags');
+ });
+
+ test('a server-escaped title renders decoded (no double-escape)', async ({ page }) => {
+ // THE REGRESSION GUARD FOR THE DOUBLE-ESCAPE FIX, and it only guards
+ // anything because the fixture is the WIRE form.
+ //
+ // The earlier shape of this row fed the RAW authored title straight to the
+ // grid. That is input the live server cannot deliver — getlist runs every
+ // pack through populate_markdown -> sanitize_tag first — and the row passed
+ // either way: escaped once, `<3` comes back as `<3`; not escaped at
+ // all, `<3` is not a tag start so it renders as text too. It asserted
+ // nothing about the chain it was named for.
+ //
+ // Now the fixture is the OUTPUT of the server's own sanitize_tag, read out
+ // of manager_util.py at run time (serverSanitizeTag) rather than re-typed,
+ // so what enters the client is exactly what the server would have emitted:
+ // wire form in, authored text out. A client that escapes the field a second
+ // time prints the entity and BOTH legs below flip.
+ //
+ // page.route still carries it: the escaped fields originate in third-party
+ // registry data (no live pack title contains an angle bracket), so this is
+ // the only deterministic way to place a known value in the grid. What
+ // page.route replaces is the TRANSPORT — the transform is the real one.
+ const AUTHORED = 'Tom & Jerry <3';
+ const ON_THE_WIRE = serverSanitizeTag(AUTHORED);
+ expect(
+ ON_THE_WIRE,
+ 'serverSanitizeTag returned its input unchanged, so the fixture is NOT the ' +
+ 'wire form and this row is back to proving nothing. Check that sanitize_tag ' +
+ 'in comfyui_manager/common/manager_util.py still escapes < and >.',
+ ).not.toBe(AUTHORED);
+
+ await routeNodeList(page, { legit: makePack('legit', { title: ON_THE_WIRE }) });
+ await openManagerMenu(page);
+ await openCustomNodesManager(page);
+
+ const text = (await page.locator(GRID).first().textContent()) ?? '';
+ const diagnostic =
+ `authored: ${JSON.stringify(AUTHORED)}\n` +
+ `on the wire (server sanitize_tag output): ${JSON.stringify(ON_THE_WIRE)}\n` +
+ `grid text (first 300 chars): ${JSON.stringify(text.slice(0, 300))}`;
+
+ expect(
+ text,
+ `the server-escaped title did not render as the author wrote it. The client ` +
+ `escaped an ALREADY-escaped value, so sanitizeHTML mapped the server's & to ` +
+ `& and the entity reached the user as literal text.\n${diagnostic}`,
+ ).toContain(AUTHORED);
+ expect(
+ text,
+ `the wire form is visible in the grid as literal text — that IS the ` +
+ `double-escape. A field in the populate_markdown set (title/name/description) ` +
+ `must reach innerHTML AS-IS.\n${diagnostic}`,
+ ).not.toContain(ON_THE_WIRE);
+ });
+
+ test('missing title/version/author renders without throwing', async ({ page }) => {
+ // Expected to PASS — `sanitizeHTML(undefined)` raises TypeError inside a
+ // render path, so the escape MUST be sanitizeHTML(String(v)).
+ // This row is what turns that mistake into a visible failure.
+ const errors = trackPageErrors(page);
+ const bare = makePack('bare');
+ delete (bare as Record).title;
+ delete (bare as Record).version;
+ delete (bare as Record).author;
+ await routeNodeList(page, { bare });
+ await openManagerMenu(page);
+ await openCustomNodesManager(page);
+
+ await expect(page.locator(GRID).first()).toBeVisible();
+ expect(
+ errors,
+ `the grid threw while rendering a row with absent title/version/author — sanitizeHTML(undefined) raises TypeError, so the escape must pass String(v). pageerrors: ${errors.join(' | ')}`,
+ ).toEqual([]);
+ });
+
+ test('composed-HTML description keeps rendering as markup (anti-regression)', async ({ page }) => {
+ // MANDATORY anti-regression. `description` is SERVER-COMPOSED HTML; the
+ // cheapest way to pass every other row here is to blind-escape the grid,
+ // which would print literal / to every user. This row
+ // guards the ESCAPING, not the defect — it is expected to pass both before
+ // and after, and must never be forced to fail.
+ const composed = serverPopulateMarkdown({ description: 'See [a/the docs](https://example.com/x) for **details**' }).description;
+ await routeNodeList(page, { doc: makePack('doc', { description: composed }) });
+ await openManagerMenu(page);
+ await openCustomNodesManager(page);
+
+ const anchors = await page.locator(`${GRID} a[href="https://example.com/x"]`).count();
+ expect(
+ anchors,
+ 'the composed-HTML description no longer renders as markup — a working is gone, so the description column was blind-escaped. That is a regression, not a fix.',
+ ).toBeGreaterThan(0);
+
+ const bold = await page.locator(`${GRID} b, ${GRID} B`).count();
+ expect(
+ bold,
+ 'the composed-HTML description lost its markup — the column was escaped wholesale.',
+ ).toBeGreaterThan(0);
+ });
+});
+
+test.describe('URL / scheme sink', () => {
+ test.beforeEach(async ({ page }) => {
+ await page.goto('/');
+ await waitForComfyUI(page);
+ });
+
+ test('the title link href carries no javascript: scheme (LIVE, click-gated)', async ({ page }) => {
+ // `link.href = ` with no scheme allow-list. This is the
+ // client-side twin of the server composer's scheme-trust hole.
+ //
+ // Asserted on the href PROPERTY, not on rendered text: the
+ // rendered label is the pack title and would look perfectly innocent while
+ // the anchor still carries a javascript: target.
+ await routeNodeList(page, {
+ evil: makePack('evil', {
+ repository: 'javascript:window.__xss_fired=1',
+ reference: 'javascript:window.__xss_fired=1',
+ }),
+ });
+ await openManagerMenu(page);
+ await openCustomNodesManager(page);
+
+ const hrefs = await page.locator(`${GRID} a`).evaluateAll(
+ (els) => els.map((e) => (e as HTMLAnchorElement).href),
+ );
+
+ // PRECONDITION — the row asserts that no anchor carries a bad scheme, and
+ // a grid with NO anchors satisfies that having tested nothing. Pin that at
+ // least one anchor rendered before reading anything into the result below.
+ expect(
+ hrefs.length,
+ `precondition: the grid rendered no anchors at all, so the href assertion below would pass over an empty list and prove nothing about the URL sink. Observed ${hrefs.length} anchors.`,
+ ).toBeGreaterThan(0);
+
+ // ASSERTED AGAINST THE ALLOW-LIST, not against a list of dangerous schemes.
+ const allowed = ['http:', 'https:'];
+ const offending = hrefs.filter((h) => {
+ const scheme = new URL(h).protocol;
+ return scheme !== null && !allowed.includes(scheme);
+ });
+ expect(
+ offending,
+ `the title link's href PROPERTY carries a scheme outside the allow-list — a ` +
+ `registry-controlled URL reached href unfiltered, so one click executes it. ` +
+ `sanitizeUrl admits ${JSON.stringify(allowed)}; observed anchors: ` +
+ `${JSON.stringify(hrefs.slice(0, 10))}`,
+ ).toEqual([]);
+ });
+});
+
+test.describe('flyover sinks', () => {
+ test.beforeEach(async ({ page }) => {
+ await page.goto('/');
+ await waitForComfyUI(page);
+ });
+
+ /** Open the node flyover by clicking the Nodes cell of the first row.
+ *
+ * Targets `.cn-pack-nodes` specifically — that div is emitted ONLY by the
+ * `nodes` column formatter, and only when `rowItem.nodes` is truthy. The
+ * grid's onClick handler routes to `showNodes` on `columnItem.id ===
+ * "nodes"`, so any looser selector (e.g. "a cell containing a digit") lands
+ * on the ID column, silently never opens the flyover, and turns the row
+ * into a timeout that LOOKS like a failed assertion. Waiting for this
+ * element first therefore doubles as a precondition that the getmappings
+ * fixture actually produced a node count. */
+ async function openFlyover(page: import('@playwright/test').Page) {
+ const nodesCell = page.locator(`${GRID} .cn-pack-nodes`).first();
+ await expect(
+ nodesCell,
+ 'flyover precondition: no .cn-pack-nodes cell rendered, so the getmappings fixture did not attach a node count to the pack — the flyover cannot open and the row would time out for a fixture reason, not a defect.',
+ ).toBeVisible({ timeout: 15_000 });
+ await nodesCell.click();
+ await page.waitForSelector(FLYOVER, { state: 'visible', timeout: 10_000 });
+ }
+
+ test('flyover pack title renders inert through the server escape', async ({ page }) => {
+ // Same contract as the grid title row: `title` is server-escaped, the
+ // client renders it as-is, so the fixture must be the WIRE form.
+ await routeNodeList(page, { h: makePack('h', { title: serverSanitizeTag(XSS_PAYLOAD) }) });
+ await routeMappings(page, {
+ 'https://github.com/example/h': [['NodeA'], { title_aux: 'h' }],
+ });
+ await openManagerMenu(page);
+ await openCustomNodesManager(page);
+ await openFlyover(page);
+
+ await expectInert(page, FLYOVER, XSS_PAYLOAD, 'img', 'flyover title');
+ });
+
+ test('flyover node NAME renders inert (LIVE, click-gated)', async ({ page }) => {
+ // `/v2/customnode/getmappings` performs NO sanitization —
+ // `populate_markdown` is never called on that route — so
+ // this node name is genuinely RAW, not defence-in-depth.
+ await routeNodeList(page, { h: makePack('h') });
+ await routeMappings(page, {
+ 'https://github.com/example/h': [[XSS_PAYLOAD], { title_aux: 'h' }],
+ });
+ await openManagerMenu(page);
+ await openCustomNodesManager(page);
+ await openFlyover(page);
+
+ await expectInert(page, FLYOVER, XSS_PAYLOAD, 'img', 'flyover node name');
+ });
+});
+
+// ===========================================================================
+// selection bar
+// ===========================================================================
+
+test.describe('selection bar', () => {
+ test.beforeEach(async ({ page }) => {
+ await page.goto('/');
+ await waitForComfyUI(page);
+ });
+
+ for (const state of ['not-a-known-install-group', 'constructor', '__proto__']) {
+ test(`an unrecognised pack state still renders the selection bar: ${state}`, async ({ page }) => {
+ const errors = trackPageErrors(page);
+ await routeNodeList(page, { odd: makePack('odd', { state }) });
+ await openManagerMenu(page);
+ await openCustomNodesManager(page);
+
+ const checkbox = page.locator(`${GRID} .tg-body .tg-row .tg-checkbox`).first();
+ await expect(checkbox).toBeVisible({ timeout: 15_000 });
+ await checkbox.click();
+
+ const bar = page.locator('.cn-manager-selection').first();
+ await expect(bar).toContainText('Selected');
+ await expect(bar).toContainText(state);
+ await expect(bar.locator('button')).toHaveCount(0);
+ expect(errors).toEqual([]);
+ });
+ }
+});
+
+// ===========================================================================
+// Install status uses the same server-composed names as the grid.
+// ===========================================================================
+
+test.describe('install status sink', () => {
+ for (const [source, visible] of [
+ [XSS_PAYLOAD, XSS_PAYLOAD],
+ ['Pack & Friends ', 'Pack & Friends '],
+ ['Pack <Flux>', 'Pack <Flux>'],
+ ]) {
+ test(`server title remains readable and inert: ${source}`, async ({ page }) => {
+ await page.goto('/');
+ await waitForComfyUI(page);
+ await routeNodeList(page, { victim: makePack('victim', {
+ title: serverSanitizeTag(source), version: 'unknown',
+ }) });
+ await page.route('**/v2/manager/queue/batch', route => route.fulfill({
+ status: 200, contentType: 'application/json', body: '{"failed": []}',
+ }));
+ await openManagerMenu(page);
+ await openCustomNodesManager(page);
+ await page.locator(`${GRID} .cn-btn-install`).first().click();
+ await expectInert(page, '.cn-manager-status', visible, 'img', 'install title');
+ const status = page.locator('.cn-manager-status');
+ await expect(status).toContainText(visible);
+ });
+ }
+});
+
+// ===========================================================================
+// batch error dialog
+// ===========================================================================
+
+test.describe('batch error dialog', () => {
+ test.beforeEach(async ({ page }) => {
+ await page.goto('/');
+ await waitForComfyUI(page);
+ });
+
+ test('server-authored batch failure text renders inert (LIVE)', async ({ page }) => {
+ // REAL CONSUMPTION PATH, not a synthetic fixture. The chain exercised here
+ // is the production one end to end on the client side:
+ // real Install click -> real POST /v2/manager/queue/batch
+ // -> real `cm-queue-status` batch-done event on ComfyUI's api
+ // -> real onQueueStatus -> onQueueCompleted -> errorMsg += v (:1604)
+ // -> real showError (:1613) -> showMessage -> .cn-manager-message
+ // .innerHTML (:2091)
+ // Only the SERVER's two responses are stubbed, which is exactly what
+ // Tier A is for — the attacker-controlled value enters at `result[hash]`,
+ // the same slot a real queue result fills.
+ let capturedBatchId = '';
+ await page.route('**/v2/manager/queue/batch', async (route) => {
+ capturedBatchId = route.request().postDataJSON()?.batch_id ?? '';
+ await route.fulfill({ status: 200, contentType: 'application/json', body: '{"failed": []}' });
+ });
+ // `version: 'unknown'` is load-bearing, not filler. The grid click handler
+ // routes install through a VERSION SELECTOR dialog when
+ // `item.originalData.version != 'unknown'` (:555), and the batch POST only
+ // happens after a version is chosen. The unknown-version shape takes the
+ // direct `installNodes` path (:559), which is the same branch the Python
+ // e2e rows drive, and keeps this test on the error-render path it is about
+ // instead of on version-picker UI.
+ await routeNodeList(page, {
+ victim: makePack('victim', { state: 'not-installed', version: 'unknown' }),
+ });
+
+ await openManagerMenu(page);
+ await openCustomNodesManager(page);
+
+ const installBtn = page.locator(`${GRID} .cn-btn-install`).first();
+ await expect(
+ installBtn,
+ 'precondition: no .cn-btn-install rendered, so the batch flow was never entered and install_context/batch_id were never set — the queue event would be dropped by onQueueCompleted and the row would prove nothing.',
+ ).toBeVisible({ timeout: 15_000 });
+ await installBtn.click();
+
+ // CAPTURE THE SINK'S WRITES, because the settled element cannot be read.
+ //
+ // onQueueCompleted writes the error into .cn-manager-message via
+ // showError (:1613) and then OVERWRITES the same element with the restart
+ // notice via showMessage (:1620) — inside ONE synchronous handler. So by
+ // the time any assertion runs, the element holds "To apply the
+ // installed/updated... restart ComfyUI", and the value under test is
+ // gone. A MutationObserver does not help: its callback is a microtask and
+ // both writes land before it runs (measured — it reports only the final
+ // state). Wrapping the innerHTML SETTER records each write at the moment
+ // it happens, which is the only way to assert on what the sink actually
+ // received. Installed here rather than via addInitScript because the
+ // writes under test all happen after this point.
+ await page.evaluate(() => {
+ const w = window as unknown as Record;
+ w.__cnMessageWrites = [];
+ const desc = Object.getOwnPropertyDescriptor(Element.prototype, 'innerHTML')!;
+ Object.defineProperty(Element.prototype, 'innerHTML', {
+ configurable: true,
+ get() {
+ return desc.get!.call(this);
+ },
+ set(value) {
+ try {
+ if ((this as Element).classList?.contains('cn-manager-message')) {
+ w.__cnMessageWrites.push(String(value));
+ }
+ } catch {
+ /* never let the probe break the page under test */
+ }
+ desc.set!.call(this, value);
+ },
+ });
+ });
+
+ await expect
+ .poll(() => capturedBatchId, {
+ timeout: 15_000,
+ message: 'precondition: the client never POSTed /v2/manager/queue/batch, so no batch_id exists to address the completion event to.',
+ })
+ .not.toBe('');
+
+ // Deliver the completion event exactly as the server does over the socket.
+ await page.evaluate(
+ ({ batchId, payload }) => {
+ const api = (window as unknown as Record).comfyAPI.api.api;
+ api.dispatchEvent(
+ new CustomEvent('cm-queue-status', {
+ detail: { status: 'batch-done', batch_id: batchId, nodepack_result: [payload] },
+ }),
+ );
+ },
+ { batchId: capturedBatchId, payload: XSS_PAYLOAD },
+ );
+
+ // The write carrying the payload must arrive, and arrive inert. The three
+ // legs are the same ones expectInert applies, evaluated against the value
+ // the sink RECEIVED rather than against the element's settled content:
+ // leg 1 presence — the payload is there as literal text;
+ // leg 2 structure — parsing that write constructs no ![]()
;
+ // leg 3 execution — window.__xss_fired never set (document-wide).
+ const sink = await page.evaluate((payload) => {
+ const w = window as unknown as Record;
+ const writes: string[] = w.__cnMessageWrites ?? [];
+ // Parse each write the way the browser did, in a detached element.
+ const probe = document.createElement('div');
+ const parsed = writes.map((html) => {
+ probe.innerHTML = html;
+ return { html, text: probe.textContent ?? '', imgs: probe.querySelectorAll('img').length };
+ });
+ return {
+ writes,
+ carrying: parsed.filter((p) => p.text.includes(payload)),
+ imgsAnywhere: parsed.reduce((n, p) => n + p.imgs, 0),
+ firedGlobal: w.__xss_fired,
+ liveImgs: document.querySelectorAll('img[src="x"]').length,
+ };
+ }, XSS_PAYLOAD);
+
+ const diagnostic =
+ `queue failure text — writes observed at .cn-manager-message: ${JSON.stringify(sink.writes)}`;
+
+ expect(
+ sink.carrying.length,
+ `precondition: no write to .cn-manager-message carried the payload at all, so the sink was never exercised and the row proves nothing — check that onQueueCompleted reached showError. ${diagnostic}`,
+ ).toBeGreaterThan(0);
+ expect(
+ sink.imgsAnywhere,
+ `a write to .cn-manager-message parsed into a live ![]()
— the server-authored queue text reached innerHTML AS MARKUP. ${diagnostic}`,
+ ).toBe(0);
+ expect(
+ sink.liveImgs,
+ `an 
from the payload is live in the document. ${diagnostic}`,
+ ).toBe(0);
+ expect(
+ sink.firedGlobal,
+ `the payload EXECUTED (window.__xss_fired is set). ${diagnostic}`,
+ ).toBeUndefined();
+ });
+});
+
+// ===========================================================================
+// client-side 403 denial text
+// ===========================================================================
+
+// The selectors below are load-bearing and were arrived at the hard way.
+// `.p-dialog input` matches the Custom Nodes SEARCH box
+// (`.cn-manager-keywords`, in `.p-dialog-header`) before it matches the prompt
+// field, so a `.first()` on it silently types the URL into the search box:
+// install_via_git_url is never called, the denial modal never renders, and the
+// row fails as though the client produced nothing. Targeting
+// `.prompt-dialog-content input` hits the prompt on the first attempt.
+//
+// The general lesson: a selector that matches SOMETHING makes a broken fixture
+// look like a product failure, and only checking WHAT it matched tells the two
+// apart.
+test.describe('403 denial text', () => {
+ test.beforeEach(async ({ page }) => {
+ await page.goto('/');
+ await waitForComfyUI(page);
+ await routeNodeList(page, { p: makePack('p') });
+ });
+
+ /** Drive the REAL "Install via Git URL" entry point: footer button ->
+ * customPrompt (ComfyUI's PrimeVue dialog) -> install_via_git_url ->
+ * the 403 arm at common.js:254. */
+ async function installViaGitUrl(page: import('@playwright/test').Page, url: string) {
+ // The REAL entry point, driven exactly as a user does it: footer button ->
+ // customPrompt (common.js:149 -> app.extensionManager.dialog.prompt) ->
+ // install_via_git_url -> the 403 arm at common.js:254.
+ //
+ // `.prompt-dialog-content input` is the load-bearing selector. A broader
+ // `.p-dialog input` matches the Custom Nodes SEARCH box first, which is
+ // silent: the URL lands in the search field, no request is made, and the
+ // row fails at the denial assertion as though the client rendered nothing.
+ await page.locator('.cn-manager-install-url').first().click();
+ const input = page.locator('.prompt-dialog-content input').first();
+ await expect(
+ input,
+ 'precondition: the customPrompt dialog never rendered its input, so install_via_git_url was never called and the 403 arm was never reached — the row would prove nothing.',
+ ).toBeVisible({ timeout: 10_000 });
+ await input.fill(url);
+ await input.press('Enter');
+ }
+
+ /** The denial modal is `app.ui.dialog.element` (`show_message` ->
+ * `app.ui.dialog.show`, common.js:98). Several `.comfy-modal` nodes exist,
+ * so filter by the denial copy rather than taking `.first()`. */
+ function denialDialog(page: import('@playwright/test').Page) {
+ return page.locator('.comfy-modal').filter({ hasText: /config\.ini/ }).first();
+ }
+
+ const MALFORMED = [
+ { label: 'empty body', body: '' },
+ { label: 'non-JSON', body: 'not json' },
+ { label: 'JSON null', body: 'null' },
+ { label: 'JSON array', body: '[]' },
+ { label: 'reason is a number', body: '{"reason": 42}' },
+ { label: 'unknown reason code', body: '{"reason": "unknown_flag"}' },
+ ];
+
+ for (const variant of MALFORMED) {
+ test(`403 with ${variant.label} falls back without throwing`, async ({ page }) => {
+ // The client PARSES the 403 body to learn which condition failed, and
+ // `res.json()` resolves for far more than an object: `null`, `[]`,
+ // `"str"` and `3` all parse cleanly. `null.reason` would throw inside the
+ // render path, while `[].reason` and `{"reason":42}` yield `undefined`
+ // and would put the literal string "undefined" in front of the user.
+ // `install_denial_message` guards each of those shapes and falls back to
+ // the calling endpoint's own condition; this row is what holds that
+ // guard in place, one variant per malformed shape.
+ const errors: string[] = [];
+ page.on('pageerror', (e) => errors.push(String(e)));
+
+ await page.route('**/v2/customnode/install/git_url', async (route) => {
+ await route.fulfill({ status: 403, contentType: 'application/json', body: variant.body });
+ });
+
+ await openManagerMenu(page);
+ await openCustomNodesManager(page);
+ await installViaGitUrl(page, 'https://github.com/example/pack');
+
+ const dialog = denialDialog(page);
+ await expect(
+ dialog,
+ `${variant.label}: the denial dialog did not render its fallback text. A 403 whose body cannot be interpreted is STILL a denial and must still be explained.`,
+ ).toBeVisible({ timeout: 10_000 });
+
+ const text = (await dialog.textContent()) ?? '';
+ expect(
+ text,
+ `${variant.label}: the rendered denial text contains the literal string "undefined" — a malformed body leaked through the reason lookup. text: ${text.slice(0, 300)}`,
+ ).not.toContain('undefined');
+ expect(
+ errors,
+ `${variant.label}: the 403 handler threw. pageerrors: ${errors.join(' | ')}`,
+ ).toEqual([]);
+ });
+ }
+
+ test('the denial text is SELECTED BY the server reason code', async ({ page }) => {
+ // CROSS-WIRED ON PURPOSE. Each install arm has its own denial text, so a
+ // per-endpoint check cannot tell a server-DERIVED selection from a
+ // hand-maintained constant that happens to match the endpoint it sits on.
+ // Answering the git_url endpoint with the PIP reason code separates them:
+ // a client that reads the server's reason names allow_pip_install, and a
+ // client that prints a per-call-site literal names the other flag no
+ // matter what the server said. That drift is what this row catches.
+ await page.route('**/v2/customnode/install/git_url', async (route) => {
+ await route.fulfill({
+ status: 403,
+ contentType: 'application/json',
+ body: JSON.stringify({ reason: 'allow_pip_install' }),
+ });
+ });
+
+ await openManagerMenu(page);
+ await openCustomNodesManager(page);
+ await installViaGitUrl(page, 'https://github.com/example/pack');
+
+ const dialog = denialDialog(page);
+ await expect(dialog).toBeVisible({ timeout: 10_000 });
+ const text = (await dialog.textContent()) ?? '';
+
+ expect(
+ text,
+ `the server said reason="allow_pip_install" and the client must name THAT condition. Observed text names the other flag, i.e. the message is a hand-maintained constant per call site rather than being selected from the server's reason code. text: ${text.slice(0, 400)}`,
+ ).toContain('allow_pip_install');
+ });
+
+ test('the denial text states the SERVER\'s restart requirement', async ({ page }) => {
+ // THE STANDING GUARD FOR A HAND-MAINTAINED COPY. Only the reason CODE is
+ // derived from the server; the wording lives in js/common.js, and nothing
+ // makes the two move together. They had already come apart: the server
+ // constants say the flag and network_mode are read once at STARTUP, so a
+ // user who edits config.ini and retries is denied identically — and the
+ // client text omitted it, leaving that user with a correct edit and no hint
+ // that a restart is what is missing.
+ //
+ // The expected sentence is READ OUT of the server constants
+ // (serverInstallDenialRestartClause), not typed here, so this row cannot
+ // become a third copy that drifts on its own. Reword the server and the
+ // client must follow; drop it from the server and this row says so.
+ const restartClause = serverInstallDenialRestartClause();
+
+ await page.route('**/v2/customnode/install/git_url', async (route) => {
+ await route.fulfill({
+ status: 403,
+ contentType: 'application/json',
+ body: JSON.stringify({ reason: 'allow_git_url_install' }),
+ });
+ });
+
+ await openManagerMenu(page);
+ await openCustomNodesManager(page);
+ await installViaGitUrl(page, 'https://github.com/example/pack');
+
+ const dialog = denialDialog(page);
+ await expect(dialog).toBeVisible({ timeout: 10_000 });
+ const text = (await dialog.textContent()) ?? '';
+
+ expect(
+ text,
+ `the client denial text does not state the restart requirement the SERVER states. ` +
+ `A user who edits config.ini and retries is denied identically, with nothing in ` +
+ `the UI saying a restart is what is missing.\n` +
+ `expected (from SECURITY_MESSAGE_FLAG_*): ${JSON.stringify(restartClause)}\n` +
+ `observed dialog text: ${JSON.stringify(text.slice(0, 600))}`,
+ ).toContain(restartClause);
+ });
+});
+
+// ===========================================================================
+// search-keyword highlight — the grid re-renders matched cells from
+// textContent, which would undo the formatter escaping unless the bundle
+// escapes every slice it splices (see tests/test_grid_highlight_escaping.py)
+// ===========================================================================
+
+test.describe('keyword highlight re-render', () => {
+ test.use({ viewport: { width: 2600, height: 1000 } });
+
+ test.beforeEach(async ({ page }) => {
+ await page.goto('/');
+ await waitForComfyUI(page);
+ });
+
+ test('a keyword matching inside an escaped raw field stays inert after highlight', async ({ page }) => {
+ // The keyword must match VISIBLE text: the match cache decodes markup-
+ // shaped values to their text, and a bare ![]()
has none.
+ const payload = XSS_PAYLOAD + 'PWN_author';
+ await routeNodeList(page, { p: makePack('p', { author: payload }) });
+ await openManagerMenu(page);
+ await openCustomNodesManager(page);
+
+ const search = page.locator('.cn-manager-keywords').first();
+ await expect(search, 'precondition: search box not rendered').toBeVisible();
+ await search.fill('PWN');
+
+ // PRECONDITION: the highlighter actually ran on the payload cell.
+ await expect(
+ page.locator(`${GRID} .cn-pack-author mark`).first(),
+ 'precondition: no highlight marker rendered in the author cell, so the re-render path was never exercised',
+ ).toBeVisible({ timeout: 10_000 });
+
+ await expectInert(page, GRID, payload, 'img', 'author after keyword highlight');
+ });
+});
diff --git a/tests/playwright/helpers.ts b/tests/playwright/helpers.ts
index dc3098639..aa0b8120f 100644
--- a/tests/playwright/helpers.ts
+++ b/tests/playwright/helpers.ts
@@ -6,8 +6,14 @@
* Model, Snapshot) are launched.
*/
+import * as fs from 'fs';
+import { execFileSync } from 'child_process';
+import * as path from 'path';
import { type Page, expect } from '@playwright/test';
+/** Repository root, resolved from this file (tests/playwright/helpers.ts). */
+const REPO_ROOT = path.resolve(__dirname, '..', '..');
+
/** Wait for the ComfyUI page to be fully loaded (queue ready). */
export async function waitForComfyUI(page: Page) {
// ComfyUI shows the canvas once the app is ready. Wait for the
@@ -126,3 +132,238 @@ export async function assertManagerMenuVisible(page: Page) {
const dialog = page.locator('#cm-manager-dialog').first();
await expect(dialog).toBeVisible();
}
+
+// ---------------------------------------------------------------------------
+// XSS fixtures. Additive — nothing above is changed.
+// ---------------------------------------------------------------------------
+
+/**
+ * The payload used by every XSS row. Assertions on it have THREE legs, because
+ * a payload can be neutered in a way that also deletes it:
+ * (1) presence — the container's textContent contains the literal payload
+ * (2) structure — no element the payload would have created exists
+ * (3) execution — window.__xss_fired is undefined after render
+ */
+export const XSS_PAYLOAD = '';
+
+/** Space-free variant — the `last_update` formatter cuts its value at the
+ * first space with `.split(' ')[0]`, so a spaced payload would be truncated
+ * and the row would pass for the wrong reason. */
+export const XSS_PAYLOAD_NOSPACE = '![](x/onerror="window.__xss_fired=1")
';
+
+/** One `/v2/customnode/getlist` pack, shaped exactly like the live response
+ * (verified against the running server: {channel, node_packs: {id: item}}). */
+export function makePack(id: string, overrides: Record = {}) {
+ return {
+ author: 'author-' + id,
+ description: 'description of ' + id,
+ files: [`https://github.com/example/${id}`],
+ install_type: 'git-clone',
+ reference: `https://github.com/example/${id}`,
+ repository: `https://github.com/example/${id}`,
+ title: id,
+ cnr_latest: '1.0.0',
+ id,
+ health: '-',
+ state: 'not-installed',
+ version: '1.0.0',
+ 'update-state': 'false',
+ stars: 5,
+ last_update: '2026-01-01 00:00:00',
+ trust: false,
+ ...overrides,
+ };
+}
+
+/** Replace the pack list the grid renders from.
+ *
+ * NOTE, because any result read from these fixtures depends on it:
+ * page.route() REPLACES the HTTP response, so the fixture BYPASSES
+ * populate_markdown. For
+ * RAW fields that is what a live server can actually deliver; for
+ * SERVER-ESCAPED fields it delivers input the live server currently cannot,
+ * so such a fixture must be pre-transformed with `serverSanitizeTag` below —
+ * otherwise the row asserts against a wire form that does not exist. */
+export async function routeNodeList(page: Page, packs: Record) {
+ await page.route('**/customnode/getlist**', async (route) => {
+ await route.fulfill({
+ status: 200,
+ contentType: 'application/json',
+ body: JSON.stringify({ channel: 'default', node_packs: packs }),
+ });
+ });
+}
+
+/** Seed `/v2/customnode/getmappings` — that route performs NO server-side
+ * sanitization, so the values it returns are genuinely raw. */
+export async function routeMappings(page: Page, mappings: Record) {
+ await page.route('**/customnode/getmappings**', async (route) => {
+ await route.fulfill({
+ status: 200,
+ contentType: 'application/json',
+ body: JSON.stringify(mappings),
+ });
+ });
+}
+
+/** Seed `/customnode/alternatives` — required to OPEN the alternatives column,
+ * which is `invisible: !this.hasAlternatives()`. Without this the alternatives
+ * column stays hidden and any test asserting on it passes vacuously. */
+export async function routeAlternatives(page: Page, alts: Record) {
+ await page.route('**/customnode/alternatives**', async (route) => {
+ await route.fulfill({
+ status: 200,
+ contentType: 'application/json',
+ body: JSON.stringify(alts),
+ });
+ });
+}
+
+/** Open Manager menu -> Custom Nodes Manager and wait for the grid to fill. */
+export async function openCustomNodesManager(page: Page) {
+ await clickMenuButton(page, 'Custom Nodes Manager');
+ await page.waitForSelector('#cn-manager-dialog', { timeout: 15_000 });
+ await page.waitForSelector('.cn-manager-grid, .tg-body', { timeout: 15_000 });
+ await page.waitForFunction(
+ () => document.querySelectorAll('.tg-body .tg-row, .cn-manager-grid tr').length > 0,
+ { timeout: 30_000, polling: 500 },
+ );
+}
+
+/** The three legs, applied to one container. `payload` is the literal string
+ * the user must SEE; `createdSelector` is what the payload would have built. */
+export async function expectInert(
+ page: Page,
+ containerSelector: string,
+ payload: string,
+ createdSelector: string,
+ label: string,
+) {
+ // ALL THREE LEGS ARE EVALUATED BEFORE ANY ASSERTION FIRES, and that is
+ // load-bearing rather than stylistic. Asserting presence first and
+ // short-circuiting cannot distinguish the two ways leg 1 fails:
+ // - the payload was PARSED AS MARKUP (the defect) — textContent holds the
+ // rendered result, not the literal source, so presence fails; or
+ // - the payload NEVER RENDERED (a broken fixture) — presence fails too.
+ // Those are opposite conclusions from an identical symptom, so a failure whose
+ // message cannot tell them apart is not evidence of anything. The
+ // diagnostic below reports all three observations on every failure.
+ const text = (await page.locator(containerSelector).first().textContent()) ?? '';
+ const created = await page.locator(`${containerSelector} ${createdSelector}`).count();
+ const fired = await page.evaluate(() => (window as unknown as Record).__xss_fired);
+
+ const present = text.includes(payload);
+ const diagnostic =
+ `${label} — three-leg result in ${containerSelector}:\n` +
+ ` leg 1 presence : ${present ? 'PASS (payload visible as literal text)' : 'FAIL (payload not present as literal text)'}\n` +
+ ` leg 2 structure : ${created === 0 ? 'PASS (no <' + createdSelector + '> constructed)' : `FAIL (${created} live <${createdSelector}> constructed — the value reached innerHTML AS MARKUP)`}\n` +
+ ` leg 3 execution : ${fired === undefined ? 'PASS (window.__xss_fired unset)' : `FAIL (window.__xss_fired=${JSON.stringify(fired)} — the payload EXECUTED)`}\n` +
+ ` READ IT THIS WAY: leg 2 or leg 3 failing = the sink is LIVE (this is the failure being demonstrated).\n` +
+ ` leg 1 failing while legs 2 and 3 PASS = the payload never rendered at all — a BROKEN FIXTURE, not a defect;\n` +
+ ` the row proves nothing in that state and must be fixed rather than reported.\n` +
+ ` container text (first 300 chars): ${JSON.stringify(text.slice(0, 300))}`;
+
+ expect(created, diagnostic).toBe(0);
+ expect(fired, diagnostic).toBeUndefined();
+ expect(present, diagnostic).toBe(true);
+}
+
+/** Run the shipped Python renderer, including its title/name/description contract. */
+export function serverPopulateMarkdown(item: Record): Record {
+ return JSON.parse(execFileSync(process.env.PYTHON || 'python3',
+ [path.join(REPO_ROOT, 'tests', 'legacy_html_testutil.py')],
+ { input: JSON.stringify(item), encoding: 'utf-8' }));
+}
+
+export function serverSanitizeTag(value: string): string {
+ return serverPopulateMarkdown({ title: value }).title;
+}
+
+/** The RESTART clause both server install-denial constants state.
+ *
+ * `js/common.js: INSTALL_DENIAL_MESSAGES` is a hand-written client copy of a
+ * fact the SERVER owns: the install flag and `network_mode` are read once at
+ * ComfyUI startup, so editing config.ini and retrying is denied identically
+ * until the server is restarted. Nothing makes the two texts move together, so
+ * the client copy is exactly the kind of thing that silently falls behind — it
+ * already had, which is why this helper exists.
+ *
+ * Read from `SECURITY_MESSAGE_FLAG_GIT_URL` / `SECURITY_MESSAGE_FLAG_PIP` and
+ * cross-checked against each other, so the assertion is against the SERVER's
+ * wording rather than a third copy typed into the test. The word "restart" is
+ * only the search key; the whole asserted sentence comes from the source. */
+export function serverInstallDenialRestartClause(): string {
+ const srcPath = path.join(REPO_ROOT, 'comfyui_manager', 'legacy', 'manager_server.py');
+ const src = fs.readFileSync(srcPath, 'utf-8');
+ const names = ['SECURITY_MESSAGE_FLAG_GIT_URL', 'SECURITY_MESSAGE_FLAG_PIP'];
+ const clauses = names.map((name) => {
+ const m = new RegExp(`^${name}\\s*=\\s*"((?:[^"\\\\]|\\\\.)*)"`, 'm').exec(src);
+ if (!m) {
+ throw new Error(
+ `serverInstallDenialRestartClause: ${name} not found as a double-quoted ` +
+ `string literal in ${srcPath} — the server constant moved or changed shape.`,
+ );
+ }
+ const sentence = m[1].split('. ').find((s) => /\brestart\b/i.test(s));
+ if (!sentence) {
+ throw new Error(
+ `serverInstallDenialRestartClause: ${name} states no sentence mentioning a ` +
+ 'restart. Either the server dropped the fact (and the client copy should ' +
+ 'drop it too) or the wording changed — do not paper over it here.',
+ );
+ }
+ return `${sentence.trim()}.`;
+ });
+ if (clauses[0] !== clauses[1]) {
+ throw new Error(
+ 'serverInstallDenialRestartClause: the two server constants state DIFFERENT ' +
+ `restart clauses, so there is no single fact to check the client against:\n` +
+ ` ${names[0]}: ${JSON.stringify(clauses[0])}\n` +
+ ` ${names[1]}: ${JSON.stringify(clauses[1])}`,
+ );
+ }
+ return clauses[0];
+}
+
+// ---------------------------------------------------------------------------
+// Model Manager fixtures.
+// ---------------------------------------------------------------------------
+
+/** One `/v2/externalmodel/getlist` model, shaped like the live response. */
+export function makeModel(name: string, overrides: Record = {}) {
+ return {
+ name,
+ type: 'checkpoint',
+ base: 'SD1.5',
+ save_path: 'checkpoints',
+ description: 'description of ' + name,
+ reference: 'https://example.com/' + name,
+ filename: name + '.safetensors',
+ url: 'https://example.com/' + name + '.safetensors',
+ size: '1.0MB',
+ installed: 'False',
+ ...overrides,
+ };
+}
+
+/** Replace the model list the Model Manager grid renders from. */
+export async function routeModelList(page: Page, models: Record[]) {
+ await page.route('**/externalmodel/getlist**', async (route) => {
+ await route.fulfill({
+ status: 200,
+ contentType: 'application/json',
+ body: JSON.stringify({ models }),
+ });
+ });
+}
+
+/** Open Manager menu -> Model Manager and wait for the grid to fill. */
+export async function openModelManager(page: Page) {
+ await clickMenuButton(page, 'Model Manager');
+ await page.waitForSelector('#cmm-manager-dialog', { timeout: 15_000 });
+ await page.waitForSelector('.cmm-manager-grid, .tg-body', { timeout: 15_000 });
+ await page.waitForFunction(
+ () => document.querySelectorAll('.tg-body .tg-row, .cmm-manager-grid tr').length > 0,
+ { timeout: 30_000, polling: 500 },
+ );
+}
diff --git a/tests/playwright/legacy-message-errors.spec.ts b/tests/playwright/legacy-message-errors.spec.ts
new file mode 100644
index 000000000..d0b066d2b
--- /dev/null
+++ b/tests/playwright/legacy-message-errors.spec.ts
@@ -0,0 +1,235 @@
+import { test, expect } from '@playwright/test';
+import {
+ waitForComfyUI, openManagerMenu, openCustomNodesManager, openModelManager,
+ routeNodeList, routeModelList, makePack, makeModel, serverSanitizeTag, XSS_PAYLOAD,
+} from './helpers';
+
+for (const kind of ['nodes', 'models']) {
+ const prefix = kind === 'nodes' ? 'cn' : 'cmm';
+ const file = kind === 'nodes' ? 'custom-nodes-manager' : 'model-manager';
+ const exported = kind === 'nodes' ? 'CustomNodesManager' : 'ModelManager';
+
+ for (const failure of ['network', 'http', 'json', 'shape']) {
+ test(`${kind}: ${failure} batch failure clears pending controls`, async ({ page }) => {
+ const errors: string[] = [];
+ page.on('pageerror', error => errors.push(String(error)));
+ await page.goto('/');
+ await waitForComfyUI(page);
+ if (kind === 'nodes') {
+ await routeNodeList(page, { victim: makePack('victim', { version: 'unknown' }) });
+ } else {
+ await routeModelList(page, [makeModel('victim')]);
+ }
+ await page.route('**/v2/manager/queue/batch', async route => {
+ if (failure === 'network') return route.abort('failed');
+ if (failure === 'http') return route.fulfill({ status: 500, json: { failed: [] } });
+ if (failure === 'json') return route.fulfill({ contentType: 'application/json', body: 'invalid' });
+ return route.fulfill({ json: { failed: {} } });
+ });
+ await openManagerMenu(page);
+ await (kind === 'nodes' ? openCustomNodesManager(page) : openModelManager(page));
+ await page.locator(`.${prefix}-manager-grid .${prefix}-btn-install`).first().click();
+ await expect(page.locator(`.${prefix}-manager-message`)).toContainText('Failed to submit installation request');
+ await expect(page.locator(`.${prefix}-manager-stop`)).toBeHidden();
+ await expect(page.locator(`.${prefix}-btn-loading`)).toHaveCount(0);
+ expect(await page.evaluate(async ({ file, exported }) => {
+ const module = await import(`/extensions/comfyui-manager-legacy/${file}.js`);
+ return !!module[exported].instance.install_context;
+ }, { file, exported })).toBe(false);
+ expect(errors).toEqual([]);
+ });
+ }
+
+ test(`${kind}: rejected batch names and unknown IDs reach both error displays safely`, async ({ page }) => {
+ const errors: string[] = [];
+ page.on('pageerror', error => errors.push(String(error)));
+ await page.goto('/');
+ await waitForComfyUI(page);
+ const title = serverSanitizeTag('Pack & Friends ' + XSS_PAYLOAD);
+ if (kind === 'nodes') {
+ await routeNodeList(page, { victim: makePack('victim', { title, version: 'unknown' }) });
+ } else {
+ await routeModelList(page, [makeModel(title)]);
+ }
+ await page.route('**/v2/manager/queue/batch', async route => {
+ const batch = route.request().postDataJSON();
+ const items = batch[kind === 'nodes' ? 'install' : 'install_model'];
+ expect(items[0].id).toBeTruthy();
+ // The socket event may arrive before the HTTP rejection response.
+ await page.evaluate(async () => {
+ const { api } = await import('/scripts/api.js');
+ api.dispatchEvent(new CustomEvent('cm-queue-status', { detail: { status: 'all-done' } }));
+ });
+ await route.fulfill({ json: { failed: [items[0].id, XSS_PAYLOAD] } });
+ });
+ await openManagerMenu(page);
+ await (kind === 'nodes' ? openCustomNodesManager(page) : openModelManager(page));
+ if (kind === 'models') {
+ await page.locator('.cmm-manager-type').evaluate(select => select.setAttribute('disabled', ''));
+ }
+ await page.locator(`.${prefix}-manager-grid .${prefix}-btn-install`).first().click();
+ const message = page.locator(`.${prefix}-manager-message`);
+ await expect(message).toContainText('[FAIL] Pack & Friends ' + XSS_PAYLOAD);
+ await expect(message).toContainText('[FAIL] ' + XSS_PAYLOAD);
+ expect(await message.locator('img').count()).toBe(0);
+ const dialog = await page.evaluate(async () => {
+ const { app } = await import('/scripts/app.js');
+ return { text: app.ui.dialog.element.textContent, images: app.ui.dialog.element.querySelectorAll('img').length };
+ });
+ expect(dialog.text).toContain('[FAIL] Pack & Friends ' + XSS_PAYLOAD);
+ expect(dialog.text).toContain('[FAIL] ' + XSS_PAYLOAD);
+ expect(dialog.images).toBe(0);
+ await expect(page.locator(`.${prefix}-manager-stop`)).toBeHidden();
+ await expect(page.locator(`.${prefix}-btn-loading`)).toHaveCount(0);
+ if (kind === 'models') {
+ await expect(page.locator('.cmm-manager-type')).toBeDisabled();
+ }
+ expect(await page.evaluate(async ({ file, exported }) => {
+ const module = await import(`/extensions/comfyui-manager-legacy/${file}.js`);
+ return !!module[exported].instance.install_context;
+ }, { file, exported })).toBe(false);
+ expect(await page.evaluate(() => (window as any).__xss_fired)).toBeUndefined();
+ expect(errors).toEqual([]);
+ });
+
+ for (const rejectedCount of [0, 1]) {
+ test(`${kind}: ${rejectedCount} rejections keep accepted work pending until completion`, async ({ page }) => {
+ await page.goto('/');
+ await waitForComfyUI(page);
+ if (kind === 'nodes') {
+ await routeNodeList(page, {
+ first: makePack('first', { version: 'unknown' }),
+ second: makePack('second', { version: 'unknown' }),
+ });
+ } else {
+ await routeModelList(page, [makeModel('first'), makeModel('second')]);
+ }
+ let completion: Record;
+ await page.route('**/v2/manager/queue/batch', async route => {
+ const batch = route.request().postDataJSON();
+ const items = batch[kind === 'nodes' ? 'install' : 'install_model'];
+ const result = Object.fromEntries(items.slice(rejectedCount).map(item => [item.ui_id, 'success']));
+ completion = { status: 'batch-done', batch_id: batch.batch_id,
+ nodepack_result: kind === 'nodes' ? result : {}, model_result: kind === 'models' ? result : {},
+ done_count: items.length - rejectedCount, total_count: items.length - rejectedCount };
+ await route.fulfill({ json: { failed: items.slice(0, rejectedCount).map(item => item.id) } });
+ });
+ await openManagerMenu(page);
+ await (kind === 'nodes' ? openCustomNodesManager(page) : openModelManager(page));
+ await page.evaluate(async ({ file, exported, kind, prefix }) => {
+ const module = await import(`/extensions/comfyui-manager-legacy/${file}.js`);
+ const manager = module[exported].instance;
+ const button = manager.element.querySelector(`.${prefix}-btn-install`);
+ if (kind === 'nodes') {
+ await manager.installNodes(Object.values(manager.custom_nodes).map((item: any) => item.hash),
+ { target: button, label: 'Install', mode: 'install' });
+ } else {
+ await manager.installModels(manager.modelList, button);
+ }
+ }, { file, exported, kind, prefix });
+ await expect(page.locator(`.${prefix}-manager-stop`)).toBeVisible();
+ expect(await page.evaluate(async ({ file, exported }) => {
+ const module = await import(`/extensions/comfyui-manager-legacy/${file}.js`);
+ return !!module[exported].instance.install_context;
+ }, { file, exported })).toBe(true);
+
+ await page.evaluate(async detail => {
+ const { api } = await import('/scripts/api.js');
+ api.dispatchEvent(new CustomEvent('cm-queue-status', { detail }));
+ }, completion!);
+ await expect(page.locator(`.${prefix}-manager-stop`)).toBeHidden();
+ await expect(page.locator(`.${prefix}-btn-loading`)).toHaveCount(0);
+ expect(await page.evaluate(async ({ file, exported }) => {
+ const module = await import(`/extensions/comfyui-manager-legacy/${file}.js`);
+ return !!module[exported].instance.install_context;
+ }, { file, exported })).toBe(false);
+ });
+ }
+
+ test(`${kind}: HTML messages preserve names and assign color through the DOM`, async ({ page }) => {
+ await page.goto('/');
+ await waitForComfyUI(page);
+ const wire = serverSanitizeTag('Pack & Friends ');
+ const result = await page.evaluate(async ({ file, exported, prefix, wire }) => {
+ const module = await import(`/extensions/comfyui-manager-legacy/${file}.js`);
+ const manager = Object.create(module[exported].prototype);
+ manager.element = document.createElement('div');
+ manager.element.innerHTML = ``;
+ document.body.append(manager.element);
+ manager.showMessage(wire, 'rgb(1, 2, 3)');
+ manager.showStatus(wire, '">');
+ const [message, status] = manager.element.children;
+ const result = { message: message.textContent, status: status.textContent,
+ color: message.style.color, children: status.children.length };
+ manager.showMessage('done');
+ return { ...result, resetColor: message.style.color };
+ }, { file, exported, prefix, wire });
+ expect(result).toEqual({ message: 'Pack & Friends ', status: 'Pack & Friends ',
+ color: 'rgb(1, 2, 3)', children: 0, resetColor: '' });
+ });
+}
+
+test('nodes: lookup errors and missing row IDs remain literal text', async ({ page }) => {
+ await page.goto('/');
+ await waitForComfyUI(page);
+ const results = await page.evaluate(async payload => {
+ const { CustomNodesManager } = await import('/extensions/comfyui-manager-legacy/custom-nodes-manager.js');
+ const common = await import('/extensions/comfyui-manager-legacy/common.js');
+ const { api } = await import('/scripts/api.js');
+ const manager = Object.create(CustomNodesManager.prototype);
+ manager.element = document.createElement('div');
+ manager.element.innerHTML = '';
+ document.body.append(manager.element);
+ manager.grid = { getRowItemBy: () => undefined, updateCell: () => { throw new Error('Missing row updated'); } };
+ const saved = api.fetchApi;
+ const previousManager = common.manager_instance;
+ common.setManagerInstance({ datasrc_combo: { value: 'cache' } });
+ api.fetchApi = async () => ({ status: 500, statusText: payload });
+ const results = [];
+ try {
+ for (const invoke of [
+ () => manager.getMissingNodesLegacy({}, new Set()),
+ () => manager.getAlternatives(),
+ () => manager.installNodes([payload], { target: document.createElement('button'), mode: 'install' }),
+ ]) {
+ await invoke();
+ const message = manager.element.querySelector('.cn-manager-message');
+ results.push({ text: message.textContent, images: message.querySelectorAll('img').length });
+ }
+ } finally {
+ api.fetchApi = saved;
+ common.setManagerInstance(previousManager);
+ }
+ return results;
+ }, XSS_PAYLOAD);
+ expect(results).toHaveLength(3);
+ for (const result of results) {
+ expect(result.text).toContain(XSS_PAYLOAD);
+ expect(result.images).toBe(0);
+ }
+ expect(await page.evaluate(() => (window as any).__xss_fired)).toBeUndefined();
+});
+
+test('nodes: workflow registry IDs remain inert in the Missing Nodes dialog', async ({ page }) => {
+ await page.goto('/');
+ await waitForComfyUI(page);
+ const result = await page.evaluate(async payload => {
+ const { app } = await import('/scripts/app.js');
+ const { CustomNodesManager } = await import('/extensions/comfyui-manager-legacy/custom-nodes-manager.js');
+ const manager = Object.create(CustomNodesManager.prototype);
+ manager.custom_nodes = {};
+ manager.getMissingNodesLegacy = async () => {};
+ const nodes = app.graph._nodes;
+ try {
+ app.graph._nodes = [{ type: 'MissingTestNode', properties: { cnr_id: payload } }];
+ await manager.getMissingNodes();
+ } finally {
+ app.graph._nodes = nodes;
+ }
+ const dialog = app.ui.dialog.element;
+ return { text: dialog.textContent, images: dialog.querySelectorAll('img').length };
+ }, XSS_PAYLOAD + ' & Registry');
+ expect(result.images).toBe(0);
+ expect(result.text).toContain(XSS_PAYLOAD + ' & Registry');
+ expect(await page.evaluate(() => (window as any).__xss_fired)).toBeUndefined();
+});
diff --git a/tests/playwright/model-manager-xss.spec.ts b/tests/playwright/model-manager-xss.spec.ts
new file mode 100644
index 000000000..5e4985123
--- /dev/null
+++ b/tests/playwright/model-manager-xss.spec.ts
@@ -0,0 +1,244 @@
+/**
+ * Browser-level XSS regression suite for the Model Manager UI.
+ *
+ * Same contract as custom-nodes-xss.spec.ts: `name` / `description` are
+ * escaped by the server (populate_markdown) and rendered as-is; every other
+ * model-list field is raw and must be escaped on the client. Rows assert the
+ * payload is visible as literal text, constructs no element, and never runs.
+ *
+ * Requires ComfyUI running with --enable-manager-legacy-ui on PORT.
+ */
+
+import { test, expect } from '@playwright/test';
+import {
+ waitForComfyUI,
+ openManagerMenu,
+ openModelManager,
+ routeModelList,
+ makeModel,
+ expectInert,
+ serverSanitizeTag,
+ XSS_PAYLOAD,
+} from './helpers';
+
+const DIALOG = '#cmm-manager-dialog';
+const GRID = '.cmm-manager-grid';
+
+function trackPageErrors(page: import('@playwright/test').Page) {
+ const errors: string[] = [];
+ page.on('pageerror', (e) => errors.push(String(e)));
+ return errors;
+}
+
+/** Records every innerHTML write to elements carrying `className`. */
+async function captureInnerHTMLWrites(page: import('@playwright/test').Page, className: string, slot: string) {
+ await page.evaluate(({ className, slot }) => {
+ const w = window as unknown as Record;
+ w[slot] = [];
+ const desc = Object.getOwnPropertyDescriptor(Element.prototype, 'innerHTML')!;
+ Object.defineProperty(Element.prototype, 'innerHTML', {
+ configurable: true,
+ get() { return desc.get!.call(this); },
+ set(value) {
+ try {
+ if ((this as Element).classList?.contains(className)) w[slot].push(String(value));
+ } catch { /* never break the page under test */ }
+ desc.set!.call(this, value);
+ },
+ });
+ }, { className, slot });
+}
+
+async function analyseWrites(page: import('@playwright/test').Page, slot: string, payload: string) {
+ return page.evaluate(({ slot, payload }) => {
+ const w = window as unknown as Record;
+ const writes: string[] = w[slot] ?? [];
+ const probe = document.createElement('div');
+ const parsed = writes.map((html) => {
+ probe.innerHTML = html;
+ return { text: probe.textContent ?? '', imgs: probe.querySelectorAll('img').length };
+ });
+ return {
+ writes,
+ carrying: parsed.filter((p) => p.text.includes(payload)).length,
+ imgs: parsed.reduce((n, p) => n + p.imgs, 0),
+ fired: w.__xss_fired,
+ liveImgs: document.querySelectorAll('img[src="x"]').length,
+ };
+ }, { slot, payload });
+}
+
+test.describe('model grid render sinks', () => {
+ // Columns are virtualised horizontally; pin a wide viewport so every body
+ // cell this suite asserts on is actually in the DOM.
+ test.use({ viewport: { width: 2600, height: 1000 } });
+
+ test.beforeEach(async ({ page }) => {
+ await page.goto('/');
+ await waitForComfyUI(page);
+ });
+
+ test('hostile raw fields render inert (LIVE no-user-action)', async ({ page }) => {
+ await routeModelList(page, [
+ makeModel('m1', { type: XSS_PAYLOAD, base: XSS_PAYLOAD }),
+ makeModel('m2', { save_path: XSS_PAYLOAD, filename: XSS_PAYLOAD }),
+ // a non-parsable size falls through to the raw branch of the formatter
+ makeModel('m3', { size: XSS_PAYLOAD }),
+ ]);
+ await openManagerMenu(page);
+ await openModelManager(page);
+
+ // The dialog covers the grid AND the type/base filter dropdowns, which are
+ // built from the same raw values.
+ await expectInert(page, DIALOG, XSS_PAYLOAD, 'img', 'type/base/save_path/filename/size');
+ });
+
+ test('a server-escaped name renders decoded (no double-escape)', async ({ page }) => {
+ const AUTHORED = 'Tom & Jerry <3';
+ const ON_THE_WIRE = serverSanitizeTag(AUTHORED);
+ expect(ON_THE_WIRE).not.toBe(AUTHORED);
+ // makeModel derives filename/reference/url/description from the name;
+ // those are RAW columns and would carry the wire form literally, so give
+ // them values that do not contain the name.
+ await routeModelList(page, [makeModel(ON_THE_WIRE, {
+ filename: 'legit.safetensors', reference: 'https://example.com/legit',
+ url: 'https://example.com/legit.safetensors', description: 'a legit model',
+ })]);
+ await openManagerMenu(page);
+ await openModelManager(page);
+
+ const text = (await page.locator(GRID).first().textContent()) ?? '';
+ expect(text, 'the server-escaped name was escaped a second time').toContain(AUTHORED);
+ expect(text, 'the wire form is visible as literal text').not.toContain(ON_THE_WIRE);
+ });
+
+ test('missing raw fields render without throwing', async ({ page }) => {
+ const errors = trackPageErrors(page);
+ const bare = makeModel('bare') as Record;
+ delete bare.type; delete bare.base; delete bare.save_path; delete bare.filename; delete bare.size; delete bare.reference;
+ await routeModelList(page, [bare]);
+ await openManagerMenu(page);
+ await openModelManager(page);
+ await expect(page.locator(GRID).first()).toBeVisible();
+ expect(errors, `pageerrors: ${errors.join(' | ')}`).toEqual([]);
+ });
+});
+
+test.describe('model URL sinks', () => {
+ test.beforeEach(async ({ page }) => {
+ await page.goto('/');
+ await waitForComfyUI(page);
+ });
+
+ test('name and download links carry no scheme outside the allow-list', async ({ page }) => {
+ await routeModelList(page, [
+ makeModel('evil', {
+ reference: 'javascript:window.__xss_fired=1',
+ url: 'javascript:window.__xss_fired=1',
+ }),
+ // an unquoted href used to let a space inject attributes
+ makeModel('inject', { reference: 'https://example.com/x onmouseover=window.__xss_fired=1' }),
+ // a quoted href must not be closed early by a quote inside a URL that
+ // still parses as https (sanitizeUrl keeps the raw string)
+ makeModel('quote', { reference: 'https://example.com/x" onmouseover="window.__xss_fired=1' }),
+ makeModel('quote2', { url: 'https://example.com/y" onmouseover="window.__xss_fired=1' }),
+ ]);
+ await openManagerMenu(page);
+ await openModelManager(page);
+
+ const anchors = await page.locator(`${GRID} a`).evaluateAll((els) =>
+ els.map((e) => ({ href: (e as HTMLAnchorElement).href, attrs: e.getAttributeNames() })),
+ );
+ expect(anchors.length, 'precondition: no anchors rendered').toBeGreaterThan(0);
+
+ const allowed = ['http:', 'https:'];
+ const offending = anchors.filter((a) => {
+ const s = new URL(a.href).protocol;
+ return s !== null && !allowed.includes(s);
+ });
+ expect(offending, `anchor href carries a scheme outside ${JSON.stringify(allowed)}`).toEqual([]);
+
+ const injected = anchors.filter((a) => a.attrs.some((n) => n.startsWith('on')));
+ expect(injected, 'an anchor gained an event-handler attribute from the URL').toEqual([]);
+ // hover the rows so an injected onmouseover would fire if present
+ for (const row of await page.locator(`${GRID} .tg-body .tg-row`).all()) {
+ await row.hover();
+ }
+ const fired = await page.evaluate(() => (window as unknown as Record).__xss_fired);
+ expect(fired, 'a handler injected through a URL EXECUTED').toBeUndefined();
+ });
+});
+
+test.describe('model status and error sinks', () => {
+ test.beforeEach(async ({ page }) => {
+ await page.goto('/');
+ await waitForComfyUI(page);
+ });
+
+ test('install status and batch failure text render inert', async ({ page }) => {
+ const onTheWire = serverSanitizeTag(XSS_PAYLOAD);
+ expect(onTheWire).not.toBe(XSS_PAYLOAD);
+ await routeModelList(page, [makeModel(onTheWire)]);
+ await page.route('**/v2/manager/queue/batch', async (route) => {
+ await route.fulfill({ status: 200, contentType: 'application/json', body: '{"failed": []}' });
+ });
+ await openManagerMenu(page);
+ await openModelManager(page);
+
+ await captureInnerHTMLWrites(page, 'cmm-manager-status', '__cmmStatusWrites');
+ await captureInnerHTMLWrites(page, 'cmm-manager-message', '__cmmMessageWrites');
+
+ const installBtn = page.locator(`${GRID} .cmm-btn-install`).first();
+ await expect(installBtn, 'precondition: no install button rendered').toBeVisible({ timeout: 15_000 });
+ await installBtn.click();
+
+ // Status line preserves the server-composed name.
+ const status = await analyseWrites(page, '__cmmStatusWrites', XSS_PAYLOAD);
+ expect(status.writes.length, 'precondition: nothing written to the status line').toBeGreaterThan(0);
+ expect(status.imgs, `status write parsed into ![]()
: ${JSON.stringify(status.writes)}`).toBe(0);
+ expect(status.carrying, 'status line does not carry the name as literal text').toBeGreaterThan(0);
+
+ // Batch completion: server-authored failure text reaches the message sink.
+ await page.evaluate((payload) => {
+ const api = (window as unknown as Record).comfyAPI.api.api;
+ api.dispatchEvent(new CustomEvent('cm-queue-status', {
+ detail: { status: 'batch-done', model_result: { x: payload }, done_count: 1, total_count: 1 },
+ }));
+ }, XSS_PAYLOAD);
+
+ const message = await analyseWrites(page, '__cmmMessageWrites', XSS_PAYLOAD);
+ expect(message.carrying, `precondition: no message write carried the payload: ${JSON.stringify(message.writes)}`).toBeGreaterThan(0);
+ expect(message.imgs, `message write parsed into ![]()
: ${JSON.stringify(message.writes)}`).toBe(0);
+ expect(message.liveImgs, 'an from the payload is live').toBe(0);
+ expect(message.fired, 'the payload EXECUTED').toBeUndefined();
+ });
+});
+
+test.describe('model keyword highlight re-render', () => {
+ test.use({ viewport: { width: 2600, height: 1000 } });
+
+ test.beforeEach(async ({ page }) => {
+ await page.goto('/');
+ await waitForComfyUI(page);
+ });
+
+ test('a keyword matching inside an escaped raw field stays inert after highlight', async ({ page }) => {
+ // The keyword must match VISIBLE text (see the custom-nodes row).
+ const payload = XSS_PAYLOAD + 'PWN_type';
+ await routeModelList(page, [makeModel('m', { type: payload })]);
+ await openManagerMenu(page);
+ await openModelManager(page);
+
+ const search = page.locator('.cmm-manager-keywords').first();
+ await expect(search, 'precondition: search box not rendered').toBeVisible();
+ await search.fill('PWN');
+
+ // The type column has no classMap, so narrow to the cell carrying the payload text.
+ await expect(
+ page.locator(`${GRID} .tg-cell`).filter({ hasText: 'PWN_type' }).locator('mark').first(),
+ 'precondition: no highlight marker rendered in the type cell, so the re-render path was never exercised',
+ ).toBeVisible({ timeout: 10_000 });
+
+ await expectInert(page, DIALOG, payload, 'img', 'type after keyword highlight');
+ });
+});
diff --git a/tests/test_config_write_and_install_denial.py b/tests/test_config_write_and_install_denial.py
new file mode 100644
index 000000000..171fe5bc7
--- /dev/null
+++ b/tests/test_config_write_and_install_denial.py
@@ -0,0 +1,800 @@
+"""Regression suite for the settings writer and the install-denial messages.
+
+Two behaviours are pinned here, both of which used to be wrong in ways a user
+only notices after losing data or acting on misleading advice:
+
+* ``write_config()`` MERGES onto what is on disk. It used to rebuild the
+ ``[default]`` section from the process-lifetime cache, so a hand edit made
+ while the server was up, a key that ``read_config`` reads but the writer
+ never writes, and any non-``[default]`` section were all silently destroyed
+ by the next settings write. Those are the four merge rows below.
+* the install-denial MESSAGES name the condition that actually failed. A
+ denial can be caused by the dedicated flag OR by the network mode, and a
+ message that names only one of them sends the user to change a setting that
+ will not help.
+
+A later group covers a silent read failure: an existing but
+UNREADABLE ``config.ini`` used to fall through the same else-branch as "no
+config yet", so the writer treated it as a fresh bootstrap and overwrote the
+file it could not read.
+
+RUN IT PINNED. A non-editable ``comfyui_manager`` can live in site-packages,
+and the console-script ``pytest`` does not put cwd on ``sys.path``, so an
+unpinned run silently exercises the stale installed copy:
+
+ PYTHONPATH= pytest tests/test_config_write_and_install_denial.py
+
+``test_import_origin_is_this_tree`` fails loudly when the run is unpinned,
+so a misleading pass or failure cannot go unnoticed.
+
+Harness: the in-repo ``tests/_install_flags_testutil.py``, parameterized over
+BOTH config readers. ``manager_server`` is NEVER imported — importing that
+module starts a network thread — so the message rows assert against parsed
+SOURCE rather than against an imported constant.
+"""
+
+from __future__ import annotations
+
+import ast
+import configparser
+import os
+import sys
+
+import pytest
+
+sys.path.insert(0, os.path.dirname(os.path.abspath(__file__)))
+from _install_flags_testutil import import_context, import_reader # noqa: E402
+
+REPO_ROOT = os.path.abspath(os.path.join(os.path.dirname(__file__), ".."))
+READERS = ("glob", "legacy")
+
+LEGACY_SERVER = os.path.join(REPO_ROOT, "comfyui_manager", "legacy", "manager_server.py")
+SHARED_MESSAGES = os.path.join(REPO_ROOT, "comfyui_manager", "common", "security_messages.py")
+
+#: the keys ``write_config()`` owns, per tree. Deliberately spelled
+#: out rather than read back from the implementation: this is the CONTRACT the
+#: code must satisfy, not a mirror of whatever the code currently does.
+OWNED_KEYS = {
+ "legacy": (
+ "git_exe", "use_uv", "channel_url", "share_option", "bypass_ssl",
+ "file_logging", "update_policy", "windows_selector_event_loop_policy",
+ "model_download_by_agent", "downgrade_blacklist", "security_level",
+ "always_lazy_install", "network_mode", "db_mode",
+ "allow_git_url_install", "allow_pip_install",
+ ),
+ "glob": (
+ "git_exe", "use_uv", "use_unified_resolver", "channel_url",
+ "share_option", "bypass_ssl", "file_logging", "update_policy",
+ "windows_selector_event_loop_policy", "model_download_by_agent",
+ "downgrade_blacklist", "security_level", "always_lazy_install",
+ "network_mode", "db_mode", "verbose",
+ "allow_git_url_install", "allow_pip_install",
+ ),
+}
+
+#: read by both readers, written by neither — a settings write must leave
+#: them alone.
+UNWRITTEN_KEYS = ("http_channel_enabled", "default_cache_as_channel_url")
+
+
+# ---------------------------------------------------------------------------
+# Fixtures
+# ---------------------------------------------------------------------------
+
+@pytest.fixture(params=READERS)
+def reader_core(request):
+ """(reader_name, manager_core) for BOTH readers, cache isolated per test."""
+ core = import_reader(request.param)
+ saved = getattr(core, "cached_config", None)
+ setattr(core, "cached_config", None)
+ yield request.param, core
+ setattr(core, "cached_config", saved)
+
+
+@pytest.fixture
+def point_config(monkeypatch):
+ """Point ``context.manager_config_path`` at a path (resolved at call time
+ by read_config AND write_config, so this redirects both readers)."""
+ ctx = import_context()
+
+ def _point(path):
+ monkeypatch.setattr(ctx, "manager_config_path", str(path))
+
+ return _point
+
+
+def _write_ini(path, body: str):
+ with open(path, "w", encoding="utf-8") as fh:
+ fh.write(body)
+ return path
+
+
+def _read_raw(path) -> str:
+ with open(path, encoding="utf-8") as fh:
+ return fh.read()
+
+
+def _parse_disk(path) -> configparser.ConfigParser:
+ cp = configparser.ConfigParser(strict=False)
+ cp.read(str(path))
+ return cp
+
+
+def _boot(core):
+ """Simulate process start: populate the module-level config cache once."""
+ setattr(core, "cached_config", None)
+ return core.get_config()
+
+
+def _settings_change(core, key, value):
+ """The real caller shape: mutate the cache, then write."""
+ core.get_config()[key] = value
+ core.write_config()
+
+
+def _message_constants(source_path):
+ """Module-level ``SECURITY_MESSAGE_* = "..."`` assignments -> {name: text}.
+
+ Parsed from source: importing ``manager_server`` starts a network thread.
+ """
+ tree = ast.parse(_read_raw(source_path), filename=source_path)
+ out = {}
+ for node in tree.body:
+ if not isinstance(node, ast.Assign):
+ continue
+ if not isinstance(node.value, ast.Constant) or not isinstance(node.value.value, str):
+ continue
+ for target in node.targets:
+ if isinstance(target, ast.Name) and target.id.startswith("SECURITY_MESSAGE_"):
+ out[target.id] = node.value.value
+ return out
+
+
+def _flag_message_emissions(source_path):
+ """Every ``logging.error(SECURITY_MESSAGE_FLAG_*...)`` site.
+
+ Returns [(lineno, const_name, formatted: bool)]. ``formatted`` is True only
+ when the argument is ``SECURITY_MESSAGE_FLAG_*.format(listen=args.listen)``.
+ """
+ tree = ast.parse(_read_raw(source_path), filename=source_path)
+ found = []
+ for node in ast.walk(tree):
+ if not isinstance(node, ast.Call):
+ continue
+ func = node.func
+ if not (isinstance(func, ast.Attribute) and func.attr == "error"):
+ continue
+ if not node.args:
+ continue
+ arg = node.args[0]
+ # bare constant: logging.error(SECURITY_MESSAGE_FLAG_GIT_URL)
+ if isinstance(arg, ast.Name) and arg.id.startswith("SECURITY_MESSAGE_FLAG_"):
+ found.append((node.lineno, arg.id, False))
+ continue
+ # formatted: logging.error(SECURITY_MESSAGE_FLAG_GIT_URL.format(listen=...))
+ if (isinstance(arg, ast.Call)
+ and isinstance(arg.func, ast.Attribute)
+ and arg.func.attr == "format"
+ and isinstance(arg.func.value, ast.Name)
+ and arg.func.value.id.startswith("SECURITY_MESSAGE_FLAG_")):
+ kwargs = {kw.arg for kw in arg.keywords}
+ listen_kw = "listen" in kwargs
+ found.append((node.lineno, arg.func.value.id, listen_kw))
+ return sorted(found)
+
+
+# ---------------------------------------------------------------------------
+# import-origin guard
+# ---------------------------------------------------------------------------
+
+def test_import_origin_is_this_tree():
+ """The suite must exercise THIS checkout, not the site-packages copy.
+
+ Unpinned, the console-script ``pytest`` resolves ``comfyui_manager`` to the
+ non-editable 4.2.2 install and every other verdict in this file becomes
+ meaningless. Measured once on the pre-existing suites: an unpinned run
+ reported 18 failed / 73 passed where a pinned run reported 91 passed.
+ """
+ core = import_reader("legacy")
+ origin = getattr(core, "__file__", None)
+ assert origin, "comfyui_manager.legacy.manager_core has no __file__ to check"
+ resolved = os.path.abspath(origin)
+ assert resolved.startswith(REPO_ROOT + os.sep), (
+ "IMPORT-ORIGIN GUARD: comfyui_manager resolved to %r, which is OUTSIDE this "
+ "checkout (%r). The run is UNPINNED and is testing a different copy of "
+ "the package. Re-run as: PYTHONPATH=%s pytest tests/test_config_write_and_install_denial.py"
+ % (resolved, REPO_ROOT, REPO_ROOT)
+ )
+
+
+# ---------------------------------------------------------------------------
+# write_config merges onto disk
+# ---------------------------------------------------------------------------
+
+def test_hand_edit_survives(reader_core, point_config, tmp_path):
+ """A hand edit made while the server is UP survives an
+ unrelated settings change.
+
+ ``write_config()`` rebuilds ``[default]`` from the process-lifetime
+ startup snapshot, so the edit is overwritten with the stale cached value —
+ the user follows the denial message, edits config.ini, touches any Manager
+ setting, and is denied again after restart.
+ """
+ reader, core = reader_core
+ ini = tmp_path / "config.ini"
+ _write_ini(ini, "[default]\nsecurity_level = normal\nallow_git_url_install = false\n")
+ point_config(ini)
+
+ _boot(core) # startup snapshot: flag False
+ # user hand-edits config.ini while ComfyUI is running
+ _write_ini(ini, "[default]\nsecurity_level = normal\nallow_git_url_install = true\n")
+ _settings_change(core, "db_mode", "local")
+
+ on_disk = _parse_disk(ini)["default"].get("allow_git_url_install", "")
+ assert str(on_disk).lower() == "true", (
+ "expected the hand-edited allow_git_url_install=true to SURVIVE "
+ "an unrelated settings change; observed %r on disk (%s reader). "
+ "write_config() re-wrote the key from the startup snapshot.\nfile:\n%s"
+ % (on_disk, reader, _read_raw(ini))
+ )
+
+
+def test_unwritten_keys_survive(reader_core, point_config, tmp_path):
+ """Keys read_config reads but write_config never writes.
+
+ ``http_channel_enabled`` gates the insecure-channel warning and
+ ``default_cache_as_channel_url`` steers channel resolution; every settings
+ write deletes both from the file.
+ """
+ reader, core = reader_core
+ ini = tmp_path / "config.ini"
+ _write_ini(
+ ini,
+ "[default]\nsecurity_level = normal\n"
+ "http_channel_enabled = true\ndefault_cache_as_channel_url = true\n",
+ )
+ point_config(ini)
+
+ _boot(core)
+ _settings_change(core, "db_mode", "local")
+
+ section = _parse_disk(ini)["default"]
+ missing = [k for k in UNWRITTEN_KEYS if k not in section]
+ assert not missing, (
+ "expected the read-but-never-written keys %s to SURVIVE a "
+ "settings write; observed %s DELETED from the file (%s reader). "
+ "write_config() rebuilds [default] from its own key list.\nfile:\n%s"
+ % (list(UNWRITTEN_KEYS), missing, reader, _read_raw(ini))
+ )
+
+
+def test_foreign_section_survives(reader_core, point_config, tmp_path):
+ """A non-[default] section survives a settings write.
+
+ ``write_config()`` builds a FRESH parser, assigns only
+ ``config['default']`` and opens the file ``'w'``, erasing every other section.
+ """
+ reader, core = reader_core
+ ini = tmp_path / "config.ini"
+ _write_ini(
+ ini,
+ "[default]\nsecurity_level = normal\n\n"
+ "[my_custom_section]\nfoo = bar\nbaz = 1\n",
+ )
+ point_config(ini)
+
+ _boot(core)
+ _settings_change(core, "db_mode", "local")
+
+ disk = _parse_disk(ini)
+ assert disk.has_section("my_custom_section"), (
+ "expected the foreign section [my_custom_section] to SURVIVE a "
+ "settings write; observed it ERASED (%s reader).\nfile:\n%s"
+ % (reader, _read_raw(ini))
+ )
+ assert disk["my_custom_section"].get("foo") == "bar", (
+ "[my_custom_section] survived but lost its content (%s reader).\nfile:\n%s"
+ % (reader, _read_raw(ini))
+ )
+
+
+def test_no_reclobber_after_persist(reader_core, point_config, tmp_path):
+ """A key changed once through the API, then hand-edited,
+ is NOT re-clobbered by the next unrelated write.
+
+ This is the residual clobber that upstream's first attempt at the merge
+ left behind, later fixed with ``dirty_keys.difference_update(keys)``:
+ without the post-write reset the key stays dirty for the life of the
+ process and every later write overlays the stale cached value again.
+ """
+ reader, core = reader_core
+ ini = tmp_path / "config.ini"
+ _write_ini(ini, "[default]\nsecurity_level = normal\ndb_mode = cache\n")
+ point_config(ini)
+
+ _boot(core)
+ _settings_change(core, "db_mode", "local") # persisted through the API
+ assert _parse_disk(ini)["default"].get("db_mode") == "local", (
+ "precondition failed: the API change did not reach disk (%s reader)" % reader
+ )
+
+ # user now hand-edits that SAME key while the server is up
+ raw = _read_raw(ini).replace("db_mode = local", "db_mode = remote")
+ _write_ini(ini, raw)
+
+ _settings_change(core, "share_option", "none") # UNRELATED settings change
+
+ on_disk = _parse_disk(ini)["default"].get("db_mode", "")
+ assert on_disk == "remote", (
+ "expected the later hand-edit db_mode=remote to SURVIVE an "
+ "unrelated settings change; observed %r (%s reader) — the previously "
+ "API-changed key was written again from the stale cache.\nfile:\n%s"
+ % (on_disk, reader, _read_raw(ini))
+ )
+
+
+def test_bootstrap_seeds_owned_keys(reader_core, point_config, tmp_path):
+ """No config.ini at all: the bootstrap path seeds the full
+ owned key set.
+
+ A guard: the merge rework must not break first-launch bootstrap.
+ """
+ reader, core = reader_core
+ ini = tmp_path / "config.ini"
+ point_config(ini)
+ assert not os.path.exists(ini), "precondition: config.ini must be absent"
+
+ _boot(core)
+ core.write_config()
+
+ assert os.path.exists(ini), (
+ "bootstrap did not create config.ini (%s reader)" % reader
+ )
+ section = _parse_disk(ini)["default"]
+ missing = [k for k in OWNED_KEYS[reader] if k not in section]
+ assert not missing, (
+ "bootstrap seeded an incomplete [default] for the %s reader — "
+ "missing %s.\nfile:\n%s"
+ % (reader, missing, _read_raw(ini))
+ )
+
+
+def test_malformed_ini_does_not_raise(reader_core, point_config, tmp_path):
+ """An unparsable config.ini must not make a settings change
+ raise; the file is rewritten from the current settings.
+
+ A guard. Deliberately does NOT assert survival of unwritten keys or
+ foreign sections: an unparsable file is exactly where the merge narrows,
+ and asserting survival here would pin a behavior the design does not
+ promise.
+ """
+ reader, core = reader_core
+ ini = tmp_path / "config.ini"
+ _write_ini(ini, "this is not an ini file\n[[[unclosed\n= = =\n")
+ point_config(ini)
+
+ _boot(core)
+ try:
+ _settings_change(core, "db_mode", "local")
+ except Exception as exc: # noqa: BLE001 — the point of the row
+ pytest.fail(
+ "a settings change raised %s on an unparsable config.ini "
+ "(%s reader): %s" % (type(exc).__name__, reader, exc)
+ )
+
+ disk = _parse_disk(ini)
+ assert disk.has_section("default"), (
+ "after the fallback rewrite the file has no [default] section "
+ "(%s reader).\nfile:\n%s" % (reader, _read_raw(ini))
+ )
+
+
+def test_unowned_key_not_persisted(reader_core, point_config, tmp_path):
+ """Dirtying a key write_config does NOT own must not
+ persist it (unchanged from today's behavior).
+
+ A guard: ownership alone decides what a write persists.
+ """
+ reader, core = reader_core
+ ini = tmp_path / "config.ini"
+ _write_ini(ini, "[default]\nsecurity_level = normal\n")
+ point_config(ini)
+
+ _boot(core)
+ core.get_config()["http_channel_enabled"] = True # unowned
+ _settings_change(core, "db_mode", "local") # owned
+
+ section = _parse_disk(ini)["default"]
+ assert "http_channel_enabled" not in section, (
+ "the unowned key http_channel_enabled was persisted (%s reader); "
+ "write_config() must persist only keys it owns.\nfile:\n%s"
+ % (reader, _read_raw(ini))
+ )
+
+
+def test_unowned_dirty_marker_survives(reader_core, point_config, tmp_path):
+ """A dirty marker for a key this write did NOT persist
+ must SURVIVE the write.
+
+ Rests on the ``DirtyTrackingConfig`` cache, and pins
+ ``difference_update(keys)`` over ``clear()``: a blanket clear would drop the
+ marker for a key the write never persisted, so the change the user made to
+ it would never reach disk.
+ """
+ reader, core = reader_core
+ ini = tmp_path / "config.ini"
+ _write_ini(ini, "[default]\nsecurity_level = normal\n")
+ point_config(ini)
+
+ cached = _boot(core)
+ cached["http_channel_enabled"] = True # unowned -> never persisted
+ _settings_change(core, "db_mode", "local") # owned -> persisted
+
+ dirty = getattr(core.get_config(), "dirty_keys", None)
+ assert dirty is not None, (
+ "the cached config exposes no dirty_keys (%s reader) — the cache is no "
+ "longer a DirtyTrackingConfig, so write_config() cannot know what "
+ "actually changed and falls back to rewriting every owned key from the "
+ "startup snapshot."
+ % reader
+ )
+ assert "http_channel_enabled" in dirty, (
+ "the dirty marker for the UNPERSISTED key http_channel_enabled "
+ "was dropped by the write (%s reader); the post-write reset must "
+ "difference_update() exactly the persisted keys, never clear() "
+ ". dirty_keys=%r" % (reader, dirty)
+ )
+ assert "db_mode" not in dirty, (
+ "db_mode was persisted by this write but is still marked dirty "
+ "(%s reader) — without the reset it stays dirty for the life of the "
+ "process and re-clobbers a later hand-edit. dirty_keys=%r"
+ % (reader, dirty)
+ )
+
+
+def test_partial_write_leaves_file_intact(reader_core, point_config, tmp_path, monkeypatch):
+ """A settings write that fails PART-WAY must leave config.ini byte-intact.
+
+ The merge is what makes this matter. Foreign keys and foreign sections are
+ carried across by reading them off the disk, so the file on disk is their
+ ONLY home — nothing in the process holds a copy. A writer that truncates
+ that file and then streams into it opens a window in which the content
+ exists nowhere at all, and a crash, a kill, or a full disk inside the window
+ leaves a half-written file and nothing to recover from. That is the same
+ loss the merge exists to prevent, arriving through the writer instead.
+
+ The failure has to be INJECTED because the window is invisible from
+ outside: a write that completes closes it either way, so only interrupting
+ one distinguishes "wrote elsewhere and renamed" from "truncated in place".
+ The injection point is ``ConfigParser.write`` mid-output, which is where a
+ real ENOSPC or SIGKILL would land.
+
+ Two legs. The file is unchanged — the previous settings survive the failed
+ attempt in full. And nothing is left behind: writing to a temporary file is
+ only an improvement if the failed attempt cleans up after itself, otherwise
+ the directory accumulates partial files that a later reader could mistake
+ for the real one.
+ """
+ reader, core = reader_core
+ ini = tmp_path / "config.ini"
+ _write_ini(ini, "[default]\nsecurity_level = normal\ndb_mode = cache\n"
+ "http_channel_enabled = true\n\n[my_section]\nfoo = bar\n")
+ point_config(ini)
+
+ _boot(core)
+ before = ini.read_bytes()
+
+ def _partial_then_fail(self, fp, *args, **kwargs):
+ fp.write("[default]\nsecurity_level = nor")
+ raise OSError(28, "No space left on device")
+
+ monkeypatch.setattr(configparser.ConfigParser, "write", _partial_then_fail)
+
+ with pytest.raises(OSError):
+ _settings_change(core, "db_mode", "local")
+
+ after = ini.read_bytes()
+ assert after == before, (
+ "a settings write that failed mid-output changed config.ini (%s reader). "
+ "The failed attempt must not touch the target at all — write a temporary "
+ "file beside it and os.replace() it into place, so the file is either the "
+ "old one or the complete new one.\nbefore:\n%s\nafter:\n%s"
+ % (reader, before.decode("utf-8", "replace"), after.decode("utf-8", "replace"))
+ )
+
+ strays = sorted(p.name for p in tmp_path.iterdir() if p.name != "config.ini")
+ assert not strays, (
+ "the failed write left %s behind next to config.ini (%s reader). A "
+ "partial file in the settings directory outlives the failure and can be "
+ "picked up later; the writer must remove its temporary file when the "
+ "write does not complete." % (strays, reader)
+ )
+
+
+# ---------------------------------------------------------------------------
+# install-denial messages
+# ---------------------------------------------------------------------------
+
+@pytest.mark.parametrize("const_name,flag_key", [
+ ("SECURITY_MESSAGE_FLAG_GIT_URL", "allow_git_url_install"),
+ ("SECURITY_MESSAGE_FLAG_PIP", "allow_pip_install"),
+])
+def test_flag_messages_state_both_conds(const_name, flag_key):
+ """The denial message states BOTH gate conditions and
+ carries a ``{listen}`` placeholder for the live value.
+
+ Naming only the config flag would send a user to enable a flag that is
+ often already enabled, while the real blocker is the network-position half
+ of the predicate. The gate is
+ ``flag AND (loopback OR network_mode == 'personal_cloud')`` — WIDER than
+ upstream's loopback-only, so BOTH alternatives must appear.
+ """
+ consts = _message_constants(LEGACY_SERVER)
+ assert const_name in consts, "%s not found in %s" % (const_name, LEGACY_SERVER)
+ text = consts[const_name]
+ lowered = text.lower()
+
+ assert flag_key in text, (
+ "%s no longer names its flag %r — the e2e denial-log assertions "
+ "depend on it. text=%r" % (const_name, flag_key, text)
+ )
+ assert "{listen}" in text, (
+ "expected %s to carry a {listen} placeholder so every emission "
+ "can print the LIVE --listen value; observed none. text=%r"
+ % (const_name, text)
+ )
+ assert ("loopback" in lowered or "127.0.0.1" in text), (
+ "expected %s to state the loopback half of the gate "
+ "(flag AND (loopback OR personal_cloud)); observed a flag-only message. "
+ "text=%r" % (const_name, text)
+ )
+ assert "personal_cloud" in lowered, (
+ "expected %s to state the network_mode = personal_cloud "
+ "alternative — our predicate is WIDER than upstream's loopback-only "
+ ", so omitting it makes the message contradict the gate. "
+ "text=%r" % (const_name, text)
+ )
+
+
+def test_flag_message_emissions_format():
+ """EVERY flag-message emission renders the live listen value.
+
+ There are 3 emission sites in legacy/manager_server.py
+ (the 404 arm of install_custom_node, plus the git_url and pip endpoints);
+ each must emit ``.format(listen=args.listen)``.
+ """
+ emissions = _flag_message_emissions(LEGACY_SERVER)
+ assert len(emissions) == 3, (
+ "expected 3 flag-message emission sites in %s; found %d: %r"
+ % (LEGACY_SERVER, len(emissions), emissions)
+ )
+ unformatted = [(line, name) for line, name, formatted in emissions if not formatted]
+ assert not unformatted, (
+ "these flag-denial emissions do not render the live listen value "
+ "— expected SECURITY_MESSAGE_FLAG_*.format(listen=args.listen) at every "
+ "site, observed a bare constant at %r. A user reading the log cannot tell "
+ "which half of the gate denied them." % (unformatted,)
+ )
+
+
+def test_flag_messages_drop_seclevel_copy():
+ """The flag messages must NOT carry the security-level copy.
+
+ A guard. Two existing e2e tests assert this
+ substring is ABSENT from flag-denial logs
+ (tests/e2e/test_e2e_secgate_legacy_flags.py:471,
+ tests/e2e/test_e2e_pip_url_form.py:286); the rewrite must not trip them.
+ """
+ consts = _message_constants(LEGACY_SERVER)
+ offenders = {
+ name: text
+ for name, text in consts.items()
+ if name.startswith("SECURITY_MESSAGE_FLAG_") and "security level to 'normal-'" in text
+ }
+ assert not offenders, (
+ "flag-denial message(s) %s carry the substring "
+ "\"security level to 'normal-'\", which the existing e2e suites assert is "
+ "ABSENT from the denial log. The dedicated flags are decoupled from "
+ "security_level, so the copy is also wrong." % sorted(offenders)
+ )
+
+
+# ---------------------------------------------------------------------------
+# stale 'middle' security_level
+# ---------------------------------------------------------------------------
+
+@pytest.mark.parametrize("source_path", [
+ # ids are repo-RELATIVE on purpose: the absolute path differs per checkout,
+ # so an absolute id would give the same test a different node-id in every
+ # checkout and defeat node-id comparison across test runs.
+ pytest.param(LEGACY_SERVER, id=os.path.relpath(LEGACY_SERVER, REPO_ROOT)),
+ pytest.param(SHARED_MESSAGES, id=os.path.relpath(SHARED_MESSAGES, REPO_ROOT)),
+])
+def test_no_middle_security_level_value(source_path):
+ """No message may quote ``'middle'`` as a security_level.
+
+ The message that motivated this row told the user to "set the security
+ level to 'middle' or 'weak'", but no such value exists (valid: strong /
+ normal / normal- / weak) — a user following that copy sets an unrecognised
+ value and stays denied. Both remaining definition sites are covered: the
+ shared module, and the per-flag messages that are still declared next to
+ the legacy gate. The internal gate-LEVEL tokens ``is_allowed_security_level
+ ('middle')`` are NOT security_level values and are deliberately out of
+ scope — this row only inspects message CONSTANTS.
+ """
+ consts = _message_constants(source_path)
+ assert consts, "no SECURITY_MESSAGE_* constants found in %s" % source_path
+ offenders = {name: text for name, text in consts.items() if "'middle'" in text}
+ assert not offenders, (
+ "message constant(s) %s in %s quote 'middle' as a security_level "
+ "value, which does not exist (valid: strong / normal / normal- / weak). "
+ "A user following this copy sets an unrecognised value and stays denied."
+ % (sorted(offenders), os.path.relpath(source_path, REPO_ROOT))
+ )
+
+
+# ---------------------------------------------------------------------------
+# Silent read failure — an existing config.ini that cannot be READ must not
+# be mistaken for "no config yet"
+# ---------------------------------------------------------------------------
+#
+# MECHANISM:
+# configparser.ConfigParser.read() SWALLOWS OSError — "if a file named in
+# filenames cannot be opened, that file will be ignored". On a chmod-0o000
+# config.ini it returns [] and raises nothing, so write_config()'s narrow
+# `except (configparser.Error, UnicodeDecodeError)` never fires, the parser
+# is empty, has_section('default') is False, and the BOOTSTRAP branch runs:
+# the file is rewritten from the startup snapshot and the settings call
+# returns success. The user's config.ini is destroyed and nothing says so.
+#
+# The distinction these rows pin: an ABSENT file may legitimately be seeded
+# (the absent-file row below is allowed to seed), an UNREADABLE file may NOT —
+# because "unreadable" means
+# the content is unknown, not that there is no content.
+
+#: chmod cannot make a file unreadable to root, so the scenario is unbuildable
+#: there. Skip rather than assert a false pass.
+_IS_ROOT = getattr(os, "geteuid", lambda: -1)() == 0
+_root_skip = pytest.mark.skipif(
+ _IS_ROOT or os.name != "posix",
+ reason="Making a config unreadable with chmod requires POSIX and a non-root user.",
+)
+
+#: WRITE-ONLY (0o200), deliberately NOT 0o000.
+#:
+#: MEASURED against the unguarded writer, and this is the whole reason these
+#: rows are worth having: under 0o000 the scenario did NOT reproduce the defect.
+#: read() still swallowed the error and the bootstrap branch still ran, but a
+#: writer that truncates the target in place ALSO hit PermissionError, so the
+#: settings change raised and the file survived — for a reason that had nothing
+#: to do with a guard. Both assertions below would have passed against the
+#: unguarded writer, so a change could have "fixed" nothing and still passed.
+#:
+#: 0o200 removes that confound: the read fails silently, the write is fully
+#: permitted, and the file is destroyed with the call reporting success. Any
+#: raise observed under 0o200 therefore comes from a DELIBERATE check, never
+#: from the OS.
+_UNREADABLE_MODE = 0o200
+
+
+def _run_settings_change_on_unreadable(core, ini):
+ """Boot readable -> make unreadable-but-writable -> settings change.
+
+ Returns ``(raised, before_bytes, after_bytes)``. Permissions are ALWAYS
+ restored, including on failure, so tmp_path teardown can clean up.
+ """
+ _boot(core) # startup snapshot, taken while readable
+ before = ini.read_bytes()
+ os.chmod(ini, _UNREADABLE_MODE) # perms tightened while the server is up
+ raised = None
+ try:
+ _settings_change(core, "db_mode", "local")
+ except Exception as exc: # noqa: BLE001 — capturing IS the assertion
+ raised = exc
+ finally:
+ os.chmod(ini, 0o600) # restore before asserting, always
+ return raised, before, ini.read_bytes()
+
+
+@_root_skip
+def test_unreadable_config_is_loud(reader_core, point_config, tmp_path):
+ """A settings write against an UNREADABLE config.ini must
+ fail LOUDLY rather than silently rewriting it.
+
+ Without the guard the call returns success: read() ignores the
+ PermissionError, the empty parser routes into the bootstrap branch, and the
+ user is told nothing while their file is replaced by defaults. This row
+ asserts the raise that closes that path.
+
+ The file is write-only (0o200), not 0o000 — see ``_UNREADABLE_MODE``. Under
+ 0o000 this row passes against the UNFIXED code, because the OS blocks the
+ destroying write for reasons unrelated to any guard.
+ """
+ reader, core = reader_core
+ ini = tmp_path / "config.ini"
+ _write_ini(ini, "[default]\nsecurity_level = normal\ndb_mode = cache\n"
+ "http_channel_enabled = true\n\n[my_section]\nfoo = bar\n")
+ point_config(ini)
+
+ raised, before, after = _run_settings_change_on_unreadable(core, ini)
+
+ assert raised is not None, (
+ "expected the settings change to RAISE on an unreadable "
+ "config.ini; it returned success (%s reader). The file was %s. "
+ "config.read() swallows the PermissionError, so the empty parser falls "
+ "into the bootstrap branch and rewrites the file from the startup "
+ "snapshot — silent data loss.\nbefore:\n%s\nafter:\n%s"
+ % (reader,
+ "DESTROYED" if after != before else "left intact",
+ before.decode("utf-8", "replace"),
+ after.decode("utf-8", "replace"))
+ )
+
+
+@_root_skip
+def test_unreadable_config_file_intact(reader_core, point_config, tmp_path):
+ """The unreadable config.ini must be left BYTE-UNTOUCHED.
+
+ Separate node from the loudness row on purpose: "was it loud" and "was
+ the file preserved" are independent failures, and a fix that only logs a
+ warning satisfies loudness while still destroying the file. Keeping them
+ apart means neither can be satisfied by sacrificing the other.
+ """
+ reader, core = reader_core
+ ini = tmp_path / "config.ini"
+ _write_ini(ini, "[default]\nsecurity_level = normal\ndb_mode = cache\n"
+ "http_channel_enabled = true\n\n[my_section]\nfoo = bar\n")
+ point_config(ini)
+
+ _raised, before, after = _run_settings_change_on_unreadable(core, ini)
+
+ assert after == before, (
+ "the unreadable config.ini was MODIFIED by a settings change "
+ "(%s reader) — an unreadable file's content is UNKNOWN, so rewriting it "
+ "destroys data the process never saw. Hand-maintained keys and foreign "
+ "sections are gone.\nbefore:\n%s\nafter:\n%s"
+ % (reader,
+ before.decode("utf-8", "replace"),
+ after.decode("utf-8", "replace"))
+ )
+
+
+def test_genuine_bootstrap_still_seeds(reader_core, point_config, tmp_path):
+ """A genuinely ABSENT config.ini must still be seeded.
+
+ The guard that keeps the read-failure check honest: it must distinguish
+ "no file, so there is nothing to lose" from "unreadable file, so the
+ content is unknown". A check that refuses BOTH breaks first-launch
+ bootstrap; this row fails if it does.
+
+ test_bootstrap_seeds_owned_keys already covers the seeding itself; what
+ this row adds is the CONTRAST — it is the absent-file half of the same
+ decision the two unreadable-file rows pin the other half of, so a future
+ reader sees why one branch may write and the other may not.
+ """
+ reader, core = reader_core
+ ini = tmp_path / "config.ini"
+ point_config(ini)
+ assert not os.path.exists(ini), "precondition: config.ini must be absent"
+
+ _boot(core)
+ try:
+ core.write_config()
+ except Exception as exc: # noqa: BLE001 — the point of the row
+ pytest.fail(
+ "genuine bootstrap (absent config.ini) raised %s on the "
+ "%s reader: %s. The read-failure guard must refuse UNREADABLE files "
+ "without refusing ABSENT ones."
+ % (type(exc).__name__, reader, exc)
+ )
+
+ assert os.path.exists(ini), (
+ "bootstrap did not create config.ini (%s reader)" % reader
+ )
+ section = _parse_disk(ini)["default"]
+ missing = [k for k in OWNED_KEYS[reader] if k not in section]
+ assert not missing, (
+ "bootstrap seeded an incomplete [default] for the %s reader "
+ "— missing %s.\nfile:\n%s"
+ % (reader, missing, _read_raw(ini))
+ )
diff --git a/tests/test_grid_highlight_escaping.py b/tests/test_grid_highlight_escaping.py
new file mode 100644
index 000000000..035987e62
--- /dev/null
+++ b/tests/test_grid_highlight_escaping.py
@@ -0,0 +1,189 @@
+"""Exercise Manager's search overrides with the shipped TurboGrid in Chromium.
+
+Browser cases require Playwright and its Chromium install. All requests are
+intercepted locally; no ComfyUI server or external services are used.
+"""
+import os
+from pathlib import Path
+
+import pytest
+
+JS = Path(os.environ.get("MANAGER_JS_DIR") or (Path(__file__).resolve().parent.parent / "comfyui_manager" / "js"))
+
+
+@pytest.mark.parametrize("filename", ["custom-nodes-manager.js", "model-manager.js"])
+def test_searching_managers_use_the_safe_grid(filename):
+ source = (JS / filename).read_text()
+ assert 'import ManagerGrid from "./manager-grid.js"' in source
+ assert "new ManagerGrid(container)" in source
+ assert 'from "./turbogrid.esm.js"' not in source
+
+
+@pytest.fixture(scope="module")
+def browser():
+ playwright = pytest.importorskip("playwright.sync_api")
+ with playwright.sync_playwright() as driver:
+ if not Path(driver.chromium.executable_path).exists():
+ pytest.skip("install Chromium with: playwright install chromium")
+ instance = driver.chromium.launch()
+ yield instance
+ instance.close()
+
+
+@pytest.fixture
+def page(browser):
+ page = browser.new_page()
+
+ def route(request):
+ name = request.request.url.rsplit("/", 1)[-1]
+ if name in {"manager-grid.js", "turbogrid.esm.js"}:
+ request.fulfill(path=str(JS / name), content_type="text/javascript")
+ elif request.request.url == "http://manager.test/":
+ request.fulfill(
+ body='',
+ content_type="text/html",
+ )
+ else:
+ request.abort()
+
+ page.route("**/*", route)
+ page.goto("http://manager.test/")
+ page.evaluate("""async () => {
+ const {default: ManagerGrid} = await import('/manager-grid.js');
+ window.grid = new ManagerGrid(document.getElementById('grid'));
+ grid.setData({columns: [{id: 'name', name: 'Name'}], rows: []});
+ grid.render();
+ }""")
+ page.wait_for_function("window.grid.options !== undefined")
+ yield page
+ page.close()
+
+
+@pytest.mark.parametrize("keyword", ["before", "img", "after"])
+def test_highlighting_keeps_all_text_inert(page, keyword):
+ text = 'before 
& after'
+ result = page.evaluate("""({text, keyword}) => {
+ const cell = document.createElement('div');
+ cell.id = 'cell';
+ cell.textContent = text;
+ document.body.append(cell);
+ grid.highlightKeywordsSync([cell], [keyword]);
+ return {text: cell.textContent, marks: [...cell.querySelectorAll('mark')].map(x => x.textContent),
+ images: cell.querySelectorAll('img').length};
+ }""", {"text": text, "keyword": keyword})
+ assert result == {"text": text, "marks": [keyword], "images": 0}
+ assert page.evaluate("window.__probe === undefined")
+
+
+def test_keyword_order_continues_across_text_nodes(page):
+ result = page.evaluate("""() => {
+ const cell = document.createElement('div');
+ cell.innerHTML = 'Alpha beta Alpha beta ';
+ grid.highlightKeywordsSync([cell], ['alpha', 'beta']);
+ return [...cell.querySelectorAll('mark')].map(x => x.textContent);
+ }""")
+ assert result == ["Alpha", "beta", "Alpha", "beta"]
+
+
+def test_search_cache_does_not_execute_html(page):
+ result = page.evaluate("""() => {
+ const row = {description: 'Alpha & Beta'};
+ const matched = grid.highlightKeywordsFilter(row, ['description'], 'alpha & beta');
+ return {matched, text: row.tg_text_description, highlight: row.tg_highlight_description};
+ }""")
+ assert result == {"matched": True, "text": "Alpha & Beta", "highlight": True}
+ page.wait_for_timeout(50)
+ assert page.evaluate("window.__probe === undefined")
+
+
+def test_search_preserves_order_columns_and_empty_query(page):
+ result = page.evaluate("""() => {
+ const row = {name: 'Alpha Beta', description: 'Gamma', missing: null, count: 0};
+ const columns = ['name', 'description', 'missing', 'count'];
+ const ordered = grid.highlightKeywordsFilter(row, columns, ' ALPHA beta ');
+ const reversed = grid.highlightKeywordsFilter(row, columns, 'beta alpha');
+ const acrossColumns = grid.highlightKeywordsFilter(row, columns, 'alpha gamma');
+ const zero = grid.highlightKeywordsFilter(row, columns, '0');
+ const empty = grid.highlightKeywordsFilter(row, columns, '');
+ return {ordered, reversed, acrossColumns, zero, empty,
+ flags: columns.map(column => row['tg_highlight_' + column])};
+ }""")
+ assert result == {"ordered": True, "reversed": False, "acrossColumns": False,
+ "zero": True, "empty": True, "flags": [None] * 4}
+
+
+def test_custom_text_generator_and_cache_keys(page):
+ result = page.evaluate("""() => {
+ Object.assign(grid.options.highlightKeywords, {
+ textKey: 'text_', highlightKey: 'mark_',
+ textGenerator: (row, column) => row[column].label
+ });
+ const row = {name: {label: 'Model <Flux>'}};
+ const matched = grid.highlightKeywordsFilter(row, ['name'], 'model ');
+ return {matched, text: row.text_name, highlight: row.mark_name};
+ }""")
+ assert result == {"matched": True, "text": "Model ", "highlight": True}
+
+
+def test_grid_render_search_and_clear_keep_names_readable(page):
+ page.evaluate("""() => {
+ window.query = 'flux';
+ grid.setOption({rowFilter: row => grid.highlightKeywordsFilter(row, ['name'], window.query)});
+ grid.setData({columns: [{id: 'name', name: 'Name'}], rows: [
+ {name: 'Model <Flux> <img src=x onerror=window.__probe=1>'},
+ {name: 'Other Model'}
+ ]});
+ grid.render();
+ }""")
+ page.wait_for_function("document.querySelectorAll('#grid mark').length === 1")
+ assert page.locator("#grid mark").all_text_contents() == ["Flux"]
+ assert page.evaluate("grid.viewRows.length") == 1
+ assert page.locator("#grid img").count() == 0
+ assert 'Model ' in page.locator("#grid").inner_text()
+
+ page.evaluate("window.query = 'other'; grid.update()")
+ page.wait_for_function("document.querySelector('#grid mark')?.textContent === 'Other'")
+ assert page.evaluate("grid.viewRows.length") == 1
+
+ page.evaluate("window.query = ''; grid.update()")
+ page.wait_for_function("grid.viewRows.length === 2 && !document.querySelector('#grid mark')")
+ assert page.locator("#grid img").count() == 0
+ assert page.evaluate("window.__probe === undefined")
+
+
+@pytest.mark.parametrize("filename,column,value,query,nonmatching", [
+ ("custom-nodes-manager.js", "title", "Pack <Flux>", "", "<flux>"),
+ ("custom-nodes-manager.js", "description", "Notes <Flux>", "", "<flux>"),
+ ("custom-nodes-manager.js", "author", "Writer <Alias>", "<alias>", ""),
+ ("custom-nodes-manager.js", "author", "Writer ", "", "<alias>"),
+ ("model-manager.js", "name", "Model <Flux>", "", "<flux>"),
+ ("model-manager.js", "description", "Notes <Flux>", "", "<flux>"),
+ ("model-manager.js", "filename", "model <Flux>", "<flux>", ""),
+ ("model-manager.js", "type", "Type ", "", "<flux>"),
+])
+def test_manager_search_matches_visible_text(page, filename, column, value, query, nonmatching):
+ source = (JS / filename).read_text()
+ init_body = source.split("\tinitGrid() {", 1)[1].split("\n\t}\n", 1)[0]
+ common = (JS / "common.js").read_text()
+ sanitizer = common.split("export function sanitizeHTML(str) {", 1)[1].split("\n}", 1)[0]
+ escape_cell = "undefined"
+ if filename == "model-manager.js":
+ escape_cell = source.split("const escapeCell = ", 1)[1].split(";", 1)[0]
+ page.evaluate("""({init_body, sanitizer, escape_cell, column, value, query}) => {
+ const sanitizeHTML = new Function('str', sanitizer);
+ const escapeCell = new Function('sanitizeHTML', `return (${escape_cell})`)(sanitizeHTML);
+ const initGrid = new Function('ManagerGrid', 'createFlyover', 'gridId', 'sanitizeHTML', 'escapeCell',
+ `return function initGrid() {${init_body}}`)(grid.constructor, () => ({}), 'review', sanitizeHTML, escapeCell);
+ const element = document.createElement('div');
+ element.innerHTML = '';
+ document.body.append(element);
+ window.manager = {element, keywords: query, showStatus() {}, handleFlyoverHover() {}, createFlyover: () => ({}), hasAlternatives: () => false};
+ initGrid.call(manager);
+ manager.grid.setData({columns: [{id: column, name: column}], rows: [{[column]: value}]});
+ manager.grid.render();
+ }""", {"init_body": init_body, "sanitizer": sanitizer, "escape_cell": escape_cell,
+ "column": column, "value": value, "query": query})
+ page.wait_for_function("manager.grid.viewRows !== undefined")
+ assert page.evaluate("manager.grid.viewRows.length") == 1
+ page.evaluate("query => {manager.keywords=query;}", nonmatching)
+ assert not page.evaluate("manager.grid.options.rowFilter(manager.grid.data.rows[0])")
diff --git a/tests/test_markdown_composer_href.py b/tests/test_markdown_composer_href.py
new file mode 100644
index 000000000..c17ef3b29
--- /dev/null
+++ b/tests/test_markdown_composer_href.py
@@ -0,0 +1,114 @@
+"""Exercise the full legacy populate_markdown path, including readable URLs."""
+from html.parser import HTMLParser
+
+import pytest
+
+from legacy_html_testutil import load_renderer
+
+RENDERER = load_renderer()
+
+
+def compose(text):
+ item = {'description': text}
+ RENDERER['populate_markdown'](item)
+ return item['description']
+
+
+class Elements(HTMLParser):
+ def __init__(self, markup):
+ super().__init__()
+ self.elements = []
+ self.text = ''
+ self.feed(markup)
+ self.close()
+
+ def handle_starttag(self, tag, attrs):
+ self.elements.append((tag, dict(attrs)))
+
+ def handle_data(self, text):
+ self.text += text
+
+
+def anchor(markup):
+ parsed = Elements(markup)
+ assert [tag for tag, _ in parsed.elements] == ['a']
+ attrs = parsed.elements[0][1]
+ assert set(attrs) == {'href', 'target', 'rel'}
+ assert attrs['target'] == '_blank'
+ assert set(attrs['rel'].split()) == {'noopener', 'noreferrer'}
+ return attrs
+
+
+@pytest.mark.parametrize('url', [
+ "' onmouseover='alert`1`", '" onclick="alert`1`',
+ 'https://example.com/a\">',
+])
+def test_attribute_breakout_is_inert(url):
+ assert anchor(compose(f'[a/link]({url})'))['href'] == url
+
+
+@pytest.mark.parametrize('url', [
+ 'javascript:alert`1`', 'JaVaScRiPt:alert`1`', 'data:text/html,evil',
+ 'vbscript:msgbox`1`', 'ftp://host/file', 'mailto:user@example.com',
+ 'java\tscript:alert`1`', 'java\nscript:alert`1`', '\x01javascript:alert`1`',
+])
+def test_disallowed_schemes_are_rejected(url):
+ assert anchor(compose(f'[a/link]({url})'))['href'] == '#'
+
+
+@pytest.mark.parametrize('url,expected', [
+ ('https://example.com/?q=', 'https://example.com/?q='),
+ ('https://example.com/?a=1&b=2', 'https://example.com/?a=1&b=2'),
+ ('https://example.com/?a=1¬ebook=2©=3', 'https://example.com/?a=1¬ebook=2©=3'),
+ ('https://example.com/?q=¬it;', 'https://example.com/?q=¬it;'),
+ ('https://example.com/?q=<Flux>', 'https://example.com/?q='),
+ ('https://example.com/?q=<', 'https://example.com/?q=<'),
+ ('https://example.com/a**b**c', 'https://example.com/a**b**c'),
+ ('https://example.com/%%white%%', 'https://example.com/%%white%%'),
+ ('https://example.com/[w/note]', 'https://example.com/[w/note]'),
+ ('https://example.com/a\nb', 'https://example.com/a\nb'),
+ ('http://example.com/x', 'http://example.com/x'),
+ ('./docs/readme.md', './docs/readme.md'), ('#section', '#section'), ('//host/path', '//host/path'),
+ ('javascript:alert`1`', 'javascript:alert`1`'),
+ ('javascript:alert`1`', 'javascript:alert`1`'),
+])
+def test_href_survives_entity_decoding_and_formatting(url, expected):
+ assert anchor(compose(f'[a/link]({url})'))['href'] == expected
+
+
+def test_labels_prose_and_notes_keep_their_formatting():
+ out = compose(' [w/Read [a/****](https://example.com)]\n%%white%% [i/info]')
+ parsed = Elements(out)
+ assert [tag for tag, _ in parsed.elements] == ['p', 'a', 'b', 'br', 'font', 'p']
+ assert parsed.text == ' Read white info'
+ assert "class='cm-warn-note'" in out and "class='cm-info-note'" in out
+
+
+@pytest.mark.parametrize('label', ['a tag', 'a <b> tag', 'a\x00 tag'])
+def test_link_label_is_escaped_once(label):
+ out = compose(f'[a/{label}](https://example.com)')
+ anchor(out)
+ assert Elements(out).text == 'a tag'
+
+
+def test_source_cannot_forge_a_href_placeholder():
+ out = compose('\x00H0\x00 [a/link](https://example.com)')
+ assert out.count('https://example.com') == 1
+ assert anchor(out)['href'] == 'https://example.com'
+
+
+def test_name_and_title_preserve_existing_server_contract():
+ item = {'name': 'Name ', 'title': 'Pack '}
+ RENDERER['populate_markdown'](item)
+ assert item == {'name': 'Name <Flux>', 'title': 'Pack <Flux>'}
+
+
+@pytest.mark.parametrize('url', [
+ 'javascript:alert`1`', 'javascript:alert`1`',
+ 'javascript:alert`1`', 'javascript:alert`1`',
+ 'javascript:alert`1`', 'javascript:alert`1`',
+ 'data:text/html;base64,PHNjcmlwdD4=', 'java script:alert`1`',
+ 'vbscript:msgbox`1`',
+])
+def test_entity_obfuscated_schemes_are_rejected(url):
+ assert anchor(compose(f'[a/link]({url})'))['href'] == '#'
diff --git a/tests/test_notice_html_sanitizer.py b/tests/test_notice_html_sanitizer.py
new file mode 100644
index 000000000..cc27b6e2e
--- /dev/null
+++ b/tests/test_notice_html_sanitizer.py
@@ -0,0 +1,134 @@
+"""Exercise the legacy notice sanitizer and its endpoint without network access."""
+from __future__ import annotations
+
+import asyncio
+import re
+from html.parser import HTMLParser
+from types import SimpleNamespace
+from unittest.mock import AsyncMock, MagicMock
+
+from aiohttp import web
+
+import pytest
+
+from legacy_html_testutil import PACKAGE, load_functions, load_renderer
+
+sanitize = load_renderer()['html_utils'].sanitize_html_fragment
+
+
+class _Tags(HTMLParser):
+ def __init__(self):
+ super().__init__(convert_charrefs=True)
+ self.tags = []
+
+ def handle_starttag(self, tag, attrs):
+ self.tags.append((tag, dict(attrs)))
+
+
+def _tags(html):
+ p = _Tags()
+ p.feed(html)
+ p.close()
+ return p.tags
+
+
+@pytest.mark.parametrize("payload", [
+ '',
+ '',
+ 'x',
+ 'x',
+ 'x',
+ '',
+ '',
+ 'ab')
+ assert sanitize('x ') == 'x'
+
+
+def test_relative_and_https_urls_kept():
+ out = sanitize('rs')
+ hrefs = [attrs["href"] for tag, attrs in _tags(out) if tag == "a"]
+ assert hrefs == ["/wiki/x", "https://a.b/c"]
+
+
+def test_script_body_ends_at_first_close_tag_like_a_browser():
+ # Script content is raw text up to the FIRST , exactly as the
+ # browser parses it; what follows is ordinary text and stays visible.
+ assert sanitize('yz') == 'yz'
+
+
+@pytest.mark.parametrize('prefix', ['
')
+ session = MagicMock()
+ session.get.return_value.__aenter__ = AsyncMock(return_value=response)
+ client = MagicMock()
+ client.__aenter__ = AsyncMock(return_value=session)
+ namespace = load_functions(PACKAGE / 'legacy/manager_server.py', {'get_notice'}, {
+ 'routes': web.RouteTableDef(), 're': re, 'web': web,
+ 'html_utils': load_renderer()['html_utils'],
+ 'aiohttp': SimpleNamespace(ClientSession=lambda **kwargs: client, TCPConnector=lambda **kwargs: None),
+ 'os': SimpleNamespace(environ={'__COMFYUI_DESKTOP_VERSION__': '1.0'}),
+ 'core': SimpleNamespace(version_str='4.2.1'),
+ })
+ result = asyncio.run(namespace['get_notice'](None))
+ assert result.status == 200
+ assert 'News' in result.text and 'ComfyUI: 1.0 [Desktop]' in result.text and 'Manager: 4.2.1' in result.text
+ assert 'boom' not in result.text and 'Restore snapshot '${target.name}'`); + self.updateMessage(`
Restore snapshot '${sanitizeHTML(String(target.name ?? ''))}'`); for(let i in self.restore_buttons) { self.restore_buttons[i].disabled = true; @@ -214,7 +214,7 @@ export class SnapshotManager extends ComfyDialog { data1.style.textAlign = "center"; data1.innerHTML = i+1; var data2 = document.createElement('td'); - data2.innerHTML = ` ${data}`; + data2.textContent = `\u00a0${data}`; var data_button = document.createElement('td'); data_button.style.textAlign = "center"; data_button.className = "data-btns"; diff --git a/comfyui_manager/legacy/html_utils.py b/comfyui_manager/legacy/html_utils.py new file mode 100644 index 000000000..3bb10944b --- /dev/null +++ b/comfyui_manager/legacy/html_utils.py @@ -0,0 +1,82 @@ +"""HTML rendering helpers used only by the legacy Manager UI.""" +import re +from html import escape, unescape +from html.entities import html5 + + +SAFE_URL_SCHEMES = frozenset({'http', 'https'}) +_URL_SCHEME_NOISE = re.compile(r'[\x00-\x20\x7f]') +_URL_HEAD_DELIMITERS = ('/', '?', '#') + + +def escape_html_attribute(value): + """Escape a value for a quoted HTML attribute.""" + return escape(str(value), quote=True) + + +_HTML_ENTITY = re.compile(r'&(#[0-9]+|#[xX][0-9a-fA-F]+|[A-Za-z][A-Za-z0-9]*);') +_HTML_TEXT_TOKEN = re.compile(_HTML_ENTITY.pattern + "|[&<>\"']") + + +def unescape_html_entities(text): + """Decode complete entities once, leaving query keys such as ¬ebook intact.""" + def replace(match): + entity = match.group(1) + if entity.startswith('#'): + return unescape(match.group(0)) + return html5.get(entity + ';', match.group(0)) + + return _HTML_ENTITY.sub(replace, text) + + +def escape_html_text(value): + """Escape HTML text without double-escaping existing entities.""" + return _HTML_TEXT_TOKEN.sub( + lambda m: m.group(0) if m.group(1) else escape_html_attribute(m.group(0)), + str(value), + ) + + +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.""" + # Keep nh3 loading within the legacy notice path. + 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'}}, + ) diff --git a/comfyui_manager/legacy/manager_core.py b/comfyui_manager/legacy/manager_core.py index 6543713e0..17938bf45 100644 --- a/comfyui_manager/legacy/manager_core.py +++ b/comfyui_manager/legacy/manager_core.py @@ -33,6 +33,7 @@ from packaging import version import uuid from ..common import cm_global +from ..common.config_writer import DirtyTrackingConfig, write_config_merged from ..common import cnr_utils from ..common import manager_util from ..common import git_utils @@ -1672,39 +1673,30 @@ class ManagerFuncs: manager_funcs = ManagerFuncs() +# Settings owned by this server; other keys and sections are preserved. +WRITTEN_CONFIG_KEYS = ( + 'git_exe', + 'use_uv', + 'channel_url', + 'share_option', + 'bypass_ssl', + 'file_logging', + 'update_policy', + 'windows_selector_event_loop_policy', + 'model_download_by_agent', + 'downgrade_blacklist', + 'security_level', + 'always_lazy_install', + 'network_mode', + 'db_mode', + 'allow_git_url_install', + 'allow_pip_install', +) + + def write_config(): - config = configparser.ConfigParser(strict=False) - - config['default'] = { - 'git_exe': get_config()['git_exe'], - 'use_uv': get_config()['use_uv'], - 'channel_url': get_config()['channel_url'], - 'share_option': get_config()['share_option'], - 'bypass_ssl': get_config()['bypass_ssl'], - "file_logging": get_config()['file_logging'], - 'update_policy': get_config()['update_policy'], - 'windows_selector_event_loop_policy': get_config()['windows_selector_event_loop_policy'], - 'model_download_by_agent': get_config()['model_download_by_agent'], - 'downgrade_blacklist': get_config()['downgrade_blacklist'], - 'security_level': get_config()['security_level'], - 'always_lazy_install': get_config()['always_lazy_install'], - 'network_mode': get_config()['network_mode'], - 'db_mode': get_config()['db_mode'], - 'allow_git_url_install': get_config()['allow_git_url_install'], - 'allow_pip_install': get_config()['allow_pip_install'], - } - - # Sanitize all string values to prevent CRLF injection attacks - for key, value in config['default'].items(): - if isinstance(value, str): - config['default'][key] = value.replace('\r', '').replace('\n', '').replace('\x00', '') - - directory = os.path.dirname(context.manager_config_path) - if not os.path.exists(directory): - os.makedirs(directory) - - with open(context.manager_config_path, 'w') as configfile: - config.write(configfile) + """Persist changed settings through the shared writer.""" + write_config_merged(context.manager_config_path, get_config(), WRITTEN_CONFIG_KEYS) def read_config(): @@ -1770,7 +1762,8 @@ def get_config(): global cached_config if cached_config is None: - cached_config = read_config() + # Start tracking changes after the startup configuration is loaded. + cached_config = DirtyTrackingConfig(read_config()) if cached_config['http_channel_enabled']: print("[ComfyUI-Manager] Warning: http channel enabled, make sure server in secure env") diff --git a/comfyui_manager/legacy/manager_server.py b/comfyui_manager/legacy/manager_server.py index d5771de4e..ee3a27b3f 100644 --- a/comfyui_manager/legacy/manager_server.py +++ b/comfyui_manager/legacy/manager_server.py @@ -19,6 +19,7 @@ import asyncio from collections import deque from . import manager_core as core +from . import html_utils from ..common import manager_util from ..common import cm_global from ..common import manager_downloader @@ -37,14 +38,15 @@ logging.info("[ComfyUI-Manager] network_mode: " + network_mode_description) comfy_ui_hash = "-" comfyui_tag = None -SECURITY_MESSAGE_MIDDLE = "ERROR: To use this action, a security_level of `normal or below` is required. Please contact the administrator.\nReference: https://github.com/Comfy-Org/ComfyUI-Manager#security-policy" -SECURITY_MESSAGE_MIDDLE_P = "ERROR: To use this action, security_level must be `normal or below`, and network_mode must be set to `personal_cloud`. Please contact the administrator.\nReference: https://github.com/ltdrdata/ComfyUI-Manager#security-policy" -SECURITY_MESSAGE_HIGH_P = "ERROR: To use this action, '--listen' must be set to a local IP and security_level must be 'normal-' or lower. Please contact the administrator.\nReference: https://github.com/Comfy-Org/ComfyUI-Manager#security-policy" -SECURITY_MESSAGE_NORMAL_MINUS = "ERROR: To use this feature, you must either set '--listen' to a local IP and set the security level to 'normal-' or lower, or set the security level to 'middle' or 'weak'. Please contact the administrator.\nReference: https://github.com/Comfy-Org/ComfyUI-Manager#security-policy" -SECURITY_MESSAGE_GENERAL = "ERROR: This installation is not allowed in this security_level. Please contact the administrator.\nReference: https://github.com/Comfy-Org/ComfyUI-Manager#security-policy" -SECURITY_MESSAGE_NORMAL_MINUS_MODEL = "ERROR: Downloading models that are not in '.safetensors' format is only allowed for models registered in the 'default' channel at this security level. If you want to download this model, set the security level to 'normal-' or lower." -SECURITY_MESSAGE_FLAG_GIT_URL = "ERROR: This action requires 'allow_git_url_install = true' in config.ini ([default] section). This setting is independent of security_level. Please contact the administrator.\nReference: https://github.com/Comfy-Org/ComfyUI-Manager#security-policy" -SECURITY_MESSAGE_FLAG_PIP = "ERROR: This action requires 'allow_pip_install = true' in config.ini ([default] section). This setting is independent of security_level. Please contact the administrator.\nReference: https://github.com/Comfy-Org/ComfyUI-Manager#security-policy" +from ..common.security_messages import ( + SECURITY_MESSAGE_MIDDLE, + SECURITY_MESSAGE_MIDDLE_P, + SECURITY_MESSAGE_HIGH_P, + SECURITY_MESSAGE_GENERAL, + SECURITY_MESSAGE_NORMAL_MINUS_MODEL, +) +SECURITY_MESSAGE_FLAG_GIT_URL = "ERROR: This action requires BOTH: (1) 'allow_git_url_install = true' in config.ini ([default] section), AND (2) a network position the Manager treats as private - either ComfyUI launched with a loopback '--listen' (any 127.x.x.x address, or ::1), OR 'network_mode = personal_cloud' in config.ini. Currently listening on {listen} with network_mode = {network_mode}. Both values are read once at ComfyUI startup, so changing either one needs a restart, done with the server down: STOP ComfyUI, change the setting, then start it again. Both are independent of security_level. Please contact the administrator.\nReference: https://github.com/Comfy-Org/ComfyUI-Manager#security-policy" +SECURITY_MESSAGE_FLAG_PIP = "ERROR: This action requires BOTH: (1) 'allow_pip_install = true' in config.ini ([default] section), AND (2) a network position the Manager treats as private - either ComfyUI launched with a loopback '--listen' (any 127.x.x.x address, or ::1), OR 'network_mode = personal_cloud' in config.ini. Currently listening on {listen} with network_mode = {network_mode}. Both values are read once at ComfyUI startup, so changing either one needs a restart, done with the server down: STOP ComfyUI, change the setting, then start it again. Both are independent of security_level. Please contact the administrator.\nReference: https://github.com/Comfy-Org/ComfyUI-Manager#security-policy" routes = PromptServer.instance.routes @@ -259,7 +261,6 @@ print_comfyui_version() core.check_invalid_nodes() - def setup_environment(): git_exe = core.get_config()['git_exe'] @@ -974,14 +975,22 @@ async def _update_all(json_data): def convert_markdown_to_html(input_text): + """Parse source markdown, escaping text and URLs at their HTML boundaries.""" pattern_a = re.compile(r'\[a/([^]]+)]\(([^)]+)\)') pattern_w = re.compile(r'\[w/([^]]+)]') pattern_i = re.compile(r'\[i/([^]]+)]') pattern_bold = re.compile(r'\*\*([^*]+)\*\*') pattern_white = re.compile(r'%%([^*]+)%%') + # Format link labels separately from URLs; escape URLs when inserted. + hrefs = [] + def replace_a(match): - return f"{match.group(1)}" + # Entities written in the source URL (e.g. &) retain their meaning. + url = html_utils.unescape_html_entities(match.group(2)) + hrefs.append(html_utils.sanitize_url(url)) + text = html_utils.escape_html_text(match.group(1)) + return f"{text}" def replace_w(match): return f"
{match.group(1)}
" @@ -995,20 +1004,35 @@ def convert_markdown_to_html(input_text): def replace_white(match): return f"{match.group(1)}" - input_text = input_text.replace('\\[', '[').replace('\\]', ']').replace('<', '<').replace('>', '>') + # NUL dropped first so the input cannot forge the href placeholders. + input_text = input_text.replace('\x00', '') + input_text = input_text.replace('\\[', '[').replace('\\]', ']') - result_text = re.sub(pattern_a, replace_a, input_text) + # Parse links before escaping prose, so generated text entities never enter URLs. + parts = [] + start = 0 + for match in pattern_a.finditer(input_text): + parts.append(manager_util.sanitize_tag(input_text[start:match.start()])) + parts.append(replace_a(match)) + start = match.end() + parts.append(manager_util.sanitize_tag(input_text[start:])) + result_text = ''.join(parts) result_text = re.sub(pattern_w, replace_w, result_text) result_text = re.sub(pattern_i, replace_i, result_text) result_text = re.sub(pattern_bold, replace_bold, result_text) result_text = re.sub(pattern_white, replace_white, result_text) + result_text = result_text.replace("\n", "") - return result_text.replace("\n", "
") + return re.sub( + r'\x00H(\d+)\x00', + lambda m: html_utils.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']) @@ -1396,7 +1420,6 @@ async def import_fail_info_bulk(request): return web.Response(status=500, text="Internal server error") - @routes.post("/v2/manager/queue/reset") async def reset_queue(request): rejection = manager_security.reject_simple_form_post(request) @@ -1412,7 +1435,6 @@ async def reset_queue(request): return web.Response(status=200) - @routes.get("/v2/manager/queue/status") async def queue_count(request): global task_queue @@ -1493,7 +1515,8 @@ async def _install_custom_node(json_data): # Flag-deny PRESERVES today's 404 response shape at this position (R1). if risky_level == 'high+': if not _dedicated_install_allowed('allow_git_url_install'): - logging.error(SECURITY_MESSAGE_FLAG_GIT_URL) + logging.error(SECURITY_MESSAGE_FLAG_GIT_URL.format( + listen=args.listen, network_mode=core.get_config()['network_mode'])) return web.Response(status=404, text="A security error has occurred. Please check the terminal logs") elif not is_allowed_security_level(risky_level): logging.error(SECURITY_MESSAGE_GENERAL) @@ -1551,8 +1574,10 @@ async def _fix_custom_node(json_data): async def install_custom_node_git_url(request): # goal265 S-A: dedicated-flag gate, decoupled from security_level (spec §1.2). if not _dedicated_install_allowed('allow_git_url_install'): - logging.error(SECURITY_MESSAGE_FLAG_GIT_URL) - return web.Response(status=403) + logging.error(SECURITY_MESSAGE_FLAG_GIT_URL.format( + listen=args.listen, network_mode=core.get_config()['network_mode'])) + # Only the flag name goes on the wire; the full message is logged above. + return web.json_response({'reason': 'allow_git_url_install'}, status=403) url = await request.text() res = await core.gitclone_install(url) @@ -1572,8 +1597,10 @@ async def install_custom_node_git_url(request): async def install_custom_node_pip(request): # goal265 S-B: dedicated-flag gate, decoupled from security_level (spec §1.2). if not _dedicated_install_allowed('allow_pip_install'): - logging.error(SECURITY_MESSAGE_FLAG_PIP) - return web.Response(status=403) + logging.error(SECURITY_MESSAGE_FLAG_PIP.format( + listen=args.listen, network_mode=core.get_config()['network_mode'])) + # Only the flag name goes on the wire; the full message is logged above. + return web.json_response({'reason': 'allow_pip_install'}, status=403) packages = await request.text() core.pip_install(packages.split(' ')) @@ -1815,19 +1842,6 @@ async def set_channel_url(request): return web.Response(status=400, text='Invalid request') -def add_target_blank(html_text): - pattern = r'(
ComfyUI: {version_tag} [Desktop]" @@ -1857,8 +1873,6 @@ async def get_notice(request): # markdown_content += f"
()" markdown_content += f"
Manager: {core.version_str}" - markdown_content = add_target_blank(markdown_content) - try: if '__COMFYUI_DESKTOP_VERSION__' not in os.environ: if core.comfy_ui_commit_datetime == datetime(1900, 1, 1, 0, 0, 0): diff --git a/pyproject.toml b/pyproject.toml index ed8e5f72f..bcfb9e103 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -35,7 +35,9 @@ dependencies = [ "typing-extensions", "toml", "uv", - "chardet" + "chardet", + "packaging", + "nh3>=0.3.7" ] [project.optional-dependencies] diff --git a/requirements.txt b/requirements.txt index b505f2adb..433c4529d 100644 --- a/requirements.txt +++ b/requirements.txt @@ -10,3 +10,5 @@ typing-extensions toml uv chardet +packaging +nh3>=0.3.7 diff --git a/tests/e2e/test_e2e_secgate_legacy_flags.py b/tests/e2e/test_e2e_secgate_legacy_flags.py index 890137b41..4e496a1d0 100644 --- a/tests/e2e/test_e2e_secgate_legacy_flags.py +++ b/tests/e2e/test_e2e_secgate_legacy_flags.py @@ -59,6 +59,7 @@ otherwise (harness precedent: tests/e2e/test_e2e_secgate_default.py). from __future__ import annotations import configparser +import json import os import shutil import subprocess @@ -506,6 +507,94 @@ class TestFlagsOffWeakLevel: ) +# =========================================================================== +# The 403 denial reason code +# +# A denied install returns 403 with a JSON body naming the flag that denied it +# — `{"reason": "allow_git_url_install"}` or `{"reason": "allow_pip_install"}` +# — so the browser can tell the user WHICH setting to change instead of +# guessing from the endpoint it happened to call. The full +# SECURITY_MESSAGE_FLAG_* text still goes to the terminal via `logging.error`; +# only the flag name goes on the wire. +# +# Every other assertion on these endpoints tests `status_code` alone, so +# nothing else in this file can see the body at all — these rows are the only +# coverage the wire contract has. +# +# Reuses CFG-D (git=F, pip=F, sl=weak) — the config that already produces a 403 +# on both arms, so these rows add no new server boot. +# =========================================================================== + +class TestDenialReasonCode: + """The 403 body must name WHICH flag denied — and nothing more.""" + + def _denial_body(self, resp): + """Parse the 403 body as the wire contract, with the failure message + carrying what was actually received.""" + assert resp.status_code == 403, ( + f"precondition: expected 403, got {resp.status_code}" + ) + try: + return json.loads(resp.text) + except json.JSONDecodeError as exc: + pytest.fail( + "the 403 body is not JSON. Expected a JSON object carrying " + "the denial reason code; " + f"observed {resp.text!r} (len={len(resp.text)}). " + f"JSONDecodeError: {exc}. A 403 emitted with no `text=` leaves " + "the body EMPTY, and the client then has nothing to derive its " + "message from." + ) + + def test_t9_pip_denial_body_names_the_flag(self, comfyui_flags_d): + """The pip 403 body carries the reason code. + + Expected: a JSON OBJECT whose `reason` is the string + `allow_pip_install`. + """ + resp = _post_pip("text-unidecode") + body = self._denial_body(resp) + + assert isinstance(body, dict), ( + "the 403 body must be a JSON OBJECT (the client's guard is " + "`typeof data === 'object' && data !== null && typeof data.reason " + f"=== 'string'`); observed {type(body).__name__} {body!r}" + ) + assert body.get("reason") == "allow_pip_install", ( + "expected the pip 403 body to carry " + "{'reason': 'allow_pip_install'} so the client can name the " + f"condition the SERVER actually failed on; observed {body!r}" + ) + + def test_t9_git_url_denial_body_names_the_flag(self, comfyui_flags_d): + """The git_url 403 body carries its own code. + + A separate node from the pip arm on purpose: the two emissions are + separate `return` statements, so one can be changed while the other is + missed. + """ + resp = _post_git_url(UNKNOWN_GIT_URL) + body = self._denial_body(resp) + + assert isinstance(body, dict), ( + "the 403 body must be a JSON OBJECT; observed " + f"{type(body).__name__} {body!r}" + ) + assert body.get("reason") == "allow_git_url_install", ( + "expected the git_url 403 body to carry " + f"{{'reason': 'allow_git_url_install'}}; observed {body!r}" + ) + + def test_t9b_denial_body_discloses_nothing_further(self, comfyui_flags_d): + """Denied requests expose only the failed flag name.""" + responses = { + "allow_pip_install": _post_pip("text-unidecode"), + "allow_git_url_install": _post_git_url(UNKNOWN_GIT_URL), + } + for reason, response in responses.items(): + assert self._denial_body(response) == {"reason": reason} + + # =========================================================================== # CFG-E: git=T, pip=T, sl=weak, nm=public — Q1 guard # =========================================================================== diff --git a/tests/legacy_html_testutil.py b/tests/legacy_html_testutil.py new file mode 100644 index 000000000..c582e078b --- /dev/null +++ b/tests/legacy_html_testutil.py @@ -0,0 +1,36 @@ +"""Execute the legacy renderer without starting ComfyUI or its network threads.""" +import ast +import importlib.util +import json +import re +import sys +import types +from pathlib import Path + +ROOT = Path(__file__).resolve().parent.parent +PACKAGE = ROOT / 'comfyui_manager' + + +def load_functions(path, names, namespace=None): + namespace = {} if namespace is None else namespace + nodes = [node for node in ast.parse(path.read_text(encoding='utf-8')).body + if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)) and node.name in names] + assert len(nodes) == len(names) + exec(compile(ast.Module(nodes, []), str(path), 'exec'), namespace) + return namespace + + +def load_renderer(): + path = PACKAGE / 'legacy/html_utils.py' + spec = importlib.util.spec_from_file_location('_legacy_html_utils', path) + helpers = importlib.util.module_from_spec(spec) + spec.loader.exec_module(helpers) + util = load_functions(PACKAGE / 'common/manager_util.py', {'sanitize_tag'}) + return load_functions(PACKAGE / 'legacy/manager_server.py', {'convert_markdown_to_html', 'populate_markdown'}, + {'re': re, 'html_utils': helpers, 'manager_util': types.SimpleNamespace(**util)}) + + +if __name__ == '__main__': + item = json.load(sys.stdin) + load_renderer()['populate_markdown'](item) + print(json.dumps(item)) diff --git a/tests/playwright/README.md b/tests/playwright/README.md index af57c61cc..df13f740b 100644 --- a/tests/playwright/README.md +++ b/tests/playwright/README.md @@ -5,20 +5,25 @@ Browser-based E2E tests for the ComfyUI-Manager legacy UI. ## Prerequisites 1. **E2E environment** built via `python tests/e2e/scripts/setup_e2e_env.py` -2. **Playwright installed**: `npx playwright install chromium` +2. **Playwright runner and Chromium** (from the repository root): + `npm install --no-save --package-lock=false @playwright/test@1.59.1`, then + `npx playwright install chromium`. 3. **ComfyUI running** with legacy UI enabled: ```bash E2E_ROOT=/tmp/e2e_full_test PORT=8199 -$E2E_ROOT/venv/bin/python $E2E_ROOT/comfyui/main.py \ +PYTHONPATH="$PWD" $E2E_ROOT/venv/bin/python $E2E_ROOT/comfyui/main.py \ --listen 127.0.0.1 --port $PORT \ - --enable-manager-legacy-ui \ + --enable-manager --enable-manager-legacy-ui \ --cpu ``` ## Running Tests +Run from the repository root. XSS fixtures execute the checkout's Python renderer +using `python3`; set `PYTHON=/path/to/python` to select another interpreter. + ```bash # With server already running: PORT=8199 npx playwright test @@ -42,3 +47,6 @@ PORT=8199 npx playwright test --debug | `legacy-ui-model-manager.spec.ts` | Model list grid, filter, search | | `legacy-ui-snapshot.spec.ts` | Snapshot list, save, remove | | `legacy-ui-navigation.spec.ts` | Dialog open/close, nested navigation, no duplicates | +| `custom-nodes-xss.spec.ts` | Node metadata, Markdown, links, errors, search highlighting | +| `model-manager-xss.spec.ts` | Model metadata, links, errors, search highlighting | +| `legacy-message-errors.spec.ts` | Batch rejection IDs, name readability, DOM colors, lookup errors | diff --git a/tests/playwright/custom-nodes-xss.spec.ts b/tests/playwright/custom-nodes-xss.spec.ts new file mode 100644 index 000000000..fbc59fb99 --- /dev/null +++ b/tests/playwright/custom-nodes-xss.spec.ts @@ -0,0 +1,796 @@ +/** + * Browser-level XSS regression suite for the Custom Nodes manager UI. + * + * Every row feeds hostile node-pack metadata through a stubbed registry + * response and asserts the value renders as INERT TEXT: the payload is visible + * as literal characters, no element is constructed from it, and its `onerror` + * never runs. The three checks are made together because they fail + * differently — see `expectInert` in ./helpers.ts. + * + * NOT EVERY ROW CLOSES A LIVE HOLE, and the distinction matters when reading a + * failure. `version`/`cnr_latest`, `author`, `stars` and `last_update` are + * served unsanitized and sit in always-visible columns, so opening the manager + * is the whole exploit; so are node names from `/v2/customnode/getmappings`, + * a route that sanitizes nothing. Those fields are escaped ON THE CLIENT and + * their rows target that escape. + * + * `title` / `name` / `description` are different: `populate_markdown` -> + * `sanitize_tag` escapes them SERVER-SIDE, and the client renders them as-is + * on purpose. Escaping them again is not free safety — `sanitizeHTML` also maps + * `&`, so a second pass turns the server's `<` into `<` and the pack + * `ComfyUI-
& after'
+ result = page.evaluate("""({text, keyword}) => {
+ const cell = document.createElement('div');
+ cell.id = 'cell';
+ cell.textContent = text;
+ document.body.append(cell);
+ grid.highlightKeywordsSync([cell], [keyword]);
+ return {text: cell.textContent, marks: [...cell.querySelectorAll('mark')].map(x => x.textContent),
+ images: cell.querySelectorAll('img').length};
+ }""", {"text": text, "keyword": keyword})
+ assert result == {"text": text, "marks": [keyword], "images": 0}
+ assert page.evaluate("window.__probe === undefined")
+
+
+def test_keyword_order_continues_across_text_nodes(page):
+ result = page.evaluate("""() => {
+ const cell = document.createElement('div');
+ cell.innerHTML = 'Alpha beta Alpha beta ';
+ grid.highlightKeywordsSync([cell], ['alpha', 'beta']);
+ return [...cell.querySelectorAll('mark')].map(x => x.textContent);
+ }""")
+ assert result == ["Alpha", "beta", "Alpha", "beta"]
+
+
+def test_search_cache_does_not_execute_html(page):
+ result = page.evaluate("""() => {
+ const row = {description: 'Alpha & Beta'};
+ const matched = grid.highlightKeywordsFilter(row, ['description'], 'alpha & beta');
+ return {matched, text: row.tg_text_description, highlight: row.tg_highlight_description};
+ }""")
+ assert result == {"matched": True, "text": "Alpha & Beta", "highlight": True}
+ page.wait_for_timeout(50)
+ assert page.evaluate("window.__probe === undefined")
+
+
+def test_search_preserves_order_columns_and_empty_query(page):
+ result = page.evaluate("""() => {
+ const row = {name: 'Alpha Beta', description: 'Gamma', missing: null, count: 0};
+ const columns = ['name', 'description', 'missing', 'count'];
+ const ordered = grid.highlightKeywordsFilter(row, columns, ' ALPHA beta ');
+ const reversed = grid.highlightKeywordsFilter(row, columns, 'beta alpha');
+ const acrossColumns = grid.highlightKeywordsFilter(row, columns, 'alpha gamma');
+ const zero = grid.highlightKeywordsFilter(row, columns, '0');
+ const empty = grid.highlightKeywordsFilter(row, columns, '');
+ return {ordered, reversed, acrossColumns, zero, empty,
+ flags: columns.map(column => row['tg_highlight_' + column])};
+ }""")
+ assert result == {"ordered": True, "reversed": False, "acrossColumns": False,
+ "zero": True, "empty": True, "flags": [None] * 4}
+
+
+def test_custom_text_generator_and_cache_keys(page):
+ result = page.evaluate("""() => {
+ Object.assign(grid.options.highlightKeywords, {
+ textKey: 'text_', highlightKey: 'mark_',
+ textGenerator: (row, column) => row[column].label
+ });
+ const row = {name: {label: 'Model <Flux>'}};
+ const matched = grid.highlightKeywordsFilter(row, ['name'], 'model x
', + 'x
',
+ '',
+])
+def test_script_bearing_markup_is_removed(payload):
+ out = sanitize(payload)
+ for tag, attrs in _tags(out):
+ assert tag not in ("script", "svg", "iframe", "style"), out
+ assert not any(k.startswith("on") for k in attrs), out
+ for k in ("href", "src"):
+ v = attrs.get(k)
+ assert v is None or not v.lower().lstrip().startswith(("javascript", "data", "vbscript")), out
+ # Script-bearing markup must be gone entirely, as tags AND as raw text.
+ assert "ab
') == "ab
" + + +def test_formatting_markup_survives(): + src = 'News
See docs and bold
line
- one
1 < 2
') == '1 < 2
' + + +def test_unknown_harmless_tag_keeps_content(): + assert sanitize('