Skip to content

fix(MESHCENT-003): CU-86akhf8u4 7 review findings across 3 files #187

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
flamingo wants to merge 3 commits into
base: master
Choose a base branch
from
+50 −2
Draft

fix(MESHCENT-003): CU-86akhf8u4 7 review findings across 3 files #187

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a2a5c8b
fix(MESHCENT-003): 7 review findings across 3 files
flamingo[bot] Sep 14, 2026
8376ef4
fix(MESHCENT-003): 7 review findings across 3 files
flamingo[bot] Sep 14, 2026
b88fb59
fix(MESHCENT-003): 7 review findings across 3 files
flamingo[bot] Sep 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion agents/meshinstall-linux.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,13 @@ if ((!skip) && ((msh.InstallFlags & 2) == 2))
{
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('--'))
{
Comment on lines 229 to 241

@flamingo flamingo Bot Sep 14, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🦩 🔴 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

Expand Down Expand Up @@ -359,3 +365,4 @@ if (!skip)
process.exit();
});
}

17 changes: 16 additions & 1 deletion plugins/migrate.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,8 @@ function loadConfig(configfile) {
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);
}
Comment on lines 132 to 139

@flamingo flamingo Bot Sep 14, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🦩 🟠 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

Expand Down Expand Up @@ -254,6 +255,7 @@ function ensureDeviceGroup(db, domain, userid, cb) {
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];
Comment on lines 255 to 261

@flamingo flamingo Bot Sep 14, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🦩 🟠 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

Expand All @@ -275,13 +277,22 @@ function writeMeshIdFiles(meshid, serverIdHex) {
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);

Comment on lines 277 to 298

@flamingo flamingo Bot Sep 14, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🦩 🔴 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

Expand All @@ -294,6 +305,10 @@ function writeMeshIdFiles(meshid, serverIdHex) {
// --- Step 7: Generate meshagent.msh file ---

function generateMshFile(meshIdHex, serverIdHex) {
if (meshIdHex.indexOf('..') !== -1 || meshIdHex.indexOf('/') !== -1 || meshIdHex.indexOf('\\') !== -1) {
throw new Error('Refusing to generate msh file: meshIdHex contains unsafe path characters: ' + meshIdHex);
}

var meshServerUrl;
if (OPENFRAME_MODE === 'true' && OPENFRAME_GATEWAY_URL) {
log('OpenFrame mode enabled — using gateway URL');
Expand Down
26 changes: 26 additions & 0 deletions plugins/openframe.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,11 @@ const path = require('path');
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) {
Comment on lines 6 to 16

@flamingo flamingo Bot Sep 14, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🦩 🟠 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

Expand Down Expand Up @@ -59,6 +64,12 @@ module.exports.openframe = function (pluginHandler) {
app.get('/generate-msh', function (req, res) {

@flamingo flamingo Bot Sep 14, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🦩 🔴 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

corsHeaders(res);

// Require an authenticated MeshCentral session, consistent with other endpoints (e.g.
// amt-ider.js), since this generates a downloadable agent config tied to server identity.
if (req.session == null || !req.session.userid) {
return sendError(res, 401, 'Authentication required');
}

var host = req.query.host;
if (!host) return sendError(res, 400, 'Missing required parameter: host');

Expand All @@ -74,6 +85,13 @@ module.exports.openframe = function (pluginHandler) {

var protocol = host.startsWith('http://') ? 'ws' : 'wss';
var cleanHost = host.replace(/^https?:\/\//, '').replace(/^wss?:\/\//, '');

// Validate the stripped host: must be a plain hostname/IPv4 with optional port and no
// newlines, whitespace or other characters that could inject extra MSH directives.
if (!HOST_PATTERN.test(cleanHost)) {
return sendError(res, 400, 'Invalid host parameter');
}

var meshServerUrl = protocol + '://' + cleanHost + '/ws/tools/agent/meshcentral-server/agent.ashx';

var mshContent = [
Expand Down Expand Up @@ -113,6 +131,10 @@ module.exports.openframe = function (pluginHandler) {

// 1. Verify device exists in DB
db.Get(nodeId, function (err, docs) {

@flamingo flamingo Bot Sep 14, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🦩 🟠 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

if (err) {
log('db.Get error for ' + nodeId + ': ' + err);
return sendError(res, 500, 'Database error');
}
if (docs == null || docs.length !== 1) return sendError(res, 404, 'Device not found');

// 2. Live connectivity state from MeshCentral in-memory store
Expand All @@ -121,6 +143,10 @@ module.exports.openframe = function (pluginHandler) {

// 3. Last connection record from DB
db.Get('lc' + nodeId, function (err, docs) {
if (err) {
log('db.Get error for lc' + nodeId + ': ' + err);
return sendError(res, 500, 'Database error');
}
var lc = (docs != null && docs.length === 1) ? docs[0] : null;

res.json({
Expand Down