fix(security): close GHSA-g94x (cross-gallery photo read) + GHSA-pv6w (admin DB export) (#924)

* fix(security): close two access-control advisories (GHSA-g94x, GHSA-pv6w) GHSA-g94x-8vv8-3c9f (HIGH) — the secure-image VIEW route (/secure-images/:slug/secure/:photoId/:token) validated only the token signature and took the gallery/photo from the URL, so a token minted on any PUBLIC gallery read every other gallery's photos with no password (its download sibling has verifyGalleryAccess; the view route can't — it serves via <img src> with no header). Bind the token to its scope instead: the URL photoId must equal the token's minted photoId (photos belong to exactly one gallery, and minting is gallery-scoped), and the gallery embedded in the token's sessionId must equal the URL gallery. GHSA-pv6w-rj34-wj9v (MEDIUM) — GET /admin/backup/picpeak/export dumps every table unredacted (bcrypt hashes, 2FA, SMTP/SSO/WhatsApp/webhook/S3 secrets) and was gated only by backup.create, which the built-in admin role holds. Gate it behind super_admin, matching the restore side (backup.restore, already admin-denied) and the masked config APIs. Regression tests pin both: cross-gallery token reads 403 (photo and gallery checks), backup export 403 for admin / passes for super_admin. * test: stub requireSuperAdmin in the backup masking mock adminBackup now calls requireSuperAdmin() at load (GHSA-pv6w export gate), and backupSecretMasking mocks the permissions module — add the new function to the mock so the module loads. * fix(security): review follow-ups on the export gate (GHSA-pv6w) - test: place the mocked export in its own mkdtemp dir. The route recursively deletes path.dirname(filePath) after download, so a stub in bare os.tmpdir() made the super_admin test wipe the whole temp root — other jest workers' DB files included (latent CI flake). - ui: hide PicpeakExportCard from non-super_admins. The role keeps settings.view + backup.create, so after the gate its Download button always 403'd with a generic toast; gate the card on role super_admin to match the endpoint. * fix(security): keep the token-mismatch audit values within varchar(20) (GHSA-g94x review) image_access_logs.access_type is varchar(20) (migration 038), but 'token_gallery_mismatch' is 22 chars — on Postgres the audit write threw value-too-long and logImageAccess swallowed it, so the security event went unrecorded (the 403 still fired; log is best-effort). Shorten to 'photo_mismatch' / 'gallery_mismatch' (14/16). --------- Co-authored-by: Paul Nothaft <[email protected]>
2026-07-30 14:24:28 +02:00
parent 342dde3589
commit 03087c798c
6 changed files with 274 additions and 3 deletions
@@ -0,0 +1,136 @@
+/**
+ * Secure-image view route token binding (GHSA-g94x-8vv8-3c9f).
+ *
+ * The view route GET /api/secure-images/:slug/secure/:photoId/:token serves
+ * via <img src> with the token in the URL, so it can't carry a gallery-token
+ * header like the download sibling. Before the fix it validated only the
+ * token signature and took the gallery/photo from the URL — so a token minted
+ * on any PUBLIC gallery read every other gallery's photos with no password.
+ *
+ * Pins that the route now enforces the scope inside the token:
+ *  - the URL photoId must equal the token's minted photoId
+ *  - the gallery embedded in the token's sessionId must equal the URL gallery
+ * A token minted on gallery A cannot read gallery B under either check; a
+ * token used on its own gallery+photo passes the binding.
+ */
+
+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-secimg-')), 'db.sqlite',
+);
+process.env.JWT_SECRET = process.env.JWT_SECRET || 'secimg-test-secret';
+process.env.STORAGE_PATH = fs.mkdtempSync(path.join(os.tmpdir(), 'picpeak-secimg-storage-'));
+
+// Stub the anti-bot/rate-limit middleware so the fingerprint is deterministic
+// — the token below is minted with the same fingerprint, so verifySecureToken
+// passes and the binding logic under test is what decides the outcome.
+jest.mock('../../src/middleware/secureImageMiddleware', () => ({
+  secureImageAccess: (req, _res, next) => {
+    req.clientInfo = { fingerprint: 'test-fp', ip: '127.0.0.1', userAgent: 'jest' };
+    next();
+  },
+  getSecurityStatus: (_req, res) => res.json({ ok: true }),
+}));
+
+const request = require('supertest');
+const express = require('express');
+
+const { bootCrmDb, seedMinimal } = require('../integration/helpers/crmDb');
+const secureImageService = require('../../src/services/secureImageService');
+
+describe('secure-image view route token binding (GHSA-g94x)', () => {
+  let db;
+  let cleanup;
+  let app;
+  let galleryA; let galleryB;
+  let photoA; let photoB;
+
+  const mkEvent = async (slug, requirePassword) => {
+    const r = await db('events').insert({
+      slug,
+      event_type: 'wedding',
+      event_name: slug,
+      event_date: '2026-08-01',
+      host_email: '[email protected]',
+      admin_email: '[email protected]',
+      password_hash: 'x',
+      require_password: requirePassword ? 1 : 0,
+      share_link: `/gallery/${slug}/share`,
+      share_token: `${slug}-share`,
+      expires_at: new Date(Date.now() + 7 * 24 * 3600 * 1000).toISOString(),
+      is_active: 1,
+      is_archived: 0,
+      is_draft: 0,
+      created_at: new Date().toISOString(),
+    }).returning('id');
+    return r[0]?.id ?? r[0];
+  };
+
+  const mkPhoto = async (eventId, slug, filename) => {
+    const dir = path.join(process.env.STORAGE_PATH, 'events/active', slug);
+    fs.mkdirSync(dir, { recursive: true });
+    fs.writeFileSync(path.join(dir, filename), Buffer.from('img'));
+    const r = await db('photos').insert({
+      event_id: eventId,
+      filename,
+      path: `${slug}/${filename}`,
+      type: 'individual',
+      uploaded_at: new Date().toISOString(),
+    }).returning('id');
+    return r[0]?.id ?? r[0];
+  };
+
+  // Mint a token exactly as the mint route does — bound to (photoId, gallery
+  // sessionId, fingerprint) — bypassing the anti-bot HTTP path.
+  const mint = (photoId, eventId) => secureImageService.generateSecureToken(
+    photoId,
+    `gallery_public_${eventId}_${Date.now()}`,
+    { clientFingerprint: 'test-fp', maxUses: 100, expiresIn: 3600 },
+  );
+
+  const view = (slug, photoId, token) => request(app)
+    .get(`/api/secure-images/${slug}/secure/${photoId}/${token}`);
+
+  beforeAll(async () => {
+    ({ db, cleanup } = await bootCrmDb());
+    await seedMinimal(db);
+    galleryA = await mkEvent('secimg-public-a', false);   // public — token source
+    galleryB = await mkEvent('secimg-private-b', true);   // password-protected — victim
+    photoA = await mkPhoto(galleryA, 'secimg-public-a', 'a.jpg');
+    photoB = await mkPhoto(galleryB, 'secimg-private-b', 'b.jpg');
+
+    app = express();
+    app.use(express.json());
+    app.use('/api/secure-images', require('../../src/routes/secureImages'));
+  }, 120000);
+
+  afterAll(async () => { if (cleanup) await cleanup(); });
+
+  it('rejects a gallery-A token used against gallery B (cross-photo)', async () => {
+    const token = mint(photoA, galleryA);
+    const res = await view('secimg-private-b', photoB, token);
+    expect(res.status).toBe(403);
+    expect(res.body.error).toMatch(/not valid for this photo/i);
+  });
+
+  it('rejects a gallery-A token replayed on gallery B with A\'s photoId', async () => {
+    const token = mint(photoA, galleryA);
+    // URL photoId matches the token, so the photo check passes — the gallery
+    // check (sessionId gallery A != URL gallery B) must catch it.
+    const res = await view('secimg-private-b', photoA, token);
+    expect(res.status).toBe(403);
+    expect(res.body.error).toMatch(/not valid for this gallery/i);
+  });
+
+  it('lets a token read its own gallery + photo (binding passes)', async () => {
+    const token = mint(photoA, galleryA);
+    const res = await view('secimg-public-a', photoA, token);
+    // Binding passes; serving may 200/404/500 depending on the pipeline, but
+    // it must NOT be rejected as a token mismatch.
+    expect(res.status).not.toBe(403);
+  });
+});