Fix arbitrary code execution and predictable session tokens in /updateTheme - #1425
Fix arbitrary code execution and predictable session tokens in /updateTheme#1425herdiyana256 wants to merge 4 commits into
Conversation
…eTheme Two issues combine into an exploitable chain: 1. server/token.mjs: getSecretFromCDAPConfig() fell back to path.resolve(__dirname, 'config', 'development', 'session.secret.key'), a file path string, used directly as key material, whenever cdapConfig['session.secret.key'] wasn't set. That value is fully predictable by anyone who knows the install directory, letting them forge valid session tokens with no access to any real secret. Now throws instead of falling back, so token generation/validation fails closed when the secret isn't configured. 2. server/uiThemeWrapper.js: extractUITheme() loaded theme files with __non_webpack_require__, which executes .js files as Node modules. Combined with the POST /updateTheme route in server/express.js, which passes req.body.uiThemePath straight into extractUITheme() with no validation beyond the (forgeable) session-token check, this let a request execute arbitrary code on the server. Theme files are JSON data, not code, so extractUITheme now reads and JSON.parses them instead of require()'ing them. Also restricted /updateTheme's uiThemePath to a filename inside config/themes/ (discarding any directory component), matching the route's own stated purpose of switching between the shipped theme files for testing. Added a regression test for each: token_test.mjs confirms a missing secret now causes generateToken to throw and validateToken to reject everything (including a token forged with the old fallback), and uiThemeWrapper.test.js confirms a .js file passed as the theme path is no longer executed while a plain JSON theme file still loads. Full yarn install could not complete in this environment (large legacy dependency tree), so uiThemeWrapper.test.js could not be run through the project's actual Jest config here; its logic was verified by extracting extractUITheme's unmodified post-fix code into a standalone harness with only __dirname/log4js/lodash shimmed for environment reasons, run directly under Node.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces critical security fixes to prevent directory traversal and arbitrary code execution. It restricts theme file resolution to a specific directory, replaces unsafe dynamic requires with JSON parsing, and fails closed when the session secret key is unconfigured. The review feedback correctly identifies that the directory traversal defense is incomplete, as inputs like '..' or '.' can bypass path.basename, and a non-string input could cause an unhandled TypeError.
path.basename returns '..' or '.' unchanged for those exact inputs (it strips directory components, it doesn't resolve '..' segments), so a request with uiThemePath: ".." resolved one level above config/themes/ instead of being contained to it. A non-string uiThemePath (e.g. an array) also threw a TypeError out of path.basename before reaching the try/catch. Both from review feedback on PR cdapio#1425.
…eTheme in prod - token.mjs: when NODE_ENV=development and session.secret.key is not configured, fall back to reading the actual file contents from config/development/session_secret.key via fs.readFileSync() instead of throwing. All other environments (including production) still fail closed. Fixes the original bug where a file *path string* was used as key material. - express.js: /updateTheme now returns 401 when NODE_ENV=production, making the endpoint completely unavailable in production as intended by the route's own doc comment. - uiThemeWrapper.js: trim the RCE-detail comment now that the endpoint is disabled in production and the attack surface no longer exists.
|
All three points addressed in f77678d, threads resolved. |
Leftover from an earlier approach to resolving __dirname; never referenced, and .eslintrc.json's no-unused-vars rule (severity: error) would fail on it.
|
Hi @herdiyana256, Thank you so much for discovering this vulnerability and taking the time to contribute a solid fix! We deeply appreciate the effort you put into helping keep this project secure. Also, thank you for your quick turnaround on the changes we discussed during the review. The code looks good to me, and I've just approved the PR so we can get this merged. We would be absolutely thrilled to see more contributions from you in the future! Whether it's more security improvements, general bug fixes, or new features, your work is very welcome here. Thanks again! Best |
|
Thanks again @GnsP, really appreciate the kind words and the quick reviews throughout. Since this is approved with all checks green and no conflicts with Would love to get this one in, and I've got #1426 and #1427 up as well whenever you have bandwidth for those. |
|
Hi @GnsP gentle bump on this one. It's been approved for two weeks now with all checks green and no conflicts with Same question applies to #1426 and #1427 whenever you have bandwidth. Thanks! |
2 participants
Two issues in server/token.mjs and server/uiThemeWrapper.js combine into an exploitable chain through the POST /updateTheme route in server/express.js.
getSecretFromCDAPConfig() in server/token.mjs fell back to path.resolve(__dirname, 'config', 'development', 'session.secret.key') -- a file path string, not that file's contents -- used directly as key material whenever cdapConfig['session.secret.key'] isn't configured. That value is fully predictable by anyone who knows the install directory, so a session token can be forged with no access to any real secret. The fallback also doesn't match any real file on disk (the actual file is session_secret.key, with an underscore), which points to this being a missing fs.readFileSync() call rather than intentional behavior.
extractUITheme() in server/uiThemeWrapper.js loaded the theme file with non_webpack_require, which executes .js files as Node modules. POST /updateTheme passes req.body.uiThemePath straight into extractUITheme() with no validation beyond the (forgeable) session-token check above. Together: forge a token, then point uiThemePath at any .js file the server can read, and its code runs with the server process's privileges.
token.mjs now throws instead of falling back to a predictable value when session.secret.key isn't configured, so token generation/validation fails closed. uiThemeWrapper.js reads and JSON.parse()s theme files instead of require()-ing them, since they're data, not code. express.js additionally resolves uiThemePath in /updateTheme to a filename inside config/themes/ only, matching the route's own doc comment describing it as a way to switch between the shipped theme files for testing.
Added a regression test for each half: token_test.mjs confirms a missing secret makes generateToken throw and validateToken reject everything, including a token forged with the old fallback logic; the new uiThemeWrapper.test.js confirms a plain JSON theme file still loads and a .js file passed as the theme path is no longer executed.
yarn install didn't complete in the environment I used for this (large, older dependency tree), so I couldn't run uiThemeWrapper.test.js through the project's actual Jest config. Verified its logic instead by extracting extractUITheme's unmodified post-fix code into a standalone harness (only __dirname/log4js/lodash shimmed for environment reasons) and running it directly under Node -- confirmed the JSON case still loads and the .js case no longer executes. Happy to re-verify against the real Jest run if that's easier for a reviewer with the deps already installed.