fix: enforce icon length cap in the sanitizer, not the schema

The schema `.max()` on iconPath rejected the whole update at parse time.
A server whose stored icon predates the cap (previous versions allowed
large data-URI images) re-submits that value from the edit dialog, so
the user was locked out of changing any field or clearing the bad icon.

Drop the schema `.max()` and enforce MAX_MCP_ICON_PATH_LENGTH in
sanitizeMcpIconPath for every value type: an over-cap SVG is compacted
or dropped, an over-cap raster/URL is dropped. Editing a server with a
pre-existing oversized icon now succeeds and clears the icon instead of
failing validation.
This commit is contained in:
Marco Beretta 2026-07-05 04:19:19 +02:00
parent e3eba21b71
commit 127bcbd1c9
No known key found for this signature in database
GPG key ID: D918033D8E74CC11
4 changed files with 40 additions and 12 deletions

View file

@ -239,8 +239,8 @@ describe('sanitizeMcpIconPath', () => {
it('never stores an icon over the length cap even when sanitizing grows it', () => {
// A base64 input under the cap whose many self-closing tags expand under
// sanitization (explicit close tags) past the cap; it must be dropped, not
// stored over-limit where the next edit's re-validation would reject it.
// sanitization (explicit close tags) past the cap; it must be dropped rather
// than stored over-limit.
const cell = '<rect x="1" y="1" width="2" height="2" fill="#abc"/>';
const raw = `<svg>${cell.repeat(3400)}</svg>`;
const input = `data:image/svg+xml;base64,${Buffer.from(raw, 'utf-8').toString('base64')}`;
@ -249,4 +249,15 @@ describe('sanitizeMcpIconPath', () => {
expect(out.length).toBeLessThanOrEqual(MAX_MCP_ICON_PATH_LENGTH);
expect(out).toBe('');
});
it('drops an over-cap non-SVG value (raster data URI) that cannot be compacted', () => {
const huge = `data:image/png;base64,${'A'.repeat(MAX_MCP_ICON_PATH_LENGTH)}`;
expect(huge.length).toBeGreaterThan(MAX_MCP_ICON_PATH_LENGTH);
expect(sanitizeMcpIconPath(huge)).toBe('');
});
it('passes an under-cap non-SVG value through unchanged', () => {
const ok = `data:image/png;base64,${'A'.repeat(1000)}`;
expect(sanitizeMcpIconPath(ok)).toBe(ok);
});
});

View file

@ -343,20 +343,20 @@ function normalizeIconValue(value: string): string {
* Sanitize a user-provided MCP `iconPath`. SVG data URIs are decoded, stripped
* of active content via an allowlist, and re-encoded as base64; a malformed SVG
* data URI resolves to an empty string so a broken icon is stored rather than
* raw markup. All other values (raster data URIs, URLs, relative paths) are
* returned unchanged.
* raw markup. Other values (raster data URIs, URLs, relative paths) pass through
* unchanged unless they exceed the length cap.
*
* Base64 is far more compact than percent-encoding for the angle-bracket-heavy
* SVG markup that sanitizing can even slightly grow (it expands self-closing
* tags to explicit close tags), so it minimizes needless rejection. The result
* is then measured against the schema length cap and dropped if it still exceeds
* it, so a value that passed the cap on input can never be stored over it and
* then rejected when the prefilled value is resubmitted on the next edit.
* This is the single enforcement point for `MAX_MCP_ICON_PATH_LENGTH`: any value
* still over the cap after sanitizing is dropped to an empty string. Enforcing it
* here rather than as a schema `.max()` means editing a server whose stored icon
* predates the cap succeeds (the oversized icon is cleared) instead of failing
* validation and locking the user out of the whole update. Base64 also keeps SVG
* output compact so a legitimate icon is rarely dropped.
*/
export function sanitizeMcpIconPath(iconPath: string): string {
const normalized = normalizeIconValue(iconPath);
if (!SVG_DATA_URI.test(normalized)) {
return iconPath;
return iconPath.length > MAX_MCP_ICON_PATH_LENGTH ? '' : iconPath;
}
const svg = decodeSvgDataUri(normalized);
if (svg == null) {

View file

@ -4,6 +4,7 @@ import {
StreamableHTTPOptionsSchema,
MCPServerUserInputSchema,
MCP_USER_INPUT_FIELDS,
MAX_MCP_ICON_PATH_LENGTH,
} from '../src/mcp';
describe('MCPOptionsSchema', () => {
@ -182,6 +183,17 @@ describe('MCP schemas', () => {
});
});
describe('iconPath', () => {
it('accepts an over-limit iconPath so editing a server with a pre-existing oversized icon is not rejected (the cap is enforced server-side by sanitizeMcpIconPath, not at parse time)', () => {
const result = MCPServerUserInputSchema.safeParse({
type: 'streamable-http',
url: 'https://mcp-server.com/http',
iconPath: `data:image/png;base64,${'A'.repeat(MAX_MCP_ICON_PATH_LENGTH + 1000)}`,
});
expect(result.success).toBe(true);
});
});
describe('env variable rejection', () => {
it('should reject SSE URLs containing env variable patterns', () => {
const result = MCPServerUserInputSchema.safeParse({

View file

@ -6,6 +6,11 @@ import { extractEnvVariable } from './utils';
* Upper bound on a stored MCP `iconPath` string (URL or inlined data URI).
* A legitimate icon is a few KB; this caps a malicious or accidental multi-MB
* data URI that would otherwise be persisted and shipped in every server list.
*
* Enforced server-side by `sanitizeMcpIconPath` (which drops or compacts an
* over-limit value), NOT as a schema `.max()`. A parse-time cap would reject the
* whole update when the edit dialog re-submits a pre-existing over-limit icon,
* locking the user out of editing other fields or clearing the bad icon.
*/
export const MAX_MCP_ICON_PATH_LENGTH = 256 * 1024;
@ -176,7 +181,7 @@ const BaseOptionsSchema = z.object({
* requiring manual authentication (e.g., GitHub PAT tokens) that need to be configured through the UI after startup
*/
startup: z.boolean().optional(),
iconPath: z.string().max(MAX_MCP_ICON_PATH_LENGTH).optional(),
iconPath: z.string().optional(),
timeout: z.number().int().nonnegative().optional(),
/** Timeout (ms) for the long-lived SSE GET stream body before undici aborts it. Default: 300_000 (5 min). */
sseReadTimeout: z.number().int().positive().optional(),