fix: close custom icon review threads

This commit is contained in:
Marco Beretta 2026-07-26 18:47:27 +02:00
parent 9c7718e854
commit a33fff3088
No known key found for this signature in database
GPG key ID: D918033D8E74CC11
8 changed files with 41 additions and 381 deletions

View file

@ -33,7 +33,7 @@ export default function CustomIcon({
const decorative = alt === '';
if (shouldTint) {
const maskUrl = `url("${src.replace(/"/g, '%22')}")`;
const maskUrl = `url("${src.replace(/["\\\n\r\f]/g, encodeURIComponent)}")`;
return (
<span
role={decorative ? undefined : 'img'}

View file

@ -46,6 +46,13 @@ describe('CustomIcon', () => {
expect(span?.style.maskImage).toBe('url("/a%22.svg")');
});
it('escapes backslashes and newlines in the mask URL', () => {
const { container } = render(<CustomIcon src={'/a\\b\nc.svg'} alt="" monochrome />);
const span = container.querySelector('span');
expect(span?.style.maskImage).toBe('url("/a%5Cb%0Ac.svg")');
});
it('does not render a probe image on the tinted path without an onError handler', () => {
const { container } = render(<CustomIcon src="/glyph.svg" alt="" monochrome />);

View file

@ -63,11 +63,12 @@ export default function useAdaptiveIcon(
monochrome: cachedVerdict(key),
}));
/** Reset synchronously when the source changes so a verdict resolved for a
* previous icon never tints the new one; seed from cache when available. */
if (state.key !== key) {
setState({ key, monochrome: cachedVerdict(key) });
}
useEffect(() => {
setState((prev) => {
const monochrome = cachedVerdict(key);
return prev.key === key && prev.monochrome === monochrome ? prev : { key, monochrome };
});
}, [key]);
useEffect(() => {
if (key == null) {

View file

@ -214,19 +214,6 @@ describe('sanitizeSvg', () => {
expect(clean).not.toContain('evil.example');
});
it('strips external url() references from presentation and style attributes', () => {
for (const attr of [
'filter="url(https://evil.example/f.svg#f)"',
'fill="url(https://evil.example/p)"',
'style="fill:url(//evil.example/p)"',
'clip-path="url(data:image/svg+xml,evil)"',
]) {
const clean = sanitizeSvg(`<svg><rect ${attr} width="10" height="10" /></svg>`);
expect(clean).not.toContain('evil.example');
expect(clean).not.toContain('evil');
}
});
it('preserves local url() paint and filter references', () => {
const dirty =
'<svg><defs><filter id="f"><feGaussianBlur stdDeviation="1" /></filter><linearGradient id="g"><stop offset="0" stop-color="#000" /></linearGradient></defs><rect fill="url(#g)" filter="url(#f)" width="10" height="10" /></svg>';
@ -244,89 +231,13 @@ describe('sanitizeSvg', () => {
expect(clean).toContain('fill="url(#g2)"');
});
it('preserves internal stylesheet paint rules', () => {
it('strips stylesheet blocks and inline style attributes', () => {
const dirty =
'<svg><style>.red{fill:#e00}.blue{fill:#00f}</style><path class="red" d="M0 0h1v1z" /><path class="blue" d="M1 1h1v1z" /></svg>';
'<svg><style>.red{fill:#e00}</style><path class="red" style="stroke:#00f" fill="#e00" d="M0 0h1v1z" /></svg>';
const clean = sanitizeSvg(dirty);
expect(clean).toContain('<style');
expect(clean).toContain('.red{fill:#e00}');
expect(clean).toContain('.blue{fill:#00f}');
expect(clean).toContain('class="red"');
});
it('scrubs @import and external url() from internal stylesheets', () => {
const dirty =
'<svg><style>@import url(https://evil.example/x.css);.a{fill:url(https://evil.example/beacon)}.b{fill:url(#grad)}</style><rect class="a" /></svg>';
const clean = sanitizeSvg(dirty);
expect(clean).toContain('<style');
expect(clean).not.toContain('evil.example');
expect(clean).not.toContain('@import');
expect(clean).toContain('url(#grad)');
});
it('strips a script smuggled after a premature </style> close', () => {
const dirty = '<svg><style>.a{}</style><script>alert(1)</script></svg>';
const clean = sanitizeSvg(dirty);
expect(clean).not.toContain('<script');
expect(clean).not.toContain('alert(1)');
});
it('does not let escaped markup in a stylesheet reintroduce elements', () => {
// Scrubbed CSS is set as a text node, so `\3c/style\3e\3cimage\3e` stays
// inert escaped text rather than becoming a real element.
const dirty =
'<svg><style>\\3c/style\\3e\\3cimage href="https://evil.example/x.png"/\\3e</style></svg>';
const clean = sanitizeSvg(dirty);
expect(clean.toLowerCase()).not.toContain('<image');
expect(clean).toContain('&lt;');
});
it('strips CSS-escaped external url() from style attributes and stylesheets', () => {
const attrEsc = sanitizeSvg(
'<svg><rect style="fill:u\\72l(https://evil.example/x)" width="10" height="10" /></svg>',
);
expect(attrEsc).not.toContain('evil.example');
const styleEsc = sanitizeSvg(
'<svg><style>.a{fill:u\\72l(https://evil.example/b)}.b{fill:url(#g)}</style><rect class="a" /></svg>',
);
expect(styleEsc).not.toContain('evil.example');
expect(styleEsc).toContain('url(#g)');
});
it('strips CSS-escaped @import from internal stylesheets', () => {
const clean = sanitizeSvg(
'<svg><style>\\40import "https://evil.example/x.css";</style><rect /></svg>',
);
expect(clean).not.toContain('evil.example');
});
it('keeps co-located local declarations when scrubbing an escaped external ref', () => {
const clean = sanitizeSvg(
'<svg><rect style="fill:u\\72l(https://evil.example/x);stroke:#000" width="10" height="10" /></svg>',
);
expect(clean).not.toContain('evil.example');
expect(clean).toContain('stroke:#000');
});
it('strips XML-entity-encoded @import from internal stylesheets', () => {
for (const enc of ['&#64;import', '&#x40;import', '&#x40;IMPORT']) {
const clean = sanitizeSvg(
`<svg><style>${enc} "https://evil.example/x.css";</style><rect /></svg>`,
);
expect(clean).not.toContain('evil.example');
}
});
it('strips a quoted CSS url() whose path contains a right parenthesis', () => {
const block = sanitizeSvg(
'<svg><style>.a{fill:url("https://evil.example/a)b")}.b{fill:url(#g)}</style><rect class="a" /></svg>',
);
expect(block).not.toContain('evil.example');
expect(block).toContain('url(#g)');
const attr = sanitizeSvg(
'<svg><rect style="fill:url(&quot;https://evil.example/a)b&quot;)" width="10" height="10" /></svg>',
);
expect(attr).not.toContain('evil.example');
expect(clean).not.toContain('<style');
expect(clean).not.toContain('style=');
expect(clean).toContain('fill="#e00"');
});
it('drops href-smuggling animation elements', () => {

View file

@ -113,130 +113,19 @@ export function detectMonochrome(src: string): Promise<boolean> {
});
}
/** Matches every `url(...)` reference in a CSS/presentation value. A quoted
* target may contain `)` (capture groups 1/2), an unquoted one may not (group 3),
* so the target is `match[1] ?? match[2] ?? match[3]`. */
const CSS_URL_REFERENCE = /url\(\s*(?:"([^"]*)"|'([^']*)'|([^'")]*))\s*\)/gi;
/** XML predefined entities the only named references a `data:image/svg+xml`
* document (parsed as XML) decodes; unknown named entities make it fail to parse. */
const XML_NAMED_ENTITIES: Record<string, string> = {
amp: '&',
lt: '<',
gt: '>',
quot: '"',
apos: "'",
};
/**
* Decodes the XML character references a browser resolves when it parses the
* stored `image/svg+xml` document, so `&#64;import` / `&#x40;import` are seen as
* `@import` before the CSS matchers run (SVG `<style>` text is otherwise raw and
* reaches the scrubber still entity-encoded). Single pass, matching the parser.
*/
export function decodeXmlEntities(text: string): string {
if (text.indexOf('&') === -1) {
return text;
}
return text.replace(
/&#(\d+);|&#[xX]([0-9a-fA-F]+);|&(amp|lt|gt|quot|apos);/g,
(match, dec?: string, hex?: string, named?: string) => {
if (named !== undefined) {
return XML_NAMED_ENTITIES[named];
}
const code = dec !== undefined ? Number.parseInt(dec, 10) : Number.parseInt(hex ?? '', 16);
if (code === 0 || code > 0x10ffff || (code >= 0xd800 && code <= 0xdfff)) {
return match;
}
return String.fromCodePoint(code);
},
);
}
/** Matches a single CSS escape: `\` + up to six hex digits (with an optional
* trailing whitespace the browser consumes), or `\` + any other character. */
const CSS_ESCAPE = /\\(?:([0-9a-fA-F]{1,6})\s?|(.))/g;
/**
* Resolves CSS escape sequences the way a browser does at tokenization time, so
* an obfuscated reference like `u\72l(…)` or `\40import` is seen as the `url()`
* / `@import` it becomes before the literal matchers run. Single pass, matching
* the browser: `u\5c72l` stays a literal backslash and is not a `url` token.
*/
export function unescapeCss(value: string): string {
if (!value.includes('\\')) {
return value;
}
return value.replace(CSS_ESCAPE, (_match, hex?: string, char?: string) => {
if (hex === undefined) {
return char ?? '';
}
const code = Number.parseInt(hex, 16);
if (code === 0 || code > 0x10ffff || (code >= 0xd800 && code <= 0xdfff)) {
return '<27>';
}
return String.fromCodePoint(code);
});
}
/** Resolves the character references and CSS escapes a browser would, in that
* order, so an obfuscated reference is seen as the token it becomes. */
function resolveCssRefs(value: string): string {
return unescapeCss(decodeXmlEntities(value));
}
/** True when a value carries a `url(...)` reference that is not a same-document
* fragment, e.g. `url(https://…)`, `url(//…)`, `url(data:…)`, or `url(x.svg#id)`. */
export function hasExternalUrlReference(value: string): boolean {
CSS_URL_REFERENCE.lastIndex = 0;
let match: RegExpExecArray | null;
const decoded = resolveCssRefs(value);
while ((match = CSS_URL_REFERENCE.exec(decoded)) !== null) {
const target = (match[1] ?? match[2] ?? match[3] ?? '').trim();
if (!target.startsWith('#')) {
return true;
}
}
return false;
}
/**
* Neutralizes external references inside a `<style>` block or inline `style`
* (used by exporter SVGs that store multi-color paint in class rules) while
* keeping local rules intact. Resolves entities and CSS escapes first so an
* obfuscated reference cannot hide, then strips comments and `@import` at-rules
* and rewrites any non-fragment `url(...)` to `none`.
*/
export function sanitizeCssText(css: string): string {
return resolveCssRefs(css)
.replace(/\/\*[\s\S]*?\*\//g, '')
.replace(/@import[^;]*;?/gi, '')
.replace(CSS_URL_REFERENCE, (full, q1, q2, q3) => {
const target = (q1 ?? q2 ?? q3 ?? '').trim();
return target.startsWith('#') ? full : 'none';
});
}
let svgPurifier: ReturnType<typeof DOMPurify> | null = null;
/**
* Dedicated DOMPurify instance for SVG icons, so the local-reference hooks never
* leak into the app's shared default instance. They keep same-document
* references (`href="#id"` on `<use>`/gradients and `url(#id)` paint/filter/clip
* values common exporter output) while stripping every external, relative, or
* scheme-carrying `href` or `url(...)`, and scrub `<style>` blocks so an internal
* stylesheet keeps its local paint rules but cannot `@import` or fetch externally.
* Dedicated DOMPurify instance for SVG icons, so the local-reference hook never
* leaks into the app's shared default instance. It keeps same-document
* `href="#id"` references on `<use>` and gradients while stripping external,
* relative, or scheme-carrying hrefs.
*/
function getSvgPurifier(): ReturnType<typeof DOMPurify> {
if (svgPurifier) {
return svgPurifier;
}
svgPurifier = DOMPurify(window);
svgPurifier.addHook('uponSanitizeElement', (node) => {
if (node.nodeName?.toLowerCase() === 'style' && node.textContent) {
node.textContent = sanitizeCssText(node.textContent);
}
});
svgPurifier.addHook('afterSanitizeAttributes', (node) => {
for (const attr of ['href', 'xlink:href']) {
const value = node.getAttribute(attr);
@ -244,32 +133,24 @@ function getSvgPurifier(): ReturnType<typeof DOMPurify> {
node.removeAttribute(attr);
}
}
for (const attr of Array.from(node.attributes)) {
if (attr.name === 'style') {
node.setAttribute('style', sanitizeCssText(attr.value));
} else if (hasExternalUrlReference(attr.value)) {
node.removeAttribute(attr.name);
}
}
});
return svgPurifier;
}
/**
* Strips active and external-referencing content from user-provided SVG markup,
* leaving only safe drawing elements. The `svg`/`svgFilters` profiles restrict
* the tag set and DOMPurify drops every `on*` handler by default; on top of that
* the forbidden tags remove embedded HTML (`foreignObject`), scripts, links, and
* animation, while `<use>` and `<style>` are re-allowed with hrefs and CSS
* references restricted to same-document fragments by the purifier hooks. These
* icons only ever render in inert `<img>`/CSS-mask contexts isolated from the
* host page, so a script-free, externally-inert `<style>` is safe to keep.
* Strips active content from user-provided SVG markup, leaving safe drawing
* elements and presentation attributes. The `svg`/`svgFilters` profiles restrict
* the tag set and DOMPurify drops every `on*` handler by default; the forbidden
* tags and attributes additionally remove embedded HTML, scripts, stylesheets,
* links, animation, and inline CSS. `<use>` is re-allowed with hrefs restricted
* to same-document fragments by the purifier hook.
*/
export function sanitizeSvg(svg: string): string {
return getSvgPurifier().sanitize(svg, {
USE_PROFILES: { svg: true, svgFilters: true },
ADD_TAGS: ['use', 'style'],
FORBID_TAGS: ['script', 'foreignObject', 'a', 'image', 'animate', 'set'],
ADD_TAGS: ['use'],
FORBID_TAGS: ['script', 'foreignObject', 'style', 'a', 'image', 'animate', 'set'],
FORBID_ATTR: ['style'],
});
}

View file

@ -94,10 +94,9 @@ class FakeContext {
return new ImageData(1, 1);
}
const data = Uint8ClampedArray.from(icon.pixels);
/* `ImageData(data, width)` the second arg is the row width in pixels, so
* this yields a 1-row image. `scanMonochrome` walks the flat RGBA buffer and
* ignores geometry, so a single row is all the detector needs. */
return new ImageData(data, data.length / 4);
/* `scanMonochrome` walks the flat RGBA buffer and ignores geometry, so a
* single row is all the detector needs. */
return new ImageData(data, data.length / 4, 1);
}
}

Binary file not shown.

View file

@ -3,10 +3,10 @@ import { MAX_MCP_ICON_PATH_LENGTH } from 'librechat-data-provider';
/**
* Server-side sanitization for user-provided MCP server icons. The client
* sanitizes uploaded SVGs before inlining them, but that runs in the browser
* sanitizes uploaded SVGs before encoding them as data URIs, but that runs in the browser
* and is trivially bypassed by posting an `iconPath` straight to the API, so
* every stored icon is re-sanitized here at the trust boundary before it is
* persisted and served back to other users.
* persisted and returned to other users in MCP configuration responses.
*
* Only `data:image/svg+xml` values carry active content worth stripping; raster
* data URIs, `http(s)` URLs, and relative paths render inertly through `<img>`
@ -47,7 +47,6 @@ const ALLOWED_SVG_TAGS = [
'pattern',
'title',
'desc',
'style',
'filter',
'feBlend',
'feColorMatrix',
@ -124,7 +123,6 @@ const ALLOWED_SVG_ATTRS = [
'preserveAspectRatio',
'id',
'class',
'style',
'href',
'xlink:href',
'filter',
@ -181,127 +179,9 @@ const ALLOWED_SVG_ATTRS = [
'tableValues',
];
/** Matches every `url(...)` reference in a presentation/style value. A quoted
* target may contain `)` (capture groups 1/2), an unquoted one may not (group 3),
* so the target is `match[1] ?? match[2] ?? match[3]`. */
const CSS_URL_REFERENCE = /url\(\s*(?:"([^"]*)"|'([^']*)'|([^'")]*))\s*\)/gi;
/** The target of a `CSS_URL_REFERENCE` match, from whichever quoting group hit. */
function urlTarget(match: RegExpExecArray | RegExpMatchArray): string {
return (match[1] ?? match[2] ?? match[3] ?? '').trim();
}
/** XML predefined entities the only named references a `data:image/svg+xml`
* document (parsed as XML) decodes; unknown named entities make it fail to parse. */
const XML_NAMED_ENTITIES: Record<string, string> = {
amp: '&',
lt: '<',
gt: '>',
quot: '"',
apos: "'",
};
/**
* Decodes the XML character references a browser resolves when it parses the
* stored `image/svg+xml` document, so `&#64;import` / `&#x40;import` are seen as
* `@import` before the CSS matchers run (SVG `<style>` text is otherwise raw and
* reaches this scrubber still entity-encoded). Single pass, matching the parser.
*/
function decodeXmlEntities(text: string): string {
if (text.indexOf('&') === -1) {
return text;
}
return text.replace(
/&#(\d+);|&#[xX]([0-9a-fA-F]+);|&(amp|lt|gt|quot|apos);/g,
(match, dec: string | undefined, hex: string | undefined, named: string | undefined) => {
if (named !== undefined) {
return XML_NAMED_ENTITIES[named];
}
const code = dec !== undefined ? Number.parseInt(dec, 10) : Number.parseInt(hex ?? '', 16);
if (code === 0 || code > 0x10ffff || (code >= 0xd800 && code <= 0xdfff)) {
return match;
}
return String.fromCodePoint(code);
},
);
}
/** Matches a single CSS escape: `\` + up to six hex digits (with an optional
* trailing whitespace the browser consumes), or `\` + any other character. */
const CSS_ESCAPE = /\\(?:([0-9a-fA-F]{1,6})\s?|(.))/g;
/**
* Resolves CSS escape sequences the way a browser does at tokenization time, so
* an obfuscated reference like `u\72l(…)` or `\40import` is seen as the `url()`
* / `@import` it becomes before the literal matchers run. Single pass, matching
* the browser: `u\5c72l` stays a literal backslash and is not a `url` token.
*/
function unescapeCss(value: string): string {
if (!value.includes('\\')) {
return value;
}
return value.replace(CSS_ESCAPE, (_match, hex: string | undefined, char: string | undefined) => {
if (hex === undefined) {
return char ?? '';
}
const code = Number.parseInt(hex, 16);
if (code === 0 || code > 0x10ffff || (code >= 0xd800 && code <= 0xdfff)) {
return '<27>';
}
return String.fromCodePoint(code);
});
}
/** Resolves the character references and CSS escapes a browser would, in that
* order, so an obfuscated reference is seen as the token it becomes. */
function resolveCssRefs(value: string): string {
return unescapeCss(decodeXmlEntities(value));
}
/** True when a value carries a `url(...)` reference that is not a same-document
* fragment, e.g. `url(https://…)`, `url(//…)`, `url(data:…)`, or `url(x.svg#id)`. */
function hasExternalUrlReference(value: string): boolean {
CSS_URL_REFERENCE.lastIndex = 0;
let match: RegExpExecArray | null;
const decoded = resolveCssRefs(value);
while ((match = CSS_URL_REFERENCE.exec(decoded)) !== null) {
if (!urlTarget(match).startsWith('#')) {
return true;
}
}
return false;
}
/** Resolves entities/escapes, then strips comments and `@import` at-rules and
* rewrites external `url(...)` to `none`, keeping only local paint rules from a
* stylesheet or inline `style`. References are resolved first so an obfuscated
* one cannot hide from the matchers. */
function sanitizeCssText(css: string): string {
return resolveCssRefs(css)
.replace(/\/\*[\s\S]*?\*\//g, '')
.replace(/@import[^;]*;?/gi, '')
.replace(CSS_URL_REFERENCE, (full, q1, q2, q3) => {
const target = (q1 ?? q2 ?? q3 ?? '').trim();
return target.startsWith('#') ? full : 'none';
});
}
/** Scrubs the CSS inside every `<style>` block of an already-tag-sanitized SVG so
* an internal stylesheet keeps its local class paints but cannot fetch externally. */
function scrubStyleBlocks(svg: string): string {
return svg.replace(
/(<style\b[^>]*>)([\s\S]*?)(<\/style>)/gi,
(_full, open, css, close) => `${open}${sanitizeCssText(css)}${close}`,
);
}
/**
* Drops references that leave the document: any `href`/`xlink:href` that is not a
* same-document fragment, and any single-value presentation attribute (`filter`,
* `fill`, `mask`, `clip-path`, ) carrying a non-fragment `url(...)`. The
* multi-declaration `style` attribute is scrubbed in place instead so its local
* paint rules survive. All checks resolve CSS escapes first (see `unescapeCss`),
* mirroring the client-side `sanitizeSvg` rule so stored icons cannot fetch.
* Drops `href`/`xlink:href` references that leave the document while preserving
* same-document fragments used by `<use>` and gradients.
*/
function keepLocalReferences(tagName: string, attribs: sanitizeHtml.Attributes): sanitizeHtml.Tag {
for (const [name, value] of Object.entries(attribs)) {
@ -309,14 +189,6 @@ function keepLocalReferences(tagName: string, attribs: sanitizeHtml.Attributes):
if (!value.trim().startsWith('#')) {
delete attribs[name];
}
continue;
}
if (name === 'style') {
attribs[name] = sanitizeCssText(value);
continue;
}
if (hasExternalUrlReference(value)) {
delete attribs[name];
}
}
return { tagName, attribs };
@ -327,17 +199,12 @@ function keepLocalReferences(tagName: string, attribs: sanitizeHtml.Attributes):
* sensitive SVG names (`viewBox`, `linearGradient`, `clipPath`, ) survive the
* round-trip; lowercasing them would break rendering. `allowedSchemes` is empty
* as a second layer behind the fragment-only href transform: a fragment carries
* no scheme, so nothing legitimate is affected. `<style>` is allowed
* (`allowVulnerableTags`) because these icons only render in inert `<img>`/CSS-mask
* contexts isolated from any host page no script runs and, after `scrubStyleBlocks`
* removes `@import`/external `url()`, no subresource loads so an internal
* stylesheet's local paint rules are safe to keep.
* no scheme, so nothing legitimate is affected.
*/
const SVG_SANITIZE_OPTIONS: sanitizeHtml.IOptions = {
allowedTags: ALLOWED_SVG_TAGS,
allowedAttributes: { '*': ALLOWED_SVG_ATTRS },
allowedSchemes: [],
allowVulnerableTags: true,
transformTags: { '*': keepLocalReferences },
parser: { lowerCaseTags: false, lowerCaseAttributeNames: false },
};
@ -410,13 +277,7 @@ export function sanitizeMcpIconPath(iconPath: string): string {
if (svg == null) {
return '';
}
const sanitized = sanitizeHtml(svg, SVG_SANITIZE_OPTIONS);
const scrubbed = scrubStyleBlocks(sanitized);
/* `scrubStyleBlocks` splices un-escaped CSS back as raw markup, so an escaped
* sequence like `\3c/style\3e\3cimage/\3e` can reintroduce a real element past
* the allowlist. When a `<style>` block was actually rewritten, re-run the
* allowlist over the result to strip anything the un-escaping surfaced. */
const clean = scrubbed === sanitized ? sanitized : sanitizeHtml(scrubbed, SVG_SANITIZE_OPTIONS);
const clean = sanitizeHtml(svg, SVG_SANITIZE_OPTIONS);
const encoded = `data:image/svg+xml;base64,${Buffer.from(clean, 'utf-8').toString('base64')}`;
return encoded.length > MAX_MCP_ICON_PATH_LENGTH ? '' : encoded;
}