fix(MESHCENT-003): CU-86akhf8u4 7 review findings across 3 files - #187
flamingo[bot] wants to merge 3 commits into
Conversation
![flamingo[bot]](https://avatars.githubusercontent.com/in/3426709?s=60&v=4){kind=small}
| @@ -59,6 +64,12 @@ module.exports.openframe = function (pluginHandler) { | |||
| app.get('/generate-msh', function (req, res) { | |||
There was a problem hiding this comment.
🦩 🔴 User-supplied host parameter used to build MSH file without path/content validation before embedding into downloadable file
In /generate-msh handler (plugins/openframe.js): added an authentication check requiring req.session && req.session.userid (401 if absent, mirroring the pattern used by amt-ider.js noted in the finding) and added a HOST_PATTERN regex validating the stripped host as a plain hostname/IPv4 with optional port, rejecting it with a 400 if it contains newlines, whitespace, or other characters that could inject extra MSH directives. Risk: the exact session/auth mechanism used elsewhere in this codebase (e.g. req.session.userid vs. some other property MeshCentral uses) could not be directly confirmed from the given file, so this may need adjustment to match the real session shape; also the hostname regex may be stricter than some legitimate internal hostnames (e.g. underscores), which a maintainer should verify against real deployment hostnames.
🤖 Prompt for AI agents
In plugins/openframe.js around line 59, review and complete this code-review fix: User-supplied host parameter used to build MSH file without path/content validation before embedding into downloadable file.
What the draft fix changed: In `/generate-msh` handler (plugins/openframe.js): added an authentication check requiring `req.session && req.session.userid` (401 if absent, mirroring the pattern used by amt-ider.js noted in the finding) and added a `HOST_PATTERN` regex validating the stripped host as a plain hostname/IPv4 with optional port, rejecting it with a 400 if it contains newlines, whitespace, or other characters that could inject extra MSH directives. Risk: the exact session/auth mechanism used elsewhere in this codebase (e.g. `req.session.userid` vs. some other property MeshCentral uses) could not be directly confirmed from the given file, so this may need adjustment to match the real session shape; also the hostname regex may be stricter than some legitimate internal hostnames (e.g. underscores), which a maintainer should verify against real deployment hostnames.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 55 low — review closely — react 👍/👎 to teach the reviewer
| const MESH_DIR = process.env.MESH_DIR || '/opt/mesh'; | ||
| const MESH_DEVICE_GROUP = process.env.MESH_DEVICE_GROUP || ''; | ||
|
|
||
| // RFC 1123 hostname (with optional port), or a bare IPv4 address (with optional port). | ||
| // This deliberately rejects control characters, whitespace, newlines and other | ||
| // characters that could inject additional MSH directives into the generated file. | ||
| const HOST_PATTERN = /^[A-Za-z0-9]([A-Za-z0-9-]{0,62}[A-Za-z0-9])?(\.[A-Za-z0-9]([A-Za-z0-9-]{0,62}[A-Za-z0-9])?)*(:[0-9]{1,5})?$/; | ||
|
|
||
| // --- Helpers --- | ||
|
|
||
| function corsHeaders(res) { |
There was a problem hiding this comment.
🦩 🟠 deriveTenantDomain duplicated inline in plugins/openframe.js instead of shared with db.js
Did not extract deriveTenantDomain into a shared module because no db.js content was provided and creating a require('./db') or new shared helper file risks importing/duplicating logic not visible in the given material (violating the "every import must exist" and "never invent an identifier" rules). Left the local deriveTenantDomain definition unchanged in plugins/openframe.js; this finding is NOT resolved by this change — a complete fix requires seeing db.js to either export the existing function from it or create a shared module with content verified against the real implementation.
🤖 Prompt for AI agents
In plugins/openframe.js around line 29, review and complete this code-review fix: deriveTenantDomain duplicated inline in plugins/openframe.js instead of shared with db.js.
What the draft fix changed: Did not extract `deriveTenantDomain` into a shared module because no `db.js` content was provided and creating a `require('./db')` or new shared helper file risks importing/duplicating logic not visible in the given material (violating the "every import must exist" and "never invent an identifier" rules). Left the local `deriveTenantDomain` definition unchanged in plugins/openframe.js; this finding is NOT resolved by this change — a complete fix requires seeing db.js to either export the existing function from it or create a shared module with content verified against the real implementation.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 30 low — review closely — react 👍/👎 to teach the reviewer
| @@ -113,6 +131,10 @@ module.exports.openframe = function (pluginHandler) { | |||
|
|
|||
| // 1. Verify device exists in DB | |||
| db.Get(nodeId, function (err, docs) { | |||
There was a problem hiding this comment.
🦩 🟠 db.Get callback in /api/deviceStatus ignores err parameter entirely
In the /api/deviceStatus handler (plugins/openframe.js), both db.Get callbacks now check if (err) first and return sendError(res, 500, 'Database error') with a log line via the existing log() helper, before falling through to the existing docs == null || docs.length !== 1 not-found check, for both the primary node lookup and the 'lc' + nodeId lookup.
🤖 Prompt for AI agents
In plugins/openframe.js around line 115, review and complete this code-review fix: db.Get callback in /api/deviceStatus ignores err parameter entirely.
What the draft fix changed: In the `/api/deviceStatus` handler (plugins/openframe.js), both `db.Get` callbacks now check `if (err)` first and return `sendError(res, 500, 'Database error')` with a log line via the existing `log()` helper, before falling through to the existing `docs == null || docs.length !== 1` not-found check, for both the primary node lookup and the `'lc' + nodeId` lookup.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer
| var parts = meshid.split('/'); | ||
| var base64Hash = parts[parts.length - 1]; | ||
|
|
||
| if (base64Hash.indexOf('..') !== -1 || base64Hash.indexOf('/') !== -1 || base64Hash.indexOf('\\') !== -1) { | ||
| throw new Error('Refusing to write mesh id files: derived base64Hash contains unsafe path characters: ' + base64Hash); | ||
| } | ||
|
|
||
| fs.writeFileSync(path.join(MESH_DIR, 'mesh_device_group_id'), base64Hash); | ||
| log('Wrote mesh_device_group_id: ' + base64Hash); | ||
|
|
||
| // Convert base64 (with MC's @$ escaping) → hex with 0x prefix | ||
| var standardBase64 = base64Hash.replace(/@/g, '+').replace(/\$/g, '/'); | ||
| var hex = Buffer.from(standardBase64, 'base64').toString('hex').toUpperCase(); | ||
| var meshIdHex = '0x' + hex; | ||
|
|
||
| if (meshIdHex.indexOf('..') !== -1 || meshIdHex.indexOf('/') !== -1 || meshIdHex.indexOf('\\') !== -1) { | ||
| throw new Error('Refusing to write mesh id files: derived meshIdHex contains unsafe path characters: ' + meshIdHex); | ||
| } | ||
|
|
||
| fs.writeFileSync(path.join(MESH_DIR, 'mesh_id'), meshIdHex); | ||
| log('Wrote mesh_id: ' + meshIdHex); | ||
|
|
There was a problem hiding this comment.
🦩 🔴 writeMeshIdFiles / generateMshFile write into MESH_DIR without validating derived tenant domain or meshid content for path traversal
In writeMeshIdFiles (around the base64Hash derivation) and generateMshFile, added explicit checks that reject base64Hash/meshIdHex containing .., /, or \ by throwing before any fs.writeFileSync call. This blocks path-traversal segments from ever reaching path.join(MESH_DIR, ...), satisfying the requirement to validate derived path segments before use in fs operations. The check is defensive/best-effort since the exact expected format (base64 with @/$ escaping, hex string) is not itself re-validated against a strict allowlist, only against traversal-relevant characters.
🤖 Prompt for AI agents
In plugins/migrate.js around line 273, review and complete this code-review fix: writeMeshIdFiles / generateMshFile write into MESH_DIR without validating derived tenant domain or meshid content for path traversal.
What the draft fix changed: In `writeMeshIdFiles` (around the base64Hash derivation) and `generateMshFile`, added explicit checks that reject `base64Hash`/`meshIdHex` containing `..`, `/`, or `\` by throwing before any `fs.writeFileSync` call. This blocks path-traversal segments from ever reaching `path.join(MESH_DIR, ...)`, satisfying the requirement to validate derived path segments before use in fs operations. The check is defensive/best-effort since the exact expected format (base64 with `@`/`$` escaping, hex string) is not itself re-validated against a strict allowlist, only against traversal-relevant characters.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer
| var raw = fs.readFileSync(configfile, 'utf8'); | ||
| // Strip any ${VAR} placeholders that weren't substituted — fall back to env | ||
| raw = raw.replace(/\$\{([A-Z_][A-Z0-9_]*)\}/g, function (m, name) { | ||
| return process.env[name] || ''; | ||
| var val = process.env[name] || ''; | ||
| return JSON.stringify(val).slice(1, -1); | ||
| }); | ||
| return JSON.parse(raw); | ||
| } |
There was a problem hiding this comment.
🦩 🟠 loadConfig performs unsafe regex-based env var substitution then JSON.parse without escaping injected values
In loadConfig, the ${VAR} regex-substitution callback now wraps process.env[name] || '' in JSON.stringify(val).slice(1, -1) before splicing it into the raw JSON text, matching the suggested fix exactly. This escapes quotes, backslashes, and newlines so injected env values cannot break out of their JSON string context.
🤖 Prompt for AI agents
In plugins/migrate.js around line 131, review and complete this code-review fix: loadConfig performs unsafe regex-based env var substitution then JSON.parse without escaping injected values.
What the draft fix changed: In `loadConfig`, the `${VAR}` regex-substitution callback now wraps `process.env[name] || ''` in `JSON.stringify(val).slice(1, -1)` before splicing it into the raw JSON text, matching the suggested fix exactly. This escapes quotes, backslashes, and newlines so injected env values cannot break out of their JSON string context.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer
| db.Get(userid, function (getErr, userDocs) { | ||
| if (getErr || !userDocs || userDocs.length !== 1) { | ||
| // Not fatal — mesh is created, just couldn't update user links | ||
| err('Could not update user links for ' + userid + ' after creating mesh ' + meshid + (getErr ? (': ' + getErr) : '')); | ||
| return cb(null, meshid); | ||
| } | ||
| var user = userDocs[0]; |
There was a problem hiding this comment.
🦩 🟠 ensureDeviceGroup swallows failed user-link update without surfacing a warning
In ensureDeviceGroup, the db.Get(userid, ...) callback's early-return branch (when getErr || !userDocs || userDocs.length !== 1) now calls err(...) with a descriptive message including the userid, meshid, and underlying getErr (if any) before returning cb(null, meshid), giving operators a diagnostic trail as suggested.
🤖 Prompt for AI agents
In plugins/migrate.js around line 253, review and complete this code-review fix: ensureDeviceGroup swallows failed user-link update without surfacing a warning.
What the draft fix changed: In `ensureDeviceGroup`, the `db.Get(userid, ...)` callback's early-return branch (when `getErr || !userDocs || userDocs.length !== 1`) now calls `err(...)` with a descriptive message including the userid, meshid, and underlying getErr (if any) before returning `cb(null, meshid)`, giving operators a diagnostic trail as suggested.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer
| { | ||
| if (process.argv[i].startsWith('--installPath=')) | ||
| { | ||
| p.push('--installPath="' + process.argv[i].split('=').pop() + '"'); | ||
| var installPathValue = process.argv[i].split('=').pop(); | ||
| if (installPathValue.indexOf('..') != -1) | ||
| { | ||
| console.log('Invalid --installPath value: path traversal ("..") is not allowed.'); | ||
| process.exit(); | ||
| } | ||
| p.push('--installPath="' + installPathValue + '"'); | ||
| } | ||
| else if(process.argv[i].startsWith('--')) | ||
| { |
There was a problem hiding this comment.
🦩 🔴 agents/meshinstall-linux.js writes user-supplied --installPath value into shell command without '..' path traversal check
In the CLI arg parsing loop (the if (!skip) { if (process.argv.includes('-install') || ...) { ... } } block near line 225), added an explicit check that rejects any --installPath= value containing .. before it is pushed onto the p array passed to _install(). If .. is found, the script logs an error and calls process.exit() instead of proceeding, preventing the tainted path from ever reaching filesystem operations performed by the spawned install process. This directly implements the required "check for '..' before use" per MESHCENT-003/MESHCENT-004-2. Residual risk: this only blocks the literal substring ..; it does not canonicalize/resolve the path (e.g. via realpath) to catch more exotic traversal encodings, and the downstream _install re-spawn (in the full-install child process, not shown/available in this file) is not modified — a complete fix might also validate the path is absolute/within an allowed root at the point it's actually used for filesystem writes.
🤖 Prompt for AI agents
In agents/meshinstall-linux.js around line 225, review and complete this code-review fix: agents/meshinstall-linux.js writes user-supplied --installPath value into shell command without '..' path traversal check.
What the draft fix changed: In the CLI arg parsing loop (the `if (!skip) { if (process.argv.includes('-install') || ...) { ... } }` block near line 225), added an explicit check that rejects any `--installPath=` value containing `..` before it is pushed onto the `p` array passed to `_install()`. If `..` is found, the script logs an error and calls `process.exit()` instead of proceeding, preventing the tainted path from ever reaching filesystem operations performed by the spawned install process. This directly implements the required "check for '..' before use" per MESHCENT-003/MESHCENT-004-2. Residual risk: this only blocks the literal substring `..`; it does not canonicalize/resolve the path (e.g. via realpath) to catch more exotic traversal encodings, and the downstream `_install` re-spawn (in the full-install child process, not shown/available in this file) is not modified — a complete fix might also validate the path is absolute/within an allowed root at the point it's actually used for filesystem writes.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 75 medium — react 👍/👎 to teach the reviewer
flamingo
Bot
changed the title
Closes 7 review findings across 3 files.
Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
plugins/openframe.js:59plugins/openframe.js:29plugins/openframe.js:115plugins/migrate.js:273plugins/migrate.js:131plugins/migrate.js:253agents/meshinstall-linux.js:225What changed — and what was deliberately left — is explained per finding as inline review comments on the lines each finding touched.
Run: https://product-hub.flamingo.so/admin/code-review
Run id:
a05149ef-c4b4-48a6-a445-70f3e3fde4b4Merging this PR is recorded as acceptance of the rule that produced it;
closing it unmerged is recorded as rejection. Both feed rule health, so
closing a wrong suggestion is useful rather than merely tidy.
ClickUp task: CU-86akhf8u4 MeshCentral review findings sweep (3 PRs)