diff --git a/server/express.js b/server/express.js index 4c65a57b061..d7d30bbd118 100644 --- a/server/express.js +++ b/server/express.js @@ -709,6 +709,13 @@ function makeApp(authAddress, cdapConfig, uiSettings) { * be used and we need to persist this information somewhere. */ app.post('/updateTheme', function (req, res) { + // This endpoint is only for testing themes in development and must not be + // active in production (it has no persistent effect and opens unnecessary + // attack surface). Return 401 so callers get a clear signal rather than a + // silent no-op. + if (isModeProduction()) { + return res.status(401).send('This endpoint is not available in production'); + } let authToken = req.headers.authorization; if ( !req.headers['session-token'] || @@ -717,10 +724,24 @@ function makeApp(authAddress, cdapConfig, uiSettings) { ) { return res.status(500).send('Unable to validate session'); } - const uiThemePath = req.body.uiThemePath; - if (!uiThemePath) { + const requestedTheme = req.body.uiThemePath; + if (typeof requestedTheme !== 'string' || !requestedTheme) { + return res.status(500).send('UnKnown theme file. Please make sure the path is valid'); + } + // This endpoint exists to switch between the theme files shipped in + // config/themes/ for testing (see doc comment above), not to load an + // arbitrary path. Resolve to a filename within that directory only, + // discarding any directory component the client supplies, so this can't + // be used to point at a file elsewhere on the filesystem. path.basename + // returns '..' or '.' unchanged for those exact inputs (it only strips + // directory components, it doesn't resolve '..' segments), so those two + // values need an explicit reject or they'd resolve one level above + // config/themes/. + const baseTheme = path.basename(requestedTheme); + if (baseTheme === '..' || baseTheme === '.' || !baseTheme) { return res.status(500).send('UnKnown theme file. Please make sure the path is valid'); } + const uiThemePath = path.join(__dirname, 'config', 'themes', baseTheme); try { uiThemeConfig = uiThemeWrapper.extractUITheme(cdapConfig, uiThemePath); } catch (e) { diff --git a/server/token.mjs b/server/token.mjs index 2c06a1ebb94..862d563b446 100644 --- a/server/token.mjs +++ b/server/token.mjs @@ -15,8 +15,11 @@ */ import crypto from 'crypto'; +import fs from 'fs'; import path from 'path'; +/* global process, __dirname */ + const EncryptionConstants = { ONE_HOUR_MILLIS: 60 * 60 * 1000, /** @@ -97,15 +100,23 @@ function decrypt(encText, key) { } function getSecretFromCDAPConfig(cdapConfig, logger) { - let secretKey = - cdapConfig['session.secret.key'] || - path.resolve(__dirname, 'config', 'development', 'session.secret.key'); if (!logger) { logger = console; } + const secretKey = cdapConfig['session.secret.key']; if (!secretKey) { - logger.warn( - 'Secret key missing. This is required to generate a strong time-based token to prevent cswh' + // In development, fall back to reading the actual secret file on disk. + // This avoids the old bug where the file *path string* was used as key + // material instead of the file's contents. + if (process.env.NODE_ENV === 'development') { + const keyFilePath = path.resolve(__dirname, 'config', 'development', 'session_secret.key'); + return fs.readFileSync(keyFilePath, 'utf8'); + } + // In production (or any other environment), fail closed: a deployment + // must explicitly configure a real secret via 'session.secret.key'. + throw new Error( + "'session.secret.key' is not configured. A real secret is required to generate " + + 'or validate session tokens.' ); } return secretKey; diff --git a/server/token_test.mjs b/server/token_test.mjs index d492226eb26..65a9606ae3c 100644 --- a/server/token_test.mjs +++ b/server/token_test.mjs @@ -41,5 +41,30 @@ function testMismatch() { assert(!isTokenValid); } +// A missing 'session.secret.key' must not fall back to any predictable value. +// generateToken should refuse to issue a token, and validateToken should +// reject everything (including a token forged with the old, predictable +// fallback), rather than accepting a guessable secret. +function testMissingSecretKeyRefusesToken() { + const cdapConfigNoSecret = { 'instance.metadata.id': 'test-instance' }; + + let threw = false; + try { + generateToken(cdapConfigNoSecret, console); + } catch (e) { + threw = true; + } + assert(threw, 'generateToken should throw when session.secret.key is not configured'); + + const isValid = validateToken('anything-at-all', cdapConfigNoSecret, console); + assert(!isValid, 'validateToken should reject when session.secret.key is not configured'); + + console.log('testMissingSecretKeyRefusesToken passed'); +} + testMatch(); +testMissingSecretKeyRefusesToken(); +// Note: testMismatch() fails on master independent of this change, since +// authToken isn't actually part of the token or the comparison in either +// generateToken or validateToken. Left as-is, out of scope here. testMismatch(); diff --git a/server/uiThemeWrapper.js b/server/uiThemeWrapper.js index b53464bcbe0..2eae1f77f86 100644 --- a/server/uiThemeWrapper.js +++ b/server/uiThemeWrapper.js @@ -50,6 +50,12 @@ function extractUIFeaturesFromConfig(cdapConfig) { return featuresMap; } +// Theme files are plain JSON data, not code. Read and parse them directly +// instead of require()'ing them to avoid executing attacker-controlled input. +function readThemeJSON(themePath) { + return JSON.parse(fs.readFileSync(themePath, 'utf8')); +} + function mergeUIThemeWithConfig(cdapConfig, themeConfig) { const configFeatures = { features: extractUIFeaturesFromConfig(cdapConfig), @@ -71,11 +77,9 @@ export function extractUITheme(cdapConfig, uiThemePath) { // Absolute path if (uiThemePath[0] === '/') { try { - if (__non_webpack_require__.resolve(uiThemePath)) { - uiThemeConfig = __non_webpack_require__(uiThemePath); - log.info(`UI using theme file: ${uiThemePath}`); - return mergeUIThemeWithConfig(cdapConfig, uiThemeConfig); - } + uiThemeConfig = readThemeJSON(uiThemePath); + log.info(`UI using theme file: ${uiThemePath}`); + return mergeUIThemeWithConfig(cdapConfig, uiThemeConfig); } catch (e) { log.info('UI Theme file not found at: ', uiThemePath); throw e; @@ -105,11 +109,9 @@ export function extractUITheme(cdapConfig, uiThemePath) { } themePath = path.join(__dirname, themePath); - if (__non_webpack_require__.resolve(themePath)) { - uiThemeConfig = __non_webpack_require__(themePath); - log.info(`UI using theme file: ${themePath}`); - return mergeUIThemeWithConfig(cdapConfig, uiThemeConfig); - } + uiThemeConfig = readThemeJSON(themePath); + log.info(`UI using theme file: ${themePath}`); + return mergeUIThemeWithConfig(cdapConfig, uiThemeConfig); } catch (e) { // This will show the user what the full path is. // This should help them give proper relative path diff --git a/server/uiThemeWrapper.test.js b/server/uiThemeWrapper.test.js new file mode 100644 index 00000000000..7b7e43314fc --- /dev/null +++ b/server/uiThemeWrapper.test.js @@ -0,0 +1,51 @@ +/* + * Copyright © 2026 Cask Data, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not + * use this file except in compliance with the License. You may obtain a copy of + * the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations under + * the License. + */ + +import fs from 'fs'; +import os from 'os'; +import path from 'path'; +import { extractUITheme } from 'server/uiThemeWrapper'; + +describe('extractUITheme', () => { + const cdapConfig = { 'ui.theme.file': true }; + + test('loads a plain JSON theme file', () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'ui-theme-test-')); + const themePath = path.join(dir, 'theme.json'); + fs.writeFileSync(themePath, JSON.stringify({ content: { theme: 'ok' } })); + + const result = extractUITheme(cdapConfig, themePath); + expect(result.content.theme).toBe('ok'); + }); + + // Regression test: extractUITheme used to load the theme file with + // __non_webpack_require__, which executes .js files as Node modules + // instead of just reading them as data. A path ending in .js that runs + // code (instead of failing to parse as JSON) means this is still + // executing the file rather than reading it. + test('does not execute a .js file passed as the theme path', () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'ui-theme-test-')); + const canaryPath = path.join(dir, 'canary.txt'); + const themePath = path.join(dir, 'theme.js'); + fs.writeFileSync( + themePath, + `require('fs').writeFileSync(${JSON.stringify(canaryPath)}, 'executed');\nmodule.exports = { content: { theme: 'pwned' } };\n` + ); + + expect(() => extractUITheme(cdapConfig, themePath)).toThrow(); + expect(fs.existsSync(canaryPath)).toBe(false); + }); +});