diff --git a/backend/__tests__/migrations/200_strip_remote_urls_from_css_templates.test.js b/backend/__tests__/migrations/200_strip_remote_urls_from_css_templates.test.js new file mode 100644 index 00000000..bbfdde79 --- /dev/null +++ b/backend/__tests__/migrations/200_strip_remote_urls_from_css_templates.test.js @@ -0,0 +1,194 @@ +/** + * Repairing stored CSS templates that carry remote url() references. + * + * The migration exists because the public render path (gallery.js, GET + * /gallery/:slug/css) serves `css_templates.css_content` VERBATIM to + * visitors — it does not re-sanitize on read. Fixing the sanitizer alone + * would only protect newly-saved templates; rows already carrying a remote + * URL would keep serving it forever. + * + * As with migration 181, most of what is pinned here is what it must NOT + * touch: inline data: images, and stylesheets that were already clean. + */ + +const path = require('path'); +const fs = require('fs'); +const os = require('os'); + +const migration = require('../../migrations/core/200_strip_remote_urls_from_css_templates'); + +const WITH_REMOTE = ` +.gallery-page { + background: #111 url(https://tracker.example/pixel.gif) no-repeat; + color: #eee; +} + +.photo-card { + border-radius: 12px; +} +`; + +const WITH_DATA_URI = ` +.photo-card { + background-image: url(data:image/png;base64,iVBORw0KGgo=); + border-radius: 8px; +} +`; + +const ALREADY_CLEAN = ` +.gallery-page { + background: #fff; + color: #222; +} +`; + +// What the old, broken sanitizer actually left in the column. +const LEGACY_MARKER = '.a{background:/* BLOCKED URL */ url(https://tracker.example/pixel.gif)}'; + +describe('migration 200 — remote url() in CSS templates', () => { + let knex; let tmpDir; + + beforeAll(async () => { + tmpDir = await fs.promises.mkdtemp(path.join(os.tmpdir(), 'picpeak-mig200-')); + knex = require('knex')({ + client: 'sqlite3', + connection: { filename: path.join(tmpDir, 'db.sqlite') }, + useNullAsDefault: true, + }); + await knex.schema.createTable('css_templates', (t) => { + t.increments('id').primary(); + t.integer('slot_number'); + t.string('name'); + t.text('css_content'); + t.timestamp('updated_at'); + }); + await knex.schema.createTable('activity_logs', (t) => { + t.increments('id').primary(); + t.string('activity_type'); + t.string('actor_type'); + t.text('metadata'); + t.timestamp('created_at').defaultTo(knex.fn.now()); + t.timestamp('read_at'); + }); + }); + + afterAll(async () => { + if (knex) await knex.destroy(); + await fs.promises.rm(tmpDir, { recursive: true, force: true }).catch(() => {}); + }); + + beforeEach(async () => { + await knex('css_templates').del(); + await knex('activity_logs').del(); + }); + + const contentOf = async (name) => + (await knex('css_templates').where({ name }).first()).css_content; + + /** What a CSS parser sees — comments are discarded before parsing. */ + const asParsed = (css) => css.replace(/\/\*[\s\S]*?\*\//g, ''); + + it('removes a remote url() from a stored template', async () => { + await knex('css_templates').insert({ slot_number: 1, name: 'Tracked', css_content: WITH_REMOTE }); + + await migration.up(knex); + + const css = await contentOf('Tracked'); + expect(asParsed(css)).not.toContain('tracker.example'); + // The rest of the stylesheet survives. + expect(css).toContain('#111'); + expect(css).toContain('border-radius: 12px'); + }); + + it('repairs a row left behind by the old inert marker', async () => { + // The exact shape the broken sanitizer wrote: a comment that a parser + // discards, followed by the live URL. + await knex('css_templates').insert({ slot_number: 2, name: 'Legacy', css_content: LEGACY_MARKER }); + + await migration.up(knex); + + const css = await contentOf('Legacy'); + expect(asParsed(css)).not.toContain('tracker.example'); + expect(css).not.toContain('BLOCKED URL'); + }); + + it('leaves an inline data: image alone', async () => { + await knex('css_templates').insert({ slot_number: 3, name: 'Inline', css_content: WITH_DATA_URI }); + + await migration.up(knex); + + expect(await contentOf('Inline')).toContain('data:image/png;base64,iVBORw0KGgo='); + }); + + it('does not touch a template that was already clean', async () => { + await knex('css_templates').insert({ slot_number: 4, name: 'Clean', css_content: ALREADY_CLEAN }); + + await migration.up(knex); + + expect(await contentOf('Clean')).toBe(ALREADY_CLEAN); + }); + + it('names every repaired slot in an activity log entry', async () => { + await knex('css_templates').insert([ + { slot_number: 1, name: 'Tracked', css_content: WITH_REMOTE }, + { slot_number: 2, name: 'Legacy', css_content: LEGACY_MARKER }, + { slot_number: 3, name: 'Clean', css_content: ALREADY_CLEAN }, + ]); + + await migration.up(knex); + + const log = await knex('activity_logs') + .where({ activity_type: 'css_template_remote_urls_removed' }).first(); + expect(log).toBeTruthy(); + const meta = JSON.parse(log.metadata); + expect(meta.count).toBe(2); + expect(meta.templates.map((t) => t.slot).sort()).toEqual([1, 2]); + // Unread, so it surfaces in the admin notification bell. + expect(log.read_at).toBeNull(); + }); + + it('writes no notification when nothing needed repair', async () => { + await knex('css_templates').insert({ slot_number: 1, name: 'Clean', css_content: ALREADY_CLEAN }); + + await migration.up(knex); + + expect(await knex('activity_logs').count({ c: '*' })).toEqual([{ c: 0 }]); + }); + + it('is idempotent — a re-run changes nothing and adds no second notification', async () => { + await knex('css_templates').insert({ slot_number: 1, name: 'Tracked', css_content: WITH_REMOTE }); + + await migration.up(knex); + const afterFirst = await contentOf('Tracked'); + await migration.up(knex); + + expect(await contentOf('Tracked')).toBe(afterFirst); + expect(await knex('activity_logs').count({ c: '*' })).toEqual([{ c: 1 }]); + }); + + it('skips rows with no CSS at all', async () => { + await knex('css_templates').insert({ slot_number: 5, name: 'Empty', css_content: null }); + + await expect(migration.up(knex)).resolves.not.toThrow(); + }); + + it('no-ops on an install with no css_templates table', async () => { + const bare = require('knex')({ + client: 'sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true, + }); + await expect(migration.up(bare)).resolves.not.toThrow(); + await bare.destroy(); + }); + + it('down() is a deliberate no-op', async () => { + await knex('css_templates').insert({ slot_number: 1, name: 'Tracked', css_content: WITH_REMOTE }); + await migration.up(knex); + const repaired = await contentOf('Tracked'); + + await migration.down(knex); + + // A rollback must not silently re-introduce third-party requests into + // pages served to visitors. + expect(await contentOf('Tracked')).toBe(repaired); + }); +}); diff --git a/backend/__tests__/utils/cssSanitizer.remoteUrls.test.js b/backend/__tests__/utils/cssSanitizer.remoteUrls.test.js new file mode 100644 index 00000000..4153406f --- /dev/null +++ b/backend/__tests__/utils/cssSanitizer.remoteUrls.test.js @@ -0,0 +1,257 @@ +/** + * cssSanitizer — remote url() removal. + * + * Regression: `sanitizeCSS` used to "block" a remote url() by prefixing it + * with a `/* BLOCKED URL *\/` COMMENT and leaving the URL in place. CSS + * comments are discarded during tokenization, so the declaration a browser + * actually parsed still carried the live URL — while the returned warning + * told the caller it had been blocked. + * + * The load-bearing assertion in most of these is therefore not "the marker is + * gone" but "the HOST is gone", checked against the comment-stripped text the + * way a parser would see it. + */ + +const { sanitizeCSS, sanitizeCss, stripDisallowedUrls } = require('../../src/utils/cssSanitizer'); + +/** What a CSS parser sees: comments are discarded before parsing. */ +const asParsed = (css) => css.replace(/\/\*[\s\S]*?\*\//g, ''); + +describe('stripDisallowedUrls', () => { + it('removes a remote url() rather than commenting near it', () => { + const { sanitized, blocked } = stripDisallowedUrls('.a{background:url(https://evil.example/p.gif)}'); + + expect(blocked).toBe(1); + expect(sanitized).not.toContain('evil.example'); + expect(asParsed(sanitized)).not.toContain('evil.example'); + }); + + it.each([ + ['unquoted', '.a{background:url(https://evil.example/p.gif)}'], + ['single-quoted', '.a{background:url(\'https://evil.example/p.gif\')}'], + ['double-quoted', '.a{background:url("https://evil.example/p.gif")}'], + ['spaced', '.a{background:url( https://evil.example/p.gif )}'], + ['uppercase URL(', '.a{background:URL(https://evil.example/p.gif)}'], + ['protocol-relative', '.a{background:url(//evil.example/p.gif)}'], + ['scheme-less host', '.a{background:url(evil.example/p.gif)}'], + ['http', '.a{background:url(http://evil.example/p.gif)}'], + ])('removes a %s remote url()', (_label, css) => { + expect(asParsed(stripDisallowedUrls(css).sanitized)).not.toContain('evil.example'); + }); + + it('keeps an inline data: image', () => { + const css = '.a{background:url(data:image/png;base64,iVBORw0KGgo=)}'; + const { sanitized, blocked } = stripDisallowedUrls(css); + + expect(blocked).toBe(0); + expect(sanitized).toBe(css); + }); + + it.each(['jpeg', 'jpg', 'png', 'gif', 'webp'])('keeps a data:image/%s URI', (type) => { + const css = `.a{background:url(data:image/${type};base64,AAAA)}`; + expect(stripDisallowedUrls(css).sanitized).toBe(css); + }); + + it('removes a data: URI that is not a raster image', () => { + // data:image/svg+xml is a script-execution vector in some contexts. + const out = stripDisallowedUrls('.a{background:url(data:image/svg+xml;base64,AAAA)}').sanitized; + expect(out).not.toContain('svg+xml'); + }); + + it('leaves the rest of a shorthand declaration intact', () => { + const out = stripDisallowedUrls('.a{background:#fff url(https://x/p.gif) no-repeat center}').sanitized; + + expect(out).toContain('#fff'); + expect(out).toContain('no-repeat center'); + expect(out).toContain('none'); + expect(out).not.toContain('https://x'); + }); + + it('counts and removes every offender in one stylesheet', () => { + const { sanitized, blocked } = stripDisallowedUrls( + '.a{background:url(https://a.example/1.gif)}' + + '.b{background:url(https://b.example/2.gif)}' + + '.c{background:url(data:image/png;base64,AAAA)}' + ); + + expect(blocked).toBe(2); + expect(sanitized).not.toContain('a.example'); + expect(sanitized).not.toContain('b.example'); + expect(sanitized).toContain('data:image/png'); + }); + + // Codex review: `)` is ordinary content inside a QUOTED url(), so a pattern + // that stops at the first `)` failed to match at all — reporting the token + // as clean and storing it verbatim. A working bypass of the block. + it.each([ + ['double-quoted', '.a{background:url("https://evil.example/pixel).gif")}'], + ['single-quoted', ".a{background:url('https://evil.example/pixel).gif')}"], + ['several parens', '.a{background:url("https://evil.example/a)b)c.gif")}'], + ])('removes a %s remote url() containing a closing paren', (_label, css) => { + const { sanitized, blocked } = stripDisallowedUrls(css); + expect(blocked).toBe(1); + expect(asParsed(sanitized)).not.toContain('evil.example'); + }); + + it('keeps a QUOTED data: image, parens and all', () => { + const css = '.a{background:url("data:image/png;base64,AAAA")}'; + expect(stripDisallowedUrls(css).sanitized).toBe(css); + }); + + it('does not swallow an unquoted url() that never closes', () => { + // Malformed input must not eat the rest of the stylesheet. + const css = '.a{background:url(https://evil.example/p.gif}.b{color:red}'; + const { sanitized } = stripDisallowedUrls(css); + expect(sanitized).toContain('color:red'); + }); + + // Round-2 review: three more ways a regex could not see CSS structure. + it('blocks a url() hidden behind a CSS escape', () => { + // `\\72` is a legal way to write `r`, so this IS url(...) to a browser. + const { sanitized, blocked } = stripDisallowedUrls('.a{background:u\\72l(https://evil.example/p.gif)}'); + expect(blocked).toBe(1); + expect(asParsed(sanitized)).not.toContain('evil.example'); + }); + + it('leaves url()-looking text inside a CSS string alone', () => { + // `content:` is inert display text, not a resource request. Rewriting it + // is a visible change to a page that never fetched anything. + const css = '.a::after{content:"url(https://docs.example)"}'; + const { sanitized, blocked } = stripDisallowedUrls(css); + expect(blocked).toBe(0); + expect(sanitized).toBe(css); + }); + + it('does not treat a ) inside a quoted string as the end of a token', () => { + const css = '.a::after{content:"a) b"}.c{background:url(https://evil.example/x.gif)}'; + const { sanitized } = stripDisallowedUrls(css); + expect(sanitized).toContain('content:"a) b"'); + expect(asParsed(sanitized)).not.toContain('evil.example'); + }); + + // Round-3 review: all three came from decoding escapes globally before + // scanning, which destroyed the difference between an escaped quote and a + // real one — and rewrote clean escapes that were never URLs. + it('does not let an escaped quote desynchronise the scan', () => { + // `\\"` inside a double-quoted string does NOT close it. Treating it as a + // delimiter put the scanner a quote out of phase and let the url() that + // followed through untouched. + const css = '.a{content:"foo\\"";background:url(https://evil.example/x)}'; + const { sanitized, blocked } = stripDisallowedUrls(css); + expect(blocked).toBe(1); + expect(asParsed(sanitized)).not.toContain('evil.example'); + }); + + it('does not treat a quote inside a CSS comment as a string', () => { + // A browser ignores the comment and makes the request; the scanner used + // to enter string mode on the apostrophe and copy the rest unscanned. + const css = "/* don't */ .a{background:url(https://evil.example/x)}"; + const { sanitized, blocked } = stripDisallowedUrls(css); + expect(blocked).toBe(1); + expect(asParsed(sanitized)).not.toContain('evil.example'); + }); + + it('leaves an escaped selector byte-identical', () => { + // `.w-1\\/2` is an ordinary escaped Tailwind class. Global decoding + // rewrote it to `.w-1/2` — a DIFFERENT selector — and migration 200 then + // committed that irreversibly, since its down() is a no-op. + const css = '.w-1\\/2{color:red}'; + const { sanitized, blocked } = stripDisallowedUrls(css); + expect(blocked).toBe(0); + expect(sanitized).toBe(css); + }); + + it('returns clean input unchanged, whatever escapes it carries', () => { + // The migration only writes when the bytes differ, so "unchanged" is + // what keeps a clean row out of the repair path entirely. + for (const css of [ + '.a{color:red}', + '/* a comment */ .b{color:blue}', + '.c\\:hover{color:green}', + '.d::after{content:"\\201C"}', + '.e{background:url(data:image/png;base64,AAAA)}', + ]) { + const { sanitized, blocked } = stripDisallowedUrls(css); + expect(blocked).toBe(0); + expect(sanitized).toBe(css); + } + }); + + it('keeps a comment that merely mentions a url', () => { + const css = '/* see url(https://docs.example) for details */ .a{color:red}'; + expect(stripDisallowedUrls(css).sanitized).toBe(css); + }); + + it('is idempotent', () => { + const once = stripDisallowedUrls('.a{background:url(https://x/p.gif)}').sanitized; + expect(stripDisallowedUrls(once).sanitized).toBe(once); + }); + + it('handles empty and nullish input', () => { + expect(stripDisallowedUrls('').sanitized).toBe(''); + expect(stripDisallowedUrls(null).sanitized).toBe(''); + expect(stripDisallowedUrls(undefined).sanitized).toBe(''); + }); +}); + +describe('sanitizeCSS', () => { + it('no longer emits an inert BLOCKED URL marker', () => { + const { sanitized } = sanitizeCSS('.a{background:url(https://evil.example/p.gif)}'); + + expect(sanitized).not.toContain('BLOCKED URL'); + expect(asParsed(sanitized)).not.toContain('evil.example'); + }); + + it('warns with the number it actually removed', () => { + const { warnings } = sanitizeCSS( + '.a{background:url(https://a.example/1.gif)}.b{background:url(https://b.example/2.gif)}' + ); + expect(warnings.some((w) => /Blocked 2 external URL references/.test(w))).toBe(true); + }); + + it('does not warn about URLs when there are none', () => { + const { warnings } = sanitizeCSS('.a{color:red}'); + expect(warnings.filter((w) => /external URL/.test(w))).toHaveLength(0); + }); + + it('blocks a url() that only becomes one after HTML comments are removed', () => { + // The comment strip JOINS the remaining characters into a live url(...). + // A scan that ran before it saw no token and called the input clean. + const { sanitized } = sanitizeCSS('.a{background:url(https://evil.example/p.gif)}'); + expect(asParsed(sanitized)).not.toContain('evil.example'); + }); + + it('still blocks the other forbidden patterns', () => { + const { sanitized } = sanitizeCSS( + '@import url("https://x/e.css"); .a{width:expression(alert(1));behavior:url(e.htc)}' + ); + expect(sanitized).not.toContain('@import'); + expect(sanitized).not.toContain('expression('); + expect(asParsed(sanitized)).not.toContain('https://x/e.css'); + }); + + it('neutralises a javascript: url()', () => { + const { sanitized } = sanitizeCSS('.a{background:url(javascript:alert(1))}'); + expect(asParsed(sanitized)).not.toContain('javascript:'); + }); + + it('leaves ordinary declarations untouched', () => { + const css = '.btn { color: #fff; padding: 12px 24px; border-radius: 6px; }'; + expect(sanitizeCSS(css).sanitized).toBe(css); + }); +}); + +describe('sanitizeCss (lowercase) — public-site path, deliberately unchanged', () => { + it('still permits a remote url()', () => { + // This function never blocked remote URLs and never claimed to. The + // public-site CSS surface may legitimately reference a remote font or + // background; changing that is a product decision, not this bug fix. + const out = sanitizeCss('.a{background:url(https://cdn.example/x.png)}'); + expect(out).toContain('https://cdn.example/x.png'); + }); + + it('still strips @import and javascript: urls', () => { + expect(sanitizeCss('@import url("https://x/e.css"); .a{color:red}')).not.toContain('@import'); + expect(sanitizeCss('.a{background:url(\'javascript:alert(1)\')}')).not.toContain('javascript:'); + }); +}); diff --git a/backend/migrations/core/200_strip_remote_urls_from_css_templates.js b/backend/migrations/core/200_strip_remote_urls_from_css_templates.js new file mode 100644 index 00000000..d2e0eb2d --- /dev/null +++ b/backend/migrations/core/200_strip_remote_urls_from_css_templates.js @@ -0,0 +1,115 @@ +/** + * Remove remote `url()` references from stored CSS templates. + * + * `sanitizeCSS` was supposed to block them on write. It didn't: it prefixed + * the offending token with a `/* BLOCKED URL *\/` COMMENT and left the URL + * in place. CSS comments are discarded during tokenization, so what a browser + * parsed still carried the live URL: + * + * '.a{background:url(https://x/p.gif)}' + * stored as '.a{background:/* BLOCKED URL *\/ url(https://x/p.gif)}' + * parsed as '.a{background: url(https://x/p.gif)}' + * + * Meanwhile the route returned `sanitization_warnings: ["Blocked external URL + * references..."]`, so the admin was told it had been stopped. + * + * The sanitizer is fixed, but that only covers FUTURE writes. The public + * render path (`gallery.js`, GET /gallery/:slug/css) serves + * `css_templates.css_content` verbatim as text/css to gallery visitors — it + * does not re-sanitize on read. So without this pass every template already + * carrying a remote URL keeps serving it to every visitor forever, which is + * exactly the population the fix exists for. + * + * WHY A MIGRATION AND NOT A BOOT SELF-HEAL: once the sanitizer is fixed, the + * write path is the only way a bad row can appear, so there is nothing left + * to self-heal. This is a one-time data correction and belongs in the ledger. + * + * IRREVERSIBLE BY DESIGN: `down()` cannot restore the removed URLs — the + * original text is gone, and re-introducing third-party requests into pages + * served to visitors is not something a rollback should do silently. It is a + * no-op, deliberately. + * + * WHAT IT COSTS: a template using a remote background image has worked until + * now (because the block was inert) and will stop. That is the point, but it + * is a visible change, so every affected slot is named in an `activity_logs` + * entry — which is what the admin notification bell reads — rather than + * changing silently. Note the CSS is served with `Cache-Control: public, + * max-age=3600`, so a visitor may hold a cached copy for up to an hour after + * this runs. + * + * Inline `data:image/*` URIs are untouched. + */ + +const { stripDisallowedUrls } = require('../../src/utils/cssSanitizer'); + +exports.up = async function (knex) { + if (!(await knex.schema.hasTable('css_templates'))) return; + + const templates = await knex('css_templates') + .whereNotNull('css_content') + .select('id', 'slot_number', 'name', 'css_content'); + + const repaired = []; + + for (const template of templates) { + // ONLY the url() pass, not the whole of sanitizeCSS. + // + // Re-running the full sanitizer would be the obvious move and is wrong: + // it strips control characters, and `\n` is one — so every template would + // come back as a single line. Migration 181 hit the same edge from the + // other side. Reformatting every stylesheet in the install is far beyond + // "remove the remote URLs", and it would land in the editor the operator + // next opens. + // + // The url() pass is also the ONLY thing that was broken: `@import`, + // `expression(`, `behavior:` and friends were genuinely removed by the + // old code. So the narrow fix is also the complete one. + const { sanitized: urlSafe, blocked } = stripDisallowedUrls(template.css_content); + + // Drop the stale marker the broken sanitizer left behind. Only rows that + // carry one are touched, and by definition those are affected rows. + const sanitized = urlSafe.replace(/\/\*\s*BLOCKED URL\s*\*\/\s*/gi, ''); + + if (blocked === 0 && sanitized === template.css_content) continue; + + await knex('css_templates') + .where({ id: template.id }) + .update({ css_content: sanitized, updated_at: knex.fn.now() }); + + repaired.push({ + slot: template.slot_number, + name: template.name || `Slot ${template.slot_number}`, + }); + } + + if (repaired.length === 0) return; + + console.log( + `200: removed remote url() references from ${repaired.length} CSS template(s): ` + + repaired.map((r) => `#${r.slot} ${r.name}`).join(', ') + ); + + // Surface it to the operator. Admin notifications ARE unread activity_logs + // rows (adminNotifications.js reads that table), so this shows up in the + // bell without a second mechanism. Written directly rather than through + // logActivity() to keep the migration free of a service import. + if (await knex.schema.hasTable('activity_logs')) { + try { + await knex('activity_logs').insert({ + activity_type: 'css_template_remote_urls_removed', + actor_type: 'system', + metadata: JSON.stringify({ + count: repaired.length, + templates: repaired, + }), + }); + } catch (err) { + // A notification is not worth failing the data correction over. + console.log(`200: could not write the activity log entry (${err.message})`); + } + } +}; + +exports.down = async function () { + // Intentionally a no-op — see the header. +}; diff --git a/backend/src/utils/cssSanitizer.js b/backend/src/utils/cssSanitizer.js index c61f98a7..578262de 100644 --- a/backend/src/utils/cssSanitizer.js +++ b/backend/src/utils/cssSanitizer.js @@ -29,8 +29,38 @@ const FORBIDDEN_PATTERNS = [ /on\w+\s*=/gi, // onclick=, onload=, etc. ]; -// Pattern for external URLs (block external, allow only safe raster data: images) -const EXTERNAL_URL_PATTERN = /url\s*\(\s*["']?(?!data:image\/(?:jpeg|jpg|png|gif|webp))/gi; +/** + * Decode CSS escape sequences. + * + * `\72` is a legal way to write `r`, so `u\72l(https://evil.example/x.gif)` + * IS a url() to a browser while matching no literal pattern for "url(". + * Decoding first means the scanner below sees what the browser will see. The + * decoded form is what gets stored, which is safe: the same stylesheet, + * spelled unambiguously. + * + * Per CSS syntax: a backslash plus 1-6 hex digits and one optional trailing + * whitespace, or a backslash plus any other single character. + */ +function decodeCssEscapes(css) { + return String(css).replace( + /\\([0-9a-fA-F]{1,6})[ \t\n\f]?|\\([^0-9a-fA-F])/g, + (match, hex, literal) => { + if (hex) { + const code = parseInt(hex, 16); + // Null, out-of-range and surrogate escapes are invalid; leave them + // exactly as written rather than throwing. + if (!Number.isFinite(code) || code === 0 || code > 0x10FFFF + || (code >= 0xD800 && code <= 0xDFFF)) return match; + return String.fromCodePoint(code); + } + return literal; + } + ); +} + +// The only target a url() may name: an inline raster data: image. Anything +// else is a request to a third party from someone else's browser. +const ALLOWED_URL_TARGET = /^data:image\/(?:jpeg|jpg|png|gif|webp)/i; // Maximum CSS size in bytes (100KB) const MAX_CSS_SIZE = 100 * 1024; @@ -69,6 +99,163 @@ function sanitizeCss(css) { return sanitized.trim(); } +/** + * Replace every `url(...)` that does not name an inline data: image with the + * inert keyword `none`. + * + * This REPLACES an earlier implementation that prefixed the offending token + * with a `/* BLOCKED URL *\/` comment and left the URL in place. CSS comments + * are discarded during tokenization, so the declaration a browser actually + * parsed still carried the live URL — the "block" was inert, while the + * warning returned to the caller said it had worked: + * + * before: '.a{background:url(https://x/p.gif)}' + * → '.a{background:/* BLOCKED URL *\/ url(https://x/p.gif)}' + * → parsed as '.a{background: url(https://x/p.gif)}' + * + * `none` is used rather than deleting the declaration because it is valid in + * the shorthand positions these appear in (`background: #fff none no-repeat`) + * and leaves the rest of the rule intact. + * + * @param {string} css + * @returns {{ sanitized: string, blocked: number }} + */ +function stripDisallowedUrls(css) { + const input = css == null ? '' : String(css); + let out = ''; + let blocked = 0; + let i = 0; + + while (i < input.length) { + // --- CSS comment --------------------------------------------------- + // Its own lexical state. A comment containing an unmatched apostrophe + // (`/* don't */`) otherwise put the scanner into string mode and let it + // copy the rest of the stylesheet — including a live url() — unscanned, + // while a browser ignores the comment entirely and makes the request. + if (input[i] === '/' && input[i + 1] === '*') { + const close = input.indexOf('*/', i + 2); + const stop = close === -1 ? input.length : close + 2; + out += input.slice(i, stop); + i = stop; + continue; + } + + // --- string ---------------------------------------------------------- + // Escape-aware: `\"` inside a double-quoted string does NOT close it. + // Decoding escapes up front (an earlier attempt) turned that into a real + // quote, desynchronised the scanner, and hid the url() that followed. + if (input[i] === '"' || input[i] === '\'') { + const quote = input[i]; + let j = i + 1; + while (j < input.length) { + if (input[j] === '\\') { j += 2; continue; } + if (input[j] === quote) { j += 1; break; } + j += 1; + } + out += input.slice(i, Math.min(j, input.length)); + i = Math.min(j, input.length); + continue; + } + + // --- url( token -------------------------------------------------------- + const ident = readIdentifier(input, i); + if (ident.end > i && decodeCssEscapes(ident.raw).toLowerCase() === 'url') { + let j = ident.end; + while (j < input.length && /\s/.test(input[j])) j += 1; + if (input[j] === '(') { + const token = readUrlToken(input, j); + if (token) { + // The target is decoded only to DECIDE; the original bytes are what + // gets emitted when it is allowed, so nothing else in the + // stylesheet is rewritten. + const target = decodeCssEscapes(token.target).trim(); + if (ALLOWED_URL_TARGET.test(target)) { + out += input.slice(i, token.end); + } else { + blocked += 1; + out += 'none'; + } + i = token.end; + continue; + } + } + // Not actually a url() call — emit the identifier and carry on. + out += input.slice(i, ident.end); + i = ident.end; + continue; + } + + out += input[i]; + i += 1; + } + + return { sanitized: out, blocked }; +} + +/** A CSS escape sequence at `start`, or null. */ +function matchEscape(input, start) { + if (input[start] !== '\\') return null; + const rest = input.slice(start, start + 8); + const m = /^\\(?:[0-9a-fA-F]{1,6}[ \t\n\f]?|[^0-9a-fA-F])/.exec(rest); + return m ? m[0] : null; +} + +/** + * Read a CSS identifier, escapes included, WITHOUT decoding it. + * + * `u\72l` is a legal spelling of `url`, so the identifier has to be decoded + * to be recognised — but only for the comparison. Returning the raw text + * means an identifier that is not a url() (`.w-1\/2`, a perfectly ordinary + * escaped Tailwind selector) is emitted byte-identical rather than silently + * rewritten to `.w-1/2`, which is a different selector. + */ +function readIdentifier(input, start) { + let j = start; + let raw = ''; + while (j < input.length) { + const escape = matchEscape(input, j); + if (escape) { raw += escape; j += escape.length; continue; } + if (/[A-Za-z0-9_-]/.test(input[j])) { raw += input[j]; j += 1; continue; } + break; + } + return { raw, end: j }; +} + +/** + * Read a `url( … )` token starting at the opening paren. + * @returns {{ target: string, end: number }|null} null when unterminated. + */ +function readUrlToken(input, openParen) { + let j = openParen + 1; + let target = ''; + while (j < input.length && /\s/.test(input[j])) j += 1; + + if (input[j] === '"' || input[j] === '\'') { + // Quoted: the quote closes the value, so ")" inside it is content. + const quote = input[j]; + j += 1; + while (j < input.length && input[j] !== quote) { + if (input[j] === '\\') { target += input.slice(j, j + 2); j += 2; continue; } + target += input[j]; + j += 1; + } + if (j >= input.length) return null; + j += 1; + } else { + while (j < input.length && input[j] !== ')') { + if (input[j] === '\\') { target += input.slice(j, j + 2); j += 2; continue; } + target += input[j]; + j += 1; + } + } + + while (j < input.length && /\s/.test(input[j])) j += 1; + // Unterminated url( — malformed. Leave it alone rather than swallowing the + // remainder of the stylesheet. + if (input[j] !== ')') return null; + return { target, end: j + 1 }; +} + /** * Enhanced CSS sanitization with warnings * @param {string} cssContent - Raw CSS content @@ -101,17 +288,24 @@ function sanitizeCSS(cssContent) { } } - // Block external URLs (only allow data: URIs for images) - EXTERNAL_URL_PATTERN.lastIndex = 0; - if (EXTERNAL_URL_PATTERN.test(sanitized)) { - warnings.push('Blocked external URL references. Only data: URIs are allowed for images.'); - EXTERNAL_URL_PATTERN.lastIndex = 0; - sanitized = sanitized.replace(EXTERNAL_URL_PATTERN, '/* BLOCKED URL */ url('); - } - // Remove HTML comments that might be used for injection sanitized = sanitized.replace(//g, ''); + // URLs are scanned AFTER the comment strip, not before. Removing + // `` from `url(https://evil.example/p.gif)` JOINS the + // remaining characters into a live `url(...)` — so a scan that ran first + // saw no token, reported the input clean, and the transformation below it + // then produced exactly the request the scan was there to prevent. Any + // pass that can join tokens has to happen before validation, not after. + const urlPass = stripDisallowedUrls(sanitized); + if (urlPass.blocked > 0) { + warnings.push( + `Blocked ${urlPass.blocked} external URL reference${urlPass.blocked === 1 ? '' : 's'}. ` + + 'Only data: URIs are allowed for images.' + ); + sanitized = urlPass.sanitized; + } + // Remove control characters // eslint-disable-next-line no-control-regex -- intentional: strips control chars from untrusted CSS sanitized = sanitized.replace(/[\u0000-\u001F\u007F]/g, ''); @@ -168,6 +362,7 @@ function scopeToGalleryPage(cssContent) { module.exports = { sanitizeCss, sanitizeCSS, + stripDisallowedUrls, validateCSS, scopeToGalleryPage, MAX_CSS_SIZE diff --git a/frontend/src/i18n/locales/de.json b/frontend/src/i18n/locales/de.json index fe6736aa..4a33185e 100644 --- a/frontend/src/i18n/locales/de.json +++ b/frontend/src/i18n/locales/de.json @@ -3127,7 +3127,8 @@ "bulkDeleteCompleted_one": "Massen-Löschung abgeschlossen: {{count}} Event entfernt", "bulkDeleteCompleted_other": "Massen-Löschung abgeschlossen: {{count}} Events entfernt", "bulkArchiveCompleted_one": "Massen-Archivierung abgeschlossen: {{count}} Event archiviert", - "bulkArchiveCompleted_other": "Massen-Archivierung abgeschlossen: {{count}} Events archiviert" + "bulkArchiveCompleted_other": "Massen-Archivierung abgeschlossen: {{count}} Events archiviert", + "cssTemplateRemoteUrlsRemoved": "Externe URLs aus {{count}} CSS-Vorlage(n) entfernt — siehe Release Notes" }, "notificationToasts": { "markedAllRead": "Alle Benachrichtigungen als gelesen markiert", @@ -3429,7 +3430,8 @@ "whatsapp_config_updated": "WhatsApp-Konfiguration aktualisiert", "feature_flags_summary_one": "{{count}} Funktion aktualisiert: {{summary}}", "feature_flags_summary_other": "{{count}} Funktionen aktualisiert: {{summary}}", - "feature_flags_updated": "Funktionseinstellungen aktualisiert" + "feature_flags_updated": "Funktionseinstellungen aktualisiert", + "css_template_remote_urls_removed": "Externe URLs aus {{count}} CSS-Vorlage(n) entfernt — siehe Release Notes" }, "people": { "title": "Personen in dieser Galerie", diff --git a/frontend/src/i18n/locales/en.json b/frontend/src/i18n/locales/en.json index c3a7a030..c31ac7da 100644 --- a/frontend/src/i18n/locales/en.json +++ b/frontend/src/i18n/locales/en.json @@ -2657,7 +2657,8 @@ "bulkDeleteCompleted_one": "Bulk delete completed: {{count}} event removed", "bulkDeleteCompleted_other": "Bulk delete completed: {{count}} events removed", "bulkArchiveCompleted_one": "Bulk archive completed: {{count}} event archived", - "bulkArchiveCompleted_other": "Bulk archive completed: {{count}} events archived" + "bulkArchiveCompleted_other": "Bulk archive completed: {{count}} events archived", + "cssTemplateRemoteUrlsRemoved": "Remote URLs removed from {{count}} CSS template(s) — see the release notes" }, "notificationToasts": { "markedAllRead": "All notifications marked as read", @@ -2961,7 +2962,8 @@ "whatsapp_config_updated": "WhatsApp configuration updated", "feature_flags_summary_one": "{{count}} features updated: {{summary}}", "feature_flags_summary_other": "{{count}} features updated: {{summary}}", - "feature_flags_updated": "Feature settings updated" + "feature_flags_updated": "Feature settings updated", + "css_template_remote_urls_removed": "Remote URLs removed from {{count}} CSS template(s) — see the release notes" }, "people": { "title": "People in this gallery",