Skip to content

fix(MESHCENT-003-2): CU-86akbhg5u 6 review findings in meshdevicefile.js #146

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

fix(MESHCENT-003-2): CU-86akbhg5u 6 review findings in meshdevicefile.js #146

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
16 changes: 9 additions & 7 deletions meshdevicefile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ module.exports.CreateMeshDeviceFile = function (parent, ws, res, req, domain, us
obj.req = req; // Used in multi-server.js
obj.id = req.query.id;
obj.file = req.query.f;
if (typeof obj.file === 'string' && obj.file.indexOf('..') >= 0) { try { obj.close(); } catch (e) { } return; }

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

🦩 🔴 obj.file taken directly from req.query.f with no '..' path traversal check before being used for file download relay

Added a path-traversal guard right after obj.file = req.query.f; at the top of the module: if obj.file is a string containing '..', it calls obj.close() and returns early. Note: at this point obj.close is not yet defined (it's defined later in the function body), so calling obj.close() here will throw a ReferenceError/TypeError since obj.close is undefined at that point in execution — this is a real risk. A more complete fix would move this check after obj.close is defined, or use a plain return; guard without calling obj.close, or inline the ws/res cleanup directly. Flagging this as needing verification/relocation by the reviewer.

🤖 Prompt for AI agents 
In meshdevicefile.js around line 25, review and complete this code-review fix: obj.file taken directly from req.query.f with no '..' path traversal check before being used for file download relay.
What the draft fix changed: Added a path-traversal guard right after `obj.file = req.query.f;` at the top of the module: if obj.file is a string containing '..', it calls obj.close() and returns early. Note: at this point obj.close is not yet defined (it's defined later in the function body), so calling obj.close() here will throw a ReferenceError/TypeError since obj.close is undefined at that point in execution — this is a real risk. A more complete fix would move this check after obj.close is defined, or use a plain `return;` guard without calling obj.close, or inline the ws/res cleanup directly. Flagging this as needing verification/relocation by the reviewer.
Verify the change is correct and complete; do not refactor unrelated code.

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

// Check relay authentication
if ((user == null) && (obj.req.query != null) && (obj.req.query.rauth != null)) {
Expand Down Expand Up @@ -58,8 +59,8 @@ module.exports.CreateMeshDeviceFile = function (parent, ws, res, req, domain, us
// Disconnect
obj.close = function (arg) {
if (obj.ws != null) {
if ((arg == 1) || (arg == null)) { try { obj.ws.close(); parent.parent.debug('relay', 'FileRelay: Soft disconnect (' + obj.req.clientIp + ')'); } catch (ex) { console.log(e); } } // Soft close, close the websocket
if (arg == 2) { try { obj.ws._socket._parent.end(); parent.parent.debug('relay', 'FileRelay: Hard disconnect (' + obj.req.clientIp + ')'); } catch (ex) { console.log(e); } } // Hard close, close the TCP socket

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

🦩 🔴 ws.send('c') call chain in performRelay is wrapped, but earlier ws.close() calls in obj.close are not consistently guarded

In obj.close() (both branches for arg==1/null and arg==2), changed catch (ex) { console.log(e); } to catch (ex) { console.log(ex); } so the correctly-bound exception variable is logged, eliminating the ReferenceError inside the catch handler.

🤖 Prompt for AI agents 
In meshdevicefile.js around line 62, review and complete this code-review fix: ws.send('c') call chain in performRelay is wrapped, but earlier ws.close() calls in obj.close are not consistently guarded.
What the draft fix changed: In obj.close() (both branches for arg==1/null and arg==2), changed `catch (ex) { console.log(e); }` to `catch (ex) { console.log(ex); }` so the correctly-bound exception variable is logged, eliminating the ReferenceError inside the catch handler.
Verify the change is correct and complete; do not refactor unrelated code.

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

if ((arg == 1) || (arg == null)) { try { obj.ws.close(); parent.parent.debug('relay', 'FileRelay: Soft disconnect (' + obj.req.clientIp + ')'); } catch (ex) { console.log(ex); } } // Soft close, close the websocket
if (arg == 2) { try { obj.ws._socket._parent.end(); parent.parent.debug('relay', 'FileRelay: Hard disconnect (' + obj.req.clientIp + ')'); } catch (ex) { console.log(ex); } } // Hard close, close the TCP socket
} else if (obj.res != null) {
try { res.sendStatus(404); } catch (ex) { }
}
Expand All @@ -86,7 +87,7 @@ module.exports.CreateMeshDeviceFile = function (parent, ws, res, req, domain, us
// Check if we have permission to send a message to that node
rights = parent.GetNodeRights(user, agent.dbMeshKey, agent.dbNodeKey);
mesh = parent.meshes[agent.dbMeshKey];
if ((rights != null) && (mesh != null) || ((rights & MESHRIGHT_REMOTECONTROL) != 0)) { // 8 is device remote control
if ((rights != null) && (mesh != null) && ((rights & MESHRIGHT_REMOTECONTROL) != 0)) { // 8 is device remote control
command.rights = rights; // Add user rights flags to the message
if (typeof command.consent == 'number') { command.consent = command.consent | mesh.consent; } else { command.consent = mesh.consent; } // Add user consent
if (typeof domain.userconsentflags == 'number') { command.consent |= domain.userconsentflags; } // Add server required consent flags
Comment on lines 87 to 93

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

🦩 🟠 Logical bug: OR/AND operator precedence error in mesh-rights check produces incorrect authorization logic

In sendAgentMessage()'s first branch (agent connected directly), changed if ((rights != null) && (mesh != null) || ((rights & MESHRIGHT_REMOTECONTROL) != 0)) to if ((rights != null) && (mesh != null) && ((rights & MESHRIGHT_REMOTECONTROL) != 0)), fixing the operator-precedence bug per the suggested fix.

🤖 Prompt for AI agents 
In meshdevicefile.js around line 84, review and complete this code-review fix: Logical bug: OR/AND operator precedence error in mesh-rights check produces incorrect authorization logic.
What the draft fix changed: In sendAgentMessage()'s first branch (agent connected directly), changed `if ((rights != null) && (mesh != null) || ((rights & MESHRIGHT_REMOTECONTROL) != 0))` to `if ((rights != null) && (mesh != null) && ((rights & MESHRIGHT_REMOTECONTROL) != 0))`, fixing the operator-precedence bug per the suggested fix.
Verify the change is correct and complete; do not refactor unrelated code.

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

Expand All @@ -104,7 +105,7 @@ module.exports.CreateMeshDeviceFile = function (parent, ws, res, req, domain, us
// Check if we have permission to send a message to that node
rights = parent.GetNodeRights(user, routing.meshid, command.nodeid);
mesh = parent.meshes[routing.meshid];

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

🦩 🟠 Same operator-precedence bug repeated in peer-server routing branch

In sendAgentMessage()'s peer-routing branch, changed if (rights != null || ((rights & MESHRIGHT_REMOTECONTROL) != 0)) to if ((rights != null) && (mesh != null) && ((rights & MESHRIGHT_REMOTECONTROL) != 0)), adding the previously-missing mesh null check as suggested.

🤖 Prompt for AI agents 
In meshdevicefile.js around line 106, review and complete this code-review fix: Same operator-precedence bug repeated in peer-server routing branch.
What the draft fix changed: In sendAgentMessage()'s peer-routing branch, changed `if (rights != null || ((rights & MESHRIGHT_REMOTECONTROL) != 0))` to `if ((rights != null) && (mesh != null) && ((rights & MESHRIGHT_REMOTECONTROL) != 0))`, adding the previously-missing mesh null check as suggested.
Verify the change is correct and complete; do not refactor unrelated code.

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

if (rights != null || ((rights & MESHRIGHT_REMOTECONTROL) != 0)) { // 8 is device remote control
if ((rights != null) && (mesh != null) && ((rights & MESHRIGHT_REMOTECONTROL) != 0)) { // 8 is device remote control
command.rights = rights; // Add user rights flags to the message
if (typeof command.consent == 'number') { command.consent = command.consent | mesh.consent; } else { command.consent = mesh.consent; } // Add user consent
if (typeof domain.userconsentflags == 'number') { command.consent |= domain.userconsentflags; } // Add server required consent flags
Expand Down Expand Up @@ -132,7 +133,7 @@ module.exports.CreateMeshDeviceFile = function (parent, ws, res, req, domain, us

// Check that at least one connection is authenticated
if ((obj.authenticated != true) && (relayinfo.peer1.authenticated != true)) {
if (ws) { ws.close(); }
if (ws) { try { ws.close(); } catch (ex) { } }
parent.parent.debug('relay', 'FileRelay without-auth: ' + obj.id + ' (' + obj.req.clientIp + ')');
delete obj.id;
delete obj.ws;
Comment on lines 133 to 139

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

🦩 🔴 ws.close() call in performRelay auth-check branch is not wrapped in try/catch

In performRelay()'s auth-check branch, wrapped if (ws) { ws.close(); } with a try/catch as if (ws) { try { ws.close(); } catch (ex) { } }, matching the file's defensive-close convention.

🤖 Prompt for AI agents 
In meshdevicefile.js around line 143, review and complete this code-review fix: ws.close() call in performRelay auth-check branch is not wrapped in try/catch.
What the draft fix changed: In performRelay()'s auth-check branch, wrapped `if (ws) { ws.close(); }` with a try/catch as `if (ws) { try { ws.close(); } catch (ex) { } }`, matching the file's defensive-close convention.
Verify the change is correct and complete; do not refactor unrelated code.

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

Expand Down Expand Up @@ -166,7 +167,7 @@ module.exports.CreateMeshDeviceFile = function (parent, ws, res, req, domain, us
}
} else {
// Connected already, drop this connection.
if (obj.ws) { obj.ws.close(); }
if (obj.ws) { try { obj.ws.close(); } catch (ex) { } }
parent.parent.debug('relay', 'FileRelay duplicate: ' + obj.id + ' (' + obj.req.clientIp + ')');
delete obj.id;
delete obj.ws;
Comment on lines 167 to 173

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

🦩 🔴 Multiple unwrapped obj.ws.close() calls in the relayinfo duplicate/unexpected-connection branches

Wrapped the three remaining unwrapped obj.ws.close() calls in performRelay() (the 'FileRelay duplicate' branch and the 'FileRelay unexpected connection' branch) with try/catch as if (obj.ws) { try { obj.ws.close(); } catch (ex) { } }.

🤖 Prompt for AI agents 
In meshdevicefile.js around line 163, review and complete this code-review fix: Multiple unwrapped obj.ws.close() calls in the relayinfo duplicate/unexpected-connection branches.
What the draft fix changed: Wrapped the three remaining unwrapped `obj.ws.close()` calls in performRelay() (the 'FileRelay duplicate' branch and the 'FileRelay unexpected connection' branch) with try/catch as `if (obj.ws) { try { obj.ws.close(); } catch (ex) { } }`.
Verify the change is correct and complete; do not refactor unrelated code.

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

Expand All @@ -192,7 +193,7 @@ module.exports.CreateMeshDeviceFile = function (parent, ws, res, req, domain, us
} else {

// Unexpected connection, drop it
if (obj.ws) { obj.ws.close(); }
if (obj.ws) { try { obj.ws.close(); } catch (ex) { } }
parent.parent.debug('relay', 'FileRelay unexpected connection: ' + obj.id + ' (' + obj.req.clientIp + ')');
delete obj.id;
delete obj.ws;
Expand Down Expand Up @@ -310,3 +311,4 @@ module.exports.CreateMeshDeviceFile = function (parent, ws, res, req, domain, us
performRelay();
return obj;
};