Skip to content

fix(MESHAGEN-004): CU-86akbhg5u 3 review findings in macOSHelpers.js #93

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
flamingo wants to merge 1 commit into
base: master
Choose a base branch
from
+89 −33
Draft

fix(MESHAGEN-004): CU-86akbhg5u 3 review findings in macOSHelpers.js #93

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
122 changes: 89 additions & 33 deletions modules/macOSHelpers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,17 @@
/*

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🔴 macOSHelpers.js has a non-conforming, incomplete license/copyright header

Replaced the non-conforming header at the top of the file with the full Apache License 2.0 block comment, including "Copyright 2024 Intel Corporation" and the canonical "http://www.apache.org/licenses/LICENSE-2.0" URL, matching the style used in modules/lib-finder.js, while preserving the trailing description lines.

🤖 Prompt for AI agents 
In modules/macOSHelpers.js around line 1, review and complete this code-review fix: macOSHelpers.js has a non-conforming, incomplete license/copyright header.
What the draft fix changed: Replaced the non-conforming header at the top of the file with the full Apache License 2.0 block comment, including "Copyright 2024 Intel Corporation" and the canonical "http://www.apache.org/licenses/LICENSE-2.0" URL, matching the style used in modules/lib-finder.js, while preserving the trailing description lines.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer

Copyright 2024
Copyright 2024 Intel Corporation

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.

macOS Platform Helper Functions
Centralizes macOS-specific utilities for bundle detection, service naming, and system operations
Expand All @@ -9,7 +21,8 @@ Centralizes macOS-specific utilities for bundle detection, service naming, and s
// CONSTANTS
// ============================================================================

var MACOS_PATHS = {
var MACOS_PATHS =
{
LAUNCH_DAEMONS: '/Library/LaunchDaemons/',
LAUNCH_AGENTS: '/Library/LaunchAgents/',
SYSTEM_LAUNCH_DAEMONS: '/System/Library/LaunchDaemons/',
Expand All @@ -18,12 +31,14 @@ var MACOS_PATHS = {
LAUNCHCTL: '/bin/launchctl'
};

var LAUNCHD_DOMAINS = {
var LAUNCHD_DOMAINS =
{
SYSTEM: 'system',
GUI_PREFIX: 'gui/'
};

var BUNDLE_STRUCTURE = {
var BUNDLE_STRUCTURE =
{
CONTENTS_PATH: '.app/Contents/',
MACOS_PATH: '.app/Contents/MacOS/',
RESOURCES_PATH: '.app/Contents/Resources/'
Expand All @@ -34,8 +49,12 @@ var BUNDLE_STRUCTURE = {
// ============================================================================

// Check if a given path is from an app bundle
function isRunningFromBundle(execPath) {
if (!execPath) execPath = process.execPath;

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 K&R-style opening braces used throughout macOSHelpers.js instead of Allman style

Reformatted all function, if/else, and object-literal opening braces throughout the file (isRunningFromBundle, getBundleParentDirectory, getBundlePathFromBinaryPath, sanitizeIdentifier, buildServiceId, getPlistPath, getLaunchdDomain, getLaunchdPath, copyBundleWithDitto, executePlistBuddy, module.exports, and the MACOS_PATHS/LAUNCHD_DOMAINS/BUNDLE_STRUCTURE object literals) to Allman style, placing each opening brace on its own line, matching modules/lib-finder.js. Single-line if (!execPath) execPath = process.execPath; guards were expanded into full Allman-braced blocks since the finding calls these out explicitly as needing reformatting.

🤖 Prompt for AI agents 
In modules/macOSHelpers.js around line 38, review and complete this code-review fix: K&R-style opening braces used throughout macOSHelpers.js instead of Allman style.
What the draft fix changed: Reformatted all function, if/else, and object-literal opening braces throughout the file (isRunningFromBundle, getBundleParentDirectory, getBundlePathFromBinaryPath, sanitizeIdentifier, buildServiceId, getPlistPath, getLaunchdDomain, getLaunchdPath, copyBundleWithDitto, executePlistBuddy, module.exports, and the MACOS_PATHS/LAUNCHD_DOMAINS/BUNDLE_STRUCTURE object literals) to Allman style, placing each opening brace on its own line, matching modules/lib-finder.js. Single-line `if (!execPath) execPath = process.execPath;` guards were expanded into full Allman-braced blocks since the finding calls these out explicitly as needing reformatting.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 80 medium — react 👍/👎 to teach the reviewer

function isRunningFromBundle(execPath)
{
if (!execPath)
{
execPath = process.execPath;
}

var indexResult = execPath.indexOf('.app/Contents/MacOS/');

Expand All @@ -46,9 +65,16 @@ function isRunningFromBundle(execPath) {

// Extract the parent directory of a bundle (e.g., /opt/meshagent/ from /opt/meshagent/MeshAgent.app/Contents/MacOS/meshagent)
// Returns null if not a bundle path
function getBundleParentDirectory(execPath) {
if (!execPath) execPath = process.execPath;
if (!isRunningFromBundle(execPath)) return null;
function getBundleParentDirectory(execPath)
{
if (!execPath)
{
execPath = process.execPath;
}
if (!isRunningFromBundle(execPath))
{
return null;
}

var parts = execPath.split('.app/Contents/MacOS/')[0].split('/');
parts.pop(); // Remove bundle name
Expand All @@ -58,9 +84,11 @@ function getBundleParentDirectory(execPath) {
// Extract the bundle path from a binary path
// e.g., /opt/meshagent/MeshAgent.app/Contents/MacOS/meshagent -> /opt/meshagent/MeshAgent.app
// Returns null if not a bundle path
function getBundlePathFromBinaryPath(binaryPath) {
function getBundlePathFromBinaryPath(binaryPath)
{

if (!isRunningFromBundle(binaryPath)) {
if (!isRunningFromBundle(binaryPath))
{
return null;
}

Expand All @@ -74,8 +102,12 @@ function getBundlePathFromBinaryPath(binaryPath) {

// Sanitize identifier to follow reverse DNS naming conventions
// Only allow alphanumeric, hyphens, and underscores (dots will be added between components)
function sanitizeIdentifier(str) {
if (!str) return null;
function sanitizeIdentifier(str)
{
if (!str)
{
return null;
}
// Replace spaces with hyphens, remove all non-alphanumeric except hyphens/underscores, convert to lowercase
return str.replace(/\s+/g, '-').replace(/[^a-zA-Z0-9_-]/g, '').toLowerCase();
}
Expand All @@ -88,38 +120,49 @@ function sanitizeIdentifier(str) {
// - meshagent.ServiceName (custom service name only)
// - meshagent (default service name only)
// - ServiceName (non-macOS platforms)
function buildServiceId(serviceName, companyName, options) {
function buildServiceId(serviceName, companyName, options)
{
options = options || {};
var platform = options.platform || process.platform;
var explicitServiceId = options.explicitServiceId || null;

// If an explicit serviceId is provided, use it directly
if (explicitServiceId !== null) {
if (explicitServiceId !== null)
{
return explicitServiceId;
}

// Non-macOS platforms use simple sanitized identifier
if (platform !== 'darwin') {
if (platform !== 'darwin')
{
return sanitizeIdentifier(serviceName);
}

// macOS composite identifier logic
var sanitizedServiceName = sanitizeIdentifier(serviceName);
var sanitizedCompanyName = sanitizeIdentifier(companyName);

if (sanitizedCompanyName) {
if (sanitizedCompanyName)
{
// Company name present
if (sanitizedServiceName && sanitizedServiceName !== 'meshagent') {
if (sanitizedServiceName && sanitizedServiceName !== 'meshagent')
{
// Custom service name + company: meshagent.ServiceName.CompanyName
return 'meshagent.' + sanitizedServiceName + '.' + sanitizedCompanyName;
} else {
}
else
{
// Default service name + company: meshagent.CompanyName
return 'meshagent.' + sanitizedCompanyName;
}
} else if (sanitizedServiceName && sanitizedServiceName !== 'meshagent') {
}
else if (sanitizedServiceName && sanitizedServiceName !== 'meshagent')
{
// Only custom service name (no company): meshagent.ServiceName
return 'meshagent.' + sanitizedServiceName;
} else {
}
else
{
// Default service name only: meshagent
return 'meshagent';
}
Expand All @@ -131,10 +174,14 @@ function buildServiceId(serviceName, companyName, options) {

// Get the plist path for a given service ID and type
// type: 'daemon' for LaunchDaemon, 'agent' for LaunchAgent
function getPlistPath(serviceId, type) {
if (type === 'daemon') {
function getPlistPath(serviceId, type)
{
if (type === 'daemon')
{
return MACOS_PATHS.LAUNCH_DAEMONS + serviceId + '.plist';
} else if (type === 'agent') {
}
else if (type === 'agent')
{
return MACOS_PATHS.LAUNCH_AGENTS + serviceId + '-agent.plist';
}
return null;
Expand All @@ -146,16 +193,19 @@ function getPlistPath(serviceId, type) {

// Get the launchd domain for a given UID
// Returns 'system' for system domain (uid=null), or 'gui/{uid}' for user domain
function getLaunchdDomain(uid) {
if (uid === null || uid === undefined) {
function getLaunchdDomain(uid)
{
if (uid === null || uid === undefined)
{
return LAUNCHD_DOMAINS.SYSTEM;
}
return LAUNCHD_DOMAINS.GUI_PREFIX + uid;
}

// Build a launchd service path in the format 'domain/serviceId'
// Used for launchctl commands like 'launchctl print system/meshagent'
function getLaunchdPath(domain, serviceId) {
function getLaunchdPath(domain, serviceId)
{
return domain + '/' + serviceId;
}

Expand All @@ -165,22 +215,25 @@ function getLaunchdPath(domain, serviceId) {

// Copy an app bundle using ditto (preserves all macOS metadata, signatures, etc.)
// Returns true on success, throws error on failure
function copyBundleWithDitto(sourcePath, targetPath) {
function copyBundleWithDitto(sourcePath, targetPath)
{
var child_process = require('child_process');
var fs = require('fs');

var dittoError = null;
var child = child_process.execFile(MACOS_PATHS.DITTO, ['ditto', sourcePath, targetPath]);

child.stderr.on('data', function(d) {
child.stderr.on('data', function(d)
{
dittoError = d.toString();
process.stderr.write(d);
});

child.waitExit();

// Verify the copy succeeded by checking if target exists
if (dittoError || !fs.existsSync(targetPath)) {
if (dittoError || !fs.existsSync(targetPath))
{
throw new Error('Bundle copy failed: ' + (dittoError || 'Target not created'));
}

Expand All @@ -189,9 +242,10 @@ function copyBundleWithDitto(sourcePath, targetPath) {

// Execute PlistBuddy command on a plist file
// Returns the output string, throws on error
function executePlistBuddy(command, plistPath) {
function executePlistBuddy(command, plistPath)
{
var child_process = require('child_process');
return child_process.execSync(MACOS_PATHS.PLIST_BUDDY + ' -c "' + command + '" "' + plistPath + '"', {
return child_process.execFileSync(MACOS_PATHS.PLIST_BUDDY, ['-c', command, plistPath], {
encoding: 'utf8'
}).trim();
}
Expand All @@ -200,7 +254,8 @@ function executePlistBuddy(command, plistPath) {
// EXPORTS
// ============================================================================

module.exports = {
module.exports =
{
// Constants
PATHS: MACOS_PATHS,

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 executePlistBuddy builds a shell command string via string concatenation with unsanitized command/path arguments

Changed executePlistBuddy to use child_process.execFileSync(MACOS_PATHS.PLIST_BUDDY, ['-c', command, plistPath], {...}) instead of building a quoted shell string for execSync, eliminating shell interpretation of the command/plistPath arguments and closing the injection vector. This assumes execFileSync is acceptable in this codebase's runtime (it mirrors the execFile usage already present in copyBundleWithDitto) and that no caller relies on shell features (e.g. globbing) previously enabled by the string form; callers were not exhaustively audited across the repository, so behavior for any edge-case shell-dependent usage should be verified.

🤖 Prompt for AI agents 
In modules/macOSHelpers.js around line 205, review and complete this code-review fix: executePlistBuddy builds a shell command string via string concatenation with unsanitized command/path arguments.
What the draft fix changed: Changed executePlistBuddy to use child_process.execFileSync(MACOS_PATHS.PLIST_BUDDY, ['-c', command, plistPath], {...}) instead of building a quoted shell string for execSync, eliminating shell interpretation of the command/plistPath arguments and closing the injection vector. This assumes execFileSync is acceptable in this codebase's runtime (it mirrors the execFile usage already present in copyBundleWithDitto) and that no caller relies on shell features (e.g. globbing) previously enabled by the string form; callers were not exhaustively audited across the repository, so behavior for any edge-case shell-dependent usage should be verified.
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

DOMAINS: LAUNCHD_DOMAINS,
Expand All @@ -226,3 +281,4 @@ module.exports = {
copyBundleWithDitto: copyBundleWithDitto,
executePlistBuddy: executePlistBuddy
};