fix(MESHCENT-003-2): 6 review findings in meshdevicefile.js - #146
fix(MESHCENT-003-2): 6 review findings in meshdevicefile.js#146flamingo[bot] wants to merge 1 commit into
Conversation
![flamingo[bot]](https://avatars.githubusercontent.com/in/3426709?s=60&v=4){kind=small}
| 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 |
There was a problem hiding this comment.
🦩 🔴 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
|
|
||
| // 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; |
There was a problem hiding this comment.
🦩 🔴 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
| } | ||
| } 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; |
There was a problem hiding this comment.
🦩 🔴 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
| 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; } | ||
|
|
There was a problem hiding this comment.
🦩 🔴 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 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 |
There was a problem hiding this comment.
🦩 🟠 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
| @@ -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]; | |||
There was a problem hiding this comment.
🦩 🟠 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
Closes 6 review findings in
meshdevicefile.js.Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
meshdevicefile.js:62meshdevicefile.js:143meshdevicefile.js:163meshdevicefile.js:25meshdevicefile.js:84meshdevicefile.js:106What 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.