Skip to content

Restrict /updateTheme to the configured theme file and harden market host verification#1405

Open
adilburaksen wants to merge 2 commits into
cdapio:developfrom
adilburaksen:fix-updatetheme-and-market-host
Open

Restrict /updateTheme to the configured theme file and harden market host verification#1405
adilburaksen wants to merge 2 commits into
cdapio:developfrom
adilburaksen:fix-updatetheme-and-market-host

Conversation

@adilburaksen
Copy link
Copy Markdown

1. /updateTheme: arbitrary file load via uiThemePath

The /updateTheme handler reads req.body.uiThemePath and passes it to uiThemeWrapper.extractUITheme(), which for an absolute path calls __non_webpack_require__(uiThemePath) — loading any absolute path on the server as a Node module. The endpoint's only legitimate use is reloading the operator-configured theme.

Fix: reject any uiThemePath that does not exactly match the configured ui.theme.file. This preserves the reload-theme behavior while removing the arbitrary-require primitive.

2. isVerifiedMarketHost: allowlist bypass via startsWith

isVerifiedMarketHost validates a market URL with url.startsWith(configuredMarketUrl). A prefix check is bypassable: with a host-only configured market URL, https://market.example.com.evil.com/... and https://market.example.com@evil.com/... both pass, letting the market proxy (/market, /forwardMarketToCdap) fetch an attacker-controlled host (server-side request forgery).

Fix: parse both the requested and configured URLs with URL, require an exact origin match, and require the requested path to be contained within the configured base path.

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request addresses security vulnerabilities by validating the theme file path against the configured theme file to prevent arbitrary file loading, and by hardening market host verification using proper URL parsing to prevent SSRF bypasses. A review comment suggests optimizing the URL verification logic by caching parsed base URLs in a WeakMap to avoid repeated parsing overhead.

Comment thread server/url-helper.js Outdated
Comment on lines +70 to +82
return !!getMarketUrls(cdapConfig).find((element) => {
let base;
try {
base = new URL(element);
} catch (e) {
return false;
}
if (requested.origin !== base.origin) {
return false;
}
const basePath = base.pathname.endsWith('/') ? base.pathname : `${base.pathname}/`;
return requested.pathname === base.pathname || requested.pathname.startsWith(basePath);
});
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Parsing the configured market URLs using 'new URL()' on every single request validation is inefficient, especially since 'getMarketUrls(cdapConfig)' is already memoized and the configuration rarely changes. We can optimize this by caching the parsed 'URL' objects using a 'WeakMap' keyed by the 'cdapConfig' object. This avoids repeated parsing overhead while preventing memory leaks and key collisions.

  if (cdapConfig && typeof cdapConfig === 'object') {
    if (!isVerifiedMarketHost.cache) {
      isVerifiedMarketHost.cache = new WeakMap();
    }
    let parsedBases = isVerifiedMarketHost.cache.get(cdapConfig);
    if (!parsedBases) {
      parsedBases = getMarketUrls(cdapConfig)
        .map((element) => {
          try {
            return new URL(element);
          } catch (e) {
            return null;
          }
        })
        .filter(Boolean);
      isVerifiedMarketHost.cache.set(cdapConfig, parsedBases);
    }
    return !!parsedBases.find((base) => {
      if (requested.origin !== base.origin) {
        return false;
      }
      const basePath = base.pathname.endsWith('/') ? base.pathname : base.pathname + '/';
      return requested.pathname === base.pathname || requested.pathname.startsWith(basePath);
    });
  }
  return false;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant