Skip to content
Draft
128 changes: 128 additions & 0 deletions command/install.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,128 @@
package command

import (
"fmt"
"log"
"path"
"regexp"
"strings"

"github.com/privateerproj/privateer-sdk/internal/install"
"github.com/privateerproj/privateer-sdk/internal/registry"
"github.com/spf13/cobra"
"github.com/spf13/viper"
)

var validNameSegment = regexp.MustCompile(`^[a-zA-Z0-9][a-zA-Z0-9._-]*$`)

// GetInstallCmd returns the install command that can be added to a root command.
func GetInstallCmd(writer Writer) *cobra.Command {
installCmd := &cobra.Command{
Use: "install [plugin-name]",
Short: "Install a vetted plugin from the registry.",
Long: "Resolve the plugin name to registry metadata, then download the plugin binary from the release URL into the binaries path.",
Args: cobra.ExactArgs(1),
RunE: func(cmd *cobra.Command, args []string) error {
pluginName := args[0]
return installPlugin(writer, pluginName)
},
}
return installCmd
}

func installPlugin(writer Writer, pluginName string) error {
client := registry.NewClient()

// Fetch the vetted list to validate the plugin name
vetted, err := client.GetVettedList()
if err != nil {
return fmt.Errorf("fetching vetted plugin list: %w", err)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not blocking: writer.Flush() is only called on the success path (line 78). If any error returns early, buffered output like "Fetching metadata..." may be lost. Idiomatic Go would use defer at the top of the function:

Suggested change
return fmt.Errorf("fetching vetted plugin list: %w", err)
func installPlugin(writer Writer, pluginName string) error {
defer func() { _ = writer.Flush() }()
client := registry.NewClient()

}

if !isVetted(vetted.Plugins, pluginName) {
return fmt.Errorf("plugin %q is not in the vetted plugin list", pluginName)
}

// Parse owner/repo from plugin name
owner, repo, err := parsePluginName(pluginName)
if err != nil {
return err
}

// Fetch plugin metadata
_, _ = fmt.Fprintf(writer, "Fetching metadata for %s/%s...\n", owner, repo)
pluginData, err := client.GetPluginData(owner, repo)
if err != nil {
return fmt.Errorf("fetching plugin data: %w", err)
}

// Determine download URL
downloadURL, err := resolveDownloadURL(pluginData)
if err != nil {
return err
}

destDir := viper.GetString("binaries-path")
binaryName := path.Base(pluginData.Name)
_, _ = fmt.Fprintf(writer, "Downloading %s to %s...\n", binaryName, destDir)

err = install.FromURL(downloadURL, destDir, binaryName)
if err != nil {
return fmt.Errorf("installing plugin: %w", err)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not blocking: if binaries-path is not configured, viper.GetString returns "". os.MkdirAll("", 0755) succeeds silently, and filepath.Join("", binaryName) resolves to just binaryName — writing the binary to the current working directory with no error.

Suggestion:

destDir := viper.GetString("binaries-path")
if destDir == "" {
	return fmt.Errorf("binaries-path is not configured")
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the behavior we want? If the user unsets this, it would likely be an intentional unset to target binaries in the working directory.

...Right? 🤔

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a fair point — I could see an argument either way. My concern is more about the accidental case: a first-time user who hasn't configured anything yet runs pvtr install and silently gets a binary dropped in their cwd. That's a confusing failure mode to debug.

If targeting the working directory is a legitimate use case, maybe make it explicit — e.g. require binaries-path: "." in the config rather than having "" silently resolve to cwd. That way it's an intentional choice, not a footgun. Either way, not blocking.

}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not blocking: path.Base(pluginData.Name) strips directory components but does not validate the resulting name. If the registry is compromised, an attacker could control the filename written to the binaries dir (e.g. .bashrc, .profile). Worth validating against a safe pattern like ^[a-zA-Z0-9][a-zA-Z0-9._-]+$ before writing.


_, _ = fmt.Fprintf(writer, "Successfully installed %s\n", pluginData.Name)
if err := writer.Flush(); err != nil {
log.Printf("Error flushing writer: %v", err)
}
return nil
}

// parsePluginName splits a plugin name into owner and repo.
// Accepts formats: "owner/repo" or just "repo" (assumes "privateerproj" as owner).
// Returns an error if the name is empty or contains path traversal characters.
func parsePluginName(name string) (owner, repo string, err error) {
name = strings.TrimSpace(name)
if name == "" {
return "", "", fmt.Errorf("plugin name must not be empty")
}
parts := strings.SplitN(name, "/", 2)
if len(parts) == 2 {
owner, repo = parts[0], parts[1]
} else {
owner, repo = "privateerproj", name
}
if !validNameSegment.MatchString(owner) {
return "", "", fmt.Errorf("invalid owner %q: must match %s", owner, validNameSegment.String())
}
if !validNameSegment.MatchString(repo) {
return "", "", fmt.Errorf("invalid repo %q: must match %s", repo, validNameSegment.String())
}
return owner, repo, nil
}

func isVetted(plugins []string, name string) bool {
name = strings.TrimSpace(name)
for _, p := range plugins {
if strings.TrimSpace(p) == name {
return true
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function can never return a non-nil error — every code path returns nil. The caller at line 51 checks if err != nil which is dead code. In Go, if a function cannot fail, it should not return an error — the signature misleads readers into thinking validation happens here when none does.

Suggestion: either drop the error return, or add actual validation (empty owner/repo, path traversal characters) so the error return earns its keep.

}
return false
}

// resolveDownloadURL determines the download URL for a plugin.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Asymmetric trimming — TrimSpace is applied to the registry list entries but not to the user-provided name. If a user passes a name with accidental whitespace (e.g. from copy-paste), the match fails silently.

Additionally, parsePluginName (line 86) does no validation, so input like "../../something" produces owner="../..", repo="something", which gets interpolated into the registry URL path.

Suggestion:

func isVetted(plugins []string, name string) bool {
	name = strings.TrimSpace(name)
	for _, p := range plugins {
		if strings.TrimSpace(p) == name {
			return true
		}
	}
	return false
}

And validate owner/repo in parsePluginName against something like ^[a-zA-Z0-9][a-zA-Z0-9._-]*$.

// If the plugin has a direct download URL, use that.
// Otherwise, infer from GitHub releases using the source and latest version.
func resolveDownloadURL(data *registry.PluginData) (string, error) {
if data.Download != "" {
return data.Download, nil
}
if data.Source == "" || data.Latest == "" {
return "", fmt.Errorf("plugin %s has no download URL and no source/version to infer one from", data.Name)
}
base := install.InferGitHubReleaseBase(data.Source, data.Latest)
binaryName := path.Base(data.Name)
artifact := install.InferArtifactFilename(binaryName)
return fmt.Sprintf("%s/%s", base, artifact), nil
}
243 changes: 243 additions & 0 deletions command/install_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,243 @@
package command

import (
"strings"
"testing"

"github.com/privateerproj/privateer-sdk/internal/registry"
)

func TestParsePluginName(t *testing.T) {
var tests = []struct {
name string
input string
expectedOwner string
expectedRepo string
expectError string
}{
{
name: "owner/repo format",
input: "ossf/pvtr-github-repo-scanner",
expectedOwner: "ossf",
expectedRepo: "pvtr-github-repo-scanner",
},
{
name: "repo only defaults to privateerproj",
input: "pvtr-example",
expectedOwner: "privateerproj",
expectedRepo: "pvtr-example",
},
{
name: "org with nested path",
input: "myorg/my-plugin",
expectedOwner: "myorg",
expectedRepo: "my-plugin",
},
{
name: "empty string",
input: "",
expectError: "must not be empty",
},
{
name: "whitespace only",
input: " ",
expectError: "must not be empty",
},
{
name: "path traversal",
input: "../etc/passwd",
expectError: "invalid owner",
},
{
name: "backslash",
input: "org\\repo",
expectError: "invalid repo",
},
{
name: "empty owner",
input: "/my-plugin",
expectError: "invalid owner",
},
{
name: "empty repo",
input: "myorg/",
expectError: "invalid repo",
},
{
name: "owner starts with dot",
input: ".hidden/repo",
expectError: "invalid owner",
},
{
name: "repo starts with hyphen",
input: "org/-repo",
expectError: "invalid repo",
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
owner, repo, err := parsePluginName(tt.input)

if tt.expectError != "" {
if err == nil {
t.Fatalf("expected error containing %q, got nil", tt.expectError)
}
if !strings.Contains(err.Error(), tt.expectError) {
t.Errorf("expected error containing %q, got: %v", tt.expectError, err)
}
return
}
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if owner != tt.expectedOwner {
t.Errorf("owner: got %q, expected %q", owner, tt.expectedOwner)
}
if repo != tt.expectedRepo {
t.Errorf("repo: got %q, expected %q", repo, tt.expectedRepo)
}
})
}
}

func TestIsVetted(t *testing.T) {
plugins := []string{"ossf/pvtr-github-repo-scanner", "privateerproj/pvtr-example", " spaced-name "}

var tests = []struct {
name string
search string
expected bool
}{
{name: "exact match", search: "ossf/pvtr-github-repo-scanner", expected: true},
{name: "another match", search: "privateerproj/pvtr-example", expected: true},
{name: "not in list", search: "unknown/plugin", expected: false},
{name: "empty string", search: "", expected: false},
{name: "trimmed match", search: "spaced-name", expected: true},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got := isVetted(plugins, tt.search)
if got != tt.expected {
t.Errorf("isVetted(plugins, %q) = %v, expected %v", tt.search, got, tt.expected)
}
})
}
}

func TestResolveDownloadURL(t *testing.T) {
var tests = []struct {
name string
data *registry.PluginData
expectURL string
expectError string
}{
{
name: "direct download URL",
data: &registry.PluginData{
Name: "ossf/pvtr-github-repo-scanner",
Download: "https://example.com/direct-download.tar.gz",
Source: "https://github.com/ossf/pvtr-github-repo-scanner",
Latest: "0.19.2",
},
expectURL: "https://example.com/direct-download.tar.gz",
},
{
name: "inferred from GitHub release",
data: &registry.PluginData{
Name: "ossf/pvtr-github-repo-scanner",
Source: "https://github.com/ossf/pvtr-github-repo-scanner",
Latest: "0.19.2",
},
expectURL: "https://github.com/ossf/pvtr-github-repo-scanner/releases/download/v0.19.2/pvtr-github-repo-scanner_",
},
{
name: "inferred from owner/repo source",
data: &registry.PluginData{
Name: "privateerproj/pvtr-example",
Source: "privateerproj/pvtr-example",
Latest: "1.0.0",
},
expectURL: "https://github.com/privateerproj/pvtr-example/releases/download/v1.0.0/pvtr-example_",
},
{
name: "missing source and download",
data: &registry.PluginData{
Name: "broken-plugin",
Latest: "1.0.0",
},
expectError: "no download URL and no source/version",
},
{
name: "missing version",
data: &registry.PluginData{
Name: "broken-plugin",
Source: "https://github.com/org/repo",
},
expectError: "no download URL and no source/version",
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got, err := resolveDownloadURL(tt.data)

if tt.expectError != "" {
if err == nil {
t.Fatalf("expected error containing %q, got nil", tt.expectError)
}
if !strings.Contains(err.Error(), tt.expectError) {
t.Errorf("expected error containing %q, got: %v", tt.expectError, err)
}
return
}
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
// For inferred URLs, we only check the prefix since the suffix depends on OS/arch
if tt.data.Download != "" {
if got != tt.expectURL {
t.Errorf("got %q, expected %q", got, tt.expectURL)
}
} else {
if !strings.HasPrefix(got, tt.expectURL) {
t.Errorf("got %q, expected prefix %q", got, tt.expectURL)
}
}
})
}
}

func TestContains(t *testing.T) {
plugins := []*PluginPkg{
{Name: "plugin-a"},
{Name: "plugin-b"},
{Name: "pvtr-github-repo-scanner"},
}

var tests = []struct {
name string
search string
expected bool
}{
{name: "found first", search: "plugin-a", expected: true},
{name: "found last", search: "pvtr-github-repo-scanner", expected: true},
{name: "not found", search: "missing-plugin", expected: false},
{name: "empty string", search: "", expected: false},
{name: "nil slice", search: "anything", expected: false},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
slice := plugins
if tt.name == "nil slice" {
slice = nil
}
got := Contains(slice, tt.search)
if got != tt.expected {
t.Errorf("Contains(plugins, %q) = %v, expected %v", tt.search, got, tt.expected)
}
})
}
}
Loading