From 9050affd8dd0d5dff0514a8a7cb677fc2d410fca Mon Sep 17 00:00:00 2001 From: Paul Nothaft <53005142+the-luap@users.noreply.github.com> Date: Sun, 2 Aug 2026 08:37:48 +0200 Subject: [PATCH] fix(security): unauth share_token leak (HIGH) + restore path-traversal, logo file-read, branding path keys (#946) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 --- .../routes/galleryResolveShareToken.test.js | 87 +++++++++++++++++++ .../__tests__/utils/resolveLogoFile.test.js | 16 +++- backend/src/routes/adminSettings.js | 9 +- backend/src/routes/gallery.js | 13 ++- backend/src/services/restoreService.js | 35 +++++++- backend/src/services/shareLinkService.js | 11 ++- backend/src/utils/resolveLogoFile.js | 28 ++++-- 7 files changed, 187 insertions(+), 12 deletions(-) create mode 100644 backend/__tests__/routes/galleryResolveShareToken.test.js diff --git a/backend/__tests__/routes/galleryResolveShareToken.test.js b/backend/__tests__/routes/galleryResolveShareToken.test.js new file mode 100644 index 00000000..3af7d170 --- /dev/null +++ b/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: 'h@example.com', + admin_email: 'a@example.com', + 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); + }); +}); diff --git a/backend/__tests__/utils/resolveLogoFile.test.js b/backend/__tests__/utils/resolveLogoFile.test.js index 0d8d2c58..00a16b31 100644 --- a/backend/__tests__/utils/resolveLogoFile.test.js +++ b/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'); }); }); diff --git a/backend/src/routes/adminSettings.js b/backend/src/routes/adminSettings.js index e467d938..3b31bd8d 100644 --- a/backend/src/routes/adminSettings.js +++ b/backend/src/routes/adminSettings.js @@ -39,7 +39,14 @@ const getStoragePath = () => process.env.STORAGE_PATH || path.join(__dirname, '. // oidc_client_secret is reserved too: it is AES-encrypted at rest and only // writable through PUT /sso below — a generic upsert would store plaintext // and break decryption (#798). -const RESERVED_SETTING_KEYS = ['setup_wizard_completed', 'setup_token']; +// Branding *path* keys (GHSA-665x) are server-computed by the logo-upload +// flow and feed a filesystem logo resolver; letting the general settings PUT +// set them to arbitrary strings makes them an input to path resolution. +// Reserve them here — the dedicated upload endpoints still write them. +const RESERVED_SETTING_KEYS = [ + 'setup_wizard_completed', 'setup_token', + 'branding_logo_path', 'branding_logo_path_dark', 'branding_watermark_logo_path', +]; // EVERY oidc_* key is reserved (#798 phase 2): the client secret would be // clobbered with plaintext, and the policy/mapping keys carry invariants // (role targets exist, break-glass account present) that only the dedicated diff --git a/backend/src/routes/gallery.js b/backend/src/routes/gallery.js index 7263bda8..b86cd297 100644 --- a/backend/src/routes/gallery.js +++ b/backend/src/routes/gallery.js @@ -153,9 +153,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, diff --git a/backend/src/services/restoreService.js b/backend/src/services/restoreService.js index 97f4b5d7..f85035c6 100644 --- a/backend/src/services/restoreService.js +++ b/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); diff --git a/backend/src/services/shareLinkService.js b/backend/src/services/shareLinkService.js index c3efaf75..d5ffe602 100644 --- a/backend/src/services/shareLinkService.js +++ b/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) }; } diff --git a/backend/src/utils/resolveLogoFile.js b/backend/src/utils/resolveLogoFile.js index 1c2114e7..6a8aea4c 100644 --- a/backend/src/utils/resolveLogoFile.js +++ b/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(process.cwd(), 'storage', 'uploads', 'logos', baseName), - path.join(process.cwd(), 'storage', 'branding', baseName), - ].filter(Boolean); - return [...new Set(candidates)]; + path.join(cwdStorage, stripped), + path.join(cwdStorage, 'uploads', 'logos', baseName), + path.join(cwdStorage, 'branding', baseName), + ]; + // 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) {