Add mac and windows changes#638
Conversation
Add combined windows and macos changes
There was a problem hiding this comment.
Pull request overview
This pull request adds support for macOS and Windows runners to the harden-runner GitHub Action, which previously only supported Ubuntu (Linux). The changes refactor platform-specific logic into separate modules and add installation and cleanup handlers for the new platforms.
Changes:
- Added
utils.tsmodule with platform-agnostic helper functions - Implemented
installMacosAgentandinstallWindowsAgentfunctions for platform-specific agent installation - Added platform-specific cleanup handlers (
handleMacosCleanup,handleWindowsCleanup,handleLinuxCleanup) - Updated checksum verification to support multiple platforms
- Fixed typo:
confgStr→configStr
Reviewed changes
Copilot reviewed 7 out of 13 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils.ts | New file with platform-agnostic utility functions (isPlatformSupported, chownForFolder, isAgentInstalled, getAnnotationLogs) |
| src/setup.ts | Added platform-specific agent installation logic with switch statement; fixed typo; moved chownForFolder to utils |
| src/install-agent.ts | Added installMacosAgent and installWindowsAgent functions; updated installAgent to return boolean on checksum failure |
| src/index.ts | Updated platform check to use isPlatformSupported function |
| src/common.ts | Updated message constant and getAnnotationLogs usage for multi-platform support |
| src/cleanup.ts | Refactored cleanup logic into platform-specific handlers; improved code style |
| src/checksum.ts | Added platform-specific checksum verification; changed return type to boolean |
| dist/pre/index.js | Rebuilt distribution file (with bundler issues) |
| dist/post/index.js | Rebuilt distribution file (with bundler issues) |
| dist/index.js | Rebuilt distribution file (with bundler issues) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| const variant = process.arch === "x64" ? "amd64" : "arm64"; | ||
| if (variant === "arm64") { | ||
| console.log(ARM64_RUNNER_MESSAGE); |
There was a problem hiding this comment.
There's inconsistent use of console.log and core.info throughout the codebase. For instance, line 159 uses console.log(ARM64_RUNNER_MESSAGE) while other informational messages use core.info(). Consider using core.info() consistently for informational messages to maintain consistency with the rest of the codebase and to leverage GitHub Actions' logging features.
| console.log(ARM64_RUNNER_MESSAGE); | |
| core.info(ARM64_RUNNER_MESSAGE); |
| export function chownForFolder(newOwner: string, target: string) { | ||
| let cmd = "sudo"; | ||
| let args = ["chown", "-R", newOwner, target]; | ||
| cp.execFileSync(cmd, args); |
There was a problem hiding this comment.
The chownForFolder function references 'cp' which is imported as an alias for 'child_process', but in the transpiled dist files (dist/index.js:31899, dist/pre/index.js:85168, dist/post/index.js:31904), it results in 'cp.execFileSync' which will cause a ReferenceError since 'cp' is not defined in the global scope. The bundler is not correctly handling the import alias. Consider using 'external_child_process_' in the dist files or ensuring the build process correctly transpiles this reference.
| cp.execFileSync(cmd, args); | |
| const childProcess = require("child_process"); | |
| childProcess.execFileSync(cmd, args); |
| logFile = "/home/agent/agent.log"; | ||
|
|
||
| cp.execSync("sudo mkdir -p /home/agent"); | ||
| chownForFolder(process.env.USER, "/home/agent"); |
There was a problem hiding this comment.
The chownForFolder function is called with process.env.USER without checking if it's defined. If the USER environment variable is not set, this will pass undefined to the chownForFolder function, which could result in unexpected behavior. Consider adding a check or using a fallback value.
| chownForFolder(process.env.USER, "/home/agent"); | |
| const userForChown = process.env.USER || process.env.SUDO_USER; | |
| if (userForChown) { | |
| chownForFolder(userForChown, "/home/agent"); | |
| } else { | |
| core.warning( | |
| "Unable to determine user for chown; skipping ownership change for /home/agent" | |
| ); | |
| } |
| // Create working directory | ||
| core.info("Creating /opt/step-security directory..."); | ||
| cp.execSync("sudo mkdir -p /opt/step-security"); | ||
| chownForFolder(process.env.USER, "/opt/step-security"); |
There was a problem hiding this comment.
The chownForFolder function is called with process.env.USER without checking if it's defined. If the USER environment variable is not set, this will pass undefined to the chownForFolder function, which could result in unexpected behavior. Consider adding a check or using a fallback value.
| return; | ||
| } | ||
|
|
||
| const pid = parseInt(fs.readFileSync(pidFile, "utf8").trim()); |
There was a problem hiding this comment.
The parseInt() call could return NaN if the PID file contains invalid data. When NaN is passed to process.kill(), it will throw an error. Consider adding validation to check if the parsed PID is a valid number before attempting to use it.
| const pid = parseInt(fs.readFileSync(pidFile, "utf8").trim()); | |
| const pid = parseInt(fs.readFileSync(pidFile, "utf8").trim(), 10); | |
| if (Number.isNaN(pid)) { | |
| console.log("Invalid PID in PID file. Agent may not be running."); | |
| fs.unlinkSync(pidFile); | |
| return; | |
| } |
| export function chownForFolder(newOwner: string, target: string) { | ||
| let cmd = "sudo"; | ||
| let args = ["chown", "-R", newOwner, target]; | ||
| cp.execFileSync(cmd, args); | ||
| } |
There was a problem hiding this comment.
The chownForFolder function executes sudo chown with user-provided parameters (newOwner and target). While process.env.USER is typically safe, consider validating the target path to prevent potential command injection or unauthorized file access if this function is ever called with untrusted input.
| console.log("PID file cleaned up"); | ||
| } | ||
| } catch (error) { | ||
| console.log("warning: error stopping agent process:", error.message); |
There was a problem hiding this comment.
The error.message property is accessed without checking if error is an Error instance. In TypeScript, caught errors have type 'unknown', so accessing .message directly could cause a runtime error if the thrown value is not an Error object. Consider using type guards or optional chaining.
| console.log("warning: error stopping agent process:", error.message); | |
| if (error instanceof Error) { | |
| console.log("warning: error stopping agent process:", error.message); | |
| } else { | |
| console.log("warning: error stopping agent process:", error); | |
| } |
| switch (platform) { | ||
| case "linux": | ||
| expectedChecksum = isTLS | ||
| ? CHECKSUMS["tls"][variant] | ||
| : CHECKSUMS["non_tls"][variant]; | ||
| break; | ||
| case "darwin": | ||
| expectedChecksum = CHECKSUMS["darwin"]; | ||
| break; | ||
| case "win32": | ||
| expectedChecksum = CHECKSUMS["windows"][variant]; | ||
| break; | ||
| default: | ||
| console.log(`Unsupported platform: ${platform}`); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
If the variant is not found in the CHECKSUMS object (e.g., CHECKSUMS["non_tls"]["arm64"] is undefined), expectedChecksum will be undefined or an empty string, which will cause the checksum verification to fail without a clear error message. Consider adding explicit checks for undefined expectedChecksum values and providing a more informative error message indicating that the variant is not supported.
| } catch (error) { | ||
| console.log("Warning: Could not fetch system log stream:", error.message); |
There was a problem hiding this comment.
The error.message property is accessed without checking if error is an Error instance. In TypeScript, caught errors have type 'unknown', so accessing .message directly could cause a runtime error if the thrown value is not an Error object. Consider using type guards or optional chaining.
| } catch (error) { | |
| console.log("Warning: Could not fetch system log stream:", error.message); | |
| } catch (error: unknown) { | |
| const message = | |
| error instanceof Error ? error.message : String(error); | |
| console.log("Warning: Could not fetch system log stream:", message); |
| process.kill(pid, "SIGKILL"); | ||
| console.log("agent process terminated forcefully"); |
There was a problem hiding this comment.
On Windows, SIGINT and SIGKILL signals are emulated by Node.js and may not behave as expected. SIGINT might not work reliably for graceful shutdown, and SIGKILL terminates the Node.js process itself rather than the target process. Consider using taskkill or other Windows-specific process termination methods for more reliable process management on Windows.
| process.kill(pid, "SIGKILL"); | |
| console.log("agent process terminated forcefully"); | |
| if (process.platform === "win32") { | |
| try { | |
| cp.execSync(`taskkill /PID ${pid} /T /F`); | |
| console.log("agent process terminated forcefully via taskkill"); | |
| } catch (taskkillError) { | |
| console.log( | |
| "warning: failed to terminate agent process via taskkill:", | |
| (taskkillError as Error).message | |
| ); | |
| } | |
| } else { | |
| process.kill(pid, "SIGKILL"); | |
| console.log("agent process terminated forcefully"); | |
| } |
src/install-agent.ts
Outdated
| core.info( | ||
| `Copying Installer from ${installerSourcePath} to /opt/step-security...` | ||
| ); | ||
| cp.execSync(`cp "${installerSourcePath}" /opt/step-security/`); |
There was a problem hiding this comment.
cp.execSync(cp "${installerSourcePath}" /opt/step-security/);
Would be slightly safer as cp.execFileSync("cp", [installerSourcePath, "/opt/step-security/"]) to avoid any shell interpretation. The path comes from tc.extractTar
so it's likely safe, but execFileSync is the pattern used elsewhere in this codebase.
No description provided.