fix(security): fix command injection and authorization bypass vulnerabilities

.github/workflows/issue-webhook.yml

@@ -9,8 +9,10 @@ permissions:
 
 jobs:
   notify:
-    # Only trigger when issue title ends with [claude bot]
-    if: endsWith(github.event.issue.title, '[claude bot]')
+    # Only trigger when issue title ends with [claude bot] and author is a collaborator
+    if: >-
+      endsWith(github.event.issue.title, '[claude bot]') &&
+      contains(fromJSON('["OWNER", "COLLABORATOR", "MEMBER"]'), github.event.issue.author_association)
     runs-on: ubuntu-latest
 
     steps:

README.md

@@ -74,6 +74,7 @@ docker:
 github:
   webhook_secret: "${GITHUB_WEBHOOK_SECRET}"  # From .env
   pat: "${GITHUB_PAT}"                         # From .env
+  allowed_authors: []                          # Optional: restrict to specific GitHub usernames (e.g. ["user1", "user2"]); empty = allow all
 
 queue:
   max_retries: 1      # Retry failed tasks once before giving up

api/controller/webhook_controller.go

@@ -31,6 +31,10 @@ func (h *webhookHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 			http.Error(w, err.Error(), http.StatusConflict)
 			return
 		}
+		if errors.Is(err, domain.ErrUnauthorized) {
+			http.Error(w, err.Error(), http.StatusForbidden)
+			return
+		}
 		slog.Error("handle webhook", "error", err)
 		http.Error(w, "internal error", http.StatusInternalServerError)
 		return

api/controller/webhook_controller_test.go

@@ -106,3 +106,22 @@ func TestWebhookHandler_DuplicateIssue(t *testing.T) {
 
 	assert.Equal(t, http.StatusConflict, rec.Code)
 }
+
+func TestWebhookHandler_UnauthorizedAuthor(t *testing.T) {
+	t.Parallel()
+	uc := new(mockUsecase)
+	handler := controller.NewWebhookHandler(uc)
+
+	payload := domain.WebhookPayload{IssueNumber: 42, Title: "test", Repository: "owner/repo", Author: "attacker"}
+	body, _ := json.Marshal(payload)
+
+	uc.On("HandleWebhook", mock.Anything, payload).Return(domain.Task{}, domain.ErrUnauthorized)
+
+	req := httptest.NewRequest(http.MethodPost, "/webhook/github", bytes.NewReader(body))
+	req.Header.Set("Content-Type", "application/json")
+	rec := httptest.NewRecorder()
+
+	handler.ServeHTTP(rec, req)
+
+	assert.Equal(t, http.StatusForbidden, rec.Code)
+}

bootstrap/bootstrap.go

@@ -78,8 +78,9 @@ func Init() (*App, error) {
 			MaxRetries: viper.GetInt("queue.max_retries"),
 		},
 		GitHub: domain.GitHubConfig{
-			WebhookSecret: viper.GetString("github.webhook_secret"),
-			PAT:           viper.GetString("github.pat"),
+			WebhookSecret:  viper.GetString("github.webhook_secret"),
+			PAT:            viper.GetString("github.pat"),
+			AllowedAuthors: viper.GetStringSlice("github.allowed_authors"),
 		},
 	}
 
@@ -134,7 +135,7 @@ func Init() (*App, error) {
 	if err != nil {
 		return nil, fmt.Errorf("init docker runner: %w", err)
 	}
-	uc := usecase.NewTaskUsecase(taskRepo, taskQueue, runner, cfg.Docker)
+	uc := usecase.NewTaskUsecase(taskRepo, taskQueue, runner, cfg.Docker, cfg.GitHub.AllowedAuthors)
 
 	mux := route.NewMux(uc, cfg.GitHub.WebhookSecret)
 

config.yaml.example

@@ -16,6 +16,7 @@ docker:
 github:
   webhook_secret: "${GITHUB_WEBHOOK_SECRET}"
   pat: "${GITHUB_PAT}"
+  allowed_authors: []  # Optional author allowlist, e.g. ["user1", "user2"]; empty = allow all
 
 queue:
   max_retries: 1

domain/config.go

@@ -31,6 +31,7 @@ type QueueConfig struct {
 }
 
 type GitHubConfig struct {
-	PAT           string
-	WebhookSecret string
+	PAT             string
+	WebhookSecret   string
+	AllowedAuthors  []string
 }

domain/errors.go

@@ -3,6 +3,7 @@ package domain
 import "errors"
 
 var (
-	ErrNotFound        = errors.New("task not found")
+	ErrNotFound         = errors.New("task not found")
 	ErrActiveTaskExists = errors.New("issue already has an active task")
+	ErrUnauthorized     = errors.New("unauthorized author")
 )

internal/docker/runner.go

@@ -47,11 +47,11 @@ func (r *runner) StartContainer(ctx context.Context, task domain.Task) (string,
 		fmt.Sprintf("ANTHROPIC_API_KEY=%s", r.apiKey),
 		fmt.Sprintf("GITHUB_TOKEN=%s", r.githubToken),
 		fmt.Sprintf("CGATE_URL=%s", r.cgateURL),
-		fmt.Sprintf("REPOSITORY=%s", task.Repository),
+		fmt.Sprintf("REPOSITORY=%s", SanitizeEnvValue(task.Repository)),
 		fmt.Sprintf("ISSUE_NUMBER=%d", task.IssueNumber),
-		fmt.Sprintf("ISSUE_TITLE=%s", task.Title),
-		fmt.Sprintf("ISSUE_BODY=%s", task.Body),
-		fmt.Sprintf("ISSUE_URL=%s", task.HTMLURL),
+		fmt.Sprintf("ISSUE_TITLE=%s", SanitizeShellValue(task.Title)),
+		fmt.Sprintf("ISSUE_BODY=%s", SanitizeEnvValue(task.Body)),
+		fmt.Sprintf("ISSUE_URL=%s", SanitizeEnvValue(task.HTMLURL)),
 		fmt.Sprintf("GIT_USER_NAME=%s", r.cfg.GitUserName),
 		fmt.Sprintf("GIT_USER_EMAIL=%s", r.cfg.GitUserEmail),
 		fmt.Sprintf("MAX_TURNS=%d", r.cfg.MaxTurns),
@@ -191,3 +191,26 @@ func (r *runner) IsRunning(ctx context.Context, containerID string) (bool, error
 	}
 	return inspect.State.Running, nil
 }
+
+// SanitizeEnvValue removes null bytes and control characters from environment
+// variable values to prevent injection in downstream shell scripts.
+func SanitizeEnvValue(s string) string {
+	var b strings.Builder
+	b.Grow(len(s))
+	for _, r := range s {
+		if r >= 0x20 || r == '\t' || r == '\n' || r == '\r' {
+			b.WriteRune(r)
+		}
+	}
+	return b.String()
+}
+
+// SanitizeShellValue strips shell command substitution patterns in addition to
+// control characters. Used for fields that flow into shell scripts where
+// expansion could occur (e.g., issue titles used in heredocs).
+func SanitizeShellValue(s string) string {
+	s = SanitizeEnvValue(s)
+	s = strings.ReplaceAll(s, "$(", "")
+	s = strings.ReplaceAll(s, "`", "")
+	return s
+}

internal/docker/runner_test.go

@@ -8,6 +8,56 @@ import (
 	"github.com/Lin-Jiong-HDU/go-project-template/internal/docker"
 )
 
+func TestSanitizeEnvValue_RemovesControlChars(t *testing.T) {
+	t.Parallel()
+	tests := []struct {
+		name     string
+		input    string
+		expected string
+	}{
+		{"removes null bytes", "hello\x00world", "helloworld"},
+		{"removes control chars", "test\x01\x02\x03value", "testvalue"},
+		{"keeps newlines", "line1\nline2", "line1\nline2"},
+		{"keeps tabs", "col1\tcol2", "col1\tcol2"},
+		{"keeps carriage returns", "line1\r\nline2", "line1\r\nline2"},
+		{"empty string", "", ""},
+		{"clean string", "hello world", "hello world"},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			got := docker.SanitizeEnvValue(tt.input)
+			if got != tt.expected {
+				t.Errorf("SanitizeEnvValue(%q) = %q, want %q", tt.input, got, tt.expected)
+			}
+		})
+	}
+}
+
+func TestSanitizeShellValue_StripsCommandSubstitution(t *testing.T) {
+	t.Parallel()
+	tests := []struct {
+		name     string
+		input    string
+		expected string
+	}{
+		{"strips dollar paren", "$(rm -rf /)", "rm -rf /)"},
+		{"strips backticks", "`cat /etc/passwd`", "cat /etc/passwd"},
+		{"strips both", "$(whoami)`id`", "whoami)id"},
+		{"clean title", "Fix bug [claude bot]", "Fix bug [claude bot]"},
+		{"nested command", "$(echo $(secret))", "echo secret))"},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			got := docker.SanitizeShellValue(tt.input)
+			if got != tt.expected {
+				t.Errorf("SanitizeShellValue(%q) = %q, want %q", tt.input, got, tt.expected)
+			}
+		})
+	}
+}
+
 func TestNewRunner_InvalidImage(t *testing.T) {
 	t.Parallel()
 	cfg := domain.DockerConfig{

runner-image/entrypoint.sh

@@ -50,26 +50,31 @@ if [ "$(id -u)" = "0" ]; then
     chown -R runner:runner /home/runner /workspace /tmp/prompt.txt
 
     # Write a launcher script that includes git push + PR creation
-    cat > /tmp/run-claude.sh <<SCRIPT
+    export branch
+    cat > /tmp/run-claude.sh <<'SCRIPT'
 #!/bin/bash
 cd /workspace/repo
 git config --global --add safe.directory /workspace/repo
-claude -p "\$(cat /tmp/prompt.txt)" --dangerously-skip-permissions
+claude_args=(-p "$(cat /tmp/prompt.txt)")
+if [ "${SKIP_PERMISSIONS:-}" = "true" ]; then
+    claude_args+=(--dangerously-skip-permissions)
+fi
+claude "${claude_args[@]}"
 
 echo "=== Pushing branch ==="
-for i in 1 2 3 4 5; do git push -u origin ${branch} && break || sleep 10; done
+for i in 1 2 3 4 5; do git push -u origin "${branch}" && break || sleep 10; done
 
 echo "=== Creating PR ==="
-pr_title=\$(echo "${ISSUE_TITLE}" | sed 's/[[:space:]]*\[claude bot\]//')
-gh pr create \\
-    --title "\$pr_title" \\
+pr_title=$(echo "${ISSUE_TITLE}" | sed 's/[[:space:]]*\[claude bot\]//')
+gh pr create \
+    --title "$pr_title" \
     --body "Closes #${ISSUE_NUMBER}
 
 Automated implementation by CGate.
 
 Changes:
-\$(git log --oneline main..HEAD)" \\
-    --base main \\
+$(git log --oneline main..HEAD)" \
+    --base main \
     --head "${branch}" || echo "PR already exists or creation skipped"
 SCRIPT
     chmod +x /tmp/run-claude.sh

usecase/task_usecase.go

@@ -11,27 +11,42 @@ import (
 )
 
 type taskUsecase struct {
-	repo      domain.TaskRepository
-	queue     domain.TaskQueue
-	runner    domain.DockerRunner
-	dockerCfg domain.DockerConfig
+	repo           domain.TaskRepository
+	queue          domain.TaskQueue
+	runner         domain.DockerRunner
+	dockerCfg      domain.DockerConfig
+	allowedAuthors []string
 
 	running   map[string]context.CancelFunc
 	mu        sync.Mutex
 	cancelCtx context.CancelFunc
 }
 
-func NewTaskUsecase(repo domain.TaskRepository, queue domain.TaskQueue, runner domain.DockerRunner, dockerCfg domain.DockerConfig) domain.TaskUsecase {
+func NewTaskUsecase(repo domain.TaskRepository, queue domain.TaskQueue, runner domain.DockerRunner, dockerCfg domain.DockerConfig, allowedAuthors []string) domain.TaskUsecase {
 	return &taskUsecase{
-		repo:      repo,
-		queue:     queue,
-		runner:    runner,
-		dockerCfg: dockerCfg,
-		running:   make(map[string]context.CancelFunc),
+		repo:           repo,
+		queue:          queue,
+		runner:         runner,
+		dockerCfg:      dockerCfg,
+		allowedAuthors: allowedAuthors,
+		running:        make(map[string]context.CancelFunc),
 	}
 }
 
 func (u *taskUsecase) HandleWebhook(ctx context.Context, payload domain.WebhookPayload) (domain.Task, error) {
+	if len(u.allowedAuthors) > 0 {
+		allowed := false
+		for _, a := range u.allowedAuthors {
+			if a == payload.Author {
+				allowed = true
+				break
+			}
+		}
+		if !allowed {
+			return domain.Task{}, domain.ErrUnauthorized
+		}
+	}
+
 	active, err := u.repo.FindActiveByIssue(ctx, payload.Repository, payload.IssueNumber)
 	if err != nil {
 		return domain.Task{}, fmt.Errorf("check active tasks: %w", err)