fix(MESHCENT-004-2): 4 review findings across 2 files - #148
fix(MESHCENT-004-2): 4 review findings across 2 files#148flamingo[bot] wants to merge 2 commits into
Conversation
![flamingo[bot]](https://avatars.githubusercontent.com/in/3426709?s=60&v=4){kind=small}
| { | ||
| if ((cmd.delfiles[i] == null) || (cmd.delfiles[i].indexOf('..') >= 0)) { continue; } | ||
| try { deleteFolderRecursive(path.join(cmd.path, cmd.delfiles[i]), cmd.rec); } catch (e) { } | ||
| } |
There was a problem hiding this comment.
🦩 🔴 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; | ||
| 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) { } | ||
| } | ||
| break; |
There was a problem hiding this comment.
🦩 🔴 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
| @@ -1,4 +1,7 @@ | |||
|
|
|||
There was a problem hiding this comment.
🦩 🟠 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
| fi | ||
| rm -f $"PIDFILE" | ||
| rm -f "$PIDFILE" | ||
| fi |
There was a problem hiding this comment.
🦩 🔴 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
Closes 4 review findings across 2 files.
Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
agents/agentrecoverycore.js:271agents/agentrecoverycore.js:280agents/agentrecoverycore.js:1agents/meshinstall-initd.sh:44What 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:
6542cba8-5031-4f6a-9825-da6bc2c6e58eMerging 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.