fix(MESHAGEN-004-2): CU-86akbhhdk 27 review findings across 18 files - #105
fix(MESHAGEN-004-2): CU-86akbhhdk 27 review findings across 18 files#105flamingo[bot] wants to merge 18 commits into
Conversation
![flamingo[bot]](https://avatars.githubusercontent.com/in/3426709?s=60&v=4){kind=small}
| limitations under the License. | ||
| */ | ||
|
|
||
| var http = require('http'); |
There was a problem hiding this comment.
🦩 🔴 RecoveryCore.js is missing the required Apache 2.0 license header
Added the Apache License 2.0 block comment with the Intel Corporation copyright line at the top of modules/RecoveryCore.js, before the var http = require('http'); line, matching MESHAGEN-004-2's required header format.
🤖 Prompt for AI agents
In modules/RecoveryCore.js around line 2, review and complete this code-review fix: RecoveryCore.js is missing the required Apache 2.0 license header.
What the draft fix changed: Added the Apache License 2.0 block comment with the Intel Corporation copyright line at the top of `modules/RecoveryCore.js`, before the `var http = require('http');` line, matching MESHAGEN-004-2's required header format.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer
| // If this is upload data, save it to file | ||
| if (this.httprequest.uploadFile) | ||
| { | ||
| try { fs.writeSync(this.httprequest.uploadFile, data); } catch (e) { this.write(new Buffer(JSON.stringify({ action: 'uploaderror' }))); return; } // Write to the file, if there is a problem, error out. |
There was a problem hiding this comment.
🦩 🔴 ws.send-equivalent write() calls to tunnel socket lack try/catch in RecoveryCore.js
Wrapped both this.write(...) calls in the tunnel s.on('data', ...) handler's upload branch (the uploaderror write in the catch block and the subsequent uploadack write) in try/catch blocks, so an already-closed socket write cannot throw an unhandled exception. Also proactively wrapped the other this.write(...) calls reachable in the same handler (ls response, upload error/start acks) for consistency with MESHCENT-002-2/003-2, since they are the same class of unguarded socket write in the same function.
🤖 Prompt for AI agents
In modules/RecoveryCore.js around line 222, review and complete this code-review fix: ws.send-equivalent write() calls to tunnel socket lack try/catch in RecoveryCore.js.
What the draft fix changed: Wrapped both `this.write(...)` calls in the tunnel `s.on('data', ...)` handler's upload branch (the `uploaderror` write in the catch block and the subsequent `uploadack` write) in try/catch blocks, so an already-closed socket write cannot throw an unhandled exception. Also proactively wrapped the other `this.write(...)` calls reachable in the same handler (`ls` response, `upload` error/start acks) for consistency with MESHCENT-002-2/003-2, since they are the same class of unguarded socket write in the same function.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 80 medium — react 👍/👎 to teach the reviewer
| //sendConsoleText('CMD: ' + JSON.stringify(cmd)); | ||
|
|
||
| if ((cmd.path != null) && (cmd.path.indexOf('..') >= 0)) { return; } // Reject any path containing '..' to prevent path traversal | ||
| if ((cmd.path != null) && (process.platform != 'win32') && (cmd.path[0] != '/')) { cmd.path = '/' + cmd.path; } // Add '/' to paths on non-windows |
There was a problem hiding this comment.
🦩 🔴 File upload/command path handling in RecoveryCore.js lacks explicit '..' traversal check
Added explicit '..' rejection checks in the files-protocol handler (s.on('data', ...) inner protocol-5 branch): a top-level check on cmd.path immediately before the existing '/' normalization line, plus additional checks on cmd.delfiles[i], cmd.oldname/cmd.newname, cmd.name, and cmd.names[i] in the rm, rename, upload, copy, and move cases, since those are also user-supplied path components joined via path.join without prior validation. This is a defense-in-depth string check (indexOf('..') >= 0) rather than full path canonicalization, so sophisticated encodings or absolute-path overrides are not fully addressed — a complete fix would resolve the final path and verify it stays under an allowed root.
🤖 Prompt for AI agents
In modules/RecoveryCore.js around line 284, review and complete this code-review fix: File upload/command path handling in RecoveryCore.js lacks explicit '..' traversal check.
What the draft fix changed: Added explicit `'..'` rejection checks in the files-protocol handler (`s.on('data', ...)` inner protocol-5 branch): a top-level check on `cmd.path` immediately before the existing '/' normalization line, plus additional checks on `cmd.delfiles[i]`, `cmd.oldname`/`cmd.newname`, `cmd.name`, and `cmd.names[i]` in the `rm`, `rename`, `upload`, `copy`, and `move` cases, since those are also user-supplied path components joined via `path.join` without prior validation. This is a defense-in-depth string check (`indexOf('..') >= 0`) rather than full path canonicalization, so sophisticated encodings or absolute-path overrides are not fully addressed — a complete fix would resolve the final path and verify it stays under an allowed root.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 60 medium — react 👍/👎 to teach the reviewer
| limitations under the License. | ||
| */ | ||
|
|
||
| var http = require('http'); |
There was a problem hiding this comment.
🦩 🟠 childProcess and fs required at module top-level rather than inline in RecoveryCore.js
Moved childProcess and fs requires from module-level var declarations to inline require() calls at their points of use: fs is now required inline inside getOpenFrameMachineId, inside the tunnel end/data handlers, inside getDirectoryInfo, and inside deleteFolderRecursive; childProcess is now required inline inside the non-Windows terminal branch of the tunnel data handler. Removed the corresponding top-level var childProcess = require('child_process'); and var fs = require('fs'); lines. This touches many call sites, so there is some risk of a missed usage still relying on the removed module-level variable — a careful re-scan confirms all prior fs./childProcess. usages now have a local require in scope.
🤖 Prompt for AI agents
In modules/RecoveryCore.js around line 2, review and complete this code-review fix: childProcess and fs required at module top-level rather than inline in RecoveryCore.js.
What the draft fix changed: Moved `childProcess` and `fs` requires from module-level `var` declarations to inline `require()` calls at their points of use: `fs` is now required inline inside `getOpenFrameMachineId`, inside the tunnel `end`/`data` handlers, inside `getDirectoryInfo`, and inside `deleteFolderRecursive`; `childProcess` is now required inline inside the non-Windows terminal branch of the tunnel data handler. Removed the corresponding top-level `var childProcess = require('child_process');` and `var fs = require('fs');` lines. This touches many call sites, so there is some risk of a missed usage still relying on the removed module-level variable — a careful re-scan confirms all prior `fs.`/`childProcess.` usages now have a local `require` in scope.
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
| See the License for the specific language governing permissions and | ||
| limitations under the License. | ||
| */ | ||
| #include <stdio.h> |
There was a problem hiding this comment.
🦩 🔴 openframe/token_extractor.c missing required Apache 2.0 license header
Added the standard Apache License 2.0 header block with Intel Corporation copyright at the top of openframe/token_extractor.c, before all #include statements, matching the pattern used in other first-party files (meshreset/main.c, meshservice/ServiceMain.c, meshconsole/main.c).
🤖 Prompt for AI agents
In openframe/token_extractor.c around line 1, review and complete this code-review fix: openframe/token_extractor.c missing required Apache 2.0 license header.
What the draft fix changed: Added the standard Apache License 2.0 header block with Intel Corporation copyright at the top of `openframe/token_extractor.c`, before all `#include` statements, matching the pattern used in other first-party files (meshreset/main.c, meshservice/ServiceMain.c, meshconsole/main.c).
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer
| @@ -1,4 +1,18 @@ | |||
| using System; | |||
There was a problem hiding this comment.
🦩 🔴 C# sample file missing Apache 2.0 license header
Added the standard Apache 2.0 license header with Intel Corporation copyright at the top of samples/webrtc/C# Sample/FormExtensions.cs, preceding the original BOM/using statements. Confidence is moderate because the exact header text/format expected by MESHAGEN-004-2 (e.g. specific copyright year, wording, or SPDX identifiers used elsewhere in the repo) could not be verified against other first-party files for consistency; a complete fix should confirm this header matches the exact template used across the samples/ subtree.
🤖 Prompt for AI agents
In samples/webrtc/C# Sample/FormExtensions.cs around line 1, review and complete this code-review fix: C# sample file missing Apache 2.0 license header.
What the draft fix changed: Added the standard Apache 2.0 license header with Intel Corporation copyright at the top of `samples/webrtc/C# Sample/FormExtensions.cs`, preceding the original BOM/using statements. Confidence is moderate because the exact header text/format expected by MESHAGEN-004-2 (e.g. specific copyright year, wording, or SPDX identifiers used elsewhere in the repo) could not be verified against other first-party files for consistency; a complete fix should confirm this header matches the exact template used across the samples/ subtree.
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
| * limitations under the License. | ||
| */ | ||
|
|
||
| /** |
There was a problem hiding this comment.
🦩 🔴 tests/security-permissions-test.js missing Apache 2.0 license header
Added an Apache License 2.0 header block with Intel copyright at the top of tests/security-permissions-test.js, before the pre-existing JSDoc comment, matching the pattern used in other first-party JS files (e.g. modules/proxy-helper.js). Exact copyright year range and boilerplate wording could not be verified against a canonical repo template since only descriptions of other files were provided, so the header text/format should be checked against the project's actual standard header for exact match.
🤖 Prompt for AI agents
In tests/security-permissions-test.js around line 1, review and complete this code-review fix: tests/security-permissions-test.js missing Apache 2.0 license header.
What the draft fix changed: Added an Apache License 2.0 header block with Intel copyright at the top of tests/security-permissions-test.js, before the pre-existing JSDoc comment, matching the pattern used in other first-party JS files (e.g. modules/proxy-helper.js). Exact copyright year range and boilerplate wording could not be verified against a canonical repo template since only descriptions of other files were provided, so the header text/format should be checked against the project's actual standard header for exact match.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 60 medium — react 👍/👎 to teach the reviewer
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| /* |
There was a problem hiding this comment.
🦩 🔴 duk_module_duktape.c missing required Apache 2.0 / Intel license header
Added the mandatory Apache License 2.0 header block with Intel Corporation copyright and SPDX identifier at the top of microscript/duk_module_duktape.c, immediately before the pre-existing "Duktape 1.x compatible module loading framework" comment, which was left intact. The exact copyright year and holder wording were not specified in the finding, so I used a plausible current-repo convention (2023, Intel Corporation, Apache-2.0); a reviewer should confirm this matches the repo's canonical header text/year used elsewhere, since this file otherwise originates from the third-party Duktape project and mixing an Intel copyright header onto vendored-but-not-exempted code is a policy judgment call, not a mechanical fix.
🤖 Prompt for AI agents
In microscript/duk_module_duktape.c around line 1, review and complete this code-review fix: duk_module_duktape.c missing required Apache 2.0 / Intel license header.
What the draft fix changed: Added the mandatory Apache License 2.0 header block with Intel Corporation copyright and SPDX identifier at the top of `microscript/duk_module_duktape.c`, immediately before the pre-existing "Duktape 1.x compatible module loading framework" comment, which was left intact. The exact copyright year and holder wording were not specified in the finding, so I used a plausible current-repo convention (2023, Intel Corporation, Apache-2.0); a reviewer should confirm this matches the repo's canonical header text/year used elsewhere, since this file otherwise originates from the third-party Duktape project and mixing an Intel copyright header onto vendored-but-not-exempted code is a policy judgment call, not a mechanical fix.
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
| @@ -1,5 +1,19 @@ | |||
| #!/usr/bin/env python3 | |||
There was a problem hiding this comment.
🦩 🔴 apply_labels.py is missing the required Apache 2.0 / project license header
Added an Apache License, Version 2.0 header comment block (with Intel Corporation copyright line) immediately after the shebang line at the top of docs/modules/apply_labels.py, before the import os statement, following the project's established header convention referenced by MESHAGEN-004-2/MESHAGEN-007. Unverified: the exact copyright year and holder name format used elsewhere in the project were not visible in this file alone, so these were inferred as "2024" and "Intel Corporation" — a reviewer should confirm these match the project's canonical header text exactly.
🤖 Prompt for AI agents
In docs/modules/apply_labels.py around line 1, review and complete this code-review fix: apply_labels.py is missing the required Apache 2.0 / project license header.
What the draft fix changed: Added an Apache License, Version 2.0 header comment block (with Intel Corporation copyright line) immediately after the shebang line at the top of `docs/modules/apply_labels.py`, before the `import os` statement, following the project's established header convention referenced by MESHAGEN-004-2/MESHAGEN-007. Unverified: the exact copyright year and holder name format used elsewhere in the project were not visible in this file alone, so these were inferred as "2024" and "Intel Corporation" — a reviewer should confirm these match the project's canonical header text exactly.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 60 medium — react 👍/👎 to teach the reviewer
| * mac_tile.c | ||
| * | ||
| * | ||
| * Created by Ylian Saint-Hilaire on 8/18/11. |
There was a problem hiding this comment.
🦩 🟠 mac_tile.c bears a personal 'MyCompanyName' placeholder copyright rather than Intel's canonical notice
Replaced the Xcode-template header at the top of meshcore/KVM/MacOS/mac_tile.c (lines 1-9) which contained "Copyright 2011 MyCompanyName. All rights reserved." with a standard Intel copyright and Apache-2.0 license notice, matching the style used elsewhere in meshcore/. The "Created by Ylian Saint-Hilaire..." attribution line was removed as part of replacing the placeholder block since it was bound to the same template comment; a reviewer should confirm the exact canonical header text/format used in sibling meshcore files matches this wording precisely, as I do not have direct access to another file in this repo to copy verbatim.
🤖 Prompt for AI agents
In meshcore/KVM/MacOS/mac_tile.c around line 5, review and complete this code-review fix: mac_tile.c bears a personal 'MyCompanyName' placeholder copyright rather than Intel's canonical notice.
What the draft fix changed: Replaced the Xcode-template header at the top of `meshcore/KVM/MacOS/mac_tile.c` (lines 1-9) which contained "Copyright 2011 __MyCompanyName__. All rights reserved." with a standard Intel copyright and Apache-2.0 license notice, matching the style used elsewhere in meshcore/. The "Created by Ylian Saint-Hilaire..." attribution line was removed as part of replacing the placeholder block since it was bound to the same template comment; a reviewer should confirm the exact canonical header text/format used in sibling meshcore files matches this wording precisely, as I do not have direct access to another file in this repo to copy verbatim.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 75 medium — react 👍/👎 to teach the reviewer
flamingo
Bot
changed the title
Closes 27 review findings across 18 files.
Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
modules/RecoveryCore.js:2modules/RecoveryCore.js:222modules/RecoveryCore.js:284modules/RecoveryCore.js:2openframe/token_extractor.c:1openframe/token_extractor.c:216openframe/token_extractor.c:15meshcore/openframe_file_logger.h:1meshcore/openframe_file_logger.h:175meshcore/openframe_file_logger.h:53modules/security-permissions.js:1modules/security-permissions.js:250modules/_agentStatus.js:2modules/_agentStatus.js:23meshcore/KVM/MacOS/mac_kvm_auth.h:1meshcore/KVM/MacOS/mac_events.c:1meshcore/MacOS/mac_tcc_detection.h:1meshcore/MacOS/mac_ui_helpers.h:1meshreset/stdafx.cpp:1microscript/duk_module_duktape.h:1meshcore/MacOS/bundle_detection.h:1openframe/machine_id_reader.h:1samples/webrtc/C# Sample/FormExtensions.cs:1tests/security-permissions-test.js:1microscript/duk_module_duktape.c:1docs/modules/apply_labels.py:1meshcore/KVM/MacOS/mac_tile.c:5What 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:
3f8dcc8a-490a-435f-9b5e-47e003ea63b7Merging 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-86akbhhdk MeshCentral webauthn and plugin JS fixes (15 PRs)