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/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/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/src/components/common/AuthenticatedImage.tsx b/frontend/src/components/common/AuthenticatedImage.tsx index 08215896..0437413a 100644 --- a/frontend/src/components/common/AuthenticatedImage.tsx +++ b/frontend/src/components/common/AuthenticatedImage.tsx @@ -78,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; @@ -95,6 +97,7 @@ export const AuthenticatedImage: React.FC = ({ ctx.drawImage(img, 0, 0); setCanvasReady(true); + return true; }, [useCanvasRendering]); useEffect(() => { @@ -251,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?.(); }; @@ -266,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/__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/gallery/layouts/GridGalleryLayout.tsx b/frontend/src/components/gallery/layouts/GridGalleryLayout.tsx index 9fe14320..66a45cd5 100644 --- a/frontend/src/components/gallery/layouts/GridGalleryLayout.tsx +++ b/frontend/src/components/gallery/layouts/GridGalleryLayout.tsx @@ -83,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={{ 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=/); + } + }); +});