diff --git a/.release-please-manifest-beta.json b/.release-please-manifest-beta.json index 03d11272..22c7b2e1 100644 --- a/.release-please-manifest-beta.json +++ b/.release-please-manifest-beta.json @@ -1,3 +1,3 @@ { - ".": "3.123.0-beta.0" + ".": "3.124.1-beta.0" } diff --git a/CHANGELOG.md b/CHANGELOG.md index 9630f7c6..48b5fbac 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,48 @@ All notable changes to PicPeak will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [3.124.1-beta.0](https://github.com/PicPeak/picpeak/compare/v3.124.0-beta.0...v3.124.1-beta.0) (2026-09-05) + + +### Bug Fixes + +* remove the fragmentation handling stranded by [#1303](https://github.com/PicPeak/picpeak/issues/1303) ([5dda14f](https://github.com/PicPeak/picpeak/commit/5dda14f7271265506a17acec5d253d9912784692)) + +## [3.124.0-beta.0](https://github.com/PicPeak/picpeak/compare/v3.123.0-beta.0...v3.124.0-beta.0) (2026-09-05) + + +### Features + +* **newsletters:** warn about deliverability before a large send ([0536c86](https://github.com/PicPeak/picpeak/commit/0536c86ec9014bf3b64c6590841e863525d90326)) +* **newsletters:** warn about deliverability before a large send ([49197be](https://github.com/PicPeak/picpeak/commit/49197be3293bac4c312352b3915d9d7fd9973c29)) + + +### Bug Fixes + +* **gallery:** give the Grid layout a lazy-loading pre-load band ([#1287](https://github.com/PicPeak/picpeak/issues/1287)) ([b1e5287](https://github.com/PicPeak/picpeak/commit/b1e5287351b43a347957e0b7a32d4c81d00ba11b)) +* **gallery:** image-loading follow-ups — pre-load band, decode release, sanitizer dedup ([905fc59](https://github.com/PicPeak/picpeak/commit/905fc595e3c15d55e00347807047acbe58c9b5bc)) +* **gallery:** release the canvas decode when it is drawn, not at unmount ([fbe9757](https://github.com/PicPeak/picpeak/commit/fbe9757a53b1cbe460837534cc3319d990180b61)), closes [#1287](https://github.com/PicPeak/picpeak/issues/1287) +* **gallery:** release the canvas-mode decode, and drop a now-duplicate sanitizer ([be8d79e](https://github.com/PicPeak/picpeak/commit/be8d79e9c4b6148a0b3f8a1f81f05fbf5fbf6880)) +* **gallery:** remove the inert image-protection prop surface from AuthenticatedImage ([1f316ef](https://github.com/PicPeak/picpeak/commit/1f316ef91cd2f74ac7ae68751fc00b9a3ccff410)) +* **gallery:** remove the inert image-protection prop surface from AuthenticatedImage ([e734e41](https://github.com/PicPeak/picpeak/commit/e734e41c412cede1be5efbe27aab617d59faee9d)), closes [#1297](https://github.com/PicPeak/picpeak/issues/1297) +* **newsletters:** make the warning's duration and queue claim honest ([7b4a65e](https://github.com/PicPeak/picpeak/commit/7b4a65ecc79d93715c52d82e9d7adb2665540536)) +* remove the image-fragmentation surface ([ae23b1a](https://github.com/PicPeak/picpeak/commit/ae23b1adea03fb6b46aac0079f6e523b97e3594e)) +* remove the image-fragmentation surface ([967224c](https://github.com/PicPeak/picpeak/commit/967224c030b9cd721fb0217d0763e1ec1978c51e)) +* **security:** apply image-security defaults on every creation path ([ab6c33d](https://github.com/PicPeak/picpeak/commit/ab6c33d9eb485cc3ed05187a98a22f51926cc125)), closes [#1296](https://github.com/PicPeak/picpeak/issues/1296) +* **security:** apply the Image-security defaults instead of storing them ([#1296](https://github.com/PicPeak/picpeak/issues/1296)) ([2e9bd54](https://github.com/PicPeak/picpeak/commit/2e9bd540c97001d274c6288800a86a164174ae9c)) +* **security:** apply the Image-security defaults instead of storing them ([#1296](https://github.com/PicPeak/picpeak/issues/1296)) ([8ca3610](https://github.com/PicPeak/picpeak/commit/8ca3610514dc9e16c1c14c82fce529042b728e47)) +* **security:** check for an escaped identifier before consuming the escape ([b6dc099](https://github.com/PicPeak/picpeak/commit/b6dc0991ce04b574a03aff9cd579f0333b11048e)), closes [#1264](https://github.com/PicPeak/picpeak/issues/1264) +* **security:** close the remaining image-security default gaps ([19c518a](https://github.com/PicPeak/picpeak/commit/19c518aaa50f1bd8fb7cef260a55bf3d5f3eb7f3)), closes [#1296](https://github.com/PicPeak/picpeak/issues/1296) +* **security:** close two CSS url() bypasses the sanitizer dedup exposed ([1cf8274](https://github.com/PicPeak/picpeak/commit/1cf82746b72c2639547871e4e479d390760b7c1d)) +* **security:** decode settings at the API boundary and honour the transaction ([0e560eb](https://github.com/PicPeak/picpeak/commit/0e560ebb193d8243ed4de059ea150f3f64fa1409)), closes [#1296](https://github.com/PicPeak/picpeak/issues/1296) +* **security:** one settings decoder, and the last creation path ([0deef25](https://github.com/PicPeak/picpeak/commit/0deef2584f4a6287bb947bdc545f585ae30aab67)), closes [#1296](https://github.com/PicPeak/picpeak/issues/1296) +* **security:** re-check inline CSS after template substitution ([027afb6](https://github.com/PicPeak/picpeak/commit/027afb608667ae072465fdf173751fa79f75110d)), closes [#1264](https://github.com/PicPeak/picpeak/issues/1264) +* **security:** reject array values for every field on the event update ([933f2d8](https://github.com/PicPeak/picpeak/commit/933f2d8e0ee0685128f2e8f8bed5169a06b4116c)), closes [#1296](https://github.com/PicPeak/picpeak/issues/1296) +* **security:** reject array values on the event update route too ([8f3436f](https://github.com/PicPeak/picpeak/commit/8f3436f17d6390a776c4258c53475d4c6038a63e)), closes [#1296](https://github.com/PicPeak/picpeak/issues/1296) +* **security:** strip control characters before scanning CSS for url() ([99f54a3](https://github.com/PicPeak/picpeak/commit/99f54a39546591d21df5ca11149770a161c447d9)), closes [#1264](https://github.com/PicPeak/picpeak/issues/1264) +* **security:** use CSS whitespace, not JavaScript's, in the url() reader ([4196e83](https://github.com/PicPeak/picpeak/commit/4196e83a5f0427984fc16538783a1f7418a10837)), closes [#1264](https://github.com/PicPeak/picpeak/issues/1264) +* **security:** validate CSS urls last, after every pass that moves text ([1151e96](https://github.com/PicPeak/picpeak/commit/1151e96144a9ada7170461e829d2f3d5dcc8edb2)), closes [#1264](https://github.com/PicPeak/picpeak/issues/1264) + ## [3.123.0-beta.0](https://github.com/PicPeak/picpeak/compare/v3.122.7-beta.0...v3.123.0-beta.0) (2026-09-04) diff --git a/backend/__tests__/integration/imageSecurityDefaults.test.js b/backend/__tests__/integration/imageSecurityDefaults.test.js new file mode 100644 index 00000000..b923fac1 --- /dev/null +++ b/backend/__tests__/integration/imageSecurityDefaults.test.js @@ -0,0 +1,220 @@ +/** + * Image-security settings applied as creation defaults (#1296). + * + * Four controls in Settings → Image security were written, reloaded and + * rendered as toggles, and read by nothing: + * + * default_protection_level, default_image_quality, + * enable_canvas_rendering + * + * Each maps onto an `events` column migration 038 already created, and each + * is labelled "… by default". `enable_devtools_protection` was the only one + * of the five ever wired. + * + * The load-bearing constraint is that this is CREATION-time only. Applying + * these to existing events would silently change live galleries on upgrade — + * an install with enable_canvas_rendering already on would flip every grid to + * canvas rendering, which is the memory profile under investigation in #1287. + */ + +const { bootCrmDb } = require('./helpers/crmDb'); + +describe('image-security creation defaults', () => { + let db; + let cleanup; + let getImageSecurityDefaults; + let resolveImageSecurityColumns; + let readBooleanSetting; + + beforeAll(async () => { + ({ db, cleanup } = await bootCrmDb()); + ({ getImageSecurityDefaults, resolveImageSecurityColumns, readBooleanSetting } = + require('../../src/routes/adminEvents/helpers')); + }, 120000); + + afterAll(async () => { + if (cleanup) await cleanup(); + }); + + const setSetting = async (key, value) => { + await db('app_settings') + .insert({ setting_key: key, setting_value: JSON.stringify(value), setting_type: 'security' }) + .onConflict('setting_key') + .merge(); + }; + + beforeEach(async () => { + await db('app_settings').whereIn('setting_key', [ + 'default_protection_level', 'default_image_quality', + 'enable_canvas_rendering', + ]).del(); + }); + + it('returns nothing when no settings are configured', async () => { + // Every key absent must fall through to the column defaults, which is + // exactly the behaviour before this existed. + expect(await getImageSecurityDefaults()).toEqual({}); + }); + + it('maps each setting onto its events column', async () => { + await setSetting('default_protection_level', 'enhanced'); + await setSetting('default_image_quality', 72); + await setSetting('enable_canvas_rendering', true); + + expect(await getImageSecurityDefaults()).toEqual({ + protection_level: 'enhanced', + image_quality: 72, + use_canvas_rendering: true, + }); + }); + + it('carries a false canvas setting through, rather than dropping it', async () => { + // `false` is a real choice — dropping it as falsy would leave the column + // default in place and make "off" unreachable. + await setSetting('enable_canvas_rendering', false); + expect(await getImageSecurityDefaults()).toEqual({ use_canvas_rendering: false }); + }); + + it.each([ + ['an unknown protection level', 'default_protection_level', 'paranoid'], + ['a non-enum protection level', 'default_protection_level', 42], + ['image quality above 100', 'default_image_quality', 250], + ['image quality of zero', 'default_image_quality', 0], + ['a non-numeric quality', 'default_image_quality', 'high'], + ['a non-boolean canvas value', 'enable_canvas_rendering', 'yes'], + // parseInt would have rescued each of these into a valid-looking + // integer. The settings PUT stores values without validating them, so + // they can genuinely be in the table. + ['a numeric prefix with trailing junk', 'default_image_quality', '72oops'], + ['a fractional quality', 'default_image_quality', 72.5], + ['a single-element array', 'default_image_quality', [72]], + ])('ignores %s and falls through to the column default', async (_label, key, value) => { + await setSetting(key, value); + expect(await getImageSecurityDefaults()).toEqual({}); + }); + + it('applies only the keys that are configured', async () => { + await setSetting('default_protection_level', 'maximum'); + expect(await getImageSecurityDefaults()).toEqual({ protection_level: 'maximum' }); + }); + + it('never throws, so a settings failure cannot block event creation', async () => { + await setSetting('default_image_quality', { nonsense: true }); + await expect(getImageSecurityDefaults()).resolves.toEqual({}); + }); + + describe('double-encoded settings (the settings tab round trip)', () => { + // GET returns setting_value undecoded and the tab PUTs the whole object + // back through JSON.stringify, so on SQLite one visit to the tab turns + // every value it read into a doubly-encoded string. A single parse left + // a string behind, the type checks rejected it, and the defaults went + // silently dead again. + const setRaw = async (key, raw) => { + await db('app_settings') + .insert({ setting_key: key, setting_value: raw, setting_type: 'security' }) + .onConflict('setting_key').merge(); + }; + + it('reads a double-encoded boolean', async () => { + await setRaw('enable_canvas_rendering', JSON.stringify(JSON.stringify(true))); + expect(await getImageSecurityDefaults()).toEqual({ use_canvas_rendering: true }); + }); + + it('reads a double-encoded protection level', async () => { + await setRaw('default_protection_level', JSON.stringify(JSON.stringify('enhanced'))); + expect(await getImageSecurityDefaults()).toEqual({ protection_level: 'enhanced' }); + }); + + it('reads a double-encoded integer', async () => { + await setRaw('default_image_quality', JSON.stringify(JSON.stringify(72))); + expect(await getImageSecurityDefaults()).toEqual({ image_quality: 72 }); + }); + + it('reads a value buried under many saves, not just one', async () => { + // Each visit to the settings tab used to add a layer, so the depth is + // however many times someone opened it — not a number to cap. + let raw = JSON.stringify('maximum'); + for (let i = 0; i < 8; i += 1) raw = JSON.stringify(raw); + await setRaw('default_protection_level', raw); + expect(await getImageSecurityDefaults()).toEqual({ protection_level: 'maximum' }); + }); + + it('still rejects a malformed value however many times it was encoded', async () => { + await setRaw('default_image_quality', JSON.stringify(JSON.stringify('72oops'))); + expect(await getImageSecurityDefaults()).toEqual({}); + }); + }); + + describe('readBooleanSetting shares the same decoder', () => { + // Every reader of app_settings has to agree, or the settings tab shows + // protection disabled while newly created galleries turn it on. + const setRaw2 = async (key, raw) => { + await db('app_settings') + .insert({ setting_key: key, setting_value: raw, setting_type: 'security' }) + .onConflict('setting_key').merge(); + }; + + it('reads a double-encoded false as false, not as absent', async () => { + await setRaw2('enable_devtools_protection', JSON.stringify(JSON.stringify(false))); + expect(await readBooleanSetting('enable_devtools_protection')).toBe(false); + }); + + it('still reads a singly-encoded value', async () => { + await setRaw2('enable_devtools_protection', JSON.stringify(true)); + expect(await readBooleanSetting('enable_devtools_protection')).toBe(true); + }); + + it('returns undefined for a non-boolean, so the caller keeps its default', async () => { + await setRaw2('enable_devtools_protection', JSON.stringify('sometimes')); + expect(await readBooleanSetting('enable_devtools_protection')).toBeUndefined(); + }); + }); + + describe('resolveImageSecurityColumns', () => { + it('omits every column when neither the request nor the settings supply one', () => { + expect(resolveImageSecurityColumns({}, {})).toEqual({}); + }); + + it('uses the global default when the request says nothing', () => { + expect(resolveImageSecurityColumns({}, { protection_level: 'maximum' })) + .toEqual({ protection_level: 'maximum' }); + }); + + it('lets an explicit request value win over the global default', () => { + expect(resolveImageSecurityColumns( + { protection_level: 'basic' }, + { protection_level: 'maximum' }, + )).toEqual({ protection_level: 'basic' }); + }); + + it('keeps an explicit false canvas value instead of reading it as absent', () => { + const columns = resolveImageSecurityColumns( + { use_canvas_rendering: false }, + { use_canvas_rendering: true }, + ); + expect(columns.use_canvas_rendering).toBeFalsy(); + }); + + it('keeps a zero-ish explicit value rather than falling through', () => { + // 0 is out of range for the column, but the guard is `!== undefined`, + // not truthiness — the validator is what rejects out-of-range input. + expect(resolveImageSecurityColumns({ image_quality: 0 }, { image_quality: 85 })) + .toEqual({ image_quality: 0 }); + }); + + it('resolves each column independently', () => { + expect(resolveImageSecurityColumns( + { image_quality: 60 }, + { protection_level: 'enhanced' }, + )).toEqual({ + protection_level: 'enhanced', + image_quality: 60, + }); + }); + + it('tolerates a missing body, which is what an empty API request looks like', () => { + expect(resolveImageSecurityColumns(undefined, { image_quality: 90 })) + .toEqual({ image_quality: 90 }); + }); + }); +}); diff --git a/backend/__tests__/routes/adminEvents.smoke.test.js b/backend/__tests__/routes/adminEvents.smoke.test.js index 713dcda1..e15bf3b3 100644 --- a/backend/__tests__/routes/adminEvents.smoke.test.js +++ b/backend/__tests__/routes/adminEvents.smoke.test.js @@ -214,6 +214,43 @@ describe('admin events CRUD endpoints (smoke)', () => { expect(row.welcome_message).toBe('Hello guests'); }); + // #1296 — express-validator runs isInt/isIn/isBoolean element-wise on + // arrays, so a single-element array satisfies its field validator and + // survives into `updates`, which is spread into .update() with no column + // allow-list. That put an array into a scalar column (a PG insert error), + // and formatBoolean([false]) read as true. Guarded for every field, not + // just the ones that prompted it. + it.each([ + ['image_quality', [72]], + ['protection_level', ['basic']], + ['use_canvas_rendering', [false]], + // Not a protection field: the guard is not scoped to that block. + ['event_name', ['Arrayed']], + ['allow_downloads', [false]], + ])('400s on an array value for %s', async (field, value) => { + const id = await insertEvent(db, adminId, { event_name: 'Unchanged' }); + const res = await auth(request(app).put(`/api/admin/events/${id}`)) + .send({ [field]: value }); + + expect(res.status).toBe(400); + expect(res.body.error).toMatch(field); + // And nothing was written. + const row = await db('events').where({ id }).first(); + expect(row.event_name).toBe('Unchanged'); + }); + + it('still accepts customer_account_ids, the one field that is an array', async () => { + const id = await insertEvent(db, adminId, { event_name: 'Keep' }); + const res = await auth(request(app).put(`/api/admin/events/${id}`)).send({ + event_name: 'Renamed', + customer_account_ids: [], + }); + + expect(res.status).toBe(200); + const row = await db('events').where({ id }).first(); + expect(row.event_name).toBe('Renamed'); + }); + it('404s when updating a missing event', async () => { const res = await auth(request(app).put('/api/admin/events/999999')).send({ event_name: 'Ghost', diff --git a/backend/__tests__/services/newsletterService.sanitize.test.js b/backend/__tests__/services/newsletterService.sanitize.test.js index c2e026be..948dc0fa 100644 --- a/backend/__tests__/services/newsletterService.sanitize.test.js +++ b/backend/__tests__/services/newsletterService.sanitize.test.js @@ -12,7 +12,9 @@ jest.mock('../../src/database/db', () => ({ db: jest.fn(), logActivity: jest.fn( const { sanitizeCampaignBody, sanitizeCampaignCss, MAX_BODY_BYTES, + sanitizeInlineStylesAfterSubstitution, } = require('../../src/services/newsletterService'); +const { safeTemplateReplace } = require('../../src/services/emailProcessor'); describe('sanitizeCampaignBody', () => { it('returns empty string for empty input', () => { @@ -92,6 +94,35 @@ describe('sanitizeCampaignBody', () => { expect(out).not.toContain('http://evil.example'); }); + it('blocks a tracking url() hidden behind " entities', () => { + // sanitize-html writes `"` inside an attribute as `"`, so the CSS + // scanner and the recipient's browser disagreed about where strings + // start: the browser decodes first and reads the apostrophe as ordinary + // text inside a real string, then fetches the background — while the + // scanner saw the apostrophe open a string and skipped past the url(). + const out = sanitizeCampaignBody( + `

hi

` + ); + expect(out).not.toContain('evil.example'); + }); + + it('blocks a tracking url() hidden behind an escaped quote', () => { + const out = sanitizeCampaignBody( + `

hi

` + ); + expect(out).not.toContain('evil.example'); + }); + + it('keeps a legitimate quoted font stack, re-encoded for the attribute', () => { + const out = sanitizeCampaignBody( + `

hi

` + ); + expect(out).toContain('color:red'); + expect(out).toContain('Helvetica Neue'); + // Re-encoded, so the attribute stays well formed rather than being cut short. + expect(out).not.toMatch(/style="[^"]*"[^>]*"/); + }); + it('strips expression() out of an inline style', () => { const out = sanitizeCampaignBody('

hi

'); expect(out).not.toContain('expression('); @@ -159,3 +190,46 @@ describe('sanitizeCampaignCss', () => { expect(css).not.toContain(''); }); }); + +describe('inline CSS is re-checked after template substitution', () => { + // The stored body is sanitized, but safeTemplateReplace rewrites it + // afterwards — so the string that was validated is not the string that is + // sent. A conditional inside a style attribute can delete the quoting that + // made a url() inert, which no amount of lexer correctness can catch. + const payload = + `

hi

`; + + it('neutralises a url() that substitution would activate', () => { + const stored = sanitizeCampaignBody(payload); + // Correctly left alone at write time: the url() really is inside a CSS + // string while the conditionals are still in place. + expect(stored).toContain('evil.example'); + + const substituted = safeTemplateReplace(stored, { company_name: '' }, { escapeHtml: true }); + // Expansion removed the quotes, so without the recheck this ships live. + expect(substituted).toMatch(/background:url\(https:\/\/evil\.example/); + + const rendered = sanitizeInlineStylesAfterSubstitution(substituted); + expect(rendered).not.toMatch(/url\(\s*['"]?https:\/\/evil\.example/); + expect(rendered).toContain('background:none'); + }); + + it('leaves a body without style attributes untouched', () => { + const html = '

Hello {{first_name}}

'; + expect(sanitizeInlineStylesAfterSubstitution(html)).toBe(html); + }); + + it('keeps legitimate inline styles through the recheck', () => { + const html = '

hi

'; + const out = sanitizeInlineStylesAfterSubstitution(html); + expect(out).toContain('color:red'); + expect(out).toContain('font-size:14px'); + }); + + it('is safe on empty and nullish input', () => { + expect(sanitizeInlineStylesAfterSubstitution('')).toBe(''); + expect(sanitizeInlineStylesAfterSubstitution(null)).toBeNull(); + }); +}); diff --git a/backend/__tests__/utils/cssSanitizer.remoteUrls.test.js b/backend/__tests__/utils/cssSanitizer.remoteUrls.test.js index 4153406f..30d1e035 100644 --- a/backend/__tests__/utils/cssSanitizer.remoteUrls.test.js +++ b/backend/__tests__/utils/cssSanitizer.remoteUrls.test.js @@ -221,6 +221,92 @@ describe('sanitizeCSS', () => { expect(asParsed(sanitized)).not.toContain('evil.example'); }); + it.each([ + ['a C0 control character', '\u0001'], + ['a NUL byte', '\u0000'], + ['a DEL byte', '\u007F'], + // Newlines are control characters for this strip, so the bypass did not + // need an exotic byte — ordinary-looking wrapped CSS was enough. + ['a newline', '\n'], + ])('blocks a url() that only becomes one after %s is removed', (_label, ch) => { + // Same token-joining hazard as the HTML-comment case above: the control + // strip used to run AFTER the URL scan, so `u\u0001rl(...)` was scanned + // as clean and then joined into a live remote request, with no warning. + const { sanitized, warnings } = sanitizeCSS( + `.a{background:u${ch}rl(https://evil.example/p.gif)}` + ); + expect(asParsed(sanitized)).not.toContain('evil.example'); + expect(warnings.join(' ')).toContain('external URL'); + }); + + it('blocks a url() hidden behind an escaped quote outside a string', () => { + // `\'` is an escaped identifier character, not a string opener. The + // scanner used to step onto the apostrophe, enter string mode, and copy + // the rest of the stylesheet — url() included — unexamined. + const { sanitized } = sanitizeCSS( + ".hero{--marker:\\';background:url(https://evil.example/p.gif)}" + ); + expect(asParsed(sanitized)).not.toContain('evil.example'); + }); + + it.each([ + ['a leading escape', '.a{background:\\75rl(https://evil.example/p.gif)}'], + ['an escape mid-identifier', '.a{background:u\\72l(https://evil.example/p.gif)}'], + ])('blocks a url() spelled with %s', (_label, css) => { + // `\75` is the CSS escape for `u`, so a browser reads `\75rl(` as + // url(). The escape-outside-a-string handling has to run AFTER the + // identifier check, or it eats the escape and hides the token. + const { sanitized } = sanitizeCSS(css); + expect(asParsed(sanitized)).not.toContain('evil.example'); + }); + + it('blocks a url() the TAG strip would have un-quoted', () => { + // `<[^>]*>` deletes the span it matches, and `<">` takes a quote with it. + // Running that after the URL scan meant the scanner saw the url() safely + // inside a string and this pass then removed the quotes that made it so. + // URL validation has to be the last thing that looks at the text. + const { sanitized } = sanitizeCSS( + '--x:x<">;background:url(https://evil.example/p.gif);--y:x<">' + ); + expect(asParsed(sanitized)).not.toContain('evil.example'); + }); + + it('does not treat NBSP as CSS whitespace inside url()', () => { + // JS `\s` matches U+00A0; CSS whitespace does not. Skipping it let the + // scanner read the following quote as a legitimate data: URI and swallow + // a remote url() inside the "string", while a browser sees an unquoted + // url-token ending at the first `)` and fetches the remote background. + const NBSP = '\u00a0'; + const { sanitized } = sanitizeCSS( + `.a{background:url(${NBSP}"data:image/png);background:url(https://evil.example/p.gif);--x:");}` + ); + expect(asParsed(sanitized)).not.toContain('evil.example'); + }); + + it('still allows ordinary CSS whitespace around a data: URI', () => { + const spaced = sanitizeCSS('.a{background:url( "data:image/png;base64,iVBORw0KGgo=" )}'); + expect(spaced.sanitized).toContain('data:image/png'); + const tabbed = sanitizeCSS('.a{background:url(\t"data:image/png;base64,iVBORw0KGgo=")}'); + expect(tabbed.sanitized).toContain('data:image/png'); + }); + + it('does not let an unterminated quote hide everything after it', () => { + // An unclosed quote is a parse error. Trusting it meant one stray + // apostrophe disabled scanning for the remainder of the stylesheet, so + // the safe reading is to treat it as an ordinary character and continue. + const { sanitized } = sanitizeCSS( + "p{font-family:'don't;background:url(https://evil.example/p.gif)}" + ); + expect(asParsed(sanitized)).not.toContain('evil.example'); + }); + + it('still keeps legitimate quoted values and data: images intact', () => { + const font = sanitizeCSS('p{font-family:"Helvetica Neue",sans-serif;color:red}'); + expect(font.sanitized).toContain('"Helvetica Neue"'); + const data = sanitizeCSS(".a{background:url('data:image/png;base64,iVBORw0KGgo=')}"); + expect(data.sanitized).toContain('data:image/png'); + }); + 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)}' diff --git a/backend/package.json b/backend/package.json index ba117667..e60ac33b 100644 --- a/backend/package.json +++ b/backend/package.json @@ -1,6 +1,6 @@ { "name": "picpeak-backend", - "version": "3.123.0-beta.0", + "version": "3.124.1-beta.0", "description": "Backend for PicPeak event photo sharing platform", "main": "server.js", "engines": { diff --git a/backend/src/routes/adminEvents/crud.js b/backend/src/routes/adminEvents/crud.js index fc2dce8e..c80e96ba 100644 --- a/backend/src/routes/adminEvents/crud.js +++ b/backend/src/routes/adminEvents/crud.js @@ -30,7 +30,8 @@ const { clampIntOrUndefined } = require('../../utils/numericHelpers'); const { getFrontendBaseUrl, getAbsoluteFrontendUrl } = require('../../utils/frontendUrl'); const downloadZipService = require('../../services/downloadZipService'); const { resolveEventFeedbackDefaults, applyFeedbackDefaults, KEYBIND_MODES } = require('../../services/feedbackDefaults'); -const { validateHeroImageAnchor, getEventFieldRequirements, readBooleanSetting, getDownloadProtectionDefaults, getBrandingDefaults, getCustomerNameFromPayload, getCustomerEmailFromPayload, getCustomerPhoneFromPayload, isPhoneFieldEnabled, mapEventForApi, hasCustomerContactColumns, deleteEventCascade, SLIDESHOW_TRANSITIONS, SLIDESHOW_COLORFILTERS } = require('./helpers'); +const { validateHeroImageAnchor, getEventFieldRequirements, readBooleanSetting, getDownloadProtectionDefaults, + getImageSecurityDefaults, resolveImageSecurityColumns, getBrandingDefaults, getCustomerNameFromPayload, getCustomerEmailFromPayload, getCustomerPhoneFromPayload, isPhoneFieldEnabled, mapEventForApi, hasCustomerContactColumns, deleteEventCascade, SLIDESHOW_TRANSITIONS, SLIDESHOW_COLORFILTERS } = require('./helpers'); /** * `events.slug` is UNIQUE, and both routes that mint one do a read-then-insert @@ -228,6 +229,12 @@ module.exports = (router) => { body('allow_downloads').optional().isBoolean(), body('disable_right_click').optional().isBoolean(), body('enable_devtools_protection').optional().isBoolean(), + // Image security. PUT /:id has validated these all along; create + // accepted none of them, so a value sent here used to be dropped on the + // floor and the column default applied instead (#1296). + body('protection_level').optional().not().isArray().isIn(['basic', 'standard', 'enhanced', 'maximum']), + body('use_canvas_rendering').optional().not().isArray().isBoolean().toBoolean(), + body('image_quality').optional().not().isArray().isInt({ min: 1, max: 100 }).toInt(), body('watermark_downloads').optional().isBoolean(), body('watermark_text').optional().trim(), // #328 follow-up: per-event opt-in for presigned-URL "Download All". @@ -544,6 +551,14 @@ module.exports = (router) => { // the request explicitly overrides it (#317 — admin disabled it globally // but new events still got it ON because the column default is true). const protectionDefaults = await getDownloadProtectionDefaults(); + // #1296 — the other four Image-security settings, which were written, + // rendered as controls, and read by nothing. Same inheritance rule as + // the devtools setting below. Creation-time only; see + // getImageSecurityDefaults for why existing events are left alone. + const imageSecurityColumns = resolveImageSecurityColumns( + req.body, + await getImageSecurityDefaults(), + ); const effectiveEnableDevtoolsProtection = enableDevtoolsProtectionInput !== undefined ? enableDevtoolsProtectionInput @@ -617,6 +632,9 @@ module.exports = (router) => { allow_downloads: formatBoolean(allow_downloads !== undefined ? allow_downloads : true), disable_right_click: formatBoolean(disable_right_click !== undefined ? disable_right_click : false), enable_devtools_protection: formatBoolean(effectiveEnableDevtoolsProtection), + // Request value, else the global default, else the column default — + // a key absent here is one the database fills in (#1296). + ...imageSecurityColumns, watermark_downloads: formatBoolean(watermark_downloads !== undefined ? watermark_downloads : false), watermark_text, allow_presigned_download: formatBoolean(allow_presigned_download === true || allow_presigned_download === 'true'), @@ -1400,6 +1418,14 @@ module.exports = (router) => { allow_downloads: source.allow_downloads, disable_right_click: source.disable_right_click, enable_devtools_protection: source.enable_devtools_protection, + // A duplicate inherits the source's protection settings, NOT the + // current global defaults — copying the gallery is the whole point. + // These four sat next to enable_devtools_protection and were simply + // missed, so duplicating a 'maximum' event produced a 'standard' one + // (#1296). + protection_level: source.protection_level, + image_quality: source.image_quality, + use_canvas_rendering: source.use_canvas_rendering, watermark_downloads: source.watermark_downloads, watermark_text: source.watermark_text, allow_presigned_download: source.allow_presigned_download, @@ -1569,7 +1595,6 @@ module.exports = (router) => { body('use_canvas_rendering').optional().isBoolean(), body('overlay_protection').optional().isBoolean(), body('image_quality').optional().isInt({ min: 1, max: 100 }), - body('fragmentation_level').optional().isInt({ min: 1, max: 10 }), body('password').optional().isString().custom((value) => { if (value === undefined || value === null || value === '') { return true; @@ -1630,6 +1655,25 @@ module.exports = (router) => { const { id } = req.params; const updates = { ...req.body }; + // express-validator applies isInt/isIn/isBoolean element-wise to + // arrays, so `image_quality: [72]` satisfies its validator and stays + // an array. This handler spreads req.body into .update() with no + // column allow-list, so such a value reaches a scalar column: a PG + // insert error, and `[false]` coerced to true by formatBoolean. + // + // Guarded here rather than per field because it applies to all 44 + // validated fields, not to a chosen few. `customer_account_ids` is the + // only field that is legitimately an array, and it is deleted from + // `updates` below before the write (#1296). + const ARRAY_VALUED_FIELDS = new Set(['customer_account_ids']); + const arrayValued = Object.keys(updates) + .filter((key) => Array.isArray(updates[key]) && !ARRAY_VALUED_FIELDS.has(key)); + if (arrayValued.length > 0) { + return res.status(400).json({ + error: `Array values are not accepted for: ${arrayValued.join(', ')}`, + }); + } + // Strip identity/provenance/secret columns from the mass-assigned // body (GHSA-3rqx). The handler spreads req.body straight into the // events UPDATE, so without this an events.edit holder could rewrite diff --git a/backend/src/routes/adminEvents/helpers.js b/backend/src/routes/adminEvents/helpers.js index be10a8cc..98fe9a9a 100644 --- a/backend/src/routes/adminEvents/helpers.js +++ b/backend/src/routes/adminEvents/helpers.js @@ -73,14 +73,37 @@ const getEventFieldRequirements = async () => { // Helper to read app_settings booleans by key, used to inherit per-setting // defaults onto new events. Returns `undefined` for missing/non-boolean rows // so callers can fall back to a legacy default. +/** + * Decode an app_settings value into the JS value it represents. + * + * setting_value is JSON text on SQLite and may already be decoded by the + * driver on a PG json column, so one parse does not normalise both. On top + * of that, the Image Security tab used to PUT back values it had read + * undecoded, wrapping another layer of quoting around each one on every + * save — the GET handler decodes now, but installs carry however many + * layers they accumulated before that. + * + * Every reader of app_settings has to agree about this, or the admin UI + * shows one thing while event creation does another. + * + * Terminates: each parse of a string is strictly shorter than its input. + */ +const decodeSettingValue = (raw) => { + let value = raw; + while (typeof value === 'string') { + let parsed; + try { parsed = JSON.parse(value); } catch { break; } + if (parsed === value) break; + value = parsed; + } + return value; +}; + const readBooleanSetting = async (key) => { try { const setting = await db('app_settings').where('setting_key', key).first(); if (!setting) return undefined; - let value = setting.setting_value; - if (typeof value === 'string') { - try { value = JSON.parse(value); } catch { /* keep raw */ } - } + const value = decodeSettingValue(setting.setting_value); return typeof value === 'boolean' ? value : undefined; } catch (error) { logger.error('Failed to read app setting', { key, error: error.message }); @@ -95,6 +118,148 @@ const getDownloadProtectionDefaults = async () => { return { enable_devtools_protection: await readBooleanSetting('enable_devtools_protection') }; }; +/** + * The rest of Settings → Image security, as creation defaults (#1296). + * + * Four settings in that panel were written, reloaded and rendered as + * controls, and read by nothing: + * + * default_protection_level → events.protection_level + * default_image_quality → events.image_quality + * enable_canvas_rendering → events.use_canvas_rendering + * + * Each maps onto a column migration 038 already created, and each is + * labelled "… by default", so applying them at creation is what the panel + * has always claimed to do. `enable_devtools_protection` above is the only + * one of the five that was ever wired. + * + * Creation-time only, deliberately. Applying them to EXISTING events would + * silently change live galleries on upgrade — an install with + * enable_canvas_rendering already on would switch every grid to canvas + * rendering, which is memory-expensive at scale and is the profile under + * investigation in #1287. New events only; existing rows untouched. + * + * Any value that is missing or malformed comes back undefined so the caller + * falls through to the column default, exactly as before this existed. + */ +const PROTECTION_LEVELS = ['basic', 'standard', 'enhanced', 'maximum']; + +// parseInt would rescue malformed settings instead of rejecting them: +// parseInt('72oops') is 72, parseInt(72.5) is 72, parseInt([72]) is 72. +// That matters because the settings PUT stores whatever JSON it is handed +// without validating the value (adminImageSecurity.js writes +// JSON.stringify(value) for any allow-listed key), so those shapes really +// can be sitting in app_settings. Accept only a genuine integer, or a +// string that is exactly one. +const toInteger = (value) => { + if (typeof value === 'number') return Number.isInteger(value) ? value : undefined; + if (typeof value === 'string' && /^[+-]?\d+$/.test(value.trim())) return Number(value.trim()); + return undefined; +}; + +const getImageSecurityDefaults = async (trx = null) => { + const defaults = {}; + try { + // Accepts a transaction the way getAppSetting does. It matters on + // sqlite3, whose pool holds a single connection: a caller already inside + // db.transaction() that read through the global `db` would block on the + // connection its own transaction holds until the acquire timeout, and + // the catch below would then quietly swallow it and drop the defaults. + const query = trx || db; + const rows = await query('app_settings') + .whereIn('setting_key', [ + 'default_protection_level', + 'default_image_quality', + 'enable_canvas_rendering', + ]) + .select('setting_key', 'setting_value'); + + // app_settings holds JSON text on SQLite, while a PG json column comes + // back already decoded — so one parse is not enough to normalise both. + // Worse, GET /api/admin/image-security/settings returns setting_value + // without decoding it and the settings tab PUTs the whole fetched object + // straight back through JSON.stringify, so opening the tab and saving + // re-encodes every value it read as text. After one such round trip + // `true` is stored as "\"true\"" and a single parse yields the string + // 'true', which the type checks below reject — the settings would go + // quietly dead again, which is the bug this whole change exists to fix. + // The GET handler now decodes, so this stops accumulating — but installs + // that already stacked N layers have to keep working, and N is however + // many times someone opened that tab. So unwrap until it stops being a + // JSON string rather than to a fixed depth; this terminates because each + // parse of a string is strictly shorter than its input. + const read = (key) => { + const row = rows.find((r) => r.setting_key === key); + if (!row) return undefined; + return decodeSettingValue(row.setting_value); + }; + + const level = read('default_protection_level'); + if (typeof level === 'string' && PROTECTION_LEVELS.includes(level)) { + defaults.protection_level = level; + } + + // The column is an integer percentage; anything outside 1..100 is a + // misconfiguration and falls through rather than being clamped into + // something the operator did not choose. + const quality = toInteger(read('default_image_quality')); + if (quality !== undefined && quality >= 1 && quality <= 100) { + defaults.image_quality = quality; + } + + const canvas = read('enable_canvas_rendering'); + if (typeof canvas === 'boolean') { + defaults.use_canvas_rendering = canvas; + } + + } catch (error) { + // A settings read must never block event creation; the column defaults + // are a correct fallback. + logger.error('Failed to read image-security defaults', { error: error.message }); + } + return defaults; +}; + +/** + * Build the image-security columns for a NEW event: an explicit request + * value wins, then the global default, then the column default (the key is + * omitted entirely so the database supplies it). + * + * Shared by the admin create route and POST /api/v1/events so the configured + * security level cannot depend on which entry point created the gallery — + * the same split that made #592 (devtools) a separate bug from #317. + * + * `body` values are already validated by the route's express-validator + * chain; `defaults` come from getImageSecurityDefaults(), which validates + * them itself. + */ +const resolveImageSecurityColumns = (body = {}, defaults = {}) => { + const { formatBoolean } = require('../../utils/dbCompat'); + const columns = {}; + // express-validator runs isInt/isIn/isBoolean element-wise on arrays, so a + // single-element array like `image_quality: [72]` passes the route's chain + // and arrives here still an array. The routes reject those with + // .not().isArray(); this guard means any future caller cannot write one + // into a scalar column (a PG insert error, or `[false]` coerced to true). + const scalar = (v) => (v !== null && typeof v === 'object' ? undefined : v); + const pick = (key) => { + const fromBody = scalar(body[key]); + return fromBody !== undefined ? fromBody : defaults[key]; + }; + + const level = pick('protection_level'); + if (level !== undefined) columns.protection_level = level; + + const quality = pick('image_quality'); + if (quality !== undefined) columns.image_quality = quality; + + const canvas = pick('use_canvas_rendering'); + if (canvas !== undefined) columns.use_canvas_rendering = formatBoolean(canvas); + + + return columns; +}; + // Helper to get branding defaults for new events (Feature 7: Branding Inheritance). // // Note: `branding_logo_position` (header bar — left/center/right) is a @@ -527,7 +692,10 @@ module.exports = { getStoragePath, getEventFieldRequirements, readBooleanSetting, + decodeSettingValue, getDownloadProtectionDefaults, + getImageSecurityDefaults, + resolveImageSecurityColumns, getBrandingDefaults, getCustomerNameFromPayload, getCustomerEmailFromPayload, diff --git a/backend/src/routes/adminImageSecurity.js b/backend/src/routes/adminImageSecurity.js index 877a5421..f2dd035e 100644 --- a/backend/src/routes/adminImageSecurity.js +++ b/backend/src/routes/adminImageSecurity.js @@ -4,6 +4,7 @@ const { adminAuth } = require('../middleware/auth'); const { requirePermission } = require('../middleware/permissions'); const secureImageMiddleware = require('../middleware/secureImageMiddleware'); const logger = require('../utils/logger'); +const { decodeSettingValue } = require('./adminEvents/helpers'); const router = express.Router(); @@ -22,7 +23,6 @@ router.get('/settings', adminAuth, requirePermission(['settings.view', 'image_se 'max_image_requests_per_hour', 'suspicious_activity_threshold', 'enable_canvas_rendering', - 'default_fragmentation_level', 'security_monitoring_enabled', 'block_suspicious_ips', 'log_security_events_to_db', @@ -32,9 +32,15 @@ router.get('/settings', adminAuth, requirePermission(['settings.view', 'image_se const config = {}; settings.forEach(setting => { - // PostgreSQL JSON columns are already parsed by the driver - // Just use the value directly - no need to JSON.parse - config[setting.setting_key] = setting.setting_value; + // setting_value is JSON text on SQLite, and already decoded by the + // driver on a PG json column — so returning it raw shipped strings + // like "true" to a tab that types the field as boolean. Worse, the + // tab PUTs this whole object straight back through JSON.stringify, + // so every save wrapped another layer of quoting around values nobody + // edited, until consumers could no longer read them (#1296). Decode + // here so a round trip is idempotent. This terminates: each parse of + // a string is strictly shorter than its input. + config[setting.setting_key] = decodeSettingValue(setting.setting_value); }); res.json(config); @@ -61,7 +67,6 @@ router.put('/settings', adminAuth, requirePermission('image_security.manage'), a 'max_image_requests_per_hour', 'suspicious_activity_threshold', 'enable_canvas_rendering', - 'default_fragmentation_level', 'security_monitoring_enabled', 'block_suspicious_ips', 'log_security_events_to_db', diff --git a/backend/src/routes/gallery.js b/backend/src/routes/gallery.js index b87e1855..4a5a162e 100644 --- a/backend/src/routes/gallery.js +++ b/backend/src/routes/gallery.js @@ -1227,7 +1227,6 @@ router.get('/:slug/photos', verifyGalleryAccess, resolveGuest, noStoreCache, asy protection_level: req.event.protection_level || 'standard', image_quality: req.event.image_quality || 85, use_canvas_rendering: parseBooleanInput(req.event.use_canvas_rendering, false), - fragmentation_level: req.event.fragmentation_level || 3, overlay_protection: parseBooleanInput(req.event.overlay_protection, true) }; diff --git a/backend/src/routes/protectedImages.js b/backend/src/routes/protectedImages.js index 6f94c9ff..a018522c 100644 --- a/backend/src/routes/protectedImages.js +++ b/backend/src/routes/protectedImages.js @@ -117,8 +117,7 @@ router.get('/:slug/photo/:photoId/view', verifyGalleryAccess, blockHiddenGallery const protectionSettings = { protectionLevel: eventProtectionLevel, quality: req.event.image_quality || 85, - addFingerprint: req.event.add_fingerprint !== false, - fragmentImage: eventProtectionLevel === 'maximum' + addFingerprint: req.event.add_fingerprint !== false }; // Resolve photo location through the storage backend (managed) or local @@ -151,21 +150,6 @@ router.get('/:slug/photo/:photoId/view', verifyGalleryAccess, blockHiddenGallery ? await withLocalCopy(storageKey, runProcessing) : await runProcessing(resolvePhotoFilePath(req.event, photo)); - if (processedImage.type === 'fragmented') { - return res.json({ - type: 'fragmented', - fragments: processedImage.fragments.map(f => ({ - index: f.index, - row: f.row, - col: f.col, - data: f.buffer.toString('base64'), - position: f.position - })), - dimensions: processedImage.originalDimensions, - fragmentDimensions: processedImage.fragmentDimensions - }); - } - finalImage = processedImage; } diff --git a/backend/src/routes/secureImages.js b/backend/src/routes/secureImages.js index c0b229c6..bf0d9684 100644 --- a/backend/src/routes/secureImages.js +++ b/backend/src/routes/secureImages.js @@ -120,8 +120,6 @@ router.get('/:slug/secure/:photoId/:token', tokenLength: token?.length, hasAuthHeader: Boolean(req.headers.authorization), }); - const { fragment } = req.query; - // Verify secure token const tokenValidation = secureImageService.verifySecureToken( token, @@ -212,8 +210,7 @@ router.get('/:slug/secure/:photoId/:token', const protectionSettings = { protectionLevel: event.protection_level || 'standard', quality: event.image_quality || 85, - addFingerprint: event.add_fingerprint !== false, - fragmentImage: event.use_canvas_rendering === true && fragment !== undefined + addFingerprint: event.add_fingerprint !== false }; let processedImage; @@ -232,11 +229,6 @@ router.get('/:slug/secure/:photoId/:token', return res.status(404).json({ error: 'Photo file not found' }); } - // Handle fragmented images - if (processedImage.type === 'fragmented') { - return await handleFragmentedImage(req, res, processedImage, fragment); - } - // Log successful access await secureImageService.logImageAccess( photoId, @@ -267,58 +259,6 @@ router.get('/:slug/secure/:photoId/:token', } ); -/** - * Handle fragmented image delivery - */ -async function handleFragmentedImage(req, res, fragmentedImage, fragmentIndex) { - const { photoId } = req.params; - - try { - if (fragmentIndex === undefined) { - // Return fragment metadata - res.json({ - type: 'fragmented', - fragments: fragmentedImage.fragments.length, - dimensions: fragmentedImage.originalDimensions, - fragmentDimensions: fragmentedImage.fragmentDimensions - }); - return; - } - - const index = parseInt(fragmentIndex); - if (isNaN(index) || index < 0 || index >= fragmentedImage.fragments.length) { - return res.status(400).json({ error: 'Invalid fragment index' }); - } - - const fragment = fragmentedImage.fragments[index]; - - // Log fragment access - await secureImageService.logImageAccess( - photoId, - req.event.id, - req.clientInfo, - `fragment_${index}` - ); - - res.set({ - 'Content-Type': 'image/jpeg', - 'Content-Length': fragment.buffer.length, - 'X-Fragment-Index': index, - 'X-Fragment-Position': JSON.stringify(fragment.position) - }); - - res.send(fragment.buffer); - - } catch (error) { - logger.error('Error serving image fragment', { - error: error.message, - fragmentIndex, - photoId - }); - res.status(500).json({ error: 'Failed to serve image fragment' }); - } -} - /** * Download protected image with watermark */ diff --git a/backend/src/routes/v1/__tests__/events.create.test.js b/backend/src/routes/v1/__tests__/events.create.test.js index 1e973a86..92b1ad8e 100644 --- a/backend/src/routes/v1/__tests__/events.create.test.js +++ b/backend/src/routes/v1/__tests__/events.create.test.js @@ -126,6 +126,7 @@ const BASE_BODY = { const baseSettingsChains = () => [ buildChain({ firstResult: null }), // feedback default buildChain({ firstResult: null }), // devtools default + buildChain({ selectResult: [] }), // image-security whereIn → empty rows (#1296) buildChain({ selectResult: [] }), // branding whereIn → empty rows ]; @@ -169,19 +170,21 @@ describe('v1 POST /events — issue #550 (color_theme + feedback row)', () => { it('creates event_feedback_settings row when feedback_enabled=true is sent', async () => { // feedback_enabled provided → feedback probe SKIPPED. Sequence: // 1. devtools probe - // 2. branding probe (whereIn → select) - // 3. slug probe - // 4. events insert - // 5. feedback sub-toggle defaults probe (whereIn → select, #1044) - // 6. event_feedback_settings insert + // 2. image-security probe (whereIn → select, #1296) + // 3. branding probe (whereIn → select) + // 4. slug probe + // 5. events insert + // 6. feedback sub-toggle defaults probe (whereIn → select, #1044) + // 7. event_feedback_settings insert const devtoolsChain = buildChain({ firstResult: null }); + const imageSecurityChain = buildChain({ selectResult: [] }); const brandingChain = buildChain({ selectResult: [] }); const slugChain = buildChain({ firstResult: null }); const insertChain = buildChain({ returningResult: [{ id: 50 }] }); const feedbackDefaultsChain = buildChain({ selectResult: [] }); const feedbackInsertChain = buildChain(); db.__setImplementations( - devtoolsChain, brandingChain, slugChain, insertChain, + devtoolsChain, imageSecurityChain, brandingChain, slugChain, insertChain, feedbackDefaultsChain, feedbackInsertChain, ); @@ -190,7 +193,7 @@ describe('v1 POST /events — issue #550 (color_theme + feedback row)', () => { .send({ ...BASE_BODY, feedback_enabled: true }) .expect(201); - expect(db).toHaveBeenNthCalledWith(6, 'event_feedback_settings'); + expect(db).toHaveBeenNthCalledWith(7, 'event_feedback_settings'); const feedbackRow = feedbackInsertChain.insert.mock.calls[0][0]; expect(feedbackRow).toMatchObject({ event_id: 50 }); @@ -216,20 +219,21 @@ describe('v1 POST /events — issue #550 (color_theme + feedback row)', () => { it('honours the event_default_feedback_enabled global when body omits feedback_enabled', async () => { // Feedback probe returns serialized "true" → fallback kicks in and // the feedback insert runs. Sequence: feedback probe, devtools probe, - // branding probe, slug, insert, sub-toggle defaults probe (#1044), - // feedback insert (7 calls total). + // image-security probe (#1296), branding probe, slug, insert, sub-toggle + // defaults probe (#1044), feedback insert (8 calls total). const feedbackProbe = buildChain({ firstResult: { setting_key: 'event_default_feedback_enabled', setting_value: 'true' }, }); const devtoolsChain = buildChain({ firstResult: null }); + const imageSecurityChain = buildChain({ selectResult: [] }); const brandingChain = buildChain({ selectResult: [] }); const slugChain = buildChain({ firstResult: null }); const insertChain = buildChain({ returningResult: [{ id: 51 }] }); const feedbackDefaultsChain = buildChain({ selectResult: [] }); const feedbackInsertChain = buildChain(); db.__setImplementations( - feedbackProbe, devtoolsChain, brandingChain, slugChain, insertChain, - feedbackDefaultsChain, feedbackInsertChain, + feedbackProbe, devtoolsChain, imageSecurityChain, brandingChain, slugChain, + insertChain, feedbackDefaultsChain, feedbackInsertChain, ); await request(buildApp()) @@ -237,7 +241,7 @@ describe('v1 POST /events — issue #550 (color_theme + feedback row)', () => { .send(BASE_BODY) .expect(201); - expect(db).toHaveBeenNthCalledWith(7, 'event_feedback_settings'); + expect(db).toHaveBeenNthCalledWith(8, 'event_feedback_settings'); expect(feedbackInsertChain.insert).toHaveBeenCalledTimes(1); }); @@ -251,9 +255,9 @@ describe('v1 POST /events — issue #550 (color_theme + feedback row)', () => { .send(BASE_BODY) .expect(201); - // 5 db() calls: feedback + devtools + branding probes, slug, insert. - // event_feedback_settings is never touched. - expect(db).toHaveBeenCalledTimes(5); + // 6 db() calls: feedback + devtools + image-security + branding probes, + // slug, insert. event_feedback_settings is never touched. + expect(db).toHaveBeenCalledTimes(6); expect(db).not.toHaveBeenCalledWith('event_feedback_settings'); }); diff --git a/backend/src/routes/v1/events.js b/backend/src/routes/v1/events.js index fd4cb2a9..982c5bc4 100644 --- a/backend/src/routes/v1/events.js +++ b/backend/src/routes/v1/events.js @@ -37,6 +37,7 @@ const logger = require('../../utils/logger'); const { slugify } = require('../../utils/slug'); const { formatBoolean } = require('../../utils/dbCompat'); const { parseBooleanInput } = require('../../utils/parsers'); +const { getImageSecurityDefaults, resolveImageSecurityColumns, decodeSettingValue } = require('../adminEvents/helpers'); const { isValidEventType } = require('../../services/eventTypeService'); const { replacePhoto } = require('../../services/photoReplacementService'); const { getMaxFileSizeBytes, DEFAULT_MAX_FILE_SIZE_MB } = require('../../services/uploadSettings'); @@ -134,6 +135,9 @@ const photoUpload = async (req, res, next) => { * color_theme: { type: string, nullable: true, description: "Preset name (e.g. 'default') or JSON-encoded ThemeConfig. Persisted as-is on the event row." } * feedback_enabled: { type: boolean, nullable: true, description: "Enable guest feedback for this gallery. When omitted, falls back to the global event_default_feedback_enabled setting." } * enable_devtools_protection: { type: boolean, nullable: true, description: "Block right-click / devtools shortcuts in the gallery. When omitted, falls back to the global enable_devtools_protection setting." } + * protection_level: { type: string, nullable: true, enum: [basic, standard, enhanced, maximum], description: "Image protection level. When omitted, falls back to the global default_protection_level setting." } + * use_canvas_rendering: { type: boolean, nullable: true, description: "Render gallery images to a canvas instead of an img tag. When omitted, falls back to the global enable_canvas_rendering setting." } + * image_quality: { type: integer, minimum: 1, maximum: 100, nullable: true, description: "Served image quality percentage. When omitted, falls back to the global default_image_quality setting." } * hero_logo_visible: { type: boolean, nullable: true, description: "Show event logo in the hero block. When omitted, falls back to the global branding_logo_display_hero setting." } * hero_logo_size: { type: string, nullable: true, enum: [small, medium, large, xlarge], description: "Hero logo size. When omitted, falls back to the global branding_logo_size setting." } * hero_logo_position: { type: string, nullable: true, enum: [top, center, bottom], description: "Hero logo position. Defaults to 'top' (not settings-backed — see migration 084)." } @@ -179,6 +183,9 @@ router.post( body('color_theme').optional({ nullable: true }).isString().trim(), body('feedback_enabled').optional().isBoolean(), body('enable_devtools_protection').optional().isBoolean(), + body('protection_level').optional().not().isArray().isIn(['basic', 'standard', 'enhanced', 'maximum']), + body('use_canvas_rendering').optional().not().isArray().isBoolean().toBoolean(), + body('image_quality').optional().not().isArray().isInt({ min: 1, max: 100 }).toInt(), body('hero_logo_visible').optional().isBoolean(), body('hero_logo_size').optional().isIn(['small', 'medium', 'large', 'xlarge']), body('hero_logo_position').optional().isIn(['top', 'center', 'bottom']) @@ -227,14 +234,24 @@ router.post( if (devtoolsInput === undefined) { const setting = await db('app_settings').where('setting_key', 'enable_devtools_protection').first(); if (setting) { - try { - const parsed = JSON.parse(setting.setting_value); - if (typeof parsed === 'boolean') devtoolsFallback = parsed; - } catch { /* keep true */ } + // Shared decoder: a legacy row can carry several layers of JSON + // quoting, and a single parse would leave the string 'false' here, + // reject it, and quietly enable protection the operator disabled. + const parsed = decodeSettingValue(setting.setting_value); + if (typeof parsed === 'boolean') devtoolsFallback = parsed; } } const enable_devtools_protection = parseBooleanInput(devtoolsInput, devtoolsFallback); + // #1296 — same shape again, for the four Image Security settings that + // were stored and applied nowhere. Shared with the admin create route + // so a gallery's security level does not depend on which endpoint made + // it; #592 above is the bug this would otherwise repeat. + const imageSecurityColumns = resolveImageSecurityColumns( + req.body, + await getImageSecurityDefaults(), + ); + // Same shape as the feedback / devtools fallbacks: honour the global // event_default_require_password toggle (#317). Without this an admin // who disabled "require password by default" globally still got @@ -324,6 +341,8 @@ router.post( // Issue #592 — write the resolved devtools setting (input value // or global fallback) so the column default doesn't shadow it. enable_devtools_protection: formatBoolean(enable_devtools_protection), + // Request value, else the global default, else the column default. + ...imageSecurityColumns, // Branding inheritance — resolved value from body or app_settings. hero_logo_visible: formatBoolean(hero_logo_visible), hero_logo_size, diff --git a/backend/src/services/contract/conversions.js b/backend/src/services/contract/conversions.js index 2b3548cb..0450c992 100644 --- a/backend/src/services/contract/conversions.js +++ b/backend/src/services/contract/conversions.js @@ -229,6 +229,8 @@ async function convertToEvent(contractId, adminId) { || (await resolveDefaultEventType()); const eventCols = await db('events').columnInfo(); + const { getImageSecurityDefaults, resolveImageSecurityColumns } = require('../../routes/adminEvents/helpers'); + const imageSecurityColumns = resolveImageSecurityColumns({}, await getImageSecurityDefaults()); const candidate = { slug: `contract-${contract.contract_number.toLowerCase()}-${crypto.randomBytes(3).toString('hex')}`, // Prefer the contract's event_name snapshot (set on the contract @@ -255,6 +257,11 @@ async function convertToEvent(contractId, adminId) { quote_id: null, created_at: new Date(), updated_at: new Date(), + // #1296 — a signed standalone contract converts straight to a gallery + // here, without going through quoteService, so the global Image Security + // defaults have to be applied on this path too. Not inside a transaction, + // so the global db read is fine. + ...imageSecurityColumns, }; const eventRow = {}; for (const [k, v] of Object.entries(candidate)) { diff --git a/backend/src/services/newsletterService.js b/backend/src/services/newsletterService.js index 46da3528..9c991ef2 100644 --- a/backend/src/services/newsletterService.js +++ b/backend/src/services/newsletterService.js @@ -141,35 +141,81 @@ function sanitizeCampaignBody(html) { }, }) // sanitize-html keeps the style ATTRIBUTE contents verbatim. Clean each. - .replace(/style="([^"]*)"/gi, (match, css) => { - const { sanitized } = sanitizeCSS(css); - const cleaned = stripRemoteCssUrls(sanitized); - return cleaned ? `style="${cleaned.replace(/"/g, '')}"` : ''; - }); + // sanitizeCSS blocks remote url() properly as of #1290 — it lexes the CSS + // rather than pattern-matching it, so the local pass this used to need is + // gone. Keeping a second copy would mean two definitions of "disallowed" + // drifting apart. + // + // Entities are decoded BEFORE the CSS is scanned, and re-encoded after. + // sanitize-html emits `"` inside an attribute as `"`, so the scanner + // and the recipient's browser otherwise disagree about where CSS strings + // begin: in `style="font-family:"don't";background:url(...)"` + // the browser decodes first and reads the apostrophe as ordinary text + // inside a real string, then makes the url() request — while the scanner + // saw the apostrophe open a string and skipped everything after it. The + // scanner has to be shown what the browser will actually parse. + .replace(STYLE_ATTRIBUTE, sanitizeStyleAttribute); +} + +/** Shared by the write-time sanitize and the post-substitution recheck. */ +const STYLE_ATTRIBUTE = /style="([^"]*)"/gi; + +function sanitizeStyleAttribute(match, css) { + const { sanitized } = sanitizeCSS(decodeHtmlEntities(css)); + return sanitized ? `style="${encodeForAttribute(sanitized)}"` : ''; } /** - * Remove every `url(...)` that is not an inline data: image. + * Re-check inline CSS AFTER template substitution. * - * The shared `sanitizeCSS` *detects* a remote url() and prefixes it with a - * `/* BLOCKED URL *\/` comment — but a CSS comment is stripped during - * tokenization, so the declaration a mail client actually parses still - * carries the live URL. Verified: + * Sanitizing runs on the stored body, but `safeTemplateReplace` rewrites it + * afterwards — so the string that was validated is not the string that gets + * sent. A conditional inside a style attribute can delete the very characters + * that made a URL inert: * - * sanitizeCSS('.a{background:url(https://x/p.gif)}').sanitized - * → '.a{background:/* BLOCKED URL *\/ url(https://x/p.gif)}' + * style="--x:x{{#if company_name}}'{{/if}};background:url(https://evil…)" * - * In a newsletter that is a tracking pixel delivered to every recipient, so - * this pass actually removes the token. Scoped to the newsletter path on - * purpose: the same weakness affects gallery custom CSS, but changing shared - * sanitizer behaviour is a separate change with its own blast radius. + * At sanitize time the url() sits inside a CSS string and is correctly left + * alone; once the conditional is expanded the quotes are gone and the + * background is live. No amount of lexer correctness fixes that, because the + * text being lexed is not the text being delivered — the check has to run + * again on the final output. Substitution cannot introduce a `"` (values are + * HTML-escaped), so the attribute regex still matches what it should. */ -function stripRemoteCssUrls(css) { - if (!css) return ''; - return String(css) - .replace(/\/\*\s*BLOCKED URL\s*\*\//gi, '') - .replace(/url\s*\(\s*(['"]?)([^)'"]*)\1\s*\)/gi, (match, _quote, target) => - (/^data:image\/(?:jpeg|jpg|png|gif|webp)/i.test(target.trim()) ? match : 'none')); +function sanitizeInlineStylesAfterSubstitution(html) { + if (!html) return html; + return String(html).replace(STYLE_ATTRIBUTE, sanitizeStyleAttribute); +} + +/** + * Decode the HTML entities sanitize-html emits inside attribute values, so + * CSS is scanned in the form the recipient's parser will see. One pass, so + * `&quot;` decodes to `"` and not to `"`. + */ +function decodeHtmlEntities(value) { + return String(value).replace( + /&(?:#(\d+)|#[xX]([0-9a-fA-F]+)|(quot|apos|amp|lt|gt));/g, + (whole, dec, hex, name) => { + if (dec !== undefined) { + const code = Number(dec); + return code >= 0 && code <= 0x10ffff ? String.fromCodePoint(code) : whole; + } + if (hex !== undefined) { + const code = parseInt(hex, 16); + return code >= 0 && code <= 0x10ffff ? String.fromCodePoint(code) : whole; + } + return { quot: '"', apos: '\'', amp: '&', lt: '<', gt: '>' }[name]; + } + ); +} + +/** Re-encode a sanitized value so it is safe inside a double-quoted attribute. */ +function encodeForAttribute(value) { + return String(value) + .replace(/&/g, '&') + .replace(/"/g, '"') + .replace(//g, '>'); } /** @@ -178,7 +224,8 @@ function stripRemoteCssUrls(css) { * `javascript:` and every `url()` that is not a `data:` image. * * That is STRICTER than the issue's "https: images only" note — the shared - * sanitizer allows no remote `url()` at all. Kept as-is rather than loosened: + * sanitizer allows no remote `url()` at all, and since #1290 it enforces + * that by lexing rather than by pattern-matching. Kept as-is rather than loosened: * a remote CSS url() in mail is a tracking pixel by another name, and a * campaign's images belong in `` tags where the scheme filter sees them. * @@ -187,7 +234,7 @@ function stripRemoteCssUrls(css) { function sanitizeCampaignCss(css) { if (!css) return { css: '', warnings: [] }; const { sanitized, warnings } = sanitizeCSS(String(css)); - return { css: stripRemoteCssUrls(sanitized), warnings }; + return { css: sanitized, warnings }; } // --------------------------------------------------------------------------- @@ -310,7 +357,12 @@ async function renderForRecipient(campaign, customer, options = {}) { // Substitution happens AFTER sanitizing, with escaping on: a customer's own // company name is untrusted text and must not be able to inject markup by // riding in through a variable the sanitizer never saw. - const body = safeTemplateReplace(safeBody, variables, { escapeHtml: true }); + // Re-checked after substitution, not only before it: expansion can remove + // the quoting that made a url() inert at sanitize time. See + // sanitizeInlineStylesAfterSubstitution. + const body = sanitizeInlineStylesAfterSubstitution( + safeTemplateReplace(safeBody, variables, { escapeHtml: true }) + ); const subject = safeTemplateReplace(campaign.subject || '', variables); const { css } = sanitizeCampaignCss(campaign.body_css); @@ -922,6 +974,7 @@ async function sendTest(campaignId, toEmail, adminId) { module.exports = { sanitizeCampaignBody, + sanitizeInlineStylesAfterSubstitution, sanitizeCampaignCss, unsubscribeToken, verifyUnsubscribeToken, diff --git a/backend/src/services/quoteService.js b/backend/src/services/quoteService.js index 9cfdd399..e6943973 100644 --- a/backend/src/services/quoteService.js +++ b/backend/src/services/quoteService.js @@ -1683,6 +1683,8 @@ async function convertToEvent(quoteId, adminId, options = {}) { // ask the DB which columns exist and only keep the matching pairs // — bullet-proof against schema drift in either direction. const eventCols = await trx('events').columnInfo(); + const { getImageSecurityDefaults, resolveImageSecurityColumns } = require('../routes/adminEvents/helpers'); + const imageSecurityColumns = resolveImageSecurityColumns({}, await getImageSecurityDefaults(trx)); const candidate = { slug: `quote-${quote.quote_number.toLowerCase()}-${crypto.randomBytes(3).toString('hex')}`, event_name: quote.event_name || `Event ${quote.quote_number}`, @@ -1705,6 +1707,11 @@ async function convertToEvent(quoteId, adminId, options = {}) { quote_id: quote.id, created_at: new Date(), updated_at: new Date(), + // #1296 — a converted quote produces a real gallery, so the global + // Image Security defaults have to reach it too. Required lazily: this + // is a service reaching into a route helper, and the lazy form keeps + // the module graph acyclic the way the storage require below does. + ...imageSecurityColumns, }; const eventRow = {}; for (const [k, v] of Object.entries(candidate)) { diff --git a/backend/src/services/secureImageService.js b/backend/src/services/secureImageService.js index 7e006940..49a7c28b 100644 --- a/backend/src/services/secureImageService.js +++ b/backend/src/services/secureImageService.js @@ -175,8 +175,7 @@ class SecureImageService { quality = 85, maxWidth = 1920, maxHeight = 1080, - addFingerprint = true, - fragmentImage = false + addFingerprint = true } = options; try { @@ -187,7 +186,7 @@ class SecureImageService { // For standard protection without fingerprinting, return original file // This avoids unnecessary recompression when no protection features are needed - if (protectionLevel === 'standard' && !addFingerprint && !fragmentImage) { + if (protectionLevel === 'standard' && !addFingerprint) { return await fs.readFile(imagePath); } @@ -268,14 +267,7 @@ class SecureImageService { }); } - const buffer = await image.toBuffer(); - - // Fragment image if requested (for canvas reconstruction) - if (fragmentImage && protectionLevel === 'maximum') { - return await this.fragmentImageBuffer(buffer, metadata); - } - - return buffer; + return await image.toBuffer(); } catch (error) { logger.error('Error processing protected image:', error); // Return original on error @@ -283,51 +275,6 @@ class SecureImageService { } } - /** - * Fragment image into multiple pieces for canvas reconstruction - */ - async fragmentImageBuffer(buffer, metadata) { - const { width, height } = metadata; - const fragments = []; - - // Create 3x3 grid of fragments - const cols = 3; - const rows = 3; - const fragmentWidth = Math.floor(width / cols); - const fragmentHeight = Math.floor(height / rows); - - for (let row = 0; row < rows; row++) { - for (let col = 0; col < cols; col++) { - const left = col * fragmentWidth; - const top = row * fragmentHeight; - - const fragment = await sharp(buffer) - .extract({ - left, - top, - width: fragmentWidth, - height: fragmentHeight - }) - .toBuffer(); - - fragments.push({ - index: row * cols + col, - row, - col, - buffer: fragment, - position: { left, top, width: fragmentWidth, height: fragmentHeight } - }); - } - } - - return { - type: 'fragmented', - fragments, - originalDimensions: { width, height }, - fragmentDimensions: { width: fragmentWidth, height: fragmentHeight, cols, rows } - }; - } - /** * Log image access for security monitoring */ diff --git a/backend/src/utils/cssSanitizer.js b/backend/src/utils/cssSanitizer.js index 578262de..b8780d68 100644 --- a/backend/src/utils/cssSanitizer.js +++ b/backend/src/utils/cssSanitizer.js @@ -147,11 +147,27 @@ function stripDisallowedUrls(css) { if (input[i] === '"' || input[i] === '\'') { const quote = input[i]; let j = i + 1; + let closed = false; while (j < input.length) { if (input[j] === '\\') { j += 2; continue; } - if (input[j] === quote) { j += 1; break; } + // A newline ends a string in CSS (it produces a bad-string token), so + // an unclosed quote must not run past the end of its own line. + if (input[j] === '\n' || input[j] === '\r' || input[j] === '\f') break; + if (input[j] === quote) { j += 1; closed = true; break; } j += 1; } + // An UNTERMINATED quote is a parse error, and trusting it is how a + // stray apostrophe hid everything after it: `font-family:"don't` + // opened a string that swallowed the url() following it, while the + // recipient's browser — which decodes the entity first — saw the + // apostrophe safely inside a real string and made the request. Failing + // closed here means emitting the quote as an ordinary character and + // carrying on scanning, so a later url() is still examined. + if (!closed) { + out += input[i]; + i += 1; + continue; + } out += input.slice(i, Math.min(j, input.length)); i = Math.min(j, input.length); continue; @@ -161,7 +177,7 @@ function stripDisallowedUrls(css) { 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; + while (j < input.length && CSS_WS.test(input[j])) j += 1; if (input[j] === '(') { const token = readUrlToken(input, j); if (token) { @@ -185,6 +201,19 @@ function stripDisallowedUrls(css) { continue; } + // --- escape that does NOT begin an identifier ------------------------- + // Ordered AFTER readIdentifier deliberately. `\75` is the escape for + // `u`, so `\75rl(...)` is url() to a browser — consuming the escape + // first would hide it from the check above, which is a bypass this + // branch introduced when it ran earlier. What is left for it is the + // `\'` case: an escaped quote that must not be read as opening a + // string, since that swallowed the rest of the stylesheet unscanned. + if (input[i] === '\\' && i + 1 < input.length) { + out += input.slice(i, i + 2); + i += 2; + continue; + } + out += input[i]; i += 1; } @@ -209,6 +238,15 @@ function matchEscape(input, start) { * escaped Tailwind selector) is emitted byte-identical rather than silently * rewritten to `.w-1/2`, which is a different selector. */ +// CSS whitespace is exactly space, tab, LF, CR and FF. JavaScript's `\s` +// is NOT the same set — it also matches NBSP and the other Unicode spaces, +// and that difference was a bypass: in `url(\u00a0"data:image/png);...")` +// the scanner skipped the NBSP as whitespace and read the following quote as +// a legitimate quoted data: URI, swallowing a remote url() inside it. A +// browser treats NBSP as an ordinary character, making that an UNQUOTED +// url-token that ends at the first `)` — leaving the remote background live. +const CSS_WS = /[ \t\n\r\f]/; + function readIdentifier(input, start) { let j = start; let raw = ''; @@ -228,7 +266,7 @@ function readIdentifier(input, start) { function readUrlToken(input, openParen) { let j = openParen + 1; let target = ''; - while (j < input.length && /\s/.test(input[j])) j += 1; + while (j < input.length && CSS_WS.test(input[j])) j += 1; if (input[j] === '"' || input[j] === '\'') { // Quoted: the quote closes the value, so ")" inside it is content. @@ -249,7 +287,7 @@ function readUrlToken(input, openParen) { } } - while (j < input.length && /\s/.test(input[j])) j += 1; + while (j < input.length && CSS_WS.test(input[j])) j += 1; // Unterminated url( — malformed. Leave it alone rather than swallowing the // remainder of the stylesheet. if (input[j] !== ')') return null; @@ -291,12 +329,35 @@ function sanitizeCSS(cssContent) { // 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. + // Remove control characters BEFORE the URL scan. This is the same + // token-joining hazard as the HTML-comment strip above: dropping the + // \u0001 from `u\u0001rl(https://evil.example/p.gif)` joins the remainder + // into a live `url(...)`, so a scan that ran first saw no token and + // reported the input clean. Newlines are control characters too, which + // made `u\nrl(...)` the same bypass in ordinary-looking CSS. + // eslint-disable-next-line no-control-regex -- intentional: strips control chars from untrusted CSS + sanitized = sanitized.replace(/[\u0000-\u001F\u007F]/g, ''); + + // URLs are scanned AFTER the comment and control-character strips, 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. + // Remove any remaining script-like content. This is the LAST pass that can + // move text, and so it must run before the URL scan, not after: it deletes + // the matched span, and a span like `<">` takes a quote with it. That is + // how `--x:x<">;background:url(https://evil.example/p.gif);--y:x<">` shipped + // a live background — the scanner saw the url() safely inside a string, and + // this line then removed the quotes that made it so. + sanitized = sanitized.replace(/<[^>]*>/g, '/* BLOCKED TAG */'); + + // URL validation runs LAST, deliberately. Every pass above rewrites the + // text, and each one that did so after this point has produced a bypass: + // the HTML-comment strip (#1290), the control-character strip, and the tag + // strip immediately above. Validating anything other than the final bytes + // means validating a string that is not the one that gets served. const urlPass = stripDisallowedUrls(sanitized); if (urlPass.blocked > 0) { warnings.push( @@ -306,13 +367,6 @@ function sanitizeCSS(cssContent) { 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, ''); - - // Remove any remaining script-like content - sanitized = sanitized.replace(/<[^>]*>/g, '/* BLOCKED TAG */'); - return { sanitized: sanitized.trim(), warnings }; } diff --git a/frontend/package.json b/frontend/package.json index 08c24ee6..7574ec1a 100644 --- a/frontend/package.json +++ b/frontend/package.json @@ -1,7 +1,7 @@ { "name": "picpeak-frontend", "private": true, - "version": "3.123.0-beta.0", + "version": "3.124.1-beta.0", "type": "module", "scripts": { "dev": "vite", diff --git a/frontend/src/components/common/AuthenticatedImage.tsx b/frontend/src/components/common/AuthenticatedImage.tsx index bd3bc8fe..0437413a 100644 --- a/frontend/src/components/common/AuthenticatedImage.tsx +++ b/frontend/src/components/common/AuthenticatedImage.tsx @@ -11,25 +11,12 @@ import { interface AuthenticatedImageProps extends Omit, 'onLoad'> { src: string; fallbackSrc?: string; - useWatermark?: boolean; isGallery?: boolean; - protectFromDownload?: boolean; slug?: string; - photoId?: number; - requiresToken?: boolean; - secureUrlTemplate?: string; - downloadUrlTemplate?: string; - onProtectionViolation?: (violationType: string) => void; - watermarkText?: string; - overlayProtection?: boolean; - fragmentGrid?: boolean; - scrambleFragments?: boolean; useCanvasRendering?: boolean; - blockKeyboardShortcuts?: boolean; - detectPrintScreen?: boolean; - detectDevTools?: boolean; - protectionLevel?: 'basic' | 'standard' | 'enhanced' | 'maximum'; - useEnhancedProtection?: boolean; + /** Fired when the canvas branch blocks a context-menu attempt. The only + * protection callback this component actually implements (#1297). */ + onProtectionViolation?: (violationType: string) => void; onLoad?: () => void; /** * Priority in the shared fetch queue (#1287). NOT the native `fetchPriority` @@ -43,51 +30,44 @@ interface AuthenticatedImageProps extends Omit + * onProtectionViolation fires from the canvas context-menu handler below + * + * `useWatermark` was removed too. #1297 did not list it — it sat outside the + * `unusedProps` block — but it was equally inert: declared, defaulted, never + * read. + */ export const AuthenticatedImage: React.FC = ({ src, fallbackSrc, alt, - useWatermark = false, isGallery = false, - protectFromDownload, slug, - photoId, - requiresToken, - secureUrlTemplate, - downloadUrlTemplate, - onProtectionViolation, - watermarkText, - overlayProtection, - fragmentGrid, - scrambleFragments, useCanvasRendering, - blockKeyboardShortcuts, - detectPrintScreen, - detectDevTools, - protectionLevel, - useEnhancedProtection, + onProtectionViolation, onLoad, queuePriority = 'normal', ...props }) => { - const unusedProps = { - protectFromDownload, - photoId, - requiresToken, - secureUrlTemplate, - downloadUrlTemplate, - onProtectionViolation, - watermarkText, - overlayProtection, - fragmentGrid, - scrambleFragments, - blockKeyboardShortcuts, - detectPrintScreen, - detectDevTools, - protectionLevel, - useEnhancedProtection - }; - void unusedProps; const [imageSrc, setImageSrc] = useState(''); const [error, setError] = useState(false); @@ -98,14 +78,16 @@ export const AuthenticatedImage: React.FC = ({ const imageRef = useRef(null); // Draw image to canvas when canvas rendering is enabled + // Returns whether the pixels actually made it onto the canvas, so the + // caller knows if the source image is still needed (#1287). const drawToCanvas = useCallback(() => { - if (!useCanvasRendering || !canvasRef.current || !imageRef.current) return; + if (!useCanvasRendering || !canvasRef.current || !imageRef.current) return false; const canvas = canvasRef.current; const img = imageRef.current; const ctx = canvas.getContext('2d'); - if (!ctx || !img.complete || img.naturalWidth === 0) return; + if (!ctx || !img.complete || img.naturalWidth === 0) return false; // Set canvas dimensions to match image canvas.width = img.naturalWidth; @@ -115,6 +97,7 @@ export const AuthenticatedImage: React.FC = ({ ctx.drawImage(img, 0, 0); setCanvasReady(true); + return true; }, [useCanvasRendering]); useEffect(() => { @@ -271,7 +254,26 @@ export const AuthenticatedImage: React.FC = ({ img.onload = () => { imageRef.current = img; - drawToCanvas(); + const drawn = drawToCanvas(); + // Once drawImage has copied the pixels into the canvas the source + // decode is dead weight, so drop it here rather than at unmount. The + // grid is not virtualised — a 546-photo event mounts 546 of these and + // none of them unmount while the gallery is open — so a cleanup-only + // release never actually runs for the case it was meant to fix + // (#1287). Nothing redraws from `imageRef` afterwards: drawToCanvas + // has this one caller. + if (drawn) { + // Handlers off BEFORE the src goes. Measured in Chromium and WebKit: + // neither fires `error` when the attribute is removed after a + // successful load, so this is not fixing an observed bug — but if any + // engine ever did, `onerror` would set canvasFailed, swap the canvas + // for a plain , and decode the image a second time, which is the + // exact opposite of what this release is for. The ordering is free. + img.onload = null; + img.onerror = null; + imageRef.current = null; + img.removeAttribute('src'); + } onLoad?.(); }; @@ -286,6 +288,17 @@ export const AuthenticatedImage: React.FC = ({ return () => { img.onload = null; img.onerror = null; + // Fallback release for the paths the onload handler above cannot + // cover: the draw failed, or the source changed / the component + // unmounted before onload ever fired. `imageRef` is what drawToCanvas + // reads and it was never cleared, so a detached Image — and the decode + // behind it — stayed pinned by a live JS reference. A decoded in + // the document is evictable under memory pressure; one held by a ref + // is not. + if (imageRef.current === img) { + imageRef.current = null; + } + img.removeAttribute('src'); }; }, [imageSrc, useCanvasRendering, drawToCanvas, onLoad]); diff --git a/frontend/src/components/common/ProtectedImage.tsx b/frontend/src/components/common/ProtectedImage.tsx index 717fa29d..dbad8683 100644 --- a/frontend/src/components/common/ProtectedImage.tsx +++ b/frontend/src/components/common/ProtectedImage.tsx @@ -12,9 +12,6 @@ interface ProtectedImageProps extends React.CanvasHTMLAttributes void; fallbackSrc?: string; @@ -26,9 +23,6 @@ export const ProtectedImage: React.FC = ({ alt, protectionLevel = 'standard', watermarkText, - fragmentGrid = false, - gridSize = 4, - scrambleFragments = false, invisibleWatermark = false, onProtectionViolation, fallbackSrc, @@ -120,52 +114,6 @@ export const ProtectedImage: React.FC = ({ ctx.shadowOffsetY = 0; }, []); - // Fragment and scramble image for maximum protection - const renderFragmentedImage = useCallback(( - ctx: CanvasRenderingContext2D, - img: HTMLImageElement, - width: number, - height: number - ) => { - const fragmentWidth = width / gridSize; - const fragmentHeight = height / gridSize; - const fragments: Array<{ x: number; y: number; destX: number; destY: number }> = []; - - // Create fragment map - for (let row = 0; row < gridSize; row++) { - for (let col = 0; col < gridSize; col++) { - fragments.push({ - x: col * fragmentWidth, - y: row * fragmentHeight, - destX: col * fragmentWidth, - destY: row * fragmentHeight, - }); - } - } - - // Scramble fragments if requested - if (scrambleFragments) { - for (let i = fragments.length - 1; i > 0; i--) { - const j = Math.floor(Math.random() * (i + 1)); - const temp = fragments[i].destX; - const tempY = fragments[i].destY; - fragments[i].destX = fragments[j].destX; - fragments[i].destY = fragments[j].destY; - fragments[j].destX = temp; - fragments[j].destY = tempY; - } - } - - // Draw fragments - fragments.forEach(fragment => { - ctx.drawImage( - img, - fragment.x, fragment.y, fragmentWidth, fragmentHeight, - fragment.destX, fragment.destY, fragmentWidth, fragmentHeight - ); - }); - }, [gridSize, scrambleFragments]); - // Main canvas rendering function - wrapped in useCallback to prevent infinite re-renders const renderToCanvas = useCallback(() => { if (!canvasRef.current || !imageRef.current) { @@ -199,14 +147,9 @@ export const ProtectedImage: React.FC = ({ ctx.globalCompositeOperation = 'source-over'; // Reset composite operation try { - if (fragmentGrid && (protectionLevel === 'enhanced' || protectionLevel === 'maximum')) { - // Render fragmented image - renderFragmentedImage(ctx, img, canvas.width, canvas.height); - } else { - // Render normal image - ensure image is valid before drawing - if (img.naturalWidth > 0 && img.naturalHeight > 0) { - ctx.drawImage(img, 0, 0, canvas.width, canvas.height); - } + // Ensure image is valid before drawing + if (img.naturalWidth > 0 && img.naturalHeight > 0) { + ctx.drawImage(img, 0, 0, canvas.width, canvas.height); } // Apply watermarks @@ -242,7 +185,7 @@ export const ProtectedImage: React.FC = ({ reportViolation('canvas_rendering_error'); setError(true); } - }, [fragmentGrid, protectionLevel, renderFragmentedImage, watermarkText, invisibleWatermark, applyInvisibleWatermark, applyVisibleWatermark, reportViolation]); + }, [protectionLevel, watermarkText, invisibleWatermark, applyInvisibleWatermark, applyVisibleWatermark, reportViolation]); // Set up protection event listeners useEffect(() => { diff --git a/frontend/src/components/common/__tests__/AuthenticatedImage.canvasRelease.test.tsx b/frontend/src/components/common/__tests__/AuthenticatedImage.canvasRelease.test.tsx new file mode 100644 index 00000000..52b21f0f --- /dev/null +++ b/frontend/src/components/common/__tests__/AuthenticatedImage.canvasRelease.test.tsx @@ -0,0 +1,135 @@ +/** + * Canvas-mode memory release (#1287). + * + * In canvas mode the component keeps a detached `Image` in `imageRef` so + * `drawToCanvas` can read it. The effect cleanup nulled `onload`/`onerror` + * but never cleared that ref, so the Image — and the decoded bitmap behind + * it — stayed pinned by a live JS reference for the component's lifetime. + * + * That is not academic at gallery scale. The photo grid is NOT virtualised: + * a 546-photo event mounts 546 of these and none ever unmount, so nothing was + * ever released. A decoded in the document is evictable under memory + * pressure; one held by a ref is not. + */ +import { render, waitFor } from '@testing-library/react'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; + +vi.mock('../../../utils/galleryAuthStorage', () => ({ + getActiveGallerySlug: () => 'demo', + getGalleryToken: () => 'token', + inferGallerySlugFromLocation: () => 'demo', + resolveSlugFromRequestUrl: () => 'demo', +})); +vi.mock('../../../utils/url', () => ({ buildResourceUrl: (u: string) => `http://localhost${u}` })); + +import { AuthenticatedImage } from '../AuthenticatedImage'; + +/** Every Image the component constructs, so the test can inspect them. */ +const created: HTMLImageElement[] = []; +let createObjectURL: ReturnType; +let revokeObjectURL: ReturnType; + +beforeEach(() => { + created.length = 0; + vi.stubGlobal('fetch', vi.fn(async () => ({ + ok: true, + blob: async () => new Blob(['x'], { type: 'image/png' }), + }))); + // Patch the two methods rather than replacing URL — spreading the + // constructor loses its prototype and breaks every `new URL(...)`. + // Unique per call: the canvas effect keys off `imageSrc`, so a constant + // URL would make a src change look like no change at all. + let n = 0; + createObjectURL = vi.fn(() => `blob:mock-url-${++n}`); + revokeObjectURL = vi.fn(); + URL.createObjectURL = createObjectURL as unknown as typeof URL.createObjectURL; + URL.revokeObjectURL = revokeObjectURL as unknown as typeof URL.revokeObjectURL; + + const RealImage = globalThis.Image; + vi.stubGlobal('Image', class extends RealImage { + constructor() { + super(); + created.push(this as unknown as HTMLImageElement); + // jsdom leaves these at 0/false for a blob: src, which makes + // drawToCanvas bail before it draws. Present a decoded image so the + // draw path is reachable. + Object.defineProperty(this, 'complete', { get: () => true }); + Object.defineProperty(this, 'naturalWidth', { get: () => 10 }); + Object.defineProperty(this, 'naturalHeight', { get: () => 10 }); + // jsdom never fires load for a blob: src, so drive it manually. + setTimeout(() => this.onload?.(new Event('load')), 0); + } + }); +}); + +afterEach(() => vi.unstubAllGlobals()); + +describe('AuthenticatedImage canvas mode', () => { + it('releases the decoded image as soon as it is drawn, without waiting for unmount', async () => { + // The case this whole change exists for. Every other test here asserts + // release on unmount or src change — neither of which happens to a grid + // tile, because the grid is not virtualised and the tiles stay mounted + // for as long as the gallery is open. Once drawImage has copied the + // pixels the source decode is dead weight and must go immediately. + const ctx = { drawImage: vi.fn() }; + const getContext = vi + .spyOn(HTMLCanvasElement.prototype, 'getContext') + .mockReturnValue(ctx as unknown as CanvasRenderingContext2D); + + render( + + ); + + await waitFor(() => expect(created.length).toBeGreaterThan(0)); + const img = created[0]; + + await waitFor(() => expect(ctx.drawImage).toHaveBeenCalled()); + // Still mounted, still the same src — and already released. + await waitFor(() => expect(img.getAttribute('src')).toBeNull()); + + getContext.mockRestore(); + }); + + it('releases the decoded image on unmount', async () => { + const { unmount } = render( + + ); + + await waitFor(() => expect(created.length).toBeGreaterThan(0)); + const img = created[0]; + + unmount(); + + // The src is dropped so the browser can reclaim the decode without + // waiting for GC, and the handlers are detached. + expect(img.getAttribute('src')).toBeNull(); + expect(img.onload).toBeNull(); + expect(img.onerror).toBeNull(); + }); + + it('revokes the blob URL on unmount', async () => { + const { unmount } = render( + + ); + + await waitFor(() => expect(created.length).toBeGreaterThan(0)); + unmount(); + + expect(revokeObjectURL).toHaveBeenCalledWith('blob:mock-url-1'); + }); + + it('releases the previous image when the src changes', async () => { + // A recycled tile (a layout reusing a component instance for a different + // photo) must not accumulate one pinned decode per photo it has shown. + const { rerender } = render( + + ); + await waitFor(() => expect(created.length).toBe(1)); + const first = created[0]; + + rerender(); + await waitFor(() => expect(created.length).toBe(2)); + + expect(first.getAttribute('src')).toBeNull(); + }); +}); diff --git a/frontend/src/components/common/__tests__/ProtectedImage.test.tsx b/frontend/src/components/common/__tests__/ProtectedImage.test.tsx index c06ceed3..8c87155a 100644 --- a/frontend/src/components/common/__tests__/ProtectedImage.test.tsx +++ b/frontend/src/components/common/__tests__/ProtectedImage.test.tsx @@ -128,25 +128,6 @@ describe('ProtectedImage', () => { expect(mockContext.fillText).toHaveBeenCalled(); }); - it('handles fragment grid rendering', async () => { - render( - - ); - - await waitFor(() => { - const canvas = screen.getByRole('img', { name: 'Test image' }); - expect(canvas).toHaveStyle({ opacity: '1' }); - }); - - // Verify multiple drawImage calls for fragments - expect(mockContext.drawImage).toHaveBeenCalled(); - }); - it('blocks interactions in maximum protection mode', async () => { const onViolation = vi.fn(); @@ -230,25 +211,6 @@ describe('ProtectedImage', () => { expect(mockContext.putImageData).toHaveBeenCalled(); }); - it('scrambles fragments when enabled', async () => { - render( - - ); - - await waitFor(() => { - const canvas = screen.getByRole('img', { name: 'Test image' }); - expect(canvas).toHaveStyle({ opacity: '1' }); - }); - - // Fragment scrambling should result in multiple drawImage calls - expect(mockContext.drawImage).toHaveBeenCalled(); - }); - it('adds random noise in maximum protection', async () => { render( = ({ compact = false, slug, protectionLevel, - useEnhancedProtection, useCanvasRendering, - allowDownloads = true, }) => { const { t } = useTranslation(); @@ -96,12 +94,6 @@ export const GalleryFolderTiles: React.FC = ({ className="w-full h-full object-cover group-hover:scale-[1.02] transition-transform" isGallery slug={slug} - photoId={coverPhoto.id} - requiresToken={coverPhoto.requires_token} - secureUrlTemplate={coverPhoto.secure_url_template} - protectFromDownload={!allowDownloads || useEnhancedProtection} - protectionLevel={protectionLevel} - useEnhancedProtection={useEnhancedProtection} // Same rule as every other gallery image path: maximum // protection implies canvas rendering even when the separate // toggle is off (its default), otherwise a cover silently diff --git a/frontend/src/components/gallery/HeroHeader.tsx b/frontend/src/components/gallery/HeroHeader.tsx index 891ff638..6f183799 100644 --- a/frontend/src/components/gallery/HeroHeader.tsx +++ b/frontend/src/components/gallery/HeroHeader.tsx @@ -43,9 +43,7 @@ export const HeroHeader: React.FC = ({ heroLogoSize = 'medium', heroLogoPosition = 'top', dividerStyle = 'wave', - allowDownloads = true, protectionLevel = 'standard', - useEnhancedProtection = false, useCanvasRendering = false, onScrollToContent, heroImageAnchor = 'center' @@ -144,10 +142,6 @@ export const HeroHeader: React.FC = ({ style={{ objectPosition: heroImageAnchor }} isGallery={true} slug={slug} - photoId={heroPhoto.id} - protectFromDownload={!allowDownloads || useEnhancedProtection} - protectionLevel={protectionLevel} - useEnhancedProtection={useEnhancedProtection} useCanvasRendering={useCanvasRendering || protectionLevel === 'maximum'} /> diff --git a/frontend/src/components/gallery/PeopleSheet.tsx b/frontend/src/components/gallery/PeopleSheet.tsx index af339142..c0ab98b0 100644 --- a/frontend/src/components/gallery/PeopleSheet.tsx +++ b/frontend/src/components/gallery/PeopleSheet.tsx @@ -144,9 +144,6 @@ export const PeopleSheet: React.FC = ({ alt="" isGallery slug={slug} - photoId={photo.id} - requiresToken={photo.requires_token} - secureUrlTemplate={photo.secure_url_template} // Crop to the face, exactly as the strip does. Without // this a group photo shows whoever is centred — often // not the person being labelled, and identical for two diff --git a/frontend/src/components/gallery/PeopleStrip.tsx b/frontend/src/components/gallery/PeopleStrip.tsx index a85e05fd..d2bacdc7 100644 --- a/frontend/src/components/gallery/PeopleStrip.tsx +++ b/frontend/src/components/gallery/PeopleStrip.tsx @@ -86,9 +86,6 @@ const PersonAvatar: React.FC = ({ alt="" isGallery slug={slug} - photoId={photo.id} - requiresToken={photo.requires_token} - secureUrlTemplate={photo.secure_url_template} style={cropStyle || { width: '100%', height: '100%', objectFit: 'cover' }} /> ) : ( diff --git a/frontend/src/components/gallery/PhotoGrid.tsx b/frontend/src/components/gallery/PhotoGrid.tsx index 2379cb94..154e8858 100644 --- a/frontend/src/components/gallery/PhotoGrid.tsx +++ b/frontend/src/components/gallery/PhotoGrid.tsx @@ -244,7 +244,6 @@ const PhotoThumbnail: React.FC = ({ onDownload, allowDownloads = true, protectionLevel = 'standard', - useEnhancedProtection = false, useCanvasRendering = false, slug, feedbackEnabled = false @@ -269,18 +268,7 @@ const PhotoThumbnail: React.FC = ({ loading="lazy" isGallery={true} slug={slug} - photoId={photo.id} - requiresToken={photo.requires_token} - secureUrlTemplate={photo.secure_url_template} - protectFromDownload={!allowDownloads || useEnhancedProtection} - protectionLevel={protectionLevel} - useEnhancedProtection={useEnhancedProtection} useCanvasRendering={useCanvasRendering || protectionLevel === 'maximum'} - fragmentGrid={protectionLevel === 'enhanced' || protectionLevel === 'maximum'} - blockKeyboardShortcuts={useEnhancedProtection} - detectPrintScreen={useEnhancedProtection} - detectDevTools={protectionLevel === 'maximum'} - watermarkText={useEnhancedProtection ? 'Protected' : undefined} onProtectionViolation={(violationType) => { // Track analytics if (typeof window !== 'undefined' && (window as any).umami) { diff --git a/frontend/src/components/gallery/PhotoLightbox.tsx b/frontend/src/components/gallery/PhotoLightbox.tsx index d5c475c0..6daf8686 100644 --- a/frontend/src/components/gallery/PhotoLightbox.tsx +++ b/frontend/src/components/gallery/PhotoLightbox.tsx @@ -1159,9 +1159,6 @@ export const PhotoLightbox: React.FC = ({ draggable={false} isGallery={true} slug={slug} - photoId={photo.id} - requiresToken={photo.requires_token} - secureUrlTemplate={photo.secure_url_template} /> )} @@ -1189,21 +1186,9 @@ export const PhotoLightbox: React.FC = ({ transition: isDragging ? 'none' : 'transform 0.2s', }} draggable={false} - useWatermark={useEnhancedProtection} - watermarkText={useEnhancedProtection ? `${photo.filename} - Protected` : undefined} isGallery={true} slug={slug} - photoId={photo.id} - requiresToken={photo.requires_token} - secureUrlTemplate={photo.secure_url_template} - protectFromDownload={!allowDownloads || useEnhancedProtection} - protectionLevel={protectionLevel} - useEnhancedProtection={useEnhancedProtection} useCanvasRendering={useCanvasRendering || protectionLevel === 'maximum'} - fragmentGrid={protectionLevel === 'enhanced' || protectionLevel === 'maximum'} - blockKeyboardShortcuts={useEnhancedProtection} - detectPrintScreen={useEnhancedProtection} - detectDevTools={protectionLevel === 'enhanced' || protectionLevel === 'maximum'} onProtectionViolation={(violationType) => { console.warn(`Protection violation in lightbox for photo ${photo.id}: ${violationType}`); diff --git a/frontend/src/components/gallery/layouts/CarouselGalleryLayout.tsx b/frontend/src/components/gallery/layouts/CarouselGalleryLayout.tsx index f341101d..d0da929e 100644 --- a/frontend/src/components/gallery/layouts/CarouselGalleryLayout.tsx +++ b/frontend/src/components/gallery/layouts/CarouselGalleryLayout.tsx @@ -89,7 +89,6 @@ export const CarouselGalleryLayout: React.FC = ({ alt={currentPhoto.filename} className="w-full h-full object-contain" isGallery={true} - protectFromDownload={!allowDownloads} /> {/* Colour labels for the photo in view (#1189). Bottom-left because it @@ -267,7 +266,6 @@ export const CarouselGalleryLayout: React.FC = ({ className="w-full h-full object-cover" loading="lazy" isGallery={true} - protectFromDownload={!allowDownloads} /> {/* The strip is the only place this layout shows more than one photo at a time, so it is the only place a label can diff --git a/frontend/src/components/gallery/layouts/GalleryPremiumLayout.tsx b/frontend/src/components/gallery/layouts/GalleryPremiumLayout.tsx index e2dedbb9..e6c99b34 100644 --- a/frontend/src/components/gallery/layouts/GalleryPremiumLayout.tsx +++ b/frontend/src/components/gallery/layouts/GalleryPremiumLayout.tsx @@ -68,9 +68,7 @@ const PhotoCard: React.FC = ({ isSelectionMode, isLiked, slug, - allowDownloads = true, protectionLevel = 'standard', - useEnhancedProtection = false, useCanvasRendering = false, feedbackEnabled = false, allowLikes = false, @@ -124,12 +122,6 @@ const PhotoCard: React.FC = ({ loading="lazy" isGallery={true} slug={slug} - photoId={photo.id} - requiresToken={photo.requires_token} - secureUrlTemplate={photo.secure_url_template} - protectFromDownload={!allowDownloads || useEnhancedProtection} - protectionLevel={protectionLevel} - useEnhancedProtection={useEnhancedProtection} useCanvasRendering={useCanvasRendering || protectionLevel === 'maximum'} /> diff --git a/frontend/src/components/gallery/layouts/GridGalleryLayout.tsx b/frontend/src/components/gallery/layouts/GridGalleryLayout.tsx index 544154ca..66a45cd5 100644 --- a/frontend/src/components/gallery/layouts/GridGalleryLayout.tsx +++ b/frontend/src/components/gallery/layouts/GridGalleryLayout.tsx @@ -48,7 +48,6 @@ const GridPhoto: React.FC = ({ allowDownloads = true, slug, protectionLevel = 'standard', - useEnhancedProtection = false, useCanvasRendering = false, feedbackEnabled = false, feedbackOptions, @@ -84,6 +83,24 @@ const GridPhoto: React.FC = ({ onToggleSelect={onToggleSelect} className={`photo-card relative group cursor-pointer aspect-square ${animationClass}`} lazy + /* + * Pre-load band (#1287). Grid was the only lazy layout passing no + * `inViewRootMargin`, so PhotoCard ran the observer at the + * IntersectionObserver default of 0px with threshold 0.1 — a tile could + * not begin loading until a tenth of it was already on screen. The + * gallery owner's description of the symptom is that exact shape: + * spinning the wheel outran loading by ~50 images, then it caught up. + * + * Viewport-relative rather than a fixed 100px like Justified: a phone + * and a 4K desktop scroll past very different amounts of grid per + * gesture, and a band tuned to one is wrong for the other. + * + * `%`, not `vh` — rootMargin only accepts px and percentages, and an + * IntersectionObserver constructed with a vh value throws. A percentage + * resolves against the root's own box, so 100% is one viewport height + * of lead in each direction, which is what vh would have meant. + */ + inViewRootMargin="100% 0px" fadeInWhenVisible={animationType === 'fade'} skeletonClassName="skeleton aspect-square w-full rounded-lg" imageProps={{ @@ -93,18 +110,7 @@ const GridPhoto: React.FC = ({ loading: 'lazy', isGallery: true, slug, - photoId: photo.id, - requiresToken: photo.requires_token, - secureUrlTemplate: photo.secure_url_template, - protectFromDownload: !allowDownloads || useEnhancedProtection, - protectionLevel, - useEnhancedProtection, useCanvasRendering: useCanvasRendering || protectionLevel === 'maximum', - fragmentGrid: protectionLevel === 'enhanced' || protectionLevel === 'maximum', - blockKeyboardShortcuts: useEnhancedProtection, - detectPrintScreen: useEnhancedProtection, - detectDevTools: protectionLevel === 'maximum', - watermarkText: useEnhancedProtection ? 'Protected' : undefined, onProtectionViolation: (violationType: string) => { console.warn(`Protection violation on grid photo ${photo.id}: ${violationType}`); }, diff --git a/frontend/src/components/gallery/layouts/JustifiedGalleryLayout.tsx b/frontend/src/components/gallery/layouts/JustifiedGalleryLayout.tsx index a99193fe..8e0d5113 100644 --- a/frontend/src/components/gallery/layouts/JustifiedGalleryLayout.tsx +++ b/frontend/src/components/gallery/layouts/JustifiedGalleryLayout.tsx @@ -70,7 +70,6 @@ const JustifiedPhoto: React.FC = ({ allowDownloads = true, slug, protectionLevel = 'standard', - useEnhancedProtection = false, useCanvasRendering = false, feedbackEnabled = false, feedbackOptions, @@ -134,18 +133,7 @@ const JustifiedPhoto: React.FC = ({ loading: 'lazy', isGallery: true, slug, - photoId: photo.id, - requiresToken: photo.requires_token, - secureUrlTemplate: photo.secure_url_template, - protectFromDownload: !allowDownloads || useEnhancedProtection, - protectionLevel, - useEnhancedProtection, useCanvasRendering: useCanvasRendering || protectionLevel === 'maximum', - fragmentGrid: protectionLevel === 'enhanced' || protectionLevel === 'maximum', - blockKeyboardShortcuts: useEnhancedProtection, - detectPrintScreen: useEnhancedProtection, - detectDevTools: protectionLevel === 'maximum', - watermarkText: useEnhancedProtection ? 'Protected' : undefined, onProtectionViolation: (violationType: string) => { console.warn(`Protection violation on justified photo ${photo.id}: ${violationType}`); }, @@ -411,10 +399,6 @@ export const JustifiedGalleryLayout: React.FC = ({ className="w-full h-full object-cover" isGallery={true} slug={slug} - photoId={heroPhoto.id} - protectFromDownload={!allowDownloads || useEnhancedProtection} - protectionLevel={protectionLevel} - useEnhancedProtection={useEnhancedProtection} useCanvasRendering={useCanvasRendering || protectionLevel === 'maximum'} /> diff --git a/frontend/src/components/gallery/layouts/MasonryGalleryLayout.tsx b/frontend/src/components/gallery/layouts/MasonryGalleryLayout.tsx index 8c9ac998..4328d9b0 100644 --- a/frontend/src/components/gallery/layouts/MasonryGalleryLayout.tsx +++ b/frontend/src/components/gallery/layouts/MasonryGalleryLayout.tsx @@ -120,7 +120,6 @@ const MasonryPhoto: React.FC = ({ className: 'w-full h-full object-cover rounded-lg', loading: 'lazy', isGallery: true, - protectFromDownload: !allowDownloads, }} overlayBaseClassName="absolute inset-0 bg-black/40 transition-opacity duration-200 rounded-lg flex items-center justify-center gap-2" allowDownloads={allowDownloads} @@ -376,7 +375,6 @@ export const MasonryGalleryLayout: React.FC = ({ className: 'w-full h-full object-cover rounded-lg transition-transform duration-300 group-hover:scale-[1.02]', loading: 'lazy', isGallery: true, - protectFromDownload: !allowDownloads, }} overlayBaseClassName="absolute inset-0 bg-black/40 transition-opacity duration-200 rounded-lg flex items-center justify-center gap-2" actionVariant="dark" @@ -434,7 +432,6 @@ export const MasonryGalleryLayout: React.FC = ({ className: 'w-full h-full object-cover rounded-lg transition-transform duration-300 group-hover:scale-[1.02]', loading: 'lazy', isGallery: true, - protectFromDownload: !allowDownloads, }} overlayBaseClassName="absolute inset-0 bg-black/40 transition-opacity duration-200 rounded-lg flex items-center justify-center gap-2" actionVariant="dark" @@ -501,7 +498,6 @@ export const MasonryGalleryLayout: React.FC = ({ className: 'w-full h-full object-cover transition-transform duration-300 group-hover:scale-105', loading: 'lazy', isGallery: true, - protectFromDownload: !allowDownloads, }} overlayBaseClassName="absolute inset-0 bg-black/40 transition-opacity duration-200 flex items-center justify-center gap-2" actionVariant="dark" diff --git a/frontend/src/components/gallery/layouts/MosaicGalleryLayout.tsx b/frontend/src/components/gallery/layouts/MosaicGalleryLayout.tsx index 6a7b2915..05d14d01 100644 --- a/frontend/src/components/gallery/layouts/MosaicGalleryLayout.tsx +++ b/frontend/src/components/gallery/layouts/MosaicGalleryLayout.tsx @@ -83,7 +83,6 @@ const MosaicPhoto: React.FC = ({ className: 'w-full h-full object-cover transition-transform duration-300 group-hover:scale-105', loading: 'lazy', isGallery: true, - protectFromDownload: !allowDownloads, }} overlayBaseClassName="absolute inset-0 bg-black/40 transition-opacity duration-200 flex items-center justify-center gap-2" allowDownloads={allowDownloads} diff --git a/frontend/src/components/gallery/layouts/TimelineGalleryLayout.tsx b/frontend/src/components/gallery/layouts/TimelineGalleryLayout.tsx index cac34ec8..b7bccf79 100644 --- a/frontend/src/components/gallery/layouts/TimelineGalleryLayout.tsx +++ b/frontend/src/components/gallery/layouts/TimelineGalleryLayout.tsx @@ -130,7 +130,6 @@ export const TimelineGalleryLayout: React.FC = ({ className: 'w-full h-full object-cover rounded-lg', loading: 'lazy', isGallery: true, - protectFromDownload: !allowDownloads, }} overlayBaseClassName="absolute inset-0 bg-black/40 transition-opacity duration-200 rounded-lg flex items-center justify-center gap-2" allowDownloads={allowDownloads} diff --git a/frontend/src/components/gallery/layouts/__tests__/gridLazyMargin.test.ts b/frontend/src/components/gallery/layouts/__tests__/gridLazyMargin.test.ts new file mode 100644 index 00000000..13fc77c2 --- /dev/null +++ b/frontend/src/components/gallery/layouts/__tests__/gridLazyMargin.test.ts @@ -0,0 +1,59 @@ +/** + * Grid's lazy-loading pre-load band (#1287). + * + * Grid was the only layout passing `lazy` without an `inViewRootMargin`, so + * PhotoCard ran its observer at the IntersectionObserver default of `0px` + * with `threshold: 0.1` — a tile could not begin loading until a tenth of it + * was already on screen. The gallery owner described exactly that: spinning + * the scroll wheel outran loading by ~50 images before it caught up. + * + * The unit matters as much as the value. `rootMargin` accepts only px and + * percentages; an IntersectionObserver constructed with a `vh` value throws + * SyntaxError, which would have broken every Grid gallery outright. Verified + * in Chrome: + * + * '100% 0px' → accepted + * '100px 0px' → accepted + * '100vh 0px' → SyntaxError: rootMargin must be specified in pixels or percent + * + * jsdom has no IntersectionObserver, so this asserts against the source + * rather than constructing one. + */ +import { describe, it, expect } from 'vitest'; +import { readFileSync } from 'fs'; +import { resolve } from 'path'; + +const layouts = resolve(__dirname, '..'); +const read = (f: string) => readFileSync(resolve(layouts, f), 'utf8'); + +/** Only px and % are legal rootMargin units. */ +const LEGAL_ROOT_MARGIN = /^(-?\d+(px|%)|0)(\s+(-?\d+(px|%)|0)){0,3}$/; + +describe('grid lazy pre-load band', () => { + it('Grid passes an inViewRootMargin', () => { + expect(read('GridGalleryLayout.tsx')).toMatch(/inViewRootMargin=/); + }); + + it('every inViewRootMargin in every layout uses a legal unit', () => { + // A vh value throws at IntersectionObserver construction and takes the + // whole gallery down with it, so this guards the unit, not just presence. + for (const file of ['GridGalleryLayout.tsx', 'JustifiedGalleryLayout.tsx']) { + const src = read(file); + for (const [, value] of src.matchAll(/inViewRootMargin="([^"]+)"/g)) { + expect(value, `${file}: "${value}"`).toMatch(LEGAL_ROOT_MARGIN); + } + } + }); + + it('every layout that lazy-renders also declares a pre-load band', () => { + // The defect was Grid being lazy with no margin. Any future layout that + // opts into `lazy` and forgets the margin reintroduces it. + for (const file of ['GridGalleryLayout.tsx', 'JustifiedGalleryLayout.tsx']) { + const src = read(file); + const isLazy = /^\s*lazy\s*$/m.test(src) || /\slazy=\{?true/.test(src); + if (!isLazy) continue; + expect(src, `${file} is lazy but declares no inViewRootMargin`) + .toMatch(/inViewRootMargin=/); + } + }); +}); diff --git a/frontend/src/components/gallery/layouts/story/StoryHero.tsx b/frontend/src/components/gallery/layouts/story/StoryHero.tsx index 3c6e23b3..66293623 100644 --- a/frontend/src/components/gallery/layouts/story/StoryHero.tsx +++ b/frontend/src/components/gallery/layouts/story/StoryHero.tsx @@ -22,9 +22,7 @@ export const StoryHero: React.FC = ({ stats, photo, slug, - allowDownloads = true, protectionLevel = 'standard', - useEnhancedProtection = false, useCanvasRendering = false }) => { const formattedDate = date @@ -51,12 +49,6 @@ export const StoryHero: React.FC = ({ className="w-full h-full object-cover" isGallery={true} slug={slug} - photoId={photo.id} - requiresToken={photo.requires_token} - secureUrlTemplate={photo.secure_url_template} - protectFromDownload={!allowDownloads || useEnhancedProtection} - protectionLevel={protectionLevel} - useEnhancedProtection={useEnhancedProtection} useCanvasRendering={useCanvasRendering || protectionLevel === 'maximum'} /> ) : ( diff --git a/frontend/src/components/gallery/layouts/story/StoryPhotoCard.tsx b/frontend/src/components/gallery/layouts/story/StoryPhotoCard.tsx index b9fa9c76..a5eb5643 100644 --- a/frontend/src/components/gallery/layouts/story/StoryPhotoCard.tsx +++ b/frontend/src/components/gallery/layouts/story/StoryPhotoCard.tsx @@ -28,9 +28,7 @@ export const StoryPhotoCard: React.FC = ({ onToggleFavorite, onClick, slug, - allowDownloads = true, protectionLevel = 'standard', - useEnhancedProtection = false, useCanvasRendering = false, featured = false, galleryId: _galleryId @@ -105,12 +103,6 @@ export const StoryPhotoCard: React.FC = ({ }`} isGallery={true} slug={slug} - photoId={photo.id} - requiresToken={photo.requires_token} - secureUrlTemplate={photo.secure_url_template} - protectFromDownload={!allowDownloads || useEnhancedProtection} - protectionLevel={protectionLevel} - useEnhancedProtection={useEnhancedProtection} useCanvasRendering={useCanvasRendering || protectionLevel === 'maximum'} /> )} diff --git a/frontend/src/features/settings/tabs/ImageSecurityTab.tsx b/frontend/src/features/settings/tabs/ImageSecurityTab.tsx index cc464b75..7c2a3354 100644 --- a/frontend/src/features/settings/tabs/ImageSecurityTab.tsx +++ b/frontend/src/features/settings/tabs/ImageSecurityTab.tsx @@ -15,7 +15,6 @@ interface ImageSecuritySettings { max_image_requests_per_hour: number; suspicious_activity_threshold: number; enable_canvas_rendering: boolean; - default_fragmentation_level: number; security_monitoring_enabled: boolean; block_suspicious_ips: boolean; log_security_events_to_db: boolean; @@ -31,7 +30,6 @@ const defaultSettings: ImageSecuritySettings = { max_image_requests_per_hour: 500, suspicious_activity_threshold: 10, enable_canvas_rendering: false, - default_fragmentation_level: 3, security_monitoring_enabled: true, block_suspicious_ips: true, log_security_events_to_db: true, @@ -161,21 +159,6 @@ export const ImageSecurityTab: React.FC = () => { />

{t('settings.imageSecurity.imageQualityHelp', '1-100, higher = better quality')}

- -
- - handleChange('default_fragmentation_level', parseInt(e.target.value) || 3)} - className="w-full px-3 py-2 border border-neutral-300 dark:border-neutral-600 rounded-lg bg-white dark:bg-neutral-800 text-neutral-900 dark:text-neutral-100 focus:ring-2 focus:ring-primary-500 focus:border-primary-500" - /> -

{t('settings.imageSecurity.fragmentationLevelHelp', '1-10, higher = more protection')}

-
diff --git a/frontend/src/hooks/useImageProtection.ts b/frontend/src/hooks/useImageProtection.ts index 5dbe2d24..bb0b7866 100644 --- a/frontend/src/hooks/useImageProtection.ts +++ b/frontend/src/hooks/useImageProtection.ts @@ -20,7 +20,6 @@ interface UseImageProtectionOptions { blockKeyboardShortcuts?: boolean; detectPrintScreen?: boolean; watermarkText?: string; - fragmentGrid?: boolean; } export const useImageProtection = (options: UseImageProtectionOptions) => { diff --git a/frontend/src/i18n/locales/de.json b/frontend/src/i18n/locales/de.json index 04b01f68..c4073fbc 100644 --- a/frontend/src/i18n/locales/de.json +++ b/frontend/src/i18n/locales/de.json @@ -1929,7 +1929,6 @@ "defaultProtectionHelp": "Diese Einstellungen gelten für alle neuen Veranstaltungen. Einzelne Veranstaltungen können diese Standardwerte überschreiben.", "protectionLevel": "Standard-Schutzstufe", "imageQuality": "Standard-Bildqualität", - "fragmentationLevel": "Fragmentierungsstufe", "enableDevtools": "DevTools-Erkennung standardmäßig aktivieren", "enableCanvas": "Canvas-Rendering standardmäßig aktivieren (erweiterter Schutz)", "rateLimiting": "Ratenbegrenzung", @@ -1946,7 +1945,6 @@ "infoTitle": "Über Bildschutz", "infoText": "Diese Schutzfunktionen helfen, gelegentliches Herunterladen und Kopieren zu verhindern, können aber nicht alle Methoden blockieren. Entschlossene Benutzer finden möglicherweise trotzdem Wege, Bilder zu erfassen. Erwägen Sie die Verwendung von Wasserzeichen und rechtlichen Vereinbarungen für umfassenden Schutz.", "imageQualityHelp": "1–100, höher = bessere Qualität", - "fragmentationLevelHelp": "1–10, höher = mehr Schutz", "suspiciousActivityThresholdHelp": "Verstöße, bevor als verdächtig markiert wird", "autoBlockThresholdHelp": "Verstöße, bevor die IP automatisch gesperrt wird" }, @@ -7019,7 +7017,7 @@ "showCss": "Eigenes CSS (optional)", "hideCss": "Eigenes CSS ausblenden", "cssHelp": "Viele E-Mail-Programme entfernen einen