fix(adhoc-sweep-fixes): CU-86akhf8u3 15 review findings across 10 files - #122
flamingo[bot] wants to merge 10 commits into
Conversation
![flamingo[bot]](https://avatars.githubusercontent.com/in/3426709?s=60&v=4){kind=small}
| fn.apply(this, opt); | ||
| }, callback, optional); | ||
| } | ||
| this.startConfiguration = function () { |
There was a problem hiding this comment.
🦩 🔴 amt_heci.js startConfiguration/stopConfiguration/openUserInitiatedConnection/closeUserInitiatedConnection reference undeclared data and callback
Added data, callback parameters to this.startConfiguration = function (data, callback) {...} so the this.sendCommand(0x29, data, ..., callback, optional) call resolves both identifiers from declared parameters instead of the (nonexistent) enclosing scope.
🤖 Prompt for AI agents
In modules/amt_heci.js around line 265, review and complete this code-review fix: amt_heci.js startConfiguration/stopConfiguration/openUserInitiatedConnection/closeUserInitiatedConnection reference undeclared `data` and `callback`.
What the draft fix changed: Added `data, callback` parameters to `this.startConfiguration = function (data, callback) {...}` so the `this.sendCommand(0x29, data, ..., callback, optional)` call resolves both identifiers from declared parameters instead of the (nonexistent) enclosing scope.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| for (var i = 2; i < arguments.length; ++i) { optional.push(arguments[i]); } | ||
| this.sendCommand(0x29, data, function (header, fn, opt) { opt.unshift(header.Status); fn.apply(this, opt); }, callback, optional); | ||
| } | ||
| this.stopConfiguration = function () { |
There was a problem hiding this comment.
🦩 🔴 amt_heci.js stopConfiguration references undeclared data and callback
Added data, callback parameters to this.stopConfiguration = function (data, callback) {...}, fixing the same undeclared-variable ReferenceError for the sendCommand(0x5E, ...) call.
🤖 Prompt for AI agents
In modules/amt_heci.js around line 270, review and complete this code-review fix: amt_heci.js stopConfiguration references undeclared `data` and `callback`.
What the draft fix changed: Added `data, callback` parameters to `this.stopConfiguration = function (data, callback) {...}`, fixing the same undeclared-variable ReferenceError for the `sendCommand(0x5E, ...)` call.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| for (var i = 2; i < arguments.length; ++i) { optional.push(arguments[i]); } | ||
| this.sendCommand(0x5E, data, function (header, fn, opt) { opt.unshift(header.Status); fn.apply(this, opt); }, callback, optional); | ||
| } | ||
| this.openUserInitiatedConnection = function () { |
There was a problem hiding this comment.
🦩 🔴 amt_heci.js openUserInitiatedConnection references undeclared data and callback
Added data, callback parameters to this.openUserInitiatedConnection = function (data, callback) {...}, fixing the same undeclared-variable ReferenceError for the sendCommand(0x44, ...) call.
🤖 Prompt for AI agents
In modules/amt_heci.js around line 275, review and complete this code-review fix: amt_heci.js openUserInitiatedConnection references undeclared `data` and `callback`.
What the draft fix changed: Added `data, callback` parameters to `this.openUserInitiatedConnection = function (data, callback) {...}`, fixing the same undeclared-variable ReferenceError for the `sendCommand(0x44, ...)` call.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| for (var i = 2; i < arguments.length; ++i) { optional.push(arguments[i]); } | ||
| this.sendCommand(0x44, data, function (header, fn, opt) { opt.unshift(header.Status); fn.apply(this, opt); }, callback, optional); | ||
| } | ||
| this.closeUserInitiatedConnection = function () { |
There was a problem hiding this comment.
🦩 🔴 amt_heci.js closeUserInitiatedConnection references undeclared data and callback
Added data, callback parameters to this.closeUserInitiatedConnection = function (data, callback) {...}, fixing the same undeclared-variable ReferenceError for the sendCommand(0x45, ...) call.
🤖 Prompt for AI agents
In modules/amt_heci.js around line 280, review and complete this code-review fix: amt_heci.js closeUserInitiatedConnection references undeclared `data` and `callback`.
What the draft fix changed: Added `data, callback` parameters to `this.closeUserInitiatedConnection = function (data, callback) {...}`, fixing the same undeclared-variable ReferenceError for the `sendCommand(0x45, ...)` call.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| @@ -297,7 +297,7 @@ function amt_heci() { | |||
| } | |||
| this.getProtocolVersion = function (callback) { | |||
There was a problem hiding this comment.
🦩 🔴 amt_heci.js getProtocolVersion references undeclared opt instead of optional
In this.getProtocolVersion, changed opt.push(arguments[i]) to optional.push(arguments[i]) in the arguments-collecting loop so it populates the actually-declared optional array instead of the undeclared opt.
🤖 Prompt for AI agents
In modules/amt_heci.js around line 298, review and complete this code-review fix: amt_heci.js getProtocolVersion references undeclared `opt` instead of `optional`.
What the draft fix changed: In `this.getProtocolVersion`, changed `opt.push(arguments[i])` to `optional.push(arguments[i])` in the arguments-collecting loop so it populates the actually-declared `optional` array instead of the undeclared `opt`.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| { | ||
| properties.push(nn.Deref().increment(i * GM.PointerSize).Deref().Wide2UTF8); | ||
| } | ||
|
|
||
| OleAut32.SafeArrayUnaccessData(nme.Deref()); | ||
| } | ||
|
|
||
| // Now we need to introspect the Array Fields |
There was a problem hiding this comment.
🦩 🟠 SafeArrayAccessData success path never calls SafeArrayUnaccessData on the outer array in win-wmi.js enumerateProperties
In enumerateProperties, added OleAut32.SafeArrayUnaccessData(nme.Deref()) immediately after the loop that consumes the property-names SAFEARRAY obtained via OleAut32.SafeArrayAccessData(nme.Deref(), nn), pairing the Access/Unaccess calls as required by the SafeArray API and matching the pattern already used for the array-value branch further down in the same function.
🤖 Prompt for AI agents
In modules/win-wmi.js around line 246, review and complete this code-review fix: SafeArrayAccessData success path never calls SafeArrayUnaccessData on the outer array in win-wmi.js enumerateProperties.
What the draft fix changed: In `enumerateProperties`, added `OleAut32.SafeArrayUnaccessData(nme.Deref())` immediately after the loop that consumes the property-names SAFEARRAY obtained via `OleAut32.SafeArrayAccessData(nme.Deref(), nn)`, pairing the Access/Unaccess calls as required by the SafeArray API and matching the pattern already used for the array-value branch further down in the same function.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer
| VERSION="${{ github.event.inputs.openssl_version || '3.5.4' }}" | ||
| BUILD_DIR="openssl-build-${{ matrix.arch.name }}-${{ matrix.config.suffix }}" | ||
| curl -L "https://github.com/openssl/openssl/releases/download/openssl-$VERSION/openssl-$VERSION.tar.gz" -o openssl.tar.gz | ||
| curl -L "https://github.com/openssl/openssl/releases/download/openssl-$VERSION/openssl-$VERSION.tar.gz.sha256" -o openssl.tar.gz.sha256 | ||
| EXPECTED_SHA256="$(cat openssl.tar.gz.sha256 | tr -d '[:space:]')" | ||
| ACTUAL_SHA256="$(sha256sum openssl.tar.gz | awk '{print $1}')" | ||
| if [ -z "$EXPECTED_SHA256" ] || [ "$EXPECTED_SHA256" != "$ACTUAL_SHA256" ]; then | ||
| echo "Checksum verification failed for openssl-$VERSION.tar.gz" | ||
| echo "Expected: $EXPECTED_SHA256" | ||
| echo "Actual: $ACTUAL_SHA256" | ||
| exit 1 | ||
| fi | ||
| mkdir -p "$BUILD_DIR" | ||
| tar -xzf openssl.tar.gz -C "$BUILD_DIR" --strip-components=1 | ||
| rm openssl.tar.gz | ||
| rm openssl.tar.gz openssl.tar.gz.sha256 | ||
|
|
||
| - name: Configure OpenSSL | ||
| shell: cmd |
There was a problem hiding this comment.
🦩 🟠 build-openssl-windows.yml downloads and builds unpinned OpenSSL source via HTTPS with no checksum verification
In the "Download OpenSSL source" step of the build-windows job (.github/workflows/build-openssl-windows.yml), added a download of the corresponding .sha256 checksum file published alongside each OpenSSL GitHub release tarball, computed the actual SHA-256 of the downloaded tarball with sha256sum, and added a comparison that fails the step (exit 1) if the expected and actual checksums don't match or the expected checksum is empty, before extraction proceeds. This mitigates a corrupted/MITM'd download producing poisoned libs. Unverified: I have not confirmed that OpenSSL's GitHub release assets actually include a .sha256 file at that exact URL path for every historical/future release (OpenSSL publishes SHA256 sums on openssl.org's download page and in some release notes, but the exact asset filename convention on GitHub Releases was not confirmed against the live release assets); if that asset does not exist for a given version, this step will fail closed (safe, but may block otherwise-valid builds) rather than silently skip verification. A more complete fix would pin the hash value explicitly per version (e.g., in a checked-in mapping) or verify a GPG signature against OpenSSL's published signing key rather than relying on a co-located hash file fetched over the same channel.
🤖 Prompt for AI agents
In .github/workflows/build-openssl-windows.yml around line 47, review and complete this code-review fix: build-openssl-windows.yml downloads and builds unpinned OpenSSL source via HTTPS with no checksum verification.
What the draft fix changed: In the "Download OpenSSL source" step of the `build-windows` job (`.github/workflows/build-openssl-windows.yml`), added a download of the corresponding `.sha256` checksum file published alongside each OpenSSL GitHub release tarball, computed the actual SHA-256 of the downloaded tarball with `sha256sum`, and added a comparison that fails the step (`exit 1`) if the expected and actual checksums don't match or the expected checksum is empty, before extraction proceeds. This mitigates a corrupted/MITM'd download producing poisoned libs. Unverified: I have not confirmed that OpenSSL's GitHub release assets actually include a `.sha256` file at that exact URL path for every historical/future release (OpenSSL publishes SHA256 sums on openssl.org's download page and in some release notes, but the exact asset filename convention on GitHub Releases was not confirmed against the live release assets); if that asset does not exist for a given version, this step will fail closed (safe, but may block otherwise-valid builds) rather than silently skip verification. A more complete fix would pin the hash value explicitly per version (e.g., in a checked-in mapping) or verify a GPG signature against OpenSSL's published signing key rather than relying on a co-located hash file fetched over the same channel.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 55 low — review closely — react 👍/👎 to teach the reviewer
| } | ||
| else | ||
| { | ||
| k32.ClosePseudoConsole(ret._h.Deref()); | ||
| k32.CloseHandle(ret._input.Deref()); | ||
| k32.CloseHandle(ret._output.Deref()); | ||
| k32.CloseHandle(ret._consoleInput.Deref()); | ||
| k32.CloseHandle(ret._consoleOutput.Deref()); | ||
| } | ||
| } | ||
|
|
||
| else | ||
| { | ||
| k32.ClosePseudoConsole(ret._h.Deref()); | ||
| k32.CloseHandle(ret._input.Deref()); | ||
| k32.CloseHandle(ret._output.Deref()); | ||
| k32.CloseHandle(ret._consoleInput.Deref()); | ||
| k32.CloseHandle(ret._consoleOutput.Deref()); | ||
| } | ||
| } | ||
| else | ||
| { | ||
| k32.ClosePseudoConsole(ret._h.Deref()); | ||
| k32.CloseHandle(ret._input.Deref()); | ||
| k32.CloseHandle(ret._output.Deref()); | ||
| k32.CloseHandle(ret._consoleInput.Deref()); | ||
| k32.CloseHandle(ret._consoleOutput.Deref()); | ||
| } | ||
| throw ('Internal Error'); | ||
| } |
There was a problem hiding this comment.
🦩 🟠 CreatePseudoConsole handle and pipes leaked when CreateProcessW / UpdateProcThreadAttribute fails
In vt.Create (modules/win-virtual-terminal.js), added cleanup blocks in the else branches for the CreateProcessW failure, the UpdateProcThreadAttribute failure, and the InitializeProcThreadAttributeList failure. Each new else now calls k32.ClosePseudoConsole(ret._h.Deref()) and k32.CloseHandle() on ret._input, ret._output, ret._consoleInput, and ret._consoleOutput before falling through to throw ('Internal Error'). This addresses the leaked pseudoconsole handle and four pipe handles on every failure path. Not freed: the attrList and startupinfoex GM.CreateVariable-backed native memory blocks — these are garbage-collected buffers wrapped by the marshal layer rather than raw HeapAlloc pointers explicitly freed elsewhere in this file (no HeapFree call exists in the original code either), so no change was made there since inventing a free call risks diverging from the module's existing memory-management pattern; a reviewer familiar with the _GenericMarshal lifecycle should confirm whether HeapFree of the attrList (obtained via HeapAlloc-style semantics if any) is actually needed.
(Automatically downgraded: no change in this fix lands near this finding's line — verify whether it was actually addressed.)
🤖 Prompt for AI agents
In modules/win-virtual-terminal.js around line 96, review and complete this code-review fix: CreatePseudoConsole handle and pipes leaked when CreateProcessW / UpdateProcThreadAttribute fails.
What the draft fix changed: In vt.Create (modules/win-virtual-terminal.js), added cleanup blocks in the `else` branches for the CreateProcessW failure, the UpdateProcThreadAttribute failure, and the InitializeProcThreadAttributeList failure. Each new `else` now calls `k32.ClosePseudoConsole(ret._h.Deref())` and `k32.CloseHandle()` on `ret._input`, `ret._output`, `ret._consoleInput`, and `ret._consoleOutput` before falling through to `throw ('Internal Error')`. This addresses the leaked pseudoconsole handle and four pipe handles on every failure path. Not freed: the `attrList` and `startupinfoex` GM.CreateVariable-backed native memory blocks — these are garbage-collected buffers wrapped by the marshal layer rather than raw HeapAlloc pointers explicitly freed elsewhere in this file (no HeapFree call exists in the original code either), so no change was made there since inventing a free call risks diverging from the module's existing memory-management pattern; a reviewer familiar with the _GenericMarshal lifecycle should confirm whether HeapFree of the attrList (obtained via HeapAlloc-style semantics if any) is actually needed.
_(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
| p.waitExit(); | ||
| } | ||
|
|
||
| var J = JSON.parse(p.stdout.str); |
There was a problem hiding this comment.
🦩 🟠 getProcesses() on linux does not check p.stdout.str for empty/parse errors before JSON.parse, risking an uncaught exception on malformed awk output
In getProcesses(), both the linux branch (primary/fallback awk pipeline output) and the darwin/freebsd branch now wrap JSON.parse(p.stdout.str) in a try/catch. On parse failure, instead of letting the exception propagate, the code builds p.args from the extra arguments (matching the existing pattern used elsewhere in this function) and invokes callback with null as the first argument (in place of the parsed process map), then breaks out of the switch case rather than continuing to process a partially-undefined J. This routes the error to the caller via the same callback-based convention already used in this function, instead of throwing synchronously. Risk: callers of getProcesses/enumerateProcesses that don't currently check for a falsy first argument will need to handle null being passed instead of a process map — this is a behavior change from "crash" to "callback(null)", which is safer but callers should be audited; I did not have visibility into every call site.
🤖 Prompt for AI agents
In modules/process-manager.js around line 169, review and complete this code-review fix: getProcesses() on linux does not check p.stdout.str for empty/parse errors before JSON.parse, risking an uncaught exception on malformed awk output.
What the draft fix changed: In `getProcesses()`, both the linux branch (primary/fallback awk pipeline output) and the darwin/freebsd branch now wrap `JSON.parse(p.stdout.str)` in a try/catch. On parse failure, instead of letting the exception propagate, the code builds `p.args` from the extra arguments (matching the existing pattern used elsewhere in this function) and invokes `callback` with `null` as the first argument (in place of the parsed process map), then breaks out of the switch case rather than continuing to process a partially-undefined `J`. This routes the error to the caller via the same callback-based convention already used in this function, instead of throwing synchronously. Risk: callers of `getProcesses`/`enumerateProcesses` that don't currently check for a falsy first argument will need to handle `null` being passed instead of a process map — this is a behavior change from "crash" to "callback(null)", which is safer but callers should be audited; I did not have visibility into every call site.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 75 medium — react 👍/👎 to teach the reviewer
| ? '/Library/Application Support/OpenFrame/machine_id' | ||
| : '/var/lib/openframe/machine_id'); | ||
| openframeMachineId = fs.readFileSync(machineIdPath).toString().trim(); | ||
| } catch (ex) { openframeMachineId = null; } | ||
| } catch (ex) { | ||
| openframeMachineId = null; | ||
| try { sendConsoleText('getOpenFrameMachineId error: ' + ex); } catch (ex2) { console.log('getOpenFrameMachineId error: ' + ex); } | ||
| } | ||
| return openframeMachineId; | ||
| } | ||
|
|
There was a problem hiding this comment.
🦩 🟠 getOpenFrameMachineId silently swallows all errors including unexpected exceptions
In getOpenFrameMachineId() (modules/RecoveryCore.js), the catch block now logs the exception before discarding it, via sendConsoleText (with a console.log fallback in case sendConsoleText itself throws, e.g. if MeshAgent isn't ready yet), so unexpected exceptions surface for diagnosis instead of being silently swallowed, while still degrading openframeMachineId to null so existing "file not found" behavior is preserved.
🤖 Prompt for AI agents
In modules/RecoveryCore.js around line 10, review and complete this code-review fix: getOpenFrameMachineId silently swallows all errors including unexpected exceptions.
What the draft fix changed: In getOpenFrameMachineId() (modules/RecoveryCore.js), the catch block now logs the exception before discarding it, via sendConsoleText (with a console.log fallback in case sendConsoleText itself throws, e.g. if MeshAgent isn't ready yet), so unexpected exceptions surface for diagnosis instead of being silently swallowed, while still degrading openframeMachineId to null so existing "file not found" behavior is preserved.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 80 medium — react 👍/👎 to teach the reviewer
flamingo
Bot
changed the title
Closes 15 review findings across 10 files.
Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
Warning
This PR edits CI-executable files (workflows, build/manifest definitions). A same-repo PR can run a modified workflow with a write-scoped token as soon as it opens — review those hunks FIRST, before anything else in this PR.
dataandcallbackmodules/amt_heci.js:265dataandcallbackmodules/amt_heci.js:270dataandcallbackmodules/amt_heci.js:275dataandcallbackmodules/amt_heci.js:280optinstead ofoptionalmodules/amt_heci.js:298data,callback, andvmodules/amt_heci.js:285meshcore/KVM/MacOS/mac_tile.h:10microstack/ILibMulticastSocket.c:213modules/lme_heci.js:143modules/amt-xml.js:73modules/win-wmi.js:246.github/workflows/build-openssl-windows.yml:47modules/win-virtual-terminal.js:96modules/process-manager.js:169modules/RecoveryCore.js:10What 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:
bc788a01-3d0c-4679-816f-282037bb5004Merging 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-86akhf8u3 MeshAgent review findings sweep (13 PRs)