diff --git a/cmd/review-bot/main.go b/cmd/review-bot/main.go index 636b19a..4b769f4 100644 --- a/cmd/review-bot/main.go +++ b/cmd/review-bot/main.go @@ -110,8 +110,12 @@ func main() { } slog.Info("loaded built-in persona", "persona", persona.Name, "display", persona.DisplayName) } else if *personaFile != "" { - var err error - persona, err = review.LoadPersona(*personaFile) + resolvedPath, err := validateWorkspacePath(*personaFile, "persona-file") + if err != nil { + slog.Error("invalid persona-file path", "error", err) + os.Exit(1) + } + persona, err = review.LoadPersona(resolvedPath) if err != nil { slog.Error("failed to load persona file", "file", *personaFile, "error", err) os.Exit(1) @@ -229,34 +233,14 @@ func main() { // Step 6b: Load additional system prompt if specified additionalPrompt := "" if *systemPromptFile != "" { - workspace := os.Getenv("GITHUB_WORKSPACE") - if workspace == "" { - workspace, _ = os.Getwd() - } - absWorkspace, err := filepath.Abs(workspace) + resolvedPath, err := validateWorkspacePath(*systemPromptFile, "system-prompt-file") if err != nil { - slog.Error("failed to resolve workspace path", "error", err) - os.Exit(1) - } - promptPath := filepath.Join(absWorkspace, *systemPromptFile) - promptPath = filepath.Clean(promptPath) - if !strings.HasPrefix(promptPath, absWorkspace+string(filepath.Separator)) && promptPath != absWorkspace { - slog.Error("system-prompt-file resolves outside workspace", "path", promptPath, "workspace", absWorkspace) - os.Exit(1) - } - // Resolve symlinks and re-validate to prevent symlink traversal - resolvedPath, err := filepath.EvalSymlinks(promptPath) - if err != nil { - slog.Error("failed to resolve system prompt file", "path", promptPath, "error", err) - os.Exit(1) - } - if !strings.HasPrefix(resolvedPath, absWorkspace+string(filepath.Separator)) && resolvedPath != absWorkspace { - slog.Error("system-prompt-file symlink resolves outside workspace", "resolved", resolvedPath, "workspace", absWorkspace) + slog.Error("invalid system-prompt-file path", "error", err) os.Exit(1) } data, err := os.ReadFile(resolvedPath) if err != nil { - slog.Error("failed to read system prompt file", "path", promptPath, "error", err) + slog.Error("failed to read system prompt file", "path", *systemPromptFile, "error", err) os.Exit(1) } additionalPrompt = string(data) @@ -608,6 +592,36 @@ func validateReviewerName(name string) error { return nil } +// validateWorkspacePath ensures a file path is within the workspace and resolves +// symlinks to prevent traversal attacks. Returns the resolved absolute path or +// an error if the path is outside the workspace. +func validateWorkspacePath(path, pathName string) (string, error) { + workspace := os.Getenv("GITHUB_WORKSPACE") + if workspace == "" { + workspace, _ = os.Getwd() + } + absWorkspace, err := filepath.Abs(workspace) + if err != nil { + return "", fmt.Errorf("failed to resolve workspace path: %w", err) + } + // Join and clean the path + fullPath := filepath.Join(absWorkspace, path) + fullPath = filepath.Clean(fullPath) + // Check path is within workspace + if !strings.HasPrefix(fullPath, absWorkspace+string(filepath.Separator)) && fullPath != absWorkspace { + return "", fmt.Errorf("%s resolves outside workspace: path=%s workspace=%s", pathName, fullPath, absWorkspace) + } + // Resolve symlinks and re-validate to prevent symlink traversal + resolvedPath, err := filepath.EvalSymlinks(fullPath) + if err != nil { + return "", fmt.Errorf("failed to resolve %s: %w", pathName, err) + } + if !strings.HasPrefix(resolvedPath, absWorkspace+string(filepath.Separator)) && resolvedPath != absWorkspace { + return "", fmt.Errorf("%s symlink resolves outside workspace: resolved=%s workspace=%s", pathName, resolvedPath, absWorkspace) + } + return resolvedPath, nil +} + // buildSupersededBody creates the body for a superseded review: struck-through banner // with collapsed original content and the commit it was evaluated against. func buildSupersededBody(originalBody, commitSHA, newReviewURL, sentinel string) string { diff --git a/cmd/review-bot/main_test.go b/cmd/review-bot/main_test.go index df1ce36..31f1864 100644 --- a/cmd/review-bot/main_test.go +++ b/cmd/review-bot/main_test.go @@ -7,6 +7,7 @@ import ( "os" "os/exec" "strings" + "path/filepath" "testing" "gitea.weiker.me/rodin/review-bot/gitea" @@ -45,6 +46,113 @@ func TestValidateReviewerName(t *testing.T) { } } +func TestValidateWorkspacePath(t *testing.T) { + // Create a temp directory as our workspace + tmpDir := t.TempDir() + + // Create a valid file inside the workspace + validFile := filepath.Join(tmpDir, "valid.json") + if err := os.WriteFile(validFile, []byte("{}"), 0644); err != nil { + t.Fatalf("failed to create test file: %v", err) + } + + // Create a subdirectory with a file + subDir := filepath.Join(tmpDir, "subdir") + if err := os.MkdirAll(subDir, 0755); err != nil { + t.Fatalf("failed to create subdir: %v", err) + } + nestedFile := filepath.Join(subDir, "nested.json") + if err := os.WriteFile(nestedFile, []byte("{}"), 0644); err != nil { + t.Fatalf("failed to create nested file: %v", err) + } + + // Create a symlink pointing outside the workspace + symlinkPath := filepath.Join(tmpDir, "evil-symlink.json") + if err := os.Symlink("/etc/passwd", symlinkPath); err != nil { + t.Fatalf("failed to create symlink: %v", err) + } + + // Save and restore GITHUB_WORKSPACE + origWorkspace := os.Getenv("GITHUB_WORKSPACE") + defer os.Setenv("GITHUB_WORKSPACE", origWorkspace) + + tests := []struct { + name string + workspace string + path string + wantErr bool + errMatch string + }{ + { + name: "valid relative path", + workspace: tmpDir, + path: "valid.json", + wantErr: false, + }, + { + name: "valid nested path", + workspace: tmpDir, + path: "subdir/nested.json", + wantErr: false, + }, + { + name: "path traversal attempt", + workspace: tmpDir, + path: "../../../etc/passwd", + wantErr: true, + errMatch: "resolves outside workspace", + }, + { + name: "absolute path gets normalized to relative", + workspace: tmpDir, + path: "/etc/passwd", + wantErr: true, + errMatch: "failed to resolve", // filepath.Join strips leading / making it /etc/passwd which doesn't exist + }, + { + name: "nonexistent file", + workspace: tmpDir, + path: "nonexistent.json", + wantErr: true, + errMatch: "failed to resolve", + }, + { + name: "symlink escaping workspace", + workspace: tmpDir, + path: "evil-symlink.json", + wantErr: true, + errMatch: "symlink resolves outside workspace", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + os.Setenv("GITHUB_WORKSPACE", tc.workspace) + resolved, err := validateWorkspacePath(tc.path, "test-file") + + if tc.wantErr { + if err == nil { + t.Errorf("expected error for %q, got nil", tc.path) + } else if tc.errMatch != "" && !strings.Contains(err.Error(), tc.errMatch) { + t.Errorf("error %q should contain %q", err.Error(), tc.errMatch) + } + } else { + if err != nil { + t.Errorf("expected no error for %q, got %v", tc.path, err) + } + if resolved == "" { + t.Error("expected non-empty resolved path") + } + // Verify resolved path is within workspace + if !strings.HasPrefix(resolved, tc.workspace) { + t.Errorf("resolved path %q not within workspace %q", resolved, tc.workspace) + } + } + }) + } +} + + func makeReview(id int64, login, state string, stale bool, body string) gitea.Review { r := gitea.Review{ ID: id,