Address code review findings: security hardening and quality improvements

action.yml

@@ -60,15 +60,25 @@ runs:
       env:
         GITHUB_TOKEN: ${{ inputs.token }}
         VERSION_FILES_YAML: ${{ inputs.version_files }}
+        INPUT_BUMP_TYPE: ${{ inputs.bump_type }}
+        INPUT_VERSION_FILE: ${{ inputs.version_file }}
+        INPUT_BASE_BRANCH: ${{ inputs.base_branch }}
+        INPUT_HELM_DOCS_ARGS: ${{ inputs.helm_docs_args }}
       run: |
+        # Validate bump_type to prevent injection
+        case "$INPUT_BUMP_TYPE" in
+          major|minor|patch) ;;
+          *) echo "Error: Invalid bump_type. Must be major, minor, or patch." >&2; exit 1 ;;
+        esac
+
         ARGS=(
-          --bump-type="${{ inputs.bump_type }}"
-          --version-file="${{ inputs.version_file }}"
-          --base-branch="${{ inputs.base_branch }}"
+          --bump-type="$INPUT_BUMP_TYPE"
+          --version-file="$INPUT_VERSION_FILE"
+          --base-branch="$INPUT_BASE_BRANCH"
         )
 
-        if [ -n "${{ inputs.helm_docs_args }}" ]; then
-          ARGS+=(--helm-docs-args="${{ inputs.helm_docs_args }}")
+        if [ -n "$INPUT_HELM_DOCS_ARGS" ]; then
+          ARGS+=(--helm-docs-args="$INPUT_HELM_DOCS_ARGS")
         fi
 
         if [ -n "$VERSION_FILES_YAML" ]; then

internal/files/path.go

@@ -0,0 +1,103 @@
+// Copyright 2025 Stacklok, Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package files
+
+import (
+	"fmt"
+	"os"
+	"path/filepath"
+	"strings"
+)
+
+// ValidatePath ensures a file path is safe and within the allowed base directory.
+// It prevents path traversal attacks by checking that the resolved path stays within bounds.
+// If basePath is empty, the current working directory is used.
+func ValidatePath(basePath, userPath string) (string, error) {
+	if userPath == "" {
+		return "", fmt.Errorf("path cannot be empty")
+	}
+
+	// Get absolute base path
+	if basePath == "" {
+		var err error
+		basePath, err = os.Getwd()
+		if err != nil {
+			return "", fmt.Errorf("getting working directory: %w", err)
+		}
+	}
+
+	absBase, err := filepath.Abs(basePath)
+	if err != nil {
+		return "", fmt.Errorf("resolving base path: %w", err)
+	}
+
+	// Clean and resolve the user path
+	cleanPath := filepath.Clean(userPath)
+
+	// Check for obvious path traversal attempts
+	if strings.HasPrefix(cleanPath, "..") || strings.Contains(cleanPath, "/../") {
+		return "", fmt.Errorf("path traversal detected in %q", userPath)
+	}
+
+	// Resolve the full path
+	var fullPath string
+	if filepath.IsAbs(cleanPath) {
+		fullPath = cleanPath
+	} else {
+		fullPath = filepath.Join(absBase, cleanPath)
+	}
+
+	// Get absolute path to handle any remaining relative components
+	absPath, err := filepath.Abs(fullPath)
+	if err != nil {
+		return "", fmt.Errorf("resolving path: %w", err)
+	}
+
+	// Ensure the resolved path is within the base directory
+	if !strings.HasPrefix(absPath, absBase+string(filepath.Separator)) && absPath != absBase {
+		return "", fmt.Errorf("path %q resolves outside allowed directory", userPath)
+	}
+
+	return absPath, nil
+}
+
+// ValidatePathRelative validates a path and returns it relative to the base directory.
+// This is useful when the relative path is needed for display or storage.
+func ValidatePathRelative(basePath, userPath string) (string, error) {
+	absPath, err := ValidatePath(basePath, userPath)
+	if err != nil {
+		return "", err
+	}
+
+	// Get absolute base path for relative calculation
+	if basePath == "" {
+		basePath, err = os.Getwd()
+		if err != nil {
+			return "", fmt.Errorf("getting working directory: %w", err)
+		}
+	}
+
+	absBase, err := filepath.Abs(basePath)
+	if err != nil {
+		return "", fmt.Errorf("resolving base path: %w", err)
+	}
+
+	relPath, err := filepath.Rel(absBase, absPath)
+	if err != nil {
+		return "", fmt.Errorf("calculating relative path: %w", err)
+	}
+
+	return relPath, nil
+}

internal/files/path_test.go

@@ -0,0 +1,203 @@
+// Copyright 2025 Stacklok, Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package files
+
+import (
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+)
+
+func TestValidatePath(t *testing.T) {
+	t.Parallel()
+
+	// Create a temp directory for testing
+	tempDir := t.TempDir()
+
+	tests := []struct {
+		name     string
+		basePath string
+		userPath string
+		wantErr  bool
+		errMsg   string
+	}{
+		{
+			name:     "valid relative path",
+			basePath: tempDir,
+			userPath: "subdir/file.txt",
+			wantErr:  false,
+		},
+		{
+			name:     "valid simple filename",
+			basePath: tempDir,
+			userPath: "file.txt",
+			wantErr:  false,
+		},
+		{
+			name:     "empty path",
+			basePath: tempDir,
+			userPath: "",
+			wantErr:  true,
+			errMsg:   "path cannot be empty",
+		},
+		{
+			name:     "path traversal with ..",
+			basePath: tempDir,
+			userPath: "../etc/passwd",
+			wantErr:  true,
+			errMsg:   "path traversal detected",
+		},
+		{
+			name:     "path traversal with multiple ..",
+			basePath: tempDir,
+			userPath: "../../etc/passwd",
+			wantErr:  true,
+			errMsg:   "path traversal detected",
+		},
+		{
+			name:     "path traversal in middle",
+			basePath: tempDir,
+			userPath: "foo/../../../etc/passwd",
+			wantErr:  true,
+			errMsg:   "path traversal detected",
+		},
+		{
+			name:     "absolute path outside base",
+			basePath: tempDir,
+			userPath: "/etc/passwd",
+			wantErr:  true,
+			errMsg:   "resolves outside",
+		},
+		{
+			name:     "valid nested path",
+			basePath: tempDir,
+			userPath: "deploy/charts/myapp/Chart.yaml",
+			wantErr:  false,
+		},
+		{
+			name:     "path with current dir reference",
+			basePath: tempDir,
+			userPath: "./file.txt",
+			wantErr:  false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			result, err := ValidatePath(tt.basePath, tt.userPath)
+
+			if tt.wantErr {
+				if err == nil {
+					t.Errorf("ValidatePath() expected error containing %q, got nil", tt.errMsg)
+					return
+				}
+				if !strings.Contains(err.Error(), tt.errMsg) {
+					t.Errorf("ValidatePath() error = %v, want error containing %q", err, tt.errMsg)
+				}
+				return
+			}
+
+			if err != nil {
+				t.Errorf("ValidatePath() unexpected error = %v", err)
+				return
+			}
+
+			// Verify result is within base directory
+			absBase, _ := filepath.Abs(tt.basePath)
+			if !strings.HasPrefix(result, absBase) {
+				t.Errorf("ValidatePath() result %q not within base %q", result, absBase)
+			}
+		})
+	}
+}
+
+func TestValidatePath_EmptyBasePath(t *testing.T) {
+	t.Parallel()
+
+	// Should use current working directory when base is empty
+	result, err := ValidatePath("", "file.txt")
+	if err != nil {
+		t.Errorf("ValidatePath() with empty base unexpected error = %v", err)
+		return
+	}
+
+	cwd, _ := os.Getwd()
+	expected := filepath.Join(cwd, "file.txt")
+	if result != expected {
+		t.Errorf("ValidatePath() = %q, want %q", result, expected)
+	}
+}
+
+func TestValidatePathRelative(t *testing.T) {
+	t.Parallel()
+
+	tempDir := t.TempDir()
+
+	tests := []struct {
+		name     string
+		basePath string
+		userPath string
+		want     string
+		wantErr  bool
+	}{
+		{
+			name:     "simple relative path",
+			basePath: tempDir,
+			userPath: "file.txt",
+			want:     "file.txt",
+			wantErr:  false,
+		},
+		{
+			name:     "nested relative path",
+			basePath: tempDir,
+			userPath: "subdir/file.txt",
+			want:     filepath.Join("subdir", "file.txt"),
+			wantErr:  false,
+		},
+		{
+			name:     "path traversal rejected",
+			basePath: tempDir,
+			userPath: "../file.txt",
+			wantErr:  true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			result, err := ValidatePathRelative(tt.basePath, tt.userPath)
+
+			if tt.wantErr {
+				if err == nil {
+					t.Error("ValidatePathRelative() expected error, got nil")
+				}
+				return
+			}
+
+			if err != nil {
+				t.Errorf("ValidatePathRelative() unexpected error = %v", err)
+				return
+			}
+
+			if result != tt.want {
+				t.Errorf("ValidatePathRelative() = %q, want %q", result, tt.want)
+			}
+		})
+	}
+}

internal/github/interfaces.go

@@ -0,0 +1,27 @@
+// Copyright 2025 Stacklok, Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package github
+
+import "context"
+
+// PRCreator defines the interface for creating release pull requests.
+// This interface enables mocking for testing purposes.
+type PRCreator interface {
+	// CreateReleasePR creates a new branch with the modified files and opens a PR.
+	CreateReleasePR(ctx context.Context, req PRRequest) (*PRResult, error)
+}
+
+// Ensure Client implements PRCreator.
+var _ PRCreator = (*Client)(nil)