fix(adhoc-sweep-fixes): 34 review findings across 27 files - #149
fix(adhoc-sweep-fixes): 34 review findings across 27 files#149flamingo[bot] wants to merge 26 commits into
Conversation
![flamingo[bot]](https://avatars.githubusercontent.com/in/3426709?s=60&v=4){kind=small}
There was a problem hiding this comment.
🦩 What this fix changed, finding by finding
34 finding(s) fixed in this draft — 33 explained inline on the diff; 4 low-confidence hunk(s) need close review before merging.
Fixes without an inline anchor in this diff
🟠 23. AmtSetupBinEncode has a logically-broken bounds check that can never be true — public/scripts/amt-setupbin-0.1.0.js:178
In AmtSetupBinEncode, changed the bounds-check operator from && to || in the guard if (obj.fileType < 1 || obj.fileType > AmtSetupBinSetupGuids.length) return null;, exactly as suggested, so invalid fileType values (0, negative, or out-of-range) are now correctly rejected before being used to index AmtSetupBinSetupGuids.
🤖 Prompt for AI agents
In public/scripts/amt-setupbin-0.1.0.js around line 178, review and complete this code-review fix: AmtSetupBinEncode has a logically-broken bounds check that can never be true.
What the draft fix changed: In `AmtSetupBinEncode`, changed the bounds-check operator from `&&` to `||` in the guard `if (obj.fileType < 1 || obj.fileType > AmtSetupBinSetupGuids.length) return null;`, exactly as suggested, so invalid `fileType` values (0, negative, or out-of-range) are now correctly rejected before being used to index `AmtSetupBinSetupGuids`.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
|
|
||
| if (!(this.selectedProtocol & Protocols.PROTOCOL_SSL)) { | ||
| var confirm = serverConnectionConfirm(); | ||
| confirm.obj.protocolNeg.obj.type.value = NegociationType.TYPE_RDP_NEG_FAILURE; | ||
| confirm.obj.protocolNeg.obj.type.value = NegotiationType.TYPE_RDP_NEG_FAILURE; | ||
| confirm.obj.protocolNeg.obj.result.value = NegotiationFailureCode.SSL_REQUIRED_BY_SERVER; | ||
| this.transport.send(confirm); | ||
| this.close(); |
There was a problem hiding this comment.
🦩 🔴 Reference to undefined 'NegociationType' (typo) will throw ReferenceError on SSL rejection path
Fixed the typo NegociationType -> NegotiationType in Server.prototype.recvConnectionRequest (SSL rejection branch), referencing the correctly-spelled object already defined at the top of the file, resolving the ReferenceError on that path.
🤖 Prompt for AI agents
In rdp/protocol/x224.js around line 259, review and complete this code-review fix: Reference to undefined 'NegociationType' (typo) will throw ReferenceError on SSL rejection path.
What the draft fix changed: Fixed the typo `NegociationType` -> `NegotiationType` in `Server.prototype.recvConnectionRequest` (SSL rejection branch), referencing the correctly-spelled object already defined at the top of the file, resolving the ReferenceError on that path.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
|
|
||
| if (!(this.selectedProtocol & Protocols.PROTOCOL_SSL)) { | ||
| var confirm = serverConnectionConfirm(); | ||
| confirm.obj.protocolNeg.obj.type.value = NegociationType.TYPE_RDP_NEG_FAILURE; | ||
| confirm.obj.protocolNeg.obj.type.value = NegotiationType.TYPE_RDP_NEG_FAILURE; | ||
| confirm.obj.protocolNeg.obj.result.value = NegotiationFailureCode.SSL_REQUIRED_BY_SERVER; | ||
| this.transport.send(confirm); | ||
| this.close(); |
There was a problem hiding this comment.
🦩 🔴 Reference to undefined 'NegotiationFailureCode' in x224.js Server negotiation failure path
Added a new NegotiationFailureCode object definition (with standard MSDN-documented failure code values, e.g. SSL_REQUIRED_BY_SERVER: 0x00000001) near the other constant definitions (MessageType, NegotiationType), and left the reference to NegotiationFailureCode.SSL_REQUIRED_BY_SERVER in Server.prototype.recvConnectionRequest unchanged since it now resolves correctly. The exact numeric values for the other failure codes are inferred from the MSDN spec referenced in the file's own comments but were not independently verified against a canonical source in this repo, so a reviewer should confirm these constants match any existing protocol expectations.
🤖 Prompt for AI agents
In rdp/protocol/x224.js around line 259, review and complete this code-review fix: Reference to undefined 'NegotiationFailureCode' in x224.js Server negotiation failure path.
What the draft fix changed: Added a new `NegotiationFailureCode` object definition (with standard MSDN-documented failure code values, e.g. `SSL_REQUIRED_BY_SERVER: 0x00000001`) near the other constant definitions (`MessageType`, `NegotiationType`), and left the reference to `NegotiationFailureCode.SSL_REQUIRED_BY_SERVER` in `Server.prototype.recvConnectionRequest` unchanged since it now resolves correctly. The exact numeric values for the other failure codes are inferred from the MSDN spec referenced in the file's own comments but were not independently verified against a canonical source in this repo, so a reviewer should confirm these constants match any existing protocol expectations.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 80 medium — react 👍/👎 to teach the reviewer
| mem.MemTotal = (mem.MemFree + mem.MemUsed); | ||
| mem.percentFree = ((mem.MemFree / mem.MemTotal) * 100);//.toFixed(2); | ||
| mem.percentConsumed = (((mem.MemTotal - mem.MemFree) / mem.MemTotal) * 100);//.toFixed(2); | ||
| return (mem); | ||
| ret._res(mem); | ||
| } | ||
| else | ||
| { | ||
| throw ('Parse Error'); | ||
| ret._rej('Parse Error'); | ||
| } | ||
| return (ret); | ||
| } | ||
|
|
||
| function windows_thermals() |
There was a problem hiding this comment.
🦩 🔴 macos_thermals references undeclared global 'child' variable via implicit global leak
In linux_thermals (agents/modules_meshcore/sysinfo.js), added var to the first child = require('child_process').execFile('/bin/sh', ['sh']); assignment so it becomes var child = ..., eliminating the implicit global leak. The second assignment later in the same function intentionally reuses the same locally-scoped child variable (already declared via var in this fix) to run a second shell command, which is correct existing behavior and requires no var since it's the same function-scoped variable.
🤖 Prompt for AI agents
In agents/modules_meshcore/sysinfo.js around line 197, review and complete this code-review fix: macos_thermals references undeclared global 'child' variable via implicit global leak.
What the draft fix changed: In linux_thermals (agents/modules_meshcore/sysinfo.js), added `var` to the first `child = require('child_process').execFile('/bin/sh', ['sh']);` assignment so it becomes `var child = ...`, eliminating the implicit global leak. The second assignment later in the same function intentionally reuses the same locally-scoped `child` variable (already declared via `var` in this fix) to run a second shell command, which is correct existing behavior and requires no `var` since it's the same function-scoped variable.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer
| mem.MemTotal = (mem.MemFree + mem.MemUsed); | ||
| mem.percentFree = ((mem.MemFree / mem.MemTotal) * 100);//.toFixed(2); | ||
| mem.percentConsumed = (((mem.MemTotal - mem.MemFree) / mem.MemTotal) * 100);//.toFixed(2); | ||
| return (mem); | ||
| ret._res(mem); | ||
| } | ||
| else | ||
| { | ||
| throw ('Parse Error'); | ||
| ret._rej('Parse Error'); | ||
| } | ||
| return (ret); | ||
| } | ||
|
|
||
| function windows_thermals() |
There was a problem hiding this comment.
🦩 🟠 macos_memUtilization throws a plain string instead of an Error and never uses the promise it constructs
In macos_memUtilization, changed the success path from return (mem); to ret._res(mem); followed by return (ret);, and changed the failure path from throw ('Parse Error'); to ret._rej('Parse Error'); followed by return (ret);, restructuring the if/else so both branches fall through to a single return (ret); at the end. This aligns the function's contract with its promise-based siblings (windows_cpuUtilization, linux_cpuUtilization, macos_cpuUtilization). Risk: any existing caller that relied on the old synchronous return value of mem or caught the thrown string will now need to use the promise interface instead — this is a behavioral change required by the finding but could affect callers outside this file that aren't visible here.
(Automatically downgraded: no change in this fix lands near this finding's line — verify whether it was actually addressed.)
🤖 Prompt for AI agents
In agents/modules_meshcore/sysinfo.js around line 168, review and complete this code-review fix: macos_memUtilization throws a plain string instead of an Error and never uses the promise it constructs.
What the draft fix changed: In macos_memUtilization, changed the success path from `return (mem);` to `ret._res(mem);` followed by `return (ret);`, and changed the failure path from `throw ('Parse Error');` to `ret._rej('Parse Error');` followed by `return (ret);`, restructuring the if/else so both branches fall through to a single `return (ret);` at the end. This aligns the function's contract with its promise-based siblings (windows_cpuUtilization, linux_cpuUtilization, macos_cpuUtilization). Risk: any existing caller that relied on the old synchronous return value of `mem` or caught the thrown string will now need to use the promise interface instead — this is a behavioral change required by the finding but could affect callers outside this file that aren't visible here.
_(Automatically downgraded: no change in this fix lands near this finding's line — verify whether it was actually addressed.)_
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
| 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 identifier 'f' after require() call causes a syntax error
In the top try block (lines around the require chain near the top of the file), removed the stray bare identifier f that followed require('../node-forge/lib/x509');, changing it to just require('../node-forge/lib/x509');. This eliminates the syntax/reference error artifact so the try block can run to completion without being swallowed by the catch, restoring the intended relative-require fallback behavior.
🤖 Prompt for AI agents
In pkcs7-modified.js around line 30, review and complete this code-review fix: Stray identifier 'f' after require() call causes a syntax error.
What the draft fix changed: In the top `try` block (lines around the require chain near the top of the file), removed the stray bare identifier `f` that followed `require('../node-forge/lib/x509');`, changing it to just `require('../node-forge/lib/x509');`. This eliminates the syntax/reference error artifact so the try block can run to completion without being swallowed by the catch, restoring the intended relative-require fallback behavior.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 97 high — react 👍/👎 to teach the reviewer
| * @returns {type.Component} per encoded object identifier | ||
| */ | ||
| function writeObjectIdentifier(oid) { | ||
| return new type.Component([new type.UInt8(5), new type.UInt8((oid[0] << 4) & (oid[1] & 0x0f)), new type.UInt8(oid[2]), new type.UInt8(oid[3]), new type.UInt8(oid[4]), new type.UInt8(oid[5])]); | ||
| return new type.Component([new type.UInt8(5), new type.UInt8((oid[0] << 4) | (oid[1] & 0x0f)), new type.UInt8(oid[2]), new type.UInt8(oid[3]), new type.UInt8(oid[4]), new type.UInt8(oid[5])]); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
🦩 🟠 writeObjectIdentifier uses bitwise AND instead of OR, corrupting encoded OID byte
In writeObjectIdentifier (rdp/protocol/t125/per.js), changed (oid[0] << 4) & (oid[1] & 0x0f) to (oid[0] << 4) | (oid[1] & 0x0f) so the high nibble from oid[0] and low nibble from oid[1] are correctly combined via bitwise OR instead of being zeroed out by AND, matching the read-side logic in readObjectIdentifier.
🤖 Prompt for AI agents
In rdp/protocol/t125/per.js around line 195, review and complete this code-review fix: writeObjectIdentifier uses bitwise AND instead of OR, corrupting encoded OID byte.
What the draft fix changed: In writeObjectIdentifier (rdp/protocol/t125/per.js), changed `(oid[0] << 4) & (oid[1] & 0x0f)` to `(oid[0] << 4) | (oid[1] & 0x0f)` so the high nibble from oid[0] and low nibble from oid[1] are correctly combined via bitwise OR instead of being zeroed out by AND, matching the read-side logic in readObjectIdentifier.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| var tokens = str.split(' - Address: '); | ||
| for (var block in tokens) | ||
| { | ||
| if (block == 0) continue; |
There was a problem hiding this comment.
🦩 🟠 wifi-scanner.js leaks tokens as an implicit global (missing var/let)
Changed tokens = str.split(' - Address: '); to var tokens = str.split(' - Address: '); inside the this.child.ms.on('end', function () {...}) callback in WiFiScanner.prototype.Scan, declaring tokens as a function-local variable instead of an implicit global.
🤖 Prompt for AI agents
In agents/modules_meshcore/wifi-scanner.js around line 87, review and complete this code-review fix: wifi-scanner.js leaks `tokens` as an implicit global (missing var/let).
What the draft fix changed: Changed `tokens = str.split(' - Address: ');` to `var tokens = str.split(' - Address: ');` inside the `this.child.ms.on('end', function () {...})` callback in `WiFiScanner.prototype.Scan`, declaring `tokens` as a function-local variable instead of an implicit global.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| this.transport.once('data', function(s) { | ||
| self.recvServerControlCooperatePDU(s); | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| var self = this; |
There was a problem hiding this comment.
🦩 🟠 recvServerControlCooperatePDU / recvServerControlGrantedPDU / recvServerFontMapPDU register a second 'once' listener even on the mismatch branch, causing duplicate handlers
Added return; immediately after the this.transport.once('data', ...) retry registration inside the mismatch if branch in recvServerControlCooperatePDU, recvServerControlGrantedPDU, and recvServerFontMapPDU (mirroring the existing pattern in recvDemandActivePDU / recvServerSynchronizePDU). This prevents the unconditional fall-through code at the bottom of each function from registering a second once('data', ...) listener for the next state when the mismatch branch already scheduled a retry, eliminating the duplicate-handler/state-machine corruption bug described in the finding. Note: I also noticed a pre-existing unrelated typo (caps.CapsETTYPE_MULTIFRAGMENTUPDATE should be caps.CapsType.CAPSETTYPE_MULTIFRAGMENTUPDATE) in the constructor, which is outside the scope of this finding and was left as originally written in the source (I did not introduce it — it was already present in the given file and is preserved verbatim, not part of this fix).
🤖 Prompt for AI agents
In rdp/protocol/pdu/global.js around line 174, review and complete this code-review fix: recvServerControlCooperatePDU / recvServerControlGrantedPDU / recvServerFontMapPDU register a second 'once' listener even on the mismatch branch, causing duplicate handlers.
What the draft fix changed: Added `return;` immediately after the `this.transport.once('data', ...)` retry registration inside the mismatch `if` branch in `recvServerControlCooperatePDU`, `recvServerControlGrantedPDU`, and `recvServerFontMapPDU` (mirroring the existing pattern in `recvDemandActivePDU` / `recvServerSynchronizePDU`). This prevents the unconditional fall-through code at the bottom of each function from registering a second `once('data', ...)` listener for the next state when the mismatch branch already scheduled a retry, eliminating the duplicate-handler/state-machine corruption bug described in the finding. Note: I also noticed a pre-existing unrelated typo (`caps.CapsETTYPE_MULTIFRAGMENTUPDATE` should be `caps.CapsType.CAPSETTYPE_MULTIFRAGMENTUPDATE`) in the constructor, which is outside the scope of this finding and was left as originally written in the source (I did not introduce it — it was already present in the given file and is preserved verbatim, not part of this fix).
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 92 high — react 👍/👎 to teach the reviewer
| var germanpass = (abc !== '' && abc.includes('Kennwort:') && !abc.includes('Numerisches Kennwort:')); // German Password | ||
| var frenchpass = (abc !== '' && abc.includes('Mot de passe :') && !abc.includes('Mot de passe num')); // French Password | ||
| if (englishidpass || germanidpass || frenchidpass|| englishpass || germanpass || frenchpass) { | ||
| if (x + 1 >= lines.length) { continue; } | ||
| var nextline = lines[x + 1].trim(); | ||
| if (x + 1 < lines.length && (nextline !== '' && (nextline.startsWith('ID:') || nextline.startsWith('ID :')) )) { | ||
| identifier = nextline.replace('ID:','').replace('ID :', '').trim(); |
There was a problem hiding this comment.
🦩 🟠 win-volumes.js: bitlocker recovery password parser reads lines[x+1] with no bounds check before out-of-range access
In windows_volumes, inside the manage-bde output parsing loop, added a bounds check if (x + 1 >= lines.length) { continue; } immediately before var nextline = lines[x + 1].trim();, preventing the unconditional out-of-range array access on lines[x + 1] when the marker line is the last line of output. The pre-existing x + 1 < lines.length checks on the following branches are left intact and now function correctly since nextline is only computed when the index is valid.
🤖 Prompt for AI agents
In agents/modules_meshcore/win-volumes.js around line 91, review and complete this code-review fix: win-volumes.js: bitlocker recovery password parser reads lines[x+1] with no bounds check before out-of-range access.
What the draft fix changed: In `windows_volumes`, inside the manage-bde output parsing loop, added a bounds check `if (x + 1 >= lines.length) { continue; }` immediately before `var nextline = lines[x + 1].trim();`, preventing the unconditional out-of-range array access on `lines[x + 1]` when the marker line is the last line of output. The pre-existing `x + 1 < lines.length` checks on the following branches are left intact and now function correctly since `nextline` is only computed when the index is valid.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer
|
|
||
| // Load saved theme from local storage | ||
| const savedTheme = localStorage.getItem("theme"); | ||
| if (savedTheme) { |
There was a problem hiding this comment.
🦩 🟠 Theme name from localStorage used to build stylesheet path with dangerous fallback to '..'
In the DOMContentLoaded handler in theme-switcher.js, replaced the encodeURIComponent-based fallback logic (which used the literal '..' for the 'default' case) with an explicit ALLOWED_THEMES allowlist (["default", "dark", "light"]) checked via .includes(); any savedTheme value not in the allowlist now safely falls back to "default" instead of being percent-encoded and used directly, and '..' is no longer referenced anywhere. Risk/unverified: the actual set of valid theme folder names on disk is not visible in this file, so the allowlist ["default", "dark", "light"] is a placeholder guess — a complete fix requires confirming the real list of theme directories shipped under styles/themes/ and updating ALLOWED_THEMES to match exactly.
🤖 Prompt for AI agents
In public/scripts/themes/theme-switcher.js around line 6, review and complete this code-review fix: Theme name from localStorage used to build stylesheet path with dangerous fallback to '..'.
What the draft fix changed: In the DOMContentLoaded handler in theme-switcher.js, replaced the encodeURIComponent-based fallback logic (which used the literal '..' for the 'default' case) with an explicit ALLOWED_THEMES allowlist (["default", "dark", "light"]) checked via `.includes()`; any savedTheme value not in the allowlist now safely falls back to "default" instead of being percent-encoded and used directly, and '..' is no longer referenced anywhere. Risk/unverified: the actual set of valid theme folder names on disk is not visible in this file, so the allowlist ["default", "dark", "light"] is a placeholder guess — a complete fix requires confirming the real list of theme directories shipped under `styles/themes/` and updating ALLOWED_THEMES to match exactly.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 65 medium — react 👍/👎 to teach the reviewer
Closes 34 review findings across 27 files.
Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
rdp/protocol/x224.js:259rdp/protocol/x224.js:259agents/modules_meshcore/sysinfo.js:197agents/modules_meshcore/sysinfo.js:168pkcs7-modified.js:30public/novnc/core/ra2.js:12interceptor.js:197interceptor.js:90agents/modules_meshcore/win-deskutils.js:208agents/modules_meshcore/win-deskutils.js:108rdp/core/log.js:63rdp/core/log.js:55length,serverSettings,clientSettingsin rdp/protocol/t125/gcc.jsrdp/protocol/t125/gcc.js:296lengthandclientSettingsin readConferenceCreateRequestrdp/protocol/t125/gcc.js:330agents/meshcore_diagnostic.js:81agents/meshcore_diagnostic.js:84amtprovisioningserver.js:40console.errin SerialTunnel._write will throw TypeError instead of logging the intended erroramt/amt-wsman-comm.js:80monitoring.js:36rdp/asn1/ber.js:30rdp/protocol/pdu/data.js:108byteStreaminstead ofthis.byteStreamrdp/security/rc4.js:61public/scripts/amt-setupbin-0.1.0.js:178finstead offileReaderin fallback branchpublic/scripts/agent-redir-rtc-0.1.0.js:42rdp/protocol/cert.js:144opaqueparameter and re-declares it as a local var, shadowing the argumentagents/modules_meshcmd/amt-wsman.js:60f.readAsArrayBufferundefined-variable bug baked into the minified bundlepublic/scripts/agent-redir-rtc-0.1.0-min.js:1mcrec.js:200modalContentHTML string that is discarded and never renderedpublic/js/ui-components.js:34rdp/protocol/t125/per.js:195tokensas an implicit global (missing var/let)agents/modules_meshcore/wifi-scanner.js:87rdp/protocol/pdu/global.js:174agents/modules_meshcore/win-volumes.js:91public/scripts/themes/theme-switcher.js:6What 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.