fix(security): unauth share_token leak (HIGH) + restore path-traversal, logo file-read (stable) (#947)

* fix(security): stop unauth share_token leak + block restore path-traversal, logo-path file read, branding path keys

* test: update resolveLogoFile for the c7x5 containment (reject outside-storage absolute paths, keep inside)

* fix(security): codex round-1 — escape LIKE wildcards in share-link resolve, keep in-storage absolute logos, guard restore verification

- shareLinkService: escape %/_ in the link_partial LIKE fallback so an
  anonymous /resolve/____… wildcard can't match an arbitrary share_link and
  leak its bearer token (reopened GHSA-rh8r). Explicit ESCAPE for SQLite.
- resolveLogoFile: re-add the raw absolute candidate but keep it subject to
  the storage-root containment filter (GHSA-c7x5) so legit in-storage
  absolute logos resolve while /etc/passwd stays rejected.
- restoreService: apply the same pathEscapes guard in post-restore
  verification so a skipped traversal entry isn't fs.access'd/hashed.

---------

Co-authored-by: Paul Nothaft <[email protected]>

backend/__tests__/routes/galleryResolveShareToken.test.js

@@ -0,0 +1,87 @@
+/**
+ * GHSA-rh8r-7x3h-36rv — the unauthenticated GET /api/gallery/resolve/:identifier
+ * must NOT return a gallery's secret share_token (nor the share links that
+ * embed it) for a bare *slug* lookup. Slugs appear in gallery URLs and are
+ * guessable; handing back the secret turns a known slug into share-link
+ * access to a no-password gallery. The token is only returned when the caller
+ * resolved via the token / full share link (i.e. already holds it).
+ */
+const path = require('path');
+const fs = require('fs');
+const os = require('os');
+
+process.env.NODE_ENV = 'test';
+process.env.TEST_DATABASE_PATH = path.join(
+  fs.mkdtempSync(path.join(os.tmpdir(), 'picpeak-resolve-')), 'db.sqlite',
+);
+process.env.JWT_SECRET = process.env.JWT_SECRET || 'resolve-test-secret';
+process.env.STORAGE_PATH = fs.mkdtempSync(path.join(os.tmpdir(), 'picpeak-resolve-storage-'));
+
+const request = require('supertest');
+const express = require('express');
+const cookieParser = require('cookie-parser');
+const { bootCrmDb, seedMinimal } = require('../integration/helpers/crmDb');
+
+const SLUG = 'resolve-test-event';
+const SHARE_TOKEN = 'a1b2c3d4e5f6a7b8c9d0e1f2a3b4c5d6';
+
+describe('GET /api/gallery/resolve/:identifier (GHSA-rh8r)', () => {
+  let db; let cleanup; let app;
+
+  beforeAll(async () => {
+    ({ db, cleanup } = await bootCrmDb());
+    await seedMinimal(db);
+    await db('events').insert({
+      slug: SLUG,
+      event_type: 'wedding',
+      event_name: 'Resolve Test',
+      event_date: '2026-08-01',
+      host_email: '[email protected]',
+      admin_email: '[email protected]',
+      password_hash: 'x',
+      share_link: `/gallery/${SLUG}/${SHARE_TOKEN}`,
+      share_token: SHARE_TOKEN,
+      require_password: 0, // no-password → the token IS the access credential
+      expires_at: new Date(Date.now() + 7 * 864e5).toISOString(),
+      is_active: 1, is_archived: 0, is_draft: 0,
+      created_at: new Date().toISOString(),
+    });
+
+    app = express();
+    app.use(express.json());
+    app.use(cookieParser());
+    app.use('/api/gallery', require('../../src/routes/gallery'));
+  }, 120000);
+
+  afterAll(async () => { if (cleanup) await cleanup(); });
+
+  it('does NOT leak the share_token (or share links) for a bare slug lookup', async () => {
+    const res = await request(app).get(`/api/gallery/resolve/${SLUG}`);
+    expect(res.status).toBe(200);
+    expect(res.body.slug).toBe(SLUG);
+    expect(res.body.matchType).toBe('slug');
+    // The secret must be absent — and must not sneak out via the share links.
+    expect(res.body.token).toBeUndefined();
+    expect(res.body.share_link).toBeUndefined();
+    expect(res.body.share_url).toBeUndefined();
+    expect(JSON.stringify(res.body)).not.toContain(SHARE_TOKEN);
+  });
+
+  it('DOES return the token when the caller already resolved via the token', async () => {
+    const res = await request(app).get(`/api/gallery/resolve/${SHARE_TOKEN}`);
+    expect(res.status).toBe(200);
+    expect(res.body.token).toBe(SHARE_TOKEN);
+    expect(res.body.matchType).toMatch(/token/);
+  });
+
+  it('does NOT leak the token via SQL LIKE wildcards in the link_partial fallback', async () => {
+    // Before the escaping fix, an anonymous request of 32 underscores matched
+    // any share_link ending in a 32-char token (`_` = single-char wildcard),
+    // resolved as matchType 'link_partial', and handed back the bearer token.
+    // The share_token here has no underscores, so an escaped LIKE must miss.
+    const res = await request(app).get(`/api/gallery/resolve/${'_'.repeat(SHARE_TOKEN.length)}`);
+    expect(res.status).toBe(404);
+    expect(res.body.token).toBeUndefined();
+    expect(JSON.stringify(res.body)).not.toContain(SHARE_TOKEN);
+  });
+});

backend/__tests__/utils/resolveLogoFile.test.js

@@ -99,10 +99,22 @@ describe('resolveLogoFile', () => {
     }
   });
 
-  it('treats absolute paths as-is when they exist', async () => {
+  it('rejects an absolute path OUTSIDE the storage roots (GHSA-c7x5)', async () => {
+    // The raw-absolute candidate was an arbitrary-file-read primitive
+    // (logo_path: '/etc/passwd' → rasterised into a PDF). Absolute paths
+    // outside the storage roots are now dropped even if they exist.
     existsSpy.mockImplementation((p) => p === '/abs/path/logo.png');
     getAppSetting.mockResolvedValue(null);
     const out = await resolveLogoFile({ logo_path: '/abs/path/logo.png' });
-    expect(out).toBe('/abs/path/logo.png');
+    expect(out).toBeNull();
+  });
+
+  it('still accepts an absolute path INSIDE the storage root', async () => {
+    // The legitimate case: multer stores the uploaded logo under
+    // storage/uploads/logos with an absolute path — that stays resolvable.
+    existsSpy.mockImplementation((p) => p === '/app/storage/uploads/logos/logo.png');
+    getAppSetting.mockResolvedValue(null);
+    const out = await resolveLogoFile({ logo_path: '/app/storage/uploads/logos/logo.png' });
+    expect(out).toBe('/app/storage/uploads/logos/logo.png');
   });
 });

backend/src/routes/gallery.js

@@ -84,9 +84,20 @@ router.get('/resolve/:identifier', handleAsync(async (req, res) => {
   }
 
   const { event, matchType, shareToken } = result;
-  const linkVariants = await buildShareLinkVariants({ slug: event.slug, shareToken });
   const requiresPassword = !(event.require_password === false || event.require_password === 0 || event.require_password === '0');
 
+  // The share_token is a bearer secret. Only return it (and the share
+  // links/URLs that embed it) when the caller already proved they hold it —
+  // i.e. they resolved via the token or the full share link. A bare *slug*
+  // lookup (slugs appear in gallery URLs and are guessable) must NOT hand
+  // back the secret, or an anonymous caller could turn a known slug into
+  // share-link access to a no-password gallery (GHSA-rh8r).
+  const callerHasToken = matchType !== 'slug';
+  if (!callerHasToken) {
+    return res.json({ slug: event.slug, matchType, requires_password: requiresPassword });
+  }
+
+  const linkVariants = await buildShareLinkVariants({ slug: event.slug, shareToken });
   res.json({
     slug: event.slug,
     token: shareToken,

backend/src/services/restoreService.js

@@ -12,6 +12,15 @@ const backupManifest = require('./backupManifest');
 const S3StorageAdapter = require('./storage/s3Storage');
 const { queueEmail } = require('./emailProcessor');
 const { formatBoolean } = require('../utils/dbCompat');
+
+// A manifest is attacker-influenceable (hand-crafted backup). Reject any
+// entry path that would resolve OUTSIDE its intended base directory
+// (traversal / absolute path) before any fs write. The target may not exist
+// yet, so resolve rather than realpath (GHSA-fm58).
+function pathEscapes(baseDir, candidate) {
+  const rel = path.relative(path.resolve(baseDir), path.resolve(candidate));
+  return !rel || rel === '..' || rel.startsWith('..' + path.sep) || path.isAbsolute(rel);
+}
 const { formatBytes } = require('../utils/formatBytes');
 const os = require('os');
 
@@ -771,7 +780,14 @@ class RestoreService {
         for (const file of filesToDownload) {
           const s3Key = path.posix.join(prefix, file.path);
           const localFilePath = path.join(localPath, file.path);
-          
+
+          // Containment guard (GHSA-fm58): reject a manifest path that would
+          // write outside the download staging dir.
+          if (pathEscapes(localPath, localFilePath)) {
+            this.log('error', `Refusing unsafe manifest path on download: ${file.path}`);
+            continue;
+          }
+
           await fs.mkdir(path.dirname(localFilePath), { recursive: true });
           
           try {
@@ -1204,6 +1220,14 @@ END $$;`
         const sourcePath = path.join(backupPath, file.path);
         const targetPath = path.join(storagePath, file.path);
 
+        // Containment guard (GHSA-fm58): a crafted manifest path like
+        // `../../etc/cron.d/x` would otherwise escape the storage root and
+        // overwrite arbitrary files. Skip any entry that escapes.
+        if (pathEscapes(backupPath, sourcePath) || pathEscapes(storagePath, targetPath)) {
+          errors.push(`Refusing unsafe manifest path: ${file.path}`);
+          continue;
+        }
+
         // Check if source file exists
         try {
           await fs.access(sourcePath);
@@ -1375,6 +1399,15 @@ END $$;`
 
         for (const file of filesToVerify) {
           const filePath = path.join(storagePath, file.path);
+          // Same containment guard as performFilesRestore: a traversal
+          // manifest entry (e.g. `../../etc/passwd`) was skipped during the
+          // restore, so it must not be fs.access'd/hashed here either —
+          // otherwise an existing outside file makes the skipped entry look
+          // "verified" (and we'd read an arbitrary file off disk).
+          if (pathEscapes(storagePath, filePath)) {
+            verification.errors.push(`Refusing unsafe manifest path on verification: ${file.path}`);
+            continue;
+          }
           try {
             await fs.access(filePath);
             

backend/src/services/shareLinkService.js

@@ -159,7 +159,16 @@ const resolveShareIdentifier = async (identifier) => {
     return { event, matchType: 'link', shareToken: getEventShareToken(event) };
   }
 
-  event = await baseQuery.clone().where('share_link', 'like', `%/${trimmed}`).first();
+  // GHSA-rh8r hardening: `trimmed` is attacker-controlled, so escape LIKE
+  // wildcards (`%`, `_`, and the escape char itself) before embedding it.
+  // Otherwise an anonymous `/resolve/________…________` (32 underscores)
+  // matches ANY share_link via single-char wildcards, resolves as
+  // matchType 'link_partial', and the /resolve route hands back the
+  // gallery's bearer token — reopening the very hole the token-withholding
+  // fix closed. Explicit ESCAPE clause because SQLite has no default LIKE
+  // escape character (Postgres defaults to backslash, but we set it for both).
+  const likeTail = `%/${trimmed.replace(/[\\%_]/g, (c) => `\\${c}`)}`;
+  event = await baseQuery.clone().whereRaw('share_link LIKE ? ESCAPE \'\\\'', [likeTail]).first();
   if (event) {
     return { event, matchType: 'link_partial', shareToken: getEventShareToken(event) };
   }

backend/src/utils/resolveLogoFile.js

@@ -57,18 +57,34 @@ function generateCandidates(raw, storageRoot) {
   if (!value) return [];
   const stripped = value.replace(/^\/+/, '');
   const baseName = path.basename(value);
+  const cwdStorage = path.join(process.cwd(), 'storage');
   // Build candidate set; dedup at the end so we don't stat the same
   // file twice when the inputs overlap.
   const candidates = [
-    path.isAbsolute(value) ? value : null,
+    // Keep the raw absolute value as a candidate so a legitimate multer path
+    // (branding_logo_path is stored absolute) or an absolute logo inside a
+    // non-standard storage subdir still resolves. The containment filter
+    // below is what enforces safety — it drops this candidate when it points
+    // outside the storage roots, so `/etc/passwd` is still rejected.
+    ...(path.isAbsolute(value) ? [value] : []),
     path.join(storageRoot, stripped),
     path.join(storageRoot, 'uploads', 'logos', baseName),
     path.join(storageRoot, 'branding', baseName),
-    path.join(process.cwd(), 'storage', stripped),
+    path.join(cwdStorage, stripped),
-    path.join(process.cwd(), 'storage', 'uploads', 'logos', baseName),
+    path.join(cwdStorage, 'uploads', 'logos', baseName),
-    path.join(process.cwd(), 'storage', 'branding', baseName),
+    path.join(cwdStorage, 'branding', baseName),
-  ].filter(Boolean);
+  ];
-  return [...new Set(candidates)];
+  // GHSA-c7x5: only read logo files INSIDE the storage roots. An admin-set
+  // logo_path of `/etc/passwd` was previously rasterised into a PDF; the
+  // filter below drops any candidate (including the raw absolute one and any
+  // `..`-escaping stripped path) that resolves outside the roots. baseName-
+  // based candidates are inherently contained.
+  const roots = [path.resolve(storageRoot), path.resolve(cwdStorage)];
+  const contained = candidates.filter((c) => {
+    const r = path.resolve(c);
+    return roots.some((root) => r === root || r.startsWith(root + path.sep));
+  });
+  return [...new Set(contained)];
 }
 
 function pickExisting(candidates) {