Skip to content

fix(MESHCENT-004): CU-86akbhg5u 8 review findings across 6 files #170

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 6 commits into
base: master
Choose a base branch
from
Draft

fix(MESHCENT-004): CU-86akbhg5u 8 review findings across 6 files #170

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
783678d
fix(MESHCENT-004): 8 review findings across 6 files
flamingo[bot] Sep 7, 2026
f6f9199
fix(MESHCENT-004): 8 review findings across 6 files
flamingo[bot] Sep 7, 2026
08d2aef
fix(MESHCENT-004): 8 review findings across 6 files
flamingo[bot] Sep 7, 2026
086baf8
fix(MESHCENT-004): 8 review findings across 6 files
flamingo[bot] Sep 7, 2026
db13959
fix(MESHCENT-004): 8 review findings across 6 files
flamingo[bot] Sep 7, 2026
723daa3
fix(MESHCENT-004): 8 review findings across 6 files
flamingo[bot] Sep 7, 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
7 changes: 6 additions & 1 deletion agents/hashagents.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
/*jshint node: true */
/*jshint strict: false */
/*jshint esversion: 6 */
"use strict";

var fs = require('fs');

@flamingo flamingo Bot Sep 7, 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/hashagents.js lacks jshint/strict-mode header conventions

Added the standard jshint directive block (/*jshint node: true */, /*jshint strict: false */, /*jshint esversion: 6 */) and a "use strict"; declaration at the top of agents/hashagents.js, before the existing var fs = require('fs'); line, matching the convention used in meshcentral.js and amt/amt-wsman.js. No other code was changed.

🤖 Prompt for AI agents 
In agents/hashagents.js around line 1, review and complete this code-review fix: agents/hashagents.js lacks jshint/strict-mode header conventions.
What the draft fix changed: Added the standard jshint directive block (`/*jshint node: true */`, `/*jshint strict: false */`, `/*jshint esversion: 6 */`) and a `"use strict";` declaration at the top of agents/hashagents.js, before the existing `var fs = require('fs');` line, matching the convention used in meshcentral.js and amt/amt-wsman.js. No other code was changed.
Verify the change is correct and complete; do not refactor unrelated code.

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


var agents = {
Expand Down Expand Up @@ -42,4 +47,4 @@ for (var i in agents) {
if (info != null) { agentinfo[agents[i]] = info; }
}
console.log(JSON.stringify(agentinfo, null, 2));
process.exit();
process.exit();
11 changes: 7 additions & 4 deletions exeHandler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,6 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

/*xjslint node: true */

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

🦩 🟠 exeHandler.js jshint block includes non-standard /xjslint/ directives alongside jshint

Removed the three dead /*xjslint ...*/ comment lines at the top of exeHandler.js (lines 17-19), leaving only the active /*jshint node: true */, /*jshint strict: false */, and /*jshint esversion: 6 */ directives intact. This is a self-contained, mechanical deletion; the finding also notes the same typo exists in firebase.js and interceptor.js, but those files are out of scope for this fix.

🤖 Prompt for AI agents 
In exeHandler.js around line 17, review and complete this code-review fix: exeHandler.js jshint block includes non-standard /*xjslint*/ directives alongside jshint.
What the draft fix changed: Removed the three dead `/*xjslint ...*/` comment lines at the top of `exeHandler.js` (lines 17-19), leaving only the active `/*jshint node: true */`, `/*jshint strict: false */`, and `/*jshint esversion: 6 */` directives intact. This is a self-contained, mechanical deletion; the finding also notes the same typo exists in `firebase.js` and `interceptor.js`, but those files are out of scope for this fix.
Verify the change is correct and complete; do not refactor unrelated code.

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

/*xjslint plusplus: true */
/*xjslint maxlen: 256 */
/*jshint node: true */
/*jshint strict: false */
/*jshint esversion: 6 */
Expand Down Expand Up @@ -225,7 +222,12 @@ module.exports.parseWindowsExecutable = function (exePath) {
// Read the authenticode certificate, only one cert (only the first entry)
var hdr = Buffer.alloc(8);
fs.readSync(fd, hdr, 0, hdr.length, retVal.CertificateTableAddress);
retVal.certificate = Buffer.alloc(hdr.readUInt32LE(0));
var certLength = hdr.readUInt32LE(0);
if (certLength > (128 * 1024 * 1024)) { // Sanity bound to guard against malformed/malicious length fields
fs.closeSync(fd);
throw ('certificate table length exceeds maximum allowed size');
}
retVal.certificate = Buffer.alloc(certLength);
fs.readSync(fd, retVal.certificate, 0, retVal.certificate.length, retVal.CertificateTableAddress + hdr.length);
retVal.certificate = retVal.certificate.toString('base64');
retVal.certificateDwLength = hdr.readUInt32LE(0);
Comment on lines 222 to 233

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

🦩 🔵 exeHandler.js parseWindowsExecutable trusts embedded certificate length fields without bounds checking

In parseWindowsExecutable, added a bounds check on the certificate table length read from the untrusted PE header before allocating retVal.certificate: the value is now captured in certLength, checked against a sanity cap of 128MB, and an exception is thrown (after closing the file descriptor) if exceeded, before Buffer.alloc(certLength) is called. The 128MB threshold is a reasonable but somewhat arbitrary bound for legitimate authenticode certificates (which are normally tiny, KB-sized) — a reviewer may want a stricter limit (e.g. a few MB) or to derive the bound from the file size/remaining bytes instead of a fixed constant; this change prevents the most severe OOM/crash cases but the exact threshold is a judgment call.

🤖 Prompt for AI agents 
In exeHandler.js around line 224, review and complete this code-review fix: exeHandler.js parseWindowsExecutable trusts embedded certificate length fields without bounds checking.
What the draft fix changed: In `parseWindowsExecutable`, added a bounds check on the certificate table length read from the untrusted PE header before allocating `retVal.certificate`: the value is now captured in `certLength`, checked against a sanity cap of 128MB, and an exception is thrown (after closing the file descriptor) if exceeded, before `Buffer.alloc(certLength)` is called. The 128MB threshold is a reasonable but somewhat arbitrary bound for legitimate authenticode certificates (which are normally tiny, KB-sized) — a reviewer may want a stricter limit (e.g. a few MB) or to derive the bound from the file size/remaining bytes instead of a fixed constant; this change prevents the most severe OOM/crash cases but the exact threshold is a judgment call.
Verify the change is correct and complete; do not refactor unrelated code.

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

Expand Down Expand Up @@ -308,3 +310,4 @@ module.exports.hashExecutableFile = function (options) {
options.state.source.pipe(options.targetStream);
}
};

7 changes: 5 additions & 2 deletions mcrec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
/* jshint node: true */
/* jshint esversion: 6 */
'use strict';

/**

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

🦩 🟠 mcrec.js missing jshint directives and 'use strict'

Added /* jshint node: true */, /* jshint esversion: 6 */, and 'use strict'; directives at the top of mcrec.js, before the existing JSDoc header comment, matching the convention described for meshaccelerator.js.

🤖 Prompt for AI agents 
In mcrec.js around line 1, review and complete this code-review fix: mcrec.js missing jshint directives and 'use strict'.
What the draft fix changed: Added `/* jshint node: true */`, `/* jshint esversion: 6 */`, and `'use strict';` directives at the top of mcrec.js, before the existing JSDoc header comment, matching the convention described for meshaccelerator.js.
Verify the change is correct and complete; do not refactor unrelated code.

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

* @description MeshCentral MeshAgent
* @author Ylian Saint-Hilaire
Expand Down Expand Up @@ -227,7 +231,6 @@ function readLastBlock(state, func) {
var magic = buf.toString('utf8', 16, 32);
if ((type == 3) && (size == 16) && (magic == 'MeshCentralMCNDX')) {

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

🦩 🟠 readLastBlock in mcrec.js references undefined variable extraMetadata

Removed the stray, unused extraMetadata = null; implicit-global assignment inside readLastBlock (in the if ((type == 3) && (size == 16) && (magic == 'MeshCentralMCNDX')) branch); the actual parsed metadata is still passed to func(...) via buf3 a few lines later, unaffected by this removal. Combined with the newly added 'use strict' from finding 1, any future reintroduction of an undeclared assignment like this would now throw instead of silently creating a global.

🤖 Prompt for AI agents 
In mcrec.js around line 228, review and complete this code-review fix: readLastBlock in mcrec.js references undefined variable `extraMetadata`.
What the draft fix changed: Removed the stray, unused `extraMetadata = null;` implicit-global assignment inside `readLastBlock` (in the `if ((type == 3) && (size == 16) && (magic == 'MeshCentralMCNDX'))` branch); the actual parsed metadata is still passed to `func(...)` via `buf3` a few lines later, unaffected by this removal. Combined with the newly added `'use strict'` from finding 1, any future reintroduction of an undeclared assignment like this would now throw instead of silently creating a global.
Verify the change is correct and complete; do not refactor unrelated code.

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

// Extra metadata present, lets read it.
extraMetadata = null;
var buf2 = Buffer.alloc(16);
fs.read(state.recFile, buf2, 0, 16, time, function (err, bytesRead, buf2) {
var xtype = buf2.readUInt16BE(0); // Type (1 = Header, 2 = Network Data, 3 = End, 4 = Extra Metadata)
Expand Down Expand Up @@ -331,4 +334,4 @@ if (directRun) { setup(); }

// Export table
module.exports.startEx = startEx;
module.exports.indexFile = indexFile;
module.exports.indexFile = indexFile;
5 changes: 1 addition & 4 deletions meshmail.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,6 @@
* @version v0.0.1
*/

/*xjslint node: true */

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

🦩 🟠 meshmail.js uses xjslint hints instead of standard jshint node/strict directives layout

Removed the three legacy /*xjslint ... */ comment lines (node: true, plusplus: true, maxlen: 256) from the file header at the top of meshmail.js, leaving only the /*jshint node: true */, /*jshint strict: false */, /*jshint esversion: 6 */ directives immediately followed by "use strict";, matching the convention described (and used in sibling modules like multiserver.js). No other code was touched.

🤖 Prompt for AI agents 
In meshmail.js around line 9, review and complete this code-review fix: meshmail.js uses xjslint hints instead of standard jshint node/strict directives layout.
What the draft fix changed: Removed the three legacy `/*xjslint ... */` comment lines (`node: true`, `plusplus: true`, `maxlen: 256`) from the file header at the top of meshmail.js, leaving only the `/*jshint node: true */`, `/*jshint strict: false */`, `/*jshint esversion: 6 */` directives immediately followed by `"use strict";`, matching the convention described (and used in sibling modules like multiserver.js). No other code was touched.
Verify the change is correct and complete; do not refactor unrelated code.

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

/*xjslint plusplus: true */
/*xjslint maxlen: 256 */
/*jshint node: true */
/*jshint strict: false */
/*jshint esversion: 6 */
Expand Down Expand Up @@ -777,4 +774,4 @@ module.exports.CreateMeshMail = function (parent, domain) {
}

return obj;
};
};
Loading