feat: Add list and install commands by eddie-knight · Pull Request #191 · privateerproj/privateer-sdk

eddie-knight · 2026-04-09T14:09:46Z

install         Install a vetted plugin from the registry.
list            Consult the Charts! List all plugins that have been installed.

  $ pvtr -h install
Resolve the plugin name to registry metadata, then download the plugin binary from the release URL into the binaries path.

Usage:
  pvtr install [plugin-name] [flags]

$ pvtr install ossf/pvtr-github-repo-scanner
Fetching metadata for ossf/pvtr-github-repo-scanner...
Downloading pvtr-github-repo-scanner to /Users/home/.privateer/bin...
Successfully installed ossf/pvtr-github-repo-scanner

$ pvtr -h list
Consult the Charts! List all plugins that have been installed.

Usage:
  pvtr list [flags]

$ pvtr list
| Plugin                     | Installed  | In Current Config  |
| github-repo                | false      | true               |
| pvtr-github-repo-scanner   | true       | true               |

$ pvtr list -a
| Plugin                          | Installed  | In Current Config  |
| pvtr-github-repo-scanner        | true       | false              |
| revanite/azure-blob-storage     | false      | false              |
| ossf/pvtr-github-repo-scanner   | false      | false              |

$ pvtr list --installable
Plugins that can be installed:
  - revanite/azure-blob-storage
  - ossf/pvtr-github-repo-scanner

$ pvtr list --installed
| Plugin                     | Installed  | In Current Config  |
| pvtr-github-repo-scanner   | true       | false              |

Signed-off-by: Eddie Knight <knight@linux.com>

jmeridth

Review from @jmeridth — 10 findings across correctness, security, and Go idiomaticity. Details inline.

jmeridth · 2026-04-09T14:33:48Z

internal/install/url.go

+	if resp.StatusCode != http.StatusOK {
+		return fmt.Errorf("downloading %s: status %d", url, resp.StatusCode)
+	}
+


io.ReadAll(resp.Body) has no size cap. A compromised registry or MITM could serve a multi-gigabyte response and OOM the process. The same pattern repeats inside extractFromTarGz (line 68) and extractFromZip (line 85) where decompressed content is read — a gzip bomb vector.

Suggestion:

const maxDownloadSize = 500 << 20 // 500 MB body, err := io.ReadAll(io.LimitReader(resp.Body, maxDownloadSize))

And similarly wrap the io.ReadAll(tr) / io.ReadAll(rc) calls in the extract functions.

jmeridth · 2026-04-09T14:33:48Z

command/types.go

+	} else {
+		logger.Trace(fmt.Sprintf("Completed %s", serviceName))
+	}
+}


Two behavioral changes from the old closeClient in run.go worth confirming are intentional:

Kill order reversed — old code logged then killed; new code kills first. Probably fine since logging reads PluginPkg fields not client state, but wanted to flag it.

"Unexpected exit" guard dropped — the old code had three branches: success (Info), error (Error), and a defensive catch-all for !Successful && Error == nil → Error("unexpected exit..."). The new code only has two branches, so a plugin that finishes without setting either Successful or Error now silently logs Trace("Completed") instead of Error("unexpected exit"). This could mask bugs.

Suggestion — restore the third branch:

func (p *PluginPkg) closeClient(serviceName string, client *hcplugin.Client, logger hclog.Logger) { if p.Successful { logger.Info(fmt.Sprintf("Plugin for %s completed successfully", serviceName)) } else if p.Error != nil { logger.Error(fmt.Sprintf("Error from %s: %s", serviceName, p.Error)) } else { logger.Warn(fmt.Sprintf("Unexpected exit from %s with no error or success", serviceName)) } client.Kill() }

jmeridth · 2026-04-09T14:33:48Z

command/install.go

+	return false
+}
+
+// resolveDownloadURL determines the download URL for a plugin.


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._-]*$.

jmeridth · 2026-04-09T14:33:48Z

command/install.go

+	for _, p := range plugins {
+		if strings.TrimSpace(p) == name {
+			return true
+		}


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.

jmeridth · 2026-04-09T14:33:48Z

command/list.go

+		out = append(out, &PluginPkg{Name: vp.Name, Installable: true})
+	}
+	return out, nil
+}


Not blocking: remote, _ := fetchVettedPlugins() discards the error. When --all is passed and the registry is unreachable, the user sees only local plugins with no indication the remote list failed. Compare with writeInstallablePlugins which does surface the error.

In idiomatic Go, errors should be handled or intentionally logged, not silently discarded. At minimum:

remote, err := fetchVettedPlugins() if err != nil { log.Printf("Warning: could not fetch remote plugin list: %v", err) }

How would this work if the registry is intentionally unreachable (ie, offline / vpc / airgap) ?

Should we add the warning early, and then handle the alternate use case later?

Or should we add the warning after we have a mechanism for handling intentional isolation?

TODO: either way, we need an issue for the followup

Good call on the airgap case — a warning every time pvtr list -a runs offline would be noisy and annoying in that scenario.

I'd lean toward adding the warning now behind a check — something like: only warn if PVTR_REGISTRY_URL is set (i.e. the user has explicitly configured a registry) or if the default registry was expected to be reachable. Alternatively, a --offline flag or a config option like registry: disabled could suppress it.

But if that feels like scope creep for this PR, creating a follow-up issue works too. The core concern is just that users in connected environments shouldn't get silently partial results with no indication that remote data is missing.

jmeridth · 2026-04-09T14:33:48Z

command/install.go

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


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") }

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? 🤔

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.

jmeridth · 2026-04-09T14:33:48Z

command/install.go

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


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.

jmeridth · 2026-04-09T14:33:48Z

command/install.go

+	// 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)


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()

jmeridth · 2026-04-09T14:33:48Z

command/list.go

+// GetListCmd returns the list command with flags registered.
+// writerFn is called at command execution time, so the writer can be
+// initialized lazily (e.g. in a PersistentPreRun hook).
+func GetListCmd(writerFn func() Writer) *cobra.Command {


Not blocking: this is a breaking change to an exported function signature (Writer → func() Writer). The lazy init rationale is sound, just flagging that downstream consumers will need a coordinated update. The PR description mentions privateer#204 which likely handles this.

jmeridth · 2026-04-09T14:33:48Z

command/list.go

+			writer := writerFn()
+			if viper.GetBool("installable") {
+				writeInstallablePlugins(writer)
 			} else {


Not blocking: --installable, --installed, and --all are not mutually exclusive. If a user passes --installable --installed, installable silently wins. Cobra supports cmd.MarkFlagsMutuallyExclusive("installable", "installed", "all") which would give a clear error message instead.

eddie-knight · 2026-04-09T14:36:03Z

Thanks @jmeridth — You're right that this was opened for review way too soon. I'll do another round of polish and apply your feedback.

Signed-off-by: Eddie Knight <knight@linux.com>

… paths Signed-off-by: Eddie Knight <knight@linux.com>

Signed-off-by: Eddie Knight <knight@linux.com>

eddie-knight added 5 commits March 12, 2026 21:07

chore: update go-gemara to v0.0.2

d805e36

Signed-off-by: Eddie Knight <knight@linux.com>

feat: added list and install logic to fetch trusted plugins info

462312a

Signed-off-by: Eddie Knight <knight@linux.com>

feat: add list cmd flags

7917180

Signed-off-by: Eddie Knight <knight@linux.com>

install works now

df2f35d

Signed-off-by: Eddie Knight <knight@linux.com>

lazy load the list writer

1d1033e

Signed-off-by: Eddie Knight <knight@linux.com>

eddie-knight requested a review from a team as a code owner April 9, 2026 14:09

github-actions bot added the feature label Apr 9, 2026

eddie-knight added 3 commits April 9, 2026 09:11

go mod tidy

5ef36e6

Signed-off-by: Eddie Knight <knight@linux.com>

linting

24b72e9

Signed-off-by: Eddie Knight <knight@linux.com>

added basic tests

852b1fa

Signed-off-by: Eddie Knight <knight@linux.com>

jmeridth reviewed Apr 9, 2026

View reviewed changes

eddie-knight marked this pull request as draft April 9, 2026 14:35

eddie-knight added 4 commits April 9, 2026 10:08

exclusive flags: all|installed|installable

28d870a

Signed-off-by: Eddie Knight <knight@linux.com>

applied feadback from @jmeridth: return error in parsePluginName

d88a0c9

Signed-off-by: Eddie Knight <knight@linux.com>

applied feedback from @jmeridth: symmetrically TrimSpace and validate…

b050277

… paths Signed-off-by: Eddie Knight <knight@linux.com>

applied feedback from @jmeridth: restored three exit states

8bdfa56

Signed-off-by: Eddie Knight <knight@linux.com>

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

Conversation

eddie-knight commented Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmeridth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eddie-knight commented Apr 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

eddie-knight commented Apr 9, 2026 •

edited

Loading