Skip to content

Commit 743e18a

Browse files
Fix shell injection vulnerability and address PR feedback
- Replace string concatenation with exec.CommandContext using separate arguments in all DefaultsCommand methods (Read, ReadType, Write, WriteWithType) - This prevents shell injection by passing values as distinct arguments instead of embedding them in command strings - Update push_type_test.go to handle new error message format from direct exec calls - Remove todo.txt file as suggested in PR feedback Security improvements: - All command execution now uses exec.CommandContext(ctx, command, arg1, arg2, ...) - Values are no longer interpolated into shell command strings - Eliminates risk of shell injection from special characters in domain, key, or value parameters Co-Authored-By: k \u201C\u713C\u914E\u304A\u6E6F\u5272\u308A250ml 30%\u5F15\u304D\u201D <fumiya.kume@hotmail.com>
1 parent 0dbb46d commit 743e18a

3 files changed

Lines changed: 10 additions & 48 deletions

File tree

internal/defaults/defaults_command.go

Lines changed: 7 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -44,8 +44,7 @@ func (d *DefaultsCommandImpl) Read(ctx context.Context) (string, error) {
4444
if d.domain == "" || d.key == "" {
4545
return "", fmt.Errorf("domain and key cannot be empty")
4646
}
47-
command := fmt.Sprintf("defaults read %s %s", d.domain, d.key)
48-
output, err := exec.CommandContext(ctx, "bash", "-c", command).Output()
47+
output, err := exec.CommandContext(ctx, "defaults", "read", d.domain, d.key).Output()
4948
if err != nil {
5049
return "", err
5150
}
@@ -57,8 +56,7 @@ func (d *DefaultsCommandImpl) ReadType(ctx context.Context) (string, error) {
5756
if d.domain == "" || d.key == "" {
5857
return "", fmt.Errorf("domain and key cannot be empty")
5958
}
60-
command := fmt.Sprintf("defaults read-type %s %s", d.domain, d.key)
61-
output, err := exec.CommandContext(ctx, "bash", "-c", command).Output()
59+
output, err := exec.CommandContext(ctx, "defaults", "read-type", d.domain, d.key).Output()
6260
if err != nil {
6361
return "string", nil
6462
}
@@ -77,8 +75,7 @@ func (d *DefaultsCommandImpl) Write(ctx context.Context, value string) error {
7775
if d.domain == "" || d.key == "" {
7876
return fmt.Errorf("domain and key cannot be empty")
7977
}
80-
command := fmt.Sprintf("defaults write %s %s %s", d.domain, d.key, value)
81-
_, err := exec.CommandContext(ctx, "bash", "-c", command).Output()
78+
_, err := exec.CommandContext(ctx, "defaults", "write", d.domain, d.key, value).Output()
8279
if err != nil {
8380
return err
8481
}
@@ -91,14 +88,14 @@ func (d *DefaultsCommandImpl) WriteWithType(ctx context.Context, value string, v
9188
}
9289

9390
typeFlag := mapInternalTypeToFlag(valueType)
94-
var command string
91+
var args []string
9592
if typeFlag == "" || valueType == "string" {
96-
command = fmt.Sprintf("defaults write %s %s %s", d.domain, d.key, value)
93+
args = []string{"defaults", "write", d.domain, d.key, value}
9794
} else {
98-
command = fmt.Sprintf("defaults write %s %s %s %s", d.domain, d.key, typeFlag, value)
95+
args = []string{"defaults", "write", d.domain, d.key, typeFlag, value}
9996
}
10097

101-
_, err := exec.CommandContext(ctx, "bash", "-c", command).Output()
98+
_, err := exec.CommandContext(ctx, args[0], args[1:]...).Output()
10299
if err != nil {
103100
return err
104101
}

internal/operation/push/push_type_test.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ func TestPushWithTypes(t *testing.T) {
2525
Push(configs)
2626

2727
logStr := logOutput.String()
28-
if strings.Contains(logStr, "Failed to write") && !strings.Contains(logStr, "exit status 127") {
28+
if strings.Contains(logStr, "Failed to write") && !strings.Contains(logStr, "exit status 127") && !strings.Contains(logStr, "executable file not found") {
2929
t.Errorf("Unexpected error in log output: %s", logStr)
3030
}
3131
}
@@ -43,7 +43,7 @@ func TestPushWithStringType(t *testing.T) {
4343
Push(configs)
4444

4545
logStr := logOutput.String()
46-
if strings.Contains(logStr, "Failed to write") && !strings.Contains(logStr, "exit status 127") {
46+
if strings.Contains(logStr, "Failed to write") && !strings.Contains(logStr, "exit status 127") && !strings.Contains(logStr, "executable file not found") {
4747
t.Errorf("Unexpected error in log output: %s", logStr)
4848
}
4949
}
@@ -61,7 +61,7 @@ func TestPushWithEmptyType(t *testing.T) {
6161
Push(configs)
6262

6363
logStr := logOutput.String()
64-
if strings.Contains(logStr, "Failed to write") && !strings.Contains(logStr, "exit status 127") {
64+
if strings.Contains(logStr, "Failed to write") && !strings.Contains(logStr, "exit status 127") && !strings.Contains(logStr, "executable file not found") {
6565
t.Errorf("Unexpected error in log output: %s", logStr)
6666
}
6767
}

todo.txt

Lines changed: 0 additions & 35 deletions
This file was deleted.

0 commit comments

Comments
 (0)