fix(secrets): Prevent a secret leak scenario with --verbose

pkg/secrets/op_cli_secrets_provider.go

@@ -9,18 +9,17 @@ import (
 )
 
 // The OnePasswordCLISecretsProvider is an implementation of the SecretsProvider interface
-// It provides integration with the 1Password CLI for secret management with automatic shell scrubbing registration
-// It serves as a bridge between the application and 1Password's secure storage with built-in security features
-// It enables retrieval and parsing of secrets from 1Password vaults while automatically registering secrets for output scrubbing
+// It provides integration with 1Password CLI for secrets management with automatic shell scrubbing registration
+// It serves as a bridge between the application and 1Password CLI with built-in security features
+// It enables secure storage and retrieval of secrets using 1Password while automatically registering secrets for output scrubbing
 
 // =============================================================================
 // Types
 // =============================================================================
 
-// OnePasswordCLISecretsProvider is an implementation of the SecretsProvider interface
-// that uses the 1Password CLI to manage secrets.
+// OnePasswordCLISecretsProvider is a struct that implements the SecretsProvider interface using 1Password CLI.
 type OnePasswordCLISecretsProvider struct {
-	BaseSecretsProvider
+	*BaseSecretsProvider
 	vault secretsConfigType.OnePasswordVault
 }
 
@@ -31,7 +30,7 @@ type OnePasswordCLISecretsProvider struct {
 // NewOnePasswordCLISecretsProvider creates a new OnePasswordCLISecretsProvider instance
 func NewOnePasswordCLISecretsProvider(vault secretsConfigType.OnePasswordVault, injector di.Injector) *OnePasswordCLISecretsProvider {
 	return &OnePasswordCLISecretsProvider{
-		BaseSecretsProvider: *NewBaseSecretsProvider(injector),
+		BaseSecretsProvider: NewBaseSecretsProvider(injector),
 		vault:               vault,
 	}
 }
@@ -40,9 +39,9 @@ func NewOnePasswordCLISecretsProvider(vault secretsConfigType.OnePasswordVault,
 // Public Methods
 // =============================================================================
 
-// GetSecret retrieves a secret value for the specified key and automatically registers it with the shell for output scrubbing.
-// It uses the 1Password CLI to fetch secrets from the configured vault and automatically registers
-// the retrieved secret with the shell's scrubbing system to prevent accidental exposure in command output.
+// GetSecret retrieves a secret value for the given key using 1Password CLI. Registers the secret for shell scrubbing to
+// prevent exposure in output. Executes the CLI directly to avoid leaking secrets before registration. If locked, returns
+// a masked value. Key format: 'secret.field'. Returns the secret or error on failure.
 func (s *OnePasswordCLISecretsProvider) GetSecret(key string) (string, error) {
 	if !s.unlocked {
 		return "********", nil
@@ -54,8 +53,8 @@ func (s *OnePasswordCLISecretsProvider) GetSecret(key string) (string, error) {
 	}
 
 	args := []string{"item", "get", parts[0], "--vault", s.vault.Name, "--fields", parts[1], "--reveal", "--account", s.vault.URL}
-
-	output, err := s.shell.ExecSilent("op", args...)
+	cmd := s.shims.Command("op", args...)
+	output, err := s.shims.CmdOutput(cmd)
 	if err != nil {
 		return "", fmt.Errorf("failed to retrieve secret from 1Password: %w", err)
 	}

pkg/secrets/op_cli_secrets_provider_test.go

@@ -7,6 +7,8 @@ package secrets
 
 import (
 	"errors"
+	"os/exec"
+	"strings"
 	"testing"
 
 	secretsConfigType "github.com/windsorcli/cli/api/v1alpha1/secrets"
@@ -65,11 +67,12 @@ func TestOnePasswordCLISecretsProvider_GetSecret(t *testing.T) {
 		}
 		provider.unlocked = true
 
-		// And a mock shell that returns a successful result
-		mocks.Shell.ExecSilentFunc = func(command string, args ...string) (string, error) {
+		// And mock shims for command execution
+		mockShims := NewShims()
+		mockShims.Command = func(name string, args ...string) *exec.Cmd {
 			// Verify the command and arguments
-			if command != "op" {
-				t.Errorf("Expected command to be 'op', got %s", command)
+			if name != "op" {
+				t.Errorf("Expected command to be 'op', got %s", name)
 			}
 
 			// Check that the arguments contain the expected values
@@ -84,9 +87,13 @@ func TestOnePasswordCLISecretsProvider_GetSecret(t *testing.T) {
 				}
 			}
 
-			// Return a successful result
-			return "secret-value", nil
+			// Return a mock command
+			return &exec.Cmd{}
+		}
+		mockShims.CmdOutput = func(cmd *exec.Cmd) ([]byte, error) {
+			return []byte("secret-value"), nil
 		}
+		provider.shims = mockShims
 
 		// When GetSecret is called
 		value, err := provider.GetSecret("test-secret.password")
@@ -190,10 +197,15 @@ func TestOnePasswordCLISecretsProvider_GetSecret(t *testing.T) {
 		}
 		provider.unlocked = true
 
-		// And a mock shell that returns an error
-		mocks.Shell.ExecSilentFunc = func(command string, args ...string) (string, error) {
-			return "", errors.New("command execution error")
+		// And mock shims that return an error
+		mockShims := NewShims()
+		mockShims.Command = func(name string, args ...string) *exec.Cmd {
+			return &exec.Cmd{}
+		}
+		mockShims.CmdOutput = func(cmd *exec.Cmd) ([]byte, error) {
+			return nil, errors.New("command execution error")
 		}
+		provider.shims = mockShims
 
 		// When GetSecret is called
 		value, err := provider.GetSecret("test-secret.password")
@@ -203,10 +215,12 @@ func TestOnePasswordCLISecretsProvider_GetSecret(t *testing.T) {
 			t.Error("Expected an error, got nil")
 		}
 
-		// And the error message should be correct
-		expectedError := "failed to retrieve secret from 1Password: command execution error"
-		if err.Error() != expectedError {
-			t.Errorf("Expected error to be '%s', got '%s'", expectedError, err.Error())
+		// And the error message should contain the expected text
+		if !strings.Contains(err.Error(), "failed to retrieve secret from 1Password") {
+			t.Errorf("Expected error to contain 'failed to retrieve secret from 1Password', got '%s'", err.Error())
+		}
+		if !strings.Contains(err.Error(), "command execution error") {
+			t.Errorf("Expected error to contain 'command execution error', got '%s'", err.Error())
 		}
 
 		// And the value should be empty
@@ -236,10 +250,15 @@ func TestParseSecrets(t *testing.T) {
 		}
 		provider.unlocked = true
 
-		// And a mock shell that returns a successful result
-		mocks.Shell.ExecSilentFunc = func(command string, args ...string) (string, error) {
-			return "secret-value", nil
+		// And mock shims for command execution
+		mockShims := NewShims()
+		mockShims.Command = func(name string, args ...string) *exec.Cmd {
+			return &exec.Cmd{}
+		}
+		mockShims.CmdOutput = func(cmd *exec.Cmd) ([]byte, error) {
+			return []byte("secret-value"), nil
 		}
+		provider.shims = mockShims
 
 		// When ParseSecrets is called with standard notation
 		input := "This is a secret: ${{ op.test-id.test-secret.password }}"

pkg/secrets/shims.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"errors"
 	"os"
+	"os/exec"
 
 	"github.com/1password/onepassword-sdk-go"
 	"github.com/getsops/sops/v3/decrypt"
@@ -26,6 +27,8 @@ type Shims struct {
 	DecryptFile          func(string, string) ([]byte, error)
 	NewOnePasswordClient func(context.Context, ...onepassword.ClientOption) (*onepassword.Client, error)
 	ResolveSecret        func(*onepassword.Client, context.Context, string) (string, error)
+	Command              func(name string, arg ...string) *exec.Cmd
+	CmdOutput            func(cmd *exec.Cmd) ([]byte, error)
 }
 
 // =============================================================================
@@ -47,5 +50,9 @@ func NewShims() *Shims {
 			}
 			return client.Secrets().Resolve(ctx, secretRef)
 		},
+		Command: exec.Command,
+		CmdOutput: func(cmd *exec.Cmd) ([]byte, error) {
+			return cmd.Output()
+		},
 	}
 }