Skip to content

fix(adhoc-sweep-fixes): CU-86akdypw4 16 review findings across 16 files #166

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

fix(adhoc-sweep-fixes): CU-86akdypw4 16 review findings across 16 files #166

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
2935d79
fix(adhoc-sweep-fixes): 16 review findings across 16 files
flamingo[bot] Sep 7, 2026
0e6b0a3
fix(adhoc-sweep-fixes): 16 review findings across 16 files
flamingo[bot] Sep 7, 2026
cf0b72b
fix(adhoc-sweep-fixes): 16 review findings across 16 files
flamingo[bot] Sep 7, 2026
0ec77e0
fix(adhoc-sweep-fixes): 16 review findings across 16 files
flamingo[bot] Sep 7, 2026
ca53f81
fix(adhoc-sweep-fixes): 16 review findings across 16 files
flamingo[bot] Sep 7, 2026
18494df
fix(adhoc-sweep-fixes): 16 review findings across 16 files
flamingo[bot] Sep 7, 2026
f7fa3d4
fix(adhoc-sweep-fixes): 16 review findings across 16 files
flamingo[bot] Sep 7, 2026
0d04c7e
fix(adhoc-sweep-fixes): 16 review findings across 16 files
flamingo[bot] Sep 7, 2026
6790b4a
fix(adhoc-sweep-fixes): 16 review findings across 16 files
flamingo[bot] Sep 7, 2026
639d788
fix(adhoc-sweep-fixes): 16 review findings across 16 files
flamingo[bot] Sep 7, 2026
0f45895
fix(adhoc-sweep-fixes): 16 review findings across 16 files
flamingo[bot] Sep 7, 2026
58fc1a2
fix(adhoc-sweep-fixes): 16 review findings across 16 files
flamingo[bot] Sep 7, 2026
7379487
fix(adhoc-sweep-fixes): 16 review findings across 16 files
flamingo[bot] Sep 7, 2026
edfaad0
fix(adhoc-sweep-fixes): 16 review findings across 16 files
flamingo[bot] Sep 7, 2026
952fe07
fix(adhoc-sweep-fixes): 16 review findings across 16 files
flamingo[bot] Sep 7, 2026
4b3e602
fix(adhoc-sweep-fixes): 16 review findings across 16 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
4 changes: 3 additions & 1 deletion .github/workflows/sync-upstream.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,4 +46,6 @@ jobs:

gh pr create --repo "$GITHUB_REPOSITORY" --base master --head "$BRANCH" \
--title "Sync from Fork" \
--body "Automatic weekly sync from \`Ylianst/MeshCentral@master\`."
--body "Automatic weekly sync from \`Ylianst/MeshCentral@master\`. **This PR pulls unreviewed third-party upstream history and requires maintainer review and explicit approval before merging — do not enable auto-merge.**"

</br>
Comment on lines 46 to +51

@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.

🦩 🟠 sync-upstream.yml grants contents:write and pulls arbitrary upstream commits into an auto-created PR without integrity verification

In the Sync step of the sync job, appended an explicit warning to the auto-generated PR body (gh pr create ... --body) stating the PR contains unreviewed third-party upstream history and requires maintainer review/approval before merging, and instructing not to enable auto-merge. This is a lightweight mitigation that increases reviewer awareness but does NOT technically enforce a review gate (e.g., branch protection requiring approvals, CODEOWNERS, or blocking auto-merge) and does NOT pin/verify upstream provenance (no commit signature check, no allow-listed tag/commit range — the workflow still does git fetch upstream master and merges the moving master branch directly). A complete fix would additionally require: (a) branch protection rules on the target branch requiring human approval before merge (configured outside this file, in repo settings), (b) switching from tracking upstream/master to a pinned, manually-reviewed tag/commit SHA that's bumped deliberately, and/or (c) verifying upstream commit signatures (e.g., git verify-commit) before merging. These are out of scope for a single-file workflow YAML change without redesigning the sync strategy, so this fix only reduces — not eliminates — the risk described in the finding.

🤖 Prompt for AI agents 
In .github/workflows/sync-upstream.yml around line 30, review and complete this code-review fix: sync-upstream.yml grants contents:write and pulls arbitrary upstream commits into an auto-created PR without integrity verification.
What the draft fix changed: In the `Sync` step of the `sync` job, appended an explicit warning to the auto-generated PR body (`gh pr create ... --body`) stating the PR contains unreviewed third-party upstream history and requires maintainer review/approval before merging, and instructing not to enable auto-merge. This is a lightweight mitigation that increases reviewer awareness but does NOT technically enforce a review gate (e.g., branch protection requiring approvals, CODEOWNERS, or blocking auto-merge) and does NOT pin/verify upstream provenance (no commit signature check, no allow-listed tag/commit range — the workflow still does `git fetch upstream master` and merges the moving `master` branch directly). A complete fix would additionally require: (a) branch protection rules on the target branch requiring human approval before merge (configured outside this file, in repo settings), (b) switching from tracking `upstream/master` to a pinned, manually-reviewed tag/commit SHA that's bumped deliberately, and/or (c) verifying upstream commit signatures (e.g., `git verify-commit`) before merging. These are out of scope for a single-file workflow YAML change without redesigning the sync strategy, so this fix only reduces — not eliminates — the risk described in the finding.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.

fix confidence: 🔴 45 low — review closely — react 👍/👎 to teach the reviewer

45 changes: 44 additions & 1 deletion agents/meshcore_diagnostic.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,29 @@ function ConfigureAgent(agent)
}
}

function validateDownloadedBinary(path)
{
try
{
var stat = require('fs').statSync(path);
if (stat == null || stat.size <= 0)
{
return (false);
}
// Verify the downloaded file is a valid, signed executable before it is
// registered as a privileged system service.
if (require('MeshAgent').isSignatureValid != null)
{
return (require('MeshAgent').isSignatureValid(path) ? true : false);
}
return (true);
}
catch (e)
{
return (false);
}
}

function start()
{
sendServerLog('Diagnostic: Start');
Comment on lines 126 to 154

@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.

🦩 🟠 meshcore_diagnostic.js downloads and installs an agent binary without validating it before granting it a service, and cleans up asynchronously without confirming service creation succeeded

In start(), added a validateDownloadedBinary() helper (statSync size check plus an optional call to require('MeshAgent').isSignatureValid() if that API exists) invoked before installService is called in both the no-existing-service branch and the existing-but-not-running branch. The temp file is now only deleted after either failing validation (immediate cleanup + giveup) or after installService/ConfigureAgent succeed (synchronous cleanup still happens right after ConfigureAgent, and installService's return value is now null-checked before configuring). This reduces but does not eliminate the risk: I do not have visibility into whether MeshAgent actually exposes a isSignatureValid-style API in this codebase, so if it does not exist the validation silently degrades to only a non-empty-file check, which is NOT a real integrity/signature verification. A complete fix requires wiring in the project's actual binary/signature verification primitive (e.g. checking against a pinned public key or hash delivered out-of-band from the same untrusted channel), and possibly making DownloadAgentBinary itself reject on rejectUnauthorized: false for this privileged path — that part of the finding (MITM via disabled TLS verification) is not addressed here since fixing it could break legitimate self-signed-cert deployments and needs maintainer input on the correct trust model.

🤖 Prompt for AI agents 
In agents/meshcore_diagnostic.js around line 87, review and complete this code-review fix: meshcore_diagnostic.js downloads and installs an agent binary without validating it before granting it a service, and cleans up asynchronously without confirming service creation succeeded.
What the draft fix changed: In `start()`, added a `validateDownloadedBinary()` helper (statSync size check plus an optional call to `require('MeshAgent').isSignatureValid()` if that API exists) invoked before `installService` is called in both the no-existing-service branch and the existing-but-not-running branch. The temp file is now only deleted after either failing validation (immediate cleanup + giveup) or after `installService`/`ConfigureAgent` succeed (synchronous cleanup still happens right after `ConfigureAgent`, and `installService`'s return value is now null-checked before configuring). This reduces but does not eliminate the risk: I do not have visibility into whether `MeshAgent` actually exposes a `isSignatureValid`-style API in this codebase, so if it does not exist the validation silently degrades to only a non-empty-file check, which is NOT a real integrity/signature verification. A complete fix requires wiring in the project's actual binary/signature verification primitive (e.g. checking against a pinned public key or hash delivered out-of-band from the same untrusted channel), and possibly making `DownloadAgentBinary` itself reject on `rejectUnauthorized: false` for this privileged path — that part of the finding (MITM via disabled TLS verification) is not addressed here since fixing it could break legitimate self-signed-cert deployments and needs maintainer input on the correct trust model.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.

fix confidence: 🔴 35 low — review closely — react 👍/👎 to teach the reviewer

Expand All @@ -139,6 +162,13 @@ function start()
// SUCCESS
try
{
if (!validateDownloadedBinary('agent_temporary.bin'))
{
sendServerLog('Diagnostic: Downloaded agent binary failed validation');
try { require('fs').unlinkSync('agent_temporary.bin'); } catch (e2) { }
giveup();
return;
}
var agent = require('service-manager').manager.installService(
{
name: process.platform == 'win32' ? 'Mesh Agent' : 'meshagent',
Expand All @@ -148,8 +178,14 @@ function start()
servicePath: 'agent_temporary.bin',
startType: 'DEMAND_START'
});
require('fs').unlinkSync('agent_temporary.bin');
if (agent == null)
{
try { require('fs').unlinkSync('agent_temporary.bin'); } catch (e3) { }
giveup();
return;
}
ConfigureAgent(agent);
require('fs').unlinkSync('agent_temporary.bin');
}
catch(e)
{
Expand Down Expand Up @@ -185,6 +221,12 @@ function start()
DownloadAgentBinary(s.appLocation()).then(
function () {
sendServerLog('Diagnostic: Downloaded Successfully');
if (!validateDownloadedBinary(s.appLocation()))
{
sendServerLog('Diagnostic: Downloaded agent binary failed validation');
giveup();
return;
}
sendServerLog('Diagnostic: Attempting to start Mesh Agent');
s.start();
sendServerLog('Diagnostic: ' + (s.isRunning() ? '(SUCCESS)' : '(FAILED)'));
Expand All @@ -204,3 +246,4 @@ function start()
}
}
};

4 changes: 2 additions & 2 deletions agents/modules_meshcmd/amt-apfclient.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ function CreateAPFClient(parent, args) {
obj.onSecureConnect = function onSecureConnect(resp, ws, head) {
Debug("APF Secure WebSocket connected.");
//console.log(JSON.stringify(resp));
obj.forwardClient.tag = { accumulator: [] };

@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.

🦩 🟠 amt-apfclient.js accumulator uses += on array/string mismatch (tag.accumulator initialized as array, appended as string)

In obj.onSecureConnect, changed obj.forwardClient.tag = { accumulator: [] }; to obj.forwardClient.tag = { accumulator: '' };. This makes the initializer's declared type match the actual runtime type produced by the += string concatenation in the subsequent data event handler and consumed by .charCodeAt()/.slice()/.substring() calls throughout ProcessData, eliminating the silent array-to-string coercion.

🤖 Prompt for AI agents 
In agents/modules_meshcmd/amt-apfclient.js around line 147, review and complete this code-review fix: amt-apfclient.js accumulator uses += on array/string mismatch (tag.accumulator initialized as array, appended as string).
What the draft fix changed: In `obj.onSecureConnect`, changed `obj.forwardClient.tag = { accumulator: [] };` to `obj.forwardClient.tag = { accumulator: '' };`. This makes the initializer's declared type match the actual runtime type produced by the `+=` string concatenation in the subsequent `data` event handler and consumed by `.charCodeAt()`/`.slice()`/`.substring()` calls throughout `ProcessData`, eliminating the silent array-to-string coercion.
Verify the change is correct and complete; do not refactor unrelated code.

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

obj.forwardClient.tag = { accumulator: '' };
obj.forwardClient.ws = ws;
obj.forwardClient.ws.on('end', function () {
Debug("APF: Connection is closing.");
Expand Down Expand Up @@ -456,4 +456,4 @@ function CreateAPFClient(parent, args) {
return obj;
}

module.exports = CreateAPFClient;
module.exports = CreateAPFClient;
4 changes: 2 additions & 2 deletions agents/modules_meshcmd/amt-mei.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ function amt_heci()

// Fill the left with zeros until the string is of a given length
function zeroLeftPad(str, len) {

@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.

🦩 🟠 amt-mei.js zeroLeftPad has broken guard logic due to operator precedence, allowing null length to bypass early return

In zeroLeftPad (agents/modules_meshcmd/amt-mei.js), changed the guard condition on the line if ((len == null) && (typeof (len) != 'number')) { return null; } to use || instead of &&, matching the intended logic stated in the finding: if ((len == null) || (typeof(len) != 'number')) { return null; }. This ensures the function returns null early both when len is null/undefined and when len is any non-numeric value (e.g., a string), preventing the silent NaN-arithmetic loop bug described in the finding. All call sites pass numeric literals for len, so this tightening of the guard does not change behavior for existing callers.

🤖 Prompt for AI agents 
In agents/modules_meshcmd/amt-mei.js around line 163, review and complete this code-review fix: amt-mei.js zeroLeftPad has broken guard logic due to operator precedence, allowing null length to bypass early return.
What the draft fix changed: In `zeroLeftPad` (agents/modules_meshcmd/amt-mei.js), changed the guard condition on the line `if ((len == null) && (typeof (len) != 'number')) { return null; }` to use `||` instead of `&&`, matching the intended logic stated in the finding: `if ((len == null) || (typeof(len) != 'number')) { return null; }`. This ensures the function returns null early both when `len` is null/undefined and when `len` is any non-numeric value (e.g., a string), preventing the silent NaN-arithmetic loop bug described in the finding. All call sites pass numeric literals for `len`, so this tightening of the guard does not change behavior for existing callers.
Verify the change is correct and complete; do not refactor unrelated code.

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

if ((len == null) && (typeof (len) != 'number')) { return null; }
if ((len == null) || (typeof (len) != 'number')) { return null; }
if (str == null) str = ''; // If null, this is to generate zero leftpad string
var zlp = '';
for (var i = 0; i < len - str.length; i++) { zlp += '0'; }
Expand Down Expand Up @@ -496,4 +496,4 @@ AMT_STATUS_RNG_NOT_READY = 48,
AMT_STATUS_CERTIFICATE_NOT_READY = 49,
AMT_STATUS_INVALID_HANDLE = 2053
AMT_STATUS_NOT_FOUND = 2068,
*/
*/
5 changes: 3 additions & 2 deletions agents/modules_meshcore/computer-identifiers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,10 +89,10 @@ function linux_identifiers()
}
} catch (xx) { }
} else {
throw('Unknown board');

@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.

🦩 🟠 linux_identifiers() throws bare strings instead of Error objects

In linux_identifiers(), replaced throw('Unknown board'); with throw (new Error('Unknown board')); and throw ('this platform does not have DMI statistics'); with throw (new Error('this platform does not have DMI statistics'));. Both throws now produce proper Error objects so callers relying on e.message/e.stack get correct values. No other throw sites (e.g. the unrelated default-platform throw ('Unsupported Platform') outside this function) were touched since the finding scoped this to linux_identifiers().

🤖 Prompt for AI agents 
In agents/modules_meshcore/computer-identifiers.js around line 92, review and complete this code-review fix: linux_identifiers() throws bare strings instead of Error objects.
What the draft fix changed: In `linux_identifiers()`, replaced `throw('Unknown board');` with `throw (new Error('Unknown board'));` and `throw ('this platform does not have DMI statistics');` with `throw (new Error('this platform does not have DMI statistics'));`. Both throws now produce proper Error objects so callers relying on `e.message`/`e.stack` get correct values. No other throw sites (e.g. the unrelated default-platform `throw ('Unsupported Platform')` outside this function) were touched since the finding scoped this to `linux_identifiers()`.
Verify the change is correct and complete; do not refactor unrelated code.

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

throw (new Error('Unknown board'));
}
} else {
throw ('this platform does not have DMI statistics');
throw (new Error('this platform does not have DMI statistics'));
}
} else {
var entries = require('fs').readdirSync('/sys/class/dmi/id');
Expand Down Expand Up @@ -900,3 +900,4 @@ module.exports.isVM = function isVM()
// board_serial = BASEBOARD->SerialNumber = ioreg/serial-number | ioreg/IOPlatformSerialNumber
// board_vendor = BASEBOARD->Manufacturer = ioreg/manufacturer
// board_version = BASEBOARD->Version

9 changes: 1 addition & 8 deletions agents/testsuite.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,13 +110,6 @@ function parseUrl(url) {
sha256.write('bob');
sha256.end();
}
{

@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/testsuite.js contains a duplicate/dead SHA256Stream test block explicitly marked FAIL

Removed the duplicate, known-broken SHA256Stream test block (the sha256x block prefixed with // FAIL!!!!!!!!!) located immediately after the first "Test 1: SHA256 hashing" block. The original, working test block using sha256 remains untouched; only the dead/duplicate block and its blank separator line were deleted.

🤖 Prompt for AI agents 
In agents/testsuite.js around line 113, review and complete this code-review fix: agents/testsuite.js contains a duplicate/dead SHA256Stream test block explicitly marked FAIL.
What the draft fix changed: Removed the duplicate, known-broken SHA256Stream test block (the `sha256x` block prefixed with `// FAIL!!!!!!!!!`) located immediately after the first "Test 1: SHA256 hashing" block. The original, working test block using `sha256` remains untouched; only the dead/duplicate block and its blank separator line were deleted.
Verify the change is correct and complete; do not refactor unrelated code.

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

// FAIL!!!!!!!!!
var sha256x = require('SHA256Stream');
sha256x.hashString = function (x) { if (x == '81B637D8FCD2C6DA6359E6963113A1170DE795E4B725B84D1E0B4CFD9EC58CE9') { console.log('Test 1 - OK: ' + x); } else { console.log('Test 1 - FAIL: ' + x); } };
sha256x.write('bob');
sha256x.end();
}

/*
{
Expand Down Expand Up @@ -154,4 +147,4 @@ function parseUrl(url) {
}

console.log('--- Tests Completed ---');
process.exit(2);
process.exit(2);
Loading