fix: redact access token in config command output

cmd/config/config.go

@@ -103,7 +103,7 @@ func run(service config.Service) func(*cobra.Command, []string) error {
 				return newErr(err.Error())
 			}
 
-			configJSON, err := json.Marshal(conf)
+			configJSON, err := json.Marshal(conf.Redacted())
 			if err != nil {
 				return newErr(err.Error())
 			}

internal/config/config.go

@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
+	"regexp"
 	"strconv"
 
 	"github.com/mitchellh/go-homedir"
@@ -16,6 +17,10 @@ import (
 
 const Filename = ".ldcli-config.yml"
 
+// RedactedValue is rendered in place of a sensitive configuration value. It is only ever
+// substituted into command output; the value stored in the config file is left untouched.
+const RedactedValue = "[REDACTED]"
+
 type ReadFile func(name string) ([]byte, error)
 
 // Config represents the data stored in the config file.
@@ -47,6 +52,37 @@ func New(filename string, readFile ReadFile) (Config, error) {
 	return c, nil
 }
 
+// Redacted returns a copy of the Config with sensitive values replaced by RedactedValue, for use
+// anywhere a Config is rendered to output. An unset sensitive value is left empty so that it stays
+// elided by omitempty rather than being reported as a value that is present but hidden.
+func (c Config) Redacted() Config {
+	if c.AccessToken != "" {
+		c.AccessToken = RedactedValue
+	}
+
+	return c
+}
+
+// configKeyPattern describes the shape of every supported configuration key: lowercase words
+// joined by single hyphens.
+var configKeyPattern = regexp.MustCompile(`^[a-z0-9]+(-[a-z0-9]+)*$`)
+
+// maxConfigKeyLength bounds what is treated as key-shaped. The longest supported key is well
+// under this, while an access token is well over it.
+const maxConfigKeyLength = 32
+
+// safeKeyForError returns key when it has the shape of a configuration key, and RedactedValue
+// otherwise. An unrecognized key is echoed back to help the user spot a typo, but the argument in
+// that position is not always a key: transposing `--set <key> <value>` puts the value there, and
+// for access-token that value is a secret. Anything that is not key-shaped is therefore elided.
+func safeKeyForError(key string) string {
+	if len(key) <= maxConfigKeyLength && configKeyPattern.MatchString(key) {
+		return key
+	}
+
+	return RedactedValue
+}
+
 // Update validates the updating fields and sets them on the Config. It returns the updated fields
 // in addition to the Config.
 func (c Config) Update(kvs []string) (Config, []string, error) {
@@ -59,7 +95,7 @@ func (c Config) Update(kvs []string) (Config, []string, error) {
 		// TODO: move this list to this package?
 		_, ok := cliflags.AllFlagsHelp()[kvs[i]]
 		if !ok {
-			return Config{}, updatedFields, errors.NewError(fmt.Sprintf("%s is not a valid configuration option", kvs[i]))
+			return Config{}, updatedFields, errors.NewError(fmt.Sprintf("%s is not a valid configuration option", safeKeyForError(kvs[i])))
 		}
 	}
 
@@ -107,7 +143,7 @@ func (c Config) Update(kvs []string) (Config, []string, error) {
 func (c Config) Remove(key string) (Config, error) {
 	_, ok := cliflags.AllFlagsHelp()[key]
 	if !ok {
-		return Config{}, errors.NewError(fmt.Sprintf("%s is not a valid configuration option", key))
+		return Config{}, errors.NewError(fmt.Sprintf("%s is not a valid configuration option", safeKeyForError(key)))
 	}
 
 	return c, nil

internal/config/config_test.go

@@ -1,6 +1,7 @@
 package config_test
 
 import (
+	"encoding/json"
 	"errors"
 	"net/http"
 	"testing"
@@ -10,6 +11,7 @@ import (
 	"gopkg.in/yaml.v3"
 
 	"github.com/launchdarkly/ldcli/internal/config"
+	"github.com/launchdarkly/ldcli/internal/output"
 	"github.com/launchdarkly/ldcli/internal/resources"
 )
 
@@ -149,3 +151,103 @@ func TestRemove(t *testing.T) {
 		assert.EqualError(t, err, "invalid is not a valid configuration option")
 	})
 }
+
+func TestRedacted(t *testing.T) {
+	t.Run("replaces a set access token", func(t *testing.T) {
+		c := config.Config{AccessToken: "test-access-token"}
+
+		redacted := c.Redacted()
+
+		assert.Equal(t, config.RedactedValue, redacted.AccessToken)
+	})
+
+	t.Run("leaves an unset access token empty so omitempty still elides it", func(t *testing.T) {
+		c := config.Config{Project: "test-project"}
+
+		redacted := c.Redacted()
+
+		assert.Equal(t, "", redacted.AccessToken)
+
+		configJSON, err := json.Marshal(redacted)
+		require.NoError(t, err)
+		assert.NotContains(t, string(configJSON), "access-token")
+	})
+
+	t.Run("leaves non-sensitive values alone", func(t *testing.T) {
+		optOut := true
+		c := config.Config{
+			AccessToken:     "test-access-token",
+			AnalyticsOptOut: &optOut,
+			BaseURI:         "http://test.com",
+			DevStreamURI:    "http://stream.test.com",
+			Environment:     "test-environment",
+			Flag:            "test-flag",
+			Output:          "json",
+			Project:         "test-project",
+		}
+
+		redacted := c.Redacted()
+
+		expected := c
+		expected.AccessToken = config.RedactedValue
+		assert.Equal(t, expected, redacted)
+	})
+
+	t.Run("does not mutate the receiver", func(t *testing.T) {
+		c := config.Config{AccessToken: "test-access-token"}
+
+		_ = c.Redacted()
+
+		assert.Equal(t, "test-access-token", c.AccessToken)
+	})
+}
+
+// TestRedactedOutput covers the rendering paths that `config --list` feeds the redacted Config
+// into, so that neither the plaintext nor the JSON representation can reveal the token.
+func TestRedactedOutput(t *testing.T) {
+	c := config.Config{
+		AccessToken: "test-access-token",
+		Project:     "test-project",
+	}
+
+	configJSON, err := json.Marshal(c.Redacted())
+	require.NoError(t, err)
+
+	for _, outputKind := range []string{"json", "plaintext"} {
+		t.Run(outputKind, func(t *testing.T) {
+			out, err := output.CmdOutputSingular(outputKind, configJSON, output.ConfigPlaintextOutputFn)
+
+			require.NoError(t, err)
+			assert.NotContains(t, out, "test-access-token")
+			assert.Contains(t, out, config.RedactedValue)
+			assert.Contains(t, out, "test-project")
+		})
+	}
+}
+
+// TestErrorDoesNotEchoNonKeyArguments covers the `--set`/`--unset` validation errors, which echo
+// back the argument they rejected. A transposed `--set <token> access-token` puts a secret in the
+// position a key was expected in, so only key-shaped arguments are echoed.
+func TestErrorDoesNotEchoNonKeyArguments(t *testing.T) {
+	const token = "api-2c2f9f1e-0b1a-4a3a-9d3f-000000000000"
+
+	t.Run("a key-shaped typo is still echoed", func(t *testing.T) {
+		_, _, err := config.Config{}.Update([]string{"projct", "test-project"})
+
+		assert.EqualError(t, err, "projct is not a valid configuration option")
+	})
+
+	t.Run("a transposed --set does not echo the value", func(t *testing.T) {
+		_, _, err := config.Config{}.Update([]string{token, "access-token"})
+
+		assert.EqualError(t, err, config.RedactedValue+" is not a valid configuration option")
+		assert.NotContains(t, err.Error(), token)
+	})
+
+	t.Run("--unset does not echo a non-key argument", func(t *testing.T) {
+		_, err := config.Config{}.Remove(token)
+
+		assert.EqualError(t, err, config.RedactedValue+" is not a valid configuration option")
+		assert.NotContains(t, err.Error(), token)
+	})
+}