fix(security): escape brand tokens, block tracker redirects, trim logo diagnostic (stable) (#967)
* 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
(cherry picked from commit 093480a753ff3d4b6ed48dd9f1108f975c8e0d47)
* 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
(cherry picked from commit c6b95d3cd1cb28e5c2828d29d4d63fadad981dcf)
---------
Co-authored-by: Paul Nothaft <[email protected]>
This commit is contained in:
Paul Nothaft
co-authored by
Paul Nothaft
Paul Nothaft
parent
4e99897313
commit
7f27e6771f
@@ -249,33 +249,84 @@ router.get(
|
||||
const brandingLogoUrl = await getAppSetting('branding_logo_url');
|
||||
const resolved = await resolveLogoFile(profile);
|
||||
|
||||
// GHSA-29vm: report candidates RELATIVE to the storage roots rather than
|
||||
// echoing absolute container paths and process.cwd(). This endpoint exists
|
||||
// to answer "which candidate did/didn't exist", which relative paths answer
|
||||
// just as well without handing out the filesystem layout.
|
||||
const cwdStorage = path.join(process.cwd(), 'storage');
|
||||
const relativise = (p) => {
|
||||
for (const [name, root] of [['STORAGE', storageRoot], ['CWD_STORAGE', cwdStorage]]) {
|
||||
const rel = path.relative(root, p);
|
||||
if (rel && !rel.startsWith('..') && !path.isAbsolute(rel)) {
|
||||
return `<${name}>/${rel.split(path.sep).join('/')}`;
|
||||
}
|
||||
}
|
||||
return path.basename(p);
|
||||
};
|
||||
|
||||
const inspect = (label, raw) => {
|
||||
const value = (raw || '').toString().trim();
|
||||
if (!value) return { label, value: null, candidates: [] };
|
||||
const stripped = value.replace(/^\/+/, '');
|
||||
const baseName = path.basename(value);
|
||||
// Mirrors resolveLogoFile's candidate list EXACTLY. It keeps the raw
|
||||
// absolute value as a candidate (multer stores branding_logo_path
|
||||
// absolute) and lets the storage-root containment filter reject it when
|
||||
// it points outside — so the diagnostic must include it too, or a
|
||||
// legitimately-contained absolute logo shows every candidate as missing
|
||||
// while resolvedTo names the file.
|
||||
// The stripped joins (`<ROOT>/<value-minus-leading-slash>`) are gated on
|
||||
// containment, NOT on path.isAbsolute(). isAbsolute() cannot tell a
|
||||
// multer disk path from a root-relative URL like `/custom/logo.png`, and
|
||||
// for the URL form `<STORAGE>/custom/logo.png` is a file the resolver
|
||||
// genuinely returns — skipping it made this endpoint report "no source
|
||||
// candidate exists" about a logo that renders fine.
|
||||
//
|
||||
// The gate is instead: does the raw value ALREADY resolve inside a
|
||||
// storage root? If so it is a real disk path, the raw candidate below
|
||||
// covers it, and the stripped join would only produce a double-prefixed
|
||||
// path that can never exist while re-embedding the absolute path
|
||||
// GHSA-29vm exists to stop echoing (redact() strips only the leading
|
||||
// root, so the inner one would survive).
|
||||
const valueInsideRoot = path.isAbsolute(value) && [
|
||||
path.resolve(storageRoot), path.resolve(cwdStorage),
|
||||
].some((root) => {
|
||||
const r = path.resolve(value);
|
||||
return r === root || r.startsWith(root + path.sep);
|
||||
});
|
||||
const strippedJoins = valueInsideRoot
|
||||
? []
|
||||
: [path.join(storageRoot, stripped), path.join(cwdStorage, stripped)];
|
||||
const candidates = [
|
||||
path.isAbsolute(value) ? value : null,
|
||||
path.join(storageRoot, stripped),
|
||||
...(path.isAbsolute(value) ? [value] : []),
|
||||
...strippedJoins,
|
||||
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);
|
||||
path.join(cwdStorage, 'uploads', 'logos', baseName),
|
||||
path.join(cwdStorage, 'branding', baseName),
|
||||
];
|
||||
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 {
|
||||
label, value,
|
||||
candidates: [...new Set(candidates)].map((p) => ({
|
||||
path: p,
|
||||
label,
|
||||
// GHSA-29vm: branding_logo_path is stored absolute by multer, so
|
||||
// echoing it back handed out the filesystem layout just as the
|
||||
// candidate paths did. Relativise it the same way.
|
||||
value: path.isAbsolute(value) ? relativise(value) : value,
|
||||
candidates: [...new Set(contained)].map((p) => ({
|
||||
path: relativise(p),
|
||||
exists: (() => { try { return fs.existsSync(p) && fs.statSync(p).isFile(); } catch { return false; } })(),
|
||||
})),
|
||||
};
|
||||
};
|
||||
|
||||
return successResponse(res, {
|
||||
storageRoot,
|
||||
cwd: process.cwd(),
|
||||
resolvedTo: resolved,
|
||||
// Absolute storageRoot / cwd deliberately omitted (GHSA-29vm); the
|
||||
// candidate paths below are shown relative to <STORAGE>/<CWD_STORAGE>.
|
||||
resolvedTo: resolved ? relativise(resolved) : null,
|
||||
sources: [
|
||||
inspect('business_profile.logo_path', profile?.logo_path),
|
||||
inspect('app_settings.branding_logo_path', brandingDiskPath),
|
||||
|
||||
Reference in New Issue
Block a user