mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-07-18 17:58:23 +00:00
fix: stop double-escaping special characters in error tab copy (#13684)
## Summary
Node and model names containing `&`, `<`, or `>` were displayed as
literal HTML entities (`&`, `<`, `>`) in the error tab — both
in the visible text and in button `aria-label`s. This fixes the
double-escaping so the original characters render correctly everywhere.
## Changes
- **Visible text**: The error catalog resolvers pass node/model names to
vue-i18n's `t(key, params)`. Because `escapeParameter` is enabled
globally (`src/i18n.ts`), those params are HTML-escaped when the
translated string is built, and the resolved `display*` / `toast*`
fields are then rendered through Vue text interpolation (`{{ }}`), which
escapes again at the render boundary — so `Foo & Bar` reached the DOM as
`Foo & Bar`. Fixed by passing `{ escapeParameter: false }` at the
two catalog translation call sites (`catalogI18n.ts`), so escaping
happens once, at the Vue render boundary.
- **Accessibility tree**: three `:aria-label` bindings re-interpolate
the now-raw resolved label through the global `t()`
(`ErrorGroupList.vue` info/locate, `MissingMediaCard.vue` locate).
Attribute values are never entity-decoded, so a screen reader announced
`Info for A & B <C>`. Same fix applied — `{ escapeParameter:
false }` at those three sites — so the accessible name matches the
visible label. (Attribute bindings go through `setAttribute`, never
`v-html`, so this introduces no injection surface.)
## Review Focus
- **XSS safety is preserved.** All affected fields are rendered as text
(`{{ }}`, `v-text`) or attribute bindings (`:aria-label`) — never
`v-html`. Vue's render/attribute boundary remains the single escaping
pass; a script-like name renders as inert text.
- **Completeness.** The interpolation sites are the two catalog
translation calls plus the three `:aria-label` calls above — those are
the only places a resolved node/model name is re-interpolated through
`t()` in the error-tab / missing-media surfaces. Sibling bindings either
render the label directly (`{{ }}` / `:title`, single-escaped by Vue) or
concatenate raw strings outside `t()`.
- **Test fixture alignment (intentional).** The shared `testI18n`
fixture in `searchbox/v2/__test__/testUtils.ts` previously omitted
`escapeParameter`, so it ran with vue-i18n's default of `false` —
diverging from production's `true`. That divergence would let a
double-escape regression pass the new tests unnoticed, so the fixture is
aligned to production. It's a no-op for the existing searchbox specs
(their params are plain ASCII). The `rightSidePanel → searchbox`
test-fixture dependency is pre-existing and worth relocating to a
neutral shared home later.
## Tests
- `errorMessageResolver.test.ts`: special characters survive through the
node-name (execution), model-name (missing model), and details-copy
paths, with a `te(...)` key-existence guard so the model-name assertion
can't silently pass via the raw fallback branch if a locale key is
renamed.
- `ErrorNodeCard.test.ts`: card text renders literal special characters,
including a script-like name rendered as text.
- `ErrorGroupList.test.ts` / `MissingMediaCard.test.ts`: the info/locate
button accessible names render literal characters with no HTML entities
(these two exercise different resolver paths, so both are covered
directly).
This commit is contained in:
committed by
github-actions[bot]
parent
b575e5033f
commit
53f2a10fb5
@@ -236,4 +236,27 @@ describe('ErrorGroupList selection emphasis', () => {
|
||||
expect(strip).toHaveTextContent('2 nodes — 2 errors')
|
||||
})
|
||||
})
|
||||
|
||||
it('preserves special characters in execution item accessible names', () => {
|
||||
const nodeDisplayName = 'A & B <C>'
|
||||
vi.mocked(getNodeByExecutionId).mockImplementation((_, nodeId) =>
|
||||
fromAny<LGraphNode, unknown>(
|
||||
String(nodeId) === '1'
|
||||
? { ...SAMPLER_NODE, title: nodeDisplayName }
|
||||
: LOADER_NODE
|
||||
)
|
||||
)
|
||||
const pinia = createPinia()
|
||||
seedTwoErrorGroups(pinia)
|
||||
|
||||
renderList(pinia)
|
||||
|
||||
expect(
|
||||
screen.getByRole('button', { name: /Info for A & B <C>/ })
|
||||
).toBeInTheDocument()
|
||||
expect(
|
||||
screen.getByRole('button', { name: /Locate A & B <C>/ })
|
||||
).toBeInTheDocument()
|
||||
expect(screen.queryAllByLabelText(/&(?:amp|lt|gt);/)).toHaveLength(0)
|
||||
})
|
||||
})
|
||||
|
||||
@@ -239,7 +239,11 @@
|
||||
)
|
||||
"
|
||||
:aria-label="
|
||||
t('rightSidePanel.infoFor', { item: item.label })
|
||||
t(
|
||||
'rightSidePanel.infoFor',
|
||||
{ item: item.label },
|
||||
{ escapeParameter: false }
|
||||
)
|
||||
"
|
||||
:aria-controls="getExecutionItemDetailId(item.key)"
|
||||
:aria-expanded="isExecutionItemDetailExpanded(item.key)"
|
||||
@@ -253,9 +257,13 @@
|
||||
size="icon-sm"
|
||||
class="size-8 shrink-0 text-muted-foreground hover:text-base-foreground focus-visible:ring-inset"
|
||||
:aria-label="
|
||||
t('rightSidePanel.locateNodeFor', {
|
||||
item: item.label
|
||||
})
|
||||
t(
|
||||
'rightSidePanel.locateNodeFor',
|
||||
{
|
||||
item: item.label
|
||||
},
|
||||
{ escapeParameter: false }
|
||||
)
|
||||
"
|
||||
@click.stop="handleLocateNode(item.nodeId)"
|
||||
>
|
||||
|
||||
@@ -6,8 +6,10 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
import { createI18n } from 'vue-i18n'
|
||||
import ErrorNodeCard from './ErrorNodeCard.vue'
|
||||
import type { ErrorCardData } from './types'
|
||||
import { resolveRunErrorMessage } from '@/platform/errorCatalog/errorMessageResolver'
|
||||
import { createNodeExecutionId } from '@/types/nodeIdentification'
|
||||
import { toNodeId } from '@/types/nodeId'
|
||||
import { validationError } from '@/utils/__tests__/nodeErrorHelpers'
|
||||
|
||||
const mockGetLogs = vi.fn(() => Promise.resolve('mock server logs'))
|
||||
const mockSerialize = vi.fn(() => ({ nodes: [] }))
|
||||
@@ -100,7 +102,7 @@ describe('ErrorNodeCard.vue', () => {
|
||||
const user = userEvent.setup()
|
||||
const onCopyToClipboard = vi.fn()
|
||||
const onLocateNode = vi.fn()
|
||||
render(ErrorNodeCard, {
|
||||
const { container } = render(ErrorNodeCard, {
|
||||
props: { card, onCopyToClipboard, onLocateNode },
|
||||
global: {
|
||||
plugins: [
|
||||
@@ -142,7 +144,7 @@ describe('ErrorNodeCard.vue', () => {
|
||||
}
|
||||
}
|
||||
})
|
||||
return { user, onCopyToClipboard, onLocateNode }
|
||||
return { container, user, onCopyToClipboard, onLocateNode }
|
||||
}
|
||||
|
||||
async function toggleRuntimeDetails(
|
||||
@@ -185,6 +187,32 @@ describe('ErrorNodeCard.vue', () => {
|
||||
}
|
||||
}
|
||||
|
||||
function makeValidationErrorCard(nodeDisplayName: string): ErrorCardData {
|
||||
const error = validationError(
|
||||
'required_input_missing',
|
||||
'model',
|
||||
{},
|
||||
'Required input is missing',
|
||||
'model'
|
||||
)
|
||||
|
||||
return {
|
||||
id: `validation-${++cardIdCounter}`,
|
||||
title: 'Validation error',
|
||||
errors: [
|
||||
{
|
||||
message: error.message,
|
||||
details: error.details,
|
||||
...resolveRunErrorMessage({
|
||||
kind: 'node_validation',
|
||||
error,
|
||||
nodeDisplayName
|
||||
})
|
||||
}
|
||||
]
|
||||
}
|
||||
}
|
||||
|
||||
it('shows runtime details by default and can collapse them', async () => {
|
||||
const reportText =
|
||||
'# ComfyUI Error Report\n## System Information\n- OS: Linux'
|
||||
@@ -280,6 +308,21 @@ describe('ErrorNodeCard.vue', () => {
|
||||
).not.toBeInTheDocument()
|
||||
})
|
||||
|
||||
it('renders catalog copy with literal special characters', () => {
|
||||
const { container } = renderCard(makeValidationErrorCard('Foo & Bar <C>'))
|
||||
|
||||
expect(container.textContent).toContain('Foo & Bar <C>')
|
||||
expect(container.textContent).not.toMatch(/&(?:amp|lt|gt);/)
|
||||
})
|
||||
|
||||
it('renders script-like node names as literal text', () => {
|
||||
const { container } = renderCard(
|
||||
makeValidationErrorCard('<script>x</script>')
|
||||
)
|
||||
|
||||
expect(container.textContent).toContain('<script>x</script>')
|
||||
})
|
||||
|
||||
it('copies enriched report when copy button is clicked for runtime error', async () => {
|
||||
const reportText = '# Full Report Content'
|
||||
mockGenerateErrorReport.mockReturnValue(reportText)
|
||||
|
||||
@@ -34,6 +34,7 @@ export function setupTestPinia() {
|
||||
export const testI18n = createI18n({
|
||||
legacy: false,
|
||||
locale: 'en',
|
||||
escapeParameter: true,
|
||||
messages: { en: enMessages }
|
||||
})
|
||||
|
||||
|
||||
@@ -15,7 +15,8 @@ export function translateCatalogMessage(
|
||||
fallback: string,
|
||||
params?: CatalogParams
|
||||
): string {
|
||||
if (te(key)) return params ? t(key, params) : t(key)
|
||||
if (te(key))
|
||||
return params ? t(key, params, { escapeParameter: false }) : t(key)
|
||||
if (!params) return fallback
|
||||
|
||||
return fallback.replace(/\{(\w+)\}/g, (match, paramName) =>
|
||||
@@ -28,7 +29,8 @@ export function translateOptionalCatalogMessage(
|
||||
fallback?: string,
|
||||
params?: CatalogParams
|
||||
): string | undefined {
|
||||
if (te(key)) return params ? t(key, params) : t(key)
|
||||
if (te(key))
|
||||
return params ? t(key, params, { escapeParameter: false }) : t(key)
|
||||
return fallback?.trim() ? fallback : undefined
|
||||
}
|
||||
|
||||
|
||||
@@ -10,7 +10,7 @@ import type { ExecutionErrorWsMessage } from '@/schemas/apiSchema'
|
||||
import type { MissingMediaGroup } from '@/platform/missingMedia/types'
|
||||
import type { MissingModelGroup } from '@/platform/missingModel/types'
|
||||
import type { MissingNodeType } from '@/types/comfy'
|
||||
import { i18n } from '@/i18n'
|
||||
import { i18n, te } from '@/i18n'
|
||||
|
||||
function nodeValidationError(
|
||||
type: string,
|
||||
@@ -131,6 +131,35 @@ describe('errorMessageResolver', () => {
|
||||
})
|
||||
})
|
||||
|
||||
it('preserves special characters in catalog copy for node names', () => {
|
||||
const nodeDisplayName = 'A & B <C>'
|
||||
const result = resolveRunErrorMessage({
|
||||
kind: 'execution',
|
||||
error: executionError('ImageDownloadError', 'Failed to validate images'),
|
||||
nodeDisplayName
|
||||
})
|
||||
const interpolatedCopy = [
|
||||
result.displayItemLabel,
|
||||
result.toastMessage
|
||||
].join(' ')
|
||||
|
||||
expect(interpolatedCopy).toContain(nodeDisplayName)
|
||||
expect(interpolatedCopy).not.toMatch(/&(?:amp|lt|gt);/)
|
||||
expect(interpolatedCopy).toContain("couldn't be loaded")
|
||||
})
|
||||
|
||||
it('preserves special characters in catalog details copy for node names', () => {
|
||||
const nodeDisplayName = 'A & B <C>'
|
||||
const result = resolveRunErrorMessage({
|
||||
kind: 'node_validation',
|
||||
error: requiredInputMissing('model'),
|
||||
nodeDisplayName
|
||||
})
|
||||
|
||||
expect(result.displayDetails).toContain(nodeDisplayName)
|
||||
expect(result.displayDetails).not.toMatch(/&(?:amp|lt|gt);/)
|
||||
})
|
||||
|
||||
it('uses catalog fallbacks when required_input_missing lacks node or input labels', () => {
|
||||
expect(
|
||||
resolveRunErrorMessage({
|
||||
@@ -1542,6 +1571,22 @@ describe('errorMessageResolver', () => {
|
||||
})
|
||||
})
|
||||
|
||||
it('preserves special characters in catalog copy for model names', () => {
|
||||
const modelName = 'sd&xl<v2>.safetensors'
|
||||
expect(
|
||||
te('errorCatalog.missingErrors.missing_model.toastTitleOneOss')
|
||||
).toBe(true)
|
||||
const result = resolveMissingErrorMessage({
|
||||
kind: 'missing_model',
|
||||
groups: missingModelGroups(modelName),
|
||||
count: 1,
|
||||
isCloud: false
|
||||
})
|
||||
|
||||
expect(result.toastTitle).toContain(modelName)
|
||||
expect(result.toastTitle).not.toMatch(/&(?:amp|lt|gt);/)
|
||||
})
|
||||
|
||||
it('resolves missing media group display and toast copy', () => {
|
||||
const groups: MissingMediaGroup[] = [
|
||||
{
|
||||
|
||||
@@ -0,0 +1,61 @@
|
||||
import { render, screen } from '@testing-library/vue'
|
||||
import { describe, expect, it, vi } from 'vitest'
|
||||
|
||||
import { testI18n } from '@/components/searchbox/v2/__test__/testUtils'
|
||||
import type { MissingMediaGroup } from '@/platform/missingMedia/types'
|
||||
import { getNodeByExecutionId } from '@/utils/graphTraversalUtil'
|
||||
import { createMockLGraphNode } from '@/utils/__tests__/litegraphTestUtils'
|
||||
|
||||
import MissingMediaCard from './MissingMediaCard.vue'
|
||||
|
||||
vi.mock('@/scripts/app', () => ({
|
||||
app: { rootGraph: {} }
|
||||
}))
|
||||
|
||||
vi.mock('@/utils/graphTraversalUtil', () => ({
|
||||
getNodeByExecutionId: vi.fn()
|
||||
}))
|
||||
|
||||
describe('MissingMediaCard', () => {
|
||||
it('preserves special characters in the locate button accessible name', () => {
|
||||
vi.mocked(getNodeByExecutionId).mockReturnValue(
|
||||
createMockLGraphNode({ title: 'A & B <C>' })
|
||||
)
|
||||
const missingMediaGroups: MissingMediaGroup[] = [
|
||||
{
|
||||
mediaType: 'image',
|
||||
items: [
|
||||
{
|
||||
name: 'image.png',
|
||||
mediaType: 'image',
|
||||
representative: {
|
||||
nodeId: '1',
|
||||
nodeType: 'LoadImage',
|
||||
widgetName: 'image',
|
||||
mediaType: 'image',
|
||||
name: 'image.png',
|
||||
isMissing: true
|
||||
},
|
||||
referencingNodes: [
|
||||
{
|
||||
nodeId: '1',
|
||||
nodeType: 'LoadImage',
|
||||
widgetName: 'image'
|
||||
}
|
||||
]
|
||||
}
|
||||
]
|
||||
}
|
||||
]
|
||||
|
||||
render(MissingMediaCard, {
|
||||
props: { missingMediaGroups },
|
||||
global: { plugins: [testI18n] }
|
||||
})
|
||||
|
||||
expect(
|
||||
screen.getByRole('button', { name: 'Locate A & B <C> - image' })
|
||||
).toBeInTheDocument()
|
||||
expect(screen.queryAllByLabelText(/&(?:amp|lt|gt);/)).toHaveLength(0)
|
||||
})
|
||||
})
|
||||
@@ -40,9 +40,13 @@
|
||||
size="icon-sm"
|
||||
class="size-8 shrink-0 text-muted-foreground hover:text-base-foreground focus-visible:ring-inset"
|
||||
:aria-label="
|
||||
t('rightSidePanel.locateNodeFor', {
|
||||
item: item.displayItemLabel
|
||||
})
|
||||
t(
|
||||
'rightSidePanel.locateNodeFor',
|
||||
{
|
||||
item: item.displayItemLabel
|
||||
},
|
||||
{ escapeParameter: false }
|
||||
)
|
||||
"
|
||||
@click.stop="emit('locateNode', item.nodeId)"
|
||||
>
|
||||
|
||||
Reference in New Issue
Block a user