fix(adhoc-sweep-fixes): CU-86akdypw4 36 review findings across 19 files - #165
fix(adhoc-sweep-fixes): CU-86akdypw4 36 review findings across 19 files#165flamingo[bot] wants to merge 19 commits into
Conversation
![flamingo[bot]](https://avatars.githubusercontent.com/in/3426709?s=60&v=4){kind=small}
| require('../node-forge/lib/pkcs7asn1'); | ||
| require('../node-forge/lib/random'); | ||
| require('../node-forge/lib/util'); | ||
| require('../node-forge/lib/x509'); f |
There was a problem hiding this comment.
🦩 🔴 Stray trailing 'f' identifier after require() call breaks pkcs7-modified.js loading path
Removed the stray trailing f identifier after require('../node-forge/lib/x509'); inside the initial try block at the top of the file. This was a syntax/runtime typo causing a ReferenceError that forced the code into the catch block, making the primary require path dead code. The line now reads exactly require('../node-forge/lib/x509'); with nothing following it, matching the suggested fix, so the primary try block can now succeed and properly bypass the fallback require('./node_modules/...') path.
🤖 Prompt for AI agents
In pkcs7-modified.js around line 30, review and complete this code-review fix: Stray trailing 'f' identifier after require() call breaks pkcs7-modified.js loading path.
What the draft fix changed: Removed the stray trailing `f` identifier after `require('../node-forge/lib/x509');` inside the initial `try` block at the top of the file. This was a syntax/runtime typo causing a `ReferenceError` that forced the code into the `catch` block, making the primary require path dead code. The line now reads exactly `require('../node-forge/lib/x509');` with nothing following it, matching the suggested fix, so the primary try block can now succeed and properly bypass the fallback `require('./node_modules/...')` path.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 97 high — react 👍/👎 to teach the reviewer
| @@ -64,7 +64,9 @@ function AMTScanner() { | |||
| if (masknum <= 16 || masknum > 32) return null; | |||
There was a problem hiding this comment.
🦩 🟠 amt-scanner.js: incorrect min bound off-by-one and dead variable in parseIPv4Range CIDR branch
Fixed the off-by-one/dead-min-max-bound bug in parseIPv4Range's CIDR branch (inside AMTScanner). Replaced the unconditional +1/-1 network/broadcast exclusion with computed netmin/netmax that are only adjusted (excluding network/broadcast addresses) when doing so keeps netmin < netmax, preventing min > max for /31, /32, or small ranges. For a /32 (mask=0) or /31 (mask=1) the range now falls back to including the full computed range instead of producing an invalid inverted bound, so the scan loop in scan() will execute instead of silently doing nothing.
🤖 Prompt for AI agents
In agents/modules_meshcmd/amt-scanner.js around line 64, review and complete this code-review fix: amt-scanner.js: incorrect min bound off-by-one and dead variable in parseIPv4Range CIDR branch.
What the draft fix changed: Fixed the off-by-one/dead-min-max-bound bug in `parseIPv4Range`'s CIDR branch (inside `AMTScanner`). Replaced the unconditional `+1`/`-1` network/broadcast exclusion with computed `netmin`/`netmax` that are only adjusted (excluding network/broadcast addresses) when doing so keeps `netmin < netmax`, preventing `min > max` for /31, /32, or small ranges. For a /32 (mask=0) or /31 (mask=1) the range now falls back to including the full computed range instead of producing an invalid inverted bound, so the scan loop in `scan()` will execute instead of silently doing nothing.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 70 medium — react 👍/👎 to teach the reviewer
| var server = this.dgram.createSocket({ type: 'udp4' }); | ||
| server.parent = this; | ||
| server.scanResults = []; | ||
| server.on('error', function (err) { console.log('Error:' + err); }); | ||
| server.on('error', function (err) { | ||
| console.log('Error:' + err); | ||
| clearTimeout(tmout); | ||
| try { server.close(); } catch (e) { } | ||
| if (callback) { | ||
| callback(server.scanResults); | ||
| } | ||
| server.parent.emit('found', server.scanResults); | ||
| }); | ||
| server.on('message', function (msg, rinfo) { if (rinfo.size > 4) { this.parent.parseRmcpPacket(this, msg, rinfo, function (s, res) { s.scanResults.push(res); }) }; }); | ||
| server.on('listening', function () { for (var i = iprange.min; i <= iprange.max; i++) { | ||
| server.send(rmcp, 623, server.parent.IPv4NumToStr(i)); } }); |
There was a problem hiding this comment.
🦩 🟠 amt-scanner.js scan() uses delete server on a local variable, which has no effect and leaks socket reference
Removed the no-op delete server; at the end of the setTimeout callback in scan() and replaced it with server = null;, which actually clears the local closure variable's reference to the socket object after close()/callback/emit have run, aiding garbage collection of the socket. This does not change any externally observable behavior (close() already released the OS resource) but eliminates the misleading dead statement.
🤖 Prompt for AI agents
In agents/modules_meshcmd/amt-scanner.js around line 97, review and complete this code-review fix: amt-scanner.js scan() uses `delete server` on a local variable, which has no effect and leaks socket reference.
What the draft fix changed: Removed the no-op `delete server;` at the end of the `setTimeout` callback in `scan()` and replaced it with `server = null;`, which actually clears the local closure variable's reference to the socket object after `close()`/callback/emit have run, aiding garbage collection of the socket. This does not change any externally observable behavior (close() already released the OS resource) but eliminates the misleading dead statement.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 60 medium — react 👍/👎 to teach the reviewer
| var server = this.dgram.createSocket({ type: 'udp4' }); | ||
| server.parent = this; | ||
| server.scanResults = []; | ||
| server.on('error', function (err) { console.log('Error:' + err); }); |
There was a problem hiding this comment.
🦩 🔵 amt-scanner.js: server.on('error') handler only logs, never invokes callback — scan hangs on socket error
Updated the server.on('error', ...) handler in scan() to, in addition to logging, clear the pending timeout (clearTimeout(tmout)), attempt to close the socket, invoke callback(server.scanResults) if provided, and emit 'found' with the (likely empty) results — mirroring the normal completion path so callers are notified promptly on socket error instead of waiting out the full timeout. Risk/incompleteness: tmout is declared with var after this handler is registered but before bind() is called synchronously so it should be defined by the time an async 'error' event fires; however, if error fires synchronously during bind() before tmout is assigned, clearTimeout(undefined) is a harmless no-op, so behavior remains safe but this ordering was not restructured further to keep the diff minimal.
🤖 Prompt for AI agents
In agents/modules_meshcmd/amt-scanner.js around line 92, review and complete this code-review fix: amt-scanner.js: server.on('error') handler only logs, never invokes callback — scan hangs on socket error.
What the draft fix changed: Updated the `server.on('error', ...)` handler in `scan()` to, in addition to logging, clear the pending timeout (`clearTimeout(tmout)`), attempt to close the socket, invoke `callback(server.scanResults)` if provided, and emit `'found'` with the (likely empty) results — mirroring the normal completion path so callers are notified promptly on socket error instead of waiting out the full timeout. Risk/incompleteness: `tmout` is declared with `var` after this handler is registered but before `bind()` is called synchronously so it should be defined by the time an async 'error' event fires; however, if `error` fires synchronously during `bind()` before `tmout` is assigned, `clearTimeout(undefined)` is a harmless no-op, so behavior remains safe but this ordering was not restructured further to keep the diff minimal.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 40 low — review closely — react 👍/👎 to teach the reviewer
|
|
||
| var protocol = host.startsWith('http://') ? 'ws' : 'wss'; | ||
| var cleanHost = host.replace(/^https?:\/\//, '').replace(/^wss?:\/\//, ''); | ||
|
|
||
| // Validate the cleaned host against a strict allowlist pattern before it is embedded | ||
| // into the generated agent config. Rejects control characters, newlines, and any | ||
| // value that isn't a plain hostname/IPv4 with an optional port, preventing both | ||
| // agent-redirection (SSRF-like) and MSH config injection via crafted `host` values. | ||
| if (!HOST_PATTERN.test(cleanHost)) { | ||
| return sendError(res, 400, 'Invalid host parameter'); | ||
| } | ||
|
|
||
| var meshServerUrl = protocol + '://' + cleanHost + '/ws/tools/agent/meshcentral-server/agent.ashx'; | ||
|
|
||
| var mshContent = [ |
There was a problem hiding this comment.
🦩 🟠 /generate-msh host parameter used to build WebSocket URL without validation, enabling potential SSRF-like agent redirection
In the /generate-msh handler (plugins/openframe.js), added a HOST_PATTERN regex constant and a validation check right after cleanHost is derived: if cleanHost doesn't match a strict hostname/IPv4(:port) pattern, the route now responds with 400 "Invalid host parameter" before building meshServerUrl. This constrains the value embedded into the generated MeshServer URL to a safe charset/structure, mitigating (not eliminating, since it still permits arbitrary allowlisted-format hostnames) the SSRF-like redirection risk. A complete fix would additionally require an actual allowlist of legitimate MeshServer hosts, which needs operational/config input not available in this file.
🤖 Prompt for AI agents
In plugins/openframe.js around line 62, review and complete this code-review fix: /generate-msh host parameter used to build WebSocket URL without validation, enabling potential SSRF-like agent redirection.
What the draft fix changed: In the /generate-msh handler (plugins/openframe.js), added a `HOST_PATTERN` regex constant and a validation check right after `cleanHost` is derived: if `cleanHost` doesn't match a strict hostname/IPv4(:port) pattern, the route now responds with 400 "Invalid host parameter" before building `meshServerUrl`. This constrains the value embedded into the generated MeshServer URL to a safe charset/structure, mitigating (not eliminating, since it still permits arbitrary allowlisted-format hostnames) the SSRF-like redirection risk. A complete fix would additionally require an actual allowlist of legitimate MeshServer hosts, which needs operational/config input not available in this file.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 70 medium — react 👍/👎 to teach the reviewer
| var port = 16994; | ||
| if (node.intelamt.tls > 0) port = 16995; // This is a direct connection, use TLS when possible | ||
|
|
||
| // Record the expected certificate fingerprint (if known) so we can verify it once the TLS handshake completes, | ||
| // since rejectUnauthorized is disabled below to allow AMT's self-signed firmware certificates. | ||
| obj.xtlsFingerprint = (node.intelamt.mpsCert && node.intelamt.mpsCert.fingerprint) ? node.intelamt.mpsCert.fingerprint : ((node.intelamt.tlsFingerprint) ? node.intelamt.tlsFingerprint : 0); | ||
|
|
||
| if (node.intelamt.tls != 1) { | ||
| // If this is TCP (without TLS) set a normal TCP socket | ||
| obj.forwardclient = new obj.net.Socket(); |
There was a problem hiding this comment.
🦩 🟠 AMT device blocklist check missing before establishing direct TLS connection with rejectUnauthorized: false
In obj.Start's direct-connect branch (around the if ((conn & 4) != 0) block), added obj.xtlsFingerprint capture from node.intelamt.mpsCert/tlsFingerprint (best-effort field names, since the actual schema field used elsewhere for this purpose is not visible in this file) and set obj.xtls = true in the TLS connect callback before calling obj.xxOnSocketConnected(). Modified obj.xxOnSocketConnected to retrieve the peer certificate via obj.forwardclient.getPeerCertificate() (falling back to obj.socket) and compare its fingerprint against obj.xtlsFingerprint when one is set, calling obj.Stop() on mismatch, mirroring the existing CIRA path's fingerprint check. RISK: the exact field name on node.intelamt that stores the expected/trusted AMT TLS fingerprint is not visible in this file and may not match mpsCert.fingerprint/tlsFingerprint — if the real field differs, obj.xtlsFingerprint will remain 0/falsy and the check becomes a no-op (same as before, but without erroring). A complete fix requires locating the actual DB field/config used elsewhere in the codebase (e.g. device group or node TLS pinning settings) to populate obj.xtlsFingerprint correctly, and possibly rejecting the connection entirely (rather than silently trusting) when no fingerprint is on file, which is a policy decision beyond this file's scope. rejectUnauthorized: false was left unchanged since removing it is a larger behavioral/architectural change affecting all AMT self-signed cert deployments.
🤖 Prompt for AI agents
In amt/amt-redir-mesh.js around line 233, review and complete this code-review fix: AMT device blocklist check missing before establishing direct TLS connection with rejectUnauthorized: false.
What the draft fix changed: In `obj.Start`'s direct-connect branch (around the `if ((conn & 4) != 0)` block), added `obj.xtlsFingerprint` capture from `node.intelamt.mpsCert`/`tlsFingerprint` (best-effort field names, since the actual schema field used elsewhere for this purpose is not visible in this file) and set `obj.xtls = true` in the TLS `connect` callback before calling `obj.xxOnSocketConnected()`. Modified `obj.xxOnSocketConnected` to retrieve the peer certificate via `obj.forwardclient.getPeerCertificate()` (falling back to `obj.socket`) and compare its fingerprint against `obj.xtlsFingerprint` when one is set, calling `obj.Stop()` on mismatch, mirroring the existing CIRA path's fingerprint check. RISK: the exact field name on `node.intelamt` that stores the expected/trusted AMT TLS fingerprint is not visible in this file and may not match `mpsCert.fingerprint`/`tlsFingerprint` — if the real field differs, `obj.xtlsFingerprint` will remain `0`/falsy and the check becomes a no-op (same as before, but without erroring). A complete fix requires locating the actual DB field/config used elsewhere in the codebase (e.g. device group or node TLS pinning settings) to populate `obj.xtlsFingerprint` correctly, and possibly rejecting the connection entirely (rather than silently trusting) when no fingerprint is on file, which is a policy decision beyond this file's scope. `rejectUnauthorized: false` was left unchanged since removing it is a larger behavioral/architectural change affecting all AMT self-signed cert deployments.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 30 low — review closely — react 👍/👎 to teach the reviewer
| // Older NodeJS does not support the keyword "class", so we do without using this syntax | ||
| // TODO: Validate that it's the same as above and that it works. | ||
| // TODO: This is duplicated in apprelays.js as well, consider extracting into a shared module. | ||
| function SerialTunnel(options) { |
There was a problem hiding this comment.
🦩 🔵 SerialTunnel helper duplicated verbatim between amt-redir-mesh.js and apprelays.js
Added a one-line TODO comment above the SerialTunnel function definition noting the duplication with apprelays.js and recommending extraction to a shared module. No functional/behavioral change was made (per the "no refactors" rule, actually extracting the shared module would touch apprelays.js too, which is out of scope for a single-file fix); the console.err typo and duplication itself remain, since fixing them would require editing apprelays.js as well and altering shared behavior across files.
🤖 Prompt for AI agents
In amt/amt-redir-mesh.js around line 80, review and complete this code-review fix: SerialTunnel helper duplicated verbatim between amt-redir-mesh.js and apprelays.js.
What the draft fix changed: Added a one-line TODO comment above the `SerialTunnel` function definition noting the duplication with `apprelays.js` and recommending extraction to a shared module. No functional/behavioral change was made (per the "no refactors" rule, actually extracting the shared module would touch `apprelays.js` too, which is out of scope for a single-file fix); the console.err typo and duplication itself remain, since fixing them would require editing `apprelays.js` as well and altering shared behavior across files.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 40 low — review closely — react 👍/👎 to teach the reviewer
| console.log('WebSocket Message', e); | ||
| if ((obj.state = 1) && (e.data == 'c')) { | ||
| obj.state = 2; | ||
| if (obj.onStateChanged) { onStateChanged(obj, 2); } |
There was a problem hiding this comment.
🦩 🟠 Undefined reference to onStateChanged instead of obj.onStateChanged in relay.js
In obj.connect's onmessage, onclose handlers and the end of obj.connect itself, changed the three bare onStateChanged(obj, ...) calls to obj.onStateChanged(obj, ...), matching the if (obj.onStateChanged) guard checks and eliminating the undefined global reference ReferenceError.
🤖 Prompt for AI agents
In public/samples/relay.js around line 24, review and complete this code-review fix: Undefined reference to onStateChanged instead of obj.onStateChanged in relay.js.
What the draft fix changed: In `obj.connect`'s `onmessage`, `onclose` handlers and the end of `obj.connect` itself, changed the three bare `onStateChanged(obj, ...)` calls to `obj.onStateChanged(obj, ...)`, matching the `if (obj.onStateChanged)` guard checks and eliminating the undefined global reference ReferenceError.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| const publicKeyDer = self.security_interface.gss_unwrapex(derBuffer); | ||
|
|
||
| // Check that the public key is identical except the first byte which is the DER encoding type. | ||
| if (!this.ntlm.publicKeyDer.slice(1).equals(publicKeyDer.slice(1))) { console.log('RDP man-in-the-middle detected.'); close(); return; } |
There was a problem hiding this comment.
🦩 🟠 nla.js man-in-the-middle detection failure only logs and calls undefined close()
In NLA.prototype.recvData (state 2 branch, public key comparison block), changed the undefined close() call to this.close(), which correctly invokes the existing NLA.prototype.close method (defined below, calling this.transport.close()). This fixes the ReferenceError so a detected MITM now actually closes the transport connection instead of crashing. Note: after this.close() the function still returns without emitting any error/close event to the caller beyond what transport.close() triggers internally (via existing 'close' listener wiring), which matches the existing close-path behavior used elsewhere in this file.
🤖 Prompt for AI agents
In rdp/protocol/nla.js around line 132, review and complete this code-review fix: nla.js man-in-the-middle detection failure only logs and calls undefined close().
What the draft fix changed: In `NLA.prototype.recvData` (state 2 branch, public key comparison block), changed the undefined `close()` call to `this.close()`, which correctly invokes the existing `NLA.prototype.close` method (defined below, calling `this.transport.close()`). This fixes the ReferenceError so a detected MITM now actually closes the transport connection instead of crashing. Note: after `this.close()` the function still `return`s without emitting any error/close event to the caller beyond what `transport.close()` triggers internally (via existing 'close' listener wiring), which matches the existing close-path behavior used elsewhere in this file.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 92 high — react 👍/👎 to teach the reviewer
| // Chrome & Firefox (Draft) | ||
| fileReaderInuse = true; | ||
| fileReader.readAsBinaryString(new Blob([e.data])); | ||
| } else if (f.readAsArrayBuffer) { | ||
| } else if (fileReader.readAsArrayBuffer) { | ||
| // Chrome & Firefox (Spec) | ||
| fileReaderInuse = true; | ||
| fileReader.readAsArrayBuffer(e.data); |
There was a problem hiding this comment.
🦩 🟠 ReferenceError: undefined variable 'f' in agent-redir-rtc-0.1.0.js fallback branch
In obj.xxOnMessage, changed the else-if condition f.readAsArrayBuffer to fileReader.readAsArrayBuffer, matching the declared fileReader variable (from var fileReader = new FileReader();) so the branch no longer throws a ReferenceError on engines that reach it.
🤖 Prompt for AI agents
In public/scripts/agent-redir-rtc-0.1.0.js around line 46, review and complete this code-review fix: ReferenceError: undefined variable 'f' in agent-redir-rtc-0.1.0.js fallback branch.
What the draft fix changed: In `obj.xxOnMessage`, changed the else-if condition `f.readAsArrayBuffer` to `fileReader.readAsArrayBuffer`, matching the declared `fileReader` variable (from `var fileReader = new FileReader();`) so the branch no longer throws a ReferenceError on engines that reach it.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
flamingo
Bot
changed the title
Closes 36 review findings across 19 files.
Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
pkcs7-modified.js:30agents/modules_meshcmd/amt-scanner.js:64delete serveron a local variable, which has no effect and leaks socket referenceagents/modules_meshcmd/amt-scanner.js:97agents/modules_meshcmd/amt-scanner.js:92plugins/openframe.js:62plugins/openframe.js:115plugins/openframe.js:75amtprovisioningserver.js:41amtprovisioningserver.js:9domainandreqvariables when building title2 placeholdersmeshscanner.js:166meshscanner.js:97rdp/protocol/t125/gcc.js:447rdp/protocol/t125/gcc.js:448agents/modules_meshcmd/amt-lme.js:215agents/modules_meshcmd/amt-lme.js:156agents/modules_meshcmd/smbios.js:301agents/modules_meshcmd/smbios.js:17agents/modules_meshcmd/sysinfo.js:197agents/modules_meshcmd/sysinfo.js:197amtscanner.js:153amtscanner.js:76interceptor.js:197interceptor.js:206pluginHandler.js:252pluginHandler.js:250agents/agentrecoverycore.js:256agents/agentrecoverycore.js:130amt/amt-xml.js:102amt/amt-xml.js:50rdp/core/layer.js:153rdp/core/layer.js:153amt/amt-redir-mesh.js:233amt/amt-redir-mesh.js:80public/samples/relay.js:24rdp/protocol/nla.js:132public/scripts/agent-redir-rtc-0.1.0.js:46What 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:
a271a374-6235-40d2-8fdd-b6d4de951f4bMerging 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.
ClickUp task: CU-86akdypw4 Ad hoc sweep fixes across services (14 PRs)