* 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
(<img src="{{brand_logo_url}}" alt="{{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
<STORAGE>/<CWD_STORAGE>, 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 `<STORAGE>/<value>` 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 `<STORAGE>/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 `<STORAGE>/…`, 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
`<STORAGE>/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 <[email protected]>
124 lines
5.3 KiB
JavaScript
124 lines
5.3 KiB
JavaScript
/**
|
|
* 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: '[email protected]',
|
|
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(/^<STORAGE>\//);
|
|
});
|
|
|
|
it('shows the <STORAGE>/<value> 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
|
|
// `<STORAGE>/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 === '<STORAGE>/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) });
|
|
});
|
|
});
|