Skip to content

fix(MESHCENT-002): 8 review findings across 3 files - #147

Draft
flamingo[bot] wants to merge 3 commits into
masterfrom
ai-fix/meshcent-002-a8c05dec-6542cba8
Draft

fix(MESHCENT-002): 8 review findings across 3 files#147
flamingo[bot] wants to merge 3 commits into
masterfrom
ai-fix/meshcent-002-a8c05dec-6542cba8

Conversation

@flamingo

@flamingo flamingo Bot commented Aug 24, 2026

Copy link
Copy Markdown

Closes 8 review findings across 3 files.

Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.

# Fix confidence Finding Location
1 🟢 92 high multiserver.js obj.ws.send() call in OutPeer authentication handler not wrapped in try/catch multiserver.js:76
2 🟢 92 high Unwrapped ws.send() when replying with certificate + signature in multiserver.js multiserver.js:96
3 🟢 90 high clients[i].publish() call in mqttbroker.js not wrapped in try/catch mqttbroker.js:137
4 🟢 92 high MQTT broker logs raw password contents at debug level mqttbroker.js:27

What 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-da6bc2c6e58e

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

@flamingo flamingo Bot left a comment

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.

🦩 What this fix changed, finding by finding

4 finding(s) fixed in this draft — 4 explained inline on the diff.

Comment thread multiserver.js
Comment on lines 86 to 92

// Start authenticate the peer server by sending a auth nonce & server TLS cert hash.
// Send 384 bits SHA384 hash of TLS cert public key + 384 bits nonce
obj.ws.send(Buffer.from(obj.common.ShortToStr(1) + obj.serverCertHash + obj.nonce, 'binary')); // Command 1, hash + nonce
try { obj.ws.send(Buffer.from(obj.common.ShortToStr(1) + obj.serverCertHash + obj.nonce, 'binary')); } catch (ex) { } // Command 1, hash + nonce
});

// If a message is received

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.

🦩 🔴 multiserver.js obj.ws.send() call in OutPeer authentication handler not wrapped in try/catch

In obj.CreatePeerOutServer's WebSocket 'open' event handler, wrapped the obj.ws.send(Buffer.from(obj.common.ShortToStr(1) + obj.serverCertHash + obj.nonce, 'binary')) call (Command 1, hash + nonce) in a try { ... } catch (ex) { } block, matching the convention used in obj.send.

🤖 Prompt for AI agents
In multiserver.js around line 76, review and complete this code-review fix: multiserver.js obj.ws.send() call in OutPeer authentication handler not wrapped in try/catch.
What the draft fix changed: In `obj.CreatePeerOutServer`'s WebSocket `'open'` event handler, wrapped the `obj.ws.send(Buffer.from(obj.common.ShortToStr(1) + obj.serverCertHash + obj.nonce, 'binary'))` call (Command 1, hash + nonce) in a `try { ... } catch (ex) { }` block, matching the convention used in `obj.send`.
Verify the change is correct and complete; do not refactor unrelated code.

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

Comment thread multiserver.js
Comment on lines 86 to 92

// Start authenticate the peer server by sending a auth nonce & server TLS cert hash.
// Send 384 bits SHA384 hash of TLS cert public key + 384 bits nonce
obj.ws.send(Buffer.from(obj.common.ShortToStr(1) + obj.serverCertHash + obj.nonce, 'binary')); // Command 1, hash + nonce
try { obj.ws.send(Buffer.from(obj.common.ShortToStr(1) + obj.serverCertHash + obj.nonce, 'binary')); } catch (ex) { } // Command 1, hash + nonce
});

// If a message is received

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.

🦩 🔴 Unwrapped ws.send() when replying with certificate + signature in multiserver.js

In obj.CreatePeerOutServer's 'message' handler, inside the acceleratorPerformSignature callback for command 1 processing, wrapped the obj.ws.send(Buffer.from(obj.common.ShortToStr(2) + ...)) call (Command 2, certificate + signature) in a try { ... } catch (ex) { } block, keeping the existing if (obj.ws != null) guard intact.

🤖 Prompt for AI agents
In multiserver.js around line 96, review and complete this code-review fix: Unwrapped ws.send() when replying with certificate + signature in multiserver.js.
What the draft fix changed: In `obj.CreatePeerOutServer`'s `'message'` handler, inside the `acceleratorPerformSignature` callback for command 1 processing, wrapped the `obj.ws.send(Buffer.from(obj.common.ShortToStr(2) + ...))` call (Command 2, certificate + signature) in a `try { ... } catch (ex) { }` block, keeping the existing `if (obj.ws != null)` guard intact.
Verify the change is correct and complete; do not refactor unrelated code.

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

Comment thread mqttbroker.js
}
// Handle messages coming from clients
function handleMessage(nodeid, meshid, domainid, topic, message) {

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.

🦩 🟠 clients[i].publish() call in mqttbroker.js not wrapped in try/catch

In obj.publishNoPeers (mqttbroker.js), wrapped the clients[i].publish(...) call inside the for loop with a try/catch block exactly as suggested, preventing an unhandled exception from a closed client connection from crashing the broker's publish loop.

🤖 Prompt for AI agents
In mqttbroker.js around line 137, review and complete this code-review fix: clients[i].publish() call in mqttbroker.js not wrapped in try/catch.
What the draft fix changed: In obj.publishNoPeers (mqttbroker.js), wrapped the `clients[i].publish(...)` call inside the for loop with a try/catch block exactly as suggested, preventing an unhandled exception from a closed client connection from crashing the broker's publish loop.
Verify the change is correct and complete; do not refactor unrelated code.

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

Comment thread mqttbroker.js
obj.generateLogin = function (meshid, nodeid) {
const meshidsplit = meshid.split('/'), nodeidsplit = nodeid.split('/');
const xmeshid = meshidsplit[2], xnodeid = nodeidsplit[2], xdomainid = meshidsplit[1];
const username = 'MCAuth1:' + xnodeid + ':' + xmeshid + ':' + xdomainid;

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.

🦩 🔵 MQTT broker logs raw password contents at debug level

In aedes.authenticate (mqttbroker.js), removed the ", Pass:" + password.toString() segment from the obj.parent.debug log line, so the plaintext password is no longer written to debug logs while other identifying info (username, ClientID, transport, address) is preserved.

🤖 Prompt for AI agents
In mqttbroker.js around line 27, review and complete this code-review fix: MQTT broker logs raw password contents at debug level.
What the draft fix changed: In aedes.authenticate (mqttbroker.js), removed the `", Pass:" + password.toString()` segment from the obj.parent.debug log line, so the plaintext password is no longer written to debug logs while other identifying info (username, ClientID, transport, address) is preserved.
Verify the change is correct and complete; do not refactor unrelated code.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0 participants