github-actions[bot]
commented
1 month ago Preparing PR description...
Closed mrheinen closed 1 month ago
github-actions[bot]
commented
1 month ago Preparing PR description...
github-actions[bot]
commented
1 month ago Preparing review...
codiumai-pr-agent-pro[bot]
commented
1 month ago | ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪ |
| 🧪 PR contains tests |
| 🔒 Security concerns Command execution: The PR introduces command execution functionality in content scripts. This feature, if not properly restricted and sanitized, could lead to remote code execution vulnerabilities. Ensure that the `allowedCommands` list is strictly controlled and that user input is never directly used as command arguments without proper validation and sanitization. |
| ⚡ Key issues to review Security Concern The command execution functionality allows running arbitrary commands. Ensure proper input validation and sanitization are in place. Potential Performance Issue The `NewGojaJavascriptRunner` function creates a new cache with a 30-minute timeout. Consider making this timeout configurable. SQL Injection Risk The `SearchContentRules` function uses dynamic SQL query building. Ensure proper parameterization is used to prevent SQL injection. |
codiumai-pr-agent-pro[bot]
commented
1 month ago | Category | Suggestion | Score |
| Security |
Implement input validation to prevent potential command injection vulnerabilities___ **Consider implementing input validation for thecommand and args parameters in the RunCommand method to prevent potential command injection vulnerabilities.**
[pkg/javascript/command_runner.go [63-81]](https://github.com/mrheinen/lophiid/pull/30/files#diff-add6ec727371609ae146df0e03ac2124dc03934f235b6867c5d868b37bd827cdR63-R81)
```diff
func (s *SingleCommandRunner) RunCommand(command string, args ...string) bool {
if !util.Contains(s.allowedCommands, command) {
slog.Error("command is not allowed", slog.String("command", command))
s.Error = fmt.Errorf("command %s is not allowed", command)
+ return false
+ }
+
+ // Validate command and args
+ if !validateInput(command) || !validateInputs(args) {
+ slog.Error("invalid input detected", slog.String("command", command))
+ s.Error = fmt.Errorf("invalid input detected")
return false
}
cmd := exec.Command(command, args...)
cmd.Stdout = &s.Stdout
cmd.Stderr = &s.Stderr
if err := cmd.Run(); err != nil {
s.Error = err
slog.Error("command failed", slog.String("error", err.Error()), slog.String("stderr", s.Stderr.String()))
return false
}
return true
}
+func validateInput(input string) bool {
+ // Implement input validation logic
+ return true
+}
+
+func validateInputs(inputs []string) bool {
+ for _, input := range inputs {
+ if !validateInput(input) {
+ return false
+ }
+ }
+ return true
+}
+
```
- [ ] **Apply this suggestion**
Suggestion importance[1-10]: 9Why: This suggestion addresses a critical security vulnerability by preventing potential command injection attacks. | 9 |
✅ Implement resource limits and timeout for executed commands to prevent potential denial-of-service attacks___Suggestion Impact:The commit implemented a timeout mechanism for command execution, which addresses part of the suggestion's goal to prevent potential denial-of-service attacks. code diff: ```diff + ctx, cancel := context.WithTimeout(context.Background(), s.commandTimeout) + defer cancel() + + cmd := exec.CommandContext(ctx, command, args...) cmd.Stdout = &s.Stdout cmd.Stderr = &s.Stderr - if err := cmd.Run(); err != nil { + if err := cmd.Start(); err != nil { + s.Error = err + slog.Error("command failed to start", slog.String("error", err.Error()), slog.String("stderr", s.Stderr.String())) + return false + } + + if err := cmd.Wait(); err != nil { s.Error = err slog.Error("command failed", slog.String("error", err.Error()), slog.String("stderr", s.Stderr.String())) ```can be used by the executed commands to prevent potential denial-of-service attacks.** [pkg/javascript/command_runner.go [63-81]](https://github.com/mrheinen/lophiid/pull/30/files#diff-add6ec727371609ae146df0e03ac2124dc03934f235b6867c5d868b37bd827cdR63-R81) ```diff func (s *SingleCommandRunner) RunCommand(command string, args ...string) bool { if !util.Contains(s.allowedCommands, command) { slog.Error("command is not allowed", slog.String("command", command)) s.Error = fmt.Errorf("command %s is not allowed", command) return false } cmd := exec.Command(command, args...) cmd.Stdout = &s.Stdout cmd.Stderr = &s.Stderr - if err := cmd.Run(); err != nil { + // Set resource limits + cmd.SysProcAttr = &syscall.SysProcAttr{ + Setpgid: true, + } + + if err := cmd.Start(); err != nil { s.Error = err - slog.Error("command failed", slog.String("error", err.Error()), slog.String("stderr", s.Stderr.String())) + slog.Error("command failed to start", slog.String("error", err.Error())) + return false + } + + done := make(chan error) + go func() { done <- cmd.Wait() }() + + select { + case err := <-done: + if err != nil { + s.Error = err + slog.Error("command failed", slog.String("error", err.Error()), slog.String("stderr", s.Stderr.String())) + return false + } + case <-time.After(30 * time.Second): + if err := syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL); err != nil { + slog.Error("failed to kill process", slog.String("error", err.Error())) + } + s.Error = fmt.Errorf("command timed out") return false } return true } ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 9Why: This suggestion significantly enhances security by preventing potential denial-of-service attacks through resource limitation and timeout mechanisms. | 9 | |
| Enhancement |
✅ Add a timeout mechanism to prevent long-running commands from blocking indefinitely___Suggestion Impact:The commit implemented a timeout mechanism using context.WithTimeout, as suggested. It also added a commandTimeout field to the struct and modified the constructor to accept this timeout value. code diff: ```diff + ctx, cancel := context.WithTimeout(context.Background(), s.commandTimeout) + defer cancel() + + cmd := exec.CommandContext(ctx, command, args...) cmd.Stdout = &s.Stdout cmd.Stderr = &s.Stderr - if err := cmd.Run(); err != nil { + if err := cmd.Start(); err != nil { + s.Error = err + slog.Error("command failed to start", slog.String("error", err.Error()), slog.String("stderr", s.Stderr.String())) + return false + } + + if err := cmd.Wait(); err != nil { s.Error = err ```RunCommand method to prevent long-running commands from blocking indefinitely.** [pkg/javascript/command_runner.go [63-81]](https://github.com/mrheinen/lophiid/pull/30/files#diff-add6ec727371609ae146df0e03ac2124dc03934f235b6867c5d868b37bd827cdR63-R81) ```diff func (s *SingleCommandRunner) RunCommand(command string, args ...string) bool { if !util.Contains(s.allowedCommands, command) { slog.Error("command is not allowed", slog.String("command", command)) s.Error = fmt.Errorf("command %s is not allowed", command) return false } - cmd := exec.Command(command, args...) + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + + cmd := exec.CommandContext(ctx, command, args...) cmd.Stdout = &s.Stdout cmd.Stderr = &s.Stderr if err := cmd.Run(); err != nil { s.Error = err slog.Error("command failed", slog.String("error", err.Error()), slog.String("stderr", s.Stderr.String())) return false } return true } ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 8Why: This suggestion addresses a potential security issue by preventing indefinite blocking, which could lead to resource exhaustion. | 8 |
Improve field naming for better code readability and understanding___ **Consider using a more descriptive name for thestrCache field, such as scriptCache or javascriptCache, to better reflect its purpose in caching JavaScript scripts.**
[pkg/javascript/goja.go [47]](https://github.com/mrheinen/lophiid/pull/30/files#diff-93ebbcb2bc5cc6ca7ef6d25c1ce739d50fc1194101ed2b4469c1941da4111828R47-R47)
```diff
-strCache *util.StringMapCache[string]
+scriptCache *util.StringMapCache[string]
```
- [ ] **Apply this suggestion**
Suggestion importance[1-10]: 6Why: The suggestion improves code readability by using a more descriptive name, but it's a minor change that doesn't address major issues or bugs. | 6 |
mrheinen
commented
1 month ago The command timeout was actually a good suggestion by the PR agent so I applied that one (via own commit) and made it configurable from the config in the backend and API
mrheinen
commented
1 month ago /describe
github-actions[bot]
commented
1 month ago Preparing PR description...
codiumai-pr-agent-pro[bot]
commented
1 month ago PR Description updated to latest commit (https://github.com/mrheinen/lophiid/commit/c3d8caacb478c9ab730daf2a1c6b1492b7a1c970)
PR Type
Enhancement, Bug fix
Description
Changes walkthrough 📝
4 files
api_server.go
Add scripting configuration and update JavaScript runnercmd/api/api_server.go
'CommandTimeout' fields
backend_main.go
Update JavaScript runner initializationcmd/backend/backend_main.go
from config
config.go
Add scripting configuration to backendpkg/backend/config.go
fields to Config
backend-config.yaml
Add scripting configuration to backend configconfig/backend-config.yaml - Added 'scripting' section with 'allowed_commands' configuration
1 files
main.go
Remove whois command implementationcmd/whois/main.go - Entire file removed
7 files
database.go
Add body setter and fix content rule querypkg/database/database.go
command_runner.go
Add command execution functionalitypkg/javascript/command_runner.go
crypto.go
Add crypto utility functionspkg/javascript/crypto.go
encoding.go
Add encoding utility functionspkg/javascript/encoding.go
goja.go
Refactor JavaScript runner and add new utilitiespkg/javascript/goja.go
files)
time.go
Add time utility functionpkg/javascript/time.go
wrappers.go
Refactor JavaScript wrappers into separate filepkg/javascript/wrappers.go
1 files
goja_test.go
Update JavaScript runner testspkg/javascript/goja_test.go
1 files
SCRIPTING.md
Update scripting documentationSCRIPTING.md