fix(security): escape brand tokens, block tracker redirects, trim logo diagnostic (GHSA-j347, mw76, 29vm) (#961)

* 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]>
2026-08-02 21:17:17 +02:00
parent e2ce95ee48
commit 164129b8f5
7 changed files with 331 additions and 16 deletions
@@ -63,13 +63,40 @@ function sanitizeBrandUrl(url) {
  }

  const trimmed = url.trim();
-  if (trimmed.startsWith('javascript:')) {
+  // GHSA-j347: the old check was a case-sensitive literal `javascript:`, which
+  // `JavaScript:` walks straight past. Allowlist the schemes a logo URL can
+  // legitimately use instead of blocklisting one spelling. Relative paths (the
+  // common case — /uploads/logos/x.png) carry no scheme and are unaffected.
+  const scheme = trimmed.match(/^\s*([a-z][a-z0-9+.-]*)\s*:/i);
+  if (scheme && !['http', 'https'].includes(scheme[1].toLowerCase())) {
    return null;
  }

  return trimmed;
 }

+/**
+ * HTML-escape a brand token value (GHSA-j347).
+ *
+ * Brand tokens are substituted AFTER sanitize-html runs, so markup in a token
+ * value reaches the public page 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 escaping the five HTML-significant
+ * characters is correct in both positions.
+ *
+ * Mirrors galleryOgService's escapeHtml, which already handles this correctly.
+ */
+function escapeTokenValue(value) {
+  if (value === null || value === undefined) return '';
+  return String(value)
+    .replace(/&/g, '&amp;')
+    .replace(/</g, '&lt;')
+    .replace(/>/g, '&gt;')
+    .replace(/"/g, '&quot;')
+    .replace(/'/g, '&#039;');
+}
+
 async function fetchBrandingContext() {
  const rows = await db('app_settings')
    .whereIn('setting_key', [
@@ -265,13 +292,18 @@ function applyBrandTokens(html, branding) {
    brand_text_hex: branding.colors?.text || '#0f172a'
  };

+  // Escape on substitution (GHSA-j347) — this runs AFTER sanitizeHtmlPayload,
+  // so an unescaped value would reintroduce raw markup into the public origin.
  return html.replace(/\{\{\s*(company_name|company_tagline|support_email|brand_logo_url|brand_primary_hex|brand_accent_hex|brand_background_hex|brand_text_hex)\s*\}\}/gi,
-    (_, key) => tokens[key] || '');
+    (_, key) => escapeTokenValue(tokens[key] || ''));
 }

 module.exports = {
  getPublicSitePayload,
  clearPublicSiteCache,
  getDefaultPublicSitePayload,
-  getRawPublicSiteSettings
+  getRawPublicSiteSettings,
+  // Exposed for tests only — the token-escaping and URL-scheme rules
+  // (GHSA-j347) are worth pinning directly rather than through the cache.
+  _internal: { applyBrandTokens, sanitizeBrandUrl }
 };
@@ -59,6 +59,13 @@ function buildAdapter({ baseUrl, websiteId, apiKey }) {
            Authorization: `Bearer ${apiKey}`,
            Accept: 'application/json',
          },
+          // Never follow a redirect (GHSA-mw76). undici only strips
+          // Authorization/Cookie/Proxy-Authorization/Host when a redirect
+          // crosses origins — a custom key header would be replayed verbatim to
+          // whatever host the tracker redirects to. Self-hosted trackers on
+          // private addresses keep working; only a proxy that 301s is affected,
+          // and that surfaces as a clear logged error rather than a silent leak.
+          redirect: 'error',
          signal: controller.signal,
        });
      } catch (err) {
@@ -41,6 +41,13 @@ function buildAdapter({ baseUrl, websiteId, apiKey }) {
            'x-umami-api-key': apiKey,
            Accept: 'application/json',
          },
+          // Never follow a redirect (GHSA-mw76). undici only strips
+          // Authorization/Cookie/Proxy-Authorization/Host when a redirect
+          // crosses origins — a custom key header would be replayed verbatim to
+          // whatever host the tracker redirects to. Self-hosted trackers on
+          // private addresses keep working; only a proxy that 301s is affected,
+          // and that surfaces as a clear logged error rather than a silent leak.
+          redirect: 'error',
          signal: controller.signal,
        });
      } catch (err) {