From 164129b8f5bbf8a68d743930a72bdb95b88fdee3 Mon Sep 17 00:00:00 2001 From: Paul Nothaft <53005142+the-luap@users.noreply.github.com> Date: Sun, 2 Aug 2026 21:17:17 +0200 Subject: [PATCH] fix(security): escape brand tokens, block tracker redirects, trim logo diagnostic (GHSA-j347, mw76, 29vm) (#961) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(security): escape brand tokens, block tracker redirects, trim logo diagnostic (GHSA-j347, mw76, 29vm) GHSA-j347 — buildCachedPayload sanitizes the operator's HTML and THEN runs applyBrandTokens over the result with a plain String.replace, so any markup in a token value reached the public origin unfiltered. The default templates interpolate tokens into text AND into quoted attributes ({{company_name}} logo, href="mailto:{{support_email}}"), so a value could close the attribute and inject. Token values are now HTML-escaped on substitution, mirroring galleryOgService's escapeHtml. sanitizeBrandUrl's case-sensitive literal 'javascript:' check (which 'JavaScript:' walked straight past) is replaced by an http/https scheme allowlist; relative logo paths are unaffected. Writer is settings.edit (super_admin only) and the CSP blocks inline script, so this is defence-in-depth — but sanitize-then-substitute is a real ordering bug regardless. GHSA-mw76 — the SSRF decline STANDS: self-hosted operators legitimately point analytics at private addresses, so connection-time IP blocking would break real deployments. Fixed only the narrow leak: undici strips Authorization/Cookie/Proxy-Authorization/Host across a cross-origin redirect, but umamiAdapter sends a CUSTOM x-umami-api-key header, which would be replayed verbatim to the redirect target. Both adapters now use redirect: 'error'. GHSA-29vm — the logo diagnostic echoed absolute storage roots, process.cwd() and absolute candidate paths. It now reports candidates relative to /, which answers the same 'which candidate existed' question. It also still advertised the raw-absolute candidate that GHSA-c7x5 removed from resolveLogoFile, so it was misreporting what the resolver tries — aligned with the real candidate list. publicSiteService.test.js expectation updated: an '&' in a company name is now emitted as '&'. Renders identically; the raw payload string differs. * fix(security): codex round 2 — stop the remaining logo-path disclosure, mirror the resolver (GHSA-29vm) - sources[].value was still echoed verbatim. branding_logo_path is stored ABSOLUTE by multer, so relativising only resolvedTo and the candidate paths left the filesystem layout going out anyway. It is now relativised too. - Round 1 dropped the raw-absolute candidate on the grounds that GHSA-c7x5 removed it from resolveLogoFile — but the c7x5 follow-up RE-ADDED it (kept, subject to the containment filter, so a legitimate multer path still resolves). The diagnostic therefore reported every candidate as missing for a contained absolute logo while resolvedTo named the file. It now mirrors the resolver, containment filter included. One deliberate cosmetic divergence, commented in place: for an absolute value the resolver also tries path.join(root, value-minus-leading-slash), which can never exist and would re-embed the absolute path this endpoint must stop echoing. Omitted; every candidate that can actually match is still shown. * fix(security): codex round 3 — mirror the resolver for root-relative logo paths (GHSA-29vm) The logo diagnostic skipped the `/` candidates whenever path.isAbsolute(value) was true. That test cannot distinguish a multer disk path from a root-relative URL such as `/custom/logo.png`, and for the URL form resolveLogoFile.generateCandidates() does try `/custom/logo.png` and can resolve it — so the endpoint reported "no source candidate exists" about a logo that renders fine, and collapsed the configured value to its basename. The stripped joins are now built unconditionally, exactly as the resolver does. Disclosure stays closed: every candidate still passes the containment filter and redact() rewrites survivors to `/…`, never an absolute host path. Claude-Session: https://claude.ai/code/session_01F211U4dDbEj4zXiyKbi9me * fix(security): gate the logo stripped-joins on containment, not isAbsolute (GHSA-29vm) The previous commit dropped the isAbsolute() gate entirely and regressed logoDiagnostic's own disclosure assertion: for a genuine multer disk path, path.join(root, value-minus-leading-slash) yields `/tmp/…/storage/custom/logo.png`, and redact() only rewrites the LEADING root — so the inner absolute path went straight back into the payload. The right discriminator is not "is this absolute" (which cannot separate a disk path from a root-relative URL) but "does the value already resolve inside a storage root". If it does, it is a real disk path, the raw candidate already covers it, and the stripped join is the double-prefixed junk that can never exist. If it does not — the `/custom/logo.png` URL form — the stripped join is exactly what resolveLogoFile resolves, and is shown. Covered by a new case asserting both halves: the candidate appears for the URL form, and the payload still contains neither the storage root nor cwd. Claude-Session: https://claude.ai/code/session_01F211U4dDbEj4zXiyKbi9me --------- Co-authored-by: Paul Nothaft --- .../__tests__/routes/logoDiagnostic.test.js | 123 ++++++++++++++++++ .../services/publicSiteBrandTokens.test.js | 91 +++++++++++++ .../src/__tests__/publicSiteService.test.js | 6 +- backend/src/routes/adminBusinessProfile.js | 75 +++++++++-- backend/src/services/publicSiteService.js | 38 +++++- .../src/services/trackers/rybbitAdapter.js | 7 + backend/src/services/trackers/umamiAdapter.js | 7 + 7 files changed, 331 insertions(+), 16 deletions(-) create mode 100644 backend/__tests__/routes/logoDiagnostic.test.js create mode 100644 backend/__tests__/services/publicSiteBrandTokens.test.js diff --git a/backend/__tests__/routes/logoDiagnostic.test.js b/backend/__tests__/routes/logoDiagnostic.test.js new file mode 100644 index 00000000..12719f26 --- /dev/null +++ b/backend/__tests__/routes/logoDiagnostic.test.js @@ -0,0 +1,123 @@ +/** + * Logo diagnostic must not leak the filesystem layout, and must mirror what + * resolveLogoFile actually tries (GHSA-29vm, codex round 2). + * + * Round 1 relativised `resolvedTo` and the candidate paths but still echoed + * `sources[].value` verbatim — and branding_logo_path is stored ABSOLUTE by + * multer, so the layout went out anyway. It also dropped the raw-absolute + * candidate, which the resolver retains (subject to containment), making the + * diagnostic report every candidate as missing for a legitimately contained + * absolute logo while `resolvedTo` named the file. + */ + +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-logodiag-')), 'db.sqlite', +); +process.env.JWT_SECRET = process.env.JWT_SECRET || 'logodiag-test-secret'; + + +const request = require('supertest'); +const express = require('express'); +const bcrypt = require('bcrypt'); +const jwt = require('jsonwebtoken'); + +const { bootCrmDb, seedMinimal } = require('../integration/helpers/crmDb'); + +describe('logo diagnostic disclosure (GHSA-29vm)', () => { + let db; let cleanup; let app; let token; + // bootCrmDb() sets STORAGE_PATH itself, so resolve these AFTER it runs. + let STORAGE; let logoDir; let logoPath; + + beforeAll(async () => { + ({ db, cleanup } = await bootCrmDb()); + await seedMinimal(db); + + // A legitimately contained absolute logo in a NON-standard storage subdir. + STORAGE = process.env.STORAGE_PATH; + logoDir = path.join(STORAGE, 'custom'); + logoPath = path.join(logoDir, 'logo.png'); + fs.mkdirSync(logoDir, { recursive: true }); + fs.writeFileSync(logoPath, 'png'); + + const setting = { setting_key: 'branding_logo_path', setting_value: JSON.stringify(logoPath), setting_type: 'branding' }; + const existing = await db('app_settings').where({ setting_key: 'branding_logo_path' }).first(); + if (existing) await db('app_settings').where({ setting_key: 'branding_logo_path' }).update(setting); + else await db('app_settings').insert(setting); + + const role = await db('roles').where({ name: 'super_admin' }).first(); + const r = await db('admin_users').insert({ + username: 'diag-admin', email: 'diag@example.com', + password_hash: await bcrypt.hash('Passw0rd!', 4), + role_id: role.id, is_active: 1, + created_at: new Date(), updated_at: new Date(), + }).returning('id'); + const id = r[0]?.id ?? r[0]; + token = jwt.sign( + { id, username: 'diag-admin', type: 'admin', role: 'super_admin', loginTime: Date.now() }, + process.env.JWT_SECRET, { expiresIn: '1h', issuer: 'picpeak-auth' }, + ); + + app = express(); + app.use(express.json()); + app.use('/api/admin/business-profile', require('../../src/routes/adminBusinessProfile')); + }, 120000); + + afterAll(async () => { if (cleanup) await cleanup(); }); + + it('does not leak absolute paths, cwd or storage root anywhere in the payload', async () => { + const res = await request(app) + .get('/api/admin/business-profile/logo-diagnostic') + .set('Authorization', `Bearer ${token}`); + + expect(res.status).toBe(200); + const body = JSON.stringify(res.body); + expect(body).not.toContain(STORAGE); + expect(body).not.toContain(process.cwd()); + expect(res.body.storageRoot).toBeUndefined(); + expect(res.body.cwd).toBeUndefined(); + }); + + it('still finds a contained absolute logo outside the standard subdirs', async () => { + const res = await request(app) + .get('/api/admin/business-profile/logo-diagnostic') + .set('Authorization', `Bearer ${token}`); + + const source = res.body.sources.find((s) => s.label === 'app_settings.branding_logo_path'); + expect(source).toBeTruthy(); + // The resolver keeps the contained absolute candidate, so the diagnostic + // must show it existing rather than reporting everything missing. + expect(source.candidates.some((c) => c.exists)).toBe(true); + expect(res.body.resolvedTo).toMatch(/^\//); + }); + + it('shows the / candidate for a ROOT-RELATIVE logo URL (round 3)', async () => { + // `/custom/logo.png` is a URL, not a disk path, but path.isAbsolute() says + // true for both. Gating the stripped joins on isAbsolute() therefore hid + // `/custom/logo.png` — a candidate resolveLogoFile does try and + // can resolve — so the diagnostic claimed nothing existed for a logo that + // renders fine, and collapsed the configured value to its basename. + await db('app_settings').where({ setting_key: 'branding_logo_path' }) + .update({ setting_value: JSON.stringify('/custom/logo.png') }); + + const res = await request(app) + .get('/api/admin/business-profile/logo-diagnostic') + .set('Authorization', `Bearer ${token}`); + + expect(res.status).toBe(200); + const source = res.body.sources.find((s) => s.label === 'app_settings.branding_logo_path'); + expect(source.candidates.some((c) => c.path === '/custom/logo.png' && c.exists)).toBe(true); + + // …and the disclosure guarantee still holds for this shape. + const body = JSON.stringify(res.body); + expect(body).not.toContain(STORAGE); + expect(body).not.toContain(process.cwd()); + + await db('app_settings').where({ setting_key: 'branding_logo_path' }) + .update({ setting_value: JSON.stringify(logoPath) }); + }); +}); diff --git a/backend/__tests__/services/publicSiteBrandTokens.test.js b/backend/__tests__/services/publicSiteBrandTokens.test.js new file mode 100644 index 00000000..a9c4c8f9 --- /dev/null +++ b/backend/__tests__/services/publicSiteBrandTokens.test.js @@ -0,0 +1,91 @@ +/** + * Brand-token substitution must not reintroduce markup after sanitization + * (GHSA-j347). + * + * buildCachedPayload sanitizes the operator's HTML and THEN calls + * applyBrandTokens on the result, which did a plain `String.replace` with no + * escaping. The default templates interpolate tokens into text and into quoted + * attributes (`{{company_name}} logo`, + * `href="mailto:{{support_email}}"`), so a token value could close the + * attribute and inject markup into the public origin. + * + * The writer is settings.edit (super_admin only) and the CSP blocks inline + * script, so this is defence-in-depth rather than a live RCE — but the + * sanitize-then-substitute ordering is a real bug either way. + */ + +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-brandtok-')), 'db.sqlite', +); +process.env.JWT_SECRET = process.env.JWT_SECRET || 'brandtok-test-secret'; + +const { _internal } = require('../../src/services/publicSiteService'); + +// applyBrandTokens / sanitizeBrandUrl are module-private; the service exports +// them under _internal for testing (see publicSiteService module.exports). +const { applyBrandTokens, sanitizeBrandUrl } = _internal || {}; + +const maybe = applyBrandTokens ? describe : describe.skip; + +maybe('applyBrandTokens escaping (GHSA-j347)', () => { + it('escapes markup in a text-position token', () => { + const out = applyBrandTokens('

{{company_name}}

', { + companyName: '', + }); + expect(out).not.toContain('