enable executing from native PS #28
Conversation
WalkthroughThe pull request introduces a new feature across two PowerShell-related modules: Changes
Sequence DiagramsequenceDiagram
participant User
participant PowerShellMethod
participant NativeShell
participant ScriptFile
participant OutputCapture
User->>PowerShellMethod: Invoke with ExecuteNativeShell=true
PowerShellMethod->>ScriptFile: Create temporary script
PowerShellMethod->>NativeShell: Execute script via powershell.exe
NativeShell-->>OutputCapture: Capture stdout/stderr
NativeShell->>ScriptFile: Delete temporary script
OutputCapture-->>User: Return execution results
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (7)
Frends.PowerShell.RunScript/CHANGELOG.md (1)
4-5: Enhance changelog clarity and fix word repetition.The changelog entry could be more descriptive and has unnecessary word repetition.
### Added -Added option to run script using native PowerShell +Option to execute scripts using native PowerShell shell, providing better compatibility and performance when running on PowerShell-enabled systems🧰 Tools
🪛 LanguageTool
[duplication] ~4-~4: Possible typo: you repeated a word
Context: ... Changelog ## [1.3.0] - 2025-01-08 ### Added - Added option to run script using native Power...(ENGLISH_WORD_REPEAT_RULE)
Frends.PowerShell.RunScript/Frends.PowerShell.RunScript/Definitions/RunScriptInput.cs (1)
46-52: Enhance documentation for ExecuteNativeShell propertyThe documentation should clarify:
- What "basic usage" means in this context
- The implications of using native shell (e.g., performance, security considerations)
- Any prerequisites or requirements for native shell execution
Consider expanding the documentation:
/// <summary> /// Define which version of PowerShell to use. - /// Useful in more complex scripts, as default version supports only basic usage + /// When set to true, executes the script using native PowerShell process instead of the PowerShell runtime. + /// Useful for complex scripts that require full PowerShell capabilities or specific PowerShell versions. + /// Note: Requires PowerShell to be installed on the system. /// </summary>Frends.PowerShell.RunScript/Frends.PowerShell.RunScript.Tests/UnitTests.cs (1)
32-53: Enhance test coverage for native shell executionWhile the basic functionality is tested, consider adding test cases for:
- Error handling in native shell
- Complex scripts that specifically require native shell
- Working directory specification
- PowerShell version selection (if applicable)
Would you like me to help generate additional test cases to improve coverage?
Frends.PowerShell.RunCommand/CHANGELOG.md (1)
3-5: Enhance changelog entry with more detailsThe changelog entry could be more descriptive about the feature and its implications.
Consider expanding the entry:
## [1.3.0] - 2025-01-08 ### Added -Added option to run command using native PowerShell +- Added `ExecuteNativeShell` option to enable native PowerShell execution +- Supports complex scripts requiring full PowerShell capabilities +- Requires PowerShell to be installed on the target system🧰 Tools
🪛 LanguageTool
[duplication] ~4-~4: Possible typo: you repeated a word
Context: ... Changelog ## [1.3.0] - 2025-01-08 ### Added - Added option to run command using native Powe...(ENGLISH_WORD_REPEAT_RULE)
Frends.PowerShell.RunCommand/Frends.PowerShell.RunCommand/Frends.PowerShell.RunCommand.csproj (1)
10-10: Update package description to reflect native shell supportThe description should mention the new native shell execution capability.
Consider updating the description:
- <Description>Task for running commands on PowerShell. Requires PowerShell 7.0 to work.</Description> + <Description>Task for running commands on PowerShell with support for both runtime and native shell execution. Requires PowerShell 7.0 or later to work.</Description>Also applies to: 19-19
Frends.PowerShell.RunCommand/Frends.PowerShell.RunCommand.Tests/UnitTests.cs (1)
67-67: Rename test method to match the functionality being testedThe test method name
RunScript_ShouldRunFromNativeShellsuggests it's testing script execution, but it's actually testing command execution usingRunCommand. Consider renaming toRunCommand_ShouldRunFromNativeShellfor clarity.- public void RunScript_ShouldRunFromNativeShell() + public void RunCommand_ShouldRunFromNativeShell()Frends.PowerShell.RunCommand/Frends.PowerShell.RunCommand/Definitions/RunCommandInput.cs (1)
30-36: Clarify Documentation forExecuteNativeShellPropertyThe XML documentation for
ExecuteNativeShellcould be improved to clearly explain its purpose and usage.Consider updating the summary to provide more detailed information:
/// <summary> -/// Define which version of PowerShell to use. -/// Useful in more complex scripts, as default version supports only basic usage +/// If true, executes the command using the native PowerShell shell (`powershell.exe`). +/// This allows for executing scripts that require features not supported by the default runtime. /// </summary>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
Frends.PowerShell.RunCommand/CHANGELOG.md(1 hunks)Frends.PowerShell.RunCommand/Frends.PowerShell.RunCommand.Tests/UnitTests.cs(1 hunks)Frends.PowerShell.RunCommand/Frends.PowerShell.RunCommand/Definitions/RunCommandInput.cs(1 hunks)Frends.PowerShell.RunCommand/Frends.PowerShell.RunCommand/Frends.PowerShell.RunCommand.csproj(1 hunks)Frends.PowerShell.RunCommand/Frends.PowerShell.RunCommand/PowerShell.cs(2 hunks)Frends.PowerShell.RunScript/CHANGELOG.md(1 hunks)Frends.PowerShell.RunScript/Frends.PowerShell.RunScript.Tests/UnitTests.cs(1 hunks)Frends.PowerShell.RunScript/Frends.PowerShell.RunScript/Definitions/RunScriptInput.cs(1 hunks)Frends.PowerShell.RunScript/Frends.PowerShell.RunScript/Frends.PowerShell.RunScript.csproj(1 hunks)Frends.PowerShell.RunScript/Frends.PowerShell.RunScript/PowerShell.cs(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- Frends.PowerShell.RunScript/Frends.PowerShell.RunScript/Frends.PowerShell.RunScript.csproj
🧰 Additional context used
🪛 LanguageTool
Frends.PowerShell.RunScript/CHANGELOG.md
[duplication] ~4-~4: Possible typo: you repeated a word
Context: ... Changelog ## [1.3.0] - 2025-01-08 ### Added - Added option to run script using native Power...
(ENGLISH_WORD_REPEAT_RULE)
Frends.PowerShell.RunCommand/CHANGELOG.md
[duplication] ~4-~4: Possible typo: you repeated a word
Context: ... Changelog ## [1.3.0] - 2025-01-08 ### Added - Added option to run command using native Powe...
(ENGLISH_WORD_REPEAT_RULE)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (csharp)
- GitHub Check: build / Build on windows-latest
- GitHub Check: build / Build on windows-latest
🔇 Additional comments (4)
Frends.PowerShell.RunScript/CHANGELOG.md (1)
3-6: LGTM! Changelog entry aligns with PR objectives.The changelog entry accurately reflects the PR's goal of enabling native PowerShell execution, maintaining consistency with similar changes in the
Frends.PowerShell.RunCommandmodule.🧰 Tools
🪛 LanguageTool
[duplication] ~4-~4: Possible typo: you repeated a word
Context: ... Changelog ## [1.3.0] - 2025-01-08 ### Added - Added option to run script using native Power...(ENGLISH_WORD_REPEAT_RULE)
Frends.PowerShell.RunCommand/Frends.PowerShell.RunCommand.Tests/UnitTests.cs (1)
80-80: Verify native shell execution behaviorThe
ExecuteNativeShellflag is set but we should verify it actually uses the native PowerShell process.✅ Verification successful
Native shell execution is properly implemented
The implementation correctly uses
powershell.exeprocess whenExecuteNativeShellis true, with appropriate execution policy and profile settings. The code handles parameter passing, output/error capturing, and cleanup properly.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the implementation to understand how native shell execution is handled ast-grep --pattern 'ExecuteNativeShell = $_' # Look for process creation or shell execution rg -l 'Process|Shell|powershell.exe'Length of output: 2388
Script:
#!/bin/bash # Check the implementation in PowerShell.cs cat "Frends.PowerShell.RunCommand/Frends.PowerShell.RunCommand/PowerShell.cs" # Check RunOptions definition cat "Frends.PowerShell.RunCommand/Frends.PowerShell.RunCommand/Definitions/RunOptions.cs"Length of output: 7642
Frends.PowerShell.RunCommand/Frends.PowerShell.RunCommand/PowerShell.cs (1)
51-52:⚠️ Potential issueParameters Not Passed to PowerShell Process
The
parameterStringis constructed but not included in theArgumentswhen starting the PowerShell process. As a result, the script may not receive the intended parameters.Include
parameterStringin theArgumentsto ensure parameters are passed correctly:-Arguments = $"-NoProfile -ExecutionPolicy Bypass -File \"{scriptPath}\" ", +Arguments = $"-NoProfile -ExecutionPolicy Bypass -File \"{scriptPath}\" {parameterString}",Likely invalid or redundant comment.
Frends.PowerShell.RunScript/Frends.PowerShell.RunScript/PowerShell.cs (1)
75-76: Inconsistent Use ofCreateNoWindowandUseShellExecuteWhen
UseShellExecuteis set tofalse,CreateNoWindowshould typically betrue. Verify if this is the intended behavior.Ensure that the process window behavior aligns with the application's requirements.
Frends.PowerShell.RunCommand/Frends.PowerShell.RunCommand.Tests/UnitTests.cs
Show resolved
Hide resolved
Frends.PowerShell.RunCommand/Frends.PowerShell.RunCommand.Tests/UnitTests.cs
Show resolved
Hide resolved
| parameterString += $"-{param.Name} \"{param.Value}\" "; | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential Command Injection Risk in Parameter Concatenation
The parameters are concatenated into parameterString without proper sanitization, potentially leading to command injection vulnerabilities if param.Name or param.Value contain malicious input.
Properly escape or validate parameters before including them in the Arguments. Suggested fix:
foreach (var param in parameters)
{
- parameterString += $"-{param.Name} \"{param.Value}\" ";
+ parameterString += $"-{EscapeArgument(param.Name)} {EscapeArgument(param.Value.ToString())} ";
}
+string EscapeArgument(string argument)
+{
+ return "\"" + argument.Replace("\"", "`\"") + "\"";
+}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
Frends.PowerShell.RunCommand/Frends.PowerShell.RunCommand/PowerShell.cs (2)
44-51:⚠️ Potential issueFix command injection vulnerability in parameter handling
Direct string concatenation of parameters creates a command injection vulnerability.
Replace the parameter handling with proper PowerShell parameter escaping:
-string parameterString = string.Empty; -if (input.Parameters != null) -{ - foreach (var param in input.Parameters) - { - parameterString += $"-{param.Name} \"{param.Value}\" "; - } -} +var parameterString = input.Parameters == null ? string.Empty : + string.Join(" ", input.Parameters.Select(p => + $"-{EscapeArgument(p.Name)} {EscapeArgument(p.Value?.ToString() ?? string.Empty)}")); +private static string EscapeArgument(string value) +{ + if (string.IsNullOrEmpty(value)) return "\"\""; + // Escape any double quotes and wrap the entire string + return "\"" + value.Replace("\"", "`\"") + "\""; +}
94-108:⚠️ Potential issueAdd proper process cleanup and timeout handling
The process handling needs improvement to prevent resource leaks and handle long-running scripts.
Enhance the process execution with timeout and proper cleanup:
try { process.Start(); process.BeginOutputReadLine(); process.BeginErrorReadLine(); - process.WaitForExit(); + // Add timeout handling (e.g., 5 minutes) + if (!process.WaitForExit(300000)) + { + throw new TimeoutException("Script execution timed out after 5 minutes"); + } + + if (process.ExitCode != 0) + { + errors.Add($"Script execution failed with exit code: {process.ExitCode}"); + } } catch (Exception ex) { errors.Add($"Exception occurred while running the script: {ex.Message}"); } finally { + try + { + if (!process.HasExited) + { + process.Kill(entireProcessTree: true); + } + } + catch (Exception ex) + { + errors.Add($"Failed to terminate process: {ex.Message}"); + } process.Close(); }
🧹 Nitpick comments (1)
Frends.PowerShell.RunCommand/Frends.PowerShell.RunCommand/PowerShell.cs (1)
74-75: Consider PowerShell Core compatibilityThe code assumes Windows PowerShell by using
powershell.exe.Consider making the PowerShell executable path configurable to support both Windows PowerShell and PowerShell Core:
+private static string GetPowerShellPath() +{ + // Check for pwsh (PowerShell Core) first + var pwshPath = Path.Combine( + Environment.GetFolderPath(Environment.SpecialFolder.ProgramFiles), + "PowerShell", "7", "pwsh.exe"); + return File.Exists(pwshPath) ? pwshPath : "powershell.exe"; +} -FileName = "powershell.exe", +FileName = GetPowerShellPath(),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Frends.PowerShell.RunCommand/Frends.PowerShell.RunCommand.Tests/TaskUserInterfaceTests.cs(1 hunks)Frends.PowerShell.RunCommand/Frends.PowerShell.RunCommand/PowerShell.cs(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- Frends.PowerShell.RunCommand/Frends.PowerShell.RunCommand.Tests/TaskUserInterfaceTests.cs
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build / Code Quality Check
- GitHub Check: build / Build on windows-latest
- GitHub Check: Analyze (csharp)
Frends.PowerShell.RunCommand/Frends.PowerShell.RunCommand/PowerShell.cs
Outdated
Show resolved
Hide resolved
Frends.PowerShell.RunCommand/Frends.PowerShell.RunCommand/PowerShell.cs
Outdated
Show resolved
Hide resolved
Frends.PowerShell.RunScript/Frends.PowerShell.RunScript/PowerShell.cs
Outdated
Show resolved
Hide resolved
Frends.PowerShell.RunScript/Frends.PowerShell.RunScript/PowerShell.cs
Outdated
Show resolved
Hide resolved
Frends.PowerShell.RunCommand/Frends.PowerShell.RunCommand/PowerShell.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (9)
Frends.PowerShell.RunScript/Frends.PowerShell.RunScript/PowerShell.cs (3)
63-70:⚠️ Potential issueSecurity Risk: Unsafe Parameter Handling
The parameter string construction is vulnerable to command injection attacks.
Previous review already suggested proper parameter escaping. Please implement the suggested fix to prevent command injection vulnerabilities.
72-81:⚠️ Potential issueCross-Platform Compatibility Issue
The hardcoded
powershell.exefilename won't work on Linux systems where the executable is typicallypwsh.Add platform-specific executable selection:
- FileName = "powershell.exe", + FileName = RuntimeInformation.IsOSPlatform(OSPlatform.Windows) ? "powershell.exe" : "pwsh",Don't forget to add:
using System.Runtime.InteropServices;
83-93:⚠️ Potential issueResource Leak: Event Handler Cleanup
Event handlers should be unwired in the finally block to prevent memory leaks.
Refactor the code to properly clean up event handlers:
try { process.Start(); process.BeginOutputReadLine(); process.BeginErrorReadLine(); process.WaitForExit(); } catch (Exception ex) { errors.Add($"Exception occurred while running the script: {ex.Message}"); } finally { + process.OutputDataReceived -= outputHandler; + process.ErrorDataReceived -= errorHandler; if (!process.HasExited) process.Kill(); process.Close(); }Move the event handler declarations outside the registration:
DataReceivedEventHandler outputHandler = (sender, args) => { if (!string.IsNullOrWhiteSpace(args.Data)) results.Add(args.Data); }; process.OutputDataReceived += outputHandler; DataReceivedEventHandler errorHandler = (sender, args) => { if (!string.IsNullOrWhiteSpace(args.Data)) errors.Add(args.Data); }; process.ErrorDataReceived += errorHandler;Also applies to: 95-110
.github/workflows/RunCommand_build_and_test_on_push.yml (1)
10-11:⚠️ Potential issueFix workflow_dispatch trigger syntax
Same issue as in RunScript workflow - the
workflow_dispatchevent trigger doesn't support path filters.Apply this diff to fix the workflow syntax:
workflow_dispatch: - paths: - - "Frends.PowerShell.RunCommand/**"🧰 Tools
🪛 actionlint (1.7.4)
10-10: expected "inputs" key for "workflow_dispatch" section but got "paths"
(syntax-check)
Frends.PowerShell.RunCommand/Frends.PowerShell.RunCommand/PowerShell.cs (5)
41-43:⚠️ Potential issueImprove temporary file security and cleanup
The temporary file handling needs improvement to prevent race conditions and ensure secure cleanup:
- Use secure file creation with restricted access
- Consider using
FileOptions.DeleteOnClose- Implement proper error handling for file operations
Previous review comment about improving temporary file security is still valid. Additionally, ensure proper error handling for file operations:
-var tempScript = Path.Combine(Path.GetTempPath(), $"{Path.GetRandomFileName()}.ps1"); -try -{ +string tempScript = null; +try +{ + // Create temp file with restricted access + tempScript = Path.Combine(Path.GetTempPath(), $"{Path.GetRandomFileName()}.ps1"); + using (var fs = new FileStream(tempScript, FileMode.CreateNew, + FileAccess.Write, FileShare.None, + 4096, FileOptions.DeleteOnClose | FileOptions.Encrypted)) + using (var sw = new StreamWriter(fs, Encoding.UTF8)) + { + sw.Write($"{input.Command} {parameterString}"); + }Also applies to: 57-58
44-52:⚠️ Potential issueFix command injection vulnerability in parameter handling
The current parameter concatenation is vulnerable to command injection attacks. Parameters need proper escaping.
Previous review comment about parameter escaping is still valid. Additionally:
- Consider using PowerShell's native parameter binding
- Implement proper parameter validation
- string parameterString = string.Empty; - if (input.Parameters != null) - { - foreach (var param in input.Parameters) - { - parameterString += $"-{param.Name} \"{param.Value}\" "; - } - } - File.WriteAllText(tempScript, $"{input.Command} {parameterString}", Encoding.UTF8); + var sb = new StringBuilder(); + sb.Append(EscapeCommand(input.Command)); + if (input.Parameters != null) + { + foreach (var param in input.Parameters) + { + if (string.IsNullOrWhiteSpace(param.Name)) + throw new ArgumentException("Parameter name cannot be empty"); + sb.Append(' ').Append('-').Append(EscapeParameterName(param.Name)); + if (param.Value != null) + sb.Append(' ').Append(EscapeParameterValue(param.Value.ToString())); + } + } + await File.WriteAllTextAsync(tempScript, sb.ToString(), new UTF8Encoding(false)); + +private static string EscapeCommand(string command) => + string.IsNullOrWhiteSpace(command) + ? throw new ArgumentException("Command cannot be empty") + : command.Replace("\"", "`\"").Replace(";", "`;"); + +private static string EscapeParameterName(string name) => + name.TrimStart('-').Replace("\"", "").Replace(";", ""); + +private static string EscapeParameterValue(string value) => + $"\"{value.Replace("\"", "`\"").Replace(";", "`;")}\"";
74-80:⚠️ Potential issueImprove process security and cross-platform support
The process configuration needs security enhancements and cross-platform support:
- Platform-specific executable name
- Working directory handling
- Environment isolation
Previous review comments about process configuration and platform support are still valid. Additionally:
+ private static string GetPowerShellExecutable() => + OperatingSystem.IsWindows() ? "powershell.exe" : "pwsh"; process.StartInfo = new() { - FileName = "powershell.exe", + FileName = GetPowerShellExecutable(), Arguments = $"-NoProfile -ExecutionPolicy Bypass -File \"{scriptPath}\" ", UseShellExecute = false, CreateNoWindow = true, RedirectStandardError = true, RedirectStandardOutput = true, + WorkingDirectory = Path.GetDirectoryName(scriptPath), + LoadUserProfile = false, }; + // Prevent injection via environment variables + process.StartInfo.Environment.Clear(); + process.StartInfo.Environment["PSModulePath"] = string.Empty;
66-67: 🛠️ Refactor suggestionAdd bounds to result collections
Unbounded collections could lead to memory issues with large output.
Previous review comment about adding bounds to result collections is still valid.
82-92:⚠️ Potential issueImprove event handler cleanup and process management
Event handlers need to be properly unwired to prevent memory leaks, and process cleanup needs to be robust.
Previous review comment about event handler cleanup is still valid. Additionally:
+ DataReceivedEventHandler outputHandler = null; + DataReceivedEventHandler errorHandler = null; + try + { + outputHandler = (sender, args) => + { + if (!string.IsNullOrWhiteSpace(args.Data)) + results.Add(args.Data); + }; + errorHandler = (sender, args) => + { + if (!string.IsNullOrWhiteSpace(args.Data)) + errors.Add(args.Data); + }; + + process.OutputDataReceived += outputHandler; + process.ErrorDataReceived += errorHandler; // ... rest of the process execution ... } finally { + if (outputHandler != null) + process.OutputDataReceived -= outputHandler; + if (errorHandler != null) + process.ErrorDataReceived -= errorHandler; if (!process.HasExited) process.Kill(); process.Close(); }Also applies to: 105-109
🧹 Nitpick comments (1)
Frends.PowerShell.RunScript/Frends.PowerShell.RunScript/PowerShell.cs (1)
112-112: Inconsistent Logging Between Execution ModesThe native shell execution mode returns an empty string for the Log property, while the managed execution mode includes information stream logs. This inconsistency might affect monitoring and debugging capabilities.
Consider capturing and returning the complete console output as the log string:
- return new PowerShellResult(results, errors, string.Empty); + var log = string.Join("\n", results.Concat(errors)); + return new PowerShellResult(results, errors, log);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/RunCommand_build_and_test_on_push.yml(1 hunks).github/workflows/RunScript_build_and_test_on_push.yml(1 hunks)Frends.PowerShell.RunCommand/Frends.PowerShell.RunCommand/PowerShell.cs(3 hunks)Frends.PowerShell.RunScript/Frends.PowerShell.RunScript.Tests/TaskUserInterfaceTests.cs(1 hunks)Frends.PowerShell.RunScript/Frends.PowerShell.RunScript.Tests/UnitTests.cs(1 hunks)Frends.PowerShell.RunScript/Frends.PowerShell.RunScript/PowerShell.cs(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- Frends.PowerShell.RunScript/Frends.PowerShell.RunScript.Tests/TaskUserInterfaceTests.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- Frends.PowerShell.RunScript/Frends.PowerShell.RunScript.Tests/UnitTests.cs
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/RunScript_build_and_test_on_push.yml
10-10: expected "inputs" key for "workflow_dispatch" section but got "paths"
(syntax-check)
.github/workflows/RunCommand_build_and_test_on_push.yml
10-10: expected "inputs" key for "workflow_dispatch" section but got "paths"
(syntax-check)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (4)
Frends.PowerShell.RunScript/Frends.PowerShell.RunScript/PowerShell.cs (1)
46-49: LGTM! Clean implementation of the execution mode switch.The conditional logic clearly separates the two execution paths while maintaining proper cleanup of temporary files.
.github/workflows/RunScript_build_and_test_on_push.yml (2)
14-20: LGTM! Clear job naming for Windows buildThe rename from
buildtobuild-windowsimproves clarity now that we're supporting multiple platforms.
21-27: Verify Linux workflow file existenceThe addition of Linux support is good, but we need to ensure the referenced workflow file exists.
✅ Verification successful
Linux workflow file verified successfully ✅
The referenced workflow file exists in the FrendsPlatform/FrendsTasks repository and is compatible with all the required parameters and secrets being passed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the referenced workflow file exists in the FrendsPlatform/FrendsTasks repository gh api \ -H "Accept: application/vnd.github+json" \ repos/FrendsPlatform/FrendsTasks/contents/.github/workflows/linux_build_test.ymlLength of output: 14590
.github/workflows/RunCommand_build_and_test_on_push.yml (1)
14-20: LGTM! Consistent multi-platform supportThe changes mirror those in the RunScript workflow, maintaining consistency across both PowerShell modules:
- Clear Windows/Linux job naming
- Same workflow file references
- Consistent secret usage
Also applies to: 21-27
| paths: | ||
| - 'Frends.PowerShell.RunScript/**' | ||
|
|
||
| - "Frends.PowerShell.RunScript/**" |
There was a problem hiding this comment.
Fix workflow_dispatch trigger syntax
The workflow_dispatch event trigger doesn't support path filters. These should be moved to inputs if manual input is needed, or removed if not required.
Apply this diff to fix the workflow syntax:
workflow_dispatch:
- paths:
- - "Frends.PowerShell.RunScript/**"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| paths: | |
| - 'Frends.PowerShell.RunScript/**' | |
| - "Frends.PowerShell.RunScript/**" | |
| workflow_dispatch: |
🧰 Tools
🪛 actionlint (1.7.4)
10-10: expected "inputs" key for "workflow_dispatch" section but got "paths"
(syntax-check)
| try | ||
| { | ||
| process.Start(); | ||
| process.BeginOutputReadLine(); | ||
| process.BeginErrorReadLine(); | ||
| process.WaitForExit(); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| errors.Add($"Exception occurred while running the script: {ex.Message}"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and process timeout
The process execution needs timeout handling and better error reporting.
+ using var cts = new CancellationTokenSource(TimeSpan.FromMinutes(5)); // 5-minute timeout
try
{
process.Start();
process.BeginOutputReadLine();
process.BeginErrorReadLine();
- process.WaitForExit();
+ if (!process.WaitForExit(300000)) // 5-minute timeout
+ {
+ throw new TimeoutException("Process execution timed out after 5 minutes");
+ }
+ if (process.ExitCode != 0)
+ {
+ throw new Exception($"Process exited with code {process.ExitCode}");
+ }
}
catch (Exception ex)
{
- errors.Add($"Exception occurred while running the script: {ex.Message}");
+ errors.Add($"Exception occurred while running the script: {ex.GetType().Name}: {ex.Message}");
+ if (ex is TimeoutException)
+ {
+ process.Kill(entireProcessTree: true);
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try | |
| { | |
| process.Start(); | |
| process.BeginOutputReadLine(); | |
| process.BeginErrorReadLine(); | |
| process.WaitForExit(); | |
| } | |
| catch (Exception ex) | |
| { | |
| errors.Add($"Exception occurred while running the script: {ex.Message}"); | |
| } | |
| using var cts = new CancellationTokenSource(TimeSpan.FromMinutes(5)); // 5-minute timeout | |
| try | |
| { | |
| process.Start(); | |
| process.BeginOutputReadLine(); | |
| process.BeginErrorReadLine(); | |
| if (!process.WaitForExit(300000)) // 5-minute timeout | |
| { | |
| throw new TimeoutException("Process execution timed out after 5 minutes"); | |
| } | |
| if (process.ExitCode != 0) | |
| { | |
| throw new Exception($"Process exited with code {process.ExitCode}"); | |
| } | |
| } | |
| catch (Exception ex) | |
| { | |
| errors.Add($"Exception occurred while running the script: {ex.GetType().Name}: {ex.Message}"); | |
| if (ex is TimeoutException) | |
| { | |
| process.Kill(entireProcessTree: true); | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
Frends.PowerShell.RunCommand/Frends.PowerShell.RunCommand/PowerShell.cs (4)
40-44:⚠️ Potential issueAdd input validation before script creation.
The code should validate the input before creating temporary files to prevent security issues.
Apply input validation at the start of the conditional block:
if (input.ExecuteNativeShell) { + if (string.IsNullOrWhiteSpace(input.Command)) + throw new ArgumentException("Command cannot be empty when executing from native shell"); + if (input.Parameters?.Any(p => string.IsNullOrWhiteSpace(p.Name)) == true) + throw new ArgumentException("Parameter names cannot be empty"); var tempScript = Path.Combine(Path.GetTempPath(), $"{Path.GetRandomFileName()}.ps1");
45-54:⚠️ Potential issueFix command injection vulnerability and improve temporary file security.
The current implementation has two security issues:
- Parameters are concatenated without proper escaping
- Temporary file creation is not secure
Apply these security improvements:
- string parameterString = string.Empty; - if (input.Parameters != null) - { - foreach (var param in input.Parameters) - { - parameterString += $"-{param.Name} \"{param.Value}\" "; - } - } - File.WriteAllText(tempScript, $"{input.Command} {parameterString}", Encoding.UTF8); + string parameterString = input.Parameters == null ? string.Empty : + string.Join(" ", input.Parameters.Select(p => + $"-{EscapeArgument(p.Name)} {EscapeArgument(p.Value?.ToString() ?? string.Empty)}")); + + using (var fs = new FileStream(tempScript, FileMode.CreateNew, + FileAccess.Write, FileShare.None, + 4096, FileOptions.DeleteOnClose | FileOptions.Encrypted)) + using (var sw = new StreamWriter(fs, Encoding.UTF8)) + { + sw.Write($"{input.Command} {parameterString}"); + } +private static string EscapeArgument(string argument) +{ + return "\"" + argument.Replace("\"", "`\"") + "\""; +}
70-79: 🛠️ Refactor suggestionImprove process configuration and security.
The process configuration needs additional security settings and proper working directory handling.
using Process process = new(); process.StartInfo = new() { FileName = RuntimeInformation.IsOSPlatform(OSPlatform.Windows) ? "powershell" : "pwsh", Arguments = $"-NoProfile -ExecutionPolicy Bypass -File \"{scriptPath}\" ", UseShellExecute = false, CreateNoWindow = true, RedirectStandardError = true, RedirectStandardOutput = true, + WorkingDirectory = Path.GetDirectoryName(scriptPath), + // Prevent injection via environment variables + LoadUserProfile = false, + Environment = { ["PSModulePath"] = string.Empty }, };
94-104: 🛠️ Refactor suggestionAdd process timeout handling.
The process execution needs timeout handling to prevent hanging.
+ using var cts = new CancellationTokenSource(TimeSpan.FromMinutes(5)); // 5-minute timeout try { process.Start(); process.BeginOutputReadLine(); process.BeginErrorReadLine(); - process.WaitForExit(); + if (!process.WaitForExit(300000)) // 5-minute timeout + { + throw new TimeoutException("Process execution timed out after 5 minutes"); + } + if (process.ExitCode != 0) + { + throw new Exception($"Process exited with code {process.ExitCode}"); + } }Frends.PowerShell.RunScript/Frends.PowerShell.RunScript/PowerShell.cs (3)
64-71:⚠️ Potential issueFix command injection vulnerability in parameter handling.
The parameter concatenation is vulnerable to command injection.
- string parameterString = string.Empty; - if (parameters != null) - { - foreach (var param in parameters) - { - parameterString += $"-{param.Name} \"{param.Value}\" "; - } - } + string parameterString = parameters == null ? string.Empty : + string.Join(" ", parameters.Select(p => + $"-{EscapeArgument(p.Name)} {EscapeArgument(p.Value?.ToString() ?? string.Empty)}")); +private static string EscapeArgument(string argument) +{ + return "\"" + argument.Replace("\"", "`\"") + "\""; +}
73-82: 🛠️ Refactor suggestionApply consistent process configuration improvements.
Apply the same process configuration improvements as suggested for RunCommand.
process.StartInfo = new() { FileName = RuntimeInformation.IsOSPlatform(OSPlatform.Windows) ? "powershell" : "pwsh", Arguments = $"-NoProfile -ExecutionPolicy Bypass -File \"{scriptPath}\" {parameterString}", UseShellExecute = false, CreateNoWindow = true, RedirectStandardError = true, RedirectStandardOutput = true, + WorkingDirectory = Path.GetDirectoryName(scriptPath), + LoadUserProfile = false, + Environment = { ["PSModulePath"] = string.Empty }, };
97-107: 🛠️ Refactor suggestionAdd timeout handling and improve error handling.
Add timeout handling and improve error reporting for consistency with RunCommand.
+ using var cts = new CancellationTokenSource(TimeSpan.FromMinutes(5)); try { process.Start(); process.BeginOutputReadLine(); process.BeginErrorReadLine(); - process.WaitForExit(); + if (!process.WaitForExit(300000)) + { + throw new TimeoutException("Process execution timed out after 5 minutes"); + } + if (process.ExitCode != 0) + { + throw new Exception($"Process exited with code {process.ExitCode}"); + } } catch (Exception ex) { - errors.Add($"Exception occurred while running the script: {ex.Message}"); + errors.Add($"Exception occurred while running the script: {ex.GetType().Name}: {ex.Message}"); + if (ex is TimeoutException) + { + process.Kill(entireProcessTree: true); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Frends.PowerShell.RunCommand/Frends.PowerShell.RunCommand/PowerShell.cs(3 hunks)Frends.PowerShell.RunScript/Frends.PowerShell.RunScript/PowerShell.cs(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: build-windows / Code Quality Check
- GitHub Check: build-windows / Code Quality Check
- GitHub Check: build-linux / Code Quality Check
- GitHub Check: build-windows / Build on windows-latest
- GitHub Check: build-windows / Build on windows-latest
- GitHub Check: build-linux / Code Quality Check
- GitHub Check: Analyze (csharp)
🔇 Additional comments (1)
Frends.PowerShell.RunCommand/Frends.PowerShell.RunCommand/PowerShell.cs (1)
67-68: Add bounds to result collections.The unbounded collections could lead to memory issues with large output.
-List<dynamic> results = new(); -List<string> errors = new(); +const int MaxResults = 10000; +List<dynamic> results = new(MaxResults); +List<string> errors = new(100); +int skippedResults = 0;
| if (input.ExecuteNativeShell) | ||
| return ExecuteProcess(tempScript, input.Parameters); | ||
| else | ||
| return ExecuteCommand(tempScript, input.Parameters, input.LogInformationStream, session.PowerShell, cancellationToken); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Align security improvements with RunCommand.
For consistency and security, apply the same improvements as suggested for the RunCommand implementation:
- Add input validation
- Use secure file creation
- Escape parameters properly
+ if (input.ExecuteNativeShell)
+ {
+ if (string.IsNullOrWhiteSpace(script))
+ throw new ArgumentException("Script cannot be empty when executing from native shell");
+ if (input.Parameters?.Any(p => string.IsNullOrWhiteSpace(p.Name)) == true)
+ throw new ArgumentException("Parameter names cannot be empty");
+ }
File.WriteAllText(tempScript, script, Encoding.UTF8);
if (input.ExecuteNativeShell)
return ExecuteProcess(tempScript, input.Parameters);Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
Frends.PowerShell.RunCommand/Frends.PowerShell.RunCommand/PowerShell.cs (5)
40-44:⚠️ Potential issueAdd input validation for command and parameters
The command and parameters should be validated before execution to prevent security issues.
Add validation at the start of the conditional block:
if (input.ExecuteNativeShell) { + if (string.IsNullOrWhiteSpace(input.Command)) + throw new ArgumentException("Command cannot be empty when executing from native shell"); + if (input.Parameters?.Any(p => string.IsNullOrWhiteSpace(p.Name)) == true) + throw new ArgumentException("Parameter names cannot be empty"); var tempScript = Path.Combine(Path.GetTempPath(), $"{Path.GetRandomFileName()}.ps1");
45-52:⚠️ Potential issueFix potential command injection vulnerability
The current parameter concatenation is vulnerable to command injection attacks.
Apply this fix:
- string parameterString = string.Empty; - if (input.Parameters != null) - { - foreach (var param in input.Parameters) - { - parameterString += $"-{param.Name} \"{param.Value}\" "; - } - } + var parameterString = input.Parameters == null ? string.Empty : + string.Join(" ", input.Parameters.Select(p => + $"-{EscapeArgument(p.Name)} {EscapeArgument(p.Value?.ToString() ?? string.Empty)}")); +private static string EscapeArgument(string argument) +{ + if (string.IsNullOrEmpty(argument)) return "\"\""; + return "\"" + argument.Replace("\"", "`\"").Replace("$", "`$") + "\""; +}
70-79:⚠️ Potential issueEnhance process security configuration
The process configuration needs additional security settings and proper working directory handling.
process.StartInfo = new() { FileName = RuntimeInformation.IsOSPlatform(OSPlatform.Windows) ? "powershell" : "pwsh", Arguments = $"-NoProfile -ExecutionPolicy Bypass -File \"{scriptPath}\" ", UseShellExecute = false, CreateNoWindow = true, RedirectStandardError = true, RedirectStandardOutput = true, + WorkingDirectory = Path.GetDirectoryName(scriptPath), + // Prevent injection via environment variables + LoadUserProfile = false, + Environment = { ["PSModulePath"] = string.Empty }, };
67-68: 🛠️ Refactor suggestionAdd bounds to result collections
The unbounded collections could lead to memory issues with large output.
-List<dynamic> results = new(); -List<string> errors = new(); +const int MaxResults = 10000; +List<dynamic> results = new(MaxResults); +List<string> errors = new(100); +int skippedResults = 0; // Later in the ProcessOutputReceiver: -if (!string.IsNullOrWhiteSpace(args.Data)) +if (!string.IsNullOrWhiteSpace(args.Data) && results.Count < MaxResults) results.Add(args.Data); +else if (!string.IsNullOrWhiteSpace(args.Data)) + skippedResults++;
94-104: 🛠️ Refactor suggestionAdd process execution timeout
Long-running processes could block indefinitely. Add a timeout mechanism.
+using var cts = new CancellationTokenSource(TimeSpan.FromMinutes(5)); // 5-minute timeout try { process.Start(); process.BeginOutputReadLine(); process.BeginErrorReadLine(); - process.WaitForExit(); + if (!process.WaitForExit(300000)) // 5-minute timeout + { + throw new TimeoutException("Process execution timed out after 5 minutes"); + } + if (process.ExitCode != 0) + { + throw new Exception($"Process exited with code {process.ExitCode}"); + } }
🧹 Nitpick comments (1)
Frends.PowerShell.RunCommand/Frends.PowerShell.RunCommand/PowerShell.cs (1)
73-73: Document platform-specific executable namesAdd XML documentation to explain the executable names used on different platforms.
+/// <summary> +/// The PowerShell executable name is platform-specific: +/// - Windows: "powershell" +/// - Linux/macOS: "pwsh" +/// </summary> FileName = RuntimeInformation.IsOSPlatform(OSPlatform.Windows) ? "powershell" : "pwsh",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Frends.PowerShell.RunCommand/Frends.PowerShell.RunCommand/PowerShell.cs(3 hunks)
🔇 Additional comments (1)
Frends.PowerShell.RunCommand/Frends.PowerShell.RunCommand/PowerShell.cs (1)
3-3: LGTM: Required imports added for native shell executionThe new imports support process handling and cross-platform compatibility.
Also applies to: 8-8, 9-9, 10-10
Summary by CodeRabbit
New Features
ExecuteNativeShellparameter for more flexible PowerShell interactions.Version Updates
Frends.PowerShell.RunCommandto version 1.3.0.Frends.PowerShell.RunScriptto version 1.3.0.Improvements