Skip to content

fix(MESHCENT-004-2): 4 review findings across 2 files #148

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 2 commits into
base: master
Choose a base branch
from
+10 −1
Draft

fix(MESHCENT-004-2): 4 review findings across 2 files #148

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
92c7ef9
fix(MESHCENT-004-2): 4 review findings across 2 files
flamingo[bot] Aug 24, 2026
b9186d9
fix(MESHCENT-004-2): 4 review findings across 2 files
flamingo[bot] Aug 24, 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
8 changes: 8 additions & 0 deletions agents/agentrecoverycore.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@

@flamingo flamingo Bot Aug 24, 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.

🦩 🟠 agentrecoverycore.js is missing jshint header and 'use strict'

Added the standard header block (/*jshint node: true */, /*jshint strict: false */, "use strict";) at the top of the file, immediately before the existing var http = require('http'); line, matching the convention used in other server-side modules; no other lines were altered.

🤖 Prompt for AI agents 
In agents/agentrecoverycore.js around line 1, review and complete this code-review fix: agentrecoverycore.js is missing jshint header and 'use strict'.
What the draft fix changed: Added the standard header block (`/*jshint node: true */`, `/*jshint strict: false */`, `"use strict";`) at the top of the file, immediately before the existing `var http = require('http');` line, matching the convention used in other server-side modules; no other lines were altered.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer

/*jshint node: true */
/*jshint strict: false */
"use strict";
var http = require('http');
var childProcess = require('child_process');
var meshCoreObj = { "action": "coreinfo", "value": "MeshCore Recovery", "caps": 14 }; // Capability bitmask: 1 = Desktop, 2 = Terminal, 4 = Files, 8 = Console, 16 = JavaScript
Expand Down Expand Up @@ -255,18 +258,22 @@ require('MeshAgent').AddCommandHandler(function (data)
break;
case 'mkdir': {
// Create a new empty folder
if ((cmd.path == null) || (cmd.path.indexOf('..') >= 0)) { break; }
fs.mkdirSync(cmd.path);
break;
}
case 'mkfile': {
// Create a new empty file
if ((cmd.path == null) || (cmd.path.indexOf('..') >= 0)) { break; }
fs.closeSync(fs.openSync(cmd.path, 'w'));
break;
}
case 'rm': {
// Delete, possibly recursive delete
if ((cmd.path == null) || (cmd.path.indexOf('..') >= 0)) { break; }
for (var i in cmd.delfiles)
{
if ((cmd.delfiles[i] == null) || (cmd.delfiles[i].indexOf('..') >= 0)) { continue; }
try { deleteFolderRecursive(path.join(cmd.path, cmd.delfiles[i]), cmd.rec); } catch (e) { }
}

@flamingo flamingo Bot Aug 24, 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.

🦩 🔴 mkdir/mkfile commands use raw cmd.path without '..' traversal validation

In the WebSocket file-command handler (protocol 5 switch), added '..' traversal guards to the mkdir case (checks cmd.path before fs.mkdirSync) and the mkfile case (checks cmd.path before fs.openSync/fs.closeSync); both now silently break if cmd.path is null or contains .., matching the established convention referenced in the finding.

🤖 Prompt for AI agents 
In agents/agentrecoverycore.js around line 271, review and complete this code-review fix: mkdir/mkfile commands use raw cmd.path without '..' traversal validation.
What the draft fix changed: In the WebSocket file-command handler (protocol 5 switch), added `'..'` traversal guards to the `mkdir` case (checks `cmd.path` before `fs.mkdirSync`) and the `mkfile` case (checks `cmd.path` before `fs.openSync`/`fs.closeSync`); both now silently `break` if `cmd.path` is null or contains `..`, matching the established convention referenced in the finding.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 80 medium — react 👍/👎 to teach the reviewer

break;
Comment on lines 258 to 279

@flamingo flamingo Bot Aug 24, 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.

🦩 🔴 File deletion command 'rm' processes user-supplied paths without a '..' traversal check

In the rm case of the same handler, added a guard rejecting the whole operation if cmd.path contains .., and added a per-entry check inside the for (var i in cmd.delfiles) loop to continue (skip) any cmd.delfiles[i] entry containing .. before it is joined into a path and passed to deleteFolderRecursive. This blocks the described traversal vector; note it does not add symlink or absolute-path canonicalization checks beyond the literal .. substring test, consistent with the same convention used elsewhere in this codebase.

🤖 Prompt for AI agents 
In agents/agentrecoverycore.js around line 280, review and complete this code-review fix: File deletion command 'rm' processes user-supplied paths without a '..' traversal check.
What the draft fix changed: In the `rm` case of the same handler, added a guard rejecting the whole operation if `cmd.path` contains `..`, and added a per-entry check inside the `for (var i in cmd.delfiles)` loop to `continue` (skip) any `cmd.delfiles[i]` entry containing `..` before it is joined into a path and passed to `deleteFolderRecursive`. This blocks the described traversal vector; note it does not add symlink or absolute-path canonicalization checks beyond the literal `..` substring test, consistent with the same convention used elsewhere in this codebase.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 65 medium — react 👍/👎 to teach the reviewer

Expand Down Expand Up @@ -469,3 +476,4 @@ function deleteFolderRecursive(path, rec) {
fs.unlinkSync(path);
}
};

3 changes: 2 additions & 1 deletion agents/meshinstall-initd.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ stop() {
else
echo 'Service not running'
fi
rm -f $"PIDFILE"
rm -f "$PIDFILE"
fi

@flamingo flamingo Bot Aug 24, 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.

🦩 🔴 meshinstall-initd.sh builds and eval's a command string containing unescaped shell metacharacters via su -c

In the stop() function, changed rm -f $"PIDFILE" to rm -f "$PIDFILE", correcting the malformed variable expansion/quoting ($"PIDFILE" treated "PIDFILE" as a literal string with a stray $, rather than expanding the PIDFILE variable) so the correct pidfile path is removed. This directly matches the suggested fix. Note: the broader concern raised about su -c "$CMD" construction and potential injection if SCRIPT/RUNAS/LOGFILE become templated from external input was not restructured, since those variables are currently static literals in this file and no injection path exists today; addressing that fully would require architectural changes (e.g., using su -s /bin/sh -c with an array-based exec or avoiding string interpolation entirely) that are out of scope for a minimal, safe fix and risk breaking the existing JS payload escaping.

🤖 Prompt for AI agents 
In agents/meshinstall-initd.sh around line 44, review and complete this code-review fix: meshinstall-initd.sh builds and eval's a command string containing unescaped shell metacharacters via su -c.
What the draft fix changed: In the stop() function, changed `rm -f $"PIDFILE"` to `rm -f "$PIDFILE"`, correcting the malformed variable expansion/quoting (`$"PIDFILE"` treated `"PIDFILE"` as a literal string with a stray `$`, rather than expanding the `PIDFILE` variable) so the correct pidfile path is removed. This directly matches the suggested fix. Note: the broader concern raised about `su -c "$CMD"` construction and potential injection if SCRIPT/RUNAS/LOGFILE become templated from external input was not restructured, since those variables are currently static literals in this file and no injection path exists today; addressing that fully would require architectural changes (e.g., using `su -s /bin/sh -c` with an array-based exec or avoiding string interpolation entirely) that are out of scope for a minimal, safe fix and risk breaking the existing JS payload escaping.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer

}
restart(){
Expand Down Expand Up @@ -82,3 +82,4 @@ case "$1" in
;;
esac
exit 0