Merge pull request #609 from the-luap/fix/admin-header-img-fallback-perm-skeleton

fix(admin): logo-img fallback + sidebar perm hydration + filename NFD transliteration (#523 follow-up 2, #607)

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');
+  });
+});

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, '');
   

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<AdminHeaderProps> = ({ 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 <img> 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 <sm the wordmark hides when the logo carries the brand identity
   // (logo_and_text). Same pattern LanguageSelector uses for its language
   // name (#527). Without this, even with truncate, a phone-width admin
@@ -64,7 +84,12 @@ export const AdminHeader: React.FC<AdminHeaderProps> = ({ 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 <sm too — otherwise the phone header
+  // shows nothing at all for the brand block.
+  const wordmarkVisibilityClass = logoEffectivelyVisible ? 'hidden sm:inline' : 'inline';
   const renderBrandBlock = () => {
     // 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<AdminHeaderProps> = ({ onMenuClick }) => {
     // action buttons on narrow mobile widths (#523 regression).
     return (
       <div className="flex items-center gap-2 min-w-0">
-        {showLogo && (
-          <img src={resolvedLogoUrl} alt={companyName} className="h-8 w-auto object-contain flex-shrink-0" />
+        {logoEffectivelyVisible && (
+          <img
+            src={logoImgSrc}
+            alt={companyName}
+            className="h-8 w-auto object-contain flex-shrink-0"
+            onError={() => {
+              // 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 && (
           <span className={`${wordmarkVisibilityClass} text-xl sm:text-2xl truncate`} style={{ fontFamily: 'Poppins, sans-serif', fontWeight: 600, color: '#145346' }}>{companyName}</span>

frontend/src/components/admin/AdminSidebar.tsx

@@ -101,7 +101,7 @@ const navigation: NavItem[] = [
 export const AdminSidebar: React.FC<AdminSidebarProps> = ({ 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<AdminSidebarProps> = ({ 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')) && (
           <div className={`flex-shrink-0 ${collapsed ? 'lg:hidden' : ''}`}>
             {/* Version Info */}
             <VersionInfo />