From f51b9cf8df2dfba07590b35cc63def689df98c4a Mon Sep 17 00:00:00 2001 From: Paul Nothaft Date: Mon, 8 Jun 2026 17:38:01 +0200 Subject: [PATCH 1/2] fix(admin): graceful logo-img fallback + show sidebar widgets during perm hydration (#523 follow-up 2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two issues Rekoo-PS hit immediately after upgrading to v3.60.3-beta.0: 1. **Broken logo URL rendered the browser's broken-image icon + alt text.** Their `` had no `onError` handler, so a 404 / slow logo URL produced the default broken-image rendering — which uses the `alt` attribute (`companyName`) as text. Visually it looked like the wordmark span had unexpectedly re-appeared on phone, even though the actual `` was correctly hidden by the existing `wordmarkVisibilityClass` logic. Fix: - `useState` tracks `logoLoadError` (first failure) and `fallbackLoadError` (second failure). On a configured-URL miss the `` swaps to the bundled `/picpeak-kamera-transparent.png`; on a second miss the `` is removed from the DOM entirely. - `useEffect([resolvedLogoUrl])` resets both flags when the URL changes, so a dark-mode toggle that flips `lightLogo ↔ darkLogo` gets a fresh attempt instead of being permanently sad. - `wordmarkVisibilityClass` now derives from `logoEffectivelyVisible` (showLogo && !fallbackLoadError) — when both the configured URL AND the bundled fallback have failed, the wordmark un-hides on ` fallback chain also covers the broader "logo hosted on a flaky CDN" case for self-hosters, not just the one-time post-upgrade asset-cache hiccup. Pure resilience polish — no behaviour change when everything works. --- frontend/src/components/admin/AdminHeader.tsx | 44 +++++++++++++++++-- .../src/components/admin/AdminSidebar.tsx | 16 +++++-- 2 files changed, 53 insertions(+), 7 deletions(-) diff --git a/frontend/src/components/admin/AdminHeader.tsx b/frontend/src/components/admin/AdminHeader.tsx index 2f5e7446..bf2402b5 100644 --- a/frontend/src/components/admin/AdminHeader.tsx +++ b/frontend/src/components/admin/AdminHeader.tsx @@ -1,4 +1,4 @@ -import React, { useState, useRef } from 'react'; +import React, { useState, useRef, useEffect } from 'react'; import { useNavigate } from 'react-router-dom'; import { Menu, User, LogOut, Settings, Bell, Lock, CheckCircle, Trash2, Sun, Moon, Globe, ChevronDown } from 'lucide-react'; import { useTranslation } from 'react-i18next'; @@ -51,11 +51,31 @@ export const AdminHeader: React.FC = ({ onMenuClick }) => { ? (logoUrl.startsWith('http') ? logoUrl : buildResourceUrl(logoUrl)) : '/picpeak-kamera-transparent.png'; + // #523 follow-up 2: graceful fallback when the configured logo URL + // 404s or stalls. Without an error handler, the failure draws + // the browser's default broken-image-icon + alt text rendering — see + // Rekoo-PS's 3.60.3-beta.0 screenshot where "Arkan Studio" appeared + // as the alt text of a broken icon, not the real wordmark span. The + // chain: + // 1. configured URL fails → try the bundled picpeak fallback + // 2. bundled fallback fails → hide the image entirely, let the + // wordmark carry the brand + // Reset on URL change so a dark-mode toggle (which can flip lightLogo + // ↔ darkLogo) retries the new URL instead of being permanently sad. + const [logoLoadError, setLogoLoadError] = useState(false); + const [fallbackLoadError, setFallbackLoadError] = useState(false); + useEffect(() => { + setLogoLoadError(false); + setFallbackLoadError(false); + }, [resolvedLogoUrl]); + const logoImgSrc = logoLoadError ? '/picpeak-kamera-transparent.png' : resolvedLogoUrl; + // Renders the logo + wordmark block per the current logo_display_mode. // Re-used in left / center / right slots below so all three positions // produce visually identical brand chrome. const showLogo = !logoInSidebar && (logoDisplayMode === 'logo_only' || logoDisplayMode === 'logo_and_text'); const showText = logoDisplayMode === 'text_only' || logoDisplayMode === 'logo_and_text'; + const logoEffectivelyVisible = showLogo && !fallbackLoadError; // On = ({ onMenuClick }) => { // cluster it overlaps the LanguageSelector button (#523 follow-up, // Rekoo-PS's "Arkan Studio" screenshot in v3.59.0-beta.0). text_only // mode keeps the wordmark on every width — nothing else would render. - const wordmarkVisibilityClass = showLogo ? 'hidden sm:inline' : 'inline'; + // + // #523 follow-up 2: when both the configured URL AND the bundled + // fallback have failed (fallbackLoadError → logoEffectivelyVisible + // false), unhide the wordmark on { // Skeleton placeholder while `usePublicSettings()` is in flight (#523 // follow-up — Rekoo-PS's "logo took some time to load" screenshot in @@ -88,8 +113,19 @@ export const AdminHeader: React.FC = ({ onMenuClick }) => { // action buttons on narrow mobile widths (#523 regression). return (
- {showLogo && ( - {companyName} + {logoEffectivelyVisible && ( + {companyName} { + // First failure: configured URL → try the bundled fallback. + // Second failure: bundled fallback → hide entirely, let + // the wordmark carry the brand (#523 follow-up 2). + if (!logoLoadError) setLogoLoadError(true); + else setFallbackLoadError(true); + }} + /> )} {showText && ( {companyName} diff --git a/frontend/src/components/admin/AdminSidebar.tsx b/frontend/src/components/admin/AdminSidebar.tsx index bdc5e552..79aefddf 100644 --- a/frontend/src/components/admin/AdminSidebar.tsx +++ b/frontend/src/components/admin/AdminSidebar.tsx @@ -101,7 +101,7 @@ const navigation: NavItem[] = [ export const AdminSidebar: React.FC = ({ isOpen, onClose, collapsed = false, onToggleCollapse }) => { const location = useLocation(); const { t } = useTranslation(); - const { hasPermission } = usePermissions(); + const { hasPermission, isLoading: permissionsLoading } = usePermissions(); const { flags } = useFeatureFlags(); // Branding lookup for the "logo_position = sidepanel" mode — when // chosen, the logo replaces the "PicPeak Admin" text in the brand @@ -283,8 +283,18 @@ export const AdminSidebar: React.FC = ({ isOpen, onClose, col {/* Bottom section - sticky to bottom (only for users with settings.view permission). Hidden on desktop when collapsed since these widgets don't fit in the icon rail; - mobile keeps them visible because mobile width is always w-64. */} - {hasPermission('settings.view') && ( + mobile keeps them visible because mobile width is always w-64. + + #523 follow-up 2: render OPTIMISTICALLY while permissions are + still hydrating from the auth context (Rekoo-PS's 3.60.3-beta.0 + screenshot showed the whole bottom block missing on first paint + right after a deploy — `hasPermission` returns false during the + ~hundreds-of-ms hydration window, the widgets vanish entirely, + then re-appear). Only HIDE the block when we definitively know + the user lacks the permission. VersionInfo + StorageInfo each + have their own loading states so admins see "—" / a spinner + instead of nothing during the actual data fetch. */} + {(permissionsLoading || hasPermission('settings.view')) && (
{/* Version Info */} From 620163f2db77cda40b81edcac79a32cbb4fd278f Mon Sep 17 00:00:00 2001 From: Paul Nothaft Date: Mon, 8 Jun 2026 18:05:46 +0200 Subject: [PATCH 2/2] fix(downloads): transliterate accented characters in filename via NFD instead of dropping them (#607) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit patchingfailed reported on v3.44 stable: a gallery named `Ägypten` with photo `Ägypten_individual_0050.jpg` downloads as `gypten_...` — the leading umlaut is dropped entirely. Their hypothesis was a Content-Disposition encoding issue, but the actual root cause sits one layer earlier: at UPLOAD time when `generatePhotoFilename` calls `sanitizeFilename`. `sanitizeFilename` did: String(str).trim() .replace(/\s+/g, '_') .replace(/[^a-zA-Z0-9_\-\.]/g, '') // ← drops `Ä` outright .replace(/[_\-]{2,}/g, '_') .replace(/^[_\-]+|[_\-]+$/g, ''); // ← would strip a leading _ too For `Ägypten`: alphanumeric-strip → `gypten` (Ä gone, no underscore left behind because the regex used '' as the replacement, not '_'). The result is stored in `photos.filename` and that's what downloads serve. By that point `buildContentDisposition` is doing the right thing (emits both `filename="..."` ASCII fallback AND RFC 5987 `filename*=UTF-8''…` — Chrome correctly picks the UTF-8 form), but the string it's encoding has already lost the umlaut at the DB layer. Cure: NFD-normalize + strip combining marks BEFORE the alphanumeric strip. Same pipeline `utils/slug.js` (#525) already uses for URL slugs: sanitized = sanitized .normalize('NFD') .replace(/[̀-ͯ]/g, ''); Now `Ägypten` → NFD-decomposed `A` + combining diaeresis → strip combining mark → `Agypten` survives the alphanumeric pass. Filename and URL slug stay in sync (the URL was already `Agypten`, per patchingfailed's report — the filename now matches). Test surface: new `filenameSanitizer.test.js` pins: - the headline #607 contract for German / Portuguese / French / Spanish accented inputs (with a counter-example using the pre-fix pipeline so a future edit can't quietly regress it) - ASCII-input parity — pre-#607 byte-identical output for every pre-existing ASCII case - `generatePhotoFilename` composed round-trip - `sanitizeForContentDisposition` + `buildContentDisposition` RFC 6266 dual-form output (since the helper sits next to this function and is the next thing to break if a refactor goes sideways) - `sanitizeForZipEntry` path-traversal blocking 31 cases total, all pass. Bundled into PR #609 since it's a small targeted fix and that PR is already an admin-UI polish branch with low review weight. --- .../utils/__tests__/filenameSanitizer.test.js | 175 ++++++++++++++++++ backend/src/utils/filenameSanitizer.js | 16 +- 2 files changed, 188 insertions(+), 3 deletions(-) create mode 100644 backend/src/utils/__tests__/filenameSanitizer.test.js diff --git a/backend/src/utils/__tests__/filenameSanitizer.test.js b/backend/src/utils/__tests__/filenameSanitizer.test.js new file mode 100644 index 00000000..7d91d464 --- /dev/null +++ b/backend/src/utils/__tests__/filenameSanitizer.test.js @@ -0,0 +1,175 @@ +/** + * Tests for filenameSanitizer. + * + * The headline contract is the #607 fix: accented characters in event + * names must transliterate to their ASCII base (Ägypten → Agypten) rather + * than being dropped outright (Ägypten → gypten). This keeps the photo + * filename consistent with the URL slug, which already does the right + * thing via `utils/slug.js`. + * + * Also pins the broader header-safety contracts for buildContentDisposition + * and the zip-entry sanitizer so a future refactor can't silently regress + * them. + */ + +const { + sanitizeFilename, + generatePhotoFilename, + sanitizeForContentDisposition, + buildContentDisposition, + sanitizeForZipEntry, +} = require('../filenameSanitizer'); + +describe('sanitizeFilename — accented characters transliterate via NFD (#607)', () => { + // Replays the broken legacy behaviour where the alphanumeric strip ran + // BEFORE any NFD normalization, dropping the whole grapheme rather than + // preserving the base letter. Pinned as a counter-example so a future + // edit that removes the NFD pass fails this test set loudly. + const legacyBroken = (s) => + String(s).trim() + .replace(/\s+/g, '_') + .replace(/[^a-zA-Z0-9_\-\.]/g, '') + .replace(/[_\-]{2,}/g, '_') + .replace(/^[_\-]+|[_\-]+$/g, ''); + + it.each([ + ['Ägypten', 'Agypten'], + ['Über uns', 'Uber_uns'], + ['Niño', 'Nino'], + ['Decoração', 'Decoracao'], + ['Crème Brûlée', 'Creme_Brulee'], + ['Fête de famille', 'Fete_de_famille'], + ['Família', 'Familia'], + ])('transliterates %j → %j (was %j before #607)', (input, expected) => { + expect(sanitizeFilename(input)).toBe(expected); + // Confirm the legacy pipeline would have dropped the leading character — + // if these ever match the new output, the broken pipeline has the same + // result and the test loses its bite (sanity guard). + expect(legacyBroken(input)).not.toBe(expected); + }); +}); + +describe('sanitizeFilename — ASCII inputs unchanged', () => { + // For pure-ASCII input the output must be byte-identical to the + // pre-#607 pipeline, so existing photo filenames in DBs around the + // world keep round-tripping through whatever lookups they participate + // in. + it.each([ + ['Wedding 2026', 'Wedding_2026'], + ['birthday-party-42', 'birthday-party-42'], + ['event_with_underscores', 'event_with_underscores'], + ['CamelCase Event Name', 'CamelCase_Event_Name'], + ['event.with.dots', 'event.with.dots'], + ['event!@#$%^&*()chars', 'eventchars'], + ])('preserves %j → %j', (input, expected) => { + expect(sanitizeFilename(input)).toBe(expected); + }); +}); + +describe('sanitizeFilename — edge cases', () => { + it('returns "unnamed" for falsy input', () => { + expect(sanitizeFilename(null)).toBe('unnamed'); + expect(sanitizeFilename(undefined)).toBe('unnamed'); + expect(sanitizeFilename('')).toBe('unnamed'); + }); + + it('returns "unnamed" when sanitization leaves the string empty', () => { + expect(sanitizeFilename('婚礼')).toBe('unnamed'); // pure CJK, no NFD decomposition to ASCII + expect(sanitizeFilename('!!!')).toBe('unnamed'); + }); + + it('respects the maxLength bound', () => { + expect(sanitizeFilename('a'.repeat(60), 10)).toBe('a'.repeat(10)); + }); + + it('strips leading/trailing underscores and hyphens', () => { + expect(sanitizeFilename('---hello---')).toBe('hello'); + expect(sanitizeFilename('___world___')).toBe('world'); + }); +}); + +describe('generatePhotoFilename — composed name uses the NFD pipeline', () => { + it('round-trips Ägypten + individual → Agypten_individual_0050.jpg (#607)', () => { + expect(generatePhotoFilename('Ägypten', 'individual', 50, '.jpg')) + .toBe('Agypten_individual_0050.jpg'); + }); + + it('handles missing category by defaulting to "uncategorized"', () => { + expect(generatePhotoFilename('Wedding', null, 1, '.jpg')) + .toBe('Wedding_uncategorized_0001.jpg'); + }); + + it('zero-pads the counter to 4 digits', () => { + expect(generatePhotoFilename('e', 'c', 7, '.png')).toBe('e_c_0007.png'); + expect(generatePhotoFilename('e', 'c', 1234, '.png')).toBe('e_c_1234.png'); + // 5+ digit counters intentionally overflow the pad — pinned because + // the unique index in the photos table doesn't care about pad width, + // only string uniqueness. + expect(generatePhotoFilename('e', 'c', 99999, '.png')).toBe('e_c_99999.png'); + }); +}); + +describe('sanitizeForContentDisposition — header-safe ASCII fallback', () => { + it('strips header-breaking control bytes', () => { + expect(sanitizeForContentDisposition('hello\rworld')).toBe('helloworld'); + expect(sanitizeForContentDisposition('hello\nworld')).toBe('helloworld'); + expect(sanitizeForContentDisposition('hello\x00world')).toBe('helloworld'); + }); + + it('replaces path separators and quote chars that would close the quoted-string', () => { + expect(sanitizeForContentDisposition('a/b\\c"d')).toBe('a_b_c_d'); + }); + + it('falls back to "download" on falsy input', () => { + expect(sanitizeForContentDisposition(null)).toBe('download'); + expect(sanitizeForContentDisposition('')).toBe('download'); + }); + + // The companion buildContentDisposition emits filename*=UTF-8'' alongside + // this ASCII fallback, so unicode bytes don't reach the wire here — + // they're carried by the RFC 5987 form on the wire instead. + it('replaces non-ASCII bytes with _ (paired with filename*= in buildContentDisposition)', () => { + expect(sanitizeForContentDisposition('Ägypten.jpg')).toBe('gypten.jpg'); + }); +}); + +describe('buildContentDisposition — RFC 6266 / RFC 5987 dual form', () => { + it('emits both filename="..." (ASCII) and filename*=UTF-8\'\'... (unicode) for accented names', () => { + const header = buildContentDisposition('Ägypten.jpg'); + expect(header).toContain("filename=\"gypten.jpg\""); + expect(header).toContain("filename*=UTF-8''%C3%84gypten.jpg"); + expect(header.startsWith('attachment;')).toBe(true); + }); + + it('honours the disposition argument when provided', () => { + expect(buildContentDisposition('hello.pdf', 'inline')).toMatch(/^inline; /); + }); + + it('falls back to "download" when name is missing', () => { + expect(buildContentDisposition('')).toContain('filename="download"'); + }); +}); + +describe('sanitizeForZipEntry — preserves unicode, blocks path traversal', () => { + it('preserves spaces, parentheses, and unicode (modern zip readers handle UTF-8)', () => { + expect(sanitizeForZipEntry('Ägypten (1).jpg')).toBe('Ägypten (1).jpg'); + }); + + it('normalises path separators to underscore so ../passwd becomes literal', () => { + // The leading `../` becomes `.._` after separator-normalize. The leading + // dots are then stripped (`^\.+/`), leaving `_etc_passwd`. The exact + // surface form is less important than the guarantee: no `/` survives, + // so it can never participate in a directory traversal when extracted. + expect(sanitizeForZipEntry('../etc/passwd')).toBe('_etc_passwd'); + expect(sanitizeForZipEntry('a\\b\\c')).toBe('a_b_c'); + }); + + it('strips leading dots so .. can never be an upward reference', () => { + expect(sanitizeForZipEntry('..secret')).toBe('secret'); + }); + + it('falls back to "download" on empty input', () => { + expect(sanitizeForZipEntry('')).toBe('download'); + expect(sanitizeForZipEntry(null)).toBe('download'); + }); +}); diff --git a/backend/src/utils/filenameSanitizer.js b/backend/src/utils/filenameSanitizer.js index 3074e40e..26adb30b 100644 --- a/backend/src/utils/filenameSanitizer.js +++ b/backend/src/utils/filenameSanitizer.js @@ -8,13 +8,23 @@ const path = require('path'); */ function sanitizeFilename(str, maxLength = 50) { if (!str) return 'unnamed'; - + // Convert to string and trim let sanitized = String(str).trim(); - + + // NFD-normalize so accented characters split into a base letter + combining + // mark, then strip the combining-mark range (U+0300–U+036F) so the ASCII + // base survives the next regex pass. Without this, the `[^a-zA-Z0-9_\-\.]` + // strip below drops the WHOLE grapheme (`Ä` → '', `Decoração` → `Decorao`). + // The URL slug pipeline in utils/slug.js already does the right thing; + // matching here keeps the photo filename and the event URL slug consistent + // (#607 — patchingfailed reported `Ägypten` → `gypten` on download, while + // the URL slug correctly showed `Agypten`). + sanitized = sanitized.normalize('NFD').replace(/[̀-ͯ]/g, ''); + // Replace spaces with underscores sanitized = sanitized.replace(/\s+/g, '_'); - + // Remove special characters except hyphens, underscores, and dots sanitized = sanitized.replace(/[^a-zA-Z0-9_\-\.]/g, '');